From 6ddd9b286be0a5e5b893ca3f390e07f63d2cb92e Mon Sep 17 00:00:00 2001 From: Petr Date: Tue, 13 Oct 2026 15:46:53 +0000 Subject: [PATCH] Reject --retries 0 instead of fetching nothing The page-load loop is `range(1, retries + 1)`, so 0 never runs it: nothing is fetched and the engine judges the browser's own about:blank (39 bytes) in a few dozen milliseconds. 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 that declares it, and a check reads the source of each one so a new engine cannot reintroduce the plain int. scraper_api_client.py is left alone where present: it counts extra attempts, so 0 is legal there. Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 15 +++++++++++ page_flow.py | 45 +++++++++++++++++++++++++++++++++ playwright_scraper.py | 2 +- puppeteer_scraper.py | 2 +- selenium_scraper.py | 2 +- smoke_test.py | 59 +++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 122 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a481427..d2efb92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,13 @@ with it, so nobody discovers it from a bill or from a diff. ## [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". + ### Changed - **CSV output now neutralises spreadsheet formulas.** A text cell that @@ -23,6 +30,14 @@ with it, so nobody discovers it from a bill or from a diff. ### 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. + - **Output files were written in place, so a crash mid-write truncated the previous good run.** `write_json`, `write_csv` and the `.meta.json` sidecar now write to a temporary file in the same directory, `fsync`, and diff --git a/page_flow.py b/page_flow.py index e8e5307..7314115 100644 --- a/page_flow.py +++ b/page_flow.py @@ -532,3 +532,48 @@ def page_bound(html: Optional[str], requested: int) -> int: "many instead of the %d requested.", stated, requested) return stated return requested + + +# --------------------------------------------------------------------------- +# --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 ec0eaac..c21249f 100644 --- a/playwright_scraper.py +++ b/playwright_scraper.py @@ -1492,7 +1492,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 5b273df..987f740 100644 --- a/puppeteer_scraper.py +++ b/puppeteer_scraper.py @@ -1041,7 +1041,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: an empty " "hub category is a correct answer, not a fault.") diff --git a/selenium_scraper.py b/selenium_scraper.py index 8860a55..bc029c3 100644 --- a/selenium_scraper.py +++ b/selenium_scraper.py @@ -1038,7 +1038,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: an empty " "hub category is a correct answer, not a fault.") diff --git a/smoke_test.py b/smoke_test.py index 9be891c..f865720 100644 --- a/smoke_test.py +++ b/smoke_test.py @@ -4468,6 +4468,64 @@ def _post(url, **kw): return ok +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 # Checks that could not run because an optional engine library is absent. @@ -4477,6 +4535,7 @@ def main() -> int: skips = [] ok &= test_price_parsing() + ok &= test_retries_is_validated_in_every_entry_point() ok &= test_second_locale() ok &= test_ads() ok &= test_ratings()