Repository navigation
Session restore stress: recover from corrupt snapshot via -previous backup #5914
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1868,13 +1868,29 @@ struct AppSessionSnapshot: Codable, Sendable { | |
| } | ||
|
|
||
| enum SessionPersistenceStore { | ||
| enum SnapshotLoadOutcome { | ||
| case loaded(AppSessionSnapshot) | ||
| /// No snapshot file on disk: a genuinely clean state. | ||
| case missing | ||
| /// A snapshot file exists but cannot be restored (unreadable data, | ||
| /// decode failure, schema version drift, or an anomalous empty | ||
| /// window list; empty states remove the file instead of writing it). | ||
| case unusable | ||
| } | ||
|
|
||
| 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) | ||
| } | ||
|
|
||
| static func load(fileURL: URL? = nil) -> AppSessionSnapshot? { | ||
| guard let fileURL = fileURL ?? defaultSnapshotFileURL() else { return nil } | ||
| guard case .loaded(let snapshot) = loadOutcome(fileURL: fileURL) else { return nil } | ||
| return snapshot | ||
| } | ||
|
|
||
|
|
@@ -1920,13 +1936,57 @@ enum SessionPersistenceStore { | |
| return load(fileURL: fileURL) | ||
| } | ||
|
|
||
| static func syncManualRestoreSnapshotCache( | ||
| bundleIdentifier: String? = Bundle.main.bundleIdentifier, | ||
| appSupportDirectory: URL? = nil | ||
| ) { | ||
| guard let backupURL = manualRestoreSnapshotFileURL( | ||
| bundleIdentifier: bundleIdentifier, | ||
| appSupportDirectory: appSupportDirectory | ||
| ) else { return } | ||
| guard let primaryURL = defaultSnapshotFileURL( | ||
| bundleIdentifier: bundleIdentifier, | ||
| appSupportDirectory: appSupportDirectory | ||
| ) else { return } | ||
| switch loadOutcome(fileURL: primaryURL) { | ||
| case .loaded(let snapshot): | ||
| _ = save(snapshot, fileURL: backupURL) | ||
| case .missing: | ||
| removeSnapshot(fileURL: backupURL) | ||
| case .unusable: | ||
| // The primary snapshot exists but cannot be restored. Keep the | ||
| // backup: it is the only remaining recovery path for the user's | ||
| // sessions (startup fallback and `cmux restore-session`). | ||
| break | ||
| } | ||
| } | ||
|
|
||
| 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: | ||
| let backup = loadReopenSessionSnapshot( | ||
| bundleIdentifier: bundleIdentifier, | ||
| appSupportDirectory: appSupportDirectory | ||
| ) | ||
| #if DEBUG | ||
| cmuxDebugLog( | ||
| "session.restore.primaryUnusable path=\(primaryURL.path) " + | ||
| "backupRecovered=\(backup != nil ? 1 : 0)" | ||
| ) | ||
| #endif | ||
| return backup | ||
| } | ||
| } | ||
|
Comment on lines
+1964
to
1990
Contributor
There was a problem hiding this comment.
When 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!
Contributor
Author
There was a problem hiding this comment. Done in aa06b13: |
||
|
|
||
| static func defaultSnapshotFileURL( | ||
|
|
||
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
loadOutcomeisinternalbut not exercised directly by testsloadOutcome(fileURL:)andSnapshotLoadOutcomeare bothinternal, which means any@testable import cmuxconsumer 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 forloadOutcometo be wider thanprivate. Making itprivate static funcwould 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declining:
loadOutcomeis 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.