From 398d992059f71a924c21416e151555d4f577e2c5 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Tue, 22 Sep 2026 19:41:56 +1000 Subject: [PATCH] perf(registry): key the related-skills cache on the request slug The slug lookup ran ahead of the cache, so every request cost one or two D1 reads even on a hit. A crawler walking 14,100 Skill pages paid that read 14,100 times an hour however warm the cache was. On 2026-09-22 that floor saturated D1. Sentry recorded 3,730 `D1 DB is overloaded` events against this route, and artifact delivery, which shares the database, failed 100 resolutions with SERVICE_UNAVAILABLE after burning a full 15-minute retry budget. Caching the response was tried twice before, on 2026-08-04 and in SKILLD-1F, and could not help while the lookup sat outside it. Move findSkill inside the cached function and key on the slug, so a hit costs no D1 read. Cache the unresolved slug as a tagged `missing` variant on a short window, because a crawler sweep over dead slugs used to cost two reads each and cache nothing. Bump the key to v4: a v3 value is an untagged raw response and must never be read as a tagged entry. --- .../server/api/skill-related/[...slug].get.ts | 38 +++++++--- layers/registry/server/utils/skill-related.ts | 69 ++++++++++++++++--- test/unit/skill-related-cache.test.ts | 57 +++++++++++++-- 3 files changed, 140 insertions(+), 24 deletions(-) diff --git a/layers/registry/server/api/skill-related/[...slug].get.ts b/layers/registry/server/api/skill-related/[...slug].get.ts index a37473ab..a8bf0d0c 100644 --- a/layers/registry/server/api/skill-related/[...slug].get.ts +++ b/layers/registry/server/api/skill-related/[...slug].get.ts @@ -1,12 +1,19 @@ import type { H3Event } from 'h3' import type { EmbeddingNeighbor } from '../../jobs/generate-embeddings' import type { CoOccurrenceNeighbor } from '../../utils/skill-co-occurrence' +import type { CachedRelated } from '../../utils/skill-related' import { readCache, readThroughCache, writeCache } from '#shared/server/cache' import { defineApiHandler } from '#shared/server/handler' import { getEmbeddingNeighbors } from '../../jobs/generate-embeddings' import { getCoOccurrenceNeighbors } from '../../utils/skill-co-occurrence' import { findSkillCommitSource as resolveRepoSourceIdentityForCommits } from '../../utils/skill-commit-source' -import { RELATED_CACHE_STALE_TTL, RELATED_CACHE_TTL, relatedCacheKey } from '../../utils/skill-related' +import { + isCachedRelated, + RELATED_CACHE_STALE_TTL, + RELATED_CACHE_TTL, + relatedCacheKey, + relatedCacheWindows, +} from '../../utils/skill-related' import { findRelatedSkills, findSkill, findSkillsByLookups } from '../../utils/skills-registry' const COMMITS_CACHE_TTL = 60 * 60 * 12 @@ -50,14 +57,17 @@ export default defineApiHandler({ if (!slug) throw createError({ statusCode: 400, message: 'Missing skill slug' }) - const skill = await findSkill(event, slug) - if (!skill) - throw createError({ statusCode: 404, message: 'Skill not found' }) - - return readThroughCache( + // The slug lookup sits inside the cached function on purpose. It is one + // to two D1 reads, and running it ahead of the cache made every request + // cost a read even on a hit. See the note in `skill-related.ts`. + const cached = await readThroughCache>( useStorage('cache'), - relatedCacheKey(skill), + relatedCacheKey(slug), async () => { + const skill = await findSkill(event, slug) + if (!skill) + return { _tag: 'missing' as const } + const source = await resolveRepoSourceIdentityForCommits(platform.db, skill) const [commits, related, coOccurrenceNeighbors, embeddingNeighbors] = await Promise.all([ @@ -83,10 +93,20 @@ export default defineApiHandler({ coOccurrenceSkills, semanticSiblings, } - return response + return { _tag: 'found' as const, response } + }, + { + ttl: RELATED_CACHE_TTL, + staleTtl: RELATED_CACHE_STALE_TTL, + windowsFor: relatedCacheWindows, + validate: isCachedRelated, }, - { ttl: RELATED_CACHE_TTL, staleTtl: RELATED_CACHE_STALE_TTL }, ) + + if (cached._tag === 'missing') + throw createError({ statusCode: 404, message: 'Skill not found' }) + + return cached.response }, }) diff --git a/layers/registry/server/utils/skill-related.ts b/layers/registry/server/utils/skill-related.ts index 20b89055..8a496702 100644 --- a/layers/registry/server/utils/skill-related.ts +++ b/layers/registry/server/utils/skill-related.ts @@ -1,5 +1,5 @@ /** - * The related-skills response is cached whole, not just its commits. + * The related-skills response is cached whole, and keyed by the request slug. * * Every uncached request costs roughly six D1 reads: skill, source identity, * same-repo and same-owner relations, co-occurrence, and neighbor hydration. @@ -13,6 +13,15 @@ * average, so crawler sweeps saturated D1 before this cache could warm. The * response already carries same-repo, same-owner, and co-occurrence results; * semantic siblings now stay empty until Vectorize has evidence. + * + * The key is the slug because the lookup that turns a slug into a Skill is + * itself one or two D1 reads, and it used to run ahead of the cache on every + * request. A crawler walking 14,100 Skill pages therefore paid that read + * 14,100 times an hour no matter how warm the cache was. On 2026-09-22 that + * floor saturated D1 again and starved artifact delivery, which shares the + * database: 100 resolutions failed `SERVICE_UNAVAILABLE` after burning a full + * 15-minute retry budget against an overloaded D1. Keying on the slug puts the + * lookup behind the cache, so a hit costs no D1 read at all. */ export const RELATED_CACHE_TTL = 60 * 60 /** @@ -22,12 +31,56 @@ export const RELATED_CACHE_TTL = 60 * 60 * beats 500ing the endpoint. */ export const RELATED_CACHE_STALE_TTL = 60 * 60 * 24 -// v3: entries carry a freshness envelope for readThroughCache, so v2 values -// (raw responses) must never be read as envelopes. -const RELATED_CACHE_VERSION = 'v3' +/** + * How long a slug that resolved to no Skill stays cached. + * + * A crawler sweep over dead slugs used to cost two D1 reads each and cache + * nothing, so the cheapest request on the site was also the one that repeated + * forever. This window is short because a Skill that appears in the registry + * should become visible in minutes, not in an hour. + */ +export const RELATED_MISSING_TTL = 10 * 60 +/** Missing entries are cheap to recompute, so they carry no stale window. */ +export const RELATED_MISSING_STALE_TTL = 0 +// v4: entries are keyed by request slug and carry a `_tag`, so v3 values +// (keyed by resolved identity, untagged) must never be read as v4 entries. +const RELATED_CACHE_VERSION = 'v4' -export function relatedCacheKey( - skill: { owner: string, repo: string, name: string }, -): string { - return `skills:related:${RELATED_CACHE_VERSION}:${skill.owner}/${skill.repo}/${skill.name}` +/** + * A cached related-skills lookup. `missing` is a real, cacheable answer, not + * an error: it says this slug resolved to no Skill, which is the single most + * common thing a crawler asks for. + */ +export type CachedRelated + = | { _tag: 'found', response: T } + | { _tag: 'missing' } + +export function relatedCacheKey(slug: string): string { + return `skills:related:${RELATED_CACHE_VERSION}:${slug}` +} + +/** + * Freshness per variant. A resolved response rides the long registry window; + * a missing one rechecks soon, so a newly indexed Skill is not hidden for an + * hour by the miss that preceded it. + */ +export function relatedCacheWindows( + value: CachedRelated, +): { ttl: number, staleTtl: number } { + return value._tag === 'missing' + ? { ttl: RELATED_MISSING_TTL, staleTtl: RELATED_MISSING_STALE_TTL } + : { ttl: RELATED_CACHE_TTL, staleTtl: RELATED_CACHE_STALE_TTL } +} + +/** + * Shape guard for KV bytes. A v3 value is an untagged raw response, so it + * fails here and is recomputed rather than served as an envelope. + */ +export function isCachedRelated(value: unknown): value is CachedRelated { + if (typeof value !== 'object' || value === null) + return false + const tag = (value as { _tag?: unknown })._tag + if (tag === 'missing') + return true + return tag === 'found' && 'response' in value } diff --git a/test/unit/skill-related-cache.test.ts b/test/unit/skill-related-cache.test.ts index 18cdfdd9..c3c24b84 100644 --- a/test/unit/skill-related-cache.test.ts +++ b/test/unit/skill-related-cache.test.ts @@ -1,15 +1,58 @@ import { describe, expect, it } from 'vitest' -import { relatedCacheKey } from '../../layers/registry/server/utils/skill-related' +import { + isCachedRelated, + RELATED_CACHE_TTL, + RELATED_MISSING_TTL, + relatedCacheKey, + relatedCacheWindows, +} from '../../layers/registry/server/utils/skill-related' describe('skill-related cache key', () => { - it('is scoped to one skill identity', () => { - expect(relatedCacheKey({ owner: 'kotlin', repo: 'kotlin-agent-skills', name: 'jpa' })) - .toBe('skills:related:v3:kotlin/kotlin-agent-skills/jpa') + it('is scoped to the request slug', () => { + expect(relatedCacheKey('kotlin/kotlin-agent-skills/jpa')) + .toBe('skills:related:v4:kotlin/kotlin-agent-skills/jpa') }) it('separates skills sharing a name across repos', () => { - const a = relatedCacheKey({ owner: 'richtabor', repo: 'agent-skills', name: 'humanize' }) - const b = relatedCacheKey({ owner: 'other', repo: 'agent-skills', name: 'humanize' }) - expect(a).not.toBe(b) + expect(relatedCacheKey('richtabor/agent-skills/humanize')) + .not + .toBe(relatedCacheKey('other/agent-skills/humanize')) + }) +}) + +describe('skill-related cache windows', () => { + it('gives a resolved response the registry window', () => { + expect(relatedCacheWindows({ _tag: 'found', response: { commits: [] } }).ttl) + .toBe(RELATED_CACHE_TTL) + }) + + it('rechecks a missing slug far sooner than a resolved one', () => { + const missing = relatedCacheWindows({ _tag: 'missing' }) + expect(missing.ttl).toBe(RELATED_MISSING_TTL) + expect(missing.ttl).toBeLessThan(RELATED_CACHE_TTL) + }) + + it('keeps no stale window for a missing slug, so a new Skill is not served as absent', () => { + expect(relatedCacheWindows({ _tag: 'missing' }).staleTtl).toBe(0) + }) +}) + +describe('cached related guard', () => { + it('accepts both tagged variants', () => { + expect(isCachedRelated({ _tag: 'missing' })).toBe(true) + expect(isCachedRelated({ _tag: 'found', response: { commits: [] } })).toBe(true) + }) + + it('rejects a v3 value, which was an untagged raw response', () => { + expect(isCachedRelated({ commits: [], relatedRepoSkills: [] })).toBe(false) + }) + + it('rejects a found entry carrying no response', () => { + expect(isCachedRelated({ _tag: 'found' })).toBe(false) + }) + + it('rejects bytes that are not an object', () => { + expect(isCachedRelated(null)).toBe(false) + expect(isCachedRelated('found')).toBe(false) }) })