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: 1 | Threads Resolved: 0 Review SummaryThe full PR diff was scanned for security and correctness (including Security IssuesNone found. Correctness IssuesNone found in the new commit. Standing (previously reported, unchanged): 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.
| // connector's client forwards to a setter that is nil for subprocess | ||
| // transports). False means the connector will observe NoopLookup no matter | ||
| // what the syncer delivers, so install must not report warm. | ||
| type sourceCacheLookupDeliverable interface { |
There was a problem hiding this comment.
🟡 Suggestion (confidence: medium): the syncer still type-asserts against this private duplicate (line 347), not the newly-promoted sourcecache.LookupDeliverabilityProbe, so the var _ compile pin added in internal/connector/connector.go:95 doesn't pin what actually drives the decision. A coordinated rename of SourceCacheLookupDeliverable on both the interface and connectorClient keeps the pin green while silently severing this assertion — exactly the false-warm reopening CO-6b-005 claims to close (evidence.md: "a method rename cannot silently sever it from the syncer's type assertion"). Deleting sourceCacheLookupDeliverable and asserting s.connector.(sourcecache.LookupDeliverabilityProbe) at line 347 makes the pin load-bearing.
| 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.
Wire the end-to-end source-cache replay contract frozen in docs/verification/sync-replay-6b/plan.md: SourceCacheCapability parsing and gating, lookup install/teardown with a transport deliverability probe (CO-6b-001: no production transport delivers this phase; such syncs run cold and never log warm), compat-record lifecycle with byte-match consume gating, fresh-page recording with scope stamping, replay handling with provenance enforcement — the sourceCacheWarm gate, hit-validator binding to the replay base, and a per-scope page lock covering replay copies AND record-page application — the warm/cold ErrReplayIntegrity verdict taxonomy, the CO-017 cross-version fold fence, produce-side guards blocking unreplayable shapes (child resource types, InsertResourceGrants) from seeding warm generations, and a sync token schema fence (reshaped fields take new keys; versioned tokens that fail to parse error loudly instead of downgrading to v0). Verified by the chaos-connector suites (gate matrix, collection semantics, ordering/pagination, interruption/resume, generational steady state), a bounded verdict-taxonomy enumeration, engine-seam sentinel-identity tests, benchmarks pinning the disabled-path and checkpoint-marshal costs, and mutation-verified instruments for every load-bearing guard. Three independent review rounds drove the hardening recorded as change orders CO-6b-003 through CO-6b-006; docs/verification/sync-replay-6b/evidence.md carries the full closure record, registered exclusions, and explicit limitations. Co-authored-by: Cursor <cursoragent@cursor.com>
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)
…e triage, witness pin Closes both round-4 MAJORs: the chaos fixture now asserts no source-cache scope lock is held at sync end (with per-handler loud-failure cells, each defer site mutation-verified), and evidence.md gains the structural- coverage triage ledger with six new instruments for the branches it judged worth real tests. Minors: MaterializationWitnessReader promoted to a pinned named interface, the syncer asserts the exported deliverability probe, the shrunk-type-list permutation is a registered exclusion, and the token-fence/lock-hold/lock-cardinality wording is corrected. Co-authored-by: Cursor <cursoragent@cursor.com>
| // 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.
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.