fix(producer): treat 4xx from Google Fonts as deterministic "not served", not as failClosed trigger#957
Merged
jrusso1020 merged 1 commit intoMay 19, 2026
Conversation
miguel-heygen
approved these changes
May 19, 2026
Collaborator
miguel-heygen
left a comment
There was a problem hiding this comment.
APPROVE — clean regression fix with good test coverage.
Verified:
- All 18
googleNamevalues match their Google Fonts listing names (includingIBM Plex Mono,JetBrains Mono,EB Garamond,Noto Sans JP) - Alias routing correct:
segoe ui→ queriesRoboto,helvetica neue→ queriesInter, etc. - Emitted
@font-facestill declaresoriginalCaseFamilyfor CSS selector matching — compositions aren't broken googleNamefield is the right design: explicit mapping vs trying to derive (would fail for multi-word names)- 3 new tests cover the critical alias path + verify the emitted font-family name
- Reworked pre-existing test now accurately tests bundled-font injection
Root cause clear: c8e8fdcf (v0.6.21+) passed raw CSS family name to fetchGoogleFont instead of the canonical Google Fonts name. Google returns 400 for non-Google families, failClosedFontFetch: true aborted the render.
This also explains the style-8-prod baseline diff from PR #946 — the font weight fetching now works correctly with canonical names.
2cd0869 to
887029d
Compare
…ed", not as failClosed trigger After c8e8fdc added a Google Fonts supplement-fetch to the Path 1 (bundled-font) branch, every `plan()` call against a composition whose CSS named a non-Google family that Google Fonts 400s on (e.g. `"Segoe UI"`, `"Arial"`, `"Futura"`) started failing on distributed renders with `FONT_FETCH_FAILED`. Distributed renders default to `failClosedFontFetch: true`, and the existing code treated *all* non-2xx responses uniformly: throw if failClosed, swallow otherwise. That conflates two very different failure modes: - **4xx** is a *deterministic* answer — Google Fonts does not serve this family, and won't serve it on retry either. The byte-identical- retry contract distributed renders rely on is unaffected; the render falls back to embedded faces / the composition's font-family chain (which is what it would have done pre-c8e8fdcf anyway). No reason to fail-close here. - **5xx** (and network / DNS / fetch exceptions) is *non-deterministic* infrastructure failure. A retry might succeed and produce different pixel output than the first attempt — exactly what `failClosedFontFetch` is meant to protect against. Keep failing closed in this mode. Fix: split the !res.ok branch in both the CSS fetch and the woff2 fetch inside `fetchGoogleFont` — only `>= 500` paired with failClosed throws; 4xx returns `[]` in both modes. Network/DNS exceptions in the catch block are unchanged (still failClosed-gated). This: - Unblocks distributed renders for compositions that name any cross-alias system font Google doesn't serve (Segoe UI, Arial, etc.). - **Preserves the current regression baseline** — Google Fonts actually *does* serve some non-canonical names (e.g. "Helvetica" and "Helvetica Neue" both return HTTP 200 with real @font-face rules, confirmed via curl), so the supplement-fetch still runs and binds those real faces to the composition's CSS family names exactly as today. style-7-prod (which uses `"Helvetica Neue", Helvetica, Arial, sans-serif`) continues to render against real Helvetica glyphs. - Leaves the FONT_ALIASES table and call-site untouched. The fix is in the right place — the error semantics inside fetchGoogleFont — not in any composition-aware logic upstream. Tests: - 2 new positive cases on `failClosedFontFetch: true`: 400 and 404 responses no longer throw, render falls back cleanly. - 2 new negative cases on `failClosedFontFetch: true`: 503 throws `FONT_FETCH_FAILED`, error includes URL + family. - 1 new case on `failClosedFontFetch: false`: 5xx swallowed as before. - The pre-existing "does NOT throw when the HTML uses a pre-bundled font" test was broken by c8e8fdc (the supplement-fetch always fires for self-aliased bundled fonts now). Updated it to use a successful empty CSS response, which is the actual invariant we want. 12/12 tests pass. Followup discussion: should `FONT_ALIASES` exist at all in a deterministic cloud renderer? Today `font-family: "Helvetica"` in CSS silently produces real Helvetica glyphs (via Google Fonts' undocumented alias serving) and `font-family: "Segoe UI"` silently produces embedded Roboto, with no warning to the author. That's a WYSIWYG violation worth its own proposal — but not in scope for this fix.
887029d to
e7fd88b
Compare
miguel-heygen
approved these changes
May 19, 2026
Collaborator
miguel-heygen
left a comment
There was a problem hiding this comment.
Re-approving at e7fd88b — same commit, re-stamping for require_last_push_approval.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
After c8e8fdc added a Google Fonts supplement-fetch to the Path 1 (bundled-font) branch of
buildFontFaceCss, everyplan()call against a composition whose CSS named a non-Google family that Google Fonts 400s on (\"Segoe UI\",\"Arial\",\"Futura\", …) started failing on distributed renders withFONT_FETCH_FAILED. Distributed defaultsfailClosedFontFetch: true, and the existing code treated all non-2xx responses the same way — throw if failClosed, swallow otherwise. That conflates two very different failure modes.Confirmed via curl against Google Fonts:
Fix
Split the
!res.okbranch infetchGoogleFont(both CSS fetch and woff2 fetch):[]in both modes; render falls back to embedded faces / the composition's font-family chain (which is what it would have done pre-c8e8fdcf anyway).failClosedFontFetchis meant to guard against. Keep failing closed in this mode.3-line change in error handling; no call-site or
FONT_ALIASESchanges; no behavior change for any composition that wasn't previously broken.Why this and not [fix at the call site]
Earlier revisions of this PR tried two other shapes that I rejected:
canonical.googleNametofetchGoogleFont(fetch Inter for Helvetica). Driftedstyle-7-prod's baseline by 67 frames because it silently bound Inter's 50+ Google weights tofont-family: \"Helvetica\", expanding the weight palette beyond what the baseline captured.style-7-prodby 60 frames — because Google Fonts actually serves real Helvetica glyphs under the\"Helvetica Neue\"and\"Helvetica\"names, and the current baseline depends on those real glyphs. Skipping the fetch fell back to embedded Inter, which is a different render.The right fix is the error-semantics one:
failClosedFontFetchshould protect against non-determinism, not against "font doesn't exist on Google" (which is itself deterministic). With this fix, every existing composition keeps the exact behavior it has today — what changes is only that distributed renders no longer crash on 4xx.Test plan
bun test packages/producer/src/services/deterministicFonts-failClosed.test.ts— 12/12 pass:failClosedFontFetch: true: 400 and 404 responses no longer throw, render resolves cleanly.failClosedFontFetch: true: 503 throwsFONT_FETCH_FAILED, error includes URL + family.failClosedFontFetch: false: 5xx swallowed as before.bun run --cwd packages/producer buildclean.bunx oxlint+bunx oxfmt --checkclean.Unblocks
Distributed renders against compositions that name any non-Google family Google Fonts 400s on (Segoe UI, Arial, Courier New, Futura). Pairs with the c=3 worker-0-only SwiftShader probe fix shipped in #956 — the texture-launch validation bench was blocked on this issue (Segoe UI in the fixture's CSS); we ran validation on a Space-Mono fixture instead and confirmed the worker-0 fix works.
Followup (not in this PR)
Should
FONT_ALIASESexist at all in a deterministic cloud renderer? Todayfont-family: \"Helvetica\"silently produces real Helvetica glyphs (via Google Fonts' undocumented alias serving) andfont-family: \"Segoe UI\"silently produces embedded Roboto, with no warning to the author. That's a WYSIWYG violation worth its own proposal.🤖 Generated with Claude Code