[PR #6193] [management] Fence peer status updates with a session token #27728

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

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

State: closed
Merged: Yes


Describe your changes

The connect/disconnect path used a best-effort LastSeen-after-streamStart comparison to decide whether a status update should land. Under contention — a re-sync arriving while the previous stream's disconnect was still in flight, or two management replicas seeing the same peer at once — the check was a read-then-decide-then-write window: any UPDATE in between caused the wrong row to be written. The Go-side time.Now() that fed the comparison also drifted under lock contention, since it was captured seconds before the write actually committed.

Replace it with an integer-nanosecond fencing token stored alongside the status. Every gRPC sync stream uses its open time (UnixNano) as its token. Connects only land when the incoming token is strictly greater than the stored one; disconnects only land when the incoming token equals the stored one (i.e. we're the stream that owns the current session). Both are single optimistic-locked UPDATEs — no read-then-write, no transaction wrapper.

The new contract:

Operation Wins when
MarkPeerConnected incoming token > stored token (strict)
MarkPeerDisconnected incoming token == stored token (exact)

Both are implemented as a single UPDATE … WHERE peer_status_session_started_at <cond>
on the peers table. No transactions, no read-then-write, no
serialization race.

LastSeen is now written by the database itself (CURRENT_TIMESTAMP). The caller never supplies it, so the value always reflects the real moment of the UPDATE rather than the moment the caller queued the work — which was already off by minutes under heavy lock contention.

Side effects (geo lookup, peer-login-expiration scheduling, network-map fan-out) are explicitly documented as running after the fence UPDATE commits, never inside it. Geo also skips the update when realIP equals the stored ConnectionIP, dropping a redundant SavePeerLocation call on same-IP reconnects.

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/__

Summary by CodeRabbit

  • Bug Fixes

    • Prevented stale/older connect/disconnect events from overwriting newer sessions by introducing session-token fencing and atomic checks.
    • Ignored disconnects that don’t match the active session to avoid premature disconnects.
  • Refactor

    • Reworked peer connection lifecycle to use session-based optimistic fencing and simplified location/login expiration handling.
  • Tests

    • Updated and strengthened tests for session-token semantics and concurrent connect/disconnect races.

Review Change Stack

**Original Pull Request:** https://github.com/netbirdio/netbird/pull/6193 **State:** closed **Merged:** Yes --- ## Describe your changes The connect/disconnect path used a best-effort LastSeen-after-streamStart comparison to decide whether a status update should land. Under contention — a re-sync arriving while the previous stream's disconnect was still in flight, or two management replicas seeing the same peer at once — the check was a read-then-decide-then-write window: any UPDATE in between caused the wrong row to be written. The Go-side time.Now() that fed the comparison also drifted under lock contention, since it was captured seconds before the write actually committed. Replace it with an integer-nanosecond fencing token stored alongside the status. Every gRPC sync stream uses its open time (UnixNano) as its token. Connects only land when the incoming token is strictly greater than the stored one; disconnects only land when the incoming token equals the stored one (i.e. we're the stream that owns the current session). Both are single optimistic-locked UPDATEs — no read-then-write, no transaction wrapper. The new contract: | Operation | Wins when | |---|---| | `MarkPeerConnected` | incoming token > stored token (strict) | | `MarkPeerDisconnected` | incoming token == stored token (exact) | Both are implemented as a single `UPDATE … WHERE peer_status_session_started_at <cond>` on the `peers` table. No transactions, no read-then-write, no serialization race. LastSeen is now written by the database itself (CURRENT_TIMESTAMP). The caller never supplies it, so the value always reflects the real moment of the UPDATE rather than the moment the caller queued the work — which was already off by minutes under heavy lock contention. Side effects (geo lookup, peer-login-expiration scheduling, network-map fan-out) are explicitly documented as running after the fence UPDATE commits, never inside it. Geo also skips the update when realIP equals the stored ConnectionIP, dropping a redundant SavePeerLocation call on same-IP reconnects. ## Issue ticket number and link ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [x] 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](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) Paste the PR link from https://github.com/netbirdio/docs here: https://github.com/netbirdio/docs/pull/__ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Prevented stale/older connect/disconnect events from overwriting newer sessions by introducing session-token fencing and atomic checks. * Ignored disconnects that don’t match the active session to avoid premature disconnects. * **Refactor** * Reworked peer connection lifecycle to use session-based optimistic fencing and simplified location/login expiration handling. * **Tests** * Updated and strengthened tests for session-token semantics and concurrent connect/disconnect races. <!-- review_stack_entry_start --> [![Review Change Stack](https://storage.googleapis.com/coderabbit_public_assets/review-stack-in-coderabbit-ui.svg)](https://app.coderabbit.ai/change-stack/netbirdio/netbird/pull/6193?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
saavagebueno added the pull-request label 2026-08-05 07:09:09 -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#27728