From 33143ad9239b78c1b79a2bf235ac202ecfe2e61b Mon Sep 17 00:00:00 2001 From: Salim Habash Date: Wed, 27 May 2026 00:46:17 -0700 Subject: [PATCH] fix(bun-polyfill): cap spawned-pipe buffer to prevent OOM Codex review pass 3 flagged a [P2]: the unbounded eager-drain buffer defeats the consumer's existing readCapped at the call site. A runaway child that prints hundreds of MB before timing out now balloons server memory regardless of the consumer's documented 1 MB cap. Cap the per-pipe drain at 16 MB by default (GSTACK_SPAWN_MAX_BUFFER overrides), generous for every real use case (DPAPI ~32B, tasklist <1KB, codex review ~100KB) and small enough to bound worst-case memory. Past the cap, the pipe keeps flowing so the child never blocks in write(), but further bytes are discarded. Consumers reading the replay stream see exactly MAX_BUFFER bytes; the existing readCapped at browser-skill- commands then applies its own 1 MB cap on top with no surprises. Regression test sets GSTACK_SPAWN_MAX_BUFFER=1024, has the child write 10 KB, asserts captured stdout is exactly 1024 bytes and exit resolves cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) --- browse/src/bun-polyfill.cjs | 29 +++++++++++++++++++++++++---- browse/test/bun-polyfill.test.ts | 26 ++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/browse/src/bun-polyfill.cjs b/browse/src/bun-polyfill.cjs index bc7236e52..897ce0915 100644 --- a/browse/src/bun-polyfill.cjs +++ b/browse/src/bun-polyfill.cjs @@ -99,11 +99,32 @@ globalThis.Bun = { // (~16-64 KB) back-pressures the child until it blocks in write() and // `exit` never fires. Eager draining keeps the pipes flowing regardless // of read order; replay below is via fresh Web ReadableStreams. + // + // Cap the buffer so a runaway child can't OOM the server. 16 MB is + // generous: DPAPI outputs are tiny, tasklist is <1 KB, and the + // browser-skill consumer has its own 1 MB readCapped. Once the cap is + // reached we keep draining the pipe (so the child never blocks) but + // discard further bytes. Override via GSTACK_SPAWN_MAX_BUFFER (bytes). + const MAX_BUFFER = Math.max( + 0, + parseInt(process.env.GSTACK_SPAWN_MAX_BUFFER || '', 10) || 16 * 1024 * 1024, + ); const drain = (stream) => { - if (!stream) return { done: Promise.resolve(), chunks: [] }; - const chunks = []; + if (!stream) return { done: Promise.resolve(), chunks: [], truncated: false }; + const state = { chunks: [], bytes: 0, truncated: false }; const done = new Promise((resolve) => { - stream.on('data', (chunk) => chunks.push(chunk)); + stream.on('data', (chunk) => { + if (state.bytes >= MAX_BUFFER) { state.truncated = true; return; } + if (state.bytes + chunk.length <= MAX_BUFFER) { + state.chunks.push(chunk); + state.bytes += chunk.length; + } else { + const remaining = MAX_BUFFER - state.bytes; + state.chunks.push(chunk.subarray(0, remaining)); + state.bytes = MAX_BUFFER; + state.truncated = true; + } + }); // Any terminal event resolves: 'end' on normal close, 'error' on a // stream-level error, 'close' as the belt-and-suspenders for spawn // failures where Node fires 'close' but neither 'end' nor 'error'. @@ -111,7 +132,7 @@ globalThis.Bun = { stream.once('error', resolve); stream.once('close', resolve); }); - return { done, chunks }; + return { done, chunks: state.chunks }; }; const stdoutDrain = drain(proc.stdout); const stderrDrain = drain(proc.stderr); diff --git a/browse/test/bun-polyfill.test.ts b/browse/test/bun-polyfill.test.ts index 6b7ae78a9..efca8f39b 100644 --- a/browse/test/bun-polyfill.test.ts +++ b/browse/test/bun-polyfill.test.ts @@ -121,6 +121,32 @@ describe('bun-polyfill', () => { expect(out).toMatch(/^exit:\d+$/); }); + // GSTACK_SPAWN_MAX_BUFFER caps the drain so a runaway child can't OOM the + // server. Past the cap, the pipe keeps flowing (child doesn't block) but + // further bytes are dropped. Set a small cap, write more than that, assert + // the captured stdout equals the cap and the child exits cleanly. + test('Bun.spawn caps buffered output at GSTACK_SPAWN_MAX_BUFFER', async () => { + const result = Bun.spawnSync(['node', '-e', ` + process.env.GSTACK_SPAWN_MAX_BUFFER = '${1024}'; + require('${polyfillPath}'); + (async () => { + // Child writes 10 KB; cap is 1 KB; drained output should be exactly 1 KB + // and exit should still resolve cleanly (child not back-pressured to death). + const p = Bun.spawn( + ['node', '-e', 'process.stdout.write("y".repeat(10 * 1024)); process.exit(0)'], + { stdio: ['ignore', 'pipe', 'ignore'] } + ); + const code = await Promise.race([ + p.exited, + new Promise((_, r) => setTimeout(() => r(new Error('timeout')), 3000)) + ]).catch(() => 'TIMEOUT'); + const out = await new Response(p.stdout).text(); + console.log(out.length + ':' + code); + })(); + `], { stdout: 'pipe', stderr: 'pipe' }); + expect(result.stdout.toString().trim()).toBe('1024:0'); + }); + // Regression for the pipe-blocking case: if the child writes more than the // OS pipe buffer (~16-64 KB) and the polyfill doesn't drain eagerly, the // child blocks in write() and `exit` never fires. 1 MB is well past every