The daily digest and sweep rollup asked 'did I send' when the channel asks 'was it delivered': the once-a-day gate now stamps delivered-today in the store, so a restart re-announces nothing and a failed delivery still retries (#3580)

[erikdarlingdata]

The mechanism

Both daily documents — the collector-cost digest (#3443) and the fleet-sweep rollup (#3466) — gate on "once per day" through a ConcurrentDictionary<string, DateTime> under one fixed key, and compare now - lastSent < interval. That dictionary is process memory. A fresh process has an empty one, so the first hourly tick after ANY start reads "not sent today" and delivers both documents again, whatever the previous process delivered an hour earlier. The gate was answering "has THIS PROCESS sent one today" when the reader's question is "has one been DELIVERED today". Both docs said so out loud — "in-memory, like every sibling's interval, and a restart costs one extra digest" — and accepted it as the cheapest failure class.

The evidence

The v3.8.0 install night priced that acceptance: three stores restarted once each, and the dogfood channel carried six re-announcements among ~23 overnight posts — a quarter of the channel was the same two documents, twice. The same night showed the opposite case working correctly and worth keeping: a digest/rollup pair whose delivery had FAILED (a transport fault, not a suppression) was re-attempted after the restart and landed. The restart was the recovery. A failed delivery is not a delivery.

Why this shape

Gate on delivered-today, recorded in the store. On a delivery the deliverer does not report failed, the evaluator writes a stamp to collect.collector_state — the store's existing key/value state table (V44), so no migration rung — and reads it back before firing. The gate skips while now - stamp < interval, restart or not. A failed delivery writes nothing, so the next tick — restart or not — retries: the install-night recovery case is preserved by construction rather than by a special case.

The row's identity. server_id = 0 is the fleet sentinel the store already uses in two places — DarlingObservability.FleetServerId for the retention purge's collection_log run-record and FleetSweepStore.FleetScopeServerId for the sweep's fleet-scope watch items — both resting on the fact that server_ids are FNV-1a hashes and nothing is registered at 0; these two documents fire under the synthetic store label, not a server, so they take the same id rather than mint a third convention (and the column is NOT NULL in the PK, so a sentinel and not a NULL). collector_name = 'self_alert' is an owner name that is not a collector definition's — the QueryStoreBackfill precedent — so no declared-key read and no per-database prune in the runner can reach it; every prune there is scoped to a real server_id AND a collector's own name. Two keys: digest_delivered_at, sweep_rollup_delivered_at. The value is ISO 8601 round-trip text with its Z, so the read side gets Kind=Utc back honestly; updated_at is the naive-UTC write time, the column's meaning on every other row.

How the deliverer's answer reaches the fire site. IAlertDeliverer.DeliverAsync returns Task and its contract is "never throw for channel failures" — correct for every condition-class alert and deliberately kept. The two documents need the disposition, so IAlertDeliverer grows DeliverAndReportAsync, a default-implemented twin that returns the AlertDelivery the deliverer recorded (or null = unreported). DarlingAlertDeliverer reports the exact disposition its history row was written with, so what the caller is told and what the operator later reads in the alert log cannot disagree; DeliverAsync is that with the answer discarded, so there is one delivery path and not a reporting one beside a silent one. Lite's deliverer and every test fake keep the default and behave unchanged. The evaluator's FireAsync now returns the report; every condition-class caller ignores it.

What counts as delivered. The stamp is withheld on exactly one disposition, AlertDelivery.ChannelFailed — a channel was attempted and came back unsuccessful, which is the install night's recovery case. Everything else is either a delivery (Sent) or the product's own decision that nothing is owed: muted (a mute rule chose the silence), unconfigured (no channel exists; the history row IS the delivery, and retrying hourly would write 24 rows a day for nothing), throttled (a copy went out inside the cooldown), folded (reported on another delivery's roster). A null report — a deliverer that does not report — is "unreported", not "failed", and stamps: that is how every fire before this PR was treated, and a deliverer that knows a send failed says so. The rule is stated as one exclusion rather than an allow-list, so a disposition added later defaults to the quiet side and one meaning "not delivered and owed" has to be added by name.

Store seeds memory; memory serves the process. The in-memory dictionaries stay as the fast path but are no longer the authority — the shape #981 gave the email cooldown (GetLastEmailSentUtcAsync seeds it across restart). Memory answers whenever it holds anything: 23 of 24 ticks still cost one lookup and no store read, and once seeded, memory may also answer "outside the interval, deliver" without re-asking, because there is one writer per store and it is this process, so the store can never be newer than memory. The store is asked only when memory is empty — a fresh process, or one that has never seen a delivery succeed. That is also why the evaluate half's pre-check and the apply half's own check cost one store read between them, not two.

Fail-open, toward delivering, with reasons. A stamp READ that throws falls back to the memory gate, warns naming the document, and is counted into #3013's swallowed-read census under daily-document delivery-stamp self-alert (one site serves both documents, and the census keys a counted site on a literal name). Fail-open is the direction every store-fault posture in this evaluator already takes for its documents — the rollup's read fault "skips the tick WITHOUT consuming the interval" so an unreadable store cannot become a permanently quiet channel — and it is bounded: memory still gates within the process once one delivery lands, so a store that cannot answer costs at most one extra copy per process, the exact pre-#3580 posture and not a spam path. Failing CLOSED would let a store hiccup silence a daily document, the worse failure. A stamp WRITE that throws leaves the document delivered and the process gated (memory is stamped first), warns that the next restart will re-announce, and is exempt from the census as a write, the resolution-row precedent.

What it does NOT do

• It does not touch any condition-class self-alert: their lifecycle is edge-driven and owes nothing to a failed send, and FireAsync's return is ignored at every one of those sites.
• It adds no rung. collect.collector_state has existed since V44; two rows of exactly its shape.
• It does not change what throttled, folded, muted or unconfigured mean — it reads them.
• It does not make the deliverer report on the Per-event split path (N sends, no single disposition → null); neither document takes that path (Context: null).
• It does not read the alert history rows as the stamp. Considered: config_alert_log already carries alert_sent/notification_type per fire and seeds the cooldowns. Declined because history rows are a RECORD under retention, not state; because "delivered" would then have to be decoded in SQL from channel strings rather than decided at the fire site where the disposition is in hand; and because a stamp is one row, one key, one statement.
• The "first tick after restart is doing a lot at once" note in the issue (cold alert-read cache excursions) is out of this lane; this PR removes one of the things that tick was doing.

Load-bearing edits outside the named files

• PerformanceMonitor.Alerting/IAlertDeliverer.cs — the reporting twin (default-implemented).
• PerformanceMonitor.Alerting/AlertReadFailureCounter.cs — FleetScopedReads names the new counted read (the Lite.Tests inventory pin derives the set from source and requires the constant to name each).
• Darling/Darling.Tests/AlertMasterSwitchSurfaceTests.cs — DeliverAndReportAsync added to the delivery-seam census regex (the funnel now has 1 site under that name, the census counts stay exact); the FireAsync declaration exclusion allows a generic Task<…> return.
• Darling/Darling.Tests/AlertReadFailureSurfaceTests.cs — evaluator census 8→9 counted, 11→12 exempt; tree total 32→33 counted, 24→25 exempt, each with its sentence.
• Lite.Tests/AlertReadFailureSurfaceTests.cs — fleet-scoped read inventory 7→8.

Tests

SelfAlertDeliveryStampTests (new, 18 cases — every pin runs over BOTH documents through their apply seams, with a reporting deliverer built on the product's own AlertDelivery.FromFanout derivation and an in-memory stamp store shared between two evaluator instances, the "simulated restart"):

• delivered → stamp written at the evaluator's clock, Kind Utc → a NEW evaluator over the same store does not re-deliver inside the interval, asks the store once, then gates from memory, and delivers past the interval re-stamping at its own clock;
• failed → no stamp → the same process retries next tick, a fresh process retries, and the stamp appears when the retry lands, after which both processes are quiet;
• a stamp older than the interval delivers; one a minute inside gates, with no write;
• stamp read fault → falls back to memory, delivers once, warns naming the document, counted once under the literal read name, the write still lands, and the next tick is gated by memory without a second count;
• stamp write fault → delivered, process gated, warns, uncounted, and a fresh process re-announces (stated as the bounded cost);
• unreported (default interface method) stamps; muted stamps; no store = memory-only gate;
• row identity: sentinel equals FleetSweepStore.FleetScopeServerId and 0, owner name is no collector's and neither other worker-owned name, keys distinct, SQL schema-qualified with the PK conflict target; the worker hands the evaluator the store-backed stamps.

SelfAlertDeliveryStampStoreLivePostgresTests (new, DARLING_TEST_PG-gated, own scratch store): both statements parse against a migrated store; round-trip to the tick with Kind Utc; a Kind-Unspecified input reads back Utc; the upsert replaces (exactly two rows under the owner); the row sits at server_id 0 / self_alert with the Z in the value and a naive-UTC updated_at within a minute of now; a hand-poisoned value reads as no stamp with a warning rather than a throw.

Executed locally via the mactest pattern (a throwaway net10.0 xunit project with AssemblyName=Darling.Tests over the built WPF-free assemblies): SelfAlertDeliveryStampTests 18/18, FleetSweepRollupTests, CollectorCostDigestTests, DarlingSelfAlertTests, AlertMasterSwitchSurfaceTests, AlertReadFailureSurfaceTests, AlertEngineTests, DarlingAlertingTests, AlertFiringLogTests, FleetSweepEngineTests and the rest of the compiled set — 643 total, 0 real failures (6 CollectionLogStoreProbeAttributionTests cases fail only because that file resolves the repo root from the assembly directory, a harness artifact). The Lite.Tests inventory pin: 6/6. The live round-trip was executed against a real timescale/timescaledb:latest-pg17 container with PgMigrations.MigrateAsync run first — 1/1. Every touched project builds -c Release -p:EnableWindowsTargeting=true with 0 warnings (Service, Alerting, Darling.Tests, Lite, Lite.Tests).

Closes #3580

[claude]

The class remarks for this method state the invariant: "the evaluate half's pre-check and the apply half's own check cost one store read between them and not two — the first seeds, the second finds memory populated." That invariant only holds when the store read succeeds (returns a stamp or null for "no row").

When _deliveryStamps.GetDeliveredAtUtcAsync throws, the catch block here (below, in the method body) logs a warning, records one read failure via _readFailures?.RecordReadFailure(...), and returns false without populating lastDelivered. Both call sites are structured as "evaluate half calls this, and if it returns false falls through to eventually call the apply half, which calls this again" (EvaluateCollectorCostAsync → ApplyCollectorCostDigestAsync at lines 1237/1507; EvaluateFleetSweepRollupAsync → ApplyFleetSweepRollupAsync at lines 1700/1782). Since memory is never seeded on a thrown exception, the apply half's own check re-invokes the store read, which (for a persistent fault) throws again.

Net effect during a fault confined to collect.collector_state (e.g., lock contention, a slow/timing-out query on that one table while the rest of the store answers fine): every hourly tick logs two "delivery stamp could not be read" warnings and increments the #3013 swallowed-read counter twice, for what is really one logical failure — double the log noise and a 2x-inflated failure count in AlertReadFailureCounter, for as long as the fault persists. This contradicts the documented "one store read between them" cost model and isn't exercised by SelfAlertDeliveryStampTests, since those tests drive ApplyCollectorCostDigestAsync/ApplyFleetSweepRollupAsync directly rather than through the Evaluate*Async wrappers that chain into them.

Same null-no-row case too (a document that has literally never delivered, e.g. brand-new install): stamped is not DateTime also returns without caching, so the apply half hits the store again — this one's just an extra round trip, not a double failure count, but same root cause.

Worth either caching a "known unreadable this tick" sentinel (or reusing now/a local flag) so the apply half's check short-circuits once the evaluate half has already tried and failed, or accepting this as a known, bounded gap in the cost/count model and saying so in the doc comment.

[erikdarlingdata]

Taken, in e60fb26 — a real hole in the cost model, and the fix is the one you named, taken one step further. The gate now caches whatever the store answered on the first ask, INCLUDING "nothing": no row, or a read that threw, stores a NoDeliveryKnown sentinel (DateTime.MinValue, compared by identity before the interval arithmetic so it can never be mistaken for a stamp), so the store is asked exactly once per document per process. The apply half's own check, seconds after the evaluate half's, finds memory populated whether the store answered, was empty, or threw — one read, one warning, one count per fault per process.

Per process rather than per tick, because it is exact under the single-writer fact the doc already leans on: a store that had no row for this process's first tick cannot gain one except through this process, which populates memory directly when a delivery lands. Re-asking on a fault would buy nothing either — the fail-open delivery has already happened by then, and memory is stamped by it. The retry the install-night recovery case needs is unaffected: a document that has never delivered reads "deliver" from memory on every tick until one lands.

Pinned: AStampReadFault_IsMetOnce_PerProcess_EvenWhenNothingLands drives two consults on one tick with BOTH the stamp read and the delivery failing (so nothing but the sentinel can be what stops the second consult) — one store read, one warning line, one #3013 count, both attempts made; the next hour still one read; a fresh process meets the fault once more. The no-row arm rides AFailedDelivery_WritesNoStamp…, which now asserts one read per process across three processes. The doc paragraph is rewritten to state the once-per-process model and why it holds in the fault case.

[claude]

Reviewed the delivered-today stamping mechanism for the collector-cost digest and fleet-sweep rollup (Darling-only; no Lite parity gap since neither document exists on Lite). The core design is sound — memory-first gate seeded from a store stamp, write-skipped only on AlertDelivery.ChannelFailed, fail-open on store faults — and the new tests exercise the install-night scenarios directly.

1. Double-counted/double-logged store-read failures (Darling/PerformanceMonitor.Darling.Service/DarlingSelfAlertEvaluator.cs, DocumentDeliveredInsideIntervalAsync, ~line 1859): the method's own doc comment claims the evaluate-half pre-check and the apply-half's own check cost "one store read between them and not two — the first seeds, the second finds memory populated." That only holds when the store read succeeds. When GetDeliveredAtUtcAsync throws, the catch block returns false without populating lastDelivered, so the apply half (called right after the evaluate half falls through) re-invokes the same store read. For a fault confined to collect.collector_state, this doubles the "delivery stamp could not be read" warning and the #3013 swallowed-read counter increment per tick, for one logical failure — not exercised by SelfAlertDeliveryStampTests, which drives the Apply*Async methods directly rather than through the Evaluate*Async wrappers that chain into them. Left as an inline comment with detail.

Everything else checked out: parameterized SQL with no injection risk, schema-qualified collect.collector_state access, naive-UTC timestamp discipline followed correctly for updated_at, the fleet-sentinel/owner-name collision analysis against existing prune paths held up, and the AlertDelivery.FromFanout precedence confirms ChannelFailed is only reached when no channel actually delivered (so a partial email-success/webhook-failure doesn't wrongly withhold the stamp).

Remediated in e60fb26: the stamp store is asked once per document per process (empty/failed answers cached as a sentinel), so the apply half no longer re-reads, re-warns or re-counts a fault the evaluate half met on the same tick; pinned by a new two-consults-one-tick test. Fresh review runs on the new head.

[claude]

DeliverAndReportAsync is added with a default interface implementation, but CONTRIBUTING.md's "Two-Store Parity" section names this exact interface as one that must not get defaulted members:

Declare interface members as required, not defaulted. IAlertStateStore, IAlertEngineSettings and IAlertDeliverer give no member a default implementation, so adding one makes the compiler name every implementer and every test fake... A default implementation compiles and leaves the ones you forgot quietly doing nothing.

Lite/Services/LiteAlertDeliverer.cs implements IAlertDeliverer but does not override DeliverAndReportAsync — it silently inherits this default. The PR's own docs argue this is deliberate (Lite has no daily documents to gate), and the outcome is currently correct, but it's exactly the pattern the guideline calls out: nothing forces a future Lite maintainer to notice this member exists or decide whether Lite should ever report a disposition. Since the guideline names IAlertDeliverer by name, was this considered, or should LiteAlertDeliverer get an explicit (even if trivially => default-delegating) override so the compiler keeps enforcing the parity contract per convention?

Not blocking — the current behavior is correct and well-tested — but worth a maintainer call given the guideline speaks to this exact interface.

[erikdarlingdata]

Taken — you are right, and CONTRIBUTING names this interface by name, which I should have read before reaching for a default. Auto-merge fired on this LGTM before the remediation reached the branch, so it lands as the follow-up: #3640 makes DeliverAndReportAsync REQUIRED, gives LiteAlertDeliverer an explicit null with the reason (its send seam returns no disposition; it hosts neither daily document), and writes the answer into all fourteen fakes by hand — the DarlingAlertingTests wrapper forwards the inner deliverer's report rather than swallowing it. No behaviour change; the compiler now names the fifteenth implementer.

[claude]

Reviewed the diff (Darling-only: delivery-stamp store, evaluator gate rework, IAlertDeliverer.DeliverAndReportAsync, and the associated test/census updates).

Correctness. The gate logic in DocumentDeliveredInsideIntervalAsync / RecordDocumentDeliveredAsync is sound: the memory-cache sentinel (NoDeliveryKnown = DateTime.MinValue) correctly collapses "store had no row" and "store read faulted" into one no-op-retry state without re-querying the store on the same tick, the single-writer-per-store argument holds (no other process ever writes these rows), and the ChannelFailed-only exclusion for withholding the stamp matches AlertDelivery.FromFanout's taxonomy (verified against PerformanceMonitor.Notifications/AlertDelivery.cs — Sent, ChannelMuted, ChannelNoneConfigured, ChannelThrottled, ChannelFolded all correctly stamp; only ChannelFailed doesn't). Confirmed the two self-alerts never take the Per-event split path (Context: null at both fire sites), so DeliverAndReportAsync never returns null from "unreported" for them in production.

Store/schema. collect.collector_state's PK is (server_id, collector_name, state_key) (V44) — matches the ON CONFLICT target. server_id = 0 matches both existing fleet-sentinel users (DarlingObservability.FleetServerId, FleetSweepStore.FleetScopeServerId). collector_name = 'self_alert' doesn't collide with CollectorCatalog.All, QueryStoreBackfillState, or QueryStoreOpenIntervalState, and the per-database prune paths in DarlingCollectorRunner are all scoped to a real server_id, so this row is unreachable from retention/prune. Postgres timestamp handling follows the project convention (timestamp not timestamptz, DateTime.SpecifyKind(..., Unspecified) before binding updated_at). Parameters are all bound, no injection surface.

Lite/Darling parity. This feature (collector-cost digest, fleet-sweep rollup) is Darling-only pre-existing; Lite has no equivalent and none is needed. The shared surface touched (IAlertDeliverer, AlertReadFailureCounter) is updated correctly on both SKUs' shared layer, and Lite.Tests' inventory pin is bumped to match. Left one inline comment on IAlertDeliverer.cs: the new DeliverAndReportAsync member is default-implemented, which is the exact pattern CONTRIBUTING.md's "Two-Store Parity" section says not to do for this interface by name (IAlertStateStore, IAlertEngineSettings, IAlertDeliverer are called out explicitly as "no member a default implementation"). LiteAlertDeliverer silently inherits the default rather than declaring it. The PR's own docs treat this as deliberate and the current behavior is correct, so I'm not treating it as blocking, but flagging since the guideline names this interface specifically.

Tests. The new SelfAlertDeliveryStampTests (18 cases, both documents driven through the same table-driven Document fixture) and the live-Postgres round-trip test look thorough and exercise the fault-injection paths (ThrowOnRead, ThrowOnWrite) against the real fallback code, not a mirror of it.

No blocking findings.

[claude]

LGTM — reviewed the delivery-stamp gate logic (DocumentDeliveredInsideIntervalAsync/RecordDocumentDeliveredAsync), the AlertDelivery disposition handling, the Postgres store's schema/sentinel/prune-safety, and Lite/Darling parity. No blocking findings; one non-blocking CONTRIBUTING.md note left inline on IAlertDeliverer.cs about the default-implemented interface member.