[matthewmcneely]

Description

Follow-up to my review on dgraph-io/badger#2331. This targets fix/empty-mem-file-open in your fork rather than upstream, so you can fold it in however you like before the next push. The .mem and .vlog stat-and-skip work is yours and unchanged; this fixes one thing the vlog half missed, plus a few small items from the review.

Why

populateFilesMap sets vlog.maxFid from the directory listing, before the 0-byte skip runs. When the crash artifact holds the highest fid, and it normally does, the skip drops it from filesMap while vlog.maxFid keeps naming it. Two things then read a fid that is no longer there:

1. The filesMap[vlog.maxFid] lookup guarding the tail truncation finds nothing, so iterate and Truncate never run. The newest surviving .vlog keeps whatever partial entry was left on it. The crash that leaves a 0-byte .vlog behind is exactly the crash that can tear the tail of the file before it, so recovery gets skipped precisely when it is needed.
2. The fid != vlog.maxFid guard stops protecting the newest real file, so a header-only newest .vlog gets deleted. Benign on its own, since a header-only file holds no entries, but it is the same root cause.

Reproduced on your branch by appending a torn record to the last .vlog and reopening:

0-byte artifact	clean	torn	after Open
absent	52570	52607	52570
present	52570	52607	52607

Not a regression, since Open failed outright before your PR. It is a hole in the recovery path the PR is meant to repair.

Changes

value.go

• Split the open loop into a stat pass and an open pass, and track newestFid, the highest fid that actually survived. Both the truncation and the header-only guard use it.
• vlog.maxFid is deliberately left alone. createVlogFile takes maxFid+1 with O_EXCL, so lowering it to the newest surviving fid would collide with the artifact still sitting on disk and fail with EEXIST. That one is worth a comment because it looks like the obvious fix.
• Comment on the read-only early return explaining why a dangling vlog.maxFid is safe there: read-only never reaches createVlogFile, so the invariant stays broken for the life of the handle, but getFileRLocked keys on vp.Fid and skips its maxFid bound check when ReadOnly is set.

value.go and memtable.go

• Infof when a 0-byte file is skipped, matching the existing Deleting empty file: line just below it. Both artifacts now stay on disk permanently, so the log line is the only way an operator learns the directory carries crash residue.
• errFile for the stat error in vlog.open, so it wraps the same way openMemTables already does.

db_test.go

• New TestEmptyVlogFileDoesNotSkipTailRecovery, run with and without the artifact present. It fails on the current branch with expected: 52570, actual: 52607 and passes with the value.go change.
• TestOpenWithEmptyVlogFile now sets ValueThreshold to 32. At the 1MB default every value was inlined in the LSM tree, so the test never touched the value log read path at all. Step 6 now reads all 100 values back through the vlog in read-only mode, which is the mode where vlog.maxFid stays dangling.

Testing

• go test ./... passes.
• TestEmptyVlogFileDoesNotSkipTailRecovery fails without the value.go change and passes with it.
• gofmt clean, go vet clean apart from the pre-existing pb.Match lock-copy warnings in publisher.go.

🤖 Generated with Claude Code

[adityeah8969]

@matthewmcneely thanks for the PR, I have merged this into my fork, and I have asked for final review in my original PR