don't let a superseded turn finish somebody else's session
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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<string, RunHandle>();
|
||||
@@ -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<typeof setTimeout> | 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;
|
||||
|
||||
Reference in New Issue
Block a user