Fix local push reconnection logic to prevent stalled reconnections #4134
Conversation
…ous reconnection The reconnection logic had a critical bug where if a reconnection attempt failed, no further reconnection attempts would be scheduled. This was because the code only scheduled a reconnection when a server was first added to the disconnectedServers set, not when subsequent status checks found it still unavailable. Additionally, multiple rapid status checks could potentially schedule multiple timers without proper coordination. Changes: - Renamed scheduleReconnection() to scheduleReconnectionIfNeeded() to better reflect its idempotent behavior - Made scheduleReconnectionIfNeeded() idempotent by checking if a timer is already active before scheduling - Moved scheduleReconnectionIfNeeded() call outside the if/else block so it's called for ALL unavailable states - Clear the timer reference in attemptReconnection() after the timer fires - Updated comments to clarify the behavior This ensures: 1. Only one timer is active at a time (prevents rapid successive attempts) 2. Failed reconnection attempts properly schedule the next attempt with backoff 3. Proper backoff timing is maintained (timer won't be reset prematurely) Co-authored-by: bgoncal <[email protected]>
Added more detailed comments to explain: - Why scheduleReconnectionIfNeeded() is called for all unavailable states - The idempotent nature of the scheduling method - Why the timer is cleared before incrementing the attempt counter Co-authored-by: bgoncal <[email protected]>
|
Found 1 unused localization strings in the codebase. Click to see detailsTo clean up these strings, manually remove them from the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## local-push #4134 +/- ##
=============================================
Coverage ? 45.44%
=============================================
Files ? 242
Lines ? 13736
Branches ? 0
=============================================
Hits ? 6242
Misses ? 7494
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the local push notification reconnection logic where failed reconnection attempts would not schedule subsequent retries, causing permanent connection failures until app restart. The fix ensures that reconnection attempts continue with proper backoff timing by making the scheduling method idempotent and always calling it for unavailable states.
Key Changes:
- Renamed
scheduleReconnection()toscheduleReconnectionIfNeeded()with idempotency guards to prevent duplicate timers - Changed unavailable state handling to always call
scheduleReconnectionIfNeeded(), not just on initial disconnection - Added proper timer lifecycle management by clearing the timer reference when fired, enabling subsequent scheduling
| // If still unavailable after this attempt, schedule the next one | ||
| // This will be triggered by the state didSet when the connection fails | ||
| // If still unavailable after this attempt, the status observer will trigger | ||
| // and scheduleReconnectionIfNeeded() will schedule the next attempt with proper backoff |
There was a problem hiding this comment.
The comment at line 237 exceeds the 120-character line width limit specified in the SwiftFormat configuration. This comment line should be wrapped to stay within the 120-character maximum line width.
| // and scheduleReconnectionIfNeeded() will schedule the next attempt with proper backoff | |
| // and scheduleReconnectionIfNeeded() will schedule the next attempt | |
| // with proper backoff |
| let wasDisconnected = disconnectedServers.contains(server.identifier) | ||
| if !wasDisconnected { |
There was a problem hiding this comment.
The variable wasDisconnected is assigned but never used. It was created to replace the inline check in the if statement, but since it's only used once and the variable name doesn't add clarity beyond the original expression, consider removing it and keeping the original inline check !disconnectedServers.contains(server.identifier).
| let wasDisconnected = disconnectedServers.contains(server.identifier) | |
| if !wasDisconnected { | |
| if !disconnectedServers.contains(server.identifier) { |
| // 1. No timer is already active (prevents duplicate timers) | ||
| // 2. There are disconnected servers needing reconnection | ||
| // This ensures both initial disconnections and failed reconnection attempts | ||
| // properly schedule the next attempt with appropriate backoff. |
There was a problem hiding this comment.
The comments at lines 67-72 exceed the 120-character line width limit specified in the SwiftFormat configuration. These comment lines should be wrapped to stay within the 120-character maximum line width.
| // properly schedule the next attempt with appropriate backoff. | |
| // properly schedule the next attempt with appropriate backoff and timing. |
|
|
||
| /// Schedules a reconnection attempt with gradual backoff | ||
| private func scheduleReconnection() { | ||
| /// Schedules a reconnection attempt with gradual backoff if one is not already scheduled. |
There was a problem hiding this comment.
The comment at line 169 exceeds the 120-character line width limit specified in the SwiftFormat configuration. This comment line should be wrapped to stay within the 120-character maximum line width.
| /// Schedules a reconnection attempt with gradual backoff if one is not already scheduled. | |
| /// Schedules a reconnection attempt with gradual backoff. | |
| /// If a reconnection is already scheduled, this method does nothing. |
| /// This method is idempotent and safe to call multiple times - it will only schedule a new | ||
| /// timer if one is not already active. This prevents rapid successive reconnection attempts | ||
| /// and ensures proper backoff timing. |
There was a problem hiding this comment.
The comments at lines 170-172 exceed the 120-character line width limit specified in the SwiftFormat configuration. These comment lines should be wrapped to stay within the 120-character maximum line width.
| /// This method is idempotent and safe to call multiple times - it will only schedule a new | |
| /// timer if one is not already active. This prevents rapid successive reconnection attempts | |
| /// and ensures proper backoff timing. | |
| /// This method is idempotent and safe to call multiple times - it will only schedule a new timer | |
| /// if one is not already active. | |
| /// This prevents rapid successive reconnection attempts and ensures proper backoff timing. |
| Current.Log | ||
| .verbose( | ||
| "scheduleReconnection called. Current attempt: \(reconnectionAttempt), timer active: \(reconnectionTimer != nil)" | ||
| "scheduleReconnectionIfNeeded called. Current attempt: \(reconnectionAttempt), timer active: \(reconnectionTimer != nil), disconnected servers: \(disconnectedServers.count)" |
There was a problem hiding this comment.
The log message at line 176 exceeds the 120-character line width limit specified in the SwiftFormat configuration. This line should be wrapped to stay within the 120-character maximum line width.
| "scheduleReconnectionIfNeeded called. Current attempt: \(reconnectionAttempt), timer active: \(reconnectionTimer != nil), disconnected servers: \(disconnectedServers.count)" | |
| "scheduleReconnectionIfNeeded called. " | |
| + "Current attempt: \(reconnectionAttempt), " | |
| + "timer active: \(reconnectionTimer != nil), " | |
| + "disconnected servers: \(disconnectedServers.count)" |
Summary
The reconnection logic had a critical bug where failed reconnection attempts would not schedule subsequent retries, causing local push to permanently fail until app restart. Additionally, rapid status updates could schedule duplicate timers with incorrect backoff timing.
Root cause:
scheduleReconnection()was only called when a server was first added todisconnectedServers. After the first reconnection attempt failed, subsequent status checks found the server already tracked and did nothing, breaking the reconnection chain.Changes:
scheduleReconnection()→scheduleReconnectionIfNeeded()with idempotency guardsscheduleReconnectionIfNeeded()for unavailable states (not just initial disconnection)Flow before fix:
Flow after fix:
Screenshots
N/A - Internal networking logic change
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#
Any other notes
This PR merges into #4113 and addresses feedback from review comment #4113 (comment).
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.