Skip to content

fs: restore stack traces of fs/promises readFile and writeFile errors #66757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vedchaudhari wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fs: restore stack traces of fs/promises readFile and writeFile errors #66757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 39 additions & 19 deletions lib/internal/fs/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2226,8 +2226,14 @@ async function writeFile(path, data, options) {

if (!flush && !isCustomIterable(data) && data.byteLength <= kWriteFileMaxChunkSize) {
path = getValidatedPath(path);
await writeFileInOneRoundTrip(path, stringToFlags(flag, 'options.flag'),
parseFileMode(options.mode, 'mode', 0o666), data);
const flagsNumber = stringToFlags(flag, 'options.flag');
const mode = parseFileMode(options.mode, 'mode', 0o666);
try {
await writeFileInOneRoundTrip(path, flagsNumber, mode, data);
} catch (err) {
ErrorCaptureStackTrace(err);
throw err;
}
checkAborted(options.signal); // An abort during the write still wins.
return;
}
Expand All @@ -2244,29 +2250,32 @@ async function writeFile(path, data, options) {

/**
* Open + write + close as one thread pool round trip.
* Throws synchronously if the permission model denies access, while the
* caller's frames are still on the stack. The caller is responsible for
* capturing the stack trace of a rejection, as no JS frames are on the stack
* when the job completes.
* @param {string|Buffer} path Validated path
* @param {number} flagsNumber
* @param {number} mode
* @param {ArrayBufferView} data
* @returns {Promise<void>}
*/
function writeFileInOneRoundTrip(path, flagsNumber, mode, data) {
return new Promise((resolve, reject) => {
const job = new binding.WriteFileJob(path, flagsNumber, mode, data);
const job = new binding.WriteFileJob(path, flagsNumber, mode, data);
const promise = new Promise((resolve, reject) => {
job.ondone = (err) => {
if (err != null) {
ErrorCaptureStackTrace(err, writeFileInOneRoundTrip);
reject(err);
} else {
resolve();
}
};
const accessError = job.run(path);
if (accessError !== undefined) {
ErrorCaptureStackTrace(accessError, writeFileInOneRoundTrip);
reject(accessError);
}
});
const accessError = job.run(path);
if (accessError !== undefined) {
throw accessError;
}
return promise;
}

function isCustomIterable(obj) {
Expand Down Expand Up @@ -2309,7 +2318,15 @@ async function readFile(path, options) {
// up to one chunk; larger files come back as an open fd + size and are
// read by readFileHandle() as before.
path = getValidatedPath(path);
const { 0: buffer, 1: fd, 2: size } = await readFileInOneRoundTrip(path, stringToFlags(flag));
const flagsNumber = stringToFlags(flag);
let result;
try {
result = await readFileInOneRoundTrip(path, flagsNumber);
} catch (err) {
ErrorCaptureStackTrace(err);

@christianaurichzm christianaurichzm Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This loses the caller's frames for permission errors:

const { readFile } = require('node:fs/promises');

function loadConfig() {
  return readFile('/notallowed/config.json').catch(console.error);
}
loadConfig();

With node --permission --allow-fs-read=. repro.cjs, main shows at loadConfig. With this PR, the stack only has readFile and processTicksAndRejections.

job.run() returns the permission error synchronously, so capturing its stack after the await loses those frames. Could we keep the existing capture for accessError in the helper and only recapture errors from the async completion?

writeFile() has the same issue, and appendFile() loses the caller's frame even when awaited. Please add a permission test with a non-async caller that checks its frame is preserved in the stack. The existing fixtures don't check the stack.

@vedchaudhari vedchaudhari Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I only tested ENOENT and didn't think about the permission error. I changed the helpers to throw it directly instead of rejecting, so the caller shows up in the stack again, appendFile() too.
I’ve also added a test that fails without this change. Fixed in 42e6ebd. If you’d prefer to keep the capture in the helper, I can change it.

@christianaurichzm christianaurichzm Oct 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, no need to move the capture back into the helper. The throw happens before anything is awaited, so the catch still sees the caller's frames.

One non-blocking nit on the new test. The child doesn't check that all four assert.rejects() calls finished. If a denied call ever stayed pending, the process would exit with 0 without checking any stack. A counter checked on exit would cover it. The existing fixtures would still catch a hang, so this is only about this test.

@vedchaudhari vedchaudhari Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I've added a counter that's checked on exit, so the test now fails if any of the calls never finishes.

throw err;
}
const { 0: buffer, 1: fd, 2: size } = result;
if (fd === -1) {
checkAborted(options.signal); // An abort during the read still wins.
return options.encoding ? buffer.toString(options.encoding) : buffer;
Expand All @@ -2323,28 +2340,31 @@ async function readFile(path, options) {
}

/**
* Throws synchronously if the permission model denies access, while the
* caller's frames are still on the stack. The caller is responsible for
* capturing the stack trace of a rejection, as no JS frames are on the stack
* when the job completes.
* @param {string|Buffer} path Validated path
* @param {number} flagsNumber
* @returns {Promise<[Buffer|undefined, number, number|undefined]>} [buffer, -1] or [undefined, fd, size]
*/
function readFileInOneRoundTrip(path, flagsNumber) {
return new Promise((resolve, reject) => {
const job = new binding.ReadFileJob(path, flagsNumber, kReadFileBufferLength);
const job = new binding.ReadFileJob(path, flagsNumber, kReadFileBufferLength);
const promise = new Promise((resolve, reject) => {
job.ondone = (err, buffer, fd, size, closeErr) => {
const error = err ?? closeErr;
if (error != null) {
ErrorCaptureStackTrace(error, readFileInOneRoundTrip);
reject(error);
} else {
resolve([buffer, fd, size]);
}
};
const accessError = job.run(path);
if (accessError !== undefined) {
ErrorCaptureStackTrace(accessError, readFileInOneRoundTrip);
reject(accessError);
}
});
const accessError = job.run(path);
if (accessError !== undefined) {
throw accessError;
}
return promise;
}

async function* _watch(filename, options = kEmptyObject) {
Expand Down
25 changes: 25 additions & 0 deletions test/parallel/test-fs-promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const fs = require('fs');
const fsPromises = fs.promises;
const {
access,
appendFile,
chmod,
chown,
copyFile,
Expand Down Expand Up @@ -62,6 +63,30 @@ assert.strictEqual(
}
).then(common.mustCall());

assert.rejects(
readFile('this file does not exist'),
{
code: 'ENOENT',
stack: /at async assert\.rejects/
}
).then(common.mustCall());

assert.rejects(
writeFile(path.join('this dir does not exist', 'file'), 'data'),
{
code: 'ENOENT',
stack: /at async assert\.rejects/
}
).then(common.mustCall());

assert.rejects(
appendFile(path.join('this dir does not exist', 'file'), 'data'),
{
code: 'ENOENT',
stack: /at async assert\.rejects/
}
).then(common.mustCall());

assert.rejects(
access(__filename, 8),
{
Expand Down
42 changes: 42 additions & 0 deletions test/parallel/test-permission-fs-promises-stack-trace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
'use strict';

require('../common');
const assert = require('assert');
const { spawnSync } = require('child_process');
const tmpdir = require('../common/tmpdir');

tmpdir.refresh();

// The permission model denies access before the first await, so the stack
// still has the frames of a caller that does not await the promise.
const { status, stderr } = spawnSync(
process.execPath,
[
'--permission',
'-e',
`
const assert = require('assert');
const { readFile, writeFile, appendFile } = require('fs/promises');
const target = ${JSON.stringify(tmpdir.resolve('file'))};

function readCaller() { return readFile(target); }
function writeCaller() { return writeFile(target, 'data'); }
function appendCaller() { return appendFile(target, 'data'); }
async function awaitAppendCaller() { await appendFile(target, 'data'); }

const cases = [
[readCaller, /at readCaller /],
[writeCaller, /at writeCaller /],
[appendCaller, /at appendCaller /],
[awaitAppendCaller, /at awaitAppendCaller /],
];
let checked = 0;
for (const [caller, stack] of cases) {
assert.rejects(caller(), { code: 'ERR_ACCESS_DENIED', stack })
.then(() => checked++);
}
process.on('exit', () => assert.strictEqual(checked, cases.length));
`,
],
);
assert.strictEqual(status, 0, stderr.toString());
Loading