fix(contract): cap maxTimeout so one request cannot pin a browser - #9
Merged
Conversation
Nothing bounded maxTimeout above, so a caller could ask for hours and get them. The session such a request marks in use is skipped by the reaper for its whole life, so the browser behind it could not be reclaimed either, and a few of those exhaust the host with no recovery short of a restart. - Clamps to MAX_TIMEOUT_MS (180000 by default) with a warning rather than refusing, so a caller already asking for more keeps working. - The default sits above the measured worst case rather than on a round number: a request that legitimately succeeded in 133 seconds is on record, so a ceiling at or below 120000 would refuse work that currently completes. - A value that cannot be read as a number now says so, instead of raising ValueError from inside the budget arithmetic several frames later. A numeric string still works, because it always has. - A boolean is refused outright, since int(True) quietly became a one millisecond budget. Closes #5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed and why
maxTimeoutwas bounded below but not above, so a caller could ask for hours and get them. The session such a request marks in use is skipped by the reaper for its whole life, so the browser behind it could not be reclaimed either, and a few of those exhaust the host with no recovery short of a restart.It is now clamped to
MAX_TIMEOUT_MS(180000 by default) with a warning, rather than refused. Refusing would break any client already sending a larger value, which the/v1compatibility rule does not allow.Provenance
Filed by
/audit-scanon theresourcesdimension, and recorded as an unfixed inherited issue inHandoff.mdbefore that. The refutation attempt looked for an existing cap inconfig.py,dtos.py, and the request-boundary validators added in 1.4.0, and for any recorded intent to leave it unbounded inCLAUDE.md, the sync ledger, or the CHANGELOG. Nothing.Two deviations from the issue, both deliberate
The issue said reject non-integers. This coerces instead.
int("5000")means a numeric string works end to end today and reaches the budget arithmetic fine, so hard-rejecting one would break callers that currently work. Only a value that cannot be read as a number is refused, and it now says so instead of raisingValueErrorfrom inside the budget arithmetic several frames later. That also closes the "bad parameter types give obscure 500s" complaint inHandoff.md, for this parameter.A boolean is refused outright.
int(True)is 1, somaxTimeout: truequietly became a one millisecond budget.Scope covered
All three sites the issue listed:
src/flaresolverr_service.py:117— the lower-bound-only check, replaced by_validate_max_timeout(req)src/dtos.py:38—maxTimeout: int = None; covered by validating at the boundary rather than by typing the DTO, matching howsession_ttl_minutesandengineare handledsrc/flaresolverr_service.py:394— where the value becomes the budget; now receives a bounded int rather than possibly a stringThe validator sits beside
_validate_session_ttland follows its shape. It is called once in_controller_v1_handler, before command dispatch, rather than at the two per-command sites, because that is where the existing default was already applied.Gate A, browser-free suite
161 tests, OK. 151 before, plus 10 covering the default, the zero and negative fallbacks, the value below and exactly at the ceiling, the clamp above it, a numeric string, an unreadable value, a boolean, and the zero-ceiling escape hatch.
Gate B, live tally against a same-window baseline
No-regression tally, interleaved trial for trial, change on 8291 and baseline on 8391:
Then the mechanism itself, A/B'd in the same window against a host stealth cannot clear from this address, so the budget is always spent in full. The ceiling was set to 15000 on a third container so the bound is reachable in seconds instead of waiting out the 180000 default:
The change stopped at its ceiling, the baseline ran the full ask, and the clamp warning appears in the change container's log and nowhere in the baseline's.
Gate C, the consuming chain
byparr-proxyin front of/v1on a private network, which is the correct chain for a controller change (the passthrough is a separate implementation and none of this code runs in it).One search: HTTP 200, 648830 bytes, 26 result rows, 11.9s, comfortably inside the ~100s an indexer waits before backing off.
The check that mattered: the chain sends
maxTimeout: 120000, and the container logged zero clamp warnings. The 180000 default sits above what the real consumer asks for, so nothing in the deployed path is affected by this change.What was not covered
byparr-proxydirectly with the request an indexer produces. Prowlarr adds scheduling and backoff, neither of which this change touches.src/flaresolverr.py:175runs waitress with its default of 4 threads, so at most about four session-less browsers exist at once, and session browsers are capped bySESSION_MAXat 20 per engine. What remains is that those four threads can each be pinned for the whole budget, so four requests can block the server for as long asmaxTimeoutallows. This change improves that: the worst case goes from unbounded to three minutes. See the correction comment on audit(contract): maxTimeout has no upper bound, so one request can pin a browser indefinitely #5.Review notes
Self-reviewed rather than run through
/pr-review, because this session carries a standing instruction not to spawn subagents unasked. Nothing found on re-read.Closes #5