diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs b/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs index 1ac26c98..bad58e2b 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs @@ -162,7 +162,7 @@ private void OnContextMenuOpening(object? sender, System.ComponentModel.CancelEv /// (or empty), and a dirty-but-empty one is a buffer the user deleted everything out of — /// replacing nothing loses nothing, so both stay as frictionless as they always were. /// Split out pure so the decision is testable without a dialog to click, the same trade - /// CollectOpenTabPaths made for the session-restore list. + /// CollectOpenTabEntries made for the session-restore list. /// internal static bool ReplaceNeedsConfirmation(bool isDirty, string? currentText) => isDirty && !string.IsNullOrEmpty(currentText); diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs index daf4fd40..b2934842 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs @@ -39,6 +39,22 @@ public partial class QuerySessionControl : UserControl /// public string? SourceFilePath { get; set; } + /// + /// Identity of this session's persisted scratch buffer (#496), or null while it has + /// none. Assigned by MainWindow the first time a never-saved session's content is + /// actually written to the scratch store — not at construction, so an empty tab never + /// mints a buffer — and carried back onto the restored session at the next start, which + /// is what makes a restored scratch CONTINUE its buffer instead of forking a new one. + /// Cleared when the buffer is deleted: the user chose its fate at a prompt (Don't Save, + /// or a save that moved the content into a real file), or there is nothing unsaved left + /// to protect. + /// + /// On the session rather than the tab for the same reason + /// is: detach discards the TabItem and the session lives on in its own window (#473), + /// and its buffer identity has to travel with it. + /// + internal Guid? ScratchBufferId { get; set; } + /// /// The encoding the file behind declared with its byte order /// mark, or null for a BOM-less file and for a scratch session — both of which save as diff --git a/src/PlanViewer.App/MainWindow.FileOps.cs b/src/PlanViewer.App/MainWindow.FileOps.cs index 3b8c70ca..4ecb5ce2 100644 --- a/src/PlanViewer.App/MainWindow.FileOps.cs +++ b/src/PlanViewer.App/MainWindow.FileOps.cs @@ -188,6 +188,15 @@ the trade every editor that saves this way makes. */ UTF-8-without-BOM this always wrote. */ AtomicFile.WriteAllText(path, session.QueryEditor.Text, session.SourceFileEncoding); session.SourceFilePath = path; + /* #496: a successful save is the user CHOOSING where this content lives — the + real file just written — so the scratch buffer that was protecting it retires + here, at the moment the choice lands. After the SourceFilePath assignment + above on purpose: from this line on the session is file-backed, its edits are + the prompts' job (the scope fence), and the persist below writes the list + with the path where the scratch: entry used to be. A file-backed + session was never a scratch, has no buffer id, and passes through as a + no-op. */ + DropScratchBuffer(session); /* #490: the one way a tab's place in the session-restore list changes while tab membership stays constant — a scratch gaining its first file, or Save As moving an existing one. The tab watcher sees neither (no tab was added, removed, or @@ -468,9 +477,14 @@ private bool ValidatePlanXml(string xml, string label) } /// - /// The file behind every open tab that has one, in tab order, then the file behind every - /// detached window that has one, in detach order. Plans and queries both, since - /// answers for either shape. + /// The session-restore entry for every open tab that has one, in tab order, then for + /// every detached window that has one, in detach order. Plans and queries both, since + /// answers for either shape — and since #496 the + /// entries are not all paths: a scratch tab with a persisted buffer rides along as + /// scratch:<guid>, IN PLACE, so the strip order the user arranged survives + /// a restart with scratch tabs interleaved among the files exactly where they were + /// (deliberately better than #495's append-after compromise, which was about detached + /// windows, not about tabs sitting between other tabs). /// /// /// Separate from so a test can assert what would be @@ -484,37 +498,37 @@ private bool ValidatePlanXml(string xml, string label) /// a file was open is the data-loss fix, remembering window geometry is a different /// feature, deliberately not built here. /// - internal List CollectOpenTabPaths() + internal List CollectOpenTabEntries() { - var paths = new List(); + var entries = new List(); foreach (var item in MainTabControl.Items) { if (item is not TabItem tab) continue; - var path = GetTabFilePath(tab); - if (!string.IsNullOrEmpty(path)) - paths.Add(path); + var entry = GetContentSessionEntry(tab.Content as Control); + if (!string.IsNullOrEmpty(entry)) + entries.Add(entry); } foreach (var content in _detachedTabContents) { - var path = GetContentFilePath(content); - if (!string.IsNullOrEmpty(path)) - paths.Add(path); + var entry = GetContentSessionEntry(content); + if (!string.IsNullOrEmpty(entry)) + entries.Add(entry); } - return paths; + return entries; } /// - /// Saves the file paths of all currently open file-based tabs, plans and queries alike, - /// docked and detached alike (#490). + /// Saves the restore entries of all currently open tabs — file paths, and since #496 + /// scratch buffer entries — docked and detached alike (#490). /// private void SaveOpenPlans() { _appSettings.OpenTabs.Clear(); - _appSettings.OpenTabs.AddRange(CollectOpenTabPaths()); + _appSettings.OpenTabs.AddRange(CollectOpenTabEntries()); AppSettingsService.Save(_appSettings); } @@ -585,7 +599,21 @@ private void FlushSessionPersist() { _sessionPersistTimer?.Stop(); - if (!_sessionPersistPending || IsShuttingDown) + if (IsShuttingDown) + return; + + /* #496: the list and the buffers it references travel together — any moment the + membership list could be written is a moment the scratch content backing its + scratch: entries must already be on disk, or a crash right after the write + leaves entries pointing at stale buffers. Draining the content writer here also + means every #495 flush point (end of restore, the membership debounce, the test + seam) drains scratch for free. Note the ordering dependency: this can mint a + first buffer id and set _sessionPersistPending, which is exactly why it runs + before the pending check below — the entry the mint created gets written in the + same flush, not a debounce later. */ + FlushScratchBuffers(); + + if (!_sessionPersistPending) return; _sessionPersistPending = false; @@ -609,7 +637,15 @@ private void FlushSessionPersist() /// list current by now anyway, but "usually" is a debounce interval wide; this write is /// what makes the restart exact. /// - internal void PersistSessionForRestart() => SaveOpenPlans(); + internal void PersistSessionForRestart() + { + /* #496: same reasoning as the write itself, one layer down — the restart skips + OnClosed, so this is the last chance for scratch content typed inside the content + debounce to reach disk, and the list written below must reference buffers that + exist. */ + FlushScratchBuffers(); + SaveOpenPlans(); + } /// /// Restores the tabs from the previous session. Skips files that no longer exist. @@ -637,8 +673,23 @@ private void FlushSessionPersist() /// is the other half of the deal — once restore completes, the rebuilt list is on disk /// immediately rather than a debounce-interval later, so the common case (restore fine, /// crash any time afterwards) loses nothing. + /// + /// Scratch entries (#496). The list routes three ways now: a + /// scratch:<guid> entry recreates a dirty query tab from its persisted + /// buffer, a plain path opens by extension as always, and an old build reading a list + /// with scratch entries in it skips them on its File.Exists guard (see + /// for why that is guaranteed). Scratch buffers share + /// the poison defense wholesale — the entry is already off the cleared list before its + /// buffer is read, and a buffer that fails to load is skipped, never re-added, and + /// deleted (). /// - private void RestoreOpenPlans() + /// + /// Whether an empty restore opens a fresh query tab. False when the caller is about to + /// open a file-argument on top (#496 review) — the fallback exists so a bare launch + /// never greets the user with an empty window, and a launch that carries a file is not + /// that. + /// + private void RestoreOpenPlans(bool createFallbackTab = true) { /* Snapshot first: SaveOpenPlans and this method share the live list, and the clear below would otherwise empty the very thing being iterated. */ @@ -647,13 +698,36 @@ below would otherwise empty the very thing being iterated. */ _appSettings.OpenTabs.Clear(); AppSettingsService.Save(_appSettings); + /* #496 orphan sweep, from the just-read snapshot and independent of the poison + clear above: buffers are deleted the moment the user chooses their fate, so a + file no entry references is debris by definition — a buffer stranded by a crash + in the write-buffer-then-write-list gap, an AtomicFile .tmp, a buffer whose entry + a previous poisoned restore dropped. Before the restore loop, so what the loop is + about to read (the referenced set) is exactly what the sweep keeps. */ + var referencedScratch = new HashSet(); + foreach (var entry in savedTabs) + { + if (ScratchBufferStore.TryParseEntry(entry, out var referencedId)) + referencedScratch.Add(referencedId); + } + ScratchBufferStore.SweepAllExcept(referencedScratch); + var restored = false; + var restoredScratch = new HashSet(); - foreach (var path in savedTabs) + foreach (var entry in savedTabs) { - if (File.Exists(path)) + if (ScratchBufferStore.TryParseEntry(entry, out var scratchId)) { - OpenFileByExtension(path); + /* Add() doubling as the seen-check: a list corrupted into naming the same + buffer twice must not open two tabs continuing one file — their flushes + would silently overwrite each other forever after. */ + if (restoredScratch.Add(scratchId) && TryRestoreScratchTab(scratchId)) + restored = true; + } + else if (File.Exists(entry)) + { + OpenFileByExtension(entry); restored = true; } } @@ -662,7 +736,7 @@ below would otherwise empty the very thing being iterated. */ down NOW so a crash a moment after startup still finds the session on disk. */ FlushSessionPersist(); - if (!restored) + if (!restored && createFallbackTab) { // Nothing to restore — open a fresh query editor like before NewQuery_Click(this, new RoutedEventArgs()); diff --git a/src/PlanViewer.App/MainWindow.ScratchPersist.cs b/src/PlanViewer.App/MainWindow.ScratchPersist.cs new file mode 100644 index 00000000..3433e82d --- /dev/null +++ b/src/PlanViewer.App/MainWindow.ScratchPersist.cs @@ -0,0 +1,376 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Runtime.CompilerServices; +using Avalonia.Controls; +using Avalonia.Threading; +using PlanViewer.App.Controls; +using PlanViewer.App.Services; + +namespace PlanViewer.App; + +public partial class MainWindow : Window +{ + // ── Scratch buffer content persistence (#496) ───────────────────────── + + /* #495 made the open-tab LIST survive abnormal exits, which brought back every tab that + had a file behind it. The remaining loss was the tab that never had one: a scratch + query — typed, never saved — kept its place on nothing and lost its CONTENT to any + crash, task kill, or OS "shut down anyway". Every interactive way to discard that + content already stops and asks (#462/#469/#473/#477), so the design center here is + the gap those prompts cannot cover: + + a buffer the user CHOSE to discard dies; + a buffer they NEVER GOT TO CHOOSE about survives. + + Concretely: content is written continuously (debounced) to one file per scratch tab, + the tab enters the #495 session list as a scratch: entry in strip order, and + the buffer file is deleted at exactly the moments the user answers for it — Don't + Save at any prompt, or a save that moves the content into a real file. After a clean + close, every scratch buffer is therefore gone, because every one of them was chosen + about; buffers exist on disk only after an exit nobody was asked about. + + SCOPE FENCE, on purpose: only SCRATCH tabs' content is persisted. Unsaved edits to a + FILE-backed tab stay guarded by the prompts alone — the file is the durable copy the + user opted into, and shadowing every open file's edits is a different feature with + different questions (staleness against on-disk changes, most of all). That is #496's + issue scope, not an oversight. */ + + /// + /// How long the writer waits after the last edit before writing a scratch buffer down. + /// Deliberately its own debounce, longer than : + /// membership changes are click-scale and rare, content changes are keystroke-scale and + /// constant, and reusing the membership timer would either write the settings file on a + /// typing cadence or slow membership writes to a typing idle. Two timers, two cadences, + /// one flush discipline (each flush point drains both — see ). + /// + private static readonly TimeSpan ScratchPersistDebounce = TimeSpan.FromSeconds(2); + + /// + /// The longest an unflushed content change may wait while the debounce keeps being + /// restarted by further typing — see the cap in . + /// + private static readonly TimeSpan ScratchPersistMaxLatency = TimeSpan.FromSeconds(10); + + /// When the oldest currently-unflushed content change arrived; null when drained. + private DateTime? _scratchPersistOldestPending; + + /// + /// A scratch buffer larger than this is not persisted. A pathological paste must not + /// grind the idle writer — rewriting megabytes to disk two seconds after every + /// keystroke — so past the cap that one tab simply behaves as it did before #496: + /// prompts guard it, crashes lose it. Chars rather than bytes because the check has to + /// be free on every flush; for SQL text the two are within a small factor of each other, + /// and the cap is a courtesy threshold, not a contract. + /// + private const int MaxScratchPersistChars = 1024 * 1024; + + /// Trailing-edge debounce for the content writer; every request restarts it. + private DispatcherTimer? _scratchPersistTimer; + + /// + /// The scratch sessions whose content changed since the last flush. A set keyed on the + /// session (not the tab) because content identity lives on the session — a detached + /// scratch window (#473) edits the same session object and lands in the same set, which + /// is the whole of how detached scratch persistence works. + /// + private readonly HashSet _scratchPersistPending = new(); + + /// + /// Which sessions already have the persistence subscription, so the re-subscription + /// path (redock rebuilds a tab around a living session via CreateTab) does not stack a + /// second handler. ConditionalWeakTable for the same reason _tabDirtyGlyphUnhooks is + /// one: the bookkeeping must not outlive the session it is about. + /// + private readonly ConditionalWeakTable _scratchPersistHooked = new(); + + /// + /// Wires a query session's edits into the scratch content writer. Called from + /// — the one place every top-level session passes through — + /// rather than at each construction site, the same sixteen-call-sites reasoning as the + /// #495 tab watcher. Idempotent per session, because redock passes a session through + /// CreateTab a second time. + /// + /// The subscription is never taken back off: unlike the glyph handler it closes + /// over no TabItem, only the session and this window, so it pins nothing a closed tab + /// should release — and a DETACHED session must keep persisting (#496's sixth + /// requirement), which is exactly the case an unhook-on-detach would break. + /// + private void HookScratchPersistence(QuerySessionControl session) + { + /* Only sessions that are scratch NOW. SourceFilePath moves null→path exactly once + (the save) and never back, so a session arriving here file-backed can never need + this hook later — the scope fence again, applied at subscription time. It also + keeps a file tab's DirtyStateChanged invocation list exactly the one subscription + the #473 glyph-leak test counts by reflection; a second subscriber there would + read as the leak that test exists to catch. A scratch that gains a file KEEPS its + subscription (nothing unhooks it), which is why the fire-time guard below still + exists: it is what makes the kept subscription inert from the save on. */ + if (session.SourceFilePath != null) + return; + + if (_scratchPersistHooked.TryGetValue(session, out _)) + return; + + _scratchPersistHooked.Add(session, new object()); + + /* DirtyStateChanged fires on every editor text change (#462's wiring), which is the + signal wanted here. It also fires on MarkClean, but a scratch session is only ever + marked clean by the save that just gave it a SourceFilePath, so the guard below + already ignores that firing. */ + session.DirtyStateChanged += (_, _) => + { + if (session.SourceFilePath == null) + RequestScratchPersist(session); + }; + + /* A session can arrive at its first CreateTab already holding text nobody typed + into it there — Edit Query hands a plan's statement to a fresh scratch session, + and a restored scratch (#496) comes back with its buffer's content. Both set the + text before the tab exists, so the subscription above never saw it; queue one + persist now so "scratch content on screen" implies "scratch content on disk, + one debounce later". (Scratch is already guaranteed by the top of this method.) */ + if (session.IsDirty) + RequestScratchPersist(session); + } + + /// + /// Notes that a scratch session's content changed and schedules the debounced write. + /// + private void RequestScratchPersist(QuerySessionControl session) + { + /* Same shutdown rule as RequestSessionPersist: OnClosed drains this set itself and + nothing may re-arm a timer against a window being torn down. */ + if (IsShuttingDown) + return; + + _scratchPersistPending.Add(session); + _scratchPersistOldestPending ??= DateTime.UtcNow; + + /* Max-latency cap (#496 review): a trailing-edge debounce restarts on every + keystroke, so continuous typing would defer the write indefinitely — the + protection at its weakest exactly while the user is producing the most content. + Once the oldest unflushed change has waited this long, write now instead of + re-arming; the flush clears the timestamp, so a pause afterwards returns to + ordinary debouncing. */ + if (DateTime.UtcNow - _scratchPersistOldestPending >= ScratchPersistMaxLatency) + { + FlushScratchBuffers(); + FlushSessionPersist(); + return; + } + + /* No real timer under the test host — read RequestSessionPersist's comment for the + full #451/#495 story: the suite shares one dispatcher, so a timer armed here would + tick during some LATER test and write THIS window's scratch buffers over whatever + that test had staged. Tests drive the flush through + FlushPendingScratchPersistForTests instead. */ + if (AppRuntimeMode.IsTestHost) + return; + + if (_scratchPersistTimer == null) + { + _scratchPersistTimer = new DispatcherTimer { Interval = ScratchPersistDebounce }; + /* The membership flush is chained on so a buffer and its scratch: entry + reach disk in the same breath: the first write for a session assigns its id + and requests a membership persist, and without the chained flush that entry + would trail the buffer by a debounce — a crash in that gap would strand a + buffer the startup sweep then deletes as unreferenced. */ + _scratchPersistTimer.Tick += (_, _) => + { + FlushScratchBuffers(); + FlushSessionPersist(); + }; + } + + // Stop-then-start restarts the interval, which is what makes it a debounce. + _scratchPersistTimer.Stop(); + _scratchPersistTimer.Start(); + } + + /// + /// Writes every pending scratch buffer down now. The timer's tick, every membership + /// flush point (, so end-of-restore and the #495 + /// debounce both drain this), , and the test seam all land here; + /// a flush with nothing pending is free. + /// + /// Deliberately NOT gated on the way the membership + /// flush is: OnClosed calls this while shutting down, precisely because the final drain + /// is part of the final write — see the ordering comment there. + /// + private void FlushScratchBuffers() + { + _scratchPersistTimer?.Stop(); + _scratchPersistOldestPending = null; + + if (_scratchPersistPending.Count == 0) + return; + + /* Snapshot-and-clear before writing: PersistScratchBuffer can call + DropScratchBuffer, which edits this set. */ + var pending = _scratchPersistPending.ToList(); + _scratchPersistPending.Clear(); + + foreach (var session in pending) + PersistScratchBuffer(session); + } + + /// + /// Writes one session's buffer, or removes it, according to what the session holds now. + /// + private void PersistScratchBuffer(QuerySessionControl session) + { + /* The scope fence, enforced at the writer as well as the subscription: a session + that gained a file between queueing and flushing (Save As raced the debounce) is + file-backed now, and SaveQueryToPath already deleted its buffer. */ + if (session.SourceFilePath != null) + return; + + /* A buffer exists to protect UNSAVED work, so a session with none sheds its buffer. + For a scratch session clean means empty — its saved-text baseline is forever "" + (#462) — so this is what erases the buffer of a tab whose text the user deleted + back out, instead of resurrecting that text at the next start as if the deletion + never happened. Clean close leans on this too: the only scratch tabs the + #462/#469/#477 prompts do not ask about are the ones with nothing typed, and this + branch is what guarantees those leave no buffer behind either. */ + if (!session.IsDirty) + { + DropScratchBuffer(session); + return; + } + + var text = session.QueryEditor.Text ?? string.Empty; + + /* Over the cap the buffer is not merely skipped but removed: a stale smaller + snapshot restoring under megabytes of newer typing would misrepresent what the + user had, which is worse than the honest pre-#496 nothing. */ + if (text.Length > MaxScratchPersistChars) + { + DropScratchBuffer(session); + return; + } + + var firstPersist = session.ScratchBufferId == null; + if (firstPersist) + { + /* The id is minted at first persist, not at construction, so an empty tab never + owns a buffer; a restored scratch arrives with its id already set and keeps + writing the same buffer across restarts. */ + session.ScratchBufferId = Guid.NewGuid(); + } + + try + { + ScratchBufferStore.Write(session.ScratchBufferId!.Value, text); + } + catch + { + /* Best-effort, same stance as AppSettingsService.Save: persistence must never + crash the editor it exists to protect. A failed write self-heals — either a + later flush succeeds, or restore finds no readable buffer and skips the + entry. */ + } + + /* A newly minted id is a membership change: the scratch: entry has to enter + the #495 list, and the tab watcher cannot see it (no tab was added or removed — + the same blind spot as SaveQueryToPath's path change, solved the same way). */ + if (firstPersist) + RequestSessionPersist(); + } + + /// + /// Deletes a session's scratch buffer and forgets its identity. The mechanics of every + /// way a buffer dies; the chose-vs-never-got-to-choose reasoning lives at the call + /// sites, because WHICH moments may call this is the entire design of #496: + /// + /// Don't Save answered at any #462/#469/#477 prompt — they chose + /// (). + /// A save that succeeded — the content lives in a real file now + /// (). + /// A scratch tab or window closed with nothing unsaved in it — nothing left to + /// protect (, the detached close, and the clean branch of + /// ). + /// + /// Cancel appears nowhere in that list: a cancelled close changes nothing. + /// + private void DropScratchBuffer(QuerySessionControl session) + { + /* Whatever was queued for this session must not be written after the drop — that + would resurrect the buffer the user just chose out of existence. */ + _scratchPersistPending.Remove(session); + + if (session.ScratchBufferId is not { } id) + return; + + session.ScratchBufferId = null; + ScratchBufferStore.TryDelete(id); + + /* The scratch: entry has to leave the #495 list with the buffer. Gated inside + RequestSessionPersist during shutdown, where OnClosed's own final SaveOpenPlans — + which runs after the final drain — writes the list without it. */ + RequestSessionPersist(); + } + + /// + /// The deterministic stand-in for the content debounce's tick — the #496 twin of + /// , for the same shared-dispatcher + /// reason (see ). Mirrors the real tick exactly, + /// membership chain included, so a test observes the same disk state a patient user + /// would. + /// + internal void FlushPendingScratchPersistForTests() + { + FlushScratchBuffers(); + FlushSessionPersist(); + } + + /// + /// Recreates one scratch tab from its persisted buffer during restore. False when the + /// buffer cannot come back, and the buffer file is deleted on that path — the #495 + /// poison invariant, mirrored: an entry that fails to load is skipped, never re-added + /// (the session that would re-list it is never created), and its file is swept rather + /// than left to fail again at every start. + /// + private bool TryRestoreScratchTab(Guid id) + { + string text; + try + { + text = File.ReadAllText(ScratchBufferStore.BufferPathFor(id)); + } + catch + { + ScratchBufferStore.TryDelete(id); + return false; + } + + /* An empty buffer should not exist — the writer deletes rather than writes empties — + so finding one means debris; restoring an empty tab from it would be noise. */ + if (string.IsNullOrEmpty(text)) + { + ScratchBufferStore.TryDelete(id); + return false; + } + + _queryCounter++; + var session = new QuerySessionControl(_credentialService, _connectionStore); + session.QueryEditor.Text = text; + + /* The SAME id, not a fresh one: this session continues the buffer it came from, so + its next flush overwrites in place and the list entry stays stable across any + number of restarts. */ + session.ScratchBufferId = id; + + /* Deliberately no MarkClean, unlike LoadSqlFile: this content is unsaved BY + DEFINITION — nothing on disk that the user chose backs it — so the tab must come + back dirty, marker and close-prompts and all. The session's empty saved-text + baseline gives that for free. */ + + var tab = CreateTab($"Query {_queryCounter}", session); + MainTabControl.Items.Add(tab); + MainTabControl.SelectedItem = tab; + UpdateEmptyOverlay(); + return true; + } +} diff --git a/src/PlanViewer.App/MainWindow.Tabs.cs b/src/PlanViewer.App/MainWindow.Tabs.cs index 53d4345f..6201cdec 100644 --- a/src/PlanViewer.App/MainWindow.Tabs.cs +++ b/src/PlanViewer.App/MainWindow.Tabs.cs @@ -109,6 +109,12 @@ The pointer handlers below live and die with the button itself. */ // A session can arrive already modified — re-docking a detached window builds a // fresh tab around a session that has been edited since it left. RefreshCloseGlyph(); + + /* #496: every top-level query session passes through here exactly when it gets + its first tab, which makes this the one wiring point for scratch content + persistence — same single-subscription argument as the #495 tab watcher. + Idempotent, because redock passes the same living session through again. */ + HookScratchPersistence(querySession); } // Middle-click to close @@ -250,6 +256,31 @@ private static void SetTabLabel(TabItem tab, string label) return null; } + /// + /// What the session-restore list records for a tab's content: the file path when there + /// is one, else the scratch:<guid> entry for a scratch session whose + /// content has actually been persisted (#496), else nothing. Layered ON TOP of + /// rather than folded into it, because that method + /// also answers Copy Path — and a scratch buffer id is precisely not a path anyone + /// should be handed to paste somewhere. + /// + /// The no-id case is deliberate, not a gap: an empty scratch tab has no buffer + /// (ids are minted at first persist), and restoring a parade of blank "Query N" tabs + /// would make persistence feel like clutter. A scratch tab earns its entry by having + /// content on disk worth coming back for. + /// + private static string? GetContentSessionEntry(Control? content) + { + var path = GetContentFilePath(content); + if (path != null) + return path; + + if (content is QuerySessionControl { SourceFilePath: null, ScratchBufferId: { } id }) + return ScratchBufferStore.EntryFor(id); + + return null; + } + private void StartRename(StackPanel header, TextBlock headerText) { var textBox = new TextBox @@ -368,6 +399,23 @@ afresh through CreateTab. */ { ForgetDetachedTabContent(c); + /* #496: a detached scratch window ACTUALLY closing is the session leaving + the app by the user's hand — the detached twin of TryCloseTabAsync's + drop. This callback never runs on redock (the helper's redocked latch + returns first), so a redocked scratch keeps its buffer. + + Gated off during shutdown's force-close (#496 review, third finding): + the walk's prompts are modal only to their own window, so the user can + type into a DIFFERENT detached scratch while a prompt is up — dirty, + never asked. OnClosed's final flush writes that buffer and lists its + entry; letting this drop run in the force-close storm afterwards would + delete the just-written buffer and leave the entry dangling. By then the + final write has already made every keep-or-drop decision, and skipping + here loses nothing: OnClosed's flush sheds clean sessions' stale buffers + itself. */ + if (!IsShuttingDown && c is QuerySessionControl { SourceFilePath: null } scratchSession) + DropScratchBuffer(scratchSession); + if (c is QueryStoreHistoryControl hc) hc.CancelFetch(); }, diff --git a/src/PlanViewer.App/MainWindow.axaml.cs b/src/PlanViewer.App/MainWindow.axaml.cs index 667c4e2e..e6e0c895 100644 --- a/src/PlanViewer.App/MainWindow.axaml.cs +++ b/src/PlanViewer.App/MainWindow.axaml.cs @@ -212,15 +212,23 @@ file behind it would still be skipped and the launch would silently session-restore instead. */ args = SingleInstance.StripNewInstanceFlag(args); - if (args.Length > 1 && File.Exists(args[1])) - { + /* Restore FIRST, on every cold start — then open the requested file on top, where + it lands focused. The old either/or (file-arg launches skipped restore entirely) + turned destructive once #495/#496 made the saved list continuously rewritten from + live membership: a double-clicked file overwrote the list within a debounce, + dropping scratch entries, and the NEXT launch's orphan sweep deleted the buffers + behind them — never-chosen content destroyed by an everyday flow (#496 review, + blocking finding). Restoring unconditionally closes that chain, and it is also + what editors do with a double-clicked file; under #489's single instance a + running Studio receives the file over the pipe and never re-enters this path, so + this only changes the cold start. The restore's new-tab fallback stays out of the + way when a file is about to open — a stray empty scratch tab beside the file the + user asked for is nobody's intent. */ + var hasFileArg = args.Length > 1 && File.Exists(args[1]); + RestoreOpenPlans(createFallbackTab: !hasFileArg); + + if (hasFileArg) OpenFileByExtension(args[1]); - } - else - { - // Restore plans that were open in the previous session - RestoreOpenPlans(); - } } private void StartPipeServer() @@ -348,7 +356,18 @@ so their paths are in it. */ _sessionPersistTimer?.Stop(); _sessionPersistPending = false; - // Save the list of currently open file-based tabs for session restore + /* #496, and the order matters: the scratch content writer drains BEFORE the final + list write, so the list below references exactly the buffers that exist. On a + clean close this drain only ever DELETES — every scratch tab with content was + resolved at the #462/#469/#477 prompts by now (saved ones stopped being + scratch, Don't-Saved ones already dropped their buffers), so what is left + pending is at most a clean scratch shedding a stale buffer. Which is the + invariant #496 promises: after a clean close, zero scratch buffers remain, + because every one of them was chosen about. */ + _scratchPersistTimer?.Stop(); + FlushScratchBuffers(); + + // Save the list of currently open tabs (paths and scratch entries) for session restore SaveOpenPlans(); _pipeCts.Cancel(); @@ -473,7 +492,7 @@ internal void RememberDetachedTabContent(Window window, Control content) if (content is QuerySessionControl session) _detachedQuerySessions.Add((window, session)); - /* This register is half of what CollectOpenTabPaths reads (the tab strip is the other + /* This register is half of what CollectOpenTabEntries reads (the tab strip is the other half), so a change to it is a membership change the tab watcher cannot see. Detach itself also removed a tab — the watcher fired — but by the time the debounced write flushes, this entry exists; the debounce is quietly doing ordering work here, since @@ -540,13 +559,7 @@ private async Task ConfirmDetachedCloseAsync(QuerySessionControl session, var choice = await UnsavedChangesDialog.ShowAsync(owner, owner.Title ?? "this query"); - return DecideClose(choice, session.SourceFilePath != null) switch - { - CloseAction.Cancel => false, - CloseAction.Close => true, - CloseAction.SaveInPlace => SaveQueryToPath(null, session, session.SourceFilePath!), - _ => await SaveQueryAsync(null, session, owner.StorageProvider) - }; + return await ResolveCloseChoiceAsync(choice, tab: null, session, owner.StorageProvider); } /// @@ -626,13 +639,49 @@ private async Task ConfirmCloseAsync(TabItem tab) MainTabControl.SelectedItem = tab; // show what is being asked about var choice = await UnsavedChangesDialog.ShowAsync(this, GetTabLabel(tab)); - return DecideClose(choice, session.SourceFilePath != null) switch + return await ResolveCloseChoiceAsync(choice, tab, session, storage: null); + } + + /// + /// Acts on the answer to an unsaved-changes prompt, docked and detached alike — the two + /// switches this replaces had drifted into near-twins, and #496 needed a THIRD copy or + /// one shared resolution point. The point matters beyond deduplication: #496's + /// delete-on-choice hooks the RESOLUTION of an answer, not the dialog that collected + /// it, so every prompt — tab close, detached close, the shutdown and restart walks — + /// honors Don't Save identically by construction. Internal so a test can also drive an + /// answer in directly, without a dialog to raise clicks on. + /// + /// Null for a detached session, which has no tab to retitle (#473). + /// + /// The picker a Save As should come off of — the detached window's own, so the dialog + /// lands where the user is looking (#473). Null means this window's. + /// + /// Whether the close may proceed. + internal async Task ResolveCloseChoiceAsync( + UnsavedChangesChoice choice, TabItem? tab, QuerySessionControl session, IStorageProvider? storage) + { + switch (DecideClose(choice, session.SourceFilePath != null)) { - CloseAction.Cancel => false, - CloseAction.Close => true, - CloseAction.SaveInPlace => SaveQueryToPath(tab, session, session.SourceFilePath!), - _ => await SaveQueryAsync(tab, session) - }; + case CloseAction.Cancel: + return false; + + case CloseAction.Close: + /* #496, the chose half of chose-vs-never-got-to-choose: Don't Save is the + user explicitly answering "this content may die" — the one signal the + crash-protection buffer must obey, or a discarded query would resurrect + at the next start and the prompt's answer would mean nothing. File-backed + sessions pass through untouched (no buffer to drop); their discarded + edits were never persisted anywhere — the scope fence. */ + DropScratchBuffer(session); + return true; + + case CloseAction.SaveInPlace: + // SaveQueryToPath retires any scratch buffer itself — content saved is content chosen. + return SaveQueryToPath(tab, session, session.SourceFilePath!); + + default: // SaveAs — DecideClose sends a scratch session's Save here, since it has no path yet. + return await SaveQueryAsync(tab, session, storage); + } } /// @@ -645,6 +694,16 @@ private async Task TryCloseTabAsync(TabItem tab) return false; MainTabControl.Items.Remove(tab); + + /* #496: a scratch tab that actually left the strip has had its fate decided — + answered at the prompt above (where Don't Save already dropped and a save made it + file-backed, so this is a no-op), or clean and therefore never asked. The clean + case is the one this line exists for: clean-for-scratch means empty, and an empty + tab closed inside the content debounce can still have a stale buffer on disk that + would otherwise sit there until the next startup sweep. */ + if (tab.Content is QuerySessionControl { SourceFilePath: null } closedScratch) + DropScratchBuffer(closedScratch); + UpdateEmptyOverlay(); return true; } diff --git a/src/PlanViewer.App/Services/AppSettingsService.cs b/src/PlanViewer.App/Services/AppSettingsService.cs index 66ba3c12..c3180b5d 100644 --- a/src/PlanViewer.App/Services/AppSettingsService.cs +++ b/src/PlanViewer.App/Services/AppSettingsService.cs @@ -20,6 +20,7 @@ internal sealed class AppSettingsService private static string SettingsDir; private static string SettingsPath; private static string OldFormatSettingsPath; + private static string ScratchDir; private static AppSettings? _cached; @@ -30,6 +31,7 @@ static AppSettingsService() "PerformanceStudio"); SettingsPath = Path.Combine(SettingsDir, "appsettings.json"); OldFormatSettingsPath = Path.Combine(SettingsDir, "perfstudio_format_settings.json"); + ScratchDir = Path.Combine(SettingsDir, "scratch"); } /// @@ -39,6 +41,16 @@ static AppSettingsService() /// internal static string SettingsFilePath => SettingsPath; + /// + /// Where scratch query buffers live (#496): one file per never-saved query tab, so an + /// abnormal exit does not take typed-but-unsaved work with it. Beside the settings file + /// rather than anywhere fancier because it is the same class of state — and, exactly like + /// , it rides , so + /// tests that exercise the real buffer writes land them in the run-scoped temp root + /// instead of the developer's profile (#487's pattern, same reasoning as #451). + /// + internal static string ScratchDirectory => ScratchDir; + /// /// Points every settings read and write at instead of the /// real per-user profile. Exists for exactly one caller: the test harness (#451). Its @@ -54,6 +66,7 @@ internal static void RedirectStorageForTestHost(string directory) SettingsDir = directory; SettingsPath = Path.Combine(directory, "appsettings.json"); OldFormatSettingsPath = Path.Combine(directory, "perfstudio_format_settings.json"); + ScratchDir = Path.Combine(directory, "scratch"); // Anything cached was loaded from the old location; drop it so the first Load // after the redirect reads the new one. diff --git a/src/PlanViewer.App/Services/ScratchBufferStore.cs b/src/PlanViewer.App/Services/ScratchBufferStore.cs new file mode 100644 index 00000000..0ae7acc4 --- /dev/null +++ b/src/PlanViewer.App/Services/ScratchBufferStore.cs @@ -0,0 +1,160 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; + +namespace PlanViewer.App.Services; + +/// +/// The on-disk half of scratch buffer persistence (#496): one file per never-saved query tab +/// under , named by the tab's stable buffer +/// id, plus the scratch:<guid> entry format that puts those tabs on the same +/// ordered open_tabs list the file-backed tabs use (#495). +/// +/// Why the entry rides the existing list instead of a second one. One list is +/// one ordering: scratch tabs interleave with file tabs on the strip, and two lists would +/// have to reinvent that interleaving (#495's detached entries already live with an +/// append-after compromise; docked tabs should not inherit it). Compatibility comes free and +/// is pinned the way #494's activation sentinel taught: an old build reading a new list +/// guards every entry with File.Exists, and contains a +/// colon — not a legal character in a Windows file name, and the app only ever writes +/// absolute paths to the list, which an entry starting with scratch: is not on any +/// platform — so old builds skip these entries silently, exactly as they skip a file that +/// was deleted. A new build reading an old list sees only plain paths and behaves exactly +/// as before. No version field, no migration. +/// +/// Privacy. Scratch SQL can hold literals — names, ids, whatever the user was +/// querying for. These buffers land in the user's local profile beside the settings file, +/// which already stores the recent-plans list and, next to it, saved plan files whose XML +/// embeds full statement text and parameter values. Same machine, same user, same +/// sensitivity class as what is already there; no new exposure class is created. +/// +internal static class ScratchBufferStore +{ + /// + /// What marks an open_tabs entry as a scratch buffer rather than a file path. + /// The colon is load-bearing — see the class comment — so the compat test pins this + /// string directly rather than proving anything with a File.Exists that would + /// be vacuous on a runner whose filesystem happily allows colons. + /// + internal const string EntryPrefix = "scratch:"; + + /// + /// .sql so a user digging through their profile can open a buffer and recognize it. + /// + private const string BufferExtension = ".sql"; + + /// + /// How long an unreferenced buffer survives the startup sweep. Three days spans a long + /// weekend of not reopening Studio after a crash — see the age-gate comment in + /// . Internal so the sweep tests can backdate past it + /// instead of hardcoding a sibling value that drifts. + /// + internal static readonly TimeSpan OrphanGracePeriod = TimeSpan.FromDays(3); + + /// The open_tabs entry for a scratch buffer. + internal static string EntryFor(Guid id) => EntryPrefix + id.ToString("N"); + + /// + /// Whether an open_tabs entry names a scratch buffer. Anything that fails here — + /// including a prefixed entry whose tail is not a GUID — is treated as a file path by + /// the caller, which is also what makes a file literally named scratch:something + /// on a colon-tolerant filesystem keep opening as the file it is. + /// + internal static bool TryParseEntry(string? entry, out Guid id) + { + id = default; + return entry != null + && entry.StartsWith(EntryPrefix, StringComparison.Ordinal) + && Guid.TryParse(entry.AsSpan(EntryPrefix.Length), out id); + } + + /// Where a buffer's content lives on disk. + internal static string BufferPathFor(Guid id) => + Path.Combine(AppSettingsService.ScratchDirectory, id.ToString("N") + BufferExtension); + + /// + /// Writes a buffer's content. Atomic for the same reason every other write in this app's + /// profile is (#495): the buffer may be the only copy of the user's typing, and a crash + /// mid-write must leave the previous content rather than a truncated file. + /// + internal static void Write(Guid id, string text) + { + Directory.CreateDirectory(AppSettingsService.ScratchDirectory); + AtomicFile.WriteAllText(BufferPathFor(id), text); + } + + /// + /// Deletes a buffer's file, best-effort. Persistence in this app never throws at the + /// user ( sets that precedent); a buffer that + /// cannot be deleted right now is unreferenced garbage the startup sweep collects later. + /// + internal static void TryDelete(Guid id) + { + try + { + File.Delete(BufferPathFor(id)); + } + catch + { + // Best-effort — see doc comment. + } + } + + /// + /// Deletes every file in the scratch directory that is not one of the referenced + /// buffers. Run once at startup, after the saved tab list has been read (#496): a clean + /// close deletes each buffer at the moment the user chooses its fate, so anything left + /// unreferenced is debris — a buffer whose entry a crash-window skew lost, an + /// AtomicFile .tmp stranded by a crash mid-write, a buffer a poisoned + /// restore skipped. Matching on the exact expected file name (not the GUID stem) is + /// what lets the sweep collect those .tmp siblings too. + /// + internal static void SweepAllExcept(IReadOnlyCollection referenced) + { + string[] files; + try + { + files = Directory.GetFiles(AppSettingsService.ScratchDirectory); + } + catch + { + // Most commonly: the directory does not exist because nothing has ever + // persisted a scratch buffer. Nothing to sweep either way. + return; + } + + var keep = referenced + .Select(id => id.ToString("N") + BufferExtension) + .ToHashSet(StringComparer.OrdinalIgnoreCase); + + /* Age gate (#496 review): an unreferenced buffer is USUALLY debris, but two crash + shapes make a fresh one innocent — a buffer written moments before a crash in + the buffer-then-list gap, and every bystander stranded when a mid-restore crash + left the poison-cleared list empty. Only files past the grace period die, which + turns "the sweep destroyed never-chosen content" into "an orphan lingered a few + days as a recognizable .sql a person can still recover by hand" — the reason + buffers carry that extension. The gate costs nothing on the paths that matter: + chosen deletions (Don't Save, Save) delete directly and never come through here, + and referenced buffers are never candidates at all. */ + var cutoff = DateTime.UtcNow - OrphanGracePeriod; + + foreach (var file in files) + { + if (keep.Contains(Path.GetFileName(file))) + continue; + + try + { + if (File.GetLastWriteTimeUtc(file) >= cutoff) + continue; + + File.Delete(file); + } + catch + { + // Locked or otherwise stuck — the next startup's sweep gets another turn. + } + } + } +} diff --git a/tests/PlanViewer.Core.Tests/OpenInEditorOverwriteTests.cs b/tests/PlanViewer.Core.Tests/OpenInEditorOverwriteTests.cs index 0ec2e03f..69e945eb 100644 --- a/tests/PlanViewer.Core.Tests/OpenInEditorOverwriteTests.cs +++ b/tests/PlanViewer.Core.Tests/OpenInEditorOverwriteTests.cs @@ -15,7 +15,7 @@ namespace PlanViewer.Core.Tests; /// /// Same testing shape as UnsavedQueryChangesTests: whether to ask is a pure value /// (, split out to be testable the -/// way CollectOpenTabPaths was), and the handler is driven directly with the prompt answered +/// way CollectOpenTabEntries was), and the handler is driven directly with the prompt answered /// by closing it — which is Cancel — or by raising a click on its Replace button. /// public class OpenInEditorOverwriteTests diff --git a/tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs b/tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs index 1321955d..17293b6f 100644 --- a/tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs +++ b/tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs @@ -39,7 +39,7 @@ public void AQueryOpenedFromAFileIsWrittenDownForTheNextSession() var window = new MainWindow(); window.LoadSqlFile(path); - Assert.Contains(path, window.CollectOpenTabPaths()); + Assert.Contains(path, window.CollectOpenTabEntries()); } finally { @@ -49,9 +49,11 @@ public void AQueryOpenedFromAFileIsWrittenDownForTheNextSession() } /// - /// The deliberate edge of the fix. A never-saved scratch buffer has no path, so there is - /// nothing to write down and it does not come back — persisting unsaved text is #462's job, - /// not this one's. + /// The edge #463 drew, redrawn by #496: a scratch tab enters the list only once its + /// CONTENT has actually persisted (as a scratch:<guid> entry — + /// ScratchBufferPersistenceTests owns that half). A fresh, empty scratch has no buffer, + /// no id, and therefore still no entry — which is what keeps a restart from restoring a + /// parade of blank "Query N" tabs. /// [Fact] public void AScratchQueryHasNoFileAndIsNotWrittenDown() @@ -63,7 +65,7 @@ public void AScratchQueryHasNoFileAndIsNotWrittenDown() var session = Sessions(window).Last(); Assert.Null(session.SourceFilePath); - Assert.Empty(window.CollectOpenTabPaths()); + Assert.Empty(window.CollectOpenTabEntries()); }); } @@ -107,7 +109,7 @@ public void APlanFileStillComesBackAsAPlanTab() var window = new MainWindow(); - Assert.Contains(path, window.CollectOpenTabPaths()); + Assert.Contains(path, window.CollectOpenTabEntries()); Assert.Contains(Viewers(window), v => v.SourceFilePath == path); } finally diff --git a/tests/PlanViewer.Core.Tests/ScratchBufferPersistenceTests.cs b/tests/PlanViewer.Core.Tests/ScratchBufferPersistenceTests.cs new file mode 100644 index 00000000..30c64ff4 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/ScratchBufferPersistenceTests.cs @@ -0,0 +1,755 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.Json; +using Avalonia.Controls; +using Avalonia.Interactivity; +using Avalonia.Threading; +using PlanViewer.App; +using PlanViewer.App.Controls; +using PlanViewer.App.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #496: after #495 made the open-tab list survive abnormal exits, the one thing still lost +/// to a crash was a scratch tab's CONTENT — typed, never saved, backed by nothing. Content +/// now persists continuously to one buffer file per scratch tab (debounced, beside the +/// settings file), the tab rides the #495 list as a scratch:<guid> entry in +/// strip order, and the buffer dies at exactly the moments the user chooses its fate. The +/// design center under every test here: a buffer the user CHOSE to discard dies; a buffer +/// they NEVER GOT TO CHOOSE about survives. +/// +/// Same disciplines as SessionPersistenceTests, because this is the same feature one +/// layer deeper: assertions read the redirected FILES (#451/#487 — which is also what makes +/// letting the startup orphan sweep actually delete things safe), the debounce is driven +/// through rather than waited +/// out (shared dispatcher — see RequestSessionPersist), and every test that stages +/// persisted state blanks it on the way out, list and buffer files both, because the next +/// MainWindow constructed anywhere in the run restores the one and sweeps the other. +/// +public class ScratchBufferPersistenceTests +{ + [Fact] + public void TypedScratchContentIsPersistedWithoutClosingTheApp() + { + HeadlessUi.Run(() => + { + try + { + var window = new MainWindow(); + var session = NewScratchTab(window, "SELECT 1 AS scratch_live;"); + + /* The keystrokes armed the content debounce through DirtyStateChanged; the + seam stands in for its tick. Before #496 this text existed nowhere but + the editor. */ + window.FlushPendingScratchPersistForTests(); + + var id = session.ScratchBufferId; + Assert.NotNull(id); // minted at first persist, not at construction + + Assert.Equal( + "SELECT 1 AS scratch_live;", + File.ReadAllText(ScratchBufferStore.BufferPathFor(id!.Value))); + + /* And the entry landed with the buffer, not a debounce later — the seam + mirrors the real tick's chained membership flush, so a crash right after + the idle write already finds both halves on disk. */ + Assert.Contains(ScratchBufferStore.EntryFor(id.Value), PersistedOpenTabs()); + } + finally + { + ResetScratchState(); + } + }); + } + + [Fact] + public void ARestoredScratchTabIsDirtyIntactAndContinuesItsBuffer() + { + HeadlessUi.Run(() => + { + var id = Guid.NewGuid(); + try + { + ScratchBufferStore.Write(id, "SELECT 1 AS survived_the_crash;"); + Seed(ScratchBufferStore.EntryFor(id)); + + var window = new MainWindow(); + + var session = Sessions(window).Single(s => s.ScratchBufferId == id); + Assert.Equal("SELECT 1 AS survived_the_crash;", session.QueryEditor.Text); + + /* Dirty by definition: nothing the user chose backs this content, so the + modified marker and every close prompt must treat it as unsaved work. */ + Assert.True(session.IsDirty); + Assert.Null(session.SourceFilePath); + + /* The SAME id came back — this session continues its buffer across + restarts rather than forking a new file per launch — and restore + re-listed it immediately, same as it does for file tabs. */ + Assert.True(File.Exists(ScratchBufferStore.BufferPathFor(id))); + Assert.Contains(ScratchBufferStore.EntryFor(id), PersistedOpenTabs()); + } + finally + { + ResetScratchState(); + } + }); + } + + /// + /// The reason scratch entries live INSIDE the one ordered list instead of appended to + /// it: a scratch tab sitting between two file tabs comes back between them. (#495's + /// detached entries keep their documented append-after compromise; this pins that + /// docked tabs never inherited it.) + /// + [Fact] + public void AScratchEntryKeepsItsPlaceAmongFilePaths() + { + HeadlessUi.Run(() => + { + var first = TempSql("SELECT 1 AS first;"); + var last = TempSql("SELECT 2 AS last;"); + try + { + var window = new MainWindow(); + window.LoadSqlFile(first); + var session = NewScratchTab(window, "SELECT 0 AS in_between;"); + window.LoadSqlFile(last); + + window.FlushPendingScratchPersistForTests(); + + var entry = ScratchBufferStore.EntryFor(session.ScratchBufferId!.Value); + Assert.Equal(new[] { first, entry, last }, PersistedOpenTabs()); + } + finally + { + ResetScratchState(); + File.Delete(first); + File.Delete(last); + } + }); + } + + /// + /// The chose half of the design center, through the real prompt: Don't Save is the user + /// answering "this content may die", and the buffer obeys — or a discarded query would + /// resurrect at the next start and the prompt's answer would mean nothing. + /// + [Fact] + public void DontSaveAtTheClosePromptDeletesTheBuffer() + { + HeadlessUi.Run(() => + { + MainWindow? window = null; + QuerySessionControl? session = null; + try + { + window = new MainWindow(); + window.Show(); // the prompt's ShowDialog needs a visible owner, headless or not + + session = NewScratchTab(window, "SELECT 1 AS discarded_on_purpose;"); + window.FlushPendingScratchPersistForTests(); + + var id = session.ScratchBufferId!.Value; + Assert.True(File.Exists(ScratchBufferStore.BufferPathFor(id))); + + var tab = TabOf(window, session); + CloseButton(tab).RaiseEvent(new RoutedEventArgs(Button.ClickEvent)); + Dispatcher.UIThread.RunJobs(); + + var prompt = Assert.Single(window.OwnedWindows); + PromptButton(prompt, "Don't Save").RaiseEvent(new RoutedEventArgs(Button.ClickEvent)); + Dispatcher.UIThread.RunJobs(); + + Assert.DoesNotContain(tab, window.MainTabControl.Items.OfType()); + Assert.False(File.Exists(ScratchBufferStore.BufferPathFor(id)), + "Don't Save is a choice, and a chosen buffer dies"); + Assert.Null(session.ScratchBufferId); + + window.FlushPendingScratchPersistForTests(); + Assert.DoesNotContain(ScratchBufferStore.EntryFor(id), PersistedOpenTabs()); + } + finally + { + /* The session too, like this test's siblings: if an assertion above fails + before the Don't Save click lands, the scratch tab is still dirty, and a + PutAway that skips MarkClean would raise the #462 walk's modal during + teardown — the leaked-window poisoning #474's helper exists to prevent, + biting exactly while masking the real failure. */ + PutAwayMainWindow(window, session); + ResetScratchState(); + } + }); + } + + /// + /// The other way a user chooses: a save that succeeds moves the content into a real + /// file, the list entry becomes the path, and the buffer that was protecting the + /// content retires. + /// + [Fact] + public void ASuccessfulSaveTurnsTheEntryIntoThePathAndDeletesTheBuffer() + { + HeadlessUi.Run(() => + { + var savedAs = Path.Combine(Path.GetTempPath(), $"saved_{Path.GetRandomFileName()}.sql"); + try + { + var window = new MainWindow(); + var session = NewScratchTab(window, "SELECT 1 AS graduated;"); + window.FlushPendingScratchPersistForTests(); + + var id = session.ScratchBufferId!.Value; + Assert.True(File.Exists(ScratchBufferStore.BufferPathFor(id))); + Assert.Contains(ScratchBufferStore.EntryFor(id), PersistedOpenTabs()); + + Assert.True(window.SaveQueryToPath(TabOf(window, session), session, savedAs)); + window.FlushPendingScratchPersistForTests(); + + Assert.False(File.Exists(ScratchBufferStore.BufferPathFor(id)), + "content saved is content chosen — the real file owns it now"); + Assert.Null(session.ScratchBufferId); + + var persisted = PersistedOpenTabs(); + Assert.Contains(savedAs, persisted); + Assert.DoesNotContain(ScratchBufferStore.EntryFor(id), persisted); + } + finally + { + ResetScratchState(); + if (File.Exists(savedAs)) + File.Delete(savedAs); + } + }); + } + + /// + /// The consequence the design center promises: a CLEAN close leaves zero buffers, + /// because every scratch tab with content was asked about on the way out — buffers + /// exist on disk only after an exit nobody got to answer. This drives the whole route: + /// window close, the #462/#473 walk, Don't Save clicked, OnClosed's final drain and + /// list write. + /// + [Fact] + public void ACleanCloseLeavesNoScratchBuffersBehind() + { + HeadlessUi.Run(() => + { + MainWindow? window = null; + QuerySessionControl? session = null; + try + { + window = new MainWindow(); + window.Show(); + + session = NewScratchTab(window, "SELECT 1 AS asked_about;"); + window.FlushPendingScratchPersistForTests(); + Assert.Single(ScratchFiles()); + + window.Close(); + Dispatcher.UIThread.RunJobs(); + + var prompt = Assert.Single(window.OwnedWindows); + PromptButton(prompt, "Don't Save").RaiseEvent(new RoutedEventArgs(Button.ClickEvent)); + Dispatcher.UIThread.RunJobs(); + + Assert.False(window.IsVisible, "the answered walk lets the close proceed"); + Assert.Empty(ScratchFiles()); + Assert.DoesNotContain(PersistedOpenTabs(), + e => e.StartsWith(ScratchBufferStore.EntryPrefix, StringComparison.Ordinal)); + } + finally + { + PutAwayMainWindow(window, session); + ResetScratchState(); + } + }); + } + + /// + /// The tab nobody is ever asked about: a scratch whose text was deleted back out is + /// clean (its saved-text baseline is forever ""), holds no unsaved work, and must not + /// resurrect its old text at the next start as if the deletion never happened. This is + /// also the branch that keeps the zero-buffers-after-clean-close invariant airtight for + /// tabs the prompts skip. + /// + [Fact] + public void AnEmptiedScratchTabShedsItsBufferOnTheNextFlush() + { + HeadlessUi.Run(() => + { + try + { + var window = new MainWindow(); + var session = NewScratchTab(window, "SELECT 1 AS typed_then_deleted;"); + window.FlushPendingScratchPersistForTests(); + + var id = session.ScratchBufferId!.Value; + Assert.True(File.Exists(ScratchBufferStore.BufferPathFor(id))); + + session.QueryEditor.Text = ""; + window.FlushPendingScratchPersistForTests(); + + Assert.False(File.Exists(ScratchBufferStore.BufferPathFor(id))); + Assert.Null(session.ScratchBufferId); + Assert.DoesNotContain(ScratchBufferStore.EntryFor(id), PersistedOpenTabs()); + } + finally + { + ResetScratchState(); + } + }); + } + + /// + /// Startup collects what nothing references — but only past the grace period (#496 + /// review): a FRESH unreferenced buffer can be an innocent bystander (written moments + /// before a crash in the buffer-then-list gap, or stranded when a mid-restore crash + /// left the poison-cleared list empty), so it survives as a recoverable .sql until the + /// grace runs out. Aged debris — stale buffers and AtomicFile .tmp siblings — still + /// dies, and the referenced buffer rides through untouched at any age. + /// + [Fact] + public void TheSweepSparesFreshOrphansAndCollectsAgedOnes() + { + HeadlessUi.Run(() => + { + var kept = Guid.NewGuid(); + var agedOrphan = Guid.NewGuid(); + var freshOrphan = Guid.NewGuid(); + var strayTmp = ScratchBufferStore.BufferPathFor(agedOrphan) + ".tmp"; + try + { + ScratchBufferStore.Write(kept, "SELECT 1 AS referenced;"); + ScratchBufferStore.Write(agedOrphan, "SELECT 2 AS stranded_long_ago;"); + ScratchBufferStore.Write(freshOrphan, "SELECT 3 AS innocent_bystander;"); + File.WriteAllText(strayTmp, "half a write"); + + /* Backdate past the grace period, derived from the store's own constant so + the two can't drift apart. The kept buffer is aged too, proving reference + beats age. */ + var stale = DateTime.UtcNow - ScratchBufferStore.OrphanGracePeriod - TimeSpan.FromMinutes(1); + File.SetLastWriteTimeUtc(ScratchBufferStore.BufferPathFor(kept), stale); + File.SetLastWriteTimeUtc(ScratchBufferStore.BufferPathFor(agedOrphan), stale); + File.SetLastWriteTimeUtc(strayTmp, stale); + + Seed(ScratchBufferStore.EntryFor(kept)); + + var window = new MainWindow(); + + Assert.True(File.Exists(ScratchBufferStore.BufferPathFor(kept))); + Assert.NotNull(Sessions(window).SingleOrDefault(s => s.ScratchBufferId == kept)); + + Assert.False(File.Exists(ScratchBufferStore.BufferPathFor(agedOrphan))); + Assert.False(File.Exists(strayTmp)); + + Assert.True( + File.Exists(ScratchBufferStore.BufferPathFor(freshOrphan)), + "a fresh orphan may be a crash bystander and must outlive the sweep until the grace period ends"); + } + finally + { + File.Delete(ScratchBufferStore.BufferPathFor(freshOrphan)); + ResetScratchState(); + } + }); + } + + /// + /// The #496 review's blocking finding, closed: a cold start that carries a file + /// argument used to SKIP restore, so the continuous writer rewrote the list without + /// the previous session's scratch entries and the next start's sweep destroyed their + /// buffers — never-chosen content lost to an everyday double-click. A file-arg start + /// now restores first and opens the file on top, and the fallback scratch tab stays + /// out of the way. + /// + [Fact] + public void AFileArgColdStartRestoresTheScratchSessionAndOpensTheFile() + { + HeadlessUi.Run(() => + { + var opened = TempSql("SELECT 1 AS from_argv;"); + var id = Guid.NewGuid(); + MainWindow? window = null; + try + { + /* The pre-relaunch on-disk state, laid down directly rather than through a + first window: a scratch buffer and its list entry, exactly what an + abnormal exit leaves behind. Seeding after construction is the #489 tests' + pattern — a window's own constructor already runs OpenFromStartupArgs once + (with the test runner's file-less argv), so driving the seam a second time + here is the single cold-start-with-a-file call under test, not a double + restore. */ + window = new MainWindow(); + ScratchBufferStore.Write(id, "SELECT 2 AS must_survive_the_double_click;"); + Seed(ScratchBufferStore.EntryFor(id)); + + window.OpenFromStartupArgs(new[] { "PerformanceStudio.exe", opened }); + window.FlushPendingSessionPersistForTests(); + + // The scratch survived the file-arg launch instead of being overwritten... + Assert.NotNull(Sessions(window).SingleOrDefault(s => s.ScratchBufferId == id)); + Assert.True(File.Exists(ScratchBufferStore.BufferPathFor(id))); + + // ...and the requested file opened on top, both on the persisted list. + var persisted = PersistedOpenTabs(); + Assert.Contains(ScratchBufferStore.EntryFor(id), persisted); + Assert.Contains(opened, persisted); + } + finally + { + PutAwayMainWindow(window); + ResetScratchState(); + File.Delete(opened); + } + }); + } + + /// + /// #495's poison invariant, mirrored for buffers: an entry whose buffer cannot load is + /// skipped, never re-added (the poison-defense clear already ran, and no session exists + /// to re-list it), and its file is removed rather than left to fail at every start. An + /// unreadable buffer (a directory squatting on its path) and an empty one (debris the + /// writer would never produce) both walk that path; the healthy entry next to them + /// restores. + /// + [Fact] + public void AFailedBufferLoadIsSkippedNotReAddedAndSwept() + { + HeadlessUi.Run(() => + { + var unreadable = Guid.NewGuid(); + var empty = Guid.NewGuid(); + var healthy = Guid.NewGuid(); + var unreadablePath = ScratchBufferStore.BufferPathFor(unreadable); + try + { + // A directory where the buffer file should be: File.ReadAllText throws. + Directory.CreateDirectory(unreadablePath); + ScratchBufferStore.Write(empty, ""); + ScratchBufferStore.Write(healthy, "SELECT 1 AS healthy;"); + Seed( + ScratchBufferStore.EntryFor(unreadable), + ScratchBufferStore.EntryFor(empty), + ScratchBufferStore.EntryFor(healthy)); + + var window = new MainWindow(); + + Assert.NotNull(Sessions(window).SingleOrDefault(s => s.ScratchBufferId == healthy)); + Assert.Null(Sessions(window).SingleOrDefault(s => s.ScratchBufferId == unreadable)); + Assert.Null(Sessions(window).SingleOrDefault(s => s.ScratchBufferId == empty)); + + Assert.False(File.Exists(ScratchBufferStore.BufferPathFor(empty))); + + /* Restore rewrote the list on its way out; the poisoned entries must not + have ridden back in. */ + var persisted = PersistedOpenTabs(); + Assert.Contains(ScratchBufferStore.EntryFor(healthy), persisted); + Assert.DoesNotContain(ScratchBufferStore.EntryFor(unreadable), persisted); + Assert.DoesNotContain(ScratchBufferStore.EntryFor(empty), persisted); + } + finally + { + ResetScratchState(); + if (Directory.Exists(unreadablePath)) + Directory.Delete(unreadablePath); + } + }); + } + + /// + /// The cap: a pathological paste must not put megabytes on the idle writer's two-second + /// cadence, so past ~1MB the tab simply behaves pre-#496 — and an already-written + /// smaller buffer is removed rather than left to restore a stale fragment of what the + /// user actually had. Shrinking back under the cap resumes persistence. + /// + [Fact] + public void ABufferOverTheSizeCapIsNotPersistedAndAStaleOneIsRemoved() + { + HeadlessUi.Run(() => + { + try + { + var window = new MainWindow(); + var session = NewScratchTab(window, "SELECT 1 AS small;"); + window.FlushPendingScratchPersistForTests(); + + var firstId = session.ScratchBufferId!.Value; + Assert.True(File.Exists(ScratchBufferStore.BufferPathFor(firstId))); + + // One char past MainWindow.MaxScratchPersistChars (1 MiB). + session.QueryEditor.Text = new string('x', (1024 * 1024) + 1); + window.FlushPendingScratchPersistForTests(); + + Assert.False(File.Exists(ScratchBufferStore.BufferPathFor(firstId))); + Assert.Null(session.ScratchBufferId); + Assert.Empty(ScratchFiles()); + + session.QueryEditor.Text = "SELECT 1 AS small_again;"; + window.FlushPendingScratchPersistForTests(); + + Assert.NotNull(session.ScratchBufferId); + Assert.Equal( + "SELECT 1 AS small_again;", + File.ReadAllText(ScratchBufferStore.BufferPathFor(session.ScratchBufferId!.Value))); + } + finally + { + ResetScratchState(); + } + }); + } + + /// + /// A list from before #496 is nothing but plain paths, and a new build treats it + /// exactly as #495 did — restored by extension, rewritten as paths, no scratch + /// machinery invoked and no scratch files invented. Zero version fields is the whole + /// compatibility design, so this pins its new-reading-old half. + /// + [Fact] + public void AnOldFormatPlainPathListRestoresUnchanged() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1 AS plain_old_path;"); + try + { + Seed(path); + + var window = new MainWindow(); + + var session = Sessions(window).Single(s => s.SourceFilePath == path); + Assert.Equal("SELECT 1 AS plain_old_path;", session.QueryEditor.Text); + Assert.Equal(new[] { path }, PersistedOpenTabs()); + Assert.Empty(ScratchFiles()); + } + finally + { + ResetScratchState(); + File.Delete(path); + } + }); + } + + /// + /// The old-reading-new half, pinned the way #494's activation sentinel taught: as the + /// STRING PROPERTY that makes it true. An old build reading a new list guards every + /// entry with File.Exists, so a scratch entry is safe to hand it exactly because the + /// entry can never name an existing file — the colon is not legal in a Windows file + /// name, and the app only ever writes absolute paths to the list, which a scratch entry + /// is not on any platform. A File.Exists assertion here would prove nothing on a runner + /// whose filesystem allows colons; the prefix's shape is the actual contract, so the + /// prefix's shape is what this test holds still. + /// + [Fact] + public void TheScratchEntryPrefixCanNeverNameAnExistingFile() + { + Assert.Contains(':', ScratchBufferStore.EntryPrefix); + Assert.Equal("scratch:", ScratchBufferStore.EntryPrefix); + + var id = Guid.NewGuid(); + var entry = ScratchBufferStore.EntryFor(id); + Assert.StartsWith(ScratchBufferStore.EntryPrefix, entry, StringComparison.Ordinal); + + // Roundtrip: what the writer emits is what restore routes to a buffer. + Assert.True(ScratchBufferStore.TryParseEntry(entry, out var parsed)); + Assert.Equal(id, parsed); + + /* Everything that is not a well-formed entry routes as a PATH — including a + prefixed entry with a mangled tail, and a real file on a colon-tolerant + filesystem that happens to be named like one. */ + Assert.False(ScratchBufferStore.TryParseEntry(null, out _)); + Assert.False(ScratchBufferStore.TryParseEntry(@"C:\temp\query.sql", out _)); + Assert.False(ScratchBufferStore.TryParseEntry("scratch:not-a-guid", out _)); + Assert.False(ScratchBufferStore.TryParseEntry("SCRATCH:" + id.ToString("N"), out _), + "the prefix is exact — case-mangled entries fall through to path handling"); + } + + /// + /// #496's sixth requirement: off the tab strip is not out of the feature. A detached + /// scratch window's edits keep flowing to the same buffer (the subscription lives on + /// the session, which detach moves and redock reuses), its entry rides the #495 + /// detached register half of the list — and Don't Save at ITS close prompt kills the + /// buffer the same way a docked one's does. + /// + [Fact] + public void ADetachedScratchWindowPersistsAndItsDontSaveDeletes() + { + HeadlessUi.Run(() => + { + MainWindow? window = null; + Window? detached = null; + QuerySessionControl? session = null; + try + { + window = new MainWindow(); + window.Show(); + + session = NewScratchTab(window, "SELECT 1 AS docked_first;"); + window.FlushPendingScratchPersistForTests(); + var id = session.ScratchBufferId!.Value; + + detached = window.DetachTabToWindow(TabOf(window, session))!; + Dispatcher.UIThread.RunJobs(); + + session.QueryEditor.Text = "SELECT 2 AS edited_detached;"; + window.FlushPendingScratchPersistForTests(); + + Assert.Equal( + "SELECT 2 AS edited_detached;", + File.ReadAllText(ScratchBufferStore.BufferPathFor(id))); + Assert.Contains(ScratchBufferStore.EntryFor(id), PersistedOpenTabs()); + + detached.Close(); + Dispatcher.UIThread.RunJobs(); + + var prompt = Assert.Single(detached.OwnedWindows); + PromptButton(prompt, "Don't Save").RaiseEvent(new RoutedEventArgs(Button.ClickEvent)); + Dispatcher.UIThread.RunJobs(); + + Assert.False(detached.IsVisible); + Assert.False(File.Exists(ScratchBufferStore.BufferPathFor(id)), + "a chosen buffer dies in a detached window too"); + + window.FlushPendingScratchPersistForTests(); + Assert.DoesNotContain(ScratchBufferStore.EntryFor(id), PersistedOpenTabs()); + } + finally + { + PutAwayDetached(detached, session); + PutAwayMainWindow(window, session); + ResetScratchState(); + } + }); + } + + // ── plumbing ────────────────────────────────────────────────────────── + + /// + /// Opens a fresh scratch tab and types into it — through NewQuery_Click and the editor + /// text, so the persistence subscription is exercised the way a user exercises it. + /// + private static QuerySessionControl NewScratchTab(MainWindow window, string text) + { + window.NewQuery_Click(window, new RoutedEventArgs()); + var session = Sessions(window).Last(); + session.QueryEditor.Text = text; + return session; + } + + /// + /// What is actually on disk, read back through the same serializer the app writes with + /// — same rationale as SessionPersistenceTests: crash survival is a property of the + /// file, so the file is what gets asserted. + /// + private static List PersistedOpenTabs() + { + var json = File.ReadAllText(AppSettingsService.SettingsFilePath); + return JsonSerializer.Deserialize(json)!.OpenTabs; + } + + /// Every file currently in the redirected scratch directory. + private static string[] ScratchFiles() => + Directory.Exists(AppSettingsService.ScratchDirectory) + ? Directory.GetFiles(AppSettingsService.ScratchDirectory) + : Array.Empty(); + + /// + /// Stages entries where the next MainWindow will look — Load returns the process-wide + /// cached instance, the very object the window reads (see RestoreQueryTabsTests.Seed). + /// + private static void Seed(params string[] entries) + { + var settings = AppSettingsService.Load(); + settings.OpenTabs.Clear(); + settings.OpenTabs.AddRange(entries); + } + + /// + /// Leaves nothing for the next test's MainWindow to restore or sweep: the list (cache + /// and file, same as SessionPersistenceTests.ResetPersistedState) and every buffer + /// file. Buffer hygiene matters doubly here — a leaked scratch entry would restore a + /// phantom tab in an unrelated test, and a leaked buffer file would be "swept" by that + /// test's window, hiding a real sweep bug or inventing one. + /// + private static void ResetScratchState() + { + var settings = AppSettingsService.Load(); + settings.OpenTabs.Clear(); + AppSettingsService.Save(settings); + + foreach (var file in ScratchFiles()) + { + try + { + File.Delete(file); + } + catch + { + // Best-effort — a stuck file will be swept by a later window anyway. + } + } + } + + /// + /// Same job as UpdateRestartGuardTests.PutAway: prompts closed, session settled, window + /// shut — from a finally, because the run where it matters is the run where an + /// assertion failed and left the window up (#474). + /// + private static void PutAwayMainWindow(MainWindow? window, QuerySessionControl? session = null) + { + if (window == null || !window.IsVisible) + return; + + foreach (var prompt in window.OwnedWindows.ToList()) + prompt.Close(); + + session?.MarkClean(); + Dispatcher.UIThread.RunJobs(); + window.Close(); + Dispatcher.UIThread.RunJobs(); + } + + /// Detached twin of the above, mirroring DetachedUnsavedChangesTests.PutAway. + private static void PutAwayDetached(Window? detached, QuerySessionControl? session) + { + if (detached == null || !detached.IsVisible) + return; + + foreach (var prompt in detached.OwnedWindows.ToList()) + prompt.Close(); + + session?.MarkClean(); + Dispatcher.UIThread.RunJobs(); + detached.Close(); + Dispatcher.UIThread.RunJobs(); + } + + private static TabItem TabOf(MainWindow window, QuerySessionControl session) => + window.MainTabControl.Items.OfType().Single(t => t.Content == session); + + private static Button CloseButton(TabItem tab) => + ((StackPanel)tab.Header!).Children.OfType