node:fs: keep options.signal only for the async forms of readFile and writeFile

src/runtime/node/node_fs.rs

@@ -4481,6 +4481,24 @@ pub(crate) mod args {
         }
     }
 
+    /// Node's sync forms only type-check `options.signal`: https://github.com/nodejs/node/blob/v26.3.0/lib/internal/fs/utils.js#L340-L342
+    #[inline(always)]
+    fn signal_option(
+        ctx: &JSGlobalObject,
+        value: JSValue,
+        will_be_async: bool,
+    ) -> JsResult<Option<AbortSignalRef>> {
+        let Some(signal) = AbortSignal::from_js(value) else {
+            return Err(ctx.throw_invalid_argument_type_value(b"signal", b"AbortSignal", value));
+        };
+        if !will_be_async {
+            return Ok(None);
+        }
+        let signal = AbortSignal::opaque_ref(signal).ref_();
+        signal.pending_activity_ref();
+        Ok(Some(signal))
+    }
+
     /// Asynchronously reads the entire contents of a file.
     /// @param path A path to a file. If a URL is provided, it must use the `file:` protocol.
     /// If a file descriptor is provided, the underlying file will _not_ be closed automatically.
@@ -4545,16 +4563,7 @@ pub(crate) mod args {
                         flag = FileSystemFlags::from_js(ctx, flag_)?.unwrap_or(flag);
                     }
                     if let Some(value) = arg.get_truthy(ctx, "signal")? {
-                        if let Some(signal) = AbortSignal::ref_from_js(value) {
-                            signal.pending_activity_ref();
-                            *abort_signal = Some(signal);
-                        } else {
-                            return Err(ctx.throw_invalid_argument_type_value(
-                                b"signal",
-                                b"AbortSignal",
-                                value,
-                            ));
-                        }
+                        *abort_signal = signal_option(ctx, value, arguments.will_be_async)?;
                     }
                 }
             }
@@ -4643,16 +4652,7 @@ pub(crate) mod args {
                         mode = node::mode_from_js(ctx, mode_)?.unwrap_or(mode);
                     }
                     if let Some(value) = arg.get_truthy(ctx, "signal")? {
-                        if let Some(signal) = AbortSignal::ref_from_js(value) {
-                            signal.pending_activity_ref();
-                            *abort_signal = Some(signal);
-                        } else {
-                            return Err(ctx.throw_invalid_argument_type_value(
-                                b"signal",
-                                b"AbortSignal",
-                                value,
-                            ));
-                        }
+                        *abort_signal = signal_option(ctx, value, arguments.will_be_async)?;
                     }
                     if let Some(flush_) = arg.get(ctx, "flush")? {
                         if flush_.is_boolean() || flush_.is_undefined_or_null() {

src/runtime/node/node_fs_binding.rs

@@ -34,6 +34,8 @@ where
     let vm: &VirtualMachine = global.bun_vm();
     let mut slice = ArgumentsSlice::init(vm, JsLocal::to_js_slice(args));
     let args = <A as FsArgument>::from_js(global, &mut slice)?;
+    // The abort checks in `NodeFS` are for a pool thread.
+    debug_assert!(args.signal().is_none(), "a sync parse keeps no signal");
 
     // R-2: `JsCell::with_mut` scopes the `&mut NodeFS` to the blocking
     // syscall; `dispatch` never re-enters JS, and `Maybe<R>` is fully owned

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

@@ -3172,6 +3172,250 @@ describe("writeFileSync", () => {
   });
 });
 
+// Node's sync forms type-check `options.signal` and never read it again:
+// https://github.com/nodejs/node/blob/v26.3.0/lib/internal/fs/utils.js#L340-L342
+describe("readFileSync / writeFileSync / appendFileSync with an aborted options.signal", () => {
+  const aborted = (): any => ({ signal: AbortSignal.abort() });
+  const codeOf = (fn: () => unknown) => {
+    try {
+      fn();
+      return "ok";
+    } catch (e: any) {
+      return e.code;
+    }
+  };
+
+  it("readFileSync reads a path", () => {
+    // More than the 256 KB that one read takes before the stat.
+    const big = Buffer.alloc(300_000, "b").toString();
+    using dir = tempDir("fs-sync-signal-read", { text: "hello", empty: "", big });
+    const at = (name: string) => join(String(dir), name);
+    expect({
+      utf8: readFileSync(at("text"), { encoding: "utf8", ...aborted() }),
+      buffer: readFileSync(at("text"), aborted()).toString("latin1"),
+      empty: readFileSync(at("empty"), { encoding: "utf8", ...aborted() }),
+      big: readFileSync(at("big"), { encoding: "utf8", ...aborted() }) === big,
+    }).toEqual({ utf8: "hello", buffer: "hello", empty: "", big: true });
+  });
+
+  it("readFileSync reads a file descriptor from its position and leaves it open", () => {
+    using dir = tempDir("fs-sync-signal-read-fd", { text: "ABCDEF" });
+    const fd = openSync(join(String(dir), "text"), "r");
+    try {
+      readSync(fd, Buffer.alloc(2), 0, 2, null);
+      expect({
+        first: readFileSync(fd, { encoding: "utf8", ...aborted() }),
+        second: readFileSync(fd, { encoding: "utf8", ...aborted() }),
+        size: fstatSync(fd).size,
+      }).toEqual({ first: "CDEF", second: "", size: 6 });
+    } finally {
+      closeSync(fd);
+    }
+  });
+
+  it("writeFileSync writes a path", () => {
+    using dir = tempDir("fs-sync-signal-write", { existing: "hello", append: "x", inPlace: "hello" });
+    const at = (name: string) => join(String(dir), name);
+    writeFileSync(at("string"), "W", aborted());
+    writeFileSync(at("buffer"), Buffer.from("B"), aborted());
+    writeFileSync(at("existing"), "W", aborted());
+    writeFileSync(at("append"), "A", { flag: "a", ...aborted() });
+    writeFileSync(at("inPlace"), "J", { flag: "r+", ...aborted() });
+    // A call that stops after its open leaves an empty file, and then the first retry fails.
+    const exclusive = [
+      codeOf(() => writeFileSync(at("exclusive"), "X", { flag: "wx", ...aborted() })),
+      codeOf(() => writeFileSync(at("exclusive"), "Y", { flag: "wx", ...aborted() })),
+    ];
+    expect({
+      string: readFileSync(at("string"), "utf8"),
+      buffer: readFileSync(at("buffer"), "utf8"),
+      existing: readFileSync(at("existing"), "utf8"),
+      append: readFileSync(at("append"), "utf8"),
+      inPlace: readFileSync(at("inPlace"), "utf8"),
+      exclusive: [...exclusive, readFileSync(at("exclusive"), "utf8")],
+    }).toEqual({
+      string: "W",
+      buffer: "B",
+      existing: "W",
+      append: "xA",
+      inPlace: "Jello",
+      exclusive: ["ok", "EEXIST", "X"],
+    });
+  });
+
+  it("writeFileSync writes a file descriptor at its position", () => {
+    using dir = tempDir("fs-sync-signal-write-fd", { text: "ABCDEF" });
+    const path = join(String(dir), "text");
+    const fd = openSync(path, "r+");
+    try {
+      readSync(fd, Buffer.alloc(2), 0, 2, null);
+      writeFileSync(fd, "xy", aborted());
+    } finally {
+      closeSync(fd);
+    }
+    expect(readFileSync(path, "utf8")).toBe("ABxyEF");
+  });
+
+  it("appendFileSync appends to a path and to a file descriptor", () => {
+    using dir = tempDir("fs-sync-signal-append", { path: "x", fd: "x" });
+    const at = (name: string) => join(String(dir), name);
+    fs.appendFileSync(at("path"), "A", aborted());
+    const fd = openSync(at("fd"), "a");
+    try {
+      fs.appendFileSync(fd, "A", aborted());
+    } finally {
+      closeSync(fd);
+    }
+    expect({ path: readFileSync(at("path"), "utf8"), fd: readFileSync(at("fd"), "utf8") }).toEqual({
+      path: "xA",
+      fd: "xA",
+    });
+  });
+
+  it("an aborted signal from AbortSignal.any()", () => {
+    using dir = tempDir("fs-sync-signal-any", {});
+    const path = join(String(dir), "file");
+    const options: any = { encoding: "utf8", signal: AbortSignal.any([AbortSignal.abort()]) };
+    writeFileSync(path, "W", options);
+    expect({ aborted: options.signal.aborted, read: readFileSync(path, options) }).toEqual({
+      aborted: true,
+      read: "W",
+    });
+  });
+
+  // The parser reads `encoding`, `flag` and `mode` before `signal`, and `flush` after it.
+  it.each(["encoding", "flag", "mode", "flush"])("an options.%s getter that aborts the controller", key => {
+    using dir = tempDir("fs-sync-signal-getter", { file: "hello" });
+    const path = join(String(dir), "file");
+    const controller = new AbortController();
+    const options: any = {
+      signal: controller.signal,
+      get [key]() {
+        controller.abort();
+        return undefined;
+      },
+    };
+    writeFileSync(path, "W", options);
+    expect({ aborted: controller.signal.aborted, read: readFileSync(path, options).toString("latin1") }).toEqual({
+      aborted: true,
+      read: "W",
+    });
+  });
+
+  it("a call from inside the abort listener of the signal", () => {
+    using dir = tempDir("fs-sync-signal-listener", { file: "hello" });
+    const path = join(String(dir), "file");
+    const controller = new AbortController();
+    const options: any = { encoding: "utf8", signal: controller.signal };
+    let seen: unknown;
+    controller.signal.addEventListener("abort", () => {
+      try {
+        const before = readFileSync(path, options);
+        writeFileSync(path, "W", options);
+        fs.appendFileSync(path, "A", options);
+        seen = [before, readFileSync(path, options)];
+      } catch (e) {
+        seen = e;
+      }
+    });
+    controller.abort();
+    expect(seen).toEqual(["hello", "WA"]);
+  });
+
+  it("the error is the error of the operation", () => {
+    using dir = tempDir("fs-sync-signal-errors", {});
+    expect({
+      directory: codeOf(() => readFileSync(String(dir), aborted())),
+      missing: codeOf(() => readFileSync(join(String(dir), "missing"), aborted())),
+    }).toEqual({ directory: "EISDIR", missing: "ENOENT" });
+  });
+
+  it("the promise forms still reject with the same options object", async () => {
+    using dir = tempDir("fs-sync-signal-shared", { read: "hello" });
+    const at = (name: string) => join(String(dir), name);
+    const options: any = { encoding: "utf8", signal: AbortSignal.abort() };
+    const abortError = { name: "AbortError", code: "ABORT_ERR" };
+    const before = readFileSync(at("read"), options);
+    await expect(promises.readFile(at("read"), options)).rejects.toMatchObject(abortError);
+    await expect(promises.writeFile(at("write"), "W", options)).rejects.toMatchObject(abortError);
+    const created = existsSync(at("write"));
+    writeFileSync(at("write"), "W", options);
+    expect({ before, created, after: readFileSync(at("write"), options) }).toEqual({
+      before: "hello",
+      created: false,
+      after: "W",
+    });
+  });
+
+  it.each([[{}], [1]])("a signal that is not an AbortSignal (%p) fails every form before any I/O", async signal => {
+    using dir = tempDir("fs-sync-signal-invalid", { read: "hello" });
+    const at = (name: string) => join(String(dir), name);
+    const options: any = { signal };
+    const unreachable = () => {
+      throw new Error("the callback must not run");
+    };
+    const rejected = async (promise: Promise<unknown>) => {
+      try {
+        await promise;
+        return "resolved";
+      } catch (e: any) {
+        return e.code;
+      }
+    };
+    expect({
+      readFileSync: codeOf(() => readFileSync(at("read"), options)),
+      writeFileSync: codeOf(() => writeFileSync(at("w1"), "W", options)),
+      appendFileSync: codeOf(() => fs.appendFileSync(at("w2"), "A", options)),
+      readFile: codeOf(() => fs.readFile(at("read"), options, unreachable)),
+      writeFile: codeOf(() => fs.writeFile(at("w3"), "W", options, unreachable)),
+      appendFile: codeOf(() => fs.appendFile(at("w4"), "A", options, unreachable)),
+      "promises.readFile": await rejected(promises.readFile(at("read"), options)),
+      "promises.writeFile": await rejected(promises.writeFile(at("w5"), "W", options)),
+      "promises.appendFile": await rejected(promises.appendFile(at("w6"), "A", options)),
+      files: readdirSync(String(dir)),
+    }).toEqual({
+      readFileSync: "ERR_INVALID_ARG_TYPE",
+      writeFileSync: "ERR_INVALID_ARG_TYPE",
+      appendFileSync: "ERR_INVALID_ARG_TYPE",
+      readFile: "ERR_INVALID_ARG_TYPE",
+      writeFile: "ERR_INVALID_ARG_TYPE",
+      appendFile: "ERR_INVALID_ARG_TYPE",
+      "promises.readFile": "ERR_INVALID_ARG_TYPE",
+      "promises.writeFile": "ERR_INVALID_ARG_TYPE",
+      "promises.appendFile": "ERR_INVALID_ARG_TYPE",
+      files: ["read"],
+    });
+  });
+
+  it("a sync call leaves no signal on the heap", async () => {
+    using dir = tempDir("fs-sync-signal-heap", { read: "hello" });
+    await using proc = Bun.spawn({
+      cmd: [
+        bunExe(),
+        "-e",
+        `const fs = require("node:fs");
+        for (let i = 0; i < 1000; i++) {
+          const { signal } = new AbortController();
+          // The wrapper of a live signal with a listener stays on the heap while native code counts a pending activity on it.
+          signal.addEventListener("abort", () => {});
+          fs.readFileSync("read", { signal });
+          fs.writeFileSync("write", "W", { signal });
+          fs.appendFileSync("append", "A", { signal });
+        }
+        Bun.gc(true);
+        console.log(require("bun:jsc").heapStats().objectTypeCounts.AbortSignal ?? 0);`,
+      ],
+      env: bunEnv,
+      cwd: String(dir),
+      stderr: "inherit",
+    });
+    const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
+    expect(stdout).toMatch(/^\d+\n$/);
+    expect(Number(stdout)).toBeLessThanOrEqual(10);
+    expect(exitCode).toBe(0);
+  });
+});
+
 describe("writeFile/appendFile data argument", () => {
   it("rejects a String wrapper object on the sync and callback paths, like node", () => {
     // Node only accepts primitive strings here (`new String("x")` is an