Skip to content

fix: honour the web fetch timeout budget end to end (#249) - #265

Open
ankitranjan7 wants to merge 1 commit into
mainfrom
fix/249-web-fetch-timeout
Open

fix: honour the web fetch timeout budget end to end (#249)#265
ankitranjan7 wants to merge 1 commit into
mainfrom
fix/249-web-fetch-timeout

Conversation

@ankitranjan7

Copy link
Copy Markdown
Contributor

Fixes #249.

Root cause

--timeout already set a deadline the retry ladder honoured — the ladder is not where the time went. Instrumenting webFetch against the URL from the issue:

0.9s plain 200            challenge=true
0.9s impit start chrome   remaining=4088ms
2.1s impit done  chrome   200
2.1s impit start firefox  remaining=2932ms
3.2s impit done  firefox  200
3.2s closing proxy
...
67.9s UNCAUGHT This socket has been ended by the other party

Everything finished in 3.2s. The remaining 65s was proxy.close(): it calls server.close(), which stays pending until every connection drains, and impit leaves keep-alive CONNECT tunnels open. The budget was enforced on the requests and then thrown away in the finally. The same dangling sockets surfaced later as an unhandled 'error' event that killed the process instead of producing a usable error.

Fix

  • src/fetch/safe-proxy.ts tracks every socket it opens — server connections, the CONNECT client half, and the upstream halves the HTTP server never learns about — and destroys them in close(). server.close() then returns immediately.
  • Socket 'error' handlers destroy rather than propagate, so a torn-down peer cannot crash the process.
  • src/fetch/client.ts maps an aborted fetch (AbortError / DOM TimeoutError) to the structured TimeoutError, so the budget being hit yields code: TIMEOUT and the TEMPFAIL exit code rather than a leaked DOMException.

Measured

before after
web fetch --url https://news.ycombinator.com --timeout 5 68.5s, process crash 3.4s, clean error
host that accepts and never responds, --timeout 3 ~68s 3.3s, TimeoutError: web fetch timed out after 3s

Tests

src/fetch/safe-proxy.test.ts opens a CONNECT tunnel to a server that never replies and asserts close() resolves in under a second — it hangs until the test timeout without this change. Plus two client.test.ts cases: the proxy is closed when the ladder throws, and an aborted fetch surfaces as TIMEOUT.

Full suite passes except tests/e2e/plugin-management.test.ts, which fails identically on main.

Noted, not fixed

  • Hacker News still runs the impit ladder pointlessly and ends in FETCH_BLOCKED: its CSP header names cdnjs.cloudflare.com and recaptcha, and isChallengeResponse matches its markers against every header value. Separate defect, filed as [Bug]: web fetch challenge detection false-positives on CSP headers (news.ycombinator.com fails with FETCH_BLOCKED) #264. This PR bounds the budget regardless of how many tiers run.
  • readBody() on an impit response is not bounded by any signal — impit's timeout covers the request, not the body stream. Marked with a ponytail: comment naming the ceiling rather than building a race for a case not yet observed.

🤖 Generated with Claude Code

`--timeout` set a deadline the retry ladder respected, but the teardown
did not: `proxy.close()` calls `server.close()`, which stays pending
until every connection drains, and impit leaves keep-alive CONNECT
tunnels open. A 5s budget against news.ycombinator.com took 68s — the
ladder finished in 3.2s and the rest was the close waiting for the OS to
drop the tunnels. The same dangling sockets then crashed the process
with an unhandled 'error' event.

Track every socket the proxy opens, including the upstream halves the
HTTP server never sees, and destroy them in close(). Swallow socket
errors so a destroyed peer cannot take down the process. Map an aborted
fetch to the structured TimeoutError instead of leaking a DOMException.

Measured after: 3.4s for the same command, and a host that never
responds now fails at the deadline with `TIMEOUT: web fetch timed out
after 3s`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

🟢 No documentation gap found — medium confidence

The automated review found no documentation gap in the supplied changes.

This review is advisory and does not block merging.

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.

[Bug]: web fetch --timeout is not honored across the retry ladder (10s budget took 68s)

2 participants