Skip to content

vfs: fs hook gaps - #65852

Open
pipobscure wants to merge 2 commits into
nodejs:mainfrom
pipobscure:vfs-fs-hook-gaps
Open

vfs: fs hook gaps#65852
pipobscure wants to merge 2 commits into
nodejs:mainfrom
pipobscure:vfs-fs-hook-gaps

Conversation

@pipobscure

@pipobscure pipobscure commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

vfs: close gaps in the fs hooks for mounted paths

Several node:fs entry points behave differently for a mounted path
than for a real one, because of how the call reaches the VFS hooks:

  • fs.watchFile and fs.promises.watch call handler methods that do
    not exist, so they throw a TypeError instead of watching.
  • fs.watch on a path that does not exist returns a polling watcher
    instead of throwing ENOENT, and that watcher keeps the process alive.
  • fs.utimesSync and fs.readdirSync consult the hook before
    validating
    their arguments: numeric-string timestamps are ignored, an object
    timestamp becomes NaN, and an invalid encoding is accepted.
  • fs.futimesSync and fs.fchmodSync on a virtual descriptor are
    no-ops while the path forms of the same operations work.
  • fs.mkdtempSync with a prefix ending in a separator creates the
    directory next to the intended parent, because the prefix is resolved
    as a path before the suffix is appended.
  • fs.mkdirSync({ recursive: true }) returns the provider-relative path
    of the first directory created instead of the mounted path.
  • Disposing an already closed virtual Dir asynchronously rejects with
    ERR_DIR_CLOSED; the real Dir treats disposal as idempotent.

Note: Since these are gaps/defects in existing functionality, I decided to create the failing tests first (first commit) and then add the fix/solution as a second commit. That way whoever wants to review this can first prove out the issue, before applying the solution.

This goes with the VFS work by @mcollina and the bug-fix PRs by @trivikr.

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Sep 6, 2026
@pipobscure
pipobscure marked this pull request as ready for review September 6, 2026 14:23
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 6, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 6, 2026
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@codecov

codecov Bot commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.67391% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.16%. Comparing base (0de4fcc) to head (c4a7af8).
⚠️ Report is 38 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/vfs/setup.js 70.58% 15 Missing ⚠️
lib/internal/vfs/dir.js 50.00% 3 Missing ⚠️
lib/internal/vfs/file_system.js 96.87% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #65852      +/-   ##
==========================================
- Coverage   90.17%   90.16%   -0.02%     
==========================================
  Files         771      771              
  Lines      265470   265548      +78     
  Branches    50463    50499      +36     
==========================================
+ Hits       239383   239426      +43     
- Misses      17055    17074      +19     
- Partials     9032     9048      +16
Files with missing lines Coverage Δ
lib/fs.js 97.30% <100.00%> (-0.01%) ⬇️
lib/internal/fs/promises.js 91.02% <100.00%> (+0.20%) ⬆️
lib/internal/vfs/file_handle.js 99.49% <100.00%> (+0.03%) ⬆️
lib/internal/vfs/file_system.js 99.61% <96.87%> (-0.08%) ⬇️
lib/internal/vfs/dir.js 97.24% <50.00%> (-2.76%) ⬇️
lib/internal/vfs/setup.js 86.90% <70.58%> (-0.90%) ⬇️

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

mcollina
mcollina reviewed Sep 6, 2026
View reviewed changes
Comment thread lib/internal/fs/promises.js Outdated
@pipobscure

This comment was marked as outdated.

Sign in to view
mcollina
mcollina approved these changes Sep 7, 2026
View reviewed changes

@mcollina mcollina 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.

lgtm

@pipobscure

This comment was marked as outdated.

Sign in to view
@pipobscure

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@trivikr trivikr added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 8, 2026
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@panva panva mentioned this pull request Sep 11, 2026
Open
@pipobscure
vfs: close the fs hook gaps for mounted paths
2ceca4a 
Several `node:fs` entry points behave differently for a mounted path
than for a real one, because of how the call reaches the VFS hooks.
Make them behave as they do for a real path:

* Add the `watchFile`, `unwatchFile` and `promisesWatch` handlers,
  backed by the provider's stat watcher and async watcher; those calls
  threw a TypeError before. Have `watch` refuse a path that does not
  exist with ENOENT instead of handing back a watcher that polls
  forever and keeps the process alive.
* Convert timestamps and validate arguments before the hook runs in
  `utimes`, `lutimes` and `readdir` (sync, callback and promise forms),
  so a mounted path gets the same ERR_INVALID_ARG_* errors and the same
  seconds-since-epoch numbers as a real one.
* Pass the mode and times through to the `fchmod` and `futimes` hooks
  and route them to the handle's entry, so descriptor operations take
  effect like their path forms instead of being no-ops; the memory
  handle validates the way a FileHandle would since one calls it
  directly.
* Treat a `mkdtemp` prefix as text rather than a path when it ends in a
  separator, so the directory is created inside the intended parent.
* Map the first directory a recursive `mkdir` created back under the
  mount point instead of returning the provider-relative path.
* Make disposing an already closed virtual `Dir` a no-op, as on the
  native `Dir`, instead of rejecting with ERR_DIR_CLOSED.

test-vfs-fs-hook-gaps adds a test per gap, stating the real-fs outcome
as the expectation. The existing file handle test asserted that
`chmod()` and `utimes()` without arguments were no-ops; they now
validate and apply, so it exercises that instead.

Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
@pipobscure

pipobscure commented Sep 11, 2026

Copy link
Copy Markdown
Contributor Author

Rebased on latest main since that moved significantly

mcollina
mcollina approved these changes Sep 11, 2026
View reviewed changes

@mcollina mcollina 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.

lgtm

mcollina
mcollina reviewed Sep 11, 2026
View reviewed changes
Comment thread test/parallel/test-vfs-fs-matches-real-fs.js
@@ -0,0 +1,111 @@
// Flags: --experimental-vfs

@mcollina mcollina Sep 11, 2026

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.

Can you name this file differently? "hook gaps" has no context

@pipobscure pipobscure Sep 11, 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.

Renamed. Do you want me to split it into topics? watchers, argument validation and descriptor calls? Or leave it as a single file for now?

@pipobscure
test: rename test-vfs-fs-hook-gaps
c4a7af8 
The test checks that `node:fs` calls on a mounted path give the same
result as on a real filesystem. With the gaps closed, "hook gaps" no
longer describes it, so name it for what every case compares against.

Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 12, 2026
@github-actions github-actions Bot added request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Sep 12, 2026
@github-actions

github-actions Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor
Failed to start CI
- Validating Jenkins credentials
✔  Jenkins credentials valid
- Querying data for job/node-test-pull-request/77227/
SyntaxError: Unexpected token '<', ..."    
  https://github.com/nodejs/node/actions/runs/34683155451

@aduh95 aduh95 added commit-queue-squash PRs the Commit Queue should land as one squashed commit. and removed request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. labels Sep 12, 2026
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/77415/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes
@panva panva panva approved these changes
@jasnell jasnell jasnell approved these changes
@trivikr trivikr trivikr approved these changes

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue-squash PRs the Commit Queue should land as one squashed commit. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pipobscure @nodejs-github-bot @mcollina @panva @jasnell @trivikr @aduh95