[steipete]

Calling close() through a forwarding Proxy around a FileHandle, or after detaching the method, uses the receiver instead of the owning handle. Node binds FileHandle.close to the handle.

Bind the existing asynchronous method in the constructor as a non-enumerable, writable, configurable own property. The close and finalizer logic stays unchanged. Keeping the function non-enumerable fixes the regression where a worker transfer threw DataCloneError if the message also reached the transferred handle through a class instance. Tests cover direct and nested proxies, get traps, detached calls, foreign receivers, repeated close calls, descriptor recycling, and that class-instance transfer case, with Node controls.

This preserves main's clone behavior, including its existing differences from Node. Bun's Object.keys(fh) excludes close, while Node's includes it. Bun still clones open and closed handles through structuredClone, v8.serialize, and postMessage without a transfer list; Node refuses those clones. In the class-instance transfer case, Bun still produces a plain object for the nested reference, while Node preserves the same FileHandle in both positions. The transfer-graph gap is pre-existing and unchanged. The FileHandle test proposed by #35841 therefore remains an expected failure; its expectFailure option needs no change.

Validation on Linux release builds: the new transfer regression fails with DataCloneError on the original PR head 630023eef and passes on 2c12f2738. The complete test/js/node/fs/fs.test.ts file has 637 passes, 8 skips, and the five expected forwarding/detached-close failures on main canary 1.4.4-canary.1+b1007f117; the candidate has 642 passes, 8 skips, and no failures. This includes all forwarding/finalizer cases and Node 26.12.1 controls. All six open/closed structuredClone, v8.serialize, and MessagePort.postMessage results match main, as do both direct Worker.postMessage checks. Prettier, diff checks, and independent P0–P2 review pass.

Ported from openclaw#183.

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The FileHandle constructor now defines a bound, non-enumerable close property. Tests cover handle transfer and close calls through multiple receiver types. The compatibility notes document the binding and cleanup behavior. The empty-name mkdtempSync test now uses a prefix with a trailing separator.

Changes

FileHandle close binding

Layer / File(s)	Summary
Close binding and regression coverage src/js/node/fs.promises.ts, test/js/node/fs/fs.test.ts, docs/runtime/nodejs-compat.mdx	The constructor defines close as a bound, non-enumerable, writable, configurable own property. Child-process tests cover handle transfer and direct, proxied, detached, and foreign-receiver close calls. The compatibility notes describe the binding and garbage-collection cleanup behavior.

mkdtempSync test prefix

Layer / File(s)	Summary
Temporary-directory prefix test/js/node/fs/fs.test.ts	The empty-name mkdtempSync test passes the temporary-directory path with a trailing separator.

Possibly related PRs

• oven-sh/bun#35841: This PR changes the FileHandle.close property shape, which affects the cloning behavior tested by that PR.

Suggested reviewers: robobun

Merge Risk: Minimal · up to 2c12f

No merge-blocking risk was identified; the change appears ready for normal checks.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly and concisely identifies the main change: binding FileHandle.close to its owning handle.
Description check		The description explains the behavior change, implementation, regression coverage, verification results, and compatibility considerations. It does not use the required section headings, but it provide…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description explains the behavior change, implementation, regression coverage, verification results, and compatibility considerations. It does not use the required section headings, but it provides the information requested by both template sections.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: oven-sh/bun
• Branch: main
• File: .coderabbit.yaml
• Commit: 5e18294
• Layers: Repository YAML
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: eb5ba576-6937-42c6-9d34-4a6c64fe53b8

Commits

Reviewing files that changed from the base of the PR and between 630023e and 2c12f27.

Files selected for processing: 2

• src/js/node/fs.promises.ts
• test/js/node/fs/fs.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

[robobun]

A note on one effect of this change that its title does not name. close becomes an own enumerable function of every FileHandle, so structured clone now refuses a FileHandle on every road (structuredClone, v8.serialize, postMessage, workerData, advanced IPC), open or closed. Node v26.3.0 refuses it too, so for those roads this is the result that node has. I applied the two lines of this diff to a build of main and ran a table of 3 FileHandle states on 12 roads against node: all 36 rows agree, where 0 agree on main.

Two things follow from it:

• node: give ChildProcess and FSWatcher node's _handle #35841 adds a FileHandle test to test/js/node/handle-layout.test.ts with expectFailure under bun, because a FileHandle still clones there. If node: give ChildProcess and FSWatcher node's _handle #35841 lands first, that test reports an unexpected pass on this branch. Remove the expectFailure option then, and the refusal is pinned.
• One case that works on main throws with an own close: a FileHandle in transferList that the message also reaches through a class instance.

class Holder {
  constructor(fh) {
    this.fh = fh;
  }
}
const fh = await fsp.open(file, "r");
new Worker(workerFile, { workerData: { fh, holder: new Holder(fh) }, transferList: [fh] });
// main: the worker gets the FileHandle (holder.fh arrives as a plain object)
// with this change: DataCloneError: The object can not be cloned.
// node v26.3.0: the worker gets the FileHandle in both places

[steipete]

Confirmed on the current Linux build: the close-binding change makes the class-instance transfer case throw DataCloneError, while Node preserves both FileHandle references. I have left this PR unchanged because fixing that properly requires extending the worker transfer-graph walk beyond plain objects; the expectFailure note for #35841 is also correct.

[steipete]

Fixed the transfer regression in 2c12f2738d: close stays bound to its handle but is now non-enumerable, so the class-instance transfer succeeds again. Linux tests confirm main’s clone behavior is preserved and forwarded/detached closes still close the owner once and disarm its finalizer.