From b30d9565fff750ba1ec13a6bbd855de01533950b Mon Sep 17 00:00:00 2001 From: kavish-19 <63698788+kavish-19@users.noreply.github.com> Date: Thu, 3 Sep 2026 22:38:49 +0530 Subject: [PATCH 1/2] fix(cli): wait for the current turn to finish before an auto-update restart main() schedules checkForUpdates 100ms after spawning the binary. When it finds a newer version it stages the download and then unconditionally SIGTERMs (SIGKILL after 5s) the running process to install it -- with no way to know whether the user is mid-turn, because the wrapper is a separate process that only sees the child's exit event, not its React state. A download that lands a few seconds into a session therefore kills a turn that is still running, which is what #994 reports. Adds a small cross-process signal in the spirit of the existing terminal-watchdog marker files: the binary writes an activity marker for the duration of a turn (subscribed once to the store's isChainInProgress, so every current and future call site is covered) and removes it when idle or on exit. The wrapper waits for that marker to clear before stopping the process for an update. Best-effort and bounded in both directions: a missing marker (already idle, an older binary that predates this file, a process that died without cleaning up) resolves immediately and preserves today's restart-right-away behavior, and a turn that never ends stops blocking the update after 10 minutes. Tests: three for the marker's write/remove/idempotence, three for waitForRunIdle's immediate, waits-then-clears, and gives-up-at-the-bound paths, plus the existing checkForUpdates source-order check extended to require the wait between staging and stopping. Confirmed red against the unfixed code, green after; the full cli suite shows the same 34 pre-existing failures before and after. Claude-Session: https://claude.ai/code/session_018vPhyqaaoKa8cgs7GEnyq5 --- cli/release-core/launcher.js | 40 ++++++++++++ .../__tests__/release/wrapper-safety.test.ts | 65 ++++++++++++++++++- cli/src/index.tsx | 7 ++ .../__tests__/run-activity-marker.test.ts | 56 ++++++++++++++++ cli/src/utils/run-activity-marker.ts | 58 +++++++++++++++++ 5 files changed, 224 insertions(+), 2 deletions(-) create mode 100644 cli/src/utils/__tests__/run-activity-marker.test.ts create mode 100644 cli/src/utils/run-activity-marker.ts diff --git a/cli/release-core/launcher.js b/cli/release-core/launcher.js index 867c1d5d6e..d685f3bc65 100644 --- a/cli/release-core/launcher.js +++ b/cli/release-core/launcher.js @@ -932,6 +932,39 @@ function createLauncher(productConfig) { } } + /** + * Path to the marker the running binary writes (run-activity-marker.ts) + * for the duration of an agent turn. Named by its own pid, which we + * already have from spawning it -- no handshake needed. + */ + function runActivityMarkerPath(pid) { + return path.join(os.tmpdir(), `codebuff-run-active-${pid}`) + } + + const RUN_IDLE_POLL_INTERVAL_MS = 1_000 + // Don't stall an update behind one long-running turn forever; fall back to + // today's immediate-restart behavior once this elapses. + const RUN_IDLE_MAX_WAIT_MS = 10 * 60 * 1000 + + /** + * Wait for the running CLI to finish its current turn before an update + * restarts it. The marker's absence -- already idle, an older binary that + * predates this file, or the process already gone -- resolves + * immediately, preserving the pre-existing restart-right-away behavior. + */ + async function waitForRunIdle(pid, options = {}) { + const { + maxWaitMs = RUN_IDLE_MAX_WAIT_MS, + pollIntervalMs = RUN_IDLE_POLL_INTERVAL_MS, + } = options + const markerPath = runActivityMarkerPath(pid) + const deadline = Date.now() + maxWaitMs + while (fs.existsSync(markerPath)) { + if (Date.now() >= deadline) return + await new Promise((resolve) => setTimeout(resolve, pollIntervalMs)) + } + } + function stopRunningProcess(runningProcess) { return new Promise((resolve, reject) => { let forceKillTimer @@ -1000,6 +1033,11 @@ function createLauncher(productConfig) { { quiet: true }, ) + // Don't interrupt a turn that's still running: wait for the binary + // to clear its activity marker (or the bounded wait to elapse) + // before stopping it for the update. + await waitForRunIdle(runningProcess.pid) + term.clearLine() runningProcess.removeListener('exit', exitListener) @@ -1461,6 +1499,8 @@ function createLauncher(productConfig) { getRequiredWrapperVersion, ensureBinaryReady, isTargetAllowedForThisMachine, + runActivityMarkerPath, + waitForRunIdle, CONFIG, }, } diff --git a/cli/src/__tests__/release/wrapper-safety.test.ts b/cli/src/__tests__/release/wrapper-safety.test.ts index aeb0d60c04..ead5ab57df 100644 --- a/cli/src/__tests__/release/wrapper-safety.test.ts +++ b/cli/src/__tests__/release/wrapper-safety.test.ts @@ -3,6 +3,7 @@ import { EventEmitter } from 'node:events' import { createServer } from 'node:http' import { copyFileSync, + existsSync, mkdirSync, mkdtempSync, readFileSync, @@ -166,7 +167,7 @@ describe('shared release launcher safety', () => { const launcherPath = join(repoRoot, 'cli/release-core/launcher.js') const { createLauncher } = require(launcherPath) - test('stages an update before stopping the running process', () => { + test('stages an update, waits for the run to go idle, then stops the running process', () => { const source = readFileSync(launcherPath, 'utf8') const updateFunction = source.slice( source.indexOf('async function checkForUpdates'), @@ -174,6 +175,7 @@ describe('shared release launcher safety', () => { const stageIndex = updateFunction.indexOf( 'const stagedBinary = await stageBinary', ) + const waitIndex = updateFunction.indexOf('await waitForRunIdle(') const stopIndex = updateFunction.indexOf( 'await stopRunningProcess(runningProcess)', ) @@ -182,10 +184,69 @@ describe('shared release launcher safety', () => { ) expect(stageIndex).toBeGreaterThan(-1) - expect(stopIndex).toBeGreaterThan(stageIndex) + expect(waitIndex).toBeGreaterThan(stageIndex) + expect(stopIndex).toBeGreaterThan(waitIndex) expect(installIndex).toBeGreaterThan(stopIndex) }) + test('waitForRunIdle resolves immediately when no activity marker exists', async () => { + const { waitForRunIdle, runActivityMarkerPath } = createLauncher({ + packageName: 'test', + displayName: 'Test', + }).__testing + const pid = 999_999_001 + + rmSync(runActivityMarkerPath(pid), { force: true }) + + const start = Date.now() + await waitForRunIdle(pid, { maxWaitMs: 5_000, pollIntervalMs: 5_000 }) + + // No poll tick should have been needed at all. + expect(Date.now() - start).toBeLessThan(500) + }) + + test('waitForRunIdle waits while the run is active and returns once it clears', async () => { + const { waitForRunIdle, runActivityMarkerPath } = createLauncher({ + packageName: 'test', + displayName: 'Test', + }).__testing + const pid = 999_999_002 + const markerPath = runActivityMarkerPath(pid) + + writeFileSync(markerPath, '') + setTimeout(() => rmSync(markerPath, { force: true }), 30) + + try { + await waitForRunIdle(pid, { maxWaitMs: 2_000, pollIntervalMs: 10 }) + expect(existsSync(markerPath)).toBe(false) + } finally { + rmSync(markerPath, { force: true }) + } + }) + + test('waitForRunIdle gives up once maxWaitMs elapses, marker or not', async () => { + const { waitForRunIdle, runActivityMarkerPath } = createLauncher({ + packageName: 'test', + displayName: 'Test', + }).__testing + const pid = 999_999_003 + const markerPath = runActivityMarkerPath(pid) + + // Never cleared during the wait: the bound must still return. + writeFileSync(markerPath, '') + + try { + const start = Date.now() + await waitForRunIdle(pid, { maxWaitMs: 30, pollIntervalMs: 10 }) + expect(Date.now() - start).toBeLessThan(1_000) + // The marker itself is untouched -- the wrapper gives up waiting, it + // doesn't force the run to look idle. + expect(existsSync(markerPath)).toBe(true) + } finally { + rmSync(markerPath, { force: true }) + } + }) + test('requires the wrapper release only for missing or older binaries', () => { const cases: Array<{ wrapperVersion: string diff --git a/cli/src/index.tsx b/cli/src/index.tsx index cae4e380eb..528bfcdead 100644 --- a/cli/src/index.tsx +++ b/cli/src/index.tsx @@ -45,6 +45,7 @@ import { exitCliWithFatalError, installProcessCleanupHandlers, } from './utils/renderer-cleanup' +import { startRunActivityMarker } from './utils/run-activity-marker' import { startTerminalWatchdog } from './utils/terminal-watchdog' import { installTerminalProtocolController } from './utils/terminal-protocol-controller' import { initializeSkillRegistry } from './utils/skill-registry' @@ -401,6 +402,12 @@ async function main(): Promise { // modes; the clean-shutdown path (renderer-cleanup) disarms it. startTerminalWatchdog() + // Lets the npm-wrapper launcher defer an auto-update restart until any + // in-progress turn finishes, instead of interrupting it. Started early so + // no isChainInProgress transition can slip by before the subscription + // exists. + startRunActivityMarker() + const renderer = await createCliRenderer({ backgroundColor: 'transparent', exitOnCtrlC: false, diff --git a/cli/src/utils/__tests__/run-activity-marker.test.ts b/cli/src/utils/__tests__/run-activity-marker.test.ts new file mode 100644 index 0000000000..538001c7e9 --- /dev/null +++ b/cli/src/utils/__tests__/run-activity-marker.test.ts @@ -0,0 +1,56 @@ +import { existsSync, rmSync } from 'fs' + +import { afterAll, beforeEach, describe, expect, test } from 'bun:test' + +import { useChatStore } from '../../state/chat-store' +import { + runActivityMarkerPath, + startRunActivityMarker, +} from '../run-activity-marker' + +describe('run-activity-marker', () => { + const markerPath = runActivityMarkerPath() + + beforeEach(() => { + rmSync(markerPath, { force: true }) + useChatStore.getState().setIsChainInProgress(false) + }) + + afterAll(() => { + rmSync(markerPath, { force: true }) + useChatStore.getState().setIsChainInProgress(false) + }) + + test('writes the marker while a turn is in progress and removes it when idle', () => { + startRunActivityMarker() + + expect(existsSync(markerPath)).toBe(false) + + useChatStore.getState().setIsChainInProgress(true) + expect(existsSync(markerPath)).toBe(true) + + useChatStore.getState().setIsChainInProgress(false) + expect(existsSync(markerPath)).toBe(false) + }) + + test('is a no-op when the value does not actually change', () => { + startRunActivityMarker() + useChatStore.getState().setIsChainInProgress(true) + rmSync(markerPath, { force: true }) + + // Re-affirming the same value must not recreate the marker: only a real + // active/idle transition should. + useChatStore.getState().setIsChainInProgress(true) + expect(existsSync(markerPath)).toBe(false) + }) + + test('registering more than once never stacks a duplicate exit handler', () => { + startRunActivityMarker() + const countAfterFirstStart = process.listenerCount('exit') + + startRunActivityMarker() + startRunActivityMarker() + + expect(process.listenerCount('exit')).toBe(countAfterFirstStart) + }) +}) diff --git a/cli/src/utils/run-activity-marker.ts b/cli/src/utils/run-activity-marker.ts new file mode 100644 index 0000000000..abf584ae53 --- /dev/null +++ b/cli/src/utils/run-activity-marker.ts @@ -0,0 +1,58 @@ +/** + * Cross-process "is a turn running" signal for the npm-wrapper launcher. + * + * The wrapper (release-core/launcher.js) checks for updates ~100ms after + * spawning this process and, on finding one, force-restarts it -- with no + * way to know whether the user is mid-turn, because it only has this + * process's exit event, not its React state. While this marker file exists, + * an agent turn is in progress; the wrapper waits for it to clear (bounded) + * before stopping the process for an update, instead of interrupting a + * turn that's still running. + * + * Named by this process's pid, which the wrapper already has from spawning + * it -- no handshake needed. Best-effort throughout: a failed write or + * remove just means the wrapper falls back to today's immediate-restart + * behavior for this session. + */ +import { rmSync, writeFileSync } from 'fs' +import os from 'os' +import path from 'path' + +import { useChatStore } from '../state/chat-store' + +export function runActivityMarkerPath(pid: number = process.pid): string { + return path.join(os.tmpdir(), `codebuff-run-active-${pid}`) +} + +let started = false + +/** Call once, before the store can start toggling isChainInProgress. */ +export function startRunActivityMarker(): void { + if (started) return + started = true + + const filePath = runActivityMarkerPath() + + const clear = () => { + try { + rmSync(filePath, { force: true }) + } catch { + // Best-effort; see module doc. + } + } + + useChatStore.subscribe((state, prevState) => { + if (state.isChainInProgress === prevState.isChainInProgress) return + if (state.isChainInProgress) { + try { + writeFileSync(filePath, '', { flag: 'w' }) + } catch { + // Best-effort; see module doc. + } + } else { + clear() + } + }) + + process.on('exit', clear) +} From 39404119b95946778459e1365eee626d8768b555 Mon Sep 17 00:00:00 2001 From: kavish-19 <63698788+kavish-19@users.noreply.github.com> Date: Fri, 4 Sep 2026 22:56:13 +0530 Subject: [PATCH 2/2] fix(cli): clear a stale run-activity marker when spawning the binary Review feedback on #1258: if the binary dies via SIGKILL or a native crash, `process.on('exit', clear)` never runs and the marker outlives it in tmpdir. Reach that pid again and the leaked file stalls the new run's updates for the whole RUN_IDLE_MAX_WAIT_MS bound. Clear it at spawn rather than stamping the marker with an identity to cross-check. At the moment spawnInstalledBinary has the child's pid, the binary has not booted, let alone started a turn -- so a marker at that path is definitionally someone else's, and no session id is needed to tell the two apart. Claude-Session: https://claude.ai/code/session_018vPhyqaaoKa8cgs7GEnyq5 --- cli/release-core/launcher.js | 19 +++++++ .../__tests__/release/wrapper-safety.test.ts | 51 +++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/cli/release-core/launcher.js b/cli/release-core/launcher.js index d685f3bc65..58576839f0 100644 --- a/cli/release-core/launcher.js +++ b/cli/release-core/launcher.js @@ -941,6 +941,22 @@ function createLauncher(productConfig) { return path.join(os.tmpdir(), `codebuff-run-active-${pid}`) } + /** + * Drop a marker left behind by a process that died before it could clear + * its own -- SIGKILL, a native crash -- since its exit handler never ran. + * Called with a pid we have only just spawned, so any marker at that path + * belongs to an earlier process the OS has since reused the pid for; the + * binary cannot have started a turn yet. Without this, that stale file + * stalls the new run's updates for the whole RUN_IDLE_MAX_WAIT_MS bound. + */ + function clearStaleRunActivityMarker(pid) { + try { + fs.rmSync(runActivityMarkerPath(pid), { force: true }) + } catch { + // Best effort: a marker we can't remove only costs us the bounded wait. + } + } + const RUN_IDLE_POLL_INTERVAL_MS = 1_000 // Don't stall an update behind one long-running turn forever; fall back to // today's immediate-restart behavior once this elapses. @@ -1305,6 +1321,8 @@ function createLauncher(productConfig) { child.on('error', exitOnSpawnFailure) child.launch = watchLaunch(child) + if (child.pid !== undefined) clearStaleRunActivityMarker(child.pid) + return child } @@ -1500,6 +1518,7 @@ function createLauncher(productConfig) { ensureBinaryReady, isTargetAllowedForThisMachine, runActivityMarkerPath, + clearStaleRunActivityMarker, waitForRunIdle, CONFIG, }, diff --git a/cli/src/__tests__/release/wrapper-safety.test.ts b/cli/src/__tests__/release/wrapper-safety.test.ts index ead5ab57df..fb75431132 100644 --- a/cli/src/__tests__/release/wrapper-safety.test.ts +++ b/cli/src/__tests__/release/wrapper-safety.test.ts @@ -247,6 +247,57 @@ describe('shared release launcher safety', () => { } }) + test('a spawned binary starts from a clean activity marker', () => { + const { clearStaleRunActivityMarker, runActivityMarkerPath } = + createLauncher({ + packageName: 'test', + displayName: 'Test', + }).__testing + const pid = 999_999_004 + const markerPath = runActivityMarkerPath(pid) + + // A marker left by a process that died before it could clear its own -- + // SIGKILL, a native crash -- outlives it in tmpdir. Reaching the same pid + // again would otherwise stall that run's updates for the full bound. + writeFileSync(markerPath, '') + + try { + clearStaleRunActivityMarker(pid) + expect(existsSync(markerPath)).toBe(false) + } finally { + rmSync(markerPath, { force: true }) + } + }) + + test('clearing a stale marker tolerates there being none', () => { + const { clearStaleRunActivityMarker, runActivityMarkerPath } = + createLauncher({ + packageName: 'test', + displayName: 'Test', + }).__testing + const pid = 999_999_005 + + rmSync(runActivityMarkerPath(pid), { force: true }) + + expect(() => clearStaleRunActivityMarker(pid)).not.toThrow() + }) + + test('spawnInstalledBinary clears the stale marker once it has a pid', () => { + const source = readFileSync(launcherPath, 'utf8') + const spawnFunction = source.slice( + source.indexOf('function spawnInstalledBinary'), + ) + const spawnIndex = spawnFunction.indexOf('child = spawn(CONFIG.binaryPath') + const clearIndex = spawnFunction.indexOf('clearStaleRunActivityMarker(') + const returnIndex = spawnFunction.indexOf('return child') + + expect(spawnIndex).toBeGreaterThan(-1) + // The pid only exists after the spawn, and the marker must be gone before + // the caller can hand this child to checkForUpdates. + expect(clearIndex).toBeGreaterThan(spawnIndex) + expect(returnIndex).toBeGreaterThan(clearIndex) + }) + test('requires the wrapper release only for missing or older binaries', () => { const cases: Array<{ wrapperVersion: string