Run pool jobs that own their memory without a VM borrow so worker.terminate() does not wait on a blocked Bun.write(file, file)

[robobun]

Problem

• Since Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075, worker.terminate() never settles if the worker has a Bun.write(file, file) in the thread pool whose source is a FIFO, a tty or a pipe nobody is writing to (this includes the documented Bun.write(Bun.stdout, Bun.stdin)). 1.3.14 settled it in a few ms.
• Job::run_on_pool (src/jsc/job.rs) takes a VM borrow around every JobContext::run, and VmHandle::close() in the worker's teardown waits for the borrow count to reach zero.
• CopyFile::run (src/runtime/webcore/blob/copy_file.rs) does the entire copy inside run, and OPEN_SOURCE_FLAGS has no O_NONBLOCK, so the pool thread sits in open(2) (or the read) holding the borrow until the other end of the FIFO shows up. getaddrinfo (dns.lookup, src/runtime/dns_jsc/dns.rs) and Bun.secrets (a keychain prompt) hold it the same way.
• Of the 19 JobContext impls, 5 actually use the borrow in run. Most of the others still need it: their off-thread half reads the caller's JS buffers or writes into a JS-allocated one. The file jobs, dns, secrets, password, Glob.scan and Archive own everything they touch, except that a file store could hold a PathLike::Buffer: Bun.file(bytes) / Bun.write(bytes, ...) kept a pointer into the caller's ArrayBuffer in the Store, pinned, so the path followed later writes to the buffer (the docs say the buffer is copied), the protect() taken for it was never released, and releasing such a store off the JS thread would unpin() a dead heap.

Fix

• JobContext gets type Vm, which is what run holds on the VM: Borrow (as before: the carrier borrows, teardown waits) or the new Unborrowed (the carrier only checks VmHandle::is_closed() to skip a job whose VM is already gone; a VM torn down under a running body refuses the completion and the job is released on the pool thread, the path that already existed for a post that loses the race with close()). The type is sealed to those two, and JsPtr::under_borrow still requires a &Borrow, so a body can only reach JS memory if it declared Borrow.
• CopyFile, ReadFile, WriteFile, the libc dns.lookup, Bun.secrets, Bun.password, Glob.scan and Bun.Archive declare Unborrowed. Bun.secrets stops carrying the global: its C++ runTask never read it. zstd, pbkdf2, scrypt, randomFill, the extern crypto jobs, CompressionStream, Image, Transpiler and both node:fs jobs declare Borrow, each with a one-line reason where run does not name the borrow (the node:fs case is Don't block worker/VM teardown on fs thread-pool ops that never complete #37170's).
• Store::init_file / init_s3 now make the path the store's own: a Buffer path is copied into owned bytes on the JS thread (which drops the pin there), string paths go through to_thread_safe() as before (moved out of find_or_create_file_from_path). With that, the three file jobs hold only native memory, so running them unborrowed and releasing them on a pool thread is sound.
• Verified:
  • test/js/web/workers/worker-terminate-lifetime.test.ts, new test: a worker's Bun.write(out, Bun.file(fifo)) blocked in open(2), then terminate(). Times out on the unfixed build, settles in ~200ms on the debug build.
  • test/js/bun/util/bun-file.test.ts, new test: Bun.file(bytes) and Bun.write(bytes, file) keep their path when the buffer is changed afterwards (Uint8Array and ArrayBuffer). Fails on the unfixed build.
  • test/js/web/workers/worker-refused-completion.test.ts: new Bun.write(file, file) row, so CopyFile's pool-thread release runs under ASAN like the other producers'. All 17 rows pass with CI's LSan settings.
  • worker-terminate-funnels, worker_threads.test.ts (122), bun-write.test.js, archive.test.ts, password.test.ts, glob/scan.test.ts, source lints, cargo clippy -p bun_jsc -p bun_runtime pass on the debug build. The pre-existing failures in this container are unrelated: the bun-write copy_file_range and glob node_modules cases exceed their timeouts under debug (test: stop the Bun.write copy_file_range fallback test from starving its concurrent siblings #37792), the c-ares terminate test's LSan report is node:fs: mark the per-VM Binding box as LSan-ignored (fixes worker-terminate-lifetime.test.ts on main) #35159, and dns/secrets tests need network / libsecret.

Background

• A Job is the carrier for work a VM sends to the thread pool: an off-thread part the pool body runs, and a JS-side part (promise, callback) the VM itself releases at teardown. When the body finishes, it posts a completion back through the VM's VmHandle; after teardown has closed the handle the post is refused and the job frees its off-thread part right there on the pool thread.
• A VM borrow (VmHandle::borrow) is how off-thread code says it is reading memory the VM owns, such as the bytes of an ArrayBuffer a caller passed to fs.write. Teardown closes the handle only once no borrow is held, so for a body that blocks on something external, holding one turns terminate() into a wait on that external party. Before Worker / worker_threads: WebCore-shaped lifetimes, joined threads, one ordered VM teardown #37075 these jobs were not waited for at all; this keeps the wait for the jobs that need it.
• A Store is the refcounted backing of a Blob: bytes, or for Bun.file() a path or fd. File jobs keep a ref to it while they run, so whichever thread drops the last ref frees it; that is why it cannot refer to anything in the JS heap.
• PathLike is the parsed form of a path argument; its Buffer variant borrows the caller's ArrayBuffer and pins it (so it cannot be detached or moved) until the PathLike is dropped, which is only safe on the JS thread.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/web/workers/worker-refused-completion.test.ts test/js/web/workers/worker-terminate-lifetime.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 3 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 35d91118-75ec-4f69-b592-8c9814010ba1

📥 Commits

Reviewing files that changed from the base of the PR and between b555e06 and 3a626be.

📒 Files selected for processing (24)

• src/jsc/JSSecrets.rs
• src/jsc/VmHandle.rs
• src/jsc/bindings/JSSecrets.cpp
• src/jsc/job.rs
• src/jsc/lib.rs
• src/runtime/api/Archive.rs
• src/runtime/api/BunObject.rs
• src/runtime/api/JSTranspiler.rs
• src/runtime/api/glob.rs
• src/runtime/crypto/PBKDF2.rs
• src/runtime/crypto/PasswordObject.rs
• src/runtime/dns_jsc/dns.rs
• src/runtime/image/Image.rs
• src/runtime/node/node_crypto_binding.rs
• src/runtime/node/node_fs.rs
• src/runtime/webcore/Blob.rs
• src/runtime/webcore/CompressionStreamCoder.rs
• src/runtime/webcore/blob/Store.rs
• src/runtime/webcore/blob/copy_file.rs
• src/runtime/webcore/blob/read_file.rs
• src/runtime/webcore/blob/write_file.rs
• test/js/bun/util/bun-file.test.ts
• test/js/web/workers/worker-refused-completion.test.ts
• test/js/web/workers/worker-terminate-lifetime.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 1:05 AM PT - Aug 14th, 2026}

❌ @robobun, your commit 3a626be has some failures in Build #95297 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38312

That installs a local version of the PR into your bun-38312 executable, so you can run:

bun-38312 --bun

[robobun]

Status: superseded in part. The Store fix is now #38456 on top of the merged ticket model (#38299); the teardown part of this PR no longer applies to main and is on hold until a maintainer picks between the two options in #38312 (comment).

CI on 3a626be (build 95297): 177 jobs passed; the two darwin 14 aarch64 - test-bun shards never got an agent and expired after ~6 hours in the test-darwin queue (nothing ran on them), and the two flaky files (test-fs-read-stream-pos.js, test-cluster-disconnect.js) passed on retry and are unrelated. The macOS shards are the only coverage of this change CI has not executed; retrying just those two jobs would complete it.

Reproduced on canary 1.4.0-canary.1+da3851e57 and on a debug build of ada9163f2: a worker running Bun.write(out, Bun.file(fifo)) (no writer on the FIFO) is terminated; terminate() is still pending after 3s on both. With this branch it settles in about 200ms on the debug build, and the same script is the new test in test/js/web/workers/worker-terminate-lifetime.test.ts.

[claude]

I reviewed this PR and the automated bug hunt found no issues. Because it changes the core JobContext contract (the new type Vm: VmHold split) and reclassifies eight jobs as safe to run while their VM tears down — each an individual memory-safety claim — a human look at the Unborrowed classifications and the Store path-ownership change would still be worthwhile.

What was reviewed:

• The VmHold sealed trait: JsPtr::under_borrow still requires &Borrow, so an Unborrowed body cannot type-check a JS-memory access.
• own_path covers the removed to_thread_safe() in find_or_create_file_from_path; the two direct Data::File(...) constructors that bypass init_file (stdio in jsc_hooks.rs and BunObject.rs) both use Fd, not PathLike::Buffer.
• Bun__SecretsJobOptions__runTask's dropped global parameter — confirmed the C++ body never read it.

Extended reasoning...

Overview

This PR fixes a regression where worker.terminate() hangs indefinitely if the worker has a blocking pool job (e.g. Bun.write(file, Bun.file(fifo)), getaddrinfo, a keychain prompt) in flight. The fix adds an associated type type Vm: VmHold to JobContext (src/jsc/job.rs), sealed to Borrow (VM teardown waits for the body, as before) and the new Unborrowed (teardown does not wait; a completion posted after close is refused and released on the pool thread). All 19 JobContext impls are classified across 15 runtime files. Additionally, Store::init_file/init_s3 now copy a PathLike::Buffer path into owned bytes (own_path) so file stores hold no JS-heap references — a prerequisite for the file jobs' Unborrowed claim, and a fix for a pre-existing doc-vs-behavior mismatch (the buffer was documented as copied but was actually borrowed and pinned, with the pin never released). Three tests cover the terminate hang, the buffer-copy semantics, and the pool-thread release path under ASAN.

Security risks

None identified. The change is about thread-lifetime coordination inside the runtime; no auth, crypto semantics, or user-input parsing is altered. The crypto job impls only add type Vm = Borrow (preserving existing behavior).

Level of scrutiny

High. This touches the core VM-handle / thread-pool lifecycle (src/jsc/VmHandle.rs, src/jsc/job.rs) and each Unborrowed declaration is an assertion that the job's OffThread and its Drop touch nothing of the JS heap — getting one wrong is a use-after-free on a pool thread after VM teardown. The own_path change in Store.rs is load-bearing for three of those assertions. The design (a sealed trait with two impls, dispatched via C::Vm::acquire) is a new abstraction that every future JobContext author must choose correctly, so a maintainer should sign off on the shape.

Other factors

• I spot-checked the two places that construct Data::File directly without going through init_file (__bun_stdio_blob_store_new in jsc_hooks.rs and build_store in BunObject.rs): both use PathOrFileDescriptor::Fd, so they cannot carry a PathLike::Buffer and don't need own_path.
• The to_thread_safe() removed from find_or_create_file_from_path in Blob.rs is subsumed by own_path inside init_file/init_s3, which is called downstream on both the file and S3 branches.
• The Unborrowed::acquire snapshot of is_closed() is racy by design (the doc says so): a body that starts against a still-open VM and then loses the race has its completion refused via the existing Postable::release_refused path — no new mechanism.
• Test coverage is good: the FIFO test would hang on the unfixed build, the byte-path test asserts the documented copy semantics, and the refused-completion row runs CopyFile's pool-thread release under ASAN. The PR description enumerates which surrounding suites were run.
• No prior human or bot review comments to address; CI is still building.

[robobun]

Follow-ups since the PR was opened:

• The review's spot check of the Data::File constructors that bypass init_file holds: the only two (build_store in BunObject.rs and __bun_stdio_blob_store_new in jsc_hooks.rs) are fd-backed stdio stores, and all three S3 store constructors go through init_s3, so own_path covers every path-carrying store.
• 056afb7: the new bun-file.test.ts case no longer asserts the copy's resolved byte count. The Windows file-to-file copy resolves 0 while writing the content correctly (Bun.write: fix file-to-file copy resolving 0 bytes on Windows and macOS overwrite #33715); the content checks are what this test is about, and they pass on the Windows lanes.
• e61cb3d: the comments comment-cop flagged are down to one line each (its run on that commit finds none left; the threads are resolved by hand because the workflow cannot resolve them itself, see ci: use a PAT to resolve stale comment-cop review threads #36959).

[claude]

I reviewed this PR and didn't find any bugs (the earlier nit on PipelineTask's type Vm doc comment was addressed in 3a626be). Because it changes the VM-borrow contract for thread-pool jobs and reclassifies eight of them as safe to run and release without one — a per-impl memory-ownership judgement whose failure mode is UAF under worker teardown — a human look would still be worthwhile.

What was reviewed:

• VmHold sealing and run_on_pool's C::Vm::acquire dispatch — JsPtr::under_borrow still requires &Borrow, so an Unborrowed body cannot reach JS memory through the typed path.
• own_path in Store::init_file/init_s3: confirmed the removed to_thread_safe() in find_or_create_file_from_path flows into init_file at Blob.rs:3707, and the fd-backed stdio / output_file_jsc constructors carry no PathLike::Buffer.
• The Borrow/Unborrowed split across all 19 impls matches the PR description; each Borrow impl that binds _vm now carries a one-line reason.

Extended reasoning...

Overview

This PR introduces an associated type JobContext::Vm: VmHold (sealed to Borrow | Unborrowed) so that thread-pool jobs whose off-thread body owns everything it touches no longer hold a VM borrow while running. VmHandle::close() waits for outstanding borrows, so a job blocked in open(2) on a FIFO (or a keychain prompt, or getaddrinfo) previously made worker.terminate() hang forever. Eight jobs are reclassified as Unborrowed (CopyFile, ReadFile, WriteFile, libc dns.lookup, Bun.secrets, Bun.password, Glob.scan, Bun.Archive); eleven keep Borrow. To make the file jobs' Unborrowed classification sound, Store::init_file/init_s3 now copy a PathLike::Buffer into owned bytes on the JS thread (fixing a documented-but-unimplemented copy semantic and a leaked protect()). Bun__SecretsJobOptions__runTask drops its unused global parameter. Three test files gain coverage.

Security risks

None identified. The change does not touch auth, crypto correctness, input validation, or trust boundaries. The risk profile is memory safety (UAF / off-thread heap access), not security.

Level of scrutiny

High. This is core VM/worker teardown lifecycle code. The correctness of each Unborrowed classification depends on the impl's off-thread body touching only owned memory and its Drop being sound off the JS thread — a wrong call is a UAF that only manifests under terminate() racing a running job. The own_path change alters what thread may drop a Store. The type-level enforcement (sealed trait, JsPtr::under_borrow requiring &Borrow) is sound, but it cannot catch a job that reads JS memory through a raw pointer rather than a JsPtr.

Other factors

The PR description is unusually thorough (per-impl reasoning, mechanism explanation, verification list). All comment-cop and prior-review threads are resolved. Tests cover the regression (FIFO-blocked terminate), the own_path behavioral fix (buffer mutation after Bun.file()), and the pool-thread release path under ASAN. I spot-checked that find_or_create_file_from_path's removed to_thread_safe() is subsumed by init_file's new own_path call, and that the non-init_file Data::File constructors are fd-only. Nothing looks wrong, but the breadth of per-job ownership reasoning across 24 files in memory-safety-critical native code warrants a maintainer's review rather than bot-only approval.

[robobun]

Where this PR stands after #38299 merged, and the question a maintainer needs to answer before it is reworked.

Split out: the Store part (a Bun.file(bytes) / Bun.write(bytes, ...) / s3:// byte path was kept as a view into the caller's ArrayBuffer, and the protect() taken on it was never released) is independent of the teardown model and is now #38456, on top of current main. This PR's remaining content is the teardown change, and the model it patched is gone: VmHandle::borrow / Borrow / Unborrowed no longer exist. On main every pool job holds a Ticket for its whole trip (Job::schedule, src/jsc/job.rs), and VmHandle::close_and_wait waits for every ticket; the stop phase shortens that by cancelling jobs that opt in (JobContext::CANCELLABLE + cancel), which today are ReadFile and WriteFile (the IoParking handshake). Everything else in flight is waited for, as Node waits for in-flight threadpool requests.

What still reproduces on main (7ba276f, debug build): a worker with Bun.write(out, Bun.file(fifo)) where nothing opens the FIFO's other end, then terminate(). CopyFile::run does the whole copy in the pool body and opens the source blocking, so terminate() never settles; the debug build reports teardown has waited 10s for 1 ticket(s) ... taken at src/runtime/webcore/blob/copy_file.rs:123. The same applies to anything else whose pool body waits on something external and has no cancel: dns.lookup through getaddrinfo, Bun.secrets (a keychain prompt). Node 26 behaves the same way for fs.copyFile of a FIFO inside a worker (terminate() does not settle), so main is consistent with Node here; this PR went further than Node. (One correction to the description above: Bun.write(Bun.stdout, Bun.stdin) in a worker does not hit this wait on main; it fails up front with EAGAIN from splice, which is a separate CopyFile bug being tracked on its own.)

The choice, the same one #37170 raises for the node:fs jobs:

1. Keep the wait (main's model, one invariant, Node's behaviour), and bound it per job where that is possible: CopyFile would get CANCELLABLE and a cancel, e.g. opening the source O_NONBLOCK and parking on the io thread the way ReadFile does, so terminate() settles for FIFO/tty/idle-pipe sources. Jobs that cannot be interrupted (getaddrinfo, a keychain prompt) keep blocking terminate() until they return, as in Node. This PR would then be replaced by a CopyFile cancellation PR.
2. Do what this PR did, in the new model: jobs whose body touches nothing the VM owns (the file jobs, libc dns.lookup, Bun.secrets, Bun.password, Glob.scan, Bun.Archive) run without a ticket, post their completion through the uncounted VmHandle and free the off-thread half on the pool thread if that is refused. terminate() is then bounded whatever the job is blocked on, but the VM has to release those jobs' JS halves (the promise) itself during teardown while the body may still be running, which brings back a separately tracked JS-side list for this class of job (what One door out of a VM's thread: tickets + a teardown that waits #38299 removed in favour of the single ticket count) and a second JobContext mode in which JsPtr is unavailable.

I am leaving this PR as is until one of those is picked; if it is (1), this PR closes and the CopyFile cancellation is a new small PR against main.

[robobun]

Update on the split-out Store part: #38456 is closed in favor of #38509, which fixes the same Store::init_file / init_s3 protect leak by pinning the backing ArrayBuffer instead of copying the path. The own_path hunk still in this branch conflicts with #38509 in Store.rs; if this PR is reworked as option (2) above (stores released on a pool thread), the path has to be copied rather than pinned, so that hunk would come back on top of #38509 rather than go away.

[robobun]

Re-checked on current main (97a4363) after the teardown rework in #38299. The scenario still reproduces, so this stays open, but the branch needs to be rebuilt on the new model.

Observed on a debug build of main, with only this PR's tests applied:

• The worker-terminate-lifetime test (worker runs Bun.write(out, Bun.file(fifo)) with nothing on the other end of the FIFO, then terminate()) never settles. The copy holds a ticket and the teardown report keeps naming it: teardown has waited 30s for 1 ticket(s) still held off-thread / taken at src/runtime/webcore/blob/copy_file.rs:123. CopyFile has no JobContext::cancel, so the stop phase has nothing to cancel and the wait is unbounded, which One door out of a VM's thread: tickets + a teardown that waits #38299 documents as the intended outcome for work that cannot be cancelled. Whether a copy blocked in open(2) should stay in that category or get a cancel path is the question a rebase has to answer: VmHandle::borrow and the Borrow/Unborrowed split this branch adds no longer exist.
• The worker-refused-completion row passes on main when written as a worker-late-completion row (ticket: "copy_file.rs"): a copy that completes during the wait is released cleanly.
• The blocking open is the only way to reach this on main. A FIFO that is already open on the other side rejects with "Non-regular files aren't supported yet" before the copy starts (same as before this PR), and inside a worker Bun.write(Bun.stdout, Bun.stdin) on an idle pipe currently fails immediately with EAGAIN from splice (a separate pre-existing bug, reported separately), so neither holds terminate().
• The Store::init_file byte-path copy and its bun-file.test.ts test are being handled separately in Bun.file(buffer) / async fs: hold the path buffer's backing store instead of rooting the JS object #38509.

[alii]

@robobun needs rebasing/conflicts

[alii]

@robobun needs rebasing/conflicts

[robobun]

The rebase is not mechanical: #38299 (merged a few hours after this opened) removed what this PR changes. Jobs now hold a Ticket for the whole trip and teardown waits for every ticket by design; VmHandle::borrow, Postable::release_refused and the refusal path no longer exist, so there is nothing left for an owned job to opt out of, and 14 of these files conflict.

The bug itself is still there: on today's canary (aec33f581, which has #38299 and #38436) the FIFO repro still leaves terminate() pending, because CopyFile is not cancellable and still opens its source blocking (Bun.file(fifo).text() now settles in ~80ms, since #38299 made ReadFile cancellable).

Reworking this PR into the shape that fits the new design: CopyFile opens the source with O_NONBLOCK (as ReadFile already does) and restores blocking mode afterwards, so a FIFO with no writer no longer parks the pool thread in open(2) and the copy otherwise behaves as before. The Bun.file(bytes) store fix is a separate concern and will move to its own PR. Making the copy itself cancellable (an idle pipe or tty source blocked in the read) would need CopyFile to park on the io loop like ReadFile; not doing that here without a nod, since #38299 files blocked pool work under Node parity.