Skip to content

feat(ws): add heartbeat reaping, buffer limits, shutdown close frames, and graceful teardown - #464

Open
sparkysolutions10-pixel wants to merge 2 commits into
Divinity-Labs:mainfrom
sparkysolutions10-pixel:feat/websocket-lifecycle-shutdown-347-336-335-330
Open

sparkysolutions10-pixel wants to merge 2 commits into
Divinity-Labs:mainfrom
sparkysolutions10-pixel:feat/websocket-lifecycle-shutdown-347-336-335-330

Conversation

@sparkysolutions10-pixel

Copy link
Copy Markdown

Summary of Changes

This PR delivers comprehensive WebSocket lifecycle management, dead connection reaping, socket buffer memory bounding, shutdown close frames, and an ordered graceful SIGTERM/SIGINT teardown coordinator.

  • WebSocket Shutdown Close Frames & Flush Window (Closes Websocket clients are dropped without a close frame on shutdown #347):

    • Enhanced FanOutManager.destroy() in src/ws/fanout.ts to send every connected WebSocket client a clean close frame with code 1001 ('Server shutting down') before the process exits.
    • Provided a configurable flush window allowing in-flight updates to clear.
    • Guaranteed full idempotency and mid-fanout safety without throwing unhandled exceptions.
  • Dead WebSocket Connection Reaping & Map Hygiene (Closes Dead websocket connections are never reaped #336):

    • Added periodic heartbeat watchdog with configurable ping interval (WS_HEARTBEAT_INTERVAL_MS, default 30s) and missed ping threshold (WS_MAX_MISSED_PINGS, default 2).
    • Clients responding with pong maintain active subscriptions. Silent/stuck connections are terminated/closed and reaped.
    • Guaranteed complete cleanup of both internal tracking maps (clients and pairClients) upon unregistration so no ghost subscription entries are orphaned.
    • Exported ws_connected_clients Prometheus gauge for live client visibility.
  • Socket Buffer Bounding & Memory Protection (Closes One slow websocket client can grow the server's memory without limit #335):

    • Inspected client.socket.bufferedAmount before transmission in sendToClient.
    • Coalesces intermediate updates past soft threshold (WS_BUFFER_THRESHOLD_BYTES, default 64KB) to prevent unbounded buffer growth, incrementing ws_messages_dropped_total.
    • Drops persistently stalled clients past hard ceiling (WS_BUFFER_MAX_BYTES, default 256KB) with close code 1008 ('Buffer overflow'), incrementing ws_slow_clients_total.
    • Ensured ws_messages_sent_total only increments upon actual socket write.
  • Graceful Shutdown Coordinator (Closes Nothing handles SIGTERM, so every deploy interrupts in-flight work #330):

    • Created GracefulShutdownCoordinator in src/shutdown.ts and wired it into src/index.ts for both SIGTERM and SIGINT.
    • Enforced strictly ordered teardown:
      1. Stop HTTP & WebSocket listener (app.close()).
      2. Stop background ingester loops (prevent crash-loop restarts).
      3. Send close frames to WebSocket clients (fanOutManager.destroy()).
      4. Close background BullMQ queues and workers.
      5. Drain and close Postgres pool (pgPool.end()) and Redis (redis.quit()).
    • Bounded grace period with hard deadline (SHUTDOWN_GRACE_PERIOD_MS, default 10s) with loud timeout logging.
    • Guaranteed idempotency: duplicate signals during an active shutdown do not re-enter the drain.

Testing & Verification

  • Authored test suite src/__tests__/websocketLifecycleShutdown.test.ts covering close frame delivery, idempotency, mid-fanout safety, ping/pong pong tracking, dead client termination, map hygiene, buffer coalescing, hard-ceiling drop, ordered teardown, and signal idempotency.
  • Verified all 18 test cases passing across websocketLifecycleShutdown.test.ts and fanout.test.ts.

@Miracle656 Miracle656 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested: one issue per PR, and only issues assigned to you

This is a process requirement, applied to all 23 of your open PRs in this repo. It is not a judgement about the code, which I have not been able to review — and that is the first half of the problem.

The bundling

Across your open PRs, 150 distinct issues are claimed. This one claims several. #483 claims 22, #481 claims 20, #477 claims 20, #474 claims 20.

A PR that closes twenty issues cannot be reviewed. Each of those issues has its own acceptance criteria — typically five — so twenty issues is a hundred separate things to verify in one diff, and if any one of them is wrong the whole PR is blocked while it is fixed. Every batch I published says the issues are deliberately independent so they can be worked in parallel, by different people, without colliding. Bundling them removes exactly that property.

The assignments

135 of the 150 issues claimed are assigned to other contributors — 67 different people.

This PR is an example: it names issues currently assigned to other contributors who are working on them now. Several have open PRs of their own. If this merged as it stands, those people would have done the work and received nothing for it, and I would have no way to tell the two efforts apart.

I am not assuming anything about why. Assignments change, Drips and GitHub can disagree, and a wide-ranging branch picks up issue numbers easily. But the effect on the people holding those assignments is the same regardless of intent, which is why I have to act on it.

What I need

  • One issue per PR. The issue number in the title, its acceptance criteria met, and nothing else in the diff.
  • Only issues assigned to you, or unassigned ones you have claimed through Drips. If an issue is assigned to someone else, it is theirs.
  • Re-open the work that meets both conditions as individual PRs. I will review them in the order they arrive and at the same pace as everyone else's.

Nothing is being closed. These stay open and are yours to re-cut, withdraw or split as you choose.

Two things worth knowing before you re-cut

Some of this has already landed. #488 (SSRF guard for bazaar_search) and #487 (the paid_call host allow-list) cover the same ground as your #449; #486 covers the MCP README. Check current main before rebuilding something that is already there.

main has moved a long way today — #306, #309, #252, #256, #355, #381, #390, #402, #476, #488 and more. Cut from current main, not from where these branches started.

If you believe an issue named here is genuinely yours, say which and I will check the assignment myself — I would rather be corrected than have someone lose work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants