Skip to content

fix: clean up subscriptions and dataStore on unsubscribe in ws-client#574

Closed
realfishsam wants to merge 1 commit into
mainfrom
fix/553-ws-client-unbounded
Closed

fix: clean up subscriptions and dataStore on unsubscribe in ws-client#574
realfishsam wants to merge 1 commit into
mainfrom
fix/553-ws-client-unbounded

Conversation

@realfishsam

Copy link
Copy Markdown
Contributor

Fixes #553

@realfishsam

Copy link
Copy Markdown
Contributor Author

PR Review: FAIL

What This Does

Adds subscription deduplication to subscribeBatch, cleans up dataStore entries after collection, and clears all maps on close() in the TS SDK WebSocket client. Targets unbounded memory growth from repeated batch subscriptions.

Blast Radius

  • SDK: sdks/typescript/pmxt/ws-client.ts only
  • No core, OpenAPI, or Python SDK changes

Findings

  1. BLOCKING -- subscribeBatch dedup creates stale-data bug: The dedup block (lines added at top of subscribeBatch) reuses an existing subscription and calls this.waitForData(existingId, timeoutMs). But waitForData (line 327-358) checks this.dataStore.get(requestId) and if data exists, immediately resolves and deletes it. Then collectBatchResult is called, which also tries this.dataStore.get(storeKey) -- but the data was already deleted by waitForData. The result will be an empty object {} on the second call to the same subscription, losing all data. This is a silent data-loss bug.

  2. BLOCKING -- collectBatchResult deletes data that subscribe (non-batch) still needs: The subscribe method on main (line 199-246) already has its own activeSubs dedup and waitForData flow. But subscribeBatch now deletes from dataStore in collectBatchResult. If subscribe and subscribeBatch ever share a requestId (they don't today, but activeSubs keys could collide if the same method+symbols are used), the deletion would cause data loss in the other path.

  3. close() cleanup is correct: Clearing subscriptions, dataStore, and activeSubs on close is a good practice and prevents memory leaks on reconnection.

  4. collectBatchResult extraction is clean: The refactor to extract the collection logic into a helper is a good structural improvement.

PMXT Pipeline Check

  • Field propagation: N/A
  • OpenAPI sync: N/A
  • Type safety: OK

Semver Impact

patch -- internal fix, no public API change

Risk

Finding #1 is a data-loss regression. When subscribeBatch is called twice for the same symbol set, the second call will return an empty object because waitForData consumes the data before collectBatchResult can read it. This needs to be fixed before merge -- either waitForData should not delete data when called from the dedup path, or the dedup path should skip waitForData and go directly to collectBatchResult with freshness checks.

@realfishsam realfishsam deleted the fix/553-ws-client-unbounded branch May 24, 2026 17:06
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.

Unbounded: sdks/typescript/pmxt/ws-client.ts — subscriptions and dataStore never evicted in subscribeBatch

1 participant