From 76eb37c01aaece271343039f44d7803017dd5c81 Mon Sep 17 00:00:00 2001 From: Orgad Shaneh Date: Mon, 11 Aug 2025 09:08:43 +0300 Subject: [PATCH] fix(downloader): safely handle concurrent file writes on Windows 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 --- internal/fileutil/fileutil_test.go | 26 ++++ internal/fileutil/fileutil_unix.go | 32 +++++ internal/fileutil/fileutil_windows.go | 54 ++++++++ pkg/downloader/chart_downloader.go | 10 +- .../chart_downloader_windows_test.go | 131 ++++++++++++++++++ 5 files changed, 251 insertions(+), 2 deletions(-) create mode 100644 internal/fileutil/fileutil_unix.go create mode 100644 internal/fileutil/fileutil_windows.go create mode 100644 pkg/downloader/chart_downloader_windows_test.go diff --git a/internal/fileutil/fileutil_test.go b/internal/fileutil/fileutil_test.go index 881fbb49d..71fcae177 100644 --- a/internal/fileutil/fileutil_test.go +++ b/internal/fileutil/fileutil_test.go @@ -119,3 +119,29 @@ func TestAtomicWriteFile_LargeContent(t *testing.T) { t.Fatalf("expected large content to match, got different length: %d vs %d", len(largeContent), len(got)) } } + +// TestPlatformAtomicWriteFile_OverwritesExisting verifies that the platform +// helper replaces existing files instead of silently skipping them. +func TestPlatformAtomicWriteFile_OverwritesExisting(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "overwrite_test") + + first := bytes.NewReader([]byte("first")) + if err := PlatformAtomicWriteFile(path, first, 0644); err != nil { + t.Fatalf("first write failed: %v", err) + } + + second := bytes.NewReader([]byte("second")) + if err := PlatformAtomicWriteFile(path, second, 0644); err != nil { + t.Fatalf("second write failed: %v", err) + } + + contents, err := os.ReadFile(path) + if err != nil { + t.Fatalf("failed reading result: %v", err) + } + + if string(contents) != "second" { + t.Fatalf("expected file to be overwritten, got %q", string(contents)) + } +} diff --git a/internal/fileutil/fileutil_unix.go b/internal/fileutil/fileutil_unix.go new file mode 100644 index 000000000..bbacb10bf --- /dev/null +++ b/internal/fileutil/fileutil_unix.go @@ -0,0 +1,32 @@ +//go:build !windows + +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fileutil + +import ( + "io" + "os" +) + +// PlatformAtomicWriteFile atomically writes a file to disk. +// +// On non-Windows platforms we don't need extra coordination, so this simply +// delegates to AtomicWriteFile to preserve the existing overwrite behaviour. +func PlatformAtomicWriteFile(filename string, reader io.Reader, mode os.FileMode) error { + return AtomicWriteFile(filename, reader, mode) +} diff --git a/internal/fileutil/fileutil_windows.go b/internal/fileutil/fileutil_windows.go new file mode 100644 index 000000000..179237860 --- /dev/null +++ b/internal/fileutil/fileutil_windows.go @@ -0,0 +1,54 @@ +//go:build windows + +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fileutil + +import ( + "io" + "os" + + "github.com/gofrs/flock" +) + +// PlatformAtomicWriteFile atomically writes a file to disk with file locking to +// prevent concurrent writes. This is particularly useful on Windows where +// concurrent writes to the same file can cause "Access Denied" errors. +// +// The function acquires a lock on the target file and performs an atomic write, +// preserving the existing behaviour of overwriting any previous content once +// the lock is obtained. +func PlatformAtomicWriteFile(filename string, reader io.Reader, mode os.FileMode) error { + // Use a separate lock file to coordinate access between processes + // We cannot lock the target file directly as it would prevent the atomic rename + lockFileName := filename + ".lock" + fileLock := flock.New(lockFileName) + + // Lock() ensures serialized access - if another process is writing, this will wait + if err := fileLock.Lock(); err != nil { + return err + } + defer func() { + fileLock.Unlock() + // Clean up the lock file + // Ignore errors as the file might not exist or be in use by another process + os.Remove(lockFileName) + }() + + // Perform the atomic write while holding the lock + return AtomicWriteFile(filename, reader, mode) +} diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index ee4f8abe3..ea178dfe9 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -156,7 +156,11 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven } destfile := filepath.Join(dest, name) - if err := fileutil.AtomicWriteFile(destfile, data, 0644); err != nil { + + // Use PlatformAtomicWriteFile to handle platform-specific concurrency concerns + // (Windows requires locking to avoid "Access Denied" errors when multiple + // processes write the same file) + if err := fileutil.PlatformAtomicWriteFile(destfile, data, 0644); err != nil { return destfile, nil, err } @@ -186,7 +190,9 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven } } provfile := destfile + ".prov" - if err := fileutil.AtomicWriteFile(provfile, body, 0644); err != nil { + + // Use PlatformAtomicWriteFile for the provenance file as well + if err := fileutil.PlatformAtomicWriteFile(provfile, body, 0644); err != nil { return destfile, nil, err } diff --git a/pkg/downloader/chart_downloader_windows_test.go b/pkg/downloader/chart_downloader_windows_test.go new file mode 100644 index 000000000..732416701 --- /dev/null +++ b/pkg/downloader/chart_downloader_windows_test.go @@ -0,0 +1,131 @@ +//go:build windows + +/* +Copyright The Helm Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package downloader + +import ( + "os" + "path/filepath" + "sync" + "testing" + + "helm.sh/helm/v4/pkg/cli" + "helm.sh/helm/v4/pkg/getter" + "helm.sh/helm/v4/pkg/repo/v1/repotest" +) + +// TestParallelDownloadTo tests that parallel downloads to the same file +// don't cause "Access Denied" errors on Windows. This test is Windows-specific +// because the file locking behavior is only needed on Windows. +func TestParallelDownloadTo(t *testing.T) { + // Set up a simple test server with a chart + srv := repotest.NewTempServer(t, repotest.WithChartSourceGlob("testdata/*.tgz")) + defer srv.Stop() + + if err := srv.CreateIndex(); err != nil { + t.Fatal(err) + } + + dest := t.TempDir() + cacheDir := t.TempDir() + + c := ChartDownloader{ + Out: os.Stderr, + RepositoryConfig: repoConfig, + RepositoryCache: repoCache, + ContentCache: cacheDir, + Cache: &DiskCache{Root: cacheDir}, + Getters: getter.All(&cli.EnvSettings{ + RepositoryConfig: repoConfig, + RepositoryCache: repoCache, + ContentCache: cacheDir, + }), + } + + // Use a direct URL to bypass repository lookup + chartURL := srv.URL() + "/local-subchart-0.1.0.tgz" + + // Number of parallel downloads to attempt + numDownloads := 10 + var wg sync.WaitGroup + errors := make([]error, numDownloads) + + // Launch multiple goroutines to download the same chart simultaneously + for i := 0; i < numDownloads; i++ { + wg.Add(1) + go func(index int) { + defer wg.Done() + _, _, err := c.DownloadTo(chartURL, "", dest) + errors[index] = err + }(i) + } + + wg.Wait() + + // Check if any download failed + failedCount := 0 + for i, err := range errors { + if err != nil { + t.Logf("Download %d failed: %v", i, err) + failedCount++ + } + } + + // With the file locking fix, all parallel downloads should succeed + if failedCount > 0 { + t.Errorf("Parallel downloads failed: %d out of %d downloads failed due to concurrent file access", failedCount, numDownloads) + } + + // Verify the file exists and is valid + expectedFile := filepath.Join(dest, "local-subchart-0.1.0.tgz") + info, err := os.Stat(expectedFile) + if err != nil { + t.Errorf("Expected file %s does not exist: %v", expectedFile, err) + } else { + // Verify the file is not empty + if info.Size() == 0 { + t.Errorf("Downloaded file %s is empty (0 bytes)", expectedFile) + } + + // Verify the file has the expected size (should match the source file) + sourceFile := "testdata/local-subchart-0.1.0.tgz" + sourceInfo, err := os.Stat(sourceFile) + if err == nil && info.Size() != sourceInfo.Size() { + t.Errorf("Downloaded file size (%d bytes) doesn't match source file size (%d bytes)", + info.Size(), sourceInfo.Size()) + } + + // Verify it's a valid tar.gz file by checking the magic bytes + file, err := os.Open(expectedFile) + if err == nil { + defer file.Close() + // gzip magic bytes are 0x1f 0x8b + magic := make([]byte, 2) + if n, err := file.Read(magic); err == nil && n == 2 { + if magic[0] != 0x1f || magic[1] != 0x8b { + t.Errorf("Downloaded file is not a valid gzip file (magic bytes: %x)", magic) + } + } + } + + // Verify no lock file was left behind + lockFile := expectedFile + ".lock" + if _, err := os.Stat(lockFile); err == nil { + t.Errorf("Lock file %s was not cleaned up", lockFile) + } + } +}