Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,23 @@ user would notice, the release notes say so first.

## [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.

- **The Scraper API path (`scraper_api_client.py`) failed whenever a wait flag
was given, and never saw the target's status.** Measured 2026-09-23 against
the live `/tasks/sync` endpoint: `waitFor` sent as a JSON-encoded string
Expand Down
45 changes: 45 additions & 0 deletions page_flow.py
Original file line number Diff line number Diff line change
Expand Up @@ -469,3 +469,48 @@ def concurrency_refusal(url: str) -> Optional[str]:
if not paginates_by_url(url):
return "%r is not a paginated dubizzle listing" % (url,)
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
`<html><head></head><body></body></html>` — 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
2 changes: 1 addition & 1 deletion playwright_scraper.py
Original file line number Diff line number Diff line change
Expand Up @@ -1514,7 +1514,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 "
Expand Down
2 changes: 1 addition & 1 deletion puppeteer_scraper.py
Original file line number Diff line number Diff line change
Expand Up @@ -1082,7 +1082,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.")
Expand Down
2 changes: 1 addition & 1 deletion selenium_scraper.py
Original file line number Diff line number Diff line change
Expand Up @@ -1018,7 +1018,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.")
Expand Down
59 changes: 59 additions & 0 deletions smoke_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3007,6 +3007,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.
Expand All @@ -3016,6 +3074,7 @@ def main() -> int:
skips = []

ok &= test_parse_is_gated_on_the_policy()
ok &= test_retries_is_validated_in_every_entry_point()
ok &= test_price_parsing()
ok &= test_listing_values()
ok &= test_null_price_is_the_site_and_not_the_parser()
Expand Down
Loading