mirror of https://github.com/garrytan/gstack.git
fix(bun-polyfill): Bun.spawn parity for proc.exited and stream consumption
The Bun.spawn polyfill omitted `proc.exited`, so eight call sites in
browse/src/ that `await proc.exited` (DPAPI decryption, isBrowserRunning,
browser-skill-commands, cli, terminal-agent) silently no-op on Windows —
the await resolves to `undefined` immediately, the caller reads stdout
before the child has produced anything, and the operation looks like a
silent failure. Most visible symptom: cookie picker's DPAPI helper
reports `DPAPI decryption failed:` with empty stderr.
Adding `proc.exited` exposes the second-order issue: every consumer that
awaits exited *before* reading stdout hits `Response body disturbed or
locked`, because Node's auto-flowing Readable has already drained by the
time the consumer wraps it. `Readable.toWeb()` (Node 18+) hands the
consumer a buffered Web ReadableStream that behaves like Bun's native
API regardless of read order. Falls back to the raw Node Readable on
Node < 18 so the polyfill stays backward-compatible.
The test file's existing `require('${polyfillPath}')` interpolation
escaped its backslashes on Windows, so the tests had been silently
unrunnable there; a `.replace(/\/g, '/')` on the resolved path fixes
the interpolation and lets the new regressions run on the
windows-free-tests matrix.
Test plan
- bun test browse/test/bun-polyfill.test.ts — 7 pass (3 new + 4 existing)
- bun test browse/test/cookie-import-browser.test.ts — 22 pass
- bun test (full suite) — passes; the unrelated batch.test.ts
beforeEach-timeout flake is the only fail and is present on main.
Related: #764 (Windows cookie import), PR #392 (open WIP for full DPAPI
+ v20 fallback support — this PR is strictly scoped to the polyfill so
it can land independently of that broader effort).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
a6fb31726c
commit
caf8b42a7f
|
|
@ -12,6 +12,7 @@
|
|||
|
||||
const http = require('http');
|
||||
const { spawnSync, spawn } = require('child_process');
|
||||
const { Readable } = require('stream');
|
||||
|
||||
globalThis.Bun = {
|
||||
serve(options) {
|
||||
|
|
@ -93,11 +94,36 @@ globalThis.Bun = {
|
|||
cwd: options.cwd,
|
||||
});
|
||||
|
||||
// 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.
|
||||
const exited = new Promise((resolveExited) => {
|
||||
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);
|
||||
});
|
||||
proc.once('error', () => resolveExited(1));
|
||||
});
|
||||
|
||||
// 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);
|
||||
|
||||
return {
|
||||
pid: proc.pid,
|
||||
stdout: proc.stdout,
|
||||
stderr: proc.stderr,
|
||||
stdout: toWeb(proc.stdout),
|
||||
stderr: toWeb(proc.stderr),
|
||||
stdin: proc.stdin,
|
||||
exited,
|
||||
unref() { proc.unref(); },
|
||||
kill(signal) { proc.kill(signal); },
|
||||
};
|
||||
|
|
|
|||
|
|
@ -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,52 @@ 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');
|
||||
});
|
||||
|
||||
test('Bun.serve creates an HTTP server that responds', async () => {
|
||||
const result = Bun.spawnSync(['node', '-e', `
|
||||
require('${polyfillPath}');
|
||||
|
|
|
|||
Loading…
Reference in New Issue