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/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/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/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)) 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")