perf(ci): order Dockerfile.test layers so producer changes do not rebuild core - #2822
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed at exact head 65ad73212a5cafdec53136518d30081c2b688a67.
Dockerfile.test:93-122 now places the cache boundary at the actual dependency boundary. I checked the package manifests, build scripts, and source references for parsers, lint, studio-server, and core: none reads engine or producer inputs. Engine and producer correctly remain below the two core build layers, while package manifests stay above bun install --frozen-lockfile so workspace installation remains complete.
The reordered image is also validated by the exact-head regression workflow: all eight Docker-backed shards passed, along with Build, Test, Typecheck, producer unit/integration, lint, and smoke lanes. The visible failed aggregator belongs to an earlier cancelled duplicate workflow; the superseding run 30234509212 completed successfully at this same head.
No blockers or follow-ups from me.
— Magi
Verdict: APPROVE
Reasoning: The layer reordering matches the workspace dependency graph and preserves a complete, green image build while preventing producer-only source changes from invalidating core build layers.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at exact head 65ad73212a5cafdec53136518d30081c2b688a67.
Straightforward cache correctness fix. The COPY packages/engine/ + COPY packages/producer/ layers move from above the parsers/lint/studio-server/core build steps to below them, so producer- or engine-only PRs stop invalidating those upstream builds.
Verification
- Dependency graph clean. Grepped every workspace
package.jsonabove the reordered COPY:packages/{core,parsers,lint,studio-server}list neither@hyperframes/enginenor@hyperframes/producerindependenciesordevDependencies. Nothing above the moved COPYs reads engine or producer sources, so bun's--filterbuild resolves without them. --no-cacheis the strongest signal. A full--no-cachebuild succeeding proves the reordered graph is genuinely satisfiable — any hidden build-script#include-style read from engine/ or producer/ would have failed at the earlier stage.generate-font-data.tsordering intact.RUN cd packages/producer && bunx tsx scripts/generate-font-data.ts(line 118 in the new layout) still sits below the newCOPY packages/producer/, so producer sources are present when it runs.- Comment carries the invariant. "Keep the ordering dependency-correct: anything the core build reads must be copied above it, and packages nothing above depends on stay below." — this is the guardrail future maintainers need. Good.
Note (not blocking)
Grep of core/package.json shows a core → {lint, parsers, studio-server} dep and parsers → core — a workspace cycle. Bun's monorepo linker handles it (and it's the pre-existing state, not touched by this PR), but if a future PR ever hits a "workspace X missing" error inside the reordered block, the cycle is worth knowing about.
Skipped-build tradeoff
Rejecting the "build once, distribute" alternative on wall-clock grounds (5.6m current vs 6.4m build-first) is the right call given the measurements. Queue time isn't the constraint. Reverting is trivial if that ever changes.
LGTM — clean, well-measured, no blockers.
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 65ad73212a5cafdec53136518d30081c2b688a67.
Verdict
APPROVE — layer split moves the cache boundary to the actual dependency boundary; producer/engine no longer sit above the core build, and the workspace graph confirms nothing above them reads their sources.
Fix verification
Dockerfile.test diff, before/after at head:
- Before (
base@372bead4, lines 93-99): all six package sources copied in one block —parsers,lint,studio-server,core,engine,producer— then the two RUN build layers. Any edit topackages/producer/**(or engine) touched theCOPY packages/producer/layer at line 99, which invalidated the two RUN builds below it. Producer- or engine-only PRs paid a full core rebuild. - After (
head@65ad7321, lines 105-119): source copy split around the build. The first block copiesparsers/lint/studio-server/core(lines 105-108), the two RUN layers execute (111-115), thenengine+producerland after core is already cached (118-119).generate-font-data.ts(122) still runs below the producer copy, so producer sources are present when it fires.ENTRYPOINTunchanged.
Net: identical final image content, one broken cache boundary corrected.
Cache-invariance audit
Walked every mutating layer in the new file at head; each is characterized by what invalidates it:
- L16-46
RUN apt-get …— invalidated by base image or Dockerfile string edits only. - L49-51
ENV …— string-invariant. - L61-64
RUN npx @puppeteer/browsers install chrome-headless-shell@148.0.7778.167— pinned version; invalidated only on Chrome bump. - L69-70
RUN curl … bun-v1.3.13— pinned. - L76
COPY package.json bun.lock ./— invalidates on root manifest / lockfile change. - L77-90
COPY packages/*/package.json …— invalidates only on any workspace manifest change. - L91
RUN bun install --frozen-lockfile— invalidates on any manifest above. - L105-108
COPY packages/{parsers,lint,studio-server,core}/— invalidates on any source change in those four. - L111-112
RUN bun run --filter '@hyperframes/{parsers,lint,studio-server}' build && bun run --cwd packages/core build— invalidates only on parsers/lint/studio-server/core source. - L115
RUN … build:hyperframes-runtime:modular— invalidates only on core source (transitively). - L118
COPY packages/engine/ packages/engine/— engine-only invalidation, and it's now below the core build so it stays cached on engine edits. - L119
COPY packages/producer/ packages/producer/— producer-only invalidation, likewise below core build. - L122
RUN cd packages/producer && bunx tsx scripts/generate-font-data.ts— invalidates on engine or producer source (correctly). - L124-136
WORKDIR+ENTRYPOINT— string-invariant.
No layer above L118 references producer or engine content, so a producer-only or engine-only edit invalidates from L118 down. Fix delivers what the PR body claims.
Cross-package coupling audit
Pulled every packages/{parsers,lint,studio-server,core}/package.json at head:
parsers.deps={@babel/parser, acorn, acorn-walk, linkedom, magic-string, recast}; devDeps include@hyperframes/core.lint.deps={@hyperframes/parsers, htmlparser2, linkedom, postcss, postcss-selector-parser}.studio-server.deps={@hyperframes/core, @hyperframes/parsers, hono, linkedom, postcss, postcss-selector-parser}.core.deps={@chenglou/pretext, @hyperframes/lint, @hyperframes/parsers, @hyperframes/studio-server, bpm-detective, linkedom, postcss}.
Neither @hyperframes/engine nor @hyperframes/producer appears in any of the four manifest dep sets. GitHub code search across packages/{parsers,lint,studio-server,core}/src for @hyperframes/engine and @hyperframes/producer returns two hits — both in packages/parsers/src/{types.ts,outputResolutionCompatibility.ts} — and both are JSDoc references (* Moved from … the compile stage (@hyperframes/producer)), not import statements. No build-time coupling upward from producer/engine into the four "core" packages. The "core cannot depend on producer" premise is verified.
packages/producer/scripts/generate-font-data.ts has no @hyperframes/* imports at head; it's self-contained inside producer. Placing the RUN below the producer COPY is sufficient.
Build-mode parity
Consumers of Dockerfile.test at head, traced via search/code for Dockerfile.test and workflow greps:
.github/workflows/regression.yml(L107file: Dockerfile.test, withload: true) — 8-shard regression suite. Path filter includesDockerfile*, so this PR triggers it..github/workflows/fast-video-validation.yml(L40file: Dockerfile.test,load: true) — fast video validation lane..github/workflows/windows-render.yml— comment reference only (skips producer on Windows because of Docker/Linux tooling); no build.examples/aws-lambda/scripts/smoke.sh(L71 comment): consumes the built image viadocker run, which is layer-agnostic. Not affected by reorder.
The 8-shard regression run at head (30234509212) shows every shard passed, plus the standalone Docker-backed Producer: integration tests, Studio: load smoke, Test: runtime contract, and CLI smoke (required) all green. The reordered image is live-tested end-to-end.
packages/gcp-cloud-run/Dockerfile is a separate file for the beginframe contract path in ci.yml and is not touched.
Fix-internal remaining-silent-invalidation audit
Six-category adversarial checklist, adapted to a build-cache reorder:
- State discard. The two core-build RUN layers write into
packages/core/dist/(and equivalents). The laterCOPY packages/engine/andCOPY packages/producer/target sibling directories (packages/engine/,packages/producer/), so they cannot overwrite the freshly built core dist. No discard. - Return invariant. Final image file set at
WORKDIR /app/packages/produceris byte-identical to the pre-PR image: same COPYs land in aggregate, just partitioned. Smoke fixtures (font-variant-numeric,many-cuts,variables-prod) confirm byte-comparable output per PR body. - Library defaults (bun workspace linker).
bun install --frozen-lockfileat L91 runs before either the old or new source copy — its input is only the manifests plus lockfile, which are unchanged in ordering. Workspace symlinks resolve into initially-emptypackages/engine/andpackages/producer/directories in both orderings; the source populates those directories later either way. No behavior delta. - Precedence / double-COPY. Neither
packages/engine/norpackages/producer/is COPYed twice; they land exactly once (L118, L119), below all core build steps. No precedence conflict. - Session ownership. N/A — no multi-tenant state in a build.
- Discovery completeness. All fourteen
packages/*/package.jsonstill copied abovebun install --frozen-lockfile, so workspace resolution is complete. The four "leaf" packages that never get their source copied at all (player,cli,studio,shader-transitions,aws-lambda,gcp-cloud-run,sdk,sdk-playground) are also unchanged from before — the--filterbuild only names parsers/lint/studio-server/core, so the missing source is intentional. The invariant comment at L93-104 restates this correctly.
No remaining silent-invalidation path.
Standards lens re-run
N/A — this diff is a single Dockerfile. No TypeScript, no ESLint rules, no CONTRIBUTING.md clauses apply. Count: 0 findings across the four TS axes (standards / spec / precision / round-trip) because none apply to build-layer ordering. Noting explicitly per the standing-checklist rule.
Non-blocking observations
- The invariant comment at L93-104 is well-formed and should be preserved on future edits: "anything the core build reads must be copied above it, and packages nothing above depends on stay below." A future contributor adding a build step for
player,cli, orstudiowould need to hoist that package's source COPY into the upper block; the comment names this discipline explicitly. - The repository has no
.dockerignoreat head. Localdist/,node_modules/,.cache/underpackages/engine/orpackages/producer/on a developer's machine would be pulled into the image by the new L118-119 COPYs, but this behavior is identical to the pre-PR layout (same COPYs, just relocated), so it is not a regression — worth mentioning only as ambient context for future hardening. packages/parserslists@hyperframes/coreas a devDep, andcore.depslists@hyperframes/parsers— a workspace cycle. Bun's linker handles it and the pre-existing state is untouched by this PR; flagging only because the reorder tightens the "core packages have no engine/producer deps" claim and the cycle among the four upper packages is orthogonal.
Peer state
- Magi (
miguel-heygen): APPROVED at head — grepped manifests + build scripts, confirmed no engine/producer reads above the reorder, noted the earlier cancelled aggregator failure belongs to a superseded run. - Rames (
james-russo-rames-d-jusso): APPROVED at head with detailed comment — verified via--no-cachebuild reasoning + generate-font-data ordering, noted the same workspace cycle observation, no blockers. - CI at head (
30234509212+30234509206): all 8 regression shards green;Producer: unit tests,Producer: integration tests,Studio: load smoke,Test: runtime contract,CLI smoke (required),Test,Typecheck,Lint,Format,Build,Fallow auditall pass. The reordered image is live-validated by the shards that run inside it.
Stamped.
— Review by Via

What
Reorders
Dockerfile.testso each workspace build runs as soon as its owninputs are present, instead of copying every package first and building
afterwards.
Why
Every package was copied before any build ran, which put
COPY packages/producer/above the core build. Docker invalidates every layerbelow a changed one, so a producer-only change rebuilt core — which cannot
depend on producer.
Measured on CI run 30229469233, those two layers are 86s and 63s of the ~4m
image build. That build runs inside all 8 shards, on every PR, so the waste is
~2.5 min of critical path and ~20 min of CPU per run.
Verified locally rather than assumed. With a warm cache and a one-line change
to
packages/producer/src/regression-harness.ts:Before, the rebuilt layers were:
After, only
COPY packages/producer/re-runs and both build layers reportCACHED. The CI-side saving is larger than 17s because those layers cost86s + 63s there rather than 15.9s + 0.7s.
Scope
This helps PRs that do not touch
packages/{parsers,lint,studio-server,core}.A core change still invalidates everything below it, exactly as before — the
change makes the cache behave correctly, it does not add caching.
What I did not do
The obvious version of this — build the image once in its own job and
distribute it to the shards — makes wall clock worse, so it is not here.
Measured job timeline on a real run:
Critical path today is 1.4m + 4.2m build. A build-once job would run from
+0.4m for 4.2m plus ~1m to push, with shards then pulling ~0.8m — about 6.4m
before the first test versus 5.6m now. It saves ~29 min of CPU, but CPU is not
the constraint and measured queue time is zero.
Test plan
--no-cachebuild succeeds, so the reordered dependency graph isgenuinely satisfiable and not just cache-warm
font-variant-numeric,many-cuts,variables-prod— 3 passed, 0 failedBlast radius
Build-time layer ordering only. Same base image, same pinned
chrome-headless-shell@148.0.7778.167, same installed packages, sameENTRYPOINT. Golden baselines are unaffected — the smoke run above confirms
byte-comparable output.