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

aws_lambda_builders/actions.py

@@ -132,13 +132,26 @@ 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))

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,19 @@ 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.
+            if self.is_building_layer 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,5 @@
+import os
+import tempfile
 from pathlib import Path
 from unittest import TestCase
 from unittest.mock import ANY, patch
@@ -13,6 +15,7 @@
     CleanUpAction,
     DependencyManager,
     LinkSinglePathAction,
+    LinkSourceAction,
 )
 
 
@@ -265,6 +268,65 @@ 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), [])
+
+
 class TestLinkSinglePathAction(TestCase):
     @patch("aws_lambda_builders.actions.os.makedirs")
     @patch("aws_lambda_builders.utils.create_symlink_or_copy")

tests/unit/test_utils.py

@@ -1,4 +1,6 @@
+import os
 import platform
+import tempfile
 from pathlib import Path
 
 from unittest import TestCase
@@ -29,6 +31,9 @@ def test_must_create_symlink_with_absolute_path(self, patched_copy_tree, patched
     @patch("aws_lambda_builders.utils.copytree")
     def test_must_copy_if_symlink_fails(self, patched_copy_tree, pathced_os, patched_path):
         pathced_os.symlink.side_effect = OSError("Unable to create symlink")
+        # Without this the mocked Path makes the already-a-symlink branch truthy and the function
+        # returns before it ever calls os.symlink.
+        patched_path.return_value.exists.return_value = False
 
         source_path = "source/path"
         destination_path = "destination/path"
@@ -40,14 +45,137 @@ def test_must_copy_if_symlink_fails(self, patched_copy_tree, pathced_os, patched
     @patch("aws_lambda_builders.utils.Path")
     @patch("aws_lambda_builders.utils.os")
     @patch("aws_lambda_builders.utils.copytree")
-    def test_must_copy_if_symlink_fails(self, patched_copy_tree, pathced_os, patched_path):
+    def test_must_not_copy_when_symlink_succeeds(self, patched_copy_tree, pathced_os, patched_path):
+        # As above: without this the already-a-symlink early return is taken and os.symlink is never reached.
+        patched_path.return_value.exists.return_value = False
+
         source_path = "source/path"
         destination_path = "destination/path"
         utils.create_symlink_or_copy(source_path, destination_path)
 
-        pathced_os.symlink.assert_not_called()
+        pathced_os.symlink.assert_called_once()
         patched_copy_tree.assert_not_called()
 
+    def test_falls_back_to_copying_a_top_level_file(self):
+        # A dependencies directory holds files as well as packages, and copytree cannot copy a file.
+        with tempfile.TemporaryDirectory() as tmp:
+            source = Path(tmp, "six.py")
+            source.write_text("body")
+            destination = Path(tmp, "artifacts", "six.py")
+
+            with patch("aws_lambda_builders.utils.os.symlink", side_effect=OSError("privilege not held")):
+                utils.create_symlink_or_copy(str(source), str(destination))
+
+            self.assertTrue(destination.is_file())
+            self.assertEqual(destination.read_text(), "body")
+
+    def test_falls_back_to_copying_a_package_directory(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            source = Path(tmp, "somepkg")
+            source.mkdir()
+            (source / "__init__.py").write_text("body")
+            destination = Path(tmp, "artifacts", "somepkg")
+
+            with patch("aws_lambda_builders.utils.os.symlink", side_effect=OSError("privilege not held")):
+                utils.create_symlink_or_copy(str(source), str(destination))
+
+            self.assertTrue(destination.is_dir())
+            self.assertEqual((destination / "__init__.py").read_text(), "body")
+
+    def test_fallback_skips_a_source_that_does_not_exist(self):
+        # maintain_symlinks passes raw os.readlink() output, which is often relative to the link
+        # rather than the CWD (npm's node_modules/.bin entries), so the fallback must skip it.
+        with tempfile.TemporaryDirectory() as tmp:
+            destination = Path(tmp, "artifacts", "tsc")
+
+            with patch("aws_lambda_builders.utils.os.symlink", side_effect=OSError("privilege not held")):
+                utils.create_symlink_or_copy("../typescript/bin/tsc", str(destination))
+
+            self.assertFalse(destination.exists())
+
+    def test_fallback_does_not_copy_through_a_dangling_destination_link(self):
+        # A dangling link at the destination is not exists(), so the already-a-symlink guard misses
+        # it and os.symlink raises FileExistsError. The fallback must not then copy through it.
+        with tempfile.TemporaryDirectory() as tmp:
+            source = Path(tmp, "six.py")
+            source.write_text("body")
+            outside = Path(tmp, "outside.txt")
+            destination = Path(tmp, "artifacts", "six.py")
+            destination.parent.mkdir()
+            destination.symlink_to(str(outside))
+
+            utils.create_symlink_or_copy(str(source), str(destination))
+
+            self.assertFalse(outside.exists(), "copy followed a dangling link outside the destination tree")
+            self.assertFalse(destination.is_symlink())
+            self.assertEqual(destination.read_text(), "body")
+
+
+class Test_copytree(TestCase):
+    def test_does_not_write_through_a_symlinked_destination(self):
+        """A linking build leaves symlinks into the shared dependencies directory. Copying the
+        source tree over a colliding name must stay inside the destination tree rather than
+        following the link and mutating a cache that later builds reuse."""
+        with tempfile.TemporaryDirectory() as tmp:
+            deps = Path(tmp, "deps", "requests")
+            deps.mkdir(parents=True)
+            (deps / "__init__.py").write_text("dependency")
+
+            artifacts = Path(tmp, "artifacts")
+            artifacts.mkdir()
+            os.symlink(str(deps), str(artifacts / "requests"))
+
+            source = Path(tmp, "source", "requests")
+            source.mkdir(parents=True)
+            (source / "my_helper.py").write_text("user code")
+
+            utils.copytree(str(Path(tmp, "source")), str(artifacts))
+
+            self.assertFalse((deps / "my_helper.py").exists(), "source leaked into the dependencies directory")
+            self.assertFalse((artifacts / "requests").is_symlink())
+            self.assertEqual((artifacts / "requests" / "my_helper.py").read_text(), "user code")
+            self.assertEqual((artifacts / "requests" / "__init__.py").read_text(), "dependency")
+
+    def test_does_not_write_through_a_symlinked_file_destination(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            deps = Path(tmp, "deps")
+            deps.mkdir()
+            (deps / "six.py").write_text("dependency")
+
+            artifacts = Path(tmp, "artifacts")
+            artifacts.mkdir()
+            os.symlink(str(deps / "six.py"), str(artifacts / "six.py"))
+
+            source = Path(tmp, "source")
+            source.mkdir()
+            (source / "six.py").write_text("user code")
+
+            utils.copytree(str(source), str(artifacts))
+
+            self.assertEqual(
+                (deps / "six.py").read_text(), "dependency", "source leaked into the dependencies directory"
+            )
+            self.assertFalse((artifacts / "six.py").is_symlink())
+            self.assertEqual((artifacts / "six.py").read_text(), "user code")
+
+    def test_does_not_create_a_file_through_a_dangling_symlink(self):
+        with tempfile.TemporaryDirectory() as tmp:
+            outside = Path(tmp, "outside", "six.py")
+            outside.parent.mkdir()
+
+            artifacts = Path(tmp, "artifacts")
+            artifacts.mkdir()
+            os.symlink(str(outside), str(artifacts / "six.py"))
+
+            source = Path(tmp, "source")
+            source.mkdir()
+            (source / "six.py").write_text("user code")
+
+            utils.copytree(str(source), str(artifacts))
+
+            self.assertFalse(outside.exists(), "copy followed a dangling link outside the destination tree")
+            self.assertEqual((artifacts / "six.py").read_text(), "user code")
+
 
 class TestDecode(TestCase):
     def test_does_not_crash_non_utf8_encoding(self):