phase 6a - replay functionality [source cache-scoped] - #1045
Conversation
kans
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| var scanned int | ||
| for primaries.First(); primaries.Valid(); primaries.Next() { | ||
| scanned++ | ||
| if scanned&0x3FF == 0 { | ||
| if err := ctx.Err(); err != nil { | ||
| primaries.Close() | ||
| return err | ||
| } | ||
| } | ||
| stamp, err := rawdb.ScanSourceScopeKeyRaw(primaries.Value(), scopeField) | ||
| if err != nil { | ||
| primaries.Close() | ||
| return fmt.Errorf("source cache replay: preflight %s primary %x: %w", rowKind, primaries.Key(), err) | ||
| } | ||
| if stamp != scopeKey { | ||
| continue | ||
| } | ||
| indexKey, ok := rawdb.AppendBySourceScopeKeyFromPrimary(nil, primaries.Key(), scopeKey) | ||
| if !ok { | ||
| primaries.Close() | ||
| return fmt.Errorf("source cache replay: preflight %s primary %x cannot derive source index", rowKind, primaries.Key()) | ||
| } | ||
| _, closer, err := prev.db.Get(indexKey) | ||
| if err != nil { | ||
| primaries.Close() | ||
| return fmt.Errorf("source cache replay: preflight %s primary %x missing source index: %w", rowKind, primaries.Key(), err) | ||
| } | ||
| closer.Close() | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: This preflight iterates the entire primary keyspace of the row kind in prev on every replay call, skipping only the per-primary Get for non-matching stamps. Since validateReplaySourceScope runs once per scope (via each ReplaySourceCache*), a sync that replays S scopes against a previous file of N rows is O(S·N) — a quadratic cliff for delta-query connectors with many small scopes and a large prior sync. The single-scope benchmark (source_cache_replay_bench_test.go puts all rows under one scope) scales linearly and won't detect this. Per the repo cost-contract criteria, consider a multi-scope benchmark that pins the cost curve, and/or bounding the primary-side pass (e.g. persisting a per-scope row count/digest in the manifest so the index-side scan alone suffices). The code comment already acknowledges this as a known limitation. (Confidence: high on the full-keyspace scan; medium on real-world impact — depends on connector scope granularity.)
There was a problem hiding this comment.
This will be done in follow up.
General PR Review: [WIP] phase 6a - replay functionalityBlocking Issues: 0 Criteria status: loaded Review SummaryScanned the full PR diff for security and correctness issues, plus the baton-sdk repo-local criteria (exported Go API stability, proto/wire contracts, serialized-state round-tripping, default-behavior stability, and go.mod/go.sum).
The one real correctness issue found (a Pebble value-handle leak in PutGrantRecordsIfNewer, pkg/dotc1z/engine/pebble/if_newer.go:77-80, where closer from e.db.Get stays open if marshalRecord errors) is already tracked in existing findings and is pre-existing/unchanged since the last reviewed SHA — not re-posted here. The sibling entitlement/resource IfNewer paths close on all paths. No new blocking security or correctness issues were found. The replay machinery is extensively self-verified. Security: None found. |
| if err != nil { | ||
| if errors.Is(err, pebble.ErrNotFound) { | ||
| continue | ||
| } | ||
| return err | ||
| } | ||
| if _, ok := seen[id]; ok { | ||
| continue | ||
| } | ||
| seen[id] = struct{}{} | ||
| identities = append(identities, id) | ||
| } | ||
| if len(identities) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| batch := e.db.NewRecordBatch() | ||
| defer func() { _ = batch.Close() }() | ||
| for _, id := range identities { | ||
| key := encodeGrantIdentityKey(id) | ||
| oldVal, closer, err := e.db.Get(key) | ||
| if errors.Is(err, pebble.ErrNotFound) { | ||
| continue | ||
| } | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := batch.StageGrantDelete(key, oldVal); err != nil { | ||
| _ = closer.Close() |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): DeleteGrantRecordsBounded (and the parallel DeleteEntitlementRecords) accumulate every resolved identity into an in-memory identities slice + seen map and stage all tombstones into a single RecordBatch committed once. Memory is O(len(externalIDs)), unlike the replay path which deliberately chunks at replayBatchRows (10k). The single atomic commit is a deliberate all-or-nothing tombstone choice and the delete set is asserted to be delta-sized (see DeleteSourceCacheRows doc), so this is likely fine — but it's exactly the "cleanup/tombstone scan builds O(scope size) batch" class this PR added to docs/BUG_CATCHING.md. Worth a bound or a note if a whole-scope tombstone can ever reach here.
General PR Review: [WIP] phase 6a - replay functionalityBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryFull PR diff scanned for security and correctness. This is a HIGH-risk change per the repo triage model: it introduces new durable storage families (by_source_scope indexes, the source-cache manifest keyspace TypeSourceCache=0x0B), a new wire-stamped record field (source_scope_key), and cross-file replay that copies raw values between SDK versions — silent + durable + version-pair-dependent, so failures escape local testing and cost a fleet-wide re-sync. The concrete correctness findings below were already identified in the prior review and remain present in the current tree; no new bugs were found, and no security issues were found. Recommend the durable-format and replay paths get the escalated instrument coverage the trusted criteria calls for (a two-artifact cross-version replay harness plus a golden-artifact corpus for the new index/manifest families), which a single-shot CI review only samples. Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
General PR Review: phase 6a - replay functionality [source cache-scoped]Blocking Issues: 0 | Suggestions: 10 | Threads Resolved: 0 Review SummaryThe new commit is a test-tiering refactor only: a new Security IssuesNone found. Correctness IssuesNone blocking. Suggestions
Prompt for AI agents |
kans
changed the title
| func (r SyncRun) UsableAsReplaySource() bool { | ||
| return r.Type == connectorstore.SyncTypeFull && !r.Compacted | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: UsableAsReplaySource() gates on !r.Compacted, but the SQLite adapter (getFinishedSync in pkg/dotc1z/sync_runs.go:306-352) never selects or sets Compacted, so it is always false for a SQLite prior run — a compacted SQLite full sync would pass this gate. Impact is currently bounded (the pebble replay path independently re-checks via validateReplaySourceEligible at source_cache.go:129, and replay is pebble-only), but the syncer comment at syncer.go:3735 says a SQLite prior run should work as a replay source. Consider documenting that Compacted is only meaningful for engines that track it, or having the SQLite adapter conservatively derive compaction provenance. (medium confidence)
Pin the corrected implementation SHA and preserve its final lint, race, soak, coverage, and repository-wide gate evidence for independent re-review. Co-authored-by: Cursor <cursoragent@cursor.com>
Preserve run-file ownership when source cleanup fails and make the remaining mutation, coverage, and API-boundary limitations explicit. Co-authored-by: Cursor <cursoragent@cursor.com>
Pin CO-012a and preserve the repeated repository, race, soak, and structural-profile evidence before the final independent review. Co-authored-by: Cursor <cursoragent@cursor.com>
Record both independent acceptance verdicts and retain an executable removal-error branch test for the final ownership obligation. Co-authored-by: Cursor <cursoragent@cursor.com>
…ing a compacted sync. ensure fold doesnt expolode
Pin the reviewed remediation and preserve the explicit verification limitations so the closure claim is auditable. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the mechanical registry portable so Windows does not report registered commit sites as missing. Co-authored-by: Cursor <cursoragent@cursor.com>
…out and we can organize stuff
| test-extra: race-check compat-check interrupt-check fuzz-smoke differential-check bench-smoke ## Run bounded confidence checks omitted from CI. | ||
|
|
||
| .PHONY: test-nightly | ||
| test-nightly: export BATON_TEST_NIGHTLY=1 |
There was a problem hiding this comment.
🟡 Suggestion: neither new tier has an automated runner. .github/workflows/ci.yaml:39 and main.yaml:56 invoke bare go test ./... with neither BATON_TEST_EXTRA nor BATON_TEST_NIGHTLY set, and there is no schedule:/workflow_dispatch: workflow that calls make test-extra or make test-nightly. As a result the 21 newly gated tests — the full deterministic chaos corpora, scheduler soak, WAL-checkpoint race, C1Z integrity, the randomized source-cache lifecycle model, checkpoint-cut enumeration, exhaustive parallel-queue interleavings, and both differential suites — now run only when a human types a Make target, which is coverage removal rather than relocation. Consider adding a nightly scheduled job that runs make test-nightly.
| func RequireExtra(t testing.TB) { | ||
| t.Helper() | ||
| if os.Getenv(ExtraEnv) == "" && os.Getenv(NightlyEnv) == "" { | ||
| t.Skipf("set %s=1 or run the corresponding Make target", ExtraEnv) | ||
| } | ||
| } | ||
|
|
||
| // RequireNightly skips a randomized, repeated, or full-corpus test unless the | ||
| // nightly confidence suite explicitly enabled it. | ||
| func RequireNightly(t testing.TB) { | ||
| t.Helper() | ||
| if os.Getenv(NightlyEnv) == "" { | ||
| t.Skipf("set %s=1 or run make test-nightly", NightlyEnv) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: both gates test os.Getenv(...) == "", so BATON_TEST_EXTRA=0 (or BATON_TEST_NIGHTLY=false) enables the tier. docs/TESTING.md documents the contract as =1, so a CI matrix or shell profile that sets the variable to 0 in order to opt out would silently opt in instead. Consider accepting only truthy values (an explicit == "1", or strconv.ParseBool on a non-empty value).
| // C10/C12: the replay commit seam supplies deterministic evidence that live | ||
| // batch cardinality is fixed, and lets retry be cut after one landed chunk. | ||
| func TestVerificationReplayBatchBoundAndInterruptedRetry(t *testing.T) { | ||
| testtier.RequireExtra(t) |
There was a problem hiding this comment.
🟡 Suggestion: require.Equal(t, replayBatchRows, highWater) on line 57 is the only assertion anywhere that the live replay path actually honors the production replayBatchRows = 10_000 bound — TestVerificationReplayCommittedPrefixRetryAllKinds lowers the seam to 2, and source_scope_verification_test.go:1113 only exercises validateBatchHighWater in isolation. Gating this test to the extra tier therefore removes CI's only guard against a regression that drops or changes the production batch bound on the new durable replay path. Consider splitting the cheap bound assertion into a CI-tier test and leaving only the 10k-row fixture behind RequireExtra.
| run, metaErr := previousSyncStore.SyncMeta().LatestFinishedSyncOfAnyType(ctx) | ||
| if metaErr != nil { | ||
| closeErr := previousSyncStore.Close(ctx) | ||
| if s.previousSyncC1ZPathOptional { | ||
| ctxzap.Extract(ctx).Warn("previous-sync c1z metadata unusable; syncing without source-cache replay", | ||
| zap.String("previous_sync_c1z_path", s.previousSyncC1ZPath), | ||
| zap.Error(errors.Join(metaErr, closeErr)), | ||
| ) | ||
| break | ||
| } | ||
| return nil, fmt.Errorf( | ||
| "error reading previous-sync c1z %q metadata: %w", | ||
| s.previousSyncC1ZPath, | ||
| errors.Join(metaErr, closeErr), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: this makes NewSyncer hard-fail for WithPreviousSyncC1ZPath callers on a metadata read error, where before this PR no metadata was read at all and the sync proceeded. Note the asymmetry with the branch immediately below — run == nil || !UsableAsReplaySource() only warns and degrades for the same non-optional caller. Since previousSyncReader is currently unconsumed scaffolding (the ETag read path is still t.Skipped in pebble_etag_replay_test.go), this only adds a new way for construction to fail; consider warn-and-degrade here too, reserving hard failure for open failures as the option doc describes. (confidence: medium)
| if mayExist { | ||
| oldScope, err := ScanSourceScopeKeyRaw(oldVal, field) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if oldScope != "" && oldScope != newScope { | ||
| if err := rb.deleteSourceScopeKey(key, oldScope); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: this returns the ScanSourceScopeKeyRaw(oldVal, …) error and fails the whole put, while the delete twin stageSourceScopeCleanup (records.go:452-455) deliberately degrades to deleteAllSourceScopeKeysForPrimary. Net effect: a record whose stored prior value is malformed can never be overwritten (permanent failure) but can be deleted (self-heals) — and before this PR the put path did not parse oldVal at all for grants/entitlements, so overwriting a bad row succeeded. Consider mirroring the cleanup path: on scan error fall back to deleteAllSourceScopeKeysForPrimary(key) and continue to the new-scope Set. (confidence: medium)
| func encodeGrantBySourceScopeIndexKey(scopeKey string, id grantIdentity) []byte { | ||
| key, _ := rawdb.AppendBySourceScopeKeyFromPrimary(nil, encodeGrantIdentityKey(id), scopeKey) | ||
| return key | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the ok return from AppendBySourceScopeKeyFromPrimary is discarded, so a grant identity that fails to splice yields a nil key that flows on as a real index key. Every other splice site in this PR (setByPrincipalKey, deleteSourceScopeKey, stageSourceScopeChange) turns !ok into an error. Either propagate the bool or panic on !ok, rather than relying on downstream len(k) < 3 checks to catch it. (confidence: medium)
| closeErr := w.Close(ctx) | ||
| if closeErr != nil { | ||
| l.Error("compactPebble: error closing source store", zap.Error(closeErr), zap.String("file", sourcePath)) | ||
| } | ||
| if err := joinSourceStoreCloseError(selectErr, closeErr, sourcePath); err != nil { | ||
| return err |
There was a problem hiding this comment.
🟡 Suggestion: source stores here are read-only inputs that have already been fully consumed (SourceFile carries only Path/SyncID/Stats). Joining their Close error into the return — same at compactor_pebble.go:561-570 — changes previously log-only behavior into a hard compaction failure, so a transient temp-dir/unlink hiccup can now fail an otherwise correct compaction. Consider keeping these logged and reserving the join for destination-side close errors. (confidence: high that the behavior changed, medium that it matters in practice)
| t.Helper() | ||
| if os.Getenv(ExtraEnv) == "" && os.Getenv(NightlyEnv) == "" { | ||
| t.Skipf("set %s=1 or run the corresponding Make target", ExtraEnv) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: the gate is != "", so BATON_TEST_EXTRA=0 (or =false) enables the extra tier — the opposite of what the value suggests, and the skip message tells the reader to set =1. Same shape in RequireNightly. Consider comparing against "1" (or parsing with strconv.ParseBool) so an explicit off value turns the tier off. (confidence: high, low impact)
1 participant
Summary
Implements Sync Replay Phase 6a for the Pebble-backed
dotc1zstore, and secondarily, runs the experiment of using a semi-formal verification plan, created independently of the implementation.This adds:
Verification record
This PR was developed and tested against an implementation-blind verification plan.
docs/verification/sync-replay-6a/plan.mddocs/verification/sync-replay-6a/evidence.mdThe plan defines C01–C43, their oracles, coverage levels, failure models, and closure requirements. The evidence record states exactly which criteria are verified, sampled, incomplete, excluded, or deferred.
Verification-driven fixes
The verification work found and corrected defects involving:
Scoped tombstone operations now commit in bounded chunks. If a later chunk fails, they report only committed progress, preserve primary/index agreement, mark the store dirty, and converge on retry.
Change orders
The committed plan records four post-freeze change orders:
Deliberately deferred
This PR does not implement:
Executable exclusions identify API-boundary cells that Phase 6a cannot represent. An exclusion is not counted as a behavioral pass.
Evidence
Passing gates: