Repository navigation
fix(lambda-nodejs): prevent pnpm store and workspace state timestamps from changing asset hash (#39550) - #39557
wasim-builds wants to merge 1 commit into
Conversation
… from changing asset hash (aws#39550)
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
|
Automated review A maintainer will still review this — treat the notes below as a starting point. 📌 Commit This PR fixes non-deterministic The change is narrowly scoped and pnpm-guarded; the two cleanup paths are mirrored correctly and their unit assertions track both. The one gap is that the Dockerfile store-redirect — the primary root cause — is exercised by no test in this PR. Otherwise the change is clean, with the acknowledged one-time asset-hash churn being the fix's intended effect rather than a regression. 🔴 0 blocking · 🟡 1 recommended · ⚪ 0 optional Files with findings (1)
Generated automatically. React 👍 or 👎 to tell us whether this review helped, so we can improve these reviews. |
There was a problem hiding this comment.
See the review summary comment for the overview; the notes below are inline.
| chmod -R 777 /tmp/pnpm-cache | ||
| ENV npm_config_store_dir=/tmp/pnpm-cache |
There was a problem hiding this comment.
🟡 Recommended — The load-bearing half of this fix — redirecting pnpm's content-addressable store out of the bind-mounted /asset-output via build-image ENV npm_config_store_dir / pnpm_config_store_dir — has no test that would fail if it regressed. The unit tests only assert the emitted shell command for the .pnpm-workspace-state-v1.json cleanup; none reference the Dockerfile or store-dir, and a Dockerfile ENV change is not observable in a synthesized-template assertion. The module's existing integ.dependencies-pnpm.ts (pnpm + nodeModules + forceDockerBundling) exercises exactly this path, but its committed AssetHashType.OUTPUT snapshot was not regenerated, so nothing proves the store no longer leaks into the asset or that the hash is now deterministic (INTEGRATION_TESTS.md#when-are-integration-tests-required).
Suggested change: Regenerate integ.dependencies-pnpm.js.snapshot via a real deploy (yarn integ --update-on-failed integ.dependencies-pnpm.js) so the asset hash reflects the de-leaked output and the pnpm Docker path is re-validated end-to-end; or, if maintainers judge regeneration unnecessary, apply the integ-test exemption label with a note explaining why.
blacksoxx
left a comment
There was a problem hiding this comment.
Read the diff and the issue; I could not build the package here, so these are reading notes.
The analysis in #39550 matches the Dockerfile: pnpm config --global set only wrote root's rc file, and the bundling container runs as the host uid with HOME=/, so the store silently fell back next to the project under /asset-output. Moving the setting to ENV makes it reach every user, pnpm has read npm_config_* for as long as the pinned 7.33.7 has existed, and the pnpm_config_* twin covers the newer prefix. Removing .pnpm-workspace-state-v1.json next to the existing .modules.yaml cleanup, on both the Docker and the local path with the two tests extended, closes the second timestamp source from the issue.
One thing worth a sentence in the description: with the store back in /tmp/pnpm-cache, existing pnpm users lose the duplicated store directory from their asset, so they will see one asset hash change on the next deploy, after which it stays stable. That is the intended effect, but it reads as a surprise if nobody says it. Nothing blocking from my side.
Issue # (if applicable)
Closes #39550.
Reason for this change
When using
NodejsFunctionwith Docker bundling andpnpm, the synthesized asset contains the pnpm store directory (.pnpm-store/). Because the store contains files with timestamps (*-index.jsonwithcheckedAtin pnpm 7 and 10,index.dbin pnpm 11+), the asset hash (AssetHashType.OUTPUT) changes on every synth even when neither source code nor dependencies have changed. Every deploy uploads a new asset and updates the function, and doubles the asset archive size.Root cause:
Dockerfilepreviously ranpnpm config --global set store-dir /tmp/pnpm-cache. This wrote to root's config (/root/.config/pnpm/rcor/root/.config/pnpm/config.yaml). When the container runs as a non-root host user (-u <uid>:<gid>withHOME=/), that user cannot read root's config. With no store directory configured, pnpm defaults to placing the store on the same device as the project, which is the bind-mounted/asset-outputdirectory.node_modules/.pnpm-workspace-state-v1.jsonwith alastValidatedTimestampproperty on every install. While CDK already cleaned upnode_modules/.modules.yaml, this file was left in the output, changing the asset hash on every synth for both Docker and local bundling.Description of changes
pnpm config --global set store-dir /tmp/pnpm-cachewith universal environment variables:/tmp/pnpm-cacheinstead of/asset-output.node_modules/.pnpm-workspace-state-v1.jsonalongsidenode_modules/.modules.yamlin both Docker bundling (dockerFileOps) and local bundling (localFileOps).Description of how you validated changes
packages/aws-cdk-lib/aws-lambda-nodejs/test/bundling.test.ts:/asset-output/node_modules/.pnpm-workspace-state-v1.json./outdir/node_modules/.pnpm-workspace-state-v1.json.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.