diff --git a/src/js/node/fs.promises.ts b/src/js/node/fs.promises.ts index 2f09aba10f03..81f63eaad2dc 100644 --- a/src/js/node/fs.promises.ts +++ b/src/js/node/fs.promises.ts @@ -206,6 +206,26 @@ async function opendir(dir: string, options) { return promise; } +// Node.js closes a FileHandle's fd in its native finalizer and raises +// ERR_INVALID_STATE (DEP0137 end-of-life) when collected without close(). +// Mirror that with a FinalizationRegistry so dropped handles don't leak fds. +let fileHandleRegistry: FinalizationRegistry<{ fd: number; path: string | undefined }> | undefined; +function onFileHandleCollected(held: { fd: number; path: string | undefined }) { + try { + fs.closeSync(held.fd); + } catch {} + const suffix = held.path !== undefined ? ` (${held.path})` : ""; + const err: NodeJS.ErrnoException = new 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: ${held.fd}${suffix}`, + ); + err.code = "ERR_INVALID_STATE"; + process.nextTick(() => { + throw err; + }); +} + const private_symbols = { kRef, kUnref, @@ -264,7 +284,11 @@ const exports = { }, statfs: asyncWrap(fs.statfs, "statfs"), open: async (path, flags = "r", mode = 0o666) => { - return new private_symbols.FileHandle(await fs.open(path, flags, mode), flags); + // Snapshot the path as a string before the fd is opened so a throwing + // Buffer/URL toString cannot leak the fd, and the registry never retains + // the caller's object. + const pathForDiag = typeof path === "string" ? path : path == null ? undefined : String(path); + return new private_symbols.FileHandle(await fs.open(path, flags, mode), flags, pathForDiag); }, read: asyncWrap(fs.read, "read"), write: asyncWrap(fs.write, "write"), @@ -383,12 +407,15 @@ function asyncWrap(fn: any, name: string) { // These functions await the result so that errors propagate correctly with // async stack traces and so that the ref counting is correct. class FileHandle extends EventEmitter { - constructor(fd, flag) { + constructor(fd, flag, path?: string) { super(); this[kFd] = fd ? fd : -1; this[kRefs] = 1; this[kClosePromise] = null; this[kFlag] = flag; + if (this[kFd] !== -1) { + (fileHandleRegistry ??= new FinalizationRegistry(onFileHandleCollected)).register(this, { fd, path }, this); + } } getAsyncId() { @@ -671,6 +698,8 @@ function asyncWrap(fn: any, name: string) { return this[kClosePromise]; } + fileHandleRegistry?.unregister(this); + if (--this[kRefs] === 0) { this[kFd] = -1; this[kClosePromise] = PromisePrototypeFinally.$call(close(fd), () => { @@ -1357,6 +1386,7 @@ function asyncWrap(fn: any, name: string) { } const fd = this[kFd]; this[kFd] = -1; + fileHandleRegistry?.unregister(this); (nodeFsForIter ??= require("node:fs")).closeSync(fd); this.emit("close"); } @@ -1369,6 +1399,7 @@ function asyncWrap(fn: any, name: string) { const fd = this[kFd]; const flag = this[kFlag]; this[kFd] = -1; + fileHandleRegistry?.unregister(this); return { data: { fd, flag }, deserializeInfo: "internal/fs/promises:FileHandle", @@ -1382,6 +1413,13 @@ function asyncWrap(fn: any, name: string) { [kDeserialize]({ fd, flag }) { this[kFd] = fd; this[kFlag] = flag; + if (fd !== -1) { + (fileHandleRegistry ??= new FinalizationRegistry(onFileHandleCollected)).register( + this, + { fd, path: undefined }, + this, + ); + } } [kRef]() { diff --git a/test/cli/install/bun-lock.test.ts b/test/cli/install/bun-lock.test.ts index ba525c633965..eca37e6cef9c 100644 --- a/test/cli/install/bun-lock.test.ts +++ b/test/cli/install/bun-lock.test.ts @@ -55,7 +55,7 @@ it("should write plaintext lockfiles", async () => { await access(join(packageDir, "bun.lock")); // Assert that the lockfile has the correct permissions - const file = await open(join(packageDir, "bun.lock"), "r"); + await using file = await open(join(packageDir, "bun.lock"), "r"); const stat = await file.stat(); // in unix, 0o644 == 33188 diff --git a/test/cli/install/bun-lockb.test.ts b/test/cli/install/bun-lockb.test.ts index 8cb78e76ff55..60a59f10a057 100644 --- a/test/cli/install/bun-lockb.test.ts +++ b/test/cli/install/bun-lockb.test.ts @@ -44,7 +44,7 @@ it("should not print anything to stderr when running bun.lockb", async () => { expect(await exists(join(packageDir, "bun.lockb"))).toBe(true); // Assert that the lockfile has the correct permissions - const file = await open(join(packageDir, "bun.lockb"), "r"); + await using file = await open(join(packageDir, "bun.lockb"), "r"); const stat = await file.stat(); // in unix, 0o755 == 33261 diff --git a/test/js/bun/util/bun-file.test.ts b/test/js/bun/util/bun-file.test.ts index dda73f30b3a3..03e5b1a61fb2 100644 --- a/test/js/bun/util/bun-file.test.ts +++ b/test/js/bun/util/bun-file.test.ts @@ -36,8 +36,7 @@ test("writer.end() should not close the fd if it does not own the fd", async () const fd = fileHandle.fd; await Bun.file(fd).writer().end(); - // @ts-ignore - await fsPromises.close(fd); + await fileHandle.close(); expect(await Bun.file(filename).text()).toBe(""); } }); diff --git a/test/js/node/fs/fs.test.ts b/test/js/node/fs/fs.test.ts index 8d081789ba98..6697d94879fa 100644 --- a/test/js/node/fs/fs.test.ts +++ b/test/js/node/fs/fs.test.ts @@ -428,6 +428,78 @@ describe("FileHandle", () => { expect(readFileSync(path, "utf8")).toBe("Test file written successfully"); }); + + // Node.js closes a FileHandle's fd in its native finalizer and raises + // ERR_INVALID_STATE (DEP0137 end-of-life) when the handle is collected + // without close(). Bun must reclaim the fd and surface the same diagnostic. + it.concurrent.skipIf(isWindows)( + "FileHandle collected without close() closes the fd and raises ERR_INVALID_STATE", + async () => { + const fixture = /* js */ ` + const fsp = require("node:fs/promises"); + const fs = require("node:fs"); + const os = require("node:os"); + const path = require("node:path"); + const fdDir = process.platform === "darwin" ? "/dev/fd" : "/proc/self/fd"; + const nfds = () => fs.readdirSync(fdDir).length; + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "fh-gc-")); + const N = 50; + const diags = []; + process.on("uncaughtException", e => diags.push({ code: e.code, message: e.message })); + + (async () => { + const before = nfds(); + await (async () => { + for (let i = 0; i < N; i++) await fsp.open(path.join(dir, "f" + i), "w"); + })(); + // force GC until every leaked fd is reclaimed and every diagnostic lands + for (let i = 0; i < 40 && (nfds() - before > 0 || diags.length < N); i++) { + Bun.gc(true); + await new Promise(r => setTimeout(r, 25)); + } + const afterGC = nfds(); + const sample = diags[0] ?? {}; + + // properly closed handles must not trip the finalizer + const marker = diags.length; + await (async () => { + for (let i = 0; i < N; i++) await (await fsp.open(path.join(dir, "g" + i), "w")).close(); + })(); + for (let i = 0; i < 10; i++) { + Bun.gc(true); + await new Promise(r => setTimeout(r, 25)); + } + + console.log(JSON.stringify({ + leakedAfterGC: afterGC - before, + diagCount: diags.length, + sampleCode: sample.code, + sampleHasFd: typeof sample.message === "string" && sample.message.includes("File descriptor: "), + falsePositives: diags.length - marker, + })); + fs.rmSync(dir, { recursive: true, force: true }); + })(); + `; + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", fixture], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ result: JSON.parse(stdout.trim()), stderr, exitCode }).toEqual({ + result: { + leakedAfterGC: 0, + diagCount: 50, + sampleCode: "ERR_INVALID_STATE", + sampleHasFd: true, + falsePositives: 0, + }, + stderr: expect.not.stringContaining("error"), + exitCode: 0, + }); + }, + ); }); it("fdatasyncSync", () => { diff --git a/test/js/node/fs/promises.test.js b/test/js/node/fs/promises.test.js index 5684080c5c73..5e1a07a6851c 100644 --- a/test/js/node/fs/promises.test.js +++ b/test/js/node/fs/promises.test.js @@ -120,11 +120,11 @@ describe("access", () => { describe("open", () => { it("should work", async () => { - await open(__filename); + await using _ = await open(__filename); }); it("should return an object", async () => { - const fh = await open(__filename); + await using fh = await open(__filename); assert.strictEqual(typeof fh, "object"); assert.strictEqual(typeof fh.fd, "number"); }); diff --git a/test/js/node/test/parallel/test-whatwg-readablebytestream.js b/test/js/node/test/parallel/test-whatwg-readablebytestream.js index e1b63248852e..c5c3f3228f8e 100644 --- a/test/js/node/test/parallel/test-whatwg-readablebytestream.js +++ b/test/js/node/test/parallel/test-whatwg-readablebytestream.js @@ -85,6 +85,10 @@ class Source { this.controller = controller; } + async cancel() { + await this.file.close(); + } + async pull(controller) { const byobRequest = controller.byobRequest; assert.match(inspect(byobRequest), /ReadableStreamBYOBRequest/);