fix(deepseek-v4): gate the DSA top-k window clamps and non-positive topk, attribute the launch error, and stop a guard comment overclaiming (#552)

[localai-bot]

Closes #552. Spec: .agents/specs/dsa-topk-bounds.md §7. Row: MODEL-TEXT-deepseek-v4-deepseek-v4-for-causal-lm. Follow-up to #542 / #505.

A fresh reviewer (never the author) reviewed #505 and returned PASS with 6 non-blocking findings. This lands all six. Worth stating plainly what the review couldn't break, because it bounds what this PR is: it found no input where the two-pass threshold selection diverges from DsaTopkSelect — 3,000,081 fuzzed shapes across three independent implementations (the host reference, a transcription of the kernel, and its own O(n²) rank-count oracle), output slots poisoned with -777 so an unwritten slot couldn't pass as legitimate -1 padding — and it reproduced both the ASan overflow and the device SIGABRT on real sm_121a.

Code

1. DsaTopkLaunch had no post-launch cudaGetLastError() while every sibling launcher here has one. That is exactly why the #505 stack overflow surfaced as cudaStreamDestroy: an illegal memory access — an error raised by whatever ran next, and the text the entire #505 evidence chain ended up quoting. Now Check(cudaGetLastError(), "dsa_topk launch"), so the next fault in this kernel is attributable to it.

2. topk <= 0 diverged between the arms. The host reference asserts (deepseek_v4_dsa.cpp:76); the device launcher silently returned an empty vector. It now throws to match. The "two independent implementations agree" gate couldn't see this because no case passed a non-positive topk.

Tests

3. Nothing exercised win_start < 0 or win_end > num_keys. Mutating either clamp away left all four #505 cases green, while an off-device fuzz caught both immediately; on device they are out-of-bounds logits reads. New case drives under-run, over-run, both-at-once and an in-range control at topk=512, and asserts no emitted key escapes the clamped window.

4. New case asserts both arms refuse topk=0 and topk=-1 — not merely that they agree on accepted inputs.

Prose — corrections to claims that were wrong, not polish

5. The w < topk comment named a NaN write-past-row as the failure class it defends. That cannot occur. Pass 2's predicate is satisfied by exactly rank(th) elements with rank(th) ≤ topk, and a NaN never satisfies it either since better(x, NaN) is false, so w cannot exceed topk for any input. The reviewer proved it by removing the bound and by weakening it to w <= topk — both left the device suite 4/4 SUCCESS and a 3M-shape fuzz clean. The guards stay because they're free; the comment now says belt-and-braces, bounded by construction. A comment that overstates what it protects is worse than none, because the next reader trusts it.

6. "Strictly cheaper" is now qualified as a comparison to the old kernel only, not fitness for the real geometry: this is one thread per token row with a dependent global load per iteration, so at V4-Pro's index_topk=1024 the last row of a 4k prompt is ~4.2M serial loads in one thread.

7. Trivia: the spec cited the pre-fix span as :624-665 (actual 624-669), and the kernel comment called n > topk the overflow condition — it only selects the branch; the overflow needs n > 512 for chosen or topk > 64 for picked, which is why the old topk=3, nk=5 gate shape was ASan-clean.

What the review closed rather than raised

Recorded in spec §7 so nobody re-opens it: five mutations left the suite green and are semantics-preserving, not coverage gaps — each is also undetected by a 200k-shape fuzz against the oracle, which is how the reviewer separated the two. Removing w < topk; weakening it to w <= topk; simplifying pass 1's argmax to v > best_val (equivalent, the scan is ascending); n <= topk → n < topk (equivalent, at n == topk the full path selects all n); dropping if (th_idx < 0) return (reachable only via NaN). No action owed.

Also confirmed, not assumed: the tie-heavy case is uniquely load-bearing — the only case in the suite that reddens a tie-break inversion or a value-only threshold.

Recorded, not repaired

b649a1ea2 introduced the #505 spec in the same commit as its code, where the protocol requires the spec first. Noted in §7 rather than quietly dropped.

Gate-reading trap worth keeping

From the reviewer's own run: -ts='*DSA top-k*' (suite filter) instead of -tc= (case filter) printed test cases: 0 | 0 passed | 0 failed | 23 skipped beside Status: SUCCESS! — a live false green from a filter matching nothing. Any scoped run of this suite must use -tc= and check the case count is non-zero. The device script for this PR encodes that.

Evidence

CPU: test_cuda_deepseek_v4 25/25 (was 23 — both new cases registered), test_deepseek_v4_dsa 13/13, both SUCCESS. scripts/agent-preflight.sh --staged fully clean, including the contention-flappy test_cpu_x86_llamacpp_floor.

Device, on dgx.casa (GB10 sm_121a), mandatory flags, fast path hard-verified in each run's own configure log. Every arm is a fresh nvcc rebuild from a pristine kernel with the binary mtime verified to advance, scoped with -tc= (never -ts=), run under flock $HOME/gpu.lock:

arm	exit	Status	reddened
baseline	0	6 | 6 passed SUCCESS	—
no_topk_guard	1	FAILURE	new refusal case: CHECK_THROWS … did NOT throw at all!
no_ws_clamp	134	FAILURE	new clamp case CRASHED, SIGABRT, illegal memory access
no_we_clamp	1	FAILURE	new clamp case, 1052 failed assertions
no_launch_check	0	SUCCESS	nothing — see below
prefix_with_check	134	FAILURE	refutes finding 4's rationale — see below
restored, full suite	0	25 | 25 passed | 0 skipped, 90062 assertions, SUCCESS	—

Both new cases have teeth against the mutation each was written for.

Finding 4's rationale was REFUTED by its own verification

Worth reading before approving, because it changed the change. The review held that a post-launch cudaGetLastError() would make the next fault in this kernel attributable to it, rather than surfacing later as cudaStreamDestroy the way #505's did. The arm built to demonstrate that disproved it. With the pre-fix #505 kernel and the new check both in place:

what():  vt cuda: cudaStreamDestroy: an illegal memory access was encountered
test_cuda_deepseek_v4.cpp:214: FATAL ERROR: test case CRASHED: SIGABRT

The error text is unchanged. A stack-overflow illegal access is an asynchronous execution fault; cudaGetLastError() right after a launch reports launch-configuration errors. Here it wasn't even caught by the following cudaStreamSynchronize, latching only at stream destruction.

The check is kept — free, matches every sibling launcher, and does cover the launch-configuration class — but its comment and spec §7.1 now say what it actually does. Claiming "this makes the next fault attributable" would have repeated finding 1's defect, an overclaiming guard comment, inside the very change that exists to correct one. no_launch_check reddening nothing is the honest counterpart: a diagnostic with no observable behaviour on a passing run.

Infrastructure incidents, handled not absorbed

Round 1's clamp mutations were refused by their own uniqueness assertion — the clamp pair appears in both DsaLogitsKernel (:604-605) and DsaTopkKernel (:628-629), so a one-line anchor was ambiguous and the assert declined to edit a kernel it wasn't aiming at. Round 2 re-ran with a three-line anchor unique to DsaTopkKernel. Then the box rebooted mid-arm (up 7 min, the known GB10 unified-memory OOM-reboot class), killing the run with no marker written; the remaining arms were relaunched on the fresh box.

The false-green shape recurred twice more: no_ws_clamp printed assertions: 16865 | 16865 passed | 0 failed and prefix_with_check 609 | 609 passed | 0 failed, both beside Status: FAILURE!.

CI note: windows-msvc-cpu / windows-msvc-vulkan are red at the repo-wide baseline, not from this change — the same two fail on #539, #541 and on #511 which already merged; the logs show every target building with no error C####, failing in the release-packaging PowerShell step.

🤖 Generated with Claude Code