Skip to content

Commit 3940411

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

2 files changed

Lines changed: 70 additions & 0 deletions

File tree

cli/release-core/launcher.js

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -941,6 +941,22 @@ function createLauncher(productConfig) {
941941
return path.join(os.tmpdir(), `codebuff-run-active-${pid}`)
942942
}
943943

944+
/**
945+
* Drop a marker left behind by a process that died before it could clear
946+
* its own -- SIGKILL, a native crash -- since its exit handler never ran.
947+
* Called with a pid we have only just spawned, so any marker at that path
948+
* belongs to an earlier process the OS has since reused the pid for; the
949+
* binary cannot have started a turn yet. Without this, that stale file
950+
* stalls the new run's updates for the whole RUN_IDLE_MAX_WAIT_MS bound.
951+
*/
952+
function clearStaleRunActivityMarker(pid) {
953+
try {
954+
fs.rmSync(runActivityMarkerPath(pid), { force: true })
955+
} catch {
956+
// Best effort: a marker we can't remove only costs us the bounded wait.
957+
}
958+
}
959+
944960
const RUN_IDLE_POLL_INTERVAL_MS = 1_000
945961
// Don't stall an update behind one long-running turn forever; fall back to
946962
// today's immediate-restart behavior once this elapses.
@@ -1305,6 +1321,8 @@ function createLauncher(productConfig) {
13051321
child.on('error', exitOnSpawnFailure)
13061322
child.launch = watchLaunch(child)
13071323

1324+
if (child.pid !== undefined) clearStaleRunActivityMarker(child.pid)
1325+
13081326
return child
13091327
}
13101328

@@ -1500,6 +1518,7 @@ function createLauncher(productConfig) {
15001518
ensureBinaryReady,
15011519
isTargetAllowedForThisMachine,
15021520
runActivityMarkerPath,
1521+
clearStaleRunActivityMarker,
15031522
waitForRunIdle,
15041523
CONFIG,
15051524
},

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

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,57 @@ describe('shared release launcher safety', () => {
247247
}
248248
})
249249

250+
test('a spawned binary starts from a clean activity marker', () => {
251+
const { clearStaleRunActivityMarker, runActivityMarkerPath } =
252+
createLauncher({
253+
packageName: 'test',
254+
displayName: 'Test',
255+
}).__testing
256+
const pid = 999_999_004
257+
const markerPath = runActivityMarkerPath(pid)
258+
259+
// A marker left by a process that died before it could clear its own --
260+
// SIGKILL, a native crash -- outlives it in tmpdir. Reaching the same pid
261+
// again would otherwise stall that run's updates for the full bound.
262+
writeFileSync(markerPath, '')
263+
264+
try {
265+
clearStaleRunActivityMarker(pid)
266+
expect(existsSync(markerPath)).toBe(false)
267+
} finally {
268+
rmSync(markerPath, { force: true })
269+
}
270+
})
271+
272+
test('clearing a stale marker tolerates there being none', () => {
273+
const { clearStaleRunActivityMarker, runActivityMarkerPath } =
274+
createLauncher({
275+
packageName: 'test',
276+
displayName: 'Test',
277+
}).__testing
278+
const pid = 999_999_005
279+
280+
rmSync(runActivityMarkerPath(pid), { force: true })
281+
282+
expect(() => clearStaleRunActivityMarker(pid)).not.toThrow()
283+
})
284+
285+
test('spawnInstalledBinary clears the stale marker once it has a pid', () => {
286+
const source = readFileSync(launcherPath, 'utf8')
287+
const spawnFunction = source.slice(
288+
source.indexOf('function spawnInstalledBinary'),
289+
)
290+
const spawnIndex = spawnFunction.indexOf('child = spawn(CONFIG.binaryPath')
291+
const clearIndex = spawnFunction.indexOf('clearStaleRunActivityMarker(')
292+
const returnIndex = spawnFunction.indexOf('return child')
293+
294+
expect(spawnIndex).toBeGreaterThan(-1)
295+
// The pid only exists after the spawn, and the marker must be gone before
296+
// the caller can hand this child to checkForUpdates.
297+
expect(clearIndex).toBeGreaterThan(spawnIndex)
298+
expect(returnIndex).toBeGreaterThan(clearIndex)
299+
})
300+
250301
test('requires the wrapper release only for missing or older binaries', () => {
251302
const cases: Array<{
252303
wrapperVersion: string

0 commit comments

Comments
 (0)