Skip to content

fix(DCMAW-21804): Advertise after opening gatt server (#522) #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
covolution wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(DCMAW-21804): Advertise after opening gatt server (#522) #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .../src/main/kotlin/uk/gov/onelogin/sharing/bluetooth/api/gatt/peripheral/GattServerEvent.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import uk.gov.onelogin.sharing.bluetooth.internal.core.SessionEndStates
sealed interface GattServerEvent {
data class Connected(val address: String) : GattServerEvent
data class Disconnected(val address: String, val isSessionEnd: Boolean) : GattServerEvent
data class ServiceAdded(val service: BluetoothGattService?) : GattServerEvent
data class ServiceAdded(val service: BluetoothGattService) : GattServerEvent

@gemini-code-assist gemini-code-assist Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Revert the service property to be nullable to safely handle cases where the GATT service registration fails and the Android framework passes null.

Suggested change
data class ServiceAdded(val service: BluetoothGattService) : GattServerEvent
data class ServiceAdded(val service: BluetoothGattService?) : GattServerEvent

data class MessageReceived(val byteArray: ByteArray) : GattServerEvent {
override fun equals(other: Any?): Boolean {
if (this === other) return true
Expand Down
5 changes: 5 additions & 0 deletions ...rc/main/kotlin/uk/gov/onelogin/sharing/bluetooth/api/gatt/peripheral/GattServerManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,10 @@ interface GattServerManager :

fun open(serviceUuid: UUID)

/**
* Disconnects the currently connected bluetooth device.
*/
fun cancelCurrentConnection()

fun notifySessionEnd(serviceUuid: UUID): SessionEndStateQueued
}
2 changes: 1 addition & 1 deletion ...th/src/main/kotlin/uk/gov/onelogin/sharing/bluetooth/api/peripheral/GattServerCallback.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ class GattServerCallback(
}
}

override fun onServiceAdded(status: Int, service: BluetoothGattService?) {
override fun onServiceAdded(status: Int, service: BluetoothGattService) {

@gemini-code-assist gemini-code-assist Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Reverting BluetoothGattService? to a non-nullable BluetoothGattService in onServiceAdded is risky. In the Android BLE framework, if service registration fails (i.e., status != GATT_SUCCESS), the framework can pass null for the service parameter.

Since Kotlin enforces strict null-safety checks on non-nullable parameters for overridden platform methods, receiving a null value at runtime will trigger an immediate NullPointerException before the method body even executes, causing the application to crash.

Please revert this parameter to be nullable.

Suggested change
override fun onServiceAdded(status: Int, service: BluetoothGattService) {
override fun onServiceAdded(status: Int, service: BluetoothGattService?) {

gatGattEventEmitter.emit(
GattServerCallbackEvent.ServiceAdded(status, service)
)
Expand Down
2 changes: 1 addition & 1 deletion ...c/main/kotlin/uk/gov/onelogin/sharing/bluetooth/api/peripheral/GattServerCallbackEvent.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ sealed interface GattServerCallbackEvent {
}
}

data class ServiceAdded(val status: Int, val service: BluetoothGattService?) :
data class ServiceAdded(val status: Int, val service: BluetoothGattService) :

@gemini-code-assist gemini-code-assist Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Revert the service property to be nullable to match the nullability of the parameter in GattServerCallback.onServiceAdded and prevent potential runtime crashes when service registration fails.

Suggested change
data class ServiceAdded(val status: Int, val service: BluetoothGattService) :
data class ServiceAdded(val status: Int, val service: BluetoothGattService?) :

GattServerCallbackEvent
data class MessageReceived(val device: BluetoothDevice, val byteArray: ByteArray) :
GattServerCallbackEvent {
Expand Down
63 changes: 55 additions & 8 deletions ...gov/onelogin/sharing/bluetooth/api/peripheral/mdoc/AndroidPeripheralBluetoothTransport.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ class AndroidPeripheralBluetoothTransport(

internal var monitoringJob: Job = monitorServerEvents()

@Volatile
internal var isServiceReady: Boolean = false

private fun cancelCurrentJobs() {
monitoringJob.cancel()
monitoringJob = monitorServerEvents()
Expand Down Expand Up @@ -87,8 +90,13 @@ class AndroidPeripheralBluetoothTransport(
ioDispatcher + "$logTag.Start".asCoroutineName()
) {
cancelCurrentJobs()
isServiceReady = false
_state.value = PeripheralBluetoothState.Idle

gattServerManager.open(serviceUuid)
}

private suspend fun startAdvertising(serviceUuid: UUID) {
monitoringJob.start()
bluetoothStateMonitor.start()
try {
Expand All @@ -98,8 +106,6 @@ class AndroidPeripheralBluetoothTransport(
_state.value =
PeripheralBluetoothState.Error(PeripheralBluetoothTransportError.ADVERTISING_FAILED)
}

gattServerManager.open(serviceUuid)
}

override suspend fun stop(serviceUuid: UUID, sendEndCommand: Boolean): Unit =
Expand All @@ -108,9 +114,9 @@ class AndroidPeripheralBluetoothTransport(
if (sendEndCommand) {
notifySessionEnd(serviceUuid)
}
bluetoothStateMonitor.stop()
bleAdvertiser.stopAdvertise()
gattServerManager.close()
bluetoothStateMonitor.stop()
_state.value = PeripheralBluetoothState.Idle
}

Expand All @@ -125,16 +131,57 @@ class AndroidPeripheralBluetoothTransport(
}

private fun handleAdvertiserState(state: AdvertiserState) {
if (state is AdvertiserState.Failed) {
_state.value =
PeripheralBluetoothState.Error(
PeripheralBluetoothTransportError.ADVERTISING_FAILED
)
when (state) {
is AdvertiserState.Started -> {
isServiceReady = true
}

AdvertiserState.Stopping,
is AdvertiserState.Stopped -> {
isServiceReady = false
}

is AdvertiserState.Failed -> {
_state.value =
PeripheralBluetoothState.Error(
PeripheralBluetoothTransportError.ADVERTISING_FAILED
)
}
Comment on lines +144 to +149

@gemini-code-assist gemini-code-assist Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When the advertiser state transitions to AdvertiserState.Failed, isServiceReady is not updated. If advertising fails, the service is no longer ready to accept connections, so isServiceReady should be explicitly set to false to prevent accepting any incoming connections in an invalid state.

Suggested change
is AdvertiserState.Failed -> {
_state.value =
PeripheralBluetoothState.Error(
PeripheralBluetoothTransportError.ADVERTISING_FAILED
)
}
is AdvertiserState.Failed -> {
isServiceReady = false
_state.value =
PeripheralBluetoothState.Error(
PeripheralBluetoothTransportError.ADVERTISING_FAILED
)
}


AdvertiserState.Idle,
AdvertiserState.Starting
-> {
// do nothing with intermediary advertisement states
}
}

logger.debug(logTag, "Advertising ${state::class.java.simpleName}")
}

private fun handleGattEvent(event: GattServerEvent) {
when (event) {
is GattServerEvent.Connected -> {
if (!isServiceReady) {
logger.debug(
logTag,
"Rejecting connection from ${event.address} - service not ready"
)
gattServerManager.cancelCurrentConnection()
}
}

is GattServerEvent.ServiceAdded -> {
coroutineScope.launch(
ioDispatcher + "$logTag.StartAdvertising".asCoroutineName()
) {
startAdvertising(event.service.uuid)
}
}
Comment on lines +173 to +179

@gemini-code-assist gemini-code-assist Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In handleGattEvent, when GattServerEvent.ServiceAdded is received, a coroutine is launched on coroutineScope (which is the ApplicationScope). Since coroutineScope is not cancelled when stop() is called, this coroutine can continue running even after the transport has been stopped. If stop() is called while startAdvertising is starting, it can lead to a race condition where advertising is started after the GATT server has been closed.

To fix this, we should launch the coroutine using a scope that is tied to the lifecycle of monitoringJob. We can create a CoroutineScope using coroutineScope.coroutineContext + monitoringJob so that the coroutine is automatically cancelled when monitoringJob is cancelled during stop().

Suggested change
is GattServerEvent.ServiceAdded -> {
coroutineScope.launch(
ioDispatcher + "$logTag.StartAdvertising".asCoroutineName()
) {
startAdvertising(event.service.uuid)
}
}
is GattServerEvent.ServiceAdded -> {
CoroutineScope(coroutineScope.coroutineContext + monitoringJob).launch(
ioDispatcher + "$logTag.StartAdvertising".asCoroutineName()
) {
startAdvertising(event.service.uuid)
}
}

Comment on lines +163 to +179

@gemini-code-assist gemini-code-assist Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This block contains two significant issues:

  1. State Leak on Rejected Connection: When !isServiceReady is true, we cancel the connection but do not prevent the event from being transformed and updating _state.value to PeripheralBluetoothState.Connected. This causes a brief, incorrect state transition to Connected before the subsequent disconnection event is processed. Returning early when rejecting the connection prevents this.
  2. Race Condition / Unsupervised Coroutine: Launching the startAdvertising coroutine on the application-level coroutineScope means it is not cancelled when stop() is called (which only cancels monitoringJob). If stop() is called while StartAdvertising is pending, the advertiser could be started after the transport has stopped, leaving it running indefinitely. Launching it on a scope tied to monitoringJob ensures it is automatically cancelled when stop() is called.

Additionally, we use a safe call (?.let) on event.service to handle the reverted nullable type safely.

Suggested change
is GattServerEvent.Connected -> {
if (!isServiceReady) {
logger.debug(
logTag,
"Rejecting connection from ${event.address} - service not ready"
)
gattServerManager.cancelCurrentConnection()
}
}
is GattServerEvent.ServiceAdded -> {
coroutineScope.launch(
ioDispatcher + "$logTag.StartAdvertising".asCoroutineName()
) {
startAdvertising(event.service.uuid)
}
}
is GattServerEvent.Connected -> {
if (!isServiceReady) {
logger.debug(
logTag,
"Rejecting connection from ${event.address} - service not ready"
)
gattServerManager.cancelCurrentConnection()
return
}
}
is GattServerEvent.ServiceAdded -> {
event.service?.let { service ->
CoroutineScope(ioDispatcher + monitoringJob + "$logTag.StartAdvertising".asCoroutineName()).launch {
startAdvertising(service.uuid)
}
}
}


else -> {
// don't perform additional logic for other events
}
}
event.let(serverEventTransformer::transform)?.let { bluetoothState ->
_state.value = bluetoothState
}.also {
Expand Down
2 changes: 1 addition & 1 deletion ...elogin/sharing/bluetooth/api/peripheral/mdoc/GattServerEventToPeripheralBluetoothState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class GattServerEventToPeripheralBluetoothState(private val logger: Logger) :
)

is GattServerEvent.ServiceAdded -> {
logger.debug(logTag, "Service Added: ${source.service?.uuid}")
logger.debug(logTag, "Service Added: ${source.service.uuid}")

@gemini-code-assist gemini-code-assist Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Revert to using a safe call (?.) since service should remain nullable to prevent potential null pointer exceptions under error conditions.

Suggested change
logger.debug(logTag, "Service Added: ${source.service.uuid}")
logger.debug(logTag, "Service Added: ${source.service?.uuid}")

null
}

Expand Down
34 changes: 11 additions & 23 deletions .../kotlin/uk/gov/onelogin/sharing/bluetooth/internal/peripheral/AndroidGattServerManager.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class AndroidGattServerManager(
)
override val events: SharedFlow<GattServerEvent> = _events
private var gattServer: BluetoothGattServer? = null
private var connectedDevice: BluetoothDevice? = null
internal var connectedDevice: BluetoothDevice? = null

@gemini-code-assist gemini-code-assist Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The connectedDevice property is read and written across multiple threads (e.g., the Android Bluetooth binder threads in handleConnectionStateChange and the application's ioDispatcher threads in cancelCurrentConnection and sendMessage). To ensure thread safety and proper visibility of updates across threads, connectedDevice should be marked with @Volatile.

Suggested change
internal var connectedDevice: BluetoothDevice? = null
@Volatile
internal var connectedDevice: BluetoothDevice? = null


@SuppressLint("MissingPermission")
private val eventEmitter = GattEventEmitter {
Expand All @@ -64,12 +64,8 @@ class AndroidGattServerManager(
private var mtu = MIN_MTU
private var isSessionEnd = false

@Volatile
private var isServiceReady = false

@RequiresPermission(Manifest.permission.BLUETOOTH_CONNECT)
override fun open(serviceUuid: UUID) {
isServiceReady = false
val gattService = gattServiceFactory(serviceUuid)

if (permissionsChecker.checkPermissions(getBluetoothPermissions()).isNotEmpty()) {
Expand Down Expand Up @@ -108,7 +104,6 @@ class AndroidGattServerManager(
gattServer = null
connectedDevice = null
isSessionEnd = false
isServiceReady = false
mtu = MIN_MTU
_events.tryEmit(GattServerEvent.ServiceStopped)
}
Expand Down Expand Up @@ -137,27 +132,23 @@ class AndroidGattServerManager(
_events.tryEmit(GattServerEvent.MessageReceived(event.byteArray))
}

@SuppressLint("MissingPermission")
override fun cancelCurrentConnection() {
connectedDevice?.let {
gattServer?.cancelConnection(it)
connectedDevice = null
}
}

@SuppressLint("MissingPermission")
private fun handleConnectionStateChange(event: GattServerCallbackEvent.ConnectionStateChange) {
val address = event.device.address

val event = when {
event.status == BluetoothGatt.GATT_SUCCESS &&
event.newState == BluetoothProfile.STATE_CONNECTED -> {
// Reject connections that arrive before addService() completes.
// This happens when a device from a previous session is still actively connecting
// Android routes it to the new GATT server immediately on openGattServer(),
// before the service is registered.
if (!isServiceReady) {
logger.debug(
logTag,
"Rejecting connection from $address - service not ready"
)
gattServer?.cancelConnection(event.device)
return
}
connectedDevice = event.device
GattServerEvent.Connected(address)
GattServerEvent.Connected(event.device.address)
}

event.newState == BluetoothProfile.STATE_DISCONNECTED -> {
Expand Down Expand Up @@ -188,11 +179,8 @@ class AndroidGattServerManager(

private fun handleServiceAdded(event: GattServerCallbackEvent.ServiceAdded) {
if (event.status == BluetoothGatt.GATT_SUCCESS) {
isServiceReady = true
_events.tryEmit(
GattServerEvent.ServiceAdded(
event.service
)
GattServerEvent.ServiceAdded(event.service)
)
} else {
logger.error(logTag, "Failed to add service, status: ${event.status}")
Expand Down
Loading
Loading