-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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.
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
fix: half-written asset zips can be uploaded if process is interrupted #22393
Conversation
Here's a possible scenario that can lead to "Uploaded file must be a non-empty zip": - Bundling starts - A partial zip file is written - The process is interrupted (`^C` or similar) while writing. - On next run, the destination file already exists, is taken to be complete, and uploaded. In this fix, we'll write the zip to a temporary file and atomically rename it into place once finished, thereby avoiding this possible scenario. Attempts to fix #18459.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
} | ||
|
||
/** | ||
* Rename the file to the target location, taking into account that we may see EPERM on Windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
How about checking the file size in the cache against aws-cdk/packages/cdk-assets/lib/private/handlers/files.ts Lines 140 to 143 in e0c0b56
|
That will only catch the case if it's actually empty, not if we succesfully wrote 10MB and yet still failed because were supposed to write 100MB. |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
aws#22393) Here's a possible scenario that can lead to "Uploaded file must be a non-empty zip": - Bundling starts - A partial zip file is written - The process is interrupted (`^C` or similar) while writing. - On next run, the destination file already exists, is taken to be complete, and uploaded. In this fix, we'll write the zip to a temporary file and atomically rename it into place once finished, thereby avoiding this possible scenario. Attempts to fix aws#18459. I don't have tests for this, I don't know how to write them properly. To my mind, if the code looks good and we pass integration tests, that should be sufficient. Add a global asset salting mechanism so that we know all asset hashes are always unique. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws#22393) Here's a possible scenario that can lead to "Uploaded file must be a non-empty zip": - Bundling starts - A partial zip file is written - The process is interrupted (`^C` or similar) while writing. - On next run, the destination file already exists, is taken to be complete, and uploaded. In this fix, we'll write the zip to a temporary file and atomically rename it into place once finished, thereby avoiding this possible scenario. Attempts to fix aws#18459. I don't have tests for this, I don't know how to write them properly. To my mind, if the code looks good and we pass integration tests, that should be sufficient. Add a global asset salting mechanism so that we know all asset hashes are always unique. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
I'm pretty sure that the correct fix for the issue this debug text was attempting to help debug was delivered in #22393 Get rid of the warning message, it only serves to confuse.
I'm pretty sure that the correct fix for the issue this debug text was attempting to help debug was delivered in #22393 Get rid of the warning message, it only serves to confuse. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
I'm pretty sure that the correct fix for the issue this debug text was attempting to help debug was delivered in aws#22393 Get rid of the warning message, it only serves to confuse. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
I'm pretty sure that the correct fix for the issue this debug text was attempting to help debug was delivered in aws#22393 Get rid of the warning message, it only serves to confuse. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
I'm pretty sure that the correct fix for the issue this debug text was attempting to help debug was delivered in aws#22393 Get rid of the warning message, it only serves to confuse. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Here's a possible scenario that can lead to "Uploaded file must be a non-empty zip":
^C
or similar) while writing.In this fix, we'll write the zip to a temporary file and atomically rename it into place once finished, thereby avoiding this possible scenario.
Attempts to fix #18459.
I don't have tests for this, I don't know how to write them properly. To my mind, if the code looks good and we pass integration tests, that should be sufficient. Add a global asset salting mechanism so that we know all asset hashes are always unique.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license