Skip to content

server: fix deadlock in load_models() when erasing a finished download - #25358

Merged
ngxson merged 2 commits into
ggml-org:masterfrom
ServeurpersoCom:server/fix-deadlock-download
Jul 6, 2026
Merged

ngxson merged 2 commits into
ggml-org:masterfrom
ServeurpersoCom:server/fix-deadlock-download

Conversation

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

Overview

Fix models download deadlock / test on real CI

Additional information

Requirements

@ServeurpersoCom
ServeurpersoCom requested a review from a team as a code owner July 6, 2026 15:23
@github-actions github-actions Bot added the server label Jul 6, 2026
The download monitoring thread acquires the models mutex on its way out,
but load_models() joined it from the erase loop while holding that mutex.
Join it outside the lock via threads_to_join like the other monitoring
threads.
@ServeurpersoCom
ServeurpersoCom force-pushed the server/fix-deadlock-download branch from c333552 to 24b4814 Compare July 6, 2026 15:27
@ngxson

ngxson commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

it's a bit surprise that python script never trigger a timeout though, I'm still a bit skeptical if this is truly a problem in C++ code

@ServeurpersoCom

Copy link
Copy Markdown
Contributor Author

Python can't time out: make_request defaults to timeout=None, so the final GET /models waits forever.

Proof it hangs in C++: in the CI log, load_models() prints its entry line Loaded 7 cached model presets at t=4.39s but never its exit line Available models (...). A C++ function that logs its entry and never its exit is stuck in C++, whatever the client does.

It's the erase loop joining the DOWNLOADED monitoring thread while holding the models mutex, while that thread waits on the same mutex to clean up stopping_models -> deadlock. Add a 1s sleep before that thread's final lock_guard and it reproduces 100%.

@ServeurpersoCom

ServeurpersoCom commented Jul 6, 2026 •

Copy link
Copy Markdown
Contributor Author

He inserted a sleep to get a deterministic reproduction, and ggerganov had killed the C++ process to unblock the Python process.

We should re-run the CI a few times to be sure:
Also on my fork : https://github.com/ServeurpersoCom/llama.cpp/actions/runs/28803965044
A lot : https://github.com/ServeurpersoCom/llama.cpp/actions/workflows/server.yml

@ngxson

ngxson commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

hmm ok that makes sense. probably make_request should have a timeout of 10 minutes to avoid hanging the CI for whole 5 hours, would appreciate if you can add that

A hung server now fails the test after 10 minutes instead of stalling
the CI job for hours. Explicit timeouts are unchanged.
@ngxson
ngxson merged commit 9abce74 into ggml-org:master Jul 6, 2026
30 checks passed
satindergrewal pushed a commit to satindergrewal/llama.cpp that referenced this pull request Aug 12, 2026
ggml-org#25358)

* server: fix deadlock in load_models() when erasing a finished download

The download monitoring thread acquires the models mutex on its way out,
but load_models() joined it from the erase loop while holding that mutex.
Join it outside the lock via threads_to_join like the other monitoring
threads.

* server: add default timeout to test requests

A hung server now fails the test after 10 minutes instead of stalling
the CI job for hours. Explicit timeouts are unchanged.
zbrad pushed a commit to zbrad/llama.cpp that referenced this pull request Sep 10, 2026
ggml-org#25358)

* server: fix deadlock in load_models() when erasing a finished download

The download monitoring thread acquires the models mutex on its way out,
but load_models() joined it from the erase loop while holding that mutex.
Join it outside the lock via threads_to_join like the other monitoring
threads.

* server: add default timeout to test requests

A hung server now fails the test after 10 minutes instead of stalling
the CI job for hours. Explicit timeouts are unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants