Close three ways the app could quietly lose a user's work - #485
Conversation
An adversarial review of the unsaved-changes work turned up three data-loss routes that all bypassed the guards #462 and #473 built: - The About window's Velopack "Restart Now" called ApplyUpdatesAndRestart, which exits the process without ever raising Closing. The unsaved-changes walk never ran, so dirty edits were discarded without a question - and OnClosed's session save never ran either, so with the saved tab list already cleared at startup the updated app relaunched empty-handed. The walk now lives in ConfirmAllUnsavedWorkAsync, reused by the close path and run by the About window before the restart; a Cancel aborts the restart with the update still downloaded, and PersistSessionForRestart writes the open tabs down after the walk (a Save answer can give a scratch tab a file worth restoring). - SaveQueryToPath wrote the user's file with a plain truncate-then- write, so a save that died halfway - disk full, crash - destroyed the only copy of the file it was trying to update. It now stages through AtomicFile like the settings writers already did: sibling .tmp, then rename over the top, so a failed save leaves the original bytes on disk and the session dirty. - "Open in Query Editor" pasted a plan's statement over the editor unconditionally - the one wholesale overwrite that skipped the dirty tracking entirely. It now confirms (Replace/Cancel) when the editor holds typed-but-unsaved work; a clean or empty editor replaces without a prompt, exactly as before. ConfirmationDialog rather than the three-button UnsavedChangesDialog, because a Save answer would need the save pipeline that lives on MainWindow; the dialog's fixed button width became a minimum so the Replace caption is not clipped. Tests pin the walk (a clean window answers yes without a prompt, a dismissed prompt refuses with everything intact), the restart persistence, save-in-place round-tripping with no staging file left behind, a save that cannot stage leaving the original untouched, and all three editor-replacement paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (Owner is MainWindow main) | ||
| { | ||
| if (!await main.ConfirmAllUnsavedWorkAsync()) | ||
| return; | ||
|
|
||
| /* After the walk, not before: a Save answer in the walk can give a | ||
| scratch tab a file, which this then writes down for the restore. */ | ||
| main.PersistSessionForRestart(); | ||
| } | ||
|
|
||
| _velopackMgr.ApplyUpdatesAndRestart(_velopackUpdate.TargetFullRelease); |
There was a problem hiding this comment.
UpdateLink is never disabled while this await is in flight. Before this change the gap between click and ApplyUpdatesAndRestart was one dialog; now it can be a whole walk over every dirty tab and detached window, each awaiting its own UnsavedChangesDialog. A second click on UpdateLink during that window re-enters UpdateLink_Click (the _updateDownloaded/_velopackMgr/_velopackUpdate guard is still satisfied), starting a second, concurrent ConfirmAllUnsavedWorkAsync() walk over the same tabs/paths — two UnsavedChangesDialogs racing for the same tab, potentially two AtomicFile writes to the same path, and possibly ApplyUpdatesAndRestart firing twice. Worth disabling the link (or setting an in-flight guard) for the duration of the confirm-and-restart sequence.
Separately: the confirmation dialogs this walk raises (UnsavedChangesDialog via ConfirmCloseAsync/ConfirmDetachedCloseAsync) are parented to main (MainWindow), while this (AboutWindow) is the window actually on top and stays open and interactive the whole time. That's exactly the failure mode ConfirmDetachedCloseAsync's own comment calls out ("a prompt parented to a window behind it is a prompt nobody can see") — here the user can still click around the still-open About window while a data-loss-preventing prompt is only modal relative to the window behind it.
There was a problem hiding this comment.
Fixed in 8f9381f — one _updateActionInFlight latch at the top of UpdateLink_Click covers every awaiting branch (dialog, walk, download), so a second click is a no-op rather than a concurrent copy of whichever step is in flight.
| (RestoreOpenPlans had already cleared the saved list at startup). Ask the | ||
| same questions the close path asks, and if anyone answers Cancel, abort | ||
| the restart and leave this window usable — the update stays downloaded. */ | ||
| if (Owner is MainWindow main) |
There was a problem hiding this comment.
if (Owner is MainWindow main) silently skips the entire unsaved-work walk and falls straight through to _velopackMgr.ApplyUpdatesAndRestart(...) if Owner is ever not a MainWindow — i.e. this is a silent fallback to exactly the pre-fix, data-losing behavior this PR is closing. It happens to always be true today (both call sites construct AboutWindow and ShowDialog it with the app's single MainWindow), but there's no assertion guarding that invariant, so a future refactor that shows AboutWindow differently reintroduces the bug with no compiler or test signal.
Also worth noting: none of the 9 new tests drive this method itself — UpdateRestartGuardTests calls window.ConfirmAllUnsavedWorkAsync() and window.PersistSessionForRestart() directly, never through AboutWindow.UpdateLink_Click. That's reasonable given headless UI can't click the "Restart Now" link, but it does mean the actual wiring here (order of the two calls, the Owner cast, the early return on cancel) is unverified by the suite.
There was a problem hiding this comment.
Fixed in 8f9381f — the main window is now resolved through the application lifetime (Owner as the fast path), so the guard can't silently vanish under a different owner. If no main window exists at all there are no sessions to lose, so restarting without a walk is genuinely safe; the comment states the fail-open hazard explicitly.
|
Reviewed the diff. The three fixes are well-targeted and the extraction of Left two inline comments on
Also flagged: the Nothing else stood out — no untrusted-XML/T-SQL surface touched here, no version-bump files touched, no new warnings apparent from the diff. |
…losed Two findings from the gate review of the restart guard: - Every branch of UpdateLink_Click awaits (dialog, unsaved-work walk, download) with the link still clickable, and the walk made the window between click and restart arbitrarily long - a second click started a concurrent copy of whichever step was in flight. One latch at the top now covers all branches. - The walk was gated on 'Owner is MainWindow', which fails OPEN: shown with any other owner, the guard silently vanishes and the route is back to discarding dirty edits. The main window is now resolved through the application lifetime with Owner as the fast path; if no main window exists at all there are no sessions to lose, so restarting without a walk is genuinely safe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
| internal async void OnOpenInEditorRequested(object? sender, string queryText) | ||
| { | ||
| /* This used to assign unconditionally — the one wholesale overwrite in the app that | ||
| skipped #462's dirty tracking, so a typed-but-unsaved query was replaced without a | ||
| question. ConfirmationDialog rather than the three-button UnsavedChangesDialog: | ||
| a Save answer here would need the save pipeline, which lives on MainWindow and | ||
| takes the tab — machinery this control has no business growing for one prompt. | ||
| Dismissing the dialog is a no, and a no leaves the editor and the sub-tab alone. */ | ||
| if (ReplaceNeedsConfirmation(IsDirty, QueryEditor.Text)) | ||
| { | ||
| var replace = await ShowConfirmationDialog( | ||
| "Unsaved Changes", | ||
| "The query editor has unsaved changes.\n\nReplace them with this statement? Your current text will be lost.", | ||
| confirmCaption: "Replace"); | ||
|
|
||
| if (!replace) | ||
| return; |
There was a problem hiding this comment.
Minor: this handler now has an await between the dirty check and the text replacement, but nothing stops it from running twice concurrently. A double-click (or two statements' "Open in Editor" clicked back-to-back) on a dirty editor fires two overlapping calls, each reading the same IsDirty/QueryEditor.Text and each popping its own ConfirmationDialog — you can end up with two stacked "Replace?" prompts.
Not data-loss (the eventual QueryEditor.Text = queryText assignment is still gated by an explicit Replace click either way), but it's the same class of bug this PR just fixed in AboutWindow.UpdateLink_Click with the _updateActionInFlight latch. Worth a similar guard here for consistency, or at least worth confirming it's an accepted gap.
There was a problem hiding this comment.
Fixed in 6213ab4 — same latch pattern as the update link: _replacePromptInFlight makes the second click a no-op while the prompt is up, with the flag cleared in finally so a dialog exception can't wedge the feature.
|
Reviewed both commits. This is a well-scoped, well-tested fix for the three data-loss routes described in the PR body, and the second commit already closes the two follow-up gaps (click-latch, fail-closed Walked through the changed paths specifically for the things this repo cares about:
Left one inline note: |
Same reentrancy class the previous commit fixed in AboutWindow: the Replace confirmation put an await between the dirty check and the assignment, so two back-to-back Open in Query Editor clicks stacked two prompts over the same buffer. Not data loss - the assignment stays gated on an explicit Replace - but the first click now wins and the second is a no-op while the prompt is up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
|
Reviewed the diff. This is a tight, well-scoped fix for the three data-loss routes described, and the changes hold up:
No untrusted-input, SQL-generation, or repo-convention issues apply here (no T-SQL, no version bump, no PlanViewer.Core/Web linkage touched). Didn't spot any correctness gaps in the close/restart/save control flow worth blocking on. |
1 participant
What does this PR do?
Fixes the three Major data-loss findings from the 2026-09-02 adversarial review:
ApplyUpdatesAndRestartexits without raisingClosing, so the Warn about unsaved query changes, and mark modified tabs #462/Detached windows discard unsaved query changes without asking #473 unsaved-changes walk never ran and the session save never happened — dirty edits silently discarded and the updated app relaunched with no tabs. The walk now lives inConfirmAllUnsavedWorkAsync(pure: no close bookkeeping), reused by the close path and run by the About window before restarting; Cancel aborts the restart with the update still downloaded, and the session is persisted after the walk since a Save answer can give a scratch tab a path worth restoring.SaveQueryToPathnow stages through the existingAtomicFile(sibling .tmp + rename), so a failed save leaves the original bytes on disk and the session dirty. Attribute/ACL trade documented in a comment.ConfirmationDialogwhen the editor holds typed-but-unsaved work; clean or empty editors replace without a prompt, exactly as before. The decision is a pure testable seam (ReplaceNeedsConfirmation).How was this tested?
Nine new tests across
UpdateRestartGuardTests,OpenInEditorOverwriteTests, and extendedOpenSaveQueryTests— including a save that cannot stage its temp leaving the original untouched (that save succeeded under the old code). Full suite at dev tip + fix: 417 tests, 416 passed, 1 platform skip, 0 failed, on Windows.🤖 Generated with Claude Code
https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n