Repository navigation
Conversation
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if s.caps.pageLedger != nil { | ||
| s.storeAttachErr = fmt.Errorf("sync store engine %q must not implement PageLedgerStore", store.Metadata().Engine) | ||
| } | ||
| if c1zstore.Engine(store.Metadata().Engine) != c1zstore.EnginePebble || s.caps.pageLedger == nil { |
There was a problem hiding this comment.
🟡 Suggestion: This changes what WithConnectorStore accepts. Before this PR, a store whose Metadata().Engine was "" or unknown (mocks, in-memory wrappers, gRPC-backed stores) ran the token path. Now Sync fails for those stores. Every in-repo implementation is Pebble, so this only affects downstream custom c1zstore.Store implementations. Per the repo criteria, the PR body should name the 0.x minor bump and include a one-line migration note for custom-store callers. (confidence: medium)
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
withLedgerCaps wraps a test double with the PageLedgerStore and WriteHookStore of a real Pebble store so setStore attaches it as ledgered; withTripwireLedgerCaps reports Pebble with nil ledger embeds for doubles that must never reach the ledger. Co-authored-by: Cursor <cursoragent@cursor.com>
setStore attached any engine and set ledgered only for Pebble with a PageLedgerStore. Pebble is the only engine, so the flag was constant true in production. Make the requirement explicit (storeAttachErr otherwise) and delete the field, its branch sites, and the token-path arm each site guarded: Checkpoint and its callers, the legacy Sync body, and the legacy SyncResourceTypes/SyncTargetedResource/SyncResources/ SyncEntitlements/SyncStaticEntitlements/SyncAssets/SyncGrants bodies, which now forward to their ledger handlers. Co-authored-by: Cursor <cursoragent@cursor.com>
The token-path bodies of resource, entitlement, grant, and asset collection had no caller once setStore required a Pebble page ledger. Remove them and the helpers only those bodies reached. Co-authored-by: Cursor <cursoragent@cursor.com>
setStore rejects a store that hides PageLedgerStore or reports an empty engine. Wrap those doubles with withLedgerCaps or withTripwireLedgerCaps, and forward the delete interfaces the double itself implements so capability resolution still sees them. Drop the checkpoint-token hooks; that writer is gone. Co-authored-by: Cursor <cursoragent@cursor.com>
Bare syncParallel and the legacy checkpoint tests called arms that require no page ledger. The ledger scheduler and takeover tests cover that behavior. transitionActionState now returns an error unless the context carries a ledger commit; production reaches it only from that commit. Co-authored-by: Cursor <cursoragent@cursor.com>
NewSyncer returns the attach error for a non-Pebble engine and for a Pebble store that does not expose PageLedgerStore. Tests that forced the token path now run the ledger path. Comparisons that existed only to match that token path are gone. Co-authored-by: Cursor <cursoragent@cursor.com>
The legacy token handler was the oracle for external selection and static entitlement materialization. Its output on origin/main at 8f8783c is stored under testdata/ledger_golden, and the ledger path must reproduce those lists. Co-authored-by: Cursor <cursoragent@cursor.com>
6d454d8 to
81dd6ca
Compare
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
The invokeActionPage fallback that returned the handler when the syncer was not ledgered is gone, so these tests' handlers never ran. TestParallelBatchFailureDuringStopStaysQuiet passed vacuously: there was no cancel-cause line to suppress. TestParallelBatchSecondGenuineFailureStillLogs missed the second warn when the first worker's queue.abort won the race. Co-authored-by: Cursor <cursoragent@cursor.com>
Superseded — see the current review report for commit
|
|
|
||
| _, err := s.syncParallel(preBatchCtx, newTestRetryer(ctx), []*Action{action}, f) | ||
| _, err := s.syncParallel(preBatchCtx, newTestRetryer(ctx), s.run.peekMatchingActions(ctx, SyncGrantsOp), f) | ||
| require.True(t, ran.Load()) |
There was a problem hiding this comment.
🟡 Suggestion: The log check below this line (line 389) looks for an ErrorLevel entry, but parallel_syncer.go:844 logs this message with l.Warn. That means the assertion can never fail, and the test would still pass if the stop-suppression guard were removed. The same mismatch is at line 236. Assert on zapcore.WarnLevel (or on any level) so the test pins the suppression its doc comment describes. (confidence: high)
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
There was a problem hiding this comment.
Nice cleanup, nothing blocking from me. A few nits inline. Some leftovers outside the diff too:
pushChildResourceActions/childResourceActions/childSchedule.recordIfNeware test-only now, and the doc still says "Factored out of getSubResources" (that's gone)runState.transitionActionhas no prod callers anymore- stale refs:
Syncdoc comment still says we checkpoint runState into the datasource, crash-harness comments mentionminCheckpointInterval,run_state.go:166says "legacy path", and the span inledger_assets.gois still namedsyncer.syncAssetsForResource
On your transitionActionState question: I couldn't find a prod path without the commit key. Every handler runs inside invokeActionPage, and commitFn builds commitCtx off the outer ctx, so it always hits the commit arm.
| @@ -166,10 +157,6 @@ func TestLedgerStaticMaterializationOrderAndResume(t *testing.T) { | |||
| for _, ent := range response.GetList() { | |||
There was a problem hiding this comment.
this one lost its proto.Equal oracle and didn't get a golden like the others. Now it only checks len + DisplayName, so an ID/slug/annotations regression would slip through. Maybe dump a static_materialization.json from main and use assertLedgerGolden? Or at least diff cold-resume against uninterrupted.
There was a problem hiding this comment.
Done in 88fefc9: the uninterrupted subtest's ListEntitlements result is held in the parent and the cold-resume subtest requires equalProtoLists against it (sorted by ID), keeping the len/DisplayName checks. Mutation check: appending to Description on the resume page fails on record 0 while len and DisplayName still pass.
| if s.caps.pageLedger != nil { | ||
| s.storeAttachErr = fmt.Errorf("sync store engine %q must not implement PageLedgerStore", store.Metadata().Engine) | ||
| } | ||
| if c1zstore.Engine(store.Metadata().Engine) != c1zstore.EnginePebble || s.caps.pageLedger == nil { |
There was a problem hiding this comment.
small thing on the PR description: with WithConnectorStore this error shows up at NewSyncer, not at the start of Sync (only WithC1ZPath defers it to loadStore). Also dotc1z.RegisterEngine still advertises adding engines, and the syncer will now reject those. Worth a line in its doc.
There was a problem hiding this comment.
Body fixed (NewSyncer with WithConnectorStore, loadStore with WithC1ZPath). RegisterEngine doc names the setStore requirement in 7bd1201.
| @@ -799,20 +750,15 @@ func (s *syncer) transitionActionState( | |||
| nextPageToken string, | |||
| childActions []Action, | |||
| ) ([]*Action, error) { | |||
There was a problem hiding this comment.
the []*Action is always nil now, so this could just return error. Right now admitLocked(batchOp, pushed) in the queue always gets nil.
There was a problem hiding this comment.
6540e24: returns error now. admitLocked keeps its slice parameter because TestParallelQueueExhaustiveInterleavings returns same-op children through the commit callback; production passes nil.
| // checkpointOnStop persists run accounting on a context detached from the | ||
| // caller's cancellation. Failures are logged, never returned, and the stop | ||
| // reason reaches the runner unchanged (RFC 0009 §4.2). | ||
| func (s *syncer) checkpointOnStop(ctx context.Context) { |
There was a problem hiding this comment.
this is just an alias for checkpointLedgerOnStop now, and both names get used side by side in the same select. I'd inline it so nobody goes looking for a difference.
There was a problem hiding this comment.
383f0fc: inlined, checkpointLedgerOnStop is the one name.
| cancel(err) | ||
| } | ||
| return loaded, err | ||
| queue.commitUnlocked = true |
There was a problem hiding this comment.
always true now, so the locked branch in transition only runs in ledger_queue_commit_test. The field doc ("Set only for a ledger batch") and the "token path performs here" comment in transition read like there are still two modes. Drop the flag or at least fix the comments?
There was a problem hiding this comment.
cdf1cad: flag removed, transition always releases q.mu around the commit. The stepper in ledger_queue_commit_test ran the locked branch; it now asserts only that next() completes during the commit.
Superseded — see the current review report for commit
|
General PR Review: sync: require the page ledger and delete the token-checkpoint sync pathBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThis PR deletes the token-checkpoint sync path. The new commits since
I scanned the full PR diff for security and correctness, and audited both prior findings against the current code. Repo criteria triage for the new commits:
Verdict for this increment: LOW. The PR as a whole still carries the default-path behaviour change tracked below. Security IssuesNone found. Correctness IssuesNone found. Suggestions
Resolved prior findings
Prompt for AI agentsReviewed commit: |
There was a problem hiding this comment.
No blocking issues found — see the full review report
Every production sync has run through the page ledger since it landed; the
s.ledgeredflag and the token-checkpoint handlers behind it were dead. This removes the flag, makes the ledger a hard requirement ofsetStore, and deletes the legacy bodies.What changed
syncer.go(4453 → 3169 lines):setStoresetsstoreAttachErrunless the store is Pebble and implementsc1zstore.PageLedgerStore. It is a plain error, notErrArtifactUnusable, because the c1z is fine; the caller wired the wrong store.Checkpoint,checkpointInterval,lastCheckPointTime,testHooks.checkpointHook, the legacySyncbody, and the cascade it alone reached (listAllResourceTypes,getSubResources,syncResources,syncEntitlementsForResource,syncStaticEntitlementsForResourceType,syncAssetsForResource,syncGrantsForResource,filterFreshGrantResource, and friends) are gone.Sync*methods forward tosyncLedger*.transitionActionStatereturns an error when no ledger commit key is present;invokeActionPagealways installs one, so the removed arm was test-only.ingest_filter.go,store_caps.go:unusedcascade only.marshalToken/unmarshalToken,GraphFromToken, compaction tokens) is untouched.ledger_takeover.goandcompaction_token.gostill read it; only the write/checkpoint path died.ledgeredStore/withLedgerCaps/withTripwireLedgerCaps(ledgered_store_test.go) give doubles the ledger capabilitiessetStorenow demands. Tests that existed to compare the ledger path against the token path lost their oracle, so the token handler's output onmainat8f8783c4is captured undertestdata/ledger_golden/andTestLedgerExternalSelectionMatchesGolden(seven selection modes),TestLedgerExternalUsesMainHandler, andTestLedgerStaticEntitlementsMatchGoldenassert against it. Deleting one grant from a golden file fails the matching subtest. Token-path-only scheduler tests are deleted where a ledger twin exists (listed per commit).TestParallelBatchSecondGenuineFailureStillLogsandTestParallelBatchFailureDuringStopStaysQuietbuilt a baresyncer{}and ran through the deleted!s.ledgeredfallback; the first failed 81/200 on this branch (0/200 onmain) because the handler never ran and the rendezvous never happened, the second passed because nothing ran. Both now usenewLedgerSchedulerFixtureand assert the handler ran: 200/200 and 50/50 under-race. Review follow-ups, one commit each:transitionActionStatereturnserror(its action slice was always nil),checkpointOnStopalias inlined,parallelActionQueue.commitUnlockedremoved (always true), the cold-resume materialization test compares against the uninterrupted run withequalProtoLists, and the two no-cancel-log assertions checkWarnLevel, the levelparallel_syncer.gologs at (they checkedErrorLeveland could not fail).What to look at
setStoreand thestoreAttachErrcheck inSync: this is the one behaviour change. A caller passing a non-Pebble or non-ledger store used to fall through to the token path and now gets an error: fromNewSyncerwithWithConnectorStore, fromloadStoreat the start ofSyncwithWithC1ZPath.dotc1z.RegisterEngine's doc now says so.transitionActionState's new error return: confirm with me if you can see a production path that reaches it without a ledger commit key.main, not hand-written.How checked
go vet,golangci-lint run ./pkg/sync/...0 issues;go test ./pkg/sync/... -count=1: sync 1026s, expand 208s, all ok. Golden mutation check as above.