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

[Divkix]

Closes #439

Summary

• Adds a local in-memory Map with 5-second TTL to KVCacheHandler that caches tag invalidation timestamps
• Eliminates redundant kv.get() calls for recently-seen tags — an entry with 20 tags no longer triggers 20 KV round-trips on every cache hit within the TTL window
• revalidateTag() updates the local cache immediately so invalidations are reflected without waiting for TTL expiry
• resetRequestCache() clears the local cache for per-request isolation

Test plan

• 6 new test cases in tests/kv-cache-handler.test.ts covering:
  • Cached tags skip KV on second get()
  • revalidateTag() updates local cache (no KV GET for invalidated tags)
  • TTL expiry triggers fresh KV fetch (fake timers)
  • Tag invalidation works end-to-end with local cache
  • Uncached tags are still fetched from KV (partial cache hit)
  • NaN tag timestamp in local cache treated as invalidation
• All 36 tests pass (31 existing + 5 new behavioral tests)

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@433

commit: 54fbf7d

[Copilot]

Pull request overview

This PR reduces Cloudflare KV round-trips on cache hits by adding a short-lived in-memory cache of tag invalidation timestamps inside KVCacheHandler, with tests to validate behavior across cache hits, TTL expiry, and tag invalidation flows.

Changes:

• Add a local in-memory tag timestamp cache with a 5s TTL to avoid repeated kv.get() calls for tags.
• Update revalidateTag() to immediately update the in-memory tag cache.
• Add a new test suite covering local tag cache hit behavior, TTL expiry, partial cache hits, and NaN/corrupt tag timestamps.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
packages/vinext/src/cloudflare/kv-cache-handler.ts	Adds local tag cache + integrates it into tag invalidation checks and revalidateTag().
tests/kv-cache-handler.test.ts	Adds tests validating local tag cache behavior, TTL refresh, and invalidation handling.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The local _tagCache is never pruned: TTL is only checked for freshness, but entries remain in the Map indefinitely unless resetRequestCache() is called. In a long-lived Worker with many unique tags, this can grow unbounded. Consider deleting stale entries when now - fetchedAt >= TAG_CACHE_TTL, and/or adding a max-size/LRU cap (or scheduled cleanup) so the cache can't accumulate forever.

[Copilot]

entry.tags is assumed to be a string array here, but validateCacheEntry() only checks Array.isArray(tags) and does not validate element types. If a corrupted/malicious KV entry contains non-string tags (e.g., objects), _tagCache will store them as Map keys (preventing GC) and KV key construction will coerce them unexpectedly. Suggest filtering entry.tags to typeof tag === 'string' and/or applying validateTag() before using/caching each tag.

[Copilot]

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]

This test enables fake timers and restores real timers at the end. If an assertion throws before vi.useRealTimers(), fake timers can leak into subsequent tests and cause cascading failures. Consider using try/finally within the test or an afterEach(() => vi.useRealTimers()) guard for this describe block.

Suggested change

	const now = 10_000;
	vi.setSystemTime(now);
	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
	await handler.get("ttl-page");
	expect(kv.get).toHaveBeenCalledTimes(2); // entry + tag
	kv.get.mockClear();
	// Second get() within TTL — uses local cache
	vi.setSystemTime(now + 4_000); // 4s < 5s TTL
	await handler.get("ttl-page");
	expect(kv.get).toHaveBeenCalledTimes(1); // entry only
	kv.get.mockClear();
	// Third get() after TTL — must re-fetch from KV
	vi.setSystemTime(now + 6_000); // 6s > 5s TTL
	await handler.get("ttl-page");
	expect(kv.get).toHaveBeenCalledTimes(2); // entry + tag
	vi.useRealTimers();
	try {
	const now = 10_000;
	vi.setSystemTime(now);
	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
	await handler.get("ttl-page");
	expect(kv.get).toHaveBeenCalledTimes(2); // entry + tag
	kv.get.mockClear();
	// Second get() within TTL — uses local cache
	vi.setSystemTime(now + 4_000); // 4s < 5s TTL
	await handler.get("ttl-page");
	expect(kv.get).toHaveBeenCalledTimes(1); // entry only
	kv.get.mockClear();
	// Third get() after TTL — must re-fetch from KV
	vi.setSystemTime(now + 6_000); // 6s > 5s TTL
	await handler.get("ttl-page");
	expect(kv.get).toHaveBeenCalledTimes(2); // entry + tag
	} finally {
	vi.useRealTimers();
	}

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Local tag cache for KV round-trip reduction

Good optimization — the two-pass approach (check local cache, then fetch uncached from KV) is clean and the test coverage is solid. The core logic is correct. I have a few issues to flag, one of which is a real bug (missed caching on early return), and the rest are hardening suggestions.

All 36 tests pass locally.

[ask-bonk]

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.

[ask-bonk]

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.

[ask-bonk]

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.

[ask-bonk]

Nit: Consider making this configurable via the constructor options (e.g., tagCacheTtlMs?: number). The 5s default is reasonable for Workers, but testing and non-Workers environments might benefit from different values. Would also eliminate the need for fake timers in the TTL test — you could just pass tagCacheTtlMs: 0.

Not a blocker.

[ask-bonk]

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.

[ask-bonk]

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
});

[ask-bonk]

Review submitted. Here's a summary of the feedback I posted on PR #433:

Review Summary

1 bug, 3 improvement suggestions, 2 nits.

Bug — Early return discards already-fetched tag results (kv-cache-handler.ts:184): When Promise.all fetches all uncached tags but an invalidating tag is found mid-loop, tags at later indices are never written to _tagCache despite the KV round-trips having already happened. Suggested splitting into two loops: one to populate cache, one to check invalidation.

Unbounded _tagCache growth (kv-cache-handler.ts:102): Stale entries are never pruned — they persist until resetRequestCache() is called, which never happens automatically. Suggested deleting stale entries when encountered in the first pass, or adding a max-size cap.

resetRequestCache() is never called (kv-cache-handler.ts:296): Nothing in the vinext request lifecycle invokes this method. The PR claims "per-request isolation" but that doesn't actually happen — the _tagCache is shared across requests in the same isolate. Suggested either wiring it into the request lifecycle or updating the docs to clarify it's intentionally cross-request.

Consider making TAG_CACHE_TTL configurable (kv-cache-handler.ts:104): Minor — would help testing and non-Workers environments.

Missing test: resetRequestCache() clears cache (kv-cache-handler.test.ts:465): No test verifies the only public API for clearing the tag cache actually forces re-fetching from KV.

github run

[james-elicx]

I did a pass over the full diff. Three things spotted and fixed in the latest commits; noting them here for the record.

---

Redundant Number(tagTime) conversion in the second pass

After the populate loop stores { timestamp: Number(tagTime) } into _tagCache, the invalidation loop was re-reading tagResults[i] and calling Number(tagTime) a second time. Fixed by reading the already-computed value from _tagCache in the check loop, which also makes the two-loop structure more obviously consistent:

// before
for (let i = 0; i < uncachedTags.length; i++) {
  const tagTime = tagResults[i];
  if (tagTime) {
    const tagTimestamp = Number(tagTime);  // computed again
    if (Number.isNaN(tagTimestamp) || tagTimestamp >= entry.lastModified) { ... }
  }
}

// after
for (const tag of uncachedTags) {
  const cached = this._tagCache.get(tag)!;  // already computed and stored
  if (cached.timestamp !== 0) {
    if (Number.isNaN(cached.timestamp) || cached.timestamp >= entry.lastModified) { ... }
  }
}

---

TTL test used fake timers unnecessarily

The test for TTL expiry used vi.useFakeTimers() + vi.setSystemTime() to simulate time passing. Now that tagCacheTtlMs is configurable (added in the previous commit), the test can pass tagCacheTtlMs: 0 to make entries expire immediately — no fake timers, no timer leak risk if an assertion throws:

// before: fake timers, try/finally, setSystemTime dance

// after
const shortTtlHandler = new KVCacheHandler(kv as any, { tagCacheTtlMs: 0 });
// first get() populates cache; second get() sees it immediately expired and re-fetches

---

No other issues found. The core logic (two-pass populate-then-check, stale entry eviction, revalidateTag updating the local cache immediately) all look correct. The resetRequestCache docstring is accurate. Test coverage is solid.

[james-elicx]

/bigbonk review

[ask-bonk]

Review: Local tag cache for KV round-trip reduction

Good optimization, well-structured after the iteration with the other reviewers. The two-pass approach (populate cache, then check invalidation) correctly avoids wasting already-completed KV fetches. The stale entry eviction on line 163 addresses unbounded growth. Test coverage is solid with 7 new tests covering the key scenarios.

The previous review issues (early return discarding fetched results, unbounded _tagCache, fake timer leakage, resetRequestCache docstring) have all been addressed in the latest commits. Code looks correct.

One minor observation below, not a blocker.

[ask-bonk]

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.

[ask-bonk]

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.

[ask-bonk]

Review submitted. Here's a summary:

Approved the PR. The optimization is well-structured and correct after the iteration with previous reviewers:

• Two-pass approach (populate all fetched tag results into local cache, then check invalidation) correctly avoids wasting KV round-trips that already completed
• Stale entry eviction (line 163: this._tagCache.delete(tag)) addresses the unbounded Map growth concern
• Configurable TTL (tagCacheTtlMs) eliminates the need for fake timers in tests
• resetRequestCache docstring accurately documents that it's an opt-in escape hatch, not automatically called per-request
• Test coverage is solid with 7 new tests covering cache hits, TTL expiry, partial cache hits, NaN handling, revalidateTag local update, end-to-end invalidation, and resetRequestCache clearing

All 40 tests pass. The previous review issues have been addressed in the latest commits. Two minor inline observations posted (not blockers).

github run