perf(cache): coalesce in-flight pkgsite misses

[garrettladley]

• wraps cold CachedDoer.Do miss fetches in singleflight.Group keyed by the existing cacheKey
• returns buffered cachedResponse records from the in-flight group and rebuilds fresh *http.Response bodies per caller
• adds CacheOutcomeCoalesced and pkgsite.cache.coalesced.count while recording miss for the fetch leader and coalesced for waiters after singleflight returns
• covers concurrent success and error bursts in internal/pkgsite/transport/cache_test.go, including one upstream call, one cache write, and reusable response bodies

[greptile-apps]

Greptile Summary

This PR coalesces concurrent cold-cache misses for CachedDoer.Do using singleflight.Group, so only one upstream call is made per in-flight cache key, with the resulting cachedResponse record shared (and written to the KV store once) across all concurrent callers.

• internal/pkgsite/transport/cache.go: Upstream fetch and cache write are moved inside a singleflight.Do closure; responseFromRecord reconstructs a fresh *http.Response (independent Body reader, cloned headers) for each caller from the shared *cachedResponse; leader flag distinguishes the fetch-driving goroutine from waiters for metric tagging.
• internal/observability/cache.go: Adds CacheOutcomeCoalesced, a coalesced OTel counter, and an extra Count call in the metric sink for the coalesced outcome.
• internal/pkgsite/transport/cache_test.go: Two new concurrent tests (success burst and error burst) use a gated round-tripper and a counting store to assert one upstream call and one cache write across 50 concurrent callers.

Confidence Score: 3/5

The singleflight core logic is correct, but the cache-error fallthrough path emits two RecordCacheLookup calls per request, which will silently corrupt cache metrics in production.

The singleflight wiring, responseFromRecord, and observability additions are sound. The cache-error fallthrough path now calls RecordCacheLookup twice per request — once as CacheOutcomeError in the switch and once as CacheOutcomeMiss/Coalesced after the singleflight — silently corrupting cache metrics whenever the KV store is unreachable.

internal/pkgsite/transport/cache.go — specifically the interaction between the cache-error arm of the switch and the unconditional RecordCacheLookup call that follows the singleflight block.

Important Files Changed

Filename	Overview
internal/pkgsite/transport/cache.go	Wraps cold-miss upstream fetches in singleflight; introduces a double RecordCacheLookup call on the cache-error fallthrough path (P1), and silently propagates leader-context cancellation to all waiters (P2).
internal/observability/cache.go	Adds CacheOutcomeCoalesced constant, a new coalesced counter, and an extra Count call for the coalesced outcome in the metric sink; changes are straightforward and consistent with existing patterns.
internal/pkgsite/transport/cache_test.go	Adds two concurrent-coalescing tests (success and error bursts) backed by a gated round-tripper and counting store; synchronization logic is sound, though the window between store.Get returning and d.group.Do being called means coalescing is probabilistic rather than guaranteed in the test.

Sequence Diagram

sequenceDiagram
    participant A as Caller A (leader)
    participant B as Caller B (waiter)
    participant SF as singleflight.Group
    participant KV as kv.Store
    participant UP as Upstream HTTP

    A->>KV: Get(key) → ErrNotFound
    B->>KV: Get(key) → ErrNotFound
    A->>SF: Do(key, fn) — starts flight
    B->>SF: Do(key, fn) — waits on in-flight
    SF->>UP: client.Do(req) [leader context]
    UP-->>SF: 200 OK + body
    SF->>KV: Set(key, encoded, ttl)
    KV-->>SF: ok
    SF-->>A: "*cachedResponse, shared=true"
    SF-->>B: "*cachedResponse, shared=true"
    A->>A: responseFromRecord → fresh Body reader
    Note over A: RecordCacheLookup(miss)
    B->>B: responseFromRecord → fresh Body reader
    Note over B: RecordCacheLookup(coalesced)

Loading

Comments Outside Diff (1)

1. internal/pkgsite/transport/cache.go, line 79-86 (link)

   Double RecordCacheLookup on cache-error path

   When d.store.Get returns an error that is not kv.ErrNotFound, RecordCacheLookup(CacheOutcomeError, …) is called on line 80, and then code falls through to the singleflight block. After singleflight completes, RecordCacheLookup is called a second time with CacheOutcomeMiss or CacheOutcomeCoalesced (line 145). Every cache-store error therefore inflates both the pkgsite.cache.error and pkgsite.cache.miss counters, distorting all downstream metrics and dashboards for that path.

_{Reviews (1): Last reviewed commit: "perf(cache): coalesce in-flight pkgsite ..." | Re-trigger Greptile}

[greptile-apps]

Leader-context cancellation silently fails all waiters

The singleflight closure captures and uses req (the leader's request), so d.client.Do(req) and d.store.Set(req.Context(), …) both run under the leader's context. If the leader's caller disconnects before the upstream fetch finishes, the HTTP call is cancelled and every waiter receives that context error — even when their own contexts are still valid. This is a known singleflight trade-off, but the pattern is not documented here, so future callers may be surprised by it.