fix(ble): fix crash and reconnect bugs in iOS BLE stack#1681
Conversation
…eplace force-unwrap UUID(uuidString:)! with safe guard-let; malformed/empty identifier in UserDefaults previously caused instant crash - handlePeripheralDisconnectError: set shouldReconnect=true for all CBError codes (was false for .connectionFailed and all default cases), and remove duplicate connectionDidDisconnect() call on the activeConnection path — BLEConnection.disconnect() already calls it internally, emitting two .deviceLost events that corrupted discovery state - handleWillRestoreState .connected branch: fix wantConfig/wantDatabase/ versionCheck all being false (contradicting the comment); now passes true so config and database are refreshed after background restoration - getPacketStream(): finish() previous continuation before overwriting it so the consumer's for-await loop terminates instead of hanging forever
…ct fixes Tests verify: - Invalid/empty BLE identifiers are handled safely (no force-unwrap crash) - UUID(uuidString:) returns nil for malformed strings (guard-let fix basis) - RSSI signal strength boundary values (exact -65 and -85 thresholds) - wasRestored flag is orthogonal to other Device properties (state restore fix) - All TransportType and ConnectionState model invariants
|
|
There was a problem hiding this comment.
Pull request overview
Fixes several crash/reconnect issues in the iOS CoreBluetooth transport layer to improve BLE reconnection reliability and state restoration behavior.
Changes:
- Hardened BLE connect path by safely parsing peripheral identifiers instead of force-unwrapping UUIDs.
- Expanded disconnect error handling to reconnect on more CoreBluetooth failure cases and removed a duplicate disconnect notification path.
- Improved BLE connection event streaming by finishing any prior
AsyncStreamcontinuation before overwriting it; added a new reconnect-focused test suite.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
Meshtastic/Accessory/Transports/Bluetooth Low Energy/BLETransport.swift |
Avoids UUID force-unwrap crash, broadens reconnect behavior, and adjusts restoration reconnect flags / disconnect notifications. |
Meshtastic/Accessory/Transports/Bluetooth Low Energy/BLEConnection.swift |
Prevents hanging consumers by finishing an old packet stream continuation before replacing it. |
MeshtasticTests/BLEReconnectTests.swift |
Adds unit tests around identifier handling, RSSI boundaries, state flags, and transport invariants. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BLETransport.swift: - handlePeripheralDisconnectError: set shouldReconnect=true for non-CBError branch (was staying false, preventing reconnect for unexpected error types) - handlePeripheralDisconnectError: remove duplicate connectionDidDisconnect call in connectContinuation branch — connect(to:) catch already calls it, causing duplicate .deviceLost events BLEReconnectTests.swift: - Fix stale branch name in header comment - Rename emptyIdentifierDoesNotProduceCrash -> emptyIdentifierStoredSafelyInDeviceModel with accurate comment - Remove 4 signal-strength boundary tests that duplicate DeviceTests.signalStrength in ConnectViewTests.swift; keep only extremeRSSIValuesDoNotCrash (unique coverage)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AsyncStream<ConnectionEvent> { continuation in | ||
| // Finish any previous stream so its consumer's `for await` loop terminates cleanly | ||
| // instead of hanging indefinitely on the abandoned continuation. | ||
| self.connectionStreamContinuation?.finish() |
There was a problem hiding this comment.
getPacketStream() now finishes any previous stream, but it still doesn’t attach an AsyncStream.Continuation.onTermination handler. If a consumer drops the stream early (e.g., stops iterating/cancels the task) without calling disconnect, BLEConnection may keep resources alive (RSSI task, peripheral notifications/delegate, etc.). Other transports do cleanup on termination (e.g., TCPConnection.getPacketStream and SerialConnection.getPacketStream). Consider adding an onTermination that triggers a disconnect/cleanup and clears connectionStreamContinuation to match the existing pattern and avoid leaks/hangs.
| self.connectionStreamContinuation?.finish() | |
| self.connectionStreamContinuation?.finish() | |
| continuation.onTermination = { [weak self] termination in | |
| guard let self else { return } | |
| self.connectionStreamContinuation = nil | |
| guard case .cancelled = termination else { return } | |
| Task { | |
| try? await self.disconnect(withError: CancellationError(), shouldReconnect: false) | |
| } | |
| } | |
There was a problem hiding this comment.
The other connection types (TCP, Serial) have a termination handler for their implementations, so I agree one should be added to BLEConnection.
The proposed termination handler here is a little more than we have in the other Connection types. This one checks for the AsyncThrowingStream.Continuation.Termination parameter, which can be .finsihed(Failure?) or .cancelled. The other handlers don't make this distinction, and simply clean up when the continuation finishes or is cancelled.
The only place were we call .finish is in the disconnect() function itself, so its circular to call disconnect when the connection stream finishes.
Recommend adding the parameter and the guard case .cancelled to the handlers in the other connection types as well.
|
The tests seem to be mostly testing swift, all of this looks like AI generated code done in an hour, I am not seeing a real fixes here. Have you tested this code on devices at all? |
|
Yeah I don't think I am going to merge any of these. |
| AsyncStream<ConnectionEvent> { continuation in | ||
| // Finish any previous stream so its consumer's `for await` loop terminates cleanly | ||
| // instead of hanging indefinitely on the abandoned continuation. | ||
| self.connectionStreamContinuation?.finish() |
There was a problem hiding this comment.
The other connection types (TCP, Serial) have a termination handler for their implementations, so I agree one should be added to BLEConnection.
The proposed termination handler here is a little more than we have in the other Connection types. This one checks for the AsyncThrowingStream.Continuation.Termination parameter, which can be .finsihed(Failure?) or .cancelled. The other handlers don't make this distinction, and simply clean up when the continuation finishes or is cancelled.
The only place were we call .finish is in the disconnect() function itself, so its circular to call disconnect when the connection stream finishes.
Recommend adding the parameter and the guard case .cancelled to the handlers in the other connection types as well.
| let connectTask = Task { @MainActor in | ||
| // In this case we need a full reconnect, so do the wantConfig, wantDatabase, and versionCheck | ||
| try await AccessoryManager.shared.connect(to: device, withConnection: restoredConnection, wantConfig: false, wantDatabase: false, versionCheck: false) | ||
| try await AccessoryManager.shared.connect(to: device, withConnection: restoredConnection, wantConfig: true, wantDatabase: true, versionCheck: true) |
There was a problem hiding this comment.
The whole point of the handleWillRestoreState is that it des not do a full wantConfig, wantDatabase, and versionCheck.
This handler is called when the app is terminated in the background. CoreBluetooth maintains the connection to the device, so the radio never knows the app was even killed. If a new packet comes in, or the user launches the app before any new BLE traffic, the app restores itself. No state has changed from when the app was terminated, so the app can resume with its current state. The previous wantConfig and database are already in the database.
We should only need a full wantConfig/wantDatabase when the app is in a state where it has missed a packet (ie first connect, or an inability to restore and handle a packet, etc.
I think the comment might be wrong and that mislead the AI?
| Logger.transport.debug("🛜 [BLETransport] Error while connected. Disconnecting the active connection.") | ||
| Task { | ||
| try? await activeConnection.disconnect(withError: error, shouldReconnect: shouldReconnect) | ||
| await self.connectionDidDisconnect(fromPeripheral: peripheral) |
There was a problem hiding this comment.
I think this one is okay, because yes, the call to .disconnect(withError:shouldReconnect:) does call connectionDidDisconnect already, so this is duplicative.
Summary
Fixes 5 crash-level bugs in the iOS BLE reconnection stack found during a full audit of the CoreBluetooth transport layer.
Changes
BLETransport.swift
UUID(uuidString: device.identifier)!with aguard letthat throwsAccessoryError.connectionFailedfor malformed/empty identifiershandlePeripheralDisconnectErroronly retried on CBError codes 6 & 7; added retry for.connectionFailed(code 4) and all other codes.deviceLostevents: Removed double call toconnectionDidDisconnect—BLEConnection.disconnect()already calls it internallyhandleWillRestoreState.connectedbranch hadwantConfig/wantDatabase/versionCheck = false; fixed totrueBLEConnection.swift
getPacketStream()overwroteconnectionStreamContinuationwithout finishing the old one; addedfinish()before overwriteTests
Added
MeshtasticTests/BLEReconnectTests.swiftcovering UUID validation, RSSI boundary values, state restoration flag, and transport type invariants.