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.85% 59.06% -0.79%
==========================================
Files 2278 2203 -75
Lines 189140 180599 -8541
==========================================
- Hits 113202 106665 -6537
+ Misses 65861 64541 -1320
+ Partials 10077 9393 -684
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 The on-disk format uses sealed/mutable files, CRC-framed records, a directory Adds Reviewed by Cursor Bugbot for commit ed97f93. 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}
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
| 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
There was a problem hiding this comment.
This pattern is intentional for performance, but not well documented.
I updated all godocs to say that it is unsafe to modify slices passed into methods or slices received from methods.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
| index: index, | ||
| serialize: func() ([]byte, error) { return s.serialize(data) }, | ||
| } | ||
| if err := s.submit(req); err != nil { |
There was a problem hiding this comment.
+1. two implementation notes if we add the gate here: (1) the gate needs to be seeded from inner.Bounds() at construction (same as newWAL does for lastAppendIndex), otherwise the first append after a reopen goes unchecked; (2) TestGenericWALAppendOrdering currently asserts the async-brick behavior as expected, so its assertions need to flip to a synchronous rejection.
| // from an interrupted rollback and seals any unsealed file left behind by a prior session. After it returns, | ||
| // every record lives in a sealed file whose name matches its content, with no orphans remaining. Shared by | ||
| // the WAL constructor and the offline GetRange/PruneAfter utilities, so all three run the same sanity pass. | ||
| func recoverDirectory(path string) error { |
There was a problem hiding this comment.
a second wal instance or an offline utility opens the same path while one is live may corrupts the log
can we add a FileLock to the dir?
There was a problem hiding this comment.
added file locks
| resp := w.startIterator(m.startIndex) | ||
| m.reply <- resp | ||
| if resp.err != nil { | ||
| w.fail(resp.err) |
There was a problem hiding this comment.
Iterator construction failure bricks the entire write path, and hard-link support becomes a hard filesystem requirement
should we distinguish the different failure cases? like for MkdirAll/os.Link error, just return error instead of bricking the entire write path
There was a problem hiding this comment.
Iterator failures are no longer fatal for failure modes that do not lead to corrupted state.
| } | ||
|
|
||
| // Write accumulates a set of changes for the given block number in memory. | ||
| func (w *stateWALImpl) Write(blockNumber uint64, cs []*proto.NamedChangeSet) error { |
There was a problem hiding this comment.
the nil changeset check is performed asynchronously in state_wal_serialization.go. As a result, Write() and potentially SignalEndOfBlock() can return successfully even when the input contains a nil changeset. The serialization goroutine discovers the error later and bricks the WAL, so the error surfaces at an unrelated subsequent operation.
can we validate nil entries synchronously in Write() ? so the invalid input is rejected at the call site without bricking the WAL.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Reviewed by Cursor Bugbot for commit ed97f93. Configure here.
| if err != nil && !errors.Is(err, ErrIteratorRange) { | ||
| s.fail(fmt.Errorf("failed to create iterator: %w", err)) | ||
| return | ||
| } |
There was a problem hiding this comment.
Wrappers brick on non-fatal iterators
High Severity
walImpl now treats snapshot I/O failures during Iterator as non-fatal so the WAL stays usable, but serializingWAL and stateWALImpl still brick on any non-ErrIteratorRange error. A transient failure creating the hard-link snapshot therefore permanently disables NewGenericWAL / StateWAL and tears down a healthy inner engine.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ed97f93. Configure here.
There was a problem hiding this comment.
A well-documented, thoroughly tested new WAL utility package (seiwal) and a statewal wrapper; the code is not yet integrated and is carefully engineered for crash durability and recovery. The only code finding is a low-severity uint64 overflow in the contiguous-index validation.
Findings: 0 blocking | 5 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's review (cursor-review.md) is empty — that second-opinion pass produced no output.
- REVIEW_GUIDELINES.md (from the base branch) is empty, so no repo-specific review standards were applied.
- seiwal_iterator.go:290 acknowledges (in a comment) that locating the start index is a linear scan over a fully-loaded file; fine for now, but worth revisiting if startup replay from a deep offset becomes a hotspot, as the comment notes.
- Overall this is high-quality, self-contained utility code with strong test coverage; the main integration/correctness risk will land in the follow-up PRs that wire it into SS/SC.
- 1 suggestion(s)/nit(s) flagged inline on specific lines.
| "append rejected: index %d is not greater than last appended index %d", | ||
| index, w.lastAppendIndex) | ||
| } | ||
| } else if index != w.lastAppendIndex+1 { |
There was a problem hiding this comment.
[nit] uint64 overflow edge case in the contiguous check: when lastAppendIndex == math.MaxUint64, lastAppendIndex+1 wraps to 0, so an Append(0, ...) is incorrectly accepted as "contiguous", producing a record with an inverted index relative to the file's existing max. The writer-side backstop at appendRecord (line 489, m.index != w.lastWrittenIndex+1) has the identical wrap. Reaching it only requires the baseline (first) append to use a near-max index rather than 2^64 total records. Realistically block numbers won't approach 2^64, so severity is low, but a cheap explicit guard (reject once the max index is reached) in both the caller-side and writer-side checks would harden it. (Also raised by Codex.)
There was a problem hiding this comment.
Beyond the inline findings, I also checked two other candidate issues and ruled them out: walImpl.Close()'s per-call closeErr local means a second Close() call after a failed first one would return nil — but Close() is only ever expected to be called once, so this isn't reachable in practice. And walImpl.Append advancing lastAppendIndex/hasAppended before sendToWriter succeeds is fine because sendToWriter only fails once the WAL is already closed or bricked, at which point no retry could succeed anyway.
Extended reasoning...
This run's two inline findings (the serializing/StateWAL layers still bricking on non-fatal iterator snapshot errors that walImpl itself now tolerates, and a narrow EOF-vs-shutdown race in the iterator) are the substantive output of this pass. I additionally reviewed the two candidate issues surfaced by finder agents and confirmed they are not real bugs, as noted in the message.
| case serIterator: | ||
| it, err := s.inner.Iterator(m.startIndex, m.endIndex) | ||
| m.reply <- serIteratorResult{it: it, err: err} | ||
| // A rejected range leaves the inner WAL healthy, so mirror that here; only a genuine inner | ||
| // failure bricks the serializing layer. | ||
| if err != nil && !errors.Is(err, ErrIteratorRange) { | ||
| s.fail(fmt.Errorf("failed to create iterator: %w", err)) | ||
| return | ||
| } |
There was a problem hiding this comment.
🔴 serializingWAL and StateWAL still brick on a non-fatal iterator snapshot I/O error (e.g. EMLINK/ENOSPC while linking the iterator// snapshot dir), even though walImpl.startIterator was explicitly fixed to treat this as recoverable and keep the inner WAL alive (per TestIteratorSnapshotIOFailureDoesNotBrick). serializingWAL.serializerLoop's serIterator case (seiwal_serializing.go:337-345) treats any non-ErrIteratorRange error as fatal and calls s.fail(), which then cascades into stateWALImpl.Iterator re-bricking the StateWAL layer too — undoing the fix for the primary NewGenericWAL/statewal.New public API.
Extended reasoning...
walImpl.startIterator (seiwal_impl.go:536-595) was deliberately changed this same PR so a snapshot-construction failure from linkSnapshot (os.MkdirAll/os.Link into iterator/<serial>/, which can fail with EMLINK once a source inode nears the hard-link ceiling, or ENOSPC/inode exhaustion, or EACCES) is returned to the writerLoop as iteratorStartResponse{err: err} with fatal left unset. writerLoop only calls w.fail() when resp.fatal is true, so this non-corrupting failure leaves the inner byte-engine WAL fully alive — exactly what TestIteratorSnapshotIOFailureDoesNotBrick asserts, and what the PR-thread comment on seiwal_impl.go:462 ("Iterator failures are no longer fatal for failure modes that do not lead to corrupted state") documents as an intentional fix in response to reviewer blindchaser's feedback.
That fix stops at the byte-engine layer. serializingWAL.serializerLoop's serIterator case (seiwal_serializing.go:337-345) calls s.inner.Iterator(...) — the now-fixed walImpl.Iterator — and then does if err != nil && !errors.Is(err, ErrIteratorRange) { s.fail(...); return }. A linkSnapshot I/O error is wrapped as fmt.Errorf("failed to create iterator: %w", resp.err) by walImpl.Iterator, which is neither nil nor ErrIteratorRange, so this branch unconditionally fires, cancelling the serializing WAL's context and closing the still-healthy inner WAL. The in-code comment directly above this branch — "only a genuine inner failure bricks the serializing layer" — is precisely the assumption that walImpl's own fix invalidated: a linkSnapshot failure is no longer a genuine failure at the inner layer, but the wrapper still treats it as one.
The damage does not stop there. stateWALImpl.Iterator (state_wal_impl.go) calls w.wal.Iterator(...) and does if errors.Is(err, seiwal.ErrIteratorRange) { return err } else { return w.fail(err) }. By the time this runs, the error surfacing from the serializing layer is already a "serializing WAL failed"/"failed to create iterator" wrapped error, not ErrIteratorRange, so stateWALImpl.fail() fires too, bricking the StateWAL layer on top of the already-bricked serializing layer.
Since NewGenericWAL (and statewal.New, built on it) is — per the PR description — the intended universal replacement for the SS/SC WALs and the API essentially every real consumer will use, walImpl is really just the low-level byte engine underneath. Any caller that opens many concurrent iterators approaching the per-inode hard-link ceiling (this WAL adds one link per file per open iterator), or that hits a transient ENOSPC/inode-exhaustion condition while creating the tiny iterator/<serial>/ snapshot directory, will permanently brick the whole WAL/StateWAL through the primary public API for a condition the inner engine and its own test explicitly demonstrate should be transient and recoverable.
Step-by-step proof: (1) A serializingWAL wraps a healthy walImpl. (2) A caller requests an iterator; walImpl.startIterator succeeds through range validation but fails in linkSnapshot (e.g. os.MkdirAll returns ENOSPC). (3) startIterator returns iteratorStartResponse{err: <linkSnapshot error>} with fatal false (unset); writerLoop sees !resp.fatal and does not call w.fail() — the inner walImpl stays alive and would happily serve appends or a retried iterator request. (4) walImpl.Iterator wraps this as fmt.Errorf("failed to create iterator: %w", resp.err) and returns it to the caller — in this case, serializingWAL.serializerLoop. (5) In the serIterator case, this error is non-nil and !errors.Is(err, ErrIteratorRange), so s.fail(...) runs, which cancels s.ctx, causing s.inner.Close() to run against the still-healthy walImpl — closing a WAL that had no need to be closed. (6) Every subsequent call on the serializingWAL (Append, Flush, Bounds, Iterator) now fails via s.senderErr()/asyncError(). (7) stateWALImpl.Iterator, which called through to this now-bricked serializing WAL, gets a non-ErrIteratorRange error and calls w.fail(err), permanently setting fatalErr on the stateWALImpl too. From this point every Write/SignalEndOfBlock/Flush/Prune/Iterator call on the StateWAL fails forever, even though the underlying data and byte-engine WAL were never actually corrupted or unhealthy.
The root cause is that the fatal/non-fatal distinction created by the fix lives only in the private iteratorStartResponse.fatal field inside walImpl — it is never surfaced through the public WAL interface. walImpl.Iterator exposes only ErrIteratorRange as a distinguishable sentinel, so any wrapper built on the generic WAL[T] interface (which is exactly what serializingWAL and stateWALImpl are) has no way to tell a recoverable snapshot I/O failure apart from a genuinely fatal one, and defaults to treating every non-range error as fatal. Fixing this requires either a second exported sentinel error (e.g. ErrIteratorSnapshotFailed) that walImpl.Iterator wraps around a non-fatal snapshot failure, or restructuring the WAL interface/response so the fatal/non-fatal split is visible above the byte-engine layer — either way, serializingWAL.serializerLoop's serIterator case (and stateWALImpl.Iterator, which inherits the collapsed error) needs to check for that sentinel instead of collapsing on "anything that isn't ErrIteratorRange."
This is worth fixing now rather than later: the PR explicitly states this WAL is not yet integrated into production, so nothing breaks today, but the whole point of NewGenericWAL/statewal.New is to be the wrapper every real consumer uses — leaving this inconsistency in means the recoverable-iterator-failure guarantee the author just added is silently absent for that primary API surface, and it directly negates a fix made in this same PR in response to reviewer feedback (blindchaser's inline comment on seiwal_impl.go:462).
| func (it *walIterator) Next() (bool, error) { | ||
| if it.done { | ||
| return false, nil | ||
| } | ||
| // Drain an already-available result first (non-blocking), so a clean EOF — the reader closing results | ||
| // with records still buffered — is never lost to a concurrent WAL shutdown in the select below. | ||
| select { | ||
| case result, ok := <-it.results: | ||
| return it.deliver(result, ok) | ||
| default: | ||
| } | ||
| // Otherwise wait for the next result, but don't block forever if the WAL is torn down. The reader | ||
| // goroutine watches the same context (see send) and stops producing when it fires, so the results | ||
| // channel would never advance again; surface the shutdown as an error rather than hang. | ||
| select { | ||
| case result, ok := <-it.results: | ||
| return it.deliver(result, ok) | ||
| case <-it.wal.ctx.Done(): | ||
| it.done = true | ||
| if err := it.wal.asyncError(); err != nil { | ||
| return false, fmt.Errorf("WAL shut down during iteration: %w", err) | ||
| } | ||
| return false, fmt.Errorf("WAL shut down during iteration: %w", it.wal.ctx.Err()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 walIterator.Next() (seiwal_iterator.go:118-142) races the reader's clean close(it.results) on legitimate EOF against an unrelated concurrent fatal WAL failure (e.g. a Flush/Append/Prune I/O error triggering w.fail(), which cancels w.ctx). If the consumer is already parked in the blocking select at the exact instant both become ready, Go's uniform-random select choice can make Next() return a spurious shutdown error even though the iterator had already delivered every record through maxIndex — the correct result was (false, nil). This is a pre-existing narrow race with no data loss (only the terminal signal is misreported); fixable by doing a final non-blocking receive on it.results inside the ctx.Done() branch before honoring the shutdown error.
Extended reasoning...
What the bug is. walIterator.Next() (sei-db/seiwal/seiwal_iterator.go:118-142) is built around two selects: a non-blocking pre-check on it.results, then a blocking select between <-it.results and <-it.wal.ctx.Done(). The reader goroutine's read() reaches a legitimate EOF via nextRecord() returning ok=false, and closes it.results directly — this path never checks w.ctx (only send(), used for in-range records, watches ctx.Done()). Separately, w.fail(err) — triggered by any concurrent, unrelated fatal I/O failure on the same walImpl (a failed Flush/Append/rotate/prune) — calls w.cancel(err), cancelling the same w.ctx that Next()'s blocking select watches.\n\nThe code path that triggers it. If the reader closes it.results (a legitimate, complete EOF) at the same instant an unrelated writer-side failure cancels w.ctx, and the consumer's Next() call is already parked in the second, blocking select (i.e., the first non-blocking pre-check ran a moment too early and found nothing), both case <-it.results (now closed, always ready) and case <-it.wal.ctx.Done() become ready simultaneously. Per the Go language spec, a select with multiple ready cases picks uniformly at random, so Next() can take the ctx.Done() branch and return 'WAL shut down during iteration: ...' even though every record through maxIndex was already delivered and the correct return value was (false, nil).\n\nWhy existing code doesn't prevent it. The first, non-blocking select in Next() exists specifically so a clean EOF is 'never lost to a concurrent WAL shutdown,' per its own comment — but it only catches a close that happened strictly before that particular Next() call executes it. It cannot catch a close that happens while the consumer is already waiting in the second, blocking select for that same call. This is a real gap between the documented invariant ('never lost') and what the code actually guarantees.\n\nWhy the existing tests don't catch it. TestIteratorSurfacesFatalCause and TestIteratorReaderExitsWhenWALTornDownWhileOrphaned both engineer the WAL failure to occur before or during draining, i.e., before EOF is reached — in those tests, taking the ctx.Done() branch is the correct outcome, since iteration is genuinely incomplete. Neither test drives the reader to close it.results at the same instant a failure cancels ctx, so the boundary race is untested.\n\nOn the refutation that this is intentional. One reviewer argued this is by-design: the first select fully honors the 'never lost' promise for the case it targets (buffered results ahead of a close), and reaching the second select means the consumer genuinely cannot yet know whether the reader is 'about to close cleanly' or 'still producing,' so surfacing the fatal WAL state as an error is a defensible fallback. That's a reasonable design rationale for most of the second select's cases — but it doesn't fully hold for the exact instant the close and the cancellation race each other. In that instant, the reader is not 'still producing' at all: it has already produced its true final signal (a closed, drained channel), and only the scheduler's essentially-random choice of which case fires determines whether the consumer sees that correct signal or a manufactured error. The code's own comment frames the first select as an ordering fix for exactly this class of problem; the second select simply doesn't fully close the same gap for the split-second window where the close and the cancellation are simultaneous.\n\nStep-by-step proof. (1) A caller opens Iterator(1, N) and calls Next() N times, each time observing (true, nil). (2) On what will be the reader's final record, it returns ok=false from nextRecord() and is about to call close(it.results). (3) Concurrently, on the same walImpl, a Flush/Append/rotate/prune I/O error occurs, and writerLoop calls w.fail(err), which calls w.cancel(err). (4) The consumer calls Next() one final time; its non-blocking pre-check finds it.results empty (the close hasn't executed yet), so it falls into the blocking select. (5) The reader's close(it.results) and the writer's cancel(err) both execute a moment later, before the blocking select resolves — both cases are now ready. (6) Per the Go spec, the runtime picks one of the two ready cases uniformly at random; roughly half the time it takes case <-it.wal.ctx.Done(), sets it.done = true, and returns (false, fmt.Errorf("WAL shut down during iteration: %w", err)) — even though every record through maxIndex was already correctly delivered in steps (1)-(2) and the true outcome should have been (false, nil).\n\nImpact and fix. No data is lost or corrupted — every record was already delivered via prior Next() calls — but a caller that treats a Next() error as 'my read may be incomplete, retry or abort' gets a false failure signal on an iteration that actually completed successfully, at the exact moment an unrelated part of the same WAL instance happens to be failing. The fix is small and localized: inside the ctx.Done() case, do one more non-blocking receive on it.results before honoring the shutdown error, so a concurrently-completed close is still observed as a clean EOF.
3 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