From f1ce2fef5d73dac6fa425d0c71da3b8a2ad2c9cd Mon Sep 17 00:00:00 2001 From: Ankit Ranjan Date: Mon, 10 Aug 2026 18:51:36 +0530 Subject: [PATCH 1/2] fix: honour the web fetch timeout budget end to end (#249) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--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 --- src/fetch/client.test.ts | 17 +++++++++++++++++ src/fetch/client.ts | 13 +++++++++++++ src/fetch/safe-proxy.test.ts | 28 +++++++++++++++++++++++++++- src/fetch/safe-proxy.ts | 27 +++++++++++++++++++++++++-- 4 files changed, 82 insertions(+), 3 deletions(-) diff --git a/src/fetch/client.test.ts b/src/fetch/client.test.ts index dd5ee867..38325eb0 100644 --- a/src/fetch/client.test.ts +++ b/src/fetch/client.test.ts @@ -19,4 +19,21 @@ describe('webFetch', () => { expect(createImpit).toHaveBeenNthCalledWith(1, expect.objectContaining({ browser: 'chrome' })); expect(createImpit).toHaveBeenNthCalledWith(2, expect.objectContaining({ browser: 'firefox' })); }); + it('closes the safe proxy even when the ladder throws', async () => { + const close = vi.fn().mockResolvedValue(undefined); + await expect(webFetch({ url: 'https://example.com', timeoutSeconds: 5, maxChars: 0, allowPrivate: false }, { + plainFetch: vi.fn().mockRejectedValue(new Error('boom')), + createImpit: vi.fn(), + createSafeProxy: async () => ({ url: 'http://proxy', close }), + })).rejects.toThrow('boom'); + expect(close).toHaveBeenCalledOnce(); + }); + it('reports an aborted fetch as a structured timeout', async () => { + const abort = Object.assign(new Error('The operation was aborted'), { name: 'TimeoutError' }); + await expect(webFetch({ url: 'https://example.com', timeoutSeconds: 5, maxChars: 0, allowPrivate: false }, { + plainFetch: vi.fn().mockRejectedValue(abort), + createImpit: vi.fn(), + createSafeProxy: async () => ({ url: 'http://proxy', close: async () => {} }), + })).rejects.toMatchObject({ code: 'TIMEOUT', message: 'web fetch timed out after 5s' }); + }); }); diff --git a/src/fetch/client.ts b/src/fetch/client.ts index f9451c87..d06deffb 100644 --- a/src/fetch/client.ts +++ b/src/fetch/client.ts @@ -51,6 +51,9 @@ export async function webFetch(options: WebFetchOptions, dependencies: WebFetchD let tier: WebFetchResult['tier'] = 'plain'; let profile: WebFetchResult['profile']; if (isJavaScriptShell(body)) throw new CliError('FETCH_REQUIRES_BROWSER', 'This page requires browser rendering.', 'Use webcmd web fetch-browser for this URL.'); if (isChallengeResponse(response.status, headersOf(response), body)) { + // ponytail: impit's timeout covers the request, not the body stream, so a + // trickling escalation body can outlive the budget. Race readBody against + // the deadline if that shows up in practice. for (const browser of ['chrome', 'firefox'] as const) { const impit = createImpit({ browser, proxyUrl: proxy.url, timeout: remaining() }); response = await impit.fetch(options.url, { redirect: 'manual', timeout: remaining() }); @@ -63,5 +66,15 @@ export async function webFetch(options: WebFetchOptions, dependencies: WebFetchD const extracted = extractFetchedContent({ body, contentType: response.headers.get('content-type') ?? '', url: options.url }); const clipped = truncate(extracted.content, options.maxChars); return { status: response.status, requestedUrl: options.url, finalUrl: response.url || options.url, contentType: response.headers.get('content-type') ?? '', tier, ...(profile && { profile }), title: extracted.title, extractionSource: extracted.source, truncated: clipped.truncated, content: clipped.content }; + } catch (error) { + throw asFetchError(error, options.timeoutSeconds); } finally { await proxy.close(); } } + +/** An aborted fetch surfaces as a DOMException; agents need the structured timeout instead. */ +function asFetchError(error: unknown, timeoutSeconds: number): unknown { + if (error instanceof CliError) return error; + const name = (error as { name?: string } | null)?.name; + if (name === 'TimeoutError' || name === 'AbortError') return new TimeoutError('web fetch', timeoutSeconds); + return error; +} diff --git a/src/fetch/safe-proxy.test.ts b/src/fetch/safe-proxy.test.ts index 3d58353e..ca72b4ce 100644 --- a/src/fetch/safe-proxy.test.ts +++ b/src/fetch/safe-proxy.test.ts @@ -1,5 +1,6 @@ +import * as net from 'node:net'; import { describe, expect, it } from 'vitest'; -import { isSafeAddress } from './safe-proxy.js'; +import { createSafeProxy, isSafeAddress } from './safe-proxy.js'; describe('isSafeAddress', () => { it.each(['127.0.0.1', '10.0.0.1', '172.16.0.1', '192.168.1.1', '169.254.169.254', '0.0.0.0', '::1', '::', 'fe80::1', '::ffff:127.0.0.1'])('rejects private address %s', address => { @@ -7,3 +8,28 @@ describe('isSafeAddress', () => { }); it('allows public IPv4 addresses', () => expect(isSafeAddress('93.184.216.34')).toBe(true)); }); + +describe('createSafeProxy close', () => { + it('does not wait for an idle CONNECT tunnel to drain', async () => { + // Stands in for the upstream host: accepts and then never says anything, + // exactly like the keep-alive tunnels impit leaves behind. + const upstream = net.createServer(() => {}); + await new Promise(done => upstream.listen(0, '127.0.0.1', () => done())); + const upstreamPort = (upstream.address() as net.AddressInfo).port; + const proxy = await createSafeProxy({ allowPrivate: true }); + const proxyPort = Number(new URL(proxy.url).port); + + const client = net.connect({ host: '127.0.0.1', port: proxyPort }); + client.on('error', () => {}); + await new Promise(done => { + client.once('data', () => done()); + client.write(`CONNECT 127.0.0.1:${upstreamPort} HTTP/1.1\r\nHost: 127.0.0.1\r\n\r\n`); + }); + + const started = Date.now(); + await proxy.close(); + expect(Date.now() - started).toBeLessThan(1000); + await new Promise(done => client.once('close', () => done())); + await new Promise(done => upstream.close(() => done())); + }); +}); diff --git a/src/fetch/safe-proxy.ts b/src/fetch/safe-proxy.ts index 302f4710..e107a65a 100644 --- a/src/fetch/safe-proxy.ts +++ b/src/fetch/safe-proxy.ts @@ -1,6 +1,7 @@ import { lookup as dnsLookup } from 'node:dns'; import * as http from 'node:http'; import * as net from 'node:net'; +import type { Duplex } from 'node:stream'; export interface SafeProxy { url: string; close(): Promise; } export interface SafeProxyOptions { allowPrivate?: boolean; lookup?: typeof dnsLookup; } @@ -38,6 +39,18 @@ async function resolve(host: string, lookup: typeof dnsLookup, allowPrivate: boo export async function createSafeProxy(options: SafeProxyOptions = {}): Promise { const lookup = options.lookup ?? dnsLookup; const allowPrivate = options.allowPrivate === true; + // Sockets opened through this proxy, including the upstream halves the HTTP + // server never learns about. `close()` destroys them: a keep-alive CONNECT + // tunnel otherwise keeps `server.close()` pending until the peer or the OS + // gives up, which turns a bounded fetch budget into a minutes-long hang. + const sockets = new Set(); + const track = (socket: T): T => { + sockets.add(socket); + socket.on('close', () => sockets.delete(socket)); + // A destroyed peer must not resurface as an unhandled 'error' event. + socket.on('error', () => socket.destroy()); + return socket; + }; const server = http.createServer(async (request, response) => { try { const target = new URL(request.url ?? ''); @@ -46,16 +59,19 @@ export async function createSafeProxy(options: SafeProxyOptions = {}): Promise response.destroy(error)); request.pipe(upstream); } catch (error) { response.writeHead(403).end(error instanceof Error ? error.message : 'Unsafe fetch destination'); } }); + server.on('connection', track); server.on('connect', async (request, client, head) => { + track(client); try { const [host, portText] = (request.url ?? '').replace(/^\[/, '').replace(']', '').split(':'); if (!host) throw new Error('Invalid CONNECT target'); const address = await resolve(host, lookup, allowPrivate); - const upstream = net.connect({ host: address, port: Number(portText) || 443 }); + const upstream = track(net.connect({ host: address, port: Number(portText) || 443 })); upstream.once('connect', () => { client.write('HTTP/1.1 200 Connection Established\r\n\r\n'); if (head.length) upstream.write(head); upstream.pipe(client); client.pipe(upstream); }); upstream.once('error', error => client.destroy(error)); } catch (error) { client.end(`HTTP/1.1 403 Forbidden\r\n\r\n${error instanceof Error ? error.message : ''}`); } @@ -63,5 +79,12 @@ export async function createSafeProxy(options: SafeProxyOptions = {}): Promise((resolveListen, reject) => { server.once('error', reject); server.listen(0, '127.0.0.1', () => resolveListen()); }); const address = server.address(); if (!address || typeof address === 'string') throw new Error('Safe proxy did not bind'); - return { url: `http://127.0.0.1:${address.port}`, close: () => new Promise((resolveClose, reject) => server.close(error => error ? reject(error) : resolveClose())) }; + return { + url: `http://127.0.0.1:${address.port}`, + close: () => new Promise((resolveClose, reject) => { + for (const socket of sockets) socket.destroy(); + sockets.clear(); + server.close(error => error ? reject(error) : resolveClose()); + }), + }; } From b8132d05309b3ef2dfc003fe50816e3ae6bca9b7 Mon Sep 17 00:00:00 2001 From: Ankit Ranjan Date: Wed, 12 Aug 2026 00:55:25 +0530 Subject: [PATCH 2/2] fix: close the safe proxy against late DNS, and normalize impit deadlines Mark the proxy closing before draining sockets: track() destroys anything presented afterwards, and both request paths re-check the flag after their awaited DNS lookup, so a resolution that lands during teardown can no longer dial upstream or leave an untracked handle behind. close() is now idempotent. Also treat any failure at or past the deadline as the structured TIMEOUT, since impit surfaces its own deadline as a generic Error; failures with budget left are passed through unchanged. Co-Authored-By: Claude Opus 5 --- src/fetch/client.test.ts | 17 ++++++++++++++++ src/fetch/client.ts | 9 +++++++-- src/fetch/safe-proxy.test.ts | 39 ++++++++++++++++++++++++++++++++++++ src/fetch/safe-proxy.ts | 16 +++++++++++++-- 4 files changed, 77 insertions(+), 4 deletions(-) diff --git a/src/fetch/client.test.ts b/src/fetch/client.test.ts index 38325eb0..ad6844f4 100644 --- a/src/fetch/client.test.ts +++ b/src/fetch/client.test.ts @@ -36,4 +36,21 @@ describe('webFetch', () => { createSafeProxy: async () => ({ url: 'http://proxy', close: async () => {} }), })).rejects.toMatchObject({ code: 'TIMEOUT', message: 'web fetch timed out after 5s' }); }); + it('reports an impit-shaped deadline as a structured timeout', async () => { + // impit reports its own deadline as a plain Error — no TimeoutError/AbortError + // name to match on — so the budget having elapsed is what identifies it. + const impitTimeout = new Error('error sending request for url (https://example.com/): operation timed out'); + await expect(webFetch({ url: 'https://example.com', timeoutSeconds: 0.05, maxChars: 0, allowPrivate: false }, { + plainFetch: vi.fn().mockImplementation(async () => { await new Promise(done => setTimeout(done, 80)); throw impitTimeout; }), + createImpit: vi.fn(), + createSafeProxy: async () => ({ url: 'http://proxy', close: async () => {} }), + })).rejects.toMatchObject({ code: 'TIMEOUT', message: 'web fetch timed out after 0.05s' }); + }); + it('does not relabel a failure that happened with budget left', async () => { + await expect(webFetch({ url: 'https://example.com', timeoutSeconds: 30, maxChars: 0, allowPrivate: false }, { + plainFetch: vi.fn().mockRejectedValue(Object.assign(new Error('connect ECONNREFUSED'), { code: 'ECONNREFUSED' })), + createImpit: vi.fn(), + createSafeProxy: async () => ({ url: 'http://proxy', close: async () => {} }), + })).rejects.toThrow('connect ECONNREFUSED'); + }); }); diff --git a/src/fetch/client.ts b/src/fetch/client.ts index d06deffb..07d701af 100644 --- a/src/fetch/client.ts +++ b/src/fetch/client.ts @@ -67,14 +67,19 @@ export async function webFetch(options: WebFetchOptions, dependencies: WebFetchD const clipped = truncate(extracted.content, options.maxChars); return { status: response.status, requestedUrl: options.url, finalUrl: response.url || options.url, contentType: response.headers.get('content-type') ?? '', tier, ...(profile && { profile }), title: extracted.title, extractionSource: extracted.source, truncated: clipped.truncated, content: clipped.content }; } catch (error) { - throw asFetchError(error, options.timeoutSeconds); + throw asFetchError(error, options.timeoutSeconds, deadline); } finally { await proxy.close(); } } /** An aborted fetch surfaces as a DOMException; agents need the structured timeout instead. */ -function asFetchError(error: unknown, timeoutSeconds: number): unknown { +function asFetchError(error: unknown, timeoutSeconds: number, deadline: number): unknown { if (error instanceof CliError) return error; const name = (error as { name?: string } | null)?.name; if (name === 'TimeoutError' || name === 'AbortError') return new TimeoutError('web fetch', timeoutSeconds); + // Impit reports its own deadline as a plain Error with a message we do not control, so the name + // check misses it. Anything that fails at or past the budget is a timeout whatever it calls + // itself; a failure with time left is a real error and is passed through untouched, so a refused + // connection or DNS failure is never mislabelled. + if (Date.now() >= deadline) return new TimeoutError('web fetch', timeoutSeconds); return error; } diff --git a/src/fetch/safe-proxy.test.ts b/src/fetch/safe-proxy.test.ts index ca72b4ce..5b8e51db 100644 --- a/src/fetch/safe-proxy.test.ts +++ b/src/fetch/safe-proxy.test.ts @@ -32,4 +32,43 @@ describe('createSafeProxy close', () => { await new Promise(done => client.once('close', () => done())); await new Promise(done => upstream.close(() => done())); }); + + it('does not open an upstream tunnel from a DNS lookup that finishes after close()', async () => { + // Counts every accepted connection: the proxy must not dial us at all once + // close() has begun, however late the lookup that was already in flight. + let accepted = 0; + const upstream = net.createServer(socket => { accepted += 1; socket.destroy(); }); + await new Promise(done => upstream.listen(0, '127.0.0.1', () => done())); + const upstreamPort = (upstream.address() as net.AddressInfo).port; + + // A lookup that only resolves when we say so, so the CONNECT handler is + // parked mid-await exactly when close() starts. + let releaseLookup: (() => void) | undefined; + let onLookup: () => void; + const lookupCalled = new Promise(done => { onLookup = done; }); + const proxy = await createSafeProxy({ + allowPrivate: true, + lookup: ((_host: string, _options: unknown, callback: (error: Error | null, result: unknown) => void) => { + releaseLookup = () => callback(null, [{ address: '127.0.0.1', family: 4 }]); + onLookup(); + }) as never, + }); + + const client = net.connect({ host: '127.0.0.1', port: Number(new URL(proxy.url).port) }); + client.on('error', () => {}); + client.write(`CONNECT example.test:${upstreamPort} HTTP/1.1\r\nHost: example.test\r\n\r\n`); + await lookupCalled; + + const started = Date.now(); + const closePromise = proxy.close(); + releaseLookup?.(); + await closePromise; + await proxy.close(); // repeated close is safe + expect(Date.now() - started).toBeLessThan(1000); + await new Promise(done => setTimeout(done, 50)); + expect(accepted).toBe(0); + + client.destroy(); + await new Promise(done => upstream.close(() => done())); + }); }); diff --git a/src/fetch/safe-proxy.ts b/src/fetch/safe-proxy.ts index e107a65a..2d82527f 100644 --- a/src/fetch/safe-proxy.ts +++ b/src/fetch/safe-proxy.ts @@ -44,7 +44,13 @@ export async function createSafeProxy(options: SafeProxyOptions = {}): Promise(); + // Both request paths await DNS before connecting upstream, so a resolution that + // lands after `close()` could otherwise open an untracked socket — outbound + // traffic after the fetch budget expired, and one more handle holding the + // process open. Once closing, nothing new is tracked or dialled. + let closing = false; const track = (socket: T): T => { + if (closing) { socket.destroy(); return socket; } sockets.add(socket); socket.on('close', () => sockets.delete(socket)); // A destroyed peer must not resurface as an unhandled 'error' event. @@ -55,6 +61,7 @@ export async function createSafeProxy(options: SafeProxyOptions = {}): Promise { response.writeHead(upstreamResponse.statusCode ?? 502, upstreamResponse.headers); upstreamResponse.pipe(response); @@ -71,6 +78,7 @@ export async function createSafeProxy(options: SafeProxyOptions = {}): Promise { client.write('HTTP/1.1 200 Connection Established\r\n\r\n'); if (head.length) upstream.write(head); upstream.pipe(client); client.pipe(upstream); }); upstream.once('error', error => client.destroy(error)); @@ -79,12 +87,16 @@ export async function createSafeProxy(options: SafeProxyOptions = {}): Promise((resolveListen, reject) => { server.once('error', reject); server.listen(0, '127.0.0.1', () => resolveListen()); }); const address = server.address(); if (!address || typeof address === 'string') throw new Error('Safe proxy did not bind'); + let closed: Promise | undefined; return { url: `http://127.0.0.1:${address.port}`, - close: () => new Promise((resolveClose, reject) => { + // Idempotent: a second close() awaits the first rather than asking an + // already-stopped server to close again. + close: () => (closed ??= new Promise((resolveClose, reject) => { + closing = true; for (const socket of sockets) socket.destroy(); sockets.clear(); server.close(error => error ? reject(error) : resolveClose()); - }), + })), }; }