diff --git a/CHANGELOG.md b/CHANGELOG.md index de83b10..520c2fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,13 @@ When it does, the release notes lead with it. ## [Unreleased] +> **Behaviour change:** `--retries 0` is now a usage error (exit 2). It used +> to be accepted and made the page-load loop unreachable, so nothing was +> fetched and the engine reported the browser's own blank page as a result: +> exit 3 or 4 with a 39-byte "capture", in tens of milliseconds. If a +> script of yours passes 0, pass **1**: the flag counts ATTEMPTS, so 1 means +> "try once and do not retry". + > **Behaviour change for existing users:** a page BBB serves that cannot be > read now fails the run instead of ending it. A run that used to finish > `complete` with exit 0 on such a page now finishes `partial` with exit 6 @@ -19,6 +26,14 @@ When it does, the release notes lead with it. ### Fixed +- **`--retries 0` fetched nothing and called it a result.** The load loop is + `range(1, retries + 1)`, so 0 never runs it and the engine judged + `about:blank`. Three third-party audits of this family ran exactly that and + reported a working scraper as broken. `--retries` now goes through + `page_flow.attempts` in every entry point; a check reads the source of each + one and calls the validator, so a new engine declaring the flag with a + plain int fails the suite. + - **A served page that could not be read was taken for the end of the listing.** `unknown` (BBB's page, neither the listing payload nor its own empty-result marker) left the page outcome `ok` with no rows, so the loop diff --git a/page_flow.py b/page_flow.py index 5874743..f2e354c 100644 --- a/page_flow.py +++ b/page_flow.py @@ -268,3 +268,48 @@ def concurrency_limit(cdp_endpoint: Optional[str]) -> Optional[int]: one run each, is the way to parallelise that path (ยง7). """ return 1 if cdp_endpoint else None + + +# --------------------------------------------------------------------------- +# --retries +# --------------------------------------------------------------------------- +import argparse as _argparse # noqa: E402 (kept beside its only user) + + +def attempts(value) -> int: + """argparse `type=` for --retries. Rejects anything below 1. + + WHY THIS IS A USAGE ERROR AND NOT A CLAMP. The page-load loop is + `for attempt in range(1, retries + 1)`, so `--retries 0` makes its body + unreachable: nothing is fetched, and the engine then judges whatever the + browser holds โ€” `about:blank`, 39 bytes of + `` โ€” and reports a block or an + empty result in a few dozen milliseconds. That is a confident, fast, + plausible answer to a question nobody asked. + + Measured on 2026-10-13 against sleepnumber-scraper, from an address the + site genuinely does refuse, so the conclusion even looked right: + + --retries 0 exit 3, 39 bytes, 36 ms <- never left the process + --retries 1 exit 3, 923 bytes, 116 ms <- the real CloudFront 403 + + A network round trip does not finish in 36 ms. Three third-party audits of + the 2scraper scrapers have now run `--retries 0`, read that output, and + reported a working scraper as broken or a site as unreachable. + + The flag counts ATTEMPTS, not re-tries, so a caller who writes 0 almost + certainly means "do not retry", which is 1. Clamping would hide the + mistake; saying so ends the ambiguity. + """ + try: + number = int(value) + except (TypeError, ValueError): + raise _argparse.ArgumentTypeError( + f"--retries must be an integer, not {value!r}") + if number < 1: + raise _argparse.ArgumentTypeError( + f"--retries must be at least 1, not {number}. It counts ATTEMPTS, " + f"not re-tries, so --retries 1 means 'try once and do not retry'. " + f"With 0 the load loop never runs, nothing is fetched, and the " + f"engine reports the browser's blank page as a result.") + return number diff --git a/playwright_scraper.py b/playwright_scraper.py index b9a0b81..8ecbcf6 100644 --- a/playwright_scraper.py +++ b/playwright_scraper.py @@ -1550,7 +1550,7 @@ def parse_args(): "own browser and holds its own proxy exit, so N>1 " "without --proxy-file just sends N times the traffic " "from one address. Ignored with --cdp-endpoint.") - p.add_argument("--retries", type=int, default=3, + p.add_argument("--retries", type=page_flow.attempts, default=3, help="Attempts per page load before giving up (default 3). " "The pause between attempts doubles each time. A page " "that comes back EMPTY is not retried โ€” see " diff --git a/puppeteer_scraper.py b/puppeteer_scraper.py index db59c4e..1e55f03 100644 --- a/puppeteer_scraper.py +++ b/puppeteer_scraper.py @@ -1081,7 +1081,7 @@ def parse_args(): p.add_argument("--concurrency", type=int, default=1, metavar="N", help="Accepted for flag parity and IGNORED here: parallel " "page fetching lives in playwright_scraper.py.") - p.add_argument("--retries", type=int, default=3, + p.add_argument("--retries", type=page_flow.attempts, default=3, help="Attempts per page load before giving up (default 3). " "A page that comes back EMPTY is not retried: a query " "that matched nothing is a correct answer, not a fault.") diff --git a/selenium_scraper.py b/selenium_scraper.py index 86bb8ed..33046b1 100644 --- a/selenium_scraper.py +++ b/selenium_scraper.py @@ -1020,7 +1020,7 @@ def parse_args(): p.add_argument("--concurrency", type=int, default=1, metavar="N", help="Accepted for flag parity and IGNORED here: parallel " "page fetching lives in playwright_scraper.py.") - p.add_argument("--retries", type=int, default=3, + p.add_argument("--retries", type=page_flow.attempts, default=3, help="Attempts per page load before giving up (default 3). " "A page that comes back EMPTY is not retried: a query " "that matched nothing is a correct answer, not a fault.") diff --git a/smoke_test.py b/smoke_test.py index 6948cd2..cd1aeda 100644 --- a/smoke_test.py +++ b/smoke_test.py @@ -2046,6 +2046,61 @@ def verdict(old, new): check("a listing that RAN OUT of pages is not a cut-short run", ok, out) +def _retries_validation_problems(): + """Every shipped --retries flag must go through page_flow.attempts. + + With `--retries 0` the load loop never runs, nothing is fetched, and the + engine reports the browser's blank page as a result. This + reads the SOURCE of every entry point rather than a hand-kept list, so a + new engine that declares the flag with a plain int is caught the day it is + added, and it also CALLS the validator rather than trusting its name. + + scraper_api_client.py is out of scope on purpose: it counts EXTRA attempts + (`retries + 1`), so 0 is legal there and means one try. + """ + import argparse as _ap + import ast as _ast + import importlib as _il + import pathlib as _pl + + root = _pl.Path(__file__).resolve().parent + pf = _il.import_module("page_flow") + problems, seen = [], 0 + for path in sorted(root.glob("*.py")): + if path.name in ("smoke_test.py", "scraper_api_client.py"): + continue + tree = _ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + for node in _ast.walk(tree): + if not (isinstance(node, _ast.Call) + and getattr(node.func, "attr", "") == "add_argument" + and node.args and isinstance(node.args[0], _ast.Constant) + and node.args[0].value == "--retries"): + continue + seen += 1 + kind = next((_ast.unparse(k.value) for k in node.keywords if k.arg == "type"), None) + want = "attempts" if path.name == "page_flow.py" else "page_flow.attempts" + if kind != want: + problems.append("%s:%d declares --retries with type=%s, expected %s" + % (path.name, node.lineno, kind, want)) + if not seen: + problems.append("found no --retries flag in any entry point, so this check " + "would pass for the wrong reason") + for bad in (0, -1, "x"): + try: + pf.attempts(bad) + except _ap.ArgumentTypeError: + continue + problems.append("page_flow.attempts accepted %r" % (bad,)) + if pf.attempts(1) != 1 or pf.attempts("3") != 3: + problems.append("page_flow.attempts rejects a valid count") + return problems + + +def check_retries_is_validated_in_every_entry_point(): + problems = _retries_validation_problems() + check("--retries is validated in every entry point, so 0 is a usage error", not problems, "; ".join(problems)) + + CHECKS = [v for k, v in sorted(globals().items()) if k.startswith("check_")]