Skip to content

fix(session): stop startup from throwing away tabs it could not read - #401

Merged
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/session-restore-resilience
Aug 2, 2026
Merged

fix(session): stop startup from throwing away tabs it could not read#401
PathGao merged 1 commit into
sftwrdotdev:masterfrom
PathGao:fix/session-restore-resilience

Conversation

@PathGao

@PathGao PathGao commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Three ways a restore lost documents. All three were permanent, because the trimmed result was written straight back to the snapshot — the failure of one launch became the state of every launch after it.

1. One failed read evicted the tab

The per-tab catch called dropRestoredTab (wired to closeTab), and the loop then re-serialised. A network share not yet mounted, an external drive not plugged in, a file briefly locked by another program — the tab was gone, and plugging the drive back in did not bring it back.

The tab now stays, with its path, and its empty buffer is marked through the existing isTruncated flag rather than a new state. That choice is the point:

  • isTruncated already means "this buffer is not the whole document", and every writer already refuses itsaveContent/saveContentAs bail out, canTransfer/canDetach refuse. A new flag would need each of those to learn about it, which is one chance per call site to forget, and the cost of forgetting is an empty buffer written over the user's file.
  • ensureFullContent already re-reads the file and clears the flag when the tab is opened for editing, so the tab heals itself once the drive is back. No new recovery machinery.
  • It matches the precedent set by fix: preserve tabs after document read errors #293 in documentSession.loadMarkdown: a read failure keeps the buffer rather than closing the tab.

A dirty buffer is never marked — unsaved text outranks a failed read of its file.

2. An interrupted restore deleted the whole snapshot

Any truthy restoreInProgressKey triggered discardPersistedState(), which clears both localStorage keys and invokes clear_window_state. The whole session record, gone.

The #260 breadcrumb was right in intent and too coarse in shape: it recorded that a restore was running, not what it was running, so the only response available was collective punishment. It now records:

{ running: boolean; pending: string | null; deferred: string[]; interruptions: number }

pending is written before each document's read and cleared after it, so a crash leaves the breadcrumb sitting on the culprit. The next launch defers that one path and restores everything else.

  • Termination: after three interruptions, startup restores the tab list and reads no file at all. That end state is stable, crash-free, and loses nothing. Three strikes distinguishes a poison file from a user force-quitting during startup.
  • Release: persistState() drops any deferred path whose tab now holds real content — Markpad read it successfully at some point, so it is not the problem any more. Without this, one bad startup would cost that file automatic restore forever; a quarantine with no exit is a permanent loss on a longer timescale.
  • Migration: the legacy 'true' value fails JSON parsing and reads as "interrupted, no suspect" — it costs one retry instead of the session.

restore() no longer deletes the snapshot anywhere, including its outer catch, which now only reports. discardPersistedState survives for explicit exit — the user's own choice.

3. The 'HOME' sentinel was written into the snapshot

serializeState filtered t.path !== '', while addHomeTab sets path: 'HOME'. tabFileActions.hasRealFilePath() tests for both, and is used everywhere except here — two spellings of "is this a real file", one of them wrong.

Reading it back invoked read_file_content('HOME'), which threw, which took path 1 above. A window whose only tab was HOME restored empty.

Both sides now use hasRealFilePath. The read side is not optional: snapshots already on users' disks contain the sentinel. A third guard in the restore loop means 'HOME' cannot reach the backend by any route.

These three ship together deliberately. Fix 1 without fix 3 would turn a legacy HOME entry from something silently dropped into a permanently unreadable phantom tab that re-persists every launch — a regression. They are the same failure: startup losing tabs it should have kept.

Tests

16 new tests across two files, executing the real code — both tabs.svelte.ts and windowSession.svelte.ts import cleanly under node --test --import tsx with the rune stubs the repo already uses in truncatedBufferGuard.test.ts, plus a stubbed __TAURI_INTERNALS__ and localStorage. sessionRestoreResilience.test.ts drives createWindowSession().restore() against a fake disk and a fake snapshot store, then asserts on the tab list, the persisted snapshot, and the breadcrumb.

Counter-proof, source files stashed and tests kept:

tests pass fail
master + these tests 419 398 21
Final 419 419 0

Named failures include: a file that cannot be read keeps its tab · the failed read is not written back into the snapshot · an interrupted restore keeps the snapshot instead of deleting it · an interrupted restore names the document it was on · a deferred document is released once Markpad has read it · a breadcrumb from an older build no longer wipes the session · a session snapshot never carries the home tab · a HOME entry in an older snapshot is never read as a file.

Two existing test files asserted the old behaviour verbatim (filter((t) => t.path !== ''), dropRestoredTab(tab.id), restoreInProgressKey … discardPersistedState()). They could not both stay and the bugs be fixed; only the assertions naming the old behaviour were rewritten, every unrelated assertion kept, each pointed at the behavioural test that now covers it.

npm run check   432 files, 0 errors, 0 warnings
npm test        419 / 419
cargo test       98 / 98   (Rust untouched)

Not covered

  • An unreadable tab renders blank with no explanation. Nothing is destroyed — edit mode is blocked by ensureFullContent and saving is refused — but the tab looks like an empty document. A "couldn't read this — retry" affordance needs MarkdownViewer.svelte and a new i18n key; deliberately out of scope here.
  • No retry on tab click. The re-read happens on entering edit or split view, not on activation.
  • MAX_INTERRUPTIONS = 3 and MAX_DEFERRED = 8 are judgement calls, not measurements. Chrome and Firefox handle the same situation by asking the user after a crashed session, which is the better answer and needs UI.
  • Case-insensitive filesystems: paths are compared exactly, so /notes/A.md and /notes/a.md are distinct. Same limitation refuseIfLossilyDecoded already documents; closing it needs backend canonicalisation.

🤖 Generated with Claude Code

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 sftwrdotdev#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: Claude Opus 5 <noreply@anthropic.com>
@PathGao
PathGao merged commit 72d63db into sftwrdotdev:master Aug 2, 2026
4 checks passed
@PathGao
PathGao deleted the fix/session-restore-resilience branch August 2, 2026 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant