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..3a2ad7628 100644 --- a/toolchain/mfc/test_monitor_exit_codes.py +++ b/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