Kans/pebble default engine by kans · Pull Request #1087 · ConductorOne/baton-sdk

kans · 2026-08-13T18:40:44Z

No description provided.

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 · 2026-08-13T18:54:05Z

General PR Review: Kans/pebble default engine

Blocking Issues: 3 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f7333f66e01d.
Review mode: incremental since 3446fe84
View review run

Review Summary

The new commit is test/comment-only: it converts all 21 dotc1z.NewStore verification opens in pkg/sync/syncer_test.go to WithReadOnly(true) with paired Close, and documents the now-EnginePebble dead fallback in storeOptionsFromC1ZOptions. Both prior findings on those two spots are addressed — every open in syncer_test.go is now read-only and closed (including clonedStore at :1764 and store1 at :1869, which close explicitly rather than via defer), and read-only close skips the re-seal path (pkg/dotc1z/pebble_store.go:381), so the mid-resume artifact in TestResumeSyncWithChildResources is no longer rewritten before syncer2 resumes. The full PR diff was re-scanned for security and correctness: no security issues, and the selectStoreDriver conversion gate (options.engine rather than requested) and the compactor's WithEngine(c.resolvedEngine()) force both verified correct — resolvedEngine() never returns "", so the Pebble dotc1z default cannot leak into a SQLite compaction. The four remaining findings below are all carried over and still open on this SHA.

Risk triage (unchanged, HIGH): silent — a wrong default produces a well-formed artifact in the other format, no panic; durable — every new .c1z produced fleet-wide changes format; uncontrolled dimensions — which SDK version wrote the artifact being read; consumer distance — the c1 platform and every downstream connector. Worst credible remediation is rung 3–4 (re-encode fleet artifacts / coordinate a contract change). Review-blind class: multi-artifact / cross-version. The instrument that would give coverage is a two-artifact cross-version harness in required CI; the PR does not contain one (see below).

Security Issues

None found.

Correctness Issues

pkg/sdk/version.go:3 — carried over, still open: Version is v0.24.1 and the PR body is empty. This is a default durable-format break, so under Versioning And Communication it needs a 0.x minor signal (v0.25.0) plus a migration/rollout note. (version.go is regenerated from the release tag by update-hardcoded-version.yaml, so the ask is the release plan and the PR description, not a hand edit.) (high confidence on the facts)
pkg/field/defaults.go:139 — carried over, still open: after the flip, --diff-syncs fails with ErrDiffUnsupported (pkg/dotc1z/engine/pebble/adapter_diff.go:15) on every default-engine artifact, since a v3 c1z holds exactly one sync by contract. The flag is still registered and fully wired (pkg/cli/commands.go:352 → connectorrunner.WithDiffSyncs → pkg/tasks/local/differ.go:62) and its description gives no hint it now works only on pre-existing v1 files. (high confidence on the code path)
cmd/baton-compat-harness/driver_test.go:107 — carried over, still open: the only cross-version instrument is TestCheckpointCompatAcrossSDKVersions, gated behind BATON_COMPAT=1 and run only by nightly.yaml via make compat-check (Makefile:77); no ci.yaml job covers it. It also exercises checkpoint/pagination-token exchange, not artifact-format readability, so nothing in required CI proves an older SDK release can read — or cleanly reject — a default-produced v3 artifact. HIGH-risk change naming no instrument and no permutation table: request the full pass-set review per docs/BUG_CATCHING.md §6 before merge. (medium-high confidence)

Suggestions

pkg/dotc1z/engine_registry.go:246 — carried over, still open: the flip subjects every default connector sync to the fixed Pebble tuning (256 MiB cache, 64 MiB memtable with stop-writes threshold 4, MaxOpenFiles: 1024 — pkg/dotc1z/engine/pebble/options.go:143-197), and pkg/dotc1z/pebble_store.go:102 exposes no C1ZOption to shrink it for memory-constrained runtimes. (high confidence on the defaults; impact deployment-dependent)

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Correctness Issues

In `pkg/sdk/version.go`:
- Around line 3: Version is v0.24.1. This PR flips the default dotc1z storage
  engine to Pebble, which changes the on-disk format of every newly created
  .c1z artifact fleet-wide. Do not ship this as a patch bump. Plan a 0.x minor
  release (v0.25.0) as the compatibility signal, and fill in the currently
  empty PR description with an explicit migration / rollout note covering: which
  SDK and platform versions can read a v3 artifact, what happens when an older
  reader encounters one, and how operators pin back to SQLite
  (--storage-engine=sqlite / WithEngine(EngineSQLite)). version.go itself is
  regenerated from the release tag by update-hardcoded-version.yaml, so edit the
  release plan and PR body, not the file.

In `pkg/field/defaults.go`:
- Around line 139-146: The --diff-syncs flag is still registered and wired
  (pkg/cli/commands.go:352 -> connectorrunner.WithDiffSyncs ->
  pkg/tasks/local/differ.go:62), but with the Pebble default every freshly
  synced artifact is v3, and the Pebble engine's GenerateSyncDiff returns
  ErrDiffUnsupported (pkg/dotc1z/engine/pebble/adapter_diff.go:15) because a v3
  c1z holds exactly one sync by contract. Either (a) update the flag description
  to state it requires a SQLite/v1 c1z and fail fast with a clear actionable
  error naming --storage-engine=sqlite when the resolved engine is Pebble, or
  (b) implement the cross-file diff path so the flag keeps working under the new
  default. Do not leave the flag silently broken under the default engine.

In `cmd/baton-compat-harness/driver_test.go`:
- Around line 107: TestCheckpointCompatAcrossSDKVersions is the only
  cross-version instrument and it is gated behind BATON_COMPAT=1, run only from
  nightly.yaml via `make compat-check` (Makefile:77), so no required CI job
  covers it. It also only exercises checkpoint / pagination-token exchange, not
  artifact-format readability. Add a required-CI two-artifact cross-version test
  that (1) writes a default-produced v3 artifact with this SDK and (2) opens it
  with a pinned older SDK release, asserting either successful read or a clean,
  typed rejection error rather than corruption or a panic. Also add a
  table-driven permutation test over {writer engine} x {reader engine} x {file
  pre-exists as v1 / v3 / absent} x {readOnly true/false} covering
  selectStoreDriver's dispatch, including the case that an engine-less open of
  an existing v1 file must NOT convert it in place.

## Suggestions

In `pkg/dotc1z/engine_registry.go`:
- Around line 246: Now that Pebble is the default, every connector sync
  inherits the fixed Pebble tuning in
  pkg/dotc1z/engine/pebble/options.go:143-197 (256 MiB block cache, 64 MiB
  memtable with MemTableStopWritesThreshold 4, MaxOpenFiles 1024), and
  pkg/dotc1z/pebble_store.go:102 offers no C1ZOption to lower it. Add a
  C1ZOption (for example WithPebbleCacheSize / WithPebbleMemTableSize) that
  threads through storeOptionsFromC1ZOptions into pebbleOpenOptionsFromC1Z, so
  memory-constrained runtimes such as small Lambda configurations can shrink the
  footprint without opting out of the engine entirely.

github-actions

Blocking issues found — see review comments.

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>

github-actions

No blocking issues found.

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>

github-actions

Blocking issues found — see review comments.

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>

github-actions

Blocking issues found — see review comments.

…ngine

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>

github-actions

Blocking issues found — see review comments.

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>

github-actions

Blocking issues found — see review comments.

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>

github-actions

Blocking issues found — see review comments.

kans and others added 3 commits August 13, 2026 12:35

Name pebble as the default in the --storage-engine help text

327d169

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>