Fix re-retained operations being swept during TTL GC by ellemedit · Pull Request #5413 · facebook/relay · GitHub
Skip to content

Fix re-retained operations being swept during TTL GC - #5413

Open
ellemedit wants to merge 1 commit into
facebook:mainfrom
ellemedit:fix/ttl-gc-retain-20260907
Open

ellemedit wants to merge 1 commit into
facebook:mainfrom
ellemedit:fix/ttl-gc-retain-20260907

Conversation

@ellemedit

Copy link
Copy Markdown

With shouldRetainWithinTTL_EXPERIMENTAL enabled, an incremental GC pass can sweep an operation's records after the operation has been retained again.

A screen can retain a query for use again, yet a previously complete read can become isMissingData solely because of GC scheduling. Losing records while the caller correctly holds a retain can force the application to recover data it already had. This fix includes operations retained again while GC was yielding in the set of records to preserve, preventing their collection despite an active retain.

Keeping data in memory and treating it as fresh are separate decisions. The change preserves the existing stale status so required revalidation is not skipped, while still allowing memory to be reclaimed after the final retain is released.

The failing sequence is:

  1. GC skips expired, unretained operation A, which is outside the release buffer.
  2. GC marks retained operation B and yields.
  3. retain(A) increments the existing root entry without a store write.
  4. The roots iterator has already passed A, so GC sweeps its records despite the active retain.

Reinsert the existing root entry when its reference count changes from zero during an active TTL GC pass. The live Map iterator then visits it again before sweeping. Reusing the entry preserves its fetch time and invalidation epoch, and retention does not advance the store's write epoch.

The seven regression tests use public retain/dispose calls and a controllable GC scheduler. They cover both a zero-sized release buffer and actual eviction from the default ten-entry buffer, stale disposal that bypasses the buffer, TTL and release-buffer controls, freshness preservation, and collection after the final release.

Validation:

  • Base 52c26aed65a496182f2fa978f9911db5be4f652d with the new tests: four regressions fail and three controls pass.
  • With this change, the new tests and existing RelayModernStore tests pass: 331 tests across two suites.
  • Full Jest on this branch: 246 suites and 3,783 tests pass; 20 tests are skipped, and all 164 snapshots pass.
  • flow full-check --max-workers 2 --show-all-errors: zero errors.
  • Prettier and ESLint pass for both changed files.

This change preserves the existing root-metadata cleanup policy; that policy can be reviewed separately from the retained-data race.

@meta-cla meta-cla Bot added the CLA Signed label Sep 7, 2026

This branch has not been deployed

No deployments
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