From c252f24f1d61d0efa83b883f71efa3c4123c2b6d Mon Sep 17 00:00:00 2001 From: Andre Padez Date: Mon, 10 Aug 2026 12:50:26 +0100 Subject: [PATCH] don't let a superseded turn finish somebody else's session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A replaced turn is killed but dies asynchronously, so its proc.exited fired long after the replacement was registered under the same sessionKey — and then ran the whole completion path against it: emitted "OpenCode exited with code 143", which the sidecar commits to chat_session_events so a false failure became permanent history, then deleted the replacement from `running`. That blinded the new Live panel, made the stop button a no-op and orphaned a process nothing could reach. Mark the handle before killing it, retire it silently, and identity-check the delete — a superseded turn does not own that key any more. Two leaks in the same family, found while fixing it. An early return would not have been enough: both watchdogs call finish, so the armed 10-minute hardTimer would have fired an error at whichever turn held the key by then. And handleLine had no `done` guard, so stdout still draining from the killed process was emitted under the replacement key. Reproduced before fixing. The lifecycle tests need no real opencode — RunnerConfig.bin takes a shell script that sleeps. The control test pins that an ordinary non-zero exit still reports an error, so the guard cannot overreach. Co-Authored-By: Claude Opus 5 --- src/servers/sidecar/opencode/runner.test.ts | 77 ++++++++++++++++++++- src/servers/sidecar/opencode/runner.ts | 50 +++++++++++-- 2 files changed, 119 insertions(+), 8 deletions(-) diff --git a/src/servers/sidecar/opencode/runner.test.ts b/src/servers/sidecar/opencode/runner.test.ts index 6627d229..d9045566 100644 --- a/src/servers/sidecar/opencode/runner.test.ts +++ b/src/servers/sidecar/opencode/runner.test.ts @@ -1,5 +1,9 @@ -import { describe, expect, it } from 'bun:test'; -import { mapRunLine } from './runner'; +import { afterAll, describe, expect, it } from 'bun:test'; +import { mkdtempSync, writeFileSync, chmodSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import type { RunnerMessage } from './runner'; +import { killOpenCodeTurn, listRunningOpenCodeTurns, mapRunLine, runOpenCodeTurn } from './runner'; // The first tests on the OpenCode path, which had none. // @@ -109,3 +113,72 @@ describe('mapRunLine — session id', () => { expect(mapRunLine(JSON.stringify({ type: 'text', part: { text: 'hi' } }))?.sessionId).toBeUndefined(); }); }); + +// ── The turn lifecycle, which `mapRunLine`'s extraction deliberately left with the caller ── +// +// Emitting the session id once and accumulating cost were kept out of the pure mapper because they span +// lines. That was right, and it left them as the only untested logic on this path — which is exactly +// where the supersede defect lived: a replaced turn's `proc.exited` fired late and ran `finish()` against +// the turn that had replaced it. +// +// No real `opencode` needed. `RunnerConfig.bin` is the only injection point, so a shell script that +// outlives the test stands in for a turn that is still generating. + +const stubDir = mkdtempSync(join(tmpdir(), 'oc-runner-test-')); + +/** Stands in for a turn that is still generating. */ +const STUB_BIN = join(stubDir, 'fake-opencode'); +writeFileSync(STUB_BIN, '#!/bin/sh\nsleep 30\n'); +chmodSync(STUB_BIN, 0o755); + +/** Stands in for a turn that fails on its own. A script, not `/bin/false` — that is `/usr/bin/false` on macOS. */ +const FAILING_BIN = join(stubDir, 'failing-opencode'); +writeFileSync(FAILING_BIN, '#!/bin/sh\necho "boom" >&2\nexit 1\n'); +chmodSync(FAILING_BIN, 0o755); + +const CONFIG = { bin: STUB_BIN, fallbackCwd: stubDir }; + +afterAll(() => { + rmSync(stubDir, { recursive: true, force: true }); +}); + +describe('runOpenCodeTurn — a second turn on a live session', () => { + it('lets the replacement keep the session: no error, still listed, still killable', async () => { + const sessionKey = 'sess-supersede'; + const messages: RunnerMessage[] = []; + const emit = (msg: RunnerMessage) => messages.push(msg); + + runOpenCodeTurn({ sessionKey, prompt: 'first', cwd: stubDir }, CONFIG, emit); + runOpenCodeTurn({ sessionKey, prompt: 'second', cwd: stubDir }, CONFIG, emit); + + // Let the superseded child actually die. Its exit handler is what used to reach across. + await Bun.sleep(750); + + // 1. Nothing is emitted for a turn the system replaced on purpose. This one mattered most: the emit + // is committed to chat_session_events by the sidecar, so a false "OpenCode exited" became history. + expect(messages.filter((m) => m.type === 'opencode:event' && m.event.type === 'error')).toEqual([]); + + // 2. The replacement is still enumerable — the Live panel's whole purpose. + expect(listRunningOpenCodeTurns()).toContainEqual({ sessionKey }); + + // 3. And still reachable by the stop button, rather than orphaned with no handle. + killOpenCodeTurn(sessionKey); + await Bun.sleep(250); + expect(listRunningOpenCodeTurns()).not.toContainEqual({ sessionKey }); + expect(messages.some((m) => m.type === 'opencode:event' && m.event.type === 'stopped')).toBe(true); + }); + + it('still reports a turn that dies on its own, rather than swallowing every exit', async () => { + // The guard must not overreach: an ordinary failure is still an error the user needs to see. + const sessionKey = 'sess-solo'; + const messages: RunnerMessage[] = []; + + runOpenCodeTurn({ sessionKey, prompt: 'only', cwd: stubDir }, { ...CONFIG, bin: FAILING_BIN }, (m) => + messages.push(m), + ); + await Bun.sleep(750); + + expect(messages.some((m) => m.type === 'opencode:event' && m.event.type === 'error')).toBe(true); + expect(listRunningOpenCodeTurns()).not.toContainEqual({ sessionKey }); + }); +}); diff --git a/src/servers/sidecar/opencode/runner.ts b/src/servers/sidecar/opencode/runner.ts index d2ea5a25..1ec9a881 100644 --- a/src/servers/sidecar/opencode/runner.ts +++ b/src/servers/sidecar/opencode/runner.ts @@ -30,7 +30,21 @@ export type RunnerMessage = type Emit = (msg: RunnerMessage) => void; -type RunHandle = { proc: Subprocess; killedByUser: boolean }; +type RunHandle = { + proc: Subprocess; + killedByUser: boolean; + /** + * Set when a newer turn has taken this sessionKey over. + * + * A killed process dies asynchronously, so a replaced turn's `proc.exited` fires LONG after its + * replacement is already running and registered under the same key. Without this flag that late + * handler ran the full completion path against the wrong turn: it emitted `OpenCode exited with code + * 143` — which the sidecar commits to `chat_session_events`, so a false failure became permanent + * history — and then deleted its replacement from `running`, which blinded the Live panel, made the + * stop button a no-op, and orphaned a process nothing could reach. + */ + superseded: boolean; +}; // One turn per sessionKey; a new turn supersedes any stale process for that key. const running = new Map(); @@ -55,9 +69,11 @@ type RunEvent = { type?: string; sessionID?: string; part?: RunPart }; export function runOpenCodeTurn(params: OpenCodeRunParams, config: RunnerConfig, emit: Emit): void { const { sessionKey } = params; - // Supersede any lingering turn for this session. + // Supersede any lingering turn for this session. Mark it BEFORE killing: the flag is what tells its + // own exit handler that this death was intentional and belongs to nobody. const stale = running.get(sessionKey); if (stale) { + stale.superseded = true; try { stale.proc.kill(); } catch { @@ -81,7 +97,7 @@ export function runOpenCodeTurn(params: OpenCodeRunParams, config: RunnerConfig, stderr: 'pipe', }); - const handle: RunHandle = { proc, killedByUser: false }; + const handle: RunHandle = { proc, killedByUser: false, superseded: false }; running.set(sessionKey, handle); let done = false; @@ -92,15 +108,27 @@ export function runOpenCodeTurn(params: OpenCodeRunParams, config: RunnerConfig, const emitEvent = (event: ChatEvent) => emit({ type: 'opencode:event', sessionKey, event }); let inactivityTimer: ReturnType | undefined; - const finish = (event: ChatEvent) => { + + /** + * Retire this turn: stop its watchdogs, release its slot, and optionally say why it ended. + * + * The delete is identity-checked because `sessionKey` is not this turn's to own once it has been + * superseded — the map may already hold a live replacement under that key, and deleting by name alone + * removed it. `null` retires silently, which is what a superseded turn needs: it must still clear its + * timers (an armed 10-minute `hardTimer` would otherwise fire an error at whichever turn holds the key + * by then, reproducing the same cross-talk on a delay) while emitting nothing at all. + */ + const settle = (event: ChatEvent | null) => { if (done) return; done = true; clearTimeout(hardTimer); if (inactivityTimer) clearTimeout(inactivityTimer); - running.delete(sessionKey); - emitEvent(event); + if (running.get(sessionKey) === handle) running.delete(sessionKey); + if (event) emitEvent(event); }; + const finish = (event: ChatEvent) => settle(event); + // ── Watchdogs ── const hardTimer = setTimeout(() => { try { @@ -163,6 +191,11 @@ export function runOpenCodeTurn(params: OpenCodeRunParams, config: RunnerConfig, })(); function handleLine(line: string): void { + // A retired turn says nothing more. Stdout is drained asynchronously, so a killed process can still + // have buffered lines in flight — and for a superseded turn those would be emitted under a + // sessionKey that now belongs to its replacement, interleaving one turn's output into another's. + if (done) return; + const mapped = mapRunLine(line); if (!mapped) return; @@ -184,6 +217,11 @@ export function runOpenCodeTurn(params: OpenCodeRunParams, config: RunnerConfig, // ── Completion: process exit is the authoritative turn-done signal ── void proc.exited.then((code) => { if (done) return; + // Replaced on purpose: not a result, not an error, and not this turn's session any more. + if (handle.superseded) { + settle(null); + return; + } if (handle.killedByUser) { finish({ type: 'stopped' }); return;