diff --git a/layers/registry/server/api/skills/[...slug].get.ts b/layers/registry/server/api/skills/[...slug].get.ts index b110db38..b17a5722 100644 --- a/layers/registry/server/api/skills/[...slug].get.ts +++ b/layers/registry/server/api/skills/[...slug].get.ts @@ -1,6 +1,9 @@ import type { H3Event } from 'h3' +import type { z } from 'zod' +import type { Platform } from '#shared/server/platform' import { LIVE_RENDER_STALE_SECONDS } from '~~/server/utils/sync-thresholds' +import { cached } from '#shared/server/cache' import { defineApiHandler } from '#shared/server/handler' import { githubSkillFileUrl } from '#shared/skill-file-url' import { selectSkillFiles } from '#shared/skill-files' @@ -32,6 +35,10 @@ interface CuratorEndorsement { const ONE_DAY_MS = 1000 * 60 * 60 * 24 +const DETAIL_CACHE_TTL = 60 +const DETAIL_CACHE_STALE_TTL = 60 * 5 +type SkillDetailPayload = z.infer + const OFFICIAL_REPO_KEYS = new Set(officialRepos.map(r => `${r.owner}/${r.repo}`)) const OFFICIAL_REPO_KIND = new Map(officialRepos.map(r => [`${r.owner}/${r.repo}`, r.kind])) @@ -183,14 +190,26 @@ const skillDetailHandler = 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 cached({ + storage: useStorage('cache'), + key: `skills:detail:v1:${slug.toLowerCase()}`, + ttlSeconds: DETAIL_CACHE_TTL, + staleSeconds: DETAIL_CACHE_STALE_TTL, + compute: () => loadSkillDetail(event, platform, slug), + schedule: promise => runAfterResponse(event, promise), + }) + }, +}) - const [curators, row, latestCommit, duplicateGroup, faqRow, tagRow, summaryRow, repoSkillRows, resolvedRepoSkillCount, ownerRow] = await Promise.all([ - Promise.resolve([] satisfies CuratorEndorsement[]), - platform.db - .prepare(`SELECT r.stars, r.forks, r.pushed_at, r.repo_created_at, r.default_branch, +async function loadSkillDetail(event: H3Event, platform: Platform, slug: string): Promise { + const skill = await findSkill(event, slug) + if (!skill) + throw createError({ statusCode: 404, message: 'Skill not found' }) + + const [curators, row, latestCommit, duplicateGroup, faqRow, tagRow, summaryRow, repoSkillRows, resolvedRepoSkillCount, ownerRow] = await Promise.all([ + Promise.resolve([] satisfies CuratorEndorsement[]), + platform.db + .prepare(`SELECT r.stars, r.forks, r.pushed_at, r.repo_created_at, r.default_branch, r.source_owner, r.source_repo, s.current_sha, s.modified_at, s.references_count, s.assets, s.last_synced_at, s.sync_status, s.source_resolved, @@ -201,266 +220,268 @@ const skillDetailHandler = defineApiHandler({ s.rendered_skill_path, s.rendered_status, s.rendered_raw, s.rendered_frontmatter, s.rendered_html, s.rendered_at FROM skills s JOIN repos r ON r.owner = s.owner AND r.repo = s.repo WHERE s.owner = ? AND s.repo = ? AND s.name = ?`) - .bind(skill.owner, skill.repo, skill.name) - .first(), - platform.db - .prepare(`SELECT sha FROM skill_revisions WHERE owner = ? AND repo = ? AND name = ? ORDER BY modified_at DESC LIMIT 1`) - .bind(skill.owner, skill.repo, skill.name) - .first<{ sha: string }>(), - findDuplicateGroupForSkill(event, `${skill.owner}/${skill.repo}/${skill.name}`), - getGenerated(platform.db, { owner: skill.owner, repo: skill.repo, name: skill.name, kind: 'faq' }), - getGenerated(platform.db, { owner: skill.owner, repo: skill.repo, name: skill.name, kind: 'tags' }), - getGenerated(platform.db, { owner: skill.owner, repo: skill.repo, name: skill.name, kind: 'summary' }), - platform.db - .prepare(`SELECT name FROM skills WHERE owner = ? AND repo = ? ORDER BY name`) - .bind(skill.owner, skill.repo) - .all(), - // Same definition as loadRepoSkillCounts (source_resolved = 1), so the - // 301 decision matches the hub routing used by feeds and sitemaps. - // repoSkillNames stays unfiltered: it feeds dependency parsing, not - // routing. - platform.db - .prepare(`SELECT COUNT(*) AS skill_count FROM skills WHERE owner = ? AND repo = ? AND source_resolved = 1`) - .bind(skill.owner, skill.repo) - .first(), - // Same `owners` row the owner page shows; the byline reuses it. - platform.db - .prepare(`SELECT name FROM owners WHERE owner = ?`) - .bind(skill.owner) - .first<{ name: string | null }>(), - ]) - - const source = resolveRepoSourceIdentityFromRow(skill, row) - const githubUrl = `https://github.com/${source.owner}/${source.repo}` - const branch = row?.default_branch || 'main' - const repoSkillNames = (repoSkillRows.results ?? []).map(candidate => candidate.name) - - // Warm path: render is in D1. Cold path (legacy rows or fetch_failed - // status): fall back to a live render so the first visit still works, - // then write back to D1. - let rendered: RenderedView - if (row?.rendered_html && row.rendered_status === 'ok') { - const reparsed = row.rendered_raw - ? await parseSkillMd(row.rendered_raw, { - owner: source.owner, - repo: source.repo, - name: skill.name, - branch, - skillDir: row.rendered_skill_path?.replace(/\/SKILL\.md$/, '') ?? '', - filePath: '', - skillNames: repoSkillNames, - registryOwner: skill.owner, - registryRepo: skill.repo, - }) - : null - rendered = { - skillPath: row.rendered_skill_path, - raw: row.rendered_raw, - frontmatter: reparsed?.frontmatter ?? parseFrontmatterJson(row.rendered_frontmatter), - body: reparsed?.body ?? stripFrontmatter(row.rendered_raw ?? ''), - html: reparsed?.html ?? row.rendered_html, - dependencies: reparsed?.dependencies ?? [], - status: 'ok', - } - } - else { - rendered = await renderLive(event, { - sourceOwner: source.owner, - sourceRepo: source.repo, - registryOwner: skill.owner, - registryRepo: skill.repo, - name: skill.name, - branch, - skillNames: repoSkillNames, - }) - // Cache cold-path result back to D1 so subsequent visits hit the warm - // path. Fire-and-forget; missing waitUntil context (e.g. local dev) - // just means we await it inline. - schedulePersist(event, platform.db, skill.owner, skill.repo, skill.name, rendered) + .bind(skill.owner, skill.repo, skill.name) + .first(), + platform.db + .prepare(`SELECT sha FROM skill_revisions WHERE owner = ? AND repo = ? AND name = ? ORDER BY modified_at DESC LIMIT 1`) + .bind(skill.owner, skill.repo, skill.name) + .first<{ sha: string }>(), + findDuplicateGroupForSkill(event, `${skill.owner}/${skill.repo}/${skill.name}`), + getGenerated(platform.db, { owner: skill.owner, repo: skill.repo, name: skill.name, kind: 'faq' }), + getGenerated(platform.db, { owner: skill.owner, repo: skill.repo, name: skill.name, kind: 'tags' }), + getGenerated(platform.db, { owner: skill.owner, repo: skill.repo, name: skill.name, kind: 'summary' }), + platform.db + .prepare(`SELECT name FROM skills WHERE owner = ? AND repo = ? ORDER BY name`) + .bind(skill.owner, skill.repo) + .all(), + // Same definition as loadRepoSkillCounts (source_resolved = 1), so the + // 301 decision matches the hub routing used by feeds and sitemaps. + // repoSkillNames stays unfiltered: it feeds dependency parsing, not + // routing. + platform.db + .prepare(`SELECT COUNT(*) AS skill_count FROM skills WHERE owner = ? AND repo = ? AND source_resolved = 1`) + .bind(skill.owner, skill.repo) + .first(), + // Same `owners` row the owner page shows; the byline reuses it. + platform.db + .prepare(`SELECT name FROM owners WHERE owner = ?`) + .bind(skill.owner) + .first<{ name: string | null }>(), + ]) + + const source = resolveRepoSourceIdentityFromRow(skill, row) + const githubUrl = `https://github.com/${source.owner}/${source.repo}` + const branch = row?.default_branch || 'main' + const repoSkillNames = (repoSkillRows.results ?? []).map(candidate => candidate.name) + + // Warm path: render is in D1. Cold path (legacy rows or fetch_failed + // status): fall back to a live render so the first visit still works, + // then write back to D1. + let rendered: RenderedView + if (row?.rendered_html && row.rendered_status === 'ok') { + const reparsed = row.rendered_raw + ? await parseSkillMd(row.rendered_raw, { + owner: source.owner, + repo: source.repo, + name: skill.name, + branch, + skillDir: row.rendered_skill_path?.replace(/\/SKILL\.md$/, '') ?? '', + filePath: '', + skillNames: repoSkillNames, + registryOwner: skill.owner, + registryRepo: skill.repo, + }) + : null + rendered = { + skillPath: row.rendered_skill_path, + raw: row.rendered_raw, + frontmatter: reparsed?.frontmatter ?? parseFrontmatterJson(row.rendered_frontmatter), + body: reparsed?.body ?? stripFrontmatter(row.rendered_raw ?? ''), + html: reparsed?.html ?? row.rendered_html, + dependencies: reparsed?.dependencies ?? [], + status: 'ok', } + } + else { + rendered = await renderLive(event, { + sourceOwner: source.owner, + sourceRepo: source.repo, + registryOwner: skill.owner, + registryRepo: skill.repo, + name: skill.name, + branch, + skillNames: repoSkillNames, + }) + // Cache cold-path result back to D1 so subsequent visits hit the warm + // path. Fire-and-forget; missing waitUntil context (e.g. local dev) + // just means we await it inline. + schedulePersist(event, platform.db, skill.owner, skill.repo, skill.name, rendered) + } - // Stale refresh: only fire when rendered_at older than threshold. - const renderedAge = secondsAgo(row?.rendered_at) - if (row?.rendered_html && renderedAge != null && renderedAge > LIVE_RENDER_STALE_SECONDS) - scheduleRefresh(event, platform.db, skill, source, skill.name, branch, repoSkillNames) - - const rawAiTags = tagRow?.payload.tags ?? [] - const tags = rawAiTags - .map(s => TAG_BY_SLUG.get(s)) - .filter((t): t is NonNullable => Boolean(t)) - const knownTagSlugs = new Set(tags.map(t => t.slug)) - const keywords = rawAiTags.filter(t => !knownTagSlugs.has(t)) - - const description = frontmatterString(rendered.frontmatter, 'description') ?? skill.description ?? null - const license = frontmatterString(rendered.frontmatter, 'license') - let assets: { path: string, size: number, type: string }[] = [] - if (row?.assets) { - try { - const parsedAssets = JSON.parse(row.assets) as unknown - if (Array.isArray(parsedAssets)) { - assets = parsedAssets.filter((a): a is { path: string, size: number, type: string } => - Boolean(a) && typeof a === 'object' && typeof (a as { path: unknown }).path === 'string') - } - } - catch { - // Ignore malformed JSON; treat as no assets. + // Stale refresh: only fire when rendered_at older than threshold. + const renderedAge = secondsAgo(row?.rendered_at) + if (row?.rendered_html && renderedAge != null && renderedAge > LIVE_RENDER_STALE_SECONDS) + scheduleRefresh(event, platform.db, skill, source, skill.name, branch, repoSkillNames) + + const rawAiTags = tagRow?.payload.tags ?? [] + const tags = rawAiTags + .map(s => TAG_BY_SLUG.get(s)) + .filter((t): t is NonNullable => Boolean(t)) + const knownTagSlugs = new Set(tags.map(t => t.slug)) + const keywords = rawAiTags.filter(t => !knownTagSlugs.has(t)) + + const description = frontmatterString(rendered.frontmatter, 'description') ?? skill.description ?? null + const license = frontmatterString(rendered.frontmatter, 'license') + let assets: { path: string, size: number, type: string }[] = [] + if (row?.assets) { + try { + const parsedAssets = JSON.parse(row.assets) as unknown + if (Array.isArray(parsedAssets)) { + assets = parsedAssets.filter((a): a is { path: string, size: number, type: string } => + Boolean(a) && typeof a === 'object' && typeof (a as { path: unknown }).path === 'string') } } - const allowedTools = parseAllowedTools(rendered.frontmatter) - const capability = classifyAllowedTools(allowedTools) - const selectedAssets = selectSkillFiles(assets) - // `rendered.*` describes the cached copy, which survives the file being - // deleted upstream, so it can only ever say "we can still render this". The - // stored `source_resolved` is the sync's verdict on whether the file is - // still there. Reading only the former reported `resolved: true` for skills - // whose SKILL.md upstream had been 404 for months. - const sourceResolved = isSourceResolved({ - sourceResolved: row?.source_resolved, - renderStatus: rendered.status, - skillPath: rendered.skillPath, - raw: rendered.raw, - }) - const sourceGone = !sourceResolved && row?.source_resolved === 0 - const sourceCommitSha = latestCommit?.sha ?? row?.current_sha ?? null - const pushedAtIso = epochToIso(row?.pushed_at) - const createdAtIso = epochToIso(row?.repo_created_at) - const repoSkillCount = resolvedRepoSkillCount?.skill_count ?? 0 - - return { + catch { + // Ignore malformed JSON; treat as no assets. + } + } + const allowedTools = parseAllowedTools(rendered.frontmatter) + const capability = classifyAllowedTools(allowedTools) + const selectedAssets = selectSkillFiles(assets) + // `rendered.*` describes the cached copy, which survives the file being + // deleted upstream, so it can only ever say "we can still render this". The + // stored `source_resolved` is the sync's verdict on whether the file is + // still there. Reading only the former reported `resolved: true` for skills + // whose SKILL.md upstream had been 404 for months. + const sourceResolved = isSourceResolved({ + sourceResolved: row?.source_resolved, + renderStatus: rendered.status, + skillPath: rendered.skillPath, + raw: rendered.raw, + }) + const sourceGone = !sourceResolved && row?.source_resolved === 0 + const sourceCommitSha = latestCommit?.sha ?? row?.current_sha ?? null + const pushedAtIso = epochToIso(row?.pushed_at) + const createdAtIso = epochToIso(row?.repo_created_at) + const repoSkillCount = resolvedRepoSkillCount?.skill_count ?? 0 + + const detail = { + owner: skill.owner, + repo: skill.repo, + name: skill.name, + registryPath: canonicalRepoSkillPath({ owner: skill.owner, repo: skill.repo, name: skill.name, - registryPath: canonicalRepoSkillPath({ - owner: skill.owner, - repo: skill.repo, - name: skill.name, - repoSkillCount, - }), - displayName: skill.displayName, - authorName: ownerRow?.name ?? null, - githubUrl, - skillPath: rendered.skillPath, - branch, - resolutionStatus: rendered.status, - sourceGone, - contentHtml: rendered.html, - dependencies: rendered.dependencies, - frontmatter: rendered.frontmatter, - raw: rendered.raw, - assets: selectedAssets.files, - assetCount: selectedAssets.total, - curators, - description, - license, - stars: row?.stars ?? 0, - forks: row?.forks ?? 0, - pushedAt: pushedAtIso, - createdAt: createdAtIso, - maturity: computeMaturity(row?.repo_created_at ?? null, row?.pushed_at ?? null), - tier: resolveTier(skill.owner, skill.repo), - sourceFacts: { - description: { - present: Boolean(description?.trim()), - length: description?.trim().length ?? 0, - source: frontmatterString(rendered.frontmatter, 'description') ? 'frontmatter' : skill.description ? 'repository' : null, - }, - repository: { - pushedAt: pushedAtIso, - pushedAgeDays: daysFromSecondsAgo(isoToSecondsAgo(pushedAtIso)), - createdAt: createdAtIso, - stars: row?.stars ?? 0, - forks: row?.forks ?? 0, - defaultBranch: branch, - }, - source: { - resolved: sourceResolved, - gone: sourceGone, - resolutionStatus: rendered.status, - skillPath: rendered.skillPath, - currentSha: row?.current_sha ?? null, - hasCurrentSha: Boolean(row?.current_sha), - latestRevisionSha: latestCommit?.sha ?? null, - modifiedAt: row?.modified_at ?? null, - modifiedAgeDays: daysFromSecondsAgo(secondsAgo(row?.modified_at)), - referencesCount: row?.references_count ?? 0, - lastSyncedAt: row?.last_synced_at ?? null, - lastSyncedAgeDays: daysFromSecondsAgo(secondsAgo(row?.last_synced_at)), - syncStatus: row?.sync_status ?? null, - }, - frontmatter: { - present: Boolean(rendered.frontmatter && Object.keys(rendered.frontmatter).length), - keys: rendered.frontmatter ? Object.keys(rendered.frontmatter).sort() : [], - model: frontmatterString(rendered.frontmatter, 'model'), - allowedTools, - capabilityScopes: capability.scopes, - mcpServers: capability.mcpServers, - }, + repoSkillCount, + }), + displayName: skill.displayName, + authorName: ownerRow?.name ?? null, + githubUrl, + skillPath: rendered.skillPath, + branch, + resolutionStatus: rendered.status, + sourceGone, + contentHtml: rendered.html, + dependencies: rendered.dependencies, + frontmatter: rendered.frontmatter, + raw: rendered.raw, + assets: selectedAssets.files, + assetCount: selectedAssets.total, + curators, + description, + license, + stars: row?.stars ?? 0, + forks: row?.forks ?? 0, + pushedAt: pushedAtIso, + createdAt: createdAtIso, + maturity: computeMaturity(row?.repo_created_at ?? null, row?.pushed_at ?? null), + tier: resolveTier(skill.owner, skill.repo), + sourceFacts: { + description: { + present: Boolean(description?.trim()), + length: description?.trim().length ?? 0, + source: frontmatterString(rendered.frontmatter, 'description') ? 'frontmatter' : skill.description ? 'repository' : null, }, - tags, - keywords, - // Deliberately top-level, not under `seo`: likes are displayed and back - // ?sort=likes, but never feed indexability or trust (ADR-0003). - likeCount: row?.like_count ?? 0, - faqs: faqRow?.payload.faqs ?? [], - summary: summaryRow?.payload?.text - ? { text: summaryRow.payload.text } - : null, - provenance: { - owner: skill.owner, - repo: skill.repo, - branch, + repository: { + pushedAt: pushedAtIso, + pushedAgeDays: daysFromSecondsAgo(isoToSecondsAgo(pushedAtIso)), + createdAt: createdAtIso, + stars: row?.stars ?? 0, + forks: row?.forks ?? 0, + defaultBranch: branch, + }, + source: { + resolved: sourceResolved, + gone: sourceGone, + resolutionStatus: rendered.status, skillPath: rendered.skillPath, - sourceCommitSha, - sourceCommitUrl: sourceCommitSha - ? `${githubUrl}/commit/${sourceCommitSha}` - : null, - skillFileUrl: githubSkillFileUrl({ - owner: source.owner, - repo: source.repo, - skillPath: rendered.skillPath, - ref: sourceCommitSha ?? branch, - }), - historyUrl: rendered.skillPath - ? `${githubUrl}/commits/${branch}/${rendered.skillPath}` - : null, + currentSha: row?.current_sha ?? null, + hasCurrentSha: Boolean(row?.current_sha), + latestRevisionSha: latestCommit?.sha ?? null, modifiedAt: row?.modified_at ?? null, + modifiedAgeDays: daysFromSecondsAgo(secondsAgo(row?.modified_at)), referencesCount: row?.references_count ?? 0, lastSyncedAt: row?.last_synced_at ?? null, + lastSyncedAgeDays: daysFromSecondsAgo(secondsAgo(row?.last_synced_at)), syncStatus: row?.sync_status ?? null, }, - seo: { - indexScore: row?.seo_index_score ?? 0, - indexable: row?.seo_indexable === 1, - reasons: row?.seo_index_reasons ? JSON.parse(row.seo_index_reasons) as string[] : [], - syncedAt: row?.seo_index_synced_at ?? null, - curatorCount: row?.curator_count ?? 0, - curatorReasonCount: row?.curator_reason_count ?? 0, - approvedSocialCount: row?.approved_social_count ?? 0, - authorSocialCount: row?.author_social_count ?? 0, - }, - trust: { - tier: row?.trust_tier ?? 'untrusted', - source: row?.trust_source ?? 'computed', - score: row?.trust_score ?? 0, - reasons: row?.trust_reasons ? JSON.parse(row.trust_reasons) as string[] : [], - syncedAt: row?.trust_synced_at ?? null, + frontmatter: { + present: Boolean(rendered.frontmatter && Object.keys(rendered.frontmatter).length), + keys: rendered.frontmatter ? Object.keys(rendered.frontmatter).sort() : [], + model: frontmatterString(rendered.frontmatter, 'model'), + allowedTools, + capabilityScopes: capability.scopes, + mcpServers: capability.mcpServers, }, - duplicateGroup, - } - }, -}) + }, + tags, + keywords, + // Deliberately top-level, not under `seo`: likes are displayed and back + // ?sort=likes, but never feed indexability or trust (ADR-0003). + likeCount: row?.like_count ?? 0, + faqs: faqRow?.payload.faqs ?? [], + summary: summaryRow?.payload?.text + ? { text: summaryRow.payload.text } + : null, + provenance: { + owner: skill.owner, + repo: skill.repo, + branch, + skillPath: rendered.skillPath, + sourceCommitSha, + sourceCommitUrl: sourceCommitSha + ? `${githubUrl}/commit/${sourceCommitSha}` + : null, + skillFileUrl: githubSkillFileUrl({ + owner: source.owner, + repo: source.repo, + skillPath: rendered.skillPath, + ref: sourceCommitSha ?? branch, + }), + historyUrl: rendered.skillPath + ? `${githubUrl}/commits/${branch}/${rendered.skillPath}` + : null, + modifiedAt: row?.modified_at ?? null, + referencesCount: row?.references_count ?? 0, + lastSyncedAt: row?.last_synced_at ?? null, + syncStatus: row?.sync_status ?? null, + }, + seo: { + indexScore: row?.seo_index_score ?? 0, + indexable: row?.seo_indexable === 1, + reasons: row?.seo_index_reasons ? JSON.parse(row.seo_index_reasons) as string[] : [], + syncedAt: row?.seo_index_synced_at ?? null, + curatorCount: row?.curator_count ?? 0, + curatorReasonCount: row?.curator_reason_count ?? 0, + approvedSocialCount: row?.approved_social_count ?? 0, + authorSocialCount: row?.author_social_count ?? 0, + }, + trust: { + tier: row?.trust_tier ?? 'untrusted', + source: row?.trust_source ?? 'computed', + score: row?.trust_score ?? 0, + reasons: row?.trust_reasons ? JSON.parse(row.trust_reasons) as string[] : [], + syncedAt: row?.trust_synced_at ?? null, + }, + duplicateGroup, + } + + return detail +} // Skill detail data is public and changes only when indexing or social counts -// update. Cache the assembled response so popular links do not repeat every D1 -// lookup and Markdown render for each reader. -export default defineCachedEventHandler(skillDetailHandler, { - maxAge: 60, - staleMaxAge: 60 * 5, - swr: true, - group: 'skill-detail', - name: 'skill-detail-v1', - getKey: event => (getRouterParam(event, 'slug') ?? '').toLowerCase(), -}) +// update. A read-through KV cache keeps popular links from repeating every D1 +// lookup and Markdown render for each reader. `cached` restores what Nitro's +// route cache used to provide here (maxAge 60, staleMaxAge 300, swr) without +// the bare `storage.setItem` that let a KV `KV PUT failed: 429` escape the +// handler as a 500 (Sentry SKILLD-1V): a fresh entry is served as-is, a stale +// entry is served while one background refresh recomputes, and a dead entry +// recomputes once per key per isolate. Without that stale shield every +// concurrent miss after the 60s TTL re-runs about six D1 queries plus a +// possible live GitHub render, and cold-key bursts are the documented D1 +// overload mechanism on these routes (Sentry SKILLD-G/H/J/K/M/N/P/Q). +export default skillDetailHandler interface RenderedView { skillPath: string | null diff --git a/nuxt.config.ts b/nuxt.config.ts index fedf39c0..9fa6081a 100644 --- a/nuxt.config.ts +++ b/nuxt.config.ts @@ -78,6 +78,7 @@ export default defineNuxtConfig({ fields: [ 'attempt', 'batch.count', + 'cache.key', 'cache.readFailed', 'cache.writeFailed', 'eligible.count', diff --git a/scripts/check-published-cli-grammar.ts b/scripts/check-published-cli-grammar.ts index afde4a57..44006aa1 100644 --- a/scripts/check-published-cli-grammar.ts +++ b/scripts/check-published-cli-grammar.ts @@ -109,6 +109,15 @@ async function publishedVersion(tag: ChannelRequirement['tag']): Promise 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 { diff --git a/shared/server/cache.ts b/shared/server/cache.ts index 25baf042..8983a981 100644 --- a/shared/server/cache.ts +++ b/shared/server/cache.ts @@ -29,6 +29,115 @@ export async function writeCache( } } +/** + * 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 { + v: T + t: number +} + +function isSwrEntry(value: unknown): value is SwrEntry { + return typeof value === 'object' && value !== null + && 'v' in value && 't' in value + && typeof (value as SwrEntry).t === 'number' +} + +export interface CachedOptions { + 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 + /** + * 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) => 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>() + +export function cached(options: CachedOptions): Promise { + 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 { + const pending = inflightComputes.get(key) + if (pending) + return pending as Promise + const promise = (async () => { + const value = await compute() + await writeCache(options.storage, key, { v: value, t: nowSeconds() } satisfies SwrEntry, { 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>(options.storage, key) + if (isSwrEntry(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}. diff --git a/test/unit/cache-write-best-effort.test.ts b/test/unit/cache-write-best-effort.test.ts index acb92c68..b791fef3 100644 --- a/test/unit/cache-write-best-effort.test.ts +++ b/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> + + // 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) => 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', ?, '

ok

', ?)`, + ).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() + 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 + } +}) diff --git a/test/unit/published-cli-grammar.test.ts b/test/unit/published-cli-grammar.test.ts index 704cb841..84d44f3c 100644 --- a/test/unit/published-cli-grammar.test.ts +++ b/test/unit/published-cli-grammar.test.ts @@ -1,3 +1,4 @@ +// @vitest-environment node import { describe, expect, it } from 'vitest' import { publishedCliInvocation, diff --git a/test/unit/skill-detail-swr.test.ts b/test/unit/skill-detail-swr.test.ts new file mode 100644 index 00000000..8ce6625c --- /dev/null +++ b/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> + let prepareCalls: number + + vi.stubGlobal('defineEventHandler', (handler: unknown) => handler) + vi.stubGlobal('createError', (input: Record) => 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', ?, '

ok

', ?)`, + ).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() + 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, 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, t: number } + expect(written.t).toBeGreaterThan(staleT) + }) + expect(prepareCalls - beforeBurst).toBeLessThanOrEqual(singleRunQueries) + const refreshed = cacheMap.get(cacheKey) as { v: Record, 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 + } +})