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/);