diff --git a/packages/web-og/src/index.tsx b/packages/web-og/src/index.tsx index 0639876..9438d90 100644 --- a/packages/web-og/src/index.tsx +++ b/packages/web-og/src/index.tsx @@ -4,8 +4,9 @@ // GET /h/:host/r/:projectPath/c/:sha.png — federated (any host, #8) // → Fetch the result data from `web` via Service Binding (D23). // → Render PNG via @cloudflare/workers-og (Satori + resvg-wasm). -// → Cache 24h. On data miss, render a neutral placeholder with short TTL -// (never a long-cached error). +// → Cache 24h, but only for a SETTLED answer (a released commit). A +// not-yet-released or partial result, and a data miss (neutral +// placeholder), get a short TTL so the card can still flip (#151). import { type LookupResult, OG_TEMPLATE_VERSION } from '@released/core'; import { type Context, Hono } from 'hono'; @@ -179,15 +180,90 @@ export default app; // of the og.* zone) recompress the PNG that `web` links as the byte-exact social // card. Everything after it is the actual freshness policy. export const LONG_CACHE = `public, no-transform, max-age=${24 * 60 * 60}, s-maxage=${24 * 60 * 60}`; + +/** A card we could NOT render from a result: the placeholder. Either `/internal` + * missed/failed (transient — retry soon), or the URL carries a template version + * this build cannot render (self-heals once web-og lands). Both want the + * shortest honest retry window, so this stays at 60s. */ export const SHORT_CACHE = 'public, no-transform, max-age=60'; +/** A card we DID render, from an answer that is still in motion: not-yet-released + * or a soft-deadline `partial`. Distinct from SHORT_CACHE because the question is + * different — not "how fast should a failure retry" but "how fast can this answer + * actually change". It cannot change faster than the data behind it, and + * `/internal` stores every computed result for 30 minutes + * (`cache.put(k, r, 30 * 60)`, packages/web/src/routes/internal.ts). A 60s TTL + * therefore bought no freshness the upstream has: it re-ran the ~700ms + * satori+resvg wasm render up to 60x/hour per URL for byte-identical JSON. 300s + * matches the TTL `badge.ts` already uses for the same pending state, and is + * still 6x fresher than the upstream cache it reads through. */ +export const PENDING_CACHE = 'public, no-transform, max-age=300, s-maxage=300'; + +/** True only when the answer the card renders can never change again: a + * completed traversal that found a release. + * + * The lifetime used to key on whether a result came back AT ALL, which + * long-cached two shapes that are still in motion (#151): + * + * - a `partial` is a best-effort answer from a traversal the soft deadline + * truncated, so its `firstRelease` is not confirmed to be the earliest one. + * It has to stay revalidatable rather than be pinned as if it were final. + * **This is the arm that changes production behaviour** (24h → 300s). + * - `firstRelease: null` renders "not yet released", the one card whose whole + * job is to flip once a release contains the commit. Two DIFFERENT shapes + * render it, and only one of them is unreachable: + * + * - BARE null (no `partial`): core does not emit it. `find-release.ts:316` + * is its only `firstRelease: null` return and it always carries + * `partial: soft_deadline`, and a genuine not-yet-released commit throws + * `NotYetReleasedError` (`:326`, `:478` for the issue/PR aggregation), + * which `/internal` turns into a 503 + * (`web/src/routes/internal.ts:67-72`) — so `fetchResult` sees + * `!res.ok`, returns null, and web-og renders the neutral PLACEHOLDER at + * `SHORT_CACHE`, never this card. The bare-null arm below is a defensive + * guard on a shape the type permits and core keeps a fallback branch for + * (`:486`), not a bug that shipped. + * - null WITH `partial: soft_deadline`: REACHABLE, and it ships the "not + * yet released" card today. `find-release.ts:312-322` returns it as a + * normal value, `/internal` caches it and answers 200, so `ResultCard` + * sets `tag = firstRelease?.tag ?? 'not yet released'`. That is the + * blown-soft-deadline route into this card, not `NotYetReleasedError`. + * PR #144 makes `/internal` 503 every `partial`, which closes this route + * until #156 reopens it with a caveat on the card — but the shape is live + * on `main` right now, so the lifetime rule has to be right for it either + * way. It is: PENDING via the `partial` arm above. `routing.test.ts` has + * a copy guard on it. + * + * What that means for #151's headline case: the not-yet-released unfurl is + * still wrong today, but wrong in a different way than "pinned for 24h" — it + * is the neutral "Looking up…" placeholder rather than a card saying the + * commit is unreleased. That is #150, and it is deliberately NOT fixed here: + * it needs `/internal` to stop 503-ing `NotYetReleasedError`, which is a + * change to the web package's error mapping, not to this lifetime rule. + * + * This is STRICTER than the web side, deliberately. `hardTtlFor()` + * (packages/web/src/resolve.ts) tests `firstRelease` first, so a partial + * that carries a `firstRelease` gets the 30-day terminal TTL there, and + * `badge.ts` long-caches the same shape for 24h. A truncated traversal that + * reported v2.0.0 when v1.9.0 was the true earliest is therefore still + * pinned on those two surfaces — the partial half of #151, tracked + * separately in #159. Here it revalidates. + * + * It is the OG analogue of the badge invariant the project already states: + * released → long cache, not-yet/checking → short cache. */ +function isTerminal(result: LookupResult): boolean { + return result.firstRelease != null && !result.partial; +} + export function renderImage( result: LookupResult | null, ctx: { owner: string; repo: string; sha?: string; number?: string }, cacheOverride?: string, ): Response { const SIZE = { width: 1200, height: 630 }; - const cacheControl = cacheOverride ?? (result ? LONG_CACHE : SHORT_CACHE); + const cacheControl = + cacheOverride ?? + (result == null ? SHORT_CACHE : isTerminal(result) ? LONG_CACHE : PENDING_CACHE); const node = result ? ResultCard(result) : PlaceholderCard(ctx); diff --git a/packages/web-og/test/routing.test.ts b/packages/web-og/test/routing.test.ts index 89b2ab1..38191d3 100644 --- a/packages/web-og/test/routing.test.ts +++ b/packages/web-og/test/routing.test.ts @@ -435,9 +435,11 @@ describe('web-og card content', () => { // The SHIPPED badge and the date are gated on `firstRelease` — both gone. expect(text).not.toContain('SHIPPED'); expect(text.some((t) => /^\d{4}-\d{2}-\d{2}$/.test(t))).toBe(false); - // A long-cache header still applies — we DID get a result, it's just unreleased. + // Pending-cached: "not yet released" is a pending state that has to flip + // when the release lands, so it is NOT long-cacheable just because a + // result came back (#151). Lifetime coverage lives in its own describe. expect(res.headers.get('cache-control')).toBe( - 'public, no-transform, max-age=86400, s-maxage=86400', + 'public, no-transform, max-age=300, s-maxage=300', ); }); @@ -745,3 +747,149 @@ describe('web-og issue/PR cards (#79)', () => { expect(/[\u{D800}-\u{DFFF}]/u.test(joined)).toBe(false); }); }); + +// #151: the cache lifetime keys on whether a result was RECEIVED, not on +// whether the answer it renders can still change. Both non-terminal shapes — +// "not yet released" (firstRelease null) and a soft-deadline `partial` — are +// real LookupResults, so both took the 24h cache. The not-yet card is the one +// card whose whole job is to flip when the release lands; a partial is an +// unconfirmed answer from a truncated traversal. Pinning either for a day in +// every crawler's cache is the OG analogue of the badge invariant the project +// already states (released → long, not-yet/checking → short). +describe('web-og cache lifetime keys on terminality, not presence (#151)', () => { + const LONG = 'public, no-transform, max-age=86400, s-maxage=86400'; + // A pending answer is backed by `/internal`'s own 30-minute result cache + // (`cache.put(k, r, 30 * 60)` in packages/web/src/routes/internal.ts), so a + // 60s edge TTL cannot buy freshness the upstream does not have — it just + // re-runs the ~700ms satori+resvg render up to 60x/hour per URL while + // `/internal` hands back byte-identical JSON. 300s matches what badge.ts + // already uses for the same pending state and is still 6x fresher than the + // data behind it. + const PENDING = 'public, no-transform, max-age=300, s-maxage=300'; + // PENDING stays separate from the placeholder's 60s SHORT_CACHE because the + // two answer different questions: SHORT_CACHE is "how fast should a FAILED + // render retry" (binding miss, unrenderable template version), PENDING is + // "how fast can this ANSWER change". Collapsing them by bumping SHORT_CACHE + // to 300 reddens the 14 existing placeholder-lifetime tests above, which is + // the guard for that direction. + + const baseInput = { + kind: 'commit', + repo: { owner: 'facebook', repo: 'react', projectPath: 'facebook/react' }, + sha: 'a'.repeat(40), + }; + const released = { tag: 'v18.2.0', sha: 's', date: '2024-03-15T09:00:00Z', url: '' }; + + async function fetchCard(result: Record): Promise { + return await app.fetch( + new Request('https://og.example/r/facebook/react/c/abc1234.png'), + makeEnv(new Response(JSON.stringify(result))), + ); + } + + // Scope this one honestly, and only to the BARE shape. `firstRelease: null` + // with NO `partial` is what core does not currently emit: `find-release.ts:316` + // is its only null return and always carries `partial: soft_deadline`, and a + // genuine not-yet-released commit throws `NotYetReleasedError`, which + // `/internal` turns into a 503. So this is a DEFENSIVE unit guard on the + // lifetime rule for a shape the type permits (and core keeps a fallback branch + // for at `:486`) — it is not evidence about a state production reaches today. + // It says nothing about `null` WITH a `partial`, which IS reachable and has its + // own copy guard below. (The same distinction is already drawn on + // `pendingFixture` in web's internal-cache-origin tests.) + it('DEFENSIVE: a bare not-yet-released result is PENDING-cached (300s), not pinned', async () => { + const res = await fetchCard({ + input: baseInput, + canonicalSha: 'abc1234def5678', + firstRelease: null, + alsoIn: [], + releaseNotesHtml: null, + rateLimit: null, + }); + expect(res.status).toBe(200); + // The card really does render the flippable copy — so this is the card + // whose lifetime matters, not an unrelated shape. + expect(collectText(lastRenderedNode)).toContain('not yet released'); + expect(res.headers.get('cache-control')).toBe(PENDING); + }); + + // The REACHABLE not-yet-released path, end to end, and the one #151 actually + // asked to be pinned down: `/internal` 503s `NotYetReleasedError`, `fetchResult` + // returns null, and web-og renders the neutral placeholder at SHORT_CACHE. This + // documents what production does today — including that the card does NOT say + // the commit is unreleased, which is #150 and out of scope here. If #150 is + // fixed by making `/internal` return a result instead of a 503, this test goes + // red and points at the lifetime decision that has to be made with it. + it('a not-yet-released commit reaches web-og as a 503 → placeholder, short-cached (#150)', async () => { + const res = await app.fetch( + new Request('https://og.example/r/facebook/react/c/abc1234.png'), + makeEnv(new Response('{"error":"not yet released"}', { status: 503 })), + ); + expect(res.status).toBe(200); + // Not the "not yet released" card — the neutral placeholder. + expect(collectText(lastRenderedNode)).toContain('Looking up…'); + expect(collectText(lastRenderedNode)).not.toContain('not yet released'); + expect(res.headers.get('cache-control')).toBe('public, no-transform, max-age=60'); + }); + + // The REACHABLE route into the "not yet released" card, and the one the + // docstring on `isTerminal` used to disclaim. A blown soft deadline with no + // gallop hit returns `firstRelease: null` + `partial` as a normal VALUE + // (`find-release.ts:312-322`); `/internal` caches it and answers 200, so + // `fetchResult` hands web-og a real result and `ResultCard` renders + // `firstRelease?.tag ?? 'not yet released'`. Asserting the COPY as well as the + // lifetime is the point: the shape reaches this card on `main` today, and #144 + // (503 on every partial) then #156 (render it with a caveat) both move that + // route without changing what the lifetime must be. If either lands without + // deciding this card's lifetime deliberately, this test is what says so. + it('a soft-deadline partial with NO release renders the not-yet copy, PENDING-cached', async () => { + const res = await fetchCard({ + input: baseInput, + canonicalSha: 'abc1234def5678', + firstRelease: null, + partial: { reason: 'soft_deadline', candidatesTried: 12 }, + alsoIn: [], + releaseNotesHtml: null, + rateLimit: null, + }); + expect(res.status).toBe(200); + expect(collectText(lastRenderedNode)).toContain('not yet released'); + // Not LONG: an unconfirmed traversal must stay revalidatable. Not the + // placeholder's SHORT_CACHE either — this is a rendered answer, not a failed + // render. + expect(res.headers.get('cache-control')).toBe(PENDING); + }); + + it('partial result is PENDING-cached even though it carries a firstRelease', async () => { + const res = await fetchCard({ + input: baseInput, + canonicalSha: 'abc1234def5678', + firstRelease: released, + partial: { reason: 'soft_deadline', candidatesTried: 12 }, + alsoIn: [], + releaseNotesHtml: null, + rateLimit: null, + }); + expect(res.status).toBe(200); + // A galloped answer under a blown soft deadline is not confirmed earliest, + // so it must stay revalidatable rather than pinned for a day. + expect(res.headers.get('cache-control')).toBe(PENDING); + }); + + // Complement. Without it, "short-cache everything" passes the two above and + // silently re-renders every settled card once a minute. (Proven: forcing + // isTerminal to false reddens this and 10 pre-existing long-cache tests.) + it('terminal released result keeps the LONG cache', async () => { + const res = await fetchCard({ + input: baseInput, + canonicalSha: 'abc1234def5678', + firstRelease: released, + alsoIn: [], + releaseNotesHtml: null, + rateLimit: null, + }); + expect(res.status).toBe(200); + expect(collectText(lastRenderedNode)).toContain('SHIPPED'); + expect(res.headers.get('cache-control')).toBe(LONG); + }); +});