fix(sea): files inside symlinks are not resolved correctly (#295) - #296
fix(sea): files inside symlinks are not resolved correctly (#295)#296mpotthoff wants to merge 2 commits into
Conversation
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #296 +/- ##
==========================================
- Coverage 87.19% 86.75% -0.45%
==========================================
Files 23 23
Lines 7929 7929
Branches 1214 1217 +3
==========================================
- Hits 6914 6879 -35
- Misses 1008 1042 +34
- Partials 7 8 +1 see 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
robertsLando
left a comment
There was a problem hiding this comment.
Review summary
Verdict: Ship with changes — one Major (perf) worth landing first.
The issue and the fix are both confirmed — verified end-to-end, not just read
The bug is real: lib/walker.ts:459 records exactly one manifest entry per link (this.symLinks[file] = realFile), so node_modules/@t/lib/package.json has no key, and the pre-fix _resolveSymlink was exact-key only. The non-SEA prelude already did prefix matching (prelude/bootstrap.js:239-252, vfsKey.startsWith(k + sep)) — SEA was the outlier, which is exactly why standard mode worked.
The fix is correct. Built at 843326b on node v22.20.0:
| build | result |
|---|---|
| committed test, with fix | exit 0 |
| committed test, without the sea-vfs-setup.js hunk | ENOENT ... '/test-99-#295/lib/log.js', exit 1 |
| the npm-workspace repro from #295 (node_modules/@t/lib -> ../../packages/lib, ESM bare import), SEA with fix | exit 0 |
| same repro, SEA without fix | ERR_MODULE_NOT_FOUND at resolveBareSpecifier → vfsResolveHook, exit 1 |
| same repro, standard (non-SEA) mode | exit 0 — confirms the report |
Replaying _resolveSymlink against synthetic manifests: the #295 shape, multi-segment remainders, and chained links all resolve correctly; cycles terminate at MAX_SYMLINK_DEPTH (i is never reset); parentIdx > 0 correctly refuses the empty root key; win32 C:/… keys walk correctly and stop at C:; and there is no substring/prefix trap (/a/bc does not match /a/bcd). Nice work — the parent walk is also strictly better than bootstrap's version (O(path depth) hash lookups instead of O(number of symlinks) scans).
Top 3 risks
- Measured ~60× slowdown of _resolveSymlink for any project that has symlinks — i.e. every project this PR fixes. See the inline comment on line 345.
- Two divergent VFS symlink implementations (SEA vs bootstrap.js) — the root-cause class of #295 itself. Inline on line 353.
- A latent ELOOP throw out of existsSync / internalModuleStat, which must not throw. Inline on line 365.
Findings outside the diff
- Major · Tests — test/test.js:60-87: test-99-#295 is a host-only SEA test (runSeaHostOnly, ignores the target arg) but wasn't added to the npmTests array. It therefore falls through the **/main.js glob and builds a SEA binary in both test:22 and test:24, each matrixed over 3 OSes, while never running in test:host — the exact redundancy the comment above npmTests exists to prevent. Adding 'test-99-#295' to that list fixes it. (test-93-sea-compress has the same omission — pre-existing, but worth folding in while you're there.)
- Minor · Design — prelude/sea-vfs-setup.js:286-292: the class JSDoc still advertises internalModuleStat() O(1) manifest hash lookup (no tree walk), statSync() O(1) and existsSync() O(1). All three are now O(path depth) with a directory walk whenever the manifest has symlinks. That doc block is what contributors read first, so it should either be restated or made true again by the memo above.
- Minor · Design — prelude/sea-vfs-setup.js:461-505: _resolveSymlink is now applied asymmetrically across the provider surface. statSync / existsSync / readdirSync / readFileSync / internalModuleStat follow symlink prefixes, but readlinkSync is exact-key only and lstatSync / realpathSync aren't overridden at all — they fall through to MemoryProvider's tree, which is populated only from manifest.directories. So a path that now stats fine has no corresponding realpath. Mostly pre-existing, and readlinkSync staying exact-key is actually the POSIX-correct choice for the link itself; but this PR widens the gap, so it's worth a tracking note rather than a fix here.
- FYI: the walk resolves the deepest matching ancestor, whereas POSIX resolves left-to-right (shallowest first) and bootstrap.js takes the first insertion-order match. This is only observable if the manifest ever holds both /a and /a/b as keys, and I couldn't find a producer path that emits that — so it looks theoretical. One comment line documenting the invariant would be enough.
- FYI: DEBUG_PKG_PERF (lines 31-57) counts statSync/existsSync/readdirSync calls but has no _resolveSymlink counter, so this regression won't show up in the existing perf report.
- FYI: dir naming, main.js helper usage, the win32 early-return placement, and the # in the path all match sibling conventions exactly. core.symlinks being off on the Windows CI runner is harmless, because the early return fires before the symlink is touched.
Coverage
Specialists run: Correctness, DRY & Codebase Fit, Performance, Tests, Design/API/BackCompat, plus an empirical build-and-run verifier. Not run: Security, Operability, Readability — no files in their lane (the symlink map is build-time output from the developer's own tree, not a trust boundary; no logging/error-path changes; no file over 80 changed lines).
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Fixes #295
This change does require us to always walk up the full path hierarchy to detect any parent symlinks, which will worsen the performance. I also had to remove the object-has-key fast path.
To at least keep the performance the same for projects that don't use any symlinks, I added a precomputed flag that determines whether any symlink exists. If there are no symlinks, we can immediately return out of the function.