Repository navigation
bft: enqueue a decision when its view is aborted - #742
Conversation
| case <-view.AbortChan(): | ||
| // The view is stopping while the controller is busy with a long sync or | ||
| // delivery. Do not block the view, or the abort would wait for it, but do | ||
| // not drop the decision either: hand it to the controller in the background. | ||
| go c.enqueueDecision(d) |
There was a problem hiding this comment.
Small thing: the deadlock in the description is the controller loop sitting in abortView waiting for this view, not a long sync or delivery. Could the comment say that? Otherwise someone might think this case only matters for slow syncs.
(Written by Claude Code.)
| // enqueueDecision hands a decision to the controller when the view that decided it is | ||
| // already stopping. The decision was accepted by a quorum, so it must not be lost: | ||
| // the controller delivers it once it is no longer busy. If the view changer has | ||
| // already delivered the same proposal, MutuallyExclusiveDeliver drops the duplicate |
There was a problem hiding this comment.
Nit: MutuallyExclusiveDeliver doesn't just drop the duplicate. It runs a Sync() and resets the checkpoint. Maybe "skips delivering it again" instead of "drops"?
(Written by Claude Code.)
| type abortRecordingProposer struct { | ||
| aborted bool | ||
| abortChan chan struct{} | ||
| } | ||
|
|
||
| func (p *abortRecordingProposer) Propose(types.Proposal) {} | ||
| func (p *abortRecordingProposer) Start() {} | ||
|
|
||
| func (p *abortRecordingProposer) Abort() { | ||
| p.aborted = true | ||
| if p.abortChan != nil { | ||
| close(p.abortChan) | ||
| } | ||
| } |
There was a problem hiding this comment.
completion_signal_test.go already uses &View{abortChan: make(chan struct{})} with view.stop(). Could this test use that instead of a new fake? aborted is never read either.
(Written by Claude Code.)
bdd7ae1 to
4bed5c2
Compare
|
fixed |
| // TestDecideDoesNotDropDecisionOnAbortedView covers the hand-off between the view | ||
| // and the controller: a view that is being aborted must not stay blocked on a full | ||
| // decision channel, but the committed decision must still reach the controller. | ||
| func TestDecideDoesNotDropDecisionOnAbortedView(t *testing.T) { |
There was a problem hiding this comment.
completion_signal_test.go is also in package bft and only has TestControllerDecideViewBookkeeping, which tests the same view-to-controller hand-off. Could both tests go in one file? Your name fits both better, so maybe rename completion_signal_test.go to controller_decide_test.go and add this test there.
(Written by Claude Code.)
Decide blocked when the decision channel was full and the view was being aborted: the controller loop could be inside abortView waiting for the view to stop, and the view could be waiting to enqueue its decision. The abort and the decision waited for each other. Hand the decision to the controller in the background when the view is aborting. The decision is not dropped: MutuallyExclusiveDeliver filters a duplicate that the view changer has already delivered. Signed-off-by: Fedor Partanskiy <[email protected]>
4bed5c2 to
107b7f8
Compare
Decide blocked when the decision channel was full and the view was being aborted: the controller loop could be inside abortView waiting for the view to stop, and the view could be waiting to enqueue its decision. The abort and the decision waited for each other.
Hand the decision to the controller in the background when the view is aborting. The decision is not dropped: MutuallyExclusiveDeliver filters a duplicate that the view changer has already delivered.
#736