[management] fix set disconnected status for connected peer

[crn4]

Describe your changes

Issue ticket number and link

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)

bug fix

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
  • Improved peer disconnection to avoid false disconnects by checking recent peer activity relative to a connection stream start, reducing race conditions during reconnections and improving status accuracy.
• Tests
  • Added a test validating that peers remain connected if they show activity newer than the stream start, and are disconnected otherwise.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request threads a new streamStartTime value from gRPC stream initialization through sync and cleanup paths, and updates account disconnection logic to skip disconnects if the peer's LastSeen is more recent than that stream start time.

Changes

Cohort / File(s)	Summary
gRPC Server Internals management/internals/shared/grpc/server.go	Introduce streamStartTime at Sync start; propagate to handleUpdates, sendUpdate, cancelPeerRoutines, and cancelPeerRoutinesWithoutLock; use it in error-triggered cleanup and cancellation paths.
Account Manager Interface & Implementation management/server/account/manager.go, management/server/account.go	Change OnPeerDisconnected signature to accept streamStartTime time.Time; implementation now fetches peer, and skips disconnection when peer.Status.LastSeen is after streamStartTime.
Mocks management/server/mock_server/account_mock.go	Update mock OnPeerDisconnected signature to accept streamStartTime and replace panic placeholder with return nil.
Tests management/server/account_test.go	Add TestDefaultAccountManager_OnPeerDisconnected_LastSeenCheck validating disconnection vs. skip behavior depending on streamStartTime relative to peer LastSeen.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant GRPC_Server as gRPC Server
  participant SyncHandler as Sync/handleUpdates
  participant AccountMgr as AccountManager
  participant Store as Peer Store

  Client->>GRPC_Server: open stream (records streamStartTime)
  GRPC_Server->>SyncHandler: start sync (streamStartTime)
  SyncHandler->>SyncHandler: process updates
  alt initial-sync error or disconnect condition
    SyncHandler->>GRPC_Server: trigger cleanup (streamStartTime)
    GRPC_Server->>AccountMgr: OnPeerDisconnected(ctx, accountID, peerPubKey, streamStartTime)
    AccountMgr->>Store: GetPeerByPeerPubKey(peerPubKey)
    Store-->>AccountMgr: peer (with LastSeen)
    alt peer.Status.LastSeen <= streamStartTime
      AccountMgr->>Store: mark peer disconnected
    else peer.Status.LastSeen > streamStartTime
      AccountMgr-->>GRPC_Server: skip disconnect (recent activity)
    end
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• [management] Fix peer deletion error handling #5188 — Touches Sync error handling in management/internals/shared/grpc/server.go and is closely related to changes that thread stream start handling.
• [management] fix ephemeral peers being not removed #5203 — Adds per-peer cleanup in management/internals/shared/grpc/server.go; closely related because this PR changes cleanup signatures and behavior.

Suggested reviewers

• mlsmaycon
• pascal-fischer

Poem

🐰 I hopped in with a time so fine,
Marked the stream's bright starting line.
If a peer's last wink came after the start,
I leave them be — no false depart.
Hooray for tidy disconnection, on time! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is largely incomplete, missing detailed explanation of changes, related issue, and proper justification for why documentation is not needed.	Add a detailed 'Describe your changes' section explaining the zombie stream protection logic, reference the related issue ticket, and provide specific reasoning for why docs aren't needed.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main bug fix: preventing disconnection of peers with recent activity during stream cleanup.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/peer-locking

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR updates the peer disconnection handling so that peers are not incorrectly marked as disconnected when a newer connection/session is already active.

Changes:

• Extends the Manager.OnPeerDisconnected interface and its mock/implementation to accept a streamStartTime parameter.
• In the gRPC Sync flow, tracks a per-stream timestamp and threads it through the update/send/cancellation pipeline.
• In DefaultAccountManager.OnPeerDisconnected, adds a guard that checks the peer’s LastSeen against the stream’s start time before marking the peer as disconnected.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
management/server/mock_server/account_mock.go	Updates the mock account manager to implement the new OnPeerDisconnected signature with a no-op body so tests can compile against the new interface.
management/server/account/manager.go	Updates the Manager interface’s OnPeerDisconnected signature to include the streamStartTime parameter used for race-avoidance logic.
management/server/account.go	Implements time-based guarding in DefaultAccountManager.OnPeerDisconnected to skip disconnect when a peer has newer activity than the current stream, and delegates disconnection to MarkPeerConnected(..., false, ...).
management/internals/shared/grpc/server.go	Captures a per-stream timestamp and passes it to handleUpdates, sendUpdate, and the peer cancellation helpers, integrating the account manager’s new disconnection semantics into the gRPC sync pipeline.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud