fix(cache): cut per-request DB+Redis load in hot path - #2669
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR adds Redis-backed mirror write throttling to the SWR cache layer and updates gateway query functions to use short-TTL cached reads instead of uncached refetches when organization credits or wallet balances are zero or negative. The throttle mechanism prevents excessive mirror rewrites within a configurable time window while allowing immediate repopulation after cache invalidation. ChangesSWR Throttling and Gateway Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a56e099b04
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export async function findOrganizationByIdFresh( | ||
| id: string, | ||
| ): Promise<Organization | undefined> { | ||
| return await swrWrap(`org:${id}`, [organizationTableName], async () => { |
There was a problem hiding this comment.
When a zero-credit org is topped up while the regular 60s Drizzle entry still says zero, findOrganizationById first runs the regular swrWrap("org:${id}"); that stale zero result can take the new mirror throttle slot, so this fresh read returns the positive balance but cannot update the SWR mirror under the same key. If Postgres/Drizzle fails during that stale-cache window, both the regular and fresh paths fall back to the old zero mirror and continue rejecting the recently topped-up org; the wallet fresh path has the same pattern. Use a separate SWR key for the fresh mirror or force the fresh result to refresh the mirror.
Useful? React with 👍 / 👎.
| if (shouldRefreshMirror(key)) { | ||
| await writeMirror(key, tables, value); | ||
| } |
There was a problem hiding this comment.
If Redis evicts/restarts/flushes a swr:<key> entry while this process still has a recent lastMirrorWriteAt timestamp, the next successful fetch returns fresh data but skips writeMirror here, leaving no SWR fallback for that key until the 30s window expires. This also affects tests that flush Redis between cases and reuse keys: a DB failure immediately after a successful fetch can miss the mirror entirely. Check that the mirror still exists before suppressing the write, or clear the throttle marker when the mirror is absent.
Useful? React with 👍 / 👎.
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cache/src/swr.ts`:
- Around line 22-35: The throttle marker is set before the mirror write and is
not reverted on write failure; change shouldRefreshMirror/usage so the key is
only set in lastMirrorWriteAt after writeMirror succeeds (or remove the marker
if writeMirror throws/returns failure). Concretely: move
lastMirrorWriteAt.set(key, now) out of shouldRefreshMirror so
shouldRefreshMirror only checks/clears bounds and returns whether a write should
be attempted; then in the code that calls writeMirror (and in any analogous
callers), set lastMirrorWriteAt.set(key, Date.now()) only after a successful
write, and if writeMirror fails ensure you do not set or you delete the marker
(lastMirrorWriteAt.delete(key)). Preserve the existing bounds logic (clear when
size >= SWR_MIRROR_THROTTLE_MAX_KEYS) where appropriate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0acfc1d7-1961-4de4-bdce-029a7731717a
📒 Files selected for processing (3)
apps/gateway/src/lib/cached-queries.tspackages/cache/src/swr.spec.tspackages/cache/src/swr.ts
swrWrap rewrote its Redis fallback mirror on every successful fetch (SET + SADD/EXPIRE per table). With ~15-30 wrapped lookups per request at high r/s that became a dominant Redis load source, starving the gateway event loop and producing upstream connect-timeout 503s under load. Throttle mirror writes to at most once per 30s per key per process; the mirror's hours-long TTL keeps the disaster-fallback valid. Invalidation also clears the throttle marker so an invalidated mirror repopulates on next fetch. findOrganizationById/findWalletById refetched truly uncached whenever credits/balance <= 0, so a BYOK/zero-credit org did one raw Postgres SELECT per request and saturated the pool (max 20). Switch those refetches to a 2s-TTL cached read via $withCache with a distinct tag, so topups still reflect within 2s while per-request DB load drops to ~1 query per window per row. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The mirror write-throttle was an in-memory per-process Map, but the thing it gated (the SWR mirror) lives in Redis and can vanish independently (eviction, TTL, FLUSHDB, failover). When they desynced, the process kept suppressing re-priming for the full window while no mirror existed, silently removing the disaster fallback — and it broke the gateway's DB-outage resilience specs, which flush Redis between tests but cannot clear the in-memory Map. Move the throttle into Redis via SET NX EX so the marker is dropped by the same events that drop the mirror; the next request then re-primes immediately. Also address review feedback: - Release the throttle slot when writeMirror fails so a transient Redis write error does not suppress the mirror for the whole window (writeMirror now reports success). - Give the zero-credit/zero-balance "fresh" reads distinct SWR mirror keys (org:fresh / wallet:fresh) so a stale-zero regular read can't claim the throttle slot and suppress the fresh value's mirror write, which could otherwise keep serving stale-zero during a DB outage right after a topup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
a56e099 to
73018ad
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73018adfa6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| await writeMirror(key, tables, value); | ||
| if (await claimMirrorWrite(key)) { |
There was a problem hiding this comment.
When a cached row changes through the management API, those updates use the uncached db.update path (for example apps/api/src/routes/keys-api.ts:1428), so the gateway only sees the new value once the Drizzle TTL expires. If that first successful read lands while this 30s throttle key still exists, the request correctly returns the new row but the SWR mirror is not rewritten; a Postgres outage in that window then falls back to the old mirror for the stale TTL, e.g. continuing to accept a key that was just marked inactive. Consider forcing a mirror refresh when the underlying query produces a new value, or ensuring these management mutations invalidate the SWR/Drizzle caches.
Useful? React with 👍 / 👎.
1 participant
Problem
A load test (
500 r/s for 5s) against production showed an ~8.8% error rate (mostlyHTTP 503 upstream connect ... connection timeout) and very high latency (avg 8.4s, p99 14.7s). The 503s are an upstream connect timeout (LB→gateway, ~5s), which points to the gateway event loop being starved rather than the request itself being slow.Two per-request load sources in the hot path were defeating the caching layer:
1. SWR mirror rewritten on every request (Redis storm)
swrWrapwraps ~15–30 lookups per chat completion (findApiKeyByToken,findProjectById,findOrganizationById,findProviderKey, …). On every successful fetch it calledwriteMirror— a Redis pipeline ofSET+ (SADD+EXPIREper table) — even when the underlying query was a cache hit and nothing changed. At 500 r/s that's ~7.5k–15k redundant Redis pipelines/sec. The mirror is only ever read as a fallback when Postgres is down (4h TTL), so rewriting it every request buys nothing and keeps the single-threaded Redis (and the gateway'sawaits) busy.2. Uncached refetch for zero-credit orgs (Postgres hammer)
findOrganizationById/findWalletByIdrefetched truly uncached whenever credits/balance<= 0(to reflect topups instantly). For a BYOK / zero-credit org that's one raw PostgresSELECTper request. Against a pool ofmax: 20, 500 r/s saturates the pool — matching the observed latencies and connect-timeout 503s.Changes
packages/cache/src/swr.ts: throttle the mirror write to at most once per 30s per key, collapsing the per-request pipeline to a single conditionalSET … NX EX. The throttle marker lives in Redis, not in process memory, on purpose: the mirror it gates can disappear independently (eviction, TTL,FLUSHDB, failover); an in-memory marker would desync and suppress re-priming for a whole window while no mirror exists — silently removing the disaster fallback. A Redis-side marker is dropped by the same events that drop the mirror, so the next request re-primes immediately.writeMirrornow reports success and the throttle slot is released on write failure so a transient Redis error doesn't suppress the mirror for the window. Invalidation clears the throttle markers for invalidated keys.apps/gateway/src/lib/cached-queries.ts: rename the*Uncachedhelpers to*Freshand switch the zero-credit/zero-balance refetch to a 2s-TTL cached read ($withCache, distinct tag,autoInvalidate: false). Topups now reflect within 2s instead of instantly, but a zero-balance org under load drops from ~500 SELECTs/s to ~1 per 2s. The fresh reads also use distinct SWR mirror keys (org:fresh/wallet:fresh) so a stale-zero regular read can't claim the mirror write-throttle slot and suppress the fresh value's mirror write (which could otherwise keep serving stale-zero during a DB outage right after a topup).Testing
packages/cache/src/swr.spec.ts— 9/9 pass (incl. throttle, invalidation-clears-throttle, and release-throttle-on-write-failure)apps/gateway/src/lib/cached-queries-swr.spec.ts(15) +apps/gateway/src/chat/chat-resilience.spec.ts(8) — pass (these exercise the SWR DB-outage fallback an in-memory throttle broke)pnpm build(turbo) clean across all 17 packages🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Refactor