phase 6b replay orchestration - #1112
Conversation
kans
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // A resume whose gates degraded to cold (compat drift, withdrawn or | ||
| // swapped previous artifact) must not honor hits recorded by an earlier | ||
| // attempt against a base this attempt never re-validated. | ||
| if !o.s.sourceCacheWarm { |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): this cold verdict has no consumer yet — ridesReplayLadder is pinned false and nothing in-tree handles ErrReplayIntegrity / ReplayVerdictCold (Phase 6c runner work). Because the offending replay verdict lives in a checkpointed EnqueuePageTokens cursor, a degrade that flips sourceCacheWarm to false mid-sync (compat drift on resume, previous artifact becoming ineligible) makes every subsequent resume re-serve the same cursor and fail identically: the sync is permanently stuck rather than degrading to a cold retry. Worth either noting the operational contract in the doc comment or having the drift/degrade path also clear the checkpointed hit-set so the connector's cursors re-plan cold.
General PR Review: phase 6b replay orchestrationBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryScanned the full PR diff (59 files, +10266/-389) for security and correctness, reading every non-test production file end to end: Risk triage per Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
PR-review round on #1112 found the residual half of the checkpoint provenance gap: hits were bare (rowKind, scopeKey) pairs, and no consume gate identifies WHICH artifact a hit came from -- two artifacts from the same connector and config carry identical compat keys, so a previous artifact swapped between attempts passed every gate and a checkpointed hit authorized a replacement copy from a base the connector never revalidated. Hits now record the validator the lookup returned (checkpoint shape: row kind -> scope -> validator), and beforeUpserts requires the current replay base's manifest entry to byte-match it before the copy runs; mismatch, absent entry, read failure, and a base without the entry surface are all cold ErrReplayIntegrity. Four taxonomy cells pin the cold paths; every warm chaos instrument now passes through the binding. Also from the same round: the stuck-resume operational contract for unconsumed cold verdicts is documented on beforeUpserts (6c's ladder automates the cold fallback), the provenance sets' checkpoint cost curve is documented and pinned by BenchmarkStateMarshalSourceCacheSets, and private backend-repo paths in docs/rfcs/0010 are scrubbed per the public-repo content guidelines. Co-authored-by: Cursor <cursoragent@cursor.com>
| // checkpointed and what the validator binds. A later hit for the same | ||
| // scope overwrites: the connector's most recent consult is the one whose | ||
| // verdict its cursors carry. | ||
| func (st *state) RecordSourceCacheHit(rowKind sourcecache.RowKind, scopeKey string, cacheValidator string) { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): the documented last-write-wins overwrite reopens the swapped-base hole in the one case CO-6b-004 targets. If attempt 1 enqueues sibling cursors carrying replay verdicts computed against base A (validator V_A) and then crashes before the planning action completes, attempt 2 re-runs the planning call against swapped base B, and RecordSourceCacheHit overwrites scopes[scopeKey] to V_B. The checkpointed attempt-1 cursors then pass beforeUpserts' binding check (baseEntry.CacheValidator == hitValidator at source_cache_orchestration.go:633) and copy B's rows under a verdict the connector never computed against B — the exact "silently stale rows in a green sync" the binding is meant to reject. Consider keeping the first recorded validator per scope (or failing cold when a re-consult returns a different validator for an already-recorded scope) so the binding stays anchored to the artifact the verdict was actually computed against.
| select { | ||
| case <-store.entered: | ||
| t.Fatal("second replay copy entered the store while the first was mid-flight: decide-copy-mark is not atomic per scope") | ||
| case <-time.After(300 * time.Millisecond): |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): this is the only instrument that pins the decide-copy-mark race itself (the chaos test admits its duplicates after the parent marked the scope, as plan.md now records), and its verdict rests on a 300 ms wall-clock window. Its failure direction is a silent pass: on a loaded runner under -race, a mutex-less second goroutine that simply isn't scheduled within 300 ms makes the test green. Worth either signaling from inside beforeUpserts that the second call reached the guard (so the window measures store entry, not goroutine startup) or recording the false-negative direction in the test comment, since evidence.md presents it as the mutation-adequate replacement for the vacuous chaos cell.
9951158 to
c33f698
Compare
| } | ||
| mu := o.s.sourceCacheScopeLock(o.rowKind, o.scopeKey) | ||
| mu.Lock() | ||
| o.held = mu |
There was a problem hiding this comment.
🟡 Suggestion: after the CO-6b-006 extension, every scoped page — record-only included — holds this mutex from beforeUpserts through afterUpserts, so the critical section now spans the entire page handler. In syncGrantsForResource that includes s.getResourceFromConnector (a connector RPC per unresolved related resource, syncer.go:2923) and the grant-discovered-resource writes; in syncResources it includes the per-resource store.GetResource loop and getSubResources. Net effect: at WithWorkerCount > 1, all pages sharing a (rowKind, scopeKey) are fully serialized behind network latency, so a connector that scopes a whole resource type loses intra-scope parallelism entirely. BenchmarkSourceCacheScopeLocks measures map access, not hold time, so nothing in the evidence bounds this. Consider narrowing the lock to the decide-copy-mark plus tombstone/publish windows (with an explicit "record page in flight" marker to keep the N1 interleaving closed), or at minimum document the serialization contract on the field and add a contention benchmark. (medium confidence on impact magnitude; high confidence on the mechanism)
| RecordSourceCacheHit(rowKind sourcecache.RowKind, scopeKey string, cacheValidator string) | ||
| SourceCacheHitValidator(rowKind sourcecache.RowKind, scopeKey string) (string, bool) | ||
| MarkSourceCacheReplayed(rowKind sourcecache.RowKind, scopeKey string) | ||
| SourceCacheReplayed(rowKind sourcecache.RowKind, scopeKey string) bool |
There was a problem hiding this comment.
🟡 Suggestion: sync.State is an exported interface, and these four methods are added to it without a versioned/adapter shape — any out-of-tree implementation (test doubles, alternate state backends) stops compiling. pkg/sdk/version.go is unchanged in this PR and the description doesn't call the interface change out, which the repo's own criteria ask for on signature-level breaks. Either note it as a deliberate break with a version signal, or keep State frozen and expose the provenance accessors on a separate optional interface that *state satisfies. (medium confidence — the blast radius depends on whether anything outside this repo implements State)
| // FUNCTIONALITY — see pkg/sourcecache). Never nil: when source-cache | ||
| // replay is disabled or degraded this is sourcecache.NoopLookup and | ||
| // every lookup misses. | ||
| Lookup sourcecache.Lookup |
There was a problem hiding this comment.
🟡 Suggestion: the "Never nil" guarantee is only enforced by pkg/connectorbuilder's four call sites (b.sourceCache() substitutes NoopLookup). SyncOpAttrs is an exported struct that connector repos construct directly in their List/Grants unit tests, and a zero value leaves Lookup as a nil interface — a connector following the doc comment and calling opts.Lookup.LookupPreviousSourceCache(...) panics there. Consider either a nil-safe accessor method on SyncOpAttrs or softening the comment to "populated by the connector builder; nil-check when constructing SyncOpAttrs yourself". (low confidence on severity, high on the mechanism)
| // path — evaluates it at test end; any scenario that errors a scoped | ||
| // page between the lock's acquire and release trips it if a handler | ||
| // loses its backstop. | ||
| if concrete, ok := sdkSyncer.(*syncer); ok { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): this ride-along is the instrument that structurally retires the lost-release() class, but the if …, ok := sdkSyncer.(*syncer); ok guard makes it fail open — if the assertion ever stops holding, every chaos suite silently stops evaluating the invariant with no signal. NewSyncer returns *syncer today (syncer.go:4573), so the guard buys nothing; concrete, ok := …; require.True(t, ok, "…") would make a future wrapper return a build/test failure instead of a quietly disarmed instrument.
Single squashed implementation commit, rebased onto main after the formal verification track (#1117) landed separately. Contains the 6b branch through CO-6b-007 plus the production fixes made during the formal effort: - Capability parsing, warm/cold lookup installation with the deliverability probe (CO-6b-001), compat record write/gating, selection fingerprint, checkpointed hit/replay provenance with the warm gate, per-scope locks (CO-6b-004/006), warm-vs-cold ErrReplayIntegrity verdict taxonomy, CO-017 cross-version fold fence. - Record-round grounding (finding 0 of formal/REPORT.md): a record round is a replacement listing, so a partition holding rows no completed round published is cleared before the round's first write (groundRecordScope + engine ClearSourceCacheScope), witnessed by TestChaosSourceCacheRecordFlipOverReplayDebris. - CO-6b-009: session persistence semantics pinned contractually; wholesale resume-clear rejected as unsound. - Test-only commit-order trace recorder (sync_trace_audit.go) and the chaos trace oracle exporting the JSONL fixtures judged by the Occult trace bridge (formal/occult/TRACE_BRIDGE.md). - Source-cache chaos suites wired into chaos-check; verification packet under docs/verification/sync-replay-6b/; RFC 0010. Co-authored-by: Cursor <cursoragent@cursor.com>
909375d to
e06c2b9
Compare
| if o.s.state.SourceCacheReplayed(o.rowKind, o.scopeKey) { | ||
| // Duplicate page / lost-response retry: the copy already ran this | ||
| // sync. Replay is replacement, so re-running it would also wipe | ||
| // overlay rows upserted since. Skip the copy; apply the page's | ||
| // upserts/tombstones normally. | ||
| l.Debug("source-cache replay already completed for scope this sync; skipping copy", | ||
| zap.String("row_kind", string(o.rowKind)), | ||
| zap.String("scope_key", o.scopeKey), | ||
| ) | ||
| } else { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): the replay branch never consults sourceCacheScopeGrounded, so the record→replay direction is unguarded while replay→record is. If a record page for scope S runs first this attempt (grounding + row puts, no validator yet) and a later page for the same scope carries SourceCacheReplay, SourceCacheReplayed(S) is still false, so the replacement copy runs and silently discards the record round's fresh rows — a stale-over-fresh blend with no error and no warn, which is exactly the outcome the rest of this file fails loud on. Consider treating "scope already grounded this attempt but not yet replayed" as a cold ErrReplayIntegrity (or at minimum a warn) alongside the existing provenance checks.
| // action — even on syncs that end up consuming cold. A capable | ||
| // connector on a store without the source-cache surface runs as if the | ||
| // capability were absent (replay is Pebble-only by design). | ||
| s.sourceCacheStore = nil |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): this resets s.sourceCacheStore per attempt and the teardown resets s.sourceCacheWarm, but s.sourceCacheScopeGrounded and s.sourceCacheScopeLocks are never cleared. Sync elsewhere explicitly contemplates "a reused syncer" (the ClearIngestInvariantVerification comment), and on a second Sync call the stale grounded set would suppress groundRecordScope for every scope grounded by the first sync — skipping exactly the debris clear CO-6b-008 added. No in-tree caller reuses a syncer today, so this is latent; resetting both maps here (s.sourceCacheScopeGrounded = native_sync.Map{} etc.) makes the per-attempt reset symmetric with the two fields that already are.
| RecordSourceCacheHit(rowKind sourcecache.RowKind, scopeKey string, cacheValidator string) | ||
| SourceCacheHitValidator(rowKind sourcecache.RowKind, scopeKey string) (string, bool) | ||
| MarkSourceCacheReplayed(rowKind sourcecache.RowKind, scopeKey string) | ||
| SourceCacheReplayed(rowKind sourcecache.RowKind, scopeKey string) bool |
There was a problem hiding this comment.
🟡 Suggestion (compatibility): State is exported, so adding four methods breaks any downstream implementation at compile time. pushAction a few hundred lines below exists specifically to avoid "changing the exported State interface", so this deviates from the file's own precedent. dotc1z.SourceCacheStore likewise gains three methods (ClearSourceCacheScope, PutSourceCacheCompat, GetSourceCacheCompat), and pkg/sdk/version.go is unchanged at v0.27.0. There are no in-tree external implementers of either interface, so practical risk is low — but per the repo's SDK criteria, either carry the new provenance accessors on the concrete *state (as pushAction does) or land the interface widening with a 0.x minor bump as the compatibility signal.
1 participant
sync: source-cache replay orchestration (Phase 6b)
Connectors that can cheaply revalidate upstream data (ETags, delta tokens)
can now skip refetching unchanged scopes: the SDK hands them their previous
validator, and on "unchanged" replays the previous sync's rows locally
instead of paging them from the API.
Core invariant: a warm sync produces the identical artifact a cold sync
would have, or fails loudly with a warm/cold
ErrReplayIntegrityverdict —never a silent blend of stale and fresh rows. Replay is gated on artifact
eligibility (Pebble, finished FULL, clean quality, byte-matching compat
record, materialization witness) and same-sync lookup provenance; anything
off degrades to a cold fetch.
Not yet live in production: subprocess connectors can't receive the
lookup, so capable connectors produce (stamp rows, publish validators) but
consume cold. Phase 6c adds cross-process delivery and the runner retry
ladder.
Verified per
docs/verification/sync-replay-6b/plan.md(frozen beforecode) — chaos suites over the real syncer and stores with differential
oracles against cold baselines, interruption/resume, generational chains,
all
-race; plus three independent reviews whose findings are fixed andinstrumented (CO-6b-003). Evidence in
evidence.md. Hot-path cost when thecapability is absent: ~100ns per page.