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) + } + } +}