fix(build): build Core once before dependents - #3588
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed the full root build graph and both packages' build scripts, not just the two-file diff.
Strengths
- The diagnosis of the duplicate Core build is correct and independently verifiable:
packages/core/package.json'sbuildbegins withbuild:hyperframes-runtime, which istsx scripts/build-hyperframes-runtime-artifact.ts, and that script does rewritesrc/generated/runtime-inline.ts. Core in the parallel group was a genuine concurrent writer, and removing it is the right minimal edit. - Choosing a static structural assertion over a threaded race test (
scripts/publish-workflow.test.mjs:70) is the correct call. A free-running concurrency test here would be textually identical to a real one and would prove nothing, and there is no way to force the interleave of two build subprocesses. - Splitting this out of the release rather than blocking 0.8.23 was right, and a 2-file
+23/-1diff on a release-critical script is appropriately scoped.
Blocker — the race is not removed; the writer count went from two to one
@hyperframes/producer is still in the parallel dependent group, and it rewrites the exact file this race is about:
packages/producer/package.json→build:... && bun run --cwd ../.. build:hyperframes-runtime:modular && node build.mjs- root
package.json→build:hyperframes-runtime:modular:bun run --filter @hyperframes/core build:hyperframes-runtime:modular packages/core/package.json→build:hyperframes-runtime:modular:SANDBOX_RUNTIME_VARIANT=modular tsx scripts/build-hyperframes-runtime-artifact.tspackages/core/scripts/build-hyperframes-runtime-artifact.tsnever readsSANDBOX_RUNTIME_VARIANT. It writes a fixed path,resolve(generatedDir, "runtime-inline.ts")(:60), through a plain non-atomicwriteFileSync(:62).
So the :modular variant writes the same packages/core/src/generated/runtime-inline.ts as Core's own build. Since producer and gcp-cloud-run remain in the same brace group, producer's rewrite still runs concurrently with gcp-cloud-run's declaration emit reading that file, which is precisely the TS2306 ... is not a module from the failed publish run. The PR body's "so the race is permanently removed" is not achieved by this diff.
Important
scripts/publish-workflow.test.mjs:76— the message "Core must be built exactly once" overclaims what the assertion checks.stages.filter((stage) => stage === coreStage)is an exact-string count, so it cannot see Core re-added inside a brace group, and thedoesNotMatchguard below it inspects onlystages[coreIndex + 1]. Re-adding{core,...}to any later stage leaves the test green. It guards the specific regression that just happened, not the invariant it states.packages/gcp-cloud-run/package.jsondeclares"@hyperframes/producer": "workspace:^", yet both build in the same parallel brace group, so a package can build concurrently with its own dependency. That is an ordering defect independent of the generated-file race.- Suggested root-cause fix, cheaper than enumerating writers: make the generator write atomically (write to a temp file, then
rename).renameis atomic within a filesystem, so a concurrent reader sees either the old or the new file and never a truncated one. That closes the window for every writer and every ordering, including future ones this test cannot anticipate.
Nit
/@hyperframes\/\{[^}]*\bcore\b[^}]*\}/would also match a hypotheticalcore-utils, since-is a word boundary. No such package exists today.
On CI: at ebc485c5c the required contexts are green except Tests on windows-latest, which is absent rather than failing (summary context, created after its matrix). Worth stating plainly: a green Build here is not evidence the race is fixed. Run 33540883501 failed and then passed on re-run at the identical commit, so this build graph is known to pass while still racy.
Verdict: REQUEST CHANGES
Reasoning: The Core dedupe is correct but incomplete. producer remains in the parallel group and still rewrites core/src/generated/runtime-inline.ts via the :modular script, whose variant flag is inert with respect to the output path, so the documented failure mode survives this fix.
— Rames Jusso
- publish runtime artifacts atomically - build GCP Cloud Run after Producer - strengthen the build-order contract
Addressed in |
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at a21972818d8a2955f3e6ac2f939bd4ad389a1dcf. All three blockers from my earlier review are resolved, verified by reading the code at this head rather than the summary. No blockers remain.
Strengths
packages/core/scripts/atomic-write-file.ts:10-13— the temp file is placed indirname(destination). That is the property that makesrenameSyncatomic; atmpdir()temp would have silently degraded into a cross-device copy and reintroduced the exact window this is meant to close.process.pid+randomUUID()also makes two concurrent writers collision-free rather than merely unlikely to collide.packages/core/scripts/atomic-write-file.test.ts:52,56— the failure test pins the two load-bearing invariants instead of assuming them:dirname(firstFailure.path) === directorypins same-directory placement, andnotEqual(firstFailure.path, secondFailure.path)pins temp uniqueness across calls. Testing the helper's invariants is a fair substitute for reproducing the partial read.- The fix is the class, not the instance: all four write sites are converted (
build-hyperframes-runtime-artifact.ts:51-53,63) and no barewriteFileSyncremains in the generator. That closes the window for every writer and ordering, including ones a static test cannot anticipate. scripts/publish-workflow.test.mjs:70-93parses every stage into package sets, so Core re-added to any later group is caught. My earlier objection — that an exact-string count could not see{core,...}inside a brace group — is fully addressed, and the assertion messages no longer claim more than they check.
Blockers — all resolved
- The race survived the dedupe. Closed at the root:
atomicWriteFileSyncat all four sites. Producer still reaches the generator viabun run --cwd ../.. build:hyperframes-runtime:modular, so it remains a writer inside the stage-2 parallel group — atomicity, not ordering, is what makes that safe now, which is the more durable of the two. gcp-cloud-runbuilt concurrently with its own dependency.gcp-cloud-runnow has its own stage after Producer's, and the test pinsproducerIndex < cloudRunIndex.- The "built exactly once" overclaim. Replaced with per-stage parsing as above.
The Fallow audit finding on isFileSystemError (CRAP 56.0 against a 30.0 threshold) is also gone as of a21972818. Dropping the guard for assert.ok(error instanceof Error) plus a cast trades a precise guard failure for a slightly less precise assertion failure if the error shape ever changes, but the three equality assertions that follow still catch it — a fair trade to clear the complexity gate in a test helper.
Important (follow-ups, not gating)
SANDBOX_RUNTIME_VARIANTis still inert. Producer invokesbuild:hyperframes-runtime:modular, but the generator never reads that variable at this head, so Producer regenerates artifacts byte-identical to the ones Core produced one stage earlier. Either honor the flag or drop Producer's regeneration step — that removes the last concurrent writer outright and saves a redundant esbuild pass. Pre-existing, worth its own change rather than this PR.- Nothing pins that the generator actually uses the helper. The atomic tests would stay green if a call site regressed to plain
writeFileSync. A one-line structural assertion that the generator source contains no barewriteFileSync(would close that, in the same style as the build-order test.
Nits
scripts/publish-workflow.test.mjs:72— the per-stage.match()is non-global, so a stage carrying two--filterarguments would register only the first. Fine for today's script; brittle if a stage ever gains a second filter.- Same test: Core is only checked in stages after its first appearance, so Core alone inside stage 0's brace group would pass. Hypothetical, not worth code.
Notes on CI
- At this head there is a single clean wave (created 18:39:49) with no failures so far; the required contexts were still in flight as I wrote this.
- The
Testandregressionfailures visible on the previous head were not code failures, and nothing needed repairing for them. Both belonged to the 18:34:59 wave that the concurrency group cancelled when a second wave started seventeen seconds later, and both are summary jobs reporting on cancelled dependencies — 5-6 second durations, annotatedProducer unit/integration tests did not succeed. - Windows is the usual trap for this pattern, since
renameSyncover a file another process holds open can throw there. Not reachable here: everybun run buildinci.ymlruns onubuntu-latest, andwindows-render.yml:460explicitly skips the publish build. - Green CI would still not prove a race fixed — a run earlier in this release cycle failed and then passed at an identical commit. The atomic rename is what makes ordering no longer load-bearing, and that is the right reason to trust this fix.
One procedural note: my earlier changes-request is still recorded against the first head, and a comment review cannot clear it, so this PR stays blocked until a maintainer either dismisses that review or approves. Flagging so it does not sit waiting on me.
Verdict: COMMENT
Reasoning: Every blocker is resolved at this exact head and the root-cause fix is the right shape; what remains is two follow-ups and two nits, none of which gate the merge, with required checks still running.
— Rames Jusso
Summary
What I measured
The first v0.8.23 publish attempt failed in
@hyperframes/gcp-cloud-rundeclaration emit withTS2306: runtime-inline.ts is not a module; the identical commit passed on rerun. The original root graph had two concurrent writers: Core itself and Producer'sbuild:hyperframes-runtime:modular. This patch closes the read-mid-write window for all four generated artifacts and orders the explicit Producer consumer edge before GCP Cloud Run.Verification at
7de506671:bun run test:scripts— 191 Node tests + 42 catalog tests passedbun run --cwd packages/core typecheck— source + runtime typechecks passedbun run build:hyperframes-runtime— generator completed with no source drift or leftover temp filesoxfmt,oxlint, andgit diff --check— passedWhat I did NOT exercise
A full local monorepo build was not usable as evidence because this host's Bun package cache contains incomplete
tsup/oxfmtpayloads. Exact-head CI is the build authority for the complete graph.