Fail a SLURM job the scheduler marked FAILED, whatever its exit code - #1879
Open
sbryngelson wants to merge 2 commits into
Open
Fail a SLURM job the scheduler marked FAILED, whatever its exit code#1879sbryngelson wants to merge 2 commits into
sbryngelson wants to merge 2 commits into
Conversation
…code
monitor_slurm_job.sh decided pass/fail from SLURM's ExitCode alone and
ignored the job state it had already queried and printed. SLURM
intermittently reports State=FAILED alongside ExitCode=0:0, and on
those jobs the log reads:
[20:05:16] Job 13132145 reached terminal state: FAILED
Job 13132145 completed successfully
The GitHub job then went green with the test failures still in its
log. Five of nine "successful" Phoenix gpu-acc runs on master were
hiding 23 failing tests this way, which is long enough for a real
regression to go unnoticed and makes a green self-hosted job worthless
as evidence.
run_monitored_slurm_job.sh already re-checks the state via sacct and
requires COMPLETED *and* 0:0 -- but only when the monitor exits
non-zero, so nothing verified the state on the success path. That
asymmetry is the bug; this makes both paths agree.
The guard is deliberately conservative: if no terminal state was
recorded it falls back to the previous exit-code-only behaviour, so it
cannot invent new red CI.
Tests: the first case reproduces the false green (the monitor returned
0 where 1 was expected) and the second pins the other direction so a
genuinely COMPLETED job still passes.
test_monitor_ci_summary.py's stub reported FAILED for every job,
including the one named "a successful job" -- a combination SLURM does
not produce for a clean run, and only harmless while the state was
being ignored. Its state now tracks the exit code, and any test that
wants the pathological pairing asks for it explicitly.
Claude-Session: https://claude.ai/code/session_017zrZooJPhZtZYgg9fJiYhg
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The monitor now validates terminal state with regression coverage for failed and completed jobs.
Pull request overview
Updates SLURM monitoring so jobs marked FAILED cannot pass with exit code 0:0.
Changes:
- Validate terminal state alongside exit code.
- Add regression tests for failed and successful jobs.
- Align CI-summary fixtures with realistic SLURM states.
File summaries
| File | Description |
|---|---|
toolchain/mfc/test_monitor_exit_codes.py |
Tests failed-state and successful-job handling. |
toolchain/mfc/test_monitor_ci_summary.py |
Updates SLURM state stubs. |
.github/scripts/monitor_slurm_job.sh |
Validates terminal state and exit code. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Exercising both versions of the monitor through run_monitored_slurm_job.sh against a stubbed SLURM, over every terminal state crossed with several exit codes, puts the behaviour change at exactly one column of that matrix: a state other than COMPLETED reported alongside ExitCode 0:0. COMPLETED is untouched at every exit code, every non-zero code already failed and still does, and 77 still relays in every state. That last one matters most and had no coverage: the submit wrapper uses 77 to exclude a bad node and resubmit, so flattening it to a generic failure would strand the job on that node. It is now pinned for each state rather than only for the default. Worth a second opinion on one point: NODE_FAIL, BOOT_FAIL and OUT_OF_MEMORY paired with 0:0 now come back as 1, a test failure, when they describe an unusable node. Previously they came back as 0, so this is strictly better than reporting them green, but 77 is arguably the truer answer and would let the existing exclude-and-resubmit path take them. Left alone here because routing them to 77 changes resubmit behaviour, which is a separate decision from fixing the false green. Claude-Session: https://claude.ai/code/session_017zrZooJPhZtZYgg9fJiYhg
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1879 +/- ##
==========================================
- Coverage 61.26% 61.25% -0.02%
==========================================
Files 84 84
Lines 22330 22336 +6
Branches 3265 3266 +1
==========================================
+ Hits 13680 13681 +1
- Misses 6207 6211 +4
- Partials 2443 2444 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
monitor_slurm_job.shdecided pass/fail from SLURM'sExitCodealone, ignoring the job state it had already queried and printed. SLURM intermittently reportsState=FAILEDalongsideExitCode=0:0, and on those jobs the log reads:The GitHub job then goes green with the test failures still in its log.
Impact
Five of nine "successful" Phoenix gpu-acc runs on master were hiding 23 failing tests. Same 23 tests, same
main.py finished with a 23 exit codein the log — the only difference between a red run and a green one was which value SLURM happened to put inExitCode.That is long enough for a real regression to go unnoticed (it hid #1878 for five days) and it makes a green self-hosted job worthless as evidence.
The bug
run_monitored_slurm_job.sh:40already re-checks via sacct and correctly requiresCOMPLETEDand0:0— but only when the monitor exits non-zero. Nothing verified the state on the success path. That asymmetry is the whole bug; this makes both paths agree.The guard is deliberately conservative: if no terminal state was recorded it falls back to the previous exit-code-only behaviour, so it cannot invent new red CI.
Tests
Written test-first — the new case reproduced the false green (monitor returned 0 where 1 was expected) before the fix, and passes after. A second test pins the other direction so a genuinely
COMPLETEDjob still passes and the guard can't over-correct.test_monitor_ci_summary.py's stub reportedFAILEDfor every job, including the one named "a successful job" — a pairing SLURM does not produce for a clean run, and only harmless while state was being ignored. Its state now tracks the exit code; a test wanting the pathological combination asks for it explicitly.671 toolchain tests pass.
Suggested review order
Independent of #1878 and #1879, but worth landing early: until it does, a green Phoenix job doesn't mean the tests passed, which makes verifying anything else on that lane harder than it should be.
https://claude.ai/code/session_017zrZooJPhZtZYgg9fJiYhg