Stop sleep monitor on user requested disconnect and restart on connect #4922
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds mutex-guarded sleep detection lifecycle to serviceClient: new Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
hurricanehrndz
changed the title
There was a problem hiding this comment.
Pull request overview
This PR addresses an issue where the sleep monitor continues running after a manual disconnect, which is unnecessary and wasteful. The changes ensure the sleep detection service is properly stopped when disconnecting and restarted when reconnecting.
Key Changes:
- Added lifecycle management for the sleep detection service, stopping it on disconnect and restarting on connect
- Introduced thread-safe state tracking for the sleep service with a mutex to prevent race conditions
- Refactored sleep service cleanup into a dedicated
stopSleepListener()method
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| client/ui/event_handler.go | Added calls to start/stop sleep listener on connect/disconnect events |
| client/ui/client_ui.go | Implemented sleep service lifecycle management with thread-safe state tracking and a new stop method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if s.sleepService != nil { | ||
| log.Debug("sleep detection service already initialized") | ||
| return | ||
| } |
There was a problem hiding this comment.
The early return check for existing sleep service occurs after acquiring the lock but doesn't prevent a potential race condition. If startSleepListener() is called multiple times concurrently, both calls could pass the nil check before either assigns to s.sleepService. Consider checking if s.sleepService != nil again after the sleep service is successfully created but before assignment, or document that this method should only be called from the event handler which is already serialized.
| return | ||
| } | ||
|
|
||
| s.sleepService = sleepService |
There was a problem hiding this comment.
If the sleep service creation succeeds but Start() fails, s.sleepService will be assigned but never started. This leaves the service in an inconsistent state. Consider only assigning s.sleepService after both creation and Start() succeed, and ensure proper cleanup if Start() fails.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
client/ui/event_handler.go (1)
96-98: Consider callingstartSleepListener()only on successful connection.Currently,
startSleepListener()is called afterupdateStatus()regardless of whethermenuUpClick()succeeded. If the connect operation fails or is cancelled, the sleep listener will still be started. Consider moving this call inside a success check or restructuring to ensure it's only called when the connection is actually established.if err := h.client.updateStatus(); err != nil { log.Debugf("failed to update status after connect: %v", err) + return } h.client.startSleepListener()Alternatively, check connection status before starting:
if h.client.connected { h.client.startSleepListener() }client/ui/client_ui.go (2)
1169-1197: Minor: Cleanup goroutine spawned on each successful initialization.The cleanup goroutine (lines 1193-1196) is spawned each time
startSleepListener()successfully initializes the service. If the user connects, disconnects, then reconnects, multiple goroutines will be waiting ons.ctx.Done(). While this is functionally safe sincestopSleepListener()is idempotent (has a nil check), it creates unnecessary goroutines over time.Consider moving the cleanup goroutine to be spawned only once (e.g., in
onTrayReady) or track whether it's already running.- // Cleanup on context cancellation - go func() { - <-s.ctx.Done() - s.stopSleepListener() - }()The cleanup on context cancellation could be handled in
onTrayExit()instead:func (s *serviceClient) onTrayExit() { s.stopSleepListener() s.cancel() }
1106-1108: Consider conditional initialization based on connection state.The sleep listener is started unconditionally on tray initialization. Given the new lifecycle management where the listener should only be active when connected, consider:
- Checking if the service is already connected before starting the listener, or
- Deferring initialization entirely to the connect flow
This would align the initial startup behavior with the PR's intent to tie sleep monitoring to connection state.
- // Start sleep detection listener - go s.startSleepListener() + // Start sleep detection listener if already connected + go func() { + if err := s.updateStatus(); err == nil && s.connected { + s.startSleepListener() + } + }()Alternatively, rely solely on the connect flow to start the listener, removing this initial call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/ui/client_ui.go(3 hunks)client/ui/event_handler.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/ui/client_ui.go (1)
client/internal/sleep/service.go (1)
Service(16-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: release
- GitHub Check: JS / Lint
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
🔇 Additional comments (3)
client/ui/event_handler.go (1)
110-111: LGTM!Stopping the sleep listener synchronously before launching the disconnect goroutine ensures proper cleanup ordering. The mutex-protected operation should be fast and won't significantly block the event loop.
client/ui/client_ui.go (2)
324-325: LGTM!Adding dedicated
sleepServiceandsleepLockfields provides proper lifecycle management and thread-safe access to the sleep detection service.
1199-1213: LGTM!The
stopSleepListener()implementation is well-structured:
- Properly guarded by mutex for thread safety
- Idempotent with nil check for safe repeated calls
- Clears the reference after deregistration to allow restart
- Logs errors without propagating them (appropriate for cleanup operations)
hurricanehrndz
force-pushed
the
fix/stop-sleep-monitor-on-disconnect
branch
from
59169b0 to
2ccc05e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/ui/event_handler.go (1)
70-99: Guard sleep listener restart on successful, non‑canceled connectAs written, the connect goroutine unconditionally calls
startSleepListener()aftermenuUpClick, even if:
- the connect attempt was canceled via
connectCtx(e.g., user clicked Disconnect while connecting), ormenuUpClickfailed for another reason (login/network error).In those cases,
handleDisconnectClickhas already calledstopSleepListener()to honor the user’s intent, but the connect goroutine will later restart it, which can reintroduce the “auto-reconnect on wake after user clicked Disconnect” behavior you’re trying to eliminate.You can keep the existing
updateStatus()behavior (to refresh the systray state) and only (re)start the sleep listener when the connect actually succeeded and the context wasn’t canceled:func (h *eventHandler) handleConnectClick() { h.client.mUp.Disable() if h.client.connectCancel != nil { h.client.connectCancel() } connectCtx, connectCancel := context.WithCancel(h.client.ctx) h.client.connectCancel = connectCancel go func() { defer connectCancel() - if err := h.client.menuUpClick(connectCtx); err != nil { - st, ok := status.FromError(err) - if errors.Is(err, context.Canceled) || (ok && st.Code() == codes.Canceled) { - log.Debugf("connect operation cancelled by user") - } else { - h.client.app.SendNotification(fyne.NewNotification("Error", "Failed to connect")) - log.Errorf("connect failed: %v", err) - } - } + err := h.client.menuUpClick(connectCtx) + if err != nil { + st, ok := status.FromError(err) + if errors.Is(err, context.Canceled) || (ok && st.Code() == codes.Canceled) { + log.Debugf("connect operation cancelled by user") + } else { + h.client.app.SendNotification(fyne.NewNotification("Error", "Failed to connect")) + log.Errorf("connect failed: %v", err) + } + } - if err := h.client.updateStatus(); err != nil { - log.Debugf("failed to update status after connect: %v", err) - } - - h.client.startSleepListener() + if updateErr := h.client.updateStatus(); updateErr != nil { + log.Debugf("failed to update status after connect: %v", updateErr) + } + + // Only (re)start the sleep listener if the connect attempt + // actually succeeded and wasn't canceled. + if err == nil && connectCtx.Err() == nil { + h.client.startSleepListener() + } }() }You may also want to consider re‑starting the listener in the disconnect goroutine if
menuDownClickfails andupdateStatusleavesh.client.connectedas true, so you don’t end up connected without sleep monitoring, but that’s a much smaller edge case.Also applies to: 101-127
🧹 Nitpick comments (1)
client/ui/client_ui.go (1)
1169-1198: start/stop implementation is safe; consider avoiding multiple ctx.Done watcher goroutinesThe
startSleepListener/stopSleepListenerpair is concurrency‑safe:
sleepLockserializes all access tosleepService.- The
s.sleepService != nilcheck is done under the lock, so concurrentstartSleepListener()calls are correctly idempotent.stopSleepListener()is also idempotent and re‑entrant safe w.r.t. thectx.Donewatcher.One minor refinement: each successful
startSleepListener()spawns a goroutine that waits ons.ctx.Done()and then callss.stopSleepListener(). If you start/stop repeatedly (via connect/disconnect cycles), you’ll accumulate one such goroutine per start until the app exits. It’s not a correctness bug, just a tiny leak.If you want to tighten this, you can start the ctx.Done watcher only the first time you initialize the service, e.g. by tracking whether the watcher is already running:
type serviceClient struct { // ... - sleepService *sleep.Service - sleepLock sync.Mutex + sleepService *sleep.Service + sleepLock sync.Mutex + sleepWatcherStarted bool } func (s *serviceClient) startSleepListener() { s.sleepLock.Lock() defer s.sleepLock.Unlock() if s.sleepService != nil { log.Debug("sleep detection service already initialized") return } sleepService, err := sleep.New() // ... error handling and Register ... s.sleepService = sleepService log.Info("sleep detection service initialized") - // Cleanup on context cancellation - go func() { - <-s.ctx.Done() - s.stopSleepListener() - }() + // Cleanup on context cancellation (only wire once) + if !s.sleepWatcherStarted { + s.sleepWatcherStarted = true + go func() { + <-s.ctx.Done() + s.stopSleepListener() + }() + } }This keeps the lifecycle semantics you introduced while ensuring there’s only a single watcher goroutine tied to
s.ctx.Also applies to: 1200-1214
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/ui/client_ui.go(3 hunks)client/ui/event_handler.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: release
🔇 Additional comments (1)
client/ui/client_ui.go (1)
323-327: Sleep listener fields and wiring look consistent with usageAdding
sleepService *sleep.ServiceandsleepLock sync.Mutex, plus kicking offgo s.startSleepListener()inonTrayReady, matches how the event handler now callsstartSleepListener()/stopSleepListener(). The single mutex‑guarded service instance is a reasonable way to coordinate lifecycle across the tray and menu handlers.Also applies to: 1107-1109
hurricanehrndz
force-pushed
the
fix/stop-sleep-monitor-on-disconnect
branch
from
2ccc05e to
60c664f
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/ui/event_handler.go (1)
80-98: Race between deferred sleep start on connect and explicit stop on disconnectUsing
defer h.client.startSleepListener()means the sleep listener is (re)started whenever the connect goroutine exits, even if the operation was canceled by a user‑initiated disconnect. WithhandleDisconnectClicknow callingh.client.stopSleepListener(), this creates a race:
- User clicks Connect → goroutine starts, registers
defer startSleepListener().- Before connect finishes, user clicks Disconnect →
connectCancel()is invoked andstopSleepListener()runs, stopping and nil‑ingsleepService.- When the connect goroutine unwinds after the cancellation, the deferred
startSleepListener()runs and may re‑createsleepServiceafter the explicit stop, leaving the sleep monitor running despite the user’s disconnect request.Final state (sleep listener on/off) becomes scheduling‑dependent, which breaks the PR’s intention and introduces a concurrency correctness issue.
A safer pattern is to only start the sleep listener if the connect actually succeeds, and to do it explicitly (not via
defer). For example:func (h *eventHandler) handleConnectClick() { @@ - go func() { - defer connectCancel() - defer h.client.startSleepListener() - - if err := h.client.menuUpClick(connectCtx); err != nil { + go func() { + defer connectCancel() + + if err := h.client.menuUpClick(connectCtx); err != nil { st, ok := status.FromError(err) if errors.Is(err, context.Canceled) || (ok && st.Code() == codes.Canceled) { log.Debugf("connect operation cancelled by user") } else { h.client.app.SendNotification(fyne.NewNotification("Error", "Failed to connect")) log.Errorf("connect failed: %v", err) } - } - - if err := h.client.updateStatus(); err != nil { - log.Debugf("failed to update status after connect: %v", err) - } + return + } + + if err := h.client.updateStatus(); err != nil { + log.Debugf("failed to update status after connect: %v", err) + } + + // Only reach here if connect succeeded; safe to (re)start sleep listener. + h.client.startSleepListener() }() }This way:
- A canceled/failed connect never restarts the sleep listener.
stopSleepListener()on disconnect deterministically wins for that user action.Also applies to: 100-110
🧹 Nitpick comments (2)
client/ui/client_ui.go (2)
1104-1109: Unconditional startup sleep listener vs connect/disconnect semanticsStarting the sleep listener unconditionally on tray ready (
go s.startSleepListener()) ensures OS sleep events are tracked even before the first manual connect, and for cases where the daemon is already connected when the UI starts. With the new connect/disconnect wiring, this is slightly asymmetric with “restart on connect” wording, but behavior is coherent:
- Initial app start → listener starts once.
- Manual disconnect →
stopSleepListener()stops it.- Subsequent connects → explicit
startSleepListener()(once the connect path is fixed per other comment) will re‑enable it.If you want the invariant “no sleep monitoring while manually disconnected” even from the very start, consider documenting this behavior or gating the initial
startSleepListener()behind a “currently connected” status check. Otherwise, this is acceptable as‑is.
1169-1198: Sleep listener lifecycle & locking are correct; minor goroutine nitThe
startSleepListener/stopSleepListenerpair looks concurrency‑safe and matches the PR’s goals:
sleepLockprotects all reads/writes ofsleepService.startSleepListeneris idempotent under the lock and only setss.sleepServiceaftersleep.New()andRegisterboth succeed, so there’s no half‑initialized state.stopSleepListenerhandles the nil case, deregisters, and clears the pointer under the same lock, so multiple callers (including those from ctx cancellation) are safe.The small trade‑off is that each successful
startSleepListenerspawns a goroutine waiting ons.ctx.Done()and callings.stopSleepListener(). Over many connect/disconnect cycles you’ll accumulate one goroutine per successful start, all blocked on the same context until exit. That’s bounded and probably fine here, but if you want to be extra tidy you could track a single “ctx cancel watcher” or check whether such a watcher is already running before starting another.Overall, the lifecycle and locking logic themselves look good.
Also applies to: 1200-1214
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/ui/client_ui.go(3 hunks)client/ui/event_handler.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: JS / Lint
- GitHub Check: Android / Build
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: release_ui
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
🔇 Additional comments (1)
client/ui/client_ui.go (1)
323-327: Sleep service fields and locking look appropriateThe addition of
sleepService *sleep.Serviceguarded bysleepLock sync.Mutexcleanly scopes sleep‑detection lifecycle insideserviceClientand centralizes synchronization. The rest of the code usessleepLockconsistently around all accesses, so field‑level changes themselves look sound.
1 participant
Describe your changes
Sleep monitor shouldn't be running if user manually clicks disconnect
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
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
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.