diff --git a/internal/fileutil/fileutil.go b/internal/fileutil/fileutil.go index 39e0e330f..d4dd16282 100644 --- a/internal/fileutil/fileutil.go +++ b/internal/fileutil/fileutil.go @@ -21,6 +21,8 @@ import ( "os" "path/filepath" + "github.com/gofrs/flock" + "helm.sh/helm/v4/internal/third_party/dep/fs" ) @@ -48,3 +50,43 @@ func AtomicWriteFile(filename string, reader io.Reader, mode os.FileMode) error return fs.RenameWithFallback(tempName, filename) } + +// LockedAtomicWriteFile 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, checks if it already exists, +// and only writes if it doesn't exist. This prevents duplicate work when multiple +// processes try to write the same file simultaneously. +// +// Returns true if the file was written, false if it already existed. +func LockedAtomicWriteFile(filename string, reader io.Reader, mode os.FileMode) (bool, 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 false, 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) + }() + + // Check if the file already exists (another process might have already written it) + if _, err := os.Stat(filename); err == nil { + // File already exists, skip writing + return false, nil + } + + // File doesn't exist, write it atomically + if err := AtomicWriteFile(filename, reader, mode); err != nil { + return false, err + } + + return true, nil +} diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 00c8c56e8..0e04e004c 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 LockedAtomicWriteFile to handle concurrent writes safely + // This prevents "Access Denied" errors on Windows when multiple processes + // try to write to the same file simultaneously + if _, err := fileutil.LockedAtomicWriteFile(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 LockedAtomicWriteFile for the provenance file as well + if _, err := fileutil.LockedAtomicWriteFile(provfile, body, 0644); err != nil { return destfile, nil, err } diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index 4349ecef9..5a45900c8 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -20,6 +20,7 @@ import ( "encoding/hex" "os" "path/filepath" + "sync" "testing" "github.com/stretchr/testify/require" @@ -485,3 +486,102 @@ func TestDownloadToCache(t *testing.T) { c.Keyring = "" }) } + +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) + } + } +}