Fix panic on peer shutdown during block commit

[Ady0333]

Type of change

• Bug fix

---

Description

This PR fixes a shutdown race in GossipStateProviderImpl that could cause a peer to panic during block commit.

On shutdown, Stop() closed the ledger immediately after signaling stopCh, while the deliverPayloads() goroutine could still be committing blocks. That commit path sends events to the snapshot manager, whose channels are closed as part of ledger.Close(). If shutdown overlaps with an in-flight commit, this results in a send on closed channel panic.

The fix waits for deliverPayloads() to complete before closing ledger resources and adds a stop check in the payload loop so shutdown exits promptly.

---

Additional details

The shutdown path is now coordinated with block commit to ensure no snapshot manager channels are closed while still in use.

Tested locally with go test ./gossip/state/....

---

Release Note

Fixes a peer shutdown race that could cause a panic during block commit when
snapshot manager channels were closed while still in use.

[Ady0333]

Hi @yacovm ,

When you get a chance, please take a look at this PR.

[C0rWin]

@Ady0333 can you please add unit test?

[cendhu]

Thank you for the contribution. We currently have neither a peer node stop nor a peer node shutdown command. Because of this, I’m not sure what this PR is intended to address. Our existing shutdown APIs are only used within tests to close services after a unit test run.

[Ady0333]

@cendhu
Thanks for the clarification.

My understanding was that snapshotMgr.shutdown() can close channels while snapshot generation goroutines are still active, which could lead to a send-on-closed-channel panic. Even if shutdown is currently only used in tests, this panic could still cause non-deterministic failures in test environments where snapshots are generated concurrently.

The intent of the fix was to prevent this race and ensure shutdown happens safely even when snapshot generation is in progress.

Please let me know if this scenario is not considered relevant for Fabric, and I can close the PR if needed.

[pfi79]

@cendhu Thanks for the clarification.

My understanding was that snapshotMgr.shutdown() can close channels while snapshot generation goroutines are still active, which could lead to a send-on-closed-channel panic. Even if shutdown is currently only used in tests, this panic could still cause non-deterministic failures in test environments where snapshots are generated concurrently.

The intent of the fix was to prevent this race and ensure shutdown happens safely even when snapshot generation is in progress.

Please let me know if this scenario is not considered relevant for Fabric, and I can close the PR if needed.

I agree with @cendhu .
We need clear evidence of the importance of your correction, for example, in the form of an integration test, so that the test ends in panic without your correction.

[Ady0333]

@pfi79 Thankyou for keeping your view on this.

I agree with your point and I will add an integration test for this fix shortly.

[Atishyy27]

I traced the panic this fixes: Stop() closes stopCh and then immediately s.ledger.Close(), so a deliverPayloads inside commitBlock fails the commit and hits the Panicf at the bottom of the pop loop (state.go L576). Waiting for deliverPayloads before closing the ledger is the right ordering, and the Add(1)-before-go placement is correct.

Two suggestions:

1. The comment says "wait for all goroutines … don't send on closed channels", but the WaitGroup only tracks deliverPayloads. The senders into stateRequestCh/stateResponseCh in directMessage (L394-402) use select { case ch <- msg: case <-s.stopCh: }, and select chooses randomly when both cases are ready — so a send onto the just-closed channel is still possible after Stop(). Either extending the WaitGroup to those paths or narrowing the comment to what's actually guaranteed would avoid giving future readers false confidence.
2. A regression test — a mock ledger whose StoreBlock blocks until released, call Stop() concurrently, assert no panic and that Close() happens after the commit returns — would lock this ordering in.