fix: make discovery REST mapper safe for concurrent use

[vroldanbet]

Summary

The discovery REST mapper used while filtering responses isn't safe for concurrent use. Under concurrent load it panics with a nil-pointer dereference, which surfaces to clients as an HTTP/2 INTERNAL_ERROR.

Commits:

1. fix: make the mapper concurrency-safe (with a test that reproduces the race).
2. perf: memoize lookups so the fix doesn't add a per-request cost — with the tradeoff worked through below.
3. test: an unrelated fix for a pre-existing Unit/e2e CI flake — embedded-SpiceDB metric double-registration (details below).
4. test(e2e): another unrelated pre-existing fix — flaky namespace-watch specs (details below).

The bug

toRestMapper returns a shortcut expander over a disk-cached discovery client. On every KindFor call the shortcut expander calls the discovery client directly, bypassing the DeferredDiscoveryRESTMapper's lock. When the on-disk cache is stale it gets rewritten, and the encoder briefly mutates the cached object's TypeMeta (sets the GVK for the wire, then restores it). Two concurrent KindFor calls race on that field, corrupt the APIVersion string, and panic in schema.ParseGroupVersion.

(Links point to k8s v0.34.1, the version this repo builds against.)

The mapper is shared across all request goroutines and KindFor runs while filtering every list/get response, so this is reachable under normal traffic.

Same class of bug as kubernetes/kubernetes#51521 (an Encode() that mutates its object), which was fixed only for the Unstructured codec. Discovery REST mappers aren't treated as concurrency-safe upstream — controller-runtime ships its own synchronized one — so the fix belongs at the caller.

Commit 1 — the fix

Serialize all access to the mapper with a mutex. The added test drives concurrent KindFor through the real discovery machinery and fails under -race without the wrapper.

Commit 2 — the optimization

Locking alone is correct but costly: the shortcut expander re-reads and JSON-decodes one discovery document per group-version on every KindFor, now serialized under the lock. The cost scales with the number of CRDs / API group-versions:

group-versions	lock-only / call	memoized / call
10	~0.24 ms	–
100	~1.15 ms	–
300	~3.36 ms (~1.3 MB, ~15k allocs)	~23 ns (0 allocs)

So we memoize successful GVR→GVK lookups; discovery is consulted only on a miss.

Why not cache forever? That would ignore the discovery cache's TTL — a GVK that changed at runtime (CRD reinstalled, version promoted) could stay pinned until the process restarts. Instead, the in-process cache uses the same TTL as the disk discovery cache (one shared value, so they can't drift) and caches only successful lookups (errors and unknown types are retried). Worst-case staleness stays the same order as today.

Commit 3 — unrelated CI fix (pre-existing flake)

This commit is not related to the REST mapper change; it fixes a separate, pre-existing flake that was failing the Unit and e2e jobs on this branch and on main.

The embedded SpiceDB helper builds an in-memory instance for tests. It already disables metrics on the dispatch and namespace caches, but the stored_schema cache was left at its default (Metrics: true), which registers Prometheus collectors on the global registry. When more than one embedded instance is created in a single test process, the second registration intermittently fails with duplicate metrics collector registration attempted. Setting Metrics: false builds that cache without registering, so the collision can't happen. (Surfaced by the SpiceDB v1.53.0 bump in #193.)

It's bundled here only because the flake blocks this PR's CI; it could equally land as its own PR.

Commit 4 — unrelated e2e flakiness fix (pre-existing)

Also not related to the REST mapper change. The "two users" namespace-watch e2e specs flaked because paul/chani are fixed identities across all specs and their namespace grants are never cleaned up (envtest has no namespace GC), so each user's cluster-scoped watch/list accumulates stale namespaces from earlier specs and the watch helper returns one of them. Fix: AfterEach now deletes the spec's namespace relationships so grants don't accumulate, and the list assertion is tightened to an exact ConsistOf — a robust leak check that fails on any cross-user namespace, current or stale. Also adds an assertion that a failed cross-user create leaves no dangling creator grant; it passes, confirming the dual-write rollback is correct. (Originally opened as #200, folded in here.)

Testing

-race reproduction of the data race; plus memoization, no-error-caching, and TTL-refresh tests. The scaling numbers above were measured with an ad-hoc benchmark (not committed). The two pre-existing flakes don't reproduce locally (timing/accumulation-dependent), so CI is the confirmation — Unit, e2e, Build, and Lint are all green (e2e confirmed green across multiple randomized orderings).

[codecov-commenter]

Codecov Report

❌ Patch coverage is 52.00000% with 24 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/proxy/restmapper.go	38.46%	24 Missing ⚠️

📢 Thoughts on this report? Let us know!

[ecordell]

LGTM, a couple of small nits but nothing that should block

[tstirrat15]

LGTM

[tstirrat15]

Should we be running under -race all of the time?

[vroldanbet]

yeah I don't think any reason not to do it (other than the feedback loop becomes slower)