fix(web-og): cache the OG card by terminality, not by result presence (#151)

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);
 

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<string, unknown>): Promise<Response> {
+    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);
+  });
+});