Repository navigation
fix(research): hard wall-clock fetch deadline that also cuts header and gzip drips - #48
Merged
Merged
Conversation
…nd gzip drips #31 gave fetch_extract a 30 s deadline, but it is only checked between redirect hops and between body chunks. A server dripping one byte every few seconds through its status line/headers (inside requests' header parse), or through a gzip header comment (urllib3's read1 loops over it internally), never trips a per-read timeout and still held the fetch thread for as long as it kept dripping: 4.0 s against a 0.5 s deadline in a loopback probe, i.e. unbounded (review F8 residual, noted in _guarded_get's own docstring). fetch_extract now GETs through a per-fetch requests Session whose pools hand every new socket to a _FetchWatchdog. At the deadline the watchdog shuts down a dup() of each socket, so any blocked recv returns at once, and fetch_extract raises TimeoutError. A body cut short by the watchdog is never returned as complete. Dups are closed and the timer cancelled on every path. Tests: header, gzip-comment and TLS-handshake drips against a loopback stub (header drip ran 8.1 s on main, now about 0.5 s); a socket connected after the deadline is cut at once; a real loopback page still fetches through the watched session with no timer thread or fd left behind. Existing fetch_extract tests now patch Session.get as well as requests.get. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ch_extract _FetchWatchdog.close() cancelled its threading.Timer but did not join it, so the pw-fetch-deadline daemon thread could still be alive for a moment after fetch_extract returned. close() now cancels and then joins it. join() only waits out a _fire that has already started, which holds the lock briefly and makes non-blocking shutdown() calls. The watchdog never writes to stdout. New test: no pw-fetch-deadline thread is alive right after fetch_extract returns, on success, on error and when the deadline fires. The real-socket happy-path test now checks this directly instead of polling for up to 1 s. Both tests fail against the cancel-only close(). Note: this does not explain PR #48's test (3.10) abort (CI run 38124448503). The lines printed just before "Fatal Python error: _enter_buffered_busy" come from the coordinator's pw-baseline daemon thread (net/baseline.py:42 and net/coordinator_app.py:413). Tests that POST /jobs start that thread with the default PW_BASELINE_LOCAL_MODEL, and it races interpreter shutdown. That code is outside this change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-strip-fetch-deadline
Root cause of PR #48's test (3.10) abort (CI run 38124448503, exit 134, "Fatal Python error: _enter_buffered_busy ... possibly due to daemon threads"). It predates this PR; the fetch watchdog is not involved. Every POST /jobs in the suite starts a pw-baseline daemon thread (net/coordinator_app.py). The thread polls for up to 900 s, tries Ollama on loopback with the default PW_BASELINE_LOCAL_MODEL, and then prints. One still running when the suite ended printed "[baseline] generation failed" into interpreter shutdown, and CPython 3.10 aborted. An autouse conftest fixture now refuses to start a pw-baseline thread while the coordinator module's current _baseline_async is the real one. It raises RuntimeError, which takes _start_baseline's own "can't start a thread" path (added in #49), so the slot is released. Emptying PW_BASELINE_LOCAL_MODEL is not enough: the thread still starts, polls and prints. A module-level patch is undone by the importlib.reload() calls many tests make, so the guard sits on Thread.start. The tests from #49 that swap in their own _baseline_async still get real threads. On Python 3.10.21 with the [all,dev] extras, a thread probe at session end before this change: no pw-baseline threads at exit, but [baseline] lines did print mid-suite. After: 0 pw-baseline threads at exit and 0 [baseline] lines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
Context: the review findings this task targets, F7 (quadratic
_HTML_JUNKregex) and F8 (no total fetch deadline, response never closed), were already fixed on main by #31 (2e239f0). The review was written against 30ae508, before #31 landed. This PR closes the one gap #31 left, which its own_guarded_getdocstring noted as a residual: the 30 s deadline was only checked between redirect hops and between body chunks. A server dripping one byte every few seconds can therefore hold the fetch thread indefinitely, with no per-read timeout ever firing, if it drips through either:FCOMMENT(urllib3'sread1loops over it internally).passiveworkers/research.py:_FetchWatchdog: a daemonthreading.Timer. At the deadline it callsshutdown(SHUT_RDWR)on adup()of every socket the fetch opened. The dup is used because it stays valid after TLS wrapping detaches the original socket object.fetch_extractraisesTimeoutError._watched_session: a per-fetchrequests.Session. Its adapter swaps each pool instance'sConnectionClsfor a subclass whose_new_connregisters the new socket. Because it subclasses whatever the pool already uses, the swap also works for HTTPS pools and for the CONNECT tunnel through anHTTPS_PROXY._guarded_get: gains asession=parameter (default: therequestsmodule). SSRF re-validation on every hop andallow_redirects=Falseare unchanged._fetch_body: after the read loop, a watchdog-cut socket that looks like a clean EOF is never returned as a complete body.tests/test_research.py:_patch_gethelper: the existingfetch_extracttests now patchSession.getas well asrequests.get.NO_PROXYpinned and no external network:TimeoutErrorin under 3 s, with_FETCH_DEADLINE=0.5.pw-fetch-deadlinethread and no extra fd.Findings addressed
Before / after timings (loopback,
_FETCH_DEADLINE=0.5, stub server gives up after 4 s)_strip_html, 40k unclosed<nav>_strip_html, 200k lone<*On main, CPython already bounds a TLS handshake with a single deadline. The TLS case is kept only as a regression guard, not claimed as a fix.
New tests on main's
research.py:2 failed, 1 passed.http_headers: 8.1 s, against a 3 s bound._FetchWatchdogdoesn't exist on main.Verification (rebased on af2e814)
python -m py_compile $(git ls-files '*.py')→ exit 0ruff check . && pytest tests/ -q --cov=passiveworkers --cov-fail-under=75→ exit 0:All checks passed!,757 passed, 9 skipped, coverage 80.90%HTTP_PROXY/HTTPS_PROXYset, the new loopback tests still pass (2 passed).NO_PROXYpinned).Not verified / residuals
requests>=2.32.HTTPConnection._new_connandHTTPConnectionPool.ConnectionClsexist there, but this is untested.HTTPAdapter.get_connection_with_tls_context: the swap would not run, and fetches would quietly fall back to main's between-chunk deadline. Releases before 2.32.2 lack this hook; I believe 2.32.0 and 2.32.1 were yanked but did not confirm. The header-drip test fails loudly if the hook isn't called.urllib3/contrib/socks.pyand expects the subclassing to work.TimeoutError. This is a safe failure.getaddrinfo) and per-address connect retries are still outside the watchdog. No socket exists yet at that point.Follow-up: test (3.10) abort in CI run 38124448503 (head 324a242)
Root cause. The abort predates this PR, and the fetch watchdog is not involved. CI aborted with exit 134 in the scoped coverage step:
Fatal Python error: _enter_buffered_busy ... possibly due to daemon threads. The two lines printed just before it come fromnet/baseline.py:42andnet/coordinator_app.py:413, and both run only in the coordinator'spw-baselinedaemon thread. EveryPOST /jobsin the suite starts that thread. It polls for up to 900 s, tries Ollama on loopback with the defaultPW_BASELINE_LOCAL_MODEL, and then prints. One still running when the suite ended printed into interpreter shutdown, and 3.10 aborted. The watchdog never writes to stdout.What changed (on top of a merge of
origin/main, which brings in #49):0b21205passiveworkers/research.py:_FetchWatchdog.close()now cancels and joins thepw-fetch-deadlinetimer, so it never outlivesfetch_extract. Previously it only cancelled it.0b21205tests/test_research.py: newtest_fetch_watchdog_thread_never_outlives_fetch_extract. It checksthreading.enumerate()right afterfetch_extractreturns, on success, on error and when the deadline fires. The real-socket happy-path test now asserts directly instead of polling for 1 s. Both fail against the cancel-onlyclose(): 3/3 runs gave1 failed, 1 passed.324a242tests/conftest.py: new autouse fixture_no_real_baseline_threads. It refuses to start apw-baselinethread while the coordinator's current_baseline_asyncis the real one. It raisesRuntimeError, which takes fix(net): cap coordinator request bodies, bound baseline threads, index hot lookups, throttle rate-limit sweep #49's own "can't start a thread" path in_start_baselineand releases the slot. EmptyingPW_BASELINE_LOCAL_MODELis not enough, because the thread still starts, polls and prints. A module-level patch is undone by theimportlib.reload()calls many tests make, so the guard sits onThread.start. fix(net): cap coordinator request bodies, bound baseline threads, index hot lookups, throttle rate-limit sweep #49's tests that swap in their own_baseline_asyncstill get real threads, and they pass.Verification (local, wikiclaw-1):
ruff check . && pytest tests/ -q --cov=passiveworkers --cov-fail-under=75→ exit 0.All checks passed!,771 passed, 9 skipped, coverage 80.75%.python -m py_compile $(git ls-files '*.py')→ exit 0..[all,dev]like CI):pytest tests/ -q --cov=passiveworkers --cov-report=term-missing --cov-fail-under=75→ exit 0,786 passed, 1 skipped, 83.45%.pytest tests/ -q --cov=passiveworkers.doctor --cov=passiveworkers.net.agent --cov=passiveworkers.operator --cov=passiveworkers.mcp_server --cov-report=term-missing --cov-fail-under=75→ exit 0,786 passed, 1 skipped, 80.38%. The log has 0[baseline]lines and 0Fatal Pythonlines.pw-baselineand 0pw-fetch-deadlinethreads at exit and 0[baseline]lines. Before it,[baseline]lines printed during the suite.Not verified:
pw-reaperdaemon threads (net/_store_jobs.py) are still alive. They print only when a reaper pass fails, so they are a latent version of the same hazard. That code is owned by PR fix(ledger): roll the in-memory ledger back when escrow persistence fails #32 and was left untouched.🤖 Generated with Claude Code