From caf8b42a7f5bf95d3ab5c242f2c39e0f2be76ab4 Mon Sep 17 00:00:00 2001 From: Salim Habash Date: Wed, 27 May 2026 00:22:56 -0700 Subject: [PATCH] fix(bun-polyfill): Bun.spawn parity for proc.exited and stream consumption MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- browse/src/bun-polyfill.cjs | 30 ++++++++++++++++-- browse/test/bun-polyfill.test.ts | 53 ++++++++++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 4 deletions(-) diff --git a/browse/src/bun-polyfill.cjs b/browse/src/bun-polyfill.cjs index e0ada11b3..ce95b6dce 100644 --- a/browse/src/bun-polyfill.cjs +++ b/browse/src/bun-polyfill.cjs @@ -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); }, }; diff --git a/browse/test/bun-polyfill.test.ts b/browse/test/bun-polyfill.test.ts index 7ca25dfab..57b2981cb 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,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}');