fix(cache): handle eviction promise rejections and use waitUntil - #16
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughEviction now runs concurrently across all storage bases via Promise.all. Eviction calls are caught and forwarded to _onError; eviction promises are scheduled with event?.req.waitUntil?.(...). Unsafe any-casts accessing waitUntil were removed and replaced with optional chaining. ChangesCache eviction & scheduling
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,230,255,0.5)
Caller->>+CacheModule: trigger eviction / SWR revalidation 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->>Event/_onError: _onError("[cache] Cache eviction error.", err)
end
opt event.req.waitUntil available
CacheModule->>Event: event.req.waitUntil(evictPromise)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
|
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
…evict-promises
Make _evictFromStorage async so a synchronous throw from storage.set becomes a rejection handled by the callers' catch, instead of masking the original resolver error. Add tests for both async and sync eviction errors and update the stale waitUntil note in AGENTS.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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