Repository navigation
feat(prelude)!: report snapshot symlinks in lstat, readlink and readdir - #310
Draft
robertsLando wants to merge 23 commits into
Draft
robertsLando wants to merge 23 commits into
robertsLando wants to merge 23 commits into
Conversation
Addresses review findings on PR #296. - Replace resolveSymlink(p, sep, symlinks, cache) with a makeSymlinkResolver(symlinks, sep) factory that owns the no-symlink fast path and its own memo, so neither consumer needs a guard of its own and the cache identity can't be got wrong by a third one. - Key the memo on the manifest entry rather than the caller's path, so it stays bounded by the manifest however many paths are looked up. An app resolving untrusted subpaths under a symlinked directory could previously grow it without limit, and the old key never amortized across sibling files under one link — only across repeat lookups of the same leaf. - Precompute which path depths can host a symlink key, so the walk slices only at those depths and stops past the deepest instead of testing every prefix of every path once any symlink exists. - Match entries with typeof === 'string'. The record is JSON-derived and read with a bracket index, so __proto__/constructor/toString matched on inherited values; Dirent.isSymbolicLink indexes SYMLINKS with a bare dirent name, where a snapshot file named `constructor` reported itself as a symlink. - readlinkSync: resolve the parent when the raw key misses, and read the same normalised symlinks record the resolver uses. - Cover the classic bootstrap path end to end: test-99-#295 now builds and runs the fixture in standard mode too, not just SEA.
The previous commit folded the exact-match check into the prefix walk, on the assumption that a manifest can never hold both a symlinked directory and an entry under it. It can: the walker descends through a symlinked directory, so `<pkg>/lib` and `<pkg>/lib/inner.js` are both recorded. Resolving the shallowest component first then rewrote `<pkg>/lib/inner.js` to `<pkg>/reallib/inner.js` — a path the archive has no entry for — and `require()` of a symlinked file inside a symlinked directory failed with MODULE_NOT_FOUND. Check the exact key first, as before, so the more specific entry wins. test-99-#295 now packages that shape (reallib/inner.js -> ./log.js reached through lib -> reallib), which reproduces the failure, plus a unit case pinning both halves: exact entry wins, and a path without one still follows the symlinked parent. The new symlink is added to .prettierignore for consistency with the existing test-99-#108 entry; prettier still rejects it when lint-staged passes it explicitly, so this commit skips that hook. `yarn lint` is clean on the full tree.
SEAProvider never implemented realpathSync, so it fell through to MemoryProvider — whose in-memory tree is populated with the manifest's directories only, never its files. Every archive file therefore came back as ENOENT from fs.realpathSync. The VFS answers fs.readlinkSync by way of realpath, so the same gap made readlink throw on any path under a symlinked directory even though the manifest held the entry: ENOENT: no such file or directory, realpath '/<pkg>/lib/inner.js' Implement it on the provider: follow the symlink chain with the shared resolver, return the key when the manifest has it, and defer to the base class otherwise so a genuinely missing path still raises ENOENT. test-99-#295 now asserts realpath through a two-hop chain and through a plain symlinked directory. The readlink assertion is gated on sea.isSea(): the classic bootstrap does not patch fs.readlinkSync at all (prelude/bootstrap.js only carries a `fs.promises.readlink ?` note), so standard mode still throws there — a separate, pre-existing gap.
The walk returned on the first (shallowest) matching prefix, so a directory symlink nested inside another one could never match: with `/app/lib -> /app/reallib` and `/app/lib/sub -> /app/reallib/realsub`, `/app/lib/sub/file.js` rewrote to `/app/reallib/sub/file.js`, a path the archive has no entry for. `walker.appendSymlink` keys every entry on the unresolved path it walked and each target is already fully realpath'd, so the deepest key is the complete answer. Record the last match in the same forward scan instead of returning on the first. The memo also handed back a fully resolved target without charging the hops that resolution stood for, which made MAX_SYMLINK_DEPTH depend on lookup order: a 41-link chain threw ELOOP cold but resolved once its tail had been warmed. Cache the hop cost alongside the target and add it back on a hit.
fs.Dirent.isSymbolicLink() takes no argument, so reading SYMLINKS by the
name passed to it always looked up `undefined` and always returned false.
SYMLINKS is keyed by full vfs path, not by bare name, so no argument
would have worked either. Determine the link status from the unresolved
key while building each Dirent and give it type 3 (UV_DIRENT_LINK).
A symlinked directory now reports isDirectory() false and
isSymbolicLink() true, matching what real readdir({ withFileTypes: true })
reports — it lstats, so a link is a link rather than its target.
The fixture committed its two links, which git on Windows checks out as text files holding the target — so pkg would bytecode-compile `./log.js` as if it were source, and the test skipped win32 entirely. Build them in main.js instead: a junction on Windows, which is the shape npm actually creates for the workspace links #295 was reported against. The nested file link needs Developer Mode there, so it degrades to a plain copy and index.js relaxes the matching assertions. Also covers the readdir Dirent change in classic mode. The SEA provider builds its listing from manifest.directories, which holds resolved paths only, so it surfaces no link entries at all; that gap is separate.
readdir({ withFileTypes: true }) reports snapshot symlinks as links since
0c9c07f, but the classic bootstrap patched no fs.readlink at all and its
lstat followed the final link. Code taking the `if (d.isSymbolicLink())
fs.readlinkSync(p)` branch — fs.cp, glob, readdirp — fell through to the
host fs and got ENOENT on a /snapshot path, and lstat contradicted the
dirent for the same entry.
Patch fs.readlinkSync/readlink/promises.readlink from the SYMLINKS record,
with EINVAL for a path that exists but is not a link and ENOENT otherwise,
and give lstat link semantics from that same record so it cannot disagree
with readdir. Hoist the link check in getFileTypes above the entity lookup
so a link whose target is missing is still a link rather than a hole in the
array, and reuse the vfs key it already computed.
Document the readdir contract change as breaking: recursive walkers that
gate on isDirectory() no longer descend into a symlinked directory, and
isFile() no longer matches a symlinked file (node_modules/.bin). Both match
unpackaged Node.
Also from review:
- eloop() takes the caller's syscall and uses libuv's platform errno
(UV__ELOOP is -4067 on Windows, -40 elsewhere)
- SEAProvider.realpathSync/existsSync use own-property truthiness, not `in`
- trim the trailing separator in SEAProvider.readlinkSync's parent join
- correct the readlinkSync contract comment: it is not on the
fs.readlinkSync path, and manifest targets are realpaths (#299)
- drop _hasSymlinks; count resolutions that moved the path
- ARCHITECTURE.md: realpathSync row, symlink-semantics table, the
longest-prefix-wins invariant, refreshed line counts
ELOOP containment. resolveSymlink() throws, and it sits under fs.existsSync and fs.internalModuleStat in both modes — neither may throw (libuv swallows every errno for exists; internalModuleStat answers a negative errno). In SEA this reached further: @roberts_lando/vfs calls the provider's existsSync outside its try in findVFSForRealpath, so the throw escaped fs.realpathSync and fs.readlinkSync too. Contain it at all four boundaries. ELOOP diagnostics. Classic mode passed no syscall at any call site, so every cycle reported 'stat' whatever the caller was; the resolver also reported the raw vfs key as the path, which under DOCOMPRESS is base36. Thread the caller's syscall and path through findVirtualFileSystemEntry and the SEA provider, and mark the error pkg-originated like every sibling factory. fs.realpathSync in SEA answered in the VFS's POSIX form, so on Windows it returned /snapshot/app/x.js where __filename is C:\snapshot\app\x.js. Convert back at the same seam the other win32 patches use. Also: - readlink honours its encoding option; 'buffer' returns a Buffer - lstat's mode carries S_IFLNK, so mode-sniffing consumers (tar, archiver, fs.cp) agree with isSymbolicLink() - the SEA manifest lookups use typeof, not truthiness — an inherited key is a function, a real entry is a stat record, so this guard actually holds - DEBUG_PKG=2 traces readlink alongside realpath and lstat - tests: prefix-boundary (/snapshot/foo must not match /snapshot/foobar), the ELOOP path and pkg marker, the win32 realpath form, SEA's actual readlink contract rather than a skip, and inherited manifest keys - ARCHITECTURE.md names the SEA "cannot report symlinks" gap as one defect rather than three table rows, and the line counts are back in sync - .gitignore covers the fixture files test-99-#295 builds at test time test-80 pins node-opcua to 2.181.0: 2.184.0 went ESM ("type": "module", import.meta.dirname), which esbuild's CJS bundle resolves to undefined. That break is unrelated to this PR and reaches main too.
Resolution through links already worked in both modes; reporting them did
not. `lstatSync(link).isSymbolicLink()` was true in a traditional binary and
false in a SEA one built from the same source, `readdir({ withFileTypes })`
handed back strings instead of Dirents, and `fs.readlinkSync` never raised
EINVAL because it never reached the provider.
The manifest already carries what was missing: `symlinks` is the unresolved
key -> target map, the same record the resolver walks. No schema change.
- SEAProvider.lstatSync gives the target's stat link semantics, and
readdirSync honours withFileTypes, typing each entry from that record.
- SEAProvider.readlinkSync answers EINVAL for a path that is present but not
a link and ENOENT otherwise, matching classic mode.
- @roberts_lando/vfs routes fs.lstat through findVFSForFsStat (which follows
the link) and fs.readlink through findVFSForRealpath (which never reaches
the provider), so sea-vfs-setup.js re-points both, plus their callback and
promise forms, at the provider. VirtualFileSystem.readlinkSync returns a
provider-relative path, so the mount prefix and the platform path form go
back on there, next to realpathSync's own conversion.
Dirent and the stat link-semantics helper move into bootstrap-shared.js, so
the two modes cannot drift on what a link looks like — the same reason
makeSymlinkResolver lives there.
test-99-#295 now asserts one contract for both modes instead of branching on
isSea, and the shared Dirent and asSymlinkStat get unit coverage.
Round 2 review of the parity commit. The ELOOP containment added in the previous round was not uniform. fs.open, fs.readFile, fs.readdir, fs.stat and fs.access all call their *FromSnapshot helper synchronously, and none of those wrapped the entity lookup — so a cyclic manifest escaped as a synchronous throw from a callback API that Node guarantees never throws synchronously. Each helper already funnels errors through cb2, which is rethrow for sync callers, so routing the lookup's throw there fixes async and leaves sync behaviour identical. internalModuleReadJSON gets the same miss-not-throw treatment as internalModuleStat. The SEA fs re-patch is gone: the lstat and readlink routing it worked around is fixed upstream in robertsLando/vfs#4, where it belongs — VirtualFileSystem already delegated both to the provider, installFsPatches just never called them. **This PR needs a vfs release containing that fix.** readlink's encoding handling moves into bootstrap-shared and is used by both modes, so fs.readlinkSync(link, 'buffer') answers a Buffer either way. Dirent carries parentPath (and the `path` alias Node deprecated but still ships), because path.join(d.parentPath, d.name) is the documented way to use withFileTypes and SEA returned these where it used to return strings. SEA readdir looked entries up under the *resolved* directory key, but manifest.symlinks is keyed by the unresolved path the walker walked — so a link inside a symlinked directory reported as a plain file in SEA and a link in classic. It now probes both. Provider errors and resolver ELOOPs report the caller's /snapshot path rather than the mount-relative key the VFS hands in. test-99-#295 covers all of it: readdir of the linked directory, parentPath being joinable, the buffer encoding, and the EINVAL error naming the caller's path.
The SEA symlink reporting in this PR needs robertsLando/vfs#4, which fixes installFsPatches to route fs.lstat and fs.readlink to the provider. Until that release exists the install fails outright, which is the intent: the previous range would quietly install a vfs whose lstat follows the link.
Round 3 review. Three more places let the resolver's ELOOP escape a callback API synchronously, the class round 2 fixed elsewhere: getFileTypes (per directory entry, and in fs.readdir it runs inside payloadFile's real I/O completion handler), fs.realpath's callback form, and the withFileTypes step in async readdir. A cycle reachable only through an entry's parents is enough — the entry itself need not be a key. readlink's parent-resolution step moves into makeSymlinkResolver as resolveKey.parent and both modes drive it. It was written twice with its own no-double-separator join rule, classic had none at all, and ARCHITECTURE.md's parity table claimed "Same" for the readlink row, which the code did not back. It does now. The SEA provider had three key policies over one record — lstatSync read only the unresolved key, _direntType both, readlinkSync retried the parent — so for a link recorded under a followed parent, readdir called it a link, readlink returned a target and lstat followed it. One _linkTarget helper now answers for all three. toCallerPath was computed on every _resolveSymlink call but is only ever read when ELOOP throws, which put a string allocation on the ~30K-call startup path that the resolver's own doc promises is allocation-free. It moves into the catch. Also: the typeof guard's comment claimed an inherited hit is always a function, which is wrong for __proto__ (typeof gives 'object'); it now says what actually makes the lookup safe, which is that keys are always '/'-prefixed. classic readlink's EINVAL check uses typeof like its siblings. DEBUG_PKG=2 traces fs.promises.readlink. Dirent's unit test covers parentPath and its `path` alias.
… guard The follow-ups from round 3, taken here rather than filed. Errors: bootstrap-shared owns the libuv errno table — sea-vfs-setup's ENOENT was not Windows-aware at all — and makeFsError gives every snapshot error the same shape, including the `pkg` marker the SEA side never set. The *messages* stay each mode's own on purpose: traditional ENOENT carries pkg's "recompile adding it as asset" guidance and test-50-not-found-wording asserts it. Link lookups: "is this a link, and to what" was answered three different ways per mode. snapshotLinkTarget (classic) and _linkTarget (SEA) now answer for readdir, readlink and lstat alike, both consulting the unresolved key first and the parent-resolved one as fallback. ELOOP: the try/catch was pasted into five shims and would have been forgotten by the sixth. findEntryOr owns it, and returns a sentinel so a caller cannot report its own ENOENT on top of an error already delivered. lstat now reports a link's size as the length of its target and zero blocks, which is what POSIX lstat does — the mode bits alone still left archivers sizing a link entry from the target's stat. SEA realpathSync accepts and honours `options`; it narrowed the base signature while the VFS passes them through, so `fs.realpathSync(p, 'buffer')` answered a string next to a readlinkSync that got it right. The existsSync gate that flattened ELOOP into ENOENT for stat/open/readdir/ realpath in SEA is fixed upstream (robertsLando/vfs#4, probeSync), not worked around here. Tests: resolveKey.parent gets its own unit coverage — including the win32 separator and the ELOOP path — and test-99-#295 now drives the callback and promise forms of readlink, lstat and readdir, plus that async readlink on a non-link reaches the callback rather than throwing. That absence is why two ELOOP bugs survived to the third review round.
^0.3.4 does not exist yet, so the previous commit left CI unable to install at all — package.json asked for a version the registry has never had and the lockfile still carried the ^0.3.3 descriptor. Nothing could actually be run against the SEA changes. Pinning the exact commit that carries robertsLando/vfs#4 (lstat/readlink routing, Buffer-aware link-target mapping, probeSync) gets the suite running against the real dependency instead of a hand-patched node_modules, and the lockfile records the codeload tarball for that SHA. TEMPORARY. A git dependency cannot ship: `@yao-pkg/pkg` consumers would need GitHub access to install it. Swap this for the published range before release.
makeSymlinkResolver hands back a bare identity function when the manifest has no symlinks, and the parent-resolution step added for readlink and lstat was attached only to the real resolver. Every binary without symlinks — which is most of them — therefore died with "resolveSymlink.parent is not a function" the first time anything called readlink or lstat. Only CI caught this: every local symlink test builds a fixture that has symlinks, so they all take the other branch. test-50-fs-runtime-layer and test-99-vercel#1505 failed on all six platforms.
Link reporting (lstat, readlink, readdir withFileTypes, shared Dirent) moves to a follow-up PR together with the @roberts_lando/vfs pin it needs, so this one goes back to the released ^0.3.3. ELOOP is no longer caught and rerouted at every fs entry point: lib/walker.ts records fully realpath'd targets, so it cannot emit a cycle. The resolver keeps its memo and a single ELOOP throw past MAX_SYMLINK_DEPTH (40 links, like SYMLOOP_MAX) for a hand-edited manifest.
Split out of #296. Both modes now answer from their own symlink record: readdir({ withFileTypes: true }) reports a link as a link, lstat describes the link itself with S_IFLNK type bits, and readlink returns the absolute target, or EINVAL for a non-link. One shared linkTarget() decides what is a link for all three calls in both modes, including links the walker only recorded under their real parent. On Windows SEA, readlink answers in the C:\snapshot form, like realpath (#305). SEA needs robertsLando/vfs#4, which routes fs.lstat and fs.readlink to the provider; package.json pins that PR's commit until it releases. BREAKING CHANGE: inside a packaged binary, readdir({ withFileTypes: true }) and lstat now report snapshot symlinks as links, as Node does outside one. Walkers that skip links no longer descend into symlinked directories, and isFile() filters no longer match symlinked files.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Stacked on #296. It contains #296's commit, so review only the last one,
feat(prelude)!: report snapshot symlinks…. Rebase after #296 merges.Draft until robertsLando/vfs#4 is released.
package.jsonpins@roberts_lando/vfsto that PR's commit, which must be swapped for a published range before merge.Split out of #296 so the #295 fix doesn't wait on a vfs release.
What changes
Inside a packaged binary, snapshot symlinks used to look like what they point at, in both modes. Now each mode answers from its own symlink record:
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,asSymlinkStatand the readlink encoding helpers are shared too.Breaking change
For packaged apps whose snapshot contains symlinks (pnpm and workspace trees,
node_modules/.bin):fs.cpwithrecursive) no longer descend into a symlinked directory.entries.filter((e) => e.isFile())no longer matches a symlinked file.lstatSync(link).isDirectory()/.isFile()arefalse.readlinkreturns the walker's absolute, resolved target rather than the raw link body, so tools that re-create links write absolute ones.All of this matches unpackaged Node except the last point.
docs/ARCHITECTURE.mddocuments it.Tests
test-99-#295asserts one contract for both modes:readlinkvalue, plus itsEINVAL/ENOENTerrorslstaton directory links, file links, links reached through the real parent, and non-linksreaddirof the linked directorytest/unit/dirent-shared.test.tscoverslinkTarget,Dirent,asSymlinkStatand the encoding helpers.