diff --git a/.gitignore b/.gitignore index cf8abcdf3..f6c623268 100644 --- a/.gitignore +++ b/.gitignore @@ -38,3 +38,4 @@ dist/ # specific tenants and their data shapes, which must not enter this # public repository (see CLAUDE.md content guidelines). docs/tasks/ +.raceout/ diff --git a/Makefile b/Makefile index 863809573..bfeea02e6 100644 --- a/Makefile +++ b/Makefile @@ -51,8 +51,12 @@ protofmt: ## Format protobuf definitions. buf format -w .PHONY: test -test: ## Run the Go test suite used by CI. - go test -tags=baton_lambda_support -v ./... +test: ## Run the ordinary Go test suite used by pull-request CI. + go test -tags=baton_lambda_support ./... + +.PHONY: test-full +test-full: ## Run complete matrices and timing-sensitive soak iterations. + BATON_FULL_TESTS=1 BATON_CUT_SWEEP=full go test -tags=baton_lambda_support -count=1 -timeout=30m ./... # Two-artifact checkpoint compatibility matrix: builds the harness against # HEAD and a pinned past release, and exchanges mid-flight checkpoints in @@ -236,7 +240,7 @@ bench: ## Run curated checkpoint and medium full-sync benchmarks. .PHONY: scheduler-soak scheduler-soak: ## Run randomized scheduler cases under race detection. - BATON_SOAK_ITERATIONS=$(SOAK_ITERATIONS) go test -race -v -count=1 -timeout=30m -run TestSchedulerSoakRandomizedFanoutWithFailures ./pkg/sync + BATON_FULL_TESTS=1 BATON_SOAK_ITERATIONS=$(SOAK_ITERATIONS) go test -race -v -count=1 -timeout=30m -run TestSchedulerSoakRandomizedFanoutWithFailures ./pkg/sync .PHONY: errorfs-soak errorfs-soak: ## Sweep whole-sync Pebble crash points using errorfs. diff --git a/docs/TESTING.md b/docs/TESTING.md index 1acc505f8..3b369b7c6 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -9,12 +9,26 @@ Run `make help` for the current target list. ## CI-equivalent tests `make test` runs the ordinary Go test suite with the same build tag used by -CI. Pull-request CI also runs lint, protobuf checks, the full build, and -`make race-shard-audit` (see [Race shards](#race-shards)). +Linux pull-request CI. Windows CI additionally uses `-short` because its +filesystem is substantially slower. Pull-request CI also runs lint, protobuf +checks, bounded chaos checks, the full build, and `make race-shard-audit` (see +[Race shards](#race-shards)). Tests in this tier should be deterministic, self-contained, and reasonably -fast. A test that only skips on Windows with `testing.Short()` is still a CI -test on the other platforms. +fast. A test that only skips with `testing.Short()` remains part of Linux CI; +Windows uses the reduced mode because filesystem-heavy matrices are +substantially slower there. + +## Full Go suite + +`make test-full` expands the topological resume/interrupt fixture matrices, +differential seed sweeps, checkpoint-cut sweep, and WAL timing soak. Ordinary +CI retains every algorithm and interruption mode over representative acyclic +and cyclic graphs, evenly spaced checkpoint/response/expiry cuts, and 20 +WAL-race attempts. The full tier adds specialized graph fixtures and wider +sampling. The six-seed randomized scheduler soak also runs here and in +`make scheduler-soak`; ordinary CI retains the deterministic scheduler tests +and the complete deterministic chaos corpora. ## Bounded checks omitted from CI diff --git a/pkg/dotc1z/c1file.go b/pkg/dotc1z/c1file.go index 70240e458..b3e34e449 100644 --- a/pkg/dotc1z/c1file.go +++ b/pkg/dotc1z/c1file.go @@ -58,6 +58,15 @@ type C1File struct { closed bool closedMu sync.Mutex + // dbClosed publishes handle closed-ness to concurrent callers. + // rawDb and db are written once at construction and never + // reassigned on a live C1File, so readers on other goroutines can + // load them without synchronization; closeRawDB flips this instead + // of nil-ing the fields. closedMu cannot serve this purpose because + // Close holds it across finalize, which is tens of minutes on a + // whale-scale file, and every reader would block for the duration. + dbClosed atomic.Bool + // bulkLoad defers secondary-index creation on a freshly-created // destination. When set, the per-table non-unique secondary indexes // are dropped right after table creation (instant on the empty table) @@ -640,7 +649,7 @@ func (c *C1File) Close(ctx context.Context) (retErr error) { // then dirtying via an attached-db mutation still releases the // SQLite handle and any FDs/goroutines it owns. if !c.dbUpdated.Load() || c.readOnly { - if c.rawDb != nil { + if c.rawDBOpen() { if err := c.closeRawDB(ctx); err != nil { return cleanupDbDir(c.dbFilePath, err) } @@ -722,10 +731,14 @@ func (c *C1File) finalize(ctx context.Context) error { l := ctxzap.Extract(finalizeCtx) // Only WAL-checkpoint and close the raw DB if a handle is open. - // Some callers (notably TestC1ZDecoder) manually close c.rawDb to + // Some callers (notably TestC1ZDecoder) release c.rawDb themselves to // force a checkpoint before calling Close — that path skips both - // operations here and proceeds directly to saveC1z. - if c.rawDb != nil { + // operations here and proceeds directly to saveC1z. Checkpointing a + // released handle would fail with sql.ErrConnDone and send us down the + // cleanupDbDir branch, deleting the working database instead of saving + // it, so this has to recognize a released handle however it was + // released. + if c.rawDBOpen() { // CRITICAL: Force a full WAL checkpoint before closing the database. // This ensures all WAL data is written back to the main database file // and the writes are synced to disk. Without this, on filesystems with @@ -801,22 +814,33 @@ func (c *C1File) finalize(ctx context.Context) error { return nil } -// closeRawDB wraps c.rawDb.Close with a span and drops the handle -// references on the C1File so callers do not have to repeat the -// nil-out. Returns the error from rawDb.Close so error paths can -// still propagate or log it. +// rawDBOpen reports whether the SQLite handle is still usable. +// +// Two idioms release the handle and both must read as closed here. +// closeRawDB publishes closed-ness through dbClosed and leaves the +// pointer in place, because concurrent readers race a nil-ing write +// (that race is what dbClosed exists to fix). Some tests instead close +// c.rawDb directly and nil it. A bare nil check would miss the first +// idiom and let a caller issue queries against a closed *sql.DB. +func (c *C1File) rawDBOpen() bool { + return c.rawDb != nil && !c.dbClosed.Load() +} + +// closeRawDB wraps c.rawDb.Close with a span and marks the handles +// closed so subsequent callers short-circuit. Returns the error from +// rawDb.Close so error paths can still propagate or log it. +// +// The CAS makes the underlying Close happen at most once; the flag is +// published before the Close so a racing caller fails closed rather +// than entering a database/sql call that is about to be torn down. func (c *C1File) closeRawDB(ctx context.Context) error { _, span := tracer.Start(ctx, "C1File.closeRawDB") var err error defer func() { uotel.EndSpanWithError(span, err) }() - if c.rawDb == nil { + if c.rawDb == nil || !c.dbClosed.CompareAndSwap(false, true) { return nil } - // Copy the rawDb to a local variable to avoid race conditions. - rawDb := c.rawDb - c.rawDb = nil - err = rawDb.Close() - c.db = nil + err = c.rawDb.Close() return err } @@ -1465,9 +1489,9 @@ func (c *C1File) countBySyncAndResourceType( return out, nil } -// validateDb ensures that the database has been opened. +// validateDb ensures that the database has been opened and not yet closed. func (c *C1File) validateDb(ctx context.Context) error { - if c.db == nil { + if c.db == nil || c.dbClosed.Load() { return ErrDbNotOpen } diff --git a/pkg/dotc1z/c1file_close_cancel_test.go b/pkg/dotc1z/c1file_close_cancel_test.go index dcd2126d2..9e1e7cacf 100644 --- a/pkg/dotc1z/c1file_close_cancel_test.go +++ b/pkg/dotc1z/c1file_close_cancel_test.go @@ -94,7 +94,7 @@ func TestC1FileCloseSurvivesCanceledCtx(t *testing.T) { // that even a Close called with a cancelled context fully releases // the underlying sql.DB handle. The cheap path's only ctx-bearing op // (closeRawDB → c.rawDb.Close) ignores cancellation; this test pins -// the structural promise that c.rawDb ends up nil regardless. +// the structural promise that the handle ends up closed regardless. func TestC1FileCloseReadOnlyClosesRawDb(t *testing.T) { openCtx := t.Context() testFilePath := filepath.Join(c1zTests.workingDir, "close-readonly.c1z") @@ -114,7 +114,7 @@ func TestC1FileCloseReadOnlyClosesRawDb(t *testing.T) { cancelCtx, cancel := context.WithCancel(openCtx) cancel() require.NoError(t, f2.Close(cancelCtx)) - require.Nil(t, f2.rawDb, "rawDb must be nil-ed out by the cheap-path close even on cancelled ctx") + require.True(t, f2.dbClosed.Load(), "rawDb must be closed by the cheap-path close even on cancelled ctx") require.True(t, f2.closed, "c.closed must be set after a successful cheap-path close") } @@ -142,6 +142,50 @@ func TestC1FileCloseReadOnlyButDirtyClosesRawDb(t *testing.T) { err = f2.Close(openCtx) require.ErrorIs(t, err, ErrReadOnly) - require.Nil(t, f2.rawDb, "rawDb must be closed before returning ErrReadOnly") + require.True(t, f2.dbClosed.Load(), "rawDb must be closed before returning ErrReadOnly") require.True(t, f2.closed, "c.closed must be set after returning ErrReadOnly so a retry short-circuits") } + +// TestC1FileFinalizeSavesWhenHandleReleasedOutOfBand covers the +// checkpoint-then-release idiom: a caller that WAL-checkpoints and +// releases the sql.DB handle before calling Close must still get its +// c1z written. +// +// finalize decides whether to checkpoint by asking whether the handle is +// open. Once closed-ness moved to the dbClosed flag, a nil-pointer check +// answered that question wrong: finalize would checkpoint a released +// handle, get sql.ErrConnDone, and take the cleanupDbDir branch — +// deleting the working database instead of saving it. +func TestC1FileFinalizeSavesWhenHandleReleasedOutOfBand(t *testing.T) { + openCtx := t.Context() + testFilePath := filepath.Join(c1zTests.workingDir, "close-handle-released.c1z") + + f, err := NewC1ZFile(openCtx, testFilePath) + require.NoError(t, err) + _, err = f.StartNewSync(openCtx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.NoError(t, f.PutResourceTypes(openCtx, v2.ResourceType_builder{Id: testResourceType}.Build())) + require.NoError(t, f.EndSync(openCtx)) + + // Checkpoint and release the handle out of band, exactly as a caller + // forcing a checkpoint before Close does. + _, _, _, err = f.truncateWAL(openCtx) + require.NoError(t, err) + require.NoError(t, f.closeRawDB(openCtx)) + require.True(t, f.dbClosed.Load(), "fixture: the handle must read as closed") + + require.NoError(t, f.Close(openCtx), "Close must skip the checkpoint and save the c1z") + + info, err := os.Stat(testFilePath) + require.NoError(t, err) + require.Greater(t, info.Size(), int64(0)) + + f2, err := NewC1ZFile(openCtx, testFilePath) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, f2.Close(context.Background())) }) + resp, err := f2.GetResourceType(openCtx, reader_v2.ResourceTypesReaderServiceGetResourceTypeRequest_builder{ + ResourceTypeId: testResourceType, + }.Build()) + require.NoError(t, err) + require.Equal(t, testResourceType, resp.GetResourceType().GetId()) +} diff --git a/pkg/dotc1z/clone_sync.go b/pkg/dotc1z/clone_sync.go index e56c7250a..d969978ab 100644 --- a/pkg/dotc1z/clone_sync.go +++ b/pkg/dotc1z/clone_sync.go @@ -233,13 +233,13 @@ func (c *C1File) SnapshotTo(ctx context.Context, outPath string, opts ...C1FOpti // connection. cloneCopy reads c.rawDb to take that one connection but never // mutates c.rawDb/c.db/c.currentSyncID/c.closed, so the live handle is left // exactly as it was found (plus the row-copy stall). Both entry points already -// reject a closed handle; the nil-rawDb guard below is defensive against a +// reject a closed handle; the validateDb guard below is defensive against a // future caller that does not. // errPrefix is the caller's error-message namespace ("clone-sync" / // "snapshot-to") so each entry point keeps its own observable error strings. func (c *C1File) cloneCopy(ctx context.Context, outPath string, syncID string, selectAll bool, errPrefix string, opts ...C1FOption) error { - if c.rawDb == nil { - return ErrDbNotOpen + if err := c.validateDb(ctx); err != nil { + return err } // Be sure that the output path is empty else return an error @@ -277,11 +277,9 @@ func (c *C1File) cloneCopy(ctx context.Context, outPath string, syncID string, s if err != nil { return err } - if err = initFile.rawDb.Close(); err != nil { + if err = initFile.closeRawDB(ctx); err != nil { return err } - initFile.rawDb = nil - initFile.db = nil qCtx, canc := context.WithCancel(ctx) defer canc() diff --git a/pkg/dotc1z/convert_open.go b/pkg/dotc1z/convert_open.go index 226baee41..e6c0af19c 100644 --- a/pkg/dotc1z/convert_open.go +++ b/pkg/dotc1z/convert_open.go @@ -144,7 +144,7 @@ func (c *C1File) closeWithoutSave(ctx context.Context) error { if c.closed { return nil } - if c.rawDb != nil { + if c.rawDBOpen() { if err := c.closeRawDB(ctx); err != nil { return err } diff --git a/pkg/dotc1z/copy_isolate_sync.go b/pkg/dotc1z/copy_isolate_sync.go index 28eef769f..abca17d4f 100644 --- a/pkg/dotc1z/copy_isolate_sync.go +++ b/pkg/dotc1z/copy_isolate_sync.go @@ -97,8 +97,8 @@ func (c *C1File) CopyIsolateSync(ctx context.Context, outPath string, syncID str ctx, span := tracer.Start(ctx, "C1File.CopyIsolateSync") defer func() { uotel.EndSpanWithError(span, err) }() - if c.rawDb == nil { - return ErrDbNotOpen + if err = c.validateDb(ctx); err != nil { + return err } if c.dbFilePath == "" { return fmt.Errorf("copy-isolate-sync: source working database path is not set") @@ -223,7 +223,7 @@ func (c *C1File) CopyIsolateSync(ctx context.Context, outPath string, syncID str // Close. Set once Close is invoked so we never double-close its rawDb. finalized := false defer func() { - if !finalized && copyFile.rawDb != nil { + if !finalized && copyFile.rawDBOpen() { if closeErr := copyFile.closeRawDB(ctx); closeErr != nil { err = errors.Join(err, closeErr) } diff --git a/pkg/dotc1z/engine/pebble/adapter.go b/pkg/dotc1z/engine/pebble/adapter.go index 272920cb2..54c0e4a76 100644 --- a/pkg/dotc1z/engine/pebble/adapter.go +++ b/pkg/dotc1z/engine/pebble/adapter.go @@ -109,7 +109,7 @@ func (e *Engine) startNewSync(ctx context.Context, syncType connectorstore.SyncT if existed, err := e.hasSyncRun(); err != nil { return "", err } else if existed { - if err := e.ResetForNewSync(ctx); err != nil { + if err := e.resetForNewSync(ctx); err != nil { return "", err } } @@ -219,7 +219,7 @@ func (e *Engine) SetCurrentSync(ctx context.Context, syncID string) error { func (e *Engine) CurrentSyncStep(ctx context.Context) (string, error) { e.lifecycleMu.Lock() defer e.lifecycleMu.Unlock() - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return "", nil } @@ -237,7 +237,7 @@ func (e *Engine) CurrentSyncStep(ctx context.Context) (string, error) { func (e *Engine) CheckpointSync(ctx context.Context, syncToken string) error { e.lifecycleMu.Lock() defer e.lifecycleMu.Unlock() - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return errors.New("CheckpointSync: no open sync") } @@ -260,7 +260,7 @@ func (e *Engine) CheckpointSync(ctx context.Context, syncToken string) error { func (e *Engine) EndSync(ctx context.Context) error { e.lifecycleMu.Lock() defer e.lifecycleMu.Unlock() - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return errors.New("EndSync: no open sync") } @@ -308,7 +308,7 @@ func (e *Engine) endSyncFinalize(ctx context.Context, existing *v3.SyncRunRecord // stashDeferredGrantStats, letting PersistSyncStats skip a second // O(grants) pass over the keyspace). if e.db.DeferredIdxPending() { - if err := e.BuildDeferredGrantIndexes(ctx); err != nil { + if err := e.buildDeferredGrantIndexes(ctx); err != nil { return fmt.Errorf("EndSync: build deferred grant indexes: %w", err) } if err := e.clearDeferredIdxPending(); err != nil { @@ -401,7 +401,7 @@ func (e *Engine) endSyncFinalize(ctx context.Context, existing *v3.SyncRunRecord // disjoint range of the records slice and uses its own arena, so no // shared mutable state across workers. func (e *Engine) PutGrants(ctx context.Context, grants ...*v2.Grant) error { - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return ErrNoCurrentSync } @@ -419,12 +419,12 @@ func (e *Engine) PutGrants(ctx context.Context, grants ...*v2.Grant) error { // most once across the whole sync (not just within this batch). Live connector // writes should use PutGrants. func (e *Engine) UnsafePutUniqueGrants(ctx context.Context, grants ...*v2.Grant) error { - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return ErrNoCurrentSync } records := translateGrants(syncID, grants) - if err := e.UnsafePutUniqueGrantRecords(ctx, records...); err != nil { + if err := e.unsafePutUniqueGrantRecords(ctx, records...); err != nil { return fmt.Errorf("UnsafePutUniqueGrants: %w", err) } return nil @@ -523,7 +523,7 @@ func translateGrantsSerial(syncID string, grants []*v2.Grant, discoveredAt []*ti // PutResourceTypes writes a batch of resource types in a single // Pebble batch. func (e *Engine) PutResourceTypes(ctx context.Context, rts ...*v2.ResourceType) error { - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return ErrNoCurrentSync } @@ -550,7 +550,7 @@ func (e *Engine) PutResourceTypes(ctx context.Context, rts ...*v2.ResourceType) // PutResources writes a batch of resources in a single Pebble batch. func (e *Engine) PutResources(ctx context.Context, resources ...*v2.Resource) error { - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return ErrNoCurrentSync } @@ -577,7 +577,7 @@ func (e *Engine) PutResources(ctx context.Context, resources ...*v2.Resource) er // PutEntitlements writes a batch of entitlements in a single Pebble batch. func (e *Engine) PutEntitlements(ctx context.Context, entitlements ...*v2.Entitlement) error { - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return ErrNoCurrentSync } @@ -606,7 +606,7 @@ func (e *Engine) PutEntitlements(ctx context.Context, entitlements ...*v2.Entitl // bare-id lookup edge. Callers holding the full grant should prefer // DeleteGrantByRefs, which needs no id-string resolution. func (e *Engine) DeleteGrant(ctx context.Context, grantID string) error { - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return ErrNoCurrentSync } @@ -621,7 +621,7 @@ func (e *Engine) DeleteGrant(ctx context.Context, grantID string) error { // an identity could not have been stored in the first place, so there is // nothing a string could correctly address here. func (e *Engine) DeleteGrantByRefs(ctx context.Context, grant *v2.Grant) error { - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return ErrNoCurrentSync } @@ -629,7 +629,7 @@ func (e *Engine) DeleteGrantByRefs(ctx context.Context, grant *v2.Grant) error { if _, err := grantIdentityFromRecord(rec); err != nil { return fmt.Errorf("DeleteGrantByRefs: grant %q: %w", grant.GetId(), err) } - return e.DeleteGrantByIdentityRefs(ctx, rec) + return e.deleteGrantByIdentityRefs(ctx, rec) } // PutAsset writes a single asset row. assetRef carries the @@ -637,7 +637,7 @@ func (e *Engine) DeleteGrantByRefs(ctx context.Context, grant *v2.Grant) error { // joined with a "/" separator since the engine's AssetRecord PK is // (sync_id, external_id). func (e *Engine) PutAsset(ctx context.Context, assetRef *v2.AssetRef, contentType string, data []byte) error { - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return ErrNoCurrentSync } @@ -678,14 +678,14 @@ func (e *Engine) Cleanup(ctx context.Context) error { return nil } // asset. The returned reader is backed by a bytes.Reader over the // fully-materialized blob. func (e *Engine) GetAsset(ctx context.Context, req *v2.AssetServiceGetAssetRequest) (string, io.Reader, error) { - syncID := e.CurrentSyncID() + syncID := e.currentSyncID() if syncID == "" { return "", nil, ErrNoCurrentSync } if req == nil || req.GetAsset() == nil { return "", nil, errors.New("GetAsset: nil request") } - rec, err := e.GetAssetRecord(ctx, req.GetAsset().GetId()) + rec, err := e.getAssetRecord(ctx, req.GetAsset().GetId()) if err != nil { return "", nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) } @@ -722,10 +722,10 @@ func (e *Engine) ListGrants(ctx context.Context, req *v2.GrantsServiceListGrants var records []*v3.GrantRecord var nextCursor string if r := req.GetResource(); r != nil && r.GetId() != nil { - records, nextCursor, err = e.PaginateGrantsByEntitlementResource(ctx, + records, nextCursor, err = e.paginateGrantsByEntitlementResource(ctx, r.GetId().GetResourceType(), r.GetId().GetResource(), cursor, limit) } else { - records, nextCursor, err = e.PaginateGrants(ctx, cursor, limit) + records, nextCursor, err = e.paginateGrants(ctx, cursor, limit) } if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) @@ -798,10 +798,10 @@ func (e *Engine) ListResources(ctx context.Context, req *v2.ResourcesServiceList var records []*v3.ResourceRecord var err error if useParent { - records, nextCursor, err = e.PaginateResourcesByParent(ctx, + records, nextCursor, err = e.paginateResourcesByParent(ctx, parent.GetResourceType(), parent.GetResource(), cursor, fetchLimit) } else { - records, nextCursor, err = e.PaginateResources(ctx, cursor, fetchLimit) + records, nextCursor, err = e.paginateResources(ctx, cursor, fetchLimit) } if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) @@ -847,7 +847,7 @@ func (e *Engine) ListResourceTypes(ctx context.Context, req *v2.ResourceTypesSer return nil, ErrNoCurrentSync } limit := clampPageSize(req.GetPageSize()) - records, nextCursor, err := e.PaginateResourceTypes(ctx, req.GetPageToken(), limit) + records, nextCursor, err := e.paginateResourceTypes(ctx, req.GetPageToken(), limit) if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) } @@ -877,10 +877,10 @@ func (e *Engine) ListEntitlements(ctx context.Context, req *v2.EntitlementsServi var records []*v3.EntitlementRecord var nextCursor string if r := req.GetResource(); r != nil && r.GetId() != nil { - records, nextCursor, err = e.PaginateEntitlementsByResource(ctx, + records, nextCursor, err = e.paginateEntitlementsByResource(ctx, r.GetId().GetResourceType(), r.GetId().GetResource(), cursor, limit) } else { - records, nextCursor, err = e.PaginateEntitlements(ctx, cursor, limit) + records, nextCursor, err = e.paginateEntitlements(ctx, cursor, limit) } if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) diff --git a/pkg/dotc1z/engine/pebble/adapter_grants_store.go b/pkg/dotc1z/engine/pebble/adapter_grants_store.go index de4144434..247ced930 100644 --- a/pkg/dotc1z/engine/pebble/adapter_grants_store.go +++ b/pkg/dotc1z/engine/pebble/adapter_grants_store.go @@ -62,7 +62,7 @@ var expandedGrantImmutableAnnotationAny = func() *anypb.Any { // Caught by TestStoreExpandedGrantsPreservesExpansion and the // SQLite conformance test of the same name. func (g pebbleGrantStore) StoreExpandedGrants(ctx context.Context, grants ...*v2.Grant) error { - syncID := g.e.CurrentSyncID() + syncID := g.e.currentSyncID() if syncID == "" { return ErrNoCurrentSync } @@ -84,7 +84,7 @@ func (g pebbleGrantStore) StoreExpandedGrants(ctx context.Context, grants ...*v2 // PutExpandedGrantRecords stamps/preserves it. The returned pointers alias // the arena, which outlives this call's use of `merged`. merged := g.translateExpanded(syncID, grants) - return g.e.PutExpandedGrantRecords(ctx, merged) + return g.e.putExpandedGrantRecords(ctx, merged) } // StoreNewExpandedGrants is the fast path for synthesized expanded grants. The @@ -92,7 +92,7 @@ func (g pebbleGrantStore) StoreExpandedGrants(ctx context.Context, grants ...*v2 // can skip the read-before-write Get used to preserve side-state and clean stale // indexes for updates. func (g pebbleGrantStore) StoreNewExpandedGrants(ctx context.Context, grants ...*v2.Grant) error { - syncID := g.e.CurrentSyncID() + syncID := g.e.currentSyncID() if syncID == "" { return ErrNoCurrentSync } @@ -100,7 +100,7 @@ func (g pebbleGrantStore) StoreNewExpandedGrants(ctx context.Context, grants ... return nil } merged := g.translateExpanded(syncID, grants) - return g.e.PutSynthesizedGrantRecords(ctx, merged) + return g.e.putSynthesizedGrantRecords(ctx, merged) } // appendSynthesizedGrantRecords translates one destination's synthesized @@ -141,7 +141,7 @@ func appendSynthesizedGrantRecords(records []synthesizedGrantRecord, dest *v2.En } func (g pebbleGrantStore) StoreNewExpandedGrantContributions(ctx context.Context, dest *v2.Entitlement, principals []*v3.PrincipalRef, sources []batonGrant.Sources) error { - if g.e.CurrentSyncID() == "" { + if g.e.currentSyncID() == "" { return ErrNoCurrentSync } if len(principals) == 0 { @@ -159,7 +159,7 @@ func (g pebbleGrantStore) StoreNewExpandedGrantContributions(ctx context.Context if err != nil { return err } - return g.e.PutSynthesizedGrantContributions(ctx, records) + return g.e.putSynthesizedGrantContributions(ctx, records) } // BeginExpandedGrantLayer opens a layer-scoped synthesized-grant layer session @@ -167,10 +167,10 @@ func (g pebbleGrantStore) StoreNewExpandedGrantContributions(ctx context.Context // by_principal index is not deferred); callers fall back to // StoreNewExpandedGrantContributions. func (g pebbleGrantStore) BeginExpandedGrantLayer(ctx context.Context) (bool, error) { - if g.e.CurrentSyncID() == "" { + if g.e.currentSyncID() == "" { return false, ErrNoCurrentSync } - return g.e.BeginSynthesizedGrantLayer(ctx) + return g.e.beginSynthesizedGrantLayer(ctx) } // AddExpandedGrantLayerContributions streams one destination's synthesized @@ -192,15 +192,15 @@ func (g pebbleGrantStore) AddExpandedGrantLayerContributions(ctx context.Context if err != nil { return err } - return g.e.AddSynthesizedGrantLayerContributions(ctx, records) + return g.e.addSynthesizedGrantLayerContributions(ctx, records) } func (g pebbleGrantStore) FinishExpandedGrantLayer(ctx context.Context) error { - return g.e.FinishSynthesizedGrantLayer(ctx) + return g.e.finishSynthesizedGrantLayer(ctx) } func (g pebbleGrantStore) AbortExpandedGrantLayer(ctx context.Context) error { - return g.e.AbortSynthesizedGrantLayer(ctx) + return g.e.abortSynthesizedGrantLayer(ctx) } func (g pebbleGrantStore) translateExpanded(syncID string, grants []*v2.Grant) []*v3.GrantRecord { @@ -238,11 +238,11 @@ func (g pebbleGrantStore) translateExpanded(syncID string, grants []*v2.Grant) [ // index scan (e.g. partial overwrite) is skipped — same orphan // semantic as the by_entitlement / by_principal indexes. func (g pebbleGrantStore) PendingExpansionPage(ctx context.Context, pageToken string) ([]c1zstore.PendingExpansion, string, error) { - syncID := g.e.CurrentSyncID() + syncID := g.e.currentSyncID() if syncID == "" { return nil, "", ErrNoCurrentSync } - records, next, err := g.e.PaginateGrantsByNeedsExpansion(ctx, pageToken, DefaultPageSize) + records, next, err := g.e.paginateGrantsByNeedsExpansion(ctx, pageToken, DefaultPageSize) if err != nil { return nil, "", c1zstore.AdaptNotFound(err, pebble.ErrNotFound) } @@ -307,11 +307,11 @@ func (g pebbleGrantStore) PendingExpansion(ctx context.Context) iter.Seq2[c1zsto // processGrantsWithExternalPrincipals and c1's fileClientWrapper — // behave identically across engines. func (g pebbleGrantStore) ListWithAnnotationsPage(ctx context.Context, pageToken string) ([]c1zstore.GrantAnnotation, string, error) { - syncID := g.e.CurrentSyncID() + syncID := g.e.currentSyncID() if syncID == "" { return nil, "", ErrNoCurrentSync } - records, next, err := g.e.PaginateGrants(ctx, pageToken, DefaultPageSize) + records, next, err := g.e.paginateGrants(ctx, pageToken, DefaultPageSize) if err != nil { return nil, "", c1zstore.AdaptNotFound(err, pebble.ErrNotFound) } @@ -350,13 +350,13 @@ func (g pebbleGrantStore) ListWithAnnotationsForResourcePage( return nil, "", errors.New("ListWithAnnotationsForResourcePage: nil resource") } if syncID == "" { - syncID = g.e.CurrentSyncID() + syncID = g.e.currentSyncID() } if syncID == "" { return nil, "", ErrNoCurrentSync } limit := clampPageSize(pageSize) - records, next, err := g.e.PaginateGrantsByEntitlementResource(ctx, + records, next, err := g.e.paginateGrantsByEntitlementResource(ctx, resource.GetId().GetResourceType(), resource.GetId().GetResource(), pageToken, limit) if err != nil { diff --git a/pkg/dotc1z/engine/pebble/adapter_reader.go b/pkg/dotc1z/engine/pebble/adapter_reader.go index d0782dd56..108fc8494 100644 --- a/pkg/dotc1z/engine/pebble/adapter_reader.go +++ b/pkg/dotc1z/engine/pebble/adapter_reader.go @@ -248,10 +248,10 @@ func (e *Engine) ListGrantsForEntitlement( var records []*v3.GrantRecord var next string if principalID != nil { - records, next, err = e.PaginateGrantsByEntitlementPrincipal(ctx, + records, next, err = e.paginateGrantsByEntitlementPrincipal(ctx, entIdentity, principalID.GetResourceType(), principalID.GetResource(), cursor, fetchLimit) } else { - records, next, err = e.PaginateGrantsByEntitlement(ctx, + records, next, err = e.paginateGrantsByEntitlement(ctx, entIdentity, cursor, fetchLimit) } if err != nil { @@ -323,7 +323,7 @@ func (e *Engine) ListGrantPrincipalKeysForEntitlement( } return nil, "", err } - keys, next, err := e.PaginateGrantPrincipalKeysByEntitlement(ctx, entIdentity, pageToken, clampPageSize(pageSize)) + keys, next, err := e.paginateGrantPrincipalKeysByEntitlement(ctx, entIdentity, pageToken, clampPageSize(pageSize)) if err != nil { return nil, "", c1zstore.AdaptNotFound(err, pebble.ErrNotFound) } @@ -382,13 +382,13 @@ func (e *Engine) ListGrantsForPrincipal( } return nil, err } - records, next, err = e.PaginateGrantsByEntitlementPrincipal(ctx, + records, next, err = e.paginateGrantsByEntitlementPrincipal(ctx, entIdentity, principal.GetResourceType(), principal.GetResource(), cursor, limit) if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) } } else { - records, next, err = e.PaginateGrantsByPrincipal(ctx, + records, next, err = e.paginateGrantsByPrincipal(ctx, principal.GetResourceType(), principal.GetResource(), cursor, limit) if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) @@ -426,7 +426,7 @@ func (e *Engine) ListGrantsForResourceType( } limit := clampPageSize(req.GetPageSize()) cursor := req.GetPageToken() - records, next, err := e.PaginateGrantsByPrincipalResourceType(ctx, rtFilter, cursor, limit) + records, next, err := e.paginateGrantsByPrincipalResourceType(ctx, rtFilter, cursor, limit) if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) } @@ -634,7 +634,7 @@ func (e *Engine) resolveActiveSyncForReader(ctx context.Context, annos []*anypb. if annoSyncID != "" { return annoSyncID, nil } - if id := e.CurrentSyncID(); id != "" { + if id := e.currentSyncID(); id != "" { return id, nil } id, err := e.LatestFinishedSyncID(ctx, connectorstore.SyncTypeAny) @@ -707,7 +707,7 @@ func (e *Engine) GetEntitlementGrantDigest(ctx context.Context, ent *v2.Entitlem if err != nil || !ok { return connectorstore.GrantDigest{}, false, err } - root, ok, err := e.GetEntitlementDigestRoot(ctx, id) + root, ok, err := e.getEntitlementDigestRoot(ctx, id) if err != nil || !ok { return connectorstore.GrantDigest{}, false, err } @@ -737,7 +737,7 @@ func (e *Engine) GetEntitlementGrantDigestNodes(ctx context.Context, ent *v2.Ent if err != nil || !ok { return nil, false, err } - root, ok, err := e.GetEntitlementDigestRoot(ctx, id) + root, ok, err := e.getEntitlementDigestRoot(ctx, id) if err != nil || !ok { return nil, false, err } @@ -794,7 +794,7 @@ func (e *Engine) ScanEntitlementGrantBucket(ctx context.Context, ent *v2.Entitle return err } bits := min(bucket.Level, digestMaxWidthBits) - return e.IterateGrantsByEntitlementBucket(ctx, id, DigestBucket{Index: bucket.Index, Bits: bits}, func(r *v3.GrantRecord) bool { + return e.iterateGrantsByEntitlementBucket(ctx, id, DigestBucket{Index: bucket.Index, Bits: bits}, func(r *v3.GrantRecord) bool { return yield(V3GrantToV2(r)) }) } diff --git a/pkg/dotc1z/engine/pebble/adapter_reader_test.go b/pkg/dotc1z/engine/pebble/adapter_reader_test.go index 7163b77d0..e0d5a4cbb 100644 --- a/pkg/dotc1z/engine/pebble/adapter_reader_test.go +++ b/pkg/dotc1z/engine/pebble/adapter_reader_test.go @@ -771,7 +771,7 @@ func TestStatsAndGrantStats(t *testing.T) { // GrantStats partitions by entitlement resource type. Our // mkV2Grant always sets the entitlement's resource to (app, github), // so all 3 grants count under "app". - gs, err := a.GrantStats(ctx, connectorstore.SyncTypeAny, syncID) + gs, err := a.grantStats(ctx, connectorstore.SyncTypeAny, syncID) require.NoError(t, err) require.Equal(t, int64(3), gs["app"], "GrantStats[app]") diff --git a/pkg/dotc1z/engine/pebble/adapter_reader_v3.go b/pkg/dotc1z/engine/pebble/adapter_reader_v3.go index 82fd4860e..103726b4b 100644 --- a/pkg/dotc1z/engine/pebble/adapter_reader_v3.go +++ b/pkg/dotc1z/engine/pebble/adapter_reader_v3.go @@ -113,10 +113,10 @@ func (r engineV3Grants) ListGrantsForEntitlement( var records []*v3.GrantRecord var next string if principalID != nil { - records, next, err = e.PaginateGrantsByEntitlementPrincipal(ctx, + records, next, err = e.paginateGrantsByEntitlementPrincipal(ctx, entIdentity, principalID.GetResourceType(), principalID.GetResource(), cursor, fetchLimit) } else { - records, next, err = e.PaginateGrantsByEntitlement(ctx, + records, next, err = e.paginateGrantsByEntitlement(ctx, entIdentity, cursor, fetchLimit) } if err != nil { @@ -180,7 +180,7 @@ func (r engineV3Grants) ListGrantsForResourceType( } limit := clampPageSize(req.GetPageSize()) cursor := req.GetPageToken() - records, next, err := e.PaginateGrantsByPrincipalResourceType(ctx, rtFilter, cursor, limit) + records, next, err := e.paginateGrantsByPrincipalResourceType(ctx, rtFilter, cursor, limit) if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) } @@ -246,7 +246,7 @@ EntitlementLoop: return nil, err } remaining := limit - len(out) - records, next, err := e.PaginateGrantsByEntitlement(ctx, entID, intraCursor, remaining) + records, next, err := e.paginateGrantsByEntitlement(ctx, entID, intraCursor, remaining) if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) } @@ -322,13 +322,13 @@ func (r engineV3Grants) ListGrantsForPrincipal( } return nil, err } - records, next, err = e.PaginateGrantsByEntitlementPrincipal(ctx, + records, next, err = e.paginateGrantsByEntitlementPrincipal(ctx, entIdentity, principal.GetResourceType(), principal.GetResource(), cursor, limit) if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) } } else { - records, next, err = e.PaginateGrantsByPrincipal(ctx, + records, next, err = e.paginateGrantsByPrincipal(ctx, principal.GetResourceType(), principal.GetResource(), cursor, limit) if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) diff --git a/pkg/dotc1z/engine/pebble/adapter_stats.go b/pkg/dotc1z/engine/pebble/adapter_stats.go index 68254ba69..8e512f766 100644 --- a/pkg/dotc1z/engine/pebble/adapter_stats.go +++ b/pkg/dotc1z/engine/pebble/adapter_stats.go @@ -167,7 +167,7 @@ func errNoFinishedSync(syncType connectorstore.SyncType) error { return status.Errorf(codes.NotFound, "no finished sync of type '%s' found", syncType) } -// GrantStats returns just the grant count, partitioned by entitlement +// grantStats returns just the grant count, partitioned by entitlement // resource_type_id. Used by progresslog to show per-RT progress. Like // Stats, this is an exact count via iteration. // @@ -180,7 +180,7 @@ func errNoFinishedSync(syncType connectorstore.SyncType) error { // Empty syncID resolves to the latest finished sync of syncType // (matching Stats / SQLite grantStats), and returns NotFound when // none exists. -func (e *Engine) GrantStats(ctx context.Context, syncType connectorstore.SyncType, syncID string) (map[string]int64, error) { +func (e *Engine) grantStats(ctx context.Context, syncType connectorstore.SyncType, syncID string) (map[string]int64, error) { if syncID == "" { var err error syncID, err = e.LatestFinishedSyncID(ctx, syncType) @@ -226,5 +226,5 @@ func (e *Engine) GrantStats(ctx context.Context, syncType connectorstore.SyncTyp // have a single file, so this returns the engine's directory. Used // by tooling that wants to copy or inspect the storage. func (e *Engine) OutputFilepath() (string, error) { - return e.DBDir(), nil + return e.databaseDir(), nil } diff --git a/pkg/dotc1z/engine/pebble/adapter_streaming.go b/pkg/dotc1z/engine/pebble/adapter_streaming.go index 7cdd0e777..9c80b420c 100644 --- a/pkg/dotc1z/engine/pebble/adapter_streaming.go +++ b/pkg/dotc1z/engine/pebble/adapter_streaming.go @@ -54,7 +54,7 @@ func (e *Engine) StreamGrants( case opts.EntitlementID != "": err = e.IterateGrantsByEntitlement(ctx, opts.EntitlementID, cb) case opts.PrincipalResourceType != "" && opts.PrincipalResourceID == "": - err = e.IterateGrantsByPrincipalResourceType(ctx, opts.PrincipalResourceType, cb) + err = e.iterateGrantsByPrincipalResourceType(ctx, opts.PrincipalResourceType, cb) default: err = e.IterateGrants(ctx, cb) } diff --git a/pkg/dotc1z/engine/pebble/adapter_sync_meta.go b/pkg/dotc1z/engine/pebble/adapter_sync_meta.go index d7ec808e2..1ecb51ce7 100644 --- a/pkg/dotc1z/engine/pebble/adapter_sync_meta.go +++ b/pkg/dotc1z/engine/pebble/adapter_sync_meta.go @@ -150,7 +150,7 @@ func (s pebbleSyncMeta) StatsV2(ctx context.Context, syncType connectorstore.Syn // sync is named. func (s pebbleSyncMeta) RecalculateStats(ctx context.Context, syncID string) error { if syncID == "" { - syncID = s.e.CurrentSyncID() + syncID = s.e.currentSyncID() } if syncID == "" { return errors.New("RecalculateStats: empty syncID and no current sync") @@ -195,8 +195,7 @@ func syncRunRecordToExported(r *v3.SyncRunRecord) *c1zstore.SyncRun { // sortedSyncRuns reads every sync_run record into the engine-neutral // c1zstore.SyncRun shape, sorted oldest-first (started_at, sync_id -// tiebreaker). Shared by CleanupCandidates and ListSyncRuns so the -// projection and ordering live in one place. +// tiebreaker), for ListSyncRuns. // // We sort explicitly rather than trust IterateAllSyncRuns' order: // the iterator walks by sync_id (KSUID), and KSUIDs only encode the @@ -257,19 +256,6 @@ func (e *Engine) sortedSyncRuns(ctx context.Context) ([]c1zstore.SyncRun, error) return out, nil } -// CleanupCandidates walks every sync_run record and projects it into -// the engine-neutral c1zstore.SyncRun shape, sorted oldest-first. -// c1zstore.SelectSyncsToDelete depends on this ordering so "drop the -// oldest overflow" trims the right end. Used by pkg/dotc1z's Pebble -// store to drive the retention policy at Cleanup. -func (e *Engine) CleanupCandidates(ctx context.Context) ([]c1zstore.SyncRun, error) { - out, err := e.sortedSyncRuns(ctx) - if err != nil { - return nil, fmt.Errorf("pebble CleanupCandidates: IterateAllSyncRuns: %w", err) - } - return out, nil -} - // ListSyncRuns returns every sync-run record projected into the // engine-neutral c1zstore.SyncRun shape, oldest-first (parent before // child for a well-formed chain), in a single page. A v3 Pebble c1z diff --git a/pkg/dotc1z/engine/pebble/assets.go b/pkg/dotc1z/engine/pebble/assets.go index 327f3f7e3..49db980ae 100644 --- a/pkg/dotc1z/engine/pebble/assets.go +++ b/pkg/dotc1z/engine/pebble/assets.go @@ -27,7 +27,7 @@ func (e *Engine) PutAssetRecord(ctx context.Context, r *v3.AssetRecord) error { }) } -func (e *Engine) GetAssetRecord(ctx context.Context, externalID string) (*v3.AssetRecord, error) { +func (e *Engine) getAssetRecord(ctx context.Context, externalID string) (*v3.AssetRecord, error) { val, closer, err := e.db.Get(encodeAssetKey(externalID)) if err != nil { return nil, err @@ -40,12 +40,6 @@ func (e *Engine) GetAssetRecord(ctx context.Context, externalID string) (*v3.Ass return r, nil } -func (e *Engine) DeleteAssetRecord(ctx context.Context, externalID string) error { - return e.withWrite(func() error { - return e.db.MetaDelete(encodeAssetKey(externalID), writeOpts(e.opts.durability)) - }) -} - func (e *Engine) IterateAssets(ctx context.Context, yield func(*v3.AssetRecord) bool) error { prefix := encodeAssetPrefix() iter, err := e.db.NewIter(&pebble.IterOptions{ diff --git a/pkg/dotc1z/engine/pebble/bulk_import.go b/pkg/dotc1z/engine/pebble/bulk_import.go index 4a97679fc..058c5b088 100644 --- a/pkg/dotc1z/engine/pebble/bulk_import.go +++ b/pkg/dotc1z/engine/pebble/bulk_import.go @@ -206,7 +206,7 @@ type BulkSyncImport struct { // files are staged in a fresh directory under tmpDir ("" = system temp // dir) and removed by Finish/Abort. func (e *Engine) StartBulkSyncImport(ctx context.Context, syncID string, tmpDir string) (*BulkSyncImport, error) { - if !e.IsFreshSync() { + if !e.isFreshSync() { return nil, errors.New("StartBulkSyncImport: sync is not fresh") } idBytes, err := codec.EncodeSyncID(syncID) diff --git a/pkg/dotc1z/engine/pebble/cleanup.go b/pkg/dotc1z/engine/pebble/cleanup.go index 416c29b50..98a459b56 100644 --- a/pkg/dotc1z/engine/pebble/cleanup.go +++ b/pkg/dotc1z/engine/pebble/cleanup.go @@ -14,7 +14,7 @@ import ( // keyspace. Engine-global metadata (keyspace-version stamp, // index-migration markers) is deliberately NOT included. Mirrors the // bucket plan in adapter_clone_sync.go; kept in lockstep so -// ResetForNewSync leaves no orphan rows that CloneSync would +// resetForNewSync leaves no orphan rows that CloneSync would // otherwise have copied. // // The returned ranges are NOT ordered for compaction efficiency. @@ -44,7 +44,7 @@ func scopedRanges() [][2][]byte { } } -// ResetForNewSync wipes every sync-scoped keyspace (records, indexes, +// resetForNewSync wipes every sync-scoped keyspace (records, indexes, // the sync-run record, and the stats sidecar) so a freshly started // sync begins on an empty keyspace. // @@ -79,8 +79,8 @@ func scopedRanges() [][2][]byte { // // Refuses while a fresh sync is in progress (between MarkFreshSync and // EndFreshSync): wiping mid-sync would corrupt the in-flight sync. -func (e *Engine) ResetForNewSync(ctx context.Context) error { - if e.IsFreshSync() { +func (e *Engine) resetForNewSync(ctx context.Context) error { + if e.isFreshSync() { return errors.New("ResetForNewSync: refusing to reset while a sync is in progress") } spans := []pebble.KeyRange{ diff --git a/pkg/dotc1z/engine/pebble/cleanup_test.go b/pkg/dotc1z/engine/pebble/cleanup_test.go index d5fb9ecfb..aa219ebc4 100644 --- a/pkg/dotc1z/engine/pebble/cleanup_test.go +++ b/pkg/dotc1z/engine/pebble/cleanup_test.go @@ -28,7 +28,7 @@ func TestResetForNewSyncRefusesActiveSync(t *testing.T) { a := NewAdapter(e) _, err := a.StartNewSync(ctx, connectorstore.SyncTypeFull, "") require.NoErrorf(t, err, "StartNewSync") - err = e.ResetForNewSync(ctx) + err = e.resetForNewSync(ctx) require.Error(t, err, "ResetForNewSync while a sync is active: expected error, got nil") } @@ -66,14 +66,14 @@ func TestResetForNewSyncReclaimsDiskImmediately(t *testing.T) { dataLo := []byte{versionV3, typeResourceType} dataHi := []byte{versionV3, typeEngineMeta} - before, err := e.EstimateDiskUsage(dataLo, dataHi) + before, err := e.estimateDiskUsage(dataLo, dataHi) require.NoErrorf(t, err, "EstimateDiskUsage (before)") require.NotZero(t, before, "sanity: expected non-zero on-disk usage for the finished sync's data span") // Replacement sync: StartNewSync excises the prior sync's data. _, err = a.StartNewSync(ctx, connectorstore.SyncTypeFull, "") require.NoErrorf(t, err, "StartNewSync (replacement)") - after, err := e.EstimateDiskUsage(dataLo, dataHi) + after, err := e.estimateDiskUsage(dataLo, dataHi) require.NoErrorf(t, err, "EstimateDiskUsage (after)") // The excise drops fully-covered SSTs from the manifest, so the // data span's estimated usage collapses to (near) zero immediately diff --git a/pkg/dotc1z/engine/pebble/compaction_pause_lifecycle_test.go b/pkg/dotc1z/engine/pebble/compaction_pause_lifecycle_test.go index 5faf5c36d..6d0b31985 100644 --- a/pkg/dotc1z/engine/pebble/compaction_pause_lifecycle_test.go +++ b/pkg/dotc1z/engine/pebble/compaction_pause_lifecycle_test.go @@ -47,7 +47,7 @@ func TestCompactionPauseLifecycle(t *testing.T) { require.NoError(t, err) require.NotNil(t, e.compactionScheduler) require.False(t, e.compactionScheduler.paused.Load(), "fresh engine starts unpaused") - require.False(t, e.IsSealed(), "fresh engine starts unsealed") + require.False(t, e.isSealed(), "fresh engine starts unsealed") a := NewAdapter(e) syncID, err := a.StartNewSync(ctx, connectorstore.SyncTypeFull, "") @@ -61,13 +61,13 @@ func TestCompactionPauseLifecycle(t *testing.T) { require.Error(t, a.EndSync(canceled), "EndSync with canceled ctx must fail in the deferred build") require.False(t, e.compactionScheduler.paused.Load(), "a failed EndSync must resume compactions: the sync stays bound and nothing else would ever resume") - require.False(t, e.IsSealed(), "a failed EndSync must not seal: the sync stays bound") + require.False(t, e.isSealed(), "a failed EndSync must not seal: the sync stays bound") // Successful EndSync: sealed (paused + writes refused) for the // save/close window. require.NoError(t, a.EndSync(ctx)) require.True(t, e.compactionScheduler.paused.Load(), "successful EndSync leaves compactions paused for the save window") - require.True(t, e.IsSealed(), "successful EndSync seals the engine") + require.True(t, e.isSealed(), "successful EndSync seals the engine") // Record writes are refused while sealed — loudly, not by silently // running on a paused scheduler. @@ -86,26 +86,26 @@ func TestCompactionPauseLifecycle(t *testing.T) { // Rebinding unseals and resumes. require.NoError(t, a.SetCurrentSync(ctx, syncID)) require.False(t, e.compactionScheduler.paused.Load(), "binding a sync must resume compactions") - require.False(t, e.IsSealed(), "binding a sync must unseal") + require.False(t, e.isSealed(), "binding a sync must unseal") // Seal again via EndSync, then prove StartNewSync (which must wipe the // sealed prior sync via ResetForNewSync) also unseals. require.NoError(t, a.EndSync(ctx)) - require.True(t, e.IsSealed()) + require.True(t, e.isSealed()) _, err = a.StartNewSync(ctx, connectorstore.SyncTypeFull, "") require.NoError(t, err, "StartNewSync over a sealed finished sync must work (ResetForNewSync is seal-exempt)") - require.False(t, e.IsSealed(), "StartNewSync must unseal") + require.False(t, e.isSealed(), "StartNewSync must unseal") require.False(t, e.compactionScheduler.paused.Load()) // Seal once more and prove a reopen starts unsealed. require.NoError(t, a.EndSync(ctx)) require.True(t, e.compactionScheduler.paused.Load()) - require.True(t, e.IsSealed()) + require.True(t, e.isSealed()) require.NoError(t, e.Close()) e2, err := Open(ctx, dir) require.NoError(t, err) defer e2.Close() require.False(t, e2.compactionScheduler.paused.Load(), "reopened engine must start unpaused") - require.False(t, e2.IsSealed(), "reopened engine must start unsealed") + require.False(t, e2.isSealed(), "reopened engine must start unsealed") } diff --git a/pkg/dotc1z/engine/pebble/deferred_index.go b/pkg/dotc1z/engine/pebble/deferred_index.go index 4baad1691..fefbd812d 100644 --- a/pkg/dotc1z/engine/pebble/deferred_index.go +++ b/pkg/dotc1z/engine/pebble/deferred_index.go @@ -41,7 +41,7 @@ import ( const deferredIndexSpillChunkBytes = 128 << 20 // deferredGrantStats carries the grant-keyspace stats accumulated during the -// BuildDeferredGrantIndexes scan: the same numbers computeSyncStats derives +// buildDeferredGrantIndexes scan: the same numbers computeSyncStats derives // from its own full grant scan. Fusing them into the index scan removes a // second O(grants) pass at EndSync. type deferredGrantStats struct { @@ -239,7 +239,7 @@ func (t *grantRebuildTee) closeAndWait() { } } -// BuildDeferredGrantIndexes rebuilds the remaining scattered expansion index +// buildDeferredGrantIndexes rebuilds the remaining scattered expansion index // family, by_principal, from entitlement-first primary grant keys. The expansion // write path can skip by_principal inline because expansion reads by entitlement // only; this method rewrites the whole by_principal range as one sorted SST at @@ -270,7 +270,7 @@ func (t *grantRebuildTee) closeAndWait() { // convention into an enforced invariant (a straggler write blocks until the // build finishes instead of racing the excise), and writeWG participation // means Close waits the build out instead of tearing down e.db under it. -func (e *Engine) BuildDeferredGrantIndexes(ctx context.Context) error { +func (e *Engine) buildDeferredGrantIndexes(ctx context.Context) error { // AllowSealed: EndSync seals BEFORE running this build so no straggler // record writer can slip a row in behind the scan (see Adapter.EndSync); // the build itself is one of the sealed window's own steps. diff --git a/pkg/dotc1z/engine/pebble/digest.go b/pkg/dotc1z/engine/pebble/digest.go index a37ff721f..09437feeb 100644 --- a/pkg/dotc1z/engine/pebble/digest.go +++ b/pkg/dotc1z/engine/pebble/digest.go @@ -454,7 +454,7 @@ func (e *Engine) buildPartitionDigestAtWidth(ctx context.Context, spec digestInd } opts := writeOpts(e.opts.durability) - if e.IsFreshSync() { + if e.isFreshSync() { opts = pebble.NoSync } if err := batch.Commit(opts); err != nil { diff --git a/pkg/dotc1z/engine/pebble/digest_test.go b/pkg/dotc1z/engine/pebble/digest_test.go index 837e712e8..c841e10d6 100644 --- a/pkg/dotc1z/engine/pebble/digest_test.go +++ b/pkg/dotc1z/engine/pebble/digest_test.go @@ -82,7 +82,7 @@ func makeGrantWithSources(syncID, externalID, entID, principalID string, sources // build is the production writer of the hash index + digests). func sealGrantDigests(t testing.TB, e *Engine) { t.Helper() - if err := e.BuildDeferredGrantIndexes(context.Background()); err != nil { + if err := e.buildDeferredGrantIndexes(context.Background()); err != nil { t.Fatalf("BuildDeferredGrantIndexes: %v", err) } } @@ -241,7 +241,7 @@ func TestGrantDigestIncludesExpandedGrants(t *testing.T) { // One expanded grant (with a source), via the expansion write path — // this arms the deferred-index marker. exp := makeGrantWithSources("", "g-expanded", "ent-A", "bob", "src-ent") - if err := e.PutExpandedGrantRecords(ctx, []*v3.GrantRecord{exp}); err != nil { + if err := e.putExpandedGrantRecords(ctx, []*v3.GrantRecord{exp}); err != nil { t.Fatalf("PutExpandedGrantRecords: %v", err) } if !e.db.DeferredIdxPending() { @@ -254,7 +254,7 @@ func TestGrantDigestIncludesExpandedGrants(t *testing.T) { if got := countKeyRangeTest(t, e, GrantByEntPrincHashLowerBound(), GrantByEntPrincHashUpperBound()); got != 2 { t.Fatalf("hash index rows = %d, want 2 (direct + expanded)", got) } - root, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) + root, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) if err != nil || !ok { t.Fatalf("root: ok=%v err=%v", ok, err) } @@ -565,7 +565,7 @@ func seedEntitlementAtWidth(t testing.TB, e *Engine, entID string, grants []*v3. // TestDigestDifferentWidthsComparison builds two digests of different // widths (4 vs 8 bits) over the SAME entitlement and exercises -// DirtyEntitlementBuckets across them. It validates two things the +// dirtyEntitlementBuckets across them. It validates two things the // equal-width tests cannot: // // - split-independence: identical grant content yields the same root @@ -604,11 +604,11 @@ func TestDigestDifferentWidthsComparison(t *testing.T) { seedEntitlementAtWidth(t, eb, "ent-A", mkGrants(), 8) entA := testEntIdentity("ent-A") - ra, okA, err := ea.GetEntitlementDigestRoot(ctx, entA) + ra, okA, err := ea.getEntitlementDigestRoot(ctx, entA) if err != nil || !okA { t.Fatalf("root A: ok=%v err=%v", okA, err) } - rb, okB, err := eb.GetEntitlementDigestRoot(ctx, entA) + rb, okB, err := eb.getEntitlementDigestRoot(ctx, entA) if err != nil || !okB { t.Fatalf("root B: ok=%v err=%v", okB, err) } @@ -620,7 +620,7 @@ func TestDigestDifferentWidthsComparison(t *testing.T) { if !bytes.Equal(ra.Hash, rb.Hash) { t.Fatalf("different-width digests over identical content disagree on root:\n A(w4)=%x\n B(w8)=%x", ra.Hash, rb.Hash) } - dirty, err := ea.DirtyEntitlementBuckets(ctx, eb, entA) + dirty, err := ea.dirtyEntitlementBuckets(ctx, eb, entA) if err != nil { t.Fatalf("DirtyEntitlementBuckets (identical): %v", err) } @@ -653,12 +653,12 @@ func TestDigestDifferentWidthsComparison(t *testing.T) { } rebuildDigestAtWidth(t, eb, "ent-A", 8) - rb2, _, _ := eb.GetEntitlementDigestRoot(ctx, entA) + rb2, _, _ := eb.getEntitlementDigestRoot(ctx, entA) if bytes.Equal(ra.Hash, rb2.Hash) { t.Fatal("mutation did not change B's root") } - dirty, err = ea.DirtyEntitlementBuckets(ctx, eb, entA) + dirty, err = ea.dirtyEntitlementBuckets(ctx, eb, entA) if err != nil { t.Fatalf("DirtyEntitlementBuckets (changed): %v", err) } @@ -676,7 +676,7 @@ func TestDigestDifferentWidthsComparison(t *testing.T) { // excludes the known-clean principal. loaded := map[string]bool{} for _, b := range dirty { - if err := eb.IterateGrantsByEntitlementBucket(ctx, entA, b, func(g *v3.GrantRecord) bool { + if err := eb.iterateGrantsByEntitlementBucket(ctx, entA, b, func(g *v3.GrantRecord) bool { loaded[g.GetPrincipal().GetResourceId()] = true return true }); err != nil { @@ -699,7 +699,7 @@ func TestDigestEmptyEntitlementSingleRoot(t *testing.T) { if got := digestNodeCount(t, e); got != 1 { t.Fatalf("empty entitlement: digest node count = %d, want 1 (root only)", got) } - root, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-empty")) + root, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-empty")) if err != nil || !ok { t.Fatalf("GetEntitlementDigestRoot: ok=%v err=%v", ok, err) } @@ -765,18 +765,18 @@ func TestDigestIdenticalGrantsSameRoot(t *testing.T) { seedEntitlement(t, eb, "ent-A", mk()) entA := testEntIdentity("ent-A") - ra, okA, err := ea.GetEntitlementDigestRoot(ctx, entA) + ra, okA, err := ea.getEntitlementDigestRoot(ctx, entA) if err != nil || !okA { t.Fatalf("root A: ok=%v err=%v", okA, err) } - rb, okB, err := eb.GetEntitlementDigestRoot(ctx, entA) + rb, okB, err := eb.getEntitlementDigestRoot(ctx, entA) if err != nil || !okB { t.Fatalf("root B: ok=%v err=%v", okB, err) } if !bytes.Equal(ra.Hash, rb.Hash) { t.Fatalf("identical grants produced different roots:\n A=%x\n B=%x", ra.Hash, rb.Hash) } - dirty, err := ea.DirtyEntitlementBuckets(ctx, eb, entA) + dirty, err := ea.dirtyEntitlementBuckets(ctx, eb, entA) if err != nil { t.Fatalf("DirtyEntitlementBuckets: %v", err) } @@ -806,13 +806,13 @@ func TestDigestContentChangeDirtyBucket(t *testing.T) { seedEntitlement(t, eb, "ent-A", baseB) entA := testEntIdentity("ent-A") - ra, _, _ := ea.GetEntitlementDigestRoot(ctx, entA) - rb, _, _ := eb.GetEntitlementDigestRoot(ctx, entA) + ra, _, _ := ea.getEntitlementDigestRoot(ctx, entA) + rb, _, _ := eb.getEntitlementDigestRoot(ctx, entA) if bytes.Equal(ra.Hash, rb.Hash) { t.Fatal("content change did not change the root hash") } - dirty, err := ea.DirtyEntitlementBuckets(ctx, eb, entA) + dirty, err := ea.dirtyEntitlementBuckets(ctx, eb, entA) if err != nil { t.Fatalf("DirtyEntitlementBuckets: %v", err) } @@ -824,7 +824,7 @@ func TestDigestContentChangeDirtyBucket(t *testing.T) { // principal). found := map[string]bool{} for _, b := range dirty { - if err := eb.IterateGrantsByEntitlementBucket(ctx, entA, b, func(g *v3.GrantRecord) bool { + if err := eb.iterateGrantsByEntitlementBucket(ctx, entA, b, func(g *v3.GrantRecord) bool { found[g.GetPrincipal().GetResourceId()] = true return true }); err != nil { @@ -853,7 +853,7 @@ func TestDigestAddedGrantDirtyBucket(t *testing.T) { seedEntitlement(t, eb, "ent-A", baseB) entA := testEntIdentity("ent-A") - dirty, err := ea.DirtyEntitlementBuckets(ctx, eb, entA) + dirty, err := ea.dirtyEntitlementBuckets(ctx, eb, entA) if err != nil { t.Fatalf("DirtyEntitlementBuckets: %v", err) } @@ -862,7 +862,7 @@ func TestDigestAddedGrantDirtyBucket(t *testing.T) { } found := map[string]bool{} for _, b := range dirty { - if err := eb.IterateGrantsByEntitlementBucket(ctx, entA, b, func(g *v3.GrantRecord) bool { + if err := eb.iterateGrantsByEntitlementBucket(ctx, entA, b, func(g *v3.GrantRecord) bool { found[g.GetPrincipal().GetResourceId()] = true return true }); err != nil { @@ -884,7 +884,7 @@ func TestDigestVariableWidth(t *testing.T) { } es, _ := newTestEngine(t) seedEntitlement(t, es, "ent-small", small) - rootS, ok, err := es.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-small")) + rootS, ok, err := es.getEntitlementDigestRoot(ctx, testEntIdentity("ent-small")) if err != nil || !ok { t.Fatalf("small root: ok=%v err=%v", ok, err) } @@ -908,7 +908,7 @@ func TestDigestVariableWidth(t *testing.T) { } el, _ := newTestEngine(t) seedEntitlement(t, el, "ent-large", large) - rootL, ok, err := el.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-large")) + rootL, ok, err := el.getEntitlementDigestRoot(ctx, testEntIdentity("ent-large")) if err != nil || !ok { t.Fatalf("large root: ok=%v err=%v", ok, err) } @@ -1065,7 +1065,7 @@ func TestDigestLeafFoldConsistent(t *testing.T) { entA := testEntIdentity("ent-A") partition := testEntPartition("ent-A") - root, ok, err := e.GetEntitlementDigestRoot(ctx, entA) + root, ok, err := e.getEntitlementDigestRoot(ctx, entA) if err != nil || !ok { t.Fatalf("root: ok=%v err=%v", ok, err) } @@ -1133,7 +1133,7 @@ func TestDigestLeafFoldConsistent(t *testing.T) { // A stored leaf is a cache of the authoritative fold. b := DigestBucket{Index: leaves[0].idx, Bits: 8} - h, c, err := e.ComputeEntitlementBucketDigest(ctx, entA, b) + h, c, err := e.computeEntitlementBucketDigest(ctx, entA, b) if err != nil { t.Fatal(err) } @@ -1163,7 +1163,7 @@ func TestDigestRebuildClearsStaleNodes(t *testing.T) { if len(before) == 0 { t.Fatal("width-8 build produced no leaves") } - rootBefore, _, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) + rootBefore, _, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) if err != nil { t.Fatal(err) } @@ -1172,7 +1172,7 @@ func TestDigestRebuildClearsStaleNodes(t *testing.T) { t.Fatalf("rebuild at width 4: %v", err) } - rootAfter, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) + rootAfter, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) if err != nil || !ok { t.Fatalf("root after rebuild: ok=%v err=%v", ok, err) } @@ -1213,7 +1213,7 @@ func TestDigestDeleteInvalidatesAndResealRecalculates(t *testing.T) { seedEntitlement(t, e, "ent-A", grants) entA := testEntIdentity("ent-A") - if _, ok, err := e.GetEntitlementDigestRoot(ctx, entA); err != nil || !ok { + if _, ok, err := e.getEntitlementDigestRoot(ctx, entA); err != nil || !ok { t.Fatalf("sealed root: ok=%v err=%v", ok, err) } if rows := entHashIndexRowCount(t, e, "ent-A"); rows != 30 { @@ -1225,7 +1225,7 @@ func TestDigestDeleteInvalidatesAndResealRecalculates(t *testing.T) { if err := e.DeleteGrantRecord(ctx, "g-008"); err != nil { t.Fatalf("DeleteGrantRecord: %v", err) } - if _, ok, err := e.GetEntitlementDigestRoot(ctx, entA); err != nil || ok { + if _, ok, err := e.getEntitlementDigestRoot(ctx, entA); err != nil || ok { t.Fatalf("root after delete: ok=%v err=%v, want missing (invalidated)", ok, err) } if got := digestNodeCount(t, e); got != 0 { @@ -1238,7 +1238,7 @@ func TestDigestDeleteInvalidatesAndResealRecalculates(t *testing.T) { // Reseal: the digest is recalculated from the surviving primaries // and byte-matches an independent from-scratch build. sealGrantDigests(t, e) - root, ok, err := e.GetEntitlementDigestRoot(ctx, entA) + root, ok, err := e.getEntitlementDigestRoot(ctx, entA) if err != nil || !ok { t.Fatalf("root after reseal: ok=%v err=%v", ok, err) } @@ -1282,13 +1282,13 @@ func TestDigestPutInvalidatesOnlyTouchedPartition(t *testing.T) { if err := e.PutGrantRecord(ctx, makeGrant("", "ga2", "ent-A", "dave")); err != nil { t.Fatalf("PutGrantRecord (post-seal): %v", err) } - if _, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-A")); err != nil || ok { + if _, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-A")); err != nil || ok { t.Fatalf("ent-A root after post-seal write: ok=%v err=%v, want missing", ok, err) } if got := entHashIndexRowCount(t, e, "ent-A"); got != 0 { t.Fatalf("ent-A hash index rows after post-seal write = %d, want 0", got) } - rootB, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-B")) + rootB, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-B")) if err != nil || !ok { t.Fatalf("ent-B root: ok=%v err=%v, want intact", ok, err) } @@ -1319,7 +1319,7 @@ func TestSealRebuildDropsStaleIndexRows(t *testing.T) { if err := e.PutGrantRecord(ctx, makeGrant("", "g2", "ent-A", "carol")); err != nil { t.Fatalf("PutGrantRecord: %v", err) } - if err := e.DeleteGrantByIdentityRefs(ctx, makeGrant("", "g2", "ent-A", "bob")); err != nil { + if err := e.deleteGrantByIdentityRefs(ctx, makeGrant("", "g2", "ent-A", "bob")); err != nil { t.Fatalf("DeleteGrantByIdentityRefs: %v", err) } sealGrantDigests(t, e) @@ -1345,7 +1345,7 @@ func TestSealRebuildDropsStaleIndexRows(t *testing.T) { if principals["bob"] || !principals["carol"] || !principals["alice"] || len(principals) != 2 { t.Fatalf("index principals after reseal = %v, want {alice, carol}", principals) } - root, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) + root, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) if err != nil || !ok { t.Fatalf("root: ok=%v err=%v", ok, err) } @@ -1465,7 +1465,7 @@ func TestGrantDigestSpillMerge(t *testing.T) { } requireSameDigestNodes(t, spilledNodes, memNodes) - root, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-big")) + root, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-big")) if err != nil || !ok { t.Fatalf("root: ok=%v err=%v", ok, err) } @@ -1505,13 +1505,13 @@ func TestDigestMissingRootWholeDirty(t *testing.T) { t.Fatal(err) } } - if _, ok, err := eb.GetEntitlementDigestRoot(ctx, entA); err != nil || ok { + if _, ok, err := eb.getEntitlementDigestRoot(ctx, entA); err != nil || ok { t.Fatalf("B unexpectedly has a root: ok=%v err=%v", ok, err) } for name, dirtyFn := range map[string]func() ([]DigestBucket, error){ - "A vs B": func() ([]DigestBucket, error) { return ea.DirtyEntitlementBuckets(ctx, eb, entA) }, - "B vs A": func() ([]DigestBucket, error) { return eb.DirtyEntitlementBuckets(ctx, ea, entA) }, + "A vs B": func() ([]DigestBucket, error) { return ea.dirtyEntitlementBuckets(ctx, eb, entA) }, + "B vs A": func() ([]DigestBucket, error) { return eb.dirtyEntitlementBuckets(ctx, ea, entA) }, } { dirty, err := dirtyFn() if err != nil { diff --git a/pkg/dotc1z/engine/pebble/discovered_at_verify_provenance_test.go b/pkg/dotc1z/engine/pebble/discovered_at_verify_provenance_test.go index 48c02889f..8e1b6c2c2 100644 --- a/pkg/dotc1z/engine/pebble/discovered_at_verify_provenance_test.go +++ b/pkg/dotc1z/engine/pebble/discovered_at_verify_provenance_test.go @@ -76,7 +76,7 @@ func TestV3ReadProvenanceClasses(t *testing.T) { Principal: v3.PrincipalRef_builder{ResourceTypeId: "user", ResourceId: "alice"}.Build(), DiscoveredAt: timestamppb.New(explicit), }.Build() - require.NoError(t, e.PutSynthesizedGrantRecords(ctx, []*v3.GrantRecord{rec})) + require.NoError(t, e.putSynthesizedGrantRecords(ctx, []*v3.GrantRecord{rec})) got := readBack(t, r, "g-synth-explicit") require.NotNil(t, got.GetDiscoveredAt()) @@ -97,7 +97,7 @@ func TestV3ReadProvenanceClasses(t *testing.T) { Principal: v3.PrincipalRef_builder{ResourceTypeId: "user", ResourceId: "bob"}.Build(), }.Build() require.Nil(t, rec.GetDiscoveredAt()) - require.NoError(t, e.PutSynthesizedGrantRecords(ctx, []*v3.GrantRecord{rec})) + require.NoError(t, e.putSynthesizedGrantRecords(ctx, []*v3.GrantRecord{rec})) after := time.Now().Add(time.Second) got := readBack(t, r, "g-synth-nil") diff --git a/pkg/dotc1z/engine/pebble/endsync_repair_test.go b/pkg/dotc1z/engine/pebble/endsync_repair_test.go index e0d57e481..664b860e9 100644 --- a/pkg/dotc1z/engine/pebble/endsync_repair_test.go +++ b/pkg/dotc1z/engine/pebble/endsync_repair_test.go @@ -112,10 +112,10 @@ func TestEndSyncSecondCallTakesTargetedRepairPath(t *testing.T) { // Sanity: only ent-A (and the global root) should be invalidated by // that write. - if _, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-A")); err != nil || ok { + if _, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-A")); err != nil || ok { t.Fatalf("ent-A root after post-seal write: ok=%v err=%v, want missing", ok, err) } - rootB, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-B")) + rootB, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-B")) if err != nil || !ok { t.Fatalf("ent-B root after post-seal write: ok=%v err=%v, want intact", ok, err) } @@ -164,7 +164,7 @@ func TestEndSyncSecondCallTakesTargetedRepairPath(t *testing.T) { // ent-A must be correctly repaired (21 grants now), and the global // root must be recomputed and correct. - rootA, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) + rootA, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) if err != nil || !ok { t.Fatalf("ent-A root after repair: ok=%v err=%v", ok, err) } diff --git a/pkg/dotc1z/engine/pebble/engine.go b/pkg/dotc1z/engine/pebble/engine.go index 6e702cf08..e6201cb0f 100644 --- a/pkg/dotc1z/engine/pebble/engine.go +++ b/pkg/dotc1z/engine/pebble/engine.go @@ -332,7 +332,7 @@ func (e *Engine) Close() error { // (which run under withWrite) can be touching the session concurrently // — Abort itself takes no write barrier, only synthLayerMu for the // pointer handoff, and is a no-op when no session is open. - _ = e.AbortSynthesizedGrantLayer(context.Background()) + _ = e.abortSynthesizedGrantLayer(context.Background()) // Hold writeMu for the teardown: writeWG only covers withWrite users, // while CheckpointTo takes writeMu directly (no WG participation). A // CheckpointTo that passed its closing check but hasn't locked yet must @@ -429,8 +429,8 @@ func (e *Engine) unseal() { e.resumeCompactions() } -// IsSealed reports whether the engine is in the post-EndSync sealed state. -func (e *Engine) IsSealed() bool { +// isSealed reports whether the engine is in the post-EndSync sealed state. +func (e *Engine) isSealed() bool { return e.sealed.Load() } @@ -474,9 +474,9 @@ func (e *Engine) clearCurrentSync() { e.currentSyncMu.Unlock() } -// IsFreshSync reports whether the engine is in the fresh-sync write +// isFreshSync reports whether the engine is in the fresh-sync write // path (set by MarkFreshSync). -func (e *Engine) IsFreshSync() bool { +func (e *Engine) isFreshSync() bool { e.currentSyncMu.RLock() defer e.currentSyncMu.RUnlock() return e.freshSync @@ -559,14 +559,14 @@ func (e *Engine) currentSyncBytes() []byte { return out } -// CurrentSyncID returns the bound sync's id string, or "" when no sync +// currentSyncID returns the bound sync's id string, or "" when no sync // is bound. THE single source of truth for "which sync is open" — the // old Adapter-level syncRunState cache that shadowed it was deleted // (PR 2.6): lifecycle readers decode this binding, and everything else // about the open sync (step token, type, parent) is read from the // durable SyncRunRecord on demand, exactly like the SQLite engine's // row-backed reads. -func (e *Engine) CurrentSyncID() string { +func (e *Engine) currentSyncID() string { e.currentSyncMu.RLock() defer e.currentSyncMu.RUnlock() return codec.DecodeSyncID(e.currentSync) @@ -658,13 +658,11 @@ func (e *Engine) withWriteAllowSealed(fn func() error) error { return fn() } -func (e *Engine) Save(ctx context.Context, dest string) error { - return errors.New("pebble engine: Save requires the dotc1z.Save shim (envelope write); use CheckpointTo for direct directory access") -} - -// DBDir returns the on-disk path the engine writes to. Exported so -// the Adapter can implement OutputFilepath / CurrentDBSizeBytes. -func (e *Engine) DBDir() string { +// databaseDir returns the on-disk path the engine writes to. The engine +// writes a directory, not an envelope; wrapping it into a .c1z is the +// store's job (see pebbleStore.save), and CheckpointTo is the way to get +// a consistent copy of this directory. +func (e *Engine) databaseDir() string { return e.dbDir } diff --git a/pkg/dotc1z/engine/pebble/engine_test.go b/pkg/dotc1z/engine/pebble/engine_test.go index 902925f89..7aa4ca954 100644 --- a/pkg/dotc1z/engine/pebble/engine_test.go +++ b/pkg/dotc1z/engine/pebble/engine_test.go @@ -315,21 +315,6 @@ func TestCheckpointTo(t *testing.T) { require.NoError(t, err, "expected Put after CheckpointTo to succeed") } -func TestSaveDoesNotCloseOnError(t *testing.T) { - ctx := context.Background() - e, dir := newTestEngine(t) - syncID := ksuid.New().String() - err := e.bindCurrentSync(syncID) - require.NoError(t, err) - - err = e.Save(ctx, filepath.Join(dir, "out.c1z3")) - require.Error(t, err, "expected Save error") - - r := makeGrant(syncID, "after-save", "e1", "p1") - err = e.PutGrantRecord(ctx, r) - require.NoError(t, err, "expected Put after failed Save to succeed") -} - func TestConcurrentGrantOverwriteIndexes(t *testing.T) { ctx := context.Background() e, _ := newTestEngine(t) diff --git a/pkg/dotc1z/engine/pebble/entitlements.go b/pkg/dotc1z/engine/pebble/entitlements.go index e9e728e26..127a4e22a 100644 --- a/pkg/dotc1z/engine/pebble/entitlements.go +++ b/pkg/dotc1z/engine/pebble/entitlements.go @@ -34,7 +34,7 @@ func (e *Engine) PutEntitlementRecords(ctx context.Context, records ...*v3.Entit priBatch := e.db.NewRecordBatch() defer priBatch.Close() - fresh := e.IsFreshSync() + fresh := e.isFreshSync() type dedupKey struct { id entitlementIdentity @@ -107,10 +107,10 @@ func (e *Engine) GetEntitlementRecord(ctx context.Context, externalID string) (* return r, nil } -// DeleteEntitlementRecord deletes by raw public id. A missing id is a +// deleteEntitlementRecord deletes by raw public id. A missing id is a // no-op; an ambiguous id is an error (a lossy string must never guess a // delete). -func (e *Engine) DeleteEntitlementRecord(ctx context.Context, externalID string) error { +func (e *Engine) deleteEntitlementRecord(ctx context.Context, externalID string) error { return e.withWrite(func() error { id, err := e.resolveEntitlementIdentityByExternalID(ctx, externalID) if err != nil { diff --git a/pkg/dotc1z/engine/pebble/errorfs_sweep_test.go b/pkg/dotc1z/engine/pebble/errorfs_sweep_test.go index 723bc3ac7..d0a5ed434 100644 --- a/pkg/dotc1z/engine/pebble/errorfs_sweep_test.go +++ b/pkg/dotc1z/engine/pebble/errorfs_sweep_test.go @@ -447,7 +447,7 @@ func (w sweepWorkload) verifyDigests(ctx context.Context, t *testing.T, e *Engin // Consistent absence: no entitlement root may survive a drop. for _, entID := range w.expectedEntIDsPerPrincipal() { id := entitlementIdentityFromParts("app", "github", entID) - _, ok, err := e.GetEntitlementDigestRoot(ctx, id) + _, ok, err := e.getEntitlementDigestRoot(ctx, id) require.NoErrorf(t, err, "%s: GetEntitlementDigestRoot(%s)", label, entID) require.Falsef(t, ok, "%s: global digest root absent but %s kept a root — partial digest state lies to the repair fast path", label, entID) } @@ -482,7 +482,7 @@ func (w sweepWorkload) verifyDigests(ctx context.Context, t *testing.T, e *Engin id := entitlementIdentityFromParts("app", "github", entID) part := digestPartitionForEntitlement(id) - root, ok, err := e.GetEntitlementDigestRoot(ctx, id) + root, ok, err := e.getEntitlementDigestRoot(ctx, id) require.NoErrorf(t, err, "%s: GetEntitlementDigestRoot(%s)", label, entID) require.Truef(t, ok, "%s: finished store must have a digest root for %s (present-means-exact)", label, entID) @@ -491,7 +491,7 @@ func (w sweepWorkload) verifyDigests(ctx context.Context, t *testing.T, e *Engin require.Equalf(t, pf.count, root.Count, "%s: stored root count vs primary fold for %s", label, entID) require.Equalf(t, pf.xor[:], root.Hash, "%s: stored root hash vs primary fold for %s", label, entID) - idxHash, idxCount, err := e.ComputeEntitlementBucketDigest(ctx, id, DigestBucket{}) + idxHash, idxCount, err := e.computeEntitlementBucketDigest(ctx, id, DigestBucket{}) require.NoErrorf(t, err, "%s: ComputeEntitlementBucketDigest(%s)", label, entID) require.Equalf(t, root.Count, idxCount, "%s: hash-index fold count vs stored root for %s", label, entID) require.Equalf(t, root.Hash, idxHash, "%s: hash-index fold hash vs stored root for %s", label, entID) diff --git a/pkg/dotc1z/engine/pebble/get_grant_sync_resolution_test.go b/pkg/dotc1z/engine/pebble/get_grant_sync_resolution_test.go index c64d9915d..d8ace786a 100644 --- a/pkg/dotc1z/engine/pebble/get_grant_sync_resolution_test.go +++ b/pkg/dotc1z/engine/pebble/get_grant_sync_resolution_test.go @@ -38,7 +38,7 @@ func TestGetGrantResolvesSyncAfterEndSync(t *testing.T) { require.NoError(t, e.EndSync(ctx)) // The binding is cleared by EndSync; the sync_id is still on disk. - require.Empty(t, e.CurrentSyncID(), "EndSync should clear the in-memory binding") + require.Empty(t, e.currentSyncID(), "EndSync should clear the in-memory binding") // v2 GetGrant resolves from the persisted sync record. v2Resp, err := e.GetGrant(ctx, reader_v2.GrantsReaderServiceGetGrantRequest_builder{GrantId: "g-1"}.Build()) diff --git a/pkg/dotc1z/engine/pebble/grant_digest.go b/pkg/dotc1z/engine/pebble/grant_digest.go index b173e5de1..d45ab8bf1 100644 --- a/pkg/dotc1z/engine/pebble/grant_digest.go +++ b/pkg/dotc1z/engine/pebble/grant_digest.go @@ -340,7 +340,7 @@ func grantPrimaryKeyFromHashIndexKey(dst, idxKey []byte) ([]byte, bool) { // --- Engine API --- -// GetEntitlementDigestRoot returns the stored grant-digest root for an +// getEntitlementDigestRoot returns the stored grant-digest root for an // entitlement. ok is false when no digest has been built for it (or it // was invalidated) — which means the caller must re-read the // entitlement's grants (or treat the whole entitlement as dirty), NOT @@ -349,7 +349,7 @@ func grantPrimaryKeyFromHashIndexKey(dst, idxKey []byte) ([]byte, bool) { // nodes, so with no root it is absent too and the fold would report // "zero grants" for an entitlement that may have millions — the // false-clean trap dirtyPartitionBuckets' doc comment describes. -func (e *Engine) GetEntitlementDigestRoot(ctx context.Context, id entitlementIdentity) (DigestRoot, bool, error) { +func (e *Engine) getEntitlementDigestRoot(ctx context.Context, id entitlementIdentity) (DigestRoot, bool, error) { return e.getPartitionDigestRoot(grantDigestSpec, digestPartitionForEntitlement(id)) } @@ -386,7 +386,7 @@ func (e *Engine) GetGrantDigestGlobalRoot(ctx context.Context) (DigestRoot, bool return DigestRoot{Hash: out, Count: count}, true, nil } -// ComputeEntitlementBucketDigest folds the grant hash index over a +// computeEntitlementBucketDigest folds the grant hash index over a // single bucket of an entitlement (the zero bucket = the whole // entitlement) — the authoritative on-demand counterpart of the stored // digest nodes, for verifying or subdividing a digest that EXISTS. @@ -395,27 +395,27 @@ func (e *Engine) GetGrantDigestGlobalRoot(ctx context.Context) (DigestRoot, bool // against a never-built or invalidated entitlement this folds an // absent index range and returns {0, 0} — "zero grants", not "unknown". // Never use it as a fallback for a missing root; see -// GetEntitlementDigestRoot and computeBucketDigest's precondition. -func (e *Engine) ComputeEntitlementBucketDigest(ctx context.Context, id entitlementIdentity, bucket DigestBucket) ([]byte, int64, error) { +// getEntitlementDigestRoot and computeBucketDigest's precondition. +func (e *Engine) computeEntitlementBucketDigest(ctx context.Context, id entitlementIdentity, bucket DigestBucket) ([]byte, int64, error) { return e.computeBucketDigest(ctx, grantDigestSpec, digestPartitionForEntitlement(id), bucket) } -// DirtyEntitlementBuckets compares this engine's entitlement against +// dirtyEntitlementBuckets compares this engine's entitlement against // other's and returns the buckets whose grants differ — see // dirtyPartitionBuckets for the comparison contract (zero bucket = // whole entitlement; nil = identical). -func (e *Engine) DirtyEntitlementBuckets(ctx context.Context, other *Engine, id entitlementIdentity) ([]DigestBucket, error) { +func (e *Engine) dirtyEntitlementBuckets(ctx context.Context, other *Engine, id entitlementIdentity) ([]DigestBucket, error) { return e.dirtyPartitionBuckets(ctx, grantDigestSpec, other, digestPartitionForEntitlement(id)) } -// IterateGrantsByEntitlementBucket yields the grants in one +// iterateGrantsByEntitlementBucket yields the grants in one // principal-hash bucket of an entitlement (the zero bucket = the whole // entitlement). This is the dirty-bucket loader: after a digest // comparison flags a bucket, the caller materializes only those grants. // The primary key is reconstructed from each index key by byte splice // (no decode); the point Get per entry is the cost of MATERIALIZING a // changed grant, not of finding it. Orphan index entries are skipped. -func (e *Engine) IterateGrantsByEntitlementBucket(ctx context.Context, id entitlementIdentity, bucket DigestBucket, yield func(*v3.GrantRecord) bool) error { +func (e *Engine) iterateGrantsByEntitlementBucket(ctx context.Context, id entitlementIdentity, bucket DigestBucket, yield func(*v3.GrantRecord) bool) error { lower, upper := grantDigestSpec.bucketBounds(digestPartitionForEntitlement(id), bucket) iter, err := e.db.NewIter(&pebble.IterOptions{LowerBound: lower, UpperBound: upper}) if err != nil { @@ -452,18 +452,6 @@ func (e *Engine) IterateGrantsByEntitlementBucket(ctx context.Context, id entitl return iter.Error() } -// DropAllGrantDigests removes every stored digest node. Called when a -// seal-time build fails partway: a partially built digest that LOOKS -// present would violate the present-means-exact contract, whereas -// absent digests just make readers re-read the grants until the next -// successful seal recalculates them. -func (e *Engine) DropAllGrantDigests(ctx context.Context) error { - return e.withWrite(func() error { - e.db.SetGrantDigestsPresent(false) - return e.db.DropKeyRange(DigestLowerBound(), DigestUpperBound(), writeOpts(e.opts.durability)) - }) -} - // DropAllGrantDigestState removes every stored digest node AND the // whole by_entitlement_principal_hash index, and clears the engine's // digests-present flag. For callers that are about to mutate grants diff --git a/pkg/dotc1z/engine/pebble/grant_digest_build.go b/pkg/dotc1z/engine/pebble/grant_digest_build.go index 8043f53ba..ee4ac6176 100644 --- a/pkg/dotc1z/engine/pebble/grant_digest_build.go +++ b/pkg/dotc1z/engine/pebble/grant_digest_build.go @@ -161,7 +161,7 @@ type grantDigestFold struct { func newGrantDigestFold(e *Engine) (*grantDigestFold, error) { opts := writeOpts(e.opts.durability) - if e.IsFreshSync() { + if e.isFreshSync() { // EndSync's EndFreshSync flush is the durability boundary for // seal-time writes; matches the deferred pass's other writes. opts = pebble.NoSync @@ -464,7 +464,7 @@ func (e *Engine) buildGrantDigestsFromSpill(ctx context.Context, dir string, has return err } opts := writeOpts(e.opts.durability) - if e.IsFreshSync() { + if e.isFreshSync() { opts = pebble.NoSync } // Arm the durable crash marker before the first digest write on @@ -730,7 +730,7 @@ func (e *Engine) buildGrantDigestsStandaloneLocked(ctx context.Context) error { func (e *Engine) dropAllGrantDigestStateLocked() error { e.db.SetGrantDigestsPresent(false) opts := writeOpts(e.opts.durability) - if e.IsFreshSync() { + if e.isFreshSync() { opts = pebble.NoSync } if err := e.db.DropKeyRange(DigestLowerBound(), DigestUpperBound(), opts); err != nil { diff --git a/pkg/dotc1z/engine/pebble/grant_digest_global_root_test.go b/pkg/dotc1z/engine/pebble/grant_digest_global_root_test.go index 90cbbd7cf..4564e7fcc 100644 --- a/pkg/dotc1z/engine/pebble/grant_digest_global_root_test.go +++ b/pkg/dotc1z/engine/pebble/grant_digest_global_root_test.go @@ -41,7 +41,7 @@ func TestGrantDigestGlobalRootMatchesFold(t *testing.T) { var wantXor [hashLen]byte var wantCount int64 for entID := range counts { - digest, count, err := e.ComputeEntitlementBucketDigest(ctx, testEntIdentity(entID), DigestBucket{}) + digest, count, err := e.computeEntitlementBucketDigest(ctx, testEntIdentity(entID), DigestBucket{}) if err != nil { t.Fatalf("ComputeEntitlementBucketDigest(%s): %v", entID, err) } @@ -177,7 +177,7 @@ func TestManifestGrantDigestRootAbsentWithoutDigests(t *testing.T) { if err := e.PutGrantRecords(ctx, makeGrant("", "g1", "ent-A", "alice")); err != nil { t.Fatalf("PutGrantRecords: %v", err) } - if err := e.BuildDeferredGrantIndexes(ctx); err != nil { + if err := e.buildDeferredGrantIndexes(ctx); err != nil { t.Fatalf("BuildDeferredGrantIndexes: %v", err) } diff --git a/pkg/dotc1z/engine/pebble/grant_digest_hash_test.go b/pkg/dotc1z/engine/pebble/grant_digest_hash_test.go index 4aa83cf86..51a12e3fb 100644 --- a/pkg/dotc1z/engine/pebble/grant_digest_hash_test.go +++ b/pkg/dotc1z/engine/pebble/grant_digest_hash_test.go @@ -249,7 +249,7 @@ func TestGrantDigestAccumulatorMatchesSealedRoots(t *testing.T) { require.NoError(t, acc.Add(g)) require.NoError(t, global.Add(g)) } - want, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity(entID)) + want, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity(entID)) require.NoError(t, err) require.True(t, ok, "digest root present for %s", entID) got := acc.Root() diff --git a/pkg/dotc1z/engine/pebble/grant_digest_repair.go b/pkg/dotc1z/engine/pebble/grant_digest_repair.go index ef93fb070..df2a30bc5 100644 --- a/pkg/dotc1z/engine/pebble/grant_digest_repair.go +++ b/pkg/dotc1z/engine/pebble/grant_digest_repair.go @@ -122,7 +122,7 @@ func (e *Engine) InvalidateGrantDigestPartitions(ctx context.Context, partitions return err } opts := writeOpts(e.opts.durability) - if e.IsFreshSync() { + if e.isFreshSync() { opts = pebble.NoSync } return batch.Commit(opts) @@ -422,7 +422,7 @@ func (e *Engine) repairOneGrantDigestPartitionLocked(ctx context.Context, partit lower, upper := grantPrimaryEntitlementBoundsFromPartition(partition) opts := writeOpts(e.opts.durability) - if e.IsFreshSync() { + if e.isFreshSync() { opts = pebble.NoSync } flushBytes := digestNodeBatchFlushBytes @@ -580,7 +580,7 @@ func (e *Engine) recomputeGrantDigestGlobalRootLocked(ctx context.Context) error return err } opts := writeOpts(e.opts.durability) - if e.IsFreshSync() { + if e.isFreshSync() { opts = pebble.NoSync } if err := e.db.DigestSet(rawdb.GlobalGrantDigestNodeKey(), packDigestLeaf(total, xor[:]), opts); err != nil { diff --git a/pkg/dotc1z/engine/pebble/grant_digest_repair_test.go b/pkg/dotc1z/engine/pebble/grant_digest_repair_test.go index ab30a648f..6297344e7 100644 --- a/pkg/dotc1z/engine/pebble/grant_digest_repair_test.go +++ b/pkg/dotc1z/engine/pebble/grant_digest_repair_test.go @@ -70,10 +70,10 @@ func TestRepairMissingGrantDigestsLeavesGlobalRootMissingOnPartialFailure(t *tes t.Fatalf("RepairMissingGrantDigests must not fail the caller: %v", err) } - if _, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-good")); err != nil || !ok { + if _, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-good")); err != nil || !ok { t.Fatalf("ent-good root: ok=%v err=%v, want repaired", ok, err) } - if _, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-bad")); err != nil || ok { + if _, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-bad")); err != nil || ok { t.Fatalf("ent-bad root: ok=%v err=%v, want still missing (repair failed)", ok, err) } if _, ok, err := e.GetGrantDigestGlobalRoot(ctx); err != nil || ok { @@ -117,14 +117,14 @@ func TestRepairMissingGrantDigestsHealsOnlyInvalidatedPartition(t *testing.T) { // Sanity: exactly ent-b and the global root went missing; ent-a and // ent-c are untouched. - if _, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-b")); err != nil || ok { + if _, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-b")); err != nil || ok { t.Fatalf("ent-b root after invalidate: ok=%v err=%v, want missing", ok, err) } if _, ok, err := e.GetGrantDigestGlobalRoot(ctx); err != nil || ok { t.Fatalf("global root after invalidate: ok=%v err=%v, want missing", ok, err) } for _, entID := range []string{"ent-a", "ent-c"} { - if _, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity(entID)); err != nil || !ok { + if _, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity(entID)); err != nil || !ok { t.Fatalf("%s root after invalidate: ok=%v err=%v, want intact", entID, ok, err) } } @@ -165,7 +165,7 @@ func TestRepairMissingGrantDigestsRediscoversInvalidatedOrphan(t *testing.T) { t.Fatalf("PutGrantRecords: %v", err) } sealGrantDigests(t, e) - if _, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-orphan")); err != nil || !ok { + if _, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-orphan")); err != nil || !ok { t.Fatalf("orphan root after seal: ok=%v err=%v, want present (seal covers orphans)", ok, err) } want := dumpDigestNodes(t, e) @@ -247,7 +247,7 @@ func TestRepairMissingGrantDigestsFallsBackWhenNeverBuilt(t *testing.T) { t.Fatalf("RepairMissingGrantDigests: %v", err) } - root, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) + root, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-A")) if err != nil || !ok { t.Fatalf("ent-A root: ok=%v err=%v", ok, err) } @@ -372,7 +372,7 @@ func TestRepairMissingGrantDigestsCountsMalformedKeys(t *testing.T) { t.Fatalf("RepairMissingGrantDigests: %v", err) } - root, ok, err := e.GetEntitlementDigestRoot(ctx, testEntIdentity("ent-a")) + root, ok, err := e.getEntitlementDigestRoot(ctx, testEntIdentity("ent-a")) if err != nil || !ok { t.Fatalf("ent-a root after repair: ok=%v err=%v, want repaired", ok, err) } diff --git a/pkg/dotc1z/engine/pebble/grant_read_arena.go b/pkg/dotc1z/engine/pebble/grant_read_arena.go index 84a349667..4ea273ca8 100644 --- a/pkg/dotc1z/engine/pebble/grant_read_arena.go +++ b/pkg/dotc1z/engine/pebble/grant_read_arena.go @@ -22,7 +22,7 @@ import ( // into 3 slice allocations sized to the page limit. // // Lifetime. The arena lives for the duration of one -// PaginateGrants call. The returned []*v3.GrantRecord +// paginateGrants call. The returned []*v3.GrantRecord // elements all point into the arena's backing arrays; the caller // retains those pointers (the engine returns them up through the // adapter). Go's GC keeps the backing arrays alive as long as any diff --git a/pkg/dotc1z/engine/pebble/grant_read_arena_test.go b/pkg/dotc1z/engine/pebble/grant_read_arena_test.go index d7abc827d..165004d48 100644 --- a/pkg/dotc1z/engine/pebble/grant_read_arena_test.go +++ b/pkg/dotc1z/engine/pebble/grant_read_arena_test.go @@ -37,7 +37,7 @@ func TestGrantReadArenaReconcileAbsent(t *testing.T) { principalID: "alice", }), val, pebble.NoSync), "raw set") - got, _, err := e.PaginateGrants(ctx, "", 0) + got, _, err := e.paginateGrants(ctx, "", 0) require.NoError(t, err, "PaginateGrantsBySync") require.Len(t, got, 1) require.Nil(t, got[0].GetEntitlement(), "GetEntitlement()") @@ -59,7 +59,7 @@ func TestGrantReadArenaPopulatedRoundtrip(t *testing.T) { require.NoError(t, e.PutGrantRecord(ctx, makeGrant(syncID, "g-"+ksuid.New().String(), "ent-A", "alice-"+strconv.Itoa(i))), "PutGrantRecord") } - got, _, err := e.PaginateGrants(ctx, "", n) + got, _, err := e.paginateGrants(ctx, "", n) require.NoError(t, err, "PaginateGrantsBySync") require.Len(t, got, n) for _, g := range got { diff --git a/pkg/dotc1z/engine/pebble/grant_write_scale_bench_test.go b/pkg/dotc1z/engine/pebble/grant_write_scale_bench_test.go index 2e19db895..97f556e86 100644 --- a/pkg/dotc1z/engine/pebble/grant_write_scale_bench_test.go +++ b/pkg/dotc1z/engine/pebble/grant_write_scale_bench_test.go @@ -90,7 +90,7 @@ func benchmarkGrantWriteScale(b *testing.B, putUnique, grantIndex bool) { } recs := makeGrantRecordBatch(benchGrantSyncID, written, m) if putUnique { - require.NoError(b, e.UnsafePutUniqueGrantRecords(ctx, recs...)) + require.NoError(b, e.unsafePutUniqueGrantRecords(ctx, recs...)) } else { require.NoError(b, e.PutGrantRecords(ctx, recs...)) } diff --git a/pkg/dotc1z/engine/pebble/grants.go b/pkg/dotc1z/engine/pebble/grants.go index a603dfcab..36fd92072 100644 --- a/pkg/dotc1z/engine/pebble/grants.go +++ b/pkg/dotc1z/engine/pebble/grants.go @@ -80,7 +80,7 @@ func (e *Engine) PutGrantRecords(ctx context.Context, records ...*v3.GrantRecord batch := e.db.NewRecordBatch() defer batch.Close() - fresh := e.IsFreshSync() + fresh := e.isFreshSync() // skipGet fires exactly once per fresh sync — only the first // PutGrantRecords call sees the keyspace empty by construction. // Subsequent calls in the same fresh sync still need @@ -162,7 +162,7 @@ func (e *Engine) PutGrantRecords(ctx context.Context, records ...*v3.GrantRecord }) } -// PutExpandedGrantRecords is the grant-expander write path — the +// putExpandedGrantRecords is the grant-expander write path — the // engine side of GrantStore.StoreExpandedGrants, and its only caller. // // Two properties distinguish it from PutGrantRecords: @@ -187,7 +187,7 @@ func (e *Engine) PutGrantRecords(ctx context.Context, records ...*v3.GrantRecord // records arrive as freshly translated v3 GrantRecords with NO // preservation or discovered_at stamping applied; this method performs // the merge so the read it already issues does double duty. -func (e *Engine) PutExpandedGrantRecords(ctx context.Context, records []*v3.GrantRecord) error { +func (e *Engine) putExpandedGrantRecords(ctx context.Context, records []*v3.GrantRecord) error { if len(records) == 0 { return nil } @@ -299,12 +299,12 @@ func (e *Engine) PutExpandedGrantRecords(ctx context.Context, records []*v3.Gran }) } -// PutSynthesizedGrantRecords writes expander-synthesized grants that the caller +// putSynthesizedGrantRecords writes expander-synthesized grants that the caller // guarantees are brand-new by structured grant identity. It skips the // read-before-write Get in PutExpandedGrantRecords because there is no prior // value whose Expansion/NeedsExpansion/DiscoveredAt or index entries must be // preserved/cleaned. -func (e *Engine) PutSynthesizedGrantRecords(ctx context.Context, records []*v3.GrantRecord) error { +func (e *Engine) putSynthesizedGrantRecords(ctx context.Context, records []*v3.GrantRecord) error { if len(records) == 0 { return nil } @@ -361,12 +361,12 @@ type synthesizedGrantRecord struct { sources batonGrant.Sources } -// PutSynthesizedGrantContributions batch-writes one destination's synthesized +// putSynthesizedGrantContributions batch-writes one destination's synthesized // contributions. It is the fallback for stores/engines that cannot run a // layer-scoped layer session (see BeginSynthesizedGrantLayer); the layer path // is preferred because it publishes sorted SSTs instead of out-of-order batch // commits. -func (e *Engine) PutSynthesizedGrantContributions(ctx context.Context, records []synthesizedGrantRecord) error { +func (e *Engine) putSynthesizedGrantContributions(ctx context.Context, records []synthesizedGrantRecord) error { if len(records) == 0 { return nil } @@ -476,12 +476,12 @@ func (s *synthGrantLayerSession) cutSegment() error { return nil } -// BeginSynthesizedGrantLayer opens a layer-scoped layer session. The ingested +// beginSynthesizedGrantLayer opens a layer-scoped layer session. The ingested // SSTs carry primary rows only; the by_principal index is always rebuilt at // EndSync, so no inline index maintenance is skipped by taking this path. // The boolean is part of the store-level contract (non-Pebble stores report // false and callers fall back to StoreNewExpandedGrantContributions). -func (e *Engine) BeginSynthesizedGrantLayer(ctx context.Context) (bool, error) { +func (e *Engine) beginSynthesizedGrantLayer(ctx context.Context) (bool, error) { if err := e.checkWritable(); err != nil { return false, err } @@ -584,10 +584,10 @@ func (e *Engine) ingestSynthLayerSegment(ctx context.Context, dir string, seg sy return nil } -// AddSynthesizedGrantLayerContributions encodes records into the open layer +// addSynthesizedGrantLayerContributions encodes records into the open layer // session. Rows become readable as their segment is ingested; callers must // not rely on visibility before FinishSynthesizedGrantLayer returns. -func (e *Engine) AddSynthesizedGrantLayerContributions(ctx context.Context, records []synthesizedGrantRecord) error { +func (e *Engine) addSynthesizedGrantLayerContributions(ctx context.Context, records []synthesizedGrantRecord) error { if len(records) == 0 { return nil } @@ -651,10 +651,10 @@ func (e *Engine) AddSynthesizedGrantLayerContributions(ctx context.Context, reco }) } -// FinishSynthesizedGrantLayer flushes the session's tail segment, waits for +// finishSynthesizedGrantLayer flushes the session's tail segment, waits for // the background worker to merge and ingest every queued segment, and closes // the session. No-op if no session is open or the session saw no rows. -func (e *Engine) FinishSynthesizedGrantLayer(ctx context.Context) error { +func (e *Engine) finishSynthesizedGrantLayer(ctx context.Context) error { return e.withWrite(func() error { s := e.takeSynthLayer() if s == nil { @@ -682,7 +682,7 @@ func (e *Engine) FinishSynthesizedGrantLayer(ctx context.Context) error { }) } -// AbortSynthesizedGrantLayer discards an in-flight layer session: already +// abortSynthesizedGrantLayer discards an in-flight layer session: already // ingested segments remain in the DB (their rows are idempotent overwrites on // retry), staged chunks are dropped. Safe to call with no open session. // @@ -690,7 +690,7 @@ func (e *Engine) FinishSynthesizedGrantLayer(ctx context.Context) error { // setting the closing flag (withWrite would refuse), and it must stay callable // as a cleanup path when a writer holding writeMu panicked. The synthLayerMu // take keeps the pointer handoff race-free against Begin/Add/Finish/Close. -func (e *Engine) AbortSynthesizedGrantLayer(ctx context.Context) error { +func (e *Engine) abortSynthesizedGrantLayer(ctx context.Context) error { s := e.takeSynthLayer() if s == nil { return nil @@ -757,7 +757,7 @@ func (e *Engine) putSynthesizedGrantContributionsBatch(ctx context.Context, reco }) } -// UnsafePutUniqueGrantRecords is the trusted-import write path: it writes +// unsafePutUniqueGrantRecords is the trusted-import write path: it writes // records unconditionally, with NO read-before-write and NO dedup pass. Do not // use it for live connector output. The engine must currently be in fresh-sync // mode, and the caller must guarantee each external_id appears at most once @@ -771,12 +771,12 @@ func (e *Engine) putSynthesizedGrantContributionsBatch(ctx context.Context, reco // write only exists to clean up stale index entries when an external_id is // rewritten within a sync — impossible when the caller guarantees global // uniqueness for the imported sync. -func (e *Engine) UnsafePutUniqueGrantRecords(ctx context.Context, records ...*v3.GrantRecord) error { +func (e *Engine) unsafePutUniqueGrantRecords(ctx context.Context, records ...*v3.GrantRecord) error { if len(records) == 0 { return nil } return e.withWrite(func() error { - if !e.IsFreshSync() { + if !e.isFreshSync() { return errors.New("UnsafePutUniqueGrantRecords: sync is not fresh") } // Fail-loud defense (review suggestion): on the fresh syncs this @@ -881,7 +881,7 @@ func (e *Engine) UnsafePutUniqueGrantRecords(ctx context.Context, records ...*v3 } opts := writeOpts(e.opts.durability) - if e.IsFreshSync() { + if e.isFreshSync() { opts = pebble.NoSync } // One atomic commit: rows and their obligations ride the same @@ -927,10 +927,10 @@ func (e *Engine) DeleteGrantRecord(ctx context.Context, externalID string) error }) } -// DeleteGrantByIdentityRefs removes a grant addressed by its structural +// deleteGrantByIdentityRefs removes a grant addressed by its structural // refs — the exact delete path for callers that hold the full grant. No // lossy id string is involved. Deleting a non-existent grant is a no-op. -func (e *Engine) DeleteGrantByIdentityRefs(ctx context.Context, r *v3.GrantRecord) error { +func (e *Engine) deleteGrantByIdentityRefs(ctx context.Context, r *v3.GrantRecord) error { id, err := grantIdentityFromRecord(r) if err != nil { return fmt.Errorf("DeleteGrantByIdentityRefs: %w", err) @@ -1106,10 +1106,10 @@ func (e *Engine) IterateGrantsByPrincipal(ctx context.Context, principalRT, prin return iter.Error() } -// IterateGrantsByPrincipalResourceType iterates the by_principal index narrowed +// iterateGrantsByPrincipalResourceType iterates the by_principal index narrowed // to a principal resource type. Yields each grant whose principal carries the // given resource_type. Stops when yield returns false. -func (e *Engine) IterateGrantsByPrincipalResourceType(ctx context.Context, principalRT string, yield func(*v3.GrantRecord) bool) error { +func (e *Engine) iterateGrantsByPrincipalResourceType(ctx context.Context, principalRT string, yield func(*v3.GrantRecord) bool) error { indexPrefix := encodeGrantByPrincipalResourceTypeIdentityPrefix(principalRT) iter, err := e.db.NewIter(&pebble.IterOptions{ LowerBound: indexPrefix, diff --git a/pkg/dotc1z/engine/pebble/grants_for_entitlements.go b/pkg/dotc1z/engine/pebble/grants_for_entitlements.go index 92881775b..87e6ed7e1 100644 --- a/pkg/dotc1z/engine/pebble/grants_for_entitlements.go +++ b/pkg/dotc1z/engine/pebble/grants_for_entitlements.go @@ -82,7 +82,7 @@ EntitlementLoop: return nil, err } remaining := limit - len(out) - records, next, err := e.PaginateGrantsByEntitlement(ctx, entID, intraCursor, remaining) + records, next, err := e.paginateGrantsByEntitlement(ctx, entID, intraCursor, remaining) if err != nil { return nil, c1zstore.AdaptNotFound(err, pebble.ErrNotFound) } diff --git a/pkg/dotc1z/engine/pebble/if_newer.go b/pkg/dotc1z/engine/pebble/if_newer.go index bf7902bf3..205cf717f 100644 --- a/pkg/dotc1z/engine/pebble/if_newer.go +++ b/pkg/dotc1z/engine/pebble/if_newer.go @@ -27,10 +27,10 @@ import ( // fresh-sync write path is disabled here — *IfNewer is by definition // not a fresh sync (we're filtering against existing data). -// PutGrantRecordsIfNewer writes records that are strictly newer than +// putGrantRecordsIfNewer writes records that are strictly newer than // the stored copy. Records without a discovered_at are treated as // "always write" (caller is asserting freshness explicitly). -func (e *Engine) PutGrantRecordsIfNewer(ctx context.Context, records ...*v3.GrantRecord) error { +func (e *Engine) putGrantRecordsIfNewer(ctx context.Context, records ...*v3.GrantRecord) error { if len(records) == 0 { return nil } @@ -96,9 +96,9 @@ func (e *Engine) PutGrantRecordsIfNewer(ctx context.Context, records ...*v3.Gran }) } -// PutResourceRecordsIfNewer writes resources only when the incoming +// putResourceRecordsIfNewer writes resources only when the incoming // discovered_at is strictly newer than the stored copy. -func (e *Engine) PutResourceRecordsIfNewer(ctx context.Context, records ...*v3.ResourceRecord) error { +func (e *Engine) putResourceRecordsIfNewer(ctx context.Context, records ...*v3.ResourceRecord) error { if len(records) == 0 { return nil } @@ -159,8 +159,8 @@ func (e *Engine) PutResourceRecordsIfNewer(ctx context.Context, records ...*v3.R }) } -// PutEntitlementRecordsIfNewer writes entitlements only when newer. -func (e *Engine) PutEntitlementRecordsIfNewer(ctx context.Context, records ...*v3.EntitlementRecord) error { +// putEntitlementRecordsIfNewer writes entitlements only when newer. +func (e *Engine) putEntitlementRecordsIfNewer(ctx context.Context, records ...*v3.EntitlementRecord) error { if len(records) == 0 { return nil } @@ -217,8 +217,8 @@ func (e *Engine) PutEntitlementRecordsIfNewer(ctx context.Context, records ...*v }) } -// PutResourceTypeRecordsIfNewer writes resource_types only when newer. -func (e *Engine) PutResourceTypeRecordsIfNewer(ctx context.Context, records ...*v3.ResourceTypeRecord) error { +// putResourceTypeRecordsIfNewer writes resource_types only when newer. +func (e *Engine) putResourceTypeRecordsIfNewer(ctx context.Context, records ...*v3.ResourceTypeRecord) error { if len(records) == 0 { return nil } diff --git a/pkg/dotc1z/engine/pebble/if_newer_test.go b/pkg/dotc1z/engine/pebble/if_newer_test.go index c5f1cbf04..9fc608929 100644 --- a/pkg/dotc1z/engine/pebble/if_newer_test.go +++ b/pkg/dotc1z/engine/pebble/if_newer_test.go @@ -92,7 +92,7 @@ func grantsByPrincipal(t *testing.T, ctx context.Context, e *Engine, principal s // TestPutGrantRecordsIfNewerSkipsStale is the end-to-end guard the // TestDiscoveredAtIsNewer predicate test points at: it drives the full -// PutGrantRecordsIfNewer path (read incumbent, compare discovered_at, +// putGrantRecordsIfNewer path (read incumbent, compare discovered_at, // conditionally write + swap derived index keys) rather than the bare // predicate. It pins that a stale (older or equal) replay never // regresses the stored grant OR its by_principal index, and that a @@ -109,20 +109,20 @@ func TestPutGrantRecordsIfNewerSkipsStale(t *testing.T) { newest := time.Unix(3000, 0).UTC() // Seed the incumbent at `newer`. - require.NoError(t, e.PutGrantRecordsIfNewer(ctx, ifNewerGrant("winner", newer))) + require.NoError(t, e.putGrantRecordsIfNewer(ctx, ifNewerGrant("winner", newer))) // Fixture external ids are connector-custom, so address rows by refs. got, err := testGrantByIdentity(ctx, e, "ent-A", "user", "winner") require.NoError(t, err) require.Equal(t, "winner", got.GetPrincipal().GetResourceId()) // Older replay: dropped — value and index unchanged. - require.NoError(t, e.PutGrantRecordsIfNewer(ctx, ifNewerGrantExternal("g-stale-older", "winner", older))) + require.NoError(t, e.putGrantRecordsIfNewer(ctx, ifNewerGrantExternal("g-stale-older", "winner", older))) got, err = testGrantByIdentity(ctx, e, "ent-A", "user", "winner") require.NoError(t, err) require.Equal(t, "winner", got.GetPrincipal().GetResourceId(), "older replay must not regress the grant") // Equal replay: dropped — strict `>` keeps the incumbent. - require.NoError(t, e.PutGrantRecordsIfNewer(ctx, ifNewerGrantExternal("g-stale-tie", "winner", newer))) + require.NoError(t, e.putGrantRecordsIfNewer(ctx, ifNewerGrantExternal("g-stale-tie", "winner", newer))) got, err = testGrantByIdentity(ctx, e, "ent-A", "user", "winner") require.NoError(t, err) require.Equal(t, "winner", got.GetPrincipal().GetResourceId(), "equal discovered_at must keep the incumbent") @@ -131,7 +131,7 @@ func TestPutGrantRecordsIfNewerSkipsStale(t *testing.T) { require.Equal(t, []string{"g-1"}, grantsByPrincipal(t, ctx, e, "winner")) // Strictly newer for the same identity: replaces the retained value. - require.NoError(t, e.PutGrantRecordsIfNewer(ctx, ifNewerGrantExternal("g-newest", "winner", newest))) + require.NoError(t, e.putGrantRecordsIfNewer(ctx, ifNewerGrantExternal("g-newest", "winner", newest))) got, err = testGrantByIdentity(ctx, e, "ent-A", "user", "winner") require.NoError(t, err) require.Equal(t, "g-newest", got.GetExternalId(), "strictly newer must replace") @@ -157,8 +157,8 @@ func TestPutRecordsIfNewerRejectsOlderAllTypes(t *testing.T) { newRec := func(dn string, at time.Time) *v3.ResourceTypeRecord { return v3.ResourceTypeRecord_builder{ExternalId: "rt-1", DisplayName: dn, DiscoveredAt: timestamppb.New(at)}.Build() } - require.NoError(t, e.PutResourceTypeRecordsIfNewer(ctx, newRec("kept", newer))) - require.NoError(t, e.PutResourceTypeRecordsIfNewer(ctx, newRec("stale", older))) + require.NoError(t, e.putResourceTypeRecordsIfNewer(ctx, newRec("kept", newer))) + require.NoError(t, e.putResourceTypeRecordsIfNewer(ctx, newRec("stale", older))) got, err := e.GetResourceTypeRecord(ctx, "rt-1") require.NoError(t, err) require.Equal(t, "kept", got.GetDisplayName()) @@ -168,8 +168,8 @@ func TestPutRecordsIfNewerRejectsOlderAllTypes(t *testing.T) { newRec := func(dn string, at time.Time) *v3.ResourceRecord { return v3.ResourceRecord_builder{ResourceTypeId: "user", ResourceId: "u1", DisplayName: dn, DiscoveredAt: timestamppb.New(at)}.Build() } - require.NoError(t, e.PutResourceRecordsIfNewer(ctx, newRec("kept", newer))) - require.NoError(t, e.PutResourceRecordsIfNewer(ctx, newRec("stale", older))) + require.NoError(t, e.putResourceRecordsIfNewer(ctx, newRec("kept", newer))) + require.NoError(t, e.putResourceRecordsIfNewer(ctx, newRec("stale", older))) got, err := e.GetResourceRecord(ctx, "user", "u1") require.NoError(t, err) require.Equal(t, "kept", got.GetDisplayName()) @@ -184,16 +184,16 @@ func TestPutRecordsIfNewerRejectsOlderAllTypes(t *testing.T) { DiscoveredAt: timestamppb.New(at), }.Build() } - require.NoError(t, e.PutEntitlementRecordsIfNewer(ctx, newRec("kept", newer))) - require.NoError(t, e.PutEntitlementRecordsIfNewer(ctx, newRec("stale", older))) + require.NoError(t, e.putEntitlementRecordsIfNewer(ctx, newRec("kept", newer))) + require.NoError(t, e.putEntitlementRecordsIfNewer(ctx, newRec("stale", older))) got, err := e.GetEntitlementRecord(ctx, "e-1") require.NoError(t, err) require.Equal(t, "kept", got.GetDisplayName()) }) t.Run("grant", func(t *testing.T) { - require.NoError(t, e.PutGrantRecordsIfNewer(ctx, ifNewerGrant("kept", newer))) - require.NoError(t, e.PutGrantRecordsIfNewer(ctx, ifNewerGrant("stale", older))) + require.NoError(t, e.putGrantRecordsIfNewer(ctx, ifNewerGrant("kept", newer))) + require.NoError(t, e.putGrantRecordsIfNewer(ctx, ifNewerGrant("stale", older))) got, err := testGrantByIdentity(ctx, e, "ent-A", "user", "kept") require.NoError(t, err) require.Equal(t, "kept", got.GetPrincipal().GetResourceId()) diff --git a/pkg/dotc1z/engine/pebble/ingest_repair.go b/pkg/dotc1z/engine/pebble/ingest_repair.go index 9225caa70..980bf707f 100644 --- a/pkg/dotc1z/engine/pebble/ingest_repair.go +++ b/pkg/dotc1z/engine/pebble/ingest_repair.go @@ -44,7 +44,7 @@ func (e *Engine) EnsureGrantIndexes(ctx context.Context) error { if !e.db.DeferredIdxPending() { return nil } - if err := e.BuildDeferredGrantIndexes(ctx); err != nil { + if err := e.buildDeferredGrantIndexes(ctx); err != nil { return err } return e.clearDeferredIdxPending() @@ -177,7 +177,7 @@ const maxHealedOrphanExamples = 25 // NoSync — the seal's durability flush hardens them, matching every // other during-sync write path. func (e *Engine) invariantWriteOpts() *pebble.WriteOptions { - if e.IsFreshSync() { + if e.isFreshSync() { return pebble.NoSync } return writeOpts(e.opts.durability) diff --git a/pkg/dotc1z/engine/pebble/lightning_bench_test.go b/pkg/dotc1z/engine/pebble/lightning_bench_test.go index 2f3f7e56d..4c0264039 100644 --- a/pkg/dotc1z/engine/pebble/lightning_bench_test.go +++ b/pkg/dotc1z/engine/pebble/lightning_bench_test.go @@ -93,7 +93,7 @@ func BenchmarkPebbleExpansion_StoreExpandedGrants(b *testing.B) { // entitlement across N finished syncs. Under the structural-identity // layout the grant primary key is entitlement-first (the retired // sync-prefixed idxGrantByEntitlement keyspace is gone), so each -// PaginateGrantsByEntitlement call is a single primary-prefix scan. +// paginateGrantsByEntitlement call is a single primary-prefix scan. // // Output metric `entries_returned` is the number of grants // surfaced for the target entitlement across all syncs — should diff --git a/pkg/dotc1z/engine/pebble/lookup_test.go b/pkg/dotc1z/engine/pebble/lookup_test.go index 752325aae..218113738 100644 --- a/pkg/dotc1z/engine/pebble/lookup_test.go +++ b/pkg/dotc1z/engine/pebble/lookup_test.go @@ -68,7 +68,7 @@ func TestBareIDEntitlementLookupExactlyOne(t *testing.T) { // Two matches → explicit ambiguity, for reads AND deletes. _, err = e.GetEntitlementRecord(ctx, "shared-id") require.ErrorIs(t, err, ErrAmbiguousExternalID) - err = e.DeleteEntitlementRecord(ctx, "shared-id") + err = e.deleteEntitlementRecord(ctx, "shared-id") require.ErrorIs(t, err, ErrAmbiguousExternalID, "an ambiguous string must never guess a delete") // Both ambiguous rows are intact. @@ -121,7 +121,7 @@ func TestBareIDEntitlementLookupInvalidation(t *testing.T) { require.NoError(t, err, "write must invalidate the cached map") require.Equal(t, "second", got.GetExternalId()) - require.NoError(t, e.DeleteEntitlementRecord(ctx, "first")) + require.NoError(t, e.deleteEntitlementRecord(ctx, "first")) _, err = e.GetEntitlementRecord(ctx, "first") require.ErrorIs(t, err, pebble.ErrNotFound, "delete must invalidate the cached map") } @@ -218,7 +218,7 @@ func TestDeleteGrantByIdentityRefsIsExact(t *testing.T) { require.NoError(t, e.PutGrantRecord(ctx, b)) // The string is ambiguous, but refs are not. - require.NoError(t, e.DeleteGrantByIdentityRefs(ctx, a)) + require.NoError(t, e.deleteGrantByIdentityRefs(ctx, a)) var survivors []string require.NoError(t, e.IterateGrants(ctx, func(r *v3.GrantRecord) bool { survivors = append(survivors, r.GetEntitlement().GetEntitlementId()) @@ -227,9 +227,9 @@ func TestDeleteGrantByIdentityRefsIsExact(t *testing.T) { require.Equal(t, []string{"group:eng:member"}, survivors, "refs delete removes exactly its row") // Delete of a non-existent identity is a no-op. - require.NoError(t, e.DeleteGrantByIdentityRefs(ctx, a)) + require.NoError(t, e.deleteGrantByIdentityRefs(ctx, a)) // Incomplete refs are an error, never a bare-id fallback. - err := e.DeleteGrantByIdentityRefs(ctx, v3.GrantRecord_builder{ExternalId: shared}.Build()) + err := e.deleteGrantByIdentityRefs(ctx, v3.GrantRecord_builder{ExternalId: shared}.Build()) require.Error(t, err) } diff --git a/pkg/dotc1z/engine/pebble/merge_accessor.go b/pkg/dotc1z/engine/pebble/merge_accessor.go index 23bb8fa31..d4c9cbe88 100644 --- a/pkg/dotc1z/engine/pebble/merge_accessor.go +++ b/pkg/dotc1z/engine/pebble/merge_accessor.go @@ -23,8 +23,20 @@ import ( type FoldBatch = rawdb.FoldBatch // engineAccessor is implemented by *Engine itself and by pkg/dotc1z's -// Pebble store wrapper (which embeds *Engine and overrides the method -// with a nil-safe version). +// Pebble store wrapper, which holds an *Engine in a named field and +// declares its own nil-safe version of this method. +// +// Note what this hands out: the raw engine, with no admission check and no +// dirty tracking. That is deliberate for the merge paths, which consume +// concrete engines (SourceSync, k-way source handles with raw iterators) that +// the store surface cannot express. It also means AsEngine is a way around +// the store's mutation gate, so its use is bounded by an ownership rule: +// extract engines only from single-owner source files the caller opened and +// will close itself, never from a shared destination store. The dest reads +// through the store's own surface (e.g. LatestFinishedSyncRecord), and dest +// writes go through WithEngineMutation / WithEngineFoldMutation below, whose +// callback parameter is the guarded grant of mutation access. A bare AsEngine +// mutation is invisible to the store's admission state. type engineAccessor interface { PebbleEngine() *Engine } @@ -40,7 +52,7 @@ func (e *Engine) PebbleEngine() *Engine { // AsEngine recovers the underlying *Engine from a connectorstore.Writer // produced by dotc1z.NewStore for the Pebble engine. NewStore returns a -// wrapper that embeds *Engine; a bare *Engine is also accepted for +// wrapper holding an *Engine; a bare *Engine is also accepted for // callers that hold one directly. Returns (nil, false) for any // non-Pebble store, so a caller can branch on the engine without // importing internal types. @@ -153,43 +165,52 @@ type fixtureNormalizer interface { NormalizeForFixtureSave(ctx context.Context, syncID string) error } -type storeDirtyMarker interface { - MarkDirty() +type guardedMutationRunner interface { + RunPebbleMutation(ctx context.Context, fn func(context.Context, *Engine) error) error } -type foldDeadBytesAdder interface { - AddFoldDeadBytes(n int64) +type guardedFoldMutationRunner interface { + RunPebbleFoldMutation(ctx context.Context, fn func(context.Context, *Engine) (int64, error)) error } -// AddFoldDeadBytes bumps a registered store's cumulative fold-waste -// counter (persisted as the envelope manifest's fold_dead_bytes at -// save). Called by the fold compactor with the exact raw bytes its -// merge shadowed in the base keyspace; the compactor's auto cutover -// later reads the counter from the envelope header to force a rebuild -// once waste crosses its threshold. Returns false when w is not a -// registered Pebble store. -func AddFoldDeadBytes(w connectorstore.Writer, n int64) bool { - s, ok := w.(foldDeadBytesAdder) - if !ok || s == nil { - return false +// WithEngineMutation runs a direct engine mutation under the owning store's +// admission guard. A bare *Engine has no envelope lifecycle to coordinate, so +// it executes the callback directly and relies on the Engine's own write guard. +func WithEngineMutation(ctx context.Context, target any, fn func(context.Context, *Engine) error) error { + if fn == nil { + return errors.New("pebble WithEngineMutation: nil callback") } - s.AddFoldDeadBytes(n) - return true -} - -// MarkStoreDirty flips a registered store's dirty bit so Close drives -// the save → checkpoint → envelope path. Engine-level writes (raw -// batches, SST ingest, direct Put*Records) bypass the registered -// store's markDirty wrappers; merge tooling that mutates the engine -// directly calls this once so the mutations are persisted at Close. -// Returns false when w is not a registered Pebble store. -func MarkStoreDirty(w connectorstore.Writer) bool { - s, ok := w.(storeDirtyMarker) - if !ok || s == nil { - return false + if s, ok := target.(guardedMutationRunner); ok { + return s.RunPebbleMutation(ctx, fn) } - s.MarkDirty() - return true + if e, ok := target.(*Engine); ok && e != nil { + return fn(ctx, e) + } + return errors.New("pebble WithEngineMutation: target is not a pebble engine or store") +} + +// WithEngineFoldMutation is the fold-specific guarded callback. The callback +// returns the number of newly shadowed bytes; the store records that manifest +// metadata before releasing admission, keeping it atomic with Close. +// +// Unlike WithEngineMutation, this requires a store: fold_dead_bytes lives in +// the envelope manifest, which only the store writes, so a bare engine has +// nowhere to record the count. Dropping it would understate accumulated waste +// and defer the rebuild that reclaims it. +func WithEngineFoldMutation(ctx context.Context, target any, fn func(context.Context, *Engine) (int64, error)) error { + if fn == nil { + return errors.New("pebble WithEngineFoldMutation: nil callback") + } + if s, ok := target.(guardedFoldMutationRunner); ok { + return s.RunPebbleFoldMutation(ctx, fn) + } + if e, ok := target.(*Engine); ok && e != nil { + // Refuse before running fn. The shadowed-byte count is only knowable + // afterwards, so checking it post-hoc would report failure for a fold + // that already landed in the engine and cannot be undone. + return errors.New("pebble WithEngineFoldMutation: a fold must target a store, not a bare engine, so fold dead bytes can be recorded") + } + return errors.New("pebble WithEngineFoldMutation: target is not a pebble engine or store") } // CloseEngineOnly closes the Pebble engine inside a registered store without diff --git a/pkg/dotc1z/engine/pebble/merge_accessor_test.go b/pkg/dotc1z/engine/pebble/merge_accessor_test.go new file mode 100644 index 000000000..bf41f1e9e --- /dev/null +++ b/pkg/dotc1z/engine/pebble/merge_accessor_test.go @@ -0,0 +1,37 @@ +package pebble + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" +) + +// A bare engine has no envelope manifest, so it cannot carry fold_dead_bytes. +// Folding into one anyway would drop the count silently, understating +// accumulated fold waste and deferring the rebuild that reclaims it. +// +// The refusal must land before the callback runs. The shadowed-byte count is +// only knowable afterwards, so a post-hoc check would return an error for a +// fold that already mutated the engine and cannot be rolled back. +func TestWithEngineFoldMutationRejectsBareEngineBeforeMutating(t *testing.T) { + ctx := context.Background() + + ran := false + err := WithEngineFoldMutation(ctx, &Engine{}, func(context.Context, *Engine) (int64, error) { + ran = true + return 4096, nil + }) + require.ErrorContains(t, err, "must target a store") + require.False(t, ran, "the callback must not run against a bare engine") + + // Same refusal when the fold would have shadowed nothing: the target is + // wrong regardless of the outcome. + ran = false + err = WithEngineFoldMutation(ctx, &Engine{}, func(context.Context, *Engine) (int64, error) { + ran = true + return 0, nil + }) + require.ErrorContains(t, err, "must target a store") + require.False(t, ran, "the callback must not run against a bare engine") +} diff --git a/pkg/dotc1z/engine/pebble/merge_surface.go b/pkg/dotc1z/engine/pebble/merge_surface.go index 6661bb59f..3e3fa4239 100644 --- a/pkg/dotc1z/engine/pebble/merge_surface.go +++ b/pkg/dotc1z/engine/pebble/merge_surface.go @@ -53,8 +53,8 @@ func (e *Engine) Metrics() *pebble.Metrics { return e.db.Metrics() } -// EstimateDiskUsage estimates on-disk size of the key range. -func (e *Engine) EstimateDiskUsage(start, end []byte) (uint64, error) { +// estimateDiskUsage estimates on-disk size of the key range. +func (e *Engine) estimateDiskUsage(start, end []byte) (uint64, error) { if e.db == nil { return 0, ErrEngineClosing } diff --git a/pkg/dotc1z/engine/pebble/paginate.go b/pkg/dotc1z/engine/pebble/paginate.go index bfcb862c9..e7e792b0d 100644 --- a/pkg/dotc1z/engine/pebble/paginate.go +++ b/pkg/dotc1z/engine/pebble/paginate.go @@ -225,7 +225,7 @@ func getGrantByIdentity(ctx context.Context, db *rawdb.DB, id grantIdentity) (*v // collapsing 2N nested allocs into 2 slice allocs per page. The // arena lifetime ends with this function — callers receive pointers // into the arena's backing arrays, so retention is intentional. -func (e *Engine) PaginateGrants( +func (e *Engine) paginateGrants( ctx context.Context, cursor string, limit int, ) ([]*v3.GrantRecord, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -252,11 +252,11 @@ func (e *Engine) PaginateGrants( return records, next, nil } -// PaginateGrantsByEntitlement scans the primary grant keyspace under the +// paginateGrantsByEntitlement scans the primary grant keyspace under the // entitlement identity prefix. Cursor is the primary key. Callers resolve // the identity from structured refs (or the bare-id lookup) — the engine // never parses id strings here. -func (e *Engine) PaginateGrantsByEntitlement( +func (e *Engine) paginateGrantsByEntitlement( ctx context.Context, entID entitlementIdentity, cursor string, limit int, ) ([]*v3.GrantRecord, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -266,11 +266,11 @@ func (e *Engine) PaginateGrantsByEntitlement( return iterateGrantPrimaryPage(ctx, e.db, encodeGrantPrimaryEntitlementPrefix(entID), cursorBytes, limit) } -// PaginateGrantPrincipalKeysByEntitlement scans the primary grant keyspace under +// paginateGrantPrincipalKeysByEntitlement scans the primary grant keyspace under // the entitlement identity prefix and returns only principal identity keys for // each matching grant. The key format is principal_resource_type + "\x00" + // principal_resource_id, matching pkg/sync/expand's descendantGrantKey. -func (e *Engine) PaginateGrantPrincipalKeysByEntitlement( +func (e *Engine) paginateGrantPrincipalKeysByEntitlement( ctx context.Context, entID entitlementIdentity, cursor string, limit int, ) ([]string, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -321,11 +321,11 @@ func (e *Engine) PaginateGrantPrincipalKeysByEntitlement( return out, nextCursor, nil } -// PaginateGrantsByEntitlementPrincipal uses the structured primary key for a +// paginateGrantsByEntitlementPrincipal uses the structured primary key for a // point lookup by entitlement + principal. This is the hot path for grant // expansion, where callers repeatedly ask whether a single principal already // has a grant on a descendant entitlement. -func (e *Engine) PaginateGrantsByEntitlementPrincipal( +func (e *Engine) paginateGrantsByEntitlementPrincipal( ctx context.Context, entID entitlementIdentity, principalRT, principalID, cursor string, limit int, ) ([]*v3.GrantRecord, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -350,9 +350,9 @@ func (e *Engine) PaginateGrantsByEntitlementPrincipal( return []*v3.GrantRecord{r}, "", nil } -// PaginateGrantsByPrincipal uses the by_principal index. Same shape +// paginateGrantsByPrincipal uses the by_principal index. Same shape // as PaginateGrantsByEntitlement. -func (e *Engine) PaginateGrantsByPrincipal( +func (e *Engine) paginateGrantsByPrincipal( ctx context.Context, principalRT, principalID, cursor string, limit int, ) ([]*v3.GrantRecord, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -420,7 +420,7 @@ func (e *Engine) PaginateGrantsByPrincipal( return out, nextCursor, nil } -// PaginateGrantsByEntitlementResource walks the primary grant keyspace for all +// paginateGrantsByEntitlementResource walks the primary grant keyspace for all // grants whose entitlement's resource is (entRT, entRID). Cursor is the primary // key. // @@ -429,7 +429,7 @@ func (e *Engine) PaginateGrantsByPrincipal( // grants.resource_id / resource_type_id (the entitlement-side resource columns). // The pre-existing Pebble path used PaginateGrantsByPrincipal here, which // returned empty for the common "grants on this group" semantic. -func (e *Engine) PaginateGrantsByEntitlementResource( +func (e *Engine) paginateGrantsByEntitlementResource( ctx context.Context, entRT, entRID, cursor string, limit int, ) ([]*v3.GrantRecord, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -442,10 +442,10 @@ func (e *Engine) PaginateGrantsByEntitlementResource( return iterateGrantPrimaryPage(ctx, e.db, encodeGrantPrimaryEntitlementResourcePrefix(entRT, entRID), cursorBytes, limit) } -// PaginateGrantsByPrincipalResourceType walks the by-principal-RT +// paginateGrantsByPrincipalResourceType walks the by-principal-RT // index. Cursor is the index key. Drives the new fast path for // the Adapter's ListGrantsForResourceType. -func (e *Engine) PaginateGrantsByPrincipalResourceType( +func (e *Engine) paginateGrantsByPrincipalResourceType( ctx context.Context, principalRT, cursor string, limit int, ) ([]*v3.GrantRecord, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -513,13 +513,13 @@ func (e *Engine) PaginateGrantsByPrincipalResourceType( return out, nextCursor, nil } -// PaginateGrantsByNeedsExpansion returns a page of grants whose +// paginateGrantsByNeedsExpansion returns a page of grants whose // NeedsExpansion flag is set. Backs the GrantStore // PendingExpansionPage path; the SQLite equivalent is a query // guarded by the partial index `WHERE needs_expansion = 1`. // // Cursor is the needs_expansion index key. -func (e *Engine) PaginateGrantsByNeedsExpansion( +func (e *Engine) paginateGrantsByNeedsExpansion( ctx context.Context, cursor string, limit int, ) ([]*v3.GrantRecord, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -591,7 +591,7 @@ func (e *Engine) PaginateGrantsByNeedsExpansion( // PaginateResourcesBySync returns a page of resources in primary // key order. -func (e *Engine) PaginateResources( +func (e *Engine) paginateResources( ctx context.Context, cursor string, limit int, ) ([]*v3.ResourceRecord, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -604,8 +604,8 @@ func (e *Engine) PaginateResources( }) } -// PaginateResourcesByParent uses the by_parent index. -func (e *Engine) PaginateResourcesByParent( +// paginateResourcesByParent uses the by_parent index. +func (e *Engine) paginateResourcesByParent( ctx context.Context, parentRT, parentID, cursor string, limit int, ) ([]*v3.ResourceRecord, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -670,7 +670,7 @@ func (e *Engine) PaginateResourcesByParent( } // PaginateResourceTypesBySync returns a page of resource_types. -func (e *Engine) PaginateResourceTypes( +func (e *Engine) paginateResourceTypes( ctx context.Context, cursor string, limit int, ) ([]*v3.ResourceTypeRecord, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -685,7 +685,7 @@ func (e *Engine) PaginateResourceTypes( // PaginateEntitlementsBySync returns a page of entitlements in // primary key order. -func (e *Engine) PaginateEntitlements( +func (e *Engine) paginateEntitlements( ctx context.Context, cursor string, limit int, ) ([]*v3.EntitlementRecord, string, error) { cursorBytes, err := decodeCursor(cursor) @@ -698,8 +698,8 @@ func (e *Engine) PaginateEntitlements( }) } -// PaginateEntitlementsByResource uses the entitlement primary key prefix. -func (e *Engine) PaginateEntitlementsByResource( +// paginateEntitlementsByResource uses the entitlement primary key prefix. +func (e *Engine) paginateEntitlementsByResource( ctx context.Context, resourceTypeID, resourceID, cursor string, limit int, ) ([]*v3.EntitlementRecord, string, error) { cursorBytes, err := decodeCursor(cursor) diff --git a/pkg/dotc1z/engine/pebble/paginate_test.go b/pkg/dotc1z/engine/pebble/paginate_test.go index d57ee9540..ae71b2baa 100644 --- a/pkg/dotc1z/engine/pebble/paginate_test.go +++ b/pkg/dotc1z/engine/pebble/paginate_test.go @@ -277,12 +277,12 @@ func TestPaginationClampedPageSize(t *testing.T) { _ = r } // Passing 0 should clamp to DefaultPageSize and return all 50. - recs, next, err := e.PaginateGrants(ctx, "", 0) + recs, next, err := e.paginateGrants(ctx, "", 0) require.NoError(t, err) require.Equal(t, total, len(recs), "clamp(0): got %d, want %d", len(recs), total) require.Empty(t, next, "clamp(0): expected empty next cursor, got %q", next) // Passing MaxPageSize+1 should clamp identically. - recs2, _, err := e.PaginateGrants(ctx, "", MaxPageSize+1) + recs2, _, err := e.paginateGrants(ctx, "", MaxPageSize+1) require.NoError(t, err) require.Equal(t, total, len(recs2), "clamp(max+1): got %d, want %d", len(recs2), total) } diff --git a/pkg/dotc1z/engine/pebble/records_test.go b/pkg/dotc1z/engine/pebble/records_test.go index 70b7a5c32..c9ac3cd78 100644 --- a/pkg/dotc1z/engine/pebble/records_test.go +++ b/pkg/dotc1z/engine/pebble/records_test.go @@ -205,7 +205,7 @@ func TestAssetRoundtrip(t *testing.T) { Data: payload, }.Build() require.NoError(t, e.PutAssetRecord(ctx, r)) - got, err := e.GetAssetRecord(ctx, "icon-1") + got, err := e.getAssetRecord(ctx, "icon-1") require.NoErrorf(t, err, "GetAssetRecord") require.Equal(t, "image/png", got.GetContentType(), "content_type") require.Equal(t, string(payload), string(got.GetData()), "data roundtrip lost bytes") @@ -246,7 +246,7 @@ func TestSyncRunRecord(t *testing.T) { })) require.Equal(t, 1, count, "sync_runs count") - require.NoError(t, e.DeleteSyncRunRecord(ctx, id2)) + require.NoError(t, e.deleteSyncRunRecord(ctx, id2)) _, err = e.GetSyncRunRecord(ctx, id2) require.ErrorIs(t, err, pebble.ErrNotFound, "expected ErrNotFound after delete") } diff --git a/pkg/dotc1z/engine/pebble/resource_types.go b/pkg/dotc1z/engine/pebble/resource_types.go index bbef3af55..6fbbfdb80 100644 --- a/pkg/dotc1z/engine/pebble/resource_types.go +++ b/pkg/dotc1z/engine/pebble/resource_types.go @@ -31,7 +31,7 @@ func (e *Engine) PutResourceTypeRecords(ctx context.Context, records ...*v3.Reso } batch := e.db.NewRecordBatch() defer batch.Close() - fresh := e.IsFreshSync() + fresh := e.isFreshSync() for _, r := range records { if r == nil { continue @@ -67,20 +67,6 @@ func (e *Engine) GetResourceTypeRecord(ctx context.Context, externalID string) ( return r, nil } -func (e *Engine) DeleteResourceTypeRecord(ctx context.Context, externalID string) error { - return e.withWrite(func() error { - // Record family, not engine-meta: resource types are record - // rows. A one-op RecordBatch commit is durability-identical to - // the old single Delete (the commit carries the write options). - batch := e.db.NewRecordBatch() - defer batch.Close() - if err := batch.StageResourceTypeDelete(encodeResourceTypeKey(externalID)); err != nil { - return err - } - return batch.Commit(writeOpts(e.opts.durability)) - }) -} - func (e *Engine) IterateResourceTypes(ctx context.Context, yield func(*v3.ResourceTypeRecord) bool) error { prefix := encodeResourceTypePrefix() iter, err := e.db.NewIter(&pebble.IterOptions{ diff --git a/pkg/dotc1z/engine/pebble/resources.go b/pkg/dotc1z/engine/pebble/resources.go index b05b5ca59..8b0e335f1 100644 --- a/pkg/dotc1z/engine/pebble/resources.go +++ b/pkg/dotc1z/engine/pebble/resources.go @@ -42,7 +42,7 @@ func (e *Engine) PutResourceRecords(ctx context.Context, records ...*v3.Resource batch := e.db.NewRecordBatch() defer batch.Close() - fresh := e.IsFreshSync() + fresh := e.isFreshSync() skipGet := e.takeFreshResourcesEmpty() type dedupKey struct { diff --git a/pkg/dotc1z/engine/pebble/session_sealed_test.go b/pkg/dotc1z/engine/pebble/session_sealed_test.go index 9e9d8417d..9ae2fc5a4 100644 --- a/pkg/dotc1z/engine/pebble/session_sealed_test.go +++ b/pkg/dotc1z/engine/pebble/session_sealed_test.go @@ -30,7 +30,7 @@ func TestSessionWritesAllowedWhileSealed(t *testing.T) { require.NoError(t, e.SessionSet(ctx, "cache-key", []byte("cache-value"), sid)) require.NoError(t, a.EndSync(ctx)) - require.True(t, e.IsSealed(), "EndSync must seal") + require.True(t, e.isSealed(), "EndSync must seal") // The Cleanup-path clear must succeed on the sealed engine. require.NoError(t, e.SessionClear(ctx, sid), "SessionClear after EndSync (connector Cleanup path)") diff --git a/pkg/dotc1z/engine/pebble/sync_runs.go b/pkg/dotc1z/engine/pebble/sync_runs.go index e87c58d61..e99c29e94 100644 --- a/pkg/dotc1z/engine/pebble/sync_runs.go +++ b/pkg/dotc1z/engine/pebble/sync_runs.go @@ -66,7 +66,7 @@ func (e *Engine) GetSyncRunRecord(ctx context.Context, syncID string) (*v3.SyncR return r, nil } -func (e *Engine) DeleteSyncRunRecord(ctx context.Context, syncID string) error { +func (e *Engine) deleteSyncRunRecord(ctx context.Context, syncID string) error { // AllowSealed: sync-run pruning is metadata maintenance on finished // syncs, same class as PutSyncRunRecord above. return e.withWriteAllowSealed(func() error { diff --git a/pkg/dotc1z/engine/pebble/synth_layer_session_test.go b/pkg/dotc1z/engine/pebble/synth_layer_session_test.go index 57e393903..29f16fe2e 100644 --- a/pkg/dotc1z/engine/pebble/synth_layer_session_test.go +++ b/pkg/dotc1z/engine/pebble/synth_layer_session_test.go @@ -63,15 +63,15 @@ func TestSynthLayerSessionMultiSegment(t *testing.T) { ctx := context.Background() e := synthLayerEngine(t, ctx) - ok, err := e.BeginSynthesizedGrantLayer(ctx) + ok, err := e.beginSynthesizedGrantLayer(ctx) require.NoError(t, err) require.True(t, ok, "Pebble must serve a layer session with a sync open") const rows = 7 // 3 cut segments + a 1-row tail flushed at Finish for i := 0; i < rows; i++ { - require.NoError(t, e.AddSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(i)})) + require.NoError(t, e.addSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(i)})) } - require.NoError(t, e.FinishSynthesizedGrantLayer(ctx)) + require.NoError(t, e.finishSynthesizedGrantLayer(ctx)) got := readSynthLayerPrincipals(t, ctx, e) require.Len(t, got, rows, "row count after multi-segment finish") @@ -80,10 +80,10 @@ func TestSynthLayerSessionMultiSegment(t *testing.T) { } // The session is closed: a new Begin must succeed (no leaked session). - ok, err = e.BeginSynthesizedGrantLayer(ctx) + ok, err = e.beginSynthesizedGrantLayer(ctx) require.NoError(t, err) require.True(t, ok) - require.NoError(t, e.AbortSynthesizedGrantLayer(ctx)) + require.NoError(t, e.abortSynthesizedGrantLayer(ctx)) } // TestSynthLayerSessionAbortAfterIngestThenRetry pins the documented abort @@ -97,15 +97,15 @@ func TestSynthLayerSessionAbortAfterIngestThenRetry(t *testing.T) { e := synthLayerEngine(t, ctx) const rows = 5 - ok, err := e.BeginSynthesizedGrantLayer(ctx) + ok, err := e.beginSynthesizedGrantLayer(ctx) require.NoError(t, err) require.True(t, ok) // Every Add cuts+queues a 1-row segment; some subset is ingested before // the abort lands. for i := 0; i < rows-1; i++ { - require.NoError(t, e.AddSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(i)})) + require.NoError(t, e.addSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(i)})) } - require.NoError(t, e.AbortSynthesizedGrantLayer(ctx)) + require.NoError(t, e.abortSynthesizedGrantLayer(ctx)) stranded := readSynthLayerPrincipals(t, ctx, e) require.LessOrEqual(t, len(stranded), rows-1, "abort must not invent rows") @@ -115,13 +115,13 @@ func TestSynthLayerSessionAbortAfterIngestThenRetry(t *testing.T) { // Retry: a fresh session re-adds ALL rows (the stranded ones included) // and finishes. - ok, err = e.BeginSynthesizedGrantLayer(ctx) + ok, err = e.beginSynthesizedGrantLayer(ctx) require.NoError(t, err, "Begin after abort") require.True(t, ok) for i := 0; i < rows; i++ { - require.NoError(t, e.AddSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(i)})) + require.NoError(t, e.addSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(i)})) } - require.NoError(t, e.FinishSynthesizedGrantLayer(ctx)) + require.NoError(t, e.finishSynthesizedGrantLayer(ctx)) got := readSynthLayerPrincipals(t, ctx, e) require.Len(t, got, rows, "retry must converge to exactly the full set") @@ -142,47 +142,47 @@ func TestSynthLayerSessionWorkerErrorPropagates(t *testing.T) { t.Run("surfaces at Add", func(t *testing.T) { e := synthLayerEngine(t, ctx) - ok, err := e.BeginSynthesizedGrantLayer(ctx) + ok, err := e.beginSynthesizedGrantLayer(ctx) require.NoError(t, err) require.True(t, ok) - require.NoError(t, e.AddSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(0)})) + require.NoError(t, e.addSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(0)})) injected := errors.New("test: synth layer worker ingest failed") e.loadSynthLayer().setErr(injected) - err = e.AddSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(1)}) + err = e.addSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(1)}) require.ErrorIs(t, err, injected, "worker error must surface at the next Add") // The session is still attached after a failed Add; Abort must // tear it down without hanging. - require.NoError(t, e.AbortSynthesizedGrantLayer(ctx)) + require.NoError(t, e.abortSynthesizedGrantLayer(ctx)) // Engine remains fully usable for a fresh session. - ok, err = e.BeginSynthesizedGrantLayer(ctx) + ok, err = e.beginSynthesizedGrantLayer(ctx) require.NoError(t, err, "Begin after worker failure") require.True(t, ok) - require.NoError(t, e.AddSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(99)})) - require.NoError(t, e.FinishSynthesizedGrantLayer(ctx)) + require.NoError(t, e.addSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(99)})) + require.NoError(t, e.finishSynthesizedGrantLayer(ctx)) got := readSynthLayerPrincipals(t, ctx, e) require.Equal(t, 1, got["u99"], "post-recovery session must write normally") }) t.Run("surfaces at Finish", func(t *testing.T) { e := synthLayerEngine(t, ctx) - ok, err := e.BeginSynthesizedGrantLayer(ctx) + ok, err := e.beginSynthesizedGrantLayer(ctx) require.NoError(t, err) require.True(t, ok) - require.NoError(t, e.AddSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(0)})) + require.NoError(t, e.addSynthesizedGrantLayerContributions(ctx, []synthesizedGrantRecord{synthLayerRow(0)})) injected := errors.New("test: synth layer worker ingest failed") e.loadSynthLayer().setErr(injected) // Finish waits the worker out and must report the stored error, // not success. - require.ErrorIs(t, e.FinishSynthesizedGrantLayer(ctx), injected, "worker error must surface at Finish") + require.ErrorIs(t, e.finishSynthesizedGrantLayer(ctx), injected, "worker error must surface at Finish") - ok, err = e.BeginSynthesizedGrantLayer(ctx) + ok, err = e.beginSynthesizedGrantLayer(ctx) require.NoError(t, err, "Begin after failed Finish") require.True(t, ok) - require.NoError(t, e.AbortSynthesizedGrantLayer(ctx)) + require.NoError(t, e.abortSynthesizedGrantLayer(ctx)) }) } diff --git a/pkg/dotc1z/engine/pebble/typed_record_ops_coverage_test.go b/pkg/dotc1z/engine/pebble/typed_record_ops_coverage_test.go index 562f474ac..040478043 100644 --- a/pkg/dotc1z/engine/pebble/typed_record_ops_coverage_test.go +++ b/pkg/dotc1z/engine/pebble/typed_record_ops_coverage_test.go @@ -65,7 +65,7 @@ func TestPutSynthesizedGrantRecordsObligations(t *testing.T) { require.NoError(t, err) e := a.PebbleEngine() - require.NoError(t, e.PutSynthesizedGrantRecords(ctx, []*v3.GrantRecord{ + require.NoError(t, e.putSynthesizedGrantRecords(ctx, []*v3.GrantRecord{ testGrantRecord("ent-A", "alice"), testGrantRecord("ent-A", "bob"), })) @@ -111,7 +111,7 @@ func TestPutSynthesizedGrantContributionsBatchObligations(t *testing.T) { } // Through the exported dispatcher so its counter bookkeeping and the // batch body are both exercised. - require.NoError(t, e.PutSynthesizedGrantContributions(ctx, records)) + require.NoError(t, e.putSynthesizedGrantContributions(ctx, records)) require.True(t, e.db.DeferredIdxPending(), "contributions must arm the deferred rebuild marker") require.NoError(t, a.EndSync(ctx)) @@ -137,7 +137,7 @@ func TestUnsafePutUniqueGrantRecordsObligations(t *testing.T) { expandable := testGrantRecord("ent-A", "alice") expandable.SetNeedsExpansion(true) plain := testGrantRecord("ent-B", "bob") - require.NoError(t, e.UnsafePutUniqueGrantRecords(ctx, expandable, plain)) + require.NoError(t, e.unsafePutUniqueGrantRecords(ctx, expandable, plain)) require.Equal(t, 2, countKeys(t, e, encodeGrantPrefix()), "both rows must land") require.Equal(t, 1, countKeys(t, e, encodeGrantByNeedsExpansionPrefix()), @@ -150,14 +150,14 @@ func TestUnsafePutUniqueGrantRecordsObligations(t *testing.T) { // freshness contract itself broke — the guard must refuse rather // than silently tombstone a trusted import's digest keyspace. e.db.SetGrantDigestsPresent(true) - require.ErrorContains(t, e.UnsafePutUniqueGrantRecords(ctx, testGrantRecord("ent-C", "carol")), + require.ErrorContains(t, e.unsafePutUniqueGrantRecords(ctx, testGrantRecord("ent-C", "carol")), "digest state present on a fresh sync") e.db.SetGrantDigestsPresent(false) // Non-fresh syncs are refused (SetCurrentSync clears freshness). require.NoError(t, a.EndSync(ctx)) require.NoError(t, a.SetCurrentSync(ctx, syncID)) - require.ErrorContains(t, e.UnsafePutUniqueGrantRecords(ctx, testGrantRecord("ent-C", "carol")), + require.ErrorContains(t, e.unsafePutUniqueGrantRecords(ctx, testGrantRecord("ent-C", "carol")), "sync is not fresh") } @@ -202,7 +202,7 @@ func TestGrantDeleteMalformedValueStillCleansObligations(t *testing.T) { // value). Both index families and the digest root must be cleaned // even though the value is garbage. rec := testGrantRecord("ent-A", "alice") - require.NoError(t, e.DeleteGrantByIdentityRefs(ctx, rec)) + require.NoError(t, e.deleteGrantByIdentityRefs(ctx, rec)) _, closer, err := e.db.Get(key) require.ErrorIs(t, err, pebble.ErrNotFound, "primary must be gone") @@ -266,7 +266,7 @@ func TestDeferredMarkerArmFailureRollsBackCAS(t *testing.T) { // The deferred-regime write must FAIL when the marker can't arm — // committing deferred rows without the durable marker is the lie. - err = e.PutSynthesizedGrantRecords(ctx, []*v3.GrantRecord{testGrantRecord("ent-A", "alice")}) + err = e.putSynthesizedGrantRecords(ctx, []*v3.GrantRecord{testGrantRecord("ent-A", "alice")}) require.ErrorIs(t, err, injected, "deferred write must surface the arm failure") // THE CONTRACT: flag rolled back, durable key absent — in agreement. @@ -283,7 +283,7 @@ func TestDeferredMarkerArmFailureRollsBackCAS(t *testing.T) { // Retry converges: with the fault gone, the same write arms the // marker durably and lands. e.db.SetDeferredMarkerTestHooks(nil, nil) - require.NoError(t, e.PutSynthesizedGrantRecords(ctx, []*v3.GrantRecord{testGrantRecord("ent-A", "alice")})) + require.NoError(t, e.putSynthesizedGrantRecords(ctx, []*v3.GrantRecord{testGrantRecord("ent-A", "alice")})) require.True(t, e.db.DeferredIdxPending()) _, closer, getErr = e.db.Get(rawdb.DeferredIdxPendingKey()) require.NoError(t, getErr, "the retried arm must persist the durable marker") @@ -319,7 +319,7 @@ func TestDeferredMarkerClearFailureKeepsAgreement(t *testing.T) { e := a.PebbleEngine() // Deferred-regime write arms the marker. - require.NoError(t, e.PutSynthesizedGrantRecords(ctx, []*v3.GrantRecord{testGrantRecord("ent-A", "alice")})) + require.NoError(t, e.putSynthesizedGrantRecords(ctx, []*v3.GrantRecord{testGrantRecord("ent-A", "alice")})) require.True(t, e.db.DeferredIdxPending()) injected := errors.New("injected deferred-marker clear failure") @@ -375,18 +375,18 @@ func TestPutResourceRecordsIfNewerBranches(t *testing.T) { byParentPrefix := []byte{versionV3, typeIndex, idxResourceByParent} // Branch 1: no prior → written, indexed under parent-A. - require.NoError(t, e.PutResourceRecordsIfNewer(ctx, mk("parent-A", old))) + require.NoError(t, e.putResourceRecordsIfNewer(ctx, mk("parent-A", old))) require.Equal(t, 1, countKeys(t, e, byParentPrefix)) // Branch 2: strictly newer → overwritten, index swapped to parent-B. - require.NoError(t, e.PutResourceRecordsIfNewer(ctx, mk("parent-B", newer))) + require.NoError(t, e.putResourceRecordsIfNewer(ctx, mk("parent-B", newer))) require.Equal(t, 1, countKeys(t, e, byParentPrefix), "old parent edge must be cleaned, new one written") got, err := e.GetResourceRecord(ctx, "group", "r1") require.NoError(t, err) require.Equal(t, "parent-B", got.GetParent().GetResourceId()) // Branch 3: not newer → skipped entirely (record and index untouched). - require.NoError(t, e.PutResourceRecordsIfNewer(ctx, mk("parent-C", old))) + require.NoError(t, e.putResourceRecordsIfNewer(ctx, mk("parent-C", old))) got, err = e.GetResourceRecord(ctx, "group", "r1") require.NoError(t, err) require.Equal(t, "parent-B", got.GetParent().GetResourceId(), "stale write must not land") diff --git a/pkg/dotc1z/format/v3/indexed.go b/pkg/dotc1z/format/v3/indexed.go index 1d051df4c..8aa8a105e 100644 --- a/pkg/dotc1z/format/v3/indexed.go +++ b/pkg/dotc1z/format/v3/indexed.go @@ -176,6 +176,14 @@ type SpliceStats struct { SplicedBytes int64 // compressed bytes copied verbatim EncodedFrames int EncodedBytes int64 // raw bytes freshly compressed + + // ReuseMissingPath names the splice source that was configured but had + // vanished by save time, so every frame was compressed afresh. It is the + // only outward difference between "nothing was reusable" and "reuse was + // impossible": both report zero spliced frames, but the second turns an + // O(changed frames) save into an O(payload) one. Non-empty means a caller + // should say so — a stale source path is a bug, not a normal outcome. + ReuseMissingPath string } // writeIndexedZstd writes the indexed payload for dir to w in a @@ -208,11 +216,19 @@ func writeIndexedZstd(w io.Writer, payloadStart int64, manifestXXH64 uint64, dir var srcFile *os.File if reuse != nil && reuse.srcPath != "" { f, err := os.Open(reuse.srcPath) - if err != nil { + switch { + case err == nil: + srcFile = f + defer srcFile.Close() + case errors.Is(err, os.ErrNotExist): + // Reuse is an optimization, not a durability dependency. The + // extracted payload is complete, so if its source envelope was + // removed while the store was open, encode every frame afresh — + // but report it, because the cost is a full re-encode. + stats.ReuseMissingPath = reuse.srcPath + default: return stats, fmt.Errorf("c1z v3: open splice source: %w", err) } - srcFile = f - defer srcFile.Close() } // WithZeroFrames is pinned (it is the library default today, but diff --git a/pkg/dotc1z/format/v3/indexed_test.go b/pkg/dotc1z/format/v3/indexed_test.go index 8fef9a576..212218f99 100644 --- a/pkg/dotc1z/format/v3/indexed_test.go +++ b/pkg/dotc1z/format/v3/indexed_test.go @@ -279,6 +279,44 @@ func TestIndexedSpliceHashFallback(t *testing.T) { require.NoError(t, err) require.Equal(t, 1, stats.SplicedFrames, "spliced frames") require.Equal(t, 0, stats.EncodedFrames, "encoded frames") + require.Empty(t, stats.ReuseMissingPath, "the splice source was present") +} + +func TestIndexedSpliceMissingSourceFallsBackToEncoding(t *testing.T) { + content := randomBytes(t, 64<<10) + dir := writeTestPayloadDir(t, map[string][]byte{"000001.sst": content}) + srcPath := writeIndexedEnvelope(t, dir) + + f, err := os.Open(srcPath) + require.NoError(t, err) + extracted := t.TempDir() + _, reuse, err := ExtractEnvelopePayload(f, extracted) + require.NoError(t, err) + require.NoError(t, f.Close()) + require.NoError(t, os.Remove(srcPath)) + + dstPath := filepath.Join(t.TempDir(), "dst.c1z") + dst, err := os.Create(dstPath) + require.NoError(t, err) + stats, err := WriteEnvelopeWithReuse(dst, indexedManifest(), extracted, reuse) + require.NoError(t, err) + require.NoError(t, dst.Close()) + require.Zero(t, stats.SplicedFrames) + require.Equal(t, 1, stats.EncodedFrames) + // Zero spliced frames alone cannot distinguish "nothing was reusable" + // from "reuse was impossible", so the missing source has to be named or + // the full re-encode is invisible to callers. + require.Equal(t, srcPath, stats.ReuseMissingPath) + + df, err := os.Open(dstPath) + require.NoError(t, err) + defer df.Close() + decoded := t.TempDir() + _, _, err = ExtractEnvelopePayload(df, decoded) + require.NoError(t, err) + got, err := os.ReadFile(filepath.Join(decoded, "000001.sst")) + require.NoError(t, err) + require.Equal(t, content, got) } // writerOnly hides every method except Write, modeling a non-seekable diff --git a/pkg/dotc1z/ingest_invariant_store.go b/pkg/dotc1z/ingest_invariant_store.go index 65ffbdbb9..9f4469d0b 100644 --- a/pkg/dotc1z/ingest_invariant_store.go +++ b/pkg/dotc1z/ingest_invariant_store.go @@ -2,6 +2,8 @@ package dotc1z import ( "context" + + "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble" ) // IngestInvariantStore is the optional store capability backing the @@ -86,7 +88,9 @@ func (s *pebbleStore) ForEachDanglingGrantEntitlement(ctx context.Context, visit } func (s *pebbleStore) EnsureGrantIndexes(ctx context.Context) error { - return s.Engine.EnsureGrantIndexes(ctx) + return s.withMutation(func(e *pebble.Engine) error { + return e.EnsureGrantIndexes(ctx) + }) } func (s *pebbleStore) ForEachDanglingGrantPrincipal(ctx context.Context, visit func(principalRT, principalID string, matchAnnotatedOnly bool, carrierGrants int64) error) error { diff --git a/pkg/dotc1z/migrate_c1z_test.go b/pkg/dotc1z/migrate_c1z_test.go index fe7de2e26..9df8d3c29 100644 --- a/pkg/dotc1z/migrate_c1z_test.go +++ b/pkg/dotc1z/migrate_c1z_test.go @@ -17,6 +17,8 @@ func TestMigrateC1ZOnly(t *testing.T) { ctx := context.Background() store, err := NewStore(ctx, path) require.NoError(t, err) - require.True(t, enginepebble.MarkStoreDirty(store)) + require.NoError(t, enginepebble.WithEngineMutation(ctx, store, func(context.Context, *enginepebble.Engine) error { + return nil + })) require.NoError(t, store.Close(ctx)) } diff --git a/pkg/dotc1z/pebble_store.go b/pkg/dotc1z/pebble_store.go index 05652cddf..a17a65eb8 100644 --- a/pkg/dotc1z/pebble_store.go +++ b/pkg/dotc1z/pebble_store.go @@ -123,7 +123,7 @@ func (pebbleDriver) OpenStore(ctx context.Context, outputFilePath string, opts S return nil, cleanupOnError(err) } - return &pebbleStore{ + store := &pebbleStore{ Engine: e, outputFilePath: outputFilePath, tmpDir: tmpDir, @@ -138,7 +138,9 @@ func (pebbleDriver) OpenStore(ctx context.Context, outputFilePath string, opts S // never writes, or every subsequent open re-pays the O(rows) // migration. dirty: !opts.ReadOnly && e.MigratedOnOpen(), - }, nil + } + store.closeCond = sync.NewCond(&store.closeMu) + return store, nil } func unpackExistingPebbleC1Z( @@ -204,7 +206,12 @@ func payloadEncodingFromProto(enc c1zv3.PayloadEncoding) c1zstore.PayloadEncodin } type pebbleStore struct { - *pebble.Engine + // Deliberately a named field, not an embedded one. Embedding promoted + // every engine mutator onto the store, where it bypassed withMutation + // silently; see pebble_store_reads.go. Reads are forwarded explicitly + // there, mutators go through the wrappers below, and nothing else on + // the engine is reachable as a store method. + Engine *pebble.Engine outputFilePath string tmpDir string readOnly bool @@ -213,7 +220,7 @@ type pebbleStore struct { // foldDeadBytes is the cumulative fold-waste counter carried in // the envelope manifest (C1ZManifestV3.fold_dead_bytes): seeded // from the file this store was opened from, optionally bumped by - // AddFoldDeadBytes during a fold compaction, and written back at + // RunPebbleFoldMutation during a fold compaction, and written back at // save. Guarded by closeMu (writes happen on the compactor's // single merge goroutine; the lock just pairs it with save/Close). foldDeadBytes int64 @@ -225,9 +232,206 @@ type pebbleStore struct { syncLimit int skipCleanup bool - closeMu sync.Mutex - closed bool - dirty bool + closeMu sync.Mutex + closeCond *sync.Cond + admission pebbleStoreAdmissionState + // activeWrites counts admitted mutations; a teardown drains them to zero + // before touching the engine. closeAttempt distinguishes successive + // teardown attempts so a waiter can tell "the attempt I was waiting on + // finished" from "a later one started". + activeWrites int + closeAttempt uint64 + dirty bool + + // mutationAdmissionHook is test-only. It runs after admission and + // immediately before the underlying mutation, without closeMu held. + mutationAdmissionHook func() + + // drainWarnInterval overrides how often a blocked teardown reports + // itself. Zero means pebbleStoreDrainWarnInterval; tests shorten it. + drainWarnInterval time.Duration +} + +type pebbleStoreAdmissionState uint8 + +const ( + pebbleStoreOpen pebbleStoreAdmissionState = iota + pebbleStoreClosing + pebbleStoreClosed +) + +func (s *pebbleStore) cond() *sync.Cond { + if s.closeCond == nil { + s.closeCond = sync.NewCond(&s.closeMu) + } + return s.closeCond +} + +// beginClose takes ownership of a teardown, closing admission and draining +// in-flight mutations before returning. It reports false when the store is +// already closed and there is nothing left to do. +// +// A caller whose attempt fails must reopenAdmission so a waiter can take over. +// Waiters then retry the teardown on their own behalf rather than adopting the +// failed attempt's error: the two callers may be closing for unrelated reasons +// (CloseEngineOnly's dirty refusal is not a reason for Close to skip its save), +// and inheriting a foreign error let one caller's refusal silently cancel +// another caller's save. +// +// The drain is deliberately not cancellable. Abandoning it would leave admitted +// writes running against an engine the caller is about to close, which is the +// failure mode admission control exists to prevent. +func (s *pebbleStore) beginClose() bool { + s.closeMu.Lock() + defer s.closeMu.Unlock() + for { + switch s.admission { + case pebbleStoreClosed: + return false + case pebbleStoreOpen: + s.admission = pebbleStoreClosing + s.closeAttempt++ + for s.activeWrites > 0 { + s.cond().Wait() + } + return true + case pebbleStoreClosing: + attempt := s.closeAttempt + for s.admission == pebbleStoreClosing && s.closeAttempt == attempt { + s.cond().Wait() + } + } + } +} + +// pebbleStoreDrainWarnInterval is how often a teardown blocked in beginClose +// announces itself. +const pebbleStoreDrainWarnInterval = 30 * time.Second + +// warnWhileBlocked reports a teardown that has been waiting long enough to be +// suspicious, and returns a func that stops the reporting. +// +// beginClose's wait is deliberately uncancellable, and a single admission can +// span an entire compaction (compactPebble and compactPebbleFold each hold one +// for the whole merge). A wedged merge or a frozen Lambda therefore turns +// Close into an unbounded block with no ctx.Err() to surface and nothing in +// the logs — diagnosable today only from a stack dump. Naming activeWrites and +// the elapsed time makes it visible in the places an operator actually looks. +func (s *pebbleStore) warnWhileBlocked(ctx context.Context) func() { + interval := s.drainWarnInterval + if interval <= 0 { + interval = pebbleStoreDrainWarnInterval + } + l := ctxzap.Extract(ctx) + start := time.Now() + stop := make(chan struct{}) + stopped := make(chan struct{}) + go func() { + defer close(stopped) + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-stop: + return + case <-ticker.C: + s.closeMu.Lock() + active := s.activeWrites + s.closeMu.Unlock() + waitingOn := "another teardown attempt" + if active > 0 { + waitingOn = "in-flight mutations" + } + l.Warn("pebble store close: still waiting to tear down", + zap.Duration("elapsed", time.Since(start)), + zap.Int("active_writes", active), + zap.String("waiting_on", waitingOn), + ) + } + } + }() + return func() { + close(stop) + <-stopped + } +} + +// isDirty reports whether the store has unsaved writes. Callers that own a +// teardown can trust the answer to stay put: admission is already shut and +// beginClose drained the in-flight mutations, so nothing can set it after this. +func (s *pebbleStore) isDirty() bool { + s.closeMu.Lock() + defer s.closeMu.Unlock() + return s.dirty +} + +// reopenAdmission abandons a teardown attempt, readmitting mutations and waking +// any caller waiting to take over. +func (s *pebbleStore) reopenAdmission() { + s.closeMu.Lock() + s.admission = pebbleStoreOpen + s.cond().Broadcast() + s.closeMu.Unlock() +} + +// finishClose publishes the terminal state and releases waiters. +func (s *pebbleStore) finishClose() { + s.closeMu.Lock() + s.admission = pebbleStoreClosed + s.cond().Broadcast() + s.closeMu.Unlock() +} + +// withMutation admits one synchronous mutation. Admission and release only +// hold closeMu briefly; admitted mutations execute concurrently. Dirty is set +// before invoking fn because an error may follow a partially committed write. +func (s *pebbleStore) withMutation(fn func(*pebble.Engine) error) error { + return s.admitMutation(true, fn) +} + +// admitMutation is the shared admission body. dirtyOnAdmission is true for +// ordinary mutations, where a returned error can still follow a committed +// write, so the envelope must be saved either way. The fold path passes false +// and marks dirty itself once the fold has fully landed: a failed fold's +// destination is a throwaway copy under the compactor's temp dir, and saving it +// would spend a full envelope write on an artifact about to be discarded. +func (s *pebbleStore) admitMutation(dirtyOnAdmission bool, fn func(*pebble.Engine) error) error { + if s == nil || s.Engine == nil { + return pebble.ErrEngineClosing + } + s.closeMu.Lock() + if s.admission != pebbleStoreOpen { + s.closeMu.Unlock() + return pebble.ErrEngineClosing + } + s.activeWrites++ + if dirtyOnAdmission && !s.readOnly { + s.dirty = true + } + hook := s.mutationAdmissionHook + s.closeMu.Unlock() + + defer func() { + s.closeMu.Lock() + s.activeWrites-- + if s.activeWrites == 0 { + s.cond().Broadcast() + } + s.closeMu.Unlock() + }() + if hook != nil { + hook() + } + return fn(s.Engine) +} + +// RunPebbleMutation implements pebble's guarded direct-engine mutation +// callback. The engine is explicit so callers cannot accidentally mutate via +// promoted pebbleStore methods outside admission. +func (s *pebbleStore) RunPebbleMutation(ctx context.Context, fn func(context.Context, *pebble.Engine) error) error { + return s.withMutation(func(e *pebble.Engine) error { + return fn(ctx, e) + }) } // Compile-time guard: a Pebble store satisfies the full C1ZStore @@ -248,7 +452,8 @@ var _ c1zstore.Store = (*pebbleStore)(nil) // save and the diff sync would exist only in the discarded temp // directory. func (s *pebbleStore) FileOps() c1zstore.FileOps { - return pebbleStoreFileOps{inner: s.FileOpsWithEncoding(s.payloadEncoding), store: s} + engine := s.Engine + return pebbleStoreFileOps{inner: engine.FileOpsWithEncoding(s.payloadEncoding), store: s} } // pebbleStoreFileOps wraps the Adapter-level FileOps to route the one @@ -269,11 +474,13 @@ func (f pebbleStoreFileOps) CopyIsolateSync(ctx context.Context, outPath string, } func (f pebbleStoreFileOps) GenerateSyncDiff(ctx context.Context, baseSyncID, appliedSyncID string) (string, error) { - diffSyncID, err := f.inner.GenerateSyncDiff(ctx, baseSyncID, appliedSyncID) - if err != nil { - return "", err - } - return diffSyncID, f.store.markDirty(nil) + var diffSyncID string + err := f.store.withMutation(func(_ *pebble.Engine) error { + var err error + diffSyncID, err = f.inner.GenerateSyncDiff(ctx, baseSyncID, appliedSyncID) + return err + }) + return diffSyncID, err } // SyncMeta overrides the Adapter-level SyncMeta so the MUTATING @@ -297,7 +504,9 @@ type pebbleStoreSyncMeta struct { var _ c1zstore.IngestInvariantVerificationWriter = pebbleStoreSyncMeta{} func (m pebbleStoreSyncMeta) MarkSyncSupportsDiff(ctx context.Context, syncID string) error { - return m.store.markDirty(m.inner.MarkSyncSupportsDiff(ctx, syncID)) + return m.store.withMutation(func(_ *pebble.Engine) error { + return m.inner.MarkSyncSupportsDiff(ctx, syncID) + }) } func (m pebbleStoreSyncMeta) MarkIngestInvariantsVerified(ctx context.Context, syncID string, verification c1zstore.IngestInvariantVerification) error { @@ -305,7 +514,9 @@ func (m pebbleStoreSyncMeta) MarkIngestInvariantsVerified(ctx context.Context, s if !ok { return errors.New("pebble sync meta: engine SyncMeta does not implement IngestInvariantVerificationWriter") } - return m.store.markDirty(w.MarkIngestInvariantsVerified(ctx, syncID, verification)) + return m.store.withMutation(func(_ *pebble.Engine) error { + return w.MarkIngestInvariantsVerified(ctx, syncID, verification) + }) } func (m pebbleStoreSyncMeta) ClearIngestInvariantVerification(ctx context.Context, syncID string) error { @@ -313,11 +524,15 @@ func (m pebbleStoreSyncMeta) ClearIngestInvariantVerification(ctx context.Contex if !ok { return errors.New("pebble sync meta: engine SyncMeta does not implement IngestInvariantVerificationWriter") } - return m.store.markDirty(w.ClearIngestInvariantVerification(ctx, syncID)) + return m.store.withMutation(func(_ *pebble.Engine) error { + return w.ClearIngestInvariantVerification(ctx, syncID) + }) } func (m pebbleStoreSyncMeta) RecalculateStats(ctx context.Context, syncID string) error { - return m.store.markDirty(m.inner.RecalculateStats(ctx, syncID)) + return m.store.withMutation(func(_ *pebble.Engine) error { + return m.inner.RecalculateStats(ctx, syncID) + }) } func (m pebbleStoreSyncMeta) LatestFullSync(ctx context.Context) (*c1zstore.SyncRun, error) { @@ -373,18 +588,16 @@ func (s *pebbleStore) CloseEngineOnly() error { if s == nil || s.Engine == nil { return nil } - s.closeMu.Lock() - if s.closed { - s.closeMu.Unlock() + if !s.beginClose() { return nil } - if !s.readOnly && s.dirty { - s.closeMu.Unlock() + if !s.readOnly && s.isDirty() { + s.reopenAdmission() return errors.New("pebble CloseEngineOnly: refusing to discard dirty writable store") } - s.closed = true - s.closeMu.Unlock() - return s.Engine.Close() + err := s.Engine.Close() + s.finishClose() + return err } // NormalizeForFixtureSave flushes and compacts the single sync, then @@ -401,51 +614,39 @@ func (s *pebbleStore) NormalizeForFixtureSave(ctx context.Context, syncID string if s.readOnly { return errors.New("pebble NormalizeForFixtureSave: store is read-only") } - if err := s.Flush(ctx); err != nil { - return err - } - if err := s.CompactAllRanges(ctx); err != nil { - return err - } - s.closeMu.Lock() - if !s.closed { - s.dirty = true - } - s.closeMu.Unlock() - return nil -} - -func (s *pebbleStore) MarkDirty() { - if s == nil { - return - } - s.closeMu.Lock() - if !s.closed { - s.dirty = true - } - s.closeMu.Unlock() -} - -// AddFoldDeadBytes bumps the cumulative fold-waste counter persisted -// in the envelope manifest at save. Called by the fold compactor with -// the raw bytes its merge shadowed in the base keyspace (see -// pebble.AddFoldDeadBytes for the Writer-level accessor). -func (s *pebbleStore) AddFoldDeadBytes(n int64) { - if s == nil || n <= 0 { - return - } - s.closeMu.Lock() - if !s.closed { - s.foldDeadBytes += n - } - s.closeMu.Unlock() + return s.withMutation(func(e *pebble.Engine) error { + if err := e.Flush(ctx); err != nil { + return err + } + return e.CompactAllRanges(ctx) + }) } -func (s *pebbleStore) markDirty(err error) error { - if err == nil { - s.MarkDirty() - } - return err +// RunPebbleFoldMutation keeps fold-dead-byte manifest metadata in the same +// admission as the direct engine mutations that produced it: activeWrites is +// still held here, so a concurrent Close is parked in its drain and cannot +// save an envelope whose counter is only half updated. +// +// Nothing is recorded on failure. A fold either lands whole or its output is +// discarded, so a failed attempt must leave the store exactly as it found it: +// dirty stays put (Close then skips the O(base) save of a doomed artifact) and +// the waste counter keeps describing only folds that actually shadowed bytes. +func (s *pebbleStore) RunPebbleFoldMutation(ctx context.Context, fn func(context.Context, *pebble.Engine) (int64, error)) error { + return s.admitMutation(false, func(e *pebble.Engine) error { + n, err := fn(ctx, e) + if err != nil { + return err + } + s.closeMu.Lock() + if !s.readOnly { + s.dirty = true + } + if n > 0 { + s.foldDeadBytes += n + } + s.closeMu.Unlock() + return nil + }) } // StartNewSync begins a sync on the Pebble v3 store. INVARIANT: a v3 Pebble @@ -456,31 +657,62 @@ func (s *pebbleStore) markDirty(err error) error { // StartNewSync after the first would discard the previous sync's records. // Future engine authors relying on this contract should preserve it here. func (s *pebbleStore) StartNewSync(ctx context.Context, syncType connectorstore.SyncType, parentSyncID string) (string, error) { - syncID, err := s.Engine.StartNewSync(ctx, syncType, parentSyncID) - if err == nil { - s.closeMu.Lock() - s.dirty = true - s.closeMu.Unlock() - } + var syncID string + err := s.withMutation(func(e *pebble.Engine) error { + var err error + syncID, err = e.StartNewSync(ctx, syncType, parentSyncID) + return err + }) return syncID, err } +func (s *pebbleStore) StartNewSyncWithID(ctx context.Context, syncType connectorstore.SyncType, syncID, parentSyncID string) (string, error) { + var id string + err := s.withMutation(func(e *pebble.Engine) error { + var err error + id, err = e.StartNewSyncWithID(ctx, syncType, syncID, parentSyncID) + return err + }) + return id, err +} + +func (s *pebbleStore) ResumeSync(ctx context.Context, syncType connectorstore.SyncType, syncID string) (string, error) { + var id string + err := s.withMutation(func(e *pebble.Engine) error { + var err error + id, err = e.ResumeSync(ctx, syncType, syncID) + return err + }) + return id, err +} + func (s *pebbleStore) StartOrResumeSync(ctx context.Context, syncType connectorstore.SyncType, syncID string) (string, bool, error) { - id, started, err := s.Engine.StartOrResumeSync(ctx, syncType, syncID) - if err == nil && started { - s.closeMu.Lock() - s.dirty = true - s.closeMu.Unlock() - } + var id string + var started bool + err := s.withMutation(func(e *pebble.Engine) error { + var err error + id, started, err = e.StartOrResumeSync(ctx, syncType, syncID) + return err + }) return id, started, err } +func (s *pebbleStore) SetCurrentSync(ctx context.Context, syncID string) error { + return s.withMutation(func(e *pebble.Engine) error { + return e.SetCurrentSync(ctx, syncID) + }) +} + func (s *pebbleStore) CheckpointSync(ctx context.Context, syncToken string) error { - return s.markDirty(s.Engine.CheckpointSync(ctx, syncToken)) + return s.withMutation(func(e *pebble.Engine) error { + return e.CheckpointSync(ctx, syncToken) + }) } func (s *pebbleStore) EndSync(ctx context.Context) error { - return s.markDirty(s.Engine.EndSync(ctx)) + return s.withMutation(func(e *pebble.Engine) error { + return e.EndSync(ctx) + }) } // Cleanup is a no-op for the Pebble v3 engine. A c1z holds exactly one @@ -495,7 +727,9 @@ func (s *pebbleStore) Cleanup(ctx context.Context) error { } func (s *pebbleStore) PutAsset(ctx context.Context, assetRef *v2.AssetRef, contentType string, data []byte) error { - return s.markDirty(s.Engine.PutAsset(ctx, assetRef, contentType, data)) + return s.withMutation(func(e *pebble.Engine) error { + return e.PutAsset(ctx, assetRef, contentType, data) + }) } // SetSupportsDiff marks the given sync as diff-capable, matching the @@ -504,7 +738,9 @@ func (s *pebbleStore) PutAsset(ctx context.Context, assetRef *v2.AssetRef, conte // output remains usable wherever the source was. Delegates to the // SyncMeta sub-store's MarkSyncSupportsDiff. func (s *pebbleStore) SetSupportsDiff(ctx context.Context, syncID string) error { - return s.markDirty(s.SyncMeta().MarkSyncSupportsDiff(ctx, syncID)) + return s.withMutation(func(e *pebble.Engine) error { + return e.SyncMeta().MarkSyncSupportsDiff(ctx, syncID) + }) } // SetSyncLink records linkedSyncID as the diff partner of syncID on the @@ -519,19 +755,23 @@ func (s *pebbleStore) SetSyncLink(ctx context.Context, syncID string, linkedSync if syncID == "" { return fmt.Errorf("SetSyncLink: empty syncID") } - r, err := s.GetSyncRunRecord(ctx, syncID) - if err != nil { - return fmt.Errorf("SetSyncLink: get: %w", err) - } - r.SetLinkedSyncId(linkedSyncID) - if err := s.PutSyncRunRecord(ctx, r); err != nil { - return fmt.Errorf("SetSyncLink: put: %w", err) - } - return s.markDirty(nil) + return s.withMutation(func(e *pebble.Engine) error { + r, err := e.GetSyncRunRecord(ctx, syncID) + if err != nil { + return fmt.Errorf("SetSyncLink: get: %w", err) + } + r.SetLinkedSyncId(linkedSyncID) + if err := e.PutSyncRunRecord(ctx, r); err != nil { + return fmt.Errorf("SetSyncLink: put: %w", err) + } + return nil + }) } func (s *pebbleStore) PutGrants(ctx context.Context, grants ...*v2.Grant) error { - return s.markDirty(s.Engine.PutGrants(ctx, grants...)) + return s.withMutation(func(e *pebble.Engine) error { + return e.PutGrants(ctx, grants...) + }) } // UnsafePutUniqueGrants is the trusted-import write path (no @@ -539,48 +779,64 @@ func (s *pebbleStore) PutGrants(ctx context.Context, grants ...*v2.Grant) error // connector output. Caller must guarantee unique external_ids across the whole // destination sync. See pebble.Adapter.UnsafePutUniqueGrants. func (s *pebbleStore) UnsafePutUniqueGrants(ctx context.Context, grants ...*v2.Grant) error { - return s.markDirty(s.Engine.UnsafePutUniqueGrants(ctx, grants...)) + return s.withMutation(func(e *pebble.Engine) error { + return e.UnsafePutUniqueGrants(ctx, grants...) + }) } func (s *pebbleStore) PutResourceTypes(ctx context.Context, resourceTypes ...*v2.ResourceType) error { - return s.markDirty(s.Engine.PutResourceTypes(ctx, resourceTypes...)) + return s.withMutation(func(e *pebble.Engine) error { + return e.PutResourceTypes(ctx, resourceTypes...) + }) } func (s *pebbleStore) PutResources(ctx context.Context, resources ...*v2.Resource) error { - return s.markDirty(s.Engine.PutResources(ctx, resources...)) + return s.withMutation(func(e *pebble.Engine) error { + return e.PutResources(ctx, resources...) + }) } func (s *pebbleStore) PutEntitlements(ctx context.Context, entitlements ...*v2.Entitlement) error { - return s.markDirty(s.Engine.PutEntitlements(ctx, entitlements...)) + return s.withMutation(func(e *pebble.Engine) error { + return e.PutEntitlements(ctx, entitlements...) + }) } func (s *pebbleStore) DeleteGrant(ctx context.Context, grantID string) error { - return s.markDirty(s.Engine.DeleteGrant(ctx, grantID)) + return s.withMutation(func(e *pebble.Engine) error { + return e.DeleteGrant(ctx, grantID) + }) } // DeleteGrantByRefs is the exact grant delete for callers holding the full // grant: identity derives from the structured refs, never the lossy id // string. The syncer prefers this when available. func (s *pebbleStore) DeleteGrantByRefs(ctx context.Context, grant *v2.Grant) error { - return s.markDirty(s.Engine.DeleteGrantByRefs(ctx, grant)) + return s.withMutation(func(e *pebble.Engine) error { + return e.DeleteGrantByRefs(ctx, grant) + }) } // DeleteResourceRecord removes a resource and marks the envelope dirty so an // explicit reconciliation performed by the syncer is persisted on Close. func (s *pebbleStore) DeleteResourceRecord(ctx context.Context, resourceTypeID, resourceID string) error { - return s.markDirty(s.Engine.DeleteResourceRecord(ctx, resourceTypeID, resourceID)) + return s.withMutation(func(e *pebble.Engine) error { + return e.DeleteResourceRecord(ctx, resourceTypeID, resourceID) + }) } // DeleteEntitlementByRefs removes one exact entitlement identity and preserves // the mutation when the envelope is closed. func (s *pebbleStore) DeleteEntitlementByRefs(ctx context.Context, entitlement *v2.Entitlement) error { resourceID := entitlement.GetResource().GetId() - return s.markDirty(s.DeleteEntitlementRecordByIdentity( - ctx, - resourceID.GetResourceType(), - resourceID.GetResource(), - entitlement.GetId(), - )) + return s.withMutation(func(e *pebble.Engine) error { + return e.DeleteEntitlementRecordByIdentity( + ctx, + resourceID.GetResourceType(), + resourceID.GetResource(), + entitlement.GetId(), + ) + }) } // Grants overrides Adapter.Grants() so the returned GrantStore @@ -608,23 +864,31 @@ var pebbleStoreExpandedGrantImmutableAnnotationAny = func() *anypb.Any { }() func (g pebbleStoreGrants) StoreExpandedGrants(ctx context.Context, grants ...*v2.Grant) error { - return g.store.markDirty(g.inner.StoreExpandedGrants(ctx, grants...)) + return g.store.withMutation(func(_ *pebble.Engine) error { + return g.inner.StoreExpandedGrants(ctx, grants...) + }) } func (g pebbleStoreGrants) StoreNewExpandedGrants(ctx context.Context, grants ...*v2.Grant) error { if fast, ok := g.inner.(interface { StoreNewExpandedGrants(context.Context, ...*v2.Grant) error }); ok { - return g.store.markDirty(fast.StoreNewExpandedGrants(ctx, grants...)) + return g.store.withMutation(func(_ *pebble.Engine) error { + return fast.StoreNewExpandedGrants(ctx, grants...) + }) } - return g.store.markDirty(g.inner.StoreExpandedGrants(ctx, grants...)) + return g.store.withMutation(func(_ *pebble.Engine) error { + return g.inner.StoreExpandedGrants(ctx, grants...) + }) } func (g pebbleStoreGrants) StoreNewExpandedGrantContributions(ctx context.Context, dest *v2.Entitlement, principals []*v3.PrincipalRef, sources []batonGrant.Sources) error { if fast, ok := g.inner.(interface { StoreNewExpandedGrantContributions(context.Context, *v2.Entitlement, []*v3.PrincipalRef, []batonGrant.Sources) error }); ok { - return g.store.markDirty(fast.StoreNewExpandedGrantContributions(ctx, dest, principals, sources)) + return g.store.withMutation(func(_ *pebble.Engine) error { + return fast.StoreNewExpandedGrantContributions(ctx, dest, principals, sources) + }) } grants := make([]*v2.Grant, 0, len(principals)) for i, principalRef := range principals { @@ -635,7 +899,9 @@ func (g pebbleStoreGrants) StoreNewExpandedGrantContributions(ctx context.Contex } grants = append(grants, grant) } - return g.store.markDirty(g.inner.StoreExpandedGrants(ctx, grants...)) + return g.store.withMutation(func(_ *pebble.Engine) error { + return g.inner.StoreExpandedGrants(ctx, grants...) + }) } // pebbleStoreGrantLayerStorer is the layer-scoped layer session surface the @@ -650,7 +916,13 @@ type pebbleStoreGrantLayerStorer interface { func (g pebbleStoreGrants) BeginExpandedGrantLayer(ctx context.Context) (bool, error) { if fast, ok := g.inner.(pebbleStoreGrantLayerStorer); ok { - return fast.BeginExpandedGrantLayer(ctx) + var started bool + err := g.store.withMutation(func(_ *pebble.Engine) error { + var err error + started, err = fast.BeginExpandedGrantLayer(ctx) + return err + }) + return started, err } return false, nil } @@ -660,7 +932,9 @@ func (g pebbleStoreGrants) AddExpandedGrantLayerContributions(ctx context.Contex if !ok { return fmt.Errorf("expanded grant layer: store does not support layer sessions") } - return fast.AddExpandedGrantLayerContributions(ctx, dest, principals, sources) + return g.store.withMutation(func(_ *pebble.Engine) error { + return fast.AddExpandedGrantLayerContributions(ctx, dest, principals, sources) + }) } func (g pebbleStoreGrants) FinishExpandedGrantLayer(ctx context.Context) error { @@ -668,7 +942,9 @@ func (g pebbleStoreGrants) FinishExpandedGrantLayer(ctx context.Context) error { if !ok { return fmt.Errorf("expanded grant layer: store does not support layer sessions") } - return g.store.markDirty(fast.FinishExpandedGrantLayer(ctx)) + return g.store.withMutation(func(_ *pebble.Engine) error { + return fast.FinishExpandedGrantLayer(ctx) + }) } func (g pebbleStoreGrants) AbortExpandedGrantLayer(ctx context.Context) error { @@ -676,7 +952,9 @@ func (g pebbleStoreGrants) AbortExpandedGrantLayer(ctx context.Context) error { if !ok { return nil } - return fast.AbortExpandedGrantLayer(ctx) + return g.store.withMutation(func(_ *pebble.Engine) error { + return fast.AbortExpandedGrantLayer(ctx) + }) } func newPebbleStorePrincipalResource(ref *v3.PrincipalRef) *v2.Resource { @@ -744,37 +1022,43 @@ func (g pebbleStoreGrants) ListWithAnnotations(ctx context.Context) iter.Seq2[c1 return g.inner.ListWithAnnotations(ctx) } -func (s *pebbleStore) Close(ctx context.Context) (retErr error) { - s.closeMu.Lock() - defer s.closeMu.Unlock() - if s.closed { +// Close saves the envelope when the store is dirty, then tears it down. It is +// idempotent: once the store is closed, later calls report nil per io.Closer +// convention, so a deferred Close following an explicit one does not re-report +// a failure the caller already handled. +func (s *pebbleStore) Close(ctx context.Context) error { + stopWarning := s.warnWhileBlocked(ctx) + owned := s.beginClose() + stopWarning() + if !owned { return nil } - if !s.readOnly && s.dirty { + if !s.readOnly && s.isDirty() { if err := s.save(ctx); err != nil { - // Tear NOTHING down: the unpacked DB under tmpDir is the only - // copy of the synced data, and save failures are frequently - // transient (target-path permissions, disk space for the - // envelope). Leave the store open so the caller can fix the - // condition and Close again; if the process exits instead, the - // temp dir survives on disk for manual recovery rather than - // being deleted out from under a failed save. + // Tear NOTHING down: the unpacked DB under tmpDir is the only copy + // of the synced data, and save failures are frequently transient + // (target-path permissions, disk space for the envelope). Leave the + // store open so the caller can fix the condition and Close again; + // if the process exits instead, the temp dir survives on disk for + // manual recovery rather than being deleted out from under a failed + // save. + s.reopenAdmission() return fmt.Errorf("pebble store close: save failed, store left open and unsaved data preserved under %s: %w", s.tmpDir, err) } + s.closeMu.Lock() s.dirty = false + s.closeMu.Unlock() } - s.closed = true - - defer func() { - if removeErr := os.RemoveAll(s.tmpDir); removeErr != nil { - retErr = errors.Join(retErr, removeErr) - } - }() + var retErr error if err := s.Engine.Close(); err != nil { retErr = errors.Join(retErr, err) } + if removeErr := os.RemoveAll(s.tmpDir); removeErr != nil { + retErr = errors.Join(retErr, removeErr) + } + s.finishClose() return retErr } @@ -791,7 +1075,8 @@ func (s *pebbleStore) save(ctx context.Context) error { if err := os.RemoveAll(checkpointDir); err != nil { return fmt.Errorf("pebble save: clear stale checkpoint dir: %w", err) } - if err := s.CheckpointTo(ctx, checkpointDir); err != nil { + engine := s.Engine + if err := engine.CheckpointTo(ctx, checkpointDir); err != nil { return err } checkpointDur := time.Since(saveStart) @@ -815,16 +1100,35 @@ func (s *pebbleStore) save(ctx context.Context) error { if err != nil { return err } - if s.foldDeadBytes > 0 { - manifest.SetFoldDeadBytes(s.foldDeadBytes) + s.closeMu.Lock() + foldDeadBytes := s.foldDeadBytes + s.closeMu.Unlock() + if foldDeadBytes > 0 { + manifest.SetFoldDeadBytes(foldDeadBytes) } encodeStart := time.Now() - if _, err := formatv3.WriteEnvelopeWithReuse(out, manifest, checkpointDir, s.payloadReuse); err != nil { + spliceStats, err := formatv3.WriteEnvelopeWithReuse(out, manifest, checkpointDir, s.payloadReuse) + if err != nil { return err } - ctxzap.Extract(ctx).Debug("pebble save: envelope written", + l := ctxzap.Extract(ctx) + if spliceStats.ReuseMissingPath != "" { + // Frame splicing was configured but its source envelope was gone, so + // this save recompressed the entire payload instead of copying the + // unchanged frames. On a whale-scale file that is the difference + // between seconds and many minutes, and nothing else reports it. + l.Warn("pebble save: splice source missing, recompressed the whole payload", + zap.String("splice_source", spliceStats.ReuseMissingPath), + zap.Int("encoded_frames", spliceStats.EncodedFrames), + ) + } + l.Debug("pebble save: envelope written", zap.Duration("checkpoint", checkpointDur), zap.Duration("envelope_encode", time.Since(encodeStart)), + zap.Int("spliced_frames", spliceStats.SplicedFrames), + zap.Int64("spliced_bytes", spliceStats.SplicedBytes), + zap.Int("encoded_frames", spliceStats.EncodedFrames), + zap.Int64("encoded_bytes", spliceStats.EncodedBytes), ) if err := out.Sync(); err != nil { return err diff --git a/pkg/dotc1z/pebble_store_admission_test.go b/pkg/dotc1z/pebble_store_admission_test.go new file mode 100644 index 000000000..288425713 --- /dev/null +++ b/pkg/dotc1z/pebble_store_admission_test.go @@ -0,0 +1,585 @@ +package dotc1z + +import ( + "bytes" + "context" + "errors" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "runtime" + "strings" + "sync" + "testing" + "time" + + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + v3 "github.com/conductorone/baton-sdk/pb/c1/storage/v3" + "github.com/conductorone/baton-sdk/pkg/connectorstore" + "github.com/conductorone/baton-sdk/pkg/dotc1z/c1zstore" + "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble" + batonGrant "github.com/conductorone/baton-sdk/pkg/types/grant" + "github.com/conductorone/baton-sdk/pkg/types/sessions" + "github.com/stretchr/testify/require" +) + +const admissionTestTimeout = 5 * time.Second + +func newAdmissionTestStore(t *testing.T, path string) *pebbleStore { + t.Helper() + store, err := pebbleDriver{}.OpenStore(context.Background(), path, StoreOptions{}) + require.NoError(t, err) + return store.(*pebbleStore) +} + +func waitForAdmissionState(t *testing.T, store *pebbleStore, want pebbleStoreAdmissionState) { + t.Helper() + deadline := time.Now().Add(admissionTestTimeout) + for time.Now().Before(deadline) { + store.closeMu.Lock() + got := store.admission + store.closeMu.Unlock() + if got == want { + return + } + runtime.Gosched() + } + t.Fatalf("timed out waiting for admission state %v", want) +} + +func receiveError(t *testing.T, ch <-chan error) error { + t.Helper() + select { + case err := <-ch: + return err + case <-time.After(admissionTestTimeout): + t.Fatal("timed out waiting for operation") + return nil + } +} + +func TestPebbleStoreMutationAdmissionPersistsBeforeClose(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "admission.c1z") + store := newAdmissionTestStore(t, path) + _, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + admitted := make(chan struct{}) + release := make(chan struct{}) + store.mutationAdmissionHook = func() { + close(admitted) + <-release + } + mutationErr := make(chan error, 1) + go func() { + mutationErr <- store.PutResourceTypes(ctx, v2.ResourceType_builder{Id: "group", DisplayName: "Group"}.Build()) + }() + select { + case <-admitted: + case <-time.After(admissionTestTimeout): + t.Fatal("mutation was not admitted") + } + + closeErr := make(chan error, 2) + go func() { closeErr <- store.Close(ctx) }() + waitForAdmissionState(t, store, pebbleStoreClosing) + go func() { closeErr <- store.Close(ctx) }() + require.ErrorIs(t, store.PutResourceTypes(ctx, v2.ResourceType_builder{Id: "late"}.Build()), pebble.ErrEngineClosing) + + select { + case err := <-closeErr: + t.Fatalf("Close returned before admitted mutation completed: %v", err) + case <-time.After(25 * time.Millisecond): + } + close(release) + require.NoError(t, receiveError(t, mutationErr)) + require.NoError(t, receiveError(t, closeErr)) + require.NoError(t, receiveError(t, closeErr)) + + reopened, err := NewStore(ctx, path, WithReadOnly(true)) + require.NoError(t, err) + defer reopened.Close(ctx) + engine, ok := pebble.AsEngine(reopened) + require.True(t, ok) + rec, err := engine.GetResourceTypeRecord(ctx, "group") + require.NoError(t, err) + require.Equal(t, "Group", rec.GetDisplayName()) +} + +func TestPebbleStoreSessionAdmissionPersistsBeforeClose(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "session-admission.c1z") + store := newAdmissionTestStore(t, path) + syncID, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + admitted := make(chan struct{}) + release := make(chan struct{}) + store.mutationAdmissionHook = func() { + close(admitted) + <-release + } + mutationErr := make(chan error, 1) + go func() { + mutationErr <- store.SessionStore().Set(ctx, "racing", []byte("persisted"), sessions.WithSyncID(syncID)) + }() + select { + case <-admitted: + case <-time.After(admissionTestTimeout): + t.Fatal("session mutation was not admitted") + } + closeErr := make(chan error, 1) + go func() { closeErr <- store.Close(ctx) }() + waitForAdmissionState(t, store, pebbleStoreClosing) + close(release) + require.NoError(t, receiveError(t, mutationErr)) + require.NoError(t, receiveError(t, closeErr)) + + reopened, err := NewStore(ctx, path, WithReadOnly(true)) + require.NoError(t, err) + defer reopened.Close(ctx) + value, found, err := reopened.SessionStore().Get(ctx, "racing", sessions.WithSyncID(syncID)) + require.NoError(t, err) + require.True(t, found) + require.Equal(t, []byte("persisted"), value) +} + +func TestPebbleStoreSaveFailureReopensAdmissionForRetry(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + parent := filepath.Join(root, "missing") + path := filepath.Join(parent, "retry.c1z") + store := newAdmissionTestStore(t, path) + syncID, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.Error(t, store.Close(ctx)) + + require.NoError(t, store.SessionStore().Set(ctx, "after-failure", []byte("kept"), sessions.WithSyncID(syncID))) + require.NoError(t, os.MkdirAll(parent, 0o755)) + require.NoError(t, store.Close(ctx)) + + reopened, err := NewStore(ctx, path, WithReadOnly(true)) + require.NoError(t, err) + defer reopened.Close(ctx) + value, found, err := reopened.SessionStore().Get(ctx, "after-failure", sessions.WithSyncID(syncID)) + require.NoError(t, err) + require.True(t, found) + require.Equal(t, []byte("kept"), value) +} + +func TestPebbleStorePartialMutationErrorRemainsDirty(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "partial-error.c1z") + initial := newAdmissionTestStore(t, path) + syncID, err := initial.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.NoError(t, initial.EndSync(ctx)) + require.NoError(t, initial.Close(ctx)) + + store := newAdmissionTestStore(t, path) + + injected := errors.New("after commit") + err = pebble.WithEngineMutation(ctx, store, func(ctx context.Context, engine *pebble.Engine) error { + if err := engine.SetCurrentSync(ctx, syncID); err != nil { + return err + } + if err := engine.PutResourceTypeRecord(ctx, v3.ResourceTypeRecord_builder{ + ExternalId: "partial", + DisplayName: "Partial", + }.Build()); err != nil { + return err + } + return injected + }) + require.ErrorIs(t, err, injected) + require.NoError(t, store.Close(ctx)) + + reopened, err := NewStore(ctx, path, WithReadOnly(true)) + require.NoError(t, err) + defer reopened.Close(ctx) + engine, ok := pebble.AsEngine(reopened) + require.True(t, ok) + _, err = engine.GetResourceTypeRecord(ctx, "partial") + require.NoError(t, err) +} + +func TestPebbleStoreCloseEngineOnlyAdmission(t *testing.T) { + ctx := context.Background() + clean := newAdmissionTestStore(t, filepath.Join(t.TempDir(), "clean.c1z")) + require.NoError(t, clean.CloseEngineOnly()) + require.ErrorIs(t, clean.PutResourceTypes(ctx, v2.ResourceType_builder{Id: "closed"}.Build()), pebble.ErrEngineClosing) + + path := filepath.Join(t.TempDir(), "dirty.c1z") + dirty := newAdmissionTestStore(t, path) + _, err := dirty.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.Error(t, dirty.CloseEngineOnly()) + require.NoError(t, dirty.PutResourceTypes(ctx, v2.ResourceType_builder{Id: "retry"}.Build())) + require.NoError(t, dirty.Close(ctx)) +} + +func TestPebbleStoreCloseReportsTerminalErrorOnlyOnce(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores the directory permissions this test relies on") + } + ctx := context.Background() + root := t.TempDir() + // The store unpacks into a child of workDir, so making workDir unwritable + // after the open leaves Close unable to unlink that child. That is the + // cheapest way to reach the closed state carrying an error: the save itself + // still succeeds, only the temp-dir cleanup fails. + workDir := filepath.Join(root, "work") + require.NoError(t, os.MkdirAll(workDir, 0o755)) + opened, err := pebbleDriver{}.OpenStore(ctx, filepath.Join(root, "terminal.c1z"), StoreOptions{TmpDir: workDir}) + require.NoError(t, err) + store := opened.(*pebbleStore) + _, err = store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.NoError(t, os.Chmod(workDir, 0o500)) + t.Cleanup(func() { _ = os.Chmod(workDir, 0o755) }) + + require.Error(t, store.Close(ctx)) + + // The store is closed. Later calls have no attempt of their own to fail, so + // they report nil per io.Closer convention — a deferred Close paired with an + // explicit one must not resurface an error the caller already handled. + require.NoError(t, store.Close(ctx)) + require.NoError(t, store.CloseEngineOnly()) +} + +func TestPebbleStoreCloseDoesNotInheritAnotherTeardownsFailure(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "handoff.c1z") + store := newAdmissionTestStore(t, path) + _, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + admitted := make(chan struct{}) + release := make(chan struct{}) + store.mutationAdmissionHook = func() { + close(admitted) + <-release + } + mutationErr := make(chan error, 1) + go func() { + mutationErr <- store.PutResourceTypes(ctx, v2.ResourceType_builder{Id: "group", DisplayName: "Group"}.Build()) + }() + select { + case <-admitted: + case <-time.After(admissionTestTimeout): + t.Fatal("mutation was not admitted") + } + + // CloseEngineOnly owns the teardown and parks draining the admitted write. + // Close arrives second and parks waiting on that attempt. + engineOnlyErr := make(chan error, 1) + go func() { engineOnlyErr <- store.CloseEngineOnly() }() + waitForAdmissionState(t, store, pebbleStoreClosing) + closeErr := make(chan error, 1) + go func() { closeErr <- store.Close(ctx) }() + select { + case err := <-closeErr: + t.Fatalf("Close returned before the owning teardown finished: %v", err) + case <-time.After(25 * time.Millisecond): + } + + close(release) + require.NoError(t, receiveError(t, mutationErr)) + // The store is dirty, so CloseEngineOnly refuses to discard it. + require.Error(t, receiveError(t, engineOnlyErr)) + // That refusal says nothing about whether Close should save, so Close runs + // its own attempt. Adopting the other caller's error instead left the sync + // stranded in the temp dir with no envelope on disk. + require.NoError(t, receiveError(t, closeErr)) + + reopened, err := NewStore(ctx, path, WithReadOnly(true)) + require.NoError(t, err) + defer reopened.Close(ctx) + engine, ok := pebble.AsEngine(reopened) + require.True(t, ok) + rec, err := engine.GetResourceTypeRecord(ctx, "group") + require.NoError(t, err) + require.Equal(t, "Group", rec.GetDisplayName()) +} + +func TestPebbleStoreFoldAccountingIgnoresFailedFolds(t *testing.T) { + ctx := context.Background() + store := newAdmissionTestStore(t, filepath.Join(t.TempDir(), "fold-accounting.c1z")) + store.closeMu.Lock() + baseline := store.foldDeadBytes + store.closeMu.Unlock() + + injected := errors.New("fold failed") + err := pebble.WithEngineFoldMutation(ctx, store, func(ctx context.Context, _ *pebble.Engine) (int64, error) { + return 4096, injected + }) + require.ErrorIs(t, err, injected) + + store.closeMu.Lock() + require.False(t, store.dirty, "a failed fold marked the store dirty, so Close would spend a full envelope save on an output the compactor is about to discard") + require.Equal(t, baseline, store.foldDeadBytes, "a failed fold recorded bytes it never shadowed, pulling the rebuild cutover forward") + store.closeMu.Unlock() + + require.NoError(t, pebble.WithEngineFoldMutation(ctx, store, func(ctx context.Context, _ *pebble.Engine) (int64, error) { + return 4096, nil + })) + store.closeMu.Lock() + require.True(t, store.dirty, "a fold that landed must mark the store dirty so Close writes the envelope") + require.Equal(t, baseline+4096, store.foldDeadBytes) + store.closeMu.Unlock() + + require.NoError(t, store.Close(ctx)) +} + +// TestPebbleStoreCloseWarnsWhileBlockedOnDrain pins the observability of the +// uncancellable drain. compactPebble and compactPebbleFold each hold a single +// admission for an entire merge, so a wedged compaction or a frozen Lambda +// leaves Close blocked with no ctx.Err() to report. Without a log line the only +// way to tell a hung Close from a slow one is a stack dump. +func TestPebbleStoreCloseWarnsWhileBlockedOnDrain(t *testing.T) { + logs := &lockedBuffer{} + encCfg := zap.NewProductionEncoderConfig() + ctx := ctxzap.ToContext(context.Background(), zap.New(zapcore.NewCore( + zapcore.NewJSONEncoder(encCfg), zapcore.AddSync(logs), zap.WarnLevel, + ))) + + store := newAdmissionTestStore(t, filepath.Join(t.TempDir(), "drain-warn.c1z")) + store.drainWarnInterval = time.Millisecond + _, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + admitted := make(chan struct{}) + release := make(chan struct{}) + store.mutationAdmissionHook = func() { + close(admitted) + <-release + } + mutationErr := make(chan error, 1) + go func() { + mutationErr <- store.PutResourceTypes(ctx, v2.ResourceType_builder{Id: "blocking"}.Build()) + }() + select { + case <-admitted: + case <-time.After(admissionTestTimeout): + t.Fatal("mutation was not admitted") + } + + closeErr := make(chan error, 1) + go func() { closeErr <- store.Close(ctx) }() + waitForAdmissionState(t, store, pebbleStoreClosing) + + deadline := time.Now().Add(admissionTestTimeout) + var logged string + for time.Now().Before(deadline) { + if logged = logs.String(); strings.Contains(logged, "still waiting to tear down") { + break + } + time.Sleep(time.Millisecond) + } + require.Contains(t, logged, "still waiting to tear down", "a blocked teardown must say so") + require.Contains(t, logged, `"active_writes":1`, "the warning must name the write it is waiting on") + require.Contains(t, logged, "in-flight mutations") + + close(release) + require.NoError(t, receiveError(t, mutationErr)) + require.NoError(t, receiveError(t, closeErr)) + + // The watchdog must stop with the teardown rather than outliving it. + before := logs.Len() + time.Sleep(20 * time.Millisecond) + require.Equal(t, before, logs.Len(), "the watchdog kept logging after Close returned") +} + +// lockedBuffer is a log sink the watchdog goroutine writes while the test +// goroutine reads. +type lockedBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *lockedBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *lockedBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +func (b *lockedBuffer) Len() int { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Len() +} + +func TestPebbleStoreWrapperFamiliesRejectAfterClosing(t *testing.T) { + ctx := context.Background() + store := newAdmissionTestStore(t, filepath.Join(t.TempDir(), "families.c1z")) + syncID, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + + admitted := make(chan struct{}) + release := make(chan struct{}) + store.mutationAdmissionHook = func() { + close(admitted) + <-release + } + mutationErr := make(chan error, 1) + go func() { + mutationErr <- store.PutResourceTypes(ctx, v2.ResourceType_builder{Id: "blocking"}.Build()) + }() + select { + case <-admitted: + case <-time.After(admissionTestTimeout): + t.Fatal("mutation was not admitted") + } + + closeErr := make(chan error, 1) + go func() { closeErr <- store.Close(ctx) }() + waitForAdmissionState(t, store, pebbleStoreClosing) + + require.ErrorIs(t, store.SyncMeta().RecalculateStats(ctx, syncID), pebble.ErrEngineClosing, "sync metadata") + _, err = store.FileOps().GenerateSyncDiff(ctx, syncID, syncID) + require.ErrorIs(t, err, pebble.ErrEngineClosing, "file operations") + require.ErrorIs(t, store.Grants().StoreExpandedGrants(ctx), pebble.ErrEngineClosing, "grant storage") + require.ErrorIs(t, store.SessionStore().Set(ctx, "late", []byte("value"), sessions.WithSyncID(syncID)), pebble.ErrEngineClosing, "session storage") + require.ErrorIs(t, store.EnsureGrantIndexes(ctx), pebble.ErrEngineClosing, "grant-index repair") + layerStore := store.Grants().(pebbleStoreGrantLayerStorer) + _, err = layerStore.BeginExpandedGrantLayer(ctx) + require.ErrorIs(t, err, pebble.ErrEngineClosing, "expanded-grant session") + + close(release) + require.NoError(t, receiveError(t, mutationErr)) + require.NoError(t, receiveError(t, closeErr)) +} + +func TestPebbleStoreAbandonedSessionsDoNotBlockClose(t *testing.T) { + t.Run("bulk import", func(t *testing.T) { + ctx := context.Background() + store := newAdmissionTestStore(t, filepath.Join(t.TempDir(), "bulk.c1z")) + syncID, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + var bulk *pebble.BulkSyncImport + require.NoError(t, pebble.WithEngineMutation(ctx, store, func(ctx context.Context, engine *pebble.Engine) error { + var err error + bulk, err = engine.StartBulkSyncImport(ctx, syncID, t.TempDir()) + return err + })) + closeErr := make(chan error, 1) + go func() { closeErr <- store.Close(ctx) }() + require.NoError(t, receiveError(t, closeErr)) + bulk.Abort() + }) + + t.Run("expanded grant layer", func(t *testing.T) { + t.Setenv("BATON_PEBBLE_SYNTH_LAYER_SEGMENT_ROWS", "1") + ctx := context.Background() + store := newAdmissionTestStore(t, filepath.Join(t.TempDir(), "expanded.c1z")) + _, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + started, err := store.Grants().(interface { + BeginExpandedGrantLayer(context.Context) (bool, error) + }).BeginExpandedGrantLayer(ctx) + require.NoError(t, err) + require.True(t, started) + layerStore := store.Grants().(pebbleStoreGrantLayerStorer) + dest := v2.Entitlement_builder{ + Id: "member", + Resource: v2.Resource_builder{ + Id: v2.ResourceId_builder{ResourceType: "group", Resource: "g1"}.Build(), + }.Build(), + }.Build() + principal := v3.PrincipalRef_builder{ResourceTypeId: "user", ResourceId: "u1"}.Build() + require.NoError(t, layerStore.AddExpandedGrantLayerContributions( + ctx, + dest, + []*v3.PrincipalRef{principal}, + []batonGrant.Sources{{{EntitlementID: "source", IsDirect: true}}}, + )) + closeErr := make(chan error, 1) + go func() { closeErr <- store.Close(ctx) }() + require.NoError(t, receiveError(t, closeErr)) + }) +} + +func TestPebbleStoreMutationWrapperInventory(t *testing.T) { + fset := token.NewFileSet() + files := []string{"pebble_store.go", "pebble_store_session.go", "ingest_invariant_store.go"} + parsed := make(map[string]*ast.File, len(files)) + for _, name := range files { + file, err := parser.ParseFile(fset, name, nil, 0) + require.NoError(t, err) + parsed[name] = file + } + + tracked := make(map[string]bool, len(guardedMutationWrappers)) + for _, name := range guardedMutationWrappers { + tracked[name] = false + } + // Every declaration bearing a guarded name must call withMutation — not + // just one of them. Keying on the name alone would let a same-named, + // unguarded method on another receiver hide behind the guarded one. + for fileName, file := range parsed { + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + if _, isTracked := tracked[fn.Name.Name]; !isTracked { + continue + } + tracked[fn.Name.Name] = true + guarded := false + ast.Inspect(fn.Body, func(node ast.Node) bool { + call, ok := node.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if ok && sel.Sel.Name == "withMutation" { + guarded = true + } + return true + }) + require.Truef(t, guarded, "mutating wrapper %s (%s) must call withMutation", fn.Name.Name, fileName) + } + } + for name, seen := range tracked { + require.Truef(t, seen, "guarded wrapper %s not found in parsed files", name) + } + + require.NoError(t, filepath.WalkDir("..", func(path string, entry os.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if entry.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + file, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + return err + } + ast.Inspect(file, func(node ast.Node) bool { + switch n := node.(type) { + case *ast.Ident: + require.NotEqualf(t, "MarkStoreDirty", n.Name, "%s uses forbidden post-write dirty marking", path) + case *ast.SelectorExpr: + require.NotEqualf(t, "markDirty", n.Sel.Name, "%s uses forbidden post-write dirty marking", path) + } + return true + }) + return nil + })) +} + +var _ c1zstore.Store = (*pebbleStore)(nil) diff --git a/pkg/dotc1z/pebble_store_capabilities_test.go b/pkg/dotc1z/pebble_store_capabilities_test.go new file mode 100644 index 000000000..d14123a53 --- /dev/null +++ b/pkg/dotc1z/pebble_store_capabilities_test.go @@ -0,0 +1,71 @@ +package dotc1z + +import ( + "context" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + storage_v3 "github.com/conductorone/baton-sdk/pb/c1/storage/v3" + "github.com/conductorone/baton-sdk/pkg/connectorstore" + "github.com/conductorone/baton-sdk/pkg/dotc1z/c1zstore" +) + +// Optional capability interfaces that callers discover with a type assertion +// rather than a declared parameter type. +// +// These are the failure mode that makes un-embedding *pebble.Engine risky. +// A required interface loses a method and the build breaks; an optional one +// loses a method and the assertion just stops matching. The caller then takes +// the fallback path — a slower scan, a skipped optimization, an absent stat — +// with no compile error and, for the pure-performance ones, no failing test +// either. Dropping the embed silently cost the store V3GrantReaderProvider, +// LatestFinishedSyncIDFetcher and Stats before these assertions existed. +// +// Anything asserted against a store anywhere in the tree belongs here, so the +// compiler is what notices next time instead of a production slow path. +var ( + _ connectorstore.DBSizeProvider = (*pebbleStore)(nil) + _ connectorstore.EntitlementGrantDigestReader = (*pebbleStore)(nil) + _ connectorstore.LatestFinishedSyncIDFetcher = (*pebbleStore)(nil) + _ connectorstore.StreamingReader = (*pebbleStore)(nil) + _ connectorstore.V3GrantReaderProvider = (*pebbleStore)(nil) + _ IngestInvariantStore = (*pebbleStore)(nil) + + // Asserted on store.SyncMeta(), not on the store itself. + _ c1zstore.IngestInvariantVerificationWriter = pebbleStoreSyncMeta{} + + // Asserted inline at the call site, so there is no named type to + // reference; these mirror the assertion shapes verbatim. + + // Asserted in pkg/dotc1z/cross_engine_parity_test.go. + _ interface { + Stats(ctx context.Context, syncType connectorstore.SyncType, syncID string) (map[string]int64, error) + } = (*pebbleStore)(nil) + + // Asserted in pkg/dotc1z/pebble_store.go via sanitizeSyncRunMetadataReader. + _ interface { + ListSyncRuns(ctx context.Context, pageToken string, pageSize uint32) ([]*c1zstore.SyncRun, string, error) + } = (*pebbleStore)(nil) + + // pkg/sync/syncer.go: expander fast path. Losing this one is invisible + // at runtime — the expander silently falls back to materializing full + // grants instead of principal keys. + _ interface { + ListGrantPrincipalKeysForEntitlement(context.Context, *v2.Entitlement, string, uint32) ([]string, string, error) + } = (*pebbleStore)(nil) + + // pkg/sync/syncer.go: gates the expander's topological-merge path. + // Losing it silently reroutes every Pebble sync onto the source-batched + // expander — no error, no failing test, just a whale-scale cost + // regression. This assertion was missing when the un-embedding landed + // and the regression shipped on the branch until review caught it. + _ interface { + GrantsForEntitlementPrincipalSorted() bool + } = (*pebbleStore)(nil) + + // pkg/synccompactor/compactor_pebble.go: the fold compactor's base-sync + // read on the shared destination store. Losing it fails the fold loudly + // ("not a pebble engine"), but the assertion belongs here all the same. + _ interface { + LatestFinishedSyncRecord(ctx context.Context, typeOK func(storage_v3.SyncType) bool) (*storage_v3.SyncRunRecord, error) + } = (*pebbleStore)(nil) +) diff --git a/pkg/dotc1z/pebble_store_cleanup_test.go b/pkg/dotc1z/pebble_store_cleanup_test.go index e326cf31f..8ae07caea 100644 --- a/pkg/dotc1z/pebble_store_cleanup_test.go +++ b/pkg/dotc1z/pebble_store_cleanup_test.go @@ -100,14 +100,14 @@ func TestPebbleSecondSyncWipesPriorData(t *testing.T) { // Old sync's grants must be gone from the primary keyspace. for _, ext := range []string{"old-g1", "old-g2"} { - _, err := rs.GetGrantRecord(ctx, ext) + _, err := rs.Engine.GetGrantRecord(ctx, ext) require.Error(t, err, "grant %s from the replaced sync still present", ext) } // Old sync's by-principal index entries must be gone too — a missing // wipe range would leak index keys the primary delete caught. count := 0 - err := rs.IterateGrantsByPrincipal(ctx, "user", "old-alice", func(*v3.GrantRecord) bool { + err := rs.Engine.IterateGrantsByPrincipal(ctx, "user", "old-alice", func(*v3.GrantRecord) bool { count++ return true }) @@ -115,7 +115,7 @@ func TestPebbleSecondSyncWipesPriorData(t *testing.T) { require.Zero(t, count, "by-principal index still has %d entries from the replaced sync", count) // New sync's grants must remain readable. - _, err = rs.GetGrantRecord(ctx, mkV2GrantID("ent", "user", "new-alice")) + _, err = rs.Engine.GetGrantRecord(ctx, mkV2GrantID("ent", "user", "new-alice")) require.NoError(t, err, "GetGrantRecord on current sync: %v", err) } diff --git a/pkg/dotc1z/pebble_store_promotion_test.go b/pkg/dotc1z/pebble_store_promotion_test.go new file mode 100644 index 000000000..b6893fe31 --- /dev/null +++ b/pkg/dotc1z/pebble_store_promotion_test.go @@ -0,0 +1,49 @@ +package dotc1z + +import ( + "reflect" + "testing" + + "github.com/stretchr/testify/require" +) + +// guardedMutationWrappers is the set of pebbleStore methods that mutate. +// TestPebbleStoreMutationWrapperInventory proves, via the AST, that every +// declaration bearing one of these names calls withMutation, so a wrapper +// that stops taking admission fails there rather than at runtime. +// +// What this list is not: proof that the store exposes no other writes. It +// covers the wrappers we know about. The store's method set being closed — +// the engine is a named field, not embedded, so nothing is promoted — is what +// keeps the unknown set small enough to review; see +// TestPebbleStoreEngineIsNotEmbedded. +var guardedMutationWrappers = []string{ + "GenerateSyncDiff", "MarkSyncSupportsDiff", "MarkIngestInvariantsVerified", + "ClearIngestInvariantVerification", "RecalculateStats", "NormalizeForFixtureSave", + "StartNewSync", "StartNewSyncWithID", "ResumeSync", "StartOrResumeSync", + "SetCurrentSync", "CheckpointSync", "EndSync", "PutAsset", + "SetSupportsDiff", "SetSyncLink", "PutGrants", "UnsafePutUniqueGrants", + "PutResourceTypes", "PutResources", "PutEntitlements", "DeleteGrant", + "DeleteGrantByRefs", "DeleteResourceRecord", "DeleteEntitlementByRefs", + "StoreExpandedGrants", "StoreNewExpandedGrants", "StoreNewExpandedGrantContributions", + "BeginExpandedGrantLayer", "AddExpandedGrantLayerContributions", + "FinishExpandedGrantLayer", "AbortExpandedGrantLayer", + "Set", "SetMany", "Delete", "Clear", "EnsureGrantIndexes", +} + +// TestPebbleStoreEngineIsNotEmbedded pins what actually keeps engine writes +// off the store: the engine is reached through a named field, so the store's +// method set is exactly what this package declares. Re-embedding would +// promote all of it at once — every engine mutator callable as s.PutX(...) +// with no admission and no dirty bit — and a promoted method is +// indistinguishable from a declared one by reflection, so assert on the field +// itself rather than trying to recognize the damage afterwards. +func TestPebbleStoreEngineIsNotEmbedded(t *testing.T) { + storeType := reflect.TypeOf(pebbleStore{}) + field, ok := storeType.FieldByName("Engine") + require.True(t, ok, "pebbleStore should keep its engine in a field named Engine") + require.False(t, field.Anonymous, + "pebbleStore must not embed *pebble.Engine: embedding promotes every engine mutator onto the "+ + "store, bypassing withMutation. Keep it a named field and forward reads explicitly in "+ + "pebble_store_reads.go.") +} diff --git a/pkg/dotc1z/pebble_store_reads.go b/pkg/dotc1z/pebble_store_reads.go new file mode 100644 index 000000000..27b350228 --- /dev/null +++ b/pkg/dotc1z/pebble_store_reads.go @@ -0,0 +1,269 @@ +package dotc1z + +import ( + "context" + "io" + "iter" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + reader_v2 "github.com/conductorone/baton-sdk/pb/c1/reader/v2" + v3 "github.com/conductorone/baton-sdk/pb/c1/storage/v3" + "github.com/conductorone/baton-sdk/pkg/connectorstore" + "github.com/conductorone/baton-sdk/pkg/dotc1z/c1zstore" +) + +// The read half of the store's interface surface, forwarded to the engine +// one method at a time. +// +// pebbleStore used to embed *pebble.Engine, which satisfied these for free — +// and, in the same stroke, promoted every engine mutator onto the store. +// Those promoted mutators bypassed withMutation entirely: no admission, no +// dirty bit, invisible to the wrapper-inventory test, and indistinguishable +// at the call site from a guarded method. Spelling the reads out costs this +// file and buys the guarantee that the store's method set contains exactly +// what is written here plus the wrappers in pebble_store.go. A mutator added +// to the engine tomorrow cannot appear on the store by accident. +// +// Keep this file free of logic. Anything that needs to touch admission, +// dirty tracking, or the envelope lifecycle belongs in pebble_store.go. + +func (s *pebbleStore) CurrentSyncStep(ctx context.Context) (string, error) { + return s.Engine.CurrentSyncStep(ctx) +} + +func (s *pebbleStore) GetAsset(ctx context.Context, req *v2.AssetServiceGetAssetRequest) (string, io.Reader, error) { + return s.Engine.GetAsset(ctx, req) +} + +func (s *pebbleStore) GetEntitlement( + ctx context.Context, + req *reader_v2.EntitlementsReaderServiceGetEntitlementRequest, +) (*reader_v2.EntitlementsReaderServiceGetEntitlementResponse, error) { + return s.Engine.GetEntitlement(ctx, req) +} + +func (s *pebbleStore) GetGrant( + ctx context.Context, + req *reader_v2.GrantsReaderServiceGetGrantRequest, +) (*reader_v2.GrantsReaderServiceGetGrantResponse, error) { + return s.Engine.GetGrant(ctx, req) +} + +func (s *pebbleStore) GetLatestFinishedSync( + ctx context.Context, + req *reader_v2.SyncsReaderServiceGetLatestFinishedSyncRequest, +) (*reader_v2.SyncsReaderServiceGetLatestFinishedSyncResponse, error) { + return s.Engine.GetLatestFinishedSync(ctx, req) +} + +func (s *pebbleStore) GetResource( + ctx context.Context, + req *reader_v2.ResourcesReaderServiceGetResourceRequest, +) (*reader_v2.ResourcesReaderServiceGetResourceResponse, error) { + return s.Engine.GetResource(ctx, req) +} + +func (s *pebbleStore) GetResourceType( + ctx context.Context, + req *reader_v2.ResourceTypesReaderServiceGetResourceTypeRequest, +) (*reader_v2.ResourceTypesReaderServiceGetResourceTypeResponse, error) { + return s.Engine.GetResourceType(ctx, req) +} + +func (s *pebbleStore) GetSync( + ctx context.Context, + req *reader_v2.SyncsReaderServiceGetSyncRequest, +) (*reader_v2.SyncsReaderServiceGetSyncResponse, error) { + return s.Engine.GetSync(ctx, req) +} + +func (s *pebbleStore) ListEntitlements( + ctx context.Context, + req *v2.EntitlementsServiceListEntitlementsRequest, +) (*v2.EntitlementsServiceListEntitlementsResponse, error) { + return s.Engine.ListEntitlements(ctx, req) +} + +//nolint:revive // method name mirrors the protobuf-generated gRPC server interface +func (s *pebbleStore) ListEntitlementsByIds( + ctx context.Context, + req *reader_v2.EntitlementsReaderServiceListEntitlementsByIdsRequest, +) (*reader_v2.EntitlementsReaderServiceListEntitlementsByIdsResponse, error) { + return s.Engine.ListEntitlementsByIds(ctx, req) +} + +func (s *pebbleStore) ListGrants( + ctx context.Context, + req *v2.GrantsServiceListGrantsRequest, +) (*v2.GrantsServiceListGrantsResponse, error) { + return s.Engine.ListGrants(ctx, req) +} + +func (s *pebbleStore) ListGrantsForEntitlement( + ctx context.Context, + req *reader_v2.GrantsReaderServiceListGrantsForEntitlementRequest, +) (*reader_v2.GrantsReaderServiceListGrantsForEntitlementResponse, error) { + return s.Engine.ListGrantsForEntitlement(ctx, req) +} + +func (s *pebbleStore) ListGrantsForEntitlements( + ctx context.Context, + req *reader_v2.GrantsReaderServiceListGrantsForEntitlementsRequest, +) (*reader_v2.GrantsReaderServiceListGrantsForEntitlementsResponse, error) { + return s.Engine.ListGrantsForEntitlements(ctx, req) +} + +func (s *pebbleStore) ListGrantsForPrincipal( + ctx context.Context, + req *reader_v2.GrantsReaderServiceListGrantsForPrincipalRequest, +) (*reader_v2.GrantsReaderServiceListGrantsForPrincipalResponse, error) { + return s.Engine.ListGrantsForPrincipal(ctx, req) +} + +func (s *pebbleStore) ListGrantsForResourceType( + ctx context.Context, + req *reader_v2.GrantsReaderServiceListGrantsForResourceTypeRequest, +) (*reader_v2.GrantsReaderServiceListGrantsForResourceTypeResponse, error) { + return s.Engine.ListGrantsForResourceType(ctx, req) +} + +func (s *pebbleStore) ListResources( + ctx context.Context, + req *v2.ResourcesServiceListResourcesRequest, +) (*v2.ResourcesServiceListResourcesResponse, error) { + return s.Engine.ListResources(ctx, req) +} + +//nolint:revive // method name mirrors the protobuf-generated gRPC server interface +func (s *pebbleStore) ListResourcesByIds( + ctx context.Context, + req *reader_v2.ResourcesReaderServiceListResourcesByIdsRequest, +) (*reader_v2.ResourcesReaderServiceListResourcesByIdsResponse, error) { + return s.Engine.ListResourcesByIds(ctx, req) +} + +func (s *pebbleStore) ListResourceTypes( + ctx context.Context, + req *v2.ResourceTypesServiceListResourceTypesRequest, +) (*v2.ResourceTypesServiceListResourceTypesResponse, error) { + return s.Engine.ListResourceTypes(ctx, req) +} + +func (s *pebbleStore) ListStaticEntitlements( + ctx context.Context, + req *v2.EntitlementsServiceListStaticEntitlementsRequest, +) (*v2.EntitlementsServiceListStaticEntitlementsResponse, error) { + return s.Engine.ListStaticEntitlements(ctx, req) +} + +func (s *pebbleStore) ListSyncs( + ctx context.Context, + req *reader_v2.SyncsReaderServiceListSyncsRequest, +) (*reader_v2.SyncsReaderServiceListSyncsResponse, error) { + return s.Engine.ListSyncs(ctx, req) +} + +func (s *pebbleStore) ListSyncRuns(ctx context.Context, pageToken string, pageSize uint32) ([]*c1zstore.SyncRun, string, error) { + return s.Engine.ListSyncRuns(ctx, pageToken, pageSize) +} + +// Optional capability reads. Callers reach these through a type assertion +// rather than a declared parameter type, so a missing forwarder here is not a +// build failure anywhere — it just silently stops the capability from being +// discovered. See pebble_store_capabilities_test.go, which asserts each one. + +func (s *pebbleStore) CurrentDBSizeBytes() (int64, error) { + return s.Engine.CurrentDBSizeBytes() +} + +func (s *pebbleStore) LatestFinishedSyncID(ctx context.Context, syncType connectorstore.SyncType) (string, error) { + return s.Engine.LatestFinishedSyncID(ctx, syncType) +} + +func (s *pebbleStore) Stats(ctx context.Context, syncType connectorstore.SyncType, syncID string) (map[string]int64, error) { + return s.Engine.Stats(ctx, syncType, syncID) +} + +func (s *pebbleStore) V3GrantReader() connectorstore.V3GrantReader { + return s.Engine.V3GrantReader() +} + +func (s *pebbleStore) ListGrantPrincipalKeysForEntitlement( + ctx context.Context, + entitlement *v2.Entitlement, + pageToken string, + pageSize uint32, +) ([]string, string, error) { + return s.Engine.ListGrantPrincipalKeysForEntitlement(ctx, entitlement, pageToken, pageSize) +} + +func (s *pebbleStore) StreamGrants( + ctx context.Context, + syncID string, + opts connectorstore.StreamGrantsOptions, +) iter.Seq2[*v2.Grant, error] { + return s.Engine.StreamGrants(ctx, syncID, opts) +} + +func (s *pebbleStore) StreamResources( + ctx context.Context, + syncID string, + opts connectorstore.StreamResourcesOptions, +) iter.Seq2[*v2.Resource, error] { + return s.Engine.StreamResources(ctx, syncID, opts) +} + +func (s *pebbleStore) StreamEntitlements(ctx context.Context, syncID string) iter.Seq2[*v2.Entitlement, error] { + return s.Engine.StreamEntitlements(ctx, syncID) +} + +// The grant-digest reader capability. The engine pins itself to +// connectorstore.EntitlementGrantDigestReader deliberately (adapter.go), and +// before the un-embedding the store inherited it. No in-repo caller asserts +// it on a store today, but this is a public SDK: an external consumer doing +// store.(connectorstore.EntitlementGrantDigestReader) would have silently +// lost constant-time digest reads and fallen back to full grant scans. + +func (s *pebbleStore) GetEntitlementGrantDigest( + ctx context.Context, + entitlement *v2.Entitlement, +) (connectorstore.GrantDigest, bool, error) { + return s.Engine.GetEntitlementGrantDigest(ctx, entitlement) +} + +func (s *pebbleStore) GetEntitlementGrantDigestNodes( + ctx context.Context, + entitlement *v2.Entitlement, + level int, +) ([]connectorstore.GrantDigestNode, bool, error) { + return s.Engine.GetEntitlementGrantDigestNodes(ctx, entitlement, level) +} + +func (s *pebbleStore) ScanEntitlementGrantBucket( + ctx context.Context, + entitlement *v2.Entitlement, + bucket connectorstore.GrantDigestBucket, + yield func(grant *v2.Grant) bool, +) error { + return s.Engine.ScanEntitlementGrantBucket(ctx, entitlement, bucket, yield) +} + +// GrantsForEntitlementPrincipalSorted reports that ListGrantsForEntitlement +// pages come back principal-sorted, which is what lets the expander use the +// topological-merge path. The syncer discovers it by inline type assertion +// (pkg/sync/syncer.go), so losing this forwarder does not fail anything — +// every Pebble sync just silently falls back to the source-batched expander. +// That exact regression shipped on this branch when the engine was +// un-embedded, and only an independent review caught it. +func (s *pebbleStore) GrantsForEntitlementPrincipalSorted() bool { + return s.Engine.GrantsForEntitlementPrincipalSorted() +} + +// LatestFinishedSyncRecord exists so the fold compactor can pick its base +// sync without extracting a raw engine from the destination store. Source +// files still go through pebble.AsEngine — the merge pipeline consumes +// concrete engines — but the shared destination should never have an +// unguarded handle pulled out of it for the sake of one read. +func (s *pebbleStore) LatestFinishedSyncRecord(ctx context.Context, typeOK func(v3.SyncType) bool) (*v3.SyncRunRecord, error) { + return s.Engine.LatestFinishedSyncRecord(ctx, typeOK) +} diff --git a/pkg/dotc1z/pebble_store_session.go b/pkg/dotc1z/pebble_store_session.go index c4fda9778..0ed014b7b 100644 --- a/pkg/dotc1z/pebble_store_session.go +++ b/pkg/dotc1z/pebble_store_session.go @@ -3,6 +3,7 @@ package dotc1z import ( "context" + "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble" "github.com/conductorone/baton-sdk/pkg/types/sessions" ) @@ -15,29 +16,40 @@ type pebbleStoreSessionStore struct { } func (s pebbleStoreSessionStore) Get(ctx context.Context, key string, opt ...sessions.SessionStoreOption) ([]byte, bool, error) { - return s.store.SessionGet(ctx, key, opt...) + engine := s.store.Engine + return engine.SessionGet(ctx, key, opt...) } func (s pebbleStoreSessionStore) Set(ctx context.Context, key string, value []byte, opt ...sessions.SessionStoreOption) error { - return s.store.markDirty(s.store.SessionSet(ctx, key, value, opt...)) + return s.store.withMutation(func(e *pebble.Engine) error { + return e.SessionSet(ctx, key, value, opt...) + }) } func (s pebbleStoreSessionStore) GetMany(ctx context.Context, keys []string, opt ...sessions.SessionStoreOption) (map[string][]byte, []string, error) { - return s.store.SessionGetMany(ctx, keys, opt...) + engine := s.store.Engine + return engine.SessionGetMany(ctx, keys, opt...) } func (s pebbleStoreSessionStore) GetAll(ctx context.Context, pageToken string, opt ...sessions.SessionStoreOption) (map[string][]byte, string, error) { - return s.store.SessionGetAll(ctx, pageToken, opt...) + engine := s.store.Engine + return engine.SessionGetAll(ctx, pageToken, opt...) } func (s pebbleStoreSessionStore) SetMany(ctx context.Context, values map[string][]byte, opt ...sessions.SessionStoreOption) error { - return s.store.markDirty(s.store.SessionSetMany(ctx, values, opt...)) + return s.store.withMutation(func(e *pebble.Engine) error { + return e.SessionSetMany(ctx, values, opt...) + }) } func (s pebbleStoreSessionStore) Delete(ctx context.Context, key string, opt ...sessions.SessionStoreOption) error { - return s.store.markDirty(s.store.SessionDelete(ctx, key, opt...)) + return s.store.withMutation(func(e *pebble.Engine) error { + return e.SessionDelete(ctx, key, opt...) + }) } func (s pebbleStoreSessionStore) Clear(ctx context.Context, opt ...sessions.SessionStoreOption) error { - return s.store.markDirty(s.store.SessionClear(ctx, opt...)) + return s.store.withMutation(func(e *pebble.Engine) error { + return e.SessionClear(ctx, opt...) + }) } diff --git a/pkg/dotc1z/race_test.go b/pkg/dotc1z/race_test.go index 6d1ff1e79..479d19c30 100644 --- a/pkg/dotc1z/race_test.go +++ b/pkg/dotc1z/race_test.go @@ -32,8 +32,12 @@ func TestWALCheckpointRace(t *testing.T) { ctx := t.Context() tmpDir := t.TempDir() - // Number of iterations - increase for more thorough testing - iterations := 100 + // Ordinary CI retains repeated attempts at the timing-sensitive race. + // The full suite keeps the original 100-attempt soak. + iterations := 20 + if os.Getenv("BATON_FULL_TESTS") != "" { + iterations = 100 + } for i := range iterations { t.Run(fmt.Sprintf("iteration_%d", i), func(t *testing.T) { diff --git a/pkg/dotc1z/to_pebble.go b/pkg/dotc1z/to_pebble.go index 91d5c1feb..a4819c5ad 100644 --- a/pkg/dotc1z/to_pebble.go +++ b/pkg/dotc1z/to_pebble.go @@ -330,11 +330,20 @@ func (c *C1File) ToPebble(ctx context.Context, outPath string, syncID string, op } stats.DestSyncID = destSyncID - destEng, ok := pebble.AsEngine(dest) - if !ok { - return nil, errors.New("to-pebble: destination store is not a pebble engine") - } - bi, err := destEng.StartBulkSyncImport(ctx, destSyncID, cfg.tmpDir) + // Only the import's start and finish take store admission; the convert + // calls below write SSTs through the bulk writer, not the engine, so they + // hold no admission and a concurrent Close is free to proceed mid-convert. + // That is intentional — an import can outlive a Close by design (see + // TestPebbleStoreAbandonedSessionsDoNotBlockClose) — and it is safe because + // nothing enters the engine keyspace until Finish, which either runs under + // admission or fails with ErrEngineClosing. The cost of losing that race is + // the conversion work already done, never a partially imported sync. + var bi *pebble.BulkSyncImport + err = pebble.WithEngineMutation(ctx, dest, func(ctx context.Context, destEng *pebble.Engine) error { + var startErr error + bi, startErr = destEng.StartBulkSyncImport(ctx, destSyncID, cfg.tmpDir) + return startErr + }) if err != nil { return nil, fmt.Errorf("to-pebble: start bulk import: %w", err) } @@ -357,7 +366,9 @@ func (c *C1File) ToPebble(ctx context.Context, outPath string, syncID string, op if err = c.convertGrants(ctx, bi, syncID, cfg, &stats.Grants); err != nil { return nil, fmt.Errorf("to-pebble: grants: %w", err) } - if err = bi.Finish(ctx); err != nil { + if err = pebble.WithEngineMutation(ctx, dest, func(ctx context.Context, _ *pebble.Engine) error { + return bi.Finish(ctx) + }); err != nil { return nil, fmt.Errorf("to-pebble: ingest: %w", err) } imported = true @@ -373,7 +384,12 @@ func (c *C1File) ToPebble(ctx context.Context, outPath string, syncID string, op // so they do not ship in the saved c1z (pkg/connectorbuilder), and nothing // can resume a sealed sync to read them back. if sync.EndedAt == nil { - if err = c.copySessions(ctx, destEng, syncID, destSyncID, &stats.Sessions); err != nil { + // Unlike the convert loops above, which write SSTs through the bulk + // writer, this one puts keys straight into the engine's session + // keyspace, so it runs under the store's admission for the copy. + if err = pebble.WithEngineMutation(ctx, dest, func(ctx context.Context, destEng *pebble.Engine) error { + return c.copySessions(ctx, destEng, syncID, destSyncID, &stats.Sessions) + }); err != nil { return nil, fmt.Errorf("to-pebble: sessions: %w", err) } } @@ -383,7 +399,12 @@ func (c *C1File) ToPebble(ctx context.Context, outPath string, syncID string, op // the freshly ingested keyspaces. statsRec := bi.ComputedStats() statsRec.SetAssets(stats.Assets.Rows) - destEng.StashComputedSyncStats(destSyncID, statsRec) + if err = pebble.WithEngineMutation(ctx, dest, func(_ context.Context, destEng *pebble.Engine) error { + destEng.StashComputedSyncStats(destSyncID, statsRec) + return nil + }); err != nil { + return nil, fmt.Errorf("to-pebble: stash destination stats: %w", err) + } // EndSync always runs: bulk import still needs deferred indexes / grant // digests / stats sidecar / durability flush. We then overlay source @@ -395,35 +416,40 @@ func (c *C1File) ToPebble(ctx context.Context, outPath string, syncID string, op if err = dest.EndSync(ctx); err != nil { return nil, fmt.Errorf("to-pebble: end destination sync: %w", err) } - rec, err := destEng.GetSyncRunRecord(ctx, destSyncID) - if err != nil { - return nil, fmt.Errorf("to-pebble: load destination sync metadata: %w", err) - } - rec.SetLinkedSyncId(sync.LinkedSyncID) - rec.SetSupportsDiff(sync.SupportsDiff) - // Localized on the way in: these scanned wall clocks become absolute - // instants in the Pebble record, and Pebble's resume cutoff compares - // started_at against time.Now() (see localizeSQLiteTimestamp). - if sync.StartedAt != nil { - rec.SetStartedAt(timestamppb.New(localizeSQLiteTimestamp(*sync.StartedAt, time.Local))) - } - if sync.EndedAt != nil { - rec.SetEndedAt(timestamppb.New(localizeSQLiteTimestamp(*sync.EndedAt, time.Local))) - // Verification provenance only rides along with a FINISHED source: - // a marker on an unfinished source (impossible through the writer - // API, but representable in a hand-edited file) must not convert - // into a sealed, verified destination. - if sync.IsVerified() { - rec.SetIngestInvariantGeneration(sync.Generation) - rec.SetIngestInvariantCoverage(append([]string(nil), sync.Coverage...)) - rec.SetIngestInvariantMode(string(sync.Mode)) + if err = pebble.WithEngineMutation(ctx, dest, func(ctx context.Context, destEng *pebble.Engine) error { + rec, err := destEng.GetSyncRunRecord(ctx, destSyncID) + if err != nil { + return fmt.Errorf("load destination sync metadata: %w", err) } - } else { - rec.ClearEndedAt() - rec.SetSyncToken(sync.SyncToken) - } - if err = destEng.PutSyncRunRecord(ctx, rec); err != nil { - return nil, fmt.Errorf("to-pebble: preserve source sync metadata: %w", err) + rec.SetLinkedSyncId(sync.LinkedSyncID) + rec.SetSupportsDiff(sync.SupportsDiff) + // Localized on the way in: these scanned wall clocks become absolute + // instants in the Pebble record, and Pebble's resume cutoff compares + // started_at against time.Now() (see localizeSQLiteTimestamp). + if sync.StartedAt != nil { + rec.SetStartedAt(timestamppb.New(localizeSQLiteTimestamp(*sync.StartedAt, time.Local))) + } + if sync.EndedAt != nil { + rec.SetEndedAt(timestamppb.New(localizeSQLiteTimestamp(*sync.EndedAt, time.Local))) + // Verification provenance only rides along with a FINISHED source: + // a marker on an unfinished source (impossible through the writer + // API, but representable in a hand-edited file) must not convert + // into a sealed, verified destination. + if sync.IsVerified() { + rec.SetIngestInvariantGeneration(sync.Generation) + rec.SetIngestInvariantCoverage(append([]string(nil), sync.Coverage...)) + rec.SetIngestInvariantMode(string(sync.Mode)) + } + } else { + rec.ClearEndedAt() + rec.SetSyncToken(sync.SyncToken) + } + if err := destEng.PutSyncRunRecord(ctx, rec); err != nil { + return fmt.Errorf("preserve source sync metadata: %w", err) + } + return nil + }); err != nil { + return nil, fmt.Errorf("to-pebble: %w", err) } endSyncDur := time.Since(endSyncStart) closeStart := time.Now() @@ -605,10 +631,14 @@ func (c *C1File) convertEmptyToPebble(ctx context.Context, outPath string, cfg * } }() - // A fresh store is not dirty until something writes. Force the envelope - // save so Close materializes an empty v3 c1z at outPath. - if !pebble.MarkStoreDirty(dest) { - return nil, errors.New("to-pebble: destination does not support dirty marking") + // A fresh store is not dirty until something writes. Take the mutation + // gate without writing anything: admission is what marks the store dirty, + // so Close still materializes an empty v3 c1z at outPath, and a store that + // is already closing says so rather than silently skipping the save. + if err = pebble.WithEngineMutation(ctx, dest, func(context.Context, *pebble.Engine) error { + return nil + }); err != nil { + return nil, fmt.Errorf("to-pebble: mark destination dirty: %w", err) } if err = dest.Close(ctx); err != nil { cleanupDest = false diff --git a/pkg/sync/checkpoint_cut_test.go b/pkg/sync/checkpoint_cut_test.go index d0192720c..a3facf218 100644 --- a/pkg/sync/checkpoint_cut_test.go +++ b/pkg/sync/checkpoint_cut_test.go @@ -509,17 +509,17 @@ func TestCheckpointCutEnumeration(t *testing.T) { cause error } var cuts []cut - for _, n := range enumerateCutPoints(baseline.checkpoints, 16) { + for _, n := range enumerateCutPoints(baseline.checkpoints, 8) { cuts = append(cuts, cut{name: fmt.Sprintf("checkpoint-%02d", n), checkpoint: n, cause: errInjectedCut}) } - for _, m := range enumerateCutPoints(baseline.responses, 16) { + for _, m := range enumerateCutPoints(baseline.responses, 8) { cuts = append(cuts, cut{name: fmt.Sprintf("response-%02d", m), response: m, cause: errInjectedCut}) } // Expiry cuts take the run-duration deadline path, which force-writes // a checkpoint of the MID-BATCH stack (spawned cursors in flight) // before exiting — the token shape hard cuts never persist, and the // one resume bugs have historically hidden in. - for _, m := range enumerateCutPoints(baseline.responses, 12) { + for _, m := range enumerateCutPoints(baseline.responses, 6) { cuts = append(cuts, cut{name: fmt.Sprintf("expire-%02d", m), response: m, cause: errInjectedExpiry}) } diff --git a/pkg/sync/expand/full_suite_test.go b/pkg/sync/expand/full_suite_test.go new file mode 100644 index 000000000..bf514f6eb --- /dev/null +++ b/pkg/sync/expand/full_suite_test.go @@ -0,0 +1,7 @@ +package expand + +import "os" + +func fullTestSuite() bool { + return os.Getenv("BATON_FULL_TESTS") != "" +} diff --git a/pkg/sync/expand/topological_merge_differential_test.go b/pkg/sync/expand/topological_merge_differential_test.go index 28d3595d5..3e0fbce2d 100644 --- a/pkg/sync/expand/topological_merge_differential_test.go +++ b/pkg/sync/expand/topological_merge_differential_test.go @@ -479,9 +479,9 @@ func TestTopologicalMergeUntouchedBaseGrantNotRewritten(t *testing.T) { // BATON_EXPAND_FUZZ_SEEDS number of seeds to sweep (count) // BATON_EXPAND_FUZZ_SEED_OFFSET first seed (start), to resume/shard a sweep func fuzzSeedRange(shortCount, longCount int) (int64, int64) { - count := int64(longCount) - if testing.Short() { - count = int64(shortCount) + count := int64(shortCount) + if fullTestSuite() && !testing.Short() { + count = int64(longCount) } if v := os.Getenv("BATON_EXPAND_FUZZ_SEEDS"); v != "" { if n, err := strconv.ParseInt(v, 10, 64); err == nil && n > 0 { diff --git a/pkg/sync/expand/topological_merge_layer_interrupt_test.go b/pkg/sync/expand/topological_merge_layer_interrupt_test.go index 37dd70d31..2e720d012 100644 --- a/pkg/sync/expand/topological_merge_layer_interrupt_test.go +++ b/pkg/sync/expand/topological_merge_layer_interrupt_test.go @@ -96,10 +96,10 @@ func TestTopologicalMergeLayerSessionInterruptResume(t *testing.T) { // TestTopologicalMergePartialInterruptResume. engine := c1zstore.EnginePebble cases := append(parityCases(), cyclicCases()...) - if testing.Short() { - // Windows CI: one representative acyclic + one cyclic case per + if testing.Short() || !fullTestSuite() { + // Ordinary CI: one representative acyclic + one cyclic case per // (algo, scenario) still exercises every layer-session code path; - // the full matrix runs on long (linux) CI. + // the full matrix runs in make test-full. cases = []sqliteParityCase{parityCases()[0], cyclicCases()[0]} } for _, algo := range algos { diff --git a/pkg/sync/expand/topological_merge_resume_test.go b/pkg/sync/expand/topological_merge_resume_test.go index af6057e8f..0f51bafd9 100644 --- a/pkg/sync/expand/topological_merge_resume_test.go +++ b/pkg/sync/expand/topological_merge_resume_test.go @@ -36,9 +36,9 @@ func TestTopologicalMergeResumeIdempotent(t *testing.T) { } interruptCases := append(parityCases(), cyclicCases()...) - if testing.Short() { + if testing.Short() || !fullTestSuite() { // See TestTopologicalMergeLayerSessionInterruptResume: representative - // subset for slow (windows) CI; full matrix on long CI. + // subset for ordinary CI; full matrix in make test-full. interruptCases = []sqliteParityCase{parityCases()[0], cyclicCases()[0]} } for _, engine := range []c1zstore.Engine{c1zstore.EnginePebble, c1zstore.EngineSQLite} { @@ -187,9 +187,9 @@ func TestTopologicalMergePartialInterruptResume(t *testing.T) { } interruptCases := append(parityCases(), cyclicCases()...) - if testing.Short() { + if testing.Short() || !fullTestSuite() { // See TestTopologicalMergeLayerSessionInterruptResume: representative - // subset for slow (windows) CI; full matrix on long CI. + // subset for ordinary CI; full matrix in make test-full. interruptCases = []sqliteParityCase{parityCases()[0], cyclicCases()[0]} } for _, engine := range []c1zstore.Engine{c1zstore.EnginePebble, c1zstore.EngineSQLite} { diff --git a/pkg/sync/scheduler_soak_test.go b/pkg/sync/scheduler_soak_test.go index 3007e7c15..aeafc8dc9 100644 --- a/pkg/sync/scheduler_soak_test.go +++ b/pkg/sync/scheduler_soak_test.go @@ -155,8 +155,8 @@ func soakSeeds(t *testing.T) []int64 { } func TestSchedulerSoakRandomizedFanoutWithFailures(t *testing.T) { - if testing.Short() { - t.Skip("skipping scheduler soak test in short mode") + if os.Getenv("BATON_FULL_TESTS") == "" { + t.Skip("scheduler soak runs in make scheduler-soak and make test-full") } for _, seed := range soakSeeds(t) { t.Run(fmt.Sprintf("seed=%d", seed), func(t *testing.T) { diff --git a/pkg/synccompactor/compactor_fold_test.go b/pkg/synccompactor/compactor_fold_test.go index 79d331ca0..b54d6b937 100644 --- a/pkg/synccompactor/compactor_fold_test.go +++ b/pkg/synccompactor/compactor_fold_test.go @@ -238,7 +238,9 @@ func TestFoldWasteCarryForwardAndAutoCutover(t *testing.T) { // from the source manifest at open and writes it back at save). w, err := dotc1z.NewStore(ctx, out.FilePath, dotc1z.WithTmpDir(t.TempDir())) require.NoError(t, err) - require.True(t, enginepkg.MarkStoreDirty(w)) + require.NoError(t, enginepkg.WithEngineMutation(ctx, w, func(context.Context, *enginepkg.Engine) error { + return nil + })) require.NoError(t, w.Close(ctx)) require.Equal(t, dead, readFoldDeadBytes(t, out.FilePath), "a non-fold save must carry the inherited fold_dead_bytes forward unchanged") @@ -257,8 +259,9 @@ func TestFoldWasteCarryForwardAndAutoCutover(t *testing.T) { // persisted by the same dirty-save path. w, err = dotc1z.NewStore(ctx, out.FilePath, dotc1z.WithTmpDir(t.TempDir())) require.NoError(t, err) - require.True(t, enginepkg.AddFoldDeadBytes(w, 1<<30)) - require.True(t, enginepkg.MarkStoreDirty(w)) + require.NoError(t, enginepkg.WithEngineFoldMutation(ctx, w, func(context.Context, *enginepkg.Engine) (int64, error) { + return 1 << 30, nil + })) require.NoError(t, w.Close(ctx)) require.Equal(t, dead+1<<30, readFoldDeadBytes(t, out.FilePath)) diff --git a/pkg/synccompactor/compactor_pebble.go b/pkg/synccompactor/compactor_pebble.go index c0ee84083..a8cec3680 100644 --- a/pkg/synccompactor/compactor_pebble.go +++ b/pkg/synccompactor/compactor_pebble.go @@ -462,11 +462,16 @@ func selectSourceSyncFromManifest(path string) (manifestSourceSelection, bool) { func (c *Compactor) compactPebbleFold(ctx context.Context) (string, error) { l := ctxzap.Extract(ctx) foldStart := time.Now() - destEng, ok := enginepkg.AsEngine(c.compactedC1z) + // Read the base sync through the store's own surface rather than pulling + // a raw engine out of the shared destination; AsEngine on the dest is + // reserved for nothing — only single-owner source files use it. + destReader, ok := c.compactedC1z.(interface { + LatestFinishedSyncRecord(ctx context.Context, typeOK func(v3.SyncType) bool) (*v3.SyncRunRecord, error) + }) if !ok { - return "", errors.New("compactPebbleFold: compacted store is not a pebble engine") + return "", errors.New("compactPebbleFold: compacted store does not expose LatestFinishedSyncRecord") } - baseRec, err := destEng.LatestFinishedSyncRecord(ctx, compactableV3SyncType) + baseRec, err := destReader.LatestFinishedSyncRecord(ctx, compactableV3SyncType) if err != nil { return "", fmt.Errorf("compactPebbleFold: select base sync: %w", err) } @@ -480,264 +485,257 @@ func (c *Compactor) compactPebbleFold(ctx context.Context) (string, error) { ) unionType := baseRec.GetType() maxEnded := baseRec.GetEndedAt().AsTime() - - // Apply partials newest-first (reverse entry order, excluding the - // base at entries[0]); strictly-newer-wins makes earlier - // applications take precedence on ties. var foldStats mergepkg.FoldStats - var partialSyncIDs []string - var partialTokens []string - // SQLite/v1 partials are converted to Pebble in the tmp dir before being - // folded in; their converted copies are removed when this run completes. - var convertedInputs []string - defer func() { - for _, path := range convertedInputs { - _ = os.Remove(path) - } - }() - for i := len(c.entries) - 1; i >= 1; i-- { - if err := ctx.Err(); err != nil { - return "", err - } - cs := c.entries[i] - sourcePath := cs.FilePath - format, err := readCompactionInputFormat(sourcePath) - if err != nil { - return "", err - } - if format == dotc1z.C1ZFormatV1 { - convertedPath, err := c.convertSQLiteInputToPebble(ctx, cs) - if err != nil { - return "", err - } - convertedInputs = append(convertedInputs, convertedPath) - sourcePath = convertedPath - } - srcSyncID := "" - if sel, ok := selectSourceSyncFromManifest(sourcePath); ok { - srcSyncID = sel.syncID - unionType = unionV3SyncType(unionType, sel.syncType) - if sel.endedAt.After(maxEnded) { - maxEnded = sel.endedAt - } - } - w, err := dotc1z.NewStore(ctx, sourcePath, dotc1z.WithReadOnly(true), dotc1z.WithTmpDir(c.tmpDir), dotc1z.WithDecoderPool(c.decoderPool)) - if err != nil { - return "", fmt.Errorf("compactPebbleFold: open input %s: %w", sourcePath, err) - } - srcEng, ok := enginepkg.AsEngine(w) - if !ok { - _ = w.Close(ctx) - return "", fmt.Errorf("compactPebbleFold: input %s is not a pebble c1z", sourcePath) - } - if srcSyncID == "" { - rec, err := srcEng.LatestFinishedSyncRecord(ctx, compactableV3SyncType) - if err != nil { - _ = w.Close(ctx) - return "", fmt.Errorf("compactPebbleFold: input %s: select compactable sync: %w", sourcePath, err) - } - if rec == nil { - _ = w.Close(ctx) - return "", fmt.Errorf("compactPebbleFold: input %s has no finished compactable sync", sourcePath) - } - srcSyncID = rec.GetSyncId() - unionType = unionV3SyncType(unionType, rec.GetType()) - if ts := rec.GetEndedAt(); ts != nil && ts.AsTime().After(maxEnded) { - maxEnded = ts.AsTime() + var newSyncID string + err = enginepkg.WithEngineFoldMutation(ctx, c.compactedC1z, func(ctx context.Context, destEng *enginepkg.Engine) (int64, error) { + id, mutationErr := func() (string, error) { + // Apply partials newest-first (reverse entry order, excluding the + // base at entries[0]); strictly-newer-wins makes earlier + // applications take precedence on ties. + var partialSyncIDs []string + var partialTokens []string + // SQLite/v1 partials are converted to Pebble in the tmp dir before being + // folded in; their converted copies are removed when this run completes. + var convertedInputs []string + defer func() { + for _, path := range convertedInputs { + _ = os.Remove(path) + } + }() + for i := len(c.entries) - 1; i >= 1; i-- { + if err := ctx.Err(); err != nil { + return "", err + } + cs := c.entries[i] + sourcePath := cs.FilePath + format, err := readCompactionInputFormat(sourcePath) + if err != nil { + return "", err + } + if format == dotc1z.C1ZFormatV1 { + convertedPath, err := c.convertSQLiteInputToPebble(ctx, cs) + if err != nil { + return "", err + } + convertedInputs = append(convertedInputs, convertedPath) + sourcePath = convertedPath + } + srcSyncID := "" + if sel, ok := selectSourceSyncFromManifest(sourcePath); ok { + srcSyncID = sel.syncID + unionType = unionV3SyncType(unionType, sel.syncType) + if sel.endedAt.After(maxEnded) { + maxEnded = sel.endedAt + } + } + w, err := dotc1z.NewStore(ctx, sourcePath, dotc1z.WithReadOnly(true), dotc1z.WithTmpDir(c.tmpDir), dotc1z.WithDecoderPool(c.decoderPool)) + if err != nil { + return "", fmt.Errorf("compactPebbleFold: open input %s: %w", sourcePath, err) + } + srcEng, ok := enginepkg.AsEngine(w) + if !ok { + _ = w.Close(ctx) + return "", fmt.Errorf("compactPebbleFold: input %s is not a pebble c1z", sourcePath) + } + if srcSyncID == "" { + rec, err := srcEng.LatestFinishedSyncRecord(ctx, compactableV3SyncType) + if err != nil { + _ = w.Close(ctx) + return "", fmt.Errorf("compactPebbleFold: input %s: select compactable sync: %w", sourcePath, err) + } + if rec == nil { + _ = w.Close(ctx) + return "", fmt.Errorf("compactPebbleFold: input %s has no finished compactable sync", sourcePath) + } + srcSyncID = rec.GetSyncId() + unionType = unionV3SyncType(unionType, rec.GetType()) + if ts := rec.GetEndedAt(); ts != nil && ts.AsTime().After(maxEnded) { + maxEnded = ts.AsTime() + } + } + partialSyncIDs = append(partialSyncIDs, srcSyncID) + partialTokens = append(partialTokens, readSourceSyncToken(ctx, srcEng, srcSyncID)) + + mergeStats, mergeErr := mergepkg.MergeInto(ctx, destEng, []mergepkg.SourceSync{{Engine: srcEng, SyncID: srcSyncID}}, baseSyncID) + foldStats.Add(mergeStats) + if cerr := w.Close(ctx); cerr != nil { + l.Error("compactPebbleFold: error closing source store", zap.Error(cerr), zap.String("file", sourcePath)) + } + if mergeErr != nil { + return "", fmt.Errorf("compactPebbleFold: merge %s: %w", sourcePath, mergeErr) + } } - } - partialSyncIDs = append(partialSyncIDs, srcSyncID) - partialTokens = append(partialTokens, readSourceSyncToken(ctx, srcEng, srcSyncID)) - mergeStats, mergeErr := mergepkg.MergeInto(ctx, destEng, []mergepkg.SourceSync{{Engine: srcEng, SyncID: srcSyncID}}, baseSyncID) - foldStats.Add(mergeStats) - if cerr := w.Close(ctx); cerr != nil { - l.Error("compactPebbleFold: error closing source store", zap.Error(cerr), zap.String("file", sourcePath)) - } - if mergeErr != nil { - return "", fmt.Errorf("compactPebbleFold: merge %s: %w", sourcePath, mergeErr) - } - } + // Only touch grant digests for the entitlement partitions this fold + // actually wrote a grant into. The dest started as a byte copy of a + // sealed base, whose digest state is already exactly correct for + // every OTHER entitlement; invalidating (and later recalling + // RepairMissingGrantDigests to rebuild) the whole file on every + // fold — even one where a single entitlement out of thousands + // changed — would reintroduce the O(base) cost fold exists to + // avoid. InvalidateGrantDigestPartitions drops exactly the touched + // partitions (+ the now-stale whole-file root); + // RepairMissingGrantDigests then rebuilds exactly what's missing, + // each from a targeted scan of just that entitlement's own grants — + // never a full-file scan — and recomputes the whole-file root from + // the (small) digest keyspace itself. A fold whose partials add + // nothing new (a common steady-state case — re-running compaction + // with no new data, or partials that only touch + // resources/entitlements) touches nothing at all. + // + // This result is FINAL only when the caller (Compact) goes on to + // skip grant expansion (WithSkipGrantExpansion, or a partial-typed + // union) — nothing else touches c.compactedC1z before Close in that + // path (GetSync is a pure read; Cleanup is a hard no-op for the + // Pebble engine). When expansion runs instead, its own grant writes + // use dedicated write paths (PutExpandedGrantRecords / + // PutSynthesizedGrantRecords / the layer-session ingest, not + // PutGrantRecords) whose invalidation correctness doesn't matter + // here: expandGrants' syncer.Sync ALWAYS calls store.EndSync + // afterward — even for a no-op expansion — and Adapter.EndSync's + // finalize unconditionally runs a FULL digest rebuild + // (BuildDeferredGrantIndexes or BuildGrantDigests) whenever the + // digest index is enabled, with no branch that skips both. So + // whatever this targeted repair produces gets unconditionally + // superseded by a full, correct rebuild the moment expansion runs — + // safe, but this optimization's actual win is scoped to + // skip-expansion compactions; a fold whose base is a full sync + // (expansion NOT skipped) pays for both the targeted repair AND the + // subsequent full rebuild. See + // TestCompactPebbleFoldWithExpansionRebuildsFullyRegardless. + // + // When the dest engine has the digest index DISABLED, touched + // digests must be dropped instead of repaired: the byte copy + // carried the sealed base's digest state unconditionally, readers + // serve whatever is stored regardless of this writer's flag + // (grantDigestsPresent is probed from the keyspace at Open), and + // every rebuild path — this one, EndSync's finalize, + // RepairMissingGrantDigests itself — gates on the same flag, so + // stale digests would ship as present-but-wrong with nothing left + // to heal them. Absent is always safe (present-means-exact). + // Dropping only on a grant write, rather than skipping the digest + // bucket copy up front, keeps the no-grant-write fold preserving + // the base's still-exact digests for free even on a disabled-index + // engine. See TestCompactPebbleFoldDigestIndexDisabledDropsDigests. + if len(foldStats.TouchedGrantPartitions) > 0 { + if !destEng.GrantDigestIndexEnabled() { + if err := destEng.DropAllGrantDigestState(ctx); err != nil { + return "", fmt.Errorf("compactPebbleFold: drop grant digest state (digest index disabled): %w", err) + } + l.Info("compactPebbleFold: grant writes with digest index disabled; dropped the base's copied digest state", + zap.Int("touched_partitions", len(foldStats.TouchedGrantPartitions))) + } else { + partitions := make([]string, 0, len(foldStats.TouchedGrantPartitions)) + for p := range foldStats.TouchedGrantPartitions { + partitions = append(partitions, p) + } + if err := destEng.InvalidateGrantDigestPartitions(ctx, partitions); err != nil { + return "", fmt.Errorf("compactPebbleFold: invalidate grant digest partitions: %w", err) + } + if err := destEng.RepairMissingGrantDigests(ctx); err != nil { + return "", fmt.Errorf("compactPebbleFold: repair grant digests: %w", err) + } + l.Info("compactPebbleFold: repaired grant digests for touched entitlements", + zap.Int("touched_partitions", len(partitions))) + } + } else { + l.Info("compactPebbleFold: no grant writes; base grant digest state left untouched") + } - // Record the bytes this fold shadowed in the base keyspace. The - // store inherited the base manifest's running fold_dead_bytes at - // open (the dest is a byte copy of the base), so adding the delta - // keeps the manifest counter cumulative across consecutive folds; - // resolvePebbleMode's waste cutover reads it to force the eventual - // rebuild that reclaims the dead weight. - if foldStats.DeadBytes > 0 { - if !enginepkg.AddFoldDeadBytes(c.compactedC1z, foldStats.DeadBytes) { - return "", errors.New("compactPebbleFold: could not record fold dead bytes") - } - } + // Optionally compact the folded LSM before save. Off by default: + // a compaction rewrites the SSTs that overlap the partials' writes + // — for scattered overrides that is most of the base — which both + // costs O(base) on the critical path and makes those files no + // longer byte-identical to the source envelope's frames, so the + // splice-at-save degrades to a full re-encode. The payoff is an + // output with zero shadowed records (overrides otherwise leave + // dead bytes inside the spliced base frames). Experimental knob + // for measuring that trade-off; the long-term plan for reclaiming + // accumulated fold bloat is routing the occasional compaction to + // the rebuild path instead. + if os.Getenv("BATON_EXPERIMENTAL_FOLD_COMPACT") == "1" { + compactStart := time.Now() + if err := destEng.CompactAllRanges(ctx); err != nil { + return "", fmt.Errorf("compactPebbleFold: compact ranges: %w", err) + } + l.Info("compactPebbleFold: compacted LSM before save", + zap.Duration("elapsed", time.Since(compactStart))) + } - // Only touch grant digests for the entitlement partitions this fold - // actually wrote a grant into. The dest started as a byte copy of a - // sealed base, whose digest state is already exactly correct for - // every OTHER entitlement; invalidating (and later recalling - // RepairMissingGrantDigests to rebuild) the whole file on every - // fold — even one where a single entitlement out of thousands - // changed — would reintroduce the O(base) cost fold exists to - // avoid. InvalidateGrantDigestPartitions drops exactly the touched - // partitions (+ the now-stale whole-file root); - // RepairMissingGrantDigests then rebuilds exactly what's missing, - // each from a targeted scan of just that entitlement's own grants — - // never a full-file scan — and recomputes the whole-file root from - // the (small) digest keyspace itself. A fold whose partials add - // nothing new (a common steady-state case — re-running compaction - // with no new data, or partials that only touch - // resources/entitlements) touches nothing at all. - // - // This result is FINAL only when the caller (Compact) goes on to - // skip grant expansion (WithSkipGrantExpansion, or a partial-typed - // union) — nothing else touches c.compactedC1z before Close in that - // path (GetSync is a pure read; Cleanup is a hard no-op for the - // Pebble engine). When expansion runs instead, its own grant writes - // use dedicated write paths (PutExpandedGrantRecords / - // PutSynthesizedGrantRecords / the layer-session ingest, not - // PutGrantRecords) whose invalidation correctness doesn't matter - // here: expandGrants' syncer.Sync ALWAYS calls store.EndSync - // afterward — even for a no-op expansion — and Adapter.EndSync's - // finalize unconditionally runs a FULL digest rebuild - // (BuildDeferredGrantIndexes or BuildGrantDigests) whenever the - // digest index is enabled, with no branch that skips both. So - // whatever this targeted repair produces gets unconditionally - // superseded by a full, correct rebuild the moment expansion runs — - // safe, but this optimization's actual win is scoped to - // skip-expansion compactions; a fold whose base is a full sync - // (expansion NOT skipped) pays for both the targeted repair AND the - // subsequent full rebuild. See - // TestCompactPebbleFoldWithExpansionRebuildsFullyRegardless. - // - // When the dest engine has the digest index DISABLED, touched - // digests must be dropped instead of repaired: the byte copy - // carried the sealed base's digest state unconditionally, readers - // serve whatever is stored regardless of this writer's flag - // (grantDigestsPresent is probed from the keyspace at Open), and - // every rebuild path — this one, EndSync's finalize, - // RepairMissingGrantDigests itself — gates on the same flag, so - // stale digests would ship as present-but-wrong with nothing left - // to heal them. Absent is always safe (present-means-exact). - // Dropping only on a grant write, rather than skipping the digest - // bucket copy up front, keeps the no-grant-write fold preserving - // the base's still-exact digests for free even on a disabled-index - // engine. See TestCompactPebbleFoldDigestIndexDisabledDropsDigests. - if len(foldStats.TouchedGrantPartitions) > 0 { - if !destEng.GrantDigestIndexEnabled() { - if err := destEng.DropAllGrantDigestState(ctx); err != nil { - return "", fmt.Errorf("compactPebbleFold: drop grant digest state (digest index disabled): %w", err) + // Mint a fresh sync id for the folded output. Renaming a v3 c1z's + // sync is a metadata-only write now that keys carry no sync_id (the + // records merged into the base keyspace are untouched by the + // rename), so the historical objection to auto-fold — that adopting + // the base id left C1's LatestCompactedSyncId unchanged and looked + // like "no new compaction" — no longer applies. ParentSyncId is + // cleared: the base sync's record is overwritten by this rename, so + // a lineage link would dangle, and the rebuild path's compacted + // output carries no parent either. + newSyncID := ksuid.New().String() + baseRec.SetSyncId(newSyncID) + baseRec.SetParentSyncId("") + baseRec.SetType(unionType) + // The fold mutated the inherited base keyspace. Never publish the base + // artifact's pre-fold verification as proof of the merged output; a later + // expansion/invariant pass will write a fresh marker when one runs. + baseRec.SetIngestInvariantGeneration("") + baseRec.SetIngestInvariantCoverage(nil) + baseRec.SetIngestInvariantMode("") + if !maxEnded.IsZero() { + baseRec.SetEndedAt(timestamppb.New(maxEnded)) } - l.Info("compactPebbleFold: grant writes with digest index disabled; dropped the base's copied digest state", - zap.Int("touched_partitions", len(foldStats.TouchedGrantPartitions))) - } else { - partitions := make([]string, 0, len(foldStats.TouchedGrantPartitions)) - for p := range foldStats.TouchedGrantPartitions { - partitions = append(partitions, p) + // PutSyncRunRecord overwrites the single fixed sync-run key, so the + // file's one sync-run record now carries newSyncID. (The compactor + // GetSync's this id right after and asserts it matches — the + // engine's GetSyncRunRecord id-match guard enforces it.) + if err := destEng.PutSyncRunRecord(ctx, baseRec); err != nil { + return "", fmt.Errorf("compactPebbleFold: persist folded sync_run: %w", err) } - if err := destEng.InvalidateGrantDigestPartitions(ctx, partitions); err != nil { - return "", fmt.Errorf("compactPebbleFold: invalidate grant digest partitions: %w", err) + // The fold rewrote the keyspace, so the cached stats sidecar is + // stale. Recompute under the NEW id so the sidecar's SyncId — and + // the envelope manifest's sync-run projection built from it at save + // — match the renamed sync. Key-range counts, not full unmarshals. + if err := destEng.PersistSyncStats(ctx, newSyncID); err != nil { + return "", fmt.Errorf("compactPebbleFold: persist stats: %w", err) } - if err := destEng.RepairMissingGrantDigests(ctx); err != nil { - return "", fmt.Errorf("compactPebbleFold: repair grant digests: %w", err) + // Rewrite the token with compaction provenance: the base token's + // timing stats describe the base sync's collection run, so the + // section re-attributes them (stats_sync_id) and adds what this fold + // merged. Provenance is best-effort — it never fails the compaction. + outputStats, statsErr := enginepkg.ReadSyncStatsRecord(ctx, destEng, newSyncID) + if statsErr != nil { + l.Warn("compactPebbleFold: could not read output stats for provenance", zap.Error(statsErr)) } - l.Info("compactPebbleFold: repaired grant digests for touched entitlements", - zap.Int("touched_partitions", len(partitions))) - } - } else { - l.Info("compactPebbleFold: no grant writes; base grant digest state left untouched") - } - - // Optionally compact the folded LSM before save. Off by default: - // a compaction rewrites the SSTs that overlap the partials' writes - // — for scattered overrides that is most of the base — which both - // costs O(base) on the critical path and makes those files no - // longer byte-identical to the source envelope's frames, so the - // splice-at-save degrades to a full re-encode. The payoff is an - // output with zero shadowed records (overrides otherwise leave - // dead bytes inside the spliced base frames). Experimental knob - // for measuring that trade-off; the long-term plan for reclaiming - // accumulated fold bloat is routing the occasional compaction to - // the rebuild path instead. - if os.Getenv("BATON_EXPERIMENTAL_FOLD_COMPACT") == "1" { - compactStart := time.Now() - if err := destEng.CompactAllRanges(ctx); err != nil { - return "", fmt.Errorf("compactPebbleFold: compact ranges: %w", err) - } - l.Info("compactPebbleFold: compacted LSM before save", - zap.Duration("elapsed", time.Since(compactStart))) - } - - // Mint a fresh sync id for the folded output. Renaming a v3 c1z's - // sync is a metadata-only write now that keys carry no sync_id (the - // records merged into the base keyspace are untouched by the - // rename), so the historical objection to auto-fold — that adopting - // the base id left C1's LatestCompactedSyncId unchanged and looked - // like "no new compaction" — no longer applies. ParentSyncId is - // cleared: the base sync's record is overwritten by this rename, so - // a lineage link would dangle, and the rebuild path's compacted - // output carries no parent either. - newSyncID := ksuid.New().String() - baseRec.SetSyncId(newSyncID) - baseRec.SetParentSyncId("") - baseRec.SetType(unionType) - // The fold mutated the inherited base keyspace. Never publish the base - // artifact's pre-fold verification as proof of the merged output; a later - // expansion/invariant pass will write a fresh marker when one runs. - baseRec.SetIngestInvariantGeneration("") - baseRec.SetIngestInvariantCoverage(nil) - baseRec.SetIngestInvariantMode("") - if !maxEnded.IsZero() { - baseRec.SetEndedAt(timestamppb.New(maxEnded)) - } - // PutSyncRunRecord overwrites the single fixed sync-run key, so the - // file's one sync-run record now carries newSyncID. (The compactor - // GetSync's this id right after and asserts it matches — the - // engine's GetSyncRunRecord id-match guard enforces it.) - if err := destEng.PutSyncRunRecord(ctx, baseRec); err != nil { - return "", fmt.Errorf("compactPebbleFold: persist folded sync_run: %w", err) - } - // The fold rewrote the keyspace, so the cached stats sidecar is - // stale. Recompute under the NEW id so the sidecar's SyncId — and - // the envelope manifest's sync-run projection built from it at save - // — match the renamed sync. Key-range counts, not full unmarshals. - if err := destEng.PersistSyncStats(ctx, newSyncID); err != nil { - return "", fmt.Errorf("compactPebbleFold: persist stats: %w", err) - } - // Rewrite the token with compaction provenance: the base token's - // timing stats describe the base sync's collection run, so the - // section re-attributes them (stats_sync_id) and adds what this fold - // merged. Provenance is best-effort — it never fails the compaction. - outputStats, statsErr := enginepkg.ReadSyncStatsRecord(ctx, destEng, newSyncID) - if statsErr != nil { - l.Warn("compactPebbleFold: could not read output stats for provenance", zap.Error(statsErr)) - } - compactedToken, tokenErr := sdksync.BuildCompactedToken(baseRec.GetSyncToken(), sdksync.CompactionTokenInput{ - Mode: string(PebbleCompactorModeFold), - BaseSyncID: baseSyncID, - PartialSyncIDs: partialSyncIDs, - PartialTokens: partialTokens, - RecordCounts: compactionRecordCounts(outputStats, &foldStats), + compactedToken, tokenErr := sdksync.BuildCompactedToken(baseRec.GetSyncToken(), sdksync.CompactionTokenInput{ + Mode: string(PebbleCompactorModeFold), + BaseSyncID: baseSyncID, + PartialSyncIDs: partialSyncIDs, + PartialTokens: partialTokens, + RecordCounts: compactionRecordCounts(outputStats, &foldStats), + }) + if tokenErr != nil { + l.Warn("compactPebbleFold: could not build compaction provenance token", zap.Error(tokenErr)) + } else { + baseRec.SetSyncToken(compactedToken) + if err := destEng.PutSyncRunRecord(ctx, baseRec); err != nil { + return "", fmt.Errorf("compactPebbleFold: persist provenance token: %w", err) + } + } + l.Info("compactPebbleFold: done", + zap.String("base_sync_id", baseSyncID), + zap.String("folded_sync_id", newSyncID), + zap.Int64("overridden_records", foldStats.OverriddenRecords), + zap.Int64("dead_bytes", foldStats.DeadBytes), + zap.Duration("elapsed", time.Since(foldStart)), + ) + return newSyncID, nil + }() + newSyncID = id + return foldStats.DeadBytes, mutationErr }) - if tokenErr != nil { - l.Warn("compactPebbleFold: could not build compaction provenance token", zap.Error(tokenErr)) - } else { - baseRec.SetSyncToken(compactedToken) - if err := destEng.PutSyncRunRecord(ctx, baseRec); err != nil { - return "", fmt.Errorf("compactPebbleFold: persist provenance token: %w", err) - } - } - // All writes above went through the engine directly; flip the - // store's dirty bit so Close saves the envelope. - if !enginepkg.MarkStoreDirty(c.compactedC1z) { - return "", errors.New("compactPebbleFold: could not mark store dirty") + if err != nil { + return "", err } - l.Info("compactPebbleFold: done", - zap.String("base_sync_id", baseSyncID), - zap.String("folded_sync_id", newSyncID), - zap.Int64("overridden_records", foldStats.OverriddenRecords), - zap.Int64("dead_bytes", foldStats.DeadBytes), - zap.Duration("elapsed", time.Since(foldStart)), - ) return newSyncID, nil } @@ -1046,242 +1044,242 @@ func (c *Compactor) convertSQLiteInputToPebble(ctx context.Context, cs *Compacta func (c *Compactor) compactPebble(ctx context.Context, newSyncId string) error { l := ctxzap.Extract(ctx) - destEng, ok := enginepkg.AsEngine(c.compactedC1z) - if !ok { - return errors.New("compactPebble: compacted store is not a pebble engine") - } - - // runPebbleRebuild's StartNewSync→EndSync left the dest engine SEALED - // (writes refused, compactions paused). The merge below writes the whole - // compacted dataset — overlay mode through raw memtable batches — so - // bind the sync first: unseals the engine and resumes the compaction - // scheduler. Without this, L0 accumulates with no compactions granted - // until pebble stalls writes at L0StopWritesThreshold, permanently - // (nothing else resumes the scheduler mid-merge). - // - // Yes, "SetCurrentSync to restart compactions" is an odd spelling. It - // is deliberate: unseal/resume is not a public engine operation, because - // the sealed state exists precisely to guarantee "no record writes - // without a bound sync". Binding the sync we're about to write under is - // the one sanctioned way to declare that intent, and unseal+resume ride - // along as consequences (see Engine.SetCurrentSync / Engine.seal). An - // exported ResumeCompactions-style escape hatch would let callers write - // on a sealed engine again, recreating the very hang this fixes. - if err := destEng.SetCurrentSync(ctx, newSyncId); err != nil { - return fmt.Errorf("compactPebble: bind dest sync: %w", err) - } - - pebbleCompactorMode := c.pebbleMode - if pebbleCompactorMode == PebbleCompactorModeAuto { - pebbleCompactorMode = c.resolvePebbleMode(ctx) - } - useOverlay := pebbleCompactorMode == PebbleCompactorModeOverlay - sources := make([]mergepkg.SourceFile, 0, len(c.entries)) - unionType := v3.SyncType_SYNC_TYPE_PARTIAL - var maxEnded time.Time - rebuildBaseSyncID := "" - var rebuildPartialSyncIDs []string - - manifestSelected := 0 - // SQLite/v1 inputs are converted to Pebble in the tmp dir before being - // merged; their converted copies are removed when this run completes. - var convertedInputs []string - defer func() { - for _, path := range convertedInputs { - _ = os.Remove(path) - } - }() - for i := len(c.entries) - 1; i >= 0; i-- { - if err := ctx.Err(); err != nil { - return err - } - cs := c.entries[i] - sourcePath := cs.FilePath - format, err := readCompactionInputFormat(sourcePath) - if err != nil { - return err - } - if format == dotc1z.C1ZFormatV1 { - convertedPath, err := c.convertSQLiteInputToPebble(ctx, cs) - if err != nil { - return err - } - convertedInputs = append(convertedInputs, convertedPath) - sourcePath = convertedPath + return enginepkg.WithEngineMutation(ctx, c.compactedC1z, func(ctx context.Context, destEng *enginepkg.Engine) error { + // runPebbleRebuild's StartNewSync→EndSync left the dest engine SEALED + // (writes refused, compactions paused). The merge below writes the whole + // compacted dataset — overlay mode through raw memtable batches — so + // bind the sync first: unseals the engine and resumes the compaction + // scheduler. Without this, L0 accumulates with no compactions granted + // until pebble stalls writes at L0StopWritesThreshold, permanently + // (nothing else resumes the scheduler mid-merge). + // + // Yes, "SetCurrentSync to restart compactions" is an odd spelling. It + // is deliberate: unseal/resume is not a public engine operation, because + // the sealed state exists precisely to guarantee "no record writes + // without a bound sync". Binding the sync we're about to write under is + // the one sanctioned way to declare that intent, and unseal+resume ride + // along as consequences (see Engine.SetCurrentSync / Engine.seal). An + // exported ResumeCompactions-style escape hatch would let callers write + // on a sealed engine again, recreating the very hang this fixes. + if err := destEng.SetCurrentSync(ctx, newSyncId); err != nil { + return fmt.Errorf("compactPebble: bind dest sync: %w", err) } - // Fast path: read the latest finished compactable sync (and its - // cached stats) from the envelope manifest's sync-run projection - // — a header read, no payload unpack. Files written before the - // projection existed fall back to the unpack path below. - if sel, ok := selectSourceSyncFromManifest(sourcePath); ok { - manifestSelected++ - sources = append(sources, mergepkg.SourceFile{Path: sourcePath, SyncID: sel.syncID, Stats: sel.stats, DecoderPool: c.decoderPool}) - unionType = unionV3SyncType(unionType, sel.syncType) - if sel.endedAt.After(maxEnded) { - maxEnded = sel.endedAt - } - continue + pebbleCompactorMode := c.pebbleMode + if pebbleCompactorMode == PebbleCompactorModeAuto { + pebbleCompactorMode = c.resolvePebbleMode(ctx) } + useOverlay := pebbleCompactorMode == PebbleCompactorModeOverlay + sources := make([]mergepkg.SourceFile, 0, len(c.entries)) + unionType := v3.SyncType_SYNC_TYPE_PARTIAL + var maxEnded time.Time + rebuildBaseSyncID := "" + var rebuildPartialSyncIDs []string - source, syncType, endedAt, err := func() (mergepkg.SourceFile, v3.SyncType, time.Time, error) { - var zeroSource mergepkg.SourceFile - w, err := dotc1z.NewStore(ctx, sourcePath, dotc1z.WithReadOnly(true), dotc1z.WithTmpDir(c.tmpDir), dotc1z.WithDecoderPool(c.decoderPool)) - if err != nil { - return zeroSource, v3.SyncType_SYNC_TYPE_UNSPECIFIED, time.Time{}, fmt.Errorf("compactPebble: open input %s: %w", sourcePath, err) + manifestSelected := 0 + // SQLite/v1 inputs are converted to Pebble in the tmp dir before being + // merged; their converted copies are removed when this run completes. + var convertedInputs []string + defer func() { + for _, path := range convertedInputs { + _ = os.Remove(path) } - defer func() { - if cerr := w.Close(ctx); cerr != nil { - l.Error("compactPebble: error closing source store", zap.Error(cerr), zap.String("file", sourcePath)) - } - }() - - srcEng, ok := enginepkg.AsEngine(w) - if !ok { - return zeroSource, v3.SyncType_SYNC_TYPE_UNSPECIFIED, time.Time{}, fmt.Errorf("compactPebble: input %s is not a pebble c1z", sourcePath) + }() + for i := len(c.entries) - 1; i >= 0; i-- { + if err := ctx.Err(); err != nil { + return err } - rec, err := srcEng.LatestFinishedSyncRecord(ctx, compactableV3SyncType) + cs := c.entries[i] + sourcePath := cs.FilePath + format, err := readCompactionInputFormat(sourcePath) if err != nil { - return zeroSource, v3.SyncType_SYNC_TYPE_UNSPECIFIED, time.Time{}, fmt.Errorf("compactPebble: select source sync for %s: %w", sourcePath, err) + return err + } + if format == dotc1z.C1ZFormatV1 { + convertedPath, err := c.convertSQLiteInputToPebble(ctx, cs) + if err != nil { + return err + } + convertedInputs = append(convertedInputs, convertedPath) + sourcePath = convertedPath } - if rec == nil { - return zeroSource, v3.SyncType_SYNC_TYPE_UNSPECIFIED, time.Time{}, fmt.Errorf("compactPebble: input %s has no finished compactable sync (diff syncs are not compactable)", sourcePath) + + // Fast path: read the latest finished compactable sync (and its + // cached stats) from the envelope manifest's sync-run projection + // — a header read, no payload unpack. Files written before the + // projection existed fall back to the unpack path below. + if sel, ok := selectSourceSyncFromManifest(sourcePath); ok { + manifestSelected++ + sources = append(sources, mergepkg.SourceFile{Path: sourcePath, SyncID: sel.syncID, Stats: sel.stats, DecoderPool: c.decoderPool}) + unionType = unionV3SyncType(unionType, sel.syncType) + if sel.endedAt.After(maxEnded) { + maxEnded = sel.endedAt + } + continue } - // Record only (Path, SyncID, Stats) and fully close the store, - // removing its unpacked directory. The merge re-unpacks each - // source when its chunk is processed and removes it when the - // chunk closes, so peak disk is O(fan-in) source directories, - // not O(len(entries)). The fallback unpacks one source at a - // time and pays one extra unpack per source (selection + merge). - source := mergepkg.SourceFile{Path: sourcePath, SyncID: rec.GetSyncId(), DecoderPool: c.decoderPool} - if useOverlay { - stats, ok, err := enginepkg.CachedSyncStats(ctx, srcEng, rec.GetSyncId()) + source, syncType, endedAt, err := func() (mergepkg.SourceFile, v3.SyncType, time.Time, error) { + var zeroSource mergepkg.SourceFile + w, err := dotc1z.NewStore(ctx, sourcePath, dotc1z.WithReadOnly(true), dotc1z.WithTmpDir(c.tmpDir), dotc1z.WithDecoderPool(c.decoderPool)) if err != nil { - return zeroSource, v3.SyncType_SYNC_TYPE_UNSPECIFIED, time.Time{}, fmt.Errorf("compactPebble: cached stats for %s: %w", sourcePath, err) + return zeroSource, v3.SyncType_SYNC_TYPE_UNSPECIFIED, time.Time{}, fmt.Errorf("compactPebble: open input %s: %w", sourcePath, err) + } + defer func() { + if cerr := w.Close(ctx); cerr != nil { + l.Error("compactPebble: error closing source store", zap.Error(cerr), zap.String("file", sourcePath)) + } + }() + + srcEng, ok := enginepkg.AsEngine(w) + if !ok { + return zeroSource, v3.SyncType_SYNC_TYPE_UNSPECIFIED, time.Time{}, fmt.Errorf("compactPebble: input %s is not a pebble c1z", sourcePath) + } + rec, err := srcEng.LatestFinishedSyncRecord(ctx, compactableV3SyncType) + if err != nil { + return zeroSource, v3.SyncType_SYNC_TYPE_UNSPECIFIED, time.Time{}, fmt.Errorf("compactPebble: select source sync for %s: %w", sourcePath, err) + } + if rec == nil { + return zeroSource, v3.SyncType_SYNC_TYPE_UNSPECIFIED, time.Time{}, fmt.Errorf( + "compactPebble: input %s has no finished compactable sync (diff syncs are not compactable)", + sourcePath, + ) } - if ok { - source.Stats = stats + + // Record only (Path, SyncID, Stats) and fully close the store, + // removing its unpacked directory. The merge re-unpacks each + // source when its chunk is processed and removes it when the + // chunk closes, so peak disk is O(fan-in) source directories, + // not O(len(entries)). The fallback unpacks one source at a + // time and pays one extra unpack per source (selection + merge). + source := mergepkg.SourceFile{Path: sourcePath, SyncID: rec.GetSyncId(), DecoderPool: c.decoderPool} + if useOverlay { + stats, ok, err := enginepkg.CachedSyncStats(ctx, srcEng, rec.GetSyncId()) + if err != nil { + return zeroSource, v3.SyncType_SYNC_TYPE_UNSPECIFIED, time.Time{}, fmt.Errorf("compactPebble: cached stats for %s: %w", sourcePath, err) + } + if ok { + source.Stats = stats + } + } + var endedAt time.Time + if ts := rec.GetEndedAt(); ts != nil { + endedAt = ts.AsTime() } + return source, rec.GetType(), endedAt, nil + }() + if err != nil { + return err } - var endedAt time.Time - if ts := rec.GetEndedAt(); ts != nil { - endedAt = ts.AsTime() + + sources = append(sources, source) + unionType = unionV3SyncType(unionType, syncType) + if endedAt.After(maxEnded) { + maxEnded = endedAt } - return source, rec.GetType(), endedAt, nil - }() - if err != nil { - return err } + for i, source := range sources { + // The loop above appends in reverse entry order, so the base + // (entries[0]) is the last-appended source. + if i == len(sources)-1 { + rebuildBaseSyncID = source.SyncID + continue + } + rebuildPartialSyncIDs = append(rebuildPartialSyncIDs, source.SyncID) + } + // unpack_selected > 0 means inputs predate the manifest sync-run + // projection and pay a full unpack just to pick a sync — a fleet + // signal that those files should be regenerated by a current SDK. + l.Info("compactPebble: source selection", + zap.Int("sources", len(sources)), + zap.Int("manifest_selected", manifestSelected), + zap.Int("unpack_selected", len(sources)-manifestSelected), + zap.String("mode", string(pebbleCompactorMode)), + ) - sources = append(sources, source) - unionType = unionV3SyncType(unionType, syncType) - if endedAt.After(maxEnded) { - maxEnded = endedAt + var statsRec *v3.SyncStatsRecord + var err error + switch pebbleCompactorMode { + case PebbleCompactorModeOverlay: + // Oversized buckets are routed to the K-way run-file path inside + // the overlay merge itself (overlayPlanBuckets), so there is no + // whole-merge fallback here. + var overlayOpts []mergepkg.OverlayOption + if c.overlaySeenKeyLimit > 0 { + overlayOpts = append(overlayOpts, mergepkg.WithOverlaySeenKeyLimit(c.overlaySeenKeyLimit)) + } + if c.overlayRecordChunkSize > 0 { + overlayOpts = append(overlayOpts, mergepkg.WithOverlayRecordChunkSize(c.overlayRecordChunkSize)) + } + if c.overlayBufferFactor > 0 { + overlayOpts = append(overlayOpts, mergepkg.WithOverlayBufferFactor(c.overlayBufferFactor)) + } + if c.overlayGateFraction > 0 { + overlayOpts = append(overlayOpts, mergepkg.WithOverlayGateFraction(c.overlayGateFraction)) + } + statsRec, err = mergepkg.MergeFilesIntoOverlay(ctx, destEng, sources, newSyncId, c.tmpDir, overlayOpts...) + default: + statsRec, err = mergepkg.MergeFilesInto(ctx, destEng, sources, newSyncId, c.tmpDir) } - } - for i, source := range sources { - // The loop above appends in reverse entry order, so the base - // (entries[0]) is the last-appended source. - if i == len(sources)-1 { - rebuildBaseSyncID = source.SyncID - continue + if err != nil { + return fmt.Errorf("compactPebble: merge: %w", err) } - rebuildPartialSyncIDs = append(rebuildPartialSyncIDs, source.SyncID) - } - // unpack_selected > 0 means inputs predate the manifest sync-run - // projection and pay a full unpack just to pick a sync — a fleet - // signal that those files should be regenerated by a current SDK. - l.Info("compactPebble: source selection", - zap.Int("sources", len(sources)), - zap.Int("manifest_selected", manifestSelected), - zap.Int("unpack_selected", len(sources)-manifestSelected), - zap.String("mode", string(pebbleCompactorMode)), - ) - var statsRec *v3.SyncStatsRecord - var err error - switch pebbleCompactorMode { - case PebbleCompactorModeOverlay: - // Oversized buckets are routed to the K-way run-file path inside - // the overlay merge itself (overlayPlanBuckets), so there is no - // whole-merge fallback here. - var overlayOpts []mergepkg.OverlayOption - if c.overlaySeenKeyLimit > 0 { - overlayOpts = append(overlayOpts, mergepkg.WithOverlaySeenKeyLimit(c.overlaySeenKeyLimit)) - } - if c.overlayRecordChunkSize > 0 { - overlayOpts = append(overlayOpts, mergepkg.WithOverlayRecordChunkSize(c.overlayRecordChunkSize)) + if err := rebuildCompactedGrantDigests(ctx, destEng); err != nil { + return fmt.Errorf("compactPebble: %w", err) } - if c.overlayBufferFactor > 0 { - overlayOpts = append(overlayOpts, mergepkg.WithOverlayBufferFactor(c.overlayBufferFactor)) + + // Set the compacted sync_run's type + ended_at to the union / max + // across the inputs so downstream gating (e.g. grant expansion) + // behaves identically to the sqlite path, then recompute stats. + rec, err := destEng.GetSyncRunRecord(ctx, newSyncId) + if err != nil { + return fmt.Errorf("compactPebble: load dest sync_run: %w", err) } - if c.overlayGateFraction > 0 { - overlayOpts = append(overlayOpts, mergepkg.WithOverlayGateFraction(c.overlayGateFraction)) + rec.SetType(unionType) + if !maxEnded.IsZero() { + rec.SetEndedAt(timestamppb.New(maxEnded)) } - statsRec, err = mergepkg.MergeFilesIntoOverlay(ctx, destEng, sources, newSyncId, c.tmpDir, overlayOpts...) - default: - statsRec, err = mergepkg.MergeFilesInto(ctx, destEng, sources, newSyncId, c.tmpDir) - } - if err != nil { - return fmt.Errorf("compactPebble: merge: %w", err) - } - - if err := rebuildCompactedGrantDigests(ctx, destEng); err != nil { - return fmt.Errorf("compactPebble: %w", err) - } - - // Set the compacted sync_run's type + ended_at to the union / max - // across the inputs so downstream gating (e.g. grant expansion) - // behaves identically to the sqlite path, then recompute stats. - rec, err := destEng.GetSyncRunRecord(ctx, newSyncId) - if err != nil { - return fmt.Errorf("compactPebble: load dest sync_run: %w", err) - } - rec.SetType(unionType) - if !maxEnded.IsZero() { - rec.SetEndedAt(timestamppb.New(maxEnded)) - } - // The merge accumulated the dest stats while writing winners, so - // persist those instead of re-scanning the freshly written output. - if statsRec != nil { - if err := destEng.PersistComputedSyncStats(ctx, newSyncId, statsRec); err != nil { - return fmt.Errorf("compactPebble: persist stats: %w", err) + // The merge accumulated the dest stats while writing winners, so + // persist those instead of re-scanning the freshly written output. + if statsRec != nil { + if err := destEng.PersistComputedSyncStats(ctx, newSyncId, statsRec); err != nil { + return fmt.Errorf("compactPebble: persist stats: %w", err) + } + } else { + if err := destEng.PersistSyncStats(ctx, newSyncId); err != nil { + return fmt.Errorf("compactPebble: persist stats: %w", err) + } + recomputed, statsErr := enginepkg.ReadSyncStatsRecord(ctx, destEng, newSyncId) + if statsErr != nil { + l.Warn("compactPebble: could not read output stats for provenance", zap.Error(statsErr)) + } else { + statsRec = recomputed + } } - } else { - if err := destEng.PersistSyncStats(ctx, newSyncId); err != nil { - return fmt.Errorf("compactPebble: persist stats: %w", err) + // Stamp compaction provenance on the (otherwise empty) rebuild token. + // Rebuild merges lose per-source attribution in their run-file paths, + // so record counts carry output totals only, and the partials' timing + // aggregate is fold-only — collecting rebuild source tokens would pay + // a second envelope unpack per source. Best-effort: provenance never + // fails the compaction. + mode := PebbleCompactorModeKWay + if useOverlay { + mode = PebbleCompactorModeOverlay } - recomputed, statsErr := enginepkg.ReadSyncStatsRecord(ctx, destEng, newSyncId) - if statsErr != nil { - l.Warn("compactPebble: could not read output stats for provenance", zap.Error(statsErr)) + compactedToken, tokenErr := sdksync.BuildCompactedToken(rec.GetSyncToken(), sdksync.CompactionTokenInput{ + Mode: string(mode), + BaseSyncID: rebuildBaseSyncID, + PartialSyncIDs: rebuildPartialSyncIDs, + RecordCounts: compactionRecordCounts(statsRec, nil), + }) + if tokenErr != nil { + l.Warn("compactPebble: could not build compaction provenance token", zap.Error(tokenErr)) } else { - statsRec = recomputed + rec.SetSyncToken(compactedToken) } - } - // Stamp compaction provenance on the (otherwise empty) rebuild token. - // Rebuild merges lose per-source attribution in their run-file paths, - // so record counts carry output totals only, and the partials' timing - // aggregate is fold-only — collecting rebuild source tokens would pay - // a second envelope unpack per source. Best-effort: provenance never - // fails the compaction. - mode := PebbleCompactorModeKWay - if useOverlay { - mode = PebbleCompactorModeOverlay - } - compactedToken, tokenErr := sdksync.BuildCompactedToken(rec.GetSyncToken(), sdksync.CompactionTokenInput{ - Mode: string(mode), - BaseSyncID: rebuildBaseSyncID, - PartialSyncIDs: rebuildPartialSyncIDs, - RecordCounts: compactionRecordCounts(statsRec, nil), + if err := destEng.PutSyncRunRecord(ctx, rec); err != nil { + return fmt.Errorf("compactPebble: persist dest sync_run: %w", err) + } + return nil }) - if tokenErr != nil { - l.Warn("compactPebble: could not build compaction provenance token", zap.Error(tokenErr)) - } else { - rec.SetSyncToken(compactedToken) - } - if err := destEng.PutSyncRunRecord(ctx, rec); err != nil { - return fmt.Errorf("compactPebble: persist dest sync_run: %w", err) - } - return nil } diff --git a/pkg/synccompactor/compactor_provenance_test.go b/pkg/synccompactor/compactor_provenance_test.go index 6e91c1c30..da94f04b6 100644 --- a/pkg/synccompactor/compactor_provenance_test.go +++ b/pkg/synccompactor/compactor_provenance_test.go @@ -30,13 +30,14 @@ func stampSyncToken(t *testing.T, ctx context.Context, path, syncID, token strin t.Helper() w, err := dotc1z.NewStore(ctx, path, dotc1z.WithTmpDir(t.TempDir())) require.NoError(t, err) - eng, ok := enginepkg.AsEngine(w) - require.True(t, ok, "store at %s is not a pebble engine", path) - rec, err := eng.GetSyncRunRecord(ctx, syncID) - require.NoError(t, err) - rec.SetSyncToken(token) - require.NoError(t, eng.PutSyncRunRecord(ctx, rec)) - require.True(t, enginepkg.MarkStoreDirty(w)) + require.NoError(t, enginepkg.WithEngineMutation(ctx, w, func(ctx context.Context, eng *enginepkg.Engine) error { + rec, err := eng.GetSyncRunRecord(ctx, syncID) + if err != nil { + return err + } + rec.SetSyncToken(token) + return eng.PutSyncRunRecord(ctx, rec) + })) require.NoError(t, w.Close(ctx)) } diff --git a/pkg/synccompactor/prodscale_phases_test.go b/pkg/synccompactor/prodscale_phases_test.go index 9a22ca1f7..556115a1a 100644 --- a/pkg/synccompactor/prodscale_phases_test.go +++ b/pkg/synccompactor/prodscale_phases_test.go @@ -50,7 +50,9 @@ func TestProdScaleFoldPhases(t *testing.T) { require.NoError(t, err) t.Logf("phase open(unpack): %s", time.Since(start).Round(time.Millisecond)) - require.True(t, enginepkg.MarkStoreDirty(w)) + require.NoError(t, enginepkg.WithEngineMutation(ctx, w, func(context.Context, *enginepkg.Engine) error { + return nil + })) start = time.Now() require.NoError(t, w.Close(ctx)) t.Logf("phase save(close): %s", time.Since(start).Round(time.Millisecond)) diff --git a/pkg/tasks/c1api/manager_test.go b/pkg/tasks/c1api/manager_test.go index 11fc52bca..c50073aab 100644 --- a/pkg/tasks/c1api/manager_test.go +++ b/pkg/tasks/c1api/manager_test.go @@ -134,12 +134,20 @@ func enableGetTasks(t *testing.T) { t.Setenv(getTasksEnv, "true") } +func bootstrapTestContext(t *testing.T) context.Context { + t.Helper() + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + t.Cleanup(cancel) + return ctx +} + func TestBootstrapSucceedsOnFirstAttempt(t *testing.T) { + withFastBackoff(t) sc := newFakeBatonServiceClient([]error{nil}) cc := &fakeConnectorClient{} mgr := newTestManager(sc) - require.NoError(t, mgr.Bootstrap(context.Background(), cc)) + require.NoError(t, mgr.Bootstrap(bootstrapTestContext(t), cc)) require.Equal(t, 1, sc.helloCalls) } @@ -151,7 +159,7 @@ func TestBootstrapRetriesOnTransientFailure(t *testing.T) { cc := &fakeConnectorClient{} mgr := newTestManager(sc) - require.NoError(t, mgr.Bootstrap(context.Background(), cc)) + require.NoError(t, mgr.Bootstrap(bootstrapTestContext(t), cc)) require.Equal(t, 3, sc.helloCalls, "expected 3 Hello calls (2 transient failures + success)") } @@ -163,7 +171,7 @@ func TestBootstrapStopsOnNonRetryableError(t *testing.T) { cc := &fakeConnectorClient{} mgr := newTestManager(sc) - err := mgr.Bootstrap(context.Background(), cc) + err := mgr.Bootstrap(bootstrapTestContext(t), cc) require.Error(t, err, "expected Bootstrap to return an error for non-retryable Hello failure") require.Equal(t, codes.Unauthenticated, status.Code(err)) require.Equal(t, 1, sc.helloCalls, "expected exactly 1 Hello call (no retries on non-retryable)") @@ -206,7 +214,7 @@ func TestBootstrapPropagatesGetMetadataError(t *testing.T) { cc.metadataErr.Store(status.Error(codes.PermissionDenied, "no metadata")) mgr := newTestManager(sc) - err := mgr.Bootstrap(context.Background(), cc) + err := mgr.Bootstrap(bootstrapTestContext(t), cc) require.Error(t, err, "expected Bootstrap to return an error when GetMetadata fails") require.Equal(t, codes.PermissionDenied, status.Code(err)) require.Equal(t, 0, sc.helloCalls, "Hello should not be invoked when GetMetadata fails") diff --git a/pkg/uhttp/dbcache_test.go b/pkg/uhttp/dbcache_test.go index 606d3e331..235a1161d 100644 --- a/pkg/uhttp/dbcache_test.go +++ b/pkg/uhttp/dbcache_test.go @@ -2,6 +2,7 @@ package uhttp import ( "net/http" + "os" "testing" "time" @@ -14,7 +15,7 @@ var urlTest = "https://jsonplaceholder.typicode.com/posts/1/comments" func TestDBCacheGettersAndSetters(t *testing.T) { cli := &http.Client{} - fc, err := getDBCacheForTesting() + fc, err := getDBCacheForTesting(t) require.Nil(t, err) req, err := http.NewRequestWithContext(ctx, http.MethodGet, urlTest, nil) @@ -50,7 +51,7 @@ func TestDBCacheGettersAndSetters(t *testing.T) { } func TestDBCache(t *testing.T) { - fc, err := getDBCacheForTesting() + fc, err := getDBCacheForTesting(t) require.Nil(t, err) data := []byte("Testing 123") @@ -65,7 +66,18 @@ func TestDBCache(t *testing.T) { require.Equal(t, data, res) } -func getDBCacheForTesting() (*DBCache, error) { +func getDBCacheForTesting(t *testing.T) (*DBCache, error) { + t.Helper() + t.Setenv("HOME", t.TempDir()) + t.Setenv("XDG_CACHE_HOME", t.TempDir()) + cacheDir, err := os.UserCacheDir() + if err != nil { + return nil, err + } + if err := os.MkdirAll(cacheDir, 0o755); err != nil { + return nil, err + } + fc, err := NewDBCache(ctx, CacheConfig{ TTL: 3600 * time.Second, })