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 #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
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>
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