Merge pull request #2148 from authzed/fix-cache-inconsistency-segfault

Fix cache inconsistency segfault

internal/datastore/proxy/schemacaching/standardcache.go

@@ -126,6 +126,15 @@ func listAndCache[T schemaDefinition](
 		}
 
 		remainingToLoad.Delete(name)
+
+		if loaded.notFound != nil {
+			// If the value was in the cache, but its notFound is defined (implying that
+			// it was not found on a previous lookup), we pass over it.
+			// We still remove it from the `remainingToLoad` set because
+			// we don't want to go to the datastore for it.
+			continue
+		}
+
 		foundDefs = append(foundDefs, datastore.RevisionedDefinition[T]{
 			Definition:          loaded.definition.(T),
 			LastWrittenRevision: loaded.updated,

internal/datastore/proxy/schemacaching/standardcaching_test.go

@@ -515,3 +515,75 @@ func TestMixedCaching(t *testing.T) {
 		})
 	}
 }
+
+// NOTE: This uses a full memdb datastore because we want to exercise
+// the cache behavior without mocking it.
+func TestInvalidNamespaceInCache(t *testing.T) {
+	invalidNamespace := "invalid_namespace"
+
+	require := require.New(t)
+
+	ctx := context.Background()
+
+	memoryDatastore, err := memdb.NewMemdbDatastore(0, 1*time.Hour, 1*time.Hour)
+	require.NoError(err)
+	ds := NewCachingDatastoreProxy(memoryDatastore, DatastoreProxyTestCache(t), 1*time.Hour, JustInTimeCaching, 100*time.Millisecond)
+
+	headRevision, err := ds.HeadRevision(ctx)
+	require.NoError(err)
+	dsReader := ds.SnapshotReader(headRevision)
+
+	namespace, _, err := dsReader.ReadNamespaceByName(ctx, invalidNamespace)
+	require.Nil(namespace)
+	// NOTE: we're expecting this to error, because the namespace doesn't exist.
+	// However, the act of calling it sets the cache value to nil, which means that
+	// subsequent calls to the cache return that nil value. That's what needed to
+	// be filtered out of the list call.
+	require.Error(err)
+
+	// Look it up again - in the bug that this captures,
+	// it was populated into the cache and came back out.
+	found, err := dsReader.LookupNamespacesWithNames(ctx, []string{invalidNamespace})
+	require.Empty(found)
+	require.NoError(err)
+}
+
+func TestMixedInvalidNamespacesInCache(t *testing.T) {
+	invalidNamespace := "invalid_namespace"
+	validNamespace := "valid_namespace"
+
+	require := require.New(t)
+
+	ctx := context.Background()
+
+	memoryDatastore, err := memdb.NewMemdbDatastore(0, 1*time.Hour, 1*time.Hour)
+	require.NoError(err)
+	ds := NewCachingDatastoreProxy(memoryDatastore, DatastoreProxyTestCache(t), 1*time.Hour, JustInTimeCaching, 100*time.Millisecond)
+
+	require.NoError(err)
+
+	// Write in the valid namespace
+	revision, err := ds.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error {
+		writeErr := rwt.WriteNamespaces(ctx, &core.NamespaceDefinition{
+			Name: validNamespace,
+		})
+		return writeErr
+	})
+	require.NoError(err)
+
+	dsReader := ds.SnapshotReader(revision)
+
+	namespace, _, err := dsReader.ReadNamespaceByName(ctx, invalidNamespace)
+	require.Nil(namespace)
+	// NOTE: we're expecting this to error, because the namespace doesn't exist.
+	// However, the act of calling it sets the cache value to nil, which means that
+	// subsequent calls to the cache return that nil value. That's what needed to
+	// be filtered out of the list call.
+	require.Error(err)
+
+	// We're asserting that we find the thing we're looking for and don't receive a notfound value
+	found, err := dsReader.LookupNamespacesWithNames(ctx, []string{invalidNamespace, validNamespace})
+	require.Len(found, 1)
+	require.Equal(validNamespace, found[0].Definition.Name)
+	require.NoError(err)
+}