fix: propagate end-to-end operation deadlines #11

Closed
clawlter wants to merge 2 commits from audit/deadline-propagation into main
Owner

Summary

  • propagate one monotonic deadline through health/lazy start, Camofox calls, crawl pages, conversion, and cleanup
  • reserve bounded cleanup time and normalize local/upstream timeout exhaustion
  • enforce the current remaining deadline on each response-body socket read, including trickled or trickled-then-stalled responses, without retaining tab/session state
  • return stable Firecrawl-compatible HTTP 504 errors without upstream detail leakage

Verification

  • ruff format --check .
  • ruff check .
  • pytest -q (30 passed)
  • python3 -m py_compile shim.py ensure_shim.py cron/ensure_camofox_firecrawl_shim.py check_updates.py
  • blocked, trickled, and trickled-then-stalled Camofox response regressions verify wall-clock bounds and tab/session cleanup

Closes task t_981ac561

## Summary - propagate one monotonic deadline through health/lazy start, Camofox calls, crawl pages, conversion, and cleanup - reserve bounded cleanup time and normalize local/upstream timeout exhaustion - enforce the current remaining deadline on each response-body socket read, including trickled or trickled-then-stalled responses, without retaining tab/session state - return stable Firecrawl-compatible HTTP 504 errors without upstream detail leakage ## Verification - `ruff format --check .` - `ruff check .` - `pytest -q` (30 passed) - `python3 -m py_compile shim.py ensure_shim.py cron/ensure_camofox_firecrawl_shim.py check_updates.py` - blocked, trickled, and trickled-then-stalled Camofox response regressions verify wall-clock bounds and tab/session cleanup Closes task t_981ac561
fix: propagate end-to-end operation deadlines
All checks were successful
CI / Validate formatting, linting, and tests (pull_request) Successful in 6m13s
d05015e179
clawlter left a comment

REQUEST CHANGES — important deadline/resource-leak defect

CamofoxClient.request does not enforce the claimed monotonic wall-clock deadline. At shim.py:538-540, urllib.request.urlopen(..., timeout=...) supplies a socket inactivity timeout; response.read() can keep receiving trickled bytes indefinitely and therefore run beyond Deadline.expires_at. Once that happens, _cleanup at shim.py:1016-1034 has no remaining time, so its deadline.timeout(...) checks fail before either DELETE is sent.

I reproduced this against exact head d05015e179 with a deterministic local fake: /tabs creates tab-leaked, then trickles its JSON response every 20 ms. A 0.2 s Deadline completed only after 0.745 s; scrape raised DeadlineExceeded, but the fake retained tabs_after_return=['tab-leaked'] and observed delete_requests=[]. A separate health-response trickle exceeded the same 0.2 s deadline by 0.777 s and returned success.

This violates the core acceptance criteria (end-to-end deadline and no resource leak), so I cannot approve yet. Please enforce a true monotonic wall-clock bound across response-body reads/cancellation, ensure bounded cleanup still executes after an upstream overrun, and add a regression that trickles a response while asserting both elapsed-time bounds and DELETE/no retained tab/session. Re-run the full 28-test suite, formatting/lint/compile, and exact-SHA CI.

REQUEST CHANGES — important deadline/resource-leak defect `CamofoxClient.request` does not enforce the claimed monotonic wall-clock deadline. At shim.py:538-540, `urllib.request.urlopen(..., timeout=...)` supplies a socket inactivity timeout; `response.read()` can keep receiving trickled bytes indefinitely and therefore run beyond `Deadline.expires_at`. Once that happens, `_cleanup` at shim.py:1016-1034 has no remaining time, so its `deadline.timeout(...)` checks fail before either DELETE is sent. I reproduced this against exact head d05015e17981dbc07d858a380a458ed3a95408f9 with a deterministic local fake: `/tabs` creates `tab-leaked`, then trickles its JSON response every 20 ms. A 0.2 s `Deadline` completed only after 0.745 s; `scrape` raised `DeadlineExceeded`, but the fake retained `tabs_after_return=['tab-leaked']` and observed `delete_requests=[]`. A separate health-response trickle exceeded the same 0.2 s deadline by 0.777 s and returned success. This violates the core acceptance criteria (end-to-end deadline and no resource leak), so I cannot approve yet. Please enforce a true monotonic wall-clock bound across response-body reads/cancellation, ensure bounded cleanup still executes after an upstream overrun, and add a regression that trickles a response while asserting both elapsed-time bounds and DELETE/no retained tab/session. Re-run the full 28-test suite, formatting/lint/compile, and exact-SHA CI.
clawlter force-pushed audit/deadline-propagation from d05015e179
All checks were successful
CI / Validate formatting, linting, and tests (pull_request) Successful in 6m13s
to 955af5cf41
Some checks failed
CI / Validate formatting, linting, and tests (pull_request) Has been cancelled
2026-07-11 21:27:58 -04:00
Compare
clawlter force-pushed audit/deadline-propagation from 955af5cf41
Some checks failed
CI / Validate formatting, linting, and tests (pull_request) Has been cancelled
to 4bd84c7251
All checks were successful
CI / Validate formatting, linting, and tests (pull_request) Successful in 2m27s
2026-07-11 22:29:19 -04:00
Compare
clawlter left a comment

CHANGES REQUESTED for exact SHA 4bd84c7251. Blocker: cleanup's tab DELETE timeout cap is applied only to urlopen, then _read_response_body resets the socket timeout to the entire remaining deadline (shim.py:547-549, 575-576, 1066-1084). A deterministic fake where work consumed the work budget and the tab DELETE trickled reproduced 0.402s total for a 0.4s deadline, only GET /health, POST /tabs, POST /tabs/.../wait, and DELETE /tabs/... were issued; no DELETE /sessions/... was attempted. The tab response was interrupted, so both cleanup paths were incomplete and resources can leak. Existing cleanup tests mask this by making session deletion remove tabs and do not record/assert both cleanup calls under a slow cleanup response. Unblock: honor each cleanup call's sub-budget through response-body reads, preserve the overall monotonic deadline, guarantee a bounded session cleanup attempt after a stalled/trickled tab cleanup, add a deterministic regression asserting both cleanup requests/no retained resources, then obtain fresh exact-SHA review and green CI.

CHANGES REQUESTED for exact SHA 4bd84c7251ed2b102f6fedcc198b4e9762285fcd. Blocker: cleanup's tab DELETE timeout cap is applied only to urlopen, then _read_response_body resets the socket timeout to the entire remaining deadline (shim.py:547-549, 575-576, 1066-1084). A deterministic fake where work consumed the work budget and the tab DELETE trickled reproduced 0.402s total for a 0.4s deadline, only GET /health, POST /tabs, POST /tabs/.../wait, and DELETE /tabs/... were issued; no DELETE /sessions/... was attempted. The tab response was interrupted, so both cleanup paths were incomplete and resources can leak. Existing cleanup tests mask this by making session deletion remove tabs and do not record/assert both cleanup calls under a slow cleanup response. Unblock: honor each cleanup call's sub-budget through response-body reads, preserve the overall monotonic deadline, guarantee a bounded session cleanup attempt after a stalled/trickled tab cleanup, add a deterministic regression asserting both cleanup requests/no retained resources, then obtain fresh exact-SHA review and green CI.
fix: preserve cleanup request sub-budgets
All checks were successful
CI / Validate formatting, linting, and tests (pull_request) Successful in 1m48s
55b6a93652
clawlter left a comment

APPROVE exact SHA 55b6a93652. Independent re-review verified the cleanup response-body sub-deadline, bounded tab and session DELETE attempts within one monotonic deadline, no retained fixture resources or post-return requests, stable Firecrawl-compatible 504 behavior, 31-test suite, focused 9-test deadline selection, Ruff format/lint, compile, diff checks, signed remote equality, mergeability, and exact-SHA green Forgejo CI. No blocker or important findings; do not merge as part of this review. Forgejo disallows formal self-approval for the PR author, so this exact-SHA COMMENT records the approval verdict.

APPROVE exact SHA 55b6a9365282b29eb3f7022146b14913ffedce90. Independent re-review verified the cleanup response-body sub-deadline, bounded tab and session DELETE attempts within one monotonic deadline, no retained fixture resources or post-return requests, stable Firecrawl-compatible 504 behavior, 31-test suite, focused 9-test deadline selection, Ruff format/lint, compile, diff checks, signed remote equality, mergeability, and exact-SHA green Forgejo CI. No blocker or important findings; do not merge as part of this review. Forgejo disallows formal self-approval for the PR author, so this exact-SHA COMMENT records the approval verdict.
Author
Owner

Closing without merge because the owner cancelled the code-security-audit-2026-07 campaign. This does not revert previously merged work.

Closing without merge because the owner cancelled the `code-security-audit-2026-07` campaign. This does not revert previously merged work.
clawlter closed this pull request 2026-07-14 08:15:28 -04:00
All checks were successful
CI / Validate formatting, linting, and tests (pull_request) Successful in 1m48s
Required
Details

Pull request closed

Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
clawlter/camofox-firecrawl-shim!11
No description provided.