State WAL replacement#3701
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3701 +/- ##
==========================================
- Coverage 59.29% 58.40% -0.89%
==========================================
Files 2272 2191 -81
Lines 188193 179278 -8915
==========================================
- Hits 111580 104714 -6866
+ Misses 66560 65209 -1351
+ Partials 10053 9355 -698
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview API & behavior: On-disk design: CRC32-framed records in mutable Runtime: Serializer + writer goroutines; rotation, pruning (whole sealed files), and iterator creation serialized on the writer. Iterators take a point-in-time sealed-file snapshot, prefetch blocks, and pin a read lease so prune cannot delete files still being read. Observability: OpenTelemetry counters for blocks/bytes written, files sealed, and files pruned. Broad unit coverage: ordering contract, rotation, prune vs leases, rollback/crash scenarios, and concurrent iteration during rotation. Reviewed by Cursor Bugbot for commit 101e586. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
A new, well-tested, not-yet-integrated state WAL package with careful crash-recovery/durability handling; no blocking correctness issues found, but the asynchronous ownership contract for caller-supplied changesets should be documented (or copies taken), and a couple of design/efficiency notes are worth tracking before integration.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor produced no second-opinion review (cursor-review.md is empty); only Codex's pass was available to merge with.
- Iterator() unconditionally seals and rotates the mutable file whenever it holds a complete block (sealForIterator -> rotate). If the integrated read path creates iterators frequently during normal operation, this will fragment the WAL into many tiny single-block sealed files. Worth confirming iterators are only created rarely (e.g. at startup replay) or reconsidering the forced-seal-on-iterate design before wiring this in.
- metrics.go initializes OTel counters at package-init time via must(...), which panics the whole process if meter/counter creation ever fails. Acceptable for a metrics setup, but note it is a hard init-time dependency.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| if err := w.enforceWriteOrdering(blockNumber); err != nil { | ||
| return fmt.Errorf("write rejected: %w", err) | ||
| } | ||
| if err := w.sendToSerializer(dataToBeSerialized{entry: NewEntry(blockNumber, cs)}); err != nil { |
There was a problem hiding this comment.
[suggestion] Write() stores the caller-owned cs []*proto.NamedChangeSet slice/pointers directly (via NewEntry) and returns before serialization happens — Serialize()/Marshal() runs later on the serializer goroutine. If the caller mutates or reuses the slice or the underlying NamedChangeSet/KVPair buffers after Write() returns, the WAL can persist data different from what was passed at the call boundary. The StateWAL.Write interface doc says the write is only "scheduled" but never states this ownership contract. Please either document that the caller must not mutate cs (or its contents) after Write() returns, or deep-copy/serialize synchronously before enqueueing. (Matches Codex's finding.)
There was a problem hiding this comment.
updated godoc to reflect that modification is unsafe after method call
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 54dd10c. Configure here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a standalone, un-integrated state WAL utility (files, framing, recovery/rollback, iterator, metrics) with strong documentation and broad unit tests. No blocking bugs in the intended single-writer path, but two edge-case robustness concerns are worth addressing before integration.
Findings: 0 blocking | 5 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) and REVIEW_GUIDELINES.md were both empty; only Codex's pass produced output, and its two findings are incorporated below.
- Overall the package is well-structured and unusually well-commented; durability ordering (fsync-before-rename, directory syncs, contiguous-prefix/suffix guarantees on prune/rollback) and the torn-tail recovery model are carefully reasoned and covered by tests.
- metrics.go: the package-level
must(...)helper panics at init if OpenTelemetry counter construction ever fails, taking down the process at import time. This is a common and low-risk pattern for otel counters, but worth a brief acknowledgement. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| if err := w.enforceWriteOrdering(blockNumber); err != nil { | ||
| return fmt.Errorf("write rejected: %w", err) | ||
| } | ||
| if err := w.sendToSerializer(dataToBeSerialized{entry: NewEntry(blockNumber, cs)}); err != nil { |
There was a problem hiding this comment.
[suggestion] The write-ordering state is validated under w.mu in enforceWriteOrdering, but the lock is released before the record is enqueued via sendToSerializer (and SignalEndOfBlock does the same at line 255). Serialization order is determined by the order of the channel sends, not by the ordering check. With concurrent callers, goroutine A can pass the check for a changeset on block N, then goroutine B can take the lock, mark the block ended, and enqueue the end-of-block marker before A enqueues its changeset — landing EOB(N) ahead of changeset(N) on disk. On replay the iterator coalesces this into a duplicated/empty-then-populated block N (mis-coalescing). Since the mu mutex exists solely to guard these single-sequence ordering fields (it implies concurrency-safety intent), either hold the lock across the enqueue or explicitly document that callers must serialize Write/SignalEndOfBlock. Not a bug for a single-writer caller, which is how the tests exercise it. (Also raised by Codex.)
| payload := data[payloadStart : payloadStart+payloadLen] | ||
| recordEnd := payloadStart + payloadLen + 4 | ||
| gotCRC := binary.BigEndian.Uint32(data[payloadStart+payloadLen : recordEnd]) | ||
| if gotCRC != crc32.ChecksumIEEE(payload) { |
There was a problem hiding this comment.
[suggestion] A CRC32 mismatch always breaks and treats everything from that point on as a torn/incomplete tail that is silently discarded. That is the right behavior for the mutable file after a crash, but parseWalFileData applies it uniformly to sealed files too (via the iterator's readWalFileFromHandle). A sealed file's name encodes first-last and is trusted by scanSealedFiles/GetStoredRange, so mid-stream bit-rot in a sealed file would leave GetStoredRange reporting blocks that iteration silently skips (contents.lastCompleteBlock drops below the name's lastBlock), diverging state without surfacing corruption. Consider surfacing a checksum mismatch in a sealed file as a hard error rather than a torn tail. Rare (requires post-fsync disk corruption), hence non-blocking. (Also raised by Codex.)
| func (w *stateWALImpl) rotate() error { | ||
| index := w.mutableFile.index | ||
| first := w.mutableFile.firstBlock | ||
| last := w.mutableFile.lastCompleteBlock | ||
| sealedName, err := w.mutableFile.seal() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to seal WAL file during rotation: %w", err) | ||
| } | ||
| w.sealedFiles.PushBack(&sealedFileInfo{index: index, name: sealedName, firstBlock: first, lastBlock: last}) | ||
| walFilesSealed.Add(w.ctx, 1) | ||
|
|
||
| mutable, err := newWalFile(w.config.Path, w.nextIndex) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to open new mutable WAL file during rotation: %w", err) | ||
| } | ||
| w.mutableFile = mutable | ||
| w.nextIndex++ | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🟡 🟡 In rotate() (state_wal_impl.go:498), sealedFiles.PushBack happens before newWalFile succeeds, and the iteratorStartRequest branch in the writer loop (line 465) does not call w.fail(err) on rotate failure — unlike appendRecord/pruneRequest. If a rotate triggered by sealForIterator fails at newWalFile (ENOSPC/EPERM/EMFILE), w.mutableFile still points at the now-sealed file, the WAL keeps limping, and the next Iterator() call calls rotate() → seal() again. seal() idempotently returns ("", nil) for the already-sealed file, so PushBack registers {index: dup, name: "", ...}. Iterators built afterward compute filepath.Join(dir, "") == dir, open the directory, and io.ReadAll on it returns EISDIR; a subsequent prune of that bogus entry runs os.Remove(dir) which fails and triggers fail(). One-line fix: call w.fail(err) on rotate failure in startIterator/sealForIterator so the WAL shuts down cleanly rather than staging bogus entries.
Extended reasoning...
What the bug is. rotate() at state_wal_impl.go:498-516 is a two-step operation: (1) seal the current mutable file and PushBack a sealedFileInfo into w.sealedFiles, (2) open a fresh mutable file via newWalFile. On step (2) failure (out of disk, permission error, EMFILE, etc.), rotate returns an error but has already committed step (1). w.mutableFile is never updated, so it keeps pointing at the just-sealed, closed walFile whose sealed=true and hasCompleteBlock=true.
Why it survives. The writer loop's iteratorStartRequest branch (state_wal_impl.go:465-466) forwards the error back to the caller via the reply channel but does not call w.fail(err) — contrast with the dataToBeWritten and pruneRequest branches, which both do. The WAL therefore keeps running with an internally-inconsistent mutableFile. Any subsequent Write would hit writeRecord's f.sealed guard (state_wal_file.go:165-167) and eventually call fail(), but between the failed rotate and the next Write, additional Iterator() calls can pile in.
How the bogus entry appears. On the next Iterator() call, sealForIterator (state_wal_impl.go:589) sees hasCompleteBlock=true, does not short-circuit, and — since size==completeSize (the common case where the caller just did SignalEndOfBlock then Iterator) — readIncompleteTail returns (nil, nil). Then rotate() is re-entered: walFile.seal() at state_wal_file.go:255-258 returns ("", nil) idempotently because f.sealed is already true, and rotate() at line 506 calls sealedFiles.PushBack(&sealedFileInfo{index: X, name: "", firstBlock: Y, lastBlock: Z}) — an entry with a duplicate index and an empty name. newWalFile may now succeed, so the WAL "recovers" while carrying this in-memory phantom.
Downstream damage. Two paths:
- Iterator —
startIteratorsnapshots the sealed-files list (state_wal_impl.go:570-577), including the bogus entry. InwalIterator.loadNextFile,path = filepath.Join(dir, "")evaluates todiritself.os.Open(dir)succeeds on Linux (opens the directory),readWalFileFromHandlecallsio.ReadAllon the directory fd, which returns EISDIR — surfacing as a confusing iterator error. - Prune —
pruneSealedFiles(state_wal_impl.go:537-553) walks entries from the front. The bogus entry and the correct one share the samelastBlock; if the correct one is drained first, the bogus one becomes the front andos.Remove(filepath.Join(dir, ""))runs on the directory, fails with!os.IsNotExist(err), and triggersw.fail(err)— shutting down the WAL from a prune.
Step-by-step proof.
- Suppose blocks 1,2 have been written and ended;
mutableFilehasindex=0,firstBlock=1,lastCompleteBlock=2,size==completeSize. - Caller invokes
Iterator(0). Writer runssealForIterator→rotate(). rotatecapturesindex=0, first=1, last=2.seal()succeeds: renames0.swal.u→0-1-2.swal, setsf.sealed=true. Line 506 pushes{0, "0-1-2.swal", 1, 2}intosealedFiles.newWalFile(dir, 1)fails (e.g., ENOSPC).rotatereturns error.startIteratorreturns{err: ...}. Writer loop moves on withoutfail().w.mutableFilestill references the sealed walFile object (sealed=true,hasCompleteBlock=true). No Write has occurred.- Caller invokes
Iterator(0)a second time.sealForIteratorproceeds.readIncompleteTailreturns(nil,nil)becausesize==completeSize. rotatere-enters. Captures the sameindex=0, first=1, last=2.seal()returns("", nil)idempotently at state_wal_file.go:256-258. Line 506 pushes{0, "", 1, 2}— bogus.newWalFile(dir, 1)succeeds this time (transient error cleared).w.mutableFileupdates.startIteratorreturns success.- The iterator's file snapshot contains both
{0, "0-1-2.swal", ...}and{0, "", ...}. WhenloadNextFilereaches the bogus one,os.Open(dir)succeeds andio.ReadAllon the directory fd returns EISDIR — the iterator'sNext()surfaces this error.
Scope / impact. Requires an initial newWalFile failure (rare — disk-related). No on-disk corruption; the bogus entry lives only in memory and does not survive restart (scanSealedFiles rebuilds from directory entries). Failure mode is loud (iterator errors, prune-triggered fail()) rather than silent data loss. The next Write on the sealed mutable file also self-heals via writeRecord's f.sealed guard, so the buggy window is between the failed rotate and the next write.
Fix. The straightforward one-liner is to call w.fail(err) when sealForIterator/rotate returns an error in the iteratorStartRequest branch, matching the appendRecord/pruneRequest branches. Alternatively, rotate() can compute the sealed name deterministically from (index, firstBlock, lastCompleteBlock) instead of trusting seal()'s return, and/or seal() can remember and return the sealed name on idempotent re-entry.
Severity: nit. Real bug, but the triggering conditions are narrow (newWalFile failure plus at least one Iterator() call before the next Write), the failure mode is loud rather than silent, no data is at risk, nothing survives restart, and the module isn't wired into SS/SC yet. Worth a robustness cleanup, not a merge blocker.
| if err := w.enforceWriteOrdering(blockNumber); err != nil { | ||
| return fmt.Errorf("write rejected: %w", err) | ||
| } | ||
| if err := w.sendToSerializer(dataToBeSerialized{entry: NewEntry(blockNumber, cs)}); err != nil { |
There was a problem hiding this comment.
Write keeps caller-owned changesets and serializes them later. Because Write returns after enqueueing, callers can mutate or reuse cs before serializerLoop runs, causing the WAL to persist changed data or race with the caller
2 participants
Describe your changes and provide context
This WAL is intended to replace all existing WALs in both SS and SC. Instead of having multiple WALs for multiple different services, we can maintain just a single WAL which is used universally across all parts of the storage stack.
This PR does not integrate the changes, it just creates a WAL utility. Integration work will happen in future PRs. We've got a pre-existing WAL, but that implementation has some serious issues. Namely, the WAL library we currently use has very inefficient garbage collection (it rewrites garbage collected files, as opposed to dropping files as a whole when data in file is all stale).
Testing performed to validate your change
unit tests