fix(downloader): prevent concurrent file access errors on Windows

When DownloadTo runs in parallel for the same ref and version, both processes try to write the same target file, causing "Access Denied" errors on Windows. This change refactors the file locking mechanism based on maintainer feedback: - Add LockedAtomicWriteFile to internal/fileutil package to encapsulate locking logic - Use the new function for both chart and provenance files - Lock is cross-process safe and automatically released on process exit - Files are only written if they don't already exist (avoiding duplicate work) The TestParallelDownloadTo test verifies the fix works correctly. Fixes #31633 Signed-off-by: Orgad Shaneh <orgad.shaneh@audiocodes.com>
6 months ago · 3575c04de7
parent f928025cdb
commit 3575c04de7
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)
+		}
+	}
+}