From fa341657634adb9d5f7187b4563477d01fa5e73f Mon Sep 17 00:00:00 2001 From: t Date: Mon, 3 Aug 2026 19:46:29 +0800 Subject: [PATCH] fix(ops): make the alert steps actually run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Run 30798868809 detected a real outage and told nobody. The publication heartbeat correctly exited 1 after 101 hours without a new post, and the delivery step was skipped. GitHub's default shell is `bash --noprofile --norc -eo pipefail`, which already sets -e. The steps declared `set -uo pipefail`, which adds -u but does not remove -e, so the non-zero exit aborted the step before `code=$?` and the `echo "exit_code=..." >> $GITHUB_OUTPUT` ever ran. The delivery step guards on `steps..outputs.exit_code != ''`, which was therefore false, so it skipped. The check worked. The notification never existed. That is precisely the failure mode this monitoring was built to eliminate, reintroduced one layer up. The same run shows a second defect: the release-state check was skipped too, because a failed heartbeat aborted the remaining steps. One broken check hid whatever else was wrong. Disable errexit around each measurement so the code is recorded, and let the checks run independently of each other. Four tests pin both properties, since the symptom of getting this wrong is silence — which is indistinguishable from health, and cannot be noticed by watching. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/monitoring.yml | 19 ++++++++ tests/test_monitoring_alert_wiring.py | 70 +++++++++++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 tests/test_monitoring_alert_wiring.py diff --git a/.github/workflows/monitoring.yml b/.github/workflows/monitoring.yml index f3b485356..a411cf22f 100644 --- a/.github/workflows/monitoring.yml +++ b/.github/workflows/monitoring.yml @@ -48,7 +48,11 @@ jobs: - name: Measure publication heartbeat id: heartbeat run: | + # GitHub's default shell already sets -e, so a non-zero exit would + # abort before the code is recorded and the delivery step would be + # skipped — the check would detect the outage and tell no one. set -uo pipefail + set +e python3 scripts/publish_heartbeat.py \ --repository-root . \ --summary-output "$RUNNER_TEMP/heartbeat.md" @@ -85,10 +89,15 @@ jobs: - name: Enforce divergence and stale thresholds id: release_state + if: always() env: MAIN_SHA: ${{ github.sha }} run: | + # GitHub's default shell already sets -e, so a non-zero exit would + # abort before the code is recorded and the delivery step would be + # skipped — the check would detect the outage and tell no one. set -uo pipefail + set +e main_sha="$(git rev-parse HEAD)" main_committed_at="$(git show -s --format=%cI HEAD)" python3 scripts/production_monitor.py \ @@ -139,10 +148,15 @@ jobs: # so it has to be caught here rather than hours later via staleness. - name: Check recent release runs id: release_runs + if: always() env: GH_TOKEN: ${{ github.token }} run: | + # GitHub's default shell already sets -e, so a non-zero exit would + # abort before the code is recorded and the delivery step would be + # skipped — the check would detect the outage and tell no one. set -uo pipefail + set +e python3 scripts/check_release_runs.py \ --repository "$GITHUB_REPOSITORY" \ --summary-output "$RUNNER_TEMP/release_runs.md" @@ -180,10 +194,15 @@ jobs: # scheduled work rather than an outage. - name: Measure capacity headroom id: capacity + if: always() env: PYTHONPATH: ${{ github.workspace }} run: | + # GitHub's default shell already sets -e, so a non-zero exit would + # abort before the code is recorded and the delivery step would be + # skipped — the check would detect the outage and tell no one. set -uo pipefail + set +e python3 scripts/capacity_report.py \ --summary-output "$RUNNER_TEMP/capacity.md" \ --fail-on never > "$RUNNER_TEMP/capacity.json" diff --git a/tests/test_monitoring_alert_wiring.py b/tests/test_monitoring_alert_wiring.py new file mode 100644 index 000000000..a5e854de8 --- /dev/null +++ b/tests/test_monitoring_alert_wiring.py @@ -0,0 +1,70 @@ +"""Wiring invariants for the monitoring job's alert delivery. + +Run 30798868809 detected a real outage — the publication heartbeat correctly +exited 1 after 101 hours without a new post — and told nobody. GitHub's default +shell already sets -e, so the non-zero exit aborted the step before +`exit_code` reached $GITHUB_OUTPUT, and the delivery step's +`steps..outputs.exit_code != ''` guard was therefore false. The alert was +skipped. + +That is exactly the failure this monitoring exists to prevent, so it is pinned +here rather than left to review. +""" + +from __future__ import annotations + +from pathlib import Path + +import yaml + +ROOT = Path(__file__).resolve().parents[1] +WORKFLOW = ROOT / ".github" / "workflows" / "monitoring.yml" + + +def _steps() -> list[dict]: + document = yaml.safe_load(WORKFLOW.read_text(encoding="utf-8")) + return document["jobs"]["verify-production-state"]["steps"] + + +def test_every_measurement_disables_errexit_before_capturing_its_code() -> None: + for step in _steps(): + run = str(step.get("run") or "") + if "GITHUB_OUTPUT" not in run or "exit_code" not in run: + continue + assert "set +e" in run, ( + f"{step.get('name')} records an exit code, but GitHub's default shell " + "sets -e; without 'set +e' the step aborts before writing the output " + "and its alert is skipped" + ) + + +def test_every_delivery_step_runs_even_after_its_measurement_fails() -> None: + for step in _steps(): + name = str(step.get("name") or "") + if not name.startswith("Deliver"): + continue + condition = str(step.get("if") or "") + assert condition.startswith("always()"), ( + f"{name} must run after a failed measurement, otherwise a detected " + "outage produces no notification" + ) + + +def test_checks_do_not_block_one_another() -> None: + # A failing heartbeat previously skipped the release-state and capacity + # checks entirely, hiding whatever else was wrong. + ids = {"release_state", "release_runs", "capacity"} + for step in _steps(): + if step.get("id") in ids: + assert str(step.get("if") or "").startswith("always()"), ( + f"{step.get('name')} must not be skipped because an earlier check failed" + ) + + +def test_the_job_still_reports_unhealthy_in_its_own_colour() -> None: + gate = [s for s in _steps() if "Fail the job" in str(s.get("name") or "")] + assert gate, "the job must fail on its own when production is unhealthy" + run = str(gate[0]["run"]) + # A broken alert channel must not be able to make an outage look healthy. + for signal in ("heartbeat", "release_state", "release_runs", "capacity"): + assert signal in run