Skip to content

Recover missing-data fragment reads by refetching the owner once (ENABLE_MISSING_DATA_OWNER_REFETCH) - #5367

Open
ellemedit wants to merge 1 commit into
facebook:mainfrom
ellemedit:fix/missing-data-owner-refetch
Open

Recover missing-data fragment reads by refetching the owner once (ENABLE_MISSING_DATA_OWNER_REFETCH)#5367
ellemedit wants to merge 1 commit into
facebook:mainfrom
ellemedit:fix/missing-data-owner-refetch

Conversation

@ellemedit

Copy link
Copy Markdown

Summary

Fixes the crash described in #5366: a fragment whose records were
garbage-collected while unobserved (hidden <Activity> route, fragment ref
held in React state after the owner query's retain lapsed) re-reads missing
data with no pending operation
, and useFragmentInternal_EXPERIMENTAL
renders the partial snapshot — undefined for fields the fragment explicitly
fetched — crashing consumers that trust the schema types.

This PR adds an opt-in recovery path behind a new feature flag,
ENABLE_MISSING_DATA_OWNER_REFETCH (default off):

  • When isMissingData(state) and getPendingOperationsForFragment finds no
    in-flight operation, refetch the fragment's owner query once and suspend
    on the request instead of rendering the partial snapshot.
  • The refetch uses {...owner.cacheConfig, force: true} with fetch policy
    'network-only' and a unique QueryResource cache breaker, so a completed
    QueryResource entry or an app-level response cache cannot short-circuit it
    into re-reading the same collected records.
  • Query owners only. A fragment owned by a mutation or subscription never
    re-executes its owner (re-running a mutation as a side effect of a render
    would be unacceptable); it falls through to today's partial-render behavior.
  • Once per owner per environment. A WeakMap<IEnvironment, Set<ownerID>>
    (FIFO-capped at 1000) marks owners that have already been refetched. If the
    refetch response is still partial, or the transport fails, the marker holds
    and execution falls through to today's behavior — one extra request, never a
    request loop.
  • The marker clears once the owner query reads back without missing data
    (environment.check(owner).status !== 'missing'). Clearing on 'stale'
    matters: data fully present but invalidated (e.g. invalidateStore() at an
    auth boundary) is loop-safe to clear, and not clearing would leave the
    owner permanently exempt from recovery in later GC episodes. Clearing on
    'missing' would loop against a server that keeps returning partial data —
    that is the one status that must keep the marker.
  • Retention of the refetched payload: when the owner query is still mounted
    (the <Activity> route case) its own retain keeps the data durably; when
    only the fragment ref survived, the data is held by the internal
    prepare() call's temporary retain and becomes GC-eligible again once that
    TTL lapses. That is by design — the marker clears on a data-complete read,
    so a later GC episode recovers again with one more request rather than
    staying broken (covered by the second-episode test).

Why the recovery cannot be limited to committed fragments: <Activity> may
either preserve a hidden hook or remount it on restore, and a remounted hook
is indistinguishable from a genuine first mount at this API surface. The
recovery therefore applies to any query-owned missing read with no pending
operation; the once-per-owner guard keeps the worst case at a single extra
request.

Reproduction

https://github.com/ellemedit/relay-gc-partial-read-repro (main = stock 21.0.1
crashes; with-fix = this change applied, recovers). The repro triggers real
GC via retain().dispose() — no store internals are touched — and shows the
dangling-link shape: a keep-alive query keeps the parent record alive while
the child record, reachable only through the released owner query, is
collected.

Test plan

New test file packages/react-relay/relay-hooks/__tests__/useFragment-missing-data-owner-refetch-test.js:

  • refetches the owner once and suspends when a subscribed fragment re-reads
    missing data after GC
  • recovers a fragment restored by <Activity> (hidden → GC → visible)
  • recovers a remounted fragment whose ref outlived the owner
  • refetches only once: a transport error does not start a request loop
  • the marker clears once data reads back complete: a second GC episode
    recovers again
  • a store-wide invalidation ('stale') clears the marker instead of permanently
    disarming recovery
  • never re-executes a non-query owner
  • does not change behavior when the flag is off

All tests trigger real GC (gcReleaseBufferSize: 0, synchronous scheduler,
real retain().dispose()).

yarn jest packages/react-relay/relay-hooks/__tests__/useFragment-missing-data-owner-refetch-test.js
yarn jest packages/react-relay/relay-hooks   # no regressions

Production validation

This recovery has been running in production (as a pnpm patch on react-relay
21.0.1) in a mobile webview app with an <Activity>-based Next.js router,
where it eliminated a recurring class of route-restore crashes
(undefined.__typename style) that Sentry attributed to fragments re-reading
GC'd records with no owner request in flight.

When a fragment reads missing data with no pending operation for its
owner — its records were garbage-collected while a React <Activity> was
hidden, or a fragment ref outlived the owner query's retain — the
EXPERIMENTAL useFragment implementation renders the partial snapshot:
fields the fragment explicitly fetched come back undefined, crashing
consumers that trust the schema types. There is no recovery path.

Behind a new feature flag ENABLE_MISSING_DATA_OWNER_REFETCH (default
off), refetch the fragment's owner query once per owner per environment
and suspend on the request instead. force:true plus a unique
QueryResource cache breaker guarantee a real network request even when
a completed QueryResource entry or an app response cache would
short-circuit; only query owners re-execute; a FIFO-capped WeakMap
marker prevents request loops when the refetch response is still
partial or the transport fails, and clears once the owner query reads
back without missing data.

All new tests trigger real GC (retain().dispose() with
gcReleaseBufferSize 0) — no store internals are stubbed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant