mirror of https://github.com/garrytan/gstack.git
fix(windows): give the bun-polyfill spawn shim a real `exited` promise
Bun.spawn exposes `proc.exited` as a Promise resolving to the exit code. The Node fallback shim (dist/bun-polyfill.cjs) returned no such field, so every `await proc.exited` on the Windows path resolved instantly to undefined — the Windows cookie picker (cookie-import-browser.ts races proc.exited at three sites) read stdout before the child produced it and silent-failed; browser-skill-commands and terminal-agent hit the same class. The shim now: - drains stdout/stderr eagerly into capped in-memory buffers (Node's Readables are pull-based; without draining, a child writing past the OS pipe buffer blocks in write() and 'exit' never fires), replaying them as fresh single-shot Web ReadableStreams so reads work before or after awaiting exit; - caps the buffer at 16 MB (GSTACK_SPAWN_MAX_BUFFER to override), still draining past the cap so a runaway child can't wedge or OOM; - resolves `exited` with Bun-matching codes (exit code, 128+signal, 1 on spawn error) after both pipes finish, and resolves on 'error' too — Node fires 'error' without 'exit' when the binary is missing, which otherwise hangs the await forever. Six tests pin exit codes, the read-after-exit ordering, spawn-failure resolution, the buffer cap, and the large-output drain. Adapted to the current test file (require path goes through the requirePath variable from the windowsHide commit), and the 1 MB drain test's child now exits in the write callback — on modern Node a pipe write past the OS buffer is async and process.exit() straight after write() truncates at ~64 KB even with a live reader, which fails the test for reasons unrelated to the shim. Contributed by @punksterlabs (PR #1743). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
5f6266e012
commit
39ed173fdc
|
|
@ -102,11 +102,103 @@ globalThis.Bun = {
|
||||||
windowsHide: options.windowsHide !== false,
|
windowsHide: options.windowsHide !== false,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// 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 {
|
return {
|
||||||
pid: proc.pid,
|
pid: proc.pid,
|
||||||
stdout: proc.stdout,
|
stdout: replay(stdoutDrain),
|
||||||
stderr: proc.stderr,
|
stderr: replay(stderrDrain),
|
||||||
stdin: proc.stdin,
|
stdin: proc.stdin,
|
||||||
|
exited,
|
||||||
unref() { proc.unref(); },
|
unref() { proc.unref(); },
|
||||||
kill(signal) { proc.kill(signal); },
|
kill(signal) { proc.kill(signal); },
|
||||||
};
|
};
|
||||||
|
|
|
||||||
|
|
@ -51,6 +51,133 @@ describe('bun-polyfill', () => {
|
||||||
expect(lines[2]).toBe('HAS_UNREF');
|
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('${requirePath}');
|
||||||
|
(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('${requirePath}');
|
||||||
|
(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('${requirePath}');
|
||||||
|
(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('${requirePath}');
|
||||||
|
(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('${requirePath}');
|
||||||
|
(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('${requirePath}');
|
||||||
|
(async () => {
|
||||||
|
const ONE_MB = 1024 * 1024;
|
||||||
|
// Exit in the write callback, not straight after write(): on modern
|
||||||
|
// Node a pipe write past the OS buffer is async, and process.exit()
|
||||||
|
// right after write() truncates at ~64 KB even with a live reader.
|
||||||
|
// The callback only fires once the full MB is flushed — which still
|
||||||
|
// requires the parent to drain, so the regression (no eager drain →
|
||||||
|
// child blocks → timeout) is still caught.
|
||||||
|
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);
|
||||||
|
|
||||||
// windowsHide is the one option where Node's default is the opposite of
|
// windowsHide is the one option where Node's default is the opposite of
|
||||||
// Bun's: Node shows the child's console window, Bun hides it. Dropping it
|
// Bun's: Node shows the child's console window, Bun hides it. Dropping it
|
||||||
// in translation makes every spawned child pop a window on Windows, which
|
// in translation makes every spawned child pop a window on Windows, which
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue