fix: exponential back-off reconnect + onClose/onError callbacks + autoReconnect ?? fix by osr21 · Pull Request #46 · Polymarket/real-time-data-client

osr21 · 2026-07-23T13:46:42Z

Summary

Fixes three related bugs in `RealTimeDataClient` and adds two new features. All changes are in `src/client.ts`.

---

### Bug 1 — `autoReconnect: false` silently ignored (fixes #43)

```ts
// before (broken) — false is falsy, so this always evaluates to true
this.autoReconnect = args!.autoReconnect || true;

// after
this.autoReconnect = args?.autoReconnect ?? true;
```

---

### Bug 2 — No `onClose` / `onError` callbacks (fixes #43)

Added both to `RealTimeDataClientArgs` and wired them into the handlers with a try/catch guard so a throwing callback cannot crash the client.

```ts
const client = new RealTimeDataClient({
  onClose: (client, event) => console.log("closed:", event.code, event.reason),
  onError: (client, err)   => console.error("ws error:", err),
});
```

---

### Bug 3 — Heap OOM on persistent network errors (fixes #38)

**Root cause:** when the socket closes with code 1006 (abnormal close — e.g. dropped mobile network), the previous code called `this.connect()` synchronously inside both `onError` and `onClose`. On a sustained outage this creates a tight loop: each failed attempt fires both handlers, both call `connect()`, two new `WebSocket` objects are allocated, and their internal buffers accumulate in the heap faster than GC can reclaim them.

**Fix:** replaced the immediate `connect()` with `scheduleReconnect()`, which uses **exponential back-off with ±1 s jitter**:

```
delay = min(reconnectDelay × 2^attempt, maxReconnectDelay) + rand(0..1000) ms
```

Defaults: **1 s** initial, **30 s** cap — both configurable:

```ts
const client = new RealTimeDataClient({
  reconnectDelay:    500,    // first retry after 500 ms
  maxReconnectDelay: 60_000, // cap at 60 s
});
```

---

### Bonus fixes

| Fix | Detail |
|---|---|
| **Double-reconnect prevention** | Added `connectionId` counter stamped at `connect()` time. `onError` and `onClose` handlers compare their captured id against the current value — only the handler that matches schedules a reconnect. Prevents two concurrent reconnect attempts when both fire on the same dead socket. |
| **`disconnect()` improvements** | Clears the pending reconnect timer on `disconnect()` (so a scheduled retry doesn't fire after the caller intentionally disconnects). Also guards against calling `ws.close()` on an already-CLOSED or CLOSING socket. |
| **Optional chaining in constructor** | Changed `args!` → `args?` throughout the constructor so `new RealTimeDataClient()` with no arguments no longer throws a runtime error on the non-null assertion. |

---

### Relation to open PRs

- **#44 / #45** — also fix the `autoReconnect` bug and add `onClose`/`onError` callbacks. This PR duplicates those two fixes but adds the exponential back-off (#38) which neither PR includes. Happy to rebase on top of whichever PR merges first and drop the duplicate hunks if that's easier to review.

---

### Testing

Manually tested against `wss://ws-live-data.polymarket.com` with simulated network drops (disabled NIC mid-stream). Confirmed:
- Reconnect attempts respect the back-off schedule
- Heap memory stays stable under sustained 1006 errors (no OOM)
- `autoReconnect: false` correctly prevents reconnection
- `disconnect()` cancels a pending retry timer

Note

Medium Risk
Changes core WebSocket connection lifecycle and status semantics; behavior shifts for consumers relying on immediate reconnect or prior status ordering, though scope is limited to one client module.

Overview
Overhauls RealTimeDataClient reconnection and lifecycle handling in src/client.ts.

Reconnect behavior: Immediate connect() on error/close is replaced with scheduleReconnect() using exponential backoff (configurable reconnectDelay / maxReconnectDelay) plus jitter, addressing tight reconnect loops and heap growth on sustained outages.

API & bugs: Adds optional onClose and onError callbacks (with try/catch). Fixes autoReconnect: false by using ?? instead of || true. Constructor uses optional chaining so new RealTimeDataClient() without args is safe.

Concurrency / state: Introduces connectionId and lastConnectedId so stale socket events cannot double-reconnect, corrupt status (CONNECTING → DISCONNECTED without CONNECTED), or keep ping chains alive. onClose alone schedules reconnect and emits DISCONNECTED only when the socket had reached CONNECTED. connect() cancels pending timers, closes the previous socket, and resets backoff when overriding a scheduled retry; disconnect() clears timers and avoids closing already-closed sockets.

^{Reviewed by Cursor Bugbot for commit 33052dc. Bugbot is set up for automated code reviews on this repo. Configure here.}

…econnect ?? fix Closes three related bugs: 1. autoReconnect: false silently ignored — || treated false as falsy. Fixed with ?? (nullish coalescing). 2. No onClose / onError callbacks exposed to callers. Added both to RealTimeDataClientArgs and wired into handlers. 3. Tight reconnect loop causes heap OOM on persistent network errors (Polymarket#38). Replaced immediate this.connect() with scheduleReconnect() which uses exponential back-off with ±1 s jitter: delay = min(reconnectDelay × 2^attempt, maxReconnectDelay) + rand(0..1000) ms Defaults: 1 s initial, 30 s cap — both configurable via RealTimeDataClientArgs. Bonus: - connectionId counter prevents double-reconnect when onError + onClose both fire on the same closed socket. - disconnect() clears the pending reconnect timer and guards against calling close() on an already-CLOSED/CLOSING socket. - Constructor uses optional chaining (args?.) so new RealTimeDataClient() with no args no longer throws on the args! non-null assertion.

…nId check, clear timer in connect() Fixes 3 issues flagged in code review: 1. [High] Stale close updates connection status onClose and onError were always running notifyStatusChange(DISCONNECTED) and user callbacks regardless of whether the event came from the current socket or a superseded one. A late event from an old WebSocket could falsely mark the client DISCONNECTED while the new socket was healthy. Fix: early-return at the top of onClose and onError when id !== connectionId. 2. [Medium] Stale open resets reconnect state onOpen received the per-connect() id but never compared it to connectionId. A delayed 'open' from a superseded socket would reset reconnectAttempt, start a duplicate ping loop, notify CONNECTED, and invoke onConnect against the wrong lifecycle. Fix: early-return at the top of onOpen when id !== connectionId. 3. [Medium] Pending reconnect timer not cleared in connect() Calling connect() manually while a back-off timer was already scheduled left the timer alive; when it fired it opened a second concurrent socket. Fix: clear reconnectTimer at the start of connect() before creating the socket.

osr21 · 2026-07-23T13:54:49Z

Thanks for the review @cursor[bot] — all three issues are addressed in commit e87b58a.

**1. [High] Stale close updates connection status**
Both `onClose` and `onError` now early-return immediately when `id !== this.connectionId`. The status notification and user callbacks are fully inside the guard, so a delayed event from a superseded socket can never falsely mark the client `DISCONNECTED` while the current socket is alive.

```ts
private onClose = (id: number, message: CloseEvent) => {
  if (id !== this.connectionId) return; // ← guard added
  // ... status change and callbacks only run for the current socket
};
```

**2. [Medium] Stale open resets reconnect state**
`onOpen` now checks `id !== this.connectionId` and returns early. A slow-connecting previous socket can no longer reset `reconnectAttempt`, start a duplicate ping loop, or invoke `onConnect` against the wrong lifecycle.

```ts
private onOpen = (id: number) => {
  if (id !== this.connectionId) return; // ← guard added
  this.reconnectAttempt = 0;
  // ...
};
```

**3. [Medium] Pending reconnect timer not cleared in connect()**
`connect()` now cancels any live `reconnectTimer` before allocating a new socket. Manual calls to `connect()` while a back-off retry is scheduled no longer cause overlapping connections.

```ts
public connect() {
  if (this.reconnectTimer !== null) { // ← clear timer before opening socket
      clearTimeout(this.reconnectTimer);
      this.reconnectTimer = null;
  }
  const id = ++this.connectionId;
  // ...
}
```

… duplicate DISCONNECTED events 1. [High] Reconnect after disconnect race scheduleReconnect()'s timer callback called connect() unconditionally. clearTimeout() cannot cancel a callback already in the task queue, so calling disconnect() right as the timer fires still opened a new socket. Fix: re-check this.autoReconnect inside the setTimeout callback as a last-resort guard after the null-timer check. 2. [Low] Duplicate DISCONNECTED status events onError was emitting notifyStatusChange(DISCONNECTED) and then onClose also emitted it, causing onStatusChange to fire twice for one drop. WebSocket guarantees onClose always follows onError on the same socket, so DISCONNECTED is now emitted only from onClose. onError is reduced to logging and invoking the user onError callback.

osr21 · 2026-07-23T14:05:11Z

Both issues from the latest review are addressed in commit 7ff91d6.

**1. [High] Reconnect after disconnect race**

`clearTimeout()` cannot cancel a callback that has already entered the JS task queue. If `disconnect()` is called in the same tick that the timer fires, the old code would still open a new socket. Fixed by re-checking `this.autoReconnect` inside the callback itself as a last-resort guard:

```ts
this.reconnectTimer = setTimeout(() => {
  this.reconnectTimer = null;
  // Re-check here: clearTimeout() cannot stop a callback already in the task queue.
  if (this.autoReconnect) {
      this.connect();
  }
}, delayMs);
```

**2. [Low] Duplicate DISCONNECTED status events**

The previous commit added `notifyStatusChange(DISCONNECTED)` to `onError`, but `onClose` also emits it — and the WebSocket spec guarantees `onClose` always fires after `onError` on the same socket. So `onStatusChange` could fire twice for a single connection drop.

Fixed by removing the status notification from `onError` entirely. `onError` is now responsible only for logging and invoking the user's `onError` callback. `onClose` remains the single place that emits `DISCONNECTED` and schedules a reconnect.

```ts
// onError — no longer emits DISCONNECTED or schedules reconnect
private onError = (id: number, err: ErrorEvent) => {
  if (id !== this.connectionId) return;
  console.error("error", err);
  if (this.onUserError) {
      try { this.onUserError(this, err); } catch (e) { console.error("onError callback threw:", e); }
  }
  // onClose fires next and handles DISCONNECTED + reconnect
};
```

…ack, backoff counter survives disconnect 1. [High] Duplicate reconnect after onClose callback onClose called scheduleReconnect() unconditionally after invoking the user's onClose callback. But the user callback may itself call connect() to reconnect immediately (e.g. custom reconnect logic). If it did, connectionId was already incremented, yet scheduleReconnect() still ran and opened a second socket. Fix: re-check id === this.connectionId after the user callback returns. If the callback called connect(), connectionId has advanced and we skip scheduleReconnect(), preventing the duplicate socket. 2. [Medium] Backoff counter survives disconnect reconnectAttempt was only reset on a successful onOpen. After many failed auto-reconnects, calling disconnect() and later reconnecting manually inherited the stale exponent, so the first failure after re-connect waited up to maxReconnectDelay instead of the initial reconnectDelay. Fix: reset reconnectAttempt = 0 in disconnect() and in connect() when it cancels a pending timer (a manual connect overrides the back-off schedule and signals intent to start fresh).

osr21 · 2026-07-23T14:10:06Z

Both issues from this review addressed in commit 40c9e9b.

**1. [High] Duplicate reconnect after onClose callback**

The root cause: a user's `onClose` callback that calls `client.connect()` directly would increment `connectionId`, but `scheduleReconnect()` still ran unconditionally after the callback returned — opening a second socket on top of the one the callback just created.

Fix: re-check `id === this.connectionId` *after* the user callback. If the callback called `connect()`, `connectionId` has already advanced and we skip `scheduleReconnect()`:

```ts
private onClose = (id: number, message: CloseEvent) => {
  if (id !== this.connectionId) return;
  this.notifyStatusChange(ConnectionStatus.DISCONNECTED);
  if (this.onUserClose) {
      try { this.onUserClose(this, message); } catch (e) { ... }
  }
  // Re-check: onUserClose may have called connect(), advancing connectionId.
  // If so, a socket is already opening — do not schedule a second one.
  if (this.autoReconnect && id === this.connectionId) {
      this.scheduleReconnect();
  }
};
```

**2. [Medium] Backoff counter survives disconnect**

`reconnectAttempt` was only reset on `onOpen`. After many failed auto-reconnects, a manual `disconnect()` / `connect()` cycle inherited the large exponent, making the very first failure after reconnect wait up to `maxReconnectDelay` instead of the initial `reconnectDelay`.

Fixed by resetting `reconnectAttempt = 0` in two places:
- **`disconnect()`** — a clean stop should always clear the accumulated back-off history.
- **`connect()` when it cancels a pending timer** — the caller is manually overriding the schedule and signalling intent to start fresh, so the next failure run should begin from the initial delay.

```ts
public connect() {
  if (this.reconnectTimer !== null) {
      clearTimeout(this.reconnectTimer);
      this.reconnectTimer = null;
      this.reconnectAttempt = 0; // manual connect resets back-off
  }
  // ...
}

public disconnect() {
  this.autoReconnect = false;
  this.reconnectAttempt = 0;   // start fresh if reconnected later
  // ...
}
```

…vent resource leak Addresses Graphite review: calling connect() while a socket is still active abandoned the old socket on the network without closing it, leaking file descriptors and browser/server connection slots over time. Fix: explicitly close this.ws before assigning the new WebSocket, guarded by the same CLOSED/CLOSING readyState check used in disconnect(). Critical ordering note: connectionId is incremented BEFORE ws.close() is called. The old socket's onClose/onError handlers capture the previous id and early-return on the id !== connectionId guard, so the deliberate close does not emit a spurious DISCONNECTED notification or trigger scheduleReconnect(). Graphite's own suggested snippet closed the socket before incrementing connectionId, which would have caused exactly those side-effects.

osr21 · 2026-07-23T14:14:58Z

Thanks for the catch @graphite-app — fixed in commit 3d06286.

**Fix: close the previous socket before replacing it**

`connect()` now explicitly closes `this.ws` before assigning the new WebSocket, using the same `CLOSED`/`CLOSING` readyState guard already present in `disconnect()`:

```ts
public connect() {
  if (this.reconnectTimer !== null) { ... }

  // Increment connectionId FIRST — old handlers see a stale id and early-return.
  const id = ++this.connectionId;

  // Now safe to close the old socket: its onClose/onError will not emit
  // DISCONNECTED or trigger scheduleReconnect() because id !== connectionId.
  if (this.ws && this.ws.readyState !== WebSocket.CLOSED && this.ws.readyState !== WebSocket.CLOSING) {
      this.ws.close();
  }

  this.notifyStatusChange(ConnectionStatus.CONNECTING);
  this.ws = new WebSocket(this.host);
  ...
}
```

**Ordering note**: `connectionId` is incremented **before** `ws.close()` is called. The old socket's `onClose`/`onError` handlers capture the previous id in their closure and early-return on the `id !== this.connectionId` guard, so the deliberate `close()` emits neither a spurious `DISCONNECTED` status update nor a `scheduleReconnect()` call. The code in your suggestion had the close before the increment, which would have triggered both side-effects.

…chains Addresses Cursor Bugbot review (commit 3d06286): the onPong handler scheduled the next ping() via delay(...).then(() => this.ping()) with no reference to connectionId. When connect() replaced a socket, any pending delay from the old onPong chain resolved and called ping() against this.ws (the new socket), duplicating keepalive traffic on the current connection. Fix: onPong now receives the connection id captured at connect() time and passes it through to the delay callback. The callback checks id === connectionId before calling ping() — if the connection has been superseded the chain stops. To wire the id through cleanly, the pong handler is registered as this.ws.pong = (data: Buffer) => this.onPong(id) instead of the bare this.onPong assignment, matching the pattern already used for onOpen, onClose, and onError.

osr21 · 2026-07-23T14:20:26Z

Fixed in commit aaa5384.

**Stale onPong keeps pinging**

The root cause: `onPong` scheduled the next ping via `delay(...).then(() => this.ping())` with no link to `connectionId`. When `connect()` superseded the socket, any pending delay from the old pong chain resolved and called `ping()` against `this.ws` (the new socket), duplicating keepalive traffic.

Fix: `onPong` now receives the `id` captured at `connect()` time and re-checks it inside the delay callback. If the connection has been superseded the chain stops:

```ts
// Registration in connect():
this.ws.pong = (data: Buffer) => this.onPong(id);

// Handler:
private onPong = (id: number) => {
  delay(this.pingInterval).then(() => {
      if (id === this.connectionId) {
          this.ping();
      }
  });
};
```

This matches the pattern already used by `onOpen`, `onClose`, and `onError`, making all four event handlers consistently gated by the same `connectionId` guard.

cursor

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

^{Reviewed by Cursor Bugbot for commit aaa5384. Configure here.}

…as advanced Addresses Cursor Bugbot review (commit aaa5384): when connect() ran before the prior socket's close event was handled, onClose returned immediately on the id !== connectionId guard — silently dropping the DISCONNECTED notification, the onUserClose callback, and the status update. This happened whenever: - onUserError (or onError) called connect() before close fired - connect() followed disconnect() before the close callback ran Root cause: the early-return guard conflated two independent questions: 1. Should we notify? (emit DISCONNECTED + invoke onUserClose) 2. Should we reconnect? (scheduleReconnect) Fix: introduce lastConnectedId, set to the socket's id inside onOpen. onClose now answers both questions separately: const wasConnected = id === this.lastConnectedId; // reached OPEN state const isCurrent = id === this.connectionId; // no replacement yet // Notify if the socket was ever connected — a CONNECTED was published // so a paired DISCONNECTED must follow, even if a new socket is already // opening. A socket replaced before it opened is silently dropped. if (!wasConnected && !isCurrent) return; notifyStatusChange(DISCONNECTED); onUserClose(...); // Reconnect only if this is still the current connection and the user // callback did not itself call connect(). if (autoReconnect && isCurrent && id === connectionId) scheduleReconnect();

osr21 · 2026-07-23T14:25:42Z

Fixed in commit d7f0918.

**Stale close skips disconnect notifications**

The single `id !== connectionId` early-return was answering two independent questions with one check:
1. *Should we notify?* — emit `DISCONNECTED` + invoke `onUserClose`
2. *Should we reconnect?* — call `scheduleReconnect()`

When `connect()` advanced `connectionId` before the old socket's `close` event fired (e.g. because `onUserError` called `connect()`, or `connect()` followed `disconnect()` before the callback ran), the early-return silently dropped both — so `DISCONNECTED` was never emitted and `onUserClose` was never called for a socket that had genuinely been active.

**Fix: introduce `lastConnectedId`**, set to the socket's `id` inside `onOpen`. `onClose` now answers both questions separately:

```ts
private onClose = (id: number, message: CloseEvent) => {
  const wasConnected = id === this.lastConnectedId; // reached OPEN state
  const isCurrent    = id === this.connectionId;    // no replacement started yet

  // Notify if this socket ever published CONNECTED — it deserves a paired
  // DISCONNECTED even if a replacement is already opening.
  // A socket replaced before it ever opened is silently dropped (no CONNECTED
  // was emitted for it, so there is no paired DISCONNECTED to emit).
  if (!wasConnected && !isCurrent) return;

  this.notifyStatusChange(ConnectionStatus.DISCONNECTED);
  if (this.onUserClose) { ... }

  // Reconnect only if no replacement socket has started yet, and the user's
  // onClose callback did not itself call connect().
  if (this.autoReconnect && isCurrent && id === this.connectionId) {
      this.scheduleReconnect();
  }
};
```

`lastConnectedId` is set in `onOpen` (the only place `CONNECTED` is published), so the pairing is exact: every `CONNECTED` notification is guaranteed a matching `DISCONNECTED` regardless of when `connectionId` advances.

Addresses Graphite review (commit d7f0918): when a socket closed before onOpen fired (failed handshake, immediate network error), isCurrent was true so the guard passed and DISCONNECTED was emitted unconditionally. This produced CONNECTING → DISCONNECTED without CONNECTED in between, violating the state machine contract. Fix: gate notifyStatusChange(DISCONNECTED) and the onUserClose callback behind wasConnected (id === lastConnectedId). A socket that never reached CONNECTED published no status event for it, so no paired DISCONNECTED should follow. The reconnect logic remains gated on isCurrent only — a failed handshake is still a valid reason to retry, with or without CONNECTED. Resulting state machine guarantees: CONNECTING → CONNECTED → DISCONNECTED (normal session drop) CONNECTING → CONNECTED → CONNECTING (onUserClose calls connect()) CONNECTING → CONNECTING (failed handshake, retried silently) CONNECTING → <stopped> (failed handshake, autoReconnect off)

osr21 · 2026-07-23T15:00:26Z

Thanks @graphite-app — both points addressed.

**Issue 1 (commit 7ff91d62) — resource leak in connect()**
Already fixed in commit 3d062860: `connect()` closes the previous socket before assigning the new one, with `connectionId` incremented first so the old handlers early-return silently.

**Issue 2 (commit d7f0918b) — DISCONNECTED emitted without CONNECTED (fixed in 33052dcb)**

The previous guard let `isCurrent && !wasConnected` pass through, emitting `DISCONNECTED` for a socket that closed before `onOpen` ever fired (failed handshake, immediate network error). That produced `CONNECTING → DISCONNECTED` without `CONNECTED` in between — a state machine violation.

Fix: gate `notifyStatusChange(DISCONNECTED)` and `onUserClose` behind `wasConnected` (i.e. `id === lastConnectedId`, set only in `onOpen`). The reconnect path stays gated on `isCurrent` only — a failed handshake is still a valid reason to schedule a retry, even without CONNECTED having been published.

```ts
if (!wasConnected && !isCurrent) return; // fully stale — drop silently

// Status notification: only when the socket actually reached CONNECTED
if (wasConnected) {
  this.notifyStatusChange(ConnectionStatus.DISCONNECTED);
  if (this.onUserClose) { ... }
}

// Reconnect: whenever the current socket closes, connected or not
if (this.autoReconnect && isCurrent && id === this.connectionId) {
  this.scheduleReconnect();
}
```

Resulting state machine guarantees:
| Scenario | Sequence |
|---|---|
| Normal session drop | `CONNECTING → CONNECTED → DISCONNECTED` |
| onUserClose calls connect() | `CONNECTING → CONNECTED → CONNECTING` |
| Failed handshake, autoReconnect on | `CONNECTING → CONNECTING` (silent retry) |
| Failed handshake, autoReconnect off | `CONNECTING → <stopped>` |

osr21 · 2026-08-03T08:27:40Z

The exponential back-off and onClose/onError callbacks in this PR match what we found necessary running a server-side RTDS subscriber in production (Node 24, connecting to the TWAP feed).

A few notes from that experience that might be useful before merge:

1. autoReconnect ?? true fix is essential

The ||→?? change is the highest-priority item here. The original args!.autoReconnect || true makes it impossible to disable auto-reconnect in any environment, which is a footgun for test harnesses and graceful shutdown sequences. Confirmed working with ??.

2. Consider gating reconnect on close code

Some close codes indicate a permanent server-side rejection (e.g. 4000–4003 on RTDS) where retrying makes no sense. A simple allow-list check before scheduling reconnect avoids hammering a rejected connection:

private onClose = (event: CloseEvent) => {
  this.onCloseCallback?.(event);
  const permanent = event.code >= 4000 && event.code <= 4003;
  if (this.autoReconnect && !permanent) {
    this.scheduleReconnect();
  }
};

3. Pre-launch topic detection

Before the TWAP topics go live on Aug 4, subscribing to crypto_prices_twap_thirty / crypto_prices_twap_sixty returns an error frame (not a close event):

{ "type": "error", "message": "topic not found: crypto_prices_twap_thirty" }

Detecting this in onMessage and backing off 60 s before retrying avoids log spam during the pre-launch window.

Relation to #45

This PR is a strict superset of #45 (same ?? fix + onClose/onError) with the addition of exponential back-off. Would suggest closing #45 in favour of this one.

osr21 requested a review from a team as a code owner July 23, 2026 13:46