Skip to content

fix(session): keep the restore breadcrumb where a kill cannot take it - #501

Merged
PathGao merged 2 commits into
masterfrom
fix/session-restore-durable-breadcrumb
Aug 6, 2026
Merged

fix(session): keep the restore breadcrumb where a kill cannot take it#501
PathGao merged 2 commits into
masterfrom
fix/session-restore-durable-breadcrumb

Conversation

@PathGao

@PathGao PathGao commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

The session-restore recovery mechanism (#260, #401) does not survive the
abnormal termination it exists to handle. It is complete, it is tested, and the
record it depends on is kept in the one store this repository has already proved
does not outlive a kill.

What the mechanism promises, and what #201 got

Before reading each document, restore() writes a breadcrumb naming it. A
launch that finds the breadcrumb still set knows the last launch died on that
document and defers it; after MAX_INTERRUPTIONS (3) consecutive interrupted
launches it stops reading documents entirely and hands back the tabs alone.

I traced that promise against windowSession.svelte.ts and it holds: recovery
costs two launches.
Launch 1 claims the pass, writes pending = P, dies on
read_file_content_checked(P). Launch 2 reads the breadcrumb, counts one
interruption, puts P in deferred, reads everything else, finishes. Two.
#201's reporter needed six.

That gap is not in the policy. It is in whether the breadcrumb is there at all
on launch 2.

Three changes

1. Claim the launch before loading the snapshot

savedData was read first, normally via await invoke('load_window_state')
an IPC round trip, because persistState moves the snapshot to the Rust side
and drops the localStorage copies. The breadcrumb was written only after that
returned. A launch killed during the round trip, or during anything before it,
left no trace, so the next one started from zero — and, more importantly, the
give-up counter that is supposed to terminate the loop never advanced. Startup
that cannot record that it failed cannot decide to stop trying.

The claim now goes in first. The case where there turns out to be no snapshot at
all is handled by the existing else branch, which writes
{ running: false, …, interruptions: 0 } and so retires the claim; I confirmed
that rather than assuming it, and there is a test for it below.

2. Route the diagnosis somewhere the user can read it

restore() already computed the answer to "why did Markpad come back without my
documents" — interrupted; deferring <path>, or interrupted with no document to blame — and sent it to onWarning, which MarkdownViewer wires to
console.warn. In a packaged Tauri app nobody can open that console. The
mechanism was diagnosing itself and writing the answer where no one could read
it.

It now also reaches addToast. The seam: windowSession has no language, so it
reports the fact and the viewer picks the wording.

onInterrupted: (interruption: { deferredPath: string | null }) => void

onWarning is unchanged — the console line stays detailed and English, because
that is who reads it.

This adds two English-only keys (toast.restoreInterrupted,
toast.restoreInterruptedDeferred). scripts/i18nCoverage.test.ts requires
every key the source asks for to exist in English and reports per-locale
completeness without enforcing it, so the other 25 locales fall back to English
until someone translates them. Saying that here rather than leaving it to be
found.

3. Persist the breadcrumb the way the snapshot already is

The evidence is this repository's own comment at MarkdownViewer.svelte:629:

setItem is an async message to the WebKit storage process that dies in
transit when the last window's close ends the process (reproduced in QA as
"close secondary first, then main → snapshot gone")

That was diagnosed here, reproduced here, and the snapshot was moved to a
Rust-written file because of it. The breadcrumb was left behind — the one piece
of state whose entire purpose is to survive an abnormal termination, in the store
this codebase had already shown does not survive one. It now takes the same path:
save_restore_progress / load_restore_progress / clear_restore_progress,
writing restore-progress-v1.json beside window-state-v2.json.

restoreInProgressKey stays as a read-once migration path: a breadcrumb an older
build left is still honoured, and dropped as soon as a Rust write succeeds. The
Rust file wins when both exist, so a downgrade and re-upgrade cannot resurrect a
stale localStorage record.

The cost, honestly

This is the part worth arguing with numbers rather than asserting.

Surface: three Tauri commands and one file in app_config_dir, all
main-window-only (restore() returns early for secondary windows).

Latency: 2N + 4 added awaited round trips per launch, N = restorable file
tabs. Two per document (before the read, after it), plus the claim, the final
write, the readProgress at startup and the one in releaseReadableDeferrals.

The write does not reuse atomic_write, deliberately. It keeps the rename,
because fs::write truncates before it writes and an empty breadcrumb parses as
"nothing was interrupted" — the one direction this record must never fail in. It
drops the two fsyncs, because they buy durability past a power cut, and this
record does not need that: after a power cut the breadcrumb is simply absent and
startup behaves exactly as it did before the file existed. The snapshot does need
it, which is why save_window_state keeps atomic_write untouched — that file
is the only record of which documents the user had open.

Measured on this machine (macOS 15.5, APFS SSD, 122-byte payload, 200 iterations
after warm-up):

atomic_write            (temp + fsync + rename + fsync parent)   7.99 ms/call
temp + fsync + rename   (no parent-dir fsync)                    3.85 ms/call
temp + rename           (what this uses)                         0.20 ms/call
fs::write               (torn; rejected)                         0.09 ms/call

So a 10-tab launch pays ~5 ms of file I/O plus 24 IPC round trips, against ~190 ms
if it had reused atomic_write. That is what makes the per-document write
affordable at all; had it cost 8 ms I would have proposed writing the breadcrumb
only at the claim and blaming the whole pass rather than one document, which is
strictly worse recovery.

One deliberate non-fallback: if the Rust write fails, writeProgress reports
through onWarning and does not fall back to localStorage. Two copies that can
disagree is how a stale record gets preferred over a fresh one, and a backend that
cannot write this cannot write the snapshot either — a failure already surfaced at
close. The degraded behaviour is the current behaviour.

Tests

scripts/sessionRestoreResilience.test.ts had 13 tests over this mechanism. All
13 start from a breadcrumb that a dead launch is assumed to have left — which
is exactly the assumption #201 disproves. They pass whatever store the record is
kept in, which is how the mechanism could be complete, tested, and still leave the
reporter relaunching into the same hang. A test that only asserts write order has
the same blind spot.

The five new tests run the kill. launch() models three things about a killed
process:

  1. the call it died in never answers, and nothing after it runs (the stub returns
    a promise that never settles — a rejection would be caught, which is the one
    thing a real kill never allows);
  2. whatever the launch had put in localStorage is gone with it;
  3. whatever reached the backend is still there when the next one starts.

(2) is the pessimistic reading — a real kill sometimes loses the write and
sometimes does not — and it is the right one for a resilience contract: the
guarantee worth having is the one that holds when the flush does not happen. It is
also what this repository already concluded about this exact store.

The contract locked is "a launch killed at any point costs at most one
repeat"
, expressed as launch counts and read counts rather than as writes:

test property
a document that kills the launch reading it is read by exactly one more launch recovery in 2 launches, /poison.md read once
launches killed before the snapshot arrives still count, so startup can give up by itself 3 pre-load deaths ⇒ the 4th reads nothing and still returns every tab
claiming the launch early does not strand a window that had nothing to restore the phantom claim retires itself; a later launch reads everything
the launch that follows an interruption tells the user which document it skipped onInterrupted({ deferredPath }) fires once, naming the path
a launch that was not interrupted tells the user nothing the ordinary case is silent

Plus one Rust test, a_reader_never_sees_a_half_written_breadcrumb: a writer and
a reader thread race over the real file, and every read must be a whole record.
It carries its own anti-vacuity guard (whole_reads > 0) because a reader that
never opened the file would otherwise sail through having checked nothing.

Falsification

Each assertion was broken on purpose, with the tests left intact, to confirm it
discriminates and names the real problem.

broken what failed
breadcrumb back in localStorage never recovered: 5 launches all died and the loop gave up, with /poison.md read 5 times — the launch that died on it left nothing the next one could find
claim moved back after the load after three launches died before they could name a suspect, startup stops opening documents at all (actual ['/a.md','/b.md','/c.md']), and the launch that died during the load is on the record
else branch no longer retires the claim a launch with nothing to restore leaves no claim behind
onInterrupted call removed actual [] / expected [{ deferredPath: '/poison.md' }]
onInterrupted fired unconditionally the ordinary case is silent (actual [{ deferredPath: null }])
write_restore_progressfs::write a launch read a breadcrumb of 256 bytes that is not any record the writer wrote; startup would parse it as 'nothing was interrupted'
English key renamed out from under the call site t() references 1 key(s) English does not define … toast.restoreInterruptedDeferred <- src/lib/MarkdownViewer.svelte
reader pointed at a path that does not exist the reader never read the breadcrumb at all, so it proved nothing

The fs::write row is worth reading twice: a reader really did observe a
256-byte partial file on APFS, so the truncation window is not theoretical.

The 5 in the first row is that test's stopping bound, not a launch count — with
the mechanism broken nothing recovers at all. It is spelled out because an
earlier wording of that message reported the bound as if it were a result, and a
reader reasonably took it for a reproduction of the number in #201's report. The
two are unrelated: his six ended in a working app.

Verification

Against origin/master (382ab8c, which is the v2.7.1 tag — the reporter is
running what this is written against):

                  baseline        this branch
npm run check     0 errors        0 errors, 653 files, 0 warnings
npm test          778 pass        783 pass, 0 fail
cargo test        125 passed      126 passed, 0 failed
npm run build     clean           clean

+5 frontend and +1 Rust are exactly the new tests. cargo clippy and
cargo fmt --check produce the same output as master (one pre-existing
push_str warning; 47 pre-existing fmt diffs, none in the new code).

Not verified

  • I could not reproduce the original hang. The reporter's file now opens
    fine and he no longer has a way to trigger it. This is a mechanism fix argued
    from the code and from the repository's own QA note, not a fix confirmed
    against the failure. What I can show is that the property holds under a
    simulated kill and fails without each change.
  • I did not reproduce the localStorage loss itself. The QA reproduction in
    MarkdownViewer.svelte:629 is for the snapshot at window close. That a
    force-quit mid-restore loses the breadcrumb the same way is the same mechanism
    applied to the same store, not a second observation. If it turns out
    setItem does survive a force-quit, change 3 is insurance and changes 1 and 2
    stand on their own.
  • No packaged build was run. restore-progress-v1.json was never written by
    a real AppHandle; restore_progress_path is exercised only through the same
    app_config_dir call window_state_path already uses. The toast was never
    seen on screen.
  • macOS only. The latency numbers are APFS on one machine. The rename is
    atomic on Windows too (MoveFileExW, per atomic_write's own note) but the
    cost ratio there is unmeasured.
  • The 26-locale wording is English for 25 of them until translated.

Two things found in passing, both left alone

The snapshot is still written once, at graceful close (MarkdownViewer.svelte:629-641).
A freeze-and-kill therefore loses the tab list entirely — which is #153's third
symptom, and probably why #201's reporter got one tab back rather than all of
them. This PR makes that seam more visible (the breadcrumb now outlives a kill
that the snapshot does not) and deliberately does not touch it, because
changing it means revisiting a decision made in #214. Filed separately.

The user is blind for the whole of restore, and this change does not fix
that.
mode is 'loading' from mount until the very end of init() — after
windowSession.restore(), claimTransferredTab(), the ?file= load and
send_markdown_path — and handleKeyDown returns early on mode !== 'app'.
So every document-level shortcut is inert for as long as restore runs, including
on a launch slowly working through a pathological document. From the user's side
that is indistinguishable from a freeze, and it is the window in which the
decision to reach for Task Manager actually gets made — the force-kill that
powers the whole #201 loop.

The toast this PR adds fires after restore. It answers "why did my documents
not come back"; it says nothing during the stretch where the user decides to
kill. Saying something during restore means rendering into the
{#if mode === 'loading'} branch, and the useful signal there is not this notice
(which is about the previous launch) but per-document progress — a different
feature on the same startup path. I left the mode gate alone: it may be
load-bearing for reasons I have not checked, and widening scope here is not worth
it. Noting it because it is the next thing someone should look at.

🤖 Generated with Claude Code

PathGao and others added 2 commits August 6, 2026 22:33
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>
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>
@PathGao
PathGao merged commit 5ab42d6 into master Aug 6, 2026
4 checks passed
@PathGao
PathGao deleted the fix/session-restore-durable-breadcrumb branch August 6, 2026 15:02
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