Skip to content
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
86 changes: 79 additions & 7 deletions .github/workflows/plugin-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,73 @@ 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 ASYNC_CRASH_DRAIN_MS = 1500;
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)));
});

// Every exit from the lifecycle check goes through here, including
// the early returns taken when start() or stop() throws. A plugin
// can schedule a failing timer and *then* throw synchronously from
// start(): the server catches that throw and keeps running (see
// doPluginStart in src/interfaces/plugins.ts), so the timer still
// fires and still kills the server. Exiting on the throw alone
// would report success for exactly that plugin.
const finish = async (code) => {
// Let queued failures surface before deciding the verdict. A
// microtask turn would cover an 'error' already on the queue, but
// the failures worth catching are slower: 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.
await new Promise((resolve) => setTimeout(resolve, ASYNC_CRASH_DRAIN_MS));

if (asyncCrashes.length > 0) {
console.log('');
// start() may run 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);
}
process.exit(code);
};
// See validate-entry.js for the rationale — object/conditional
// exports must be resolved, not flattened to 'index.js'.
function resolveEntry(p, fallback) {
Expand Down Expand Up @@ -713,6 +780,8 @@ jobs:
plugin = ctor(mockApp);
} catch (e) {
console.log('Plugin constructor requires server context — skipping lifecycle check');
// Pre-IIFE skip path: start() never ran, so there is nothing for
// finish() to drain (and top-level await is not available here).
process.exit(0);
}

Expand Down Expand Up @@ -754,7 +823,7 @@ jobs:
} else {
console.log('::warning::Plugins should handle empty/default configuration gracefully.');
}
process.exit(0);
await finish(0);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

try {
Expand All @@ -769,7 +838,10 @@ jobs:
} else {
console.log('::error::plugin.stop() threw: ' + msg);
console.log('::error::This causes the server to leak resources when the plugin is disabled or restarted.');
process.exit(1);
// finish() rather than a bare exit: the job already fails here,
// but stop() may also have scheduled a delayed failure, and
// reporting it tells the author about both problems at once.
return await finish(1);
}
}

Expand All @@ -782,11 +854,11 @@ jobs:
if (isMockGap(msg)) {
console.log('::warning::plugin.start({}) threw on restart: ' + msg);
console.log('::warning::This looks like a missing CI mock method, not a plugin bug. Please report at https://github.com/SignalK/signalk-server/issues');
process.exit(0);
await finish(0);
}
console.log('::error::plugin.start({}) threw on second call: ' + msg);
console.log('::error::Plugins must support restart (stop then start). This fails when users toggle the plugin in the server UI.');
process.exit(1);
return await finish(1);
}

try { await Promise.resolve(plugin.stop()); } catch (e) { /* ignore */ }
Expand All @@ -802,11 +874,11 @@ jobs:
console.log('');
console.log('Malformed deltas are silently discarded by the server —');
console.log('the plugin appears to work but data never reaches consumers.');
process.exit(1);
await finish(1);
}

console.log('Plugin lifecycle (start/stop/restart) is clean');
process.exit(0);
await finish(0);
})().catch(e => {
console.log('::error::Unexpected error during lifecycle check: ' + (e && e.message || String(e)));
process.exit(1);
Expand Down Expand Up @@ -1895,7 +1967,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"
Expand Down
57 changes: 57 additions & 0 deletions docs/develop/plugins/ci.md
Original file line number Diff line number Diff line change
Expand Up @@ -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** — Watches for roughly 1.5 seconds after the lifecycle calls return and fails the job if the plugin fails asynchronously in that window — an unhandled `'error'` event (which ends the server process) or a floating promise that rejects late. A failure that takes longer to surface will not be caught, so a clean run is not proof there is none. See [Crashing the server after start()](#crashing-the-server-after-start) below.

**API usage** — Scans source files for:

- Deprecated APIs (`setProviderStatus` → `setPluginStatus`, `setProviderError` → `setPluginError`)
Expand Down Expand Up @@ -175,6 +177,61 @@ 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.

Attach an `'error'` listener to every emitter your plugin holds, at the point
you create it, and report the failure through `app.setPluginError()`. A
listener that only handles the success event (`'data'`, `'message'`,
`'connect'`) leaves the failure path unguarded.

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. Node terminates the process for
these by default, but the server installs an `unhandledRejection` handler: it
logs the rejection and, when the originating plugin can be identified from the
stack, records a plugin error. So a floating rejection usually degrades your
plugin rather than killing the server — the failure is silent from the user's
point of view, and the work you meant to do never happened.

Either await the call inside `start()`, so its own error handling applies, or —
when the work must continue in the background — attach a rejection handler when
you kick it off. What fails the check is a call whose rejection nothing
observes.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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 of the first case. The
server's own `uncaughtException` handler logs the error and flags the plugin,
but it cannot stop an unhandled `'error'` event: Node throws on those
synchronously, and the throw ends the process regardless of what the handler
logged. One faulty emitter therefore takes the whole server down, 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.
17 changes: 11 additions & 6 deletions src/api/ble/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand Down Expand Up @@ -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 []
}
Expand Down
6 changes: 3 additions & 3 deletions src/api/ble/localProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
Loading
Loading