build: order smoke_test and dsymutil after strip

scripts/build/bun.ts

@@ -509,40 +509,8 @@ export function emitBun(n: Ninja, cfg: Config, sources: Sources): BunOutput {
     linkerMapOutput: cfg.linux && cfg.release && !cfg.asan && !cfg.valgrind ? linkerMapPath(cfg) : undefined,
   });
 
-  // ─── Step 8: post-link (strip + dsymutil) ───
-  // Plain release only: produce stripped `bun` alongside `bun-profile`.
-  // Debug/asan/etc. keep symbols (you want them for debugging).
-  let strippedExe: string | undefined;
-  let dsym: string | undefined;
-  if (shouldStrip(cfg)) {
-    strippedExe = emitStrip(n, cfg, exe, flags.stripflags);
-    // darwin: extract debug symbols from the UNSTRIPPED exe into a .dSYM
-    // bundle. dsymutil reads DWARF from bun-profile, writes bun-profile.dSYM.
-    // Must run BEFORE stripping could discard sections it needs (we don't
-    // strip bun-profile itself, only copy → bun, so this is safe).
-    if (cfg.darwin) {
-      dsym = emitDsymutil(n, cfg, exe, exeName);
-    }
-  }
-
-  // Phony `bun` target for convenience — only when strip DIDN'T produce a
-  // literal file named `bun` (which would collide with the phony). When
-  // strip runs, `ninja bun` builds the actual stripped file; no phony needed.
-  if (strippedExe === undefined) {
-    n.phony("bun", [exe]);
-  }
-
-  // ─── Step 9: smoke test ───
-  // Run `<exe> --revision`. If it exits non-zero or crashes, something
-  // broke at load time (missing symbol, static initializer blowup, ABI
-  // mismatch). Catching this HERE is much better than "CI passes, user
-  // runs bun, it segfaults".
-  //
-  // Linux+ASAN quirk: some systems need ASLR disabled (`setarch -R`) for
-  // ASAN binaries to run from subprocesses (shadow memory layout conflict
-  // with ELF_ET_DYN_BASE, see sanitizers/856). We try with setarch first,
-  // fall back to direct invocation.
-  emitSmokeTest(n, cfg, exe, exeName);
+  // ─── Step 8: post-link (strip, dsymutil, smoke test) ───
+  const { strippedExe, dsym } = emitPostLink(n, cfg, exe, exeName, flags.stripflags);
 
   return { exe, strippedExe, dsym, deps, codegen, rustObjects, objects: allObjects };
 }
@@ -652,14 +620,7 @@ function emitLinkOnly(n: Ninja, cfg: Config): BunOutput {
   });
 
   // Strip + smoke test — same as full mode.
-  let strippedExe: string | undefined;
-  let dsym: string | undefined;
-  if (shouldStrip(cfg)) {
-    strippedExe = emitStrip(n, cfg, exe, flags.stripflags);
-    if (cfg.darwin) dsym = emitDsymutil(n, cfg, exe, exeName);
-  }
-  if (strippedExe === undefined) n.phony("bun", [exe]);
-  emitSmokeTest(n, cfg, exe, exeName);
+  const { strippedExe, dsym } = emitPostLink(n, cfg, exe, exeName, flags.stripflags);
 
   return {
     exe,
@@ -733,14 +694,7 @@ function emitRustAndLink(n: Ninja, cfg: Config, sources: Sources): BunOutput {
     linkerMapOutput: cfg.linux && cfg.release && !cfg.asan && !cfg.valgrind ? linkerMapPath(cfg) : undefined,
   });
 
-  let strippedExe: string | undefined;
-  let dsym: string | undefined;
-  if (shouldStrip(cfg)) {
-    strippedExe = emitStrip(n, cfg, exe, flags.stripflags);
-    if (cfg.darwin) dsym = emitDsymutil(n, cfg, exe, exeName);
-  }
-  if (strippedExe === undefined) n.phony("bun", [exe]);
-  emitSmokeTest(n, cfg, exe, exeName);
+  const { strippedExe, dsym } = emitPostLink(n, cfg, exe, exeName, flags.stripflags);
 
   return {
     exe,
@@ -753,13 +707,71 @@ function emitRustAndLink(n: Ninja, cfg: Config, sources: Sources): BunOutput {
   };
 }
 
+/**
+ * Post-link steps shared by every linking mode (full, link-only,
+ * rust-and-link): strip, dsymutil, the `bun` phony, and the `--revision`
+ * smoke test.
+ *
+ * Centralized because the smoke_test and dsymutil edges must be ordered
+ * after strip — their rule commands wrap through `cfg.jsRuntime`
+ * (process.execPath), which can BE the strip output when `bun` on PATH
+ * resolves into the build directory (build/release/bun). Without the
+ * ordering, ninja runs strip and the wrapper exec concurrently (both
+ * depend only on `exe`) and the wrapper fails with "Permission denied" on
+ * the half-written file. Open-coding this in each mode already caused one
+ * call site to be missed (#30539), so the invariant lives here.
+ */
+export function emitPostLink(
+  n: Ninja,
+  cfg: Config,
+  exe: string,
+  exeName: string,
+  stripflags: string[],
+): { strippedExe: string | undefined; dsym: string | undefined } {
+  // Plain release only: produce stripped `bun` alongside `bun-profile`.
+  // Debug/asan/valgrind/assertions keep symbols (you want them for
+  // debugging).
+  let strippedExe: string | undefined;
+  let dsym: string | undefined;
+  if (shouldStrip(cfg)) {
+    strippedExe = emitStrip(n, cfg, exe, stripflags);
+    // darwin: extract debug symbols from the UNSTRIPPED exe into a .dSYM
+    // bundle. dsymutil reads DWARF from bun-profile, writes
+    // bun-profile.dSYM. The input exe is never stripped in-place (strip
+    // writes a new file via -o), so the read is safe.
+    if (cfg.darwin) dsym = emitDsymutil(n, cfg, exe, exeName, strippedExe);
+  }
+
+  // `bun` phony — only when strip didn't produce a literal file named
+  // `bun` (which would collide with the phony). When strip runs, `ninja
+  // bun` builds the stripped file; no phony needed.
+  if (strippedExe === undefined) n.phony("bun", [exe]);
+
+  // Run `<exe> --revision`. If it exits non-zero or crashes, something
+  // broke at load time (missing symbol, static initializer blowup, ABI
+  // mismatch). Catching this HERE is much better than "CI passes, user
+  // runs bun, it segfaults".
+  //
+  // Linux+ASAN quirk: some systems need ASLR disabled (`setarch -R`) for
+  // ASAN binaries to run from subprocesses (shadow memory layout conflict
+  // with ELF_ET_DYN_BASE, see sanitizers/856). We try with setarch first,
+  // fall back to direct invocation.
+  emitSmokeTest(n, cfg, exe, exeName, strippedExe);
+
+  return { strippedExe, dsym };
+}
+
 /**
  * Smoke test: run the built executable with --revision. If it crashes or
  * errors, the build failed — typically means a link-time issue that the
  * linker didn't catch (missing symbol only referenced at init, ICU ABI
  * mismatch, etc.).
+ *
+ * `strippedExe` is the strip output (release builds only), added as an
+ * order-only input so this rule never runs while strip is mid-write; see
+ * emitPostLink for why.
  */
-function emitSmokeTest(n: Ninja, cfg: Config, exe: string, exeName: string): void {
+function emitSmokeTest(n: Ninja, cfg: Config, exe: string, exeName: string, strippedExe: string | undefined): void {
   // Skip when the binary can't run on this host (different os/arch/abi) —
   // `ninja check` becomes a no-op alias for the exe.
   if (!cfg.canRunOnHost) {
@@ -804,6 +816,7 @@ function emitSmokeTest(n: Ninja, cfg: Config, exe: string, exeName: string): voi
     outputs: [stamp],
     rule: "smoke_test",
     inputs: [exe],
+    ...(strippedExe !== undefined ? { orderOnlyInputs: [strippedExe] } : {}),
   });
 
   // Phony target — `ninja check` runs the smoke test.
@@ -860,8 +873,11 @@ function emitStrip(n: Ninja, cfg: Config, inputExe: string, stripflags: string[]
  * Runs dsymutil on bun-profile (which has full DWARF). The .dSYM lets you
  * symbolicate crash logs from the stripped `bun` — lldb/Instruments find
  * it automatically by UUID.
+ *
+ * `strippedExe` is order-only for the same reason as emitSmokeTest: the
+ * `cfg.jsRuntime` wrapper may be the strip output itself.
  */
-function emitDsymutil(n: Ninja, cfg: Config, inputExe: string, exeName: string): string {
+function emitDsymutil(n: Ninja, cfg: Config, inputExe: string, exeName: string, strippedExe: string): string {
   assert(cfg.darwin, "dsymutil is darwin-only");
   assert(cfg.dsymutil !== undefined, "dsymutil not found in toolchain");
 
@@ -892,6 +908,7 @@ function emitDsymutil(n: Ninja, cfg: Config, inputExe: string, exeName: string):
     outputs: [out],
     rule: "dsymutil",
     inputs: [inputExe],
+    orderOnlyInputs: [strippedExe],
   });
 
   return out;

test/internal/build-post-link-ordering.test.ts

@@ -0,0 +1,138 @@
+/**
+ * Regression tests for ninja ordering of post-link steps (strip, smoke test,
+ * dsymutil) in scripts/build/bun.ts.
+ *
+ * The smoke_test and dsymutil rule commands are wrapped through
+ * `cfg.jsRuntime` (= process.execPath). When `bun` on PATH resolves inside the
+ * build directory, that path is the strip output itself (build/release/bun),
+ * and without an ordering edge ninja will run strip and the wrapper exec
+ * concurrently, failing with "Permission denied" on the half-written file.
+ *
+ * These exercise the ninja-emission logic only (no compiler or ninja needed),
+ * so they run on every host.
+ */
+import { describe, expect, test } from "bun:test";
+import { isMacOS, tempDir } from "harness";
+import { join, resolve } from "node:path";
+
+import { emitPostLink } from "../../scripts/build/bun.ts";
+import { resolveConfig, type Config, type PartialConfig, type Toolchain } from "../../scripts/build/config.ts";
+import { Ninja } from "../../scripts/build/ninja.ts";
+
+/** A fully-populated fake toolchain; resolveConfig never spawns any of these. */
+function mockToolchain(overrides: Partial<Toolchain> = {}): Toolchain {
+  return {
+    cc: "/fake/llvm/bin/clang",
+    cxx: "/fake/llvm/bin/clang++",
+    hostCc: undefined,
+    hostCxx: undefined,
+    clangVersion: "21.1.8",
+    clangResourceDir: "/fake/llvm/lib/clang/21",
+    ar: "/fake/llvm/bin/llvm-ar",
+    ranlib: "/fake/llvm/bin/llvm-ranlib",
+    ld: "/fake/llvm/bin/ld.lld",
+    ld64Lld: "/fake/llvm/bin/ld64.lld",
+    rustLld: undefined,
+    rustLlvmVersion: "22.1.4",
+    rustSysroot: undefined,
+    rustHostTriple: undefined,
+    strip: "/fake/bin/strip",
+    llvmStrip: "/fake/llvm/bin/llvm-strip",
+    dsymutil: "/fake/llvm/bin/dsymutil",
+    bun: "/fake/bin/bun",
+    jsRuntime: "/fake/bin/bun",
+    esbuild: "/fake/bin/esbuild",
+    ccache: undefined,
+    cmake: "/fake/bin/cmake",
+    cargo: undefined,
+    cargoHome: undefined,
+    rustupHome: undefined,
+    msvcLinker: undefined,
+    rc: undefined,
+    mt: undefined,
+    nasm: undefined,
+    ...overrides,
+  };
+}
+
+/**
+ * Resolve a host-targeted config: no os/arch override, so `canRunOnHost` is
+ * true and the smoke_test rule emits the real edge (not the phony short-circuit).
+ */
+function hostConfig(partial: PartialConfig, buildDir: string): Config {
+  return resolveConfig(
+    { buildDir, ...partial },
+    // jsRuntime = the strip output: what resolveToolchain() produces when
+    // `bun` on PATH resolves into build/release/.
+    mockToolchain({ jsRuntime: join(buildDir, "bun") }),
+  );
+}
+
+/** Find one build-edge line in the generated ninja text (continuations unwrapped). */
+function buildEdge(ninja: string, rule: string): string {
+  const flat = ninja.replace(/ \$\n +/g, " ");
+  const line = flat.split("\n").find(l => l.startsWith("build ") && l.includes(`: ${rule} `));
+  if (line === undefined) throw new Error(`no '${rule}' edge in ninja output:\n${ninja}`);
+  return line;
+}
+
+describe("emitPostLink ninja ordering", () => {
+  test("release smoke_test is ordered after strip", () => {
+    using dir = tempDir("build-post-link", {});
+    const buildDir = String(dir);
+    const cfg = hostConfig({ buildType: "Release" }, buildDir);
+    expect(cfg.canRunOnHost).toBe(true);
+
+    const n = new Ninja({ buildDir });
+    const exe = resolve(buildDir, `bun-profile${cfg.exeSuffix}`);
+    const { strippedExe } = emitPostLink(n, cfg, exe, "bun-profile", []);
+    const out = n.toString();
+
+    expect(strippedExe).toBe(resolve(buildDir, `bun${cfg.exeSuffix}`));
+    // strip writes `bun`; the smoke_test wrapper execs cfg.jsRuntime
+    // (= `bun` here). Without `|| bun` ninja schedules them concurrently
+    // and the wrapper sees a half-written file.
+    expect(buildEdge(out, "smoke_test")).toBe(
+      `build bun-profile.smoke-test-passed: smoke_test bun-profile${cfg.exeSuffix} || bun${cfg.exeSuffix}`,
+    );
+    expect(buildEdge(out, "strip")).toBe(`build bun${cfg.exeSuffix}: strip bun-profile${cfg.exeSuffix}`);
+  });
+
+  test("debug smoke_test has no strip dep (nothing to order against)", () => {
+    using dir = tempDir("build-post-link", {});
+    const buildDir = String(dir);
+    const cfg = hostConfig({ buildType: "Debug", assertions: true }, buildDir);
+
+    const n = new Ninja({ buildDir });
+    const exe = resolve(buildDir, `bun-debug${cfg.exeSuffix}`);
+    const { strippedExe, dsym } = emitPostLink(n, cfg, exe, "bun-debug", []);
+    const out = n.toString();
+
+    expect({ strippedExe, dsym }).toEqual({ strippedExe: undefined, dsym: undefined });
+    expect(buildEdge(out, "smoke_test")).toBe(
+      `build bun-debug.smoke-test-passed: smoke_test bun-debug${cfg.exeSuffix}`,
+    );
+    expect(buildEdge(out, "phony")).toBe(`build bun: phony bun-debug${cfg.exeSuffix}`);
+  });
+
+  // Cross-config path only: on macOS, resolveConfig({ os: "darwin" }) probes
+  // xcode-select for the real SDK, which belongs to the native test above.
+  // The ordering logic is identical to the smoke_test case.
+  test.skipIf(isMacOS)("darwin release dsymutil is ordered after strip", () => {
+    using dir = tempDir("build-post-link", {});
+    const buildDir = String(dir);
+    const cfg = resolveConfig({ os: "darwin", arch: "aarch64", buildType: "Release", buildDir }, mockToolchain());
+    expect(cfg.canRunOnHost).toBe(false);
+
+    const n = new Ninja({ buildDir });
+    const exe = resolve(buildDir, "bun-profile");
+    const { dsym } = emitPostLink(n, cfg, exe, "bun-profile", []);
+    const out = n.toString();
+
+    expect(dsym).toBe(resolve(buildDir, "bun-profile.dSYM"));
+    expect(buildEdge(out, "dsymutil")).toBe("build bun-profile.dSYM: dsymutil bun-profile || bun");
+    // Cross-compile: smoke_test short-circuits to a `check` phony (the
+    // binary can't run on this host), so the strip race can't happen there.
+    expect(buildEdge(out, "phony")).toBe("build check: phony bun-profile");
+  });
+});