[relay-runtime] Don't garbage-collect records that an active subscription is still reading#5336
Open
ellemedit wants to merge 1 commit into
Open
[relay-runtime] Don't garbage-collect records that an active subscription is still reading#5336ellemedit wants to merge 1 commit into
ellemedit wants to merge 1 commit into
Conversation
… reading RelayModernStore._collect marks GC reachability only from retained operations (this._roots -> RelayReferenceMarker.mark). It never consults active store subscriptions, so a record co-referenced by two operations — where only one selects a given linked child — loses that child when the selecting op is released, even while a live subscription (e.g. a mounted useFragment) still reads it. The surviving parent then yields a partial read (isMissingData) and unguarded nested access throws. React <Activity> makes this reachable by keeping the consuming subtree mounted while the loader releases the owner query's retention. Fix: in _collect, after marking from retained operations, also mark every record in each active subscription's latest snapshot.seenRecords. seenRecords is the exact set a live read traversed and is refreshed on every overlapping write (_updateSubscription), so this never over-retains stale data; once a subscription is disposed its records become collectible again. Gated to the standard GC path and intentionally skipped under shouldRetainWithinTTL_EXPERIMENTAL: that mode deliberately frees an Activity-hidden subtree's data once its query passes the cache TTL even though the subtree stays mounted (and subscribed), so protecting subscription records there would defeat the time-based release (and regress useLazyLoadQueryNode-activity-test). Adds RelayStoreSubscriptions.markActiveSubscriptionRecords + a test (RelayModernStore-SubscriptionGc-test.js, graphql operations with generated artifacts) that is RED before this change and GREEN after, plus an over-retention guard. Full JS suite (3715 tests), Flow, eslint and prettier pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ee64a5c to
88757a0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RelayModernStore's GC (_collect) computes record reachability only fromretained operations (
this._roots→RelayReferenceMarker.mark); it neverconsults active store subscriptions. So a record co-referenced by two operations —
where only one selects a given linked child — loses that child when the selecting
operation is released, even while a live subscription (e.g. a mounted
useFragment) is still reading it. The surviving parent then yields a partialread (
isMissingData: true);useFragmentInternaldoes not re-suspend a committedfragment that has no in-flight operation, so it returns the partial snapshot and
unguarded nested access throws a
TypeError.React
<Activity>(e.g. Next.jscacheComponents) makes this reachable inpractice: it keeps the consuming subtree mounted (and therefore subscribed) while
the query loader releases the owner query's retention.
Fix
In
_collect, after marking from retained operations, also mark every record ineach active subscription's latest
snapshot.seenRecords— the exact set a live readtraversed, refreshed on every overlapping write (
_updateSubscription). A record alive reader still needs is therefore never collected; once the subscription is
disposed its records become collectible again (no leak).
This is gated to the standard GC path and intentionally skipped under
shouldRetainWithinTTL_EXPERIMENTAL: that mode deliberately frees an<Activity>-hidden subtree's data once its query passes the cache TTL even thoughthe subtree stays mounted (and subscribed), so protecting subscription records there
would defeat the time-based release (and would regress
useLazyLoadQueryNode-activity-test).RelayStoreSubscriptions.js— newmarkActiveSubscriptionRecords(references)RelayStoreTypes.js— declare it on theStoreSubscriptionsinterfaceRelayModernStore.js— call it inside the_collectGC loop (gated)Test Plan
New
RelayModernStore-SubscriptionGc-test.js:operation is released" — fails before this change (the child is collected →
partial read), passes after.
passes both before and after (guards against the fix leaking records).
I confirmed the test is RED→GREEN by reverting only the three source files (the new
test fails), then re-applying (it passes), so it genuinely exercises the change.
The same mechanism is reproduced end-to-end — a minimal Next.js + Relay app that
crashes into an error boundary on stock
relay-runtime@21.0.1and renders cleanlyonce this change is compiled in — at https://github.com/ellemedit/relay-gc-bug.
Notes
graphqlliterals; the committed__generated__artifacts wereproduced by the in-repo compiler (
scripts/compile-tests.shconfig), so theCompiler-output check stays green.
retained operation or an active subscription needs it"), so it's offered as a
proposal — feedback on the approach and the TTL gating is very welcome.