fix(producer): retry transient deterministic font fetches - #2865
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 24bc8a8.
Clean, tightly-scoped fix. The typed-error hierarchy (FontFetchError base + FontFetchUnavailableError subclass carrying code=FONT_FETCH_UNAVAILABLE), the bounded retry mechanism (2 attempts × 8s attempt-timeout × 20s shared budget, honors Retry-After only when it fits the budget, full-jitter backoff), determinism preservation (4xx not retried, still classifies as FONT_FETCH_FAILED), and the AWS/GCP boundary normalizers all read as a considered production fix rather than a shotgun retry-everywhere.
Verified:
- Determinism preserved. 4xx statuses (excluding 408/425/429) short-circuit out of the retry loop and reach
fetchGoogleFont's deterministicreturn [](missing-font) branch. Only retryable statuses (5xx/408/425/429) + transport errors + timeouts enter the loop; only exhaustion producesFONT_FETCH_UNAVAILABLE. - CodeQL fix at this head is real.
resolveFontCacheRootnow usesmkdtempSync(join(tmpdir(), "hyperframes-fonts-"))for the Lambda path, memoized inlambdaFontCacheRootper warm process. Owner-only0o700frommkdtempSync, unguessable subdirectory. Combined with the pre-existing{flag: "wx", mode: 0o644}on the actual file write, the symlink-race surface is closed on Lambda. - Cancellation propagation.
abortSignalthreads throughrunCompileStage→compileForRender→injectDeterministicFontFaces→fetchFontResource→AbortSignal.anymerge with the per-attempt timeout signal.throwIfCallerAbortedafter every fetch attempt and insidewaitForFontFetchRetry. ThecompileStage.test.ts:242-278test doesawait expect(result).rejects.toBe(cancellation)— reference-equality, proving the caller's Error is propagated as-is through the whole chain. - Test coverage. 15 retry-focused specs in the new
deterministicFonts-retries.test.tscover: retry-per-status (408/425/429/500/502/503), CSS retry recovery, woff2 retry recovery, exhaustion → UNAVAILABLE, stalled body-cancel path, response-body transport failure, per-attempt timeout, long Retry-After exceeding budget, shared elapsed budget across multiple font resources, 4xx no-retry (stillFONT_FETCH_FAILED), caller cancellation without retry/wrap, fail-open one-attempt-no-typed-failure. Plus updateddeterministicFonts-failClosed.test.tsfor the new error types. Plus AWS/GCP boundary normalization tests. Plus the snapshot-test assertion thatFONT_FETCH_UNAVAILABLEis NOT in the terminal-code set.
Concerns are all nits — inline for the specific lines.
Scope check
PR body explicitly excludes #2768 / #2778. Files touched are all deterministicFonts.ts/tests, adapter boundaries, and compileStage/plan abort-signal threading. No changes to font aliases, URLs, or authored font data. Scope check passes.
Rollout ordering
Rollout order (Merge OSS → publish → merge internal mapping → bump internal → EF classification) is safe: hf-internal#533's mapping is inert against 0.7.78 (only fires when OSS emits the new code), so #533 is mergeable independently and only activates after the OSS package publishes and the internal bumps.
|
|
||
| let lastCause: { status: number } | { error: unknown } = { | ||
| error: new DOMException("Font fetch budget exhausted", "TimeoutError"), | ||
| }; |
There was a problem hiding this comment.
🟢 Full-jitter can produce 0-ms retries. Math.floor(Math.random() * (ceiling + 1)) where ceiling = baseDelayMs * 2 ** attempt. At baseDelayMs=250 on attempt 0, this returns [0, 250]. Roughly ~0.4% of retries will land at 0-ms delay.
For a real Google Fonts 502 storm with a large HF install base retrying concurrently, full-jitter's whole point is to spread the retry wave across the delay window. A 0-ms delay is a legitimate outcome of the full-jitter formula (that's what "full" means), but it means every ~250th retry hammers Google immediately. Whether this matters depends on your concurrent-install scale during a 502 incident.
If the intent is "spread but never zero-delay", the fix is Math.floor(Math.random() * ceiling) + minDelayMs with a small floor (say 50ms). If full-jitter-with-zero is intentional (matching AWS's canonical full-jitter recipe), leave the code and add a short comment justifying it. Non-blocking. — Rames D Jusso
| : `failed: ${(cause.error as Error).message}`; | ||
| : `failed: ${cause.error instanceof Error ? cause.error.message : String(cause.error)}`; | ||
| const message = | ||
| `[deterministicFonts] ${what} fetch for ${JSON.stringify(familyName)} ${reason}. ` + |
There was a problem hiding this comment.
🟢 Error message says "deterministic" for both codes. FontFetchError and FontFetchUnavailableError share this factory, so both variants get the trailing sentence "Distributed renders require deterministic fonts; system-font fallback would produce non-byte-identical output."
That's accurate for FONT_FETCH_FAILED (deterministic — will always fail, needs terminal handling). For FONT_FETCH_UNAVAILABLE it's misleading: the retry is expected to succeed and produce byte-identical output. A support engineer reading a log line for an UNAVAILABLE case would reasonably conclude the render is permanently broken when it's actually retryable.
Cosmetic — worth splitting the trailing sentence into two variants (retryable → "Distributed adapters will retry; check upstream health if this persists."; terminal → the current text). — Rames D Jusso
|
|
||
| export interface FontFetchRetryPolicy { | ||
| /** Total fetch attempts for one CSS or woff2 URL. Default: 2. */ | ||
| maxAttempts: number; |
There was a problem hiding this comment.
🟢 Only the Lambda path gets mkdtempSync protection. The CodeQL flag was Lambda-specific (shared /tmp), and the fix here closes it correctly. But HYPERFRAMES_FONT_CACHE_DIR (explicit override) and the non-Lambda homedir()/.cache/hyperframes/fonts path retain the pre-fix posture — predictable path, world-visible on multi-user machines, no mkdtempSync wrapping.
For local dev this is fine — single-user machines, public font content, and the {flag: "wx"} O_EXCL flag on the actual file write prevents symlink races. For any future multi-tenant surface that plumbs HYPERFRAMES_FONT_CACHE_DIR to a shared dir, the CodeQL concern re-emerges. Worth a comment above resolveFontCacheRoot locking the invariant ("the HYPERFRAMES_FONT_CACHE_DIR override MUST be a private per-process directory"). Non-blocking. — Rames D Jusso
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head review at 24bc8a8221a632471f2d917d4600f30a5080643e: approved. The transient/deterministic error split, bounded per-URL retries, shared deadline/abort propagation, adapter normalization, and regression coverage are coherent; exact-head CI is fully green. Rollout remains gated on publishing a newly versioned OSS producer/adapters package before the internal dependency bump activates this contract.
What
Retry transient deterministic Google Fonts fetch failures during distributed render planning and preserve a retryable
FONT_FETCH_UNAVAILABLEerror across the AWS and GCP adapters.Why
The render-error spike was caused by transient Google Fonts CSS/woff2 502 responses. The existing path made one request, collapsed the failure into
FONT_FETCH_FAILED, and caused orchestration to treat temporary dependency unavailability as terminal.The affected fonts are reachable now. This change does not modify font aliases, URLs, or authored font data, and intentionally does not include or depend on #2768 or #2778.
How
Retry-Afteronly when it fits the shared budget.FONT_FETCH_FAILED.FONT_FETCH_UNAVAILABLE.FONT_FETCH_FAILEDterminal.Test plan
Rollout
Companion PRs: