fix(normalizr): do not mutate cached journey on result-cache hit#3924
Open
fix(normalizr): do not mutate cached journey on result-cache hit#3924
Conversation
`GlobalCache.getResults` called `paths.shift()` on a cache hit, mutating the `journey` array stored by reference on the `WeakDependencyMap` `Link` node. After the first hit stripped the placeholder input slot, every subsequent hit on the same cached entry would shift off a real `EntityPath`, progressively losing subscription entries. This corrupts downstream consumers that capture `paths` by reference: - `Controller.getResponseMeta` passes `paths` into `GCPolicy.createCountRef`, whose increment/decrement closures iterate the captured array at effect time. After three React subscribers to the same endpoint+args (miss + 2 hits), the journey has been shifted twice — so subscribers 2 and 3 silently skip the first entity, leaving `entityCount` under-counted and making GC reap entities still in use. - `entityExpiresAt(paths, ...)` returns an incorrect (too-late) expiry when paths drop entries, suppressing auto-refetch. The hit path now returns a non-mutating copy (`paths.slice(1)`).
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
🦋 Changeset detectedLatest commit: 95e758f The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Size Change: 0 B Total Size: 80.7 kB ℹ️ View Unchanged
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Benchmark React
Details
| Benchmark suite | Current: 95e758f | Previous: ce99359 | Ratio |
|---|---|---|---|
data-client: getlist-100 |
133.33 ops/s (± 5.3%) |
136.99 ops/s (± 5.1%) |
1.03 |
data-client: getlist-500 |
39.92 ops/s (± 5.4%) |
41.85 ops/s (± 4.7%) |
1.05 |
data-client: update-entity |
357.14 ops/s (± 10.1%) |
333.33 ops/s (± 9.8%) |
0.93 |
data-client: update-user |
317.54 ops/s (± 7.2%) |
344.83 ops/s (± 7.8%) |
1.09 |
data-client: getlist-500-sorted |
36.83 ops/s (± 7.5%) |
46.08 ops/s (± 6.6%) |
1.25 |
data-client: update-entity-sorted |
277.78 ops/s (± 6.9%) |
294.12 ops/s (± 7.4%) |
1.06 |
data-client: update-entity-multi-view |
322.58 ops/s (± 6.0%) |
317.54 ops/s (± 8.1%) |
0.98 |
data-client: list-detail-switch-10 |
6.98 ops/s (± 5.0%) |
7.48 ops/s (± 6.1%) |
1.07 |
data-client: update-user-10000 |
66.45 ops/s (± 15.0%) |
74.91 ops/s (± 14.1%) |
1.13 |
data-client: invalidate-and-resolve |
34.97 ops/s (± 5.1%) |
36.1 ops/s (± 5.2%) |
1.03 |
data-client: unshift-item |
212.77 ops/s (± 2.8%) |
238.1 ops/s (± 2.1%) |
1.12 |
data-client: delete-item |
285.71 ops/s (± 4.1%) |
270.27 ops/s (± 7.0%) |
0.95 |
data-client: move-item |
175.44 ops/s (± 5.3%) |
181.82 ops/s (± 5.0%) |
1.04 |
This comment was automatically generated by workflow using github-action-benchmark.
Contributor
There was a problem hiding this comment.
Benchmark
Details
| Benchmark suite | Current: 95e758f | Previous: 4f5b6b6 | Ratio |
|---|---|---|---|
normalizeLong |
466 ops/sec (±1.87%) |
458 ops/sec (±1.29%) |
0.98 |
normalizeLong Values |
423 ops/sec (±0.22%) |
417 ops/sec (±0.37%) |
0.99 |
denormalizeLong |
243 ops/sec (±4.30%) |
394 ops/sec (±1.17%) |
1.62 |
denormalizeLong Values |
221 ops/sec (±3.99%) |
339 ops/sec (±0.50%) |
1.53 |
denormalizeLong donotcache |
1123 ops/sec (±0.28%) |
1015 ops/sec (±0.10%) |
0.90 |
denormalizeLong Values donotcache |
770 ops/sec (±1.12%) |
758 ops/sec (±0.23%) |
0.98 |
denormalizeShort donotcache 500x |
1494 ops/sec (±0.13%) |
1558 ops/sec (±0.14%) |
1.04 |
denormalizeShort 500x |
666 ops/sec (±3.51%) |
1060 ops/sec (±2.16%) |
1.59 |
denormalizeShort 500x withCache |
7813 ops/sec (±0.19%) |
7687 ops/sec (±0.11%) |
0.98 |
queryShort 500x withCache |
3292 ops/sec (±0.16%) |
2999 ops/sec (±0.11%) |
0.91 |
buildQueryKey All |
49080 ops/sec (±0.36%) |
54049 ops/sec (±0.42%) |
1.10 |
query All withCache |
5538 ops/sec (±0.24%) |
6752 ops/sec (±0.32%) |
1.22 |
denormalizeLong with mixin Entity |
230 ops/sec (±4.39%) |
357 ops/sec (±2.60%) |
1.55 |
denormalizeLong withCache |
8783 ops/sec (±0.31%) |
7722 ops/sec (±0.45%) |
0.88 |
denormalizeLong Values withCache |
5637 ops/sec (±4.53%) |
5215 ops/sec (±0.19%) |
0.93 |
denormalizeLong All withCache |
6213 ops/sec (±0.72%) |
6457 ops/sec (±0.11%) |
1.04 |
denormalizeLong Query-sorted withCache |
5116 ops/sec (±0.83%) |
6764 ops/sec (±0.21%) |
1.32 |
denormalizeLongAndShort withEntityCacheOnly |
1792 ops/sec (±0.22%) |
1837 ops/sec (±0.18%) |
1.03 |
denormalize bidirectional 50 |
4690 ops/sec (±4.65%) |
6952 ops/sec (±0.28%) |
1.48 |
denormalize bidirectional 50 donotcache |
43222 ops/sec (±1.79%) |
40359 ops/sec (±1.32%) |
0.93 |
getResponse |
5998 ops/sec (±0.77%) |
4714 ops/sec (±0.69%) |
0.79 |
getResponse (null) |
10651347 ops/sec (±0.69%) |
10852221 ops/sec (±0.75%) |
1.02 |
getResponse (clear cache) |
224 ops/sec (±3.78%) |
338 ops/sec (±0.33%) |
1.51 |
getSmallResponse |
3893 ops/sec (±0.97%) |
3511 ops/sec (±0.09%) |
0.90 |
getSmallInferredResponse |
2888 ops/sec (±0.96%) |
2617 ops/sec (±0.11%) |
0.91 |
getResponse Collection |
5391 ops/sec (±0.64%) |
4708 ops/sec (±0.82%) |
0.87 |
get Collection |
4889 ops/sec (±0.39%) |
4732 ops/sec (±0.47%) |
0.97 |
get Query-sorted |
6084 ops/sec (±2.92%) |
4672 ops/sec (±0.27%) |
0.77 |
setLong |
474 ops/sec (±0.32%) |
469 ops/sec (±0.13%) |
0.99 |
setLongWithMerge |
265 ops/sec (±0.84%) |
255 ops/sec (±0.21%) |
0.96 |
setLongWithSimpleMerge |
277 ops/sec (±1.28%) |
277 ops/sec (±0.12%) |
1 |
setSmallResponse 500x |
914 ops/sec (±0.11%) |
918 ops/sec (±0.74%) |
1.00 |
This comment was automatically generated by workflow using github-action-benchmark.
3 tasks
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
1 participant
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
GlobalCache.getResultscalledpaths.shift()on a result-cache hit, mutating thejourneyarray stored by reference on theWeakDependencyMapLinknode. After the first hit stripped the placeholder input slot, every subsequent hit on the same cached entry would shift off a realEntityPath, progressively losing subscription entries.The hit path now returns a non-mutating copy (
paths.slice(1)).} else { - paths.shift(); + paths = paths.slice(1); }Why it matters
Downstream consumers capture
pathsby reference:Controller.getResponseMetapassespathsintoGCPolicy.createCountRef, whose increment/decrement closures iterate the captured array at effect time. After three React subscribers to the same endpoint + args with entities unchanged (1 miss + 2 hits), the journey has been shifted twice — subscribers 2 and 3 silently skip the first entity.entityCountis under-counted and GC can reap entities still referenced by mounted components.entityExpiresAt(paths, ...)returns an incorrect (too-late) expiry when paths drop entries, suppressing auto-refetch.useSuspense,useCache,useLive,useDLE,useQuery) all funnel throughgetResponseMeta, so any app rendering the same endpoint from multiple places can hit this.Empirical confirmation
On master, a tight loop of result-cache hits against the same primed
MemoCachedrains the journey to zero:Test plan
packages/core/src/controller/__tests__/getResponseMeta-countRef.tssimulates three React subscribers to the same endpoint + args, asserts all threecountRefclosures correctly incrementGCPolicy.entityCountfor every entity, and that decrement restores counts to zero. Fails on master (under-counts first entity); passes with fix.yarn jest --selectProjects ReactDOM --testPathPatterns "packages/(core|normalizr)"— 259 tests pass.Performance analysis
Run on Linux 6.6.87.2 (WSL2), AMD Ryzen 9 7950X, Node v20.18.2,
example-benchmarkwithNODE_ENV=productionand--expose_gc.Methodological caveat
Master's
withCachebenchmark numbers are not directly comparable to the fix's numbers. The bug makes eachpaths.shift()mutate the shared journey on every hit, so afterbenchmark.js's ~100k-iteration warmup the journey is fully drained and master enters a steady state where each hit returns an empty paths array (doing nearly no work). The fix does honest work on every hit (slice of a full-length journey).The numbers below are reported as-measured. Interpret
withCacherows with that bias in mind — a small slowdown vs. a journey-corrupting baseline is expected.normalizrsuite† withCache scenarios — master is biased low-work by journey drain. See caveat.
‡ Scenarios not touching the hit path. Deltas here are run-to-run noise (these rows do not call
getResultson a hit, so the fix cannot affect them). Re-running multiple times produced swings of similar magnitude, including >10% swings on theValues withCacherow (9,190 → 11,078 on repeated master runs).coresuite (does not exercise the hit path in the changed branch; all rows are noise-level)Core suite is mostly within run-to-run noise (±3–5%).
getResponse (clear cache)has known wide variance; the -29% is noise, not a fix-related regression.Controlled microbenchmark (drain-free)
To isolate the per-hit cost without the benchmark.js journey-drain artifact, I ran three scenarios with fresh
MemoCacheper outer iteration (so the bug can't bias toward "shift on empty array"):The last row — the only scenario where both branches do equal work — shows the fix is actually faster.
Array.prototype.shifton a 2800-element array is O(n) reindex-in-place, which V8 routinely hits slow paths for;Array.prototype.slice(1)is O(n) allocate + memcpy which V8 optimizes well. Once we stop the bug from letting master do less work, the fix wins.Real-world impact
A React app does not iterate one endpoint 10,000 times in a tight loop — it does
O(hook instances × re-renders)hits per second. At typical hook counts (tens to low hundreds), one extraslice(1)per hit on a ~50-element paths array is sub-microsecond per render. The correctness benefits (no silent under-subscription, no premature GC of live entities, correctentityExpiresAt) are unambiguously worth it.Changeset
Included as
.changeset/fix-journey-mutation.md. Version-linked packages:normalizr,core,endpoint,rest,graphql,react.Note
Medium Risk
Touches core caching/subscription metadata used by GC ref-counting; behavior change is small but impacts correctness across repeated cache hits and could affect dependency tracking if wrong.
Overview
Fixes a bug where
GlobalCache.getResultsmutated cached dependency "journey" data on result-cache hits by replacingpaths.shift()with a non-mutatingpaths.slice(1)copy, preventing progressive loss of entity subscription paths.Adds an integration test covering repeated
Controller.getResponseMeta()calls to ensurecountRefincrements/decrements entity ref-counts correctly across multiple subscribers, and includes a changeset bumping several@data-client/*packages.Reviewed by Cursor Bugbot for commit 95e758f. Bugbot is set up for automated code reviews on this repo. Configure here.