LTRAC-1583: Add a unit test harness to core and expire in-memory KV entries - #3176
Merged
Conversation
🦋 Changeset detectedLatest commit: 2290b71 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Bundle Size ReportComparing against baseline from No bundle size changes detected. |
jorgemoya
force-pushed
the
jorgemoya/ltrac-1499-cloudflare-kv-adapter
branch
from
August 18, 2026 02:19
d6395c3 to
a7b3c85
Compare
jorgemoya
changed the base branch from
jorgemoya/ltrac-1499-cloudflare-kv-adapter
to
jorgemoya/core-vitest-setup
August 18, 2026 02:20
jorgemoya
force-pushed
the
jorgemoya/ltrac-1583-memory-kv-ttl
branch
from
August 18, 2026 02:20
3751490 to
df1de76
Compare
Contributor
Unlighthouse Performance Comparison — VercelComparing PR preview deployment Unlighthouse scores vs production Unlighthouse scores. Summary ScoreAggregate score across all categories as reported by Unlighthouse.
Category Scores
Core Web Vitals
|
chanceaclark
left a comment
chanceaclark
left a comment
Contributor
There was a problem hiding this comment.
Before I review this any further, can you consult with our platform-dispatch-router (internal) and see if there is anything we can borrow from that in-memory cache, including TTLs and whatnot.
core has never had one. `tests/` holds 63 Playwright specs run by `playwright test`; there was no `test` script, no runner, and no way to unit test anything under `lib/`. Scoped to `lib/**/*.spec.ts` deliberately. Playwright specs share the `*.spec.ts` suffix, so an unscoped Vitest would collect all 63 and fail on their fixture imports. This glob is the part worth reviewing closely: widen it and Vitest swallows the e2e suite, narrow it and specs silently stop running. Colocated rather than under `tests/`, matching how packages/catalyst already does unit tests -- build.spec.ts next to build.ts, and so on. `tests/` is Playwright's testDir, so specs placed there would need either a separate suffix or a testIgnore entry to keep the two runners apart; and `tests/lib/` already means "helpers for Playwright tests", which would make `tests/lib/kv` ambiguous. The `~` alias mirrors the tsconfig path alias so lib code importing `~/...` resolves the same way it does in the app. Kept as its own commit so the harness is separable from the fix that follows, but shipped in the same PR: a runner with no specs passes CI vacuously and proves nothing, which is a poor thing to ask a reviewer to approve. Refs LTRAC-1583 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jorgemoya
force-pushed
the
jorgemoya/ltrac-1583-memory-kv-ttl
branch
from
August 18, 2026 16:46
df1de76 to
db507ee
Compare
jorgemoya
marked this pull request as draft
August 18, 2026 16:46
jorgemoya
changed the title
jorgemoya
marked this pull request as ready for review
August 18, 2026 16:49
jorgemoya
marked this pull request as draft
August 18, 2026 17:12
jorgemoya
force-pushed
the
jorgemoya/ltrac-1583-memory-kv-ttl
branch
from
August 18, 2026 17:37
db507ee to
f4cdf83
Compare
jorgemoya
force-pushed
the
jorgemoya/ltrac-1583-memory-kv-ttl
branch
from
August 18, 2026 17:52
f4cdf83 to
1961a66
Compare
…e refreshes MemoryKvAdapter doubles as an L1 in front of whichever shared adapter is selected, and KV.mget skips the shared store whenever memory holds every requested key. Nothing ever expired those entries, so that short-circuit was permanent. That left the `expiryTime` callers embed in the value as the only thing driving refreshes -- and that path refetches from the origin, not from the shared store. So each process ran its own refresh loop on a clock starting from whenever it first cached the key, and never picked up a value another process had already fetched and written. Observed on a deployed store: storeStatus was written at 01:19:01 with a window ending 01:24:01, and at 01:21:12 a request read the *previous* value and refetched again. That read never reached the shared store despite a fresher value having been there for two minutes. A later trace showed a route entry served from memory 14 hours stale. Nothing was ever served incorrectly -- staleness stayed bounded by the caller's own expiryTime, and stale-while-revalidate still served fast and refreshed behind the request. The cost was duplicated work: origin requests and cache writes scaling with process count instead of being shared. 60s matches Workers KV's floor for `cacheTtl` on a read, so the two layers share one staleness window rather than one each. It sits inside the shortest window callers embed in their values (5 minutes for storefront status, 30 for routes). The trade is one shared-store read per key per window against a value nothing ever replaces; those reads become near-free once CloudflareKvAdapter passes `cacheTtl`, which serves them from the colo cache. Capacity goes from 500 to 4096. Cache keys include the query string, so distinct keys accumulate much faster than the number of real paths suggests -- a crawler walking `?utm_*` permutations alone can churn through the old limit. Not using allowStale/noDeleteOnStaleGet, which would serve an expired entry instantly and refresh it off the response path: that needs a `waitUntil` handle to run the refresh on, and KV.mget is called from with-routes without the request's event. Returning stale values with no refresh would reinstate the bug this fixes. Expiry now comes from lru-cache rather than being tracked by hand. The previous expiresAt bookkeeping was only consulted by a private `get` that nothing called -- KV.get delegates to mget -- so it expired nothing, and a caller passing `ex` would have had it silently ignored. `ex` now maps to a per-entry TTL override. mget also returns null rather than undefined for a miss, matching KvAdapter. Two things worth knowing about the tests. lru-cache captures a reference to the `performance` object at module load, and Vitest's fake timers swap the global for a new object that reference never sees -- so the cache's clock has to be moved by stubbing the method in place. And lru-cache treats a recorded start time of exactly 0 as "no TTL", so the fake clock starts at a non-zero baseline; starting at zero makes every expiry assertion pass whether or not the TTL works. Verified both the TTL and the capacity change fail their specs when reverted. Refs LTRAC-1583 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jorgemoya
force-pushed
the
jorgemoya/ltrac-1583-memory-kv-ttl
branch
from
August 18, 2026 17:56
1961a66 to
2290b71
Compare
Contributor
Author
Consulted and added what made sense. |
jorgemoya
marked this pull request as ready for review
August 18, 2026 18:01
chanceaclark
approved these changes
Aug 18, 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.
Linear: LTRAC-1583
Two commits: the test harness
corehas never had, and the fix it exists to cover. Base for #3170.What/Why?
lib/kvkeeps a per-processMemoryKvAdapterin front of whichever shared adapter is selected, andKV.mgetskips the shared store whenever memory holds every requested key. Nothing ever expired those entries, so that short-circuit was permanent — once a process had seen a key it never consulted the shared store again, could not observe a refresh another process had written, and re-ran the refresh itself.Caught on a deployed store:
01:19:01—storeStatuswritten, window ends01:24:0101:21:12— a request reads the previous value and revalidates againThat read never reached the shared store despite a fresher value having sat there for two minutes. A later trace showed a route entry served from memory 14 hours stale.
Nothing was ever served incorrectly. Staleness stayed bounded by the caller's own
expiryTime, and stale-while-revalidate still served fast and refreshed behind the request. The cost was duplicated work — origin fetches and cache writes scaling with process count rather than being shared.The two values
60s TTL (was 30s) — matches Workers KV's floor for
cacheTtlon a read, so the layers share one staleness window rather than one each. It sits inside the shortest window callers embed in their values (5 min for storefront status, 30 for routes).4096 entries (was 500) — cache keys include the query string, so distinct keys accumulate much faster than the number of real paths suggests; a crawler walking
?utm_*permutations alone can churn through the old limit.The trade, stated plainly
This adds a shared-store read per key per window that we previously didn't do, in exchange for origin fetches and cache writes no longer multiplying by process count. Storefront API calls are the scarce resource and KV reads are cheap, so it's favourable — but it isn't free.
It also pairs with a follow-up:
CloudflareKvAdapterpassingcacheTtlon the read would serve those extra reads from the colo cache, shared across every isolate in a data centre. That belongs in #3170, since this layer also fronts Upstash and Vercel. Worth noting the two only work together —cacheTtlis inert if memory never expires, because the read never happens.Considered and not adopted
allowStale+noDeleteOnStaleGetwould serve an expired entry instantly and refresh it off the response path, which is nicer than a blocking read. It needs somewhere to run the refresh:KV.mgetis called fromwith-routes.ts:262without the request's event and has nowaitUntilhandle. Returning stale values with no refresh would reinstate the exact bug this fixes.Single-flight on revalidation — two concurrent requests observing the same stale entry currently fire two origin fetches and two KV writes, seen twice in traces including two writes to the same route key 207ms apart. That revalidation lives in
with-routes, not here.Also unchanged:
KV.mgetfilters memory values with.filter(Boolean), so a legitimately cachednull— a path with no route — reads as a miss and re-hits the shared store.Testing
vitest run— 10 pass. Lint andtsc --noEmitclean.memory.spec.tscovers the round trip, key ordering across a mixed hit/miss batch,nullround-tripping, expiry at the default window, expiry inside the callers' shortest window,exoverriding the default, the window refreshing on rewrite, eviction at capacity, and that capacity now exceeds the old 500.Verified both changed values fail their specs when reverted — removing the TTL breaks two specs, restoring
max: 500breaks the capacity spec. Two things made that non-obvious, both noted at their call sites:lru-cachecaptures a reference to theperformanceobject at module load. Vitest's fake timers replace the global with a new object that reference never sees, so the cache's clock has to be moved by stubbing the method in place.lru-cachetreats a recorded start time of exactly0as "no TTL". The fake clock therefore starts at a non-zero baseline — starting at zero makes every expiry assertion pass whether or not the TTL works, which is how this nearly shipped green.Migration
None. No API change; the only behavioural difference is that a process re-reads the shared store for a key it has held for more than 60 seconds.