Clone cache to prevent mutation of cache#3122
Conversation
…. that solves bug of not-loading translations.
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent cached API/text objects from being mutated after caching, addressing a bug where translations were not loading due to unintended shared-reference mutations.
Changes:
- Clone values before storing them in the APIV3 texts cache (
_textsStoreSet). - Clone values before storing them in the generic
_cachedApiPromisecache.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
static/js/sefaria/sefaria.js
Outdated
| _textsStoreSet: function(key, value) { | ||
| this._textsStore[key] = value; | ||
| this._textsStore[key] = Sefaria.util.clone(value); | ||
| }, |
There was a problem hiding this comment.
Cloning on set prevents later mutations of the original value from affecting the cache, but callers that read from _textsStore still receive the cached object by reference (e.g., via _cachedApiPromise), so they can still mutate and corrupt the cache after a cache hit. If the goal is to make the cache mutation-safe, consider returning a cloned copy on reads (or freezing cached objects in dev) rather than only cloning on writes.
static/js/sefaria/sefaria.js
Outdated
| store[key] = data; | ||
| store[key] = Sefaria.util.clone(data); | ||
| return data; | ||
| }) |
There was a problem hiding this comment.
_cachedApiPromise now caches a clone but returns the original data on a cache miss, while returning the cached value (by reference) on a cache hit. This makes the first result a different object than subsequent results and still allows cache corruption when callers mutate the returned object after a cache hit. Consider returning the cached cloned object consistently (and/or cloning on reads if you need strong immutability guarantees).
static/js/sefaria/sefaria.js
Outdated
| .then(data => { | ||
| if (processor) { data = processor(data); } | ||
| store[key] = data; | ||
| store[key] = Sefaria.util.clone(data); | ||
| return data; |
There was a problem hiding this comment.
Deep-cloning every _cachedApiPromise response can be expensive for large payloads (e.g., bulk text / text API responses) and doubles memory for cached entries. If this is intended mainly for specific caches (like _textsStore), consider limiting cloning to those stores or gating it behind a debug/opt-in flag to avoid a broad performance regression.
🧪 CI InsightsHere's what we observed from your CI run for a92fc41. 🟢 All jobs passed!But CI Insights is watching 👀 |
…ing only when unpacking from props.
YishaiGlasner
changed the title
stevekaplan123
left a comment
stevekaplan123
left a comment
There was a problem hiding this comment.
I'm worried about the expense. I think we should check if this affects performance for certain API calls first.
|
Wait, how big is the data we're talking about?
…On Sun, 8 Mar 2026, 10:56 stevekaplan123, ***@***.***> wrote:
***@***.**** commented on this pull request.
I'm worried about the expense. I think we should check if this affects
performance for certain API calls first.
—
Reply to this email directly, view it on GitHub
<#3122 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BLNFDLKRYX6XR47H52M5CJT4PUYSNAVCNFSM6AAAAACWEI66PKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTSMJRGIYTINBVHA>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
@yitzhakc it's a section |
Clone before returning from cache. When unpacking from props clone before saving in cache.
That solves bug of not-loading translations.
Important note
I'm open to the claim that it costs too much in performance, so the fix should be finding where the mutation happens and fix there. But, as we're using the cache, it's just expected that it will occur more that something is taken from cache, passed thru some component and then someone changes it without knowing he corrupts the cache.