Skip to content

fix(websocket): report errors as Error objects and recover from auth failures - #198

Open
thorbenziemek wants to merge 1 commit into
mainfrom
fix/websocket-error-handling
Open

thorbenziemek wants to merge 1 commit into
mainfrom
fix/websocket-error-handling

Conversation

@thorbenziemek

Copy link
Copy Markdown
Contributor

Problem

The websocket client reported almost nothing to applications, and what it did report was not usable as an error.

  • Subscription callbacks received the raw wire objects ({ error: { message } }). err instanceof Error was false, err.message was undefined (the message sits at err.error.message), and there was no name, code or stack. Consumers had to reconstruct an error from the payload and decide severity by matching the literal string "authentication failed" — the same string the client itself matches on.
  • onerror was never assigned, a rejected Auth.getToken() was only console.error-ed, and JSON.parse on inbound frames was unguarded. These escaped as uncaught exceptions (with the ws package, an unhandled error event takes down the process) and reached no subscriber.
  • Because _errorCount was only incremented from onmessage, real connection failures never escalated the backoff: a client retried every ~0.6 s indefinitely against a down server.
  • A rejected getToken() left an open, unauthenticated socket. this._socket stayed non-null, so the reconnect interval's !this._socket guard never fired and the client hung permanently with no signal.
  • An authentication failure bricked the instance for good, and the documented remedy — recreate it — was not reachable from outside, since AuthorizedApiContext owns the client internally.
  • Callback exceptions were only contained in the fatal-error path. Elsewhere one throwing subscriber aborted the forEach and silently starved every subscriber after it.

This is the cause of the crashes seen downstream in bg-trading during the 2026-10-05 outage ("Object captured as exception with keys: error"), which that repo currently works around by converting the payload into an error itself.

Change

Errors are errors. BrokerizeWebSocketError extends BrokerizeError carries a stable code (AUTH, WEBSOCKET_CONNECTION_FAILED, WEBSOCKET_PROTOCOL_ERROR, WEBSOCKET_ERROR), the matching httpStatusCode, an optional cause and a terminal flag. An authentication failure maps to AUTH / 401, so it can be handled like a rejected token refresh on the REST API. isBrokerizeWebSocketError() is exported for name-based checks, which survive duplicate copies of the package in a dependency tree.

Two channels, with a clear contract. Errors delivered to a subscription callback are now always terminal: the subscription is over. Everything the client recovers from on its own goes to the new subscribeConnectionState(cb), which reports closed | connecting | open | reconnecting | fatal plus the causing error, and emits the current state on subscribe. Applications can show that live updates are unavailable instead of tearing down subscriptions on every hiccup — and, importantly, mapping the callback's err straight onto an RxJS subscriber.error() stays correct.

Authentication failures are recoverable. The usual cause is an id token that expired in flight, so the client now retries once with getToken({ forceRefresh: true }) and only escalates to fatal if that is rejected as well. A successful authentication restores the retry budget. A client that did go fatal can be revived with reconnect(); its subscriptions were terminated, so callers subscribe again.

The silent failure paths now report. onerror, a rejected getToken() (which also closes the socket so the reconnect interval takes over, instead of hanging on an unauthenticated connection) and unparsable frames all produce a BrokerizeWebSocketError and count towards the backoff — exactly once per failed attempt, whether it surfaces as an error event, a close event or both.

Lifecycle fixes.

  • _invokeCallback contains every subscriber call, so a throwing subscriber cannot starve the others.
  • onclose no longer nulls this._socket unconditionally. It closed over this, not over its own socket, so the late close of a replaced socket discarded a freshly created connection and leaked the old one. Handlers are now detached on close and every handler ignores events from an outdated socket.
  • else if (message as WebSocketError) was a cast, not a check, and matched every unrecognised message. It now checks .error and logs genuinely unknown messages.
  • Subscribing on a fatal client reports asynchronously, so the caller holds the Subscription before the callback runs.
  • _endSubscription no longer opens a connection just to send an unsubscribe, and tolerates an entry that is already gone.
  • New destroy() closes the socket, drops subscriptions and clears the reconnect and ping timers. AuthorizedApiContext.destroy() calls it for the client it created itself — previously that client kept reconnecting forever with the tokens of a context that was gone. A client passed in by the application is left alone.
  • A request on a destroyed AuthorizedApiContext now throws BrokerizeAbortError, whose name is AbortError. destroy() aborts the context's AbortController, so a request already in flight rejects with a DOMException named AbortError; both cases can now be handled with one check, the way fetch behaves with an already-aborted signal.

API

Additive, except that BrokerizeWebSocketClient gains subscribeConnectionState, getConnectionState and reconnect. Applications that wrap a client (the normal case) are unaffected; an application that implements the interface itself has to add them. destroy() is deliberately not on the interface, so that a client obtained from createWebSocketClient() cannot be used to tear down the context's shared client. Auth.getToken and CognitoFacade's session take an optional GetTokenOptions, which existing implementations may ignore.

Newly exported: BrokerizeWebSocketError, BrokerizeWebSocketErrorCode, BrokerizeWebSocketErrorParams, isBrokerizeWebSocketError, BrokerizeAbortError, GetTokenOptions, WebSocketConnectionState, WebSocketConnectionStatus, ConnectionStateCallback.

Server-side follow-up

WebSocketTypes.BrokerizeWebSocketError (the wire payload) gained an optional code, which isAuthenticationError prefers over matching message. The backend does not send it yet, so the string match remains as a fallback. src/websocketTypes.ts is a manual copy of the backend file and needs this field added there.

Tests

npm test (new, node:test, no new dependencies, wired into the Build workflow) covers the error conversion, the auth retry and the escalation to fatal, the restored retry budget, reconnect(), the asynchronous report on a dead client, callback containment, unparsable messages, the getToken() rejection, backoff accounting, the outdated-socket close event and destroy(). 11 passing. tsc, eslint and prettier clean; API report regenerated.

🤖 Generated with Claude Code

…failures

The websocket client only ever reported server-sent error messages, as plain
`{ error: { message } }` objects, and permanently bricked itself on an
authentication failure. Connection-level failures reached nobody at all and did
not count towards the backoff.

- All errors are now `BrokerizeWebSocketError extends BrokerizeError`, with a
  stable `code` and a `terminal` flag.
- Errors passed to a subscription callback are always terminal; errors the
  client recovers from are reported via the new `subscribeConnectionState`.
- A rejected token is retried once with a forced refresh before the client goes
  fatal, and `reconnect()` revives a fatal client.
- `onerror`, a rejected `getToken()` and unparsable messages are handled
  instead of escaping as uncaught exceptions, and count towards the backoff.
- Subscriber callbacks are contained, so one throwing subscriber no longer
  starves the others.
- `AuthorizedApiContext.destroy()` destroys the websocket client it created,
  and reports requests on a destroyed context as `AbortError`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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