fix(ble): ensure GATT cleanup runs under NonCancellable on cancellation#5207
Open
jdogg172 wants to merge 3 commits intomeshtastic:mainfrom
Open
fix(ble): ensure GATT cleanup runs under NonCancellable on cancellation#5207jdogg172 wants to merge 3 commits intomeshtastic:mainfrom
jdogg172 wants to merge 3 commits intomeshtastic:mainfrom
Conversation
…on discoverServicesAndSetupCharacteristics() caught bare Exception which swallowed CancellationException, causing disconnect() to be skipped when the scope was cancelled. Skipping disconnect() leaks the BluetoothGatt handle and reliably triggers GATT status 133 on the next reconnect attempt. Now re-throws CancellationException after running disconnect() under NonCancellable, and also wraps the error-path disconnect() in NonCancellable to guard against late cancellation during cleanup. Also mark KableBleConnection.peripheral/stateJob/connectionScope as @volatile to ensure cross-thread visibility between connect() and disconnect() callers.
…fix Tests verify: - close() calls disconnect() to release GATT handle (prevents status 133 on reconnect) - disconnect() is called when profile setup throws CancellationException - disconnect() is called on connection failure - only transient (non-permanent) onDisconnect is signalled after failure threshold All 4 new tests pass alongside 104 existing tests (BUILD SUCCESSFUL).
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the BLE reconnection path by ensuring GATT cleanup (disconnect) still runs when coroutines are cancelled, preventing leaked BluetoothGatt handles that can lead to GATT status 133 on subsequent reconnects. It also adds tests targeting the reconnect crash scenarios and improves cross-thread visibility in the Kable-based BLE connection implementation.
Changes:
- Ensure
BleRadioTransport.discoverServicesAndSetupCharacteristics()disconnects underNonCancellableand rethrowsCancellationException. - Add
@Volatileto key fields inKableBleConnectionto improve cross-thread visibility between connect/disconnect callers. - Add
BleRadioTransportReconnectCrashTestwith coverage for cleanup and reconnect signaling behaviors.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt | Ensures disconnect cleanup runs under NonCancellable, including the cancellation path. |
| core/ble/src/commonMain/kotlin/org/meshtastic/core/ble/KableBleConnection.kt | Adds @Volatile to shared mutable fields used across connect/disconnect. |
| core/network/src/commonTest/kotlin/org/meshtastic/core/network/radio/BleRadioTransportReconnectCrashTest.kt | Adds regression tests for cancellation + failure cleanup and transient disconnect behavior. |
Comment on lines
+94
to
+98
| @Volatile private var peripheral: Peripheral? = null | ||
|
|
||
| @Volatile private var stateJob: Job? = null | ||
|
|
||
| @Volatile private var connectionScope: CoroutineScope? = null |
Author
There was a problem hiding this comment.
Fixed: added import kotlin.concurrent.Volatile at the top of KableBleConnection.kt so the annotation resolves correctly in commonMain.
Comment on lines
+64
to
+65
| private val testScope = TestScope() | ||
| private val scanner = FakeBleScanner() |
Author
There was a problem hiding this comment.
Fixed: removed the unused testScope = TestScope() field. Also restored the SharedFlow, asSharedFlow, and advanceTimeBy imports that the CancellingProfileBleConnection inner class and virtual-time tests depend on.
- KableBleConnection: add missing kotlin.concurrent.Volatile import (was causing compile error in commonMain) - BleRadioTransportReconnectCrashTest: remove unused testScope field; restore SharedFlow/asSharedFlow/advanceTimeBy imports that were needed by CancellingProfileBleConnection inner class
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes 2 crash-level bugs in the Android BLE reconnection stack.
Changes
BleRadioTransport.kt
discoverServicesAndSetupCharacteristics()had a barecatch (e: Exception)that swallowedCancellationException, causingdisconnect()to be skipped when the scope was cancelled. Skippingdisconnect()leaks the BluetoothGatt handle and reliably triggers GATT status 133 on the next reconnect. Now re-throwsCancellationExceptionafter runningdisconnect()underNonCancellable. Error path disconnect also wrapped inNonCancellable.KableBleConnection.kt
@Volatiletoperipheral,stateJob, andconnectionScopefields to ensure cross-thread visibility betweenconnect()anddisconnect()callers.Tests
Added
BleRadioTransportReconnectCrashTestwith 4 new tests covering GATT handle cleanup, CancellationException path, failure disconnect, and transient-only disconnect signalling. All 108 tests pass (BUILD SUCCESSFUL on JVM).