From bbc105f28b64d53bb65b9a5c5a9735d0ca40b8c4 Mon Sep 17 00:00:00 2001 From: sunhua Date: Sun, 27 Sep 2026 04:29:15 +0000 Subject: [PATCH 1/4] perf(python): symlink layer dependencies instead of copying them PR #391 disabled symlinking wholesale (`if False and ...`) because symlinked dependencies broke `sam local`. That is true for FUNCTIONS only: a function's artifacts are bind-mounted at /var/task, so a symlink pointing outside the mount dangles unless `sam local invoke --mount-symlinks` is passed -- and that option does not exist on `sam local start-api` / `start-lambda`. A LAYER's artifacts are tarred into the local invoke image, and the tar dereferences symlinks. So gate on `is_building_layer`, which BaseWorkflow already carries. Functions keep copying. Measured on a 185 MB / ~6,200-file layer with SAM CLI: warm `sam build --cached` 1.88s -> 0.93s, build dir 184.5 MB -> ~20 KB. See aws/aws-sam-cli#4828. Enabling this for Python exposed three latent defects in the shared link path, all fixed here because a pip target directory reaches them where a node `node_modules/`-only directory did not: - `LinkSourceAction` had no unit tests, and `os.remove()` raises IsADirectoryError on a real directory left by an earlier copying build, while a dangling symlink is not `exists()` so it was left for `os.symlink` to fail on. - `create_symlink_or_copy`'s copy fallback called `copytree` unconditionally, which makedirs a folder named `six.py` and then raises NotADirectoryError. A dependencies directory always has top-level files, so the advertised fallback was a hard failure wherever symlink creation is not permitted. - `copytree` treated a symlinked destination as existing and recursed into it, so a source-tree name colliding with a dependency name was written THROUGH the link into the shared dependencies directory, corrupting a cache that later `--cached` builds reuse. It now materialises such a destination first, keeping the merge inside the destination tree as copying did. --- aws_lambda_builders/actions.py | 9 ++- aws_lambda_builders/utils.py | 35 ++++++++++- .../workflows/python_pip/workflow.py | 11 +++- tests/unit/test_actions.py | 54 +++++++++++++++++ tests/unit/test_utils.py | 59 ++++++++++++++++++- .../workflows/python_pip/test_workflow.py | 48 ++++++++++++--- 6 files changed, 204 insertions(+), 12 deletions(-) diff --git a/aws_lambda_builders/actions.py b/aws_lambda_builders/actions.py index 491bcc5a6..0ba2ce4da 100644 --- a/aws_lambda_builders/actions.py +++ b/aws_lambda_builders/actions.py @@ -137,8 +137,15 @@ def execute(self): 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)) diff --git a/aws_lambda_builders/utils.py b/aws_lambda_builders/utils.py index d18ccf0a9..17a303cca 100644 --- a/aws_lambda_builders/utils.py +++ b/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) @@ -210,7 +236,14 @@ 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) + # 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) + else: + os.makedirs(os.path.dirname(destination), exist_ok=True) + shutil.copy2(source, destination) def _is_within_directory(directory: Union[str, os.PathLike], target: Union[str, os.PathLike]) -> bool: diff --git a/aws_lambda_builders/workflows/python_pip/workflow.py b/aws_lambda_builders/workflows/python_pip/workflow.py index d6c84e198..c8c2ce22d 100644 --- a/aws_lambda_builders/workflows/python_pip/workflow.py +++ b/aws_lambda_builders/workflows/python_pip/workflow.py @@ -115,8 +115,15 @@ 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. + 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)) diff --git a/tests/unit/test_actions.py b/tests/unit/test_actions.py index e85b12778..9323d8260 100644 --- a/tests/unit/test_actions.py +++ b/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,57 @@ 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() + + class TestLinkSinglePathAction(TestCase): @patch("aws_lambda_builders.actions.os.makedirs") @patch("aws_lambda_builders.utils.create_symlink_or_copy") diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index aecb86797..1f80d6604 100644 --- a/tests/unit/test_utils.py +++ b/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,7 +45,7 @@ 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): source_path = "source/path" destination_path = "destination/path" utils.create_symlink_or_copy(source_path, destination_path) @@ -48,6 +53,58 @@ def test_must_copy_if_symlink_fails(self, patched_copy_tree, pathced_os, patched pathced_os.symlink.assert_not_called() 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") + + +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") + class TestDecode(TestCase): def test_does_not_crash_non_utf8_encoding(self): diff --git a/tests/unit/workflows/python_pip/test_workflow.py b/tests/unit/workflows/python_pip/test_workflow.py index b8ec16da8..85b2d1eaf 100644 --- a/tests/unit/workflows/python_pip/test_workflow.py +++ b/tests/unit/workflows/python_pip/test_workflow.py @@ -12,14 +12,24 @@ @parameterized_class( - ("experimental_flags",), + ("experimental_flags", "is_building_layer"), [ - ([]), - ([EXPERIMENTAL_FLAG_BUILD_PERFORMANCE]), + ([], False), + ([], True), + ([EXPERIMENTAL_FLAG_BUILD_PERFORMANCE], False), + ([EXPERIMENTAL_FLAG_BUILD_PERFORMANCE], True), ], ) class TestPythonPipWorkflow(TestCase): experimental_flags = [] + is_building_layer = False + + @property + def expects_linked_dependencies(self): + """Dependencies are symlinked only when the build performance flag is on AND we are + building a layer -- a function's artifacts are bind-mounted into the local invoke + container, where symlinks pointing outside the mount dangle.""" + return bool(self.experimental_flags) and self.is_building_layer def setUp(self): self.osutils = OSUtils() @@ -121,10 +131,10 @@ def test_workflow_sets_up_actions_without_download_dependencies_with_dependencie dependencies_dir="dep", download_dependencies=False, experimental_flags=self.experimental_flags, + is_building_layer=self.is_building_layer, ) self.assertEqual(len(self.workflow.actions), 2) - # symlinking python dependencies is disabled for now since it is breaking sam local commands - if False and self.experimental_flags: + if self.expects_linked_dependencies: self.assertIsInstance(self.workflow.actions[0], LinkSourceAction) else: self.assertIsInstance(self.workflow.actions[0], CopySourceAction) @@ -143,12 +153,12 @@ def test_workflow_sets_up_actions_with_download_dependencies_and_dependencies_di dependencies_dir="dep", download_dependencies=True, experimental_flags=self.experimental_flags, + is_building_layer=self.is_building_layer, ) self.assertEqual(len(self.workflow.actions), 4) self.assertIsInstance(self.workflow.actions[0], CleanUpAction) self.assertIsInstance(self.workflow.actions[1], PythonPipBuildAction) - # symlinking python dependencies is disabled for now since it is breaking sam local commands - if False and self.experimental_flags: + if self.expects_linked_dependencies: self.assertIsInstance(self.workflow.actions[2], LinkSourceAction) else: self.assertIsInstance(self.workflow.actions[2], CopySourceAction) @@ -193,6 +203,30 @@ def test_workflow_sets_up_actions_without_combine_dependencies(self): self.assertIsInstance(self.workflow.actions[1], PythonPipBuildAction) self.assertIsInstance(self.workflow.actions[2], CopySourceAction) + def test_layer_links_dependencies_while_function_copies_them(self): + """The layer/function distinction is the whole reason linking is safe at all, so assert the + contrast on inputs that are otherwise identical -- otherwise a future change that drops + is_building_layer still passes every other test in this class.""" + + def actions_for(is_building_layer): + osutils_mock = Mock(spec=self.osutils) + osutils_mock.file_exists.return_value = True + return PythonPipWorkflow( + "source", + "artifacts", + "scratch_dir", + "manifest", + runtime="python3.9", + osutils=osutils_mock, + dependencies_dir="dep", + download_dependencies=True, + experimental_flags=[EXPERIMENTAL_FLAG_BUILD_PERFORMANCE], + is_building_layer=is_building_layer, + ).actions + + self.assertIsInstance(actions_for(is_building_layer=True)[2], LinkSourceAction) + self.assertIsInstance(actions_for(is_building_layer=False)[2], CopySourceAction) + @patch("aws_lambda_builders.workflows.python_pip.workflow.PythonPipBuildAction") def test_must_build_with_architecture(self, PythonPipBuildActionMock): self.workflow = PythonPipWorkflow( From 900fe2315d29b407ea7e3629395041d605b9238f Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Mon, 28 Sep 2026 16:15:39 +0000 Subject: [PATCH 2/4] fix: do not write through symlinked file destinations; skip missing fallback sources copytree's file branch now unlinks a symlinked destination before copy2, so a source file colliding with a linked dependency (six.py) no longer overwrites the shared dependencies directory, and a dangling link no longer creates a file outside the artifacts tree. create_symlink_or_copy's copy fallback restores the warn-and-skip for a source that is neither a directory nor a file, e.g. a relative os.readlink() target under maintain_symlinks. --- aws_lambda_builders/utils.py | 8 +++++- tests/unit/test_utils.py | 51 ++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/aws_lambda_builders/utils.py b/aws_lambda_builders/utils.py index 17a303cca..8e8dbcc8f 100644 --- a/aws_lambda_builders/utils.py +++ b/aws_lambda_builders/utils.py @@ -112,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) @@ -241,9 +245,11 @@ def create_symlink_or_copy(source: str, destination: str) -> None: # then raise NotADirectoryError on listdir. if os.path.isdir(source): copytree(source, destination) - else: + 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: diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 1f80d6604..cbca5ef8a 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -79,6 +79,17 @@ def test_falls_back_to_copying_a_package_directory(self): 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()) + class Test_copytree(TestCase): def test_does_not_write_through_a_symlinked_destination(self): @@ -105,6 +116,46 @@ def test_does_not_write_through_a_symlinked_destination(self): 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): From fadaa49bd37658c4445d1522fe5f9ed4243f1342 Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Mon, 28 Sep 2026 16:32:22 +0000 Subject: [PATCH 3/4] fix: skip a missing dependencies dir when linking; pin symlink-success test LinkSourceAction now warns and returns when its source directory does not exist, matching the CopySourceAction it replaces for layers, instead of raising FileNotFoundError (which the workflow wraps into a build failure). test_must_not_copy_when_symlink_succeeds took the already-a-symlink early return because the mocked Path made exists()/is_symlink() truthy; it now forces the guard false and asserts os.symlink is called. Note next to the python_pip layer gate that the links are absolute and that SAM CLI's container build never passes a dependencies_dir, so it cannot reach the linking branch. --- aws_lambda_builders/actions.py | 6 ++++++ aws_lambda_builders/workflows/python_pip/workflow.py | 4 ++++ tests/unit/test_actions.py | 8 ++++++++ tests/unit/test_utils.py | 5 ++++- 4 files changed, 22 insertions(+), 1 deletion(-) diff --git a/aws_lambda_builders/actions.py b/aws_lambda_builders/actions.py index 0ba2ce4da..7c9155ddf 100644 --- a/aws_lambda_builders/actions.py +++ b/aws_lambda_builders/actions.py @@ -132,6 +132,12 @@ 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: diff --git a/aws_lambda_builders/workflows/python_pip/workflow.py b/aws_lambda_builders/workflows/python_pip/workflow.py index c8c2ce22d..8b72577a8 100644 --- a/aws_lambda_builders/workflows/python_pip/workflow.py +++ b/aws_lambda_builders/workflows/python_pip/workflow.py @@ -123,6 +123,10 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim # 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: diff --git a/tests/unit/test_actions.py b/tests/unit/test_actions.py index 9323d8260..a4229ff3f 100644 --- a/tests/unit/test_actions.py +++ b/tests/unit/test_actions.py @@ -318,6 +318,14 @@ def test_is_idempotent(self): 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") diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index cbca5ef8a..e12472116 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -46,11 +46,14 @@ def test_must_copy_if_symlink_fails(self, patched_copy_tree, pathced_os, patched @patch("aws_lambda_builders.utils.os") @patch("aws_lambda_builders.utils.copytree") 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): From d0f5f6af59250e587836ea0bf503c0decf15b56c Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Mon, 28 Sep 2026 17:00:26 +0000 Subject: [PATCH 4/4] fix: clear a leftover destination link before the copy fallback A dangling symlink at the destination is not exists(), so the already-a-symlink guard in create_symlink_or_copy misses it and os.symlink raises FileExistsError. The copy fallback then ran with the stale link still in place, and shutil.copy2 followed it, writing the dependency outside the destination tree and leaving the destination a symlink. Reachable through LinkSinglePathAction (nodejs_npm, nodejs_npm_esbuild) and through copytree's maintain_symlinks branch, neither of which removes the destination first. The handler now unlinks a symlinked destination before copying, which covers every caller of the helper. --- aws_lambda_builders/utils.py | 5 +++++ tests/unit/test_utils.py | 17 +++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/aws_lambda_builders/utils.py b/aws_lambda_builders/utils.py index 8e8dbcc8f..46b0af16d 100644 --- a/aws_lambda_builders/utils.py +++ b/aws_lambda_builders/utils.py @@ -240,6 +240,11 @@ 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, ) + 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. diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index e12472116..19a1bbeb4 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -93,6 +93,23 @@ def test_fallback_skips_a_source_that_does_not_exist(self): 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):