feat(cache): stale-on-error via LRU touch on bgFlights failure#1132
Open
feat(cache): stale-on-error via LRU touch on bgFlights failure#1132
Conversation
Contributor
Tagging OptionsShould a new tag be published when this PR is merged?
|
📝 WalkthroughWalkthroughWhen background revalidation fails, the system now extends the cached entry's lifetime using an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="blocks/loader.ts">
<violation number="1" location="blocks/loader.ts:345">
P1: The optional `touch` call is followed by a non-optional `.catch`, which can throw when `touch` is not implemented.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+345
to
+346
| .touch?.(request) | ||
| .catch((e) => logger.error(`loader touch error ${e}`)); |
Contributor
There was a problem hiding this comment.
P1: The optional touch call is followed by a non-optional .catch, which can throw when touch is not implemented.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At blocks/loader.ts, line 345:
<comment>The optional `touch` call is followed by a non-optional `.catch`, which can throw when `touch` is not implemented.</comment>
<file context>
@@ -337,7 +337,14 @@ const wrapLoader = (
+ // Origin failed — extend LRU TTL so stale content keeps being served
+ // while origin recovers, instead of evicting and returning a hard error.
+ (cache as Cache & { touch?: (r: RequestInfo | URL) => Promise<void> })
+ .touch?.(request)
+ .catch((e) => logger.error(`loader touch error ${e}`));
+ });
</file context>
Suggested change
| .touch?.(request) | |
| .catch((e) => logger.error(`loader touch error ${e}`)); | |
| .touch?.(request)?.catch((e) => logger.error(`loader touch error ${e}`)); |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@blocks/loader.ts`:
- Around line 340-347: The file has formatting differences causing CI to fail;
run the formatter (deno fmt) on the file containing the loader error handling
block (the code using logger.error(...) and the cache.touch?.(request) call) to
normalize spacing and line breaks so the catch block matches project style;
after formatting, re-run the formatter check to ensure deno fmt --check passes.
- Around line 340-347: The current code calls (cache as Cache & { touch?: (r:
RequestInfo | URL) => Promise<void> }).touch?.(request).catch(...), which throws
when touch is undefined because .catch is invoked on undefined; change the call
so the optional chaining also applies to .catch — i.e. call touch via (cache
...).touch?.(request)?.catch((e) => logger.error(`loader touch error ${e}`)) —
or alternatively guard with an if (typeof touch === 'function') before invoking
— referencing the cache variable, its touch? method, request and logger to
locate and update the loader error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87577151-0254-4bd7-a7a5-dba432343c67
📒 Files selected for processing (2)
blocks/loader.tsruntime/caches/lrucache.ts
Comment on lines
+340
to
+347
| .catch((error) => { | ||
| logger.error(`loader error ${error}`); | ||
| // Origin failed — extend LRU TTL so stale content keeps being served | ||
| // while origin recovers, instead of evicting and returning a hard error. | ||
| (cache as Cache & { touch?: (r: RequestInfo | URL) => Promise<void> }) | ||
| .touch?.(request) | ||
| .catch((e) => logger.error(`loader touch error ${e}`)); | ||
| }); |
There was a problem hiding this comment.
Fix formatting to pass CI.
The pipeline reports deno fmt --check found formatting differences. Run deno fmt on this file to resolve the CI failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@blocks/loader.ts` around lines 340 - 347, The file has formatting differences
causing CI to fail; run the formatter (deno fmt) on the file containing the
loader error handling block (the code using logger.error(...) and the
cache.touch?.(request) call) to normalize spacing and line breaks so the catch
block matches project style; after formatting, re-run the formatter check to
ensure deno fmt --check passes.
TypeError when touch is undefined: optional chaining doesn't extend to .catch().
If the cache implementation doesn't have a touch method (e.g., non-LRU caches), touch?.(request) returns undefined, and then .catch(...) is invoked on undefined, throwing a TypeError.
🐛 Proposed fix: extend optional chaining to the catch call
(cache as Cache & { touch?: (r: RequestInfo | URL) => Promise<void> })
.touch?.(request)
- .catch((e) => logger.error(`loader touch error ${e}`));
+ ?.catch((e) => logger.error(`loader touch error ${e}`));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@blocks/loader.ts` around lines 340 - 347, The current code calls (cache as
Cache & { touch?: (r: RequestInfo | URL) => Promise<void>
}).touch?.(request).catch(...), which throws when touch is undefined because
.catch is invoked on undefined; change the call so the optional chaining also
applies to .catch — i.e. call touch via (cache ...).touch?.(request)?.catch((e)
=> logger.error(`loader touch error ${e}`)) — or alternatively guard with an if
(typeof touch === 'function') before invoking — referencing the cache variable,
its touch? method, request and logger to locate and update the loader error
handling.
hugo-ccabral
approved these changes
Mar 20, 2026
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.
Summary
touch()method to LRU cache that extends TTL without modifying the underlying data orexpiresheadersizeas LRU value (wastrue) sotouchcan re-set the correct size when extending TTLERROR_STALE_TTLenv var (default 1h) to control the extension windowtouchin loader.tsbgFlights.catchafter logging the errorMotivation
When
bgFlightsfails due to origin outage (e.g. VTEX down), the stale cache entry expires afterSTALE_TTL_PERIOD(30s) and the next request becomes a hard MISS — returning an error to the user.With this change, on failure the LRU TTL is extended by
ERROR_STALE_TTL(1h), so stale content keeps being served while the origin recovers. Once the origin comes back, the next successfulbgFlightscallscache.putwhich resets the TTL back to normal (90s).This preserves the normal 30s
STALE_TTL_PERIODfor healthy traffic — avoiding the memory pressure of a globally extended stale window, which would cause LRU slot exhaustion in high-cache-hit scenarios.Test plan
ERROR_STALE_TTLenv var overrides default 1hSummary by cubic
Implements stale-on-error: when
bgFlightsrevalidation fails, the LRU entry’s TTL is extended (default 1h) so stale content keeps serving instead of returning errors. Healthy traffic is unchanged; TTL resets on the next successful revalidation.touch()to extend TTL without changing data or theexpiresheader.true) sotouchpreserves correct size; updateddisposesignature accordingly.ERROR_STALE_TTLenv var (default 1h) to control the extension window.cache.touchinbgFlightsfailure path inloader.ts, with error logging.Written for commit eb0371e. Summary will update on new commits.
Summary by CodeRabbit
Performance Improvements
Bug Fixes