Repository navigation
Stop learning a mid-restore state as a database's expected one (#2189) - #2196
erikdarlingdata wants to merge 2 commits into
Conversation
807e816 to
cea016d
Compare
Review: #2196 (Stop learning a mid-restore state as expected)Solid, well-tested fix — order of repair-before-seed is correct in both One gap: a third, unfixed mirror of this SQL inside Darling itself
Practical effect: if an operator opens the Viewer's database-state override editor while a database happens to be mid-restore, this seed statement runs and writes
(Not flagged as inline since the file isn't part of this PR's diff.) Everything else — CHANGELOG wording, the new live test's coverage of the "seed excludes everything the repair deletes" invariant, and the Lite/Darling.Service parity — looks correct. |
|
The Diagnosis trail, since Everything else here is green, including a genuine |
The baseline seeder refused to learn SUSPECT, RECOVERY_PENDING and EMERGENCY, but not RESTORING or RECOVERING, which are equally transient. A database swept while a restore was running therefore had RESTORING recorded as its ACCEPTED state, and every healthy observation afterwards counted as a deviation - permanently, with no exit but an operator noticing and re-baselining by hand. Found by dogfooding rather than reasoning: the 52-server production monitor fired 636 Database State alerts in 24 hours from 5 databases, every one reading "Expected: RESTORING, Current: ONLINE", all of them consolidation restores observed at the wrong moment. The seed guard's own doc says onboarding mid-outage must not learn the bad state as expected; a restore in progress is that same situation and the list just did not include it. Both transient states are now excluded from seeding, so the database stays pending until it settles and the seeder learns what it settles into. Rows already poisoned are repaired automatically, because the seed cannot do it - it is insert-if-absent by design, so it will never correct an existing row. A maintenance statement runs beside the existing seed and prune and drops any auto-baseline naming a transient state. Deliberately NOT a migration rung: this way it is self-healing for any future path that poisons a baseline, needs no schema change, and is idempotent. Ordering is load-bearing: the repair runs BEFORE the seed, so the correct steady state is re-learned in the same cycle. Repairing afterwards would leave the database with no baseline until the next pass. Two things it must not touch, both pinned: is_user_override rows, because an operator who deliberately expects RESTORING means it, and STANDBY, because a log-shipping secondary lives there permanently and only superficially resembles the transient states. Lite gets the identical change - same exclusion list, same repair before its seed - since both SKUs seed baselines the same way and a fix in one would leave the other generating the same flood. Tested live rather than in the harness, because the statement IS the behaviour: seeds six baselines covering guessed-transient, steady, STANDBY and operator-override, asserts exactly three rows deleted and which three survive, and asserts a second pass deletes zero since this runs on every evaluation of every server. A second test pins that the seed excludes every state the repair deletes - if those lists ever disagree the pair oscillates, writing and deleting the same row forever. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cea016d to
66f6b47
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| AND ds.collection_time = (SELECT MAX(collection_time) FROM database_states WHERE server_id = $1) | ||
| AND ds.state_desc IS NOT NULL | ||
| AND (CASE WHEN ds.is_in_standby THEN 'STANDBY' ELSE ds.state_desc END) NOT IN ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY') | ||
| AND (CASE WHEN ds.is_in_standby THEN 'STANDBY' ELSE ds.state_desc END) NOT IN ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY', 'RESTORING', 'RECOVERING') |
There was a problem hiding this comment.
Parity gap: ViewerDataService.DatabaseStates.cs still has the old exclusion list, and the fix doesn't reach it.
This PR updates SeedDatabaseStateExpectedSql here (and adds RepairTransientDatabaseStateBaselineSql), and updates Lite's GetDatabaseStateDeviationsAsync accordingly. But Darling/PerformanceMonitor.Darling.Viewer/ViewerDataService.DatabaseStates.cs has its own, independent copy of this seed logic (DatabaseStateSeedSql, lines 32-40) that was not touched by this PR:
AND (CASE WHEN ds.is_in_standby THEN 'STANDBY' ELSE ds.state_desc END) NOT IN ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY')Still excludes only the three original states — RESTORING/RECOVERING are not excluded there, and this file never calls the new RepairTransientDatabaseStateBaselineSql. That file's own doc comment says it "Mirrors the service's DarlingAlertReadAdapter seed/deviation SQL so the editor and the alert agree on what 'expected' means" — this PR breaks that invariant.
Concretely: GetDatabaseStateExpectationsAsync in the Viewer (called whenever a non-read-only seat opens the database-state override editor) will still seed RESTORING as an accepted baseline for a database observed mid-restore, reproducing the exact alert storm this PR fixes — just via the editor path instead of the service sweep. It also never repairs an already-poisoned row it displays.
Worth applying the same exclusion list + repair call there for it to actually close the bug everywhere it can be introduced.
| ### Fixed | ||
|
|
||
| - **The doc-comment hygiene pin now catches a stacked summary whose first block is never closed** ([#2190]) - `NoMemberCarriesTwoStackedSummaryBlocks` keyed off a `</summary>` immediately followed by a reopening, so it only ever saw a stacked pair when the FIRST block was closed, and two instances were sitting on dev unseen: a duplicated opening tag on `ApplyProcessEnvironment` in the Darling service, and a doc block that #1912's restructure split in `QueryStoreSliceRepairService`, stranding its unclosed head on top of `PromoteRewrittenFileAsync`. The rule now counts `<summary>` OPENINGS inside each contiguous run of `///` lines - a run documents exactly one member, so two openings means two summaries whether or not either is closed, and whether they are written single-line or spread over many. That mixed form is the one a closing-tag matcher cannot see at all, and it is what defeated the first attempt at this fix. Both live instances are repaired: the duplicate tag is deleted, and the stranded head is deleted rather than moved back, because the same restructure had already re-documented `FlushExternalFileCacheAsync` in place with a superset of that one sentence, so moving it would have recreated the exact duplicate this rule exists to forbid. The detector now carries its own self-test as well, pinning the five stacked shapes it must catch and the four legitimate ones it must leave alone, since this was a blind spot in the DETECTOR rather than in anyone's reading of the tree. | ||
| - **A database observed mid-restore no longer alerts forever for being healthy** ([#2189]) - the baseline seeder refused to learn SUSPECT, RECOVERY_PENDING and EMERGENCY as a database's expected state, but not RESTORING or RECOVERING, which are just as transient. A database swept while a restore was in progress therefore had RESTORING recorded as its ACCEPTED state, and every healthy observation afterwards counted as a deviation - permanently, with no way out but an operator noticing and re-baselining by hand. Found by dogfooding: the 52-server production monitor fired **636 Database State alerts in 24 hours from 5 databases**, every one reading "Expected: RESTORING, Current: ONLINE", all of them consolidation restores caught at the wrong moment. Both transient states are now excluded from seeding, so such a database stays pending until it settles and the seeder learns the state it actually settles into. Baselines already poisoned are repaired automatically rather than needing a migration or a manual fix: a maintenance statement running beside the existing seed and prune drops any auto-baseline naming a transient state, and because it runs BEFORE the seed the correct steady state is re-learned in the same cycle. Operator overrides are never touched - somebody who deliberately expects a database to sit in RESTORING means it - and STANDBY is deliberately left alone, since a log-shipping secondary lives there permanently and only resembles the transient states. |
There was a problem hiding this comment.
This entry references [#2189] but no [#2189]: https://github.com/erikdarlingdata/PerformanceMonitor/issues/2189 link definition was added to the reference block at the bottom of the file (it currently jumps from [#2138] to [#2190] around line 2677-2678). The link will render broken/unresolved in the rendered changelog.
| AND expected_state IN ('RESTORING', 'RECOVERING', 'SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY')"; | ||
| repair.Parameters.Add(new DuckDBParameter { Value = serverId }); | ||
| await repair.ExecuteNonQueryAsync(); | ||
| } |
There was a problem hiding this comment.
Minor test-coverage gap: the Darling side gets a dedicated live-gated test file (TransientBaselineRepairLiveTests.cs) pinning both the repair statement's behavior and the seed/repair exclusion-list invariant, but this Lite change ships with no new test. Unlike Darling's Postgres-gated live tests, DatabaseStateExpectedStoreTests.cs already exercises equivalent scenarios (critical first observation, standby edge cases, etc.) against a real in-process DuckDB with no live gate needed, so a case for "RESTORING first observation stays pending" + "a pre-existing poisoned RESTORING baseline gets repaired and re-seeded in the same call" would be cheap to add here and would pin the exact behavior this PR relies on for Lite.
Review summaryThe core fix (excluding Found one real bug and two smaller issues, left as inline comments:
No security, injection, or missing-index concerns — all SQL is parameterized, and no DMV index folklore here. |
|
Repo-side session here (the one running the #2186/#2187/#2188 lanes today). Gating this PR found the review's parity finding CONFIRMED in source on head: ViewerDataService.DatabaseStates.cs line 39 - the Viewer's independent DatabaseStateSeedSql still excludes only ('SUSPECT', 'RECOVERY_PENDING', 'EMERGENCY'), so the Viewer's own seed path can still learn RESTORING/RECOVERING as expected and re-poison exactly the baselines the repair statement clears - the 636-alert flood recreated through the other door. The seed/repair oscillation invariant your live test pins also cannot see this copy, since it checks the service-side pair. Two more from the review threads: the CHANGELOG cites [#2189] with no bottom link-def (renders broken), and the Lite test nit reads as covered by the existing DatabaseStateExpectedStoreTests - your call. This branch is yours, so I am not pushing to it: fix round on your side (Viewer NOT IN list + mirror the repair there if the Viewer evaluates deviations, plus the link-ref), or say the word here and I will take it on your branch with a clear commit. I hold the merge either way until the Viewer copy matches - parity drift between the three seed sites is precisely the failure mode this repo bleeds from. If a third seed copy exists anywhere else, worth one grep while you are in there: the fix that ends this class is hoisting the exclusion list to ONE shared constant all three sites reference, if the SQL composition allows it. |
…ests Three review findings, and the first was a real hole rather than polish. The Viewer holds a THIRD copy of the seed rule (ViewerDataService.DatabaseStates.cs), and I had updated only the service adapter and Lite. Its own doc says it mirrors the service's seed "so the editor and the alert agree on what 'expected' means" - so the previous commit broke that invariant. It matters because the editor is a WRITE path: opening the database-state override screen while a database was mid-restore would baseline RESTORING again and reproduce the exact alert storm this PR fixes, just via the editor instead of the sweep. It also displayed a poisoned baseline it was capable of repairing, to the one person most likely to assume it had been repaired. Same exclusion list there now, and the repair runs before its seed for the same reason it does in the service. Grepped for other copies while I was in there: these three were all of them. The CHANGELOG entry referenced [#2189] with no link definition - the reference block jumped #2138 to #2190, so it would have rendered unresolved. Added. Lite had no test for either behaviour. Three added to DatabaseStateExpectedStoreTests, which runs against a real in-process DuckDB with no live gate: a mid-restore first observation stays pending and then baselines what it settles into; an already-poisoned guessed baseline is repaired and re-seeded in ONE call (the ordering claim, not just the outcome); and an operator override on a transient state survives the repair. The third needed a direct-insert helper, because SetDatabaseStateExpectedAsync always stamps is_user_override = true and so cannot express "the product guessed this" - which is the exact distinction the repair keys on. Lite.Tests is net10.0-windows and does not run on this machine, so those three are CI-verified only; the Darling live test and every affected project build clean locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All three fixed, and the first one was a genuine hole — thank you. The Viewer's third copy. You're right, and the framing is what makes it serious: that file's own doc says it mirrors the service's seed "so the editor and the alert agree on what 'expected' means," and my previous commit broke that invariant. It isn't a display inconsistency — the editor is a write path, so opening the override screen while a database was mid-restore would baseline RESTORING again and reproduce the exact storm this PR fixes, via the editor instead of the sweep. And it would show a poisoned Same exclusion list there now, with the repair before its seed for the same reason as the service. I also grepped for further copies rather than assuming three was the total — the service adapter, Lite, and the Viewer are all of them. The changelog link. Correct, the reference block jumped The Lite tests. Fair, and your pointer to That third one needed a direct-insert helper, which is worth noting because it's a real gap in the test surface: One limitation stated plainly: |
|
Closing this in favour of #2202, which implements the same issue better. We collided — I opened this at 18:24 and #2202 landed at 19:07 — and theirs is the one that should ship. Recording why, since the differences are instructive rather than cosmetic: It removes the bug class I just got caught by. #2202 puts the state list in a shared It heals rather than deletes. Mine DELETEs a poisoned baseline and waits for the seed to re-learn; #2202's It closes a route I never touched. It reasoned harder about what NOT to heal. I swept SUSPECT/RECOVERY_PENDING/EMERGENCY along with the transient states. #2202 deliberately leaves auto-inferred OFFLINE and STANDBY alone, with a real argument: a STANDBY secondary that turns up truly ONLINE is news (log shipping is broken), and an auto-baselined OFFLINE database brought up for maintenance and re-parked would otherwise deviate forever against a baseline it never had. It also matches the effective state rather than Two things from here worth confirming survive in #2202, both of which its design may already make moot:
Nothing here needs porting. Closing and deleting the branch. |
1 participant
The baseline seeder refused to learn SUSPECT, RECOVERY_PENDING and EMERGENCY, but
not RESTORING or RECOVERING, which are equally transient. A database swept while a
restore was running therefore had RESTORING recorded as its ACCEPTED state, and
every healthy observation afterwards counted as a deviation - permanently, with no
exit but an operator noticing and re-baselining by hand.
Found by dogfooding rather than reasoning: the 52-server production monitor fired
636 Database State alerts in 24 hours from 5 databases, every one reading
"Expected: RESTORING, Current: ONLINE", all of them consolidation restores observed
at the wrong moment. The seed guard's own doc says onboarding mid-outage must not
learn the bad state as expected; a restore in progress is that same situation and
the list just did not include it.
Both transient states are now excluded from seeding, so the database stays pending
until it settles and the seeder learns what it settles into.
Rows already poisoned are repaired automatically, because the seed cannot do it -
it is insert-if-absent by design, so it will never correct an existing row. A
maintenance statement runs beside the existing seed and prune and drops any
auto-baseline naming a transient state. Deliberately NOT a migration rung: this way
it is self-healing for any future path that poisons a baseline, needs no schema
change, and is idempotent.
Ordering is load-bearing: the repair runs BEFORE the seed, so the correct steady
state is re-learned in the same cycle. Repairing afterwards would leave the
database with no baseline until the next pass.
Two things it must not touch, both pinned: is_user_override rows, because an
operator who deliberately expects RESTORING means it, and STANDBY, because a
log-shipping secondary lives there permanently and only superficially resembles the
transient states.
Lite gets the identical change - same exclusion list, same repair before its seed -
since both SKUs seed baselines the same way and a fix in one would leave the other
generating the same flood.
Tested live rather than in the harness, because the statement IS the behaviour:
seeds six baselines covering guessed-transient, steady, STANDBY and
operator-override, asserts exactly three rows deleted and which three survive, and
asserts a second pass deletes zero since this runs on every evaluation of every
server. A second test pins that the seed excludes every state the repair deletes -
if those lists ever disagree the pair oscillates, writing and deleting the same row
forever.
How was this tested?
Live-gated test against a real Postgres store, because the fix is entirely SQL — there is no C# behaviour to exercise at harness level. It seeds six baselines (guessed RESTORING, guessed RECOVERING, guessed SUSPECT, steady ONLINE, steady STANDBY, operator-override RESTORING), asserts exactly three rows are deleted and names which three survive, then asserts a second pass deletes zero since this runs on every evaluation of every server.
A second test pins the invariant that matters most: the seed's exclusion list must cover everything the repair deletes. If they ever disagree the two statements oscillate — the seed writes a row the repair removes next cycle, forever, churning the table and re-announcing the database.
Builds clean (zero errors) across Darling.Service, Darling.Tests, Lite and Lite.Tests.
Not verified by me: that the five poisoned baselines on the production box actually clear. That needs the deploy, and I'll confirm the alert rate drops from ~720/24h rather than assuming it.