fix: validate snapshot hint and CRC invariants - #3301
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3301 +/- ##
=======================================
Coverage 90.35% 90.36%
=======================================
Files 251 251
Lines 89404 89496 +92
Branches 89404 89496 +92
=======================================
+ Hits 80784 80870 +86
- Misses 5685 5687 +2
- Partials 2935 2939 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
the
breaking-change
Benchmark results: ✅ PassSummary: 🚀 0 · ✅ 10 · ☑️ 5 · 🚧 0 · ❌ 0 Per-benchmark results (15 rows)
Legend: 🚀 ≥1.15x faster · ✅ faster or unchanged · ☑️ ≤1.03x slower · 🚧 1.03x-1.15x slower · ❌ ≥1.15x slower |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review (draft - human review required)
Show review
This PR adds three sanity checks: snapshot hints must carry a commit at their target version, CRC in-commit timestamps must be present exactly when ICT is enabled, and file-size histograms must be feasible for their bins and match the CRC totals. The histogram math is correct and conservative (i128 per-bin products, i64 checked_add for totals, exclusive upper bound, unbounded last bin), and the tests are well parameterized. No blocking issues.
Non-blocking notes
Nit1: kernel/src/crc/writer.rs:35 (RIGHT). The write path derives ICT enablement from the metadata property alone (configuration().get(ENABLE_IN_COMMIT_TIMESTAMPS) == "true"), while the hint path in builder.rs uses table_configuration.in_commit_timestamp_enablement(), which requires both the InCommitTimestamp feature and the property. Since the write check is now ict_enabled == ict_value_present, a CRC whose metadata sets the property to true without the protocol feature is treated as enabled by the writer but not-enabled by the hint path, so the two paths can disagree on the same CRC. This is only reachable with a malformed table (TableConfiguration::try_new does not reject property-without-feature), so it is low priority, but the divergence is real.
Raised by: architecture-reviewer, maintainer-claude-reviewer, maintainer-codex-reviewer, delta-protocol-reviewer.
Suggested fix: derive enablement the same way in both places, ideally a shared feature-plus-property helper used by the writer and the builder.
Nit2: kernel/src/snapshot/builder.rs:596 (RIGHT). The new require! rejects any hint whose LogSegment lacks a commit at the target version. This is stricter than kernel's general construction path (log_segment/mod.rs only requires the target commit when the commit list is non-empty) and than the protocol, where a snapshot at a checkpoint version is loadable from the checkpoint alone. A legitimate checkpoint-only snapshot cannot be expressed as a hint. This looks intentional and is defensible because Snapshot::get_timestamp already depends on the target commit being present, but the stricter-than-general behavior is not documented.
Raised by: maintainer-claude-reviewer, delta-protocol-reviewer.
Suggested fix: add a comment at this require! explaining that hints must include the target-version commit even when a checkpoint alone would suffice, and that this is deliberately stricter than general construction (ties into #3293).
Nit3: kernel/src/crc/file_size_histogram.rs:326 (RIGHT). The file_count == num_files and total_bytes == table_size_bytes checks are the central histogram-to-CRC cross-validation, but no test reaches them: the impossible-bin cases fail earlier in the per-bin loop, and the overflow cases fail at checked_add before the equality comparison. If either comparison were inverted or dropped, a mismatched histogram would validate silently on both the read and write paths.
Raised by: test-coverage-reviewer.
Suggested fix: add a case with a bin-consistent histogram whose totals disagree with num_files / table_size_bytes so control reaches both equality checks, for example:
#[case::num_files_mismatch(vec![0, 10], vec![3, 0], vec![30, 0], 2, 30, "does not match numFiles")]
#[case::table_size_mismatch(vec![0, 10], vec![3, 0], vec![30, 0], 3, 99, "does not match tableSizeBytes")]Nit4: kernel/src/crc/file_size_histogram.rs:308 (RIGHT). The upper-bound feasibility check bytes < upper * count is looser than achievable: count files each strictly below upper can sum to at most (upper - 1) * count, so bin [0, 10) with count 2 wrongly accepts bytes = 19 (max is 18). This never rejects valid data, so it is only a slightly weaker integrity check, not a correctness problem.
Raised by: delta-protocol-reviewer.
Suggested fix: optionally tighten to bytes <= (upper - 1) * count.
Summary
This is a focused hardening change with correct histogram feasibility math, safe removal of is_complete() from production, and good test coverage across most branches. There are no blocking issues. The two most useful follow-ups are unifying the ICT-enablement definition between the write and hint paths and documenting the intentionally stricter target-commit requirement for hints; the histogram totals-mismatch branches also lack direct test coverage. The docs-reviewer found no doc/code mismatches.
Automated review - workflow run
| .validate_complete(stats.num_files(), stats.table_size_bytes()) | ||
| .map_err(|error| Error::ChecksumWriteUnsupported(error.to_string()))?; | ||
| } | ||
| let ict_enabled = crc |
There was a problem hiding this comment.
Nit1 The write path derives ICT enablement from the metadata property alone (configuration().get(ENABLE_IN_COMMIT_TIMESTAMPS) == "true"), while the hint path in builder.rs uses in_commit_timestamp_enablement(), which requires both the InCommitTimestamp feature and the property. With the write check now ict_enabled == ict_value_present, a CRC whose metadata sets the property to true without the protocol feature is treated as enabled by the writer but not-enabled by the hint path, so the two paths can disagree on the same CRC. Only reachable with a malformed table (TableConfiguration::try_new does not reject property-without-feature), so low priority, but real. Raised by: architecture-reviewer, maintainer-claude-reviewer, maintainer-codex-reviewer, delta-protocol-reviewer. Suggested fix: derive enablement the same way in both places, ideally a shared feature-plus-property helper.
| .is_none_or(|published_version| published_version <= version), | ||
| SnapshotHintError::MaxPublishedVersion { hint: version }.into() | ||
| ); | ||
| require!( |
There was a problem hiding this comment.
Nit2 This require! rejects any hint whose LogSegment lacks a commit at the target version, which is stricter than kernel's general construction path (only requires the target commit when the commit list is non-empty) and than the protocol, where a snapshot at a checkpoint version is loadable from the checkpoint alone. A legitimate checkpoint-only snapshot cannot be expressed as a hint. This looks intentional and is defensible because Snapshot::get_timestamp already depends on the target commit, but the stricter behavior is undocumented. Raised by: maintainer-claude-reviewer, delta-protocol-reviewer. Suggested fix: add a comment explaining hints must include the target-version commit even when a checkpoint alone would suffice, and that this is deliberately stricter than general construction (#3293).
| sum.checked_add(*bytes) | ||
| .ok_or_else(|| Error::internal_error("Histogram total bytes overflow")) | ||
| })?; | ||
| require!( |
There was a problem hiding this comment.
Nit3 The file_count == num_files and total_bytes == table_size_bytes checks are the central histogram-to-CRC cross-validation but are never reached by any test: impossible-bin cases fail earlier in the per-bin loop and overflow cases fail at checked_add before the equality comparison. An inverted or dropped comparison would let a mismatched histogram validate silently. Raised by: test-coverage-reviewer. Suggested fix: add a case with a bin-consistent histogram whose totals disagree with num_files / table_size_bytes, e.g. #[case::num_files_mismatch(vec![0,10], vec![3,0], vec![30,0], 2, 30, "does not match numFiles")] and a tableSizeBytes variant.
| let aggregates_are_possible = (count == 0 && bytes == 0) | ||
| || (count > 0 | ||
| && bytes >= lower * count | ||
| && upper.is_none_or(|upper| bytes < upper * count)); |
There was a problem hiding this comment.
Nit4 The upper-bound feasibility check bytes < upper * count is looser than achievable: count files each strictly below upper sum to at most (upper - 1) * count, so bin [0, 10) with count 2 wrongly accepts bytes = 19 (max is 18). This never rejects valid data, so it is only a weaker integrity check, not a correctness problem. Raised by: delta-protocol-reviewer. Suggested fix: optionally tighten to bytes <= (upper - 1) * count.
jtuglu1
removed
the
breaking-change
|
Naive comment from someone reviewing on mobile: I don't see a file name in this PR that has "hint" in its name. That is, this PR seems very CRC centric but doesn't seem to touch anything called a SnapshotHint? Have we already started shipping SnapshotHint? Apologies if I've missed something |
Hint was merged here. FFI layer is coming in part 2 here. For what's related to the hint, this is specifically trying to fix #3293 which was brought up during the hint PR. TLDR; it is adding a few more validations that are used by snapshot hint (and in the case of CRC/file size histogram, normal log replay). I can split the latter 2 up if necessary. |
2 participants
What changes are proposed in this pull request?
Validate that:
Normal replay and snapshot hints have different input contracts.
The normal replay path in
SnapshotBuilder::build()must tolerate metadata cleanup. After the configured retention period, cleanup may remove JSON commits covered by a retained checkpoint. For example:00000000000000000000.jsonwhile retaining the version 0 checkpoint.latest_commit_file.The checkpoint contains enough state to reconstruct the table, but operations requiring target-commit metadata may fail. For example, the commit timestamp is unavailable, and enabling ICT in a subsequent commit may require that timestamp to preserve timestamp monotonicity.
A snapshot hint instead claims to provide complete cached snapshot state without listing storage. Its producer is expected to retain target-commit metadata independently of the files needed for replay. Kernel treats a hint without that metadata as incomplete rather than silently constructing a snapshot with unavailable timestamp information. The caller can discard the rejected hint and retry through normal replay.
The broader normal-replay behavior remains unchanged; see #3293.
How was this change tested?
cargo +nightly fmtcargo clippy -p delta_kernel --tests --all-features -- -D warningscargo doc -p delta_kernel --all-features --no-depscargo nextest run -p delta_kernel --all-features(9,135 passed)