Skip to content

Add thread safety to local push reconnection logic #4132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 4 commits into
base: local-push
Choose a base branch
from
Draft

Add thread safety to local push reconnection logic #4132

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40ca8ce
Initial plan
Copilot Dec 22, 2025
5465ade
Add thread safety to NotificationManagerLocalPushInterfaceExtension
Copilot Dec 22, 2025
976d57d
Lint
bgoncal Dec 22, 2025
c77ae71
Merge branch 'local-push' into copilot/sub-pr-4113
bgoncal Dec 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 96 additions & 45 deletions Sources/App/Notifications/NotificationManagerLocalPushInterfaceExtension.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@ final class NotificationManagerLocalPushInterfaceExtension: NSObject, Notificati
}
}

// Serial queue for thread-safe access to shared mutable state
private let queue = DispatchQueue(label: "io.homeassistant.LocalPushInterface")

// Reconnection timer properties
// These properties must only be accessed on the main queue since Timer.scheduledTimer requires main thread
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment states "These properties must only be accessed on the main queue since Timer.scheduledTimer requires main thread", but this is inaccurate. Timer.scheduledTimer does not require the main thread - it requires the thread with an active RunLoop. Timers can be scheduled on any thread with a RunLoop, though the main thread is most common for UI-related code.

Suggested change
// These properties must only be accessed on the main queue since Timer.scheduledTimer requires main thread
// These properties must only be accessed on a thread with an active RunLoop (typically the main queue),
// since Timer.scheduledTimer relies on a RunLoop rather than strictly requiring the main thread

Copilot uses AI. Check for mistakes.
private var reconnectionTimer: Timer?
private var reconnectionAttempt = 0
/// Backoff delays (in seconds) between reconnection attempts:
Expand All @@ -38,6 +42,7 @@ final class NotificationManagerLocalPushInterfaceExtension: NSObject, Notificati
]

// Track servers that have failed connections
// Access to this property is synchronized via the queue
private var disconnectedServers = Set<Identifier<Server>>()

func status(for server: Server) -> NotificationManagerLocalPushStatus {
Expand All @@ -50,43 +55,52 @@ final class NotificationManagerLocalPushInterfaceExtension: NSObject, Notificati
Current.Log.verbose("Server \(server.identifier.rawValue) sync state: \(state)")

// Track disconnected state for reconnection logic
switch state {
case .unavailable:
if !disconnectedServers.contains(server.identifier) {
Current.Log.info("Server \(server.identifier.rawValue) local push became unavailable")
Current.Log
.verbose(
"Adding server \(server.identifier.rawValue) to disconnected set. Current disconnected servers: \(disconnectedServers.map(\.rawValue))"
)
disconnectedServers.insert(server.identifier)
Current.Log.verbose("Disconnected servers after insert: \(disconnectedServers.map(\.rawValue))")
scheduleReconnection()
} else {
Current.Log.verbose("Server \(server.identifier.rawValue) already in disconnected set")
}
case .available, .establishing:
if disconnectedServers.contains(server.identifier) {
Current.Log.info("Server \(server.identifier.rawValue) local push reconnected successfully")
Current.Log
.verbose(
"Removing server \(server.identifier.rawValue) from disconnected set. Current disconnected servers: \(disconnectedServers.map(\.rawValue))"
)
disconnectedServers.remove(server.identifier)
Current.Log.verbose("Disconnected servers after remove: \(disconnectedServers.map(\.rawValue))")
if disconnectedServers.isEmpty {
Current.Log.verbose("All servers reconnected, cancelling reconnection timer")
cancelReconnection()
// Use queue to synchronize access to disconnectedServers
queue.sync {
switch state {
case .unavailable:
if !disconnectedServers.contains(server.identifier) {
Current.Log.info("Server \(server.identifier.rawValue) local push became unavailable")
Current.Log
.verbose(
"Adding server \(server.identifier.rawValue) to disconnected set. Current disconnected servers: \(disconnectedServers.map(\.rawValue))"
)
disconnectedServers.insert(server.identifier)
Current.Log
.verbose("Disconnected servers after insert: \(disconnectedServers.map(\.rawValue))")
DispatchQueue.main.async { [weak self] in
self?.scheduleReconnection()
}
Comment on lines +71 to +73
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using DispatchQueue.main.async inside queue.sync can lead to deadlocks if the serial queue is blocked waiting for the main queue, while the main queue needs to acquire the serial queue lock. Consider restructuring this code to release the queue lock before dispatching to main, or move the decision logic outside the sync block.

Copilot uses AI. Check for mistakes.
} else {
Current.Log.verbose("Server \(server.identifier.rawValue) already in disconnected set")
}
case .available, .establishing:
if disconnectedServers.contains(server.identifier) {
Current.Log.info("Server \(server.identifier.rawValue) local push reconnected successfully")
Current.Log
.verbose(
"Removing server \(server.identifier.rawValue) from disconnected set. Current disconnected servers: \(disconnectedServers.map(\.rawValue))"
)
disconnectedServers.remove(server.identifier)
Current.Log
.verbose("Disconnected servers after remove: \(disconnectedServers.map(\.rawValue))")
if disconnectedServers.isEmpty {
Current.Log.verbose("All servers reconnected, cancelling reconnection timer")
DispatchQueue.main.async { [weak self] in
self?.cancelReconnection()
}
Comment on lines +89 to +91
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using DispatchQueue.main.async inside queue.sync can lead to deadlocks if the serial queue is blocked waiting for the main queue, while the main queue needs to acquire the serial queue lock. Consider restructuring this code to release the queue lock before dispatching to main, or move the decision logic outside the sync block.

Copilot uses AI. Check for mistakes.
} else {
Current.Log
.verbose(
"Still have \(disconnectedServers.count) disconnected server(s), keeping timer active"
)
}
} else {
Current.Log
.verbose(
"Still have \(disconnectedServers.count) disconnected server(s), keeping timer active"
"Server \(server.identifier.rawValue) is connected and was not in disconnected set"
)
}
} else {
Current.Log
.verbose(
"Server \(server.identifier.rawValue) is connected and was not in disconnected set"
)
}
}

Expand All @@ -99,12 +113,14 @@ final class NotificationManagerLocalPushInterfaceExtension: NSObject, Notificati
} else {
// manager isn't running
Current.Log.verbose("Server \(server.identifier.rawValue) has no active managers")
if disconnectedServers.contains(server.identifier) {
Current.Log
.verbose(
"Removing server \(server.identifier.rawValue) from disconnected set (manager not running)"
)
disconnectedServers.remove(server.identifier)
queue.sync {
if disconnectedServers.contains(server.identifier) {
Current.Log
.verbose(
"Removing server \(server.identifier.rawValue) from disconnected set (manager not running)"
)
disconnectedServers.remove(server.identifier)
}
}
return .disabled
}
Expand Down Expand Up @@ -154,13 +170,19 @@ final class NotificationManagerLocalPushInterfaceExtension: NSObject, Notificati

deinit {
Current.Log.verbose("NotificationManagerLocalPushInterfaceExtension deinit, cleaning up reconnection timer")
cancelReconnection()
// Cancel timer on main thread since Timer must be invalidated on the thread it was created
DispatchQueue.main.async { [reconnectionTimer] in
reconnectionTimer?.invalidate()
Comment on lines +174 to +175
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deinit captures reconnectionTimer in a closure that is dispatched asynchronously to the main queue. If deinit is called from a background thread and the object is deallocated before the async block executes, the captured timer reference may become invalid. Consider using a pattern that safely invalidates the timer synchronously on the main thread if already on main, or dispatch sync to main if on a background thread.

Suggested change
DispatchQueue.main.async { [reconnectionTimer] in
reconnectionTimer?.invalidate()
if Thread.isMainThread {
reconnectionTimer?.invalidate()
} else {
let timer = reconnectionTimer
DispatchQueue.main.sync {
timer?.invalidate()
}

Copilot uses AI. Check for mistakes.
}
}

// MARK: - Reconnection Logic

/// Schedules a reconnection attempt with gradual backoff
/// Must be called on the main thread
private func scheduleReconnection() {
dispatchPrecondition(condition: .onQueue(.main))

Current.Log
.verbose(
"scheduleReconnection called. Current attempt: \(reconnectionAttempt), timer active: \(reconnectionTimer != nil)"
Expand All @@ -173,11 +195,16 @@ final class NotificationManagerLocalPushInterfaceExtension: NSObject, Notificati
let delayIndex = min(reconnectionAttempt, reconnectionDelays.count - 1)
let delay = reconnectionDelays[delayIndex]

// Get disconnected server count in a thread-safe way
let serverInfo = queue.sync { () -> (count: Int, identifiers: [String]) in
(disconnectedServers.count, disconnectedServers.map(\.rawValue))
}
Comment on lines +199 to +201
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pattern of creating a tuple with (count: Int, identifiers: [String]) from disconnectedServers is repeated in multiple locations (lines 186-188, 216-218, 265-267, 309-311). Consider extracting this into a private helper method to reduce code duplication and improve maintainability.

Copilot uses AI. Check for mistakes.

Current.Log
.info(
"Scheduling local push reconnection attempt #\(reconnectionAttempt + 1) in \(delay) seconds for \(disconnectedServers.count) server(s)"
"Scheduling local push reconnection attempt #\(reconnectionAttempt + 1) in \(delay) seconds for \(serverInfo.count) server(s)"
)
Current.Log.verbose("Disconnected servers: \(disconnectedServers.map(\.rawValue))")
Current.Log.verbose("Disconnected servers: \(serverInfo.identifiers)")
Current.Log.verbose("Using delay index \(delayIndex) from reconnectionDelays array")

reconnectionTimer = Timer.scheduledTimer(
Expand All @@ -192,13 +219,22 @@ final class NotificationManagerLocalPushInterfaceExtension: NSObject, Notificati
}

/// Attempts to reconnect by reloading managers
/// Must be called on the main thread
private func attemptReconnection() {
dispatchPrecondition(condition: .onQueue(.main))

reconnectionAttempt += 1

// Get disconnected server info in a thread-safe way
let serverInfo = queue.sync { () -> (count: Int, identifiers: [String]) in
(disconnectedServers.count, disconnectedServers.map(\.rawValue))
}

Current.Log
.info(
"Attempting local push reconnection #\(reconnectionAttempt) for servers: \(disconnectedServers.map(\.rawValue))"
"Attempting local push reconnection #\(reconnectionAttempt) for servers: \(serverInfo.identifiers)"
)
Current.Log.verbose("Current disconnected server count: \(disconnectedServers.count)")
Current.Log.verbose("Current disconnected server count: \(serverInfo.count)")
Current.Log
.verbose(
"Next delay will be: \(reconnectionDelays[min(reconnectionAttempt, reconnectionDelays.count - 1)])s"
Expand All @@ -214,7 +250,10 @@ final class NotificationManagerLocalPushInterfaceExtension: NSObject, Notificati
}

/// Cancels any pending reconnection timer and resets the attempt counter
/// Must be called on the main thread
private func cancelReconnection() {
dispatchPrecondition(condition: .onQueue(.main))

Current.Log
.verbose(
"cancelReconnection called. Timer active: \(reconnectionTimer != nil), attempt count: \(reconnectionAttempt)"
Expand All @@ -234,7 +273,13 @@ final class NotificationManagerLocalPushInterfaceExtension: NSObject, Notificati

private func updateManagers() {
Current.Log.info("updateManagers called - loading NEAppPushManager preferences")
Current.Log.verbose("Current disconnected servers: \(disconnectedServers.map(\.rawValue))")

// Get disconnected server info in a thread-safe way
let disconnectedServerIds = queue.sync {
disconnectedServers.map(\.rawValue)
}

Current.Log.verbose("Current disconnected servers: \(disconnectedServerIds)")
Current.Log.verbose("Reconnection attempt count: \(reconnectionAttempt)")
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessing reconnectionAttempt in updateManagers() without thread safety. This method is called from background thread (NEAppPushManager.loadAllFromPreferences callback at line 272), but reconnectionAttempt is a main-thread-only property. This creates a race condition when the timer fires on the main thread and modifies reconnectionAttempt at the same time.

Copilot uses AI. Check for mistakes.

NEAppPushManager.loadAllFromPreferences { [weak self] managers, error in
Expand Down Expand Up @@ -272,7 +317,13 @@ final class NotificationManagerLocalPushInterfaceExtension: NSObject, Notificati
/// Managers for removed SSIDs or disabled servers are intentionally not recreated.
private func reloadManagersAfterSave() {
Current.Log.info("Reloading managers after configuration changes")
Current.Log.verbose("Current disconnected servers: \(disconnectedServers.map(\.rawValue))")

// Get disconnected server info in a thread-safe way
let disconnectedServerIds = queue.sync {
disconnectedServers.map(\.rawValue)
}

Current.Log.verbose("Current disconnected servers: \(disconnectedServerIds)")

NEAppPushManager.loadAllFromPreferences { [weak self] managers, error in
guard let self else {
Expand Down
Loading