[PR #6663] [MERGED] [client] Fix backoff.Ticker goroutine leak in reconnect guard #28719

Open
opened 2026-08-05 08:06:49 -04:00 by saavagebueno · 0 comments
Owner

📋 Pull Request Information

Original PR: https://github.com/netbirdio/netbird/pull/6663
Author: @theodorsm
Created: 7/3/2026
Status: Merged
Merged: 7/3/2026
Merged by: @theodorsm

Base: mainHead: backoff-leak


📝 Commits (1)

  • 9fdf564 Fix ticker defer stop and add test

📊 Changes

2 files changed (+97 additions, -1 deletions)

View changed files

📝 client/internal/peer/guard/guard.go (+5 -1)
client/internal/peer/guard/guard_leak_test.go (+92 -0)

📄 Description

Describe your changes

During testing with high number of peers (6000) we observed that orphaned backoff.Ticker goroutines accumulate after teardown of the engine:

image

This PR ensures that the ticker goroutine is closed. We prevously relied on the context to close the ticker goroutine after we replace the inital ticker upon reconnect, but backoff.WithContext does not close with a Context.Done while it's blocked in Ticker.send. A regression test is also added in this PR.

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • This change does not modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — OR I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See CONTRIBUTING.md.

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Docs PR URL (required if "docs added" is checked)

Summary by CodeRabbit

  • Bug Fixes

    • Improved shutdown handling to ensure reconnect retries are stopped reliably and do not leave background work behind.
  • Tests

    • Added a regression test that verifies reconnect-related goroutines exit cleanly during shutdown and do not leak.

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/netbirdio/netbird/pull/6663 **Author:** [@theodorsm](https://github.com/theodorsm) **Created:** 7/3/2026 **Status:** ✅ Merged **Merged:** 7/3/2026 **Merged by:** [@theodorsm](https://github.com/theodorsm) **Base:** `main` ← **Head:** `backoff-leak` --- ### 📝 Commits (1) - [`9fdf564`](https://github.com/netbirdio/netbird/commit/9fdf564e6353aaf8e87963b628c1d40777fef2c5) Fix ticker defer stop and add test ### 📊 Changes **2 files changed** (+97 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `client/internal/peer/guard/guard.go` (+5 -1) ➕ `client/internal/peer/guard/guard_leak_test.go` (+92 -0) </details> ### 📄 Description ## Describe your changes During testing with high number of peers (6000) we observed that orphaned `backoff.Ticker` goroutines accumulate after teardown of the engine: <img width="1887" height="232" alt="image" src="https://github.com/user-attachments/assets/9b234bb8-24d1-459e-9b8e-fed7010dabc0" /> This PR ensures that the ticker goroutine is closed. We prevously relied on the context to close the ticker goroutine after we replace the inital ticker upon reconnect, but `backoff.WithContext` does not close with a `Context.Done` while it's blocked in `Ticker.send`. A regression test is also added in this PR. ## Issue ticket number and link ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) - [x] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature — **OR** I have discussed it with the NetBird team beforehand (link the issue / Slack thread in the description). See [CONTRIBUTING.md](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTING.md#discuss-changes-with-the-netbird-team-first). > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved shutdown handling to ensure reconnect retries are stopped reliably and do not leave background work behind. * **Tests** * Added a regression test that verifies reconnect-related goroutines exit cleanly during shutdown and do not leak. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
saavagebueno added the pull-request label 2026-08-05 08:06:49 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#28719