Add short-TTL bundle cache with singleflight de-duplication

[bdehamer]

Summary

Front the OCI bundle fetch with singleflight de-duplication and a short-TTL, digest-only in-memory cache, so that repeated and concurrent validations of the same image no longer each perform a full registry round-trip. This reduces the redundant registry load that can push attestation fetches past the admission webhook timeout.

Today Provider.Validate re-fetches bundles and re-runs verification on every request — even for an image validated moments earlier, and even when many identical requests arrive at once.

Failure modes this addresses

• Concurrent duplicate fetches during rollouts (thundering herd). When a workload rolls out, many pods are admitted within a short window and each admission triggers a separate, uncached fetch for the same image. That concurrent load adds latency at the registry and can push fetches past the admission webhook deadline, cancelling the request mid-fetch. → Singleflight collapses concurrent fetches for the same reference into a single upstream request whose result is shared with every waiter, removing the duplicate load that causes the latency.
• Redundant repeat validations. Gatekeeper validates the same long-running images repeatedly — at admission and during audit — re-fetching and re-verifying each time. → A short-TTL cache serves repeat validations of the same digest from memory.
• Wasted work when the caller has already timed out. If the admission request is cancelled at the webhook deadline, a fetch bound to that request's context is cancelled with it, so nothing is cached and the retry repeats the work. → The de-duplicated fetch runs detached from the caller's cancellation, so it completes and warms the cache for the retry that follows.

What this PR does

fetcher.CachingBundleFetcher decorates the OCI fetch with two mechanisms that have different correctness properties:

• Singleflight — keyed on the requested reference. Concurrent fetches for the same reference collapse into one shared upstream request, so a burst of identical admissions makes a single upstream call. This is the reliability fix for the herd. Coalescing shares the leader's result among the joined callers; for tag references that means sharing the leader's tag→digest resolution for the duration of one in-flight fetch — an intentional, bounded trade-off described under Design decision below.
• Short-TTL time cache — keyed on the resolved digest (digest-only). OCI tags are mutable: a tag can be repointed to a new digest within the TTL window. Serving a tag from a persisted entry could therefore return a verification result for a digest the tag no longer points to — an admission bypass. So the time cache only ever stores and serves digest references. Tag references are still de-duplicated by singleflight but are never time-cached; a successful tag fetch only warms a digest-keyed entry that a later by-digest request can reuse. This guarantee is independent of any deployment's tag conventions or registry behavior — it is gated purely on the reference type. Note it bounds image identity, not the attestation set: the referrer set for a digest is mutable, so a cached positive result can be up to one TTL stale with respect to attestations being added or removed (removal used as revocation is not reflected until expiry). "No attestations" results are not cached, so a newly-published attestation is picked up on the next validation.
• Detached shared fetch. A fetch that has already begun runs on context.WithoutCancel(ctx) so it completes and warms the cache even if the triggering caller's admission was cancelled mid-flight. A request that is already cancelled on arrival serves a cache hit if one exists but does not start a new fetch — so a timed-out multi-image Validate cannot spawn orphaned registry work. The inner fetcher's per-attempt timeout and retry budget still bound it.
• Bounded size. The cache is capped; when full, storing a new entry evicts the soonest-to-expire one (dependency-free).
• Verification is never cached. Failed fetches are not cached, and verification still runs on every call in Provider.Validate. The cache operates at the fetcher layer and returns {bundles, digest}, never a verdict — so cryptographic / trust-root revocation is re-checked on every hit. (Registry-side attestation removal is a referrer-set change and is only reflected once the entry expires — see the digest bullet above.)
• No shared mutable state across concurrent verifications. The cache stores bundles in their immutable serialized form and hands every consumer — each cache hit and each singleflight recipient — a freshly-unmarshalled copy, so concurrent validations of the same digest never share a *bundle.Bundle instance (sigstore-go memoizes verification state on the bundle receiver, which would otherwise be an unsynchronized write).

Design decision: tag references & singleflight (reviewed, intentional)

Singleflight is keyed on the requested reference, tags included. This was raised in review as a potential tag-mutation bypass; it is a deliberate, bounded trade-off, documented here so it isn't re-litigated:

• No verification result is ever persisted against a tag. The time cache is digest-only, so nothing keyed on a mutable tag survives beyond a single in-flight request.
• Singleflight only coalesces concurrent in-flight requests. Joiners share the leader's tag→digest resolution for the duration of one fetch (bounded by the fetch timeout). If a tag is repointed mid-flight, a joiner can receive the leader's digest instead of re-resolving on its own.
• That window is subsumed by the inherent admission→kubelet-pull TOCTOU. For any mutable-tag admission, the kubelet resolves the tag at pull time, independently of what the provider validated — so a tag that moves after admission already yields "validated A, running B" regardless of caching or singleflight. Coalescing only shifts the reference instant by at most one fetch duration, strictly inside that pre-existing gap; it does not change the security posture in kind.
• Restricting singleflight to digests was considered and rejected. It would close this narrow window but reintroduce the concurrent tag→digest resolve storm that is the dominant source of the fetch timeouts this change targets — resolving each tag independently is exactly the load we are trying to collapse.
• Strong binding (running image == validated image) is obtained by referencing images by digest, or by enforcing registry tag-immutability. That is independent of this cache and is the correct control for deployments that need it.

Configuration

• -bundle-cache-ttl — result cache TTL. Default 60s; 0 disables the time cache (singleflight de-duplication stays active).
• -bundle-cache-max-entries — maximum cached entries. Default 4096; 0 = unbounded. Negative values for either flag are rejected at startup.

Metrics

• aaop_bundle_cache_hits_total, aaop_bundle_cache_misses_total — digest lookups only; an errored upstream fetch counts as a miss.
• aaop_bundle_fetch_deduped_total — joined (non-leader) callers served by an in-flight singleflight leader.
• aaop_bundle_cache_entries — current cache size.
• aaop_bundle_cache_evictions_total — capacity evictions.

Scope and limitations

• The cache is per-process and in-memory. It de-duplicates within a single provider instance; it does not coordinate across replicas or clusters, so the more provider replicas a deployment runs, the thinner requests spread and the lower the per-instance hit rate.
• It is defense-in-depth, not a substitute for an adequate admission webhook timeoutSeconds. Where fetch latency under load is the root cause, raising the webhook timeout addresses the cancellation directly and independently of load; this PR reduces the load that produces that latency in the first place. The two are complementary.

Testing

• go build ./... ✅
• go test ./... -race ✅ — digest cache hit + TTL expiry; tag references are not served from the time cache (a moved tag re-fetches); concurrent tag references are singleflighted into a single fetch; singleflight still de-duplicates when the time cache is disabled (ttl=0); a digest with no attestations is not cached (a later-published attestation is seen on the next call); a request cancelled on arrival serves a cache hit but starts no fetch; every caller receives a distinct, freshly-unmarshalled bundle set (no shared *bundle.Bundle across concurrent verifications); serialize↔deserialize round-trips losslessly; errors are not cached; an errored upstream fetch counts as a miss; a mid-flight cancelled caller still warms the cache; max-entries eviction increments the evictions metric; negative flags are rejected; distinct references; idempotent Stop.
• golangci-lint run ./... ✅ 0 issues.

Open questions for reviewers

• Default values — -bundle-cache-ttl (60s) and -bundle-cache-max-entries (4096) are starting points; open to tuning.
• Singleflight is always on. The tag-coalescing trade-off is covered in Design decision above; let me know if you'd nonetheless prefer it gated behind a flag.

Follow-ups

• Expose the cache flags via the Helm chart, and keep the registry-fetch metrics (aaop_attestations_retrieved_*) uncontaminated by cache hits — tracked in Bundle cache follow-ups: expose cache flags via Helm chart; keep registry-fetch metrics uncontaminated by cache hits #202.

[Copilot]

Pull request overview

Adds in-memory OCI bundle caching and singleflight de-duplication to reduce registry load and admission latency.

Changes:

• Adds a bounded, digest-keyed TTL cache and concurrent fetch coalescing.
• Exposes cache configuration flags and Prometheus metrics.
• Adds cache behavior, cancellation, eviction, and concurrency tests.

Show a summary per file

File	Description
README.md	Documents caching behavior, configuration, and metrics.
pkg/metrics/prom.go	Defines cache and singleflight metrics.
pkg/fetcher/cache.go	Implements caching and fetch de-duplication.
pkg/fetcher/cache_test.go	Tests cache behavior and lifecycle.
go.mod	Promotes x/sync to a direct dependency.
cmd/aaop/aaop.go	Adds flags and integrates the caching fetcher.

Review details

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Suppressed comments (1)

pkg/fetcher/cache.go:263

• This gauge update can race with a store after the sweep unlocks, allowing the sweep's older count to overwrite the newer count. Publish the count before releasing c.mu to preserve mutation order.

	n := len(c.cache)
	c.mu.Unlock()
	metrics.BundleCacheEntries.Set(float64(n))

• Files reviewed: 6/6 changed files
• Comments generated: 5
• Review effort level: Balanced

[Copilot]

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

pkg/fetcher/cache.go:109

• An already-canceled request still reaches DoChan, whose detached closure starts a new registry fetch before the later select returns the cancellation. This is especially costly for multi-image requests: Provider.Validate continues its loop after each fetch error, so once the deadline expires every remaining key can launch orphaned work for the full retry budget. Return immediately when ctx.Err() is already set; only fetches that began while the request was live should be detached to finish.

This issue also appears on line 128 of the same file.

func (c *CachingBundleFetcher) BundleFromName(ctx context.Context, ref name.Reference, ro []remote.Option) ([]*bundle.Bundle, *v1.Hash, error) {

pkg/fetcher/cache.go:190

• This also caches a successful “no attestations” lookup for digest inputs: DoBundleFromName returns (nil, nil, nil) when remote.Referrers is empty (bundle.go:256-260), and this branch stores that result. The subject digest is immutable, but its OCI referrer set is not; if validation races attestation publication, subsequent validations keep returning image_unsigned for the full TTL after the attestation appears. Avoid negative-caching this mutable lookup (or give it a separately documented, shorter TTL).

			case isDigest:

pkg/fetcher/cache.go:130

• A cache hit returns the same type as an upstream fetch, so provider.go:133,151 records it in aaop_attestations_retrieved_timer and aaop_attestations_retrieved_total. The timer's Help and README define it as OCI-registry fetch latency/downloads; once hits dominate, its latency distribution no longer measures the registry and can mask the timeout signal this PR is meant to monitor. Propagate hit/upstream status (or observe registry metrics around the inner fetch) and update the metric contract accordingly.

		if e, ok := c.load(key); ok {
			metrics.BundleCacheHits.Inc()
			return e.bundles, e.hash, nil

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Balanced

[bdehamer]

Round-2 review disposition

Thanks for the second pass. Summary of how each finding was handled (three had no inline thread to reply to):

• D — negative-caching "no attestations": fixed in 2d98a17. Empty results (len(bundles) == 0) are no longer cached, so a newly-published attestation is picked up on the next validation instead of being pinned for a full TTL.
• C — cancelled-caller fetch guard: fixed in 2d98a17. An already-cancelled request still serves a cache hit if present, but no longer starts a detached fetch, so a timed-out multi-image Provider.Validate can't spawn orphaned registry work during a herd.
• B — referrer-set staleness: documented in 2d98a17 (README + the code comment at the digest gate) — digest-keying bounds image identity, not the attestation set; positive entries can be up to one TTL stale w.r.t. attestations being added/removed; re-verification checks the cryptographic material against the trust root, not the referrer set.
• A — Helm chart cannot configure the new flags: valid; tracked in Bundle cache follow-ups: expose cache flags via Helm chart; keep registry-fetch metrics uncontaminated by cache hits #202.
• E — registry-metric contamination: tracked in Bundle cache follow-ups: expose cache flags via Helm chart; keep registry-fetch metrics uncontaminated by cache hits #202.

Validation on 2d98a17: go build ./..., go test ./... -race, and golangci-lint run ./... all pass. Keeping the PR as a draft.

[Copilot]

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/fetcher/cache.go:183

• The second-chance lookup can return a cache entry after this invocation already incremented BundleCacheMisses at line 143, so that request is reported as a miss even though no upstream fetch occurs. This race is exactly why this lookup exists (another completed flight can populate the entry between the first lookup and DoChan). Carry whether the singleflight result came from this lookup and record hit versus miss only after the source is known.

		// Another caller may have populated the cache while this call queued.
		if timeCacheable {
			if e, ok := c.load(key); ok {
				return e, nil
			}

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[bdehamer]

Round-3 review disposition

• Data race in verification (pkg/fetcher/cache.go): fixed in f21a52b. The cache stored and returned shared *bundle.Bundle objects, which concurrent validations mutate inside sigstore-go v1.3.0 — Bundle.TlogEntries() memoizes hasInclusionPromise / hasInclusionProof on the receiver, and the PGI verify path calls it. The cache now stores the immutable serialized bytes and unmarshals a private bundle set per caller (cache-hit path, singleflight leader, and every joiner), so no two verifications share a mutable *Bundle. Added a -race concurrency test asserting distinct instances, plus a serialize→deserialize round-trip test.
• Second-chance miss mis-counting (cache.go, suppressed finding): also fixed in f21a52b. The second-chance cache lookup inside the singleflight closure could return a concurrently-populated entry after a miss had already been counted at the first lookup. The hit/miss decision is now deferred until the result source is known, so a request served by the second-chance lookup counts as a hit.

Validation on f21a52b: go build ./..., go test ./... -race, and golangci-lint run ./... all pass. Keeping the PR a draft.

[Copilot]

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/fetcher/cache_test.go:221

• This sleep does not guarantee that all follower goroutines have entered DoChan. On a loaded CI worker, a follower can be scheduled only after inner.block is closed; because tag results are not cached, that follower starts another upstream fetch and makes both the call-count and de-duplication assertions flaky. Use deterministic synchronization (for example, a test hook/barrier that confirms every follower has joined the flight) before releasing the fake fetcher.

This issue also appears on line 277 of the same file.

	time.Sleep(50 * time.Millisecond)

pkg/fetcher/cache_test.go:277

• The fixed sleep cannot ensure every goroutine has joined the in-flight call before the leader is released. If a goroutine is delayed past this point, ttl=0 means it cannot use a warmed cache and it will perform a second upstream call, intermittently failing the callCount() == 1 assertion. Replace the timing assumption with a deterministic barrier/test hook that confirms all followers are waiting on the flight.

	time.Sleep(50 * time.Millisecond)

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[piceri]

👍