Entitlement Grant Hash Index & Entitlement Grant Digest Tree#999
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return &Options{ | ||
| durability: DurabilitySync, | ||
| slowQueryThreshold: 5 * time.Second, | ||
| grantDigestIndex: true, |
There was a problem hiding this comment.
🟡 Suggestion: Defaulting grantDigestIndex to true means every Pebble seal now runs an extra full O(grants) scan + spill-sort + ingest + digest fold — including the common PutGrantRecords path that previously ran no deferred pass at all (endSyncFinalize now calls BuildGrantDigests on that branch). This adds seal-time latency and disk I/O for all existing downstream callers, most of which never grant-diff. The repo's own compat guidance prefers new behavior to default to the old behavior and be opt-in. It's mitigated by WithGrantDigestIndex(false) and is documented, so this is non-blocking — but consider defaulting off (or at least calling out the added seal cost in the PR/rollout notes). (medium confidence)
General PR Review: Entitlement Grant Hash Index & Entitlement Grant Digest TreeBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness, with focus on the SDK/wire-compatibility surfaces this repo demands: the new digest core ( Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
37b586c to
9a0012a
Compare
| } | ||
| } | ||
| if e.grantDigestsPresent.Load() { | ||
| if id, err := grantIdentityFromRecord(r); err == nil { |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): this swallows the error from grantIdentityFromRecord(r) — on the err != nil branch the touched entitlement's digest is silently not invalidated, which would leave a stale-but-present digest and violate the present-means-exact contract. In practice a record that reaches writeGrantIndexes already had its identity derived to build the primary key, so this is likely unreachable; but the sibling paths (writeGrantIndexesForIdentityScratch, deleteGrantIndexesScratch) can't fail-open here. Consider propagating the error instead of dropping it.
|
Final brief: @mj-palanker do we really want to exclude sources from grant hashes? Thats user facing data, and maybe goes into ticket processing? @alan-lee-12 you possibly know best ^ Brief: grant-digest review fixes (mjp/grant-digest-v2)Findings from a correctness/error-recovery review of the branch. One Fix 1 (must-fix): standalone build crash window breaks "root present ⇒ index present"
Failure sequence: inline-only sync (deferred marker never armed) → Fix options (pick one):
Verification: a test that kills the build between the digest-node Fix 2:
|
51bf06c to
278309f
Compare
| // partition turns out to be missing. Orphan grant-bearing entitlements | ||
| // without a record are not covered — see RepairMissingGrantDigests's | ||
| // doc comment. | ||
| func (e *Engine) findMissingGrantDigestPartitionsLocked(ctx context.Context) ([]string, error) { |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): findMissingGrantDigestPartitionsLocked walks only entitlement records, so an orphan partition (grants with no entitlement record) that was invalidated by InvalidateGrantDigestPartitions during a fold is never rediscovered or rebuilt here. recomputeGrantDigestGlobalRootLocked then folds only the roots that remain stored, so the whole-file grant_digest_root written after a fold excludes a touched orphan — whereas a seal-time grantDigestFold includes orphan grants in the global root. The manifest whole-file root can therefore represent the same grant set differently depending on whether it came from a seal vs. a fold+repair, which weakens (and in contrived symmetric cases could falsely-match) the header-only "did anything change?" check. Orphans are an acknowledged edge case, but consider either rebuilding invalidated partitions that still hold grants regardless of an entitlement record, or documenting that the global root is orphan-lossy after a targeted repair.
Compacted outputs previously shipped with no grant-digest state in every merge mode (fold/k-way/overlay), defeating the digest feature for exactly the files downstream diff consumers ingest most. After each mode's merge finishes writing the final grant keyspace, the compactor now calls the existing standalone Engine.BuildGrantDigests (the same lean build EndSync runs when the deferred index pass didn't fire), giving compacted files digests indistinguishable from a seal-time build without re-deriving by_principal/the primary grant keyspace, and with the same fail-safe (drop-and-continue) semantics. Also adds a whole-file grant digest root to the v3 envelope manifest (GrantDigestRoot: xor_digest/count/abi_version) so a consumer can answer "did anything change?" from a header read. It's a running (xor, count) accumulator threaded through the same build that writes per-entitlement digest nodes, and is dropped by the same invalidation paths whenever any partition's digest goes stale. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4b032ea to
f516a82
Compare
| // (expansion NOT skipped) pays for both the targeted repair AND the | ||
| // subsequent full rebuild. See | ||
| // TestCompactPebbleFoldWithExpansionRebuildsFullyRegardless. | ||
| if len(foldStats.TouchedGrantPartitions) > 0 && destEng.GrantDigestIndexEnabled() { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): the touched-partition invalidation is gated on destEng.GrantDigestIndexEnabled(), but the fold's initial byte copy (see bucket_plans.go's digest / grant_by_entitlement_principal_hash entries) copies the base's digest + hash-index keyspaces unconditionally. If the dest engine is opened with WithGrantDigestIndex(false) while the base was sealed with digests, this branch is skipped, rebuildCompactedGrantDigests is not called for fold, and no EndSync rebuild runs on the skip-expansion path — so the copied digests for touched entitlements stay present-but-stale, violating present-means-exact for any downstream reader (readers trust stored digests regardless of the writer's flag). Consider dropping all digest state in the else branch when grants were touched but the index is disabled (or skipping the digest bucket-plan copy when disabled).
2 participants
Adds a new "hash index" which stores a descriptive hash of the content of a grant on a new index which is shaped exactly like the normal grant key but with a new 2 bytes of the hash of the principal keys. This distributes all grants for an entitlement randomly across the keyspace when considering only the hash. A "bucket" is just a prefix scan of those 2 bytes, meaning you can chop the grant space into as many as 2^16 even buckets.
Also introduces the concept of a grant digest which is the XOR of all grant hashes for a given bucket (plus the # of grants in said bucket). By default we always create a root digest storing info on all the entitlement's grants, plus a set of "leaf nodes" for some desired "depth" into the tree.
This desired depth is calculates such that (if distributed evenly) we have no more than 512 grants per bucket.
16 bits lets this hold until we hit 33.5 Million Grants on a single entitlement.
since this desired depth calculated value isn't assumed ever, we could change this in the sdk without breaking anything
Any stored depth can be rolled up into any less granular depth simply by XOR-ing the hashes of nodes, and adding the counts, meaning you can trivially compare grant digests stored at different resolutions. We may want to leverage this to store a fixed max resolution in c1.
Derived from #977 via robot onto main
perf tests show we doubled the End Sync time, but that's from ~25ms to ~50ms