diff --git a/.specgit.yaml b/.specgit.yaml index 8f391bf947..4f6bf63f68 100644 --- a/.specgit.yaml +++ b/.specgit.yaml @@ -1,17 +1,23 @@ 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: 508 - kind: kind::docs -pr: 509 + - issue: 502 + kind: kind::fix + - issue: 503 + kind: kind::fix + - issue: 504 + kind: kind::fix +pr: 505 diff --git a/packages/opencode/src/cli/heap.ts b/packages/opencode/src/cli/heap.ts index e8ec8f1bd0..253bdfa1ce 100644 --- a/packages/opencode/src/cli/heap.ts +++ b/packages/opencode/src/cli/heap.ts @@ -1,14 +1,50 @@ import path from "path" +import * as fs from "fs/promises" import { writeHeapSnapshot } from "node:v8" import { Flag } from "@opencode-ai/core/flag/flag" import { Global } from "@opencode-ai/core/global" const MINUTE = 60_000 const LIMIT = 2 * 1024 * 1024 * 1024 +// Each snapshot is hundreds of MB and RSS storms re-arm; keep only the newest +// few so repeated snapshots cannot fill the log directory. +const RETAINED_SNAPSHOTS = 2 let timer: Timer | undefined let lock = false let armed = true +export function pruneHeapSnapshots(directory: string, keep = RETAINED_SNAPSHOTS) { + return fs + .readdir(directory, { withFileTypes: true }) + .then((entries) => { + const names = entries + .filter((entry) => entry.isFile() && entry.name.startsWith("heap-") && entry.name.endsWith(".heapsnapshot")) + .map((entry) => entry.name) + // Oldest-first by embedded timestamp, NOT by full name: the layout is + // heap--, so a plain lexicographic sort orders snapshots by + // pid across runs (pid digit-count changes and wraparound) and pruning + // would delete the newest snapshot while keeping stale ones. + .sort((a, b) => snapshotTime(a).localeCompare(snapshotTime(b))) + return Promise.all( + names.slice(0, Math.max(0, names.length - keep)).map((name) => + fs.rm(path.join(directory, name), { force: true }).catch((cause) => { + console.warn(`opencode: failed to prune heap snapshot ${name}: ${String(cause)}`) + }), + ), + ) + }) + .catch((cause) => { + // A missing log directory is the normal first-run state; anything else + // is a real prune failure and best-effort cleanup must still surface it. + if ((cause as { code?: string }).code === "ENOENT") return + console.warn(`opencode: failed to list heap snapshots for pruning: ${String(cause)}`) + }) +} + +function snapshotTime(name: string) { + return name.slice(name.lastIndexOf("-") + 1) +} + export function start() { if (!Flag.OPENCODE_AUTO_HEAP_SNAPSHOT) return if (timer) return @@ -32,6 +68,7 @@ export function start() { await Promise.resolve() .then(() => writeHeapSnapshot(file)) .catch(() => {}) + await pruneHeapSnapshots(Global.Path.log) lock = false } diff --git a/packages/opencode/src/hook/settings.ts b/packages/opencode/src/hook/settings.ts index 89dd6c587e..cf5d6bc3b9 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/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 2bbdcf95cb..5b37cf8250 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/memory/store.ts b/packages/opencode/src/memory/store.ts index e371f0f4e9..aafe852fd2 100644 --- a/packages/opencode/src/memory/store.ts +++ b/packages/opencode/src/memory/store.ts @@ -29,6 +29,10 @@ const METADATA_KEYS = [ ] as const const ITEM_KEYS = ["id", "kind", "content", "rationale", "confirmed_at"] as const +// Keep a few recent generations on disk: a reader holding a just-published +// manifest must still find its generation after later commits GC older ones. +const RETAINED_GENERATIONS = 3 + const PROHIBITED_CONTENT = [ /```|`[^`]+`/, /(?:^|\s)(?:~\/|\.\.?\/|\/)[^\s]+/, @@ -236,6 +240,25 @@ export const layer = Layer.effect( } satisfies Snapshot }) + const gcGenerations = Effect.fnUntraced(function* (projectID: ProjectV2.ID) { + const generations = home.generations(projectID) + const entries = yield* fs.readDirectoryEntries(generations) + const stale = entries + .filter((entry) => entry.type === "directory" && !entry.name.startsWith(".")) + .sort( + (a, b) => Number.parseInt(b.name, 10) - Number.parseInt(a.name, 10) || b.name.localeCompare(a.name), + ) + .slice(RETAINED_GENERATIONS) + .map((entry) => join(generations, entry.name)) + // Orphan staging directories are rename leftovers from crashed writes. + const staging = entries + .filter((entry) => entry.name.startsWith(".") && entry.name.endsWith(".tmp")) + .map((entry) => join(generations, entry.name)) + yield* Effect.forEach([...stale, ...staging], (path) => fs.remove(path, { force: true, recursive: true }), { + discard: true, + }) + }) + const writeSnapshot = Effect.fnUntraced(function* ( projectID: ProjectV2.ID, revision: number, @@ -265,6 +288,11 @@ export const layer = Layer.effect( ) }).pipe(Effect.onError(() => fs.remove(staging, { force: true, recursive: true }).pipe(Effect.ignore))) yield* fs.remove(home.topics(projectID), { force: true, recursive: true }).pipe(Effect.ignore) + // GC is best-effort: the commit has already landed, a cleanup failure + // must never fail it. + yield* gcGenerations(projectID).pipe( + Effect.catchCause((cause) => Effect.logWarning("memory generation GC failed", { cause: cause })), + ) }) const readTopics = Effect.fn("MemoryStore.readTopics")((projectID: ProjectV2.ID) => diff --git a/packages/opencode/src/share/share-next.ts b/packages/opencode/src/share/share-next.ts index 4269c65252..172e94d6cf 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/src/util/process.ts b/packages/opencode/src/util/process.ts index 173210f23c..b24538f7de 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/cli/heap.test.ts b/packages/opencode/test/cli/heap.test.ts new file mode 100644 index 0000000000..ed93cec93b --- /dev/null +++ b/packages/opencode/test/cli/heap.test.ts @@ -0,0 +1,61 @@ +import { describe, expect, test } from "bun:test" +import * as fs from "node:fs/promises" +import path from "node:path" +import { Heap } from "@/cli/heap" +import { tmpdir } from "../fixture/fixture" + +describe("heap snapshot rotation", () => { + test("prunes old snapshots keeping only the newest", async () => { + await using dir = await tmpdir() + const names = [ + "heap-111-20260101T000000000Z", + "heap-111-20260102T000000000Z", + "heap-111-20260103T000000000Z", + "heap-111-20260104T000000000Z", + ] + for (const name of names) await fs.writeFile(path.join(dir.path, `${name}.heapsnapshot`), "x") + await fs.writeFile(path.join(dir.path, "heap-not-a-snapshot.log"), "x") + await fs.writeFile(path.join(dir.path, "other-20260101T000000000Z.heapsnapshot"), "x") + + await Heap.pruneHeapSnapshots(dir.path) + + expect((await fs.readdir(dir.path)).sort()).toEqual([ + "heap-111-20260103T000000000Z.heapsnapshot", + "heap-111-20260104T000000000Z.heapsnapshot", + "heap-not-a-snapshot.log", + "other-20260101T000000000Z.heapsnapshot", + ]) + }) + + test("leaves fewer snapshots than the retention limit untouched", async () => { + await using dir = await tmpdir() + await fs.writeFile(path.join(dir.path, "heap-111-20260101T000000000Z.heapsnapshot"), "x") + + await Heap.pruneHeapSnapshots(dir.path) + + expect(await fs.readdir(dir.path)).toEqual(["heap-111-20260101T000000000Z.heapsnapshot"]) + }) + + test("prunes by embedded timestamp when pids differ across runs", async () => { + await using dir = await tmpdir() + // Lexicographic order of the full names puts heap-1000-* before heap-999-*, + // so a name-sorted prune would delete the NEWEST snapshot (1000-0103). + const names = [ + "heap-999-20260101T000000000Z", + "heap-999-20260102T000000000Z", + "heap-1000-20260103T000000000Z", + ] + for (const name of names) await fs.writeFile(path.join(dir.path, `${name}.heapsnapshot`), "x") + + await Heap.pruneHeapSnapshots(dir.path) + + expect((await fs.readdir(dir.path)).sort()).toEqual([ + "heap-1000-20260103T000000000Z.heapsnapshot", + "heap-999-20260102T000000000Z.heapsnapshot", + ]) + }) + + test("tolerates a missing log directory", async () => { + await expect(Heap.pruneHeapSnapshots(path.join("/", "opencode-missing-log-dir"))).resolves.toBeUndefined() + }) +}) 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 0000000000..ca5f31c0dd --- /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 }, + ) +}) 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 0000000000..9806e2a5f7 --- /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 b54938541b..73d1b93706 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 0000000000..0e10ca7112 --- /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).toBe(0) + const jsonLine = stdout.trimEnd().split("\n").pop() + expect(JSON.parse(jsonLine ?? "")).toMatchObject({ ok: true, rootDead: true, childDead: true }) +}) diff --git a/packages/opencode/test/memory/memory-persistence.test.ts b/packages/opencode/test/memory/memory-persistence.test.ts index a1d5421e0f..4d00f70f86 100644 --- a/packages/opencode/test/memory/memory-persistence.test.ts +++ b/packages/opencode/test/memory/memory-persistence.test.ts @@ -7,6 +7,7 @@ import { AbsolutePath } from "@opencode-ai/core/schema" import { EffectFlock } from "@opencode-ai/core/util/effect-flock" import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" import { Effect, Exit, Fiber, Layer, Ref, Schema } from "effect" +import { chmod } from "node:fs/promises" import path from "node:path" import { MemoryConfig } from "@/memory/config" import { MemoryHome } from "@/memory/home" @@ -771,6 +772,87 @@ describe("Project-owned MEMORY persistence", () => { }), ) + it.live( + "retains only the newest generations after repeated commits and sweeps orphan staging", + () => + Effect.gen(function* () { + const root = yield* tmpdirScoped() + yield* Effect.gen(function* () { + const fs = yield* FSUtil.Service + const home = yield* MemoryHome.Service + const store = yield* MemoryStore.Service + + // Crash leftover: a staging directory that never reached its rename. + const orphan = path.join(home.generations(projectID), ".0-crashed.tmp") + yield* fs.makeDirectory(orphan, { recursive: true }) + yield* fs.writeFileString(path.join(orphan, "project-architecture.yaml"), "id: orphan\n") + + for (let i = 1; i <= 5; i++) { + yield* replaceTopics(store, projectID, [topic(`第${i}版已确认架构边界`)]) + } + + const entries = yield* fs.readDirectoryEntries(home.generations(projectID)) + const generations = entries + .filter((entry) => !entry.name.startsWith(".")) + .map((entry) => entry.name) + .sort((a, b) => Number.parseInt(a, 10) - Number.parseInt(b, 10)) + expect(generations).toHaveLength(3) + expect(generations.map((name) => Number.parseInt(name, 10))).toEqual([3, 4, 5]) + expect(entries.some((entry) => entry.name.endsWith(".tmp"))).toBe(false) + // The manifest still points at a retained generation. + expect(yield* store.readSnapshot(projectID)).toEqual({ + revision: 5, + topics: [topic("第5版已确认架构边界")], + }) + }).pipe(Effect.provide(layers(root))) + }), + ) + + // Windows chmod is a no-op on directories, so the undeletable-generation + // injection cannot be staged there. + const itPosix = process.platform === "win32" ? it.live.skip : it.live + itPosix( + "keeps committing when generation GC cannot delete a stale generation", + () => + Effect.gen(function* () { + const root = yield* tmpdirScoped() + yield* Effect.gen(function* () { + const fs = yield* FSUtil.Service + const home = yield* MemoryHome.Service + const store = yield* MemoryStore.Service + + for (let i = 1; i <= 3; i++) { + yield* replaceTopics(store, projectID, [topic(`第${i}版已确认架构边界`)]) + } + const entries = yield* fs.readDirectoryEntries(home.generations(projectID)) + const oldest = entries + .filter((entry) => !entry.name.startsWith(".")) + .map((entry) => entry.name) + .sort((a, b) => Number.parseInt(a, 10) - Number.parseInt(b, 10))[0] + const oldestPath = path.join(home.generations(projectID), oldest) + + // A read-only directory with a file inside cannot be removed; GC + // fails while the commit that already landed must not. + yield* Effect.acquireUseRelease( + Effect.promise(() => chmod(oldestPath, 0o555)), + () => + Effect.gen(function* () { + const exit = yield* Effect.exit(replaceTopics(store, projectID, [topic("第四版已确认架构边界")])) + expect(Exit.isSuccess(exit)).toBe(true) + expect(yield* store.readSnapshot(projectID)).toEqual({ + revision: 4, + topics: [topic("第四版已确认架构边界")], + }) + // The undeletable generation is still on disk — the failure + // is contained in GC, not the commit. + expect(yield* fs.exists(oldestPath)).toBe(true) + }), + () => Effect.promise(() => chmod(oldestPath, 0o755)).pipe(Effect.ignore), + ) + }).pipe(Effect.provide(layers(root))) + }), + ) + it.live( "an orphaned staging generation never shadows the committed generation (MEM-PR01-R1-21)", () => diff --git a/packages/opencode/test/share/share-next.test.ts b/packages/opencode/test/share/share-next.test.ts index 7a9a2f6747..6d418f429b 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 @@ -294,6 +333,7 @@ describe("ShareNext", () => { expect(seen).toHaveLength(1) expect(seen[0].url).toBe("https://legacy-share.example.com/api/share/shr_abc/sync") + // oxlint-disable-next-line typescript-eslint/no-unsafe-type-assertion -- intentional wire-shape assertion on parsed JSON in a test const body = JSON.parse(seen[0].body) as { secret: string data: Array<{ @@ -325,4 +365,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) + }), + ) + }) }) diff --git a/packages/opencode/test/util/process.test.ts b/packages/opencode/test/util/process.test.ts index 934833d1d0..90db2c7961 100644 --- a/packages/opencode/test/util/process.test.ts +++ b/packages/opencode/test/util/process.test.ts @@ -126,3 +126,106 @@ 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( + // Trap BEFORE the ready write: the parent stops as soon as it sees + // "ready", and under CI load the child can be preempted between the two + // statements, taking the first SIGTERM with the default handler still + // installed (exits SIGTERM instead of escalating to SIGKILL). + node('process.on("SIGTERM", () => {});process.stdout.write("ready\\n");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