feat: support isEntryStillCurrentFn on in-memory-only loaders#420
Merged
Conversation
The staleness probe was hard-gated on an async cache tier: it only fired in Loader/GroupLoader's async-hit branch and bumped freshness via asyncCache.resetTtl/resetTtlFromGroup, so an in-memory-only loader in the refresh window always paid a full data-source reload even when a cheap probe could prove the cached value still current. Make the existing pair of inMemoryCache.ttlLeftBeforeRefreshInMsecs and isEntryStillCurrentFn a supported combination: - Relax assertStalenessCheckSupported to accept a probe when either the async cache has a refresh window (+ resetTtl, unchanged) or the in-memory cache has a refresh window. - Route the in-memory refresh-ahead through the probe via a new overridable scheduleInMemoryRefresh seam on the abstract caches; the loaders run the probe against the in-memory value and bump its TTL when current, falling back to the full background reload otherwise. - Add resetTtl/resetTtlFromGroup to the in-memory caches (and NoopCache) plus the SynchronousCache/SynchronousGroupCache interfaces. - Reuse the existing isKeyRefreshing / groupRefreshFlags stampede guards. Precedence is unchanged: when an async cache has a refresh window it keeps owning the probe, so existing configurations behave identically. Non-breaking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PYBYKbY1BJD9HFM1fXtFqW
|
Warning Review limit reached
Next review available in: 50 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. 📝 WalkthroughWalkthroughThis PR extends ChangesIn-memory staleness check feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Loader
participant isEntryStillCurrentFn
participant InMemoryCache
participant DataSource
Client->>Loader: get(key)
Loader->>Loader: scheduleInMemoryRefresh(key)
Loader->>isEntryStillCurrentFn: probe staleness
isEntryStillCurrentFn-->>Loader: true/false
alt still current
Loader->>InMemoryCache: resetTtl(key)
else stale
Loader->>DataSource: loadFromLoaders(key)
DataSource-->>Loader: fresh value
Loader->>InMemoryCache: set(key, value)
end
sequenceDiagram
participant Client
participant GroupLoader
participant isEntryStillCurrentFn
participant InMemoryGroupCache
participant DataSource
Client->>GroupLoader: get(key, group)
GroupLoader->>GroupLoader: scheduleInMemoryRefresh(key, group)
GroupLoader->>isEntryStillCurrentFn: probe staleness
isEntryStillCurrentFn-->>GroupLoader: true/false
alt still current
GroupLoader->>InMemoryGroupCache: resetTtlFromGroup(key, group)
else stale
GroupLoader->>DataSource: loadFromLoaders(key)
DataSource-->>GroupLoader: fresh value
GroupLoader->>InMemoryGroupCache: set(key, group, value)
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
lib/GroupLoader.ts (1)
126-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame duplication concern as
Loader.ts.
refreshOrBumpInMemoryTtlclosely mirrorsrefreshOrBumpTtl(Lines 150-180) - same probe/fallback shape, differing only in the value source (getFromGroupvs already-resolvedcachedValue) and reset target (resetTtlFromGroupvsasyncCache.resetTtlFromGroup). Consider factoring the shared probe+fallback logic into a helper, mirroring the suggestion made forLoader.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/GroupLoader.ts` around lines 126 - 149, `refreshOrBumpInMemoryTtl` duplicates the same probe-and-fallback flow used by `refreshOrBumpTtl`, so factor the shared logic into a helper to avoid maintaining two near-identical paths. Keep the helper generic over the cached value source and TTL reset target, then have `refreshOrBumpInMemoryTtl` and `refreshOrBumpTtl` delegate to it using their respective cache access/reset methods and the existing `isCurrentEntryTtlBumped` check.lib/Loader.ts (1)
178-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting shared probe/fallback logic.
refreshOrBumpInMemoryTtllargely duplicates the existingrefreshOrBumpTtl(Lines 202-227): both callisCurrentEntryTtlBumpedwith anisEntryStillCurrentFnwrapper, then fall back toloadFromLoaders+inMemoryCache.seton staleness/failure. The only real differences are the source ofcachedValueand the reset-TTL callback target.A shared private helper taking
cachedValue,resetTtl: () => Promise<boolean>as parameters would remove ~15 duplicated lines and keep future staleness-handling changes in one place.♻️ Possible consolidation
- private async refreshOrBumpInMemoryTtl(key: string, loadParams: LoadParams): Promise<void> { - const cachedValue = this.inMemoryCache.get(key) - if ( - this.isEntryStillCurrentFn && - // undefined means the entry vanished (expired/invalidated) between the read and the probe. - cachedValue !== undefined && - (await this.isCurrentEntryTtlBumped( - key, - () => this.isEntryStillCurrentFn!(cachedValue, loadParams), - () => Promise.resolve(this.inMemoryCache.resetTtl(key)), - )) - ) { - // Still current - the in-memory TTL was bumped, nothing else to do. - return - } - - // The entry is stale, the check failed, or the bump failed (entry expired/deleted meanwhile), - // so run the full background refresh from the data sources. - const freshValue = await this.loadFromLoaders(key, loadParams) - if (freshValue !== undefined) { - this.inMemoryCache.set(key, freshValue) - } - } + private async refreshOrBumpInMemoryTtl(key: string, loadParams: LoadParams): Promise<void> { + const cachedValue = this.inMemoryCache.get(key) + // undefined means the entry vanished (expired/invalidated) between the read and the probe. + if (cachedValue === undefined) { + return this.reloadAndCache(key, loadParams) + } + return this.refreshOrBumpTtlCommon(key, loadParams, cachedValue, () => + Promise.resolve(this.inMemoryCache.resetTtl(key)), + ) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/Loader.ts` around lines 178 - 201, `refreshOrBumpInMemoryTtl` duplicates the probe-and-fallback flow already used by `refreshOrBumpTtl`; extract that shared logic into a private helper and have both methods delegate to it. Keep the helper centered around `isCurrentEntryTtlBumped`, `loadFromLoaders`, and the cache write path, while parameterizing the `cachedValue` source and the `resetTtl` callback for `inMemoryCache`.test/memory/InMemoryGroupCache.spec.ts (1)
321-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
nullcached values inresetTtlFromGroup.The implementation explicitly treats a cached
nullas a valid, extendable entry (distinct fromundefined), but no test exercises that branch here.✅ Suggested additional test
it('returns false when the entry does not exist in an existing group', () => { const cache = new InMemoryGroupCache({ ttlInMsecs: 1000 }) cache.setForGroup('other', 'value', 'group1') expect(cache.resetTtlFromGroup('missing', 'group1')).toBe(false) }) + + it('extends the ttl of an existing entry cached as null', () => { + const cache = new InMemoryGroupCache({ ttlInMsecs: 1000 }) + cache.setForGroup('key', null, 'group1') + + expect(cache.resetTtlFromGroup('key', 'group1')).toBe(true) + }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/memory/InMemoryGroupCache.spec.ts` around lines 321 - 347, Add a test in InMemoryGroupCache.spec.ts for resetTtlFromGroup covering a cached null value as a valid entry. Use InMemoryGroupCache, setForGroup, and resetTtlFromGroup to store null under a group, then verify the call returns true and the expiration is extended, matching the existing existing-entry TTL behavior rather than the missing-entry cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/GroupLoader.ts`:
- Around line 126-149: `refreshOrBumpInMemoryTtl` duplicates the same
probe-and-fallback flow used by `refreshOrBumpTtl`, so factor the shared logic
into a helper to avoid maintaining two near-identical paths. Keep the helper
generic over the cached value source and TTL reset target, then have
`refreshOrBumpInMemoryTtl` and `refreshOrBumpTtl` delegate to it using their
respective cache access/reset methods and the existing `isCurrentEntryTtlBumped`
check.
In `@lib/Loader.ts`:
- Around line 178-201: `refreshOrBumpInMemoryTtl` duplicates the
probe-and-fallback flow already used by `refreshOrBumpTtl`; extract that shared
logic into a private helper and have both methods delegate to it. Keep the
helper centered around `isCurrentEntryTtlBumped`, `loadFromLoaders`, and the
cache write path, while parameterizing the `cachedValue` source and the
`resetTtl` callback for `inMemoryCache`.
In `@test/memory/InMemoryGroupCache.spec.ts`:
- Around line 321-347: Add a test in InMemoryGroupCache.spec.ts for
resetTtlFromGroup covering a cached null value as a valid entry. Use
InMemoryGroupCache, setForGroup, and resetTtlFromGroup to store null under a
group, then verify the call returns true and the expiration is extended,
matching the existing existing-entry TTL behavior rather than the missing-entry
cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0ffcd67-ba89-4527-bfe3-1d8254b5c31f
📒 Files selected for processing (18)
CHANGELOG.mdREADME.mdlib/AbstractCache.tslib/AbstractFlatCache.tslib/AbstractGroupCache.tslib/GroupLoader.tslib/Loader.tslib/memory/InMemoryCache.tslib/memory/InMemoryGroupCache.tslib/memory/NoopCache.tslib/types/SyncDataSources.tstest/GroupLoader-in-memory-staleness-check.spec.tstest/GroupLoader-staleness-check.spec.tstest/Loader-in-memory-staleness-check.spec.tstest/Loader-staleness-check.spec.tstest/fakes/DummyGroupedCache.tstest/memory/InMemoryCache.spec.tstest/memory/InMemoryGroupCache.spec.ts
…lyResolved The new in-memory refresh path issued its stale-fallback reload via a raw loadFromLoaders + inMemoryCache.set/setForGroup, outside the runningLoads / groupLoads registry. That bypassed the invalidation fence and dedup that the blind refresh path relies on, so: - an entry invalidated (invalidateCacheFor) while the fallback reload was in flight got resurrected after the fact; - a concurrent forceSetValue / forceSetValueForGroup was clobbered by the older in-flight snapshot; - the fallback reload was not deduplicated against a concurrent cache-miss load (double fetch). Route the fallback through the existing getAsyncOnlyResolved machinery (now protected) instead: it registers the reload in runningLoads/groupLoads, so the result is deduped and fenced by invalidateCacheFor / forceSetValue exactly like every other load. Also report resetTtl rejections against the tier that owns the refresh window (asyncCache ?? inMemoryCache) so the error handler never dereferences an undefined asyncCache on the in-memory-only path. Adds regression tests covering invalidation and forceSetValue racing an in-flight fallback reload for both Loader and GroupLoader. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WCqiRPk8mruEJfYdhZ2BSk
The method is now shared between the async and in-memory refresh paths (the reset target is injected via runResetTtl), so the summary no longer refers to "async cache TTL" specifically. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WCqiRPk8mruEJfYdhZ2BSk
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
2 participants
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.
The staleness probe was hard-gated on an async cache tier: it only fired in
Loader/GroupLoader's async-hit branch and bumped freshness via
asyncCache.resetTtl/resetTtlFromGroup, so an in-memory-only loader in the
refresh window always paid a full data-source reload even when a cheap probe
could prove the cached value still current.
Make the existing pair of inMemoryCache.ttlLeftBeforeRefreshInMsecs and
isEntryStillCurrentFn a supported combination:
cache has a refresh window (+ resetTtl, unchanged) or the in-memory cache has
a refresh window.
scheduleInMemoryRefresh seam on the abstract caches; the loaders run the probe
against the in-memory value and bump its TTL when current, falling back to the
full background reload otherwise.
the SynchronousCache/SynchronousGroupCache interfaces.
Precedence is unchanged: when an async cache has a refresh window it keeps
owning the probe, so existing configurations behave identically. Non-breaking.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01PYBYKbY1BJD9HFM1fXtFqW
Summary by CodeRabbit