Skip to content

fix: goroutine leaks#2206

Merged
nimrod-teich merged 2 commits into
mainfrom
fix-goroutine-leaks
Jan 29, 2026
Merged

fix: goroutine leaks#2206
nimrod-teich merged 2 commits into
mainfrom
fix-goroutine-leaks

Conversation

@NadavLevi

Copy link
Copy Markdown
Contributor

Description

Closes: #XXXX


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • read the contribution guide
  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the main branch
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

@codecov

codecov Bot commented Jan 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.41379% with 8 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
protocol/lavasession/consumer_types.go 72.41% 8 Missing ⚠️
Flag Coverage Δ
consensus 8.55% <ø> (ø)
protocol 33.85% <72.41%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
protocol/lavasession/consumer_types.go 71.92% <72.41%> (+0.58%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Jan 28, 2026

Copy link
Copy Markdown

Test Results

    7 files  ±0     85 suites  ±0   30m 54s ⏱️ -30s
3 337 tests +3  3 336 ✅ +3  1 💤 ±0  0 ❌ ±0 
3 530 runs  +3  3 529 ✅ +3  1 💤 ±0  0 ❌ ±0 

Results for commit a3ee2d6. ± Comparison against base commit 8a8e946.

♻️ This comment has been updated with latest results.

@NadavLevi NadavLevi force-pushed the fix-goroutine-leaks branch from d82099a to ea5a923 Compare January 28, 2026 14:58
@NadavLevi NadavLevi requested a review from AnnaR-prog January 29, 2026 09:32
@NadavLevi NadavLevi force-pushed the fix-goroutine-leaks branch 4 times, most recently from ad28dff to 04b9a74 Compare January 29, 2026 10:38
Replace polling goroutine with context-aware WaitForStateChange loop:
- Use conn.WaitForStateChange(ctx, state) to wait for connection readiness
- Return connectCtx.Err() and close connection on timeout/cancel
- No goroutine spawned, eliminating leak risk entirely

Add goleak-based regression tests that verify:
- No goroutines leaked on timeout
- Prompt return on context cancellation
- Successful connections still work correctly
Add sync.RWMutex to Endpoint struct to protect concurrent access to
Connections, ConnectionRefusals, and Enabled fields.

Use explicit lock scopes to avoid holding lock during network calls:
- Lock for cleanup and existing connection selection
- Explicit unlock before network dial (not deferred across dial)
- Re-lock with defer for state updates after dial

This pattern is safer than Lock+defer Unlock with manual unlock/lock
around the dial, which risks double-unlock panics if control flow changes.
@NadavLevi NadavLevi force-pushed the fix-goroutine-leaks branch from 04b9a74 to a3ee2d6 Compare January 29, 2026 10:38
@nimrod-teich nimrod-teich merged commit 4b0baf6 into main Jan 29, 2026
31 checks passed
@nimrod-teich nimrod-teich deleted the fix-goroutine-leaks branch January 29, 2026 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants