feat: coalesce concurrent CIMD document fetches for the same client_id

[saucam]

What

Collapses concurrent CIMD document fetches for the same client_id into a single outbound fetch.

The gap

The resolution cache only helps once a fetch has completed. Until then every arriving request is a miss and starts its own fetch — so N simultaneous first-time requests for one client_id meant N DNS resolutions, N TLS handshakes, and N × up-to-5s of request occupancy, for a document that is byte-identical every time.

That multiplier was free to the caller and required no credential: client resolution runs ahead of the PrincipalResolver chain (#285), so an unauthenticated request reaches the fetch. Concurrency was the only input needed.

ResolveClient now uses golang.org/x/sync/singleflight; the fetch/validate/synthesize/cache body moves to resolveUncached so the callback stays readable.

Two details that are easy to get wrong

• The flight re-checks the cache before fetching. A fetch can complete between the outer miss and entering the flight; starting another would be duplicate work the flight itself cannot see.
• Each waiter gets its own clone. singleflight hands the same value to every caller, and callers receive a mutable *domain.OAuthClient — the same reason cachedResult already returns a copy. Sharing one instance would let any waiter mutate what the others hold.

Scope — what this does not fix

Stated plainly because it's easy to over-read: this bounds duplicate concurrent work for one client_id. It does not bound distinct-URL abuse. A caller cycling unique paths gets a fresh flight each time, misses the cache, walks past negative caching (which is per-URL), and churns the 1000-entry cache's eviction.

Only an edge rate limit closes that. docs/cimd.md now says so explicitly rather than as a trailing aside, and separates what each control actually bounds: the caps bound one fetch, coalescing bounds duplicate concurrent work, allowed_domains bounds who can aim it.

Tracked on the deployer side in highflame-cloud#2358.

Verification

• Mutation-checked. Bypassing the flight makes the new test report concurrent resolutions performed 8 fetches, want 1. A coalescing test that would pass anyway is worth nothing, so this matters more than the green run.
• The test's handler blocks until every caller has arrived, so an implementation that serialises rather than coalesces also fails — otherwise the first fetch could finish and warm the cache before the others start, and the test would pass for the wrong reason.
• Asserts waiters hold distinct *domain.OAuthClient values, and that mutating one caller's RedirectURIs doesn't change another's (catches a shallow clone).
• Passes under -race. Full internal/... unit suites green. golangci-lint: 0 issues.

Context

Found while reviewing why CIMD is disabled in Highflame production. The two blockers were this and the missing edge rate limit; with both addressed, an open-ecosystem CIMD deployment is defensible.

🤖 Generated with Claude Code

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	golang/​golang.org/​x/​sync@​v0.22.0

View full report

[adeinega]

As a couple of side notes... the database, or shared distributed caches are the safe places to store the CIMD Metadata. The reasons is simple - you might have multiple running instances of ZeroID. cache map[string]cimdCacheEntry is going to work well only within one (running) instance.

The CIMD specification, as of the time this comment was written, is currently in its second draft, and my guess is we're going to see changes in it.

[saucam]

Both good calls, thank you — and the first one changed how I'd frame this PR.

On the per-instance cache

You're right, and it's worth putting a number on it: prod AuthN autoscales 2–6 replicas, so cache map[string]cimdCacheEntry is 2–6 independent caches. The singleflight here only collapses concurrent fetches within one process; across replicas it does nothing, so the fan-out you're describing survives this PR entirely. I'll say that in the PR description rather than leave it implied.

One distinction I'd draw before we reach for shared storage, because I think it splits the problem in two:

A shared cache fixes fan-out and consistency. It does not fix staleness. Redis or Postgres would make all replicas consistently stale rather than inconsistently stale — nothing about shared storage makes a cached document fresher. The security-relevant property is revocation latency: a client that pulls a compromised redirect_uri out of its document wants that to take effect promptly. The only levers on that are the TTL and honouring the document's Cache-Control (ZeroID already takes the shorter of the two, floored at 60s).

So I'd treat them as separate decisions:

• Consistency across replicas → shared cache. Real property, worth wanting.
• Revocation latency → TTL. Independent of where the cache lives.

Today the inconsistency has a concrete edge: two replicas can serve different versions of one document for up to an hour, so a client's own remediation may or may not have taken effect depending on which pod they land on — and they can't tell which. That's the part I find least comfortable, and notably it's the part shared storage doesn't fix.

On medium, if we do go shared

I'd argue Redis over the database, fairly strongly. CIMD's defining property is that nothing is persisted — the synthesized client carries registration_source: cimd and never reaches a table. A DB-backed cache reintroduces a row per client_id and a write per resolution, which is the DCR bloat CIMD exists to remove, and puts a write on the /oauth2/authorize hot path. It would also recreate a registry-first shadowing hazard we just had to fix downstream in Studio, where a persisted row silently overrode the published document.

Redis is a much better fit — AuthN already has it for backchannel, quarantine and revocation, so no new dependency. One thing I'd want decided deliberately rather than as a side effect, though: what's being cached is redirect_uris, the load-bearing anti-impersonation control. An in-process cache is only poisonable by compromising the process; a shared cache is poisonable by anything that can write to Redis, and whoever writes it chooses where authorization codes get delivered. That's a real widening of Redis's blast radius. Solvable — signed entries, or a considered "Redis is in the TCB" decision — but I don't think it should ride in as an implementation detail of a caching change.

My honest read for right now: at 2–6 replicas the fan-out is a handful of extra fetches per hour, and the abuse case it amplifies is bounded by edge rate limiting on /oauth2/authorize (tracked in highflame-cloud#2358), which holds regardless of replica count. So I'd land this as the in-process improvement it is, and take shared caching as its own change with the poisoning question answered up front. Happy to be argued out of that ordering if you think the inconsistency window is the more pressing half.

On the draft moving

Agreed, and this PR now records where we stand — I've added a "Specification revision and deviations" section to docs/cimd.md covering the revision implemented against, the two places we're deliberately stricter than the draft, and what we've intentionally not built.

The strictness is the part your comment made me want written down. Rejecting a query string (draft says only SHOULD NOT) and requiring client_name (draft merely RECOMMENDS) are both defensible, but "stricter than the spec" ages badly in exactly the way you're describing: a later draft can bless something we refuse, and then we're rejecting valid clients for reasons nobody remembers choosing.

The failure mode I'd flag hardest is the discovery field name. If client_id_metadata_document_supported is renamed in a later draft, our advertisement silently stops being understood, clients fall back to DCR, and everything keeps working — no error anywhere. Our tests wouldn't catch it either, since they assert we emit that field; they'd stay green while no client could see it. We hit precisely that shape last week in a different component. The only real guard is tracking the draft, so the doc now says so explicitly rather than relying on someone remembering.

Two things that already make drift survivable, which I've also written down: DCR is retained as a deliberate fallback, and registry-first resolution means any client caught by a spec change can be pinned by registering it, overriding whatever its document says.

[saucam]

Pushed e71c8fd with the docs, plus one correction to this PR's own framing.

The PR description overstated what this fixes. It said coalescing bounds duplicate concurrent work, which is true, but it did not say that the bound is per process. With prod AuthN autoscaling 2–6 replicas, the fan-out you identified survives this change entirely — the singleflight collapses concurrent fetches within one process and does nothing across them. docs/cimd.md now says that in a deployment note rather than leaving it for a reader to infer.

The new Specification revision and deviations section records:

• the revision implemented against (draft-...-02, WG-adopted Oct 2025), stated as a draft that will change
• the two places we are deliberately stricter — rejecting a query string (draft: SHOULD NOT) and requiring client_name (draft: RECOMMENDED) — each with its reasoning and an explicit revisit on every draft bump
• what is deliberately not built (confidential clients, software_statement), which is also where the draft is likeliest to move
• the silent-failure case: a rename of client_id_metadata_document_supported produces no error anywhere, clients quietly fall back to DCR, and our tests stay green because they assert we emit the field
• what makes drift survivable — DCR retained as fallback, and registry-first resolution as a per-client pinning mechanism

Limitations / future work now links to those sections instead of restating them, so the two cannot drift apart. All intra-doc anchors verified.

I have deliberately not changed the caching design in this PR — it stays the in-process improvement it is, with the limitation documented. If you would rather the inconsistency window be closed before this lands, say so and I will pick up shared caching as its own change with the poisoning question settled first; I do not think it should ride in as a caching detail either way.