Skip to content

Navigation Menu

Sign in
Sign up

fix(cli): wait for the current turn to finish before an auto-update restart #1258

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kavish-19 wants to merge 2 commits into CodebuffAI:main
base: main
Choose a base branch
Loading
from kavish-19:fix/defer-auto-update-restart-until-idle
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions cli/release-core/launcher.js
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -1267,6 +1321,8 @@ function createLauncher(productConfig) {
child.on('error', exitOnSpawnFailure)
child.launch = watchLaunch(child)

if (child.pid !== undefined) clearStaleRunActivityMarker(child.pid)

return child
}

Expand Down Expand Up @@ -1461,6 +1517,9 @@ function createLauncher(productConfig) {
getRequiredWrapperVersion,
ensureBinaryReady,
isTargetAllowedForThisMachine,
runActivityMarkerPath,
clearStaleRunActivityMarker,
waitForRunIdle,
CONFIG,
},
}
Expand Down
116 changes: 114 additions & 2 deletions cli/src/__tests__/release/wrapper-safety.test.ts
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { EventEmitter } from 'node:events'
import { createServer } from 'node:http'
import {
copyFileSync,
existsSync,
mkdirSync,
mkdtempSync,
readFileSync,
Expand Down Expand Up @@ -166,14 +167,15 @@ 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'),
)
const stageIndex = updateFunction.indexOf(
'const stagedBinary = await stageBinary',
)
const waitIndex = updateFunction.indexOf('await waitForRunIdle(')
const stopIndex = updateFunction.indexOf(
'await stopRunningProcess(runningProcess)',
)
Expand All @@ -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
Expand Down
7 changes: 7 additions & 0 deletions cli/src/index.tsx
View file Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -401,6 +402,12 @@ async function main(): Promise<void> {
// 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,
Expand Down
56 changes: 56 additions & 0 deletions cli/src/utils/__tests__/run-activity-marker.test.ts
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
})
})
58 changes: 58 additions & 0 deletions cli/src/utils/run-activity-marker.ts
View file Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
}
Loading

AltStyle によって変換されたページ (->オリジナル) /