diff --git a/.github/workflows/scheduled.yml b/.github/workflows/scheduled.yml index 9e04e460..7d66e9b0 100644 --- a/.github/workflows/scheduled.yml +++ b/.github/workflows/scheduled.yml @@ -47,22 +47,48 @@ 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" + 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 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 +105,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..6c9096fd 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,18 @@ # 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 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 + @dataclass class SourceEntry: @@ -1004,6 +1017,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 +1071,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 +1345,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,6 +1473,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"] + 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", @@ -1446,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} |") @@ -1541,6 +1590,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 +1600,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 +1628,16 @@ 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 + 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 ba64b628..11574d84 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 "") @@ -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", ), ] @@ -2372,3 +2372,133 @@ 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), + (["DRIFT_DETECTED", "ERROR"], 3), + (["DRIFT_DETECTED", "ERROR", "UNREACHABLE"], 7), + ], +) +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: + body = alert.read_text(encoding="utf-8") + assert ("Sources That Could Not Be Reached" in body) == ("UNREACHABLE" in statuses) + assert ("Sources That Failed To Check" in body) == ("ERROR" in statuses)