diff --git a/CHANGELOG.md b/CHANGELOG.md index 37369e4..6ed40e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,23 @@ from a row count, it leads the section in a blockquote. ## [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". + +### 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. + ## [0.2.0] — 2026-10-09 A third-party audit of 8 October, worked end to end. All four of its findings diff --git a/page_flow.py b/page_flow.py index d536d03..2b3b90b 100644 --- a/page_flow.py +++ b/page_flow.py @@ -516,3 +516,48 @@ def concurrency_refusal(url: str = "", mode: str = "") -> Optional[str]: f"{kind} page and it is one fetch. Use --mode events to fetch " f"the event pages a listing names across workers, or run several " f"listings in parallel, one process each") + + +# --------------------------------------------------------------------------- +# --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 a356873..9d5c05f 100644 --- a/playwright_scraper.py +++ b/playwright_scraper.py @@ -1608,7 +1608,7 @@ def parse_args(): "--mode events the twenty event URLs page 1 names ARE " "independent addresses, each worker owns its own " "browser and its own exit, and no lock is needed.") - 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 c12b946..4f9c586 100644 --- a/puppeteer_scraper.py +++ b/puppeteer_scraper.py @@ -1038,7 +1038,7 @@ def parse_args(): "--mode markets and --mode event for a second " "reason: those are one fetch each and there is no " "address to hand a worker.") - 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).") p.add_argument("--retry-delay", type=float, default=2.0, help="Seconds before the first page-load retry, doubling " diff --git a/selenium_scraper.py b/selenium_scraper.py index b41456b..25cc079 100644 --- a/selenium_scraper.py +++ b/selenium_scraper.py @@ -1053,7 +1053,7 @@ def parse_args(): "--mode markets and --mode event for a second " "reason: those are one fetch each and there is no " "address to hand a worker.") - 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).") p.add_argument("--retry-delay", type=float, default=2.0, help="Seconds before the first page-load retry, doubling " diff --git a/smoke_test.py b/smoke_test.py index d69510b..5f7bead 100644 --- a/smoke_test.py +++ b/smoke_test.py @@ -3090,11 +3090,70 @@ def fake_post(url, **kw): +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 test_retries_is_validated_in_every_entry_point(): + group("--retries 0 must be a usage error, not a silent no-op") + problems = _retries_validation_problems() + for line in problems: + print(" " + line) + return check("--retries is validated in every entry point, so 0 is a usage error", not problems) + + def main() -> int: ok = True skips = [] ok &= test_payload_decoding() + ok &= test_retries_is_validated_in_every_entry_point() ok &= test_values_on_real_fixtures() ok &= test_the_three_read_paths() ok &= test_the_page_number_is_threaded_through()