Kans/pebble default engine - #1087
Merged
Merged
Conversation
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
General PR Review: Kans/pebble default engineBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe full PR diff was re-scanned for security and correctness; nothing new surfaced. The new commits are test-only: the five Risk triage (carried forward, unchanged): silence — yes (a wrong format flip produces a well-formed artifact); durability — yes (c1z on-disk format); uncontrolled dimensions — yes (which SDK version reads the artifact); consumer distance — the c1 platform and every downstream connector. Remediation rung 3–4. Verdict: HIGH. The instrument that gives real coverage on a version-pair flip is a two-artifact cross-version harness, and it now exists and is cited: Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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 ee6683a. Co-authored-by: Cursor <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
ggreer
approved these changes
Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Flips the default storage engine for newly created
.c1zfiles from SQLite (v1) to Pebble (v3). An engine-lessdotc1z.NewStoreon a new/empty path now writes a v3 artifact, matching the existing explicitWithEngine(EnginePebble)behavior.What this does NOT change
WithEngine(EnginePebble)and is logged at Info.WithEngine(EngineSQLite)/--storage-engine sqlitestill produce v1 artifacts.NewC1ZFileremains SQLite-only and rejects v3 files, as before.Versioning
This is a default durable-format change, so it ships as a minor bump (v0.25.0), not a patch.
pkg/sdk/version.gois regenerated from the release tag; no hand edit in this PR.Compatibility evidence
TestCheckpointCompatAcrossSDKVersionswas run locally against v0.20.2 in both directions, green, with the new-gen artifact being v3. Thenew_gen_old_resumeleg forces the old binary to read the v3 file, so this covers artifact-format readability, not just checkpoints.Rollout notes for downstream connectors
--storage-engine sqliteorWithEngine(EngineSQLite).--diff-syncsflag requires a multi-sync v1 artifact and returnsErrDiffUnsupportedagainst v3 files. Per RFC 0002's call-site audit this surface has no live callers.Follow-up (out of scope)
Pebble's fixed tuning (256 MiB block cache, 64 MiB memtables) is not yet overridable through
C1ZOptions; memory-constrained runtimes that need to opt down should get plumbing in a separate change.