Repository navigation
fix: report snapshot symlinks as links in lstat, readlink and readdir - #310
Conversation
robertsLando
left a comment
There was a problem hiding this comment.
deep-review: Ship with minor changes
Scope: this PR's own two commits only (15b6da1..a17221e): 11 files, +769 / −64. #296's commits were not reviewed here. CI: 28/28 green.
Merge order: stacked on #296, which is still open, and #296's remote head (f0d409d) lacks 15b6da1 refactor(prelude): narrow #296 to symlink resolution. Land that on #296 first, then rebase this PR.
Top risks: (1) readlink returns the absolute resolved target, which a major release makes the contract; (2) SEA readlink with hex/base64 encoding loses the /snapshot prefix; (3) classic and SEA throw different readlink ENOENT errors.
Strengths: builds on #296's shared resolver instead of adding a second path, uses vfs 0.4.0's own extension points (lstatSync/readlinkSync), and Security came back clean.
Not anchored to a diff line
Design/API
docs-site/guide/snapshot-fs.md:breaking-change-absent-user-docs: the breaking-change notice and workarounds (globfollow: true,fs.cpdereference) live only in the contributor-facingdocs/ARCHITECTURE.md. Add a short Symlinks section to the user guide.Readability: comment trims. Cut the 5-line readlink banner in
bootstrap.js, the history comments ingetFileTypes, theparentPathalias ofpath_, the 3-linetoCallerPathcomment, the vfs#4 reference inSEAProvider.readlinkSync, and doc blocks inbootstrap-shared.jsthat are longer than their bodies._einval/_enoentare near-identical;readFileSyncparses the encoding inline, duplicatingreadlinkEncoding; the localnoopduplicates the sharedalwaysFalse; thetypeof === 'string'inherited-key guard inlinkTargethas no caller passing such keys; theARCHITECTURE.mdline count goes stale; the body of commit8179dd6still says package.json pins the vfs PR commit (fix in the squash message).
FYI
- A dangling link (readdir says link, lstat throws ENOENT) is not reachable today:
Walker.appendSymlinkalways records a stat forrealFile. If it ever matters, build a link stat instead of throwing. - Promise and callback forms reach the new code in both modes. SEA: vfs 0.4.0 routes
fs.readlink/fs.lstat/fs.promises.*to the provider. Classic:util.promisify.
Coverage: Correctness, Security, Design/API, Simplicity, DRY, Readability, Operability, Tests. Clean on: Security. Not run: Performance (no files in its lane).
a17221e to
5426852
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #310 +/- ##
==========================================
- Coverage 87.65% 87.62% -0.04%
==========================================
Files 23 23
Lines 7917 7926 +9
Branches 1213 1214 +1
==========================================
+ Hits 6940 6945 +5
- Misses 970 974 +4
Partials 7 7
🚀 New features to boost your workflow:
|
finalize-pr — 4 rounds (all full), merge-readyDecided
Moved to #296 — 4 Minor notes on #296's own code, posted before it merged: macOS Base sync — Dropped (Minor/Nit, with reason)
|
Inside a packaged binary, snapshot symlinks looked like what they point
at, unlike outside a binary. Both modes now answer from their symlink
record the way Node does:
- readdir({ withFileTypes: true }) reports a link as a link
- lstat describes the link: S_IFLNK type bits, size of the target path
- readlink returns the absolute target in the snapshot; EINVAL for a
non-link, ENOENT for a missing path; any encoding, including hex and
base64 in SEA
- Dirent drops the path alias on Node 24, as fs.Dirent does
The walker now keys a link inside a linked directory under the real
directory, so reallib/inner.js resolves when the app only required
lib/inner.js (it threw ENOENT before).
Requires @roberts_lando/vfs ^0.4.0 (robertsLando/vfs#4).
64ab493 to
eb9ed2e
Compare
Follow-up to #296, split out so the #295 fix didn't wait on a vfs release. Needs
@roberts_lando/vfs^0.4.0 (robertsLando/vfs#4), which this PR bumps.What changes
Inside a packaged binary, snapshot symlinks used to look like what they point at. Now both modes answer from their own symlink record, the way Node does outside a binary:
readdir({ withFileTypes: true })manifest.symlinkslstatS_IFLNKtype bits, size of the target pathreadlinkEINVALfor a non-link,ENOENTfor a missing pathC:\snapshot\...form on Windows, likerealpath(#305)One shared
linkTarget()inbootstrap-shared.jsdecides what is a link for all three calls in both modes. It also finds links the walker recorded only under their real parent:lib/other.js, wherelib -> realliband the walker only sawreallib/other.js.Dirent,asSymlinkStatandapplyReadlinkEncodingare shared too. Both modes find the link throughresolveKey.parent(), a new resolver step that follows a path's parents but not its last component, which is what POSIX does before reading a link.Direntdrops thepathalias on Node 24, asfs.Direntdoes.In SEA,
readlinkapplies the caller's encoding after the VFS adds the mount prefix, so'hex'/'base64'targets keep/snapshot.The walker now keys a link that sits inside a linked directory under the real directory, and records the directory link with it. Before, an app that required
lib/inner.js(withlib -> reallibandreallib/inner.jsa link) gotreallib/inner.jsmissing from the snapshot: every fs call on that spelling threwENOENT, on 6.24.0 too. Both spellings resolve now.Not a breaking change
This is a bug fix: packaged apps now get what plain
nodegives. Code that relied on pkg's old, wrong answer may notice, but only when the snapshot contains symlinks (pnpm and workspacenode_modules,node_modules/.bin, links among assets):d.isDirectory()/d.isFile()filters skip linked entries, andglobwithoutfollow: trueno longer descends into linked directories. fast-glob and readdirp follow links by default and are unaffected.lstatSync(link).isDirectory()/.isFile()arefalse.readlinkreturns the walker's absolute, resolved target, not the raw link body. It always points inside the snapshot. Tools that re-create links from it (tar, archiver, hand-written copies) write absolute/snapshot/...links;fs.cpcan't copy out of the snapshot in either mode, before or after this PR.Everything except the
readlinkvalue matches unpackaged Node, so code that works undernodekeeps working packaged. In practice only traditional mode changes: in SEA, 6.24.0'swithFileTypesentries had noDirentmethods, so nothing could rely on them. The user guide (Snapshot filesystem → Symlinks) anddocs/ARCHITECTURE.mddocument it.Tests
test-99-#295asserts one contract for both modes, on node22 and node24:readlink: full value,'buffer'and'hex'encodings,EINVAL/ENOENTwith the caller'spathlstaton directory links, file links, links reached through the real parent, a nested link by its real path, plain files reached through a linked dir, and missing pathsreaddirof the linked and the real directory, with the exactparentPathtest/unit/dirent-shared.test.tscoverslinkTarget,Dirent,asSymlinkStatandapplyReadlinkEncoding.