fix(moe): the weights own their resident device state, not a static address map (#237) - #276
Merged
Merged
Conversation
…ddress map (#237) richiejp found a second engine in the same process emitting corrupted and zeroed output token ids, isolated it to the bf16 fast MoE path with VT_MOE_BF16_FAST=0, and named the cause: `MoeBf16ResidentFor` keeps its state in a process-lifetime `static` map keyed on the ADDRESS of a `MoeBlockWeights`. Confirmed, and it is six caches, not one: qwen3_5.cpp:539 MoeBlockWeights* fp4 fused MoE qwen3_5.cpp:562 MoeBlockWeights* bf16 fast MoE <- reported qwen3_5.cpp:654 MoeBlockWeights* Marlin grouped MoE qwen3_5.cpp:2271 Nvfp4Weight* Marlin dense qwen3_5.cpp:2439 Nvfp4Weight* Marlin dense pair laguna.cpp:469 LagunaMoeWeights* Laguna NVFP4 MoE All six `static`, all keyed on a raw address, and `erase`/`clear` appears zero times in the repo. The bf16 one surfaced because VT_MOE_BF16_FAST was the A/B lever available; the Marlin ones have no such switch and sit on the DEFAULT quantized path. Why it corrupts rather than crashes: nothing in `src/` calls `cudaDeviceReset` or destroys the CUDA context, so the leaked resident arrays stay mapped after an engine dies. They hold the previous engine's `d_dev` addresses, which WERE freed with the weights — so the next engine allocates over them and the fast path reads its own activations as expert pointers. Intermittent by nature: it needs the allocator to hand back a colliding address. Fixed structurally, as the issue asks. Residency is now a `ResidentSlot` member of the weights it describes, so it cannot outlive them and an address cannot be inherited; the maps are gone. The slot is deliberately opaque (`shared_ptr<void>`) — the resident types are CUDA-path details of the model .cpp files and do not belong in a public header — and it still runs the right destructor. The per-call lock the map accessors held is kept rather than narrowed, so a regression cannot be ambiguous between lifetime and synchronisation. Deliberately NOT changed: the device allocations those types point at are still leaked for the process, exactly as before. Freeing them is a question about backend teardown ordering, and putting a shutdown hazard on the critical path of a correctness fix is the wrong trade. Verified here (CPU, x86-64, Release -Werror clean rebuild after the header change): new `test_moe_resident_lifetime` 6 cases / 19 assertions, including placement-new reconstruction of a block in the exact storage a built-up block just vacated — the aliasing the old map could not distinguish. Mutation check: restoring the old ownership (a static map keyed on the slot) reds that case and only that case, 5/6. Focused model suite 22/22. NOT yet verified: the repeated-engine CUDA gate on real hardware, which is the arm that proves the reported corruption is gone rather than the invariant that prevents it. It is owed on the GB10 box and this fix should not be considered closed until it runs. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude-Code:claude-opus-5 [ClaudeCode]
…237) The issue asks for a deterministic repeated-engine regression test. This is the CUDA half: build a Qwen3-Coder-30B-A3B engine, greedy-decode, DESTROY it, then build another in the same process and decode the same prompt, three rounds, asserting identical ids. Qwen3-Coder specifically because it is the checkpoint the bf16 fast-MoE resident path was written for, so every expert goes through the per-expert device-pointer array that used to be cached against the ADDRESS of a MoeBlockWeights. An earlier attempt at gating this ran the ordered ctest selection from the issue and got a meaningless 10/10 on the PRE-FIX tree: without issue #206's benchmark fixture, test_bench builds an 8-prompt toy engine that never reaches MoE at all. Picking the model that actually routes through the code is the whole point. The baseline is checked for non-degeneracy before anything is compared. The reported symptom was ZEROED output ids, so two all-zero decodes would satisfy an equality check while being exactly the bug. HONEST RESULT, and the reason this lands as a no-regression gate rather than a proof: on GB10 the PRE-FIX arm PASSES. A same-tree A/B (identical flags, only qwen3_5.cpp/laguna.cpp swapped) ran both arms against the real 30B model, 8 assertions each, tree-checked by grepping the built source: arm=prefix address-keyed map PRESENT PASS arm=fixed address-keyed map ABSENT PASS The defect only manifests when the allocator hands the second engine an address the first freed, and on this box it never did. That is inherent: the reporter's own repro is intermittent, needs a specific ctest ordering plus #206's fixture, and reproduces on an RTX 5070 Ti, not here. A test cannot command address reuse. So what is deterministic lives in test_moe_resident_lifetime, which forces reuse with placement-new and whose mutation check reds exactly that case. This test adds the arm that would catch a real e2e regression on the gate model, and it is the one for the reporter to run where the corruption does reproduce. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude-Code:claude-opus-5 [ClaudeCode]
localai-bot
pushed a commit
that referenced
this pull request
Aug 10, 2026
Brings the row onto `origin/main` @848d4a87 (22 commits, including #237/#276's `ResidentSlot` members on `Nvfp4Weight`, #150's follow-ups, the docs site, and 450a1b6 retiring the per-class PR line budgets). Keyed records were resolved by taking the TARGET BRANCH version wholesale and reapplying this row's scoped edit, never a three-way combination: .agents/NOW.md main's 27B line kept ("0.72x -> 0.85x", and main's shortened f32-out/invocation-parity/MiniMax rows), with this row's "#213 head packed" + next-step reapplied. docs/STATUS.md main's 27B and 35B lines kept verbatim (the 35B binding grid rewrite from f500d1a is main's, not ours), with the packed-lm_head clause reapplied to the 27B row alone. qwen3_5_weights.h both sides ADD to `Nvfp4Weight`; union, no edit to either. After the merge the four keyed records differ from `origin/main` by exactly this row's scoped edit: NOW.md 1 line, roadmap_v1.md 1 line, STATUS.md 1 line, BENCHMARKS.md 13, plus ENVIRONMENT.md 1, FEATURES.md 1, USAGE.md 23. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Claude:claude-opus-5 [ClaudeCode]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #237 pending reporter confirmation — please read the evidence split before merging.
@richiejp found a second engine in one process emitting corrupted and zeroed output token ids, isolated it with
VT_MOE_BF16_FAST=0, and named the cause. Confirmed — and it is six caches, not one:qwen3_5.cpp:539MoeBlockWeights*qwen3_5.cpp:562MoeBlockWeights*qwen3_5.cpp:654MoeBlockWeights*qwen3_5.cpp:2271Nvfp4Weight*qwen3_5.cpp:2439Nvfp4Weight*laguna.cpp:469LagunaMoeWeights*All
static, all keyed on a raw address, anderase/clearappears zero times in the repo. The bf16 one surfaced becauseVT_MOE_BF16_FASTwas the A/B lever available; the Marlin ones have no such switch and sit on the default quantized path.Why it corrupts rather than crashes: nothing in
src/callscudaDeviceResetor destroys the CUDA context, so the leaked resident arrays stay mapped after an engine dies. They hold the previous engine'sd_devaddresses, which were freed with the weights — so the next engine allocates over them and the fast path reads its own activations as expert pointers.The fix
Structural, as the issue asks. Residency is now a
ResidentSlotmember of the weights it describes, so it cannot outlive them and an address cannot be inherited. The maps are gone. The slot is deliberately opaque (shared_ptr<void>) — the resident types are CUDA-path details of the model.cppfiles and do not belong in a public header — and it still runs the right destructor. The per-call lock the map accessors held is kept rather than narrowed, so a regression cannot be ambiguous between lifetime and synchronisation.Not changed: the device allocations those types point at are still leaked for the process, exactly as before. Freeing them is a question about backend teardown ordering, and putting a shutdown hazard on the critical path of a correctness fix is the wrong trade.
Evidence, including what it does not show
Deterministic (CPU):
test_moe_resident_lifetime, 6 cases / 19 assertions — including placement-new reconstruction of a block in the exact storage a built-up block just vacated, which is the aliasing the old map could not distinguish. Mutation check: restoring the old ownership reds that case and only that case, 5/6. Full CPU ctest 367/367.On real hardware (GB10), and here is the honest part — the pre-fix arm PASSES. Same-tree A/B, identical flags, only
qwen3_5.cpp/laguna.cppswapped, both arms tree-checked by grepping the built source:Both loaded the real Qwen3-Coder-30B-A3B, built three engines each in one process, and decoded identically with non-degenerate tokens. So this is a no-regression gate, not a red/green proof.
The defect only fires when the allocator hands the second engine an address the first freed, and on this box it never did. That is inherent — your repro is intermittent, needs a specific ctest ordering plus #206's fixture, and reproduces on a 5070 Ti, not on GB10. A test cannot command address reuse.
Two earlier gating attempts are worth recording so nobody repeats them: the ordered ctest selection from the issue returns a meaningless 10/10 on the pre-fix tree, because without #206's fixture
test_benchbuilds an 8-prompt toy engine that never reaches MoE — andtest_gdn_decode_fusedhas never existed in this repo (git log -Sfinds nothing), so that line was always running 5 tests, not 6.What I am asking of you
@richiejp — could you run
test_moe_two_engines(and your original repro) against this branch on the 5070 Ti? You have the hardware and the fixture where the corruption actually reproduces; I can show the invariant holds and that nothing regressed, but not that your symptom is gone.