move pebble checkpoints off of the execution goroutine - #4039
Conversation
PR SummaryHigh Risk Overview Commit now Adds Also bumps default Reviewed by Cursor Bugbot for commit ef4687b. 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).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4039 +/- ##
==========================================
- Coverage 61.22% 60.62% -0.60%
==========================================
Files 2153 2079 -74
Lines 188393 182423 -5970
==========================================
- Hits 115351 110602 -4749
+ Misses 62298 61677 -621
+ Partials 10744 10144 -600
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Moving FlatKV pebble checkpointing onto a dedicated SnapshotWriter goroutine is well-structured: reservation ownership is handed off cleanly at every path (written/declined/failed/discarded), failures are latched and re-surfaced on the commit path, and teardown drains an in-flight checkpoint before the databases close. Two non-blocking issues: the new max-snapshot-lag-blocks key is declared configurable but never read, and the 256× increase in MaxUnflushedVersions raises an in-memory backlog that has no byte-based bound.
Findings: 0 blocking | 2 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // reach disk until it completes, and each one is retained in memory meanwhile. This bounds how far | ||
| // that can run, trading a pause in block production for the memory the backlog would otherwise | ||
| // consume. It bounds blocks rather than bytes, so it mitigates exhaustion rather than preventing it. | ||
| MaxSnapshotLagBlocks uint32 `mapstructure:"max-snapshot-lag-blocks"` |
There was a problem hiding this comment.
[suggestion] The mapstructure:"max-snapshot-lag-blocks" tag is inert: neither sei-cosmos/server/config.GetConfig (which explicitly reads five other state-commit.flatkv.* keys — fsync, async-write-buffer, snapshot-interval, snapshot-keep-recent, enable-read-write-metrics) nor app/parseSCConfigs reads this key, and the Viper path ignores unknown TOML keys. An operator who sets state-commit.flatkv.max-snapshot-lag-blocks in app.toml gets silence, and the queue stays pinned at 512.
This knob is the whole of the writer's backpressure and the one lever an operator has when a checkpoint outruns block production, so it is the flatkv field most worth wiring rather than least. Suggest adding the guarded read alongside the other four in GetConfig, and a row in sei-cosmos/server/config/config_fuzz_test.go ({Key: "state-commit.flatkv.max-snapshot-lag-blocks", Path: "StateCommit.FlatKVConfig.MaxSnapshotLagBlocks", Set: ...}) per testutil/configtest/AGENTS.md. If it is deliberately not operator-tunable for now, dropping the tag (as ExternalPruning does with mapstructure:"-") records that decision instead of implying one that does not hold.
| MetricsEnabled: true, | ||
| MetricsScrapeIntervalSeconds: 10, | ||
| MaxUnflushedVersions: 4, | ||
| MaxUnflushedVersions: 1024, |
There was a problem hiding this comment.
any OOM risk here to MaxUnflushedVersions to 1024?
There was a problem hiding this comment.
Changed this to 128. In benchmarking, I observed 1024 was safe. But probably not necessary to be quite as big as that.
| // Step 5: Offer the block to the snapshot writer, which decides whether it becomes a snapshot and, | ||
| // if so, writes it on its own goroutine. Periodic snapshots are what keep the WAL bounded and | ||
| // restarts fast. | ||
| if s.snapshotWriter != nil { |
There was a problem hiding this comment.
Offer now reports the writer's latched failure, so the error names the current height, not the height whose snapshot failed. The halt is also one block late. Both are acceptable if nothing downstream reads that height for recovery. Please confirm, and consider naming the failed height in the latched error so the log points at the real block.
There was a problem hiding this comment.
Error currently reports both the block that failed and the block where the error bubbles up. Nothing should be attempting to recover this, the block number is for debugging only.
| // cadence is outrunning. | ||
| w.phaseTimer.SetPhase("idle") | ||
| select { | ||
| case <-w.ctx.Done(): |
There was a problem hiding this comment.
select randomly chooses among ready cases, so shutdown competes with the queue instead of taking priority. Once stop() is called, w.ctx.Done() remains ready, causing races on both sides of the channel.
Here, a queued message may win, causing Close to wait for another snapshot and breaking its guarantee that queued work is discarded.
In enqueue at L166, a send may win if the queue has room, so Offer returns nil even though the writer may exit without processing it. Commit then reports success for a snapshot that may never be written.
Both sides need to be fixed: one prevents extra work during shutdown; the other prevents false success after shutdown.
There was a problem hiding this comment.
IMO, not a problem if shutdown loses the race. Even if shutdown was guaranteed to win in the select statement, we might receive a shutdown one nanosecond after we start a pebble checkpoint... so it's still possible that shutdown might be delayed by a pebble checkpoint.
Also mitigating this problem is that in production use cases, we don't really shut down a store so much as pull the rug out from under it... so I don't think a slow shutdown really hurts us much.
| FlatKVConfig.AccountStoreConfig.MetricsEnabled = bool(true) | ||
| FlatKVConfig.AccountStoreConfig.MetricsScrapeIntervalSeconds = float64(10) | ||
| FlatKVConfig.AccountStoreConfig.MaxUnflushedVersions = uint64(4) | ||
| FlatKVConfig.AccountStoreConfig.MaxUnflushedVersions = uint64(1024) |
There was a problem hiding this comment.
This seems to be a pretty big jump? Why do we need that big of backlog?
There was a problem hiding this comment.
It was helpful during performance testing to absorb bursty behavior. 1024 is larger than it needs to be though, after I did several optimizations. Reduced to 128.
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 ef4687b. Configure here.
| case <-w.ctx.Done(): | ||
| return fmt.Errorf("clone snapshot for version %d: %w", targetVersion, w.stoppedError()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Clone aborts while copy still runs
Medium Severity
CloneSnapshot returns as soon as the writer context is cancelled, even after the clone is already queued or running. LoadVersionReadOnly then treats that as failure and Close deletes readOnlyWorkDir while createWorkingDir may still be copying into it. That is the documented Close-during-export window: the copy can recreate or leak a readonly-* directory, or observe a half-deleted dest, instead of finishing and failing later on WAL close.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ef4687b. Configure here.
3 participants
Describe your changes and provide context
Moves pebble checkpointing (what we call snapshots) off of the transaction execution goroutine