Bound memory in processGrantsWithExternalPrincipals's grant rewrite (CXP-834) by c1-squire-dev[bot] · Pull Request #1047 · ConductorOne/baton-sdk

c1-squire-dev · 2026-07-29T13:29:02Z

TL;DR

The goal, unchanged since the first commit: bound memory in processGrantsWithExternalPrincipals's grant rewrite. Writes now flush in batches of 500 instead of buffering the whole scan in memory, and pending-delete grants are trimmed to just the fields needed to delete them.
An earlier version of this PR also stripped an annotation off resolved grants to cut down on wasted work when a sync resumes. Reverted — it silently broke the mechanism that revokes a grant when its external principal later disappears or changes. Kept the annotation, accepted the (bounded) wasted work instead. This decision is final — it's not open for further debate, only for a clear objection to the tradeoff itself.
Also: fixed a tracing bug that hid write failures from spans, fixed a gap where SQLite skipped stale-grant cleanup entirely, and added a benchmark that hard-fails on cost regressions instead of just reporting a number that could drift quietly.
Status: ready for human review. This PR has been through several rounds of automated review; every finding that represented a real bug or a real coverage gap has been fixed (with a regression test proving it), and every finding that was mistaken has a reply explaining why. Further automated-review rounds finding smaller things is expected and not, by itself, a reason to keep iterating before merge.
6 commits, each independently buildable/testable — see below for the full reasoning on each.

Problem

CXP-834 investigates why baton-sharepoint syncs on large tenants never finish. #1046 fixes the algorithmic hot loop in processGrantsWithExternalPrincipals (O(grants × principals) → O(grants + principals) via a principal index). This PR fixes a second, separate problem #1046 explicitly calls out as "Not in scope":

expandedGrants and grantsToDelete still accumulate fully in memory before a single PutGrants, and the MatchAll branch appends one grant per principal. That's a real OOM risk on large tenants.

#1046 fixes the CPU/wall-clock cost of matching; this fixes the memory cost of writing the results.

This branch is now rebased directly on main (#1046 is merged). It previously stacked on jallers/cxp-498-external-principal-index before that landed; the rebase folded away three commits that #1046's merge made fully redundant — an earlier single-commit draft of the same indexing work, a progress-logging commit, and a case-fold normalization fix (the version that merged into main went further, using true Unicode EqualFold-based folding instead of strings.ToLower, which covers what the normalization commit was fixing). What remains is only this PR's own change, replayed onto main's current shape of the function — including the generic matchTraits[trait]/matchProfileAndExpand path from the already-landed CE-975 trait-generic refactor (#1043), which post-dates this PR's original commits.

Change

expandedGrants is now flushed in bounded batches (externalGrantFlushBatchSize = 500) via PutGrants, instead of being accumulated into one slice and written once at the end. This includes flushing from inside the ExternalResourceMatchAll branch's fan-out loop — the sharpest case, since a single matched grant there can produce one replacement per matching principal (tens of thousands on a large tenant).
newGrantIDs (used to avoid deleting a grant we just rewrote with the same id) is now built incrementally as grants are buffered, instead of via a second pass over the fully-retained slice. This set itself still holds one string per expanded grant for the whole scan — the memory bound here is over the grant-proto buffer specifically, not this membership set, which stays an O(fan-out) allocation. Flagged in review and accepted as out of scope for this pass (see Out of scope).
grantsToDelete entries (both here and in deleteStaleExternalPrincipals's own pending-delete list) are shrunk via minimalGrantForDelete to a minimal reconstruction (Id/Entitlement/Principal only — the fields DeleteGrantByRefs/DeleteGrant actually use), instead of retaining the full original grant proto (which can carry a bulky GrantExpandable entitlement-id list or annotations). This list still can't be flushed early: the delete-dedup check needs the complete newGrantIDs set, which is only final once the whole scan finishes, so early deletes risk deleting a grant a later-discovered replacement should have protected.
The final PutGrants call after the scan loop stays unconditional (even with zero grants), matching the pre-existing behavior — the Pebble engine's PutGrants has a markDirty side effect on every call, empty or not.
processGrantsWithExternalPrincipals is split into a thin tracing wrapper plus processGrantsWithExternalPrincipalsInner. The original single function's deferred uotel.EndSpanWithError closed over an err that a range-over-func loop and several per-branch X, err := ... declarations each shadowed in their own block scope, so a write failure deep in the scan still correctly failed the sync but silently closed its tracing span as successful. The wrapper owns the span, a single err assigned exactly once from the inner call's return value, and the defer — sidestepping the shadowing instead of chasing it through the whole function.
deleteStaleExternalPrincipals's grant-level cleanup (revoking a grant whose external principal has disappeared entirely) now runs on every storage engine, falling back to the id-based DeleteGrant when the refs-based fast path isn't available. It previously required resourceRecordDeleter, entitlementRecordDeleter, and grantByRefsDeleter support before doing anything at all — but only the Pebble engine implements any of the three, so on SQLite (the default engine for existing tenants until their next writable sync converts them) this whole reconciliation silently no-opped. Resource/entitlement row cleanup still requires the refs-based deleters and degrades with a warning when unavailable — a leftover stale resource/entitlement row is inert metadata, not live access, so that's the safer place to degrade.
Added BenchmarkProcessGrantsWithExternalPrincipals, pinning the ExternalResourceMatchAll fan-out's cost curve at 1k/10k principals on both engines. It measures the actual number of grants written (via a counting store wrapper) against the loop's own cost prediction (principalCount+1 per iteration) and fails outright on any drift, rather than just reporting a timing number that could regress silently in CI.

Three hazards from writing mid-scan (not present in the original code, where nothing was written until after the scan finished)

ListWithAnnotations pages read the live engine on each page fetch (not a snapshot). Replacement grants inherit their placeholder's ExternalResourceMatch* annotation via newGrantForExternalPrincipal, so an early-flushed replacement could land in a not-yet-scanned key range and get picked up — and re-expanded as if still unresolved — by a later page of the same scan. Fixed by skipping any grant whose id is already in newGrantIDs (populated the instant a grant is buffered, before it physically flushes).
The ExternalResourceMatchID branch mutates its replacement grant in place (GrantExpandable remapping) after it's first built. A flush landing between the initial buffering and that mutation would have committed the pre-mutation grant, missing its expansion annotation. Fixed by deferring the flush-size check until after all possible mutation for that grant completes — the buffer holds the pointer, so the later mutation is reflected automatically without a second append.
The same annotation-inheritance from (1) also survives across a full restart, where the same-scan newGrantIDs guard offers no protection — this action has no internal checkpointing, so an interruption mid-scan means the next attempt reruns the whole thing from scratch with a fresh, empty newGrantIDs. A previously-flushed, already-resolved replacement grant still carries the annotation that made it look like a placeholder, so the resumed scan re-matches and rewrites it as if unresolved. Grant ids are deterministic per (principal, entitlement), so this never produces a duplicate grant record — but it is real, wasted work on every resume (flagged during [CXP-498] Index external principals for grant matching #1046's review as retry amplification). Tried and reverted: an earlier version of this PR stripped ExternalResourceMatch* from every replacement grant to eliminate the amplification. That closed hazard 3 but opened a different, worse gap: with the annotation gone, a resolved grant became invisible to this function's own re-scan, which is the only mechanism that revokes it if its external principal later disappears or simply stops satisfying the match criteria (deleteStaleExternalPrincipals only catches principals that are entirely absent, never ones that still exist but no longer match). That's a silent, permanent access-control gap, on every storage engine — worse than bounded wasted work. Accepted as-is: the annotation is retained, hazard 3's amplification stands as a known, bounded cost, and every resolved grant stays self-healing.

Out of scope (follow-up)

The principals []*v2.Resource slice passed into this function is still fully materialized upstream before it's called — that's a separate, larger change (reading principals back from the store instead of holding them all in memory) that needs its own design.
newGrantIDs's O(fan-out) string-set footprint (see Change above) — a fixed-footprint replacement (bounded-error filter, or a second re-verification scan instead of an in-memory set) is a larger, riskier change than this PR's scope.
deleteStaleExternalPrincipals's resource/entitlement row cleanup remains Pebble-only. Implementing the refs-based deleters SQLite would need is a substantial addition for a storage engine that's no longer the manually-gated, SKU-restricted minority case it once was — FEATURE_FLAG_ID_PEBBLE_DEFAULT_STORAGE_ENGINE is now SKU_ALL — but existing tenants still run on it until their next writable sync converts them, and the row-cleanup gap is inert metadata, not live access, so it isn't worth the risk here.

Testing

TestExternalResourceMatchAllBatchedFlush{SQLite,Pebble}: drives an ExternalResourceMatchAll grant against enough external principals to span 3 flush batches, via a store wrapper that records every PutGrants call, on both storage engines. Asserts no single call exceeds the batch size, that it took more than one call, and that the final grant set is exactly one grant per principal with no duplicates. The Pebble-specific refs-based delete path and the SQLite-specific id-based fallback are each asserted on their own engine.
TestExternalResourceMatchAllSkipsItsOwnFlushedReplacements: regression test for the newGrantIDs re-encounter guard — mid-scan flushing means a replacement grant can land in a not-yet-scanned key range and get read back by the same scan. Builds a grant table deliberately larger than one dotc1z page (the only way SQLite's ascending-rowid pagination actually re-reads a flush) and asserts each external user's grant is written exactly once. Verified this fails (turning ~600 writes into ~300,000) with the guard removed.
TestExternalPrincipalCleanupFallsBackToIDDeleteWithoutRefsDeleters: direct unit test for deleteStaleExternalPrincipals's id-based DeleteGrant fallback on a store exposing none of the optional refs-based deleters (the SQLite shape). This path has no coverage in the interrupted-then-resumed end-to-end tests below, since a resolved grant's retained match annotation lets processGrantsWithExternalPrincipals's own scan revoke it first and mask whether the fallback ran at all. Verified this fails against the old bail-out-without-all-three-deleters behavior.
TestExternalResourceMatchIDBidFailureDropsAttempt: regression test for a blocking bug caught in review — the MatchID branch's GrantExpandable remap used to buffer its replacement grant before the bid.MakeBid failure path that drops the whole attempt, so a bid failure left a half-resolved grant persisted alongside its never-deleted placeholder.
TestDeleteStaleExternalPrincipalsRevokesGrantAfterCutAndShrink{SQLite,Pebble}: end-to-end regression test asserting the outcome (no live grant survives a departed external principal across an interrupted-then-resumed sync) rather than which of the two revocation paths gets there first, since that has already changed once underneath this test.
TestResolvedGrantRevokedWhenMatchCriteriaChanges{SQLite,Pebble}: regression test for hazard 3's revert. An external principal's profile changes (still present, no longer matches) between an interrupted attempt and its resume; asserts the stale resolved grant is revoked on both engines. Verified this fails on the annotation-stripped code with exactly that symptom.
TestProcessGrantsWithExternalPrincipalsRecordsSpanErrorOnWriteFailure / TestProcessGrantsWithExternalPrincipalsEndsSpanOnPanic: drive a real mid-scan PutGrants failure and a mid-scan panic, respectively, against a hand-rolled fake sdktrace.SpanExporter, asserting the span is marked as an error and that it's still ended (not leaked) even when the inner call panics. Verified each fails against the specific regression it targets (shadowed err; missing defer).
BenchmarkProcessGrantsWithExternalPrincipals: verified the measured grants-written/op matches the loop's own cost prediction exactly (1001 and 10001 respectively) at both 1k and 10k principal counts, on both engines.
Existing suite (TestExternalResourceMatch*, TestExpandGrant*) passes unchanged, including TestExternalResourceMatchIDWithExpandableRemapping, which exercises the GrantExpandable remapping path this PR restructures, and [CXP-498] Index external principals for grant matching #1046's own principal-index tests.
Full pkg/sync and pkg/dotc1z suites pass with -race.
go vet and golangci-lint clean on the touched files.

linear-code · 2026-07-29T13:29:06Z

CXP-834

github-actions · 2026-07-29T13:34:06Z

General PR Review: Bound memory in processGrantsWithExternalPrincipals's grant rewrite (CXP-834)

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base f7333f66e01d.
Review mode: incremental since cf87a717
View review run

Review Summary

The new commit (c5a5dfa) addresses the prior round's finding directly: the span defer in processGrantsWithExternalPrincipals now recovers a panic, ends the span with an explicit panic: %v error, and re-panics with the original value, so a panicking sync no longer produces an Unset-status span indistinguishable from a success — and TestProcessGrantsWithExternalPrincipalsEndsSpanOnPanic was extended to assert otelcodes.Error alongside the existing "span was ended" check. I verified the re-panic preserves the panic value and that the deferred frames are still on the stack at re-panic time, so the original panic site remains in the printed traceback; the pattern also matches the existing precedent at pkg/uhttp/transport.go:275. The full PR diff was scanned for security and correctness: no injection, secret, auth, or resource-exhaustion issues; I separately re-verified that the mid-scan flush is safe against both engines' paging (SQLite id >= token keyset in grants_expandable_query.go:187, Pebble key-cursor in paginate.go:241 — neither is offset-based, so no unscanned grant can be skipped), that neither PutGrants implementation retains the caller's slice (so the expandedGrantsBuf[:0] reuse is safe), that minimalGrantForDelete keeps exactly the fields grantIdentityFromRecord requires, and that the SQLite DeleteGrant fallback is scoped by sync_id. One doc-only nit below.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

pkg/sync/syncer.go:3469-3471 (medium confidence) — the flushExpandedGrants doc comment claims it is called "once more after the scan loop ends", but the post-loop flush at :3720 is a direct, deliberately-unconditional s.store.PutGrants call; the stale comment invites a future cleanup that would drop the Pebble markDirty side effect on an empty batch.

Prompt for AI agents

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

## Suggestions

In `pkg/sync/syncer.go`:
- Around line 3469-3471: The doc comment on the `flushExpandedGrants` closure says it is
  "Called both mid-scan (once the buffer fills) and once more after the scan loop ends to
  flush any remainder." That second clause is false: the only call site is inside
  `appendExpandedGrant` (line 3493). The post-scan write at line 3720 is a direct
  `s.store.PutGrants(ctx, expandedGrantsBuf...)`, kept unconditional on purpose so Pebble's
  `markDirty` side effect still fires even for an empty batch (see the comment at line 3716).
  Reword the closure comment to say it is called only mid-scan when the buffer fills, and
  cross-reference the line-3716 note explaining why the final write does not go through this
  closure — otherwise a later cleanup could replace line 3720 with `flushExpandedGrants()`,
  which early-returns on an empty buffer and silently drops the dirty-marking side effect.

github-actions

No blocking issues found.

…CXP-834) processGrantsWithExternalPrincipals accumulated every expanded/replacement grant into one slice and wrote it in a single PutGrants call only after its whole grant-scan loop finished. A single ExternalResourceMatchAll grant fans out to one replacement per matching principal, so on large tenants this slice could hold tens of thousands of full Grant protos before a single write -- called out as explicit follow-up ("Not in scope") in baton-sdk#1046, which addresses the O(grants x principals) CPU cost of the matching loop itself but leaves this accumulation untouched. This is a companion, not a replacement, for that PR. Flush expanded grants in bounded batches (externalGrantFlushBatchSize = 500) instead, including from inside the MatchAll fan-out's inner loop, and shrink each deferred grantsToDelete entry to just the fields DeleteGrantByRefs/ DeleteGrant actually need (Id/Entitlement/Principal), dropping bulky Sources/Annotations. grantsToDelete itself still can't be flushed early: the dedup guard against deleting a grant we just rewrote needs the complete newGrantIDs set, which is only final once the whole scan finishes. Interleaving writes with the scan (previously impossible, since nothing was written until after the loop ended) introduced two subtler hazards, both fixed here: - ListWithAnnotations pages read the live engine on each page fetch, not a snapshot. Replacement grants inherit their placeholder's ExternalResourceMatch* annotation, so an early flush could land in a not-yet-scanned key range and get re-encountered and re-expanded by a later page of the same scan. Guarded by skipping any grant whose id is already in the (incrementally-built) newGrantIDs set. - The MatchID branch mutates its replacement grant in place (GrantExpandable remapping) after buffering it. Flushing between the buffer and the mutation would have committed the pre-mutation grant. Fixed by deferring the flush-size check until after all possible mutation for that grant completes. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

github-actions

Blocking issues found — see review comments.

…ants newGrantForExternalPrincipal copied a placeholder's full annotation set onto the replacement grant it built, so a resolved replacement kept carrying the very annotation that got its placeholder into the scan. A restart's fresh, empty newGrantIDs set has no memory of what a prior, interrupted attempt already resolved, so the next attempt re-matched and rewrote every such leftover as if it were still a placeholder. Grant ids are deterministic per (principal, entitlement), so this never produced a duplicate grant record, but it is real, wasted work on every resume, scaling with the number of already-resolved leftovers and dependent on storage-engine scan order (confirmed via reproduction: SQLite's insertion-order scan happened to self-correct, Pebble's did not). This was flagged during #1046's review as retry amplification and never resolved before the memory-bounding work that made it reachable split into this PR. The true placeholder grant is untouched -- it keeps its annotation until processGrantsWithExternalPrincipals's delete loop removes it -- so ingest invariant I9's dangling-principal exemption is unaffected. Adds regression coverage across all three match annotation shapes (ExternalResourceMatchAll, ExternalResourceMatch, ExternalResourceMatchID) and both storage engines, asserting a resumed-after-mid-scan-cut run writes exactly as much as a clean run.

github-actions

No blocking issues found.

Address the review bot's findings on this PR: - Blocking bug: the MatchID branch's GrantExpandable remap buffered the replacement grant before the bid.MakeBid(grant.GetPrincipal()) failure path, whose continue targets the outer grant loop. Pre-batching, that continue skipped the append that would persist the replacement, so the whole attempt was dropped. Now it also skipped the trailing grantsToDelete append -- so a bid failure left a half-resolved replacement grant (missing its expansion remap) persisted alongside its never-deleted placeholder. Fixed by buffering once, only after all mutation completes, restoring the original drop-on-error semantics. Added TestExternalResourceMatchIDBidFailureDropsAttempt, verified against the unfixed code to fail with exactly that symptom. - countingGrantPutStore didn't forward DeleteGrantByRefs, so the syncer's grantByRefsDeleter type assertion always failed on it, silently routing every delete through the id-based fallback -- leaving minimalGrantForDelete's refs-based path untested on any engine. Added a forwarding DeleteGrantByRefs that falls back to DeleteGrant when the underlying store doesn't implement the refs path, so the wrapper stays behavior-preserving on every engine (an unconditional passthrough would have made the type assertion succeed even for SQLite, breaking its fallback). TestExternalResourceMatchAllBatchedFlush now runs its internal store on Pebble and asserts the refs-based path actually fired. - Documented two accepted-as-is suggestions: newGrantIDs remains an O(fan-out) allocation (the memory bound this PR adds is over grant protos, not id strings), and mid-scan flushing causes the scan to read back its own writes (G+N rows instead of G, correctness unaffected).

github-actions

No blocking issues found.

- Fix tracing span error-reporting: the final PutGrants call and every appendExpandedGrant call site used if err := ...; err != nil { return err }, shadowing the outer err the deferred uotel.EndSpanWithError reads. The function still correctly failed the sync -- this only fixes the span's error status, matching the pattern the delete loop right below already used. - countingGrantPutStore and failAfterNPutGrants now both forward DeleteGrantByRefs, falling back to DeleteGrant when the underlying store doesn't implement the refs-based path (so the wrapper stays correct on SQLite too, not just Pebble). TestExternalResourceMatchAllBatchedFlush now runs on Pebble and asserts the refs-based path actually fires. - Pin checkpointInterval in the resume tests: the baseline/resume write-count comparison only holds if resume replays from scratch, which only holds if no non-forced checkpoint lands before the injected cut. A slow CI runner could otherwise land one and fail the test for a reason unrelated to the property it checks. - Add requireExternalMatchBatchFlushedBeforeCut: asserts a resolved external-match replacement actually flushed before the cut, so a future change to page sizing or call ordering can't silently shift the cut earlier and leave these tests covering nothing while still passing. - Hoist externalResourceMatchAnnotationSentinels (matching unsafeForSlimSentinels's existing pattern in pkg/dotc1z/grants.go): stripExternalResourceMatchAnnotations ran once per fan-out principal and was allocating three zero-value protos per call. - Add BenchmarkProcessGrantsWithExternalPrincipals to pin the cost curve of the ExternalResourceMatchAll fan-out on both engines. - Update unsafeForSlim's contract comments (grants.go, c1file.go): the exemption only applies to unresolved placeholder grants now that resolved replacements no longer carry the match annotation. - Collapse bufferExpandedGrant/maybeFlushExpandedGrants/appendExpandedGrant into one closure: after the MatchID fix, no call site used the first two directly, so the three-closure split no longer bought anything. - Add skipChaosInShort to the MatchAll resume test for consistency with the other two (in practice the fastest of the three, not the heaviest). Also adds TestExternalResourceMatchIDBidFailureDropsAttempt, a regression test for a separately-fixed blocking bug: the MatchID branch's GrantExpandable remap buffered its replacement grant before the bid.MakeBid failure path that drops the whole attempt, so a bid failure used to persist a half-resolved grant while leaving the placeholder undeleted. Verified this test fails on the prior code with exactly that symptom.

deleteStaleExternalPrincipals required resourceRecordDeleter, entitlementRecordDeleter, AND grantByRefsDeleter support before doing anything -- but only the Pebble engine implements any of the three, so on SQLite (the default storage engine absent an explicit, manually gated per-tenant opt-in) this whole reconciliation silently no-opped. Before newGrantForExternalPrincipal stopped copying the ExternalResourceMatch* annotation onto resolved replacement grants, that gap was accidentally masked: a resolved grant retained its match annotation forever, so a later scan of processGrantsWithExternalPrincipals would re-evaluate and delete it once its external principal disappeared. With that annotation gone, SQLite lost its only remaining path to revoking access for a departed external principal -- a grant pointing at a principal that no longer exists would persist indefinitely as a phantom. Grant cleanup now always runs, falling back to the id-based DeleteGrant (every store implements it) when the refs-based path isn't available -- the same fallback pattern processGrantsWithExternalPrincipals's own delete loop already uses. Resource/entitlement row cleanup still requires the refs-based deleters and is skipped, with a warning, when unavailable: a leftover stale resource or entitlement row is inert metadata, not live access, so degrading there is the safer tradeoff over skipping the whole pass. Verified against the unfixed code: the new test fails on SQLite with exactly the phantom-grant symptom (the departed principal's grant survives) while Pebble is unaffected either way, confirming the gap was engine-specific and this closes it without touching SQLite's storage engine at all.

github-actions

Blocking issues found — see review comments.

…g (CXP-834) stripExternalResourceMatchAnnotations (8059635) traded an optimization (stop a resumed scan from re-matching already-resolved leftovers) for a real, silent access-control gap: once a resolved replacement grant stopped carrying its ExternalResourceMatch* annotation, processGrantsWithExternalPrincipals's own re-scan could no longer re-verify it. deleteStaleExternalPrincipals (00e76b2) closed the departed-principal half of that gap, but not the case where an external principal still exists and simply stops satisfying the placeholder's match criteria between an interrupted attempt and its resume -- that leftover is invisible to both mechanisms once its annotation is gone, and would silently persist as a phantom grant. This gap is engine-agnostic: deleteStaleExternalPrincipals's departed-principal reconciliation only ever compares principal existence against the current external listing, never match-criteria state, so no amount of refs-based delete/entitlement-record support would have closed the stopped-matching case on either engine. The fix that does close it -- retaining the match annotation so processGrantsWithExternalPrincipals's own re-scan re-verifies resolved grants -- is likewise engine-agnostic. Reverting the optimization and accepting the resulting retry amplification (bounded, real, wasted work on a resume's mid-scan flushes) is the safer tradeoff over a silent, permanent access-control gap, on either engine. newGrantForExternalPrincipal now copies the placeholder's full annotation set again, and stripExternalResourceMatchAnnotations plus its hoisted sentinel var are removed as dead code. The pkg/dotc1z comments this optimization touched (unsafeForSlim, WithC1FV2GrantsWriter) are reverted to state plainly that the exemption covers resolved replacement grants too, not just unresolved placeholders. external_principal_resume_test.go is removed: its entire premise (a resume's write count should equal a from-scratch baseline, with no amplification) no longer holds once amplification is an accepted tradeoff. Its genuinely reusable test infrastructure -- failAfterNPutGrants, errMidScanCut, requireExternalMatchBatchFlushedBeforeCut -- moves to stale_external_principal_test.go, which also gains testResolvedGrantRevokedWhenMatchCriteriaChanges: a regression test for exactly the stopped-matching case above, verified (both as a throwaway pre-revert reproduction and in its final form) to catch the gap on both storage engines.

processGrantsWithExternalPrincipals's deferred uotel.EndSpanWithError read a function-scope err, but a range-over-func loop (for ga, err := range ...) and several per-branch "X, err := ..." declarations each shadowed it in their own block scope. Every one of those nested paths assigned into the nearest shadow, not the function's own err -- so a write failure deep in the scan still correctly failed the sync (the function's own return value was always right) but the deferred closure saw an unset outer err and closed the span as successful regardless. The prior review-comment pass (1f53a72) converted several of those inner "X, err := ..." sites to "X, err = ..." to try to reach the outer err, but := inside a nested block still declares a new variable whenever the enclosing var err error is in an outer block -- switching the operator at the assignment site doesn't change which declaration it resolves to. That fix didn't take. Splitting the function into a thin wrapper (owning the span, a single err assigned exactly once from the inner call's return value, and the defer) and processGrantsWithExternalPrincipalsInner (the unchanged body, including all the pre-existing nested shadows) sidesteps the shadowing entirely instead of chasing it through every branch of a ~350-line function. Verified with a hand-rolled fake sdktrace.SpanExporter (the vendored go.opentelemetry.io/otel/sdk/trace/tracetest package isn't part of this repo's vendor tree, so this avoids adding one just for a verification test): drives a real mid-scan PutGrants failure via failAfterNPutGrants and asserts the resulting span is marked codes.Error. Confirmed this test fails when the fix is reverted (closing over a var declared outside the wrapper, matching the pre-fix shape) and passes against the actual fix.

deleteStaleExternalPrincipals's grant cleanup no longer early-returns on SQLite (00e76b2), so its staleGrants accumulation is now a full grant-keyspace scan on the default engine -- one that was retaining complete grant protos (Sources/Annotations included) for every pending delete across the whole scan, in a PR whose stated purpose is bounding exactly that footprint. minimalGrantForDelete already exists for this: it's the same helper processGrantsWithExternalPrincipalsInner uses for its own pending-delete list, stripping a grant down to just the Id/Entitlement/Principal fields DeleteGrantByRefs and the DeleteGrant id fallback actually read.

BenchmarkProcessGrantsWithExternalPrincipals reported b.ReportMetric(float64(principalCount), "grants/op") -- a loop constant known before the benchmark even ran, not a measurement, so it carried no signal beyond what the sub-benchmark name already encoded. The doc comment also promised "ms/sync", which was never actually reported (the standard go test -bench metric is ns/op). Most importantly, nothing enforced the cost curve this benchmark exists to pin: a regression in the matching loop or the mid-scan read-your-own-writes amplification would only ever show up as a number that quietly got bigger in CI, easy to miss without a side-by-side benchstat comparison. countingStore wraps the real store to tally every grant actually passed to PutGrants across the run. Each iteration's setup is simple enough to state its own cost model exactly: one internal group with a single ExternalResourceMatchAll placeholder grant matched against principalCount external users predicts exactly principalCount+1 grants written (1 native placeholder + one resolved replacement per matched principal, however that fans out across externalGrantFlushBatchSize flushes). The benchmark now asserts the measured total against that prediction and fails outright on any drift -- a hard structural gate, not a timing number -- and reports the real total as grants-written/op. Verified this holds exactly (1001 and 10001 grants written respectively) at both the 1k and 10k principal counts, on both storage engines. Also unifies store construction across engines via dotc1z.NewStore for both SQLite and Pebble (previously SQLite went through the WithC1ZPath option instead), since countingStore needs to wrap the concrete store either way.

github-actions

No blocking issues found.

- processGrantsWithExternalPrincipals: restore the deferred EndSpanWithError call the wrapper split (97ca61f) dropped. That split fixed the span's error status but, in doing so, replaced a defer with a plain post-call invocation -- so a panic inside processGrantsWithExternalPrincipalsInner now skipped EndSpanWithError entirely, leaking the span unended. Restored the defer (err is still assigned exactly once, in this function's own scope, so the original shadowing bug stays fixed). Added TestProcessGrantsWithExternalPrincipalsEndsSpanOnPanic, verified to fail without the defer and pass with it. - testResolvedGrantRevokedWhenMatchCriteriaChanges: cut placeholder count from one-per-user (1050) to 2. Every placeholder in this test shares the same ExternalResourceMatch{department=Sales} annotation and matches all 1050 external users, so 1050 placeholders were 1050 redundant rewrites of an identical resolved-grant set (newGrantForExternalPrincipal keys a replacement on principal+entitlement, not on which placeholder produced it) -- roughly 1.1M grants and ~2,200 PutGrants calls per engine for a test that needs exactly one. 2 rather than 1 keeps the one thing a single placeholder would lose: a flush batch straddling two placeholders' expansions. Runtime for this test dropped from ~27s to ~1s combined across both engines. Verified the reduced fixture still fails on the annotation-stripped code on both engines. - TestExternalResourceMatchAllBatchedFlush: parameterized over both storage engines (...SQLite/...Pebble), gating the Pebble-only DeleteGrantByRefs assertion and asserting its SQLite-side complement (zero refs-based delete calls, id-based fallback instead) rather than simply dropping it. This closes a real gap -- SQLite, the default engine, had no coverage of this test's batch-cap/per-principal-count/ final-grant-set assertions -- but not the one review flagged: disabling the newGrantIDs re-encounter guard and running the full package showed this test's fixture never exercises it on either engine (its grant table is far smaller than one dotc1z page, so the scan never pages a second time and never reads back its own flushes). Added TestExternalResourceMatchAllSkipsItsOwnFlushedReplacements, built with a fixture deliberately larger than one page, as the guard's actual regression test -- verified it turns ~600 replacement writes into ~300,000 with the guard removed. - testDeleteStaleExternalPrincipalsRevokesGrantAfterCutAndShrink: rewrote the doc comment, which described the annotation-stripping regression this PR already reverted as if it were still the current behavior. The test now asserts the end-to-end outcome (no live grant survives a departed principal) rather than which of two revocation paths gets there first, since that has already changed once underneath it and the test can't actually distinguish them: with the match annotation retained, processGrantsWithExternalPrincipals's own re-scan revokes the grant before deleteStaleExternalPrincipals's id-based fallback ever gets a chance to (reverting that fallback back to its old bail-out-without-all-three-deleters behavior still leaves this test green). Added TestExternalPrincipalCleanupFallsBackToIDDeleteWithoutRefsDeleters, a direct unit test against a store exposing none of the optional refs-based deleters, to actually pin the fallback this PR's stated purpose is about -- verified it fails against the old bail-out behavior. Full pkg/sync and pkg/dotc1z suites pass with -race (547s/pkg/sync, down from 894s before the placeholder-count fix above). go vet and golangci-lint clean.

github-actions · 2026-08-14T23:44:52Z

+		} else {
+			// Not merely "skip the Pebble assertion": pinning this at zero is
+			// what proves countingGrantPutStore stayed behavior-preserving on
+			// an engine with no grantByRefsDeleter, rather than quietly
+			// steering SQLite onto a path it doesn't have. The placeholder
+			// still has to be gone -- the final grant-set check below covers
+			// that on both engines.
+			require.Zero(t, counting.deleteByRefsCalls, "SQLite has no refs-based delete; the placeholder must go through the id-based DeleteGrant fallback")


🟡 Suggestion: countingGrantPutStore declares DeleteGrantByRefs unconditionally, so the syncer's own s.store.(grantByRefsDeleter) assertion (syncer.go:3753) succeeds on SQLite too — the syncer takes the refs branch on both engines and its else { s.store.DeleteGrant(...) } fallback at syncer.go:3761 is never reached here. The id-based delete happens one level down, inside the wrapper. So this assertion pins "the underlying store is not a refs deleter", not "SQLite goes through the syncer's id-based fallback" as the comment (and the doc block at 1024-1030) states. The syncer branch is covered elsewhere by tests that pass a raw store, so this is a comment-accuracy issue rather than a coverage hole.

Agreed on the mechanism, and agreed with your own downgrade -- this is a comment-accuracy issue, not a coverage gap. Since countingGrantPutStore always implements DeleteGrantByRefs, the syncer's type assertion takes the refs-branch on both engines here; the id-based delete happens inside the wrapper's own fallback, not through syncer.go's else branch at 3761. The syncer's own id-based-fallback branch is exercised elsewhere by tests using a raw store.

Not changing the code for this -- the assertion is still correct (it pins "the underlying store isn't a refs deleter"), just not exactly what the surrounding comment/doc-block claims it's exercising. Leaving as-is rather than reworking the fixture for a wording-only issue.

github-actions

No blocking issues found.

Pass over every comment this PR added or modified (scoped to the diff against #1046's merge base, not pre-existing comments in touched files), condensing verbose "why" explanations down to the same information in less text. No behavior change; content-only edit. Two side effects worth noting: - The re-encounter check in processGrantsWithExternalPrincipalsInner duplicated most of the cost-note comment directly above it; the duplication is gone, with the check's comment now just pointing at the cost note for mechanism instead of re-deriving it. - A doc comment on testResolvedGrantRevokedWhenMatchCriteriaChanges referenced a test name (...AfterCut) that doesn't match the actual function; renamed the reference to the real name while trimming it. go build, go vet, and golangci-lint clean. Full pkg/sync and pkg/dotc1z suites pass with -race (594s/pkg/sync).

github-actions

No blocking issues found.

processGrantsWithExternalPrincipals's defer restored panic-safety (the span still gets ended) but not error-status-on-panic: err is still nil at the point a panic skips the assignment below it, so EndSpanWithError(span, nil) left the span's status Unset -- a panicking sync produced a span indistinguishable from a successful one, the same "silently closed as successful" failure this whole wrapper/inner split exists to fix, just manifesting on the panic path instead of the returned-error path. The defer now recovers just long enough to mark the span as an error (RecordError + SetStatus(Error)) before re-panicking, so the panic still propagates to the caller exactly as before. TestProcessGrantsWithExternalPrincipalsEndsSpanOnPanic previously only asserted the span was exported (i.e. not leaked), which this bug would have passed. Extended it to also assert the span's status is Error. Verified it fails without the recover (status stays Unset) and passes with it.

github-actions · 2026-08-15T09:45:00Z

+	// flushExpandedGrants writes and clears the current batch. Called both
+	// mid-scan (once the buffer fills) and once more after the scan loop
+	// ends to flush any remainder.


🟡 Suggestion: this doc comment is stale — flushExpandedGrants is only ever called from appendExpandedGrant (line 3493); the post-loop flush at line 3720 is a direct s.store.PutGrants(ctx, expandedGrantsBuf...), deliberately unconditional so the Pebble markDirty side effect still fires on an empty batch. As written, the comment invites a future "cleanup" that swaps line 3720 for flushExpandedGrants() and silently drops that side effect. Suggest dropping the "and once more after the scan loop ends" clause and pointing at line 3716's note instead. (medium confidence, doc-only)

github-actions

No blocking issues found.

c1-squire-dev Bot requested a review from a team July 29, 2026 13:29

c1-squire-dev Bot force-pushed the john.allers/CXP-834/batch-external-match-grant-writes branch from aae9965 to d07d136 Compare July 29, 2026 14:13

c1-squire-dev Bot changed the base branch from main to jallers/cxp-498-external-principal-index July 29, 2026 14:13

github-actions Bot reviewed Jul 29, 2026

View reviewed changes

c1-squire-dev Bot force-pushed the john.allers/CXP-834/batch-external-match-grant-writes branch from d07d136 to f455016 Compare August 3, 2026 14:01

johnallers marked this pull request as draft August 3, 2026 16:17

c1-squire-dev Bot force-pushed the jallers/cxp-498-external-principal-index branch 2 times, most recently from 985c6d8 to 532bc49 Compare August 6, 2026 17:18

Base automatically changed from jallers/cxp-498-external-principal-index to main August 13, 2026 20:48

johnallers force-pushed the john.allers/CXP-834/batch-external-match-grant-writes branch from f455016 to 925424a Compare August 14, 2026 10:00