transpiler cache: give the Rust line its own .pile2 filename namespace#31803
transpiler cache: give the Rust line its own .pile2 filename namespace#31803robobun wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR consolidates runtime transpiler cache type definitions into the canonical AST crate, bumps the cache format version to 23, migrates on-disk cache file suffixes from ChangesRuntime transpiler cache format and type consolidation
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:24 PM PT - Jun 29th, 2026
❌ @robobun, your commit c18495b has 1 failures in
🧪 To try this PR locally: bunx bun-pr 31803That installs a local version of the PR into your bun-31803 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — effectively a version-constant bump into a reserved namespace, with a solid regression test; the flagged mirror constant is pre-existing dead code and non-blocking.
Extended reasoning...
Overview
This PR touches two files: src/jsc/RuntimeTranspilerCache.rs (changes EXPECTED_VERSION from 22 to 1_000_000 + 23, adds a RUST_VERSION_NAMESPACE constant and an explanatory doc comment) and test/cli/run/transpiler-cache.test.ts (adds one regression test). The functional change is a single constant value; Metadata::decode already rejects any cache_version != EXPECTED_VERSION with StaleCache, which triggers the existing unlink-and-rewrite path in from_file_with_cache_file_path. No control flow, layout, or hashing changes.
Security risks
None introduced. The change strictly tightens which on-disk cache entries are trusted — entries from the Zig 1.3.x line or pre-namespace canaries now fail the version check and are discarded/rewritten instead of being executed. The cache directory remains per-user; no new attack surface.
Level of scrutiny
Low. This is the canonical "bump cache version to invalidate stale entries" pattern, just with a large offset to permanently separate two independently-bumped numbering lines. The worst-case outcome is a one-time cold cache for users upgrading, which is the explicit intent. The new test is well-constructed: it writes a real entry, patches it to a colliding foreign version with a valid self-hash, and asserts the sentinel does not execute and the entry is rewritten under the new namespace.
Other factors
The one inline finding is explicitly pre-existing: a dead mirror constant in src/bundler/cache.rs that was already stale (20 vs 22) before this PR and has no live references — purely a hygiene note, not a blocker. No CODEOWNERS cover the touched paths, no outstanding human review comments, and the existing transpiler-cache test suite covers the surrounding hit/miss/invalidation behavior.
There was a problem hiding this comment.
LGTM — thanks for the cleanup in 3e64804; the remaining //! header nit is cosmetic and non-blocking.
Extended reasoning...
Overview
The functional change is a single constant: EXPECTED_VERSION in src/jsc/RuntimeTranspilerCache.rs moves from 22 to 1_000_000 + 23, namespacing the Rust-line transpiler cache versions away from the Zig 1.3.x maintenance line so the two implementations can never accidentally trust each other's on-disk entries. The rest of the diff is (a) an explanatory doc comment on the constant, (b) a new regression test in transpiler-cache.test.ts that forges a colliding version-22 entry and asserts it's rejected and rewritten, and (c) the follow-up commit 3e64804 deleting dead mirror structs from src/bundler/cache.rs plus a doc-pointer fix in src/ast/transpiler_cache.rs — both done in response to my earlier review comment, which is now resolved.
Security risks
None introduced. The version field is already compared via strict equality in Metadata::decode (!= EXPECTED_VERSION → StaleCache), so changing its value only changes which entries match — there's no new parsing surface, no new file I/O, and no widening of trust. If anything this narrows the attack/confusion surface by guaranteeing entries written by a different bun implementation are unlinked rather than executed. The deleted bundler mirror structs were verifiably unreferenced (confirmed by grep: zero remaining hits for any of the removed symbols).
Level of scrutiny
Low-to-medium. The runtime transpiler cache is on the hot path for bun run, but the mechanism touched here is the existing version-mismatch → unlink → re-transpile path, which is already exercised by the existing test suite and the new test. The constant fits comfortably in u32, the encode/decode wire format is unchanged, and the only user-visible effect of the bump is a one-time cache invalidation on upgrade — which is the explicit goal. The dead-code deletion is pure subtraction with no callers.
Other factors
- My previous inline comment was addressed and resolved by the author in 3e64804.
- The one new finding is a doc-comment nit (the module-level
//!header atsrc/ast/transpiler_cache.rs:10-11still referencesbun_bundler::cache) — same class of stale-pointer cleanup already done twice in 3e64804, zero runtime impact, fine to fold in or leave for a follow-up. - CI failures on this build (musl LTO link error in
bun_rust.lto.o.tmp,bunx.test.ts) are unrelated infrastructure/flakes — none touch the transpiler cache and none reference the changed files. - No CODEOWNERS cover the touched paths.
There was a problem hiding this comment.
LGTM — both prior review notes addressed; the functional change is just the EXPECTED_VERSION namespace offset plus verified-dead-code removal and a regression test.
Extended reasoning...
Overview
The functional core of this PR is a single constant change in src/jsc/RuntimeTranspilerCache.rs: EXPECTED_VERSION moves from 22 to RUST_VERSION_NAMESPACE + 23 (= 1_000_023), reserving cache versions ≥ 1_000_000 for the Rust line so entries written by the Zig 1.3.x maintenance line (or by the v20–22 overlap-window canaries) always fail the existing cache_version != EXPECTED_VERSION check in Metadata::decode and get unlinked/rewritten. The wire format is unchanged — cache_version was already a u32 LE at offset 0, so the larger value round-trips identically. Bumping this constant is routine for this file (the doc comment lists 22 prior bumps); only the magnitude is novel, and it's well-motivated and thoroughly documented inline.
The remaining changes are non-functional: ~90 lines of dead mirror structs deleted from src/bundler/cache.rs (I flagged these in an earlier review round and re-verified zero references remain in src/), doc-comment corrections in src/ast/transpiler_cache.rs and src/bundler/cache.rs pointing at the canonical bun_jsc::runtime_transpiler_cache types, and a new regression test in transpiler-cache.test.ts that synthesizes a foreign-version-22 entry with valid self-hashes and asserts it is rejected and rewritten under the new namespace.
Security risks
None introduced. The change is strictly defensive — it narrows which on-disk cache entries the runtime will trust, closing a cross-implementation cache-poisoning window. No new file paths, no parsing of additional untrusted data, no auth/crypto/permission surface.
Level of scrutiny
Low-to-moderate. The transpiler cache is user-facing (cache poisoning produces persistent wrong behavior), but the actual mechanism touched is the existing version-mismatch → StaleCache → unlink path, which is unchanged; only the constant it compares against moved. The dead-code deletion was already traced to zero call sites in my first review round and re-confirmed by grep on the current tree. No CODEOWNERS paths match.
Other factors
Both of my earlier review comments (stale RUNTIME_TRANSPILER_CACHE_VERSION mirror, stale module //! header) were addressed in 3e64804 and d5fd546 respectively and are resolved. The bug-hunting system found nothing on the current revision. The CI failures reported by robobun on 3e64804 (musl LTO llvm-link data-layout error, bunx.test.ts) are unrelated infra/flake — none of these files touch the linker pipeline or bunx, and the only delta since then (d5fd546) is a doc-comment edit.
|
CI triage (current: build 67084 on c18495b, rebased onto main 16a7269, all lanes finished): the diff is green — 72/72 individual lanes passed, including both touched test files (
Ready to merge over that lane, or to rebase once the memleak test is fixed on main. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
How is this expected to load and be consistent?
robobun
changed the title
You're right — it wasn't. With the version offset the two implementation lines still read and wrote the same Reworked in 559ef8f: the Rust line now writes
The regression test now forges the entry a Zig-line bun would have written for the exact same source (legacy |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/runtime/environment-variables.mdx`:
- Line 199: Reword the BUN_RUNTIME_TRANSPILER_CACHE_PATH description to improve
flow by consolidating the three "If" sentences into one clear sentence that
lists the conditions and outcomes for the variable; reference the environment
variable name BUN_RUNTIME_TRANSPILER_CACHE_PATH and state that setting it to an
empty string or "0" disables caching, leaving it unset uses the platform cache
directory, and any other non-empty value is treated as the explicit cache
directory path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67f13b7c-bd71-48ac-bf03-82e3bbdae685
📒 Files selected for processing (6)
docs/runtime/environment-variables.mdxsrc/ast/transpiler_cache.rssrc/bundler/cache.rssrc/jsc/RuntimeTranspilerCache.rstest/cli/run/transpiler-cache.test.tstest/regression/issue/30887.test.ts
There was a problem hiding this comment.
All my earlier nits are addressed and the bug hunter found nothing this round; deferring to a human to confirm the .pile2 filename-namespace approach (the mid-PR redesign prompted by the "how is this expected to load and be consistent?" review) before it ships as the on-disk format.
Extended reasoning...
Overview
The runtime change is two string literals (b".pile" → b".pile2", b".debug.pile" → b".debug.pile2") in write_cache_filename and one constant bump (EXPECTED_VERSION 22 → 23) in src/jsc/RuntimeTranspilerCache.rs. Everything else is supporting work: dead mirror-struct removal in src/bundler/cache.rs, doc-comment updates in src/ast/transpiler_cache.rs, user-facing docs (.pile → .pile2, 50 KB → 4 KB) in docs/runtime/environment-variables.mdx, a thorough new regression test in test/cli/run/transpiler-cache.test.ts that forges a colliding-version .pile entry and asserts it's never served or unlinked, and a filename-filter widening in test/regression/issue/30887.test.ts.
Security risks
None identified. The change narrows what the cache loader will read (only .pile2 files written by this implementation line), which strictly reduces the attack/poisoning surface relative to the status quo. No auth, crypto, or permission logic touched.
Level of scrutiny
Medium. The mechanical change is tiny and obviously correct, but it's a one-way compatibility commitment for the on-disk cache format, and the PR went through a mid-flight design pivot (version-offset namespace → disjoint filename suffix) in response to a human reviewer's "how is this expected to load and be consistent?" question. The author's reply and the in-code EXPECTED_VERSION doc comment explain the new design thoroughly, but the reviewer who prompted the redesign hasn't yet confirmed. That sign-off is the right gate, not mine.
Other factors
- I left five inline comments across earlier revisions (stale mirror constant, stale module-header doc, stale PR description, stale
.pilein docs, missed third "50 kb" instance); all five are addressed and resolved as of 5ee64ad + 69110f9 (autofix table reflow). Nothing remains from my side. - CI is red, but the author's triage attributes both failures to repo-wide breakage in this window (musl LTO data-layout mismatch;
bunx.test.tsbroken by upstream@angular/cliengine constraint) and not to this diff. I find that triage plausible — nothing here touches the linker or bunx — but it's another reason a human should make the merge-over-red call rather than a bot. - No CODEOWNERS entries cover the touched paths.
- The new regression test is well-constructed (sentinel payload, byte-identical-after assertion, third-run cache-hit check) and would have caught the original poisoning.
robobun
force-pushed
the
farm/ad2450b3/transpiler-cache-version-namespace
branch
from
69110f9 to
1466ba9
Compare
There was a problem hiding this comment.
All prior nits are resolved and I found nothing new — the runtime change is just the .pile → .pile2 suffix plus the version-23 bump. Deferring only because the cross-implementation-line cache-namespace design (and leaving Zig-line .pile files untouched forever) is a maintainer-level call on a path that decides which transpiled bytes get executed.
Extended reasoning...
Overview
This PR separates the Rust-line runtime transpiler cache from the Zig 1.3.x maintenance line by changing the on-disk filename suffix from .pile to .pile2 and bumping EXPECTED_VERSION from 22 to 23. Around that two-line core change it: deletes ~85 lines of provably-dead mirror structs from src/bundler/cache.rs; updates three doc comments in src/ast/transpiler_cache.rs and src/bundler/cache.rs to point at the surviving bun_jsc::runtime_transpiler_cache types; updates docs/runtime/environment-variables.mdx (.pile → .pile2, 50 KB → 4 KB threshold in three places); adds a forged-entry regression test in transpiler-cache.test.ts that plants a hash-valid .pile file and asserts it is never loaded, never deleted, and a fresh .pile2 is written alongside; and relaxes the 30887 regression test's filename filter from .endsWith(".pile") to .includes(".pile") so it matches .pile2/.debug.pile2.
Security risks
None introduced. The change strictly reduces cross-trust: foreign .pile entries are no longer opened at all, eliminating the cross-implementation cache-poisoning vector the PR was filed to fix. Cache-directory resolution, hash seeding, and metadata validation are unchanged. The deleted bundler structs were unreferenced (verified in my earlier reviews), so their removal cannot affect any code path.
Level of scrutiny
Moderate-to-high. The mechanical diff is tiny, but the transpiler cache sits on the module-loading hot path and decides which transpiled bytes JSC executes — a wrong answer here is silent miscompilation, which is exactly what the original bug report described. The PR also went through a mid-stream design pivot (version-number offset → disjoint filename suffix) in response to a "how is this expected to load and be consistent?" question, and the chosen answer (.pile2 coexists, .pile is never read/trusted/deleted) is a long-term cross-line compatibility commitment that a maintainer should ratify rather than a bot.
Other factors
I have reviewed this PR across four prior rounds; every comment I raised (dead mirror constant, stale module-doc pointers, stale PR description, .pile in user docs, the third 50 kb mention) is now resolved and visible in the diff. The bug-hunting system found nothing on the current head. CI was reported green for both touched test files across all 15 lanes, with the red lanes being repo-wide breakage (bunx Angular engine check, darwin PTY flake, windows baseline infra) unrelated to this diff. No CODEOWNERS apply to the touched paths. The only open thread is a CodeRabbit prose-style nitpick on a pre-existing docs sentence, which the author reasonably scoped out.
robobun
force-pushed
the
farm/ad2450b3/transpiler-cache-version-namespace
branch
from
1466ba9 to
319675f
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Rebased onto main (319675f). One conflict with de73fda resolved: the On consistent data layout (the question I initially misread as being about cross-line thrash): Yes. I kept the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/runtime/environment-variables.mdx`:
- Line 199: The documentation on lines 199, 210, and 228 describing the
BUN_RUNTIME_TRANSPILER_CACHE_PATH behavior incorrectly states "larger than 4 KB"
when the actual runtime behavior is "4 KB or larger". Replace all three
instances of "larger than 4 KB" with "4 KB or larger" to accurately reflect that
files of exactly 4 KB are included in caching, not excluded. This clarifies the
threshold boundary from a greater-than comparison to a greater-than-or-equal
comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5f73b1bf-1941-418e-84a3-ea1bb118d2ca
📒 Files selected for processing (3)
docs/runtime/environment-variables.mdxsrc/ast/transpiler_cache.rssrc/bundler/cache.rs
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the runtime delta is just the .pile → .pile2 suffix and the 22 → 23 version bump, with a solid regression test.
Extended reasoning...
Overview
The load-bearing runtime change is two string literals in write_cache_filename (.pile/.debug.pile → .pile2/.debug.pile2) plus EXPECTED_VERSION 22 → 23 in src/jsc/RuntimeTranspilerCache.rs, accompanied by a thorough doc comment explaining the Zig/Rust-line cache-poisoning failure mode. Everything else is cleanup that fell out of review iteration on this PR: deletion of the dead RuntimeTranspilerCache{Entry,Metadata} mirror block in src/bundler/cache.rs (verified unreferenced in earlier review rounds), three doc-comment pointer fixes in src/ast/transpiler_cache.rs / src/bundler/cache.rs, the user-facing docs page updated to .pile2 and the correct 4 KB threshold, a new regression test that forges a hash-valid .pile entry and asserts it's never loaded or deleted while a fresh .pile2 is written, and a one-line filter widening in test/regression/issue/30887.test.ts.
Security risks
None. The cache remains content-addressable under the same per-user directories and the same wyhash; only the filename suffix changes. No new inputs are trusted — if anything, strictly fewer on-disk files are read than before (legacy .pile files are now ignored entirely rather than version-checked-and-unlinked).
Level of scrutiny
Moderate code path (every >4 KB module on warm runs goes through this), but the code delta is mechanical: a constant bump and a suffix swap at the single filename-construction site. The conceptual design question (filename namespace vs. version-number offset) was already iterated on mid-PR, settled in 559ef8f/23d298a7, and is exhaustively documented in the EXPECTED_VERSION doc comment. The new test directly exercises the exact poisoning scenario the PR fixes and would fail on a pre-fix build.
Other factors
I left five rounds of inline comments on earlier revisions (dead mirror structs, stale module-header doc, stale PR metadata, stale .pile/50 KB docs, and the missed lowercase "50 kb" table row); every one has been addressed and resolved. The bug-hunting system found nothing on the current revision. CI was green on the touched test files per the earlier triage; remaining red was repo-wide and unrelated. No CODEOWNERS paths match. No outstanding human reviewer comments.
…g space The Zig 1.3.x maintenance line shares the runtime transpiler cache's directory, filename scheme, metadata layout, and feature-hash inputs, but transpiles with a different implementation and bumps its own expected_version independently (18 in bun-v1.3.12, 20 in bun-v1.3.13/1.3.14). When both lines use the same number — as happened for version 20 — each implementation loads entries written by the other: every stored hash verifies, so a foreign entry is served indefinitely, never unlinked, and survives version up/downgrades within the line that trusts it. Reserve versions >= 1_000_000 for the Rust line so entries written by the other line always fail the cache_version check, are unlinked, and get rewritten.
RUNTIME_TRANSPILER_CACHE_VERSION (stale at 20), CacheEncoding, MetadataModuleType, RuntimeTranspilerCacheEntry and RuntimeTranspilerCacheMetadata in src/bundler/cache.rs had no references; the real on-disk Entry/Metadata live in src/jsc/RuntimeTranspilerCache.rs and reach the parser through the vtable bridge. Also point the bun_ast::RuntimeTranspilerCache.entry doc comment at the concrete type that is actually boxed there.
The //! header still pointed readers at bun_bundler::cache for the concrete Entry/Metadata types deleted in the previous commit; they live in bun_jsc::runtime_transpiler_cache.
…n offset A version-number offset still left both implementation lines reading and writing the same <hash>.pile files: whichever bun ran last unlinked and rewrote the other line's entries, so alternating bun versions could never keep a loadable, consistent cache. Write <hash>.pile2 (debug: .debug.pile2) instead. The two caches are now fully disjoint: an entry only ever loads in the implementation line that wrote it, both coexist when installed bun versions alternate, and .pile files are never read, trusted, or deleted. EXPECTED_VERSION returns to a plain 23.
The runtime transpiler cache writes .pile2 files now (1.3.x uses .pile), and the minimum cached source size is 4 KB, not 50 KB (MINIMUM_CACHE_SIZE in src/jsc/RuntimeTranspilerCache.rs).
robobun
force-pushed
the
farm/ad2450b3/transpiler-cache-version-namespace
branch
from
319675f to
c18495b
Compare
2 participants
Symptom
Reported pattern (second-hand, via a user who tried the Rust-port canaries): a test suite worked once, started failing permanently on the second run, kept failing after downgrading bun, and was only "fixed" by recreating the project. Classic runtime transpiler cache poisoning — except downgrading should have invalidated the cache, and it didn't.
Cause
The Zig 1.3.x maintenance line and the Rust line share the runtime transpiler cache byte for byte: same directory (
~/.cache/bun/@t@, …), same content-hash.pilefilenames (wyhash seed 42, kept deliberately interchangeable), same metadata layout, same feature-hash inputs — but they transpile with different implementations and bumpexpected_versionindependently:Two failure modes, both from sharing one filename namespace:
input_hash,features_hash, and all payload hashes verify (they hash the entry's own payload) — so foreign transpiled output (including early-port transpiler bugs likebun test --paralleldoes not wait for async preload to finish #30887) is served indefinitely, never unlinked, and survives version up/downgrades within the line that trusts it.Fix
Stop sharing the namespace: the Rust line writes
<hash>.pile2(debug builds:.debug.pile2). The two caches are fully disjoint — an entry only ever loads in the implementation line that wrote it, both caches coexist when bun versions alternate, and.pilefiles are never read, trusted, or deleted (they belong to the Zig line, which versions them independently).EXPECTED_VERSIONstays a plain23and only has to be consistent with this implementation. Also deletes the deadRuntimeTranspilerCache*mirror structs (incl. a stale version constant) frombun_bundler::cache.Verification
New test in
test/cli/run/transpiler-cache.test.tsforges the entry a Zig-line bun would have written for the exact same source — legacy.pilename, matching input/feature hashes, valid self-hashes, different (sentinel) transpiled output:Expected: "original-…" Received: "POISONED"..pile2written alongside), the.pilefile is byte-identical afterwards (never loaded, never deleted), and the third run hits the.pile2entry without the sentinel ever surfacing.bun bd test test/cli/run/transpiler-cache.test.ts— 12/12 pass;test/regression/issue/30887.test.ts— 5/5 pass (its cache-file filter updated for the new suffix). Also swept ESM/CJS/TLA/import defer/TS-enum/non-ASCII/error-stack modules across 3 runs each against a fresh cache to confirm no run-to-run divergence.Related but distinct: #31758 fixes a same-family poisoning between plain-run and
bun testentries.