Merge 436aaf100d into 93c02523bb

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

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
 		}
 

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