Fix race conditions and correctness issues in caching layers#417
Conversation
Synthesizes PR #414 (invalidation/in-flight race audit) and PR #415 (correctness audit), combining best-of-breed fixes from both. Race conditions between invalidation and in-flight loads: - getAsyncOnly (flat + group) fences its write-back: the resolved value is persisted only if the dedup entry still belongs to that load, so any invalidation or forced write that evicted the entry cancels the write-back while awaiting callers still receive the value. - Invalidation paths (invalidateCache/invalidateCacheFor/invalidateCacheForMany/ invalidateCacheForGroup) evict running loads before and after the awaited async delete and clear the in-memory entry last, so a get racing a slow async delete cannot repopulate memory after the invalidation resolves. - deleteGroupRunningLoad only drops the group entry when it still points at the settling load's map, so a load settling against a stale map cannot evict a newer load's map and break deduplication. Correctness fixes: - InMemoryCache.getMany / InMemoryGroupCache.getManyFromGroup treated cached falsy values (0, '', false, null) as misses via a truthy check; use an undefined check instead, consistent with single get and RedisCache. - InMemoryGroupCache reads (getFromGroup, getManyFromGroup, getExpirationTimeFromGroup, deleteFromGroup) no longer insert empty groups into the LRU on a miss, which could evict a populated group's still-valid data. - Loader/GroupLoader preemptive-refresh expiration lookups now have a .catch, so an async-cache error no longer becomes an unhandled promise rejection. - RedisGroupCache.deleteGroup expires the group index with groupTtlInMsecs instead of the entry ttlInMsecs, consistent with setForGroup/setManyForGroup. Redis notification consumers: - Register the message listener before subscribing, so no message is dropped in the window between the SUBSCRIBE ack and listener attachment (from #414). - Filter incoming messages by channel, so consumers sharing one connection no longer cross-apply each other's messages. - Guard JSON.parse, so a malformed payload no longer crashes the consumer (#415). All fixes are covered by regression tests.
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (21)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
The race-condition/correctness commit added branches that were untested, dropping line/statement coverage below the 100% threshold and functions below 97%, which turned CI red. Cover the new paths: - Loader/GroupLoader: the .catch on the fire-and-forget expiration lookup, exercised by stubbing expirationTimeLoadingOperation to reject. - RedisGroupNotificationConsumer: the malformed-payload guard and the wrong-channel filter on a shared consumer connection, mirroring the existing flat-consumer tests.
* Apply performance optimizations from #418 on top of #417 The bug fixes originally bundled in #418 (falsy value caching in InMemoryCache/InMemoryGroupCache getMany, and reads no longer inserting empty groups into the LRU) already landed via #417, so only the performance and code-quality changes remain: - AbstractFlatCache/AbstractGroupCache: resolve the cache key once and reuse it across getInMemoryOnly/getAsyncOnly/get instead of resolving it multiple times per call - getMany merge appends async results into the in-memory result array in place (it is always freshly allocated) rather than spreading into a new array - Loader/GroupLoader: build cache entries with an explicit loop, merge cached and loaded values with concat (cachedValues may be owned by a user-implemented async cache), and hoist the dataSource local - AbstractRedisCache: precompute keyPrefix; RedisGroupCache: precompute the group index prefix and the per-group entry prefix - unique(): early return for arrays of 0-1 elements Regression tests for the falsy-caching path at the loader level and for the unique() early return are included. * Update toad-cache
1 participant
Synthesizes #414 and #415 into a single change, combining the best-of-breed solution from each and addressing every issue both raised. Where the two overlapped (getAsyncOnly write-back fencing, Redis channel filtering, the GroupLoader background-refresh in-memory propagation) the strongest version of each fix is kept; where they were disjoint, both are included.
Race conditions between invalidation and in-flight loads (from #414)
getAsyncOnly(flat + group) fences its write-back: the resolved value is persisted only if the dedup entry still belongs to that load. Any invalidation or forced write that evicted the entry cancels the write-back, while callers already awaiting the promise still receive the value.invalidateCache,invalidateCacheFor,invalidateCacheForMany,invalidateCacheForGroup) evict running loads before and after the awaited async delete and clear the in-memory entry last, so agetracing a slow async delete cannot repopulate memory after the invalidation resolves.deleteGroupRunningLoadonly drops the group entry when it still points at the settling load's map, so a load settling against a stale map cannot evict a newer load's map and break deduplication.Correctness fixes (from #415)
InMemoryCache.getMany/InMemoryGroupCache.getManyFromGrouptreated cached falsy values (0,'',false,null) as misses via a truthy check; they now use anundefinedcheck, consistent with singlegetandRedisCache.InMemoryGroupCacheread operations (getFromGroup,getManyFromGroup,getExpirationTimeFromGroup,deleteFromGroup) no longer insert empty groups into the LRU on a miss, which could evict a populated group's still-valid data.Loader/GroupLoaderpreemptive-refresh expiration lookups now have a.catch, so an async-cache error no longer becomes an unhandled promise rejection.RedisGroupCache.deleteGroupexpires the group index withgroupTtlInMsecsinstead of the entryttlInMsecs, consistent withsetForGroup/setManyForGroup.Redis notification consumers (best-of-breed merge of both)
JSON.parse, so a malformed payload no longer crashes the consumer (from Fix correctness issues in caching layers #415).Notes
mainvia Add optional staleness checker for conditional cache refresh #416 (inrefreshOrBumpTtl), so it is not re-applied here; the associated regression test is still added.tsc --noEmitis clean. Biome reports only the 10 pre-existing formatting diagnostics already present onmain; this change adds none.Supersedes #414 and #415.