Skip to content

perf(kv): local tag cache to reduce KV round-trips on cache hit #433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
james-elicx merged 5 commits into from
Mar 11, 2026
+311 −13
Merged

perf(kv): local tag cache to reduce KV round-trips on cache hit #433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
24b27ba
perf(kv): add local in-memory tag cache to reduce KV round-trips
Divkix Mar 11, 2026
94f07b7
fix(test): guard fake timers with try/finally to prevent leaks
Divkix Mar 11, 2026
50694d6
fix(kv): address bonk review comments on local tag cache
james-elicx Mar 11, 2026
549a350
chore: merge main, resolve conflict (ExecutionContextLike)
james-elicx Mar 11, 2026
6653e29
refactor(kv): reuse cached timestamps in invalidation check loop
james-elicx Mar 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 80 additions & 13 deletions packages/vinext/src/cloudflare/kv-cache-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,26 @@ export class KVCacheHandler implements CacheHandler {
private ctx: ExecutionContextLike | undefined;
private ttlSeconds: number;

/** Local in-memory cache for tag invalidation timestamps. Avoids redundant KV reads. */
private _tagCache = new Map<string, { timestamp: number; fetchedAt: number }>();
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unbounded growth: The _tagCache Map is never pruned. TTL-expired entries remain in the map indefinitely — they're just treated as cache misses and overwritten on the next fetch. In a long-lived Worker with many unique tags, this grows without bound.

Since resetRequestCache() is never called in the vinext request lifecycle (see below), this map will accumulate entries for the lifetime of the isolate.

Consider either:

  1. Deleting stale entries when they're encountered (cheap: just add else { this._tagCache.delete(tag); } after the TTL check in the first pass), or
  2. Adding a max-size cap and evicting oldest entries when exceeded.

/** TTL (ms) for local tag cache entries. After this, re-fetch from KV. */
private _tagCacheTtl: number;

constructor(
kvNamespace: KVNamespace,
options?: { appPrefix?: string; ctx?: ExecutionContextLike; ttlSeconds?: number },
options?: {
appPrefix?: string;
ctx?: ExecutionContextLike;
ttlSeconds?: number;
/** TTL in milliseconds for the local tag cache. Defaults to 5000ms. */
tagCacheTtlMs?: number;
},
) {
this.kv = kvNamespace;
this.prefix = options?.appPrefix ? `${options.appPrefix}:` : "";
this.ctx = options?.ctx;
this.ttlSeconds = options?.ttlSeconds ?? 30 * 24 * 3600;
this._tagCacheTtl = options?.tagCacheTtlMs ?? 5_000;
}

async get(key: string, _ctx?: Record<string, unknown>): Promise<CacheHandlerValue | null> {
Expand Down Expand Up @@ -130,21 +142,57 @@ export class KVCacheHandler implements CacheHandler {
}
}

// Check tag-based invalidation (parallel for lower latency)
// Check tag-based invalidation.
// Uses a local in-memory cache to avoid redundant KV reads for recently-seen tags.
if (entry.tags.length > 0) {
const tagResults = await Promise.all(
entry.tags.map((tag) => this.kv.get(this.prefix + TAG_PREFIX + tag)),
);
for (let i = 0; i < entry.tags.length; i++) {
const tagTime = tagResults[i];
if (tagTime) {
const tagTimestamp = Number(tagTime);
if (Number.isNaN(tagTimestamp) || tagTimestamp >= entry.lastModified) {
// Tag was invalidated after this entry, or timestamp is corrupted
// — treat as miss to force re-render
const now = Date.now();
const uncachedTags: string[] = [];

// First pass: check local cache for each tag.
// Delete expired entries to prevent unbounded Map growth in long-lived isolates.
for (const tag of entry.tags) {
const cached = this._tagCache.get(tag);
if (cached && now - cached.fetchedAt < this._tagCacheTtl) {
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: now - cached.fetchedAt < this._tagCacheTtl means a TTL of 0 will always be expired (since now - fetchedAt is always >= 0). This is correct and the TTL=0 test relies on it, but it's worth noting that tagCacheTtlMs: 0 effectively disables the local cache entirely, which is a useful property for testing.

// Local cache hit — check invalidation inline
if (Number.isNaN(cached.timestamp) || cached.timestamp >= entry.lastModified) {
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The timestamp >= entry.lastModified check uses >= which means a tag invalidated at the exact same millisecond as lastModified triggers invalidation. This matches the original code's behavior so it's not a regression, just calling it out — it means revalidateTag() called in the same millisecond as set() will invalidate the entry. Seems like the right call.

this._deleteInBackground(kvKey);
return null;
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: When a locally-cached tag triggers invalidation (early return here), the remaining tags in entry.tags that were in the uncachedTags list never get fetched. This is fine for correctness (entry is invalid regardless), but means those tags won't be warmed in the local cache for other entries that share them.

This is the right tradeoff — avoiding unnecessary KV reads on an already-invalidated entry is more valuable than speculatively warming the cache. Just calling it out for documentation purposes.

}
} else {
// Expired or absent — evict stale entry and re-fetch from KV
if (cached) this._tagCache.delete(tag);
uncachedTags.push(tag);
}
}

// Second pass: fetch uncached tags from KV in parallel.
// Populate the local cache for ALL fetched tags before checking invalidation,
// so that KV round-trips are not wasted when an earlier tag triggers an
// early return — subsequent get() calls benefit from the already-fetched results.
if (uncachedTags.length > 0) {
const tagResults = await Promise.all(
uncachedTags.map((tag) => this.kv.get(this.prefix + TAG_PREFIX + tag)),
);

// Populate cache for all results first, then check for invalidation.
// Two-loop structure ensures all tag results are cached even when an
// earlier tag would cause an early return — so subsequent get() calls
// for entries sharing those tags don't redundantly re-fetch from KV.
for (let i = 0; i < uncachedTags.length; i++) {
const tagTime = tagResults[i];
const tagTimestamp = tagTime ? Number(tagTime) : 0;
this._tagCache.set(uncachedTags[i], { timestamp: tagTimestamp, fetchedAt: now });
}

// Then check for invalidation using the now-cached timestamps
for (const tag of uncachedTags) {
const cached = this._tagCache.get(tag)!;
if (cached.timestamp !== 0) {
if (Number.isNaN(cached.timestamp) || cached.timestamp >= entry.lastModified) {
this._deleteInBackground(kvKey);
return null;
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: early return discards already-fetched tag results.

The Promise.all on line 171 already resolved all tag fetches, but when an invalidating tag is found at index i, tags at indices i+1..n are never written to _tagCache. Those KV round-trips already happened — their results should be cached so subsequent get() calls don't re-fetch them.

Move the cache-population loop before the invalidation check, or populate all results before iterating for invalidation:

Suggested change
return null;
// Populate local cache (0 for missing tags, NaN for corrupted)
this._tagCache.set(uncachedTags[i], { timestamp: tagTimestamp, fetchedAt: now });
}
// Check invalidation after populating cache for all fetched tags
for (let i = 0; i < uncachedTags.length; i++) {
const tagTime = tagResults[i];
const tagTimestamp = tagTime ? Number(tagTime) : 0;
if (tagTime) {
if (Number.isNaN(tagTimestamp) || tagTimestamp >= entry.lastModified) {
this._deleteInBackground(kvKey);
return null;
}
}

Alternatively, a simpler refactor: split into two loops — one to populate cache for all results, one to check invalidation.

}
}
}
}
}
Expand Down Expand Up @@ -246,10 +294,29 @@ export class KVCacheHandler implements CacheHandler {
}),
),
);
// Update local tag cache immediately so invalidations are reflected
// without waiting for the TTL to expire
for (const tag of validTags) {
this._tagCache.set(tag, { timestamp: now, fetchedAt: now });
}
}

Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this resetRequestCache() is meant to provide per-request isolation, it currently isn't called anywhere in this repo (only defined). Either wire it into the request lifecycle (where the cache handler is used) or clarify that _tagCache is intentionally cross-request; otherwise it’s easy to assume isolation that isn’t actually happening.

Suggested change
/**
* Manually clear the in-memory tag cache for this KVCacheHandler instance.
*
* Note: KVCacheHandler instances are typically reused across multiple
* requests in a Cloudflare Worker, and `_tagCache` is intentionally shared
* across those requests as an optimization. vinext does NOT automatically
* call this method per request, so there is no implicit per-request
* isolation of tag state.
*
* This method is provided as an explicit escape hatch for environments or
* tests that need stricter isolation and are managing handler instances or
* lifecycle hooks themselves. Callers who require per-request isolation
* should either:
* - construct a fresh KVCacheHandler per request, or
* - invoke resetRequestCache() at an appropriate point in their own
* request lifecycle.
*/

Copilot uses AI. Check for mistakes.
/**
* Clear the in-memory tag cache for this KVCacheHandler instance.
*
* Note: KVCacheHandler instances are typically reused across multiple
* requests in a Cloudflare Worker. The `_tagCache` is intentionally
* cross-request — it reduces redundant KV reads for recently-seen tags
* across all requests hitting the same isolate, bounded by `tagCacheTtlMs`
* (default 5s). vinext does NOT call this method per request.
*
* This is an opt-in escape hatch for callers that need stricter isolation
* (e.g., tests, or environments with custom lifecycle management).
* Callers that require per-request isolation should either construct a
* fresh KVCacheHandler per request or invoke this method explicitly.
*/
resetRequestCache(): void {
// No-op — KV is stateless per request
this._tagCache.clear();
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resetRequestCache() is never called.

Grep confirms that nothing in packages/vinext/src/ calls .resetRequestCache() on any cache handler. The PR description says "clears the local cache for per-request isolation" but that isolation doesn't actually happen — this is dead code in production.

This isn't necessarily wrong (the 5s TTL provides a staleness bound regardless), but the claim of "per-request isolation" in the PR description is misleading. The _tagCache is shared across all requests hitting the same Worker isolate.

Suggestions:

  1. Either wire resetRequestCache() into the request lifecycle (e.g., call it at the start of each request in the worker entry), or
  2. Update the docstring to clarify this is intentionally cross-request and the method is an opt-in escape hatch.

If per-request isolation isn't needed (and the 5s TTL is the intended staleness bound), option 2 is fine. But the current state is a documentation/expectation mismatch.

}

/**
Expand Down
231 changes: 231 additions & 0 deletions tests/kv-cache-handler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,237 @@ describe("KVCacheHandler", () => {
});
});

// -------------------------------------------------------------------------
// Local tag cache
// -------------------------------------------------------------------------

describe("local tag cache", () => {
it("cached tags skip KV on second get()", async () => {
Copy link
Contributor

@ask-bonk ask-bonk bot Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test coverage. One missing case worth adding: resetRequestCache() actually clears the local cache. There's no test that calls handler.resetRequestCache() and then verifies that tags are re-fetched from KV on the next get(). This would validate the only public API for clearing the cache.

Something like:

it("resetRequestCache() forces tags to be re-fetched from KV", async () => {
  // ... setup entry with tags, do first get() to populate cache ...
  kv.get.mockClear();
  handler.resetRequestCache();
  await handler.get("tagged-page");
  // Should fetch tags from KV again
  expect(kv.get).toHaveBeenCalledTimes(3); // entry + 2 tags
});

const entryTime = 1000;
store.set(
"cache:tagged-page",
JSON.stringify({
value: { kind: "PAGES", html: "<p>hi</p>", pageData: {}, status: 200 },
tags: ["t1", "t2"],
lastModified: entryTime,
revalidateAt: null,
}),
);
// No tag invalidation timestamps in KV — tags are valid

// First get() — should fetch tags from KV (cache miss in local cache)
const result1 = await handler.get("tagged-page");
expect(result1).not.toBeNull();

// kv.get calls: 1 for the entry + 2 for the tags = 3
expect(kv.get).toHaveBeenCalledTimes(3);

// Reset call counts
kv.get.mockClear();

// Second get() — tags should come from local cache, NOT from KV
const result2 = await handler.get("tagged-page");
expect(result2).not.toBeNull();

// kv.get calls: 1 for the entry only, 0 for tags
expect(kv.get).toHaveBeenCalledTimes(1);
expect(kv.get).toHaveBeenCalledWith("cache:tagged-page");
});

it("revalidateTag() updates local cache so subsequent get() skips KV for that tag", async () => {
const entryTime = 1000;

// revalidateTag sets the invalidation timestamp
await handler.revalidateTag("t1");

kv.get.mockClear();

// Now store an entry with tag t1 that was created BEFORE the invalidation
store.set(
"cache:rt-page",
JSON.stringify({
value: { kind: "PAGES", html: "<p>old</p>", pageData: {}, status: 200 },
tags: ["t1"],
lastModified: entryTime,
revalidateAt: null,
}),
);

// get() should see tag t1 is invalidated via local cache — no KV GET for __tag:t1
const result = await handler.get("rt-page");
expect(result).toBeNull(); // invalidated

// kv.get: 1 for entry, 0 for tags (t1 was in local cache)
expect(kv.get).toHaveBeenCalledTimes(1);
expect(kv.get).toHaveBeenCalledWith("cache:rt-page");
});

it("TTL expiry triggers fresh KV fetch", async () => {
// Use tagCacheTtlMs: 0 so entries expire immediately — no fake timers needed.
const shortTtlHandler = new KVCacheHandler(kv as any, { tagCacheTtlMs: 0 });

const entryTime = 1000;
store.set(
"cache:ttl-page",
JSON.stringify({
value: { kind: "PAGES", html: "<p>hi</p>", pageData: {}, status: 200 },
tags: ["t1"],
lastModified: entryTime,
revalidateAt: null,
}),
);

// First get() — populates local tag cache (entry + tag = 2 calls)
await shortTtlHandler.get("ttl-page");
expect(kv.get).toHaveBeenCalledTimes(2);
kv.get.mockClear();

// Second get() — TTL is 0ms so entry is already expired; must re-fetch tag from KV
await shortTtlHandler.get("ttl-page");
expect(kv.get).toHaveBeenCalledTimes(2); // entry + tag again
});

it("tag invalidation works end-to-end with local cache", async () => {
const entryTime = 1000;
store.set(
"cache:e2e-page",
JSON.stringify({
value: { kind: "PAGES", html: "<p>original</p>", pageData: {}, status: 200 },
tags: ["t1"],
lastModified: entryTime,
revalidateAt: null,
}),
);

// First get() succeeds (no invalidation yet)
const result1 = await handler.get("e2e-page");
expect(result1).not.toBeNull();

// Now invalidate tag t1
await handler.revalidateTag("t1");

// get() should return null (cache miss due to tag invalidation)
const result2 = await handler.get("e2e-page");
expect(result2).toBeNull();
});

it("uncached tags are still fetched from KV", async () => {
const entryTime = 1000;

// Store entry with two tags
store.set(
"cache:partial-page",
JSON.stringify({
value: { kind: "PAGES", html: "<p>hi</p>", pageData: {}, status: 200 },
tags: ["t1", "t2"],
lastModified: entryTime,
revalidateAt: null,
}),
);

// First get() populates local cache for both t1 and t2
await handler.get("partial-page");
kv.get.mockClear();

// Now add a DIFFERENT entry that shares t1 but also has t3 (not yet cached)
store.set(
"cache:partial-page2",
JSON.stringify({
value: { kind: "PAGES", html: "<p>other</p>", pageData: {}, status: 200 },
tags: ["t1", "t3"],
lastModified: entryTime,
revalidateAt: null,
}),
);

const result = await handler.get("partial-page2");
expect(result).not.toBeNull();

// kv.get: 1 for entry + 1 for t3 (t1 was cached). NOT 2 for tags.
expect(kv.get).toHaveBeenCalledTimes(2);
// Verify the calls are for the entry and t3 only
expect(kv.get).toHaveBeenCalledWith("cache:partial-page2");
expect(kv.get).toHaveBeenCalledWith("__tag:t3");
});

it("NaN tag timestamp in local cache treated as invalidation", async () => {
const entryTime = 1000;

// Put a non-numeric tag value in KV
store.set("__tag:bad-tag", "not-a-number");

store.set(
"cache:nan-page",
JSON.stringify({
value: { kind: "PAGES", html: "<p>hi</p>", pageData: {}, status: 200 },
tags: ["bad-tag"],
lastModified: entryTime,
revalidateAt: null,
}),
);

// First get() — fetches from KV, gets NaN, caches it, returns null
const result1 = await handler.get("nan-page");
expect(result1).toBeNull();

kv.get.mockClear();

// Re-store the entry (it was deleted by the first get)
store.set(
"cache:nan-page",
JSON.stringify({
value: { kind: "PAGES", html: "<p>hi</p>", pageData: {}, status: 200 },
tags: ["bad-tag"],
lastModified: entryTime,
revalidateAt: null,
}),
);

// Second get() — NaN is in local cache, should still treat as invalidation
const result2 = await handler.get("nan-page");
expect(result2).toBeNull();

// kv.get: 1 for entry, 0 for tag (NaN was cached locally)
expect(kv.get).toHaveBeenCalledTimes(1);
});

it("resetRequestCache() forces tags to be re-fetched from KV", async () => {
const entryTime = 1000;
store.set(
"cache:reset-page",
JSON.stringify({
value: { kind: "PAGES", html: "<p>hi</p>", pageData: {}, status: 200 },
tags: ["t1", "t2"],
lastModified: entryTime,
revalidateAt: null,
}),
);

// First get() — populates local tag cache (1 entry + 2 tags = 3 calls)
const result1 = await handler.get("reset-page");
expect(result1).not.toBeNull();
expect(kv.get).toHaveBeenCalledTimes(3);
kv.get.mockClear();

// Second get() without reset — tags served from local cache (1 entry only)
const result2 = await handler.get("reset-page");
expect(result2).not.toBeNull();
expect(kv.get).toHaveBeenCalledTimes(1);
kv.get.mockClear();

// Clear the local cache
handler.resetRequestCache();

// Third get() after reset — tags must be re-fetched from KV (1 entry + 2 tags = 3 calls)
const result3 = await handler.get("reset-page");
expect(result3).not.toBeNull();
expect(kv.get).toHaveBeenCalledTimes(3);
expect(kv.get).toHaveBeenCalledWith("cache:reset-page");
expect(kv.get).toHaveBeenCalledWith("__tag:t1");
expect(kv.get).toHaveBeenCalledWith("__tag:t2");
});
});

// -------------------------------------------------------------------------
// STALE → regen → HIT lifecycle
//
Expand Down
Loading