diff --git a/.claude/skills/maintenance/SKILL.md b/.claude/skills/maintenance/SKILL.md new file mode 100644 index 00000000..86d4125c --- /dev/null +++ b/.claude/skills/maintenance/SKILL.md @@ -0,0 +1,74 @@ +--- +name: maintenance +description: Quarterly maintenance pass (every 1-3 months) — dependency/security audit, build health, and repo hygiene for PerformanceMonitor and PerformanceStudio +argument-hint: [optional: deps | build | all] +disable-model-invocation: false +--- + +# Routine Maintenance Pass + +A repeatable every-1-3-months health check for the .NET desktop + SQL-monitoring repos +(PerformanceMonitor = WPF, PerformanceStudio = Avalonia; same dependency/build/release shape). +`$ARGUMENTS` optionally scopes it (`deps`, `build`, or `all` — default `all`). + +Work top to bottom. Do read-only scans first and report findings; make fixes on a feature +branch/worktree (branch protection: never commit to `dev`/`main`), build + test, then PR to `dev`. +Low-risk patch/minor bumps can go in one PR; majors and anything release-critical get their own. + +## A. Dependencies & Security + +> **Scope — scan every project, not just the solution.** A repo's `.sln` may not list every project. PerformanceStudio's `PlanViewer.sln` omits `server/PlanShare.csproj` and the SSMS VSIX (`PlanViewer.Ssms`, `PlanViewer.Ssms.Installer`), so the `dotnet list .sln …` scans and the solution build all silently skip them. Run the checks against those projects too. The net472 VSIX is old-style, so `dotnet list` is unreliable on it — read its ``s by hand (its `Microsoft.VSSDK.BuildTools` is intentionally held on the 17.x line; 18.x is un-restorable from nuget.org and targets VS 18, not VS 2022). Where the `.sln` covers all projects, the solution is enough. + +1. **Outdated packages.** `dotnet list .sln package --outdated` + - Take low-risk **patch/minor** bumps (Microsoft.Extensions.*, Test.Sdk, etc.). + - **Majors get their own effort** — especially the update framework (Velopack: bump the library AND the `vpk` CLI pin in the release workflow — `.github/workflows/release.yml` for PerformanceStudio — together; validate the in-app + Setup.exe update path) and anything release-critical. + - **Engine-wrapped bindings** (e.g. DuckDB.NET) trail the native engine — bump when the binding catches up, and validate behavior (for DuckDB run `tools/CompactionRepro` re: the parquet-COPY memory-limit floor before changing the version). + - After version edits, **if the repo uses lock files** (`packages.lock.json` — PerformanceMonitor does; PerformanceStudio does not), regenerate them: `dotnet restore .sln --force-evaluate` (CI restores `--locked-mode`). + +2. **Vulnerable packages (security).** `dotnet list .sln package --vulnerable --include-transitive` + - Must be **zero**. Any hit (incl. transitive) is urgent — bump or pin to a fixed version. This catches CVEs the `--outdated` check does not. + - **Code-level pass, not just packages:** run the `security-review` skill/agent on the diff since the last maintenance pass. These apps have real attack surface beyond their dependencies — PerformanceStudio's MCP server opens a local network listener, both store DB credentials (Windows Credential Manager), and both parse untrusted input (e.g. execution-plan XML). Triage anything it flags. + - **Calibrate severity to the deployment.** PerformanceStudio runs on a single-user personal laptop, and its MCP tools are strictly read-only (no arbitrary SQL, no writes/config changes). So loopback-bound / opt-in / local-IPC findings — the MCP listener, the named-pipe single-instance server — are **Low/informational here, not High**: there's no other local user or attacker to exploit them, and read-only tools can at most leak data they already return. Reserve High for *remotely reachable* vectors (e.g. a missing `Host`/`Origin` check that allows DNS rebinding) or credential disclosure. This calibration would change only if Studio shipped the MCP server enabled-by-default or ran on a shared/multi-user host. Don't re-raise the same local-IPC findings at High each pass. + +3. **Deprecated packages.** `dotnet list .sln package --deprecated` — replace anything abandoned. + +4. **Framework / runtime currency.** Confirm the TFM is on a **supported, released** .NET (do NOT move to a preview). Take the latest servicing patch of the current major. WPF/Avalonia track the runtime/their own NuGet — check both. + +5. **CI tool & action pins.** In `.github/workflows/*.yml`: confirm `uses:` actions are on current majors, and that any `dotnet tool install` is **version-pinned to match its library** (e.g. `vpk --version` must equal the Velopack PackageReference). Bump GitHub Actions that are behind / deprecated. + +6. **Dependabot (optional — not a finding).** Two separate features: *security alerts* (passive CVE notifications that close the gap between manual `--vulnerable` passes — mild value) and *version-update PRs* (automated bump PRs — redundant and noisy once you do periodic manual sweeps). For PerformanceStudio, Erik relies on the manual passes; treat Dependabot as **optional and do not report it as a finding**. If alerts are ever wanted they're a one-toggle enable (repo Settings → Code security, no config file); the version-update PRs aren't wanted. + +## B. Code & Build Health + +7. **Zero-warning build.** Build the whole solution and capture warnings — the standard is **0**: + ``` + dotnet build .sln -c Debug --nologo 2>&1 | Select-String ": warning " + ``` + - Kill the running app(s) first OR build in a worktree — a running Dashboard/Lite/Studio locks its `bin` DLLs (MSB3021). Note: an incremental no-op build emits no warnings; force a clean compile of any project you're checking. + - Fix each warning honestly (don't add to `NoWarn` to silence). Remove dead code (e.g. an unused test seam → CS0649). + +8. **NoWarn review.** Scan each csproj ``: every suppressed rule should have a reason (these repos keep inline comments). Remove suppressions that no longer fire; don't let the list grow silently. Most existing CA suppressions are intentional high-count ones — leave those. + +9. **Stale markers.** `grep -rE "\b(TODO|FIXME|HACK|XXX)\b" --include=*.cs` — resolve or file an issue; a `// TODO: restore to X` next to a non-X value may mean the comment is the leftover (ask before flipping). + +10. **Git repo hygiene.** + - **Line endings / `.gitattributes`.** If the repo has no `.gitattributes`, line endings drift (CRLF/LF mixed, `core.autocrlf=false`) and bulk edits balloon diffs. Add one and run a **dedicated** `git add --renormalize .` commit (its own PR, when no other work is in flight — it touches nearly every file). + - **Stale branches & worktrees.** `git worktree list` — remove leftover worktrees (anything under `.claude/worktrees/` or other agent/isolation worktrees) with `git worktree remove`. Then `git fetch --prune` to drop stale remote-tracking refs, and audit: `git branch --merged origin/dev` (local branches already in dev — safe to delete) and `git branch -r` (remote branches from closed/merged PRs). Delete merged/dead branches; **keep intentionally-parked ones** (note which and why — e.g. a blocked-upgrade branch like `upgrade/avalonia-12`). For branches *you didn't create*, surface and confirm before deleting rather than assuming abandoned. Confirm open PRs are still wanted. + +## C. App data & runtime hygiene (lighter) + +11. **Retention / archive end-to-end.** Confirm the app's data retention/purge and (Lite) parquet archiving actually prune old data, and logs rotate. A new time-series table must be registered for retention/archive or it grows forever. + +12. **Perf regression spot-check.** Re-run the UI-latency-under-load harness if available; watch known hot spots (e.g. the Lite Blocking tab render hitch). Quick collector-health pass. + +## D. Release & platform currency (lighter) + +13. **Release infra freshness.** Test servers online/patched; signing cert (SignPath) not near expiry; cloud creds (`az`/`aws`) valid; the `release-checklist` skill still accurate. + - **Cross-platform publish smoke.** `dotnet publish` the desktop app for the non-Windows runtimes it ships (PerformanceStudio: `linux-x64`, `osx-arm64`/`osx-x64`) and confirm each still produces a runnable app. The Avalonia/SkiaSharp native pins are fragile — the Linux `SkiaSharp.NativeAssets.Linux` pin exists to guard GitHub issue #139 — and a Windows-only build won't catch a broken Linux/macOS runtime. + +14. **SQL Server / cloud drift + bundled tools.** New SQL Server CU/version, new DMVs/columns, Azure SQL DB / RDS changes (cloud collector paths have a bug history); refresh bundled community procs (sp_WhoIsActive, sp_BlitzLock, sp_HealthParser, sp_HumanEventsBlockViewer). + +## Output + +Report per section: ✅ clean / ⚠️ findings (with the fix made or recommended). Group merged PRs and +"parked" items (e.g. a major bump deferred) so the next pass knows where things stand. diff --git a/.gitignore b/.gitignore index 3bcca1d9..5da77bca 100644 --- a/.gitignore +++ b/.gitignore @@ -424,5 +424,6 @@ FodyWeavers.xsd # Internal development files (not for public release) .internal/ -.claude/ +.claude/* +!.claude/skills/ CLAUDE.md diff --git a/server/PlanShare/Program.cs b/server/PlanShare/Program.cs index 93148c75..c88a8657 100644 --- a/server/PlanShare/Program.cs +++ b/server/PlanShare/Program.cs @@ -105,6 +105,16 @@ created_at TEXT NOT NULL const int MaxTtlDays = 365; +// Depth ceiling for parsing an uploaded share, mirroring PlanViewer.Core's +// AnalysisJson.MaxDepth — that class is the source of truth for how deep a serialized +// AnalysisResult can go (#431: an operator costs two JSON levels, so the JsonDocument +// default of 64 rejects a plan ~30 operators deep as "Invalid JSON" after the client +// happily serialized it at 1024). Mirrored rather than referenced because this project +// deliberately takes no dependency on PlanViewer.Core, and a shared constant only helps +// call sites that reference it; this one cannot. If AnalysisJson.MaxDepth ever changes, +// change this with it — the client-side depth tests pin 1024, so start there. +var shareDocumentOptions = new JsonDocumentOptions { MaxDepth = 1024 }; + // --- Endpoints --- app.MapGet("/health", () => Results.Content("OK", "text/plain")); @@ -127,11 +137,13 @@ created_at TEXT NOT NULL return Results.BadRequest("Empty body"); } - // Parse and extract ttl_days from the JSON + // Parse and extract ttl_days from the JSON. shareDocumentOptions, not defaults: this body + // wraps a full serialized analysis, and the default 64-level ceiling turned a deep plan's + // legitimate upload into a 400 before ttl_days was ever read. int ttlDays = 7; try { - using var doc = JsonDocument.Parse(body); + using var doc = JsonDocument.Parse(body, shareDocumentOptions); if (doc.RootElement.TryGetProperty("ttl_days", out var ttlProp) && ttlProp.TryGetInt32(out var t)) ttlDays = Math.Clamp(t, 1, MaxTtlDays); } diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 32cb4c9e..4f2bcb34 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -15,7 +15,7 @@ Tests and server/ projects are outside src/ and are unaffected. --> - 1.23.0 + 1.24.0 Erik Darling Darling Data LLC Performance Studio diff --git a/src/PlanViewer.App/AboutWindow.axaml.cs b/src/PlanViewer.App/AboutWindow.axaml.cs index 5db3a71f..b77fa041 100644 --- a/src/PlanViewer.App/AboutWindow.axaml.cs +++ b/src/PlanViewer.App/AboutWindow.axaml.cs @@ -6,6 +6,7 @@ using System; using System.Diagnostics; +using System.Threading.Tasks; using System.Reflection; using System.Runtime.InteropServices; using Avalonia.Controls; @@ -188,7 +189,28 @@ private async void CheckUpdate_Click(object? sender, RoutedEventArgs e) private bool _updateDownloaded; + /* Every branch of UpdateLink_Click awaits — a dialog, the unsaved-work walk, a download — + and the link stays clickable the whole time, so a second click would start a second + concurrent copy of whichever step is in flight (two restart dialogs, two walks prompting + about the same tabs, two downloads). One latch at the top covers all of them. */ + private bool _updateActionInFlight; + private async void UpdateLink_Click(object? sender, PointerPressedEventArgs e) + { + if (_updateActionInFlight) + return; + _updateActionInFlight = true; + try + { + await HandleUpdateLinkClickAsync(); + } + finally + { + _updateActionInFlight = false; + } + } + + private async Task HandleUpdateLinkClickAsync() { // Step 3: User clicks "Restart now" after download — confirm first if (_updateDownloaded && _velopackMgr != null && _velopackUpdate != null) @@ -231,6 +253,33 @@ private async void UpdateLink_Click(object? sender, PointerPressedEventArgs e) if (result) { + /* ApplyUpdatesAndRestart kills the process outright: MainWindow.OnClosing + never fires, so the unsaved-changes walk (#462/#473) never ran on this + route and dirty edits were discarded without a question — and OnClosed's + session save never ran either, so the relaunched app restored nothing + (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. */ + /* Resolved through the application lifetime, not Owner: an Owner-typed check + would fail OPEN — shown with any other owner, the walk silently vanishes and + this route is right back to discarding dirty edits, the exact bug being + fixed. The main window owns every session, so if none exists there is no + unsaved work to lose and restarting without a walk is genuinely safe. */ + var main = Owner as MainWindow + ?? (Avalonia.Application.Current?.ApplicationLifetime + as Avalonia.Controls.ApplicationLifetimes.IClassicDesktopStyleApplicationLifetime) + ?.MainWindow as MainWindow; + + if (main != null) + { + 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); } return; diff --git a/src/PlanViewer.App/App.axaml.cs b/src/PlanViewer.App/App.axaml.cs index 8b23a246..ea9104cf 100644 --- a/src/PlanViewer.App/App.axaml.cs +++ b/src/PlanViewer.App/App.axaml.cs @@ -54,7 +54,11 @@ public override void OnFrameworkInitializationCompleted() // Register the .sqlplan association (Windows/Linux) off the UI thread so it // never delays first paint. Best-effort; the OS then routes double-clicks to // the existing argv/pipe open path. No-op on macOS (handled by Info.plist). - Task.Run(FileAssociationService.RegisterForCurrentExecutable); + // Never in the test host: this writes HKCU\Software\Classes, and the harness + // booting the real App (#451) rewrote the machine's .sqlplan association and + // DefaultIcon to point at the test runner executable — confirmed live. + if (!AppRuntimeMode.IsTestHost) + Task.Run(FileAssociationService.RegisterForCurrentExecutable); base.OnFrameworkInitializationCompleted(); } diff --git a/src/PlanViewer.App/AppRuntimeMode.cs b/src/PlanViewer.App/AppRuntimeMode.cs new file mode 100644 index 00000000..be25f53b --- /dev/null +++ b/src/PlanViewer.App/AppRuntimeMode.cs @@ -0,0 +1,25 @@ +namespace PlanViewer.App; + +/// +/// Process-wide answer to "is this the real app or the test host?" +/// +/// The headless test harness (#451) boots the REAL — deliberately, +/// because MainWindow resolves styles from the application XAML — which means the real +/// startup side effects run inside the test runner unless something says otherwise. That +/// was not hypothetical: a local dotnet test rewrote HKCU's .sqlplan association to +/// point at the test host executable, polluted the developer's Recent Plans with fixture +/// paths, and destroyed the saved open-tab list, all confirmed live. +/// +/// This is the one seam those side effects consult, rather than a scattering of +/// environment sniffs. The harness sets it before the first App boots; nothing in the +/// product ever sets it, so in the real app every check reads a constant false and +/// behavior is unchanged. +/// +internal static class AppRuntimeMode +{ + /// + /// True only inside the test host. Set once by the test harness's module initializer, + /// never by the app itself. + /// + internal static bool IsTestHost; +} diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs index 7dd13b2d..72163891 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Interaction.cs @@ -360,7 +360,11 @@ private async void SavePlan_Click(object? sender, RoutedEventArgs e) catch (Exception ex) { System.Diagnostics.Debug.WriteLine($"SavePlan failed: {ex.Message}"); - CostText.Text = $"Save failed: {(ex.Message.Length > 60 ? ex.Message[..60] + "..." : ex.Message)}"; + /* #452's mirror, the sixth site (spotted during the review sweep): pre-cutting to + 60 characters threw away exactly the part of an I/O error that says what to fix. + Full message out; the tooltip carries whatever the label clips. */ + CostText.Text = $"Save failed: {ex.Message}"; + ToolTip.SetTip(CostText, ex.Message); } } } diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs b/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs index 99132efe..25e175a7 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.Statements.cs @@ -16,13 +16,16 @@ namespace PlanViewer.App.Controls; public partial class PlanViewerControl : UserControl { - private void PopulateStatementsGrid(List statements) + /* Takes the container-aware entries rather than bare statements (#456 follow-up): the grid + now lists stored procedure and UDF body statements alongside the outer batch, and a row + needs to say WHICH module its statement came from or five bare SELECTs are indistinguishable. */ + private void PopulateStatementsGrid(List statements) { StatementsHeader.Text = $"Statements ({statements.Count})"; - var hasActualTimes = statements.Any(s => s.QueryTimeStats != null && - (s.QueryTimeStats.CpuTimeMs > 0 || s.QueryTimeStats.ElapsedTimeMs > 0)); - var hasUdf = statements.Any(s => s.QueryUdfElapsedTimeMs > 0); + var hasActualTimes = statements.Any(e => e.Statement.QueryTimeStats != null && + (e.Statement.QueryTimeStats.CpuTimeMs > 0 || e.Statement.QueryTimeStats.ElapsedTimeMs > 0)); + var hasUdf = statements.Any(e => e.Statement.QueryUdfElapsedTimeMs > 0); // Build columns StatementsGrid.Columns.Clear(); @@ -129,7 +132,7 @@ private void PopulateStatementsGrid(List statements) var rows = new List(); for (int i = 0; i < statements.Count; i++) { - var stmt = statements[i]; + var stmt = statements[i].Statement; var allWarnings = stmt.PlanWarnings.ToList(); if (stmt.RootNode != null) CollectNodeWarnings(stmt.RootNode, allWarnings); @@ -137,6 +140,14 @@ private void PopulateStatementsGrid(List statements) var fullText = stmt.StatementText; if (string.IsNullOrWhiteSpace(fullText)) fullText = $"Statement {i + 1}"; + + /* A body statement gets its module name in front ("dbo.Proc > SELECT ...") in both + the cell and its tooltip — display only. Copy/open-in-editor read + row.Statement.StatementText and hand out the statement exactly as the plan + recorded it, prefix-free. */ + if (!string.IsNullOrEmpty(statements[i].ContainerPath)) + fullText = $"{statements[i].ContainerPath} > {fullText}"; + var displayText = fullText.Length > 120 ? fullText[..120] + "..." : fullText; rows.Add(new StatementRow diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs index b35d66ad..5df3f24e 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs @@ -345,9 +345,20 @@ public bool LoadPlan(string planXml, string label, string? queryText = null) PlanAnalysisPipeline.AnalyzeParsed(_currentPlan, ConfigLoader.Load(), _serverMetadata); - var allStatements = _currentPlan.Batches - .SelectMany(b => b.Statements) - .Where(s => s.RootNode != null) + /* #456 gave the analysis pipeline and Human/Robot Advice (ResultMapper) the shared + PlanStatements.EnumerateAll traversal, which descends into stored procedure and UDF + bodies. The grid and the MCP registration below kept walking batch.Statements, so an + EXEC plan showed one grid row — the EXEC itself — and registered near-zero + counts, while the advice discussed dozens of warnings the UI could neither display nor + navigate to. Same traversal here so what the grid shows, what the session reports, and + what the advice says are the same plan. */ + var everyStatement = PlanStatements.EnumerateAllWithContainer(_currentPlan).ToList(); + + /* Only statements with a root node can render on the canvas. The same filter has always + applied to the outer batch (where the parser's synthetic statement roots mean it + rarely excludes anything); it now applies across the whole traversal. */ + var allStatements = everyStatement + .Where(e => e.Statement.RootNode != null) .ToList(); if (allStatements.Count == 0) @@ -361,16 +372,20 @@ public bool LoadPlan(string planXml, string label, string? queryText = null) PlanScrollViewer.IsVisible = true; // Always show statement grid — useful summary even for single-statement plans - _allStatements = allStatements; + _allStatements = allStatements.Select(e => e.Statement).ToList(); PopulateStatementsGrid(allStatements); ShowStatementsPanel(); StatementsGrid.SelectedIndex = 0; - // Register with MCP session manager for AI tool access - // Count warnings from both statement-level PlanWarnings and all node Warnings + /* Register with MCP session manager for AI tool access. Counts run over EVERY statement, + renderable or not, because that is what the advice an MCP client reads was built from: + statement-level PlanWarnings plus all node warnings, proc/UDF bodies included. + StatementCount likewise matches the analysis output's total_statements rather than the + grid's renderable subset. */ int warningCount = 0, criticalCount = 0; - foreach (var s in allStatements) + foreach (var entry in everyStatement) { + var s = entry.Statement; warningCount += s.PlanWarnings.Count; criticalCount += s.PlanWarnings.Count(w => w.Severity == PlanWarningSeverity.Critical); if (s.RootNode != null) @@ -385,8 +400,8 @@ public bool LoadPlan(string planXml, string label, string? queryText = null) Source = sessionSource, Plan = _currentPlan, QueryText = queryText, - StatementCount = allStatements.Count, - HasActualStats = allStatements.Any(s => s.QueryTimeStats != null), + StatementCount = everyStatement.Count, + HasActualStats = everyStatement.Any(e => e.Statement.QueryTimeStats != null), WarningCount = warningCount, CriticalWarningCount = criticalCount, MissingIndexCount = _currentPlan.AllMissingIndexes.Count diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs b/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs index a1db6987..bad58e2b 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.Editor.cs @@ -155,8 +155,60 @@ private void OnContextMenuOpening(object? sender, System.ComponentModel.CancelEv } } - private void OnOpenInEditorRequested(object? sender, string queryText) + /// + /// Whether "Open in Query Editor" has to stop and ask before pasting over the editor. + /// + /// Only typed-but-unsaved work earns a prompt: a clean editor is already on disk + /// (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 + /// CollectOpenTabEntries made for the session-restore list. + /// + internal static bool ReplaceNeedsConfirmation(bool isDirty, string? currentText) => + isDirty && !string.IsNullOrEmpty(currentText); + + /* The prompt below puts an await between the dirty check and the replacement, so without + a latch two back-to-back "Open in Query Editor" clicks would each read the same dirty + state and stack two Replace prompts — the same reentrancy class the About window's + update link had. Not data loss (the assignment stays gated on an explicit Replace + either way), just two dialogs racing; first click wins, the second is a no-op. */ + private bool _replacePromptInFlight; + + /// + /// Puts a statement from a plan into the query editor. Internal (like the MainWindow + /// Click handlers) so tests can drive it without a plan viewer to click through. + /// + internal async void OnOpenInEditorRequested(object? sender, string queryText) { + if (_replacePromptInFlight) + return; + + /* 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)) + { + _replacePromptInFlight = true; + bool replace; + try + { + 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"); + } + finally + { + _replacePromptInFlight = false; + } + + if (!replace) + return; + } + QueryEditor.Text = queryText; SubTabControl.SelectedIndex = 0; // Switch to the editor tab QueryEditor.Focus(); diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs b/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs index 9f9582ad..cc250454 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs @@ -197,15 +197,7 @@ failure is reported. A SQL error is the one string in this app a user most needs // Replace loading content with the plan viewer SetStatus($"{planType} plan captured ({sw.Elapsed.TotalSeconds:F1}s)"); - var viewer = new PlanViewerControl(); - viewer.Metadata = _serverMetadata; - viewer.ConnectionString = _connectionString; - viewer.SetConnectionServices(_credentialService, _connectionStore); - if (_serverConnection != null) - viewer.SetConnectionStatus(_serverConnection.ServerName, _selectedDatabase); - viewer.OpenInEditorRequested += OnOpenInEditorRequested; - viewer.LoadPlan(planXml, tabLabel, queryText); - loadingTab.Content = viewer; + ShowCapturedPlan(loadingTab, planXml, tabLabel, queryText); HumanAdviceButton.IsEnabled = true; RobotAdviceButton.IsEnabled = true; } @@ -224,6 +216,32 @@ failure is reported. A SQL error is the one string in this app a user most needs } } + /// + /// Puts a captured plan into the tab that has been showing the progress spinner for it. + /// + /// Both execution paths end here — the estimated/actual capture and Get Actual Plan — and + /// this is the moment a plan appears in a session, so it is the moment Compare Plans has to be + /// re-decided. That happens through the sub-tab + /// rather than a call added below, because the assignment + /// on the last line is what it is watching for (#447). + /// + /// Internal so a test can drive the plan landing with XML it already has, rather than + /// needing a SQL Server to produce some. The half of these paths that reaches out to a server + /// is above this; everything that decides what the user ends up looking at is here. + /// + internal void ShowCapturedPlan(TabItem planTab, string planXml, string tabLabel, string queryText) + { + var viewer = new PlanViewerControl(); + viewer.Metadata = _serverMetadata; + viewer.ConnectionString = _connectionString; + viewer.SetConnectionServices(_credentialService, _connectionStore); + if (_serverConnection != null) + viewer.SetConnectionStatus(_serverConnection.ServerName, _selectedDatabase); + viewer.OpenInEditorRequested += OnOpenInEditorRequested; + viewer.LoadPlan(planXml, tabLabel, queryText); + planTab.Content = viewer; + } + /// /// Reports a query failure in the plan tab, in full (#448). /// @@ -402,15 +420,7 @@ private async void GetActualPlan_Click(object? sender, RoutedEventArgs e) } SetStatus($"Actual plan captured ({sw.Elapsed.TotalSeconds:F1}s)"); - var actualViewer = new PlanViewerControl(); - actualViewer.Metadata = _serverMetadata; - actualViewer.ConnectionString = _connectionString; - actualViewer.SetConnectionServices(_credentialService, _connectionStore); - if (_serverConnection != null) - actualViewer.SetConnectionStatus(_serverConnection.ServerName, _selectedDatabase); - actualViewer.OpenInEditorRequested += OnOpenInEditorRequested; - actualViewer.LoadPlan(actualPlanXml, tabLabel, queryText); - loadingTab.Content = actualViewer; + ShowCapturedPlan(loadingTab, actualPlanXml, tabLabel, queryText); } catch (OperationCanceledException) { @@ -432,10 +442,10 @@ private async void GetActualPlan_Click(object? sender, RoutedEventArgs e) } /// - /// Shows a modal confirmation dialog and returns true if the user clicked OK. + /// Shows a modal confirmation dialog and returns true if the user confirmed. /// - private Task ShowConfirmationDialog(string title, string message) - => Dialogs.ConfirmationDialog.ShowAsync(GetParentWindow(), title, message); + private Task ShowConfirmationDialog(string title, string message, string confirmCaption = "OK") + => Dialogs.ConfirmationDialog.ShowAsync(GetParentWindow(), title, message, confirmCaption); /// /// Extracts the database name from plan XML's StmtSimple DatabaseContext attribute. diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs b/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs index f2305266..52cd3993 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs @@ -13,12 +13,14 @@ using Avalonia.Input.Platform; using Avalonia.Interactivity; using Avalonia.Layout; +using Avalonia.LogicalTree; using Avalonia.Media; using AvaloniaEdit; using AvaloniaEdit.CodeCompletion; using AvaloniaEdit.TextMate; using Microsoft.Data.SqlClient; using PlanViewer.App.Dialogs; +using PlanViewer.App.Helpers; using PlanViewer.App.Services; using PlanViewer.Core.Interfaces; using PlanViewer.Core.Models; @@ -108,7 +110,6 @@ private bool AddPlanTab(string planXml, string queryText, bool estimated, string SubTabControl.Items.Add(tab); SubTabControl.SelectedItem = tab; - UpdateCompareButtonState(); return true; } @@ -159,7 +160,6 @@ private void ClosePlanTab_Click(object? sender, RoutedEventArgs e) if (tab.Content is PlanViewerControl viewer) viewer.Clear(); SubTabControl.Items.Remove(tab); - UpdateCompareButtonState(); } } @@ -180,7 +180,6 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) if (tab.Content is PlanViewerControl closeViewer) closeViewer.Clear(); SubTabControl.Items.Remove(tab); - UpdateCompareButtonState(); } break; @@ -199,7 +198,6 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) SubTabControl.Items.Remove(t); } SubTabControl.SelectedItem = keepTab; - UpdateCompareButtonState(); } break; @@ -215,7 +213,6 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) SubTabControl.Items.Remove(t); } SubTabControl.SelectedIndex = 0; // back to Editor - UpdateCompareButtonState(); break; } } @@ -225,10 +222,23 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) /// from another is the ordinary case, and counting only this session's own tabs left the button /// disabled in both — the reporter had to save a plan and reopen it to get at a comparison the /// app could already do. + /// + /// Called by the wired to this session's sub-tabs, and by + /// when this session leaves the tab strip — the + /// watcher only fires on sub-tab changes, and detaching changes none, so without that call the + /// button froze at whatever the window-wide count last said until the next plan landed. It used + /// to be called by hand at the five places that add or remove a plan tab, which is why the + /// paths that instead fill in an existing tab — every executed query — never reached it. /// - private void UpdateCompareButtonState() + internal void UpdateCompareButtonState() { - if (TopLevel.GetTopLevel(this) is MainWindow owner) + /* Logical tree, not TopLevel.GetTopLevel. A TabControl realises the selected tab's content + and nothing else, so a session sitting in a background tab has no visual root and cannot + see its own window — and a query started in one tab and left to run while the user works + in another lands its plan in exactly that state. GetTopLevel returned null there and the + fallback below silently reinstated the bug this method exists to fix. The logical parent + chain holds whether the tab is on screen or not. */ + if (this.FindLogicalAncestorOfType() is { } owner) { /* Refreshes every session, not just this one: a plan appearing here can be the second plan that makes Compare available over THERE. */ diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs b/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs index fe2bf9b8..ff786cb6 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs @@ -261,7 +261,14 @@ family as #448. */ SubTabControl.SelectedItem = tab; } - private void OnQueryStorePlansSelected(object? sender, List plans) + /// + /// Opens a plan tab for each plan picked out of the Query Store grid. + /// + /// Internal so a test can hand it plans it already has. The grid is what fetches them from + /// a server; nothing below this line needs one, which is what makes the Query Store side of + /// #447 testable without a live instance. + /// + internal void OnQueryStorePlansSelected(object? sender, List plans) { int loaded = 0; foreach (var qsPlan in plans) diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs index 8be55abe..b2934842 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; +using System.Text; using System.Text.Json; using System.Text.RegularExpressions; using System.Threading; @@ -18,6 +19,7 @@ using AvaloniaEdit.TextMate; using Microsoft.Data.SqlClient; using PlanViewer.App.Dialogs; +using PlanViewer.App.Helpers; using PlanViewer.App.Services; using PlanViewer.Core.Interfaces; using PlanViewer.Core.Models; @@ -37,6 +39,34 @@ 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 + /// UTF-8 without a BOM, which is what every save wrote before this existed. + /// + /// Captured at open so a save writes the bytes the file arrived with. SSMS writes + /// .sql files as UTF-16 with a BOM; opening one read fine (File.ReadAllText honors the + /// mark) and then the first Ctrl+S silently transcoded the whole file to UTF-8 — every + /// byte changed, the BOM gone, without the user asking for any of it. + /// + public Encoding? SourceFileEncoding { get; set; } + /// /// The editor text as of the last load or save. A new session starts empty, so a /// never-saved scratch tab with anything typed into it is dirty too (#462). @@ -131,6 +161,13 @@ public QuerySessionControl(ICredentialService credentialService, ConnectionStore _statusClearCts = null; }; + /* #447: a plan appearing in — or leaving — this session changes whether Compare Plans is + offered in every session in the window, not just this one. Watched here rather than + called at each site that produces a plan, because the sites that produce a plan are the + ones nobody remembers: executing a query fills in a tab that already exists, which is + neither an Add nor a Remove and is exactly the case the first fix missed. */ + TabContentWatcher.Watch(SubTabControl, UpdateCompareButtonState); + // Focus the editor when the Editor tab is selected; toggle plan-dependent buttons SubTabControl.SelectionChanged += (_, _) => { diff --git a/src/PlanViewer.App/Controls/QueryStoreGridControl.Fetch.cs b/src/PlanViewer.App/Controls/QueryStoreGridControl.Fetch.cs index f4b9a10f..e31ecd6a 100644 --- a/src/PlanViewer.App/Controls/QueryStoreGridControl.Fetch.cs +++ b/src/PlanViewer.App/Controls/QueryStoreGridControl.Fetch.cs @@ -57,7 +57,10 @@ private async void Fetch_Click(object? sender, RoutedEventArgs e) } catch (Exception ex) { - StatusText.Text = ex.Message.Length > 80 ? ex.Message[..80] + "..." : ex.Message; + /* #452's mirror: pre-cutting to 80 characters threw away text the strip would have + shown and the tooltip (wired in the constructor) would have recovered. The full + message goes out; where it gets clipped is the display's business. */ + StatusText.Text = ex.Message; } finally { @@ -107,7 +110,8 @@ private async System.Threading.Tasks.Task FetchPlansForRangeAsync() } catch (Exception ex) { - StatusText.Text = ex.Message.Length > 80 ? ex.Message[..80] + "..." : ex.Message; + // Same #452 mirror as Fetch_Click above: full message out, tooltip carries the rest. + StatusText.Text = ex.Message; } finally { diff --git a/src/PlanViewer.App/Controls/QueryStoreGridControl.Sort.cs b/src/PlanViewer.App/Controls/QueryStoreGridControl.Sort.cs index aa8bbb65..68de6e53 100644 --- a/src/PlanViewer.App/Controls/QueryStoreGridControl.Sort.cs +++ b/src/PlanViewer.App/Controls/QueryStoreGridControl.Sort.cs @@ -68,7 +68,8 @@ private async void OrderBy_SelectionChanged(object? sender, SelectionChangedEven catch (OperationCanceledException) { } catch (Exception ex) { - StatusText.Text = ex.Message.Length > 80 ? ex.Message[..80] + "..." : ex.Message; + // #452's mirror: full message out, the status tooltip carries what the strip clips. + StatusText.Text = ex.Message; } finally { diff --git a/src/PlanViewer.App/Controls/QueryStoreGridControl.WaitStats.cs b/src/PlanViewer.App/Controls/QueryStoreGridControl.WaitStats.cs index 077a15ec..124bdda0 100644 --- a/src/PlanViewer.App/Controls/QueryStoreGridControl.WaitStats.cs +++ b/src/PlanViewer.App/Controls/QueryStoreGridControl.WaitStats.cs @@ -41,7 +41,8 @@ private async System.Threading.Tasks.Task LoadTimeSlicerDataAsync( catch (OperationCanceledException) { throw; } catch (Exception ex) { - StatusText.Text = $"Slicer: {(ex.Message.Length > 60 ? ex.Message[..60] + "..." : ex.Message)}"; + // #452's mirror: full message out, the status tooltip carries what the strip clips. + StatusText.Text = $"Slicer: {ex.Message}"; } } diff --git a/src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs b/src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs index 41ba627d..fbb393d9 100644 --- a/src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs +++ b/src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs @@ -84,6 +84,19 @@ public QueryStoreGridControl(ServerConnection serverConnection, ICredentialServi ServerFilterExpander.Expanded += ServerFilterExpander_StateChanged; ServerFilterExpander.Collapsed += ServerFilterExpander_StateChanged; + /* #452's pattern, carried to this surface: the status strip is one line, so a long + message — an error, usually — is readable on hover or not at all. QuerySessionControl + funnels every status through SetStatus, which mirrors the text into the tooltip; this + control writes StatusText.Text from a dozen sites across five partials, so the mirror + is one subscription rather than a funnel every future site would have to remember — + and a tooltip set at just the error sites would go stale the moment "Fetching plans..." + was written over the text but not the tip. */ + StatusText.PropertyChanged += (_, e) => + { + if (e.Property == TextBlock.TextProperty) + ToolTip.SetTip(StatusText, string.IsNullOrEmpty(StatusText.Text) ? null : StatusText.Text); + }; + ResultsGrid.ItemsSource = _filteredRows; Helpers.DataGridBehaviors.Attach(ResultsGrid); Helpers.DataGridBehaviors.AttachCopyGuard(ResultsGrid, @@ -159,7 +172,11 @@ private async void QsDatabase_SelectionChanged(object? sender, SelectionChangedE } catch (Exception ex) { - StatusText.Text = ex.Message.Length > 60 ? ex.Message[..60] + "..." : ex.Message; + /* Was cut to 60 characters + "..." before display. Trimming to the space available + is the display layer's job — the strip clips at its own edge — and pre-cutting + here also destroyed the only recovery path: the tooltip the constructor mirrors + onto StatusText now carries the full message on hover. Same family as #452. */ + StatusText.Text = ex.Message; QsDatabaseBox.SelectedItem = _database; // revert return; } diff --git a/src/PlanViewer.App/Dialogs/ConfirmationDialog.cs b/src/PlanViewer.App/Dialogs/ConfirmationDialog.cs index ae6dba60..ded79b5f 100644 --- a/src/PlanViewer.App/Dialogs/ConfirmationDialog.cs +++ b/src/PlanViewer.App/Dialogs/ConfirmationDialog.cs @@ -13,7 +13,10 @@ namespace PlanViewer.App.Dialogs; /// public static class ConfirmationDialog { - public static async Task ShowAsync(Window owner, string title, string message) + /// What the confirming button says. "OK" reads fine for + /// gating an execution; a destructive confirmation ("Replace") should name the act, so + /// the button says what clicking it costs. + public static async Task ShowAsync(Window owner, string title, string message, string confirmCaption = "OK") { var result = false; @@ -28,9 +31,10 @@ public static async Task ShowAsync(Window owner, string title, string mess var okBtn = new Button { - Content = "OK", + Content = confirmCaption, Height = 32, - Width = 80, + // Min rather than fixed: "OK" renders the same, a longer caption ("Replace") isn't clipped. + MinWidth = 80, Padding = new Avalonia.Thickness(16, 0), FontSize = 12, HorizontalContentAlignment = HorizontalAlignment.Center, @@ -42,7 +46,7 @@ public static async Task ShowAsync(Window owner, string title, string mess { Content = "Cancel", Height = 32, - Width = 80, + MinWidth = 80, Padding = new Avalonia.Thickness(16, 0), FontSize = 12, Margin = new Avalonia.Thickness(8, 0, 0, 0), diff --git a/src/PlanViewer.App/Helpers/DetachedWindowHelper.cs b/src/PlanViewer.App/Helpers/DetachedWindowHelper.cs index a613a63b..4ec30e9a 100644 --- a/src/PlanViewer.App/Helpers/DetachedWindowHelper.cs +++ b/src/PlanViewer.App/Helpers/DetachedWindowHelper.cs @@ -87,6 +87,14 @@ public static Window ShowDetached( // Set once the guard has answered, so the re-issued close is not questioned again. bool closeConfirmed = false; + /* Set while a guard is still ASKING, which closeConfirmed cannot cover — it only + latches after a yes. The guard's prompt is modal to this window, but a Save As + picked from that prompt is not, so a second X during the picker used to reach the + guard again and stack a second prompt over the first. Same walk-in-progress latch + MainWindow.OnClosing carries, for the same reentrancy (#485 review's update-link + class). */ + bool closeGuardPending = false; + redockBtn.Click += (_, _) => { if (redocked) return; @@ -104,16 +112,25 @@ public static Window ShowDetached( // latch that stops the second pass asking all over again. async Task ReissueCloseIfConfirmed(Task pending) { - if (!await pending) - return; + try + { + if (!await pending) + return; - closeConfirmed = true; + closeConfirmed = true; - // Posted rather than called: a guard that answers without ever actually waiting - // would otherwise land Close() in the middle of the Closing handler that called - // it. Safe to post because the guard returns null during app shutdown, so nothing - // is ever queued against a dispatcher that is going away. - Dispatcher.UIThread.Post(detachedWindow.Close); + // Posted rather than called: a guard that answers without ever actually waiting + // would otherwise land Close() in the middle of the Closing handler that called + // it. Safe to post because the guard returns null during app shutdown, so nothing + // is ever queued against a dispatcher that is going away. + Dispatcher.UIThread.Post(detachedWindow.Close); + } + finally + { + // Cleared on every ending — refused, confirmed, or thrown — so a kept-open + // window can be asked about again the next time someone tries to close it. + closeGuardPending = false; + } } detachedWindow.Closing += (_, e) => @@ -123,9 +140,18 @@ async Task ReissueCloseIfConfirmed(Task pending) if (!closeConfirmed && closeGuard != null) { + if (closeGuardPending) + { + // A guard is already asking about this window; a second close request + // joins that question rather than raising its own copy of it. + e.Cancel = true; + return; + } + var pending = closeGuard(content, detachedWindow); if (pending != null) { + closeGuardPending = true; e.Cancel = true; _ = ReissueCloseIfConfirmed(pending); return; diff --git a/src/PlanViewer.App/Helpers/TabContentWatcher.cs b/src/PlanViewer.App/Helpers/TabContentWatcher.cs new file mode 100644 index 00000000..bd78cc33 --- /dev/null +++ b/src/PlanViewer.App/Helpers/TabContentWatcher.cs @@ -0,0 +1,76 @@ +using System; +using System.Collections.Generic; +using System.Collections.Specialized; +using System.Linq; +using Avalonia; +using Avalonia.Controls; + +namespace PlanViewer.App.Helpers; + +/// +/// Reports every change to what a is showing, so state derived from its +/// contents can be recomputed without a call planted at each site that changes them (#447). +/// +/// Why a collection subscription is not enough. A tab's content is changed two ways, +/// and only one of them is a collection change. Tabs are added and removed, which +/// Items raises; but a tab is also created holding a progress spinner and later has its +/// Content swapped for the finished article — which is how every plan produced by executing +/// a query arrives, and which the collection says nothing about. #449 watched the collection alone +/// and so fixed the file path while leaving the execution path exactly as broken as it was +/// reported. +/// +/// Both are watched here, which is the point: the next path that produces a plan is correct +/// without its author knowing this exists, because a plan cannot reach the screen without either +/// adding a tab or filling one in. +/// +internal static class TabContentWatcher +{ + /// + /// Invokes whenever a tab is added to or removed from + /// , or an existing tab's content is replaced. + /// + /// Meant to be called once, where the control is built. The subscriptions live as long as + /// the tab control does, which for both call sites is the lifetime of the window. + /// + internal static void Watch(TabControl tabs, Action onChanged) + { + /* Tracked rather than derived from the collection-changed args, because a Reset carries + neither OldItems nor NewItems and would otherwise leave stale subscriptions behind. */ + var watched = new HashSet(); + + void OnTabPropertyChanged(object? sender, AvaloniaPropertyChangedEventArgs e) + { + if (e.Property == ContentControl.ContentProperty) + onChanged(); + } + + void Resync() + { + var current = tabs.Items.OfType().ToHashSet(); + + foreach (var gone in watched.Except(current).ToList()) + { + gone.PropertyChanged -= OnTabPropertyChanged; + watched.Remove(gone); + } + + foreach (var arrived in current.Except(watched).ToList()) + { + arrived.PropertyChanged += OnTabPropertyChanged; + watched.Add(arrived); + } + } + + /* Tabs declared in XAML are already in the collection before anyone gets to watch it. */ + Resync(); + + if (tabs.Items is INotifyCollectionChanged observable) + { + observable.CollectionChanged += (_, _) => + { + Resync(); + onChanged(); + }; + } + } +} diff --git a/src/PlanViewer.App/MainWindow.FileOps.cs b/src/PlanViewer.App/MainWindow.FileOps.cs index 78b5d2bd..4ecb5ce2 100644 --- a/src/PlanViewer.App/MainWindow.FileOps.cs +++ b/src/PlanViewer.App/MainWindow.FileOps.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.IO; using System.Linq; +using System.Text; using System.Text.Json; using System.Threading; using System.Threading.Tasks; @@ -173,8 +174,36 @@ internal bool SaveQueryToPath(TabItem? tab, QuerySessionControl session, string { try { - File.WriteAllText(path, session.QueryEditor.Text); + /* Atomic on purpose: on the save-in-place path this file is the user's only copy + of their query, and a plain truncate-then-write destroys it when the write dies + halfway — disk full, crash, yanked share. AtomicFile stages a sibling .tmp and + renames it over the top, so a failed save leaves the original bytes on disk and + lands in the catch below with the session still dirty. The rename gives the file + the temp's attributes and inherited ACLs rather than preserving the original's — + the trade every editor that saves this way makes. */ + /* In the encoding the file was opened with, when it declared one: a .sql from SSMS + is UTF-16 with a BOM, and writing it back as the default UTF-8 was a silent + transcode of the user's only copy — reading honored the BOM, saving discarded + it. A scratch session (and a BOM-less file) has null here and keeps the default + 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 + swapped), so the persist trigger lives at the assignment. Detached sessions save + through here too (tab == null); their register entry serves up the new path the + same way. */ + RequestSessionPersist(); // Only a write that actually happened settles the dirty state; the catch below // deliberately leaves the session modified so the work is still guarded. session.MarkClean(); @@ -271,6 +300,8 @@ internal void LoadSqlFile(string filePath) var session = new QuerySessionControl(_credentialService, _connectionStore); session.QueryEditor.Text = text; session.SourceFilePath = filePath; + // What the file declared is what a save must write back — see SourceFileEncoding. + session.SourceFileEncoding = DetectBomEncoding(filePath); // What was just loaded is what is on disk — the baseline every later edit is measured against. session.MarkClean(); @@ -285,6 +316,38 @@ internal void LoadSqlFile(string filePath) } } + /// + /// The encoding a file's byte order mark declares, or null when it has none. + /// + /// Deliberately a sniff of the mark rather than StreamReader.CurrentEncoding after a + /// read: CurrentEncoding only moves off its default for UTF-16/32 marks, so it cannot tell + /// a UTF-8-with-BOM file from a plain one — and the default instance it reports EMITS a + /// mark on write, so handing it to the save would stamp BOMs onto files that never had + /// one, a silent change in the opposite direction from the one being fixed. The mark is + /// the fact being preserved, so the mark is what gets read. + /// + private static Encoding? DetectBomEncoding(string filePath) + { + Span mark = stackalloc byte[4]; + int read; + using (var stream = File.OpenRead(filePath)) + read = stream.Read(mark); + + // UTF-32 LE opens with UTF-16 LE's mark plus two zero bytes, so it is checked first. + if (read >= 4 && mark[0] == 0xFF && mark[1] == 0xFE && mark[2] == 0x00 && mark[3] == 0x00) + return new UTF32Encoding(bigEndian: false, byteOrderMark: true); + if (read >= 4 && mark[0] == 0x00 && mark[1] == 0x00 && mark[2] == 0xFE && mark[3] == 0xFF) + return new UTF32Encoding(bigEndian: true, byteOrderMark: true); + if (read >= 3 && mark[0] == 0xEF && mark[1] == 0xBB && mark[2] == 0xBF) + return new UTF8Encoding(encoderShouldEmitUTF8Identifier: true); + if (read >= 2 && mark[0] == 0xFF && mark[1] == 0xFE) + return Encoding.Unicode; + if (read >= 2 && mark[0] == 0xFE && mark[1] == 0xFF) + return Encoding.BigEndianUnicode; + + return null; + } + /// /// One modal for every file operation that can fail, so opening and saving report /// trouble the same way. @@ -414,40 +477,176 @@ private bool ValidatePlanXml(string xml, string label) } /// - /// The file behind every open tab that has one, in tab 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 persisted - /// without writing over the user's real settings file. + /// Separate from so a test can assert what would be + /// persisted without writing over the user's real settings file. + /// + /// #490's detached half: this used to walk MainTabControl alone, so a file-backed + /// plan or query detached into its own window at exit was never written down and never + /// came back. Detached entries are appended AFTER the docked tabs — docked order is the + /// order the user arranged and keeps it; detach order is best-effort. On the next start + /// they all come back as ordinary docked tabs, not re-detached windows: remembering THAT + /// 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 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. + /// 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); } + // ── Continuous session persistence (#490) ───────────────────────────── + + /* #468 wrote the list once, at clean close, and #490 is what that cost: any abnormal exit + — a crash, a task kill, an OS "shut down anyway" past the dirty-tab prompt — restored + zero tabs, because the only writer never ran. Membership changes now write the list as + they happen, debounced so a burst (session restore, Close All) lands as one write. The + triggers are wired centrally, not per call site — see the TabContentWatcher hookup in + the constructor for why. */ + + /// How long the writer waits for a burst of membership changes to settle. + private static readonly TimeSpan SessionPersistDebounce = TimeSpan.FromSeconds(1); + + /// Trailing-edge debounce for ; every request restarts it. + private DispatcherTimer? _sessionPersistTimer; + + /// + /// Whether a membership change is waiting to be written. Under the test host this flag is + /// the whole mechanism — see . + /// + private bool _sessionPersistPending; + + /// + /// Notes that tab membership changed — a tab opened, closed, detached, redocked, or gained + /// a file path — and schedules the debounced write. Called by the tab watcher for + /// everything visible on the strip, and explicitly by the detached register and + /// for the two changes the strip cannot show. + /// + private void RequestSessionPersist() + { + /* OnClosed is already writing the final authoritative list (and force-closing the + detached windows, whose Forget calls land right back here). Nothing may re-arm the + timer against a window being torn down. */ + if (IsShuttingDown) + return; + + _sessionPersistPending = true; + + /* No real timer under the test host, in #451's pattern: the suite shares one + dispatcher across every test, so a timer armed here would tick during some LATER + test's RunJobs and write THIS window's tab list over whatever that test had staged + in the redirected settings file — exactly the cross-test bleed the redirect exists + to stop. Tests drive the flush deterministically through + instead. */ + if (AppRuntimeMode.IsTestHost) + return; + + if (_sessionPersistTimer == null) + { + _sessionPersistTimer = new DispatcherTimer { Interval = SessionPersistDebounce }; + _sessionPersistTimer.Tick += (_, _) => FlushSessionPersist(); + } + + // Stop-then-start restarts the interval, which is what makes it a debounce. + _sessionPersistTimer.Stop(); + _sessionPersistTimer.Start(); + } + + /// + /// Writes a pending membership change down now. The timer's tick, the end of + /// , and the test seam all land here; a flush with nothing + /// pending is free. + /// + private void FlushSessionPersist() + { + _sessionPersistTimer?.Stop(); + + 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; + SaveOpenPlans(); + } + + /// + /// The deterministic stand-in for the debounce timer's tick — tests call this where the + /// real app waits out . A seam rather than a sleep + /// because the harness shares one dispatcher across the whole suite; see + /// for what a real timer does to that arrangement. + /// + internal void FlushPendingSessionPersistForTests() => FlushSessionPersist(); + + /// + /// Writes the open-tab list down for the session that comes back after a Velopack + /// restart. does this on every ordinary shutdown, but + /// ApplyUpdatesAndRestart exits the process without closing the window — and + /// already cleared the saved list at startup, so without + /// this the updated app relaunched with nothing. #490's continuous writer usually has the + /// list current by now anyway, but "usually" is a debounce interval wide; this write is + /// what makes the restart exact. + /// + 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. /// Falls back to a new query tab if nothing was restored. @@ -455,25 +654,89 @@ private void SaveOpenPlans() /// The saved list holds queries as well as plans, so it routes on extension the /// same way an ordinary file open does. Sending a .sql file to LoadPlanFile would greet /// the user with "the XML is not valid" where their query used to be. + /// + /// The crash-loop defense (#490). The list is cleared and saved EMPTY before + /// the first file is opened, so a file that crashes the app during its own load is already + /// off the list when the next start reads it — a poisoned entry can never wedge the app + /// into crashing at every launch. (The clear used to run after the loop, which only + /// defended against crashes AFTER restore finished; a file that crashed DURING it left + /// the list intact and looped forever.) Every file that opens successfully re-enters the + /// list through the debounced writer, so the net behavior is strictly better than the old + /// all-or-nothing: the poison never persists, and everything that actually opened + /// does. + /// + /// Honestly, "everything that opened" holds for a crash after restore, not during + /// it. The re-adds are debounced and this loop runs synchronously on the UI thread, so a + /// crash while file B loads means file A's pending re-add never flushed and A is lost for + /// that start too. Accepted: the invariant being defended is that the poison never + /// persists, not that every good tab survives a mid-restore crash. The flush at the end + /// 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. */ + var savedTabs = _appSettings.OpenTabs.ToList(); + + _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 _appSettings.OpenTabs) + 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; } } - // Clear the restored list now that its tabs are back on screen - _appSettings.OpenTabs.Clear(); - AppSettingsService.Save(_appSettings); + /* Each successful open armed the debounced writer through the tab watcher; write it + 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.PlanViewer.cs b/src/PlanViewer.App/MainWindow.PlanViewer.cs index abf53acb..a39b64e9 100644 --- a/src/PlanViewer.App/MainWindow.PlanViewer.cs +++ b/src/PlanViewer.App/MainWindow.PlanViewer.cs @@ -27,7 +27,13 @@ namespace PlanViewer.App; public partial class MainWindow : Window { - private DockPanel CreatePlanTabContent(PlanViewerControl viewer) + /// + /// Wraps a loaded plan in the toolbar a window-level plan tab shows above it. + /// + /// Internal so a test can reproduce what Get Actual Plan does to a file tab — build this + /// and assign it over a tab that was holding a spinner — without executing anything (#447). + /// + internal DockPanel CreatePlanTabContent(PlanViewerControl viewer) { var humanBtn = new Button { 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 8871dc8a..6201cdec 100644 --- a/src/PlanViewer.App/MainWindow.Tabs.cs +++ b/src/PlanViewer.App/MainWindow.Tabs.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.IO; using System.Linq; +using System.Runtime.CompilerServices; using System.Text.Json; using System.Threading; using System.Threading.Tasks; @@ -37,6 +38,17 @@ private static string GetTabLabel(TabItem tab) return "Tab"; } + /* The glyph refresher CreateTab subscribes onto a query session, keyed by the tab it was + built for, so DetachTabToWindow can take exactly its own subscription off again. The + session OUTLIVES its tab on the detach path — the tab is discarded, the session moves + into the detached window still holding a handler that closes over the dead tab's close + button — and redock re-subscribes through CreateTab, so every detach/redock cycle used + to pin one more dead TabItem (visual tree and all) to the session for the session's + lifetime. A ConditionalWeakTable rather than a Dictionary so the bookkeeping cannot + become its own leak: a tab closed normally dies together with its session, and its + entry evaporates with the key. */ + private readonly ConditionalWeakTable _tabDirtyGlyphUnhooks = new(); + private TabItem CreateTab(string label, Control content) { var headerText = new TextBlock @@ -83,13 +95,26 @@ the dot back into a close button. */ void RefreshCloseGlyph() => closeBtn.Content = CloseButtonGlyph(querySession.IsDirty, closeBtn.IsPointerOver); - querySession.DirtyStateChanged += (_, _) => RefreshCloseGlyph(); + /* Named and written down rather than subscribed inline: of everything wired up + here, this is the one subscription that lands on an object that can outlive the + tab, so it is the one detach has to be able to undo — see _tabDirtyGlyphUnhooks. + The pointer handlers below live and die with the button itself. */ + EventHandler refreshOnDirtyChange = (_, _) => RefreshCloseGlyph(); + querySession.DirtyStateChanged += refreshOnDirtyChange; + _tabDirtyGlyphUnhooks.Add(tab, () => querySession.DirtyStateChanged -= refreshOnDirtyChange); + closeBtn.PointerEntered += (_, _) => RefreshCloseGlyph(); closeBtn.PointerExited += (_, _) => RefreshCloseGlyph(); // 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 @@ -205,10 +230,17 @@ private static void SetTabLabel(TabItem tab, string label) /// session, and whether Copy Path appears on the tab's context menu. A shape it does /// not know about loses both without saying anything. /// - private static string? GetTabFilePath(TabItem tab) + private static string? GetTabFilePath(TabItem tab) => GetContentFilePath(tab.Content as Control); + + /// + /// The same answer keyed on the content itself, because since #490 the question is also + /// asked about content with no tab behind it: a detached window holds the control that WAS + /// a tab's content, and what file backs it is a fact about the control, not the strip. + /// + private static string? GetContentFilePath(Control? content) { // Plans opened from file are wrapped in a DockPanel with the viewer as the last child - if (tab.Content is DockPanel dp) + if (content is DockPanel dp) { foreach (var child in dp.Children) { @@ -218,12 +250,37 @@ private static void SetTabLabel(TabItem tab, string label) } // Queries are the session control itself, with no wrapper around it - if (tab.Content is QuerySessionControl session) + if (content is QuerySessionControl session) return session.SourceFilePath; 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 @@ -303,6 +360,16 @@ private static string GetQueryTextFromPlan(PlanViewerControl viewer) var label = GetTabLabel(tab); + /* The session is about to leave this tab behind. Take the glyph subscription off it + first: the handler is the one reference that would keep the discarded TabItem alive + from the still-living session (see _tabDirtyGlyphUnhooks), and redock subscribes + afresh through CreateTab. */ + if (_tabDirtyGlyphUnhooks.TryGetValue(tab, out var unhookDirtyGlyph)) + { + unhookDirtyGlyph(); + _tabDirtyGlyphUnhooks.Remove(tab); + } + // Remove the tab MainTabControl.Items.Remove(tab); tab.Content = null; @@ -318,7 +385,7 @@ private static string GetQueryTextFromPlan(PlanViewerControl viewer) backgroundBrush: (Avalonia.Media.IBrush?)this.FindResource("BackgroundBrush"), onRedock: c => { - ForgetDetachedQuerySession(c); + ForgetDetachedTabContent(c); if (!IsShuttingDown) { @@ -330,14 +397,42 @@ private static string GetQueryTextFromPlan(PlanViewerControl viewer) }, onClosing: c => { - ForgetDetachedQuerySession(c); + 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(); }, closeGuard: DetachedQueryCloseGuard); - RememberDetachedQuerySession(detachedWindow, content); + RememberDetachedTabContent(detachedWindow, content); + + /* #447 made Compare a window-wide count, and this session just left the window: the + count it is showing is about tabs it can no longer reach. Nothing else recomputes it + here — the session's sub-tab watcher fires on sub-tab changes only, and detaching + changes none — so the pre-detach state stuck until the next plan landed. Recompute + now; with no MainWindow above it any more, the method's own fallback counts the + session's plans, which inside a detached window is the only honest answer. Redock + needs no twin call: adding the tab back fires MainWindow's collection watcher. */ + if (content is QuerySessionControl detachedSession) + detachedSession.UpdateCompareButtonState(); + return detachedWindow; } } diff --git a/src/PlanViewer.App/MainWindow.axaml.cs b/src/PlanViewer.App/MainWindow.axaml.cs index 466574c1..e6e0c895 100644 --- a/src/PlanViewer.App/MainWindow.axaml.cs +++ b/src/PlanViewer.App/MainWindow.axaml.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.Collections.Specialized; using System.IO; using System.IO.Pipes; using System.Linq; @@ -19,6 +18,7 @@ using Avalonia.Threading; using PlanViewer.App.Controls; using PlanViewer.App.Dialogs; +using PlanViewer.App.Helpers; using PlanViewer.App.Services; using PlanViewer.Core.Interfaces; using PlanViewer.Core.Models; @@ -30,8 +30,6 @@ namespace PlanViewer.App; public partial class MainWindow : Window { - private const string PipeName = "SQLPerformanceStudio_OpenFile"; - private readonly ICredentialService _credentialService; private readonly ConnectionStore _connectionStore; private readonly CancellationTokenSource _pipeCts = new(); @@ -47,6 +45,16 @@ public partial class MainWindow : Window /// internal bool IsShuttingDown { get; private set; } + /// + /// Whether this window's process-external startup services actually launched. Set by the + /// launch methods themselves rather than by the gate, so the + /// test pinning that they stay off under the harness (#451) still fails if a future change + /// starts one through a new path. Never read by the app. + /// + internal bool PipeServerStarted { get; private set; } + internal bool StartupUpdateCheckStarted { get; private set; } + internal bool McpServerStartAttempted { get; private set; } + public MainWindow() { _credentialService = CredentialServiceFactory.Create(); @@ -57,13 +65,22 @@ public MainWindow() if (Enum.TryParse(_appSettings.QueryStoreDefaultTimeDisplay, true, out var tdm)) TimeDisplayHelper.Current = tdm; - // Listen for file paths from other instances (e.g. SSMS extension) - StartPipeServer(); + // Listen for file paths from other launches (SSMS extension, a second Studio + // launch handing over its file) and for the #489 surface-yourself sentinel a bare + // second launch sends instead of running a full instance. + // Not in the test host (#451): every test window would grab the machine's single + // SQLPerformanceStudio_OpenFile pipe slot and never release it — OnClosed never + // runs there — racing any real Studio instance on the same box. + if (!AppRuntimeMode.IsTestHost) + StartPipeServer(); InitializeComponent(); - // Check for updates on startup (non-blocking) - _ = CheckForUpdatesOnStartupAsync(); + // Check for updates on startup (non-blocking). + // Not in the test host (#451): one GitHub hit per constructed window meant ~36 + // update checks per local test run. + if (!AppRuntimeMode.IsTestHost) + _ = CheckForUpdatesOnStartupAsync(); // Build the Recent Plans submenu from saved state RebuildRecentPlansMenu(); @@ -79,9 +96,26 @@ public MainWindow() remove a tab. Compare Plans depends on how many plans exist across the WHOLE window, so opening or closing any tab can change whether it is available in every OTHER tab, and a refresh that has to be remembered at sixteen call sites is one that gets forgotten at the - seventeenth. */ - if (MainTabControl.Items is INotifyCollectionChanged tabs) - tabs.CollectionChanged += (_, _) => RefreshComparePlanAvailability(); + seventeenth. + + Watching content replacement as well as the collection is the part the first attempt at + this got wrong: Get Actual Plan opens a tab holding a spinner and later swaps in the plan, + so the tab that gains a plan is one this window already had. + + #490 hangs session persistence on the same watcher, for exactly the reason above: the + open-tab list used to be written only at clean close (#468's original scope), so any + abnormal exit — crash, task kill, an OS "shut down anyway" past the dirty-tab prompt — + restored zero tabs. Requesting a persist at each place that opens or closes a tab is + the same sixteen-call-sites trap #447 named; the watcher is the one place that sees + them all. The two changes it cannot see get explicit calls instead: detached windows + (off the tab strip — see RememberDetachedTabContent / ForgetDetachedTabContent) and a + tab whose PATH changes while its membership does not (SaveQueryToPath, where a scratch + gains a file). */ + TabContentWatcher.Watch(MainTabControl, () => + { + RefreshComparePlanAvailability(); + RequestSessionPersist(); + }); // Global hotkeys via tunnel routing so they fire before AvaloniaEdit consumes them AddHandler(KeyDownEvent, (_, e) => @@ -144,23 +178,63 @@ refresh that has to be remembered at sixteen call sites is one that gets forgott }, RoutingStrategies.Tunnel); // Accept command-line argument or restore previously open plans - var args = Environment.GetCommandLineArgs(); - if (args.Length > 1 && File.Exists(args[1])) - { - LoadPlanFile(args[1]); - } - else - { - // Restore plans that were open in the previous session - RestoreOpenPlans(); - } + OpenFromStartupArgs(Environment.GetCommandLineArgs()); + + // Start MCP server if enabled in settings. + // Not in the test host (#451): this reads the user's real ~/.planview settings and, + // when enabled there, binds a real TCP port from inside the test runner. The menu + // item needs no fallback write — its XAML default is already "MCP Server: Off". + if (!AppRuntimeMode.IsTestHost) + StartMcpServer(); + } - // Start MCP server if enabled in settings - StartMcpServer(); + /// + /// Routes the file handed over on the command line — a double-clicked file association, or + /// "PerformanceStudio.exe path" from a shell. Split from the constructor so the routing can + /// be tested with an argv of the test's choosing. + /// + /// Routed by extension, not straight to LoadPlanFile: every other way a path reaches + /// this window (the pipe from a second instance, drag-and-drop, session restore) already + /// goes through , and RestoreOpenPlans' own doc comment + /// describes exactly what skipping it does — "Sending a .sql file to LoadPlanFile would + /// greet the user with 'the XML is not valid' where their query used to be." That greeting + /// is precisely what "PerformanceStudio.exe query.sql" produced. Cold start was the one + /// path left hard-wired to the plan loader. + /// + internal void OpenFromStartupArgs(string[] args) + { + /* #489: --new-instance is a launcher directive, not a file, and it is scrubbed + HERE as well as in Program.Main because this method consumes the raw + Environment.GetCommandLineArgs() — Program's scrubbed copy never reaches it. + Without this, "PerformanceStudio.exe --new-instance file.sqlplan" would find the + flag at args[1]: it happens to fail the File.Exists guard below, but the user's + file behind it would still be skipped and the launch would silently + session-restore instead. */ + args = SingleInstance.StripNewInstanceFlag(args); + + /* 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]); } private void StartPipeServer() { + PipeServerStarted = true; + var token = _pipeCts.Token; Task.Run(async () => { @@ -169,21 +243,22 @@ private void StartPipeServer() try { using var server = new NamedPipeServerStream( - PipeName, PipeDirection.In, 1, + SingleInstance.PipeName, PipeDirection.In, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); await server.WaitForConnectionAsync(token); using var reader = new StreamReader(server); - var filePath = await reader.ReadLineAsync(); + var line = await reader.ReadLineAsync(); - if (!string.IsNullOrWhiteSpace(filePath) && File.Exists(filePath)) + /* Classified out here rather than inside the UI dispatch on purpose: + Classify calls File.Exists, and a dead UNC path can block for + seconds — that wait belongs on this pipe task, not the dispatcher. */ + var kind = SingleInstance.Classify(line); + if (kind != SingleInstance.PipeMessage.Ignore) { await Dispatcher.UIThread.InvokeAsync(() => - { - OpenFileByExtension(filePath); - Activate(); - }); + DispatchPipeMessage(kind, line!)); } } catch (OperationCanceledException) @@ -202,8 +277,55 @@ await Dispatcher.UIThread.InvokeAsync(() => }, token); } + /// + /// Acts on one classified pipe line, on the UI thread. Split from the pipe loop so the + /// dispatch can be pinned by tests without a pipe (#489). + /// + /// The classification is the compatibility contract with every sender version: + /// an existing file path opens — what the SSMS extension and any Studio build have + /// always sent, unchanged — the activation sentinel surfaces the window (what a bare + /// second launch sends since #489), and anything else, including a path that stopped + /// existing between send and receive, was already dropped silently before the + /// classifier existed and still is. + /// + internal void DispatchPipeMessage(SingleInstance.PipeMessage kind, string line) + { + switch (kind) + { + case SingleInstance.PipeMessage.OpenFile: + OpenFileByExtension(line); + SurfaceWindow(); + break; + + case SingleInstance.PipeMessage.Activate: + SurfaceWindow(); + break; + } + } + + /// + /// Brings the main window back to the user after a second launch handed its work to + /// this one (#489): restore from minimized, then activate. Deliberately minimal next + /// to Lite's surface path — Studio never hides to a tray, so there is nothing to + /// re-show. + /// + /// The file-open message uses it too, where the old handler only called + /// Activate(): a plan sent from SSMS to a minimized window used to load into a window + /// that stayed in the taskbar. + /// + internal void SurfaceWindow() + { + if (WindowState == WindowState.Minimized) + WindowState = WindowState.Normal; + Activate(); + } + private void StartMcpServer() { + // Set before the settings read: reading the user's real ~/.planview file is itself + // part of what the test host must not do, so "attempted" starts here. + McpServerStartAttempted = true; + var settings = McpSettings.Load(); if (!settings.Enabled) { @@ -225,7 +347,27 @@ protected override async void OnClosed(EventArgs e) { IsShuttingDown = true; - // Save the list of currently open file-based plans for session restore + /* The debounced writer (#490) is finished: stop its timer so a pending tick cannot + fire into a window being torn down, and let the write below be the last word. + It stays the authoritative final write — mostly redundant now that membership + changes persist continuously, but it covers anything the debounce had not flushed + yet, and it runs while the detached windows below are still open and registered, + so their paths are in it. */ + _sessionPersistTimer?.Stop(); + _sessionPersistPending = false; + + /* #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(); @@ -267,6 +409,15 @@ private void UpdateEmptyOverlay() /// private bool _closeConfirmed; + /* _closeConfirmed only latches AFTER the walk answers yes, so it does nothing about a + second close request arriving WHILE the walk is still asking — a double-clicked X, or + another Close() between two of the walk's dialogs (each prompt is modal, but the window + is briefly its own again between them, and for the whole of a Save As picker). Each + such request used to start a second concurrent walk: duplicate prompts about the same + tabs, and a Cancel to one walk that the other never heard about. Same reentrancy class, + and same one-latch answer, as the About window's update link (#485 review). */ + private bool _closeWalkInProgress; + private const string CloseGlyph = "\u2715"; // ✕ private const string ModifiedGlyph = "\u25CF"; // ● @@ -315,16 +466,56 @@ internal List TabsWithUnsavedChanges() => internal IReadOnlyList<(Window Window, QuerySessionControl Session)> DetachedQuerySessions => _detachedQuerySessions; - internal void RememberDetachedQuerySession(Window window, Control content) + /// + /// EVERY top-level tab content currently detached, in detach order — plans and Query Store + /// windows included, not just the query sessions above. + /// + /// #490's second register, kept next to #473's because they answer different + /// questions. The prompt register above only cares about content that can lose an edit; + /// session persistence cares about content that came from a FILE, and a detached plan is + /// exactly that — file-backed, read-only, and invisible to a save list built by walking + /// MainTabControl. Sub-tab detaches (a plan torn out of a query session's sub-tab strip) + /// stay off both registers on purpose: their session's own tab is still docked and already + /// carries the path. + /// + private readonly List _detachedTabContents = new(); + + /// + /// Books a detached top-level tab's content into both registers. Named for the content + /// rather than the session since #490: persistence needs every detached tab remembered, + /// and only the #473 prompt half is query-session-only. + /// + internal void RememberDetachedTabContent(Window window, Control content) { + _detachedTabContents.Add(content); + if (content is QuerySessionControl session) _detachedQuerySessions.Add((window, session)); + + /* 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 + a synchronous write at the watcher's moment would have landed in the gap between + Items.Remove and this call and dropped the detached file. */ + RequestSessionPersist(); } - internal void ForgetDetachedQuerySession(Control content) + /// + /// Takes a detached tab's content back off both registers — on redock, and on the detached + /// window actually closing. + /// + internal void ForgetDetachedTabContent(Control content) { + _detachedTabContents.Remove(content); + if (content is QuerySessionControl session) _detachedQuerySessions.RemoveAll(d => d.Session == session); + + /* Redock re-adds a tab and the watcher sees that; a detached window closed by its own + X changes no tab at all, and this is the only place that knows the file left the + app. One debounced write covers both. */ + RequestSessionPersist(); } /// @@ -368,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); } /// @@ -454,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); + } } /// @@ -473,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; } @@ -519,8 +750,20 @@ protected override void OnClosing(WindowClosingEventArgs e) { if (!_closeConfirmed && CloseNeedsConfirmation()) { + /* The cancel is unconditional — a close arriving mid-walk must not fall through + to base and succeed while the walk is still asking — but only the FIRST one may + start a walk. The latch check lives inside this branch on purpose: the walk's + own Close() at the end re-enters here with _closeWalkInProgress still true, and + it has to sail through on _closeConfirmed above, not be swallowed as a + duplicate. */ e.Cancel = true; - _ = ConfirmWindowCloseAsync(); + + if (!_closeWalkInProgress) + { + _closeWalkInProgress = true; + _ = ConfirmWindowCloseAsync(); + } + return; } @@ -529,12 +772,42 @@ protected override void OnClosing(WindowClosingEventArgs e) private async Task ConfirmWindowCloseAsync() { - /* #473: the sessions that are no longer tabs are asked about here, while every window - is still up, rather than from OnClosed where they are force-closed. By then the main - window is gone and the app is on its way out — a dialog raised there is at best a - window nobody expects and at worst a shutdown that never finishes. Asking here is - what earns DetachedContentNeedsSavePrompt the right to wave that force-close - through. */ + try + { + if (!await ConfirmAllUnsavedWorkAsync()) + return; // one Cancel cancels the shutdown + + _closeConfirmed = true; + Close(); + } + finally + { + /* Cleared however the walk ends — confirmed, cancelled, or a prompt that threw — + so a cancelled shutdown leaves the X able to start a fresh walk. */ + _closeWalkInProgress = false; + } + } + + /// + /// Asks about every piece of unsaved work in the app, and answers whether whatever is + /// about to destroy that work may proceed. + /// + /// Split out of because the window close is + /// not the only way the process ends: the About window's Velopack "Restart Now" calls + /// ApplyUpdatesAndRestart, which exits without ever raising Closing — so #462's and + /// #473's walk never ran on that route and dirty edits were discarded without a word. + /// This is only the walk: it does not latch and does not + /// Close, so the restart path can take the answer without the close's bookkeeping. + /// + /// #473: the sessions that are no longer tabs are asked about here, while every + /// window is still up, rather than from OnClosed where they are force-closed. By then the + /// main window is gone and the app is on its way out — a dialog raised there is at best a + /// window nobody expects and at worst a shutdown that never finishes. Asking here is what + /// earns DetachedContentNeedsSavePrompt the right to wave that force-close through. + /// + /// False the moment anyone answers Cancel, or a save they asked for fails. + internal async Task ConfirmAllUnsavedWorkAsync() + { foreach (var work in UnsavedWorkOnClose()) { var mayClose = work.Tab != null @@ -542,11 +815,10 @@ what earns DetachedContentNeedsSavePrompt the right to wave that force-close : await ConfirmDetachedCloseAsync(work.Session, work.Owner); if (!mayClose) - return; // one Cancel cancels the shutdown + return false; } - _closeConfirmed = true; - Close(); + return true; } @@ -686,6 +958,9 @@ private void ShowError(string message) private async Task CheckForUpdatesOnStartupAsync() { + // Before the first await, so the flag is visible to a caller synchronously. + StartupUpdateCheckStarted = true; + try { await Task.Delay(5000); // Don't slow down startup diff --git a/src/PlanViewer.App/Mcp/McpPlanTools.cs b/src/PlanViewer.App/Mcp/McpPlanTools.cs index da6f8a5a..6dae20ee 100644 --- a/src/PlanViewer.App/Mcp/McpPlanTools.cs +++ b/src/PlanViewer.App/Mcp/McpPlanTools.cs @@ -308,7 +308,16 @@ public static string GetReproScript( if (statement is null) return "No executable statement found in this plan."; - var queryText = session.QueryText ?? statement.StatementText ?? ""; + /* #482: the parameterized form on purpose. ReproScriptBuilder wraps this body in + sp_executesql with a parameter list read out of the same plan, so a body that already + has the literals inlined would declare parameters that appear nowhere in it — and the + plan it produced would be the constant-folded one, not the parameterized compile the + repro script exists to reproduce. Falls through to StatementText, which is the same + string whenever nothing was substituted. */ + var queryText = session.QueryText + ?? statement.ParameterizedStatementText + ?? statement.StatementText + ?? ""; var databaseName = session.DatabaseName ?? statement.OperatorTree?.DatabaseName; return ReproScriptBuilder.BuildReproScript( diff --git a/src/PlanViewer.App/Mcp/McpQueryStoreTools.cs b/src/PlanViewer.App/Mcp/McpQueryStoreTools.cs index c4f0c54b..636f8a79 100644 --- a/src/PlanViewer.App/Mcp/McpQueryStoreTools.cs +++ b/src/PlanViewer.App/Mcp/McpQueryStoreTools.cs @@ -311,8 +311,16 @@ internal static PlanSession CaptureSession( string connectionInfo) { var analysis = ResultMapper.Map(parsed, "query-store"); - var allStatements = parsed.Batches.SelectMany(batch => batch.Statements).ToList(); - var executableStatement = allStatements.FirstOrDefault(statement => statement.RootNode is not null); + + /* #456 follow-up: the counts used to come from a second walk over batch.Statements while + the Analysis stored on this very session is mapped from PlanStatements.EnumerateAll — + stored procedure and UDF bodies included, node-level warnings counted — so a Query + Store EXEC plan registered statement_count 1 and warning_count 0 alongside an analysis + full of findings. The counts now come from that analysis's own summary: one source, and + nothing left to disagree with what an MCP client reads. */ + var executableStatement = Core.Services.PlanStatements.EnumerateAll(parsed) + .FirstOrDefault(statement => statement.RootNode is not null); + var session = new PlanSession { SessionId = sessionId, @@ -324,12 +332,11 @@ internal static PlanSession CaptureSession( DatabaseName = executableStatement?.RootNode?.DatabaseName, QueryText = queryText, ConnectionInfo = connectionInfo, - StatementCount = allStatements.Count, - HasActualStats = false, - WarningCount = allStatements.Sum(statement => statement.PlanWarnings.Count), - CriticalWarningCount = allStatements.Sum(statement => - statement.PlanWarnings.Count(warning => warning.Severity == Core.Models.PlanWarningSeverity.Critical)), - MissingIndexCount = parsed.AllMissingIndexes.Count + StatementCount = analysis.Summary.TotalStatements, + HasActualStats = analysis.Summary.HasActualStats, + WarningCount = analysis.Summary.TotalWarnings, + CriticalWarningCount = analysis.Summary.CriticalWarnings, + MissingIndexCount = analysis.Summary.MissingIndexes }; parsed.RawXml = string.Empty; diff --git a/src/PlanViewer.App/Program.cs b/src/PlanViewer.App/Program.cs index 3d750fc8..82bb239d 100644 --- a/src/PlanViewer.App/Program.cs +++ b/src/PlanViewer.App/Program.cs @@ -2,6 +2,7 @@ using System; using System.IO; using System.IO.Pipes; +using System.Threading; using System.Threading.Tasks; using PlanViewer.App.Services; using Velopack; @@ -10,7 +11,14 @@ namespace PlanViewer.App; class Program { - private const string PipeName = "SQLPerformanceStudio_OpenFile"; + /// + /// Held — never released — by the instance that owns the single-instance slot (#489). + /// The OS tears the mutex down when the process exits, crash included, so there is no + /// release path to get wrong; a static field keeps the handle rooted for the whole run + /// (the previous mutex attempt died precisely because its handle was disposed the + /// moment the acquiring method returned, so no instance ever actually held it). + /// + private static Mutex? _singleInstanceMutex; [STAThread] public static void Main(string[] args) @@ -36,12 +44,72 @@ public static void Main(string[] args) } velopack.Run(); - // If another instance is running, send the file path to it and exit - if (args.Length > 0 && TrySendToRunningInstance(args[0])) - return; + /* #489: every instance holds a whole-file AppSettings snapshot and Save writes the + whole file, so a second instance makes the settings file last-write-wins — the + instance that exits second silently clobbers the other's open_tabs and every + other setting (AtomicFile prevents torn writes, not lost updates). File-argument + launches already forwarded to the running instance over the pipe; a BARE second + launch ran a full instance and was exactly the clobber case. So unless the user + explicitly asks for a second instance, a launch that finds one running hands it + its work — a file path, or a bare "surface yourself" — and exits. */ + var newInstanceRequested = SingleInstance.NewInstanceRequested(args); + + // The flag is a launcher directive, not a file: strip it so nothing downstream can + // mistake it for a path ("PerformanceStudio.exe --new-instance file.sqlplan" must + // still open the file). MainWindow re-reads the raw argv and scrubs it again itself. + var effectiveArgs = SingleInstance.StripNewInstanceFlag(args); + + if (!newInstanceRequested) + { + /* The pre-#489 forwarding, kept first and unchanged: a with-file launch tries + the pipe before anything else. Beyond being the common case, probing before + the mutex makes the WITH-FILE path version-skew-proof — an already-running + build that predates the mutex answers its pipe but holds no mutex, and a + mutex-first flow would run a second full window beside it instead of handing + the file over. + + A BARE launch during that same skew is the one #489 case deliberately left + open: it cannot probe first, because an old receiver silently drops the + sentinel (File.Exists guard) while delivery still reports success — the + launch would exit having surfaced nothing, which is worse than a second + instance. So a bare launch beside a pre-mutex build claims the free mutex + and runs fully: the pre-#489 status quo, for one transient upgrade window + that ends when the old instance exits. Documented rather than solved; a + real fix needs an acknowledged (duplex) surfacing protocol. */ + if (effectiveArgs.Length > 0 && TrySendToRunningInstance(effectiveArgs[0], maxAttempts: 1)) + return; + + if (!TryBecomeSingleInstanceOwner()) + { + /* Another instance owns the slot but hasn't answered its pipe yet — bare + launches never probed above, and a with-file probe may have raced the + owner's boot (the pipe server starts in the MainWindow constructor, + which on a cold start is seconds after its Main). Retry for ~2s before + giving up on delivery. */ + var message = effectiveArgs.Length > 0 + ? effectiveArgs[0] + : SingleInstance.ActivateSentinel; + if (TrySendToRunningInstance(message, maxAttempts: 4)) + return; + + /* Delivery failed after retries: the owner is wedged, exiting, or still + booting slowly. Losing the user's action — their double-clicked file, or + the app simply appearing at all — is worse than a rare second instance, + so fall through and run fully. This is also the honest residue of the + startup race: two simultaneous bare launches can BOTH end up proceeding + when the loser's retries run out before the winner's pipe exists. The + mutex closes most of that window; what remains falls back to the + pre-#489 last-write-wins behavior, which is the accepted floor. */ + } + } BuildAvaloniaApp() - .StartWithClassicDesktopLifetime(args); + .StartWithClassicDesktopLifetime(effectiveArgs); + + // Reached only at app shutdown. Statics are GC roots, so the field alone keeps the + // owner's mutex handle alive; this read exists to say out loud that the handle's + // LIFETIME is the point (and to keep the field from reading as write-only). + GC.KeepAlive(_singleInstanceMutex); } // Avalonia configuration, don't remove; also used by visual designer. @@ -52,32 +120,87 @@ public static AppBuilder BuildAvaloniaApp() .LogToTrace(); /// - /// Tries to hand the file path to an already-running instance over its named pipe. - /// A failed/timed-out connect means no instance is listening, so the caller should - /// launch normally. Returns true only if the path was actually delivered. + /// Tries to claim the single-instance slot (#489). True means this process is the + /// owner and should run; false means another instance holds the slot and this launch + /// should hand its work over instead. /// - /// - /// Detection is via the pipe itself rather than a named mutex: the previous mutex - /// was disposed as soon as this method returned, so no instance ever held it and - /// the forwarding path was never taken. - /// - private static bool TrySendToRunningInstance(string filePath) + private static bool TryBecomeSingleInstanceOwner() { try { - using var client = new NamedPipeClientStream(".", PipeName, PipeDirection.Out); - // Short timeout: a running instance's listener is idle and connects - // immediately; when none is running this is the only added launch delay. - client.Connect(500); - using var writer = new StreamWriter(client); - writer.WriteLine(filePath); - writer.Flush(); + var mutex = new Mutex(initiallyOwned: true, SingleInstance.MutexName, out var createdNew); + if (createdNew) + { + _singleInstanceMutex = mutex; + return true; + } + + /* Another process owns it. Close our handle right away: if this launch ends up + running anyway (the pipe fallback above), a lingering handle would keep the + kernel object alive after the real owner exits, and a THIRD launch would then + see the name taken with nobody serving the pipe behind it. */ + mutex.Dispose(); + return false; + } + catch (Exception ex) + { + /* Mutex machinery unavailable — an ACL mismatch on the name, a restrictive + sandbox, or a platform where the named-mutex shim misbehaves (on Unix these + are file-backed under /tmp; PlatformNotSupportedException would land here + too). Claiming ownership is the conservative answer: this instance runs + fully, which is exactly the pre-#489 behavior for every launch. + + Said out loud rather than swallowed, because the degradation is otherwise + invisible: if this fires on every launch, single-instancing has quietly + no-oped and #489's settings clobber is back with no symptom pointing here. + stderr is the right channel — a Windows GUI launch has no console and loses + it harmlessly, while the Unix platforms this is most likely to fire on are + exactly where launching from a terminal is common. A unit test exercises + the named-mutex machinery per platform in CI so a shim that throws fails + loudly there first. */ + Console.Error.WriteLine( + $"PerformanceStudio: single-instance detection unavailable ({ex.GetType().Name}); running as a full instance."); return true; } - catch + } + + /// + /// Tries to hand one line — a file path, or the activation sentinel — to an + /// already-running instance over its named pipe. Returns true only if the line was + /// actually delivered; the caller decides what a failed delivery costs. + /// + private static bool TrySendToRunningInstance(string message, int maxAttempts) + { + for (var attempt = 1; attempt <= maxAttempts; attempt++) { - // No instance listening (or pipe busy) — fall through to launch normally. - return false; + if (attempt > 1) + { + // Between attempts only: the owner is presumably mid-boot, so give its + // pipe server a beat to come up rather than burning connects back-to-back. + Thread.Sleep(100); + } + + try + { + using var client = new NamedPipeClientStream(".", SingleInstance.PipeName, PipeDirection.Out); + // 500ms per attempt: a running instance's listener is idle and connects + // immediately, while Connect burns the full timeout when nothing is + // listening — so the single-attempt probe on a with-file launch adds at + // most the same half second it always has, and the 4-attempt retry path + // totals roughly the ~2s boot grace it exists for. + client.Connect(500); + using var writer = new StreamWriter(client); + writer.WriteLine(message); + writer.Flush(); + return true; + } + catch + { + // Not listening yet, or the single server slot was mid-conversation with + // another client — retry if the budget allows, otherwise report undelivered. + } } + + return false; } } diff --git a/src/PlanViewer.App/Services/AppSettingsService.cs b/src/PlanViewer.App/Services/AppSettingsService.cs index 56dcd678..c3180b5d 100644 --- a/src/PlanViewer.App/Services/AppSettingsService.cs +++ b/src/PlanViewer.App/Services/AppSettingsService.cs @@ -14,9 +14,13 @@ namespace PlanViewer.App.Services; internal sealed class AppSettingsService { private const int MaxRecentPlans = 10; - private static readonly string SettingsDir; - private static readonly string SettingsPath; - private static readonly string OldFormatSettingsPath; + + // Not readonly only because RedirectStorageForTestHost exists; nothing in the + // product assigns these outside the static constructor. + private static string SettingsDir; + private static string SettingsPath; + private static string OldFormatSettingsPath; + private static string ScratchDir; private static AppSettings? _cached; @@ -27,6 +31,46 @@ static AppSettingsService() "PerformanceStudio"); SettingsPath = Path.Combine(SettingsDir, "appsettings.json"); OldFormatSettingsPath = Path.Combine(SettingsDir, "perfstudio_format_settings.json"); + ScratchDir = Path.Combine(SettingsDir, "scratch"); + } + + /// + /// The file settings are read from and written to right now — the real per-user path + /// unless moved it. Exposed so the test suite + /// can pin that the redirection actually happened (#451) instead of trusting it. + /// + 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 + /// MainWindow-driving tests run the real settings code — RestoreOpenPlans clears and + /// saves the open-tab list, LoadPlanFile saves Recent Plans — and were doing all of it + /// against the developer's actual appsettings.json: fixture paths evicted real recent + /// entries and the saved session-restore list was destroyed, confirmed live. When this + /// is never called, the paths keep their static-constructor defaults byte for byte, so + /// the real app is untouched. + /// + 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. + _cached = null; } private static readonly JsonSerializerOptions JsonOptions = new() diff --git a/src/PlanViewer.App/Services/AtomicFile.cs b/src/PlanViewer.App/Services/AtomicFile.cs index c2c24474..581b0bda 100644 --- a/src/PlanViewer.App/Services/AtomicFile.cs +++ b/src/PlanViewer.App/Services/AtomicFile.cs @@ -1,4 +1,5 @@ using System.IO; +using System.Text; namespace PlanViewer.App.Services; @@ -15,10 +16,20 @@ internal static class AtomicFile /// keeps its previous contents and a stray /// .tmp sibling is left behind (cleaned up on the next call). /// - public static void WriteAllText(string path, string contents) + /// + /// Bytes to write the text as; null keeps File.WriteAllText's default, UTF-8 + /// without a BOM. Callers saving over a file the user opened pass the encoding + /// that file arrived with, so a save-in-place is not also a silent transcode. + /// + public static void WriteAllText(string path, string contents, Encoding? encoding = null) { var tmp = path + ".tmp"; - File.WriteAllText(tmp, contents); + + if (encoding != null) + File.WriteAllText(tmp, contents, encoding); + else + File.WriteAllText(tmp, contents); + // File.Move with overwrite:true maps to MoveFileEx(MOVEFILE_REPLACE_EXISTING) // on Windows and rename(2) on Unix — both atomic when source and destination // live on the same filesystem, which is always the case here. 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/src/PlanViewer.App/Services/TextBoxClipboardGuard.cs b/src/PlanViewer.App/Services/TextBoxClipboardGuard.cs index bc102d0a..55cd6fce 100644 --- a/src/PlanViewer.App/Services/TextBoxClipboardGuard.cs +++ b/src/PlanViewer.App/Services/TextBoxClipboardGuard.cs @@ -14,8 +14,19 @@ namespace PlanViewer.App.Services; /// internal static class TextBoxClipboardGuard { + private static bool _registered; + public static void Register() { + // Once per process. The real app calls this once at startup anyway, but the test + // harness (#451) boots a fresh App per test dispatch, and class handlers live on the + // static routed events — not the Application — so each boot was stacking another set + // of handlers onto every TextBox in the test host. The guard stays registered for + // any test that exercises clipboard behavior; it just stops accumulating. + if (_registered) + return; + _registered = true; + TextBox.CopyingToClipboardEvent.AddClassHandler(OnCopying); TextBox.CuttingToClipboardEvent.AddClassHandler(OnCutting); TextBox.PastingFromClipboardEvent.AddClassHandler(OnPasting); diff --git a/src/PlanViewer.App/SingleInstance.cs b/src/PlanViewer.App/SingleInstance.cs new file mode 100644 index 00000000..625fe7a6 --- /dev/null +++ b/src/PlanViewer.App/SingleInstance.cs @@ -0,0 +1,124 @@ +using System; +using System.IO; +using System.Linq; + +namespace PlanViewer.App; + +/// +/// The names and message grammar shared by both halves of single-instance startup (#489): +/// the launcher side in that decides whether to run or to hand its +/// work to an already-running instance, and the receiver side in 's +/// pipe server that acts on what arrives. +/// +/// Why single-instance at all. Every instance holds a whole-file +/// AppSettings snapshot and Save writes the whole file, so two instances make the +/// settings file last-write-wins: whichever exits second silently clobbers the other's +/// open_tabs and every other setting. AtomicFile prevents torn files, not lost updates. +/// A launch with a file argument already forwarded to the running instance over the named +/// pipe; a bare launch ran a full second instance and was exactly the clobber case. +/// +/// Why the grammar is this shape. The pipe protocol is one line per +/// connection, historically always a file path, and two other sender/receiver pairs speak +/// it: the SSMS extension's AppLauncher (sends plain paths, cannot be updated in lockstep +/// with the app) and any older Studio build still running across an upgrade. So the +/// activation message is not a version field or a framed header — it is a single reserved +/// line that an OLD receiver safely ignores: the pre-#489 handler's only guard is +/// File.Exists(line), and contains characters that +/// are illegal in Windows file names, so it can never name an existing file there. An old +/// receiver reads it, finds no such file, drops it, and keeps serving — the worst-case +/// skew is a bare second launch that exits without surfacing anything, not a crash or a +/// junk tab. +/// +internal static class SingleInstance +{ + /// + /// The pipe every Studio sender and receiver has always shared — also written by the + /// SSMS extension's AppLauncher, which is why the name can never change casually. + /// + internal const string PipeName = "SQLPerformanceStudio_OpenFile"; + + /// + /// Unprefixed, so it lands in the default per-user-session Local\ namespace on + /// Windows — two users (or two RDP sessions) each get their own instance, which is the + /// scope the settings file conflict actually has. Distinct from Lite's + /// PerformanceMonitorLite_SingleInstance; the two apps must never see each other. + /// + internal const string MutexName = "SQLPerformanceStudio_SingleInstance"; + + /// + /// Escape hatch (#489): skip the single-instance check and run a full second instance. + /// A user who runs two on purpose accepts settings last-write-wins as their informed + /// choice. Stripped from argv before any file-open logic sees it. + /// + internal const string NewInstanceFlag = "--new-instance"; + + /// + /// The line a bare second launch sends to mean "surface your main window". The + /// double-colons make it unrepresentable as a Windows file name on purpose — see the + /// class comment for why that property is the entire backward-compatibility story. + /// + internal const string ActivateSentinel = "::activate::"; + + /// What one received pipe line means. See . + internal enum PipeMessage + { + /// Blank, or a path that doesn't exist — dropped silently, exactly as the pre-#489 receiver did. + Ignore, + + /// The — a bare second launch asking this window to surface. + Activate, + + /// An existing file — the SSMS extension or a second launch handing over a path to open. + OpenFile, + } + + /// + /// The receiver's dispatch decision for one pipe line, extracted from the pipe loop so + /// it can be pinned by tests without a pipe. + /// + /// The sentinel is checked before File.Exists, not after: on Windows the + /// order can't matter (the sentinel is an illegal file name), but on Linux a file named + /// ::activate:: is representable, and the reserved meaning must win over any + /// such file. File.Exists on a garbage line returns false rather than throwing, + /// which is what made the old receiver's guard safe and keeps this one safe too. + /// + internal static PipeMessage Classify(string? line) + { + if (string.IsNullOrWhiteSpace(line)) + return PipeMessage.Ignore; + + if (string.Equals(line, ActivateSentinel, StringComparison.Ordinal)) + return PipeMessage.Activate; + + if (File.Exists(line)) + return PipeMessage.OpenFile; + + return PipeMessage.Ignore; + } + + /// True when argv carries anywhere. + internal static bool NewInstanceRequested(string[] args) => + args.Any(IsNewInstanceFlag); + + /// + /// Argv minus every , order otherwise preserved. Applied + /// in BOTH places argv is consumed: (so the forwarded path + /// and the args handed to Avalonia are clean) and + /// (which reads the raw + /// Environment.GetCommandLineArgs() itself, so Program's scrubbed copy never + /// reaches it). Without the second scrub, "PerformanceStudio.exe --new-instance + /// file.sqlplan" would see the flag at args[1] instead of the file. The flag happens to + /// fail that path's File.Exists guard today, but the file arg behind it would + /// still be skipped — being explicit here is what makes the flag invisible rather than + /// merely unlucky. + /// + internal static string[] StripNewInstanceFlag(string[] args) => + args.Where(a => !IsNewInstanceFlag(a)).ToArray(); + + /// + /// Case-insensitive, because Windows users type flags in whatever case survived their + /// muscle memory and there is no second flag for this one to collide with. + /// + private static bool IsNewInstanceFlag(string arg) => + string.Equals(arg, NewInstanceFlag, StringComparison.OrdinalIgnoreCase); +} diff --git a/src/PlanViewer.Core/Models/PlanModels.cs b/src/PlanViewer.Core/Models/PlanModels.cs index d6341d03..84256400 100644 --- a/src/PlanViewer.Core/Models/PlanModels.cs +++ b/src/PlanViewer.Core/Models/PlanModels.cs @@ -12,8 +12,14 @@ public class ParsedPlan public bool ClusteredMode { get; set; } public List Batches { get; set; } = new(); - public List AllMissingIndexes => Batches - .SelectMany(b => b.Statements) + /* #456 follow-up: analysis output lists a missing index per statement, procedure and UDF + bodies included, so this rollup must descend the same way — it feeds the MissingIndexCount + the MCP session registrations report, and an EXEC plan whose only missing-index + suggestions live in the body used to register as having none while the advice discussed + them. Uses the shared traversal rather than its own descent so it cannot drift from what + the analysis actually saw. */ + public List AllMissingIndexes => + Services.PlanStatements.EnumerateAll(this) .SelectMany(s => s.MissingIndexes) .ToList(); } diff --git a/src/PlanViewer.Core/Output/AnalysisJson.cs b/src/PlanViewer.Core/Output/AnalysisJson.cs index 5361d217..884f0f70 100644 --- a/src/PlanViewer.Core/Output/AnalysisJson.cs +++ b/src/PlanViewer.Core/Output/AnalysisJson.cs @@ -70,4 +70,34 @@ public static class AnalysisJson MaxDepth = MaxDepth, DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, }; + + /// + /// The share wire format: default JSON in every respect except the depth ceiling. + /// + /// #431 raised the ceiling for "every writer of this object", and the web share + /// endpoints turned out to be three more writers nobody listed: the upload serialized the + /// analysis with inline default options, and loading a share back parsed and deserialized it + /// at the default 64 again — so a deep-but-real plan (~30 nested operators) analyzed fine on + /// screen and then failed to Share, or shared and failed to open, with an "object cycle" + /// message pointing at the wrong cause. This is deliberately NOT one of the WithoutNulls + /// variants: shares already in the database were written with default formatting, and the fix + /// is the ceiling, not a wire-format change riding along with it. Used for both directions — + /// deserializers enforce MaxDepth too, and a share that was legal to write must be legal to + /// read back. + /// + public static readonly JsonSerializerOptions Wire = new() + { + MaxDepth = MaxDepth, + }; + + /// + /// The same ceiling for + /// call sites: JsonDocumentOptions is a separate type with its own default MaxDepth of 64, so a + /// reader that picks a share apart with JsonDocument before deserializing — which is exactly + /// what loading a share does — hits the same wall the serializer options alone cannot fix. + /// + public static readonly JsonDocumentOptions Document = new() + { + MaxDepth = MaxDepth, + }; } diff --git a/src/PlanViewer.Core/Output/AnalysisResult.cs b/src/PlanViewer.Core/Output/AnalysisResult.cs index c9a3e706..796f7dee 100644 --- a/src/PlanViewer.Core/Output/AnalysisResult.cs +++ b/src/PlanViewer.Core/Output/AnalysisResult.cs @@ -54,9 +54,31 @@ public class AnalysisSummary public class StatementResult { + /// + /// The statement, with the plan's captured parameter values put back where the engine left + /// @0, @1 … (#482). Identical to what the plan records for any statement with + /// nothing to substitute, which is nearly all of them. + /// [JsonPropertyName("statement_text")] public string StatementText { get; set; } = ""; + /// + /// The statement exactly as the plan records it, present only when + /// had values substituted into it and therefore says something + /// different. + /// + /// This is the text that matches the plan cache and Query Store, and the text that has to + /// be paired with a declared parameter list — get_repro_script builds an + /// sp_executesql call around it, and a body with the literals already inlined would + /// declare parameters it never uses and stop reproducing the parameterized compile the script + /// exists to reproduce. + /// + /// Not to be confused with PlanStatement.ParameterizedText, which is showplan's own + /// ParameterizedText element and comes from the plan rather than from this mapping. + /// + [JsonPropertyName("parameterized_statement_text")] + public string? ParameterizedStatementText { get; set; } + [JsonPropertyName("statement_type")] public string StatementType { get; set; } = ""; diff --git a/src/PlanViewer.Core/Output/ResultMapper.cs b/src/PlanViewer.Core/Output/ResultMapper.cs index 13436886..59435f23 100644 --- a/src/PlanViewer.Core/Output/ResultMapper.cs +++ b/src/PlanViewer.Core/Output/ResultMapper.cs @@ -87,9 +87,26 @@ private static StatementResult MapStatement( CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); + + /* #482: every consumer of the analysis — advice for humans, advice for robots, the HTML + export, the comparison report, the MCP tools — reads its statement text from here, so this + is where the parameter values go back in. #467 substituted at the two copy paths it was + looking at, which left the same @0 in every one of these. + + The parser's PlanStatement is deliberately untouched: the analyzer's rules regex over that + text (OPTIMIZE FOR UNKNOWN, NOT IN, MAXDOP hints), the properties panel shows what the plan + records, and none of that should start reading manufactured literals. */ + var runnable = ParameterSubstitution.Apply(stmt.StatementText, stmt.Parameters); + var result = new StatementResult { - StatementText = stmt.StatementText, + StatementText = runnable.Text, + + /* Carried, not discarded — this is the text that matches the plan cache and Query Store, + and get_repro_script has to pair a query body with a declared parameter list. Null when + nothing was substituted so it never appears on the overwhelming majority of statements + that have nothing to say here. */ + ParameterizedStatementText = runnable.SubstitutionCount > 0 ? stmt.StatementText : null, StatementType = stmt.StatementType, EstimatedCost = stmt.StatementSubTreeCost, EstimatedRows = stmt.StatementEstRows, diff --git a/src/PlanViewer.Core/Services/BenefitScorer.cs b/src/PlanViewer.Core/Services/BenefitScorer.cs index 21b3145f..2d5ddd14 100644 --- a/src/PlanViewer.Core/Services/BenefitScorer.cs +++ b/src/PlanViewer.Core/Services/BenefitScorer.cs @@ -30,23 +30,26 @@ public static void Score(ParsedPlan plan) => internal static void ScoreCancellable(ParsedPlan plan, CancellationToken cancellationToken) { - foreach (var batch in plan.Batches) + /* #456 made the analyzer descend into stored procedure and UDF bodies via + PlanStatements.EnumerateAll, but this walk was left on batch.Statements. The analyzer + then CREATED warnings on the body statements and the scorer never visited them, so their + MaxBenefitPercent stayed null and their wait stats were never scored or surfaced — the + UI sorts unquantified warnings below quantified ones, which quietly buried every finding + inside an EXEC plan. Same shared traversal as the analyzer so the two passes + cannot see different statements again. */ + foreach (var stmt in PlanStatements.EnumerateAll(plan)) { cancellationToken.ThrowIfCancellationRequested(); - foreach (var stmt in batch.Statements) - { - cancellationToken.ThrowIfCancellationRequested(); - ScoreStatementWarnings(stmt); + ScoreStatementWarnings(stmt); - if (stmt.RootNode != null) - ScoreNodeTree(stmt.RootNode, stmt, cancellationToken); + if (stmt.RootNode != null) + ScoreNodeTree(stmt.RootNode, stmt, cancellationToken); - if (stmt.WaitStats.Count > 0 && stmt.QueryTimeStats != null) - ScoreWaitStats(stmt); + if (stmt.WaitStats.Count > 0 && stmt.QueryTimeStats != null) + ScoreWaitStats(stmt); - if (stmt.WaitStats.Count > 0) - EmitWaitStatWarnings(stmt); - } + if (stmt.WaitStats.Count > 0) + EmitWaitStatWarnings(stmt); } } diff --git a/src/PlanViewer.Core/Services/ParameterSubstitution.cs b/src/PlanViewer.Core/Services/ParameterSubstitution.cs index 1428e2e7..3ab9c521 100644 --- a/src/PlanViewer.Core/Services/ParameterSubstitution.cs +++ b/src/PlanViewer.Core/Services/ParameterSubstitution.cs @@ -47,6 +47,14 @@ public static ParameterSubstitutionResult Apply( if (values.Count == 0) return new ParameterSubstitutionResult(statementText, 0); + /* One fact about the whole statement, settled up front: inside an EXEC statement, every + token sitting to the left of an "=" is an assignment target — the return-status variable + or a named argument's name — because EXEC grammar has no other use for "=" at all. A + per-token back-scan cannot see this for the FIRST named argument (what precedes it is + the procedure name, not a keyword), which is how "EXEC dbo.p @debug = @debug" got its + left-hand side substituted into "EXEC dbo.p 1 = @debug". */ + var assignsThroughEquals = StatementLeadsWithExec(statementText); + var sb = new StringBuilder(statementText.Length); var substitutions = 0; var i = 0; @@ -93,7 +101,8 @@ tail of an identifier such as "t@0" is part of that identifier. The scan below c end++; var token = statementText[i..end]; - if (values.TryGetValue(token, out var value)) + if (values.TryGetValue(token, out var value) + && !IsAssignmentTarget(statementText, i, end, assignsThroughEquals)) { sb.Append(value); substitutions++; @@ -116,6 +125,261 @@ tail of an identifier such as "t@0" is part of that identifier. The scan below c : new ParameterSubstitutionResult(sb.ToString(), substitutions); } + /// + /// True when the parameter token spanning to is + /// being assigned TO rather than read from, in which case its value must not be written over it. + /// + /// Why (#482). SELECT @job_name = name, @owner_sid = owner_sid FROM … is a + /// real committed plan, and its ParameterList records both of those variables with a compiled + /// value of NULL. Substituted blindly it reads SELECT NULL = name, NULL = owner_sid + /// — not merely unrunnable but quietly misleading, because it now looks like a comparison. That + /// mattered less while substitution was something you asked for on the clipboard; #482 makes it + /// what the advice, the exports and the MCP tools show by default. + /// + /// The lead-ins recognised: SELECT @a = … and SET @a = …, with one balanced + /// parenthesized group tolerated between the keyword and the target so SELECT TOP (1) @a = + /// … is the assignment it is; the second and later items of either list, via the comma; any + /// token in front of an = when the statement is an EXEC + /// ( — see ); and + /// FETCH … INTO @a, @b, which assigns with no = anywhere. A parameter on the right + /// of an = is a read and is left to be substituted, and so is one followed by >=, + /// <=, <> or !=, since the scan forward from the token meets that + /// operator's first character rather than the =. + /// + private static bool IsAssignmentTarget(string text, int start, int end, bool assignsThroughEquals) + { + var forward = end; + while (forward < text.Length && char.IsWhiteSpace(text[forward])) + forward++; + + var followedByLoneEquals = + forward < text.Length + && text[forward] == '=' + && !(forward + 1 < text.Length && text[forward + 1] == '='); + + /* FETCH ... INTO @a, @b assigns without any "=": what follows the token is a comma or the + end of the statement, so the forward scan has nothing to find and the question is + answered entirely by what leads the list. Checked only on the no-equals path — "INTO + @a =" is not a shape T-SQL has — which keeps every "=" case on the scans below. */ + if (!followedByLoneEquals) + return IsFetchIntoTarget(text, start); + + /* EXEC @rc = dbo.proc, and the FIRST named argument of EXEC dbo.p @debug = @debug. The + back-scan below cannot see either (what precedes them is a procedure name, not SELECT + or SET), but EXEC grammar has no reads-then-"=" shape at all, so inside one the "=" + alone settles it. Later named arguments were already caught by the comma rule; this + makes the first one match its siblings. */ + if (assignsThroughEquals) + return true; + + var back = start - 1; + while (back >= 0 && char.IsWhiteSpace(text[back])) + back--; + + if (back < 0) + return false; + + /* "SELECT @a = x, @b = y" — everything after the first comma is still a select list. */ + if (text[back] == ',') + return true; + + /* SELECT TOP (1) @a = col: the token before the target is TOP's balanced group, which the + old scan met as ")" and gave up on — it extracted an empty word and substituted @a into + "NULL = col". Skip exactly ONE balanced group backwards and judge what precedes it; + nothing but TOP puts a group in an assignment lead-in, so anything else still answers + no. TOP (@n) rides along for free — the group's content is opaque to this walk, and @n + itself is a read whose own forward scan meets ")" rather than "=". */ + if (text[back] == ')') + { + var depth = 0; + while (back >= 0) + { + if (text[back] == ')') + { + depth++; + } + else if (text[back] == '(' && --depth == 0) + { + back--; + break; + } + + back--; + } + + /* Unbalanced means truncated statement text. With no way to tell what leads in, stay + on the substitute side, which is where the empty-word scan always landed. */ + if (depth != 0) + return false; + + while (back >= 0 && char.IsWhiteSpace(text[back])) + back--; + } + + var word = ReadWordEndingAt(text, ref back); + + /* TOP sits between SELECT and its group, so the decider is the word before it. */ + if (word.Equals("TOP", StringComparison.OrdinalIgnoreCase)) + { + while (back >= 0 && char.IsWhiteSpace(text[back])) + back--; + word = ReadWordEndingAt(text, ref back); + } + + return word.Equals("SELECT", StringComparison.OrdinalIgnoreCase) + || word.Equals("SET", StringComparison.OrdinalIgnoreCase); + } + + /// + /// True when the token at sits in an INTO list — FETCH … INTO @a, + /// @b — which assigns into its variables with no = in sight. + /// + /// The walk hops backwards over ", @name" pairs so every member of the list is + /// protected, not just the first. The hop insists each prior list item is itself an @token, + /// which is how the commas of an IN list, a VALUES row, or a positional EXEC argument list — + /// all places a parameter is read — fall out: at the first non-variable item, or at the "(" + /// that opens them. INSERT INTO @tv lands here too, and "assignment target" is the + /// right answer there as well — a table variable's NAME is never a value to write over. + /// + private static bool IsFetchIntoTarget(string text, int start) + { + var back = start - 1; + + while (true) + { + while (back >= 0 && char.IsWhiteSpace(text[back])) + back--; + + if (back < 0) + return false; + + if (text[back] == ',') + { + back--; + while (back >= 0 && char.IsWhiteSpace(text[back])) + back--; + + var previousItem = ReadWordEndingAt(text, ref back); + if (previousItem.Length == 0 || previousItem[0] != '@') + return false; + + continue; + } + + return ReadWordEndingAt(text, ref back) + .Equals("INTO", StringComparison.OrdinalIgnoreCase); + } + } + + /// + /// Whether the statement's leading keyword chain reaches EXEC or EXECUTE, in which case an + /// = can only ever mean assignment — the return-status variable or a named argument. + /// + /// Why that holds statement-wide rather than per argument: T-SQL restricts an EXEC + /// argument's VALUE to a literal, a variable, NULL, or DEFAULT. An expression — a CASE, a + /// subquery, arithmetic — is a compile error there (Incorrect syntax), so no legal + /// compiled statement can put a read like CASE WHEN @x = 1 … to the right of a named + /// argument. The only @name = pairs that can exist in the region this flag governs are + /// assignments. A hand-crafted plan file can of course contain anything, but its author already + /// controls the whole statement text, so precision against it protects nothing. + /// + /// The chain is EXEC itself, or the INSERT … EXEC prelude: INSERT, an optional INTO, the + /// target's possibly bracketed and dotted name, an optional parenthesized column list. The walk + /// is forward from the start and gives up at the first character that cannot belong to such a + /// prelude — an operator, a quote, a semicolon — so a SELECT or UPDATE never gets anywhere near + /// a false positive: it hits its own = or * within a few tokens. Bracketed name + /// parts are skipped opaquely so a procedure named [exec] cannot read as the keyword, and + /// EXEC('…') is no counterexample — the string stays opaque to Apply's main loop, so nothing + /// inside it is substituted regardless of what this answers. Leading comments are not chased; + /// a statement that opens with one keeps the old per-token behavior. + /// + private static bool StatementLeadsWithExec(string text) + { + var i = 0; + + /* A handful of tokens is all the INSERT ... EXEC prelude can hold. The bound is a fuse + against pathological text, not part of the grammar. */ + for (var tokens = 0; tokens < 8; tokens++) + { + while (i < text.Length && char.IsWhiteSpace(text[i])) + i++; + + if (i >= text.Length) + return false; + + var c = text[i]; + + // The INSERT target's column list, skipped as one balanced unit. + if (c == '(') + { + var depth = 0; + while (i < text.Length) + { + if (text[i] == '(') + { + depth++; + } + else if (text[i] == ')' && --depth == 0) + { + i++; + break; + } + + i++; + } + + if (depth != 0) + return false; + + continue; + } + + // A delimited name part, skipped opaquely; dots just join parts. + if (c == '[' || c == '"') + { + var close = text.IndexOf(c == '[' ? ']' : '"', i + 1); + if (close < 0) + return false; + + i = close + 1; + continue; + } + + if (c == '.') + { + i++; + continue; + } + + if (!IsIdentifierPart(c)) + return false; + + var wordStart = i; + while (i < text.Length && IsIdentifierPart(text[i])) + i++; + + var word = text[wordStart..i]; + if (word.Equals("EXEC", StringComparison.OrdinalIgnoreCase) + || word.Equals("EXECUTE", StringComparison.OrdinalIgnoreCase)) + return true; + } + + return false; + } + + /// + /// The identifier run ending at , or the empty string when the character + /// there is not part of one. Leaves on the character before the run. + /// + private static string ReadWordEndingAt(string text, ref int back) + { + var wordEnd = back + 1; + while (back >= 0 && IsIdentifierPart(text[back])) + back--; + + return text[(back + 1)..wordEnd]; + } + /// /// Maps parameter name to the literal that should replace it, skipping parameters with no /// captured value at all. diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs index ce63fa3e..dbd1b217 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Detection.cs @@ -211,8 +211,12 @@ private static bool IsScanOperator(PlanNode node) /// why this reads the CONVERT_IMPLICIT argument list rather than splitting on the comparison /// operator the way does. The first argument is the target /// type and carries no brackets; a column reference in the remainder is the conversion input. + /// + /// Internal so the column-vs-variable line can be tested against raw predicate strings in + /// showplan shape — the table-variable case ([@tv].[col] IS a column) has no committed plan + /// fixture, and the distinction lives entirely in this method and the regex it shares. /// - private static bool ConvertImplicitWrapsColumn(string predicate) + internal static bool ConvertImplicitWrapsColumn(string predicate) { foreach (Match match in ConvertImplicitRegex.Matches(predicate)) { diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.Helpers.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.Helpers.cs index 6bf75858..cb141850 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.Helpers.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.Helpers.cs @@ -8,11 +8,20 @@ namespace PlanViewer.Core.Services; public static partial class PlanAnalyzer { + /* Both passes below match on WarningType alone, and a type name is not unique to us: + "Implicit Conversion" is rule 29's legacy-listed type AND what the parser stamps on the + engine's own PlanAffectingConvert element (Source = SqlServer). Matching by name only + therefore branded the ENGINE's record "[SQL Server] [legacy]" — a badge that exists to + flag our un-migrated rules on a warning that is not ours at all — and TryOverrideSeverity + routed a user's rule-number override onto engine warnings the rule never produced (the + Contains matching makes it worse: every engine Spill variant lands on rule 7, "Memory + Grant" on rule 9). Legacy status and rule severity are facts about OUR rules, so anything + the engine said is skipped by both. */ private static void MarkLegacyWarnings(PlanStatement stmt) { foreach (var w in stmt.PlanWarnings) { - if (LegacyWarningTypes.Contains(w.WarningType)) + if (w.Source != PlanWarningSource.SqlServer && LegacyWarningTypes.Contains(w.WarningType)) w.IsLegacy = true; } if (stmt.RootNode != null) @@ -23,25 +32,28 @@ private static void MarkLegacyWarningsOnTree(PlanNode node) { foreach (var w in node.Warnings) { - if (LegacyWarningTypes.Contains(w.WarningType)) + if (w.Source != PlanWarningSource.SqlServer && LegacyWarningTypes.Contains(w.WarningType)) w.IsLegacy = true; } foreach (var child in node.Children) MarkLegacyWarningsOnTree(child); } + /* #456 follow-up: the analyzer walks every statement — stored procedure and UDF bodies + included — through PlanStatements.EnumerateAll, but this pass still walked batch.Statements, + so a user's severity override applied to a warning on the outer batch and silently did not + apply to the identical warning inside an EXEC body. (MarkLegacyWarnings does not + have this problem: it is called per-statement from inside the analyzer's EnumerateAll loop, + so it was carried along when that loop learned to descend.) */ private static void ApplySeverityOverrides(ParsedPlan plan, AnalyzerConfig cfg) { - foreach (var batch in plan.Batches) + foreach (var stmt in PlanStatements.EnumerateAll(plan)) { - foreach (var stmt in batch.Statements) - { - foreach (var w in stmt.PlanWarnings) - TryOverrideSeverity(w, cfg); + foreach (var w in stmt.PlanWarnings) + TryOverrideSeverity(w, cfg); - if (stmt.RootNode != null) - ApplyOverridesToTree(stmt.RootNode, cfg); - } + if (stmt.RootNode != null) + ApplyOverridesToTree(stmt.RootNode, cfg); } } @@ -55,6 +67,11 @@ private static void ApplyOverridesToTree(PlanNode node, AnalyzerConfig cfg) private static void TryOverrideSeverity(PlanWarning warning, AnalyzerConfig cfg) { + /* The engine's warnings carry no rule number because no rule of ours produced them; see + the comment on MarkLegacyWarnings for what the name-only matching did to them here. */ + if (warning.Source == PlanWarningSource.SqlServer) + return; + // Find the rule number for this warning type (partial match for flexibility) int? ruleNumber = null; foreach (var (rule, type) in RuleWarningTypes) diff --git a/src/PlanViewer.Core/Services/PlanAnalyzer.cs b/src/PlanViewer.Core/Services/PlanAnalyzer.cs index 2d064441..aea2befb 100644 --- a/src/PlanViewer.Core/Services/PlanAnalyzer.cs +++ b/src/PlanViewer.Core/Services/PlanAnalyzer.cs @@ -28,11 +28,15 @@ public static partial class PlanAnalyzer @"\bCONVERT_IMPLICIT\s*\(", RegexOptions.IgnoreCase | RegexOptions.Compiled); - // A column reference in a ScalarString is multi-part bracket-qualified ([schema].[table]). - // A variable is a single bracket pair with an @ prefix ([@0]), so excluding @ from the first - // part is what separates the two. + /* A column reference in a ScalarString is multi-part bracket-qualified ([schema].[table]). + A variable is a single bracket pair with an @ prefix ([@0]) and no dotted part after it — + the "].[" sequence is what separates the two, NOT the @. The first cut of this pattern also + excluded @ from the first part, which read as belt-and-braces but was actually a hole: a + TABLE-variable column renders as [@tv].[col], so a genuine column-side CONVERT_IMPLICIT on + one failed the match and the Non-SARGable warning silently vanished. A bare [@p] still + cannot match, because nothing dotted follows it. */ private static readonly Regex ColumnReferenceRegex = new( - @"\[[^\]@]+\]\.\[", + @"\[[^\]]+\]\.\[", RegexOptions.Compiled); public static void Analyze(ParsedPlan plan, AnalyzerConfig? config = null, ServerMetadata? serverMetadata = null) => diff --git a/src/PlanViewer.Core/Services/PlanStatements.cs b/src/PlanViewer.Core/Services/PlanStatements.cs index 97390a5b..ca347719 100644 --- a/src/PlanViewer.Core/Services/PlanStatements.cs +++ b/src/PlanViewer.Core/Services/PlanStatements.cs @@ -34,35 +34,90 @@ public static IEnumerable EnumerateAll(ParsedPlan plan) } /// - /// and everything nested beneath them. + /// and everything nested beneath them. Defined in terms of + /// so the plain and the + /// context-carrying enumeration are literally the same walk — a consumer picking either one + /// gets the same statements in the same order, which is the whole point of this class. + /// + public static IEnumerable EnumerateAll(IReadOnlyList statements) + { + foreach (var entry in EnumerateAllWithContainer(statements)) + yield return entry.Statement; + } + + /// + /// Every statement in the plan with the name of the module whose body it came from, for + /// consumers that show statements to a person rather than just walking them. + /// + /// Why the context matters (#456 follow-up): once the desktop statements grid started + /// listing procedure-body statements alongside the outer batch, five rows of bare SELECTs with + /// nothing saying which came from EXEC dbo.Whatever would be technically complete and + /// practically unreadable. The container name is part of the traversal rather than something + /// each UI reconstructs with its own walk, because two walks that must agree is exactly the + /// arrangement that produced #455. + /// + public static IEnumerable EnumerateAllWithContainer(ParsedPlan plan) + { + foreach (var batch in plan.Batches) + { + foreach (var entry in EnumerateAllWithContainer(batch.Statements)) + yield return entry; + } + } + + /// + /// and everything nested beneath them, each paired with the + /// module path it lives in (null for the outer batch). /// /// An explicit stack rather than recursion, because procedure bodies nest — a procedure /// calling a procedure calling a function — and #430 was a crash caused by assuming a plan's /// shapes are shallow. /// - public static IEnumerable EnumerateAll(IReadOnlyList statements) + public static IEnumerable EnumerateAllWithContainer(IReadOnlyList statements) { - var pending = new Stack(); + var pending = new Stack(); for (var i = statements.Count - 1; i >= 0; i--) - pending.Push(statements[i]); + pending.Push(new StatementWithContainer(statements[i], ContainerPath: null)); - while (pending.TryPop(out var statement)) + while (pending.TryPop(out var entry)) { - yield return statement; + yield return entry; /* Pushed in reverse so the bodies come back out in source order, and pushed AFTER the statement is yielded so a body follows the EXEC that owns it rather than preceding it. */ + var statement = entry.Statement; for (var i = statement.UdfPlans.Count - 1; i >= 0; i--) - PushAll(statement.UdfPlans[i].Statements, pending); + PushAll(statement.UdfPlans[i], entry.ContainerPath, pending); if (statement.StoredProcPlan is not null) - PushAll(statement.StoredProcPlan.Statements, pending); + PushAll(statement.StoredProcPlan, entry.ContainerPath, pending); } } - private static void PushAll(IReadOnlyList statements, Stack pending) + private static void PushAll(FunctionPlanInfo body, string? outerPath, Stack pending) { - for (var i = statements.Count - 1; i >= 0; i--) - pending.Push(statements[i]); + var path = AppendModule(outerPath, body.ProcName); + for (var i = body.Statements.Count - 1; i >= 0; i--) + pending.Push(new StatementWithContainer(body.Statements[i], path)); + } + + /// + /// Chains module names for nesting, so a statement two bodies deep reads + /// "dbo.Outer > dbo.Inner" rather than pretending it sits directly in dbo.Inner's caller. + /// A module the plan left unnamed contributes nothing to the path — the traversal reports what + /// the plan said, not a placeholder — so its statements inherit the enclosing path (or none). + /// + private static string? AppendModule(string? outerPath, string procName) + { + if (string.IsNullOrEmpty(procName)) + return outerPath; + return outerPath is null ? procName : outerPath + " > " + procName; } } + +/// +/// One statement from : +/// the statement itself, and the proc/UDF module path it came from — null for a statement in the +/// outer batch, "dbo.Proc" for a procedure body, "dbo.Outer > dbo.Inner" for nested bodies. +/// +public readonly record struct StatementWithContainer(PlanStatement Statement, string? ContainerPath); diff --git a/src/PlanViewer.Core/Services/ShowPlanParser.cs b/src/PlanViewer.Core/Services/ShowPlanParser.cs index 453d2095..b5a778e9 100644 --- a/src/PlanViewer.Core/Services/ShowPlanParser.cs +++ b/src/PlanViewer.Core/Services/ShowPlanParser.cs @@ -15,14 +15,23 @@ public static partial class ShowPlanParser // Plan XML is untrusted input (opened/pasted/downloaded). Cap recursion depth so a // maliciously deep tree throws a catchable exception instead of an uncatchable // StackOverflowException that takes the whole process down. - private const int MaxParseDepth = 1000; - private const int MaxParseCharacters = 16 * 1024 * 1024; + // Internal so tests can pin behavior just past each limit without hardcoding the values. + internal const int MaxParseDepth = 1000; + internal const int MaxParseCharacters = 16 * 1024 * 1024; public static ParsedPlan Parse(string xml) { var plan = new ParsedPlan { RawXml = xml }; try { + /* Same ceiling ParseAsync enforces through XmlReaderSettings.MaxCharactersInDocument, + which this synchronous path (PlanViewerControl, the web viewer, the analysis + pipeline) never had - it went straight to XDocument.Parse with no limit at all. + The input is already an in-memory string here, so a length check is the equivalent + guard; like the reader setting, the limit is in characters, not bytes. */ + if (xml.Length > MaxParseCharacters) + throw new InvalidOperationException( + $"Plan XML exceeds the supported size limit of {MaxParseCharacters.ToString("N0", CultureInfo.InvariantCulture)} characters."); return ParseDocument(XDocument.Parse(xml), plan, CancellationToken.None); } catch (Exception exception) @@ -109,7 +118,7 @@ private static ParsedPlan ParseDocument( foreach (var statementElement in root.Descendants(Ns + "StmtSimple")) { cancellationToken.ThrowIfCancellationRequested(); - var statement = ParseStatement(statementElement, cancellationToken); + var statement = ParseStatement(statementElement, depth: 0, cancellationToken); if (statement is not null) batch.Statements.Add(statement); } @@ -205,6 +214,20 @@ private static List ParseStatementAndChildren( stmt.CursorRequestedType = cursorRequestedType; stmt.CursorConcurrency = cursorConcurrency; stmt.CursorForwardOnly = cursorForwardOnly; + + /* #491: the same StoredProc/UDF descent every other statement shape gets. + #456 taught ParseStatement to read sub-plan bodies, but a cursor's + operation statements are built HERE, through ParseQueryPlanAsStatement, + and never pass through ParseStatement - so a function called by the + cursor's query carried its whole body in the XML (the Operation element + holds the UDF sub-plan right next to this QueryPlan) and the parser + dropped every statement of it. Attaching the bodies to the operation's + statement is all it takes: PlanStatements.EnumerateAll already walks + UdfPlans/StoredProcPlan on every statement it yields, and since #486 + every consumer reads that traversal. Depth passes through unchanged + (#484) - resetting it at this boundary would reopen the MaxParseDepth + bypass across cursor/procedure nesting. */ + ParseSubPlans(stmt, opEl, depth, cancellationToken); results.Add(stmt); } } @@ -213,7 +236,7 @@ private static List ParseStatementAndChildren( else { // StmtSimple or any other statement type - var stmt = ParseStatement(stmtEl, cancellationToken); + var stmt = ParseStatement(stmtEl, depth, cancellationToken); if (stmt != null) results.Add(stmt); } @@ -221,8 +244,16 @@ private static List ParseStatementAndChildren( return results; } + /* The depth parameter exists for the StoredProc/UDF descents below. #456 added those + descents calling ParseStatementAndChildren without a depth argument, so the recursion + depth silently reset to zero at every procedure boundary and the MaxParseDepth guard in + ParseStatementAndChildren could never fire across StoredProc/UDF nesting - a crafted plan + alternating StmtSimple > StoredProc > Statements a few thousand levels deep (about sixty + bytes each) still reached the uncatchable StackOverflowException the guard exists to + prevent. Carrying the caller's depth through this method closes that reset. */ private static PlanStatement? ParseStatement( XElement stmtEl, + int depth = 0, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); @@ -283,46 +314,7 @@ private static List ParseStatementAndChildren( so it took that early return and never reached this code, seventy lines further down. The parser looked like it descended into procedures and in the one case that matters never did. The same was true of a UDF call whose statement carries no plan of its own. */ - // XSD gap: UDF sub-plans - foreach (var udfEl in stmtEl.Elements(Ns + "UDF")) - { - var udfInfo = new FunctionPlanInfo - { - ProcName = udfEl.Attribute("ProcName")?.Value ?? "", - IsNativelyCompiled = udfEl.Attribute("IsNativelyCompiled")?.Value is "true" or "1" - }; - var udfStmts = udfEl.Element(Ns + "Statements"); - if (udfStmts != null) - { - foreach (var childStmt in udfStmts.Elements()) - { - var parsed = ParseStatementAndChildren(childStmt, cancellationToken: cancellationToken); - udfInfo.Statements.AddRange(parsed); - } - } - stmt.UdfPlans.Add(udfInfo); - } - - // XSD gap: StoredProc sub-plan - var storedProcEl = stmtEl.Element(Ns + "StoredProc"); - if (storedProcEl != null) - { - var spInfo = new FunctionPlanInfo - { - ProcName = storedProcEl.Attribute("ProcName")?.Value ?? "", - IsNativelyCompiled = storedProcEl.Attribute("IsNativelyCompiled")?.Value is "true" or "1" - }; - var spStmts = storedProcEl.Element(Ns + "Statements"); - if (spStmts != null) - { - foreach (var childStmt in spStmts.Elements()) - { - var parsed = ParseStatementAndChildren(childStmt, cancellationToken: cancellationToken); - spInfo.Statements.AddRange(parsed); - } - } - stmt.StoredProcPlan = spInfo; - } + ParseSubPlans(stmt, stmtEl, depth, cancellationToken); if (queryPlanEl == null) { @@ -383,6 +375,64 @@ did. The same was true of a UDF call whose statement carries no plan of its own. return stmt; } + /// + /// Reads the StoredProc/UDF sub-plan bodies hanging off onto + /// . One reader shared by ParseStatement — where StmtSimple carries the + /// UDF/StoredProc elements directly — and the StmtCursor branch (#491), where the same UDF + /// element sits beside the QueryPlan under CursorPlan > Operation instead, so the two shapes + /// cannot drift apart the way the analyzer and the mapper once did (#455). + /// The caller's depth carries into the body statements (#484): this descent is what makes + /// module nesting recursive, and resetting depth at a sub-plan boundary is exactly the + /// MaxParseDepth bypass #484 closed. + /// + private static void ParseSubPlans( + PlanStatement stmt, + XElement containerEl, + int depth, + CancellationToken cancellationToken) + { + // XSD gap: UDF sub-plans + foreach (var udfEl in containerEl.Elements(Ns + "UDF")) + { + var udfInfo = new FunctionPlanInfo + { + ProcName = udfEl.Attribute("ProcName")?.Value ?? "", + IsNativelyCompiled = udfEl.Attribute("IsNativelyCompiled")?.Value is "true" or "1" + }; + var udfStmts = udfEl.Element(Ns + "Statements"); + if (udfStmts != null) + { + foreach (var childStmt in udfStmts.Elements()) + { + var parsed = ParseStatementAndChildren(childStmt, depth + 1, cancellationToken); + udfInfo.Statements.AddRange(parsed); + } + } + stmt.UdfPlans.Add(udfInfo); + } + + // XSD gap: StoredProc sub-plan + var storedProcEl = containerEl.Element(Ns + "StoredProc"); + if (storedProcEl != null) + { + var spInfo = new FunctionPlanInfo + { + ProcName = storedProcEl.Attribute("ProcName")?.Value ?? "", + IsNativelyCompiled = storedProcEl.Attribute("IsNativelyCompiled")?.Value is "true" or "1" + }; + var spStmts = storedProcEl.Element(Ns + "Statements"); + if (spStmts != null) + { + foreach (var childStmt in spStmts.Elements()) + { + var parsed = ParseStatementAndChildren(childStmt, depth + 1, cancellationToken); + spInfo.Statements.AddRange(parsed); + } + } + stmt.StoredProcPlan = spInfo; + } + } + /// /// Parse a QueryPlan element that comes from a cursor Operation (no parent StmtSimple attributes). /// diff --git a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs index a38cc167..d33afa94 100644 --- a/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs +++ b/src/PlanViewer.Ssms/Properties/AssemblyInfo.cs @@ -7,5 +7,5 @@ [assembly: AssemblyProduct("Performance Studio for SSMS")] [assembly: AssemblyCopyright("Copyright Darling Data 2026")] [assembly: ComVisible(false)] -[assembly: AssemblyVersion("1.23.0.0")] -[assembly: AssemblyFileVersion("1.23.0.0")] +[assembly: AssemblyVersion("1.24.0.0")] +[assembly: AssemblyFileVersion("1.24.0.0")] diff --git a/src/PlanViewer.Ssms/source.extension.vsixmanifest b/src/PlanViewer.Ssms/source.extension.vsixmanifest index 15b50bca..28464a3c 100644 --- a/src/PlanViewer.Ssms/source.extension.vsixmanifest +++ b/src/PlanViewer.Ssms/source.extension.vsixmanifest @@ -3,7 +3,7 @@ xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011"> Performance Studio for SSMS diff --git a/src/PlanViewer.Web/Pages/Index.razor b/src/PlanViewer.Web/Pages/Index.razor index ffa6b28d..f4878cdd 100644 --- a/src/PlanViewer.Web/Pages/Index.razor +++ b/src/PlanViewer.Web/Pages/Index.razor @@ -214,7 +214,14 @@ else private PlanNode? selectedNode; private StatementResult? ActiveStmt => result?.Statements.ElementAtOrDefault(activeStatement); - private PlanStatement? ActiveStmtPlan => parsedPlan?.Batches.SelectMany(b => b.Statements).ElementAtOrDefault(activeStatement); + /* activeStatement indexes result.Statements, which ResultMapper builds from + PlanStatements.EnumerateAll — since #456 that includes statements inside stored procedure + and UDF bodies. Mapping the same index into the outer-only batch list returned null for + every body statement, so clicking a body statement's tab showed its analysis with no + operator tree. The two lists must be walked by the same traversal to line up. */ + private PlanStatement? ActiveStmtPlan => parsedPlan == null + ? null + : PlanStatements.EnumerateAll(parsedPlan).ElementAtOrDefault(activeStatement); private async Task AnalyzePasted() { diff --git a/src/PlanViewer.Web/PlanViewer.Web.csproj b/src/PlanViewer.Web/PlanViewer.Web.csproj index 9e26aeaa..05845575 100644 --- a/src/PlanViewer.Web/PlanViewer.Web.csproj +++ b/src/PlanViewer.Web/PlanViewer.Web.csproj @@ -36,7 +36,9 @@ + + diff --git a/src/PlanViewer.Web/Services/PlanShareService.cs b/src/PlanViewer.Web/Services/PlanShareService.cs index 3368dfb1..e25d511a 100644 --- a/src/PlanViewer.Web/Services/PlanShareService.cs +++ b/src/PlanViewer.Web/Services/PlanShareService.cs @@ -46,12 +46,16 @@ public sealed class PlanShareService : IPlanShareService public async Task ShareAsync(AnalysisResult result, string text, int ttlDays) { + /* AnalysisJson.Wire, not default options: this serializes an AnalysisResult, and #431's + depth ceiling exists for "every writer of this object". Default MaxDepth is 64, an + operator costs two JSON levels, so sharing a plan ~30 operators deep threw an "object + cycle" JsonException here while the same analysis rendered fine everywhere else. */ var payload = JsonSerializer.Serialize(new { result = result, text = text, ttl_days = ttlDays - }); + }, AnalysisJson.Wire); var content = new StringContent(payload, Encoding.UTF8, "application/json"); var response = await _http.PostAsync($"{ApiBase}/api/share", content); @@ -83,9 +87,13 @@ public async Task LoadAsync(string id) } var json = await response.Content.ReadAsStringAsync(); - using var doc = JsonDocument.Parse(json); + /* Reading a share hits the default 64 ceiling twice — JsonDocumentOptions and + JsonSerializerOptions each carry their own MaxDepth — so a deep plan that ShareAsync + could now write would still fail to load without both raised. A share must never be + writable but not readable. */ + using var doc = JsonDocument.Parse(json, AnalysisJson.Document); var root = doc.RootElement; - var result = JsonSerializer.Deserialize(root.GetProperty("result").GetRawText()); + var result = JsonSerializer.Deserialize(root.GetProperty("result").GetRawText(), AnalysisJson.Wire); var text = root.GetProperty("text").GetString(); return new SharedPlan(result, text); } diff --git a/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs b/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs index b786d494..49c592b2 100644 --- a/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs +++ b/tests/PlanViewer.Core.Tests/AnalysisJsonDepthTests.cs @@ -107,6 +107,11 @@ from field in type.GetFields(System.Reflection.BindingFlags.NonPublic /// Pins the headroom itself. 1024 is about 500 nested operators against the ~30 that used to fail; /// a future edit dropping it back toward the default would re-open #430 for large plans only, which /// is the shape of bug that reaches users rather than tests. + /// + /// The Wire and Document pins matter beyond this assembly: server/PlanShare cannot + /// reference PlanViewer.Core and mirrors this number as a literal (a shared constant only + /// helps call sites that reference it), so this test is the tripwire that a change here means + /// changing the server too. /// [Fact] public void TheCeilingIsFarAboveAnyRealPlan() @@ -114,5 +119,50 @@ public void TheCeilingIsFarAboveAnyRealPlan() Assert.Equal(1024, AnalysisJson.MaxDepth); Assert.Equal(AnalysisJson.MaxDepth, AnalysisJson.Indented.MaxDepth); Assert.True(AnalysisJson.Indented.WriteIndented, "advice output is read by people as well as models"); + Assert.Equal(AnalysisJson.MaxDepth, AnalysisJson.Wire.MaxDepth); + Assert.Equal(AnalysisJson.MaxDepth, AnalysisJson.Document.MaxDepth); + Assert.False(AnalysisJson.Wire.WriteIndented, + "the share wire format has always been default-formatted JSON; only the ceiling changed"); + } + + /// + /// The three writers #431 missed, found in review: the web Share upload serialized the + /// analysis with inline default options, and loading a share back parsed AND deserialized it + /// at the default 64 again — so a deep plan analyzed fine on screen and then failed to Share, + /// or shared and failed to open. This walks the exact envelope shape the share path uses + /// ({result, text, ttl_days} → JsonDocument → GetRawText → Deserialize) through the shared + /// options, at 100 operators — past the old ceiling, far under the new one. + /// + /// Scope, stated honestly: the actual call sites live in PlanViewer.Web (a Blazor WASM + /// project this suite does not reference) and server/PlanShare (which references nothing), so + /// they are verified by inspection to use these options / mirror this constant. What this test + /// pins is the contract those sites rely on — that the shared options round-trip the envelope + /// both directions. + /// + [Fact] + public void TheShareEnvelopeRoundTripsADeepPlan() + { + var payload = JsonSerializer.Serialize(new + { + result = WithOperatorChain(100), + text = "=== Summary ===", + ttl_days = 7 + }, AnalysisJson.Wire); + + /* Both proofs that the options are load-bearing: the default reader rejects what the + shared writer produced (ThrowsAny because document parsing surfaces the depth failure + as JsonReaderException, a JsonException subclass)... */ + Assert.ThrowsAny(() => JsonDocument.Parse(payload)); + + /* ...and the shared reader carries it, all the way back to a typed result. */ + using var doc = JsonDocument.Parse(payload, AnalysisJson.Document); + var result = JsonSerializer.Deserialize( + doc.RootElement.GetProperty("result").GetRawText(), AnalysisJson.Wire); + + Assert.NotNull(result); + var depth = 0; + for (var op = result!.Statements.Single().OperatorTree; op is not null; op = op.Children.FirstOrDefault()) + depth++; + Assert.Equal(100, depth); } } diff --git a/tests/PlanViewer.Core.Tests/AnalysisParameterSubstitutionTests.cs b/tests/PlanViewer.Core.Tests/AnalysisParameterSubstitutionTests.cs new file mode 100644 index 00000000..eebb6053 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/AnalysisParameterSubstitutionTests.cs @@ -0,0 +1,206 @@ +using System; +using System.IO; +using System.Text.Json; +using PlanViewer.App.Mcp; +using AnalyzerConfig = PlanViewer.Core.Models.AnalyzerConfig; +using CorePlanSession = PlanViewer.Core.Models.PlanSession; +using PlanViewer.Core.Output; +using PlanViewer.Core.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #482: #467 put the parameter values back at the two copy paths it was looking at, and the +/// reporter came straight back with "still shows parametrized in Human and Robot Advice". Both +/// advice buttons — and the HTML export, the comparison report, and every MCP tool — read their +/// statement text out of , so that is the seam these cover. +/// +/// The plan is the #466 reproduction: seven parameters the engine manufactured under +/// PARAMETERIZATION FORCED, six of them carrying a runtime value and the seventh only a +/// compiled one. +/// +public class AnalysisParameterSubstitutionTests +{ + private const string ParameterizedPlan = "forced_parameterization_plan.sqlplan"; + private const string NamedParameterPlan = "local_variable_plan.sqlplan"; + + /* Two of the seven. The first is a string that keeps its quotes, the second a numeric IN list + whose values arrive from showplan wrapped in parentheses. */ + private const string SubstitutedPredicate = "[t0].[AuthUserId]='123456'"; + private const string ParameterizedPredicate = "[t0].[AuthUserId]=@0"; + private const string SubstitutedInList = "[t].[StatusId] in (5,6,7,8,9)"; + + [Fact] + public void AdviceForHumans_ShowsTheValuesInsteadOfTheParameterNames() + { + var text = TextFormatter.Format(Analyze(ParameterizedPlan)); + + Assert.Contains(SubstitutedPredicate, text); + Assert.Contains(SubstitutedInList, text); + Assert.Contains("[t].[FromDateTime]>='2026-05-28 10:28:07.3132561'", text); + + /* Scoped to the predicate rather than a bare "@0" — the Parameters section below the + statement lists the names on purpose, and should keep doing so. */ + Assert.DoesNotContain(ParameterizedPredicate, text); + } + + [Fact] + public void AdviceForRobots_ShowsTheValuesAndStillCarriesTheParameterizedForm() + { + var json = JsonSerializer.Serialize(Analyze(ParameterizedPlan), AnalysisJson.Indented); + + using var document = JsonDocument.Parse(json); + var statement = document.RootElement.GetProperty("statements")[0]; + var runnable = statement.GetProperty("statement_text").GetString(); + var parameterized = statement.GetProperty("parameterized_statement_text").GetString(); + + Assert.NotNull(runnable); + Assert.NotNull(parameterized); + + Assert.Contains(SubstitutedPredicate, runnable); + Assert.DoesNotContain(ParameterizedPredicate, runnable); + + /* Both forms, and they say different things — the parameterized one is what matches the + plan cache and Query Store, and throwing it away to fix the advice would have been a + trade nobody asked for. */ + Assert.Contains(ParameterizedPredicate, parameterized); + Assert.NotEqual(runnable, parameterized); + } + + [Fact] + public void HtmlExport_ShowsTheValues() + { + var analysis = Analyze(ParameterizedPlan); + + var html = HtmlExporter.Export(analysis, TextFormatter.Format(analysis)); + + /* The IN list rather than the string predicate: the export HTML-encodes what it writes, and + an assertion carrying quotes would be testing HttpUtility rather than this change. */ + Assert.Contains(SubstitutedInList, html); + Assert.DoesNotContain("[t].[StatusId] in (@1,@2,@3,@4,@5)", html); + } + + [Fact] + public void ComparisonReport_ShowsTheValues() + { + var analysis = Analyze(ParameterizedPlan); + + var comparison = ComparisonFormatter.Compare(analysis, analysis, "before", "after"); + + Assert.Contains(SubstitutedPredicate, comparison); + Assert.DoesNotContain(ParameterizedPredicate, comparison); + } + + [Fact] + public void ComparisonStillPairsTwoRunsOfTheSameQueryWithDifferentParameterValues() + { + /* The reason this change had to be established before it was made. Substituting gives two + executions of one query two different statement texts, so if pairing had keyed on that + text, comparing a fast run against a slow one would have stopped matching them and + reported two unrelated statements instead of one regression. It pairs on QueryHash, which + is why substituting here is safe — and this fails the moment somebody changes that. */ + var slow = Analyze(ParameterizedPlan); + var fast = ResultMapper.Map( + ShowPlanParser.Parse(PlanXml(ParameterizedPlan).Replace("123456", "999999", StringComparison.Ordinal)), + ParameterizedPlan); + + Assert.NotEqual(slow.Statements[0].StatementText, fast.Statements[0].StatementText); + Assert.Equal(slow.Statements[0].QueryHash, fast.Statements[0].QueryHash); + + var comparison = ComparisonFormatter.Compare(slow, fast, "before", "after"); + + Assert.Contains("--- Statement 1 ---", comparison); + Assert.DoesNotContain("only in Plan", comparison); + } + + [Fact] + public async Task McpAnalyzePlan_HandsTheModelTheRunnableStatement() + { + /* The consumer that matters most: a model handed @0 with no values is being handed a + question it cannot answer. */ + var manager = new PlanSessionManager(); + var sessionId = $"mcp-{Guid.NewGuid():N}"; + manager.Register(new CorePlanSession + { + SessionId = sessionId, + Label = ParameterizedPlan, + Source = "file", + Plan = PlanTestHelper.LoadAndAnalyze(ParameterizedPlan) + }); + var operations = new PlanOperations(manager, AnalyzerConfig.Default, enforceQueryAdmission: false); + + var json = await McpPlanTools.AnalyzePlan( + manager, + operations, + sessionId, + TestContext.Current.CancellationToken); + + using var document = JsonDocument.Parse(json); + var statement = document.RootElement.GetProperty("statements")[0]; + Assert.Contains(SubstitutedPredicate, statement.GetProperty("statement_text").GetString()); + } + + [Fact] + public void McpReproScript_StillBuildsAParameterizedBody() + { + /* The one consumer that wants the other form. get_repro_script wraps this body in + sp_executesql with a parameter list read out of the same plan; a body with the literal + already inlined would declare a parameter it never uses, and would compile the + constant-folded plan rather than the parameterized one the script exists to reproduce. + + A named parameter rather than the forced-parameterization plan, because ReproScriptBuilder + requires a parameter name a human could have typed and drops @0 … @6 before it gets this + far — so that plan never reaches the sp_executesql branch this is about. */ + var manager = new PlanSessionManager(); + var sessionId = $"repro-{Guid.NewGuid():N}"; + manager.Register(new CorePlanSession + { + SessionId = sessionId, + Label = NamedParameterPlan, + Source = "file", + Plan = PlanTestHelper.LoadAndAnalyze(NamedParameterPlan) + }); + + var script = McpPlanTools.GetReproScript(manager, sessionId); + + Assert.Contains("sp_executesql", script); + Assert.Contains("@date datetime", script); + Assert.Contains("CreationDate >= @date", script); + Assert.DoesNotContain("CreationDate >= '2013-01-01 00:00:00.000'", script); + } + + [Fact] + public void AssignmentTargets_AreLeftAloneRatherThanTurnedIntoComparisons() + { + /* SELECT @job_name = name, @owner_sid = owner_sid FROM msdb.dbo.sysjobs_view WHERE + (job_id = @job_id) — both assigned variables carry a compiled value of NULL, and writing + it over them gives "SELECT NULL = name", which reads as a comparison. The parameter that + is actually read still gets its value. */ + var statement = Analyze("compile_memory_exceeded_plan.sqlplan").Statements[0]; + + Assert.Contains("@job_name = name", statement.StatementText); + Assert.Contains("@owner_sid = owner_sid", statement.StatementText); + Assert.DoesNotContain("NULL = name", statement.StatementText); + Assert.Contains("job_id = '846EFE14-2AEE-4A65-9EE0-213187F82250'", statement.StatementText); + } + + [Fact] + public void StatementWithNothingToSubstitute_KeepsItsTextAndCarriesNoSecondForm() + { + /* Nearly every plan. The mapped text has to stay byte-identical here, because the CLI's + JSON output is hashed as a contract against a plan of exactly this shape — a change in + these bytes is something to decide on, not to discover. */ + var plan = PlanTestHelper.LoadAndAnalyze("row_goal_plan.sqlplan"); + + var statement = ResultMapper.Map(plan, "row_goal_plan.sqlplan").Statements[0]; + + Assert.Equal(PlanTestHelper.FirstStatement(plan).StatementText, statement.StatementText); + Assert.Null(statement.ParameterizedStatementText); + } + + private static AnalysisResult Analyze(string planFile) => + ResultMapper.Map(PlanTestHelper.LoadAndAnalyze(planFile), planFile); + + private static string PlanXml(string planFile) => + File.ReadAllText(Path.Combine(AppContext.BaseDirectory, "Plans", planFile)); +} diff --git a/tests/PlanViewer.Core.Tests/ComparePlansAvailabilityTests.cs b/tests/PlanViewer.Core.Tests/ComparePlansAvailabilityTests.cs index b8b70f93..a3eaba3e 100644 --- a/tests/PlanViewer.Core.Tests/ComparePlansAvailabilityTests.cs +++ b/tests/PlanViewer.Core.Tests/ComparePlansAvailabilityTests.cs @@ -1,9 +1,12 @@ +using System.Collections.Generic; using System.IO; using System.Linq; using Avalonia.Controls; using Avalonia.Interactivity; +using Avalonia.Threading; using PlanViewer.App; using PlanViewer.App.Controls; +using PlanViewer.Core.Models; namespace PlanViewer.Core.Tests; @@ -14,13 +17,128 @@ namespace PlanViewer.Core.Tests; /// able to see across sessions. The reporter had to save a plan and reopen it to get at a comparison /// the app could already do. /// -/// The scenario is built from plan FILES rather than executed queries deliberately: getting a plan -/// into a session needs a live SQL Server, and the defect does not require one. What it requires is -/// plans existing somewhere OTHER than the session whose button is being judged, which two file tabs -/// provide exactly. +/// The first version of this file is why the issue was reopened. It built its scenario +/// out of plan FILES on purpose, reasoning that getting a plan into a session needed a live SQL +/// Server and that the defect did not require one. The second half of that was true and the first +/// half was the bug: a plan file opens a window-level tab, which is the one path that was already +/// recomputing. Every test passed against a build in which running two queries — the thing being +/// reported — still left both buttons dead. +/// +/// So the tests below drive the paths a plan actually arrives by. What still needs a server is +/// the round trip that produces plan XML, and only that: the landing step each path performs with +/// the XML in hand is reachable directly, and is where the whole defect lived. /// public class ComparePlansAvailabilityTests { + /// + /// The report, verbatim: a query run in one tab, another query run in a second tab, Compare + /// disabled in both. Driven through the same ShowCapturedPlan both execution paths use + /// once the server has answered. + /// + [Fact] + public void RunningAQueryInEachOfTwoSessionsOffersCompareInBoth() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + + var first = NewSession(window); + var second = NewSession(window); + + RunQuery(first, "row_goal_plan.sqlplan", "Plan 1"); + + Assert.All(new[] { first, second }, session => Assert.False( + CompareButton(session).IsEnabled, + "one plan cannot be compared against anything")); + + RunQuery(second, "key_lookup_plan.sqlplan", "Plan 1"); + + Assert.All(new[] { first, second }, session => Assert.True( + CompareButton(session).IsEnabled, + "a query has now run in each session, which is the whole of the report")); + }); + } + + /// + /// Two queries run in the SAME session, which is the case the pre-#449 arithmetic did handle. + /// Here because that arithmetic no longer exists — the count comes from the window now, and a + /// window-wide count has its own way of getting a single session wrong. + /// + [Fact] + public void RunningTwoQueriesInOneSessionOffersCompareThere() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + var session = NewSession(window); + + RunQuery(session, "row_goal_plan.sqlplan", "Plan 1"); + RunQuery(session, "key_lookup_plan.sqlplan", "Plan 2"); + + Assert.True(CompareButton(session).IsEnabled); + }); + } + + /// + /// The Query Store path, which opens its plans by adding tabs rather than filling one in, and a + /// plan tab being closed again. Both used to say so with a call written out at the site; they + /// now go through the same watcher as everything else, so they need pinning where they did not + /// before. + /// + [Fact] + public void QueryStorePlansOfferCompare_AndClosingOneTakesItAway() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + var session = NewSession(window); + + session.OnQueryStorePlansSelected(null, new List + { + QueryStorePlanFrom(11, "row_goal_plan.sqlplan"), + QueryStorePlanFrom(22, "key_lookup_plan.sqlplan") + }); + + Assert.True(CompareButton(session).IsEnabled, + "two Query Store plans are open in this session"); + + ClosePlanTab(session); + + Assert.False(CompareButton(session).IsEnabled, + "one of the two was closed, so there is nothing left to compare against"); + }); + } + + /// + /// Get Actual Plan on a window-level plan tab, which produces its plan the same way an executed + /// query does — into a tab that has been showing a spinner since the query was sent, and so is + /// not an addition to anything. + /// + [Fact] + public void AnActualPlanArrivingInAnExistingWindowTabIsNoticed() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + + window.LoadPlanFile(PlanPath("row_goal_plan.sqlplan")); + var session = NewSession(window); + + Assert.False(CompareButton(session).IsEnabled); + + var tabs = window.FindControl("MainTabControl")!; + var spinnerTab = new TabItem { Header = "Actual Plan", Content = new Grid() }; + tabs.Items.Add(spinnerTab); + + var viewer = new PlanViewerControl(); + Assert.True(viewer.LoadPlan(PlanXml("key_lookup_plan.sqlplan"), "Actual Plan")); + spinnerTab.Content = window.CreatePlanTabContent(viewer); + + Assert.True(CompareButton(session).IsEnabled, + "the window gained a second plan without gaining a tab"); + }); + } + [Fact] public void ASessionOffersCompareWhenThePlansAreElsewhereInTheWindow() { @@ -86,15 +204,144 @@ public void OpeningASecondPlanEnablesCompareInASessionThatAlreadyExisted() }); } + /// + /// A session that leaves for its own window leaves the window-wide count behind: detached, + /// it can only ever compare its own plans, and the sub-tab watcher that keeps the button + /// honest fires on sub-tab changes — of which a detach makes none. The button used to keep + /// its pre-detach answer until the next plan landed, offering a comparison whose second + /// plan was in a window it could no longer reach. + /// + [Fact] + public void DetachingASessionRecomputesCompareFromItsOwnPlans() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + + window.LoadPlanFile(PlanPath("row_goal_plan.sqlplan")); + var session = NewSession(window); + RunQuery(session, "key_lookup_plan.sqlplan", "Plan 1"); + + Assert.True(CompareButton(session).IsEnabled, + "docked, the window's plan and the session's own make a pair"); + + var tab = window.FindControl("MainTabControl")!.Items + .OfType().First(t => t.Content == session); + var detached = window.DetachTabToWindow(tab)!; + + Assert.False(CompareButton(session).IsEnabled, + "detached, the session holds one plan and the window's other one is out of reach"); + + /* And coming back is noticed without any hand-written call: re-docking adds a tab, + which is exactly what the window's collection watcher listens for. */ + RedockButton(detached).RaiseEvent(new RoutedEventArgs(Button.ClickEvent)); + Dispatcher.UIThread.RunJobs(); + + Assert.True(CompareButton(session).IsEnabled, "docked again, the pair is back"); + }); + } + + /// + /// The other direction of the same recompute: a session that owns a pair keeps its button + /// through a detach, because the honest own-plans answer is still yes. + /// + [Fact] + public void ADetachedSessionWithTwoOwnPlansKeepsCompare() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + var session = NewSession(window); + RunQuery(session, "row_goal_plan.sqlplan", "Plan 1"); + RunQuery(session, "key_lookup_plan.sqlplan", "Plan 2"); + + var tab = window.FindControl("MainTabControl")!.Items + .OfType().First(t => t.Content == session); + var detached = window.DetachTabToWindow(tab)!; + try + { + Assert.True(CompareButton(session).IsEnabled, + "both plans travelled with the session, so there is still a pair to offer"); + } + finally + { + // Nothing dirty, so the close takes the silent first-pass path. + detached.Close(); + Dispatcher.UIThread.RunJobs(); + } + }); + } + + private static Button RedockButton(Window detached) => + ((DockPanel)detached.Content!).Children.OfType().Single() + .Children.OfType