fix(checkpoint): bound snapshot count preallocation by file size - #302
Open
detail-app[bot] wants to merge 1 commit into
Open
detail-app[bot] wants to merge 1 commit into
detail-app[bot] wants to merge 1 commit into
Conversation
ApprovabilityVerdict: Approved at 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. |
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
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.
Detail bug report: View on Detail
Summary
Snapshot.loadin the tail checkpoint could abort tail startup witherror.OutOfMemorywhen the persistedcheckpoint.snaphad a valid magic/version but a corruptedcountfield. 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:header.countis au64read straight from disk, gated only by themagic/versionchecks. A corruptedcountwhose reservation exceeds what the host allocator will grant (e.g. a single-bit flip at bit 56 of thecountfield makes the reservation ~4 EB) madeloadreturnerror.OutOfMemorybefore the loop's short-read guard could degrade gracefully.The error propagated uncompensated through
Lane.recover→Lane.init(runtime.zig:200) →main, aborting the entire tail runtime at startup. The only operator "recovery" was to manually deletecheckpoint.snap— on the exact corruption class every other guard inloadexists 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:
This uses
file.length(self.io), consistent with the existing idiom inwal.zig:90. A legitimatecountcan never exceedfile_size / @sizeOf(SnapshotEntry), so the preallocation optimization is preserved for the common (non-corrupt) case. A corruptcountnow hits the existing short-readbreakandloadreturns a partial list, lettingLane.recoverfall back to WAL replay — the same recovery path already used for every other corruption class.Testing
checkpoint/lane: corrupted snapshot count degrades to wal replaythat writes a valid snapshot, flips byte 15 (top byte of the little-endiancountfield, count=1 → 2⁵⁶+1) while leaving magic/version intact, appends the same value to the WAL, and assertsLane.initsucceeds and recovers the offset via WAL replay.error.OutOfMemoryoriginating atsnapshot.zig:65and propagating throughrecover→init, confirming the test genuinely catches the bug.zig build,zig build tail,zig fmt --check, andziglintall pass under both Debug and ReleaseSafe (task testruns ReleaseSafe on CI). The full suite is 520 pass / 1 skip (the skip is pre-existing and unrelated).edge-tailbinary: with a corruptcheckpoint.snap, the fixed binary starts cleanly and rewrites a clean snapshot after recovery, whereas a buggy build aborts at startup witherror: OutOfMemorythrough the exactmain→runFilesToOutput→Lane.init→recover→loadpath the bug report describes.testing.allocator(DebugAllocator) and the productionstd.heap.c_allocator(libc malloc) paths — a temporary test drivingLane.init(std.heap.c_allocator, ...)fails without the fix and passes with it.Automatic Fixes PRs can be configured here.