Address review comments

Author: mpotthoff

prelude/bootstrap-shared.js

@@ -622,6 +622,75 @@ function installDiagnostic(snapshotPrefix) {
   }
 }
 
+// /////////////////////////////////////////////////////////////////
+// SYMLINK PROCESSING //////////////////////////////////////////////
+// /////////////////////////////////////////////////////////////////
+
+// Matches the typical Linux SYMLOOP_MAX. Bounds the symlink resolution
+// loop so a manifest cycle (or a corrupt manifest) cannot hang startup.
+var MAX_SYMLINK_DEPTH = 40;
+
+function resolveSymlink(p, sep, symlinks, cache) {
+  // Cache symlink resolution results to avoid re-walking the same path.
+  // The cache is keyed by the original path, not the resolved path, so that
+  // repeated calls with the same input path hit the cache. Only paths that
+  // actually traverse a symlink get cached (see below) — the vast majority
+  // of lookups are non-symlinked files, and most of those are looked up
+  // once (module resolution tries many one-off candidate paths), so
+  // memoizing them would grow the cache unboundedly for no benefit and add
+  // Map overhead to every miss without amortizing it. Bounding the cache to
+  // real hits keeps it both fast and small.
+  var cached = cache.get(p);
+  if (cached !== undefined) return cached;
+
+  var original = p;
+  var matched = false;
+  for (var i = 0; i < MAX_SYMLINK_DEPTH; i++) {
+    // Exact match first (e.g. the path itself is the symlink).
+    var target = symlinks[p];
+    if (!target) {
+      // Walk the path front-to-back (POSIX-style): resolve the shallowest
+      // symlinked component first. This is O(path depth) hash lookups,
+      // independent of how many symlinks exist in the manifest. Symlinks
+      // (e.g. a package manager's node_modules entries) sit near the root
+      // while the remainder of the path can be arbitrarily deep, so this
+      // finds a hit in far fewer lookups than scanning from the leaf
+      // backwards would.
+      var pos = p.indexOf(sep, 1);
+      while (pos > 0) {
+        var prefix = p.slice(0, pos);
+        var t = symlinks[prefix];
+        if (t) {
+          // If the symlink target ends with a separator, we need to skip
+          // the leading separator of the remainder to avoid a double
+          // separator. Otherwise, we can just append the remainder as-is.
+          target = t.endsWith(sep) ? t + p.slice(pos + 1) : t + p.slice(pos);
+          break;
+        }
+        pos = p.indexOf(sep, pos + 1);
+      }
+    }
+
+    if (!target) {
+      // No symlink found in the path, so the current path is fully resolved.
+      if (matched) cache.set(original, p);
+      return p;
+    }
+
+    matched = true;
+    p = target;
+  }
+
+  var err = new Error(
+    "ELOOP: too many symbolic links encountered, '" + original + "'",
+  );
+  err.code = 'ELOOP';
+  err.errno = -40;
+  err.syscall = 'stat';
+  err.path = original;
+  throw err;
+}
+
 module.exports = {
   patchDlopen: patchDlopen,
   patchChildProcess: patchChildProcess,
@@ -631,4 +700,5 @@ module.exports = {
   COMPRESS_NONE: COMPRESS_NONE,
   pickDecompressorSync: pickDecompressorSync,
   pickDecompressorAsync: pickDecompressorAsync,
+  resolveSymlink: resolveSymlink,
 };

prelude/bootstrap.js

@@ -231,25 +231,16 @@ function toOriginal(fShort) {
     .join(path.sep);
 }
 
-const symlinksEntries = Object.entries(SYMLINKS);
+const hasSymlinks = Object.keys(SYMLINKS).length > 0;
+const symlinkCache = new Map();
 
 // separator for substitution depends on platform;
 const sepsep = DOCOMPRESS ? separator : path.sep;
 
 function findVirtualFileSystemKeyAndFollowLinks(path_) {
   let vfsKey = findVirtualFileSystemKey(path_, path.sep);
-  let needToSubstitute = true;
-  while (needToSubstitute) {
-    needToSubstitute = false;
-    for (const [k, v] of symlinksEntries) {
-      if (vfsKey.startsWith(`${k}${sepsep}`) || vfsKey === k) {
-        vfsKey = vfsKey.replace(k, v);
-        needToSubstitute = true;
-        break;
-      }
-    }
-  }
-  return vfsKey;
+  if (!hasSymlinks) return vfsKey;
+  return REQUIRE_SHARED.resolveSymlink(vfsKey, sepsep, SYMLINKS, symlinkCache);
 }
 
 function realpathFromSnapshot(path_) {

prelude/sea-vfs-setup.js

@@ -24,10 +24,6 @@ try {
 var VirtualFileSystem = vfsModule.VirtualFileSystem;
 var MemoryProvider = vfsModule.MemoryProvider;
 
-// Matches the typical Linux SYMLOOP_MAX. Bounds the symlink resolution
-// loop so a manifest cycle (or a corrupt manifest) cannot hang startup.
-var MAX_SYMLINK_DEPTH = 40;
-
 // /////////////////////////////////////////////////////////////////
 // PERFORMANCE INSTRUMENTATION /////////////////////////////////////
 // /////////////////////////////////////////////////////////////////
@@ -151,6 +147,7 @@ var perf = {
       'statSync calls',
       'existsSync calls',
       'readdirSync calls',
+      '_resolveSymlink calls',
     ];
     counterOrder.forEach(function (label) {
       var v = self._counters[label];
@@ -283,13 +280,21 @@ function _makeStats(meta) {
  *
  * Performance design:
  *
- *   - internalModuleStat()  O(1) manifest hash lookup (no tree walk).
+ *   - internalModuleStat()  Symlink resolution (no-op O(1) if the manifest has
+ *     no symlinks; O(path depth) for any path that isn't itself symlinked,
+ *     paid on every call — not memoised, since most lookups are one-off
+ *     candidate paths and caching them would grow the cache unboundedly for
+ *     no benefit; O(1) amortized only for paths that actually traverse a
+ *     symlink, via a Map keyed by the original path — see resolveSymlink()
+ *     in bootstrap-shared.js) + O(1) manifest lookup.
  *     This is the hottest path (~30K calls for large projects).
  *
- *   - statSync()            O(1) manifest lookup + lightweight stat allocation.
+ *   - statSync()            Same symlink resolution as above + O(1) manifest
+ *     lookup + lightweight stat allocation.
  *     Not on the module resolution hot path.  Returns a fresh object each call.
  *
- *   - existsSync()          O(1) manifest lookup.
+ *   - existsSync()          Same symlink resolution as above + O(1) manifest
+ *     lookup.
  *
  *   - readFileSync()        Zero-copy subarray from the archive with a Map
  *     cache.  Bypasses the MemoryProvider tree entirely.  Returns a Buffer
@@ -307,9 +312,8 @@ class SEAProvider extends MemoryProvider {
     this._manifest = seaManifest;
     this._fileCache = new Map();
 
-    // Precompute whether the manifest has any symlinks.
-    // If a project has no symlinks, there is also no need to resolve them.
-    this._hasSymlinks = Object.keys(seaManifest.symlinks).length > 0;
+    this._hasSymlinks = Object.keys(seaManifest.symlinks || {}).length > 0;
+    this._symlinkCache = new Map();
 
     // Pick the per-file decompressor once at construction time.  Absent or 0 =
     // uncompressed archive (backward compat with pre-#250 SEA binaries).  The
@@ -341,37 +345,14 @@ class SEAProvider extends MemoryProvider {
   }
 
   _resolveSymlink(p) {
-    // Fast path: if the manifest has no symlinks, skip the loop entirely.
+    perf.count('_resolveSymlink calls');
     if (!this._hasSymlinks) return p;
-    var symlinks = this._manifest.symlinks;
-    var original = p;
-    for (var i = 0; i < MAX_SYMLINK_DEPTH; i++) {
-      // First check the full path, then walk up the directory tree to find a symlink.
-      var target = symlinks[p];
-      if (!target) {
-        var parentIdx = p.lastIndexOf('/');
-        while (parentIdx > 0) {
-          var parent = p.slice(0, parentIdx);
-          target = symlinks[parent];
-          if (target) {
-            // Resolve the symlink and append the remainder of the original path.
-            target = target + p.slice(parentIdx);
-            break;
-          }
-          parentIdx = parent.lastIndexOf('/');
-        }
-      }
-      if (!target) return p;
-      p = target;
-    }
-    var err = new Error(
-      "ELOOP: too many symbolic links encountered, '" + original + "'",
+    return shared.resolveSymlink(
+      p,
+      '/',
+      this._manifest.symlinks,
+      this._symlinkCache,
     );
-    err.code = 'ELOOP';
-    err.errno = -40;
-    err.syscall = 'stat';
-    err.path = original;
-    throw err;
   }
 
   get fileCacheSize() {
@@ -459,6 +440,10 @@ class SEAProvider extends MemoryProvider {
   }
 
   readlinkSync(filePath) {
+    // readlinkSync must return the symlink target verbatim, without resolving
+    // it. If the path is not a symlink, fall back to the super method (which throws
+    // ENOENT for non-existent paths).  The manifest's symlinks map is keyed by
+    // the symlink path and contains the target path, so we can look it up directly.
     var p = toManifestKey(filePath);
     var target = this._manifest.symlinks[p];
     if (target) return target;

test/test-99-#295/package.json

@@ -1,5 +1,5 @@
 {
-  "name": "test-99-#295",
+  "name": "test-99-295",
   "version": "1.0.0",
   "main": "index.js",
   "bin": "index.js"

test/test.js

@@ -85,6 +85,7 @@ const npmTests = [
   'test-91-sea-esm-entry',
   'test-92-sea-tla',
   'test-94-sea-esm-import-meta',
+  'test-99-#295',
 ];
 
 if (testFilter) {