From cd81b2216d02b442d00854efe52fbd6049544b74 Mon Sep 17 00:00:00 2001 From: Derek Roberts Date: Sun, 18 Oct 2026 17:41:58 -0700 Subject: [PATCH 1/2] fix(sync): retry network errors and report unreachable sources A source that cannot be reached is now UNREACHABLE (exit bit 4), separate from drift (1) and check errors (2), so drift and unreachable sources are reported together. The drift job lists unreachable sources in its summary and fails after filing any drift issue. Refs #782 --- .github/workflows/scheduled.yml | 46 +++++++++-- app/scripts/sync_sources.py | 83 ++++++++++++++++---- app/tests/test_sync_sources.py | 130 +++++++++++++++++++++++++++++++- 3 files changed, 236 insertions(+), 23 deletions(-) diff --git a/.github/workflows/scheduled.yml b/.github/workflows/scheduled.yml index 9e04e460..976f52d0 100644 --- a/.github/workflows/scheduled.yml +++ b/.github/workflows/scheduled.yml @@ -47,22 +47,44 @@ jobs: - name: Run Drift Detection id: drift_check + # Exit status (app/scripts/sync_sources.py): bit 1 drift, 2 check error, + # 4 unreachable source. Drift and unreachable sources are reported together. run: | cd app set +e uv run python scripts/sync_sources.py --check --json --alert-body ../issue_body.md > ../drift_report.json EXIT_CODE=$? set -e - echo "exit_code=$EXIT_CODE" >> "$GITHUB_OUTPUT" cat ../drift_report.json - if [ "$EXIT_CODE" -gt 1 ]; then - echo "::error::Drift detection encountered an operational error (exit code $EXIT_CODE)" + if [ "$EXIT_CODE" -gt 7 ]; then + echo "::error::Drift detection did not run to completion (exit code $EXIT_CODE)" exit "$EXIT_CODE" fi - + { + echo "exit_code=$EXIT_CODE" + echo "drift=$(( EXIT_CODE & 1 ? 1 : 0 ))" + echo "check_error=$(( EXIT_CODE & 2 ? 1 : 0 ))" + echo "unreachable=$(( EXIT_CODE & 4 ? 1 : 0 ))" + } >> "$GITHUB_OUTPUT" + { + echo "## Document Drift Check" + echo "" + echo "- Drifted: $(jq '[.[] | select(.status == "DRIFT_DETECTED")] | length' ../drift_report.json)" + echo "- Check errors: $(jq '[.[] | select(.status == "ERROR")] | length' ../drift_report.json)" + echo "- Unreachable: $(jq '[.[] | select(.status == "UNREACHABLE")] | length' ../drift_report.json)" + if [ $(( EXIT_CODE & 6 )) -ne 0 ]; then + echo "" + echo "| Status | Document Path | Upstream URL | Error |" + echo "| :--- | :--- | :--- | :--- |" + jq -r '.[] | select(.status == "ERROR" or .status == "UNREACHABLE") + | "| \(.status) | `\(.path)` | \(.url) | \(.error // "" | gsub("\\|"; "\\|")) |"' ../drift_report.json + fi + } >> "$GITHUB_STEP_SUMMARY" + jq -r '.[] | select(.status == "UNREACHABLE") + | "::error title=Unreachable source::\(.path) (\(.url)): \(.error)"' ../drift_report.json - name: File Drift Alert Issue - if: steps.drift_check.outputs.exit_code == '1' + if: steps.drift_check.outputs.drift == '1' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | @@ -79,4 +101,16 @@ jobs: --label "documentation" fi - + - name: Fail on Unreachable Sources or Check Errors + if: steps.drift_check.outputs.unreachable == '1' || steps.drift_check.outputs.check_error == '1' + env: + UNREACHABLE: ${{ steps.drift_check.outputs.unreachable }} + CHECK_ERROR: ${{ steps.drift_check.outputs.check_error }} + run: | + if [ "$UNREACHABLE" = "1" ]; then + echo "::error::One or more upstream sources could not be reached after retries. See the job summary." + fi + if [ "$CHECK_ERROR" = "1" ]; then + echo "::error::One or more sources failed to check. See the job summary." + fi + exit 1 diff --git a/app/scripts/sync_sources.py b/app/scripts/sync_sources.py index 5465270d..3e1f3259 100755 --- a/app/scripts/sync_sources.py +++ b/app/scripts/sync_sources.py @@ -18,6 +18,7 @@ import os import re import sys +import time import urllib.error import urllib.request from dataclasses import asdict, dataclass @@ -52,6 +53,19 @@ # changes the Markdown those functions emit. A new value forces a full fetch. EXTRACTOR_VERSION = "1" +# A fetch that fails before any HTTP status arrives is retried with backoff. +# An HTTP status response (including 4xx and 5xx) is not retried. +# One delay per retry, so FETCH_ATTEMPTS is the delay count plus the first try. +FETCH_BACKOFF_SECONDS = (5, 15) +FETCH_ATTEMPTS = len(FETCH_BACKOFF_SECONDS) + 1 + +# --check exit status. DRIFT and CHECK_ERROR keep their earlier values; the +# UNREACHABLE bit is added on top, so drift and unreachable sources are both +# visible in one code (1 | 4 == 5). +EXIT_DRIFT = 1 +EXIT_CHECK_ERROR = 2 +EXIT_UNREACHABLE = 4 + @dataclass class SourceEntry: @@ -1004,6 +1018,36 @@ def _header_map(headers: object) -> dict[str, str]: return {str(key).lower(): str(value) for key, value in items()} +def _is_network_error(exc: BaseException) -> bool: + """True for a failure with no HTTP status: DNS, connect, reset, closed connection, timeout. + + ``http.client.RemoteDisconnected`` ("Remote end closed connection without + response") is a ``ConnectionResetError``, so it is a network error. + """ + if isinstance(exc, urllib.error.HTTPError): + return False + return isinstance( + exc, + (urllib.error.URLError, ConnectionError, TimeoutError), + ) + + +def _urlopen_with_retries(req: urllib.request.Request, timeout: int) -> Any: + """Open ``req``; a network error is retried until FETCH_ATTEMPTS attempts are spent.""" + for attempt, delay in enumerate(FETCH_BACKOFF_SECONDS, start=1): + try: + return urllib.request.urlopen(req, timeout=timeout) + except Exception as exc: + if not _is_network_error(exc): + raise + logger.warning( + f"Network error fetching {req.full_url} (attempt {attempt}/{FETCH_ATTEMPTS}): " + f"{exc}. Retrying in {delay}s." + ) + time.sleep(delay) + return urllib.request.urlopen(req, timeout=timeout) + + def fetch_upstream( url: str, timeout: int = 15, @@ -1028,7 +1072,7 @@ def fetch_upstream( req_headers["If-Modified-Since"] = upstream_last_modified req = urllib.request.Request(url, headers=req_headers) try: - resp_ctx = urllib.request.urlopen(req, timeout=timeout) + resp_ctx = _urlopen_with_retries(req, timeout) except urllib.error.HTTPError as exc: if exc.code != 304 or not (etag or upstream_last_modified): raise @@ -1302,16 +1346,16 @@ def check_source_drift(source: SourceEntry, repo_root: Path) -> DriftResult: upstream_hash=registry_hash, upstream_last_modified=source.upstream_last_modified, ) - except (urllib.error.URLError, TimeoutError) as e: - logger.warning(f"Network error checking {source.url}: {e}") - return DriftResult( - path=source.path, - url=source.url, - status="ERROR", - local_hash=local_hash or registry_hash, - error=f"Network error: {e}", - ) except Exception as e: + if _is_network_error(e): + logger.warning(f"Unreachable {source.url}: {e}") + return DriftResult( + path=source.path, + url=source.url, + status="UNREACHABLE", + local_hash=local_hash or registry_hash, + error=f"Network error: {e}", + ) logger.error(f"Failed to check {source.url}: {e}", exc_info=True) return DriftResult( path=source.path, @@ -1430,7 +1474,7 @@ def sync_source( def format_drift_alert_markdown(results: list[DriftResult]) -> str: """Formats markdown issue body for drifted documents.""" drifted = [r for r in results if r.status == "DRIFT_DETECTED"] - errored = [r for r in results if r.status == "ERROR"] + errored = [r for r in results if r.status in ("ERROR", "UNREACHABLE")] lines = [ "## Upstream Document Drift Detected", "", @@ -1541,6 +1585,7 @@ def _run(args: argparse.Namespace) -> int: results: list[DriftResult] = [] has_drift = False has_error = False + has_unreachable = False for entry in entries: logger.info(f"Checking {entry.path}...") @@ -1550,6 +1595,8 @@ def _run(args: argparse.Namespace) -> int: has_drift = True elif res.status == "ERROR": has_error = True + elif res.status == "UNREACHABLE": + has_unreachable = True if args.alert_body and has_drift: args.alert_body.resolve().write_text( @@ -1576,16 +1623,20 @@ def _run(args: argparse.Namespace) -> int: if r.status == "DRIFT_DETECTED": print(f" Local Hash: {r.local_hash}") print(f" Upstream Hash: {r.upstream_hash}") - elif r.status == "ERROR": + elif r.status in ("ERROR", "UNREACHABLE"): print(f" Error: {r.error}") elif r.status == "UNTRACKED": print(f" No local file and no registry hash — run --sync to initialise.") if has_drift: - return 1 - if has_error: - return 2 - return 0 + code = EXIT_DRIFT + elif has_error: + code = EXIT_CHECK_ERROR + else: + code = 0 + if has_unreachable: + code |= EXIT_UNREACHABLE + return code if args.sync or args.dry_run: updated = 0 diff --git a/app/tests/test_sync_sources.py b/app/tests/test_sync_sources.py index ba64b628..aafcc914 100644 --- a/app/tests/test_sync_sources.py +++ b/app/tests/test_sync_sources.py @@ -842,7 +842,7 @@ def test_check_source_drift_network_error(mock_fetch): mock_fetch.side_effect = urllib.error.URLError("Connection refused") res = check_source_drift(entry, REPO_ROOT) - assert res.status == "ERROR" + assert res.status == "UNREACHABLE" assert "Connection refused" in (res.error or "") @@ -2372,3 +2372,131 @@ def test_sync_regenerates_manifest_and_drawer(mock_manifest, tmp_path, monkeypat mock_manifest.assert_called_once_with(data_dir=tmp_path / "app" / "data") +def _no_sleep(monkeypatch) -> list[float]: + delays: list[float] = [] + monkeypatch.setattr(sync_sources.time, "sleep", delays.append) + return delays + + +def test_fetch_upstream_retries_closed_connection_then_succeeds(monkeypatch): + """A connection closed before any response is retried with backoff.""" + import http.client + + delays = _no_sleep(monkeypatch) + calls: list[int] = [] + + def urlopen(req: urllib.request.Request, timeout: int) -> object: + calls.append(1) + if len(calls) < 3: + raise http.client.RemoteDisconnected("Remote end closed connection without response") + response = Mock() + response.headers = EmailMessage() + response.read.return_value = b"ok" + response.__enter__ = Mock(return_value=response) + response.__exit__ = Mock(return_value=False) + return response + + monkeypatch.setattr(sync_sources.urllib.request, "urlopen", urlopen) + + content, _headers = fetch_upstream("https://example.com/doc") + + assert content == "ok" + assert len(calls) == 3 + assert delays == list(sync_sources.FETCH_BACKOFF_SECONDS) + + +def test_fetch_upstream_does_not_retry_http_status(monkeypatch): + """An HTTP status response is an answer from the server, not a network error.""" + delays = _no_sleep(monkeypatch) + calls: list[int] = [] + + def urlopen(req: urllib.request.Request, timeout: int) -> object: + calls.append(1) + raise urllib.error.HTTPError(req.full_url, 503, "Service Unavailable", EmailMessage(), None) + + monkeypatch.setattr(sync_sources.urllib.request, "urlopen", urlopen) + + with pytest.raises(urllib.error.HTTPError): + fetch_upstream("https://example.com/doc") + assert len(calls) == 1 + assert delays == [] + + +def test_check_reports_unreachable_after_retries(monkeypatch, tmp_path): + """Every attempt failing on the network is UNREACHABLE, not drift and not MATCH.""" + import http.client + + _target, digest = _write_doc(tmp_path) + delays = _no_sleep(monkeypatch) + calls: list[int] = [] + + def urlopen(req: urllib.request.Request, timeout: int) -> object: + calls.append(1) + raise http.client.RemoteDisconnected("Remote end closed connection without response") + + monkeypatch.setattr(sync_sources.urllib.request, "urlopen", urlopen) + entry = SourceEntry( + path="app/data/doc.md", + url="https://example.com/doc", + type="html_selector", + selector="#body", + content_hash=digest, + ) + + result = check_source_drift(entry, tmp_path) + + assert result.status == "UNREACHABLE" + assert "Remote end closed connection" in (result.error or "") + assert len(calls) == sync_sources.FETCH_ATTEMPTS + assert len(delays) == sync_sources.FETCH_ATTEMPTS - 1 + + +@pytest.mark.parametrize( + ("statuses", "expected"), + [ + (["MATCH", "MATCH"], 0), + (["DRIFT_DETECTED", "MATCH"], 1), + (["ERROR", "MATCH"], 2), + (["UNREACHABLE", "MATCH"], 4), + (["DRIFT_DETECTED", "UNREACHABLE"], 5), + (["ERROR", "UNREACHABLE"], 6), + ], +) +def test_check_exit_code_reports_drift_and_unreachable_together( + monkeypatch, tmp_path, statuses, expected +): + """Drift never hides an unreachable source, and an unreachable source never hides drift.""" + from scripts.sync_sources import DriftResult + + config = tmp_path / "sources.yaml" + config.write_text( + "sources:\n" + + "".join( + f" - path: app/data/doc{i}.md\n" + f" url: https://example.com/doc{i}\n" + " type: html_selector\n" + " selector: '#body'\n" + " category: resources\n" + f" content_hash: {str(i) * 64}\n" + for i in range(len(statuses)) + ), + encoding="utf-8", + ) + by_path = {f"app/data/doc{i}.md": status for i, status in enumerate(statuses)} + + def fake_check(entry: SourceEntry, repo_root: Path) -> DriftResult: + return DriftResult(path=entry.path, url=entry.url, status=by_path[entry.path]) + + monkeypatch.setattr(sync_sources, "check_source_drift", fake_check) + monkeypatch.setattr(sync_sources, "_REPO_ROOT", tmp_path) + alert = tmp_path / "alert.md" + monkeypatch.setattr( + "sys.argv", + ["sync_sources.py", "--config", str(config), "--check", "--alert-body", str(alert)], + ) + + assert sync_sources.main() == expected + if "DRIFT_DETECTED" in statuses and "UNREACHABLE" in statuses: + body = alert.read_text(encoding="utf-8") + assert "Sources That Could Not Be Reached" in body + assert "doc1.md" in body From 37e77774526fdbc2ee1c80ce524f094c70185119 Mon Sep 17 00:00:00 2001 From: Derek Roberts Date: Sun, 18 Oct 2026 18:04:17 -0700 Subject: [PATCH 2/2] fix(sync): combine every check outcome in the exit status Drift plus a check error is now 3 instead of 1. The drift issue body lists check errors apart from unreachable sources, and the workflow stops when the drift report is missing or invalid. Refs #782 --- .github/workflows/scheduled.yml | 4 ++++ app/scripts/sync_sources.py | 33 +++++++++++++++++---------------- app/tests/test_sync_sources.py | 12 +++++++----- 3 files changed, 28 insertions(+), 21 deletions(-) diff --git a/.github/workflows/scheduled.yml b/.github/workflows/scheduled.yml index 976f52d0..7d66e9b0 100644 --- a/.github/workflows/scheduled.yml +++ b/.github/workflows/scheduled.yml @@ -55,6 +55,10 @@ jobs: uv run python scripts/sync_sources.py --check --json --alert-body ../issue_body.md > ../drift_report.json EXIT_CODE=$? set -e + if ! jq -e 'type == "array"' ../drift_report.json > /dev/null 2>&1; then + echo "::error::Drift report is missing or not valid JSON (exit code $EXIT_CODE)" + exit 1 + fi cat ../drift_report.json if [ "$EXIT_CODE" -gt 7 ]; then echo "::error::Drift detection did not run to completion (exit code $EXIT_CODE)" diff --git a/app/scripts/sync_sources.py b/app/scripts/sync_sources.py index 3e1f3259..6c9096fd 100755 --- a/app/scripts/sync_sources.py +++ b/app/scripts/sync_sources.py @@ -59,9 +59,8 @@ FETCH_BACKOFF_SECONDS = (5, 15) FETCH_ATTEMPTS = len(FETCH_BACKOFF_SECONDS) + 1 -# --check exit status. DRIFT and CHECK_ERROR keep their earlier values; the -# UNREACHABLE bit is added on top, so drift and unreachable sources are both -# visible in one code (1 | 4 == 5). +# --check exit status is a bit set, so outcomes that happen together are all +# visible in one code (drift plus an unreachable source is 1 | 4 == 5). EXIT_DRIFT = 1 EXIT_CHECK_ERROR = 2 EXIT_UNREACHABLE = 4 @@ -1474,7 +1473,8 @@ def sync_source( def format_drift_alert_markdown(results: list[DriftResult]) -> str: """Formats markdown issue body for drifted documents.""" drifted = [r for r in results if r.status == "DRIFT_DETECTED"] - errored = [r for r in results if r.status in ("ERROR", "UNREACHABLE")] + unreachable = [r for r in results if r.status == "UNREACHABLE"] + errored = [r for r in results if r.status == "ERROR"] lines = [ "## Upstream Document Drift Detected", "", @@ -1490,15 +1490,20 @@ def format_drift_alert_markdown(results: list[DriftResult]) -> str: last_mod = d.upstream_last_modified or "Unknown" lines.append(f"| `{d.path}` | [{doc_name}]({d.url}) | `{local_h}` | `{upstream_h}` | {last_mod} |") - if errored: + for heading, group in ( + ("Sources That Could Not Be Reached", unreachable), + ("Sources That Failed To Check", errored), + ): + if not group: + continue lines.extend([ "", - "### Sources That Could Not Be Reached", + f"### {heading}", "", "| Document Path | Upstream URL | Error |", "| :--- | :--- | :--- |", ]) - for e in errored: + for e in group: doc_name = e.path.split("/")[-1] error_msg = (e.error or "Unknown error").replace("|", "\\|") lines.append(f"| `{e.path}` | [{doc_name}]({e.url}) | {error_msg} |") @@ -1628,15 +1633,11 @@ def _run(args: argparse.Namespace) -> int: elif r.status == "UNTRACKED": print(f" No local file and no registry hash — run --sync to initialise.") - if has_drift: - code = EXIT_DRIFT - elif has_error: - code = EXIT_CHECK_ERROR - else: - code = 0 - if has_unreachable: - code |= EXIT_UNREACHABLE - return code + return ( + (EXIT_DRIFT if has_drift else 0) + | (EXIT_CHECK_ERROR if has_error else 0) + | (EXIT_UNREACHABLE if has_unreachable else 0) + ) if args.sync or args.dry_run: updated = 0 diff --git a/app/tests/test_sync_sources.py b/app/tests/test_sync_sources.py index aafcc914..11574d84 100644 --- a/app/tests/test_sync_sources.py +++ b/app/tests/test_sync_sources.py @@ -1082,7 +1082,7 @@ def test_check_source_drift_returns_untracked_when_no_local_file_and_no_hash(tmp def test_format_drift_alert_markdown_includes_error_entries(): - """Regression W4: ERROR sources must appear in the alert body when drift and errors co-occur.""" + """Regression W4: unreachable sources must appear in the alert body when drift and errors co-occur.""" from scripts.sync_sources import DriftResult, format_drift_alert_markdown results = [ @@ -1097,7 +1097,7 @@ def test_format_drift_alert_markdown_includes_error_entries(): DriftResult( path="app/data/01_primary/Some_Policy.md", url="https://example.com/policy", - status="ERROR", + status="UNREACHABLE", error="Network error: timed out", ), ] @@ -2460,6 +2460,8 @@ def urlopen(req: urllib.request.Request, timeout: int) -> object: (["UNREACHABLE", "MATCH"], 4), (["DRIFT_DETECTED", "UNREACHABLE"], 5), (["ERROR", "UNREACHABLE"], 6), + (["DRIFT_DETECTED", "ERROR"], 3), + (["DRIFT_DETECTED", "ERROR", "UNREACHABLE"], 7), ], ) def test_check_exit_code_reports_drift_and_unreachable_together( @@ -2496,7 +2498,7 @@ def fake_check(entry: SourceEntry, repo_root: Path) -> DriftResult: ) assert sync_sources.main() == expected - if "DRIFT_DETECTED" in statuses and "UNREACHABLE" in statuses: + if "DRIFT_DETECTED" in statuses: body = alert.read_text(encoding="utf-8") - assert "Sources That Could Not Be Reached" in body - assert "doc1.md" in body + assert ("Sources That Could Not Be Reached" in body) == ("UNREACHABLE" in statuses) + assert ("Sources That Failed To Check" in body) == ("ERROR" in statuses)