Fixed heap out of memory on network issues

[aizo231]

Heap out of memory error happened, because WebSocket instance was not terminated when establishing new one, leading to massive spike in memory on network errors.

---

Note

Medium Risk
Changes core connection lifecycle and reconnect behavior; incorrect cleanup could drop subscriptions or cause double-reconnect edge cases, but the fix is narrowly scoped to teardown and guards.

Overview
Fixes heap out-of-memory when network failures trigger repeated reconnects by fully tearing down the previous WebSocket before opening a new one.

On error and close, the client now removes listeners, calls terminate(), and sets this.ws to null before autoReconnect runs connect() again. Error handling also emits DISCONNECTED via notifyStatusChange. Ping/pong, disconnect, subscribe, and unsubscribe guard against a missing socket so stale timers or API calls cannot touch a dead instance.

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

[osr21]

Review: Fixed heap out of memory on network issues (#39)

Addressing memory growth under sustained network failures is important. A few questions:

1. Is the growth from unbounded reconnect event accumulation, or from the `ping` chain continuing after disconnect?
2. Does this cap reconnect frequency, or only the memory growth?

For context: PR #46 adds exponential back-off (`min(delay × 2^attempt, 30s) + jitter`) which should address thundering-herd scenarios. Worth checking for overlap before merging both.

[aizo231]

I think it's because it doesn't reconnect old socket, but calls new connect function, in which new socket is initialized, so I just deleted the old one manually not relying on GC and it worked, and this solution doesn't cap reconnect time, because we know time is money.

[osr21]

Thanks for clarifying the root cause — that's exactly right. The orphaned sockets survive GC because the ws library stores onopen/onclose/onerror property handlers as EventEmitter listeners internally, which creates a reference chain from the old socket back into the client instance. Calling terminate() (vs the graceful close()) is specifically correct here because it forces an immediate TCP RST, releasing the socket from the ws library's internal handle map without waiting for the FIN/FIN-ACK exchange — that's the piece that was keeping sockets alive under rapid reconnects.

The ordering you chose — `removeAllListeners()` before `terminate()` — is also right. Without it, `terminate()` emits `close` on the old socket, which would trigger `onClose`, which would call `connect()` again alongside the one already called in `onError`. You'd get a double-connect race. So that ordering matters.

**One new issue introduced by this PR**

After `this.ws = null`, `subscribe()` and `unsubscribe()` crash immediately:

```ts
public subscribe(msg: SubscriptionMessage) {
  if (this.ws.readyState !== WebSocket.OPEN) {  // TypeError: Cannot read properties of null
```

The field is declared as `private ws!: WebSocket` (non-null assertion), so TypeScript won't catch this. Any caller that calls `subscribe()` between disconnect and reconnect will throw. A null guard is needed:

```ts
public subscribe(msg: SubscriptionMessage) {
  if (!this.ws || this.ws.readyState !== WebSocket.OPEN) {
      return console.warn("Socket not open. Ready state is:", this.ws?.readyState);
  }
  // ...
}
```

**On the reconnect delay**

"Time is money" makes sense for a transient blip (server restart, brief network hiccup) — zero delay means you're reconnected in one RTT. But under a sustained outage it inverts: each failed TCP handshake returns a RST or times out in seconds, and at zero delay the client hammers the endpoint as fast as the OS allows — easily hundreds of attempts per minute. Polymarket's load balancer will see that as abuse traffic and can temporarily block the IP. When the endpoint recovers, you're now locked out.

A minimal base delay of ~1 second has no perceptible cost for a transient failure (reconnected in ~1s + 1 RTT) and prevents the IP block entirely under sustained outages. It doesn't need to be full exponential backoff to be effective:

```ts
if (this.autoReconnect) {
  delay(1000).then(() => this.connect());
}
```

That single line is the difference between "reconnects immediately after a 1s blip" and "gets rate-blocked and can't reconnect when the outage ends."

[aizo231]

To be honest, I don't know TS at all. I'm JS developer. I kinda doesn't understand types at all.

[osr21]

No worries — the TypeScript part doesn't matter here, the crash is a plain JavaScript runtime problem.

After your fix runs `this.ws = null`, there's a window (between disconnect and reconnect) where `subscribe()` can be called by a user of the library. That method does this:

```js
if (this.ws.readyState !== WebSocket.OPEN) {  // boom — this.ws is null
```

In plain JavaScript that throws `TypeError: Cannot read properties of null (reading 'readyState')`. TypeScript would normally warn about it at compile time, but because the original author wrote `ws!` (a "trust me, it's never null" annotation), the TypeScript compiler stays quiet — so the bug slips through to runtime. The fix is the same whether you write TS or JS:

```js
// change this:
if (this.ws.readyState !== WebSocket.OPEN) {

// to this:
if (!this.ws || this.ws.readyState !== WebSocket.OPEN) {
```

Same change needed in `unsubscribe()` for the same reason.

Your memory fix is solid — this is just a small follow-up to make the null you're now deliberately setting safe to live with.

[aizo231]

Okey, I think solution could be hold on subscribe, and handle it on connect.

[aizo231]

Maybe even a quite bigger rework of whole think... User trough subscribe / unsubscribe regulates set of subscription messages, that are fired on moment called and also onOpen, if somebody has subscriptions in onOpen there could be a tracking of already fired subscribe messages after onOpen when subscribtions could be fired once again, so it won't fire already fired, but in the same loop it would free them to fire again, if called on already opened socket, so both working with subscribe before object exist, and after it would be possible without interfering with backwards compatibility.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 3 potential issues.

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

[cursor]

Stale handler kills new socket

High Severity

In onClose and onError, teardown uses this.ws instead of the socket that raised the event. After one handler sets this.ws to null and connect() assigns a new instance, a late close/error from the previous socket can terminate the active connection and trigger another reconnect.

Additional Locations (1)

• src/client.ts#L125-L132

 

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

[cursor]

Browser incompatible listener cleanup

High Severity

New teardown calls removeAllListeners on this.ws, but isomorphic-ws uses the native browser WebSocket, which has no such method. On network errors the handler throws before terminate, nulling, or reconnect run, so orphaned sockets and the heap growth can persist in browser builds.

Additional Locations (1)

• src/client.ts#L144-L145

 

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

[osr21]

That's the right direction — it's called a subscription store and it's how production WebSocket clients handle reconnects. Here's how it maps to this codebase concretely.

**The store**

Add one `Map` to the class to hold active subscriptions:

```js
// keyed by "topic:type" — stable identity across sessions
this._subscriptions = new Map();
```

**subscribe / unsubscribe**

```js
subscribe(msg) {
  // Register in store (survives reconnect)
  for (const sub of msg.subscriptions) {
      this._subscriptions.set(sub.topic + ':' + sub.type, sub);
  }
  // Send immediately only if connected
  if (this.ws && this.ws.readyState === WebSocket.OPEN) {
      this.ws.send(JSON.stringify({ action: 'subscribe', ...msg }));
  }
  // else: onOpen will replay it
}

unsubscribe(msg) {
  // Remove from store so it doesn't come back on reconnect
  for (const sub of msg.subscriptions) {
      this._subscriptions.delete(sub.topic + ':' + sub.type);
  }
  if (this.ws && this.ws.readyState === WebSocket.OPEN) {
      this.ws.send(JSON.stringify({ action: 'unsubscribe', ...msg }));
  }
}
```

**onOpen — replay before firing the user callback**

```js
onOpen = async () => {
  this.ping();
  // Replay all stored subscriptions first
  if (this._subscriptions.size > 0) {
      const subs = Array.from(this._subscriptions.values());
      this.ws.send(JSON.stringify({ action: 'subscribe', subscriptions: subs }));
  }
  this.notifyStatusChange(ConnectionStatus.CONNECTED);
  if (this.onConnect) this.onConnect(this);
};
```

Replaying before `onConnect` means by the time the user's callback fires, the server already has the subscriptions active. No extra tracking of "already fired" is needed — the Map is the source of truth, and it's rebuilt fresh on every `onOpen`.

**One gotcha with the key**

Looking at `SubscriptionMessage`, each subscription object can contain `clob_auth` or `gamma_auth` credentials. Those rotate per session, so you can't use the full `JSON.stringify(sub)` as the key — the same logical subscription would get a different key every reconnect and pile up in the Map. Using `topic + ':' + type` as the key (the stable identity) avoids that. When you replay, just use the stored sub object as-is (the new session's auth gets set by whoever called subscribe, or the server re-authenticates via the open handshake).

**Backwards compatibility**

Calling `subscribe()` on an already-open socket still sends immediately, same as before. The only behavioural addition is that it also survives reconnect — no existing code breaks.