Fixes #2923

[erikdarlingdata]

Fixes #2923

Two things, both test-only: a pin that closes #2923's hole, and the consolidation of BraceBalanced that the second copy created.

1. The hole

NoStoreRead_IsFiredByBothFleetTimers cuts OnRefreshTimerTick at its first return; and intersects only the region ABOVE it with OnOverviewTimerTick. Move the fan-out below that return and the region empties, the intersection empties with it, and the assertion is satisfied because the regression happened.

Measured, not assumed: that relocation leaves all four of ViewerFleetTimerGuardTests' facts green. Its Assert.True(firstReturn.Success, …) staleness guard cannot see it, because a relocation leaves the return; exactly where it was.

#2907 named that move as a regression in the same breath as the duplication the pin does catch — it "would stop the alert poll and the store-size read entirely while the Overview or a per-server tab is up" — and the Overview is the tab that ships selected.

What the new pin asserts

Darling/Darling.Tests/ViewerFleetTimerFanOutPositionTests.cs, three facts:

1. TheUnconditionalFanOut_IsFiredAboveTheTabEarlyReturn — every fire-and-forget in the tick has a call at an offset above the first return;. The set is derived from the tick (so a fan-out added later is covered without anyone remembering this file), with a floor of 4 so it cannot hold over nothing, and the three names Two viewer fleet-timer store reads have no in-flight guard, and the Overview timer double-fires one of them #2907 argues about asserted to still be in that set so a rename is loud rather than vacuously green.
2. TheVisibleTabRefresh_IsAwaitedBelowTheTabEarlyReturn — the other half of the ordering, and the reason the sibling pin's split means anything: RefreshVisibleAsync stays below. Above it, both timers refresh the same grid every cycle at one interval.
3. TheAboveTheReturnCheck_SeesARelocatedFanOut_OnlyThroughTheWalker — the control, in CI rather than in a commit message.

No product code changed. No schema rung, no version bump.

The walker, and being exact about what it buys

Read over CSharpSourceWalker.StripCommentsAndStrings (#2913/#2925) rather than a sixth private copy of the walk.

Being precise, because #2923 overstates this: the tick's comment-only mentions are ONE each of RefreshServerStatusAsync, PollAlertsAsync and RefreshVisibleAsync, none of RefreshStoreSizeAsync and none of return — the issue's 2/1/2/2/2 are the totals including the code. None is call-shaped, so raw and stripped agree on every call offset in this tick today and stripping does not change the current answer. What it buys is the next comment: those three mentions sit above the early-return, at offsets 328, 472 and 622, so one comment written PollAlertsAsync() instead of PollAlertsAsync turns prose into an above-the-return hit and the pin green on the regression. Fact 3 runs exactly that case both ways, and mutation 7 below confirms the honesty of the claim.

The control, since a count is invariant by construction

A relocation leaves every occurrence count where it was — one _ = RefreshServerStatusAsync(); before, one after — so an anchor tally reports the mutation as applied whether it applied or not. The control is the offset comparison itself: every failure message carries the offsets, each relocation mutation was verified by offset through an independent raw-text instrument (Python, deliberately not the pin's own walker, so the thing under test is not its own control), and Fact 3 carries the same comparison in CI.

2. BraceBalanced, consolidated

The new pin arrived with its own BraceBalanced, making two — one here, one in ViewerFleetTimerGuardTests. Brace-matching is only correct over the walker's output: braces in prose and in literals are exactly what unbalance it, which is already why blanking a hole's DELIMITERS is part of StripCommentsAndStrings' contract. So it now lives in CSharpSourceWalker as internal static, with both private copies deleted and all three call sites pointing at it. Same argument #2925 made for the walk itself.

They had diverged. Textually, not behaviourally: the newer copy fused the decrement into its condition (else if (c == '}' && --depth == 0)) where the older decrements inside the block. Rather than read them and conclude, they were run against each other at every offset of all 190 viewer .cs files — 2,836,304 positions, zero differences — with the differential harness positive-controlled against a third form that omits the decrement (reported as different, so the harness can see one). Kept the older expanded form: it is the one on dev, and it matches StatementSpanFrom's style in the file it moves into. The only behavioural change is the ArgumentNullException.ThrowIfNull the walker's three other entry points already carry; no caller passes null.

BodyAfterSignature deliberately stays put. It has one class's two call sites and no other caller in the repo, and its contract is specific to that pin's call-graph walk — it returns an expression-bodied member's expression without braces so the walk can follow RefreshAgAsync => LoadAsync();. Moving a single-caller helper would grow the shared surface without removing a duplicate, which is the opposite of the reason BraceBalanced moved.

RepoRoot is out of scope. 43 files in Darling.Tests declare their own walk-up helper (27 named exactly RepoRoot), so that one is settled convention rather than duplication.

Red-first evidence

Nine mutations proved the pin; all seven that bear on these two classes were re-run after the consolidation, each from a tree verified clean beforehand (pre-mutation dirty files: 0), each compiling Darling.Tests itself, each still failing the same assertion, with the count unmoved at 7.

#	Mutation	Control	Fails
1	remove RefreshServerStatusAsync's if (_statusRefreshInFlight) test	count 1→0	EveryFleetTimerFanOutTarget_IsSingleFlightOnEveryPathToAStoreRead
2	add _ = RefreshServerStatusAsync(); to OnOverviewTimerTick	count []→['RefreshServerStatusAsync']	NoStoreRead_IsFiredByBothFleetTimers
3	OnRefreshTimerTick passes replayIfBusy: true	count 1→2	OnlyANonPeriodicCaller_AsksTheStatusRefreshToReplay
4	_alertPollInFlight = false; out of its finally	count 1→0	EveryFleetTimerGuard_IsReleasedInAFinally
5	relocate the fan-out below the first return;	offset: ABOVE 2420 → BELOW 2312	TheUnconditionalFanOut_… only — all four old facts GREEN
6	await RefreshVisibleAsync(); above it	offset: BELOW 2420 → ABOVE 2457	TheVisibleTabRefresh_… + NoStoreRead_…
7	neuter the walker's strip to an identity	git diff +2, return text;	TheAboveTheReturnCheck_…

Two more from the original battery, not affected by the consolidation: renaming PollAlertsAsync across the viewer fails the name-staleness guard; deleting both early-returns fails the boundary guard in both pins (3 facts).

Mutation 5's message, and the offsets are byte-identical before and after the consolidation — which is how the move is shown behaviour-preserving through the pins rather than merely still-green, since the shared helper is what extracts that body:

OnRefreshTimerTick's first return; is at body offset 2344 and these fan-out calls are no longer above it, so they stop firing entirely while the Overview or a per-server tab is up — the regression #2907 named and #2923 found unguarded: PollAlertsAsync called at offset(s) 2852, all below 2344; RefreshServerStatusAsync called at offset(s) 2773, all below 2344; RefreshStoreSizeAsync called at offset(s) 2814, all below 2344

Mutations 1–4 are what discharges "the existing four facts still assert what they claim" — each still goes red on its own mutation, after the refactor as before it.

Mutation 7 also confirms the honesty above: with the strip neutered, facts 1 and 2 and all four existing facts stay green, so the walker is protecting the next comment rather than fixing today's answer.

Three earlier mutation attempts failed to apply and were reported as such rather than counted — await RefreshVisibleAsync(); is not a unique anchor in MainWindow.xaml.cs (5 at one indent, 2 at another). That is the silent-non-application failure mode; it was loud only because the driver asserts anchor uniqueness before every edit.

Verification scope

Darling.Tests is net10.0-windows and cannot run on macOS — CI is the arbiter. What ran locally: the real ViewerFleetTimerGuardTests.cs, CSharpSourceWalker.cs and the new file compiled from the repo by absolute path (never a copy) into a throwaway net10.0 xunit host with AssemblyName=Darling.Tests, so [CallerFilePath] resolves and the pins read the real viewer project. 7 tests, 4 existing + 3 new, 0 failed, each named in the run output, before and after the consolidation.

Because this PR edits CSharpSourceWalker.cs, a second host ran the pins that scan the tree: CSharpSourceWalkerTests (its per-construct witnesses and the repo-wide balance sweep), DocCommentHygieneTests and FleetIdentifierScrubTests — 31 tests, 0 failed, with the host's output directory relocated inside the repo so DocCommentHygieneTests' AppContext.BaseDirectory walk-up actually resolves and it genuinely scanned all three edited files. Darling.Tests.csproj and the Viewer project both build clean with EnableWindowsTargeting=true.

All four committed blobs are pure LF, no BOM, no stray CR; the working tree is CRLF per .gitattributes. CHANGELOG.md staged with a plain git add as 2 insertions, then 1 changed line.

🤖 Generated with Claude Code

[claude]

Reviewed this as a test-only change (no production code touched): Darling/Darling.Tests/ViewerFleetTimerFanOutPositionTests.cs + a CHANGELOG.md entry.

Correctness — traced the test logic against the real OnRefreshTimerTick/OnOverviewTimerTick in Darling/PerformanceMonitor.Darling.Viewer/MainWindow.xaml.cs:

• The floor of 4 fire-and-forget calls matches the shipped code (RefreshServerStatusAsync, RefreshStoreSizeAsync, PollAlertsAsync, conditional RefreshAvailabilityGroupsAsync).
• EarlyReturnOffset intentionally takes the first \breturn\s*; match, which lands on the Recommendations/Overview early-return (not the later per-server-tab return) — correct, since that's the boundary ViewerFleetTimerGuardTests.NoStoreRead_IsFiredByBothFleetTimers also splits on (same regex literal, \breturn\s*;, in both files).
• FanOutNotAbove's "any occurrence above split ⇒ not an offender" logic is deliberately permissive of a call appearing both above and below the split (documented as "ANY call shape counts as above"), which is consistent with its stated purpose.
• Walked through the control fact (TheAboveTheReturnCheck_SeesARelocatedFanOut_OnlyThroughTheWalker) by hand: on the raw fixture, the comment's call-shaped mentions (_ = PollAlertsAsync(); etc.) supply an above-the-split hit for each of the 3 named calls, so missed.Count == 0; on the stripped fixture those comment mentions are blanked, leaving only the relocated code occurrences (all below split), so seen.Count == 3. Matches what the test asserts.

Lite/Darling parity — no concern. This pins WPF-viewer-specific dual-timer behavior (MainWindow.xaml.cs's _refreshTimer/_overviewTimer) that only exists in Darling's multi-seat viewer architecture; Lite has no equivalent construct, so no counterpart change is expected.

Security / Performance — none applicable; this only reads known repo-relative file paths (MainWindow.xaml.cs via RepoRoot()/[CallerFilePath]) inside a test assembly.

One thing I checked but didn't flag as an issue: RepoRoot/BraceBalanced here duplicate the same-named private helpers in ViewerFleetTimerGuardTests.cs rather than sharing them (unlike the CSharpSourceWalker consolidation from #2913 called out in this same diff's CHANGELOG entry). Looking at the rest of Darling.Tests, though, per-file private copies of small utilities like these appear to be the established convention across the ~300 test files here (no shared TestHelpers-style file exists), so this isn't a deviation worth blocking on — just noting I considered it.

No correctness, parity, or security issues found.

[claude]

Reviewed. This PR is Darling-only test infrastructure (no T-SQL, no product code) closing #2923 by pinning the fan-out's position relative to the tab early-return, plus consolidating the two private BraceBalanced copies into CSharpSourceWalker.

Traced the new logic by hand against the actual MainWindow.xaml.cs source:

• EarlyReturnOffset in the new file uses the identical \breturn\s*; cut as NoStoreRead_IsFiredByBothFleetTimers in ViewerFleetTimerGuardTests, so the two pins agree on the boundary as the doc comment claims.
• FanOutNotAbove's "any occurrence above split → not an offender" logic is deliberately permissive about duplication (that's the sibling pin's job) and correctly scoped to a single brace-balanced method body, so it can't leak into OnOverviewTimerTick.
• The control test (TheAboveTheReturnCheck_SeesARelocatedFanOut_OnlyThroughTheWalker) genuinely demonstrates the walker's necessity: run raw, the fixture's comment prose (_ = PollAlertsAsync(); written in call-shape above the return) masks the relocation; run through CSharpSourceWalker.StripCommentsAndStrings, it doesn't. Verified this by tracing the regex matches against the fixture text by hand.
• CSharpSourceWalker.BraceBalanced is a faithful lift of the old private copy plus ArgumentNullException.ThrowIfNull, which is safe since no caller passes null.
• No Lite counterpart is needed — Lite has no fleet-timer/multi-tab construct analogous to this WPF viewer's dual DispatcherTimer setup, so this isn't a parity gap.
• CONTRIBUTING.md's C# conventions (XML doc comments, WHY-focused prose comments) are followed; no T-SQL files are touched so the T-SQL style rules don't apply here.

No correctness, security, or performance issues found. One purely cosmetic nit: the retained comment block in ViewerFleetTimerGuardTests.cs (around the old BraceBalanced removal) has one line noticeably longer than its neighbors' wrap width — not worth a fix on its own.

[claude]

Reviewed the diff (test-only: CSharpSourceWalker.cs, ViewerFleetTimerFanOutPositionTests.cs, ViewerFleetTimerGuardTests.cs, CHANGELOG.md).

Findings: none blocking.

What I checked:

• BraceBalanced consolidation into CSharpSourceWalker: the kept implementation is behaviorally identical to the deleted private copy (decrement-then-check-zero form), and all three call sites (ViewerFleetTimerGuardTests.cs ×2, the new file ×1) correctly reference the shared method — no orphaned private copy left that would fail to compile.
• Traced the new test's regex/offset logic against the actual MainWindow.xaml.cs: OnRefreshTimerTick has exactly the 4 fire-and-forget calls the floor check expects (status, store-size, alert-poll, AG-probe), all above the first return;, and RefreshVisibleAsync sits below both early-returns as TheVisibleTabRefresh_IsAwaitedBelowTheTabEarlyReturn requires. EarlyReturnOffset intentionally keys off the first return;, which matches the intended split point given the two early-returns in that method.
• The control test (TheAboveTheReturnCheck_SeesARelocatedFanOut_OnlyThroughTheWalker) fixture is correctly constructed: the comment-embedded call-shaped prose is only visible to FiredNames/CallOffsets on the raw path (blanked by StripCommentsAndStrings on the stripped path), so it genuinely demonstrates the walker's necessity rather than being vacuous.
• Lite/Darling parity: Lite has no CSharpSourceWalker, BraceBalanced, or equivalent dual-timer viewer construct — this is Darling-only WPF test infrastructure, so there's no missing Lite counterpart.
• No T-SQL, no security-sensitive surface (test-only file I/O against the repo's own tree via the existing RepoRoot()/[CallerFilePath] convention used elsewhere in Darling.Tests).

Nothing else stood out as a correctness, parity, or security concern.