From c328683f7955b2ce91ad6d554ec106cb896a261b Mon Sep 17 00:00:00 2001 From: AndreasS Date: Sat, 3 Oct 2026 05:54:30 +0200 Subject: [PATCH 1/4] docs(sidecar): orphaned sidecars cannot be reclaimed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 44 sidecars accumulated on one machine and had to be killed by explicit PID. Two independent mechanisms miss them, and neither is accidental. sweepStaleSidecars removes registration FILES whose process has already exited. It never signals a running process, so it cannot reclaim an orphan by construction. The self-termination guard tests process.ppid !== originalPpid every 2s, which catches a parent that dies after startup. If the parent dies BEFORE the child reads process.ppid, the child is already reparented to 1, originalPpid captures that, and the comparison is false forever. The sidecar then has no parent to signal it and no sweep that can see it. Worse in combination: sidecar-manager.ts:121 spawns before writeSidecarRegistration, so a parent exiting in that window leaves a process that is both unregistered and unreclaimable. All 44 were in exactly that state — none held a session database, and none were routable, since resolveOpencodeSender resolves only through the registry. They could not receive anything and could be addressed by nothing. The existing test at sidecar-e2e.test.ts:222 waits 1500ms for registration before killing the parent, so it covers the case the guard handles. Neither startup race is tested. Phase 0 is confirmation, not implementation: reproduce the race, establish how a sidecar loses its registration, census for other leak shapes, and decide whether the fix must also reclaim sidecars already leaked elsewhere. A fix that only prevents future leaks leaves existing ones running. The spec keeps the boot sweep's safety property explicit: files for dead processes only, never a signal to a running one. Reclaiming a live sidecar is an explicit, confirming operator action that must refuse any candidate whose session is live. No code change. --- .../sidecar-orphan-cleanup/proposal.md | 80 +++++++++++++++++++ .../specs/sidecar-orphan-cleanup/spec.md | 63 +++++++++++++++ .../changes/sidecar-orphan-cleanup/tasks.md | 53 ++++++++++++ 3 files changed, 196 insertions(+) create mode 100644 openspec/changes/sidecar-orphan-cleanup/proposal.md create mode 100644 openspec/changes/sidecar-orphan-cleanup/specs/sidecar-orphan-cleanup/spec.md create mode 100644 openspec/changes/sidecar-orphan-cleanup/tasks.md diff --git a/openspec/changes/sidecar-orphan-cleanup/proposal.md b/openspec/changes/sidecar-orphan-cleanup/proposal.md new file mode 100644 index 000000000000..3435d8396ee4 --- /dev/null +++ b/openspec/changes/sidecar-orphan-cleanup/proposal.md @@ -0,0 +1,80 @@ +# 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. + +## 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..199d6d01fe1f --- /dev/null +++ b/openspec/changes/sidecar-orphan-cleanup/tasks.md @@ -0,0 +1,53 @@ +# Tasks + +## Phase 0: confirm the mechanism before fixing it + +- [ ] 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. +- [ ] 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 + +- [ ] 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 From 7efff559d0cae9617a2f65a40fef3efcd8bc6e38 Mon Sep 17 00:00:00 2001 From: AndreasS Date: Sat, 3 Oct 2026 06:16:32 +0200 Subject: [PATCH 2/4] fix(sidecar): a sidecar cannot outlive a parent that died before it started MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ppid guard compares process.ppid against a value captured at startup, so it can only see a parent that dies afterwards. If the parent is already gone by the time the sidecar reads its own ppid, that value is the reparented one, the comparison is false forever, and the sidecar survives indefinitely — which is how 44 of them accumulated here and had to be killed by explicit PID. Mechanism reproduced before fixing (Bun 1.3.14/macOS): SIGKILL the spawner 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 the sidecar's life, so EOF cannot be missed by a startup race. Exit on it. The discriminator is isSocket(), not isFIFO(). Measured: a child spawned with stdio ["pipe"] reports isSocket()=true, isFIFO()=false (mode 140000), while stdio "ignore" reports isCharacterDevice()=true (/dev/null). My first attempt checked isFIFO(), which silently disabled this guard in production and matched nothing — the failing test caught it, not review. The check exists because every test spawns with stdio "ignore", where stdin is /dev/null and reads EOF immediately; an unguarded listener would kill a healthy sidecar on startup. Also bound the stop. shutdown() set shuttingDown before awaiting sidecar.stop(), so a stop() that never settled left the process alive and every later SIGTERM swallowed — killable only by SIGKILL. The exit now happens on a timer rather than on stop() settling. The new test retries rather than asserting once: hitting the race depends on bun's startup time, so an attempt where the sidecar never registered proves nothing and is discarded. It only passes having observed a registration appear and then disappear, so it cannot pass vacuously. Verified by disabling the guard — the test fails, and passes with it restored. test/peer 211 pass / 0 fail. Not addressed here: reclaiming sidecars already leaked elsewhere, and the operator command. Those are separate tasks in the change. --- fork/manifest.json | 1 + .../opencode/src/peer/claude/sidecar-entry.ts | 51 +++++++ .../peer/claude/sidecar-startup-race.test.ts | 138 ++++++++++++++++++ 3 files changed, 190 insertions(+) create mode 100644 packages/opencode/test/peer/claude/sidecar-startup-race.test.ts 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/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..9299ee32f135 --- /dev/null +++ b/packages/opencode/test/peer/claude/sidecar-startup-race.test.ts @@ -0,0 +1,138 @@ +// 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") + +/** + * Spawns a sidecar whose parent exits immediately, without waiting for the + * sidecar to register — so the child is very likely already reparented by the + * time it evaluates process.ppid. `killAfterMs: 0` makes the race as tight as + * possible. + */ +async function spawnWithParentExitingAt(opts: { killAfterMs: number; stdin: "pipe" | "ignore" }) { + const claudeConfigDir = await mkdtemp(join(tmpdir(), "sidecar-race-claude-")) + await Bun.write(join(claudeConfigDir, "placeholder"), "") + const socketDir = await mkdtemp(join(tmpdir(), "sidecar-race-sock-")) + const fakeParent = join(await mkdtemp(join(tmpdir(), "sidecar-race-parent-")), "spawn-and-exit.ts") + + await Bun.write( + fakeParent, + ` + import { spawn } from "child_process" + const child = spawn("bun", ["run", ${JSON.stringify(ENTRY)}], { + env: { ...process.env }, + stdio: [${JSON.stringify(opts.stdin)}, "pipe", "ignore"], + }) + child.stdout.on("data", () => {}) + // Expire without signalling: no SIGTERM to the sidecar, so nothing but the + // sidecar's own detection can end it. + setTimeout(() => process.exit(0), ${opts.killAfterMs}) + `, + ) + + const proc = spawn("bun", ["run", fakeParent], { + env: { + ...process.env, + CLAUDE_CONFIG_DIR: claudeConfigDir, + OPENCODE_SIDECAR_OWNER_SESSION_ID: "ses_race_test", + OPENCODE_SIDECAR_CWD: "/repo", + OPENCODE_SIDECAR_NAME: "opencode-race-test", + OPENCODE_SIDECAR_SOCKET_DIR: socketDir, + }, + stdio: "ignore", + }) + await new Promise((resolve) => proc.once("exit", () => resolve())) + return { claudeConfigDir, socketDir } +} + +describe("sidecar startup race", () => { + test("a sidecar whose parent exits before its first ppid read still terminates", async () => { + // Timing-dependent by nature: to hit the race the parent must die before the + // child reads its own ppid, and bun's own startup time decides whether that + // happened. So this retries, and — crucially — only concludes when it has + // actually OBSERVED the sidecar register and then clean up. An attempt where + // the sidecar never registered proves nothing and is retried, so this cannot + // pass by never exercising the race at all. + const ATTEMPTS = 4 + let sawRegistration = false + let leaked = false + + for (let attempt = 0; attempt < ATTEMPTS && !leaked; attempt++) { + const { claudeConfigDir } = await spawnWithParentExitingAt({ killAfterMs: 0, stdin: "pipe" }) + const sessionsDir = join(claudeConfigDir, "sessions") + const appeared = Date.now() + 8_000 + let registered = false + while (Date.now() < appeared && !registered) { + const entries = await readdir(sessionsDir).catch(() => []) + registered = entries.some((e) => e.endsWith(".json")) + if (!registered) await new Promise((r) => setTimeout(r, 100)) + } + if (!registered) { + await rm(claudeConfigDir, { recursive: true, force: true }) + continue // inconclusive: the race window was not reached this time + } + sawRegistration = true + + const gone = Date.now() + 12_000 + let stillThere = true + while (Date.now() < gone && stillThere) { + const entries = await readdir(sessionsDir).catch(() => []) + stillThere = entries.some((e) => e.endsWith(".json")) + if (stillThere) await new Promise((r) => setTimeout(r, 150)) + } + leaked = stillThere + await rm(claudeConfigDir, { recursive: true, force: true }) + } + + // Guard the guard: if no attempt ever registered, the test proved nothing. + expect(sawRegistration).toBe(true) + expect(leaked).toBe(false) + }, 120_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 From fc2bbed6f2db6dff0998c39d6f6987e368d5fe50 Mon Sep 17 00:00:00 2001 From: AndreasS Date: Sat, 3 Oct 2026 06:18:13 +0200 Subject: [PATCH 3/4] docs(sidecar): record the confirmed mechanism and the landed fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two peer sessions reproduced the race independently. The second narrowed the window in a way that corrects this document: it is the duration of startSidecar, not an abstract read of process.ppid. Registration is written from inside startSidecar (sidecar-server.ts:156) before originalPpid is captured (sidecar-entry.ts:93), which splits the window in two — alive-and-unregistered before the write, alive-and-registered after it. Both are real; the sweep skips both. All 44 observed were the first. Also records that the discriminator is isSocket() and not isFIFO(), because an isFIFO check reads as correct, matches nothing in production, and would have left the guard dead. That was caught by the failing test rather than by review, which is the only reason it is written down here. Phase 1 is done in 7efff559d0; Phase 0's reproduction and registration questions are answered. Reclamation of already-leaked sidecars is untouched. --- .../sidecar-orphan-cleanup/proposal.md | 29 +++++++++++++++++++ .../changes/sidecar-orphan-cleanup/tasks.md | 18 ++++++++++-- 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/openspec/changes/sidecar-orphan-cleanup/proposal.md b/openspec/changes/sidecar-orphan-cleanup/proposal.md index 3435d8396ee4..5dc3d4bd4976 100644 --- a/openspec/changes/sidecar-orphan-cleanup/proposal.md +++ b/openspec/changes/sidecar-orphan-cleanup/proposal.md @@ -57,6 +57,35 @@ already gone when the sidecar begins running. No test covers either, and the gua 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 diff --git a/openspec/changes/sidecar-orphan-cleanup/tasks.md b/openspec/changes/sidecar-orphan-cleanup/tasks.md index 199d6d01fe1f..7efb2d0636ce 100644 --- a/openspec/changes/sidecar-orphan-cleanup/tasks.md +++ b/openspec/changes/sidecar-orphan-cleanup/tasks.md @@ -2,11 +2,11 @@ ## Phase 0: confirm the mechanism before fixing it -- [ ] 0.1 Reproduce the startup race. Spawn a sidecar from a process that exits immediately, so +- [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. -- [ ] 0.2 Determine how a running sidecar ends up with no registration: parent exit inside the +- [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 @@ -14,7 +14,19 @@ - [ ] 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 +## 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 From e86a6fdf79c4623aa8516f638c9859da9de2e31a Mon Sep 17 00:00:00 2001 From: AndreasS Date: Sat, 3 Oct 2026 19:39:04 +0200 Subject: [PATCH 4/4] test(sidecar): prove the EOF guard fires, not merely that the sidecar exits Reviewer was right, and the criticism was of the same kind I have been making of other checks: the test proved the sidecar terminates, not that the EOF guard terminates it. If the parent dies after the child reads process.ppid, the pre-existing ppid guard kills the sidecar anyway, so removing the EOF guard left the test green. It was a real mutation check on paper and not one in fact. Replaced with a deterministic test. The parent is this test process and 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, so if the sidecar exits and unregisters, the EOF guard is the only possible cause. Verified: with the guard disabled this test fails, and only it fails. Removed the timing-based test. With the fix in place the sidecar now dies so fast after its parent exits that the test could no longer reliably observe the registration it was asserting on, and the 4-attempt retry was passing for the wrong reason or failing for a timing reason. Hitting that race depends on bun's startup time and cannot be made deterministic; the new test isolates the mechanism without depending on it at all. Suite went from 15s and flaky to 1.7s and stable. test/peer 211 pass / 0 fail. Typecheck clean, openspec validate passes, fork:verify 251/251 owned, 117/117 patched. --- .../peer/claude/sidecar-startup-race.test.ts | 126 +++++++----------- 1 file changed, 48 insertions(+), 78 deletions(-) diff --git a/packages/opencode/test/peer/claude/sidecar-startup-race.test.ts b/packages/opencode/test/peer/claude/sidecar-startup-race.test.ts index 9299ee32f135..e1bfff5e8c6c 100644 --- a/packages/opencode/test/peer/claude/sidecar-startup-race.test.ts +++ b/packages/opencode/test/peer/claude/sidecar-startup-race.test.ts @@ -17,91 +17,61 @@ import { spawn } from "child_process" const ENTRY = join(import.meta.dir, "../../../src/peer/claude/sidecar-entry.ts") -/** - * Spawns a sidecar whose parent exits immediately, without waiting for the - * sidecar to register — so the child is very likely already reparented by the - * time it evaluates process.ppid. `killAfterMs: 0` makes the race as tight as - * possible. - */ -async function spawnWithParentExitingAt(opts: { killAfterMs: number; stdin: "pipe" | "ignore" }) { - const claudeConfigDir = await mkdtemp(join(tmpdir(), "sidecar-race-claude-")) - await Bun.write(join(claudeConfigDir, "placeholder"), "") - const socketDir = await mkdtemp(join(tmpdir(), "sidecar-race-sock-")) - const fakeParent = join(await mkdtemp(join(tmpdir(), "sidecar-race-parent-")), "spawn-and-exit.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-")) - await Bun.write( - fakeParent, - ` - import { spawn } from "child_process" - const child = spawn("bun", ["run", ${JSON.stringify(ENTRY)}], { - env: { ...process.env }, - stdio: [${JSON.stringify(opts.stdin)}, "pipe", "ignore"], + 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"], }) - child.stdout.on("data", () => {}) - // Expire without signalling: no SIGTERM to the sidecar, so nothing but the - // sidecar's own detection can end it. - setTimeout(() => process.exit(0), ${opts.killAfterMs}) - `, - ) + let out = "" + child.stdout?.on("data", (c: Buffer) => (out += c.toString("utf8"))) - const proc = spawn("bun", ["run", fakeParent], { - env: { - ...process.env, - CLAUDE_CONFIG_DIR: claudeConfigDir, - OPENCODE_SIDECAR_OWNER_SESSION_ID: "ses_race_test", - OPENCODE_SIDECAR_CWD: "/repo", - OPENCODE_SIDECAR_NAME: "opencode-race-test", - OPENCODE_SIDECAR_SOCKET_DIR: socketDir, - }, - stdio: "ignore", - }) - await new Promise((resolve) => proc.once("exit", () => resolve())) - return { claudeConfigDir, socketDir } -} + 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() -describe("sidecar startup race", () => { - test("a sidecar whose parent exits before its first ppid read still terminates", async () => { - // Timing-dependent by nature: to hit the race the parent must die before the - // child reads its own ppid, and bun's own startup time decides whether that - // happened. So this retries, and — crucially — only concludes when it has - // actually OBSERVED the sidecar register and then clean up. An attempt where - // the sidecar never registered proves nothing and is retried, so this cannot - // pass by never exercising the race at all. - const ATTEMPTS = 4 - let sawRegistration = false - let leaked = false + // 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() - for (let attempt = 0; attempt < ATTEMPTS && !leaked; attempt++) { - const { claudeConfigDir } = await spawnWithParentExitingAt({ killAfterMs: 0, stdin: "pipe" }) - const sessionsDir = join(claudeConfigDir, "sessions") - const appeared = Date.now() + 8_000 - let registered = false - while (Date.now() < appeared && !registered) { - const entries = await readdir(sessionsDir).catch(() => []) - registered = entries.some((e) => e.endsWith(".json")) - if (!registered) await new Promise((r) => setTimeout(r, 100)) - } - if (!registered) { - await rm(claudeConfigDir, { recursive: true, force: true }) - continue // inconclusive: the race window was not reached this time - } - sawRegistration = true + 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() - const gone = Date.now() + 12_000 - let stillThere = true - while (Date.now() < gone && stillThere) { - const entries = await readdir(sessionsDir).catch(() => []) - stillThere = entries.some((e) => e.endsWith(".json")) - if (stillThere) await new Promise((r) => setTimeout(r, 150)) - } - leaked = stillThere - await rm(claudeConfigDir, { recursive: true, force: true }) - } + // 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) - // Guard the guard: if no attempt ever registered, the test proved nothing. - expect(sawRegistration).toBe(true) - expect(leaked).toBe(false) - }, 120_000) + 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