residency config #1119: a hyphen typo silently disables the tier, and nothing gates the expert-stream knob reaching its decision

[localai-bot]

A fresh review of PR #1119 (head 4f71c69b4, row ENG-RESIDENCY-CONFIG, spec .agents/specs/weight-residency-config.md, implementing #1110) returned FAIL: 2 high, 3 medium, 8 low, and 14 of 69 claims about the code wrong. The mechanism is largely right; these are the load-bearing exceptions.

H1 — a misspelled TOP-LEVEL key is silently ignored, while the header says it is an error

include/vllm/config/weight_residency.h:132-138 claims a typo "is an error". Measured through the product parser:

{"vllm-cpp":{"mmap":{"enabled":true}}}   -> ACCEPTED, empty=1   (NO refusal)
{"VLLM_CPP":{"mmap":{"enabled":true}}}   -> ACCEPTED, empty=1   (NO refusal)
{"vllm_ccp":{...}} / {"vllmcpp":{...}}   -> ACCEPTED, empty=1   (NO refusal)
control {"vllm_cpp":{"mmapp":{...}}}     -> REFUSED: unknown key "vllm_cpp.mmapp"

RejectUnknownKeys enumerates inside vllm_cpp; nothing enumerates the document. The hyphenated spelling is the most likely typo of all, because every CLI flag around it is hyphenated — and it starts a server that does not borrow its weights, which the operator meets as an OOM kill. That is precisely the failure the refusal exists to prevent. tests/vllm/config/test_weight_residency_config.cpp:21 repeats the claim and its refused[] list omits the case, so no gate can see it.

Fix: enumerate the top-level document (refuse anything not in offload_backend/uva/prefetch/vllm_cpp), with a test per spelling — or delete the claim from the header, the test header and docs/USAGE.md:3336 and record the hazard under ## Owed.

M2 — surviving mutation: nothing gates expert_stream.enabled reaching the streaming decision

src/vllm/config/weight_residency.cpp:360-366. Rewiring ResolveExpertStreamRequested to read ActiveWeightResidencyConfig().mmap instead of .expert_stream — same type, adjacent field — leaves all four suites GREEN (config 11/138, reach 5/39, serve 5/55, mixed_slot 1/181), on both heads.

So the headline knob, the one the 370 GiB case exists for, could silently not stream. The pure ExpertStreamRequestedFrom test covers the decision; the wrapper's wiring to the correct field is untested, while the env-name half is covered.

Fix: a dedicated test binary (the latch is process-wide) that installs {expert_stream:true} with no env set and asserts ResolveExpertStreamRequested() == true, as test_weight_residency_reach.cpp:134-168 already does for mmap.

H2 — the public ABI comment for offload_config is now false

include/vllm.h:429-435 still says "ACCEPTED BUT NOT YET ACTED ON … no weight moves yet". After this row the same string also carries vllm_cpp, which does move weights, and test_weight_residency_reach.cpp:170-192 asserts new VLLM_ERR_INVALID_ARGUMENT refusals through vllm_engine_load. A C client is told the field is inert. Violates ## Shared seams and ## Public documents (the C API changed).

M1 — the late-install throw fires on a legitimate two-model process, and its reason is false for 2 of 5 knobs

weight_residency.cpp:236-247. Measured through the public ABI: load model A with no residency config, then load model B whose --offload-config carries vllm_cpp → model B cannot load (rc=2 VLLM_ERR_MODEL_LOAD). The spec justifies this as "the knobs latched during the first load", which is false for mmap (GgufLoadPolicy::FromEnv() runs per load) and for prefault (this change deleted its static). Only expert_stream and the slot store genuinely latch. The header's THE LATCH paragraph still lists prefault's static and claims every knob latches, contradicted by the same header at :200. And test_weight_residency_config.cpp:387 is named "…still allows an install" while its body asserts CHECK_THROWS_AS.

Fix: narrow the refusal to the knobs that latch, or keep it and correct all three sentences plus the test name.

M3 — "the line reports what the engine will use" is false

model_loader.cpp:1284,1292. It prints installed.Describe() — the fields the operator set, not the resolved values. The row's own CASE 5 proves it: with VT_MOE_EXPERT_STREAM=0 it asserts the output contains expert_stream=on while the engine runs it off. Reading the global back is still the correct mechanism (it proves the install ran); only the sentence is wrong. test_serve_residency_config.cpp:51 and :237 state opposite positions.

Low

• L1 ExtObject hardcodes the vllm_cpp. prefix, so {"vllm_cpp": 5} reports "vllm_cpp.vllm_cpp" must be a JSON object; the test uses CHECK_THROWS_AS only, so the wrong path passes.
• L2 The relaxed atomic is sound — no config state is published through the flag, so release/acquire is not required — but 5e82f8e04's stated reason ("that call takes the mutex, so it synchronises with nothing weaker") is untrue as written: a mutex acquire does not synchronise-with a relaxed store made outside it. Correct the reason to "install and resolve are never concurrent on any production path", or use release/acquire.
• L3 ActiveWeightResidencyConfig() returns a reference after releasing the mutex, so resolvers read unsynchronised while the lock gives false assurance (pre-existing).
• L4 --offload-config is silently dropped on the pooling and transcription entry points (server_main.cpp:946-1001 returns before the parse); not a regression but unrecorded.
• L5 The config surface does not reach vllm-cli, the binary the same USAGE section uses for the env form, with no note saying why.
• L6 docs/ENVIRONMENT.md:110-111 enumerate a resolution that is false when a config is present (VT_MOE_EXPERT_STREAM_SLOTS=0 with slots:8000 resolves to 8000, not 64), contradicting the row's own override clause.
• L7 DescribeEnvOverrides reports presence, so VT_MOE_EXPERT_STREAM_SLOTS=banana is announced as an override although the resolver ignores it — and the comment cites that very fact as a reason not to read the value.
• L8 EnvOn cited at gguf_keep_quant.cpp:60-65, actually 61-66; the two index rows are inserted three lines from the end of a 329-line file rather than appended.

Cleared, checked

The upstream mirror is byte-identical and include/vllm/config/offload.h is unchanged; the mirrored parser genuinely does not enumerate and its validator reads fields, so a sibling key is invisible to both. Precedence (env > config > default) holds for all five knobs in both directions. The reachability chain through both entry points is pinned. #1109 is genuinely fixed — both doc sites now say on, matching the code, and no "off" statement survives anywhere in docs/ or .agents/.

Gate on the reviewed head: build rc 0 with zero warnings, ctest 504/504 rc 0, all five focused-suite counts matching the implementer's report exactly.

Owning row: ENG-RESIDENCY-CONFIG.

[localai-bot]

Repaired in flow on row/ENG-RESIDENCY-CONFIG (#1119), head c7fa7084b. Per-finding disposition:

Finding	Disposition
H1 top-level typo accepted	FIXED — the extension parser enumerates the whole document (offload_backend, uva, prefetch, vllm_cpp); a test per spelling (hyphen, upper case, two transpositions, run-together, dotted) plus the mirrored half's own typos, through the parser, the C ABI and a real vllm serve
H2 public ABI comment false	FIXED — the inert sentence is scoped to the three mirrored keys; the vllm_cpp half, its schema, its precedence and the new VLLM_ERR_INVALID_ARGUMENT refusals are documented beside it
M1 late-install throw on a legal second load	FIXED by NARROWING — the latch is per-decision (ResidencyLatch::kExpertStream, kExpertStreamGeometry), the shared resolvers mark nothing, and the refusal fires only on a field a taken decision has frozen. mmap/prefault resolve per load and no longer block a second engine. The header's THE LATCH paragraph and the test name are corrected either way
M2 headline knob's wiring ungated	FIXED — tests/vllm/config/test_expert_stream_latch.cpp, its own binary because the answer is observable once per process. The .mmap swap is re-applied and RED
M3 "reports what the engine will use"	FIXED as a sentence — the mechanism (reading the global back) is unchanged; three sentences, one test constant and one test comment corrected, and test_serve_residency_config:51 and :237 now agree
L1 hardcoded vllm_cpp. prefix	FIXED — the path is built from the document, and the tests assert the message rather than only the throw
L2 untrue memory-ordering reason	FIXED as a sentence — relaxed is kept, the reason is now the monotonic single bit and the absence of published state, and the window no ordering can close is recorded rather than denied
L3 reference returned after unlock	FIXED — ActiveWeightResidencyConfig returns by value, copied under the lock
L4 dropped on pooling/transcription	RECORDED under the spec's ## Owed and in docs/USAGE.md. Pre-existing and wider than this key: those paths build EngineParams without the offload document at all, mirrored half included. Fixing it means moving the parse ahead of the architecture branch, which is ENG-WEIGHT-OFFLOAD's surface too
L5 absent from vllm-cli	RECORDED in the same two places. A new flag is out of this row's scope
L6 docs/ENVIRONMENT.md resolution false with a config	FIXED — both count rows now say the value falls THROUGH to the config key
L7 presence reported as an override	FIXED — DescribeEnvOverrides asks the same predicate the resolver uses, so SLOTS=banana is not announced
L8 EnvOn anchor off by one	FIXED — 61-66, in all three places that cited it

On L8's second half, the #1110/#1109 index rows are left where they are, three lines from the old end of the file. Relocating a row is a delete plus an add, which is what the append-only rule forbids and what makes two branches' relocations merge into a duplicate under merge=union. The new row is appended properly.

Eight repair mutations, all red, each with git diff --stat, the file's sha256 before and after, the build exit status, an ENOSPC count, and a non-zero doctest case count. One is worth recording: restoring the coarse latch first left the reach suite GREEN while the config suite went red, because a per-field comparison is insensitive to where the flag is marked unless the incoming document changes the frozen field. The reach case was widened to the shape measured here — a second engine arriving with a vllm_cpp document — and goes red too.

Claims audit, since the review found 14 of 69 sentences wrong: 46 claims examined in and around the change; 36 inherited ones wrong or imprecise and repaired; 5 of my own new sentences caught by the audit and corrected before the commit (an unverifiable "every vLLM config dataclass", an imprecise utils.py anchor, "never concurrent on any production path", the inherited "does not borrow its weights" which is really "runs this tier at its defaults", and an unverified claim about --compilation-config); 5 verified true as written by reading the code (FromEnv has no static, the prefault static is gone, KqExpertSlice -> Get -> Requested really is the per-slice path, the store is built once, and the four top-level keys are every key the mirrored parser reads).

Gate on the merged tree (origin/main at 4d7748646): build exit 0, zero warnings, zero ENOSPC; ctest -j 6 506/506 exit 0; scripts/agent-preflight.sh exit 0 with no gate skipped.

Staying open rather than closed with the merge: L4 and L5 are recorded, not fixed, and this issue is what the spec's ## Owed names for them.

FOLLOWING_AGENTS_PROTOCOL

Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:claude-opus-5-1m [claude-code]