Skip to content

Persist the session continuously so restore survives the exits it exists for - #495

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/session-restore-continuity
Sep 3, 2026
Merged

Persist the session continuously so restore survives the exits it exists for#495
erikdarlingdata merged 1 commit into
devfrom
fix/session-restore-continuity

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

What does this PR do?

Fixes #490.

Session restore had two gaps: a file-backed tab detached into its own window at exit was never persisted, and the open-tab list was written only at clean close — any abnormal exit (crash, task kill, OS "shut down anyway" past the dirty-tab prompt) restored zero tabs. Restore protection evaporated exactly when it was needed.

  • Continuous, debounced persistence. Membership changes write the list as they happen (1s trailing-edge debounce so a burst lands as one write, via the existing AtomicFile path). The trigger hangs on Notice a plan arriving by every route, not just the one with a test (#447) #481's TabContentWatcher — the one subscription that sees every open/close/restore/redock/content-swap, per that PR's own sixteen-call-sites rationale — plus exactly three explicit calls for what the strip cannot show: detach registration, detached-window close, and a tab gaining its first file path in SaveQueryToPath. The debounce also quietly does ordering work: a synchronous write at the watcher's moment would land in the gap between Items.Remove and the detached-register add and drop the detached file.
  • Crash-loop defense preserved and strengthened. RestoreOpenPlans still clears the list — but now BEFORE the first open rather than after the loop, which closes a pre-existing hole: the old ordering meant a crash during restore left the poisoned list intact and looped forever. Each successfully opened tab re-enters via the watcher, and restore ends with a synchronous flush. Net invariant, tested: a file that crashes (or fails) during load never persists; every tab that opened successfully does. A crash mid-restore can lose earlier good tabs for that one start — stated in the doc comment as the accepted cost; the invariant is poison-never-persists.
  • Detached windows join persistence via a second register beside Detached windows discard unsaved query changes without asking #473's prompt-only one (that one is query-only by design; persistence needs detached plans too). Detached paths append after docked tabs; on the next start they return as ordinary docked tabs — remembering THAT a file was open is the data-loss fix, window geometry is a different feature, deliberately not built.
  • Test-host discipline (Add a headless Avalonia harness, and use it on #447 and #448 #451/Stop the test harness from mutating real machine state #487 pattern): no real DispatcherTimer under the harness — the suite shares one dispatcher, and a live timer would tick during a later test and clobber its staged settings. Tests drive an internal flush seam and assert the redirected settings file, not memory.
  • Scratch-buffer content persistence remains deliberately out of scope (tracked separately).

How was this tested?

Eight new tests in SessionPersistenceTests plus RestoreQueryTabsTests updates: open-persists-without-close, close-removes, detached query/plan persisted with redock-exactly-once and detached-close-removes, post-restore immediate repopulation, the poison-exclusion invariant (a well-formed-but-invalid .sqlplan fails restore and is absent while the good file persists), and scratch-gains-file-joins-list. Existing tests whose "restore does not leak" assumption the new rewrite invalidated were fixed with proper seeding/cleanup rather than weakened. Full suite: 453 tests, 452 passed, 1 platform skip, 0 failed — in the worktree and again in the main checkout.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n

)

Session restore had two gaps, both from writing the list only at clean
close (#468's original scope): any abnormal exit -- crash, task kill, an
OS "shut down anyway" past the dirty-tab prompt -- restored zero tabs,
and a file-backed plan or query detached into its own window was never
written down at all, because SaveOpenPlans walked MainTabControl alone.

Membership changes now write the list as they happen, debounced one
second so a burst (restore, Close All) lands as one write. The trigger
rides the #481 TabContentWatcher rather than the call sites, for exactly
that commit's reason: a persist remembered at sixteen call sites gets
forgotten at the seventeenth. The two changes the strip cannot show get
explicit calls -- the detached register (a window closing changes no
tab) and SaveQueryToPath (a scratch gaining its first file changes no
membership).

Detached windows join the collected set through a second register next
to #473's: the prompt register is query-sessions-only because only an
edit can be lost, while persistence needs every FILE-backed detached
window, plans included. Detached entries append after the docked tabs;
on the next start they come back as ordinary docked tabs, deliberately
not re-detached windows.

The crash-loop defense is kept and sharpened: RestoreOpenPlans clears
and saves the empty list BEFORE the first open (it used to clear after
the loop, which only defended against crashes after restore finished),
and each file that opens successfully re-enters through the debounced
writer, which restore flushes synchronously at its end. Net invariant:
a file that crashes the app during load never persists -- it died before
its own re-add -- while everything that opened does. A crash mid-restore
still loses the tabs opened before it (their re-add was pending, the UI
thread never flushed it); accepted, and said so in the code.

OnClosed keeps its save as the final authoritative write, now with the
debounce timer stopped first so no tick lands in a torn-down window.
Under the test host no real timer is armed at all -- the suite shares
one dispatcher, and a stray tick would write one test's tabs over
another's staged state -- so tests drive the flush through a
deterministic seam and assert the redirected settings file (#451/#487).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown

Reviewed the session-persistence changes (MainWindow.FileOps.cs, MainWindow.Tabs.cs, MainWindow.axaml.cs, plus the new/updated tests).

Traced the mechanics closely for the failure modes that matter here — crash/kill mid-session, crash mid-restore, detach/redock ordering, and the scratch-gains-a-path case — and didn't find a correctness bug:

  • The TabContentWatcher.Watch subscription (which now also calls RequestSessionPersist) is wired before OpenFromStartupArgs/RestoreOpenPlans runs in the constructor, so tabs opened during restore do get captured by the debounced-write flag before the synchronous flush at the end of RestoreOpenPlans.
  • RequestSessionPersist is a no-op once IsShuttingDown is set, and OnClosed writes the authoritative SaveOpenPlans() before it force-closes the detached windows below it, so those windows' paths are still in _detachedTabContents at write time — matches the doc comment.
  • The detach path (DetachTabToWindow) removes the tab from MainTabControl.Items and then calls RememberDetachedTabContent, both of which request a persist; since the real writer is debounced (and the test-only path is an explicit flush called after the whole detach completes), both mutations always land in the same write — no window where a detached file briefly vanishes from the persisted list.
  • RestoreOpenPlans's clear-before-load ordering genuinely fixes the crash-loop hole described in the PR body (old code cleared after the loop, so a poison file that crashed mid-restore would wedge every subsequent launch).
  • Test hygiene: the two Seed() call sites in RestoreQueryTabsTests both got matching Unseed() calls, and the new SessionPersistenceTests reset persisted state in every finally, so no cross-test pollution via the shared settings file.

No T-SQL, versioned-project, or Blazor-linked-file changes in this PR, so those repo-convention checks don't apply. Nothing to flag — this looks solid.

@erikdarlingdata
erikdarlingdata merged commit a791a6a into dev Sep 3, 2026
3 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/session-restore-continuity branch September 3, 2026 14:26
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