Avoid blocking on completion signals when the waiter has left

[kevindharmawan]

The controller and the view changer both used unbuffered channels to report completion, where the receiver can legitimately stop waiting and leave the sender blocked forever.

Controller: Decide stops waiting on deliverChan when the view aborts, but decide was still blocked sending on it, so the run loop stalled until shutdown. Now each decision carries its own delivered channel, which decide closes after delivery. Each decision also records the view that was current when Decide was called, and Decide waits on that view's abort channel rather than on whichever view is current by then. After delivery, if that view was aborted or replaced, decide only prunes revoked requests and skips the view bookkeeping (decision count, rotation check, leader-token acquisition), so a decision from a stale view is not accounted to the current one.

View changer: once commitInFlightProposal returned via timeout or stop, nothing read inFlightDecideChan or inFlightSyncChan. A late Decide or Sync then blocked the in-flight view's run goroutine, which blocked Abort() since it waits on that goroutine, which blocked the view changer in its own cleanup. No later attempt could start to drain the channel. Decide could at least escape on stopChan, while Sync had no escape at all. Now each call to commitInFlightProposal creates an inFlightAttempt with its own buffered decideCh and syncCh, which are signaled with non-blocking sends. The in-flight view's Decider and Sync are callbacks bound to that attempt at creation, so a late Decide or Sync can only signal its own attempt and cannot disturb a later one.

Adds regression tests for the three completion paths (controller delivery, in-flight decide, in-flight sync) and for skipping view bookkeeping when the deciding view was aborted or replaced.

[HagarMeir]

Behavior change worth confirming: previously decide blocked on the unbuffered deliverChan send when the waiter had left (view aborted), so the post-delivery bookkeeping below (incrementCurrentDecisionsInView, checkIfRotate/changeView, MaybePruneRevokedRequests, acquireLeaderToken) never ran in the aborted case. It now always runs once delivery happened. This unblocks the run loop (the real fix) and is defensible because the decision was in fact delivered, but it means a rotation changeView and leader-token acquisition can now fire for a view that is being aborted, racing the in-progress view change. changeView guards against going backwards (latestView > newViewNumber), which mitigates it, but please confirm the extra increment/rotate cannot momentarily double-count or start a spurious rotation view before the pending abort/view-change is processed.

[kevindharmawan]

Addressed in a167920.decide now records the view that produced the decision, and decide checks after delivery that this view is still current and not stopped. Otherwise it only prunes revoked requests and returns, skipping the increment, checkIfRotate/changeView, and acquireLeaderToken. The check runs on the controller run loop, the same goroutine that handles abortView and changeView, so a view change already processed is always observed first. For a dead view the result matches the pre-PR behavior, where the blocked send prevented the bookkeeping, minus the hang.

TestControllerDecideSkipsViewBookkeepingIfDecidingViewIsGone covers the aborted and replaced cases.

[HagarMeir]

Dead state: id, view, and sequence on inFlightAttempt (and ViewChanger.inFlightAttemptSeq that feeds id) are written but never read anywhere — attempt identity is established purely by pointer equality (v.inFlightAttempt == attempt in the cleanup defer). Only decideCh, syncCh, and viewRef are actually used. Consider dropping the unused fields and the inFlightAttemptSeq counter to reduce clutter.

[kevindharmawan]

Removed in 72dd0a2, along with the inFlightAttemptSeq counter.

[HagarMeir]

These public Decide/Sync methods (and currentInFlightAttempt) are now dead in production. In-flight views are wired with the per-attempt callbacks (Decider: callbacks, Sync: callbacks), and nothing else wires ViewChanger as a Decider/Synchronizer (the heartbeat handler is c.controller, and consensus.go never passes the ViewChanger as a Decider/Sync). Only the new tests call them. Beyond being dead code, they are a latent foot-gun: when invoked with a nil attempt they still call Application.Deliver/Synchronizer.Sync without stopping any view or signaling. Consider removing them rather than keeping them as a compatibility shim.

[kevindharmawan]

Removed in 72dd0a2, together with currentInFlightAttempt. The tests now call decideInFlight and syncInFlight directly with an explicit attempt, so the nil-attempt path no longer exists.

[HagarMeir]

Dead branch: because this select has a default, it never blocks, so the case <-v.stopChan: branch can never be the distinguishing choice — and all three branches just return anyway. The stopChan case is unreachable/pointless here (same in syncInFlight, which correctly omits it). Simplify to a plain non-blocking send:

	select {
	case attempt.decideCh <- struct{}{}:
	default:
	}

[kevindharmawan]

Simplified to the plain non-blocking send in 72dd0a2, matching syncInFlight.

[HagarMeir]

🤖 This PR was reviewed by Claude. See the inline comments above for detailed findings.

[HagarMeir]

@kevindharmawan please rebase on the new main
and please update the PR's description to match your latest commit

[kevindharmawan]

@kevindharmawan please rebase on the new main and please update the PR's description to match your latest commit

Done!

[HagarMeir]

🤖 This review was written by Claude (on behalf of @HagarMeir).

[HagarMeir]

🤖 Written by Claude.

This check treats two different cases the same way, and in one of them skipping the increment leaves currDecisionsInView one too low:

• Replaced (d.view != currView): skipping all of the bookkeeping is right.
• Still current but stopped (e.g. AbortView at the start of a view change; the run loop can handle abortViewChan before the buffered decision in decisionChan): the decision really was delivered in this view, and MutuallyExclusiveDeliver has already moved the checkpoint to its sequence, but incrementCurrentDecisionsInView is skipped.

In the second case, if the node then syncs and finds nothing newer, the same view is restarted with the lower count:

• run -> c.changeView(c.getCurrentViewNumber(), vs.(ViewSequence).ProposalSeq, c.getCurrentDecisionsInView()) (line 522), or
• sync() -> newDecisionsInView = c.getCurrentDecisionsInView() when latestDecisionSeq == controllerSequence (line 648).

With leader rotation on, getLeaderID then gives this node a different leader from the other nodes until the next ViewChanged resets the count to 0.

Possible fix: keep the full skip for the replaced case, but when d.view == currView and it is stopped, still call incrementCurrentDecisionsInView() and skip only checkIfRotate/changeView and acquireLeaderToken, the steps that could race the pending view change. Or, if the lower count is harmless here, could you explain why?

[HagarMeir]

🤖 This review was written by Claude (on behalf of @HagarMeir). These are all nits and not blocking.

One more nit that can't go inline because the line isn't part of this diff: in commitInFlightProposal, after v.inFlightViewLock.Unlock(), the log line v.Logger.Debugf("Node %d started a view %d for the in flight proposal", v.SelfID, v.inFlightView.Number) reads the shared v.inFlightView field without the lock. It's safe today, since only this goroutine writes the field, but using the local inFlightView.Number would be clearer now that the field is also cleared in the deferred cleanup.

[HagarMeir]

🤖 Written by Claude. Nit: this test can't fail. decide now only closes d.delivered, and close never blocks, so requireReturns passes whether or not anyone is waiting. The same goes for the two view-changer tests below: they call decideInFlight/syncInFlight directly, and the non-blocking select makes them pass trivially.

None of them reproduce the hangs this PR fixes:

• controller: Decide returns through view.AbortChan() while the decision is still buffered in decisionChan, then the run loop handles it and must keep processing later events;
• view changer: commitInFlightProposal times out or stops, and the deferred inFlightView.Abort() must not hang on a late Decide/Sync from the in-flight view.

A test that drives the real run loop (or commitInFlightProposal with a ticker that times out) and checks it doesn't hang would be a much stronger regression test, since it would fail on the old code.

[HagarMeir]

🤖 Written by Claude. Nit: d.delivered is never nil in production, since Decide always sets it, while a nil d.view panics in isRunningCurrentView. The two fields are set together, so it would be more consistent to treat them the same way: either drop this nil check, or handle a nil view the same way instead of panicking.

[HagarMeir]

🤖 Written by Claude. Nit (optional): if the attempt times out or is stopped while the in-flight view is inside decideInFlight, this Abort() waits for the delivery to finish, so the in-flight proposal was in fact committed. commitInFlightProposal still returns false, and the node skips changing the view until the next view change. Since decideCh is buffered, a non-blocking receive on attempt.decideCh after the abort could turn the result into true (e.g. via the named success return). This doesn't affect correctness, only how quickly the node recovers.