Skip to content

fs: preserve readFile() errors for numeric file descriptors - #64368

Open
jibin7jose wants to merge 2 commits into
nodejs:mainfrom
jibin7jose:fix-fs-readfile-buffer-fd-error
Open

fs: preserve readFile() errors for numeric file descriptors#64368
jibin7jose wants to merge 2 commits into
nodejs:mainfrom
jibin7jose:fix-fs-readfile-buffer-fd-error

Conversation

@jibin7jose

@jibin7josejibin7jose commented Jul 8, 2026

Copy link
Copy Markdown

Summary

fs.readFile() with a numeric file descriptor and the new buffer option could lose errors on the user-fd path.

Root cause

In lib/internal/fs/read/context.js, ReadFileContext.close(err) did not forward err when isUserFd was true. As a result, failures that should have reached the callback, including an undersized caller-provided buffer, could be treated as successful completion.

Fix

Forward the close error for user-supplied file descriptors so the final callback receives the actual failure instead of silently succeeding.

Tests

Add regression coverage for the numeric file descriptor path with:

  • a preallocated buffer
  • a function-returned buffer
  • a buffer that is too small

This verifies the error is preserved for the callback-based API.

Fixes: #64367

@nodejs-github-botnodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jul 8, 2026
@jibin7josejibin7jose changed the title fs: preserve readFile() errors for numeric fdsfs: preserve readFile() errors for numeric file descriptorsJul 8, 2026
@jibin7jose

Copy link
Copy Markdown
Author

Hi @joyeecheung, could you please take a look at this PR when you have a chance? It fixes an issue where fs.readFile() with a numeric file descriptor and the new buffer option could lose errors on the user-FD path. I've added regression tests covering the affected scenarios. Any feedback would be greatly appreciated. Thank you!

@joyeecheungjoyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not very familiar with this code any more, but it seems straightforward enough and from a glance it makes sense to me. Would be better if @addaleax can take a look since it was introduced in #35487

Comment threadtest/parallel/test-fs-readfile-buffer-option.js Outdated
@joyeecheungjoyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2026
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 20, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecovBot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.23%. Comparing base (1912893) to head (8b57081).
⚠️ Report is 97 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #64368 +/- ##
==========================================
- Coverage 90.25% 90.23% -0.03% 
==========================================
Files 741 741 Lines 241165 241207 +42 Branches 45423 45426 +3 ==========================================
- Hits 217673 217655 -18 - Misses 15069 15120 +51 - Partials 8423 8432 +9 
Files with missing linesCoverage Δ
lib/internal/fs/read/context.js94.92% <100.00%> (ø)

... and 46 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.

Signed-off-by: jibin7jose <jibinjose884@gmail.com>
Signed-off-by: jibin7jose <jibinjose884@gmail.com>
@jibin7jose
jibin7joseforce-pushed the fix-fs-readfile-buffer-fd-error branch from 8b57081 to 64d5e81CompareJuly 21, 2026 06:24
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fsIssues and PRs related to the fs subsystem / file system.needs-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs: preserve readFile() errors when using a numeric fd and buffer option

4 participants

@jibin7jose@nodejs-github-bot@joyeecheung@StefanStojanovic