mirror of https://github.com/garrytan/gstack.git
fix(bun-polyfill): drain spawned pipes eagerly to handle large output
Codex review on the prior commit flagged the pull-based Readable.toWeb approach: when the child writes more than the OS pipe buffer (~16-64 KB depending on platform) and the consumer awaits proc.exited before reading stdout, the buffer fills, the child blocks in write(), and exit never fires. browser-skill-commands.ts hits exactly this pattern when a spawned skill produces non-trivial output. Replace toWeb with eager-drain-then-replay: stdout and stderr are read into in-memory chunk arrays via 'data' events the moment the child starts, so pipes never back-pressure. proc.exited now resolves only after both exit and the drain have completed, so consumers reading stdout AFTER awaiting exit always see the full output. Consumers reading BEFORE awaiting exit still get the full output too — the replayed ReadableStream's start() awaits the drain before enqueueing. Adds a regression test that writes 1 MB to stdout — pre-fix this hangs forever, post-fix it returns in <500 ms. Removes the now-unused stream.Readable require. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
caf8b42a7f
commit
1d77c85b07
|
|
@ -12,7 +12,6 @@
|
|||
|
||||
const http = require('http');
|
||||
const { spawnSync, spawn } = require('child_process');
|
||||
const { Readable } = require('stream');
|
||||
|
||||
globalThis.Bun = {
|
||||
serve(options) {
|
||||
|
|
@ -94,34 +93,70 @@ 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.
|
||||
const drain = (stream) => {
|
||||
if (!stream) return { done: Promise.resolve(), chunks: [] };
|
||||
const chunks = [];
|
||||
const done = new Promise((resolve) => {
|
||||
stream.on('data', (chunk) => chunks.push(chunk));
|
||||
stream.once('end', resolve);
|
||||
stream.once('error', resolve); // exit event carries the real status
|
||||
});
|
||||
return { done, 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) resolveExited(code);
|
||||
else if (signal) resolveExited(128 + (require('os').constants.signals[signal] || 0));
|
||||
else resolveExited(0);
|
||||
if (code !== null) exitStatus = code;
|
||||
else if (signal) exitStatus = 128 + (require('os').constants.signals[signal] || 0);
|
||||
else exitStatus = 0;
|
||||
});
|
||||
proc.once('error', () => resolveExited(1));
|
||||
proc.once('error', () => {
|
||||
if (exitStatus === undefined) exitStatus = 1;
|
||||
});
|
||||
Promise.all([
|
||||
new Promise((r) => proc.once('exit', r)),
|
||||
stdoutDrain.done,
|
||||
stderrDrain.done,
|
||||
]).then(() => resolveExited(exitStatus !== undefined ? exitStatus : 0));
|
||||
});
|
||||
|
||||
// Bun gives consumers a Web ReadableStream so `new Response(proc.stdout)`
|
||||
// works regardless of read order. With Node's Readable, the stream auto-
|
||||
// drains once the child exits, so `await proc.exited` followed by
|
||||
// `new Response(proc.stdout).text()` throws "body disturbed or locked".
|
||||
// Readable.toWeb hands the consumer a fresh ReadableStream that buffers
|
||||
// until it's read. Falls back to the raw Node stream on Node < 18.
|
||||
const toWeb = (s) => (s && typeof Readable.toWeb === 'function' ? Readable.toWeb(s) : s);
|
||||
// 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: toWeb(proc.stdout),
|
||||
stderr: toWeb(proc.stderr),
|
||||
stdout: replay(stdoutDrain),
|
||||
stderr: replay(stderrDrain),
|
||||
stdin: proc.stdin,
|
||||
exited,
|
||||
unref() { proc.unref(); },
|
||||
|
|
|
|||
|
|
@ -97,6 +97,31 @@ describe('bun-polyfill', () => {
|
|||
expect(result.stdout.toString().trim()).toBe('ready: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}');
|
||||
|
|
|
|||
Loading…
Reference in New Issue