From bbc105f28b64d53bb65b9a5c5a9735d0ca40b8c4 Mon Sep 17 00:00:00 2001 From: sunhua Date: Sun, 27 Sep 2026 04:29:15 +0000 Subject: [PATCH 1/7] 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/7] 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/7] 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/7] 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): From 00ac451a20e5a6a807b9291dafbc38ccf25fb8f6 Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Wed, 28 Oct 2026 21:46:59 +0000 Subject: [PATCH 5/7] fix: prune links left by dependencies dropped from the manifest LinkSourceAction.execute() iterates the source directory, so a dependency removed from the manifest keeps the destination entry from the previous build. While that entry was a copied directory it was merely stale; as a link into a cleaned dependencies dir it dangles, and a dangling link is not a directory -- os.walk yields it among the files, where zipfile.write and tarfile.add(dereference=True) both raise FileNotFoundError. Reconcile after linking, removing only broken links that point back into our own source directory so entries another consumer placed in the destination are left alone. --- aws_lambda_builders/actions.py | 34 ++++++++++++++++++ tests/unit/test_actions.py | 66 ++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/aws_lambda_builders/actions.py b/aws_lambda_builders/actions.py index 7c9155ddf..f0a70a552 100644 --- a/aws_lambda_builders/actions.py +++ b/aws_lambda_builders/actions.py @@ -156,6 +156,40 @@ def execute(self): 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 + + source_root = Path(self._source_dir).resolve() + + 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 + # realpath() resolves a broken link to where it points rather than raising, which is + # what we need to compare against; Path.resolve() would be fine too, but the intent is + # the recorded target, not a path that must exist. + if not Path(os.path.realpath(stale_path)).is_relative_to(source_root): + continue + LOG.debug("Removing %s: a link to a source that no longer exists", stale_path) + os.remove(stale_path) + class LinkSinglePathAction(BaseAction): NAME = "LinkSource" diff --git a/tests/unit/test_actions.py b/tests/unit/test_actions.py index a4229ff3f..ce0f07100 100644 --- a/tests/unit/test_actions.py +++ b/tests/unit/test_actions.py @@ -1,4 +1,5 @@ import os +import shutil import tempfile from pathlib import Path from unittest import TestCase @@ -326,6 +327,71 @@ def test_skips_a_source_that_does_not_exist(self): 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_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") From 0e904a88e17276eb812456959b5bb2526f90602b Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Wed, 28 Oct 2026 23:30:58 +0000 Subject: [PATCH 6/7] fix: identify our own stale links by readlink, not realpath Windows CI caught the prune step skipping every dangling link. For a BROKEN link os.path.realpath disagrees by platform: POSIX 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 the containment check always failed. Read the target with os.readlink, which returns what was recorded on both platforms, join a relative target against the link's parent, and compare with samefile so two spellings of one directory (8.3 short names, case, a symlinked temp root) cannot disagree. Reject a target whose parent is gone before comparing, since samefile raises on a missing path. --- aws_lambda_builders/actions.py | 31 +++++++++++++++++++++++++------ tests/unit/test_actions.py | 24 ++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/aws_lambda_builders/actions.py b/aws_lambda_builders/actions.py index f0a70a552..80c010ed5 100644 --- a/aws_lambda_builders/actions.py +++ b/aws_lambda_builders/actions.py @@ -176,20 +176,39 @@ def _prune_dangling_links(self, source_files): if not os.path.isdir(self._dest_dir): return - source_root = Path(self._source_dir).resolve() - 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 - # realpath() resolves a broken link to where it points rather than raising, which is - # what we need to compare against; Path.resolve() would be fine too, but the intent is - # the recorded target, not a path that must exist. - if not Path(os.path.realpath(stale_path)).is_relative_to(source_root): + 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" diff --git a/tests/unit/test_actions.py b/tests/unit/test_actions.py index ce0f07100..f8f0fe486 100644 --- a/tests/unit/test_actions.py +++ b/tests/unit/test_actions.py @@ -344,6 +344,30 @@ def test_prunes_a_link_to_a_dependency_dropped_from_the_manifest(self): 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. From 8fe67dfd36251e9a4d858a89831267d9b300c44d Mon Sep 17 00:00:00 2001 From: Harold Sun Date: Wed, 28 Oct 2026 23:39:12 +0000 Subject: [PATCH 7/7] fix: keep copying for a layer that also creates parent packages PythonCreateParentPackagesAction runs last and calls mkdir(parents=True) on artifacts_dir//..., which follows a link of that name into dependencies_dir and then renames the build's own source files in there -- writing into the cache that later --cached builds reuse. Reproduced with a linked `google` dependency and parent_python_packages="google.cloud": the source file landed at dependencies_dir/google/cloud/app.py. SAM CLI reaches this for a layer whose metadata carries BuildProperties.ParentPackageMode, since _get_build_options is called for layers as well as functions. Copying for that configuration keeps the dependencies directory read-only to this workflow. --- .../workflows/python_pip/workflow.py | 15 ++++++++++- .../workflows/python_pip/test_workflow.py | 26 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/aws_lambda_builders/workflows/python_pip/workflow.py b/aws_lambda_builders/workflows/python_pip/workflow.py index 8b72577a8..aeee60d57 100644 --- a/aws_lambda_builders/workflows/python_pip/workflow.py +++ b/aws_lambda_builders/workflows/python_pip/workflow.py @@ -127,7 +127,20 @@ def __init__(self, source_dir, artifacts_dir, scratch_dir, manifest_path, runtim # 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): + # + # Parent packages are the one layer configuration that must keep copying. + # PythonCreateParentPackagesAction runs last and calls mkdir(parents=True) on + # artifacts_dir//..., 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)) diff --git a/tests/unit/workflows/python_pip/test_workflow.py b/tests/unit/workflows/python_pip/test_workflow.py index 85b2d1eaf..d693e0150 100644 --- a/tests/unit/workflows/python_pip/test_workflow.py +++ b/tests/unit/workflows/python_pip/test_workflow.py @@ -227,6 +227,32 @@ def actions_for(is_building_layer): self.assertIsInstance(actions_for(is_building_layer=True)[2], LinkSourceAction) self.assertIsInstance(actions_for(is_building_layer=False)[2], CopySourceAction) + def test_a_layer_wanting_parent_packages_copies_instead_of_linking(self): + """PythonCreateParentPackagesAction runs last and mkdir()s through artifacts_dir//..., so a link of that name leads straight into dependencies_dir and the source + files are renamed into the cache that --cached reuses. Copying keeps the cache read-only.""" + + def actions_for(options): + 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=True, + options=options, + ).actions + + self.assertIsInstance(actions_for({"parent_python_packages": "google.cloud"})[2], CopySourceAction) + # Any other option leaves the optimisation in place. + self.assertIsInstance(actions_for({"artifact_executable_name": "app"})[2], LinkSourceAction) + @patch("aws_lambda_builders.workflows.python_pip.workflow.PythonPipBuildAction") def test_must_build_with_architecture(self, PythonPipBuildActionMock): self.workflow = PythonPipWorkflow(