fs.promises: close leaked FileHandle fds on GC and raise ERR_INVALID_STATE - #33693
Conversation
…STATE A FileHandle dropped without close() was never finalized: the fd stayed open until process exit with no diagnostic. Node.js closes the fd in its native finalizer and (since v25, DEP0137 end-of-life) raises ERR_INVALID_STATE as an uncaught exception. Register each open FileHandle with a FinalizationRegistry that closes the fd and reports the error via process.nextTick when collected unclosed. Unregister on close()/[kCloseSync]()/[kTransfer](). Handles deserialized from worker transfer are re-registered in [kDeserialize](). Also fix three existing tests that were leaking FileHandles.
|
Updated 6:24 PM PT - Jul 7th, 2026
❌ @robobun, your commit 87612ae has some failures in 🧪 To try this PR locally: bunx bun-pr 33693That installs a local version of the PR into your bun-33693 --bun |
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds FinalizationRegistry-based cleanup for leaked ChangesFileHandle GC Leak Detection
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
bun-lock/bun-lockb tests opened the lockfile without closing it; switch to await using. test-whatwg-readablebytestream.js synced with upstream Node which added a cancel() handler to close the file when the stream is cancelled via break/throw in for-await.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Avoid retaining the caller's Buffer/URL for the handle's lifetime and avoid dispatching through user-overridable toString at finalizer time.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/js/node/fs.promises.ts (2)
218-223: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute the diagnostic through
$ERR_INVALID_STATE.Manually creating
Errorand assigning.codebypasses Bun’s centralized Node error machinery, so the uncaught diagnostic can diverge in shape/format from otherERR_INVALID_STATEerrors.Proposed fix
- const err: NodeJS.ErrnoException = new Error( + const err = $ERR_INVALID_STATE( "A FileHandle object was closed during garbage collection. This used to be allowed " + "with a deprecation warning but is now considered an error. Please close FileHandle " + `objects explicitly. File descriptor: ${held.fd}${suffix}`, ); - err.code = "ERR_INVALID_STATE";As per coding guidelines, user-facing JS errors should go through centralized ErrorCode machinery rather than inline
new Errorwith hand-assigned.code.🤖 Prompt for 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. In `@src/js/node/fs.promises.ts` around lines 218 - 223, The FileHandle GC diagnostic is being built manually with new Error and a hand-assigned .code, which bypasses Bun’s centralized Node error machinery. Update the FileHandle finalization path in fs.promises.ts so the uncaught diagnostic is created through the existing ERR_INVALID_STATE error helper/$ERR_INVALID_STATE mechanism instead of constructing the error inline, keeping the message content and FileDescriptor context intact.Source: Coding guidelines
224-226: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t schedule internal diagnostics through mutable
process.nextTick.Userland can replace
process.nextTick, suppressing or changing the finalizer’s uncaughtERR_INVALID_STATEpath. Capture the scheduler at module init and invoke it via.$call.Proposed fix
let fileHandleRegistry: FinalizationRegistry<{ fd: number; path: string | undefined }> | undefined; +const processNextTick = process.nextTick; + function onFileHandleCollected(held: { fd: number; path: string | undefined }) {- process.nextTick(() => { + processNextTick.$call(process, () => { throw err; });As per coding guidelines, built-in JS modules should avoid routing internal logic through user-overridable machinery.
🤖 Prompt for 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. In `@src/js/node/fs.promises.ts` around lines 224 - 226, Avoid routing the finalizer’s diagnostic throw through mutable process.nextTick, since userland can override it and alter the ERR_INVALID_STATE path. Update the fs.promises module to capture the scheduler once at module initialization and use that captured reference when scheduling the throw in the relevant finalizer path. The change should be made in the code that currently calls process.nextTick inside the internal cleanup/diagnostics logic, preserving the uncaught error behavior via the captured scheduler’s .$call invocation.Source: Coding guidelines
🤖 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 `@test/js/node/test/parallel/test-whatwg-readablebytestream.js`:
- Around line 88-90: This change is in a vendored Node compatibility test, so do
not edit the existing test-whatwg-readablebytestream.js in place. Revert the
local cancel() cleanup in the upstream-mirrored test, and if the behavior still
needs coverage, add a separate Bun-owned test under test/js/node/... instead of
modifying the parallel Node test mirror. If this fix corresponds to an upstream
Node update, sync the whole vendored file from upstream rather than patching
just this method.
---
Outside diff comments:
In `@src/js/node/fs.promises.ts`:
- Around line 218-223: The FileHandle GC diagnostic is being built manually with
new Error and a hand-assigned .code, which bypasses Bun’s centralized Node error
machinery. Update the FileHandle finalization path in fs.promises.ts so the
uncaught diagnostic is created through the existing ERR_INVALID_STATE error
helper/$ERR_INVALID_STATE mechanism instead of constructing the error inline,
keeping the message content and FileDescriptor context intact.
- Around line 224-226: Avoid routing the finalizer’s diagnostic throw through
mutable process.nextTick, since userland can override it and alter the
ERR_INVALID_STATE path. Update the fs.promises module to capture the scheduler
once at module initialization and use that captured reference when scheduling
the throw in the relevant finalizer path. The change should be made in the code
that currently calls process.nextTick inside the internal cleanup/diagnostics
logic, preserving the uncaught error behavior via the captured scheduler’s
.$call invocation.
🪄 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: cb92f2c2-6915-4722-9f78-227402ed7651
📒 Files selected for processing (4)
src/js/node/fs.promises.tstest/cli/install/bun-lock.test.tstest/cli/install/bun-lockb.test.tstest/js/node/test/parallel/test-whatwg-readablebytestream.js
|
Addressing the two outside-diff review findings:
Using
|
Move the String(path) conversion from the FileHandle constructor to the open() wrapper, before the native open runs. A Buffer/URL with a throwing toString now surfaces before an fd exists rather than leaking one between open and registry registration.
There was a problem hiding this comment.
All prior threads are addressed and I found nothing new, but this introduces a user-visible behavior change (uncaught ERR_INVALID_STATE for previously-silent leaks) and you explicitly flagged the readableWebStream() / module-level wrapper retention question for maintainer judgment — worth a human sign-off on both.
Extended reasoning...
Overview
This PR adds a FinalizationRegistry to src/js/node/fs.promises.ts so that a FileHandle collected without close() gets its fd closed and raises ERR_INVALID_STATE via process.nextTick, matching Node's DEP0137 end-of-life behavior. It wires unregister into close(), [kCloseSync], and [kTransfer], re-registers in [kDeserialize], snapshots the diagnostic path as a string before fs.open runs, and fixes six pre-existing tests that were leaking handles (including the upstream-sourced cancel() addition to the vendored test-whatwg-readablebytestream.js). A new subprocess test in fs.test.ts asserts fds are reclaimed, diagnostics fire, and properly-closed handles produce no false positives.
Security risks
None identified. The change is fd-lifecycle bookkeeping; it does not touch auth, permissions, network input, or path validation. The finalizer's closeSync runs on an fd Bun itself opened.
Level of scrutiny
Medium-high. The diff is small but it's a behavior change, not a bug fix in the narrow sense: code that previously worked (while silently leaking an fd) will now emit an uncaught exception. That's the intended Node-compat outcome, but it's the kind of thing a maintainer should consciously ship. It also lives in a hot builtin (src/js/) and relies on GC-finalizer timing, which is inherently subtle.
Other factors
Two rounds of review feedback have been addressed (path snapshotting, ordering vs. fs.open), and the $ERR_INVALID_STATE vs. hand-rolled Error question was resolved with an empirical Node comparison. The one substantive item left open is by the author's own choice: readableWebStream() and the module-level readFile/writeFile/appendFile wrappers still borrow the raw fd without retaining the handle, and the author declined to rework them here, saying they'd do it as a follow-up if a maintainer wants it. That's a scope/design call I shouldn't make on their behalf.
|
CI status for 87612ae (build #70046, final): 284 jobs passed, including every Linux, Windows, darwin-x64, and darwin-14-aarch64 test lane. The new The only two failures are both No test-level error annotations were generated. Ready for review; the red is infra on the darwin-26-aarch64 lane. |
…File (#34283) ## What Sync `validateReadFileProc()` with upstream nodejs/node@2eeb65fa81 (`const` to `await using`) so the `/proc/sys/kernel/hostname` FileHandle is disposed. ## Why #33693 added a FinalizationRegistry that throws `ERR_INVALID_STATE` when a `FileHandle` is collected without `close()` (DEP0137 end-of-life). This test opened the hostname fd and never closed it; whenever GC happened to run during the later `doReadAndCancel()` section, the registry fired and the process exited 1 with: ``` error: A FileHandle object was closed during garbage collection. This used to be allowed with a deprecation warning but is now considered an error. Please close FileHandle objects explicitly. File descriptor: 9 (/proc/sys/kernel/hostname) code: "ERR_INVALID_STATE" at onFileHandleCollected (node:fs/promises:119:78) ``` It has been in the `flaky` annotation on 140 of the last 400 Buildkite builds (Linux lanes only, heaviest on `x64-asan` where GC pressure is highest). #33693 already fixed three other vendored tests the same way; this one was missed because it is Linux-only and GC-timing-dependent. Upstream Node made the same change in nodejs/node@2eeb65fa81. ## Verification ``` $ bun bd test/js/node/test/parallel/test-fs-promises-file-handle-readFile.js # exit 0 ``` The failure is GC-timing-dependent so a deterministic fail-before is not available locally; the evidence is the CI annotation count plus the fact that the unclosed handle is the only one in the file whose path matches the error's `(/proc/sys/kernel/hostname)`. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
2 participants
What does this PR do?
A
FileHandledropped withoutclose()was never finalized in Bun: the underlying fd stayed open until process exit with no diagnostic. Node.js closes the fd in its native finalizer and, since v25 (DEP0137 end-of-life), raisesERR_INVALID_STATEas an uncaught exception.Reproduction
ERR_INVALID_STATE)ERR_INVALID_STATE)Fix
Register each open
FileHandlewith aFinalizationRegistrykeyed on{fd, path}. When collected unclosed, the finalizer:closeSyncbindingErrorwithcode: "ERR_INVALID_STATE"and Node's exact message (including fd number and path) viaprocess.nextTick, so every handle in a GC batch gets processed rather than the first throw aborting the batchExplicit
close(),[Symbol.asyncDispose],[kCloseSync]()(pull/writer autoClose), and[kTransfer]()unregister the handle so no false positives fire. Handles deserialized from a worker transfer are re-registered in[kDeserialize]().Also fixes three existing tests that were leaking
FileHandles and would now be caught by the finalizer.How did you verify your code works?
test/js/node/fs/fs.test.tsspawns a subprocess that drops 50 handles, forces GC, and asserts 0 fds remain open with 50ERR_INVALID_STATEdiagnostics; then opens and properly closes 50 more and asserts 0 false positives.leakedAfterGC: 50, diagCount: 0), passes on this branch.bun bd test test/js/node/fs/fs.test.ts(382 pass),promises.test.js,fs-leak.test.js,bun-file.test.ts, and the FileHandle transfer tests inworker_threads.test.tsall pass.