Skip to content

Comments

Fix stale subscription client after auth mode change#956

Open
pistachiobaby wants to merge 1 commit intomainfrom
nick/fix-live-query-subscription-client-reset
Open

Fix stale subscription client after auth mode change#956
pistachiobaby wants to merge 1 commit intomainfrom
nick/fix-live-query-subscription-client-reset

Conversation

@pistachiobaby
Copy link
Contributor

Summary

resetClients() and close() in GadgetConnection disposed the WebSocket subscription client without clearing the baseSubscriptionClient reference. This caused getBaseSubscriptionClient() to return a disposed graphql-ws client on subsequent calls instead of creating a fresh one — meaning live queries silently broke after any enableSessionMode() call (auth refresh, switching to browser session auth, etc.).

Also adds the first-ever test coverage for auth credentials in WebSocket connectionParams, which previously had zero tests across all five auth modes.

Architecture

Regular queries and live queries take completely different transport paths through GadgetConnection:

                    GadgetConnection
                          │
              ┌───────────┴───────────┐
              │                       │
      Regular queries            Live queries
      (HTTP fetch)             (WebSocket subscription)
              │                       │
      requestHeaders()        connectionParams()
      ← called per request    ← called on WS connect
              │                       │
         baseClient            baseSubscriptionClient
      (urql Client)           (graphql-ws Client, lazy)
              │                       │
      ┌───────────┐          ┌────────────────┐
      │ New fetch  │          │ Persistent WS  │
      │ per query  │          │ connection     │
      └───────────┘          └────────────────┘

baseSubscriptionClient is lazily initializedgetBaseSubscriptionClient() creates it on first live query, then reuses it:

private getBaseSubscriptionClient() {
    if (!this.baseSubscriptionClient) {                          // ← guard
        this.baseSubscriptionClient = this.newSubscriptionClient({ lazy: true });
    }
    return this.baseSubscriptionClient;
}

The Bug: Dispose Without Clear

enableSessionMode() calls resetClients() to rebuild the urql pipeline with new auth. Before the fix:

resetClients()
    │
    ├─ this.disposeClient(this.baseSubscriptionClient)
    │      └─ calls graphql-ws client.dispose()
    │         └─ sets internal `disposed = true`, closes WebSocket
    │
    ├─ this.baseSubscriptionClient = ???
    │      └─ ⚠️ NOTHING — still holds the disposed client object
    │
    └─ this.baseClient = this.newBaseClient()
           └─ new urql Client with fresh subscription exchanges
              └─ exchanges call this.getBaseSubscriptionClient()

After reset, getBaseSubscriptionClient() runs:

if (!this.baseSubscriptionClient)     → false (stale disposed object is truthy)
return this.baseSubscriptionClient    → returns disposed client ← BUG

The graphql-ws library sets disposed = true internally on dispose(). A disposed client:

  • Can attempt new connections (the initial subscribe() still tries to open a WebSocket)
  • Cannot reconnect after failures (if (disposed) return false in retry logic)
  • Cannot recover from network interruptions

So live queries appear to work initially but silently die on the first network hiccup, with no reconnection.

Concrete Scenarios

1. Happy path (with fix) — Auth refresh re-establishes live queries

1. App starts with API key auth
2. Live query mounts → getBaseSubscriptionClient() creates client A
3. Client A connects, connectionParams sends { auth: { type: "api-key", key: "gsk-..." } }
4. User switches to browser session → enableSessionMode({ initialToken: "tok-123" })
5. resetClients() disposes client A AND sets baseSubscriptionClient = undefined  ← FIX
6. Live query re-subscribes → getBaseSubscriptionClient() creates client B (fresh)
7. Client B connects, connectionParams sends { auth: { type: "browser-session", sessionToken: "tok-123" } }

2. Failure mode (without fix) — Live queries go stale after auth change

1. App starts with API key auth
2. Live query mounts → getBaseSubscriptionClient() creates client A
3. enableSessionMode() → resetClients() disposes client A but does NOT clear the reference
4. Live query re-subscribes → getBaseSubscriptionClient() returns disposed client A
5. Client A opens a new WebSocket (initial connect works despite disposed=true)
6. Network blip occurs → WebSocket drops
7. graphql-ws retry logic: if (disposed) return false → NO RECONNECTION
8. Live query is permanently dead — user sees stale data with no errors

3. Edge case — close() had the same leak

1. connection.close() disposes the subscription client
2. Something later calls getBaseSubscriptionClient()
3. Without fix: returns the disposed client (same failure mode)
4. With fix: baseSubscriptionClient is undefined, creates a fresh client

Key insight: The fix is two lines (this.baseSubscriptionClient = undefined) but the impact is significant — without it, any call to enableSessionMode() after a live query has been established permanently breaks WebSocket reconnection for all live queries on that connection.

Changes

Bug fix:

  • packages/api-client-core/src/GadgetConnection.ts — Clear baseSubscriptionClient reference after disposing in both resetClients() and close()

Test coverage (previously zero for subscription auth):

  • packages/api-client-core/spec/GadgetConnection-suite.ts — 8 new tests:
    • 5 auth connectionParams tests (API key, browser session, internal, custom, anonymous)
    • 3 lifecycle tests (resetClients clears reference, new client gets updated auth, close() clears reference)

Test Plan

  • New lifecycle tests fail without the fix (6 failures across browser + node environments), pass with it
  • New auth connectionParams tests pass (verify all 5 auth modes send correct credentials)
  • All 120 GadgetConnection tests pass (3 suites: browser, node, remote-ui)
  • All 359 api-client-core tests pass
  • liveQueryExchange tests pass (7 tests)

🤖 Generated with Claude Code

resetClients() and close() disposed the baseSubscriptionClient without
clearing the reference, so getBaseSubscriptionClient() would return the
disposed client instead of creating a fresh one. This caused live queries
to use a broken WebSocket client after any enableSessionMode() call,
leading to unauthenticated or silently failing live subscriptions.

Also adds test coverage for auth credentials in subscription
connectionParams (API key, browser session, internal, custom, anonymous)
which previously had zero coverage.
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