fix(sync): handle dangling entitlements incrementally - #1108
Conversation
…al run loadEntitlementGraph adds an edge's endpoints without checking that their entitlement rows exist, so an edge whose source or destination has no row still lands in the graph and expansion skips it. If a later increment supplies that missing row but no grants, the change adds neither an edge nor a grant — the only two signals incremental expansion seeds from. Nothing seeds the walk, so the fast path silently omits grants full expansion would write. Same artifact, two different access answers, no error. Record which endpoints were skipped and seed exactly those ids on the next run. An id that has since resolved gets expanded; one still missing is re-skipped for the cost of one lookup and re-recorded on the way out. The set is rebuilt every run, so a resolved id drops out instead of re-walking its forward closure forever. Scoping matters here. A graph-wide "something was dangling" flag would re-arm on every full expansion, because most dangling references are permanent — connector magic-id bugs and disabled-by-default resource types, which DroppedEdgeStats measures at millions of warnings a week. That flag would disable the fast path for those tenants for good, which is the opposite of what this series is for. Recording sits at loadExpansionEntitlements, the single point where the topological evaluator resolves every entitlement in the graph; every downstream "not in store" skip is a consequence of that absence, so one site is provably complete. The source-batched evaluator deletes the edge instead of skipping it, so seeding could not reach it — those two sites mark the graph unseedable and the next compaction declines outright. That evaluator never runs on Pebble, and only Pebble persists a graph, so it is a guard rather than a live path. The dangling set is capped; past the cap the graph records overflow and the next compaction declines rather than seed from an incomplete list. graphBlobFormatVersion goes to 3. A v2 blob predates this bookkeeping, so what it skipped is unknowable; the version check already declines a mismatch, so old sidecars fall back to full expansion. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
General PR Review: fix(sync): handle dangling entitlements incrementallyBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe full PR diff was scanned for security and correctness; no blocking issues found. The new commit adds a Risk triage (per Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| func TestIncremental_DanglingPrecheckHonorsCanceledContext(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| cancel() | ||
|
|
||
| store := NewMockExpanderStore() | ||
| g := NewEntitlementGraph(ctx) | ||
| g.NoteDanglingReference("still:missing") | ||
|
|
||
| _, err := NewIncrementalExpander(store, g).ExpandChanges(ctx, nil, nil) | ||
| require.ErrorIs(t, err, context.Canceled) | ||
| require.Contains(t, g.DanglingEntitlementIDs, "still:missing") | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): this pins the new ctx.Err() guard but not the invariant precheckDanglingEntitlements documents — "publish it only after every lookup succeeds, so a transient store error does not partially mutate the graph". Because the context is cancelled before the first iteration, the loop exits before stillMissing is ever built, so this test would still pass if ie.graph.DanglingEntitlementIDs = stillMissing were moved inside the loop body. Consider a case that fails mid-precheck (a store wrapper returning an error on the Nth GetEntitlement, or a context cancelled after the first lookup) with two or more recorded ids, asserting the full original set survives — that is the durable half a later generation reads back from the sidecar.
|
This looks good - the main suggestion is to make one test stronger. Requested before merge:
Verdict: no correctness bugs found. Four independent code-reading passes Verified correct:
Non-issues (traced, all three reviewers agreed):
By design, worth knowing: a permanent dangling id costs one lookup per |
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![C1 Squire [DEV]](https://avatars.githubusercontent.com/in/2846581?s=40&u=9dfdc6298bb22a241aaf41a6558bc1563c5c6466&v=4){kind=avatar}
|
Addressed in 6e05bbd:
Full |
| "a row-only dangling resolution must use the incremental path") | ||
|
|
||
| fullEntries := buildResolvedDanglingFixtures(t, ctx, t.TempDir(), tc.missingSource) | ||
| full, fullCleanup, err := NewCompactor(ctx, t.TempDir(), fullEntries, |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): the full compactor is the differential oracle for require.Equal(t, fullGrants, incrementalGrants) at line 402, but nothing pins that it actually took the full path — the only thing making it an oracle is the absence of WithIncrementalExpansion(). If that option ever becomes the default (or a future NewCompactor change enables it when a sidecar is present), this comparison silently degrades to incremental-vs-incremental and stops being able to fail. Adding require.False(t, full.incrementalExpansionRan, "the differential oracle must be full expansion") after full.Compact makes the oracle self-verifying; the field is already read at line 399 for the other side.
| require.Contains(t, g.DanglingEntitlementIDs, "still:missing") | ||
| } | ||
|
|
||
| func TestIncremental_DanglingOverflowDeclines(t *testing.T) { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): this graph has an empty DanglingEntitlementIDs, so the test pins "overflow ⇒ decline" but not the ordering the new guard exists for — that the decline happens before precheckDanglingEntitlements replaces the recorded set. Move the guard below the precheck and this test still passes, even though an overflowed graph would then come back with its dangling ids silently rewritten. Seeding g.NoteDanglingReference("still:missing") before NoteUnrecoverableDangling() and asserting require.Contains(t, g.DanglingEntitlementIDs, "still:missing") after the decline would pin it. Same shape as the open note on TestIncremental_DanglingPrecheckHonorsCanceledContext at line 604.
2 participants
Summary
Why
A partial sync can add a previously missing entitlement row without adding an edge or changing a grant. Without recording that missing endpoint, incremental expansion has no seed and silently differs from full expansion. This addresses the dangling-reference review item from #1013.
Bounded dangling state
The sidecar stores at most 4,096 distinct missing entitlement IDs to keep connector defects from growing it without bound. If that set overflows, incremental expansion deliberately declines because the retained IDs are no longer a complete description of skipped work; full expansion remains the correctness path. This can leave tenants with more than 4,096 persistent missing IDs ineligible for the fast path, which is an intentional bounded-state tradeoff rather than silently producing incomplete grants.
Testing
go test ./pkg/sync/expand ./pkg/synccompactorgo test -race ./pkg/sync/expand -run TestIncremental_... -count=1go vet ./pkg/sync/expand ./pkg/synccompactor