Skip to content

fix: include lock files in dependency hash for cache invalidation #8818

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dcabib wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix: include lock files in dependency hash for cache invalidation #8818

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df95413
fix: include lock files in dependency hash for cache invalidation
dcabib Mar 17, 2026
0d47042
Merge branch 'develop' into fix/issue-8242-include-lockfiles-in-cache…
vicheey Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 37 additions & 3 deletions samcli/lib/build/dependency_hash_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,27 @@
"""Utility Class for Getting Function or Layer Manifest Dependency Hashes"""

import hashlib
import pathlib
from typing import Any, Optional

from samcli.lib.build.workflow_config import get_workflow_config
from samcli.lib.utils.hash import file_checksum


# Mapping of dependency managers to their lock file names
LOCK_FILE_MAPPING = {
"npm": "package-lock.json",

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BUG] LOCK_FILE_MAPPING only maps npm/npm-esbuild to package-lock.json, but SAM classifies every Node.js project under dependency_manager="npm" regardless of whether the user actually uses npm, yarn, or pnpm (there is no separate yarn/pnpm workflow in samcli/lib/build/workflows.py). A project with a yarn.lock or pnpm-lock.yaml and no package-lock.json will not get cache invalidation when the lock file changes — exactly the bug this PR is trying to fix. @seshubaws raised this and it has not been addressed.

Since only one lock file name is stored per manager, consider either supporting a list of candidate lock files (checking each and hashing whichever exists) or falling back through the known Node lock file names. For example:

LOCK_FILE_MAPPING = {
   "npm": ["package-lock.json", "yarn.lock", "pnpm-lock.yaml"],
   "npm-esbuild": ["package-lock.json", "yarn.lock", "pnpm-lock.yaml"],
   ...
}

and hash whichever candidate exists.

"npm-esbuild": "package-lock.json",

@seshubaws seshubaws Mar 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we not need to add yarn.lock and pnpm-lock.yaml?

"bundler": "Gemfile.lock",
"gradle": "gradle.lockfile",
"cli-package": "packages.lock.json",
"modules": "go.sum",
"cargo": "Cargo.lock",
"uv": "uv.lock",
"poetry": "poetry.lock",
}


# TODO Expand this class to hash specific sections of the manifest
class DependencyHashGenerator:
_code_uri: str
Expand Down Expand Up @@ -50,15 +65,17 @@ def __init__(
self._hash = None

def _calculate_dependency_hash(self) -> Optional[str]:
"""Calculate the manifest file hash
"""Calculate the manifest file hash, including lock file if applicable

Returns
-------
Optional[str]
Returns manifest hash. If manifest does not exist or not supported, None will be returned.
Returns combined hash of manifest and lock file (if present).
If manifest does not exist or not supported, None will be returned.
"""
if self._manifest_path_override:
manifest_file = self._manifest_path_override
config = None

@seshubaws seshubaws Mar 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we not wanting to add the config to the hash if there is a different manifest path? Can the lock file not still be resolved relative to the override path's directory?

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BUG] Setting config = None in the manifest_path_override branch guarantees the lock-file logic below (if config and config.dependency_manager in LOCK_FILE_MAPPING) is skipped entirely. This was raised by @seshubaws and has not been addressed: the lock file can still be resolved relative to self._code_dir even when the manifest path is overridden, and skipping it defeats the whole point of the PR for users with overrides.

There are two further consequences of this design:

  • PYTHON_UV_CONFIG in samcli/lib/build/workflows.py has manifest_name=None, so uv projects can only produce a hash via manifest_path_override. Combined with this branch, the new "uv": "uv.lock" mapping is unreachable.
  • There is no poetry entry in ALL_CONFIGS, so config.dependency_manager is never "poetry" and "poetry": "poetry.lock" is also unreachable.

Consider resolving the workflow config (or at least the dependency manager) even when an override is present, e.g.:

if self._manifest_path_override:
   manifest_file = self._manifest_path_override
else:
   manifest_file = None
config = get_workflow_config(self._runtime, self._code_dir, self._base_dir)
if manifest_file is None:
   manifest_file = config.manifest_name

so the lock file is honored in every case where a dependency manager is known.

else:
config = get_workflow_config(self._runtime, self._code_dir, self._base_dir)
manifest_file = config.manifest_name
Expand All @@ -70,7 +87,24 @@ def _calculate_dependency_hash(self) -> Optional[str]:
if not manifest_path.is_file():
return None

return file_checksum(str(manifest_path), hash_generator=self._hash_generator)
manifest_hash = file_checksum(str(manifest_path), hash_generator=self._hash_generator)

# Check if there's a lock file for this dependency manager
if config and config.dependency_manager in LOCK_FILE_MAPPING:

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[GENERAL] @seshubaws explicitly asked for unit tests covering "if there is a lock file present and not present, and if there is an unknown dep manager." The diff does not modify tests/unit/lib/build_module/test_dependency_hash_generator.py, and the PR's own test-plan checkbox for running/adding unit tests is unchecked.

Given the number of code paths introduced (lock file present, lock file absent, dep manager not in LOCK_FILE_MAPPING, manifest_path_override with and without lock file, and the combined-hash construction), regressions here are easy to miss. Please add tests that:

  • Exercise the case where the manifest exists but no lock file exists (should return the manifest hash unchanged, preserving previous caches).
  • Exercise the case where both manifest and lock file exist (verify a distinct combined hash and that both files are read).
  • Exercise a runtime whose dependency_manager is not in LOCK_FILE_MAPPING (e.g. maven) — no lock lookup should occur.
  • Exercise the manifest_path_override path (see comment Bundling a new version of GoFormation. #1 — behavior here should be defined and tested).

lock_file_name = LOCK_FILE_MAPPING[config.dependency_manager]
lock_file_path = pathlib.Path(self._code_dir, lock_file_name).resolve()

# If lock file exists, combine hashes
if lock_file_path.is_file():
lock_file_hash = file_checksum(str(lock_file_path), hash_generator=self._hash_generator)

# Combine both hashes into a single hash
combined = f"{manifest_hash}:{lock_file_hash}"
hasher = self._hash_generator() if self._hash_generator else hashlib.md5()

@aws-sam-tooling-bot aws-sam-tooling-bot Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[BUG] hasher = self._hash_generator() if self._hash_generator else hashlib.md5() treats _hash_generator as a factory/class, but everywhere else in the codebase it is an already-constructed hasher instance:

  • The class docstring documents hash_generator as hashlib.md5()/hashlib.sha256() (i.e. instances).
  • file_checksum in samcli/lib/utils/hash.py calls hash_generator.update(buf) directly on the value, confirming it is an instance.

Hash objects returned by hashlib.md5() are not callable, so if any caller ever passes a non-None hash_generator, this line raises TypeError: '_hashlib.HASH' object is not callable.

There is a related latent bug on lines 90/99: file_checksum does not reset or copy the hasher it receives; it calls update() on it in place. So when self._hash_generator is non-None, the second file_checksum call inherits the manifest state and lock_file_hash ends up being a hash of manifest+lock rather than lock alone.

Suggested fix — always construct fresh hashers locally (and use the codebase's _get_md5 helper for consistency):

manifest_hash = file_checksum(str(manifest_path), hash_generator=self._hash_generator)

if config and config.dependency_manager in LOCK_FILE_MAPPING:
   lock_file_path = pathlib.Path(self._code_dir, LOCK_FILE_MAPPING[config.dependency_manager]).resolve()
   if lock_file_path.is_file():
       lock_file_hash = file_checksum(str(lock_file_path))  # fresh hasher
       hasher = hashlib.md5(usedforsecurity=False)
       hasher.update(f"{manifest_hash}:{lock_file_hash}".encode("utf-8"))
       return hasher.hexdigest()

hasher.update(combined.encode("utf-8"))
return hasher.hexdigest()

return manifest_hash

@property
def hash(self) -> Optional[str]:
Expand Down