test(ep-cuda): list the NMS and Unique materialization syncs, both reviewed

[justinchuby]

Fixes the red in #2149. Two allowlist entries, no kernel changes.

The decision each half needed

#2149 asks the right question rather than reaching for the allowlist: an entry is only correct if the kernel can decline capture, otherwise the sync must be guarded with CudaRuntime::is_capturing and an entry would silence a real capture-graph hazard. Both halves check out, read from production source:

kernel	declaration	line
non_max_suppression.rs	CaptureSupport::unsupported("NonMaxSuppression performs one 8-byte count D2H synchronization before dynamic ORT output allocation")	566
unique.rs	CaptureSupport::unsupported("Unique performs an 8-byte count D2H synchronization before dynamic ORT output allocation")	584

Both are outside capture, so both take the reviewed entry. NMS is the half #2149 left open — its author did not have that file in their tree — and the answer is the same as Unique's.

One detail I did not want to lose, because it is the sort of thing that makes an entry unreviewable later: the sync being listed is not the sync the declaration names. The declaration names the 8-byte count D2H in the counting phase; the listed one is the trailing drain after the materialization launch, before the governed workspace is returned. Both entries say so. That matches how dft.rs and multi_head_attention.rs are written — they explain the specific barrier rather than pointing at the declaration.

How it was verified without a CUDA toolchain

The crate does not build here, but capture_sync_contract.rs is pure std — it scans source text. So I ran the contract's own code, verbatim, with a one-line redirection of kernel_root() to the real kernel directory (diff against the original is exactly that line):

$ KERNEL_ROOT=.../src/kernels cargo test
test every_allowlisted_file_can_decline_capture ... ok
test unconditional_syncs_are_limited_to_capture_unsupported_paths ... ok

That is a green, so on its own it is worth nothing. Two mutations make it mean something:

M1 — the harness reproduces the CI failure, byte for byte. Same file at origin/main (i.e. without this patch), same kernel tree:

left:  {..., "nary.rs::run", "non_max_suppression.rs::materialize", "nonzero.rs::execute", ...
        ..., "sparse_kv_gather.rs::execute", "unique.rs::materialize", "varlen_attention.rs::execute"}
right: {..., "nary.rs::run", "nonzero.rs::execute", ...
        ..., "sparse_kv_gather.rs::execute", "varlen_attention.rs::execute"}

Identical to the set diff in the CUDA compile (Linux x86_64) log on #2151. So the harness is measuring the same thing the lane measures, and this patch is what flips it.

M2 — the premise check is live, not vacuous. Adding softmax.rs::run (a file with no CaptureSupport::unsupported) to the allowlist:

every_allowlisted_file_can_decline_capture ... FAILED
softmax.rs is on the unconditional-sync allowlist for run, but no capture_support in it
returns CaptureSupport::unsupported

So if I had made the wrong call on either kernel, the second test would have caught it. cargo fmt --check clean.

What this does not do

• It does not touch either kernel. If @leon or @roy judge that either trailing drain should be removed or narrowed rather than accepted, the entry is one line to delete and this PR is the thing to revert. I am recording a review decision, not defending the sync.
• It does not stop the next one. CUDA compile (Linux x86_64) is advisory — the required set is exactly Fast (Linux x86_64) and Rust quality — which is why feat(cuda): add bounded device Unique #2113 and feat(cuda): add bounded device-workspace NMS #2130 merged green through the lane whose entire purpose is to stop exactly this. That is the third instance in the same lane (CUDA capture-sync gate is red on every PR: dft.rs::run missing from the allowlist since #2080 #2094 was dft.rs::run, three days ago), and it is why I have argued in Required status checks are bypassable, so --auto merges before CI reports #1966 that a contract test which has now failed to block three times should be required.

Refs #2149.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.58%. Comparing base (edc42d2) to head (49c523e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2157   +/-   ##
=======================================
  Coverage   80.57%   80.58%           
=======================================
  Files         432      432           
  Lines      218152   218156    +4     
  Branches   218152   218156    +4     
=======================================
+ Hits       175771   175795   +24     
+ Misses      36537    36520   -17     
+ Partials     5844     5841    -3     

Flag	Coverage Δ
cli-ort-linux	72.51% <ø> (?)
cli-ort-windows	72.01% <ø> (ø)
mlas	85.80% <ø> (-0.10%)	⬇️
offline	80.70% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 6 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justinchuby]

Refreshed onto main (edc42d2cb) by normal merge — 49c523ea6. That clears two inherited reds (Root file allowlist, fixed by #2156/#2161; Mobius metadata packages (signal), green again).

Holding this rather than merging it, and the reason is worth stating.

CUDA compile (Linux x86_64) is red on this PR, but not at the step this PR is about:

Check CUDA execution provider and Python feature   failure   <- #2158, inherited from main
Verify CUDA test inventory and skip honesty        skipped
Audit CUDA kernel capture-sync contract            skipped   <- the step this PR fixes

onnx-runtime-python stopped compiling on main at 66ca26a5d (#2132's merge commit), so the compile step ahead of the audit fails and the assertion this PR repairs never executes. Merging now would give me a green required set and a skipped for the only check that can tell me I am right. That is exactly the "empty is not green" pattern I have been filing about all week, so I am not doing it on my own PR.

Evidence the fix is correct in the meantime, since the lane cannot supply it: I ran capture_sync_contract.rs verbatim as a standalone crate with only kernel_root() redirected (one line, diff-confirmed), because the contract is pure std and reads kernel sources rather than touching a device.

arm	expectation	result
unpatched	reproduce CI's failure	left/right set diff byte-identical to the CI log
patched	both tests pass	pass
patched + softmax.rs::run wrongly allowlisted	the decline check fires	fires

The third arm is the one that matters: it shows the suite still rejects a bad entry, so the pass in arm 2 is the fix and not a check that stopped discriminating.

Plan: when #2158 lands, re-run this lane, confirm Audit CUDA kernel capture-sync contract goes green rather than skipped, and merge on that. If #2158 stalls I will say so here rather than quietly merging on a skip.

[justinchuby]

Re-validated at the merged head 49c523ea6, because main moved ~20 commits under this branch and a new kernel with an unconditional sync would have made this PR incomplete rather than merely blocked.

Same standalone harness (the contract is pure std — it reads kernel sources — so it runs verbatim outside the CUDA crate with one line changed, kernel_root() → an env var; diff confirms exactly one line differs):

$ CSC_KERNEL_ROOT=.../src/kernels cargo test --offline
test every_allowlisted_file_can_decline_capture ... ok
test unconditional_syncs_are_limited_to_capture_unsupported_paths ... ok

And the control, since a pass means nothing without one — delete "unique.rs::materialize" from the allowlist and re-run against the same kernels:

test unconditional_syncs_are_limited_to_capture_unsupported_paths ... FAILED

So: still exactly the two entries, still discriminating, no new unallowlisted sync arrived with the merge.

Also worth recording here because it decides how long this stays blocked — the two current CUDA reds are the same inherited stack seen at two depths, not two live causes. The distinguishing field is in every job log:

grep -m1 'HEAD is now at' <job log>

A base containing 66ca26a5d (#2132's merge) fails at the compile step with 40 E0277 (#2158) and skips everything behind it. A base older than that reaches the audit step and fails there — which is this PR's target, #2149. #2124 is currently showing the older shape purely because its merge ref predates #2132.

The consequence I care about: as bases roll forward, #2149 stops being visible to anyone, masked behind a compile error in an unrelated crate. It is not fixed by that; it is hidden by it. That is the reason to keep this open and merge it on a green audit step rather than quietly on a skip.

[justinchuby]

Closing this as superseded by #2180, which landed the same two allowlist entries and more.

I held this PR rather than merging it, on a published criterion: its target step, Audit CUDA kernel capture-sync contract, had to be green, not skipped. That criterion is now met — by someone else's fix — so the honest outcome is to close mine rather than reopen the same ground.

#2180 is a superset. It adds non_max_suppression.rs::materialize and unique.rs::materialize with the DeviceWorkspace/dynamic-output rationale, and does two things this PR did not: binds each entry to its owning kernel and two-phase output policy, and exercises a real begin_graph_capture refusal, proving it happens before either kernel phase runs. Mine asserted the rationale in a comment; #2180 asserts it in a test.

The defect was real, for the record. Before closing I ran the exact CI command locally against both versions of the contract file, on identical kernel sources:

arm	expect	got
this branch's allowlist	PASS	PASS (2 tests)
control: pre-#2180 main allowlist	FAIL	FAIL (3 tests, dynamic_output_syncs_are_bound_to_their_capture_refusal)
restored	PASS	PASS (2 tests)

And the step-8 history is worth recording, because the same job colour meant three different things in three days:

main	step 6 (Check CUDA … Python feature)	step 8 (Audit … capture-sync)
bc79b3ff1	failure	skipped — fail-fast, never ran
2e1746448	failure	failure — #2170 added if: !cancelled(), so it finally ran
97353d5a7	failure	success — #2180

The job is red in all three rows. Only the step records distinguish "not passing" from "not run" from "fixed". I originally wrote the middle row as skipped on #2173 by citing a stale SHA; corrected there.

Step 6 is still red at main head — that is #2158, and #2173 is what fixes it.

Two instrument errors of mine that this cleanup exposed, both of which briefly produced a confident wrong reading:

• git diff origin/main...HEAD (three dots) diffs from the merge base, so it reports only what I added and is structurally blind to what main gained since. It showed +11/-0 while main had in fact grown a whole new test my branch lacked. Two-dot for "am I behind".
• git checkout origin/main -- <file> stages the result, so the plain git diff I used to confirm the revert showed nothing — and I read that as "the file is unchanged", i.e. as the opposite of what had happened.

Same shape both times, and the same shape as the --exact filter that matched no tests and printed ok: the reading I got when the instrument was pointed at the wrong thing was a plausible value, not an error. No merge, no bypass; nothing from this branch reached main.