Repository navigation
cacheable - fix: size each key value in deleteMany stats - #1742
Conversation
Before, deleteMany with stats sized the full array of values once for each key. The time was cubic in the input size, and it blocked the event loop. It also changed count, ksize, and deletes for keys that were not in the cache. Now deleteMany sizes only the value of each key and skips missing keys. Stats.roughSizeOfObject uses a Set for visited objects, so each call is linear. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQyS4TFa7JFA5PEcKuLj4T
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1742 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 29 29
Lines 3612 3615 +3
Branches 809 809
=========================================
+ Hits 3612 3615 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33af2ab3e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (statResult === undefined) { | ||
| continue; |
There was a problem hiding this comment.
When the primary copy expires before the secondary copy, has(key) still reports that the key exists. This check skips the key, although deleteMany removes the secondary copy, so all four storage statistics remain stale. Check the secondary store before the code classifies the key as absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I did not change this in this PR. These are the reasons:
deleteInternal(packages/cacheable/src/index.ts:1098) also reads only the primary store. This PR keepsdeleteanddeleteManythe same.- The stats do not decrease when a primary entry expires or when the LRU removes it. So the stats do not follow the secondary store in those paths either.
- A secondary read adds a remote call to each
deleteManywhen stats are on. - A shared secondary store can hold keys that a different instance set. This instance did not count those keys, but the check would decrease the stats for them.
A stats model for two layers is a different issue from #1738. I leave this thread open for the maintainer.
Generated by Claude Code
Before, deleteMany read values with get(). A cached undefined value and a missing key both gave undefined, so deleteMany did not change the stats for a cached undefined value. Now deleteMany reads the raw entries with getManyRaw() and sizes the value of each entry. Also split the code comment into two instructions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DQyS4TFa7JFA5PEcKuLj4T
2 participants
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix. Fixes #1738.
Problem
When stats are on,
Cacheable.deleteMany(keys)had two defects:Stats.roughSizeOfObjectusedArray.prototype.includesfor visited objects, so each call was O(n²). Together, the call was approximately cubic and blocked the event loop.count,ksize,vsize, anddeletesfor keys that were not in the cache.Change
packages/cacheable/src/index.ts:deleteManyInternalreads the raw entries withgetManyRaw(). It sizes only the value of each entry. It skips keys that are not in the cache. The raw entries let it count a cachedundefinedvalue, whichget()cannot tell apart from a missing key.packages/utils/src/stats.ts:roughSizeOfObjectuses aSetfor visited objects. ASetuses the same SameValueZero equality asincludes. AWeakSetis not safe here, because abigintcan get to that branch.Verification
Reproduction script from the issue, before and after the fix:
Partial delete,
deleteMany(["a", "missing1", "missing2"])on 3 cached keys:count=0,ksize=0,vsize=0,deletes=3.count=2,ksize=4,vsize=14,deletes=1. These are the correct values.Checks:
pnpm testpasses inpackages/utils,packages/cacheable,packages/memory, andpackages/node-cache, with a local Redis.stats.tshas 100% coverage. The line that is not covered incacheable/src/index.ts(line 422, thesyncsetter) was not covered before this PR.Test audit:
cacheable stats enabled > should get stats on delete many. This test owns thedeleteManystats contract, so I extended it and did not add a near-duplicate.deleteManychanges the stats only for keys that exist, also when the cached value isundefined. It subtracts the size of each value one time.undefined) and one missing key. It comparesksizeandvsizewith a cache that holds only the remaining key.main(deletesis 3, expected 2). It fails on the first commit of this PR (deletesis 1, expected 2), becauseget()skipped the cachedundefinedvalue. Thevsizecheck also fails if a change brings back the whole-array sizing, even when all keys exist.Setchange inroughSizeOfObject. The behavior does not change, and only a timing test can find the regression. A timing test is not stable in CI. The circular-reference tests inpackages/utils/test/stats.test.tscover the visited-object check.🤖 Generated with Claude Code
https://claude.ai/code/session_01DQyS4TFa7JFA5PEcKuLj4T