fix(finality-grandpa): stop the voter without racing Start or leaking its reader

[dimartiro]

Changes

Voter.Stop did not tear down what NewVoter and Start had set up.

The wait token is now claimed in NewVoter, not in Start. Start blocks, so every owner runs it in a goroutine — which means Stop can reach wg.Wait before Start reaches wg.Add(1). That is an Add concurrent with a Wait, and
worse than a detector warning: Wait returns, Stop proceeds to close channels the voter is about to poll, and Start then blocks forever. A runState atomic.Int32 decides who releases the token — Start via its defer, or Stop when
Start never ran, in which case a later Start declines with an error instead of coming up into a torn-down voter. The CAS is the synchronisation point.

wakerChan's forwarding goroutine is now released on Stop. It starts in NewVoter and ranges over the input channel, so for globalIn — which belongs to the caller and is not the voter's to close — nothing ever ended it. It outlived
the voter holding one item hostage on the unbuffered out, and kept taking items off a channel a caller may well hand to the next voter. wakerChan gains a stop channel and an idempotent close(), selected on in both waits, and Voter.Stop
calls v.globalIn.close().

Stop is now idempotent. It closes channels, so a second call panicked on a closed channel. An owner that both supervises the voter and shuts the node down reaches it twice — this is the panic go-jam hit when rebuilding the voter across
an authority-set rotation. The body moves to stop() behind a sync.Once that records the error, so later callers block until the first has finished and get the same result.

Tests

go test -race -count=2 ./pkg/finality-grandpa/...

Four new tests in voter_test.go, all failing on development — and two of them not by failing an assertion but by hanging, which is the point:

┌────────────────────────────────┬─────────────────────────────────────────────────────────────────────────────────────┐
│              test              │                                    on development                                   │
├────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────┤
│ TestVoter_StartAndStopRace     │ deadlock: Stop returns, Start stays blocked in the poll loop, test times out at 45s │
├────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────┤
│ TestVoter_StopBeforeStart      │ deadlock: Start never returns after a Stop that preceded it                         │
├────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────┤
│ TestVoter_StopReleasesGlobalIn │ fails: "something is still reading globalIn after Stop"                             │
├────────────────────────────────┼─────────────────────────────────────────────────────────────────────────────────────┤
│ TestVoter_StopIsIdempotent     │ panic: close of closed channel                                                      │
└────────────────────────────────┴─────────────────────────────────────────────────────────────────────────────────────┘

The existing suite passes with -race -count=2, and golangci-lint is clean on the package.

[timwu20]

Hey @dimartiro — I've opened #4852, which I think supersedes this. Laying out the reasoning rather than just closing, because all three problems you identified are real and two turned out to be bigger than described.

What this PR gets right. I reproduced all three against development:

• The wg.Add(1)-in-Start racing Stop's wg.Wait is genuine misuse, with the consequence you describe. The race detector catches Stop's teardown switch reading inner.bestRound.state while votingRound.primaryPropose writes it — Wait returned early and Stop walked into a voter still coming up.
• The forwarder over globalIn does outlive the voter: 10 leaked across 10 rotations with traffic in flight, parked on the unbuffered out holding an item.
• Stop does panic on a second call.

Why #4852 takes a different route. Rather than fixing each symptom, it removes the split that produces them. The voter had two lifecycle mechanisms — Stop signalled shutdown and tore down, while the caller separately owned globalIn — and neither was sufficient alone. Collapsing that to one:

NewVoter(...) *Voter             // constructs and runs
(*Voter) Done() <-chan error     // why it stopped, once it has finished
(*Voter) VoterState() VoterState[ID]
// shutdown: close(globalIn)

Each of your three dissolves rather than needing machinery:

• The Start/Stop race — Start is gone; NewVoter runs the voter, so there's no window to race into. That's why refactor(pkg/finality-grandpa): make closing globalIn the voter's shutdown signal #4852 needs neither the runState CAS nor the wg-token arbitration this PR adds: there's nothing left to police.
• The forwarder leak — stopChan is gone. The poll loop has no early exit, so it can only leave by observing globalIn's close, which means first draining whatever the forwarder holds. It releases itself.
• Idempotency — Wait doesn't exist; Done() is a channel, and teardown runs once on the voter's own goroutine before publishing.

Where your instinct was right and I was initially wrong. I argued wakerChan.close() was unnecessary. True for globalIn — but only because removing stopChan created the drain guarantee, and it made me miss that forwarders were leaking far worse elsewhere. Attributing live forwarders by item type over 10 rounds:

leaking	per round	parked on
timers	3	send
roundData.Incoming	1	receive
globalIn	0	—

Four goroutines per round, forever, in a running node. The timer case is the interesting one: it wrapped an unbuffered channel in a wakerChan purely to deliver a wake, but nothing anywhere reads a timer's out — consumers only use SetWaker and Elapsed. So every timer that fired stranded its forwarder on a send with no receiver that would ever exist, and Close() couldn't reach it, for exactly the reason you gave: closing an input never releases a goroutine blocked on a send. #4852 removes the wakerChan from timer entirely and closes round inputs from Concluded, pinned by a test at +0 over 8 rounds against +32 without.

Right diagnosis, and the mechanism you reached for was needed — just for the channels that weren't in scope here.

[dimartiro]

@timwu20 thanks for your time addressing this totally agree — closing this in favour of #4852, will review it there