Conversation
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Results
Reviewed: 587257c..0c0df09
Files: 4
Comments: 3
Comments on lines outside the diff:
[aws_lambda_builders/actions.py:151] [BUG] The copy fallback described in the PR description does not actually work for file entries, and a Python dependencies directory always has them. create_symlink_or_copy falls back to copytree(source, destination) when os.symlink raises (the common case being WinError 1314, privilege not held), and copytree assumes source is a directory:
if not os.path.exists(destination):
os.makedirs(destination) # creates a directory named six.py
...
names = os.listdir(source) # NotADirectoryError on a regular fileThe copytree call is inside the except OSError handler, so the resulting NotADirectoryError is not caught and the build fails. Until now the only LinkSourceAction consumer was nodejs_npm_esbuild, whose dependencies dir contains just node_modules/, so the bug stayed dormant. A pip-installed target dir reliably has top-level files (six.py, typing_extensions.py, *.pth), so enabling this for Python layers turns "fall back to copying" into a hard failure on any host where symlink creation is not permitted.
Either handle files in create_symlink_or_copy (shutil.copy2 when Path(source).is_file()) or pick the copy primitive in LinkSourceAction based on source_path.is_dir(). A test that forces os.symlink to raise OSError and asserts a file dependency still lands as a file would lock this down — the new tests all assert is_symlink(), so none of them exercise the fallback.
[aws_lambda_builders/workflows/python_pip/workflow.py:127] [BUG] The final CopySourceAction(source_dir, artifacts_dir) queued below runs after the dependency links exist, and copytree recurses into a destination directory without checking whether it is a symlink:
if not os.path.exists(destination): # True for a symlink to a directory -> skipped
os.makedirs(destination)
...
elif os.path.isdir(new_source):
copytree(new_source, new_destination)So if a top-level name in source_dir collides with a top-level name in dependencies_dir, the source files are written through the symlink into the shared dependencies directory instead of into the artifacts directory. With copying, that merge stayed inside artifacts_dir and was discarded on the next build; with links it mutates the cache that --cached reuses across builds, and CleanUpAction only wipes it when download_dependencies is true, so the pollution survives and gets published into later builds.
The collision requires a matching name, so it is not the common case, but the blast radius is a corrupted shared cache outside the build directory. Worth either replacing a colliding link with a real copy or asserting the artifacts-dir boundary before the source copy runs.
| 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(): |
There was a problem hiding this comment.
[BUG] os.remove() cannot delete a directory symlink on Windows — DeleteFileW fails with ERROR_ACCESS_DENIED for a reparse point that is a directory, which surfaces as PermissionError. Since is_symlink() is now the first branch, every rebuild of an already-linked dependency directory takes it, so the idempotent second build that test_is_idempotent covers on POSIX will fail on Windows for package directories (requests/, certifi/, …) whenever symlink creation succeeded on the first build (developer mode / elevated shell). The exception is not an ActionFailedError, so it propagates as a hard workflow failure rather than degrading to a copy.
The directory symlink must be removed with os.rmdir() on Windows (it unlinks the link, it does not recurse):
if destination_path.is_symlink():
if sys.platform == "win32" and destination_path.is_dir():
# os.remove() cannot delete a directory symlink/junction on Windows
os.rmdir(destination_path)
else:
os.remove(destination_path)
elif destination_path.is_file():
os.remove(destination_path)
elif destination_path.is_dir():
shutil.rmtree(destination_path)
else:
os.makedirs(destination_path.parent, exist_ok=True)Note this also pre-dates the PR (the old exists() branch called os.remove on the same input), but this PR is what makes the path reachable for Python layer builds, and it is the branch being rewritten here.
There was a problem hiding this comment.
Checked this against Windows rather than reasoning about it, and it does not reproduce: all four TestLinkSourceAction tests pass on windows-latest, including test_is_idempotent, which is exactly the scenario described here.
test_is_idempotent links somepkg (a real directory), then runs the action again. The second run takes the is_symlink() branch and calls os.remove() on a directory symlink. _assert_linked then asserts destination.is_symlink() is True for that entry, so a silent fallback to copying would fail the test too — the symlink really was created and really was removed.
From windows-latest / 3.10 / unit-functional on this head (job 108558335756):
tests/unit/test_actions.py::TestLinkSourceAction::test_is_idempotent PASSED
tests/unit/test_actions.py::TestLinkSourceAction::test_links_into_an_empty_destination PASSED
tests/unit/test_actions.py::TestLinkSourceAction::test_replaces_a_dangling_symlink PASSED
tests/unit/test_actions.py::TestLinkSourceAction::test_replaces_a_real_directory_left_by_an_earlier_copying_build PASSED
Same result on the 3.11, 3.12 and 3.13 unit-functional jobs, so it is not a single-version quirk.
The Win32 distinction you cite is real — DeleteFile's own docs say "To remove an empty directory, use the RemoveDirectory function" — but os.remove is not a thin wrapper over DeleteFileW, and empirically CPython handles the directory reparse point on all four supported versions. Adding a sys.platform == "win32" branch would therefore be an untested path guarding a condition that does not occur, so I would rather not carry it.
Happy to reconsider with a failing case on a Windows configuration the CI matrix does not cover.
Your other two comments were both real and are fixed — replies on those separately.
PR aws#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.
|
Re Reproduced it exactly as you described, by forcing So the fallback this PR advertises was a hard failure on any host where symlink creation is not permitted, and you are right that it stayed dormant because
if os.path.isdir(source):
copytree(source, destination)
else:
os.makedirs(os.path.dirname(destination), exist_ok=True)
shutil.copy2(source, destination)After the fix, the same probe gives You were also right that none of my tests exercised the fallback, since they all assert One thing I found while adding them, worth flagging separately: |
|
Re A dependency Exactly as you described: the file lands outside the artifacts directory, in the cache that later Fixed in 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)That keeps the semantics a copying build had — the collision merges inside the artifacts directory and is discarded on the next build — rather than just refusing the copy. After the fix: Pinned by I took the shared-layer route over "assert the artifacts-dir boundary" because the boundary check would turn a name collision into a build failure, and collisions were previously legal and harmless. Full gates on the amended commit: |
0c0df09 to
bbc105f
Compare
| LOG.warning("Skipping copy operation since source %s does not exist", source) | ||
| return | ||
|
|
||
| _materialize_symlinked_destination(destination) |
There was a problem hiding this comment.
[BUG] The write-through fix only covers directory entries; file entries still write into the shared dependencies cache.
_materialize_symlinked_destination is called once per copytree invocation, and copytree only recurses (and therefore only re-checks the destination) when the source entry is a directory. The leaf branch is unchanged:
elif os.path.isdir(new_source):
copytree(new_source, new_destination, ...) # materialize runs on new_destination
else:
shutil.copy2(new_source, new_destination) # follows a symlinked new_destinationSo for the same layer scenario you reproduced, but with a top-level module instead of a package, CopySourceAction(source_dir, artifacts_dir) still leaks: artifacts/six.py is a symlink into deps/six.py after LinkSourceAction, the user's own six.py hits the else branch, and shutil.copy2 opens the destination for writing, follows the link, and overwrites the file inside the shared deps directory that later builds reuse. A dangling destination symlink is worse — copy2 creates the file at the link target, outside the artifacts tree entirely.
Top-level files are exactly the case this PR added elsewhere (create_symlink_or_copy's six.py fallback, and the six.py fixture in TestLinkSourceAction), so a Python dependencies directory reliably produces symlinked file destinations.
Unlinking is sufficient here — copy2 overwrites the whole file, so there is nothing to preserve:
else:
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)Worth extending test_does_not_write_through_a_symlinked_destination with a symlinked file entry, since that test currently only exercises the directory path.
There was a problem hiding this comment.
Confirmed, and fixed in 900fe23. Reproduced against bbc105f: after LinkSourceAction, artifacts/six.py is a symlink into deps/six.py, and CopySourceAction with a user six.py left deps/six.py containing the user's code.
The fix is what you suggested: the file branch of copytree unlinks a symlinked new_destination before shutil.copy2. This also covers the dangling-link case, so no file is created at the link target.
Two new tests, test_does_not_write_through_a_symlinked_file_destination and test_does_not_create_a_file_through_a_dangling_symlink. Removing the unlink makes both fail.
| # then raise NotADirectoryError on listdir. | ||
| if os.path.isdir(source): | ||
| copytree(source, destination) | ||
| else: |
There was a problem hiding this comment.
[ERROR_HANDLING] The new else branch drops copytree's warn-and-return on a missing source, turning a skip into a build failure.
copytree starts with:
if not os.path.exists(source):
LOG.warning("Skipping copy operation since source %s does not exist", source)
returnEvery fallback used to inherit that. Now a source that is neither a dir nor a file reaches shutil.copy2(source, destination) and raises FileNotFoundError, which nothing in the action chain catches.
That case is reachable, and not only through the dependencies directory. copytree's maintain_symlinks branch passes the raw os.readlink() result:
linkto = os.readlink(new_source)
create_symlink_or_copy(linkto, new_destination)linkto is commonly relative (npm writes node_modules/.bin/tsc -> ../typescript/bin/tsc), so it does not resolve against the process CWD and both isdir and isfile are False. CopyDependenciesAction does the same. nodejs_npm uses maintain_symlinks=is_building_in_source, and the trigger for reaching the fallback at all is a host where os.symlink raises — Windows without the symlink privilege — which is precisely the configuration where those relative link targets now crash the build instead of being skipped with a warning.
Restoring the third branch keeps the previous contract:
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)Not re-raised: the Windows os.remove()-on-directory-symlink concern from the earlier review, which you dismissed with windows-latest CI evidence for all four TestLinkSourceAction cases. The is_building_layer gate itself checks out — BaseWorkflow sets the attribute (workflow.py:266) and __main__.py threads it through from the JSON-RPC request.
There was a problem hiding this comment.
Confirmed, and fixed in 900fe23. With os.symlink failing, create_symlink_or_copy("../typescript/bin/tsc", dest) raised FileNotFoundError from shutil.copy2 on bbc105f. Before this PR it went through copytree and was skipped with a warning.
The third branch is restored as you proposed: directory → copytree, file → copy2, anything else → the same "Skipping copy operation" warning copytree logs.
Pinned by test_fallback_skips_a_source_that_does_not_exist. It fails if the missing source is sent down the file branch.
…allback 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.
There was a problem hiding this comment.
Code Review Results
Reviewed: 587257c..900fe23
Files: 6
Comments: 3
Comments on lines outside the diff:
[aws_lambda_builders/actions.py:137] [ERROR_HANDLING] LinkSourceAction.execute() opens with source_files = set(os.listdir(self._source_dir)) (line 135) with no existence guard, while the CopySourceAction it replaces on this path degrades gracefully:
# utils.copytree
if not os.path.exists(source):
LOG.warning("Skipping copy operation since source %s does not exist", source)
returnThe branch this PR enables is reached whenever dependencies_dir and combine_dependencies, independently of download_dependencies. So a layer build with download_dependencies=False and a dependencies_dir that does not exist on disk (cache directory deleted while build.toml still records a matching manifest hash, or an external JSON-RPC integrator passing a not-yet-created path) changes from "warn and continue" to a hard failure: FileNotFoundError is not an ActionFailedError, so BaseWorkflow.run wraps it in WorkflowUnknownError and the whole build aborts.
Matching the existing contract keeps the two branches interchangeable:
def execute(self):
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))This also matters for the pre-existing nodejs_npm_esbuild consumer, which calls LinkSourceAction(self.dependencies_dir, self.scratch_dir) on the same "reusing dependencies" assumption.
| # 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): |
There was a problem hiding this comment.
[BUG] create_symlink_or_copy writes an absolute link target:
os.symlink(Path(source).absolute(), Path(destination).absolute())Until now that was harmless for every LinkSourceAction consumer, because the only one (nodejs_npm_esbuild) links into self.scratch_dir, which is consumed by EsbuildBundleAction in the same process and then discarded. This PR is the first to leave absolute symlinks in a persisted artifacts_dir, so the build output is now only valid while dependencies_dir remains readable at that exact absolute path.
The case worth confirming before merge is sam build --use-container. There the workflow executes inside the container, so dependencies_dir is a container path and the links it writes point at container paths; once the artifacts are transferred back to the host those links have nothing to resolve to, and neither a dereferencing tar nor a zip can recover the contents. The PR description reasons carefully about sam local but does not mention the container build path.
If is_building_layer and dependencies_dir can be combined with a container build, this branch needs to exclude that mode as well. If they cannot, it is worth stating so in the comment next to the gate, since the comment currently enumerates the sam local reasoning only and a future reader has no signal that the container path was considered.
There was a problem hiding this comment.
Checked, and the container build can't reach this branch. LambdaBuildContainer in SAM CLI builds its JSON-RPC params without a dependencies_dir key (samcli/local/docker/lambda_build_container.py). In _build_layer, the container call _build_function_on_container(...) isn't passed one either, only _build_function_in_process is. So inside the container self.dependencies_dir is None, and the if self.dependencies_dir and self.combine_dependencies guard skips both link and copy.
Agreed that a future reader should see this. The comment next to the gate now says the links are absolute, that sam build --use-container never sends a dependencies_dir, and that any other caller has to share the path. This is in fadaa49.
| @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): |
There was a problem hiding this comment.
[GENERAL] test_must_not_copy_when_symlink_succeeds does not exercise a successful os.symlink — it never reaches the call at all.
Because aws_lambda_builders.utils.Path is replaced by a MagicMock, both Path(destination).exists() and Path(destination).is_symlink() return truthy mocks, so create_symlink_or_copy takes the "already a symlink" early return:
if Path(destination).exists() and Path(destination).is_symlink():
LOG.debug("Symlink between %s and %s already exists, skipping generating symlink", source, destination)
returnBoth assertions (symlink.assert_not_called(), copytree.assert_not_called()) then pass for a reason unrelated to the test name, and the test would keep passing if the symlink-success path regressed. This is exactly the pitfall the PR already fixes one method above, where patched_path.return_value.exists.return_value = False was added with a comment explaining it.
Either make the test assert the branch it actually reaches (and rename it accordingly, e.g. test_must_skip_when_destination_is_already_a_symlink, asserting copytree is not called), or force the guard false so a real os.symlink call is verified:
def test_must_not_copy_when_symlink_succeeds(self, patched_copy_tree, pathced_os, patched_path):
patched_path.return_value.exists.return_value = False
utils.create_symlink_or_copy("source/path", "destination/path")
pathced_os.symlink.assert_called_once()
patched_copy_tree.assert_not_called()There was a problem hiding this comment.
Confirmed, the test never reached os.symlink. Replacing os.symlink with a bare return still passes it. Fixed in fadaa49 as you suggested: the test sets patched_path.return_value.exists.return_value = False and asserts symlink.assert_called_once(). With the same mutation, it now fails.
…s 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.
|
Confirmed for the finding on In fadaa49, |
| if os.path.isdir(source): | ||
| copytree(source, destination) | ||
| elif os.path.isfile(source): | ||
| os.makedirs(os.path.dirname(destination), exist_ok=True) |
There was a problem hiding this comment.
[BUG] The new copy fallback does not account for destination already existing, and a dangling symlink there is precisely what sends it down this path.
os.symlink raises FileExistsError (a subclass of OSError) when the destination path is occupied — including by a broken link, since the guard above it (Path(destination).exists() and Path(destination).is_symlink()) is False for a dangling link. So control reaches the except OSError handler with the stale link still in place, and both new branches misbehave:
os.path.isdir(source)→ copytree(source, destination) → os.path.exists(destination) is False for the dangling link → os.makedirs(destination) raises FileExistsError, which nothing catches. A hard build failure.os.path.isfile(source)→ shutil.copy2(source, destination) follows the link and creates the file at the link target, outside the destination tree, leaving destination a symlink. This is the same write-through that test_does_not_create_a_file_through_a_dangling_symlink now pins for copytree's leaf branch — the guard added there was not applied here.
It also misreports the cause: the operator sees "Symbolic link creation failed... consider enabling the necessary settings or privileges on your system" when the real problem is a leftover link, not a missing privilege.
LinkSourceAction is immune because this PR makes it unlink the destination first, but that fix lives in one caller while the shared helper stays broken for the others — LinkSinglePathAction (used by nodejs_npm and nodejs_npm_esbuild) passes a destination it has not removed, and so does copytree's maintain_symlinks branch via create_symlink_or_copy(linkto, new_destination). That branch has a related gap: when new_destination is a valid symlink into the dependencies directory, the early return keeps it and the source's own link is silently never created.
Clearing the destination in the handler fixes every caller at once and makes the LinkSourceAction workaround redundant:
except OSError as ex:
LOG.warning(
"Symbolic link creation failed, falling back to copying files instead. ...",
exc_info=ex if LOG.isEnabledFor(logging.DEBUG) else None,
)
if os.path.islink(destination):
# A leftover link at the destination is what made os.symlink raise. Copying through it
# would write outside the destination tree, and makedirs() would fail on it outright.
os.unlink(destination)
if os.path.isdir(source):
copytree(source, destination)
elif os.path.isfile(source):
...There was a problem hiding this comment.
Partly confirmed. I reproduced all three sub-claims, and one of the two "misbehaves" cases doesn't hold.
The premise is right: for a dangling link Path(destination).exists() is False, so the guard is skipped, and os.symlink raises FileExistsError: [Errno 17]. Control does reach the handler with the stale link in place.
The file branch is a real bug. With a dangling destination and a file source, shutil.copy2 followed the link and wrote the dependency to the link target, leaving destination a symlink. I hit it through a real caller with no forced error at all — LinkSinglePathAction against a dangling destination wrote the file outside the artifact directory. Same write-through that the copytree leaf branch was fixed for; you're right that the guard wasn't applied here.
The directory branch does not fail. copytree starts with _materialize_symlinked_destination(destination), which this PR added, and os.path.islink is True for a dangling link, so the link is unlinked before os.makedirs ever runs. I ran it: no exception, destination ends up a real directory with the source's contents, and nothing is written to the link target.
Fixed in d0f5f6a with the handler-level unlink you proposed, since it covers LinkSinglePathAction and the maintain_symlinks branch too rather than just the one caller. New test test_fallback_does_not_copy_through_a_dangling_destination_link drives the real FileExistsError path (no mocked os.symlink) and asserts nothing lands at the link target; it fails if the unlink is disabled.
Two notes on the rest:
- I kept the
LinkSourceActionremoval rather than treating it as redundant. It runs on the happy path, so the symlink succeeds; without it every stale entry would degrade to a full copy through this handler. It also clears stale real directories left by an earlier copying build, which the handler'sislinkcheck doesn't cover. - The last point — a valid symlink at
new_destinationmakes the early return keep it, so the source's own link is never created — reproduces, but that guard is unchanged by this PR (it's the same ondevelop). I'd rather not change the existing skip-if-already-a-symlink contract inside a perf change for the python layer path; worth its own issue.
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.
There was a problem hiding this comment.
Code Review Results
Reviewed: 587257c..d0f5f6a
Files: 6
Comments: 2
Comments on lines outside the diff:
[aws_lambda_builders/actions.py:143] [BUG] execute() iterates over source_files only, so destination entries that no longer exist in the source are never reconciled. Switching layers from copying to linking changes the consequence of that from benign to fatal.
Concretely, for the python_pip layer path the workflow is CleanUpAction(dependencies_dir) → pip install → LinkSourceAction(dependencies_dir, artifacts_dir) → CopySourceAction(source_dir, artifacts_dir). CleanUpAction is only ever applied to dependencies_dir — nothing in the workflow cleans artifacts_dir, and the PR's own test_replaces_a_real_directory_left_by_an_earlier_copying_build is premised on that directory surviving into the next build. So if a package is dropped from requirements.txt:
- before this PR:
artifacts_dir/requests/is a stale real directory — wrong content, but it still packages and runs. - after this PR:
artifacts_dir/requestsis a symlink into a target thatCleanUpActionjust deleted and pip did not recreate, andLinkSourceActionnever visits the name because it is absent fromsource_files. It stays dangling.
A dangling symlink is not a directory, so os.walk yields it in the files list and both zipfile.ZipFile.write and tarfile.add(..., dereference=True) raise FileNotFoundError on it. The dereferencing tarball this PR relies on for sam local is exactly one of those consumers, so the failure lands on the path the PR is optimizing.
Reconciling the destination after linking keeps this narrow enough to be safe for the nodejs_npm_esbuild consumer, which links into a scratch_dir that already holds copied source — only links that point into self._source_dir and no longer resolve are removed:
for source_file in source_files:
...
# A dependency dropped from the manifest is absent from source_files, so the loop above
# never visits its stale link. Left in place it dangles, and packing the artifacts
# (tar with dereference, or zip) fails on a broken link rather than skipping it.
for stale in set(os.listdir(self._dest_dir)) - source_files:
stale_path = Path(self._dest_dir, stale)
if stale_path.is_symlink() and not stale_path.exists():
if _is_within_directory(self._source_dir, os.path.realpath(stale_path)):
LOG.debug("Removing dangling symlink %s left by an earlier build", stale_path)
os.remove(stale_path)_is_within_directory already exists in aws_lambda_builders/utils.py. Worth a test alongside test_replaces_a_dangling_symlink: link two dependencies, remove one from the source directory, re-run, and assert the destination no longer contains a broken link.
| # 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): |
There was a problem hiding this comment.
[GENERAL] The safety argument for enabling this on layers covers one consumer of the layer artifacts directory but not the other. The comment reasons about the local invoke image (tarball, dereferenced) versus the function bind mount at /var/task — but artifacts_dir for a layer is also the input that sam package / sam deploy / sam sync zip and upload, and that path is not addressed here or in the PR description.
The reason it matters is that symlink handling differs by entry type. A symlinked top-level module such as six.py is yielded by os.walk in the files list and gets written into the archive with its content followed. A symlinked package directory such as requests is yielded in the dirs list, and os.walk does not descend into directory symlinks unless followlinks=True is passed. If the zipping code does not opt in, the failure mode is silent: the layer uploads with every package directory missing and only loose module files present, and the error surfaces at invoke time as ModuleNotFoundError rather than at build time.
Please confirm against the packaging code path (not just sam local) that a layer built with SAM_CLI_BETA_BUILD_PERFORMANCE deploys with complete dependencies, and record that in the validation notes the same way the local-invoke reasoning is recorded. If it does not hold, the gate needs to be narrower than is_building_layer.
1 participant
Issue #, if available: aws/aws-sam-cli#4828
Description of changes
sam build --cachedre-copies every Python dependency file on every build. For a project with a large dependency layer that is most of the build time — measured on a 185 MB / ~6,200-file layer, a warm no-change rebuild spends 0.96 s of 1.88 s re-copying the layer.This workflow can already symlink dependencies instead of copying them, but the branch is dead code:
workflow.py:119readsif False and is_experimental_build_improvements_enabled(...). #391 disabled it in 2022 because symlinked dependencies brokesam local.That reason holds for functions but not for layers, because the two reach the local invoke container by different routes:
/var/task, so a symlink pointing outside the mount dangles inside the container.sam local invoke --mount-symlinksopts into mounting the targets, butsam local start-apiandstart-lambdahave no such option — linking function dependencies really would break them.So this gates the existing
LinkSourceActiononis_building_layer, whichBaseWorkflowalready carries and whichjava_gradle/java_mavenalready branch on. Functions keep copying, and the layer path stays behindSAM_CLI_BETA_BUILD_PERFORMANCE. With SAM CLI 1.166.2 the warm rebuild goes 1.88 s -> 0.93 s and the layer build directory 184.5 MB -> ~20 KB.LinkSourceActionalso had no unit tests and two latent bugs that this second consumer hits:os.remove()raisesIsADirectoryErroron a real directory left behind by an earlier copying build, and a dangling symlink is notexists(), so it was left in place foros.symlinkto fail on and silently fall back to a full copy. Both are fixed here.Description of how you validated changes
Unit tests:
tests/unitpasses (861 tests). Coverage is 94%, meeting the--cov-fail-under 94gate.ruff checkandblack --checkare clean.New tests cover the layer/function contrast on otherwise-identical inputs, and
LinkSourceActionlinking into an empty destination, replacing a real directory, replacing a dangling symlink, and being idempotent. Both changes are mutation-checked: reverting theis_building_layergate fails 6 tests, and reverting theLinkSourceActionfix fails 2.End to end with SAM CLI against a template with 10
python3.12functions and oneAWS::Serverless::LayerVersion:sam local invokewith no flags —pandasandcryptographyimport from/opt/python. Repeated afterdocker rmiof the built image, to rule out a stale image.sam local start-lambda— returnsStatusCode 200, noFunctionError.sam deploy— the uploaded layer isCodeSize: 61936488, no zip entry is stored as a symlink, and the deployed function importspandasfrom/opt/pythonin the Lambda runtime. The layer zip's inventory (6,158 entries, 61.9 MB) is identical to a copying build's zip of the same tree..aws-sam/build/<Fn>/requests/is a directory, not a link), and plainsam buildwithout--cachedhas nodependencies_dir, so it is unaffected.Note for reviewers: the symlinks point at absolute paths under
.aws-sam/deps/<uuid>, so CI that tars.aws-sam/buildto hand to a later stage needstar -h. That is part of why this stays behind the experimental flag.Two pre-existing failures in this environment, unrelated to the change and reproduced on a clean
origin/developcheckout: 13 errors intests/functional/workflows/python_uv(uvnot on PATH) and 6 failures intests/functional/test_cli.py::TestCliWithHelloWorkflow.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.