From d73a8229431d97c451d2d245c3676b7a869216c8 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 13 Sep 2026 10:51:31 -0400 Subject: [PATCH 1/2] fix: fail a SLURM job the scheduler marked FAILED, whatever its exit 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 --- .github/scripts/monitor_slurm_job.sh | 19 +++++++++++++++---- toolchain/mfc/test_monitor_ci_summary.py | 10 ++++++++-- toolchain/mfc/test_monitor_exit_codes.py | 20 ++++++++++++++++++++ 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/.github/scripts/monitor_slurm_job.sh b/.github/scripts/monitor_slurm_job.sh index d508306c5..057cd54ca 100755 --- a/.github/scripts/monitor_slurm_job.sh +++ b/.github/scripts/monitor_slurm_job.sh @@ -208,6 +208,7 @@ while true; do fi if is_terminal_state "$state"; then + final_state="$state" echo "[$(date +%H:%M:%S)] Job $job_id reached terminal state: $state" break fi @@ -308,16 +309,26 @@ case "$exit_code" in ;; esac -# Check if job succeeded -if [ "$exit_code" != "0:0" ]; then - echo "ERROR: Job $job_id failed with exit code $exit_code" +# Check if job succeeded. +# +# Both the recorded state and the exit code have to agree. SLURM intermittently +# reports State=FAILED alongside ExitCode=0:0 -- on Phoenix the same job that +# printed "reached terminal state: FAILED" here then reported "completed +# successfully" and went green with 23 failing tests still in its log. Five of +# nine "successful" gpu-acc runs on master were hiding failures that way, which +# made a green self-hosted job worthless as evidence. +# +# run_monitored_slurm_job.sh already re-checks the state via sacct, but only +# when this script exits non-zero, so nothing verified it on the success path. +if [ "$exit_code" != "0:0" ] || { [ -n "${final_state:-}" ] && [ "$final_state" != "COMPLETED" ]; }; then + echo "ERROR: Job $job_id failed (state=${final_state:-unknown}, exit code $exit_code)" # A GPU memory fault explains itself in a block the test harness prints; lift # it onto the summary page so the faulting kernel and source line are visible # without opening the log at all. if grep -q 'GPU fault summary' "$output_file" 2>/dev/null; then ci_summary "### GPU memory fault\n\n\`\`\`\n$(grep -A6 'GPU fault summary' "$output_file" | head -8 | sed 's/`/'"'"'/g')\n\`\`\`\n" else - ci_summary "### Job \`$job_id\` failed (exit $exit_code)\n\n\`\`\`\n$(tail -n 15 "$output_file" | sed 's/`/'"'"'/g')\n\`\`\`\n" + ci_summary "### Job \`$job_id\` failed (state ${final_state:-unknown}, exit $exit_code)\n\n\`\`\`\n$(tail -n 15 "$output_file" | sed 's/`/'"'"'/g')\n\`\`\`\n" fi exit 1 fi diff --git a/toolchain/mfc/test_monitor_ci_summary.py b/toolchain/mfc/test_monitor_ci_summary.py index 8f07362cf..b1323553b 100644 --- a/toolchain/mfc/test_monitor_ci_summary.py +++ b/toolchain/mfc/test_monitor_ci_summary.py @@ -31,14 +31,20 @@ def slurm(tmp_path): binz = tmp_path / "bin" binz.mkdir() - def configure(exit_code, output): + # The reported state has to track the exit code: the monitor now requires both + # to say the job succeeded, so a stub that always says FAILED would describe a + # job SLURM never produces for a clean run. + def configure(exit_code, output, state=None): + if state is None: + state = "COMPLETED" if exit_code == "0:0" else "FAILED" + def exe(name, body): path = binz / name path.write_text(body) path.chmod(path.stat().st_mode | stat.S_IEXEC) exe("squeue", "#!/bin/bash\nexit 0\n") - exe("sacct", f'#!/bin/bash\nfor a in "$@"; do [ "$a" = "--format=ExitCode" ] && {{ echo "{exit_code}"; exit 0; }}; done\necho FAILED\n') + exe("sacct", f'#!/bin/bash\nfor a in "$@"; do [ "$a" = "--format=ExitCode" ] && {{ echo "{exit_code}"; exit 0; }}; done\necho "{state}"\n') exe("scontrol", f'#!/bin/bash\necho "ExitCode={exit_code}"\n') exe("scancel", "#!/bin/bash\nexit 0\n") diff --git a/toolchain/mfc/test_monitor_exit_codes.py b/toolchain/mfc/test_monitor_exit_codes.py index 59347905b..f8240b585 100644 --- a/toolchain/mfc/test_monitor_exit_codes.py +++ b/toolchain/mfc/test_monitor_exit_codes.py @@ -91,3 +91,23 @@ def test_the_runner_relays_the_infrastructure_exit_code(tmp_path, monitor_exit): timeout=180, ) assert result.returncode == monitor_exit + + +def test_monitor_fails_a_job_slurm_marked_failed_despite_a_zero_exit_code(slurm): + """A terminal state of FAILED is a failure even when ExitCode reads 0:0. + + SLURM reports that combination intermittently on Phoenix. Deciding purely on + the exit code turned those jobs green with the test failures still in the + log -- 5 of 9 "successful" gpu-acc runs on master were hiding 23 failing + tests this way. + """ + tmp_path, binz, configure = slurm + out = configure("0:0", state="FAILED") + assert run_script(tmp_path, binz, "monitor_slurm_job.sh", "1234", str(out)).returncode == 1 + + +def test_monitor_still_passes_a_genuinely_completed_job(slurm): + """The guard above must not turn healthy jobs red.""" + tmp_path, binz, configure = slurm + out = configure("0:0", state="COMPLETED") + assert run_script(tmp_path, binz, "monitor_slurm_job.sh", "1234", str(out)).returncode == 0 From 558f7ec4a10a6c9b121f0b62f18c34b71c375c86 Mon Sep 17 00:00:00 2001 From: Spencer Bryngelson Date: Sun, 13 Sep 2026 12:09:33 -0400 Subject: [PATCH 2/2] test: pin the monitor's behaviour on every non-COMPLETED terminal state 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 --- toolchain/mfc/test_monitor_exit_codes.py | 32 ++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/toolchain/mfc/test_monitor_exit_codes.py b/toolchain/mfc/test_monitor_exit_codes.py index f8240b585..3a2ad7628 100644 --- a/toolchain/mfc/test_monitor_exit_codes.py +++ b/toolchain/mfc/test_monitor_exit_codes.py @@ -111,3 +111,35 @@ def test_monitor_still_passes_a_genuinely_completed_job(slurm): tmp_path, binz, configure = slurm out = configure("0:0", state="COMPLETED") assert run_script(tmp_path, binz, "monitor_slurm_job.sh", "1234", str(out)).returncode == 0 + + +# Every terminal state SLURM can report for a job that did not complete. The pairing +# with 0:0 is the one the scheduler actually produces intermittently, and the one that +# used to be read as success. +NOT_COMPLETED = [ + "FAILED", + "CANCELLED", + "CANCELLED+", + "TIMEOUT", + "OUT_OF_MEMORY", + "NODE_FAIL", + "BOOT_FAIL", + "DEADLINE", + "REVOKED", +] + + +@pytest.mark.parametrize("state", NOT_COMPLETED) +def test_a_zero_exit_code_does_not_rescue_a_job_that_did_not_complete(slurm, state): + tmp_path, binz, configure = slurm + out = configure("0:0", state=state) + assert run_script(tmp_path, binz, "monitor_slurm_job.sh", "1234", str(out)).returncode == 1 + + +@pytest.mark.parametrize("state", NOT_COMPLETED) +def test_an_infrastructure_fault_still_outranks_the_state(slurm, state): + """77 has to survive: the submit wrapper uses it to exclude the node and resubmit, + and flattening it to a generic failure would strand the job on a bad node.""" + tmp_path, binz, configure = slurm + out = configure("77:0", state=state) + assert run_script(tmp_path, binz, "monitor_slurm_job.sh", "1234", str(out)).returncode == 77