Repository navigation
Conversation
…received A FileHandle in transferList travels to the worker as a bare fd inside workerData. The parent handle is detached at once, and only the worker turns the marker back into a FileHandle. A worker that exits before it unpacks its workerData left the fd open for the life of the process. Each marker now carries a one-slot SharedArrayBuffer. The thread that flips the slot first owns the fd: the worker when it deserializes the marker, or the parent when it rolls the transfer back or when the worker has exited. On 'close' the parent closes every fd that nobody claimed. An object in workerData that only imitates the marker key has no claim flag and is now delivered as plain data.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for one more day.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for one more day. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 23 seconds for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 8:04 PM PT - Sep 19th, 2026
❌ @robobun, your commit 805e28e has 2 failures in
🧪 To try this PR locally: bunx bun-pr 43541That installs a local version of the PR into your bun-43541 --bun |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The claim flag no longer needs the SharedArrayBuffer, Int32Array and Atomics globals, which user code can replace or remove. Two native helpers make the 4 shared bytes and flip them with a compare-and-swap. A marker is now also checked for its data: an object in workerData that has the marker key but no numeric data.fd, or no valid claim flag, stays plain data and cannot throw in the worker bootstrap. The worker claims the fd after it loaded node:fs and made the handle, so that a terminate() cannot land in that load with the fd already claimed.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the new native claim path in src/jsc/bindings/webcore/Worker.cpp (dynamicDowncast, isShared and byteLength guards run before impl()/data() are touched, and the CAS is seq_cst, so a wrong-typed or non-shared argument cannot reach the atomic) and the constructor rollback ordering in src/js/node/worker_threads.ts (the claim is allocated before kTransfer(), so nothing can throw between neutering a handle and recording it for restoreNeutered) — neither turned up a problem.
Extended reasoning...
The second push moved the claim flag into two host functions and added the data.fd shape check on the worker side, which changes the surface I reviewed last time. I re-read both files at HEAD: the C++ functions validate type before any non-throwing accessor, the only throwing call (throwOutOfMemoryError) is under a ThrowScope, and the JS side reaches every lifecycle exit (rollback, finalize, close) through the single closeIfUnclaimed helper so no fd can be closed twice. The one confirmed inline finding (shared buffers rejected by the serializer when useSharedArrayBuffer is disabled) is the remaining item for the author; the terminate-after-boot and nested-worker leaks are documented by the author as out of scope and were already raised earlier, so I am not repeating them.
The claim of a FileHandle in transit is now a random id in a process-wide native set. The thread that removes the id owns the fd. The marker carries the id as a plain number, so the transfer no longer depends on a SharedArrayBuffer that the structured clone must accept, and an object in workerData that imitates a marker cannot hold a live id. Also shorten the comments to one line each.
An object in workerData that fabricates the marker key no longer deserializes, because it cannot hold a live claim id.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the new native claim path in Worker.cpp (the lock is never held across anything that can enter JS, and the argument is type-checked before asNumber) and the one undelivered case the reclaim does not reach — a FileHandle transferred from a script in a disposed Bun.ModuleGraph — which is pre-existing and not introduced by this PR.
Extended reasoning...
This run re-read the reworked mechanism (native 53-bit claim id replacing the earlier SharedArrayBuffer flag) and the worker-side marker shape validation, which address the points raised in prior rounds. The remaining items examined and ruled out were the exception/lock discipline of the two new host functions and the disposed-ModuleGraph undelivered path, which leaks on the base branch as well. The change is not approved here because it adds cross-thread native state and fd-ownership logic that a human should still weigh, and because a posted nit remains open.
A detached handle transfers fd -1. Its claim id stayed in the native set because closeIfUnclaimed returned on the fd before it took the claim.
There was a problem hiding this comment.
I re-reviewed the latest push (through 6c5bebdc) and found no bugs; the nit from the previous round is addressed, since closeIfUnclaimed now takes the claim before the fd check. Because this introduces a process-wide native registry and a cross-thread fd-ownership protocol, a human look at the design would still be worthwhile.
What was reviewed:
- Claim lifecycle: every path that owns a marker (worker
deserializeJSTransferable,restoreNeutered,finalizeJSTransferables, the'close'reclaim) removes the id under the same lock, so an id is never left in the set once the worker has closed. - Ordering of the reclaim:
WebWorker::shutdowninsrc/jsc/web_worker.rspostsworkerGlobalScopeDestroyedonly after the worker VM is deallocated, andthread_mainreachesshutdown()even when termination lands beforestart_vm, so no worker-side claim can race the parent's#onClose. jsFunctionClaimJSTransferable:isNumber()is checked beforeasNumber(), NaN/Infinity/fractions/out-of-range fall tofalse, and the 53-bit ids can never collide with WTFHashSet<uint64_t>'s empty (0) or deleted (max) sentinels.- Marker imitations in
workerDatanow fall through as plain data (memoized as themselves) rather than throwing in the worker bootstrap; thetest.eachmatrix covers the shapes.
Extended reasoning...
Overview
The PR touches three files: src/js/node/worker_threads.ts (claim ids attached to in-band FileHandle markers, a shared closeIfUnclaimed helper used by rollback, finalize and the new 'close'-time reclaim), src/jsc/bindings/webcore/Worker.cpp (two host functions backed by a process-wide NeverDestroyed<HashSet<uint64_t>> under a static Lock, exposed at binding indices 17/18), and six new serial tests in test/js/node/worker_threads/worker_threads.test.ts. The goal is to close an fd exactly once when a transferred FileHandle is never deserialized by the worker (missing entry, early terminate(), handle not referenced from workerData), matching Node's TransferData destructor.
Security risks
None new. The claim id is a random 53-bit number that user data cannot guess in practice, and jsFunctionClaimJSTransferable validates the argument as an integral number in [1, 2^53) before touching the set, so a forged marker in workerData cannot claim or close a foreign fd; it is delivered as plain data. The registry is process-global by design (the parent VM mints, the worker VM consumes), and every accessor takes s_transferClaimsLock, so the REVIEW.md "never back per-VM state with globals" concern does not apply here: the state is genuinely cross-VM. closeSync failures are swallowed with a comment, which is the same pattern the existing finalizeJSTransferables used.
Level of scrutiny
High. This is cross-thread resource ownership with a new native primitive, and correctness depends on a sequencing argument (the 'close' event is dispatched from workerGlobalScopeDestroyed, which WebWorker::shutdown posts only after the worker VM is deallocated, so all worker-side claim attempts are complete before the parent reclaims). I traced that ordering in src/jsc/web_worker.rs and WorkerMessagingProxy.cpp and it holds, including the early-terminate path where thread_main goes straight to shutdown(). Still, the design choice (per-handle claim vs a native transfer owner) is one a maintainer should weigh, and the previously raised pre-existing gap (a worker that itself spawns a child worker and is torn down before the child's 'close' is delivered) is not addressed by this PR and was already noted in an earlier round.
Other factors
The multi-agent hunt exited on dry_streak with no findings. The single commit since the last review (6c5bebdc) does exactly what the last nit asked: closeIfUnclaimed claims first, then checks fd, so an fd -1 handle no longer leaves its id in the set. Tests are serial with a stated reason (they observe descriptor numbers), drain subprocess pipes concurrently, release resources via using/await using registered before assertions, and the BUN_JSC_useSharedArrayBuffer=0 subprocess test guards the plain-number design choice. I could not compile locally (no debug build and the WebKit headers are not in this checkout), so cryptographicallyRandomNumber<uint64_t>() availability rests on the author's reported debug-build test runs. No CODEOWNERS entry covers the changed files.
This branch has not been deployed
1 participant
Problem
FileHandlein thetransferListof anode:worker_threadsWorker leaks its fd when the worker never starts. That is a missing entry, or aterminate()that stops the thread first. The parent handle is already detached. Node closes the fd.workerData. Only the worker turns it back into aFileHandle, inunpackJSTransferables(src/js/node/worker_threads.ts:586). A worker that exits earlier never closes it.Fix
Worker.cpp). The marker carries the id. The thread that removes the id first owns the fd.FileHandle. The parent claims it in#onCloseand closes every fd that nobody claimed. The constructor rollback claims too.closefires after the worker VM is destroyed.test/js/node/worker_threads/worker_threads.test.ts, 7 fail without the fix. Also the whole file and Node'stest-worker-*tests (debug ASAN build). Self-reviewed: 9 concerns raised, 7 addressed, 2 rejected (Notes).Background
FileHandlenatively.packJSTransferablescallskTransfer(), which detaches the handle and returns{ fd, flag }. That object replaces the handle insideworkerDataas a marker.node:worker_threadsloads before user code. Its module body turns each marker into a newFileHandle.TransferDataobject. Its destructor closes an undelivered fd.Notes
Repro (
bun fh-leak.mjs missing,bun fh-leak.mjs terminate):missingterminateparentFd: -1, stillOpen: falseparentFd: -1, stillOpen: falseparentFd: -1, stillOpen: trueparentFd: -1, stillOpen: trueparentFd: -1, stillOpen: falseparentFd: -1, stillOpen: falseWhich line is the fix. The call
this.#reclaimJSTransferables?.()in#onClose. The claim exists so that this call, the worker, the rollback and the existing cleanup infinalizeJSTransferablescan never close or restore the same fd twice.Why a claim per handle and not one "workerData was never taken" signal from the native side
workerDatanatively when thenode:worker_threadsmodule starts to load. The module body builds theFileHandlelater, after it loadednode:fsand more. Aterminate()can land in between. A native signal says "taken" in that case and the fd leaks. A claim is taken only when the worker builds the handle.Why a native id and not a
SharedArrayBufferflag. The first revision of this PR put a one-slotSharedArrayBufferin the marker and usedAtomics.compareExchange. Review found two ways in which that breaks a transfer that works today. User code can replace theSharedArrayBufferandAtomicsglobals. WithBUN_JSC_useSharedArrayBuffer=0the structured clone rejects every shared buffer. The id is a plain number, so neither matters. An object inworkerDatathat imitates a marker cannot hold a live id either.Timing probes
terminate()after a delay (measured on the first revision, with a spy on the parent'sAtomics.compareExchange). Release build, 10 runs per delay: the parent wins every claim up to 5 ms, the worker wins every claim from 15 ms. Debug build, 4 runs per delay: the parent wins every claim up to 600 ms. In every run that the parent won, the fd was closed whenterminate()resolved.terminate()in the same tick as the constructor, debug build: 0 of 450 runs left the fd open.node:fsand makes the handle before it claims. With the claim before that load, 1 of 120 runs left the fd open: theterminate()landed in the load.Tests
terminate(), and the 5 marker imitations.is closed only once when workerData does not reference the handlefails whenfinalizeJSTransferablescloses without a claim.is not closed by the parent when the worker received the handlefails when the worker does not take the claim.is transferred when SharedArrayBuffer is not availableguards the choice of a plain number. It runs a subprocess withBUN_JSC_useSharedArrayBuffer=0and with the globals removed.terminate()test repeats an attempt in which the fd is still open, up to 10 times. A thread that wins the race againstterminate()receives the handle, and then the parent must not close the fd. Without the fix every attempt leaves the fd open.worker_threads.test.ts152 pass.worker-transfer-list,worker-transfer-terminate-stress,worker-async-dispose,worker-shutdown-post-leakand15787pass.test/js/node/test/parallel/test-worker-*.jsandtest-fs-promises-file-handle-read-worker.jswithbun <file>: 106 of 107 pass.test-worker-arraybuffer-zerofill.jsneeds thebun testrunner and fails the same way without this change.Other changes in this diff
finalizeJSTransferables(a handle intransferListthatworkerDatadoes not reference) and the rollback inrestoreNeuteredgo through the same claim. The rollback can run after the worker thread started, when something in the constructor throws late.data.fdand a live claim id. An object inworkerDatathat only imitates a marker is delivered as plain data, as in Node. Before, the worker built aFileHandlearound thefdvalue of that object, or threw in its bootstrap.#onClosecalls is made at module scope. A closure made insidepackJSTransferableskeeps the scope of that call alive, and that scope holds theworkerDatagraph of the user.worker.postMessage(fh, [fh])is not affected. Bun rejects aFileHandlein that transfer list with aDataCloneErrorand leaves the handle usable.Self-review. Addressed: a marker imitation can no longer throw in the worker bootstrap, the claim does not depend on globals that user code can replace, the function kept by the Worker no longer holds the
workerDatagraph, the fd-reuse test parks descriptors so the worker thread does not take the number, the tests collecterrorand wait forexit, the tests release their descriptors on failure, and the comment links Node's source. Rejected:expect(held).toContain(fd)in the two fd-reuse tests. Another thread can take the number first, and then the assertion fails for no fault of the code.closelistener up in the constructor. That layout is older than this change, and nothing between the two points throws in practice.Not changed
close()still leaks the fd. That is the scope of node:worker_threads: close leaked fs fds at worker exit (trackUnmanagedFds, FileHandle) #34260.kDeserializeinside the worker (a few bytecodes) still leaks one fd. The worker claims before it takes the fd so that an fd is never closed twice.#onClose. The fd still leaks there and Node closes it. That needs an owner on the native side: worker_threads: a transferred FileHandle leaks its fd when the parent thread exits before the worker receives it #43542.Bun.ModuleGraphcreates is stopped at birth (JSWorker.cpp:354) and reports noclose, so#onClosedoes not run for it either. I read this from the code and did not reproduce it.Found outside this task. The
worker stop orderingtests in the same file log withput(), which adds to the count before it stores the tag. Aterminate()between the two leaves a0tag, and the test then sees[12, 0]. That happened once on the x64-asan lane and passed on the retry.[human-review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file