From 64ca27ff024a6b76769e98286ef4bc421401913a Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Mon, 17 Aug 2026 16:20:03 +0100 Subject: [PATCH 01/22] fix(web): align /internal/* result cache with the public routes (#143) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every cold-cache OG unfurl served the neutral placeholder. Only the URLs the liveness probe re-requests each cycle came back real, which is why this looked green for weeks. Two independent misalignments in the /internal/* endpoint web-og calls: - ORIGIN. web-og targets `https://web/internal/...`, so makeWorkerCache keyed on `https://web/__cache__/...` — a different namespace from the public permalink routes', and a non-routable hostname the Cache API silently declines to store (the class of bug cache.ts's header note already records for `cache.invalid`). The OG path could neither reuse a warm public entry nor persist its own, so it never self-healed. - KEY PARTS. The public routes key on five parts ending in the default `cull`/`nopre` option suffixes, and spell ids `issue#`/`pr#`; /internal used a three-part key with `issue:`/`pr:`. Even with the origin fixed, that can never land on a slot a public hit warms. Key on the canonical public origin (PUBLIC_BASE_URL, else the committed PROD_HOST var, else the request origin) and mirror the public key exactly. A cache read failure now degrades to a recompute rather than a 500, since the key URL is deliberately not this request's origin. Regression of #53. --- packages/web/src/routes/internal.ts | 57 ++++- packages/web/test/integration.test.ts | 17 +- .../web/test/internal-cache-origin.test.ts | 203 ++++++++++++++++++ 3 files changed, 259 insertions(+), 18 deletions(-) create mode 100644 packages/web/test/internal-cache-origin.test.ts diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 3fb52d0..8c23494 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -6,7 +6,7 @@ import { cacheKey, findRelease, type LookupInput, type LookupResult } from '@released/core'; import type { Context } from 'hono'; -import { makeWorkerCache } from '../cache.js'; +import { makeWorkerCache, type WorkerCache } from '../cache.js'; import type { Env } from '../env.js'; import { makeProvider } from '../provider.js'; import { singleFlight } from '../single-flight.js'; @@ -30,20 +30,54 @@ function isServiceBinding(c: Context): boolean { return !!marker && marker === secret; } +/** Origin for the result-cache key URL. + * + * web-og hardcodes `https://web/internal/...` as the Service-Binding target, so + * keying the cache on the incoming request broke OG renders two ways (#143): + * `web` is not a routable hostname, which the Cache API silently declines to + * store (see cache.ts's header note), and it is a different namespace from the + * public permalink routes'. The OG path could therefore neither reuse a warm + * public entry nor persist its own, so every cold unfurl paid a full lookup, + * blew web-og's deadline, and got the placeholder cached by the crawler. + * + * Resolve the canonical public origin instead: an explicit PUBLIC_BASE_URL, else + * the committed PROD_HOST var, else the request's own origin (`wrangler dev` + * and tests, where neither var is set). */ +function cacheOrigin(env: Env, req: Request): string { + if (env.PUBLIC_BASE_URL) return env.PUBLIC_BASE_URL.replace(/\/$/, ''); + if (env.PROD_HOST) return `https://${env.PROD_HOST}`; + return new URL(req.url).origin; +} + +/** cache.get that is never fatal. The key URL is deliberately not this request's + * origin (see cacheOrigin), so a Cache API refusal must degrade to a recompute + * rather than a 500 that web-og would render as a placeholder. */ +async function cachedResult(cache: WorkerCache, k: string): Promise { + try { + return await cache.get(k); + } catch { + return null; + } +} + /** Resolve the LookupResult JSON for a lookup input. Cache-first, then compute - * via the (relay-aware) provider. Host-aware cache key so OG renders share slots - * with the public routes' `${host}/${projectPath}` prefix; the input kind+id - * distinguishes the slot (`sha:` / `issue:` / `pr:`), mirroring the commit - * endpoint's `sha:${sha}` scheme. */ + * via the (relay-aware) provider. + * + * The cache key MUST match the public permalink routes' exactly (result.tsx, + * issue.tsx, pr.tsx) or the OG card renders into a namespace no public hit can + * ever warm — that was half of #143. That means all five parts, including the + * `cull`/`nopre` suffixes for the default (non-strict, no-prerelease) options an + * OG card always renders, and the public `issue#`/`pr#` id spelling rather than + * the `issue:`/`pr:` this endpoint used to invent. */ async function resolveResult(c: Context, input: LookupInput): Promise { const env = c.env as Env; const req = c.req.raw; const { host, projectPath } = input.repo; - const idPart = input.kind === 'commit' ? `sha:${input.sha}` : `${input.kind}:${input.number}`; - const k = await cacheKey('res', `${host}/${projectPath}`, idPart); - const cache = makeWorkerCache(req); - let result: LookupResult | null = await cache.get(k); + const idPart = input.kind === 'commit' ? `sha:${input.sha}` : `${input.kind}#${input.number}`; + const k = await cacheKey('res', `${host}/${projectPath}`, idPart, 'cull', 'nopre'); + const cache = makeWorkerCache(new Request(cacheOrigin(env, req))); + let result: LookupResult | null = await cachedResult(cache, k); if (result) { return new Response(JSON.stringify(result), { headers: { 'content-type': 'application/json' }, @@ -57,7 +91,10 @@ async function resolveResult(c: Context, input: LookupInput): Promise result = await singleFlight(k, async () => { const re = await cache.get(k); if (re) return re; - const r = await findRelease(input, { client }); + // Options stated explicitly: they are what the `cull`/`nopre` key parts + // above promise, so the slot this writes is the one a default public + // permalink hit reads back. + const r = await findRelease(input, { client, strict: false, includePrereleases: false }); await cache.put(k, r, 30 * 60); return r; }); diff --git a/packages/web/test/integration.test.ts b/packages/web/test/integration.test.ts index 032bafc..cf5e3ea 100644 --- a/packages/web/test/integration.test.ts +++ b/packages/web/test/integration.test.ts @@ -36,7 +36,7 @@ const INTERNAL_SECRET = 'test-shared-secret'; // typed `unknown`, not LookupResult (a full LookupResult isn't required to exercise // the route, and the seeded fixtures below don't populate one). async function seedFederatedResult(sha: string, result: unknown) { - const key = await cacheKey('res', 'gitlab.gnome.org/GNOME/gimp', `sha:${sha}`); + const key = await cacheKey('res', 'gitlab.gnome.org/GNOME/gimp', `sha:${sha}`, 'cull', 'nopre'); cacheStore.set( `https://released.example/__cache__/${encodeURIComponent(key)}`, new Response(JSON.stringify(result), { headers: { 'content-type': 'application/json' } }), @@ -600,9 +600,10 @@ describe('web Worker — issue/PR internal endpoints (#79)', () => { releaseNotesHtml: null, rateLimit: null, }; - // Seed the slot the route reads: cacheKey('res', `${host}/${projectPath}`, - // `issue:${number}`) — mirroring the commit endpoint's `sha:${sha}` key. - const key = await cacheKey('res', 'github.com/honojs/hono', 'issue:11'); + // Seed the slot the route reads. Since #143 that is the SAME key the public + // /i/ permalink writes: cacheKey('res', `${host}/${projectPath}`, + // `issue#${number}`, 'cull', 'nopre') — see routes/internal.ts. + const key = await cacheKey('res', 'github.com/honojs/hono', 'issue#11', 'cull', 'nopre'); cacheStore.set( `https://released.example/__cache__/${encodeURIComponent(key)}`, new Response(JSON.stringify(seeded), { headers: { 'content-type': 'application/json' } }), @@ -631,7 +632,7 @@ describe('web Worker — issue/PR internal endpoints (#79)', () => { releaseNotesHtml: null, rateLimit: null, }; - const key = await cacheKey('res', 'github.com/honojs/hono', 'pr:17'); + const key = await cacheKey('res', 'github.com/honojs/hono', 'pr#17', 'cull', 'nopre'); cacheStore.set( `https://released.example/__cache__/${encodeURIComponent(key)}`, new Response(JSON.stringify(seeded), { headers: { 'content-type': 'application/json' } }), @@ -664,7 +665,7 @@ describe('web Worker — issue/PR internal endpoints (#79)', () => { rateLimit: null, }; // Proves the route keys the cache by host on the (Hono-decoded) projectPath. - const key = await cacheKey('res', 'gitlab.gnome.org/GNOME/glib', 'issue:1234'); + const key = await cacheKey('res', 'gitlab.gnome.org/GNOME/glib', 'issue#1234', 'cull', 'nopre'); cacheStore.set( `https://released.example/__cache__/${encodeURIComponent(key)}`, new Response(JSON.stringify(seeded), { headers: { 'content-type': 'application/json' } }), @@ -705,7 +706,7 @@ describe('web Worker — issue/PR internal endpoints (#79)', () => { rateLimit: null, }; // Seed the slot the FIXED route keys on (Hono already decoded bad%25 → bad%). - const key = await cacheKey('res', 'gitlab.gnome.org/bad%', 'issue:1'); + const key = await cacheKey('res', 'gitlab.gnome.org/bad%', 'issue#1', 'cull', 'nopre'); cacheStore.set( `https://released.example/__cache__/${encodeURIComponent(key)}`, new Response(JSON.stringify(seeded), { headers: { 'content-type': 'application/json' } }), @@ -721,7 +722,7 @@ describe('web Worker — issue/PR internal endpoints (#79)', () => { it('GET /internal/issue/... fails CLOSED when INTERNAL_SECRET is unset (no web-og fallback)', async () => { cacheStore.clear(); - const key = await cacheKey('res', 'github.com/honojs/hono', 'issue:11'); + const key = await cacheKey('res', 'github.com/honojs/hono', 'issue#11', 'cull', 'nopre'); cacheStore.set( `https://released.example/__cache__/${encodeURIComponent(key)}`, new Response( diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts new file mode 100644 index 0000000..d9ae9a7 --- /dev/null +++ b/packages/web/test/internal-cache-origin.test.ts @@ -0,0 +1,203 @@ +// Guard for #143: every cold-cache OG unfurl served the neutral placeholder, +// because the /internal/* result endpoint web-og calls could not see — or write — +// the result cache the public permalink routes use. +// +// Two independent misalignments caused it, and this file pins BOTH: +// +// 1. ORIGIN. web-og calls `env.WEB.fetch('https://web/internal/...')`, so +// makeWorkerCache derived the key URL `https://web/__cache__/...`. That is a +// different namespace from the public routes' `https:///__cache__/...`, +// and `web` is a non-routable hostname, which the Cache API silently declines to +// store — the same class of bug cache.ts's header note records for `cache.invalid`. +// So the OG path neither read nor wrote the cache, and never self-healed. +// 2. KEY PARTS. The public routes key on the 5-part +// ('res', host/path, 'sha:' | 'issue#' | 'pr#', 'cull', 'nopre'); +// /internal keyed on a 3-part ('res', host/path, 'sha:' | 'issue:'). +// Even with the origin fixed, that can never hit a public-route entry. +// +// Every test here calls the route with the REAL production URL shape +// (`https://web/internal/...`). The pre-existing /internal tests in +// integration.test.ts all use a public-looking `https://released.example/...`, which +// is exactly why this regression shipped green. + +import { cacheKey } from '@released/core'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// findRelease is the upstream lookup. Mocking it lets a cache HIT be proven by +// "the provider was never consulted", and lets the cold path complete without network. +const findReleaseMock = vi.hoisted(() => vi.fn()); +vi.mock('@released/core', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, findRelease: findReleaseMock }; +}); + +// Polyfill the Workers-only `caches.default` so cache.ts works under Node. +const cacheStore = new Map(); +(globalThis as unknown as { caches: { default: Cache } }).caches = { + default: { + async match(req: Request | string) { + const url = typeof req === 'string' ? req : req.url; + const stored = cacheStore.get(url); + return stored ? stored.clone() : undefined; + }, + async put(req: Request | string, res: Response) { + const url = typeof req === 'string' ? req : req.url; + cacheStore.set(url, res); + }, + } as unknown as Cache, +}; + +const { default: app } = await import('../src/index.js'); + +const INTERNAL_SECRET = 'test-shared-secret'; +const PROD_HOST = 'released.blabberate.com'; +const PUBLIC_ORIGIN = `https://${PROD_HOST}`; +const ENV = { INTERNAL_SECRET, PROD_HOST }; +const SHA = 'a'.repeat(40); + +/** The cache key the PUBLIC permalink routes write (result.tsx / issue.tsx / pr.tsx): + * 5 parts, ending in the default `cull` + `nopre` option suffixes. */ +function publicKey(repo: string, idPart: string): Promise { + return cacheKey('res', repo, idPart, 'cull', 'nopre'); +} + +function keyUrl(origin: string, key: string): string { + return `${origin}/__cache__/${encodeURIComponent(key)}`; +} + +function seed(origin: string, key: string, value: unknown): void { + cacheStore.set( + keyUrl(origin, key), + new Response(JSON.stringify(value), { headers: { 'content-type': 'application/json' } }), + ); +} + +/** Partial LookupResult — only the fields the route serializes back. */ +function fixture(tag: string): unknown { + return { + input: { kind: 'commit', repo: { host: 'github.com', projectPath: 'honojs/hono' }, sha: SHA }, + canonicalSha: SHA, + firstRelease: { tag, sha: 'tagsha', date: '2024-02-01T00:00:00Z', url: '' }, + alsoIn: [], + releaseNotesHtml: null, + rateLimit: null, + }; +} + +/** A request in the shape web-og actually sends over the Service Binding. */ +function svc(url: string): Request { + return new Request(url, { headers: { 'x-released-internal': INTERNAL_SECRET } }); +} + +async function tagOf(res: Response): Promise { + const body = (await res.json()) as { firstRelease?: { tag?: string } }; + return body.firstRelease?.tag; +} + +beforeEach(() => { + cacheStore.clear(); + findReleaseMock.mockReset(); +}); + +describe('/internal/* reads the cache the PUBLIC routes populate (#143)', () => { + it('serves a commit result the public permalink route already cached', async () => { + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + seed(PUBLIC_ORIGIN, k, fixture('v4.8.12')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.8.12'); + // The whole point: a warm public route means the OG render costs no lookup. + expect(findReleaseMock).not.toHaveBeenCalled(); + }); + + it('serves an issue result on the public `issue#` key, not the legacy `issue:`', async () => { + const k = await publicKey('github.com/honojs/hono', 'issue#11'); + seed(PUBLIC_ORIGIN, k, fixture('v0.0.11')); + + const res = await app.fetch(svc('https://web/internal/issue/honojs/hono/11'), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v0.0.11'); + expect(findReleaseMock).not.toHaveBeenCalled(); + }); + + it('serves a PR result on the public `pr#` key, not the legacy `pr:`', async () => { + const k = await publicKey('github.com/honojs/hono', 'pr#4800'); + seed(PUBLIC_ORIGIN, k, fixture('v4.9.0')); + + const res = await app.fetch(svc('https://web/internal/pr/honojs/hono/4800'), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.9.0'); + expect(findReleaseMock).not.toHaveBeenCalled(); + }); + + it('serves a federated (non-GitHub) commit result from the host-keyed public slot', async () => { + const k = await publicKey('gitlab.gnome.org/GNOME/gimp', `sha:${SHA}`); + seed(PUBLIC_ORIGIN, k, fixture('GIMP_2_10_36')); + + const res = await app.fetch( + svc(`https://web/internal/h/gitlab.gnome.org/r/GNOME%2Fgimp/${SHA}`), + ENV, + ); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('GIMP_2_10_36'); + expect(findReleaseMock).not.toHaveBeenCalled(); + }); +}); + +describe('/internal/* WRITES back to the slot the public routes read (#143)', () => { + it('warms the public cache key on a cold lookup, and writes nothing under `https://web`', async () => { + findReleaseMock.mockResolvedValue(fixture('v4.10.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.10.0'); + expect(findReleaseMock).toHaveBeenCalledOnce(); + + // The cold OG render must leave the answer where the public route will find it, + // otherwise every unfurl pays a full lookup forever (the #143 "does not self-heal"). + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + expect([...cacheStore.keys()]).toContain(keyUrl(PUBLIC_ORIGIN, k)); + + // And nothing may land in the non-routable Service-Binding namespace, which the + // real Cache API silently drops. + expect([...cacheStore.keys()].filter((u) => u.startsWith('https://web/'))).toEqual([]); + }); +}); + +describe('/internal/* cache origin falls back to the request origin', () => { + it('uses the request origin when no PROD_HOST/PUBLIC_BASE_URL is configured', async () => { + // `wrangler dev` and the unit tests have neither var set; the route must still + // key on something routable rather than hardcoding the production hostname. + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + seed('https://released.example', k, fixture('v4.7.0')); + + const res = await app.fetch( + svc(`https://released.example/internal/result/honojs/hono/${SHA}`), + { INTERNAL_SECRET }, + ); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.7.0'); + expect(findReleaseMock).not.toHaveBeenCalled(); + }); + + it('prefers an explicit PUBLIC_BASE_URL over PROD_HOST', async () => { + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + seed('https://staging.example', k, fixture('v4.6.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), { + INTERNAL_SECRET, + PROD_HOST, + PUBLIC_BASE_URL: 'https://staging.example/', + }); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.6.0'); + expect(findReleaseMock).not.toHaveBeenCalled(); + }); +}); From 837ec792706729043c92358539ea0490c9ee9b12 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Mon, 17 Aug 2026 16:48:30 +0100 Subject: [PATCH 02/22] fix(web): make /internal follow the shared slot's cache policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three review findings on #144. All three are defects in what this PR introduced — it made /internal share the public routes' cache slot, so it has to share the policy and the failure modes that govern it. - Cache faults are never fatal. Only the FIRST get was guarded; the inner get and the put sat inside the try whose catch returns 503, which web-og renders as the neutral placeholder — the #143 symptom, from a successful lookup. A neverFatal() wrapper degrades any Cache API refusal to "served, just not cached". - /internal now resolves through resolveLookup, the same resolver the public routes use: per-state hard TTLs (30d terminal / 24h pending / 60s partial) instead of a flat 30 minutes, getEntry()+isFresh() instead of a bare read, and the negative back-off for free. A cold OG render no longer downgrades a 30-day terminal slot to 30 minutes, and no longer serves a 60-second partial for half an hour after the public page has moved on. - Preview sets its own PUBLIC_BASE_URL. PROD_HOST is committed in [env.preview.vars] too, so preview keyed /internal on a host it does not serve while its public routes keyed on the preview origin — #143, unfixed in the one environment OG changes get reviewed in. Guards mutation-proved: all 7 new tests were RED on the pre-fix code (503 on a refused write, 503 on a throwing read, max-age=1800 where the policy says 2592000 and 60, a 10-minute-old pending answer served without revalidation, no negative marker, no preview PUBLIC_BASE_URL), 14/14 green after. Full gate green: 599 tests, typecheck, lint, build, deploy-config. --- packages/web/src/routes/internal.ts | 122 +++++++++----- .../web/test/internal-cache-origin.test.ts | 156 +++++++++++++++++- packages/web/wrangler.toml | 6 + 3 files changed, 240 insertions(+), 44 deletions(-) diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 8c23494..dacc308 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -4,12 +4,13 @@ // web-og calls these via a Cloudflare Service Binding (env.WEB.fetch(...)) to get // the result JSON for rendering the OG PNG. Direct public hits are rejected. -import { cacheKey, findRelease, type LookupInput, type LookupResult } from '@released/core'; +import { cacheKey, findRelease, type LookupInput } from '@released/core'; import type { Context } from 'hono'; -import { makeWorkerCache, type WorkerCache } from '../cache.js'; +import type { CacheEntry, WorkerCache } from '../cache.js'; +import { makeWorkerCache } from '../cache.js'; import type { Env } from '../env.js'; import { makeProvider } from '../provider.js'; -import { singleFlight } from '../single-flight.js'; +import { type Resolved, resolveLookup } from '../resolve.js'; /** Marker header set by the web-og Service Binding to identify itself. * Cloudflare Service Binding requests can also be checked via the routing @@ -40,24 +41,60 @@ function isServiceBinding(c: Context): boolean { * public entry nor persist its own, so every cold unfurl paid a full lookup, * blew web-og's deadline, and got the placeholder cached by the crawler. * - * Resolve the canonical public origin instead: an explicit PUBLIC_BASE_URL, else - * the committed PROD_HOST var, else the request's own origin (`wrangler dev` - * and tests, where neither var is set). */ + * Resolve the origin this deployment's own public routes key on: an explicit + * PUBLIC_BASE_URL, else the committed PROD_HOST var, else the request's origin. + * + * PUBLIC_BASE_URL is what makes that per-environment. PROD_HOST is committed in + * BOTH [vars] and [env.preview.vars] (it gates analytics, which must stay + * prod-only), so without an explicit override the preview Worker — and + * `wrangler dev`, which loads [vars] — would key on the production origin while + * their public routes key on the origin they actually serve. wrangler.toml sets + * PUBLIC_BASE_URL for preview; for `wrangler dev`, put + * `PUBLIC_BASE_URL=http://localhost:8787` in packages/web/.dev.vars. Only the + * unit tests reach the request-origin fallback. */ function cacheOrigin(env: Env, req: Request): string { if (env.PUBLIC_BASE_URL) return env.PUBLIC_BASE_URL.replace(/\/$/, ''); if (env.PROD_HOST) return `https://${env.PROD_HOST}`; return new URL(req.url).origin; } -/** cache.get that is never fatal. The key URL is deliberately not this request's - * origin (see cacheOrigin), so a Cache API refusal must degrade to a recompute - * rather than a 500 that web-og would render as a placeholder. */ -async function cachedResult(cache: WorkerCache, k: string): Promise { - try { - return await cache.get(k); - } catch { - return null; - } +/** Wrap a cache so no Cache API call can be fatal. The key URL is deliberately + * not this request's own origin (see cacheOrigin) and the Cache API is entitled + * to refuse such a read or write; that must degrade to "served, just not + * cached". A 503 here is what web-og renders as the neutral placeholder — the + * #143 symptom — so a cache fault must never throw away a computed answer. */ +function neverFatal(cache: WorkerCache): WorkerCache { + return { + async get(key: string): Promise { + try { + return await cache.get(key); + } catch { + return null; + } + }, + async getEntry(key: string): Promise | null> { + try { + return await cache.getEntry(key); + } catch { + return null; + } + }, + async put(key: string, value: T, ttlSeconds: number): Promise { + try { + await cache.put(key, value, ttlSeconds); + } catch { + // Served, just not cached. + } + }, + }; +} + +/** Message for the 503 web-og reads as "no result" (it renders the neutral card + * for any non-OK response, so only the body text differs by cause). */ +function failureMessage(resolved: Exclude): string { + if (resolved.status === 'not_yet') return resolved.error.message; + if (resolved.status === 'transient') return resolved.kind; + return (resolved.error as Error)?.message ?? 'failed'; } /** Resolve the LookupResult JSON for a lookup input. Cache-first, then compute @@ -76,37 +113,36 @@ async function resolveResult(c: Context, input: LookupInput): Promise const idPart = input.kind === 'commit' ? `sha:${input.sha}` : `${input.kind}#${input.number}`; const k = await cacheKey('res', `${host}/${projectPath}`, idPart, 'cull', 'nopre'); - const cache = makeWorkerCache(new Request(cacheOrigin(env, req))); - let result: LookupResult | null = await cachedResult(cache, k); - if (result) { - return new Response(JSON.stringify(result), { - headers: { 'content-type': 'application/json' }, - }); - } - - // Cache miss: compute. The web-og caller chose to wait for this on its side. - // Anubis-protected hosts get a relay-backed fetch (see makeProvider/relay.ts). - try { - const client = makeProvider(env, req, host); - result = await singleFlight(k, async () => { - const re = await cache.get(k); - if (re) return re; - // Options stated explicitly: they are what the `cull`/`nopre` key parts - // above promise, so the slot this writes is the one a default public - // permalink hit reads back. - const r = await findRelease(input, { client, strict: false, includePrereleases: false }); - await cache.put(k, r, 30 * 60); - return r; - }); - return new Response(JSON.stringify(result), { - headers: { 'content-type': 'application/json' }, - }); - } catch (err) { - return new Response(JSON.stringify({ error: (err as Error)?.message ?? 'failed' }), { - status: 503, + const cache = neverFatal(makeWorkerCache(new Request(cacheOrigin(env, req)))); + + // Same resolver the public routes use, on the same slot — sharing a cache slot + // means sharing the policy that governs it: per-state hard TTLs (30 days + // terminal / 24h pending / 60s partial), a 5-minute freshness window, and the + // negative back-off that keeps a down host from being pounded. A flat TTL here + // would downgrade a terminal slot the permalink would have kept for 30 days, + // and a bare read would keep serving a 60-second partial for far longer than + // the public page does. Options are stated explicitly because they are what the + // `cull`/`nopre` key parts promise. Anubis-protected hosts get a relay-backed + // fetch (see makeProvider/relay.ts). The web-og caller chose to wait for this. + const resolved = await resolveLookup({ + cache, + key: k, + load: () => + findRelease(input, { + client: makeProvider(env, req, host), + strict: false, + includePrereleases: false, + }), + }); + if (resolved.status === 'ok') { + return new Response(JSON.stringify(resolved.result), { headers: { 'content-type': 'application/json' }, }); } + return new Response(JSON.stringify({ error: failureMessage(resolved) }), { + status: 503, + headers: { 'content-type': 'application/json' }, + }); } /** Parse a permalink :number param into a positive int, or null if invalid. */ diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index d9ae9a7..b2cfba8 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -20,7 +20,10 @@ // integration.test.ts all use a public-looking `https://released.example/...`, which // is exactly why this regression shipped green. -import { cacheKey } from '@released/core'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { cacheKey, ProviderServerError } from '@released/core'; +import { parse as parseToml } from 'smol-toml'; import { beforeEach, describe, expect, it, vi } from 'vitest'; // findRelease is the upstream lookup. Mocking it lets a cache HIT be proven by @@ -32,15 +35,20 @@ vi.mock('@released/core', async (importOriginal) => { }); // Polyfill the Workers-only `caches.default` so cache.ts works under Node. +// `cacheFault` lets a test make the Cache API itself fail, which is the failure +// mode the real edge has for a key URL on an origin this Worker doesn't serve. const cacheStore = new Map(); +let cacheFault: 'none' | 'match' | 'put' = 'none'; (globalThis as unknown as { caches: { default: Cache } }).caches = { default: { async match(req: Request | string) { + if (cacheFault === 'match') throw new Error('cache read refused'); const url = typeof req === 'string' ? req : req.url; const stored = cacheStore.get(url); return stored ? stored.clone() : undefined; }, async put(req: Request | string, res: Response) { + if (cacheFault === 'put') throw new Error('cache write refused'); const url = typeof req === 'string' ? req : req.url; cacheStore.set(url, res); }, @@ -94,8 +102,49 @@ async function tagOf(res: Response): Promise { return body.firstRelease?.tag; } +/** Seed a slot with an explicit age, the way cache.ts stamps `x-cached-at`. */ +function seedAged(origin: string, key: string, value: unknown, ageSeconds: number): void { + cacheStore.set( + keyUrl(origin, key), + new Response(JSON.stringify(value), { + headers: { + 'content-type': 'application/json', + 'x-cached-at': String(Date.now() - ageSeconds * 1000), + }, + }), + ); +} + +/** A non-terminal result — looked up, not in a release yet. The shared policy + * (resolve.ts) revalidates these every 5 minutes; `subject` identifies which + * copy of the answer a response came from. */ +function pendingFixture(subject: string): unknown { + return { ...(fixture('unused') as Record), firstRelease: null, subject }; +} + +/** A soft-deadline best-effort answer that ran out of budget before finding a + * containing release. The shared policy trusts these for 60 seconds — and the + * OG path is the deadline-pressured one, so it produces them most. */ +function partialFixture(): unknown { + return { + ...(fixture('unused') as Record), + firstRelease: null, + partial: { reason: 'soft_deadline', candidatesTried: 3 }, + }; +} + +async function subjectOf(res: Response): Promise { + const body = (await res.json()) as { subject?: string }; + return body.subject; +} + +function cacheControlOf(origin: string, key: string): string | null { + return cacheStore.get(keyUrl(origin, key))?.headers.get('cache-control') ?? null; +} + beforeEach(() => { cacheStore.clear(); + cacheFault = 'none'; findReleaseMock.mockReset(); }); @@ -201,3 +250,108 @@ describe('/internal/* cache origin falls back to the request origin', () => { expect(findReleaseMock).not.toHaveBeenCalled(); }); }); + +// Sharing the slot with the public routes means sharing the POLICY that governs +// it (resolve.ts): 30-day terminal / 24h pending / 60s partial hard TTLs, a +// 5-minute freshness window, and a negative back-off. /internal used to invent a +// flat 30-minute TTL and a bare read, so a cold OG render could downgrade a +// terminal slot to 30 minutes and keep serving a 60-second partial for half an hour. +describe('/internal/* follows the cache policy that governs the shared slot', () => { + it('writes a terminal answer with the public routes 30-day hard TTL', async () => { + findReleaseMock.mockResolvedValue(fixture('v4.10.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + expect(res.status).toBe(200); + + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + expect(cacheControlOf(PUBLIC_ORIGIN, k)).toBe(`public, max-age=${30 * 24 * 60 * 60}`); + }); + + it('writes a soft-deadline partial with the 60-second TTL, not a flat 30 minutes', async () => { + findReleaseMock.mockResolvedValue(partialFixture()); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + expect(res.status).toBe(200); + + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + expect(cacheControlOf(PUBLIC_ORIGIN, k)).toBe('public, max-age=60'); + }); + + it('revalidates a pending answer past its 5-minute freshness window', async () => { + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + seedAged(PUBLIC_ORIGIN, k, pendingFixture('stale pending'), 10 * 60); + findReleaseMock.mockResolvedValue(fixture('v4.12.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + + expect(res.status).toBe(200); + // The public page would already be showing v4.12.0; the OG card must not keep + // rendering the pending answer for another 30 minutes. + expect(await tagOf(res)).toBe('v4.12.0'); + expect(findReleaseMock).toHaveBeenCalledOnce(); + }); + + it('serves the last-known-good answer when the upstream blips, and backs off', async () => { + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + seedAged(PUBLIC_ORIGIN, k, pendingFixture('last known good'), 10 * 60); + findReleaseMock.mockRejectedValue( + new ProviderServerError('github.com', 503, 'Service Unavailable'), + ); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + + expect(res.status).toBe(200); + expect(await subjectOf(res)).toBe('last known good'); + // And the next unfurl inside the back-off window must not re-hit the down host. + expect([...cacheStore.keys()]).toContain(keyUrl(PUBLIC_ORIGIN, `${k}:neg`)); + }); +}); + +// The key URL is deliberately NOT this request's own origin (the Service Binding +// arrives on `https://web`), and the Cache API is entitled to refuse such a +// write. A refusal must degrade to "served, just not cached" — a 503 here is +// what web-og turns into the neutral placeholder, which IS the #143 symptom. +describe('/internal/* never turns a Cache API failure into a placeholder', () => { + it('serves the computed answer when the cache WRITE is refused', async () => { + findReleaseMock.mockResolvedValue(fixture('v4.13.0')); + cacheFault = 'put'; + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.13.0'); + }); + + it('serves the computed answer when the cache READ throws', async () => { + findReleaseMock.mockResolvedValue(fixture('v4.14.0')); + cacheFault = 'match'; + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.14.0'); + expect(findReleaseMock).toHaveBeenCalledOnce(); + }); +}); + +// PROD_HOST is committed in BOTH [vars] and [env.preview.vars], so without an +// explicit override the preview Worker would key /internal on the production +// origin — a host it does not serve — while its own public routes key on the +// preview origin. That is the #143 misalignment, still live in the one +// environment OG changes get reviewed in. +describe('preview keys the result cache on an origin it actually serves', () => { + const cfg = parseToml( + readFileSync(fileURLToPath(new URL('../wrangler.toml', import.meta.url)), 'utf8'), + ) as { + vars: { PROD_HOST: string }; + env: { preview: { name: string; vars: { PUBLIC_BASE_URL?: string } } }; + }; + + it('sets PUBLIC_BASE_URL in [env.preview.vars] to the preview Worker own origin', () => { + const url = cfg.env.preview.vars.PUBLIC_BASE_URL; + expect(url, 'preview must override the inherited prod cache origin').toBeTypeOf('string'); + const host = new URL(url as string).host; + expect(host).not.toBe(cfg.vars.PROD_HOST); + expect(host).toContain(cfg.env.preview.name); + }); +}); diff --git a/packages/web/wrangler.toml b/packages/web/wrangler.toml index 8498843..5508254 100644 --- a/packages/web/wrangler.toml +++ b/packages/web/wrangler.toml @@ -122,6 +122,12 @@ ANUBIS_HOSTS = "" # the preview never writes the prod released_events dataset. See isProdRequest() # in src/analytics.ts. The analytics binding below is isolated regardless. PROD_HOST = "released.blabberate.com" +# Cache origin + canonical URLs for THIS deployment. Without it the /internal/* +# endpoints web-og calls fall back to PROD_HOST above and key the result cache on +# a host the preview Worker does not serve — the #143 misalignment, in the one +# environment OG changes get reviewed in. Must stay the preview Worker's own URL +# (name above + the account's workers.dev subdomain). +PUBLIC_BASE_URL = "https://released-web-preview.lukaso.workers.dev" [env.preview.assets] directory = "./public" From d5d95941fd01c52b827d2e3c0b0827cfe5cd9508 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Wed, 19 Aug 2026 15:13:54 +0100 Subject: [PATCH 03/22] fix(web): don't block the OG render on a revalidation; normalise the cache origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from the review round on #144, both #143 reintroductions through the new /internal cache path. 1. Sharing the public routes' cache policy also shared their WAIT. resolveLookup revalidates a pending answer after 5 minutes and a partial after 60 seconds, and web-og awaits /internal with no timeout — so a merely-stale entry put findRelease's 24s soft deadline back on the crawler's critical path, and a blown deadline hands the crawler the neutral placeholder at max-age=60. That is the #143 outcome reached from a stale slot instead of a cold one. resolveLookup gains an opt-in `revalidate` callback: a stale answer is served immediately and the refresh runs via executionCtx.waitUntil. The public HTML routes omit it and keep the blocking behaviour, so their semantics are unchanged. A genuinely cold slot still blocks — there is nothing to serve — but it write-backs, so it is cold at most once. 2. cacheOrigin concatenated a scheme onto PROD_HOST unconditionally. PROD_HOST is shared with isProdRequest(), which documents itself as tolerant of a value written WITH a scheme; `new URL('https://https://host')` does not throw, it yields origin `https://https`, so every entry would key on a non-routable host the Cache API drops — silently, with neverFatal swallowing it. The reverse slip was worse: a scheme-less PUBLIC_BASE_URL made `new Request()` throw OUTSIDE neverFatal, turning a computed answer into a 503 → placeholder. Both spellings now normalise through URL().origin. Mutation-tested — each guard was watched failing on the defect it names: - drop `revalidate:` → both stale-while-revalidate tests time out (the render waits on an upstream lookup that never resolves). - restore the naive concatenation → the PROD_HOST-with-scheme test reads 'MISSED-THE-PUBLIC-SLOT' instead of the seeded 'v4.16.0' (proving the entry landed in a different namespace), and the scheme-less PUBLIC_BASE_URL test gets 500 instead of 200. Tests use a distinct SHA each so a hanging test cannot poison the next through singleFlight's module-level in-flight map. Full gate green: 296 passed | 6 skipped (web), typecheck clean. --- packages/web/src/resolve.ts | 17 ++- packages/web/src/routes/internal.ts | 47 +++++- .../web/test/internal-cache-origin.test.ts | 137 +++++++++++++++++- 3 files changed, 193 insertions(+), 8 deletions(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index f4f4fa1..2c1edf1 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -90,8 +90,14 @@ export async function resolveLookup(args: { key: string; load: () => Promise; now?: () => number; + /** Opt-in stale-while-revalidate, for callers on a latency-critical path. + * When given, a cached-but-stale answer is returned IMMEDIATELY and the + * revalidation is handed to this callback to run off the response path + * (`executionCtx.waitUntil`). Callers that can afford to wait — the public + * HTML routes — omit it and keep the blocking behaviour. */ + revalidate?: (task: Promise) => void; }): Promise { - const { cache, key, load } = args; + const { cache, key, load, revalidate } = args; const now = args.now ?? Date.now; const prior = await cache.getEntry(key); @@ -107,6 +113,15 @@ export async function resolveLookup(args: { cached: true, }); + // Stale-while-revalidate: serve what we have, refresh behind it. The refresh is + // a plain recursive call WITHOUT `revalidate`, so it takes the blocking path and + // cannot recurse again. Errors are absorbed here — a background failure must not + // surface as an unhandled rejection on a response that already succeeded. + if (prior && revalidate) { + revalidate(resolveLookup({ cache, key, load, now }).catch(() => undefined)); + return staleHit(); + } + // Did we try (and fail transiently) very recently? If so, don't pound the // upstream again yet — serve the last-known-good if we have one, else a soft // transient. diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index dacc308..2691563 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -53,9 +53,43 @@ function isServiceBinding(c: Context): boolean { * `PUBLIC_BASE_URL=http://localhost:8787` in packages/web/.dev.vars. Only the * unit tests reach the request-origin fallback. */ function cacheOrigin(env: Env, req: Request): string { - if (env.PUBLIC_BASE_URL) return env.PUBLIC_BASE_URL.replace(/\/$/, ''); - if (env.PROD_HOST) return `https://${env.PROD_HOST}`; - return new URL(req.url).origin; + return originOf(env.PUBLIC_BASE_URL) ?? originOf(env.PROD_HOST) ?? new URL(req.url).origin; +} + +/** Normalise a configured host or base URL to a bare origin, or null if it is + * unset/unparseable. + * + * Both spellings have to be tolerated, because both vars are already written + * both ways: PUBLIC_BASE_URL carries a scheme, PROD_HOST does not, and + * isProdRequest() (analytics.ts) documents PROD_HOST as forgiving of a value + * "copied from PUBLIC_BASE_URL". Concatenating a scheme blindly does NOT throw + * on the mixed case — `new URL('https://https://host')` yields origin + * `https://https` — so it would silently key every entry on a non-routable host + * the Cache API drops, which is #143 exactly, with neverFatal swallowing it. + * The reverse slip is worse: a scheme-less PUBLIC_BASE_URL made `new Request()` + * throw OUTSIDE neverFatal, turning a computed answer into a 503 → placeholder. + * Parsing both through URL and taking .origin also drops any path/trailing slash. */ +function originOf(value: string | undefined): string | null { + if (!value) return null; + try { + return new URL(value.includes('//') ? value : `https://${value}`).origin; + } catch { + return null; + } +} + +/** Run a task off the response path. web-og awaits this endpoint with no timeout + * and the crawler caches whatever it finally gets, so a revalidation that blocks + * here is the #143 mechanism reached from a merely-stale entry: findRelease's own + * soft deadline is 24s, and a blown deadline hands the crawler the neutral + * placeholder at max-age=60. */ +function background(c: Context, task: Promise): void { + try { + c.executionCtx.waitUntil(task); + } catch { + // No ExecutionContext (unit tests, some dev runners): the refresh still runs, + // it just isn't kept alive by the runtime. Already .catch()-guarded upstream. + } } /** Wrap a cache so no Cache API call can be fatal. The key URL is deliberately @@ -121,7 +155,10 @@ async function resolveResult(c: Context, input: LookupInput): Promise // negative back-off that keeps a down host from being pounded. A flat TTL here // would downgrade a terminal slot the permalink would have kept for 30 days, // and a bare read would keep serving a 60-second partial for far longer than - // the public page does. Options are stated explicitly because they are what the + // the public page does. Sharing the policy must NOT mean sharing the wait: this + // caller is a crawler's critical path, so it opts into stale-while-revalidate + // (`revalidate`) and never blocks on a refresh. + // Options are stated explicitly because they are what the // `cull`/`nopre` key parts promise. Anubis-protected hosts get a relay-backed // fetch (see makeProvider/relay.ts). The web-og caller chose to wait for this. const resolved = await resolveLookup({ @@ -133,6 +170,8 @@ async function resolveResult(c: Context, input: LookupInput): Promise strict: false, includePrereleases: false, }), + // Serve a cached answer immediately and refresh behind it (see background()). + revalidate: (task) => background(c, task), }); if (resolved.status === 'ok') { return new Response(JSON.stringify(resolved.result), { diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index b2cfba8..d628c0d 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -142,6 +142,33 @@ function cacheControlOf(origin: string, key: string): string | null { return cacheStore.get(keyUrl(origin, key))?.headers.get('cache-control') ?? null; } +/** Let a background (waitUntil) refresh run to completion. Must flush MACROtasks, + * not just microtasks: the refresh chain awaits several real promises, and + * singleFlight keeps a module-level in-flight entry per key that only clears when + * the load settles — leaving one pending would make the NEXT test join it. */ +async function settle(): Promise { + for (let i = 0; i < 3; i++) await new Promise((r) => setTimeout(r, 0)); +} + +/** The tag currently stored in a cache slot — how a BACKGROUND refresh is proven + * to have landed, since it by definition isn't in the response body. */ +async function tagOfSlot(origin: string, key: string): Promise { + const stored = cacheStore.get(keyUrl(origin, key)); + if (!stored) return undefined; + const body = (await stored.clone().json()) as { firstRelease?: { tag?: string } }; + return body.firstRelease?.tag; +} + +/** An upstream lookup that hangs until the test releases it — the only way to + * prove a response did NOT wait for it. */ +function deferred(): { promise: Promise; resolve: (v: unknown) => void } { + let resolve!: (v: unknown) => void; + const promise = new Promise((r) => { + resolve = r; + }); + return { promise, resolve }; +} + beforeEach(() => { cacheStore.clear(); cacheFault = 'none'; @@ -283,12 +310,14 @@ describe('/internal/* follows the cache policy that governs the shared slot', () findReleaseMock.mockResolvedValue(fixture('v4.12.0')); const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + await settle(); expect(res.status).toBe(200); - // The public page would already be showing v4.12.0; the OG card must not keep - // rendering the pending answer for another 30 minutes. - expect(await tagOf(res)).toBe('v4.12.0'); + // The revalidation still happens — it just no longer sits on the render path + // (see the stale-while-revalidate suite below); the slot ends up refreshed so + // the OG card can't keep rendering the pending answer for another 30 minutes. expect(findReleaseMock).toHaveBeenCalledOnce(); + expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.12.0'); }); it('serves the last-known-good answer when the upstream blips, and backs off', async () => { @@ -299,6 +328,7 @@ describe('/internal/* follows the cache policy that governs the shared slot', () ); const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + await settle(); expect(res.status).toBe(200); expect(await subjectOf(res)).toBe('last known good'); @@ -355,3 +385,104 @@ describe('preview keys the result cache on an origin it actually serves', () => expect(host).toContain(cfg.env.preview.name); }); }); + +// A crawler caches whatever the unfurl returns, so anything that makes web-og +// WAIT is a #143 risk: the shared policy revalidates a pending answer after 5 +// minutes and a partial after 60 seconds, and findRelease's own soft deadline is +// 24s. Blocking on that revalidation would hand the crawler a placeholder with +// max-age=60 — the #143 outcome, reached from a merely-stale entry instead of a +// cold one. So on this path a cached answer is served IMMEDIATELY and the refresh +// runs in the background. (A genuinely COLD slot still blocks — there is nothing +// to serve — but it write-backs, so it is cold at most once.) +describe('/internal/* never blocks the render on a revalidation', () => { + it('serves a stale pending answer without waiting for the upstream lookup', async () => { + const sha = 'b'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + seedAged(PUBLIC_ORIGIN, k, pendingFixture('stale pending'), 10 * 60); + const slow = deferred(); + findReleaseMock.mockReturnValue(slow.promise); + + // The upstream lookup has NOT resolved at this point. If the render waited on + // it, this await never returns and the test times out — which is the whole claim. + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(res.status).toBe(200); + expect(await subjectOf(res)).toBe('stale pending'); + + slow.resolve(fixture('v4.12.0')); + await settle(); + // The revalidation was not skipped — it ran behind the render. + expect(findReleaseMock).toHaveBeenCalledOnce(); + expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.12.0'); + }); + + it('serves a stale PARTIAL without waiting, then refreshes the slot behind it', async () => { + const sha = 'c'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + seedAged(PUBLIC_ORIGIN, k, partialFixture(), 5 * 60); + const slow = deferred(); + findReleaseMock.mockReturnValue(slow.promise); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBeUndefined(); // the partial, served as-is + expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBeUndefined(); // not refreshed YET + + slow.resolve(fixture('v4.12.0')); + await settle(); + expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.12.0'); // the background refresh landed + }); + + it('still blocks (and write-backs) when the slot is genuinely cold', async () => { + const sha = 'd'.repeat(40); + findReleaseMock.mockResolvedValue(fixture('v4.15.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.15.0'); + }); +}); + +// PROD_HOST is shared with isProdRequest() (analytics.ts), which documents itself +// as tolerant of a value written WITH a scheme ("copied from PUBLIC_BASE_URL"). +// `https://` + that value parses without throwing — to origin `https://https` — +// so an un-normalised read here would key every /internal entry on a non-routable +// host the Cache API drops: #143 again, silent, with neverFatal swallowing it. +describe('/internal/* normalises a configured cache origin', () => { + it('keys on the real origin when PROD_HOST is written WITH a scheme', async () => { + const sha = 'e'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + seed(PUBLIC_ORIGIN, k, fixture('v4.16.0')); + // Distinguishable from the seeded slot: if the key lands on `https://https` + // the seeded entry is invisible and this recomputed answer is what comes back. + findReleaseMock.mockResolvedValue(fixture('MISSED-THE-PUBLIC-SLOT')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), { + INTERNAL_SECRET, + PROD_HOST: `https://${PROD_HOST}`, + }); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.16.0'); // the PUBLIC slot was hit + expect(findReleaseMock).not.toHaveBeenCalled(); + }); + + it('keys on the real origin when PUBLIC_BASE_URL is written WITHOUT one', async () => { + const sha = 'f'.repeat(40); + findReleaseMock.mockResolvedValue(fixture('v4.17.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), { + INTERNAL_SECRET, + PUBLIC_BASE_URL: PROD_HOST, + }); + + // Pre-fix this threw out of `new Request(...)` — outside neverFatal — so the + // computed answer became a 503, which web-og renders as the placeholder. + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.17.0'); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.17.0'); + }); +}); From 2deac84c46d59e6ccf9d08880b36467a2d55f5cd Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 20 Aug 2026 12:56:18 +0100 Subject: [PATCH 04/22] fix(web): don't let a shared back-off marker hand the crawler a placeholder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Aligning the /internal cache key with the public routes (this PR's fix) also aligned the negative back-off marker `:neg`, which public page views write. Backing off is cheap for a page a human can reload and permanent for a crawler that unfurls once: a human's failed page view could 503 the next OG render without ever calling findRelease, and the crawler caches that placeholder for good — #143 again, through the very alignment meant to fix it. resolveLookup gains an opt-in `bypassBackOffWhenCold`. With a prior to stale-serve the back-off is unchanged (stale beats a recompute, and the down host is left alone); only the cold case — where the alternative is a permanent placeholder — attempts the load. The marker is still written on failure, and singleFlight collapses concurrent attempts, so it is one extra upstream call per back-off window, not a stampede. Only /internal opts in; result/badge/pr/issue keep the blocking back-off. Co-Authored-By: Claude Opus 5 --- packages/web/src/resolve.ts | 29 ++++++++++--- packages/web/src/routes/internal.ts | 6 +++ .../web/test/internal-cache-origin.test.ts | 43 +++++++++++++++++++ 3 files changed, 71 insertions(+), 7 deletions(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index 2c1edf1..c8e6ca6 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -13,6 +13,9 @@ // 3. Don't hammer a down host. A transient failure writes a short-lived // "negative" marker; while it's warm we skip the upstream call entirely // (serving stale if we have it, otherwise a soft "checking…" transient). +// A caller whose consumer asks only ONCE can opt out of the cold half of +// that (`bypassBackOffWhenCold`) — backing off is cheap for a page a human +// can reload and permanent for a crawler that unfurls once. // // "not yet released" is a thrown NotYetReleasedError (not a cacheable result), // so it surfaces as its own status and, during an outage with no prior, degrades @@ -96,8 +99,15 @@ export async function resolveLookup(args: { * (`executionCtx.waitUntil`). Callers that can afford to wait — the public * HTML routes — omit it and keep the blocking behaviour. */ revalidate?: (task: Promise) => void; + /** Opt-in for callers whose consumer only ever asks ONCE, so a soft failure + * becomes permanent for them (the OG crawler). When set, the shared negative + * back-off marker is honoured only if there is a prior to stale-serve — + * with nothing to serve, an attempt beats handing back a placeholder that + * gets cached forever. The marker is still WRITTEN on failure, and callers + * that can retry (the public HTML routes) omit this and keep backing off. */ + bypassBackOffWhenCold?: boolean; }): Promise { - const { cache, key, load, revalidate } = args; + const { cache, key, load, revalidate, bypassBackOffWhenCold } = args; const now = args.now ?? Date.now; const prior = await cache.getEntry(key); @@ -130,12 +140,17 @@ export async function resolveLookup(args: { Boolean(neg?.value?.transient) && (neg?.ageSeconds ?? Number.POSITIVE_INFINITY) < NEG_TTL; if (backedOff) { if (prior) return staleHit(); - return { - status: 'transient', - kind: neg?.value.kind ?? 'provider_server_error', - upstreamStatus: neg?.value.status, - anubis: neg?.value.anubis, - }; + if (!bypassBackOffWhenCold) { + return { + status: 'transient', + kind: neg?.value.kind ?? 'provider_server_error', + upstreamStatus: neg?.value.status, + anubis: neg?.value.anubis, + }; + } + // Cold + opted out: fall through and attempt the load. singleFlight still + // collapses concurrent attempts on this key, so this is one extra upstream + // call per back-off window, not a stampede. } try { diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 2691563..c5f457b 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -172,6 +172,12 @@ async function resolveResult(c: Context, input: LookupInput): Promise }), // Serve a cached answer immediately and refresh behind it (see background()). revalidate: (task) => background(c, task), + // The shared key means a public page view's failed lookup also writes the + // shared `:neg` back-off marker. Honouring that on a COLD slot would 503 here + // without ever calling findRelease, and the crawler caches the resulting + // placeholder for good — #143 all over again, via the alignment that fixes it. + // With a prior to stale-serve, the back-off still holds. + bypassBackOffWhenCold: true, }); if (resolved.status === 'ok') { return new Response(JSON.stringify(resolved.result), { diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index d628c0d..a9c9ebd 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -486,3 +486,46 @@ describe('/internal/* normalises a configured cache origin', () => { expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.17.0'); }); }); + +// Aligning the /internal key with the public one (the fix above) also aligned the +// NEGATIVE back-off marker `:neg`, which public page views write. That marker +// is a good idea for a page a human can reload, and a bad one for a crawler: a +// crawler unfurls ONCE and keeps what it got. So a 60-second marker left by an +// unrelated human page view could hand the crawler a permanent placeholder — the +// exact #143 symptom this PR exists to remove, re-introduced through the shared key. +// The back-off is therefore honoured only when there is a prior to stale-serve. +describe('/internal/* does not let a shared back-off marker cause a permanent placeholder', () => { + it('computes on a COLD slot even when a public page view left a warm `:neg` marker', async () => { + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + // A human loaded the permalink seconds ago, GitHub 502'd, the public route + // wrote the shared back-off marker. No result was ever cached. + seedAged(PUBLIC_ORIGIN, `${k}:neg`, { transient: true, kind: 'github_server_error' }, 10); + // Upstream has since recovered. + findReleaseMock.mockResolvedValue(fixture('v4.12.11')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + await settle(); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.12.11'); + expect(findReleaseMock).toHaveBeenCalledTimes(1); + // ...and the answer is warm for the next unfurl. + expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.12.11'); + }); + + it('still honours the marker when there IS a prior — a down host is never pounded', async () => { + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + seedAged(PUBLIC_ORIGIN, k, pendingFixture('last known good'), 10 * 60); + seedAged(PUBLIC_ORIGIN, `${k}:neg`, { transient: true, kind: 'github_server_error' }, 10); + findReleaseMock.mockResolvedValue(fixture('v4.12.11')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + await settle(); + + expect(res.status).toBe(200); + expect(await subjectOf(res)).toBe('last known good'); + // Stale-serve is strictly better than a recompute here, so the back-off holds + // and the upstream is left alone — including on the background revalidation. + expect(findReleaseMock).not.toHaveBeenCalled(); + }); +}); From 2e85c325c56335d99474e3f2d77cc5ac464018b8 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 20 Aug 2026 13:16:54 +0100 Subject: [PATCH 05/22] fix(web): guard the opaque `null` origin; correct the preview cache claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the automated review of this PR, all in its own diff: - `originOf` could return the literal string `"null"`. `URL.origin` is `"null"` for any opaque origin (`file:`, `foo:`), and such a value contains `//`, so it skipped the scheme-prefix branch, parsed cleanly, and satisfied `??` — then threw out of `new Request()` OUTSIDE `neverFatal`. `app.onError` turned a computable OG lookup into a 500 and web-og rendered the neutral placeholder: the same failure class as the scheme-less `PUBLIC_BASE_URL` slip this PR already fixed. Now returns null so the configured fallbacks apply. - Corrected the `[env.preview.vars]` comment. Cloudflare documents Cache API operations as functional only on custom domains, so a `*.workers.dev` preview cannot exercise the cache alignment at all — the unit tests prove it and it takes effect on the prod custom domain. The var stays: it keeps preview off the prod key namespace and drives canonical URLs. - Documented why the `:neg` back-off WRITE is deliberately shared while the cold-slot READ is bypassed. Test added first and watched fail on the real defect: with `PUBLIC_BASE_URL=file:///srv/web` the route returned 500 (expected 200) before the guard, and now falls through to PROD_HOST's public slot. --- packages/web/src/routes/internal.ts | 19 +++++++++++++- .../web/test/internal-cache-origin.test.ts | 25 +++++++++++++++++++ packages/web/wrangler.toml | 14 ++++++++--- 3 files changed, 54 insertions(+), 4 deletions(-) diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index c5f457b..1f268d5 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -72,7 +72,14 @@ function cacheOrigin(env: Env, req: Request): string { function originOf(value: string | undefined): string | null { if (!value) return null; try { - return new URL(value.includes('//') ? value : `https://${value}`).origin; + const origin = new URL(value.includes('//') ? value : `https://${value}`).origin; + // `URL.origin` is the literal string "null" for any opaque origin (a + // non-special scheme: file:, foo:). Such a value contains '//', so it skips + // the scheme-prefix branch and parses cleanly — returning a non-null, + // non-URL string that satisfies `??` and then throws out of `new Request()` + // below, OUTSIDE neverFatal. That is the scheme-less slip again: a 500 where + // a computed answer was available, rendered as the neutral placeholder. + return origin === 'null' ? null : origin; } catch { return null; } @@ -177,6 +184,16 @@ async function resolveResult(c: Context, input: LookupInput): Promise // without ever calling findRelease, and the crawler caches the resulting // placeholder for good — #143 all over again, via the alignment that fixes it. // With a prior to stale-serve, the back-off still holds. + // + // The asymmetry is deliberate: this caller opts out of READING the marker on + // a cold slot, but resolveLookup still WRITES it, so a failure discovered + // here can back off a human permalink for up to 60s. That is the point of + // sharing the slot — the marker describes the HOST being down, not who found + // it out, and the host is equally down for the human. They get the + // "checking…" recovery card (never a wrong "not yet released") and can + // reload; the crawler asks once and keeps what it got. Suppressing the write + // would instead leave the key with no back-off at all whenever the crawler + // touches it first, and every human reload would pound the down host. bypassBackOffWhenCold: true, }); if (resolved.status === 'ok') { diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index a9c9ebd..de1f7e7 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -485,6 +485,31 @@ describe('/internal/* normalises a configured cache origin', () => { const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.17.0'); }); + + it('ignores a configured value whose origin is OPAQUE, rather than 500ing', async () => { + const sha = '9'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + seed(PUBLIC_ORIGIN, k, fixture('v4.18.0')); + // Distinguishable from the seeded slot, as above. + findReleaseMock.mockResolvedValue(fixture('MISSED-THE-PUBLIC-SLOT')); + + // `URL.origin` is the literal string "null" for any opaque (non-special + // scheme) origin. `file:///srv/web` contains '//', so it skips the + // scheme-prefix branch, parses fine, and yields "null" — a non-null, + // non-URL string. Unguarded that satisfies `??` and reaches + // `new Request(...)`, which throws OUTSIDE neverFatal: app.onError turns a + // computable OG lookup into a 500 and web-og renders the neutral + // placeholder. Same failure class as the scheme-less PUBLIC_BASE_URL above. + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), { + INTERNAL_SECRET, + PUBLIC_BASE_URL: 'file:///srv/web', + PROD_HOST, + }); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.18.0'); // fell through to PROD_HOST's public slot + expect(findReleaseMock).not.toHaveBeenCalled(); + }); }); // Aligning the /internal key with the public one (the fix above) also aligned the diff --git a/packages/web/wrangler.toml b/packages/web/wrangler.toml index 5508254..5a4f257 100644 --- a/packages/web/wrangler.toml +++ b/packages/web/wrangler.toml @@ -124,9 +124,17 @@ ANUBIS_HOSTS = "" PROD_HOST = "released.blabberate.com" # Cache origin + canonical URLs for THIS deployment. Without it the /internal/* # endpoints web-og calls fall back to PROD_HOST above and key the result cache on -# a host the preview Worker does not serve — the #143 misalignment, in the one -# environment OG changes get reviewed in. Must stay the preview Worker's own URL -# (name above + the account's workers.dev subdomain). +# a host the preview Worker does not serve. Must stay the preview Worker's own +# URL (name above + the account's workers.dev subdomain). +# +# NOTE: this does NOT make preview a place the #143 cache alignment can be +# exercised. Cloudflare documents Cache API operations as functional only for +# Workers on custom domains, so on a *.workers.dev preview neither the public +# routes nor /internal ever warm a slot, and a second unfurl still pays a full +# lookup however the origins line up. The alignment is proven by the unit tests +# (test/internal-cache-origin.test.ts) and only takes effect on the prod custom +# domain. The var is still correct to set: it keeps preview off the prod key +# namespace and drives canonical URLs. PUBLIC_BASE_URL = "https://released-web-preview.lukaso.workers.dev" [env.preview.assets] From df3f20b35dee6d09b7a371b8bf2533eb9a956426 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 20 Aug 2026 13:46:22 +0100 Subject: [PATCH 06/22] fix(web): bound the SWR window and keep a background refresh off the flight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-4 review findings, all three in the stale-while-revalidate path this PR added (4cdb212), not in the cache-key alignment it is about. Bound the SWR window (SWR_MAX_STALE, 30 min). `prior && revalidate` fired for any entry past the 5-minute freshness window, and a pending answer's hard TTL is 24h — so an unfurl could be served a 23h-old "not yet released" answer, which web-og long-caches for another 24h (`result ? longCache : shortCache`). The background refresh fixes the slot but cannot invalidate the PNG already rendered. 30 minutes is what /internal used as a flat TTL before it shared this slot, so the bound is never worse than the code it replaced. Keep the background refresh off singleFlight (`coalesce: false`). The recursive call became the flight owner, and singleFlight only clears its module-level entry in the loader's `finally`. Under `waitUntil` the IoContext can be torn down before the subrequest settles, so the promise never settles, the `finally` never runs, and every later request in that isolate on that key — a human on the permalink, badge.ts on the same cull/nopre key — joins a dead promise. Correct the back-off bypass comment. It claimed singleFlight made this "one extra upstream call per back-off window, not a stampede". singleFlight collapses only concurrent calls within one isolate, so sequential and cross-colo unfurls each run a full lookup against a down host. The cost is real and accepted; the comment now says so rather than claiming a throttle that isn't there. --- packages/web/src/resolve.ts | 56 ++++++++++++++----- packages/web/test/resolve.test.ts | 89 +++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 12 deletions(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index c8e6ca6..6ed3634 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -37,6 +37,15 @@ const HARD_TTL_RELEASED = 30 * 24 * 60 * 60; // terminal — keep ~30 days const HARD_TTL_PENDING = 24 * 60 * 60; // long enough to stale-serve through an outage const HARD_TTL_PARTIAL = 60; // partial is itself a soft state; don't trust it long const NEG_TTL = 60; // back off this long when upstream is down +// Upper bound on what stale-while-revalidate will hand back UNBLOCKED. Past it +// we block on the refresh instead. web-og long-caches any non-null result for +// 24h (renderImage: `result ? longCache : shortCache`), so an answer served +// here is pinned in the crawler's cache for a day and the background refresh +// cannot invalidate a PNG that has already been rendered from it — a "not yet +// released" prior that has since shipped would show the wrong card until +// tomorrow. 30 minutes is what /internal used as its flat TTL before it shared +// this slot, so bounding here is never worse than the code it replaced. +const SWR_MAX_STALE = 30 * 60; // Error kinds a later retry might succeed on → eligible for stale-serve (when we // have a prior) or a short negative cache (when we don't). Everything else is a @@ -106,8 +115,20 @@ export async function resolveLookup(args: { * gets cached forever. The marker is still WRITTEN on failure, and callers * that can retry (the public HTML routes) omit this and keep backing off. */ bypassBackOffWhenCold?: boolean; + /** Internal. Set false for the background refresh on the SWR path: that task + * runs under `executionCtx.waitUntil`, whose IoContext workerd can tear down + * before the subrequest settles. singleFlight only clears its module-level + * entry in the loader's `finally`, so a background owner that never settles + * leaves a dead promise under this key, and every later request in the same + * isolate — a human on the permalink, badge.ts on the same cull/nopre key — + * joins it: a hang, or "Cannot perform I/O on behalf of a different request", + * which resolveLookup classifies as a non-transient error and the page renders + * as a hard failure. A foreground caller is always a live, awaiting request; a + * background one is not. A duplicated refresh is far cheaper than poisoning + * the key for the lifetime of the isolate. */ + coalesce?: boolean; }): Promise { - const { cache, key, load, revalidate, bypassBackOffWhenCold } = args; + const { cache, key, load, revalidate, bypassBackOffWhenCold, coalesce } = args; const now = args.now ?? Date.now; const prior = await cache.getEntry(key); @@ -123,12 +144,15 @@ export async function resolveLookup(args: { cached: true, }); - // Stale-while-revalidate: serve what we have, refresh behind it. The refresh is - // a plain recursive call WITHOUT `revalidate`, so it takes the blocking path and - // cannot recurse again. Errors are absorbed here — a background failure must not - // surface as an unhandled rejection on a response that already succeeded. - if (prior && revalidate) { - revalidate(resolveLookup({ cache, key, load, now }).catch(() => undefined)); + // Stale-while-revalidate: serve what we have, refresh behind it — but only up to + // SWR_MAX_STALE, past which we block rather than hand back an answer the crawler + // would pin for a day. The refresh is a plain recursive call WITHOUT `revalidate`, + // so it takes the blocking path and cannot recurse again, and with + // `coalesce: false` so a task the runtime may kill never owns the flight for this + // key. Errors are absorbed here — a background failure must not surface as an + // unhandled rejection on a response that already succeeded. + if (prior && revalidate && prior.ageSeconds < SWR_MAX_STALE) { + revalidate(resolveLookup({ cache, key, load, now, coalesce: false }).catch(() => undefined)); return staleHit(); } @@ -148,19 +172,27 @@ export async function resolveLookup(args: { anubis: neg?.value.anubis, }; } - // Cold + opted out: fall through and attempt the load. singleFlight still - // collapses concurrent attempts on this key, so this is one extra upstream - // call per back-off window, not a stampede. + // Cold + opted out: fall through and attempt the load. This is NOT throttled. + // singleFlight collapses only CONCURRENT calls, and only inside ONE isolate + // (see its header), so during a host outage every cold unfurl — a different + // colo, a different social platform, or simply a later one — runs its own + // findRelease out to the hard deadline against the down host and re-stamps + // the marker. NEG_TTL throttles the human page views on this key, not this + // path. That cost is accepted deliberately: the crawler asks ONCE, so the + // alternative is a placeholder pinned in its cache long after the host + // recovers. Gating the bypass on a fraction of NEG_TTL would only move which + // unfurls get the permanent placeholder, not stop them. } try { - const result = await singleFlight(key, async () => { + const run = async () => { const re = await cache.getEntry(key); if (re && isFresh(re)) return re.value; const r = await load(); await cache.put(key, r, hardTtlFor(r)); return r; - }); + }; + const result = coalesce === false ? await run() : await singleFlight(key, run); return { status: 'ok', result, stale: false, staleAsOf: null, cached: false }; } catch (err) { if (err instanceof NotYetReleasedError) return { status: 'not_yet', error: err }; diff --git a/packages/web/test/resolve.test.ts b/packages/web/test/resolve.test.ts index f0b2d2b..19981d3 100644 --- a/packages/web/test/resolve.test.ts +++ b/packages/web/test/resolve.test.ts @@ -254,3 +254,92 @@ describe('resolveLookup — real answers pass through', () => { expect(f.has(negKey)).toBe(false); }); }); + +// Round-4 review of #144. The stale-while-revalidate path added in 4cdb212 is +// what lets the OG render return without blocking on a refresh. Both guards +// below pin a defect in THAT path, not in the cache-key alignment #144 is about. +describe('resolveLookup — stale-while-revalidate is bounded', () => { + it('serves a recently-stale prior immediately and refreshes behind it', async () => { + const f = makeFakeCache(); + // Past the 5-minute pending freshness window, well inside the SWR bound. + f.seed(KEY, mkResult({ released: false }), 10 * 60); + const load = vi.fn().mockResolvedValue(mkResult({ released: true })); + const tasks: Promise[] = []; + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + revalidate: (t) => { + tasks.push(t); + }, + }); + + expect(r.status).toBe('ok'); + if (r.status === 'ok') expect(r.stale).toBe(true); + expect(tasks).toHaveLength(1); + await Promise.all(tasks); + expect(load).toHaveBeenCalledTimes(1); + }); + + it('blocks rather than hand back an answer stale past the bound', async () => { + const f = makeFakeCache(); + // 23h old: still inside HARD_TTL_PENDING (24h), so getEntry returns it. + // web-og long-caches ANY non-null result for 24h, so serving this unblocked + // pins a day-old "not yet released" card in the crawler's cache for another + // day, and the background refresh cannot invalidate the PNG already made. + f.seed(KEY, mkResult({ released: false }), 23 * 60 * 60); + const load = vi.fn().mockResolvedValue(mkResult({ released: true })); + const tasks: Promise[] = []; + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + revalidate: (t) => { + tasks.push(t); + }, + }); + + expect(tasks).toHaveLength(0); + expect(r.status).toBe('ok'); + if (r.status === 'ok') { + expect(r.stale).toBe(false); + expect(r.result.firstRelease?.tag).toBe('4.18.0'); + } + expect(load).toHaveBeenCalledTimes(1); + }); + + it('does not let a torn-down background refresh poison the key for the isolate', async () => { + const f = makeFakeCache(); + f.seed(KEY, mkResult({ released: false }), 10 * 60); + + // The background refresh never settles — exactly what a waitUntil task looks + // like when workerd tears the IoContext down mid-subrequest. singleFlight + // only clears its entry in the loader's `finally`, so if the background call + // owns the flight, that entry is never cleared. + const hung = vi.fn().mockReturnValue(new Promise(() => {})); + const first = await resolveLookup({ + cache: f.cache, + key: KEY, + load: hung, + revalidate: () => {}, + }); + expect(first.status).toBe('ok'); + // Let the background call reach its load() and register whatever flight it + // is going to register, before a second request asks for the same key. + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(hung).toHaveBeenCalledTimes(1); + + // A later request in the SAME isolate on the SAME key — a human on the + // permalink, or badge.ts, which share this slot — must still get an answer. + const good = vi.fn().mockResolvedValue(mkResult({ released: true })); + const settled = await Promise.race([ + resolveLookup({ cache: f.cache, key: KEY, load: good }), + new Promise((resolve) => setTimeout(() => resolve('JOINED-A-DEAD-FLIGHT'), 100)), + ]); + + expect(settled).not.toBe('JOINED-A-DEAD-FLIGHT'); + expect(good).toHaveBeenCalledTimes(1); + }); +}); From 3b0886ede81a0ab699e33f329dc79db18831630b Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 20 Aug 2026 14:41:46 +0100 Subject: [PATCH 07/22] fix(web): collapse concurrent SWR background refreshes onto one lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-5 review of #144, both findings in this PR's own diff. `coalesce: false` (round 4) kept the background refresh from OWNING the singleFlight entry, but it also dropped it out of coalescing entirely. The SWR branch fires on EVERY request in the stale window, so one link unfurled by four platforms in the same second in one colo ran four full findRelease traversals against the same repo on the shared token, none seeing the others. The two properties aren't in tension, but "join without registering" does not fix it either: with no foreground flight to join, four background tasks still all miss. So background refreshes get their own map (`backgroundFlight`) that foreground callers never join — they collapse onto each other, and no live request can ever join a task the runtime may tear down. Also pins the commit URL shape web-og actually sends (7-char sha, not the 40-char form the rest of the guard file used), and names #147 for the remaining half: the public route keys on the full sha, so the two sides still spell one commit two ways. Mutation evidence: - collapse guard, before the fix: "expected vi.fn() to be called 1 times, but got 4 times" - both short-sha guards, with cacheOrigin reverted to makeWorkerCache(req) (the #143 defect): "expected 503 to be 200" and "expected [ Array(1) ] to include 'https://released.blabberate.com/__cac…'" Gate: 613 tests, typecheck, lint clean. Co-Authored-By: Claude Opus 5 --- packages/web/src/resolve.ts | 16 ++++++-- packages/web/src/single-flight.ts | 35 ++++++++++++++++ .../web/test/internal-cache-origin.test.ts | 39 ++++++++++++++++++ packages/web/test/resolve.test.ts | 40 +++++++++++++++++++ 4 files changed, 126 insertions(+), 4 deletions(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index 6ed3634..f908eb8 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -29,7 +29,7 @@ import { } from '@released/core'; import { upstreamStatusOf } from './analytics.js'; import type { CacheEntry, WorkerCache } from './cache.js'; -import { singleFlight } from './single-flight.js'; +import { backgroundFlight, singleFlight } from './single-flight.js'; // Freshness windows + hard TTLs (seconds). const FRESH_WINDOW_PENDING = 5 * 60; // re-check non-released answers every 5 min @@ -148,11 +148,19 @@ export async function resolveLookup(args: { // SWR_MAX_STALE, past which we block rather than hand back an answer the crawler // would pin for a day. The refresh is a plain recursive call WITHOUT `revalidate`, // so it takes the blocking path and cannot recurse again, and with - // `coalesce: false` so a task the runtime may kill never owns the flight for this - // key. Errors are absorbed here — a background failure must not surface as an + // `coalesce: false` so a task the runtime may kill never owns the foreground + // flight for this key. `backgroundFlight` then restores the collapsing that + // dropping out of `singleFlight` cost: this branch fires on EVERY request in the + // stale window, so four crawlers unfurling one link in the same second would + // otherwise run four full lookups against the same repo on the shared token. + // Errors are absorbed here — a background failure must not surface as an // unhandled rejection on a response that already succeeded. if (prior && revalidate && prior.ageSeconds < SWR_MAX_STALE) { - revalidate(resolveLookup({ cache, key, load, now, coalesce: false }).catch(() => undefined)); + revalidate( + backgroundFlight(key, () => resolveLookup({ cache, key, load, now, coalesce: false })).catch( + () => undefined, + ), + ); return staleHit(); } diff --git a/packages/web/src/single-flight.ts b/packages/web/src/single-flight.ts index 0a3fa8d..2105e77 100644 --- a/packages/web/src/single-flight.ts +++ b/packages/web/src/single-flight.ts @@ -22,3 +22,38 @@ export async function singleFlight(key: string, loader: Loader): Promise>(); + +/** Collapse concurrent BACKGROUND refreshes for `key` onto one run, in a map + * foreground callers never join. Registration is synchronous, so two refreshes + * fired in the same tick cannot both miss it. */ +export function backgroundFlight(key: string, loader: Loader): Promise { + const existing = background.get(key) as Promise | undefined; + if (existing) return existing; + const p = (async () => { + try { + return await loader(); + } finally { + background.delete(key); + } + })(); + background.set(key, p); + return p; +} diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index de1f7e7..acfc5c0 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -62,6 +62,11 @@ const PROD_HOST = 'released.blabberate.com'; const PUBLIC_ORIGIN = `https://${PROD_HOST}`; const ENV = { INTERNAL_SECRET, PROD_HOST }; const SHA = 'a'.repeat(40); +// What web-og ACTUALLY puts in the internal URL for a commit: ogImageUrlForCommit +// -> shortSha(sha) -> 7 characters (ui/og-meta.tsx). The rest of this file uses the +// 40-char form; these two must both work, and #147 tracks the fact that they are +// two different slots. +const SHORT_SHA = SHA.slice(0, 7); /** The cache key the PUBLIC permalink routes write (result.tsx / issue.tsx / pr.tsx): * 5 parts, ending in the default `cull` + `nopre` option suffixes. */ @@ -245,6 +250,40 @@ describe('/internal/* WRITES back to the slot the public routes read (#143)', () }); }); +// The commit URL web-og really sends is the 7-char one, so the origin fix has to +// hold for that shape too — the rest of this file exercises the 40-char form. +// +// NOTE the half this PR deliberately does NOT fix: the public permalink route keys +// on the sha as it appears in the page URL, and /lookup redirects to the FULL 40 +// chars (index.ts, "short prefixes collide in large repos"). So `sha:<7>` and +// `sha:<40>` are different digests and the first unfurl of a full-sha permalink is +// still cold. That is #147 — its fix lives in ui/og-meta.tsx / result.tsx, files +// this PR does not touch, and changes the PUBLIC routes' key namespace. +describe('/internal/* keys a commit on the sha web-og really sends (7 chars, #147)', () => { + it('serves a cached result on the short-sha public key shape', async () => { + const k = await publicKey('github.com/honojs/hono', `sha:${SHORT_SHA}`); + seed(PUBLIC_ORIGIN, k, fixture('v4.9.9')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHORT_SHA}`), ENV); + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.9.9'); + // A cache HIT is proven by never consulting the provider. + expect(findReleaseMock).not.toHaveBeenCalled(); + }); + + it('writes a cold short-sha lookup back to the public origin, not `https://web`', async () => { + findReleaseMock.mockResolvedValue(fixture('v4.10.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHORT_SHA}`), ENV); + expect(res.status).toBe(200); + expect(findReleaseMock).toHaveBeenCalledOnce(); + + const k = await publicKey('github.com/honojs/hono', `sha:${SHORT_SHA}`); + expect([...cacheStore.keys()]).toContain(keyUrl(PUBLIC_ORIGIN, k)); + expect([...cacheStore.keys()].filter((u) => u.startsWith('https://web/'))).toEqual([]); + }); +}); + describe('/internal/* cache origin falls back to the request origin', () => { it('uses the request origin when no PROD_HOST/PUBLIC_BASE_URL is configured', async () => { // `wrangler dev` and the unit tests have neither var set; the route must still diff --git a/packages/web/test/resolve.test.ts b/packages/web/test/resolve.test.ts index 19981d3..e2e1e9e 100644 --- a/packages/web/test/resolve.test.ts +++ b/packages/web/test/resolve.test.ts @@ -310,6 +310,46 @@ describe('resolveLookup — stale-while-revalidate is bounded', () => { expect(load).toHaveBeenCalledTimes(1); }); + // Round-5 review of #144. `coalesce: false` (round 4) kept the background + // refresh from OWNING the singleFlight entry, but it also dropped it out of + // coalescing entirely — and the SWR branch fires on EVERY request in the stale + // window. One link unfurled by Slack, X, Discord and LinkedIn in the same + // second in one colo then ran four full findRelease traversals against the same + // repo on the shared GITHUB_TOKEN, none seeing the others. + it('collapses concurrent background refreshes for one key onto a single lookup', async () => { + const f = makeFakeCache(); + f.seed(KEY, mkResult({ released: false }), 10 * 60); + + // Slow enough that all four refreshes are genuinely in flight together. + const load = vi.fn().mockImplementation( + () => + new Promise((res) => { + setTimeout(() => res(mkResult({ released: true })), 10); + }), + ); + const tasks: Promise[] = []; + const revalidate = (t: Promise) => { + tasks.push(t); + }; + + const results = await Promise.all( + Array.from({ length: 4 }, () => + resolveLookup({ cache: f.cache, key: KEY, load, revalidate }), + ), + ); + + // Every caller still gets its stale answer immediately — collapsing the + // refresh must not make anyone wait. + for (const r of results) { + expect(r.status).toBe('ok'); + if (r.status === 'ok') expect(r.stale).toBe(true); + } + expect(tasks).toHaveLength(4); + + await Promise.all(tasks); + expect(load).toHaveBeenCalledTimes(1); + }); + it('does not let a torn-down background refresh poison the key for the isolate', async () => { const f = makeFakeCache(); f.seed(KEY, mkResult({ released: false }), 10 * 60); From 3f74b7e841ccf7c8f7179237db538a15ebb2fd09 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 20 Aug 2026 15:08:46 +0100 Subject: [PATCH 08/22] fix(web): bound EVERY stale exit for a consumer that pins the answer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rounds 4/5 added SWR_MAX_STALE because web-og long-caches any non-null result for 24h, so an answer handed to the crawler is pinned for a day and no later refresh can invalidate the PNG already rendered from it. That bound only guarded the stale-while-revalidate return. Both stale-if-error exits — the back-off short-circuit and the transient catch — still served a prior of any age, and this PR is what made that reachable on /internal: before it, the route had a flat 30-minute TTL, so a 23h-old prior could not exist there; now it shares the public routes' 24h HARD_TTL_PENDING slot. Rename it MAX_STALE_PINNED and apply it at every exit that hands back a prior, gated on a new opt-in `consumerPinsResult` that only the OG route sets. Public HTML routes keep stale-if-error UNBOUNDED: a human gets an explicit "stale as of" caveat and can reload, and their answer is not pinned anywhere, so serving through a long outage stays the right degrade for them. Also document the local `PUBLIC_BASE_URL=http://localhost:8787` .dev.vars line in the README, not only in the JSDoc that introduced the requirement. Mutation-proven both ways: reverting the two guards fails exactly the two new exit tests, and applying the bound unconditionally fails the public-routes test. --- README.md | 13 ++++ packages/web/src/resolve.ts | 51 +++++++++++----- packages/web/src/routes/internal.ts | 13 +++- packages/web/src/single-flight.ts | 2 +- packages/web/test/resolve.test.ts | 95 +++++++++++++++++++++++++++++ 5 files changed, 157 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 9f2928c..5cc5a20 100644 --- a/README.md +++ b/README.md @@ -136,6 +136,19 @@ pnpm --filter @released/web-og dev # wrangler dev for web-og pnpm --filter git-released dev -- # tsx-run the CLI in place ``` +Working on OG rendering locally? Put this in `packages/web/.dev.vars`: + +``` +PUBLIC_BASE_URL=http://localhost:8787 +``` + +`/internal/*` (what web-og calls) keys the result cache on the deployment's own +public origin. `wrangler dev` loads `[vars]`, where `PROD_HOST` is set, so +without the override the OG path keys on `https://released.blabberate.com` while +your public routes key on `http://localhost:8787`: two namespaces on one +machine, and every local OG request pays a full lookup instead of hitting the +slot the permalink just warmed. Prod and the preview Worker set it themselves. + ### Local checks reference | Command | What it does | Gate? | diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index f908eb8..0fa2c1f 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -37,15 +37,24 @@ const HARD_TTL_RELEASED = 30 * 24 * 60 * 60; // terminal — keep ~30 days const HARD_TTL_PENDING = 24 * 60 * 60; // long enough to stale-serve through an outage const HARD_TTL_PARTIAL = 60; // partial is itself a soft state; don't trust it long const NEG_TTL = 60; // back off this long when upstream is down -// Upper bound on what stale-while-revalidate will hand back UNBLOCKED. Past it -// we block on the refresh instead. web-og long-caches any non-null result for -// 24h (renderImage: `result ? longCache : shortCache`), so an answer served -// here is pinned in the crawler's cache for a day and the background refresh -// cannot invalidate a PNG that has already been rendered from it — a "not yet -// released" prior that has since shipped would show the wrong card until -// tomorrow. 30 minutes is what /internal used as its flat TTL before it shared -// this slot, so bounding here is never worse than the code it replaced. -const SWR_MAX_STALE = 30 * 60; +// Upper bound on the age of a prior we will hand to a consumer that PINS what we +// give it. web-og long-caches any non-null result for 24h (renderImage: +// `result ? longCache : shortCache`), so an answer served to it is stuck in the +// crawler's cache for a day and no later refresh can invalidate a PNG already +// rendered from it — a "not yet released" prior that has since shipped would +// show the wrong card until tomorrow. 30 minutes is what /internal used as its +// flat TTL before it shared this 24h slot, so bounding is never worse than the +// code it replaced. +// +// It bounds EVERY exit that hands back a prior, not just the SWR one. Rounds 4/5 +// bounded only the stale-while-revalidate return, which left both stale-if-error +// exits (back-off below, and the transient catch) free to serve an answer of any +// age — and sharing the 24h slot is exactly what made a 23h-old prior possible on +// the /internal path (`consumerPinsResult`, set by the OG route). Public HTML +// routes leave the flag unset and keep stale-if-error UNBOUNDED: a human sees an +// explicit "stale as of" caveat and can reload, and their answer is not pinned +// anywhere, so serving through a long outage is the right degrade for them. +const MAX_STALE_PINNED = 30 * 60; // Error kinds a later retry might succeed on → eligible for stale-serve (when we // have a prior) or a short negative cache (when we don't). Everything else is a @@ -127,8 +136,15 @@ export async function resolveLookup(args: { * background one is not. A duplicated refresh is far cheaper than poisoning * the key for the lifetime of the isolate. */ coalesce?: boolean; + /** Opt-in for callers whose consumer CACHES whatever we hand back, for longer + * than we can correct (the OG crawler pins a rendered PNG for 24h). When set, + * no exit returns a prior older than `MAX_STALE_PINNED`: we would rather pay a + * fresh lookup, or hand back a transient the caller renders as a short-cached + * placeholder, than pin a day-old answer that has since changed. */ + consumerPinsResult?: boolean; }): Promise { - const { cache, key, load, revalidate, bypassBackOffWhenCold, coalesce } = args; + const { cache, key, load, revalidate, bypassBackOffWhenCold, coalesce, consumerPinsResult } = + args; const now = args.now ?? Date.now; const prior = await cache.getEntry(key); @@ -144,8 +160,15 @@ export async function resolveLookup(args: { cached: true, }); + /** True when `prior` is too old to hand to a consumer that pins it. Always + * false for callers that did not opt in, so stale-if-error is unchanged for + * the public HTML routes. */ + const tooStaleToPin = (): boolean => + Boolean(consumerPinsResult) && + (prior as CacheEntry).ageSeconds >= MAX_STALE_PINNED; + // Stale-while-revalidate: serve what we have, refresh behind it — but only up to - // SWR_MAX_STALE, past which we block rather than hand back an answer the crawler + // MAX_STALE_PINNED, past which we block rather than hand back an answer a crawler // would pin for a day. The refresh is a plain recursive call WITHOUT `revalidate`, // so it takes the blocking path and cannot recurse again, and with // `coalesce: false` so a task the runtime may kill never owns the foreground @@ -155,7 +178,7 @@ export async function resolveLookup(args: { // otherwise run four full lookups against the same repo on the shared token. // Errors are absorbed here — a background failure must not surface as an // unhandled rejection on a response that already succeeded. - if (prior && revalidate && prior.ageSeconds < SWR_MAX_STALE) { + if (prior && revalidate && prior.ageSeconds < MAX_STALE_PINNED) { revalidate( backgroundFlight(key, () => resolveLookup({ cache, key, load, now, coalesce: false })).catch( () => undefined, @@ -171,7 +194,7 @@ export async function resolveLookup(args: { const backedOff = Boolean(neg?.value?.transient) && (neg?.ageSeconds ?? Number.POSITIVE_INFINITY) < NEG_TTL; if (backedOff) { - if (prior) return staleHit(); + if (prior && !tooStaleToPin()) return staleHit(); if (!bypassBackOffWhenCold) { return { status: 'transient', @@ -213,7 +236,7 @@ export async function resolveLookup(args: { { transient: true, kind: err.kind, status: upstreamStatus, anubis }, NEG_TTL, ); - if (prior) return staleHit(); + if (prior && !tooStaleToPin()) return staleHit(); return { status: 'transient', kind: err.kind, upstreamStatus, anubis }; } return { status: 'error', error: err }; diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 1f268d5..16f5206 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -50,8 +50,10 @@ function isServiceBinding(c: Context): boolean { * `wrangler dev`, which loads [vars] — would key on the production origin while * their public routes key on the origin they actually serve. wrangler.toml sets * PUBLIC_BASE_URL for preview; for `wrangler dev`, put - * `PUBLIC_BASE_URL=http://localhost:8787` in packages/web/.dev.vars. Only the - * unit tests reach the request-origin fallback. */ + * `PUBLIC_BASE_URL=http://localhost:8787` in packages/web/.dev.vars (README, + * "Daily flow", says the same where a dev will actually look). Only the unit + * tests reach the request-origin fallback — never the Service Binding, whose + * request origin is the non-routable `https://web` that #143 was about. */ function cacheOrigin(env: Env, req: Request): string { return originOf(env.PUBLIC_BASE_URL) ?? originOf(env.PROD_HOST) ?? new URL(req.url).origin; } @@ -195,6 +197,13 @@ async function resolveResult(c: Context, input: LookupInput): Promise // would instead leave the key with no back-off at all whenever the crawler // touches it first, and every human reload would pound the down host. bypassBackOffWhenCold: true, + // web-og renders whatever we return into a PNG it long-caches for 24h, and + // nothing here can invalidate that PNG afterwards. So no exit may hand this + // caller a prior older than the stale bound — before #143 this route had a + // flat 30-minute TTL and could not, and it now shares the public routes' + // 24h slot, where a 23h-old answer is representable. Rather than pin one, we + // pay a fresh lookup, or 503 into a short-cached placeholder. + consumerPinsResult: true, }); if (resolved.status === 'ok') { return new Response(JSON.stringify(resolved.result), { diff --git a/packages/web/src/single-flight.ts b/packages/web/src/single-flight.ts index 2105e77..e4747b1 100644 --- a/packages/web/src/single-flight.ts +++ b/packages/web/src/single-flight.ts @@ -38,7 +38,7 @@ export async function singleFlight(key: string, loader: Loader): Promise>(); /** Collapse concurrent BACKGROUND refreshes for `key` onto one run, in a map diff --git a/packages/web/test/resolve.test.ts b/packages/web/test/resolve.test.ts index e2e1e9e..aad3ceb 100644 --- a/packages/web/test/resolve.test.ts +++ b/packages/web/test/resolve.test.ts @@ -383,3 +383,98 @@ describe('resolveLookup — stale-while-revalidate is bounded', () => { expect(good).toHaveBeenCalledTimes(1); }); }); + +// Round-6 review of #144. Rounds 4/5 bounded only the stale-while-revalidate +// return, so both stale-if-error exits — the back-off short-circuit and the +// transient catch — could still hand the OG crawler a prior of any age. Sharing +// the public routes' 24h slot is what made that reachable: before #143 this +// route had a flat 30-minute TTL, so a 23h-old prior could not exist on it. +// A pinned consumer renders whatever it gets into a PNG cached for a day. +describe('resolveLookup — a pinned consumer is never handed a prior past the bound', () => { + const aged = () => mkResult({ released: false }); + + it('back-off exit: attempts a fresh lookup instead of serving a 23h-old prior', async () => { + const f = makeFakeCache(); + f.seed(KEY, aged(), 23 * 60 * 60); // inside HARD_TTL_PENDING (24h), way past the bound + f.seed(negKey, { transient: true, kind: 'github_server_error' }, 10); // a page view just failed + const fresh = mkResult({ released: true }); + const load = vi.fn().mockResolvedValue(fresh); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + bypassBackOffWhenCold: true, + consumerPinsResult: true, + }); + + expect(load).toHaveBeenCalledTimes(1); + expect(r.status).toBe('ok'); + if (r.status === 'ok') { + expect(r.stale).toBe(false); + expect(r.result.firstRelease?.tag).toBe('4.18.0'); + } + }); + + it('transient-catch exit: returns transient rather than the 23h-old prior', async () => { + const f = makeFakeCache(); + f.seed(KEY, aged(), 23 * 60 * 60); + const load = vi.fn(async () => { + throw new ProviderServerError('github.com', 503, 'Service Unavailable'); + }); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + bypassBackOffWhenCold: true, + consumerPinsResult: true, + }); + + expect(load).toHaveBeenCalledTimes(1); + expect(r.status).toBe('transient'); + }); + + it('a prior INSIDE the bound is still stale-served to a pinned consumer', async () => { + const f = makeFakeCache(); + const pending = mkResult({ released: false, partial: true }); + f.seed(KEY, pending, 120); // stale (past the 60s partial window), inside the 30-min bound + f.seed(negKey, { transient: true, kind: 'github_server_error' }, 10); + const load = vi.fn(); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + bypassBackOffWhenCold: true, + consumerPinsResult: true, + }); + + expect(load).not.toHaveBeenCalled(); + expect(r.status).toBe('ok'); + if (r.status === 'ok') { + expect(r.stale).toBe(true); + expect(r.result).toEqual(pending); + } + }); + + it('public routes keep UNBOUNDED stale-if-error — the bound is opt-in only', async () => { + const f = makeFakeCache(); + const old = aged(); + f.seed(KEY, old, 23 * 60 * 60); + const load = vi.fn(async () => { + throw new ProviderServerError('github.com', 503, 'Service Unavailable'); + }); + + // Same 23h prior, same failure — but no consumerPinsResult: a human page is + // not pinned anywhere and shows an explicit "stale as of" caveat, so serving + // through a long outage stays the right degrade. + const r = await resolveLookup({ cache: f.cache, key: KEY, load }); + + expect(r.status).toBe('ok'); + if (r.status === 'ok') { + expect(r.stale).toBe(true); + expect(r.result).toEqual(old); + } + }); +}); From 780df8d6cadc79432ae7e78d089ac2edea9d3833 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 20 Aug 2026 16:31:56 +0100 Subject: [PATCH 09/22] fix(web): never hand a pinning consumer a cached partial MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-7 review of #144. The round-6 stale bound (`tooStaleToPin`) was consulted on the three STALE exits only. The fresh-hit return above them never asked, and `isFresh()` calls a `partial` fresh for its whole 60s life — so the exit that matters most was unguarded. This PR is what made it reachable. `badge.ts` loads the identical five-part key with an 8s soft deadline, so on a large repo it writes a `partial` (firstRelease `null`) that `/internal` then served as a plain 200. web-og renders `firstRelease?.tag ?? 'not yet released'` and long-caches any non-null result, so a RELEASED commit got a "not yet released" card pinned for a day: the CLAUDE.md guardrail ("partial state is not 'not yet released'") and precisely what `consumerPinsResult` was added to prevent. On main it could not happen — `/internal` had its own 3-part key, so a badge- or page-written partial was never visible here. `tooStaleToPin()` becomes `unpinnable(entry)`, rejecting a partial as well as an over-age prior, and is applied at every exit that can return a cached entry: the fresh hit, the SWR return, both stale-if-error exits, and singleFlight's double-check read (which would otherwise re-serve the entry the exits just refused). Still gated on `consumerPinsResult`, so the public HTML routes are untouched — the result card renders `partial` as an explicit best-effort caveat, which is the right answer there. Also renames `bypassBackOffWhenCold` to `bypassBackOffWhenUnservable`. The old name and its docstring both claimed "cold", but once `consumerPinsResult` is set a warm-but-unpinnable prior falls through to that bypass too. The behaviour is intended (the alternative is a permanent placeholder); the name was narrower than the code. --- packages/web/src/resolve.ts | 61 ++++++---- packages/web/src/routes/internal.ts | 14 ++- .../web/test/internal-cache-origin.test.ts | 20 ++-- packages/web/test/resolve.test.ts | 105 +++++++++++++++++- 4 files changed, 160 insertions(+), 40 deletions(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index 0fa2c1f..cc3f503 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -14,7 +14,7 @@ // "negative" marker; while it's warm we skip the upstream call entirely // (serving stale if we have it, otherwise a soft "checking…" transient). // A caller whose consumer asks only ONCE can opt out of the cold half of -// that (`bypassBackOffWhenCold`) — backing off is cheap for a page a human +// that (`bypassBackOffWhenUnservable`) — backing off is cheap for a page a human // can reload and permanent for a crawler that unfurls once. // // "not yet released" is a thrown NotYetReleasedError (not a cacheable result), @@ -119,11 +119,16 @@ export async function resolveLookup(args: { revalidate?: (task: Promise) => void; /** Opt-in for callers whose consumer only ever asks ONCE, so a soft failure * becomes permanent for them (the OG crawler). When set, the shared negative - * back-off marker is honoured only if there is a prior to stale-serve — - * with nothing to serve, an attempt beats handing back a placeholder that - * gets cached forever. The marker is still WRITTEN on failure, and callers - * that can retry (the public HTML routes) omit this and keep backing off. */ - bypassBackOffWhenCold?: boolean; + * back-off marker is honoured only if there is a prior we can actually SERVE + * — with nothing servable, an attempt beats handing back a placeholder that + * gets cached forever. "Nothing servable" is broader than an empty slot: with + * `consumerPinsResult`, a prior past `MAX_STALE_PINNED` or a truncated + * `partial` is unservable too, so a warm-but-unpinnable key reaches this path + * as well. That is deliberate (the alternative is a permanent placeholder), + * but it is NOT throttled — see the fall-through comment below for the cost. + * The marker is still WRITTEN on failure, and callers that can retry (the + * public HTML routes) omit this and keep backing off. */ + bypassBackOffWhenUnservable?: boolean; /** Internal. Set false for the background refresh on the SWR path: that task * runs under `executionCtx.waitUntil`, whose IoContext workerd can tear down * before the subrequest settles. singleFlight only clears its module-level @@ -143,12 +148,33 @@ export async function resolveLookup(args: { * placeholder, than pin a day-old answer that has since changed. */ consumerPinsResult?: boolean; }): Promise { - const { cache, key, load, revalidate, bypassBackOffWhenCold, coalesce, consumerPinsResult } = - args; + const { + cache, + key, + load, + revalidate, + bypassBackOffWhenUnservable, + coalesce, + consumerPinsResult, + } = args; const now = args.now ?? Date.now; + /** True when a cached entry must NOT be handed to a consumer that pins the + * answer — either because it is too old to still be true, or because it is a + * `partial`: a truncated traversal whose `firstRelease: null` means "we + * stopped looking", not "not released". The result card renders that caveat; + * web-og cannot (`firstRelease?.tag ?? 'not yet released'`, long-cached for + * any non-null result), so a partial pins a wrong answer for a day. `badge.ts` + * loads THIS key with an 8s soft deadline, so on a large repo it writes the + * partial that would otherwise be served here — reachable only since this + * route joined the public five-part key. Always false for callers that did + * not opt in, so the public HTML routes are unchanged. */ + const unpinnable = (entry: CacheEntry): boolean => + Boolean(consumerPinsResult) && + (entry.ageSeconds >= MAX_STALE_PINNED || Boolean(entry.value.partial)); + const prior = await cache.getEntry(key); - if (prior && isFresh(prior)) { + if (prior && isFresh(prior) && !unpinnable(prior)) { return { status: 'ok', result: prior.value, stale: false, staleAsOf: null, cached: true }; } @@ -160,13 +186,6 @@ export async function resolveLookup(args: { cached: true, }); - /** True when `prior` is too old to hand to a consumer that pins it. Always - * false for callers that did not opt in, so stale-if-error is unchanged for - * the public HTML routes. */ - const tooStaleToPin = (): boolean => - Boolean(consumerPinsResult) && - (prior as CacheEntry).ageSeconds >= MAX_STALE_PINNED; - // Stale-while-revalidate: serve what we have, refresh behind it — but only up to // MAX_STALE_PINNED, past which we block rather than hand back an answer a crawler // would pin for a day. The refresh is a plain recursive call WITHOUT `revalidate`, @@ -178,7 +197,7 @@ export async function resolveLookup(args: { // otherwise run four full lookups against the same repo on the shared token. // Errors are absorbed here — a background failure must not surface as an // unhandled rejection on a response that already succeeded. - if (prior && revalidate && prior.ageSeconds < MAX_STALE_PINNED) { + if (prior && revalidate && prior.ageSeconds < MAX_STALE_PINNED && !unpinnable(prior)) { revalidate( backgroundFlight(key, () => resolveLookup({ cache, key, load, now, coalesce: false })).catch( () => undefined, @@ -194,8 +213,8 @@ export async function resolveLookup(args: { const backedOff = Boolean(neg?.value?.transient) && (neg?.ageSeconds ?? Number.POSITIVE_INFINITY) < NEG_TTL; if (backedOff) { - if (prior && !tooStaleToPin()) return staleHit(); - if (!bypassBackOffWhenCold) { + if (prior && !unpinnable(prior)) return staleHit(); + if (!bypassBackOffWhenUnservable) { return { status: 'transient', kind: neg?.value.kind ?? 'provider_server_error', @@ -218,7 +237,7 @@ export async function resolveLookup(args: { try { const run = async () => { const re = await cache.getEntry(key); - if (re && isFresh(re)) return re.value; + if (re && isFresh(re) && !unpinnable(re)) return re.value; const r = await load(); await cache.put(key, r, hardTtlFor(r)); return r; @@ -236,7 +255,7 @@ export async function resolveLookup(args: { { transient: true, kind: err.kind, status: upstreamStatus, anubis }, NEG_TTL, ); - if (prior && !tooStaleToPin()) return staleHit(); + if (prior && !unpinnable(prior)) return staleHit(); return { status: 'transient', kind: err.kind, upstreamStatus, anubis }; } return { status: 'error', error: err }; diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 16f5206..5bef939 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -196,13 +196,17 @@ async function resolveResult(c: Context, input: LookupInput): Promise // reload; the crawler asks once and keeps what it got. Suppressing the write // would instead leave the key with no back-off at all whenever the crawler // touches it first, and every human reload would pound the down host. - bypassBackOffWhenCold: true, + bypassBackOffWhenUnservable: true, // web-og renders whatever we return into a PNG it long-caches for 24h, and // nothing here can invalidate that PNG afterwards. So no exit may hand this - // caller a prior older than the stale bound — before #143 this route had a - // flat 30-minute TTL and could not, and it now shares the public routes' - // 24h slot, where a 23h-old answer is representable. Rather than pin one, we - // pay a fresh lookup, or 503 into a short-cached placeholder. + // caller a CACHED entry that is either older than the stale bound — before + // #143 this route had a flat 30-minute TTL and could not, and it now shares + // the public routes' 24h slot, where a 23h-old answer is representable — or + // a truncated `partial`, whose `firstRelease: null` web-og would render as a + // definite "not yet released" (badge.ts writes those onto this same key with + // an 8s soft deadline). Rather than pin either, we pay a fresh lookup, or + // 503 into a short-cached placeholder. web-og long-caching a partial that + // THIS route's own lookup produced is the remaining half, tracked in #151. consumerPinsResult: true, }); if (resolved.status === 'ok') { diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index acfc5c0..bb381bc 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -455,22 +455,24 @@ describe('/internal/* never blocks the render on a revalidation', () => { expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.12.0'); }); - it('serves a stale PARTIAL without waiting, then refreshes the slot behind it', async () => { + // Round 7 (#144). This used to assert the opposite — that a cached `partial` + // was stale-served to web-og while a refresh ran behind it. That is the bug: + // web-og renders `firstRelease?.tag ?? 'not yet released'` and long-caches any + // non-null result, so a truncated traversal of a RELEASED commit got pinned as + // "not yet released" for 24h. `badge.ts` writes those onto this very key (8s + // soft deadline), which only this PR's key alignment made visible here. + it('does NOT hand back a cached PARTIAL — it blocks and recomputes', async () => { const sha = 'c'.repeat(40); const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); seedAged(PUBLIC_ORIGIN, k, partialFixture(), 5 * 60); - const slow = deferred(); - findReleaseMock.mockReturnValue(slow.promise); + findReleaseMock.mockResolvedValue(fixture('v4.12.0')); const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); expect(res.status).toBe(200); - expect(await tagOf(res)).toBeUndefined(); // the partial, served as-is - expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBeUndefined(); // not refreshed YET - - slow.resolve(fixture('v4.12.0')); - await settle(); - expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.12.0'); // the background refresh landed + expect(await tagOf(res)).toBe('v4.12.0'); // the real answer, not the partial + expect(findReleaseMock).toHaveBeenCalledTimes(1); + expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.12.0'); // and the slot is corrected }); it('still blocks (and write-backs) when the slot is genuinely cold', async () => { diff --git a/packages/web/test/resolve.test.ts b/packages/web/test/resolve.test.ts index aad3ceb..e9d6ead 100644 --- a/packages/web/test/resolve.test.ts +++ b/packages/web/test/resolve.test.ts @@ -404,7 +404,7 @@ describe('resolveLookup — a pinned consumer is never handed a prior past the b cache: f.cache, key: KEY, load, - bypassBackOffWhenCold: true, + bypassBackOffWhenUnservable: true, consumerPinsResult: true, }); @@ -427,7 +427,7 @@ describe('resolveLookup — a pinned consumer is never handed a prior past the b cache: f.cache, key: KEY, load, - bypassBackOffWhenCold: true, + bypassBackOffWhenUnservable: true, consumerPinsResult: true, }); @@ -437,8 +437,10 @@ describe('resolveLookup — a pinned consumer is never handed a prior past the b it('a prior INSIDE the bound is still stale-served to a pinned consumer', async () => { const f = makeFakeCache(); - const pending = mkResult({ released: false, partial: true }); - f.seed(KEY, pending, 120); // stale (past the 60s partial window), inside the 30-min bound + // Deliberately NOT a `partial`: round 7 made those unpinnable at every exit + // regardless of age, so a partial would no longer isolate the age bound. + const pending = mkResult({ released: false }); + f.seed(KEY, pending, 10 * 60); // stale (past the 5-min freshness window), inside the 30-min bound f.seed(negKey, { transient: true, kind: 'github_server_error' }, 10); const load = vi.fn(); @@ -446,7 +448,7 @@ describe('resolveLookup — a pinned consumer is never handed a prior past the b cache: f.cache, key: KEY, load, - bypassBackOffWhenCold: true, + bypassBackOffWhenUnservable: true, consumerPinsResult: true, }); @@ -478,3 +480,96 @@ describe('resolveLookup — a pinned consumer is never handed a prior past the b } }); }); + +// Round-7 review of #144. The round-6 bound was consulted only on the three +// STALE exits. The fresh-hit return above them never asked — and `isFresh()` +// calls a `partial` fresh for its whole 60s life. Aligning this route onto the +// public five-part key (this PR) is what made that reachable: `badge.ts` loads +// the identical key with an 8s soft deadline, so on a large repo it writes a +// `partial` (firstRelease `null`) that `/internal` then served as a plain 200. +// web-og renders `firstRelease?.tag ?? 'not yet released'` and long-caches any +// non-null result, so a RELEASED commit gets a "not yet released" card pinned +// for a day — the CLAUDE.md guardrail ("Partial state != not yet released") +// and the exact outcome `consumerPinsResult` exists to prevent. +describe('resolveLookup — a pinned consumer is never handed a cached PARTIAL', () => { + it('fresh exit: recomputes rather than serve a 10s-old partial written by badge.ts', async () => { + const f = makeFakeCache(); + const truncated = mkResult({ released: false, partial: true }); + f.seed(KEY, truncated, 10); // well inside the 60s partial freshness window + const fresh = mkResult({ released: true }); + const load = vi.fn().mockResolvedValue(fresh); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + }); + + expect(load).toHaveBeenCalledTimes(1); + expect(r.status).toBe('ok'); + if (r.status === 'ok') expect(r.result.firstRelease?.tag).toBe('4.18.0'); + }); + + it('SWR exit: a fresh partial is not handed back while a refresh runs behind it', async () => { + const f = makeFakeCache(); + const truncated = mkResult({ released: false, partial: true }); + f.seed(KEY, truncated, 10); + const fresh = mkResult({ released: true }); + const load = vi.fn().mockResolvedValue(fresh); + const tasks: Promise[] = []; + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + revalidate: (t) => { + tasks.push(t); + }, + consumerPinsResult: true, + }); + + expect(tasks).toHaveLength(0); // blocked instead of stale-serving the partial + expect(r.status).toBe('ok'); + if (r.status === 'ok') { + expect(r.stale).toBe(false); + expect(r.result.firstRelease?.tag).toBe('4.18.0'); + } + }); + + it('stale exit: a partial inside the 30-min bound is still not pinnable', async () => { + const f = makeFakeCache(); + const truncated = mkResult({ released: false, partial: true }); + f.seed(KEY, truncated, 120); // past the 60s partial window, inside the 30-min bound + f.seed(negKey, { transient: true, kind: 'github_server_error' }, 10); + const fresh = mkResult({ released: true }); + const load = vi.fn().mockResolvedValue(fresh); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + bypassBackOffWhenUnservable: true, + consumerPinsResult: true, + }); + + expect(load).toHaveBeenCalledTimes(1); + expect(r.status).toBe('ok'); + if (r.status === 'ok') expect(r.result.firstRelease?.tag).toBe('4.18.0'); + }); + + it('public routes still get the fresh partial — the guard is opt-in only', async () => { + const f = makeFakeCache(); + const truncated = mkResult({ released: false, partial: true }); + f.seed(KEY, truncated, 10); + const load = vi.fn(); + + // No consumerPinsResult: the result card renders `partial` as an explicit + // best-effort caveat (CLAUDE.md), so a fresh partial is the right answer. + const r = await resolveLookup({ cache: f.cache, key: KEY, load }); + + expect(load).not.toHaveBeenCalled(); + expect(r.status).toBe('ok'); + if (r.status === 'ok') expect(r.result).toEqual(truncated); + }); +}); From fd09b1be8706c3eceeb5e93e08d16291269f48ea Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 20 Aug 2026 18:15:50 +0100 Subject: [PATCH 10/22] fix(web): exempt terminal answers from the pin bound; 503 a computed partial MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from this PR's own review, both on the `consumerPinsResult` mechanism it introduces: 1. `unpinnable()` applied the 30-minute `MAX_STALE_PINNED` age bound to TERMINAL results too, so `/internal/*` discarded exactly the warm entries this PR exists to reuse. Which release first contains a commit cannot change — `isFresh()` treats a `firstRelease` answer as fresh forever and `hardTtlFor()` gives it 30 days — so every unfurl more than 30 minutes after the last write paid a full findRelease on the crawler's critical path, and during an upstream outage returned a `transient` (neutral placeholder) over a perfectly good cached answer. The bound now skips terminal answers, matching what `isFresh()`/`hardTtlFor()` already do. 2. Refusing to SERVE a cached partial was only half the guardrail: on a repo that reliably blows the soft deadline the forced recompute returns a partial too, and returning it as a 200 pinned the same wrong "not yet released" card for 24h that the refusal exists to prevent. A computed `partial` with no `firstRelease` now falls through to the 503, which web-og renders as the neutral placeholder at max-age=60 and self-heals. The bound is `partial && !firstRelease` — a truncated traversal that DID find a containing release carries a real tag web-og renders correctly. Both guards were mutation-proven against the defect in their own title. --- packages/web/src/resolve.ts | 12 ++- packages/web/src/routes/internal.ts | 20 ++++- .../web/test/internal-cache-origin.test.ts | 39 +++++++++- packages/web/test/resolve.test.ts | 74 +++++++++++++++++++ 4 files changed, 142 insertions(+), 3 deletions(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index cc3f503..e77adcb 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -168,9 +168,19 @@ export async function resolveLookup(args: { * loads THIS key with an 8s soft deadline, so on a large repo it writes the * partial that would otherwise be served here — reachable only since this * route joined the public five-part key. Always false for callers that did - * not opt in, so the public HTML routes are unchanged. */ + * not opt in, so the public HTML routes are unchanged. + * + * The age bound does NOT apply to a terminal `firstRelease` answer: which + * release first contains a commit cannot change, which is why `isFresh()` + * treats it as fresh forever and `hardTtlFor()` gives it `HARD_TTL_RELEASED` + * (30 days). `MAX_STALE_PINNED` exists for the opposite case — a "not yet + * released" prior that has since shipped. Applying it to a terminal answer + * would discard exactly the warm entries this route joined the public key to + * reuse, paying a full findRelease on the crawler's critical path for every + * unfurl more than 30 minutes after the last write. */ const unpinnable = (entry: CacheEntry): boolean => Boolean(consumerPinsResult) && + !entry.value.firstRelease && (entry.ageSeconds >= MAX_STALE_PINNED || Boolean(entry.value.partial)); const prior = await cache.getEntry(key); diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 5bef939..99f89fa 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -209,8 +209,26 @@ async function resolveResult(c: Context, input: LookupInput): Promise // THIS route's own lookup produced is the remaining half, tracked in #151. consumerPinsResult: true, }); + // Refusing to SERVE a cached partial (consumerPinsResult, above) is only half + // the guardrail. On a repo that reliably blows findRelease's soft deadline the + // recompute it forces returns a partial too, and a 200 here pins exactly the + // wrong card the refusal exists to prevent: web-og renders + // `firstRelease?.tag ?? 'not yet released'` and long-caches any non-null + // result, so a truncated traversal of a RELEASED commit becomes a definite + // "not yet released" for 24h — the CLAUDE.md guardrail ("Partial state != + // 'not yet released'"). Falling through to the 503 renders the neutral + // placeholder at max-age=60 instead, which self-heals on the next unfurl. + // The bound is `partial && !firstRelease`: a truncated traversal that DID find + // a containing release carries a real tag web-og renders correctly. if (resolved.status === 'ok') { - return new Response(JSON.stringify(resolved.result), { + const pinsWrongAnswer = Boolean(resolved.result.partial) && !resolved.result.firstRelease; + if (!pinsWrongAnswer) { + return new Response(JSON.stringify(resolved.result), { + headers: { 'content-type': 'application/json' }, + }); + } + return new Response(JSON.stringify({ error: 'partial' }), { + status: 503, headers: { 'content-type': 'application/json' }, }); } diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index bb381bc..372ba68 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -337,7 +337,11 @@ describe('/internal/* follows the cache policy that governs the shared slot', () findReleaseMock.mockResolvedValue(partialFixture()); const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); - expect(res.status).toBe(200); + // The RESPONSE is a 503 (a computed partial is never pinned — see the + // guardrail suite below); the point here is that the write-back still + // happens, and lands on the shared 60-second partial TTL rather than + // /internal's old flat 30 minutes, so a public page view can reuse it. + expect(res.status).toBe(503); const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); expect(cacheControlOf(PUBLIC_ORIGIN, k)).toBe('public, max-age=60'); @@ -484,6 +488,39 @@ describe('/internal/* never blocks the render on a revalidation', () => { expect(res.status).toBe(200); expect(await tagOf(res)).toBe('v4.15.0'); }); + + // Round 7 (#144), the other half of the same guardrail. Refusing to SERVE a + // cached partial is only half the job: on a repo that reliably blows the soft + // deadline the forced recompute returns a partial too, and returning that as a + // 200 pins the same wrong "not yet released" card for 24h that the refusal + // exists to prevent (web-og: `firstRelease?.tag ?? 'not yet released'`, + // long-cached for any non-null result). A 503 instead renders the neutral + // placeholder at max-age=60, which self-heals on the next unfurl. + it('503s rather than pin a COMPUTED partial as "not yet released"', async () => { + const sha = '1'.repeat(40); + findReleaseMock.mockResolvedValue(partialFixture()); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(res.status).toBe(503); + expect(findReleaseMock).toHaveBeenCalledTimes(1); + }); + + // The bound is on `partial && !firstRelease`, not on `partial` alone: a + // truncated traversal that DID find a containing release carries a real tag, + // which web-og renders correctly. 503ing that would throw away a right answer. + it('still serves a partial that carries a real firstRelease', async () => { + const sha = '2'.repeat(40); + findReleaseMock.mockResolvedValue({ + ...(fixture('v4.19.0') as Record), + partial: { reason: 'soft_deadline', candidatesTried: 3 }, + }); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.19.0'); + }); }); // PROD_HOST is shared with isProdRequest() (analytics.ts), which documents itself diff --git a/packages/web/test/resolve.test.ts b/packages/web/test/resolve.test.ts index e9d6ead..2756843 100644 --- a/packages/web/test/resolve.test.ts +++ b/packages/web/test/resolve.test.ts @@ -573,3 +573,77 @@ describe('resolveLookup — a pinned consumer is never handed a cached PARTIAL', if (r.status === 'ok') expect(r.result).toEqual(truncated); }); }); + +// A RELEASED answer is terminal: which release first contains a commit cannot +// change, which is why `isFresh()` treats it as fresh forever and `hardTtlFor()` +// gives it a 30-day TTL. `MAX_STALE_PINNED` exists for the opposite case — a +// "not yet released" prior that has since shipped — so applying its 30-minute +// age bound to a terminal answer discards exactly the warm entries `/internal/*` +// joined the public key to reuse (#143), and pays a full findRelease on the +// crawler's critical path for every unfurl more than 30 minutes after the last +// write. +describe('resolveLookup — a terminal RELEASED prior stays pinnable at any age', () => { + it('fresh exit: serves a 2h-old released prior instead of recomputing', async () => { + const f = makeFakeCache(); + const released = mkResult({ released: true }); + f.seed(KEY, released, 2 * 60 * 60); // the normal state of a 30-day slot + const load = vi.fn(); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + }); + + expect(load).not.toHaveBeenCalled(); + expect(r.status).toBe('ok'); + if (r.status === 'ok') { + expect(r.cached).toBe(true); + expect(r.result.firstRelease?.tag).toBe('4.18.0'); + } + }); + + it('back-off exit: an upstream outage still serves the 2h-old released prior', async () => { + const f = makeFakeCache(); + const released = mkResult({ released: true }); + f.seed(KEY, released, 2 * 60 * 60); + f.seed(negKey, { transient: true, kind: 'github_server_error' }, 10); + const load = vi.fn(); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + bypassBackOffWhenUnservable: true, + consumerPinsResult: true, + }); + + // Without the terminal exemption this returns `transient`, which web-og + // renders as the neutral placeholder at max-age=60 — and each unfurl during + // the outage runs its own lookup out to the soft deadline against the down + // host. + expect(load).not.toHaveBeenCalled(); + expect(r.status).toBe('ok'); + if (r.status === 'ok') expect(r.result.firstRelease?.tag).toBe('4.18.0'); + }); + + it('a 2h-old NOT-YET-released prior is still unpinnable — the bound it exists for', async () => { + const f = makeFakeCache(); + const notYet = mkResult({ released: false }); + f.seed(KEY, notYet, 2 * 60 * 60); + const fresh = mkResult({ released: true }); + const load = vi.fn().mockResolvedValue(fresh); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + }); + + expect(load).toHaveBeenCalledTimes(1); + expect(r.status).toBe('ok'); + if (r.status === 'ok') expect(r.result.firstRelease?.tag).toBe('4.18.0'); + }); +}); From 58963336b89e99fc761856c836cb7f1f3bc24cf9 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Mon, 24 Aug 2026 18:58:04 +0100 Subject: [PATCH 11/22] fix(web): throttle the partial 503, refuse gallop-only partials, bound backgroundFlight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-8 review of #144. Four findings, all in code this PR introduces. - The computed-partial 503 was never recorded, so no read path accepted the entry resolveLookup had just written and every unfurl of a deadline-blowing repo ran another full 24s traversal on the shared token. A partial inside its own HARD_TTL_PARTIAL is now handed back instead of recomputed; the route still 503s it, so the refusal costs a cache read rather than a findRelease. - The pin bound exempted any entry carrying a firstRelease, but find-release.ts also returns a partial carrying a GALLOP hit the bisect never confirmed is the earliest containing release. hardTtlFor()/isFresh() both test firstRelease first, so that shape was stored 30 days and reported fresh forever. No partial is servable to a pinning consumer now, either shape. - originOf() accepted a non-routable single-label host, so a var set to the Service-Binding target would look configured while caching nothing. Rejected (localhost excepted); a wrangler.toml suite asserts every env sets PROD_HOST or PUBLIC_BASE_URL, which is the only place the unset case can be caught. - backgroundFlight entries never expired, so a refresh abandoned by a torn-down IoContext left a dead promise every later refresh joined — revalidation dead for the isolate's lifetime. Entries now carry startedAt and expire at 30s. Each guard mutation-proved against the defect in its own title; details in the PR body. --- packages/web/src/resolve.ts | 55 ++++-- packages/web/src/routes/internal.ts | 47 +++-- packages/web/src/single-flight.ts | 44 +++-- .../web/test/internal-cache-origin.test.ts | 186 +++++++++++++++++- packages/web/test/resolve.test.ts | 73 ++++++- packages/web/test/single-flight.test.ts | 83 ++++++++ 6 files changed, 432 insertions(+), 56 deletions(-) create mode 100644 packages/web/test/single-flight.test.ts diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index e77adcb..09c4808 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -160,28 +160,41 @@ export async function resolveLookup(args: { const now = args.now ?? Date.now; /** True when a cached entry must NOT be handed to a consumer that pins the - * answer — either because it is too old to still be true, or because it is a - * `partial`: a truncated traversal whose `firstRelease: null` means "we - * stopped looking", not "not released". The result card renders that caveat; - * web-og cannot (`firstRelease?.tag ?? 'not yet released'`, long-cached for - * any non-null result), so a partial pins a wrong answer for a day. `badge.ts` - * loads THIS key with an 8s soft deadline, so on a large repo it writes the - * partial that would otherwise be served here — reachable only since this - * route joined the public five-part key. Always false for callers that did - * not opt in, so the public HTML routes are unchanged. + * answer. Always false for callers that did not opt in, so the public HTML + * routes are unchanged. Three shapes, three rules: * - * The age bound does NOT apply to a terminal `firstRelease` answer: which - * release first contains a commit cannot change, which is why `isFresh()` - * treats it as fresh forever and `hardTtlFor()` gives it `HARD_TTL_RELEASED` - * (30 days). `MAX_STALE_PINNED` exists for the opposite case — a "not yet - * released" prior that has since shipped. Applying it to a terminal answer - * would discard exactly the warm entries this route joined the public key to - * reuse, paying a full findRelease on the crawler's critical path for every - * unfurl more than 30 minutes after the last write. */ - const unpinnable = (entry: CacheEntry): boolean => - Boolean(consumerPinsResult) && - !entry.value.firstRelease && - (entry.ageSeconds >= MAX_STALE_PINNED || Boolean(entry.value.partial)); + * TERMINAL (`firstRelease`, no `partial`) — always servable. Which release + * first contains a commit cannot change, which is why `isFresh()` treats it as + * fresh forever and `hardTtlFor()` keeps it 30 days. `MAX_STALE_PINNED` exists + * for the opposite case, a "not yet released" prior that has since shipped; + * applying it here would discard exactly the warm entries this route joined the + * public key to reuse, paying a full findRelease per unfurl. + * + * PARTIAL (either shape) — never servable to a pinning consumer, but only + * worth recomputing once its own 60-second TTL is up. A partial is a truncated + * traversal: with `firstRelease: null` it means "we stopped looking", which + * web-og renders as a definite "not yet released"; WITH a `firstRelease` it + * carries the gallop hit, and the bisect that would confirm no EARLIER release + * contains the commit is what the deadline cut short (find-release.ts:288-292). + * The result card renders that caveat, web-og cannot — it long-caches the bare + * tag for 24h. So neither shape may be pinned. Inside `HARD_TTL_PARTIAL` the + * entry is still handed BACK (the caller 503s it into a short-cached neutral + * placeholder): that is what throttles a repo which reliably blows the soft + * deadline, where recomputing per unfurl would run a full traversal on the + * shared token for every crawler, forever. Past 60s we recompute instead. + * + * PENDING (no `firstRelease`, no `partial`) — bounded by `MAX_STALE_PINNED`: + * an answer older than that may have shipped since, and a PNG already rendered + * from it cannot be invalidated. */ + const isRecentPartial = (entry: CacheEntry): boolean => + Boolean(entry.value.partial) && entry.ageSeconds < HARD_TTL_PARTIAL; + + const unpinnable = (entry: CacheEntry): boolean => { + if (!consumerPinsResult) return false; + if (entry.value.firstRelease && !entry.value.partial) return false; + if (isRecentPartial(entry)) return false; + return entry.ageSeconds >= MAX_STALE_PINNED || Boolean(entry.value.partial); + }; const prior = await cache.getEntry(key); if (prior && isFresh(prior) && !unpinnable(prior)) { diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 99f89fa..bcba795 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -81,7 +81,19 @@ function originOf(value: string | undefined): string | null { // non-URL string that satisfies `??` and then throws out of `new Request()` // below, OUTSIDE neverFatal. That is the scheme-less slip again: a 500 where // a computed answer was available, rendered as the neutral placeholder. - return origin === 'null' ? null : origin; + if (origin === 'null') return null; + // A single-label host (`web`, `localhost:8787`'s sibling shapes) is not + // routable, and the Cache API silently declines a key URL on one — #143's + // mechanism. `https://web` parses cleanly and yields a plausible-looking + // origin, so a var accidentally set to the Service-Binding target would + // otherwise sail through here looking configured while caching nothing. + // Falling through to the next candidate is strictly better: a wrong-but- + // routable origin still persists, a non-routable one persists nothing. + // `localhost` is the one single-label host that is real — `wrangler dev` + // serves on it, and README tells developers to put it in PUBLIC_BASE_URL. + const { hostname } = new URL(origin); + const routable = hostname.includes('.') || hostname === 'localhost' || hostname.includes(':'); + return routable ? origin : null; } catch { return null; } @@ -210,19 +222,30 @@ async function resolveResult(c: Context, input: LookupInput): Promise consumerPinsResult: true, }); // Refusing to SERVE a cached partial (consumerPinsResult, above) is only half - // the guardrail. On a repo that reliably blows findRelease's soft deadline the + // the guardrail: on a repo that reliably blows findRelease's soft deadline the // recompute it forces returns a partial too, and a 200 here pins exactly the - // wrong card the refusal exists to prevent: web-og renders - // `firstRelease?.tag ?? 'not yet released'` and long-caches any non-null - // result, so a truncated traversal of a RELEASED commit becomes a definite - // "not yet released" for 24h — the CLAUDE.md guardrail ("Partial state != - // 'not yet released'"). Falling through to the 503 renders the neutral - // placeholder at max-age=60 instead, which self-heals on the next unfurl. - // The bound is `partial && !firstRelease`: a truncated traversal that DID find - // a containing release carries a real tag web-og renders correctly. + // answer the refusal exists to prevent. + // + // NO partial is servable to this caller, whichever shape it has. With + // `firstRelease: null`, web-og renders `firstRelease?.tag ?? 'not yet released'` + // and long-caches it, so a truncated traversal of a RELEASED commit becomes a + // definite "not yet released" for 24h — the CLAUDE.md guardrail ("Partial state + // != 'not yet released'"). WITH a `firstRelease` it is no safer: that tag is the + // gallop hit, and the bisect that would confirm no EARLIER release contains the + // commit is precisely what the deadline cut short ("the gallop-found tag is + // almost always the right answer; bisect just verifies could there be an earlier + // one", find-release.ts:288-292). "Almost always" is a caveat the result card + // renders and an OG card cannot. Answering "which release FIRST contains this + // commit" with a possibly-later release is the one thing this product must not + // do, so both fall through to the 503: web-og caches the neutral placeholder at + // max-age=60, which claims nothing and self-heals on the next unfurl, and the + // permalink it links to still shows the best-effort answer WITH its caveat. + // + // resolveLookup hands back a partial it computed less than HARD_TTL_PARTIAL ago + // rather than recomputing it (see `unpinnable`), so this 503 is throttled to one + // traversal per 60s per key instead of one per unfurl. if (resolved.status === 'ok') { - const pinsWrongAnswer = Boolean(resolved.result.partial) && !resolved.result.firstRelease; - if (!pinsWrongAnswer) { + if (!resolved.result.partial) { return new Response(JSON.stringify(resolved.result), { headers: { 'content-type': 'application/json' }, }); diff --git a/packages/web/src/single-flight.ts b/packages/web/src/single-flight.ts index e4747b1..6576dfe 100644 --- a/packages/web/src/single-flight.ts +++ b/packages/web/src/single-flight.ts @@ -34,26 +34,46 @@ export async function singleFlight(key: string, loader: Loader): Promise>(); +// That separation bounds who can be hurt by an abandoned promise, but not how +// long. The map is module-level, so an entry registered under request A's +// IoContext is handed to request B; when workerd cancels A's context the promise +// never settles, so the loader's `finally` never runs and the entry never clears. +// Every later refresh for that key joins the dead promise, and background +// revalidation for it is dead for the isolate's lifetime — recovery only when the +// prior crosses MAX_STALE_PINNED and the foreground path blocks. So entries carry +// the time they were registered and expire: past MAX_BACKGROUND_AGE_MS the entry +// is treated as absent and the next refresh starts a fresh run, which also caps +// the map at the keys refreshed in the last window. +const background = new Map; startedAt: number }>(); + +/** How long a background entry may be joined before it is presumed abandoned. + * findRelease's own HARD deadline is 28s, so a refresh still running past this + * is not slow — it is a promise whose IoContext went away. Erring long is the + * safe direction: the cost of expiring too early is one duplicated traversal, + * while the cost of never expiring is no revalidation at all for that key. */ +const MAX_BACKGROUND_AGE_MS = 30_000; /** Collapse concurrent BACKGROUND refreshes for `key` onto one run, in a map * foreground callers never join. Registration is synchronous, so two refreshes * fired in the same tick cannot both miss it. */ export function backgroundFlight(key: string, loader: Loader): Promise { - const existing = background.get(key) as Promise | undefined; - if (existing) return existing; - const p = (async () => { + const existing = background.get(key); + if (existing && Date.now() - existing.startedAt < MAX_BACKGROUND_AGE_MS) { + return existing.promise as Promise; + } + const entry: { promise: Promise; startedAt: number } = { + promise: undefined as unknown as Promise, + startedAt: Date.now(), + }; + entry.promise = (async () => { try { return await loader(); } finally { - background.delete(key); + // Only clear OUR entry. An abandoned promise that settles late (or a run + // superseded after expiry) must not evict the live entry that replaced it. + if (background.get(key) === entry) background.delete(key); } })(); - background.set(key, p); - return p; + background.set(key, entry); + return entry.promise as Promise; } diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index 372ba68..f9728bd 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -429,6 +429,99 @@ describe('preview keys the result cache on an origin it actually serves', () => }); }); +// The other half of the same failure, and the one that has no fix at runtime: +// `cacheOrigin` falls back to the REQUEST origin, which for a real Service +// Binding is web-og's hardcoded `https://web` — a non-routable single-label host +// the Cache API declines, i.e. #143 exactly, swallowed by neverFatal. PROD_HOST +// is optional in `Env` and documents itself as the analytics gate ("Unset => +// record everything"), so dropping it from a [vars] block — or adding a named env +// that re-declares vars without it, which [env.preview] already had to do — +// silently reverts this whole fix with no signal. The config is the only place +// that can be guarded, so guard it there. +describe('every deployed environment configures a routable cache origin', () => { + const cfg = parseToml( + readFileSync(fileURLToPath(new URL('../wrangler.toml', import.meta.url)), 'utf8'), + ) as { + vars?: Record; + env?: Record }>; + }; + + const environments: [string, Record | undefined][] = [ + ['[vars]', cfg.vars], + ...Object.entries(cfg.env ?? {}).map( + ([name, e]) => [`[env.${name}.vars]`, e.vars] as [string, Record | undefined], + ), + ]; + + it('declares at least one env to check, so this suite cannot pass vacuously', () => { + expect(environments.length).toBeGreaterThanOrEqual(2); + }); + + it.each(environments)('%s sets PROD_HOST or PUBLIC_BASE_URL', (_label, vars) => { + const configured = vars?.PUBLIC_BASE_URL ?? vars?.PROD_HOST; + expect( + configured, + 'without one of these /internal keys the result cache on the Service Binding origin `https://web`, which the Cache API drops (#143)', + ).toBeTypeOf('string'); + const host = new URL( + (configured as string).includes('//') ? (configured as string) : `https://${configured}`, + ).hostname; + expect(host, 'a single-label host is not routable and is not cacheable').toContain('.'); + }); +}); + +// Belt to that suspenders: a value that IS set but is not routable must not be +// used either. `https://web` parses fine and yields a plausible-looking origin, +// so without this it would sail through and reinstate #143 while looking configured. +describe('/internal/* refuses a configured cache origin that is not routable', () => { + it('ignores a single-label PROD_HOST rather than keying on a host it cannot cache', async () => { + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + seed('https://released.example', k, fixture('v4.5.0')); + + const res = await app.fetch( + svc(`https://released.example/internal/result/honojs/hono/${SHA}`), + { INTERNAL_SECRET, PROD_HOST: 'web' }, + ); + + // Fell through to the request origin, where the warm entry actually is. + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.5.0'); + expect(findReleaseMock).not.toHaveBeenCalled(); + }); + + it('still accepts localhost, which `wrangler dev` really serves on', async () => { + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + seed('http://localhost:8787', k, fixture('v4.4.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), { + INTERNAL_SECRET, + PUBLIC_BASE_URL: 'http://localhost:8787', + }); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.4.0'); + expect(findReleaseMock).not.toHaveBeenCalled(); + }); + + it('documents the unguardable case: unset vars key on the Service Binding origin', async () => { + // Nothing at runtime can recover the public origin here, so this pins what + // actually happens rather than implying it is safe: the entry is written under + // `https://web`, which the real Cache API drops. The wrangler.toml suite above + // is what keeps this shape from ever shipping. + const sha = '7'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + findReleaseMock.mockResolvedValue(fixture('v4.3.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), { + INTERNAL_SECRET, + }); + + expect(res.status).toBe(200); + expect(cacheStore.has(keyUrl(PUBLIC_ORIGIN, k))).toBe(false); + expect(cacheStore.has(keyUrl('https://web', k))).toBe(true); + }); +}); + // A crawler caches whatever the unfurl returns, so anything that makes web-og // WAIT is a #143 risk: the shared policy revalidates a pending answer after 5 // minutes and a partial after 60 seconds, and findRelease's own soft deadline is @@ -506,10 +599,20 @@ describe('/internal/* never blocks the render on a revalidation', () => { expect(findReleaseMock).toHaveBeenCalledTimes(1); }); - // The bound is on `partial && !firstRelease`, not on `partial` alone: a - // truncated traversal that DID find a containing release carries a real tag, - // which web-og renders correctly. 503ing that would throw away a right answer. - it('still serves a partial that carries a real firstRelease', async () => { + // Round 8 (#144). Earlier rounds bounded this on `partial && !firstRelease`, + // reasoning that a truncated traversal which DID find a containing release + // carries a tag web-og renders correctly. find-release.ts says otherwise: the + // tag in that shape is the GALLOP hit, and the bisect that would confirm no + // EARLIER release contains the commit is exactly what the deadline cut short + // ("the gallop-found tag is almost always the right answer; bisect just + // verifies could there be an earlier one", find-release.ts:288-292). "Almost + // always" is a caveat the result card renders and an OG card cannot: web-og + // pins the bare tag for 24h. Answering "which release FIRST contains this + // commit" with a possibly-later release is the failure this product exists to + // avoid, so for a consumer that pins, no partial is servable — the neutral + // placeholder claims nothing, and the permalink it links to shows the + // best-effort answer WITH its caveat. + it('503s rather than pin a partial whose tag the bisect never confirmed', async () => { const sha = '2'.repeat(40); findReleaseMock.mockResolvedValue({ ...(fixture('v4.19.0') as Record), @@ -518,8 +621,81 @@ describe('/internal/* never blocks the render on a revalidation', () => { const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + expect(res.status).toBe(503); + }); + + // ...and the same shape read back from the SHARED slot. `hardTtlFor()` and + // `isFresh()` both test `firstRelease` before `partial`, so a gallop-only + // answer is stored for 30 days and reported fresh forever. Joining the public + // key is what first exposed the OG path to an entry that old (its own cache was + // a flat 30 minutes), so the pin bound has to reject it on the way OUT. + // The underlying terminal-classification bug is on `main` and affects the + // public routes too — tracked separately, not widened into this PR. + it('does not serve a 20-day-old partial the cache classified as terminal', async () => { + const sha = '5'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + seedAged( + PUBLIC_ORIGIN, + k, + { ...(fixture('v4.9.0') as Record), partial: { reason: 'soft_deadline' } }, + 20 * 24 * 60 * 60, + ); + findReleaseMock.mockResolvedValue(fixture('v4.8.0')); // the real earliest + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.8.0'); + expect(findReleaseMock).toHaveBeenCalledTimes(1); + }); + + // Refusing to serve a partial must not turn into refusing to CACHE the refusal. + // resolveLookup writes the computed partial to the slot at HARD_TTL_PARTIAL, but + // every read path rejects it — fresh, SWR and back-off exits alike — so `run()` + // falls through to `load()` again. On a repo that reliably blows the 24s soft + // deadline that is a full traversal per unfurl on the shared token, forever, + // where the flat 30-minute TTL this route replaced made zero upstream calls. + it('does not re-run the lookup for every unfurl of a deadline-blowing repo', async () => { + const sha = '3'.repeat(40); + findReleaseMock.mockResolvedValue(partialFixture()); + + const first = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + const second = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(first.status).toBe(503); + expect(second.status).toBe(503); + expect(findReleaseMock).toHaveBeenCalledTimes(1); + }); + + // The complement, and what stops the throttle above from becoming a permanent + // placeholder: the recorded partial is only honoured inside its own 60-second + // TTL, the same window the shared policy already trusts a partial for. + it('recomputes once the recorded partial ages out of its 60-second window', async () => { + const sha = '4'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + seedAged(PUBLIC_ORIGIN, k, partialFixture(), 61); + findReleaseMock.mockResolvedValue(fixture('v4.20.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + expect(res.status).toBe(200); - expect(await tagOf(res)).toBe('v4.19.0'); + expect(await tagOf(res)).toBe('v4.20.0'); + expect(findReleaseMock).toHaveBeenCalledTimes(1); + }); + + // ...and the throttle must never outrank a REAL answer. A public page view has + // no soft-deadline pressure from web-og and can land a terminal result in the + // shared slot inside that 60-second window; the next unfurl must serve it. + it('serves a real answer that landed in the slot inside the partial window', async () => { + const sha = '6'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + seedAged(PUBLIC_ORIGIN, k, fixture('v4.21.0'), 30); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.21.0'); + expect(findReleaseMock).not.toHaveBeenCalled(); }); }); diff --git a/packages/web/test/resolve.test.ts b/packages/web/test/resolve.test.ts index 2756843..43976ce 100644 --- a/packages/web/test/resolve.test.ts +++ b/packages/web/test/resolve.test.ts @@ -492,7 +492,15 @@ describe('resolveLookup — a pinned consumer is never handed a prior past the b // for a day — the CLAUDE.md guardrail ("Partial state != not yet released") // and the exact outcome `consumerPinsResult` exists to prevent. describe('resolveLookup — a pinned consumer is never handed a cached PARTIAL', () => { - it('fresh exit: recomputes rather than serve a 10s-old partial written by badge.ts', async () => { + // Round 8 narrows this ONE case, and only where it costs nothing: a partial + // still inside its own 60s TTL is handed BACK rather than recomputed. It is not + // thereby pinnable — the route refuses every `partial` (routes/internal.ts) and + // 503s it into a neutral placeholder at max-age=60. Recomputing here instead is + // what made the refusal unthrottled: no read path accepted the entry + // resolveLookup had just written, so on a repo that reliably blows the 24s soft + // deadline every unfurl ran another full traversal on the shared token, where + // the flat 30-minute TTL this route replaced made zero upstream calls. + it('fresh exit: hands back a 10s-old partial rather than re-run the lookup', async () => { const f = makeFakeCache(); const truncated = mkResult({ released: false, partial: true }); f.seed(KEY, truncated, 10); // well inside the 60s partial freshness window @@ -506,12 +514,17 @@ describe('resolveLookup — a pinned consumer is never handed a cached PARTIAL', consumerPinsResult: true, }); - expect(load).toHaveBeenCalledTimes(1); + expect(load).not.toHaveBeenCalled(); expect(r.status).toBe('ok'); - if (r.status === 'ok') expect(r.result.firstRelease?.tag).toBe('4.18.0'); + // Still flagged, so the pinning caller refuses it — the refusal just costs a + // cache read now instead of a findRelease. + if (r.status === 'ok') { + expect(r.result.partial).toBeTruthy(); + expect(r.result.firstRelease).toBeNull(); + } }); - it('SWR exit: a fresh partial is not handed back while a refresh runs behind it', async () => { + it('SWR exit: a fresh partial is never stale-served with a refresh behind it', async () => { const f = makeFakeCache(); const truncated = mkResult({ released: false, partial: true }); f.seed(KEY, truncated, 10); @@ -529,14 +542,62 @@ describe('resolveLookup — a pinned consumer is never handed a cached PARTIAL', consumerPinsResult: true, }); - expect(tasks).toHaveLength(0); // blocked instead of stale-serving the partial + // The fresh exit takes it first, so no background refresh is ever fired and + // the answer is not marked stale — a crawler must not be told "stale" about a + // response the route is going to refuse anyway. + expect(tasks).toHaveLength(0); + expect(load).not.toHaveBeenCalled(); expect(r.status).toBe('ok'); if (r.status === 'ok') { expect(r.stale).toBe(false); - expect(r.result.firstRelease?.tag).toBe('4.18.0'); + expect(r.result.partial).toBeTruthy(); } }); + // find-release.ts returns a SECOND partial shape: a gallop hit the bisect never + // confirmed is the earliest containing release (find-release.ts:293-305). Both + // `hardTtlFor()` and `isFresh()` test `firstRelease` before `partial`, so that + // shape is stored for 30 days and reported fresh forever — a pinning consumer + // would get a possibly-wrong tag from an entry of any age, with no caveat and + // no revalidation. (The terminal misclassification itself is on `main` and + // affects the public routes too; the pin bound rejecting it on the way out is + // what this PR owes.) + it('a gallop-only partial is not treated as terminal, however old', async () => { + const f = makeFakeCache(); + const gallopOnly = { ...mkResult({ released: true }), partial: { reason: 'soft_deadline' } }; + f.seed(KEY, gallopOnly, 20 * 24 * 60 * 60); // 20 days — "fresh forever" today + const load = vi.fn().mockResolvedValue(mkResult({ released: true })); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + }); + + expect(load).toHaveBeenCalledTimes(1); + expect(r.status).toBe('ok'); + if (r.status === 'ok') expect(r.result.partial).toBeUndefined(); + }); + + it('...but a TERMINAL answer of the same age is still served, never recomputed', async () => { + // The complement that stops the bound above from swallowing the warm entries + // this route joined the public key to reuse. + const f = makeFakeCache(); + f.seed(KEY, mkResult({ released: true }), 20 * 24 * 60 * 60); + const load = vi.fn().mockResolvedValue(mkResult({ released: true })); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + }); + + expect(load).not.toHaveBeenCalled(); + expect(r.status).toBe('ok'); + }); + it('stale exit: a partial inside the 30-min bound is still not pinnable', async () => { const f = makeFakeCache(); const truncated = mkResult({ released: false, partial: true }); diff --git a/packages/web/test/single-flight.test.ts b/packages/web/test/single-flight.test.ts new file mode 100644 index 0000000..9a1fb6e --- /dev/null +++ b/packages/web/test/single-flight.test.ts @@ -0,0 +1,83 @@ +// backgroundFlight coalesces stale-while-revalidate refreshes (resolve.ts) in a +// map foreground callers never join. Its hazard is the one singleFlight's own +// header describes: the map is module-level and outlives any single request, so +// an entry registered under request A's IoContext can be handed to request B. +// workerd tears A's IoContext down when A's response ends, so if the refresh had +// not settled by then its promise NEVER settles — and the `finally` that clears +// the entry never runs either. Every later refresh for that key in the isolate +// then joins a dead promise, and background revalidation for it is dead for the +// isolate's lifetime. +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { backgroundFlight } from '../src/single-flight.js'; + +/** A promise that never settles — what a torn-down IoContext leaves behind. */ +function neverSettles(): Promise { + return new Promise(() => {}); +} + +beforeEach(() => { + vi.useFakeTimers(); +}); +afterEach(() => { + vi.useRealTimers(); +}); + +describe('backgroundFlight collapses concurrent refreshes', () => { + it('runs the loader once for two refreshes fired in the same tick', async () => { + const loader = vi.fn(async () => 'v1'); + + const [a, b] = await Promise.all([ + backgroundFlight('same-tick', loader), + backgroundFlight('same-tick', loader), + ]); + + expect(a).toBe('v1'); + expect(b).toBe('v1'); + expect(loader).toHaveBeenCalledTimes(1); + }); + + it('lets the next refresh run once the previous one has settled', async () => { + const loader = vi.fn(async () => 'v1'); + + await backgroundFlight('settled', loader); + await backgroundFlight('settled', loader); + + expect(loader).toHaveBeenCalledTimes(2); + }); +}); + +describe('backgroundFlight self-heals a refresh workerd killed mid-flight', () => { + it('does not hand a later refresh a promise abandoned by a dead IoContext', async () => { + const dead = vi.fn(neverSettles); + const live = vi.fn(async () => 'fresh'); + + // Unfurl A registers the refresh, then its response ends and workerd cancels + // the IoContext: the promise is abandoned, so the loader's `finally` — the + // only thing that clears the entry — never runs. + void backgroundFlight('abandoned', dead); + + // Unfurl B, well after the hard deadline any real refresh could still be + // running under. Joining A's entry here would hang B's refresh too, and every + // later one, for the lifetime of the isolate. + vi.setSystemTime(Date.now() + 60_000); + const b = backgroundFlight('abandoned', live); + + await expect(b).resolves.toBe('fresh'); + expect(live).toHaveBeenCalledTimes(1); + }); + + it('still joins a refresh that is merely SLOW, inside the deadline', async () => { + // The complement, and what keeps the test above from passing vacuously: an + // entry younger than the bound must still coalesce, or the map stops doing + // the job it exists for and four crawlers run four full traversals. + const slow = vi.fn(neverSettles); + const second = vi.fn(async () => 'second'); + + void backgroundFlight('slow', slow); + vi.setSystemTime(Date.now() + 1_000); + void backgroundFlight('slow', second); + + await vi.advanceTimersByTimeAsync(0); + expect(second).not.toHaveBeenCalled(); + }); +}); From 66b4c5ce38a139b4570f2a36614d806cfa7201ea Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 27 Aug 2026 11:09:06 +0100 Subject: [PATCH 12/22] fix(web): drop the unreachable SWR path, cap a pinned partial's TTL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 9 review of #144. Four findings, all on code this PR introduces. 1. The stale-while-revalidate path was dead in production. `findRelease` emits only two LookupResult shapes — TERMINAL (`firstRelease`, no `partial`) and PARTIAL (either sub-shape); a "not yet released" is a thrown error, never a cached result. `/internal` is the only caller that passes `revalidate`, and it also sets `consumerPinsResult`, under which a terminal prior returns at the fresh exit and a partial is either fresh (same exit) or unpinnable. No entry could reach the SWR branch. Removed it, with `coalesce`, `backgroundFlight` and `background()`. The guard that appeared to prove it live seeded `firstRelease: null` with no `partial` — a shape production cannot produce — so it passed without exercising anything reachable. 2. The back-off bypass is therefore unconditional on that path, not cold-only as its comment claimed. Behaviour kept (a crawler asks once; the alternative is a placeholder pinned past the recovery), the claim corrected, and the reachable case pinned by a test that used to assert the opposite on the same impossible shape. 3. Sharing the public slot also made `/internal` a WRITER into it, with `hardTtlFor()` in place of its old flat 30 minutes. `hardTtlFor()` tests `firstRelease` before `partial`, so an OG-triggered gallop-only partial would pin the permalink and the badge to a tag the bisect never confirmed for 30 days, with `isFresh()` never going false. A pinning consumer's write now uses HARD_TTL_PARTIAL for any partial — `unpinnable` trusts one for 60s anyway, so the long TTL bought this caller nothing. The same misclassification on the PUBLIC routes' own writes predates this PR and stays in #155. 4. The routability guard's rationale over-claimed: with PROD_HOST="web" the `??` chain falls through to the request origin, which on the real Service Binding is that same `https://web`. No runtime fix exists — cache.ts requires the key URL to be on the Worker's own hostname, so a synthetic constant no-ops identically. The comment now states what the guard does fix (a routable request origin) and points the rest at the wrangler.toml env suite that already guards it. Mutation-proved, each against the defect in its own title: - reinstating `hardTtlFor()` for the pinning writer → the gallop-only partial test fails (`max-age=2592000` vs `max-age=60`). - narrowing the TTL for EVERY pinning write → both terminal-TTL tests fail, so the guard cannot hold by rejecting everything. - making the bypass cold-only → the new back-off test fails, plus the two round-6/7 pin-bound tests. Net −280 lines. --- packages/web/src/resolve.ts | 94 ++++------ packages/web/src/routes/internal.ts | 50 +++--- packages/web/src/single-flight.ts | 55 ------ .../web/test/internal-cache-origin.test.ts | 141 +++++++++------ packages/web/test/resolve.test.ts | 165 +----------------- packages/web/test/single-flight.test.ts | 83 --------- 6 files changed, 154 insertions(+), 434 deletions(-) delete mode 100644 packages/web/test/single-flight.test.ts diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index 09c4808..f96d300 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -29,7 +29,7 @@ import { } from '@released/core'; import { upstreamStatusOf } from './analytics.js'; import type { CacheEntry, WorkerCache } from './cache.js'; -import { backgroundFlight, singleFlight } from './single-flight.js'; +import { singleFlight } from './single-flight.js'; // Freshness windows + hard TTLs (seconds). const FRESH_WINDOW_PENDING = 5 * 60; // re-check non-released answers every 5 min @@ -46,11 +46,10 @@ const NEG_TTL = 60; // back off this long when upstream is down // flat TTL before it shared this 24h slot, so bounding is never worse than the // code it replaced. // -// It bounds EVERY exit that hands back a prior, not just the SWR one. Rounds 4/5 -// bounded only the stale-while-revalidate return, which left both stale-if-error -// exits (back-off below, and the transient catch) free to serve an answer of any -// age — and sharing the 24h slot is exactly what made a 23h-old prior possible on -// the /internal path (`consumerPinsResult`, set by the OG route). Public HTML +// It bounds EVERY exit that hands back a prior. Sharing the 24h slot is exactly +// what made a 23h-old prior possible on the /internal path (`consumerPinsResult`, +// set by the OG route): before it, that route had a flat 30-minute TTL of its own +// and an entry that old could not exist. Public HTML // routes leave the flag unset and keep stale-if-error UNBOUNDED: a human sees an // explicit "stale as of" caveat and can reload, and their answer is not pinned // anywhere, so serving through a long outage is the right degrade for them. @@ -111,36 +110,24 @@ export async function resolveLookup(args: { key: string; load: () => Promise; now?: () => number; - /** Opt-in stale-while-revalidate, for callers on a latency-critical path. - * When given, a cached-but-stale answer is returned IMMEDIATELY and the - * revalidation is handed to this callback to run off the response path - * (`executionCtx.waitUntil`). Callers that can afford to wait — the public - * HTML routes — omit it and keep the blocking behaviour. */ - revalidate?: (task: Promise) => void; /** Opt-in for callers whose consumer only ever asks ONCE, so a soft failure * becomes permanent for them (the OG crawler). When set, the shared negative * back-off marker is honoured only if there is a prior we can actually SERVE * — with nothing servable, an attempt beats handing back a placeholder that - * gets cached forever. "Nothing servable" is broader than an empty slot: with - * `consumerPinsResult`, a prior past `MAX_STALE_PINNED` or a truncated - * `partial` is unservable too, so a warm-but-unpinnable key reaches this path - * as well. That is deliberate (the alternative is a permanent placeholder), - * but it is NOT throttled — see the fall-through comment below for the cost. - * The marker is still WRITTEN on failure, and callers that can retry (the - * public HTML routes) omit this and keep backing off. */ + * gets cached forever. The marker is still WRITTEN on failure, and callers + * that can retry (the public HTML routes) omit this and keep backing off. + * + * Be honest about the reachable behaviour. The only caller that sets this also + * sets `consumerPinsResult`, and `findRelease` emits just two entry shapes: + * TERMINAL (fresh forever, so it returns at the fresh exit above) and PARTIAL + * (fresh for its 60s, unpinnable after). Neither can reach the stale-serve on + * the back-off line below, so on that path the bypass is UNCONDITIONAL, not + * cold-only. That is accepted, not overlooked: a crawler asks once, so the + * alternative is a placeholder pinned long after the host recovers, and the + * cost is the unthrottled load documented at the fall-through. The + * `prior && !unpinnable(prior)` stale-serve is live for the public routes, + * which do not set either flag. */ bypassBackOffWhenUnservable?: boolean; - /** Internal. Set false for the background refresh on the SWR path: that task - * runs under `executionCtx.waitUntil`, whose IoContext workerd can tear down - * before the subrequest settles. singleFlight only clears its module-level - * entry in the loader's `finally`, so a background owner that never settles - * leaves a dead promise under this key, and every later request in the same - * isolate — a human on the permalink, badge.ts on the same cull/nopre key — - * joins it: a hang, or "Cannot perform I/O on behalf of a different request", - * which resolveLookup classifies as a non-transient error and the page renders - * as a hard failure. A foreground caller is always a live, awaiting request; a - * background one is not. A duplicated refresh is far cheaper than poisoning - * the key for the lifetime of the isolate. */ - coalesce?: boolean; /** Opt-in for callers whose consumer CACHES whatever we hand back, for longer * than we can correct (the OG crawler pins a rendered PNG for 24h). When set, * no exit returns a prior older than `MAX_STALE_PINNED`: we would rather pay a @@ -148,15 +135,7 @@ export async function resolveLookup(args: { * placeholder, than pin a day-old answer that has since changed. */ consumerPinsResult?: boolean; }): Promise { - const { - cache, - key, - load, - revalidate, - bypassBackOffWhenUnservable, - coalesce, - consumerPinsResult, - } = args; + const { cache, key, load, bypassBackOffWhenUnservable, consumerPinsResult } = args; const now = args.now ?? Date.now; /** True when a cached entry must NOT be handed to a consumer that pins the @@ -209,26 +188,6 @@ export async function resolveLookup(args: { cached: true, }); - // Stale-while-revalidate: serve what we have, refresh behind it — but only up to - // MAX_STALE_PINNED, past which we block rather than hand back an answer a crawler - // would pin for a day. The refresh is a plain recursive call WITHOUT `revalidate`, - // so it takes the blocking path and cannot recurse again, and with - // `coalesce: false` so a task the runtime may kill never owns the foreground - // flight for this key. `backgroundFlight` then restores the collapsing that - // dropping out of `singleFlight` cost: this branch fires on EVERY request in the - // stale window, so four crawlers unfurling one link in the same second would - // otherwise run four full lookups against the same repo on the shared token. - // Errors are absorbed here — a background failure must not surface as an - // unhandled rejection on a response that already succeeded. - if (prior && revalidate && prior.ageSeconds < MAX_STALE_PINNED && !unpinnable(prior)) { - revalidate( - backgroundFlight(key, () => resolveLookup({ cache, key, load, now, coalesce: false })).catch( - () => undefined, - ), - ); - return staleHit(); - } - // Did we try (and fail transiently) very recently? If so, don't pound the // upstream again yet — serve the last-known-good if we have one, else a soft // transient. @@ -262,10 +221,21 @@ export async function resolveLookup(args: { const re = await cache.getEntry(key); if (re && isFresh(re) && !unpinnable(re)) return re.value; const r = await load(); - await cache.put(key, r, hardTtlFor(r)); + // A consumer that PINS what we hand back is also the most deadline-pressured + // producer of partials, and `hardTtlFor()` tests `firstRelease` before + // `partial` — so a gallop-only partial (find-release.ts ~295: the gallop hit + // WITH `partial: soft_deadline`) would take the TERMINAL 30-day branch and + // `isFresh()` would report it fresh forever. On the shared slot that pins the + // public permalink and badge to a tag the bisect never confirmed, for a month, + // with no upstream call able to correct it. `unpinnable` trusts a partial for + // HARD_TTL_PARTIAL and no longer, so the long TTL buys this caller nothing. + // (The same misclassification on the PUBLIC routes' own writes predates this + // PR and is tracked in #155; their semantics are deliberately untouched here.) + const pinnedPartial = Boolean(consumerPinsResult && r.partial); + await cache.put(key, r, pinnedPartial ? HARD_TTL_PARTIAL : hardTtlFor(r)); return r; }; - const result = coalesce === false ? await run() : await singleFlight(key, run); + const result = await singleFlight(key, run); return { status: 'ok', result, stale: false, staleAsOf: null, cached: false }; } catch (err) { if (err instanceof NotYetReleasedError) return { status: 'not_yet', error: err }; diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index bcba795..70c3bcc 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -87,8 +87,19 @@ function originOf(value: string | undefined): string | null { // mechanism. `https://web` parses cleanly and yields a plausible-looking // origin, so a var accidentally set to the Service-Binding target would // otherwise sail through here looking configured while caching nothing. - // Falling through to the next candidate is strictly better: a wrong-but- - // routable origin still persists, a non-routable one persists nothing. + // + // Be precise about the reach of this guard, because the ?? chain it falls + // through to ends at the REQUEST origin. It fixes the case where the request + // origin is routable (a `wrangler dev` or public request to /internal): the + // key lands on a host this Worker actually serves and the entry persists. It + // does NOT fix the Service-Binding case, where the request origin is that same + // `https://web` — there is no routable candidate left, and a synthetic constant + // would not help, because cache.ts's rule is that the key URL must be on the + // Worker's OWN hostname (a made-up one no-ops exactly like `cache.invalid`). + // That case is unguardable at runtime and is guarded in config instead, by the + // wrangler.toml suite that requires every env to set PROD_HOST or + // PUBLIC_BASE_URL. + // // `localhost` is the one single-label host that is real — `wrangler dev` // serves on it, and README tells developers to put it in PUBLIC_BASE_URL. const { hostname } = new URL(origin); @@ -99,20 +110,6 @@ function originOf(value: string | undefined): string | null { } } -/** Run a task off the response path. web-og awaits this endpoint with no timeout - * and the crawler caches whatever it finally gets, so a revalidation that blocks - * here is the #143 mechanism reached from a merely-stale entry: findRelease's own - * soft deadline is 24s, and a blown deadline hands the crawler the neutral - * placeholder at max-age=60. */ -function background(c: Context, task: Promise): void { - try { - c.executionCtx.waitUntil(task); - } catch { - // No ExecutionContext (unit tests, some dev runners): the refresh still runs, - // it just isn't kept alive by the runtime. Already .catch()-guarded upstream. - } -} - /** Wrap a cache so no Cache API call can be fatal. The key URL is deliberately * not this request's own origin (see cacheOrigin) and the Cache API is entitled * to refuse such a read or write; that must degrade to "served, just not @@ -176,9 +173,12 @@ async function resolveResult(c: Context, input: LookupInput): Promise // negative back-off that keeps a down host from being pounded. A flat TTL here // would downgrade a terminal slot the permalink would have kept for 30 days, // and a bare read would keep serving a 60-second partial for far longer than - // the public page does. Sharing the policy must NOT mean sharing the wait: this - // caller is a crawler's critical path, so it opts into stale-while-revalidate - // (`revalidate`) and never blocks on a refresh. + // the public page does. This caller BLOCKS on a refresh when the slot is not + // servable. An earlier round added a stale-while-revalidate opt-out for it, but + // `findRelease` emits only two entry shapes — TERMINAL (fresh forever) and + // PARTIAL (fresh for its 60s, then unpinnable to a pinning consumer) — and + // neither can be both stale and servable, so nothing could ever reach it (round + // 9: dead code, removed). web-og waiting on a cold or unpinnable slot is #152. // Options are stated explicitly because they are what the // `cull`/`nopre` key parts promise. Anubis-protected hosts get a relay-backed // fetch (see makeProvider/relay.ts). The web-og caller chose to wait for this. @@ -191,13 +191,19 @@ async function resolveResult(c: Context, input: LookupInput): Promise strict: false, includePrereleases: false, }), - // Serve a cached answer immediately and refresh behind it (see background()). - revalidate: (task) => background(c, task), // The shared key means a public page view's failed lookup also writes the // shared `:neg` back-off marker. Honouring that on a COLD slot would 503 here // without ever calling findRelease, and the crawler caches the resulting // placeholder for good — #143 all over again, via the alignment that fixes it. - // With a prior to stale-serve, the back-off still holds. + // + // On THIS path the opt-out is unconditional, not cold-only, and the flag name + // reads more conditional than the code is: of the two entry shapes findRelease + // emits, a TERMINAL prior returns at the fresh exit and a PARTIAL one is either + // fresh (same exit) or unpinnable, so no prior ever reaches the marker's + // stale-serve. The cost is real and accepted — during a + // host outage each unfurl runs its own findRelease out to the hard deadline + // (see resolveLookup's fall-through comment) — because the alternative for a + // consumer that asks ONCE is a placeholder pinned long after the host recovers. // // The asymmetry is deliberate: this caller opts out of READING the marker on // a cold slot, but resolveLookup still WRITES it, so a failure discovered diff --git a/packages/web/src/single-flight.ts b/packages/web/src/single-flight.ts index 6576dfe..0a3fa8d 100644 --- a/packages/web/src/single-flight.ts +++ b/packages/web/src/single-flight.ts @@ -22,58 +22,3 @@ export async function singleFlight(key: string, loader: Loader): Promise; startedAt: number }>(); - -/** How long a background entry may be joined before it is presumed abandoned. - * findRelease's own HARD deadline is 28s, so a refresh still running past this - * is not slow — it is a promise whose IoContext went away. Erring long is the - * safe direction: the cost of expiring too early is one duplicated traversal, - * while the cost of never expiring is no revalidation at all for that key. */ -const MAX_BACKGROUND_AGE_MS = 30_000; - -/** Collapse concurrent BACKGROUND refreshes for `key` onto one run, in a map - * foreground callers never join. Registration is synchronous, so two refreshes - * fired in the same tick cannot both miss it. */ -export function backgroundFlight(key: string, loader: Loader): Promise { - const existing = background.get(key); - if (existing && Date.now() - existing.startedAt < MAX_BACKGROUND_AGE_MS) { - return existing.promise as Promise; - } - const entry: { promise: Promise; startedAt: number } = { - promise: undefined as unknown as Promise, - startedAt: Date.now(), - }; - entry.promise = (async () => { - try { - return await loader(); - } finally { - // Only clear OUR entry. An abandoned promise that settles late (or a run - // superseded after expiry) must not evict the live entry that replaced it. - if (background.get(key) === entry) background.delete(key); - } - })(); - background.set(key, entry); - return entry.promise as Promise; -} diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index f9728bd..a0d381e 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -122,7 +122,17 @@ function seedAged(origin: string, key: string, value: unknown, ageSeconds: numbe /** A non-terminal result — looked up, not in a release yet. The shared policy * (resolve.ts) revalidates these every 5 minutes; `subject` identifies which - * copy of the answer a response came from. */ + * copy of the answer a response came from. + * + * Honest scope (round 9, #144): `findRelease` does not currently EMIT this shape. + * Its three LookupResult return sites are a terminal answer, a gallop-hit + * `partial`, and a soft-deadline `partial` with `firstRelease: null`; a genuine + * "not yet released" is a thrown NotYetReleasedError, never a cached result. So + * the tests below that seed it exercise resolve.ts's GENERIC pending branch + * (HARD_TTL_PENDING / FRESH_WINDOW_PENDING, both pre-dating this PR) through this + * route — they are not evidence about a state production can reach today. Any + * claim about the OG path's reachable behaviour must be pinned on one of the two + * shapes above instead. */ function pendingFixture(subject: string): unknown { return { ...(fixture('unused') as Record), firstRelease: null, subject }; } @@ -164,16 +174,6 @@ async function tagOfSlot(origin: string, key: string): Promise; resolve: (v: unknown) => void } { - let resolve!: (v: unknown) => void; - const promise = new Promise((r) => { - resolve = r; - }); - return { promise, resolve }; -} - beforeEach(() => { cacheStore.clear(); cacheFault = 'none'; @@ -347,6 +347,45 @@ describe('/internal/* follows the cache policy that governs the shared slot', () expect(cacheControlOf(PUBLIC_ORIGIN, k)).toBe('public, max-age=60'); }); + // Round 9 (#144). The partial above has `firstRelease: null`, which `hardTtlFor()` + // already routes to its 60-second branch. The OTHER partial shape does not: + // find-release.ts (~295) returns the gallop hit WITH `partial`, and `hardTtlFor()` + // tests `firstRelease` BEFORE `partial`, so it takes the terminal 30-day branch — + // and `isFresh()` reports it fresh forever for the same reason. On `main` that + // could not reach the public routes: /internal wrote a flat 30 minutes onto a key + // nothing else read. Sharing the slot (this PR) makes the most deadline-pressured + // producer of partials a WRITER into it, so one OG-triggered truncated traversal + // would pin the permalink and the badge to a tag the bisect never confirmed for a + // month, with no upstream call able to correct it. `unpinnable` trusts a partial + // for 60 seconds and no longer, so the long TTL buys this route nothing. + // (#155 tracks the same misclassification on the public routes' OWN writes.) + it('writes a gallop-only partial with the 60-second TTL, not the terminal 30 days', async () => { + const sha = '9'.repeat(40); + findReleaseMock.mockResolvedValue({ + ...(fixture('v4.19.0') as Record), + partial: { reason: 'soft_deadline', candidatesTried: 3 }, + }); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + expect(res.status).toBe(503); // never pinned to the crawler — the round-8 guard + + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + expect(cacheControlOf(PUBLIC_ORIGIN, k)).toBe('public, max-age=60'); + }); + + // The complement: narrowing the TTL for a pinning consumer must not touch the + // answer the route actually exists to reuse. A terminal result still gets 30 days. + it('a terminal answer keeps the 30-day TTL when the SAME caller writes it', async () => { + const sha = 'a'.repeat(40); + findReleaseMock.mockResolvedValue(fixture('v4.18.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + expect(res.status).toBe(200); + + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + expect(cacheControlOf(PUBLIC_ORIGIN, k)).toBe(`public, max-age=${30 * 24 * 60 * 60}`); + }); + it('revalidates a pending answer past its 5-minute freshness window', async () => { const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); seedAged(PUBLIC_ORIGIN, k, pendingFixture('stale pending'), 10 * 60); @@ -356,9 +395,9 @@ describe('/internal/* follows the cache policy that governs the shared slot', () await settle(); expect(res.status).toBe(200); - // The revalidation still happens — it just no longer sits on the render path - // (see the stale-while-revalidate suite below); the slot ends up refreshed so - // the OG card can't keep rendering the pending answer for another 30 minutes. + // The revalidation happens on the render path (this route blocks; #152) and + // the slot ends up refreshed, so the OG card can't keep rendering the stale + // answer for another 30 minutes. expect(findReleaseMock).toHaveBeenCalledOnce(); expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.12.0'); }); @@ -522,36 +561,16 @@ describe('/internal/* refuses a configured cache origin that is not routable', ( }); }); -// A crawler caches whatever the unfurl returns, so anything that makes web-og -// WAIT is a #143 risk: the shared policy revalidates a pending answer after 5 -// minutes and a partial after 60 seconds, and findRelease's own soft deadline is -// 24s. Blocking on that revalidation would hand the crawler a placeholder with -// max-age=60 — the #143 outcome, reached from a merely-stale entry instead of a -// cold one. So on this path a cached answer is served IMMEDIATELY and the refresh -// runs in the background. (A genuinely COLD slot still blocks — there is nothing -// to serve — but it write-backs, so it is cold at most once.) -describe('/internal/* never blocks the render on a revalidation', () => { - it('serves a stale pending answer without waiting for the upstream lookup', async () => { - const sha = 'b'.repeat(40); - const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); - seedAged(PUBLIC_ORIGIN, k, pendingFixture('stale pending'), 10 * 60); - const slow = deferred(); - findReleaseMock.mockReturnValue(slow.promise); - - // The upstream lookup has NOT resolved at this point. If the render waited on - // it, this await never returns and the test times out — which is the whole claim. - const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); - - expect(res.status).toBe(200); - expect(await subjectOf(res)).toBe('stale pending'); - - slow.resolve(fixture('v4.12.0')); - await settle(); - // The revalidation was not skipped — it ran behind the render. - expect(findReleaseMock).toHaveBeenCalledOnce(); - expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.12.0'); - }); - +// web-og renders `firstRelease?.tag ?? 'not yet released'` and long-caches any +// non-null result for 24h, and nothing here can invalidate a PNG already made. A +// `partial` is a truncated traversal, so neither of its shapes may be pinned: with +// `firstRelease: null` a RELEASED commit becomes a definite "not yet released", and +// WITH one the tag is the gallop hit the bisect never confirmed is the earliest. +// Both fall through to a 503, which web-og renders as the neutral placeholder at +// max-age=60 — it claims nothing and self-heals. The refusal is then throttled by +// the partial the lookup just recorded, so a repo that reliably blows the soft +// deadline costs one traversal per 60s per key, not one per unfurl. +describe('/internal/* refuses to pin a partial, and throttles the refusal', () => { // Round 7 (#144). This used to assert the opposite — that a cached `partial` // was stale-served to web-og while a refresh ran behind it. That is the bug: // web-og renders `firstRelease?.tag ?? 'not yet released'` and long-caches any @@ -651,7 +670,7 @@ describe('/internal/* never blocks the render on a revalidation', () => { // Refusing to serve a partial must not turn into refusing to CACHE the refusal. // resolveLookup writes the computed partial to the slot at HARD_TTL_PARTIAL, but - // every read path rejects it — fresh, SWR and back-off exits alike — so `run()` + // every read path rejects it — the fresh exit and the back-off exit alike — so `run()` // falls through to `load()` again. On a repo that reliably blows the 24s soft // deadline that is a full traversal per unfurl on the shared token, forever, // where the flat 30-minute TTL this route replaced made zero upstream calls. @@ -792,9 +811,18 @@ describe('/internal/* does not let a shared back-off marker cause a permanent pl expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.12.11'); }); - it('still honours the marker when there IS a prior — a down host is never pounded', async () => { + // Round 9 (#144). This used to claim the opposite — "the marker still holds when + // there IS a prior" — on a seeded `firstRelease: null, no partial` entry, a shape + // findRelease never emits (see pendingFixture). On the two shapes it DOES emit the + // bypass is unconditional, and that is worth pinning honestly rather than papering + // over: a TERMINAL prior returns at the fresh exit before the marker is ever read, + // and a PARTIAL prior past its 60s is unpinnable, so the marker is skipped and the + // lookup runs. The cost (an outage is re-probed by every unfurl) is accepted in + // resolveLookup's fall-through comment; the alternative for a consumer that asks + // once is a placeholder pinned long after the host recovers. + it('skips a warm marker even WITH a stale partial prior — the bypass is not cold-only', async () => { const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); - seedAged(PUBLIC_ORIGIN, k, pendingFixture('last known good'), 10 * 60); + seedAged(PUBLIC_ORIGIN, k, partialFixture(), 10 * 60); // past its 60s: unpinnable seedAged(PUBLIC_ORIGIN, `${k}:neg`, { transient: true, kind: 'github_server_error' }, 10); findReleaseMock.mockResolvedValue(fixture('v4.12.11')); @@ -802,9 +830,22 @@ describe('/internal/* does not let a shared back-off marker cause a permanent pl await settle(); expect(res.status).toBe(200); - expect(await subjectOf(res)).toBe('last known good'); - // Stale-serve is strictly better than a recompute here, so the back-off holds - // and the upstream is left alone — including on the background revalidation. + expect(await tagOf(res)).toBe('v4.12.11'); + expect(findReleaseMock).toHaveBeenCalledTimes(1); + }); + + // ...and the complement, so the guard above cannot pass by simply never reading + // the marker: a TERMINAL prior is served from the fresh exit, upstream untouched. + it('a terminal prior is still served from cache while the marker is warm', async () => { + const sha = '8'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + seedAged(PUBLIC_ORIGIN, k, fixture('v4.11.0'), 10 * 60); + seedAged(PUBLIC_ORIGIN, `${k}:neg`, { transient: true, kind: 'github_server_error' }, 10); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.11.0'); expect(findReleaseMock).not.toHaveBeenCalled(); }); }); diff --git a/packages/web/test/resolve.test.ts b/packages/web/test/resolve.test.ts index 43976ce..9611158 100644 --- a/packages/web/test/resolve.test.ts +++ b/packages/web/test/resolve.test.ts @@ -255,138 +255,9 @@ describe('resolveLookup — real answers pass through', () => { }); }); -// Round-4 review of #144. The stale-while-revalidate path added in 4cdb212 is -// what lets the OG render return without blocking on a refresh. Both guards -// below pin a defect in THAT path, not in the cache-key alignment #144 is about. -describe('resolveLookup — stale-while-revalidate is bounded', () => { - it('serves a recently-stale prior immediately and refreshes behind it', async () => { - const f = makeFakeCache(); - // Past the 5-minute pending freshness window, well inside the SWR bound. - f.seed(KEY, mkResult({ released: false }), 10 * 60); - const load = vi.fn().mockResolvedValue(mkResult({ released: true })); - const tasks: Promise[] = []; - - const r = await resolveLookup({ - cache: f.cache, - key: KEY, - load, - revalidate: (t) => { - tasks.push(t); - }, - }); - - expect(r.status).toBe('ok'); - if (r.status === 'ok') expect(r.stale).toBe(true); - expect(tasks).toHaveLength(1); - await Promise.all(tasks); - expect(load).toHaveBeenCalledTimes(1); - }); - - it('blocks rather than hand back an answer stale past the bound', async () => { - const f = makeFakeCache(); - // 23h old: still inside HARD_TTL_PENDING (24h), so getEntry returns it. - // web-og long-caches ANY non-null result for 24h, so serving this unblocked - // pins a day-old "not yet released" card in the crawler's cache for another - // day, and the background refresh cannot invalidate the PNG already made. - f.seed(KEY, mkResult({ released: false }), 23 * 60 * 60); - const load = vi.fn().mockResolvedValue(mkResult({ released: true })); - const tasks: Promise[] = []; - - const r = await resolveLookup({ - cache: f.cache, - key: KEY, - load, - revalidate: (t) => { - tasks.push(t); - }, - }); - - expect(tasks).toHaveLength(0); - expect(r.status).toBe('ok'); - if (r.status === 'ok') { - expect(r.stale).toBe(false); - expect(r.result.firstRelease?.tag).toBe('4.18.0'); - } - expect(load).toHaveBeenCalledTimes(1); - }); - - // Round-5 review of #144. `coalesce: false` (round 4) kept the background - // refresh from OWNING the singleFlight entry, but it also dropped it out of - // coalescing entirely — and the SWR branch fires on EVERY request in the stale - // window. One link unfurled by Slack, X, Discord and LinkedIn in the same - // second in one colo then ran four full findRelease traversals against the same - // repo on the shared GITHUB_TOKEN, none seeing the others. - it('collapses concurrent background refreshes for one key onto a single lookup', async () => { - const f = makeFakeCache(); - f.seed(KEY, mkResult({ released: false }), 10 * 60); - - // Slow enough that all four refreshes are genuinely in flight together. - const load = vi.fn().mockImplementation( - () => - new Promise((res) => { - setTimeout(() => res(mkResult({ released: true })), 10); - }), - ); - const tasks: Promise[] = []; - const revalidate = (t: Promise) => { - tasks.push(t); - }; - - const results = await Promise.all( - Array.from({ length: 4 }, () => - resolveLookup({ cache: f.cache, key: KEY, load, revalidate }), - ), - ); - - // Every caller still gets its stale answer immediately — collapsing the - // refresh must not make anyone wait. - for (const r of results) { - expect(r.status).toBe('ok'); - if (r.status === 'ok') expect(r.stale).toBe(true); - } - expect(tasks).toHaveLength(4); - - await Promise.all(tasks); - expect(load).toHaveBeenCalledTimes(1); - }); - - it('does not let a torn-down background refresh poison the key for the isolate', async () => { - const f = makeFakeCache(); - f.seed(KEY, mkResult({ released: false }), 10 * 60); - - // The background refresh never settles — exactly what a waitUntil task looks - // like when workerd tears the IoContext down mid-subrequest. singleFlight - // only clears its entry in the loader's `finally`, so if the background call - // owns the flight, that entry is never cleared. - const hung = vi.fn().mockReturnValue(new Promise(() => {})); - const first = await resolveLookup({ - cache: f.cache, - key: KEY, - load: hung, - revalidate: () => {}, - }); - expect(first.status).toBe('ok'); - // Let the background call reach its load() and register whatever flight it - // is going to register, before a second request asks for the same key. - await new Promise((resolve) => setTimeout(resolve, 0)); - expect(hung).toHaveBeenCalledTimes(1); - - // A later request in the SAME isolate on the SAME key — a human on the - // permalink, or badge.ts, which share this slot — must still get an answer. - const good = vi.fn().mockResolvedValue(mkResult({ released: true })); - const settled = await Promise.race([ - resolveLookup({ cache: f.cache, key: KEY, load: good }), - new Promise((resolve) => setTimeout(() => resolve('JOINED-A-DEAD-FLIGHT'), 100)), - ]); - - expect(settled).not.toBe('JOINED-A-DEAD-FLIGHT'); - expect(good).toHaveBeenCalledTimes(1); - }); -}); - -// Round-6 review of #144. Rounds 4/5 bounded only the stale-while-revalidate -// return, so both stale-if-error exits — the back-off short-circuit and the -// transient catch — could still hand the OG crawler a prior of any age. Sharing +// Round-6 review of #144. Earlier rounds bounded only one exit, so both +// stale-if-error exits — the back-off short-circuit and the transient catch — +// could still hand the OG crawler a prior of any age. Sharing // the public routes' 24h slot is what made that reachable: before #143 this // route had a flat 30-minute TTL, so a 23h-old prior could not exist on it. // A pinned consumer renders whatever it gets into a PNG cached for a day. @@ -524,36 +395,6 @@ describe('resolveLookup — a pinned consumer is never handed a cached PARTIAL', } }); - it('SWR exit: a fresh partial is never stale-served with a refresh behind it', async () => { - const f = makeFakeCache(); - const truncated = mkResult({ released: false, partial: true }); - f.seed(KEY, truncated, 10); - const fresh = mkResult({ released: true }); - const load = vi.fn().mockResolvedValue(fresh); - const tasks: Promise[] = []; - - const r = await resolveLookup({ - cache: f.cache, - key: KEY, - load, - revalidate: (t) => { - tasks.push(t); - }, - consumerPinsResult: true, - }); - - // The fresh exit takes it first, so no background refresh is ever fired and - // the answer is not marked stale — a crawler must not be told "stale" about a - // response the route is going to refuse anyway. - expect(tasks).toHaveLength(0); - expect(load).not.toHaveBeenCalled(); - expect(r.status).toBe('ok'); - if (r.status === 'ok') { - expect(r.stale).toBe(false); - expect(r.result.partial).toBeTruthy(); - } - }); - // find-release.ts returns a SECOND partial shape: a gallop hit the bisect never // confirmed is the earliest containing release (find-release.ts:293-305). Both // `hardTtlFor()` and `isFresh()` test `firstRelease` before `partial`, so that diff --git a/packages/web/test/single-flight.test.ts b/packages/web/test/single-flight.test.ts deleted file mode 100644 index 9a1fb6e..0000000 --- a/packages/web/test/single-flight.test.ts +++ /dev/null @@ -1,83 +0,0 @@ -// backgroundFlight coalesces stale-while-revalidate refreshes (resolve.ts) in a -// map foreground callers never join. Its hazard is the one singleFlight's own -// header describes: the map is module-level and outlives any single request, so -// an entry registered under request A's IoContext can be handed to request B. -// workerd tears A's IoContext down when A's response ends, so if the refresh had -// not settled by then its promise NEVER settles — and the `finally` that clears -// the entry never runs either. Every later refresh for that key in the isolate -// then joins a dead promise, and background revalidation for it is dead for the -// isolate's lifetime. -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { backgroundFlight } from '../src/single-flight.js'; - -/** A promise that never settles — what a torn-down IoContext leaves behind. */ -function neverSettles(): Promise { - return new Promise(() => {}); -} - -beforeEach(() => { - vi.useFakeTimers(); -}); -afterEach(() => { - vi.useRealTimers(); -}); - -describe('backgroundFlight collapses concurrent refreshes', () => { - it('runs the loader once for two refreshes fired in the same tick', async () => { - const loader = vi.fn(async () => 'v1'); - - const [a, b] = await Promise.all([ - backgroundFlight('same-tick', loader), - backgroundFlight('same-tick', loader), - ]); - - expect(a).toBe('v1'); - expect(b).toBe('v1'); - expect(loader).toHaveBeenCalledTimes(1); - }); - - it('lets the next refresh run once the previous one has settled', async () => { - const loader = vi.fn(async () => 'v1'); - - await backgroundFlight('settled', loader); - await backgroundFlight('settled', loader); - - expect(loader).toHaveBeenCalledTimes(2); - }); -}); - -describe('backgroundFlight self-heals a refresh workerd killed mid-flight', () => { - it('does not hand a later refresh a promise abandoned by a dead IoContext', async () => { - const dead = vi.fn(neverSettles); - const live = vi.fn(async () => 'fresh'); - - // Unfurl A registers the refresh, then its response ends and workerd cancels - // the IoContext: the promise is abandoned, so the loader's `finally` — the - // only thing that clears the entry — never runs. - void backgroundFlight('abandoned', dead); - - // Unfurl B, well after the hard deadline any real refresh could still be - // running under. Joining A's entry here would hang B's refresh too, and every - // later one, for the lifetime of the isolate. - vi.setSystemTime(Date.now() + 60_000); - const b = backgroundFlight('abandoned', live); - - await expect(b).resolves.toBe('fresh'); - expect(live).toHaveBeenCalledTimes(1); - }); - - it('still joins a refresh that is merely SLOW, inside the deadline', async () => { - // The complement, and what keeps the test above from passing vacuously: an - // entry younger than the bound must still coalesce, or the map stops doing - // the job it exists for and four crawlers run four full traversals. - const slow = vi.fn(neverSettles); - const second = vi.fn(async () => 'second'); - - void backgroundFlight('slow', slow); - vi.setSystemTime(Date.now() + 1_000); - void backgroundFlight('slow', second); - - await vi.advanceTimersByTimeAsync(0); - expect(second).not.toHaveBeenCalled(); - }); -}); From 317ca2bb5223db34f648174005a35281266c6581 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 27 Aug 2026 11:46:19 +0100 Subject: [PATCH 13/22] docs(web): correct two rationales that read as adequacy (#156, #157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 10 review, doc-only — no behaviour change. - internal.ts: the "throttled to one traversal per 60s per key" note is true but reads as a bounded cost. HARD_TTL_PARTIAL and web-og's placeholder max-age are BOTH 60s, so under active unfurling the cadences coincide and the throttle buys close to nothing: on a deadline-heavy repo the card never converges and upstream load goes to a full traversal per minute. Stated, and the remedy (web-og short-caching a gallop-hit card) tracked in #156. - resolve.ts: the existing rebuttal covers the crawler but not the second-order effect — every bypassed load re-stamps the marker, so it is almost never older than NEG_TTL and human page views sit on "checking..." for the whole outage. Tracked in #157. Co-Authored-By: Claude Opus 5 --- packages/web/src/resolve.ts | 9 +++++++++ packages/web/src/routes/internal.ts | 11 ++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index f96d300..44ad0c5 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -214,6 +214,15 @@ export async function resolveLookup(args: { // alternative is a placeholder pinned in its cache long after the host // recovers. Gating the bypass on a fraction of NEG_TTL would only move which // unfurls get the permanent placeholder, not stop them. + // + // What that argument does NOT cover, and what #157 tracks: every bypassed load + // RE-STAMPS the marker, so under a steady crawler cadence the marker is almost + // never older than NEG_TTL. Human page views on this key do not bypass, so they + // hit a warm marker on nearly every request and sit on the "checking..." card + // for the whole outage instead of getting a retry window each minute. The + // crawler's unthrottled probing starves the humans' back-off of its recovery + // window; fixing that means changing the marker's semantics (a `bypassed` flag, + // or not re-stamping on a bypassed load), not the bypass condition. } try { diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 70c3bcc..99c9575 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -249,7 +249,16 @@ async function resolveResult(c: Context, input: LookupInput): Promise // // resolveLookup hands back a partial it computed less than HARD_TTL_PARTIAL ago // rather than recomputing it (see `unpinnable`), so this 503 is throttled to one - // traversal per 60s per key instead of one per unfurl. + // traversal per 60s per key. Do NOT read that as "the cost is bounded": web-og + // short-caches the neutral placeholder at max-age=60 and HARD_TTL_PARTIAL is + // also 60, so for a URL under active unfurling the two cadences COINCIDE and the + // throttle buys close to nothing. On a repo that reliably blows the soft deadline + // (GNOME/gimp: large tag set, single-instance relay) the card never converges — + // it is the placeholder forever — and upstream load for that key goes from ~0 to + // a full traversal per minute for as long as the link is being unfurled. That is + // the accepted price of not pinning an unconfirmed answer for 24h; making the + // card render the gallop hit while keeping it revalidatable needs web-og to + // short-cache it, which is #156 (adjacent to #151). if (resolved.status === 'ok') { if (!resolved.result.partial) { return new Response(JSON.stringify(resolved.result), { From 5b1b72d590e6e615366c12e7a17dbe3636ba6b7c Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 27 Aug 2026 12:13:34 +0100 Subject: [PATCH 14/22] fix(web): keep /internal off badge's flight; fix two guards that could not fire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 11 review findings, all three on code this PR introduces. 1. singleFlight collision (internal.ts). Aligning the cache key also aligned the in-isolate flight key, and singleFlight runs the FIRST registrant's loader for every joiner. badge.ts builds a byte-identical key for issue#N/pr#N with a tighter 8s/9s deadline, so a badge request landing first would hand /internal a truncated partial, which it 503s into a pinned placeholder — #143's symptom on a link its own 24s deadline answers. resolveLookup gains an optional `flightKey`; /internal passes its own. Concurrent OG unfurls still collapse into one flight. 2. `hostname.includes(':')` is dead (internal.ts). URL.hostname excludes the port, so that clause matched only a bracketed IPv6 literal. A dev origin like http://app:8787 (Codespaces/Docker/WSL) was rejected, and a rejected PUBLIC_BASE_URL is indistinguishable from an unset one — so the chain fell to PROD_HOST and keyed /internal on production while the public routes keyed on the dev host. Read URL.port instead. 3. The wrangler.toml guard passed on the config this PR fixes. It asserted `PUBLIC_BASE_URL ?? PROD_HOST` is set and routable, and [env.preview] already declared the prod PROD_HOST. Replaced with a pure checker asserting each named env has a routable cache origin OF ITS OWN, exercised against known-bad configs (the pre-fix [env.preview] included) so it cannot pass vacuously. Mutation-proved, each against the defect named above: reinstating the colon clause fails the port guard ('v9.9.9' vs 'v4.2.0' — it fell to PROD_HOST and missed the warm slot); dropping flightKey fails the flight guard (503 vs 200); restoring the old config assertion returns [] for the pre-fix [env.preview]. --- packages/web/src/resolve.ts | 15 +- packages/web/src/routes/internal.ts | 30 ++- .../web/test/internal-cache-origin.test.ts | 200 +++++++++++++++--- 3 files changed, 211 insertions(+), 34 deletions(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index 44ad0c5..fc3133a 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -134,8 +134,21 @@ export async function resolveLookup(args: { * fresh lookup, or hand back a transient the caller renders as a short-cached * placeholder, than pin a day-old answer that has since changed. */ consumerPinsResult?: boolean; + /** Override the in-isolate single-flight key, which otherwise IS `key`. + * + * `singleFlight` hands every joiner the FIRST registrant's promise and runs + * only that owner's `load`, so two callers sharing a key also share a loader — + * including its deadlines. Callers on this same cache slot do not agree on + * those: badge.ts runs an 8s/9s findRelease so a slow repo returns a + * short-cached "checking…", while the permalink and /internal callers run the + * 24s/28s defaults. Sharing a slot is deliberate (that is the whole point of a + * common key); sharing a TRUNCATION is not, so a caller whose consumer cannot + * caveat a partial passes its own flight key and always runs its own lookup. + * Concurrent calls from that same caller still collapse into one. */ + flightKey?: string; }): Promise { const { cache, key, load, bypassBackOffWhenUnservable, consumerPinsResult } = args; + const flightKey = args.flightKey ?? key; const now = args.now ?? Date.now; /** True when a cached entry must NOT be handed to a consumer that pins the @@ -244,7 +257,7 @@ export async function resolveLookup(args: { await cache.put(key, r, pinnedPartial ? HARD_TTL_PARTIAL : hardTtlFor(r)); return r; }; - const result = await singleFlight(key, run); + const result = await singleFlight(flightKey, run); return { status: 'ok', result, stale: false, staleAsOf: null, cached: false }; } catch (err) { if (err instanceof NotYetReleasedError) return { status: 'not_yet', error: err }; diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 99c9575..5ccc517 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -100,10 +100,21 @@ function originOf(value: string | undefined): string | null { // wrangler.toml suite that requires every env to set PROD_HOST or // PUBLIC_BASE_URL. // - // `localhost` is the one single-label host that is real — `wrangler dev` - // serves on it, and README tells developers to put it in PUBLIC_BASE_URL. - const { hostname } = new URL(origin); - const routable = hostname.includes('.') || hostname === 'localhost' || hostname.includes(':'); + // Single-label hosts are the ones to reject, with two real exceptions. + // `localhost` is one — `wrangler dev` serves on it and README tells + // developers to put it in PUBLIC_BASE_URL. An explicit PORT is the other: a + // dev on Codespaces / Docker / WSL reaches the Worker at a service name like + // `http://app:8787`, which is a host this Worker really does serve. Note the + // port is NOT in `hostname` (it lives in `URL.port`; `hostname` holds a colon + // only for a bracketed IPv6 literal), so it has to be read separately. + // + // Rejecting one of those would not be a harmless over-strictness: a REJECTED + // PUBLIC_BASE_URL is indistinguishable from an unset one, so the ?? chain + // falls through to PROD_HOST and every /internal entry keys on the production + // origin while the public routes key on the dev one — the two-namespace split + // of #143, reached silently despite the var being set correctly. + const { hostname, port } = new URL(origin); + const routable = hostname.includes('.') || hostname === 'localhost' || port !== ''; return routable ? origin : null; } catch { return null; @@ -226,6 +237,17 @@ async function resolveResult(c: Context, input: LookupInput): Promise // 503 into a short-cached placeholder. web-og long-caching a partial that // THIS route's own lookup produced is the remaining half, tracked in #151. consumerPinsResult: true, + // Share the SLOT with badge.ts and the permalink pages, but not the in-isolate + // FLIGHT. Aligning the cache key also aligned the single-flight key, and + // `singleFlight` runs the first registrant's loader for every joiner: a badge + // request that registers first (badge.ts:110 builds a byte-identical key for + // `issue#N`/`pr#N`) would hand this route its own 8-second-deadline result. On + // a repo that needs longer than that, this caller would inherit a `partial` and + // 503 into a pinned placeholder — #143's symptom, on a link where this route's + // own 24s deadline finds the answer. On main that could not happen, because + // /internal keyed on a three-part key of its own. Concurrent OG unfurls of the + // same key still collapse into one flight; only the cross-caller join is cut. + flightKey: `${k}:og`, }); // Refusing to SERVE a cached partial (consumerPinsResult, above) is only half // the guardrail: on a repo that reliably blows findRelease's soft deadline the diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index a0d381e..63f3be5 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -56,6 +56,9 @@ let cacheFault: 'none' | 'match' | 'put' = 'none'; }; const { default: app } = await import('../src/index.js'); +// The real in-isolate flight registry the routes use — a test can register a +// flight the way a concurrent badge request would, and see who runs the loader. +const { singleFlight } = await import('../src/single-flight.js'); const INTERNAL_SECRET = 'test-shared-secret'; const PROD_HOST = 'released.blabberate.com'; @@ -471,41 +474,127 @@ describe('preview keys the result cache on an origin it actually serves', () => // The other half of the same failure, and the one that has no fix at runtime: // `cacheOrigin` falls back to the REQUEST origin, which for a real Service // Binding is web-og's hardcoded `https://web` — a non-routable single-label host -// the Cache API declines, i.e. #143 exactly, swallowed by neverFatal. PROD_HOST -// is optional in `Env` and documents itself as the analytics gate ("Unset => -// record everything"), so dropping it from a [vars] block — or adding a named env -// that re-declares vars without it, which [env.preview] already had to do — -// silently reverts this whole fix with no signal. The config is the only place -// that can be guarded, so guard it there. -describe('every deployed environment configures a routable cache origin', () => { +// the Cache API declines, i.e. #143 exactly, swallowed by neverFatal. The config +// is the only place that can be guarded, so guard it there. +// +// Guard the invariant that actually broke, not a weaker one that survives it. +// `[env.preview]` DID declare a var — it copied the production `PROD_HOST`, which +// every env pins because it gates analytics — and keyed /internal on the prod +// origin while serving `released-web-preview.*.workers.dev`. So "PROD_HOST or +// PUBLIC_BASE_URL is set and routable" goes GREEN on the exact config this PR is +// fixing. What a named env must have is a cache origin OF ITS OWN. +type WranglerCfg = { + vars?: Record; + env?: Record }>; +}; + +/** The invariant as a pure function, so it can be run against configs that are + * KNOWN BAD as well as the committed one. A checker that has only ever seen the + * good config demonstrates nothing about what it rejects. Returns one string per + * violation; `[]` means the config cannot reintroduce #143's namespace split. */ +function cacheOriginProblems(cfg: WranglerCfg): string[] { + const problems: string[] = []; + const hostOf = (value: string): string | null => { + try { + return new URL(value.includes('//') ? value : `https://${value}`).host; + } catch { + return null; + } + }; + // `host` keeps the port, which is what makes a single-label dev origin real. + const routable = (host: string): boolean => host.includes('.') || host.startsWith('localhost'); + + const topConfigured = cfg.vars?.PUBLIC_BASE_URL ?? cfg.vars?.PROD_HOST; + const prodHost = topConfigured ? hostOf(topConfigured) : null; + if (!topConfigured) { + problems.push('[vars] sets neither PROD_HOST nor PUBLIC_BASE_URL'); + } else if (!prodHost || !routable(prodHost)) { + problems.push(`[vars] cache origin \`${topConfigured}\` is not routable`); + } + + for (const [name, e] of Object.entries(cfg.env ?? {})) { + const url = e.vars?.PUBLIC_BASE_URL; + if (!url) { + problems.push( + `[env.${name}.vars] does not set its own PUBLIC_BASE_URL — /internal falls back to PROD_HOST and keys the result cache on an origin this Worker does not serve`, + ); + continue; + } + const host = hostOf(url); + if (!host || !routable(host)) { + problems.push(`[env.${name}.vars] PUBLIC_BASE_URL \`${url}\` is not routable`); + continue; + } + if (host === prodHost) { + problems.push(`[env.${name}.vars] PUBLIC_BASE_URL keys on the PRODUCTION origin \`${host}\``); + } + } + return problems; +} + +describe('every deployed environment configures a routable cache origin of its OWN', () => { const cfg = parseToml( readFileSync(fileURLToPath(new URL('../wrangler.toml', import.meta.url)), 'utf8'), - ) as { - vars?: Record; - env?: Record }>; - }; + ) as WranglerCfg; - const environments: [string, Record | undefined][] = [ - ['[vars]', cfg.vars], - ...Object.entries(cfg.env ?? {}).map( - ([name, e]) => [`[env.${name}.vars]`, e.vars] as [string, Record | undefined], - ), - ]; + it('holds for the committed wrangler.toml', () => { + expect(cacheOriginProblems(cfg)).toEqual([]); + }); - it('declares at least one env to check, so this suite cannot pass vacuously', () => { - expect(environments.length).toBeGreaterThanOrEqual(2); + it('runs against the real named environments, so it cannot pass vacuously', () => { + expect(Object.keys(cfg.env ?? {})).toContain('preview'); }); - it.each(environments)('%s sets PROD_HOST or PUBLIC_BASE_URL', (_label, vars) => { - const configured = vars?.PUBLIC_BASE_URL ?? vars?.PROD_HOST; - expect( - configured, - 'without one of these /internal keys the result cache on the Service Binding origin `https://web`, which the Cache API drops (#143)', - ).toBeTypeOf('string'); - const host = new URL( - (configured as string).includes('//') ? (configured as string) : `https://${configured}`, - ).hostname; - expect(host, 'a single-label host is not routable and is not cacheable').toContain('.'); + it('rejects the pre-fix [env.preview] — the config that actually shipped #143', () => { + const preFix: WranglerCfg = { + vars: { PROD_HOST }, + // Exactly what was committed before this PR: the prod PROD_HOST copied in + // (it gates analytics, so every env pins it) and no origin of its own. + env: { preview: { name: 'released-web-preview', vars: { ANUBIS_HOSTS: '', PROD_HOST } } }, + }; + expect(cacheOriginProblems(preFix)).toEqual([ + expect.stringContaining('[env.preview.vars] does not set its own PUBLIC_BASE_URL'), + ]); + }); + + it('rejects a NEW env that copies PROD_HOST the way [env.preview] once did', () => { + const withStaging: WranglerCfg = { + ...cfg, + env: { + ...cfg.env, + staging: { name: 'released-web-staging', vars: { ANUBIS_HOSTS: '', PROD_HOST } }, + }, + }; + expect(cacheOriginProblems(withStaging)).toEqual([ + expect.stringContaining('[env.staging.vars] does not set its own PUBLIC_BASE_URL'), + ]); + }); + + it('rejects an env whose PUBLIC_BASE_URL IS the production origin', () => { + const aliased: WranglerCfg = { + ...cfg, + env: { + ...cfg.env, + staging: { + name: 'released-web-staging', + vars: { PUBLIC_BASE_URL: PUBLIC_ORIGIN }, + }, + }, + }; + expect(cacheOriginProblems(aliased)).toEqual([ + expect.stringContaining('keys on the PRODUCTION origin'), + ]); + }); + + it('rejects an env whose PUBLIC_BASE_URL is a non-routable single-label host', () => { + const bound: WranglerCfg = { + ...cfg, + env: { + ...cfg.env, + staging: { name: 'released-web-staging', vars: { PUBLIC_BASE_URL: 'https://web' } }, + }, + }; + expect(cacheOriginProblems(bound)).toEqual([expect.stringContaining('is not routable')]); }); }); @@ -542,6 +631,30 @@ describe('/internal/* refuses a configured cache origin that is not routable', ( expect(findReleaseMock).not.toHaveBeenCalled(); }); + it('accepts a single-label host WITH a port, which Docker/Codespaces dev serves on', async () => { + // `URL.hostname` never carries the port (that is `URL.port`), so a check for a + // colon in the hostname matches only a bracketed IPv6 literal — it never sees + // `app:8787`. Rejecting this origin is not harmless over-strictness: a rejected + // PUBLIC_BASE_URL is indistinguishable from an unset one, so the ?? chain lands + // on PROD_HOST and /internal keys on the production origin while the public + // routes key on the dev one. That is #143's split, reached with the var set. + const k = await publicKey('github.com/honojs/hono', `sha:${SHA}`); + seed('http://app:8787', k, fixture('v4.2.0')); + // Only reachable if the origin was rejected and the chain fell to PROD_HOST. + findReleaseMock.mockResolvedValue(fixture('v9.9.9')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), { + INTERNAL_SECRET, + PROD_HOST, + PUBLIC_BASE_URL: 'http://app:8787', + }); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.2.0'); + expect(findReleaseMock).not.toHaveBeenCalled(); + expect(cacheStore.has(keyUrl(PUBLIC_ORIGIN, k))).toBe(false); + }); + it('documents the unguardable case: unset vars key on the Service Binding origin', async () => { // Nothing at runtime can recover the public origin here, so this pins what // actually happens rather than implying it is safe: the entry is written under @@ -849,3 +962,32 @@ describe('/internal/* does not let a shared back-off marker cause a permanent pl expect(findReleaseMock).not.toHaveBeenCalled(); }); }); + +// Aligning the cache key also aligned the in-isolate SINGLE-FLIGHT key, which is a +// sharing this route did not have on main (it keyed on a three-part key of its +// own). `singleFlight` hands every joiner the FIRST registrant's promise and runs +// only that owner's loader, and badge.ts builds a byte-identical key for +// `issue#N`/`pr#N` (badge.ts:110) with a deliberately tighter 8s/9s deadline. So a +// badge request landing ~1s earlier in the same isolate would hand this route a +// truncated `partial`, which it 503s into a pinned neutral placeholder — #143's +// symptom, on a link where this route's own 24s deadline finds the real answer. +describe('/internal/* runs its own lookup instead of joining the badge flight', () => { + it('does not inherit an 8-second-deadline partial registered by badge.ts', async () => { + // The key badge.ts registers its flight under for `/badge/.../issue/11.svg`. + const k = await publicKey('github.com/honojs/hono', 'issue#11'); + let finishBadge: (value: unknown) => void = () => {}; + const badgeFlight = singleFlight(k, () => new Promise((r) => (finishBadge = r))); + + findReleaseMock.mockResolvedValue(fixture('v4.13.0')); + const pending = app.fetch(svc('https://web/internal/issue/honojs/hono/11'), ENV); + await settle(); + // Badge's tighter deadline ran out: a truncated traversal, all it can offer. + finishBadge(partialFixture()); + await badgeFlight; + + const res = await pending; + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.13.0'); + expect(findReleaseMock).toHaveBeenCalledOnce(); + }); +}); From a679bfc69edf54f647fb1c9693c0daee2649b3a7 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 27 Aug 2026 13:03:37 +0100 Subject: [PATCH 15/22] fix(web): scope the partial throttle to this caller; guard the shipped routability rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings from round 12, all on code this PR introduces. 1. The truncation still travelled through the CACHE (resolve.ts). Round 11 split /internal off badge's single-flight entry so it could not inherit badge's 8s deadline. The throttle that hands a <60s `partial` back rather than recomputing it did not ask WHOSE deadline produced it, so the same truncation arrived one hop later: camo fetches a README `badge.svg`, badge.ts's 8s lookup truncates and writes a partial to the byte-identical key, and an unfurl ten seconds later reads it as "recent", 503s it, and Slack pins the neutral placeholder — on a link this route's own 24s deadline answers. `main` could not do this, because /internal keyed on a key of its own; the key alignment in this PR is what opened it. resolveLookup now writes a companion `:pinpartial` marker ONLY for a partial a `consumerPinsResult` caller computed itself, same 60s TTL as the entry, and the throttle honours a partial only when that marker is fresh. A foreign partial is recomputed. The read costs nothing on the common paths: it happens only when there IS a prior partial to throttle. 2. `unpinnable()` no longer meant what its name and contract said. `isRecentPartial` made it return false for a partial (so a pinning caller IS handed one), and a terminal entry is exempt from the age bound — yet it was the pinning-safety helper by name, with the safety actually living in the route's own second check. Renamed to `shouldRecompute()`, which is what it decides, with the doc stating that refusing to PIN a partial is the caller's job. The `consumerPinsResult` option doc, which claimed no exit returns a prior older than MAX_STALE_PINNED, now names both exceptions. 3. The wrangler.toml config guard restated `originOf()`'s rule instead of calling it, and the two copies disagreed. The test read `URL.host` with `includes('.') || startsWith('localhost')`; the route reads `URL.hostname` + `URL.port`. So `PUBLIC_BASE_URL = "http://app:8787"` — the Docker / Codespaces shape round 11 deliberately added support for — was accepted by production and REJECTED by the suite, failing the build on a legitimate config; and `localhostx` was accepted by the suite and rejected by production. `isRoutableOrigin()` is now factored out of `originOf()`, both are exported, and the guard runs the shipped predicate. Mutation proof, both new guards: - Restoring the age-only throttle (`return entry.ageSeconds >= HARD_TTL_PARTIAL`) reddens exactly one test — "recomputes a 10s-old partial ANOTHER caller wrote" — with `expected 503 to be 200`. That is the failure in this commit's own title, not an adjacent one. - Restoring the duplicated config predicate reddens exactly the two new cases ("accepts an env on a single-label host WITH a port" → `expected [ Array(1) ] to deeply equal []`, and "rejects `localhostx`"). The four pre-existing config cases stay green either way, which is why they could not catch the drift. Full gate green: 335 web + 230 core + 45 cli + 33 web-og. Refs #143. --- packages/web/src/resolve.ts | 129 ++++++++++++------ packages/web/src/routes/internal.ts | 25 +++- .../web/test/internal-cache-origin.test.ts | 107 ++++++++++++--- packages/web/test/resolve.test.ts | 4 +- 4 files changed, 197 insertions(+), 68 deletions(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index fc3133a..0eb66b4 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -84,11 +84,21 @@ function isFresh(entry: CacheEntry): boolean { return entry.ageSeconds < FRESH_WINDOW_PENDING; } +type PinnedPartialMarker = { pinnedPartial: true }; type NegMarker = { transient: true; kind: string; status?: number; anubis?: boolean }; function negKey(key: string): string { return `${key}:neg`; } +/** Companion marker for `${key}`, written ONLY when a `consumerPinsResult` + * caller computed a partial itself. The result slot is shared across callers + * that do not agree on a soft deadline, so the entry alone cannot say whose + * truncation produced it; this marker can, and it expires on its own after + * `HARD_TTL_PARTIAL`. */ +function partialKey(key: string): string { + return `${key}:pinpartial`; +} + export type Resolved = | { status: 'ok'; @@ -120,19 +130,25 @@ export async function resolveLookup(args: { * Be honest about the reachable behaviour. The only caller that sets this also * sets `consumerPinsResult`, and `findRelease` emits just two entry shapes: * TERMINAL (fresh forever, so it returns at the fresh exit above) and PARTIAL - * (fresh for its 60s, unpinnable after). Neither can reach the stale-serve on + * (handed back inside its own 60s throttle, recomputed after). Neither can reach the stale-serve on * the back-off line below, so on that path the bypass is UNCONDITIONAL, not * cold-only. That is accepted, not overlooked: a crawler asks once, so the * alternative is a placeholder pinned long after the host recovers, and the * cost is the unthrottled load documented at the fall-through. The - * `prior && !unpinnable(prior)` stale-serve is live for the public routes, + * `prior && !shouldRecompute(prior)` stale-serve is live for the public routes, * which do not set either flag. */ bypassBackOffWhenUnservable?: boolean; /** Opt-in for callers whose consumer CACHES whatever we hand back, for longer * than we can correct (the OG crawler pins a rendered PNG for 24h). When set, - * no exit returns a prior older than `MAX_STALE_PINNED`: we would rather pay a - * fresh lookup, or hand back a transient the caller renders as a short-cached - * placeholder, than pin a day-old answer that has since changed. */ + * a PENDING prior older than `MAX_STALE_PINNED` is never returned: we would + * rather pay a fresh lookup, or hand back a transient the caller renders as a + * short-cached placeholder, than pin a day-old "not yet released" that has + * since shipped. Two shapes are deliberately outside that bound, both spelled + * out at `shouldRecompute` below: a TERMINAL answer is servable at any age, + * and a partial this caller itself computed within `HARD_TTL_PARTIAL` is + * handed back so the caller can refuse it without re-running the traversal. + * A partial is therefore still REACHABLE by a pinning caller — refusing to + * PIN one is the caller's own job (internal.ts:285), not this flag's. */ consumerPinsResult?: boolean; /** Override the in-isolate single-flight key, which otherwise IS `key`. * @@ -151,45 +167,69 @@ export async function resolveLookup(args: { const flightKey = args.flightKey ?? key; const now = args.now ?? Date.now; - /** True when a cached entry must NOT be handed to a consumer that pins the - * answer. Always false for callers that did not opt in, so the public HTML - * routes are unchanged. Three shapes, three rules: - * - * TERMINAL (`firstRelease`, no `partial`) — always servable. Which release - * first contains a commit cannot change, which is why `isFresh()` treats it as - * fresh forever and `hardTtlFor()` keeps it 30 days. `MAX_STALE_PINNED` exists - * for the opposite case, a "not yet released" prior that has since shipped; - * applying it here would discard exactly the warm entries this route joined the - * public key to reuse, paying a full findRelease per unfurl. + const prior = await cache.getEntry(key); + + /** True when the partial in the slot is one THIS caller computed less than + * `HARD_TTL_PARTIAL` ago — the only case the throttle below is allowed to + * honour. * - * PARTIAL (either shape) — never servable to a pinning consumer, but only - * worth recomputing once its own 60-second TTL is up. A partial is a truncated - * traversal: with `firstRelease: null` it means "we stopped looking", which - * web-og renders as a definite "not yet released"; WITH a `firstRelease` it - * carries the gallop hit, and the bisect that would confirm no EARLIER release - * contains the commit is what the deadline cut short (find-release.ts:288-292). - * The result card renders that caveat, web-og cannot — it long-caches the bare - * tag for 24h. So neither shape may be pinned. Inside `HARD_TTL_PARTIAL` the - * entry is still handed BACK (the caller 503s it into a short-cached neutral - * placeholder): that is what throttles a repo which reliably blows the soft - * deadline, where recomputing per unfurl would run a full traversal on the - * shared token for every crawler, forever. Past 60s we recompute instead. + * Aligning the key put `/internal` on the same slot as badge.ts and the + * permalink pages, and those callers do not agree on a soft deadline + * (badge.ts runs 8s/9s, everyone else 24s/28s). Round 11 stopped the + * truncation travelling through the in-isolate FLIGHT (`flightKey`); this + * stops it travelling through the CACHE, which is the same hole one hop + * later. Without it: a README badge on `/i/kubernetes/kubernetes/12345` + * makes camo fetch `badge.svg`, badge.ts's 8s deadline truncates and writes + * a partial to the byte-identical key, and an unfurl ten seconds later reads + * that partial as "recent", 503s it, and Slack pins the neutral placeholder + * — on a link where this route's own 24s deadline finds the answer. On + * `main` that could not happen, because /internal keyed on a key of its own. * - * PENDING (no `firstRelease`, no `partial`) — bounded by `MAX_STALE_PINNED`: - * an answer older than that may have shipped since, and a PNG already rendered - * from it cannot be invalidated. */ - const isRecentPartial = (entry: CacheEntry): boolean => - Boolean(entry.value.partial) && entry.ageSeconds < HARD_TTL_PARTIAL; + * Read once, here, and only when there IS a prior partial to throttle, so + * the common paths pay no extra cache read. `run()`'s re-read uses the same + * value: a partial that appeared in between is by definition not one this + * call throttled, and recomputing it is the safe direction. */ + let ownRecentPartial = false; + if (consumerPinsResult && prior?.value.partial) { + const mark = await cache.getEntry(partialKey(key)); + ownRecentPartial = mark?.value?.pinnedPartial === true && mark.ageSeconds < HARD_TTL_PARTIAL; + } - const unpinnable = (entry: CacheEntry): boolean => { + /** True when a cached entry must NOT be served to this caller and the lookup + * has to run again. Named for what it decides, not for pinning-safety: it is + * NOT the invariant "this entry may be pinned". A pinning caller still has to + * make its own call on what it does with a partial it is handed — see the + * refusal at internal.ts:285 — because inside the throttle window below this + * predicate deliberately returns false for one. Always false for callers that + * did not opt in, so the public HTML routes are unchanged. */ + const shouldRecompute = (entry: CacheEntry): boolean => { if (!consumerPinsResult) return false; + // TERMINAL (`firstRelease`, no `partial`) — always servable, at any age. + // Which release first contains a commit cannot change, which is why + // `isFresh()` treats it as fresh forever and `hardTtlFor()` keeps it 30 + // days. `MAX_STALE_PINNED` exists for the opposite case, a "not yet + // released" prior that has since shipped; applying it here would discard + // exactly the warm entries this route joined the public key to reuse, + // paying a full findRelease per unfurl. if (entry.value.firstRelease && !entry.value.partial) return false; - if (isRecentPartial(entry)) return false; - return entry.ageSeconds >= MAX_STALE_PINNED || Boolean(entry.value.partial); + // PARTIAL (either shape) — a truncated traversal. With `firstRelease: null` + // it means "we stopped looking", which web-og renders as a definite "not yet + // released"; WITH a `firstRelease` it carries the gallop hit, and the bisect + // that would confirm no EARLIER release contains the commit is what the + // deadline cut short (find-release.ts:288-292). The result card renders that + // caveat, web-og cannot. So the caller must refuse to PIN either shape — but + // recomputing one this caller itself produced under its OWN deadline, within + // its 60s TTL, would only reproduce it, so inside that window we hand it back + // (the caller 503s it into a short-cached placeholder) instead of running a + // full traversal on the shared token for every crawler. Someone ELSE's + // partial earns no such trust: recompute it. + if (entry.value.partial) return !ownRecentPartial; + // PENDING (no `firstRelease`, no `partial`) — bounded by `MAX_STALE_PINNED`: + // an answer older than that may have shipped since, and a PNG already + // rendered from it cannot be invalidated. + return entry.ageSeconds >= MAX_STALE_PINNED; }; - - const prior = await cache.getEntry(key); - if (prior && isFresh(prior) && !unpinnable(prior)) { + if (prior && isFresh(prior) && !shouldRecompute(prior)) { return { status: 'ok', result: prior.value, stale: false, staleAsOf: null, cached: true }; } @@ -208,7 +248,7 @@ export async function resolveLookup(args: { const backedOff = Boolean(neg?.value?.transient) && (neg?.ageSeconds ?? Number.POSITIVE_INFINITY) < NEG_TTL; if (backedOff) { - if (prior && !unpinnable(prior)) return staleHit(); + if (prior && !shouldRecompute(prior)) return staleHit(); if (!bypassBackOffWhenUnservable) { return { status: 'transient', @@ -241,7 +281,7 @@ export async function resolveLookup(args: { try { const run = async () => { const re = await cache.getEntry(key); - if (re && isFresh(re) && !unpinnable(re)) return re.value; + if (re && isFresh(re) && !shouldRecompute(re)) return re.value; const r = await load(); // A consumer that PINS what we hand back is also the most deadline-pressured // producer of partials, and `hardTtlFor()` tests `firstRelease` before @@ -249,12 +289,17 @@ export async function resolveLookup(args: { // WITH `partial: soft_deadline`) would take the TERMINAL 30-day branch and // `isFresh()` would report it fresh forever. On the shared slot that pins the // public permalink and badge to a tag the bisect never confirmed, for a month, - // with no upstream call able to correct it. `unpinnable` trusts a partial for + // with no upstream call able to correct it. `shouldRecompute` trusts a partial for // HARD_TTL_PARTIAL and no longer, so the long TTL buys this caller nothing. // (The same misclassification on the PUBLIC routes' own writes predates this // PR and is tracked in #155; their semantics are deliberately untouched here.) const pinnedPartial = Boolean(consumerPinsResult && r.partial); await cache.put(key, r, pinnedPartial ? HARD_TTL_PARTIAL : hardTtlFor(r)); + // Record WHOSE truncation this was, so the throttle above honours it only + // for this caller. Same TTL as the entry, so it can never outlive it. + if (pinnedPartial) { + await cache.put(partialKey(key), { pinnedPartial: true }, HARD_TTL_PARTIAL); + } return r; }; const result = await singleFlight(flightKey, run); @@ -270,7 +315,7 @@ export async function resolveLookup(args: { { transient: true, kind: err.kind, status: upstreamStatus, anubis }, NEG_TTL, ); - if (prior && !unpinnable(prior)) return staleHit(); + if (prior && !shouldRecompute(prior)) return staleHit(); return { status: 'transient', kind: err.kind, upstreamStatus, anubis }; } return { status: 'error', error: err }; diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 5ccc517..9f9680b 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -58,6 +58,20 @@ function cacheOrigin(env: Env, req: Request): string { return originOf(env.PUBLIC_BASE_URL) ?? originOf(env.PROD_HOST) ?? new URL(req.url).origin; } +/** The routability rule `originOf()` applies, exported so the wrangler.toml + * config guard can run the SHIPPED predicate instead of a lookalike. A + * duplicate drifts: an earlier copy of this rule in the test read `URL.host` + * and `startsWith('localhost')`, so it rejected `http://app:8787` — which + * production accepts — and accepted `localhostx`, which production rejects. + * + * Note the port is NOT part of `hostname` (it lives in `URL.port`; `hostname` + * holds a colon only for a bracketed IPv6 literal), so it has to be read + * separately. */ +export function isRoutableOrigin(origin: string): boolean { + const { hostname, port } = new URL(origin); + return hostname.includes('.') || hostname === 'localhost' || port !== ''; +} + /** Normalise a configured host or base URL to a bare origin, or null if it is * unset/unparseable. * @@ -71,7 +85,7 @@ function cacheOrigin(env: Env, req: Request): string { * The reverse slip is worse: a scheme-less PUBLIC_BASE_URL made `new Request()` * throw OUTSIDE neverFatal, turning a computed answer into a 503 → placeholder. * Parsing both through URL and taking .origin also drops any path/trailing slash. */ -function originOf(value: string | undefined): string | null { +export function originOf(value: string | undefined): string | null { if (!value) return null; try { const origin = new URL(value.includes('//') ? value : `https://${value}`).origin; @@ -113,9 +127,7 @@ function originOf(value: string | undefined): string | null { // falls through to PROD_HOST and every /internal entry keys on the production // origin while the public routes key on the dev one — the two-namespace split // of #143, reached silently despite the var being set correctly. - const { hostname, port } = new URL(origin); - const routable = hostname.includes('.') || hostname === 'localhost' || port !== ''; - return routable ? origin : null; + return isRoutableOrigin(origin) ? origin : null; } catch { return null; } @@ -270,8 +282,9 @@ async function resolveResult(c: Context, input: LookupInput): Promise // permalink it links to still shows the best-effort answer WITH its caveat. // // resolveLookup hands back a partial it computed less than HARD_TTL_PARTIAL ago - // rather than recomputing it (see `unpinnable`), so this 503 is throttled to one - // traversal per 60s per key. Do NOT read that as "the cost is bounded": web-og + // rather than recomputing it (see `shouldRecompute`), so this 503 is throttled to + // one traversal per 60s per key, and ONLY for a partial this route itself computed + // (a badge-truncated one in the shared slot is recomputed, not inherited). Do NOT read that as "the cost is bounded": web-og // short-caches the neutral placeholder at max-age=60 and HARD_TTL_PARTIAL is // also 60, so for a URL under active unfurling the two cadences COINCIDE and the // throttle buys close to nothing. On a repo that reliably blows the soft deadline diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index 63f3be5..8b078dd 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -59,6 +59,9 @@ const { default: app } = await import('../src/index.js'); // The real in-isolate flight registry the routes use — a test can register a // flight the way a concurrent badge request would, and see who runs the loader. const { singleFlight } = await import('../src/single-flight.js'); +// The SHIPPED routability rule, so the wrangler.toml guard below cannot drift +// from the route it guards. +const { originOf } = await import('../src/routes/internal.js'); const INTERNAL_SECRET = 'test-shared-secret'; const PROD_HOST = 'released.blabberate.com'; @@ -494,22 +497,21 @@ type WranglerCfg = { * violation; `[]` means the config cannot reintroduce #143's namespace split. */ function cacheOriginProblems(cfg: WranglerCfg): string[] { const problems: string[] = []; - const hostOf = (value: string): string | null => { - try { - return new URL(value.includes('//') ? value : `https://${value}`).host; - } catch { - return null; - } - }; - // `host` keeps the port, which is what makes a single-label dev origin real. - const routable = (host: string): boolean => host.includes('.') || host.startsWith('localhost'); - - const topConfigured = cfg.vars?.PUBLIC_BASE_URL ?? cfg.vars?.PROD_HOST; - const prodHost = topConfigured ? hostOf(topConfigured) : null; - if (!topConfigured) { + // Run the SHIPPED normaliser (`originOf`, exported from the route), not a + // second copy of its rule. The copy this replaced tested `URL.host` with + // `host.includes('.') || host.startsWith('localhost')`, which disagreed with + // production on two inputs: it rejected `http://app:8787` — the Docker / + // Codespaces shape the route deliberately accepts, so a legitimate config + // would have reddened the build — and accepted `localhostx`, which the route + // rejects. A guard that can pass or fail differently from the code it guards + // is not a guard. `originOf` also returns null for an unparseable value, so + // the two cases collapse into one branch here. + const prodOrigin = cfg.vars?.PUBLIC_BASE_URL ?? cfg.vars?.PROD_HOST; + const prodCacheOrigin = originOf(prodOrigin); + if (!prodOrigin) { problems.push('[vars] sets neither PROD_HOST nor PUBLIC_BASE_URL'); - } else if (!prodHost || !routable(prodHost)) { - problems.push(`[vars] cache origin \`${topConfigured}\` is not routable`); + } else if (!prodCacheOrigin) { + problems.push(`[vars] cache origin \`${prodOrigin}\` is not routable`); } for (const [name, e] of Object.entries(cfg.env ?? {})) { @@ -520,13 +522,15 @@ function cacheOriginProblems(cfg: WranglerCfg): string[] { ); continue; } - const host = hostOf(url); - if (!host || !routable(host)) { + const origin = originOf(url); + if (!origin) { problems.push(`[env.${name}.vars] PUBLIC_BASE_URL \`${url}\` is not routable`); continue; } - if (host === prodHost) { - problems.push(`[env.${name}.vars] PUBLIC_BASE_URL keys on the PRODUCTION origin \`${host}\``); + if (origin === prodCacheOrigin) { + problems.push( + `[env.${name}.vars] PUBLIC_BASE_URL keys on the PRODUCTION origin \`${origin}\``, + ); } } return problems; @@ -586,6 +590,41 @@ describe('every deployed environment configures a routable cache origin of its O ]); }); + // The reason this suite calls `originOf` instead of restating its rule. A + // single-label host WITH a port is what a dev on Docker / Codespaces / WSL + // reaches the Worker at, and the route accepts it (internal.ts's + // `isRoutableOrigin`). The duplicated predicate this replaced tested + // `host.includes('.') || host.startsWith('localhost')` on `URL.host`, so + // `app:8787` matched neither arm: production accepted the config and the + // build failed on it. Proven by restoring that predicate — this test and the + // `localhostx` one below are the two that redden (`expected [ Array(1) ] to + // deeply equal []`); the four pre-existing cases stay green either way, which + // is why they could not catch the drift. + it('accepts an env on a single-label host WITH a port, exactly as the route does', () => { + const docker: WranglerCfg = { + ...cfg, + env: { + ...cfg.env, + dev: { name: 'released-web-dev', vars: { PUBLIC_BASE_URL: 'http://app:8787' } }, + }, + }; + expect(cacheOriginProblems(docker)).toEqual([]); + }); + + // The other half of the divergence: the old predicate's `startsWith` accepted + // any host merely PREFIXED with localhost. `originOf` requires the whole + // hostname (or a port, which this has neither of). + it('rejects `localhostx`, which only a prefix match would accept', () => { + const typo: WranglerCfg = { + ...cfg, + env: { + ...cfg.env, + dev: { name: 'released-web-dev', vars: { PUBLIC_BASE_URL: 'http://localhostx' } }, + }, + }; + expect(cacheOriginProblems(typo)).toEqual([expect.stringContaining('is not routable')]); + }); + it('rejects an env whose PUBLIC_BASE_URL is a non-routable single-label host', () => { const bound: WranglerCfg = { ...cfg, @@ -704,6 +743,36 @@ describe('/internal/* refuses to pin a partial, and throttles the refusal', () = expect(await tagOfSlot(PUBLIC_ORIGIN, k)).toBe('v4.12.0'); // and the slot is corrected }); + // Round 12 (#144). Round 11 stopped badge's 8-second truncation reaching this + // route through the in-isolate FLIGHT (`flightKey: `${k}:og``). The same + // truncation still reached it one hop later, through the CACHE: the throttle + // that hands a <60s partial back rather than recomputing it did not ask WHOSE + // deadline produced it. So the README-badge case — camo fetches `badge.svg`, + // badge.ts writes a partial to the byte-identical key, Slack unfurls the + // permalink ten seconds later — read that partial as "recent", 503'd, and + // pinned the neutral placeholder, on a link this route's own 24s deadline + // answers. `main` could not do this: /internal keyed on a key of its own. + // + // The throttle now keys on a companion marker resolveLookup writes ONLY for a + // partial IT produced under this caller's deadline, so a foreign one is + // recomputed. (Proven: restoring the age-only test — `return + // entry.ageSeconds >= HARD_TTL_PARTIAL` in `shouldRecompute` — reddens this + // test alone, with `expected 503 to be 200`.) + it('recomputes a 10s-old partial ANOTHER caller wrote, rather than 503 into a pinned placeholder', async () => { + const sha = 'b'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + // badge.ts's 8s deadline truncated and wrote this; no `:pinpartial` marker, + // because badge does not set consumerPinsResult. + seedAged(PUBLIC_ORIGIN, k, partialFixture(), 10); + findReleaseMock.mockResolvedValue(fixture('v4.13.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.13.0'); + expect(findReleaseMock).toHaveBeenCalledTimes(1); + }); + it('still blocks (and write-backs) when the slot is genuinely cold', async () => { const sha = 'd'.repeat(40); findReleaseMock.mockResolvedValue(fixture('v4.15.0')); diff --git a/packages/web/test/resolve.test.ts b/packages/web/test/resolve.test.ts index 9611158..f797624 100644 --- a/packages/web/test/resolve.test.ts +++ b/packages/web/test/resolve.test.ts @@ -62,6 +62,7 @@ function makeFakeCache() { const KEY = 'res:gtk:pr#9951'; const negKey = `${KEY}:neg`; +const pinPartialKey = `${KEY}:pinpartial`; describe('isTransientError', () => { it('treats 5xx / network / timeout / rate-limit as transient', () => { @@ -371,10 +372,11 @@ describe('resolveLookup — a pinned consumer is never handed a cached PARTIAL', // resolveLookup had just written, so on a repo that reliably blows the 24s soft // deadline every unfurl ran another full traversal on the shared token, where // the flat 30-minute TTL this route replaced made zero upstream calls. - it('fresh exit: hands back a 10s-old partial rather than re-run the lookup', async () => { + it('fresh exit: hands back a 10s-old partial THIS caller wrote rather than re-run the lookup', async () => { const f = makeFakeCache(); const truncated = mkResult({ released: false, partial: true }); f.seed(KEY, truncated, 10); // well inside the 60s partial freshness window + f.seed(pinPartialKey, { pinnedPartial: true }, 10); // ...and this caller produced it const fresh = mkResult({ released: true }); const load = vi.fn().mockResolvedValue(fresh); From cb23da8347236b1dcae225791538275e4ab41e43 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 27 Aug 2026 13:51:26 +0100 Subject: [PATCH 16/22] fix(web): bind the :pinpartial marker to the entry it vouches for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The marker says "the partial in this slot is one THIS caller computed", but the throttle trusted it on its own age alone — it never checked the marker still describes the entry actually sitting there. The slot is shared with callers that run a different soft deadline, so it can be overwritten between our marker write and the next read, and the marker is then inherited by a stranger's partial: T=0 an unfurl blows the 24s soft deadline -> writes its partial + marker T=20 camo fetches badge.svg on the same key; badge's 8s deadline truncates harder and overwrites the slot (badge writes no marker of its own) T=30 the next unfurl reads badge's partial (age 10s) under our marker (age 30s), calls it ours, and 503s into a pinned neutral placeholder — on a link this route's own 24s deadline answers That is precisely the inheritance internal.ts:286 claims is prevented. run() puts the slot before the marker, so for a pair this caller produced the marker can never be OLDER than the entry. When it is, the slot moved under us — recompute. One extra clause, no extra cache read. Impact was bounded (the route 503s on any partial either way, so no wrong answer was ever pinned — only a recompute that would have succeeded got skipped, for at most 60s), which is why this rides here rather than as its own issue. Guard proven by mutation: dropping the `mark.ageSeconds <= prior.ageSeconds` clause reddens the new test alone, with `expected 503 to be 200`. Its complement pins the opposite direction — the ordinary own-partial pair (marker younger than entry) must still throttle to a 503 with no second traversal — so the clause cannot silently disable the throttle it guards. Co-Authored-By: Claude Opus 5 --- packages/web/src/resolve.ts | 12 ++++- .../web/test/internal-cache-origin.test.ts | 50 +++++++++++++++++++ 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index 0eb66b4..3f3c560 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -192,7 +192,17 @@ export async function resolveLookup(args: { let ownRecentPartial = false; if (consumerPinsResult && prior?.value.partial) { const mark = await cache.getEntry(partialKey(key)); - ownRecentPartial = mark?.value?.pinnedPartial === true && mark.ageSeconds < HARD_TTL_PARTIAL; + ownRecentPartial = + mark?.value?.pinnedPartial === true && + mark.ageSeconds < HARD_TTL_PARTIAL && + // ...and the marker has to actually IDENTIFY the entry it vouches for, not + // merely be young. `run()` writes the slot first and the marker second, so + // for a pair this caller produced the marker can never be OLDER than the + // entry. When it is, the slot was overwritten AFTER we marked it — by a + // caller on the same key with a different deadline (badge.ts, 8s) — so the + // partial sitting there is someone else's truncation wearing our marker. + // Recompute it, which is exactly what the marker exists to make possible. + mark.ageSeconds <= prior.ageSeconds; } /** True when a cached entry must NOT be served to this caller and the lookup diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index 8b078dd..d318e33 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -773,6 +773,56 @@ describe('/internal/* refuses to pin a partial, and throttles the refusal', () = expect(findReleaseMock).toHaveBeenCalledTimes(1); }); + // Round 13 (#144). The marker above says "a partial THIS caller wrote is in the + // slot", but the previous round trusted it on its own age alone — it never + // checked that the marker still describes the entry actually sitting there. The + // slot is shared, so it can be overwritten between our marker write and the next + // read, and the marker is then inherited by a stranger's partial: + // + // T=0 an unfurl blows the 24s soft deadline → writes its partial + marker + // T=20 camo fetches badge.svg on the same key; badge's 8s deadline truncates + // harder and OVERWRITES the slot (it writes no marker of its own) + // T=30 the next unfurl reads badge's partial (age 10s) under OUR marker + // (age 30s), calls it ours, and 503s into a pinned placeholder — on a + // link this route's own 24s deadline answers. + // + // `run()` puts the slot before the marker, so our marker can never be OLDER than + // our entry; when it is, the slot moved under us. (Proven: dropping the + // `mark.ageSeconds <= prior.ageSeconds` clause reddens this test alone, with + // `expected 503 to be 200`.) + it('recomputes when the slot was overwritten AFTER our marker, rather than inheriting the marker', async () => { + const sha = '8'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + // Ours, written at T=0 and still inside its 60s TTL... + seedAged(PUBLIC_ORIGIN, `${k}:pinpartial`, { pinnedPartial: true }, 30); + // ...but the partial in the slot is badge.ts's, written at T=20 — NEWER than + // the marker, so the marker cannot be describing it. + seedAged(PUBLIC_ORIGIN, k, partialFixture(), 10); + findReleaseMock.mockResolvedValue(fixture('v4.14.0')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.14.0'); + expect(findReleaseMock).toHaveBeenCalledTimes(1); + }); + + // The complement, and what keeps the clause above from silently disabling the + // throttle altogether: when the marker IS younger than the entry — the ordinary + // own-partial pair `run()` writes — the throttle still engages and the route + // still 503s without a second traversal. + it('still honours the marker for a pair this caller wrote (marker younger than entry)', async () => { + const sha = '9'.repeat(40); + const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); + seedAged(PUBLIC_ORIGIN, k, partialFixture(), 30); + seedAged(PUBLIC_ORIGIN, `${k}:pinpartial`, { pinnedPartial: true }, 29); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${sha}`), ENV); + + expect(res.status).toBe(503); + expect(findReleaseMock).not.toHaveBeenCalled(); + }); + it('still blocks (and write-backs) when the slot is genuinely cold', async () => { const sha = 'd'.repeat(40); findReleaseMock.mockResolvedValue(fixture('v4.15.0')); From a90f5389046f4fb3e8e2b0dbcad997c26482d80b Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 27 Aug 2026 18:23:42 +0100 Subject: [PATCH 17/22] fix(web): order the :pinpartial marker by stamp, not by two read-time ages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 14 review of #144, three findings. 1. `mark.ageSeconds <= prior.ageSeconds` (round 13) compares two ages floored from two DIFFERENT `Date.now()` samples, one Cache API round trip apart, so the floors can straddle a second boundary and report the marker as OLDER than an entry it was in fact written AFTER. The caller then disowns its own seconds-old partial and runs a full traversal — on exactly the deadline- blowing repos the throttle exists for. `getEntry` now reports the stored `x-cached-at` stamp and the ordering test uses it, falling back to ages when a stamp is missing (pre-header entry: unprovable order still recomputes). 2. `consumerPinsResult`'s JSDoc led with the `MAX_STALE_PINNED` bound, which is vacuous: `findRelease` cannot emit a PENDING entry (no `firstRelease`, no `partial`) — a real "not yet released" is thrown and never cached. Reworded to say what the flag actually does (the partial handling) and to mark the PENDING arm as defensive, so the next change does not assume the OG path is bounded by it. The named failure is #151/#150, not this. 3. `PUBLIC_BASE_URL` also repoints `publicBaseUrl()`, so a rename of `[env.preview] name` silently emits canonical/`og:url` for a host that does not exist. Noted the coupling in wrangler.toml. Mutation-proven, both directions: - revert (1) to `mark.ageSeconds <= prior.ageSeconds` → "honours its own marker when the two READ ages straddle a second boundary" FAILS ("expected vi.fn() to not be called at all, but actually been called 1 times"). - make the ordering test vacuous (`true`) → "still disowns a partial written AFTER the marker (badge.ts overwrote the slot)" FAILS ("expected vi.fn() to be called 1 times, but got 0 times"), plus internal.ts 503→200. pnpm test green: web 339 passed, web-og 33, cli 45, core unchanged. --- packages/web/src/cache.ts | 19 ++++++--- packages/web/src/resolve.ts | 67 ++++++++++++++++++++++++------ packages/web/test/cache.test.ts | 7 ++++ packages/web/test/resolve.test.ts | 69 ++++++++++++++++++++++++++++--- packages/web/wrangler.toml | 9 ++++ 5 files changed, 148 insertions(+), 23 deletions(-) diff --git a/packages/web/src/cache.ts b/packages/web/src/cache.ts index ca7def8..8376f20 100644 --- a/packages/web/src/cache.ts +++ b/packages/web/src/cache.ts @@ -15,8 +15,16 @@ import type { CacheStore } from '@released/core'; -/** A cached value plus how long ago it was written (seconds). */ -export type CacheEntry = { value: T; ageSeconds: number }; +/** A cached value, how long ago it was written (seconds), and the raw + * `x-cached-at` wall-clock stamp it was derived from (null for an entry + * written before this header existed). + * + * `ageSeconds` is floored from a `Date.now()` sample taken at READ time, so two + * entries read one Cache API round trip apart carry ages sampled at two + * different instants — comparing them can invert the true write order when the + * floors straddle a second boundary. `stampedAt` is the write-time instant + * itself, so an ordering test between two entries is exact. */ +export type CacheEntry = { value: T; ageSeconds: number; stampedAt: number | null }; export type WorkerCache = CacheStore & { /** Like get(), but also reports the entry's age so callers can judge staleness. */ @@ -53,11 +61,10 @@ export function makeWorkerCache(req: Request, ttlSecondsDefault = 1800): WorkerC return null; } const stamped = Number(res.headers.get('x-cached-at')); + const stampedAt = Number.isFinite(stamped) && stamped > 0 ? stamped : null; const ageSeconds = - Number.isFinite(stamped) && stamped > 0 - ? Math.max(0, Math.floor((Date.now() - stamped) / 1000)) - : 0; - return { value, ageSeconds }; + stampedAt === null ? 0 : Math.max(0, Math.floor((Date.now() - stampedAt) / 1000)); + return { value, ageSeconds, stampedAt }; }, async put(key: string, value: T, ttlSeconds?: number): Promise { diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index 3f3c560..e057d21 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -99,6 +99,30 @@ function partialKey(key: string): string { return `${key}:pinpartial`; } +/** True when `mark` was written no earlier than `entry`. + * + * Compare the STORED `x-cached-at` stamps, not the two `ageSeconds` values. + * `cache.getEntry` floors each age from a `Date.now()` sample taken when THAT + * entry was read, and the two reads are one Cache API round trip apart, so the + * floors can straddle a second boundary and invert the ordering for a pair that + * really was written entry-then-marker: + * + * entry stamped t=1000ms, read at 2990ms -> floor(1990/1000) = 1 + * marker stamped t=1010ms, read at 3015ms -> floor(2005/1000) = 2 + * + * `2 <= 1` is false, so the caller would disown its own 2-second-old partial and + * run another full traversal — precisely the traversal this throttle exists to + * avoid, on the deadline-blowing repos it exists for. The write-time stamps say + * 1010 >= 1000, which is the quantity the guard actually reasons about. + * + * Falls back to the age comparison when either stamp is missing (an entry written + * by a version that predates the `x-cached-at` header): same conservative + * direction as before — an unprovable ordering recomputes. */ +function writtenNoEarlierThan(mark: CacheEntry, entry: CacheEntry): boolean { + if (mark.stampedAt !== null && entry.stampedAt !== null) return mark.stampedAt >= entry.stampedAt; + return mark.ageSeconds <= entry.ageSeconds; +} + export type Resolved = | { status: 'ok'; @@ -139,16 +163,30 @@ export async function resolveLookup(args: { * which do not set either flag. */ bypassBackOffWhenUnservable?: boolean; /** Opt-in for callers whose consumer CACHES whatever we hand back, for longer - * than we can correct (the OG crawler pins a rendered PNG for 24h). When set, - * a PENDING prior older than `MAX_STALE_PINNED` is never returned: we would - * rather pay a fresh lookup, or hand back a transient the caller renders as a - * short-cached placeholder, than pin a day-old "not yet released" that has - * since shipped. Two shapes are deliberately outside that bound, both spelled - * out at `shouldRecompute` below: a TERMINAL answer is servable at any age, - * and a partial this caller itself computed within `HARD_TTL_PARTIAL` is - * handed back so the caller can refuse it without re-running the traversal. - * A partial is therefore still REACHABLE by a pinning caller — refusing to - * PIN one is the caller's own job (internal.ts:285), not this flag's. */ + * than we can correct (the OG crawler pins a rendered PNG for 24h). + * + * What it does TODAY is entirely the PARTIAL handling at `shouldRecompute` + * below: someone else's truncated traversal is recomputed rather than served, + * and one this caller itself produced within `HARD_TTL_PARTIAL` is handed back + * so the caller can refuse it (internal.ts:285) without paying another + * traversal. A TERMINAL answer is servable at any age and is deliberately + * outside every bound. Refusing to PIN a partial is the caller's own job, not + * this flag's. + * + * It ALSO bounds a PENDING prior (no `firstRelease`, no `partial`) at + * `MAX_STALE_PINNED`, so a day-old "not yet released" that has since shipped is + * never handed to a pinning consumer. Be honest about that arm: `findRelease` + * cannot currently emit that shape — its three value returns are the gallop hit + * (`find-release.ts:299`, always `partial`), the soft-deadline miss (`:316`, + * always `partial`) and the terminal answer (`:391`); a genuine "not yet + * released" is THROWN (`:326`) and `resolve.ts` returns it as `status: + * 'not_yet'` without ever calling `cache.put`. So no PENDING entry exists to + * bound, and the arm is a defensive guard on a shape the TYPE permits (core + * keeps a matching fallback at `:486`), not a live protection. The failure it + * is named for is real but is fixed elsewhere: the 24h pin of a fresh answer is + * #151, and the `not_yet` 503 is #150. Do not read this bound as covering the + * OG path. (`HARD_TTL_PENDING` and `FRESH_WINDOW_PENDING` are dead for the same + * reason, and predate this PR.) */ consumerPinsResult?: boolean; /** Override the in-isolate single-flight key, which otherwise IS `key`. * @@ -202,7 +240,7 @@ export async function resolveLookup(args: { // caller on the same key with a different deadline (badge.ts, 8s) — so the // partial sitting there is someone else's truncation wearing our marker. // Recompute it, which is exactly what the marker exists to make possible. - mark.ageSeconds <= prior.ageSeconds; + writtenNoEarlierThan(mark, prior); } /** True when a cached entry must NOT be served to this caller and the lookup @@ -236,7 +274,12 @@ export async function resolveLookup(args: { if (entry.value.partial) return !ownRecentPartial; // PENDING (no `firstRelease`, no `partial`) — bounded by `MAX_STALE_PINNED`: // an answer older than that may have shipped since, and a PNG already - // rendered from it cannot be invalidated. + // rendered from it cannot be invalidated. DEFENSIVE ONLY: `findRelease` emits + // no such entry today (a real "not yet released" is thrown, never cached — see + // the `consumerPinsResult` doc above), so this line does not fire in + // production. It stays because the TYPE permits the shape and core keeps a + // fallback branch that would return it (`find-release.ts:486`); it must not be + // read as the bound that protects the OG path. return entry.ageSeconds >= MAX_STALE_PINNED; }; if (prior && isFresh(prior) && !shouldRecompute(prior)) { diff --git a/packages/web/test/cache.test.ts b/packages/web/test/cache.test.ts index e417aa4..0d3691c 100644 --- a/packages/web/test/cache.test.ts +++ b/packages/web/test/cache.test.ts @@ -109,6 +109,10 @@ describe('makeWorkerCache', () => { expect(entry?.value).toEqual({ x: 7 }); expect(entry?.ageSeconds).toBeGreaterThanOrEqual(41); expect(entry?.ageSeconds).toBeLessThanOrEqual(44); + // The raw write-time stamp is reported too: `ageSeconds` is floored from a + // read-time `Date.now()`, so an ordering test between two entries read at + // different instants needs the stamp itself (resolve.ts writtenNoEarlierThan). + expect(entry?.stampedAt).toBe(cachedAt); }); it('getEntry returns null when there is no cached entry', async () => { @@ -127,5 +131,8 @@ describe('makeWorkerCache', () => { const cache = makeWorkerCache(new Request('https://released-web.lukaso.workers.dev/')); const entry = await cache.getEntry('k'); expect(entry?.ageSeconds).toBe(0); + // ...and reports the stamp as absent rather than inventing one, so an + // ordering test falls back to ages instead of comparing against a fake 0. + expect(entry?.stampedAt).toBeNull(); }); }); diff --git a/packages/web/test/resolve.test.ts b/packages/web/test/resolve.test.ts index f797624..a2a1627 100644 --- a/packages/web/test/resolve.test.ts +++ b/packages/web/test/resolve.test.ts @@ -37,23 +37,28 @@ function mkResult(opts: { released: boolean; partial?: boolean }): LookupResult /** In-memory WorkerCache whose entry ages are set explicitly by the test. */ function makeFakeCache() { - const store = new Map(); + const store = new Map(); const cache: WorkerCache = { async get(key: string) { return (store.get(key)?.value as T) ?? null; }, async getEntry(key: string): Promise | null> { const e = store.get(key); - return e ? { value: e.value as T, ageSeconds: e.ageSeconds } : null; + return e ? { value: e.value as T, ageSeconds: e.ageSeconds, stampedAt: e.stampedAt } : null; }, async put(key: string, value: T) { - store.set(key, { value, ageSeconds: 0 }); + store.set(key, { value, ageSeconds: 0, stampedAt: Date.now() }); }, }; return { cache, - seed(key: string, value: unknown, ageSeconds: number) { - store.set(key, { value, ageSeconds }); + /** `stampedAt` is the write-time `x-cached-at` the real cache stores. It is + * INDEPENDENT of `ageSeconds` here on purpose: production derives the two + * from different `Date.now()` samples, so a test has to be able to express a + * pair whose ages disagree with their true write order. Default it to a + * stamp consistent with the age, so existing tests are unaffected. */ + seed(key: string, value: unknown, ageSeconds: number, stampedAt?: number) { + store.set(key, { value, ageSeconds, stampedAt: stampedAt ?? Date.now() - ageSeconds * 1000 }); }, has: (key: string) => store.has(key), get: (key: string) => store.get(key), @@ -405,6 +410,60 @@ describe('resolveLookup — a pinned consumer is never handed a cached PARTIAL', // no revalidation. (The terminal misclassification itself is on `main` and // affects the public routes too; the pin bound rejecting it on the way out is // what this PR owes.) + // Round 14 review of #144. Round 13 bound the marker to the entry it vouches + // for with `mark.ageSeconds <= prior.ageSeconds` — but those two ages are + // floored from two DIFFERENT `Date.now()` samples, one Cache API round trip + // apart, so the floors can straddle a second boundary and report the marker as + // OLDER than an entry it was in fact written AFTER. The guard then disowns this + // caller's own seconds-old partial and runs a full traversal — on exactly the + // repos the throttle exists for. The stored `x-cached-at` stamps are the + // quantity the ordering actually depends on. + it('honours its own marker when the two READ ages straddle a second boundary', async () => { + const f = makeFakeCache(); + const truncated = mkResult({ released: false, partial: true }); + // The reviewer's arithmetic, seeded directly: entry written at t=1000 and + // read back as 1s old; marker written 10ms LATER at t=1010 and read back as + // 2s old, because its read happened 25ms further on. + f.seed(KEY, truncated, 1, 1000); + f.seed(pinPartialKey, { pinnedPartial: true }, 2, 1010); + const load = vi.fn().mockResolvedValue(mkResult({ released: true })); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + }); + + expect(load).not.toHaveBeenCalled(); + expect(r.status).toBe('ok'); + if (r.status === 'ok') expect(r.result.partial).toBeTruthy(); + }); + + // ...and the ordering test must still REJECT a slot genuinely overwritten after + // the marker was written, or the fix above is just a way of deleting the guard. + it('still disowns a partial written AFTER the marker (badge.ts overwrote the slot)', async () => { + const f = makeFakeCache(); + const someoneElses = mkResult({ released: false, partial: true }); + // Marker at t=1000; the slot then overwritten at t=5000 by an 8s-deadline + // caller on the same key. Ages alone would say the marker is the younger one. + f.seed(KEY, someoneElses, 1, 5000); + f.seed(pinPartialKey, { pinnedPartial: true }, 5, 1000); + const fresh = mkResult({ released: true }); + const load = vi.fn().mockResolvedValue(fresh); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + }); + + expect(load).toHaveBeenCalledTimes(1); + expect(r.status).toBe('ok'); + if (r.status === 'ok') expect(r.result.firstRelease?.tag).toBe('4.18.0'); + }); + it('a gallop-only partial is not treated as terminal, however old', async () => { const f = makeFakeCache(); const gallopOnly = { ...mkResult({ released: true }), partial: { reason: 'soft_deadline' } }; diff --git a/packages/web/wrangler.toml b/packages/web/wrangler.toml index 5a4f257..fc09660 100644 --- a/packages/web/wrangler.toml +++ b/packages/web/wrangler.toml @@ -135,6 +135,15 @@ PROD_HOST = "released.blabberate.com" # (test/internal-cache-origin.test.ts) and only takes effect on the prod custom # domain. The var is still correct to set: it keeps preview off the prod key # namespace and drives canonical URLs. +# +# COUPLING: this var has a SECOND effect. `publicBaseUrl()` (src/env.ts) prefers +# it over the incoming request origin, so preview's canonical links, `og:url` and +# every `pubBase`-derived href come from this literal rather than being correct by +# construction. Rename `[env.preview] name` below, or deploy under a different +# account subdomain, and preview emits URLs for a host that does not exist — the +# config guard (`cacheOriginProblems`) checks only that the var is set, routable +# and not the production origin, never that it matches the deployed Worker. Update +# this line whenever that name changes. PUBLIC_BASE_URL = "https://released-web-preview.lukaso.workers.dev" [env.preview.assets] From ea541fed183a7da8bf2c38d6cd36967b4b175293 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 27 Aug 2026 19:01:08 +0100 Subject: [PATCH 18/22] fix(web): keep the shared slot's TTL and back-off clock caller-independent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 15 review on #144. Three findings on the diff itself; one declined. 1. resolve.ts — a pinning caller wrote HARD_TTL_PARTIAL onto the SHARED entry. Since this PR aligned /internal onto the public key, that 60s imposed itself on badge.ts and the permalink pages: one OG unfurl replaced the 30-day entry a page view had just written, so 61s later every human page view paid a fresh traversal and issue.tsx/pr.tsx's bot branch rendered the deferred card off a slot that had been warm. The entry now always carries hardTtlFor(); this caller's 60s distrust of a partial rides on its own `:pinpartial` marker, which is what shouldRecompute already read. Public semantics are now actually untouched, as the comment claimed. (The 30-day pin of a gallop partial is real and stays #155's to fix, on the public routes' own writes.) 2. resolve.ts — a bypassed load that failed RE-STAMPED the shared `:neg` marker. Under a once-a-minute unfurl cadence the marker never reached NEG_TTL, so a human on the same key sat on "checking..." for a whole outage and never got the per-minute retry the back-off exists to give them. The marker is now written only when it was cold; the bypass path leaves its age alone. 3. wrangler.toml / config guard — PUBLIC_BASE_URL also drives publicBaseUrl(), so preview's canonical/og:url/sitemap became "correct only while this literal matches". cacheOriginProblems() now asserts, for every named env on a workers.dev host, that the first label equals `[env.] name` — a rename fails the build instead of shipping URLs for a host that does not exist. The ACCOUNT subdomain is not in the file at all, so it stays a comment. Declined: the 503-on-partial regression for deadline-heavy repos (#156). It is deliberate, the merge order #144 -> #158 -> #156 is written on #156, and the remedy is web-og short-caching the gallop shape, which is #158's change. Mutation-proven, each against the defect in its own claim: - restore `pinnedPartial ? HARD_TTL_PARTIAL : hardTtlFor(r)` -> 3 tests redden ("...caller-independent TTL", "...IDENTICAL entry TTL", "...via its own marker"). - drop `if (!backedOff)` -> "does NOT re-stamp a negative marker it bypassed" reddens; the cold-slot companion test stays green, so the back-off still exists. - neuter the name check -> "left behind by a `name` rename" reddens. - drop the `.workers.dev` condition -> the custom-domain and `app:8787` tests redden, so the guard is scoped, not merely present. pnpm test 352 web / 45 cli / 33 web-og green; typecheck clean. --- packages/web/src/resolve.ts | 76 +++++++---- packages/web/src/routes/internal.ts | 17 ++- .../web/test/internal-cache-origin.test.ts | 100 ++++++++++++-- packages/web/test/resolve.test.ts | 126 +++++++++++++++++- packages/web/wrangler.toml | 11 +- 5 files changed, 279 insertions(+), 51 deletions(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index e057d21..1ca1e2e 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -321,14 +321,17 @@ export async function resolveLookup(args: { // recovers. Gating the bypass on a fraction of NEG_TTL would only move which // unfurls get the permanent placeholder, not stop them. // - // What that argument does NOT cover, and what #157 tracks: every bypassed load - // RE-STAMPS the marker, so under a steady crawler cadence the marker is almost - // never older than NEG_TTL. Human page views on this key do not bypass, so they - // hit a warm marker on nearly every request and sit on the "checking..." card - // for the whole outage instead of getting a retry window each minute. The - // crawler's unthrottled probing starves the humans' back-off of its recovery - // window; fixing that means changing the marker's semantics (a `bypassed` flag, - // or not re-stamping on a bypassed load), not the bypass condition. + // What it must NOT also cost is the humans' recovery window. A bypassed load + // that fails does NOT re-stamp the marker (see the catch below): re-stamping + // would reset the age that the callers who DO honour the marker — the public + // HTML routes, on this same shared key — read, so under a steady crawler + // cadence (a link circulating on Slack unfurls roughly once a minute) the + // marker would never reach NEG_TTL and a human would sit on the "checking..." + // card for the whole outage rather than getting a fresh attempt each minute. + // The marker now ages out on its own clock, as it does on `main`, where + // /internal kept a negative marker of its own. #157 tracks the wider question + // of what the unthrottled bypass should cost; this is the half the shared key + // introduced. } try { @@ -336,20 +339,32 @@ export async function resolveLookup(args: { const re = await cache.getEntry(key); if (re && isFresh(re) && !shouldRecompute(re)) return re.value; const r = await load(); - // A consumer that PINS what we hand back is also the most deadline-pressured - // producer of partials, and `hardTtlFor()` tests `firstRelease` before - // `partial` — so a gallop-only partial (find-release.ts ~295: the gallop hit - // WITH `partial: soft_deadline`) would take the TERMINAL 30-day branch and - // `isFresh()` would report it fresh forever. On the shared slot that pins the - // public permalink and badge to a tag the bisect never confirmed, for a month, - // with no upstream call able to correct it. `shouldRecompute` trusts a partial for - // HARD_TTL_PARTIAL and no longer, so the long TTL buys this caller nothing. - // (The same misclassification on the PUBLIC routes' own writes predates this - // PR and is tracked in #155; their semantics are deliberately untouched here.) + // The ENTRY's TTL is the caller-independent one, always. This slot is SHARED + // with badge.ts and the permalink pages, so a TTL picked to suit this caller + // is silently imposed on theirs. `hardTtlFor()` puts a gallop partial + // (`firstRelease` set + `partial: soft_deadline`) on the terminal 30-day + // branch; writing HARD_TTL_PARTIAL here instead would drop the PUBLIC routes' + // effective TTL on that shape from 30 days to 60 seconds for as long as a + // link is being unfurled — one OG unfurl replacing the month-long entry a + // human page view just wrote, so the next page view pays another full + // traversal and issue.tsx/pr.tsx's bot branch (`if (!cached) return + // renderDeferred(...)`) falls back to the deferred card off a slot that was + // warm a minute ago. That misclassification is a real defect — it is just not + // this caller's to fix on someone else's entry. It is tracked in #155, and + // the public routes' semantics are deliberately untouched here. + // + // What THIS caller needs — never trusting a partial for longer than + // HARD_TTL_PARTIAL — rides on the companion marker below instead. The marker + // is caller-private and expires on its own clock, and `shouldRecompute` reads + // the MARKER, never the entry's TTL, so the 60s throttle window is exactly + // what it was. const pinnedPartial = Boolean(consumerPinsResult && r.partial); - await cache.put(key, r, pinnedPartial ? HARD_TTL_PARTIAL : hardTtlFor(r)); - // Record WHOSE truncation this was, so the throttle above honours it only - // for this caller. Same TTL as the entry, so it can never outlive it. + await cache.put(key, r, hardTtlFor(r)); + // Record WHOSE truncation this was, so the throttle above honours it only for + // this caller, and for no longer than HARD_TTL_PARTIAL. The entry may now + // outlive the marker (a gallop partial keeps hardTtlFor's 30 days); that is + // the safe direction — no marker means `ownRecentPartial` is false and the + // partial is recomputed rather than served to a pinning consumer. if (pinnedPartial) { await cache.put(partialKey(key), { pinnedPartial: true }, HARD_TTL_PARTIAL); } @@ -363,11 +378,20 @@ export async function resolveLookup(args: { // Throttle the next retry, then serve last-known-good if we have it. const upstreamStatus = upstreamStatusOf(err); const anubis = err instanceof ProviderJsonError && err.looksLikeAnubis; - await cache.put( - negKey(key), - { transient: true, kind: err.kind, status: upstreamStatus, anubis }, - NEG_TTL, - ); + // ...but never re-stamp a marker this call walked straight past. `backedOff` + // is still true here only on the bypass fall-through above, which means the + // marker was already warm and this caller ignored it. Writing it again resets + // its age for everyone reading the shared key, which is what would starve the + // public routes' retry window (see the fall-through comment). Skipping the + // write costs this caller nothing — it does not read the marker on this path, + // and the warm marker it bypassed already records the host as down. + if (!backedOff) { + await cache.put( + negKey(key), + { transient: true, kind: err.kind, status: upstreamStatus, anubis }, + NEG_TTL, + ); + } if (prior && !shouldRecompute(prior)) return staleHit(); return { status: 'transient', kind: err.kind, upstreamStatus, anubis }; } diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 9f9680b..8be8869 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -229,14 +229,21 @@ async function resolveResult(c: Context, input: LookupInput): Promise // consumer that asks ONCE is a placeholder pinned long after the host recovers. // // The asymmetry is deliberate: this caller opts out of READING the marker on - // a cold slot, but resolveLookup still WRITES it, so a failure discovered - // here can back off a human permalink for up to 60s. That is the point of - // sharing the slot — the marker describes the HOST being down, not who found - // it out, and the host is equally down for the human. They get the - // "checking…" recovery card (never a wrong "not yet released") and can + // a cold slot, but resolveLookup still WRITES it when the slot was cold, so a + // failure discovered here can back off a human permalink for up to 60s. That + // is the point of sharing the slot — the marker describes the HOST being down, + // not who found it out, and the host is equally down for the human. They get + // the "checking…" recovery card (never a wrong "not yet released") and can // reload; the crawler asks once and keeps what it got. Suppressing the write // would instead leave the key with no back-off at all whenever the crawler // touches it first, and every human reload would pound the down host. + // + // What it does NOT do is re-stamp a marker it bypassed. Writing on every + // bypassed failure would keep the shared marker permanently younger than + // NEG_TTL under a once-a-minute unfurl cadence, and the humans on this key — + // who do not bypass — would never see it expire, i.e. never get the retry + // window the back-off exists to give them. resolveLookup writes the marker + // only when it was cold (`!backedOff`), so their clock runs uninterrupted. bypassBackOffWhenUnservable: true, // web-og renders whatever we return into a PNG it long-caches for 24h, and // nothing here can invalidate that PNG afterwards. So no exit may hand this diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index d318e33..9cb5f8e 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -353,19 +353,29 @@ describe('/internal/* follows the cache policy that governs the shared slot', () expect(cacheControlOf(PUBLIC_ORIGIN, k)).toBe('public, max-age=60'); }); - // Round 9 (#144). The partial above has `firstRelease: null`, which `hardTtlFor()` - // already routes to its 60-second branch. The OTHER partial shape does not: - // find-release.ts (~295) returns the gallop hit WITH `partial`, and `hardTtlFor()` - // tests `firstRelease` BEFORE `partial`, so it takes the terminal 30-day branch — - // and `isFresh()` reports it fresh forever for the same reason. On `main` that - // could not reach the public routes: /internal wrote a flat 30 minutes onto a key - // nothing else read. Sharing the slot (this PR) makes the most deadline-pressured - // producer of partials a WRITER into it, so one OG-triggered truncated traversal - // would pin the permalink and the badge to a tag the bisect never confirmed for a - // month, with no upstream call able to correct it. `unpinnable` trusts a partial - // for 60 seconds and no longer, so the long TTL buys this route nothing. - // (#155 tracks the same misclassification on the public routes' OWN writes.) - it('writes a gallop-only partial with the 60-second TTL, not the terminal 30 days', async () => { + // Rounds 9 and 15 (#144). The partial above has `firstRelease: null`, which + // `hardTtlFor()` already routes to its 60-second branch. The OTHER partial shape + // does not: find-release.ts (~295) returns the gallop hit WITH `partial`, and + // `hardTtlFor()` tests `firstRelease` BEFORE `partial`, so it takes the terminal + // 30-day branch — and `isFresh()` reports it fresh forever for the same reason. + // That IS a real defect, and #155 tracks it. + // + // Round 9 tried to contain it by writing 60s from THIS caller. Round 15 showed + // why that is the wrong lever: the whole point of this PR is that the slot is + // SHARED, so a TTL chosen here is imposed on badge.ts and the permalink pages + // too. A public page view writes the gallop answer at 30 days; one OG unfurl a + // moment later replaced it with a 60-second entry, and 61 seconds on every later + // human page view paid a fresh traversal while issue.tsx/pr.tsx's bot branch + // rendered the deferred card off a slot that had been warm. Narrowing the public + // routes' TTL is #155's call to make, on the public routes' own writes — not a + // side effect of an unfurl. + // + // So the ENTRY carries the caller-independent TTL, and this caller's refusal to + // trust a partial for more than 60 seconds rides on its own `:pinpartial` marker. + // The route still 503s the partial rather than pinning it (round 8), and + // `shouldRecompute` still recomputes it after 60s (resolve.test.ts) — what + // changed is that nobody else's entry is shortened to buy that. + it('writes a gallop partial on the caller-independent TTL, throttling via its own marker', async () => { const sha = '9'.repeat(40); findReleaseMock.mockResolvedValue({ ...(fixture('v4.19.0') as Record), @@ -376,7 +386,12 @@ describe('/internal/* follows the cache policy that governs the shared slot', () expect(res.status).toBe(503); // never pinned to the crawler — the round-8 guard const k = await publicKey('github.com/honojs/hono', `sha:${sha}`); - expect(cacheControlOf(PUBLIC_ORIGIN, k)).toBe('public, max-age=60'); + // Same value a public caller writes for this shape: the unfurl did not + // shorten anyone's entry. Restoring the round-9 `HARD_TTL_PARTIAL` write + // reddens this with `max-age=60`. + expect(cacheControlOf(PUBLIC_ORIGIN, k)).toBe(`public, max-age=${30 * 24 * 60 * 60}`); + // ...and the 60-second distrust lives on the caller-private marker instead. + expect(cacheControlOf(PUBLIC_ORIGIN, `${k}:pinpartial`)).toBe('public, max-age=60'); }); // The complement: narrowing the TTL for a pinning consumer must not touch the @@ -531,6 +546,25 @@ function cacheOriginProblems(cfg: WranglerCfg): string[] { problems.push( `[env.${name}.vars] PUBLIC_BASE_URL keys on the PRODUCTION origin \`${origin}\``, ); + continue; + } + // ...and on a `*.workers.dev` host the origin is DERIVED from the Worker name: + // the first label IS `[env.] name`. So the literal can be checked against + // the deployment it claims to describe, and a rename that leaves the URL behind + // fails the build instead of shipping an env whose canonical links, `og:url` and + // sitemap point at a host that no longer exists (`publicBaseUrl()` prefers this + // var over the request origin, so those are no longer correct by construction). + // Only for workers.dev: an env on a custom domain has no such relationship, and + // asserting one there would reject a legitimate config. The account subdomain + // (`..workers.dev`) is NOT in this file at all, so no config guard can + // check it — a deploy under a different account is caught by the preview + // liveness check, not here. + const host = new URL(origin).host; + const workerName = e.name; + if (workerName && host.endsWith('.workers.dev') && host.split('.')[0] !== workerName) { + problems.push( + `[env.${name}.vars] PUBLIC_BASE_URL \`${origin}\` does not match [env.${name}] name \`${workerName}\``, + ); } } return problems; @@ -590,6 +624,44 @@ describe('every deployed environment configures a routable cache origin of its O ]); }); + // The rename this guard exists for. `PUBLIC_BASE_URL` is a literal, but on + // workers.dev the host it must equal is generated from `name` — so the two can + // drift with a one-line edit and nothing at runtime would notice: the origin + // stays set, routable and non-prod, and `publicBaseUrl()` goes on serving + // canonical/`og:url`/sitemap URLs for a Worker that no longer answers. + it('rejects an env whose PUBLIC_BASE_URL was left behind by a `name` rename', () => { + const renamed: WranglerCfg = { + ...cfg, + env: { + ...cfg.env, + preview: { + name: 'released-web-pr-preview', + vars: { PUBLIC_BASE_URL: 'https://released-web-preview.lukaso.workers.dev' }, + }, + }, + }; + expect(cacheOriginProblems(renamed)).toEqual([ + expect.stringContaining('does not match [env.preview] name'), + ]); + }); + + // ...and the same check must not fire on an env served from a custom domain, + // where the host bears no relation to the Worker name. Without the + // `.workers.dev` condition this config would be rejected outright. + it('accepts an env on a custom domain, whose host cannot match the Worker name', () => { + const custom: WranglerCfg = { + ...cfg, + env: { + ...cfg.env, + staging: { + name: 'released-web-staging', + vars: { PUBLIC_BASE_URL: 'https://staging.blabberate.com' }, + }, + }, + }; + expect(cacheOriginProblems(custom)).toEqual([]); + }); + // The reason this suite calls `originOf` instead of restating its rule. A // single-label host WITH a port is what a dev on Docker / Codespaces / WSL // reaches the Worker at, and the route accepts it (internal.ts's diff --git a/packages/web/test/resolve.test.ts b/packages/web/test/resolve.test.ts index a2a1627..1dc96d5 100644 --- a/packages/web/test/resolve.test.ts +++ b/packages/web/test/resolve.test.ts @@ -38,6 +38,7 @@ function mkResult(opts: { released: boolean; partial?: boolean }): LookupResult /** In-memory WorkerCache whose entry ages are set explicitly by the test. */ function makeFakeCache() { const store = new Map(); + const puts: { key: string; ttlSeconds?: number }[] = []; const cache: WorkerCache = { async get(key: string) { return (store.get(key)?.value as T) ?? null; @@ -46,12 +47,19 @@ function makeFakeCache() { const e = store.get(key); return e ? { value: e.value as T, ageSeconds: e.ageSeconds, stampedAt: e.stampedAt } : null; }, - async put(key: string, value: T) { + async put(key: string, value: T, ttlSeconds?: number) { + puts.push({ key, ttlSeconds }); store.set(key, { value, ageSeconds: 0, stampedAt: Date.now() }); }, }; return { cache, + /** Every `cache.put` in call order, with the TTL the caller asked for. The + * slot is shared across callers that do not agree on a deadline, so the TTL + * a write imposes on it is observable behaviour, not an implementation + * detail — asserting only the stored VALUE cannot see a caller shortening + * someone else's entry. */ + puts, /** `stampedAt` is the write-time `x-cached-at` the real cache stores. It is * INDEPENDENT of `ageSeconds` here on purpose: production derives the two * from different `Date.now()` samples, so a test has to be able to express a @@ -610,3 +618,119 @@ describe('resolveLookup — a terminal RELEASED prior stays pinnable at any age' if (r.status === 'ok') expect(r.result.firstRelease?.tag).toBe('4.18.0'); }); }); + +// Round 15. Two defects the SHARED key introduced, both invisible to a test that +// looks only at what a single caller is handed back: the key is shared with +// badge.ts and the permalink pages, so what /internal writes to it — the entry's +// TTL, and the negative marker's age — is imposed on THEM. +describe('resolveLookup — a pinning caller does not rewrite the SHARED slot for everyone else', () => { + const HARD_TTL_RELEASED = 30 * 24 * 60 * 60; + const HARD_TTL_PARTIAL = 60; + + const galloped = () => mkResult({ released: true, partial: true }); + + it('writes a gallop partial on the caller-independent TTL, not its own 60s throttle', async () => { + const f = makeFakeCache(); + const load = vi.fn().mockResolvedValue(galloped()); + + await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + bypassBackOffWhenUnservable: true, + }); + + // The ENTRY keeps hardTtlFor()'s terminal branch — the same value a public + // page view writes — so an unfurl cannot shorten the permalink's month-long + // slot to a minute. Writing HARD_TTL_PARTIAL here reddens this. + expect(f.puts.find((p) => p.key === KEY)?.ttlSeconds).toBe(HARD_TTL_RELEASED); + // The caller's own distrust of the partial rides on the marker instead. + expect(f.puts.find((p) => p.key === pinPartialKey)?.ttlSeconds).toBe(HARD_TTL_PARTIAL); + }); + + it('a public caller and a pinning caller write the IDENTICAL entry TTL', async () => { + const pub = makeFakeCache(); + const pin = makeFakeCache(); + const load = vi.fn().mockResolvedValue(galloped()); + + await resolveLookup({ cache: pub.cache, key: KEY, load }); + await resolveLookup({ + cache: pin.cache, + key: KEY, + load, + consumerPinsResult: true, + bypassBackOffWhenUnservable: true, + }); + + const ttlOf = (f: ReturnType) => + f.puts.find((p) => p.key === KEY)?.ttlSeconds; + expect(ttlOf(pin)).toBe(ttlOf(pub)); + }); + + it('still recomputes its OWN partial once the marker has expired', async () => { + const f = makeFakeCache(); + // A 30-day entry sitting in the slot, and a marker that has aged out. + f.seed(KEY, galloped(), 5 * 60); + f.seed(pinPartialKey, { pinnedPartial: true }, HARD_TTL_PARTIAL + 1); + const load = vi.fn().mockResolvedValue(mkResult({ released: true })); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + bypassBackOffWhenUnservable: true, + }); + + // The longer entry TTL must not make a stale partial servable: the throttle + // reads the MARKER. Making shouldRecompute trust the entry reddens this. + expect(load).toHaveBeenCalledOnce(); + expect(r.status).toBe('ok'); + if (r.status === 'ok') expect(r.result.partial).toBeUndefined(); + }); + + it('does NOT re-stamp a negative marker it bypassed — the humans keep their clock', async () => { + const f = makeFakeCache(); + // An outage: a page view failed 55s ago, so the shared marker is 5s from + // expiring and a human is 5s from their next real attempt. + f.seed(negKey, { transient: true, kind: 'github_server_error' }, 55); + const load = vi.fn().mockRejectedValue(new ProviderServerError('gitlab.gnome.org', 503, 'x')); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + bypassBackOffWhenUnservable: true, + }); + + // The bypass still ran the load (that is its whole point)... + expect(load).toHaveBeenCalledOnce(); + expect(r.status).toBe('transient'); + // ...but left the marker's age alone. Removing the `!backedOff` guard reddens + // this: the marker is rewritten at age 0 and the human's window restarts, + // which under a once-a-minute unfurl cadence never lets it expire at all. + expect(f.puts.some((p) => p.key === negKey)).toBe(false); + expect(f.get(negKey)?.ageSeconds).toBe(55); + }); + + it('DOES stamp the marker when the slot was cold — the back-off still exists', async () => { + const f = makeFakeCache(); + const load = vi.fn().mockRejectedValue(new ProviderServerError('gitlab.gnome.org', 503, 'x')); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + bypassBackOffWhenUnservable: true, + }); + + expect(r.status).toBe('transient'); + // Suppressing the write unconditionally would leave the key with NO back-off + // whenever the crawler touches it first, and every human reload would pound + // the down host. Guarding on `backedOff` keeps this arm live. + expect(f.puts.some((p) => p.key === negKey)).toBe(true); + }); +}); diff --git a/packages/web/wrangler.toml b/packages/web/wrangler.toml index fc09660..2b21915 100644 --- a/packages/web/wrangler.toml +++ b/packages/web/wrangler.toml @@ -139,11 +139,12 @@ PROD_HOST = "released.blabberate.com" # COUPLING: this var has a SECOND effect. `publicBaseUrl()` (src/env.ts) prefers # it over the incoming request origin, so preview's canonical links, `og:url` and # every `pubBase`-derived href come from this literal rather than being correct by -# construction. Rename `[env.preview] name` below, or deploy under a different -# account subdomain, and preview emits URLs for a host that does not exist — the -# config guard (`cacheOriginProblems`) checks only that the var is set, routable -# and not the production origin, never that it matches the deployed Worker. Update -# this line whenever that name changes. +# construction. The `[env.preview] name` half of that is guarded: on a workers.dev +# host the first label IS the Worker name, and `cacheOriginProblems` +# (test/internal-cache-origin.test.ts) fails the build when the two drift, so a +# rename cannot silently leave this line behind. The ACCOUNT subdomain half is not +# guardable from this file — `.lukaso.workers.dev` appears nowhere else in it — so +# deploying under a different account still needs this line updated by hand. PUBLIC_BASE_URL = "https://released-web-preview.lukaso.workers.dev" [env.preview.assets] From ef2bde6c110b8f67773c7ecefe77cf67e5ef2e9d Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 27 Aug 2026 21:30:50 +0100 Subject: [PATCH 19/22] fix(web): recompute an unstamped marker/entry pair; correct two stale rationales Round 15 review of #144. Three findings, all in this diff's own code or its own docs. 1. `writtenNoEarlierThan`'s missing-stamp fallback went the OPPOSITE way from the direction its header claimed. `cache.getEntry` reports `ageSeconds: 0` for an entry with no `x-cached-at`, so an unstamped pair evaluated `0 <= 0` -> true and the marker vouched for the entry unconditionally. The fallback is removed: an unprovable ordering now recomputes, as documented. 2. The `bypassBackOffWhenUnservable` doc still said "the marker is still WRITTEN on failure". `ea541fe` added `if (!backedOff)`, which makes that false on exactly the path the flag creates. 3. The shared-TTL note explained the gallop-partial misclassification but not what this PR changes about it: sharing the key makes an UNFURL a writer of the slot, so crawler traffic can now pin badge/permalink to an unconfirmed gallop tag. Stated plainly, with the fix still scoped to #155/#159. --- packages/web/src/resolve.ts | 42 +++++++++++++++++----- packages/web/test/resolve.test.ts | 60 +++++++++++++++++++++++++++++-- 2 files changed, 91 insertions(+), 11 deletions(-) diff --git a/packages/web/src/resolve.ts b/packages/web/src/resolve.ts index 1ca1e2e..e8a9cbc 100644 --- a/packages/web/src/resolve.ts +++ b/packages/web/src/resolve.ts @@ -115,12 +115,21 @@ function partialKey(key: string): string { * avoid, on the deadline-blowing repos it exists for. The write-time stamps say * 1010 >= 1000, which is the quantity the guard actually reasons about. * - * Falls back to the age comparison when either stamp is missing (an entry written - * by a version that predates the `x-cached-at` header): same conservative - * direction as before — an unprovable ordering recomputes. */ + * With either stamp missing (an entry written by a version that predates the + * `x-cached-at` header, or one whose header an intermediary dropped) the + * ordering is unprovable, so this says no and the partial is recomputed. + * + * It deliberately does NOT fall back to comparing the two `ageSeconds`. That + * reads as the conservative choice and is the opposite of one: `cache.getEntry` + * reports `ageSeconds: 0` for an unstamped entry (cache.ts:65-66), so an + * unstamped pair evaluated `0 <= 0` -> true and the marker vouched for the entry + * unconditionally — someone else's truncation trusted rather than recomputed. + * Unreachable in production, where every write goes through + * `makeWorkerCache.put` and is stamped; reachable from a seeded fixture, which + * is exactly where a claim like this one gets believed. */ function writtenNoEarlierThan(mark: CacheEntry, entry: CacheEntry): boolean { - if (mark.stampedAt !== null && entry.stampedAt !== null) return mark.stampedAt >= entry.stampedAt; - return mark.ageSeconds <= entry.ageSeconds; + if (mark.stampedAt === null || entry.stampedAt === null) return false; + return mark.stampedAt >= entry.stampedAt; } export type Resolved = @@ -148,8 +157,11 @@ export async function resolveLookup(args: { * becomes permanent for them (the OG crawler). When set, the shared negative * back-off marker is honoured only if there is a prior we can actually SERVE * — with nothing servable, an attempt beats handing back a placeholder that - * gets cached forever. The marker is still WRITTEN on failure, and callers - * that can retry (the public HTML routes) omit this and keep backing off. + * gets cached forever. A bypassed attempt that then FAILS does not re-stamp + * the marker (`!backedOff`, see the catch below), so opting in never resets + * the clock the public routes read; a failure on the ordinary path still + * writes it. Callers that can retry (the public HTML routes) omit this flag + * and keep backing off. * * Be honest about the reachable behaviour. The only caller that sets this also * sets `consumerPinsResult`, and `findRelease` emits just two entry shapes: @@ -350,8 +362,20 @@ export async function resolveLookup(args: { // traversal and issue.tsx/pr.tsx's bot branch (`if (!cached) return // renderDeferred(...)`) falls back to the deferred card off a slot that was // warm a minute ago. That misclassification is a real defect — it is just not - // this caller's to fix on someone else's entry. It is tracked in #155, and - // the public routes' semantics are deliberately untouched here. + // this caller's to fix on someone else's entry. It is tracked in #155/#159, + // and the public routes' semantics are deliberately untouched here. + // + // Be explicit about what this PR does change, which is not the + // misclassification but WHO can trigger it. On `main` /internal keyed on a + // key of its own, so only a human page view or a badge fetch could write a + // gallop partial into the shared slot. Sharing the key makes an UNFURL a + // writer of it: one Slack post of a deadline-blowing repo can now pin + // `badge.svg` and the permalink to an unconfirmed gallop tag — rendered + // with no caveat, because a badge has nowhere to put one — for the full 30 + // days, and each further unfurl restarts that clock. Accepting it here is a + // scope call, not a claim that it is harmless: the fix belongs in + // `hardTtlFor()`/`isFresh()`, on the public routes' side, where #155/#159 + // can be reviewed as the behaviour change to those routes that it is. // // What THIS caller needs — never trusting a partial for longer than // HARD_TTL_PARTIAL — rides on the companion marker below instead. The marker diff --git a/packages/web/test/resolve.test.ts b/packages/web/test/resolve.test.ts index 1dc96d5..99abcab 100644 --- a/packages/web/test/resolve.test.ts +++ b/packages/web/test/resolve.test.ts @@ -65,8 +65,15 @@ function makeFakeCache() { * from different `Date.now()` samples, so a test has to be able to express a * pair whose ages disagree with their true write order. Default it to a * stamp consistent with the age, so existing tests are unaffected. */ - seed(key: string, value: unknown, ageSeconds: number, stampedAt?: number) { - store.set(key, { value, ageSeconds, stampedAt: stampedAt ?? Date.now() - ageSeconds * 1000 }); + seed(key: string, value: unknown, ageSeconds: number, stampedAt?: number | null) { + // `null` seeds an UNSTAMPED entry (one written before `x-cached-at` + // existed, or whose header an intermediary dropped) — distinct from + // omitting the argument, which derives the stamp from the age. + store.set(key, { + value, + ageSeconds, + stampedAt: stampedAt === undefined ? Date.now() - ageSeconds * 1000 : stampedAt, + }); }, has: (key: string) => store.has(key), get: (key: string) => store.get(key), @@ -448,6 +455,55 @@ describe('resolveLookup — a pinned consumer is never handed a cached PARTIAL', if (r.status === 'ok') expect(r.result.partial).toBeTruthy(); }); + // Round 15 review of #144. The header claims the missing-stamp fallback goes + // "the same conservative direction as before — an unprovable ordering + // recomputes". It did the opposite: `cache.getEntry` reports `ageSeconds: 0` + // for an entry with no `x-cached-at` (cache.ts:65-66), so a pair where BOTH + // stamps are absent evaluated `0 <= 0` -> true and the unstamped marker + // vouched for the unstamped entry unconditionally. + it('recomputes when NEITHER the marker nor the entry carries a stamp', async () => { + const f = makeFakeCache(); + const someoneElses = mkResult({ released: false, partial: true }); + // Both unstamped, so `ageSeconds` is 0 on both sides — the shape that made + // the fallback vouch instead of recompute. + f.seed(KEY, someoneElses, 0, null); + f.seed(pinPartialKey, { pinnedPartial: true }, 0, null); + const load = vi.fn().mockResolvedValue(mkResult({ released: true })); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + }); + + expect(load).toHaveBeenCalledTimes(1); + expect(r.status).toBe('ok'); + }); + + // ...and one stamp present is just as unprovable as none: there is nothing to + // compare the stamped side AGAINST, so this must recompute too. + it('recomputes when only ONE side of the pair carries a stamp', async () => { + const f = makeFakeCache(); + const someoneElses = mkResult({ released: false, partial: true }); + // The entry is unstamped and read as 5s old; the marker IS stamped and read + // as 0s old. Ages alone say `0 <= 5` -> vouch, which is the same wrong + // answer by a different route. + f.seed(KEY, someoneElses, 5, null); + f.seed(pinPartialKey, { pinnedPartial: true }, 0, 1000); + const load = vi.fn().mockResolvedValue(mkResult({ released: true })); + + const r = await resolveLookup({ + cache: f.cache, + key: KEY, + load, + consumerPinsResult: true, + }); + + expect(load).toHaveBeenCalledTimes(1); + expect(r.status).toBe('ok'); + }); + // ...and the ordering test must still REJECT a slot genuinely overwritten after // the marker was written, or the fix above is just a way of deleting the guard. it('still disowns a partial written AFTER the marker (badge.ts overwrote the slot)', async () => { From 5bc4e13714efea83cdf2888f24f308af3fb6f341 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Thu, 27 Aug 2026 22:04:09 +0100 Subject: [PATCH 20/22] docs(web): scope the key-alignment claim to issue/PR; correct a stale test rationale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two comment-only corrections from review round 16. No behaviour change. - `routes/internal.ts`: the doc block claimed the `/internal` key "MUST match the public permalink routes' exactly" without noting that this holds for the issue and PR routes only. On the commit route `og-meta.tsx` builds the `og:image` URL from `shortSha()` (7 chars) while `result.tsx` keys the permalink on the full 40, so a commit unfurl still misses the slot the permalink warmed. Record the caveat and point at #147, so the function is not read as having closed #143 for commit links. - `test/cache.test.ts`: the rationale said an unstamped pair makes an ordering test "fall back to ages". `writtenNoEarlierThan` does the opposite — it returns false when either stamp is null, so the pair is always recomputed, never served. State what ships. --- packages/web/src/routes/internal.ts | 10 +++++++++- packages/web/test/cache.test.ts | 4 +++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 8be8869..d260798 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -180,7 +180,15 @@ function failureMessage(resolved: Exclude): string { * ever warm — that was half of #143. That means all five parts, including the * `cull`/`nopre` suffixes for the default (non-strict, no-prerelease) options an * OG card always renders, and the public `issue#`/`pr#` id spelling rather than - * the `issue:`/`pr:` this endpoint used to invent. */ + * the `issue:`/`pr:` this endpoint used to invent. + * + * CAVEAT — this alignment is complete for the issue and PR routes only. On the + * COMMIT route `input.sha` is whatever web-og was handed, and `ui/og-meta.tsx` + * builds the `og:image` URL from `shortSha()` (7 chars) while `result.tsx` keys + * the permalink on the full 40. The two digests differ, so a commit unfurl still + * misses the slot the permalink warmed and still pays a full `findRelease`. + * Closing that needs a change to the PUBLIC routes' key namespace and is tracked + * as #147 — do not read this function as having fixed #143 for commit links. */ async function resolveResult(c: Context, input: LookupInput): Promise { const env = c.env as Env; const req = c.req.raw; diff --git a/packages/web/test/cache.test.ts b/packages/web/test/cache.test.ts index 0d3691c..a33b3ee 100644 --- a/packages/web/test/cache.test.ts +++ b/packages/web/test/cache.test.ts @@ -132,7 +132,9 @@ describe('makeWorkerCache', () => { const entry = await cache.getEntry('k'); expect(entry?.ageSeconds).toBe(0); // ...and reports the stamp as absent rather than inventing one, so an - // ordering test falls back to ages instead of comparing against a fake 0. + // ordering test refuses the pair instead of comparing against a fake 0: + // `writtenNoEarlierThan` (resolve.ts) returns false when either stamp is + // null, so an unstamped marker/entry pair is always recomputed, never served. expect(entry?.stampedAt).toBeNull(); }); }); From 70a164050158470c47f37f9403f0ca76486d2212 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Fri, 28 Aug 2026 22:08:51 +0100 Subject: [PATCH 21/22] fix(web): warn when /internal has no routable cache origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 17 review, packages/web/src/routes/internal.ts:58. `cacheOrigin` passes its two CONFIGURED arms through `originOf`, which rejects a single-label host precisely because `https://web` is what the Cache API silently declines — but the request-origin fallback skips that check, and for a real Service Binding that origin IS `https://web`. So if PROD_HOST is dropped from [vars], blanked in the dashboard, or a new [env.*] ships that the committed wrangler.toml does not describe, every /internal read misses and every write no-ops. `neverFatal` then renders that as "served, just not cached": #143 is back with no error, no log and no metric — the silence that made it look green for weeks. Behaviour is unchanged; the relapse just stops being invisible. The wrangler.toml guard in internal-cache-origin.test.ts covers the committed file, and this covers the config it cannot see. Mutation evidence (all three assertions fail on a concrete input): - remove the warn -> "warns when it falls back to a request origin the Cache API will decline" fails, expected '' to contain 'https://web' - warn unconditionally -> "stays silent when the request-origin fallback is itself routable" fails - warn above the configured early return -> both "stays silent" tests fail pnpm -r test 429 passed, pnpm -r typecheck, pnpm lint clean. --- packages/web/src/routes/internal.ts | 29 +++++++-- .../web/test/internal-cache-origin.test.ts | 61 +++++++++++++++++++ 2 files changed, 86 insertions(+), 4 deletions(-) diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index d260798..3dfe9a6 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -51,11 +51,32 @@ function isServiceBinding(c: Context): boolean { * their public routes key on the origin they actually serve. wrangler.toml sets * PUBLIC_BASE_URL for preview; for `wrangler dev`, put * `PUBLIC_BASE_URL=http://localhost:8787` in packages/web/.dev.vars (README, - * "Daily flow", says the same where a dev will actually look). Only the unit - * tests reach the request-origin fallback — never the Service Binding, whose - * request origin is the non-routable `https://web` that #143 was about. */ + * "Daily flow", says the same where a dev will actually look). + * + * The fallback is NOT passed through `originOf`, because a request origin has no + * configured spelling to normalise — but it is the one input `originOf` exists to + * reject: a real Service Binding arrives as the non-routable `https://web` that + * #143 was about. Today only the unit tests reach it, and only with a routable + * origin; that holds because BOTH deployed envs set a var. If one ever stops — + * PROD_HOST dropped from [vars], blanked in the dashboard, or a new [env.*] + * deployed that the committed wrangler.toml does not describe — every read here + * misses and every write no-ops, and `neverFatal` renders that as "served, just + * not cached". Silence of exactly that kind is why #143 looked green for weeks, + * so say it out loud: the wrangler.toml guard in internal-cache-origin.test.ts + * covers the committed file, and this covers the config it cannot see. */ function cacheOrigin(env: Env, req: Request): string { - return originOf(env.PUBLIC_BASE_URL) ?? originOf(env.PROD_HOST) ?? new URL(req.url).origin; + const configured = originOf(env.PUBLIC_BASE_URL) ?? originOf(env.PROD_HOST); + if (configured) return configured; + + const fallback = new URL(req.url).origin; + if (!isRoutableOrigin(fallback)) { + console.warn( + `released: /internal has no routable cache origin (${fallback}) — ` + + 'the result cache is disabled and every OG unfurl pays a full lookup (#143). ' + + 'Set PUBLIC_BASE_URL or PROD_HOST for this environment.', + ); + } + return fallback; } /** The routability rule `originOf()` applies, exported so the wrangler.toml diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index 9cb5f8e..f9e2626 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -323,6 +323,67 @@ describe('/internal/* cache origin falls back to the request origin', () => { }); }); +// The fallback arm of `cacheOrigin` is the ONE input `originOf` exists to reject: +// a real Service Binding arrives as `https://web`, the non-routable origin the +// Cache API silently declines. `originOf` guards the two CONFIGURED arms, but the +// request-origin fallback is not passed through it — so if PROD_HOST is dropped +// from [vars], blanked in the dashboard, or a new [env.*] ships without one, every +// /internal read misses and every write no-ops, `neverFatal` reports "served, just +// not cached", and #143 is back with no error, no log and no metric. That silence +// is what made #143 look green for weeks. The wrangler.toml guard further down +// cannot see a dashboard-set var or an env added outside the committed file, so +// the relapse has to be observable at RUNTIME too. +describe('/internal/* makes a non-routable cache origin observable (#143 relapse)', () => { + it('warns when it falls back to a request origin the Cache API will decline', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + findReleaseMock.mockResolvedValue(fixture('v4.9.0')); + + // Production's exact shape with PROD_HOST lost: the Service Binding's own + // non-routable `https://web` origin is all that is left to key on. + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), { + INTERNAL_SECRET, + }); + + // The warning is observability, never a behaviour change: the answer still serves. + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.9.0'); + + const said = warn.mock.calls.map((c) => String(c[0])).join('\n'); + warn.mockRestore(); + expect(said).toContain('https://web'); + expect(said).toMatch(/cache/i); + }); + + it('stays silent when a routable origin IS configured', async () => { + // Fails if the warning is unconditional rather than gated on routability — + // production would then log this on every single OG unfurl. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + findReleaseMock.mockResolvedValue(fixture('v4.9.1')); + + await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), ENV); + + const calls = warn.mock.calls.length; + warn.mockRestore(); + expect(calls).toBe(0); + }); + + it('stays silent when the request-origin fallback is itself routable', async () => { + // `wrangler dev` and the unit tests: neither var set, but the request origin is + // a real hostname, so the cache works and there is nothing to report. Fails if + // the warning keys on "took the fallback arm" instead of "origin is unusable". + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + findReleaseMock.mockResolvedValue(fixture('v4.9.2')); + + await app.fetch(svc(`https://released.example/internal/result/honojs/hono/${SHA}`), { + INTERNAL_SECRET, + }); + + const calls = warn.mock.calls.length; + warn.mockRestore(); + expect(calls).toBe(0); + }); +}); + // Sharing the slot with the public routes means sharing the POLICY that governs // it (resolve.ts): 30-day terminal / 24h pending / 60s partial hard TTLs, a // 5-minute freshness window, and a negative back-off. /internal used to invent a From 7da69e473c99b249a07ff574611963ccba7bf592 Mon Sep 17 00:00:00 2001 From: liveapp-bot Date: Sat, 29 Aug 2026 00:08:27 +0100 Subject: [PATCH 22/22] fix(web): warn when a cache-origin var is set but rejected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 18 review, packages/web/src/routes/internal.ts:73. The warning added in 70a1640 only fires when NEITHER var is set. A var that IS set but rejected by `originOf` is indistinguishable from unset: the `??` chain falls straight through to the next arm, `configured` comes back truthy, and the fallback branch is never reached. The reviewer's own scenario does not hold — a fall-through to a valid PROD_HOST lands on `https://released.blabberate.com`, which this Worker does serve, so the cache works. But the finding survives that correction, because the real hazard is PREVIEW, not prod: PUBLIC_BASE_URL exists specifically so preview does not key on production's origin, and PROD_HOST is committed in [env.preview.vars] too (it gates analytics). Mistype PUBLIC_BASE_URL in the dashboard — where the wrangler.toml guard cannot see it — and `originOf` discards it, PROD_HOST answers instead, and the preview Worker writes every /internal entry onto the PRODUCTION origin. Silently, with a perfectly routable origin hiding the fault. Behaviour is unchanged; the discarded override just stops being invisible. Mutation evidence (every assertion fails on a concrete input): - drop the `raw &&` present-check -> "stays silent when a routable origin IS configured" fails (expected 1 to be +0) AND "stays silent when the request-origin fallback is itself routable" fails (expected 2 to be +0) - check only PUBLIC_BASE_URL -> "warns when PROD_HOST is set but rejected" fails, expected '' to contain 'PROD_HOST' - before the fix, both new tests were red on the real defect: expected '' to contain 'PUBLIC_BASE_URL' / 'PROD_HOST' pnpm test 431 passed, pnpm -r typecheck, pnpm lint clean. --- packages/web/src/routes/internal.ts | 20 +++++++ .../web/test/internal-cache-origin.test.ts | 54 +++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/packages/web/src/routes/internal.ts b/packages/web/src/routes/internal.ts index 3dfe9a6..91a1306 100644 --- a/packages/web/src/routes/internal.ts +++ b/packages/web/src/routes/internal.ts @@ -65,6 +65,26 @@ function isServiceBinding(c: Context): boolean { * so say it out loud: the wrangler.toml guard in internal-cache-origin.test.ts * covers the committed file, and this covers the config it cannot see. */ function cacheOrigin(env: Env, req: Request): string { + // A var that is SET but rejected by `originOf` is otherwise indistinguishable + // from unset — the `??` chain falls through, `configured` comes back truthy from + // the next arm, and the fallback warning below is never reached. That silence + // matters most on preview: PUBLIC_BASE_URL exists to stop preview keying on + // production, but PROD_HOST is committed in [env.preview.vars] too, so a + // dashboard-set PUBLIC_BASE_URL typo is caught by neither the `??` chain nor the + // wrangler.toml guard, and preview writes onto the PRODUCTION origin silently. + for (const [name, raw] of [ + ['PUBLIC_BASE_URL', env.PUBLIC_BASE_URL], + ['PROD_HOST', env.PROD_HOST], + ] as const) { + if (raw && !originOf(raw)) { + console.warn( + `released: ${name} \`${raw}\` is not a routable cache origin — ignored. ` + + 'Its /internal cache entries key on whatever origin resolves next, which ' + + "may be another environment's (#143).", + ); + } + } + const configured = originOf(env.PUBLIC_BASE_URL) ?? originOf(env.PROD_HOST); if (configured) return configured; diff --git a/packages/web/test/internal-cache-origin.test.ts b/packages/web/test/internal-cache-origin.test.ts index f9e2626..cf3b18f 100644 --- a/packages/web/test/internal-cache-origin.test.ts +++ b/packages/web/test/internal-cache-origin.test.ts @@ -382,6 +382,60 @@ describe('/internal/* makes a non-routable cache origin observable (#143 relapse warn.mockRestore(); expect(calls).toBe(0); }); + + // The fallback warning above only fires when NEITHER var is set. A var that IS + // set but rejected by `originOf` is indistinguishable from unset: the `??` chain + // falls straight through to the next arm, `configured` is truthy, and the operator + // never learns their override was discarded. + // + // That is not cosmetic on THIS app, because PUBLIC_BASE_URL exists precisely to + // stop preview keying on production: wrangler.toml sets it for [env.preview], and + // PROD_HOST is committed in [env.preview.vars] too (it gates analytics, which must + // stay prod-only). Mistype PUBLIC_BASE_URL in the dashboard — where the + // wrangler.toml guard below cannot see it — and preview writes every /internal + // entry onto the PRODUCTION origin, silently, with a perfectly routable origin + // hiding the fault. + it('warns when PUBLIC_BASE_URL is set but rejected, even though PROD_HOST saves it', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + findReleaseMock.mockResolvedValue(fixture('v4.9.2')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), { + INTERNAL_SECRET, + PROD_HOST, + // Single-label host: `originOf` rejects it for the same reason it rejects + // `web` — the Cache API declines a key URL on a non-routable hostname. + PUBLIC_BASE_URL: 'web-preview', + }); + + // Observability only: the answer still serves off the PROD_HOST origin. + expect(res.status).toBe(200); + expect(await tagOf(res)).toBe('v4.9.2'); + + const said = warn.mock.calls.map((c) => String(c[0])).join('\n'); + warn.mockRestore(); + expect(said).toContain('PUBLIC_BASE_URL'); + expect(said).toContain('web-preview'); + }); + + // The mirror case, and NOT a duplicate: it fails if the check only ever looks at + // PUBLIC_BASE_URL. Here the override is valid and the committed var is the broken + // one, so nothing about the served origin is wrong — only the operator's belief + // about which var is doing the work. + it('warns when PROD_HOST is set but rejected, even though PUBLIC_BASE_URL saves it', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + findReleaseMock.mockResolvedValue(fixture('v4.9.3')); + + const res = await app.fetch(svc(`https://web/internal/result/honojs/hono/${SHA}`), { + INTERNAL_SECRET, + PROD_HOST: 'https:released.example.com', + PUBLIC_BASE_URL: PUBLIC_ORIGIN, + }); + + expect(res.status).toBe(200); + const said = warn.mock.calls.map((c) => String(c[0])).join('\n'); + warn.mockRestore(); + expect(said).toContain('PROD_HOST'); + }); }); // Sharing the slot with the public routes means sharing the POLICY that governs