[PR #6200] [management] Account-scoped ephemeral peer cleanup #27748

Open
opened 2026-08-05 07:09:10 -04:00 by saavagebueno · 0 comments
Owner

Original Pull Request: https://github.com/netbirdio/netbird/pull/6200

State: closed
Merged: No


Describe your changes

Replace the per-peer linked list with a per-account map keyed by accountID. Each entry holds only the latest disconnect timestamp we have observed for that account and a single timer that fires the next sweep. Sweeps query the database for the authoritative stale set, batch the deletes through peers.Manager.DeletePeers, then drop the account from the tracker when lastDisc + lifeTime <= now (else re-arm at horizon + cleanupWindow).

The drop rule is the entire termination story: an account stays tracked only while OnPeerDisconnected keeps refreshing the timestamp. There is no internal feedback loop that can advance lastDisc on its own, so once disconnects stop the account drops in at most one sweep.

A timestamp beats the ref-counter alternative because the counter drifts positive in three real situations the cleanup loop has no signal for: peers deleted via the API while offline, peers that reconnect within the lifetime window, and management restarts. The timestamp design never claims to know the size of the stale set — it only knows the latest disconnect we observed and uses that to bound when it is safe to drop the account.

OnPeerConnected becomes a no-op. The sweep query already filters reconnected peers at the database level (peer_status_connected = false in the WHERE clause), so there is nothing the in-memory tracker needs to do on reconnect. The interface method is preserved for call-site compatibility.

LoadInitialPeers no longer runs the catch-up query synchronously. It schedules a deferred load via time.AfterFunc at a random delay between 8 and 10 minutes. Without the jitter, every management replica in a fleet-wide deploy would issue the catch-up query simultaneously. The catch-up itself is one GROUP BY against the peers table:

  SELECT account_id, MAX(peer_status_last_seen)
  FROM peers
  WHERE ephemeral = true AND peer_status_connected = false
  GROUP BY account_id

For each row the tracker seeds an entry and arms a sweep at max(now, last_seen + lifeTime) + cleanupWindow — so accounts whose backlog is already stale get cleaned soon after the delay elapses, and accounts that disconnected recently wait the remaining window. OnPeerDisconnected calls that arrive during the delay window seed the tracker live, and the catch-up query skips accounts that are already tracked.

Stop() cancels both the deferred initial-load timer and every per-account sweep timer, and flips a stopped flag so subsequent OnPeerDisconnected calls are ignored. This makes restarts and test teardown clean.

Two new store methods:
GetStaleEphemeralPeerIDsForAccount(ctx, accountID, olderThan)
GetEphemeralAccountsLastDisconnect(ctx)
Both are scoped, indexable queries that the existing peers table supports without schema changes.

The pending metric is renamed from
management.ephemeral.peers.pending to
management.ephemeral.accounts.tracked to reflect the new semantics (it now counts accounts on the cleanup list, not peers). Method names on the metrics type are unchanged so no production call site has to move. No new metric labels, no per-account cardinality.

The algorithm was validated against an in-memory SQLite peers table through an 11-scenario prototype kept under proto/, including pathological-churn and 4-hour randomized simulations. All scenarios terminate; max observed per-account sweep rate stays bounded near the lifeTime + cleanupWindow cadence even under sustained disconnect churn.

Verification: go build, go vet, race-clean tests across the ephemeral, store, and telemetry packages, plus a clean golangci-lint pass on the touched packages.

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)

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)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

**Original Pull Request:** https://github.com/netbirdio/netbird/pull/6200 **State:** closed **Merged:** No --- ## Describe your changes Replace the per-peer linked list with a per-account map keyed by accountID. Each entry holds only the latest disconnect timestamp we have observed for that account and a single timer that fires the next sweep. Sweeps query the database for the authoritative stale set, batch the deletes through peers.Manager.DeletePeers, then drop the account from the tracker when lastDisc + lifeTime <= now (else re-arm at horizon + cleanupWindow). The drop rule is the entire termination story: an account stays tracked only while OnPeerDisconnected keeps refreshing the timestamp. There is no internal feedback loop that can advance lastDisc on its own, so once disconnects stop the account drops in at most one sweep. A timestamp beats the ref-counter alternative because the counter drifts positive in three real situations the cleanup loop has no signal for: peers deleted via the API while offline, peers that reconnect within the lifetime window, and management restarts. The timestamp design never claims to know the size of the stale set — it only knows the latest disconnect we observed and uses that to bound when it is safe to drop the account. OnPeerConnected becomes a no-op. The sweep query already filters reconnected peers at the database level (peer_status_connected = false in the WHERE clause), so there is nothing the in-memory tracker needs to do on reconnect. The interface method is preserved for call-site compatibility. LoadInitialPeers no longer runs the catch-up query synchronously. It schedules a deferred load via time.AfterFunc at a random delay between 8 and 10 minutes. Without the jitter, every management replica in a fleet-wide deploy would issue the catch-up query simultaneously. The catch-up itself is one GROUP BY against the peers table: ```sql SELECT account_id, MAX(peer_status_last_seen) FROM peers WHERE ephemeral = true AND peer_status_connected = false GROUP BY account_id ``` For each row the tracker seeds an entry and arms a sweep at max(now, last_seen + lifeTime) + cleanupWindow — so accounts whose backlog is already stale get cleaned soon after the delay elapses, and accounts that disconnected recently wait the remaining window. OnPeerDisconnected calls that arrive during the delay window seed the tracker live, and the catch-up query skips accounts that are already tracked. Stop() cancels both the deferred initial-load timer and every per-account sweep timer, and flips a stopped flag so subsequent OnPeerDisconnected calls are ignored. This makes restarts and test teardown clean. Two new store methods: GetStaleEphemeralPeerIDsForAccount(ctx, accountID, olderThan) GetEphemeralAccountsLastDisconnect(ctx) Both are scoped, indexable queries that the existing peers table supports without schema changes. The pending metric is renamed from management.ephemeral.peers.pending to management.ephemeral.accounts.tracked to reflect the new semantics (it now counts accounts on the cleanup list, not peers). Method names on the metrics type are unchanged so no production call site has to move. No new metric labels, no per-account cardinality. The algorithm was validated against an in-memory SQLite peers table through an 11-scenario prototype kept under proto/, including pathological-churn and 4-hour randomized simulations. All scenarios terminate; max observed per-account sweep rate stays bounded near the lifeTime + cleanupWindow cadence even under sustained disconnect churn. Verification: go build, go vet, race-clean tests across the ephemeral, store, and telemetry packages, plus a clean golangci-lint pass on the touched packages. ## Issue ticket number and link ## Stack <!-- branch-stack --> ### Checklist - [ ] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [x] It is a refactor - [ ] Created tests that fail without the change (if possible) > 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 - [ ] Documentation is **not needed** for this change (explain why) ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: https://github.com/netbirdio/docs/pull/__
saavagebueno added the pull-request label 2026-08-05 07:09:10 -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#27748