Persist scratch query buffers so a crash cannot take never-saved work

[erikdarlingdata]

What does this PR do?

Fixes #496 — the last gap in session survival: a scratch tab (typed, never saved) lost its content on any abnormal exit. Every interactive discard route already prompts (#469/#477), so the design center is the distinction between chose to discard and never got to choose: a buffer the user answered "Don't Save" about dies; one they never got asked about survives a crash and comes back as a dirty scratch tab.

• One file per scratch buffer (.sql, so a user digging through their profile recognizes it) under a scratch directory beside the settings file, written atomically, riding the Stop the test harness from mutating real machine state #487 test-host redirection. Size-capped at 1MB so a pathological paste can't grind the writer. Privacy reasoned in the class doc: same profile, same sensitivity class as the recent-plans list and stored plan XML already there.
• Inline scratch:<guid> entries on the existing open_tabs list, preserving strip order (no second list reinventing interleaving). Compatibility is free and pinned the way Surface the running instance on a bare second launch instead of clobbering settings #494's sentinel lesson taught — as a string property, not a vacuous File.Exists: an old build's path guard silently skips the entries (colon is illegal in Windows file names, and the app only writes absolute paths to that list); a new build reading an old list sees plain paths and behaves exactly as before. A prefixed entry whose tail is not a GUID is treated as a path, so a file literally named scratch:something on a colon-tolerant filesystem still opens.
• Content persists on a 2s idle debounce (keystroke-scale, separate from Persist the session continuously so restore survives the exits it exists for #495's 1s membership debounce), chained so a first-persist's minted entry lands together with its buffer; every Persist the session continuously so restore survives the exits it exists for #495 flush point drains both; no real timers under the test host (shared-dispatcher reasoning, same as Persist the session continuously so restore survives the exits it exists for #495).
• Delete-on-choice hooks the resolution, not the dialog: the docked and detached prompt switches had drifted into near-twins, and rather than a third copy they now share one ResolveCloseChoiceAsync — so tab close, detached close, and the shutdown/restart walks honor "Don't Save" identically by construction. Save retires the buffer inside SaveQueryToPath. Consequence, tested: after a clean close, zero scratch buffers remain, because every one was chosen about.
• Orphan sweep + poison parity with Persist the session continuously so restore survives the exits it exists for #495: unreferenced buffer files are swept at startup; a buffer that fails to load during restore is skipped, not re-added, and swept.
• Detached scratch windows persist like docked ones. Scope fence stated in code: only scratch content is persisted — unsaved edits to file-backed tabs remain guarded by prompts, which is Persist scratch query buffer content across restarts #496's scope, not an oversight.

One design-forced product improvement: scratch-persistence hooks subscribe only sessions that are scratch at hook time (a file-backed session can never become scratch), which kept the #473 glyph-leak reflection test's exact subscriber counts — fixed in the product rather than by loosening the test.

How was this tested?

Thirteen new tests in ScratchBufferPersistenceTests covering: idle-flush persistence without close, restore recreating a dirty scratch tab with identical content and the same GUID, interleaved entry order, Don't-Save deletion, Save converting entry to path and retiring the buffer, the clean-close zero-buffers invariant, orphan sweep, corrupt-buffer poison parity, the size cap, old-format plain-path lists restoring unchanged, the scratch: string-property compat pin, and detached scratch persistence. Full suite at dev tip: 484 tests, 483 passed, 1 platform skip, 0 failed — twice in the worktree and once in the main checkout.

🤖 Generated with Claude Code

https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n

[claude]

Reviewed the scratch-buffer persistence implementation (ScratchBufferStore, MainWindow.ScratchPersist.cs, and the open_tabs/ResolveCloseChoiceAsync changes).

No correctness, crash, or security issues found. Specifically checked and confirmed correct:

• Ordering/races around mint-then-list: PersistScratchBuffer mints the buffer id and writes the file before RequestSessionPersist() fires, and FlushScratchBuffers() runs before the pending-list check in FlushSessionPersist, so a scratch:<guid> entry is never written before its backing file exists.
• Drop-on-choice reentrancy: DropScratchBuffer is called from multiple paths that can overlap for the same session (ResolveCloseChoiceAsync → Close, then TryCloseTabAsync's own check; detached onClosing after a confirmed guard). It's correctly idempotent (if (session.ScratchBufferId is not { } id) return;), so the redundant calls are harmless no-ops.
• Save-in-flight race: a Save As that lands between a scratch tab being queued for persist and the debounce flushing is handled — PersistScratchBuffer re-checks SourceFilePath != null at flush time and no-ops, since SaveQueryToPath already dropped the buffer.
• Startup ordering: RestoreOpenPlans snapshots savedTabs before clearing/saving the list, sweeps orphans from that snapshot, then restores — so the orphan sweep and the poison-defense clear can't race each other or delete a buffer that's about to be restored.
• Hook timing: HookScratchPersistence's scope-fence check (SourceFilePath != null → skip) is safe against load-order because every caller that opens a file-backed session (LoadSqlFile, plan loads) sets SourceFilePath before CreateTab runs, so a file tab is never transiently treated as scratch.
• Backward/forward compat: the scratch: prefix can't collide with a real entry the app itself would ever write (always absolute paths), and old builds skip the new entries via their existing File.Exists guard — verified against both directions in ScratchBufferPersistenceTests.

Test coverage is thorough — debounce/flush ordering, orphan sweep, poisoned/unreadable buffers, the size cap, detached windows, and old↔new list compatibility are all exercised via the redirected test-host storage.

No T-SQL, plan-XML parsing, or PlanViewer.Ssms/PlanViewer.Web files are touched by this PR, so those repo-convention checks don't apply here.

[claude]

Reviewed. This is confined to PlanViewer.App (+ its tests), so the Ssms version-bump and Blazor linked-Compile Include conventions don't apply, and there's no plan-XML/T-SQL surface touched.

Traced the full scratch-buffer lifecycle — hook/hydrate/debounce/flush/drop across tab close, detached close, save, save-as, shutdown, and restart — and it holds together:

• The scope fence (session.SourceFilePath != null bail) is checked consistently at every mutation site, so a file-backed session's buffer machinery is correctly inert.
• DropScratchBuffer is idempotent and correctly ordered relative to IsShuttingDown (the OnClosed drain writes SaveOpenPlans() directly rather than relying on the gated RequestSessionPersist, so nothing is lost when the debounce flag is suppressed at shutdown).
• Orphan sweep + poison-entry handling mirrors Persist the session continuously so restore survives the exits it exists for #495's existing discipline (unreferenced files collected at startup, failed loads skipped-not-readded-and-swept).
• Old→new and new→old list compatibility is genuinely safe: the scratch: prefix contains a colon, which can't appear in a written path, so an old build's File.Exists guard skips these entries silently — and this is pinned with a shape assertion instead of a File.Exists check that would be vacuous on colon-tolerant filesystems.
• Test coverage is unusually complete: compat in both directions, size cap (write + shed-when-over-cap), detached window persistence, orphan sweep, and poisoned/unreadable buffer entries are all exercised against real files under the redirected test-host directory.

No blocking findings. Nice work tracing the "chose to discard vs. never got to choose" distinction through every code path.

[claude]

Reviewed. This PR doesn't touch execution-plan XML parsing, operator-tree walking, or any generated T-SQL, so the top review priorities (untrusted plan XML, SQL injection via string concatenation) don't apply here — the surface is purely tab-session lifecycle in the Avalonia app. No Directory.Build.props/vsixmanifest version bump needed (no Ssms/Core/Web changes), no new NoWarn.

Traced the scratch-buffer lifecycle in detail (ScratchBufferStore, MainWindow.ScratchPersist.cs, the ResolveCloseChoiceAsync consolidation, and the RestoreOpenPlans/orphan-sweep ordering):

• Buffer identity (ScratchBufferId) is minted only at first persist and correctly guarded from re-arming after DropScratchBuffer (pending-set removal happens before the delete).
• SaveQueryToPath drops the buffer after assigning SourceFilePath, so the DirtyStateChanged fired by the trailing MarkClean() is correctly ignored by the scratch hook's SourceFilePath == null guard — no re-mint-after-save race.
• The IsShuttingDown gating is consistent: OnClosed calls FlushScratchBuffers() directly (bypassing FlushSessionPersist's early-return-on-shutdown) so the final drain still runs, and RequestSessionPersist() calls made from inside that drain are safe no-ops because SaveOpenPlans() right after reads live session state, not the pending flag.
• Orphan sweep computes its referenced set from the pre-restore snapshot before any new sessions are created, so there's no race between the sweep and TryRestoreScratchTab minting matching IDs.
• The scratch: prefix / colon-illegal-in-Windows-filenames compatibility argument for old builds checks out, and is correctly tested as a string-shape pin rather than a vacuous File.Exists.
• Detach/redock and the shutdown force-close path for detached windows are correctly distinguished (onClosing vs onRedock callbacks), avoiding the double-drop / lost-buffer scenarios the code comments call out.

Didn't find a correctness bug or crash path. Test coverage (13 new tests) matches the scenarios actually exercised: idle-flush, restore identity, interleaved ordering, Don't-Save deletion, save-retires-buffer, clean-close invariant, orphan sweep aging, poison-load parity, size cap, old-format compat, and detached persistence. Nothing further to flag.