From b5496117e990969de76511ff2ce8d9de40a18003 Mon Sep 17 00:00:00 2001 From: Cornna <96944678+ymylive@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:44:23 -0700 Subject: [PATCH] Give the upgrade socket an error listener before it can be reset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The http server drops its own socket error handler before emitting 'upgrade', so handleUpgrade receives a bare socket — and then awaited resolveTarget, which walks locks.run into wakeSandbox and can sit there for seconds on a cold wake, before attaching any listener of its own. A TCP reset inside that window is a listener-less 'error' event, which throws; nothing in the server installs an uncaughtException handler, so the daemon exits and every in-flight exec, restore and archive dies with it. handleRequest never had the problem: its ServerResponse keeps the server's own cleanup. Only a reset does this — a graceful FIN does not — and an HTTP reverse proxy absorbs a client's reset rather than passing it upstream, so with the daemon bound to loopback this is a local robustness defect, not a remotely reachable one. The realistic trigger is a local client aborting during a wake. The listener moves to the first statement, before any await. Its 'error' twin further down goes away rather than being kept in parallel: the new listener destroys the socket, and a destroyed socket always reaches 'close', which already runs the same settle() and upstream.destroy(). Verifying the fix against a real daemon turned up the same bug a second time, in the fake: its echo upstream self-pipes the upgrade socket with no error listener, so tearing the connection down after a client reset re-emits 'error' on a pipe destination with zero listeners. The real executor's upstream is a separate process and cannot take the daemon down that way, which made the stand-in deadlier than the thing it stands in for. Both tests are red without their own fix: one resets while the wake is held in the key slot, the other after the handshake, staggered, because a single well-timed close does not reproduce it. --- packages/server/src/executor/fake.ts | 6 ++ packages/server/src/sandbox-proxy.test.ts | 105 ++++++++++++++++++++++ packages/server/src/sandbox-proxy.ts | 12 ++- 3 files changed, 119 insertions(+), 4 deletions(-) diff --git a/packages/server/src/executor/fake.ts b/packages/server/src/executor/fake.ts index 1ba6f3d..0f7314f 100644 --- a/packages/server/src/executor/fake.ts +++ b/packages/server/src/executor/fake.ts @@ -442,6 +442,12 @@ export class FakeExecutor implements Executor { ); }); upstream.on('upgrade', (_req, socket) => { + // The echo pipe's destination is this socket, so a reset reaching + // it would re-emit 'error' with no listener and take the daemon + // down — the real executor's upstream is another process and + // cannot do that, so without this the fake is deadlier than what + // it stands in for. + socket.on('error', () => socket.destroy()); socket.write( 'HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket\r\nConnection: Upgrade\r\n\r\n', ); diff --git a/packages/server/src/sandbox-proxy.test.ts b/packages/server/src/sandbox-proxy.test.ts index 9013730..4fabc9f 100644 --- a/packages/server/src/sandbox-proxy.test.ts +++ b/packages/server/src/sandbox-proxy.test.ts @@ -260,6 +260,111 @@ describe('sandbox port proxy', () => { expect(JSON.parse(other.body)).toMatchObject({ sandboxId }); }); + it('survives a client reset while the upgrade is still waking the sandbox', async () => { + const t = await listeningApp(); + const sandboxId = await createSandbox(t.port); + const name = findById(t.db, sandboxId)?.name; + if (name === undefined) throw new Error('no sandbox row'); + + // Hold the sandbox's lock slot so resolveTarget's own locks.run queues + // behind it: the seconds-long window a cold wake really has, made + // deterministic. A reset — not a graceful close — is what raises + // 'error'; with no listener yet attached it throws out of the + // EventEmitter, and the daemon has no uncaughtException handler. + let release!: () => void; + const occupied = new Promise((resolve) => { + release = resolve; + }); + const slot = t.locks.run(name, () => occupied); + + const crashes: unknown[] = []; + const onUncaught = (err: unknown) => crashes.push(err); + process.on('uncaughtException', onUncaught); + try { + const socket = net.connect(t.port, '127.0.0.1', () => { + socket.write( + [ + 'GET /ws HTTP/1.1', + `Host: 5173-${sandboxId}.${DOMAIN}`, + 'Connection: Upgrade', + 'Upgrade: websocket', + '', + '', + ].join('\r\n'), + ); + }); + socket.on('error', () => { + // The client end of the reset we are about to send. + }); + // Let the request arrive and park inside handleUpgrade, then reset. + await new Promise((r) => setTimeout(r, 100)); + socket.resetAndDestroy(); + await new Promise((r) => setTimeout(r, 100)); + release(); + await slot; + await new Promise((r) => setTimeout(r, 100)); + expect(crashes).toEqual([]); + } finally { + process.off('uncaughtException', onUncaught); + release(); + } + + // Still serving: the reset took its own connection down, nothing else. + const after = await rawGet(t.port, '/healthz', 'localhost'); + expect(after.status).toBe(200); + }); + + it('survives a client reset after the upgrade is established', async () => { + const t = await listeningApp(); + const sandboxId = await createSandbox(t.port); + + // The other half of the window: once the handshake lands, both the + // proxy's socket and the upstream's are piped. A reset here reaches + // the upstream end too — for the fake that end lives in this process, + // so it must be as unkillable as a real container's would be. + // Staggered across the handshake rather than one clean case: the reset + // has to land while bytes are still moving for the upstream end to see + // it as an error, and a single well-timed close does not reproduce it. + const crashes: unknown[] = []; + const onUncaught = (err: unknown) => crashes.push(err); + process.on('uncaughtException', onUncaught); + try { + await Promise.all( + Array.from( + { length: 60 }, + (_, i) => + new Promise((resolve) => { + const socket = net.connect(t.port, '127.0.0.1', () => { + socket.write( + [ + 'GET /ws HTTP/1.1', + `Host: 5173-${sandboxId}.${DOMAIN}`, + 'Connection: Upgrade', + 'Upgrade: websocket', + '', + '', + ].join('\r\n'), + ); + setTimeout(() => { + socket.resetAndDestroy(); + resolve(); + }, i % 10); + }); + socket.on('error', () => resolve()); + setTimeout(resolve, 3000); + }), + ), + ); + await new Promise((r) => setTimeout(r, 300)); + expect(crashes).toEqual([]); + } finally { + process.off('uncaughtException', onUncaught); + } + + const after = await rawGet(t.port, '/healthz', 'localhost'); + expect(after.status).toBe(200); + }); + it('passes WebSocket upgrades through, both directions', async () => { const t = await listeningApp(); const sandboxId = await createSandbox(t.port); diff --git a/packages/server/src/sandbox-proxy.ts b/packages/server/src/sandbox-proxy.ts index 4d1167d..7bca1a4 100644 --- a/packages/server/src/sandbox-proxy.ts +++ b/packages/server/src/sandbox-proxy.ts @@ -209,6 +209,12 @@ export function createSandboxProxy(deps: SandboxProxyDeps): SandboxProxy { }, handleUpgrade(req, socket, head) { + // The http server drops its own socket error handler before emitting + // 'upgrade', so until this line the socket is one reset away from + // throwing a listener-less 'error' and taking the daemon with it — + // and resolveTarget below can wait seconds on a cold wake. This is + // what handleRequest gets for free from its ServerResponse. + socket.on('error', () => socket.destroy()); void (async () => { let resolved: Awaited>; try { @@ -256,10 +262,8 @@ export function createSandboxProxy(deps: SandboxProxyDeps): SandboxProxy { settle(); socket.destroy(); }); - socket.on('error', () => { - settle(); - upstream.destroy(); - }); + // No 'error' twin here: the listener above already destroys the + // socket, and a destroyed socket always reaches 'close'. socket.on('close', () => { settle(); upstream.destroy();