Merge 3575c04de7 into 5b78ee8dff

1 day ago · df8757a6c8
parent 5b78ee8dff 3575c04de7
commit df8757a6c8
4 changed files with 250 additions and 2 deletions
--- a/internal/fileutil/fileutil_unix.go
+++ b/internal/fileutil/fileutil_unix.go
@ -0,0 +1,45 @@
+//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"
+)
+
+// LockedAtomicWriteFile atomically writes a file to disk.
+//
+// On non-Windows platforms, this is a simple wrapper around AtomicWriteFile
+// since concurrent file access errors are not an issue on Unix-like systems.
+//
+// Returns true if the file was written, false if it already existed.
+func LockedAtomicWriteFile(filename string, reader io.Reader, mode os.FileMode) (bool, error) {
+	// Check if the file already exists
+	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
+}
--- a/internal/fileutil/fileutil_windows.go
+++ b/internal/fileutil/fileutil_windows.go
@ -0,0 +1,66 @@
+//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"
+)
+
+// 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
+}
--- 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
 		}

--- a/pkg/downloader/chart_downloader_windows_test.go
+++ 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)
+		}
+	}
+}