Skip to content

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

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

dhruv8sh wants to merge 6 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.

Comment thread crates/trusted-server-js/build.rs

@prk-Jr prk-Jr 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

Building each Cargo invocation into its own OUT_DIR removes the shared-dist race, and validating the expected bundle set prevents silently embedding missing or empty modules. No blocking findings remain; the two recommendations below improve dependency tracking and regression coverage.

One inline comment includes a one-click GitHub suggestion with the verified replacement.

Non-blocking

♻️ refactor

  • Track the external Prebid builder as a JS test input — see inline at crates/trusted-server-js/build.rs:105.

Cross-cutting / body-level findings

  • 🌱 Automate the build regressions. The seven new helper tests cover bundle-set validation. Add regression coverage for simultaneous builds with separate output directories and incremental TSJS_TEST=1 builds after changing the external Prebid builder. Check that both concurrent builds contain the complete non-empty module set and that changing an imported implementation invalidates the requested tests. The current full JS suite creates temporary generated files under watched lib/src, making unchanged test-enabled builds dirty; the incremental regression should account for that side effect rather than pass because unrelated directory timestamps changed.

Verification

The one-line suggestion passed formatting, all eight target-matched Clippy aliases, Fastly/Axum/Cloudflare compilation checks, all four adapter test aliases, and cross-adapter parity. Rust tests: 3,306 passed, 13 ignored. Broad Rust checks used validated prebuilt bundles; the actual npm build and TSJS_TEST=1 path were also exercised with Node 24.12.0. All 1,185 JS tests and JS formatting passed. Two concurrent JS builds produced all 13 expected non-empty bundles with identical bytes.

The isolated Cargo harness confirms that adding the missing watched input triggers rebuilding. The actual full suite currently changes watched source-directory timestamps on each run, masking the omission, so this is a non-blocking improvement rather than a demonstrated skipped-test regression. The scratch patch remained byte-identical after verification and was discarded.

CI Status

Comment thread crates/trusted-server-js/build.rs Outdated

@prk-Jr prk-Jr 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.

Approved. Verification found no blocking issues. The dependency-tracking suggestion and regression-test recommendation in the earlier review are non-blocking.

…nal Prebid builder

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Resolve the semantic conflict with #1180: the generated ALL_MODULE_IDS length now uses the renamed expected module list.

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>

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

4 participants