Make the viewer's fleet-timer freshness reads single-flight (#2907) - #2910
Conversation
OnRefreshTimerTick fired RefreshServerStatusAsync and RefreshStoreSizeAsync unawaited with no in-flight guard, while the two siblings in the same fan-out both had one. With the interval clamped to a 10s floor, a 15s per-read deadline and MaxPoolSize = 10, a slow read was joined by the next tick's rather than being the only one outstanding. The two guards are different shapes on purpose: store size takes no scope, so a dropped request is redundant and it gets _alertPollInFlight's plain drop. The freshness read is fetched before an add/edit/remove and applied to the fleet after it, so a server registered mid-read paints as never-collected - that caller gets the coalescing replay, opt-in per call site so the periodic callers still drop rather than deleting the interval's gap. OnOverviewTimerTick also stopped duplicating the fan-out, which ran two concurrent freshness read-pairs per cycle on the tab that ships selected.
|
Reviewed the diff ( Correctness — checked out clean:
Lite/Darling parity — no drift. This fan-out (fleet timers, Security / T-SQL style — N/A, no SQL or input-handling surface touched. Performance — this is a net improvement (eliminates stacked/duplicate store reads under load), no regression. No findings to flag. |
… right reason The pin claimed it reached a guard living one level down. A mutation aimed at that claim rather than at the fix - stripping AvailabilityGroupsTab's _loading - did not fail it, which is how the false negative surfaced. CalledNames only matched the fan-out's own two shapes (_ = X() and await X()), and RefreshAgAsync is => LoadAsync(): a plain call in an expression body, matching neither. The walk ran out of edges there and called the AG path safe without ever reaching the _loading that makes it safe. It now follows any invocation that resolves to a method this project declares, and reports RefreshAvailabilityGroupsAsync -> RefreshAgAsync -> LoadAsync when that guard is removed. Deciding which methods a tick FIRES still uses the narrow matcher - a tick's fan-out is its unawaited and awaited calls, not the loop conditions around them.
49c37be to
b2f2c52
Compare
|
Reviewed. This is Darling-viewer-only code ( Walked the new guard logic carefully:
No correctness, security, or performance issues found. The new |
…d-guards # Conflicts: # CHANGELOG.md
|
Reviewed the diff ( Correctness — Lite/Darling parity — Lite's Test file ( Security / SQL — no SQL or network/process/file boundary changes in this diff; nothing flagged. Comments and CHANGELOG entry follow the house style; XML docs are well-formed. |
1 participant
Fixes #2907
OnRefreshTimerTickfiredRefreshServerStatusAsyncandRefreshStoreSizeAsyncunawaited with no in-flight guard, while the two siblings in that same fan-out both had one (_alertPollInFlight, and_refreshInFlightplus its_refreshRequestedreplay). A read that outlived the interval was simply joined by the next tick's.Three numbers, all in the shipped code, make that expensive rather than untidy:
NocRefreshIntervalSeconds— default 30, clamped to a floor of 10 (ViewerAppSettings.cs:201);MaxPoolSize = 10deliberately (Cap the viewer's store pool + document the postgres.exe process anatomy (round-4 field follow-up) #1566), so read eleven waitsConnectionTimeoutSecondsfor a slot and then throws a connect error that misattributes a slow store to the network;RefreshServerStatusAsyncawaits a second read throughUpdateCollectorHealthTextAsync;PollAlertsAsyncawaits a third throughUpdateServerSilencedAsync. One tick is five store reads beforeRefreshVisibleAsyncstarts — six while the AG tab is still hidden, which is the ship default on a non-AG fleet.#2901 bounded each of these at
InteractiveReadSeconds(15 s) and deliberately left the guarding alone. That was the right split: a deadline caps how long a stacked read can hold a permit, which is a strictly smaller claim than not stacking.The two guards are different shapes on purpose
Not a style choice — a dropped request is recoverable in one case and not the other.
RefreshStoreSizeAsyncgets_alertPollInFlight's plain drop. It takes no scope: one store-widepg_database_size, one status-bar field. The in-flight read paints that field with an answer no staler than a fresh read's, so a dropped request is genuinely redundant rather than merely late, and no caller is left waiting on a value the running read will not produce.RefreshServerStatusAsyncgets the coalescing replay, opt-in per call site. The freshness dictionary is fetched before an add/edit/remove and applied to_fleet.Allafter it, so a server registered mid-read is simply absent from the dictionary and paints as never-collected. Dropping the connect/reload caller would leave a just-added server's dot wrong for a whole interval. But the replay is opt-in, because a periodic caller must still drop: the next tick is already its retry, and replaying one would delete the interval's gap exactly when the store is slowest, which is the state being fixed rather than a fix.The Overview double-fire
OnOverviewTimerTickstopped duplicating the fan-out. It ran at the same interval andOnRefreshTimerTick's Overview early-return sits below its fan-out, so with the Overview selected — the tab that ships selected — the viewer issued two concurrent freshness read-pairs per cycle.Moving
OnRefreshTimerTick's fan-out below the early-return instead would have been a regression: it would silence the alert poll and the store-size read entirely while the Overview or a per-server tab is up, which is exactly what "regardless of the visible tab" is there for. And once the guard exists the duplicate is worse than wasteful — a periodic second call can only be dropped, and one that asked to replay would guarantee the extra pass the guard exists to prevent.Comments corrected
Three, not two — the third is one #2901 left behind:
MainWindow.xaml.csfan-out: "a cheap pair of single-query reads" → the real count, and why all three are single-flight.MainWindow.xaml.cs_overviewTimer: "they never double-refresh the same grid" was true of the grid and false of these reads, because the early-return that guarantees it sits below the fan-out. Now says so narrowly.ViewerCommandDeadlines.InteractiveReadSeconds: its permit argument cited "an unguarded fleet-timer read can re-fire every 10 s", which was true when it landed and is not now. The ten-way panel fan-out is the binding permit argument on its own, and it is not a guarding problem — those ten reads are one deliberateTask.WhenAll, not an overlap.Pinning
ViewerFleetTimerGuardTestswalks the shipped source. The defect was never in any method's logic — every one of these methods was individually correct, and #2901 reviewed these same methods without the gap surfacing. What was wrong is that a fan-out site and a guard were written in two different places with nothing tying them together. There is also nowhere to stand a behavioural test up: this isMainWindowcode-behind on aDispatcherTimer, no seam, no injectable clock.The invariant: no path from a fleet-timer fan-out target to a
_dataServiceread may exist without a single-flight guard on it. The walk stops at the first guard it finds, which is what lets it be honest about a guard that legitimately lives one level down — the AG probe's isAvailabilityGroupsTab.LoadAsync's_loading, not inMainWindowat all. The guard is matched by shape, never by field name: the three that predate the pin are_refreshInFlight,_alertPollInFlightand_loading, so a name pattern would have needed an exception on day one.Stacking and leaking are checked separately, and that split is load-bearing. Folding the
finallyrelease into the stacking predicate looked tidier and was wrong twice over: it made the release assertion unreachable (it only ever saw bodies that had already satisfied the release check) and it pointed a leaked guard at the stacking message, sending the reader to look for a guard that is sitting right there. The first version of this pin had exactly that bug; the mutation run below is what found it.Proven red five ways, each mutation failing exactly one assertion with the message that names its own fix:
RefreshServerStatusAsync's guardRefreshServerStatusAsync -> ReadAndApplyServerStatusAsyncRefreshStoreSizeAsync's guardRefreshStoreSizeAsyncfinally_statusRefreshInFlight is not reset in a finallyfired by BOTH fleet timersreplayIfBusyturns a slow store into back-to-back readsThe first row is the one that matters most: the fix moves the freshness read into a helper, so a naive "does this method touch
_dataService" check would have excused an unguardedRefreshServerStatusAsync— it touches no store at all now. The walk descends and names the helper.Verification
PerformanceMonitor.Darling.ViewerandDarling.Testsboth build clean on macOS (EnableWindowsTargeting), 0 errors, no new warnings.[Fact]s were run against the real source by compiling the actual test file, unmodified into a throwawaynet10.0console harness with a ~30-line xUnit shim. That is the shipped assertion running on the shipped source, not a retyped copy. 4/4 pass; all five mutations above were run through the same harness.[CallerFilePath]repo-root walk resolves in CI's checkout.ViewerCommandTimeoutTestsalready relies on that same walk, so it should — but CI is the arbiter.Known adjacent gap, filed not fixed
UpdateCollectorHealthTextAsyncis reachable from two places — insideRefreshServerStatusAsync, and awaited directly byMainTabs_SelectionChangedbecause the health text's scope flips with the active tab. GuardingRefreshServerStatusAsyncdoes not serialize those against each other, and the method readsMainTabs.SelectedItembefore its await, so a tab switch during a timer pass can have the timer's copy repaint the pre-switch scope after the tab handler already painted the correct one. That is a last-writer-wins scope race, not read stacking; it is user-gesture-bounded rather than timer-driven; and it predates this change. Written up in #2907 rather than fixed here.