fix: bound the internal url and Range header caches - #2405
Conversation
Both memoized caches are module-level and were never evicted, so a
long-running server kept every key it had ever seen. Their keys come from
request data -- a url, and the Range header, which the client chooses --
so the key space is not bounded by anything the server controls, and
being module-level they outlived the middleware instance.
Measured before and after, with a fixture project:
50,000 distinct urls 9.24 MB retained -> 0.37 MB
(9.18 MB of it survived instance.close())
20,000 Range headers 4.70 MB retained -> bounded; the remainder is
fixed http overhead and does not grow with N
memorize() now keeps a fixed number of entries and drops the least
recently used one, re-inserting on a hit so map insertion order tracks
recency.
Also drops a spread that copied nothing: `{ ...urlObject }` on a URL
yields an empty object, since a URL's properties are prototype accessors,
so the value was `{ pathname }` while being annotated `@type {URL}`.
Reading `.search` off it would have type-checked and been undefined at
runtime. Only the pathname was ever used, so it is returned on its own.
🦋 Changeset detectedLatest commit: 01914f3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. WalkthroughThe PR adds a default cache limit of 1000 entries to Merge Risk: ⚪ Minimal · up to This change bounds memoization caches with LRU eviction and updates pathname handling. No current merge-blocking risk is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 9905928d-fbc4-481f-bdec-10f9026e068b
📒 Files selected for processing (4)
.changeset/bound-memoize-caches.mdsrc/middleware.jssrc/utils.jstest/utils/memorize.test.js
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2405 +/- ##
==========================================
+ Coverage 96.98% 97.04% +0.06%
==========================================
Files 12 12
Lines 1656 1660 +4
==========================================
+ Hits 1606 1611 +5
+ Misses 50 49 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
memorize()backs two module-level caches that were never evicted:memoizedParse, keyed by request url, andparseRangeHeaders, keyed bysizeplus theRangeheader. Their keys come from request data — and in the second case from a header the client chooses — so the key space is not bounded by anything the server controls. Being module-level, they also outlived the middleware instance.Measured against a fixture project, before and after:
instance.close())RangeheadersThe cache now keeps a fixed number of entries and drops the least recently used one, re-inserting on a hit so map insertion order tracks recency.
It also drops a spread that copied nothing:
{ ...urlObject }on aURLyields an empty object, because aURL's properties are prototype accessors. The value was therefore{ pathname }while being annotated@type {URL}— reading.searchoff it would have type-checked and beenundefinedat runtime. Only the pathname was ever used, so it is returned on its own.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes —
test/utils/memorize.test.jscovers the hit path, the callback, that the cache stops growing, and that the evicted key is the least recently used one.Does this PR introduce a breaking change?
No.
memorize()is internal, and the cache limit is an option with a default.If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
AI was used. Claude Code found the caches while reviewing for memory issues, measured the retention before and after with
--expose-gc, wrote the fix and its tests, and drafted this description. Every number above comes from running those measurements.🤖 Generated with Claude Code
https://claude.ai/code/session_01KUpsHWHZG2FxHzJxRUvVv3
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Performance
Documentation