Keep webFetch's timeout armed until the response body has been read - #40
Closed
snowyukitty wants to merge 1 commit into
Closed
Keep webFetch's timeout armed until the response body has been read#40snowyukitty wants to merge 1 commit into
snowyukitty wants to merge 1 commit into
Conversation
`fetch()` resolves as soon as the response headers arrive, and the `finally` cleared the timeout right there -- before `readBodyCapped()` ran. The declared 30s bound therefore only covered connect and time-to-headers, and the body read had no deadline at all. The 1 MiB cap does not substitute for one: it bounds total bytes, so a server that answers promptly and then trickles below the cap is never cut off. Measured against a server that flushes headers and then writes one byte every 2s, with the timeout shortened to 5s: today's code was still pending when the client gave up at 25s; with this change it fails at 5039ms. Clear the timer on each exit path instead, and map an abort during the body read onto the same "Fetch timed out" error -- the existing catch wraps only the fetch call, so otherwise a timeout there surfaces as a bare AbortError. Not addressed here, since both change the function's signature or its caller: the abort signal pi-agent-core passes to the tool is still dropped, so Stop cannot cancel a slow fetch; and convertToMarkdown still runs outside the deadline.
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
recheck |
This was referenced Aug 6, 2026
Author
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.
Refs #27 — this covers the timeout half only, so please keep that issue open for the rest (see the
list at the bottom).
fetch()resolves as soon as the response headers arrive, and thefinallyclearedFETCH_TIMEOUT_MSright there — beforereadBodyCapped()ran. The declared 30s bound thereforeonly covered connect and time-to-headers, and the body read had no deadline at all.
readBodyCappedis a barewhile (true) { await reader.read() }with no signal, so a server thatanswers promptly and then stalls holds the agent turn open with nothing to stop it.
The 1 MiB cap does not substitute for a deadline: it bounds total bytes, so a server trickling
below the cap never trips it.
Measured on workerd, against a server that flushes headers then writes one byte every 2s, with
the timeout shortened to 5s:
AbortErrorafter 5057msError: Fetch timed out after 5000msafter 5039msThe middle row is why the diff is not a one-line move: the abort does reach an in-flight
reader.read(), but thecatchthat produces the friendly message wraps only thefetchcall, soa timeout during the body read would otherwise surface as a bare
AbortError. The change clearsthe timer on each exit path and maps an abort during the body read onto the same error.
Two things this deliberately leaves alone, both because they change the function's signature or its
caller, and both noted in #27:
AbortSignalpi-agent-corepasses to a tool asexecute's third argument is still droppedby the
webFetchtool, so Stop still cannot cancel a slow fetch.convertToMarkdownruns after the body read, so it stays outside the deadline.Verified:
pnpm lint:checkandpnpm --filter @gadgets/workshop-backend types:checkpass.