Merge branch 'fix/cached-telemetry-drain-hot-loop'

This commit is contained in:
Joseph Doherty
2026-07-27 15:50:42 -04:00
4 changed files with 328 additions and 28 deletions
@@ -1,8 +1,55 @@
# Cached-telemetry drain hot-loops forever on a row whose tracking snapshot is gone
**Date:** 2026-07-20 · **Status:** OPEN · **Severity:** Medium (log flood + wasted I/O; no data loss)
**Date:** 2026-07-20 · **Status:** RESOLVED (2026-07-20) · **Severity:** High — revised up from Medium on investigation (permanent stall of the cached-telemetry path, not just a log flood)
· **Area:** AuditLog / Site Telemetry
## Resolution (2026-07-20)
Fixed by letting an unresolvable row LEAVE the queue.
`SiteAuditTelemetryActor.OnCachedDrainAsync` now abandons the operational half of a cached row
whose tracking snapshot is still unresolvable after a grace period
(`SiteAuditTelemetryOptions.CachedTrackingGraceSeconds`, default **300 s**) and marks it
`Forwarded`. A row with no `CorrelationId` at all is abandoned immediately — it can never resolve.
**Marking `Forwarded` does not drop the audit data.** That state's role in this machine is "no
longer owed by the drain, still eligible for reconciliation", which is exactly the situation:
`ISiteAuditQueue.ReadPendingSinceAsync` covers `Forwarded` rows as well as `Pending` ones and
central dedups on `EventId`, so the reconciliation pull still delivers them. What is genuinely lost
is the operational (`SiteCalls`) half — unrecoverable regardless, since the tracking row it would
have been built from no longer exists.
Three further behaviours, each deliberate:
- **Inside the grace window the row is still retried.** A missing snapshot is normally a brief
write race (the audit row lands microseconds before the tracking row); abandoning on the first
failed lookup would discard the operational half of every cached call that lost that race.
- **A tracking-store THROW never abandons.** A throw is a store fault (locked, corrupt,
mid-restore), not a verdict about the row. Those rows stay `Pending` however old they are.
- **Logging is per drain pass, not per row.** One summary line each for abandoned, lookup-failed
(with the first exception attached) and deferred rows. The deferred case dropped to Debug — being
inside the grace window is normal operation, not a warning.
### Severity was revised UP during the fix
The original write-up called this a log flood with "no data loss", which understated it. The queue
is read **oldest-first with a fixed `BatchSize` (default 256)**, so once a batch's worth of
permanently-unresolvable rows collects at the head, every drain re-reads exactly those rows, fails
identically, and **never reaches the newer rows behind them**. The cached-telemetry path stalls
permanently. The audit half still reached central by reconciliation, so "no data loss" held — but
"Medium" did not.
**Fixed in:** `SiteAuditTelemetryActor.cs` (`AbandonUnresolvableAsync` + the rewritten skip
branches), `SiteAuditTelemetryOptions.cs` (`CachedTrackingGraceSeconds`).
**Tests:** `SiteAuditTelemetryActorTests``CachedDrain_OrphanRow_PastGrace_IsAbandoned_...`,
`..._InsideGrace_IsRetried_NotAbandoned`, `..._FullBatchOfUnresolvableRows_DoesNotStarveTheQueue`,
`..._TrackingStoreThrow_DoesNotAbandonTheRow`. The pre-existing
`CachedDrain_OrphanRow_NoTrackingSnapshot_IsSkipped_DoesNotCrash` was **retargeted, not deleted**
it had pinned the defect ("skipped and stays Pending"), so its orphan assertion is inverted while
the half that still holds is kept verbatim. Verified non-vacuous: with abandonment disabled the two
abandonment tests go red; the two must-NOT-abandon guards stay green in both directions.
The diagnosis below is retained as the record of how it was found.
## Summary
`SiteAuditTelemetryActor`'s cached-telemetry drain reads Pending audit rows, looks up each row's