Skip to content

fix(ep-cpu): make the dispatcher-placement probes inert under Miri (main is red, mine) - #1921

Merged
justinchuby merged 1 commit into
mainfrom
squad/roy-miri-dispatcher
Aug 24, 2026
Merged

justinchuby merged 1 commit into
mainfrom
squad/roy-miri-dispatcher

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

Main is red on Miri and it is mine

Reporting this against myself. #1915 merged at 01:59:54Z on required checks
(Fast (Linux x86_64), Rust quality) while Miri unsafe-crate soundness
was still running
, and it subsequently failed. Miri is not a required
check on this repo, so auto-merge fired legitimately — no --admin, no
ruleset bypass — but the outcome is the same as if it had been bypassed: a
defect landed on main that CI caught. I had the fix pushed to the PR branch at
02:03, four minutes too late.

The defect. The Miri job runs
decode_spmd::tests::a_panic_in_the_dispatcher_shard_still_waits_for_the_workers.
That test dispatches on a real pool, and #1915 made dispatch call
sample_dispatcher_cpu() → libc::sched_getcpu(), which Miri has no shim for:

error: unsupported operation: can't call foreign function `sched_getcpu` on OS `linux`
    --> crates/onnx-runtime-ep-cpu/src/decode_spmd.rs:1607:32

A panic-safety test failing for reasons that have nothing to do with panic
safety.

The fix. sample_dispatcher_cpu, current_thread_os_id and the
sched_setaffinity call are gated on not(miri). Nothing is lost: a
CPU-placement sample is meaningless under an interpreter that does not model
CPUs, there is no /proc for a tid to index, and the property Miri exists to
check — that the unsafe blocks are sound — does not depend on the calls being
made. Setting ONNX_GENAI_CPU_DECODE_DISPATCHER_PIN under Miri now degrades to
"not pinned" instead of failing an unrelated test.

Verified with the workflow's exact invocation, on this branch, off current
main:

MIRIFLAGS="-Zmiri-disable-isolation -Zmiri-num-cpus=4 -Zmiri-ignore-leaks" \
  cargo +nightly miri test --locked -p onnx-runtime-ep-cpu --lib \
  decode_spmd::tests::a_panic_in_the_dispatcher
→ test result: ok. 1 passed; 0 failed

What I'm taking from it. "Wait for required CI" is not sufficient when a
job that can fail is not in the required set. For anything that adds an FFI
call inside a code path a Miri test executes, the check to run before merging
is Miri itself, locally, not the required set. I should have run it before
opening #1915 — the test is named in .github/workflows/miri.yml:227 and I
touched the exact function it exercises.

CI caught this, which is the whole argument for waiting on it: the Miri job
runs `decode_spmd::tests::a_panic_in_the_dispatcher_shard_still_waits_for_the_workers`,
that test dispatches on a real pool, and dispatch now calls
`sample_dispatcher_cpu()` -> `libc::sched_getcpu()`, which Miri has no shim
for. The test aborted with "can't call foreign function `sched_getcpu`" --
a failure in a panic-safety test that has nothing to do with placement.

`sample_dispatcher_cpu`, `current_thread_os_id` and the `sched_setaffinity`
call are now gated on `not(miri)`. Nothing is lost by it: a CPU-placement
sample is meaningless under an interpreter that does not model CPUs, there is
no `/proc` for a tid to index, and the property Miri is actually there to check
-- that the unsafe blocks are sound -- does not depend on the calls being made.
Setting the knob under Miri now degrades to "not pinned" rather than failing an
unrelated test.

Verified locally with the workflow's exact invocation:
`MIRIFLAGS="-Zmiri-disable-isolation -Zmiri-num-cpus=4 -Zmiri-ignore-leaks"
cargo +nightly miri test --locked -p onnx-runtime-ep-cpu --lib
decode_spmd::tests::a_panic_in_the_dispatcher` -> 1 passed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby enabled auto-merge (squash) August 24, 2026 02:10
@codecov

codecov Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.32%. Comparing base (cdc7d93) to head (f299761).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
crates/onnx-runtime-ep-cpu/src/decode_spmd.rs 0.00% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1921      +/-   ##
==========================================
+ Coverage   80.14%   80.32%   +0.17%     
==========================================
  Files         413      415       +2     
  Lines      200822   204575    +3753     
  Branches   200822   204575    +3753     
==========================================
+ Hits       160957   164333    +3376     
- Misses      34343    34669     +326     
- Partials     5522     5573      +51     
Flag Coverage Δ
cli-ort-linux 72.51% <ø> (?)
cli-ort-windows 72.01% <ø> (ø)
mlas 85.20% <ø> (?)
offline 80.45% <0.00%> (+0.09%) ⬆️

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

Files with missing lines Coverage Δ
crates/onnx-runtime-ep-cpu/src/decode_spmd.rs 89.82% <0.00%> (-0.75%) ⬇️

... and 26 files with indirect coverage changes

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

@justinchuby
justinchuby merged commit bb6329d into main Aug 24, 2026
15 of 20 checks passed
@justinchuby
justinchuby deleted the squad/roy-miri-dispatcher branch August 24, 2026 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants