[liran-funaro]

Type of change

• Bug fix
• Test update

Description

• BFTCensorshipMonitor.Monitor() checks a new isStopped() (backed by an atomic stopFlag) before launching header receivers and before censorship detection, so it can no longer start header receivers that are never stopped, or report ErrCensorship after Stop().
• launchHeaderReceivers() repeats the check under the mutex Stop() takes, which closes the window where Stop() runs between the check in Monitor() and the launch.
• BFTDeliverer.FetchBlocks cancels a just-connected stream when Stop() ran after its DoneC check, instead of leaving a block receiver nothing will stop.
• The BFT deliverer test fake now ends each deliver stream only when its own connection context is cancelled, like gRPC. Previously, all streams shared one fake whose CloseSend pushed an error onto the shared response channel, so a late-closing old stream could fail the stream that replaced it. The CloseSendStub = nil workarounds and the deliverClientDoneC escape hatch are removed.
• New TestBFTCensorshipMonitor_StopBeforeMonitor covers the [blocksprovider] BFTCensorshipMonitor launches header receivers after Stop() #155 regression deterministically: it calls Stop() before Monitor() and checks that no source is connected. TestBFTCensorshipMonitor_NoSources no longer calls Stop(), which raced with the failure it asserts.

Additional details (Optional)

Ran all TestBFTDeliverer_* and TestBFTCensorshipMonitor_* tests 100 times each with -race -cpu 1,2,4 and no failures. Before the fix, a similar run reproduced the #155 panic; #128 did not reproduce locally, so its fix rests on the CI log.

Related issues

• resolves [flaky test] TestBFTDeliverer_CensorshipMonitorEvents/repeated_censorship_events,_with_exponential_backoff #128
• resolves [blocksprovider] BFTCensorshipMonitor launches header receivers after Stop() #155

[coveralls]

coverage: 82.169% (-0.02%) from 82.188% — liran-funaro:fix-flaky-bft-deliverer-tests into hyperledger:main

[tock-ibm]

Automated review (high effort). Inline findings below, plus three that sit on lines outside the diff:

1. common/deliverclient/blocksprovider/bft_censorship_monitor.go:365: header receivers leak their stream context and gRPC connection. launchHeaderReceivers throws away the clientCloser returned by Connect (// TODO use the clientCloser function), and BFTHeaderReceiver.Stop() only calls CloseSend(). With real gRPC, CloseSend does not unblock Recv (this PR's new fake now models that correctly). So after BFTCensorshipMonitor.Stop(), which runs on every block-source switch, each DeliverHeaders goroutine stays blocked in Recv, and its ctx and ClientConn are never released. This repeats for every monitor generation. Suggest keeping the closer in the tracker or receiver and calling it from Stop().

2. common/deliverclient/blocksprovider/bft_censorship_monitor.go:172: after Stop(), the select in Monitor() picks at random between the closed stopCh and a timer that may also have fired. When the timer wins, detectBlockCensorship() runs after the stop and can emit ErrCensorship instead of ErrStopping, and the loop goes around again into launchHeaderReceivers. The new guard depends on this loop eventually choosing stopCh. A non-blocking check of stopCh at the top of the loop (or before detectBlockCensorship) would make shutdown deterministic.

3. common/deliverclient/blocksprovider/deliverer_test.go:113 (altitude): the CFT deliverer suite still has the same pattern this PR removes from the BFT suite: one shared fakeDeliverClient whose CloseSendStub pushes onto a shared recvStep (and again at about line 332). An old stream's teardown can still wake the stream that replaced it. Moving perStreamDeliverStreamer and perStreamDeliverClient into a shared test helper and using it in both suites would fix the root cause once.

[tock-ibm]

Stop race in BFTDeliverer is still hidden by deliverClientDoneC. Every stream now unblocks when its own ctx is cancelled, so this deliverClientDoneC case is only needed when a BlockReceiver is never stopped. That can happen in BFTDeliverer.FetchBlocks. DoneC is checked only at the top of the loop, so if Stop() runs after Connect (bft_deliverer.go:348) but before d.blockReceiver = blockRcv (line 368), Stop() stops the previous receiver. ProcessIncoming on the new one then waits on stopC, recvC and Refreshed indefinitely. With real gRPC, that goroutine and its connection stay open until the orderer sends something. In this test, s.stop() closing deliverClientDoneC is what makes Recv return (nil, nil) and lets it out. Suggest re-checking stopFlag/DoneC under d.mutex when assigning d.blockReceiver and calling cancel() if already stopped.

[liran-funaro]

Fixed in bb3c7f1. FetchBlocks now re-checks stopFlag under d.mutex before assigning d.blockReceiver; if Stop() already ran, it calls cancel() and reports ErrStopping. With that, the deliverClientDoneC case is no longer needed, so I removed it from the fake and from stop(). Every stream now ends only through its own context.

[tock-ibm]

Shutdown still depends on a random select. Returning nil here sends Monitor() back into its select, where a closed stopCh and an already-fired time.After are both ready. Go picks one at random, so after Stop() the loop can still call detectBlockCensorship() and send ErrCensorship on errorCh (buffered, size 1) instead of ErrStopping. It then returns without ever reporting the stop. This guard prevents the leaked receivers from #155, but stopping is still not deterministic. Checking stopCh non-blockingly in the Monitor() loop, before launching and before detecting, would cover both.

[liran-funaro]

Fixed in bb3c7f1. Monitor() now waits on the select, then checks stopCh non-blockingly before calling detectBlockCensorship(), so a stop always reports ErrStopping. The guard in launchHeaderReceivers() stays for the pass that is already running when Stop() is called.

[liran-funaro]

Correction to my previous reply: the random-select premise doesn't hold. time.After(...) creates a new timer on every pass, so it is never ready at the moment select starts. If stopCh is already closed, that case always wins. (I checked: against a 10ms timer, the closed channel won 100,000/100,000 times.) The only real window is the timer firing first, then Stop() running before detectBlockCensorship(). That is an ordering race, and a second select only narrows it. In c9d1ed4 I restored the original select and made detectBlockCensorship() return false when stopFlag is set. It already holds m.mutex, so the check is race-free, and the next pass then reports ErrStopping.

[tock-ibm]

Simplification and ordering. This guard comes after the no endpoints check, so a stopped monitor whose endpoints are empty still returns an error, and Monitor() then emits ErrFatal after Stop(). In practice fetchSources never changes, so this is minor. A cleaner fix is to put the stop check first, or to move it into the Monitor() loop as a select { case <-m.stopCh: ...; default: } before calling launchHeaderReceivers. That keeps the stop handling in one place and avoids a nil return that callers can't tell apart from a successful launch.

[liran-funaro]

Kept the order on purpose: I tried putting the stop check first, and it breaks TestBFTCensorshipMonitor_NoSources, which expects no endpoints even though the test calls Stop() right away. The nil return now has a comment saying Monitor() reports the stop itself, and the new stopCh check in Monitor() (previous thread) is what reports it.

[liran-funaro]

Done in 1e9f8ad, following this suggestion: Monitor() now owns the stop checks, using a new isStopped() backed by an atomic stopFlag. It checks before launchHeaderReceivers() (reporting ErrStopping) and before detectBlockCensorship(). I kept one check inside launchHeaderReceivers(), under the mutex: a check in Monitor() alone leaves a window where Stop() runs between the check and the launch, which would bring back the #155 leak. The check in detectBlockCensorship() is removed. TestBFTCensorshipMonitor_NoSources no longer calls Stop(), because with the top-of-loop check that call raced with the no endpoints failure the test asserts.

[tock-ibm]

No test covers this regression. The only coverage for the fix is the existing TestBFTCensorshipMonitor_FaultySourceRecovery, which caught #155 only by chance. Please add a deterministic monitor test: call Stop(), then run another launchHeaderReceivers pass (for example through an export_test hook, or by checking that ConnectCallCount does not increase after Stop()). That would stop this guard from being dropped later without anyone noticing.

[liran-funaro]

Added TestBFTCensorshipMonitor_NoLaunchAfterStop in bb3c7f1. It calls Stop(), runs one launch pass through a test-only LaunchHeaderReceivers hook in export_test.go, and checks that ConnectCallCount() stays 0. I confirmed it fails with the guard removed.

[liran-funaro]

Update: in a9b8442 I replaced the hook-based test with a black-box one, TestBFTCensorshipMonitor_StopBeforeMonitor, and deleted export_test.go. It calls Stop() before Monitor(), which is also a real ordering because the deliverer starts go Monitor() and can stop it before the goroutine runs. It then checks that the monitor reports received a stop signal and that ConnectCallCount() is 0. With the guard removed it fails with 3 connect calls.

[tock-ibm]

Conventions. The repo-root CLAUDE.md says: "github.com/pkg/errors is banned by the depguard linter for new code (legacy ported files still use it)." This new line uses errors.New from github.com/pkg/errors (the file's existing import) and replaces the previous fmt.Errorf("fake-recv-step-error"). depguard won't flag it because the import already exists. Keeping fmt.Errorf, or using github.com/cockroachdb/errors, avoids adding new pkg/errors usage.

[liran-funaro]

Agreed, reverted to fmt.Errorf in bb3c7f1 (with a nolint:revive for unnecessary-format). Moving the file off pkg/errors is already tracked in #222.

[tock-ibm]

The fake's Recv results are silently ignored. The wrapper calls the inner fake.DeliverClient.Recv() only to record the call and then throws away what it returns. Any test that later sets setup.fakeDeliverClient.RecvReturns(...), RecvReturnsOnCall or RecvStub will see them have no effect, with no error. It is also easy to configure a RecvStub that blocks, which would deadlock here before the select. Consider recording calls with your own counter or a stub-free fake, or adding a comment on bftDelivererTestSetup.fakeDeliverClient that Recv behaviour must be driven through recvStepC.

[liran-funaro]

Documented in bb3c7f1: the fakeDeliverClient field comment says it only records calls and Recv() must be driven through recvStepC, and perStreamDeliverClient.Recv says it ignores the fake stubs and return values. No test configures Recv stubs on it today.

[liran-funaro]

Thanks for the review. On the three points outside the diff:

1. Header receivers leak their stream context and connection (bft_censorship_monitor.go, // TODO use the clientCloser function). Agreed, this is real. It's a separate lifecycle bug from [flaky test] TestBFTDeliverer_CensorshipMonitorEvents/repeated_censorship_events,_with_exponential_backoff #128/[blocksprovider] BFTCensorshipMonitor launches header receivers after Stop() #155 and predates this PR (it comes from upstream Fabric), so I'd rather fix it in its own PR than grow this one. Proposed as a follow-up issue.
2. Random select in Monitor() after Stop(). The random choice cannot actually happen, because the time.After timer is new on every pass. The real gap was the timer firing just before Stop(). Fixed in c9d1ed4: detectBlockCensorship() returns false once stopped (details in the inline thread).
3. CFT deliverer suite has the same shared-fake pattern (deliverer_test.go). Agreed. It isn't flaking today, and that suite is Ginkgo, which drives Recv/CloseSend stubs per call (e.g. the burst test around line 332), so moving it onto the per-stream fake is a larger rewrite. Proposed as a follow-up issue as well.

[liran-funaro]

Closing without merging. Nothing in the Fabric-X repos uses common/deliverclient: the orderer has its own copy and the committer has its own implementation. So this package should be removed rather than fixed; see #236. Removing it also resolves #128 and #155.

The bugs found here are still in the orderer's copy, which does run; they are filed as hyperledger/fabric-x-orderer#1372, which refers back to this PR's fixes.