Repository navigation
test(ep-cpu): fail closed when the surviving-worker scan cannot see /proc - #1992
Conversation
…proc
`live_spmd_worker_threads` returned `0` when `/proc/self/task` could not be
listed. `0` is also the *passing* value for
`a_failed_build_leaves_no_workers_running`, whose whole claim is that a failed
build leaves no workers holding the machine -- so the test reported success
precisely when its instrument had gone blind. A scan that can see nothing
agrees that nothing survived.
Demonstrated rather than argued. Injecting the blindness (`read_dir` pointed at
a path that does not exist) against the pre-fix code:
test a_failed_build_leaves_no_workers_running ... ok
and against this commit:
the surviving-worker scan went blind, so its count is not evidence either
way: /proc/self/task could not be listed: No such file or directory
A second blind spot had no fault path at all: a listable directory does not
imply readable entries, and it is the entries the filter depends on. A `/proc`
that enumerates tasks but refuses every `comm` read yielded the same `0`. The
tally now requires at least one name to have been read -- the calling thread is
always present and cannot exit under itself -- and distinguishes that from the
benign `NotFound` of a task exiting mid-scan, which is the outcome the callers
are hoping for and must not stop the count.
The other caller was already guarded, by `spmd_threads_before > 0` in
`realized_width_child`'s parent. That idiom is what this generalizes: the
guarantee belongs in the instrument, not in one of its two users.
Also removes a naming-drift hazard Gaff raised on #1805. The thread name was a
string literal at the spawn site and a second, independent string literal in
the scan's filter. A filter that matches nothing does not fail -- it returns
"no threads", the passing answer for every caller -- so a rename on one side
would have silently emptied the count. Both now read `SPMD_THREAD_NAME_PREFIX`,
and `the_worker_name_survives_proc_truncation` asserts that what Linux keeps of
a worker's name in 15 bytes is exactly what the scan looks for.
The blind cases get deterministic negative controls by taking the reads as an
argument (`tally_worker_comms`) instead of performing them: `/proc` cannot be
made to fail on demand, and a policy exercised only by the happy path is a
policy nobody has tested.
Validation: x86_64 lib suite 1717 passed / 0 failed / 24 ignored; clippy
--all-targets -D warnings clean; cargo fmt --check clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…OENT
Review finding on the commit before this one, and it would have fired on the
*success* path.
`read_to_string("/proc/self/task/<tid>/comm")` is `open` then `read`, so a task
vanishing under the scan has two outcomes, not one. Gone before the `open` is
`ENOENT`. Gone before the first `read` makes the kernel's `comm_show` fail its
`get_proc_task` and return `ESRCH` -- `man 5 proc` says an exited thread's files
give "ESRCH or ENOENT", and only the second was handled.
The reason a kind-based test misses it, confirmed on this toolchain:
ESRCH(3) -> Uncategorized
ENOENT(2) -> NotFound
EIO(5) -> Uncategorized
Rust's `decode_error_kind` has no mapping for `ESRCH`, so it arrives as
`Uncategorized` -- which is also where `EIO` arrives, and `EIO` is real
blindness. Matching on `ErrorKind` has to be wrong about one of them, so the
benign arm now matches the raw errno, the same idiom `decode_affinity.rs:844`
already uses for `EINVAL`.
The window is not theoretical and is worst when the code under test is
*correct*. `shutdown_join_child` counts immediately after `shutdown_pools`
joins, and this file already documents that regime at
`shutdown_pools_is_a_barrier_not_a_request`: "the futex that unblocks `join` is
signalled before the kernel unhashes the task." `ready_leak_child` polls for ten
seconds precisely while surviving workers exit. Under the previous commit an
`ESRCH` there panicked the child, so it never printed its count and the parent
failed with "child never reported" -- a new flake, in the exact race the change
set out to handle.
Controls: `a_task_that_exits_mid_scan_is_not_blindness` now feeds both errnos
and asserts up front that `ESRCH.kind() != NotFound`, so the raw-errno arm
cannot quietly become dead code. `an_uncategorized_error_is_not_by_itself_a_
vanished_task` pins the `EIO`/`ESRCH` collision that forces the errno match, and
first asserts the two still share a kind. Deleting the `ESRCH` arm fails the
first with `left: Blind("No such process (os error 3)"), right: Vanished`.
Also closes the residual half of the naming-drift hazard: two bench scripts
exact-match the same truncated name and cannot read the Rust const, so both are
now pinned to it by comment. `acc0_w16_mode_placement.py` already discarded an
empty census as a defect; `decode_placement_census.sh` printed
"spawned spmd threads : 0" as though it were a measurement, and now says an
empty census is not evidence of a placement.
Validation: x86_64 1718 passed / 0 failed / 24 ignored; aarch64 qemu 1609
passed / 0 failed / 15 ignored; clippy --all-targets -D warnings clean;
cargo fmt --check clean; bash -n and py_compile clean.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1992 +/- ##
==========================================
+ Coverage 80.24% 80.44% +0.20%
==========================================
Files 408 425 +17
Lines 189152 208238 +19086
Branches 189152 208238 +19086
==========================================
+ Hits 151784 167524 +15740
- Misses 31953 35067 +3114
- Partials 5415 5647 +232
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
justinchuby
left a comment
justinchuby
left a comment
There was a problem hiding this comment.
Reviewed. The policy is right and the ESRCH/EIO discrimination is the part most people would have got wrong — matching on ErrorKind there has to be wrong about one of them, and an_uncategorized_error_is_not_by_itself_a_vanished_task plus the assert_ne!(raced.kind(), NotFound) dead-code tripwire is the correct pair. tally_worker_comms taking the reads as an argument is what makes the blind arms falsifiable at all; without it the policy would only ever be exercised by the happy path.
Two findings. The first is the one I would act on.
1. The truncation tripwire asserts <= where the scripts need ==
the_worker_name_survives_proc_truncation asserts SPMD_THREAD_NAME_PREFIX.len() <= COMM_MAX_BYTES. That is the right bound for Rust, which uses starts_with. It is not the bound the two shell/Python censuses need, because they compare for equality against the already-truncated comm:
[ "$(cat "$t/comm" 2>/dev/null)" = "onnx-genai-spmd" ] || continue # census.sh:56if fh.read().strip() != "onnx-genai-spmd": # acc0_w16_mode_placement.py:88Those agree with the Rust scan only because the prefix is exactly 15 bytes — printf onnx-genai-spmd | wc -c = 15 = COMM_MAX_BYTES. At that length, comm holds precisely the prefix and equality and starts_with coincide.
Shorten the name to 13 bytes — well within the <= the tripwire allows — and they diverge: comm holds 15 bytes of the full name (onnx-genai-sp-n), Rust still matches, both scripts match nothing, and every census silently empties. That is the exact failure mode this PR exists to close, still reachable through a change the tripwire calls fine.
Two ways out, either is fine: assert SPMD_THREAD_NAME_PREFIX.len() == COMM_MAX_BYTES with the reason ("the censuses compare for equality against the truncated name"), or change the scripts to prefix-match. The first is one character and one comment.
2. The "keep it equal" comment is the remedy this PR argues is insufficient
The comment added to both scripts says a rename "empties the census silently rather than failing it" — a correct diagnosis whose treatment is then a comment asking the next person to remember. There is also a third copy the PR does not mention, benches/acc0_lowwidth_smt.py (lines 46/100/208). That one is less bad — it reports discarded: "no onnx-genai-spmd threads alive at the sample point" rather than passing — but it is a fourth site the const does not reach.
The repo already has the mechanical form of this: shape_dispatch_gate greps git ls-files and fails when a symbol appears where it must not. The inverse is the same ten lines, and it puts the guarantee where this PR puts the others — in the instrument:
#[test]
fn every_census_filters_on_the_name_the_pool_spawns() {
for script in [
"crates/onnx-runtime-ep-cpu/benches/decode_placement_census.sh",
"crates/onnx-runtime-ep-cpu/benches/acc0_w16_mode_placement.py",
"crates/onnx-runtime-ep-cpu/benches/acc0_lowwidth_smt.py",
] {
let source = std::fs::read_to_string(repo_root().join(script)).expect(script);
assert!(
source.contains(SPMD_THREAD_NAME_PREFIX),
"{script} no longer filters on the name the pool spawns, so its census is empty by construction"
);
}
}Not blocking — the Rust instrument is the one the tests read, and it is fixed here. But a duplicated filter guarded by a comment is the same bet this PR just refused to take in live_spmd_worker_threads.
Note for whoever merges second
#1991 touches the same test module in decode_spmd.rs (the four silent core_count() < 2 sites and environment_can_pin). No textual overlap with this diff that I can see, but a clean text merge is not evidence of a clean semantic one — I got that exact result on #1926 last week, and again on #2003 four hours ago. If this lands first I will re-run -p onnx-runtime-ep-cpu --lib on the merged tree rather than trusting the merge.
|
Merging with two red lanes, both provably inherited from
On pristine This branch touches three files, none of them engine files: Reproduced locally on an unmodified checkout and filed as #2004 — reintroduced by #1982 ( Everything else is green: 18 pass. Local validation on the merged tree — Worth noting why this took tracing rather than a shrug: |
1 participant
Follow-up to #1949. Same defect class, one level down: an instrument whose
failure value is identical to its passing value.
The false oracle
decode_spmd.rstests count surviving SPMD worker threads by scanning/proc/self/task/<tid>/comm. Onmainthe scan opens with:0is also the passing value fora_failed_build_leaves_no_workers_running,whose whole claim is that a build which fails must not leave workers spinning or
parked and holding the machine. A scan that can see nothing agrees that nothing
survived.
Demonstrated, not argued. The same blindness injected (
read_dirpointed at apath that does not exist), against
main:and against this branch:
A second blind spot had no fault path at all. A listable directory does not
imply readable entries, and it is the entries the filter depends on: a
/procthat enumerates tasks but refuses everycommread produced the same0. The tally now requires at least one name to have actually been read — thecalling thread is always in
/proc/self/taskand cannot exit under itself, soreading no name at all is only possible when the scan is blind.
The other consumer was already guarded, by
spmd_threads_before > 0inrealized_width_child's parent. That idiom is what this generalises: theguarantee belongs in the instrument, not in one of its two users.
The mid-scan race, and why the first commit got it wrong
Round-two review (Opus, adversarial brief) found a HIGH in the first commit, and
it would have fired on the success path.
read_to_stringisopenthenread, so a task vanishing under a scan that isnot a snapshot has two outcomes, not one. Gone before the
openisENOENT.Gone before the first
readmakes the kernel'scomm_showfail itsget_proc_taskand returnESRCH—man 5 procsays an exited thread's filesgive "ESRCH or ENOENT". Only the second was handled.
Why a kind-based test misses it, measured on this toolchain:
ErrorKindENOENT(2)NotFoundESRCH(3)UncategorizedEIO(5)UncategorizedRust's
decode_error_kindhas no mapping forESRCH, so it lands inUncategorized— which is also whereEIOlands, andEIOis genuineblindness. Matching on
ErrorKindhas to be wrong about one of them, so thebenign arm matches the raw errno, the idiom
decode_affinity.rs:844alreadyuses for
EINVAL.The window is worst exactly when the code under test is correct.
shutdown_join_childcounts immediately aftershutdown_poolsjoins, and thisfile already documents that regime: "the futex that unblocks
joinissignalled before the kernel unhashes the task."
ready_leak_childpolls for tenseconds precisely while surviving workers leave. Under the first commit an
ESRCHthere panicked the child, which then never printed its count, and theparent failed with "child never reported" — a new flake, in the exact race the
change set out to handle.
Naming drift
Raised by @gaff-1 on #1805: matching workers by thread name is a filter, and
a filter that matches nothing does not fail — it returns "no threads", which is
the passing answer for every caller.
The name was a string literal at the spawn site and a second, independent string
literal in the scan. Both now read
SPMD_THREAD_NAME_PREFIX, andthe_worker_name_survives_proc_truncationasserts that what Linux keeps of aworker's name in 15 bytes is exactly what the scan looks for. Two bench scripts
match the same truncated name and cannot read a Rust const, so they are pinned
to it by comment;
decode_placement_census.shalso printedspawned spmd threads : 0as though it were a measurement and now says an emptycensus is not evidence of a placement.
Tests
Blind cases get deterministic negative controls by taking the reads as an
argument (
tally_worker_comms) rather than performing them:/proccannot bemade to fail on demand, and a policy exercised only by the happy path is a
policy nobody has tested.
the_worker_name_survives_proc_truncationan_unreadable_comm_makes_the_scan_blindcommthat cannot be read is blindness, never "no worker here"a_task_that_exits_mid_scan_is_not_blindnessENOENTandESRCHare benign; asserts up front thatESRCH.kind() != NotFoundso the raw-errno arm cannot quietly become dead codean_uncategorized_error_is_not_by_itself_a_vanished_taskEIO/ESRCHkind collision that forces the errno match; asserts first that the two still share a kinda_scan_that_read_nothing_is_never_reported_as_no_survivorsthe_live_scan_can_see_this_processMutation controls. Deleting the
ESRCHarm failsa_task_that_exits_mid_scan_is_not_blindnesswithleft: Blind("No such process (os error 3)"), right: Vanished. Pointingread_dirat a nonexistent path fails boththe_live_scan_can_see_this_processand
a_failed_build_leaves_no_workers_running, wheremainpasses.Validation
cargo clippy -p onnx-runtime-ep-cpu --all-targets -- -D warningscleancargo fmt --checkcleanbash -nandpy_compileclean on the two bench scriptsTest-only change apart from the shared name const; no production behaviour is
altered.