Skip to content

vfs: align virtual file handles with open(2) - #65854

Open
pipobscure wants to merge 2 commits into
nodejs:mainfrom
pipobscure:vfs-handle-semantics
Open

vfs: align virtual file handles with open(2)#65854
pipobscure wants to merge 2 commits into
nodejs:mainfrom
pipobscure:vfs-handle-semantics

Conversation

@pipobscure

Copy link
Copy Markdown
Contributor

vfs: align virtual file handles with open(2)

A file descriptor obtained on a mounted path answers several node:fs
calls differently from one on a real file, in both providers:

  • writeFileSync(path, data, { flag: 'r+' }) replaces the whole file
    instead of overwriting bytes from offset 0 and keeping the tail.
  • Numeric open flags are mapped by treating any write-ish bit as "w":
    O_WRONLY alone truncates, and O_RDONLY | O_CREAT opens the file
    write-only and truncates it.
  • A handle opened with "a+" starts its read offset at the end of the
    file, so the first read returns nothing; O_APPEND only affects writes.

The ZipProvider handle additionally:

  • throws EISDIR instead of EBADF when reading a write-only handle or
    writing a read-only one;
  • leaves stale bytes in place when ftruncate grows a file that was
    previously shrunk, where real files read back as zeros;
  • rejects a BigInt position with a TypeError from mixing number and
    BigInt arithmetic.

This adds a test that runs the same sequence of calls against a memory
mount and a ZIP mount and expects the real-fs result, so every
divergence shows up as its own failing case.

Proposed solution: decode numeric flags bit by bit (O_TRUNC decides
truncation, O_CREAT decides creation, O_WRONLY/O_RDWR decide access)
instead of collapsing them to a flag string; keep the read offset at 0
for append handles and only force writes to the end; make the handle
writeFile for non-truncating flags write at offset 0 without
shrinking; in the ZIP handle use EBADF for access-mode violations,
zero-fill on growth in #doTruncate, and coerce position with
Number() as the memory handle does.

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.

A file descriptor obtained on a mounted path answers several `node:fs`
calls differently from one on a real file, in both providers:

* `writeFileSync(path, data, { flag: 'r+' })` replaces the whole file
  instead of overwriting bytes from offset 0 and keeping the tail.
* Numeric open flags are mapped by treating any write-ish bit as "w":
  `O_WRONLY` alone truncates, and `O_RDONLY | O_CREAT` opens the file
  write-only and truncates it.
* A handle opened with "a+" starts its read offset at the end of the
  file, so the first read returns nothing; O_APPEND only affects writes.

The ZipProvider handle additionally:

* throws EISDIR instead of EBADF when reading a write-only handle or
  writing a read-only one;
* leaves stale bytes in place when `ftruncate` grows a file that was
  previously shrunk, where real files read back as zeros;
* rejects a BigInt `position` with a TypeError from mixing number and
  BigInt arithmetic.

This adds a test that runs the same sequence of calls against a memory
mount and a ZIP mount and expects the real-fs result, so every
divergence shows up as its own failing case.

Proposed solution: decode numeric flags bit by bit (O_TRUNC decides
truncation, O_CREAT decides creation, O_WRONLY/O_RDWR decide access)
instead of collapsing them to a flag string; keep the read offset at 0
for append handles and only force writes to the end; make the handle
`writeFile` for non-truncating flags write at offset 0 without
shrinking; in the ZIP handle use EBADF for access-mode violations,
zero-fill on growth in `#doTruncate`, and coerce `position` with
`Number()` as the memory handle does.

Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
Decode open flags once, in `VirtualFileHandle`, into what they ask for
(readable, writable, create, exclusive, truncate, append) and let both
providers and both handle classes act on those bits instead of on a
flag string. Numeric `fs.constants` combinations that have no string
spelling keep their meaning: a plain `O_WRONLY` neither creates nor
truncates, and `O_RDONLY | O_CREAT` opens an existing file readable and
intact. `handle.flags` stays a string, now purely descriptive.

On top of that, in both handles:

* An append handle no longer starts its read offset at the end of the
  file; O_APPEND only forces writes there.
* `writeFile` writes from the current position, like
  `filehandle.writeFile()`, so "r+" overwrites in place and keeps any
  tail while "w" has already truncated.

And in the ZipProvider handle:

* Access-mode violations are EBADF rather than EISDIR.
* `ftruncate` zero-fills the region it grows into instead of exposing
  bytes cut off by an earlier shrink.
* A BigInt `position` is accepted.
* `readSync` and `writeSync` return the byte count, as `fs.readSync`,
  `fs.writeSync` and the memory handle do, instead of the promise-shaped
  `{ bytesRead }` object.

The existing ZipProvider handle test asserted the old EISDIR code, the
object-shaped `readSync` result and the end-of-file read offset for
append handles; it now asserts the corrected behaviour.

Signed-off-by: Philipp Dunkel <pip@pipobscure.com>
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. vfs Issues and PRs related to the virtual filesystem subsystem. labels Sep 6, 2026
@pipobscure
pipobscure marked this pull request as ready for review September 6, 2026 14:24
@trivikr trivikr added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 6, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codecov

codecov Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.14530% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.20%. Comparing base (7991140) to head (6c91c33).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/vfs/providers/memory.js 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65854      +/-   ##
==========================================
+ Coverage   90.19%   90.20%   +0.01%     
==========================================
  Files         771      771              
  Lines      264622   264584      -38     
  Branches    50223    50206      -17     
==========================================
- Hits       238663   238661       -2     
+ Misses      16965    16909      -56     
- Partials     8994     9014      +20     
Files with missing lines Coverage Δ
lib/internal/vfs/file_handle.js 99.46% <100.00%> (+0.56%) ⬆️
lib/internal/vfs/providers/ziparchive.js 98.41% <100.00%> (+0.48%) ⬆️
lib/internal/vfs/providers/memory.js 94.99% <87.50%> (-0.20%) ⬇️

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

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

Labels

needs-ci PRs that need a full CI run. vfs Issues and PRs related to the virtual filesystem subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants