diff --git a/.github/scripts/sweep-stalled-ally-reviews.py b/.github/scripts/sweep-stalled-ally-reviews.py index d449fac67784..055a2d8ad5ed 100755 --- a/.github/scripts/sweep-stalled-ally-reviews.py +++ b/.github/scripts/sweep-stalled-ally-reviews.py @@ -44,11 +44,13 @@ """ import argparse +import http.client import json import os import re import sys import time +import traceback import urllib.error import urllib.request from datetime import datetime, timezone @@ -1486,13 +1488,27 @@ def main(argv=None): sys.exit(EXIT_SWEEP_DEGRADED) -if __name__ == "__main__": +def _dispatch(): + """Run `main()` under the abort-vs-alarm exit-code policy. + + A function rather than a bare `if __name__ == "__main__"` body so the arms + below are reachable from the test suite. They were not, and that is why + the `http.client` hole sat open: every arm here is a guard whose whole + purpose is to fire on a path nothing else exercises, and an unreachable + guard is a comment. Tests assert each arm's exit code by calling this. + """ # Every arm here exits EXIT_SWEEP_DEGRADED, never EXIT_ALARM: reaching # this handler means the sweep aborted outright (typically on the initial # open-PR list, before any PR was evaluated), so nothing is known about # whether a PR is stranded. Reporting that as the stranded-PR alarm would # send a human looking for a PR to review when the actual fault is that # the reconciler could not talk to GitHub. + # + # review-gate-sweep.yml fails the job on ANY non-zero exit, so choosing + # EXIT_SWEEP_DEGRADED over EXIT_ALARM does not turn a red run green. What + # it buys is that the red is correctly DIAGNOSED -- "could not talk to + # GitHub" rather than "go review a stranded PR". Judge these arms on the + # log line, not on the workflow conclusion. try: main() except RateLimitExhausted as error: @@ -1507,6 +1523,32 @@ def main(argv=None): # shadow the status-code message above. print("GitHub API request failed (transport): %s" % error.reason, file=sys.stderr) sys.exit(EXIT_SWEEP_DEGRADED) + except http.client.HTTPException as error: + # A truncated or malformed response body raises from `http.client`, + # whose exception tree hangs off Exception and NOT off OSError: + # + # IncompleteRead -> HTTPException -> Exception + # URLError -> OSError + # + # So none of the arms above and none below catch it, and uncaught it + # exits 1 == EXIT_ALARM -- a truncated GitHub page reporting itself as + # "a PR is stranded, go review it". Measured live on run 35605218498 + # (2026-09-21T13:22Z): `IncompleteRead(703602 bytes read, 68373 more + # expected)` while paginating the open-PR list, which a human then had + # to read a traceback to tell apart from a real alarm. Exactly the + # hazard the OSError arm below already documents, arriving through the + # one door that arm cannot cover (BLO-35151). + # + # Catch the HTTPException BASE, not IncompleteRead: BadStatusLine and + # LineTooLong are siblings with identical consequences, and naming + # only the subclass we happened to observe would leave the same hole. + # + # No retry here on purpose. The sweep runs hourly and is idempotent, + # so the schedule already supplies the retry; the 13:22Z truncation + # self-healed at 14:24Z on the same commit. Retrying in-process would + # add a failure mode to buy back an hour that costs nothing. + print("GitHub API request failed (malformed or truncated HTTP response): %r" % error, file=sys.stderr) + sys.exit(EXIT_SWEEP_DEGRADED) except OSError as error: # A REQUEST_TIMEOUT_SECONDS expiry during the response *read* raises a # bare TimeoutError (== socket.timeout), which is an OSError but NOT a @@ -1518,3 +1560,33 @@ def main(argv=None): # itself an OSError, so the arms above still take precedence. print("GitHub API request failed (socket): %r" % error, file=sys.stderr) sys.exit(EXIT_SWEEP_DEGRADED) + except Exception: + # Terminal arm: any exception class not enumerated above (e.g. a + # json.JSONDecodeError == ValueError from _request()'s json.loads on a + # 200 carrying a non-JSON proxy/WAF page during the unisolated open-PR + # pagination) would otherwise escape and exit 1 == EXIT_ALARM. Anything + # reaching here by definition never finished reading the PR list, so it + # is degraded, not an alarm. `Exception`, NOT `BaseException`: main()'s + # deliberate sys.exit(EXIT_ALARM) raises SystemExit, which must pass + # through untouched (BLO-35151). + print("GitHub API sweep crashed before completing: %s" % traceback.format_exc(), file=sys.stderr) + sys.exit(EXIT_SWEEP_DEGRADED) + + +def run_cli(): + """Run `_dispatch()` under the abort-vs-alarm exit-code policy; see its docstring.""" + try: + _dispatch() + except Exception: + # The arm BODIES in _dispatch() are siblings of its terminal arm, not + # inside its try: an exception raised while REPORTING a failure (live + # case: `error.read()` on an HTTPError re-raising IncompleteRead off + # the socket) would escape and exit 1 == EXIT_ALARM. `Exception`, not + # `BaseException`, so each arm's own sys.exit(SystemExit) passes + # through untouched (BLO-35151). + print("GitHub API sweep crashed while reporting a failure: %s" % traceback.format_exc(), file=sys.stderr) + sys.exit(EXIT_SWEEP_DEGRADED) + + +if __name__ == "__main__": + run_cli() diff --git a/.github/scripts/test_sweep_stalled_ally_reviews.py b/.github/scripts/test_sweep_stalled_ally_reviews.py index d63c6fa96d6d..0ac949b7ad9a 100644 --- a/.github/scripts/test_sweep_stalled_ally_reviews.py +++ b/.github/scripts/test_sweep_stalled_ally_reviews.py @@ -7,6 +7,7 @@ """ import contextlib +import http.client import importlib.util import io import os @@ -1966,5 +1967,117 @@ def test_a_clean_run_writes_no_guard_section(self): self.assertNotIn("withheld by the pre-write guard", summary) +class TestRunCliExitCodePolicy(unittest.TestCase): + """Every abort arm in run_cli() must exit DEGRADED, never ALARM. + + EXIT_ALARM is the "a PR is stranded, go review it" signal. CPython exits + 1 on an uncaught exception and EXIT_ALARM is 1, so ANY exception class + that escapes run_cli() silently becomes a false alarm -- it sends a human + to look for review work that does not exist. That is not hypothetical: + `http.client.IncompleteRead` escaped every arm and did exactly this on + run 35605218498 (BLO-35151). + + These tests exist because the arms used to live in a bare + `if __name__ == "__main__"` block, where no test could reach them. + """ + + def _exit_code_for(self, error): + """Raise `error` out of main() and return run_cli()'s exit code.""" + original_main = sweep.main + sweep.main = lambda: (_ for _ in ()).throw(error) + stderr = io.StringIO() + try: + with contextlib.redirect_stderr(stderr): + with self.assertRaises(SystemExit) as caught: + sweep.run_cli() + finally: + sweep.main = original_main + return caught.exception.code, stderr.getvalue() + + def test_alarm_and_degraded_are_distinct_nonzero_codes(self): + """The guard below is meaningless if these two ever collide.""" + self.assertEqual(sweep.EXIT_ALARM, 1) + self.assertNotEqual(sweep.EXIT_SWEEP_DEGRADED, sweep.EXIT_ALARM) + + def test_incomplete_read_is_degraded_not_alarm(self): + code, err = self._exit_code_for(http.client.IncompleteRead(b"partial", 68373)) + self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED) + self.assertIn("truncated HTTP response", err) + + def test_bad_status_line_is_degraded_not_alarm(self): + """The arm catches the HTTPException BASE, not just IncompleteRead. + + Naming only the subclass we happened to observe would leave the same + hole open for its siblings. + """ + code, err = self._exit_code_for(http.client.BadStatusLine("garbage")) + self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED) + self.assertIn("truncated HTTP response", err) + + def test_incomplete_read_is_not_an_oserror(self): + """Pins WHY the pre-existing arms could not catch it. + + If a future Python made HTTPException an OSError subclass this test + fails, flagging that the dedicated arm is now redundant rather than + letting it rot as unexplained duplication. + """ + self.assertFalse(issubclass(http.client.HTTPException, OSError)) + self.assertTrue(issubclass(urllib.error.URLError, OSError)) + + def test_timeout_error_is_degraded_not_alarm(self): + code, err = self._exit_code_for(TimeoutError("read timed out")) + self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED) + self.assertIn("socket", err) + + def test_url_error_keeps_its_transport_message(self): + """Arm precedence is unchanged by the insertion above it.""" + code, err = self._exit_code_for(urllib.error.URLError("dns failure")) + self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED) + self.assertIn("transport", err) + + def test_rate_limit_keeps_its_own_message(self): + code, err = self._exit_code_for(sweep.RateLimitExhausted("budget spent")) + self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED) + self.assertIn("rate limit exhausted", err) + + def test_unenumerated_exception_classes_are_degraded_not_alarm(self): + """Pins the class-level invariant the docstring states, not just the + enumerated arms: ANY escaping exception is degraded, never alarm.""" + import json + for error in ( + json.JSONDecodeError("Expecting value", "", 0), + ValueError("unrelated"), + KeyError("missing"), + RuntimeError("unrelated"), + ): + with self.subTest(error=type(error).__name__): + code, err = self._exit_code_for(error) + self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED) + self.assertIn("crashed before completing", err) + + def test_deliberate_alarm_exit_passes_through_terminal_arm(self): + """SystemExit is a BaseException; the terminal `except Exception` + must not reclassify main()'s own sys.exit(EXIT_ALARM) to degraded.""" + code, _ = self._exit_code_for(SystemExit(sweep.EXIT_ALARM)) + self.assertEqual(code, sweep.EXIT_ALARM) + + + def test_exception_raised_while_reporting_a_failure_is_degraded_not_alarm(self): + """Arm bodies are siblings of the terminal arm, not inside its try. + Live case: HTTPError.read() re-raising IncompleteRead off the socket + while the HTTPError arm formats its message (BLO-35151).""" + class _TruncatedBody: + def read(self): + raise http.client.IncompleteRead(b"partial", 100) + + def close(self): + pass + + error = urllib.error.HTTPError("https://api.github.com/x", 500, "boom", {}, _TruncatedBody()) + code, err = self._exit_code_for(error) + self.assertEqual(code, sweep.EXIT_SWEEP_DEGRADED) + self.assertIn("while reporting a failure", err) + + if __name__ == "__main__": unittest.main()