From ce405a4cff7076b94c210195e76037ab64f37c43 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Tue, 18 Aug 2026 10:59:44 -0400 Subject: [PATCH] fix(respawn): do not revive a stopped controller after a cycle-step write MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each cycle step (kickstart, update, /clear, /init) checks for `stopped` before `await session.writeViaMux(...)`, then emits `stepSent` and calls `setState('waiting_*')` after it. stop() is asynchronous with respect to that await. One that lands while the write is in flight has already passed the guard that ran, so the post-await setState() puts a stopped controller back into a waiting state — re-arming its step timers against a session the user asked to stop. Re-check after the await, before emitting and setting state. The guard reads the public `state` getter rather than `_state` on purpose: TypeScript narrows `_state` across the await from the pre-await check and cannot see that stop() mutated it, so `this._state === 'stopped'` is rejected as a comparison with no overlap (TS2367) at all four sites. Adds test/respawn-stop-race.test.ts, which drives the interleaving deterministically by calling stop() from inside the mocked write rather than relying on timing. All four steps go red without these guards. --- src/respawn-controller.ts | 20 ++++++++ test/respawn-stop-race.test.ts | 86 ++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 test/respawn-stop-race.test.ts diff --git a/src/respawn-controller.ts b/src/respawn-controller.ts index 2ac81696..ef5d84f8 100644 --- a/src/respawn-controller.ts +++ b/src/respawn-controller.ts @@ -1624,6 +1624,11 @@ export class RespawnController extends EventEmitter { const prompt = this.config.kickstartPrompt!; this.logAction('command', `Sending kickstart: "${prompt.substring(0, 40)}..."`); await this.session.writeViaMux(prompt + '\r'); // \r triggers key.return in Ink/Claude CLI + // COD-51: stop() may have run during the await; re-check before reviving the + // state machine. Reads the public getter, not `_state`: TypeScript narrows + // `_state` across the await from the guard above and cannot see that stop() + // mutated it, so the comparison would be flagged as impossible. + if (this.state === 'stopped') return; this.emit('stepSent', 'kickstart', prompt); this.setState('waiting_kickstart'); this.promptDetected = false; @@ -2833,6 +2838,11 @@ export class RespawnController extends EventEmitter { const input = updatePrompt + '\r'; // \r triggers Enter in Ink/Claude CLI this.logAction('command', `Sending: "${updatePrompt.substring(0, 50)}..."`); await this.session.writeViaMux(input); + // COD-51: stop() may have run during the await; re-check before reviving the + // state machine. Reads the public getter, not `_state`: TypeScript narrows + // `_state` across the await from the guard above and cannot see that stop() + // mutated it, so the comparison would be flagged as impossible. + if (this.state === 'stopped') return; this.emit('stepSent', 'update', updatePrompt); this.setState('waiting_update'); this.promptDetected = false; @@ -2860,6 +2870,11 @@ export class RespawnController extends EventEmitter { if (this._state === 'stopped') return; this.logAction('command', 'Sending: /clear'); await this.session.writeViaMux('/clear\r'); // \r triggers Enter in Ink/Claude CLI + // COD-51: stop() may have run during the await; re-check before reviving the + // state machine. Reads the public getter, not `_state`: TypeScript narrows + // `_state` across the await from the guard above and cannot see that stop() + // mutated it, so the comparison would be flagged as impossible. + if (this.state === 'stopped') return; this.emit('stepSent', 'clear', '/clear'); this.setState('waiting_clear'); this.promptDetected = false; @@ -2902,6 +2917,11 @@ export class RespawnController extends EventEmitter { if (this._state === 'stopped') return; this.logAction('command', 'Sending: /init'); await this.session.writeViaMux('/init\r'); // \r triggers Enter in Ink/Claude CLI + // COD-51: stop() may have run during the await; re-check before reviving the + // state machine. Reads the public getter, not `_state`: TypeScript narrows + // `_state` across the await from the guard above and cannot see that stop() + // mutated it, so the comparison would be flagged as impossible. + if (this.state === 'stopped') return; this.emit('stepSent', 'init', '/init'); this.setState('waiting_init'); this.promptDetected = false; diff --git a/test/respawn-stop-race.test.ts b/test/respawn-stop-race.test.ts new file mode 100644 index 00000000..1885ef4b --- /dev/null +++ b/test/respawn-stop-race.test.ts @@ -0,0 +1,86 @@ +/** + * COD-51: stop() landing DURING a cycle step's write must not revive the machine. + * + * Each cycle step (kickstart, update, /clear, /init) guards on `stopped` before + * `await session.writeViaMux(...)`, then emits `stepSent` and calls + * `setState('waiting_*')` after it. `stop()` is asynchronous with respect to + * that await: a stop that lands while the write is in flight passed the guard + * that already ran, so the post-await `setState()` puts a stopped controller + * back into a waiting state, re-arming its timers against a session the user + * asked to stop. + * + * The race is driven deterministically here by stopping from inside the mocked + * write itself — that is exactly the interleaving, without leaning on timing. + */ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +vi.mock('node:child_process', async (orig) => { + const actual = await orig(); + return { ...actual, exec: vi.fn((_cmd: string, cb?: (e: Error | null, o: string) => void) => cb?.(null, '')) }; +}); + +import { RespawnController } from '../src/respawn-controller.js'; +import { Session } from '../src/session.js'; +import { MockSession } from './mocks/index.js'; + +describe('COD-51 respawn stop() race', () => { + let session: MockSession; + let controller: RespawnController; + + beforeEach(() => { + session = new MockSession(); + controller = new RespawnController(session as unknown as Session, { + idleTimeoutMs: 100, + interStepDelayMs: 10, + completionConfirmMs: 10, + noOutputTimeoutMs: 500, + aiIdleCheckEnabled: false, + // sendKickstart dereferences this before it reaches the write. + kickstartPrompt: 'continue', + }); + }); + + afterEach(() => controller.stop()); + + /** + * Run `step`, stopping the controller from inside the write it awaits. + * + * The step methods are SYNCHRONOUS: they set `sending_*` and schedule a + * `step-delay` timer, and the write happens inside that callback. So the race + * only exists once the timer has fired — hence the settle below rather than a + * bare `await step()`, which returns before anything interesting happens. + */ + async function stopDuringWrite(step: () => void) { + const stepSent = vi.fn(); + controller.on('stepSent', stepSent); + const wrote = new Promise((resolve) => { + vi.spyOn(session, 'writeViaMux').mockImplementation(async () => { + controller.stop(); + resolve(); + return true; + }); + }); + step(); + await wrote; + // Let the continuation after the await run before asserting on it. + await new Promise((r) => setTimeout(r, 20)); + return stepSent; + } + + it.each([ + ['update', () => (controller as never as { sendUpdateDocs(): void }).sendUpdateDocs()], + ['clear', () => (controller as never as { sendClear(): void }).sendClear()], + ['init', () => (controller as never as { sendInit(): void }).sendInit()], + ['kickstart', () => (controller as never as { sendKickstart(): void }).sendKickstart()], + ])('a stop during the %s write leaves the controller stopped', async (_label, step) => { + (controller as never as { _state: string })._state = 'watching'; + + const stepSent = await stopDuringWrite(step); + + // The observable damage is a revived state machine: `waiting_*` re-arms the + // step timers, so the cycle keeps driving a session the user stopped. + expect(controller.state).toBe('stopped'); + expect(controller.isRunning).toBe(false); + expect(stepSent).not.toHaveBeenCalled(); + }); +});