Grant Hash Calc V2 - #1089
Grant Hash Calc V2#1089
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // silently turned either back into a no-op (regressing to v1's blind | ||
| // spot — see grantContentHash64's ABI doc) would otherwise go | ||
| // unnoticed. | ||
| func TestGrantContentHashDistinguishesImmutabilityAndDirectness(t *testing.T) { |
There was a problem hiding this comment.
🟡 Suggestion: every immutable case in this file makes GrantImmutable the only annotation, so the multi-entry branch of scanGrantContentFactsRawBytes's case 8 loop is untested. Add a case where the annotation list carries another annotation (e.g. GrantMetadata) before/after GrantImmutable, plus one where a GrantExpandable was stripped by V2GrantToV3 — a raw-scan bug that stopped at the first Any would silently regress isImmutable to false for real connector grants, and the seal path has no oracle that would notice.
General PR Review: Grant Hash Calc V2Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness: Risk triage (per
Verified the escape paths are closed rather than assumed: Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsReviewed at head |
|
I poked at this for a bit: Review Brief:
|
grantContentHash64 previously hashed only the grant's identity tuple plus the set of source-entitlement ids, deliberately excluding all annotations and source-map values as sync-transient noise. But GrantImmutable and per-source is_direct are stable per-grant facts (not bookkeeping that churns every sync) that the SDK already uses elsewhere to distinguish otherwise-identical grants (rollback_expansion's suspect-grant check, topological_merge's direct-wins-over-indirect upgrade) — so two grants differing only in immutability or provenance were silently digesting as identical. Bumps GrantDigestABIVersion to 2 since this changes what "the same grant" hashes to. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The seal-time digest build hashes a grant's stored value bytes with a raw protowire field scan instead of a proto unmarshal. That scan disagreed with proto.Unmarshal on wire shapes proto.Marshal never emits but the decoder accepts: a repeated is_direct or type_url took the first occurrence (proto: last wins), a duplicated map-entry value fragment replaced rather than merged, duplicate map keys were both folded into the hash (proto: last entry wins), and a known field number carrying the wrong wire type errored (proto: unknown field). The engine stores and compacts value bytes verbatim, so any such bytes would hash one way at seal time and another way for readers and GrantContentHash, forever. Fix the scanner to follow the decoder, and make sortGrantSourceFacts collapse equal keys after its stable sort. No hash framing or GrantDigestABIVersion change: SDK-written bytes carry no duplicates, so no stored hash changes. Pin it two ways: a table-driven differential test over each duplicate shape, and a fuzz target whose seeds and saved crasher run as unit tests under plain go test. The first fuzz round found the wire-type case in two seconds; after the fix a 90s run was clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The missing-stamp test was written while GrantDigestABIVersion was 1, where the correct Open behavior was to keep the state, and skipped its Open half once the constant moved. With the constant at 2 the live contract is the drop: a missing stamp reads as version 1, whose hashes omit isImmutable and is_direct, so a writable Open must drop the nodes and hash index and the next seal must rebuild and stamp them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
60a8496 to
03bebe3
Compare
| // Exported so consumers of GrantContentHash / GrantDigestAccumulator | ||
| // can check a stored root's abi_version before comparing. | ||
| const GrantDigestABIVersion uint32 = 1 | ||
| const GrantDigestABIVersion uint32 = 2 |
There was a problem hiding this comment.
🟡 Suggestion: with the constant now 2, the unstamped-file paragraph three lines above (68–71: "absence reads as grantDigestABIVersionUnstamped and is current for as long as this constant stays 1 — introducing the stamp costs no rebuild") describes behavior this PR's own TestGrantDigestABIMissingStampReadsAsVersion1 now asserts is false — a pre-stamp file is dropped and rebuilt on writable Open. Reword it to say absence reads as 1 and is therefore stale at ABI ≥ 2, so the reader of this doc reasons correctly about pre-stamp files in a mixed-version fleet.
There was a problem hiding this comment.
Reworded — the paragraph now says plainly that a missing stamp reads as version 1 and is stale at ABI >= 2, instead of a conditional that only read as current when the stamp was introduced. f94606a
| require.NoError(t, verifyGrantHashIndexAgainstPrimaries(t, e2), "repaired state must check out against the primaries") | ||
| require.NotZero(t, digestNodeCount(t, e2), "reseal must rebuild digest nodes") | ||
| require.EqualValues(t, n, entHashIndexRowCount(t, e2, entID), "reseal must rebuild every hash-index row") | ||
| require.NoError(t, verifyGrantHashIndexAgainstPrimaries(t, e2), "oracle must pass over the rebuilt state") |
There was a problem hiding this comment.
🟡 Suggestion: this oracle (verifyGrantHashIndexAgainstPrimaries, the one instrument that pins the seal-time raw scan against decode-then-hash through the real spill-sorter/SST path) only ever runs over makeTestGrants → makeGrant, which sets no annotations and no sources; digest_test.go's makeGrantWithSources builds GrantSourceRecord_builder{}, so is_direct is false there too. Every oracle-verified grant therefore takes grantContentHash64's !isImmutable && len(sortedSources)==0 early return or the all-false path — the two facts ABI v2 exists for are never exercised end-to-end. That matters most for the highest-volume producer: fillSynthGrantRecord stamps GrantImmutable on every synthesized grant and appendGrantSourcesWire hand-encodes is_direct in bytes that never pass through proto.Marshal. Seal a fixture with an immutable grant and a is_direct: true source (and one expanded grant written via PutExpandedGrantRecords) and run this oracle over it.
There was a problem hiding this comment.
Added TestGrantDigestABIOracleCoversImmutableAndDirectSources: seals one entitlement mixing plain grants, an immutable grant, a grant with an is_direct:true source, and a grant written through PutSynthesizedGrantContributions (the expander's hand-encoded-wire path, not proto.Marshal), then runs verifyGrantHashIndexAgainstPrimaries over it. Confirmed it's a real oracle by temporarily disabling the raw-scan isImmutable detection locally — this test failed while the pre-existing TestGrantDigestABIOracle kept passing, then reverted. f94606a
| return fmt.Errorf("grant hash index: primary key %x did not split as a 6-segment identity", primaryKey) | ||
| } | ||
| srcs, err := scanGrantSourceKeysRawBytes(value, s.srcKeys[:0]) | ||
| isImmutable, srcs, err := scanGrantContentFactsRawBytes(value, s.srcKeys[:0]) |
There was a problem hiding this comment.
🟡 Suggestion: field 8 used to be skipped by a single ConsumeFieldValue; now every grant's annotation list is walked and each Any's type_url tail compared. Big-O is unchanged and scanAnyEntryIsTypeRaw is genuinely alloc-free, but this is a new per-row constant on the seal loop that runs ~50M times per whale expansion — and every synthesized grant carries exactly one GrantImmutable Any, so the new descent is taken on all of them. BenchmarkGrantWriteScale / BenchmarkRegisteredPebbleWritePack (vs their _NoDigestIndex variants) already isolate this path; per docs/BUG_CATCHING.md's cost-contract rule, a stated ns/op + B/op delta from those would close it.
There was a problem hiding this comment.
Added BenchmarkRegisteredPebbleWritePackImmutable + its _NoDigestIndex twin (grants all carrying GrantImmutable, the real synthesized-grant shape) and stated the cost contract on appendGrantHashIndexRow's doc comment. Measured at grants=100000, -benchtime=8x, isolating digest-on vs digest-off to strip the unrelated write-path marshal cost: B/op delta is unmeasurable (139.4MB vs 139.8MB) confirming scanAnyEntryIsTypeRaw is alloc-free, but ns/op attributable to the digest build goes ~242ns/grant -> ~402ns/grant (~66% increase in this function's own per-row cost), ~8s added seal time at 50M grants. f94606a
- Reword the unstamped-file paragraph in grant_digest.go to state plainly that a missing stamp reads as version 1 and is stale at ABI >= 2, matching TestGrantDigestABIMissingStampReadsAsVersion1's assertion. - Extend the hash-index oracle (verifyGrantHashIndexAgainstPrimaries) to run over a seal that mixes an immutable grant, a direct source, and a grant written through the expander's hand-encoded wire path (PutSynthesizedGrantContributions) — the two v2 facts (isImmutable, is_direct) were never exercised end-to-end through the real seal build by any oracle-verified test before this. - Add BenchmarkRegisteredPebbleWritePackImmutable (+ _NoDigestIndex twin) and state the measured cost-contract delta on appendGrantHashIndexRow: no measurable extra allocation, ~242ns/grant -> ~402ns/grant digest-build time with a GrantImmutable annotation on every grant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| continue | ||
| } | ||
| srcs, serr := scanGrantSourceKeysRawBytes(value, scratch.srcKeys[:0]) | ||
| isImmutable, srcs, serr := scanGrantContentFactsRawBytes(value, scratch.srcKeys[:0]) |
There was a problem hiding this comment.
🟡 Suggestion: repairOneGrantDigestPartitionLocked is a second, independently-written copy of the scan → sortGrantSourceFacts → grantContentHash64 sequence, but nothing oracle-verifies it over the two new v2 facts. TestGrantDigestABIOracleCoversImmutableAndDirectSources reaches only the seal build (sealGrantDigests → BuildDeferredGrantIndexes), and every repair-path test (grant_digest_repair_test.go, endsync_repair_test.go, compactor_grant_digest_test.go) builds rows via makeGrant/makeGrantWithSources, which set no annotations and leave is_direct false — so both still take grantContentHash64's early return. A drift between the two writers would leave one file's hash index internally inconsistent per partition: silent and durable, exactly the gap the previous round raised for the seal path. Consider adding an immutable + is_direct: true grant to a repair/invalidate test and asserting verifyGrantHashIndexAgainstPrimaries. (confidence: high)
| // gets one; see fillSynthGrantRecord) that benchmarkGrants itself does not | ||
| // cover, so a benchmark built on this is the one that costs the grant-digest | ||
| // seal-time annotation walk instead of taking its early return. | ||
| func benchmarkGrantsImmutable(n int) []*v2.Grant { |
There was a problem hiding this comment.
🟡 Suggestion: this fixture gives every grant exactly one annotation, and that annotation is GrantImmutable — so scanGrantContentFactsRawBytes matches on the first field-8 entry and its if !isImmutable guard short-circuits the rest. That's the cheapest non-trivial shape, yet it is what the +160 ns/grant cost contract in grant_digest_build.go:66 is derived from. The expensive shape is unmeasured: a grant with several annotations and no GrantImmutable (every connector-emitted, non-expanded grant carrying GrantMetadata etc.) never short-circuits, so scanAnyEntryIsTypeRaw descends into and scans the type_url of every Any — where v1 skipped all of field 8 with one ConsumeFieldValue per entry. Consider a third fixture with k non-matching annotations so the stated delta bounds the worst case rather than the best. (confidence: medium)
2 participants
grantContentHash64 previously hashed only the grant's identity tuple
plus the set of source-entitlement ids, deliberately excluding all
annotations and source-map values as sync-transient noise. But
GrantImmutable and per-source is_direct are stable per-grant facts
(not bookkeeping that churns every sync) that the SDK already uses
elsewhere to distinguish otherwise-identical grants (rollback_expansion's
suspect-grant check, topological_merge's direct-wins-over-indirect
upgrade) — so two grants differing only in immutability or provenance
were silently digesting as identical. Bumps GrantDigestABIVersion to 2
since this changes what "the same grant" hashes to.
Also adds new tests and consistency to grant hash calculation.
(A previous version of this PR included the version stamping to make
upgrading safe, that was already merged on its own in advance)