diff --git a/CHANGELOG.md b/CHANGELOG.md index 954f0a1..6fa9711 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,13 @@ notes say so first, because nobody should discover that from their output. ## [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". + ### Added - **CSV cells that a spreadsheet would run as a formula are prefixed with an @@ -25,6 +32,14 @@ notes say so first, because nobody should discover that from their output. ### 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 are written atomically.** `write_json`, `write_csv` and `write_run_meta` opened their target with a truncating `open`, so a crash, a kill or a full disk halfway through left a SHORTER file where the last diff --git a/page_flow.py b/page_flow.py index e461578..98b478e 100644 --- a/page_flow.py +++ b/page_flow.py @@ -467,3 +467,48 @@ def concurrency_refusal(url: str) -> str: def comparable(url: str) -> bool: """Whether two runs of this URL can be diffed against each other.""" return listing_kind(url) in ("search", "posting") + + +# --------------------------------------------------------------------------- +# --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 25e7c17..e09746d 100644 --- a/playwright_scraper.py +++ b/playwright_scraper.py @@ -1498,7 +1498,7 @@ def parse_args(): "?page= and ?s= return the same document. To spread a " "run, use the site's own filters to make several " "narrower URLs and run each of them separately.") - 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 23a004e..db34858 100644 --- a/puppeteer_scraper.py +++ b/puppeteer_scraper.py @@ -1297,7 +1297,7 @@ def parse_args(): "so there is nothing to hand a worker -- in any " "engine. Use the site's own filters to make several " "narrower URLs and run each separately.") - 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 " "matching nothing is a correct answer, not a fault.") diff --git a/selenium_scraper.py b/selenium_scraper.py index 9d78a80..8a04840 100644 --- a/selenium_scraper.py +++ b/selenium_scraper.py @@ -1144,7 +1144,7 @@ def parse_args(): "so there is nothing to hand a worker -- in any " "engine. Use the site's own filters to make several " "narrower URLs and run each separately.") - 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 " "matching nothing is a correct answer, not a fault.") diff --git a/smoke_test.py b/smoke_test.py index 87f1a92..7d92d1b 100644 --- a/smoke_test.py +++ b/smoke_test.py @@ -3224,6 +3224,63 @@ 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(): + 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(): print("smoke_test.py -- craigslist-scraper") print("Zero network, zero browser. Fixtures are real captures, trimmed " @@ -3231,6 +3288,7 @@ def main(): skips = [] ok = True ok &= test_parse_is_gated_on_the_policy() + ok &= test_retries_is_validated_in_every_entry_point() ok &= test_urls() ok &= test_pagination() ok &= test_prices()