Skip to content

fix(bindings): null-guard BluetoothDevice.connectGatt return in connectToDevice - #280

Merged
bahdotsh merged 1 commit into
mainfrom
mb/upstream-off-1985-connectgatt-null-guard
Aug 5, 2026
Merged

fix(bindings): null-guard BluetoothDevice.connectGatt return in connectToDevice#280
bahdotsh merged 1 commit into
mainfrom
mb/upstream-off-1985-connectgatt-null-guard

Conversation

@taayyohh

@taayyohh taayyohh commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Problem

BluetoothDevice.connectGatt is declared @Nullable in the Android framework. Kotlin treats the return as a platform type, but MeshConnectionRegistry.registerGatt takes a non-null BluetoothGatt, so a null return trips the compiler-inserted Intrinsics.checkNotNullParameter and crashes with NullPointerException.

connectGatt returns null when the adapter has just turned off, when the underlying hardware handle is stale, or when the device unbonded between scan and connect — the same adapter-race conditions that already motivate the SecurityException handler at the tail of connectToDevice.

Fix

Guard the connectGatt return before it reaches registerGatt. On null:

  • Log at info and emit an info-level diagnostic (adapter-off is a user action, not an SDK error).
  • Call connections.consumePendingRole(device.address) to release the pending role reserved for this address. This mirrors the RSSI-skip, connection-cap, and SecurityException paths — without it, the pending-role reservation leaks and blocks future connection attempts to that address.
  • return out of the synchronized(connectionLock) { ... } block, matching the early-return convention used by the existing skip paths in the same block.

Why this is a real crash and not theoretical

Same adapter-off race as the IllegalStateException scan-site crashes filed as a sibling PR. Three users, three events on the pre-patch version of the MINE app before this shipped as a patch-package override.

Notes

…ctToDevice

BluetoothDevice.connectGatt is declared @nullable in the Android
framework. Kotlin treats the return as a platform type, but
MeshConnectionRegistry.registerGatt takes a non-null BluetoothGatt, so a
null return trips the compiler-inserted Intrinsics.checkNotNullParameter
and crashes with NullPointerException.

connectGatt returns null when the adapter has just turned off, when the
underlying hardware handle is stale, or when the device unbonded between
scan and connect — the same adapter-race conditions that motivate the
SecurityException handler already present at the tail of this function.

Guard the return, release the pending role reserved for this address
(mirroring the RSSI-skip, connection-cap, and SecurityException paths so
state does not hang half-set-up), and emit an info diagnostic so callers
can see how often the race fires in the field.

Observed impact: 3 users, 3 events on the pre-patch version of the MINE
app before this shipped as a patch-package override. Same adapter-off
race as the IllegalStateException scan-site crashes.

Retires the MINE-side patch tracked by Linear OFF-1985.
@linear

linear Bot commented Aug 4, 2026

Copy link
Copy Markdown
OFF-1985 mesh-sdk: BleTransportFacade.connectToDevice NPE — connectGatt platform-nullable

Upstream request for Gokul (mesh-sdk maintainer)

Repro / observation

On v1.0.14 (mesh-sdk 0.12.0), Play Console reports 3 users / 3 events hitting:

NullPointerException:
  at com.offlineprotocol.ble.BleTransportFacade.connectToDevice (BleTransportFacade.kt:2099)
  at com.offlineprotocol.ble.BleTransportFacade.handleScanResult (BleTransportFacade.kt:1792)
  at com.offlineprotocol.ble.BleTransportFacade.access$handleScanResult (BleTransportFacade.kt:152)
  at BleTransportFacade$startScanning$1.onScanResult$lambda$0 (BleTransportFacade.kt:1356)
  at BleTransportFacade$startScanning$1$$ExternalSyntheticLambda2.run

Flow: BLE scan callback fires → onScanResult$lambda$0 posts a Runnable to a Handler → handleScanResultconnectToDevice at line 2099 crashes with NPE.

Root cause

BleTransportFacade.connectToDevice at lines 2098–2099:

val gatt = device.connectGatt(context, false, centralClient.callback, BluetoothDevice.TRANSPORT_LE)
connections.registerGatt(device.address, gatt)  // line 2099 — NPE here

BluetoothDevice.connectGatt() is declared @Nullable BluetoothGatt in the Android framework — the documented contract is that it returns null when the adapter is off, the hardware handle is stale, or the device unbonded between scan and connect (see e.g. Android source).

Kotlin treats this as a platform type (BluetoothGatt!), but MeshConnectionRegistry.registerGatt(address: String, gatt: BluetoothGatt) declares its second parameter as non-null. The Kotlin compiler inserts Intrinsics.checkNotNullParameter(gatt, "gatt") at the call site — so when connectGatt returns null, the intrinsic throws NullPointerException before registerGatt even executes.

This is the same class of adapter race that motivated OFF-1896 (BT adapter flips off between the isScanning check and the framework call, or the discovered device unbonds between the scan callback and the connect attempt), but on the connect path rather than the scan path — different code path, different exception class, requires its own guard.

Suggested fix (upstream, minimal)

Null-guard the return of connectGatt in connectToDevice:

val gatt = device.connectGatt(context, false, centralClient.callback, BluetoothDevice.TRANSPORT_LE)
if (gatt == null) {
    Log.i(TAG, "connectGatt returned null for ${device.address} — adapter unavailable")
    connections.consumePendingRole(device.address)  // release the pending-connection slot reserved above
    return
}
connections.registerGatt(device.address, gatt)

The consumePendingRole(device.address) call mirrors the cleanup already done in the sibling SecurityException catch below at line 2107 — without it, the pending role reserved earlier in connectToDevice hangs half-set-up and blocks the next connect attempt to the same address.

Alternative shapes: change MeshConnectionRegistry.registerGatt to accept BluetoothGatt? and no-op on null, or wrap the connectGatt return in a ?.let { ... }. The above is the least invasive.

MINE-side interim (documented exception under CLAUDE.md carve-out)

Extending our existing patches/offline-protocol+mesh-sdk+0.12.0.patch with this guard as a third fix family alongside OFF-1801 (foreground-service) and OFF-1896 (scan-site IllegalStateException). Retire the patch line when mesh-sdk 0.12.x ships the guard upstream.

Related

Review in Linear

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@taayyohh

taayyohh commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

recheck

@taayyohh

taayyohh commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

bahdotsh added a commit that referenced this pull request Aug 5, 2026
The notification Stop action added earlier in this branch runs the
same teardown as the JS-facing stop(), except it does it on its own
thread. Nothing was stopping the two from overlapping, and a user
tapping Stop while the app foregrounds and calls stop() is not an
exotic scenario.

Two threads interleaving through stopTransportsAndProtocol
double-stop every transport mid-pass. Worse, if both throw — and BLE
teardown throwing on real devices is the entire reason #279 and #280
exist — neither pass ever reaches the remaining transports, the
keep-alive service, or the protocol core. The user-stop path then
clears the notification and tells JS the mesh is down while five
transports are still burning battery.

That is precisely the lie this branch was written to prevent.

The JS path had the same hole without needing a race at all: one
throwing transport skipped the foreground-service stop and
protocol.stop() outright, so the notification kept advertising an
active mesh over a half-dead stack.

So serialize the shared teardown, and move the keep-alive and core
shutdown into a finally. The second entrant through the lock re-runs
the stops after a completed pass, which is their idempotent no-op
path. The exception still propagates, so stop() rejects exactly as
it did before.

While at it, drop the now-redundant keep-alive stop from the
user-stop path — the shared function guarantees it now.
bahdotsh added a commit that referenced this pull request Aug 5, 2026
…e + add Stop action (#278)

* fix(bindings): promote MeshForegroundService to foreground in onCreate

Android gives an app 5 seconds after startForegroundService() to reach
startForeground(). MeshForegroundService currently calls startForeground()
from onStartCommand, which on cold start and on resume can be delayed past
that window by JS-thread initialisation and main-thread work. When that
happens the OS terminates the process with a fatal RemoteServiceException.

Move the startForeground() call into onCreate so the deadline is
unreachable regardless of what runs on the main thread after service
creation. The subsequent startForeground() calls in onStartCommand are
idempotent on the same service instance and are safe re-promotes.

Also add a "Stop" notification action so the user can shut mesh down from
the notification shade. It routes back through the service's own
ACTION_STOP handler via PendingIntent.getForegroundService on Android O+
(getService on older releases), so no separate BroadcastReceiver is
required and the delivery is legal under the background service-start
restrictions that apply when the user taps the action while the app is in
the background.

Observed impact: this fix has been shipping in-app via a patch-package
override since v67 of the MINE app; the underlying crash was the top
ANR-adjacent fatal on Redmi Note 8 Pro (Android 11), fingerprinted by
Sentry as REACT-NATIVE-5Z.

Retires the MINE-side patch tracked by Linear OFF-1801.

* fix(bindings): make the mesh notification Stop action stop the mesh

The Stop action added alongside the onCreate promotion routes
straight back into the service's own ACTION_STOP handler, which
drops the keep-alive and nothing else.

But this service is *only* a keep-alive — the module owns the
protocol and the transports, exactly as the class comment has
said all along. So tapping Stop cleared the notification and the
foreground protection while BLE, WiFi Direct, Nostr and the
process scheduler kept right on running, with nothing told to
JS. The user sees "mesh off" while the radios keep draining the
battery until the OS gets around to reaping the process.

Give the notification its own action that hands off to a host
callback instead. The module runs the same teardown as its
JS-facing stop() and emits mesh_stopped_by_user so app state
can't silently diverge, and the service stays up until that
teardown comes back around through ACTION_STOP — clearing the
notification while the mesh is still running is exactly the lie
we're fixing here. With no host registered we still drop the
keep-alive, because a dead button is its own kind of bug.

While at it, the try/catch only wrapped the onCreate promotion;
the other two call sites were bare. That matters, because a
connectedDevice promotion throws once the Nearby-Devices
permissions are revoked — so the guarded failure came straight
back uncaught a few milliseconds later. All three share a helper
now. It is not immunity: if the instance came from
startForegroundService() and promotion genuinely fails, the
system still raises its own timeout kill, and the helper says so.
And stop() is a no-op when nothing is running, since creating an
instance purely to tear it down now means a notification flash on
a path the app already runs twice.

The new event tag is emitted by the bridge, not by the core enum,
so it joins the bridge-only allowlist in the types.ts drift
guard. That guard is doing exactly what it was built for — it
caught the omission before I did.

The tests pin the part that is invisible in the source: the
service is in the foreground by the end of onCreate, with no
start command delivered. Move that promotion back into
onStartCommand and the code still reads fine while the
five-second deadline is quietly reachable again.

* fix(bindings): serialize mesh teardown and always run its tail

The notification Stop action added earlier in this branch runs the
same teardown as the JS-facing stop(), except it does it on its own
thread. Nothing was stopping the two from overlapping, and a user
tapping Stop while the app foregrounds and calls stop() is not an
exotic scenario.

Two threads interleaving through stopTransportsAndProtocol
double-stop every transport mid-pass. Worse, if both throw — and BLE
teardown throwing on real devices is the entire reason #279 and #280
exist — neither pass ever reaches the remaining transports, the
keep-alive service, or the protocol core. The user-stop path then
clears the notification and tells JS the mesh is down while five
transports are still burning battery.

That is precisely the lie this branch was written to prevent.

The JS path had the same hole without needing a race at all: one
throwing transport skipped the foreground-service stop and
protocol.stop() outright, so the notification kept advertising an
active mesh over a half-dead stack.

So serialize the shared teardown, and move the keep-alive and core
shutdown into a finally. The second entrant through the lock re-runs
the stops after a completed pass, which is their idempotent no-op
path. The exception still propagates, so stop() rejects exactly as
it did before.

While at it, drop the now-redundant keep-alive stop from the
user-stop path — the shared function guarantees it now.

* test(bindings): stop leaking the start-request flag between tests

The service tracks start intent in a companion field, and onDestroy
is what clears it. A test that calls start() without ever creating
an instance therefore leaves that flag set for whatever runs next —
there is no service instance for the fixture to destroy.

Nothing is red today, because the one test that cares re-establishes
its own precondition. That is luck, not design, and it lasts exactly
until someone adds a test between those two.

So just call stop() in the fixture. It no-ops once both flags are
already down, which costs nothing on every other test.

---------

Co-authored-by: bahdotsh <appu.yess@gmail.com>
@bahdotsh
bahdotsh merged commit 5f886d4 into main Aug 5, 2026
14 of 15 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 5, 2026
@bahdotsh
bahdotsh deleted the mb/upstream-off-1985-connectgatt-null-guard branch August 13, 2026 06:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants