fix(ENG-EXPERT-STREAM): the liveness line could not print the zero the docs told an operator to read, and then its comments outran the code twice (#912, #1091, #1106, #1108)

[localai-bot]

Repairs the six findings of #1091, a fresh review of the #912 wiring repair that landed as a332fb98d (#1076), and then the four findings of #1106, a fresh review of this pull request. Row ENG-EXPERT-STREAM, spec expert-streaming.md. None of the ten findings was a red test. Every one was a gap in what the gate could see, which is the same class as the defect the first repair had just fixed — and #1106 finding 1 is that class reintroduced, one file away, by the change that fixes it.

The liveness line could not print the zero the docs told an operator to read

ReportStats had exactly one caller, EndStep, and it returned early on steps == 0. So the run where the step boundary is never reached — F1, the defect the line exists to reveal — printed nothing at all. Measured on one binary with VT_MOE_EXPERT_STREAM_STATS_EVERY=1: healthy, 8 lines; F1 reinjected, 0 lines and only the startup banner.

stats_every_ also defaults to 16, so a short healthy run prints nothing either. A benchmark that reads absence as failure therefore calls a working lane VOID, which is what happened to the streaming benchmark and why it had to be restarted.

One final line is now printed from the store's own destructor, once per process, crossing both early returns. Measured red-then-green on the same production binary:

before: ./build/tests/test_expert_stream_wiring 2>&1 | grep -c 'steps=' -> 0
after:  ./build/tests/test_expert_stream_wiring 2>&1 | grep -c 'steps=' -> 1
        [expert-stream] steps=8 hits=96 misses=177 evictions=113 fills=177 bytes=192576 exhausted=0 advised=177

Not a second teardown hook registered when streaming is REQUESTED, which was the first shape tried and would have covered the "asked for it, never built a store" run as well. On a CPU-only host that hook's only unique job is unreachable from any test, because Reserve and Get sit in one call chain and a device platform is what separates them. It would have been an untestable branch added to fix an untestable-branch problem. What replaces it is a protocol the docs now state: the [expert-stream] ON ... banner means a store was built, the final line says what it did, and each present/absent combination means exactly one thing. The residual is recorded under the spec's ## Owed with its reason.

docs/ENVIRONMENT.md:82 and docs/USAGE.md:3105 both instructed the operator to read steps == 0 off the unreachable line; both now describe the final line and the banner pair.

CHECK(s.advised > 0) could not fail for the defect it named

Reinjecting the pre-fix unaligned madvise address exits 0 in 40 of 40 runs. The measured reason is that > 0 over 48 calls is satisfied whenever heap layout happens to page-align a single slice, and one did: advised=1 against fills=48.

advised == fills is the true healthy invariant on that arm — madvise is issued on the mapping-copy path only, and only when the key is not already resident, which is exactly the condition under which EnsureSpan goes on to fill, with exhausted == 0 asserted beside it as the premise. It was verified stable over 50 consecutive runs before being asserted rather than after.

Two residuals it rests on are now stated beside it rather than left to be rediscovered: madvise rounds the range's END up past a heap allocation and would return ENOMEM on an unmapped trailing page, so the equality holds because the allocator's arena page is mapped and not because the arithmetic guarantees it; and the counters are cumulative, so the equality is a statement about everything that ran before it — the pread case at the end of the file fills without advising. That ordering is not implicit: the CHECK_FALSE(s0.active) opening the case fails loudly if anything ran first.

"Every MoE entry point funnels through here exactly once per forward" was false

Qwen3_5Model::ForwardDense, Qwen3_5MTPModel::Forward, Qwen3_5MTPModel::ForwardPaged and Qwen3_5ReplayLayer all reach ExpertMlpKq -> KqExpertSlice and marked no step. ONE of them, Qwen3_5MTPModel::ForwardPaged, is the production spec-decode DRAFT forward, so a draft's acquisitions stayed protected_this_step across the following target forward — F1 at draft scale. This paragraph said "the MTP pair" through two revisions; the table further down is the measured version, and the last section of this body is the repair.

One forward is one step, and that is the call the draft forced. A draft is a complete forward whose slices are finished with when it returns; folding it into the target's step would pin them across a second forward for no benefit, so a spec-decode iteration advances the clock once per draft plus once for the target. The opposite mistake is the one adding guards invites — a guard nested inside another ends the step twice, which decays every resident entry an extra tick for a step that never happened — so the guard REFUSES to nest, stated as a precondition in the same idiom MatmulF32Slice uses for expert >= 0 rather than handled.

RunMoeBlock stays deliberately unguarded: it is one block, not a forward, and qwen3_moe.cpp owns the boundary for the model that composes it. That exemption is what makes the steps == 0 case constructible without breaking anything.

Three smaller repairs, all the same class

ExpertStreamer::EnsureFile is the arm every real GGUF-mmap checkpoint takes and no test reached it, so the file_offset + offset composition was unverified. A CPU-local case now drives it through the production seam from a temp file at a deliberately awkward offset (4109 bytes: past a page, not on a page, not on a 34-byte Q8_0 block), and it PROVES the arm rather than assuming it — advised stays flat while fills grows, which is the one number that separates a pread from an EnsureSpan. No box and no 370 GiB checkpoint needed.

OwnedTensor::TowerUid's comment promised an identity for "this tensor's CURRENT bytes" while the code keys on bytes.data(). The comment now states where the guarantee stops, and a borrowed-buffer case pins both halves — same address with different contents keeps the uid, a different address does not — because #1066 was that same overclaim on that same field. The smaller correct change: the comment, not the code.

SetForceFallback has no production caller and was incrementing the operator-facing exhausted_, so a gate asking for the unstreamed arm told an operator to raise a budget that was never the reason. Measured: exhausted=42 from the switch alone. It has its own counter now, kept off the stderr line because in a production process it is always zero.

Then the review of that repair (#1106) found three claims that outran the code

The six repairs above are correct, and a fresh reviewer reproduced all thirteen of their mutation claims independently. What failed is what was said about them.

The teardown comment named a hook that was never built. qwen3_5_internal.h said the final line is reached at teardown by "a static registered the first time streaming is requested, plus the store's own destructor, whichever runs first". There is no such static — grep -rn 'atexit\|quick_exit' over qwen3_5.cpp returns nothing, and the section above says in this same body that the hook was deliberately not built. It also promised "exactly one line per process, even on a run with zero steps" without either qualifier docs/USAGE.md carries. This is #1091 finding 5 — a comment promising more than the code — reintroduced one file away in the change that fixes it, which is the strongest argument on record that the class is a habit and not an accident.

~Qwen35ExpertStream is now named as the only production path to the LINE, with both qualifiers (a store must have been BUILT; the process must RUN its static destructors), and with what the exposed seam costs a caller: it takes the once-flag, so calling it suppresses the teardown line for the rest of the process. That is a fourth banner/final-line shape, and docs/USAGE.md now tabulates it rather than leaving a gate's own output looking like a crashed process. docs/FEATURES.md carried the same overclaim in one line and now says IF a store existed.

"Nothing lands dead" was claimed for four step guards and holds for one. The claim in this body's previous revision — "every added path is reached from a production entry point at this commit" — was true of one guard in four:

Guard	Production caller
Qwen3_5MTPModel::ForwardPaged	YES — runner.cpp:2183 -> spec_decode/mtp/speculator.cpp:107,262
Qwen3_5MTPModel::Forward	no — reached only through ForwardLogitsHost, a "standalone parity convenience" (qwen3_5_mtp.h:135) with no caller outside tests/
Qwen3_5Model::ForwardDense	no — the parity reference by qwen3_5.h:234; callers are test_op_parity.cpp:1107, test_runner.cpp:1278, test_qwen35_paged_forward.cpp:293,320,385,403
Qwen3_5ReplayLayer	no — per-layer parity replay by qwen3_5.h:322; only caller test_op_parity.cpp:1050

Per .agents/reachability.md, a call site inside a test is not reach. No guard is deleted. They are correct where they sit, they cost nothing, and they become live the moment any of those entry points gains a production caller — and adding the guard later, together with the caller, is precisely how this row lost its step boundary in the first place. What changes is the record: the three are named as a staged slice that lands unreached, in the commit body, here, and under the spec's ## Owed, tracked as #1108.

The nesting refusal was asserted everywhere and pinned nowhere. The source, the spec and this body all stated that the guard refuses to nest. Deleting its VT_CHECK left both focused binaries fully green — test_expert_stream_steps 6/6 rc 0, test_expert_stream_wiring 4/4 rc 0 — and it appeared in none of the thirteen mutations. No legitimate call graph can nest a step, because every forward that takes expert slices is a complete forward that no other one contains, so a gate cannot reach the refusal through production code, and a gate that reimplemented the flag would prove its own copy.

Qwen35ExpertStreamStep therefore names its Begin/End, detail::ExpertStreamStepScope forwards to them, and the new case asserts the refusal twice: a second scope throws, AND a real ForwardDense entered while the scope is held throws too. The second is the load-bearing one — it is what shows the two share a boundary rather than agreeing by coincidence, and mutation MN3, which gives the scope a parallel flag, kills exactly that pair and nothing else.

That refusal stays UNGATED on Qwen35ExpertStreamRequested(), deliberately. "One forward is one step" is a property of the call graph, not of the streaming lane, so a nest is a defect whether or not a store exists. Arming it only under streaming — the rare configuration — would let the default path establish a nest that nobody sees until someone switches streaming on, which is this row's recurring shape. The cost is that a forced nest reds every Qwen3.5 forward and not merely the streamed ones, and that breadth is the point.

The MSVC repair was incomplete. ::setenv sat at namespace scope in both new gates with no _WIN32 guard, so the previous revision's claim that "only the two questions about the statistics line need POSIX" was false and so was the file comment saying the step-clock cases are "built everywhere". setenv(3) is POSIX, MSVC's CRT has only _putenv_s, tests/CMakeLists.txt:1087 adds the target unconditionally and scripts/build-windows-release.ps1 configures VLLM_CPP_BUILD_TESTS=ON — neither translation unit compiled there. Both now use vllm_test::SetEnv from tests/support/test_env.h, which is where that branch has lived since #603 and which a new env-flipping test is supposed to use; the one overwrite=0 call keeps its semantics as an explicit getenv test, because the shim is deliberately two-argument.

CI could not have reported it. The Windows lanes fail earlier, inside the product library, on the pre-existing #1068 (verified: git diff origin/main...HEAD on qwen3_5_weights.cpp is empty), and a lane that never reaches a test TU cannot fail in one. The static checker that could have is blind twice over — scripts/check-windows-portability.py reads only the sources reachable from the shipped SERVER target, so no test TU at all, and its POSIX_PATTERNS name neither setenv nor unsetenv. Measured Windows portability contract OK, rc 0, on the unrepaired tree. Filed as #1107 against ENG-RELEASE-WINDOWS and NOT fixed here: changing a checker's semantics needs its own spec and red-before evidence, and widening the scan to tests/ has to separate a guarded POSIX call from an unguarded one across a large surface.

And then the review of THAT repair: the code was right and the sentence about it was not, for the fourth time

A fresh scoped review of 4ada1fb8d returned FAIL on one blocking finding and three advisory ones. It reproduced every mutation independently and ran a 500/500 gate, so nothing here changes behaviour. What it found is that two source comments asserted the reachability claim this same delta's records refute: qwen3_5.cpp called "the MTP pair" the production spec-decode draft path five lines above the guard, and test_expert_stream_steps.cpp said the same, while the spec's ## Owed, the #1108 index row and this body's own table all said one guard in four. The tree stated two contradictory things about one fact, and the false half sat where a reader hits it first.

Its closing warning is the instruction this round actually followed: "this is the fourth consecutive review in which the code was right and the sentence about it was not, and a targeted patch on two known lines is how the fifth one gets set up." So rather than patch the two cited lines, every claim in this row's delta about reachability, production paths and guarantees was audited against the code.

51 claims examined, 12 wrong, 12 repaired. Search terms: production, reached/reaches, only caller, no caller, every, always, exactly, guarantee, never, plus call-graph greps for each named symbol (ExpertMlpKq, KqExpertSlice, ForwardLayers, ForwardDense, ForwardPaged, ForwardLogitsHost, Qwen3_5ReplayLayer, ExpertStreamSetForceFallback, ExpertStreamFlushStats, FlushFinalStats, ExpertStreamStepScope, RunMoeBlock, ReportStats). The other 39 hold, including every one the reviewer had already reproduced.

Five of the twelve were cited by the review. Seven were not, and they are the reason the audit was worth doing rather than the patch:

#	Where	The claim	Why it is wrong
1	qwen3_5.cpp:5359	"TEARDOWN IS THE REAL CALLER, in ~Qwen35ExpertStream below" heading FlushFinalStats	The destructor deliberately does NOT route through FlushFinalStats — its own comment three lines below says so. Identical in shape to the advisory finding about the header, and it had gone unreported
2	qwen3_5.cpp:8176	Qwen3_5MTPModel::Forward's guard: "a spec-decode iteration therefore advances the clock once per draft plus once for the target"	True of the pair, but written inside the overload that never runs it. ForwardPaged does
3	.agents/specs/expert-streaming.md:1233	"The MTP pair is the production spec-decode DRAFT path"	Same false claim, in the record that is supposed to be the correction
4	.agents/specs/expert-streaming.md:1291	"named as the only production caller"	Caller vs path — the same conflation as the header
5	.agents/issue-index.md:310 (#1091 row)	"The MTP pair is the production spec-decode DRAFT path"	In an APPEND-ONLY record. Corrected in its only open window: the row was added by this pull request and does not exist at the merge base, so the diff carries no removal line and a union merge cannot duplicate it. check-issue-index-append-only.py re-run green against 22056e238
6	this body	"The MTP pair is the production spec-decode DRAFT path"	Contradicted by this body's own table, 40 lines later
7	this body	"named as the only production caller"	Same conflation

The three advisory findings are repaired as asked. docs/FEATURES.md promised the exit line under one of its two qualifiers; the cell measured 220 of 220 against MAX_CELL_CHARS, so the fix is a trade and the trade is stated: (#1106) is spent to buy "on a clean exit", which is worth more to an operator than a cross-reference the spec, both docs and git log --grep already carry. Back to exactly 220. docs/USAGE.md's fourth shape was keyed on an observable that does not discriminate — PrintStatsLine makes the periodic and final lines byte-identical, so a run of ≥16 steps that then crashes matches it as well as row 3 — and is now keyed on its cause, a call to ExpertStreamFlushStats, with a paragraph saying outright that stderr cannot separate the two. qwen3_5_internal.h:422 now leads with the sharper true statement: ExpertStreamFlushStats has ZERO production callers and exists for the gate, while the only production path to the LINE is the destructor, which does not call it.

On the judgement call the review left open: ForwardPaged's caller is itself "UNREACHABLE unless a speculator is configured" (runner.cpp:2120), and .agents/reachability.md defines a production entry point as a server or command-line path on its default configuration. "Has a production caller" is true; "reached on the default configuration" would not be. That nuance is recorded on #1108, which already owns this debt — but the qualifier is also carried in the three sentences being rewritten here anyway, because leaving a knowingly imprecise clause standing immediately after an audit for imprecise clauses is the failure this round exists to stop. No guard changed, and no claim structure was re-litigated beyond that clause.

Nothing executable changed, which is exactly why only a reader catches this class: the focused gate cannot regress on any of it. The evidence is the audit — the counts and greps above — plus an unchanged gate.

Evidence

Red first for every fix. Findings 1, 3 and 6 went red on the unmodified tree (0 statistics lines; Steps() - before == 1 failing 0 == 1 at all four entry points; off.exhausted == 0 failing 42 == 0). Finding 2's red is the reinjected F5 defect against the tightened assertion (1 == 48). Findings 4 and 5 are reachability, so their red is the mutation. The #1106 repairs are comments, records and one pinned guarantee, so the guarantee's red is its mutation and the rest have nothing executable to redden.

16 mutations, 16 caught. Every row records a non-empty git diff --stat or a changed sha256, a compile status, and the doctest case count, because a mutation that does not build reads as a passing test and a filter matching nothing prints SUCCESS. Two first attempts were INVALID rather than passing and are recorded as such: M9 did not build (-Werror on an unused file_offset), and M1/M2 first reported the CHILD process's doctest summary, because a failing case dumps the child's output into the parent's log and the first test cases: match therefore belongs to the child. Every row below takes the LAST match.

id	finding	target	applied	compiled	run	doctest cases (parent)	verdict
M1	1 teardown flush	test_expert_stream_steps	153 ins / 23 del	rc=0	rc=1	6 run / 5 passed / 1 failed	CAUGHT
M2	1 final bypasses both early returns	test_expert_stream_steps	156 ins / 24 del	rc=0	rc=1	6 run / 4 passed / 2 failed	CAUGHT
M3	2 unaligned madvise (the pre-fix F5 defect)	test_expert_stream_wiring	157 ins / 25 del	rc=0	rc=1	4 run / 3 passed / 1 failed	CAUGHT
M4	3 MTP Forward guard	test_expert_stream_steps	153 ins / 23 del	rc=0	rc=1	6 run / 5 passed / 1 failed	CAUGHT
M5	3 MTP ForwardPaged guard	test_expert_stream_steps	153 ins / 23 del	rc=0	rc=1	6 run / 5 passed / 1 failed	CAUGHT
M6	3 Qwen3_5Model::ForwardDense guard	test_expert_stream_steps	153 ins / 23 del	rc=0	rc=1	6 run / 5 passed / 1 failed	CAUGHT
M7	3 Qwen3_5ReplayLayer guard	test_expert_stream_steps	153 ins / 23 del	rc=0	rc=1	6 run / 5 passed / 1 failed	CAUGHT
M8	3 regression: the ForwardLayers guard itself	test_expert_stream_wiring	154 ins / 24 del	rc=0	rc=1	4 run / 1 passed / 3 failed	CAUGHT
M9	4 pread drops file_offset	test_expert_stream_wiring	155 ins / 24 del	rc=0	rc=1	4 run / 3 passed / 1 failed	CAUGHT
M10	4 pread drops the slice offset	test_expert_stream_wiring	155 ins / 24 del	rc=0	rc=1	4 run / 3 passed / 1 failed	CAUGHT
M11	5 TowerUid stops re-stamping a moved buffer	test_qwen36_weights	1 ins / 1 del	rc=0	rc=1	10 run / 8 passed / 2 failed	CAUGHT
M12	6 forced fallback charged back to exhausted	test_expert_stream_wiring	153 ins / 22 del	rc=0	rc=1	4 run / 3 passed / 1 failed	CAUGHT
M13	reachability: the slice seam itself	test_expert_stream_wiring	155 ins / 24 del	rc=0	rc=1	4 run / 1 passed / 3 failed	CAUGHT
MN1	#1106.3 delete the nesting VT_CHECK	test_expert_stream_steps	2 lines, sha 9ca33ee207a5→52b389634b48	rc=0	rc=1	7 run / 6 passed / 1 failed	CAUGHT
MN2	#1106.3 End never clears the flag	test_expert_stream_steps	1 line, sha 9ca33ee207a5→f3f740d98573	rc=0	rc=1	7 run / 3 passed / 4 failed	CAUGHT
MN3	#1106.3 the scope gets a PARALLEL flag	test_expert_stream_steps	13 lines, sha 9ca33ee207a5→b0d983980f3b	rc=0	rc=1	7 run / 6 passed / 1 failed	CAUGHT

MN1 reds all six assertions of the new case and reports Steps() - before as 3 where 1 is correct, which is the double-count the guard exists to stop. MN3 reds exactly two — forward_threw and its message — which is the pair that proves the scope and the production guard share a boundary; the "a second scope throws" half survives a parallel flag by construction, and that is why it is not asserted alone. Each mutation was restored from a byte copy (never git checkout --, which would have restored the index over uncommitted work) and the file's sha256 re-checked against the pre-mutation value before the next one ran.

Not mutation-proven: the Windows repair. No MSVC is reachable from this host, the Windows CI lanes cannot report a test TU while #1068 stands, and the static checker that would have caught it is #1107. The spec's ## Owed says so rather than leaving it to be assumed.

Gate

Merged origin/main at 22056e238 first. The branch was behind it, so agent-preflight.sh had been SKIPPING its commit-trailers and commit-style range gates, and a conflict-free git merge-tree says nothing about whether the merged tree compiles. cmake --build build -j 12 && ctest -j 6 on the MERGED tree, CPU-only Release, 20 cores:

build_rc=0
100% tests passed, 0 tests failed out of 500
ctest_rc=0

500 rather than 498: test_expert_stream_steps is new here, and test_nemotron_h_moe_device arrived with the merge. scripts/agent-preflight.sh reports All gates green, with doc-checkpoint, issue-index append-only, commit-trailers and commit-style all RUN over 22056e238..HEAD rather than skipped.

The claims-accuracy repair on top (3ef9d023c) reruns the same gate from a fresh build tree, and it has to be UNCHANGED because nothing executable moved:

build_rc=0
100% tests passed, 0 tests failed out of 500
ctest_rc=0

scripts/agent-preflight.sh All gates green, rc 0, both --staged and over the committed range. Two notes on instrument hygiene, since this round is entirely about claims that were not checked. The header edits invalidate dependents, so the first cmake --build was followed by a second that recompiled 506 targets — running ctest after the first alone would have measured stale objects; a third build reports ninja: no work to do, which is what says the tested binaries are the committed tree. And issue-index append-only passing is not by itself evidence, because a vacuous range also prints OK: the range it actually examined is 5 added lines and 0 removal lines over 22056e238..HEAD, which is why editing the #1091 row this pull request appends cannot union-duplicate anything.

windows-msvc-cpu and windows-msvc-vulkan are red, as they are on main and on every recent pull request: qwen3_5_weights.cpp does not compile under MSVC since #1054, filed as #1068 and untouched here.

Two docs/FEATURES.md trades are recorded rather than hidden, because that cell sits against the 220-character keyed-table limit and every addition to it evicts something. The first cost "LFU + LRU tiebreak" to buy "IF a store existed": an eviction policy is implementation and is stated twice in the spec, while which guarantee an operator actually gets is not. The second cost (#1106) to buy "on a clean exit", the qualifier the first trade had left out — a cross-reference the spec, docs/ENVIRONMENT.md, docs/USAGE.md and git log --grep all still carry, spent on the half of the guarantee an operator cannot recover from anywhere else. The cell measures 220 of 220 both before and after.

Not run: anything on dgx.casa. A benchmark held the host mutex and every repair here is CPU-local. The decode re-measure on a live cache stays owed for that host, unchanged by this change.

FOLLOWING_AGENTS_PROTOCOL

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