fix(ep-cpu): bound the decode pool's readiness barrier and release its workers on failure by justinchuby · Pull Request #1825 · justinchuby/onnx-genai

justinchuby · 2026-08-23T08:41:30Z

What breaks today

SpmdDecodePools::build_with_schedule ends with a readiness barrier that is a
bare spin_loop() — no yield, no deadline, no escape:

while shared.ready.load(Ordering::Acquire) < total_threads {
    std::hint::spin_loop();
}

Two distinct failures reach it, and it cannot survive either.

1. Starvation livelock. The barrier spins waiting on threads that need a
core to make progress. Where the builder and its workers contend for the same
CPUs — an emulated target, a cpuset-confined process, a test harness building
several pools at once — the spinner starves the very workers it is waiting for.
That is livelock, not slow progress, and it is most reachable exactly where
cores are scarcest.

2. Permanently unsatisfiable. worker_loop indexes worker_node and reads
decode_blocktime() before ready.fetch_add. Any panic in that pre-loop
setup means the count can never complete, and the builder spins on a condition
no longer satisfiable by anything.

Either way the process burns every core it holds, forever, and it is
indistinguishable from work from outside. This is not hypothetical: an
aarch64 cargo test on this repo held a shared host for 5h40m at ~1778% CPU
across 64 threads, silently blocking two other agents, and was noticed only
because someone went reading /proc. One of them lost a full A/B run to it.
That job was mine.

Note worker_wait in this same file already has the correct bounded pattern —
spin ramp, yield_now, futex park. The build barrier was the one wait in the
file that never got it.

The fix

Four changes, all in decode_spmd.rs, none touching the hot decode path.

1. Bound the barrier. Spin budget → yield_now() → deadline → panic naming
how many of how many workers arrived. POOL_READY_TIMEOUT is 120s: workers
announce within microseconds, so a wait that reaches it is reporting a broken
pool, not a slow one. It is a liveness backstop, not a performance bound.

2. Release the workers when the barrier gives up. The first version set only
shutdown. That is not enough — a worker already parked on the futex never
re-reads the flag, so a failed build leaked its started workers for the life
of the process, which is a smaller copy of the bug being fixed. The panic path
now performs the same publish-then-wake sequence as shutdown(): shutdown
store, then per-node sense.fetch_add(Release) + wake_all. Deliberately
not joined — a build that failed this way may have a wedged worker, and
blocking on it would reintroduce the unbounded wait.

3. Accept a pool that becomes ready at the deadline boundary. The loop
condition and the diagnostic's re-load are two separate reads, so a worker can
announce between them. Capture once and break rather than emit the
self-contradictory "N of N workers announced … never became ready" and tear
down a healthy pool.

4. Check the deadline on every yield, not on CLOCK_CHECK_STRIDE. See
below — this one was found by a surviving mutation and is the most valuable
change in the PR.

The stride defect

Mutation M7 set the deadline (100ms) below the injected worker delay (300ms)
and the test still passed. It should have been impossible to survive. Rather
than dismiss it, I instrumented the barrier:

spins=4141  elapsed=312ms  deadline=100ms

The yield phase starts at spin 4096. The next multiple of CLOCK_CHECK_STRIDE
(64) is 4160, which was never reached. The deadline was evaluated exactly
once, early, and then never again — a build 3× past its deadline completed as
if nothing were wrong.

The stride is correct for the spin phase, where an iteration costs nanoseconds
and Instant::now() would dominate. It is wrong for the yield phase, where an
iteration costs microseconds to milliseconds, because it multiplies the
deadline's granularity by 64 yields of an already-starved thread. The condition
that makes yields slow is CPU starvation — the exact livelock the backstop
exists to escape. A stride tuned for cheap iterations had silently become a
multiplier on the failure it guarded: the check was rarest precisely when it
mattered most.

CLOCK_CHECK_STRIDE is unchanged and still used by worker_wait, where it is
correct.

Memory ordering

ready.fetch_add(1, AcqRel) in worker_loop now carries a comment explaining
what is actually load-bearing about it — the Release half. It orders each
worker's Relaxed pin_failed store before the builder's Acquire load of
ready.

The first version of that comment claimed Release alone would be a data race
and that the Acquire half was required to chain each RMW to the previous
one's. That is false, and a second review round caught it. A release
sequence headed by a release store extends through every subsequent
read-modify-write on that location whatever ordering those RMWs use, and
every announcement here is an RMW — so the final fetch_add that brings
ready to total_threads lies in the release sequence headed by each
worker's store, and the builder's Acquire load of that value
synchronizes-with all of them. Release alone suffices; the Acquire half is
a harmless superset, not a requirement.

The ordering is unchanged — the code was correct, the stated reason was not.
Worth fixing rather than shrugging at, because this file uses ordering comments
as its correctness documentation and reuses this same counter pattern
elsewhere; a reader who believes release-sequence chaining requires Acquire
will mis-reason about those.

The panic path's shutdown.store(SeqCst) is sequenced-before each
sense.fetch_add(Release) + wake_all, so there is no lost-wakeup window —
atomic_wait::wait re-checks under the futex bucket lock.

Not joining is sound: handles are dropped (detached) during unwind, the
Arc<SharedState> keeps the pointee alive, and no job is ever published in a
failed build, so a woken worker returns on the shutdown check before touching
the UnsafeCell.

Tests

Four new tests plus a /proc helper.

test	proves
`a_worker_that_never_announces_fails_the_build_instead_of_spinning`	the unsatisfiable case panics instead of hanging
`a_healthy_pool_never_trips_the_readiness_backstop`	slow ≠ broken; asserts `elapsed >= DELAY_MS` so the barrier provably waited
`a_failed_build_leaves_no_workers_running`	no worker leak after a failed build
`ready_leak_child`	the child half of the above (`#[ignore]`, linux-only)

Two details worth recording:

The healthy-pool test was vacuous as first written, and review caught it.
Real workers announce inside the spin budget, so the deadline was never
consulted — it would have passed just as happily with a 1ms timeout. It now
injects a 300ms per-worker delay against a 5s deadline, which forces the
yield/clock path to actually execute. That is also what made the stride defect
observable at all.

The leak test needs a child process. /proc/<pid>/task/*/comm truncates at
15 bytes and onnx-genai-spmd is exactly 15, so every worker of every pool in
a process is indistinguishable — per-worker identity by thread name is
impossible. The child forces blocktime=0 so its workers are parked, not
spinning, which is the state the old code could not wake. (Checked against false
positives: the other pool names threads onnx-genai-decode-*, truncating to
onnx-genai-deco.)

Test-only fault injection is thread-scoped, and that is not cosmetic

The knobs (POOL_READY_TIMEOUT_MS, FAIL_WORKER_BEFORE_READY,
DELAY_WORKER_BEFORE_READY_MS) are #[cfg(test)] thread_local! Cells latched
on the builder thread before the spawn loop and moved by value into each
worker closure.

They were global statics first, and that was a defect: with --test-threads=2
an injected worker panic reached whichever unrelated pool happened to be
building concurrently, which is how this branch broke
an_idle_gap_far_longer_than_the_blocktime_parks_the_workers (120s hang). A
Mutex around the injecting tests does not fix it, because the tests being
corrupted never take it. Thread scoping needs no lock and cannot be forgotten by
a future test. Test time went 120s → 0.43s.

Mutation battery: 9/9 effective

Every mutation was verified to actually apply — a Python str.replace with no
match returns the original silently, so an unapplied mutation looks exactly like
a surviving one. The battery reports SCRIPT BUG for an absent target rather
than counting it as survived; that fired twice and both were real script errors,
not real survivals.

#	mutation	caught by
M1	restore the verbatim original unbounded `spin_loop()` barrier	hangs — timeout enforced the bound
M2	deadline never checked (`if false`)	hangs — timeout enforced the bound
M3	diagnostic drops the arrival count	`a_worker_that_never_announces_…`
M4	test deadline override ignored (production 120s only)	`a_worker_that_never_announces_…`
M5	barrier reports success without waiting (`while false`)	all three barrier tests
M6	no wake on giveup (parked workers leak)	`a_failed_build_leaves_no_workers_running`
M7	deadline shorter than a healthy pool's startup	`a_healthy_pool_never_trips_the_readiness_backstop`
M8	injected startup delay ignored (healthy test goes vacuous)	`a_healthy_pool_never_trips_the_readiness_backstop`
M9	ready-at-deadline `break` widened to swallow a real failure	`a_worker_that_never_announces_…`, `a_failed_build_leaves_no_workers_running`

M1 and M2 are the controls that matter most: they fail as a hang, not as a
test failure, which is the actual shape of the outage. M7 is the mutation that
survived the first battery and exposed the stride defect above; it is caught
now.

Validation

All on this branch after a normal merge of origin/main (no rebase).

cargo fmt --check clean
clippy -D warnings, default features and --features mlas, --all-targets
-p onnx-runtime-ep-cpu full: 1670 lib + 12 suites, 0 failed
--features mlas decode_spmd: 86 passed
ORT plugin e2e conformance: 56 passed
aarch64 under qemu: 1562 passed, 0 failed (full lib suite, taskset-bounded
to 8 CPUs, --test-threads=2, hard timeout, run on a verified-clean host)
Miri (-Zmiri-disable-isolation): both barrier tests + all 34
decode_affinity tests clean
Mutation battery re-run on the final tree: 9/9 caught, zero stray processes
Two independent adversarial Opus reviews. Round 1 (on the barrier itself):
APPROVE, no CRITICAL/HIGH/MEDIUM; its LOW (the two-reads race) and its
vacuity finding are fixed above. Round 2, commissioned specifically because
the stride fix was production code round 1 never saw: REQUEST CHANGES on
the false ordering comment, now corrected. Round 2 explicitly cleared the
every-yield clock cost (the yield phase is unreachable on a healthy pool),
the u32 spin wrap, the break's synchronisation, the thread-local
latching, and the hot decode path.

Rebased onto #1826, which broke this branch silently

While this PR was in review, #1826 landed in the same file and split the
per-node sense line into a NodeSense struct with separate ops and wake
words. The merge of main was textually clean — the two changes touch
different regions — and semantically broken: my readiness backstop still bumped
the node sense as a bare atomic. It failed at compile time rather than at run
time, which is the good outcome, but it is worth naming as the reason this
branch was re-validated end to end rather than trusting the earlier matrix.

The panic path now follows the new contract exactly as shutdown() does: bump
only the wake word and futex-wake on it, leaving ops untouched so a woken
worker can still distinguish teardown from a published op and will not re-run a
retired job. It is deliberately not routed through begin_shutdown, which
first waits out SHUTDOWN_DISPATCH_QUIESCE for an in-flight dispatch to drain:
nothing can be in flight here, because the pool has never been returned to a
caller and no job has ever been published. Adding a timed wait to the failure
path whose whole purpose is to stop waiting would defeat the backstop.

Mutation M6 was retargeted to the new sense API and still fires, so the leak
test is proven to still catch a missing wake under #1826's structure.

Disclosures

One unexplained failure. Early on, the mlas lane failed once (78 passed, 1
failed) and passed on rerun. I lost the test's name to a grep filter and could
not reproduce it in 18+ subsequent reps, quiet and under 4× CPU
oversubscription. My contention hypothesis was falsified. Recording it rather
than quietly re-rolling past it.
Two of my own validation runs were discarded as contaminated, not
reported: two mutation batteries ran concurrently while an aarch64 build read
the same mutated sources, and one battery read the other's mutation, producing
a bogus 4/5. Both runs were binned and re-run serially. Never run two
batteries at once, or one alongside any build reading the same tree.
Stopping a tool shell does not kill its process tree. Killing the aarch64
run left qemu-aarch64-static children still burning cores — the same 5h40m
pattern in miniature. Reaped explicitly by PID.
My own mutation battery reproduced the outage. subprocess.run(timeout=)
kills only the direct child, so when the hang-mutations (M1, M2) did what they
are designed to do, timeout reaped the cargo wrapper and left its test
binaries orphaned with ppid=1. Ten of them accumulated across runs and held
~289% CPU on this shared host for over five hours before I found them in
ps — the same pattern, the same file, the same failure to bound a wait, this
time in my own harness rather than in the product. All reaped by explicit PID.
The battery now runs each mutation in its own process group, killpgs on
timeout with SIGTERM→SIGKILL, and verifies the reap with pgrep -g instead
of assuming it. Re-verified: M1 still caught, zero survivors.
That load overlapped the one unexplained mlas failure above. It is a
plausible cause and I am recording the correlation, but I did not reproduce
the failure and cannot claim causation — the test name was already lost.
Correctness greens are not invalidated by that contention. Contention
produces false reds, not false greens; the one timing-sensitive test
(300ms delay against a 5s deadline) would flake toward failure, not success.
The aarch64 lane was nevertheless re-run from scratch on a verified-clean host.
The ready-at-deadline break is asserted by reasoning, not by a test.
Reaching it requires a worker to announce in the window between two adjacent
atomic loads, which no deterministic test can force. Both reviews agree the
branch is sound and that its Acquire re-load gives it exactly the
synchronisation the normal loop exit gives; recording that it ships uncovered.
An earlier revision of this PR body carried an invented mutation table. I
wrote plausible-sounding mutation descriptions from memory instead of reading
the battery, including one row (AcqRel → Relaxed) that does not exist in
it. The table above is now generated from an actual run. Flagging it because a
fabricated test table is worse than no table.
The exact mechanism of the original 5h40m hang (starvation vs. a pre-loop
worker panic) was never directly reproduced. Both are live against the old
code and both are fixed; I cannot say which one fired.

The persistent SPMD pool ended its build with an unbounded `spin_loop()` barrier: no yield, no deadline. Two distinct failures reach it. Starvation livelock: the barrier waits on threads that need a core, so wherever builder and workers contend -- qemu-user, a cpuset-confined process, or `cargo test` building several pools at once -- the spinner starves the very workers it is waiting for. Permanently unsatisfiable: `worker_loop` indexes `worker_node` and reads `decode_blocktime()` before `ready.fetch_add`, so any pre-loop panic means the count can never complete and the barrier spins forever. Either way it burns every core it holds, indefinitely, and is indistinguishable from work from outside. One such run held a shared host for 5h40m at ~1778% CPU -- roughly 50x the suite's 7-minute runtime -- and silently blocked two other agents, costing one of them a full A/B. Bounded spin -> `yield_now` -> deadline, then a panic naming how many workers arrived. This is the pattern `worker_wait` already uses; the build barrier was the one wait in this file that never got it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…es up The backstop panicked after setting only the stop flag. A worker already parked on the futex never re-reads that flag, so a failed build left its started workers parked for the life of the process -- measured at 5 of 6. A guard that stops an unbounded spin by leaking threads has relocated the defect, not fixed it. Publish-then-wake, matching `shutdown()`. Deliberately not joined: a build that failed this way may have a wedged worker, and blocking on it would reintroduce the unbounded wait the backstop exists to remove. Proved by a child process, because thread identity is unavailable in-process -- `/proc/<pid>/task/*/comm` truncates at 15 bytes, so every worker of every pool reports `onnx-genai-spmd` and a concurrent test's pool cannot be told apart from this one's. The child forces blocktime 0 so its workers are parked rather than spinning; on the stop flag alone they stay parked and the test reports 5 survivors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…rrier

codecov · 2026-08-23T08:52:33Z

Codecov Report

❌ Patch coverage is 92.54658% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.02%. Comparing base (cb81745) to head (d178275).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/onnx-runtime-ep-cpu/src/decode_spmd.rs	92.54%	9 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1825      +/-   ##
==========================================
- Coverage   80.25%   80.02%   -0.23%     
==========================================
  Files         412      412              
  Lines      201780   197605    -4175     
  Branches   201780   197605    -4175     
==========================================
- Hits       161932   158139    -3793     
+ Misses      34324    33972     -352     
+ Partials     5524     5494      -30

Flag	Coverage Δ
cli-ort-linux	`72.47% <ø> (ø)`
cli-ort-windows	`72.06% <ø> (ø)`
mlas	`85.20% <ø> (+0.09%)`	⬆️
offline	`80.15% <92.54%> (-0.24%)`	⬇️

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	`90.30% <92.54%> (-0.10%)`	⬇️

... and 51 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.

Barrier: the loop condition and the diagnostic's re-load are two separate reads, so a worker can announce between them. Accept that pool instead of tearing down a healthy one and reporting the self-contradictory 'N of N workers announced ... never became ready'. A condition that just became satisfiable is not the case this backstop exists for. The healthy-pool test was vacuous. Real workers announce inside the spin budget, so the barrier never reached its clock check and the deadline was never consulted -- the test passed just as happily with a 1ms timeout as a 120s one. A test-only per-worker delay now pushes the barrier onto its yield/clock path with the deadline genuinely in play, and the test asserts the build actually waited, so 'slow is not the same as broken' is finally something the test can fail on. Also document why worker readiness is AcqRel rather than Release: the Acquire half is what orders every worker's pin_failed store ahead of the builder's Relaxed read of it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…ride The deadline was gated on `spins.is_multiple_of(CLOCK_CHECK_STRIDE)`. That stride is right for the spin phase, where an iteration costs nanoseconds and Instant::now() would dominate. It is wrong for the yield phase, where an iteration costs microseconds to milliseconds, because it multiplies the deadline's granularity by 64 yields of an already-starved thread. Measured, with a 100ms deadline and workers delayed 300ms on a contended pair of CPUs: the loop reached spin 4141 in 312ms. The yield phase starts at 4096, so the only multiple of 64 it ever saw was 4096 itself. The deadline was evaluated once, early, and never again -- a build that had blown its deadline by 3x completed as though nothing had happened. The condition that makes yields slow is CPU starvation, which is precisely the livelock this backstop exists to escape. So the check was rarest exactly when it was needed most: a stride tuned for cheap iterations had silently become a multiplier on the failure it was guarding. Found because a mutation -- deadline shorter than a healthy pool's startup -- survived. That should have been impossible; investigating why it was not produced this. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

An independent review found the comment on `ready.fetch_add(AcqRel)` asserts something false: that plain `Release` "would leave only the release-sequence head ordered and make that read a race". It would not. A release sequence headed by a release store extends through every subsequent read-modify-write on that location regardless of the ordering those RMWs use, and every announcement here is an RMW. So the final `fetch_add` that brings `ready` to `total_threads` lies in the release sequence headed by *each* worker's store, and the builder's `Acquire` load of that value synchronizes-with all of them. `Release` alone is sufficient; the `Acquire` half is a harmless superset, not a requirement. This matters because this file uses ordering comments as its correctness documentation and reuses this same counter pattern elsewhere. A reader who believes release-sequence chaining requires `Acquire` will mis-reason about those, and cannot safely simplify this one even where the model allows it. The ordering itself is unchanged -- the code was correct, the reason was not. Also record that the ready-at-deadline `break` is asserted by reasoning rather than by a test: reaching it needs a worker to announce between two adjacent atomic loads, which no deterministic test can force. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…rrier

The merge of main was textually clean and semantically broken: #1826 split the per-node sense line into a `NodeSense` struct with separate `ops` and `wake` words, and my readiness backstop's release path still bumped the node sense as a bare atomic. Different regions of the file, so no conflict -- it failed at compile time instead. Follow the new contract exactly as `shutdown()` does: bump only the `wake` word and futex-wake on it, leaving `ops` untouched so a woken worker can still distinguish a teardown from a published op and will not re-run a retired job. Deliberately not routed through `begin_shutdown`: that waits out SHUTDOWN_DISPATCH_QUIESCE for an in-flight dispatch to drain, and nothing can be in flight here -- the pool has never been returned to a caller and no job has ever been published. Adding a timed wait to the failure path whose purpose is to stop waiting would defeat the backstop. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…n force (#1851) ## What this fixes The SPMD decode pool prints its width and calls it verified: ``` decode_width requested=16 realized=16 path=spmd-pool as_requested ``` The **count** is honest. The **placement is never reported and never checked**, and nothing in the test suite compares a placement the pool claims against the affinities the kernel actually enforced. That gap is #1792 concretely: `ONNX_GENAI_CPU_DECODE_AFFINITY` is **inert** on the default SPMD path — `off`, `numa-split`, `node:0` and an explicit CPU list all produce byte-identical placement, verified from `/proc`. The only user-facing placement control silently does nothing, placement is worth ~7% latency and ~31% CPU (#1729), and **no test failed**, because no test ever asked the question. This adds the missing question. ## The invariant `worker_cpus()` — what the pool reports — is cross-checked against `/proc/self/task/*/status` `Cpus_allowed_list` — what the kernel enforced — and the two must agree. Threads are matched as a **multiset, never per-worker**. `/proc/<pid>/task/*/comm` truncates at 15 bytes and `onnx-genai-spmd` is *exactly* 15, so every worker of every pool in the process is indistinguishable by name. Per-worker identity is not merely inconvenient here, it is unavailable. Multiset equality — "these N CPUs are the ones actually in force" — is the whole property worth asserting anyway, and it does not require identity. A pinned thread is confined to exactly one CPU, so an observed list wider than one element is a pin the kernel did not apply. That is scored `Some(false)`, not `None`: it is precisely the dishonesty the predicate exists to catch. ## The split that matters: policy-neutral vs policy-dependent The assertion is deliberately in two halves, and **this PR corrects my own merged #1805**, which asserted the second as if it were the first. **(1) Policy-neutral, always required.** The placement the pool reports is the placement in force. A runtime that reports `realized=16 as_requested` and then runs somewhere else is wrong under *every* policy. Nothing in this half says where workers ought to run. **(2) Policy-dependent, and now labelled as such.** One worker per physical core is #1729's *spread* policy, not a law. It is measured better on a quiet host and measured **~26% worse with a single ~90% co-tenant**, because a compact pool leaves half the box for the co-tenant to land on. The standing user direction (`.squad/decisions/inbox/copilot-cpu-shared-host-default-2026-08-23.md`, via #1729) is that **a policy which wins only under exclusive quiet-host conditions is not a valid default**. So this assertion may legitimately have to change — and the comment at the assertion site says so, with the measurement and the directive cited, so that whoever changes the policy changes the test *deliberately* rather than discovering it as a mystery failure. It must not be "fixed" by loosening it to match whatever the pool did, which would return it to asserting nothing. Live on an 8-CPU / 4-core cpuset, the row that demonstrates the split is width 8: ``` requested=1 workers=1 cores=4 placement=1 honest=1 requested=2 workers=2 cores=4 placement=1 honest=1 requested=4 workers=4 cores=4 placement=1 honest=1 requested=8 workers=8 cores=4 placement=0 honest=1 <- policy cannot hold; honesty still does ``` Eight workers cannot occupy four cores one-per-core, so the **policy** verdict is correctly `0` while the **honesty** verdict is still `1`. The two halves are measurably independent, not two names for one check. ## Why you should believe the assertion is connected to anything A real pool on a healthy host is honest, so this assertion would otherwise only ever be observed *passing* — and an assertion nobody has seen fail is an assertion nobody has shown is wired up. So there is fault injection (test-only by construction; the whole module is `cfg(test)`) that makes a child claim a CPU it is not on — exactly the #1792 shape — and a negative test that drives **the same** assertion via `catch_unwind`. Same helper, not a similar-looking copy: a check the negative test does not go through is a check the negative test does not cover. ``` injection off (control): placement=1 honest=1 injection on : placement=1 honest=0 <- only `honest` moves ``` The injection env is **never inherited**: it is explicitly set or `env_remove`d on every child spawn. Inherited, it would make the sweep's assertion meaningless if it could switch on, and unfalsifiable if it could switch off. ## The anti-vacuity guard, and the one I threw away `assert_placement_is_honest` is a no-op on `None`, and `None` is the *correct* answer for an unpinned or partially pinned pool. So the check can stop running and keep reporting `ok`. My first guard accumulated `saw_honesty_check |= verdict.is_some()` across the sweep and asserted it once at the end. **The mutation battery showed it was never load-bearing**, and I removed it rather than ship it: - its precondition (`allowed >= 2 && host().is_some()`) is *identical* to the negative test's skip condition, and that test's control arm already asserts `Some(true)` at width 2 — strictly stronger than `is_some()`; - being an aggregate, **one width satisfied it for all five**. I could not construct a mutation that only it caught, which is the definition of a decorative check. It is replaced by a **per-width implication**: a width whose child reported a fully pinned pool (`distinct_cores.is_some()`, i.e. every worker pinned and topology readable, which is exactly when the `/proc` cross-check is answerable) must have produced a verdict. It is an implication rather than a bare `is_some()` because demanding a verdict from an unpinned pool would assert a pinning policy this half is specifically not allowed to assert. Then the discriminating control the old guard never had — *suppress verdicts for every width except the one the negative test uses*: | arm | result | |---|---| | suppression only | **CAUGHT**, and only by the sweep — the negative test does not see it | | suppression + guard removed | **SURVIVED** | That pair is the proof the new guard is load-bearing. ## Mutation battery: 9 / 10 | # | mutation | caught by | |---|---|---| | P1 | verdict always honest | verdict unit test, negative e2e | | P2 | an unapplied pin treated as unanswerable, not dishonest | verdict unit test | | P3 | partially pinned pool scored on its pinned subset | verdict unit test | | P4 | `/proc` cross-check disabled (verdict never produced) | sweep, negative e2e | | P5 | honesty assertion never fires | negative e2e | | P6 | per-width anti-vacuity guard removed | **survives alone — by construction** | | P7 | injected dishonesty is a no-op (negative test goes vacuous) | negative e2e | | P8 | dishonesty env leaks into the sweep's children | sweep, negative e2e | | P9 | verdict produced only when dishonesty is injected | sweep, negative e2e | | P10 | verdict suppressed for every width except the negative test's | **sweep only** | P6 cannot fail alone: removing a guard breaks nothing until something makes its condition false. Its evidence is the **P10+P6** pair above, not a solo run. P3, P5 and P9 exist because earlier versions of these tests survived them — each survivor was a genuine weakness (a length check masking a case, an unfalsifiable assertion, a silently vacuous sweep), not battery noise. This table is generated from an actual battery run. ## Validation All on this branch merged with latest `main`: - `cargo fmt --all --check` ✅ - clippy `-p onnx-runtime-ep-cpu --all-targets`, default **and** `--features mlas` — **0 warnings** ✅ - `cargo test -p onnx-runtime-ep-cpu` — **1672 lib + 12 suites, 0 failed** ✅ - `--features mlas` `decode_spmd` — **86 passed** ✅ - ORT plugin e2e conformance — **56 passed** ✅ - Miri, `decode_affinity` — **34 passed**, no UB ✅ - aarch64 under qemu, `--lib` — **1564 passed, 0 failed** ✅ - mutation battery — **9/10**, with the P10+P6 control ✅ Scope note: no `unsafe` and no interior mutability is added; the new code is `cfg(test)` only and the pool's hot path is untouched. Host discipline, after my own #1825 incident: the aarch64 run is `taskset`-bound to 8 CPUs, `--test-threads=2`, `setsid`-isolated, under a hard `timeout`, and verified stray-free with `pgrep -g` afterwards. Refs #1792, #1729, #1805. Complements #1825. --------- Co-authored-by: resch <resch@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…1825 missed (#1868)

… it fires (#1919) ## What Adds one test pinning that the readiness backstop fires *within* its deadline, and a `#[cfg(test)]` knob that makes the failing regime reachable. Follows #1868 (@gaff-1) and #1825. Both fixed the same defect at different sites; this guards the site that had a measured 3x deadline overrun. No production behaviour changes — the only non-test code is a `#[cfg(test)]` block. ## The defect, and why nothing caught it Both PRs fixed a deadline evaluated on a `spins.is_multiple_of(64)` stride inside a **yield** phase that begins at `SPIN_LOOP_BUDGET`. 4096 is itself a multiple of 64, so the clock was read once — on the first yield — and then not for 64 more. `decode_spmd.rs:1339` records the consequence in the source: *"a build that had blown its deadline by 3x completed as if nothing was wrong."* Both fixes are correct. **Neither is guarded.** Reverting both leaves the crate green: ``` $ # stride reinstated at BOTH sites $ cargo test -p onnx-runtime-ep-cpu test result: ok. 1691 passed; 0 failed # + 54 across 13 more suites = 1745 ``` The two tests that look like they cover this do not: | test | why it is blind | |---|---| | `a_worker_that_never_announces_fails_the_build_instead_of_spinning` | bounds the *test* at 30s against a 250ms deadline — 120x. Its own comment says so: *"Bounds the test, and with it the claim."* | | `a_healthy_pool_never_trips_the_readiness_backstop` | 5s deadline, 300ms delay. A 64-yield overshoot stays far inside the margin. | | `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. Invariant under the defect. | | `back_to_back_dispatches_are_caught_by_the_spin_window` | exercises the *spin* phase, which #1868 deliberately left strided. Never reaches the yield phase. | ## Why the obvious test doesn't work My first attempt asserted the right thing and **passed against a reinstated stride.** I nearly shipped it. The defect is invisible when yields are cheap. An uncontended `yield_now` costs ~1.2µs here, so a stride of 64 moves the deadline by ~78µs — nothing an assertion against a millisecond deadline can see. The production measurement was **~7ms per yield** on contended CPUs, three orders of magnitude larger. The defect lives in a regime, and the regime — not the deadline — is what has to be injected. Manufacturing real contention in a unit test is exactly the load-dependent arrangement that flakes. So `SLOW_YIELD_US` injects the yield *cost* instead: a `#[cfg(test)]` thread-local beside the knobs already there for this purpose, read on the builder thread, compiled out of production. With 20ms yields, a stride cannot re-consult the deadline until `64 × 20ms = 1280ms`, while the workers announce at 800ms and the loop exits first. **Fires-late becomes never-fires** — the observable is panic-versus-success, not a duration. ## Falsification ``` stride reinstated -> the_readiness_backstop_fires_within_its_deadline_not_a_stride_later FAILED "the barrier reported success after 804.460969ms" a_healthy_pool_never_trips_the_readiness_backstop ............ ok <- the blindness stride absent -> both ok, 20/20 consecutive runs ``` The 8x gap between when the barrier must give up (~100ms) and when it is let off the hook (800ms) is deliberate headroom: this can only fail because the deadline went unconsulted, never because a runner descheduled the builder thread. ## Validation `cargo fmt --check` clean · `clippy --all-targets -D warnings` **0** · **1746 passed / 0 failed** (lib 1692, was 1691 — exactly this test). ## Scope / limitation The sibling site in `worker_wait` — the one #1868 fixed — is **left unguarded, deliberately.** Its deadline is `decode_blocktime()`, latched process-wide in a `OnceLock` at 500µs, so the window where the defect is observable sits between one and 64 yield costs. That is a sub-millisecond timing assertion, which `decode_spmd.rs:5352` already documents as runner-sensitive, and a flaky test in a required lane is worse than none. Closing it properly needs a blocktime override plumbed through `build_with_schedule` the way `delay_worker_before_ready` already is. Called out on #1868 rather than half-done here. I also checked whether a third instance of the species is still live: `is_multiple_of` co-located with a clock read appears exactly once more in the workspace, `task_runtime/pool.rs:408`, which is the spin phase #1868 correctly kept (4096 iterations × ~31ns against a ~32ns clock read — the stride genuinely amortises there). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@Gaff

…ad (#1933) 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_loop`s) 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.)* --------- Co-authored-by: gaff <gaff@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…e check (#2022) ## What this is #1868 corrected the spin-window deadline at **two** sites — it kept the stride in the pure-`spin_loop` phase and dropped it in the yield phase, in both `decode_spmd`'s readiness barrier and `task_runtime::pool`'s worker loop. **Only the `decode_spmd` site got a test.** This adds the missing guard for the other half. I found this while validating merged code, not while reading the diff. The finding is a mutation result, not an opinion about coverage. ## The gap, measured Baseline on `main`: `cargo test -p onnx-runtime-ep-cpu` → **1718 passed / 0 failed**. | mutation (reintroduce the stride in the yield phase) | result | |---|---| | both sites (`decode_spmd` + `pool.rs`) | **1717 / 1** — `decode_spmd::dispatch_claim_tests::the_blocktime_deadline_is_evaluated_on_every_yield_not_on_a_stride` | | `pool.rs` only (`decode_spmd` restored) | **1718 / 0 — SURVIVED** | One test covers both mutations only because one of them was never covered. The `task_runtime::pool` half of #1868 was held in place by nothing but the comment beside it. ## Why it is worth a test rather than a comment `SPIN_LOOP_BUDGET` (4096) is an exact multiple of `CLOCK_CHECK_STRIDE` (64), so the yield phase begins **on** a stride boundary and the next evaluation is 64 yields away. `MAX_SPIN`'s stated contract is ~0% CPU within a millisecond of going idle. Under the contention that makes a yield expensive — microseconds to milliseconds rather than the ~1.2us an uncontended one costs — a strided check holds a core for most of a second. On a shared box that cost lands on a co-tenant, i.e. it fails hardest in exactly the regime it exists for. ## How it is tested `spin_for_dispatch` is split out of `worker_loop` verbatim (the loop body is unchanged except `break` → `return SpinOutcome::*` and `thread::yield_now()` → `spin_yield()`, which *is* `thread::yield_now()` in a non-test build). The split is what makes the policy drivable: a real worker runs this loop on a thread the test does not own, so the only observable from outside the pool is wall-clock idle CPU, which does not discriminate on this box. Yield cost is injected through a `#[cfg(test)]` thread-local — the same technique `decode_spmd`'s test uses. Thread-scoping is what makes it sound: a real pool's workers never set it and always read `0`. **The observable is the yield count, deliberately.** It is monotone in the right direction under load — a starved thread accumulates wall time faster per yield, so it crosses the deadline in **fewer** yields, never more. Contention can therefore never turn a real failure into a pass. A wall-clock observable ("was it parked at T?") is not monotone that way and goes flaky beside 1700 siblings. `TaskPool::new(1)` spawns no threads, so the `Shared`'s epoch cannot move under the test. ## Falsified in both directions, on this exact tree - correct code → `test result: ok. 15 passed; 0 failed` (`--lib task_runtime::pool`) - stride reintroduced at the yield site → **14 passed / 1 failed**: > left the yield phase after **65** yields against a 200ms window and 10ms yields, i.e. the clock was not re-read on every yield **65 is the predicted number**, not a threshold tuned to fail: spins resume at 4097 and the next multiple of 64 is 4160. The every-yield form exits at ~20 (200ms / 10ms). The test also asserts **non-vacuity explicitly** (`yields >= 2`, with an "it is not a pass" message). If the window had already elapsed when the yield phase began, both the strided and unstrided forms exit on the first yield and the test discriminates nothing — that state must be reported as inconclusive, not green. This is the #1817 class, so the guard should not be able to join it. ## Local validation Rebased onto `1bf87c86a`, run under `scripts/hostlock.sh run --wait` (live PID anchor, no TTL), `taskset -c 8-15`, `CARGO_INCREMENTAL=0`: ``` cargo fmt --all -- --check -> clean cargo clippy -p onnx-runtime-ep-cpu --all-targets -- -D warnings -> 0 cargo clippy -p onnx-runtime-ep-cpu --all-targets --features mlas -- -D warnings -> 0 cargo test -p onnx-runtime-ep-cpu -> 1719 passed / 0 failed / 24 ignored (+ 11 smaller suites green) ``` 1719 = the 1718 baseline + this test. Clippy is run on **both** feature arms because a `#[cfg(test)]` helper used by only one arm is dead code in the other, and #1973 made default-feature linting a required-lane concern. Local validation is necessary and not sufficient — this waits for the required GitHub checks and merges by normal auto-merge. No admin bypass. Refs #1868, #1825, #1817. --- ## Update: the Miri lane caught this, and it caught it the right way The first CI run went red on `Miri unsafe-crate soundness` — **on this test's own non-vacuity assertion**, not on a passing-but-empty green: ``` ---- task_runtime::pool::tests::the_spin_window_deadline_is_evaluated_on_every_yield_not_on_a_stride ---- inconclusive: left the yield phase after 0 yield(s), so the 200ms window had already elapsed before the second check. This test cannot tell a strided clock read from an unstrided one in that regime — it is not a pass ``` **Miri makes the test's premise false rather than its assertion wrong.** The test needs the spin phase to be short against the window — 4096 `spin_loop`s measure 128us natively against a 200ms window, so the yield phase is reached with nearly the whole window left. Under the interpreter those 4096 iterations and their strided `Instant::now()` calls outlast the window, so the yield phase is entered *already expired* and the strided and every-yield forms both exit at yield 0. That is precisely the regime the `yields >= 2` assertion was written to refuse, and without it this run would have been a **silent green in the lane where the test discriminates nothing** — the #1817 shape, in the guard for an #1817 instance. I would rather report the mechanism than the outcome: I did not predict Miri specifically, I asserted the condition the test depends on, and the condition is what failed. Fixed with `#[cfg_attr(miri, ignore = "spin-vs-window ratio is wall-clock, not emulated")]`, matching the precedent directly below it in the same module — `workers_park_when_idle_and_wake_again`, ignored under Miri for the same class of reason (a wall-clock policy, not a memory-model one). **It costs no coverage, checked rather than assumed:** ``` $ python3 .github/scripts/workspace_test_packages.py cargo-args offline-linux | tr ' ' '\n' | grep -x onnx-runtime-ep-cpu onnx-runtime-ep-cpu ``` `ci.yml:224` runs that package set in **`Fast (Linux x86_64)`, a required lane**, natively. The guard runs where its premise holds. Reproduced locally with the exact lane command (`miri.yml:137`), under hostlock and `taskset -c 8-15`: ``` MIRIFLAGS=-Zmiri-disable-isolation cargo +nightly miri test --locked -p onnx-runtime-ep-cpu --lib task_runtime:: -> 30 passed; 0 failed; 2 ignored (was 30 passed; 1 failed; 1 ignored) cargo test -p onnx-runtime-ep-cpu --lib task_runtime::pool -> 15 passed; 0 failed (the guard still runs, and still passes) ``` --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…ils loud instead of burning cores (#2027) Closes #2067. ## What A thread-pool constructor waited for its workers to announce readiness with an **unbounded pure-spin loop**. One worker that dies or wedges before entering its loop makes the condition permanently unsatisfiable, and the builder then burns every core it holds *forever*. That is strictly worse than deadlocking quietly, because full occupancy is indistinguishable from work: an aarch64 qemu lane sat in it for **5h40m at ~1778% CPU** on a shared 32-CPU host and was noticed only by someone reading `/proc`. Two production sites had this shape: - `TaskPool::new` — `crates/onnx-runtime-ep-cpu/src/task_runtime/pool.rs` - `WorkStealingThreadPool::new` — `crates/mlas-sys/src/work_stealing_pool.rs` ### Correction to the original report The assignment named `build_with_schedule` in `decode_spmd.rs`. That barrier is **already bounded** — by #1825, with follow-ups #1845 and #1933 — and is untouched here. The two above were the genuinely unbounded ones. ## How Spin for a budget, then **yield**, then give up **loudly**, checking the deadline on **every yield**: - The stride is right for the spin phase, where an iteration costs nanoseconds and `Instant::now()` would dominate. It is wrong in the yield phase, where a yield under contention costs microseconds to milliseconds, so a stride of N multiplies the deadline's granularity by N yields of an already-starved thread. #1933 made exactly this correction to `decode_spmd`'s barrier. - On timeout, `abandon_unready_workers` publishes shutdown, bumps the epoch, wakes everyone, and drops the handles **without joining**. Joining would block on precisely the worker that is not coming, reintroducing the unbounded wait this exists to remove. - `TaskPool::new` panics; the mlas pool returns `io::ErrorKind::TimedOut`. `roy_validate.sh` step G2 gains a hard `timeout`, `taskset` confinement and bounded concurrency for the external aarch64 lane, so an unbounded wait there costs wall-clock rather than the host. Missing tools are **warned about**, never silently dropped. ## Instrumentation is free in production Every injection knob is `#[cfg(test)]` — thread-locals latched on the builder thread, and a `ready_yield()` whose entire body is `#[cfg(test)]`. Nothing new is read on a release path. ## Mutation evidence Every claim below was verified by reintroducing the defect and watching exactly one test go red. **All of these previously survived** the suite: | Mutation | Result | |---|---| | Strided clock check in the yield phase (ep-cpu) | `the_deadline_is_checked_on_every_yield_not_on_a_stride` — "yielded 65 times … not leaving before yield 64 is the signature of a strided check" | | Strided clock check (mlas-sys) | same test — "65 … more than 3x the 9 an every-yield check can take" | | `drop(handles)` → `join()` (ep-cpu) | `an_abandoned_worker_that_ignores_shutdown_does_not_delay_the_builder` | | `drop(workers)` → `join()` (mlas-sys) | same test | | Delete the `ready >= workers` race re-check (both) | `a_worker_that_announces_in_the_race_window_is_not_torn_down` — ep-cpu reports the self-contradictory "2 of 2 workers announced … never became ready" | ## Two independent reviews, and what they changed Both reviews found **false oracles in my own tests** — instruments whose failure value equalled their passing value. Fixed rather than deferred: **Opus.** The headline regression test bounded *elapsed time* at 1000 ms. At the injected 12 ms/yield a stride of 64 fires at ~780 ms — **under** that bound — so it passed with the #1933 defect reinstated. The comment even named the number and then put the bound on the wrong side of it. Now it discriminates on **yield count**, which is the property rather than a proxy: a wall-clock bound must sit above the healthy path (~110 ms plus whatever the host adds) and below the strided signature (~770 ms), an interval contention can close from below — at which point the test either reds on the host or gets widened past 770 ms and silently stops catching the stride. A stride of N cannot leave before its first multiple of N, and a slow host only makes each yield cost *more*, moving the count away from the failing value. Opus also found the **no-join contract had no oracle at all**: the injected held worker parks on `shared.shutdown`, the same flag the abandon path sets, so it is *cooperative* — a restored `join()` returns promptly and the suite stays green while production hangs. **Pris (tester).** Found that this PR closed the false oracles in **one of two identical barriers**: `mlas-sys` had no yield counter, no yield-cost injection and an unconditionally cooperative held worker, so both mutations survived its whole suite. Also found the **race re-check** (`ready >= workers`) uncovered in both, and that the no-join oracle was an absolute 2 s wall-clock bound in a test that runs under **qemu emulation** — an environmental red whose cheapest resolution is to widen it. The no-join oracle is now the worker's own **deaf flag**: correct code returns *during* the deaf window, a restored join only after it closes. Both sides scale with the host, so there is no threshold to breach. That flag is **generation-tagged**, which the first version got wrong and the test caught: a held worker from an earlier test outlives the constructor that abandoned it, and the store recording its exit can be descheduled past the next test's reset — two booleans reported the previous test's exit as this one's. ## Miri opt-outs, and why each is honest The Miri lane runs `--lib task_runtime::` **without** `-Zmiri-ignore-leaks`. - `a_pool_whose_workers_are_merely_slow_still_builds` — states a *race*: 150 ms of delay must outlast `SPIN_LOOP_BUDGET` spins. Miri's clock is virtual and a spin costs it no time, so the delay elapses inside the budget and the anti-vacuity guard fails on a healthy build. Weakening the guard would delete the only thing keeping the test from passing with the yield phase removed. The sibling tests need no opt-out and keep none: they inject a *hold*, so the builder exhausts the spin budget whatever a spin costs. - `an_abandoned_worker_that_ignores_shutdown_does_not_delay_the_builder` — leaves a deliberately deaf thread alive, which the remaining-threads check reports. - `the_live_scan_can_see_this_process` / `a_failed_build_leaves_no_workers_running` — `/proc` and a subprocess. The two properties that must stay covered under Miri **are**: wedge → fail loudly, and the every-yield check. Both run there with real spawned threads. ## Known limitation, disclosed rather than papered over The epoch bump in the abandon path is not deterministically covered. The teardown test only exercises workers already parked before `abandon`, which `wake_all` alone releases; the bump exists for a worker transitioning *into* the wait concurrently with `abandon`, which no test forces. It mirrors the established `shutdown()` publish-then-wake sequence. ## Validation `cargo fmt --check`; `clippy -D warnings` on x86_64 **and** aarch64 cross; **1752** `onnx-runtime-ep-cpu` + **46** `mlas-sys` lib tests; the **Miri** `task_runtime` lane. Run bounded (`taskset`, `-j4`) on a shared host. Full saturating qemu validation of step G2 is deliberately **not** run yet — it needs the host lock (#1806). Closes the hang described above. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

`worker_wait`'s active window spun 4096 times and then called `sched_yield` on every subsequent iteration until the blocktime expired. The intent, per its own doc, was politeness: release the core so a busy host can schedule other work while we finish the window. On the cpuset a decode budget confines the process to, there is nobody to release it to. `bound_process_to_decode_budget` pins the process to exactly its budget's CPUs, so mid-barrier every other runnable thread is a peer in this same loop. The yield finds nothing eligible, returns immediately, releases nothing, and still pays a syscall and a full scheduler pass. The politeness mechanism degenerated into a syscall storm that occupied the very cores it meant to hand back. Measured at width 16 on a 16-CPU cpuset, `int4_decode_loop_ab`, 20 runs per arm interleaved with an A/A null arm: | | main | this | |---------------------|--------------------|--------------------| | kernel time | 2.61 of 16 cores | 0.22 of 16 cores | | yields per 20ms | 14896 | 384 | | tokens/s | 279.6 | 276.2 | | total occupancy | 15.76 cores | 15.88 cores | Kernel time is disjoint between the arms (base [2.34, 3.25] against [0.19, 0.24]) and every paired ratio is far outside the A/A null envelope. Throughput overlaps and its paired ratios sit inside the null in both directions, so the storm was buying nothing. The core is still held -- this converts kernel time into user-space `spin_loop`, it does not park earlier -- so co-tenants gain the runqueue lock and the scheduler passes back, not the core. The separate question of whether the window earns its 500us at all is #2071, and is deliberately not answered here: it needs the inter-token gap regime the window was designed for, not the tight loop this bench runs. The deadline is now evaluated on a stride of 64 pure `spin_loop`s (~640ns) rather than on every yield. That is not the stride #1825 removed: that one was a stride of *yields*, each costing microseconds to milliseconds of an already-starved thread. The guarantee is strengthened, and `the_blocktime_deadline_is_evaluated_on_every_yield_not_on_a_stride` still passes unchanged. `slow_yield` now counts unconditionally and only sleeps when injected, because the new test has to count yields in the uninjected regime it asserts about -- injecting a sleep to make yields countable would set the interval under test. A counter that only counts when its subject has been perturbed cannot observe the unperturbed case, which is the same defect shape as #1736. Mutation-proved three ways, each killing only the new test: * yield on every iteration regardless of the interval -> 4884 yields, fails the rate bound * `YIELD_INTERVAL` set to 1ns -> 4837 yields, fails. This is why the ceiling is stated absolutely rather than derived from the constant under test: a derived ceiling moves with it and would have passed. * window shortened below the spin phase -> 0 yields, fails the non-vacuity assertion rather than passing on a bound nothing reached Note for anyone tempted to confirm the syscall count with `strace`: it cannot. At ~55us per traced syscall the instrument's own cost exceeds the 50us interval it is measuring, so both arms yield on every check and the difference collapses to 17797 against 13701. The in-process counter is the only instrument here that does not set the property. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

## The defect `SharedState::worker_wait` spins 4096 times on `spin_loop`, then calls `thread::yield_now()` on **every subsequent iteration** until the blocktime window expires. Its own doc says why: > then relax to `yield_now` so a busy host can schedule other work while we finish out the blocktime window On the cpuset a decode budget confines the process to, there is nobody to hand the core to. `bound_process_to_decode_budget` (`provider.rs:344`, at EP `initialize()`) pins the process to exactly its budget's CPUs and caps the global Rayon pool to match. Mid-barrier, every other runnable thread is a peer in this same wait. `sched_yield` finds nothing eligible, returns immediately, releases nothing — and still pays a syscall and a full scheduler pass. The politeness mechanism was occupying the cores it meant to hand back. ## Measurement `int4_decode_loop_ab`, width 16, production-confined, `PROBE_TOKENS=1024 PROBE_REPS=2`. Two binaries from the same tree differing only in `worker_wait`. 10 rounds of `[base, fixed, base', fixed']` so host drift lands inside every contrast, with `base` vs `base'` as the A/A null. n=20 per arm. | | main | this PR | paired ratio (A/B) | A/A null | resolvable? | |---|---|---|---|---|---| | **kernel time** | 2.61 cores [2.34, 3.25] | **0.22 cores** [0.19, 0.24] | 0.088 / 0.085 | 1.028 [0.83, 1.11] | **yes, disjoint** | | yields per 20ms window | 14896 | **384** | — | — | 38.8x | | tokens/s | 279.6 [217.7, 287.3] | 276.2 [252.8, 279.6] | 0.991 / 0.978 | 1.004 [0.92, 1.16] | no | | total occupied cores | 15.76 | 15.88 | — | — | no | Kernel time is disjoint and every one of the 20 paired ratios sits far outside the A/A null. Throughput overlaps, and its paired ratios poke outside the null in *both* directions, which is what noise looks like. ## What this does and does not buy **Does not**: free a core. Occupancy is unchanged (15.76 → 15.88) because the worker still holds it, now spinning in user space instead of in the kernel. A co-tenant gets the runqueue lock and the scheduler passes back, not the CPU. **Does**: remove ~2.4 cores' worth of syscalls that returned without doing anything, and make `sys_frac` a usable signal again. At width 8 the old loop was bimodal — 0.4% kernel time and 216 tokens/s in one run, 15.7% and 130 tokens/s in the next — which made "are we thrashing" and "are we waiting" indistinguishable in my own matrices. The larger question — whether the 500us window earns its occupancy at all — is **#2071**, deliberately not answered here. It needs the inter-token gap regime the window was designed for, not the tight loop this bench runs, and that harness (#1395) is not merged. ## Preserving #1825 The deadline is now evaluated on a stride of 64 pure `spin_loop`s (~640ns) rather than after every yield. That is **not** the stride #1825 removed: that one was a stride of *yields*, each costing microseconds to milliseconds of an already-starved thread. Checking every ~640ns is strictly more often in wall-clock terms than "on every yield" ever achieved, and `the_blocktime_deadline_is_evaluated_on_every_yield_not_on_a_stride` passes unchanged. `YIELD_CLOCK_STRIDE` is kept a divisor of `SPIN_LOOP_BUDGET` so the yield phase gets its first clock read on the iteration it begins at. ## The test, and why it is shaped this way `the_active_window_yields_on_an_interval_not_on_every_iteration` counts yields over a 20ms window and asserts the phase cannot exceed one yield per 20us. The bound is **arithmetic, not empirical**: a yield happens only when `elapsed` reaches `next_yield`, and `next_yield` advances by at least `YIELD_INTERVAL` each time, so the count is capped no matter how fast the loop spins. Load can only reduce it. The ceiling is **stated absolutely rather than derived from `YIELD_INTERVAL`**. A ceiling computed from the constant under test moves with it — setting the interval to one nanosecond restores the exact defect and still satisfies a derived bound. That is the same shape as an A/B whose control is latched (#1736), and mutation M2 below is the proof it would have mattered. `slow_yield` now counts unconditionally and only *sleeps* when injected. The test has to count yields in the uninjected regime it asserts about; injecting a sleep to make them countable would have set the interval under test. ## Mutation-proved, three ways Each kills only the new test: | mutation | observed | fails on | |---|---|---| | yield on every iteration, ignoring the interval | 4884 yields | rate bound | | `YIELD_INTERVAL = 1ns` | 4837 yields | rate bound — the reason the ceiling is absolute | | window shortened below the spin phase | 0 yields | **non-vacuity**, not a silent pass on a bound nothing reached | And the main-equivalent loop (no stride, yield every iteration) measures **14896**, which is the baseline quoted above. ## A note on instruments `strace` cannot confirm this. At ~55us per traced syscall the instrument's own cost exceeds the 50us interval it is measuring, so both arms yield on every check and the difference collapses to 17797 vs 13701 — a 1.3x that looks like a weak result and is actually the instrument setting the property. The in-process counter is the only tool here that does not. `perf` tracepoints are unavailable at `perf_event_paranoid=1`. ## Validation - `cargo test -p onnx-runtime-ep-cpu --lib` — **1752 passed, 0 failed**, 25 ignored - `--features mlas` — **1778 passed, 0 failed**, 36 ignored - `cargo clippy --lib --all-targets -- -D warnings` clean on both lanes; `cargo fmt --check` clean - All benchmark runs took `scripts/hostlock.sh` for the whole window --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Closes item 2 of #2075. ## The headline is a negative result, and it is in the constant's doc comment **This does not reduce CPU.** It moves it from the `sys` column to the `user` column. A `sched_yield` costs ~1.05 µs of kernel time and advances the wall clock ~1.05 µs; a `spin_loop` advances it ~0.3 ns at ~0.3 ns of user time. Per unit of *waiting*, both hold the core at ~100%. Replacing one with the other therefore relabels the accounting and does not give the core back. Measured on a 7×7 order-cancelled, per-repetition-paired matrix (`bench_decode_gap`, gaps 20 µs–3 ms × t=2/4/8/16): | | before | after | |---|---|---| | sys share of CPU | 20–31% | 3.6–5.9% | | **cpu-per-wall** | — | **−0.02% … −0.85%** | Every cpu-per-wall delta is smaller than the two same-binary null arms' own drift (−0.71% and −0.43%). Only t=16 sits outside at −6.6% ± 0.4 (~2.4σ) — suggestive, not established, and not claimed. Latency is **unresolvable** at this sample size: p50 medians *looked* 7–15% better under co-tenancy, but paired per-repetition analysis gives ratio means 1.005 / 1.016 / 0.960 with sd up to 0.515. No latency claim is made in either direction. Co-tenant harm is ruled out precisely. A dependency-free CPU-bound competitor at 8/16/32 threads shows throughput **1.000, 1.000, 1.002** against a null of **1.000 ± 0.001**. ## So what is it worth? The yield rate today is set by *how fast the loop happens to run*, not by policy — nothing bounds it. This makes it a bounded, tested quantity: **≤64 yields per `MAX_SPIN` window** (`YIELD_MIN_INTERVAL` = 10 µs = `MIN_SPIN`/2), which is 5–12× less run-queue traffic imposed on the host, with proven-zero harm to co-tenants. Whether that reduction helps other tenants via run-queue lock contention is plausible and **unmeasured**, so it is deliberately not claimed. ## The attribution behind it (this part is solid) Before changing anything I checked #2075's premise with a regression rather than an anecdote. Nine arms spanning a **94× range** of yield counts: ``` sys ≈ 1.053 µs × yields + 68.3 µs × parks + ~0 R² = 0.989, n = 18 (pooled with a replicate) ``` The fit was given only counter totals, and it **recovers both syscall costs** — ~1.05 µs for an uncontended `sched_yield` and ~68 µs for a futex round-trip — from data that contained neither. Replication agrees to 1.6%. The control fails: `sys ~ user_s` fits worse (R²=0.86) *and* impossibly, with a **−0.43 s intercept**. Sys share spans 2.4%→31.6% across arms, so kernel time is not a fixed fraction of CPU. Yields explain 77–101% of kernel time in every arm above 50 `CLK_TCK` ticks (arms below that are 10 ms-quantised and are not quoted). **So #2075's attribution is correct.** The payoff simply is not there. ## Implementation Phase 2 of `spin_for_dispatch` becomes: ```rust let elapsed = start.elapsed(); if elapsed >= spin_window { break Expired; } if elapsed >= next_yield { spin_yield(); next_yield = elapsed + YIELD_MIN_INTERVAL; } else { spin_loop(); } ``` - The `start.elapsed()` read is the one #1825 already mandates per phase-2 iteration; it now gates both the deadline and the yield. **No added clock read.** - `next_yield` is computed from the **pre-yield** `elapsed`, which makes the limit self-disable under contention: an 11 ms yield returns with the next one already due, so a genuinely contended box behaves exactly as before. - The deadline keeps strict precedence over the yield. Leaving late costs a syscall, which is the thing both this change and #1825 exist to stop. - `Caught`/`Shutdown` latency strictly improves: the epoch is re-read every ~25 ns instead of being invisible for up to 11 ms inside a blocking yield. ## Independent Opus review — one real find, taken **[RISK]** I had claimed "the phase's first iteration always yields", so the tests asserted `performed > 0`. But the deadline check now precedes the yield, and phase 1's last stride check is at `spins == 4032` — iterations 4033–4095 run with **no clock read**. On a loaded box the window can therefore already be expired at phase-2 entry, giving **zero** yields with `spins >= SPIN_LOOP_BUDGET`. The assertion could hard-fail on correct code. The reviewer offered relaxing the assertion, or yielding before honouring an expired deadline. I rejected the latter as actively wrong (it spends a syscall to leave late) and rejected merely relaxing as losing the test's teeth. Instead the assertion is now **exact**, using a discriminator that needs no clock: > Conditional on `outcome == Expired` (asserted first): `spins == SPIN_LOOP_BUDGET` ⟺ the loop broke on the first phase-2 pass **without** yielding; `spins > SPIN_LOOP_BUDGET` ⟹ the first pass yielded, because `next_yield` starts at zero. So `performed > 0` ⟺ `spins > SPIN_LOOP_BUDGET`. Proved by construction, not by argument: with `CLOCK_CHECK_STRIDE` mutated to `1<<20` and the window to 1 ns, the run reaches phase 2 with the deadline already blown and a probe confirms `spins=4096, performed=0` — the reviewer's exact scenario, which the old assertion would have failed and the new three-way `iff` handles. Seven further items were traced and cleared: `spins` u32 wrap (~107 s of looping vs a 500 µs window cap, and a wrap self-corrects within ≤64 iterations), `next_yield` overflow, #1825's guarantee and its test's continued discriminating power, self-disable under contention, the `YIELD_COUNT` change, and release-build cost on the `spin_window <= 130 µs` path. ## Tests - `the_yield_phase_rate_limits_its_yields_against_the_wall_clock` — absolute ceiling of 64 yields per window, with a non-vacuity gate. - `the_spin_phase_yield_counter_counts_exactly_the_yields_the_window_performed` — the three-way exact `iff` above. - `YIELD_COUNT`'s increment moved **out** of `spin_yield`'s injection branch so it is unconditional ground truth (still `#[cfg(test)]`, zero release cost). Mutation-proved: removing the limit → 221 yields against the 64 ceiling → fails. Delaying the first yield → fails all three of the new lower bound, the counter identity, **and** the pre-existing #1825 deadline test. ## Where the loss actually is The pool burns ~15 cores at a 300 µs gap and **the yield has nothing to do with it**. That is spin-*window* policy — `MAX_SPIN`/`MIN_SPIN` adaptation — which I am filing separately with the cpu-per-wall framing this matrix established. ## Validation `cargo test -p onnx-runtime-ep-cpu --lib` **1828 passed / 0 failed**; `--features mlas` **1854 / 0**; `clippy --all-targets --all-features -D warnings` clean; `fmt --check` clean. ## Scope corrections to #2075 (restated) There is **one** yield site, not two — `:429`/`:520` was my misreading. And `DISPATCHER_YIELD_STRIDE` did not regress anything; it landed in #1201, the same commit that introduced the pool. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

justinchuby and others added 3 commits August 23, 2026 05:59

Merge remote-tracking branch 'origin/main' into resch/bounded-pool-ba…

d7e1abd

…rrier

justinchuby and others added 5 commits August 23, 2026 09:08

Merge remote-tracking branch 'origin/main' into resch/bounded-pool-ba…

11e7b90

…rrier

justinchuby marked this pull request as ready for review August 23, 2026 12:59

justinchuby merged commit bda5ded into main Aug 23, 2026
14 of 20 checks passed

justinchuby deleted the resch/bounded-pool-barrier branch August 23, 2026 13:10

justinchuby mentioned this pull request Aug 23, 2026

fix(ep-cpu): check the spin deadline on every yield at the two sites #1825 missed #1868

Merged

justinchuby added a commit that referenced this pull request Aug 24, 2026

fix(ep-cpu): check the spin deadline on every yield at the two sites #…

7e274a4

…1825 missed (#1868)

justinchuby mentioned this pull request Aug 24, 2026

test(ep-cpu): pin the readiness deadline's granularity, not just that it fires #1919

Merged

justinchuby mentioned this pull request Aug 24, 2026

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

Merged

This was referenced Aug 24, 2026

test(ep-cpu): assert realized decode-pool placement, not just worker count #1805

Merged

test(ep-cpu): guard the task-pool half of #1868's every-yield deadline check #2022

Merged

This was referenced Aug 24, 2026

fix(ep-cpu): bound the task-pool readiness wait so a wedged worker fails loud instead of burning cores #2027

Merged

perf(ep-cpu): stop the decode wait yielding on every iteration #2072

Merged

justinchuby mentioned this pull request Aug 25, 2026

task_runtime::pool has the same yield-every-iteration wait as the decode pool did — measure before assuming it transfers #2075

Open

4 tasks

This was referenced Aug 25, 2026

perf(ep-cpu): bound the worker spin phase's sched_yield rate #2151

Merged

perf(ep-cpu): the spin window controller has no CPU term, so it holds ~15 idle cores whenever the decode gap is under MAX_SPIN #2152

Open

Navigation Menu

Repository navigation

fix(ep-cpu): bound the decode pool's readiness barrier and release its workers on failure - #1825

fix(ep-cpu): bound the decode pool's readiness barrier and release its workers on failure#1825
justinchuby merged 8 commits into
mainfrom
resch/bounded-pool-barrier

justinchuby commented Aug 23, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Aug 23, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

justinchuby commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What breaks today

The fix

The stride defect

Memory ordering

Tests

Test-only fault injection is thread-scoped, and that is not cosmetic

Mutation battery: 9/9 effective

Validation

Rebased onto #1826, which broke this branch silently

Disclosures

Uh oh!

codecov Bot commented Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

justinchuby commented Aug 23, 2026 •

edited

Loading

codecov Bot commented Aug 23, 2026 •

edited

Loading