diff --git a/cli/release-core/launcher.js b/cli/release-core/launcher.js index 867c1d5d6e..58576839f0 100644 --- a/cli/release-core/launcher.js +++ b/cli/release-core/launcher.js @@ -932,6 +932,55 @@ 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}`) + } + + /** + * 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. + 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 +1049,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) @@ -1267,6 +1321,8 @@ function createLauncher(productConfig) { child.on('error', exitOnSpawnFailure) child.launch = watchLaunch(child) + if (child.pid !== undefined) clearStaleRunActivityMarker(child.pid) + return child } @@ -1461,6 +1517,9 @@ function createLauncher(productConfig) { getRequiredWrapperVersion, 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 aeb0d60c04..fb75431132 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,120 @@ 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('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 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) +}