diff --git a/.github/workflows/cli-checks.yml b/.github/workflows/cli-checks.yml index 2c88aba..38ebf48 100644 --- a/.github/workflows/cli-checks.yml +++ b/.github/workflows/cli-checks.yml @@ -62,6 +62,7 @@ jobs: - name: Install locked dependencies run: uv sync --locked --all-groups - name: Exercise real CNMV filings + timeout-minutes: 17 env: CNMV_LIVE_TESTS: '1' run: uv run --no-sync pytest -v -s tests/test_live.py diff --git a/skills/cnmv-cli/SKILL.md b/skills/cnmv-cli/SKILL.md index 774590f..f812bfd 100644 --- a/skills/cnmv-cli/SKILL.md +++ b/skills/cnmv-cli/SKILL.md @@ -70,7 +70,7 @@ terminal(command="uv run cnmv filing compare --older-url 'https://www.cnmv.es/ - Exit code `1` with an error containing `must use HTTPS on cnmv.es` means the source URL or a redirect is outside the allowed official origin. Obtain a URL from `filing list`; do not bypass the guard. - `unsupported content type` means the response is not HTML/XHTML. Select a `consolidated_xhtml` or `individual_xhtml` URL instead of a ZIP/PDF endpoint. - `HTML/XHTML document has no body` or `has no visible text` means the response cannot be compared safely. Preserve the failing URL and content type; do not present a partial delta. -- `CNMV request failed` is an upstream/network failure. Retry once after a short delay, then report the failure; do not substitute another source. For `CNMV page is missing`, `multiple ... tables`, or `filing row is missing required cells`, stop and report a CNMV page-format failure; do not retry or guess fields. +- `CNMV request failed: [retryable]` identifies a timeout, connection/transport failure, or temporary HTTP status (408, 429, 500, 502, 503, 504). Retry once after 10 seconds, then report the failure; do not substitute another source. Request errors include the exception type and URL. Errors without this marker, including permanent HTTP errors, certificate failures, and malformed pages, should not be retried. ## Development Verification @@ -82,6 +82,8 @@ terminal(command="uv sync --all-groups && uv run pytest -q && uv run ruff check For a live end-to-end check, first call `filing list`, retain the selected metadata, then compare two real `consolidated_xhtml` URLs with a disposable database directory. Remove generated database directories before staging. +`CNMV_LIVE_TESTS=1 uv run pytest -v -s tests/test_live.py` runs the live smoke plus offline tests of its retry/deadline handling. The smoke shares a 16-minute budget across list, download, compare, and retry delays; the Actions test step is capped at 17 minutes within the 20-minute job. Each attempt logs its limit, duration, and exit status. A subprocess timeout fails immediately with captured output; it does not start another attempt. + ## Pitfalls - CNMV's listing page can expose the same issuer under hyphenated and compact `A`-prefixed NIF forms; `filing list` tries the alternate representation for this pattern. Do not normalize other NIF forms yourself. diff --git a/src/cnmv_cli/download.py b/src/cnmv_cli/download.py index bb1a7d3..d70e254 100644 --- a/src/cnmv_cli/download.py +++ b/src/cnmv_cli/download.py @@ -87,7 +87,7 @@ def download_document(url: str, output: Path) -> dict[str, object]: except (InvalidDocumentUrl, UpstreamError): raise except (urllib.error.URLError, http.client.HTTPException, OSError) as exc: - raise UpstreamError(f"CNMV request failed: {exc}") from exc + raise UpstreamError.from_request(exc, current_url) from exc def _write_response( diff --git a/src/cnmv_cli/filings.py b/src/cnmv_cli/filings.py index a4d7463..56eb3b1 100644 --- a/src/cnmv_cli/filings.py +++ b/src/cnmv_cli/filings.py @@ -1,7 +1,12 @@ from __future__ import annotations +import errno +import http.client import re +import socket +import ssl import unicodedata +import urllib.error from urllib.parse import urljoin import httpx @@ -19,6 +24,51 @@ class MalformedPageError(ValueError): class UpstreamError(RuntimeError): """CNMV could not be reached successfully.""" + @classmethod + def from_request(cls, exc: Exception, url: str) -> UpstreamError: + reason = ( + exc.reason + if isinstance(exc, urllib.error.URLError) + and not isinstance(exc, urllib.error.HTTPError) + else exc + ) + retryable = ( + isinstance( + reason, + ( + httpx.TimeoutException, + httpx.NetworkError, + httpx.RemoteProtocolError, + TimeoutError, + ConnectionError, + http.client.IncompleteRead, + ), + ) + or ( + isinstance(reason, OSError) + and reason.errno in {errno.ENETUNREACH, errno.EHOSTUNREACH} + ) + or ( + isinstance(reason, socket.gaierror) and reason.errno == socket.EAI_AGAIN + ) + ) + if isinstance(exc, (httpx.HTTPStatusError, urllib.error.HTTPError)): + status = ( + exc.response.status_code + if isinstance(exc, httpx.HTTPStatusError) + else exc.code + ) + retryable = status in {408, 429, 500, 502, 503, 504} + # Certificate failures are permanent, even when wrapped as connection errors. + cause = reason + while isinstance(cause, BaseException): + if isinstance(cause, ssl.SSLError): + retryable = False + cause = cause.__cause__ or cause.__context__ + marker = "[retryable] " if retryable else "" + kind = type(reason if isinstance(reason, BaseException) else exc).__name__ + return cls(f"CNMV request failed: {marker}{kind} at {url}: {exc}") + def fetch_filings( nif: str, *, transport: httpx.BaseTransport | None = None @@ -43,7 +93,7 @@ def fetch_filings( ) response.raise_for_status() except httpx.HTTPError as exc: - raise UpstreamError(f"CNMV request failed: {exc}") from exc + raise UpstreamError.from_request(exc, str(exc.request.url)) from exc try: return parse_filings(response.content, str(response.url)) diff --git a/tests/test_cli.py b/tests/test_cli.py index f1aee62..54eeaf1 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,6 +1,7 @@ import json from pathlib import Path +import httpx from typer.testing import CliRunner from cnmv_cli import cli @@ -51,6 +52,7 @@ def fake_fetch(nif: str): def test_filing_list_returns_nonzero_for_clear_service_errors(monkeypatch) -> None: for error in ( UpstreamError("CNMV request failed: 503"), + UpstreamError.from_request(httpx.ReadTimeout(""), "https://www.cnmv.es/list"), MalformedPageError("missing table"), ): monkeypatch.setattr( diff --git a/tests/test_download.py b/tests/test_download.py index 62391c9..ceff264 100644 --- a/tests/test_download.py +++ b/tests/test_download.py @@ -1,4 +1,6 @@ +import errno import hashlib +import socket import urllib.error from collections.abc import Iterable from typing import Self @@ -213,14 +215,51 @@ def test_download_document_stream_cap_preserves_existing_output_and_cleans_temp( assert list(tmp_path.glob(".*.part")) == [] -def test_download_document_wraps_url_errors(monkeypatch, tmp_path) -> None: +@pytest.mark.parametrize( + ("error", "retryable", "kind"), + [ + (urllib.error.URLError("offline"), False, "URLError"), + (urllib.error.URLError(TimeoutError("timed out")), True, "TimeoutError"), + ( + urllib.error.URLError(OSError(errno.ENETUNREACH, "unreachable")), + True, + "OSError", + ), + ( + urllib.error.URLError( + socket.gaierror(socket.EAI_AGAIN, "temporary DNS failure") + ), + True, + "gaierror", + ), + ( + urllib.error.URLError(socket.gaierror(socket.EAI_NONAME, "unknown host")), + False, + "gaierror", + ), + (ConnectionResetError("reset"), True, "ConnectionResetError"), + (PermissionError("denied"), False, "PermissionError"), + ( + FileNotFoundError(errno.ENOENT, "missing directory"), + False, + "FileNotFoundError", + ), + ], +) +def test_download_document_wraps_url_errors( + monkeypatch, tmp_path, error, retryable, kind +) -> None: class FailingOpener: def open(self, request: object, *, timeout: float) -> None: - raise urllib.error.URLError("offline") + raise error monkeypatch.setattr( download.urllib.request, "build_opener", lambda *args: FailingOpener() ) - with pytest.raises(download.UpstreamError, match="CNMV request failed:.*offline"): + with pytest.raises(download.UpstreamError, match="CNMV request failed:") as caught: download_document(DOCUMENT_URL, tmp_path / "document") + message = str(caught.value) + assert ("[retryable]" in message) == retryable + assert kind in message + assert DOCUMENT_URL in message diff --git a/tests/test_filings.py b/tests/test_filings.py index 934850d..a674dcd 100644 --- a/tests/test_filings.py +++ b/tests/test_filings.py @@ -1,3 +1,6 @@ +import ssl +import urllib.error + import httpx import pytest @@ -170,7 +173,7 @@ def handler(request: httpx.Request) -> httpx.Response: ] -@pytest.mark.parametrize("failure", ["status", "connection"]) +@pytest.mark.parametrize("failure", ["status", "connection", "timeout"]) def test_fetch_filings_does_not_retry_upstream_failures(failure: str) -> None: requested_nifs: list[str] = [] @@ -178,12 +181,48 @@ def handler(request: httpx.Request) -> httpx.Response: requested_nifs.append(request.url.params["nif"]) if failure == "connection": raise httpx.ConnectError("offline", request=request) + if failure == "timeout": + raise httpx.ReadTimeout("", request=request) return httpx.Response(503, request=request) - with pytest.raises(UpstreamError, match="CNMV request failed"): + with pytest.raises( + UpstreamError, match="CNMV request failed: \\[retryable\\]" + ) as error: fetch_filings("A12345678", transport=httpx.MockTransport(handler)) assert requested_nifs == ["A12345678"] + assert ( + "https://www.cnmv.es/portal/consultas/ifa/listadoifa?id=0&lang=es&nif=A12345678" + in str(error.value) + ) + if failure == "timeout": + assert "ReadTimeout" in str(error.value) + + +@pytest.mark.parametrize( + "status", [400, 401, 403, 404, 408, 429, 500, 501, 502, 503, 504] +) +def test_request_error_classifies_http_status_for_both_clients(status: int) -> None: + response = httpx.Response(status, request=httpx.Request("GET", PAGE_URL)) + errors = ( + httpx.HTTPStatusError( + "HTTP failure", request=response.request, response=response + ), + urllib.error.HTTPError(PAGE_URL, status, "HTTP failure", {}, None), + ) + for error in errors: + message = str(UpstreamError.from_request(error, PAGE_URL)) + assert ("[retryable]" in message) == (status in {408, 429, 500, 502, 503, 504}) + assert type(error).__name__ in message + assert PAGE_URL in message + + +def test_request_error_does_not_retry_certificate_failures() -> None: + certificate_error = ssl.SSLCertVerificationError("certificate verify failed") + httpx_error = httpx.ConnectError("certificate verify failed") + httpx_error.__cause__ = certificate_error + for error in (httpx_error, urllib.error.URLError(certificate_error)): + assert "[retryable]" not in str(UpstreamError.from_request(error, PAGE_URL)) def test_fetch_filings_does_not_retry_other_malformed_pages() -> None: diff --git a/tests/test_live.py b/tests/test_live.py index 953bd50..0a46aef 100644 --- a/tests/test_live.py +++ b/tests/test_live.py @@ -7,42 +7,164 @@ import subprocess import time from datetime import UTC, datetime +from unittest.mock import Mock import pytest -pytestmark = pytest.mark.skipif( - os.environ.get("CNMV_LIVE_TESTS") != "1", - reason="set CNMV_LIVE_TESTS=1 to contact the real CNMV website", +SMOKE_TIMEOUT = 16 * 60 +RETRY_DELAY = 10 + + +@pytest.fixture +def cli_process(monkeypatch): + now = [0.0] + process = Mock() + sleep = Mock(side_effect=lambda seconds: now.__setitem__(0, now[0] + seconds)) + monkeypatch.setattr(subprocess, "run", process) + monkeypatch.setattr(time, "monotonic", lambda: now[0]) + monkeypatch.setattr(time, "sleep", sleep) + return process, sleep, now + + +@pytest.mark.parametrize("recovers", [True, False]) +def test_run_cli_retries_transient_failure_once(cli_process, capsys, recovers) -> None: + process, sleep, now = cli_process + failure = subprocess.CompletedProcess( + ["cnmv"], + 1, + "", + "Error: CNMV request failed: [retryable] ReadTimeout at https://www.cnmv.es/", + ) + success = subprocess.CompletedProcess(["cnmv"], 0, "result", "") + process.side_effect = [failure, success if recovers else failure] + if recovers: + assert run_cli("filing", "list", deadline=60) == "result" + else: + with pytest.raises(pytest.fail.Exception, match="attempt 2/2.*ReadTimeout"): + run_cli("filing", "list", deadline=60) + assert process.call_count == 2 + sleep.assert_called_once_with(10) + assert [call.kwargs["timeout"] for call in process.call_args_list] == [60, 50] + assert now[0] == 10 + assert "Attempt 1/2 finished in 0.0s, exit=1" in capsys.readouterr().out + + +@pytest.mark.parametrize( + "error", + [ + "Error: CNMV request failed: HTTPStatusError at https://www.cnmv.es/: 404", + "Error: CNMV page is missing the table", + "Error: unsupported content type", + "Error: CNMV request failed: redirect has no Location", + ], ) +def test_run_cli_does_not_retry_permanent_errors(cli_process, error) -> None: + process, sleep, _ = cli_process + process.return_value = subprocess.CompletedProcess(["cnmv"], 1, "", error) + with pytest.raises(pytest.fail.Exception, match="attempt 1/2 failed"): + run_cli("filing", "list", deadline=60) + process.assert_called_once() + sleep.assert_not_called() + + +def test_run_cli_reports_subprocess_timeout_without_retry(cli_process) -> None: + process, sleep, _ = cli_process + process.side_effect = subprocess.TimeoutExpired( + ["cnmv"], 30, b"partial", b"diagnostic" + ) + with pytest.raises( + pytest.fail.Exception, match="TimeoutExpired.*limit=30.0s.*partial.*diagnostic" + ): + run_cli("filing", "compare", deadline=30, timeout=900) + process.assert_called_once() + sleep.assert_not_called() -def run_cli(*arguments: str, timeout: int = 120) -> str: - print(f"Running cnmv {' '.join(arguments)}", flush=True) - for attempt in range(2): - result = subprocess.run( - ["cnmv", *arguments], - capture_output=True, - text=True, - timeout=timeout, - check=False, +def test_run_cli_shares_deadline_between_commands(cli_process) -> None: + process, _, now = cli_process + + def complete(*args, **kwargs): + now[0] += 5 + return subprocess.CompletedProcess(args[0], 0, "result", "") + + process.side_effect = complete + run_cli("filing", "list", deadline=10) + run_cli("filing", "download", deadline=10, timeout=360) + with pytest.raises(pytest.fail.Exception, match="deadline exhausted"): + run_cli("filing", "compare", deadline=10, timeout=900) + assert [call.kwargs["timeout"] for call in process.call_args_list] == [10, 5] + + +def test_run_cli_does_not_sleep_past_deadline(cli_process) -> None: + process, sleep, _ = cli_process + process.return_value = subprocess.CompletedProcess( + ["cnmv"], 1, "", "Error: CNMV request failed: [retryable] ReadTimeout" + ) + with pytest.raises(pytest.fail.Exception, match="deadline leaves no time to retry"): + run_cli("filing", "list", deadline=10) + process.assert_called_once() + sleep.assert_not_called() + + +def run_cli(*arguments: str, deadline: float, timeout: int = 120) -> str: + command = ["cnmv", *arguments] + for attempt in range(1, 3): + started = time.monotonic() + remaining = deadline - started + if remaining <= 0: + pytest.fail(f"Live CNMV deadline exhausted before {' '.join(command)}") + attempt_timeout = min(timeout, remaining) + print( + f"Running {' '.join(command)} (attempt {attempt}/2, " + f"timeout={attempt_timeout:.1f}s)", + flush=True, + ) + try: + result = subprocess.run( + command, + capture_output=True, + text=True, + timeout=attempt_timeout, + check=False, + ) + except subprocess.TimeoutExpired as exc: + pytest.fail( + f"{' '.join(command)} attempt {attempt}/2: TimeoutExpired after " + f"{time.monotonic() - started:.1f}s (limit={attempt_timeout:.1f}s); " + f"stdout={exc.stdout!r}; stderr={exc.stderr!r}" + ) + elapsed = time.monotonic() - started + print( + f"Attempt {attempt}/2 finished in {elapsed:.1f}s, exit={result.returncode}", + flush=True, ) - if ( - attempt == 0 - and result.returncode != 0 - and "CNMV request failed" in result.stderr + if result.returncode == 0: + return result.stdout + error = result.stderr or result.stdout + if attempt == 1 and result.stderr.startswith( + "Error: CNMV request failed: [retryable] " ): - print(f"Retrying one upstream failure: {result.stderr}", flush=True) - time.sleep(3) + if deadline - time.monotonic() <= RETRY_DELAY: + pytest.fail(f"Live CNMV deadline leaves no time to retry: {error}") + print(f"Retrying in {RETRY_DELAY}s: {error}", flush=True) + time.sleep(RETRY_DELAY) continue - break - assert result.returncode == 0, result.stderr or result.stdout - return result.stdout + pytest.fail( + f"{' '.join(command)} attempt {attempt}/2 failed after {elapsed:.1f}s: {error}" + ) +@pytest.mark.skipif( + os.environ.get("CNMV_LIVE_TESTS") != "1", + reason="set CNMV_LIVE_TESTS=1 to contact the real CNMV website", +) def test_live_list_download_and_compare(tmp_path) -> None: + deadline = time.monotonic() + SMOKE_TIMEOUT filings = [ json.loads(line) - for line in run_cli("filing", "list", "--nif", "A08001851").splitlines() + for line in run_cli( + "filing", "list", "--nif", "A08001851", deadline=deadline + ).splitlines() ] assert filings, "CNMV returned no filings for the smoke-test issuer" for filing in filings: @@ -95,6 +217,7 @@ def test_live_list_download_and_compare(tmp_path) -> None: "--output", str(output), timeout=360, + deadline=deadline, ) ) content = output.read_bytes() @@ -118,6 +241,7 @@ def test_live_list_download_and_compare(tmp_path) -> None: "--database", str(database), timeout=900, + deadline=deadline, ) ) for label in ("older", "newer"):