Skip to content

Build tsjs bundles into a private OUT_DIR and validate the set - #1214

Open
dhruv8sh wants to merge 3 commits into
mainfrom
fix/tsjs-build-race
Open

dhruv8sh wants to merge 3 commits into
mainfrom
fix/tsjs-build-race

Conversation

@dhruv8sh

@dhruv8sh dhruv8sh commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Every cargo build shared crates/trusted-server-js/dist, which build-all.mjs deletes and rewrites. Overlapping builds (dev + release in one target dir, two target dirs, or a manual npm run build) could exit 0 having embedded a partial or empty bundle set, including no core. The build script now builds into a private OUT_DIR/tsjs-dist and never reads dist.
  • build.rs derives the expected module set the same way build-all.mjs does (core + every lib/src/integrations/<id>/index.ts) and fails on any missing, empty or unexpected bundle.
  • The silent fallbacks to a stale dist are gone: TSJS_SKIP_BUILD and a missing npm fail with instructions, and the new TSJS_PREBUILT_DIR embeds prebuilt bundles after the same check.

Changes

File Change
crates/trusted-server-js/lib/build-all.mjs Accept --out-dir <dir>; default stays ../dist, so npm run build, Playwright and CI are unchanged
crates/trusted-server-js/build/bundle_set.rs New std-only module: expected-set discovery, bundle dir scan, and check_bundle_set (missing / empty / unexpected), with unit tests
crates/trusted-server-js/build.rs Build into OUT_DIR/tsjs-dist and validate before codegen; TSJS_PREBUILT_DIR path; TSJS_SKIP_BUILD and missing npm fail with instructions; stale node_modules (hidden lockfile older than package-lock.json) fails instead of reinstalling; npm ci on a missing node_modules serialized with a file lock; TSJS_TEST failures fail the build; rerun-if-env-changed for every variable read; rerun-if-changed narrowed from ~34k paths to the sources and lockfiles
crates/trusted-server-js/src/lib.rs Include bundle_set.rs under #[cfg(test)] so its tests run with the crate
crates/trusted-server-js/lib/.gitignore Ignore the npm ci lock file
scripts/template-cache-local-test.sh Look for the GPT bundle under the new out/tsjs-dist/ path
docs/guide/error-reference.md Replace the TSJS_SKIP_BUILD tip with TSJS_PREBUILT_DIR; document each new build-script error
crates/trusted-server-js/README.md, docs/guide/creative-processing.md Note that cargo builds into OUT_DIR and dist is only written by npm run build

Behavior changes

  • TSJS_SKIP_BUILD=1 now fails; use TSJS_PREBUILT_DIR=<dir with tsjs-*.js>.
  • After a change to package-lock.json (for example switching branches), the build asks for npm ci instead of building against out-of-date dependencies.
  • A missing node_modules is still installed automatically (the Axum, Cloudflare, Spin and clippy CI jobs rely on this), but a failed npm ci now fails the build.

Coordination

#1180 and #855 also touch the code build.rs generates. This PR changes only the include_str! path in that output (/tsjs-dist/tsjs-<id>.js), so rebasing either should be small.

Closes

Closes #1200

Test plan

  • cargo test-fastly && cargo test-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run
  • JS format: cd crates/trusted-server-js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: race reproduction from Concurrent cargo builds race on the shared tsjs dist and can embed a partial bundle set #1200, run against main and this branch on the same machine
Scenario main This branch
Dev + release build overlapping, delays 0.1–0.6 s, both orders 13/40 bad (exit 0 with 2–12 modules, or "no tsjs-*.js files found") 0/80
Two dev builds in separate target dirs, delays 0.2–0.6 s 9/15 bad 0/15
Cargo builds while npm run build loops on dist 4/15 bad 0/15
Two builds with node_modules missing, second started mid-npm ci — 3/3 pass, one npm ci

Also checked by hand: TSJS_SKIP_BUILD=1, no npm on PATH, stale node_modules, and a TSJS_PREBUILT_DIR with one missing and one empty bundle each fail with the documented message; a valid TSJS_PREBUILT_DIR builds; a no-change rebuild stays fresh.

Checklist

  • Changes follow AGENTS.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing macros (not println!)
  • New code has tests
  • No secrets or credentials committed

The build script shared crates/trusted-server-js/dist with every other cargo build and with manual npm runs, so overlapping builds could embed a partial or empty bundle set and still exit 0. build-all.mjs now accepts --out-dir, and build.rs builds into OUT_DIR/tsjs-dist and fails unless it holds exactly core plus every lib/src/integrations/<id>/index.ts, each non-empty.

Silent reuse of dist is gone: TSJS_SKIP_BUILD and a missing npm now fail with instructions, and TSJS_PREBUILT_DIR embeds prebuilt bundles after the same check. Stale node_modules fails instead of reinstalling, npm ci on a missing node_modules is serialized with a file lock, TSJS_TEST failures fail the build, rerun-if-env-changed covers every variable read, and rerun-if-changed is narrowed to the sources.

Fixes #1200

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
@dhruv8sh dhruv8sh self-assigned this Sep 25, 2026
Cargo reruns a build script on every invocation while a watched path is missing, so always watching lib/node_modules/.package-lock.json made every build rerun under TSJS_PREBUILT_DIR without node_modules. Watch it only on the npm build path, after the freshness check has confirmed it exists.

Watch lib/test and lib/vitest.config.ts when TSJS_TEST=1 so edited tests rerun, and note in the error reference that the timestamp-based freshness check also fires when a checkout rewrites an unchanged lockfile.

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
The build script now writes bundles to OUT_DIR/tsjs-dist, so the GPT module lookup in template-cache-local-test.sh must search out/tsjs-dist/tsjs-gpt.js.

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
@dhruv8sh
dhruv8sh marked this pull request as ready for review September 28, 2026 10:50
@aram356 aram356 added this to the 202610 milestone Sep 28, 2026

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

The private OUT_DIR bundle builds and completeness validation fix the original concurrency race. Concurrent debug and release builds produced separate complete 13-bundle sets, and an incomplete prebuilt set failed before code generation. I found one medium-severity recovery issue, included inline.

.lock()
.unwrap_or_else(|err| panic!("tsjs: failed to lock {}: {err}", lock_path.display()));

if node_modules.exists() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: A failed automatic npm ci leaves retries stuck on its partial directory

Issue: npm ci creates node_modules before installation finishes. If it then exits unsuccessfully, the assertion below panics without removing that partial directory. The next Cargo build acquires the lock, sees that node_modules exists here, and skips npm ci.

Impact: A transient install failure, or a concurrent build waiting behind it, cannot recover through the documented automatic installation path. It instead fails later with the misleading stale or missing hidden-lockfile error until someone manually removes or repairs node_modules.

Evidence: In a temporary npm fixture whose dependency install script exits 7, npm ci --foreground-scripts exited 7 while leaving node_modules present and node_modules/.package-lock.json absent. The next invocation returns here, then ensure_dependencies_fresh rejects the incomplete tree.

Suggested fix: While holding the lock, remove node_modules when the spawned npm ci fails before reporting the error. Add a fake-npm regression test that creates node_modules, exits nonzero, and verifies the next invocation attempts installation again.

This branch has not been deployed

No deployments
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.

Concurrent cargo builds race on the shared tsjs dist and can embed a partial bundle set

3 participants