From cbf81a05a35b8ddd378744c17b89e2579d6e42ef Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Mon, 29 Jun 2026 23:38:11 -0500 Subject: [PATCH] =?UTF-8?q?fix(#146):=20crash-hardening=20=E2=80=94=20one?= =?UTF-8?q?=20device's=20handler=20throw=20can't=20take=20down=20the=20fle?= =?UTF-8?q?et?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found in the alpha load test: client-chosen pairing codes collide by birthday paradox, the provisioning INSERT hit UNIQUE(devices.pairing_code), the SqliteError threw out of the (synchronous) socket handler -> uncaughtException -> logFatalAndExit -> the WHOLE server exited and every device dropped. The colliding flood crash-LOOPED the container (2 restarts). Two layers, same "one device can't take down the fleet" theme as #142/#143/#144: 1. Narrow (deviceSocket.js): wrap the device:register provisioning INSERT in try/catch — a UNIQUE pairing_code collision (or ANY db error) rejects THAT registration (device:auth-error -> client retries) instead of throwing. currentDeviceId/authenticated now set only AFTER the row exists (no half-auth socket on failure). 2. Broader (lib/safe-socket.js): protectSocket() overrides socket.on per connection so any handler throw is caught, logged (event + id + stack), the socket told, and DISCONNECTED — per-CONNECTION fail-fast, not whole-PROCESS. We don't keep serving a connection from possibly-half-mutated state (honors the existing fail-fast intent), we just contain it to "one device reconnects" (a non-event after beta5). Wired into both the /device and /dashboard connection handlers; auto-covers future handlers. Audited first: no handler throws as control flow, so blanket-wrapping is safe. Tests (mutation-verified, fail without their fix): - register-insert-crash.test.js: a pairing_code collision AND a general bind error each reject-one-device with no uncaughtException; server keeps serving. - socket-handler-isolation.test.js: a throwing handler disconnects only that socket; the server + other sockets stay alive. Full suite 243/243. Co-Authored-By: Claude Opus 4.8 (1M context) --- server/lib/safe-socket.js | 48 ++++++++++ server/test/register-insert-crash.test.js | 94 ++++++++++++++++++++ server/test/socket-handler-isolation.test.js | 70 +++++++++++++++ server/ws/dashboardSocket.js | 4 + server/ws/deviceSocket.js | 47 +++++++--- 5 files changed, 250 insertions(+), 13 deletions(-) create mode 100644 server/lib/safe-socket.js create mode 100644 server/test/register-insert-crash.test.js create mode 100644 server/test/socket-handler-isolation.test.js diff --git a/server/lib/safe-socket.js b/server/lib/safe-socket.js new file mode 100644 index 0000000..332630d --- /dev/null +++ b/server/lib/safe-socket.js @@ -0,0 +1,48 @@ +'use strict'; +// #146 scale-hardening — narrow fail-fast from whole-PROCESS to single-CONNECTION. +// +// The process INTENTIONALLY fail-fasts on an uncaught throw (server.js logFatalAndExit: +// "after an uncaught throw the process state is undefined, so we never keep serving"). +// For a per-DEVICE socket handler that blast radius is wrong: one device's bad input — +// a DB error inside a handler — threw out of the handler -> uncaughtException -> +// logFatalAndExit -> the WHOLE server exited and EVERY device dropped (found in the +// alpha load test: a colliding pairing code crash-LOOPED the fleet). +// +// protectSocket() overrides socket.on for ONE connection so every handler is wrapped. +// On a throw it does NOT keep serving that connection from possibly-half-mutated state +// (that would defeat the fail-fast intent); it logs (event + id + stack), tells the +// socket, and DISCONNECTS just that socket — which reconnects clean, a non-event after +// the beta5 reconnect fixes. So fail-fast becomes per-CONNECTION instead of +// whole-PROCESS. Per-site try/catch (e.g. the device:register INSERT) stays the primary +// guard; this is the backstop — and because it wraps socket.on itself, any FUTURE +// handler is covered automatically (no per-site swap to forget). +// +// Only socket.on is used in the ws layer (verified — no once/off/prependListener), so +// wrapping socket.on covers the whole handler surface for this connection. + +function protectSocket(socket, ctxFn) { + const rawOn = socket.on.bind(socket); + socket.on = (event, handler) => rawOn(event, (...args) => { + try { + const r = handler(...args); + // No handler is async today; if one becomes a promise, contain a rejection the + // same way instead of letting it become an unhandledRejection -> exit. + if (r && typeof r.then === 'function') r.catch((e) => bail(socket, event, e, ctxFn)); + } catch (e) { + bail(socket, event, e, ctxFn); + } + }); + return socket; +} + +function bail(socket, event, err, ctxFn) { + let who = socket.id; + try { const c = ctxFn && ctxFn(); if (c) who = `${c} (${socket.id})`; } catch (_) { /* ctx must never re-throw */ } + console.error(`[socket:${event}] handler threw for ${who} — disconnecting this socket (server stays up):\n${(err && err.stack) || err}`); + try { socket.emit('server:error', { event, error: 'internal error, please reconnect' }); } catch (_) { /* */ } + // nextTick disconnect so the error notice flushes before the transport closes + // (same pattern as the reconnect-throttle throttled-disconnect). + process.nextTick(() => { try { socket.disconnect(true); } catch (_) { /* */ } }); +} + +module.exports = { protectSocket }; diff --git a/server/test/register-insert-crash.test.js b/server/test/register-insert-crash.test.js new file mode 100644 index 0000000..48e126a --- /dev/null +++ b/server/test/register-insert-crash.test.js @@ -0,0 +1,94 @@ +'use strict'; + +// #146 scale-hardening — a DB error in the device:register provisioning INSERT must +// reject THAT device's registration, NOT throw out of the handler -> uncaughtException +// -> logFatalAndExit -> whole-server exit. Found in the alpha load test: client-chosen +// pairing codes collide by birthday paradox, the UNIQUE INSERT threw, and the server +// crash-LOOPED, dropping the entire fleet. +// +// Teeth: a process 'uncaughtException' listener captures any escaped throw. With the +// fix the array stays empty (the INSERT error is caught in-handler) and the colliding +// device gets device:auth-error while the server keeps serving. Neutralize the +// try/catch around the INSERT in deviceSocket.js and this test goes RED — the second +// (colliding) registration throws an uncaughtException. + +const { test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const os = require('node:os'); +const crypto = require('node:crypto'); + +process.env.DATA_DIR = path.join(os.tmpdir(), 'st-regcrash-' + crypto.randomBytes(4).toString('hex')); +process.env.SELF_HOSTED = 'true'; +process.env.NODE_ENV = 'test'; + +const http = require('node:http'); +const { Server } = require('socket.io'); +const ioClient = require('socket.io-client'); +const setupDeviceSocket = require('../ws/deviceSocket'); + +let httpServer, io, base; +const uncaught = []; +const onUncaught = (e) => uncaught.push(e); + +before(async () => { + process.on('uncaughtException', onUncaught); // capture escaped throws instead of dying + httpServer = http.createServer(); + io = new Server(httpServer); + setupDeviceSocket(io); + await new Promise((r) => httpServer.listen(0, r)); + base = `http://127.0.0.1:${httpServer.address().port}`; +}); +after(() => { + process.off('uncaughtException', onUncaught); + try { io.close(); } catch { /* */ } + try { httpServer.close(); } catch { /* */ } +}); + +const connect = () => ioClient(`${base}/device`, { transports: ['websocket'], reconnection: false, forceNew: true }); + +// Register with a payload; resolves the outcome (registered | authError | timeout). +function register(payload) { + return new Promise((resolve) => { + const s = connect(); + let done = false; + const fin = (r) => { if (done) return; done = true; try { s.close(); } catch { /* */ } resolve(r); }; + s.on('connect', () => s.emit('device:register', payload)); + s.on('device:registered', (d) => fin({ registered: true, id: d.device_id })); + s.on('device:auth-error', (e) => fin({ authError: true, error: e && e.error })); + setTimeout(() => fin({ timeout: true }), 3000); + }); +} + +const settle = () => new Promise((r) => setTimeout(r, 150)); + +test('pairing-code collision rejects the 2nd device, does NOT crash the server', async () => { + const code = String(crypto.randomInt(100000, 1000000)); + const a = await register({ pairing_code: code }); + assert.ok(a.registered, 'first device with the code registers'); + + // Same code again -> UNIQUE constraint on devices.pairing_code. + const b = await register({ pairing_code: code }); + await settle(); + assert.ok(b.authError, 'the colliding 2nd registration is rejected (device:auth-error)'); + assert.equal(uncaught.length, 0, 'a collision must NOT raise an uncaughtException'); + + // Server is still alive: a fresh unique registration still works. + const c = await register({ pairing_code: String(crypto.randomInt(100000, 1000000)) }); + assert.ok(c.registered, 'server keeps serving after the collision'); + assert.equal(uncaught.length, 0, 'still no uncaughtException'); +}); + +test('a general DB error in the register INSERT also rejects-one, not crash', async () => { + // device_info.screen_width as an object makes better-sqlite3's bind throw a + // DIFFERENT error than UNIQUE — proves the catch is error-type-agnostic. + const r = await register({ pairing_code: String(crypto.randomInt(100000, 1000000)), device_info: { screen_width: {} } }); + await settle(); + assert.ok(r.authError || r.timeout, 'the bad-bind registration is rejected, not completed'); + assert.equal(uncaught.length, 0, 'a general DB/bind error must NOT raise an uncaughtException'); + + // Server still serving. + const ok = await register({ pairing_code: String(crypto.randomInt(100000, 1000000)) }); + assert.ok(ok.registered, 'server keeps serving after a general register error'); + assert.equal(uncaught.length, 0, 'still no uncaughtException'); +}); diff --git a/server/test/socket-handler-isolation.test.js b/server/test/socket-handler-isolation.test.js new file mode 100644 index 0000000..8ce0988 --- /dev/null +++ b/server/test/socket-handler-isolation.test.js @@ -0,0 +1,70 @@ +'use strict'; + +// #146 scale-hardening — protectSocket() narrows fail-fast from whole-PROCESS to +// single-CONNECTION. A throwing socket handler must disconnect ONLY that socket and +// leave the server + every other socket fully alive — instead of escalating to +// uncaughtException -> process exit -> fleet outage (the alpha load-test crash). +// +// Teeth: an 'uncaughtException' capture asserts no throw escaped; client B proves the +// server kept serving. Swap protectSocket() for a raw socket.on (mutation) and this +// goes RED — the throw becomes an uncaughtException and socket A is never disconnected. + +const { test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const http = require('node:http'); +const { Server } = require('socket.io'); +const ioClient = require('socket.io-client'); +const { protectSocket } = require('../lib/safe-socket'); + +let httpServer, io, base; +const uncaught = []; +const onUncaught = (e) => uncaught.push(e); +const sleep = (ms) => new Promise((r) => setTimeout(r, ms)); + +before(async () => { + process.on('uncaughtException', onUncaught); + httpServer = http.createServer(); + io = new Server(httpServer); + io.of('/t').on('connection', (socket) => { + protectSocket(socket, () => socket.id); // <-- the fix under test + socket.on('boom', () => { throw new Error('handler blew up'); }); + socket.on('ping', (data, ack) => { if (ack) ack('pong'); }); + }); + await new Promise((r) => httpServer.listen(0, r)); + base = `http://127.0.0.1:${httpServer.address().port}`; +}); +after(() => { + process.off('uncaughtException', onUncaught); + try { io.close(); } catch { /* */ } + try { httpServer.close(); } catch { /* */ } +}); + +const connect = () => ioClient(`${base}/t`, { transports: ['websocket'], reconnection: false, forceNew: true }); +const onceConnected = (s) => new Promise((r) => s.on('connect', r)); + +test('a throwing handler disconnects only that socket; server + other sockets survive', async () => { + const A = connect(), B = connect(); + await Promise.all([onceConnected(A), onceConnected(B)]); + + let aGotError = false; + A.on('server:error', (m) => { if (m && m.event === 'boom') aGotError = true; }); + const aDisconnected = new Promise((r) => A.on('disconnect', () => r(true))); + + // Sanity: B works before the boom. + const pre = await new Promise((r) => B.emit('ping', {}, r)); + assert.equal(pre, 'pong', 'B responsive before the throw'); + + // A triggers a handler throw. + A.emit('boom', {}); + const dropped = await Promise.race([aDisconnected, sleep(2000).then(() => false)]); + + assert.equal(dropped, true, 'the throwing socket (A) is disconnected'); + assert.equal(uncaught.length, 0, 'no uncaughtException escaped to the process'); + + // The server is still up and a DIFFERENT socket is unaffected. + const post = await new Promise((r) => B.emit('ping', {}, r)); + assert.equal(post, 'pong', 'B still served after A threw — server survived'); + assert.ok(aGotError, 'A was told via server:error before being dropped'); + + try { B.close(); } catch { /* */ } +}); diff --git a/server/ws/dashboardSocket.js b/server/ws/dashboardSocket.js index bef4d05..43f003a 100644 --- a/server/ws/dashboardSocket.js +++ b/server/ws/dashboardSocket.js @@ -3,6 +3,7 @@ const { verifyToken } = require('../middleware/auth'); const { db } = require('../db/database'); const { accessContext, accessibleWorkspaceIds } = require('../lib/tenancy'); const { workspaceRoom } = require('../lib/socket-rooms'); +const { protectSocket } = require('../lib/safe-socket'); // Phase 2.3: workspace-scoped socket rooms + per-command permission gates. // Replaces the previous flat dashboardNs.emit broadcast (which leaked every @@ -48,6 +49,9 @@ module.exports = function setupDashboardSocket(io) { }); dashboardNs.on('connection', (socket) => { + // #146: same per-connection fail-fast as the device namespace — a throwing + // dashboard handler disconnects only that client, never crashes the server. + protectSocket(socket, () => socket.userId); // Note on workspace-switch lifecycle: the switcher (Phase 3 MVP) calls // window.location.reload() after switching, which forces a new socket // connection with fresh JWT claims. So workspace memberships are diff --git a/server/ws/deviceSocket.js b/server/ws/deviceSocket.js index 230ea11..3b49656 100644 --- a/server/ws/deviceSocket.js +++ b/server/ws/deviceSocket.js @@ -9,6 +9,7 @@ const commandQueue = require('../lib/command-queue'); const reconnectThrottle = require('../lib/reconnect-throttle'); const contentAckLimiter = require('../lib/content-ack-limiter'); const statusLogWriter = require('../lib/status-log-writer'); +const { protectSocket } = require('../lib/safe-socket'); const loopLag = require('../services/loop-lag'); // Debounce window for marking a device offline on socket disconnect. Brief @@ -278,6 +279,11 @@ module.exports = function setupDeviceSocket(io) { let currentDeviceId = null; let authenticated = false; // Track whether this socket has been authenticated + // #146: wrap every handler on THIS socket so a throw disconnects only this device + // (logged with its id) instead of crashing the whole server. Backstop to the + // per-site try/catch in the handlers below. + protectSocket(socket, () => currentDeviceId); + // Device registers with a pairing code (first time) or device_id + device_token (reconnect) socket.on('device:register', (data) => { const { pairing_code, device_id, device_token, device_info, fingerprint } = data; @@ -533,22 +539,37 @@ module.exports = function setupDeviceSocket(io) { // New device registering with pairing code — generate a device_token const id = uuidv4(); const newToken = generateDeviceToken(); + + // #146 scale-hardening: a DB error on this INSERT must reject THIS device's + // registration, never throw out of the handler. The likely error is a UNIQUE + // pairing_code collision when many devices provision at once (client-supplied + // 6-digit codes collide by birthday paradox), but ANY error counts. An + // unhandled throw in a socket handler escalates to uncaughtException -> + // logFatalAndExit -> the WHOLE server exits and every device drops — one + // colliding code crash-looped the fleet in the load test. Catch it, log, and + // tell just this device to retry. currentDeviceId/authenticated are set only + // AFTER the row exists, so a failed insert leaves no half-authenticated socket. + try { + db.prepare(` + INSERT INTO devices (id, pairing_code, device_token, status, ip_address, android_version, app_version, screen_width, screen_height, render_width, render_height, last_heartbeat) + VALUES (?, ?, ?, 'provisioning', ?, ?, ?, ?, ?, ?, ?, strftime('%s','now')) + `).run( + id, pairing_code, newToken, getClientIp(socket), + device_info?.android_version || null, + device_info?.app_version || null, + device_info?.screen_width || null, + device_info?.screen_height || null, + device_info?.render_width || null, + device_info?.render_height || null + ); + } catch (e) { + console.warn(`Provisioning rejected for pairing_code ${pairing_code} from ${getClientIp(socket)}: ${e.message}`); + socket.emit('device:auth-error', { error: 'Registration failed, please retry.' }); + return; + } currentDeviceId = id; authenticated = true; - db.prepare(` - INSERT INTO devices (id, pairing_code, device_token, status, ip_address, android_version, app_version, screen_width, screen_height, render_width, render_height, last_heartbeat) - VALUES (?, ?, ?, 'provisioning', ?, ?, ?, ?, ?, ?, ?, strftime('%s','now')) - `).run( - id, pairing_code, newToken, getClientIp(socket), - device_info?.android_version || null, - device_info?.app_version || null, - device_info?.screen_width || null, - device_info?.screen_height || null, - device_info?.render_width || null, - device_info?.render_height || null - ); - heartbeat.registerConnection(id, socket.id); socket.join(id); socket.emit('device:registered', { device_id: id, device_token: newToken, status: 'provisioning' });