Skip to content

fix(core): embed the weight-list font when the page's Google link gives none - #5186

Merged
xuanruli merged 4 commits into
mainfrom
claude/bricolage-font-fail-closed-e118b1
Oct 8, 2026
Merged

xuanruli merged 4 commits into
mainfrom
claude/bricolage-font-fail-closed-e118b1

Conversation

@xuanruli

@xuanruli xuanruli commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

What

Since #4569 the renderer fetches the Google Fonts stylesheet the page itself links, so the film draws the same cut as the preview. When that link gives no usable faces for a family, the family stays unresolved and a distributed render (fail-closed) throws Unresolved fonts in fail-closed mode. Before #4569 the renderer's own weight-list request (ital,wght@0,100;…;0,900;1,400;1,700) resolved those families.

Production root cause (confirmed)

Datadog, last 14 days, Unresolved fonts in fail-closed mode: 1,031 of ~1,125 errors name Bricolage Grotesque, all HyperframeRenderStreamingWorkflow, first seen 2026-10-06 23:48 UTC — right after hyperframe-internal moved producer from 0.8.70 to 0.8.138 (#2852), the first producer build containing #4569.

The three failing compositions I pulled from S3 link:

css2?family=Bricolage+Grotesque:opsz,wght@7..96,500&family=Hanken+Grotesk:wght@400;500;600&display=swap
css2?family=Bricolage+Grotesque:opsz,wght@7..96,200..800&family=DM+Sans:ital,opsz,wght@0,9..40,100..1000;1,9..40,100..1000&display=swap

Bricolage's opsz axis starts at 12. Requested alone, opsz,wght@7..96,500 is a 400; inside a multi-family link Google answers 200 and silently omits Bricolage, returning only the other family. So the failure is "200 but no faces for this family", not an HTTP 400.

Running injectDeterministicFontFaces(html, { failClosedFontFetch: true, allowSystemFontCapture: false }) on those three real index.html files against live Google Fonts:

  • main: all three throw Unresolved fonts in fail-closed mode: Bricolage Grotesque (matches production).
  • this PR: all three resolve and embed Bricolage Grotesque plus the linked second family.

Other single-family links Google rejects outright (400), e.g. Bricolage+Grotesque:wght@100..900 or wght@900; LLM-written pages produce such links often (experiment-framework VA-1582 repairs wght@400700-style URLs on the GSAP path).

Change

In fetchGoogleFont, if the page's links yield no usable faces for a family, fall back to the weight-list request and warn with fields only — google_font_link_fallback family="…" link_outcomes=http_400|no_faces fallback_faces=N — so Datadog can count real 400s vs silently-dropped families and whether the fallback resolved them. No URL, CSS or page text is logged (an authored link can carry page text in text=). Previously the status was dropped silently.

  • A link that yields faces is used unchanged (the fix(producer): embed the Google font the page linked instead of a wider cut #4569 behavior is kept).
  • Under fail-closed, transient failures (408/425/429/5xx, network) still retry and throw before the fallback.
  • If the fallback also fails, the family is unresolved and fail-closed still throws.
  • Studio preview, the hyperframes-localize-fonts CLI (Video Agent streamed preview) and producer all call injectDeterministicFontFaces, so they embed the same faces.

Falling back on any no-faces outcome (not only 400) is deliberate: the production case is a 200. The fallback is the request the renderer sent before #4569, so font resolution is never worse than it was then.

Tests

deterministicFonts-googleSubsets.test.ts:

  • Multi-family link that returns 200 without Bricolage (the production shape) renders under fail-closed.
  • Bricolage + wght@100..900 (400) renders under fail-closed.
  • 403 / 404 / 200-without-faces fall back; 503 fails-open falls back; 503 under fail-closed throws without falling back.
  • A working link sends no weight-list request.
  • Preview (fail-open) and render (fail-closed) output are byte-identical when the link is rejected.

deterministicFonts-declaredAlias.test.ts: a verbatim-declared unknown family is now queried twice by the same spelling (link + weight list) and still fails closed.

Mutation-checked: the previous resolver fails the new cases; narrowing the fallback to 400 only fails the 403/404/503/200 cases. src/fonts: 227/228 pass; the remaining one (local system-font capture timeout) fails on main too in this environment.

🤖 Generated with Claude Code

xuanruli and others added 3 commits October 7, 2026 14:48
…es none

Since #4569 the renderer fetches the Google Fonts URL the page wrote. When
that link is malformed (e.g. Bricolage Grotesque with wght@100..900, past the
family's 200..800), Google answers 400, the family stays unresolved, and a
distributed render fails closed. Before #4569 the renderer's own weight-list
request resolved it.

If the page's links yield no usable faces, fall back to that weight-list
request and warn with each link's status. Fail-closed transient errors still
throw before the fallback, and a working link is used unchanged. Preview and
render share this resolver, so both embed the same faces.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mily

Production pages link Bricolage Grotesque with opsz@7..96 alongside a second
family; Google answers 200 and returns only the second family.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An authored link can carry page text in `text=`, so the warning now records
only the family, each link's outcome (http_<status> or no_faces) and how many
faces the weight-list fallback embedded.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at 476a6dc4.

The root cause holds against live Google Fonts. I sent the request shapes from the body to fonts.googleapis.com with a woff2 user agent:

  • Bricolage+Grotesque:opsz,wght@7..96,500&family=Hanken+Grotesk:wght@400;500;600 returns 200 with 12 faces, all Hanken and none Bricolage. That is the silent drop.
  • Bricolage+Grotesque:opsz,wght@7..96,500 alone returns 400, and with 12..96 it returns 200 with 3 faces.
  • The fallback, ital,wght@0,100;…;0,900;1,400;1,700, returns 200 with Bricolage at 200 to 800, with and without text=. Google tolerates the out-of-range 100/900 and the missing italics, so the fallback does resolve the production family.

The change is right.

  • fetchFontResource throws under fail-closed on a retryable status or network error once retries run out. So a transient miss on the page's link still fails the render before any fallback, as the body says. Only a 4xx or a 200 without faces reaches the weight list.
  • Faces still go through parseGoogleFontSource's font-family check, so a multi-family link can't lend one family's faces to another. A link that yields any faces returns them unchanged, which keeps the #4569 behavior.
  • The fallback goes through the same per-mode googleFontCssCache key, so repeat compiles of a dropped family make one extra request per process.
  • Families in FONT_ALIASES and families with no page link take the weight-list path directly, as they did before.

Local runs. vitest run src/fonts passes 225 of 226. The one failure is aliasSupplement "has a canonical display name for every alias target", which also fails at the merge-base 21d14b25b. When I replaced if (faces.length > 0) return faces; with return faces;, 9 tests in googleSubsets/declaredAlias failed, including the multi-family drop case.

Non-blocking

  • Comments is red because this adds 2 comment lines to deterministicFonts.ts, raising its comment share from 192/1949 to 194/1969. It isn't required, but cutting one comment line keeps it green.
  • link_outcomes=no_faces covers three different cases: a 200 that drops the family, a fail-open network error ({ css: null } with no status), and a woff2 that wouldn't download. If the Datadog count needs to tell those apart, the network case could report network.
  • Under fail-open (Studio preview), a page whose link drops a family now embeds the weight-list faces instead of falling back to a system font. That is the pre-#4569 result and matches the render. The faces are static instances at default optical size rather than the opsz range the page asked for.

CI. At this head, 35 checks had passed and the required regression shards, Windows render and tests, and Studio edit-accuracy were still running when I posted.

— Rames

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Edit accuracy: accurate 2059 (base branch 2059), smooth 1629 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

Unstable (1)

  • crop-none-px-r30-root-z100: tracking 0.04, pressJump 0, drop 40.08, reload 40.08, render 34.74, renderKey -, undo true, teleport true / tracking 0.04, pressJump 0, drop 0.08, reload 0.08, render 0.26, renderKey -, undo true, teleport true / tracking 0.04, pressJump 0, drop 0.08, reload 0.08, render 0.26, renderKey -, undo true, teleport true

@jerrai-bot-heygen jerrai-bot-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At 476a6dc4, the fallback is functionally sound in the reviewed paths: a 200 stylesheet that omits Bricolage triggers the family-specific weight list without replacing Hanken's linked faces; transient authored-link failures still throw before fallback in fail-closed mode. The new tests cover both boundary cases, and required hosted checks are green. I did not run local tests or a live render.

One change before my approval: the added test cases use function-local dynamic imports in deterministicFonts-googleSubsets.test.ts. Please hoist the imported symbol to the existing module-level import (inline comment). Our top-level-import-only rule includes tests. The non-required Comments ratchet and the broad no_faces warning label are cleanup/observability notes, not functional blockers. No merge or rollout action from this review.

— Jerrai

Comment thread packages/core/src/fonts/deterministicFonts-googleSubsets.test.ts Outdated
…bset tests

Also drops the two comments the fallback added, so the file's comment share does not rise.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@jrusso1020 jrusso1020 left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving at 3e64e33f. The delta since my approval at 476a6dc4 is the fix for the change request on function-local imports, plus two deleted comments.

  • injectDeterministicFontFaces is now imported at module scope alongside _clearGoogleFontCssCacheForTests, and all 20 await import("./deterministicFonts.js") calls are gone, including the pre-existing ones. This doesn't change behavior. The test file already imported ./deterministicFonts.js statically, so the module was loaded before any test ran. HYPERFRAMES_FONT_CACHE_DIR, which beforeAll sets, is read lazily inside resolveFontCacheRoot() / defaultCacheDir() at call time, never at module load.
  • deterministicFonts.ts loses only the two comments above the fallback request and the warn line. No code changes, and this should clear the comment ratchet.

Required checks were still running when I posted; GitHub won't merge until they're green.

— Rames

@jerrai-bot-heygen jerrai-bot-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import-hygiene hold is fixed at 3e64e33f: the test file now imports injectDeterministicFontFaces at module scope, removes the function-local dynamic imports, and leaves the fallback/error behavior unchanged. The two new production comments were removed; Comments is green. My previous functional review still applies to this one-commit cleanup. Required Windows render and runtime-contract checks were still pending at approval; code approval is not CI completion or deployment evidence. The broader no_faces label can be refined separately if the dashboard needs that distinction.

— Jerrai

@xuanruli
xuanruli enabled auto-merge October 7, 2026 22:52
@xuanruli
xuanruli added this pull request to the merge queue Oct 8, 2026
Merged via the queue into main with commit 09f359c Oct 8, 2026
142 of 143 checks passed
@xuanruli
xuanruli deleted the claude/bricolage-font-fail-closed-e118b1 branch October 8, 2026 00:23
jerrai-bot-heygen added a commit that referenced this pull request Oct 8, 2026
…0072026

Conflict in packages/core/src/fonts/deterministicFonts.ts: kept #5186's authored-zero-faces
default fallback and its stylesheet result/status shapes unchanged; threaded the diagnostics
attempt through every stylesheet fetch (authored and default fallback each get their own).

- Jerrai
@xuanruli xuanruli mentioned this pull request Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants