fix(cli): wait for the current turn to finish before an auto-update restart - #1258
Open
kavish-19 wants to merge 1 commit into
Open
fix(cli): wait for the current turn to finish before an auto-update restart#1258kavish-19 wants to merge 1 commit into
kavish-19 wants to merge 1 commit into
Conversation
…estart 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 CodebuffAI#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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #994.
The bug
main()schedulescheckForUpdates100ms after spawning the binary. When it finds a newer version it stages the download and then unconditionally stops the running process to install it:The wrapper has no way to know whether the user is mid-turn — it's a separate process that only sees the child's exit event, not its state. So a download that lands a few seconds into a session kills a turn that is still running, which is exactly what the reporter describes: "once the download finishes, the CLI automatically restarts, even if I'm in the middle of a session."
The fix
A small cross-process signal, in the spirit of the marker files
terminal-watchdog.tsalready uses for the same kind of wrapper/binary coordination:cli/src/utils/run-activity-marker.ts): writes a marker file for the duration of a turn and removes it when idle or on exit. It subscribes once to the store'sisChainInProgress, so every current and future site that toggles that flag is covered without touching any of them. Named by the process's own pid, which the wrapper already has from spawning it — no handshake needed.cli/release-core/launcher.js):waitForRunIdle(pid)polls for that marker to clear, andcheckForUpdatesawaits it after staging and before stopping the process.Bounded and best-effort in both directions, so it can only ever delay a restart, never prevent one:
The staging download itself is unchanged and still happens up front; only the stop-and-swap waits.
Testing
New tests:
waitForRunIdle: returns immediately with no marker, waits and returns once the marker clears, and gives up atmaxWaitMswithout touching the marker.checkForUpdatessource-order check to require the wait between staging and stopping.Confirmed red against the unfixed code (all four new launcher assertions fail —
waitForRunIdledoesn't exist) and green after.Also ran the full
cli/srcsuite before and after: identical 34 pre-existing failures and 32 errors either way (they reproduce on unmodifiedmain— an OSC 52 clipboard test plus missing@types/react-dom/tarin the local environment), with 6 new passing tests and no regressions.tsc --noEmiton theclipackage reports nothing new in any touched file.