fix(cache): handle eviction promise rejections and use waitUntil#16
fix(cache): handle eviction promise rejections and use waitUntil#16zsilbi wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughStale-cache eviction was converted to asynchronous execution via Promise.all across base tiers. Eviction calls are now caught and forwarded to _onError; eviction promises are scheduled with event?.req.waitUntil?.(...) where available. Unsafe type casts were removed in favor of direct optional chaining. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,230,255,0.5)
Caller->>+CacheModule: trigger eviction / handle SWR paths
end
rect rgba(220,255,200,0.5)
CacheModule->>+StorageTierA: set(key, null)
CacheModule->>+StorageTierB: set(key, null)
CacheModule->>+StorageTierN: set(key, null)
end
rect rgba(255,230,200,0.5)
StorageTierA-->>-CacheModule: Promise resolves/rejects
StorageTierB-->>-CacheModule: Promise resolves/rejects
StorageTierN-->>-CacheModule: Promise resolves/rejects
end
CacheModule->>CacheModule: Promise.all(...) aggregates
alt eviction error
CacheModule->>EventLoop/_onError: _onError("[cache] Cache eviction error.", err)
end
opt event.req.waitUntil available
CacheModule->>Event: event.req.waitUntil(evictPromise)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 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 |
This comment has been minimized.
This comment has been minimized.
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cache.ts`:
- Around line 140-142: Replace direct runtime-specific accesses to
event.req.waitUntil with the repo-standard pattern by reading the waitUntil
function once into a local const (e.g., const waitUntil = (event.req as
any).waitUntil) and then call waitUntil(evictPromise) at each site; update all
four call sites that currently call event.req.waitUntil(...) to use the local
waitUntil reference (so references like event.req.waitUntil(evictPromise) become
waitUntil(evictPromise)) to avoid repetition and ensure compatibility with
srvx/Cloudflare runtimes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
1 participant
Based on the review in #15 (comment)
_evictFromStoragewas callingstorage.set(..., null)in a bareforloop with no promise handling, leaving rejections from async storage backends unhandled._evictFromStoragenow returnsPromise.all(...)instead of fire-and-forgetting in a loop.catchthat routes errors through the existing_onErrorhookevent.req.waitUntilis available (srvx / Cloudflare Workers pattern), the eviction promise is registered there so the runtime keeps the request context alive until the eviction completesSummary by CodeRabbit