From 5967ae81dbbd8b09eb557d2cc8bfed479a7eaa00 Mon Sep 17 00:00:00 2001 From: Richie Caputo Date: Wed, 26 Aug 2026 17:45:17 -0400 Subject: [PATCH 1/2] Harden the shared Bun download cache against eviction and torn publishes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An evicted cache entry was never repopulated: downloadedBunExecutable returned a Revalidate.Never PathRef to a file outside every Task.dest, so deleting ~/.cache/mill-bun left every build trusting a path that no longer exists until the user guessed the right `clean`. Cache refs now carry quick (mtime+size) signatures with Revalidate.Always — recovery costs one stat per evaluation. The cross-filesystem publish fallback copied straight to the checksum-keyed final path, so a concurrent reader could execute a partially written binary and a build killed mid-copy poisoned the cache permanently. The fallback now stages next to the final path and renames — the publish is always a same-filesystem atomic move — and preserves the executable bit the old os.copy.over silently dropped. Losing a publish race is judged by the outcome (cached exists), not the exception type, since Windows reports a sharing violation rather than FileAlreadyExistsException. Also: copyTree preserves relative symlink targets verbatim instead of absolutizing them into the source tree; a relative MILL_BUN_CACHE_DIR resolves against the workspace root instead of crashing; the CI build job pins MILL_BUN_USE_SYSTEM=true so the system-Bun path keeps coverage now that the suite defaults to managed; and pull_request CI is no longer restricted to PRs targeting main, so stacked PRs get checks. Every fix was confirmed to fail on the parent commit before trusting its pass: the eviction integration test (forked evals with a private MILL_BUN_CACHE_DIR) fails there with a successful eval pointing at a missing file, and the relative-symlink test fails with an absolutized target. Co-Authored-By: Claude Fable 5 --- .github/workflows/ci.yml | 6 ++- .../BunManagedToolchainIntegrationTests.scala | 26 +++++++++ millbun/src/mill/bun/BunToolchainModule.scala | 53 ++++++++++++++++--- .../test/src/mill/bun/BunToolchainTests.scala | 25 +++++++++ millbun/test/src/mill/bun/CopyTreeTests.scala | 15 ++++++ 5 files changed, 116 insertions(+), 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e36b0e8..1694ba3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,7 +3,6 @@ on: push: branches: [main] pull_request: - branches: [main] jobs: build: @@ -29,7 +28,12 @@ jobs: - name: Unit Tests run: ./mill --no-server millbun.test + # The suite defaults to the managed toolchain; this job pins system Bun to the same + # version so `findOnPath` resolution and version verification keep CI coverage, while + # the managed-bun job covers the download/cache path. - name: Integration Tests + env: + MILL_BUN_USE_SYSTEM: 'true' run: ./mill --no-server millbun.integration managed-bun: diff --git a/millbun/integration/src/mill/bun/BunManagedToolchainIntegrationTests.scala b/millbun/integration/src/mill/bun/BunManagedToolchainIntegrationTests.scala index 3aae15f..366c1c5 100644 --- a/millbun/integration/src/mill/bun/BunManagedToolchainIntegrationTests.scala +++ b/millbun/integration/src/mill/bun/BunManagedToolchainIntegrationTests.scala @@ -20,3 +20,29 @@ object BunManagedToolchainIntegrationTests extends TestSuite: val executable = tester.out("app.bunExecutable").value[String] val version = os.proc(executable, "--version").call(stdout = os.Pipe).out.text().trim assert(version == "1.4.0") + + test("an evicted download cache is repopulated, not trusted"): + val cacheDir = os.temp.dir() + val env = Map("MILL_BUN_CACHE_DIR" -> cacheDir.toString) + // Forked evals, not in-memory: `Task.env` reads the Mill process's own environment, so a + // per-eval env override only reaches a subprocess. + val tester = new IntegrationTester( + daemonMode = false, + workspaceSourcePath = resourceDir / "managed-bun", + millExecutable = millExe, + useInMemory = false + ) + assert(tester.eval("app.bunExecutable", env = env).isSuccess) + // Proves MILL_BUN_CACHE_DIR reached the build before the eviction step relies on it. + assert(os.walk(cacheDir).exists(p => os.isFile(p))) + + // Users legitimately evict the cache directory; the build must recover on its own + // rather than trust a stale task result pointing at a file that no longer exists. + os.remove.all(cacheDir) + os.makeDir.all(cacheDir) + + assert(tester.eval("app.bunExecutable", env = env).isSuccess) + val executable = tester.out("app.bunExecutable").value[String] + assert(os.isFile(os.Path(executable))) + val version = os.proc(executable, "--version").call(stdout = os.Pipe).out.text().trim + assert(version == "1.4.0") diff --git a/millbun/src/mill/bun/BunToolchainModule.scala b/millbun/src/mill/bun/BunToolchainModule.scala index af89292..980d333 100644 --- a/millbun/src/mill/bun/BunToolchainModule.scala +++ b/millbun/src/mill/bun/BunToolchainModule.scala @@ -206,13 +206,46 @@ object BunToolchainModule { ) cached catch - case _: java.nio.file.FileAlreadyExistsException => cached - case _: java.nio.file.AtomicMoveNotSupportedException => - // Cache on a different filesystem than the task dest: fall back to a plain copy. - os.copy.over(staged, cached, createFolders = true) + case _: java.nio.file.AtomicMoveNotSupportedException => publishViaCopy(staged, cached) + case scala.util.control.NonFatal(_) if os.exists(cached) => + // Lost the publish race. The path is keyed by the verified checksum, so the winner's + // bytes are the right bytes. Windows reports this as a sharing violation rather than + // FileAlreadyExistsException, so match on the outcome, not the exception type. cached } + /** + * Publish across filesystems by staging a copy next to the final path, so the last hop is a + * same-filesystem atomic rename. Copying straight to the published name would let a concurrent + * reader execute a partially written binary — and a build killed mid-copy would leave a + * truncated file that every later build trusts, because the path is the checksum. + */ + private[bun] def publishViaCopy(staged: os.Path, cached: os.Path): os.Path = { + val tmp = cached / os.up / + s"${cached.last}.tmp-${ProcessHandle.current().pid()}-${System.nanoTime()}" + os.copy(staged, tmp, copyAttributes = true, createFolders = true) + try + java.nio.file.Files.move( + tmp.toNIO, + cached.toNIO, + java.nio.file.StandardCopyOption.ATOMIC_MOVE + ) + catch + case scala.util.control.NonFatal(_) if os.exists(cached) => os.remove.all(tmp) + cached + } + + /** + * A `PathRef` into the shared download cache. + * + * The file lives outside every `Task.dest`, so Mill must re-check it each evaluation: entries + * are immutable but evictable, and without revalidation an evicted executable is never + * re-downloaded — every bun invocation fails until the user guesses the right `clean`. Quick + * (mtime + size) signatures keep the per-evaluation cost to a stat. + */ + private[bun] def cachedExecutableRef(cached: os.Path): PathRef = + PathRef(cached, quick = true).withRevalidate(PathRef.Revalidate.Always) + /** Parse package.json-style `name@specifier` declarations without slicing scoped names incorrectly. */ private[bun] def parseDependency(input: String): Either[String, NpmDependency] = { val trimmed = input.trim @@ -323,7 +356,10 @@ object BunToolchainModule { if (os.isLink(path)) { os.makeDir.all(target / os.up) if (os.exists(target, followLinks = false)) os.remove(target) - os.symlink(target, os.readLink.absolute(path)) + // Preserve the raw target: absolutizing a tree-internal relative link (bun's + // node_modules/.bin entries) would point the copy back into the source tree, so the + // copy stops being self-contained the moment the source is cleaned or relocated. + os.symlink(target, os.readLink(path)) } else if (os.isDir(path, followLinks = false)) { os.makeDir.all(target) } else { @@ -416,7 +452,8 @@ trait BunToolchainModule extends Module { * Lives outside the workspace, so Mill's filesystem checker does not restrict it. */ def bunDownloadCacheDir: T[os.Path] = Task.Input { - Task.env.get("MILL_BUN_CACHE_DIR").filter(_.nonEmpty).map(os.Path(_)) + Task.env.get("MILL_BUN_CACHE_DIR").filter(_.nonEmpty) + .map(os.Path(_, BuildCtx.workspaceRoot)) .getOrElse(os.home / ".cache" / "mill-bun") } @@ -560,7 +597,7 @@ trait BunToolchainModule extends Module { // Keyed by the verified checksum, so a cache hit is proof of the right bytes. val cached = bunDownloadCacheDir() / checksum / dist.executableName - if (os.exists(cached)) PathRef(cached) + if (os.exists(cached)) BunToolchainModule.cachedExecutableRef(cached) else { val archive = Task.dest / dist.assetName val staged = Task.dest / dist.executableName @@ -570,7 +607,7 @@ trait BunToolchainModule extends Module { Task.fail(s"Bun archive checksum mismatch for $url: expected $checksum, received $actual") } BunToolchainModule.extractExecutable(archive, dist.executableName, staged) - PathRef(BunToolchainModule.publishToCache(staged, cached)) + BunToolchainModule.cachedExecutableRef(BunToolchainModule.publishToCache(staged, cached)) } } diff --git a/millbun/test/src/mill/bun/BunToolchainTests.scala b/millbun/test/src/mill/bun/BunToolchainTests.scala index d1ac89c..98bddf3 100644 --- a/millbun/test/src/mill/bun/BunToolchainTests.scala +++ b/millbun/test/src/mill/bun/BunToolchainTests.scala @@ -126,6 +126,31 @@ object BunToolchainTests extends TestSuite: assert(BunToolchainModule.publishToCache(second, cached) == cached) assert(os.read(cached) == "bun-binary") + test("cross-filesystem publish preserves permissions and leaves no temp debris"): + val root = os.temp.dir() + val cached = root / "cache" / "def456" / "bun" + val staged = root / "staged" / "bun" + os.write(staged, "bun-binary", createFolders = true) + os.perms.set(staged, "rwxr-xr-x") + + assert(BunToolchainModule.publishViaCopy(staged, cached) == cached) + assert(os.read(cached) == "bun-binary") + assert(os.perms(cached).toString == "rwxr-xr-x") + // The bytes must travel under a temp name and arrive by rename: a crash mid-publish can + // never leave a partial file at the published path, and success leaves nothing behind. + assert(os.list(cached / os.up) == Seq(cached)) + + test("cross-filesystem publish tolerates losing the race"): + val root = os.temp.dir() + val cached = root / "cache" / "0123" / "bun" + os.write(cached, "bun-binary", createFolders = true) + val staged = root / "staged" / "bun" + os.write(staged, "bun-binary", createFolders = true) + + assert(BunToolchainModule.publishViaCopy(staged, cached) == cached) + assert(os.read(cached) == "bun-binary") + assert(os.list(cached / os.up) == Seq(cached)) + test("computes SHA-256"): val file = os.temp(contents = "hello") assert( diff --git a/millbun/test/src/mill/bun/CopyTreeTests.scala b/millbun/test/src/mill/bun/CopyTreeTests.scala index c36807b..db629e7 100644 --- a/millbun/test/src/mill/bun/CopyTreeTests.scala +++ b/millbun/test/src/mill/bun/CopyTreeTests.scala @@ -56,6 +56,21 @@ object CopyTreeTests extends TestSuite: assert(os.isLink(dest / "alias.js")) assert(os.read(dest / "alias.js") == "real") + test("relative symlink targets are preserved verbatim"): + // bun's node_modules/.bin entries are relative links; absolutizing them would point the + // copy back into the source tree, dangling as soon as the source task is cleaned. + val source = os.temp.dir() + os.write(source / "esbuild" / "bin" / "esbuild", "#!/usr/bin/env node", createFolders = true) + os.makeDir.all(source / ".bin") + os.symlink(source / ".bin" / "esbuild", os.RelPath("../esbuild/bin/esbuild")) + + val dest = os.temp.dir() + BunToolchainModule.copyTree(source, dest) + assert(os.readLink(dest / ".bin" / "esbuild") == os.RelPath("../esbuild/bin/esbuild")) + // Self-contained: the copied link resolves inside the copy even after the source vanishes. + os.remove.all(source) + assert(os.read(dest / ".bin" / "esbuild") == "#!/usr/bin/env node") + test("nested directories and empty directories are preserved"): val source = os.temp.dir() os.makeDir.all(source / "empty") From 9282ea459d0e9e1a6c35aa9c9a1abe7d92f8c226 Mon Sep 17 00:00:00 2001 From: Richie Caputo Date: Wed, 26 Aug 2026 17:52:23 -0400 Subject: [PATCH 2/2] Make bare test modules actually reuse the outer install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BunTypeScriptTests extends upstream Mill's TypeScriptTests, so the unqualified this.tsDeps() inside bunTestPackageJson resolved to the Node toolchain defaults (ts-node, tsconfig-paths, @types/node) that the outer trait deliberately replaced. Those names always survived the outer-name filter, so a bare test module's package.json never equaled the outer's and the reusesOuterInstall branch was unreachable — with bunRequireLockfile on, every bare test module demanded its own lockfile, contradicting the documented contract. The trait now pins tsDeps to the outer module's. The guard test passed anyway because both of its assertions also hold in the broken standalone branch (the suite relaxes the lockfile requirement, and installs never write into the source tree). It now also asserts the returned install path is the outer module's npmInstall.dest — confirmed failing before this fix and passing after. Also: dev() on BunTypeScriptWebModule served straight out of the cached webStage output while its sync thread mirrored live edits (but never deletions) into it, and bundle builds from that same stage — a file created and deleted during a dev session would ship in the next production bundle. dev() now serves from a private copy in its own command dest; copyTree preserves the node_modules symlink, so the copy is cheap. Co-Authored-By: Claude Fable 5 --- .../src/mill/bun/BunTypeScriptIntegrationTests.scala | 4 ++++ .../mill/javascriptlib/bun/BunTypeScriptModule.scala | 12 ++++++++++++ .../javascriptlib/bun/BunTypeScriptWebModule.scala | 6 +++++- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/millbun/integration/src/mill/bun/BunTypeScriptIntegrationTests.scala b/millbun/integration/src/mill/bun/BunTypeScriptIntegrationTests.scala index 0aa008e..45eb959 100644 --- a/millbun/integration/src/mill/bun/BunTypeScriptIntegrationTests.scala +++ b/millbun/integration/src/mill/bun/BunTypeScriptIntegrationTests.scala @@ -243,6 +243,10 @@ object BunTypeScriptIntegrationTests extends TestSuite { val tester = this.tester("typescript-tests") assert(tester.eval("app.test.npmInstall").isSuccess) assert(!os.exists(tester.workspacePath / "test" / "bun.lock")) + // And must actually reuse the outer install — not run a second one that merely succeeds + // because the lockfile requirement happens to be off in this suite. + val installPath = outputPath(tester, "app.test.npmInstall") + assert(installPath == tester.workspacePath / "out" / "app" / "npmInstall.dest") } test("bunEnv") { diff --git a/millbun/src/mill/javascriptlib/bun/BunTypeScriptModule.scala b/millbun/src/mill/javascriptlib/bun/BunTypeScriptModule.scala index 44644f0..6ff9dbc 100644 --- a/millbun/src/mill/javascriptlib/bun/BunTypeScriptModule.scala +++ b/millbun/src/mill/javascriptlib/bun/BunTypeScriptModule.scala @@ -409,6 +409,18 @@ trait BunTypeScriptModule extends TypeScriptModule with BunToolchainModule with */ trait BunTypeScriptTests extends TypeScriptTests { + /** + * The outer module's Bun-specific TS toolchain, not upstream Mill's ts-node defaults. + * + * This trait extends upstream `TypeScriptTests`, so an unqualified `tsDeps()` resolves to + * the Node toolchain (`ts-node`, `tsconfig-paths`, `@types/node`) that the outer trait + * deliberately replaced. Those names always survived the outer-name filter in + * [[bunTestPackageJson]], so a bare test module's package.json never matched the outer's + * and the install-reuse path in [[npmInstall]] was unreachable — with `bunRequireLockfile` + * on, every bare test module demanded its own lockfile. + */ + override def tsDeps: T[Seq[String]] = Task { outer.tsDeps() } + /** Test timeout in milliseconds. 0 means no timeout. */ def bunTestTimeout: T[Int] = Task { 0 } diff --git a/millbun/src/mill/javascriptlib/bun/BunTypeScriptWebModule.scala b/millbun/src/mill/javascriptlib/bun/BunTypeScriptWebModule.scala index b66d9e9..22c9ac6 100644 --- a/millbun/src/mill/javascriptlib/bun/BunTypeScriptWebModule.scala +++ b/millbun/src/mill/javascriptlib/bun/BunTypeScriptWebModule.scala @@ -74,7 +74,11 @@ trait BunTypeScriptWebModule extends BunTypeScriptModule: /** Start Bun's HTML development server with source mirroring for native HMR. */ def dev(): Command[Unit] = Task.Command { - val stage = webStage().path + // Serve from a private copy: the sync thread mirrors live edits (but never deletions) into + // the serving root, and `bundle` builds from the same cached stage — mutating it in place + // would let a file created and deleted during a dev session ship in the production bundle. + val stage = Task.dest / "stage" + mill.bun.BunToolchainModule.copyTree(webStage().path, stage) val entries = BunWebSupport.htmlEntries(webEntryPoints(), moduleDir, stage) val syncRoots = (sources() ++ generatedSources() ++ resources() ++ webEntryPoints() ++ webPublicSources()) .filter(ref => os.exists(ref.path) && ref.path.startsWith(moduleDir))