c1z replay#1002
Conversation
|
|
2c28a48 to
ae0ccd4
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
General PR Review: c1z replayBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness, using the GitHub PR file patches as the authoritative diff (the recorded base has advanced past the PR branch point, so a local base..head diff spuriously surfaces Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents``` SuggestionsIn `pkg/connectorbuilder/resource_syncer.go`:
In `pkg/sync/ingest_invariants.go`:
In `pkg/dotc1z/source_cache.go`:
|
5fd87b4 to
b00db4e
Compare
b00db4e to
79f5efd
Compare
87b604c to
be70f42
Compare
9a5647d to
194e255
Compare
Connector-driven source-cache replay for pebble c1z stores: scope manifests with ETag revalidation, engine-side row replay with overlay and delete tombstones, continuation (ask/answer) lookups for single-shot transports, type-scoped grants via SpawnCursors, sync run stats, and the replay-equivalence differential harness together with the replay side-effect bug fixes it drove out (child scheduling, external-match grants, related-resource rows, exclusion groups, continuation caps). Co-authored-by: Cursor <cursoragent@cursor.com>
…g cache invalidation
dee4cbc to
3d9e7dd
Compare
3d9e7dd to
8c090ac
Compare
8c090ac to
0b83cab
Compare
| if deferred { | ||
| b.m.RecordTaskSuccess(ctx, tt, b.nowFunc().Sub(start)) | ||
| // Session usage rides the ask too: the phase-1 handler may have | ||
| // touched the session before deferring, and the protocol turn is | ||
| // the only response that work will ever produce. | ||
| return v2.ResourcesServiceListResourcesResponse_builder{Annotations: appendSessionUsage(askAnnos, sessionUsage)}.Build(), nil |
There was a problem hiding this comment.
🟡 Suggestion: On the deferred (ask/answer) path this returns a successful response with nil error, but the function-scoped err still holds the wrapped ErrLookupDeferred. The defer uotel.EndSpanWithError(span, err) reads err at return, so every normal protocol bounce is recorded on its trace span as an error. Set err = nil before returning here (same for the entitlements/grants deferred blocks around lines 487 and ~584). (confidence: high)
| "(resource type synced: %t — false means a config gap, true a magic-id construction bug)", | ||
| rt, rid, probe.typeExists(ctx, rt)) | ||
| } | ||
| n, ids, err := facts.DeleteEntitlementsForResource(ctx, rt, rid, maxDanglingIDExamples-len(entIDExamples)) |
There was a problem hiding this comment.
🟡 Suggestion: maxDanglingIDExamples-len(entIDExamples) can go negative if a prior call returns more ids than requested, and the negative cap is passed into the store. The sibling checks (checkGrantEntitlementReferences/checkGrantPrincipalReferences) guard with an explicit < maxDanglingIDExamples check; clamp this to max(0, ...) for parity. (confidence: low)
General PR Review: c1z replayBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe full PR diff (155 files, ~+29k) was scanned for security and correctness across the new source-cache replay, type-scoped listing, and ingestion-invariant machinery, with deep reads of the sourcecache/connectorbuilder, dotc1z + pebble engine, and sync/tasks/compactor clusters. No new blocking security or correctness issues were found: the proto changes are purely additive (new field numbers 9/10/11 and new messages, no renumbering, type changes, or removals) with matching regenerated pb/; the new TypeScoped*Syncer interfaces are opt-in (type-asserted and wired through newResourceSyncerV1toV2), so existing connectors keep compiling; and the serialized-state/compat records degrade absent-or-mismatched artifacts to a cold sync rather than stranding in-progress syncs. Source-cache replay is gated to full syncs, Pebble-only, non-compacted, with ErrReplayIntegrity classification driving cold-retry attribution. The three prior findings (span-error on the deferred ask/answer path, I7 negative example clamp, ApplyTombstones MarkDirty ordering) remain valid and are not re-flagged here. Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents``` SuggestionsIn pkg/sdk/version.go:
|
Replay: scope pre-clear makes resumed copies idempotent (count check stays exact); lookup slot is sync-aware so concurrent syncs can't cross-wire validators; compaction artifacts from pre-flag binaries are gated by their provenance token; I6 is gated off for compacted runs. Drops: I8/I9 exemptions judged per grant (machinery rows can't shield connector rows); deletes stream chunked batches instead of per-row fsyncs; IngestFactStore renamed IngestInvariantStore to match what it now does. Local runner keeps the user-supplied previous-sync file and names it in an actionable error instead of deleting it. Tests: warning-silence oracle in the equivalence harness (invariant warnings are verdicts nobody asserts on — now they fail clean scenarios); mixed-population exemption fixtures; replay-resume, mixed-version, and drop-scale tests; deferred ask/answer turns no longer mark trace spans as errors.
| _, err := p.s.store.GetResourceType(ctx, reader_v2.ResourceTypesReaderServiceGetResourceTypeRequest_builder{ | ||
| ResourceTypeId: resourceTypeID, | ||
| }.Build()) | ||
| exists := err == nil | ||
| p.known[resourceTypeID] = exists | ||
| return exists |
There was a problem hiding this comment.
🟡 Suggestion: typeExists collapses every GetResourceType error into "type does not exist" (exists := err == nil). A non-not-found error (transient I/O, unmarshal failure, ErrNoCurrentSync) is then memoized as "type not synced", which in default mode flips the routing at lines 590/682/782 from the loud unexpectedDanglingError (→ ErrReplayIntegrity on warm syncs) into the silent DROP arm — the exact silent-row-drop these invariants exist to prevent. Note the sibling existence check HasResourceRecord (line 577) correctly propagates errors. Consider distinguishing a genuine not-found (pebble.ErrNotFound / gRPC NotFound) from other errors and failing the check on the latter, only memoizing confirmed absence. (confidence: medium — severe if triggered, but requires a non-not-found error from a local store read.)
…emory stamp unreachable) Co-authored-by: Cursor <cursoragent@cursor.com>
sync.Pool randomly discards Puts under -race, same as the other two pool-reuse assertions already guarded in this file. Co-authored-by: Cursor <cursoragent@cursor.com>
|
This work is being broken up into multiple, smaller, PRs. The first two have already been merged. |
1 participant
POCs:
ConductorOne/baton-okta#182
ConductorOne/baton-github#177
https://github.com/ConductorOne/baton-sharepoint/pull/72
https://github.com/ConductorOne/baton-microsoft-entra/pull/164
PR: c1z source-cache replay, type-scoped listing, and ingestion invariants
122 files, +22,960/−661. Pebble-only where noted; SQLite artifacts are
untouched by every new mechanism (pinned by tests).
What this adds
Source-cache replay
Connectors that can answer "has this scope changed?" cheaply (etags,
delta tokens) can now skip re-fetching unchanged data. The previous
sync's c1z acts as the cache: rows are stamped with a connector-chosen
scope key at ingest, a manifest records (scope, cache validator) pairs,
and on the next sync the connector is handed a lookup — on a validator
match it returns an empty page with a
SourceCacheReplayannotation andthe engine copies the scope's rows (grants, entitlements, or resources)
from the previous file directly. Delta-capable APIs (e.g. MS Graph) use
overlay rounds: replay the base, apply changed rows and tombstones
(canonical-id and principal-scoped), rotate the token.
Gating: full syncs only, Pebble only, and never from compacted
artifacts (compaction is an upsert-merge, so no input's validator can
vouch for the merged row set — compacted files carry a
compactedflagand no manifest entries, and every lookup against them misses).
Failure handling: any evidence that replay lost or corrupted rows
(copy failures, count mismatches against the scope index, stale-index
refusals, unrepairable I3 violations, previous-file read errors) is
classified under an
ErrReplayIntegritysentinel. Both task runnersrespond by discarding the output, retiring the previous-sync file, and
re-running the sync cold. The retry doubles as attribution: a clean
cold run means replay was at fault and the sync self-healed; a cold run
that still fails is connector data, and those failures deliberately do
not carry the sentinel, so a recurring connector bug fails plainly
instead of looping warm-attempt-plus-retry forever.
Type-scoped grants and entitlements
Resource types annotated with
TypeScopedGrants/TypeScopedEntitlementsare enumerated once per type instead of once per resource. The request
carries the marker annotation over a
{type, type}stub resource;connectors can fan out shards via
EnqueuePageTokens(each tokenbecomes an ordinary checkpointed page action). The connectorbuilder
enforces registration coherence: the annotation and the corresponding
syncer interface must be declared together. This composes with
source-cache scopes, which is what makes chunked delta APIs (50-id
Graph chunks) practical.
Ingestion invariants (I1–I9)
Replay added a second ingestion path, which turned every side effect
attached to the connector-response loop into a latent bug class. This
branch replaces stream-coupled side effects with store-derived
invariants evaluated at consuming seams — each is a pure function of
store contents, so stream, replay, resume, expansion, and external-match
ingestion all yield identical verdicts:
repaired from the previous sync's copy; unrepairable ⇒
ErrReplayIntegrity.bad trade at whale scale for a warning).
pass (engine-agnostic; replaces both old arm sites).
grant→entitlement, grant→principal — via prefix-skip scans that are
O(distinct referents), never O(rows).
WithFailFastInvariants()(tests, the equivalence harness) turns everyinvariant verdict into a hard failure with the offending reference
named; production default warns/repairs/drops.
Dangling-reference drops
Production data motivated going further than warning: the expansion
path alone logs ~2.9M missing-entitlement edges per week, dominated by
connector magic-id bugs and disabled-by-default resource types, and the
platform's uplift provably discards referentially-dangling rows. In
default mode I7/I8/I9 drop the dangling rows at the post-collection
seam under one rule: connector-owned dangling references are dropped;
SDK-machinery-owned shapes are kept (
InsertResourceGrantsgrants,unprocessed
ExternalResourceMatch*carriers; I3 repairs rather thandrops). Each drop emits a single aggregated warning with totals, capped
id examples, and a config-gap vs magic-id-bug attribution based on
whether the referenced resource type was synced at all. Deletes stream
through chunked batches (measured: 100k rows in 171ms). Full rationale
and safety argument:
docs/tasks/dangling-reference-drops.md.Verification machinery
proving warm syncs produce semantically identical artifacts to cold
syncs (grants/resources/entitlements oracles, manifest and scope-index
comparison, multi-page scopes, deletion mutations, model-derived
grant oracle), plus an env-gated 20-seed soak.
(replay-copied, rows-committed, tombstones-applied, manifest-written)
with resume verification.
57.7M-grant post-expansion artifact — production invariant overhead
totals ~1.5s of scans plus 3–5s for I9, against a multi-minute
expansion phase; the rejected O(grants) design measures 170× worse.
baton source-cache: CLI audit of manifest entries, scope-indexcounts, validators, and orphan scopes (non-zero exit on I6 violations).
Proto changes
New annotations (
SourceCacheCapability,SourceCacheRecord,SourceCacheReplay,SourceCacheLookup*,TypeScopedGrants,TypeScopedEntitlements,EnqueuePageTokens), storage v3 recordfields (
source_scope_key,cache_validator), and sync-run metadata(
compacted).buf breakingis clean against main.Verification
pkg/syncand all ofpkg/dotc1z, lint clean, generated code insync, no vendor drift.
C1File dirty/closed flags moved to atomics (races found by the
batteries, not by review).
Notes for reviewers
docs/tasks/source-cache-ingestion-invariants.md)is the map for the I1–I9 code; the annotation coverage table in
ingest_invariants.gois enforced by a meta-test, so newside-effect-implying annotations can't ship without an invariant.
runner retiring one on
ErrReplayIntegrity) only costs a cold sync.output on purpose — upsert-merge can manufacture dangling references
no input contained, and the drop pass converges the artifact to what
a fresh sync would assert.