From 1834755c0901c9bd87585fca4c58aa0a866906d5 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Thu, 13 Aug 2026 12:35:19 -0600 Subject: [PATCH 01/15] Default the dotc1z storage engine to Pebble instead of SQLite An unset engine in NewStore now resolves to EnginePebble: new c1z files are written in the v3/Pebble format, and writable opens of existing v1/SQLite files convert to Pebble, matching the existing explicit WithEngine(EnginePebble) behavior. Explicit WithEngine(EngineSQLite) still selects the legacy v1 engine, and NewC1ZFile remains SQLite-only. Downstream tests that relied on the SQLite default are expected to break; the direct default-assertion tests are updated here. Co-authored-by: Cursor --- pkg/dotc1z/c1file.go | 4 ++-- pkg/dotc1z/c1file_test.go | 8 ++++---- pkg/dotc1z/c1zstore/engine.go | 9 +++++---- pkg/dotc1z/engine_registry.go | 6 +++--- pkg/dotc1z/engine_registry_test.go | 5 +++-- pkg/field/defaults.go | 2 +- 6 files changed, 18 insertions(+), 16 deletions(-) diff --git a/pkg/dotc1z/c1file.go b/pkg/dotc1z/c1file.go index 70240e458..f060e335c 100644 --- a/pkg/dotc1z/c1file.go +++ b/pkg/dotc1z/c1file.go @@ -433,8 +433,8 @@ func WithSyncLimit(limit int) C1ZOption { } // WithEngine selects the storage engine for newly created .c1z files. -// Default is EngineSQLite (v1 format). EnginePebble enables the v3 -// engine. +// Default is EnginePebble (v3 format). EngineSQLite selects the legacy +// v1 engine. // // Reading existing files dispatches on the file's magic byte and is // independent of this option. diff --git a/pkg/dotc1z/c1file_test.go b/pkg/dotc1z/c1file_test.go index aef2a9254..5f719810d 100644 --- a/pkg/dotc1z/c1file_test.go +++ b/pkg/dotc1z/c1file_test.go @@ -686,13 +686,13 @@ func TestC1ZCachedViewSyncRunInvalidation(t *testing.T) { require.NoError(t, err) } -// TestEngineDefaultsToSQLite proves that omitting WithEngine yields -// c1zstore.EngineSQLite (the documented default), not the empty zero value. -func TestEngineDefaultsToSQLite(t *testing.T) { +// TestEngineDefaultsToPebble proves that omitting WithEngine yields +// c1zstore.EnginePebble (the documented default), not the empty zero value. +func TestEngineDefaultsToPebble(t *testing.T) { dir := t.TempDir() f, err := NewStore(context.Background(), filepath.Join(dir, "default.c1z")) require.NoError(t, err, "NewStore") defer f.Close(context.Background()) engine := f.Metadata().Engine - require.Equal(t, string(c1zstore.EngineSQLite), engine) + require.Equal(t, string(c1zstore.EnginePebble), engine) } diff --git a/pkg/dotc1z/c1zstore/engine.go b/pkg/dotc1z/c1zstore/engine.go index 833fa7cd4..d6e31ff2b 100644 --- a/pkg/dotc1z/c1zstore/engine.go +++ b/pkg/dotc1z/c1zstore/engine.go @@ -13,12 +13,13 @@ import ( type Engine string const ( - // EngineSQLite is the default engine: the v1 .c1z format backed by - // a zstd-compressed SQLite database. Connectors use this; backend - // infra can opt out. + // EngineSQLite is the legacy v1 engine: the v1 .c1z format backed by + // a zstd-compressed SQLite database. Callers opt into it via + // WithEngine; the default engine is EnginePebble. EngineSQLite Engine = "sqlite" - // EnginePebble is the v3 engine: a Pebble LSM wrapped in the v3 + // EnginePebble is the v3 engine and the default when callers do not + // specify one: a Pebble LSM wrapped in the v3 // envelope. This is the in-process identity AND the value callers // select with (the --storage-engine flag and the gRPC sync-task // field both pass "pebble"); it must stay "pebble" for those diff --git a/pkg/dotc1z/engine_registry.go b/pkg/dotc1z/engine_registry.go index db95f26ba..8fea24a37 100644 --- a/pkg/dotc1z/engine_registry.go +++ b/pkg/dotc1z/engine_registry.go @@ -204,7 +204,7 @@ func storeOptionsFromC1ZOptions(options *c1zOptions) StoreOptions { MaxDecoderMemoryBytes: maxDecoderMemoryBytes, } if out.Engine == "" { - out.Engine = c1zstore.EngineSQLite + out.Engine = c1zstore.EnginePebble } out.Pragmas = make([]StorePragma, 0, len(options.pragmas)) for _, p := range options.pragmas { @@ -218,7 +218,7 @@ func storeOptionsFromC1ZOptions(options *c1zOptions) StoreOptions { // Dispatch policy (in order): // // 1. If the file doesn't exist or is empty, honor the caller's -// `WithEngine(...)` choice (defaulting to EngineSQLite when +// `WithEngine(...)` choice (defaulting to EnginePebble when // unset). The about-to-be-written file gets the requested format. // 2. If the file exists with content, dispatch by the on-disk magic // byte — v1 → SQLite, v3 → whatever engine name the manifest @@ -235,7 +235,7 @@ func selectStoreDriver(ctx context.Context, outputFilePath string, options *c1zO l := ctxzap.Extract(ctx) requested := options.engine if requested == "" { - requested = c1zstore.EngineSQLite + requested = c1zstore.EnginePebble } stat, err := os.Stat(outputFilePath) // #nosec G703 -- c1z path is caller-controlled by API design. diff --git a/pkg/dotc1z/engine_registry_test.go b/pkg/dotc1z/engine_registry_test.go index 3470d3c2d..34a5be1b3 100644 --- a/pkg/dotc1z/engine_registry_test.go +++ b/pkg/dotc1z/engine_registry_test.go @@ -64,14 +64,15 @@ func TestRegisterEngineRejectsDuplicateEngine(t *testing.T) { require.Error(t, err, "RegisterEngine duplicate returned nil error") } -func TestNewStoreDefaultsToSQLiteDriver(t *testing.T) { +func TestNewStoreDefaultsToPebbleDriver(t *testing.T) { ctx := context.Background() path := filepath.Join(t.TempDir(), "default.c1z") store, err := NewStore(ctx, path) require.NoError(t, err) defer func() { _ = store.Close(ctx) }() _, ok := store.(*C1File) - require.True(t, ok, "NewStore default type = %T, want *C1File", store) + require.False(t, ok, "NewStore default type = %T, want the pebble store, not *C1File", store) + require.Equal(t, string(c1zstore.EnginePebble), store.Metadata().Engine) } func TestNewStoreRequiresRegisteredEngineForNewFile(t *testing.T) { diff --git a/pkg/field/defaults.go b/pkg/field/defaults.go index 168570423..81816c717 100644 --- a/pkg/field/defaults.go +++ b/pkg/field/defaults.go @@ -355,7 +355,7 @@ var ( })) // StorageEngineField selects the dotc1z storage engine for sync tasks. - // Empty uses the baton-sdk default (sqlite for new files). + // Empty uses the baton-sdk default (pebble for new files). StorageEngineField = StringField("storage-engine", WithDescription("The storage engine to use when opening the sync c1z file: sqlite or pebble. "+ "Leave unset to use the baton-sdk default."), From fa70491f5a71aa52135bac8940d7810696e4ecfe Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Thu, 13 Aug 2026 12:39:20 -0600 Subject: [PATCH 02/15] Make convert-open sqlite test request SQLite explicitly With Pebble as the default engine, an engine-less writable reopen of a v1 file now converts it to v3. The test's purpose is to prove an explicit SQLite request never converts, so pass WithEngine(EngineSQLite) on the reopen. Co-authored-by: Cursor --- pkg/dotc1z/convert_open_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/dotc1z/convert_open_test.go b/pkg/dotc1z/convert_open_test.go index 311f27981..2e29d0668 100644 --- a/pkg/dotc1z/convert_open_test.go +++ b/pkg/dotc1z/convert_open_test.go @@ -111,7 +111,7 @@ func TestNewC1ZFileDoesNotConvertExistingSQLiteWhenEngineSQLite(t *testing.T) { require.NoError(t, seedFinishedSQLiteSync(ctx, t, src)) require.NoError(t, src.Close(ctx)) - f, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithTmpDir(dir)) + f, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithTmpDir(dir), dotc1z.WithEngine(c1zstore.EngineSQLite)) require.NoError(t, err) defer func() { require.NoError(t, f.Close(ctx)) }() From 327d169466f778373787ab48f912e422bde8d5e6 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Thu, 13 Aug 2026 12:42:13 -0600 Subject: [PATCH 03/15] Name pebble as the default in the --storage-engine help text The flag description said "leave unset to use the baton-sdk default" without naming it; now that the default flipped to pebble, say so. Co-authored-by: Cursor --- pkg/field/defaults.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/field/defaults.go b/pkg/field/defaults.go index 81816c717..4dc0bd192 100644 --- a/pkg/field/defaults.go +++ b/pkg/field/defaults.go @@ -358,7 +358,7 @@ var ( // Empty uses the baton-sdk default (pebble for new files). StorageEngineField = StringField("storage-engine", WithDescription("The storage engine to use when opening the sync c1z file: sqlite or pebble. "+ - "Leave unset to use the baton-sdk default."), + "Defaults to pebble when unset."), WithPersistent(true), WithExportTarget(ExportTargetNone), WithString(func(r *StringRuler) { From da7afdcd145b45b338c3e9a3d43cddd8c7329c31 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Thu, 13 Aug 2026 13:41:09 -0600 Subject: [PATCH 04/15] Convert v1 files only on an explicit Pebble request With Pebble as the engine default, the default was also triggering the in-place v1-to-pebble conversion on every engine-less writable open of an existing v1 file. Read-intent callers (provisioner, local differ, baton recalculate-stats, baton optimize) open stores without an engine and would silently rewrite user files; optimize would convert the file and then report "no changes made". The conversion now requires an explicit WithEngine(EnginePebble); engine-less opens of existing files always dispatch on the magic byte. New files still default to Pebble. The conversion log moves from Debug to Info so an in-place format migration is visible, and tests cover the engine-less writable and read-only reopen paths plus the on-disk v3 header of a default-engine artifact. Co-authored-by: Cursor --- pkg/dotc1z/convert_open_test.go | 31 ++++++++++++++++++++++++++++++ pkg/dotc1z/engine_registry.go | 13 ++++++++++--- pkg/dotc1z/engine_registry_test.go | 16 ++++++++++++++- 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/pkg/dotc1z/convert_open_test.go b/pkg/dotc1z/convert_open_test.go index 2e29d0668..cc6337f59 100644 --- a/pkg/dotc1z/convert_open_test.go +++ b/pkg/dotc1z/convert_open_test.go @@ -100,6 +100,37 @@ func TestNewC1ZFileDoesNotConvertNewSQLiteFile(t *testing.T) { require.Equal(t, string(c1zstore.EngineSQLite), f.Metadata().Engine) } +// TestEnginelessOpenOfExistingV1DoesNotConvert proves the EnginePebble +// default never rewrites existing v1 files: engine-less opens dispatch +// on the magic byte and stay SQLite, whether writable or read-only. +// Only an explicit WithEngine(EnginePebble) converts (see +// TestNewStoreConvertsExistingSQLiteToPebble). +func TestEnginelessOpenOfExistingV1DoesNotConvert(t *testing.T) { + ctx := context.Background() + + dir := t.TempDir() + c1zPath := filepath.Join(dir, "source.c1z") + + src, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithTmpDir(dir), dotc1z.WithEngine(c1zstore.EngineSQLite)) + require.NoError(t, err) + require.NoError(t, seedFinishedSQLiteSync(ctx, t, src)) + require.NoError(t, src.Close(ctx)) + + // Writable engine-less open: no conversion. + f, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithTmpDir(dir)) + require.NoError(t, err) + require.Equal(t, string(c1zstore.EngineSQLite), f.Metadata().Engine) + require.NoError(t, f.Close(ctx)) + require.Equal(t, dotc1z.C1ZFormatV1, mustReadHeaderFormat(t, c1zPath)) + + // Read-only engine-less open: no conversion. + ro, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithTmpDir(dir), dotc1z.WithReadOnly(true)) + require.NoError(t, err) + require.Equal(t, string(c1zstore.EngineSQLite), ro.Metadata().Engine) + require.NoError(t, ro.Close(ctx)) + require.Equal(t, dotc1z.C1ZFormatV1, mustReadHeaderFormat(t, c1zPath)) +} + func TestNewC1ZFileDoesNotConvertExistingSQLiteWhenEngineSQLite(t *testing.T) { ctx := context.Background() diff --git a/pkg/dotc1z/engine_registry.go b/pkg/dotc1z/engine_registry.go index 8fea24a37..39b37e25d 100644 --- a/pkg/dotc1z/engine_registry.go +++ b/pkg/dotc1z/engine_registry.go @@ -226,6 +226,11 @@ func storeOptionsFromC1ZOptions(options *c1zOptions) StoreOptions { // case because we can't re-encode an existing file at open time; // the on-disk format is authoritative. This preserves the // read-any-format semantics that pre-dates the engine option. +// Exception: an EXPLICIT WithEngine(EnginePebble) on a writable v1 +// file converts it to Pebble in place. The EnginePebble default +// never triggers that conversion — an engine-less open of an +// existing v1 file stays SQLite, so read-intent callers (diff, +// stats, provisioning) don't rewrite files as a side effect. // // When the caller's WithEngine disagrees with the on-disk format we // log a warning so the divergence is observable. Callers that want @@ -267,7 +272,9 @@ func selectStoreDriver(ctx context.Context, outputFilePath string, options *c1zO switch format { case C1ZFormatV1: // Maybe error if the file is read-only? - if requested == c1zstore.EnginePebble && !options.readOnly { + // Only an explicit pebble request converts; the engine default + // (options.engine == "") must not rewrite existing v1 files. + if options.engine == c1zstore.EnginePebble && !options.readOnly { // Close our header-read handle before converting: the conversion // renames a temp file over outputFilePath, which fails on Windows // if any handle to the destination is still open. Nil out f so @@ -277,11 +284,11 @@ func selectStoreDriver(ctx context.Context, outputFilePath string, options *c1zO if closeErr != nil { return nil, closeErr } - l.Debug("converting existing v1 c1z to pebble", zap.String("output_file_path", outputFilePath)) + l.Info("converting existing v1 c1z to pebble", zap.String("output_file_path", outputFilePath)) if err := convertExistingV1C1ZFile(ctx, outputFilePath, pebbleOpenOptionsFromC1Z(options)); err != nil { return nil, fmt.Errorf("select-store-driver: convert existing v1 c1z to pebble: %w", err) } - l.Debug("converted existing v1 c1z to pebble", zap.String("output_file_path", outputFilePath)) + l.Info("converted existing v1 c1z to pebble", zap.String("output_file_path", outputFilePath)) return requireEngineDriver(c1zstore.EnginePebble) } fileEngine = c1zstore.EngineSQLite diff --git a/pkg/dotc1z/engine_registry_test.go b/pkg/dotc1z/engine_registry_test.go index 34a5be1b3..c47fe67a1 100644 --- a/pkg/dotc1z/engine_registry_test.go +++ b/pkg/dotc1z/engine_registry_test.go @@ -69,10 +69,24 @@ func TestNewStoreDefaultsToPebbleDriver(t *testing.T) { path := filepath.Join(t.TempDir(), "default.c1z") store, err := NewStore(ctx, path) require.NoError(t, err) - defer func() { _ = store.Close(ctx) }() _, ok := store.(*C1File) require.False(t, ok, "NewStore default type = %T, want the pebble store, not *C1File", store) require.Equal(t, string(c1zstore.EnginePebble), store.Metadata().Engine) + + // Seal a sync and close so the artifact lands on disk, then prove + // the on-disk format is v3 by magic byte, not just in-process + // metadata. + _, err = store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.NoError(t, store.EndSync(ctx)) + require.NoError(t, store.Close(ctx)) + + f, err := os.Open(path) + require.NoError(t, err) + defer f.Close() + format, err := ReadHeaderFormat(f) + require.NoError(t, err) + require.Equal(t, C1ZFormatV3, format, "default-engine artifact must be a v3 c1z on disk") } func TestNewStoreRequiresRegisteredEngineForNewFile(t *testing.T) { From 4a1e46c4428acb5026fc0dae201e81e80915cf16 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Thu, 13 Aug 2026 13:41:09 -0600 Subject: [PATCH 05/15] Pin the compactor destination store to the resolved engine The SQLite branch of doOneCompaction opened the destination with no engine and relied on the dotc1z default being SQLite. With the default now Pebble, an all-SQLite compaction created a v3 destination and then failed in the attached compactor. Always pass the resolved engine when opening the destination, and fix the comments that claimed the unset compactor default is SQLite (it follows the inputs). Co-authored-by: Cursor --- pkg/synccompactor/compactor.go | 21 +++++++++++---------- pkg/synccompactor/compactor_pebble.go | 9 +++++---- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/pkg/synccompactor/compactor.go b/pkg/synccompactor/compactor.go index b1cfeee3e..243e77b74 100644 --- a/pkg/synccompactor/compactor.go +++ b/pkg/synccompactor/compactor.go @@ -45,9 +45,11 @@ type Compactor struct { c1zOptions []dotc1z.C1ZOption skipGrantExpansion bool // engine selects the storage engine for the compacted output. - // Empty means EngineSQLite (the default; behavior is unchanged and - // the output is byte-identical to the pre-engine-option compactor). - // EnginePebble produces a v3 Pebble c1z via a native record merge. + // Empty means "follow the inputs": Compact resolves it via + // inferEngineFromInputs (any Pebble input → Pebble, all-SQLite → + // SQLite), so the compactor does NOT follow the dotc1z engine + // default. EnginePebble produces a v3 Pebble c1z via a native + // record merge. engine c1zstore.Engine // pebbleMode optionally forces the Pebble merge strategy; the zero // value (Auto) lets the compactor choose. See WithPebbleCompactorMode. @@ -348,13 +350,12 @@ func (c *Compactor) Compact(ctx context.Context) (*CompactableSync, error) { opts = append(opts, dotc1z.WithDecoderPool(c.decoderPool)) } - if c.resolvedEngine() == c1zstore.EnginePebble { - // Force the resolved engine last so a stray engine passed via - // WithC1ZOptions cannot mislabel the artifact. - c.compactedC1z, err = dotc1z.NewStore(ctx, destFilePath, append(opts, dotc1z.WithEngine(c1zstore.EnginePebble))...) - } else { - c.compactedC1z, err = dotc1z.NewStore(ctx, destFilePath, opts...) - } + // Force the resolved engine last: a stray engine passed via + // WithC1ZOptions cannot mislabel the artifact, and the dotc1z + // engine default (Pebble) cannot leak into a SQLite compaction — + // the destination is a new file, so an engine-less open would + // otherwise create a v3 store under the SQLite merge path. + c.compactedC1z, err = dotc1z.NewStore(ctx, destFilePath, append(opts, dotc1z.WithEngine(c.resolvedEngine()))...) if err != nil { l.Error("doOneCompaction failed: could not create c1z file", zap.Error(err)) return nil, err diff --git a/pkg/synccompactor/compactor_pebble.go b/pkg/synccompactor/compactor_pebble.go index c0ee84083..5dd100a46 100644 --- a/pkg/synccompactor/compactor_pebble.go +++ b/pkg/synccompactor/compactor_pebble.go @@ -27,10 +27,11 @@ import ( ) // WithEngine selects the storage engine for the compacted output. -// The default (unset) is sqlite, which is byte-identical to the -// historical compactor. EnginePebble produces a v3 Pebble c1z via a -// native record merge whose strategy (overlay / fold / kway) is -// resolved per run by resolvePebbleMode. +// The default (unset) follows the inputs — any Pebble input makes the +// output Pebble; all-SQLite inputs keep SQLite output, byte-identical +// to the historical compactor (see inferEngineFromInputs). EnginePebble +// produces a v3 Pebble c1z via a native record merge whose strategy +// (overlay / fold / kway) is resolved per run by resolvePebbleMode. // // This is the only supported way to choose the engine; an engine // passed through WithC1ZOptions does not select the compaction From 12fd57ed75b9fa38ea523de4957eaa9c1d4194ed Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Thu, 13 Aug 2026 13:41:10 -0600 Subject: [PATCH 06/15] Update storage_engine proto comment and RFC 0001 for the pebble default The storage_engine field comment claimed SQLite is the default engine; regenerated the pb mirrors. RFC 0001 gets an amendment note that the default flipped to Pebble for new files while existing files keep their on-disk format. Co-authored-by: Cursor --- docs/rfcs/0001-pebble-storage-engine.md | 8 ++++++++ pb/c1/connectorapi/baton/v1/baton.pb.go | 4 ++-- pb/c1/connectorapi/baton/v1/baton_protoopaque.pb.go | 2 +- proto/c1/connectorapi/baton/v1/baton.proto | 2 +- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/docs/rfcs/0001-pebble-storage-engine.md b/docs/rfcs/0001-pebble-storage-engine.md index e7bd619f4..78ff1bf2e 100644 --- a/docs/rfcs/0001-pebble-storage-engine.md +++ b/docs/rfcs/0001-pebble-storage-engine.md @@ -6,6 +6,14 @@ - **Prior art:** `morgabra/pebble` branch (commit `ae4b9693`, Aug 2025), `.kiro/specs/pebble-storage-engine/` - **Related PRs on main:** #591 (parallel sync), #666 (expandable at SQL), #773 (zstd pool), #769/#768 (skip cleanup), #735 (size verification removal) +> **Amendment (Aug 2026):** The default flipped. This RFC was written when +> SQLite was the default engine and Pebble was opt-in; the SDK default is now +> Pebble for NEWLY CREATED c1z files (`kans/pebble-default-engine`). Existing +> files still dispatch on their on-disk magic byte, and only an explicit +> `WithEngine(EnginePebble)` converts a v1 file in place. Statements below +> that describe SQLite as the connector default (Summary, §5.3/§5.4) reflect +> the original rollout posture and are retained for historical context. + ## 1. Summary Introduce a storage-engine abstraction in `pkg/dotc1z/` so that the same sync, compaction, and read/write code paths can run against either the current SQLite-in-a-`.c1z` backend or a new Pebble-backed backend. Connectors continue to default to SQLite (unchanged binary footprint, no cgo regressions, full on-disk compatibility). Backend infra (where we own both read and write sides and want more throughput) can opt into the Pebble engine via explicit configuration. diff --git a/pb/c1/connectorapi/baton/v1/baton.pb.go b/pb/c1/connectorapi/baton/v1/baton.pb.go index fe7c7788b..cb17330f3 100644 --- a/pb/c1/connectorapi/baton/v1/baton.pb.go +++ b/pb/c1/connectorapi/baton/v1/baton.pb.go @@ -2935,7 +2935,7 @@ type Task_SyncFullTask struct { SyncResourceTypeIds []string `protobuf:"bytes,5,rep,name=sync_resource_type_ids,json=syncResourceTypeIds,proto3" json:"sync_resource_type_ids,omitempty"` // If true, skip syncing grants. Resources and entitlements will still be synced. SkipGrants bool `protobuf:"varint,6,opt,name=skip_grants,json=skipGrants,proto3" json:"skip_grants,omitempty"` - // Storage engine to use for the sync. If empty, the default engine will be used (currently SQLite). + // Storage engine to use for the sync. If empty, the default engine will be used (currently Pebble for new c1z files; existing files keep their on-disk format). StorageEngine string `protobuf:"bytes,7,opt,name=storage_engine,json=storageEngine,proto3" json:"storage_engine,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache @@ -3057,7 +3057,7 @@ type Task_SyncFullTask_builder struct { SyncResourceTypeIds []string // If true, skip syncing grants. Resources and entitlements will still be synced. SkipGrants bool - // Storage engine to use for the sync. If empty, the default engine will be used (currently SQLite). + // Storage engine to use for the sync. If empty, the default engine will be used (currently Pebble for new c1z files; existing files keep their on-disk format). StorageEngine string } diff --git a/pb/c1/connectorapi/baton/v1/baton_protoopaque.pb.go b/pb/c1/connectorapi/baton/v1/baton_protoopaque.pb.go index 833a47a42..aa86aa5b1 100644 --- a/pb/c1/connectorapi/baton/v1/baton_protoopaque.pb.go +++ b/pb/c1/connectorapi/baton/v1/baton_protoopaque.pb.go @@ -3022,7 +3022,7 @@ type Task_SyncFullTask_builder struct { SyncResourceTypeIds []string // If true, skip syncing grants. Resources and entitlements will still be synced. SkipGrants bool - // Storage engine to use for the sync. If empty, the default engine will be used (currently SQLite). + // Storage engine to use for the sync. If empty, the default engine will be used (currently Pebble for new c1z files; existing files keep their on-disk format). StorageEngine string } diff --git a/proto/c1/connectorapi/baton/v1/baton.proto b/proto/c1/connectorapi/baton/v1/baton.proto index 64a24b404..8855f9bf9 100644 --- a/proto/c1/connectorapi/baton/v1/baton.proto +++ b/proto/c1/connectorapi/baton/v1/baton.proto @@ -59,7 +59,7 @@ message Task { repeated string sync_resource_type_ids = 5; // If true, skip syncing grants. Resources and entitlements will still be synced. bool skip_grants = 6; - // Storage engine to use for the sync. If empty, the default engine will be used (currently SQLite). + // Storage engine to use for the sync. If empty, the default engine will be used (currently Pebble for new c1z files; existing files keep their on-disk format). string storage_engine = 7 [(validate.rules).string = { in: [ "pebble", From ee6683a7d3f39bd85f5b2259a92165d451231ee0 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Thu, 13 Aug 2026 14:12:24 -0600 Subject: [PATCH 07/15] Open sync-test artifacts with the engine-neutral NewStore The syncer now writes v3/Pebble files by default, and tests that reopened the artifact with NewC1ZFile (the SQLite-only constructor) failed with a magic-number mismatch. Reopen through NewStore, reach ListSyncRuns and CloneSync through their engine-neutral surfaces, and pin TestCleanupContextDeadlineExceeded to SQLite: its 200ms run budget is only reliably exceeded by SQLite's row-by-row cleanup, while Pebble drops old syncs via cheap range deletes and completes the sync. Co-authored-by: Cursor --- pkg/sync/ingest_filter_test.go | 10 +++---- pkg/sync/syncer_cleanup_test.go | 7 ++++- pkg/sync/syncer_test.go | 48 ++++++++++++++++++--------------- 3 files changed, 37 insertions(+), 28 deletions(-) diff --git a/pkg/sync/ingest_filter_test.go b/pkg/sync/ingest_filter_test.go index 692696012..ce2365831 100644 --- a/pkg/sync/ingest_filter_test.go +++ b/pkg/sync/ingest_filter_test.go @@ -365,7 +365,7 @@ func TestFreshIngestFilterExternalMatchResolvesUnknownPrincipalType(t *testing.T require.NoError(t, internalSyncer.Sync(ctx)) require.NoError(t, internalSyncer.Close(ctx)) - store, err := dotc1z.NewC1ZFile(ctx, internalC1zPath) + store, err := dotc1z.NewStore(ctx, internalC1zPath) require.NoError(t, err) defer func() { require.NoError(t, store.Close(ctx)) }() @@ -436,7 +436,7 @@ func TestFreshIngestFilterInsertResourceGrantsResourceInserts(t *testing.T) { require.NoError(t, syncer.Sync(ctx)) require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewC1ZFile(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath) require.NoError(t, err) defer func() { require.NoError(t, store.Close(ctx)) }() @@ -481,7 +481,7 @@ func TestInsertResourceGrantsRejectsReservedBatonIDResource(t *testing.T) { require.ErrorContains(t, syncer.Sync(ctx), "SDK-reserved BatonID ownership annotation") require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewC1ZFile(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath) require.NoError(t, err) defer func() { require.NoError(t, store.Close(ctx)) }() drives, err := store.ListResources(ctx, @@ -561,7 +561,7 @@ func TestFreshIngestFilterRunsBeforeExclusionGroupValidation(t *testing.T) { require.NoError(t, syncer.Sync(ctx)) require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewC1ZFile(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath) require.NoError(t, err) defer func() { require.NoError(t, store.Close(ctx)) }() @@ -797,7 +797,7 @@ func TestFreshIngestFilterHandlesMultiPageResourceTypes(t *testing.T) { require.NoError(t, syncer.Sync(ctx)) require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewC1ZFile(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath) require.NoError(t, err) defer func() { require.NoError(t, store.Close(ctx)) }() diff --git a/pkg/sync/syncer_cleanup_test.go b/pkg/sync/syncer_cleanup_test.go index 7291a6e29..8ad09cdf1 100644 --- a/pkg/sync/syncer_cleanup_test.go +++ b/pkg/sync/syncer_cleanup_test.go @@ -10,6 +10,7 @@ import ( reader_v2 "github.com/conductorone/baton-sdk/pb/c1/reader/v2" "github.com/conductorone/baton-sdk/pkg/dotc1z" + "github.com/conductorone/baton-sdk/pkg/dotc1z/c1zstore" "github.com/conductorone/baton-sdk/pkg/dotc1z/c1ztest" "github.com/conductorone/baton-sdk/pkg/logging" "github.com/stretchr/testify/require" @@ -29,7 +30,11 @@ func TestCleanupContextDeadlineExceeded(t *testing.T) { testFilePath := filepath.Join(tmpDir, "test.c1z") - f, err := dotc1z.NewStore(ctx, testFilePath) + // Pinned to SQLite: the test relies on the 200ms run budget expiring + // during cleanup of ~98 old syncs, which is only reliably slow on the + // row-by-row SQLite delete path. Pebble drops syncs via cheap range + // deletes, the sync completes, and ErrSyncNotComplete never fires. + f, err := dotc1z.NewStore(ctx, testFilePath, dotc1z.WithEngine(c1zstore.EngineSQLite)) require.NoError(t, err) // Create and end a bunch of syncs. We should delete all but 2 of them in Cleanup(). diff --git a/pkg/sync/syncer_test.go b/pkg/sync/syncer_test.go index 112053fcb..cabe01de3 100644 --- a/pkg/sync/syncer_test.go +++ b/pkg/sync/syncer_test.go @@ -119,7 +119,7 @@ func TestExpandGrants(t *testing.T) { require.NoError(t, err) // Validate that grants got expanded - store, err := dotc1z.NewC1ZFile(ctx, c1zpath) + store, err := dotc1z.NewStore(ctx, c1zpath) require.NoError(t, err) // Yes it's wasteful to load all grants into memory, but this connector doesn't make a ton of grants. @@ -317,7 +317,7 @@ func TestExpandGrantImmutable(t *testing.T) { err = syncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewC1ZFile(ctx, c1zpath) + store, err := dotc1z.NewStore(ctx, c1zpath) require.NoError(t, err) allGrantsReq := &v2.GrantsServiceListGrantsRequest{} @@ -422,7 +422,7 @@ func TestExpandGrantImmutableCycle(t *testing.T) { err = syncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewC1ZFile(ctx, c1zpath) + store, err := dotc1z.NewStore(ctx, c1zpath) require.NoError(t, err) allGrantsReq := &v2.GrantsServiceListGrantsRequest{} @@ -629,7 +629,7 @@ func TestExternalResourcePath(t *testing.T) { err = internalSyncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewC1ZFile(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath) require.NoError(t, err) resources, err := store.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ @@ -712,7 +712,7 @@ func TestPartialSync(t *testing.T) { err = partialSyncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewC1ZFile(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath) require.NoError(t, err) resourcesResp, err := store.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ @@ -778,7 +778,7 @@ func TestPartialSyncSkipEntitlementsAndGrants(t *testing.T) { err = syncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewC1ZFile(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath) require.NoError(t, err) resources, err := store.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ @@ -843,10 +843,14 @@ func TestPartialSyncUnimplemented(t *testing.T) { err = partialSyncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewC1ZFile(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath) require.NoError(t, err) - syncs, _, err := store.ListSyncRuns(ctx, "", 100) + lister, ok := store.(interface { + ListSyncRuns(context.Context, string, uint32) ([]*c1zstore.SyncRun, string, error) + }) + require.True(t, ok, "store must support ListSyncRuns") + syncs, _, err := lister.ListSyncRuns(ctx, "", 100) require.NoError(t, err) require.Equal(t, 1, len(syncs)) require.Equal(t, connectorstore.SyncTypePartial, syncs[0].Type) @@ -931,7 +935,7 @@ func TestExternalResourceMatchAll(t *testing.T) { require.NoError(t, err) // Verify grants were created for all external users - store, err := dotc1z.NewC1ZFile(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath) require.NoError(t, err) // Get grants for the internal group entitlement @@ -1021,7 +1025,7 @@ func TestExternalResourceMatchID(t *testing.T) { require.NoError(t, err) // Verify grant was created for the matching external user - store, err := dotc1z.NewC1ZFile(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath) require.NoError(t, err) allGrants, err := store.ListGrants(ctx, &v2.GrantsServiceListGrantsRequest{}) @@ -1121,7 +1125,7 @@ func TestExternalResourceMatchIDWithExpandableRemapping(t *testing.T) { // The remapping should produce an entitlement ID using the matched external // group's resource, which is the same as the original in this case but was // generated via entitlement.NewEntitlementID(matchedPrincipal, slug). - store, err := dotc1z.NewC1ZFile(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath) require.NoError(t, err) allGrants, err := store.ListGrants(ctx, &v2.GrantsServiceListGrantsRequest{}) @@ -1230,7 +1234,7 @@ func TestExternalResourceEmailMatch(t *testing.T) { require.NoError(t, err) // Verify grant was created for the matching external user - store, err := dotc1z.NewC1ZFile(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath) require.NoError(t, err) // Get grants for the internal group entitlement @@ -1322,7 +1326,7 @@ func TestExternalResourceGroupProfileMatch(t *testing.T) { require.NoError(t, err) // Verify grant was created for the matching external group - store, err := dotc1z.NewC1ZFile(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath) require.NoError(t, err) // Verify the external group was synced with correct properties @@ -1424,7 +1428,7 @@ func TestExternalResourceMatchAllAppTrait(t *testing.T) { require.NoError(t, internalSyncer.Sync(ctx)) require.NoError(t, internalSyncer.Close(ctx)) - store, err := dotc1z.NewC1ZFile(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath) require.NoError(t, err) grants, err := store.ListGrantsForEntitlement(ctx, reader_v2.GrantsReaderServiceListGrantsForEntitlementRequest_builder{ @@ -1496,7 +1500,7 @@ func TestExternalResourceAppTraitNotMatchedByDefault(t *testing.T) { require.NoError(t, internalSyncer.Sync(ctx)) require.NoError(t, internalSyncer.Close(ctx)) - store, err := dotc1z.NewC1ZFile(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath) require.NoError(t, err) grants, err := store.ListGrantsForEntitlement(ctx, reader_v2.GrantsReaderServiceListGrantsForEntitlementRequest_builder{ @@ -1564,7 +1568,7 @@ func TestExternalResourceWithGrantToEntitlement(t *testing.T) { require.NoError(t, err) // Verify external resources were synced via SyncExternalResourcesWithGrantToEntitlement - store, err := dotc1z.NewC1ZFile(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath) require.NoError(t, err) // SyncExternalResourcesWithGrantToEntitlement syncs resources based on grants to a specific entitlement. @@ -1620,11 +1624,11 @@ func TestExternalResourceWithGrantToEntitlement(t *testing.T) { syncID := resp.GetSyncs()[0].GetId() // Clone the finished sync to a new c1z clonedC1zpath := filepath.Join(tempDir, "cloned.c1z") - err = store.CloneSync(ctx, clonedC1zpath, syncID) + err = store.FileOps().CloneSync(ctx, clonedC1zpath, syncID) require.NoError(t, err) // Load the cloned c1z - clonedStore, err := dotc1z.NewC1ZFile(ctx, clonedC1zpath) + clonedStore, err := dotc1z.NewStore(ctx, clonedC1zpath) require.NoError(t, err) clonedSyncs, err := clonedStore.ListSyncs(ctx, reader_v2.SyncsReaderServiceListSyncsRequest_builder{ PageSize: 2, @@ -1719,7 +1723,7 @@ func TestResumeSyncWithChildResources(t *testing.T) { require.NoError(t, err) // Verify that parent_1 was synced before the failure. - store1, err := dotc1z.NewC1ZFile(ctx, c1zPath) + store1, err := dotc1z.NewStore(ctx, c1zPath) require.NoError(t, err) parentResources, err := store1.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ @@ -1752,7 +1756,7 @@ func TestResumeSyncWithChildResources(t *testing.T) { require.NoError(t, err) // Verify that child resources are now synced. - store2, err := dotc1z.NewC1ZFile(ctx, c1zPath) + store2, err := dotc1z.NewStore(ctx, c1zPath) require.NoError(t, err) childResourcesAfterResume, err := store2.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ @@ -2113,7 +2117,7 @@ func TestSyncGrants_PropagatesInsertResourceGrantsAnnotation(t *testing.T) { require.NoError(t, syncer.Sync(ctx)) require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewC1ZFile(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath) require.NoError(t, err) resp, err := store.ListGrants(ctx, v2.GrantsServiceListGrantsRequest_builder{}.Build()) @@ -2154,7 +2158,7 @@ func TestSyncGrants_DoesNotPropagateAnnotationWhenAbsent(t *testing.T) { require.NoError(t, syncer.Sync(ctx)) require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewC1ZFile(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath) require.NoError(t, err) resp, err := store.ListGrants(ctx, v2.GrantsServiceListGrantsRequest_builder{}.Build()) From c6633ecc262de6791cdac3c13728d4da99ba7795 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Fri, 14 Aug 2026 10:53:47 -0600 Subject: [PATCH 08/15] Surface diff-syncs' sqlite requirement and cover the pebble cleanup path Flipping the default engine to pebble made diff-syncs structurally impossible against default-engine artifacts: a v3 c1z holds a single sync, so a base and an applied sync can never coexist in one file. The local differ now wraps ErrDiffUnsupported with remediation (re-run the syncs with --storage-engine sqlite), the diff-syncs flag documents the requirement, and tests pin both the rejection and the surviving v1 diff path. Also adds the default-engine sibling of the sqlite-pinned cleanup deadline test, asserting pebble's replacement-based lifecycle: the run completes within budget, exactly one sealed sync remains, and the artifact is v3 on disk. Co-authored-by: Cursor --- pkg/field/defaults.go | 3 +- pkg/sync/syncer_cleanup_test.go | 77 +++++++++++++++++++++++++++++ pkg/tasks/local/differ.go | 15 ++++++ pkg/tasks/local/differ_test.go | 87 +++++++++++++++++++++++++++++++++ 4 files changed, 181 insertions(+), 1 deletion(-) create mode 100644 pkg/tasks/local/differ_test.go diff --git a/pkg/field/defaults.go b/pkg/field/defaults.go index 4dc0bd192..f34390643 100644 --- a/pkg/field/defaults.go +++ b/pkg/field/defaults.go @@ -138,7 +138,8 @@ var ( WithExportTarget(ExportTargetNone)) diffSyncsField = BoolField( "diff-syncs", - WithDescription("Create a new partial SyncID from a base and applied sync."), + WithDescription("Create a new partial SyncID from a base and applied sync. "+ + "Requires a sqlite-engine (v1) c1z: pebble (v3) files hold a single sync, so run the syncs with --storage-engine sqlite."), WithHidden(true), WithPersistent(true), WithExportTarget(ExportTargetNone), diff --git a/pkg/sync/syncer_cleanup_test.go b/pkg/sync/syncer_cleanup_test.go index 8ad09cdf1..b835b6e7e 100644 --- a/pkg/sync/syncer_cleanup_test.go +++ b/pkg/sync/syncer_cleanup_test.go @@ -4,6 +4,7 @@ package sync import ( + "os" "path/filepath" "testing" "time" @@ -91,3 +92,79 @@ func TestCleanupContextDeadlineExceeded(t *testing.T) { err = syncer.Close(ctx) require.NoError(t, err) } + +// TestCleanupPebbleSingleSyncLifecycle is the default-engine sibling of +// TestCleanupContextDeadlineExceeded (which is pinned to SQLite because +// its deadline assertion depends on SQLite's slow row-by-row deletes). +// On pebble — now the default — end-of-sync cleanup is a no-op by +// design: each StartNewSync replaces the prior sync in place +// (single-sync contract), so a backlog of old syncs can never +// accumulate and the run budget cannot expire during cleanup. The +// pebble analog of "old syncs dropped within budget" is therefore: +// the syncer run completes well inside a budget, the seeded sync was +// replaced rather than retained, exactly one sealed sync remains, and +// the artifact is v3 on disk. +func TestCleanupPebbleSingleSyncLifecycle(t *testing.T) { + ctx := t.Context() + tmpDir := t.TempDir() + + ctx, err := logging.Init( + ctx, + logging.WithLogFormat(logging.LogFormatConsole), + logging.WithLogLevel("debug"), + ) + require.NoError(t, err) + + testFilePath := filepath.Join(tmpDir, "test.c1z") + + // Engine-less NewStore: the default (pebble) path under test. + f, err := dotc1z.NewStore(ctx, testFilePath) + require.NoError(t, err) + + // Seed synced data the same way the SQLite test does. Under the + // single-sync contract each iteration replaces the previous sync, so + // unlike SQLite this can never build up a cleanup backlog. + seededSyncID := "" + for range 3 { + seededSyncID, err = c1ztest.CreateTestSync(ctx, t, f, c1ztest.C1ZCounts{ + ResourceTypeCount: 10, + ResourceCount: 100, + UserCount: 100, + EntitlementCount: 10, + GrantCount: 250, + }) + require.NoError(t, err) + } + + // The budget is intentionally generous: with no-op cleanup the only + // thing that could burn it is the sync itself, and a tight bound + // would test machine speed, not engine behavior. What matters is + // that ErrSyncNotComplete — the SQLite test's expected outcome — + // cannot happen here. + syncer, err := NewSyncer(ctx, newMockConnector(), WithRunDuration(30*time.Second), WithConnectorStore(f)) + require.NoError(t, err) + err = syncer.Sync(ctx) + require.NoError(t, err, "pebble sync must complete within budget; cleanup is a no-op") + + // Exactly one sync remains, it is sealed, and it is the syncer's run, + // not the seeded one (replacement, the pebble analog of cleanup). + syncsResp, err := f.ListSyncs(ctx, reader_v2.SyncsReaderServiceListSyncsRequest_builder{ + PageSize: 100, + }.Build()) + require.NoError(t, err) + syncs := syncsResp.GetSyncs() + require.Len(t, syncs, 1, "single-sync contract: exactly one sync after the run") + require.NotNil(t, syncs[0].GetEndedAt(), "the remaining sync must be sealed") + require.NotEqual(t, seededSyncID, syncs[0].GetId(), "the seeded sync must have been replaced") + + err = syncer.Close(ctx) + require.NoError(t, err) + + // The default-engine artifact must be v3 on disk. + file, err := os.Open(testFilePath) + require.NoError(t, err) + defer file.Close() + format, err := dotc1z.ReadHeaderFormat(file) + require.NoError(t, err) + require.Equal(t, dotc1z.C1ZFormatV3, format, "default-engine artifact must be a v3 c1z on disk") +} diff --git a/pkg/tasks/local/differ.go b/pkg/tasks/local/differ.go index c1d76e937..8f816908b 100644 --- a/pkg/tasks/local/differ.go +++ b/pkg/tasks/local/differ.go @@ -3,11 +3,13 @@ package local import ( "context" "errors" + "fmt" "sync" "time" v1 "github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1" "github.com/conductorone/baton-sdk/pkg/dotc1z" + pebbleengine "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble" "github.com/conductorone/baton-sdk/pkg/tasks" "github.com/conductorone/baton-sdk/pkg/types" "github.com/conductorone/baton-sdk/pkg/uotel" @@ -61,6 +63,19 @@ func (m *localDiffer) Process(ctx context.Context, task *v1.Task, cc types.Conne newSyncID, err := file.FileOps().GenerateSyncDiff(ctx, m.baseSyncID, m.appliedSyncID) if err != nil { + if closeErr := file.Close(ctx); closeErr != nil { + log.Error("failed to close store after diff error", zap.Error(closeErr)) + } + if errors.Is(err, pebbleengine.ErrDiffUnsupported) { + // Structural, not a missing feature: a pebble (v3) c1z holds + // exactly one sync, so the base and applied syncs GenerateSyncDiff + // needs can never coexist in it. Point the operator at the + // engine that supports the workflow. + return fmt.Errorf( + "diff-syncs requires a sqlite-engine (v1) c1z: %s uses the pebble (v3) engine, which holds a single sync, "+ + "so a base and an applied sync cannot coexist in it; re-run the syncs with --storage-engine sqlite to use diff-syncs: %w", + m.dbPath, err) + } return err } diff --git a/pkg/tasks/local/differ_test.go b/pkg/tasks/local/differ_test.go new file mode 100644 index 000000000..b50747125 --- /dev/null +++ b/pkg/tasks/local/differ_test.go @@ -0,0 +1,87 @@ +package local + +import ( + "context" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/connectorstore" + "github.com/conductorone/baton-sdk/pkg/dotc1z" + pebbleengine "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble" +) + +// TestDifferRejectsPebbleArtifactWithGuidance pins the behavior change +// from flipping the default engine to pebble: a default-engine (v3) +// artifact holds a single sync, so the diff-syncs workflow is +// structurally impossible against it. The differ must surface +// ErrDiffUnsupported wrapped with remediation (use --storage-engine +// sqlite) rather than a bare engine error. +func TestDifferRejectsPebbleArtifactWithGuidance(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + path := filepath.Join(dir, "pebble.c1z") + + // Engine-less NewStore is the exact path connector syncs take; with + // the pebble default this produces a v3 artifact. + store, err := dotc1z.NewStore(ctx, path, dotc1z.WithTmpDir(dir)) + require.NoError(t, err) + syncID, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.NoError(t, store.EndSync(ctx)) + require.NoError(t, store.Close(ctx)) + + mgr := NewDiffer(ctx, path, syncID, syncID) + err = mgr.Process(ctx, nil, nil) + require.Error(t, err) + require.ErrorIs(t, err, pebbleengine.ErrDiffUnsupported) + require.Contains(t, err.Error(), "--storage-engine sqlite", + "differ error must tell the operator how to get a diffable artifact") +} + +// TestDifferSucceedsOnSQLiteArtifact guards the surviving diff-syncs +// path: a v1 (sqlite) c1z with two ended syncs still diffs after the +// default-engine flip, because the differ's engine-less open dispatches +// on the on-disk magic byte rather than the new pebble default. +func TestDifferSucceedsOnSQLiteArtifact(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + path := filepath.Join(dir, "sqlite.c1z") + + store, err := dotc1z.NewC1ZFile(ctx, path) + require.NoError(t, err) + + putResource := func(id string) { + t.Helper() + require.NoError(t, store.PutResources(ctx, v2.Resource_builder{ + Id: v2.ResourceId_builder{ResourceType: "user", Resource: id}.Build(), + }.Build())) + } + + baseSyncID, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.NoError(t, store.PutResourceTypes(ctx, v2.ResourceType_builder{Id: "user"}.Build())) + putResource("alice") + require.NoError(t, store.EndSync(ctx)) + + appliedSyncID, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") + require.NoError(t, err) + require.NoError(t, store.PutResourceTypes(ctx, v2.ResourceType_builder{Id: "user"}.Build())) + putResource("alice") + putResource("bob") + require.NoError(t, store.EndSync(ctx)) + require.NoError(t, store.Close(ctx)) + + mgr := NewDiffer(ctx, path, baseSyncID, appliedSyncID) + require.NoError(t, mgr.Process(ctx, nil, nil)) + + // The diff sync must have landed next to the two originals. + reopened, err := dotc1z.NewC1ZFile(ctx, path) + require.NoError(t, err) + defer func() { _ = reopened.Close(ctx) }() + runs, _, err := reopened.ListSyncRuns(ctx, "", 100) + require.NoError(t, err) + require.Len(t, runs, 3, "expected base + applied + generated diff sync") +} From 715a499c08df266f807d695e7b9e435641364dd0 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Fri, 14 Aug 2026 11:28:23 -0600 Subject: [PATCH 09/15] Open CXP-498's new test artifact with the engine-neutral NewStore TestExternalResourceUserProfileMatch (merged from main, #1046) opened its freshly synced c1z with the sqlite-only NewC1ZFile constructor, which rejects the v3 artifact the pebble default now produces. Same class and fix as the rest of the pkg/sync sweep in ee6683a7. Co-authored-by: Cursor --- pkg/sync/syncer_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/sync/syncer_test.go b/pkg/sync/syncer_test.go index 7e3461e17..fb9893164 100644 --- a/pkg/sync/syncer_test.go +++ b/pkg/sync/syncer_test.go @@ -1338,7 +1338,9 @@ func TestExternalResourceUserProfileMatch(t *testing.T) { err = internalSyncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewC1ZFile(ctx, internalC1zpath) + // Engine-neutral open: with the pebble default the synced artifact is + // v3, and the sqlite-only NewC1ZFile constructor would reject it. + store, err := dotc1z.NewStore(ctx, internalC1zpath) require.NoError(t, err) grants, err := store.ListGrantsForEntitlement(ctx, reader_v2.GrantsReaderServiceListGrantsForEntitlementRequest_builder{ From fe593a84d64ccda6a97353af9fd30dc06a16a35f Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Fri, 14 Aug 2026 11:31:25 -0600 Subject: [PATCH 10/15] Drop the diff-syncs remediation gate; the surface has no live callers GenerateSyncDiff's only production caller is the local differ, reached solely through the hidden diff-syncs flag, and RFC 0002's call-site audit records no live callers. Guidance text, flag docs, and tests for a workflow nobody runs are noise, so restore the differ and flag description to their prior state. Pebble's ErrDiffUnsupported still propagates as-is if the dead path is ever exercised. Co-authored-by: Cursor --- pkg/field/defaults.go | 3 +- pkg/tasks/local/differ.go | 15 ------ pkg/tasks/local/differ_test.go | 87 ---------------------------------- 3 files changed, 1 insertion(+), 104 deletions(-) delete mode 100644 pkg/tasks/local/differ_test.go diff --git a/pkg/field/defaults.go b/pkg/field/defaults.go index f34390643..4dc0bd192 100644 --- a/pkg/field/defaults.go +++ b/pkg/field/defaults.go @@ -138,8 +138,7 @@ var ( WithExportTarget(ExportTargetNone)) diffSyncsField = BoolField( "diff-syncs", - WithDescription("Create a new partial SyncID from a base and applied sync. "+ - "Requires a sqlite-engine (v1) c1z: pebble (v3) files hold a single sync, so run the syncs with --storage-engine sqlite."), + WithDescription("Create a new partial SyncID from a base and applied sync."), WithHidden(true), WithPersistent(true), WithExportTarget(ExportTargetNone), diff --git a/pkg/tasks/local/differ.go b/pkg/tasks/local/differ.go index 8f816908b..c1d76e937 100644 --- a/pkg/tasks/local/differ.go +++ b/pkg/tasks/local/differ.go @@ -3,13 +3,11 @@ package local import ( "context" "errors" - "fmt" "sync" "time" v1 "github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1" "github.com/conductorone/baton-sdk/pkg/dotc1z" - pebbleengine "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble" "github.com/conductorone/baton-sdk/pkg/tasks" "github.com/conductorone/baton-sdk/pkg/types" "github.com/conductorone/baton-sdk/pkg/uotel" @@ -63,19 +61,6 @@ func (m *localDiffer) Process(ctx context.Context, task *v1.Task, cc types.Conne newSyncID, err := file.FileOps().GenerateSyncDiff(ctx, m.baseSyncID, m.appliedSyncID) if err != nil { - if closeErr := file.Close(ctx); closeErr != nil { - log.Error("failed to close store after diff error", zap.Error(closeErr)) - } - if errors.Is(err, pebbleengine.ErrDiffUnsupported) { - // Structural, not a missing feature: a pebble (v3) c1z holds - // exactly one sync, so the base and applied syncs GenerateSyncDiff - // needs can never coexist in it. Point the operator at the - // engine that supports the workflow. - return fmt.Errorf( - "diff-syncs requires a sqlite-engine (v1) c1z: %s uses the pebble (v3) engine, which holds a single sync, "+ - "so a base and an applied sync cannot coexist in it; re-run the syncs with --storage-engine sqlite to use diff-syncs: %w", - m.dbPath, err) - } return err } diff --git a/pkg/tasks/local/differ_test.go b/pkg/tasks/local/differ_test.go deleted file mode 100644 index b50747125..000000000 --- a/pkg/tasks/local/differ_test.go +++ /dev/null @@ -1,87 +0,0 @@ -package local - -import ( - "context" - "path/filepath" - "testing" - - "github.com/stretchr/testify/require" - - v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" - "github.com/conductorone/baton-sdk/pkg/connectorstore" - "github.com/conductorone/baton-sdk/pkg/dotc1z" - pebbleengine "github.com/conductorone/baton-sdk/pkg/dotc1z/engine/pebble" -) - -// TestDifferRejectsPebbleArtifactWithGuidance pins the behavior change -// from flipping the default engine to pebble: a default-engine (v3) -// artifact holds a single sync, so the diff-syncs workflow is -// structurally impossible against it. The differ must surface -// ErrDiffUnsupported wrapped with remediation (use --storage-engine -// sqlite) rather than a bare engine error. -func TestDifferRejectsPebbleArtifactWithGuidance(t *testing.T) { - ctx := context.Background() - dir := t.TempDir() - path := filepath.Join(dir, "pebble.c1z") - - // Engine-less NewStore is the exact path connector syncs take; with - // the pebble default this produces a v3 artifact. - store, err := dotc1z.NewStore(ctx, path, dotc1z.WithTmpDir(dir)) - require.NoError(t, err) - syncID, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") - require.NoError(t, err) - require.NoError(t, store.EndSync(ctx)) - require.NoError(t, store.Close(ctx)) - - mgr := NewDiffer(ctx, path, syncID, syncID) - err = mgr.Process(ctx, nil, nil) - require.Error(t, err) - require.ErrorIs(t, err, pebbleengine.ErrDiffUnsupported) - require.Contains(t, err.Error(), "--storage-engine sqlite", - "differ error must tell the operator how to get a diffable artifact") -} - -// TestDifferSucceedsOnSQLiteArtifact guards the surviving diff-syncs -// path: a v1 (sqlite) c1z with two ended syncs still diffs after the -// default-engine flip, because the differ's engine-less open dispatches -// on the on-disk magic byte rather than the new pebble default. -func TestDifferSucceedsOnSQLiteArtifact(t *testing.T) { - ctx := context.Background() - dir := t.TempDir() - path := filepath.Join(dir, "sqlite.c1z") - - store, err := dotc1z.NewC1ZFile(ctx, path) - require.NoError(t, err) - - putResource := func(id string) { - t.Helper() - require.NoError(t, store.PutResources(ctx, v2.Resource_builder{ - Id: v2.ResourceId_builder{ResourceType: "user", Resource: id}.Build(), - }.Build())) - } - - baseSyncID, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") - require.NoError(t, err) - require.NoError(t, store.PutResourceTypes(ctx, v2.ResourceType_builder{Id: "user"}.Build())) - putResource("alice") - require.NoError(t, store.EndSync(ctx)) - - appliedSyncID, err := store.StartNewSync(ctx, connectorstore.SyncTypeFull, "") - require.NoError(t, err) - require.NoError(t, store.PutResourceTypes(ctx, v2.ResourceType_builder{Id: "user"}.Build())) - putResource("alice") - putResource("bob") - require.NoError(t, store.EndSync(ctx)) - require.NoError(t, store.Close(ctx)) - - mgr := NewDiffer(ctx, path, baseSyncID, appliedSyncID) - require.NoError(t, mgr.Process(ctx, nil, nil)) - - // The diff sync must have landed next to the two originals. - reopened, err := dotc1z.NewC1ZFile(ctx, path) - require.NoError(t, err) - defer func() { _ = reopened.Close(ctx) }() - runs, _, err := reopened.ListSyncRuns(ctx, "", 100) - require.NoError(t, err) - require.Len(t, runs, 3, "expected base + applied + generated diff sync") -} From 3446fe8469ace3b86c0d7ac50e57aebebf3ca945 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Fri, 14 Aug 2026 13:41:37 -0600 Subject: [PATCH 11/15] Align stale default-engine comments and fix a test store leak Review sweep after the pebble default flip: scope WithEngine's and the Engine constants' "default is pebble" docs to NewStore (NewC1ZFile is SQLite-only and normalizes unset to sqlite), correct the c1zOptions field comment that still claimed a SQLite default, reword sanitize's fallback comment now that SQLite there is an explicit pin, widen the RFC 0001 amendment to name every superseded SQLite-default statement, and guard TestNewStoreDefaultsToPebbleDriver so a mid-test failure cannot leak the open pebble store for the rest of the test binary. Co-authored-by: Cursor --- cmd/baton/sanitize.go | 4 +++- docs/rfcs/0001-pebble-storage-engine.md | 8 +++++--- pkg/dotc1z/c1file.go | 10 +++++++--- pkg/dotc1z/c1zstore/engine.go | 8 +++++--- pkg/dotc1z/engine_registry_test.go | 10 ++++++++++ 5 files changed, 30 insertions(+), 10 deletions(-) diff --git a/cmd/baton/sanitize.go b/cmd/baton/sanitize.go index 7a2725091..79cf41196 100644 --- a/cmd/baton/sanitize.go +++ b/cmd/baton/sanitize.go @@ -111,7 +111,9 @@ func runSanitize(cmd *cobra.Command, args []string) error { // Default the output engine to the source's so `sanitize` round-trips // the engine unless the operator asks otherwise. An empty source engine - // (virtual/unknown store) falls back to the SQLite default. + // (virtual/unknown store) is pinned to SQLite explicitly — this + // predates the Pebble default flip and is no longer "the default", + // but the behavior is deliberately unchanged. dstEngine := c1zstore.Engine(outEngineRaw) if outEngineRaw == "" { dstEngine = c1zstore.Engine(src.Metadata().Engine) diff --git a/docs/rfcs/0001-pebble-storage-engine.md b/docs/rfcs/0001-pebble-storage-engine.md index 78ff1bf2e..1f8fec51f 100644 --- a/docs/rfcs/0001-pebble-storage-engine.md +++ b/docs/rfcs/0001-pebble-storage-engine.md @@ -10,9 +10,11 @@ > SQLite was the default engine and Pebble was opt-in; the SDK default is now > Pebble for NEWLY CREATED c1z files (`kans/pebble-default-engine`). Existing > files still dispatch on their on-disk magic byte, and only an explicit -> `WithEngine(EnginePebble)` converts a v1 file in place. Statements below -> that describe SQLite as the connector default (Summary, §5.3/§5.4) reflect -> the original rollout posture and are retained for historical context. +> `WithEngine(EnginePebble)` converts a v1 file in place. Every statement +> below that describes SQLite as the default engine — including the Summary, +> §2's closing rationale, §3 Goals 2–3, and §5.3/§5.4 (including §5.4's +> "writing v1 also stays the default") — reflects the original rollout +> posture and is retained for historical context. ## 1. Summary diff --git a/pkg/dotc1z/c1file.go b/pkg/dotc1z/c1file.go index f060e335c..9ad1d8d07 100644 --- a/pkg/dotc1z/c1file.go +++ b/pkg/dotc1z/c1file.go @@ -348,7 +348,9 @@ type c1zOptions struct { disableGrantDigestIndex bool // engine is the storage engine to use for newly created files. - // Reads dispatch on magic byte regardless. Default EngineSQLite. + // Reads dispatch on magic byte regardless. NewStore defaults an + // unset engine to EnginePebble; NewC1ZFile (SQLite-only) normalizes + // unset to EngineSQLite. engine c1zstore.Engine // payloadEncoding controls the v3 envelope payload framing. Only @@ -433,8 +435,10 @@ func WithSyncLimit(limit int) C1ZOption { } // WithEngine selects the storage engine for newly created .c1z files. -// Default is EnginePebble (v3 format). EngineSQLite selects the legacy -// v1 engine. +// Under NewStore the default is EnginePebble (v3 format); EngineSQLite +// selects the legacy v1 engine. NewC1ZFile does not share that default: +// it is the SQLite-only constructor, treats an unset engine as +// EngineSQLite, and rejects a writable EnginePebble request. // // Reading existing files dispatches on the file's magic byte and is // independent of this option. diff --git a/pkg/dotc1z/c1zstore/engine.go b/pkg/dotc1z/c1zstore/engine.go index d6e31ff2b..9fb8a684e 100644 --- a/pkg/dotc1z/c1zstore/engine.go +++ b/pkg/dotc1z/c1zstore/engine.go @@ -15,11 +15,13 @@ type Engine string const ( // EngineSQLite is the legacy v1 engine: the v1 .c1z format backed by // a zstd-compressed SQLite database. Callers opt into it via - // WithEngine; the default engine is EnginePebble. + // WithEngine; the NewStore default is EnginePebble. (The SQLite-only + // NewC1ZFile constructor is the exception: it treats an unset engine + // as EngineSQLite.) EngineSQLite Engine = "sqlite" - // EnginePebble is the v3 engine and the default when callers do not - // specify one: a Pebble LSM wrapped in the v3 + // EnginePebble is the v3 engine and the NewStore default when + // callers do not specify one: a Pebble LSM wrapped in the v3 // envelope. This is the in-process identity AND the value callers // select with (the --storage-engine flag and the gRPC sync-task // field both pass "pebble"); it must stay "pebble" for those diff --git a/pkg/dotc1z/engine_registry_test.go b/pkg/dotc1z/engine_registry_test.go index c47fe67a1..2e28611ed 100644 --- a/pkg/dotc1z/engine_registry_test.go +++ b/pkg/dotc1z/engine_registry_test.go @@ -69,6 +69,15 @@ func TestNewStoreDefaultsToPebbleDriver(t *testing.T) { path := filepath.Join(t.TempDir(), "default.c1z") store, err := NewStore(ctx, path) require.NoError(t, err) + // The happy path closes explicitly below (the close must succeed for + // the on-disk header check); this guard keeps a mid-test require + // failure from leaking the store for the rest of the test binary. + storeClosed := false + defer func() { + if !storeClosed { + _ = store.Close(ctx) + } + }() _, ok := store.(*C1File) require.False(t, ok, "NewStore default type = %T, want the pebble store, not *C1File", store) require.Equal(t, string(c1zstore.EnginePebble), store.Metadata().Engine) @@ -80,6 +89,7 @@ func TestNewStoreDefaultsToPebbleDriver(t *testing.T) { require.NoError(t, err) require.NoError(t, store.EndSync(ctx)) require.NoError(t, store.Close(ctx)) + storeClosed = true f, err := os.Open(path) require.NoError(t, err) From bf3b7adc0ba816e08e42b2883fe522bc8ea93701 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Fri, 14 Aug 2026 15:07:46 -0600 Subject: [PATCH 12/15] Open syncer_test verification stores read-only and close them The NewC1ZFile-to-NewStore sweep left 19 of 21 post-sync verification opens unclosed, which under the pebble default leaks an unpacked temp DB, open fds, and background compaction goroutines for the rest of the test binary. These opens only read, so request read-only and close them. TestResumeSyncWithChildResources gains the most: its mid-resume inspection can no longer write to the artifact syncer2 resumes from. Also note that the empty-engine fallback in storeOptionsFromC1ZOptions is defensive only; NewStore overwrites it with the selected driver's engine on the next line. Co-authored-by: Cursor --- pkg/dotc1z/engine_registry.go | 3 ++ pkg/sync/syncer_test.go | 65 +++++++++++++++++++++++------------ 2 files changed, 46 insertions(+), 22 deletions(-) diff --git a/pkg/dotc1z/engine_registry.go b/pkg/dotc1z/engine_registry.go index 39b37e25d..180176b92 100644 --- a/pkg/dotc1z/engine_registry.go +++ b/pkg/dotc1z/engine_registry.go @@ -203,6 +203,9 @@ func storeOptionsFromC1ZOptions(options *c1zOptions) StoreOptions { MaxDecodedPayloadBytes: maxDecodedPayloadBytes, MaxDecoderMemoryBytes: maxDecoderMemoryBytes, } + // Defensive only: NewStore, the sole caller, overwrites Engine with + // the selected driver's engine on the next line. Real default + // selection lives in selectStoreDriver. if out.Engine == "" { out.Engine = c1zstore.EnginePebble } diff --git a/pkg/sync/syncer_test.go b/pkg/sync/syncer_test.go index fb9893164..d29ba89ea 100644 --- a/pkg/sync/syncer_test.go +++ b/pkg/sync/syncer_test.go @@ -119,8 +119,9 @@ func TestExpandGrants(t *testing.T) { require.NoError(t, err) // Validate that grants got expanded - store, err := dotc1z.NewStore(ctx, c1zpath) + store, err := dotc1z.NewStore(ctx, c1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() // Yes it's wasteful to load all grants into memory, but this connector doesn't make a ton of grants. allGrants := make([]*v2.Grant, 0) @@ -317,8 +318,9 @@ func TestExpandGrantImmutable(t *testing.T) { err = syncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewStore(ctx, c1zpath) + store, err := dotc1z.NewStore(ctx, c1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() allGrantsReq := &v2.GrantsServiceListGrantsRequest{} allGrants, err := store.ListGrants(ctx, allGrantsReq) @@ -422,8 +424,9 @@ func TestExpandGrantImmutableCycle(t *testing.T) { err = syncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewStore(ctx, c1zpath) + store, err := dotc1z.NewStore(ctx, c1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() allGrantsReq := &v2.GrantsServiceListGrantsRequest{} allGrants, err := store.ListGrants(ctx, allGrantsReq) @@ -629,8 +632,9 @@ func TestExternalResourcePath(t *testing.T) { err = internalSyncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewStore(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() resources, err := store.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ ResourceTypeId: userResourceType.GetId(), @@ -712,8 +716,9 @@ func TestPartialSync(t *testing.T) { err = partialSyncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewStore(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() resourcesResp, err := store.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ ResourceTypeId: userResourceType.GetId(), @@ -778,8 +783,9 @@ func TestPartialSyncSkipEntitlementsAndGrants(t *testing.T) { err = syncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewStore(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() resources, err := store.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ ResourceTypeId: groupResourceType.GetId(), @@ -843,8 +849,9 @@ func TestPartialSyncUnimplemented(t *testing.T) { err = partialSyncer.Close(ctx) require.NoError(t, err) - store, err := dotc1z.NewStore(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() lister, ok := store.(interface { ListSyncRuns(context.Context, string, uint32) ([]*c1zstore.SyncRun, string, error) @@ -935,8 +942,9 @@ func TestExternalResourceMatchAll(t *testing.T) { require.NoError(t, err) // Verify grants were created for all external users - store, err := dotc1z.NewStore(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() // Get grants for the internal group entitlement grants, err := store.ListGrantsForEntitlement(ctx, reader_v2.GrantsReaderServiceListGrantsForEntitlementRequest_builder{ @@ -1025,8 +1033,9 @@ func TestExternalResourceMatchID(t *testing.T) { require.NoError(t, err) // Verify grant was created for the matching external user - store, err := dotc1z.NewStore(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() allGrants, err := store.ListGrants(ctx, &v2.GrantsServiceListGrantsRequest{}) require.NoError(t, err) @@ -1125,8 +1134,9 @@ func TestExternalResourceMatchIDWithExpandableRemapping(t *testing.T) { // The remapping should produce an entitlement ID using the matched external // group's resource, which is the same as the original in this case but was // generated via entitlement.NewEntitlementID(matchedPrincipal, slug). - store, err := dotc1z.NewStore(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() allGrants, err := store.ListGrants(ctx, &v2.GrantsServiceListGrantsRequest{}) require.NoError(t, err) @@ -1234,8 +1244,9 @@ func TestExternalResourceEmailMatch(t *testing.T) { require.NoError(t, err) // Verify grant was created for the matching external user - store, err := dotc1z.NewStore(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() // Get grants for the internal group entitlement grants, err := store.ListGrantsForEntitlement(ctx, reader_v2.GrantsReaderServiceListGrantsForEntitlementRequest_builder{ @@ -1340,8 +1351,9 @@ func TestExternalResourceUserProfileMatch(t *testing.T) { // Engine-neutral open: with the pebble default the synced artifact is // v3, and the sqlite-only NewC1ZFile constructor would reject it. - store, err := dotc1z.NewStore(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() grants, err := store.ListGrantsForEntitlement(ctx, reader_v2.GrantsReaderServiceListGrantsForEntitlementRequest_builder{ Entitlement: internalGroupEnt, @@ -1430,8 +1442,9 @@ func TestExternalResourceGroupProfileMatch(t *testing.T) { require.NoError(t, err) // Verify grant was created for the matching external group - store, err := dotc1z.NewStore(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() // Verify the external group was synced with correct properties groupResources, err := store.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ @@ -1536,8 +1549,9 @@ func TestExternalResourceMatchAllAppTrait(t *testing.T) { require.NoError(t, internalSyncer.Sync(ctx)) require.NoError(t, internalSyncer.Close(ctx)) - store, err := dotc1z.NewStore(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() grants, err := store.ListGrantsForEntitlement(ctx, reader_v2.GrantsReaderServiceListGrantsForEntitlementRequest_builder{ Entitlement: internalGroupEnt, @@ -1608,8 +1622,9 @@ func TestExternalResourceAppTraitNotMatchedByDefault(t *testing.T) { require.NoError(t, internalSyncer.Sync(ctx)) require.NoError(t, internalSyncer.Close(ctx)) - store, err := dotc1z.NewStore(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() grants, err := store.ListGrantsForEntitlement(ctx, reader_v2.GrantsReaderServiceListGrantsForEntitlementRequest_builder{ Entitlement: internalGroupEnt, @@ -1676,8 +1691,9 @@ func TestExternalResourceWithGrantToEntitlement(t *testing.T) { require.NoError(t, err) // Verify external resources were synced via SyncExternalResourcesWithGrantToEntitlement - store, err := dotc1z.NewStore(ctx, internalC1zpath) + store, err := dotc1z.NewStore(ctx, internalC1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() // SyncExternalResourcesWithGrantToEntitlement syncs resources based on grants to a specific entitlement. // It: @@ -1736,7 +1752,7 @@ func TestExternalResourceWithGrantToEntitlement(t *testing.T) { require.NoError(t, err) // Load the cloned c1z - clonedStore, err := dotc1z.NewStore(ctx, clonedC1zpath) + clonedStore, err := dotc1z.NewStore(ctx, clonedC1zpath, dotc1z.WithReadOnly(true)) require.NoError(t, err) clonedSyncs, err := clonedStore.ListSyncs(ctx, reader_v2.SyncsReaderServiceListSyncsRequest_builder{ PageSize: 2, @@ -1830,8 +1846,10 @@ func TestResumeSyncWithChildResources(t *testing.T) { err = syncer1.Close(ctxToCancel) require.NoError(t, err) - // Verify that parent_1 was synced before the failure. - store1, err := dotc1z.NewStore(ctx, c1zPath) + // Verify that parent_1 was synced before the failure. Read-only so + // inspecting the mid-resume artifact cannot write to it before + // syncer2 resumes from the same checkpoint. + store1, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) parentResources, err := store1.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ @@ -1864,8 +1882,9 @@ func TestResumeSyncWithChildResources(t *testing.T) { require.NoError(t, err) // Verify that child resources are now synced. - store2, err := dotc1z.NewStore(ctx, c1zPath) + store2, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store2.Close(ctx) }() childResourcesAfterResume, err := store2.ListResources(ctx, v2.ResourcesServiceListResourcesRequest_builder{ ResourceTypeId: childResourceType.GetId(), @@ -2225,8 +2244,9 @@ func TestSyncGrants_PropagatesInsertResourceGrantsAnnotation(t *testing.T) { require.NoError(t, syncer.Sync(ctx)) require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewStore(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() resp, err := store.ListGrants(ctx, v2.GrantsServiceListGrantsRequest_builder{}.Build()) require.NoError(t, err) @@ -2266,8 +2286,9 @@ func TestSyncGrants_DoesNotPropagateAnnotationWhenAbsent(t *testing.T) { require.NoError(t, syncer.Sync(ctx)) require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewStore(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) + defer func() { _ = store.Close(ctx) }() resp, err := store.ListGrants(ctx, v2.GrantsServiceListGrantsRequest_builder{}.Build()) require.NoError(t, err) From 9c5b02a8e23c322aeedd0339fd5a295c840e75d1 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Tue, 18 Aug 2026 15:09:04 -0600 Subject: [PATCH 13/15] Pin the full-sync bench sqlite arm now that NewStore defaults to Pebble WithC1ZPath with no engine follows the NewStore default, so the named sqlite comparison was running Pebble on both sides after the default flip. Co-authored-by: Cursor --- pkg/sync/full_sync_bench_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/sync/full_sync_bench_test.go b/pkg/sync/full_sync_bench_test.go index 475a30f64..7581649a7 100644 --- a/pkg/sync/full_sync_bench_test.go +++ b/pkg/sync/full_sync_bench_test.go @@ -16,8 +16,10 @@ import ( // BenchmarkFullSync_BatonDemoShape drives pkg/sync.NewSyncer // end-to-end against a mockConnector seeded at baton-demo scale — // the same data dimensions the public baton-demo connector emits — -// through both the SQLite engine (WithC1ZPath) and the Pebble -// engine (WithConnectorStore against a Pebble-backed c1zstore.Store). +// through both the SQLite engine (WithC1ZPath + WithStorageEngine) +// and the Pebble engine (WithConnectorStore against a Pebble-backed +// c1zstore.Store). WithC1ZPath alone follows the NewStore default, +// which is Pebble, so the sqlite arm must pin EngineSQLite. // // This is the FULL Sync() pipeline, not just the writer surface: // ResourceTypes → Resources → Entitlements → Grants iteration @@ -138,7 +140,7 @@ func runOneFullSync(b *testing.B, engine string, nUsers, nGroups, membershipsPer var opts []SyncOpt switch engine { case "sqlite": - opts = []SyncOpt{WithC1ZPath(c1zPath), WithTmpDir(tmpDir)} + opts = []SyncOpt{WithC1ZPath(c1zPath), WithTmpDir(tmpDir), WithStorageEngine(c1zstore.EngineSQLite)} case "pebble": store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithEngine(c1zstore.EnginePebble), From 6b486664a814df72a76019ca53f25dc3fd85debd Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Fri, 21 Aug 2026 12:13:28 -0600 Subject: [PATCH 14/15] Open ingest-filter verification stores read-only The NewC1ZFile-to-NewStore sweep left five post-sync verification opens in ingest_filter_test.go writable, unlike their siblings here and the 13 sites in syncer_test.go. These opens only read after the syncer is closed, so request read-only and stop re-sealing the v3 envelope on Close. Co-authored-by: Cursor --- pkg/sync/ingest_filter_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/sync/ingest_filter_test.go b/pkg/sync/ingest_filter_test.go index ce2365831..d11b8b99c 100644 --- a/pkg/sync/ingest_filter_test.go +++ b/pkg/sync/ingest_filter_test.go @@ -365,7 +365,7 @@ func TestFreshIngestFilterExternalMatchResolvesUnknownPrincipalType(t *testing.T require.NoError(t, internalSyncer.Sync(ctx)) require.NoError(t, internalSyncer.Close(ctx)) - store, err := dotc1z.NewStore(ctx, internalC1zPath) + store, err := dotc1z.NewStore(ctx, internalC1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) defer func() { require.NoError(t, store.Close(ctx)) }() @@ -436,7 +436,7 @@ func TestFreshIngestFilterInsertResourceGrantsResourceInserts(t *testing.T) { require.NoError(t, syncer.Sync(ctx)) require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewStore(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) defer func() { require.NoError(t, store.Close(ctx)) }() @@ -481,7 +481,7 @@ func TestInsertResourceGrantsRejectsReservedBatonIDResource(t *testing.T) { require.ErrorContains(t, syncer.Sync(ctx), "SDK-reserved BatonID ownership annotation") require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewStore(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) defer func() { require.NoError(t, store.Close(ctx)) }() drives, err := store.ListResources(ctx, @@ -561,7 +561,7 @@ func TestFreshIngestFilterRunsBeforeExclusionGroupValidation(t *testing.T) { require.NoError(t, syncer.Sync(ctx)) require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewStore(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) defer func() { require.NoError(t, store.Close(ctx)) }() @@ -797,7 +797,7 @@ func TestFreshIngestFilterHandlesMultiPageResourceTypes(t *testing.T) { require.NoError(t, syncer.Sync(ctx)) require.NoError(t, syncer.Close(ctx)) - store, err := dotc1z.NewStore(ctx, c1zPath) + store, err := dotc1z.NewStore(ctx, c1zPath, dotc1z.WithReadOnly(true)) require.NoError(t, err) defer func() { require.NoError(t, store.Close(ctx)) }() From ac3877343fb624ec9e1d5785860e1e95ed2c77e8 Mon Sep 17 00:00:00 2001 From: Matt Kaniaris Date: Fri, 21 Aug 2026 12:15:29 -0600 Subject: [PATCH 15/15] Guard the pebble cleanup test against leaking its store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A require failure between NewStore and syncer.Close left the open pebble store leaked for the rest of the test binary — the same class guarded in engine_registry_test.go. Pebble Close is idempotent, so the fail-safe defer is a no-op on the happy path where syncer.Close owns the store shutdown. Co-authored-by: Cursor --- pkg/sync/syncer_cleanup_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/sync/syncer_cleanup_test.go b/pkg/sync/syncer_cleanup_test.go index b835b6e7e..6cb2b58d7 100644 --- a/pkg/sync/syncer_cleanup_test.go +++ b/pkg/sync/syncer_cleanup_test.go @@ -120,6 +120,11 @@ func TestCleanupPebbleSingleSyncLifecycle(t *testing.T) { // Engine-less NewStore: the default (pebble) path under test. f, err := dotc1z.NewStore(ctx, testFilePath) require.NoError(t, err) + // Fail-safe only: a mid-test require failure must not leak the open + // pebble store for the rest of the test binary. On the happy path + // syncer.Close closes the store and this is a no-op (Close is + // idempotent). + defer func() { _ = f.Close(ctx) }() // Seed synced data the same way the SQLite test does. Under the // single-sync contract each iteration replaces the previous sync, so