fix(ble): local provider drops devices seen before the first subscriber

src/api/ble/index.ts

@@ -80,6 +80,18 @@ export class BLEApi implements IBLEApi {
   private wsClients: Set<WebSocket> = new Set()
   private localProviders: Map<string, LocalBLEProvider> = new Map() // key = providerId
   private localProviderErrors: Map<string, string> = new Map() // key = adapterName
+  // Tracks an initLocalProviders() attempt still in flight for a given
+  // providerId, from before provider.init() resolves through to
+  // register()/startDiscovery() (or the catch-block rollback). Without
+  // this, two overlapping initLocalProviders() calls for the same adapter
+  // (e.g. start() racing a PUT /settings-triggered reinit) can both pass
+  // the `localProviders.has(providerId)` skip-check before either await
+  // completes, each construct their own LocalBLEProvider, and the later
+  // one's register() ends up displacing the earlier one's registration -
+  // but the earlier LocalBLEProvider instance keeps running its own
+  // discovery loop, now unreferenced by localProviders and therefore
+  // unreachable by shutdown.
+  private localProviderInitializations: Map<string, Promise<void>> = new Map()
   private defaultProviderId: string | null = null
   private settings: BLESettings
   private remoteGatewayProvider: RemoteGatewayProvider | null = null
@@ -176,50 +188,122 @@ export class BLEApi implements IBLEApi {
     for (const adapterName of adapterNames) {
       const providerId = `_localBLE:${adapterName}`
       if (this.localProviders.has(providerId)) continue
-      let provider: LocalBLEProvider | undefined
+
+      // Serialize against a concurrent initLocalProviders() call already
+      // in flight for this same adapter (see
+      // localProviderInitializations's comment) - await its result
+      // instead of racing it with a second LocalBLEProvider construction.
+      const inFlight = this.localProviderInitializations.get(providerId)
+      if (inFlight) {
+        await inFlight
+        continue
+      }
+
+      const initialization = this.initOneLocalProvider(
+        adapterName,
+        providerId
+      )
+      this.localProviderInitializations.set(providerId, initialization)
       try {
-        provider = new LocalBLEProvider(
-          adapterName,
-          this.settings.localMaxGATTSlots,
-          providerId
-        )
-        await provider.init()
-        await provider.startDiscovery()
-        this.localProviders.set(providerId, provider)
-        this.localProviderErrors.delete(adapterName)
-        this.register(providerId, {
-          name: `Local Bluetooth (${adapterName})`,
-          methods: provider.getMethods()
-        })
-        debug.enabled &&
-          debug(`Local BLE provider registered and scanning: ${providerId}`)
-      } catch (e: any) {
-        // Roll back any partial state if init succeeded but startDiscovery failed
-        if (provider) {
-          try {
-            provider.shutdown()
-          } catch (_e) {
-            /* ignore */
-          }
+        await initialization
+      } finally {
+        if (this.localProviderInitializations.get(providerId) === initialization) {
+          this.localProviderInitializations.delete(providerId)
+        }
+      }
+    }
+  }
+
+  private async initOneLocalProvider(adapterName: string, providerId: string) {
+    let provider: LocalBLEProvider | undefined
+    let registeredProvider: BLEProvider | undefined
+    try {
+      provider = new LocalBLEProvider(
+        adapterName,
+        this.settings.localMaxGATTSlots,
+        providerId
+      )
+      await provider.init()
+      this.localProviders.set(providerId, provider)
+      this.localProviderErrors.delete(adapterName)
+      // Register (which wires up onAdvertisement -> _handleAdvertisement,
+      // populating deviceTable) before starting discovery. Discovery's
+      // first pass emits advertisements for every already-known device
+      // synchronously as part of startDiscovery(), and LocalBLEProvider's
+      // emitDeviceAdvertisement drops advertisements entirely when nobody
+      // has subscribed yet - registering afterward silently lost that
+      // whole first batch, leaving those devices absent from the device
+      // table (and therefore unreachable via subscribeGATT) until BlueZ
+      // happened to re-report one of their properties later.
+      registeredProvider = {
+        name: `Local Bluetooth (${adapterName})`,
+        methods: provider.getMethods()
+      }
+      this.register(providerId, registeredProvider)
+      await provider.startDiscovery()
+      debug.enabled &&
+        debug(`Local BLE provider registered and scanning: ${providerId}`)
+    } catch (e: any) {
+      // Roll back any partial state if init and/or register succeeded but
+      // startDiscovery failed. Only touch state that's still this
+      // provider's - localProviderInitializations already prevents a
+      // concurrent initLocalProviders() call for the same adapter from
+      // running at the same time as this one, but the identity checks
+      // stay as defense in depth (e.g. a caller invoking
+      // initOneLocalProvider directly, or a future refactor that removes
+      // the serialization).
+      if (this.localProviders.get(providerId) === provider) {
+        if (this.bleProviders.get(providerId) === registeredProvider) {
+          this.unRegister(providerId)
         }
         this.localProviders.delete(providerId)
-        const msg = `Local BLE adapter ${adapterName} unavailable: ${e.message}`
-        debug(msg)
-        // Suppress console.log for expected "no hardware / no BlueZ" errors
-        const isExpected =
-          e.message?.includes('org.freedesktop.DBus.Error.ServiceUnknown') ||
-          e.message?.includes('not provided by any .service files') ||
-          e.message?.includes('ENOENT') ||
-          e.message?.includes('ECONNREFUSED')
-        if (!isExpected) {
-          console.log(`[BLE API] ${msg}`)
+      }
+      if (provider) {
+        try {
+          provider.shutdown()
+        } catch (_e) {
+          /* ignore */
         }
-        this.localProviderErrors.set(adapterName, e.message)
       }
+      const msg = `Local BLE adapter ${adapterName} unavailable: ${e.message}`
+      debug(msg)
+      // Suppress console.log for expected "no hardware / no BlueZ" errors
+      const isExpected =
+        e.message?.includes('org.freedesktop.DBus.Error.ServiceUnknown') ||
+        e.message?.includes('not provided by any .service files') ||
+        e.message?.includes('ENOENT') ||
+        e.message?.includes('ECONNREFUSED')
+      if (!isExpected) {
+        console.log(`[BLE API] ${msg}`)
+      }
+      this.localProviderErrors.set(adapterName, e.message)
     }
   }
 
   private async shutdownLocalProviders() {
+    // Wait out any initOneLocalProvider() calls still in flight before
+    // tearing anything down. Without this, a call still awaiting
+    // provider.startDiscovery() (DBus) can resolve *after* this method
+    // has already run - at that point it finishes registering/logging
+    // for a provider this method just unregistered and shut down, and
+    // its now-orphaned discovery loop is left running (unreferenced by
+    // localProviders, so a later shutdown can no longer find it either).
+    // Waiting first means initOneLocalProvider's own try/catch has always
+    // fully settled localProviders/bleProviders for a given providerId by
+    // the time the loop below runs, so there's nothing left in flight to
+    // race against.
+    //
+    // A single Promise.all() snapshot isn't enough: initLocalProviders()
+    // could still be running concurrently (e.g. a settings change firing
+    // again mid-shutdown) and add a *new* entry to
+    // localProviderInitializations while this method's own await is
+    // pending, and that new entry wouldn't be in the snapshot. Re-check
+    // and re-await until the map is actually empty, so a fresh entry
+    // added during the wait still gets caught before teardown proceeds.
+    while (this.localProviderInitializations.size > 0) {
+      await Promise.all(this.localProviderInitializations.values())
+    }
+
     for (const [providerId, provider] of this.localProviders) {
       // One misbehaving adapter must not keep the others registered
       try {

src/api/ble/localProvider.ts

@@ -19,10 +19,17 @@ import {
 
 // How often to poll BlueZ for newly discovered devices
 const DEVICE_WATCH_INTERVAL_MS = 5000
-// waitDevice timeout when attaching a listener to an already-known device
-const DEVICE_ATTACH_TIMEOUT_S = 1
-// waitDevice timeout when establishing a GATT connection
-const GATT_CONNECT_TIMEOUT_S = 30
+// waitDevice timeout (ms) when attaching a listener to an already-known
+// device. @naugehyde/node-ble's Adapter#waitDevice races a timeout against
+// a discoveryHandler whose first check() only runs after one full
+// discoveryInterval (default 1000ms) has elapsed, via setInterval rather
+// than an immediate check - so a timeout at or below that interval loses
+// the race almost every time, before a single check can succeed. This must
+// stay comfortably above the default discoveryInterval.
+const DEVICE_ATTACH_TIMEOUT_MS = 5000
+// waitDevice timeout (ms) when establishing a GATT connection. Same
+// discoveryInterval-race constraint as above applies.
+const GATT_CONNECT_TIMEOUT_MS = 30000
 // GATT reconnect exponential backoff bounds
 const RECONNECT_BACKOFF_BASE_MS = 5000
 const RECONNECT_BACKOFF_MAX_MS = 60000
@@ -280,7 +287,7 @@ export class LocalBLEProvider {
     // iterations don't both kick off an attach for the same MAC.
     this.deviceListeners.set(mac, () => {})
     try {
-      const device = await this.adapter.waitDevice(mac, DEVICE_ATTACH_TIMEOUT_S)
+      const device = await this.adapter.waitDevice(mac, DEVICE_ATTACH_TIMEOUT_MS)
       await device.helper._prepare()
       // Discovery may have been stopped while the awaits above ran —
       // registering now would repopulate the cleared listener table
@@ -483,7 +490,7 @@ export class LocalBLEProvider {
       debug.enabled && debug(`GATT connecting to ${mac}`)
 
       // 1. Find device
-      const device = await this.adapter.waitDevice(mac, GATT_CONNECT_TIMEOUT_S)
+      const device = await this.adapter.waitDevice(mac, GATT_CONNECT_TIMEOUT_MS)
       session.device = device
 
       // 2. Connect
@@ -709,7 +716,7 @@ export class LocalBLEProvider {
     let device: any
     try {
       device = await this.connectQueue.enqueue(async () => {
-        const dev = await this.adapter.waitDevice(mac, GATT_CONNECT_TIMEOUT_S)
+        const dev = await this.adapter.waitDevice(mac, GATT_CONNECT_TIMEOUT_MS)
         await dev.helper.callMethod('Connect')
         if (this.scanning) {
           try {