Conversation
A read-only open went through tidwall's wal.Open on the live directory, which completes an interrupted TruncateFront and may truncate a torn tail. Racing a live writer's TruncateFront, it removed the writer's segments and made the writer's own truncation fail as a fatal WAL error. Config.ReadOnly now opens the log from a private view beside the directory, hard-linking its segments and copying the files the open may rewrite, and refuses writes and truncations. memiavl's read-only open and the changelog range reader set it.
|
I'll fix CI failures and address comments from users with write access that start with 'Devin'.
|
|
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 #4441 +/- ##
==========================================
+ Coverage 56.75% 56.77% +0.02%
==========================================
Files 2126 2127 +1
Lines 166836 166925 +89
==========================================
+ Hits 94691 94775 +84
- Misses 72140 72145 +5
Partials 5 5
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 memiavl passes Reviewed by Cursor Bugbot for commit 01cb9e6. Bugbot is set up for automated code reviews on this repo. Configure here. |
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 212a92c. Configure here.
|
@seidroid review |
There was a problem hiding this comment.
Read-only changelog WAL opens now run against a private directory beside the log, made of hard links plus copies of the tail and any truncation markers, so tidwall's interrupted-truncation cleanup and torn-tail repair no longer touch a log another writer is using; memiavl read-only opens and the changelog-range reader use it. The approach holds up against the writer races checked, so this is an approval; the remaining items concern how robust the view is and leaks on error paths, and none is a correctness break today. Both of codex's findings are kept: the tail-selection one is downgraded from high to a suggestion because no file that sorts after the tail can trigger the harmful case today, and the trailing-slash one is a nit because every current caller passes a cleaned path.
Non-blocking
3 findings on the changed lines, as inline comments.
- Cleanup of crashed readers' views depends on memiavl's writer-side
removeTmpDirs. That function deletes every*-tmpdirectory under the memiavl dir at writer startup, including views a live reader in another process still holds open, such as theseidb evm-logical-digestsidecar. tidwall loads segments by path on demand, so a node restart during a sidecar scan now makes the scan fail with ENOENT, which reading the live directory did not do. Consider giving reader views a distinct marker, or a liveness check such as an flock, before removing them.
1 nit, not posted on the code
sei-db/wal/readonly.go:26— (From codex, verified.) With a trailing slash,filepath.Dir(dir)returns the WAL directory itself, so the view would be created inside the log it promises not to modify. Current callers passfilepath.Joinoutput, so this is latent;filepath.Clean(dir)first closes it.
seidroid review · decision approve · session f64ef74857fb4bef9d4980d311f8efcf · turn resp_claude_3e73e9d130f12efd1e2659e91d2f0639 · item d724d0f0b2f45e0994ba913998ddc7db
Findings: 0 blocking | 4 non-blocking | 3 posted inline
…B errors The view linked the tail segment whenever a file that is not a segment sorted after it, so repairing a corrupted tail truncated the live segment. Copy both the last segment and the file the tail repair targets, close the WAL when OpenDB fails after opening it, clean the source path, and count failed opens in the concurrency test.
|
@seidroid review |
There was a problem hiding this comment.
Since the last review, the read-only view also copies the last segment-length file that the tail repair truncates, OpenDB closes the WAL (and its view) when it fails after opening it, the concurrent test now counts reader failures, and the directory is cleaned before its parent is taken; all three of my earlier threads are resolved. Nothing blocks; codex's new .-directory finding holds but is a nit because callers always pass a path ending in changelog, and my earlier note about removeTmpDirs deleting views that live readers still hold has not been addressed.
Non-blocking
- Cleanup of crashed readers' views still relies on memiavl's writer-side
removeTmpDirs. At writer startup it deletes every*-tmpdirectory, including views that a live reader in another process, such as theseidb evm-logical-digestsidecar, still holds open. tidwall loads segments by path on demand, so restarting a node during a sidecar scan now makes the scan fail with ENOENT. Before this change, reading the live directory did not fail that way. Consider a distinct marker or a liveness check, such as an flock, before removing a view.
1 nit, not posted on the code
sei-db/wal/readonly.go:28— (From codex, verified.) Fordir == ".",filepath.Dirreturns., so the view would be created inside the directory it promises not to modify. Every current caller passesfilepath.Join(..., "changelog"), so this cannot happen today; usingfilepath.Absinstead offilepath.Cleancloses it.
seidroid review · decision approve · session f64ef74857fb4bef9d4980d311f8efcf · turn resp_claude_f6bcd0d7b5cb1fb400284a6fe9cc9934 · item 51287ec447845756b75a97c55175e14d
Findings: 0 blocking | 1 non-blocking | 0 posted inline

Every read-only open of a memIAVL changelog goes through tidwall's
wal.Openon the live directory: theseidb evm-logical-digestsidecar scan, historical ABCI proof queries andCacheMultiStoreForExportviaCommitStore.LoadVersion(v, true), and the changelog-range reader. On load, tidwall finishes any interruptedTruncateFrontit finds, deleting the segments ahead of a.STARTmarker and renaming the marker, and the repair path mayos.Truncatea torn tail in place. When the open lands between a live writer's creation of.STARTand its removal of the old segments, the reader deletes those segments first. The writer's own remove then fails with ENOENT, tidwall marks the log corrupt, andhandleTruncatereports it as a fatal WAL error.NoRepairOnOpennever covered this cleanup.wal.Config.ReadOnlynow opens the log from a private view created beside the directory (<dir>-readonly-*-tmp). The view hard-links the segments and copies the files the open may rewrite in place: the tail segment, any.START/.ENDmarker, and the last file with a segment-length name, which the tail repair truncates and which differs from the tail when a stray file sorts after it. Cleanup and repair therefore touch only the view. If the writer removes a segment while the view is being built, the build is retried. A read-only WAL refusesWriteand truncations, never prunes, and removes the view onClose.memiavl.OpenDBpassesOptions.ReadOnlythrough, and the changelog-range reader sets it. I rejected skipping tidwall's open, since that would mean reimplementing its segment loading.There is no consensus or state impact, and writers are unchanged. While a read-only log is open, its view keeps segments the writer has truncated alive on disk, and each open copies the tail segment (up to 20 MB). The
-tmpsuffix means memIAVL's startupremoveTmpDirsdeletes views left behind by a crashed reader, andOpenDBcloses the WAL, removing its view, when it fails after opening it.TestReadOnlyOpenDuringWriterTruncationruns four read-only openers against a writer that keeps truncating. Before this change the same loop made the writer fail within a few writes withremove ...: no such file or directory; with it, 20 race-enabled runs pass. The wal, memiavl, composite, seidb operations and rootmulti packages pass under-race.