fix(web-og): stop workers-og's 1-year immutable default overriding OG cache-control - #141
Conversation
Preview deployed
Federated GitLab lookups (freedesktop / GNOME) degrade to the "use the CLI" card — the Anubis relay is off in preview. GitHub lookups, permalinks, and OG render work once |
Preview verification (head
|
cache-control on /placeholder.png |
|
|---|---|
production (og.released.blabberate.com) |
public, immutable, no-transform, max-age=31536000, public, max-age=60 |
| preview (this PR) | public, max-age=60 |
The library default is gone and the caller's value is the whole header. That is the bug this PR is about, confirmed end-to-end against workers-og as actually bundled — not against the test mock.
osv dependency scan is now green on this head (was the only red check).
Honest limit on this verification: preview only exercises the SHORT-cache path
I could not confirm the long-cache (real-result) path on the preview, and want to be explicit rather than imply broader coverage than I have.
preview /r/honojs/hono/c/f82aba8….png → 200, 14348B, cache-control: public, max-age=60
prod /r/honojs/hono/c/f82aba8….png → 200, 28899B, cache-control: public, …, max-age=86400
The preview is serving a placeholder for a commit that is stably released — so every OG image on the preview is the short-cache branch, whatever the input.
It is not this PR and not a production bug (prod renders the real card correctly). Isolating it:
- preview
webresolves the lookup fine —POST /api/lookuponreleased-web-previewreturnsfirstReleasefor that commit, so provider access and the algorithm are healthy there; - preview
web-ogstill falls back to placeholder for the same commit.
So the failure is between them: the web-og → web Service Binding handshake over /internal/*, which fails closed without a matching INTERNAL_SECRET on the preview env (1bdafac). That secret is set on production but appears not to be set for [env.preview].
Worth fixing separately, because it is quietly misleading: anyone exercising an OG change on a preview is only ever testing the placeholder branch, and a real-result regression would look green there. Filed to the backlog; not a merge blocker for this PR.
The long-cache path here is covered by the unit tests instead — test/cards.render.test.ts asserts exact cache-control against the real workers-og under workerd, and the mutation proof in the description shows those assertions failing on the reverted fix.
Third commit:
|
|
liveapp: both review threads on
Both guards were mutation-tested against the defect in their own title (evidence in each thread). Test count on this PR: 44 unit + 8 workers-runtime, all green. |
|
Independent production evidence that this PR fixes a live bug — gathered from prod
and a dynamic card ( Two cache-control values are concatenated into one field. workers-og's 1-year default Verification detail, since it changes how much the check is worth:
This PR remains 10/10 green with 0 unresolved review threads. Not re-pushing it just to One note on the CVE floors this branch also carries ( |
… cache-control
Every OG image shipped a merged cache-control header:
public, immutable, no-transform, max-age=31536000, public, max-age=60
workers-og builds its response headers as
{ 'Content-Type': …, 'Cache-Control': <1-year immutable default>, ...opts.headers }
and object spread is case-SENSITIVE, so renderImage's lowercase 'cache-control'
never replaced the capitalized default. Both keys reached `new Response`, where
Headers merged them into one comma-joined value — and caches honor the FIRST
max-age.
Impact is on the short-cached cards. A placeholder render (cold lookup, upstream
rate-limit, service-binding miss, or the notFound path hit during the web→web-og
deploy window) is deliberately max-age=60 so the unfurl refreshes once the real
answer is available. Instead it went out immutable for a year, so a transient
failure froze a wrong social card in every downstream cache. This is the
mechanism behind the "sustained placeholder on a stably-released MR" seen on the
federated PR OG route.
Fix: set cache-control on the Response after construction. Headers.set is
case-insensitive, so it replaces the library default whatever casing it uses.
Tests: the routing suite mocked ImageResponse with a stub that passed the
caller's headers straight through, so it never saw the library default — every
cache-control assertion there was unfalsifiable. The mock now mirrors
workers-og's real header construction (capitalized default + case-sensitive
spread), the assertions are exact-match instead of substring (a substring match
passes on the merged value), and cards.render.test.ts asserts the header against
the REAL library under workerd.
The un-merge fix in c102b89 corrected every short-cached card, but it also dropped /placeholder.png from an (accidentally) effectively-immutable header to public, max-age=60 — it routes through renderImage(null, …), so it inherits the null-result short cache. That route is the one null-result render that is not transient: it takes no owner/repo/sha, so the PNG is byte-identical on every request, and web only ever links it as /placeholder.png?v=${OG_TEMPLATE_VERSION} (og-meta.tsx:56), so a template change busts the URL rather than waiting out a TTL. At 60s every homepage/result-less unfurl re-runs a ~700ms satori+resvg wasm render for an image that cannot differ. renderImage takes an explicit cacheOverride and the two cache strings are hoisted to LONG_CACHE / SHORT_CACHE. Only the static route passes it; the genuinely transient null renders (service-binding miss, the notFound deploy-window path) keep the short cache, which is the point of c102b89. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
release.yml deploys `web` before `web-og`, so on an OG_TEMPLATE_VERSION bump `web` is already emitting `og:image=.../placeholder.png?v=og.vNEXT` while this Worker is still the old build. The old route ignored the query and stamped the 24h cache on a stale-template card — poisoning the busting URL itself, with no second URL left to bump. That is the same "one transient render freezes a wrong social card" class this PR exists to fix. Take the long cache only when the requested `v` is a version this build can actually render; anything else falls back to the 60s cache and self-heals once web-og lands. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ransform
Two review findings on this PR's own diff.
1. The deploy-window version gate landed on `/placeholder.png` only, but
`web` links EVERY card as `<url>.png?v=${OG_TEMPLATE_VERSION}`
(og-meta.tsx) and `release.yml` deploys `web` before `web-og`. So during
the window the six dynamic routes (/r, /h/../r, /i, /p, /h/../i, /h/../p)
still took the 24h cache for a `?v=og.vNEXT` URL rendered from the OLD
template — pinning a stale card, on exactly the routes whose content
differs per commit/issue/PR, with no second URL left to bust. The gate now
lives in a shared `renderCard` helper the routes call: a `v` this build
cannot render forces SHORT_CACHE and self-heals 60s after web-og lands. No
`v` at all is not evidence of a mismatch, so it keeps the default.
2. `res.headers.set` replaces workers-og's whole default value, which
silently dropped the `no-transform` that shipped on every OG response
before this PR. Folded back into LONG_CACHE/SHORT_CACHE with a comment
saying why: these PNGs are the byte-exact social card, and a transforming
edge (Polish/Mirage) must not recompress them.
ff3068e to
ea85570
Compare
The bug
Every OG image in production ships a merged
cache-controlheader. Live right now:workers-ogbuilds its response headers asObject spread is case-sensitive, so
renderImage's lowercase'cache-control'neverreplaced the library's capitalized
'Cache-Control'default. Both keys reachednew Response, whereHeadersmerged them into one comma-joined value — and caches honorthe first
max-age.Why it matters
The damage is on the short-cached cards. A placeholder render — cold lookup, upstream
rate-limit, service-binding miss, or the
notFoundpath hit during theweb→web-ogdeploy window — is deliberately
max-age=60so the unfurl refreshes once the real answerexists. Instead it went out
immutablefor a year, so one transient failure froze a wrongsocial card in every downstream cache that honors it.
This is a concrete mechanism for the "sustained placeholder on a stably-released MR"
behaviour previously observed on the federated PR OG route: once a placeholder was cached,
nothing could dislodge it.
Front door stayed green throughout — status 200, valid PNG, correct content-type. Only the
header was wrong.
Fix
Set
cache-controlon the Response after construction.Headers.setis case-insensitive,so it replaces the library default whatever casing the library uses (rather than relying on
matching its casing in the spread, which would re-break on a library change).
Why the tests didn't catch it
test/routing.test.tsmockedImageResponsewith a stub that passed the caller's headersstraight through, so it never reproduced the library default. All 11
cache-controlassertions were unfalsifiable — they read back exactly what the caller passed. They were
also substring matches (
toMatch(/max-age=60/)), which pass on the merged value even with afaithful mock. Both were fixed:
workers-og's real header construction (capitalized default +case-sensitive spread of the caller's headers);
toBe);test/cards.render.test.tsgets two assertions against the real library underworkerd — no mock fidelity required.
Mutation proof
Reverting the one-line fix (passing
'cache-control'back throughImageResponse'sheadersoption) while keeping the tests:The
Received:values are byte-identical to what production serves today. Restored → 35/35 pass(27 routing + 8 render).
Second commit:
fix(deps)— clearing the redosv dependency scanThis branch was based on
main, which carries 3 High CVEs, so the osv gate was red for areason unrelated to the OG fix. Rather than leave the PR unmergeable, the floors are raised
here (root
pnpm.overrides, lockfile-only resolution changes):undicijs-yamlnanoidThe
undiciandjs-yamlfloors are byte-identical to the ones #137/#138/#139 alreadycarry.
nanoidis a newer advisory that none of those three cover — their osv checkslast ran 2026-08-07 and passed, which is now stale. So merging #137, #138 or #139 alone
leaves main at 1 High; this is currently the only open PR that takes main to zero High.
Verified locally on this exact tree — the gate goes red → green:
pnpm lint,pnpm -r typecheck,pnpm -r test(357 tests) all green on the merged branch.Deliberately not fixed here: the 4 remaining
honomediums (4.12.32 → 4.12.34,GHSA-54fx / -79qm / -8j4g / -f23p).
honois the production runtime framework and a directdependency; the osv gate does not block on Medium, and Dependabot's weekly
npm-minor-patchgroup already has it in range (
^4.12.32). Left to that PR so this one stays reviewable.