refactor(pkg/finality-grandpa): make closing globalIn the voter's shutdown signal - #4852
Merged
Conversation
…tdown signal Voter.Stop both signalled shutdown and tore the voter down, while the caller separately owned the globalIn channel. Neither alone was sufficient, and that split is where the forwarder leak lived. Closing globalIn is now the only shutdown signal, and Stop becomes Wait: it blocks until the Start loop has returned, then releases what the voter itself owns. - processIncoming returns a sentinel when globalIn closes, and Start reports it as a nil error, since an orderly shutdown is not a failure. - stopChan is gone. Removing it is what fixes the leak: the poll loop no longer has an early exit, so it can only leave by observing the close, which means first draining whatever the forwarder is holding on the unbuffered out. - wakerChan.start wakes its consumer on exit. Without it the poll loop would sit on the waker until some unrelated source happened to fire. - Wait takes the WaitGroup token out in NewVoter and arbitrates ownership with runState. Start runs in a goroutine, so Wait can otherwise reach wg.Wait before Start reaches wg.Add, and proceed to tear down a voter coming up. - Wait is idempotent, so an owner that both supervises the voter and shuts the node down can reach it twice. Handle closed channels at the other wakerChan consumers too, where an unchecked receive stayed permanently ready and handed out zero values: pruneBackgroundRounds spun while holding v.inner, which also blocked VoterState; votingRound.processIncoming dereferenced a nil Message interface; and finishPrevoting read a closed channel as "no best chain", parking the round in prevoting forever. Fix a pre-existing race in waker.wake, which read w.wakeCh inside the spawned goroutine after releasing the lock, racing register's write. Give the test network per-node teardown. route delivered outside the lock, so closing a single node's channel could race a send.
…Start Start existed only so callers could schedule the poll loop themselves, and every one of them did the same thing with it: run it in a goroutine straight after construction. The window it opened between building a voter and running one is where the lifecycle bugs lived — Wait could reach wg.Wait before Start reached wg.Add, and a voter could be torn down while coming up. NewVoter now runs the voter, so there is no such window and no half-live state to observe. That removes the machinery that existed to police it: the runState atomic and its CAS in both Start and Wait, the lifecycle constants, the WaitGroup token arbitrated between two owners, and the "already started" error path. What replaces it is a done channel the run loop closes, which also publishes runErr. Add Done, returning that channel. Owners that supervise a voter alongside other work can select on it instead of committing to a blocking Wait, which is what voterWork was hand-rolling with an error channel and a goroutine. The public surface is now NewVoter, Done, Wait and VoterState, with close(globalIn) as the shutdown signal. Tests no longer wire globalIn after construction. Passing a placeholder and then overwriting voter.globalIn was always a smell; against a running voter it would be a data race, so globalIn is now built before NewVoter and passed in.
… drop Wait A voter has one owner, so the outcome does not need broadcast semantics. Done becomes a chan error carrying the terminal error directly, which collapses the two-step "select on Done, then call Wait" into one receive. Wait existed to make the teardown unskippable by binding it to the only way of learning why the voter stopped. Tearing down on the voter's own goroutine before publishing gives that for free and more strongly: receiving from Done means the voter has not merely stopped but finished releasing its finalization channel and every round timer. Callers have no teardown obligation at all. That removes the rest of the lifecycle machinery: Wait, the teardown sync.Once and its stored error, the separate runErr field, and waitTimeout along with the timeout error that was the one non-nil return meaning "still running". A caller wanting a deadline composes it against Done. The public surface is now NewVoter, Done and VoterState, with close(globalIn) as the shutdown signal. The trade is that the error goes to a single receiver and the channel closes behind it, so a later receive yields nil. That is documented on Done and pinned by a test.
…Wait Comments still described the lifecycle in terms of methods that no longer exist, including one predating this branch that referred to stalling Stop().
timwu20
marked this pull request as ready for review
August 5, 2026 22:45
timwu20
marked this pull request as draft
August 5, 2026 22:53
…advance A running voter accumulated four goroutines per round, none of them released until the process ended. Over 8 rounds the forwarder count grew by 32. Timers accounted for three of the four. A timer wrapped an unbuffered channel in a wakerChan purely to deliver a wake, but nothing anywhere reads a timer's out: consumers use SetWaker and Elapsed, and Elapsed reads an atomic. So every timer that fired left its forwarder parked on a send with no receiver that would ever exist, and Close could not reach it — closing the input does not release a goroutine blocked on a send. The wakerChan is gone; the timer holds its waker directly and wakes it from the goroutine that already watches the deadline. Close now ends that goroutine when a round finishes before its timer fires. Setting expired before waking also fixes a race: waking first let a poller observe the timer as pending and go back to sleep. The fourth is roundData.Incoming, which belongs to the environment. Its forwarder ends only when that channel closes, so a round input left open outlives its round. Environment.RoundData now documents that implementations close it once the round concludes and release any still open at shutdown, and the test environment does so from Concluded. RoundData is called from two sites and may be reached more than once for a round number, so each call's channel is tracked rather than one per round.
timwu20
marked this pull request as ready for review
August 5, 2026 23:13
haikoschol
reviewed
Aug 6, 2026
Closing globalIn is the shutdown signal, and a nil channel reads as already closed, so a voter built on one stopped immediately and reported a clean shutdown through Done — indistinguishable from a successful one.
dimartiro
approved these changes
Aug 6, 2026
haikoschol
approved these changes
Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
The voter's lifecycle was split across two owners with overlapping duties:
Stopboth signalled shutdown and tore the voter down, while the caller separately ownedglobalIn.Stopleft the forwarder reading a channel it did not own; closingglobalInreleased nothing the voter owned.Startseparately opened a window between constructing a voter and running one, in which a shutdown could race a startup.The voter is now three things:
Close
globalInto shut it down:globalInis the only shutdown signal. The poll loop has no early exit, so it leaves only by observing the close — which means first draining whatever the forwarder is holding on the unbufferedout. A retired voter's forwarder can no longer outlive it: across 10 rotations with traffic in flight, 10 leaked before, 0 now.NewVoterruns the voter, so there is no half-live state and nothing to schedule.Done, so a receive there means it has released its finalization channel and every round timer. Callers have no teardown obligation.globalInbelongs to the caller, who must be its only writer by the time it closes, or must serialise its writers against the close.A running voter no longer accumulates goroutines
Separately from shutdown, the voter leaked four goroutines per round, released only when the process ended. Over 8 rounds the forwarder count grew by 32.
Three of the four were timers. A timer wrapped an unbuffered channel in a
wakerChanpurely to deliver a wake, but nothing anywhere reads a timer'sout— consumers useSetWakerandElapsed, andElapsedreads an atomic. So every timer that fired stranded its forwarder on a send with no receiver that would ever exist, andClosecould not reach it, because closing an input never releases a goroutine blocked on a send. ThewakerChanis gone: the timer holds its waker directly and wakes it from the goroutine already watching the deadline, andCloseends that goroutine when a round finishes before its timer fires. Settingexpiredbefore waking also fixes a race where a poller could observe the timer as pending and go back to sleep.The fourth was
roundData.Incoming, which belongs to the environment. Its forwarder ends only when that channel closes, so a round input left open outlives its round.Environment.RoundDatanow documents that implementations close it once the round concludes and release any still open at shutdown; the test environment does so fromConcluded.RoundDatais reachable more than once for a round number, so each call's channel is tracked rather than one per round.TestVoter_RoundsDoNotAccumulateForwarderspins this: +0 over 8 rounds, against +32 without the fixes.Closed channels at the other wakerChan consumers
An unchecked
<-chon a closed channel stays permanently ready and hands out zero values, so thedefaultarm that would break the loop is unreachable. Three sites:pruneBackgroundRoundsv.inner, which also blocksVoterStatevotingRound.processIncomingMessageinterface — panicfinishPrevotingpast_rounds.goalready handled this and is unchanged: a background round's commit channel closes when the round concludes, which the live poll loop is expected to see, so breaking out is right there.Incidental
waker.wake, which readw.wakeChinside the spawned goroutine after releasing the lock, racingregister's write.wakerChan.startwakes its consumer on exit, so a shutdown reaches the poll loop rather than waiting on an unrelated source.routesnapshottedsendersunder the lock but delivered outside it, so closing one node's channel could race a send.globalInbeforeNewVoterand pass it in, rather than overwritingvoter.globalInafterwards.Known gaps outside the package
The gossamer callers under
internal/client/consensus/grandpaare not brought up to the new contract here. Listed for the record rather than as pending work:voterWorknever closesglobalIn, so a retired voter's forwarder is not released, and theerr == nilguard inpollwould read an orderly shutdown asErrSafetyonce it does.environment.RoundDatadoes not close round inputs, so it does not meet the obligation now documented on the interface.communication.go'scombinedIncomingforwardercontinues past its own break check, so the break is dead code and the goroutine spins once its input closes.Test_networkBridgeraces ondevelopmenttoday, unrelated to this change.