fix(ble): stop a missing D-Bus socket killing the server

.github/workflows/plugin-ci.yml

@@ -536,6 +536,30 @@ jobs:
           cat << 'LIFECYCLE' > /tmp/check-lifecycle.js
           const path = require('path');
           const pkg = require(process.cwd() + '/package.json');
+
+          // ── Async crash trap ─────────────────────────────────────
+          // A plugin can start cleanly and still take the server down a
+          // moment later: an 'error' event emitted on a socket/emitter with
+          // no listener, or a floating promise that rejects after start()
+          // has already resolved. Neither travels the promise path, so the
+          // try/catch around `await plugin.start({})` below cannot see them.
+          //
+          // In production the server has no such trap either — Node's
+          // default action for an unhandled 'error' event is to throw, and
+          // under systemd Restart=always the result is an endless
+          // crash-restart loop rather than one bad plugin.
+          //
+          // Recording them here (rather than dying immediately) lets the
+          // lifecycle run finish and report every problem it found, which
+          // is more useful than surfacing whichever one happened to fire
+          // first. Anything caught is fatal — see the report below.
+          const asyncCrashes = [];
+          process.on('uncaughtException', (e) => {
+            asyncCrashes.push('uncaught exception: ' + ((e && e.message) || String(e)));
+          });
+          process.on('unhandledRejection', (e) => {
+            asyncCrashes.push('unhandled rejection: ' + ((e && e.message) || String(e)));
+          });
           // See validate-entry.js for the rationale — object/conditional
           // exports must be resolved, not flattened to 'index.js'.
           function resolveEntry(p, fallback) {
@@ -805,6 +829,40 @@ jobs:
             process.exit(1);
           }
 
+          // ── Report async crashes ────────────────────────────────
+          // Let queued failures surface before deciding the verdict. A
+          // microtask turn would cover an 'error' already sitting on the
+          // queue, but the failures worth catching are slower than that: a
+          // socket connect, a D-Bus handshake or a DNS lookup started during
+          // start() typically fails a second or so in, which is exactly the
+          // "started fine, died shortly after" shape this check exists for.
+          //
+          // 1.5s covers those while staying far inside the step's 2-minute
+          // budget. It is a fixed cost on every run, so it is not raised
+          // further: this drains what start() already kicked off, and is not
+          // meant to wait out a slow remote endpoint.
+          const ASYNC_CRASH_DRAIN_MS = 1500;
+          await new Promise((resolve) => setTimeout(resolve, ASYNC_CRASH_DRAIN_MS));
+
+          if (asyncCrashes.length > 0) {
+            console.log('');
+            // start() runs twice (initial + restart), so one faulty code path
+            // reports the same message twice. Collapse duplicates — two
+            // identical lines read as two separate problems.
+            [...new Set(asyncCrashes)].forEach(c => console.log('::error::Lifecycle: ' + c));
+            console.log('');
+            console.log('The plugin crashed the process after start() had already');
+            console.log('returned. The server does not sandbox plugins: an unhandled');
+            console.log("'error' event or a rejected floating promise terminates the");
+            console.log('whole server, and under systemd it then restart-loops.');
+            console.log('');
+            console.log('Common cause: an emitter or socket used without an error');
+            console.log("listener. Attach one ('error' handlers are required, not");
+            console.log('optional), or await the operation so the failure reaches the');
+            console.log('try/catch in start().');
+            process.exit(1);
+          }
+
           console.log('Plugin lifecycle (start/stop/restart) is clean');
           process.exit(0);
           })().catch(e => {
@@ -1895,7 +1953,7 @@ jobs:
             echo "- **package.json** — \`signalk-node-server-plugin\` keyword, \`main\`/\`exports\` field, \`engines.node\`"
             echo "- **Entry point** — Plugin exports a constructor function"
             echo "- **plugin.schema()** — Returns valid JSON Schema without crashing"
-            echo "- **Lifecycle** — start()/stop()/restart cycle works without errors"
+            echo "- **Lifecycle** — start()/stop()/restart cycle works without errors, and the plugin does not crash the process asynchronously afterwards"
             echo "- **API usage** — No deprecated (\`setProviderStatus\`), internal (\`app.server\`), file storage, or security anti-patterns"
             echo "- **Node built-in modules** — \`node:sqlite\` requires \`engines.node >= 22.5.0\` declared in package.json"
             echo "- **npm pack** — All files referenced by \`main\`/\`exports\` are included in the package"

docs/develop/plugins/ci.md

@@ -58,6 +58,8 @@ The desktop jobs (Linux, Linux arm64, macOS, Windows) run these checks, even if
 
 **Lifecycle** — Runs `start()` → `stop()` → `start()` (restart) with an empty configuration. Validates delta messages emitted during startup and checks that `registerDeltaInputHandler` handlers forward deltas correctly.
 
+**Async crashes** — Fails the job if the plugin terminates the process _after_ `start()` has returned — an unhandled `'error'` event or a floating promise that rejects late. See [Crashing the server after start()](#crashing-the-server-after-start) below.
+
 **API usage** — Scans source files for:
 
 - Deprecated APIs (`setProviderStatus` → `setPluginStatus`, `setProviderError` → `setPluginError`)
@@ -175,6 +177,77 @@ test-cerbo-hardware:
     - run: npm test
 ```
 
+## Crashing the server after start()
+
+The server does not sandbox plugins. Everything runs in one Node process, so
+an error your plugin fails to handle does not disable just that plugin — it
+terminates the whole server. Under a process supervisor (`Restart=always`,
+Docker's `restart: unless-stopped`) the server then restart-loops: it comes up,
+runs for a few seconds, dies again, and keeps going indefinitely.
+
+The dangerous case is the one that escapes `try/catch`. Wrapping the body of
+`start()` is not enough, because two common failures arrive _after_ `start()`
+has already returned successfully:
+
+**Unhandled `'error'` events.** In Node, `'error'` is special: an `EventEmitter`
+that emits it with no listener attached throws, and there is nothing up the
+stack to catch it. Sockets, streams, D-Bus connections, serial ports and MQTT
+clients all report failures this way.
+
+```js
+// BAD — start() resolves cleanly, the server dies a moment later
+const client = net.connect('/var/run/some.sock')
+client.on('data', handle)
+
+// GOOD — the failure is handled where it happens
+const client = net.connect('/var/run/some.sock')
+client.on('data', handle)
+client.on('error', (err) => {
+  app.setPluginError(`connection failed: ${err.message}`)
+})
+```
+
+Note that a library opening the connection for you does not remove the
+obligation — if it hands back an emitter and attaches no listener itself, the
+responsibility is still yours.
+
+**Floating promises.** An async call started but never awaited (and with no
+`.catch()`) becomes an unhandled rejection, which terminates the process on
+Node 15 and later.
+
+```js
+// BAD — nothing observes the rejection
+start: () => {
+  connectToDevice()
+}
+
+// GOOD — await it so start()'s own error handling applies...
+start: async () => {
+  try {
+    await connectToDevice()
+  } catch (err) {
+    app.setPluginError(err.message)
+  }
+}
+
+// ...or attach a handler if it must run in the background
+start: () => {
+  connectToDevice().catch((err) => app.setPluginError(err.message))
+}
+```
+
+The general rule: a plugin should report failures through
+`app.setPluginError()` and keep the process alive. Hardware that is absent, a
+socket that is missing, a remote service that is down — these are normal
+conditions on a boat, not reasons to take navigation data offline.
+
+CI installs `uncaughtException` and `unhandledRejection` handlers around the
+lifecycle check and **fails the job** if either fires, including when
+`start()`, `stop()` and restart all report success. The check is an error
+rather than a warning because the consequence is not confined to the plugin:
+one faulty plugin makes the entire server unusable, and the resulting
+crash-loop is difficult for users to trace back to its cause.
+
 ## See also
 
 - [Releases and Changelogs](./release.md) — once CI passes, automate the release cut and publish step.

src/api/ble/index.ts

@@ -15,6 +15,7 @@ import { SignalKMessageHub, WithConfig } from '../../app'
 import WebSocket from 'ws'
 import { writeSettingsFile } from '../../config/config'
 import { LocalBLEProvider } from './localProvider'
+import { createBluetoothSafe } from './safeBluetooth'
 import { RemoteGatewayProvider } from './remoteProvider'
 import { bleVendorName } from './bleCompanyIds'
 
@@ -150,12 +151,16 @@ export class BLEApi implements IBLEApi {
     }
 
     try {
-      // eslint-disable-next-line @typescript-eslint/no-require-imports
-      const { createBluetooth } = require('@naugehyde/node-ble')
-      const { bluetooth, destroy } = createBluetooth()
-      const adapters = await bluetooth.activeAdapters()
-      destroy()
-      return adapters.map((a: any) => a.adapter as string)
+      const { bluetooth, destroy } = createBluetoothSafe()
+      // destroy() in finally: activeAdapters() rejects on a host with no
+      // usable bus, and skipping the teardown would leak the D-Bus
+      // connection every time this probe runs.
+      try {
+        const adapters = await bluetooth.activeAdapters()
+        return adapters.map((a: any) => a.adapter as string)
+      } finally {
+        destroy()
+      }
     } catch {
       return []
     }

src/api/ble/localProvider.ts

@@ -17,6 +17,8 @@ import {
   GATTSubscriptionHandle
 } from '@signalk/server-api'
 
+import { createBluetoothSafe } from './safeBluetooth'
+
 // 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
@@ -113,9 +115,7 @@ export class LocalBLEProvider {
 
   async init(): Promise<void> {
     try {
-      // eslint-disable-next-line @typescript-eslint/no-require-imports
-      const { createBluetooth } = require('@naugehyde/node-ble')
-      const bt = createBluetooth()
+      const bt = createBluetoothSafe()
       this.bluetooth = bt.bluetooth
       this.destroy = bt.destroy
       this.adapter = await this.bluetooth.getAdapter(this.adapterName)

src/api/ble/safeBluetooth.ts

@@ -0,0 +1,71 @@
+/* eslint-disable @typescript-eslint/no-explicit-any */
+/**
+ * Safe wrapper around `@naugehyde/node-ble`'s `createBluetooth()`.
+ *
+ * `createBluetooth()` opens a D-Bus system-bus connection eagerly and returns
+ * synchronously, without attaching an `error` listener to it. The underlying
+ * `@jellybrick/dbus-next` connection reports transport failures by emitting
+ * `error` on that connection rather than by rejecting the pending call, so a
+ * missing or unreachable `/var/run/dbus/system_bus_socket` surfaces as an
+ * `error` event with no listener — which Node escalates into an uncaught
+ * exception. Because it arrives on the event emitter and not through the
+ * promise chain, an `await createBluetooth()...` inside `try/catch` never sees
+ * it: the process dies with `connect ENOENT /var/run/dbus/system_bus_socket`
+ * a few seconds after the server has otherwise started cleanly.
+ *
+ * This happens on any host without a reachable system bus — most commonly a
+ * container that does not bind-mount the socket, but also a stripped-down
+ * Linux install with no D-Bus daemon running.
+ *
+ * Attaching a listener before handing the session back keeps the failure on
+ * the promise path, where the existing `try/catch` blocks already handle it.
+ */
+
+import { createDebug } from '../../debug'
+const debug = createDebug('signalk-server:api:ble:safe')
+
+export interface BluetoothSession {
+  bluetooth: any
+  destroy: () => void
+}
+
+/**
+ * The only part of the dbus-next connection this module touches. node-ble
+ * ships no types, so the surface is declared here rather than pulling in an
+ * `any` and losing the check on the call below.
+ */
+interface ErrorEmitter {
+  on(event: 'error', listener: (err: unknown) => void): void
+}
+
+/**
+ * Creates a node-ble session whose D-Bus connection can never raise an
+ * unhandled `error` event.
+ *
+ * The listener is attached synchronously, before the caller gets a chance to
+ * await anything, so there is no window in which an early transport error can
+ * escape. Errors are logged to debug only: every caller already treats "no
+ * usable adapter" as a normal outcome, and a missing system bus is the
+ * expected case on non-BlueZ hosts rather than something worth logging loudly.
+ */
+export function createBluetoothSafe(): BluetoothSession {
+  // eslint-disable-next-line @typescript-eslint/no-require-imports
+  const { createBluetooth } = require('@naugehyde/node-ble')
+  const session: BluetoothSession = createBluetooth()
+
+  // node-ble keeps the dbus-next connection on the Bluetooth instance; guard
+  // the lookup so an upstream rename degrades to today's behaviour rather
+  // than throwing from inside the safety wrapper itself.
+  const bus = (session.bluetooth as { dbus?: ErrorEmitter } | undefined)?.dbus
+  if (bus && typeof bus.on === 'function') {
+    bus.on('error', (err: unknown) => {
+      debug(
+        `D-Bus system bus error: ${err instanceof Error ? err.message : String(err)}`
+      )
+    })
+  } else {
+    debug('Could not attach D-Bus error listener — node-ble internals changed')
+  }
+
+  return session
+}

test/ble-dbus-error.ts

@@ -0,0 +1,102 @@
+import { expect } from 'chai'
+import { EventEmitter } from 'node:events'
+import Module from 'node:module'
+
+/**
+ * Regression test for the BLE local provider taking the whole server down.
+ *
+ * `@naugehyde/node-ble`'s `createBluetooth()` opens a D-Bus system-bus
+ * connection eagerly and hands it back without an `error` listener. dbus-next
+ * reports transport failures by emitting `error` on that connection, so on a
+ * host with no reachable `/var/run/dbus/system_bus_socket` (any container that
+ * does not mount it) the failure arrives as an unhandled `error` event and
+ * Node escalates it to an uncaught exception — several seconds after the
+ * server has otherwise started cleanly. The `try/catch` around the awaited
+ * calls cannot catch it because it never travels the promise path.
+ *
+ * `createBluetoothSafe()` attaches the listener synchronously, before the
+ * caller can await anything, closing that window.
+ */
+
+// Stand-in for the dbus-next connection: an emitter whose only interesting
+// behaviour is that an unlistened 'error' throws, exactly as Node's does.
+class FakeBus extends EventEmitter {}
+
+// Module.prototype.require is not in @types/node's public surface, so the
+// patch point is described structurally rather than reached through `any`.
+type Requirer = (this: unknown, id: string, ...rest: unknown[]) => unknown
+interface PatchableModule {
+  prototype: { require: Requirer }
+}
+
+const patchable = Module as unknown as PatchableModule
+const requireStub: Requirer = patchable.prototype.require
+
+const withStubbedNodeBle = (fn: () => void) => {
+  patchable.prototype.require = function (
+    this: unknown,
+    id: string,
+    ...rest: unknown[]
+  ) {
+    if (id === '@naugehyde/node-ble') {
+      return {
+        createBluetooth: () => {
+          const dbus = new FakeBus()
+          return { bluetooth: { dbus }, destroy: () => undefined }
+        }
+      }
+    }
+    return requireStub.call(this, id, ...rest)
+  }
+  try {
+    fn()
+  } finally {
+    patchable.prototype.require = requireStub
+  }
+}
+
+describe('BLE D-Bus transport errors', () => {
+  it('does not let a system-bus error escape as an uncaught exception', () => {
+    withStubbedNodeBle(() => {
+      // Imported inside the stub so the wrapper picks up the fake module.
+      const { createBluetoothSafe } =
+        // eslint-disable-next-line @typescript-eslint/no-require-imports
+        require('../src/api/ble/safeBluetooth') as typeof import('../src/api/ble/safeBluetooth')
+
+      const session = createBluetoothSafe()
+      const bus = (session.bluetooth as { dbus: FakeBus }).dbus
+
+      expect(bus.listenerCount('error')).to.equal(
+        1,
+        'expected an error listener to be attached synchronously'
+      )
+
+      // The real failure: ENOENT on the system bus socket. Without a
+      // listener attached, this emit throws and takes the process down.
+      const emit = () =>
+        bus.emit(
+          'error',
+          Object.assign(
+            new Error('connect ENOENT /var/run/dbus/system_bus_socket'),
+            {
+              code: 'ENOENT'
+            }
+          )
+        )
+
+      expect(emit).to.not.throw()
+    })
+  })
+
+  it('still returns a usable session when the bus is healthy', () => {
+    withStubbedNodeBle(() => {
+      const { createBluetoothSafe } =
+        // eslint-disable-next-line @typescript-eslint/no-require-imports
+        require('../src/api/ble/safeBluetooth') as typeof import('../src/api/ble/safeBluetooth')
+
+      const session = createBluetoothSafe()
+      expect(session.bluetooth).to.be.an('object')
+      expect(session.destroy).to.be.a('function')
+    })
+  })
+})