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

Good diagnosis: checkForUpdates in launcher.js really does call stopRunningProcess unconditionally after staging, and the wrapper genuinely has no visibility into the child's internal state, so this is a real bug with a clear cause (issue #994).

The fix itself is sensible and cheap: a pid-named marker file toggled off isChainInProgress, polled by the wrapper with a bounded 10-minute fallback so a stuck turn can never permanently block an update. Subscribing once to the store instead of touching every site that sets isChainInProgress is the right layer for this — it stays correct as new call sites appear. Tests cover the marker's write/remove/no-duplicate-listener behavior and the three waitForRunIdle cases (no marker, clears mid-wait, gives up at the bound), which is more than most launcher changes in this repo get.

Two things worth thinking about before this lands for real, even if they don't block the idea:

1. Race window: waitForRunIdle is only called once, right after staging. If a turn starts between that check and the eventual stopRunningProcess call, it still gets killed. Given staging can take a while this window isn't negligible. A loop that re-checks right before the actual kill (or checks isChainInProgress again just before SIGTERM) would close this.
2. Marker staleness on hard kill: if the binary dies via SIGKILL, process.on('exit', clear) never runs and the marker leaks in os.tmpdir(). Pid reuse is rare but not impossible, and a leaked marker would falsely stall the next run's updates for up to 10 minutes. Worth stamping the marker with a start-time or session id you can also cross-check.

Neither is disqualifying — both degrade gracefully to "restart eventually happens" rather than silent breakage — but they're the kind of edge case a maintainer will ask about.

Thanks — took both. Point 2 is fixed in 3940411; point 1 I think rests on a misread of the ordering, details below.

2. Marker staleness on hard kill — real, fixed.

Agreed: SIGKILL or a native crash means process.on('exit', clear) never runs and the marker outlives the process, so a reused pid stalls the next run's updates for the full bound.

I went with clearing at spawn rather than stamping an identity into the marker. At the point spawnInstalledBinary has child.pid, the binary has not booted, let alone started a turn — so any marker at that path is definitionally an earlier process's, and no session id or start-time is needed to tell them apart. One rmSync on a path we already compute, versus a write format plus a parse-and-compare on the wrapper side. Covered by three tests: the clear itself, the no-marker case, and a source-order assertion that spawnInstalledBinary clears after it has a pid and before it returns the child.

1. Race window — the ordering is the other way round.

The sequence in checkForUpdates is:

const stagedBinary = await stageBinary(...) // staging
await waitForRunIdle(runningProcess.pid) // then wait
term.clearLine()
runningProcess.removeListener('exit', exitListener)
await stopRunningProcess(runningProcess) // then stop

Staging happens before waitForRunIdle, not between it and the kill — so "given staging can take a while this window isn't negligible" doesn't apply. What actually sits in the gap is term.clearLine() and removeListener(): two synchronous calls, no await. The existing test in wrapper-safety.test.ts pins that order (stageIndex < waitIndex < stopIndex) precisely so a later edit can't reintroduce the window you're describing.

There is still a sub-millisecond TOCTOU gap, since these are separate processes and the user could hit Enter inside it. But that gap is irreducible without a handshake: re-checking immediately before SIGTERM moves the window, it doesn't close it, because the binary can always start a turn after the wrapper's last look. Closing it properly means the wrapper asking the binary to stop accepting turns and waiting for an ack — which is a bidirectional protocol, and this repo deliberately avoids pipes between these two processes.

For a bug whose current behavior is "kill unconditionally, mid-turn, every time", trading that for a sub-millisecond window seemed like the right amount of machinery. Happy to add the extra re-check if you'd still prefer it — it's cheap and harmless, I just don't want to claim it fixes something it doesn't.

Full suite is unchanged at 39 failures, identical to the pre-change baseline. tsc --noEmit clean on the touched files. launcher.js still fails prettier --check, but only on a pre-existing guard clause in checkForUpdates that fails on unmodified main too — my lines are clean, and I've left that one alone rather than bury the diff in an unrelated reformat.