Skip to content

fix(static): send the request once when retries is below 1 - #420

Merged
D4Vinci merged 2 commits into
D4Vinci:devfrom
Yigtwxx:fix/static-zero-retries-skips-request
Aug 23, 2026
Merged

fix(static): send the request once when retries is below 1#420
D4Vinci merged 2 commits into
D4Vinci:devfrom
Yigtwxx:fix/static-zero-retries-skips-request

Conversation

@Yigtwxx

@Yigtwxx Yigtwxx commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Proposed change

Both _make_request implementations in scrapling/engines/static.py drive the retry loop straight off the retries value:

max_retries = self._get_param(kwargs, "retries", self._default_retries)
...
    for attempt in range(max_retries):
        ...
finally:
    if session and one_off_request:
        session.close()

raise RuntimeError("No active session available.")  # pragma: no cover

With retries=0 the loop body never runs, so no HTTP request is made at all and control falls through to that trailing raise. The message is also wrong: the session is alive, it was simply never used.

from scrapling import Fetcher

Fetcher.get("https://example.com", retries=0)
# RuntimeError: No active session available.

A negative value behaves the same. retries=None fails one step earlier, on range(None):

Fetcher.get("https://example.com", retries=None)
# TypeError: 'NoneType' object cannot be interpreted as an integer

None is worth handling because it is type-legal on three public surfaces: RequestsSession/GetRequestParams (scrapling/engines/_browsers/_types.py), FetcherSession.__init__(retries: Optional[int] = 3), and _shell_signatures.py. Every entry point is affected — Fetcher, AsyncFetcher, FetcherSession(retries=0) and AsyncFetcherSession.

The browser engines never had this problem, because they declare the same parameter as a bounded type and reject the value cleanly:

# scrapling/engines/_browsers/_validators.py
RetriesCount = Annotated[int, Meta(ge=1, le=10)]
...
retries: RetriesCount = 3
validate({"retries": 0}, PlaywrightConfig)
# TypeError: Invalid argument type: Expected `int` >= 1 - at `$.retries`

The HTTP side has no such bound, and the MCP layer re-exposes the parameter unbounded — ScraplingMCPServer.get and bulk_get both declare retries: Optional[int] = 3 and forward the value unconditionally. An LLM reading "Number of retry attempts. Defaults to 3." and passing 0 to mean "do not retry" gets the bogus RuntimeError instead of a page:

await ScraplingMCPServer().get(url="https://example.com", retries=0)
# RuntimeError: No active session available.

The change

max_retries is clamped to at least one attempt at the single place each method reads it:

# Always attempt the request once; `retries` below 1 (or `None`) means "send it, but don't retry"
max_retries = max(1, self._get_param(kwargs, "retries", self._default_retries) or 1)

or 1 maps None and 0 to 1, and the enclosing max(1, ...) covers negatives — max(1, None) would raise on its own, which would leave the None path broken.

I went with clamping rather than mirroring RetriesCount's ge=1 on the HTTP side, because retries=0 is a value users reasonably pass today meaning "send it once, do not retry", and turning that into a hard failure is a breaking change for a bug fix. It also matches the max_pages clamp in #393.

Since max_retries is the single variable feeding the loop, the attempt < max_retries - 1 check and the f"Failed after {max_retries} attempts" log, all three stay consistent. Retry behaviour for retries >= 1 is untouched, which the existing test_proxy_rotates_per_retry_attempt tests still assert.

The two trailing raise RuntimeError("No active session available.") lines stay as they are. They become genuinely unreachable, but mypy reports Missing return statement on both methods without a terminal raise after the loop, so removing them would mean a larger diff for no behavioural gain.

Tests

Ten cases added across the four existing fetcher test files, none of which touch the public internet:

  • tests/fetchers/{sync,async}/test_requests_session.py: session-level retries of 0, -1 and None, plus a per-request retries=0 overriding a session default of 3. These patch curl_cffi's request and assert call_count == 1, which is stronger than "no exception was raised" — it also catches a fix that loops more than once.
  • tests/fetchers/{sync,async}/test_requests.py: Fetcher.get(..., retries=0) and retries=-1 against pytest_httpbin. This covers the other branch of _make_request — the one-off session that FetcherClient creates — and proves a request actually goes out.

All ten fail on dev and pass with the change. pytest tests/fetchers is green (241 passed), and ruff check, ruff format --check, mypy, pyright, bandit and vermin -t=3.10- are all clean locally on Python 3.12.

Type of change:

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests
  • Add or change doctests? -- Note: Please avoid changing both code and tests in a single pull request.
  • Documentation change?

Additional information

  • This PR fixes or closes an issue: fixes #
  • This PR is related to an issue: #
  • Link to documentation pull request: **

I deliberately left the docstrings alone. After the clamp, ":param retries: Number of retry attempts. Defaults to 3." is not wrong for any input, and that exact sentence exists in eleven places (eight method docstrings in static.py, FetcherSession.__init__, and the two MCP tools) — editing a subset would be inconsistent, and editing all eleven would bury a two-line behaviour fix. Happy to add a sentence about values below 1 if you would rather have it spelled out.

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • All new Python files are placed inside an existing directory.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • All functions and variable names follow Python naming conventions.
  • All function parameters and return values are annotated with Python type hints.
  • All functions have doc-strings.

AI assistance disclosure

Per AI_POLICY.md: I used Claude Code while working on this. It helped me compare how the HTTP fetchers handle retries against the browser validators' RetriesCount, and it drafted the patch and the tests. I found and reproduced the defect myself, decided on clamping over rejecting, checked that no existing test or doc pinned the old behaviour, reviewed the diff, and ran the test suite and the quality checks locally.

`_make_request` looped `for attempt in range(max_retries)`, so a `retries`
value of 0 or below skipped the request entirely and fell through to
`raise RuntimeError("No active session available.")` while the session was
alive. `retries=None` - type-legal through `RequestsSession`,
`GetRequestParams` and `FetcherSession.__init__` - failed one step earlier
with a `TypeError` raised by `range(None)`.

- Clamp `max_retries` to at least one attempt in both the sync and the async
  `_make_request`, so those values send the request once without retrying.
  The clamped value also feeds the `attempt < max_retries - 1` check and the
  "Failed after N attempts" log, so they stay consistent.
- Add regression tests for session-level and per-request `retries` of 0, -1
  and None, sync and async, plus the public `Fetcher`/`AsyncFetcher` path.
@Yigtwxx

Yigtwxx commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Small update rather than a ping: after 1181fc6 the renamed make_request tool still publishes retries: Optional[int] = 3 to the agent, so the path this PR fixes is now reachable straight from the newest MCP surface — no network, no browser:

await ScraplingMCPServer.make_request("http://127.0.0.1:1/", retries=0)
# RuntimeError: No active session available.

The diff here is unchanged and still merges cleanly onto dev.


Disclosure: this comment was written with AI assistance (Claude), per AI_POLICY.md. Reproduced locally against dev at 0cb3e97.

@D4Vinci
D4Vinci merged commit ac18622 into D4Vinci:dev Aug 23, 2026
5 checks passed
@D4Vinci

D4Vinci commented Aug 23, 2026

Copy link
Copy Markdown
Owner

If that happened, then it would be the user doing that for themselves, but anyway, let's fix it. Thanks @Yigtwxx

@Yigtwxx
Yigtwxx deleted the fix/static-zero-retries-skips-request branch August 23, 2026 19:29
@D4Vinci D4Vinci mentioned this pull request Aug 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants