diff --git a/browse/src/bun-polyfill.cjs b/browse/src/bun-polyfill.cjs index e0ada11b3..897ce0915 100644 --- a/browse/src/bun-polyfill.cjs +++ b/browse/src/bun-polyfill.cjs @@ -93,11 +93,103 @@ globalThis.Bun = { cwd: options.cwd, }); + // Drain stdout/stderr eagerly into in-memory buffers. Bun's spawn buffers + // these for the consumer; Node's Readables are pull-based, so if the caller + // awaits `proc.exited` before reading, anything past the OS pipe buffer + // (~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: [], truncated: false }; + const state = { chunks: [], bytes: 0, truncated: false }; + const done = new Promise((resolve) => { + 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'. + stream.once('end', resolve); + stream.once('error', resolve); + stream.once('close', resolve); + }); + return { done, chunks: state.chunks }; + }; + const stdoutDrain = drain(proc.stdout); + const stderrDrain = drain(proc.stderr); + + // Bun's spawn exposes `proc.exited` as a Promise resolving to the exit + // code; several call sites — DPAPI decryption, isBrowserRunning, + // browser-skill-commands — `await proc.exited` directly or via + // Promise.race with a timeout. Without this, those awaits resolve to + // `undefined` immediately and the operation looks like a silent failure. + // Resolve only after both pipes have finished draining so consumers that + // read stdout AFTER awaiting exit see the full output, not a partial buffer. + const exited = new Promise((resolveExited) => { + let exitStatus; + proc.once('exit', (code, signal) => { + // Match Bun: exit code on normal exit; 128 + signal number on signal; + // 0 if neither was reported. + if (code !== null) exitStatus = code; + else if (signal) exitStatus = 128 + (require('os').constants.signals[signal] || 0); + else exitStatus = 0; + }); + proc.once('error', () => { + if (exitStatus === undefined) exitStatus = 1; + }); + // Wait for either 'exit' (normal child lifecycle) or 'error' (spawn + // failure — Node fires error without exit when the binary is missing). + // Either path resolves the lifecycle promise; without listening to both + // a spawn error hangs `await proc.exited` until the consumer's own + // timeout fires. + const lifecycle = new Promise((r) => { + proc.once('exit', r); + proc.once('error', r); + }); + Promise.all([lifecycle, stdoutDrain.done, stderrDrain.done]) + .then(() => resolveExited(exitStatus !== undefined ? exitStatus : 0)); + }); + + // Replay buffered output as a fresh Web ReadableStream. `start()` awaits + // the drain before enqueueing so `new Response(proc.stdout).text()` yields + // the complete output regardless of whether the consumer reads before or + // after awaiting `proc.exited`. Stream is single-shot (locked after one + // read), matching Bun's behavior. + const replay = (d) => new ReadableStream({ + async start(controller) { + await d.done; + for (const chunk of d.chunks) { + controller.enqueue(chunk instanceof Uint8Array ? chunk : new Uint8Array(chunk)); + } + controller.close(); + }, + }); + return { pid: proc.pid, - stdout: proc.stdout, - stderr: proc.stderr, + stdout: replay(stdoutDrain), + stderr: replay(stderrDrain), stdin: proc.stdin, + exited, unref() { proc.unref(); }, kill(signal) { proc.kill(signal); }, }; diff --git a/browse/test/bun-polyfill.test.ts b/browse/test/bun-polyfill.test.ts index 7ca25dfab..efca8f39b 100644 --- a/browse/test/bun-polyfill.test.ts +++ b/browse/test/bun-polyfill.test.ts @@ -1,8 +1,11 @@ import { describe, test, expect, afterAll } from 'bun:test'; import * as path from 'path'; -// Load the polyfill into a fresh object (don't clobber globalThis.Bun) -const polyfillPath = path.resolve(import.meta.dir, '../src/bun-polyfill.cjs'); +// Load the polyfill into a fresh object (don't clobber globalThis.Bun). +// Forward-slash on Windows so the path interpolates cleanly into the +// `require('${polyfillPath}')` template literals below — raw backslashes +// would be interpreted as JS escape sequences in the spawned Node script. +const polyfillPath = path.resolve(import.meta.dir, '../src/bun-polyfill.cjs').replace(/\\/g, '/'); describe('bun-polyfill', () => { // We test the polyfill by requiring it in a subprocess under Node.js @@ -48,6 +51,127 @@ describe('bun-polyfill', () => { expect(lines[2]).toBe('HAS_UNREF'); }); + // Bun.spawn parity: `proc.exited` is a Promise resolving to the exit code. + // The DPAPI helper and isBrowserRunning both `await proc.exited`; without + // it the awaits resolve immediately to `undefined` and the caller reads + // stdout before the child has produced it — surfacing as a silent failure. + test('Bun.spawn exposes proc.exited that resolves to the exit code', async () => { + const result = Bun.spawnSync(['node', '-e', ` + require('${polyfillPath}'); + (async () => { + const p = Bun.spawn(['node', '-e', 'process.exit(0)'], { stdio: ['ignore', 'ignore', 'ignore'] }); + console.log(typeof p.exited === 'object' && typeof p.exited.then === 'function' ? 'IS_PROMISE' : 'NOT_PROMISE'); + console.log('exit:' + await p.exited); + })(); + `], { stdout: 'pipe', stderr: 'pipe' }); + const lines = result.stdout.toString().trim().split('\n'); + expect(lines[0]).toBe('IS_PROMISE'); + expect(lines[1]).toBe('exit:0'); + }); + + test('Bun.spawn proc.exited reflects non-zero exit codes', async () => { + const result = Bun.spawnSync(['node', '-e', ` + require('${polyfillPath}'); + (async () => { + const p = Bun.spawn(['node', '-e', 'process.exit(3)'], { stdio: ['ignore', 'ignore', 'ignore'] }); + console.log('exit:' + await p.exited); + })(); + `], { stdout: 'pipe', stderr: 'pipe' }); + expect(result.stdout.toString().trim()).toBe('exit:3'); + }); + + test('Bun.spawn proc.exited resolves before reading stdout (no race)', async () => { + const result = Bun.spawnSync(['node', '-e', ` + require('${polyfillPath}'); + (async () => { + // Real-world pattern: write to stdout, then exit. Awaiting proc.exited + // before reading must guarantee the bytes are flushed. + const p = Bun.spawn(['node', '-e', 'process.stdout.write("ready"); process.exit(0)'], { + stdio: ['ignore', 'pipe', 'ignore'] + }); + const code = await p.exited; + const out = await new Response(p.stdout).text(); + console.log(out + ':' + code); + })(); + `], { stdout: 'pipe', stderr: 'pipe' }); + expect(result.stdout.toString().trim()).toBe('ready:0'); + }); + + // Spawn-failure case: Node emits 'error' but not 'exit' when the binary + // is missing, so listening only for 'exit' hangs `await proc.exited` + // forever. The lifecycle promise must resolve on either event. + test('Bun.spawn proc.exited resolves on spawn failure (missing binary)', async () => { + const result = Bun.spawnSync(['node', '-e', ` + require('${polyfillPath}'); + (async () => { + const p = Bun.spawn(['this-binary-does-not-exist-zzz-' + Date.now()], { + stdio: ['ignore', 'pipe', 'pipe'] + }); + const code = await Promise.race([ + p.exited, + new Promise((_, r) => setTimeout(() => r(new Error('timeout')), 3000)) + ]).catch(() => 'TIMEOUT'); + console.log('exit:' + code); + })(); + `], { stdout: 'pipe', stderr: 'pipe' }); + // Anything other than 'TIMEOUT' (and ideally a non-zero number) means the + // lifecycle promise resolved on the spawn error. + const out = result.stdout.toString().trim(); + expect(out).not.toBe('exit:TIMEOUT'); + 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 + // OS pipe buffer size. Pre-fix this test hangs forever; post-fix it returns + // in <500ms. Bun's default per-test timeout is 5s — generous here. + test('Bun.spawn drains large stdout so proc.exited still resolves', async () => { + const result = Bun.spawnSync(['node', '-e', ` + require('${polyfillPath}'); + (async () => { + const ONE_MB = 1024 * 1024; + const p = Bun.spawn( + ['node', '-e', 'process.stdout.write("x".repeat(' + ONE_MB + ')); process.exit(0)'], + { stdio: ['ignore', 'pipe', 'ignore'] } + ); + const code = await Promise.race([ + p.exited, + new Promise((_, r) => setTimeout(() => r(new Error('timeout')), 10000)) + ]).catch(e => 'TIMEOUT'); + const out = await new Response(p.stdout).text(); + console.log(out.length + ':' + code); + })().catch((e) => { console.log('THREW:' + e.message); }); + `], { stdout: 'pipe', stderr: 'pipe' }); + expect(result.stdout.toString().trim()).toBe('1048576:0'); + }, 15000); + test('Bun.serve creates an HTTP server that responds', async () => { const result = Bun.spawnSync(['node', '-e', ` require('${polyfillPath}');