fix(chunker): platform-independent hash_item + collapse single-child root - #193
Closed
presempathy-awb wants to merge 2 commits into
Closed
fix(chunker): platform-independent hash_item + collapse single-child root#193presempathy-awb wants to merge 2 commits into
presempathy-awb wants to merge 2 commits into
Conversation
The tree is content-addressed, so equal get_hash() => byte-identical subtree. The old diff descended incorrectly and could silently drop changes (root-only style data loss). Rewrite diff_nodes_recursive to: short-circuit equal-hash subtrees, merge-join leaf pairs, and for internal nodes skip children whose stored child-hash matches (O(diff), no load) — flattening only the divergent region. Kept honest by a #[cfg(test)] full-leaf flatten oracle (diff_nodes_flatten) that every new differential test asserts byte-identical against, plus ground-truth add/remove/modify assertions. Old diff_recursive retained (dead_code). Uses only existing APIs; no format/hash change. Tests: mod odiff_differential.
…single-child root Two content-defined-chunker canonicality bugs: 1. hash_item fed the item through the slice Hash impl, which prefixes a platform-width usize length (8 bytes native, 4 on wasm32) before the bytes — so chunk boundaries, and thus every tree's shape and root hash, DIFFER between 32- and 64-bit targets. Use hasher.write(item) to hash the raw content bytes only (in streaming_chunker and in ProllyNode's NodeChunk). Deterministic across targets. 2. A content-defined boundary on the final item could leave the root a single-child internal node — a non-minimal spine diverging from a fresh batch build. Collapse single-child internal roots in done(). Because merge() diffs internally, the new boundaries surface the root-only diff data-loss bug during merge, so this fix REQUIRES the canonical-structural-diff fix (this branch is stacked on it). Golden value in test_delete updated for the corrected boundary. Tests: mod chunker_invariants.
zhangfengcdt
approved these changes
Aug 25, 2026
zhangfengcdt
left a comment
zhangfengcdt
left a comment
Owner
There was a problem hiding this comment.
Reviewed: implementation and tests pass; no public API surface change detected.
Owner
|
Resolved and integrated via #244 after CI passed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bugs
Platform-dependent chunking.
hash_itemfed the item through the sliceHashimpl (<[u8] as Hash>::hash), which prefixes a platform-widthusizelength (8 bytes on 64-bit, 4 on wasm32) before the bytes. So content-defined chunk boundaries — and therefore every tree's shape and root hash — differ between 32- and 64-bit / wasm targets. Fixed in bothstreaming_chunkerandProllyNode'sNodeChunkby hashing the raw content bytes:hasher.write(item).Single-child root. A content-defined boundary on the final item can leave the root a single-child internal node — a non-minimal spine that diverges from a fresh batch build.
done()now collapses single-child internal roots to their child.Why it depends on #191
The corrected boundaries surface the root-only diff data-loss bug during
merge(merge diffs internally). With the chunker fix alone, the existingmerge_canonicality_testsfail (4/4); with #191 present they pass. Verified: chunker-alone → 4merge_canonicalityfailures; chunker + #191 → full suite green (272 tests).Tests
mod chunker_invariants(proptest: no single-child internal nodes over the default config).test_delete's golden traverse is updated for the corrected, platform-independent boundary ([17]now seals its own chunk).Verification
cargo test(with #191) — full suite green. Based onmain@373128e.