mirror of
https://github.com/screentinker/screentinker.git
synced 2026-08-13 22:03:13 -06:00
fix(#148) Items 2-4: mark-offline closes the socket + tighten ping + TCP keepalive
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.
This commit is contained in:
parent
8809007d9e
commit
bcfe3eaf8b
|
|
@ -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,
|
||||
|
|
|
|||
14
server/lib/tcp-keepalive.js
Normal file
14
server/lib/tcp-keepalive.js
Normal file
|
|
@ -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 };
|
||||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
78
server/test/148-half-open.test.js
Normal file
78
server/test/148-half-open.test.js
Normal file
|
|
@ -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)');
|
||||
});
|
||||
23
server/test/tcp-keepalive.test.js
Normal file
23
server/test/tcp-keepalive.test.js
Normal file
|
|
@ -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'); } }));
|
||||
});
|
||||
Loading…
Reference in a new issue