Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dd9e44f to
d328e8f
Compare
The engine test helpers drained core.CheckpointCh with a for-range loop
that read the channel a second time inside its body:
for range core.CheckpointCh {
checkpointChanMsg := <-core.CheckpointCh
...
}
Every iteration therefore consumed two signals, and a body reached with a
single signal pending blocked forever. A single pending signal is all the
sender can leave behind now that it coalesces, so the stall got easy to
hit - and it stayed silent: the helpers are long-lived goroutines with no
exit path, so nothing failed, the loop just never ran again.
Take the value from the range variable instead, which reads every signal
exactly once. The shape is old - it goes back to XinFinOrg#46 (2022) and was copied
into the other five sites over the years - so the fix covers all six.
d328e8f to
be15ad4
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The focused changes correctly eliminate double channel receives across all affected helpers.
Pull request overview
Fixes checkpoint-signal handling in consensus test helpers so each loop iteration consumes exactly one channel value.
Changes:
- Corrected five V2 helper consumers.
- Corrected one V1 helper consumer.
File summaries
| File | Description |
|---|---|
consensus/tests/engine_v2_tests/helper.go |
Uses range values at five checkpoint consumers. |
consensus/tests/engine_v1_tests/helper.go |
Uses the range value at the V1 checkpoint consumer. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
What
Six engine-test helpers drained
core.CheckpointChwith a loop that read the channel twice per iteration:for rangealready received a signal and the body received another one, so every iteration consumed two signals — and a body reached with only one signal pending blocked forever.Why it matters
These helpers run in long-lived goroutines with no exit path, so the stall is silent: nothing fails, the loop simply never runs again.
It also got much easier to hit.
CheckpointChnow carries a buffer of one and its senders coalesce on purpose, so at most one signal is ever pending — which is exactly the case this loop deadlocks on. While the channel was unbuffered and the import path could queue several signals back to back, a burst could mask the bug.How it changed
Both helpers take the value from the range variable, so every signal is read exactly once:
consensus/tests/engine_v2_tests/helper.go— 5 sites (now lines 523, 610, 666, 731, 795)consensus/tests/engine_v1_tests/helper.go— 1 site (line 307)Verification
gofmt/goimportsclean;go vet ./consensus/tests/...passesTestIsAuthorisedMNForConsensusV1passesmake allandmake quick-test— no failuresThe checkpoint coalescing this interacts with belongs to the #2534 / #2535 line of work. This change is independent of it: it touches the engine test helpers only, never production code.