From 6ae607d4c37f2c61a5b06fc5649e46dac63eaac8 Mon Sep 17 00:00:00 2001 From: Yuriy Andamasov Date: Wed, 22 Jul 2026 18:13:55 +0300 Subject: docs-gates: close HTTPError response; name failed assertion in smoke logs (CR round 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two accepted GitHub-side CodeRabbit findings on the smoke gate: Close the HTTPError response stream: _probe_once read the non-2xx body via e.read() but never closed the HTTPError, which is file-like and owns the response socket — leaking it / raising ResourceWarning on the expected-404 path every run. The read is now wrapped in "with e:" INSIDE the crash-containment nesting, so the stream is closed even if the read raises (still yielding the retryable transport-error result, never a traceback). Name the failed assertion in retry/fail logs: _probe_once now returns a compact `detail` naming which check failed ("status" / "docs-build" / "apex-build" / "search-mount", multiple joined by "+", or the transport error text; None when ok) instead of a bare transport-only field. SMOKE-RETRY / SMOKE-FAIL lines gain `detail=<...>` alongside the existing status / docs-build fields, so an apex-build or search-mount failure no longer logs an opaque "status=200 docs-build=". ok-path behavior and the JSON / exit contract are unchanged. Tests: HTTPError stream is closed on the happy-404 path and when the read raises (RecordingBody close recorder); apex-build-only and search-mount-only failures name their detail in the logs; _probe_once joins multiple failed checks with "+". Removed the now-unused _http_error helper. 🤖 Generated by [robots](https://vyos.io) --- scripts/docs_gates/smoke.py | 34 +++++++------- scripts/docs_gates/test_smoke.py | 96 ++++++++++++++++++++++++++++++++++------ 2 files changed, 101 insertions(+), 29 deletions(-) (limited to 'scripts') diff --git a/scripts/docs_gates/smoke.py b/scripts/docs_gates/smoke.py index 2dab82ff..874eb5cf 100644 --- a/scripts/docs_gates/smoke.py +++ b/scripts/docs_gates/smoke.py @@ -87,10 +87,12 @@ def search_mount_present(html: str) -> bool: def _probe_once(host: str, probe: Probe, expect_sha: str, access_id: str, access_secret: str) -> tuple[bool, int | None, str | None, str | None]: - """One probe attempt. Returns (ok, status, docs_build, error). ANY exception in the open - OR body-read path — including a transport error DURING HTTPError.read() — is contained and - yields (False, None, None, ): a retryable failure, never a traceback that crashes the - gate. status/docs_build are surfaced for retry logging.""" + """One probe attempt. Returns (ok, status, docs_build, detail). `detail` names the failed + check(s) — "status" / "docs-build" / "apex-build" / "search-mount" joined by "+", or the + transport error text — and is None when ok. ANY exception in the open OR body-read path + (including a transport error DURING HTTPError.read()) is contained and yields + (False, None, None, ): a retryable failure, never a traceback. The HTTPError + response stream is always closed — it owns a socket, so a bare e.read() would leak it.""" req = urllib.request.Request(f"https://{host}{probe.path}", method="GET") req.add_header("CF-Access-Client-Id", access_id) req.add_header("CF-Access-Client-Secret", access_secret) @@ -100,18 +102,22 @@ def _probe_once(host: str, probe: Probe, expect_sha: str, access_id: str, with _OPENER.open(req, timeout=30) as resp: status, headers, body = resp.status, resp.headers, resp.read() except urllib.error.HTTPError as e: # non-2xx still carries headers/body - status, headers, body = e.code, e.headers, e.read() + with e: # HTTPError is file-like and owns the response socket — always close it + status, headers, body = e.code, e.headers, e.read() except Exception as e: # noqa: BLE001 — open OR read failure → retryable probe result return False, None, None, str(e) - ok = status == probe.expect_status + reasons: list[str] = [] + if status != probe.expect_status: + reasons.append("status") if probe.assert_docs_build and not docs_build_ok(headers.get("X-Docs-Build"), expect_sha): - ok = False + reasons.append("docs-build") if probe.assert_apex_build and not headers.get("X-Apex-Build"): - ok = False + reasons.append("apex-build") if probe.assert_search_mount and not search_mount_present( body.decode("utf-8", errors="replace")): - ok = False - return ok, status, headers.get("X-Docs-Build"), None + reasons.append("search-mount") + detail = "+".join(reasons) if reasons else None + return not reasons, status, headers.get("X-Docs-Build"), detail def run(host: str, slug: str, expect_sha: str, access_id: str, access_secret: str, @@ -139,15 +145,13 @@ def run(host: str, slug: str, expect_sha: str, access_id: str, access_secret: st deadline_hit = True unprobed = pending[i:] # not reached this round → still unresolved break - ok, status, docs_build, error = _probe_once( + ok, status, docs_build, detail = _probe_once( host, probe, expect_sha, access_id, access_secret) if ok: continue still_failing.append(probe) - detail = f"status={status} docs-build={docs_build}" - if error is not None: - detail += f" error={error}" - detail_by_path[probe.path] = detail + detail_by_path[probe.path] = ( + f"status={status} docs-build={docs_build} detail={detail}") pending = still_failing + unprobed if deadline_hit or not pending or round_num == MAX_ROUNDS: break diff --git a/scripts/docs_gates/test_smoke.py b/scripts/docs_gates/test_smoke.py index 063b3a7a..d32ec0cb 100644 --- a/scripts/docs_gates/test_smoke.py +++ b/scripts/docs_gates/test_smoke.py @@ -116,24 +116,30 @@ class _FakeOpener: return item -def _http_error(code: int, headers: dict[str, str]) -> urllib.error.HTTPError: - hdrs = Message() - for k, v in headers.items(): - hdrs[k] = v - return urllib.error.HTTPError("https://host.invalid/x", code, "msg", hdrs, io.BytesIO(b"")) +class _RecordingBody(io.BytesIO): + """HTTPError.fp stand-in: records close() (so tests can assert the response stream is + closed) and can optionally raise on read (a transport error DURING body read). urllib's + addbase binds read through to fp and closes fp on close(), so overriding them here is what + e.read() / closing e actually hit.""" - -class _ReadBoom(io.BytesIO): - """A body whose .read() raises — models a transport error DURING HTTPError.read(). - urllib's addbase binds the instance's read to fp.read, so overriding it on the fp is the - reliable way to make e.read() blow up.""" + def __init__(self, data: bytes = b"", raise_on_read: bool = False): + super().__init__(data) + self.close_calls = 0 + self._raise_on_read = raise_on_read def read(self, *args: object) -> bytes: # noqa: D401 - raise OSError("reset during body read") + if self._raise_on_read: + raise OSError("reset during body read") + return super().read(*args) + + def close(self) -> None: + self.close_calls += 1 + super().close() def _http_error_read_boom(code: int) -> urllib.error.HTTPError: - return urllib.error.HTTPError("https://host.invalid/x", code, "msg", Message(), _ReadBoom()) + return urllib.error.HTTPError( + "https://host.invalid/x", code, "msg", Message(), _RecordingBody(raise_on_read=True)) class _PathOpener: @@ -188,10 +194,10 @@ def test_httperror_read_crash_is_contained_as_failure(monkeypatch): monkeypatch.setattr(smoke, "_OPENER", _FakeOpener([_http_error_read_boom(502)])) probe = smoke.Probe("/en/rolling/index.html", 200, assert_docs_build=False, assert_apex_build=False) - ok, status, docs_build, error = smoke._probe_once("host", probe, "sha", "id", "sec") + ok, status, docs_build, detail = smoke._probe_once("host", probe, "sha", "id", "sec") assert ok is False assert status is None and docs_build is None - assert error is not None and "reset during body read" in error + assert detail is not None and "reset during body read" in detail def test_sleeps_once_per_inter_round_gap_not_per_probe(monkeypatch): @@ -263,3 +269,65 @@ def test_probe_plan_dedups_index_html(): assert len(index_probes) == 1 # not duplicated by the critical list assert index_probes[0].assert_search_mount is True # still the single search-mount probe assert sum(1 for p in plan if p.path == "/en/rolling/cli.html") == 1 # cli.html preserved + + +# --- CR round 2: close the HTTPError response stream (it owns a socket) + name the failed +# assertion in retry/fail logs. --- + +def test_httperror_response_is_closed(monkeypatch): + # HTTPError owns the response socket; the expected-404 path must close it, not leak. + body = _RecordingBody(b"not found") + monkeypatch.setattr(smoke, "_OPENER", _FakeOpener([ + urllib.error.HTTPError("https://host.invalid/x", 404, "msg", Message(), body)])) + probe = smoke.Probe("/en/rolling/missing.html", 404, + assert_docs_build=False, assert_apex_build=False) + ok, *_ = smoke._probe_once("host", probe, "sha", "id", "sec") + assert ok is True # 404 expected → passes + assert body.close_calls >= 1 # response stream closed (no socket leak / ResourceWarning) + + +def test_httperror_response_is_closed_even_when_read_raises(monkeypatch): + body = _RecordingBody(raise_on_read=True) + monkeypatch.setattr(smoke, "_OPENER", _FakeOpener([ + urllib.error.HTTPError("https://host.invalid/x", 502, "msg", Message(), body)])) + probe = smoke.Probe("/en/rolling/index.html", 200, + assert_docs_build=False, assert_apex_build=False) + ok, status, docs_build, detail = smoke._probe_once("host", probe, "sha", "id", "sec") + assert ok is False + assert detail is not None and "reset during body read" in detail + assert body.close_calls >= 1 # close still attempted despite the read raising + + +def test_apex_build_failure_is_named_in_logs(monkeypatch, capsys): + # 200 but no X-Apex-Build: without a named detail the log read "status=200 docs-build=None". + probe = smoke.Probe("/versions.json", 200, assert_docs_build=False, assert_apex_build=True) + monkeypatch.setattr(smoke, "probe_plan", lambda slug, pdf, critical: [probe]) + monkeypatch.setattr(smoke, "MAX_ROUNDS", 1) + monkeypatch.setattr(smoke, "_OPENER", _PathOpener({"/versions.json": [_FakeResp(200, {})]})) + assert smoke.run("host", "rolling", "sha", "id", "sec", None, []) == 1 + err = capsys.readouterr().err + assert "SMOKE-FAIL /versions.json:" in err + assert "detail=apex-build" in err # names the failed check + assert "status=200" in err # existing fields preserved + + +def test_search_mount_failure_is_named_in_logs(monkeypatch, capsys): + # 200 + correct build, but the HTML lacks the #vyos-search mount div. + probe = smoke.Probe("/en/rolling/index.html", 200, assert_docs_build=True, + assert_apex_build=False, assert_search_mount=True) + monkeypatch.setattr(smoke, "probe_plan", lambda slug, pdf, critical: [probe]) + monkeypatch.setattr(smoke, "MAX_ROUNDS", 1) + monkeypatch.setattr(smoke, "_OPENER", _PathOpener({ + "/en/rolling/index.html": [_FakeResp(200, {"X-Docs-Build": "goodsha"}, + b"no search")]})) + assert smoke.run("host", "rolling", "goodsha", "id", "sec", None, []) == 1 + assert "detail=search-mount" in capsys.readouterr().err + + +def test_probe_once_detail_joins_multiple_failed_checks(monkeypatch): + # wrong status AND missing X-Apex-Build → detail "status+apex-build" + monkeypatch.setattr(smoke, "_OPENER", _FakeOpener([_FakeResp(500, {})])) + probe = smoke.Probe("/versions.json", 200, assert_docs_build=False, assert_apex_build=True) + ok, status, docs_build, detail = smoke._probe_once("host", probe, "sha", "id", "sec") + assert ok is False + assert detail == "status+apex-build" -- cgit v1.2.3