From bcfe3eaf8badfba493cdb38a5b801d78c52b36e1 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Thu, 2 Jul 2026 14:59:25 -0500 Subject: [PATCH] fix(#148) Items 2-4: mark-offline closes the socket + tighten ping + TCP keepalive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Item 2: when the heartbeat checker marks a device offline it now also disconnects any socket it still holds for it, so DB-offline can't diverge from socket-state into a silent half-open (defensive — the live-socket guard already defers genuinely-live sockets). Item 3: tighten half-open detection WITHOUT reintroducing the TV-WebKit decode-load risk the 30s pong-timeout was chosen for — lower only pingInterval 30s->15s (probe more often), KEEP pingTimeout at 30s. Detection = interval+timeout = 45s (was 60s), and the client inherits these via the handshake so BOTH ends detect a dead peer ~25% sooner. (Deliberately did NOT drop pingTimeout to ~20s: MAXHUB is a video-playing TV-class device and the code comment warns tighter timeouts cause spurious drops under decode load.) Item 4: SO_KEEPALIVE on every accepted connection (lib/tcp-keepalive.js) so a half-open TCP can't persist indefinitely at the OS layer, independent of the app ping. Tests: server closes a non-ponging peer within ~pingInterval+pingTimeout while a ponging peer survives; a device whose transport dies ends offline with its connection torn down; keepalive applied to each accepted connection (and never breaks setup on error). Suite 328/328. --- server/config.js | 11 ++++- server/lib/tcp-keepalive.js | 14 ++++++ server/server.js | 3 ++ server/services/heartbeat.js | 9 ++++ server/test/148-half-open.test.js | 78 +++++++++++++++++++++++++++++++ server/test/tcp-keepalive.test.js | 23 +++++++++ 6 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 server/lib/tcp-keepalive.js create mode 100644 server/test/148-half-open.test.js create mode 100644 server/test/tcp-keepalive.test.js diff --git a/server/config.js b/server/config.js index 96e20d1..769e05c 100644 --- a/server/config.js +++ b/server/config.js @@ -43,9 +43,16 @@ module.exports = { // Engine.IO transport-level ping/pong. Raised from Socket.IO defaults // (25000/20000) because TV WebKits (LG webOS, older Tizen) miss pongs // under decode load - tighter values cause spurious transport drops. - // Worst-case dead-socket detection: pingInterval + pingTimeout = 60s. - pingInterval: parseInt(process.env.PING_INTERVAL) || 30000, + // #148: faster half-open detection WITHOUT reintroducing that risk — we lower only the + // PING INTERVAL (probe more often), keeping the deliberately-generous 30s pong TIMEOUT so a + // decode-loaded TV WebKit still has the full window to answer. Detection = interval + + // timeout = 45s (was 60s); the client inherits these via the handshake so BOTH ends detect + // a dead peer ~25% sooner. Do NOT drop pingTimeout below ~30s (see the decode-load note). + pingInterval: parseInt(process.env.PING_INTERVAL) || 15000, pingTimeout: parseInt(process.env.PING_TIMEOUT) || 30000, + // #148 Item 4: TCP SO_KEEPALIVE idle delay — OS-level dead-peer probing independent of the + // app ping, so a half-open TCP can't persist indefinitely. + tcpKeepAliveMs: parseInt(process.env.TCP_KEEPALIVE_MS) || 20000, maxFileSize: 500 * 1024 * 1024, // 500MB thumbnailWidth: 320, screenshotQuality: 70, diff --git a/server/lib/tcp-keepalive.js b/server/lib/tcp-keepalive.js new file mode 100644 index 0000000..6ee28ed --- /dev/null +++ b/server/lib/tcp-keepalive.js @@ -0,0 +1,14 @@ +'use strict'; + +// #148 Item 4 — enable TCP SO_KEEPALIVE on every accepted connection so a half-open TCP +// (e.g. a connection severed silently by an edge firewall/NAT reap, leaving no FIN) can't +// persist indefinitely at the OS layer, independent of the Engine.IO application ping. The +// http/https server's 'connection' event fires with the raw net.Socket (before TLS), which +// is where setKeepAlive belongs. Best-effort — never let it break connection setup. +function applyTcpKeepAlive(server, idleMs) { + server.on('connection', (socket) => { + try { socket.setKeepAlive(true, idleMs); } catch (_) { /* best-effort */ } + }); +} + +module.exports = { applyTcpKeepAlive }; diff --git a/server/server.js b/server/server.js index 7eaf1c2..29c25cb 100644 --- a/server/server.js +++ b/server/server.js @@ -57,6 +57,9 @@ if (hasSsl) { server = http.createServer(app); } +// #148 Item 4: TCP SO_KEEPALIVE on every accepted connection (lib/tcp-keepalive.js). +require('./lib/tcp-keepalive').applyTcpKeepAlive(server, config.tcpKeepAliveMs); + // Socket.IO CORS is checked via the same corsOriginCheck function defined below // (after config is loaded). Hoisted into a closure so we can reference it before // the function is defined — at first connection time, corsOriginCheck exists. diff --git a/server/services/heartbeat.js b/server/services/heartbeat.js index c57d7f3..937e49d 100644 --- a/server/services/heartbeat.js +++ b/server/services/heartbeat.js @@ -47,6 +47,15 @@ function startHeartbeatChecker(io) { const lastBeat = conn ? conn.lastHeartbeat : (device.last_heartbeat ? device.last_heartbeat * 1000 : 0); if (now - lastBeat > config.heartbeatTimeout) { + // #148 Item 2: marking a device offline MUST also close any socket we still hold for + // it, so DB-offline can never diverge from socket-state into a silent half-open the + // client is never told about. The live-socket guard above already `continue`d for a + // genuinely-live socket, so this only reaps a stale/half-open one (Engine.IO's + // ping-timeout also reaps it, but this makes offline<=>closed explicit + immediate). + if (conn) { + const sock = deviceNs.sockets.get(conn.socketId); + if (sock) { try { sock.disconnect(true); } catch (_) { /* already gone */ } } + } db.prepare("UPDATE devices SET status = 'offline', updated_at = strftime('%s','now') WHERE id = ?") .run(device.id); deviceConnections.delete(device.id); diff --git a/server/test/148-half-open.test.js b/server/test/148-half-open.test.js new file mode 100644 index 0000000..b960744 --- /dev/null +++ b/server/test/148-half-open.test.js @@ -0,0 +1,78 @@ +'use strict'; + +// #148 Items 2 & 3 — booted server: +// - the server closes a peer that stops responding to pings within pingInterval+pingTimeout +// (Item 3, the tightened half-open detection), while a peer that keeps ponging survives; +// - a device whose transport dies ends up OFFLINE with its connection torn down — no +// offline-but-still-tracked divergence (Item 2 invariant). + +const { test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const { spawn } = require('node:child_process'); +const WebSocket = require('../node_modules/ws'); +const ioClient = require('socket.io-client'); +const path = require('node:path'); const os = require('node:os'); const fs = require('node:fs'); const crypto = require('node:crypto'); + +const PORT = 3957; +const BASE = `http://127.0.0.1:${PORT}`; +const DATA_DIR = path.join(os.tmpdir(), 'st-ho-' + crypto.randomBytes(4).toString('hex')); +let proc; +const sleep = (ms) => new Promise(r => setTimeout(r, ms)); + +before(async () => { + const logFd = fs.openSync(path.join(os.tmpdir(), 'st-ho.log'), 'w'); + proc = spawn('node', ['server.js'], { + cwd: path.join(__dirname, '..'), + env: { ...process.env, DATA_DIR, SELF_HOSTED: 'true', PORT: String(PORT), NODE_ENV: 'test', + PING_INTERVAL: '400', PING_TIMEOUT: '400', // half-open closed ~800ms + HEARTBEAT_INTERVAL: '400', HEARTBEAT_TIMEOUT: '800' }, // checker marks offline fast + stdio: ['ignore', logFd, logFd], + }); + let up = false; + for (let i = 0; i < 80; i++) { try { const r = await fetch(BASE + '/api/status'); if (r.ok) { up = true; break; } } catch { /* */ } await sleep(250); } + if (!up) throw new Error('server did not boot'); +}); +after(() => { try { proc.kill('SIGKILL'); } catch { /* */ } }); + +const connected = async () => (await (await fetch(BASE + '/api/status')).json()).devices_connected; + +test('Item 3: server closes a NON-ponging peer within pingInterval+pingTimeout', async () => { + const ws = new WebSocket(`ws://127.0.0.1:${PORT}/socket.io/?EIO=4&transport=websocket`); + let open = null, openAt = 0, closeDelay = null; + ws.on('message', (d) => { const s = d.toString(); if (s[0] === '0') { open = JSON.parse(s.slice(1)); openAt = Date.now(); ws.send('40/device,'); } /* never pong '2' */ }); + ws.on('close', () => { if (openAt) closeDelay = Date.now() - openAt; }); + await sleep(4000); + assert.ok(open && open.pingInterval === 400 && open.pingTimeout === 400, 'server advertises the tightened ping values to the client'); + assert.ok(closeDelay != null, 'server closed the non-ponging (half-open) peer'); + // Detection time measured from the engine OPEN (excludes boot/connection-setup latency) + // ~= pingInterval + pingTimeout = 800ms. Bounded, not indefinite. + assert.ok(closeDelay < 1500, `closed ~${closeDelay}ms after open (bounded ~pingInterval+pingTimeout)`); +}); + +test('Item 3: a peer that keeps ponging is NOT falsely dropped', async () => { + const ws = new WebSocket(`ws://127.0.0.1:${PORT}/socket.io/?EIO=4&transport=websocket`); + let closed = false; + ws.on('message', (d) => { const s = d.toString(); if (s[0] === '0') ws.send('40/device,'); else if (s[0] === '2') ws.send('3'); /* pong */ }); + ws.on('close', () => { closed = true; }); + await sleep(2000); // > 2× the detect window + assert.equal(closed, false, 'a healthy, ponging peer stays connected past the detection window'); + try { ws.close(); } catch { /* */ } +}); + +test('Item 2: a device whose transport dies ends OFFLINE with its connection torn down', async () => { + const base0 = await connected(); + const s = ioClient(`${BASE}/device`, { transports: ['websocket'], reconnection: false, forceNew: true }); + await new Promise((resolve) => { + s.on('connect', () => s.emit('device:register', { pairing_code: String(crypto.randomInt(100000, 1000000)) })); + s.on('device:registered', resolve); + setTimeout(resolve, 3000); + }); + await sleep(300); + assert.ok((await connected()) > base0, 'device is counted as connected after register'); + // Kill the underlying transport abruptly (simulate a silent/half-open drop). + try { s.io.engine.transport.ws.terminate(); } catch { try { s.io.engine.close(); } catch { /* */ } } + // Within ping + heartbeat windows the server must reap it: connection torn down. + let ok = false; + for (let i = 0; i < 20; i++) { if ((await connected()) <= base0) { ok = true; break; } await sleep(300); } + assert.ok(ok, 'connection is torn down after the transport dies (no lingering half-open)'); +}); diff --git a/server/test/tcp-keepalive.test.js b/server/test/tcp-keepalive.test.js new file mode 100644 index 0000000..fcf48ab --- /dev/null +++ b/server/test/tcp-keepalive.test.js @@ -0,0 +1,23 @@ +'use strict'; + +// #148 Item 4 — SO_KEEPALIVE is applied to every accepted connection. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { EventEmitter } = require('node:events'); +const { applyTcpKeepAlive } = require('../lib/tcp-keepalive'); + +test('applyTcpKeepAlive enables keepalive on each accepted connection', () => { + const server = new EventEmitter(); + applyTcpKeepAlive(server, 20000); + const calls = []; + server.emit('connection', { setKeepAlive: (enable, ms) => calls.push([enable, ms]) }); + server.emit('connection', { setKeepAlive: (enable, ms) => calls.push([enable, ms]) }); + assert.deepEqual(calls, [[true, 20000], [true, 20000]]); +}); + +test('a socket that throws on setKeepAlive never breaks connection setup', () => { + const server = new EventEmitter(); + applyTcpKeepAlive(server, 20000); + assert.doesNotThrow(() => server.emit('connection', { setKeepAlive: () => { throw new Error('boom'); } })); +});