Fail a SLURM job the scheduler marked FAILED, whatever its exit code (#1879)

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,55 @@ 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
+
+
+# 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