[wip] ledger storage [remove checkpoints 1 of 2] - #1131
Conversation
Adds the four ledger messages the atomic-pages storage layer writes, plus
the compaction provenance that moves out of the sync token:
LedgerActionIdentity the tuple that determines an action's connector
request; equal identity means equal work
LedgerRow one committed page: transition, children, counts
LedgerCounterBucket one (run, worker) bucket of sync-level counters
LedgerFrontier takeover record for a sync begun token-only
CompactionProvenance what a compaction produced and from what
SyncStatsRecord gains compaction = 14. Field 14 was unused; ingest_quality
is 13 and written_at is 100.
No Go code reads these yet. Generated pb/ output is byte-identical to the
frozen reference at ref/6b-ledger-monolith, so this commit is the proto
change and nothing else.
Co-authored-by: Cursor <cursoragent@cursor.com>
The raw key layout for the page ledger, plus the staging ops that write it. Nothing calls these yet. TypeLedger is 0x0C, the first unassigned type byte (0x01-0x0B in use, 0xFF is engine meta). The family splits on a sub-kind byte: 0x00 page rows, 0x01 facts, 0x02 counter buckets, 0x03 frontier. The ledger rides RecordBatch rather than getting a writer of its own. A row means "the records staged beside me landed", so the only batch it may ride is the one carrying those records. There is no standalone ledger writer, which is what makes a row without its page, or a page without its row, unexpressible rather than merely avoided. StageLedgerTakeover is the one RecordBatch op that touches the sync-run key. The frontier record and the token-cleared sync run have to land together: a frontier without a cleared token is taken over twice, and a cleared token without a frontier loses the action stack. Fact values carry a one-byte tag so a bare fact (0x01) and a valued one (0x02 + bytes) share a keyspace; DecodeLedgerFactValue reads both. This is an on-disk ABI. Applied from ref/6b-ledger-monolith. families.go and records.go are byte-identical to it; keyspace.go differs only where main removed the source-cache compat record, which is not ledger code. Co-authored-by: Cursor <cursoragent@cursor.com>
The store-facing surface for atomic pages: the interfaces an engine must implement to accept page-at-a-time writes, and the plain types they carry (LedgerActionIdentity, LedgerRow, LedgerCounters, LedgerFrontier, SyncStats). These land before the pebble implementation because the adapter implements them; the engine cannot compile without them. Nothing implements them yet, so this commit is types and contracts only. The package stays independent of any engine: ledger.go imports only sourcecache and the connector protos. Applied from ref/6b-ledger-monolith, byte-identical to it. Co-authored-by: Cursor <cursoragent@cursor.com>
Extracts the staging loop out of each Put*Records into a stage*Records(batch, records) helper for resource types, resources, entitlements, and grants. Same keys, same marshaling, same typed Stage* ops, same order; Put*Records now calls the helper. The page unit needs to stage records into a batch it owns rather than one Put*Records opened, and it must stage them exactly the way Put*Records does or a page's rows would differ from a non-ledgered write of the same records. Sharing the function is what makes that identity hold instead of being a thing two code paths agree about. Behavior-preserving on its own; no caller of the new helpers beyond Put*Records yet. Co-authored-by: Cursor <cursoragent@cursor.com>
The engine side of atomic pages, in three parts. ledger.go is the row codec and the read side: LedgerIdentity and its key encoding, the proto conversions, and iteration (whole family, by op, by resource). Reads compare the row's echoed identity against the identity being resolved, so a key collision from an identity omission or a key-function bug surfaces as a re-run and a LedgerMismatches bump rather than a silently skipped page. It also holds the token scrub (ScrubLedgerTokens, for connectors that declare page tokens sensitive), the counter fold (SumLedgerCounters), and takeoverToken, which moves a token-only sync's stack into the frontier. page_unit.go is PageUnit: a batch that accumulates one page's records, facts, and counter bucket, then Commit stages all of it plus the ledger row into a single RecordBatch. One pebble commit, one fsync, so "the rows landed" and "the page is done" cannot disagree. It stages through the stage*Records helpers from the previous commit, so a page's rows are identical to a non-ledgered write of the same records. adapter_page.go implements c1zstore.PageWriter and PageLedgerStore over PageUnit, converting connector v2 messages to storage v3 records at the boundary. sync_stats_sidecar.go rides here rather than with the record staging: its overlay is how the ledger's EndSync hands the engine a folded stats value, and it reads the syncStatsOverlay field engine.go adds in this commit. Tests come with their subject rather than trailing: ledger_test.go, ledger_state_test.go, adapter_page_test.go, and the commit-point enumeration that asserts which batch each write lands in. Applied from ref/6b-ledger-monolith. Every file byte-identical to it except cleanup.go, which differs by the one source-cache compat bound main removed. Co-authored-by: Cursor <cursoragent@cursor.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func (u *PageUnit) release() { | ||
| u.done = true | ||
| u.resourceTypes, u.resources, u.entitlements, u.grants = nil, nil, nil, nil | ||
| u.resourceIdx = nil | ||
| } |
There was a problem hiding this comment.
🟠 Bug: release() nils entitlements but leaves entitlementIdx populated, while resourceIdx is cleared. GetEntitlementRecord (line 309) has no done guard, so after Commit/Discard a lookup that hits the stale index does u.entitlements[i] on a nil slice and panics — where the resource path correctly falls through to the DB. Confidence: high on the defect (the asymmetry is clearly unintended), lower on reachability today since nothing calls PageWriter yet.
| func (u *PageUnit) release() { | |
| u.done = true | |
| u.resourceTypes, u.resources, u.entitlements, u.grants = nil, nil, nil, nil | |
| u.resourceIdx = nil | |
| } | |
| func (u *PageUnit) release() { | |
| u.done = true | |
| u.resourceTypes, u.resources, u.entitlements, u.grants = nil, nil, nil, nil | |
| u.resourceIdx = nil | |
| u.entitlementIdx = nil | |
| } |
| func (e *Engine) DropLedger(ctx context.Context) error { | ||
| return e.withWriteAllowSealed(func() error { | ||
| lo, hi := rawdb.LedgerBounds() | ||
| return e.db.DropKeyRange(lo, hi, writeOpts(e.opts.durability)) | ||
| }) | ||
| } | ||
|
|
||
| // ResetLedger implements c1zstore.PageLedgerStore. | ||
| func (e *Engine) ResetLedger(ctx context.Context) error { | ||
| return e.DropLedger(ctx) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: DropLedger removes the family but leaves ledgerInFlight set and the on-disk stamp at keyspaceVersionLedgerInFlight. For the two unsealed callers named in the doc comment (the sanitizer's drop policy, compaction outputs) the store then has no ledger yet still refuses CheckpointSync with ErrLedgeredSyncWritesNoToken and refuses plain EndSync with ErrLedgeredSyncNeedsStats, and an older SDK still refuses to open the file. Clearing the flag/stamp here (or documenting that the drop is sealed-only) would close that. Low confidence on impact today — the only wired caller, ResetLedger on a rebound finished sync, runs with the flag already cleared by the seal.
General PR Review: [wip] ledger storage [remove checkpoints 1 of 2]Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe full PR diff was scanned for security and correctness: proto wire compat, exported-API stability, serialized state, and dependency manifests ( Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Two things the store owes the ledger. The write seam. Every direct record write on pebbleStore now calls s.seam(ctx, method) first. The hook lives in an atomic.Pointer, so with none installed the idle cost is one atomic load and nothing else — PageOpen is short-circuited and never reached. Its purpose is to catch a write that bypasses the page unit while a page is open: such a write lands in its own batch, so it is durable independently of the page that logically contains it, which is the one thing atomic pages exist to prevent. Nothing installs a hook yet. Coverage is the whole point of a seam like this, so it is checked rather than assumed: the 21 guarded methods are the monolith's 23 minus ClearSourceCacheScope and PutSourceCacheCompat, which main deleted. That establishes fidelity to the reference, not that the reference drew the line correctly — FinishExpandedGrantLayer and AddExpandedGrantLayerContributions are record-affecting writers with no guard, which is a real gap and is left for the commit that installs the first hook. The rest of the unguarded set is the dirty helpers, the sync lifecycle, and the ledger's own Commit / TakeoverToken / PutCounterBucket, which are the page write and cannot guard against themselves. The ledger plumbing. BeginPage, EndSyncWithStats, TakeoverToken, and PutCounterBucket implement c1zstore.PageLedgerStore and SyncStatsStore at the store layer. BeginPage wraps the engine's writer in dirtyPageWriter so a committed page marks the store dirty like every other write; miss that and a page's records are durable in pebble but the envelope is never rewritten on Close. Reconciled rather than replayed, since main moved under both files. source_cache.go takes 5 of the monolith's 7 seam guards: main deleted PutSourceCacheCompat, GetSourceCacheCompat, and ClearSourceCacheScope, and a 3-way merge happily resurrects all three, so that hunk was dropped by hand. pebble_store.go's first hunk keyed on the MaterializationWitnessReader assertion main removed. The result is additive against main, which is what says no part of main's work was reverted on the way in. Co-authored-by: Cursor <cursoragent@cursor.com>
Moves what a compaction produced and from what out of the sync token and into the SyncStatsRecord sidecar. A ledgered sync writes no token, so a token section is not somewhere provenance can live. compactPebbleFold used to call sdksync.BuildCompactedToken and store the result with baseRec.SetSyncToken. It now reads each source's stats sidecar, overlays the base's timings, folds each partial's on top, and stamps outputStats.SetCompaction with the CompactionProvenance message added in the proto commit. Still best-effort: provenance never fails a compaction, and a source without a sidecar just contributes no timings. provenance.go holds the four pieces that were inline before: buildCompactionProvenance, overlayTimingStats, foldPartialTimings, and compactionRecordCounts. compactor_pebble.go drops its pkg/sync import as a result, though the package still depends on pkg/sync through compactor.go. BuildCompactedToken now has no caller in this repo; it stays exported for external consumers and stays covered by the v1_compaction.json golden fixture. Deleting it belongs to 2b, which removes the token write path. Applied from ref/6b-ledger-monolith, all three files byte-identical to it. Co-authored-by: Cursor <cursoragent@cursor.com>
kans
changed the title
Three reviewers read the ledger port independently. None found a way to split a page — the records, row, facts and bucket are genuinely one RecordBatch — but all three found defects in the lifecycle around the page. Six are fixed here. Every one is latent today: nothing calls the ledger until the syncer does, which is why they are cheap to fix now rather than after a caller depends on the current behavior. Each fix has a test in ledger_lifecycle_regression_test.go that was run against the unfixed code first: reverting any one of the six fails its test, and the release() revert reproduces the panic below rather than merely failing an assertion. 1. The seal left verbatim page tokens in the artifact. takeoverToken stores the taken-over sync token JSON verbatim in the frontier, and every Action in that JSON carries a page_token. ScrubLedgerTokens iterates LedgerRowBounds, which is kind 0x00 only; the frontier is kind 0x03, so it was never rewritten. On a connector that declared its tokens sensitive, a sync that began token-only and was taken over sealed with credentials readable in the file — the one thing SetLedgerTokensSensitive promises cannot happen. The token-only path never had this exposure: its stack is empty by the time it seals. Attempt and taken_over_at stay, so the audit trail survives; only the state goes, and nothing reads it after the resume that consumed it. 2. ResetLedger and DropLedger did not mark the store dirty. Both were promoted from the embedded Engine with no wrapper while Close saves only when dirty, so a session whose only write was the drop discarded it and the next open still enumerated every prior action as done. The sync then skips work it never ran: silent under-collection, no error anywhere. This is the same failure the commit adding dirtyPageWriter described, unextended to the siblings. 3. CheckpointSync and EndSync gated on the in-flight flag alone. clearLedgerInFlight drops the durable stamp and the in-memory flag before endSyncFinalize writes ended_at, so two states have a ledger while the flag reads false: the finalize failing after the clear (the caller may then keep writing or retry), and a crash in the same window (the reopen reads a v2 stamp over rows that are still there). Both let a token be written beside a live ledger, the second lagging authority ErrLedgeredSyncWritesNoToken exists to refuse. Rows outlive the stamp, so ledgerActive asks about rows. Called from those two places only, never per record. A side effect worth naming: a rebind that skips the documented ResetLedger now fails loudly instead of silently running with two authorities. 4. ResetForNewSync left a stamp describing a ledger it had deleted. The excise covers the ledger family but the keyspace stamp lives in the preserved engine-meta range, so after an abandoned ledgered sync the replacement sync was refused a token by CheckpointSync AND refused a plain seal by EndSync, on a file with no ledger at all — neither protocol could finish it. Safe in this direction: a wiped file has no rows for an older SDK to misread. 5. TakeoverToken wrote no bucket for stats-only counters. The gate tested Counters and Flags; LedgerCounters has five fields. A token whose phases had run without completing a page lost its timings and call stats, and takeover clears that token in the same batch, so there was no second copy to fold from. PutCounterBucket filtered nothing, so the two paths disagreed. 6. A spent PageUnit panicked on one of its two page-scoped reads. release() nilled resourceIdx but not entitlementIdx, and neither getter checked done, so GetResourceRecord fell through to the DB while GetEntitlementRecord indexed a nil slice: index out of range for any id the page had staged. Both now return ErrPageUnitCommitted, and release() clears every field it owns. Not fixed here, and named so they are not lost: the takeover bucket key (runID, worker 0) collides with real worker 0's page bucket; applySyncerStats consumes the stats overlay before the write that uses it, and a failed finalize leaves a stale overlay for a later seal; the ledger row's write counts use raw buffer length while the stagers dedup by identity; and FinishExpandedGrantLayer and AddExpandedGrantLayerContributions are record-affecting writers with no write-seam guard. All four need the syncer's side to settle first. Co-authored-by: Cursor <cursoragent@cursor.com>
6ca1cc6 to
5eda7c1
Compare
| rec, err := e.GetSyncRunRecord(ctx, syncID) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| state := rec.GetSyncToken() | ||
| if state == "" { | ||
| return "", nil | ||
| } | ||
| frontier := v3.LedgerFrontier_builder{State: state, Attempt: syncID, TakenOverAt: timestamppb.Now()}.Build() | ||
| fv, err := marshalRecord(frontier) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| updated := proto.Clone(rec).(*v3.SyncRunRecord) | ||
| updated.SetSyncToken("") | ||
| rv, err := marshalRecord(updated) | ||
| if err != nil { | ||
| return "", err | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: takeoverToken is a read-check-write over the single sync-run record (GetSyncRunRecord → clear sync_token → stage), but it does not hold lifecycleMu — the mutex engine.go:59 documents as serializing exactly these sequences, and which CheckpointSync, CurrentSyncStep and endSync all take. withWrite only serializes the commit, not the read that precedes it. Concretely: if endSync snapshots existing and takeoverToken commits its token-cleared record in between, endSyncFinalize's clone-and-stamp writes the pre-takeover snapshot back and resurrects the verbatim checkpoint token in the sealed artifact — beside a live frontier, the two-authority state ErrLedgeredSyncWritesNoToken exists to prevent. No in-tree caller races these today, so this is a latent invariant break rather than a live bug. BoundSyncFinished (line 655) reads CurrentSyncID() then the record without the lock for the same reason CurrentSyncStep takes it.
| // Empty reports whether nothing has been staged. A page that wrote | ||
| // nothing still commits (its ledger row is the fact that it ran). | ||
| func (u *PageUnit) Empty() bool { | ||
| return len(u.resourceTypes) == 0 && len(u.resources) == 0 && | ||
| len(u.entitlements) == 0 && len(u.grants) == 0 | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Empty reports "nothing has been staged" but only inspects the four record slices. A page that staged only grantDeletes (the external-resource phase's replaced originals), only facts, or only a counter bucket reports true. If the syncer in the follow-up PR uses Empty() to skip work, a grant removal or a needs_expansion fact would be silently dropped — and the ledger row would still claim the page ran. Either fold grantDeletes/facts/bucketKey into the predicate or rename it to say it covers records only.
| func ledgerCountersToProto(c c1zstore.LedgerCounters) *v3.LedgerCounterBucket { | ||
| return v3.LedgerCounterBucket_builder{ | ||
| Counters: c.Counters, | ||
| Flags: c.Flags, | ||
| ConnectorCalls: callStatsToProto(c.ConnectorCalls), | ||
| StepDurationsMs: cloneInt64Map(c.StepDurationsMs), | ||
| SessionCalls: callStatsToProto(c.SessionCalls), | ||
| }.Build() | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Counters is aliased into the proto (callStatsToProto and cloneInt64Map copy; this one does not). On the PutCounterBucket path that is harmless — PutLedgerCounterBucket marshals immediately. On the page path it is not: PageUnit.StageCounterBucket stores the proto and marshalRecord runs later inside Commit (page_unit.go:431), so whatever the caller's map holds at commit time is what lands, not what it held at SetCounterBucket. PageWriter.SetCounterBucket's contract says the caller owns the cache and "last call before Commit wins", which invites exactly that reuse. Copy the map here (as the other three fields do) so both paths snapshot at the same moment.
| if e.compactionScheduler != nil && e.compactionScheduler.paused.Load() { | ||
| e.resumeCompactions() | ||
| defer e.pauseCompactions() | ||
| } | ||
| lo, hi := rawdb.LedgerBounds() | ||
| if err := e.db.Compact(ctx, lo, hi, true); err != nil { | ||
| return fmt.Errorf("PurgeLedgerResidue: %w", err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🟡 Suggestion: this adds an unbounded manual compaction to the seal path for every sensitive-token connector, and its cost is not the ledger's own bytes — it is every SST overlapping LedgerBounds(), which on a fresh sync means the L0 files the ledger rows are interleaved with. docs/BUG_CATCHING.md's cost-contract rule asks for a stated big-O in ledger size and a benchmark that enforces it on seal-path work; ledger_cost_bench_test.go benchmarks synthetic page commits and the resume walk against a raw DB, not ScrubLedgerTokens (an O(rows) full rewrite) or this compaction. TestLedgerScrubLeavesNoSSTResidue proves correctness on a 3-row ledger and says nothing about the whale case. A bench over the real path at 10^5–10^6 rows would tell you whether this is seconds or minutes added to every seal.
An external review of the branch. One finding it raised was already fixed by the previous commit; these are the other three. All latent, no caller until the syncer's side lands. Each has a test that fails when its fix is reverted. 1. The takeover's counter bucket shared a key with worker 0. Buckets are blind-written whole totals keyed by (run, worker) and the fold sums across them, so staging the migrated bucket at (runID, 0) put it on an index a real page worker owns: worker 0's first commit in the same run replaced it, and the pre-takeover counters left the fold with nothing to notice. It now has a reserved index of its own, c1zstore.TakeoverBucketWorker. Not RunBucketWorker, which the review suggested first — the syncer writes the run-level bucket there, which is the same collision one index over. 2. DropLedger removed the rows and left the stamp that describes them. A drop before the seal left a file with no ledger that still refused CheckpointSync and still refused a plain EndSync, and still read as an unsupported layout to a token-only SDK. This is the defect the previous commit fixed in ResetForNewSync, in the other place that deletes these rows; fixing one and not the other is how it survived three reviews. The clear runs after the drop: if it fails, the rows are gone but the file still refuses a token, which is the safe way round. 3. A failed seal left its stats overlay behind. endSync stashed the overlay before two fallible steps, and only PersistSyncStats consumes it, so a failed seal parked those stats under syncID for whatever sealed that id next — the retry's own stats silently losing to a dead attempt's. setSyncStatsOverlay's comment already promised the value never outlives the EndSync that supplied it, so the code contradicted a documented contract rather than merely lacking one. The stash moved after GetSyncRunRecord so exactly one path can leave it behind, and that path now drops it. Still open, unchanged from the previous commit's list: the ledger row's write counts use raw buffer length while the stagers dedup by identity, and FinishExpandedGrantLayer and AddExpandedGrantLayerContributions are record-affecting writers with no write-seam guard. Both want the syncer's side to settle first. Co-authored-by: Cursor <cursoragent@cursor.com>
| type pebbleDriver struct{} | ||
|
|
||
| var _ c1zstore.Store = (*pebbleStore)(nil) | ||
| var _ c1zstore.WriteSeamStore = (*pebbleStore)(nil) |
There was a problem hiding this comment.
🟡 Suggestion: *Engine has var _ c1zstore.PageLedgerStore = (*Engine)(nil) (adapter_page.go:28), but *pebbleStore — the value the syncer will type-assert — has no equivalent assertion for PageLedgerStore or SyncStatsStore. It satisfies both today, yet five of those methods are store-level overrides that shadow the promoted engine methods (BeginPage, TakeoverToken, ResetLedger, PutCounterBucket, EndSyncWithStats); a signature drift in any one of them would shadow the promoted method, fail the assertion at runtime, and silently drop the sync back to the token-only path with no compile error and no test failure here. Adding var _ c1zstore.PageLedgerStore = (*pebbleStore)(nil) and var _ c1zstore.SyncStatsStore = (*pebbleStore)(nil) next to the WriteSeamStore assertion turns that into a build break. (confidence: high)
| // setSyncStatsOverlay's contract is that the value never outlives | ||
| // the EndSync that supplied it, and the retry brings its own. | ||
| // A no-op when finalize already consumed it. | ||
| if overlay != nil { |
There was a problem hiding this comment.
🟡 Suggestion: this drops the stash only on the finalize-failure path. endSyncFinalize reaches PersistSyncStats (adapter.go:454) and, when the seal takes the non-stashed branch, PersistSyncStats returns early on computeSyncStats error before applySyncerStats consumes the overlay (sync_stats_sidecar.go:272-275). That error is warn-only, so endSync returns nil with the entry still parked in e.syncStatsOverlay[syncID] — exactly the "value never outlives the EndSync that supplied it" contract this comment states, violated on the success path this time. A later SetCurrentSync(syncID) + reseal (grant expansion's follow-up sync rebinds the same id) would then write the previous run's step durations, call stats and ingest quality onto the new seal. Taking the overlay unconditionally after endSyncFinalize returns, rather than only in the error branch, closes it. (confidence: medium)
Found by sweeping one class rather than reading for bugs. Nine of the twelve findings on this branch share a shape — one fact with two representations, and a path that updates only one — so the sweep enumerated every in-memory mirror the ledger introduced against every path that mutates the durable state behind it. Three cells came back clean: all three ledger writers stamp in-flight before staging, the seal scrubs and purges before clearing the stamp, and all six mutating methods on PageLedgerStore and SyncStatsStore mark the store dirty. A fourth found a third deleter of ledger keys, cloneSync, which is safe only because it refuses a sync without ended_at, so the seal has already scrubbed and cleared by the time it copies. The fifth cell is this. ledgerTokensSensitive had one setter, one reader, and nothing on disk. The reader is the seal, deciding whether to rewrite every ledger row to hash-only tokens. The setter is called by whoever STARTS the sync — and the process that seals need not be that one: a crash mid-sync is resumed and sealed by whatever runs next, which has no way to know the connector's tokens carry credentials. It skipped the scrub and shipped them verbatim in the artifact. Same failure the frontier scrub fixed, reached from the crash-and-reopen direction instead of the sub-family-coverage one. The declaration is now c1zstore.LedgerFactTokensSensitive, staged into the page's own batch, so it is exactly as durable as the tokens it governs and costs no extra write. Blind-set per page: a fact is a monotone last-writer-wins key, so re-staging is one key and self-heals a run whose declaring process died after the first page. takeoverToken stages it too — it writes a frontier holding a verbatim token before any page exists, so a crash right after it would otherwise leave that token with no fact to scrub it by. The seal reads fact OR flag. Same remedy as the ledgerActive gate two commits back: derive from durable state, do not trust the mirror. The test reopens the engine and resumes through NewAdapter().ResumeSync without ever calling SetLedgerTokensSensitive, which is the cross-process case the flag cannot survive. Reverting the seal to the flag alone fails it. Co-authored-by: Cursor <cursoragent@cursor.com>
| e.resumeCompactions() | ||
| defer e.pauseCompactions() | ||
| } | ||
| lo, hi := rawdb.LedgerBounds() |
There was a problem hiding this comment.
🟡 Suggestion (medium-high confidence): the purge range is the ledger family only, but the takeover path leaves a verbatim token outside it. takeoverToken clears sync_token on the sync-run record, which lives at the single fixed key v3|TypeSyncRun (rawdb.SyncRunKey) — every prior CheckpointSync wrote a superseded version of that key carrying the connector's page tokens verbatim. Pebble never overwrites in place, so those versions sit in whatever SST they were flushed to; ScrubLedgerTokens doesn't touch them and Compact(LedgerBounds()) doesn't cover 0x06. The result is exactly the failure this function's own doc comment describes ("query-invisible, strings-visible") for a sensitive-tokens connector whose sync began token-only and was taken over — the frontier gets scrubbed and purged, the record it was migrated out of does not.
Not reachable today (no production caller of TakeoverToken yet), so non-blocking, but it should close before CXE-1358 wires the takeover up. Fix: when sensitive, also compact the sync-run key range ({VersionV3, TypeSyncRun} .. UpperBound) alongside the ledger family.
| // (stats_sync_id) and records what this fold merged; each partial's | ||
| // timings are folded on top. Provenance is best-effort — it never | ||
| // fails the compaction. | ||
| outputStats, statsErr := enginepkg.ReadSyncStatsRecord(ctx, destEng, newSyncID) |
There was a problem hiding this comment.
🟡 Suggestion (compatibility): this moves compaction provenance and the folded partial timings out of the compacted output's sync_token and into the stats sidecar, so BuildCompactedToken now has no caller in this repo. Any consumer that reads the compaction section — or the folded step_durations_ms / connector_call_stats — off a compacted artifact's sync token stops finding them, and does so silently (empty section, not an error). pkg/sdk/version.go is unchanged and the PR carries no migration note for that consumer. Worth either a 0.x minor bump plus an explicit note on where provenance now lives, or a transitional write to both places until downstream readers move.
| // | ||
| // DropLedger, not ResetForNewSync: drop the trace and keep the | ||
| // records, which is the caller DropLedger's comment names. | ||
| if err := destEng.DropLedger(ctx); err != nil { |
There was a problem hiding this comment.
🟡 Suggestion: DropLedger is a DeleteRange, so the base's ledger rows are query-invisible but still physically present in the SSTs copyFileForFold byte-copied — and the fold deliberately doesn't compact before save (BATON_EXPERIMENTAL_FOLD_COMPACT is off), so those bytes ship in the output. That's benign for an ordinary base (the seal already ran ScrubLedgerTokens + PurgeLedgerResidue, so the copied rows carry hash-only tokens), but a base sealed under LedgerFactRetainTokens keeps verbatim page tokens in its rows; the fold output then carries them as strings-visible residue while reporting no ledger, and no later scrub can reach rows that iteration no longer sees. Worth either pairing the drop with PurgeLedgerResidue (bounded to the family's overlapping SSTs, not O(base)) or stating in the comment why inheriting a retain-tokens base's residue is acceptable. Medium confidence — reachable only via the debug opt-out, and no connector emits a ledgered c1z until CXE-1358.
| // DropLedger, not ResetForNewSync: drop the trace and keep the | ||
| // records, which is the caller DropLedger's comment names. |
There was a problem hiding this comment.
🟡 Suggestion: the last clause doesn't parse — "which is the caller DropLedger's comment names" looks like a dropped word or a half-finished edit. DropLedger's own doc lists "compaction outputs" among the callers that keep the sync and drop only its trace; say that directly.
| // DropLedger, not ResetForNewSync: drop the trace and keep the | |
| // records, which is the caller DropLedger's comment names. | |
| // DropLedger, not ResetForNewSync: keep the records and drop only | |
| // the trace, which is the "compaction outputs" caller DropLedger's | |
| // own comment names. |
| if err != nil { | ||
| return fmt.Errorf("EndSync: read retain-tokens fact: %w", err) | ||
| } | ||
| if scrub { |
There was a problem hiding this comment.
🟠 Bug: sealScrubsTokens() returns true whenever the retain fact is absent, which is every sync that has no ledger at all — i.e. every sync today and every token-only/SQLite-converted sync after CXE-1358. So ScrubLedgerTokens + PurgeLedgerResidue run on every EndSync, not just ledgered ones. The scrub is cheap over an empty range; the purge is not: db.Compact rewrites every SST that overlaps [v3|0x0C, v3|0x0D), and on a ledger-free file the SSTs spanning the gap between TypeSourceCache (0x0B) and TypeEngineMeta (0xFF) overlap it at every level (maxLevelWithFiles then drives manualCompact at levels 0..N). It also un-pauses the compaction scheduler that e.seal() deliberately paused for this window, and the DeleteRange over LedgerBounds that ResetForNewSync now stages can force a memtable flush via Compact's overlap check.
ledger_seal_cost_bench_test.go sweeps pages ∈ {1000, 10000} and never measures pages=0, so the shape that regresses is the one shape the benchmark doesn't cover. Suggest gating on ledger presence (ledgerActive(), or skipping the purge when the scrub rewrote no rows) and adding a pages=0 bench arm.
| e.resumeCompactions() | ||
| defer e.pauseCompactions() | ||
| } | ||
| lo, hi := rawdb.LedgerBounds() |
There was a problem hiding this comment.
🔴 Security (non-blocking): the purge range is LedgerBounds only, but the takeover path writes the verbatim token outside it. takeoverToken → StageLedgerTakeover rewrites SyncRunKey() (v3|TypeSyncRun, 0x06) to clear sync_token; pebble never overwrites in place, so the superseded sync-run value — the full checkpoint-token JSON, with a page_token per Action — stays in whatever SST it was flushed to and ships in the saved c1z. scrubLedgerFrontierLocked closes the frontier's copy of those same bytes and the purge removes it; the sync-run copy is left behind, so the seal's stated byte-level guarantee doesn't hold for a taken-over sync.
Not a new exposure (those bytes were already in the file before the takeover cleared them), which is why I'm not marking it blocking — but it defeats the reason scrubLedgerFrontierLocked exists. TestLedgerScrubLeavesNoSSTResidue only exercises page tokens, so a takeover arm would catch it.
| for _, partial := range partialStats { | ||
| foldPartialTimings(outputStats, partial) | ||
| } | ||
| outputStats.SetCompaction(buildCompactionProvenance( |
There was a problem hiding this comment.
🟡 Suggestion: provenance moves to the sidecar here and in compactPebble, so after this PR nothing in the repo writes the token's compaction section. sdksync.BuildCompactedToken and sdksync.CompactionStatsFromToken stay exported and keep working, but any downstream caller that reads provenance off a compacted artifact's sync_token now silently gets nil instead of a section — a behavior change with no error and no fallback. Worth a // Deprecated: on CompactionStatsFromToken pointing at SyncStatsRecord.compaction, plus a pkg/sdk/version.go bump and a migration note, since the PR touches no version file.
| if syncID == "" { | ||
| return "", errors.New("takeoverToken: no open sync") | ||
| } | ||
| rec, err := e.GetSyncRunRecord(ctx, syncID) |
There was a problem hiding this comment.
🟡 Suggestion: this read-modify-write of the sync-run record spans two lock scopes. GetSyncRunRecord runs unlocked here, then StageLedgerTakeover rewrites SyncRunKey under writeMu inside withWrite. CheckpointSync and endSync serialize their own get-then-put on lifecycleMu, which this path never takes, so a checkpoint interleaving between the read and the commit is silently clobbered and the frontier captures the pre-checkpoint state. ledgerActive() doesn't close the window either — markLedgerInFlight only runs inside the withWrite closure below, after the read. Taking lifecycleMu for the whole function would make it consistent with the other sync-run mutators.
…r leaves sealScrubsTokens reports true whenever the retain-tokens fact is absent, and a sync with no ledger never writes that fact, so endSyncFinalize reached PurgeLedgerResidue on every seal in the fleet. Its db.Compact rewrites every SST whose bounds overlap the ledger range, and on a ledger-free file the SSTs spanning the gap between TypeSourceCache and TypeEngineMeta do overlap it, at every level with files. Gate the scrub and the purge on ledgerActive. That gate opened two paths where the ledger is gone from the keyspace while its verbatim page tokens are still bytes in the SSTs a checkpoint hard-links: DropLedger mid-sync then seal, which is compactPebbleFold's case, and an interrupted ledgered sync followed by a ledger-free one, where ResetForNewSync excises the rows. Both now arm encodeLedgerResiduePendingKey before deleting, since the deletion is what destroys the evidence, and purgeMarkedLedgerResidue consumes it only after its compaction succeeds — so a failed or interrupted purge is retried by the next seal instead of shipping tokens. The marker records how the rows were deleted, because db.Compact selects files by their bounds. DropKeyRange leaves them covering the ledger range, so compacting that range rewrites them; ExciseRange narrows them into virtual files that exclude it, and then only a wider compaction still overlaps. Keeping the two apart matters because DropLedger runs on every compaction fold, where the wide compaction would rewrite the whole artifact. TestLedgerResidueOutlivesTheLedger covers four arms and fails with either half of the fix removed. TestLedgerFreeSealSkipsResiduePurge pins the gate in both directions through testSeams.ledgerResiduePurges, which counts compactions rather than calls. endSyncFinalize also drops the sync-stats overlay when PersistSyncStats fails. That call can return on a computeSyncStats error before applySyncerStats consumes the stash, and the seal warns and continues, which left the stats parked under a finished sync id. Co-authored-by: Cursor <cursoragent@cursor.com>
BeginPage captures the sync open at the time; Commit's requireCurrentSync only asserts that some sync is open. A page lives for seconds to minutes, long enough to straddle an EndSync followed by a StartNewSync, and the rebound engine is unsealed again — so the previous run's buffered records would land in the replacement sync, and its ledger row would enumerate a page that run never ran. The keyspace holds one sync at a time, so those records are indistinguishable from the new run's once they land. Commit now returns ErrPageUnitForeignSync, before markLedgerInFlight, and the unit survives for a retry. The check is not atomic against a bind racing it: the binding flips under currentSyncMu, not writeMu. What it closes is the wide window; withWrite's sealed check already rejects a commit arriving between a seal and the next bind. TestPageUnitCommitRefusesAForeignSync pins both arms and fails with the condition neutered. PageUnit.Empty counted only the four record slices, so a page staging only grant deletes, only a fact, or only its counter bucket read as empty while still writing on commit. ledgerCountersToProto copies every map it takes. StageCounterBucket holds the bucket until Commit, seconds to minutes later, and its doc leaves the cache with the caller, so an aliased map let a worker's later increment change what gets marshaled. Co-authored-by: Cursor <cursoragent@cursor.com>
Callers reach PageLedgerStore and SyncStatsStore through a runtime ok assertion, so a signature drift turned into a false ok that sent the caller down its no-ledger path, with no build error anywhere. The compile-time assertions make that a build failure. They do not cover the other direction: pebbleStore embeds *pebble.Engine, so a new mutating method is promoted, satisfies the interface, and skips the markDirty wrapper that gets the mutation into the saved file — the write is then lost at Close with no error. TestPebbleStoreDirtyCoverage requires every method of both interfaces to be classified with a reason, and every write to be declared on *pebbleStore with markDirty in its body. Deleting the ResetLedger override or stripping the mark from dirtyPageWriter.Commit both fail it. TestWriteSeamOutcomes covers StrictWriteSeam's four outcomes and that an empty bypass reason does not register. Co-authored-by: Cursor <cursoragent@cursor.com>
A compaction output inherits its base's sync token, so a base compacted by
an SDK that still wrote the token's compaction section makes the output
report that compaction as its own — wrong mode, base id and counts.
ClearCompactionSection removes that section and nothing else, because
PersistSyncStats still falls back to the token for timings and ingest
quality when a sync has no stats overlay. An empty token stays empty
rather than round-tripping, since Unmarshal("") seeds an InitOp action
that a finished output must not carry.
Both compactor output paths strip it, best-effort like the provenance
write itself. BuildCompactedToken and CompactionStatsFromToken are marked
deprecated in favour of SyncStatsRecord.compaction, where provenance now
lives.
Co-authored-by: Cursor <cursoragent@cursor.com>
| // Armed before the excise, not after: a crash in between leaves the | ||
| // marker over rows that are still there, which costs one compaction at | ||
| // the next seal. The other order loses the marker and ships the bytes. | ||
| if ledgered { |
There was a problem hiding this comment.
🟠 Bug: ledgerActive() is true for a ledger that has already been scrubbed and purged, so this arms residueExcised on the ordinary "previous sync finished, start the next one in the same file" path — EndSync scrubs rows, it never deletes them, so the rows are still there at the next StartNewSync. The next EndSync then reaches purgeMarkedLedgerResidue with residueExcised and compacts [0x03, 0x04) — the whole v3 keyspace, a full-artifact rewrite — chasing residue that sync A's own seal already purged over the ledger range. It also runs under lifecycleMu, and a standing excised marker makes any later DropLedger/ResetLedger do the same wide compaction inline, which is the cost the tombstoned/excised split exists to avoid.
Gate the arming on the prior ledger possibly still holding verbatim tokens rather than on presence: skip it when the prior sync has an ended_at and the retain-tokens fact is absent (that pair means scrub + purge already ran), and keep arming for an interrupted sync or a retain-tokens sync. BenchmarkLedgerSealCost has no shape for a seal carrying an excised marker, so this curve is currently unmeasured.
426ac2a to
4113137
Compare
| return nil | ||
| } | ||
| lo, hi := rawdb.LedgerBounds() | ||
| if kind == residueExcised { |
There was a problem hiding this comment.
🟡 Suggestion: the corruption policy is inconsistent between the two halves. ledgerResidueKind returns residueExcised for a wrong-length value on the stated grounds that the wider kind is the safe reading, but a single byte that is neither 't' nor 'x' reaches here as a nonzero kind and falls into the narrow LedgerBounds() compaction, then gets its marker consumed. Inverting the test — treat only residueTombstoned as narrow and everything else as excised — makes both corruption shapes take the wider range.
| return len(u.resourceTypes) == 0 && len(u.resources) == 0 && | ||
| len(u.entitlements) == 0 && len(u.grants) == 0 && | ||
| len(u.grantDeletes) == 0 && len(u.facts) == 0 && | ||
| u.bucketValue == nil |
There was a problem hiding this comment.
🟡 Suggestion: Empty() now uses bucketValue as the bucket's presence signal while Commit gates the staging on bucketKey != nil. StageCounterBucket(runID, worker, nil) sets the key and leaves the value nil, so the unit reports empty yet the commit stages a counter-bucket row whose value is a marshaled nil message. No in-repo caller passes nil (SetCounterBucket always builds a bucket), but the two predicates should agree — either test bucketKey here too, or have StageCounterBucket reject a nil bucket the way PutLedgerCounterBucket does.
Co-authored-by: Cursor <cursoragent@cursor.com>
| } | ||
| lo, hi := rawdb.LedgerBounds() | ||
| if kind == residueExcised { | ||
| lo, hi = []byte{versionV3}, []byte{versionV3 + 1} |
There was a problem hiding this comment.
🟡 Suggestion: (confidence: medium) The residueExcised arm compacts the whole v3 keyspace — a full-artifact rewrite — and purgeMarkedLedgerResidue runs on the seal path ungated by ledgerActive, so the next EndSync after a ResetForNewSync that replaced a ledgered sync pays it even if that replacement sync is token-only. BenchmarkLedgerSealCost never arms this marker (its shapes only reach PurgeLedgerResidue over LedgerBounds), so the one seal-path cost that scales with file size rather than ledger size has no benchmark holding it. Worth a kind=excised shape in that sweep, or a stated bound on how often the excised marker can be armed.
| // DropLedger, not ResetForNewSync: drop the trace and keep the | ||
| // records. This is the compaction-output caller DropLedger's own doc | ||
| // names. | ||
| if err := destEng.DropLedger(ctx); err != nil { |
There was a problem hiding this comment.
🟡 Suggestion: (confidence: medium) DropLedger now runs on every fold, and a sealed ledgered base keeps its rows by design (ledger.go: "Rows persist after the sync seals as its execution trace"), so once CXE-1358 lands each fold does DropKeyRange over the family plus a db.Compact over LedgerBounds on the freshly copied base — new per-compaction work proportional to the SSTs overlapping the ledger range. Nothing in pkg/synccompactor benchmarks it, and BenchmarkLedgerSealCost measures the seal, not the fold. A fold-path shape with a ledgered base would pin the cost before the syncer starts producing them.
| if overlay := e.takeSyncStatsOverlay(syncID); overlay != nil { | ||
| if len(overlay.GetStepDurationsMs()) > 0 { | ||
| rec.SetStepDurationsMs(overlay.GetStepDurationsMs()) | ||
| } | ||
| if len(overlay.GetConnectorCallStats()) > 0 { | ||
| rec.SetConnectorCallStats(overlay.GetConnectorCallStats()) | ||
| } | ||
| if len(overlay.GetSessionStoreStats()) > 0 { | ||
| rec.SetSessionStoreStats(overlay.GetSessionStoreStats()) | ||
| } | ||
| if overlay.HasIngestQuality() { | ||
| rec.SetIngestQuality(overlay.GetIngestQuality()) | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: (confidence: high) The return makes the token fallback conditional on the overlay's presence, not on whether it supplied a value, while the doc comment reads as per-field ("the stats EndSyncWithStats was given ... else lifted from the sync_run's sealed token"). syncStatsOverlay always returns a non-nil record, so EndSyncWithStats(ctx, c1zstore.SyncStats{}) — or any partially-populated SyncStats — silently drops the sync_run token's step_durations_ms / call stats for the fields it did not fill, where plain EndSync would have lifted them. Harmless for a ledgered sync (no token), but EndSyncWithStats is exported on Engine and on pebbleStore via c1zstore.SyncStatsStore, so a non-ledgered caller can hit it. Either fall through to ApplySyncTokenStatsRecord for the fields the overlay left empty, or say in the comment that the overlay is all-or-nothing.
Three findings from re-applying docs/COMMENTS.md after its revision: endSyncFinalize's gate comment explained why ledgerActive gates the scrub block and never said what it leaves out, which is the silence that let a deleted ledger's residue through in the first place. It now names the case and points at the marker block below it. The bounds mechanism read complete at both residueTombstoned and TestLedgerResidueOutlivesTheLedger; the constants own it and the test points at them. PurgeLedgerResidue's doc had grown to 30 lines over a 4-line wrapper. The scheduler-pause fact moved down to compactForLedgerResidue, which is the code that un-pauses; the cost and memtable-flush facts stayed. Co-authored-by: Cursor <cursoragent@cursor.com>
The four *_written counts came from the PageUnit's buffer lengths, but three of the four stagers dedup by identity before staging, so a page that staged one identity twice recorded more than the keyspace holds. Each stager now returns the number of distinct keys it staged and the ledger row is built from those, after staging rather than before. stageResourceTypeRecords had no dedup pre-pass at all, unlike the other three. Added, so its return means the same thing they do; the duplicate put it was making was cost rather than a wrong outcome, since the later put won. Nothing outside tests reads these counts yet. They are the only record of what a page put in the keyspace, so they have to be right before anything reconciles a ledger against a file. TestLedgerRowCountsDistinctKeysNotBufferedRecords stages one identity twice per family and pins both the count and the keyspace. Verified by restoring the buffer-length counts: it reports 2 against 1 key. Co-authored-by: Cursor <cursoragent@cursor.com>
AddExpandedGrantLayerContributions reached markDirty on no path, though it mutates the file twice over: the first Add arms the deferred by_principal rebuild, and a segment that fills mid-layer is ingested into the live keyspace. Only FinishExpandedGrantLayer marked, so the wrapper's rule held by luck of every persisting path going through Finish. TestPebbleStoreDirtyCoverage did not see it: the layer session hangs off Grants(), so its methods are on pebbleStoreGrants rather than *pebbleStore, and the walker only read pointer receivers on the two store capabilities. It now covers pebbleStoreGrantLayerStorer as a third capability with its own receiver, and reads value receivers. Abort is classified as no-write on purpose. It drops staged chunks and the temp dir; the ingested segments it leaves behind are documented as idempotent overwrites on retry, and they are Add's writes, which now mark. Verified by planting both defect shapes: dropping Add's markDirty, and leaving a layer method unclassified. Co-authored-by: Cursor <cursoragent@cursor.com>
The dedup pre-pass added with the ledger counts was verified only by a key count, and a pre-pass that kept the FIRST occurrence leaves one key too. It would persist the stale record. TestFreshSyncWithinCallDuplicateResourceType- Dedup asserts the survivor, alongside the three same-shaped tests the other families already had; verified against a planted first-wins pre-pass, which the count test does not catch. Also two comment trims from re-applying docs/COMMENTS.md to the last three commits: the count rationale read complete both at the assignment in page_unit.go and on the test, and the stager's doc restated the mechanism before drawing the conclusion from it. Co-authored-by: Cursor <cursoragent@cursor.com>
docs/COMMENTS.md on kans/comments-example names section banners and labels under Not comments: navigation the structure should provide, and if one stays it is a name rather than a sentence. The three in capabilityMethods stay as names; splitting the map three ways to carry the same grouping would cost more than it tells a reader. Co-authored-by: Cursor <cursoragent@cursor.com>
| // A segment that fills mid-layer is ingested into the live keyspace here, | ||
| // and the first Add arms the deferred by_principal rebuild, so this | ||
| // mutates the file before Finish is ever called. | ||
| return g.store.markDirty(fast.AddExpandedGrantLayerContributions(ctx, dest, principals, sources)) |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): this comment now asserts AddExpandedGrantLayerContributions mutates the live keyspace before Finish, and the new capabilityMethods entry classifies it dirtyWrite — but it is the only pebbleStoreGrants write with no g.store.seam(ctx, ...) guard, alongside FinishExpandedGrantLayer. StoreExpandedGrants, StoreNewExpandedGrants and StoreNewExpandedGrantContributions all carry one, and PageWriter has no expanded-grant staging method, so once CXE-1358 opens pages around ExpandGrants these calls are precisely the durable-write-outside-the-page's-batch shape the seam exists to observe — and it cannot see them to demand a WithPageWriteBypass registration.
Nothing in the diff enforces guard placement statically: the 21 guards were checked by hand against the monolith, whereas the markDirty obligation got TestPebbleStoreDirtyCoverage. A sibling method-set meta-test over the writes reachable through pebbleStore/pebbleStoreGrants — each classified guarded or reasoned-exempt — would make the same drift a build-time failure instead of silent under-reporting.
1 participant
Ledger storage: rawdb ledger family, PageUnit, PageWriter, SyncStatsStore, compaction provenance
CXE-1357, step 3 of CXE-1354. Engine-only. Blocked by CXE-1356 (merged);
blocks CXE-1358, which puts the syncer on this.
Ported from the monolith's storage files
(
git diff e06c2b95 ref/6b-ledger-monolith -- pkg/dotc1z pkg/synccompactor proto).What this is for
A sync currently records its progress in a checkpoint token: one opaque
string, rewritten on every checkpoint. A page's records and the token that
says the page is done are separate writes, so a crash between them leaves
a c1z whose records and whose idea of what it has collected disagree.
This adds the storage the ledger needs to close that: a page's records and
the ledger row saying the page happened commit as one pebble batch.
Nothing uses it yet — no
pkg/syncchange is in this PR, which is theacceptance criterion for this step.
Commits
Each builds and vets on its own.
proto/c1/storage/v3: ledger records and compaction provenancepb/churn)pebble/rawdb: ledger keyspace and typed staging opsc1zstore: PageLedgerStore, PageWriter, and ledger value typespebble: share record staging between Put*Records and the page unitpebble: the page ledger and the atomic page unitdotc1z: write seam and the store's ledger plumbingsynccompactor: compaction provenance into the stats sidecarRead them in order. 1 is generated output, 2 is the on-disk key layout,
3 is the interfaces, 4 is a pure refactor with no behavior change, 5 is
the actual ledger, 6 wires it into the store, 7 is the compactor
consequence. 4548 handwritten insertions; 3739 of the total are
pb/.1. Proto
SyncStatsRecordgains the fields the token carried (step durations,connector call stats, session stats, ingest quality) plus
CompactionProvenance. New messages for the ledger itself:LedgerActionIdentity,LedgerRow,LedgerCounterBucket,LedgerFrontier. Generated withmake protogen; the output isbyte-identical to the monolith's.
2. Keyspace
TypeLedger(0x0C, checked against the existing type bytes forcollision) with four kinds: rows
0x00keyed by semantic action identity,facts
0x01(bare and valued), counter buckets0x02per run and worker,frontier
0x03for the taken-over token. Plus the typedStageLedger*staging ops and
RecordBatch.Empty()/Len().3. Interfaces
PageLedgerStore,PageWriter,SyncStatsStore, and the plain (non-proto)value types
LedgerActionIdentity,LedgerRow,LedgerCounters,LedgerFrontier,SyncStats. Engine implementations depend on theserather than on proto.
4. Record staging refactor
The staging loops inside
PutResourceTypeRecords,PutResourceRecords,PutEntitlementRecords, andPutGrantRecordscome out asstage*Recordshelpers so the page unit stages records the same way thedirect writes do. Two callers of one function each, instead of two
implementations that have to be kept in step. No behavior change.
5. The ledger and the page unit
PageUnitbuffers a page's writes — puts, grant deletes, staged-row drops,facts, counter buckets — and commits them with the ledger row as one batch,
with read-your-writes for entitlements and resources.
ledger.goholdsidentity encoding, proto conversion, iteration, token scrubbing, counter
folding, and takeover.
keyspaceVersionLedgerInFlightstamps a c1z that ismid-ledgered-sync so an older SDK fails loudly instead of reading it as
token-only.
CheckpointSyncreturnsErrLedgeredSyncWritesNoTokenonce aledger exists;
ResetLedgerdrops a sealed sync's ledger on rebind;PurgeLedgerResiduehandles scrubbed connectors.6. Write seam and store plumbing
BeginPage,EndSyncWithStats,TakeoverToken,PutCounterBucketat thestore layer.
BeginPagewraps the engine's writer indirtyPageWritersoa committed page marks the store dirty — miss that and a page's records are
durable in pebble while the envelope is never rewritten on
Close.The write seam is a test-time instrument: every direct record write calls
s.seam(ctx, method)first, a no-op unless a hook is installed and ctxis inside an open page. It exists to catch a write that bypasses the page
unit while a page is open, because such a write lands in its own batch and
is durable independently of the page that logically contains it — the one
thing atomic pages exist to prevent.
7. Compactor
Compaction provenance moves from the sync token to the
SyncStatsRecordsidecar, because a ledgered sync writes no token.
compactPebbleFoldstops calling
sdksync.BuildCompactedTokenand instead reads eachsource's sidecar, overlays the base's timings, folds each partial's on
top, and stamps
SetCompaction. Still best-effort: provenance never failsa compaction.
This leaves
BuildCompactedTokenwith no caller in this repo. It staysexported for external consumers and stays covered by the
v1_compaction.jsongolden fixture; CXE-1358 deletes it with the tokenwrite path.
Reviewing a port
HIGH under
docs/REVIEW_CHECKLIST.md. Three things are worth checkingrather than assuming, and each was:
Seam coverage. A missing guard makes the instrument silently
under-report, and no single run would show it. The 21 guarded methods are
the monolith's 23 minus
ClearSourceCacheScopeandPutSourceCacheCompat,which
maindeleted. The 14 write methods left unguarded are exactly themonolith's 14 — so
mainadded no write path the seam misses — and theyare the right 14: the dirty helpers, the sync lifecycle, and the ledger's
own
Commit/TakeoverToken/PutCounterBucket, which are the page writeand cannot guard against themselves.
Fidelity. 26 of the 34 files are byte-identical to
ref/6b-ledger-monolith. Five differ only wheremainmoved underneath:keyspace.goandcleanup.goby the source-cache compat recordmainremoved,
grants.gobymain's chunk-cleanup fix and a comment rewording,and
pebble_store.go/source_cache.goby thematerializationWitnessplumbing and the three source-cache methods
maindeleted. No differenceis ledger code.
The reconciliation needed hand work.
git apply -3happily resurrectsPutSourceCacheCompat,GetSourceCacheCompat, andClearSourceCacheScopeinside a conflict region, since the monolith adds a seam guard to methods
maindeleted. Those hunks were dropped by hand, leaving 5 of themonolith's 7 source-cache guards. Commit 6 is additive against
main—+167 insertions, no deletions — which is the check that says none of
main's work was reverted on the way in.Verification
(
git rebase --exec 'go build ./... && go vet ./pkg/dotc1z/... ./pkg/synccompactor/...' origin/main).golangci-lint0 issues;gofmtclean.pkg/dotc1z(133s),pkg/dotc1z/c1zstore,pkg/dotc1z/engine/pebble(114s),.../codec,.../microtests,pkg/synccompactor(292s),.../attached,.../pebble(79s),and
pkg/sync(249s).pkg/sync—git diff origin/main..HEAD -- pkg/sync/is empty.
TestCommitPointsHaveFailureSeamsincluded: that meta-test asserts every commit point has a registered
failure seam, which is what holds the ledger's new commit points to the
engine's existing
errorfs_sweep_test.goinstead of a hand-kept list.Self-checking mutant cases pass too, e.g.
TestLedgerScrubLeavesNoSSTResidue/mutant:_scrub_without_residue_purge_leaks.The gap to know about
Nothing installs a
WriteSeamHook, so the write seam is inert as shipped.The test that gives it teeth is
pkg/sync/chaos_atomic_pages_sweep_test.go(+528), which lands with CXE-1358 along with
page_ledger_state_test.go(+477) — both need a syncer that can actually produce a bypassing write.
So this PR ships the mechanism and the engine-level tests; the sweep
proving the mechanism catches a bypass arrives with the syncer. The seam's
coverage is verified statically here, which is what can be checked without
that syncer.