fix(vfs): tolerate L1 files that straddle the poll watermark - #1464
Open
darkgnotic wants to merge 2 commits into
Open
fix(vfs): tolerate L1 files that straddle the poll watermark#1464darkgnotic wants to merge 2 commits into
darkgnotic wants to merge 2 commits into
Conversation
Contributor
Author
|
@corylanou, I am somewhat uneasy about changing the semantics of the |
6 tasks
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.
Description
The VFS live-tailer's incremental poll (
pollLevel) and everyReplicaClientbackend's
LTXFiles(seek)implementation assumed a resumed watermark alwaysfalls exactly on a compaction-level file boundary (
file.MinTXID == watermark+1).That assumption breaks whenever a file's range straddles the watermark —
MinTXID <= watermark < MaxTXID— which happens:maxTXID1is seeded frompos.TXID(an L0/snapshot position) whenL1 is still empty. That seed is correct and necessary (L1 files get pruned by
retention, so L1 doesn't generally start at TXID 1) — but it need not land on
a future L1 file's boundary.
In both cases, every backend's seek filter (
MinTXID < seek-> skip) makes thestraddling file invisible, and
pollLevel's exact-match contiguity check(
MinTXID == watermark+1) then rejects the next file as non-contiguous. At L1and above this returns a hard error; nothing retries or recovers, so a follower
wedges permanently at the stale watermark — polling forever, logging
"non-contiguous ltx file", and serving stale reads with no indication to
callers.
This PR makes the seek + contiguity logic straddle-tolerant:
pollLevel(vfs.go) now classifies each candidate file against the currentwatermark instead of requiring an exact match:
MaxTXID <= watermark) -> skip (defends against within-leveloverlap; not produced by litestream today)).
MinTXID <= watermark+1 <= MaxTXID) -> apply and advance the watermark toMaxTXID. Re-applying thealready-covered portion of the page index is an idempotent overwrite, so
this is safe.
MinTXID > watermark+1) -> unchanged: defer at L0, error above.LTXFilesseek filter changes fromMinTXID < seektoMaxTXID < seek, so a straddling file is returned instead of silentlydropped:
s3,file,sftp,webdav,nats,oss.gsandabsadditionally usedseekas a list-key prefix(
prefix += seek.String()), which excludes a straddling file at the listinglevel regardless of the post-filter. Both now list the full level directory
and filter by
MaxTXID < seekin the iterator, matchings3/oss. Thistrades a narrower list for correctness — same list volume
s3/ossalreadyhave.
mockneeds no change; it delegates entirely to a caller-supplied func.The LTXFiles seek-semantics change is a no-op for every existing caller except pollLevel: seek==0 callers are unaffected by definition, L0 callers only ever see single-TXID files (MinTXID==MaxTXID), and compaction always seeks on a level-aligned boundary. Only pollLevel passes a seek that can fall inside a file's range.
Motivation and Context
Found via a production incident: a read-only VFS follower tailing S3 wedged
permanently on
poll L1: non-contiguous ltx file: level=1, current=...6, next=...c-...1deven though the backup data in object storage was fully intact and contiguous.
Root-caused to the seed-vs-boundary mismatch above (case 1) — the L1 file that
would have bridged the gap (
1-b, covering the missing range) was present instorage the whole time but permanently invisible to the seek.
Fixes Issue #1460
How Has This Been Tested?
TestVFSFile_StraddlingL1FileAfterSeed, which reproduces the exactproduction sequence (snapshot seeds
maxTXID1=6, first L1 compaction emits astraddling
1-b, next L1 file isc-1d) — fails on the old code(
maxTXID1stuck at 6) and passes with this fix.TestVFSFile_MergedWiderL1FileAfterConsume, a defensive test thatpollLevel doesn't wedge on within-level overlap; not a state litestream produces today.
mockReplicaClient.LTXFilesin tests to mirror the fixed backendseek semantics (
MaxTXID >= seek).-tags vfssuite passes with no regressions:go test -tags vfs ./....go build -tags vfs ./...,go vetclean on all backend packages.s3,gs,oss,nats,webdav,file) pass;abs/sftp/mockhave no existing test files to run.gofmt -lclean on all touched files.Note: this also fixes a latent, unrelated compile break in the
-tags vfstestsuite —
ReplicaClientgainedSetLoggerin a prior commit but the VFS testdoubles (
mockReplicaClient,countingReplicaClient,writeTestReplicaClient)were never updated, so
go test -tags vfs ./...hasn't built since. Addedno-op
SetLoggerimplementations to unblock running (and adding to) thesetests; worth wiring
-tags vfsinto CI separately so this doesn't recur.Types of changes
Checklist
go fmt,go vet)go test ./...)