perf(nodejs): derive a monorepo function's dependency closure from the lockfile

aws_lambda_builders/workflows/nodejs_npm/actions.py

@@ -10,6 +10,7 @@
 from aws_lambda_builders import utils
 from aws_lambda_builders.actions import ActionFailedError, BaseAction, Purpose
 from aws_lambda_builders.utils import extract_tarfile
+from aws_lambda_builders.workflows.nodejs_npm.lockfile_closure import production_closure
 from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError, SubprocessNpm
 
 LOG = logging.getLogger(__name__)
@@ -421,7 +422,11 @@ def __init__(self, install_dir, project_root, artifacts_dir, subprocess_npm, osu
         self._osutils = osutils
 
     def execute(self):
-        closure = self._subprocess_npm.resolve_dependency_closure(self._install_dir)
+        # npm's hidden lockfile records the tree the install just reified, so read that rather than
+        # paying an npm process per function; it answers None whenever it cannot be trusted
+        closure = production_closure(self._project_root, self._install_dir)
+        if closure is None:
+            closure = self._subprocess_npm.resolve_dependency_closure(self._install_dir)
         destination = os.path.join(self._artifacts_dir, "node_modules")
 
         if closure is None:
@@ -562,8 +567,9 @@ def _outermost_packages(self, closure):
         is - hoisting it would shadow the top-level version for every other caller - and it is already
         reachable through the dependency that contains it, so only the outermost paths are linked.
         """
-        # Compare through normcase, link the original path: npm's spelling and the build's can differ in
-        # case on Windows and still name one directory. See aws/aws-lambda-builders#935.
+        # Compare through normcase, link the original path: an `npm ls` closure carries npm's spelling
+        # while project_root and install_dir carry the build's, and on Windows those can differ in case
+        # and still name one directory. See aws/aws-lambda-builders#935.
         paths = [os.path.realpath(path) for path in closure]
         excluded = {os.path.normcase(os.path.realpath(d)) for d in (self._project_root, self._install_dir)}
         candidates = [path for path in paths if os.path.normcase(path) not in excluded]

aws_lambda_builders/workflows/nodejs_npm/lockfile_closure.py

@@ -0,0 +1,165 @@
+"""
+Derive a workspace member's production dependency closure from a lockfile, with no npm subprocess.
+
+Replaces `npm ls --all --parseable --omit=dev` for the #933 artifact-narrowing step. Only
+lockfileVersion 2 and 3 carry the `packages` map this needs, so callers fall back to asking npm when
+this answers None.
+
+Read npm's HIDDEN lockfile, `<project_root>/node_modules/.package-lock.json`, never the project's own
+`package-lock.json`: npm writes the hidden one to describe the tree it actually reified, and the
+in-source install runs `npm install --no-save`, which leaves the project lockfile untouched. A manifest
+listing a dependency the project lockfile has not recorded yet therefore installs the package while the
+project lockfile still denies it exists - and a closure read from that file drops it from the artifacts.
+See aws/aws-lambda-builders#938.
+
+Returns the same thing npm's --parseable output gives: absolute directories, real paths, one per
+resolved package -- including workspace dependencies reported as their own source directory.
+"""
+
+import json
+import logging
+import os
+from typing import Dict, List, Optional, Set
+
+LOG = logging.getLogger(__name__)
+
+# lockfileVersion 1 predates the per-path `packages` map this resolver reads
+MINIMUM_LOCKFILE_VERSION = 2
+
+# npm's record of the tree it reified, written on every install from npm 7 on
+HIDDEN_LOCKFILE = os.path.join("node_modules", ".package-lock.json")
+
+
+def hidden_lockfile_path(project_root: str) -> str:
+    """Where npm records the tree it installed under `project_root`."""
+    return os.path.join(project_root, HIDDEN_LOCKFILE)
+
+
+def _describes_the_installed_tree(project_root: str) -> bool:
+    """
+    Is npm's hidden lockfile still a true record of `node_modules`?
+
+    npm's own test, from its `package-lock.json` docs: the hidden lockfile counts only while its mtime
+    is at least as recent as the tree it describes, because "if another CLI mutates the tree in any
+    way, this will be detected, and the hidden lockfile will be ignored". Reading it without that test
+    would trust a file npm itself would discard.
+
+    `node_modules` alone, not every package folder: its mtime moves whenever a top-level entry is added
+    or removed, which is what another package manager rewriting the tree does, and one `stat` keeps this
+    cheaper than the `npm ls` it exists to avoid. `>=`, not `>`: a fresh install writes the two in the
+    same clock tick, so `>` would send every first build down the fallback.
+    """
+    try:
+        hidden = os.path.getmtime(hidden_lockfile_path(project_root))
+        tree = os.path.getmtime(os.path.join(project_root, "node_modules"))
+    except OSError:
+        return False
+    return hidden >= tree
+
+
+def _resolve_key(packages: Dict, from_key: str, name: str) -> Optional[str]:
+    """
+    Find the packages-map key that `name` resolves to when required from `from_key`.
+
+    Mirrors node's resolution: walk up the directory chain looking for node_modules/<name>, which in
+    the lockfile is spelled as a key. `from_key` is "" for the root, "endpoints/orders" for a
+    workspace member, or "node_modules/express" for an installed package.
+    """
+    prefix = from_key
+    while True:
+        candidate = f"{prefix}/node_modules/{name}" if prefix else f"node_modules/{name}"
+        if candidate in packages:
+            return candidate
+        if not prefix:
+            return None
+        # strip one path segment and try again, which is what walking up node_modules means
+        prefix = prefix.rsplit("/", 1)[0] if "/" in prefix else ""
+        if prefix.endswith("/node_modules"):
+            prefix = prefix[: -len("/node_modules")]
+
+
+def _entry_dir(project_root: str, key: str, entry: Dict) -> str:
+    """The directory a packages-map entry lives in."""
+    # a workspace link points at the real source directory rather than the link's own path
+    if entry.get("link") and entry.get("resolved"):
+        return os.path.join(project_root, *entry["resolved"].split("/"))
+    return os.path.join(project_root, *key.split("/")) if key else project_root
+
+
+def production_closure(project_root: str, install_dir: str) -> Optional[List[str]]:
+    """
+    The production dependency closure of `install_dir`, as absolute directories.
+
+    Returns None whenever npm's hidden lockfile cannot answer -- it is absent (npm 6, or a tree another
+    package manager built), stale against `node_modules`, an unsupported version, unreadable, or does
+    not describe this install directory -- so the caller falls back to asking npm.
+    """
+    if not _describes_the_installed_tree(project_root):
+        LOG.debug("NODEJS no current hidden lockfile under %s; asking npm instead", project_root)
+        return None
+
+    try:
+        with open(hidden_lockfile_path(project_root)) as handle:
+            lock = json.load(handle)
+    except (OSError, ValueError):
+        return None
+
+    if lock.get("lockfileVersion", 1) < MINIMUM_LOCKFILE_VERSION or "packages" not in lock:
+        return None
+
+    packages = lock["packages"]
+    project_root = os.path.realpath(project_root)
+    rel = os.path.relpath(os.path.realpath(install_dir), project_root).replace(os.sep, "/")
+    start = "" if rel == "." else rel
+    if start not in packages:
+        return None
+
+    # a link entry for a workspace member carries no dependencies; its source entry does
+    def deps_of(key: str) -> Dict[str, str]:
+        entry = packages.get(key) or {}
+        if entry.get("link") and entry.get("resolved"):
+            entry = packages.get(entry["resolved"], entry)
+        # Production only, never devDependencies. Peers count: npm 7+ installs them and `npm ls --all`
+        # walks them, so omitting them drops a package the function resolves - a plugin's host package,
+        # typically. An optional peer the install declined is removed by the on-disk check below.
+        out = dict(entry.get("dependencies") or {})
+        out.update(entry.get("optionalDependencies") or {})
+        out.update(entry.get("peerDependencies") or {})
+        return out
+
+    found: Set[str] = set()
+    stack = [start]
+    while stack:
+        key = stack.pop()
+        for name in deps_of(key):
+            resolved = _resolve_key(packages, key, name)
+            if resolved is None or resolved in found:
+                continue
+            entry = packages[resolved]
+            if entry.get("dev"):
+                continue
+            found.add(resolved)
+            # a link's dependencies must be resolved from its source directory, not from the
+            # link's own node_modules path: walking up from the link finds the hoisted copy of
+            # a package the source directory nests (node resolves through the real path too)
+            if entry.get("link") and entry.get("resolved"):
+                stack.append(entry["resolved"])
+            else:
+                stack.append(resolved)
+
+    # An entry can be in the lockfile without being on disk: an optional dependency skipped for this
+    # platform (fsevents is darwin-only), or one whose os/cpu/libc constraints excluded it. npm ls
+    # reports the installed tree, so match that and keep only what is actually there. A required
+    # dependency missing here means the install itself failed, which the install step surfaces.
+    dirs = set()
+    for key in found:
+        directory = _entry_dir(project_root, key, packages[key])
+        if os.path.isdir(directory):
+            dirs.add(directory)
+        else:
+            LOG.debug("NODEJS lockfile lists %s but it is not installed; leaving it out", key)
+    # npm --parseable also prints the project itself and the install dir; the caller filters those,
+    # so emit them for parity
+    dirs.add(project_root)
+    dirs.add(os.path.realpath(install_dir))
+    return sorted(dirs)

tests/integration/workflows/nodejs_npm/test_nodejs_npm.py

@@ -13,6 +13,7 @@
 from aws_lambda_builders.builder import LambdaBuilder
 from aws_lambda_builders.exceptions import WorkflowFailedError
 from aws_lambda_builders.supported_runtimes import NODEJS_RUNTIMES
+from aws_lambda_builders.workflows.nodejs_npm.lockfile_closure import production_closure
 from aws_lambda_builders.workflows.nodejs_npm.npm import SubprocessNpm
 from aws_lambda_builders.workflows.nodejs_npm.utils import EXPERIMENTAL_FLAG_NODEJS_MONOREPO, OSUtils
 from tests.testing_utils import read_link_without_junction_prefix
@@ -538,6 +539,60 @@ def test_build_in_source_in_workspaces_monorepo_links_the_hoisted_dependencies(s
         )
         self.assertEqual(require_handler.returncode, 0, require_handler.stderr)
 
+    def test_the_lockfile_closure_agrees_with_npm_ls_on_a_real_install(self):
+        # this is the property that lets the lockfile replace `npm ls`: against a real npm install, on a
+        # real workspaces monorepo, the two must name the same set of directories. A unit test cannot
+        # assert it - it would be comparing this code against a fixture written from this code.
+        monorepo_dir = os.path.join(self.temp_testdata_dir, "workspaces-monorepo")
+        npm = SubprocessNpm(OSUtils())
+        npm.run(["install", "--silent", "--no-audit", "--no-fund"], cwd=monorepo_dir)
+
+        for endpoint in ("fn", "other"):
+            install_dir = os.path.join(monorepo_dir, "endpoints", endpoint)
+            from_npm = npm.resolve_dependency_closure(install_dir)
+            self.assertIsNotNone(from_npm, "npm ls must answer, or this test proves nothing")
+            from_lockfile = production_closure(monorepo_dir, install_dir)
+            self.assertIsNotNone(from_lockfile, "npm's hidden lockfile must be readable by the resolver")
+
+            normalise = {os.path.normcase(os.path.realpath(p)) for p in from_npm}
+            self.assertEqual(
+                {os.path.normcase(os.path.realpath(p)) for p in from_lockfile},
+                normalise,
+                f"the two closure sources disagree for endpoints/{endpoint}",
+            )
+
+    def test_a_manifest_the_project_lockfile_has_not_recorded_still_reaches_the_artifacts(self):
+        # `npm install --no-save` installs a newly declared dependency without writing package-lock.json,
+        # so a closure read from THAT file drops the package and the function fails with Cannot find
+        # module. npm's hidden lockfile records the tree it reified, which is why the resolver reads it.
+        monorepo_dir = os.path.join(self.temp_testdata_dir, "workspaces-monorepo")
+        source_dir = os.path.join(monorepo_dir, "endpoints", "fn")
+        npm = SubprocessNpm(OSUtils())
+        npm.run(["install", "--silent", "--no-audit", "--no-fund"], cwd=monorepo_dir)
+
+        # declare a dependency the lockfile knows nothing about, exactly as editing package.json does
+        manifest_path = os.path.join(source_dir, "package.json")
+        with open(manifest_path) as manifest:
+            manifest_json = json.load(manifest)
+        manifest_json["dependencies"]["ms"] = "^2.1.3"
+        with open(manifest_path, "w") as manifest:
+            json.dump(manifest_json, manifest)
+        with open(os.path.join(monorepo_dir, "package-lock.json")) as lockfile:
+            self.assertNotIn("ms", json.load(lockfile)["packages"]["endpoints/fn"].get("dependencies", {}))
+
+        self.builder.build(
+            source_dir,
+            self.artifacts_dir,
+            self.scratch_dir,
+            manifest_path,
+            # one runtime: this pins the closure source, which no runtime varies
+            runtime=NODEJS_RUNTIMES[-1],
+            build_in_source=True,
+            experimental_flags=[EXPERIMENTAL_FLAG_NODEJS_MONOREPO],
+        )
+
+        self.assertIn("ms", set(os.listdir(os.path.join(self.artifacts_dir, "node_modules"))))
+
     @parameterized.expand(SUPPORTED_RUNTIMES)
     def test_build_in_source_with_removed_dependencies(self, runtime):
         # run a build with default requirements and confirm dependencies are downloaded

tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/package.json

@@ -3,5 +3,8 @@
   "version": "1.0.0",
   "license": "APACHE2.0",
   "main": "index.js",
-  "files": ["index.js"]
+  "files": ["index.js"],
+  "peerDependencies": {
+    "fake-http-request": "^1.4.0"
+  }
 }

tests/unit/workflows/nodejs_npm/test_actions.py

@@ -18,6 +18,7 @@
     NodejsNpmCIAction,
     NodejsNpmTestAction,
 )
+from aws_lambda_builders.workflows.nodejs_npm.lockfile_closure import hidden_lockfile_path
 from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError
 from aws_lambda_builders.workflows.nodejs_npm.utils import OSUtils
 
@@ -312,6 +313,17 @@ def _action(self):
             osutils=self.osutils,
         )
 
+    def _hidden_lockfile(self, version, packages):
+        # npm's hidden lockfile is the only one the action reads, and it must not be older than the
+        # tree it describes
+        path = hidden_lockfile_path(self.root)
+        os.makedirs(os.path.dirname(path), exist_ok=True)
+        with open(path, "w") as handle:
+            json.dump({"lockfileVersion": version, "packages": packages}, handle)
+        tree = os.path.getmtime(os.path.join(self.root, "node_modules"))
+        os.utime(path, (tree + 1, tree + 1))
+        return path
+
     def _linked(self):
         node_modules = os.path.join(self.artifacts_dir, "node_modules")
         found = set()
@@ -322,6 +334,47 @@ def _linked(self):
                 found.add(entry)
         return found
 
+    def test_a_usable_hidden_lockfile_answers_the_closure_without_running_npm(self):
+        # the whole point of reading the lockfile: no npm process per function
+        self._package("endpoints/fn", "@mono/fn")
+        self._package("node_modules/lodash", "lodash")
+        self._hidden_lockfile(
+            3,
+            {
+                "": {"workspaces": ["endpoints/fn"]},
+                "endpoints/fn": {"dependencies": {"lodash": "^4.0.0"}},
+                "node_modules/lodash": {"version": "4.17.21"},
+            },
+        )
+
+        self._action().execute()
+
+        self.assertEqual(self._linked(), {"lodash"})
+        self.subprocess_npm.resolve_dependency_closure.assert_not_called()
+
+    def test_a_hidden_lockfile_that_cannot_answer_falls_back_to_npm(self):
+        # version 1 carries no per-path map, so the fallback is the only thing that can produce a closure
+        self._package("endpoints/fn", "@mono/fn")
+        mine = self._package("node_modules/lodash", "lodash")
+        self._hidden_lockfile(1, {})
+        self.subprocess_npm.resolve_dependency_closure.return_value = [self.root, self.install_dir, mine]
+
+        self._action().execute()
+
+        self.assertEqual(self._linked(), {"lodash"})
+        self.subprocess_npm.resolve_dependency_closure.assert_called_once_with(self.install_dir)
+
+    def test_no_hidden_lockfile_falls_back_to_npm(self):
+        # the pre-npm-7 and other-package-manager case: every existing closure test runs this path
+        self._package("endpoints/fn", "@mono/fn")
+        mine = self._package("node_modules/lodash", "lodash")
+        self.subprocess_npm.resolve_dependency_closure.return_value = [self.root, self.install_dir, mine]
+
+        self._action().execute()
+
+        self.assertEqual(self._linked(), {"lodash"})
+        self.subprocess_npm.resolve_dependency_closure.assert_called_once_with(self.install_dir)
+
     def test_links_only_this_function_s_dependencies(self):
         self._package("endpoints/fn", "@mono/fn")
         mine = self._package("node_modules/lodash", "lodash")