Skip to content

Land @defer chunks that arrive with a subPath (fixes #5354) - #5370

Open
jonreading81 wants to merge 6 commits into
facebook:mainfrom
jonreading81:jr-fix-5354-subpath-drop
Open

Land @defer chunks that arrive with a subPath (fixes #5354)#5370
jonreading81 wants to merge 6 commits into
facebook:mainfrom
jonreading81:jr-fix-5354-subpath-drop

Conversation

@jonreading81

@jonreading81 jonreading81 commented Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown

Summary

Fixes #5371 (supersedes closed #5354). OperationExecutor._processIncrementalResponses looks up placeholders by exact (label, chunk.path.join('.')). Placeholders register at pendingPart.path — the fragment-spread path — with no subPath suffix. Any chunk arriving from a spec-compliant incremental-delivery server that carries a subPath (e.g. graphql-core 3.3+'s per-item non-Node list chunks, or field-dedup chunks addressed at a sub-record) therefore lands in a map bucket no placeholder ever registers at → buffered forever → silently dropped by the client store.

The bug manifests in real Relay clients paired with graphql-core / Strawberry backends. Two documented symptom classes:

  • Non-Node list overlap — a @defer'd fragment and a non-deferred sibling both dig through a non-Node list. graphql-core emits per-item chunks with subPath: [listField, N] (or deeper). All chunks are dropped; the list renders empty despite the count / other scalars being correct.
  • @connection in a fragment-nested spread — same mechanism, different shape. Edge chunks arrive with subPath: [connField, 'edges', N, 'node', ...] and are dropped; header renders "N items", list renders blank.

Fix

Prefix-match placeholder lookup plus shape-specific normalisation:

  • _processIncrementalResponses — when the exact key misses, walk shorter path prefixes until a placeholder is found; compute the delta subPath = path.slice(prefix.length) and hand it to _processDeferResponse.

  • _processDeferResponse (new subPath param):

    • String-only subPath (single sub-record, e.g. ['owner']): fold data under the subPath keys and normalise at the placeholder's root selector. Seed the local normalisation source with a clone of the existing store record (see normalizeResponse change) so _normalizeLink's id-fallback re-uses the store's real linked-record IDs instead of synthesising a client ID that would sever the parent's link on publish.
    • Numeric-in-subPath (non-Node list per-item, connection edge): the new walkDeferSubPath helper walks the fragment's normalization AST alongside the store from parentID, following each subPath key (string = LinkedField, number = index into the previous plural link), to resolve the child dataID + item-level selection + concrete type. Normalise chunk data directly INTO that child record — prevents per-item chunks from clobbering the parent's plural link via setLinkedRecordIDs.

  • normalizeResponse grows an optional existingRootRecord parameter (also added to NormalizeResponseFunction in RelayStoreTypes). When provided, the local source is seeded with a clone of that record instead of a fresh empty one; existing linked-record IDs are then visible to _normalizeLink's local-record fallback. Fully optional and backward-compatible with every existing caller.

Test plan

  • yarn typecheck — clean.
  • yarn test RelayModernEnvironment-ExecuteWithDefer — 78/78 pre-existing defer + deferredStreamedConnection tests pass. Chunks arriving without a subPath still take the exact-key path, unchanged.
  • Verified empirically end-to-end against a graphql-core 3.3+ backend (Strawberry) in a shipping app:
    • String-only subPath shape: @defer on a fragment whose deduplicated data targets a nested Node record (subPath: ['owner'], chunk {isBranded: false}) — the field lands on the existing owner record and the parent's link is preserved.
    • Numeric-in-subPath shape: @defer on a fragment overlapping a non-Node list (per-item chunks with subPath: ['tags', N]) — every item's data lands in its own child record.
    • @connection shape: @defer on a fragment-nested spread that reaches a @connection (spotlight.users(first: 4)) — all 4 edges land with correct usernames + follower counts.

Notes

  • No test additions in this PR yet — happy to add coverage for the three shapes if reviewers prefer; the existing 78 tests already prove backward-compat.
  • The recovery inside _processDeferResponse was intentionally kept in the executor rather than pushed into the normalizer, so the AST + store walk sits alongside the placeholder metadata it already has.

Fixes #5371. (Supersedes closed #5354 — same root cause, unified writeup.)

@meta-cla

meta-cla Bot commented Jul 21, 2026

Copy link
Copy Markdown

Hi @jonreading81!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@jonreading81 jonreading81 mentioned this pull request Jul 21, 2026
Closed
@jonreading81
jonreading81 force-pushed the jr-fix-5354-subpath-drop branch 4 times, most recently from c255d2e to 0082694 Compare July 21, 2026 11:45
@meta-cla meta-cla Bot added the CLA Signed label Jul 21, 2026
@jonreading81
jonreading81 force-pushed the jr-fix-5354-subpath-drop branch 7 times, most recently from dbea995 to c6c9b4a Compare July 21, 2026 12:06
jonreading81 and others added 6 commits July 21, 2026 13:06
@jonreading81
Land @defer chunks that arrive with a subPath (fixes facebook#5354)
c6c9b4a 
`OperationExecutor._processIncrementalResponses` looks up placeholders
by exact `(label, chunk.path.join('.'))`. Placeholders register at
`pendingPart.path` — the fragment-spread path — with no `subPath`
suffix. Any chunk arriving from a spec-compliant incremental-delivery
server that carries a `subPath` (e.g. graphql-core 3.3+'s per-item
non-Node list chunks, or field-dedup chunks addressed at a sub-record)
therefore lands in a map bucket no placeholder ever registers at →
buffered forever → silently dropped by the client store.

Recover with prefix-match placeholder lookup plus shape-specific
normalisation:

  * `_processIncrementalResponses`: when the exact key misses, walk
    shorter path prefixes until a placeholder is found; compute the
    delta `subPath = path.slice(prefix.length)` and hand it to
    `_processDeferResponse`.

  * `_processDeferResponse` (new `subPath` param):
    - String-only subPath (single sub-record, e.g. `['owner']`): fold
      `data` under the subPath keys and normalise at the placeholder's
      root selector. Seed the local normalisation source with a clone
      of the existing store record (see `normalizeResponse` change) so
      `_normalizeLink`'s id-fallback re-uses the store's real linked-
      record IDs instead of synthesising a client ID that would sever
      the parent's link on publish.
    - Numeric-in-subPath (non-Node list per-item, connection edge): the
      new `walkDeferSubPath` helper walks the fragment's normalization
      AST alongside the store from `parentID`, following each subPath
      key (string = LinkedField, number = index into the previous
      plural link), to resolve the child dataID + item-level selection
      + concrete type. Normalise chunk data directly INTO that child
      record — prevents per-item chunks from clobbering the parent's
      plural link via `setLinkedRecordIDs`.

  * `normalizeResponse` grows an optional `existingRootRecord` parameter
    (also added to `NormalizeResponseFunction` in `RelayStoreTypes`).
    When provided, the local source is seeded with a clone of that
    record instead of a fresh empty one; existing linked-record IDs
    are then visible to `_normalizeLink`'s local-record fallback. Fully
    optional and backward-compatible with every existing caller.

Backward-compatible: chunks arriving without a subPath still take the
exact-key path unchanged. All 78 pre-existing defer/deferredStreamedConnection
tests continue to pass.

Fixes: facebook#5354
@jonreading81
Add two tests locking in the @defer subPath fix
e807103 
RelayModernEnvironment-ExecuteWithDeferAndSubPath-test.js
  + `processes deferred sub-record payloads addressed via numeric+string
    subPath` — exercises the mixed walk: parent selects
    `allPhones { phoneNumber { displayNumber } }` and a `@defer`'d
    fragment selects `allPhones { phoneNumber { countryCode } }`.
    graphql-core dedups the shared sub-selections, so per-item chunks
    arrive with `path: ['node', 'allPhones', <i>, 'phoneNumber']` and
    just `{countryCode}`. `_walkDeferSubPath` steps into the plural
    field, then the numeric index, then the non-plural sub-record, and
    the chunk normalises into that PhoneNumber record. Reader returns
    the merged data on every item.

RelayModernEnvironment-ExecuteWithDeferInFragmentAndConnection-test.js
  + `populates the connection edges from the deferred payload` — the
    fragment-nested @defer shape: a wrapper fragment on User contains
    `...ConnectionFragment @defer`, and ConnectionFragment selects
    `friends(first: 2) @connection(...)`. Historically the deferred
    chunk's `edges` came through empty even though it arrived with two
    nodes; the connection handler wasn't seeing the edges list on
    hydration. This test asserts that with the subPath fix in place,
    the deferred chunk's edges land in the store and the reader returns
    the assembled connection (edges + cursor + pageInfo).

Both tests run against RelayModernEnvironment and MultiActorEnvironment
via `describe.each`. 8 total cases, all passing.
@jonreading81
Strip explanatory comments from the new @defer tests
b804a8a
@jonreading81 @claude
Register nested @defer placeholders eagerly during subPath chunk proc…
6f0256a 
…essing

When a chunk arrives with a `subPath` and lands via prefix-match on a
parent @defer's placeholder, `_processDeferResponse` normalizes only
the addressed sub-selection (a single sub-record, or a per-item slice
of a non-Node list). The parent fragment's top-level selections are
never walked, so any nested `Defer` AST node inside that fragment is
never encountered by the normalizer — its label never enters
`_incrementalResults`. Chunks for the nested defer then arrive with
an unregistered label, get buffered as pending responses, and are
silently dropped.

Reproduces on any query shape where an outer @defer'd fragment
selects a non-Node list AND declares an inner @defer, e.g.:

    query Q($id: ID!) {
      node(id: $id) {
        ... on User {
          allPhones { phoneNumber { displayNumber } }
          ...Outer @defer(label: "Outer")
        }
      }
    }
    fragment Outer on User {
      allPhones { isVerified }
      ...Inner @defer(label: "Inner")
    }

The `Outer` chunk streams as per-item sub-path chunks (Bug B path)
and never as a root chunk; the `Inner` chunk arrives at
`['node']` with `label: 'Q$defer$OuterFragment$defer$Inner'` and no
placeholder exists for it.

Fix: after successfully looking up (either exact-match or via
prefix-match) a parent @defer's placeholder in
`_processIncrementalResponses`, walk the parent's fragment
selections for `Defer` AST nodes and register their placeholders
eagerly, mirroring what `_normalizeDefer` would do if the fragment
root were normalized. Idempotent (skips labels already registered),
recurses so multi-level nesting is handled, and drains any queued
response chunks that arrived before the placeholder existed.

Extracted helpers, module-level:

  * `forEachInnerDefer(selector, visit)` — walks the top-level
    selections of the placeholder's fragment for `Defer` nodes,
    descending through wrapper selections that don't change the
    record scope (`InlineFragment`, `ClientExtension`, `Condition`).
    Conditional defers are honored: `@defer(if: $var)` is skipped
    when the variable resolves falsy at the placeholder's
    variables, matching `_normalizeDefer`.

  * `buildInnerDeferPlaceholder(parent, defer)` — constructs a
    `DeferPlaceholder` from a parent placeholder + a child `Defer`
    AST node.

RelayModernEnvironment-ExecuteWithDeferAndSubPath-test.js
  + `processes a nested @defer chunk when the outer defer streams
    as sub-path chunks only` — outer @defer fragment streams only
    per-item chunks (['node', 'allPhones', 0]), then an inner
    @defer's chunk arrives at ['node']. Asserts the reader
    returns the inner defer's data. Runs against both
    `RelayModernEnvironment` and `MultiActorEnvironment` via
    `describe.each`. Verified to fail without the fix.

Fixes: nested @defer under a subPath-streamed parent (companion
to facebook#5354).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jonreading81
Also register inner @defer placeholders when parent placeholder is cr…
1c233b6 
…eated

The previous fix (6f0256a) registered inner defer placeholders
when a chunk arrived matching the parent placeholder. That covers the
case where the parent streams as sub-path chunks (per-item, single
sub-record) so the parent root is never normalized.

It misses another case: when the parent @defer selects only inner
@defer fragments (no scalar/list fields of its own), the server never
emits a chunk for the parent label at all — only chunks for its
inner defers. `_registerInnerDeferPlaceholders` never fires, so the
inner chunks arrive with no registered placeholder and get queued
forever in `_incrementalResults`.

Fix: also call `_registerInnerDeferPlaceholders(placeholder)` from
`_processIncrementalPlaceholder` when a defer placeholder is created
from the initial payload's `incrementalPlaceholders`. Idempotent
registration keeps the existing chunk-arrival call safe.
@jonreading81
Add test locking in the empty-outer-defer case
7528fb6 
Sibling to the existing 'outer defer streams as sub-path chunks only'
test — same fragment topology, but the server emits NO chunk at all
for the outer label. Mirrors the production shape where the outer
fragment's non-defer selections resolve to null/empty and the server
skips its chunk.

Verified fails on parent commit (invariant thrown when inner chunk
finds no placeholder), passes with the fix in place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

CLA Signed

Projects

None yet

Milestone

No milestone

1 participant

@jonreading81