Harden the shared Bun download cache against eviction and torn publishes - #19
Merged
arcaputo3 merged 3 commits intoAug 26, 2026
Merged
Conversation
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Make bare test modules actually reuse the outer install
arcaputo3
changed the base branch from
agent/bun-vendored-guard
to
agent/idiomatic-scalajs-mill-workflows
August 26, 2026 22:17
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack 6/N. Base is
agent/bun-vendored-guard(#18), notmain.Why
An adversarial review pass over the stack found two ways the shared download cache introduced in
#14 could wedge or poison the now-default managed toolchain, plus three smaller sharp edges.
The bugs
An evicted cache was never repopulated.
downloadedBunExecutablereturned a plainPathRefinto the cache —
Revalidate.Never— for a file that lives outside everyTask.dest. Deleting~/.cache/mill-bun(it is named and documented as a cache; evicting it is legitimate) left everyMill run trusting a task result that points at a file that no longer exists. Nothing recovered
until the user guessed the right
clean. The refs are now quick-signature (mtime + size)Revalidate.Always, so recovery costs one stat per evaluation.The cross-filesystem publish was not atomic. When the cache sits on a different filesystem
than
out/(a mounted CI cache volume; Docker with a bind-mounted workspace), the fallback was aplain copy straight to the checksum-keyed final path. A concurrent reader could execute a
partially written binary, and a build killed mid-copy left a truncated file that every later build
trusts unconditionally — permanent poisoning, because existence at the path is the cache's proof
of validity. The fallback now copies to a temp name inside the cache directory and renames, so
the publish itself is always a same-filesystem atomic move. The copy also preserves file
attributes: the old
os.copy.oversilently dropped the executable bit.Also fixed
cachedexists), not the exception type —Windows reports it as a sharing violation, not
FileAlreadyExistsException.copyTreerecreated relative symlinks as absolute paths into the source tree, so stagedcopies stopped being self-contained the moment the upstream task was cleaned. Raw link targets
are now preserved verbatim (bun's
node_modules/.binentries are relative links).MILL_BUN_CACHE_DIRresolved against nothing and crashed with a rawIllegalArgumentException; it now resolves against the workspace root.buildjob now setsMILL_BUN_USE_SYSTEM=trueso the system-Bun path (findOnPath,version verification) keeps coverage after Pin Bun 1.4.0, compose the asset matrix, share the download cache #14 flipped the suite default to managed — that flip
had made the job's
setup-bunstep install a binary nothing used. Andpull_requestCI is nolonger restricted to PRs targeting
main, so stacked PRs get checks.Tests
Each fix was confirmed against the parent: the new test fails without the fix, passes with it.
an evicted download cache is repopulated, not trusted(integration, forked evals with aprivate
MILL_BUN_CACHE_DIR) — fails on the parent at exactly the reviewed scenario: thepost-eviction eval succeeds while pointing at a file that no longer exists.
relative symlink targets are preserved verbatim— fails on the parent (target absolutized).cross-filesystem publish preserves permissions and leaves no temp debrisand… tolerates losing the racepin the new publish contract.Known limits (unchanged, deliberate)
muslcompat package;the musl binary still runs there, and
bunUseMuslis the documented override.prepareWebStagesymlinksnode_moduleson Windows, which needs Developer Mode or admin;the Windows CI leg planned for this stack will tell us whether a junction fallback is needed.