[management] check stream start time for connecting peer#5267
Conversation
📝 WalkthroughWalkthroughThis PR threads a synchronization timestamp parameter ( Changes
Sequence Diagram(s)sequenceDiagram
participant GRPCServer as gRPC Server
participant AccountMgr as Account Manager
participant Peer as Peer Handler
participant Store as Store/Transaction
GRPCServer->>GRPCServer: Capture reqStart timestamp
GRPCServer->>AccountMgr: SyncAndMarkPeer(ctx, ..., reqStart)
AccountMgr->>Peer: MarkPeerConnected(ctx, ..., reqStart)
Peer->>Store: Begin transaction
Store-->>Peer: Transaction handle
Peer->>Peer: Check if peer.lastSeen >= syncTime
alt Peer is newer than syncTime
Peer->>Peer: Skip update, log trace
Peer-->>AccountMgr: Return early
else Peer needs update
Peer->>Peer: updatePeerStatusAndLocation(...)
Peer->>Store: Update peer with LastSeen=syncTime
Store-->>Peer: Updated
Peer->>Store: Commit transaction
Store-->>Peer: Success
Peer-->>AccountMgr: Return updated peer
end
AccountMgr-->>GRPCServer: Return peer, netMap, etc.
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug related to checking stream start time for connecting peers. It adds a syncTime parameter to track when sync requests start, preventing stale connection updates from overwriting newer peer activity. This addresses race conditions where delayed or blocked goroutines could incorrectly update peer connection states.
Changes:
- Added
syncTimeparameter toMarkPeerConnectedandSyncAndMarkPeerfunctions to enable stale request detection - Implemented logic to skip connection updates when the provided
syncTimeis not after the peer's currentLastSeentimestamp - Updated gRPC server to use request start time (
reqStart) consistently as the sync time throughout the request lifecycle
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| management/server/peer.go | Added syncTime parameter to MarkPeerConnected and updatePeerStatusAndLocation; implemented stale request detection logic |
| management/server/account/manager.go | Updated interface signatures to include syncTime parameter |
| management/server/account.go | Updated SyncAndMarkPeer to pass syncTime; OnPeerDisconnected uses time.Now().UTC() |
| management/internals/shared/grpc/server.go | Changed to use reqStart consistently as syncTime instead of creating new timestamps |
| management/server/account_test.go | Updated existing tests and added new test case for stale connect protection |
| management/server/mock_server/account_mock.go | Updated mock signatures (with critical bugs in implementation) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (am *MockAccountManager) MarkPeerConnected(ctx context.Context, peerKey string, connected bool, realIP net.IP, accountID string, syncTime time.Time) error { | ||
| if am.MarkPeerConnectedFunc != nil { | ||
| return am.MarkPeerConnectedFunc(ctx, peerKey, connected, realIP) | ||
| return am.MarkPeerConnectedFunc(ctx, peerKey, connected, realIP, syncTime) |
There was a problem hiding this comment.
The call to MarkPeerConnectedFunc is missing the accountID parameter. The function signature requires (ctx, peerKey, connected, realIP, accountID, syncTime) but only (ctx, peerKey, connected, realIP, syncTime) is being passed. This will cause a compilation error or runtime panic if this mock function is used.
| return am.MarkPeerConnectedFunc(ctx, peerKey, connected, realIP, syncTime) | |
| return am.MarkPeerConnectedFunc(ctx, peerKey, connected, realIP, accountID, syncTime) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@management/server/peer.go`:
- Around line 120-132: The disconnect path can regress peer state because
updatePeerStatusAndLocation always sets LastSeen to syncTime; change the guard
so when handling a disconnect (connected==false) you skip the update if syncTime
is older than the current peer.Status.LastSeen (i.e. if
syncTime.Before(peer.Status.LastSeen) do not call updatePeerStatusAndLocation),
but still allow updates when syncTime.Equal(peer.Status.LastSeen) for the same
stream; apply the same check wherever the symmetric disconnect block exists (the
other occurrence around lines referenced in the review) to ensure LastSeen and
Connected are not overwritten by stale disconnect events.
🧹 Nitpick comments (1)
management/server/mock_server/account_mock.go (1)
40-41: ExposeaccountIDinMarkPeerConnectedFuncto mirror the interface.
MockAccountManager.MarkPeerConnectedreceivesaccountIDbut drops it when delegating, which limits test assertions. Since the signature changed here, consider passing it through.♻️ Suggested change
- MarkPeerConnectedFunc func(ctx context.Context, peerKey string, connected bool, realIP net.IP, syncTime time.Time) error + MarkPeerConnectedFunc func(ctx context.Context, peerKey string, connected bool, realIP net.IP, accountID string, syncTime time.Time) error- return am.MarkPeerConnectedFunc(ctx, peerKey, connected, realIP, syncTime) + return am.MarkPeerConnectedFunc(ctx, peerKey, connected, realIP, accountID, syncTime)Also applies to: 325-328
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
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