test(e2e): fix flaky namespace-watch specs (+ rollback guard) - #200
Closed
vroldanbet wants to merge 4 commits into
Closed
test(e2e): fix flaky namespace-watch specs (+ rollback guard)#200vroldanbet wants to merge 4 commits into
vroldanbet wants to merge 4 commits into
Conversation
The REST mapper returned by toRestMapper -- a shortcut expander over a disk-cached discovery client -- is not safe for concurrent use. On every KindFor call the shortcut expander calls the discovery client's ServerGroupsAndResources() directly, bypassing the DeferredDiscoveryRESTMapper's lock. When the on-disk discovery cache is stale, that path rewrites the cache, and the versioning codec does a read-modify-write on the cached object's TypeMeta (SetGroupVersionKind followed by a deferred restore). Two concurrent KindFor calls race on that field, tearing the APIVersion string and panicking with a nil-pointer dereference inside schema.ParseGroupVersion. The proxy shares a single mapper across all request goroutines and calls KindFor while filtering every list/get response, so this is reachable under normal concurrent traffic: the apiserver handler panics, HandleCrash re-raises it, and the HTTP/2 server resets the stream with INTERNAL_ERROR to the caller. Serialize all access to the mapper through a mutex. Adds a test that reproduces the data race; it fails under `-race` without the wrapper. Signed-off-by: Víctor Roldán Betancort <vroldanbet@authzed.com>
Serializing the mapper (previous commit) fixes the race but is expensive: the shortcut expander re-reads and JSON-decodes one discovery document per group-version on every KindFor call, and that now runs under the mapper's lock. The cost grows linearly with the number of CRDs / API group-versions, so it caps throughput on busy, CRD-heavy clusters. Measured cost (warm on-disk cache, per KindFor): group-versions lock-only memoized 10 ~0.24 ms - 100 ~1.15 ms - 300 ~3.36 ms ~23 ns (300: ~1.29 MB / ~15k allocs per call vs 0 B / 0 allocs) Memoize successful GVR->GVK lookups so the steady state is an O(1) map read; the delegate (and its discovery I/O) is consulted only on a miss. Design discussion / concern addressed: Caching indefinitely was considered and rejected. It would silently ignore the discovery cache's TTL: a GVK that changed at runtime (CRD reinstalled or a version promoted), or a transient inconsistent discovery result, would be pinned until the process restarted. The underlying disk discovery cache deliberately expires entries to bound exactly this staleness. Instead, the in-process cache carries the SAME TTL as the disk discovery cache. The value is threaded from a single source (newCachedRESTMapper's ttl) into both the disk client and this wrapper, so the two cannot drift. Worst-case staleness stays the same order as today's behavior rather than unbounded. Only successful lookups are cached; errors and not-yet-known resource types are retried. Adds tests for memoization, error pass-through (no negative caching), and TTL-based refresh. Signed-off-by: Víctor Roldán Betancort <vroldanbet@authzed.com>
The embedded SpiceDB helper (pkg/spicedb/spicedb.go) already disables metrics on the dispatch and namespace caches, but left the stored_schema cache at its default (Metrics: true). That cache registers Prometheus collectors named "stored_schema" on the global registry, so when more than one embedded instance is created in a single test process the second registration intermittently fails with "duplicate metrics collector registration attempted", flaking the Unit and e2e CI jobs. With Metrics: false the cache is built without registering anything (see CacheConfig.Complete in spicedb's server package), so multiple embedded instances can no longer collide. Unrelated to the REST mapper concurrency fix in this PR; included only because this flake was blocking CI on this branch (and on main). Signed-off-by: Víctor Roldán Betancort <vroldanbet@authzed.com>
vroldanbet
force-pushed
the
fix/e2e-watch-test-flakiness
branch
from
June 10, 2026 12:41
003ed12 to
f3b961b
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…llback The "two users" namespace-watch specs flake in CI: the cluster-scoped namespace watch/list returns stale namespaces left over from earlier specs. Root cause: paul and chani are the same identities across all specs, and each spec's namespace `creator`/`viewer` relationships are never cleaned up (envtest has no namespace GC, and AfterEach only orphan-deletes the namespaces). So a user's set of viewable namespaces grows every spec, and the cluster-scoped WatchNamespaces helper -- which returns the first event -- returns a stale namespace before the one the spec just created. Fix at the source: AfterEach now deletes the namespace relationships the spec created, so grants don't accumulate across specs (adds a DeleteAllTuples helper). With no stale state, each user sees exactly their own namespace, so the list assertion is tightened to an exact ConsistOf match -- a robust leak check that fails on any cross-user namespace, current or stale (the previous per-name checks could not catch a leaked stale namespace). WatchNamespaces is left as-is; cleanup alone makes it reliable. Also add an assertion to "recovers when there are kube write failures": after paul's failed create of chani's namespace, no `namespace:<ns>#creator@user:paul` relationship may remain (with `view = viewer + creator`, a dangling grant would let paul see chani's namespace). It passes in both lock modes -- the dual-write rollback is correct -- so it stands as a regression guard. Unrelated to the REST mapper race; this is pre-existing e2e test-harness flakiness, exposed once the embedded-SpiceDB metrics fix let the e2e suite run far enough to reach these specs. Signed-off-by: Víctor Roldán Betancort <vroldanbet@authzed.com>
vroldanbet
force-pushed
the
fix/e2e-watch-test-flakiness
branch
from
June 10, 2026 13:18
f3b961b to
cff1dd4
Compare
Contributor
Author
|
Folded into #199 (cherry-picked as |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes pre-existing flakiness in the e2e "two users" namespace-watch specs by cleaning up per-spec state at its source, and adds a regression guard for the dual-write rollback path.
Depends on #199 —⚠️ do not merge before #199. The e2e suite only runs once #199's embedded-SpiceDB metrics fix is present, so this PR currently carries #199's commits in its diff; once #199 merges to
main, this collapses to just the e2e change. (It targetsmainrather than #199's branch because the repo's Build & Test workflow only runs for PRs whose base ismain.) This is unrelated to #199's REST-mapper race — it's pre-existing e2e test-harness debt that #199 merely unmasked by letting the suite run far enough to reach these specs.Root cause
paulandchaniare the same identities across all specs, and each spec's namespacecreator/viewerrelationships are never cleaned up (envtest has no namespace GC, andAfterEachonly orphan-deletes the namespaces). So each user's set of viewable namespaces grows every spec. The cluster-scopedWatchNamespaceshelper returns the first event, so once stale namespaces have accumulated it returns one of them instead of the namespace the spec just created. (Only the namespace watch is affected —WatchPodsis scoped to a fresh per-spec namespace.)Fix (at the source)
AfterEachdeletes the namespace relationships the spec created, so grants don't accumulate across specs (newDeleteAllTupleshelper). The specs run sequentially (ginkgo, no-p/--procs), so this can't race another spec.ConsistOf— a robust leak check that fails on any cross-user namespace, current or stale. (An earlier attempt filtered the watch to the expected name, but that could hide a leaked stale namespace; cleanup + the list snapshot avoids that.)Rollback guard (investigation result)
A hypothesis from the CI logs was that
recovers when there are kube write failuresleaves a danglingcreatorgrant after paul's failed create of chani's namespace (which would let paul see it). I added an explicit assertion for that invariant. It passes in both lock modes — the dual-write rollback is correct — so it stands as a regression guard, not a bug fix.Testing
Affected specs pass locally (focused, k8s 1.33 envtest). The flake itself only reproduces under CI accumulation/ordering, so CI is the confirmation.