perf(python): symlink layer dependencies instead of copying them

aws_lambda_builders/actions.py

@@ -132,17 +132,83 @@ def __init__(self, source_dir, dest_dir):
         self._dest_dir = dest_dir
 
     def execute(self):
+        # Match CopySourceAction, which this replaces for layers: a dependencies directory that was
+        # never created (download_dependencies=False against a missing cache) is skipped, not fatal.
+        if not os.path.isdir(self._source_dir):
+            LOG.warning("Skipping link operation since source %s does not exist", self._source_dir)
+            return
+
         source_files = set(os.listdir(self._source_dir))
 
         for source_file in source_files:
             source_path = Path(self._source_dir, source_file)
             destination_path = Path(self._dest_dir, source_file)
-            if destination_path.exists():
+            if destination_path.is_symlink() or destination_path.is_file():
+                # is_symlink() is checked first and deliberately: a dangling symlink is not
+                # exists(), so the previous exists() check left it in place and os.symlink then
+                # failed with FileExistsError.
                 os.remove(destination_path)
+            elif destination_path.is_dir():
+                # A real directory left behind by an earlier copying build. os.remove cannot remove
+                # it, and leaving it would shadow the symlink we are about to create.
+                shutil.rmtree(destination_path)
             else:
                 os.makedirs(destination_path.parent, exist_ok=True)
             utils.create_symlink_or_copy(str(source_path), str(destination_path))
 
+        self._prune_dangling_links(source_files)
+
+    def _prune_dangling_links(self, source_files):
+        """
+        Remove links this action left behind for sources that have since disappeared.
+
+        The loop above only visits names present in the source directory, so a dependency dropped
+        from the manifest keeps its destination entry from the previous build. While that entry was
+        a copied directory it was merely stale; as a link into a dependencies directory that has
+        been cleaned it dangles, and a dangling link is not a directory -- os.walk yields it among
+        the *files*, where both zipfile.write and tarfile.add(dereference=True) raise
+        FileNotFoundError instead of skipping it.
+
+        Only broken links pointing back into our own source directory are removed, so a link that
+        another consumer put in the destination (nodejs_npm_esbuild links into a scratch_dir that
+        already holds copied source) is left alone.
+        """
+        if not os.path.isdir(self._dest_dir):
+            return
+
+        for name in set(os.listdir(self._dest_dir)) - source_files:
+            stale_path = Path(self._dest_dir, name)
+            if not stale_path.is_symlink() or stale_path.exists():
+                continue
+            if not self._points_into_source(stale_path):
+                continue
+            LOG.debug("Removing %s: a link to a source that no longer exists", stale_path)
+            os.remove(stale_path)
+
+    def _points_into_source(self, link_path):
+        """
+        Is this link one we created, i.e. does it name a direct child of our source directory?
+
+        Read with os.readlink rather than os.path.realpath. For a BROKEN link the two disagree by
+        platform: POSIX realpath expands the recorded target whether or not it exists, while
+        Windows falls back to _getfinalpathname_nonstrict, which resolves as much of the path as
+        exists and joins the rest -- for a dangling link that is the link's own path, so the
+        recorded target is never consulted and every comparison against the source directory
+        fails. Windows CI is the gate that catches a regression here.
+
+        samefile() does the comparison because it is immune to the ways two spellings of one
+        directory differ (8.3 short names such as RUNNER~1, case, a symlinked temp root).
+        """
+        target = os.readlink(link_path)
+        if not os.path.isabs(target):
+            # A relative target is recorded verbatim and is meaningless without its link's parent.
+            target = os.path.join(os.path.dirname(link_path), target)
+
+        target_parent = os.path.dirname(os.path.normpath(target))
+        if not os.path.isdir(target_parent):
+            return False
+        return os.path.samefile(target_parent, self._source_dir)
+
 
 class LinkSinglePathAction(BaseAction):
     NAME = "LinkSource"

aws_lambda_builders/utils.py

@@ -15,6 +15,30 @@
 LOG = logging.getLogger(__name__)
 
 
+def _materialize_symlinked_destination(destination: str) -> None:
+    """
+    Replace a symlinked destination with a real copy of what it points at.
+
+    A linking build leaves symlinks into the shared dependencies directory. Copying into one would
+    follow the link and write outside the destination tree, mutating a cache that later builds
+    reuse. Materialising it first keeps the merge where the caller asked for it, which is what a
+    copying build did.
+    """
+    if not os.path.islink(destination):
+        return
+
+    LOG.debug("Replacing symlinked destination %s with a real copy before copying into it", destination)
+    link_target = os.path.realpath(destination)
+    os.unlink(destination)
+
+    if os.path.isdir(link_target):
+        copytree(link_target, destination)
+    elif os.path.isfile(link_target):
+        os.makedirs(os.path.dirname(destination), exist_ok=True)
+        shutil.copy2(link_target, destination)
+    # A dangling link leaves nothing to preserve; the caller creates the destination itself.
+
+
 def copytree(
     source: str,
     destination: str,
@@ -48,6 +72,8 @@ def copytree(
         LOG.warning("Skipping copy operation since source %s does not exist", source)
         return
 
+    _materialize_symlinked_destination(destination)
+
     if not os.path.exists(destination):
         LOG.debug("Creating target folders at %s", destination)
         os.makedirs(destination)
@@ -86,6 +112,10 @@ def copytree(
         elif os.path.isdir(new_source):
             copytree(new_source, new_destination, ignore=ignore, include=include, maintain_symlinks=maintain_symlinks)
         else:
+            # copy2 opens the destination for writing and would follow a symlink into the shared
+            # dependencies directory; it replaces the whole file, so unlinking loses nothing.
+            if os.path.islink(new_destination):
+                os.unlink(new_destination)
             LOG.debug("Copying source file (%s) to destination (%s)", new_source, new_destination)
             shutil.copy2(new_source, new_destination)
 
@@ -210,7 +240,21 @@ def create_symlink_or_copy(source: str, destination: str) -> None:
             "consider enabling the necessary settings or privileges on your system to support symbolic links.",
             exc_info=ex if LOG.isEnabledFor(logging.DEBUG) else None,
         )
-        copytree(source, destination)
+        if os.path.islink(destination):
+            # A leftover link is one reason os.symlink raised: the guard above misses a dangling one,
+            # which is not exists(). Copying through it would write outside the destination tree.
+            LOG.debug("Removing existing symlink at destination %s before copying", destination)
+            os.unlink(destination)
+        # A dependencies directory holds top-level files as well as packages (six.py, *.pth), and
+        # copytree assumes its source is a directory -- it would makedirs a folder named six.py and
+        # then raise NotADirectoryError on listdir.
+        if os.path.isdir(source):
+            copytree(source, destination)
+        elif os.path.isfile(source):
+            os.makedirs(os.path.dirname(destination), exist_ok=True)
+            shutil.copy2(source, destination)
+        else:
+            LOG.warning("Skipping copy operation since source %s does not exist", source)
 
 
 def _is_within_directory(directory: Union[str, os.PathLike], target: Union[str, os.PathLike]) -> bool:

aws_lambda_builders/workflows/python_pip/workflow.py

@@ -115,8 +115,32 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim
         # folder
         if self.dependencies_dir and self.combine_dependencies:
             # when copying downloaded dependencies back to artifacts folder, don't exclude anything
-            # symlinking python dependencies is disabled for now since it is breaking sam local commands
-            if False and is_experimental_build_improvements_enabled(self.experimental_flags):
+            #
+            # Symlinking is only safe for layers. A layer's artifacts are packed into a tarball
+            # (which dereferences symlinks) before they reach the local invoke container, whereas a
+            # function's artifacts are bind-mounted at /var/task, where a symlink pointing outside
+            # the mount dangles unless the caller passes `sam local invoke --mount-symlinks`. That
+            # option does not exist on `sam local start-api` / `start-lambda`, so linking function
+            # dependencies would break those commands -- which is why this was disabled wholesale in
+            # https://github.com/aws/aws-lambda-builders/pull/391. Keep copying for functions.
+            #
+            # The links are absolute, so they only resolve on the machine that built them. SAM CLI's
+            # container build (`sam build --use-container`) never sends a dependencies_dir over
+            # JSON-RPC, so it cannot reach this branch; a caller that does must share the path.
+            #
+            # Parent packages are the one layer configuration that must keep copying.
+            # PythonCreateParentPackagesAction runs last and calls mkdir(parents=True) on
+            # artifacts_dir/<first segment>/..., which follows a link of that name straight into
+            # dependencies_dir and then renames the source files in there -- writing the build's
+            # own files into the cache that later --cached builds reuse. SAM CLI reaches this for a
+            # layer whose metadata carries BuildProperties.ParentPackageMode (app_builder.py's
+            # _get_build_options is called for layers too), so copying here is what keeps the
+            # dependencies directory read-only to this workflow.
+            if (
+                self.is_building_layer
+                and not self._should_create_parent_packages()
+                and is_experimental_build_improvements_enabled(self.experimental_flags)
+            ):
                 self._actions.append(LinkSourceAction(self.dependencies_dir, artifacts_dir))
             else:
                 self._actions.append(CopySourceAction(self.dependencies_dir, artifacts_dir))

tests/unit/test_actions.py

@@ -1,3 +1,6 @@
+import os
+import shutil
+import tempfile
 from pathlib import Path
 from unittest import TestCase
 from unittest.mock import ANY, patch
@@ -13,6 +16,7 @@
     CleanUpAction,
     DependencyManager,
     LinkSinglePathAction,
+    LinkSourceAction,
 )
 
 
@@ -265,6 +269,154 @@ def _convert_strings_to_paths(source_dest_list):
         return map(lambda item: (Path(item[0]), Path(item[1])), source_dest_list)
 
 
+class TestLinkSourceAction(TestCase):
+    def setUp(self):
+        self._tmp = tempfile.TemporaryDirectory()
+        self.addCleanup(self._tmp.cleanup)
+        self.source_dir = Path(self._tmp.name, "deps")
+        self.dest_dir = Path(self._tmp.name, "artifacts")
+        (self.source_dir / "somepkg").mkdir(parents=True)
+        (self.source_dir / "somepkg" / "__init__.py").write_text("hello")
+        (self.source_dir / "six.py").write_text("six")
+        self.dest_dir.mkdir()
+
+    def _execute(self):
+        LinkSourceAction(str(self.source_dir), str(self.dest_dir)).execute()
+
+    def _assert_linked(self):
+        for name in ("somepkg", "six.py"):
+            destination = self.dest_dir / name
+            self.assertTrue(destination.is_symlink(), f"{name} should be a symlink")
+            self.assertEqual(os.path.realpath(destination), str((self.source_dir / name).resolve()))
+        self.assertEqual((self.dest_dir / "somepkg" / "__init__.py").read_text(), "hello")
+
+    def test_links_into_an_empty_destination(self):
+        self._execute()
+        self._assert_linked()
+
+    def test_replaces_a_real_directory_left_by_an_earlier_copying_build(self):
+        # A build that copied dependencies leaves real directories behind. Without --clean they
+        # survive into the next build, and os.remove() cannot remove a directory.
+        stale = self.dest_dir / "somepkg"
+        stale.mkdir()
+        (stale / "__init__.py").write_text("stale")
+        (self.dest_dir / "six.py").write_text("stale")
+
+        self._execute()
+        self._assert_linked()
+
+    def test_replaces_a_dangling_symlink(self):
+        # A dangling symlink is not exists(), so it used to be left in place and os.symlink then
+        # raised FileExistsError, which create_symlink_or_copy swallowed into a full copy.
+        (self.dest_dir / "six.py").symlink_to(self._tmp.name + "/gone")
+        self.assertFalse((self.dest_dir / "six.py").exists())
+
+        self._execute()
+        self._assert_linked()
+
+    def test_is_idempotent(self):
+        self._execute()
+        self._execute()
+        self._assert_linked()
+
+    def test_skips_a_source_that_does_not_exist(self):
+        # CopySourceAction warns and continues here; the layer path must not turn that into a failure.
+        missing = Path(self._tmp.name, "never-created")
+
+        LinkSourceAction(str(missing), str(self.dest_dir)).execute()
+
+        self.assertEqual(os.listdir(self.dest_dir), [])
+
+    def test_prunes_a_link_to_a_dependency_dropped_from_the_manifest(self):
+        # The link loop only visits names still in the source, so a dependency dropped from the
+        # manifest keeps its link from the previous build. It dangles once the dependencies dir is
+        # cleaned, and both zipfile.write and tarfile.add(dereference=True) raise FileNotFoundError
+        # on a broken link rather than skipping it.
+        self._execute()
+        shutil.rmtree(self.source_dir / "somepkg")
+        self.assertTrue((self.dest_dir / "somepkg").is_symlink())
+
+        self._execute()
+
+        self.assertNotIn("somepkg", os.listdir(self.dest_dir))
+        self.assertTrue((self.dest_dir / "six.py").is_symlink())
+        for root, _, files in os.walk(self.dest_dir, followlinks=True):
+            for name in files:
+                self.assertTrue(Path(root, name).exists(), f"{name} is a broken link")
+
+    def test_prunes_a_link_whose_recorded_target_is_relative(self):
+        # os.readlink returns a relative target verbatim, so it means nothing without the link's
+        # own parent. Without that join the containment check rejects the link and it survives.
+        self._execute()
+        (self.dest_dir / "somepkg").unlink()
+        os.symlink(os.path.join("..", "deps", "somepkg"), self.dest_dir / "somepkg")
+        shutil.rmtree(self.source_dir / "somepkg")
+        self.assertTrue((self.dest_dir / "somepkg").is_symlink())
+
+        self._execute()
+
+        self.assertNotIn("somepkg", os.listdir(self.dest_dir))
+
+    def test_keeps_a_broken_link_whose_target_tree_is_gone_entirely(self):
+        # samefile() raises FileNotFoundError when either side is missing, so a link into a tree
+        # that has been removed wholesale must be rejected before the comparison, not during it.
+        orphan = self.dest_dir / "orphan"
+        orphan.symlink_to(Path(self._tmp.name, "removed-tree", "pkg"))
+
+        self._execute()
+
+        self.assertTrue(orphan.is_symlink())
+        self._assert_linked()
+
+    def test_keeps_a_broken_link_pointing_outside_the_source_directory(self):
+        # nodejs_npm_esbuild links into a scratch_dir that already holds copied source. An entry
+        # this action did not create is not ours to reconcile, even when it is broken.
+        foreign = self.dest_dir / "foreign"
+        foreign.symlink_to(Path(self._tmp.name, "elsewhere"))
+
+        self._execute()
+
+        self.assertTrue(foreign.is_symlink())
+        self._assert_linked()
+
+    def test_leaves_a_real_stale_entry_from_a_dropped_dependency(self):
+        # Only broken links are ours to reconcile. A real directory left by an earlier copying
+        # build is stale but still packages and runs, which is how it behaved before this change;
+        # pruning build artifacts a build did not produce belongs to the caller, not this action.
+        stale = self.dest_dir / "dropped"
+        stale.mkdir()
+        (stale / "__init__.py").write_text("stale")
+
+        self._execute()
+
+        self.assertTrue(stale.is_dir())
+        self.assertEqual((stale / "__init__.py").read_text(), "stale")
+
+    def test_keeps_a_working_link_into_the_source_tree_under_another_name(self):
+        # Only BROKEN links are reconciled. A link that still resolves is serving someone, even
+        # when it points into our source tree under a name the source directory does not list.
+        alias = self.dest_dir / "alias.py"
+        alias.symlink_to(self.source_dir / "somepkg" / "__init__.py")
+        self.assertTrue(alias.exists())
+
+        self._execute()
+
+        self.assertTrue(alias.is_symlink())
+        self.assertEqual(alias.read_text(), "hello")
+
+    def test_tolerates_an_empty_source_and_a_destination_that_was_never_created(self):
+        # With at least one source file the link loop creates the destination on the way past, so
+        # this is the only ordering that reaches the prune step with no destination directory: an
+        # empty dependencies dir (pip downloaded nothing) and a first build.
+        empty_source = Path(self._tmp.name, "empty-deps")
+        empty_source.mkdir()
+        dest = Path(self._tmp.name, "artifacts-2")
+
+        LinkSourceAction(str(empty_source), str(dest)).execute()
+
+        self.assertFalse(dest.exists())
+
+
 class TestLinkSinglePathAction(TestCase):
     @patch("aws_lambda_builders.actions.os.makedirs")
     @patch("aws_lambda_builders.utils.create_symlink_or_copy")