From e5ea63cfbff2e662dac329a476894ba3eecfd169 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Wed, 19 Aug 2026 13:10:11 +1000 Subject: [PATCH] fix(registry): stop a KV outage 500ing every /gh page The gone-skill middleware read its key set with a bare `useStorage('cache').getItem`, so `KV GET failed: 500 Internal Server Error` travelled out of the read-through cache and failed the request (Sentry SKILLD-S). The D1 fallback behind it could fail the same way. `resolveGoneSkillKeys` answers null when neither store can be reached, and the middleware leaves the response status alone. A middleware whose only job is upgrading 200 to 410 should never be why a page fails. --- server/middleware/skill-source-gone.ts | 19 +++----- server/utils/source-gone-skills.ts | 44 +++++++++++++++++ shared/server/cache.ts | 7 ++- test/unit/source-gone-skills.test.ts | 66 +++++++++++++++++++++++++- 4 files changed, 120 insertions(+), 16 deletions(-) diff --git a/server/middleware/skill-source-gone.ts b/server/middleware/skill-source-gone.ts index 0925edf1..683ec78a 100644 --- a/server/middleware/skill-source-gone.ts +++ b/server/middleware/skill-source-gone.ts @@ -1,8 +1,5 @@ -import { writeCache } from '#shared/server/cache' import { - GONE_SKILLS_CACHE_KEY, - GONE_SKILLS_CACHE_TTL, - selectGoneSkillKeys, + resolveGoneSkillKeys, skillKeyFromPath, } from '../utils/source-gone-skills' @@ -18,6 +15,10 @@ import { * The key set is cached because this runs on every `/gh` request, and an * uncached per-request D1 read on a hot path is exactly what produced the * 2026-08-04 overload burst. One query an hour returns about 130 rows. + * + * `resolveGoneSkillKeys` answers null when it cannot reach either store, and + * that leaves the response status alone. A middleware that only ever upgrades a + * status must never be the reason a page fails. */ export default defineEventHandler(async (event) => { const key = skillKeyFromPath(getRequestURL(event).pathname) @@ -28,13 +29,7 @@ export default defineEventHandler(async (event) => { if (!db) return - const cache = useStorage('cache') - let keys = await cache.getItem(GONE_SKILLS_CACHE_KEY) - if (!Array.isArray(keys)) { - keys = await selectGoneSkillKeys(db) - await writeCache(cache, GONE_SKILLS_CACHE_KEY, keys, { ttl: GONE_SKILLS_CACHE_TTL }) - } - - if (keys.includes(key)) + const keys = await resolveGoneSkillKeys(useStorage('cache'), db) + if (keys?.includes(key)) setResponseStatus(event, 410) }) diff --git a/server/utils/source-gone-skills.ts b/server/utils/source-gone-skills.ts index 12a888b4..0efff9d1 100644 --- a/server/utils/source-gone-skills.ts +++ b/server/utils/source-gone-skills.ts @@ -1,5 +1,8 @@ /// +import type { ReadThroughCache } from '#shared/server/cache' +import { readCache, writeCache } from '#shared/server/cache' + /** * Skills whose SKILL.md was deleted upstream still serve a page, because the * cached render survives the deletion. The page is worth keeping: it shows the @@ -79,3 +82,44 @@ export async function selectGoneSkillKeys(db: D1Database): Promise { const rows = await db.prepare(GONE_SKILLS_SQL).bind().all() return goneSkillKeys(rows.results ?? []) } + +/** + * The gone-skill key set for this request, or null when it cannot be resolved. + * + * This runs on every `/gh` request and its only job is to upgrade a 200 to a + * 410. Neither the KV read nor the D1 fallback is worth the page. Workers KV + * answered a `getItem` with `KV GET failed: 500 Internal Server Error` and the + * rejection travelled out of the bare read-through cache here and 500'd the + * page (Sentry SKILLD-S, `/gh/dylantarre/animation-principles/educator-teacher`, + * still live 2026-08-18). The D1 read behind it fails the same way, and D1 + * answers `{"D1_RESET_DO":true}` on its own schedule (Sentry SKILLD-Y). + * + * Failing to resolve the set costs one stale 200 on a deleted skill until the + * next request. Propagating the failure costs every `/gh` visitor the page. + * + * This is deliberately not a silent catch: the wide event carries the reason, + * so an outage shows up as a spike in unresolved lookups rather than as + * nothing at all. + */ +export async function resolveGoneSkillKeys( + cache: ReadThroughCache, + db: D1Database, +): Promise { + const cached = await readCache(cache, GONE_SKILLS_CACHE_KEY) + if (Array.isArray(cached)) + return cached + + try { + const keys = await selectGoneSkillKeys(db) + await writeCache(cache, GONE_SKILLS_CACHE_KEY, keys, { ttl: GONE_SKILLS_CACHE_TTL }) + return keys + } + catch (error) { + emitOperationalEvent(createWideEvent({ + operation: 'source-gone-keys', + outcome: 'failed', + reason: error instanceof Error ? error.message : String(error), + })) + return null + } +} diff --git a/shared/server/cache.ts b/shared/server/cache.ts index ba36f721..25baf042 100644 --- a/shared/server/cache.ts +++ b/shared/server/cache.ts @@ -2,7 +2,7 @@ * The write half of unstorage's `Storage`, structurally typed so this module * does not depend on unstorage's own types being resolvable. */ -interface CacheStorage { +export interface CacheStorage { setItem: (key: string, value: never, options?: { ttl?: number }) => Promise } @@ -33,10 +33,13 @@ export async function writeCache( * The read half of unstorage's `Storage`, structurally typed for the same * reason as {@link CacheStorage}. */ -interface CacheReadStorage { +export interface CacheReadStorage { getItem: (key: string) => Promise } +/** A cache a read-through helper both reads and writes. */ +export type ReadThroughCache = CacheReadStorage & CacheStorage + /** * Read from the KV-backed cache without letting the read decide whether the * request succeeds. diff --git a/test/unit/source-gone-skills.test.ts b/test/unit/source-gone-skills.test.ts index 266e60fd..353c377a 100644 --- a/test/unit/source-gone-skills.test.ts +++ b/test/unit/source-gone-skills.test.ts @@ -1,5 +1,5 @@ -import { describe, expect, it } from 'vitest' -import { goneSkillKeys, skillKeyFromPath } from '../../server/utils/source-gone-skills' +import { describe, expect, it, vi } from 'vitest' +import { goneSkillKeys, resolveGoneSkillKeys, skillKeyFromPath } from '../../server/utils/source-gone-skills' const microsoft = { owner: 'microsoft', @@ -77,3 +77,65 @@ describe('goneSkillKeys', () => { expect(goneSkillKeys([])).toEqual([]) }) }) + +describe('resolveGoneSkillKeys', () => { + const goneRow = { + owner: 'microsoft', + repo: 'skills', + name: 'entra-app-registration', + slug: null, + name_collides_with_repo: 0, + } + + function stubDb(all: () => Promise<{ results: typeof goneRow[] }>) { + return { + prepare: () => ({ bind: () => ({ all }) }), + } as unknown as Parameters[1] + } + + it('returns the cached key set without touching the database', async () => { + const all = vi.fn() + const keys = await resolveGoneSkillKeys( + { getItem: async () => ['acme/skills/deploy'], setItem: async () => {} }, + stubDb(all as never), + ) + + expect(keys).toEqual(['acme/skills/deploy']) + expect(all).not.toHaveBeenCalled() + }) + + it('treats a KV read failure as a miss and answers from the database', async () => { + // Sentry SKILLD-S: `KV GET failed: 500 Internal Server Error` travelled out + // of this read-through cache and 500'd every `/gh` page. + const keys = await resolveGoneSkillKeys( + { + getItem: async () => { throw new Error('KV GET failed: 500 Internal Server Error') }, + setItem: async () => {}, + }, + stubDb(async () => ({ results: [goneRow] })), + ) + + expect(keys).toEqual(['microsoft/skills/entra-app-registration']) + }) + + it('answers null when the database read fails, rather than failing the page', async () => { + const keys = await resolveGoneSkillKeys( + { getItem: async () => null, setItem: async () => {} }, + stubDb(async () => { throw new Error('D1_ERROR: {"D1_RESET_DO":true}') }), + ) + + expect(keys).toBeNull() + }) + + it('still answers when the cache write fails after a database read', async () => { + const keys = await resolveGoneSkillKeys( + { + getItem: async () => null, + setItem: async () => { throw new Error('KV PUT failed: 429 Too Many Requests') }, + }, + stubDb(async () => ({ results: [goneRow] })), + ) + + expect(keys).toEqual(['microsoft/skills/entra-app-registration']) + }) +})