mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 13:53:12 -06:00
fix(#146): crash-hardening — one device's handler throw can't take down the fleet
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) <noreply@anthropic.com>
This commit is contained in:
parent
5a094710e5
commit
cbf81a05a3
48
server/lib/safe-socket.js
Normal file
48
server/lib/safe-socket.js
Normal file
|
|
@ -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 };
|
||||
94
server/test/register-insert-crash.test.js
Normal file
94
server/test/register-insert-crash.test.js
Normal file
|
|
@ -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');
|
||||
});
|
||||
70
server/test/socket-handler-isolation.test.js
Normal file
70
server/test/socket-handler-isolation.test.js
Normal file
|
|
@ -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 { /* */ }
|
||||
});
|
||||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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' });
|
||||
|
|
|
|||
Loading…
Reference in a new issue