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

Author: sbryngelson

.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

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")
 

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