Repository navigation
test(cpu): bound forced-pool affinity child thread count to fix Windows ARM64 flaky - #117
Merged
Merged
Conversation
…ndows ARM64 flaky) run_affinity_defer_child hard-coded DECODE_THREADS=8 and RAYON_NUM_THREADS=8. The forced scenario forces the persistent SPMD pool, spinning up ~8 busy-waiting workers plus an 8-thread Rayon pool inside an already-spawned child test binary. On the constrained native Windows ARM64 CI runner this oversubscribes the CPU and hard-crashes the child with an empty-stderr STATUS_ACCESS_VIOLATION (0xC0000005), intermittently failing forced_persistent_pool_ignores_explicit_affinity. Reuse the existing parity_worker_count() helper (largest odd count <= available_parallelism(), capped at 15) for both env vars, mirroring the PR #111 fix for the sibling odd-worker parity flaky. Test-harness only; no product code changed. The pool is still forced (parity_worker_count() >= 1 => pools().is_some()) and the affinity-defer assertions are unchanged, so the test stays meaningful. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
==========================================
+ Coverage 82.49% 82.83% +0.33%
==========================================
Files 231 231
Lines 99759 99760 +1
Branches 99759 99760 +1
==========================================
+ Hits 82294 82632 +338
+ Misses 14074 13733 -341
- Partials 3391 3395 +4
🚀 New features to boost your workflow:
|
justinchuby
added a commit
that referenced
this pull request
Jul 24, 2026
…64 exit crash (#119) ## Summary Fixes the intermittent **Windows ARM64** `STATUS_ACCESS_VIOLATION` (0xC0000005, empty stderr) in `forced_persistent_pool_ignores_explicit_affinity` — the *real* root cause behind the recurring flaky that #117's thread-cap only partially mitigated. This is also a genuine robustness bug for real Windows ARM64 users. ## Root cause The persistent SPMD decode pool lives in a module-level `static OnceLock<Option<SpmdDecodePools>>`. **Rust never runs `Drop` on statics at process exit**, so the existing `impl Drop` (which signals shutdown + joins workers) never ran for the process-wide pool. Its worker threads — spinning on `sequence` or parked, all holding `Arc<SharedState>` — stayed alive into CRT teardown; on weakly-ordered Windows ARM64 a worker touching that shared state during teardown faults the process. The `forced_off` test child builds the pool, asserts, and returns, leaking N live workers straight into exit. #117's thread cap only shrank the race window (fewer live threads), which is why it reduced but didn't eliminate the crash. ## Fix (product-first) - `SpmdDecodePools::shutdown(&self)`: sets the stop flag, bumps `sequence`, **unconditionally unparks all** workers, and **joins** them. Join handles moved behind a `Mutex<Vec<JoinHandle>>` so it's reachable through the `&'static` pool and idempotent (drained once). `Drop` delegates to it. - `decode_spmd::shutdown_pools()`: peeks the `OnceLock` (never force-builds) and joins only if the pool was ever built. - The `forced_off` test child calls `shutdown_pools()` before returning → deterministic stop+join before exit, closing the race. **Deadlock/lost-wakeup safety** (independently verified): unconditional unpark-all means a worker that just set `parked` can't be missed; `park_timeout` consumes pending tokens with a 50 ms backstop; every worker is guaranteed to observe stop and return, so `join()` cannot hang. ## Hot path / perf Zero impact. Per-decode dispatch stays lock-free (`publish` unparks via a plain thread slice); the `Mutex<Vec<JoinHandle>>` is touched only at build/teardown, never per-decode. No numerics change. ## Test stays meaningful `forced_off` still forces the pool and asserts `pools().is_some()`; `auto_*` still assert `pools().is_none()`. `shutdown_pools()` is teardown hygiene, not a masking retry. Nothing ignored/skipped/weakened. ## Follow-up (out of scope) `decode_numa::NumaDecodePools` uses the same static pattern and warrants a peer `shutdown()`; and an optional tripwire so post-shutdown dispatch panics loudly instead of hanging. ## Gates (Linux x86_64) mlas 904+10 pass (incl. the affinity test); no-mlas 873+10 pass; clippy clean both; `cargo fmt --check` clean (only known `mlas-sys:1211` FP); aarch64 `-D warnings` check clean. Reviewed independently 🟢 APPROVE (non-author). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a Windows ARM64 CI flaky (hard subprocess crash,
STATUS_ACCESS_VIOLATION/ empty stderr) in the persistent-pool affinity tests. Same class as #111.Root cause
run_affinity_defer_child(test harness inmatmul_nbits.rs) hard-codedONNX_GENAI_CPU_DECODE_THREADS=8andRAYON_NUM_THREADS=8for its subprocess children. Theforced_offscenario forces the persistent SPMD pool, which spins up ~8 busy-waiting SPMD workers (plus a Rayon pool) inside an already-spawned child test binary. On the low-core native Windows ARM64 runner this oversubscribes CPUs and hard-aborts the child before it can print — surfacing asforced_persistent_pool_ignores_explicit_affinityfailing with truncated stdout and empty stderr. It intermittently fails any ep-cpu PR (hit #115 twice in a row).Fix
Replace the hard-coded
"8"with the existingparity_worker_count()helper (largest odd ≤available_parallelism(), capped at 15, always ≥ 1) — the exact remedy #111 applied to a sibling test. Test-harness only; no product/kernel code changed.Why the test stays meaningful
forced_offstill forces the pool (PERSISTENT_POOL=1) and still assertspools().is_some()(parity_worker_count() ≥ 1⇒ a real pool is built).auto_*children still set explicit affinity and assertpools().is_none().#[ignore], no weakened assertion, no OS/arch skip — only the thread count is bounded to a runner-safe value.Sibling tests
Two other forced-pool subprocess tests use small hard-coded counts (MLAS-shard 3/3, MLAS-prefill 3/6) — below the oversubscription threshold and not reported flaky; left unchanged to preserve the prefill test's deliberate
RAYON > workersrow-block invariant.Gates (Linux x86_64)
cargo test -p onnx-runtime-ep-cpu --features mlas— 903 pass; no-mlas pass; clippy clean both;cargo fmt --checkclean (only knownmlas-sys:1211false positive); aarch64 cross-check clean.Reviewed independently 🟡 APPROVE-WITH-NITS (non-author).
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com