Skip to content

feat(producer): fail-closed font fetch flag in deterministicFonts#775

Open
jrusso1020 wants to merge 1 commit into
feat/producer-plan-validate-no-system-fontsfrom
feat/producer-fail-closed-font-fetch
Open

feat(producer): fail-closed font fetch flag in deterministicFonts#775
jrusso1020 wants to merge 1 commit into
feat/producer-plan-validate-no-system-fontsfrom
feat/producer-fail-closed-font-fetch

Conversation

@jrusso1020
Copy link
Copy Markdown
Collaborator

@jrusso1020 jrusso1020 commented May 13, 2026

What

Adds an options bag to injectDeterministicFontFaces:

injectDeterministicFontFaces(html, {
  failClosedFontFetch?: boolean;  // default false
  fetchImpl?: typeof fetch;       // default global fetch
})

When failClosedFontFetch === true, any non-OK Google Fonts CSS response, any non-OK woff2 response, and any network error during either fetch throws FontFetchError with code === "FONT_FETCH_FAILED". When false (the default), behavior is unchanged: failures swallowed, composition falls through to warnUnresolvedFonts.

Why

Part of Phase 2, §5.3 (banned in distributed mode) and §9.3 (typed non-retryable failures).

The current in-process behavior — silently fall back to system fonts when a Google Fonts fetch fails — would silently desync chunk workers in distributed mode (workers run in a Linux container that doesn't have macOS / Windows system fonts). Phase 3's plan() needs the worker pool to fail closed on missing fonts so the failure is observable before any chunk is rendered.

The flag is gated false for in-process callers so producer regression baselines stay byte-identical.

How

  • Threaded the flag through injectDeterministicFontFaces → buildFontFaceCss → fetchGoogleFont via an internal InternalFontFetchOptions type.
  • Both fetch call sites (CSS request, woff2 request) wrap their failure paths: HTTP errors and exception paths each check the flag and either swallow (legacy) or throw a typed FontFetchError (locked).
  • FontFetchError carries code, familyName, url, and (where applicable) the underlying cause.
  • fetchImpl defaults to global fetch so production callers don't need to plumb anything.

Test plan

  • Unit tests added: deterministicFonts-failClosed.test.ts (10 cases). Uses an injected fetchImpl that throws a TypeError or returns HTTP 404 to simulate failure without going over the network.
    • Default failClosedFontFetch: false: both network failure and 404 swallowed, no data-hyperframes-deterministic-fonts style block injected.
    • Locked: network failure → throws FontFetchError with FONT_FETCH_FAILED code, offending family name preserved; HTTP 404 → same with HTTP 404 in the message; URL included in error.url.
    • Carve-out: font-family: "Inter" uses bundled font data and never reaches the fetch path, so failures don't trip the flag.
    • Empty composition (no font-family) → no error.
  • In-process caller (htmlCompiler.ts) continues to call without options — gets legacy behavior.
  • bun run --cwd packages/producer typecheck clean.
  • bunx oxlint + bunx oxfmt --check clean.

This is part of a stack of 10 PRs; this is PR 10 of 10. Stacked on top of #774.

🤖 Generated with Claude Code

Part of Phase 2 of the distributed rendering plan (determinism hardening).
See DISTRIBUTED-RENDERING-PLAN.md §5.3 (banned in distributed mode) and
§9.3 (typed non-retryable failures).

Today `injectDeterministicFontFaces(html)` swallows external font-fetch
failures: a failed Google Fonts CSS request or woff2 download returns
empty arrays, the composition warns via `warnUnresolvedFonts`, and Chrome
falls back to system fonts. That fallback would silently desync chunk
workers in distributed mode (workers run in a Linux container that
doesn't have macOS / Windows system fonts), so distributed renders need
to fail closed.

This change adds an options bag to `injectDeterministicFontFaces`:

  injectDeterministicFontFaces(html, {
    failClosedFontFetch?: boolean;  // default false
    fetchImpl?: typeof fetch;       // default global fetch
  })

When `failClosedFontFetch === true`, any non-OK CSS response, any non-OK
woff2 response, and any network error during either fetch throws a typed
`FontFetchError` with `code === FONT_FETCH_FAILED`. When `false` (the
default), behavior is unchanged.

`fetchImpl` lets unit tests inject failing-fetch stubs without going over
the network.

The in-process caller (`htmlCompiler.ts`) continues to call
`injectDeterministicFontFaces(html)` without options and gets the legacy
behavior. Phase 3's `plan()` will pass `failClosedFontFetch: true`.

Producer regression baselines remain byte-identical: no caller flips the
flag.

10 unit tests at packages/producer/src/services/
deterministicFonts-failClosed.test.ts pin both branches (default
swallows network error / 404; locked throws FontFetchError with correct
code, URL, and family name) plus the "no fetch happens for bundled
fonts" carve-out.

This is part of a stack of 10 PRs; this is PR 10 of 10.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jrusso1020 jrusso1020 force-pushed the feat/producer-fail-closed-font-fetch branch from 571e58d to f79fc8a Compare May 13, 2026 01:33
@jrusso1020 jrusso1020 force-pushed the feat/producer-plan-validate-no-system-fonts branch from 8d068be to b13acb6 Compare May 13, 2026 01:33
Copy link
Copy Markdown
Collaborator

@miguel-heygen miguel-heygen left a comment

Choose a reason for hiding this comment

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

Well-threaded flag. The failClosedFontFetch gate is clean — both fetch sites (CSS and woff2) check the flag, FontFetchError is re-thrown untouched through the catch chain, and the default false preserves in-process behavior. The fetchImpl injection for tests is the right approach — no global mocking needed.

The centralized fontFetchError builder keeping message phrasing identical across the four call sites (CSS/woff2 x HTTP-error/exception) is a nice touch.

CI note: the render-compat shard failure is a Docker image build step issue (unrelated to this PR's changes).

LGTM.

— Magi

Copy link
Copy Markdown
Collaborator

@vanceingalls vanceingalls left a comment

Choose a reason for hiding this comment

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

Strengths:

  • failClosedFontFetch flag threaded through THREE fetch sites (CSS request, woff2 HTTP error path, woff2 exception path) — deterministicFonts.ts:382-401 (CSS) and :454-471 (woff2). Each site has both "swallow" and "throw typed" branches; pinned by tests at deterministicFonts-failClosed.test.ts:67 (network failure) and :81 (HTTP 404). Rule 2 contract audit confirms: all fetch-fail-paths consistently honor the flag.
  • Centralized fontFetchError builder (deterministicFonts.ts:373) keeps the four call-sites phrased identically — important because a workflow adapter regex-ing message strings would otherwise need four patterns.
  • if (err instanceof FontFetchError) throw err; at deterministicFonts.ts:399 and :469 correctly re-raises typed errors without re-wrapping — guards against double-wrap when the inner fontFetchError already threw.
  • The bundled-font carve-out test (deterministicFonts-failClosed.test.ts:118 "Inter" uses bundled data) pins the important contract that bundled fonts NEVER hit the failClosed path — so production callers that only use bundled fonts can opt in to failClosed without false positives.
  • fetchImpl injectable (deterministicFonts.ts:354) keeps the tests pure — no real network reach.

Findings:

important: deterministicFonts.ts:471 woff2-loop iteration: when a SINGLE weight/style face fails (e.g. only the italic-900 woff2 returns 404) under failClosedFontFetch: true, the throw aborts the ENTIRE family load, including the other 17 weights that may have already been cached or downloaded successfully. This is fine if the policy is "any missing face = fail closed" (which is what the comment at deterministicFonts.ts:486 implies). But Google Fonts CSS responses sometimes legitimately omit a weight/style combo the request asked for (the wght@0,100;… request asks for weights the family may not publish). Need to verify: does Google Fonts CSS always return a src: url(...woff2) for every requested face, or does it 404 on missing faces? If the latter, this validator will spuriously fail for any partial-weight-coverage family. Suggested test gap: a test that pins behavior when only SOME woff2 fetches 404 — does it throw (current behavior) or fall back to the loaded faces? The current code throws; the right answer depends on intended UX. Worth confirming with a fixture test.

important: deterministicFonts.ts:475 if (existsSync(cachePath)) — when the woff2 is already cached on disk, the fetch is SKIPPED entirely under failClosedFontFetch: true. A pre-existing cached file from a prior non-failClosed run could mask a Google Fonts removal — the family appears available because the cache file exists, but a fresh worker pod without the cache would fail closed. Distributed workers may not share the disk cache (chunks in separate containers). Pinning: add a test that asserts failClosedFontFetch: true + cache-miss + 404 = throw, AND failClosedFontFetch: true + cache-hit + (hypothetical) Google removal = passes (current behavior). Or, more cleanly, disable cache reads under failClosedFontFetch: true so the failure mode is observable on every chunk. Worth a follow-up.

nit: deterministicFonts.ts:362 cause?: unknownError.cause is the standard property. Setting it via this.cause = cause (line :364) is fine but the standard idiom is to pass it to super({ cause }). Either works in modern Node.

nit: deterministicFonts.ts:351 readonly code: typeof FONT_FETCH_FAILED = FONT_FETCH_FAILED — literal-type annotation forces err.code to narrow to "FONT_FETCH_FAILED" at call sites. Good for exhaustive switch statements over error codes. Worth noting in a comment so future readers don't widen the type.

CI: the regression-shards (render-compat, ...) failure shown is still running on the latest commit per gh run view --in-progress — not a confirmed failure on this PR. Will resolve when the run completes. (Rule 5 check: cross-checked on main where the same shard is also in-flight, so it appears to be a base-state delay rather than a regression introduced by this PR.)

Verdict: APPROVE
Reasoning: Three-fetch-site failClosed contract is consistently applied, bundled-font carve-out is pinned. The partial-weight-404 and cache-mask concerns are real but warrant follow-up tests/decisions before Phase 3 wires this for production, not blockers on the default-off land.

— Vai

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