Skip to content

Commit b30d956

Browse files
committed
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
1 parent 4511764 commit b30d956

5 files changed

Lines changed: 224 additions & 2 deletions

File tree

cli/release-core/launcher.js

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -932,6 +932,39 @@ function createLauncher(productConfig) {
932932
}
933933
}
934934

935+
/**
936+
* Path to the marker the running binary writes (run-activity-marker.ts)
937+
* for the duration of an agent turn. Named by its own pid, which we
938+
* already have from spawning it -- no handshake needed.
939+
*/
940+
function runActivityMarkerPath(pid) {
941+
return path.join(os.tmpdir(), `codebuff-run-active-${pid}`)
942+
}
943+
944+
const RUN_IDLE_POLL_INTERVAL_MS = 1_000
945+
// Don't stall an update behind one long-running turn forever; fall back to
946+
// today's immediate-restart behavior once this elapses.
947+
const RUN_IDLE_MAX_WAIT_MS = 10 * 60 * 1000
948+
949+
/**
950+
* Wait for the running CLI to finish its current turn before an update
951+
* restarts it. The marker's absence -- already idle, an older binary that
952+
* predates this file, or the process already gone -- resolves
953+
* immediately, preserving the pre-existing restart-right-away behavior.
954+
*/
955+
async function waitForRunIdle(pid, options = {}) {
956+
const {
957+
maxWaitMs = RUN_IDLE_MAX_WAIT_MS,
958+
pollIntervalMs = RUN_IDLE_POLL_INTERVAL_MS,
959+
} = options
960+
const markerPath = runActivityMarkerPath(pid)
961+
const deadline = Date.now() + maxWaitMs
962+
while (fs.existsSync(markerPath)) {
963+
if (Date.now() >= deadline) return
964+
await new Promise((resolve) => setTimeout(resolve, pollIntervalMs))
965+
}
966+
}
967+
935968
function stopRunningProcess(runningProcess) {
936969
return new Promise((resolve, reject) => {
937970
let forceKillTimer
@@ -1000,6 +1033,11 @@ function createLauncher(productConfig) {
10001033
{ quiet: true },
10011034
)
10021035

1036+
// Don't interrupt a turn that's still running: wait for the binary
1037+
// to clear its activity marker (or the bounded wait to elapse)
1038+
// before stopping it for the update.
1039+
await waitForRunIdle(runningProcess.pid)
1040+
10031041
term.clearLine()
10041042

10051043
runningProcess.removeListener('exit', exitListener)
@@ -1461,6 +1499,8 @@ function createLauncher(productConfig) {
14611499
getRequiredWrapperVersion,
14621500
ensureBinaryReady,
14631501
isTargetAllowedForThisMachine,
1502+
runActivityMarkerPath,
1503+
waitForRunIdle,
14641504
CONFIG,
14651505
},
14661506
}

cli/src/__tests__/release/wrapper-safety.test.ts

Lines changed: 63 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { EventEmitter } from 'node:events'
33
import { createServer } from 'node:http'
44
import {
55
copyFileSync,
6+
existsSync,
67
mkdirSync,
78
mkdtempSync,
89
readFileSync,
@@ -166,14 +167,15 @@ describe('shared release launcher safety', () => {
166167
const launcherPath = join(repoRoot, 'cli/release-core/launcher.js')
167168
const { createLauncher } = require(launcherPath)
168169

169-
test('stages an update before stopping the running process', () => {
170+
test('stages an update, waits for the run to go idle, then stops the running process', () => {
170171
const source = readFileSync(launcherPath, 'utf8')
171172
const updateFunction = source.slice(
172173
source.indexOf('async function checkForUpdates'),
173174
)
174175
const stageIndex = updateFunction.indexOf(
175176
'const stagedBinary = await stageBinary',
176177
)
178+
const waitIndex = updateFunction.indexOf('await waitForRunIdle(')
177179
const stopIndex = updateFunction.indexOf(
178180
'await stopRunningProcess(runningProcess)',
179181
)
@@ -182,10 +184,69 @@ describe('shared release launcher safety', () => {
182184
)
183185

184186
expect(stageIndex).toBeGreaterThan(-1)
185-
expect(stopIndex).toBeGreaterThan(stageIndex)
187+
expect(waitIndex).toBeGreaterThan(stageIndex)
188+
expect(stopIndex).toBeGreaterThan(waitIndex)
186189
expect(installIndex).toBeGreaterThan(stopIndex)
187190
})
188191

192+
test('waitForRunIdle resolves immediately when no activity marker exists', async () => {
193+
const { waitForRunIdle, runActivityMarkerPath } = createLauncher({
194+
packageName: 'test',
195+
displayName: 'Test',
196+
}).__testing
197+
const pid = 999_999_001
198+
199+
rmSync(runActivityMarkerPath(pid), { force: true })
200+
201+
const start = Date.now()
202+
await waitForRunIdle(pid, { maxWaitMs: 5_000, pollIntervalMs: 5_000 })
203+
204+
// No poll tick should have been needed at all.
205+
expect(Date.now() - start).toBeLessThan(500)
206+
})
207+
208+
test('waitForRunIdle waits while the run is active and returns once it clears', async () => {
209+
const { waitForRunIdle, runActivityMarkerPath } = createLauncher({
210+
packageName: 'test',
211+
displayName: 'Test',
212+
}).__testing
213+
const pid = 999_999_002
214+
const markerPath = runActivityMarkerPath(pid)
215+
216+
writeFileSync(markerPath, '')
217+
setTimeout(() => rmSync(markerPath, { force: true }), 30)
218+
219+
try {
220+
await waitForRunIdle(pid, { maxWaitMs: 2_000, pollIntervalMs: 10 })
221+
expect(existsSync(markerPath)).toBe(false)
222+
} finally {
223+
rmSync(markerPath, { force: true })
224+
}
225+
})
226+
227+
test('waitForRunIdle gives up once maxWaitMs elapses, marker or not', async () => {
228+
const { waitForRunIdle, runActivityMarkerPath } = createLauncher({
229+
packageName: 'test',
230+
displayName: 'Test',
231+
}).__testing
232+
const pid = 999_999_003
233+
const markerPath = runActivityMarkerPath(pid)
234+
235+
// Never cleared during the wait: the bound must still return.
236+
writeFileSync(markerPath, '')
237+
238+
try {
239+
const start = Date.now()
240+
await waitForRunIdle(pid, { maxWaitMs: 30, pollIntervalMs: 10 })
241+
expect(Date.now() - start).toBeLessThan(1_000)
242+
// The marker itself is untouched -- the wrapper gives up waiting, it
243+
// doesn't force the run to look idle.
244+
expect(existsSync(markerPath)).toBe(true)
245+
} finally {
246+
rmSync(markerPath, { force: true })
247+
}
248+
})
249+
189250
test('requires the wrapper release only for missing or older binaries', () => {
190251
const cases: Array<{
191252
wrapperVersion: string

cli/src/index.tsx

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ import {
4545
exitCliWithFatalError,
4646
installProcessCleanupHandlers,
4747
} from './utils/renderer-cleanup'
48+
import { startRunActivityMarker } from './utils/run-activity-marker'
4849
import { startTerminalWatchdog } from './utils/terminal-watchdog'
4950
import { installTerminalProtocolController } from './utils/terminal-protocol-controller'
5051
import { initializeSkillRegistry } from './utils/skill-registry'
@@ -401,6 +402,12 @@ async function main(): Promise<void> {
401402
// modes; the clean-shutdown path (renderer-cleanup) disarms it.
402403
startTerminalWatchdog()
403404

405+
// Lets the npm-wrapper launcher defer an auto-update restart until any
406+
// in-progress turn finishes, instead of interrupting it. Started early so
407+
// no isChainInProgress transition can slip by before the subscription
408+
// exists.
409+
startRunActivityMarker()
410+
404411
const renderer = await createCliRenderer({
405412
backgroundColor: 'transparent',
406413
exitOnCtrlC: false,
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import { existsSync, rmSync } from 'fs'
2+
3+
import { afterAll, beforeEach, describe, expect, test } from 'bun:test'
4+
5+
import { useChatStore } from '../../state/chat-store'
6+
import {
7+
runActivityMarkerPath,
8+
startRunActivityMarker,
9+
} from '../run-activity-marker'
10+
11+
describe('run-activity-marker', () => {
12+
const markerPath = runActivityMarkerPath()
13+
14+
beforeEach(() => {
15+
rmSync(markerPath, { force: true })
16+
useChatStore.getState().setIsChainInProgress(false)
17+
})
18+
19+
afterAll(() => {
20+
rmSync(markerPath, { force: true })
21+
useChatStore.getState().setIsChainInProgress(false)
22+
})
23+
24+
test('writes the marker while a turn is in progress and removes it when idle', () => {
25+
startRunActivityMarker()
26+
27+
expect(existsSync(markerPath)).toBe(false)
28+
29+
useChatStore.getState().setIsChainInProgress(true)
30+
expect(existsSync(markerPath)).toBe(true)
31+
32+
useChatStore.getState().setIsChainInProgress(false)
33+
expect(existsSync(markerPath)).toBe(false)
34+
})
35+
36+
test('is a no-op when the value does not actually change', () => {
37+
startRunActivityMarker()
38+
useChatStore.getState().setIsChainInProgress(true)
39+
rmSync(markerPath, { force: true })
40+
41+
// Re-affirming the same value must not recreate the marker: only a real
42+
// active/idle transition should.
43+
useChatStore.getState().setIsChainInProgress(true)
44+
expect(existsSync(markerPath)).toBe(false)
45+
})
46+
47+
test('registering more than once never stacks a duplicate exit handler', () => {
48+
startRunActivityMarker()
49+
const countAfterFirstStart = process.listenerCount('exit')
50+
51+
startRunActivityMarker()
52+
startRunActivityMarker()
53+
54+
expect(process.listenerCount('exit')).toBe(countAfterFirstStart)
55+
})
56+
})
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
/**
2+
* Cross-process "is a turn running" signal for the npm-wrapper launcher.
3+
*
4+
* The wrapper (release-core/launcher.js) checks for updates ~100ms after
5+
* spawning this process and, on finding one, force-restarts it -- with no
6+
* way to know whether the user is mid-turn, because it only has this
7+
* process's exit event, not its React state. While this marker file exists,
8+
* an agent turn is in progress; the wrapper waits for it to clear (bounded)
9+
* before stopping the process for an update, instead of interrupting a
10+
* turn that's still running.
11+
*
12+
* Named by this process's pid, which the wrapper already has from spawning
13+
* it -- no handshake needed. Best-effort throughout: a failed write or
14+
* remove just means the wrapper falls back to today's immediate-restart
15+
* behavior for this session.
16+
*/
17+
import { rmSync, writeFileSync } from 'fs'
18+
import os from 'os'
19+
import path from 'path'
20+
21+
import { useChatStore } from '../state/chat-store'
22+
23+
export function runActivityMarkerPath(pid: number = process.pid): string {
24+
return path.join(os.tmpdir(), `codebuff-run-active-${pid}`)
25+
}
26+
27+
let started = false
28+
29+
/** Call once, before the store can start toggling isChainInProgress. */
30+
export function startRunActivityMarker(): void {
31+
if (started) return
32+
started = true
33+
34+
const filePath = runActivityMarkerPath()
35+
36+
const clear = () => {
37+
try {
38+
rmSync(filePath, { force: true })
39+
} catch {
40+
// Best-effort; see module doc.
41+
}
42+
}
43+
44+
useChatStore.subscribe((state, prevState) => {
45+
if (state.isChainInProgress === prevState.isChainInProgress) return
46+
if (state.isChainInProgress) {
47+
try {
48+
writeFileSync(filePath, '', { flag: 'w' })
49+
} catch {
50+
// Best-effort; see module doc.
51+
}
52+
} else {
53+
clear()
54+
}
55+
})
56+
57+
process.on('exit', clear)
58+
}

0 commit comments

Comments
 (0)