Skip to content

server: temporary skip model downloading API test#25355

Merged
ngxson merged 1 commit into
ggml-org:masterfrom
ngxson:xsn/server_skip_router_download_test
Jul 6, 2026
Merged

server: temporary skip model downloading API test#25355
ngxson merged 1 commit into
ggml-org:masterfrom
ngxson:xsn/server_skip_router_download_test

Conversation

@ngxson

@ngxson ngxson commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Overview

Ref: #25043 (comment)

Temporary disable this test for now, to investigate in the future

Requirements

@ngxson ngxson requested a review from a team as a code owner July 6, 2026 13:35
@github-actions github-actions Bot added the server label Jul 6, 2026

@ggerganov ggerganov left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to reproduce the problem manually on the mac mini, but it never happens.

@ngxson ngxson merged commit bfdf581 into ggml-org:master Jul 6, 2026
10 checks passed
@ggerganov

Copy link
Copy Markdown
Member

@ngxson I killed the parent process manually, and this allowed the CI to continue and print the logs:

https://github.com/ggml-org/llama.cpp/actions/runs/28778682435/job/85328649929#step:7:869

These might help to track down the issue.

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

When analyzing the problem with Fable, we speculate with a high level of confidence regarding this:

root@pod:/mnt/workspace/git/llama.cpp# git diff
diff --git a/tools/server/server-models.cpp b/tools/server/server-models.cpp
index b0d957f1dc..fb765da9bb 100644
--- a/tools/server/server-models.cpp
+++ b/tools/server/server-models.cpp
@@ -523,6 +523,7 @@ void server_models::load_models() {

         // collect all threads to join in one pass while the lock is held:
         // - monitoring threads from just-unloaded models (to_unload)
+        // - threads of finished downloads (DOWNLOADED), they acquire the mutex on exit
         // - threads of already-UNLOADED models that are being removed from source
         std::vector<std::thread> threads_to_join;
         for (const auto & name : to_unload) {
@@ -535,6 +536,13 @@ void server_models::load_models() {
             if (inst.meta.status == SERVER_MODEL_STATUS_DOWNLOADING) {
                 continue; // downloading models are not from config sources, leave them alone
             }
+            if (inst.meta.status == SERVER_MODEL_STATUS_DOWNLOADED) {
+                // joining this thread under the lock deadlocks: it locks the mutex on its way out
+                if (inst.th.joinable()) {
+                    threads_to_join.push_back(std::move(inst.th));
+                }
+                continue;
+            }
             if (final_presets.find(name) == final_presets.end() && !inst.meta.is_running() && inst.th.joinable()) {
                 threads_to_join.push_back(std::move(inst.th));
             }
@@ -550,10 +558,8 @@ void server_models::load_models() {
             if (it->second.meta.status == SERVER_MODEL_STATUS_DOWNLOADING) {
                 ++it; // download thread is still busy, skip
             } else if (it->second.meta.status == SERVER_MODEL_STATUS_DOWNLOADED) {
-                // download finished, safe to erase
-                if (it->second.th.joinable()) {
-                    it->second.th.join();
-                }
+                // download finished, thread is joined above, safe to erase
+                GGML_ASSERT(!it->second.th.joinable());
                 it = mapping.erase(it);
             } else if (final_presets.find(it->first) == final_presets.end()) {
                 SRV_INF("(reload) removing model name=%s (no longer in source)\n", it->first.c_str());
root@pod:/mnt/workspace/git/llama.cpp#

@ServeurpersoCom

Copy link
Copy Markdown
Contributor

He even yelled at me when I told him it was speculation 🤣

Here you go, faithful translation:


Fabulous, and it's no fable: we have better than speculation, we have a deterministic A/B proof. Your comment undersells the fix's real value. What we established on the pod:

  • deadlock reproduced 100%: 1.5s sleep injected before the monitor thread's exit lock_guard + emulation of the CI condition (final_presets contains the model) -> GET /models times out, server unkillable except by force kill, exactly Georgi's symptom
  • same setup with the fix -> GET responds, zero hang
  • the signature in his own CI log: orphaned Loaded 7 cached model presets at t=4.39s (7 = 6 + the freshly downloaded model, the proof that final_presets contains it), then neither Available models nor instance ... exited with status, both threads dead
  • the intermittence explained: race window [child EOF -> lock at line 995] versus [GET -> lock inside load_models], microseconds on Georgi's mac mini, hundreds of ms on a loaded macOS runner

The two stacks for the comment:

  • thread A (HTTP worker): GET /models -> get_all_meta -> need_reload -> load_models -> erase loop -> th.join() at line 555 while holding mutex
  • thread B (monitor of the download child): lock_guard(mutex) at line 995 (stopping_models.erase + cv_stop.notify_all) -> blocks
  • A joins B, B waits on A. The comment at line 543 states the rule, line 555 violates it. The comment at line 1010 ("cleaned up on next load_models") wrongly assumes the download thread no longer touches the mutex.

Bonus: your diff allows removing both @pytest.mark.skip in the same PR, the CI becomes the non-regression test again.

Want me to write the follow-up in English with the two stacks, the log signature, and the repro protocol (sleep + presets emulation), ready to paste under your comment to turn "speculate" into "confirmed with deterministic repro"? Nothing to touch on the pod, everything is already in its final state.


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.

3 participants