fix(downloader): prevent concurrent file access errors on Windows#31128
fix(downloader): prevent concurrent file access errors on Windows#31128
Conversation
There was a problem hiding this comment.
You might want to wait for feedback on other maintainers, but maybe make a LockedAtomicWirteFile to fileutils instead of this and definitely don't use a goto in that.
Thank you! Done. |
|
ping? |
1285e06 to
f678f68
Compare
|
ping |
|
Please review this. It hits me all the time when working on Windows. See hashicorp/terraform-provider-helm#1683. |
5bb0120 to
436aaf1
Compare
|
ping? |
|
Anybody? @gjenkins8 @robertsirc @scottrigby? |
|
Especially with the Helm 4.0 announcement, it would be great to get this in so that the basics of Helm continue to work solidly for everyone. I hit this issue multiple times per day. I noticed it's linked to an issue that appears to be auto-closed. I wonder if a new issue being opened and linked to this would get it the attention it deserves? |
|
Closes: #31633 |
Updated the commit message and the PR description. |
|
What is the process for this PR to be reviewed and approved by one additional person? Asking because there was a comment on the issue that the PR is stale, but it's already received one approval and is just pending (presumably) 1 more approval. Asking as the non-author, because I have hit this issue 4 times now during attempted Helm deployments this morning and am very much looking forward to the symptom being resolved. :) |
|
I just gave up and switched to WSL. It works much better. |
I'd consider that except I'm working with a team whose understanding and familiarity with WSL varies. Hopefully this PR can get the one additional approval it needs and get things resolved. 🤞 |
|
Hi all -- I see @gjenkins8 reviewed this and added a comment and @orgads responded in a few hours. That was 3 weeks ago. Coming back to this thread because I was once again blocked on a deployment and had to repeat it 5 times due to the locking issue. Is there a way we can come to agreement on this? The point @orgads raises makes sense to be in general, and it seems that all the environments would benefit from the change. However, I'm not an expert in this specific stack. |
|
Rebased and separated Windows/non-Windows. |
There was a problem hiding this comment.
This breaks existing behavior. It will leave stale chart/provenance data if a chart is republished with the same version/name
When multiple processes try to download the same chart version concurrently (e.g., via Terraform), they can race to write the destination file. On Windows, this results in 'Access Denied' errors because the file cannot be renamed while another process has a handle to the destination. This commit introduces 'PlatformAtomicWriteFile' to the fileutil package. On Unix-like systems, it simply delegates to AtomicWriteFile, maintaining existing behavior. On Windows, it coordinates writes using a lock file (.lock). It acquires the lock, then performs the atomic write. Crucially, this implementation ensures that existing files are overwritten (rather than skipped). This ensures that if a chart is republished with the same version, the local cache is correctly updated, preventing stale data issues. Fixes helm#31633 Signed-off-by: Orgad Shaneh <orgad.shaneh@audiocodes.com>
|
Thank you @joejulian and @TerryHowe for giving this the reviews it needs. Thank you to @orgads for continuing to work on this and get it across the finish line 🎉 It seems like there's nothing blocking this at this point from being merged and getting into the release cycle. I and other Helm users on Windows will be grateful for that! |
What this PR does / why we need it:
When multiple processes try to download the same chart version concurrently (e.g., via Terraform), they can race to write the destination file. On Windows, this results in 'Access Denied' errors because the file cannot be renamed while another process has a handle to the destination.
This commit introduces 'PlatformAtomicWriteFile' to the fileutil package. On Unix-like systems, it simply delegates to AtomicWriteFile, maintaining existing behavior. On Windows, it coordinates writes using a lock file (.lock). It acquires the lock, then performs the atomic write.
Crucially, this implementation ensures that existing files are overwritten (rather than skipped). This ensures that if a chart is republished with the same version, the local cache is correctly updated, preventing stale data issues.
Fixes #31633
Signed-off-by: Orgad Shaneh orgad.shaneh@audiocodes.com
Special notes for your reviewer:
If applicable:
docs neededlabel should be applied if so)