fix: correctly classify Bluetooth releases and peer responses

Author: cssmagic

Magic Switch/Manager/OutgoingConnection.swift

@@ -10,6 +10,12 @@ enum OutgoingFailure: Error {
   case connectionFailed(String)
   case connectTimeout
   case handshakeFailed(SecureChannelError)
+  /// The authenticated peer received the request but reported that it could
+  /// not complete the operation.
+  case remoteOperationFailed
+  /// The authenticated peer replied with data that is not valid for the
+  /// current command.
+  case invalidResponse
   case bodyFailed
   /// Self-throttled because of recent repeated failures to this host.
   /// Prevents a runaway retry loop from racking up failures the peer's
@@ -36,6 +42,10 @@ enum OutgoingFailure: Error {
       return "Couldn't establish a secure connection (possible tampering)."
     case .handshakeFailed:
       return "Couldn't establish a secure connection."
+    case .remoteOperationFailed:
+      return "The other Mac reported that it couldn't complete the operation."
+    case .invalidResponse:
+      return "The other Mac returned an invalid response."
     case .bodyFailed:
       return "The connection dropped mid-message."
     case .tooManyRecentFailures:
@@ -163,7 +173,9 @@ final class OutgoingConnection {
   /// receives a `Result` whose failure case carries a categorised reason
   /// (see `OutgoingFailure`) so the caller can render a useful notification.
   func run(
-    body: @escaping (SecureChannel, @escaping (Bool) -> Void) -> Void,
+    body: @escaping (
+      SecureChannel, @escaping (Result<Void, OutgoingFailure>) -> Void
+    ) -> Void,
     completion: @escaping (Result<Void, OutgoingFailure>) -> Void
   ) {
     selfRef = self
@@ -222,9 +234,8 @@ final class OutgoingConnection {
                 provedByHandshake: provedFingerprint)
             }
             self.startBodyTimer(completion: completion)
-            body(channel) { ok in
-              self.finish(
-                ok ? .success(()) : .failure(.bodyFailed), completion: completion)
+            body(channel) { result in
+              self.finish(result, completion: completion)
             }
           case .failure(let err):
             print("OutgoingConnection handshake failed: \(err)")
@@ -270,13 +281,15 @@ final class OutgoingConnection {
     bodyTimer = nil
     channel?.cancel()
     connection.cancel()
-    // Feed the outbound rate limiter so a series of failures throttles
-    // future attempts, and a success clears the counter immediately. Skip
+    // Feed the outbound rate limiter so a series of transport/protocol
+    // failures throttles future attempts, and a healthy authenticated reply
+    // clears the counter immediately. OP_FAILED is an operation failure, not
+    // a network failure: the peer received and answered the request. Skip
     // `tooManyRecentFailures` — that's the limiter's own refusal and would
     // double-count. Background reachability probes opt out entirely.
     if countsTowardRateLimit {
       switch result {
-      case .success:
+      case .success, .failure(.remoteOperationFailed):
         rateLimiter.recordSuccess(host: host)
       case .failure(.tooManyRecentFailures):
         break

Magic Switch/Model/Store/BluetoothPeripheralStore.swift

@@ -70,6 +70,15 @@ final class BluetoothPeripheralStore: NSObject, ObservableObject, BluetoothPerip
     static let adoptionMaxConnectAttempts = 3
   }
 
+  /// Result of asking IOBluetooth to close an existing baseband connection.
+  /// `closeConnection()` is synchronous and its success return is authoritative;
+  /// an immediately-following `isConnected()` snapshot can still be stale or
+  /// reflect a rapid macOS reconnect, so it must never overturn that result.
+  private enum CloseConnectionOutcome {
+    case released
+    case failed(IOReturn)
+  }
+
   // MARK: - Dependencies
 
   private let bluetoothQueue = DispatchQueue(label: Constants.queueLabel, qos: .userInitiated)
@@ -411,10 +420,10 @@ final class BluetoothPeripheralStore: NSObject, ObservableObject, BluetoothPerip
         else { continue }
         connectedIDs.append(peripheral.id)
         guard shouldRelease else { continue }
-        let result = device.closeConnection()
-        if !device.isConnected() {
+        switch closeConnectionIfNeeded(device) {
+        case .released:
           releasedIDs.append(peripheral.id)
-        } else {
+        case .failed(let result):
           print("Before sleep: failed to disconnect \(peripheral.name): \(result)")
         }
       }
@@ -616,47 +625,54 @@ final class BluetoothPeripheralStore: NSObject, ObservableObject, BluetoothPerip
       print("Bluetooth is off; \(peripheral.name) is already disconnected")
       markDisconnectedAfterRelease(peripheral.id)
       finishReleasePeripheral(
-        peripheral, success: true, connectedAfterFailure: false,
-        detail: "", completion: completion)
+        peripheral, success: true, detail: "", completion: completion)
       return
     }
     guard let device = IOBluetoothDevice(addressString: peripheral.id) else {
       print("\(peripheral.name) is absent from the Bluetooth stack and already disconnected")
       markDisconnectedAfterRelease(peripheral.id)
       finishReleasePeripheral(
-        peripheral, success: true, connectedAfterFailure: false,
-        detail: "", completion: completion)
+        peripheral, success: true, detail: "", completion: completion)
       return
     }
 
-    var result = kIOReturnSuccess
-    if device.isConnected() {
-      result = device.closeConnection()
-    }
-    let success = !device.isConnected()
-    if success {
+    switch closeConnectionIfNeeded(device) {
+    case .released:
       print("Disconnected \(peripheral.name) without changing its pairing")
       markDisconnectedAfterRelease(peripheral.id)
-    } else {
+      finishReleasePeripheral(
+        peripheral, success: true, detail: "", completion: completion)
+    case .failed(let result):
       print("Failed to disconnect \(peripheral.name): \(result)")
+      finishReleasePeripheral(
+        peripheral, success: false, detail: "closeConnection returned \(result)",
+        completion: completion)
+    }
+  }
+
+  /// Closes `device` without modifying its pairing record. A successful
+  /// IOBluetooth return is final. When the command reports an error, a live
+  /// state check may still prove that the connection disappeared concurrently,
+  /// which also satisfies the release request.
+  private func closeConnectionIfNeeded(_ device: IOBluetoothDevice) -> CloseConnectionOutcome {
+    guard device.isConnected() else { return .released }
+    let result = device.closeConnection()
+    if result == kIOReturnSuccess || !device.isConnected() {
+      return .released
     }
-    finishReleasePeripheral(
-      peripheral, success: success, connectedAfterFailure: device.isConnected(),
-      detail: "closeConnection returned \(result)", completion: completion)
+    return .failed(result)
   }
 
   private func finishReleasePeripheral(
     _ peripheral: BluetoothPeripheral,
     success: Bool,
-    connectedAfterFailure: Bool,
     detail: String,
     completion: ((Bool) -> Void)?
   ) {
     DispatchQueue.main.async {
       if !success {
         self.clearReleaseLease(peripheral.id)
-        self.setConnectionState(
-          connectedAfterFailure ? .connected : .disconnected, for: peripheral.id)
+        self.setConnectionState(.connected, for: peripheral.id)
         self.setPeripheralError("Couldn't release.", for: peripheral.id)
         NotificationManager.showNotification(
           title: "Couldn't Release Peripheral",
@@ -1458,16 +1474,13 @@ final class BluetoothPeripheralStore: NSObject, ObservableObject, BluetoothPerip
     guard hasActiveReleaseLease(address) else { return }
     bluetoothQueue.async { [weak self] in
       guard let self = self else { return }
-      var result = kIOReturnSuccess
-      if device.isConnected() {
-        result = device.closeConnection()
-      }
-      let released = !device.isConnected()
+      let outcome = self.closeConnectionIfNeeded(device)
       DispatchQueue.main.async {
         guard self.hasActiveReleaseLease(address) else { return }
-        if released {
+        switch outcome {
+        case .released:
           self.markDisconnectedAfterRelease(address)
-        } else {
+        case .failed(let result):
           print("Release lease couldn't disconnect \(address): \(result)")
           self.setPeripheralError("Couldn't keep released.", for: address)
           NotificationManager.showNotification(
@@ -1611,8 +1624,8 @@ final class BluetoothPeripheralStore: NSObject, ObservableObject, BluetoothPerip
   /// failure; an adoption — no prior claim — takes it only once the peer is
   /// provably absent: unreachable at the connect layer for
   /// `adoptionRequiredAbsentStreak` consecutive probes. A peer that answers
-  /// at all — an explicit "not holding" (`.bodyFailed`) included — outranks
-  /// us, so stand down and leave the move to its reclaim or to the user.
+  /// at all — an explicit "not holding" (`.remoteOperationFailed`) included —
+  /// outranks us, so stand down and leave the move to its reclaim or to the user.
   /// Connection attempts are capped: repeated failures usually mean the
   /// device is busy with a peer we cannot reach.
   private func continueAdoption(of peripheral: BluetoothPeripheral, after failure: OutgoingFailure)

Magic Switch/Model/Store/NetworkDeviceStore.swift

@@ -908,6 +908,25 @@ enum DeviceCommand: String, Codable {
   case introduce = "INTRODUCE"
 }
 
+/// Decodes the standard OP_SUCCESS/OP_FAILED acknowledgement used by normal
+/// commands. A valid failure acknowledgement proves the authenticated channel
+/// stayed healthy; malformed or command-shaped replies are protocol errors.
+private func decodeOperationResponse(_ data: Data) -> Result<Void, OutgoingFailure> {
+  guard let response = String(data: data, encoding: .utf8),
+    let command = DeviceCommand(rawValue: response)
+  else {
+    return .failure(.invalidResponse)
+  }
+  switch command {
+  case .operationSuccess:
+    return .success(())
+  case .operationFailed:
+    return .failure(.remoteOperationFailed)
+  default:
+    return .failure(.invalidResponse)
+  }
+}
+
 /// Identity carried by `INTRODUCE` frames: `<listenPort>|<name>`, port first
 /// so the name may contain `|`.
 struct IntroducedIdentity {
@@ -997,21 +1016,16 @@ extension NetworkDeviceStore {
         channel.send(Data(command.rawValue.utf8)) { sendErr in
           if let sendErr = sendErr {
             print("Failed to send command: \(sendErr)")
-            done(false)
+            done(.failure(.bodyFailed))
             return
           }
           channel.receive { result in
             switch result {
             case .failure(let err):
               print("Failed to receive response: \(err)")
-              done(false)
+              done(.failure(.bodyFailed))
             case .success(let data):
-              let response = String(data: data, encoding: .utf8) ?? ""
-              if let resp = DeviceCommand(rawValue: response) {
-                done(resp == .operationSuccess)
-              } else {
-                done(false)
-              }
+              done(decodeOperationResponse(data))
             }
           }
         }
@@ -1038,14 +1052,14 @@ extension NetworkDeviceStore {
         channel.send(Data(DeviceCommand.notification.rawValue.utf8)) { err in
           if let err = err {
             print("Notification command send failed: \(err)")
-            done(false)
+            done(.failure(.bodyFailed))
             return
           }
           let payload = "\(title)|\(message)"
           channel.send(Data(payload.utf8)) { err2 in
             if let err2 = err2 {
               print("Notification payload send failed: \(err2)")
-              done(false)
+              done(.failure(.bodyFailed))
               return
             }
             // Wait for the receiver's OP_SUCCESS/OP_FAILED before tearing
@@ -1057,10 +1071,9 @@ extension NetworkDeviceStore {
               switch result {
               case .failure(let err):
                 print("Notification ack receive failed: \(err)")
-                done(false)
+                done(.failure(.bodyFailed))
               case .success(let data):
-                let response = String(data: data, encoding: .utf8) ?? ""
-                done(DeviceCommand(rawValue: response) == .operationSuccess)
+                done(decodeOperationResponse(data))
               }
             }
           }
@@ -1102,13 +1115,13 @@ extension NetworkDeviceStore {
         channel.send(Data(DeviceCommand.syncPeripherals.rawValue.utf8)) { err in
           if let err = err {
             print("syncPeripherals command send failed: \(err)")
-            done(false)
+            done(.failure(.bodyFailed))
             return
           }
           channel.send(Data(jsonString.utf8)) { err2 in
             if let err2 = err2 {
               print("syncPeripherals payload send failed: \(err2)")
-              done(false)
+              done(.failure(.bodyFailed))
               return
             }
             // Same rationale as the notification path: wait for the
@@ -1118,10 +1131,9 @@ extension NetworkDeviceStore {
               switch result {
               case .failure(let err):
                 print("syncPeripherals ack receive failed: \(err)")
-                done(false)
+                done(.failure(.bodyFailed))
               case .success(let data):
-                let response = String(data: data, encoding: .utf8) ?? ""
-                done(DeviceCommand(rawValue: response) == .operationSuccess)
+                done(decodeOperationResponse(data))
               }
             }
           }
@@ -1223,32 +1235,32 @@ extension NetworkDeviceStore {
         channel.send(Data(DeviceCommand.introduce.rawValue.utf8)) { err in
           if let err = err {
             print("INTRODUCE command send failed: \(err)")
-            done(false)
+            done(.failure(.bodyFailed))
             return
           }
           channel.send(Data(local.encoded.utf8)) { err2 in
             if let err2 = err2 {
               print("INTRODUCE payload send failed: \(err2)")
-              done(false)
+              done(.failure(.bodyFailed))
               return
             }
             channel.receive { result in
               switch result {
               case .failure(let err):
                 print("INTRODUCE reply receive failed: \(err)")
-                done(false)
+                done(.failure(.bodyFailed))
               case .success(let data):
                 let response = String(data: data, encoding: .utf8) ?? ""
                 if let identity = IntroducedIdentity(payload: response),
                   let proved = outgoing.provedFingerprint
                 {
                   reply = .peer(identity, provedFingerprint: proved)
-                  done(true)
+                  done(.success(()))
                 } else if DeviceCommand(rawValue: response) == .operationFailed {
                   reply = .legacy
-                  done(true)
+                  done(.success(()))
                 } else {
-                  done(false)
+                  done(.failure(.invalidResponse))
                 }
               }
             }
@@ -1258,7 +1270,7 @@ extension NetworkDeviceStore {
       completion: { result in
         switch result {
         case .success:
-          completion(reply.map { .success($0) } ?? .failure(.bodyFailed))
+          completion(reply.map { .success($0) } ?? .failure(.invalidResponse))
         case .failure(let err):
           completion(.failure(err))
         }
@@ -1358,23 +1370,22 @@ extension NetworkDeviceStore {
         channel.send(Data(command.rawValue.utf8)) { err in
           if let err = err {
             print("\(command.rawValue) command send failed: \(err)")
-            done(false)
+            done(.failure(.bodyFailed))
             return
           }
           channel.send(Data(payload.utf8)) { err2 in
             if let err2 = err2 {
               print("\(command.rawValue) payload send failed: \(err2)")
-              done(false)
+              done(.failure(.bodyFailed))
               return
             }
             channel.receive { result in
               switch result {
               case .failure(let err):
                 print("\(command.rawValue) ack receive failed: \(err)")
-                done(false)
+                done(.failure(.bodyFailed))
               case .success(let data):
-                let response = String(data: data, encoding: .utf8) ?? ""
-                done(DeviceCommand(rawValue: response) == .operationSuccess)
+                done(decodeOperationResponse(data))
               }
             }
           }