2 warnings found
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.