Repository navigation
Session restore stress: recover from corrupt snapshot via -previous backup - #5914
Conversation
…re backup A corrupt primary session snapshot at startup currently makes syncManualRestoreSnapshotCache delete session-<bundle>-previous.json (the restore-session backup) and the app silently starts fresh. These tests pin the desired behavior: keep the backup and recover startup restore from it. Includes tests/test_session_restore_stress_kill_cycles.py, an end-to-end stress harness covering repeated clean relaunches, SIGKILL relaunch, and corrupt-snapshot recovery for tracked Claude/Codex/OpenCode sessions. SessionPersistenceStore.syncManualRestoreSnapshotCache and the new loadStartupSnapshot gain injectable bundle/app-support parameters (behavior unchanged in this commit) so the regression tests can run against temp paths.
…apshot is corrupt SessionPersistenceStore.load() treated a corrupt primary snapshot the same as a missing one: startup silently began a fresh session and syncManualRestoreSnapshotCache deleted session-<bundle>-previous.json, the only remaining copy of the user's workspaces, so restore-session could not recover anything either. loadOutcome now distinguishes a missing snapshot (clean state) from an unusable one (unreadable data, decode failure, schema drift, anomalous empty window list). When the primary is unusable, the backup is preserved and startup restore falls back to it, so workspaces and tracked agent sessions come back automatically. Also captures launch environment in the stress harness hook entries so fake-claude resume wins the PATH lookup (claude resume intentionally routes through the wrapper shim / PATH instead of the captured executable).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCentralizes snapshot-file validation into SnapshotLoadOutcome, updates cache sync and startup-loading to prefer a valid primary and fall back to a previous backup only when appropriate, switches AppDelegate to the new startup loader, and adds unit and integration tests for corruption, missing-primary, and multi-phase persistence. ChangesSession restoration with backup fallback and recovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes a data-loss bug where a corrupt primary session snapshot caused cmux to silently start fresh and delete the
Confidence Score: 5/5Safe to merge — the fix narrowly changes how a corrupt-primary startup is handled, the happy path is unchanged, and the new behavior is covered by both unit and end-to-end tests. The change is well-scoped: the three state transitions (loaded/missing/unusable) are exhaustively tested in unit tests with isolated fixtures, the call-site change in AppDelegate is a single-line swap, and the logic for each case was verified empirically against a pre-fix failing build and a post-fix passing build. No production logging rule, actor isolation, or blocking-runtime concern was introduced or worsened beyond the pre-existing synchronous startup disk reads. No files require special attention; Sources/SessionPersistence.swift carries the most new logic but all three outcome branches are covered by dedicated unit tests. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[prepareStartupSessionSnapshotIfNeeded] --> B[syncManualRestoreSnapshotCache]
B --> C{loadOutcome primary}
C -->|.loaded| D[save primary → backup]
C -->|.missing| E[delete backup]
C -->|.unusable| F[keep backup unchanged]
A --> G{shouldAttemptRestore?}
G -->|no| Z[return — no restore]
G -->|yes| H[loadStartupSnapshot]
H --> I{loadOutcome primary}
I -->|.loaded| J[return primary snapshot]
I -->|.missing| K[return nil — fresh start]
I -->|.unusable| L[loadReopenSessionSnapshot backup]
L --> M{backup valid?}
M -->|yes| N[return backup snapshot]
M -->|no/missing| O[return nil — fresh start]
N --> P[DEBUG: log primaryUnusable + backupRecovered]
Reviews (7): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static func loadOutcome(fileURL: URL) -> SnapshotLoadOutcome { | ||
| guard FileManager.default.fileExists(atPath: fileURL.path) else { return .missing } | ||
| guard let data = try? Data(contentsOf: fileURL) else { return .unusable } | ||
| let decoder = JSONDecoder() | ||
| guard let snapshot = try? decoder.decode(AppSessionSnapshot.self, from: data) else { return .unusable } | ||
| guard snapshot.version == SessionSnapshotSchema.currentVersion else { return .unusable } | ||
| guard !snapshot.windows.isEmpty else { return .unusable } | ||
| return .loaded(snapshot) |
There was a problem hiding this comment.
loadOutcome is internal but not exercised directly by tests
loadOutcome(fileURL:) and SnapshotLoadOutcome are both internal, which means any @testable import cmux consumer can call them directly and couple to the intermediate outcome values. The four new unit tests only call the higher-level helpers (syncManualRestoreSnapshotCache, loadStartupSnapshot, load, save), so there is no test reason for loadOutcome to be wider than private. Making it private static func would keep the public contract at the level of the store's API rather than its parsing step.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Declining: loadOutcome is deliberately covered through the public behaviors that depend on it (syncManualRestoreSnapshotCache, loadStartupSnapshot, load) in SessionPersistenceTests; testing the enum directly would pin implementation rather than behavior.
| static func loadStartupSnapshot( | ||
| bundleIdentifier: String? = Bundle.main.bundleIdentifier, | ||
| appSupportDirectory: URL? = nil | ||
| ) -> AppSessionSnapshot? { | ||
| guard let primaryURL = defaultSnapshotFileURL( | ||
| bundleIdentifier: bundleIdentifier, | ||
| appSupportDirectory: appSupportDirectory | ||
| ) else { return nil } | ||
| switch loadOutcome(fileURL: primaryURL) { | ||
| case .loaded(let snapshot): | ||
| return snapshot | ||
| case .missing: | ||
| return nil | ||
| case .unusable: | ||
| return loadReopenSessionSnapshot( | ||
| bundleIdentifier: bundleIdentifier, | ||
| appSupportDirectory: appSupportDirectory | ||
| ) | ||
| } | ||
| _ = save(snapshot, fileURL: fileURL) | ||
| } |
There was a problem hiding this comment.
os.log diagnostic when falling back from corrupt primary to backup
When loadStartupSnapshot hits the .unusable branch and silently returns a backup snapshot, the event is invisible in Console.app. If the primary is corrupt for a systemic reason (bad autosave write, filesystem issue), there is no telemetry to detect frequency or correlate with user-facing confusion. An os_log warning at the .unusable branch in both syncManualRestoreSnapshotCache and loadStartupSnapshot naming the file URL and outcome would make field incidents diagnosable without exposing user content.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Done in aa06b13: session.restore.primaryUnusable path=... backupRecovered=0|1 debug-log line on the fallback path.
| try: | ||
| os.kill(pid, signal.SIGKILL) | ||
| except ProcessLookupError: | ||
| pass |
There was a problem hiding this comment.
time.sleep(1.5) after socket readiness hides internal-initialization races
_wait_for_socket proves the socket is reachable but not that the app has finished internal startup (session snapshot applied, workspaces restored, surfaces ready). The 1.5 s wall-clock delay is a best-guess window that can fail on a slow CI host or pass on a fast host that has not actually initialized. The same pattern appears in _quit (time.sleep(0.8)) and _force_kill (time.sleep(0.8)). A real signal from the app (startup-complete ping response, workspace-count API reply, or an explicit ready handshake) would make phase transitions deterministic.
There was a problem hiding this comment.
Declining the post-socket sleep change: it matches the established harness pattern in tests/test_session_relaunch_resumes_agent_sessions.py, and hook stores are read synchronously at snapshot-save time, so there is no ingestion race. The suite has passed repeatedly end to end.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_session_restore_stress_kill_cycles.py`:
- Around line 336-344: The test currently uses a fixed sleep after writing hook
state files (around client.select_workspace(0) then time.sleep(0.4)) which can
allow an incomplete initial snapshot; replace that fixed sleep with a poll that
verifies the app has ingested all six seeded sessions before calling _quit().
After calling _write_hook_state(hook_state_files[launcher], entries) and
client.select_workspace(0), repeatedly query the authoritative condition (for
example via the client API that reports tracked/ingested sessions or a session
list/count exposed by the test harness) until it shows six sessions are tracked,
with a short sleep between attempts and a sensible overall timeout that fails
the test if readiness is not reached; only then proceed to _quit(bundle_id,
socket_path) (preserve existing failure handling via failures and _report).
- Around line 390-395: Add an explicit invocation of the CLI entrypoint that
exercises the same recovery code path: call “cmux restore-session” (which routes
to session.restore_previous) after confirming previous_snapshot.exists() and
before calling _quit(bundle_id, socket_path), then re-check the resumed/marker
artifacts the same way the startup path does; specifically, in
tests/test_session_restore_stress_kill_cycles.py add a step that runs the cmux
restore-session command for the bundle_id/socket_path and assert the resumed
markers (the same checks used for startup-restore) to ensure the CLI path is
tested in addition to on-startup restore.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50695f88-a7b0-42dd-9d74-e8dadca2f45f
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/SessionPersistence.swiftcmuxTests/SessionPersistenceTests.swifttests/test_session_restore_stress_kill_cycles.py
…g and wrapper resolution Claude hook records are only restorable when their transcript exists on disk (hookRecordIsRestorable), and claude resume routes through the cmux claude wrapper, which resolves the real binary instead of the captured executable. The relaunch harness silently lost its claude assertion when those behaviors landed (it is not run in CI): fake claude sessions were dropped at index load, and when they did resume the real claude binary ran instead of the fake. Both harnesses now write a transcriptPath for claude sessions, point CMUX_CUSTOM_CLAUDE_PATH at the fake binary, and match resume markers as order-agnostic tokens on one line (the wrapper inserts its own arguments around --resume).
…file length budget
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…-session in corrupt phase Adds a session.restore.primaryUnusable debug-log line when startup falls back from an unusable primary snapshot, and extends the corrupt-snapshot stress phase to run the bundled cmux restore-session verb, asserting it reopens the backed-up session in a new window (window count, since the restored workspaces share ids with the startup fallback restore).
…store-stress # Conflicts: # .github/swift-file-length-budget.tsv
…store-stress # Conflicts: # .github/swift-file-length-budget.tsv
…store-stress # Conflicts: # .github/swift-file-length-budget.tsv
…store-stress # Conflicts: # .github/swift-file-length-budget.tsv
…ackup (manaflow-ai#5914) * Add failing coverage: corrupt session snapshot must not destroy restore backup A corrupt primary session snapshot at startup currently makes syncManualRestoreSnapshotCache delete session-<bundle>-previous.json (the restore-session backup) and the app silently starts fresh. These tests pin the desired behavior: keep the backup and recover startup restore from it. Includes tests/test_session_restore_stress_kill_cycles.py, an end-to-end stress harness covering repeated clean relaunches, SIGKILL relaunch, and corrupt-snapshot recovery for tracked Claude/Codex/OpenCode sessions. SessionPersistenceStore.syncManualRestoreSnapshotCache and the new loadStartupSnapshot gain injectable bundle/app-support parameters (behavior unchanged in this commit) so the regression tests can run against temp paths. * Recover session restore from the -previous backup when the primary snapshot is corrupt SessionPersistenceStore.load() treated a corrupt primary snapshot the same as a missing one: startup silently began a fresh session and syncManualRestoreSnapshotCache deleted session-<bundle>-previous.json, the only remaining copy of the user's workspaces, so restore-session could not recover anything either. loadOutcome now distinguishes a missing snapshot (clean state) from an unusable one (unreadable data, decode failure, schema drift, anomalous empty window list). When the primary is unusable, the backup is preserved and startup restore falls back to it, so workspaces and tracked agent sessions come back automatically. Also captures launch environment in the stress harness hook entries so fake-claude resume wins the PATH lookup (claude resume intentionally routes through the wrapper shim / PATH instead of the captured executable). * Repair claude coverage in relaunch/stress harnesses: transcript gating and wrapper resolution Claude hook records are only restorable when their transcript exists on disk (hookRecordIsRestorable), and claude resume routes through the cmux claude wrapper, which resolves the real binary instead of the captured executable. The relaunch harness silently lost its claude assertion when those behaviors landed (it is not run in CI): fake claude sessions were dropped at index load, and when they did resume the real claude binary ran instead of the fake. Both harnesses now write a transcriptPath for claude sessions, point CMUX_CUSTOM_CLAUDE_PATH at the fake binary, and match resume markers as order-agnostic tokens on one line (the wrapper inserts its own arguments around --resume). * Compact snapshot backup tests behind a shared fixture; refresh swift file length budget * Address review: log corrupt-primary backup fallback, exercise restore-session in corrupt phase Adds a session.restore.primaryUnusable debug-log line when startup falls back from an unusable primary snapshot, and extends the corrupt-snapshot stress phase to run the bundled cmux restore-session verb, asserting it reopens the backed-up session in a new window (window count, since the restored workspaces share ids with the startup fallback restore).
This branch was successfully deployed
1 participant
Summary
session-<bundle>-previous.json(therestore-sessionbackup), so the user's workspaces and agent sessions were unrecoverable.SessionPersistenceStorenow distinguishes a missing snapshot (clean state) from an unusable one (unreadable data, decode failure, schema drift, anomalous empty window list): when the primary is unusable the backup is preserved and startup restore falls back to it, so workspaces and tracked agent sessions come back automatically.tests/test_session_restore_stress_kill_cycles.py: six Claude/Codex/OpenCode sessions across six workspaces, then clean relaunch, second relaunch, relaunch after SIGKILL (autosave path), and relaunch with a corrupted primary snapshot (backup recovery path). Fake agents stay running so every snapshot records the agent as live.SessionPersistenceTests(two-commit structure, commit 1 red / commit 2 green): corrupt primary preserves the backup, startup load recovers from the backup, missing primary still clears the stale backup and does not resurrect it.tests/test_session_relaunch_resumes_agent_sessions.py(not run in CI): claude hook records are only restorable when their transcript exists (hookRecordIsRestorable), and claude resume resolves the binary through the cmux claude wrapper, not the captured executable. Both harnesses now writetranscriptPath, pointCMUX_CUSTOM_CLAUDE_PATHat the fake binary, and match resume markers as order-agnostic tokens.Testing
workspaces=1after relaunch and the-previousbackup deleted.tests/test_session_restore_stress_kill_cycles.pyPASS (all phases: clean relaunch x2, SIGKILL relaunch, corrupt-snapshot recovery with backup preserved andcmux restore-sessionreopening the backed-up session in a new window, all 6 sessions resumed including claude through the wrapper).tests/test_session_relaunch_resumes_agent_sessions.pyPASS post-repair (was failing on claude against unmodified main).cmuxTests/SessionPersistenceTestson AWS M4 Pro (macOS 15.7.4): 135 passed, all 4 new tests passed; red/green proven empirically (the two corrupt-recovery tests fail at the test-only commit f15bf28, pass at head). 4 pre-existingtestHermesAgentHookSurfaceResume*failures on that box are environment-dependent (subrouter bootstrap state) and untouched by this change; the CItestsjob passes.Issues
Note
Medium Risk
Changes core session persistence and startup restore paths where corrupt snapshots previously caused silent data loss; behavior is well-covered by new unit and stress tests but affects all users on launch.
Overview
Fixes session restore when the primary
session-<bundle>.jsonis corrupt or unreadable: startup no longer treats that like a clean slate and stops wiping thesession-<bundle>-previous.jsonbackup used bycmux restore-session.SessionPersistenceStoreaddsSnapshotLoadOutcome(loaded/missing/unusable) vialoadOutcome.syncManualRestoreSnapshotCachenow copies a good primary to the backup, clears the backup only when the primary is truly missing, and leaves the backup alone when the primary exists but is unusable.loadStartupSnapshotis used at launch (replacingload()inAppDelegate) and falls back to the-previousfile when the primary is unusable.Adds
SessionPersistenceTestsfor corrupt-primary backup preservation, startup fallback, and missing-primary behavior. Adds end-to-endtests/test_session_restore_stress_kill_cycles.py(relaunch, SIGKILL, corrupt primary). Updatestest_session_relaunch_resumes_agent_sessions.pyso Claude resume checks usetranscriptPath,CMUX_CUSTOM_CLAUDE_PATH, and order-agnostic scrollback markers.Reviewed by Cursor Bugbot for commit 09904ad. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests