Skip to content

Add snapshot completeness validation - #65

Open
corylanou wants to merge 1 commit into
mainfrom
snapshot-validation
Open

Add snapshot completeness validation#65
corylanou wants to merge 1 commit into
mainfrom
snapshot-validation

Conversation

@corylanou

Copy link
Copy Markdown
Collaborator

Summary

  • Adds validation to ensure snapshot LTX files contain all expected pages
  • Implements the TODO comment in decoder.go about verifying last read page equals commit for snapshots
  • Adds comprehensive tests for snapshot completeness validation

Changes

  • Modified decoder.go to track the last page number read and validate it matches the expected value for snapshots
  • Added TestDecoder_SnapshotCompleteness with four test cases:
    • Complete snapshot validation
    • Incomplete snapshot detection
    • Special handling for lock page boundary
    • Non-snapshot files are not validated

Implementation Details

The decoder now tracks lastPgno during page reads and validates during Close() that:

  • For snapshots (MinTXID=1), all pages from 1 to Commit have been read
  • Special case: when Commit equals the lock page number, we expect the last page to be Commit-1 (since the lock page is never written)
  • Non-snapshot files are not subject to this validation

This ensures snapshot files are complete and haven't been truncated or corrupted.

🤖 Generated with Claude Code

- Implement validation to ensure snapshots contain all expected pages
- Track lastPgno during page decoding
- Add comprehensive tests for snapshot completeness validation
- Handle special case where commit equals lock page number
- Resolves TODO: Ensure last read page is equal to the commit for snapshot LTX files
@corylanou

Copy link
Copy Markdown
Collaborator Author

Reviewed this while clearing the older PRs. It's still correct against current main and the lock-page handling is right, but there's one hole worth closing before it lands, plus a note on the base.

The completeness check is skipped entirely for a snapshot with no pages at all.

if dec.header.IsSnapshot() && dec.lastPgno != 0 {

dec.lastPgno stays 0 when no page is ever decoded, so a snapshot claiming Commit=3 and carrying zero pages short-circuits past the check and is accepted. That's the most extreme incompleteness there is, and it's the one case that slips through. Verified against this branch:

EmptySnapshotClaimingPages: snapshot with Commit=3 and zero pages was accepted
TruncatedSnapshot:          caught — "expected last page 3, got 2"

So truncation is caught, total absence is not.

The lastPgno != 0 guard looks like it's there to let commit-zero deletion files through, but it isn't needed for that. With Commit=0 the computed expectedLastPage is 0 and lastPgno is 0, so they match and the file passes on its own. Dropping the guard is enough:

if dec.header.IsSnapshot() {

I applied exactly that on the branch and re-ran:

EmptySnapshotClaimingPages   now caught — "expected last page 3, got 0"
CommitZeroDeletionStillValid still accepted
NormalSnapshotStillValid     still accepted
TestDecoder (existing suite)  pass

One thing to consider beyond the fix. Checking only the last page number means a file with a gap in the middle — pages 1 and 3 with Commit=3 — still validates, because the last page is 3 as expected. The encoder enforces contiguity so it won't produce one, but this check exists to defend against files the encoder didn't produce. dec.pageN is already tracked, so comparing the page count against the expected count catches gaps and truncation together: Commit pages normally, or Commit-1 when Commit >= LockPgno. Worth doing here or worth a follow-up; either way the gap case is currently unguarded.

Base is old. This branch sits on d017048 and predates #89, #90, #91 and #92. GitHub still reports it mergeable and it tests clean, but it's worth rebasing before merge so CI runs against what's actually on main#53 sat open long enough to go quietly stale, which is what prompted me to check this one.

Also worth noting this implements the // TODO: Ensure last read page is equal to the commit for snapshot LTX files that's still sitting in decoder.go on main today, so the TODO should come out with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants