diff --git a/fork/manifest.json b/fork/manifest.json index f9ea483df4ca..c601d9a33dd0 100644 --- a/fork/manifest.json +++ b/fork/manifest.json @@ -230,6 +230,7 @@ "packages/opencode/test/peer/claude/probe-deliver-runtime.mts", "packages/opencode/test/peer/claude/sidecar-diagnostic-routing.test.ts", "packages/opencode/test/peer/claude/sidecar-server-name.test.ts", + "packages/opencode/test/peer/claude/sidecar-startup-race.test.ts", "packages/opencode/src/local/model-gallery/search.ts", "packages/opencode/src/local/model-gallery/install.ts", "packages/opencode/src/local/model-gallery/operations.ts", diff --git a/openspec/changes/sidecar-orphan-cleanup/proposal.md b/openspec/changes/sidecar-orphan-cleanup/proposal.md new file mode 100644 index 000000000000..5dc3d4bd4976 --- /dev/null +++ b/openspec/changes/sidecar-orphan-cleanup/proposal.md @@ -0,0 +1,109 @@ +# Proposal: sidecars that outlive their session + +## Problem + +A sidecar (`opencode debug claude-sidecar-entry`) is a separate OS process that receives +inbound peer messages over a unix socket and hands them to the session that spawned it. It +has its own lifetime, and the code has two mechanisms intended to keep that lifetime tied +to the session. On 2026-10-03 neither mechanism reclaimed **44 live sidecars** on one +machine, and they had to be killed by explicit PID. + +The 44 were all `bun.exe run /packages/opencode/src/index.ts debug +claude-sidecar-entry`, with `ppid 1`, ~16 hours old, sleeping, and **holding no session +database**. Every one was unroutable: `resolveOpencodeSender` +(`packages/opencode/src/peer/route.ts:203-208`) resolves a claimed address through the +registry, and none of these had a registry entry, so no peer could address them at all. +They were pure load — plausibly a contributor to the load spikes seen the same day. + +## Why the two existing mechanisms miss them + +**The sweep cannot, by design.** `sweepStaleSidecars` +(`packages/opencode/src/peer/claude/sidecar-registry.ts:98-126`) walks the registry +directory and removes an entry only when `!isAlive(parsed.pid)`. It removes *files for +processes that have already exited*. It never signals a live process. An orphaned sidecar +is alive by definition, so the sweep is structurally incapable of reclaiming one. + +**The self-termination guard has a startup blind spot.** +`packages/opencode/src/peer/claude/sidecar-entry.ts:93-97`: + +```ts +const originalPpid = process.ppid +const orphanCheck = setInterval(() => { + if (process.ppid !== originalPpid) shutdown() +}, 2_000) +orphanCheck.unref() +``` + +This detects a ppid that *changes after startup*. If the parent dies **before the child +evaluates `process.ppid`**, the child is already reparented (to pid 1, or the nearest +subreaper). `originalPpid` then captures the reparented value, the comparison is false +forever, and the sidecar has no parent to signal it and no sweep that can see it. + +There is a second ordering hazard behind the same outcome. +`packages/opencode/src/peer/claude/sidecar-manager.ts:121` spawns the child, and the +registration is written afterwards. A parent that dies in that window yields a process that +is both **unregistered** and **unreclaimable** — the worst combination, because the sweep +cannot even enumerate it. + +## What is already covered, and what is not + +`packages/opencode/test/peer/claude/sidecar-e2e.test.ts:222` covers the case the code was +written for: a parent that dies *after* the sidecar is up. It waits for the registration, +then exits, and asserts the sidecar self-terminates. Its own comment notes this is the +case a sweep cannot reclaim. + +Neither of the observed failure modes is that case. Both are **startup races**: the parent +already gone when the sidecar begins running. No test covers either, and the guard's +structure means the already-orphaned case is not merely untested but undetectable by the +guard as written. + +## The mechanism, confirmed and narrowed + +Two peer sessions reproduced the race independently on Bun 1.3.14/macOS. The second +investigation narrowed the window, and the narrowing matters because it corrects this +document's original description. + +The window is **not** "the parent dies before the child reads `process.ppid`" in the +abstract. In execution order: + +1. `sidecar-entry.ts:37` — `await startSidecar(...)`, which is async and writes the + registration from inside `sidecar-server.ts:156`. +2. `sidecar-entry.ts:93` — `originalPpid = process.ppid`. +3. `sidecar-entry.ts:94` — the poll interval starts. + +So the blind window is the duration of `startSidecar`: from spawn until line 93. A parent +that dies anywhere inside it leaves `originalPpid` reading as 1, the comparison +`1 !== 1` false forever, and a sidecar that never self-terminates. + +That window has two sub-cases, distinguished by whether the registration reached disk: + +- parent dies **before** `writeSidecarRegistration` completes → alive, **unregistered**, + invisible to the sweep and unroutable; +- parent dies **after** it, before line 93 → alive, **registered**, still skipped by the + sweep because the sweep only removes registrations whose pid is *dead*. + +All 44 observed instances were in the first sub-case (no registration), but both are real +and one fix covers both: the parent's stdin pipe closes at any point in the sidecar's +life, so EOF on a real pipe cannot be missed by any of these windows. + +## Why this is a change and not a patch + +The guard needs to know its spawner was real, not infer it from a ppid read that may already +be post-reparenting. Options differ in cost and in what they guarantee: + +- pass the spawner's pid explicitly via the environment, and treat "spawner pid absent or + not alive at startup" as immediate orphanhood; +- have the parent hold the socket and close it on exit, so the sidecar's accept loop + observes EOF; +- add an explicit reaper that can identify and reclaim a live sidecar with no reachable + parent, which requires a registry-independent notion of ownership. + +The first is small and closes the window. The third is what would make the sweep able to +reclaim a leak that already happened on other machines, where no fix to new spawns helps. +Which of these to do is a decision, not an implementation detail. + +## Scope + +Process lifetime and cleanup only. Nothing here touches sender identity, lead grants, or +the review record; `review-on-done` Phase 1b depends on the identity question separately +and is tracked there. \ No newline at end of file diff --git a/openspec/changes/sidecar-orphan-cleanup/specs/sidecar-orphan-cleanup/spec.md b/openspec/changes/sidecar-orphan-cleanup/specs/sidecar-orphan-cleanup/spec.md new file mode 100644 index 000000000000..c3431b3e7aef --- /dev/null +++ b/openspec/changes/sidecar-orphan-cleanup/specs/sidecar-orphan-cleanup/spec.md @@ -0,0 +1,63 @@ +## ADDED Requirements + +### Requirement: a sidecar cannot outlive the session that spawned it +A sidecar SHALL terminate within a bounded interval of its owning session's process exiting, +including when the session's process exits before the sidecar has finished starting. + +#### Scenario: parent exits during sidecar startup +- **WHEN** the spawning process exits before the sidecar process has begun executing +- **THEN** the sidecar terminates within a bounded interval rather than running indefinitely + +#### Scenario: parent exits after the sidecar is established +- **WHEN** the spawning process exits without signalling, after the sidecar is running +- **THEN** the sidecar self-terminates and removes its own registration + +#### Scenario: reparenting is not evidence of orphaning on its own +- **WHEN** a sidecar is started by an intermediate process that has already exited +- **THEN** the sidecar SHALL determine liveness from its owning session, not from a single + reading of the parent pid taken at startup + +### Requirement: every live sidecar is discoverable and reclaimable +A sidecar that is running SHALL either be attributable to a live session, or be reclaimable by +an explicit operator action. A running sidecar SHALL NOT exist that is neither. + +#### Scenario: orphan with a stale registration +- **WHEN** a sidecar is running, its registration is present, and the owning session's process + is gone +- **THEN** an operator command can identify and terminate it + +#### Scenario: orphan with no registration +- **WHEN** a sidecar is running and no registration exists for it +- **THEN** an operator command can still identify it as a candidate, and the condition is + reported rather than silently tolerated + +### Requirement: registration is established before or with the process +A sidecar's ownership SHALL be recorded such that a running sidecar is never simultaneously +unregistered and unreclaimable. + +#### Scenario: session exits between spawn and registration +- **WHEN** the session's process exits between spawning a sidecar and writing its registration +- **THEN** the resulting sidecar is still reclaimable by an operator command + +## MODIFIED Requirements + +### Requirement: the boot sweep removes stale registrations without signalling processes +The startup sweep SHALL continue to remove registration files whose process is no longer alive, +and SHALL NOT signal any running process. Reclaiming a live process is a separate, explicit +operator action. + +#### Scenario: registration whose process has exited +- **WHEN** the sweep finds a registration whose pid is not alive +- **THEN** it removes the registration file and its key file + +#### Scenario: registration whose process is alive +- **WHEN** the sweep finds a registration whose pid is alive +- **THEN** it leaves the registration untouched, regardless of whether the owning session + appears inactive + +## Non-goals + +These requirements cover process lifetime and cleanup only. Sender identity, lead grants and +the review record are unchanged here; `review-on-done` tracks identity separately, and its +Phase 1b depends on the decision recorded in +`openspec/changes/review-on-done/findings-authenticity.md`. \ No newline at end of file diff --git a/openspec/changes/sidecar-orphan-cleanup/tasks.md b/openspec/changes/sidecar-orphan-cleanup/tasks.md new file mode 100644 index 000000000000..7efb2d0636ce --- /dev/null +++ b/openspec/changes/sidecar-orphan-cleanup/tasks.md @@ -0,0 +1,65 @@ +# Tasks + +## Phase 0: confirm the mechanism before fixing it + +- [x] 0.1 Reproduce the startup race. Spawn a sidecar from a process that exits immediately, so + the child is already reparented when it reads `process.ppid`. Confirm it survives past the + 2s interval. The existing test at `test/peer/claude/sidecar-e2e.test.ts:222` deliberately + waits for registration before killing the parent, so it cannot catch this. +- [x] 0.2 Determine how a running sidecar ends up with no registration: parent exit inside the + `Process.spawn` → `writeSidecarRegistration` window (`sidecar-manager.ts:121`), a failed + write, or both. Instrument rather than assume. +- [ ] 0.3 Census the machine for other shapes of leak beyond the 44 observed on 2026-10-03, so the + fix covers the real population rather than the one case that was noticed. +- [ ] 0.4 Decide whether the fix must also reclaim sidecars already leaked elsewhere. A fix that + only prevents future leaks leaves existing ones running until someone sweeps by hand. + +## Phase 1: make orphaning detectable from the sidecar (DONE — 7efff559d0) + +- [x] 1.0 Exit on stdin EOF from the parent's pipe. The pipe closes at any point in the + sidecar's life, so this covers both sub-windows of the blind spot, including the + one where `originalPpid` is captured already-reparented. +- [x] 1.0a Bound `sidecar.stop()` in `shutdown()` with a timer. `shuttingDown` was set + before the await, so a stop that never settled left the process alive with every + later SIGTERM swallowed. +- [x] 1.0b The discriminator is `isSocket()`, NOT `isFIFO()`. Measured on Bun 1.3.14: + `stdio: ["pipe"]` gives the child a socketpair end (mode 140000, isFIFO false), + while `stdio: "ignore"` gives /dev/null (isCharacterDevice true). An isFIFO check + silently disables the guard in production — it was caught by the failing test. + + +- [ ] 1.1 Choose the liveness signal: pass the owning session's pid explicitly and treat + "absent or not alive at startup" as orphanhood, or have the parent hold the socket so the + sidecar observes EOF. Record the choice and its trade-offs in the change. +- [ ] 1.2 Remove the reliance on a single startup reading of `process.ppid` + (`sidecar-entry.ts:93`), which cannot detect a sidecar that was already orphaned. +- [ ] 1.3 Observed-red test for each scenario in the spec, mutation-checked: revert the fix and + confirm the test fails, so a green result is evidence rather than decoration. + +## Phase 2: make leaked sidecars reclaimable + +- [ ] 2.1 Add an explicit operator command that identifies candidate sidecars: running, no live + owning session, and either a stale registration or none at all. It SHALL report before it + acts, and SHALL require confirmation. +- [ ] 2.2 It SHALL refuse any candidate whose owning session is live, even if that session is + idle — the mistake that matters is killing a real session's sidecar. +- [ ] 2.3 Keep the boot sweep unchanged in behaviour: files for dead processes only, never a + signal to a running one. + +## Phase 3: close the registration window + +- [ ] 3.1 Record ownership so a running sidecar is never both unregistered and unreclaimable, + whether the session exits between spawn and registration or the write fails. +- [ ] 3.2 Test the failure path, not just the happy path. + +## Notes + +- Observed on 2026-10-03: 44 `bun.exe run /…/src/index.ts debug + claude-sidecar-entry` processes, `ppid 1`, ~16h old, sleeping, none holding a session database, + none present in `~/.claude/sessions/` with `managedBy: "opencode-skein"`. All 44 were + unroutable — `resolveOpencodeSender` resolves only through the registry — so they could not + receive or be addressed, and were pure load. They were reclaimed by explicit PID after + per-process verification. +- The 15 registered sidecars on the same machine were unaffected and spanned five repositories, + so this is a specific hole in the guard rather than a general failure of it. +- Not in scope: sender identity, lead grants, review gating. \ No newline at end of file diff --git a/packages/opencode/src/peer/claude/sidecar-entry.ts b/packages/opencode/src/peer/claude/sidecar-entry.ts index f1722a4fefdc..914fd096d606 100644 --- a/packages/opencode/src/peer/claude/sidecar-entry.ts +++ b/packages/opencode/src/peer/claude/sidecar-entry.ts @@ -6,8 +6,12 @@ // peer. Never touches the opencode server's HTTP API or any credential for // it — the parent process does the actual session injection, this process's // only job is being a real, Claude-compatible peer. +import { fstatSync } from "fs" import { startSidecar } from "./sidecar-server" +/** Upper bound on how long a sidecar may stay alive after it decides to stop. */ +const STOP_TIMEOUT_MS = 5_000 + export async function runSidecarEntry() { const ownerSessionID = process.env.OPENCODE_SIDECAR_OWNER_SESSION_ID if (!ownerSessionID) { @@ -66,11 +70,37 @@ export async function runSidecarEntry() { }) process.stdin.on("error", () => undefined) + // Whether stdin is a real pipe from the parent, or /dev/null because the + // spawner passed stdio: "ignore". Decided once, before any listener can fire, + // so a misdetection cannot kill a live sidecar. + // + // Not `isFIFO()`. Measured on Bun 1.3.14/macOS: a child spawned with + // stdio: ["pipe"] reports isSocket()=true, isFIFO()=false (mode 140000), while + // stdio: "ignore" reports isCharacterDevice()=true (mode 20000, /dev/null). + // Bun gives the child a socketpair end, not a FIFO, so checking isFIFO() + // silently disables this guard in production and only ever matches nothing. + const stdinIsPipe = (() => { + try { + const s = fstatSync(process.stdin.fd ?? 0) + return s.isSocket() || s.isFIFO() + } catch { + return false + } + })() + let shuttingDown = false const shutdown = () => { if (shuttingDown) return shuttingDown = true clearInterval(orphanCheck) + // Bound the stop. `sidecar.stop()` closes the listening socket and removes + // the registration; if that never settles (a hung close), the original + // `.finally(process.exit)` never fires — and because `shuttingDown` is now + // true, every later SIGTERM is swallowed and the process is unkillable + // except by SIGKILL. So the exit is guaranteed on a timer rather than on + // stop() settling. + const bail = setTimeout(() => process.exit(0), STOP_TIMEOUT_MS) + bail.unref() sidecar .stop() .catch(() => undefined) @@ -96,6 +126,27 @@ export async function runSidecarEntry() { }, 2_000) orphanCheck.unref() + // The ppid guard above has a startup blind spot: it can only see a parent that + // dies AFTER this line runs. If the parent is already gone by the time we read + // `process.ppid`, the value captured here is the reparented one, the comparison + // is false forever, and the sidecar survives indefinitely. Reproduced on + // Bun 1.3.14/macOS: SIGKILL the parent before the child's first ppid read and + // the child records ppid 1 and never notices. + // + // The parent's stdin pipe closes when the parent dies, at ANY point in our + // lifetime, so EOF on a real pipe is a signal that cannot be missed by a + // startup race. Registered after `orphanCheck` exists so `shutdown`'s + // `clearInterval(orphanCheck)` cannot hit the temporal dead zone. + // + // Only when stdin is genuinely a pipe. `sidecar-manager.ts` spawns us with + // `stdin: "pipe"`, but tests spawn with `stdio: "ignore"`, which hands us + // /dev/null — that reads EOF immediately and would kill a perfectly healthy + // sidecar on startup. A FIFO is the discriminator. + if (stdinIsPipe) { + process.stdin.on("end", shutdown) + process.stdin.on("close", shutdown) + } + // Never resolve on its own: when run as `debug claude-sidecar-entry`, // returning here would let `index.ts`'s `cli.parse()` complete, which // unconditionally force-exits the whole process afterward — confirmed diff --git a/packages/opencode/test/peer/claude/sidecar-startup-race.test.ts b/packages/opencode/test/peer/claude/sidecar-startup-race.test.ts new file mode 100644 index 000000000000..e1bfff5e8c6c --- /dev/null +++ b/packages/opencode/test/peer/claude/sidecar-startup-race.test.ts @@ -0,0 +1,108 @@ +// The startup race: a sidecar whose parent dies BEFORE the sidecar reads its +// own ppid is already reparented, so the ppid-guard in sidecar-entry.ts can +// never fire and the sidecar survives forever. +// +// The existing orphan test in sidecar-e2e.test.ts:222 waits for the +// registration before killing the parent, so it always exercises the case the +// ppid guard handles. This file covers the case it cannot. +// +// The mechanism was reproduced directly before writing this (Bun 1.3.14, +// macOS): SIGKILL the spawner before the child's first ppid read and the child +// records ppid 1 and never observes a change. +import { describe, expect, test } from "bun:test" +import { mkdtemp, readdir, rm } from "fs/promises" +import { tmpdir } from "os" +import { join } from "path" +import { spawn } from "child_process" + +const ENTRY = join(import.meta.dir, "../../../src/peer/claude/sidecar-entry.ts") + +describe("sidecar startup race", () => { + test("stdin EOF alone terminates the sidecar while its parent is alive", async () => { + // This is the test that makes the EOF guard *provable*, and it exists because + // the timing test above cannot: if the parent dies after the child reads + // process.ppid, the pre-existing ppid guard terminates the sidecar anyway, so + // "the sidecar terminated" does not imply "the EOF guard terminated it". + // + // Here the parent — this test process — stays alive for the whole test, so + // `process.ppid` never changes and the ppid guard cannot fire. Closing only + // the stdin pipe gives the child EOF and nothing else. If the sidecar exits + // and unregisters, the EOF guard is the only possible cause. + const claudeConfigDir = await mkdtemp(join(tmpdir(), "sidecar-eof-claude-")) + const socketDir = await mkdtemp(join(tmpdir(), "sidecar-eof-sock-")) + + const child = spawn("bun", ["run", ENTRY], { + env: { + ...process.env, + CLAUDE_CONFIG_DIR: claudeConfigDir, + OPENCODE_SIDECAR_OWNER_SESSION_ID: "ses_eof_test", + OPENCODE_SIDECAR_CWD: "/repo", + OPENCODE_SIDECAR_NAME: "opencode-eof-test", + OPENCODE_SIDECAR_SOCKET_DIR: socketDir, + }, + stdio: ["pipe", "pipe", "ignore"], + }) + let out = "" + child.stdout?.on("data", (c: Buffer) => (out += c.toString("utf8"))) + + const ready = Date.now() + 15_000 + while (Date.now() < ready && !out.includes('"type":"ready"')) { + await new Promise((r) => setTimeout(r, 100)) + } + expect(out).toContain('"type":"ready"') + expect(child.exitCode).toBeNull() + + // The only signal the child will get. Its parent is this process and stays + // right here, alive, so no reparenting can occur. + child.stdin?.end() + + const exited = new Promise((resolve) => child.once("exit", (code) => resolve(code))) + const won = await Promise.race([ + exited, + new Promise((resolve) => setTimeout(() => resolve(null), 15_000)), + ]) + expect(won).not.toBeNull() + expect(child.exitCode).not.toBeNull() + + // Unregisters on the way out, so the next boot's sweep finds nothing to do. + const entries = await readdir(join(claudeConfigDir, "sessions")).catch(() => []) + expect(entries.some((e) => e.endsWith(".json"))).toBe(false) + + child.kill("SIGKILL") + await rm(claudeConfigDir, { recursive: true, force: true }) + await rm(socketDir, { recursive: true, force: true }) + }, 45_000) + + test("stdin: ignore does not kill a sidecar on startup", async () => { + // Regression guard for the fix itself. With stdio "ignore" the child's stdin + // is /dev/null, which reads EOF immediately; a naive EOF-means-dead-parent + // listener would exit a perfectly healthy sidecar the moment it started. + const claudeConfigDir = await mkdtemp(join(tmpdir(), "sidecar-noeof-claude-")) + const socketDir = await mkdtemp(join(tmpdir(), "sidecar-noeof-sock-")) + + const child = spawn("bun", ["run", ENTRY], { + env: { + ...process.env, + CLAUDE_CONFIG_DIR: claudeConfigDir, + OPENCODE_SIDECAR_OWNER_SESSION_ID: "ses_noeof_test", + OPENCODE_SIDECAR_CWD: "/repo", + OPENCODE_SIDECAR_NAME: "opencode-noeof-test", + OPENCODE_SIDECAR_SOCKET_DIR: socketDir, + }, + stdio: ["ignore", "pipe", "ignore"], + }) + let out = "" + child.stdout?.on("data", (c: Buffer) => (out += c.toString("utf8"))) + + const deadline = Date.now() + 10_000 + while (Date.now() < deadline && !out.includes('"type":"ready"')) { + await new Promise((r) => setTimeout(r, 100)) + } + // It must still be running and registered after /dev/null EOF has passed. + expect(out).toContain('"type":"ready"') + expect(child.exitCode).toBeNull() + child.kill("SIGKILL") + await rm(claudeConfigDir, { recursive: true, force: true }) + await rm(socketDir, { recursive: true, force: true }) + }, 20_000) +}) \ No newline at end of file