Issue 2960 router llm pool sizing

[QuManu]

Summary

Fixes #2960

The router's LLM pool shares one max_loaded_models slot count with every model type, so a policy alternating between several local candidates reloads one on every switch. This raises the LLM pool's capacity to fit the active policy's local candidates, with a llm_pool_autosize off-switch (on by default) to fall back to today's behavior.

Scope

• This PR addresses one clear issue or change.
• I reviewed the full diff myself before submitting.
• I removed unrelated local changes.
• I kept refactoring separate unless it is required for this change.

Testing

• Code builds without errors locally.
• I tested this change locally.
• I described the testing performed below.

Testing details:
Full C++ CI suite passes (67/67, ctest -L cpp-ci). Router e2e suite passes live against a built server (18/18, 1 pre-existing unrelated skip), including new tests for PID stability while alternating candidates, correct union-not-sum floor aggregation across overlapping policies, cloud-candidate exclusion, and the off-switch restoring the old reload behavior.

Documentation

• Documentation is affected and has been updated.

Breaking Changes

• This PR does not introduce breaking changes.

Worth flagging anyway: llm_pool_autosize defaults to on, so existing installs with a multi-candidate local router policy will use more memory after upgrading, since the pool no longer thrashes them down to one. auto_evict is off by default too, so there's no automatic VRAM backstop out of the box — a warning gets logged when that combination happens, but it's worth a release-note mention.

[SlawomirNowaczyk]

Nice work overall. A few improvement suggestions:

1. /health reads three snapshots under three different locks — the payload can be internally inconsistent. In handle_health, max_models.llm comes from get_max_model_limits() (takes load_mutex_), candidate_floor from router_->llm_candidate_floor() (re-takes load_mutex_), and policies from llm_candidate_floor_info_mutex_. A reconcile landing between these acquisitions can produce a response where max_models.llm, candidate_floor, and the per-policy sum disagree — which is precisely the "the two never disagree" invariant the comments claim to guarantee. Consider snapshotting the floor + limit once under a single load_mutex_ hold, and folding the per-policy map into the same struct so one lock order produces one coherent view.

2. Confirm get_max_model_limits() newly acquiring load_mutex_ can't deadlock. This method was previously lock-free and is now taking load_mutex_. Please double-check no caller invokes it while already holding load_mutex_ (e.g. from within another Router method under lock) — that would be a self-deadlock on a non-recursive mutex. A quick grep of the call sites in the PR description would put this to rest.

3. The catch (const std::exception&) { continue; } in active_policy_llm_candidate_floor() silently shrinks the floor. If a candidate is transiently unresolvable, it's dropped from the union with no trace — which reintroduces exactly the reload-thrash this PR fixes, but invisibly and only sometimes. A LOG(DEBUG, ...) on the swallowed candidate would make that failure diagnosable instead of a silent regression.

4. Per-policy counts aren't de-duplicated within a policy. local_llm_count increments once per candidate in route_policy->candidates with no dedup, while the union (result.models) does dedup via the set. A policy that lists the same candidate twice will report an inflated policies[...] number in /health that's larger than its true distinct-local count (and larger than its contribution to the floor). Dedup per policy (e.g. a local std::set) so the breakdown and the union use the same counting rule.

5. The auto_evict-disabled warning fires on every reconcile, not on change. reconcile_llm_candidate_floor logs the "no VRAM backstop" WARNING whenever floor > max_loaded && !auto_evict, and reconcile runs on every models-changed notification — so unrelated model edits will re-emit the same warning repeatedly. Gate it on an actual floor transition (only warn when floor changed, or when it crosses max_loaded) to avoid log spam.

6. I believe we must be much more explicit about the memory-consumption consequences — in release notes and at runtime. With llm_pool_autosize defaulting to true and auto_evict defaulting to false, max_loaded_models silently stops being a hard cap on LLM memory for anyone with a multi-candidate local policy. A user who set max_loaded_models: 1 specifically to bound GPU/VRAM will, after upgrading, find the LLM pool holding N models with no backstop — an unbounded-memory surprise from a version bump, not a config change they made. This deserves loud, layered communication rather than a single line in the PR body:

• Release notes / CHANGELOG: call this out under a dedicated "Behavior change — may increase memory use after upgrade" heading (not buried under "enhancements"). State plainly: the LLM residency pool can now exceed max_loaded_models; to restore a hard cap set llm_pool_autosize: false; to keep autosize but bound memory set auto_evict: true. Include the exact keys so it's copy-pasteable.
• Startup log (once): when the effective LLM floor is raised above max_loaded_models, emit a single INFO at startup summarizing it — e.g. LLM pool auto-sized to N (max_loaded_models=1) to fit router policy candidates; set llm_pool_autosize=false to disable. This is distinct from the per-reconcile WARNING in suggestion #5, which should stay change-gated.
• First-run-after-upgrade notice: since the risky state only exists for users upgrading into a raised floor with auto_evict off, consider a one-time notice keyed off a config/version marker (e.g. a config_version bump or a persisted "seen" flag), so the warning fires once on the first lemond start after upgrade rather than every boot. Gate it on the actual dangerous combination (floor > max_loaded_models && !auto_evict) so users who don't hit it never see noise.
• /health surfacing: the llm_pool_autosize block already reports candidate_floor — consider adding an explicit boolean like exceeds_max_loaded_models (or an advisory string) so a UI/tray client can show "LLM pool is above your configured limit; no VRAM backstop" without re-deriving it. That makes the consequence visible in the product, not just the logs. Possibly even include information on how to revert to previous behavior, e.g., "set llm_pool_autosize=false to disable"

The goal is that nobody discovers this by watching VRAM climb — they should be told once, clearly, at the moment it becomes true, with the exact off-switch in hand.

[fl0rianr]

Thanks for the fixes. I still see a few things that should be tackled before approval:

1. Disabling autosize / lowering the floor does not actually restore the strict limit for an already populated pool. ensure_residency_capacity() evicts exactly one model and then admits the new one. So if autosize left e.g. 3 LLMs resident and the limit becomes 1, the next admission goes 3 -> 2 -> 3 and never converges back to 1. Even more importantly, alternating between two already-loaded candidates never reaches the capacity check at all, so switching llm_pool_autosize=false does not restore the old thrashing behavior. test_609 currently hides this by calling unload_all_models() after toggling the setting. Please test the real transition with both candidates already resident and enforce the reduced ceiling correctly.

2. The generation guard still does not cover the /health snapshot. reconcile_llm_candidate_floor() correctly rejects an older generation, but the callback unconditionally writes llm_candidate_floor_info_ afterwards. An older callback can therefore lose the Router reconcile race but still overwrite a newer /health snapshot. That is not just a narrow temporary consistency window — it can remain stale until the next registry change. The floor + diagnostic snapshot should be published with the same generation guard.

3. The warning still ignores llm_pool_autosize. With autosize explicitly disabled, floor > max_loaded_models && !auto_evict still logs that the "LLM pool floor raised", although that floor is not being applied. Please include the autosize state in that condition.

4. I also still agree that the memory behavior change needs explicit surfacing. With autosize default-on and auto_evict default-off, max_loaded_models is no longer a hard LLM residency cap after upgrade. A clear release-note/runtime notice with the exact llm_pool_autosize=false escape hatch would avoid users discovering this through increased VRAM use.

Small observability nit: lemonade_max_loaded_models is documented as the configured limit, but for LLM it now reports the effective autosized limit. The metric/help text should distinguish configured vs effective capacity.

The overall approach looks good; the first two are the main correctness blockers for me.

[QuManu]

Thanks @SlawomirNowaczyk for checking this so carefully. I fixed most of it:

1. Fixed. /health now reads candidate_floor and policies from one place, under one lock. So they can't show different, mismatched numbers anymore. Note: candidate_floor will still be a different number than max_models.llm when llm_pool_autosize is off, that's on purpose, not a bug. candidate_floor is meant to always show the real count, even when the switch is off.

2. Fixed. If a candidate can't be found, we now log a warning that says which policy and which candidate. Before, it just failed quietly.

3. Fixed. If a policy lists the same model twice, we don't count it twice anymore.

4. Fixed. The warning only shows again if the number actually changes. Before, it kept repeating on every unrelated change.

About your deadlock question (point 2): I checked it again. The two places that call get_max_model_limits() never hold the lock first. And every other place that touches llm_candidate_floor_ already holds the lock. So there's no way to deadlock here.

About your last point on telling users about the memory change: I agree, and it matters even more now . @fl0rianr's review caught that disabling the switch didn't actually reclaim memory until this next round of fixes, worth a release-note line, but that's a release-process thing rather than something for this diff.

[QuManu]

@fl0rianr thank you for the review. All four are fixed now:

1. Fixed. Added a new function that actively frees up LLM slots when the limit goes down instead of waiting for new models to load one at a time. It runs both when a policy loses candidates, and when someone changes llm_pool_autosize or max_loaded_models through /internal/set. That second case matters because before, changing those settings didn't do anything until a new model tried to load. Added two tests that check both cases work.

2. Fixed. The /health cache now checks the "generation" number before saving new data, same as the Router already does. Good catch before this, an older update could overwrite a newer one.

3. Fixed. The warning now also checks if llm_pool_autosize is actually on. Before, it could show up even when the setting was off and doing nothing.

4. Fixed. The lemonade_max_loaded_models metric description now says "effective" instead of "configured", since that's what it reports now.

About telling users (same point you and @SlawomirNowaczyk raised): agreed. And it's more important now that the first fix above is really working. The switch actually frees memory right away now. Still think release notes is the right place for that, not this PR.

[fl0rianr]

Thanks, this is much closer now. The previous blockers are addressed, but I still see two correctness issues before approval:

1. In-flight load race: capacity is checked before the slow backend load, then load_mutex_ is released. If the floor drops or llm_pool_autosize is disabled during that load, the new enforcer only sees already-resident models. When the load finishes, the new server is committed without re-checking the current effective limit, so the pool can immediately end up over-limit again. Please revalidate capacity after re-acquiring the lock and before adding the loaded server.

2. Busy LRU can stall convergence: enforce_llm_pool_capacity_locked() picks the LRU without excluding busy models. evict_server(..., EVICTION_TIMEOUT) can then wait 5s, fail because the model is still busy, and the loop may pick the same busy LRU again. That can block under load_mutex_ for up to N×5s while idle models could have been evicted instead. The shrink path should skip/defer busy candidates and continue with other evictable residents.

Two smaller follow-ups:

• max_loaded_models and llm_pool_autosize are still documented as deferred/no-immediate-side-effect keys, but they now trigger immediate eviction.
• The no-VRAM-backstop warning is only evaluated during floor reconcile, so live config changes like lowering max_loaded_models or re-enabling autosize can enter the risky state without emitting it.

Overall the design looks solid now; the in-flight-load race is the remaining blocker for me.

[SlawomirNowaczyk]

Previous issues are now fixed, but a few more emerged:

1. /internal/set HTTP handler can now block for up to global_timeout (600s) on an unrelated in-flight load. apply_config_side_effects() runs on the request thread (server.cpp:7124) and calls enforce_llm_pool_capacity(), which unconditionally load_cv_.wait(...)s for !is_loading_. A config set max_loaded_models=… (or llm_pool_autosize) issued while a slow model is loading will hang the client for the whole load. This is a new blocking path on the request thread — consider a bounded wait, or scheduling the enforce asynchronously and returning immediately.

2. Docs still label max_loaded_models a "Deferred key … no immediate side effect" — that's now false. getting-started.md:651 and runtime.md:177 both list it under "Deferred keys (affect the next model load or eviction decision, no immediate side effect)", and llm_pool_autosize was added to that same table. Both keys now trigger immediate eviction via apply_config_side_effects. This is fl0rianr's still-open follow-up — move them out of the deferred table (or correct the wording) so users know these take effect live and can evict.

3. Load-path re-validation only single-evicts, so full convergence depends on a second actor. The post-load ensure_residency_capacity evicts exactly one resident; if the effective limit shrank by more than one during the load, the load path alone leaves the pool over-limit. It's only correct because every limit-lowering path (reconcile_llm_candidate_floor, apply_config_side_effects) separately runs enforce_llm_pool_capacity after is_loading_ clears. That's a real invariant but it's implicit — either converge fully in the re-validation, or add a comment/assert documenting that every limit-lowering path must call enforce_llm_pool_capacity, so a future caller that forgets doesn't silently leave the pool over-limit.

4. Stale rationale comment in set_models_changed_callback. The comment still says the floor reconcile is "a cheap lock-and-assign" ordered first so it "shouldn't sit behind" the blocking helper reconcile — but after 41e0bbe the floor reconcile also load_cv_.waits and can block just as long. The ordering justification no longer holds; update it (exactly the comment-rot the repo guidelines call out).

5. All-busy case still holds load_mutex_ for up to N×EVICTION_TIMEOUT. The idle-preference fixes the common path, but when every LLM resident is busy, enforce_llm_pool_capacity_locked falls back to the busy LRU and can block ~N×5s under load_mutex_, stalling all inference. It's bounded now (no infinite loop), but consider deferring the shrink when no idle candidate exists rather than blocking on busy evicts.

6. Memory-behavior surfacing is still deferred to "release process." Both reviewers flagged that with autosize on + auto_evict off, max_loaded_models silently stops being a hard LLM cap after upgrade. This still remains to be done. At minimum the CHANGELOG/release-note line with the llm_pool_autosize=false escape hatch must land with this PR, not after — otherwise upgraders discover it by watching VRAM climb.

[QuManu]

@SlawomirNowaczyk thanks, all six addressed:

1. Fixed. Changing llm_pool_autosize or max_loaded_models through /internal/set now runs the cleanup on a background thread instead of the request thread. Before, if a model happened to be mid-load, the HTTP request would just sit there waiting for that load to finish, sometimes for a long time. Every other setting in that endpoint already returns right away, this one should too.

2. Fixed. Updated the docs max_loaded_models and llm_pool_autosize were listed as "deferred" (no immediate effect), but now they do have an immediate effect, so moved them to the other table.

3. Fixed. After a slow model load finishes, we now do the full cleanup pass, not just free up one slot. If the limit dropped by more than one while the load was running, one slot wasn't enough. Added a test that actually loads real models and races a load against a dropping limit to prove this works.

4. Fixed the outdated comment. It used to say the floor check was "cheap" and wouldn't block, that's no longer true since the last fix made it wait too.

5. Fixed. If every currently loaded model is busy, we now just stop and try again later instead of waiting on one of them. Waiting there would have blocked the lock that basically everything else in the router needs, so that's worse than doing nothing for now.

6. Checked docs/dev/release.md. There is no changelog file to edit in a PR, release notes get auto-generated later from the PR description. So this already lives in the right place: the "Breaking Changes" section above.

[SlawomirNowaczyk]

Looks mostly good, but the async fix introduced a new problem:

The detached enforce thread is a use-after-free hazard on shutdown.

std::thread([this]() { router_->enforce_llm_pool_capacity(); }).detach();

This captures this, and enforce_llm_pool_capacity() load_cv_.wait(...)s until load quiescence — potentially for a long in-flight load. Nothing couples that detached thread's lifetime to Router/Server teardown. If ~Server (which owns router_ as a unique_ptr) runs while the thread is still blocked in or entering enforce_llm_pool_capacity(), router_ is destroyed out from under it → UAF/crash on shutdown. The wait predicate also doesn't consult reclaim_shutdown_, so it can wake into destroyed state. This is exactly the teardown race the existing reclaim_shutdown_ machinery was built to prevent, and it's out of step with the sibling routing-helper reconcile, which runs on the managed model-manager notification thread rather than a raw detach(). Please give this a tracked lifetime — run it on the existing notification/worker thread, or join it during shutdown, or gate the wait on reclaim_shutdown_ and guarantee the thread is quiesced before ~Router.

Two smaller things also worth fixing:

1. A config-driven shrink that defers (all-busy) is never retried. When enforce_llm_pool_capacity_locked breaks because every LLM is busy, the pool stays over the configured limit and only reconverges "on the next admission or reconcile." For the headline use case — a user lowering max_loaded_models to bound VRAM — the cap can silently not take effect until unrelated traffic arrives. Consider a bounded re-arm (retry after the busy models idle) rather than relying on the next incidental event.

2. If a client (or a script, or a UI slider) sends several updates in quick succession, the code spawns unbounded detached threads. They serialize on load_mutex_ and each reads config live, so the end state is correct. Elsewhere in this same PR, however, the floor-reconcile path already solved exactly this class of problem with a generation counter: each policy-change notification carries an increasing generation, and reconcile_llm_candidate_floor ignores any call whose generation is older than the last one applied:
   if (generation <= last_llm_floor_generation_) { return; } // stale, skip
   This way stale/superseded work is dropped instead of executed. The config-driven enforce_llm_pool_capacity() path has no equivalent, so every toggle runs to completion even when a newer toggle has already superseded it.

[QuManu]

@SlawomirNowaczyk good catches, especially #.1 fixed all three:

1. Fixed. The detached thread wasn't tied to server shutdown, so a shutdown could crash mid use of a Router that was already gone. Now it runs through the same tracked thread system the server already waits on before shutting down. Tested with 20 rapid config changes + immediate shutdown: clean exit every time.

2. Fixed. Hooked into the existing 5-second background check, so a deferred cleanup retries automatically once something goes idle.

3. Fixed. A cleanup already running just gets flagged to rerun instead of spawning a new thread.

(Sorry for the delay. Committed locally for a bit before pushing.)

[SlawomirNowaczyk]

Thanks. And I found two more, this time minor, things worth fixing:

1. Latent lock-ordering hazard (worth a small refactor). request_llm_pool_enforcement() holds llm_pool_enforce_mutex_ and then acquires background_sync_mutex_ (for the reap + push_back). Meanwhile ~Server holds background_sync_mutex_ while joining the worker, and the worker needs llm_pool_enforce_mutex_ to finish its loop. That's a 3-way cycle (request-thread: enforce→sync; ~Server: holds sync, waits worker; worker: waits enforce). It's only safe because stop() is expected to drain HTTP handler threads before the join loop runs — i.e. it relies on no request_llm_pool_enforcement() being in flight during teardown. Easy to make robust: release llm_pool_enforce_mutex_ before taking background_sync_mutex_ (the flag check and the thread bookkeeping don't need to be held together).

2. Shutdown can block for up to global_timeout behind an in-flight load. The worker calls enforce_llm_pool_capacity(), whose load_cv_.wait(...) predicate is !is_loading_ && (!exclusive_active_ || owner) — it does not consult reclaim_shutdown_. If a slow load is in flight at shutdown, ~Server's join waits for the worker, which waits for the load. Not a permanent deadlock (the load eventually times out), but teardown can stall. Adding reclaim_shutdown_ to that wait predicate would give a clean fast-exit, consistent with the reclaim machinery already used elsewhere in Router.

[QuManu]

@SlawomirNowaczyk Thanks again, both issues are fixed now.

• The lock order is fixed. The enforce mutex is released before the background sync mutex is taken, so the two can no longer deadlock against each other.
• Shutdown no longer stalls. Added a begin_shutdown signal that Server::stop() raises early, so a pending reconcile wakes up and exits right away instead of waiting behind a load that's still running.

Covered by a new unit test, plus the full local suite (unit + CI + live server tests). Pushed in the latest commit.

[fl0rianr]

Thanks, this is very close now. I still see two concurrency issues before approval:

1. The callback ordering reopens the stale routing-helper race. reconcile_llm_candidate_floor() runs first and can wait for !is_loading_ before reconcile_routing_helpers() publishes the new needed_helper_models_. That defeats the helper reconcile's explicit guarantee to publish the authoritative set immediately during an in-flight load. Floor + helper state should ideally be published together under load_mutex_, then wait once and run both cleanup passes.

2. Shutdown only breaks the LLM reconcile wait, not the routing-helper reconcile wait. begin_shutdown() makes the floor/enforcement waits exit, but apply_routing_helper_reconcile() still waits only for load/exclusive quiescence. The same callback can therefore wake from the floor reconcile during shutdown and immediately block again in helper reconcile.

Small robustness fix: wrap the background enforce_llm_pool_capacity() worker in try/catch; an exception escaping the thread entry would call std::terminate.

Everything else from the previous reviews looks addressed. These are the remaining blockers for me.