Skip to content

fix return value of fs APIs #58996

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.

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

Merged
merged 1 commit into from
Jul 20, 2025

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Jul 8, 2025

Fixed: #58747

The C++ layer just reject the Promise but does not return the Promise to JS layer when an error occurs(Libuv error or permission error).

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jul 8, 2025
@theanarkh theanarkh force-pushed the fix_fs_return_value branch from a3f7322 to c2451ee Compare July 8, 2025 18:29
@bakkot
Copy link
Contributor

bakkot commented Jul 8, 2025

For testing, you can after this line the test

  assert.rejects(async () => {
    await fs.promises.mkdtemp(path.join(blockedFolder, 'any-folder'));
  }, {
    code: 'ERR_ACCESS_DENIED',
    permission: 'FileSystemWrite',
  });

That's what I had originally done when I ran into this.

@theanarkh theanarkh force-pushed the fix_fs_return_value branch 2 times, most recently from 81c0615 to ee0fda6 Compare July 9, 2025 01:27
@theanarkh theanarkh marked this pull request as ready for review July 9, 2025 01:51
@theanarkh theanarkh force-pushed the fix_fs_return_value branch from ee0fda6 to 4a03ac6 Compare July 9, 2025 01:56
Copy link

codecov bot commented Jul 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.05%. Comparing base (9c025a9) to head (0b4820b).
Report is 53 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58996      +/-   ##
==========================================
- Coverage   90.07%   90.05%   -0.03%     
==========================================
  Files         641      645       +4     
  Lines      188594   189168     +574     
  Branches    36998    37097      +99     
==========================================
+ Hits       169869   170346     +477     
- Misses      11438    11519      +81     
- Partials     7287     7303      +16     
Files with missing lines Coverage Δ
src/node_file-inl.h 87.66% <100.00%> (+0.39%) ⬆️
src/node_file.cc 75.85% <ø> (-0.05%) ⬇️

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

@theanarkh theanarkh force-pushed the fix_fs_return_value branch from 4a03ac6 to 0b4820b Compare July 10, 2025 18:31
@RafaelGSS RafaelGSS added the permission Issues and PRs related to the Permission Model label Jul 14, 2025
@RafaelGSS RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 14, 2025
@RafaelGSS
Copy link
Member

PTAL @nodejs/fs

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 15, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@theanarkh theanarkh added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 18, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 20, 2025
@nodejs-github-bot nodejs-github-bot merged commit bf2384f into nodejs:main Jul 20, 2025
68 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in bf2384f

aduh95 pushed a commit that referenced this pull request Jul 21, 2025
PR-URL: #58996
Fixes: #58747
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Jason Zhang <[email protected]>
Reviewed-By: Ethan Arrowood <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fs.promises.mkdtemp has internal error under --permissions
6 participants