fix(registry): route skill detail cache writes through writeCache

nuxt.config.ts

@@ -78,6 +78,7 @@ export default defineNuxtConfig({
     fields: [
       'attempt',
       'batch.count',
+      'cache.key',
       'cache.readFailed',
       'cache.writeFailed',
       'eligible.count',

scripts/check-published-cli-grammar.ts

@@ -109,6 +109,15 @@ async function publishedVersion(tag: ChannelRequirement['tag']): Promise<string>
   return version
 }
 
+/**
+ * Runs a published package through `pnpm dlx` rather than `npx`.
+ *
+ * `npx` downloads into a per-container npm cache, so every ephemeral CI
+ * container pays the full download over the shared uplink and a slow one
+ * overruns the 180s kill and reads as a grammar failure. `pnpm dlx` resolves
+ * through the shared pnpm store the setup action mounts, so only the first
+ * container ever downloads a version.
+ */
 export function publishedCliInvocation(version: string, subcommand?: string): PublishedCliInvocation {
   return {
     file: 'pnpm',
@@ -211,6 +220,11 @@ async function checkChannel(
   }
 }
 
+/**
+ * Checks one channel at a time. Each channel's first call downloads its
+ * package through the shared store, and two parallel downloads contend for
+ * the same uplink the CI box shares with every other job.
+ */
 export async function runPublishedCliGrammar(
   dependencies: PublishedCliGrammarDependencies,
 ): Promise<PublishedCliGrammarResult> {

shared/server/cache.ts

@@ -29,6 +29,115 @@ export async function writeCache<T>(
   }
 }
 
+/**
+ * What {@link cached} stores for one key: the computed value plus the epoch
+ * seconds it was computed at. Freshness is decided from `t`, so the storage
+ * TTL only has to carry the entry through the whole fresh + stale window.
+ */
+interface SwrEntry<T> {
+  v: T
+  t: number
+}
+
+function isSwrEntry<T>(value: unknown): value is SwrEntry<T> {
+  return typeof value === 'object' && value !== null
+    && 'v' in value && 't' in value
+    && typeof (value as SwrEntry<T>).t === 'number'
+}
+
+export interface CachedOptions<T> {
+  storage: ReadThroughCache
+  key: string
+  /** Seconds a stored value is served as-is. */
+  ttlSeconds: number
+  /** Extra seconds a stale value is served while one refresh recomputes. Zero means the entry is dead at TTL expiry. */
+  staleSeconds?: number
+  /** The full computation a cache miss must run. */
+  compute: () => Promise<T>
+  /**
+   * Runs the background refresh promise when a stale value is served.
+   * Pass the event's `waitUntil` wrapper so Workers keeps the refresh alive;
+   * the default drops it to fire-and-forget.
+   */
+  schedule?: (promise: Promise<unknown>) => void
+  /** Epoch seconds clock. */
+  now?: () => number
+}
+
+/**
+ * One in-flight computation per cache key.
+ *
+ * A bare read-through cache turns every TTL expiry into a thundering herd:
+ * after the entry dies, each concurrent request re-runs the whole compute
+ * until the first write lands, and on these routes a compute is about six D1
+ * queries plus a possible live GitHub render. The ops triage ledger
+ * attributes recurring D1 overload bursts (Sentry SKILLD-G/H/J/K/M/N/P/Q) to
+ * exactly that shape, and Nitro's `defineCachedEventHandler` had the same
+ * hole covered by `swr: true` + `staleMaxAge` before the detail route left it
+ * (Sentry SKILLD-1V). This helper restores both halves without the bare
+ * `setItem` that made a KV 429 a 500:
+ *
+ * - fresh (`age < ttlSeconds`): serve the stored value.
+ * - stale (`age < ttlSeconds + staleSeconds`): serve the stored value and
+ *   refresh in the background through `schedule`.
+ * - dead or absent: recompute, awaited.
+ *
+ * All three paths funnel into one shared promise per key, so concurrent
+ * callers share a single computation, a single D1 pass, and a single KV
+ * write, which also keeps the write under KV's one-write-per-second limit.
+ * The map is per isolate, so the guarantee is per isolate; that is still the
+ * difference between N computes and one.
+ */
+const inflightComputes = new Map<string, Promise<unknown>>()
+
+export function cached<T>(options: CachedOptions<T>): Promise<T> {
+  const { key, compute } = options
+  const freshSeconds = options.ttlSeconds
+  const maxAgeSeconds = freshSeconds + (options.staleSeconds ?? 0)
+  const nowSeconds = options.now ?? (() => Math.floor(Date.now() / 1000))
+
+  function computeOnce(): Promise<T> {
+    const pending = inflightComputes.get(key)
+    if (pending)
+      return pending as Promise<T>
+    const promise = (async () => {
+      const value = await compute()
+      await writeCache(options.storage, key, { v: value, t: nowSeconds() } satisfies SwrEntry<T>, { ttl: maxAgeSeconds })
+      return value
+    })()
+    inflightComputes.set(key, promise)
+    // Both callbacks return normally, so this derived promise never rejects
+    // and the original rejection still reaches every caller.
+    void promise.then(() => inflightComputes.delete(key), () => inflightComputes.delete(key))
+    return promise
+  }
+
+  return (async () => {
+    const entry = await readCache<SwrEntry<T>>(options.storage, key)
+    if (isSwrEntry<T>(entry)) {
+      const age = nowSeconds() - entry.t
+      if (age < freshSeconds)
+        return entry.v
+      if (age < maxAgeSeconds) {
+        const refresh = computeOnce().catch((error: unknown) => {
+          emitOperationalEvent(createWideEvent({
+            'operation': 'swr-refresh',
+            'outcome': 'failed',
+            'cache.key': key,
+            'reason': error instanceof Error ? error.message : String(error),
+          }))
+        })
+        if (options.schedule)
+          options.schedule(refresh)
+        else
+          void refresh
+        return entry.v
+      }
+    }
+    return computeOnce()
+  })()
+}
+
 /**
  * The read half of unstorage's `Storage`, structurally typed for the same
  * reason as {@link CacheStorage}.

test/unit/cache-write-best-effort.test.ts

@@ -1,7 +1,10 @@
+import type { H3Event } from 'h3'
+import type { SqliteD1 } from './helpers/d1-sqlite'
 import { readdirSync, readFileSync, statSync } from 'node:fs'
 import { join } from 'node:path'
-import { describe, expect, it, vi } from 'vitest'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
 import { readCache, writeCache } from '../../shared/server/cache'
+import { allMigrations, createSqliteD1 } from './helpers/d1-sqlite'
 
 function sourceFiles(directory: string): string[] {
   const entries = readdirSync(directory)
@@ -103,3 +106,73 @@ describe('best-effort cache reads', () => {
     expect(offenders).toEqual([])
   })
 })
+
+describe('skill detail route cache', () => {
+  const NOW_SEC = Math.floor(Date.now() / 1000)
+  const slug = 'ericzakariasson/scandinavian-design/alpha'
+  let harness: SqliteD1
+  let handler: (event: H3Event) => Promise<Record<string, unknown>>
+
+  // Nitro's cached handler used to guard this route and awaited a bare
+  // storage.setItem, so a KV PUT rejection travelled out of the handler as a
+  // 500 (Sentry SKILLD-1V). The route now caches through `cached`, whose
+  // writes go through the best-effort `writeCache`; this proves the old
+  // failure mode stays dead.
+  vi.stubGlobal('defineEventHandler', (handler: unknown) => handler)
+  vi.stubGlobal('createError', (input: Record<string, unknown>) => Object.assign(new Error(String(input.message)), input))
+  vi.stubGlobal('getUserSession', () => Promise.resolve(null))
+  vi.stubGlobal('getRouterParam', (_event: unknown, key: string) => (key === 'slug' ? slug : undefined))
+
+  beforeEach(async () => {
+    vi.resetModules()
+    harness = createSqliteD1(allMigrations())
+    harness.raw.prepare(`INSERT INTO repos (owner, repo) VALUES ('ericzakariasson', 'scandinavian-design')`).run()
+    const raw = `---\nname: alpha\ndescription: A alpha skill.\n---\n\nBody of alpha.\n`
+    harness.raw.prepare(
+      `INSERT INTO skills (owner, repo, name, slug, display_name, source_resolved,
+         rendered_skill_path, rendered_status, rendered_raw, rendered_html, rendered_at)
+       VALUES ('ericzakariasson', 'scandinavian-design', 'alpha', ?, 'alpha', 1,
+         'skills/alpha/SKILL.md', 'ok', ?, '<p>ok</p>', ?)`,
+    ).run(slug, raw, NOW_SEC)
+    handler = (await import('../../layers/registry/server/api/skills/[...slug].get')).default
+  })
+
+  it('serves the page even when the KV cache write is rate limited', async () => {
+    const setItem = vi.fn(async () => {
+      throw new Error('KV PUT failed: 429 Too Many Requests')
+    })
+    vi.stubGlobal('useStorage', () => ({
+      getItem: async () => null,
+      setItem,
+    }))
+
+    const body = await handler(event())
+
+    expect(body.registryPath).toBe('/gh/ericzakariasson/scandinavian-design')
+    expect(setItem).toHaveBeenCalledWith(
+      expect.stringContaining('skills:detail:'),
+      expect.objectContaining({ v: expect.objectContaining({ registryPath: '/gh/ericzakariasson/scandinavian-design' }), t: expect.any(Number) }),
+      { ttl: 360 },
+    )
+  })
+
+  it('serves the cached response on a hit without recomputing', async () => {
+    const cacheMap = new Map<string, unknown>()
+    vi.stubGlobal('useStorage', () => ({
+      getItem: async (key: string) => cacheMap.get(key) ?? null,
+      setItem: async (key: string, value: unknown) => {
+        cacheMap.set(key, value)
+      },
+    }))
+
+    const fresh = await handler(event())
+    await expect(handler(event())).resolves.toBe(fresh)
+  })
+
+  function event(): H3Event {
+    return {
+      context: { platform: { db: harness.db } },
+      node: { req: { headers: {} } },
+    } as unknown as H3Event
+  }
+})

test/unit/published-cli-grammar.test.ts

@@ -1,3 +1,4 @@
+// @vitest-environment node
 import { describe, expect, it } from 'vitest'
 import {
   publishedCliInvocation,

test/unit/skill-detail-swr.test.ts

@@ -0,0 +1,114 @@
+import type { H3Event } from 'h3'
+import type { SqliteD1 } from './helpers/d1-sqlite'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+import { allMigrations, createSqliteD1 } from './helpers/d1-sqlite'
+
+// The skill detail route lost Nitro's stale-while-revalidate shield when it
+// moved to a manual KV read-through cache (Sentry SKILLD-1V fix). Without
+// SWR or single-flight, every concurrent request that lands after the 60s
+// TTL expires re-runs the full handler, about six D1 queries plus a possible
+// live GitHub render, and the ops triage ledger attributes recurring D1
+// overload bursts (SKILLD-G/H/J/K/M/N/P/Q) to exactly this shape on these
+// routes. These tests pin the replacement behaviour: one computation per key
+// at a time, with stale entries served while a single refresh recomputes.
+describe('skill detail SWR cache', () => {
+  const NOW_SEC = Math.floor(Date.now() / 1000)
+  const slug = 'ericzakariasson/scandinavian-design/alpha'
+  const cacheKey = `skills:detail:v1:${slug}`
+  let harness: SqliteD1
+  let handler: (event: H3Event) => Promise<Record<string, unknown>>
+  let prepareCalls: number
+
+  vi.stubGlobal('defineEventHandler', (handler: unknown) => handler)
+  vi.stubGlobal('createError', (input: Record<string, unknown>) => Object.assign(new Error(String(input.message)), input))
+  vi.stubGlobal('getUserSession', () => Promise.resolve(null))
+  vi.stubGlobal('getRouterParam', (_event: unknown, key: string) => (key === 'slug' ? slug : undefined))
+
+  beforeEach(async () => {
+    vi.resetModules()
+    harness = createSqliteD1(allMigrations())
+    prepareCalls = 0
+    const db = harness.db as unknown as { prepare: (sql: string) => unknown }
+    const realPrepare = db.prepare.bind(db)
+    db.prepare = (sql: string) => {
+      prepareCalls++
+      return realPrepare(sql)
+    }
+
+    harness.raw.prepare(`INSERT INTO repos (owner, repo) VALUES ('ericzakariasson', 'scandinavian-design')`).run()
+    const raw = `---\nname: alpha\ndescription: A alpha skill.\n---\n\nBody of alpha.\n`
+    harness.raw.prepare(
+      `INSERT INTO skills (owner, repo, name, slug, display_name, source_resolved,
+         rendered_skill_path, rendered_status, rendered_raw, rendered_html, rendered_at)
+       VALUES ('ericzakariasson', 'scandinavian-design', 'alpha', ?, 'alpha', 1,
+         'skills/alpha/SKILL.md', 'ok', ?, '<p>ok</p>', ?)`,
+    ).run(slug, raw, NOW_SEC)
+    handler = (await import('../../layers/registry/server/api/skills/[...slug].get')).default
+  })
+
+  it('shares one computation between concurrent requests when the entry has expired', async () => {
+    // The 60s TTL has passed: KV answers nothing, every request is a miss.
+    const setItem = vi.fn(async () => {})
+    vi.stubGlobal('useStorage', () => ({
+      getItem: async () => null,
+      setItem,
+    }))
+    const detailWrites = () => setItem.mock.calls.filter(([key]) => String(key).startsWith('skills:detail:')).length
+
+    await handler(event())
+    const singleRunQueries = prepareCalls
+
+    const writesBeforeBurst = detailWrites()
+    const beforeBurst = prepareCalls
+    const results = await Promise.all(Array.from({ length: 8 }, () => handler(event())))
+    const burstQueries = prepareCalls - beforeBurst
+
+    expect(new Set(results).size).toBe(1)
+    expect(burstQueries).toBeLessThanOrEqual(singleRunQueries)
+    expect(detailWrites() - writesBeforeBurst).toBe(1)
+  })
+
+  it('serves a stale entry while exactly one refresh recomputes in the background', async () => {
+    const cacheMap = new Map<string, unknown>()
+    const setItem = vi.fn(async (key: string, value: unknown) => {
+      cacheMap.set(key, value)
+    })
+    vi.stubGlobal('useStorage', () => ({
+      getItem: async (key: string) => cacheMap.get(key) ?? null,
+      setItem,
+    }))
+
+    // Control run: one full compute, stored fresh in the cache.
+    await handler(event())
+    const singleRunQueries = prepareCalls
+    const fresh = cacheMap.get(cacheKey) as { v: Record<string, unknown>, t: number }
+    expect(fresh.v.registryPath).toBe('/gh/ericzakariasson/scandinavian-design')
+
+    // Age the entry past the 60s TTL, into the 5 minute stale window.
+    const staleT = Math.floor(Date.now() / 1000) - 61
+    cacheMap.set(cacheKey, { v: fresh.v, t: staleT })
+
+    const beforeBurst = prepareCalls
+    const results = await Promise.all(Array.from({ length: 8 }, () => handler(event())))
+
+    // Every request is served the stale value immediately.
+    expect(results.every(result => result === fresh.v)).toBe(true)
+
+    // One background refresh recomputes and writes the entry back.
+    await vi.waitFor(() => {
+      const written = cacheMap.get(cacheKey) as { v: Record<string, unknown>, t: number }
+      expect(written.t).toBeGreaterThan(staleT)
+    })
+    expect(prepareCalls - beforeBurst).toBeLessThanOrEqual(singleRunQueries)
+    const refreshed = cacheMap.get(cacheKey) as { v: Record<string, unknown>, t: number }
+    expect(refreshed.v.registryPath).toBe('/gh/ericzakariasson/scandinavian-design')
+    expect(refreshed.t).toBeGreaterThanOrEqual(fresh.t)
+  })
+
+  function event(): H3Event {
+    return {
+      context: { platform: { db: harness.db } },
+      node: { req: { headers: {} } },
+    } as unknown as H3Event
+  }
+})