firebase / firebase/firebase-js-sdk

DataConnect: subscribe() silently drops the second observer when two QueryRefs pass equal variables in a different property order

Open
#10,332 1 comment 0 reactions 1 assignee Claimed by @stephenarosaj View on GitHub
api: dataconnect needs-attention Repro Needed stack:NextJS
Dominant language
TypeScript
Stars
5.1k
Forks
1k
Avg merge
2d 21h
Merged PRs (30d)
37

Description

### Operating System

Windows 11 Pro (26200). The defect is in shared SDK code, so it is not platform-specific.

### Environment (if applicable)

Google Chrome (browser bundle); Node.js v22.22.0 for tooling

### Firebase SDK Version

`firebase@12.18.0` / `@firebase/data-connect@0.7.4` (both latest at time of writing). Also present on `master`.

### Firebase SDK Product(s)

DataConnect

### Project Tooling

Next.js 16 + React 19 + TypeScript, pnpm monorepo, generated Data Connect JS SDK (`javascriptSdk` with `clientCache: { maxAge: 5s, storage: memory }`), realtime `subscribe()` with `@refresh(onMutationExecuted: …)` directives.

### Detailed Problem Description

Two `QueryRef`s for the **same query with the same variable values but a different property order** are treated as *two* subscriptions by `QueryManager` and as *one* by the stream transport. `invokeSubscribe` finds the existing request and returns **without registering the new observer**. The second subscriber receives its initial (cache-served) result and then never receives another update — no `onError`, no log, no thrown error. The surface simply renders stale data until a full page reload.

**Where the three layers disagree**

| layer | keys by | verdict |
| --- | --- | --- |
| `QueryManager.addSubscription` | `encoderImpl({ name, variables, refType })` | **TWO** callback lists → `invokeSubscribe` called twice |
| `AbstractDataConnectStreamTransport.getMapKey` | `JSON.stringify({ operationName, variables: sortObjectKeys(variables) })` | **ONE** map key |
| `invokeSubscribe` | the above map key | existing request found → new observer **discarded** |

1. [`util/encoder.ts`](https://github.com/firebase/firebase-js-sdk/blob/master/packages/data-connect/src/util/encoder.ts) — the default encoder is `JSON.stringify(sortKeysForObj(o))`, and `sortKeysForObj` is **shallow**. It sorts the envelope (`name` / `variables` / `refType`) but leaves the `variables` object untouched, so variable property order survives into the key.
2. [`network/stream/streamTransport.ts`](https://github.com/firebase/firebase-js-sdk/blob/master/packages/data-connect/src/network/stream/streamTransport.ts) — `getMapKey` calls `sortObjectKeys(variables)`, which **is** recursive. The two layers therefore disagree for any property-order difference at any depth.
3. In `invokeSubscribe`, when `existingSubscribe` is truthy and `pendingCancellations` does not hold its `requestId`, the `if` body does nothing and the `observer` argument is dropped on the floor. `subscribeObservers` is `Map` — one observer per request — so it cannot represent two subscribers to one map key even if the branch were fixed in isolation.

**Second consequence, arguably worse than the first**

`addSubscription`'s `unsubscribe` calls `transport.invokeUnsubscribe(queryRef.name, queryRef.variables)`, which resolves through the same shared `getMapKey`. So when the *deaf* subscriber unmounts, it cancels the transport subscription the *live* subscriber depends on — while the live subscriber's `callbacks` list is still populated and `QueryManager` believes it is still subscribed. The first subscriber then goes silently deaf as well. (Derived from reading `master`; the primary symptom below is what we measured.)

**Why this is easy to hit unintentionally**

In our app a breadcrumb-label hook resolves a record with `{ recordId, projectId, orgId }` while the record page reads the same record with `{ orgId, projectId, recordId }`. The hook mounts above the page, so the *page's* subscription is the one whose observer is dropped. Every record page rendered stale after any mutation until a hard reload, with `@refresh` correct and firing the whole time.

It was expensive to diagnose precisely because the data does arrive: logging `result.source` on the surviving subscription showed `SERVER` carrying the new value in the same second the mutation committed, while the other subscriber went on emitting the old one. Everything that could be checked — directive present, WebSocket `readyState === 1`, `hasActiveSubscriptions === true`, no console errors — looked healthy.

**Provenance**

`0.7.0` — *"Hardened the Firebase SQL Connect streaming transport with intelligent reconnection, **query de-duplication**, and resume optimizations."* That de-duplication is the `getMapKey` path above. `0.7.1`–`0.7.4` touch idle timeout, backoff, WebSocket URL and App Check, none of which affect this.

**Expected behaviour**

Both subscribers receive every update. Whether the SDK opens one stream request or two is an implementation detail; dropping a caller's observer silently is not an acceptable outcome of de-duplication.

**Possible fixes** (in preference order, as an outside observer)

1. Make the `QueryManager` encoder sort `variables` recursively as well, so both layers agree and `addSubscription` de-duplicates at its own layer — the second `subscribe()` then joins the existing callback list and never reaches the transport.
2. And/or, in the transport: hold a *set* of observers per `requestId`, register the new observer on the `existingSubscribe` branch, and only send `invokeUnsubscribe` when the last observer for that map key goes away.

**Workaround for anyone else hitting this**

Key your own subscription store by a canonical (recursively key-sorted) serialisation of `variables`, so only one `subscribe()` per logical subscription ever reaches the SDK. Make sure the canonicalisation is at least as coarse as `getMapKey`, and take care not to rebuild non-plain objects — rebuilding drops prototype methods, so a naive `typeof x === 'object'` canonicaliser turns every `Date` into `{}` and merges subscriptions that genuinely differ.

### Steps and code to reproduce issue

No codegen required — `queryRef` is public API. Substitute any query in your connector that a mutation can change, and any two variables.

```js
import { initializeApp } from 'firebase/app';
import { getDataConnect, queryRef, subscribe } from 'firebase/data-connect';

const app = initializeApp({ /* your config */ });
const dc = getDataConnect(app, {
connector: 'default',
service: 'my-service',
location: 'us-central1',
});

// Same query. Same variable VALUES. Different property ORDER.
const refA = queryRef(dc, 'GetMovieById', { id: MOVIE_ID, language: 'en' });
const refB = queryRef(dc, 'GetMovieById', { language: 'en', id: MOVIE_ID });

subscribe(refA, (r) => console.log('A', r.source, r.data));
subscribe(refB, (r) => console.log('B', r.source, r.data));

// Now change that row — a client mutation, the Admin SDK, or the Firebase console.
// The query needs a refresh path: a by-id lookup (implicit) or an explicit
// @refresh(onMutationExecuted: { operation: "…" }) directive.
```

With the generated SDK it is the same two calls:

```js
subscribe(getMovieByIdRef({ id, language }), onA);
subscribe(getMovieByIdRef({ language, id }), onB);
```

**Actual:** `A` logs the update. `B` logs its initial result and nothing ever again.

**Also reproduces the second consequence:** unsubscribe `B` (or unmount the component holding it) and `A` stops receiving updates too, with no error.

**Control:** give both refs the *same* property order and both subscribers receive every update — `addSubscription` then produces one key, pushes both callbacks onto one list, and calls `invokeSubscribe` once.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.