diff --git a/CHANGELOG.md b/CHANGELOG.md index 8502031..804628b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,8 +11,23 @@ empty output file. ## [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. + - **A refused `--cdp-endpoint` explained the wrong cause.** Every connect failure was followed by the same paragraph about `profile_locked`, so an expired login (HTTP 401, `deny_no_user`) sent the reader looking for a diff --git a/page_flow.py b/page_flow.py index c70a349..dcfb3f0 100644 --- a/page_flow.py +++ b/page_flow.py @@ -547,3 +547,48 @@ def concurrency_refusal(url: str) -> Optional[str]: f"— /tag/{{slug}}/archive/{{yyyy}}/{{mm}}/{{dd}} — to use " f"workers, or run several tags in parallel, one process each") return 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 6f1dd3e..1769401 100644 --- a/playwright_scraper.py +++ b/playwright_scraper.py @@ -1503,7 +1503,7 @@ def parse_args(): "exists only inside the browser that scrolled " "through batches 1-4. Run several tags or authors in " "parallel instead, one process each.") - 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 c1607b4..196c5d4 100644 --- a/puppeteer_scraper.py +++ b/puppeteer_scraper.py @@ -993,7 +993,7 @@ def parse_args(): "implemented in this engine: the worker pool lives in " "playwright_scraper.py, and this engine walks archive " "days one at a time with identical output.") - 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 19cc0de..a7c9d19 100644 --- a/selenium_scraper.py +++ b/selenium_scraper.py @@ -1005,7 +1005,7 @@ def parse_args(): "implemented in this engine: the worker pool lives in " "playwright_scraper.py, and this engine walks archive " "days one at a time with identical output.") - 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 316eec2..34e6e2f 100644 --- a/smoke_test.py +++ b/smoke_test.py @@ -2709,11 +2709,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_medium_encodings() + ok &= test_retries_is_validated_in_every_entry_point() ok &= test_values_on_real_fixtures() ok &= test_the_four_read_paths() ok &= test_a_post_page_holds_more_than_its_own_story()