server: fix deadlock in load_models() when erasing a finished download (#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.

diff --git a/tools/server/server-models.cpp b/tools/server/server-models.cpp
index 2ee318f14469a0695e84302cc7f42f54a931b37d..6a8eb2a2be184f14e6b57611e5e8ccbc929045e5 100644 (file)
--- 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());

diff --git a/tools/server/tests/unit/test_router.py b/tools/server/tests/unit/test_router.py
index cdd6f531441e4ee64be58e244cae43d3e8204970..94165e520e6f6531fae8116de9c844feccc3ac85 100644 (file)
--- a/tools/server/tests/unit/test_router.py
+++ b/tools/server/tests/unit/test_router.py
@@ -314,7 +314,6 @@ def _wait_for_sse_event(collected: list, event_type: str, model: str, timeout: i
     return False
 
 
-@pytest.mark.skip(reason="sse_thread sometimes hangs on GH actions, to be investigated")
 def test_router_download_model():
     """Case 1: download a model, verify SSE events and GET /models."""
     global server
@@ -358,7 +357,6 @@ def test_router_download_model():
     assert MODEL_DOWNLOAD_ID in ids, f"{MODEL_DOWNLOAD_ID} not found in /models after download"
 
 
-@pytest.mark.skip(reason="sse_thread sometimes hangs on GH actions, to be investigated")
 def test_router_delete_model():
     """Case 2: delete the downloaded model, verify it disappears from GET /models."""
     global server

diff --git a/tools/server/tests/utils.py b/tools/server/tests/utils.py
index 63a959449eb6a9e192d3bcf4ca47c5a6818c6e12..67d7d20dbdc7ed83efa719b1337307fa7b99076d 100644 (file)
--- a/tools/server/tests/utils.py
+++ b/tools/server/tests/utils.py
@@ -31,6 +31,9 @@ import wget
 
 DEFAULT_HTTP_TIMEOUT = 60
 
+# per-request timeout, a hung server fails the test instead of stalling the CI for hours
+DEFAULT_REQUEST_TIMEOUT = 600
+
 
 class ServerResponse:
     headers: dict
@@ -330,7 +333,7 @@ class ServerProcess:
         path: str,
         data: dict | Any | None = None,
         headers: dict | None = None,
-        timeout: float | None = None,
+        timeout: float | None = DEFAULT_REQUEST_TIMEOUT,
     ) -> ServerResponse:
         url = f"http://{self.server_host}:{self.server_port}{path}"
         parse_body = False
@@ -389,7 +392,7 @@ class ServerProcess:
         path: str,
         data: dict | None = None,
         headers: dict | None = None,
-        timeout: float | None = None,
+        timeout: float | None = DEFAULT_REQUEST_TIMEOUT,
     ) -> dict:
         stream = data.get('stream', False)
         if stream: