From 91da44f866ff5211e2762af533b98d08be939bfd Mon Sep 17 00:00:00 2001 From: LukeParkerDev <10430890+Hona@users.noreply.github.com> Date: Fri, 29 May 2026 14:32:04 +1000 Subject: [PATCH 1/7] fix(core): resolve spawn completion on exit, not only close The shell tool hangs on Windows when a command spawns a detached child that inherits stdio (e.g. a backgrounded web server or playwright-cli). The cross-spawn spawner resolved process completion only on the child's `close` event, which fires after *all* stdio streams close. A detached child keeps the inherited pipe's write end open, so `close` is delayed indefinitely even though the main process already exited -- and exitCode (and thus the shell tool) never resolves. Resolve on whichever of `exit`/`close` fires first. `exit` returns promptly once the main process is gone (fixing the hang); `close` is kept as the required backstop for failed spawns, where cross-spawn emits spawn->error->close but never exit, so exit-only would hang that path forever. Closes #24731 --- packages/core/src/cross-spawn-spawner.ts | 20 +++++++ .../test/effect/cross-spawn-spawner.test.ts | 53 +++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/packages/core/src/cross-spawn-spawner.ts b/packages/core/src/cross-spawn-spawner.ts index ad8d4126d454..71dc81387362 100644 --- a/packages/core/src/cross-spawn-spawner.ts +++ b/packages/core/src/cross-spawn-spawner.ts @@ -271,8 +271,28 @@ export const make = Effect.gen(function* () { proc.on("error", (err) => { resume(Effect.fail(toPlatformError("spawn", err, command))) }) + // Complete on whichever of "exit"/"close" fires first; both are required. + // + // "exit" means the process itself terminated; "close" means it ended AND all of + // its stdio streams closed. They differ because multiple processes can share the + // same stdio streams. We must handle both because neither alone is sufficient: + // + // - A detached child that inherits this process's stdio keeps the pipe's write + // end open, so "close" can be delayed indefinitely even after the main process + // exits. Resolving on "exit" returns promptly instead of hanging (issue #24731). + // + // - For a failed spawn (e.g. ENOENT), cross-spawn on Windows emits "spawn" then + // "error" then "close" but NEVER "exit" — so exitCode would hang forever if we + // only listened for "exit". "close" is the backstop that completes that path. + // (Verified: raw child_process never reproduces this; it's specific to + // cross-spawn's ENOENT handling, independent of node vs bun.) + // + // Deferred.doneUnsafe is a no-op once the Deferred is already completed. proc.on("exit", (...args) => { exit = args + if (end) return + end = true + Deferred.doneUnsafe(signal, Exit.succeed(args)) }) proc.on("close", (...args) => { if (end) return diff --git a/packages/core/test/effect/cross-spawn-spawner.test.ts b/packages/core/test/effect/cross-spawn-spawner.test.ts index 8a2fab493063..0fd2c688b86b 100644 --- a/packages/core/test/effect/cross-spawn-spawner.test.ts +++ b/packages/core/test/effect/cross-spawn-spawner.test.ts @@ -286,6 +286,54 @@ describe("cross-spawn spawner", () => { ) }) + describe("detached children (issue #24731)", () => { + fx.live( + "exitCode resolves when the main process exits even if a detached child keeps stdio open", + Effect.gen(function* () { + const tmp = yield* Effect.acquireRelease( + Effect.promise(() => tmpdir()), + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ) + const pidFile = path.join(tmp.path, "daemon.pid") + + // Mimics playwright-cli / a backgrounded web server: spawn a long-lived + // detached child that inherits stdio (keeping the parent's stdout pipe's + // write end open), then exit the main process immediately. + const code = [ + 'const cp = require("node:child_process")', + 'const fs = require("node:fs")', + 'const daemon = cp.spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { detached: true, stdio: "inherit" })', + "daemon.unref()", + "fs.writeFileSync(process.argv[1], String(daemon.pid))", + 'process.stdout.write("started")', + "process.exit(0)", + ].join("\n") + + const handle = yield* ChildProcess.make(process.execPath, ["-e", code, pidFile]) + + const result = yield* Effect.raceAll([ + handle.exitCode.pipe(Effect.map((exit) => ({ kind: "exit" as const, code: exit }))), + Effect.sleep("5 seconds").pipe(Effect.as({ kind: "timeout" as const, code: null })), + ]) + + // Reap the detached daemon so it does not leak and so the spawner's + // "close" can fire during scope teardown. + const pid = Number(yield* Effect.promise(() => fs.readFile(pidFile, "utf-8").catch(() => "0"))) + if (pid) { + yield* Effect.sync(() => { + try { + process.kill(pid) + } catch {} + }) + } + + expect(result.kind).toBe("exit") + expect(result.code).toBe(ChildProcessSpawner.ExitCode(0)) + }), + 15_000, + ) + }) + describe("error handling", () => { fx.effect( "fails for invalid command", @@ -423,3 +471,8 @@ describe("cross-spawn spawner", () => { ) }) }) + + + + + From 714325aa15d52edb6802b5a2c58985fce2e7415e Mon Sep 17 00:00:00 2001 From: LukeParkerDev <10430890+Hona@users.noreply.github.com> Date: Fri, 29 May 2026 14:58:05 +1000 Subject: [PATCH 2/7] fix(core): preserve kill escalation after parent exit --- packages/core/src/cross-spawn-spawner.ts | 90 ++++++++----------- .../test/effect/cross-spawn-spawner.test.ts | 38 ++++++++ 2 files changed, 75 insertions(+), 53 deletions(-) diff --git a/packages/core/src/cross-spawn-spawner.ts b/packages/core/src/cross-spawn-spawner.ts index 71dc81387362..3b41edebfa88 100644 --- a/packages/core/src/cross-spawn-spawner.ts +++ b/packages/core/src/cross-spawn-spawner.ts @@ -263,49 +263,33 @@ export const make = Effect.gen(function* () { } const spawn = (command: ChildProcess.StandardCommand, opts: NodeChildProcess.SpawnOptions) => - Effect.callback((resume) => { - const signal = Deferred.makeUnsafe() - const proc = launch(command.command, command.args, opts) - let end = false - let exit: readonly [code: number | null, signal: NodeJS.Signals | null] | undefined - proc.on("error", (err) => { - resume(Effect.fail(toPlatformError("spawn", err, command))) - }) - // Complete on whichever of "exit"/"close" fires first; both are required. - // - // "exit" means the process itself terminated; "close" means it ended AND all of - // its stdio streams closed. They differ because multiple processes can share the - // same stdio streams. We must handle both because neither alone is sufficient: - // - // - A detached child that inherits this process's stdio keeps the pipe's write - // end open, so "close" can be delayed indefinitely even after the main process - // exits. Resolving on "exit" returns promptly instead of hanging (issue #24731). - // - // - For a failed spawn (e.g. ENOENT), cross-spawn on Windows emits "spawn" then - // "error" then "close" but NEVER "exit" — so exitCode would hang forever if we - // only listened for "exit". "close" is the backstop that completes that path. - // (Verified: raw child_process never reproduces this; it's specific to - // cross-spawn's ENOENT handling, independent of node vs bun.) - // - // Deferred.doneUnsafe is a no-op once the Deferred is already completed. - proc.on("exit", (...args) => { - exit = args - if (end) return - end = true - Deferred.doneUnsafe(signal, Exit.succeed(args)) - }) - proc.on("close", (...args) => { - if (end) return - end = true - Deferred.doneUnsafe(signal, Exit.succeed(exit ?? args)) - }) - proc.on("spawn", () => { - resume(Effect.succeed([proc, signal])) - }) - return Effect.sync(() => { - proc.kill("SIGTERM") - }) - }) + Effect.callback( + (resume) => { + const exited = Deferred.makeUnsafe() + const closed = Deferred.makeUnsafe() + const proc = launch(command.command, command.args, opts) + let exit: readonly [code: number | null, signal: NodeJS.Signals | null] | undefined + proc.on("error", (err) => { + resume(Effect.fail(toPlatformError("spawn", err, command))) + }) + // A detached descendant can hold stdio open after this process exits. Report its + // exit promptly, while keeping "close" for kill/escalation and failed spawns. + proc.on("exit", (...args) => { + exit = args + Deferred.doneUnsafe(exited, Exit.succeed(args)) + }) + proc.on("close", (...args) => { + Deferred.doneUnsafe(closed, Exit.succeed(exit ?? args)) + Deferred.doneUnsafe(exited, Exit.succeed(exit ?? args)) + }) + proc.on("spawn", () => { + resume(Effect.succeed([proc, exited, closed])) + }) + return Effect.sync(() => { + proc.kill("SIGTERM") + }) + }, + ) const killGroup = ( command: ChildProcess.StandardCommand, @@ -388,7 +372,7 @@ export const make = Effect.gen(function* () { const extra = fds(command.options) const dir = yield* cwd(command.options) - const [proc, signal] = yield* Effect.acquireRelease( + const [proc, exited, closed] = yield* Effect.acquireRelease( spawn(command, { cwd: dir, env: env(command.options), @@ -397,11 +381,11 @@ export const make = Effect.gen(function* () { shell: command.options.shell, windowsHide: process.platform === "win32", }), - Effect.fnUntraced(function* ([proc, signal]) { - const done = yield* Deferred.isDone(signal) + Effect.fnUntraced(function* ([proc, exited, closed]) { + const done = yield* Deferred.isDone(exited) const kill = timeout(proc, command, command.options) if (done) { - const [code] = yield* Deferred.await(signal) + const [code] = yield* Deferred.await(exited) if (process.platform === "win32") return yield* Effect.void if (code !== 0 && Predicate.isNotNull(code)) return yield* Effect.ignore(kill(killGroup)) return yield* Effect.void @@ -409,11 +393,11 @@ export const make = Effect.gen(function* () { const send = (s: NodeJS.Signals) => Effect.catch(killGroup(command, proc, s), () => killOne(command, proc, s)) const sig = command.options.killSignal ?? "SIGTERM" - const attempt = send(sig).pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid) + const attempt = send(sig).pipe(Effect.andThen(Deferred.await(closed)), Effect.asVoid) const escalated = command.options.forceKillAfter ? Effect.timeoutOrElse(attempt, { duration: command.options.forceKillAfter, - orElse: () => send("SIGKILL").pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid), + orElse: () => send("SIGKILL").pipe(Effect.andThen(Deferred.await(closed)), Effect.asVoid), }) : attempt return yield* Effect.ignore(escalated) @@ -431,8 +415,8 @@ export const make = Effect.gen(function* () { all: out.all, getInputFd: fd.getInputFd, getOutputFd: fd.getOutputFd, - isRunning: Effect.map(Deferred.isDone(signal), (done) => !done), - exitCode: Effect.flatMap(Deferred.await(signal), ([code, signal]) => { + isRunning: Effect.map(Deferred.isDone(exited), (done) => !done), + exitCode: Effect.flatMap(Deferred.await(exited), ([code, signal]) => { if (Predicate.isNotNull(code)) return Effect.succeed(ExitCode(code)) return Effect.fail( toPlatformError( @@ -446,11 +430,11 @@ export const make = Effect.gen(function* () { const sig = opts?.killSignal ?? "SIGTERM" const send = (s: NodeJS.Signals) => Effect.catch(killGroup(command, proc, s), () => killOne(command, proc, s)) - const attempt = send(sig).pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid) + const attempt = send(sig).pipe(Effect.andThen(Deferred.await(closed)), Effect.asVoid) if (!opts?.forceKillAfter) return attempt return Effect.timeoutOrElse(attempt, { duration: opts.forceKillAfter, - orElse: () => send("SIGKILL").pipe(Effect.andThen(Deferred.await(signal)), Effect.asVoid), + orElse: () => send("SIGKILL").pipe(Effect.andThen(Deferred.await(closed)), Effect.asVoid), }) }, unref: Effect.sync(() => { diff --git a/packages/core/test/effect/cross-spawn-spawner.test.ts b/packages/core/test/effect/cross-spawn-spawner.test.ts index 0fd2c688b86b..ce04058949e9 100644 --- a/packages/core/test/effect/cross-spawn-spawner.test.ts +++ b/packages/core/test/effect/cross-spawn-spawner.test.ts @@ -58,6 +58,16 @@ async function gone(pid: number, timeout = 5_000) { return !alive(pid) } +async function readPid(file: string, timeout = 5_000) { + const end = Date.now() + timeout + while (Date.now() < end) { + const value = await fs.readFile(file, "utf-8").catch(() => undefined) + if (value) return Number(value) + await new Promise((resolve) => setTimeout(resolve, 50)) + } + throw new Error(`Process did not write its pid to ${file}`) +} + describe("cross-spawn spawner", () => { describe("basic spawning", () => { fx.effect( @@ -275,6 +285,34 @@ describe("cross-spawn spawner", () => { }), ) + fx.live( + "forceKillAfter escalates when the parent exits before a stubborn descendant", + Effect.gen(function* () { + if (process.platform === "win32") return + + const tmp = yield* Effect.acquireRelease( + Effect.promise(() => tmpdir()), + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ) + const pidFile = path.join(tmp.path, "stubborn-child.pid") + const code = [ + 'const cp = require("node:child_process")', + 'const child = cp.spawn(process.execPath, ["-e", "const fs = require(\\"node:fs\\"); process.on(\\"SIGTERM\\", () => {}); fs.writeFileSync(process.argv[1], String(process.pid)); setInterval(() => {}, 1000)", process.argv[1]], { stdio: "inherit" })', + "setInterval(() => {}, 1000)", + ].join("\n") + const handle = yield* ChildProcess.make(process.execPath, ["-e", code, pidFile]) + const pid = yield* Effect.promise(() => readPid(pidFile)) + + yield* handle.kill({ forceKillAfter: 100 }).pipe(Effect.ignore) + const terminated = yield* Effect.promise(() => gone(pid, 1_000)) + if (!terminated) { + yield* Effect.sync(() => process.kill(pid, "SIGKILL")) + } + expect(terminated).toBe(true) + }), + 10_000, + ) + fx.effect( "isRunning reflects process state", Effect.gen(function* () { From cfa0c931d1ecfeb03022bdc2cf1997369475db60 Mon Sep 17 00:00:00 2001 From: LukeParkerDev <10430890+Hona@users.noreply.github.com> Date: Fri, 29 May 2026 15:02:28 +1000 Subject: [PATCH 3/7] chore(core): remove trailing blank lines from spawner test --- packages/core/test/effect/cross-spawn-spawner.test.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/core/test/effect/cross-spawn-spawner.test.ts b/packages/core/test/effect/cross-spawn-spawner.test.ts index ce04058949e9..a6daf7e3192f 100644 --- a/packages/core/test/effect/cross-spawn-spawner.test.ts +++ b/packages/core/test/effect/cross-spawn-spawner.test.ts @@ -509,8 +509,3 @@ describe("cross-spawn spawner", () => { ) }) }) - - - - - From f498bd2fc633ee037e733e6048a8940c9c9160e5 Mon Sep 17 00:00:00 2001 From: LukeParkerDev <10430890+Hona@users.noreply.github.com> Date: Fri, 29 May 2026 15:06:36 +1000 Subject: [PATCH 4/7] fix(core): escalate failed process groups after exit --- packages/core/src/cross-spawn-spawner.ts | 34 +++---------- .../test/effect/cross-spawn-spawner.test.ts | 48 ++++++++++++++++--- 2 files changed, 48 insertions(+), 34 deletions(-) diff --git a/packages/core/src/cross-spawn-spawner.ts b/packages/core/src/cross-spawn-spawner.ts index 3b41edebfa88..da3251cea4da 100644 --- a/packages/core/src/cross-spawn-spawner.ts +++ b/packages/core/src/cross-spawn-spawner.ts @@ -323,27 +323,6 @@ export const make = Effect.gen(function* () { return Effect.fail(toPlatformError("kill", new Error("Failed to kill child process"), command)) }) - const timeout = - ( - proc: NodeChildProcess.ChildProcess, - command: ChildProcess.StandardCommand, - opts: ChildProcess.KillOptions | undefined, - ) => - ( - f: ( - command: ChildProcess.StandardCommand, - proc: NodeChildProcess.ChildProcess, - signal: NodeJS.Signals, - ) => Effect.Effect, - ) => { - const signal = opts?.killSignal ?? "SIGTERM" - if (Predicate.isUndefined(opts?.forceKillAfter)) return f(command, proc, signal) - return Effect.timeoutOrElse(f(command, proc, signal), { - duration: opts.forceKillAfter, - orElse: () => f(command, proc, "SIGKILL"), - }) - } - const source = (handle: ChildProcessHandle, from: ChildProcess.PipeFromOption | undefined) => { const opt = from ?? "stdout" switch (opt) { @@ -383,13 +362,6 @@ export const make = Effect.gen(function* () { }), Effect.fnUntraced(function* ([proc, exited, closed]) { const done = yield* Deferred.isDone(exited) - const kill = timeout(proc, command, command.options) - if (done) { - const [code] = yield* Deferred.await(exited) - if (process.platform === "win32") return yield* Effect.void - if (code !== 0 && Predicate.isNotNull(code)) return yield* Effect.ignore(kill(killGroup)) - return yield* Effect.void - } const send = (s: NodeJS.Signals) => Effect.catch(killGroup(command, proc, s), () => killOne(command, proc, s)) const sig = command.options.killSignal ?? "SIGTERM" @@ -400,6 +372,12 @@ export const make = Effect.gen(function* () { orElse: () => send("SIGKILL").pipe(Effect.andThen(Deferred.await(closed)), Effect.asVoid), }) : attempt + if (done) { + const [code] = yield* Deferred.await(exited) + if (process.platform === "win32") return yield* Effect.void + if (code !== 0 && Predicate.isNotNull(code)) return yield* Effect.ignore(escalated) + return yield* Effect.void + } return yield* Effect.ignore(escalated) }), ) diff --git a/packages/core/test/effect/cross-spawn-spawner.test.ts b/packages/core/test/effect/cross-spawn-spawner.test.ts index a6daf7e3192f..7da34ab16e0d 100644 --- a/packages/core/test/effect/cross-spawn-spawner.test.ts +++ b/packages/core/test/effect/cross-spawn-spawner.test.ts @@ -68,6 +68,16 @@ async function readPid(file: string, timeout = 5_000) { throw new Error(`Process did not write its pid to ${file}`) } +function stubbornDescendant(pidFile: string, parent: string, opts?: ChildProcess.CommandOptions) { + const code = [ + 'const cp = require("node:child_process")', + 'const fs = require("node:fs")', + 'cp.spawn(process.execPath, ["-e", "const fs = require(\\"node:fs\\"); process.on(\\"SIGTERM\\", () => {}); fs.writeFileSync(process.argv[1], String(process.pid)); setInterval(() => {}, 1000)", process.argv[1]], { stdio: "inherit" })', + parent, + ].join("\n") + return ChildProcess.make(process.execPath, ["-e", code, pidFile], opts) +} + describe("cross-spawn spawner", () => { describe("basic spawning", () => { fx.effect( @@ -295,12 +305,7 @@ describe("cross-spawn spawner", () => { (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), ) const pidFile = path.join(tmp.path, "stubborn-child.pid") - const code = [ - 'const cp = require("node:child_process")', - 'const child = cp.spawn(process.execPath, ["-e", "const fs = require(\\"node:fs\\"); process.on(\\"SIGTERM\\", () => {}); fs.writeFileSync(process.argv[1], String(process.pid)); setInterval(() => {}, 1000)", process.argv[1]], { stdio: "inherit" })', - "setInterval(() => {}, 1000)", - ].join("\n") - const handle = yield* ChildProcess.make(process.execPath, ["-e", code, pidFile]) + const handle = yield* stubbornDescendant(pidFile, "setInterval(() => {}, 1000)") const pid = yield* Effect.promise(() => readPid(pidFile)) yield* handle.kill({ forceKillAfter: 100 }).pipe(Effect.ignore) @@ -313,6 +318,37 @@ describe("cross-spawn spawner", () => { 10_000, ) + fx.live( + "scope cleanup escalates after a failed parent leaves a stubborn descendant", + Effect.gen(function* () { + if (process.platform === "win32") return + + const tmp = yield* Effect.acquireRelease( + Effect.promise(() => tmpdir()), + (tmp) => Effect.promise(() => tmp[Symbol.asyncDispose]()), + ) + const pidFile = path.join(tmp.path, "failed-parent-child.pid") + const pid = yield* Effect.scoped( + Effect.gen(function* () { + const handle = yield* stubbornDescendant( + pidFile, + 'const ready = setInterval(() => { if (fs.existsSync(process.argv[1])) { clearInterval(ready); process.exit(42) } }, 10)', + { forceKillAfter: 100 }, + ) + expect(yield* handle.exitCode).toBe(ChildProcessSpawner.ExitCode(42)) + return yield* Effect.promise(() => readPid(pidFile)) + }), + ) + + const terminated = yield* Effect.promise(() => gone(pid, 1_000)) + if (!terminated) { + yield* Effect.sync(() => process.kill(pid, "SIGKILL")) + } + expect(terminated).toBe(true) + }), + 10_000, + ) + fx.effect( "isRunning reflects process state", Effect.gen(function* () { From 8e72840cb14c3171c4b0eabbcdd32d1859b0af5e Mon Sep 17 00:00:00 2001 From: LukeParkerDev <10430890+Hona@users.noreply.github.com> Date: Fri, 29 May 2026 15:37:07 +1000 Subject: [PATCH 5/7] fix(opencode): drain buffered shell output after exit --- packages/opencode/src/tool/shell.ts | 13 +++++++++-- packages/opencode/test/tool/shell.test.ts | 27 +++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/packages/opencode/src/tool/shell.ts b/packages/opencode/src/tool/shell.ts index b6a95b5c0970..611e43eafd2d 100644 --- a/packages/opencode/src/tool/shell.ts +++ b/packages/opencode/src/tool/shell.ts @@ -1,4 +1,4 @@ -import { Effect, Stream } from "effect" +import { Effect, Fiber, Stream } from "effect" import os from "os" import { createWriteStream } from "node:fs" import * as Tool from "./tool" @@ -481,7 +481,7 @@ export const ShellTool = Tool.define( yield* Effect.addFinalizer(closeSink) const handle = yield* spawner.spawn(cmd(input.shell, input.command, input.cwd, input.env)) - yield* Effect.forkScoped( + const output = yield* Effect.forkScoped( Stream.runForEach(Stream.decodeText(handle.all), (chunk) => { const size = Buffer.byteLength(chunk, "utf-8") list.push({ text: chunk, size }) @@ -554,6 +554,15 @@ export const ShellTool = Tool.define( yield* handle.kill({ forceKillAfter: "3 seconds" }).pipe(Effect.orDie) } + if (exit.kind === "exit") { + // Process exit can arrive before buffered output is handled, while a + // detached child can keep the stream open forever. Drain briefly, then stop. + yield* Fiber.join(output).pipe( + Effect.timeoutOrElse({ duration: "100 millis", orElse: () => Effect.void }), + Effect.catch(() => Effect.void), + ) + } + return exit.kind === "exit" ? exit.code : null }), ).pipe(Effect.orDie) diff --git a/packages/opencode/test/tool/shell.test.ts b/packages/opencode/test/tool/shell.test.ts index ddaa5c2ec7b1..730c1b1c8be7 100644 --- a/packages/opencode/test/tool/shell.test.ts +++ b/packages/opencode/test/tool/shell.test.ts @@ -1227,4 +1227,31 @@ describe("tool.shell truncation", () => { }), ), ) + + it.live("saves all fast output while metadata processing is delayed", () => + runIn( + projectRoot, + Effect.gen(function* () { + const byteCount = 1_000_000 + const result = yield* run( + { + command: fill("bytes", byteCount), + description: "Generate fast output with delayed metadata", + }, + { + ...ctx, + metadata: (input) => { + const output = (input.metadata as { output?: string })?.output + if (!output) return Effect.void + return Effect.sleep("50 millis") + }, + }, + ) + const filepath = (result.metadata as { outputPath?: string }).outputPath + expect(filepath).toBeTruthy() + const saved = yield* (yield* AppFileSystem.Service).readFileString(filepath!) + expect(saved.length).toBe(byteCount) + }), + ), + ) }) From 08eee5b98d247c0566bf896c8d4a877c6d1f1044 Mon Sep 17 00:00:00 2001 From: LukeParkerDev <10430890+Hona@users.noreply.github.com> Date: Fri, 29 May 2026 16:57:16 +1000 Subject: [PATCH 6/7] fix(opencode): join shell output reader after exit, bound only detached pipes Replace the fixed 100ms post-exit drain (which truncated output when a slow per-chunk consumer was still working through a backlog) with joining the output reader fiber. For a normal command this returns the instant the stdio streams reach EOF -- full output, no fixed delay, matching upstream's backpressure-synced behavior. A detached child can hold the pipe open forever with no further output, so an idle watcher that resets on every chunk bounds only that case; a command still producing output is never cut off. --- packages/opencode/src/tool/shell.ts | 24 +++++++++---- packages/opencode/test/tool/shell.test.ts | 42 +++++++++++++++++++++++ 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/packages/opencode/src/tool/shell.ts b/packages/opencode/src/tool/shell.ts index 611e43eafd2d..ecb8f18c1e96 100644 --- a/packages/opencode/src/tool/shell.ts +++ b/packages/opencode/src/tool/shell.ts @@ -438,6 +438,7 @@ export const ShellTool = Tool.define( let last = "" const list: Chunk[] = [] let used = 0 + let drained = 0 let file = "" let sink: ReturnType | undefined let cut = false @@ -483,6 +484,7 @@ export const ShellTool = Tool.define( const output = yield* Effect.forkScoped( Stream.runForEach(Stream.decodeText(handle.all), (chunk) => { + drained++ const size = Buffer.byteLength(chunk, "utf-8") list.push({ text: chunk, size }) used += size @@ -555,12 +557,22 @@ export const ShellTool = Tool.define( } if (exit.kind === "exit") { - // Process exit can arrive before buffered output is handled, while a - // detached child can keep the stream open forever. Drain briefly, then stop. - yield* Fiber.join(output).pipe( - Effect.timeoutOrElse({ duration: "100 millis", orElse: () => Effect.void }), - Effect.catch(() => Effect.void), - ) + // The process has exited, but output it already wrote may still be buffered + // in the OS pipe / Node streams. Join the reader so it drains to EOF: for a + // normal command this returns the instant the streams end (full output, no + // fixed delay). A detached child can hold the pipe open forever with no + // further output, so also stop once the reader has produced nothing for a + // full idle window. The window resets on every chunk (`drained`), so a + // command that is still producing output is never cut off — only a process + // that has exited and left an idle pipe open hits the bound. + const settle = Effect.gen(function* () { + let seen = -1 + while (drained !== seen) { + seen = drained + yield* Effect.sleep("500 millis") + } + }) + yield* Effect.raceAll([Fiber.join(output).pipe(Effect.ignore), settle]) } return exit.kind === "exit" ? exit.code : null diff --git a/packages/opencode/test/tool/shell.test.ts b/packages/opencode/test/tool/shell.test.ts index 730c1b1c8be7..bee86928c3ee 100644 --- a/packages/opencode/test/tool/shell.test.ts +++ b/packages/opencode/test/tool/shell.test.ts @@ -1156,6 +1156,48 @@ describe("tool.shell abort", () => { }), ), ) + + it.live( + "returns promptly when a command leaves a detached child holding stdio open", + () => + Effect.gen(function* () { + if (process.platform === "win32") return + const dir = yield* tmpdirScoped() + const pidFile = path.join(dir, "daemon.pid") + // Spawn a long-lived detached child that inherits stdio (keeps the shell's + // stdout pipe open), record its pid, print output, then exit immediately. + const code = + 'const cp=require("node:child_process");const fs=require("node:fs");' + + 'const d=cp.spawn(process.execPath,["-e","setTimeout(()=>{},60000)"],{detached:true,stdio:"inherit"});' + + 'd.unref();fs.writeFileSync(Bun.argv[1],String(d.pid));process.stdout.write("STARTED");process.exit(0)' + const command = `${bin} -e ${squote(code)} ${squote(pidFile)}` + + const start = Date.now() + const result = yield* runIn( + projectRoot, + run({ command, description: "Detached child", timeout: 30_000 }), + ) + const elapsed = Date.now() - start + + // Reap the detached daemon so the pipe closes and it does not leak. + const fsvc = yield* AppFileSystem.Service + const pid = Number(yield* fsvc.readFileString(pidFile).pipe(Effect.catch(() => Effect.succeed("0")))) + if (pid) + yield* Effect.sync(() => { + try { + process.kill(pid) + } catch {} + }) + + expect(result.output).toContain("STARTED") + expect(result.metadata.exit).toBe(0) + expect(result.output).not.toContain("exceeding timeout") + // Returned via the idle drain (~500ms), not by waiting on the daemon's + // inherited pipe (60s) or the command timeout (30s). + expect(elapsed).toBeLessThan(10_000) + }), + 40_000, + ) }) describe("tool.shell truncation", () => { From 5cdcbff34111fec55f1836941a15e16f81509606 Mon Sep 17 00:00:00 2001 From: LukeParkerDev <10430890+Hona@users.noreply.github.com> Date: Fri, 29 May 2026 18:02:37 +1000 Subject: [PATCH 7/7] fix(opencode): preserve in-flight shell output while draining --- packages/opencode/src/tool/shell.ts | 27 ++++++++++++++++++----- packages/opencode/test/tool/shell.test.ts | 6 +++-- 2 files changed, 25 insertions(+), 8 deletions(-) diff --git a/packages/opencode/src/tool/shell.ts b/packages/opencode/src/tool/shell.ts index ecb8f18c1e96..d766722e460a 100644 --- a/packages/opencode/src/tool/shell.ts +++ b/packages/opencode/src/tool/shell.ts @@ -439,6 +439,7 @@ export const ShellTool = Tool.define( const list: Chunk[] = [] let used = 0 let drained = 0 + let processing = false let file = "" let sink: ReturnType | undefined let cut = false @@ -484,7 +485,7 @@ export const ShellTool = Tool.define( const output = yield* Effect.forkScoped( Stream.runForEach(Stream.decodeText(handle.all), (chunk) => { - drained++ + processing = true const size = Buffer.byteLength(chunk, "utf-8") list.push({ text: chunk, size }) used += size @@ -519,6 +520,12 @@ export const ShellTool = Tool.define( }, }), ), + Effect.ensuring( + Effect.sync(() => { + processing = false + drained++ + }), + ), ) } } @@ -528,7 +535,14 @@ export const ShellTool = Tool.define( output: last, description: input.description, }, - }) + }).pipe( + Effect.ensuring( + Effect.sync(() => { + processing = false + drained++ + }), + ), + ) }), ) @@ -562,12 +576,13 @@ export const ShellTool = Tool.define( // normal command this returns the instant the streams end (full output, no // fixed delay). A detached child can hold the pipe open forever with no // further output, so also stop once the reader has produced nothing for a - // full idle window. The window resets on every chunk (`drained`), so a - // command that is still producing output is never cut off — only a process - // that has exited and left an idle pipe open hits the bound. + // full idle window. The window resets after every completely processed + // chunk (`drained`) and cannot settle while a chunk is still being written + // or reported (`processing`), so only an exited process with an idle pipe + // hits the bound. const settle = Effect.gen(function* () { let seen = -1 - while (drained !== seen) { + while (processing || drained !== seen) { seen = drained yield* Effect.sleep("500 millis") } diff --git a/packages/opencode/test/tool/shell.test.ts b/packages/opencode/test/tool/shell.test.ts index bee86928c3ee..4706a54a70ab 100644 --- a/packages/opencode/test/tool/shell.test.ts +++ b/packages/opencode/test/tool/shell.test.ts @@ -1275,6 +1275,7 @@ describe("tool.shell truncation", () => { projectRoot, Effect.gen(function* () { const byteCount = 1_000_000 + let delayed = false const result = yield* run( { command: fill("bytes", byteCount), @@ -1284,8 +1285,9 @@ describe("tool.shell truncation", () => { ...ctx, metadata: (input) => { const output = (input.metadata as { output?: string })?.output - if (!output) return Effect.void - return Effect.sleep("50 millis") + if (!output || delayed) return Effect.void + delayed = true + return Effect.sleep("1200 millis") }, }, )