more review comments

Signed-off-by: grokspawn <jordan@nimblewidget.com>

Author: grokspawn

internal/catalogd/graphql/graphql.go

@@ -334,6 +334,8 @@ func determineFieldType(value interface{}) (reflect.Kind, bool) {
 	return reflect.TypeOf(firstElem).Kind(), true
 }
 
+// Sampling limits for schema inference — controls how many slice elements and
+// distinct values are examined when inferring GraphQL types from catalog data.
 const maxSampleElements = 10
 const maxSampleValues = 10
 

internal/catalogd/graphql/validation.go

@@ -8,9 +8,9 @@ import (
 )
 
 const (
-	MaxQueryDepth   = 10
-	MaxQueryAliases = 50
-	MaxQueryFields  = 500
+	maxQueryDepth   = 10
+	maxQueryAliases = 50
+	maxQueryFields  = 500
 )
 
 type queryComplexity struct {
@@ -51,21 +51,21 @@ func (c *queryComplexity) walkSelectionSet(ss *ast.SelectionSet, depth int) erro
 	if ss == nil {
 		return nil
 	}
-	if depth > MaxQueryDepth {
-		return fmt.Errorf("query exceeds maximum depth of %d", MaxQueryDepth)
+	if depth > maxQueryDepth {
+		return fmt.Errorf("query exceeds maximum depth of %d", maxQueryDepth)
 	}
 
 	for _, sel := range ss.Selections {
 		switch s := sel.(type) {
 		case *ast.Field:
 			c.fields++
-			if c.fields > MaxQueryFields {
-				return fmt.Errorf("query exceeds maximum field count of %d", MaxQueryFields)
+			if c.fields > maxQueryFields {
+				return fmt.Errorf("query exceeds maximum field count of %d", maxQueryFields)
 			}
 			if s.Alias != nil && s.Alias.Value != "" {
 				c.aliases++
-				if c.aliases > MaxQueryAliases {
-					return fmt.Errorf("query exceeds maximum alias count of %d", MaxQueryAliases)
+				if c.aliases > maxQueryAliases {
+					return fmt.Errorf("query exceeds maximum alias count of %d", maxQueryAliases)
 				}
 			}
 			if err := c.walkSelectionSet(s.SelectionSet, depth+1); err != nil {

internal/catalogd/graphql/validation_test.go

@@ -24,14 +24,14 @@ func TestValidateQueryComplexity_ParseError(t *testing.T) {
 }
 
 func TestValidateQueryComplexity_ExceedsDepth(t *testing.T) {
-	// Build a query that exceeds MaxQueryDepth (10)
+	// Build a query that exceeds maxQueryDepth (10)
 	var b strings.Builder
 	b.WriteString("{ ")
-	for i := 0; i <= MaxQueryDepth+1; i++ {
+	for i := 0; i <= maxQueryDepth+1; i++ {
 		b.WriteString(fmt.Sprintf("f%d { ", i))
 	}
 	b.WriteString("leaf")
-	for i := 0; i <= MaxQueryDepth+1; i++ {
+	for i := 0; i <= maxQueryDepth+1; i++ {
 		b.WriteString(" }")
 	}
 	b.WriteString(" }")
@@ -46,14 +46,14 @@ func TestValidateQueryComplexity_ExceedsDepth(t *testing.T) {
 }
 
 func TestValidateQueryComplexity_WithinDepthLimit(t *testing.T) {
-	// Build a query at exactly MaxQueryDepth (should pass)
+	// Build a query at exactly maxQueryDepth (should pass)
 	var b strings.Builder
 	b.WriteString("{ ")
-	for i := 1; i < MaxQueryDepth; i++ {
+	for i := 1; i < maxQueryDepth; i++ {
 		b.WriteString(fmt.Sprintf("f%d { ", i))
 	}
 	b.WriteString("leaf")
-	for i := 1; i < MaxQueryDepth; i++ {
+	for i := 1; i < maxQueryDepth; i++ {
 		b.WriteString(" }")
 	}
 	b.WriteString(" }")
@@ -67,7 +67,7 @@ func TestValidateQueryComplexity_WithinDepthLimit(t *testing.T) {
 func TestValidateQueryComplexity_ExceedsAliases(t *testing.T) {
 	var b strings.Builder
 	b.WriteString("{ ")
-	for i := 0; i <= MaxQueryAliases; i++ {
+	for i := 0; i <= maxQueryAliases; i++ {
 		b.WriteString(fmt.Sprintf("a%d: name ", i))
 	}
 	b.WriteString("}")
@@ -84,7 +84,7 @@ func TestValidateQueryComplexity_ExceedsAliases(t *testing.T) {
 func TestValidateQueryComplexity_ExceedsFieldCount(t *testing.T) {
 	var b strings.Builder
 	b.WriteString("{ ")
-	for i := 0; i <= MaxQueryFields; i++ {
+	for i := 0; i <= maxQueryFields; i++ {
 		b.WriteString(fmt.Sprintf("f%d ", i))
 	}
 	b.WriteString("}")

internal/catalogd/storage/localdir.go

@@ -81,20 +81,19 @@ func NewLocalDirV1(rootDir string, rootURL *url.URL, enableMetasHandler MetasHan
 }
 
 func (s *LocalDirV1) Store(ctx context.Context, catalog string, fsys fs.FS) error {
+	s.m.Lock()
 	catalogDir, err := s.storeAtomicSwap(ctx, catalog, fsys)
 	if err != nil {
+		s.m.Unlock()
 		return err
 	}
+	s.m.Unlock()
 
-	// Pre-warm GraphQL schema cache outside the write lock.
-	// Concurrent queries during this window get a cache miss and trigger
-	// their own build via singleflight, which is safe — the data is on disk.
 	if s.graphqlSvc != nil {
 		s.graphqlSvc.InvalidateCache(catalog)
 
 		if _, err := s.graphqlSvc.GetSchema(ctx, catalog); err != nil {
-			// Schema build failed — remove the catalog to maintain consistency.
-			// Re-acquire the write lock for the rollback since it touches shared filesystem state.
+			s.graphqlSvc.InvalidateCache(catalog)
 			s.m.Lock()
 			removeErr := os.RemoveAll(catalogDir)
 			s.m.Unlock()
@@ -109,11 +108,8 @@ func (s *LocalDirV1) Store(ctx context.Context, catalog string, fsys fs.FS) erro
 }
 
 // storeAtomicSwap writes catalog data to a temp dir and atomically swaps it
-// into place. Holds the write lock for the duration of the filesystem operations only.
+// into place. Caller must hold s.m write lock.
 func (s *LocalDirV1) storeAtomicSwap(ctx context.Context, catalog string, fsys fs.FS) (string, error) {
-	s.m.Lock()
-	defer s.m.Unlock()
-
 	if err := os.MkdirAll(s.RootDir, 0700); err != nil {
 		return "", err
 	}
@@ -330,6 +326,9 @@ func (s *LocalDirV1) NewObjectLoader(catalog string) (gql.ObjectLoader, error) {
 	catalogPath := catalogFilePath(s.catalogDir(catalog))
 
 	return func(schemaName string, offset, limit int) ([]map[string]interface{}, error) {
+		s.m.RLock()
+		defer s.m.RUnlock()
+
 		sections := idx.GetSchemaSections(schemaName)
 		if sections == nil {
 			return nil, nil
@@ -350,8 +349,12 @@ func (s *LocalDirV1) NewObjectLoader(catalog string) (gql.ObjectLoader, error) {
 		}
 		defer f.Close()
 
+		const maxEntrySize = 16 * 1024 * 1024 // 16 MiB
 		results := make([]map[string]interface{}, 0, len(sections))
 		for _, sec := range sections {
+			if sec.Length <= 0 || sec.Length > maxEntrySize {
+				return nil, fmt.Errorf("invalid section length %d at offset %d", sec.Length, sec.Offset)
+			}
 			buf := make([]byte, sec.Length)
 			if _, err := f.ReadAt(buf, sec.Offset); err != nil {
 				return nil, fmt.Errorf("error reading section at offset %d: %w", sec.Offset, err)