transpiler cache: continue a short write from the unwritten bytes - #39719
transpiler cache: continue a short write from the unwritten bytes#39719robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reachedYour included review limit has been reached. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset (next review available in 3 minutes), then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: ready for review. Land #39717 first, this PR rebases on top of it in a minute. Reproduction: no filesystem in a test produces a short write that a retry can finish, so debug builds read An earlier head of this PR (adb700a) used one |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the short-write continuation logic is correct and the fix is well-contained.
What was reviewed:
- Traced
advance_sections+ the rebuilt-per-iteration iovec loop: empty/already-written sections are skipped vialen == 0 → continue,budgetis guarded> 0sovecs_lenis never 0 inside the loop, and the common path (no cap, one write) is unchanged. - Verified the test's header offsets (38/62/86) against
Metadata::encodefield order; they match. - Checked the env-var hook follows the existing
BUN_DEBUG_*pattern and costs one cached atomic load per save when unset.
Extended reasoning...
Overview
This PR fixes a latent bug in Entry::save (src/jsc/RuntimeTranspilerCache.rs): when pwritev returns a short count, the old loop retried with the same iovecs at an advanced offset, producing a corrupt cache file that the next run's hash check would reject and unlink. The fix tracks the unwritten remainder of each section in pending: [&[u8]; 4], rebuilds iovecs from pending each pass, and advances the slices via a small advance_sections helper. A new BUN_DEBUG_TRANSPILER_CACHE_MAX_WRITE env var (following the BUN_DEBUG_TEST_TEXT_LOCKFILE / BUN_DEBUG_FORCE_NIX_HOST pattern) caps each write so the continuation path is testable without a filesystem that produces real short writes. Two tests are added to the existing test/cli/run/transpiler-cache.test.ts.
Security risks
None. The change only affects how a process writes its own transpiler-cache tempfile before renaming it into place. No untrusted-input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Moderate. The write loop is native I/O code, but the change is small, mechanical, and self-contained: one loop body + a 9-line helper + an env-var declaration. The Rust borrow checker enforces the iovec lifetimes. I verified: advance_sections correctly trims a prefix of the section concatenation; the iovec-building loop skips empty and budget-exhausted sections so pwritev is never handed a zero-length vec array; max_write is guarded > 0; the common path (one write, no cap) still builds all four iovecs in one pass and exits after one iteration.
Other factors
- The test is strong: byte-for-byte comparison against a single-write baseline at two cap values (100 cuts inside the header so a single
advance_sectionscall both drops a finished section and trims a partial one; 4093 cuts inside every section), plus a check that the pieced-together file is actually served on the next run (inode/mtime unchanged). The debug-only log-count test proves the hook engages, so a future refactor that silently ignores the cap would fail CI. - I cross-checked the test's hardcoded header offsets (38, 62, 86 for the three
byte_lengthfields) againstMetadata::encode's field order — they are correct. - The PR description is unusually thorough, including a fail-before analysis and an explanation of why a real short write can't be forced in a test.
- No prior reviewer comments to address; the bug hunting system found nothing.
2c663ad to
24c396e
Compare
|
Updated 7:40 AM PT - Aug 20th, 2026
✅ @robobun, your commit 04df944832453993f8a8d79784bd04b97a1353df passed in 🧪 To try this PR locally: bunx bun-pr 39719That installs a local version of the PR into your bun-39719 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
24c396e to
adb700a
Compare
robobun
changed the title
There was a problem hiding this comment.
LGTM — the description now matches the committed 4×pwrite_all shape, and the Notes section states the reason for dropping the written == 0 guard, so both of my earlier findings are addressed.
What was reviewed:
- Verified the section offsets computed once at the top are the same values written into the header and used as
pwrite_alloffsets — no drift between the two. - Confirmed
sys::File::borrowis a#[repr(transparent)]view that does not close the fd, so_close_guardremains the sole owner. - Confirmed the pattern matches
NodeCompileCache.rs:1048-1051(sequentialpwrite_allat fixed offsets). - Checked that empty sections are handled:
pwrite_allon an empty slice is a no-op, and the reader accepts zero-length sections.
Extended reasoning...
Overview
The PR replaces the pwritev retry loop in Entry::save (src/jsc/RuntimeTranspilerCache.rs) with four sequential File::pwrite_all calls at header-recorded offsets. Section offsets are hoisted to locals and reused for both the Metadata struct fields and the write offsets, eliminating duplicated arithmetic. Net -26 lines. On-disk format is unchanged (author verified byte-identical output vs 1.4.0).
What changed since my last review
The code is on the same commit (adb700a). The PR description and status comment were rewritten to accurately describe the pwrite_all approach — my earlier 🔴 finding about the description/code mismatch no longer applies. The Notes section now also states a reason for the dropped written == 0 → WriteFailed guard (POSIX regular files never return 0 for a non-empty write; if one did, Entry::load's size/hash checks reject it), which satisfies REVIEW.md's "every deletion needs a stated reason" for the 🟡 nit.
Security risks
None. The transpiler cache is a local perf optimization keyed by content hash; worst-case failure is a rejected cache entry and a re-transpile. No untrusted input parsing was added.
Level of scrutiny
Low-to-medium. Small mechanical change to a non-critical path (cache write, once per source file), directly copying the shape already in NodeCompileCache.rs:1048-1051. File::borrow is a transparent view (src/sys/file.rs:60) so fd ownership stays with _close_guard. The old code skipped empty sections from the iovec; pwrite_all on an empty slice returns immediately (while !buf.is_empty()), so behavior is preserved.
Other factors
The "no new test" argument is defensible: the short-write retry now lives inside the shared pwrite_all helper, and REVIEW.md says not to add production code solely to make a test writable — the first revision's env-var hook was exactly that. The happy path (which is now the only path in this function) is covered by the four existing suites the author ran (44 pass). The remaining 🟡 about pwrite_all returning Ok(()) on n==0 is a pre-existing property of the shared helper, applies equally to NodeCompileCache, and the author has stated the reason; changing it belongs in a separate PR against src/sys/file.rs. The "land #39717 first" note is a merge-order concern for the author, not a review blocker.
adb700a to
088994c
Compare
robobun
changed the title
Entry::save writes a cache entry with one pwritev over the header, the output, the sourcemap and the esm record. When the write was short, the loop called pwritev again with the same iovecs, so the start of every section was written again at the new offset and the damaged file was renamed into place. save now keeps the unwritten rest of each section and builds the iovecs of every write from that. The common case is still one pwritev. Debug builds read BUN_DEBUG_TRANSPILER_CACHE_MAX_WRITE, a cap on the bytes handed to one write, and log every write that save continues after. The test uses it to save an entry in 172 and in 5 writes and compares the result with the entry written in one write.
088994c to
04df944
Compare
There was a problem hiding this comment.
LGTM — the earlier concerns are all addressed: title/description now match the committed pwritev + advance_sections implementation, the test + BUN_DEBUG_TRANSPILER_CACHE_MAX_WRITE hook are present and verified fail-before/pass-after, and the written == 0 guard is preserved.
What was reviewed:
- Loop invariants in
Entry::save:budget -= lencannot underflow (len = section.len().min(budget)),vecs_len >= 1wheneverposition < end_position, andadvance_sectionstrims exactly the reported prefix (written <= sum(iovecs) <= sum(pending)). - Release path:
max_write = usize::MAXreduces to the original single-pwritevcommon case; the env var declaration follows the unconditional pattern of neighboringBUN_DEBUG_*hooks. - Test: derives write counts from entry size (not hardcoded), checks byte-identical output, and the inode/mtime check proves the piecewise-written entry is accepted on the next run.
Extended reasoning...
Overview
Fixes a short-write bug in Entry::save (src/jsc/RuntimeTranspilerCache.rs): the old loop retried pwritev with the original iovecs after a short write, so each section restarted at the new offset and the renamed-in entry was corrupt (rejected by hash on next load → one wasted transpile). The fix keeps a pending: [&[u8]; 4] array of unwritten section tails, rebuilds iovecs from it each iteration, and trims via advance_sections. A debug-only BUN_DEBUG_TRANSPILER_CACHE_MAX_WRITE env var caps each write so the retry path is testable, and a new test.skipIf(!isDebug) test in transpiler-cache.test.ts verifies byte-identical output for 1/172/5-write saves plus cache-hit on the next run.
Security risks
None. This is the transpiler cache write path — no untrusted input reaches the changed code (all four sections are Bun-generated), and the worst case on any residual bug is a hash-rejected cache entry and a re-transpile. The debug hook is #[cfg(bun_debug)]-gated and does not exist in release builds.
Level of scrutiny
Medium. The loop logic is nontrivial but small and self-contained; I traced the invariants (no underflow on budget -= len; vecs_len never 0 while bytes remain; advance_sections can never over-trim since written <= sum(iovecs) <= sum(pending)). The release path with max_write = usize::MAX is behaviorally identical to the old happy path (one pwritev). The env-var declaration matches the neighboring BUN_DEBUG_TEST_TEXT_LOCKFILE / BUN_DEBUG_FORCE_NIX_HOST pattern.
Other factors
This PR has already been through three review rounds. My earlier 🔴 (missing test/hook, description mismatch) and 🟡 (pwrite_all 0-byte return, second description mismatch) findings are all resolved — the code no longer calls pwrite_all, keeps the explicit if written == 0 { return Err(WriteFailed) }, and the description accurately describes what landed (with the pwrite_all alternative noted only as an option). The robobun evidence gate confirms the new test fails on main (1 write vs 172) and passes with the fix, and skips cleanly on release. The comment-cop flags were addressed by shortening comments to one line. The description notes a merge-order dependency on #39717 for whoever merges.
1 participant
Problem
Entry::save(src/jsc/RuntimeTranspilerCache.rs) writes a cache entry with onepwritevover four buffers. After a short write it calledpwritevagain with the same iovecs, so every section started again at the new offset.tsc --help2x faster #7365 (2023), no reports. Found in Verify a transpiler cache entry before any field of it is used #39717.Fix
savekeeps the unwritten rest of each section inpending, builds the iovecs of every write from it, andadvance_sectionsdrops the written bytes. The common case is still onepwritev. Thewritten == 0check stays.pwritevwrites the buffers in order: the written bytes are a prefix of the pending sections, and writing the rest atpositiongives one complete write.BUN_DEBUG_TRANSPILER_CACHE_MAX_WRITE, a cap on the bytes per write, and log each continued write. Release builds contain neither. The test saves one entry in 1, 172 and 5 writes, checks the counts, that the entries are identical, and that the next run is served from the last one.bun bd test test/cli/run/transpiler-cache.test.ts(14 pass). Without the fix the new test fails: 1 write instead of 172.Background
savewrites a temporary file and renames it, so a reader sees a complete entry or none.pwritev(2)writes a list of buffers (iovecs) at one offset in one system call. Likewrite(2), it may write less than requested and returns the count.cfg(bun_debug)marks debug builds. The-debugversion suffix (isDebug) and the debug logs exist under the same cfg as the hook.Notes
Land #39717 first. Both change the same part of
save. The test reads one header field (esm record length at offset 86), which #39717 does not move.Why a hook. A short write that a retry can finish needs a filesystem that produces one (FUSE, network filesystems) or a single write above 2 GiB (
MAX_RW_COUNTon Linux).RLIMIT_FSIZEandENOSPCalso produce a short write, but the retry fails too, so the old and the new loop both unlink the temporary file. On Windowsuv_fs_writereturns a short count when a later buffer fails, and the retry fails the same way. The hook follows the testing hooks already inenv_var.rs(BUN_DEBUG_FORCE_NIX_HOST,BUN_DEBUG_TEST_TEXT_LOCKFILE, andBUN_DEBUG_ENABLE_RESTORE_FROM_TRANSPILER_CACHEin this same file). It is an env var because the save happens in the spawnedbun test --isolateprocess. It is debug-only so that release builds get the loop fix and nothing else (plaincargo clippycompiles that arm,bun bdthe other).Alternative. Commit adb700a (an earlier head of this PR) replaced the loop with one
File::pwrite_allper section, asNodeCompileCache.rsdoes. That is 26 lines shorter, but leaves nothing a test can observe and drops thewritten == 0check. If that shape is preferred without a test, say so and I will switch back.What the test covers. With a cap of 100 the first write stops inside the header, and the second write hands the kernel the last 2 header bytes plus 98 bytes of output, so one
advance_sectionscall drops a finished section and trims a partly written one. With 4093 the cuts fall inside every section. The write counts come from the log line, so a cap that stops engaging fails the test. The final run checks inode and mtime: a rejected entry is unlinked and written again. The counts 172 and 5 are for the current 17140 byte entry; the test derives them from the entry size.What the old loop produced. The first N bytes of the entry, then the whole entry again at offset N. With N at or past the header, the output hash check rejects it on the next run. Below that, the header is torn and the metadata checks reject it. On a filesystem that short-writes regularly, every save of a large file was wasted and the file was transpiled on every run.
Also run with
bun bd test:test/cli/test/isolation.test.ts,test/regression/issue/30887.test.ts,test/regression/issue/28159.test.ts(31 pass),test/internal/source-lints(160 pass),cargo clippy -p bun_jsc -p bun_core,cargo fmt, prettier.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file