@claude review

Claude finished @cffls's task in 1m 24s —— View job

Code Review: Fix goroutine leaks in witness request path

• Gather context and read changed files
• Review eth/protocols/eth/dispatcher.go changes
• Review eth/peer.go changes
• Review eth/peer_test.go changes
• Post final review

Overall this is a well-motivated fix for a real goroutine leak problem. The three root causes are clearly identified and the fixes are targeted. I have a few observations, one of which is a potential correctness issue:

Issue: doWitnessRequest — double wg.Done() / semaphore release on cancel during forward

eth/peer.go:750-769 — In the goroutine spawned by doWitnessRequest, when cancel fires during the forward path (witReqResCh <- ...), the goroutine calls witReqsWg.Done() and <-witReqSem (lines 767-768). But the adapter goroutine in RequestWitnessesWithVerification (lines 383-384) also does <-witReqSem and witReqsWg.Done() for every item received from witReqResCh.

If cancel fires after the response was successfully received from witResCh but before forwarding to witReqResCh, the cancel branch at line 766 fires and does cleanup. This is correct since the message was never sent to the adapter — no double-free occurs. However, there's a subtle race: if witReqResCh <- ... and <-cancel are both ready simultaneously, Go's select picks one randomly. If the send wins, the adapter will also do Done()/semaphore release. If cancel wins, the goroutine does it. The random selection means at most one path runs, so this is actually safe. Just worth noting for reviewers.

Issue: witRes.Done <- nil can block if Done is unbuffered

eth/peer.go:760-762 — After receiving the wit response, the code sends witRes.Done <- nil. If the upstream wit.Response.Done channel happens to be unbuffered and nobody is reading the other end, this will block and the goroutine won't exit even when cancel fires. The code does check witRes.Done != nil but doesn't check if cancel is needed here too:

if witRes != nil && witRes.Done != nil {
    witRes.Done <- nil  // can block indefinitely
}

Consider wrapping this in a select with cancel:

if witRes != nil && witRes.Done != nil {
    select {
    case witRes.Done <- nil:
    case <-cancel:
        witReqsWg.Done()
        <-witReqSem
        return
    }
}

This would make the cancellation path fully robust even if upstream misbehaves. Fix this →

Observation: Request.Close() double-close protection via select

eth/protocols/eth/dispatcher.go:79-84 — The double-close protection using select { case <-r.Cancel: default: close(r.Cancel) } is a well-known Go pattern and works correctly. Good change.

One note: the original code closed Cancel only inside the reqCancel success path (i.e., only for real peers). Now it always closes Cancel, even for real peers where the dispatcher already saw the cancellation. This is fine since the select-based guard prevents double-close panics, and it ensures the channel is always signaled regardless of path.

Observation: Buffered doneCh replacing drainer goroutines

eth/peer.go:400,430 — Replacing go func(){ <-doneCh }() with make(chan error, 1) is the correct fix. The old goroutines would block forever since nothing wrote to them. Buffered channels let the consumer write without blocking and without a drainer. Clean fix.

Observation: Adapter defers closing wit requests

eth/peer.go:370-376 — Deferring witReq.Close() for all underlying wit requests ensures cleanup on any exit path from the adapter goroutine. This is important since the concurrent fetcher only has access to the *eth.Request shim and can't close the wit-level requests directly.

Note: iterating over wrapperReq.witReqs in the defer is safe because witReqs is only appended to (under lock) by buildWitnessRequests, and by the time the adapter exits (after witReqResCh is closed → after witReqsWg.Wait() completes), all request-building is done.

Tests

The test coverage is thorough:

• TestRequestCloseShimClosesCancelChannel — covers the dispatcher fix including double-close safety
• TestDoWitnessRequestExitsOnCancel — directly verifies goroutine cleanup on cancel
• TestRequestWitnessesNoGoroutineLeakOnCancel — end-to-end test for the full flow
• TestRequestWitnessesDoneChannelBuffered — verifies the buffered channel fix

The goroutine-counting approach with slack (goroutinesBefore+2) is pragmatic for leak detection tests.

Summary

The PR correctly addresses all three root causes of goroutine leaks. The changes are well-scoped and the test coverage is good. The one actionable item is wrapping witRes.Done <- nil in a select with cancel to prevent a potential indefinite block if Done is ever unbuffered upstream.

Codecov Report

❌ Patch coverage is 70.96774% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.57%. Comparing base (2a0bfe7) to head (75d4eb8).
⚠️ Report is 1 commits behind head on develop.

❌ Your patch status has failed because the patch coverage (70.96%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

@@             Coverage Diff             @@
##           develop    #2157      +/-   ##
===========================================
+ Coverage    51.51%   51.57%   +0.05%     
===========================================
  Files          882      882              
  Lines       154140   154164      +24     
===========================================
+ Hits         79403    79504     +101     
+ Misses       69559    69494      -65     
+ Partials      5178     5166      -12     

Quality Gate failed

Failed conditions
4.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

LGTM - I would test that a node can do a graceful shutdown. We've faced issues with go routines blocking bor from doing a graceful shutdown.

Thanks - tested multiple times locally and the node was able to shutdown gracefully.