Skip to content
31 changes: 26 additions & 5 deletions src/js/node/fs.promises.ts
Original file line number Diff line number Diff line change
Expand Up @@ -684,23 +684,26 @@
const fd = this[kFd];
throwEBADFIfNecessary("writeFile", fd);
let encoding: string = "utf8";
let flush = false;
let signal: AbortSignal | undefined = undefined;

if (options == null || typeof options === "function") {
} else if (typeof options === "string") {
encoding = options;
} else {
encoding = options?.encoding ?? encoding;
flush = options?.flush ?? flush;
signal = options?.signal ?? undefined;
}

try {
this[kRef]();
return await writeFile(fd, data, {
encoding,
flush,
flag: this[kFlag],
signal,
});

Check warning on line 706 in src/js/node/fs.promises.ts

View check run for this annotation

Claude / Claude Code Review

fh.writeFile: string data now fsyncs the caller's fd, iterable data does not — the two data paths diverge

After 9ddc998 gated the iterable-path fsync on `mustClose`, `fh.writeFile(data, {flush:true})` now diverges by data type: string/Buffer data reaches native `_writeFile(fd, ..., {flush:true})`, whose fsync at node_fs.rs:7520 is not gated on `PathOrFileDescriptor::Path` and so *does* sync the caller's fd, while iterable data hits `mustClose=false` and skips it. The comment here ("never a caller's FileHandle"), the test comment at the fsync-count fixture, and the PR description's "keeps the two dat
Comment thread
claude[bot] marked this conversation as resolved.
Resolved
} finally {
this[kUnref]();
}
Expand Down Expand Up @@ -1609,10 +1612,13 @@
async function writeFileAsyncIterator(fdOrPath, iterable, optionsOrEncoding, flag, mode) {
let encoding;
let signal: AbortSignal | null = null;
let flush = false;
if (typeof optionsOrEncoding === "object") {
encoding = optionsOrEncoding?.encoding ?? (encoding || "utf8");
flag = optionsOrEncoding?.flag ?? (flag || "w");
mode = optionsOrEncoding?.mode ?? (mode || 0o666);
flush = optionsOrEncoding?.flush ?? false;
validateBoolean(flush, "options.flush");
Comment thread
claude[bot] marked this conversation as resolved.
Resolved
signal = optionsOrEncoding?.signal ?? null;
if (signal?.aborted) {
throw $makeAbortError(undefined, { cause: signal.reason });
Expand All @@ -1628,9 +1634,9 @@
throw new TypeError(`Unknown encoding: ${encoding}`);
}

let mustClose = typeof fdOrPath === "string";
// Callers already unwrapped a FileHandle to its fd; fs.open validates everything else.
const mustClose = typeof fdOrPath !== "number";
if (mustClose) {
// Rely on fs.open for further argument validaiton.
fdOrPath = await fs.open(fdOrPath, flag, mode);
}

Expand All @@ -1641,12 +1647,15 @@

let totalBytesWritten = 0;

let error: Error | undefined;
// Tracked separately from `error`: an iterable may throw a falsy value.
let failed = false;
let error: unknown;

try {
totalBytesWritten = await writeFileAsyncIteratorInner(fdOrPath, iterable, encoding, signal);
} catch (err) {
error = err as Error;
failed = true;
error = err;
}

// Handle cleanup outside of try-catch
Expand All @@ -1657,15 +1666,27 @@
} catch {}
}

// Like node, `flush` syncs the file this call opened (never a caller's
// FileHandle), and only after a write that succeeded.
Comment thread
robobun marked this conversation as resolved.
Resolved
if (flush && !failed) {
try {
await fs.fsync(fdOrPath);
} catch (err) {
failed = true;
error = err;
}
}

await fs.close(fdOrPath);
}

// Abort signal shadows other errors
if (signal?.aborted) {
failed = true;
error = $makeAbortError(undefined, { cause: signal.reason });
}

if (error) {
if (failed) {
throw error;
}
}
Loading
Loading