Skip to content

fix(checkpoint): bound snapshot count preallocation by file size - #302

Open
detail-app[bot] wants to merge 1 commit into
masterfrom
detail/bug-fix/fix-checkpoint-bound-snapshot-count-preallocation-678e17
Open

detail-app[bot] wants to merge 1 commit into
masterfrom
detail/bug-fix/fix-checkpoint-bound-snapshot-count-preallocation-678e17

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 16, 2026

Copy link
Copy Markdown

Detail bug report: View on Detail

Summary

Snapshot.load in the tail checkpoint could abort tail startup with error.OutOfMemory when the persisted checkpoint.snap had a valid magic/version but a corrupted count field. This is the same class of on-disk corruption the function is designed to survive (it already degrades gracefully on bad magic, bad version, short header, short entry read, and bad checksum). Fixes ENG-703.

Bug

Snapshot.load (src/tail/checkpoint/snapshot.zig) reads the checkpoint snapshot at startup so the tail can resume file offsets. It pre-reserved the result list with:

try out.ensureTotalCapacity(allocator, @intCast(header.count));

header.count is a u64 read straight from disk, gated only by the magic/version checks. A corrupted count whose reservation exceeds what the host allocator will grant (e.g. a single-bit flip at bit 56 of the count field makes the reservation ~4 EB) made load return error.OutOfMemory before the loop's short-read guard could degrade gracefully.

The error propagated uncompensated through Lane.recoverLane.init (runtime.zig:200) → main, aborting the entire tail runtime at startup. The only operator "recovery" was to manually delete checkpoint.snap — on the exact corruption class every other guard in load exists to tolerate.

Fix

Cap the preallocation hint by the actual remaining file size so a corrupted header can never request more capacity than the file could legitimately contain:

const file_size = try file.length(self.io);
const entry_space = if (file_size > @sizeOf(SnapshotHeader))
    file_size - @sizeOf(SnapshotHeader)
else
    0;
const max_entries = entry_space / @sizeOf(SnapshotEntry);
const hint = @min(header.count, max_entries);
try out.ensureTotalCapacity(allocator, @intCast(hint));

This uses file.length(self.io), consistent with the existing idiom in wal.zig:90. A legitimate count can never exceed file_size / @sizeOf(SnapshotEntry), so the preallocation optimization is preserved for the common (non-corrupt) case. A corrupt count now hits the existing short-read break and load returns a partial list, letting Lane.recover fall back to WAL replay — the same recovery path already used for every other corruption class.

Testing

  • Added a regression test checkpoint/lane: corrupted snapshot count degrades to wal replay that writes a valid snapshot, flips byte 15 (top byte of the little-endian count field, count=1 → 2⁵⁶+1) while leaving magic/version intact, appends the same value to the WAL, and asserts Lane.init succeeds and recovers the offset via WAL replay.
  • Reverse-verification: temporarily reverting only the fix makes the new test fail with error.OutOfMemory originating at snapshot.zig:65 and propagating through recoverinit, confirming the test genuinely catches the bug.
  • Unit tests, zig build, zig build tail, zig fmt --check, and ziglint all pass under both Debug and ReleaseSafe (task test runs ReleaseSafe on CI). The full suite is 520 pass / 1 skip (the skip is pre-existing and unrelated).
  • End-to-end verification against the real edge-tail binary: with a corrupt checkpoint.snap, the fixed binary starts cleanly and rewrites a clean snapshot after recovery, whereas a buggy build aborts at startup with error: OutOfMemory through the exact mainrunFilesToOutputLane.initrecoverload path the bug report describes.
  • Confirmed the fix resolves both the testing.allocator (DebugAllocator) and the production std.heap.c_allocator (libc malloc) paths — a temporary test driving Lane.init(std.heap.c_allocator, ...) fails without the fix and passes with it.

Automatic Fixes PRs can be configured here.

@macroscopeapp

macroscopeapp Bot commented Sep 16, 2026

Copy link
Copy Markdown

Approvability

Verdict: Approved at 6c06efa

Macroscope's review found this PR approvable — This is a narrowly scoped checkpoint-recovery bug fix that bounds preallocation using the actual snapshot file size, preserving valid snapshots while preventing corrupted counts from causing startup failure. The added regression test verifies WAL recovery for the malformed snapshot case.

You can add or adjust custom eligibility rules. Learn more.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant