Skip to content

Harden WebSocket Lifecycle, Reconnection Backoff, and Message Deserialization in Real-Time Data Client - #49

Open
mozluk wants to merge 2 commits into
Polymarket:mainfrom
mozluk:mozluk-patch-1
Open

Harden WebSocket Lifecycle, Reconnection Backoff, and Message Deserialization in Real-Time Data Client#49
mozluk wants to merge 2 commits into
Polymarket:mainfrom
mozluk:mozluk-patch-1

Conversation

@mozluk

@mozluk mozluk commented Sep 9, 2026

Copy link
Copy Markdown

Description

This pull request addresses reliability, resource lifecycle, and payload parsing findings identified in real-time-data-client during the workspace audit[cite: 8]. Previously, the client suffered from unhandled disconnections, unconstrained reconnection loops without backoff or jitter, platform-incompatible keepalive handlers, and unvalidated message payload deserialization[cite: 8]. This PR introduces full exponential backoff with jitter, decouples socket event listeners cleanly upon teardown, implements a cross-platform keepalive loop, and validates message shapes safely[cite: 8].

Key Changes & Remediations

1. Reconnection Resilience & Backoff Logic (src/client.ts)

  • Exponential Backoff with Full Jitter: Replaced immediate zero-delay reconnect storms with an exponential backoff schedule bounded by DEFAULT_MAX_RECONNECT_DELAY (30s)[cite: 8]. Full jitter (Math.random() * exponentialDelay) prevents thundering-herd reconnect storms against the live data cluster.
  • Single Reconnect Timer: Unified onError and onClose dispatch into a single idempotent scheduleReconnect() call, preventing concurrent duplicate socket spawns upon network drops.

2. Resource Lifecycle & Teardown Cleanliness (src/client.ts)

  • Clean Socket Teardown: Added teardownSocket() to nullify event listeners (onopen, onmessage, onclose, onerror) prior to closing the socket, preventing orphaned sockets from firing handlers or queueing redundant reconnects.
  • Cross-Platform Keepalive: Replaced non-standard ws.pong handler bindings with a self-rescheduling timer (startKeepalive) that sends periodic "ping" text frames uniformly across Node.js and browser environments.

3. Configuration & Payload Validation (src/client.ts)

  • Fail-Safe Construction: Replaced unsafe non-null assertions with default arguments (args = {}) and nullish coalescing (??), ensuring new RealTimeDataClient() runs without runtime TypeError while honoring autoReconnect: false.
  • Bounded Ping Intervals: Enforced finite, strictly positive millisecond checks on pingInterval.
  • Safe Message Parsing: Eliminated fragile substring searching for "payload". Incoming frames are now parsed with JSON.parse inside a try/catch and validated against the isMessage guard to discard malformed payloads gracefully without terminating the connection.

How to Review

  1. Backoff Schedule: Review scheduleReconnect() to verify the exponential backoff calculation and jitter bounds.
  2. Socket Teardown: Check teardownSocket() and connect() to confirm old sockets are detached and closed before a new connection is instantiated.
  3. Keepalive Flow: Inspect startKeepalive() to confirm periodic ping execution without platform-specific pong assignments.
  4. Message Guards: Inspect onMessage() and isMessage() to ensure non-conforming JSON payloads do not throw unhandled exceptions.

Note

Medium Risk
Touches core live connection behavior (reconnect timing, message filtering, and subscribe/send semantics), so consumers may see different delivery or retry patterns even though the changes fix known bugs.

Overview
Hardens RealTimeDataClient WebSocket reliability, lifecycle, and inbound message handling in src/client.ts.

Reconnection no longer fires immediate duplicate connects from onerror/onclose: a single scheduleReconnect() applies exponential backoff with full jitter (configurable reconnectBaseDelay / maxReconnectDelay), resets on successful open, and respects disconnect() via closedByCaller. connect() tears down prior sockets (handler detach + close) before opening a new one.

Keepalive is a cross-platform timer loop sending "ping" text frames instead of the broken ws.pong assignment. Constructor defaults fix new RealTimeDataClient() and autoReconnect: false; pingInterval is validated as a positive finite value.

Inbound frames are parsed safely (try/catch + isMessage), ping/pong strings are ignored, and sends go through rawSend (no Node-only send callbacks). subscribe / unsubscribe return booleans and no longer force-close the socket on send failure.

Reviewed by Cursor Bugbot for commit f16d572. Bugbot is set up for automated code reviews on this repo. Configure here.

…lization in Real-Time Data Client

### Description
This pull request addresses reliability, resource lifecycle, and payload parsing findings identified in `real-time-data-client` during the workspace audit[cite: 8]. Previously, the client suffered from unhandled disconnections, unconstrained reconnection loops without backoff or jitter, platform-incompatible keepalive handlers, and unvalidated message payload deserialization[cite: 8]. This PR introduces full exponential backoff with jitter, decouples socket event listeners cleanly upon teardown, implements a cross-platform keepalive loop, and validates message shapes safely[cite: 8].

### Key Changes & Remediations

#### 1. Reconnection Resilience & Backoff Logic (`src/client.ts`)
* **Exponential Backoff with Full Jitter:** Replaced immediate zero-delay reconnect storms with an exponential backoff schedule bounded by `DEFAULT_MAX_RECONNECT_DELAY` (30s)[cite: 8]. Full jitter (`Math.random() * exponentialDelay`) prevents thundering-herd reconnect storms against the live data cluster.
* **Single Reconnect Timer:** Unified `onError` and `onClose` dispatch into a single idempotent `scheduleReconnect()` call, preventing concurrent duplicate socket spawns upon network drops.

#### 2. Resource Lifecycle & Teardown Cleanliness (`src/client.ts`)
* **Clean Socket Teardown:** Added `teardownSocket()` to nullify event listeners (`onopen`, `onmessage`, `onclose`, `onerror`) prior to closing the socket, preventing orphaned sockets from firing handlers or queueing redundant reconnects.
* **Cross-Platform Keepalive:** Replaced non-standard `ws.pong` handler bindings with a self-rescheduling timer (`startKeepalive`) that sends periodic `"ping"` text frames uniformly across Node.js and browser environments.

#### 3. Configuration & Payload Validation (`src/client.ts`)
* **Fail-Safe Construction:** Replaced unsafe non-null assertions with default arguments (`args = {}`) and nullish coalescing (`??`), ensuring `new RealTimeDataClient()` runs without runtime `TypeError` while honoring `autoReconnect: false`.
* **Bounded Ping Intervals:** Enforced finite, strictly positive millisecond checks on `pingInterval`.
* **Safe Message Parsing:** Eliminated fragile substring searching for `"payload"`. Incoming frames are now parsed with `JSON.parse` inside a try/catch and validated against the `isMessage` guard to discard malformed payloads gracefully without terminating the connection.

### How to Review
1. **Backoff Schedule:** Review `scheduleReconnect()` to verify the exponential backoff calculation and jitter bounds.
2. **Socket Teardown:** Check `teardownSocket()` and `connect()` to confirm old sockets are detached and closed before a new connection is instantiated.
3. **Keepalive Flow:** Inspect `startKeepalive()` to confirm periodic ping execution without platform-specific `pong` assignments.
4. **Message Guards:** Inspect `onMessage()` and `isMessage()` to ensure non-conforming JSON payloads do not throw unhandled exceptions.
@mozluk
mozluk requested a review from a team as a code owner September 9, 2026 15:20

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit c82563c. Configure here.

Comment thread src/client.ts
@mozluk

mozluk commented Sep 9, 2026

Copy link
Copy Markdown
Author

Good catch. The manual mutation this.autoReconnect = false in disconnect() was redundant with closedByCaller = true and permanently broke auto-reconnection if connect() was later called again.

We have removed the mutation, made autoReconnect a readonly configuration property, and preserved closedByCaller to govern manual disconnect and reconnect lifecycles cleanly.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant