fix: bug-bash batch 2 — six small correctness/hygiene fixes by erikdarlingdata · Pull Request #250 · erikdarlingdata/PerformanceStudio

erikdarlingdata · 2026-04-21T22:06:57Z

Summary

Batch 2 from the bug-bash review — bundles six fixes that are all a few lines each, no feature changes.

#	File	Fix
5	`ReproScriptBuilder.cs`	Double `]` in DB names so `USE [cool]]stuff]` is well-formed instead of unbalanced.
6	`ReproScriptBuilder.cs`	Whitelist `SET TRANSACTION ISOLATION LEVEL` against the five valid T-SQL names; unknown/hostile strings drop silently.
9	`ShowPlanParser.cs`	`ParseLong` now passes `NumberStyles.Integer` + `InvariantCulture`, matching the sibling `ParseDouble`.
10	`KeychainCredentialService.cs`	Read stdout+stderr concurrently to avoid the classic sync-stderr-while-stdout-async deadlock (bites on `security dump-keychain` with large keychains).
7	`QueryStoreHistoryWindow.axaml.cs`	`OnClosed` override cancels + disposes `_fetchCts` so closing mid-load doesn't leave the SqlConnection open on the server.
12	`QuerySessionControl.axaml.cs`	`DetachedFromVisualTree` also cancels `_statusClearCts` so the fire-and-forget status-clear dispatch can't touch a detached control.

Test plan

dotnet build PlanViewer.sln — 0 errors.
Add MCP server for AI-assisted plan and Query Store analysis #5 (USE escape) — runnable repro:
- databaseName = "cool]stuff" → emits USE [cool]]stuff]; ✅
- databaseName = "NormalDb" → emits USE [NormalDb]; (regression check) ✅
Add MCP server for AI-assisted plan analysis #6 (isolation whitelist) — runnable repro:
- "read committed" → emits SET TRANSACTION ISOLATION LEVEL READ COMMITTED; ✅
- "READ COMMITTED; DROP DATABASE [prod];--" → no SET line ✅
- "CHAOS" → no SET line ✅
Release v0.8.0 — MCP server, rename, release automation #9 (ParseLong culture) — runnable repro:
- Under de-DE, invariant-digit string "123456789012345" parses correctly ✅
- Overflow ("99999999999999999999") still returns 0 — documented limitation, deliberately not changed in this PR to keep the diff tight.
Rename to Performance Studio, add MCP docs and settings toggle #7, Fix release artifacts to match v0.7.0 format #10, Add auto-release workflow on dev-to-main merge #12 — code review only (require live UI / macOS keychain). Each change is local and behaviorally narrow.

🤖 Generated with Claude Code

- ReproScriptBuilder: double ']' in database names before emitting USE, so identifiers like `cool]stuff` produce valid T-SQL instead of unbalanced brackets. - ReproScriptBuilder: whitelist SET TRANSACTION ISOLATION LEVEL against the five valid T-SQL names; unknown/hostile strings drop silently instead of interpolating into the script. - ShowPlanParser.ParseLong: pass NumberStyles.Integer + InvariantCulture to match sibling ParseDouble; makes numeric parsing culture-independent. - KeychainCredentialService: read stdout and stderr concurrently via Task.WhenAll to avoid the classic "sync stderr while stdout is async" deadlock, which could surface on `security dump-keychain` with large keychains. - QueryStoreHistoryWindow: override OnClosed to cancel + dispose the in-flight fetch CancellationTokenSource, so SqlConnection doesn't sit open on the server when the dialog is closed mid-load. - QuerySessionControl: on DetachedFromVisualTree, cancel + dispose _statusClearCts alongside the existing TextMate teardown, so the fire-and-forget status-clear dispatch can't touch a dead control. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

erikdarlingdata

Review

What it does: Bug-bash batch 2 — five files, six narrow fixes: ]-doubling for USE [db], whitelist for SET TRANSACTION ISOLATION LEVEL, culture-invariant ParseLong, deadlock fix in security subprocess I/O, and cancel+dispose of two CancellationTokenSource fields on window close / control detach.

Good:

Targets dev ✓
Isolation-level whitelist uses the exact T-SQL-spec set with StringComparer.Ordinal + ToUpperInvariant() — correct.
]→]] doubling is the correct escape for bracket-quoted identifiers.
ParseLong now matches the ParseDouble sibling (NumberStyles.Integer + InvariantCulture) — no more de-DE surprises.
Keychain deadlock fix is real; switching stderr from sync ReadToEnd to async solves the classic pipe-buffer deadlock.
Both CTS-cleanup fixes (QueryStoreHistoryWindow.OnClosed, QuerySessionControl.DetachedFromVisualTree) follow the cancel-then-dispose-then-null pattern correctly.

Needs attention (non-blocking, none regress existing behavior):

QueryStoreHistoryWindow.LoadHistoryAsync still has a post-await UI-write path that doesn't check ct.IsCancellationRequested. If the fetch returns between OnClosed and await resumption, HistoryDataGrid.ItemsSource = … runs on a closed window. Pre-existing; flagged since you're touching this lifecycle.
KeychainCredentialService.RunSecurity ordering: Microsoft's documented pattern is Task.WhenAll(reads); WaitForExit(); rather than WaitForExit(); WaitAll(reads);. Works either way with both streams async, but worth aligning.
ReproScriptBuilder whitelist + ]-escape are both new security-hardening surface area with no coverage in tests/PlanViewer.Core.Tests/. Two xunit cases would pin the hostile-string-drops and bracket-escape behaviors.

Avalonia / theming: No AvaloniaEdit, no PlaceholderText/Watermark, no :pointerover selectors, no new brushes or MVVM creep — nothing to flag.

PlanAnalyzer: Not touched. No Dashboard/Lite sync required.

Noted this was merged seven seconds after opening — comments are for the record / follow-up.

Generated by Claude Code

erikdarlingdata · 2026-04-21T22:08:26Z

+        _fetchCts?.Dispose();
+        _fetchCts = null;
+        base.OnClosed(e);
+    }


Pre-existing, not introduced here, but worth flagging while you're in this file: LoadHistoryAsync (lines 191–239) catches OperationCanceledException but the success path still touches StatusText, HistoryDataGrid.ItemsSource, UpdateChart(), and PopulateLegendPanel() without checking ct.IsCancellationRequested. If the fetch returns normally in the tiny window between OnClosed firing and the await resuming, those UI writes happen on a closed window. Consider a ct.ThrowIfCancellationRequested() after the await (or before the UI writes) so the post-await path routes through the OperationCanceledException catch.

Generated by Claude Code

erikdarlingdata · 2026-04-21T22:08:31Z

        process.WaitForExit();
-        var stdout = stdoutTask.Result;
+        Task.WaitAll(stdoutTask, stderrTask);



Minor: Task.WaitAll(stdoutTask, stderrTask) after process.WaitForExit() is redundant — once the process has exited the pipes are closed and both ReadToEndAsync tasks will already be complete. Not a bug (it does guarantee exceptions surface rather than being lost on the task), just noting it. The Microsoft docs actually recommend the opposite order: start async reads, then WaitForExit (or WaitForExitAsync) — because WaitForExit without draining can itself block indefinitely in some shell scenarios. Current ordering works because both reads are async, but flipping to await Task.WhenAll(stdoutTask, stderrTask); process.WaitForExit(); would be the textbook pattern.

Generated by Claude Code

erikdarlingdata · 2026-04-21T22:08:38Z

+        "READ COMMITTED",
+        "REPEATABLE READ",
+        "SNAPSHOT",
+        "SERIALIZABLE",


Whitelist is correct per T-SQL spec — these are exactly the five values SET TRANSACTION ISOLATION LEVEL accepts. READ COMMITTED SNAPSHOT is deliberately absent and should be, since RCSI is a database-level option (ALTER DATABASE … SET READ_COMMITTED_SNAPSHOT ON) and not a transaction isolation level keyword. 👍

No tests in tests/PlanViewer.Core.Tests/ cover ReproScriptBuilder whitelist or ] escaping behavior — given this is now the security-hardening layer for a code-gen path, a couple of xunit cases pinning the two behaviors (valid level passes through, hostile string is silently dropped; cool]stuff → cool]]stuff) would be cheap insurance against regressions.

Generated by Claude Code

erikdarlingdata · 2026-04-21T22:08:43Z

        if (!string.IsNullOrEmpty(databaseName) && !isAzureSqlDb)
        {
-            sb.AppendLine($"USE [{databaseName}];");
+            sb.AppendLine($"USE [{databaseName.Replace("]", "]]")}];");


Escape is correct for the bracket-quoted identifier form. If anywhere else in this builder emits the database name without [...] brackets (e.g. as a plain token or inside a single-quoted string for sp_executesql), the ]→]] doubling alone isn't the right escape for those forms. A quick grep for other databaseName usages in this file confirms whether this is the only emission site.

Generated by Claude Code

erikdarlingdata · 2026-04-21T22:08:46Z

        if (string.IsNullOrEmpty(value)) return 0;
-        return long.TryParse(value, out var result) ? result : 0;
+        return long.TryParse(value, System.Globalization.NumberStyles.Integer,
+            System.Globalization.CultureInfo.InvariantCulture, out var result) ? result : 0;


Good alignment with ParseDouble. PR body notes the overflow case (99999…) still returns 0 — that's fine for ShowPlan's invariant-formatted payloads, just worth noting that silent-zero-on-overflow could mask a malformed plan. Not worth changing in this PR.

Generated by Claude Code

erikdarlingdata merged commit 719d861 into dev Apr 21, 2026
2 checks passed

erikdarlingdata deleted the fix/bug-bash-batch-2 branch April 21, 2026 22:07

erikdarlingdata commented Apr 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bug-bash batch 2 — six small correctness/hygiene fixes#250

fix: bug-bash batch 2 — six small correctness/hygiene fixes#250
erikdarlingdata merged 1 commit intodevfrom
fix/bug-bash-batch-2

erikdarlingdata commented Apr 21, 2026

Uh oh!

Uh oh!

erikdarlingdata left a comment

Uh oh!

erikdarlingdata Apr 21, 2026

Uh oh!

erikdarlingdata Apr 21, 2026

Uh oh!

erikdarlingdata Apr 21, 2026

Uh oh!

erikdarlingdata Apr 21, 2026

Uh oh!

erikdarlingdata Apr 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

erikdarlingdata commented Apr 21, 2026

Summary

Test plan

Uh oh!

Uh oh!

erikdarlingdata left a comment

Choose a reason for hiding this comment

Review

Uh oh!

erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

erikdarlingdata Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant