Skip to content

fix(graphql-react-ws): forward fatal per-operation errors to onError - #178

Merged
jswirbs merged 1 commit into
mainfrom
fix/operation-error-swallowed
Jul 23, 2026
Merged

fix(graphql-react-ws): forward fatal per-operation errors to onError#178
jswirbs merged 1 commit into
mainfrom
fix/operation-error-swallowed

Conversation

@jswirbs

@jswirbs jswirbs commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

A fatal, per-operation transport error (the server terminates just this subscription — e.g. a duplicate-operation-id conflict — as opposed to a full socket close) was silently resolving every consumer of that subscription as gracefully complete instead of erroring, with zero signal to the caller.

Two compounding bugs:

  1. graphqlWsSubscribe's error callback calls cleanupGenerator(), which synchronously aborts every listener's AbortController via onDispose(). That abort synchronously runs subscribe()'s cleanupListener, which calls listener.resolveCompleted() — resolving the listener as done, not errored. This races ahead of (and wins against) the async rejection that would otherwise propagate through BroadcastAsyncGenerator.broadcast()'s for await loop, so the real error never reaches the listener.
  2. Even when a rejection does propagate, useSubscription's effect had no try/catch around its own for await (const result of iterator), so the rejection was swallowed by subscribe()'s generic .catch(console.error) further up the stack — onError never fires.

Fix

  • Added BroadcastAsyncGenerator.rejectAll(error), called synchronously before the abort cascade in the error callback, so every listener's done flag is already true (with the real error recorded) by the time the abort-triggered resolveCompleted() runs — making that call a no-op instead of masking the error.
  • Wrapped useSubscription's for await loop in a try/catch that forwards the caught error into onErrorCallback/store.error, matching the existing handling for GraphQL-level result.errors.
  • Added a repro test (client.test.ts) that mocks graphql-ws, fires a fatal per-operation error, and asserts onError fires — fails without either fix, passes with both.
  • Excluded test files from the published dist output (they were being bundled in via entryPoints: ['src']).

Test plan

  • pnpm --filter @soundxyz/graphql-react-ws test — repro test passes
  • pnpm --filter @soundxyz/graphql-react-ws prepare — builds cleanly, dist contains no test files
  • Validated against the 2.0.0 API surface (via a separate worktree) to confirm this is a clean, non-breaking patch for existing consumers
  • pnpm prettier:check passes

Made with Cursor

Previously, a server-terminated operation (as opposed to a full socket
close) silently resolved every consumer of that subscription as
gracefully complete instead of errored. `graphqlWsSubscribe`'s `error`
callback synchronously aborted every listener's `AbortController` before
the async rejection could propagate through `BroadcastAsyncGenerator`'s
`for await` loop, so the abort-triggered `resolveCompleted()` always won
the race and masked the error. Even when the rejection did propagate,
`useSubscription`'s effect had no `try/catch` around its own `for await`,
so it was swallowed by `subscribe()`'s generic `.catch(console.error)`.

Adds `BroadcastAsyncGenerator.rejectAll()` (called before the abort
cascade) and wraps `useSubscription`'s loop in a `try/catch` that forwards
into `onError`/`store.error`. Includes a repro test that fails without
either fix.

Co-authored-by: Cursor <cursoragent@cursor.com>
@changeset-bot

changeset-bot Bot commented Jul 8, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: c9d349b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@soundxyz/graphql-react-ws Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jswirbs
jswirbs merged commit b420de5 into main Jul 23, 2026
2 checks passed
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