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

[ankitranjan7]

Fixes #249.

The problem

--timeout is meant to bound the whole web fetch command. It didn't: a 10-second budget took 68 seconds and then crashed the process.

$ time webcmd web fetch --url https://news.ycombinator.com --timeout 10
… 1:08.65 total

Root cause

Not the retry ladder — that already honoured the deadline. Instrumenting webFetch shows all the fetching finished in 3.2s:

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

The other 65 seconds were spent closing the proxy. proxy.close() waits for every connection to drain, and impit leaves connections open, so the cleanup in finally ignored the budget we had just enforced. Those same leftover connections later fired an unhandled 'error' event, which killed the process instead of returning a usable error.

What I changed

1. src/fetch/safe-proxy.ts — keep a list of every socket the proxy opens (including the upstream halves the HTTP server never sees) and destroy them all in close(). Closing now returns immediately.
2. src/fetch/safe-proxy.ts — socket 'error' handlers destroy the socket instead of re-throwing, so a dead peer can no longer crash the process.
3. src/fetch/client.ts — an aborted fetch (AbortError / TimeoutError) is now converted into our structured TimeoutError, so hitting the budget gives code: TIMEOUT and the TEMPFAIL exit code instead of a raw DOMException.

Proof

	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

New test src/fetch/safe-proxy.test.ts opens a tunnel to a server that never replies and asserts close() finishes in under a second — it hangs until the test timeout without this change. Two more in client.test.ts: 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 (fixed separately in #267).

Not in this PR

• Hacker News still runs the ladder pointlessly and ends in FETCH_BLOCKED — that is a separate defect ([Bug]: web fetch challenge detection false-positives on CSP headers (news.ycombinator.com fails with FETCH_BLOCKED) #264, fixed in fix: stop CSP allow-lists triggering challenge detection (#264) #266). This PR bounds the time regardless of how many tiers run.
• Reading the response body has no timeout of its own (impit's timeout covers the request, not the body stream). Left a ponytail: comment naming the limit rather than building a race for a case we have not seen.

🤖 Generated with Claude Code

[github-actions]

🟢 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.

[beubax]

Maintainer review: changes requested

Issue #249 is valid, and the diagnosis is substantially correct. The retry ladder already shares a deadline, but server.close() can wait for Impit's keep-alive CONNECT tunnels to drain. Tracking both client and upstream sockets and destroying them during teardown is the correct layer to repair that hang.

Two lifetime/error gaps remain.

1. A DNS completion can create a socket after cleanup

Both the HTTP and CONNECT paths await DNS before creating their upstream connection. close() currently:

1. destroys the sockets present in the set;
2. clears the set;
3. calls server.close().

There is no closed/closing state. If DNS resolution is still pending when cleanup starts, the handler can resume afterward and create/track a new upstream socket. Because the set was already drained, that late socket is not destroyed by the close pass and may again keep the process alive or issue an outbound connection after the fetch budget has expired.

Please add a closing barrier:

• mark the proxy closing before destroying sockets;
• have track() immediately destroy any socket presented after that point;
• check the state after awaited DNS and before opening/using an upstream connection;
• make repeated close() calls safe if that is part of the returned interface contract.

The smallest regression test is an injected lookup that does not resolve until after proxy.close() begins. Assert that no late tunnel survives and close completes within the bound.

2. The timeout normalization test models the wrong error shape

asFetchError() maps only errors whose name is TimeoutError or AbortError. The added unit test creates exactly that mocked shape, so it proves the branch but not the pinned Impit behavior.

With the pinned Impit version, a deadline can surface as a generic Error whose message contains its timeout condition rather than one of those names. That path still leaks a generic error instead of the structured Webcmd TIMEOUT envelope.

Please add a representative Impit-shaped timeout test (or a focused integration diagnostic using the pinned client), then normalize that known shape without turning unrelated network failures into timeouts. An elapsed-deadline check at the catch boundary may be safer than broad message matching, provided it cannot mislabel an early failure.

Existing tests

The idle CONNECT test is useful and should stay: it proves that already-established tunnels no longer make close() wait indefinitely. It does not cover the pending-DNS race.

The “closes the safe proxy when the ladder throws” test verifies the existing finally behavior, not the new socket lifecycle. Please keep it if desired, but it cannot substitute for the delayed-lookup regression.

Scope and security

This remains SSRF-sensitive code. The proposed socket destruction does not weaken address validation, but the late-resolution path must not be allowed to open a connection after teardown. Preserve DNS validation and all existing private-address checks.

Today this affects client-owned web fetch, so users need a new CLI release; a Cloud deploy is not required for the current dispatch path.

Once the closing-state race and real timeout normalization are covered, the implementation direction is sound.

[ankitranjan7]

Both gaps addressed in b8132d0.

1. Closing barrier for the late-DNS race

createSafeProxy now has an explicit closing state:

• close() sets closing = true before destroying the tracked sockets;
• track() destroys any socket presented after that point instead of adding it to the (already drained) set;
• both request paths re-check closing immediately after their awaited resolve() and before creating/using the upstream connection — the HTTP path destroys the response, the CONNECT path destroys the client;
• close() is idempotent: the promise is memoized, so a repeated call awaits the first instead of asking a stopped server to close again.

Regression test is the injected lookup you described: it parks the CONNECT handler mid-await, close() starts, then the lookup is released. The upstream server counts accepted connections and the assertion is that it accepted zero, plus close() completing under the bound and a second close() succeeding. I checked it actually bites — removing either guard fails the test.

2. Real timeout normalization

Took the elapsed-deadline approach rather than message matching: the existing name === 'TimeoutError' | 'AbortError' branch stays, and asFetchError now also receives the deadline. Anything failing at or past it becomes the structured TIMEOUT envelope whatever the error calls itself; anything failing with budget left is returned untouched, so a refused connection or DNS failure can't be mislabelled.

Two tests: one rejecting with an impit-shaped generic Error (operation timed out, no special name) after the budget elapses, asserting TIMEOUT; one rejecting with ECONNREFUSED well inside a 30s budget, asserting the original error survives.

Existing tests

Kept both — the idle CONNECT test and the ladder-throws test. Agreed neither substitutes for the delayed-lookup case.

Security

DNS validation and every private-address check are unchanged; the closing state only ever prevents a connection, never permits one. vitest run --project unit src/fetch passes (26 tests), tsc --noEmit clean.