fix(ci): generated files never read half-written, lint scaling test runner-proof, probe cache test off disk

[miguel-heygen]

Two unrelated pull requests kept failing CI for reasons that had nothing to do with their changes. Both failures are fixed here at the cause, and both come with a check that fails if the fix is removed.

A generated file could be read while it was still being written

The Typecheck job failed with TS1002: Unterminated string literal, pointing at a file the build had generated seconds earlier.

The root build compiles several packages at the same time. More than one of them regenerates files under a package's src/generated while a sibling package's typechecker is already reading them. Writing a file is not one action: it empties the file first and then refills it, so for a few milliseconds what is on disk is the first half of the new file. Whichever build happened to read it in that gap got a truncated file and stopped. It never reproduced locally, because locally nothing else is reading at that instant.

Every generator now publishes instead of overwriting. It writes the new version under a temporary name in the same folder and then renames it over the target, which the filesystem does in a single step. A reader gets either the whole previous version or the whole new one; there is no moment where it can see half of either. Content that has not changed is not written at all, so a repeat build no longer touches these files or their timestamps.

Four generators write into a src/generated folder. All four go through one shared helper now, so the rule has a single owner rather than four copies that can drift:

Generator	Produces
packages/core/scripts/build-audio-fx-runtime.ts	audio-FX runtime module
packages/core/scripts/build-position-edits-render.ts	position-edits render module
packages/core/scripts/build-hyperframes-runtime-artifact.ts	runtime module + its three build outputs
packages/producer/scripts/build-hf-early-stub.ts	early-stub module

The last one was not in the original report. It is a copy of the third, says so in its own header, and had the identical defect plus a second one: it reformatted the file after publishing it, which reopened the same window a moment later. Formatting now happens before publication.

The build also built the core package twice, once on its own and then again in parallel with the packages that read its output. The second build was already redundant. Removing it takes those writers out of the window entirely and makes the build shorter. Renaming is what makes the file safe; this just means fewer writers are racing in the first place.

A performance test was measuring the CI runner, not the code

A test guarding a linter scan against a known slow-scanning bug failed on four of the last ten failed runs on main. It timed the scan with a stopwatch and required the result to come in under two seconds. One run took 3.7s.

A stopwatch on a shared runner also counts the time this process spent waiting while the machine ran someone else's job, so an absolute limit in milliseconds is a claim about how busy the runner was, not about the code. The test now counts only the processor time this process actually used, and checks that the cost grows in step with the input rather than against a fixed number of milliseconds.

Measured on a machine at load average 16, for the same input: the stopwatch ratio reached 45x while the processor-time ratio stayed at 20x. The slow-scanning bug it guards against measures 60x or more, so the check is still tight enough to catch it.

Proving the checks are real

Each fix was verified by putting the defect back and watching the check fail.

Removing the rename and the unchanged-content skip:

not ok - leaves the target untouched when the content is byte-identical
  Expected values to be strictly equal:  true !== false
not ok - replaces the target rather than rewriting it in place
  Expected "actual" to be strictly unequal to:  290227027

Reintroducing the slow scan:

FAIL  stripJsStringLiterals scaling > stays linear in slash-dense input
AssertionError: expected 72.61645364536454 to be less than 32

Checks run

Root build, workspace typecheck, script typecheck, root lint, format check, script tests (192 passing), and the linter package suite (33 passing) all pass. Building the core and producer packages twice in a row leaves every generated file untouched the second time.

---

Part 3: keep the probe cache bound test off the filesystem

Why

probeMediaProfile > bounds the process-scoped probe cache is a recurring failure on the Windows test lane. It shows up on unrelated pull requests and on main, always as a 5s timeout. In a normal run the test takes ~300ms on Windows; on the failing runs it took over 5s while neighbouring tests in the same file only slowed about 2×.

The test created 129 real files in the temp dir, stat'ed each through the code under test, then deleted them, to check that the 129th insert evicts the oldest entry. That rule is in-memory. The file churn made the test's runtime a function of filesystem contention on the runner, not of the code.

What changed

• The test synthesises statSync results (identity is dev:ino:size:mtime:ctime) instead of writing files, so it never touches disk. Locally it drops from ~115ms to ~35ms; on Windows it no longer scales with disk load.
• The test now also covers the LRU touch: a hit before the bound is reached keeps that entry resident and the next-oldest is evicted instead. The old shape never produced a cache hit during the fill, so removing the touch left it green.

Verification

• ffprobe.test.ts: 115 passed.
• Raising the bound to 129 in ffprobe.ts fails the test (expected 130, got 129).
• Removing the LRU re-insert on hit fails the test (expected 129, got 130).
• oxfmt, oxlint and tsc --noEmit clean for the engine package.

No product code changes.

[terencecho]

APPROVE — CI-fix PR is a genuine root-cause fix across four independent axes with no coverage reduction, no runtime touch, and no symptom-masking. All checks so far green.

Head + trust. 6e7c68aa5276fc358409f94b532baaa680c11f45, 4 commits all by miguel-heygen. 8 files, +222/-83. mergeable=MERGEABLE, mergeStateStatus=BLOCKED = missing approval only.

1. Atomic write pattern. packages/core/scripts/writeGeneratedFile.ts:39-49

• Temp path: ${outPath}.${process.pid}.${randomBytes(4).toString("hex")}.tmp — pid + 32 bits of entropy, safe against concurrent writers to the same target.
• writeFileSync(tempPath) → renameSync(tempPath, outPath) — POSIX-atomic, same-directory-by-construction ensures single filesystem so no cross-fs degradation.
• finally { rmSync(tempPath, {force: true}) } — cleans up on failure; no-op on successful rename. Orphaned temps only survive a hard SIGKILL between write and finally.
• Non-.ts suffix (writeGeneratedFile.ts:41-42) — deliberate so tsc in same package's src glob won't try to compile in-flight temp files.
• Byte-identity skip (writeGeneratedFile.ts:36) — no-op rebuilds don't touch mtime, so downstream mtime-keyed invalidation stops firing spuriously.
• Windows: renameSync maps to MoveFileExW with MOVEFILE_REPLACE_EXISTING (Node 10+); can still throw EBUSY if a reader holds the target, but that's strictly no worse than the pre-fix writeFileSync. Actual reported failure was Linux truncation.

2. Root build no longer rebuilds core concurrently. package.json:14

• Diff drops core, from the second parallel filter: '@hyperframes/{core,engine,producer,player,studio,shader-transitions,aws-lambda,gcp-cloud-run,sdk}' → '@hyperframes/{engine,producer,player,studio,shader-transitions,aws-lambda,gcp-cloud-run,sdk}'. Core is now built ONCE (the earlier bun run --filter @hyperframes/core build) before its readers run in the second batch.
• Pre-fix, core got rebuilt IN PARALLEL WITH its readers — that's the race window for TS1002: Unterminated string literal. Removing the redundant build closes the trigger.
• Correct layering: the atomic-rename fix is the load-bearing durable property (reader-always-sees-old-or-new-never-prefix); the ordering change is trigger removal. Defense-in-depth on the remaining producer/reader pair (cloud-run typecheck reaching core through producer path mapping) survives future concurrent-writer surfaces.
• No wall-clock trade-off — pure dead-weight removal.

3. Lint scaling test: wall-clock → CPU ratio. packages/lint/src/utils.test.ts:87-116

• Old: performance.now() + absolute expect(large).toBeLessThan(2_000) — this was the flake source (wall clock counts descheduled time).
• New: process.cpuUsage() counting user+system μs → ms; only the ratio is asserted; absolute bound removed.
• Sample sizes reduced from 20k/160k → 10k/80k. Comment utils.test.ts:97-98 explains: above ~100k the output string's own growth dominates and even a linear scan measures ~20x for 8x input; below 100k, 8x input measures ~8x for linear, 60x+ for the quadratic bug being guarded.
• Warmup call stripJsStringLiterals(src) before samples eliminates JIT-comp noise; 5 runs (up from 3) with Math.min for best-of.
• .toBeLessThan(24) on ratio at 8x input: quadratic would be 64x, so the assertion has ~2.5× headroom against a real regression while giving ~3x/doubling slack for CI noise. Explicit { timeout: 30_000 } catches a truly pathological runner without silent hang.
• Symmetry protection: if the scanner grows features, both samples run the CURRENT scanner, so any shape-preserving change shifts both sides together and the ratio is invariant. Only a shape regression (linear → quadratic) moves it.
• Removal-of-fix proof in PR body cites 72.6 !< 32 — stale (current code uses 24), but 72 > 24 too, so the demonstration still holds; if anything the current threshold is stricter.

4. Probe cache test off disk. packages/engine/src/utils/ffprobe.test.ts:253-292

• Old: 129 real temp files created / stat'd / deleted — file churn dominated runtime and blew the 5s timeout on contended Windows.
• New: synthetic string paths (/probe-lru/asset-N, never touched), vi.doMock("fs", ...) returns a synthetic statSync that fabricates the identity tuple (dev, ino, size, mtimeNs, ctimeNs). Cache identity IS this tuple, so the mock exercises the exact same identity contract without disk I/O.
• Coverage actually improved: new test asserts LRU-touch behavior (ffprobe.test.ts:282-289) — hit refreshes entry, next-oldest evicted instead of first-in. Old test never produced a cache hit during the fill, so the LRU-touch branch was untested.
• Removal-of-fix proofs (PR body): raising bound to 129 fails (expected 130 got 129); removing LRU re-insert on hit fails (expected 129 got 130). Both assertions bind meaningfully.
• No scope reduction — the disk-touch was never load-bearing; it was just how statSync got a value to return.

5. Symptom vs. root cause. All four are root fixes: atomic-rename fixes the actual TOCTOU race; removing the redundant core build removes writers from the race entirely; CPU time is the mathematically correct measurement (scheduling-invariant); stat mocking is legitimate because cache identity IS the stat tuple. No .skip(), no timeout inflation as the primary lever, no retries added.

6. Blast radius. Zero runtime touch. All changed code paths are build-time or test-time:

• package.json — build script.
• packages/core/scripts/{build-hyperframes-runtime-artifact,buildInjectedArtifact,writeGeneratedFile}.ts — build-time generators.
• packages/core/scripts/writeGeneratedFile.test.ts — NEW, wired into test:scripts in package.json.
• packages/engine/src/utils/ffprobe.test.ts, packages/lint/src/utils.test.ts — test-only.
• packages/producer/scripts/build-hf-early-stub.ts — build-time.

Output CONTENT of generators is byte-for-byte identical; only HOW it's published changed. Runtime is unaffected.

Second commit (17728e4d) is a logistical fix: helper lives in packages/core/scripts/ (not scripts/ at root) so container image copy of packages/** picks it up. Producer imports it via ../../core/scripts/writeGeneratedFile.js — cross-package script import, precedent noted in commit message.

formatGeneratedSource (writeGeneratedFile.ts:57-64) formats via bun x oxfmt --stdin-filepath=... on stdin BEFORE atomic publication (closes the second window Miguel found in the early-stub generator, which previously formatted AFTER publication and reopened the race).

7. CI. Green so far: Preflight (lint + format) on all four detect-changes lanes, Lint, Format, Fallow audit, Test: skills + skill bootstrap (Windows, Node 24), SDK: unit + contract + smoke, Skills: manifest in sync + project-native lint + mirror, Codex plugin package, CLI: npx shim (ubuntu / macos), Semantic PR title, File size check, CodeQL (actions + python). In progress: Build, Typecheck, Producer: unit + integration, Test: runtime contract, Studio: load smoke + timeline viewport, CLI smoke, CLI: npx shim (windows), all regression-shards (1..9), Windows render, Perf: {load, fps, scrub, drift, parity}, Preview parity, CodeQL (js/ts). No failures.

8. Concurrent review. Only github-advanced-security[bot] reviews present (2 empty COMMENTED, one at each intermediate commit; both auto-cleared as no findings). No CHANGES_REQUESTED, no APPROVE from another peer.

— tai

[terencecho]

APPROVE at c212e6f7b24816a09c8121187b8aad2c8bd83cd4 — re-stamp after force-push. Delta since 6e7c68aa… is test-only (writeGeneratedFile.test.ts, +17/-2, snapshot() helper that reads identity through openSync+fstatSync+closeSync instead of path statSync). Defeats CodeQL's js/file-system-race check-then-use pattern by making the identity read go through a descriptor, not a path.

Assertions unchanged (.mtimeMs and .ino accessors identical). Two call sites migrated. Helper (writeGeneratedFile.ts) and build script (package.json) untouched — all axes from prior review (6e7c68aa… at review 5162115389) still hold.

— tai

[vanceingalls]

Reviewed at head 6e7c68aa5, in an isolated worktree (/tmp/hf-3834-review), against the actual source rather than the diff summary. Verified the three fixes empirically, not just by reading them.

Verification performed

1. Atomic writes (packages/core/scripts/writeGeneratedFile.ts)

• Confirmed the previous code path (buildInjectedArtifact.ts, build-hyperframes-runtime-artifact.ts, build-hf-early-stub.ts on main) used plain writeFileSync straight to the final path, with build-hf-early-stub.ts additionally reformatting after publish (a second race window) — exactly as the PR body describes.
• Reproduced the original bug directly: a writer doing plain writeFileSync in a loop against a 2MB target while a concurrent reader polled it produced 4 corrupt reads out of 683 (including zero-length reads and truncated content) in a few seconds.
• Ran the same harness against the new writeGeneratedFile (temp file in the same directory + renameSync): 0 corrupt reads out of 2771, repeated. Same-directory temp path is confirmed in the code (${outPath}.${pid}.${rand}.tmp), so the rename is guaranteed atomic (same filesystem).
• Confirmed all four generators actually route through the shared helper: build-audio-fx-runtime.ts and build-position-edits-render.ts both call buildInjectedArtifact(), which now calls writeGeneratedFile/formatGeneratedSource — so the PR body's "four generators" claim holds even though only two call sites plus the shared module needed direct edits.
• Idempotency / tracked-artifact safety: built @hyperframes/core twice back-to-back — zero bytes/mtimes changed on the second build (find … -printf diff was empty). Ran build-hf-early-stub.ts twice — identical md5 and unchanged mtime, confirming the new stdin-based oxfmt formatting is deterministic and the byte-identical skip in writeGeneratedFile.ts:34 actually fires with the new formatting path (this was the one place the fix could have quietly regressed: if stdin-mode oxfmt formatted differently from in-place oxfmt, the skip would never trigger and generated files would drift from what's committed — it doesn't).

2. Redundant core build removal (package.json)

• Confirmed via diff: build used to run core standalone, then include core again in the parallel filter group with engine,producer,... — i.e., core really was being rebuilt a second time concurrently with its own readers. That's now removed.
• This is fragile-but-working, not structurally enforced: nothing (no test, no lint rule) stops someone from re-adding core to that filter string later. However, since the atomic-rename fix in (1) is what actually closes the race, a future regression here would just reintroduce redundant work, not reintroduce the truncated-read bug — so this is a real but low-severity gap, not a blocker.

3. Lint scaling test (packages/lint/src/utils.test.ts)

• Confirmed the change: wall-clock performance.now() + absolute <2000ms bound → process.cpuUsage() ratio bound (10k vs 80k chars, <24), with an explicit 30s test timeout.
• Ran it 5x locally: consistent pass, ~400-440ms each.
• Verified test power directly: injected a genuine O(n²) scan into stripJsStringLiterals (temporarily, reverted after) and reran — failed with a measured ratio of ~79 against the <24 bound, consistent with the PR's own quoted numbers. The bound has real margin between a linear-with-overhead scan (~8-20x) and a quadratic one (60x+).

4. Probe cache test off disk (packages/engine/src/utils/ffprobe.test.ts)

• Confirmed: previously wrote/statted/deleted 129 real temp files; now mocks fs.statSync to synthesize dev:ino:size:mtimeNs:ctimeNs and never touches disk.
• Confirmed it's more thorough than before, not less: the old test only checked that inserting a 130th entry evicted the first one. The new test also exercises the LRU touch — a hit on an entry before the bound is reached keeps it resident and the next-oldest is evicted instead — which the old shape never exercised (it never produced a mid-fill cache hit).
• Regression-risk check: the sibling test right above it ("deduplicates probes within one cancellation scope…", line 222) still writes real files and uses real statSync, so real disk-backed identity derivation is still covered elsewhere — mocking statSync in this one test doesn't create a blind spot for whether the real disk-backed path works.
• Ran it 6x locally (including a fresh @hyperframes/core+parsers build needed to resolve the workspace deps): consistent pass, ~120-170ms each.
• Nit: the mock hardcodes the stat shape as (filePath) => {...} and ignores any options argument; if the production code ever switches from mtimeNs/ctimeNs to mtimeMs, the mock would silently return undefined for those fields and the cache-invalidation-on-modification behavior would stop being exercised without failing. Not a regression from this PR — the old test didn't cover file-modification invalidation either — but worth a one-line comment if anyone touches this later.

5. Scope

File list is exactly: package.json, the write-path in the three generators, the new helper + its test, and the two flaky test files. No unrelated product code. oxfmt/oxlint/tsc --noEmit all clean locally for the touched packages; scripts typecheck and the full test:scripts suite (192/192) and packages/lint (33/33) pass locally.

Other notes

• The two github-advanced-security[bot] CodeQL comments on writeGeneratedFile.test.ts:57 ("Potential file system race condition") are false positives — that's a single-threaded Node test asserting on its own synchronous writeGeneratedFile → readFileSync sequence, with no concurrent writer. Worth dismissing explicitly on the alert so it doesn't linger.
• Nothing overengineered here — the shared helper is small, and consolidating four near-duplicate write sites into one is the right call given two of them already had divergent defects.

Solid, well-verified fix with real reproductions behind each claim. The one structural gap (build-ordering enforcement) is real but non-blocking given the atomic rename is the actual guarantee.

— Vai

[vanceingalls]

Re-verified at the new head c212e6f7b (moved from 6e7c68aa5 since my last review).

Head move: confirmed test-only

git diff 6e7c68aa5...c212e6f7b touches exactly one file: packages/core/scripts/writeGeneratedFile.test.ts (+17/-2). No change to writeGeneratedFile.ts, the three build scripts, package.json, or either flaky test file — all byte-identical to what I already verified. The change swaps the "before" snapshot in two assertions from statSync(path) to a snapshot() helper that opens an fd, fstatSyncs it, and closes it — decoupling the baseline read from a path-based check-then-use pattern, which is specifically what CodeQL's js/file-system-race query pattern-matches on. Ran the 4 tests 3x locally, still 4/4 pass. Confirmed on GitHub: CodeQL is now completed / success at this head (previously the two bot comments were open against the prior commits). This is a real (if narrow) fix to the query match, not just a suppression — the assertions and semantics are unchanged.

Via's three angles, checked against source (not just Miguel's description)

1. Atomic-write portability / Windows

• Temp path is ${outPath}.${pid}.${randomHex}.tmp, written via dirname(outPath) (same dir as target) — confirmed same-filesystem-by-construction, matching Miguel's description exactly.
• Windows CI: the root bun run build (the job that had the original TS1002 concurrent-build failure) only ever runs on ubuntu-latest (checked .github/workflows/ci.yml — build, typecheck, smoke-global-install, cli-smoke-required are all ubuntu-latest). However, windows-render.yml independently runs bun run --cwd packages/core build on windows-latest in both render-windows and test-windows-lane, which does exercise the atomic temp+rename path for real on Windows. Both are green at this head (Render on windows-latest, Tests on windows-latest: studio-core, Tests on windows-latest: studio-engine-cli all completed/success).
• Net: rename-over-existing-file mechanics are verified to work on Windows (Node's renameSync uses MoveFileExW with replace-existing). What is not verified on Windows is the concurrent scenario — two processes racing to write/read the same generated file at once — because the Windows jobs build core sequentially, not concurrently with a reader the way the Linux build job does. Worth stating plainly: atomicity-of-rename is cross-platform proven, but the original race repro is Linux-only (which matches where it was originally observed).

2. Lint scaling test tolerance

• Confirmed exactly: 10_000 vs 80_000 chars, bound <24, { timeout: 30_000 }, comment states the ~100k boundary and the 8x/60x+ figures — matches Miguel's description verbatim.
• Instrumented the real test (temporarily, reverted) to print the actual ratio across 8 runs on this box: 5.3–8.6, consistently well under 24. That's real margin (~3-4x headroom to the bound), not a tolerance picked to barely pass once.
• One genuine nuance Miguel didn't mention: the reasoning "ratio is stable because both samples run the same code path" is correct for guarding against clearly-quadratic regressions, but the <24 bound is loose enough that a mildly superlinear regression (e.g. an accidental O(n log n) or O(n^1.3) path) would likely still pass — 8^1.3 ≈ 14.3, 8^1.5 ≈ 22.6, both under 24. The test reliably catches true O(n²) (which is the documented, actual historical bug) but isn't a general-purpose complexity regression detector. Nit, not a blocker — that's not what this test was ever built to catch.

3. Producer/reader concurrency scope

• Verified Miguel's claim in the actual build graph: packages/producer/package.json's build script runs ... && bun run --cwd ../.. build:hyperframes-runtime:modular && node build.mjs, and root's build:hyperframes-runtime:modular maps to SANDBOX_RUNTIME_VARIANT=modular tsx scripts/build-hyperframes-runtime-artifact.ts inside packages/core — i.e., the same generator this PR fixed, re-invoked from inside producer's own build step. (Note: SANDBOX_RUNTIME_VARIANT isn't actually read anywhere in the codebase — it's currently a no-op env var, so this re-invocation writes to the exact same output paths as core's own regular build.)
• Since producer runs concurrently with engine/player/etc. in the top-level --filter '@hyperframes/{engine,producer,...}' build group, this is a live, still-present concurrent writer against core's generated files, and it was not addressed by the ordering fix (which only removed the redundant listing of core itself in that same filter group) — it's a different trigger for the same class of race.
• This confirms Miguel's claim precisely: the atomic rename, not the ordering change, is the thing making this safe. I'd already stress-tested the atomic-rename mechanism directly (0/2771 corrupt reads under a tight writer/reader loop, vs. 4/683 corrupt reads with the old plain-writeFileSync code) — that test is agnostic to why two writers/a writer-and-reader end up concurrent, so it already covers this producer/core/engine pattern too. Reverting the ordering change would not reintroduce the truncated-read bug; only the atomic rename load-bears here. This is a stronger and more precise statement than what I wrote in my first review (I'd called the ordering fix "fragile but non-blocking defense-in-depth" — it's more accurate to say the ordering fix is a minor cleanup/perf win, and the atomic rename was always the complete, sufficient fix for this whole class of problem, independent of build ordering).
• Answering Via's actual question: this is not narrowly scoped to a single instance that happened to flake visibly — there's at least one other structurally-identical concurrent pair in the graph today, and it's fine specifically because the fix is general (a write-primitive change), not because of anything specific to the one reported pair.

Updated verdict

Everything above is additive confirmation, no new blockers. Approving again at this head.

— Vai