fix(node/fs): fs.close and fs.Dir.closeSync (#16686)

src/bun.js/bindings/ErrorCode.cpp

@@ -1065,6 +1065,8 @@ JSC_DEFINE_HOST_FUNCTION(Bun::jsFunctionMakeErrorWithCode, (JSC::JSGlobalObject
         return JSC::JSValue::encode(createError(globalObject, ErrorCode::ERR_STREAM_UNABLE_TO_PIPE, "Cannot pipe to a closed or destroyed stream"_s));
     case ErrorCode::ERR_ILLEGAL_CONSTRUCTOR:
         return JSC::JSValue::encode(createError(globalObject, ErrorCode::ERR_ILLEGAL_CONSTRUCTOR, "Illegal constructor"_s));
+    case ErrorCode::ERR_DIR_CLOSED:
+        return JSC::JSValue::encode(createError(globalObject, ErrorCode::ERR_DIR_CLOSED, "Directory handle was closed"_s));
 
     default: {
         break;

src/bun.js/bindings/ErrorCode.ts

@@ -92,6 +92,9 @@ const errors: ErrorCodeMapping = [
   // Console
   ["ERR_CONSOLE_WRITABLE_STREAM", TypeError, "TypeError"],
 
+  // FS
+  ["ERR_DIR_CLOSED", Error],
+
   // DNS
   ["ERR_DNS_SET_SERVERS_FAILED", Error],
 

src/js/node/fs.ts

@@ -1013,19 +1013,25 @@ function opendirSync(path, options) {
 }
 
 class Dir {
-  #handle;
+  /**
+   * `-1` when closed. stdio handles (0, 1, 2) don't actually get closed by
+   * {@link close} or {@link closeSync}.
+   */
+  #handle: number;
   #path;
   #options;
   #entries: any[] | null = null;
 
   constructor(handle, path, options) {
-    if (handle == null) throw $ERR_MISSING_ARGS("handle");
+    if ($isUndefinedOrNull(handle)) throw $ERR_MISSING_ARGS("handle");
     this.#handle = handle;
     this.#path = path;
     this.#options = options;
   }
 
   readSync() {
+    if (this.#handle < 0) throw $ERR_DIR_CLOSED();
+
     let entries = (this.#entries ??= fs.readdirSync(this.#path, {
       withFileTypes: true,
       encoding: this.#options?.encoding,
@@ -1035,6 +1041,8 @@ class Dir {
   }
 
   read(cb?): any {
+    if (this.#handle < 0) throw $ERR_DIR_CLOSED();
+
     if (cb) {
       return this.read().then(entry => cb(null, entry));
     }
@@ -1054,13 +1062,19 @@ class Dir {
   }
 
   close(cb?: () => void) {
+    const handle = this.#handle;
     if (cb) {
       process.nextTick(cb);
     }
-    return fs.closedirSync(this.#handle);
+    if (handle > 2) fs.closeSync(handle);
+    this.#handle = -1;
   }
 
-  closeSync() {}
+  closeSync() {
+    const handle = this.#handle;
+    if (handle > 2) fs.closeSync(handle);
+    this.#handle = -1;
+  }
 
   get path() {
     return this.#path;

test/harness.ts

@@ -1040,7 +1040,7 @@ export function mergeWindowEnvs(envs: Record<string, string | undefined>[]) {
   return flat;
 }
 
-export function tmpdirSync(pattern: string = "bun.test.") {
+export function tmpdirSync(pattern: string = "bun.test."): string {
   return fs.mkdtempSync(join(fs.realpathSync.native(os.tmpdir()), pattern));
 }
 

test/js/node/fs/dir.test.ts

@@ -0,0 +1,34 @@
+import { describe, beforeAll, afterAll, it, expect } from "bun:test";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+
+describe("given a directory that exists", () => {
+  let dirname: string;
+
+  beforeAll(() => {
+    const name = "dir-sync.test." + String(Math.random() * 100).substring(0, 6);
+    dirname = path.join(os.tmpdir(), name);
+    fs.mkdirSync(dirname);
+  });
+
+  afterAll(() => {
+    fs.rmdirSync(dirname, { recursive: true });
+  });
+
+  it("can be opened/closed synchronously", () => {
+    const dir = fs.opendirSync(dirname);
+    expect(dir).toBeDefined();
+    expect(dir).toBeInstanceOf(fs.Dir);
+    expect(dir.closeSync()).toBeUndefined();
+    expect(() => dir.readSync()).toThrow("Directory handle was closed");
+  });
+
+  it("can be opened/closed asynchronously", async () => {
+    const dir = await fs.promises.opendir(dirname);
+    expect(dir).toBeDefined();
+    expect(dir).toBeInstanceOf(fs.Dir);
+    expect(await dir.close()).toBeUndefined();
+    expect(() => dir.read()).toThrow("Directory handle was closed");
+  });
+});