fix(ep-cpu): bound the SPMD pool teardown join instead of hanging on a wedged worker

[justinchuby]

Closes #2123.

The defect

SpmdDecodePools::shutdown ended in an unbounded join:

for handle in handles {
    let _ = handle.join();
}

join has no timeout, so one worker that never leaves its loop hangs teardown permanently. Drop for SpmdDecodePools calls shutdown(), and so does shutdown_pools(), so this is reachable from ordinary teardown — including the implicit drop at the end of a test, a session close, or process exit.

This is the same defect family as #2027, relocated to the other end of the pool's life. #2027 removed an unbounded wait entering the pool; this removes the unbounded wait leaving it.

It fails the same way, too, and that is the part worth stating plainly: the wedged worker is not the only thread involved. The others are woken and exit, but any worker still spinning out its blocktime window keeps burning a full core while the joiner blocks. The process sits at high CPU and never exits — indistinguishable from work, which is exactly why the 5h40m aarch64 hang behind #2027 went unnoticed until someone read /proc.

The rest of the file already reasons correctly about this. The constructor's failure path, added by #2027, explicitly refuses to join:

Deliberately not joined -- a build that failed this way may have a wedged worker, and blocking on it here would reintroduce exactly the unbounded wait this backstop exists to remove.

begin_shutdown bounds its quiesce wait at SHUTDOWN_DISPATCH_QUIESCE and proceeds loudly. The worker wait path is bounded by a blocktime ramp into a futex park. So shutdown reasoned about every wait it performed except its last one, which had no bound at all.

The fix

Wait on the counter, not the handles.

workers_exited is incremented by an ExitCount drop guard that fires however a worker leaves, including a panic unwind, and it fires before the thread's epilogue. So:

1. Bounded wait for workers_exited >= handles.len(), yielding rather than spinning — spinning here starves the very workers whose departure is the exit condition, the same argument the readiness barrier makes.
2. If reached, join every handle. Each thread has only its epilogue left, so the wait is bounded in fact, not by hope.
3. If the deadline passes, do not join. That is precisely the case that hangs; joining it would reinstate the defect at the moment it is diagnosed. Report loudly and detach.

Detaching is memory-safe: worker_loop(shared: Arc<SharedState>, …) — each worker owns an Arc, so an abandoned worker cannot outlive the state it reads.

There is one re-read after the loop, deliberately: a worker can count out between the last load and the deadline check, and abandoning a pool that just became joinable would report a fault that no longer exists. That mirrors the same re-read the readiness barrier does on its own break path.

SHUTDOWN_JOIN_TIMEOUT is 30s — generous on purpose. A healthy worker leaves within microseconds of the stop flag, so a teardown that reaches the deadline is reporting a broken pool, not a slow one. It is a liveness backstop, not a performance bound.

Making the branch assertable

The give-up path records workers_abandoned, exposed as SpmdDecodePools::workers_abandoned(). A branch reported only by a log line is one nobody can prove was not taken, and the healthy direction is half the contract here: a backstop that fires on a correct pool converts every clean shutdown into a leaked thread and a false bug report, which is worse than the hang it guards.

Tests, and the mutations that justify them

a_worker_that_never_exits_is_abandoned_instead_of_hanging_shutdown wedges one worker and asserts both that shutdown returns bounded and that it reports the worker it gave up on. Neither assertion implies the other: the elapsed bound alone passes on an implementation where the wedge never fired, and the count alone passes on one that reported correctly after hanging for an hour.

The wedge is injected inside worker_loop while ExitCount is still alive, via a guard declared after it (drop order is reverse of declaration), so it covers every exit path including a panic unwind. That placement is the whole point: wedging after the count would prove nothing, because teardown would see its target reached and join a live thread, which no production path does. Selection is thread-local and latched on the builder thread, per this file's existing doctrine about global fault injection reaching unrelated pools; only the release flag is global, and only a worker already selected by the thread-scoped knob ever reads it.

a_healthy_teardown_never_abandons_a_worker is the other direction — abandoned == 0 and every worker counted out.

Both were falsified by mutation, not assumed:

mutation	result
restore the unconditional join()	test hangs; killed at 120s
force the deadline to 0 so the backstop always fires	healthy test FAILED

The first is also the cleanest available demonstration that the defect is real on main: with the fix reverted, the new test does not fail, it never returns.

Validation

• onnx-runtime-ep-cpu full lib suite: 1816 passed, 0 failed, 76.9s
• cargo clippy -p onnx-runtime-ep-cpu --lib --tests --all-features: clean (covers the tracing arm)
• cargo fmt --check: clean
• Both mutations above

All runs under scripts/hostlock.sh run (#1806), taskset -c 24-31.

Provenance

Found while auditing four processes @holden reported from my worktree holding the box at loadavg 12.44 — three generations of one --exact decode-pool test at 38–61 minutes each. Those specific processes are dead and their binary predates #2027 by ~40h, so they are not evidence about current main, and the test they named passes 8/8 in 0.26s here. But the shape Holden identified — "worth attaching a bounded wait so a hang fails loudly instead of pinning a core indefinitely" — was a real unbounded wait still on main, at teardown rather than at build. The report was right even though the processes were not the proof.

[codecov]

Codecov Report

❌ Patch coverage is 99.15254% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.18%. Comparing base (272bd94) to head (d9d7153).
⚠️ Report is 20 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/onnx-runtime-ep-cpu/src/decode_spmd.rs	99.15%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2126      +/-   ##
==========================================
+ Coverage   80.50%   81.18%   +0.67%     
==========================================
  Files         427      429       +2     
  Lines      210487   216013    +5526     
  Branches   210487   216013    +5526     
==========================================
+ Hits       169450   175361    +5911     
+ Misses      35343    34876     -467     
- Partials     5694     5776      +82     

Flag	Coverage Δ
cli-ort-linux	72.51% <ø> (?)
cli-ort-windows	72.10% <ø> (ø)
mlas	86.02% <ø> (?)
offline	81.32% <99.15%> (+0.60%)	⬆️

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.42% <99.15%> (+0.54%)	⬆️

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