From d2be57a8f738bcfbb2d59c3d3fc5ddb0f3437d64 Mon Sep 17 00:00:00 2001 From: roboomp Date: Thu, 2 Jul 2026 08:33:32 +0000 Subject: [PATCH] fix(plugins): drain subprocess pipes concurrently with proc.exited MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PluginManager.install (bun install + bun update), PluginManager.uninstall, PluginManager.#fixMissingPlugin, the legacy installer.ts install/uninstall helpers, and generate-legacy-pi-bundled-registry.ts's formatInPlace all called Bun.spawn with stdout/stderr piped and awaited proc.exited before touching either stream. Once a child's output exceeded the ~64 KiB OS pipe buffer, the child would block on write(2) while the parent blocked on exit — a classic pipe-buffer deadlock. Even where Bun's current runtime happens to buffer eagerly, the pattern silently leaked unbounded bytes. Each site now starts new Response(proc.stdout).text() and stderr readers immediately after Bun.spawn and awaits them alongside proc.exited via Promise.all. Existing error semantics are preserved: install throws with stderr, uninstall keeps its generic error, and formatInPlace still includes Biome's stderr in the failure message. Adds a regression test (plugin-install-git.test.ts) that models the OS-pipe deadlock by holding proc.exited until both mock streams are drained — install must read them before awaiting exit, else the test hits its 2s Promise.race timeout. Fixes #4230 --- packages/coding-agent/CHANGELOG.md | 4 ++ .../generate-legacy-pi-bundled-registry.ts | 10 +++- .../src/extensibility/plugins/installer.ts | 15 ++++- .../src/extensibility/plugins/manager.ts | 40 ++++++++++--- .../test/plugin-install-git.test.ts | 58 +++++++++++++++++++ 5 files changed, 114 insertions(+), 13 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 64c3c57ae..3c8dda0c0 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed a pipe-buffer deadlock hazard in plugin install/uninstall paths: `PluginManager.install`, `PluginManager.uninstall`, `PluginManager.#fixMissingPlugin`, the git-refresh `bun update` step, the legacy `installer.ts` install/uninstall helpers, and the bundled-registry generator's `formatInPlace` all awaited `proc.exited` before draining stdout/stderr. Verbose `bun install`/`bun uninstall`/`biome check` output above the ~64 KiB OS pipe buffer could block the child on `write(2)` while the parent blocked on exit, and even under Bun's current eager buffering this leaked unbounded bytes into memory. Each site now drains both pipes concurrently with `proc.exited` via `Promise.all`. ([#4230](https://github.com/can1357/oh-my-pi/issues/4230)) + ## [16.3.1] - 2026-07-02 ### Breaking Changes diff --git a/packages/coding-agent/scripts/generate-legacy-pi-bundled-registry.ts b/packages/coding-agent/scripts/generate-legacy-pi-bundled-registry.ts index c1f7f1a12..6e686cf38 100755 --- a/packages/coding-agent/scripts/generate-legacy-pi-bundled-registry.ts +++ b/packages/coding-agent/scripts/generate-legacy-pi-bundled-registry.ts @@ -332,9 +332,15 @@ async function formatInPlace(targets: readonly string[]): Promise { stdout: "pipe", stderr: "pipe", }); - const exit = await proc.exited; + // Drain both pipes concurrently with proc.exited to avoid a pipe-buffer + // deadlock — biome check can emit thousands of lines when it rewrites the + // generated registry, easily exceeding the ~64 KiB OS pipe buffer. + const [exit, , stderr] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]); if (exit !== 0) { - const stderr = await new Response(proc.stderr).text(); throw new Error(`biome check --write failed (exit ${exit}): ${stderr}`); } } diff --git a/packages/coding-agent/src/extensibility/plugins/installer.ts b/packages/coding-agent/src/extensibility/plugins/installer.ts index 66649a2ea..24a2e3968 100644 --- a/packages/coding-agent/src/extensibility/plugins/installer.ts +++ b/packages/coding-agent/src/extensibility/plugins/installer.ts @@ -53,9 +53,14 @@ export async function installPlugin(packageName: string): Promise { windowsHide: true, }); - const exitCode = await proc.exited; + const [exitCode] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]); if (exitCode !== 0) { throw new Error(`Failed to uninstall ${name}`); } diff --git a/packages/coding-agent/src/extensibility/plugins/manager.ts b/packages/coding-agent/src/extensibility/plugins/manager.ts index 274ddab23..f1e7801c3 100644 --- a/packages/coding-agent/src/extensibility/plugins/manager.ts +++ b/packages/coding-agent/src/extensibility/plugins/manager.ts @@ -458,10 +458,17 @@ export class PluginManager { stderr: "pipe", windowsHide: true, }); - const installExit = await installProc.exited; + // Drain stdout+stderr concurrently with proc.exited. Awaiting exited + // before reading either pipe risks a >64 KiB OS-pipe-buffer deadlock + // once bun install prints enough progress; even where Bun currently + // buffers eagerly, doing this leaks unbounded memory. + const [installExit, , installStderr] = await Promise.all([ + installProc.exited, + new Response(installProc.stdout).text(), + new Response(installProc.stderr).text(), + ]); if (installExit !== 0) { - const stderr = await new Response(installProc.stderr).text(); - throw new Error(`bun install failed: ${stderr}`); + throw new Error(`bun install failed: ${installStderr}`); } // Resolve actual package name. npm specs encode the name (strip version); // git specs do not, so diff plugins/package.json deps to find the new entry. @@ -508,10 +515,14 @@ export class PluginManager { stderr: "pipe", windowsHide: true, }); - const updateExit = await updateProc.exited; + // Same drain-concurrent-with-exit pattern as the bun install above. + const [updateExit, , updateStderr] = await Promise.all([ + updateProc.exited, + new Response(updateProc.stdout).text(), + new Response(updateProc.stderr).text(), + ]); if (updateExit !== 0) { - const stderr = await new Response(updateProc.stderr).text(); - throw new Error(`bun update ${actualName} failed: ${stderr}`); + throw new Error(`bun update ${actualName} failed: ${updateStderr}`); } } @@ -608,7 +619,13 @@ export class PluginManager { windowsHide: true, }); - const exitCode = await proc.exited; + // Drain both pipes concurrently with proc.exited to avoid a pipe-buffer + // deadlock if bun uninstall floods stdout/stderr. + const [exitCode] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]); if (exitCode !== 0) { throw new Error(`npm uninstall failed for ${name}`); } @@ -1007,7 +1024,14 @@ export class PluginManager { stderr: "pipe", windowsHide: true, }); - return (await proc.exited) === 0; + // Drain pipes concurrently with proc.exited; otherwise a chatty + // bun install can block on a full OS pipe buffer. + const [exit] = await Promise.all([ + proc.exited, + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]); + return exit === 0; } catch { return false; } diff --git a/packages/coding-agent/test/plugin-install-git.test.ts b/packages/coding-agent/test/plugin-install-git.test.ts index 3a5349d5e..347c23b3e 100644 --- a/packages/coding-agent/test/plugin-install-git.test.ts +++ b/packages/coding-agent/test/plugin-install-git.test.ts @@ -272,6 +272,64 @@ describe("PluginManager.install with git sources", () => { expect(spawnedCommands).toEqual([["bun", "install", "github:foo/bar"]]); }); + test("drains stdout/stderr concurrently with proc.exited (pipe-buffer deadlock, #4230)", async () => { + // Model the OS-pipe semantics that caused the deadlock: `exited` cannot + // resolve until both pipes have been read. If PluginManager.install + // awaits `exited` before starting to drain either stream, this test + // hangs — which we catch with Promise.race + a short timeout. + await Bun.write( + pluginsPkgJson, + JSON.stringify({ name: "omp-plugins", private: true, dependencies: {} }, null, 2), + ); + + const makeGatedStream = (payload: string): { stream: ReadableStream; drained: Promise } => { + const { promise: drained, resolve: onDrained } = Promise.withResolvers(); + const stream = new ReadableStream({ + pull(controller) { + controller.enqueue(new TextEncoder().encode(payload)); + controller.close(); + onDrained(); + }, + }); + return { stream, drained }; + }; + + vi.spyOn(Bun, "spawn").mockImplementation(((cmd: string[]) => { + expect(cmd).toEqual(["bun", "install", "github:foo/bar"]); + const { stream: stdout, drained: stdoutDrained } = makeGatedStream("progress\n"); + const { stream: stderr, drained: stderrDrained } = makeGatedStream(""); + const exited = Promise.all([stdoutDrained, stderrDrained]).then(async () => { + await Bun.write( + pluginsPkgJson, + JSON.stringify( + { + name: "omp-plugins", + private: true, + dependencies: { "real-name": "github:foo/bar" }, + }, + null, + 2, + ), + ); + const installedDir = path.join(pluginsNodeModules, "real-name"); + await fs.mkdir(installedDir, { recursive: true }); + await Bun.write( + path.join(installedDir, "package.json"), + JSON.stringify({ name: "real-name", version: "0.1.0" }, null, 2), + ); + return 0; + }); + return { pid: 1, stdout, stderr, exited } as Subprocess; + }) as typeof Bun.spawn); + + const mgr = new PluginManager(tmpRoot); + const installed = await Promise.race([ + mgr.install("github:foo/bar"), + new Promise((_, reject) => setTimeout(() => reject(new Error("install deadlocked")), 2000)), + ]); + expect(installed.name).toBe("real-name"); + }); + test("rejects git specs containing shell metacharacters", async () => { const mgr = new PluginManager(tmpRoot); await expect(mgr.install("github:foo/bar; rm -rf /")).rejects.toThrow(/Invalid characters in plugin source/);