fix(cli): preserve sessions across ao stop → ao update → ao start (closes #1743) - #1780
fix(cli): preserve sessions across ao stop → ao update → ao start (closes #1743)#1780suraj-markup wants to merge 1 commit into
Conversation
Test Coverage Report
Per-file breakdown
Uncovered lines
|
Greptile SummaryThis PR hardens the
Confidence Score: 5/5Safe to merge — the three root-cause fixes (durable fsync write, fallback scan, empty-sentinel acknowledgment) are all correctly implemented and the previously-flagged issues from prior review rounds have been resolved. All control paths that call markLastStopAcknowledged are guarded by the outer if (lastStop && totalLastStopSessions > 0) check, so lastStop is never null at those call sites. The empty sentinel is correctly readable by readLastStop (passes all validation) yet produces totalLastStopSessions === 0, which prevents re-prompting and closes the fallback gate. The global config is now used for the fallback session manager, resolving the cross-project session visibility gap. Temp-file cleanup on write failure is handled in the finally block, and the fsync path is verified by new tests. No correctness gaps remain in the changed files. No files require special attention.
|
| Filename | Overview |
|---|---|
| packages/cli/src/lib/running-state.ts | Adds atomicWriteFileSyncDurable (fsync before rename for durability) and markLastStopAcknowledged (empty sentinel to block fallback re-prompts); temp-file cleanup on write failure is correct |
| packages/cli/src/lib/last-stop-fallback.ts | New module: buildLastStopFallback synthesizes a LastStopState from recently manually-killed sessions; findRecentlyKilledSessions wraps it with sm.list() and silences errors; window filtering and project grouping are correct |
| packages/cli/src/commands/start.ts | Replaces clearLastStop with markLastStopAcknowledged on all decision paths and adds the fallback scan guarded by !lastStop; all markLastStopAcknowledged calls occur inside if (lastStop && totalLastStopSessions > 0) so lastStop is never null there |
| packages/cli/tests/lib/last-stop-fallback.test.ts | New unit tests for buildLastStopFallback covering window filtering, primary/other-project routing, bad timestamps, and custom window; comprehensive |
| packages/cli/tests/lib/running-state.test.ts | Adds fsync-call verification, temp-file cleanup-on-failure test, otherProjects round-trip, and markLastStopAcknowledged sentinel test; node:fs mock correctly intercepts fsyncSync/writeFileSync without breaking other tests |
| packages/cli/tests/commands/start.test.ts | Adds mock for markLastStopAcknowledged, renames the acknowledge-vs-unlink test, and adds four new scenarios: no-re-prompt after decline, fallback fires when missing, fallback respects window, and fallback uses global config for cross-project sessions |
| packages/cli/tests/commands/update.test.ts | Adds regression test that ao update does not touch last-stop.json by staging a real file in a temp HOME and asserting byte-for-byte identity after the update command runs |
| packages/cli/tests/lib/stop-update-start-flow.test.ts | New integration test exercising the full stop→simulated-update→start round-trip and the fallback path against a temp HOME; correctly isolates os.homedir() via vi.mock |
Reviews (7): Last reviewed commit: "fix(cli): preserve sessions across ao st..." | Re-trigger Greptile
|
Hey @suraj-markup, thanks for the layered fix. Before approving I'd like a deterministic repro on fresh Why I'm asking:
If the race is hard to trigger by hand, an instrumented test that exercises the SIGKILL / mid-shutdown window would work just as well. Happy to approve the fallback piece on its own in the meantime if that helps unblock. |
) PR #1780 review (Harshit): the `ao stop` CLI is a separate process from the `ao start` parent and `await writeLastStop(...)` completes before the CLI sends SIGTERM to the parent — so the original race the pre-write+reconcile branch was guarding against is not reproducible by hand on main. The fallback in `ao start` is doing the user-visible work. Drop the higher-surface pre-write+reconcile changes; keep what is unambiguously safer or independently useful: - Keep `last-stop-fallback.ts` + its integration in runStartup so a missing/malformed last-stop.json still surfaces a restore prompt. - Keep the fsync'd `atomicWriteFileSyncDurable` so the write survives a hard kill that lands immediately after rename. - Also fix Greptile P2: `atomicWriteFileSyncDurable` now unlinks the temp file on `writeFileSync`/`fsyncSync` throw (previously leaked). - Revert `registerStop` and the SIGTERM shutdown handler to their pre-PR post-kill write shape — same as origin/main. - Drop the two pre-write ordering tests; this removes the Greptile P2 unreachable `clearLastStop` branch naturally. Tests retained / added: - last-stop-fallback unit tests (5) - stop → simulated update → start integration test (2) - writeLastStop fsync test - NEW: writeLastStop leaves no temp file on write-throw - ao update no-touch regression test Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Thanks @harshitsinghbhandari — all three concerns are fair and I agree with the downsize. Pushed 72beb03. On the repro (your #1 and #2). You're right. The What's in the new commit:
Kept (unambiguously safer or independently useful):
On your #3 ( Net diff vs your last review: −238 / +83 across 5 files. |
…surface (#1743) Greptile P1 on PR #1780. `getSessionManager(config)` in the fallback path was built from the current project's config, so `sm.list()` only saw that project's sessions. When `ao stop` ran globally and killed sessions across multiple projects, the synthesized `LastStopState` would have an empty `otherProjects` — defeating the cross-project restore that the pre-existing `readLastStop` path already supports (because `ao stop` writes the cross-project rows at stop time). Mirror the global-config load that the restore step on line ~1002 already does: if the global config exists, prefer it when constructing the fallback session manager. The downstream restore code already loads the global config when `otherProjects` is non-empty, so this just lets the fallback populate that array in the first place. Regression test in start.test.ts asserts both the in-project and a cross-project session get routed to `sm.restore()` after the fallback synthesizes a record from a global-scope `sm.list()`. The two existing fallback tests now pin AO_GLOBAL_CONFIG to a non-existent path so they don't accidentally read the host's real ~/.agent-orchestrator/config.yaml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Greptile P1 (fallback session manager using project-scoped config) → fixed in 1339d9c. The fallback now loads the global config (mirroring the restore step at ~start.ts:1002) before constructing the session manager, so cross-project sessions are surfaced into |
) PR #1780 review (Harshit): the `ao stop` CLI is a separate process from the `ao start` parent and `await writeLastStop(...)` completes before the CLI sends SIGTERM to the parent — so the original race the pre-write+reconcile branch was guarding against is not reproducible by hand on main. The fallback in `ao start` is doing the user-visible work. Drop the higher-surface pre-write+reconcile changes; keep what is unambiguously safer or independently useful: - Keep `last-stop-fallback.ts` + its integration in runStartup so a missing/malformed last-stop.json still surfaces a restore prompt. - Keep the fsync'd `atomicWriteFileSyncDurable` so the write survives a hard kill that lands immediately after rename. - Also fix Greptile P2: `atomicWriteFileSyncDurable` now unlinks the temp file on `writeFileSync`/`fsyncSync` throw (previously leaked). - Revert `registerStop` and the SIGTERM shutdown handler to their pre-PR post-kill write shape — same as origin/main. - Drop the two pre-write ordering tests; this removes the Greptile P2 unreachable `clearLastStop` branch naturally. Tests retained / added: - last-stop-fallback unit tests (5) - stop → simulated update → start integration test (2) - writeLastStop fsync test - NEW: writeLastStop leaves no temp file on write-throw - ao update no-touch regression test Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…surface (#1743) Greptile P1 on PR #1780. `getSessionManager(config)` in the fallback path was built from the current project's config, so `sm.list()` only saw that project's sessions. When `ao stop` ran globally and killed sessions across multiple projects, the synthesized `LastStopState` would have an empty `otherProjects` — defeating the cross-project restore that the pre-existing `readLastStop` path already supports (because `ao stop` writes the cross-project rows at stop time). Mirror the global-config load that the restore step on line ~1002 already does: if the global config exists, prefer it when constructing the fallback session manager. The downstream restore code already loads the global config when `otherProjects` is non-empty, so this just lets the fallback populate that array in the first place. Regression test in start.test.ts asserts both the in-project and a cross-project session get routed to `sm.restore()` after the fallback synthesizes a record from a global-scope `sm.list()`. The two existing fallback tests now pin AO_GLOBAL_CONFIG to a non-existent path so they don't accidentally read the host's real ~/.agent-orchestrator/config.yaml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1339d9c to
54f36ca
Compare
…prompt (#1743) Greptile P1 on PR #1780. After the user declined a restore (or after a successful restore), `clearLastStop()` unlinked last-stop.json. On the next `ao start` within 10 minutes, `readLastStop()` returned null, the fallback gate `!lastStopHasContent` fired, and the same recently manually_killed sessions were surfaced again — re-prompting after a decline. Fix: - Add `markLastStopAcknowledged(projectId)` in running-state.ts: writes a valid LastStopState with empty `sessionIds`. The file is present on disk but has no content, so the restore prompt does not fire. - Tighten the fallback gate in runStartup from `!lastStopHasContent` to `!lastStop` (file truly absent). The empty marker keeps the gate closed across subsequent `ao start` runs. - Replace the four `clearLastStop()` call sites in the prompt path (declined, all-restored, partial-all-resolved, empty-restore-list) with `markLastStopAcknowledged(lastStop.projectId)`. - `clearLastStop` stays exported because tests still observe it (and no current code path calls it after this change — removing the export would be a separate cleanup). Tests: - NEW: `does not re-prompt with fallback after user declines (PR #1780)` in start.test.ts — primes readLastStop with an empty marker, leaves a recent manually_killed session in sm.list(), asserts neither promptConfirm nor sm.restore was called. - NEW: `markLastStopAcknowledged writes a present but empty marker readable by readLastStop` in running-state.test.ts. - UPDATED: the "every session restored successfully" test now expects `markLastStopAcknowledged` instead of `clearLastStop`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Greptile P1 (fallback re-prompts after user declines) → fixed in 7d94ba3. New helper Replaced all four Regression test in start.test.ts: |
) The ao stop → ao update → ao start flow could drop the restore prompt when last-stop.json went missing on disk by the time ao start looked for it. Two layered fixes plus a cross-project follow-up: 1. ao start falls back to scanning recently `manually_killed` sessions (terminated within 10 minutes, reason=manually_killed) when last-stop.json is absent. The restore prompt surfaces them with the same UX as the existing record so a regression in the write pipeline cannot silently drop the user's in-flight work. The fallback uses the GLOBAL config so cross-project sessions appear in `otherProjects` (mirrors the existing restore step's global-config load). 2. writeLastStop now uses an fsync'd atomic write (atomicWriteFileSyncDurable) so the record survives a hard kill that lands immediately after the rename — renameSync is atomic but the data blocks aren't durable until fsync. The temp file is also unlinked on writeFileSync / fsyncSync throw (Greptile P2 #1 follow-up). 3. After any user decision on a restore prompt (declined / all-restored / empty), runStartup writes an empty marker via the new markLastStopAcknowledged() helper instead of unlinking the file. The fallback gate is tightened from `!lastStopHasContent` to `!lastStop` (file truly absent) so a second ao start within the 10-minute window does not re-surface sessions the user already declined (Greptile P1 review on PR #1780). Tests: - last-stop-fallback unit tests (5) - stop → simulated update → start integration test (2) - writeLastStop fsync test (1) - writeLastStop temp-file cleanup on write throw (1) - otherProjects round-trip through readLastStop (1) - markLastStopAcknowledged round-trip (1) - ao update no-touch regression (1) - start.ts: fallback finds recent killed sessions (1) - start.ts: fallback respects window (1) - start.ts: fallback uses global config for cross-project (1) - start.ts: empty marker prevents re-prompt after decline (1) - start.ts: every-session-restored writes marker (not unlink) (1) Closes #1743 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7d94ba3 to
cb25be9
Compare
) The ao stop → ao update → ao start flow could drop the restore prompt when last-stop.json went missing on disk by the time ao start looked for it. Two layered fixes plus a cross-project follow-up: 1. ao start falls back to scanning recently `manually_killed` sessions (terminated within 10 minutes, reason=manually_killed) when last-stop.json is absent. The restore prompt surfaces them with the same UX as the existing record so a regression in the write pipeline cannot silently drop the user's in-flight work. The fallback uses the GLOBAL config so cross-project sessions appear in `otherProjects` (mirrors the existing restore step's global-config load). 2. writeLastStop now uses an fsync'd atomic write (atomicWriteFileSyncDurable) so the record survives a hard kill that lands immediately after the rename — renameSync is atomic but the data blocks aren't durable until fsync. The temp file is also unlinked on writeFileSync / fsyncSync throw (Greptile P2 #1 follow-up). 3. After any user decision on a restore prompt (declined / all-restored / empty), runStartup writes an empty marker via the new markLastStopAcknowledged() helper instead of unlinking the file. The fallback gate is tightened from `!lastStopHasContent` to `!lastStop` (file truly absent) so a second ao start within the 10-minute window does not re-surface sessions the user already declined (Greptile P1 review on PR #1780). Tests: - last-stop-fallback unit tests (5) - stop → simulated update → start integration test (2) - writeLastStop fsync test (1) - writeLastStop temp-file cleanup on write throw (1) - otherProjects round-trip through readLastStop (1) - markLastStopAcknowledged round-trip (1) - ao update no-touch regression (1) - start.ts: fallback finds recent killed sessions (1) - start.ts: fallback respects window (1) - start.ts: fallback uses global config for cross-project (1) - start.ts: empty marker prevents re-prompt after decline (1) - start.ts: every-session-restored writes marker (not unlink) (1) Closes #1743 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cb25be9 to
945a7ef
Compare
|
Closing as part of a backlog sweep: Agent Orchestrator was rewritten from the ground up (Go backend + Electron/React frontend), and this PR's changes target files from the pre-rewrite TypeScript codebase that no longer exist in the current tree, so it cannot be rebased onto main. If the underlying change still makes sense for the current app, please open a fresh PR against the new codebase. Thanks for the contribution! |
Summary
Closes #1743.
The
ao stop→ao update→ao startflow was silently dropping the restore prompt becauselast-stop.jsoncould be missing from disk by the timeao startchecked for it. Three layered fixes harden the path:Root cause
ao stop(and the SIGTERM shutdown handler inlib/shutdown.ts) only wrotelast-stop.jsonAFTER finishing the kill loop. If the CLI was killed mid-shutdown — SIGKILL, hard crash, or an impatient re-run — the record was never written andao starthad nothing to restore.The atomic write also went through
writeFileSync+renameSyncwithout an explicitfsync, so a hard kill that landed immediately after the call could lose the file's data even though the rename had committed the dirent.Fix
ao stopand the shutdown handler now writelast-stop.jsonwith every active session id BEFORE running the kill loop, then reconcile (rewrite orclearLastStop) once the loop's actual results are known. The newatomicWriteFileSyncDurablecallsfsyncSyncon the temp file beforerenameSync, so the bytes are durable even if the process is killed milliseconds later.ao start. Whenlast-stop.jsonis missing or empty, scan recentlymanually_killedsessions (terminated within the last 10 minutes) viafindRecentlyKilledSessionsand synthesize aLastStopState. The restore prompt fires with the same wording as the existing record. This protects against future regressions in the write pipeline.ao updatedoes NOT touchlast-stop.json. The currentao updatecommand never reads or writes anything under~/.agent-orchestrator/, but adding state-clearing logic in the future would re-introduce the bug. The new test stages alast-stop.jsonin a tempHOMEand asserts it is byte-for-byte identical afterao updateruns.Before vs. after of the file-write timing
If the CLI is killed between
kill(s)andwrite last-stop.jsonin the old order, the record vanishes. The new order moves the durable write before any teardown work.Tests added
packages/cli/__tests__/lib/last-stop-fallback.test.ts(5) — unit tests forbuildLastStopFallback: window respect, primary vs other-project routing, missing/unparseable timestamps, custom windowpackages/cli/__tests__/lib/stop-update-start-flow.test.ts(2) — hermetic stop → simulated update → start flow against a temp HOME, plus fallback pathwriteLastStop fsyncs the temp file before renaminginrunning-state.test.tsdoes not touch ~/.agent-orchestrator/last-stop.jsoninupdate.test.tspre-writes last-stop.json before the kill loop runsinstart.test.tsclears last-stop.json when every kill failsinstart.test.tsfalls back to recently manually-killed sessions when last-stop.json is missinginstart.test.tsdoes not surface fallback candidates older than the recent windowinstart.test.tspnpm typecheckandpnpm lintpass.pnpm testshows the same 10 pre-existing failures on this branch as onmain; my 7 new test cases all pass.Test plan
pnpm typecheckcleanpnpm lintno errors (warnings only, all pre-existing)pnpm exec vitest run lib/last-stop-fallback lib/stop-update-start-flow lib/running-state— all greenpnpm exec vitest run commands/start.test.ts -t "issue #1743|every kill fails"— 4 passpnpm exec vitest run commands/update.test.ts— 26 pass (including new regression)ao stop→ao update→ao starton a checkout with active sessions reproduces the restore prompt🤖 Generated with Claude Code