Repository navigation
Fix pruning issue in SS causing huge disk spike - #4321
Conversation
PR SummaryMedium Risk Overview In Also wires Reviewed by Cursor Bugbot for commit e615a10. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
There was a problem hiding this comment.
The fix is correct: deleted keys are cloned via bytes.Clone, so closing the scan iterator before commit and compaction is safe, and it releases the pinned readState before compactPrunedRange runs. The notes below cover structure, log noise, and the same bug still sitting in the ascending path.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] No regression test comes with the change. A test could check that the deferred-close path still works when the loop returns an error. Better, it could assert that obsolete sstables are freed after
pruneDescending, for example throughdb.storage.Metrics()or the table count / disk usage after a prune plus compaction. Either would stop the iterator lifetime from quietly regressing. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
sei-db/db_engine/pebbledb/mvcc/db_ascending.go(pruneAscending, around line 122) still has the bug this PR fixes. It closes the scan iterator withdeferonly aftercompactPrunedRangereturns, so nodes on the legacy ascending encoding keep the same sstable pinning and disk spike. Apply the same early close there, ideally through a shared helper. (Also raised by Codex.)
| // closing it twice: the scan closes it explicitly as soon as it's done | ||
| // reading, right before compaction, and this defer only still applies if an | ||
| // error returned from inside the loop below. | ||
| itrOpen := true |
There was a problem hiding this comment.
[suggestion] AGENTS.md ('Structural corrections') asks that this kind of fix be made by naming a step, not by adding an itrOpen flag, a closeItr closure, and two long inline comments. Consider moving the scan into its own function, e.g. scanAndDeletePruned(...) (first, last []byte, err error), that opens the iterator and closes it with a plain defer. pruneDescending then reads as scan → commit → compact, and the iterator is closed before compaction by construction. The note about readState pinning belongs in that function's doc comment. pruneAscending has the same bug and could use the same shape.
| return db.compactPrunedRange(firstDeletedKey, lastDeletedKey) | ||
| compactStart := time.Now() | ||
| err = db.compactPrunedRange(firstDeletedKey, lastDeletedKey) | ||
| logger.Info("pruneDescending: compacted pruned range", "version", version, "elapsed", time.Since(compactStart), "err", err) |
There was a problem hiding this comment.
[suggestion] This Info line runs on every prune pass, even when nothing was deleted and compactPrunedRange returned immediately (first == nil). It also logs failures at Info as err=<error>. Consider logging only when a compaction actually ran (firstDeletedKey != nil), and moving failures to Error/Warn or leaving them to the caller, which already receives the returned error.
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 184297f. Configure here.
| compactStart := time.Now() | ||
| err = db.compactPrunedRange(firstDeletedKey, lastDeletedKey) | ||
| logger.Info("pruneDescending: compacted pruned range", "version", version, "elapsed", time.Since(compactStart), "err", err) | ||
| return err |
There was a problem hiding this comment.
Ascending prune still pins iterator
Medium Severity
The early iterator close before compactPrunedRange is only applied in pruneDescending. pruneAscending still defers itr.Close() until after compaction, so the scan iterator keeps Pebble's readState pinned and obsolete sstables stay undeletable. Legacy and archive nodes on the ascending path still hit the same disk spike this change targets.
Triggered by learned rule: sei-db pebbledb: ascending and descending MVCC iterators must stay aligned
Reviewed by Cursor Bugbot for commit 184297f. Configure here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4321 +/- ##
==========================================
- Coverage 67.50% 66.44% -1.07%
==========================================
Files 2181 2060 -121
Lines 167848 155789 -12059
==========================================
- Hits 113306 103511 -9795
+ Misses 54532 52268 -2264
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Nice Finding! |
* main: (21 commits) Backport evmonly parse, app-hash and changeset perf fixes from giga-1 (#4345) Remove the oracle module behind a v6.8 upgrade (#4319) fix(seidb): report only the current migration boundary on the snapshot gauge (#4327) fix(flatkv): keep 10 old checkpoints instead of mirroring memIAVL's count (#4322) Backport Autobahn execute-loop and produced-tx metrics from giga-1 (#4330) Add giga.storage.receipts to toggle the Autobahn receipt store (#4333) optimize gather phase (#4326) Regenerate the Unreleased changelog as a plain PR list (#4336) Bump sei-protocol/go-ethereum to v1.15.7-sei-21 (#4332) Fail dynamic-gas precompile out-of-gas as an EVM out-of-gas call (#4318) Add dashboard and topology option for Autobahn e2e (#4167) Add Giga fetch/serve and BlockDB prune metrics (#4329) Add eth_getLogs to the EVM-only Giga RPC (#4308) Generate v6.8 precompiles (#4320) Add [giga] app.toml section and honor it on the Autobahn node (#4323) feat(evmonly): add eth_estimateGas via existing libraries (#4325) Use Pebble batch directly in SS (#4300) Fix pruning issue in SS causing huge disk spike (#4321) Make Autobahn always run the EVM-only executor, disable/remove some integration tests (#4316) reduce seal lock contention (#4314) ...
|
/backport |
|
Successfully created backport PR for |


left: fix

right: the bug