fix: hook up schema caching proxies with schemareader #2868

tstirrat15 · 2026-01-30T16:24:11Z

Description

In #2805 we refactored the caching proxies, and a part of that refactor made SchemaReader() return the underlying Reader instance rather than a reference to the current proxy. This meant that the proxy was effectively disabled for anything that interacted with schema reading/writing, because there wasn't a way to get a handle on the proxy that exercised the schema reading logic.

This remedies that by returning a wrapped version of the proxy that adds adapters for the new methods.

It also breaks out the LookupSchemaDefinitions's definition into two other functions that are caveat and typedef specific, which means that we can use the specific version at the callsite and prevent cache misses and overfetching.

Changes

Will annotate.

Testing

Review. Load-test this against the previous commit and see that the schema cache is actually populated and used.

tstirrat15

See comments

tstirrat15 · 2026-01-30T16:25:31Z

internal/datastore/proxy/schemacaching/standardcache.go

+// that caching logic stays in place.
 func (r *definitionCachingReader) SchemaReader() (datastore.SchemaReader, error) {
-	return r.Reader.SchemaReader()
+	return schemautil.NewLegacySchemaReaderAdapter(r), nil


I took the expedient approach in this file under the understanding that this code will be completely discarded once new schema storage is in place.

this code will be completely discarded

this entire file?

tstirrat15 · 2026-01-30T16:26:16Z

internal/datastore/proxy/schemacaching/watchingcache.go

 	p   *watchingCachingProxy
 }

+var _ datastore.SchemaReader = (*watchingCachingReader)(nil)


I was slightly more thorough in this file under the understanding that the new schema storage mechanism will reuse some of this logic.

tstirrat15 · 2026-01-30T16:27:20Z

internal/datastore/proxy/schemacaching/watchingcache.go

+
+// ListAllTypeDefinitions delegates to the underlying reader, as the cache doesn't currently
+// support reading all definitions directly.
+// TODO: implement this in the cache


Question for reviewers: does this seem sane? Listing all will bypass the cache, but I don't have a sense of whether that's actually used in places where caching matters.

tstirrat15 · 2026-01-30T16:28:08Z

internal/datastore/proxy/schemacaching/watchingcache.go

+
+// LookupSchemaDefinitionsByNames looks up type and caveat definitions by name.
+// TODO: implement this directly in the cache, rather than overfetching from the caches
+func (w *watchingCachingReader) LookupSchemaDefinitionsByNames(ctx context.Context, names []string) (map[string]datastore.SchemaDefinition, error) {


This function is slightly inefficient because it's always going to make twice as many requests as necessary, but the caches should hold a notFound after their first request so it should be amortized (?).

codecov · 2026-01-30T16:29:23Z

Codecov Report

❌ Patch coverage is 70.00000% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.39%. Comparing base (c684353) to head (0aabd07).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/datastore/schema/schema.go	70.00%	4 Missing and 2 partials ⚠️
internal/relationships/validation.go	66.67%	4 Missing and 2 partials ⚠️
internal/services/v1/experimental.go	60.00%	4 Missing and 2 partials ⚠️
internal/services/v1/permissions.go	60.00%	4 Missing and 2 partials ⚠️

❌ Your project status has failed because the head coverage (74.39%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2868      +/-   ##
==========================================
- Coverage   74.43%   74.39%   -0.04%     
==========================================
  Files         484      484              
  Lines       57764    57792      +28     
==========================================
- Hits        42991    42987       -4     
- Misses      11754    11779      +25     
- Partials     3019     3026       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tstirrat15

See comments

tstirrat15 · 2026-02-02T13:34:37Z

internal/datastore/proxy/schemacaching/standardcaching_test.go

 }

-type testerDef struct {
+type oldTesterDef struct {


Renaming here was mostly about ensuring that I didn't miss a test.

tstirrat15 · 2026-02-02T13:35:39Z

internal/datastore/proxy/schemacaching/standardcaching_test.go


-func TestRWTCaching(t *testing.T) {
-	for _, tester := range testers {
+func TestSnapshotCaching(t *testing.T) {


Most of the refactors in these two files look roughly like this - flattening the tables and refactoring to use the new methods.

tstirrat15 · 2026-02-02T13:36:29Z

internal/datastore/proxy/schemacaching/standardcaching_test.go

+
+	oneReader := &proxy_test.MockReader{}
+	dsMock.On("SnapshotReader", one).Return(oneReader)
+	oneReader.On("LegacyReadNamespaceByName", nsA).Return(nil, old, nil).Once()


Note that the mock still references the legacy method - this is because the wrapper is just invoking the legacy method.

tstirrat15 · 2026-02-02T13:38:04Z

internal/datastore/proxy/schemacaching/standardcaching_test.go

+		dsMock.On("SnapshotReader", one).Return(oneReader)
+		oneReader.
+			On("LegacyReadNamespaceByName", nsA).
+			WaitUntil(time.After(50*time.Millisecond)).


Using synctest here to avoid needing to actually sleep for the given period of time.

tstirrat15 · 2026-02-02T13:38:44Z

internal/datastore/proxy/schemacaching/standardcaching_test.go

+
+// singleflightReader is used to test singleflight context cancellation
+// behavior.
+type singleflightReader struct {


Renaming this to make its use a bit clearer.

nit: don't name things based on how they are expected to be used, name them after what they do. in this case, it's a slow reader that reacts to context cancellation

tstirrat15 · 2026-02-02T13:45:05Z

internal/datastore/proxy/schemacaching/standardcaching_test.go

 }

+// TODO
+func TestMixedCaching(t *testing.T) {


This is the test whose semantics differ the most from the test it's based on.

Most of it comes down to the LookupSchemaDefinitionsByNames function taking namespace or caveat names as args, which means that it needs to look up the names as both caveats and namespaces.

Because the caches don't hold negative results, you can't make the assertion that the lookup function doesn't go to the underlying reader, because the lookup function is always going to be looking in the wrong place for a name half of the time.

I was able to remedy this by adding separate Lookup methods that return just caveats and namespaces.

tstirrat15 · 2026-02-02T13:46:07Z

internal/datastore/proxy/schemacaching/watchingcache.go

 }

-func (r *watchingCachingReader) LegacyReadNamespaceByName(
+func (w *watchingCachingReader) LegacyReadNamespaceByName(


This bugged me so I updated it.

tstirrat15 · 2026-02-02T13:47:11Z

internal/datastore/proxy/schemacaching/watchingcache_test.go

+	require.NoError(t, err)
+	require.Equal(t, "somenamespace", nsRevDef.Definition.Name)
+
+	// NOTE: we don't read via lookup because we can't retain the semantic of the test


This test's semantic also differs from the previous one, for the same reason as previously - we can't assert that the Lookup doesn't go to the underlying datastore because it's always going to miss the cache half the time.

internal/datastore/proxy/schemacaching/standardcaching_test.go

miparnisari · 2026-02-02T18:50:38Z

internal/datastore/proxy/schemacaching/standardcaching_test.go

+		go readCaveat()
+		go readCaveat()
+
+		// Let the timers elapse


what timer?

The timers on the singleflight reader mock

miparnisari · 2026-02-02T18:58:51Z

internal/datastore/proxy/schemacaching/standardcaching_test.go

+		dsMock := &proxy_test.MockDatastore{}
+		ctx1, cancel1 := context.WithCancel(t.Context())
+		defer cancel1()
+		// Note that ctx2 will not be cancelled


?

t.Context returns a context that is canceled just before Cleanup-registered functions are called.

In the context of the test :P I meant that the second context isn't imperatively cancelled during test execution

tstirrat15

See comments

tstirrat15 · 2026-02-02T22:53:23Z

internal/caveats/run.go

 		return err
 	}
-	foundDefs, err := schemaReader.LookupSchemaDefinitionsByNames(ctx, caveatNames.AsSlice())
+	foundDefs, err := schemaReader.LookupCaveatDefinitionsByNames(ctx, caveatNames.AsSlice())


Using the specific lookup should prevent an NS lookup.

tstirrat15 · 2026-02-02T22:54:02Z

internal/datastore/proxy/schemacaching/standardcaching_test.go

+		go readCaveat()
+		go readCaveat()
+
+		// Let the timers elapse


The timers on the singleflight reader mock

tstirrat15 · 2026-02-02T22:56:31Z

internal/datastore/proxy/schemacaching/standardcaching_test.go

+		dsMock := &proxy_test.MockDatastore{}
+		ctx1, cancel1 := context.WithCancel(t.Context())
+		defer cancel1()
+		// Note that ctx2 will not be cancelled


In the context of the test :P I meant that the second context isn't imperatively cancelled during test execution

tstirrat15 · 2026-02-02T22:57:01Z

internal/datastore/proxy/schemacaching/standardcaching_test.go

 }

+// TODO
+func TestMixedCaching(t *testing.T) {


I was able to remedy this by adding separate Lookup methods that return just caveats and namespaces.

tstirrat15 · 2026-02-02T22:57:39Z

internal/datastore/proxy/schemacaching/watchingcache_test.go

+	require.Equal(t, "somenamespace", nsRevDef.Definition.Name)
+
+	// Repeat with lookup
+	nsDefMap, err := schemaReader.LookupTypeDefinitionsByNames(t.Context(), []string{"somenamespace"})


Same in this file - using the new methods means that the new test better reflects the old one.

tstirrat15 · 2026-02-02T22:58:27Z

internal/namespace/util.go

 		return err
 	}
-	foundDefs, err := schemaReader.LookupSchemaDefinitionsByNames(ctx, nsNames.AsSlice())
+	foundDefs, err := schemaReader.LookupTypeDefinitionsByNames(ctx, nsNames.AsSlice())


This only handles typedefs so we can use the more specific function.

tstirrat15 · 2026-02-02T22:59:25Z

internal/relationships/validation.go

 	var referencedNamespaceMap map[string]*schema.Definition
 	var referencedCaveatMap map[string]*core.CaveatDefinition

-	if !referencedNamespaceNames.IsEmpty() || !referencedCaveatNamesWithContext.IsEmpty() {


I'd welcome another pair of eyes on this file.

The old logic combined the namespace and caveat names and then made a single Lookup call. The new one keeps them separate and handles them separately, which should reduce the work and also makes the code slightly easier to follow at the expense of some duplication.

This looks good to me

tstirrat15 · 2026-02-02T22:59:34Z

internal/services/v1/experimental.go

 			numWritten += streamWritten

 			// The stream has terminated because we're awaiting namespace and/or caveat information
-			if len(adapter.awaitingNamespaces) > 0 || len(adapter.awaitingCaveats) > 0 {


This is the exact same refactor.

tstirrat15 · 2026-02-02T22:59:55Z

internal/services/v1/permissions.go

 			numWritten += streamWritten

 			// The stream has terminated because we're awaiting namespace and/or caveat information
-			if len(adapter.awaitingNamespaces) > 0 || len(adapter.awaitingCaveats) > 0 {


And this too.

tstirrat15 · 2026-02-02T23:00:08Z

pkg/datastore/schema.go

 	LookupSchemaDefinitionsByNames(ctx context.Context, names []string) (map[string]SchemaDefinition, error)
+
+	// LookupTypeDefinitionsByNames looks up type definitions by name.
+	LookupTypeDefinitionsByNames(ctx context.Context, names []string) (map[string]SchemaDefinition, error)


These are the new methods added.

josephschorr

LGTM

pkg/datastore/schema.go

miparnisari · 2026-02-03T14:34:31Z

pkg/datastore/schema.go

@@ -42,6 +42,12 @@ type SchemaReader interface {

 	// LookupSchemaDefinitionsByNames looks up type and caveat definitions by name.
 	LookupSchemaDefinitionsByNames(ctx context.Context, names []string) (map[string]SchemaDefinition, error)


LookupSchemaDefinitionsByNames isn't used anymore (well except in tests). Can we mark it as deprecated with pointers to the new ones?

We'll use it when we move to the new schema storage mechanism, because it will make sense when the cost is the same to go look at the entire schema.

miparnisari · 2026-02-03T15:17:47Z

internal/relationships/validation.go

 		}
-
+		referencedNamespaceMap = make(map[string]*schema.Definition, len(foundNamespaceDefs))
 		ts := schema.NewTypeSystem(schema.ResolverForDatastoreReader(reader))


this is funny.. if you remove ts and pass schema.NewDefinition(nil,...) the unit tests still pass 💀

not saying you change that but i think the schema.Definition struct needs a review

Co-authored-by: Maria Ines Parnisari <maria.ines.parnisari@authzed.com>

tstirrat15 requested a review from a team as a code owner January 30, 2026 16:24

github-actions bot added the area/datastore Affects the storage system label Jan 30, 2026

tstirrat15 commented Jan 30, 2026

View reviewed changes

tstirrat15 force-pushed the hook-up-cache-proxies-to-schemareader branch 3 times, most recently from 7396adf to 496d3f6 Compare January 31, 2026 03:07

github-actions bot added the area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) label Jan 31, 2026

tstirrat15 force-pushed the hook-up-cache-proxies-to-schemareader branch from 496d3f6 to 303e311 Compare February 2, 2026 13:33

tstirrat15 commented Feb 2, 2026

View reviewed changes

tstirrat15 force-pushed the hook-up-cache-proxies-to-schemareader branch from 303e311 to f5101b9 Compare February 2, 2026 14:59

tstirrat15 mentioned this pull request Feb 2, 2026

chore: go through other proxies #2871

Merged

tstirrat15 changed the title ~~fix: hook up proxies with schemareader~~ fix: hook up schema caching proxies with schemareader Feb 2, 2026

miparnisari reviewed Feb 2, 2026

View reviewed changes

internal/datastore/proxy/schemacaching/standardcaching_test.go Show resolved Hide resolved

miparnisari reviewed Feb 2, 2026

View reviewed changes

miparnisari previously approved these changes Feb 2, 2026

View reviewed changes

tstirrat15 dismissed miparnisari’s stale review via fd224db February 2, 2026 22:46

tstirrat15 force-pushed the hook-up-cache-proxies-to-schemareader branch from fd224db to 502cbd2 Compare February 2, 2026 22:52

github-actions bot added the area/api v1 Affects the v1 API label Feb 2, 2026

tstirrat15 commented Feb 2, 2026

View reviewed changes

josephschorr previously approved these changes Feb 2, 2026

View reviewed changes

tstirrat15 added 3 commits February 2, 2026 16:13

fix: hook up proxies with schemareader

3e9f185

chore: add and use new lookup methods

eead2d3

chore: empty commit

8f3e04b

tstirrat15 force-pushed the hook-up-cache-proxies-to-schemareader branch from 502cbd2 to 8f3e04b Compare February 2, 2026 23:13

miparnisari reviewed Feb 3, 2026

View reviewed changes

pkg/datastore/schema.go Outdated Show resolved Hide resolved

miparnisari reviewed Feb 3, 2026

View reviewed changes

tstirrat15 dismissed josephschorr’s stale review via ace7341 February 3, 2026 15:44

chore: update comment

0aabd07

Co-authored-by: Maria Ines Parnisari <maria.ines.parnisari@authzed.com>

tstirrat15 force-pushed the hook-up-cache-proxies-to-schemareader branch from ace7341 to 0aabd07 Compare February 3, 2026 15:46

miparnisari approved these changes Feb 3, 2026

View reviewed changes

tstirrat15 merged commit 10cc7f7 into main Feb 3, 2026
74 of 78 checks passed

tstirrat15 deleted the hook-up-cache-proxies-to-schemareader branch February 3, 2026 16:10

github-actions bot locked and limited conversation to collaborators Feb 3, 2026

		@@ -42,6 +42,12 @@ type SchemaReader interface {

		// LookupSchemaDefinitionsByNames looks up type and caveat definitions by name.
		LookupSchemaDefinitionsByNames(ctx context.Context, names []string) (map[string]SchemaDefinition, error)

fix: hook up schema caching proxies with schemareader #2868

fix: hook up schema caching proxies with schemareader #2868

Uh oh!

Conversation

tstirrat15 commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Testing

Uh oh!

tstirrat15 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tstirrat15 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstirrat15 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tstirrat15 commented Jan 30, 2026 •

edited

Loading

codecov bot commented Jan 30, 2026 •

edited

Loading

miparnisari Feb 3, 2026 •

edited

Loading

miparnisari Feb 3, 2026 •

edited

Loading