Skip to content

Commit c336508

Browse files
authored
fix(producer): treat 4xx from Google Fonts as deterministic "not served", not as failClosed trigger (#957)
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.
1 parent 21f5066 commit c336508

2 files changed

Lines changed: 79 additions & 16 deletions

File tree

packages/producer/src/services/deterministicFonts-failClosed.test.ts

Lines changed: 63 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,12 @@
44
* Production callers (the in-process `htmlCompiler`) call the function
55
* without options and get the legacy behavior: external font fetch failures
66
* are swallowed and a warning is logged. Distributed callers pass
7-
* `failClosedFontFetch: true` so missing fonts surface as typed
8-
* non-retryable failures before any chunk is rendered.
7+
* `failClosedFontFetch: true` so non-deterministic infrastructure failures
8+
* (5xx, network errors, DNS) surface as typed non-retryable failures before
9+
* any chunk is rendered. 4xx responses are treated as a *deterministic*
10+
* "Google Fonts does not serve this family" answer — same outcome on every
11+
* retry — and pass through to the embedded-face fallback without
12+
* tripping `failClosedFontFetch` in either mode.
913
*
1014
* The tests inject `fetchImpl` so no real network call happens.
1115
*/
@@ -38,6 +42,19 @@ function makeHttp404Fetch(): typeof fetch {
3842
new Response("", { status: 404, statusText: "Not Found" })) as unknown as typeof fetch;
3943
}
4044

45+
function makeHttp400Fetch(): typeof fetch {
46+
return (async () =>
47+
new Response("", { status: 400, statusText: "Bad Request" })) as unknown as typeof fetch;
48+
}
49+
50+
function makeHttp503Fetch(): typeof fetch {
51+
return (async () =>
52+
new Response("", {
53+
status: 503,
54+
statusText: "Service Unavailable",
55+
})) as unknown as typeof fetch;
56+
}
57+
4158
describe("injectDeterministicFontFaces — failClosedFontFetch: false (default)", () => {
4259
it("swallows a network failure and returns the original HTML (no throw)", async () => {
4360
const result = await injectDeterministicFontFaces(HTML_REQUESTING_UNRESOLVED_FONT, {
@@ -57,6 +74,14 @@ describe("injectDeterministicFontFaces — failClosedFontFetch: false (default)"
5774
expect(result.includes("data-hyperframes-deterministic-fonts")).toBe(false);
5875
});
5976

77+
it("swallows a 5xx response and returns the original HTML (no throw)", async () => {
78+
const result = await injectDeterministicFontFaces(HTML_REQUESTING_UNRESOLVED_FONT, {
79+
failClosedFontFetch: false,
80+
fetchImpl: makeHttp503Fetch(),
81+
});
82+
expect(result.includes("data-hyperframes-deterministic-fonts")).toBe(false);
83+
});
84+
6085
it("preserves legacy behavior when no options object is supplied at all", async () => {
6186
// injectDeterministicFontFaces(html) — no second arg.
6287
// We can't easily mock fetch globally here, so just assert the call
@@ -84,28 +109,50 @@ describe("injectDeterministicFontFaces — failClosedFontFetch: true", () => {
84109
expect((caught as Error).message).toContain("simulated network failure");
85110
});
86111

87-
it("throws FontFetchError on a 404 response", async () => {
112+
it("does NOT throw on a 4xx response — 4xx means 'Google Fonts does not serve this family', a deterministic answer", async () => {
113+
// Google Fonts returns HTTP 400 for non-Google families like
114+
// "Segoe UI", "Arial", "Futura". Same response on every retry, so it
115+
// doesn't violate the byte-identical-retry contract — the render
116+
// falls back to embedded faces / the composition's font-family chain.
117+
// No FONT_FETCH_FAILED.
118+
const result = await injectDeterministicFontFaces(HTML_REQUESTING_UNRESOLVED_FONT, {
119+
failClosedFontFetch: true,
120+
fetchImpl: makeHttp400Fetch(),
121+
});
122+
// No @font-face was injected (no faces returned), but the call resolves.
123+
expect(result.includes("data-hyperframes-deterministic-fonts")).toBe(false);
124+
});
125+
126+
it("does NOT throw on a 404 response either — same reasoning as 4xx generally", async () => {
127+
const result = await injectDeterministicFontFaces(HTML_REQUESTING_UNRESOLVED_FONT, {
128+
failClosedFontFetch: true,
129+
fetchImpl: makeHttp404Fetch(),
130+
});
131+
expect(result.includes("data-hyperframes-deterministic-fonts")).toBe(false);
132+
});
133+
134+
it("throws FontFetchError on a 5xx response — non-deterministic, could differ on retry", async () => {
88135
let caught: unknown;
89136
try {
90137
await injectDeterministicFontFaces(HTML_REQUESTING_UNRESOLVED_FONT, {
91138
failClosedFontFetch: true,
92-
fetchImpl: makeHttp404Fetch(),
139+
fetchImpl: makeHttp503Fetch(),
93140
});
94141
} catch (err) {
95142
caught = err;
96143
}
97144
expect(caught).toBeInstanceOf(FontFetchError);
98145
expect((caught as FontFetchError).code).toBe(FONT_FETCH_FAILED);
99-
expect((caught as Error).message).toContain("HTTP 404");
146+
expect((caught as Error).message).toContain("HTTP 503");
100147
expect((caught as Error).message).toContain("NotARealFontFamilyForTest");
101148
});
102149

103-
it("includes the requested URL in the error", async () => {
150+
it("includes the requested URL in 5xx errors", async () => {
104151
let caught: unknown;
105152
try {
106153
await injectDeterministicFontFaces(HTML_REQUESTING_UNRESOLVED_FONT, {
107154
failClosedFontFetch: true,
108-
fetchImpl: makeHttp404Fetch(),
155+
fetchImpl: makeHttp503Fetch(),
109156
});
110157
} catch (err) {
111158
caught = err;
@@ -114,15 +161,19 @@ describe("injectDeterministicFontFaces — failClosedFontFetch: true", () => {
114161
expect((caught as FontFetchError).url).toContain("NotARealFontFamilyForTest");
115162
});
116163

117-
it("does NOT throw when the HTML uses a pre-bundled font (no fetch happens)", async () => {
118-
// "Inter" is in FONT_ALIASES → uses bundled font data, never reaches
119-
// the Google Fonts path → failClosedFontFetch=true is irrelevant here
120-
// and shouldn't trip. (The full <html><head> wrap is required because
164+
it("does NOT throw when bundled-font Google Fonts supplement returns no extra faces", async () => {
165+
// "Inter" is in FONT_ALIASES → uses the embedded font bundle. Since
166+
// c8e8fdcf, the resolver also queries Google Fonts to supplement any
167+
// weights not in the embedded bundle. A successful empty CSS response
168+
// means the bundle was sufficient — `failClosedFontFetch` doesn't
169+
// trip. (The full <html><head> wrap is required because
121170
// injectDeterministicFontFaces injects into <head>.)
122171
const html = `<!doctype html><html><head><style>body { font-family: "Inter", sans-serif; }</style></head><body></body></html>`;
172+
const fetchImpl = (async () =>
173+
new Response("/* no faces */", { status: 200 })) as unknown as typeof fetch;
123174
const result = await injectDeterministicFontFaces(html, {
124175
failClosedFontFetch: true,
125-
fetchImpl: makeFailingFetch(),
176+
fetchImpl,
126177
});
127178
expect(result).toContain("data-hyperframes-deterministic-fonts");
128179
});

packages/producer/src/services/deterministicFonts.ts

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -430,15 +430,24 @@ async function fetchGoogleFont(
430430
headers: { "User-Agent": WOFF2_USER_AGENT },
431431
});
432432
if (!res.ok) {
433-
if (options.failClosedFontFetch) {
433+
// 4xx is a *deterministic* answer from Google Fonts that this
434+
// family is not served (e.g. HTTP 400 for "Segoe UI", "Arial",
435+
// "Futura" — names absent from Google's catalog) or is misnamed.
436+
// The render falls back to embedded faces / the composition's
437+
// font-family chain; we return [] in both modes. 5xx (and other
438+
// transient upstream failures) could return faces on retry, which
439+
// would break the byte-identical-retry contract distributed
440+
// renders rely on — those still fail closed when requested.
441+
if (res.status >= 500 && options.failClosedFontFetch) {
434442
throw fontFetchError(familyName, url, "Google Fonts CSS", { status: res.status });
435443
}
436444
return [];
437445
}
438446
cssText = await res.text();
439447
} catch (err) {
440-
// Rethrow typed error untouched. Other errors (network, DNS) get wrapped
441-
// when failClosed is on, swallowed otherwise.
448+
// Rethrow typed error untouched. Network / DNS / fetch-throws are
449+
// non-deterministic infrastructure failures — wrapped when failClosed
450+
// is on, swallowed otherwise.
442451
if (err instanceof FontFetchError) throw err;
443452
if (options.failClosedFontFetch) {
444453
throw fontFetchError(familyName, url, "Google Fonts CSS", { error: err });
@@ -467,7 +476,10 @@ async function fetchGoogleFont(
467476
try {
468477
const fontRes = await options.fetchImpl(woff2Url);
469478
if (!fontRes.ok) {
470-
if (options.failClosedFontFetch) {
479+
// Same 4xx vs 5xx split as the CSS fetch above: 4xx = the
480+
// font's woff2 isn't served, skip silently; 5xx = transient
481+
// upstream failure, may fail closed.
482+
if (fontRes.status >= 500 && options.failClosedFontFetch) {
471483
throw fontFetchError(familyName, woff2Url, woff2What, { status: fontRes.status });
472484
}
473485
continue;

0 commit comments

Comments
 (0)