mirror of https://github.com/garrytan/gstack.git
fix(bun-polyfill): resolve proc.exited on spawn failure too
Codex review pass 2 flagged a [P2]: Node emits 'error' but not 'exit' when the spawned binary is missing, so the lifecycle Promise that only listened to 'exit' hangs forever. `await proc.exited` waits until the caller's own timeout fires, which on a missing `bun`, `powershell`, or similar burns the full 10 s the DPAPI helper allows. Listen to both 'exit' and 'error' on the lifecycle Promise so spawn failures resolve immediately. Also add 'close' as a third resolve trigger on the pipe drains for the same reason — the stdio streams may fire 'close' without 'end' or 'error' when the process never started. Regression test asserts proc.exited resolves (not 'TIMEOUT') when the binary doesn't exist. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
1d77c85b07
commit
899b3be30f
|
|
@ -104,8 +104,12 @@ globalThis.Bun = {
|
||||||
const chunks = [];
|
const chunks = [];
|
||||||
const done = new Promise((resolve) => {
|
const done = new Promise((resolve) => {
|
||||||
stream.on('data', (chunk) => chunks.push(chunk));
|
stream.on('data', (chunk) => chunks.push(chunk));
|
||||||
|
// 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('end', resolve);
|
||||||
stream.once('error', resolve); // exit event carries the real status
|
stream.once('error', resolve);
|
||||||
|
stream.once('close', resolve);
|
||||||
});
|
});
|
||||||
return { done, chunks };
|
return { done, chunks };
|
||||||
};
|
};
|
||||||
|
|
@ -131,11 +135,17 @@ globalThis.Bun = {
|
||||||
proc.once('error', () => {
|
proc.once('error', () => {
|
||||||
if (exitStatus === undefined) exitStatus = 1;
|
if (exitStatus === undefined) exitStatus = 1;
|
||||||
});
|
});
|
||||||
Promise.all([
|
// Wait for either 'exit' (normal child lifecycle) or 'error' (spawn
|
||||||
new Promise((r) => proc.once('exit', r)),
|
// failure — Node fires error without exit when the binary is missing).
|
||||||
stdoutDrain.done,
|
// Either path resolves the lifecycle promise; without listening to both
|
||||||
stderrDrain.done,
|
// a spawn error hangs `await proc.exited` until the consumer's own
|
||||||
]).then(() => resolveExited(exitStatus !== undefined ? exitStatus : 0));
|
// 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
|
// Replay buffered output as a fresh Web ReadableStream. `start()` awaits
|
||||||
|
|
|
||||||
|
|
@ -97,6 +97,30 @@ describe('bun-polyfill', () => {
|
||||||
expect(result.stdout.toString().trim()).toBe('ready:0');
|
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+$/);
|
||||||
|
});
|
||||||
|
|
||||||
// Regression for the pipe-blocking case: if the child writes more than the
|
// 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
|
// 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
|
// child blocks in write() and `exit` never fires. 1 MB is well past every
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue