RFC: Router Admit Decision for Same-Device-Pinned Models (#66)

[marksverdhei]

RFC detailing the architectural decision between Approach 1 (in-router mini-bootstrap) and Approach 2 (out-of-process dry-run) for the router admit decision. See Issue #66.

[marksverdhei]

Supplement: the recommended Approach 1 already shipped in #136 — this RFC is now largely retroactive

Reviewed the RFC against the tree on origin/ht. The core decision it poses is already resolved in code: Approach 1 landed in #136 (30023636e, merged 2026-06-20) — a week before this RFC PR was opened (2026-06-27). The router does an in-process fit, no subprocess. Mapping the RFC to what's actually on disk:

Recommendation (Approach 1) — ✅ shipped

• Preset → common_params was already available via preset.apply_to_params(fit_params) + common_params_handle_models(...) (tools/server/server-models.cpp:227-229) — this is the same path the child boot uses, so no new common_preset_to_params() extraction was actually needed.
• The only new surface is a thin wrapper, common_fit_params_from_common_params(common_params&) (common/fit.cpp:1003, decl common/fit.h:77), which does common_params → mparams/cparams (via the pre-existing common_model_params_to_llama / common_context_params_to_llama), calls common_fit_params() in-process, and returns per_device_bytes aligned to the system's global GPU order (common/fit.cpp:1052-1079).
• common_fit_params() gained an out_bytes_per_device out-param (common/fit.h:29-38) — this is the "dry-run per-device demand without allocating" that the issue-thread sketched as common_fit_plan / common_fit_params_dry_run, folded into the existing function instead of a new one.

Next Step 2 (per-device admit + targeted LRU) — ✅ shipped

server_models::unload_lru() (server-models.cpp:856-944) now:

• queries live per-GPU free via ggml_backend_dev_memory() (:882-889) — deliberately live, which also captures the external ComfyUI allocation on titan GPU1 (matches your "accounting correction" comment: live query already reflects loaded allocations, so no static reserved[] double-count);
• checks candidate.per_device_bytes[id] > free[id] per device and records constrained devices (:895-901);
• evicts LRU only from constrained devices (helps_memory, :917-924), not globally.

So the "48 GB single pool" bug from #66 is fixed for the steady state.

⚠️ One gap that reproduces the exact #66 titan OOM — the LOADING-state reserve is NOT implemented

Your 2nd issue comment specified reserved[d] += plan[d] on admit→LOADING, -= plan[d] on LOADING→LOADED, to cover the window before a child's cudaMalloc is visible to cudaMemGetInfo. That accumulator does not exist (no reserved[] in server-models.{h,cpp}). The admit relies purely on live cudaMemGetInfo, and the under-lock re-check in load() (:988-998) guards only the count (models_max), not per-device memory.

Consequence on titan (--models-max 2, both pinned CUDA0): model A admitted → LOADING (:1004) → child spawned async; model B requested close behind → its unload_lru reads live free on GPU0 before A's cudaMalloc shows up → sees stale-high free → co-admits B → both allocate on GPU0 → OOM. The count check passes (2 ≤ 2), so only the memory check stands, and it's racy for concurrent same-device admits. Narrower window than the original bug, but the same failure. Worth confirming whether loads are serialized-to-LOADED anywhere upstream of this; if not, the reserved[] (LOADING-only) guard is still needed to fully close #66.

Next Step 3 (remove tools/fit-params dry-run) — open call, recommend KEEP

The standalone llama-fit-params CLI + --.../fit_params_print_plan flag still exist (tools/fit-params/, common/arg.cpp:2478, common/common.h:467). Since the router uses the in-process path, this is off the hot path — but it's a useful ops/debug tool (print a model's resolved per-device plan without booting the server). Suggest keeping it, documented as debug-only, rather than deleting — and explicitly noting it must NOT be wired into the router admit path (that would reintroduce Approach 2's latency + parsing brittleness the RFC rightly rejected).

Suggested disposition

Repurpose this doc as an ADR — "Decision: Approach 1; implemented in #136" — with the mapping above, rather than a forward-looking proposal, and track the two residuals as their own small issues:

1. LOADING-state reserved[] guard to close the concurrent-admit race (the real remaining Router co-loads same-device-pinned models and OOMs: --models-max fit decision ignores per-device VRAM #66 exposure);
2. document llama-fit-params as debug-only / off-router-path.

Happy to open (1) and (2) as issues and draft the reserved[] patch if you want it.

_{— code-grounded review against origin/ht; citations are file:line at time of writing.}

[marksverdhei]

Drafted the residual from my supplement above as #148 — the LOADING-state reserved[] guard. It closes the concurrent same-device admit race that #136's steady-state fix left open. The PR flags one policy decision for you (hard 503 gate vs. proceed-and-log when in-flight loads saturate a device).