From d58037d9082ea36b4a6da8f51379f8877251b0f9 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 10:47:46 +0800 Subject: [PATCH 01/13] chore: record delivery binding for hook-command-grandchildren --- .specgit.yaml | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/.specgit.yaml b/.specgit.yaml index 8f391bf94..fbc397a70 100644 --- a/.specgit.yaml +++ b/.specgit.yaml @@ -1,17 +1,22 @@ version: 1 -delivery: accept-builtin-spec +delivery: hook-command-grandchildren context: kind: branch - branch: fix/506-accept-builtin-spec + branch: fix/500-hook-command-grandchildren issues: - - 506 - - 507 - - 508 + - 500 + - 501 + - 502 + - 503 + - 504 issueKinds: - - issue: 506 + - issue: 500 kind: kind::fix - - issue: 507 + - issue: 501 + kind: kind::fix + - issue: 502 + kind: kind::fix + - issue: 503 + kind: kind::fix + - issue: 504 kind: kind::fix - - issue: 508 - kind: kind::docs -pr: 509 From bf76ff24828396f5540646bff9ca3e7ccc9bae0b Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 10:48:14 +0800 Subject: [PATCH 02/13] chore: record delivery binding for hook-command-grandchildren --- .specgit.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.specgit.yaml b/.specgit.yaml index fbc397a70..4f6bf63f6 100644 --- a/.specgit.yaml +++ b/.specgit.yaml @@ -20,3 +20,4 @@ issueKinds: kind: kind::fix - issue: 504 kind: kind::fix +pr: 505 From 19fcd386888e82343a87acde36a6d7e261f4080a Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 13:32:53 +0800 Subject: [PATCH 03/13] fix(hook): kill the process group on timeout so pipe-holding grandchildren cannot hang triggers A timed-out command hook only SIGTERM'd the shell wrapper; grandchildren keeping the stdio pipes open meant the close event never fired and the hook trigger hung forever. Exit and stream-drain are now awaited separately with a bounded grace, then the whole group (detached, negative pid on POSIX; taskkill /T /F on Windows) is SIGKILL'd and reaped. --- packages/opencode/src/hook/settings.ts | 93 +++++++++++++++++-- .../test/hook/grandchild-pipe-hang.test.ts | 91 ++++++++++++++++++ 2 files changed, 177 insertions(+), 7 deletions(-) create mode 100644 packages/opencode/test/hook/grandchild-pipe-hang.test.ts diff --git a/packages/opencode/src/hook/settings.ts b/packages/opencode/src/hook/settings.ts index 89dd6c587..cf5d6bc3b 100644 --- a/packages/opencode/src/hook/settings.ts +++ b/packages/opencode/src/hook/settings.ts @@ -1009,6 +1009,12 @@ export function warnUnsupportedFields( const DEFAULT_TIMEOUT_MS = 60_000 // CC default +// #500: after the child exits (or the spawn timeout SIGTERMs it), stdio EOF is +// given this grace before the whole process group is SIGKILLed; DRAIN_MS is +// the post-kill window for final buffered output before resolving partial. +const KILL_GRACE_MS = 2_000 +const DRAIN_MS = 500 + function execShell( entry: HookCommand, stdinJSON: string, @@ -1051,12 +1057,16 @@ function execShell( } } + // POSIX: run the child as a process-group leader so a hung grandchild + // holding the stdio pipes can be signaled as a group (#500). Windows + // relies on `taskkill /T` tree kill instead. const child = spawn(expandedCommand, [], { cwd, shell, env: { ...process.env, ...extraEnv }, stdio: ["pipe", "pipe", "pipe"], timeout: timeoutMs, + detached: process.platform !== "win32", }) let stdout = "" @@ -1078,20 +1088,89 @@ function execShell( log.warn("hook stdin write failed", { command, error: String(err) }) } - child.on("error", (err) => { - log.error("hook command failed to spawn", { command, error: err.message }) - resolve({ exitCode: null, stdout, stderr, spawnError: err.message }) + // #500: `close` only fires after BOTH exit and stdio EOF; a grandchild + // that inherits the pipes and outlives the shell blocks EOF forever, so + // the old `close` waiter never resolved. Exit and stream EOF are now + // awaited as independent conditions, with a process-group SIGKILL as the + // fallback that guarantees resolution. + let exitCode: number | null = null + let settled = false + const timers = new Set() + const arm = (fire: () => void, ms: number) => { + const timer = setTimeout(() => { + timers.delete(timer) + if (!settled) fire() + }, ms) + timers.add(timer) + } + const exited = new Promise((resolveExit) => { + child.on("exit", (code) => { + exitCode = code + resolveExit() + }) }) - - child.on("close", (code) => { + // `end` alone is not enough: on spawn failure the streams can close or + // error without ever reaching EOF. + const streamSettled = (stream: NodeJS.ReadableStream) => + new Promise((resolveStream) => { + stream.on("end", resolveStream) + stream.on("close", resolveStream) + stream.on("error", resolveStream) + }) + const streamsDone = Promise.all([streamSettled(child.stdout), streamSettled(child.stderr)]) + const finish = (spawnError?: string) => { + if (settled) return + settled = true + for (const timer of timers) clearTimeout(timer) + timers.clear() + child.stdout.destroy() + child.stderr.destroy() log.debug("hook close", { command: command.slice(0, 80), - exitCode: code, + exitCode, stdoutLen: stdout.length, stderrLen: stderr.length, }) - resolve({ exitCode: code, stdout, stderr }) + resolve(spawnError === undefined ? { exitCode, stdout, stderr } : { exitCode, stdout, stderr, spawnError }) + } + + child.on("error", (err) => { + log.error("hook command failed to spawn", { command, error: err.message }) + finish(err.message) }) + + let killSent = false + const killGroup = () => { + if (killSent || child.pid === undefined) return + killSent = true + if (process.platform === "win32") { + spawn("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore", windowsHide: true }).on( + "error", + (err) => log.warn("hook taskkill failed", { command, error: err.message }), + ) + return + } + try { + process.kill(-child.pid, "SIGKILL") + } catch (err) { + log.warn("hook process-group kill failed", { command, error: String(err) }) + } + } + const afterKill = () => { + killGroup() + const drained = new Promise((resolveDrain) => arm(resolveDrain, DRAIN_MS)) + void Promise.all([exited, Promise.race([streamsDone, drained])]).then(() => finish()) + } + + void Promise.all([exited, streamsDone]).then(() => finish()) + + // Child exited but pipes are still open (grandchild holds them): kill the + // group after the EOF grace, then resolve with whatever was captured. + void exited.then(() => arm(afterKill, KILL_GRACE_MS)) + + // Child ignored the spawn-timeout SIGTERM and never exited: kill the group + // at the absolute deadline, wait for the reap, then resolve. + arm(afterKill, timeoutMs + KILL_GRACE_MS) }) } diff --git a/packages/opencode/test/hook/grandchild-pipe-hang.test.ts b/packages/opencode/test/hook/grandchild-pipe-hang.test.ts new file mode 100644 index 000000000..ca5f31c0d --- /dev/null +++ b/packages/opencode/test/hook/grandchild-pipe-hang.test.ts @@ -0,0 +1,91 @@ +import { describe, expect } from "bun:test" +import { Effect, Layer } from "effect" +import { spawnSync } from "child_process" +import * as fs from "fs/promises" +import path from "path" +import { SettingsHook } from "@/hook/settings" +import { SessionHooks } from "@/hook/session-hooks" +import { EventV2Bridge } from "@/event-v2-bridge" +import { Database } from "@opencode-ai/core/database/database" +import { testEffect } from "../lib/effect" + +// #500: a hook command that spawns a grandchild holding the stdio pipes and +// then hits its timeout must not hang the trigger forever. execShell kills +// the child's process group once the EOF grace elapses, so the trigger +// returns within the timeout window plus grace, and the grandchild is gone. +// Mirrors stdout-context.test.ts (real SettingsHook layer, execShell actually +// runs the command, hooks.json written via init). + +const testLayer = SettingsHook.layer.pipe( + Layer.provide(EventV2Bridge.defaultLayer), + Layer.provide(Database.defaultLayer), + Layer.provideMerge(SessionHooks.defaultLayer), +) +const it = testEffect(testLayer) + +const writeHooks = (hooks: unknown) => (dir: string) => + Effect.promise(() => + fs.mkdir(path.join(dir, ".opencode"), { recursive: true }).then(() => + fs.writeFile(path.join(dir, ".opencode", "hooks.json"), JSON.stringify(hooks)), + ), + ) + +// Unique marker so pgrep only matches this test's grandchild. +const GRANDCHILD = "sleep 597.3" + +const grandchildGone = () => + Effect.promise(async () => { + for (let i = 0; i < 20; i++) { + // pgrep exits 1 when no process matches; a null status means pgrep is + // unavailable — nothing to assert. + if (spawnSync("pgrep", ["-f", GRANDCHILD]).status !== 0) return true + await new Promise((resolve) => setTimeout(resolve, 100)) + } + return false + }) + +describe("SettingsHook execShell pipe-holding grandchild (#500)", () => { + it.instance( + "timed-out hook with pipe-holding grandchild resolves and kills the group", + () => + Effect.gen(function* () { + if (process.platform === "win32") return + const hook = yield* SettingsHook.Service + const startedAt = Date.now() + const r = yield* hook.trigger( + { event: "UserPromptSubmit", prompt: "test" }, + { sessionID: "sess-500-1", transcriptPath: "" }, + ) + const elapsed = Date.now() - startedAt + // Must resolve far below the 597s the grandchild would hold the pipe. + expect(elapsed).toBeLessThan(15_000) + expect(r.additionalContexts).toEqual([]) + expect(yield* grandchildGone()).toBe(true) + }), + { + init: writeHooks({ + UserPromptSubmit: [{ hooks: [{ type: "command", command: `${GRANDCHILD} & wait`, timeout: 1 }] }], + }), + }, + { timeout: 30_000 }, + ) + + it.instance( + "fast command with a configured timeout still completes normally", + () => + Effect.gen(function* () { + const hook = yield* SettingsHook.Service + const r = yield* hook.trigger( + { event: "UserPromptSubmit", prompt: "test" }, + { sessionID: "sess-500-2", transcriptPath: "" }, + ) + expect(r.additionalContexts).toEqual(["fast-ok-500"]) + }), + { + init: writeHooks({ + UserPromptSubmit: [{ hooks: [{ type: "command", command: "printf '%s' 'fast-ok-500'", timeout: 30 }] }], + }), + }, + { timeout: 10_000 }, + ) +}) From cbca6c3ef54e26a90de8c4aef1149d668c0b05df Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 13:33:10 +0800 Subject: [PATCH 04/13] fix(share): unsubscribe the five instance event listeners on dispose watch() discarded every Unsubscribe, so each instance dispose/remount cycle leaked five permanent EventV2 listeners holding the instance context. The finalizer now unsubscribes before closing the scope. --- packages/opencode/src/share/share-next.ts | 11 +++- .../opencode/test/share/share-next.test.ts | 63 ++++++++++++++++++- 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/share/share-next.ts b/packages/opencode/src/share/share-next.ts index 4269c6525..172e94d6c 100644 --- a/packages/opencode/src/share/share-next.ts +++ b/packages/opencode/src/share/share-next.ts @@ -150,9 +150,16 @@ export const layer = Layer.effect( const state: InstanceState.InstanceState = yield* InstanceState.make( Effect.fn("ShareNext.state")(function* (_ctx) { const cache: State = { queue: new Map(), scope: yield* Scope.make(), shared: new Map() } + // EventV2 listeners live in a process-level array; collect their + // unsubscribers or every instance remount leaks another batch of + // subscribers pinning this closure. + const unsubscribers: Array = [] yield* Effect.addFinalizer(() => - Scope.close(cache.scope, Exit.void).pipe( + // Unsubscribe before closing the scope so no in-flight event lands + // in a subscriber whose fork scope is already gone. + Effect.forEach(unsubscribers, (unsubscribe) => unsubscribe, { discard: true }).pipe( + Effect.andThen(Scope.close(cache.scope, Exit.void)), Effect.andThen( Effect.sync(() => { cache.queue.clear() @@ -182,7 +189,7 @@ export const layer = Layer.effect( Effect.logError("share subscriber failed", { type: def.type, cause: cause }), ), ) - }) + }).pipe(Effect.tap((unsubscribe) => Effect.sync(() => unsubscribers.push(unsubscribe)))) yield* watch(Session.Event.Updated, (data) => Effect.gen(function* () { diff --git a/packages/opencode/test/share/share-next.test.ts b/packages/opencode/test/share/share-next.test.ts index 7a9a2f674..87ea4bd80 100644 --- a/packages/opencode/test/share/share-next.test.ts +++ b/packages/opencode/test/share/share-next.test.ts @@ -1,5 +1,5 @@ import { beforeEach, describe, expect } from "bun:test" -import { Effect, Exit, Layer, Option } from "effect" +import { Effect, Exit, Layer, Option, Context } from "effect" import { HttpClient, HttpClientRequest, HttpClientResponse } from "effect/unstable/http" import { LayerNode } from "@opencode-ai/core/effect/layer-node" import { httpClient } from "@opencode-ai/core/effect/layer-node-platform" @@ -9,6 +9,8 @@ import { SessionProjector } from "@opencode-ai/core/session/projector" import { AccessToken, AccountID, OrgID, RefreshToken } from "../../src/account/schema" import { AccountRepo } from "../../src/account/repo" import { EventV2Bridge } from "../../src/event-v2-bridge" +import { InstanceStore } from "../../src/project/instance-store" +import { EventV2 } from "@opencode-ai/core/event" import { Session } from "@/session/session" import type { SessionID } from "../../src/session/schema" import { ShareNext } from "@/share/share-next" @@ -55,6 +57,43 @@ function integrationLayer(client: HttpClient.HttpClient) { ) } +type ListenerCounts = { listen: number; removed: number } + +// Wraps the real bridge and counts listen registrations and the unsubscribers +// ShareNext is expected to run on instance disposal. +function countingBridgeNode(counts: ListenerCounts) { + return LayerNode.make( + Layer.effect( + EventV2Bridge.Service, + Effect.gen(function* () { + const bridge = Context.get(yield* Layer.build(EventV2Bridge.layer), EventV2Bridge.Service) + const listen: EventV2.Interface["listen"] = (listener) => + Effect.suspend(() => { + counts.listen++ + return bridge.listen(listener).pipe( + Effect.map((unsubscribe) => + Effect.sync(() => { + counts.removed++ + }).pipe(Effect.andThen(unsubscribe)), + ), + ) + }) + return EventV2Bridge.Service.of({ ...bridge, listen }) + }), + ), + [EventV2.node], + ) +} + +function countingLayer(counts: ListenerCounts) { + return LayerNode.buildLayer( + LayerNode.group([ShareNext.node, Session.node, SessionProjector.node, AccountRepo.node, Database.node]), + { + replacements: [LayerNode.replaceWithNode(EventV2Bridge.node, countingBridgeNode(counts))], + }, + ) +} + const share = (id: SessionID) => Effect.gen(function* () { const { db } = yield* Database.Service @@ -325,4 +364,26 @@ describe("ShareNext", () => { { config: { enterprise: { url: "https://legacy-share.example.com" } } }, ), ) + + it.live("unsubscribes instance event listeners on dispose so remounts do not accumulate", () => { + const counts: ListenerCounts = { listen: 0, removed: 0 } + const layers = countingLayer(counts) + return provideTmpdirInstance((directory) => + Effect.gen(function* () { + const store = yield* InstanceStore.Service + + yield* ShareNext.use.init().pipe(Effect.provide(layers)) + expect(counts.listen).toBe(5) + + yield* store.disposeDirectory(directory) + expect(counts.removed).toBe(5) + + yield* ShareNext.use.init().pipe(Effect.provide(layers)) + expect(counts.listen).toBe(10) + + yield* store.disposeDirectory(directory) + expect(counts.removed).toBe(10) + }), + ) + }) }) From 92cc97e447d0667acd14827c439b4239ca7f1924 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 13:33:29 +0800 Subject: [PATCH 05/13] fix(tui): clean up route and prompt event subscriptions on unmount Three event.on handlers discarded their unsubscribe functions and accumulated in the app-level SDK handler set on every route transition, retaining the opentui editor and renderer trees. --- packages/tui/src/component/prompt/index.tsx | 24 ++-- packages/tui/src/routes/session/index.tsx | 64 +++++---- .../tui/test/cli/tui/event-cleanup.test.tsx | 134 ++++++++++++++++++ 3 files changed, 181 insertions(+), 41 deletions(-) create mode 100644 packages/tui/test/cli/tui/event-cleanup.test.tsx diff --git a/packages/tui/src/component/prompt/index.tsx b/packages/tui/src/component/prompt/index.tsx index b205a4877..6214598d5 100644 --- a/packages/tui/src/component/prompt/index.tsx +++ b/packages/tui/src/component/prompt/index.tsx @@ -231,18 +231,20 @@ export function Prompt(props: PromptProps) { let promptPartTypeId = 0 const event = useEvent() - event.on("tui.prompt.append", (evt, { workspace }) => { - if (workspace !== project.workspace.current()) return - if (!input || input.isDestroyed) return - input.insertText(evt.properties.text) - setTimeout(() => { - // setTimeout is a workaround and needs to be addressed properly + onCleanup( + event.on("tui.prompt.append", (evt, { workspace }) => { + if (workspace !== project.workspace.current()) return if (!input || input.isDestroyed) return - input.getLayoutNode().markDirty() - input.gotoBufferEnd() - renderer.requestRender() - }, 0) - }) + input.insertText(evt.properties.text) + setTimeout(() => { + // setTimeout is a workaround and needs to be addressed properly + if (!input || input.isDestroyed) return + input.getLayoutNode().markDirty() + input.gotoBufferEnd() + renderer.requestRender() + }, 0) + }), + ) createEffect(() => { if (!input || input.isDestroyed) return diff --git a/packages/tui/src/routes/session/index.tsx b/packages/tui/src/routes/session/index.tsx index 8bd5c25ce..9ec525597 100644 --- a/packages/tui/src/routes/session/index.tsx +++ b/packages/tui/src/routes/session/index.tsx @@ -324,21 +324,23 @@ export function Session() { }) let lastSwitch: string | undefined = undefined - event.on("message.part.updated", (evt) => { - const part = evt.properties.part - if (part.type !== "tool") return - if (part.sessionID !== route.sessionID) return - if (part.state.status !== "completed") return - if (part.id === lastSwitch) return - - if (part.tool === "plan_exit") { - local.agent.set("build") - lastSwitch = part.id - } else if (part.tool === "plan_enter") { - local.agent.set("plan") - lastSwitch = part.id - } - }) + onCleanup( + event.on("message.part.updated", (evt) => { + const part = evt.properties.part + if (part.type !== "tool") return + if (part.sessionID !== route.sessionID) return + if (part.state.status !== "completed") return + if (part.id === lastSwitch) return + + if (part.tool === "plan_exit") { + local.agent.set("build") + lastSwitch = part.id + } else if (part.tool === "plan_enter") { + local.agent.set("plan") + lastSwitch = part.id + } + }), + ) let seeded = false let scroll: ScrollBoxRenderable @@ -354,25 +356,27 @@ export function Session() { const dialog = useDialog() const renderer = useRenderer() - event.on("session.status", (evt) => { - if (evt.properties.sessionID !== route.sessionID) return - if (evt.properties.status.type !== "retry") return - if (!evt.properties.status.action) return - if (dialog.stack.length > 0) return + onCleanup( + event.on("session.status", (evt) => { + if (evt.properties.sessionID !== route.sessionID) return + if (evt.properties.status.type !== "retry") return + if (!evt.properties.status.action) return + if (dialog.stack.length > 0) return - const keys = goUpsellKeys(evt.properties.status.action) - if (!keys) return + const keys = goUpsellKeys(evt.properties.status.action) + if (!keys) return - const seen = kv.get(keys.lastSeenAt) - if (typeof seen === "number" && Date.now() - seen < GO_UPSELL_WINDOW) return + const seen = kv.get(keys.lastSeenAt) + if (typeof seen === "number" && Date.now() - seen < GO_UPSELL_WINDOW) return - if (kv.get(keys.dontShow)) return + if (kv.get(keys.dontShow)) return - void DialogRetryAction.show(dialog, evt.properties.status.action).then((dontShowAgain) => { - if (dontShowAgain) kv.set(keys.dontShow, true) - kv.set(keys.lastSeenAt, Date.now()) - }) - }) + void DialogRetryAction.show(dialog, evt.properties.status.action).then((dontShowAgain) => { + if (dontShowAgain) kv.set(keys.dontShow, true) + kv.set(keys.lastSeenAt, Date.now()) + }) + }), + ) // Helper: Find next visible message boundary in direction const findNextVisibleMessage = (direction: "next" | "prev"): string | null => { diff --git a/packages/tui/test/cli/tui/event-cleanup.test.tsx b/packages/tui/test/cli/tui/event-cleanup.test.tsx new file mode 100644 index 000000000..bb4420d08 --- /dev/null +++ b/packages/tui/test/cli/tui/event-cleanup.test.tsx @@ -0,0 +1,134 @@ +/** @jsxImportSource @opentui/solid */ +import { describe, expect, test } from "bun:test" +import { testRender } from "@opentui/solid" +import type { Event, GlobalEvent } from "@opencode-ai/sdk/v2" +import { createSignal, onCleanup, onMount, Show } from "solid-js" +import { SDKProvider } from "../../../src/context/sdk" +import { useEvent } from "../../../src/context/event" +import { createEventSource, createFetch, directory } from "../../fixture/tui-sdk" +import { TestTuiContexts } from "../../fixture/tui-environment" + +// Route components (routes/session/index.tsx, component/prompt/index.tsx) +// subscribe to app-level events via `onCleanup(event.on(...))` so the +// handler dies with the owning scope. These tests pin that contract at the +// seam it depends on: while the SDKProvider (app lifetime) stays alive, +// unmounting the owning component must remove its handler from the +// app-level emitter, and mount/unmount cycles must not accumulate handlers. + +const sessionID = "ses_route" + +async function wait(fn: () => boolean, timeout = 2000) { + const start = Date.now() + while (!fn()) { + if (Date.now() - start > timeout) throw new Error("timed out waiting for condition") + await Bun.sleep(10) + } +} + +function event(payload: Event): GlobalEvent { + return { directory, payload } +} + +function partUpdated(text: string): Event { + return { + id: `evt_${text}`, + type: "message.part.updated", + properties: { + sessionID, + time: 1, + part: { id: `part_${text}`, sessionID, messageID: "msg_1", type: "text", text }, + }, + } +} + +// Mirrors the production subscription shape: the unsubscribe returned by +// event.on is registered with onCleanup in the component body. +function RouteProbe(props: { received: string[] }) { + const event = useEvent() + onCleanup( + event.on("message.part.updated", (evt) => { + if (evt.properties.part.type !== "text") return + props.received.push(evt.properties.part.text) + }), + ) + return +} + +// Root-level subscription that never unmounts. When it has observed an +// event, the emitter batch has flushed, so any still-registered route +// handler would have observed it in the same pass. +function ControlProbe(props: { received: string[]; onReady: () => void }) { + const event = useEvent() + onCleanup(event.subscribe((evt) => props.received.push(evt.id))) + onMount(() => props.onReady()) + return +} + +async function mount() { + const events = createEventSource() + const calls = createFetch() + const route: string[] = [] + const control: string[] = [] + const [mounted, setMounted] = createSignal(true) + let ready!: () => void + const done = new Promise((resolve) => { + ready = resolve + }) + + const app = await testRender(() => ( + + + + + + + + + )) + + await done + return { + app, + emit: (e: GlobalEvent) => events.emit(e), + route, + control, + unmount: () => setMounted(false), + remount: () => setMounted(true), + } +} + +describe("event.on cleanup", () => { + test("unmounted component stops receiving events while the SDK provider lives", async () => { + const { app, emit, route, control, unmount } = await mount() + + try { + emit(event(partUpdated("before"))) + await wait(() => control.includes("evt_before")) + expect(route).toEqual(["before"]) + + unmount() + emit(event(partUpdated("after"))) + await wait(() => control.includes("evt_after")) + expect(route).toEqual(["before"]) + } finally { + app.renderer.destroy() + } + }) + + test("mount/unmount cycles do not accumulate handlers", async () => { + const { app, emit, route, control, unmount, remount } = await mount() + + try { + unmount() + remount() + unmount() + remount() + + emit(event(partUpdated("single"))) + await wait(() => control.includes("evt_single")) + expect(route).toEqual(["single"]) + } finally { + app.renderer.destroy() + } + }) +}) From dd70af13a80fc9af3e4d6499fa713744392917b1 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 13:33:57 +0800 Subject: [PATCH 06/13] fix(process): await exit with SIGKILL escalation in stop and MCP client shutdown stop() on POSIX returned right after SIGTERM, so servers ignoring it stayed alive as orphans while instance finalizers reported success. It now waits a bounded grace, escalates to SIGKILL, and awaits exit; the SDK copy adapts the same escalation synchronously with an unref'd timer. MCP client shutdown reaps the whole process tree through a shared shutdownClient used by the state finalizer, closeClient, and the create rollback path. --- packages/opencode/src/mcp/index.ts | 41 ++++---- packages/opencode/src/util/process.ts | 73 ++++++++++++++ .../test/mcp/fixtures/process-tree-probe.ts | 84 ++++++++++++++++ .../test/mcp/fixtures/server-stdio.ts | 12 +++ .../opencode/test/mcp/process-tree.test.ts | 27 +++++ packages/opencode/test/util/process.test.ts | 99 +++++++++++++++++++ packages/sdk/js/src/process.ts | 14 ++- 7 files changed, 329 insertions(+), 21 deletions(-) create mode 100644 packages/opencode/test/mcp/fixtures/process-tree-probe.ts create mode 100644 packages/opencode/test/mcp/process-tree.test.ts diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 2bbdcf95c..5b37cf825 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -12,6 +12,7 @@ import { ConfigMCPV1 } from "@opencode-ai/core/v1/config/mcp" import { NamedError } from "@opencode-ai/core/util/error" import { InstallationVersion } from "@opencode-ai/core/installation/version" import { withTimeout } from "@/util/timeout" +import { Process } from "@/util/process" import { FSUtil } from "@opencode-ai/core/fs-util" import { McpOAuthProvider, OAUTH_CALLBACK_PATH } from "./oauth-provider" import { McpOAuthCallback } from "./oauth-callback" @@ -398,7 +399,10 @@ export const layer = Layer.effect( } satisfies CreateResult }).pipe( Effect.catchCause((cause) => - Effect.tryPromise(() => mcpClient.close()).pipe(Effect.ignore, Effect.andThen(Effect.failCause(cause))), + Effect.gen(function* () { + yield* shutdownClient(mcpClient) + return yield* Effect.failCause(cause) + }), ), ) }, @@ -437,6 +441,19 @@ export const layer = Layer.effect( Effect.catch(() => Effect.succeed([] as number[])), ) + // Close a client and make sure its whole process tree is reaped. The + // descendant snapshot must be taken while the root pid is alive (pgrep + // walks parent links); close() shuts the root down gracefully, and + // stopTree force-kills whatever survived (ignored SIGTERM, orphaned + // grandchildren) and waits for exit. + const shutdownClient = Effect.fnUntraced(function* (client: MCPClient) { + const pid = client.transport instanceof StdioClientTransport ? client.transport.pid : null + const tree = typeof pid === "number" ? [pid, ...(yield* descendants(pid))] : [] + yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore) + if (tree.length === 0) return + yield* Effect.tryPromise(() => Process.stopTree(tree)).pipe(Effect.ignore) + }) + function watch(s: State, name: string, client: MCPClient, bridge: EffectBridge.Shape, timeout?: number) { // mcp-elicitation-notification: handle `elicitation/create` reverse requests. // Routes through the Question service (best-effort session via SessionContext), @@ -536,23 +553,7 @@ export const layer = Layer.effect( s.clients = {} s.defs = {} s.instructions = {} - yield* Effect.forEach( - clients, - (client) => - Effect.gen(function* () { - const pid = client.transport instanceof StdioClientTransport ? client.transport.pid : null - if (typeof pid === "number") { - const pids = yield* descendants(pid) - for (const dpid of pids) { - try { - process.kill(dpid, "SIGTERM") - } catch {} - } - } - yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore) - }), - { concurrency: "unbounded" }, - ) + yield* Effect.forEach(clients, (client) => shutdownClient(client), { concurrency: "unbounded" }) pendingOAuthTransports.clear() }), ) @@ -567,7 +568,7 @@ export const layer = Layer.effect( delete s.defs[name] delete s.instructions[name] if (!client) return Effect.void - return Effect.tryPromise(() => client.close()).pipe(Effect.ignore) + return shutdownClient(client) } const storeClient = Effect.fnUntraced(function* ( @@ -586,7 +587,7 @@ export const layer = Layer.effect( if (instructions) s.instructions[name] = instructions else delete s.instructions[name] watch(s, name, client, bridge, timeout) - if (previous) yield* Effect.tryPromise(() => previous.close()).pipe(Effect.ignore) + if (previous) yield* shutdownClient(previous) return s.status[name] }) diff --git a/packages/opencode/src/util/process.ts b/packages/opencode/src/util/process.ts index 173210f23..b24538f7d 100644 --- a/packages/opencode/src/util/process.ts +++ b/packages/opencode/src/util/process.ts @@ -144,6 +144,12 @@ export async function run(cmd: string[], opts: RunOptions = {}): Promise throw new RunFailedError(cmd, out.code, out.stdout, out.stderr) } +// Bounded-stop escalation constants, shared by stop() and stopTree(): time +// allowed for exit after SIGTERM before escalating to SIGKILL, and the bounded +// wait for exit after SIGKILL. +export const STOP_TERM_GRACE_MS = 3_000 +export const STOP_KILL_GRACE_MS = 2_000 + // Duplicated in `packages/sdk/js/src/process.ts` because the SDK cannot import // `opencode` without creating a cycle. Keep both copies in sync. export async function stop(proc: ChildProcess) { @@ -151,6 +157,9 @@ export async function stop(proc: ChildProcess) { if (process.platform !== "win32" || !proc.pid) { proc.kill() + if (await exitedWithin(proc, STOP_TERM_GRACE_MS)) return + proc.kill("SIGKILL") + await exitedWithin(proc, STOP_KILL_GRACE_MS) return } @@ -162,6 +171,70 @@ export async function stop(proc: ChildProcess) { proc.kill() } +function exitedWithin(proc: ChildProcess, timeoutMs: number) { + if (proc.exitCode !== null || proc.signalCode !== null) return Promise.resolve(true) + return new Promise((resolve) => { + const done = () => { + clearTimeout(timer) + resolve(true) + } + const timer = setTimeout(() => { + proc.off("exit", done) + proc.off("error", done) + resolve(proc.exitCode !== null || proc.signalCode !== null) + }, timeoutMs) + proc.once("exit", done) + proc.once("error", done) + }) +} + +export interface StopTreeOptions { + termGraceMs?: number + killGraceMs?: number +} + +// Kill every pid in the list: SIGTERM round, bounded wait, SIGKILL round, +// bounded wait. The pids may belong to processes we did not spawn +// (grandchildren), so liveness is polled with signal 0 instead of exit events. +export async function stopTree(pids: number[], opts: StopTreeOptions = {}) { + const targets = pids.filter((pid) => pid > 1) + if (targets.length === 0) return + signalTree(targets, "SIGTERM") + if (await treeExitedWithin(targets, opts.termGraceMs ?? STOP_TERM_GRACE_MS)) return + signalTree(targets, "SIGKILL") + await treeExitedWithin(targets, opts.killGraceMs ?? STOP_KILL_GRACE_MS) +} + +function signalTree(pids: number[], signal: NodeJS.Signals) { + for (const pid of pids) { + try { + process.kill(pid, signal) + } catch {} + } +} + +function treeExitedWithin(pids: number[], timeoutMs: number) { + const deadline = Date.now() + timeoutMs + return new Promise((resolve) => { + const tick = () => { + if (pids.every((pid) => !alive(pid))) return resolve(true) + if (Date.now() >= deadline) return resolve(false) + setTimeout(tick, 50) + } + tick() + }) +} + +function alive(pid: number) { + try { + process.kill(pid, 0) + return true + } catch (error) { + // EPERM: the process exists but belongs to another user. + return (error as NodeJS.ErrnoException).code === "EPERM" + } +} + export async function text(cmd: string[], opts: RunOptions = {}): Promise { const out = await run(cmd, opts) return { diff --git a/packages/opencode/test/mcp/fixtures/process-tree-probe.ts b/packages/opencode/test/mcp/fixtures/process-tree-probe.ts new file mode 100644 index 000000000..9806e2a5f --- /dev/null +++ b/packages/opencode/test/mcp/fixtures/process-tree-probe.ts @@ -0,0 +1,84 @@ +// Runs in a fresh bun process so the real @modelcontextprotocol transports are +// used even when the surrounding test run has mock.module overrides active +// (Bun's module registry is process-global across the suite). Drives +// MCP.Service against the server-stdio fixture: connect a local server that +// spawns a child, disconnect, and report whether the whole process tree was +// reaped (issue #503). +import fs from "fs/promises" +import path from "path" +import { Effect } from "effect" +import { StdioClientTransport } from "@modelcontextprotocol/client/stdio" +import { MCP } from "../../../src/mcp/index" +import { TestInstance, withTmpdirInstance } from "../../fixture/fixture" + +function alive(pid: number) { + try { + process.kill(pid, 0) + return true + } catch { + return false + } +} + +function waitDead(pid: number, timeoutMs: number) { + return new Promise((resolve) => { + const deadline = Date.now() + timeoutMs + const tick = () => { + if (!alive(pid)) return resolve(true) + if (Date.now() >= deadline) return resolve(false) + setTimeout(tick, 50) + } + tick() + }) +} + +async function waitForPidFile(file: string, timeoutMs: number) { + const deadline = Date.now() + timeoutMs + while (Date.now() < deadline) { + try { + return parseInt(await fs.readFile(file, "utf8"), 10) + } catch { + await new Promise((resolve) => setTimeout(resolve, 50)) + } + } + return undefined +} + +const result = await Effect.runPromise( + withTmpdirInstance({ config: { mcp: {} } })( + Effect.gen(function* () { + const mcp = yield* MCP.Service + const { directory } = yield* TestInstance + const childPidFile = path.join(directory, "fixture-child.pid") + + yield* mcp.add("tree-server", { + type: "local", + command: [process.execPath, path.join(import.meta.dir, "server-stdio.ts")], + environment: { MCP_FIXTURE_CHILD_PID_FILE: childPidFile }, + }) + + const status = (yield* mcp.status())["tree-server"] + if (status?.status !== "connected") return { ok: false, stage: "connect", status } + + const client = (yield* mcp.clients())["tree-server"] + const rootPid = client?.transport instanceof StdioClientTransport ? client.transport.pid : null + if (typeof rootPid !== "number") return { ok: false, stage: "root-pid" } + + const childPid = yield* Effect.promise(() => waitForPidFile(childPidFile, 5_000)) + if (childPid === undefined) return { ok: false, stage: "child-pid-file", rootPid } + + yield* mcp.disconnect("tree-server") + + return { + ok: true, + rootDead: yield* Effect.promise(() => waitDead(rootPid, 10_000)), + childDead: yield* Effect.promise(() => waitDead(childPid, 10_000)), + rootPid, + childPid, + } + }), + ).pipe(Effect.scoped, Effect.provide(MCP.defaultLayer)), +) + +console.log(JSON.stringify(result)) +if (!result.ok || !result.rootDead || !result.childDead) process.exit(1) diff --git a/packages/opencode/test/mcp/fixtures/server-stdio.ts b/packages/opencode/test/mcp/fixtures/server-stdio.ts index b54938541..73d1b9370 100644 --- a/packages/opencode/test/mcp/fixtures/server-stdio.ts +++ b/packages/opencode/test/mcp/fixtures/server-stdio.ts @@ -1,9 +1,21 @@ // Dual-era fixture: the SAME factory serves 2026-07-28 (server/discover) and // legacy (initialize) clients — serveStdio owns the era decision per connection. +import { spawn } from "node:child_process" +import { writeFileSync } from "node:fs" import { z } from "zod" import { McpServer } from "@modelcontextprotocol/server" import { serveStdio } from "@modelcontextprotocol/server/stdio" +// Process-tree fixture (#503): optionally spawn a long-lived child so tests can +// assert the server's whole tree is reaped on disconnect. The child's pid is +// published to the file named by MCP_FIXTURE_CHILD_PID_FILE. +const childPidFile = process.env.MCP_FIXTURE_CHILD_PID_FILE +if (childPidFile) { + const child = spawn(process.execPath, ["-e", "setInterval(() => {}, 60000)"], { stdio: "ignore" }) + child.unref() + writeFileSync(childPidFile, String(child.pid)) +} + serveStdio( () => { const server = new McpServer({ name: "v2-fixture", version: "1.0.0" }, { capabilities: { tools: {} } }) diff --git a/packages/opencode/test/mcp/process-tree.test.ts b/packages/opencode/test/mcp/process-tree.test.ts new file mode 100644 index 000000000..651070a07 --- /dev/null +++ b/packages/opencode/test/mcp/process-tree.test.ts @@ -0,0 +1,27 @@ +import path from "node:path" +import { expect, test } from "bun:test" + +// Issue #503: dynamic disconnect must reap the local server's whole process +// tree — the root stdio process AND any children it spawned — not just close +// the client. The probe runs in a subprocess because sibling mcp test files +// mock @modelcontextprotocol/client via mock.module, whose registry is +// process-global across the suite; a fresh process guarantees the real +// transports (same pattern as session-recovery.test.ts). +test("mcp disconnect kills the local server process and its spawned child", async () => { + if (process.platform === "win32") return // descendants discovery is POSIX-only + + const child = Bun.spawn([process.execPath, path.join(import.meta.dir, "fixtures", "process-tree-probe.ts")], { + cwd: path.join(import.meta.dir, "../.."), + stdout: "pipe", + stderr: "pipe", + }) + const [code, stdout, stderr] = await Promise.all([ + child.exited, + Bun.readableStreamToText(child.stdout), + Bun.readableStreamToText(child.stderr), + ]) + + expect(code, stderr.toString()).toBe(0) + const jsonLine = stdout.toString().trimEnd().split("\n").pop() + expect(JSON.parse(jsonLine ?? "")).toMatchObject({ ok: true, rootDead: true, childDead: true }) +}) diff --git a/packages/opencode/test/util/process.test.ts b/packages/opencode/test/util/process.test.ts index 934833d1d..e4b37f314 100644 --- a/packages/opencode/test/util/process.test.ts +++ b/packages/opencode/test/util/process.test.ts @@ -126,3 +126,102 @@ describe("util.process", () => { }) }) }) + +describe("util.process stop", () => { + test("fast path: awaits exit when the child honors SIGTERM", async () => { + if (process.platform === "win32") return + + const proc = Process.spawn(node("setInterval(() => {}, 1000)")) + const started = Date.now() + await Process.stop(proc) + await proc.exited + + expect(Date.now() - started).toBeLessThan(1500) + }, 3000) + + test("escalates to SIGKILL when the child ignores SIGTERM", async () => { + if (process.platform === "win32") return + + const proc = Process.spawn( + node('process.stdout.write("ready\\n");process.on("SIGTERM", () => {});setInterval(() => {}, 1000)'), + { stdout: "pipe" }, + ) + await new Promise((resolve) => proc.stdout!.once("data", resolve)) + + const started = Date.now() + await Process.stop(proc) + await proc.exited + + expect(proc.signalCode).toBe("SIGKILL") + expect(Date.now() - started).toBeLessThan(6000) + }, 10000) + + test("is a no-op for an already-exited child", async () => { + const proc = Process.spawn(node("process.exit(0)")) + await proc.exited + + const started = Date.now() + await Process.stop(proc) + + expect(Date.now() - started).toBeLessThan(100) + }) +}) + +describe("util.process stopTree", () => { + test("terminates a spawned process tree", async () => { + if (process.platform === "win32") return + + const pids = await spawnTree("echo $$; sleep 300 & echo $!; sleep 300 & echo $!; wait", 3) + const started = Date.now() + await Process.stopTree(pids) + + for (const pid of pids) expect(treeAlive(pid)).toBe(false) + expect(Date.now() - started).toBeLessThan(2000) + }, 5000) + + test("escalates to SIGKILL when tree members ignore SIGTERM", async () => { + if (process.platform === "win32") return + + const pids = await spawnTree('echo $$; trap "" TERM; sleep 300 & echo $!; wait', 2) + const started = Date.now() + await Process.stopTree(pids, { termGraceMs: 250, killGraceMs: 2000 }) + + for (const pid of pids) expect(treeAlive(pid)).toBe(false) + expect(Date.now() - started).toBeLessThan(3000) + }, 5000) + + test("returns immediately for an empty pid list", async () => { + await Process.stopTree([]) + }) +}) + +function treeAlive(pid: number) { + try { + process.kill(pid, 0) + return true + } catch { + return false + } +} + +// Spawns `sh -c