Skip to content

fix(cli): preload render.js once in renderLocal test suite (Windows CI)#659

Merged
jrusso1020 merged 1 commit intomainfrom
fix/cli-render-windows-cold-import-v2
May 7, 2026
Merged

fix(cli): preload render.js once in renderLocal test suite (Windows CI)#659
jrusso1020 merged 1 commit intomainfrom
fix/cli-render-windows-cold-import-v2

Conversation

@jrusso1020
Copy link
Copy Markdown
Collaborator

What

Hoist the dynamic await import("./render.js") out of every test in
renderLocal browser GPU config and into a beforeAll. Same pattern the
parseVariablesArg and validateVariablesAgainstProject describe blocks
in this file already use.

Why

The Windows render verification workflow has been red on main since the
merge of #642 (auto-detect-browser-gpu) — most recent failure on main:
https://github.com/heygen-com/hyperframes/actions/runs/25470257972/job/74732502915.

The first dynamic await import("./render.js") in this file cold-loads in

5 s on Windows runners, which blows vitest's default 5 s test timeout in
whichever test happens to run it first. Subsequent imports take <10 ms
because the module is cached, so only test #1 ever times out.

The downstream failure was more subtle. When test #1 timed out, vitest
moved on, but its leaked async function eventually hit the synchronous
producer.createRenderJob(...) line and pushed a stale config to
producerState.createdJobs. That push landed AFTER test #2's beforeEach
cleared the array, so test #2's createdJobs[0] was the leaked test #1
entry instead of its own — which is why test #2 reported
browserGpuMode: 'software' when it expected 'auto'. Both failures had
a single cause.

The reason this passed on PR #642's branch but failed once merged: PR #642
was the run that first introduced "forwards browserGpuMode='auto'" as
test #2 in this describe block. On its own branch, the previously-first
test ran without the per-iteration import latency stacking. After merge,
the test ordering plus full-build module graph put it past the 5 s edge
on every push.

How

beforeAll resolves the import once outside any test's timeout window,
then the typed renderLocal / resolveBrowserGpuForCli references are
reused by every it(...) in the block. Cold-load happens once, every
test stays fast, no leaked promise can corrupt state. Behaviour identical
on Linux/macOS — local bun run --cwd packages/cli test is still 22/22
passing in <1 s.

Test plan

  • Unit tests added/updated — restructured the existing tests to share a single import; assertions unchanged
  • Manual testing performed — bun run --cwd packages/cli test → 277/277 passing locally
  • Documentation updated (if applicable)

…dows CI

The first dynamic `await import("./render.js")` cold-load takes >5 s on
Windows runners — long enough to blow vitest's default 5 s timeout in
whichever test ran it first. Subsequent imports are <10 ms because the
module is now cached, so only test #1 ever times out.

The downstream failure is more subtle: when test #1 times out, vitest
moves on, but its leaked async function eventually hits the synchronous
`producer.createRenderJob(...)` line and pushes a stale config to
`producerState.createdJobs`. That push lands AFTER test #2's `beforeEach`
clears the array, so test #2's `createdJobs[0]` is the leaked test #1
entry instead of its own. That's why test #2 saw `browserGpuMode: 'software'`
when it expected `'auto'`.

Hoist the import into `beforeAll` (matching the pattern the existing
`parseVariablesArg` and `validateVariablesAgainstProject` describe blocks
in this file already use). Cold-load happens once outside any test's
timeout window, every test stays fast, no leaked promise can corrupt
state.

Failing run: https://github.com/heygen-com/hyperframes/actions/runs/25470257972/job/74732502915
Started failing on main with the merge of #642 (auto-detect-browser-gpu),
which added the "forwards browserGpuMode='auto'" test as test #2.
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.

Clean fix, well-investigated root cause. The before/after is straightforward: 10 per-test dynamic imports collapse into one beforeAll, matching the pattern parseVariablesArg and validateVariablesAgainstProject already use in this same file. The resolveBrowserGpuForCli test correctly drops async since it no longer awaits an import.

All CI green, including the Windows render and test jobs that were previously failing.

No issues found.

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.

Verdict: LGTM — recommend approval (deferring the stamp to a human). Tightly-scoped fix for a real Windows CI flake, with a diagnosis that goes well past "tests are slow" into the actual leaked-promise / state-corruption mechanism. The fix matches the beforeAll-hoist pattern already used by the parseVariablesArg and validateVariablesAgainstProject describe blocks lower in the same file, so it's consistent with existing convention rather than introducing a new one. CI is green on Render on windows-latest (4m35s) and Tests on windows-latest (4m4s) — i.e. the exact jobs that were red on main post-#642.

Praise

  • The PR description accurately separates the direct failure (test #1 timeout) from the downstream contamination of test #2 via the leaked createRenderJob push landing after the next beforeEach clear. That second-order analysis is what made #642 look like a flaky GPU test rather than a test-infrastructure problem; calling it out plainly in both the description and the inline comment will save the next person.
  • The // Pre-resolve once. … block comment is the right size: explains the Windows-specific cause, the shifting-index failure mode, and why beforeAll is the cure. Future readers won't be tempted to "simplify" it back to per-test imports.
  • One file, +14/-12, no drive-by refactors. Good discipline.

Nits

  • nitpackages/cli/src/commands/render.test.ts:34-37 — the typed let renderLocal / let resolveBrowserGpuForCli declarations are now duplicated for a third describe block in this file (the other two use the same pattern). If a fourth shows up it'd be worth a small loadRenderModule() helper at module scope, but not blocking and arguably premature today.
  • nitpackages/cli/src/commands/render.test.ts:123 — converting it("resolves browser GPU from CLI flags…", () => …) from async to sync is correct (no awaits left in the body) and a nice touch, but worth flagging in the PR body since it's the one behavioural change beyond the hoist.

— Vai

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.

Approve. (Upgrading my prior LGTM-as-comment to a stamp now.)

Targeted fix for the Windows CI red — hoists the cold await import("./render.js") from every it into beforeAll, removing the cold-load spike that blew vitest's 5s timeout. The real second-order failure was the leaked late createRenderJob push landing after the next beforeEach cleared — corrupting test #2's createdJobs[0]. Inline comment captures this for future readers.

Pattern already used twice elsewhere in the same file. Single-file +14/-12, no scope creep. The previously-red Render on windows-latest job is green on this branch. Magi's approval converges.

— Vai

@jrusso1020 jrusso1020 merged commit f385181 into main May 7, 2026
28 checks passed
@jrusso1020 jrusso1020 deleted the fix/cli-render-windows-cold-import-v2 branch May 7, 2026 02:35
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