fix(producer): keep large local fonts file-backed - #2864
Conversation
17b6c03 to
adc9401
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — grade A — rubric CORRECT
Root-cause fit: Matches APPS-972: compile-stage V8 OOM from readFileSync(font) + Buffer.toString('base64') + result.replaceAll producing multiple immutable copies of a multi-MB string. Streaming read short-circuits above 5 MiB, keeping large fonts file-backed and bounding the compiler heap. Fix is format-agnostic (ttc/ttf/otf/woff/woff2), which is the right call.
Claims verified (each at HEAD):
- 5 MiB threshold + streamed size probe:
htmlCompiler.ts:1660-1679—MAX_LOCAL_FONT_DATA_URI_BYTES = 5 * 1024 * 1024,for await+totalBytes > thresholdreturnsfile-backedbefore buffering the whole file. - Authored relative URL preserved when file-backed:
htmlCompiler.ts:1708-1715— file-backed branch skipsresult.replaceAll, only adds toembeddedPathsso authoredurl("assets/…")survives. - Small fonts still inline:
htmlCompiler.ts:1716-1721— unchangedtoDataUripath; existing dedup test athtmlCompiler.test.ts:892(fake-woff2) exercises inline path. - 6 MiB regression test:
htmlCompiler.test.ts:894-919— assertsnot.toContain("data:font/collection;base64,")andBuffer.byteLength(compiled.html) < 1 MiB. - File-list matches +59/-5:
htmlCompiler.ts (+32/-5)+htmlCompiler.test.ts (+27/-0).
Adversarial findings:
- nit —
existsSync(absPath)check removed athtmlCompiler.ts:1703in favor of ENOENT-through-catch. A genuinely missing font now silently traverses the emptycatch {}at:1729with no log versus the prior silentcontinue. Same observability gap, slightly worse call cost. - nit — file-backed reads bypass
dataUriByAbsolutePathcache, so the same font referenced via two URL spellings (assets/x.ttcvs./assets/x.ttc) re-streams up to 5 MiB per spelling. Minor. - nit — threshold is a module-private const with no env override. Fine for a targeted fix; document if it ever needs tuning per-tenant.
CI state: All required checks green. (The two "fail" rows in Lint / Producer: integration tests are runner cancellations during bun install / apt-get setup, not assertion failures.)
Suggested next step: Ship. Optional follow-up: promote MAX_LOCAL_FONT_DATA_URI_BYTES to an env-tunable and cache the file-backed decision by absPath so alternate URL spellings only stream once.
— Review by Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at adc94017.
Clean fix. Stream-then-decide bounds the compiler heap without changing the URL-serving contract for local renders and distributed plans, and the test exercises the boundary well (6 MiB fixture, HTML < 1 MiB assertion, both not.toContain data:font/collection and toContain url("assets/large.ttc") to catch either regression direction). LGTM from my side.
Observations
MAX_LOCAL_FONT_DATA_URI_BYTES = 5 * 1024 * 1024is a magic number. The comment explains the rationale (base64 expansion + immutable-string retention), but not why 5 MiB specifically — five feels about right for common.ttccollections, but a follow-up telemetry read (or a boot-time env override) would make future tuning cheaper if the OOM reappears with 4-MiB fonts at higher HTML replacement counts.- The
existsSync(absPath)pre-check was removed, and missing files now surface as acreateReadStreamENOENT that the surroundingtry/catchswallows — equivalent behavior (URL is preserved because thereplaceAllnever runs), just a slightly more expensive miss.
Nits
- The file-backed verdict isn't cached per
absPath. If the same font is referenced under two different URL strings (e.g.assets/large.ttcand./assets/large.ttc), both strings resolve to the sameabsPathbut each triggers a fresh stream that decides file-backed again. Bounded and unlikely, but aSet<string>of file-backed abs-paths would be cheap symmetry withdataUriByAbsolutePath. - Boundary is
>(strict), so an exactly-5-MiB font gets inlined. Matches the comment's intent, just worth naming in the constant's docstring so the boundary is not accidentally re-tuned to>=.
What I didn't verify
- End-to-end that the compiled
url("assets/large.ttc")actually resolves at bothfileServer.ts(local) and the distributed plan-copy path. The PR body claims both do; trusting it based on the "project assets already available to both execution modes" invariant.
— Review by Rames D Jusso
adc9401 to
ce95743
Compare
|
Review follow-up at exact head
Verification: producer compiler suite 105/105, producer typecheck, changed-file lint/format, diff check, and commit hooks. |
* fix(producer): keep large local fonts file-backed * fix(producer): avoid local font file races * fix(producer): bound local font stream reads * fix(producer): cache large font file-backed decisions
What
Keep project-local font files larger than 5 MiB file-backed during render compilation instead of expanding them into compiled HTML data URIs.
Why
Raw font collections can be tens of megabytes. Base64 adds roughly 33%, and repeated immutable HTML strings add further transient copies, so embedding one large local font can exhaust the compiler's V8 heap before capture begins.
Project assets are already available to local and distributed render modes, so large fonts do not need to be inlined for deterministic rendering.
How
Test plan