server: fix model eviction race in router(#28698)

[ac-mmi]

Overview

Fixes #28698 by reserving req_count during ensure_model_ready() and preventing requests from treating stopping models as ready.

Added regression tests for:

• fast-path eviction before proxying
• proxying into a stopping model

tick(), pick_victim(), and LRU behavior are unchanged.

Additional information

Tests: unit/test_router.py — 16 passed.

Requirements

• I have read and agree with the contributing guidelines
• AI usage disclosure: YES — AI was used to assist with analysis, implementation, and debugging. I reviewed and am responsible for all submitted changes.

[marcodyga]

Pulled your fix, works on my end. Thank you for your work!

[pwilkin]

/bot review

[ggml-gh-bot]

Automated code review

Code review: PR #28913 - router model eviction race

The core approach (reserving req_count inside ensure_model_ready so tick()/pick_victim() cannot evict between the readiness check and the proxy) is sound, and the two regression tests exercise the right windows under LLAMA_SERVER_DEBUG_FAKE_TIMING. A few issues below.

Blocking

(point 1) Permanent hang in the new stopping-model wait loop - tools/server/server-models.cpp, top of ensure_model_ready (the stopping_models.count(name) block). The loop only breaks on UNLOADED or on entry erasure. If, while the waiter sleeps in cv.wait_for(200ms), the stopped child exits and another thread immediately loads a new instance (status goes UNLOADED -> LOADING -> LOADED before the waiter re-acquires the mutex), the waiter never sees UNLOADED again and polls forever - the model is loaded and serving while the request is stuck. For session POSTs should_stop is nullptr (see proxy_post), so the hang is unbounded and leaks a handler thread. Minimal fix: also break out when !stopping_models.count(name) - the main wait loop below already handles LOADING/LOADED. Note the ordering wrinkle: on_child_exit() erases from stopping_models before update_status() sets UNLOADED, so breaking purely on !stopping can race into proxying a dead-but-still-LOADED child; either break on !stopping && status != LOADED, or erase from stopping_models only after the status update in on_child_exit().

(point 2) Unrelated, undisclosed changes in this PR - common/common.cpp (async-backend error probe via decode/synchronize/re-decode in common_context_can_seq_rm) and tools/server/server-context.cpp (hard-fail init when the probe fails) have nothing to do with the router eviction race and are not mentioned in the PR description at all. Per the contributing guidelines this should be split into its own PR with its own issue and rationale. If it stays here, the description must cover it.

Will slow the review

(point 3) Reservation is lost across instance replacement - load() replaces mapping[name] with a fresh instance_t, which resets req_count to 0. In the wait loop's UNLOADED branch (did_load || saw_loading, non-failed), reserved stays true across the reload, so the if (!reserved) guard skips re-incrementing and the new instance serves the request with req_count == 0 - unprotected against pick_victim, i.e. the exact race this PR fixes, resurfacing after a respawn. The subsequent proxy cleanup can also decrement another request's count if one arrived meanwhile. The stop path is covered (the stopping_models branch releases first), but the spontaneous-exit path is not. Simplest fix: call release_locked() in that branch before re-queueing so the reservation is re-taken on the new instance's LOADING phase.

(point 4) Unload semantics change - a request that waits out an explicit /models/unload now re-spawns the model (the new join/tick/reload path in the UNLOADED branch). The new test asserts this, so it seems intended, but it means an admin unload can be immediately undone by pending traffic. This deserves a sentence in the PR description (or maintainer sign-off) since it changes admin-facing behavior.

(point 5) Inconsistent handling of the draft context probe - server-context.cpp hard-fails when ctx_tgt's probe fails, but the ctx_dft probe at line 1275 gets no equivalent check: a broken draft context still passes init and fails later with a confusing runtime error. Also, NO && llama_get_memory(ctx_tgt) != nullptr is an inference; consider having common_context_can_seq_rm distinguish "unsupported" from "probe failed" explicitly (separate return value or out-param) rather than reconstructing the reason at the call site.

(point 6) Test nits - tools/server/tests/unit/test_router.py: (a) test_router_fast_path_load_not_evicted_before_proxy sets server.log_path to a mkstemp file but never reads it, and the temp file is never removed - drop the dead setup or add cleanup; (b) in test_router_does_not_proxy_into_stopping_model the final assertion assert not isinstance(res.body, str) or ... is a no-op when the body is parsed JSON - a stronger pin would be to wait for the model to return to loaded (proving the reload path was taken rather than a lucky proxy into the dying child).

Nits

(point 7) SRV_ERR("%s", "failed to evaluate the context during initialization\n") does not say which context/probe failed; proxy_request's new "model name=X is stopping" error surfaces as a generic 500 where a retryable status would fit better. proxy_request still uses mapping[name] (operator[]) after a lock-free get_meta(), which can insert a ghost entry if a reload erases the name in between - pre-existing, but worth switching to find() while touching these lines.

This review was generated automatically by pi coding agent using zai-org/GLM-5.3. It may contain mistakes. Maintainers make the final call.

[ac-mmi]

CI is currently showing two failing checks. The NVIDIA WebGPU check is failing with a ggml_webgpu queue timeout, while the Ubuntu check is failing in test_vision_embeddings. The router-related tests are passing.

[pwilkin]

Please address the review points.

[ServeurpersoCom]

I ran my Fable 5.1 agent on it and it found things the bot didn't, but he validated the majority of what the bot found. It was too large and complex for me to review quickly :

Thanks for the fix. I reproduced both variants of #28698 locally and reviewed the diff against master. Confirming the two state bugs the automated review found, adding a third window it missed, plus a few notes.

Repro setup

Router with three small models, --models-max 1, LLAMA_SERVER_DEBUG_FAKE_TIMING=1 to widen the windows (load() sleeps 2s up front, proxy_request() sleeps 2s after the req_count++). Both variants are deterministic on master.

Variant A, request for A, then a request for B 2.1s later so B queues while A is still LOADING:

ensure_model: model name=A is not loaded, loading...
join: models_max reached, request for name=B queued at position 1
tick: evicting idle LRU name=A for a queued request
proxy_reques: proxying request to model A on port 52181
http client error: Could not establish connection   -> request A gets a 500

Variant B, A loaded and idle, request for B, then a request for A 50ms later:

tick: evicting idle LRU name=A for a queued request
run: instance name=A exited with status 0
proxy_reques: proxying request to model A on port 41765
http client error: Could not establish connection   -> request A gets a 500

The diagnosis in the PR description matches what the logs show, and the direction (make the model count as busy before the readiness check is released) is the right one.

Confirming the automated review

Its point 1 (permanent hang in the new stopping wait loop) is real. The loop only leaves on UNLOADED or on entry erasure, so if the child exits and another request reloads the model while the waiter sits in cv.wait_for(200ms), UNLOADED is never observed again and the waiter polls forever on a model that is loaded and serving. Session POSTs pass should_stop == nullptr, so nothing breaks the loop.

Its point 3 (reservation lost across instance replacement) is real too, and I checked the mechanism: load() does mapping[name] = std::move(inst) with a fresh instance_t, so req_count goes back to 0. In the UNLOADED branch of the wait loop, reserved stays true across the reload, the if (!reserved) guard skips the re-increment, and the new instance serves the request with req_count == 0. That is the exact race this PR closes, reappearing after a respawn. The proxy cleanup can then also decrement a count that belongs to another request.

Its point 2 (out of scope changes) is worth acting on: the common/common.cpp and tools/server/server-context.cpp hunks look like the content of your own #28242 that rode along on this branch.

One more window, not covered by either the PR or the bot

on_child_exit() erases from stopping_models under the lock, releases it, and only then calls update_status(UNLOADED), which takes the lock again. In between, the model is no longer stopping but still reports LOADED while its child is already gone. A request landing there passes the new stopping_models.count(name) check in proxy_request(), takes the reservation and proxies into a dead port, which is the original symptom. Erasing the stopping mark under the same lock that sets UNLOADED (for instance inside update_status()) closes it, and it also makes the new check in proxy_request() exact rather than best effort.

Design notes

The reservation is threaded through a bool * reserved_out, an occupy flag on proxy_request(), a commit_reserved lambda, a public release_occupancy() and two try/catch blocks in the routes. A move-only holder returned by ensure_model_ready() and consumed by proxy_request() carries the same information with the release in a destructor, which removes the catch blocks and makes the leak paths unreachable rather than handled. I have a working variant of that here if it is useful.

Taking the reservation during LOADING is what exposes it to the instance replacement described above. Taking it at the moment the model is observed ready, under the same lock that releases the queue entry, keeps the handoff atomic and leaves the protection during the load to the queue entry, which pick_victim() already honors. That also removes the need for the fast path to be special: if the load goes through join() plus an immediate try_claim(), the entry covers variant A and the branch disappears.

Re-spawning the model in the UNLOADED branch means an explicit /models/unload can be undone by a request that was already waiting. The new test asserts it, so it looks intended, but the PR description says behavior is unchanged and this is admin facing.

Tests

test_router_fast_path_load_not_evicted_before_proxy starts the second request only after the first model is observed loaded. On master the req_count++ follows the LOADED transition by microseconds, so I suspect this passes without the fix. Sending the second request while the first model is still LOADING is what makes tick() run the eviction while the first requester is still asleep in cv.wait_for(), and that fails on master every time.

The router preset already sets LLAMA_SERVER_DEBUG_FAKE_TIMING=1 for the whole suite, so the new 2s sleep in ensure_model_ready() stacks with the one in proxy_request(), which is not gated on occupy. That is 4s per autoloaded request across the router tests.

[ac-mmi]

@pwilkin I've addressed the review feedback. Could you please take another look?

[ac-mmi]

@ServeurpersoCom Thanks for the detailed review. I addressed the two state/race issues identified by the automated review, removed the unrelated changes from #28242, and also updated the stopping_models/UNLOADED transition so it is synchronized under the same mutex as you suggested for the additional race window. I also verified the relevant router regression tests locally.

[ac-mmi]

@ServeurpersoCom The remaining 3 CI checks (gpu-openvino-low-perf, gpu-vulkan-intel-linux, and gpu-vulkan-intel-windows) have been queued for ~21 hours and are still waiting for their self-hosted runners. All other CI checks have completed successfully.

Just flagging this in case the runners need attention.

[ngxson]

I will have a look on either this should be done along with an upcoming refactoring, or can be a separate fix on its own

[ServeurpersoCom]

Thanks for digging into this. I reproduced both variants on master with the script from the issue, which simply sends requests to two models at once, and fixed them in #29217 with two small targeted commits that reuse the existing request queue of ngxson's scheduler instead of adding a new reservation mechanism; the full server test suite passes with both. With AI-generated fixes answering AI reviews, the patch kept growing, and for concurrency code like this it was simpler to restart from zero than to review it.