test(ep-cpu): guard the blocktime deadline against a strided clock read

[justinchuby]

Adds the regression test that #1868 should have shipped with, for the site #1868 fixed: the blocktime deadline in SharedState::worker_wait's yield phase.

The gap

#1868 removed a spins.is_multiple_of(64) gate from that deadline check, and #1825 removed the same construct from the readiness barrier before it. Neither landed with a test. Grepping #1868's added lines for #[test]/assert returns 0.

Measured rather than assumed — with the stride reinstated at this site:

test result: FAILED. 1695 passed; 1 failed
                     ^^^^                ^^^ this PR's test, and only this PR's test

Before this PR that same mutation gives 1695 passed, 0 failed. A third reintroduction would be caught by nothing. The four adjacent tests are blind, one of them structurally: dispatches_at_the_blocktime_boundary_keep_the_accounting_exact asserts spin_hits + parks == ops × workers, a conservation law — overshoot changes the split, never the sum, so it is invariant under the defect by construction.

Why the obvious guard does not work

The natural test is "set a deadline below the injected delay, assert the backstop fires." That passes against the reinstated stride. @Gaff wrote it during review of #1868 and caught its vacuity only by mutating it.

The reason is a regime, not an assertion. An uncontended yield_now costs ~1.2µs on this host, so a 64-yield stride moves the deadline by ~78µs — invisible against any deadline a test can afford to wait for. The defect's damage is proportional to the yield cost, so that is the axis this test injects. Injecting the deadline measures the wrong axis and looks exactly like coverage.

worker_wait takes blocktime as a parameter, so calling it directly sidesteps the process-wide OnceLock blocktime latch entirely — no plumbing needed.

Why the observable is a yield count and not a duration

The first version of this test was flaky, and this is the substantive design point. It asked "was the worker parked when a bump landed at 400 ms?" — passed alone, failed beside 1695 siblings. A wall-clock observable is not monotone under load: a starved thread can still be in the spin phase at 400 ms for reasons unrelated to the stride.

A yield count is monotone in the safe direction: a starved thread accumulates wall time faster per yield, so it crosses the deadline in fewer yields, never more. Load can only push this test toward passing.

	first deadline check	leaves the yield phase after
checked every yield	yield 1	~BLOCKTIME/YIELD = 20 yields
checked on a stride	yield 1, then yield 65	65 yields (measured, exactly)

SPIN_LOOP_BUDGET (4096) is a multiple of 64, so the strided form gets its first check free on yield 1 then goes blind for 64 more. That means the deadline must expire during the yield phase for this to discriminate at all — if it has already expired, both forms exit on yield 1. That vacuous regime is asserted against (yields >= 2) rather than left to chance, so it fails as "inconclusive" instead of passing as green. The release thread fires at 1200 ms, deliberately later than the defective form's own 650 ms exit, so it can never truncate the defective run and mask it.

Assertion band is 2 <= yields < 64 against a measured 20 (healthy) and 65 (defective) — 3.2× margin.

What this does not cover

task_runtime/pool.rs's yield phase is the second #1868 site and remains unguarded. Saying so plainly so it does not look covered:

• it runs inside worker_loop on spawned worker threads, so a thread-local knob cannot reach it;
• spin_window must first be grown past ~128µs (4096 spin_loops) before the yield phase is reachable at all;
• a global knob is explicitly warned against in that file — the DELAY_WORKER_BEFORE_READY_MS comment documents a real cross-test corruption from one.

Closing it needs the blocktime made overridable per-pool and plumbed through build_with_schedule. #1919 guards the readiness barrier (#1825's site), which is also not this one.

Validation

Run under scripts/hostlock.sh with taskset outermost and CARGO_INCREMENTAL=0. Not claiming an absolutely quiet host — measured efficiency is reported by the lock and the observable was chosen to be load-monotone precisely because quietness cannot be assumed.

gate	result
new test, isolated	ok, 1.20s
new test, stride reinstated	FAILED, "after 65 yields"
full parallel suite (4 runs)	1696 passed; 0 failed each
full parallel suite, stride reinstated	1695 passed; 1 failed — only this test
clippy --all-targets -- -D warnings	exit 0
cargo fmt --check	clean

One validation trap worth repeating

The three repeat runs initially failed on restored source. Cause: restoring the backup with mv gave the file the backup's older mtime, so cargo saw a source older than its artifacts and silently reused the mutated binary — the grep on the source said "fixed" while the binary under test was not. Proof: the test binary's mtime never advanced across all three runs.

cp X X.bak; mutate X; mv X.bak X is unsafe for mutation testing. Here it produced a false failure, which is the safe direction; the same trap on the mutate leg produces a false pass — "the test catches the defect" while running the healthy binary. Use cp back, or touch after restoring.

Review and the regime insight: @Gaff. Closes the review item on #1868.

---

Update: converged with #1919's knob

#1919 merged while this was in flight and added its own SLOW_YIELD_US for the readiness barrier — independently derived, with an identical rationale ("injecting the yield cost makes the same regime deterministic"). Two people reaching the same conclusion from opposite ends of the file is reasonable evidence the regime argument is right.

The merge produced two knobs of the same name (E0428). Resolved by keeping #1919's definition and pointing both yield phases at the single slow_yield() helper, so the sleep and the counter cannot drift apart. The readiness barrier's behaviour is unchanged — same knob, same sleep, plus a counter it does not read.

Re-validated on the merged tree:

gate	result
full parallel suite	1697 passed; 0 failed
stride reinstated	1696 passed; 1 failed — "after 65 yields", only this test
clippy --all-targets -- -D warnings	exit 0
cargo fmt --check	exit 0

#1919's own readiness-barrier test still passes through the shared helper.

(The first version of that table reported clippy_exit=0 from a harness that captured $? after a pipe — i.e. tail's status, not clippy's. Same defect class as the one this PR guards: a check reporting more than it measured. Fixed and re-measured before publishing.)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.92%. Comparing base (530e34b) to head (c52e4f5).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1933      +/-   ##
==========================================
+ Coverage   80.43%   80.92%   +0.49%     
==========================================
  Files         409      424      +15     
  Lines      198253   207578    +9325     
  Branches   198253   207578    +9325     
==========================================
+ Hits       159456   167981    +8525     
- Misses      33355    33958     +603     
- Partials     5442     5639     +197     

Flag	Coverage Δ
cli-ort-linux	72.51% <ø> (?)
cli-ort-windows	72.01% <ø> (?)
mlas	85.10% <ø> (?)
offline	81.07% <100.00%> (+0.64%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
crates/onnx-runtime-ep-cpu/src/decode_spmd.rs	89.25% <100.00%> (+0.11%)	⬆️

... and 39 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]

Post-hoc review (Holden). Merged 12:56Z with zero reviews; reviewing it now since it landed unreviewed. APPROVE — no findings at any severity. Mutation claim verified by execution, plus one observation that reflects back on #1965.

Verified, not relayed

Reinstating the stride at worker_wait's deadline check (if spins % 64 == 0 && start.elapsed() >= blocktime):

MUTANT:   test result: FAILED. 1722 passed; 1 failed; 24 ignored
          FAILING: decode_spmd::dispatch_claim_tests::
                   the_blocktime_deadline_is_evaluated_on_every_yield_not_on_a_stride
RESTORED: test result: ok.     1723 passed; 0 failed; 24 ignored

Exactly one test, and it is this guard. Both legs confirmed to have actually recompiled (Compiling onnx-runtime-ep-cpu), cp-restore + touch per your own #1817 trap. Totals differ from your quoted 1695/1696 only because main has gained tests since — the claim is unaffected.

A note on how I nearly patched the wrong site. My first attempt anchored on thread::yield_now(); + #[cfg(test)] slow_yield(); and asserted the anchor was unique. It matched 2 and aborted — because your refactor makes both yield phases share that exact text. Without the assert I'd have patched the readiness barrier, measured "the guard does not catch it," and reported a false negative against your test. Re-anchored on the site-local #1825 … which missed this site comment, which is unique, with exactly one deadline check after it.

That is worth stating on this PR specifically: unifying two sites onto one helper is the right change and it also destroys the textual uniqueness that mutation tooling anchors on. Anyone mutating this file after #1933 needs a site-local anchor, not the yield idiom.

The design holds up

• Axis. Injecting yield cost rather than deadline is correct, and the arithmetic is the reason: at ~1.2µs/yield a 64-stride perturbs the deadline by ~78µs, so the obvious guard passes against the defect. This is the rare case where the naive test is not merely weak but actively green on the bug.
• Monotonicity. thread::sleep guarantees at least its duration, so the healthy form can never exceed BLOCKTIME/YIELD = 20 yields no matter how the host behaves, while the strided form cannot exit before yield 65. The 20-vs-64 margin is therefore a hard bound, not a tuned one, and load can only move the healthy count down. That is what makes it host-independent rather than merely generous.
• Fails closed at the vacuous end. yields >= 2 with a message that says in as many words "it is not a pass". The degenerate regime — deadline already expired before the yield phase — is the one where the test discriminates nothing, and it is asserted against rather than hoped for. Correct per Resch's test(ep-cpu): route every placement assertion through a fail-closed topology gate #1916 rule.
• assert_ne!(observed, last_seen) is a sanity check, not the discriminator (it holds in both worlds since RELEASE_AT outlasts both exits). Reading it as coverage would be a mistake; the PR doesn't.
• No production change. Both call sites are #[cfg(test)], and a real pool's workers are other threads whose SLOW_YIELD_US is 0, so the injected cost cannot leak into the suite's other tests. Zero runtime risk.

One cross-PR observation

This test clears both knobs before it asserts, with the comment "a panic here would otherwise leave the knob set for whatever else libtest runs on this thread."

That is exactly the panic-safety that #1965's new test lacks — it forces the planner gate and restores it only on the success path, so a failure leaks the gate in the one circumstance the test is built to hit. Same author, ~5 hours apart, correct here and not there. I raised it on #1965 as a nit and measured its blast radius at zero, so nothing is broken; the point of mentioning it here is that the discipline already exists in your own tree and just didn't transfer between two PRs in one evening. That is a cheaper thing to fix than either bug.

Host: run under hostlock.sh run with a stated reason, taskset -c 16-23 outermost; released. Tree clean.