fix(ble): cleanup races discovered while reviewing #5207 (#5221)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
James Rich
2026-04-22 16:22:42 -05:00
committed by GitHub
parent 6547877e7d
commit 20e078d2d7
2 changed files with 32 additions and 6 deletions

View File

@@ -139,10 +139,17 @@ class KableBleConnection(private val scope: CoroutineScope) : BleConnection {
meshtasticDevice.advertisement?.let { adv -> Peripheral(adv) { commonConfig() } }
?: createPeripheral(device.address) { commonConfig() }
cleanUpPeripheral(device.address)
peripheral = p
ActiveBleConnection.active = ActiveConnection(p, device.address)
// Install ownership of the new peripheral atomically. Cancellation between
// peripheral construction and field assignment would strand `p` (Kable allocates
// a per-peripheral scope + Bluetooth-state observer eagerly), so the cleanup,
// assignment, and ActiveBleConnection update must complete as a single unit.
// _deviceFlow.emit() is intentionally outside this block — making it
// non-cancellable could hang teardown on a slow collector.
withContext(NonCancellable) {
cleanUpPeripheral(device.address)
peripheral = p
ActiveBleConnection.active = ActiveConnection(p, device.address)
}
_deviceFlow.emit(device)
@@ -212,11 +219,18 @@ class KableBleConnection(private val scope: CoroutineScope) : BleConnection {
stateJob?.cancel()
stateJob = null
// Capture the peripheral we own before clearing it so we can identity-check
// ActiveBleConnection below. A stale disconnect from an earlier connection
// attempt's exception handler must not clobber a newer connection that has
// already installed itself as active.
val owned = peripheral
safeClosePeripheral("disconnect")
peripheral = null
connectionScope = null
ActiveBleConnection.active = null
if (owned != null && ActiveBleConnection.active?.peripheral === owned) {
ActiveBleConnection.active = null
}
_deviceFlow.emit(null)
}

View File

@@ -103,9 +103,17 @@ class BleRadioTransport(
internal val address: String,
) : RadioTransport {
// Detached cleanup scope for last-ditch GATT teardown from the exception handler.
// Must NOT be a child of `scope`: when an uncaught exception fires in connectionScope,
// upper layers often tear down `scope` immediately. Launching cleanup on `scope` then
// races the cancellation and may never start, leaking BluetoothGatt (status 133 on
// the next reconnect). This scope is cancelled in close() once our own disconnect
// has completed and the safety net is no longer needed.
private val cleanupScope: CoroutineScope = CoroutineScope(SupervisorJob() + scope.coroutineContext.minusKey(Job))
private val exceptionHandler = CoroutineExceptionHandler { _, throwable ->
Logger.w(throwable) { "[$address] Uncaught exception in connectionScope" }
scope.launch {
cleanupScope.launch {
try {
bleConnection.disconnect()
} catch (e: Exception) {
@@ -427,6 +435,10 @@ class BleRadioTransport(
Logger.w(e) { "[$address] Failed to disconnect in close()" }
}
}
// Our own disconnect succeeded — the exception-handler safety net is no longer
// needed. Cancel the detached cleanup scope so it doesn't outlive us in tests
// or process lifetime.
cleanupScope.cancel("close() called")
}
private fun dispatchPacket(packet: ByteArray) {