Skip to content

fix(fabricx): stop leaking the finality goroutine on broadcast error - #2581

Merged
Effi-S merged 1 commit into
mainfrom
2559
Nov 1, 2026
Merged

Effi-S merged 1 commit into
mainfrom
2559

Conversation

@AkramBitar

Copy link
Copy Markdown
Contributor

Broadcast started a finality poller that sent on an unbuffered channel. When the ordering call failed, the function returned without reading it — so the poller blocked forever on its send. One goroutine leaked permanently per failed submit.

Fix: buffer the channel so the send always completes, and cancel the poller's context on return so it stops instead of burning its full retry budget unheard.

Also: getFinality takes a context, passes it to IsFinal, and waits on select over ctx.Done()/timer instead of a blind time.Sleep. Broadcast logic moved into broadcastAndWaitForFinality behind a small ordering interface so it is testable without a real network service. Exported interfaces unchanged.

Tests: two goleak regression tests — both fail against the old code with goroutines parked in the poller — plus the retry matrix and cancellation cases. make checks clean, green under -race.

Note: make lint reports 6 pre-existing revive issues in token/services/storage/db/, unrelated to this change (they reproduce on main).

Fixes #2559

🤖 Generated with Claude Code

@AkramBitar AkramBitar added this to the Q4/26 milestone Oct 29, 2026
@AkramBitar AkramBitar self-assigned this Oct 29, 2026
@AkramBitar
AkramBitar requested review from Effi-S and adecaro October 29, 2026 12:44

@Effi-S Effi-S left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

In `fnsBroadcaster.Broadcast`, the finality channel was unbuffered and the
polling goroutine ran `final <- getFinality(...)`. When
`fns.Ordering().Broadcast(...)` failed, the function returned without ever
reading `<-final`, so the goroutine blocked forever on its send and leaked
permanently. Repeated submit failures accumulated one leaked goroutine each,
along with up to 10s of pointless finality polling per leak.

Buffer the finality channel so the send can always complete, and derive a
cancellable context that is cancelled on return so an orphaned poller stops
promptly instead of running out its retry budget with nobody listening.
`getFinality` now takes that context, passes it to `IsFinal`, and waits on a
`select` over `ctx.Done()` and the retry timer rather than a blind
`time.Sleep`; it also skips the trailing sleep after the final attempt.

The broadcast logic moves into `broadcastAndWaitForFinality`, behind a small
consumer-side `ordering` interface, so it is unit-testable without a full
`fabric.NetworkServiceProvider`. The exported `EnvelopeBroadcaster` and
`Submitter` interfaces are unchanged, so there is no ripple into callers.

Tests cover the `getFinality` retry matrix, early cancellation, and the
success and finality-error paths, plus two `goleak` regression tests for the
broadcast-error path. Both fail against the previous implementation with
goroutines parked in the finality poller.

Fixes #2559

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Akram Bitar <akram@il.ibm.com>
@Effi-S
Effi-S merged commit 4290c97 into main Nov 1, 2026
221 checks passed
@Effi-S
Effi-S deleted the 2559 branch November 1, 2026 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Goroutine leak on early Broadcast error (unbuffered finality channel)

2 participants