Repository navigation
Drop Checkpoints, and persist Pebble sync pages atomically - #1140
Merged
Merged
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Comment on lines
+120
to
+123
| counters := c1zstore.LedgerCounters{ | ||
| Counters: make(map[string]uint64), ConnectorCalls: make(map[string]c1zstore.CallStat), | ||
| StepDurationsMs: maps.Clone(parts.stats.stepDurationsMs), SessionCalls: make(map[string]c1zstore.CallStat), | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): the takeover bucket and the run bucket look like they will double-count the same pre-interruption stats. decodeLedgerCheckpoint imports the checkpoint's cumulative stepDurationsMs, connectorCalls and sessionOps into TakeoverBucketWorker, but loadRunStats (token.go:221-232) restores those same cumulative maps into the resumed run's in-memory runStats. When the caller later hands that in-memory state to flushRunCounters/prepareSeal, it lands in RunBucketWorker, and LedgerCounters sums every bucket — so ledgerSyncStats reports the pre-interruption durations and call counts twice. ledgerCompletedActions is safe because the resumed run only counts actions it actually completes; the stats maps are not, because they are carried whole rather than derived per page.
ledgerRuntime.prior is loaded in newLedgerRuntime and never read anywhere, which is where that reconciliation would go — implementation.md §4 says prior buckets are "aggregated only for reporting and gates". No test covers the combination: every prepareSeal/flushRunCounters test starts from a ledger with no takeover bucket. Worth resolving (or at least asserting) before the production handler starts supplying real run counters.
Contributor
Superseded — see the current review report for commit
|
Comment on lines
+72
to
+75
| worker, _ := ctx.Value(ledgerWorkerKey{}).(int) | ||
| if worker < 0 || worker >= int(c1zstore.TakeoverBucketWorker) { | ||
| return errors.New("invalid ledger worker index") | ||
| } |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: two things in this guard.
-
The discarded
okmakes a missing or wrong-typedledgerWorkerKey{}indistinguishable from a deliberate worker 0, and the bounds check below cannot detect it.implementation.md§17 says sequential phases intentionally use worker zero, but that intent currently rests on a dropped type assertion — any future call site that forgets to seed the index silently shares bucket 0 instead of failing. Consider the two-value form plus an explicit worker-zero default at the sequential call sites. -
int(c1zstore.TakeoverBucketWorker)is a constant conversion ofuint32 = 0xFFFFFFFE; on a 32-bitGOARCHthat value is not representable inintand the package fails to compile. No 32-bit release target exists here, but this is a library hundreds of connectors import. Comparing inuint32space avoids it:
Suggested change
| worker, _ := ctx.Value(ledgerWorkerKey{}).(int) | |
| if worker < 0 || worker >= int(c1zstore.TakeoverBucketWorker) { | |
| return errors.New("invalid ledger worker index") | |
| } | |
| worker, _ := ctx.Value(ledgerWorkerKey{}).(int) | |
| if worker < 0 || uint32(worker) >= c1zstore.TakeoverBucketWorker { | |
| return errors.New("invalid ledger worker index") | |
| } |
| // carry no `test` prefix of their own — any `test`-prefixed field elsewhere | ||
| // in the package is a seam that escaped this struct. | ||
| type syncTestHooks struct { | ||
| ledgerHandler func(context.Context, *Action, *ledgerPage) error |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: ledgerHandler is the only field here without the per-seam doc comment the other three carry, and it does not match the type's own description of "observation and fault-injection points". It replaces the production record handler: in invokeActionPage the handler argument is dropped on the ledgered path and a nil ledgerHandler returns "ledger production handlers are not integrated" rather than running production code. A reader of this file would conclude it is an observer like checkpointHook. Document what it substitutes for and that it is the only executable body on the ledgered path until K2d lands.
| child.Spawned = recorded.Spawned | ||
| children = append(children, child) | ||
| } | ||
| return s.nextPageOrFinishAction(ctx, action, row.NextPageToken, children...) |
Contributor
There was a problem hiding this comment.
🟠 Bug: the replay transition drops row.TypeScopedPlanned. TypeScopedPlanned is not part of LedgerActionIdentity, so it survives only on the row — which is why walkWithSeen restores it onto the continuation (ledger_walk.go:59). Here the continuation keeps the in-memory action's value (false for a child just pushed by a parent's transition), so when the next page of a SyncEntitlementsOp/SyncGrantsOp root runs, !action.TypeScopedPlanned is true again (syncer.go:2051, syncer.go:2613) and the whole type-scoped fan-out is re-planned — silent duplicate actions. Set s.markTypeScopedPlanned(action) (or copy the flag) from row.TypeScopedPlanned before transitioning.
| if row.Scrubbed { | ||
| return errLedgerScrubbedUnfinished | ||
| } | ||
| children := make([]Action, 0, len(row.Children)) |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: replay reconstructs only children and the next token, so a page that originally finished as a warning replays as a clean completion — nextPageOrFinishAction reaches finishActionLocked(..., false) and invokeActionPage returns nil, so the caller never calls finishActionWithWarning. LedgerRow has no warning marker (the count lives only in the counter bucket), so replay cannot recover it, and the in-memory ratio that gates ErrTooManyWarnings under-counts. Either record the warning on the row or note this as an explicit open item alongside the pending fact restoration.
| _, err = s.ledger.runPageWithCommit(ctx, uint32(worker), ledgerIdentity(action), func(pageCtx context.Context, page *ledgerPage) error { | ||
| invocation.page = page | ||
| page.row.Spawned = action.Spawned | ||
| page.row.TypeScopedPlanned = action.TypeScopedPlanned |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: page.row.TypeScopedPlanned is snapshotted here, before the handler runs, but markTypeScopedPlanned (syncer.go:2100, syncer.go:2663) sets action.TypeScopedPlanned during the handler — after nextPageOrFinishAction has already staged the transition. The page that plans the type-scoped fan-out therefore commits a row with TypeScopedPlanned=false, so the new restore at line 90 and ledger_walk.go:59 can never observe it for that page: a resume at the next page re-enters SyncEntitlements/SyncGrants with the flag clear and re-plans the whole type-scoped fan-out. Assign the row field from the transition/commit callback (or have markTypeScopedPlanned write through to invocation.page.row) so the row records the value leaving the page. Note the two tests that cover this set page.row.TypeScopedPlanned = true by hand (ledger_restore_test.go:129, ledger_walk_test.go:21), so the write side is untested.
| if !finished && s.cfg.onlyExpandGrants { | ||
| return nil, conflict("unstarted", "nothing has been collected under this sync ID") | ||
| } | ||
| case SyncGrantExpansionOp.String(), SyncExternalResourcesOp.String(): |
Contributor
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): when only the import is left in the queue, collectionFlagConflict no longer runs. Its external_source_configured / external_entitlement_id_filter checks used to refuse a resumer that leaves out WithExternalResourceC1ZPath. That resumer now runs SyncExternalResources with s.externalResourceReader == nil, and listExternalResourceTypes / GetEntitlement dereference the nil interface and panic, where it used to get a clean ErrLedgerStateConflict. Suggest refusing (or comparing only the external fields) when SyncExternalResourcesOp is queued and s.externalResourceReader == nil, and adding a third case to TestLedgerExpansionPassWithExternalImportResumes with no external path.
Contributor
Superseded — see the current review report for commit
|
Contributor
There was a problem hiding this comment.
No blocking issues found — see the full review report
kans
force-pushed
the
matt.kaniaris/CXE-1358/syncer-ledger-plan
branch
from
October 5, 2026 18:40
5db9303 to
b6834d0
Compare
Contributor
Superseded — see the current review report for commit
|
| BoundSyncUnstarted(ctx context.Context) (bool, error) | ||
| } | ||
|
|
||
| type PageLedgerStore interface { |
Contributor
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): PageLedgerStore and PageWriter both ship in v0.35.0. This PR removes TakeoverToken, BoundSyncFinished and EndSyncWithStats from PageLedgerStore and adds about 15 required methods across the two interfaces, and the PR body also states a Pebble ledger-format downgrade boundary. pkg/sdk/version.go is still v0.35.0, and the repo criteria ask for a 0.x minor bump to signal this kind of break. Bump the minor version here, or confirm the release process will do it.
| for _, child := range row.GetChildren() { | ||
| id := child.GetIdentity() | ||
| // Local phases complete without collection history. | ||
| if id.GetOp() == "grant-expansion" || id.GetOp() == "list-external-resources" { |
Contributor
There was a problem hiding this comment.
🟡 Suggestion: this still matches "grant-expansion" and now also "list-external-resources" as bare string literals, copied from pkg/sync's ActionOp.String(). If either op string changes, the reference check stops skipping local-phase children and starts failing on valid reports. Nothing in this package would catch that. Export the op names as shared constants (for example in c1zstore) or add a cross-package test that pins them.
Contributor
Superseded — see the current review report for commit
|
Contributor
There was a problem hiding this comment.
No blocking issues found — see the full review report
ggreer
approved these changes
Oct 5, 2026
Contributor
Superseded — see the current review report for commit
|
Contributor
Superseded — see the current review report for commit
|
Contributor
There was a problem hiding this comment.
No blocking issues found — see the full review report
Pebble collection pages commit records, facts, accounting and pending-work transitions atomically. Resume reads unfinished work in bounded windows through the existing scheduler. Separate work IDs and revisions allow repeated request tokens to execute normally. Existing checkpoint tokens seed the queue atomically during migration. SQLite continues checkpointing. Flags are locked per phase on both engines: a finished sync accepts only `WithOnlyExpandGrants`; on Pebble a resumer whose collection flags differ from the first attempt's recorded options is refused before any write; an expansion-only invocation plans from the file's skip facts and reads none of its own collection flags or targets, on SQLite as well as Pebble. Expansion and external import retain main's handlers and optimized write paths; neither is buffered in a page transaction. Static entitlements materialize one resource page at a time, preserving template order and cold resume. Successful collection archives a mechanical stats report, drops ledger history by default and purges discarded token data. Explicit ledger debug retains scrubbed history; retaining tokens requires an additional explicit option. Debug logging alone does not enable retention. Attempt metadata keeps first/latest options and folded prior-attempt accounting rather than growing with every retry. Plain `EndSync()` preserves its existing lifecycle meaning: ending a run does not assert collection completed. It saves committed accounting, finalizes indexes, flushes and detaches while preserving data and pending recovery state through close/reopen. Explicit same-ID continuation can use that state. The existing end/cleanup/start-new reset sequence works; starting a new Pebble sync retains main's reset behavior. `EndSyncWithStats` remains the syncer's checked completion path. `WithConnectorStore(nil)` still falls back to the configured file path. Custom sync stores: Pebble requires `PageLedgerStore` and is ledgered; every other engine, including an empty or unknown one, checkpoints through the token path as on main and must not expose `PageLedgerStore`. Pebble wrappers must forward the capability. There is no Pebble checkpoint fallback. The ledger interfaces gain methods; custom implementations must update accordingly. **SDK downgrade constraint:** unfinished and early-ended Pebble artifacts can retain the new ledger recovery format. A host reusing those artifacts cannot downgrade its vendored SDK across that format boundary. Keep the host on a ledger-capable SDK, or discard affected recovery artifacts and start fresh before downgrading. This is distinct from rolling back a connector binary without changing the host's SDK. Validation: - The unexpanded-upload/host-expansion regression collects and saves three base grants with expansion disabled, copies the artifact, and requests expansion on the same sync ID. Exact six-grant output is checked after reopen with one and four workers, default/debug retention and normal/early ending. An explicit expansion-only call finishes any prior seal and performs the requested expansion in the same invocation, including unfinished empty queues and prepared seals. Ordinary recovery can finish the prior seal alone. Explicit expansion requests may redo deterministic expansion; accounting records both actual passes. Handoff failure tests cover seal/rebind errors, graph persistence and retention reset. Storage clear failure/crash tests preserve metadata/accounting and queue atomicity. Full suites, focused race tests, CI-equivalent lint and bounded independent correction review pass. - Service-mode rollback uses actual daemon and connector subprocesses against a local TLS/gRPC C1 API. Targets `bba86699` (v0.30.1) and `eb63f1b5` pass 16 cases covering single/batched polling, spare off/on, and process kill/reported sync error. The same directory and options survive the version switch; old daemons upload usable data and complete another task, then the new SDK succeeds after roll-forward. Three ordinary repetitions pass; the current harness also passes race checks. This resource-only fixture simulates C1 redelivery and does not claim production workflow coverage. - Full sync, public storage, Pebble and compactor suites pass; focused lifecycle and attachment race checks pass. - A real `eb63f1b5` checkpoint is compared with that SDK's completed artifact and an uninterrupted ledger migration. Twenty-eight crash combinations cover takeover, grant and terminal commits, repeated crashes, WAL/flush recovery and 1/4 workers. Three repetitions and a race run pass, comparing records, indexes, digest, normalized stats and ledger accounting. Dropping imported action accounting makes the test fail. - Independent bounded reviews covered lifecycle changes and the migration test's oracle. Seal/archive crash tests are separate from the combined historical fixture. - Changed-code lint passes locally. Final-head CI is pending. Historical-SDK builds and performance drivers remain opt-in. [Review guide and performance](https://github.com/ConductorOne/baton-sdk/blob/0bd0e5fb/docs/verification/syncer-on-ledger/README.md) · [Plan/change orders](https://github.com/ConductorOne/baton-sdk/blob/0bd0e5fb/docs/verification/syncer-on-ledger/plan.md) · [Evidence and review dispositions](https://github.com/ConductorOne/baton-sdk/blob/0bd0e5fb/docs/verification/syncer-on-ledger/evidence.md) The evidence records executed coverage and residual gaps. It does not claim complete coverage of every original plan product or completion of the original full C49 performance matrix. Successful page retries preserve every observed connector call, session usage report and reported wait in committed/live totals. The regression test checks exact totals, repeated-token page isolation, failed-attempt record/fact exclusion and close/reopen, with three race repetitions. Retry observations before any successful commit remain best-effort. Squash of 240 commits on matt.kaniaris/CXE-1358/syncer-ledger-plan; pre-squash head kept at backup/CXE-1358-pre-squash (5db9303). Co-authored-by: Cursor <cursoragent@cursor.com>
The in-flight stamp was its own synced write on both sides of a ledger's life: set before the first ledger batch, cleared after the purge and before the seal batch that writes ended_at. A crash between the clear and the seal left a v2 stamp over an unfinished sync with no token; a token-only SDK opened that file, seeded Init, and collected again on top of the sealed records, after which this SDK refused the file as a legacy checkpoint beside pending work. The arm side had the inverse image: a v3 stamp over no rows. Ledger.stageMarkInFlight stages the stamp in the batch that writes the first ledger key; Ledger.stageClearInFlight stages its return to v2 in the Drop batch and the seal batch. TestLedgerDiscardDurableSealCuts asserts the stamp at every seal image and its before-ended cell was red before this. TestLedgerTakeoverCrashImages' mid cell now opens under a token-only SDK. TestDropLedgerCommitFailureKeepsRowsAndStamp covers Drop's commit route. CO-041. Co-authored-by: Cursor <cursoragent@cursor.com>
…l import A baseline token whose stack is exactly the expansion step cannot say whether expansion ran: baseline SDKs clear its cursor and keep no graph. main's resumer treats that stack as a step still to take, so --dont-expand-grants skips it and seals. Taking it over as Expanding made this SDK refuse that resume, which a self-hosted connector with the flag in fixed configuration could never satisfy. legacyStackPhase is removed and the refusal with it. SyncExternalResources returns an error when the invocation has no external resource source instead of dereferencing the nil reader; the pending entry stays for an invocation that has one. Comments on the in-flight stamp updated for CO-041. Co-authored-by: Cursor <cursoragent@cursor.com>
first_options was written before parallelSync and compared only once collection work was queued. An attempt that died between that write and its Init commit left a record with no plan behind it; the next attempt, finding only the Init seed, was not compared, planned under its own flags, and left the first attempt's flags as the record a third attempt was held to. The record now rides the planning commit: the Init page (recordFirstReportOptions), or the takeover batch when a legacy stack already carries collection work. BeginFromToken takes facts as a map so the batch can carry the value. The attempt snapshot (latest_options) stays a coordinator write before any page. CO-042 in plan.md. Co-authored-by: Cursor <cursoragent@cursor.com>
BeginCollecting takes facts map[string]string so the batch that seeds a frontier's stack carrying collection work can stage first_options, as the takeover batch does. Closes review H1 at 45ea871b: that reseed planned the pass without the record, so the attempt that seeded it was never the lock. TestLedgerFrontierSeedArmsCollectionFlagLock was red at the record assertion before the change.
The token is the only record that collection saw external-match grants; the takeover carries it into the pass C1 runs with only-expand and an external source. The control arm (fact absent) leaves the placeholder principal.
Both branches shadowed err, so the deferred EndSpanWithError saw nil.
kans
force-pushed
the
matt.kaniaris/CXE-1358/syncer-ledger-plan
branch
from
October 6, 2026 19:22
758d5dc to
548f484
Compare
Contributor
Superseded — see the current review report for commit
|
| return s.nextPageOrFinishAction(ctx, action, resp.GetNextPageToken(), actions...) | ||
| } | ||
|
|
||
| return s.collectLedgerResources(ctx, action) |
Contributor
There was a problem hiding this comment.
🟡 Suggestion (high confidence, tracing only): the deferred EndSpanWithError(span, err) reads the outer err, but nothing ever assigns it. resp, err := at line 34 shadows it, and this return doesn't set it. A failing ListResources page therefore closes syncer.SyncResources as successful. syncLedgerStaticEntitlements has the same problem (ledger_static_entitlements.go:33 and the for rts, err := range at line 40). Assign it the way syncLedgerGrants does (err = s.collect...; return err). This is the same class of bug 548f484 fixed for SyncExternalResources.
| ledgerCommitted func(c1zstore.LedgerRow) | ||
| ledgerStop func(context.Context) | ||
| ledgerWalk func(bool) | ||
| ledgerHandler func(context.Context, *Action, *ledgerPage) error |
Contributor
There was a problem hiding this comment.
🟡 Suggestion (prior — still present; the earlier thread is outdated): ledgerCommitted, ledgerStop, ledgerWalk and ledgerHandler are the only seams here without a per-field doc comment. ledgerHandler also replaces the production handler rather than observing or injecting a fault. Add one line per seam saying what it observes or replaces and which boundary it guards.
| BoundSyncUnstarted(ctx context.Context) (bool, error) | ||
| } | ||
|
|
||
| type PageLedgerStore interface { |
Contributor
There was a problem hiding this comment.
🟡 Suggestion (prior — still present, medium confidence; the earlier threads are outdated): PageLedgerStore and PageWriter shipped in v0.35.0/v0.36.0. Here they lose methods (TakeoverToken, BoundSyncFinished, EndSyncWithStats) and gain many, which breaks any out-of-repo implementation or wrapper. The ledger format also gains a downgrade boundary. pkg/sdk/version.go is unchanged at v0.36.0. Make sure the release that carries this is a minor bump and that its notes repeat the PR's downgrade constraint.
Contributor
General PR Review: Drop Checkpoints, and persist Pebble sync pages atomicallyBlocking Issues: 0 | Suggestions: 13 | Threads Resolved: 0 Review SummaryOn the Pebble engine, the syncer stops writing checkpoint tokens. Each page now commits its records, facts, counters and pending-work changes in one batch, and resume reads a durable pending-work queue in windows of at most 100. Legacy checkpoint tokens are migrated into that queue. Seal archives a stats report and drops ledger history by default. SQLite keeps checkpointing.
Criteria triage:
Verdict: HIGH. The review-blind classes are multi-artifact (cross-version) and schedule/crash. The instruments the PR names are present in the diff: crash-image tests at each seal cut, a token-only-SDK open harness, and a real Exported-API criteria: the Security IssuesNone found. Correctness IssuesNone found. Suggestions
Resolved prior findings
Prompt for AI agentsReviewed commit: |
Contributor
There was a problem hiding this comment.
No blocking issues found — see the full review report
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.
Pebble collection pages commit records, facts, accounting and pending-work transitions atomically. Resume reads unfinished work in bounded windows through the existing scheduler. Separate work IDs and revisions allow repeated request tokens to execute normally. Existing checkpoint tokens seed the queue atomically during migration. SQLite continues checkpointing. Flags are locked per phase on both engines: a finished sync accepts only
WithOnlyExpandGrants; on Pebble a resumer whose collection flags differ from the first attempt's recorded options is refused before any write; an expansion-only invocation plans from the file's skip facts and reads none of its own collection flags or targets, on SQLite as well as Pebble.Expansion and external import retain main's handlers and optimized write paths; neither is buffered in a page transaction. Static entitlements materialize one resource page at a time, preserving template order and cold resume.
Successful collection archives a mechanical stats report, drops ledger history by default and purges discarded token data. Explicit ledger debug retains scrubbed history; retaining tokens requires an additional explicit option. Debug logging alone does not enable retention. Attempt metadata keeps first/latest options and folded prior-attempt accounting rather than growing with every retry.
Plain
EndSync()preserves its existing lifecycle meaning: ending a run does not assert collection completed. It saves committed accounting, finalizes indexes, flushes and detaches while preserving data and pending recovery state through close/reopen. Explicit same-ID continuation can use that state. The existing end/cleanup/start-new reset sequence works; starting a new Pebble sync retains main's reset behavior.EndSyncWithStatsremains the syncer's checked completion path.WithConnectorStore(nil)still falls back to the configured file path.Custom sync stores: Pebble requires
PageLedgerStoreand is ledgered; every other engine, including an empty or unknown one, checkpoints through the token path as on main and must not exposePageLedgerStore. Pebble wrappers must forward the capability. There is no Pebble checkpoint fallback. The ledger interfaces gain methods; custom implementations must update accordingly.SDK downgrade constraint: unfinished and early-ended Pebble artifacts can retain the new ledger recovery format. A host reusing those artifacts cannot downgrade its vendored SDK across that format boundary. Keep the host on a ledger-capable SDK, or discard affected recovery artifacts and start fresh before downgrading. This is distinct from rolling back a connector binary without changing the host's SDK.
Validation:
bba86699(v0.30.1) andeb63f1b5pass 16 cases covering single/batched polling, spare off/on, and process kill/reported sync error. The same directory and options survive the version switch; old daemons upload usable data and complete another task, then the new SDK succeeds after roll-forward. Three ordinary repetitions pass; the current harness also passes race checks. This resource-only fixture simulates C1 redelivery and does not claim production workflow coverage.eb63f1b5checkpoint is compared with that SDK's completed artifact and an uninterrupted ledger migration. Twenty-eight crash combinations cover takeover, grant and terminal commits, repeated crashes, WAL/flush recovery and 1/4 workers. Three repetitions and a race run pass, comparing records, indexes, digest, normalized stats and ledger accounting. Dropping imported action accounting makes the test fail.Review guide and performance · Plan/change orders · Evidence and review dispositions
The evidence records executed coverage and residual gaps. It does not claim complete coverage of every original plan product or completion of the original full C49 performance matrix.
Successful page retries preserve every observed connector call, session usage report and reported wait in committed/live totals. The regression test checks exact totals, repeated-token page isolation, failed-attempt record/fact exclusion and close/reopen, with three race repetitions. Retry observations before any successful commit remain best-effort.