Repository navigation
Lite re-fires a standing AG replica disconnect (#2426) - #2440
Conversation
|
Reviewed. This is a clean, well-tested piece of work — traced the new logic carefully and didn't find a correctness or parity bug worth flagging. Specifically verified:
No missing-index-DMV concerns (none present), no T-SQL in this diff, no secrets/injection surface touched. |
"AG Replica Disconnected" is a pure edge in Lite, so a replica that goes down and stays down is announced exactly once. In an alert history a week-long outage then reads identically to a momentary blip, and there is no setting anywhere in the app that changes that. Darling has had the answer since #1696 -- ag_disconnect_refire_minutes, 0 = off, clamped 0-1440 -- and #1726 mirrored three of Darling's four AG knobs into Lite and stopped, which was correct at the time because Darling's fourth landed later. The AG POLICY is already shared, so it would be easy to read this as supply-a-setting. It is not. AgAlertPolicy holds the pure decisions and each app deliberately owns its own edge STATE and its own delivery, and a re-fire is made of exactly the two halves Lite did not have: a clock per ag+replica, and something that consumes it. The setting is the small part; the state is the feature. The decision itself belongs in the shared policy rather than beside each app's state, and that is the one structural change here. Darling computed "still disconnected" as a five-clause expression next to its dictionaries; Lite's copy of that expression would have been the second one, and the clauses with sharp corners are not the arithmetic -- they are that a re-fire must never double up with the edge that just fired, and that an unrecognized connected_state_desc must not become a standing page any more than it may become a single one. Both apps now call one DecideConnection overload returning StillDisconnected, and what stays app-side is what genuinely is app-shaped: the stamp, and the delivery it is stamped on. That is also the answer to the question the issue left open about ConnectionRefireMinutes. The two re-fires are not one mechanism and should not be merged -- one is per server off a live connection probe, the other per ag+replica off a collected snapshot -- but the WINDOW is the same three-line predicate carrying the same two rules that are easy to misremember: a non-positive interval is off, and a missing stamp is due NOW rather than never-announced-so-stay-quiet. The second is what carries a standing outage across a restart, and it was about to be written a third time, so it is AlertRefireWindow now and ConnectionAlertPolicy calls it too. One departure from the AG family's first-sighting rule is deliberate, and is documented where the rule itself is stated: with re-fire ON, a replica already disconnected the first time it is seen announces rather than baselining silently. Both apps hold this state in memory, so every restart re-baselines, and under the silent-baseline rule alone a restart during a week-long outage would silence it permanently -- the opt-in would fail in precisely the case it exists for. Darling has behaved this way since #1696; stating it in the shared policy is the change, not the behavior. Lite's stamp is the caller's, beside the send, the same split its connection re-fire already uses. That is load-bearing rather than stylistic: Lite evaluates AG health on every sweep and skips delivery while a server is acknowledged or silenced, so stamping at decision time would spend window after window on alerts nobody received, and the operator would come back from an acknowledgement to silence rather than to a re-announcement. The #2417 exemption comes out with it. ag.disconnect_refire_minutes was exempted by name in McpAlertSettingsKeyTests with a test asserting both halves of the omission; both halves are now asserted the other way round -- Darling still emits it, and Lite emits it too, carrying its own live value rather than the constant 0 that was rejected for telling an agent it can tune something the app cannot. LiteOmittedMembers is empty, and the emptiness is asserted rather than merely true, so the next exemption still has to be justified here. Verified by compiling the real test files into a net10.0 xUnit-shim harness. The new tests cannot be expressed against dev at all -- the compiler names StillDisconnected, the five-argument DecideConnection, the three-argument EvaluateReplicas and NoteDelivered as missing -- which is the stronger statement. 66 cases pass against this branch. Both behavior-preservation claims were checked by running the UNCHANGED suites on each branch through the same harness: Darling's 36 AG self-alert tests and the 11 ConnectionAlertPolicy tests produce byte-identical results either side of the refactor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9326309 to
e21f26f
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| _utcNow()); | ||
| _agReplicaConnectedState[key] = replica.ConnectedStateDesc; | ||
|
|
||
| bool stillDisconnected = connection == AgConnectionDecision.StillDisconnected; |
There was a problem hiding this comment.
Lite/Darling parity drift introduced by this PR, even though the lines it lands in (868-880, just below this hunk) aren't themselves touched here.
Lite's new AgAlertEvaluator.EvaluateReplicas branches DetailText on decision == AgConnectionDecision.StillDisconnected, embedding "STILL DISCONNECTED ... (re-alerting every N min)" directly in the text that gets persisted to alert history and shown in the Alert Detail window.
Darling's FireAsync call just below only branches shortMessage on stillDisconnected (line ~875) — detail (line ~868) stays the generic "is DISCONNECTED from the primary..." string regardless of whether this is the first edge or the 6th re-fire. And shortMessage never reaches persisted storage: DarlingAlertDeliverer.DeliverAsync forwards outcome.DetailText (not ShortMessage) into SendAndRecordAsync → PgAlertHistoryStore, and EmailTemplateBuilder.BuildAlertEmail doesn't consume ShortMessage either (it's the interactive-host toast body per its own doc comment).
Net effect: an operator opening Darling's Alert Detail window for a re-fired "AG Replica Disconnected" alert sees byte-identical text to the very first disconnect notice — no indication it's a re-fire, no interval — while Lite's equivalent view now says so explicitly. That's exactly the "week-long outage reads identically to a momentary blip" problem this PR set out to fix, still open on the Darling side of the exact same feature.
Worth noting this is also inconsistent with Darling's own established pattern for the connection re-fire a few dozen lines above (ConnectionAlertDecision.StillDown => $"Still unreachable (re-alerting every {_connectionRefireMinutes()} min): {reason}"), which does bake the re-fire status into detail.
Suggest branching detail here the same way, e.g. appending $" (re-alerting every {refireMinutes} min)" when stillDisconnected is true.
There was a problem hiding this comment.
Verified all three legs and you are right on each: DeliverAsync forwards outcome.DetailText (never ShortMessage) into SendAndRecordAsync, EmailTemplateBuilder does not consume ShortMessage at all, and the connection re-fire a few hundred lines up already bakes it into detail. So Darling has been storing a re-fire that reads identically to the first notice since #1696, in the one view an incident actually gets reconstructed from — and this PR would have made Lite say it while Darling did not.
Fixed in the latest commit. The opening sentence now branches, worded to match Lite word for word, and the guidance after it is unchanged — so both SKUs store identical text for the edge and for the re-fire. shortMessage keeps its own wording.
Pinned in DarlingSelfAlertTests in both directions (the re-fire says it and names the interval; the opening edge does not), and that assertion is expressible against dev and red there: the re-fire's DetailText on dev is the generic disconnect notice.
Review summaryReviewed the AG-disconnect-refire feature (Lite catching up to Darling's #1696 knob, shared decision moved into What checks out:
One parity issue flagged inline: Lite's new code embeds the "STILL DISCONNECTED (re-alerting every N min)" wording into |
Darling's AG disconnect re-fire has branched its shortMessage since #1696 and its detail never. ShortMessage is the interactive toast body: it reaches neither the history row nor the email, because DarlingAlertDeliverer forwards DetailText into SendAndRecordAsync and EmailTemplateBuilder does not consume ShortMessage at all. So an operator opening Alert Detail on the sixth re-announcement of a week-long outage read text byte-identical to the first notice - which is exactly the "a week-long outage reads like a momentary blip" problem the knob exists to end, still standing in the one view an incident is actually reconstructed from. Review found it as a parity drift this PR introduced, and that is the fairer framing: Lite's new evaluator puts it in the detail because the detail is all Lite has, so the two SKUs' history rows would have disagreed about the same condition under the same metric name. Darling's own CONNECTION re-fire has baked it into detail since #1659 ("Still unreachable (re-alerting every N min)"), so the AG side was also inconsistent with its own sibling. The opening sentence now branches, worded to match Lite's word for word, and the guidance after it is unchanged - so the two apps' stored text is identical for both the edge and the re-fire. The assertion that pays for it is in DarlingSelfAlertTests and is expressible against dev, where it is red: the re-fire's DetailText there is the generic disconnect notice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed the diff against What I checked closely:
Nothing else stood out — no null-handling gaps, no thread-safety issues (Darling's |
Closes #2426.
AG Replica Disconnectedis a pure edge in Lite, so a replica that goes down and stays down is announced exactly once. A week-long outage reads identically to a momentary blip in the alert history, and no setting in the app changes that. Darling has hadag_disconnect_refire_minutessince #1696; #1726 mirrored three of Darling's four AG knobs into Lite and stopped, correctly at the time, because Darling's fourth landed later.This was not supply-a-setting
AgAlertPolicyis already shared, which makes it tempting to read the gap as a missing config key. It is not. The file's own discipline is that pure decisions live in the policy while each app owns its edge STATE and its delivery, and a re-fire is made of exactly the two halves Lite did not have: a clock per ag+replica, and something that consumes it. The setting is the small part.What moved into the shared policy, and why
Darling computed "still disconnected" as a five-clause expression sitting next to its dictionaries. Writing Lite's copy of that expression would have been the second one, and the clauses with sharp corners are not the arithmetic — they are that a re-fire must never double up with the edge that just fired, and that an unrecognized
connected_state_descmust not become a standing page any more than it may become a single one. Both apps now call oneDecideConnectionoverload that returnsStillDisconnected; what stays app-side is what genuinely is app-shaped, the stamp and the delivery it is stamped on.That is also the answer to the
ConnectionRefireMinutesquestion the issue left open. The two re-fires are not one mechanism and are not merged — one is per server off a live connection probe, the other per ag+replica off a collected snapshot. But the WINDOW is the same three-line predicate carrying the same two rules that are easy to misremember: a non-positive interval is off, and a missing stamp is due now rather than never-announced-so-stay-quiet. That second rule is what carries a standing outage across a restart, and it was about to be written a third time, so it isAlertRefireWindowandConnectionAlertPolicycalls it too.The one deliberate departure from rule 1
With re-fire ON, a replica already disconnected the first time it is seen announces rather than baselining silently. Both apps hold this state in memory, so every restart re-baselines; under the silent-baseline rule alone, a restart during a week-long outage would silence it permanently — the opt-in would fail in precisely the case it exists for. Darling has behaved this way since #1696. Stating it in the shared policy is the change, not the behavior.
Delivery-stamped, on purpose
Lite's stamp is the caller's, beside the send, matching the split its connection re-fire already uses. Lite evaluates AG health on every sweep and skips delivery while a server is acknowledged or silenced, so stamping at decision time would spend window after window on alerts nobody received, and the operator would come back from an acknowledgement to silence rather than to a re-announcement.
The #2417 exemption comes out with it
ag.disconnect_refire_minuteswas exempted by name inMcpAlertSettingsKeyTestswith a test asserting both halves of the omission. Both halves are now asserted the other way round: Darling still emits it, and Lite emits it too, carrying its own live value rather than the constant0that was rejected for telling an agent it can tune something the app cannot.LiteOmittedMembersis empty, and the emptiness is asserted in the parity test rather than merely being true, so the next exemption still has to be justified in that file.Verification
The Windows suites cannot run on macOS, so the real test files were compiled into a
net10.0xUnit-shim harness against the real projects.devat all. Compiling them againstdev's sources fails namingStillDisconnected, the five-argumentDecideConnection, the three-argumentEvaluateReplicasandNoteDeliveredas missing.DetailTextit actually stores. Against dev that reads the generic disconnect notice, byte-identical to the first announcement.AgAlertPolicyTests,AgAlertEvaluatorTests,ConnectionAlertPolicyTests), plus Darling's 36 AG self-alert tests.DarlingSelfAlertTests) and the 11ConnectionAlertPolicytests produce byte-identical results either side of the refactor.SettingsSampleTestsis satisfied:ag_disconnect_refire_minutesis documented inLite/config/settings.sample.jsonbeside the AG knobs it belongs with.Review round
claude[bot]found a parity drift this PR introduced, and it was real. Darling has branched only itsshortMessageon the AG re-fire since #1696, andshortMessageis the interactive toast body —DarlingAlertDelivererforwardsDetailTextintoSendAndRecordAsync, andEmailTemplateBuildernever consumesShortMessage— so the history row and the email for the sixth re-announcement of a week-long outage read byte-identical to the first notice. Lite's new evaluator puts it in the detail because the detail is all Lite has, so the two SKUs would have disagreed about the same condition under the same metric name. Darling's own connection re-fire has baked it intodetailsince #1659, so the AG side was inconsistent with its own sibling too. Fixed, worded to match Lite word for word so both apps store identical text, and pinned in both directions.🤖 Generated with Claude Code