Skip to content

The per-server collection-health memo keys on the window START and its caller passes a fresh UtcNow, so it never hits in production and its dictionary never evicts (defect in #3856/#3880) #3894

Closed

Description

@erikdarlingdata

Found while reading for #3893 arm 1, verified in source at d21bbc9a2. This is a defect in my own merged work (#3856 → c0eb0046e, stamped by #3880 → d5868c67d), and it has two consequences, one of which means that fix is currently inert.

The defect

DarlingDataReader.PerServerCollectionHealthMemo keys its dictionary on the window START:

  • Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingDataReader.cs:1853 — private readonly Dictionary<(int ServerId, DateTime WindowStartUtc), KeyState> _byKey = new();
  • :1891 — var key = (serverId, windowStartUtc);, no rounding.

Its only production caller hands it a fresh clock on every call:

  • Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpDataTools.cs:1007-1009 — var nowUtc = DateTime.UtcNow; then GetCollectionHealthMemoizedAsync(postgres, resolved.ServerId, nowUtc.AddDays(-7), nowUtc).

DateTime.UtcNow carries sub-microsecond precision, so nowUtc.AddDays(-7) is a different key on literally every call.

Consequence 1 — the memo never hits, so #3856 is inert

Every get_collection_health call misses the memo and starts its own 7-day scan of v_collection_log. That is precisely the race #3856 existed to stop: the per-server read crossing the mcp role's 15 s statement_timeout during the store's write bands (57014, first observed 2026-09-21 21:3xZ). The fix has been merged and reporting a collection_health_age_seconds stamp of 0 on every call — which is honest, and is also the tell nobody read: a memo that always reports age 0 is a memo that never hit.

Consequence 2 — unbounded growth in a long-lived host

_byKey is never pruned. Reading the whole class: TryGetValue, an assignment, and a second TryGetValue — no Remove, no Clear, no cap, no lifetime sweep. One KeyState (holding its rows list) accumulates per call, for the life of the MCP host process. The fleet twin does not have this exposure because it keeps a single _latest slot rather than a dictionary.

Why the pins passed

Both suites hold the key constant by construction, so they prove the mechanism works when handed the same key and never that production hands it the same key:

  • PerServerCollectionHealthMemoTests — unit arms reuse a fixed WindowFrom(T0) while advancing only nowUtc.
  • its live class (:456-466) — reuses now.AddDays(-7) as the window start while nowUtc advances by a second.

The missing pin is the one that exercises the production entry point twice with two fresh clocks and asserts ScansStarted == 1.

Fix shape

  1. Key on the window LENGTH, not its start — every caller means "seven days back from now", so the length is the stable part and the scan should cut scanNow - length itself. (This is also the shape The fleet collection-health rollup scans a fixed seven days regardless of the caller's window, and that scan doubled in two days — honour hours_back, then precompute it #3893's handoff independently concluded the fleet memo would need if its window ever moved, so the two memos end up honest in the same way.)
  2. Bound the dictionary — evict on lifetime expiry, or cap with an LRU. A per-server memo on a fleet-sized host should hold at most one entry per server per live window.
  3. Pin what the old pins could not see — a test that calls the production path twice with two real clocks and asserts one scan, plus an eviction/bound assertion. The existing key-constant arms stay; they are not wrong, only insufficient.

Ships with the next nightly regardless of this fix, so the inert-memo half is live on .458+ until it lands.

Activity

  1. erikdarlingdata commented on Sep 22, 2026

    @erikdarlingdata
    OwnerAuthor

    Claimed at filing (pm-user-worker seat) — this is my own defect from #3856/#3880 and I am fixing it now, ahead of #3893's arm 1, because the inert half is live on .458+ and the growth half runs in a long-lived host. Fix as described: key on window length, bound the dictionary, and add the pin that exercises the production entry point with two real clocks (the existing key-constant arms stay; they were insufficient, not wrong).

  2. erikdarlingdata commented on Sep 22, 2026

    @erikdarlingdata
    OwnerAuthor

    Closed by the watcher: delivered in PR #3900, merged to dev.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions