perf(routing): stabilize cached query keys - #2654
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTwo unrelated edits: (1) provider metrics history aggregation now pins a stable Drizzle cache tag and disables write-based invalidation while retaining TTL expiry; (2) discount lookup removed SQL expiry filtering, instead selecting ChangesCache Strategy for Provider Metrics History
Discount expiry filter relocation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 0bff2d1185
ℹ️ 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".
| .$withCache({ | ||
| autoInvalidate: false, | ||
| config: { ex: HISTORY_SWR_TTL_SECONDS }, |
There was a problem hiding this comment.
This does not stop the hot-table writes from evicting the cache entry in this repo: RedisCache.put() still adds every cached result to the table index, and RedisCache.onMutate() unconditionally calls invalidateByTables() for mutated tables, so the next model_provider_mapping_history upsert still deletes this entry before its 30s TTL. autoInvalidate: false only avoids the timestamp check in get(), so under the high-throughput path described in the commit the heavy aggregation will continue to run after almost every write unless the table-index deletion is also bypassed for this query.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified this is not the case here. With autoInvalidate: false, drizzle-orm passes tables: [] to RedisCache.put() (pg-core/session.js: this.cacheConfig.autoInvalidate ? this.queryMetadata.tables : []), so this entry is never added to the drizzle:table_keys:* index and invalidateByTables() can't reach it. Additionally, the worker writes model_provider_mapping_history via the uncached db client, so onMutate() never fires for this table at all. The stable tag is what actually fixes the original problem (the millisecond Date.now() window params were busting the per-query hash key); autoInvalidate: false just documents the TTL-only intent.
Two hot-path queries were silently bypassing their Redis cache and hitting
Postgres on every request, overloading the DB under high throughput (~50 r/s).
Root cause (both): the cached Drizzle client keys its cache on
hashQuery(sql, params). When a query embeds a `Date` derived from Date.now()
at millisecond precision as a SQL parameter, the key is unique per request,
so the cache never hits. swrWrap does not help here — it only serves its
Redis mirror as a fallback on DB error, never on the happy path.
- provider-metrics-history: the routing-metrics weighted aggregation derives
window/tier boundaries from Date.now(). Pin a stable $withCache({ tag })
using the timestamp-independent (history-config-hash, modelIds) key so
requests with the same model set share one cached result for the TTL.
- findEffectiveDiscount: the discount lookup filtered `expiresAt >= now` in
SQL. Move the expiry check to JS (evaluated fresh per call) so the SQL stays
time-independent and the cache key is stable. This path is especially hot —
it runs once per provider candidate during routing.
Both tolerate the existing cache TTL of staleness by design.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0bff2d1 to
31e2567
Compare
steebchen
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31e25672fd
ℹ️ 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".
|
|
||
| const now = Date.now(); | ||
| const discounts = rows.filter( | ||
| (row) => row.expiresAt === null || row.expiresAt.getTime() >= now, |
There was a problem hiding this comment.
When this lookup hits the Drizzle Redis cache for a discount with a non-null expiry, expiresAt is no longer a Date: RedisCache.put() stores the row through JSON.stringify and get() returns the parsed payload, so the timestamp comes back as a string. In that cached-hit path this filter throws row.expiresAt.getTime is not a function, breaking requests for org/provider/model combinations covered by an expiring discount after the first cached read; parse the value with new Date(...) or compare as a timestamp-safe type before calling getTime().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified empirically that expiresAt is a Date even on a Drizzle cache hit, so .getTime() does not throw. Drizzle caches the raw pg result (text values + field dataTypeIDs), and on restore it re-applies pg's type parsers — a timestamp column (dataTypeID 1114) is parsed back to a Date. Logging row.expiresAt instanceof Date on the cache-hit path returns true. (The swrWrap mirror also can't surface a string here — it caches the fetcher's return {discount, source}, which has no expiresAt.) I've still wrapped the compare in new Date() defensively and added a regression test (173410e) that resolves an expiring discount through the Drizzle cache with the SWR mirror flushed.
Add a regression test exercising findEffectiveDiscount served from the Drizzle cache for a discount with a non-null expiry, and normalize the expiry compare with new Date() defensively (expiresAt is already a Date on both fresh and cached reads, since Drizzle re-applies the timestamp parser on cache restore). Addresses automated review feedback on #2654. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Problem
Under high throughput on chat completions (~50 r/s), Postgres gets overloaded. Two hot-path queries that appear to be cached were in fact hitting Postgres on every request.
Root cause (shared)
The cached Drizzle client (
cdb) uses a Redis cache with strategy"all". Unless a$withCache({ tag })is given, the cache key ishashQuery(sql, JSON.stringify(params)). When a query embeds aDatederived fromDate.now()at millisecond precision as a SQL parameter, the key is unique on every request, so the cache never hits.swrWrapdoes not save us here: it always calls the fetcher on the happy path and only serves its Redis mirror as a fallback when Postgres errors — so a stable swrWrap key does nothing to reduce DB load. The real per-request cache is the Drizzle$withCachelayer, and its key was being busted.(Note:
autoInvalidate-on-write was not the lever — the worker writes these tables via the uncacheddb, soonMutate/table-invalidation never fires for them.)Fixes
1.
packages/db/src/provider-metrics-history.ts— routing metrics aggregationThe weighted
sum(...)aggregation overmodel_provider_mapping_historyderives its window/tier boundaries fromDate.now(), which entered the query as params → unique key per request → every routing decision re-ran the heavy aggregation against Postgres. Fix: pin a stable$withCache({ tag })using the timestamp-independent(history-config-hash, modelIds)key (the same key the SWR mirror already uses), so requests with the same model set + history config share one cached result for the 30s TTL.2.
apps/gateway/src/lib/cached-queries.ts—findEffectiveDiscountThe discount lookup filtered
expiresAt >= nowin SQL, so thenowDate param busted the cache key the same way. This path is especially hot — it runs once per provider candidate during routing. Fix: drop the time predicate from SQL and apply the expiry filter in JS (evaluated fresh per call). The SQL is now time-independent → the cache key is stable → the cache actually hits, while expiry is still evaluated correctly on every call.Both queries already tolerate up to their cache TTL of staleness by design, so only when the cache refreshes changes — not correctness.
Impact
DB load for each of these drops from ~one query per request to at most one per TTL per distinct cache key.
Testing
pnpm format— cleanpnpm build— 17/17 successfulpnpm vitest rundiscount cache tests — pass (other failures in the suite are pre-existing local DB-seeding/FK issues, in unrelated tests)🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes
Tests