diff-aware incremental grant expansion

[manojacs]

Expand only the subgraph affected by an incremental change instead of rebuilding and walking the whole entitlement graph. Seeds from both new edges and changed-membership entitlements, so a new member on an existing group propagates; new edges that close a cycle fall back to full expansion. Source reads stream and writes flush in chunks to bound memory. Additions only — callers use full expansion for change sets with revocations.

• expand: IncrementalExpander.ExpandChanges (streaming, chunked flush)
• sync: WithPreserveEntitlementGraph, GraphFromToken, NewExpanderStore
• synccompactor: WithIncrementalExpansion (diff-aware, cycle fallback)
• tests: expander + Pebble-c1z compactor differentials (incremental == full)

[github-actions]

General PR Review: diff-aware incremental grant expansion

Blocking Issues: 0 | Suggestions: 5 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 7443247ebac6.
Review mode: full
View review run

Reviewed at head 3da22557ac19c15919f07cbe8dc3b23e6215f733 against base 7443247ebac6dc8d350f884a107d5090137da32b.

Review Summary

Scanned the full PR diff (40 files, +7587/-142) for security and correctness, with focused reads of the new incremental expander, the compactor's diff-aware path, the Pebble graph sidecar, and the fold/merge changes. Prior finding cmd/baton-compat-harness/main.go:576 is now addressed — commit 3da2255 guards the nil-run case there, and all four LatestFinishedSyncOfAnyType sites in the harness are guarded. The other four prior findings (unwired incremental-performance-check / incremental-soak Makefile targets, the State interface additions, the dangling-destination seed gap, and the loadIncrementalBaseGraph re-open) are unchanged in this increment and are not restated below. No new blocking issues found.

Risk triage (per docs/BUG_CATCHING.md section 2): silence — high (a missed seed yields a silently absent expanded grant, not a failure); durability — high (the graph sidecar and expanded grants are durable c1z content read by future SDK versions); uncontrolled dimensions — high (version-pair dependence via the sidecar, plus crash/checkpoint timing and data volume); consumer distance — high (c1 platform plus future SDKs). Remediation rung 2-3 (re-sync / re-compact). Verdict: HIGH. The PR does carry the instruments this calls for — a differential oracle against full expansion (incremental_differential_test.go, FuzzIncrementalVsFullExpansion), a two-artifact cross-version harness (graph_modes_new.go plus graph-compat-check), crash/retry fault injection (incrementalTestHook, the ProcessKillRetry/CrashRetry tests), and a permutation table (incremental_exhaustive_test.go) — and the default path is gated behind WithIncrementalExpansion() with fail-safe declines (missing/stale/unverified base graph, collapsed cycles, narrowed or dropped edges, dense change, non-Pebble engine). The gap is that the cost gate and the soak are still not reachable from CI, which is the prior finding above; that remains the escalation ask before merge.

Security Issues

None found. The added #nosec annotations sit on pre-existing caller-supplied compaction paths and do not widen input trust; the graph sidecar is written by this process and its reuse is gated on both sync ID and the sealed whole-file grant digest, so a foreign or mutated blob is rejected rather than trusted.

Correctness Issues

None found. The seed derivation (new edges plus fold-collected changed entitlements), the affected-closure topological order, the reverse dropped-edge check against PendingExpansion, and the store restore/finalize split on detached contexts all check out; scanGrantIndexFieldsBytes's third return really is entitlement_id, and NeedsExpansion is a durable property of expandable grants, so the reverse edge check sees the complete current edge set.

Suggestions

• pkg/sync/expand/incremental.go:368-390 - recomputeDestination's memory-bound doc comment omits contrib, which is the dominant and uncapped term (one retained *v2.Resource per distinct fan-in principal).
• pkg/sync/expand/incremental.go:222-226 - the dense-change decline is node-count only and disabled below 1000 nodes, so per-destination grant volume is unbounded and unbenchmarked.
• pkg/synccompactor/compactor.go:687-690 - currentEdges / sourceEntitlements materialize whole-artifact state before any edge-shape decline check, contrary to the deferred-Clone() policy at L826/L843.
• pkg/sync/expand/incremental.go:197 - widened edges take AddEdge's duplicate-edge branch and emit a misleading Warn per edge.
• pkg/sync/state.go:116-121 - GraphFromToken skips the reinitMaps() normalization that the sidecar reader applies, leaving a nil-map panic hazard on a new exported API.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/sync/expand/incremental.go`:
- Around line 368-371: The doc comment on recomputeDestination claims peak memory is
  "one page plus one flush buffer plus the destination's existing-key set". That omits
  the contrib map built at line 390, which holds one *principalContribution (each
  retaining a *v2.Resource) per distinct principal across every source feeding this
  destination, and is uncapped for the whole call. The step-1 comment at lines 385-389
  acknowledges this, so the two comments contradict each other. Reconcile them: either
  list the fan-in map as part of the bound, or state explicitly that the fan-in map is
  the memory bound and the page/flush caps only cover the read and write paths.
- Around line 222-226: The dense-change decline compares len(affected) to
  len(ie.graph.Nodes) and is skipped entirely when the graph has fewer than
  incrementalDenseGraphMinNodes (1000) nodes. Incremental cost is driven by the number
  of grants under the affected destinations, not by node count, so a small graph with a
  single whale destination is accepted and pays a full re-read plus re-merge of that
  destination's grant set on top of base-graph load and Clone(). Add a grant-volume
  component to the decline (for example, decline once the affected destinations' grant
  count exceeds a fraction of total grants), and/or add a benchmark that pins
  per-destination cost rather than only node scaling, since
  TestIncrementalPerformanceGates currently scales nodes only.
- Around line 197: ExpandChanges passes both brand-new edges and widened existing edges
  (classified edgeSpecWidened in pkg/synccompactor/compactor.go) to graph.AddEdge. For a
  widened edge AddEdge takes its existing-edge branch and logs a Warn reading
  "duplicate edge from datasource; merged specs (deep-wins, unfiltered-wins)" at
  pkg/sync/expand/graph.go:523. That message is wrong for the incremental path, where a
  widened edge is expected, and it fires once per widened edge. Either suppress or
  downgrade that log for the incremental caller (for example an AddEdge variant that
  takes the intent), or reword it so it does not read as a datasource anomaly.

In `pkg/synccompactor/compactor.go`:
- Around line 687-760: expandGrantsIncremental builds currentEdges (one entry per
  distinct expandable edge in the whole artifact) and sourceEntitlements (a full
  *v2.Entitlement proto per distinct source entitlement) before any edge-shape decline
  check runs, so a chronically-declining input pays that full scan and allocation on
  every compaction. This contradicts the policy stated at lines 826/843, where
  base.Clone() is deliberately deferred until every cheap decline check passes. At
  minimum, cache only the source resource type id and resource id (the two fields used
  at lines 731-741) instead of retaining the whole entitlement proto.

In `pkg/sync/state.go`:
- Around line 116-121: GraphFromToken returns st.entitlementGraph exactly as JSON
  decoded it, without the reinitMaps() normalization that
  expand.UnmarshalGraphBlobWithGrantDigest applies (its comment: "json leaves absent
  maps nil"). If a token's serialized graph has null maps, the returned graph panics
  with "assignment to entry in nil map" on AddEntitlementID/AddEdge and
  ValidateCompleted() reports misleading errors. Since GraphFromToken is newly exported,
  normalize the graph before returning it — for example expose a normalization entry
  point from pkg/sync/expand and call it here so the token reader matches the sidecar
  reader.

[github-actions]

No blocking issues found.

[kans]

@manojacs

Review feedback from both me, and two Fable passes over the PR considering it in an of itself, and in regard to the big sync replay effort. Replays (eg delta queries in entra) may as well be a new flavor of online/live compaction with certified merge semantics around connector defined scopes (an etag for a page...) instead of the SDK's row level scoping, and they already solve the negative case of tomb stones. I plan on finishing off both sides after both change sets land - incremental expansion for replays, and tombstones for compaction.

Review: incremental grant expansion

The design direction is right. But there are two blockers that mean the incremental path never actually executes against a real store today, two correctness divergences from full expansion that are masked by those blockers, and several soundness/lifecycle gaps. Details below, then the requested changes.

Blockers

B1. The incremental expander cannot read grants through the real store adapter — it always errors and silently falls back to full expansion.
The compactor wires the expander through sync.NewExpanderStore, whose read methods enforce requireEntitlementRefs (entitlement must carry structured resource refs, never a bare id — pkg/sync/syncer.go). incremental.go builds bare-id entitlements (v2.Entitlement_builder{Id: entitlementID}.Build()) for every grant read in both forEachGrant and principalKeySet. The first recomputeDestination fails with "has no resource refs", and expandGrants warn-logs and runs full expansion. Both compactor parity tests pass through the fallback — "fell back to full" and "incremental worked" produce identical output, and the tests assert only output parity. Fix is mechanical (the destination entitlement is already fetched — pass it through; fetch source entitlements too), but the tests must also assert the incremental path actually ran (assert on IncrementalResult or a fallback counter), or this regresses invisibly.

B2. SQLite lifecycle refuses the resume the incremental path depends on.
The merge leaves the new sync ended; expandGrantsIncremental reopens it with StartOrResumeSync. C1File.ResumeSync hard-rejects ended syncs with FailedPrecondition ("cannot resume sync that has already ended"), which StartOrResumeSync propagates — so incremental always declines on SQLite outputs. It only works on Pebble because that adapter's ResumeSync happens not to check ended_at — an accident, not a contract (and both compactor tests use Pebble). Either reopen via a supported path (SetCurrentSync) with tests on both engines, or make WithIncrementalExpansion loudly Pebble-only.

Correctness bugs (currently masked by B1)

C1. Wrong directness predicate for shallow edges.
incremental.go gates shallow edges with isDirectGrant (len(sources) == 0). The full expander uses isGrantDirectOnEntitlement (len(sources) == 0 || sources[entitlementID] != nil). These disagree for a principal who is both a direct member of B and expanded into B — full expansion merges that into one grant whose sources map marks B direct. Add a new shallow edge B→Y in an increment: full propagates the principal to Y; incremental sees a non-empty sources map, calls it indirect, and silently drops the grant. The same predicate also sets IsDirect on newly written grants, so provenance is recorded wrong even on deep edges.

C2. Sources/provenance are never merged, and the drift compounds.
recomputeDestination skips any principal already present on the destination (no sources-map update, unlike mergeContributionIntoExistingGrant on the full path), and records only the first contributing edge for new principals. Even when the (entitlement, principal) row set matches full expansion, the sources maps don't — and directness for future shallow-edge evaluation is read from those sources maps, so each incremental round consumes the previous round's drifted provenance. Errors compound across compaction cycles. Parity tests must compare full grant rows including sources, not just entitlement|principal keys.

C3. Edge-spec changes on an existing edge are invisible.
baseGraphHasEdge checks only that (src, dst) exists. An increment can overwrite a rule grant with a widened annotation — shallow→deep, broader/removed resource-type filter — same endpoints, different semantics. Incremental skips it, seeds nothing, and produces a silently under-expanded artifact. (AddEdge already has deep-wins/unfiltered-wins merge semantics for duplicate edges; the compactor path never lets it see the new spec.) A narrowed spec (deep→shallow, filter shrunk, entitlement id removed from the annotation) is a revocation-shaped change and must auto-decline to full expansion. Note true row deletions can't happen in today's compactor (diff/deletion sync types are rejected as inputs), but annotation rewrites absolutely can — this is the concrete, checkable form of "detect revocations and auto-decline".

Unsoundness

U1. The caller's base graph is mutated in place, poisoning retries.
ExpandChanges adds new edges to the caller-owned incrementalBaseGraph before the cycle check. If a Compact run fails after that mutation and the caller retries with the same options object, baseGraphHasEdge now reports the never-expanded edges as present, newEdges comes out empty, and the retry finishes "successfully" with an unexpanded artifact. Clone the graph, or stage edges until a commit point.

U2. WithPreserveEntitlementGraph breaks expansion replay.
PrepareExpansionReplayToken relies on a finished token carrying no graph. A preserved graph has Loaded=true and every edge IsExpanded=true, so a replayed sync skips graph loading and the expander reports done immediately — replay silently no-ops. On main this can't happen because final tokens never carry a graph; this branch changes that invariant. PrepareExpansionReplayToken must clear/reset a preserved graph, plus a replay test pinning the behavior. (This is the direct collision point with the sync-replay work.)

Requested changes

1. Fix B1 — pass full entitlement records to all grant reads; make the compactor tests fail if the incremental path falls back (assert on IncrementalResult / a fallback counter).
2. Fix or scope B2 — supported reopen on SQLite (SetCurrentSync) with both-engine tests, or explicit Pebble-only.
3. Fix C1 — use isGrantDirectOnEntitlement(grant, sourceEntitlementID) for shallow filtering and IsDirect recording. Add a differential: principal who is both direct and expanded member of the source, new shallow edge in the increment.
4. Fix C2 — merge sources into existing grants and record all contributing edges on new grants; parity tests compare full rows including sources maps.
5. Derive changes inside the compactor — the merge already touches every applied record: collect changed entitlement ids and new/changed expandable annotations there, and drop the caller-supplied changedEntitlementIDs parameter (WithIncrementalExpansion(baseGraph) only). SQLite's attached merge can SELECT DISTINCT entitlement_id of applied-sync grants for pennies; the Pebble overlay/fold observes every applied record during the merge it already does. Compare edge specs, not just endpoints (fixes C3): widened specs seed re-expansion; narrowed specs / access-removing overwrites auto-decline to full. Trust the data, not the call site.
6. Make the revocation auto-decline a distinct, named branch — it is the future tombstone hook. Implement the decline from change 5 as its own explicit code path (e.g. a named sentinel like ErrIncrementalRevocationDecline, sibling to ErrIncrementalFallback), not folded into a generic error path. Today that branch means "decline to full." A later stage flips that same detection point from "decline" to "apply deletions in the merge, seed the affected entitlements, reconcile" — the flip must be a local change at one named site, not archaeology through error handling.
7. Document the seed contract as direction-neutral. Name and document changedEntitlementIDs (or its compactor-derived successor) as "entitlements whose membership changed in either direction." The additions-only restriction belongs to the write behavior (the current decline in change 6), not to the seed semantics. Costs a comment now; prevents a v2 interface later when removals are seeded through the same parameter.
8. Fix U1 — never mutate the caller's graph on a path that can fail or decline.
9. Converge the finish path with the other compaction paths — call Cleanup before EndSync/Close; treat post-EndSync Close failure as fatal (currently it triggers full-expansion fallback against a closed store); EndSync on the generic-error path like the cycle-fallback path does; bound the walk with remaining runDuration and poll ctx.Err(); skip-with-warn on dangling entitlement refs like the full evaluator (fetchEntitlement treats NotFound as skip) instead of erroring into fallback; log IncrementalResult and whether incremental ran or fell back.
10. Fix U2 — reset the preserved graph in PrepareExpansionReplayToken; add the replay test.
11. Cheap insurance tests —
    (a) Dangling-ref differential: increment adds a grant referencing an entitlement absent from the merged set; assert identical final rows between incremental and full.
    (b) Sealed-artifact lifecycle: after incremental's end→resume→write→end sequence, reopen the artifact and verify the sync record is sealed and the Pebble by_principal index covers the incrementally written grants (today this works via deferredIdxPending re-arming, but nothing pins it).
    (c) Revocation parity differential, asserting today's behavior: an increment carrying a revocation-shaped change (narrowed annotation / access-removing overwrite) hits the named decline branch from change 6, and incremental-with-fallback == full. This pins the decline contract, and it is the red test the future apply-deletions stage turns green.
12. Measure the preserved-graph token on a large nested-groups graph, and strip Actions/ExpansionPlan/ExpansionMetrics from the graph before the final checkpoint regardless (nothing strips them today). If still large, persist the graph in the c1z instead of the token.

Items 1–4 are correctness blockers; 5–8 and 10 are unsoundness/API-shape (6 and 7 are cheap now and structural later); 9 is convergence hygiene; 11–12 are cheap tests and a measurement.

Notes (no action required)

• Mid-sync checkpoint/resume machinery is untouched and remains correct; the only behavioral change is skipping ClearEntitlementGraph on the final checkpoint when preserve is set.
• A crash mid-incremental-expansion is safe: the compaction tmp dir is discarded and re-run, and expanded-grant writes are idempotent by deterministic grant id. Worth stating in the PR as a deliberate choice.
• The cycle → full-expansion fallback is the right call: a cycle has no topological order, its correct semantics are a fixpoint (every member of the cycle gets the union of all members), and the full path's SCC-collapse machinery (FixCycles) already handles that. Re-implementing it incrementally would be re-implementing full expansion in the riskiest corner of the problem space.
• The Pebble reopen-after-seal lifecycle (resume unseals, grant writes re-arm the deferred index, second EndSync rebuilds by_principal) looks deliberate and correct — test 11(b) exists to pin it, since it works by a chain of properties nobody asserts together.

Other stuff

• After the code is stable, do a profiling pass over the incremental expansion path for both cpu and memory, and then give it to Fable (historically, everything else will fail). Lets make sure allocations don't explode, etc.

[github-actions]

🟡 Suggestion: This adds a method to the exported State interface, which is a breaking change for any downstream that implements it. In practice State is only implemented by the unexported *state and can't be injected into the syncer (no public constructor/setter accepts a custom State), so the real-world impact is low. Flagging only for SDK-compat awareness — no change required if State isn't intended as a downstream extension point. (low confidence)

[github-actions]

No blocking issues found.

[github-actions]

🟡 Suggestion: If StartOrResumeSync fails here, the store may be left partially resumed, but this returns a plain error that the caller treats as a safe fallback to full expansion (case err != nil → warn + full). Elsewhere the code deliberately marks state-ambiguous failures (restoreEndedSync, finishIncrementalExpansion) as errIncrementalFatal precisely because running full expansion against an unknown store state is unsafe. Consider whether a resume failure should also be fatal (or otherwise guaranteed to leave the store in the ended state the full path expects). Low confidence — depends on StartOrResumeSync leaving the store untouched on error.

[kans]

^ This code is scary. Actually starting a new sync would mean throwing out all existing data. Why not call ResumeSync?

[github-actions]

No blocking issues found.

[github-actions]

No blocking issues found.

[github-actions]

No blocking issues found.

[kans]

@manojacs - review by me, Fable, and a bunch of sub-agents. Since you started work on this, we landed a major refactor to the SDK that centralizes the declaration and handling of SDK invariants (referential & business consistency) for the new write paths that I'm working towards - ie, partial sync replays.

Task: Rebase manoj/incremental-grant-expansion onto main, integrate with ingestion

invariants, fix verified bugs (test-first), add a differential fuzz harness, harden,

then benchmark

Repo: baton-sdk (Go). Branch: manoj/incremental-grant-expansion (exists on origin).

Background

The branch implements diff-aware incremental grant expansion:

• The base sync's entitlement graph is persisted in the c1z as a sidecar blob
  (pkg/sync/expand/graph_blob.go, pkg/dotc1z/engine/pebble/entitlement_graph_sidecar.go),
  stamped with a sync ID that UnmarshalGraphBlob/GraphFromStore use to reject stale blobs.
• The Pebble compaction fold collects changed-entitlement IDs during its merge
  (pkg/synccompactor/pebble/merge.go).
• Compactor.expandGrantsIncremental (pkg/synccompactor/compactor.go) diffs current
  PendingExpansion edges against the base graph and runs
  IncrementalExpander.ExpandChanges (pkg/sync/expand/incremental.go) over only the
  affected subgraph. Cycles and revocation-shaped changes decline to full expansion.
• WithPreserveEntitlementGraph (pkg/sync/syncer.go) writes the graph to the sidecar
  at sync end (Pebble) or keeps it in the token (SQLite).

A four-way review verified the bugs below against the code. Work in stages, in order.
Stages 0 and 1 are required; Stages 2-4 in order as capacity allows — stop cleanly at a
stage boundary rather than half-finishing one. For EACH bug in Stage 1: first write a
test that FAILS and demonstrates the behavior, run it to confirm it fails for the right
reason, then fix, then confirm it passes. One commit per bug (test + fix together).

NOTE: file/line references below were taken against the pre-rebase branch; expect them
to shift after Stage 0. Verify each cited behavior in the rebased code before acting.

Stage 0: rebase onto main + ingestion-invariant integration (do this FIRST)

The branch is based ~11 commits behind main, before "Phase 3 - ingestion invariants"
(#1017) landed. The invariant machinery (pkg/sync/ingest_invariants.go,
runIngestionInvariants in syncer.Sync, the verification marker persisted after
EndSync) does not exist on the branch. Fixing bugs against the stale base would just
create a second merge problem, and the branch has an unexamined semantic conflict with
the invariant design.

0a. Rebase

• Rebase the branch onto current main (or merge main in, if the team prefers merge
  commits — check recent history for convention). Resolve conflicts.
• Danger zone: conflicts will likely cluster in pkg/sync/syncer.go (end-of-sync
  sequence now includes runIngestionInvariants at ~line 936 and
  persistIngestInvariantVerification after EndSync at ~line 982) and
  pkg/synccompactor/compactor_pebble.go (the fold now clears the inherited invariant
  marker at ~lines 684-688: SetIngestInvariantGeneration("") etc.).
• After rebase, before any other work: run
  go test ./pkg/sync/... ./pkg/synccompactor/... ./pkg/dotc1z/... and confirm green.
  (Slow: budget 20+ minutes.)

0b. Invariant integration for the incremental path (test-first)

Semantic conflict to resolve: main's design assumes grant expansion goes through the
syncer, which evaluates ingestion invariants (I3-I9) over the finished store and stamps
a verification marker after EndSync. compactPebbleFold clears the inherited marker
with the comment "expansion/invariant pass will write a fresh marker when one runs."
But expandGrantsIncremental bypasses the syncer entirely (resumes the ended sync,
writes grants directly, EndSync/Close), so on the rebased branch:

• Incremental compactions produce artifacts with NO invariant evaluation and NO
  verification marker (fail-closed: reads as unverified — a silent verification
  downgrade on every incremental compaction).
• The full-expansion path gets the compaction-merge invariant checks; the incremental
  path — the riskier, newer write path — gets none. In particular I8 (grant→entitlement
  referential integrity) is the exact class of damage an incremental-expansion bug
  produces.

Fix:

• Failing test first: run an incremental compaction (reuse the harness in
  pkg/synccompactor/incremental_expansion_test.go), open the output artifact, and
  assert the sync-run record carries a current-generation ingest-invariant verification
  marker (see c1zstore.IngestInvariantVerification, mode
  IngestInvariantVerificationModeCompactionMerge). Currently fails (no marker).
• Fix: after a successful incremental expansion, before/around finalization in
  expandGrantsIncremental / finishIncrementalExpansion, run the exported
  sync.RunIngestInvariants (see pkg/sync/ingest_invariants.go, ~line 408) over the
  compacted store with compaction-merge policy (mirror how the syncer builds
  IngestInvariantsPolicy for CompactionMerge mode, including FailFast wiring), then
  persist the verification marker AFTER EndSync — the marker must only be readable on a
  sealed sync (see persistIngestInvariantVerification for the sealed-write pattern).
• If an invariant hard-fails under fail-fast during an incremental run, treat it like
  other pre-finalization errors: restore the ended sync and fall back to full expansion
  (which will re-evaluate invariants through the syncer). Do NOT wrap invariant
  failures in the fatal sentinel unless the store teardown itself failed.
• Commit Stage 0 as its own commit(s): rebase separate from invariant integration.

Stage 1: verified bugs

Bug 1 (correctness, highest priority): dropped expansion sources are never detected

expandGrantsIncremental iterates current PendingExpansion and classifies each edge
against the base graph (new / widened / narrowed / unchanged), but never performs the
reverse check: a base-graph edge that is ABSENT from the current pending set. Nothing in
compactor.go iterates base.Edges.

Because the fold merge is newer-wins, an increment can replace a grant carrying
GrantExpandable{entitlement_ids:[A,B]} with one carrying [A]. The dead B→C edge
survives in the cloned base graph, is persisted to the sidecar via persistGraphSidecar,
and recomputeDestination walks ALL incoming graph edges — so a later increment that adds
a member to B expands grants onto C through an edge that no longer exists. Full expansion
(which rebuilds the graph from pending) would not produce those grants: incremental ≠ full.

Note: the doc comment on ErrIncrementalRevocationDecline
(pkg/sync/expand/incremental.go) already claims "a source dropped" is covered. It is not.

• Failing test: in pkg/synccompactor/incremental_expansion_test.go, base sync has an
  expandable grant with entitlement_ids:[A,B] targeting C; increment replaces it with
  [A] and adds a member to B; assert the compaction output's grants on C match a full
  expansion of the same inputs (they currently won't — the incremental path writes extra
  grants via the ghost edge). Also assert the incremental path declined
  (incrementalExpansionRan == false) once fixed. If parity assertion is awkward in the
  harness, acceptable fallback: assert the decline sentinel fires and no grants on C are
  sourced via B.
• Fix: after collecting the pending (src,dst) edge set, iterate base.Edges (skip
  same-node/collapsed pairs); if any base edge is missing from pending, return
  expand.ErrIncrementalRevocationDecline (restore the ended sync first, matching the
  other decline paths).

Bug 2 (soundness): persisted graph violates its own invariants after incremental success

AddEdge (pkg/sync/expand/graph.go) creates edges with IsExpanded: false and sets
g.HasNoCycles = false. ExpandChanges never marks edges expanded and never restores
HasNoCycles after its cycle check passes. So a successful incremental run persists a
sidecar graph that is semantically expanded but reports IsExpanded() == false — and
Expander.IsDone() (pkg/sync/expand/expander.go) delegates directly to
graph.IsExpanded(). The documented precondition on IncrementalExpander says the graph
is "a prior completed expansion's graph (edges already expanded)"; the artifact this code
writes breaks that precondition for the NEXT run.

• Failing test: run an incremental compaction that adds a new edge, reload the sidecar
  graph from the output artifact (sync.GraphFromStore), assert graph.IsExpanded() is
  true and HasNoCycles is true. Currently fails.
• Fix: in ExpandChanges after the walk succeeds (or in the compactor just before
  persistGraphSidecar), mark all edges expanded and set HasNoCycles = true (the cycle
  check already passed). Mirror what markExpansionComplete does for the full expander.

Bug 3 (soundness): fold orphans the graph sidecar under the new sync ID

compactPebbleFold (pkg/synccompactor/compactor_pebble.go) mints newSyncID, rewrites
the sync-run record AND recomputes the stats sidecar under the new ID (and, post-rebase,
clears the inherited invariant marker) — but never touches the entitlement-graph sidecar,
which remains stamped with the base sync's ID. The delete-inherited-sidecar logic exists
only inside expandGrants; the skip-expansion path in doOneCompaction
(c.skipGrantExpansion or partial sync type) closes and publishes without ever reaching
it. Result: the published artifact carries a stale blob; GraphFromStore(ctx, store, newSyncID) returns nil and the incremental chain silently breaks (falls back to full
forever).

• Failing test: fold-compact a base that has a graph sidecar, with skip-expansion enabled;
  open the output and assert either (a) GraphFromStore with the new sync ID returns a
  usable graph, or (b) the raw sidecar blob is absent — i.e. never a blob stamped with the
  old ID. Currently the stale base-ID blob is present.
• Fix: in compactPebbleFold, after minting newSyncID, handle the graph sidecar the
  same way the stats sidecar and (post-rebase) the invariant marker are handled: either
  rewrite the envelope's sync ID (keeping the base graph available as the incremental
  base) or delete the blob. Deleting is the simpler, safe choice; rewriting is better for
  chain continuity — prefer rewriting only if you can do it without unmarshaling in the
  engine layer (the blob format is owned by pkg/sync/expand, the engine treats it as
  opaque — respect that layering).

Bug 4 (minor, no test needed): fix the WithPreserveEntitlementGraph godoc

pkg/sync/syncer.go: the godoc says the option "keeps the entitlement graph in the final
sync token instead of clearing it". On Pebble the actual behavior writes the graph to the
c1z sidecar and CLEARS it from the token; only SQLite keeps it in the token. Also update
WithIncrementalExpansion's doc in pkg/synccompactor/compactor.go, which directs
callers to sync.GraphFromToken — the primary loader is sync.GraphFromStore;
GraphFromToken is the SQLite/legacy fallback.

Stage 2: differential fuzz harness (incremental vs. full expansion parity)

The ghost-edge bug shows the scenario space (edge specs × membership deltas × annotation
replacements × cycles) is too combinatorial for hand-written cases. Build a differential
property test. The oracle is built in: for any base state + delta sequence, incremental
expansion must produce exactly the same grants as full expansion, OR decline (declining
IS full expansion, so parity holds trivially).

Two levels, weighted toward the cheap one:

2a. Fast loop in pkg/sync/expand (the main harness)

• Use pgregory.net/rapid (preferred, has shrinking; add to go.mod) or a seeded
  math/rand scenario generator with the seed logged on failure.
• Generate: a universe of ~5-30 entitlements and ~10-100 principals; a base state of
  direct grants and expandable edges (random shallow/deep, random resource-type filters,
  some multi-source annotations, occasional cycles); run FULL expansion over an in-memory
  ExpanderStore to establish the base (grants + graph). Follow the existing in-memory
  store patterns in pkg/sync/expand/incremental_test.go.
• Generate a delta script of 1-10 random operations: add member, remove member, replace
  an expandable annotation with a subset/superset of entitlement_ids, flip shallow/deep,
  tighten/widen a resource-type filter, add a brand-new edge, add a cycle-closing edge.
• Apply deltas to a copy of the store; compute newEdges/changedEntitlementIDs the same way
  the compactor does (including the Bug-1 reverse check once fixed); run
  IncrementalExpander.ExpandChanges against the base graph clone.
• Reference: run full expansion from scratch over the post-delta store.
• Assert, when the incremental path did NOT decline:
  1. Grant-set parity: same (entitlement, principal) pairs, same sources maps, same
     directness flags. Sort and diff; print the seed and the minimal delta script on
     failure.
  2. The post-run graph satisfies IsExpanded() and HasNoCycles, and survives a
     MarshalGraphBlob/UnmarshalGraphBlob round trip.
• Track the decline rate across the run and assert a healthy fraction (e.g. >30%) of
  cases actually exercised the incremental path (incrementalExpansionRan-equivalent).
  Without this, a regression that makes everything decline passes the fuzzer while
  silently killing the feature.
• Wire it as a standard go test with a bounded case count (e.g. 200 cases) so it runs
  in CI in seconds, plus an opt-in env var / flag for a long run (e.g. 50k cases).

2b. Thin end-to-end differential test at the compactor level

• One test in pkg/synccompactor that generates a handful (~10, seeded) of random
  base+increment c1z pairs, compacts each twice — once with WithIncrementalExpansion,
  once without — and asserts grant-set parity between the two outputs plus a valid
  sidecar graph under the output sync ID, plus (post-Stage-0) a current-generation
  invariant verification marker. This covers what 2a can't see: the fold's changed-ID
  collection, sidecar lifecycle, invariant marker, and decline plumbing. Keep the case
  count low; Pebble compactions are seconds each.

Stage 3: hardening and design follow-ups

Items 3.1-3.4 are implementation work. Items 3.5-3.6 are design decisions: produce a
short written proposal (in the PR description or a doc comment), do NOT implement
unilaterally.

3.1. Version the sidecar blob format (small, do it now)

graphBlobEnvelope (pkg/sync/expand/graph_blob.go) is JSON with a sync ID but no
format version. Go's JSON silently zero-fills missing or renamed fields, so the first
refactor of EntitlementGraph's serialized shape will successfully parse old blobs into
wrong graphs — no error, just corrupt incremental bases. Add a format_version (or a
generation string mirroring IngestInvariantGeneration) to the envelope; on mismatch,
UnmarshalGraphBlob returns (nil, nil) — same treatment as a sync-ID mismatch, so the
chain falls back to full expansion and heals. Test: a blob with a stale version is
rejected; a current one round-trips.

3.2. Artifact fsck: one validator for sealed-artifact invariants

Several Stage 1 bugs are one bug: a sealed c1z can violate its own self-description.
Write a single validation helper (suggested: a new file in pkg/synccompactor or a
shared testutil package) that, given a sealed store, asserts:

• If a graph sidecar exists, its envelope sync ID matches the store's sync-run record,
  its format version is current, and the graph passes IsExpanded(), HasNoCycles,
  and a marshal/unmarshal round trip.
• The ingest-invariant verification marker is present with the current generation
  (for artifacts produced by paths that promise verification).
• Sanity: sync-run record is sealed/ended.
  Call this validator at the end of every compactor test that produces an artifact (both
  incremental and full paths) and from the Stage 2b harness. This retires the category
  instead of pinning one regression test per lifecycle bug.

3.3. Instrument the fallback funnel

The design degrades to full expansion silently: an orphaned sidecar, chronic decline, or
marshal error produces zero errors and zero incremental runs, forever — the feature can
be dead in production with no signal. The only current observability is the test-only
incrementalExpansionRan bool. Add structured logging (and metrics if the compactor has
a metrics handle — check pkg/metrics conventions) recording per compaction: incremental
attempted / succeeded / declined (with reason: cycle, revocation-shaped, dropped-edge) /
fell back (with error class) / not attempted (no base graph, engine, skip), plus
grants written and entitlements walked on success. Keep field names stable — these will
be dashboarded. Replace test usage of the bool with these outcomes if straightforward;
otherwise leave the bool.

3.4. Crash/resume schedules in the fuzz harness

The checkpoint-focused review found ordering windows by hand (sidecar durable before
seal, Sync-vs-NoSync writes, fold rename). Extend the Stage 2 harness with interrupt
schedules: cancel the context / simulate failure at randomized seams (after sidecar
write, mid-ExpandChanges, before EndSync), then retry/resume the operation from its
inputs, and run the 3.2 fsck plus the parity oracle on the final artifact. Follow the
existing interrupt-test patterns (pkg/sync/expand/topological_merge_layer_interrupt_test.go,
pkg/dotc1z/engine/pebble/endsync_concurrent_writers_test.go). Assert the retried run
converges: no duplicate grants, no stale sidecar, no artifact published from a failed
attempt.

3.5. PROPOSAL ONLY: make the invariant seam structural

Stage 0b wires invariants into the incremental path by convention — the next new write
path can forget again, which is the exact failure mode the Phase 3 design warns about
("any second ingestion path turns every stream-coupled side effect into a latent bug").
Propose a structural enforcement: e.g. a store that is resumed-and-written after seal
cannot re-seal without either a fresh verification pass or an explicit unverified stamp.
Sketch the API change (likely in c1zstore / the engine's EndSync path), its blast
radius, and migration. Do not implement.

3.6. PROPOSAL ONLY: decide the deletions contract

Additions-only is load-bearing everywhere: the decline sentinels, the fuzz oracle, the
seed semantics of changedEntitlementIDs, and the Bug 1 fix shape. The code carries a
named hook (ErrIncrementalRevocationDecline) for a future tombstone/deletion stage.
Write a short decision memo: (a) if deletions are coming, what the oracle, fsck
invariants, and Bug-1-style reverse diff need to look like to accommodate them; (b) if
additions-only is permanent, which aspirational comments to delete and where to document
the contract (package doc of pkg/sync/expand). Recommend one. Do not implement either.

Stage 4: CPU/memory benchmarks + easy performance wins

The feature exists for performance, but the branch has no benchmarks: nothing proves the
incremental win, locates the crossover point vs full expansion, or measures what a FAILED
attempt costs (a tenant with chronic declines pays the incremental overhead on every
compaction and then runs full expansion anyway).

4a. Benchmarks (write these first, before touching any code)

Go benchmarks with b.ReportAllocs(); capture peak heap via runtime.ReadMemStats
deltas (or -benchmem plus a peak-tracking allocator shim if the repo has one). Compare
runs with benchstat. Parameterize by scale; suggested grid: entitlements E in
{1k, 10k, 100k}, principals P in {10k, 100k}, edges ~2x E, delta size K in {1, 100, 10k}.

Happy path (fast, in pkg/sync/expand against the in-memory store from Stage 2a):

• BenchmarkIncrementalExpand/E=..,K=.. — ExpandChanges on a K-sized delta.
• BenchmarkFullExpand/E=.. — full expansion of the same post-delta state.
• Report the crossover: at what K/E ratio does incremental stop winning? Put the numbers
  in the PR description.
• BenchmarkGraphClone/E=.. and BenchmarkMarshalGraphBlob/E=.. — the per-attempt
  fixed costs.

Sad paths:

• BenchmarkIncrementalDecline/E=.. — an attempt that declines (e.g. a narrowed edge)
  measured END-TO-END including clone, pending scan, cycle check, and restore: this is
  pure overhead added to every full expansion for a declining tenant. This number is
  the feature's worst-case regression; it must be small relative to full expansion.
• BenchmarkIncrementalFallbackCompaction — one compactor-level (Pebble) benchmark:
  compaction with incremental-requested-but-declined vs compaction without incremental.
  Keep it coarse (single scale) — it's seconds per iteration.

One e2e happy-path compactor benchmark at a single realistic scale (base + small
increment) for wall-clock and RSS sanity; the fine-grained work stays at the expand level.

4b. Easily pickable fruit (only with benchmark evidence)

Rules: only local, semantics-preserving changes; each must show a measurable win in 4a
benchmarks; re-run the Stage 2 fuzz harness after each; one commit per change with the
benchstat delta in the commit message. Known candidates from review (verify against the
rebased code):

1. Defer/avoid the whale clone on decline paths. expandGrantsIncremental clones the
   entire base graph via JSON round-trip BEFORE the pending scan and spec classification,
   but those checks only read the graph. Restructure so the clone (or the mutation phase)
   happens only after the decline checks pass. For a chronically-declining tenant this
   removes the entire clone cost from every compaction.
2. Replace Clone-by-JSON with a structural deep copy (maps/slices copied directly).
   The quality review flagged it; justify with the BenchmarkGraphClone numbers. Keep
   reinitMaps for the unmarshal path.
3. Memoize getEntitlement within one ExpandChanges run. recomputeDestination
   re-fetches the same source entitlement for every destination that shares it; a
   per-run map[string]*v2.Entitlement caps lookups at one per distinct entitlement.
   (Bound it or accept the graph-sized worst case — it's one proto per entitlement.)
4. Skip topologicalNodeOrder over the WHOLE graph when the affected set is tiny:
   order only the affected subgraph, or early-out when len(affected) is small relative
   to the graph. Only do this if the benchmark shows the full-graph sort matters at
   E=100k with K=1.
5. IncrementalResult.EntitlementsWalked accumulates every walked entitlement ID but
   callers only log its length — replace with a count (check all callers first).

Do NOT chase anything beyond these without flagging it — deeper changes (e.g. altering
flush chunking, store read paths) belong in the planned quality pass.

Constraints

• Do not restructure the error protocol or refactor expandGrantsIncremental beyond what
  the fixes require — a separate quality pass is planned. (Stage 4b item 1 is the one
  sanctioned exception: moving the clone is allowed.)
• Follow existing test patterns in pkg/synccompactor/incremental_expansion_test.go and
  pkg/sync/expand/incremental_test.go.
• After all work: run go test ./pkg/sync/... ./pkg/synccompactor/... ./pkg/dotc1z/...
  (slow, 20+ min) and make lint. All must pass. If the fuzz harness finds NEW
  divergences beyond the bugs above, do not paper over them: minimize the repro, add it
  as a named regression test, and report it — fix it only if the fix is small and
  obvious.
• This is a public repo: no customer names or production identifiers in code, comments,
  or commit messages.

[kans]

Another random thought. We are accumulating fuzzers - it would be nice to have a make fuzzball target that runs all of them for a bit ...

edit: not to swamp you, but I have been working on a rubric to drive this sort of in depth AI review (it should be in main now, so you can run it yourself). I had it reevaluate the originate feedback, and it generated this revision:

Addendum to the brief above (re-review against docs/BUG_CATCHING.md)

We re-evaluated this branch against the current review rubric. The original
brief stands; this adds one new finding, three strengthened requirements, and
a Stage 0 refresh.

New finding — unversioned sidecar envelope (same failing-test-first
discipline as the others):
graphBlobEnvelope is {sync_id, graph} JSON with no version field. Any
future change to the graph's serialized shape zero-fills silently in one
direction or the other, and a silently wrong base graph produces silently
wrong expansion — no error anywhere. Add a version field: writers stamp it,
readers fail closed on missing/unknown versions by degrading to full
expansion (never a partial parse). Round-trip test plus a wrong-version
fixture. The SyncID guard stays, but it only defends against the wrong sync,
not the wrong schema.

Strengthened requirements:

1. Build the differential fuzzer first, not last. Incremental-vs-full is a
   free oracle; it mechanically covers the class the review rounds only
   sampled. Order of work: Stage-0 rebase, then the fuzzer, then bug fixes
   against it.
2. Prove the fuzzer can see its bugs (mutation adequacy). Plant a ghost
   edge, a skipped re-expansion, and a stale-sidecar acceptance; the fuzzer
   must fail on each before its green runs count as evidence.
3. Fix the fold/copy sidecar bug structurally, not point-wise. "Also handle
   the graph on fold" leaves the next sidecar to be forgotten the same way.
   Add a stored-state descriptor — key range + lifecycle policy declared
   once — that fold, cleanup, reset, and conversion all iterate.

Cross-version cell table (lives in code, not here): the graph moved from
the sync token to a c1z sidecar, so every cell of (old SDK / new SDK) ×
(mid-sync token, final token, sidecar present/absent, legacy graph-in-token)
needs a disposition. Cells this repo can execute go in one table-driven test
— the expected value is the disposition ("degrades to full expansion" is
fine; an unwritten cell is not), excluded cells are skipped rows with a
reason. Cells requiring an old SDK binary go in a comment on the envelope
type, where the next person changing the format will see them. This PR's
description should just link both.

Stage 0 refresh: main has moved since the brief was written — the
ingestion-invariant seam (#1017) has landed and the invariant verification
marker work is in flight. The compactor's expansion pass rewrites grants on a
sealed store: after rebase it must run the invariant pass in compaction mode,
and clear/re-stage the verification marker around the rewrite if that work
has merged. The rebase debt has grown; do Stage 0 first.

Fallback visibility (small, new): incremental expansion declines to full
on cycles, narrowed specs, missing sidecar, non-Pebble engines. A fallback
that always fires is a dead feature with green tests — add one test asserting
the incremental path engages on the happy path, and make decline reasons
countable so "never runs in prod" is observable.

[github-actions]

🟡 Suggestion: incremental-performance-check and incremental-soak are not referenced by .github/workflows/nightly.yaml (which enumerates targets explicitly: compat-check, fuzz-smoke, differential-check, interrupt-check, …) or by any other target, so TestIncrementalPerformanceGates — which is additionally skipped unless BATON_INCREMENTAL_PERF=1 — never runs automatically. docs/BUG_CATCHING.md cost-contract guidance asks for a benchmark that enforces the cost curve on grant-expansion paths; an unwired gate does not. Adding both to the nightly suite matrix would close it. (confidence: high)

[github-actions]

Blocking issues found — see review comments.

[github-actions]

🟡 Suggestion: a destination entitlement that was dangling during the base expansion and materializes in an increment is never seeded, so incremental expansion silently omits grants full expansion would write.

loadEntitlementGraph (pkg/sync/syncer.go:2504) calls AddEntitlementID(dstEntitlementID) unconditionally — only the source is existence-checked — so an edge src -> dst lands in the base graph even when dst has no entitlement row. The base expansion then skips dst (dangling-ref skip-with-warn). If a later increment adds the dst entitlement record but no grants on dst:

• PendingExpansion still reports the same src -> dst edge, baseGraphEdge finds it inBase with an unchanged spec → no entry in newEdges;
• the fold's changed set only collects entitlement ids from the grants bucket (pkg/synccompactor/pebble/merge.go:1116), and deriveChangedEntitlementIDs likewise only reads grants, so dst is not in changedEntitlementIDs.

The walk therefore has no seed for dst and writes nothing, while a full expansion would now populate it. Consider also seeding graph nodes whose entitlement record newly resolves in the store (or declining when a base edge's destination was dangling at base-expansion time). (confidence: medium — the logic gap is clear; reachability depends on connectors emitting GrantExpandable targets ahead of the entitlement itself.)

[manojacs]

Acknowledged — this one is real. The dangling-destination precondition is common (I8's own docs note expansion drops millions of edges per week to this class, and I8 is warn-only in default mode, so a base with dangling destinations still seals verified and passes the incremental reuse gate). The uncommon step is the destination actually materializing later, but the config-gap class (resource type enabled after initial setup) makes that plausible.

The fix needs some more thought — the candidate shapes pull in different directions:

• decline-on-dangling-destination: record in the sidecar which edge destinations had no entitlement record at base-expansion time, and decline to full when any exist. Always correct, but given the volume of permanent magic-id danglings, this could decline a large fraction of real compactions and gut the fast path.
• seed-on-materialization: have the fold/derive changed-set also collect entitlement ids that newly resolve (entitlements bucket, not just grants). Precise and keeps the fast path, but touches the merge plumbing and needs its own differential coverage for the dangling→materialized sequence.

Rather than rushing either into this PR, we'll follow up with the appropriate fix in a separate change.

[github-actions]

No blocking issues found.

[github-actions]

🟡 Suggestion: in fold mode this is a second full extraction of the base c1z on every compaction. copyFileForFold already byte-copies entries[0] to destFilePath and doOneCompaction opens it as c.compactedC1z, so the base envelope has already been unpacked once — and compactPebbleFold deletes the sidecar from that copy at line 697 before expandGrants runs, so the graph has to be re-read from the original. Capturing the sidecar blob (and the base's verification run) inside compactPebbleFold before DeleteEntitlementGraphSidecar would avoid re-unpacking a whale base. Note also that this open omits WithDecoderOptions(WithDecoderConcurrency(-1)) and the shared c.decoderPool that deriveChangedEntitlementIDs and doOneCompaction pass, so the extra decode also runs with default decoder settings. (confidence: medium)

[manojacs]

Did some napkin math on this against observed artifact sizes from the last two weeks of production telemetry: typical artifacts are ~20 MB uncompressed (~1 MB compressed), and the largest observed is ~2.5 GB uncompressed (~330 MB compressed).

The redundant second open costs roughly: read the compressed file + single-threaded zstd decompress (~500 MB/s) + temp-dir write (~250–500 MB/s):

• typical (~22 MB uncompressed): ~0.1 s per compaction — noise
• largest observed (~2.5 GB): ~10–15 s per compaction, plus a transient ~2.5 GB temp-disk spike while both extractions exist

So the cost is real but only meaningful at the far right of the size distribution, and it scales with compaction frequency, not correctness. Leaving this as a follow-up rather than expanding this PR:

• cheap half: pass WithDecoderConcurrency(-1) + the shared c.decoderPool to this open (parallel decompress, near-zero risk)
• full fix: capture the sidecar blob + base verification run inside compactPebbleFold before DeleteEntitlementGraphSidecar, which touches the fold plumbing and deserves its own change

[github-actions]

No blocking issues found.

[github-actions]

No blocking issues found.

[kans]

This is safe to merge and is a large effort. Lets pick up the remainder in another PR before we turn it on:

Blocking enablement

1. Dangling-destination seeding gap (r3764614368)
   Destination dangling in base graph, entitlement row added by later increment
   with no grant changes -> never seeded -> grants silently missing vs full
   expansion. Fix: seed nodes whose entitlement record newly resolves, or
   decline when a base edge's destination was dangling at base-expansion time.

2. Decline must not eat the run budget (no PR comment - add issue)
   Base unpack + PendingExpansion classification + restore all run before the
   leftover-duration check, on parent ctx not runCtx. A decline (incl. the
   guaranteed first run after enable) can fail compaction with "unable to
   finish" instead of running full expansion. Fix: cheap eligibility checks
   first; charge incremental work against runCtx; leftover check before fallback.

3. Redundant base re-unpack per fold (r3767877771)
   Every opted-in fold extracts the whole base c1z a SECOND time just to read
   the sidecar the fold copy already had (and deleted at compactor_pebble.go:697)

   • can make fold+incremental slower than fold+full on whale bases. Fix:
     capture sidecar blob + verification run inside compactPebbleFold before
     DeleteEntitlementGraphSidecar; pass shared decoder pool/options.

4. ResumeSync instead of StartOrResumeSync (r3600362362) - one-liner
   compactor.go:678: lookup error falls through to StartNewSync ->
   ResetForNewSync wipes the merged dest. ResumeSync is already on the
   interface and fails closed.

5. Wire incremental-performance-check / incremental-soak into nightly CI
   (r3764191460) - perf gates currently never run automatically.

Non-blocking

6. Cleanup: WithPreserveEntitlementGraph SQLite token fallback is dead code
   (never fires without WithEntitlementGraphInCheckpoints); fix docs or
   delete the branch.