fix: eliminate race condition in rewriteChartDependencies by yxxhero · Pull Request #2541 · helmfile/helmfile

yxxhero · 2026-04-17T13:23:56Z

Summary

rewriteChartDependencies previously modified the original Chart.yaml in-place to rewrite relative file:// dependency paths to absolute paths. When multiple releases referenced the same local chart, concurrent goroutines would race on the shared file, causing corruption (Error: validation: chart.metadata.name is required).

Changes

pkg/state/state.go: Changed rewriteChartDependencies to copy the chart to a temp directory via st.fs.CopyDir and rewrite the copy's Chart.yaml instead of the original. The function signature changed from (func(), error) to (string, func(), error) — the returned string is the (possibly new) chart path to use. The per-chart mutex is no longer needed in this function.
pkg/state/state.go: Updated processChartification caller to use the returned chart path.
pkg/state/chart_dependencies_rewrite_test.go: Updated all tests for the new signature; renamed CleanupRestoresOriginal to OriginalNotModified to verify the original file is never touched.

Testing

All TestRewriteChartDependencies* tests pass
TestRewriteChartDependencies_RaceCondition passes with 10 concurrent goroutines operating on the same chart path
go vet and go build pass cleanly

aditmeno

LGTM

Copilot

Pull request overview

This PR fixes a concurrency bug in Helmfile’s chart preparation flow by ensuring rewriteChartDependencies no longer edits a shared local chart’s Chart.yaml in-place. Instead, when dependency path rewrites are needed, it operates on a per-call temporary copy of the chart directory, preventing concurrent goroutines from corrupting the source chart metadata (as reported in #2502).

Changes:

Update rewriteChartDependencies to copy the chart directory to a temp location and rewrite the copy’s Chart.yaml, returning the (possibly new) chart path plus a cleanup function.
Update processChartification to use the returned rewritten chart path.
Update/rename tests to validate the original chart is not modified and add a concurrency test that exercises the prior race scenario.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
`pkg/state/state.go`	Avoids in-place mutation by rewriting `Chart.yaml` only in a temporary copied chart directory; updates caller to use returned path.
`pkg/state/chart_dependencies_rewrite_test.go`	Adjusts tests for new signature and adds coverage ensuring the original `Chart.yaml` remains unchanged under concurrent use.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copilot · 2026-04-17T13:42:38Z

-	if chartMeta.Dependencies[0].Name != "dep1" {
-		t.Errorf("expected dependency name 'dep1', got %q", chartMeta.Dependencies[0].Name)
+		t.Errorf("expected original chart name 'test-chart', got %q", chartMeta.Name)
 	}


chartMeta.Dependencies[0] is accessed without checking len(chartMeta.Dependencies), which can panic (e.g., if YAML unmarshals but the dependencies field is missing/empty due to corruption). Add an explicit length check (and ideally verify the dependency name) before indexing so the test fails with a clear assertion instead of an out-of-range panic.

Suggested change

}

}

if len(chartMeta.Dependencies) == 0 {

t.Fatalf("expected original Chart.yaml to contain at least one dependency, got %d", len(chartMeta.Dependencies))

}

const wantDependencyName = "relative-chart"

if chartMeta.Dependencies[0].Name != wantDependencyName {

t.Errorf("expected first dependency name %q, got %q", wantDependencyName, chartMeta.Dependencies[0].Name)

}

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copilot · 2026-04-17T15:06:20Z

+		rewrittenPath, restoreChart, err := st.rewriteChartDependencies(chartPath)
 		if err != nil {
 			return "", false, fmt.Errorf("failed to rewrite chart dependencies: %w", err)
 		}
+		chartPath = rewrittenPath
 		defer restoreChart()


The second return value from rewriteChartDependencies is now a cleanup that removes the temporary chart copy, but the local variable is still named restoreChart. Renaming it (and the deferred call) to something like cleanupTempChart/cleanupChartCopy would better reflect the new behavior and avoid confusion with the previous “restore original Chart.yaml” semantics.

Suggested change

rewrittenPath, restoreChart, err := st.rewriteChartDependencies(chartPath)

if err != nil {

return "", false, fmt.Errorf("failed to rewrite chart dependencies: %w", err)

}

chartPath = rewrittenPath

defer restoreChart()

rewrittenPath, cleanupTempChart, err := st.rewriteChartDependencies(chartPath)

if err != nil {

return "", false, fmt.Errorf("failed to rewrite chart dependencies: %w", err)

}

chartPath = rewrittenPath

defer cleanupTempChart()

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Copilot · 2026-04-17T15:29:38Z

+	tempDir, err := os.MkdirTemp("", "chart-deps-rewrite-*")
+	if err != nil {
+		return chartPath, func() {}, fmt.Errorf("failed to create temp directory for chart rewrite: %w", err)
 	}

-	if err := os.WriteFile(chartYamlPath, updatedData, 0644); err != nil {
-		// File may be truncated/partially written; restore original content before releasing the lock.
-		if restoreErr := os.WriteFile(chartYamlPath, originalContent, 0644); restoreErr != nil {
-			st.logger.Warnf("Failed to restore Chart.yaml at %s after write error (%v): %v", chartYamlPath, err, restoreErr)
+	if err := st.fs.CopyDir(chartPath, tempDir); err != nil {
+		if removeErr := os.RemoveAll(tempDir); removeErr != nil {
+			st.logger.Warnf("Failed to remove temp chart directory %s: %v", tempDir, removeErr)
 		}
-		mu.Unlock()
-		return func() {}, fmt.Errorf("failed to write Chart.yaml: %w", err)
+		return chartPath, func() {}, fmt.Errorf("failed to copy chart to temp directory: %w", err)
+	}
+
+	tempChartYamlPath := filepath.Join(tempDir, "Chart.yaml")
+	if err := os.WriteFile(tempChartYamlPath, updatedData, 0644); err != nil {
+		if removeErr := os.RemoveAll(tempDir); removeErr != nil {
+			st.logger.Warnf("Failed to remove temp chart directory %s: %v", tempDir, removeErr)
+		}
+		return chartPath, func() {}, fmt.Errorf("failed to write Chart.yaml: %w", err)
 	}


The function now relies more heavily on os.* for temp dir creation, file writes, and cleanup while simultaneously using st.fs.CopyDir, which undermines the filesystem abstraction and makes it harder to support non-OS filesystems consistently. Consider routing temp-dir creation, file read/write, and remove-all through st.fs (or expanding the fs interface with MkdirTemp/RemoveAll/WriteFile equivalents) so this logic remains portable and uniformly testable.

Copilot · 2026-04-17T15:29:39Z

+	if err := st.fs.CopyDir(chartPath, tempDir); err != nil {
+		if removeErr := os.RemoveAll(tempDir); removeErr != nil {
+			st.logger.Warnf("Failed to remove temp chart directory %s: %v", tempDir, removeErr)
 		}
-		mu.Unlock()
-		return func() {}, fmt.Errorf("failed to write Chart.yaml: %w", err)
+		return chartPath, func() {}, fmt.Errorf("failed to copy chart to temp directory: %w", err)
+	}


Copying the entire chart directory into a fresh temp directory for every rewrite can become expensive when many releases reference the same chart (especially concurrently)—it fixes the data race but may add substantial IO and temp space pressure. A tangible improvement is to introduce a short-lived cache/singleflight keyed by chartPath that creates at most one rewritten temp copy per unique chart per run (with ref-counted cleanup), so concurrent callers can share the same rewritten copy without touching the original.

Copilot · 2026-04-17T15:29:39Z

+			rewrittenPath, cleanup, err := st.rewriteChartDependencies(tempDir)
 			if err != nil {
 				errCh <- err
 				return
 			}
-			defer cleanup()
+
+			data, readErr := os.ReadFile(filepath.Join(rewrittenPath, "Chart.yaml"))
+			if readErr != nil {
+				errCh <- readErr
+				cleanup()
+				return
+			}
+
+			type ChartDependency struct {
+				Name       string `yaml:"name"`
+				Repository string `yaml:"repository"`
+				Version    string `yaml:"version"`
+			}
+			type ChartMeta struct {
+				APIVersion   string            `yaml:"apiVersion"`
+				Name         string            `yaml:"name"`
+				Version      string            `yaml:"version"`
+				Dependencies []ChartDependency `yaml:"dependencies,omitempty"`
+			}
+
+			var meta ChartMeta
+			if unmarshalErr := yaml.Unmarshal(data, &meta); unmarshalErr != nil {
+				errCh <- unmarshalErr
+				cleanup()
+				return
+			}
+
+			if meta.Name != "test-chart" {
+				errCh <- fmt.Errorf("expected chart name 'test-chart', got %q", meta.Name)
+				cleanup()
+				return
+			}
+
+			cleanup()


The test manually calls cleanup() along multiple early-return paths; this works but is easy to regress as the test evolves. Consider defer cleanup() immediately after the successful call to rewriteChartDependencies and then remove the repeated cleanup() calls—this ensures the temp dir is cleaned up even if additional assertions are added later and fail early.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Copilot · 2026-04-17T23:07:08Z

+	// Check cache: if another goroutine already created a rewritten copy for this chartPath, reuse it.
+	if cached, ok := rewrittenChartCache.Load(chartPath); ok {
+		st.logger.Debugf("Reusing cached rewritten chart at %s for %s", cached.(string), chartPath)
+		return cached.(string), func() {}, nil
+	}
+
+	// Use per-path mutex to serialize temp-copy creation for the same chart path.
+	mu := st.getNamedRWMutex(chartPath)
+	mu.Lock()
+	defer mu.Unlock()
+
+	// Double-check after acquiring lock.
+	if cached, ok := rewrittenChartCache.Load(chartPath); ok {
+		st.logger.Debugf("Reusing cached rewritten chart at %s for %s", cached.(string), chartPath)
+		return cached.(string), func() {}, nil
 	}



rewrittenChartCache is keyed only by chartPath, so once a rewritten copy is cached it will be reused even if the chart directory (or its Chart.yaml) changes later in the run (e.g., via per-release prepare hooks that mutate local charts). That can make subsequent releases operate on a stale copy and produce surprising results.

If caching is kept, consider keying by something that reflects Chart.yaml content (hash/mtime) or limiting cache scope/lifetime to a single immutable preparation phase; otherwise, prefer creating a fresh temp copy per call.

Suggested change

// Check cache: if another goroutine already created a rewritten copy for this chartPath, reuse it.

if cached, ok := rewrittenChartCache.Load(chartPath); ok {

st.logger.Debugf("Reusing cached rewritten chart at %s for %s", cached.(string), chartPath)

return cached.(string), func() {}, nil

}

// Use per-path mutex to serialize temp-copy creation for the same chart path.

mu := st.getNamedRWMutex(chartPath)

mu.Lock()

defer mu.Unlock()

// Double-check after acquiring lock.

if cached, ok := rewrittenChartCache.Load(chartPath); ok {

st.logger.Debugf("Reusing cached rewritten chart at %s for %s", cached.(string), chartPath)

return cached.(string), func() {}, nil

}

// Always create a fresh rewritten copy for the current chart contents.

// Reusing a cached copy keyed only by chartPath can return stale data when

// prepare hooks or other steps mutate the local chart directory during the run.

Copilot · 2026-04-17T23:07:09Z

+	defer cleanup()
+


These tests defer the cleanup returned by rewriteChartDependencies, but the current implementation always returns a no-op cleanup even when it creates a rewritten temp chart directory. That means the tests will leave temp directories behind on the machine running the suite.

Once cleanup is implemented to remove the temp copy (and clear any cache entry), it would be good to assert the rewrittenPath directory is removed after cleanup; alternatively, explicitly os.RemoveAll(rewrittenPath) in the tests when rewrittenPath != tempDir.

Suggested change

defer cleanup()

t.Cleanup(func() {

cleanup()

if rewrittenPath == tempDir {

return

}

if _, statErr := os.Stat(rewrittenPath); statErr != nil {

if os.IsNotExist(statErr) {

return

}

t.Errorf("failed to stat rewritten chart path during cleanup: %v", statErr)

return

}

if err := os.RemoveAll(rewrittenPath); err != nil {

t.Errorf("failed to remove rewritten chart path %q: %v", rewrittenPath, err)

return

}

if _, statErr := os.Stat(rewrittenPath); !os.IsNotExist(statErr) {

t.Errorf("expected rewritten chart path %q to be removed, stat err: %v", rewrittenPath, statErr)

}

})

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

…chart before modifying Instead of modifying the original Chart.yaml in-place (which causes race conditions when multiple releases reference the same local chart), copy the chart to a temporary directory and rewrite the copy's dependencies. This eliminates the need for per-chart mutex locks and prevents file corruption when concurrent goroutines process releases sharing the same local chart. Fixes #2502 Signed-off-by: yxxhero <[email protected]>

- Handle non-NotExist errors from st.fs.Stat to surface permission/IO failures - Reword function doc to clarify temp copy is conditional on rewrite being needed - Assert rewrittenPath vs tempDir based on expectModified in test table Signed-off-by: yxxhero <[email protected]>

… local chart Signed-off-by: yxxhero <[email protected]>

Signed-off-by: yxxhero <[email protected]>

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Copilot · 2026-04-18T06:00:19Z

 func DefaultFileSystem() *FileSystem {
 	dfs := FileSystem{
 		ReadDir:      os.ReadDir,
+		WriteFile:    os.WriteFile,
 		DeleteFile:   os.Remove,
+		MkdirTemp:    os.MkdirTemp,
+		RemoveAll:    os.RemoveAll,
 		Glob:         filepath.Glob,


DefaultFileSystem now wires new injectable funcs (WriteFile/MkdirTemp/RemoveAll), but the existing filesystem unit test that checks DefaultFileSystem fields are non-nil doesn’t cover these additions. Consider extending that test to assert these new function pointers are set, to prevent future nil regressions.

…m test Signed-off-by: yxxhero <[email protected]>

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Copilot · 2026-04-18T06:22:42Z

+releases:
+  - name: app
+    chart: helm/my-chart
+  - name: app-2
+    chart: helm/my-chart
+  - name: app-3
+    chart: helm/my-chart
+  - name: app-4
+    chart: helm/my-chart
+  - name: app-5
+    chart: helm/my-chart


This integration test config has multiple releases sharing the same local chart, but it doesn't include any chartify trigger (e.g., forceNamespace/jsonPatches/strategicMergePatches/transformers). rewriteChartDependencies is only called from processChartification, which only runs when PrepareChartify returns non-nil, so this test may not exercise the race-condition fix and could pass even on the buggy behavior. Consider adding a minimal chartify trigger (e.g., forceNamespace: default) to each release to ensure the concurrent chartify path (and dependency rewrite) is executed.

…test Signed-off-by: yxxhero <[email protected]>

…th strategic merge patches The race condition test values.yaml had templates at the top level instead of scoped under the raw subchart key, causing helm template to produce no output and chartify's ReplaceWithRendered to fail with an empty helmx.1.rendered directory. Also align the ConfigMap name to match the strategicMergePatches target. Signed-off-by: yxxhero <[email protected]>

Copilot

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

yxxhero requested review from aditmeno, itscaro, mumoshu, xiaomudk and zhaque44 as code owners April 17, 2026 13:23

yxxhero requested a review from Copilot April 17, 2026 13:25

aditmeno previously approved these changes Apr 17, 2026

View reviewed changes

Copilot started reviewing on behalf of yxxhero April 17, 2026 13:25 View session