Conversation
connectedClients tracks Newt/Olm WebSocket connections with no server-initiated liveness check and no TCP keepalive configured on the sockets. A connection that dies without a clean close (network drop, sleep, killed process, silent NAT/firewall drop) stays in connectedClients indefinitely - sendToClient/broadcastToAllExcept keep "succeeding" into a socket that goes nowhere, and hasActiveConnections() never notices. Adds the standard ws-library heartbeat pattern: ping every tracked connection every 30s, terminate any connection that didn't answer the previous ping. Bounds detection of a dead connection to 1-2 heartbeat intervals instead of never happening on its own. The sweep logic lives in a dependency-free heartbeat.ts so it's unit-testable against fake connections without a live socket or the database. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
heartbeat.test.ts only exercises sweepConnection/sweepAllConnections against plain fake objects. This adds a real ws.WebSocketServer + real WebSocket client integration test that drives the actual shipped sweepAllConnections() against genuine TCP sockets. Verified by hand first: the obvious way to "kill" a connection in a test, client._socket.destroy(), does NOT simulate the bug this PR fixes - on localhost it sends an immediate TCP RST that the server's pre-existing `close` handler already reacts to (code 1006). The actual bug is a connection that goes silent with no close/error/RST ever reaching the server (WiFi vanishing, laptop sleep, a NAT/firewall black-holing packets). That's simulated correctly here with client._socket.pause(), confirmed by hand to produce no close, error, or pong on the server - a true black hole that only the heartbeat sweep can detect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The heartbeat sweep currently lacks resilience against exceptions during ping()/terminate(), which can break the periodic sweep or crash the process if an error occurs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a heartbeat-based liveness sweep for WebSocket connections to ensure silently-dead sockets are detected and cleaned up, keeping connectedClients accurate and preventing unbounded growth from connections that never emit a close event.
Changes:
- Introduces a heartbeat sweep module (
sweepConnection/sweepAllConnections) and integrates it into the WS router on a 30s interval. - Adds
isAlivetracking +ponghandling to mark connections responsive between sweeps. - Adds unit + integration tests validating termination behavior on real
wssockets and fake connections.
File summaries
| File | Description |
|---|---|
| server/routers/ws/ws.ts | Starts a periodic sweep over tracked sockets; marks connections alive on pong; clears interval on shutdown. |
| server/routers/ws/types.ts | Extends AuthenticatedWebSocket with an optional isAlive flag. |
| server/routers/ws/heartbeat.ts | Implements the minimal, testable heartbeat sweep logic. |
| server/routers/ws/heartbeat.test.ts | Unit tests for sweep logic using fake connections. |
| server/routers/ws/heartbeat.integration.test.ts | Integration test proving the sweep works against real ws server/client sockets. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| export function sweepConnection(connection: HeartbeatConnection): void { | ||
| if (connection.isAlive === false) { | ||
| connection.terminate(); | ||
| return; | ||
| } | ||
| connection.isAlive = false; | ||
| connection.ping(); | ||
| } |
There was a problem hiding this comment.
Good catch — fixed in 09b58fa.
For the exception safety: I verified against ws's own source that ping() throws synchronously when readyState is CONNECTING. Since this runs from a bare setInterval and this app has a global uncaughtException handler that calls process.exit(1), an uncaught throw here would've crashed the whole server for every connected user, not just missed detecting one bad connection. sweepConnection() now catches that and falls back to terminating the connection as dead, with a nested fallback in case terminate() itself ever fails too.
| heartbeatTimer = setInterval(() => { | ||
| sweepAllConnections(Array.from(connectedClients.values()).flat()); | ||
| }, HEARTBEAT_INTERVAL_MS); |
There was a problem hiding this comment.
Fixed in 09b58fa — switched to exactly this: connectedClients.forEach((clients) => sweepAllConnections(clients)) instead of building the flattened array copy every tick. That's also why the autofix shows as outdated now — the code already matches your suggestion.
Addresses two Copilot review findings on fosrl#3697: - sweepConnection() called ping()/terminate() with no error handling. Verified against ws's own source: ping() throws synchronously when readyState is CONNECTING. This runs from a bare setInterval with no surrounding try/catch, and this app's global uncaughtException handler (server/logger.ts) calls process.exit(1) - so one bad connection could crash the entire server for every connected user, not just fail to detect itself. Now caught and treated as dead. - The heartbeat interval rebuilt a flattened array of every tracked connection on every tick (Array.from(...).flat()). sweepAllConnections already accepts any iterable, so sweeping each client array directly removes that allocation entirely. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Community Contribution License Agreement
By creating this pull request, I grant the project maintainers an unlimited,
perpetual license to use, modify, and redistribute these contributions under any terms they
choose, including both the AGPLv3 and the Fossorial Commercial license terms. I
represent that I have the right to grant this license for all contributed content.
AI Disclosure
Claude Code (Anthropic) was used throughout this PR: to trace the gap, write the implementation and tests, and build/run the benchmarks and simulations below. I directed the investigation and verified the results myself, including catching a real flaw in the first draft of the integration test (see "How this was verified" below) before it shipped.
Description
Summary
server/routers/ws/ws.tstracks every connected Newt/Olm client in an in-memoryconnectedClientsmap, keyed by client ID. The server only removes an entry when it receives a WebSocketcloseevent - and it has no way to generate that event itself. There's no server-initiated liveness check anywhere in the file, and the underlying sockets have no TCP keepalive configured either (checked:keepAliveis set for the Postgres pool, Redis, and an AI-gateway HTTP client, but never for these WebSocket connections).So when a connection dies without a clean close - WiFi drops, a laptop sleeps, a process is killed, a NAT/firewall silently stops forwarding packets - nothing ever tells the server. The stale entry sits in
connectedClientsforever:hasActiveConnections()keeps reportingtrue,sendToClient()/broadcastToAllExcept()keep "succeeding" into a socket that goes nowhere, and the connection count only ever grows.Why this helps
This adds the heartbeat pattern the
wslibrary's own documentation recommends for exactly this problem: ping every tracked connection on an interval, and terminate any connection that didn't answer the previous ping. Aponghandler marks a connection alive again the moment a response comes back, so nothing is disconnected for being slow - only for being silent across a full interval. This bounds detection of a truly dead connection to 1-2 heartbeat cycles instead of never happening on its own, keepingconnectedClients(and anything that reasons about it) accurate.What changed
server/routers/ws/heartbeat.ts(new) -sweepConnection()/sweepAllConnections(). Deliberately takes a minimal duck-typed{ isAlive, ping(), terminate() }shape rather than importingws'sWebSockettype or anything from@server/db, so the logic is unit-testable without a live socket or the database.server/routers/ws/ws.ts- a 30ssetIntervalsweeping every tracked connection viasweepAllConnections();ws.isAlive = trueand aponghandler added insetupConnection(); the interval is cleared in the existingcleanup()function on graceful shutdown.sweepConnection'sterminate()call fires the existingclosehandler, so cleanup reuses theremoveClient()path that already exists - no new cleanup code needed.server/routers/ws/types.ts- addedisAlive?: booleantoAuthenticatedWebSocket.server/routers/ws/heartbeat.test.ts(new) - 12 assertions against fake connections: fresh connections survive their first sweep, responsive connections survive indefinitely, unresponsive ones are terminated on exactly their 2nd missed cycle (not 1st, not 3rd+).server/routers/ws/heartbeat.integration.test.ts(new) - see below.Scope note:
server/private/routers/ws/ws.tsis explicitly proprietary (Fossorial Commercial License, not AGPL) and is intentionally not touched here.How this was verified (and a mistake I caught before shipping)
A unit test against fake objects proves the sweep logic is correct, but not that it behaves correctly against a real socket. So I added a second test,
heartbeat.integration.test.ts, that spins up an actualws.WebSocketServerand connects a realWebSocketclient over a real TCP socket on localhost, then drives the actualsweepAllConnections()this PR ships against it.My first attempt at "kill the connection" used
client._socket.destroy(), which felt like the obvious way to simulate a dropped connection. Testing it by hand first (see comments in the test file) showed it does not simulate the bug this PR fixes: on localhost,destroy()sends an immediate TCP RST, which the server's existingclosehandler already receives and reacts to (close code 1006) - that path was never broken. The actual bug only happens when nothing reaches the server at all: no RST, no FIN, no error. I reproduced that correctly withclient._socket.pause(), which stops the client from processing incoming frames without tearing down the TCP connection - confirmed by hand this produces zeroclose,error, orpongevents on the server, a genuine silent black hole. Only the heartbeat sweep can detect that; the integration test proves it does, within exactly 2 cycles, while also proving a live, responsive connection is never mistakenly dropped across repeated sweeps.Impact & metrics
Before vs. after (simulated: 200 connections, 30% go silent without a clean close):
connectedClients.sizeMatches the math exactly (60 dead / 200 = 30%). Detection is bounded to 2 heartbeat cycles - ~60-90s at the 30s interval used here - instead of unbounded.
Overhead of the sweep itself (runs once per 30s interval, not per-request; each row measured in its own fresh process with 3,000 warm-up iterations, to avoid an earlier tier's JIT warm-up making a later one look artificially fast):
Negligible at any plausible scale, and it's a periodic background sweep, not something that runs per-message or per-request.
Memory overhead. Measured directly (heap delta, forced GC, 5,000 simulated connections, 3 repeated runs) rather than estimated: the
isAliveboolean plus theponglistener closuresetupConnection()registers together cost ~137 bytes/connection, consistently. The listener closure - not the boolean - is the dominant cost. At 1,000 connections that's ~137 KB; at 5,000, ~0.65 MB. Trivial at any scale Pangolin realistically runs at.Graceful shutdown. The heartbeat's
setIntervalhandle is cleared in the existingcleanup()function, which is wired to bothSIGTERMandSIGINTviaserver/cleanup.ts. It's cleared before the existing socket-termination loop runs, specifically to close a race: without that ordering, the interval could fire mid-shutdown and call.ping()/.terminate()on a socket that's already being torn down. (Note:server/cleanup.tscallsprocess.exit(0)explicitly once cleanup finishes, so an uncleared timer wouldn't have blocked process exit either way here - the real reason to clear it first is that shutdown ordering, not event-loop lifetime.)How to test?
npx tsx server/routers/ws/heartbeat.test.ts- unit tests against fake connections.npx tsx server/routers/ws/heartbeat.integration.test.ts- integration test against a realwsserver/client pair; simulates a silently dropped connection and a healthy one, asserts both are handled correctly.npx tsc --noEmitandnpx eslint . --ext .js,.jsx,.ts,.tsx- both clean.Client disconnected...) within ~60s instead of the entry lingering.