Add global checkpoint scheduler - #4026
Conversation
PR SummaryMedium Risk Overview Refactor: Per-engine checkpoint orchestration ( The new global scheduler is test-only for now—no production wiring in this PR (follow-up expected). Reviewed by Cursor Bugbot for commit 60fe243. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4026 +/- ##
==========================================
- Coverage 61.25% 60.33% -0.92%
==========================================
Files 2153 2069 -84
Lines 188426 178153 -10273
==========================================
- Hits 115414 107483 -7931
+ Misses 62274 60785 -1489
+ Partials 10738 9885 -853
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![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 445e090. Configure here.
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean relocation of the SS checkpoint helpers into ss/snapshot plus a new global CheckpointScheduler; the move is complete and consistent, and the scheduler is thoroughly unit-tested. The scheduler is not yet wired into production, and its store contract leaves checkpoint completion inferred rather than reported, which is worth pinning down before implementations land.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] The new global scheduler is not reachable from production: nothing implements
CheckpointableStore(grepforCheckpointInProgressfinds no implementation outsidesei-db/controller), andNewCheckpointScheduleris called only fromcheckpoint_scheduler_test.go. SS still constructs the existing per-composite coordinator insei-db/state_db/ss/composite/snapshot.go, so this PR adds the mechanism without enabling global checkpoint scheduling. Fine as a staged change — worth stating in the description that a follow-up wires it up, so it is not mistaken for a live feature. - [suggestion]
CheckpointableStoregives a store no way to report that a checkpoint failed. A failed checkpoint clearsCheckpointInProgress()and the store's version still advances past the target, so the scheduler reads it as a success: it starts the min-time gate and moves to the next boundary. Combined with the fact that only dispatch is logged (checkpoint scheduled), a node can silently stop producing usable checkpoints while the scheduler reports healthy activity. Consider a completion/error signal on the interface, or at minimum a failure counter the stores increment. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
There was a problem hiding this comment.
The relocation of the SS checkpoint helpers into ss/snapshot and the Scheduler→Checkpointer rename are complete and consistent (no stale references to the removed controller symbols remain), and the new global CheckpointScheduler is well tested. Two non-blocking issues remain in the scheduler: a dispatch that can be silently dropped by a store yet counted as a completed checkpoint, and a first cycle that runs before shutdown/cancellation is observed.
Findings: 0 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
checkpointPollIntervalis a hard-coded 10s while the rest of the cadence is configurable, andCheckpointConfig.Validatedoes not relate the two. The scheduler can therefore dispatch at most one boundary per 10s, so a smallCheckpointInterval(or fast blocks) silently yields a longer effective cadence than configured. Worth either validating/warning when the configured interval is short relative to the poll rate, or deriving the poll interval from the config. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| // scheduleNextCheckpoint runs one cycle: it hands the next boundary to every store, or does nothing | ||
| // when a store is still writing the last checkpoint, has not committed past its scheduled version, or | ||
| // the last one finished too recently. | ||
| func (s *CheckpointScheduler) scheduleNextCheckpoint() { |
There was a problem hiding this comment.
One nice to have property would be if this utility ensured that different checkpoint stores checkpoint the same block number. Not required for correctness of course, we're intentionally designing a system that doesn't break when checkpoints don't align (since a crash can cause us to be missing some checkpoints, even if we intend to checkpoint the same block). But its still nice to align them when we can.
If we wanted this property, we'd have to flip the model a little. Instead of the scheduler saying "it's time to checkpoint your current block", we'd have each of the stores ask the scheduler "should I checkpoint block X?". Via this mechanism we could ensure that we don't have an off-by-one block height difference when we snapshot our different stores.
There was a problem hiding this comment.
Changed to a poll model, but poll model has its own edge case and limitation as well.
|
@seidroid review |
There was a problem hiding this comment.
The controller → ss/snapshot relocation is complete and consistent (no stale controller.* checkpoint references remain, and the Scheduler → Checkpointer rename is applied everywhere), and the redesigned pull-based CheckpointScheduler is well documented and thoroughly tested. Two non-blocking issues concern the baseline used before the first checkpoint, which makes the two intervals behave asymmetrically across restarts.
Findings: 0 blocking | 3 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion] A store registers by asking and there is no way to remove it, so a store that asks once and then stops asking — closed, disabled by a mode switch, or its commit loop wedged — is cloned into
awaitingat the next pick and stops checkpointing for the whole node permanently. The only log on the path ischeckpoint complete, so this is indistinguishable from an idle node. Since nothing implements the caller side yet, either an explicit deregistration call or a periodic warn naming the stores a held height is still waiting on is cheap to add before the follow-up wires real stores up. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| config: cfg, | ||
| registered: make(map[string]struct{}), | ||
| awaiting: make(map[string]struct{}), | ||
| checkpointedAt: time.Now(), |
There was a problem hiding this comment.
[suggestion] Seeding checkpointedAt with time.Now() restarts the time interval on every process start, so a node that restarts more often than TimeInterval never checkpoints at all. With DefaultCheckpointConfig that is a 1-hour window: a node cycling every ~50 minutes produces no checkpoints and logs nothing beyond checkpoint scheduler created.
The analogous memiavl path deliberately avoids this — db.go:283 seeds lastSnapshotTime from the existing snapshot directory's mtime rather than from process start (getSnapshotModTime). Since DefaultCheckpointConfig is documented as mirroring those settings, consider giving the scheduler the same recovery: seed checkpointedAt from the newest existing checkpoint when the caller can supply one, rather than unconditionally from construction time.
| // hasReachedNextInterval reports whether every configured interval has passed for version. | ||
| func (s *CheckpointScheduler) hasReachedNextInterval(version int64) bool { | ||
| timeElapsed := s.config.TimeInterval <= 0 || time.Since(s.checkpointedAt) >= s.config.TimeInterval | ||
| blocksElapsed := s.config.BlockInterval <= 0 || version-s.nextCheckpointVersion >= s.config.BlockInterval |
There was a problem hiding this comment.
[suggestion] The block gate is measured from nextCheckpointVersion, which is 0 until the first height is picked, so before the first checkpoint it is effectively version >= BlockInterval — trivially true for any live chain. With a block-only config (TimeInterval unset), the very first version any store commits after a restart is therefore always a checkpoint height, whatever the node's height. The type doc at line 33 says both intervals are "measured from ... the scheduler's creation before there is one", which for the block interval would mean the height at creation; 0 is not that.
Recording a baseline version on the first ask (or, like checkpointedAt, seeding it at construction) would make the two gates behave the same way before the first checkpoint and match the documented contract. Worth noting that this and the checkpointedAt seeding are opposite failure modes of the same missing "initial baseline": the block gate never restricts, the time gate always restricts.
2 participants
Describe your changes and provide context
Two main changes in this PR:
This PR introduce the interface of new global scheduler, which is not reachable from production yet, nothing implements it. A follow-up PR will wire it up with existing stores.
Testing performed to validate your change