[vedchaudhari]

Since #65327 and #65489, errors from fs.promises.readFile(), writeFile() and
appendFile() have no stack frames. This affects v26.8.0+ (readFile()) and
v26.9.0+ (writeFile(), appendFile()); v24.x and v22.x aren't affected.

# main
Error: ENOENT: no such file or directory, open '/missing/config.json'

# this PR
Error: ENOENT: no such file or directory, open '/missing/config.json'
    at readFile (node:internal/fs/promises:2301:7)
    at async loadConfig ([eval]:4:3)

The stack was captured from a C++ callback with no JS frames on it. This captures
it after the await instead, like the rest of fs/promises. The success path is
unchanged, and the fs/readfile-promises and fs/writefile-promises benchmarks
show no difference.

Benchmark results

$ node benchmark/compare.js --old ./node-main --new ./node-pr --runs 10 \
    --set duration=1 --filter readfile-promises --filter writefile-promises fs

macOS arm64, 10 runs per binary. p-values are from a Welch's t-test; * marks p < 0.05.

benchmark	configuration	main (ops/s)	this PR (ops/s)	change	p-value
readfile-promises	concurrent=1 len=1024 encoding=''	56,875	57,183	+0.54%	0.49
readfile-promises	concurrent=1 len=1024 encoding='utf-8'	57,179	58,407	+2.15%	0.10
readfile-promises	concurrent=1 len=16777216 encoding=''	749	743	-0.82%	0.76
readfile-promises	concurrent=1 len=16777216 encoding='utf-8'	424	429	+1.24%	0.71
readfile-promises	concurrent=1 len=33554432 encoding=''	331	332	+0.58%	0.67
readfile-promises	concurrent=1 len=33554432 encoding='utf-8'	175	173	-1.25%	0.62
readfile-promises	concurrent=1 len=4194304 encoding=''	3,047	3,056	+0.29%	0.85
readfile-promises	concurrent=1 len=4194304 encoding='utf-8'	2,578	2,627	+1.93%	0.62
readfile-promises	concurrent=1 len=524288 encoding=''	20,307	20,203	-0.51%	0.80
readfile-promises	concurrent=1 len=524288 encoding='utf-8'	17,994	17,918	-0.42%	0.87
readfile-promises	concurrent=1 len=8388608 encoding=''	1,593	1,576	-1.05%	0.64
readfile-promises	concurrent=1 len=8388608 encoding='utf-8'	1,142	1,139	-0.26%	0.93
readfile-promises	concurrent=10 len=1024 encoding=''	184,282	186,444	+1.17%	0.77
readfile-promises	concurrent=10 len=1024 encoding='utf-8'	185,096	177,114	-4.31%	0.23
readfile-promises	concurrent=10 len=16777216 encoding=''	1,564	1,593	+1.87%	0.44
readfile-promises	concurrent=10 len=16777216 encoding='utf-8'	438	437	-0.30%	0.95
readfile-promises	concurrent=10 len=33554432 encoding=''	797	802	+0.59%	0.90
readfile-promises	concurrent=10 len=33554432 encoding='utf-8'	213	203	-4.79%	0.26
readfile-promises	concurrent=10 len=4194304 encoding=''	6,222	6,135	-1.38%	0.32
readfile-promises	concurrent=10 len=4194304 encoding='utf-8'	2,104	2,104	+0.01%	1.00
readfile-promises	concurrent=10 len=524288 encoding=''	29,102	30,652	+5.32%	0.16
readfile-promises	concurrent=10 len=524288 encoding='utf-8'	28,673	27,922	-2.62%	0.63
readfile-promises	concurrent=10 len=8388608 encoding=''	2,931	2,857	-2.52%	0.20
readfile-promises	concurrent=10 len=8388608 encoding='utf-8'	921	913	-0.93%	0.73
writefile-promises	concurrent=1 size=1024 encodingType='asc'	26,216	26,881	+2.54%	0.65
writefile-promises	concurrent=1 size=1024 encodingType='buf'	24,924	26,980	+8.25%	0.30
writefile-promises	concurrent=1 size=1024 encodingType='utf'	25,694	25,851	+0.61%	0.94
writefile-promises	concurrent=1 size=1048576 encodingType='asc'	2,830	2,870	+1.39%	0.78
writefile-promises	concurrent=1 size=1048576 encodingType='buf'	2,868	3,067	+6.94%	0.04 *
writefile-promises	concurrent=1 size=1048576 encodingType='utf'	2,894	2,879	-0.50%	0.93
writefile-promises	concurrent=1 size=2 encodingType='asc'	28,047	27,462	-2.08%	0.60
writefile-promises	concurrent=1 size=2 encodingType='buf'	27,610	28,756	+4.15%	0.46
writefile-promises	concurrent=1 size=2 encodingType='utf'	29,195	28,797	-1.36%	0.69
writefile-promises	concurrent=1 size=65535 encodingType='asc'	21,725	21,700	-0.11%	0.98
writefile-promises	concurrent=1 size=65535 encodingType='buf'	22,605	25,022	+10.69%	0.09
writefile-promises	concurrent=1 size=65535 encodingType='utf'	19,432	20,483	+5.41%	0.44
writefile-promises	concurrent=10 size=1024 encodingType='asc'	28,006	26,607	-5.00%	0.11
writefile-promises	concurrent=10 size=1024 encodingType='buf'	26,818	26,754	-0.24%	0.96
writefile-promises	concurrent=10 size=1024 encodingType='utf'	27,985	26,590	-4.98%	0.19
writefile-promises	concurrent=10 size=1048576 encodingType='asc'	3,049	2,819	-7.52%	0.14
writefile-promises	concurrent=10 size=1048576 encodingType='buf'	2,997	2,851	-4.85%	0.43
writefile-promises	concurrent=10 size=1048576 encodingType='utf'	2,975	2,940	-1.17%	0.80
writefile-promises	concurrent=10 size=2 encodingType='asc'	28,423	27,387	-3.65%	0.14
writefile-promises	concurrent=10 size=2 encodingType='buf'	27,868	27,790	-0.28%	0.94
writefile-promises	concurrent=10 size=2 encodingType='utf'	27,878	27,841	-0.13%	0.98
writefile-promises	concurrent=10 size=65535 encodingType='asc'	24,832	23,330	-6.05%	0.24
writefile-promises	concurrent=10 size=65535 encodingType='buf'	24,651	25,360	+2.88%	0.62
writefile-promises	concurrent=10 size=65535 encodingType='utf'	22,357	22,614	+1.15%	0.80

1 of 48 configurations is under p < 0.05, about what chance alone gives with
48 comparisons. It's a 1 MiB write (+6.9%), which is above kWriteFileMaxChunkSize
(512 KiB) and doesn't go through the changed code.

AI-assisted. I reproduced the bug and ran the tests and benchmarks myself.

cc @codebytere

[codecov]

Codecov Report

❌ Patch coverage is 84.61538% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.68%. Comparing base (97aed16) to head (b9df2e3).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/fs/promises.js	84.61%	6 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #66757   +/-   ##
=======================================
  Coverage   90.67%   90.68%           
=======================================
  Files         797      797           
  Lines      278773   278807   +34     
  Branches    53607    53613    +6     
=======================================
+ Hits       252778   252836   +58     
+ Misses      16356    16311   -45     
- Partials     9639     9660   +21     

Files with missing lines	Coverage Δ
lib/internal/fs/promises.js	91.37% <84.61%> (+0.15%)	⬆️

... and 36 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[vedchaudhari]

@jasnell since you reviewed #65327 and #65489, would you have a moment to take
a look? It restores the stack traces those round-trip changes dropped for
fs.promises readFile/writeFile errors. Thanks!

[vedchaudhari]

@anonrig since you reviewed #65327 and #65489, would you have a moment to take a look? It restores the stack traces of fs.promises readFile/writeFile errors that those changes dropped. Thanks!

[christianaurichzm]

Thanks for fixing this. I left one comment about permission errors losing the caller's stack frames.

[christianaurichzm]

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]

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]

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]

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