fix: recover from interrupted session restores - #260
Merged
alecdotdev merged 4 commits intoJul 29, 2026
Merged
Conversation
PathGao
force-pushed
the
codex/recover-interrupted-session-restore
branch
2 times, most recently
from
July 28, 2026 05:46
69ebe22 to
5e44133
Compare
PathGao
force-pushed
the
codex/recover-interrupted-session-restore
branch
from
July 29, 2026 07:06
4fd6f46 to
dcfdeb9
Compare
Collaborator
Author
|
@alecdotdev This branch has been rebased onto the latest #258 branch and force-pushed; the merge conflict is resolved. Validation passed: |
This was referenced Jul 29, 2026
This was referenced Aug 2, 2026
PathGao
added a commit
that referenced
this pull request
Aug 2, 2026
…401) Three ways a restore lost documents, all of them permanent because the trimmed result was written straight back to the snapshot. - A single failed read evicted the tab. A network share not yet mounted, an external drive not plugged in, a file briefly locked - the tab was gone, and plugging the drive back in did not bring it back. The tab now stays, with its buffer marked through the existing `isTruncated` flag: every writer already refuses such a buffer, and `ensureFullContent` already re-reads and clears it, so the tab heals itself the next time it is opened. A dirty buffer is never marked - unsaved text outranks a failed read of its file. - An interrupted restore deleted the whole snapshot. The #260 breadcrumb recorded that a restore was running, not what it was running, so the only available response was collective punishment. It now records the document it was on, so the next launch defers that one and restores everything else. After three interruptions startup restores the tab list without reading any file, which is a stable end state that loses nothing. A deferred path is released once Markpad has read it successfully - a quarantine with no exit is a permanent loss on a longer timescale. - The `'HOME'` sentinel was written into the snapshot, because the filter tested `path !== ''` while `hasRealFilePath()` - used everywhere else - tests for both. Reading it back invoked `read_file_content('HOME')`, which threw, which took the first path above. Both sides now use `hasRealFilePath`; the read side is required because snapshots already on disk contain it. `restore()` no longer deletes the snapshot anywhere, including its outer catch. `discardPersistedState` survives for explicit exit only. Co-authored-by: PathGao <gaoyanbo@gaoyanbodeMacBook-Air.local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 3, 2026
PathGao
added a commit
that referenced
this pull request
Aug 6, 2026
…#501) * fix(session): keep the restore breadcrumb where a kill cannot take it The recovery mechanism from #260/#401 writes a breadcrumb naming each document before it reads it, so the next launch knows which one killed the last one and skips it. By that design recovery costs two launches. #201's reporter needed six. Three things were in the way, in ascending order of how much they mattered: 1. The breadcrumb was written after `load_window_state`, an IPC round trip, so a launch killed during it — or during anything before it — left no trace at all and could not advance the give-up counter that is supposed to end the loop. The claim now goes in first. A launch that turns out to have no snapshot retires its own claim in the `else` branch, so claiming early cannot accumulate phantom strikes. 2. The mechanism's diagnosis — "interrupted; deferring <path>" — went only to `console.warn`, which in a packaged build nobody can open. It now also reaches `addToast`. The session has no language, so it reports the fact (`onInterrupted({ deferredPath })`) and the viewer picks the wording through `t()`; the console line stays detailed and English. Two new English-only keys; the other 25 locales fall back. 3. The breadcrumb lived in localStorage. This repository had already found, and reproduced in QA, that `setItem` is an async message to the WebKit storage process that dies in transit when the process does — which is why the snapshot was moved to a Rust-written file. The one piece of state whose entire purpose is to outlive an abnormal termination was left in the store that does not. It now has the same durable path: `save/load/clear_restore_progress`, a file beside `window-state-v2.json`. That write does not reuse `atomic_write`. The rename it does keep, because `fs::write` truncates first and an empty breadcrumb parses as "nothing was interrupted" — the one direction this record must never fail in. The two fsyncs it drops buy durability past a power cut, which this record does not need (after one it is simply absent and startup behaves as it did before the file existed) and which is not free on a path that runs once per document at every launch: measured on macOS/APFS, ~8ms per call against ~0.2ms for temp-file-and-rename. The new tests run the kill instead of assuming its result. The thirteen existing ones all start from a breadcrumb a dead launch is *assumed* to have left, which is exactly the assumption #201 disproves — they pass whatever store the record is kept in. `launch()` models a killed process: the call it died in never answers, whatever it had put in localStorage is gone with it, and whatever reached the backend is still there. The contract is "a launch killed at any point costs at most one repeat", not "the breadcrumb is written". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(session): say "never recovered", not "recovery took 5 launches" The poison-document test loops until a launch restores or the bound runs out, and reported `outcomes.length` as a launch count either way. When the mechanism is broken nothing recovers, so that number is the loop's own cap — and the message read as a result. It misled a reader into believing the suite had reproduced the launch count from #201's report; the two numbers are unrelated, one ending in success and the other in the bound. The message now branches on whether anything actually recovered. The assertion is unchanged and was already correct. The bound moves 6 -> 5 and is named. Six matched the number in the report by coincidence, which is most of how the confusion started; any value comfortably above the two the design promises does the job. This is the same hazard `assert.ok(settled || died, …)` already guards one level down — "every assertion below would be measuring the loop bound instead of the mechanism". The reasoning applied to the outer loop too; it just never reached the wording. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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 #259.
Summary
Validation