fix: resolve issues #2295, #2296, and #2297 by aditmeno · Pull Request #2298 · helmfile/helmfile

aditmeno · 2025-11-26T14:49:31Z

Summary

This PR fixes four related bugs that affect chart preparation, caching, and OCI registry authentication:

Issue #2295: OCI chart cache conflicts with parallel helmfile processes

Problem: When running multiple helmfile processes in parallel using the same OCI chart, race conditions cause "file already exists" errors due to concurrent cache writes.

Solution:

Added filesystem-level locking using flock for cross-process synchronization
Uses reader-writer locks: shared locks (RLock) for reading cached charts, exclusive locks (Lock) for download/refresh
Implements double-check locking pattern for efficiency (re-check cache after acquiring lock)
Simplified approach: Locks are released immediately after download completes. This is safe because the tempDir cleanup is deferred until after helm operations complete in withPreparedCharts(), so charts won't be deleted mid-use
In-memory chart cache prevents redundant downloads within a single process
Removed the arbitrary 60s refresh marker timeout - lock-based synchronization provides proper coordination

Files changed: pkg/state/state.go (acquireChartLock, acquireSharedLock, acquireExclusiveLock, getOCIChart, forcedDownloadChart functions)

Issue #2296: `helmDefaults.skipDeps` and `helmDefaults.skipRefresh` are ignored

Problem: CLI flags --skip-deps and --skip-refresh work correctly, but setting these in helmfile.yaml under helmDefaults has no effect - repos are still synced.

Root cause: SkipRepos was only set from CLI options in ChartPrepareOptions, not from helmDefaults.

Solution: Check both CLI options AND r.state.HelmDefaults.SkipDeps/SkipRefresh when deciding whether to skip repo sync.

Files changed: pkg/app/run.go (withPreparedCharts and Deps functions)

Issue #2297: Local chart + transformers causes panic

Problem: Regression from 1.1.9 to 1.2.1. When using a local chart with a relative path (chart: ../chart) combined with transformers in helmfile.d/, helmfile fails with:

helm pull ../chart --untar fails with "repo .. not found"

Root cause: The relative chart path was not normalized to an absolute path before being passed to chartify. Chartify then tried to helm pull ../chart treating it as a remote chart reference.

Solution: Normalize local chart paths to absolute before calling processChartification().

Files changed: pkg/state/state.go (prepareChartForRelease function)

OCI Registry Login URL Fix (from PR #2301)

Problem: When using OCI registries with chart paths (e.g., ECR with /charts path), helm registry login would fail because the full URL was passed instead of just the registry host.

Solution:

Added extractRegistryHost() function to extract just the registry host from OCI URLs
e.g., 123456789012.dkr.ecr.us-east-1.amazonaws.com/charts → 123456789012.dkr.ecr.us-east-1.amazonaws.com
Fixed SyncRepos to use the extracted host for OCI registry login

Files changed: pkg/state/state.go (extractRegistryHost, SyncRepos functions)

Test Plan

Unit tests added for issue .helmDefaults.skipDeps and .helmDefaults.skipRefresh are ignored #2296 (pkg/state/issue_2296_test.go)
Unit tests added for issue helmfile 1.2.1 fails with multiple release files using local chart and transformers #2297 (pkg/state/issue_2297_test.go)
Unit tests added for issue Cache conflicts running multiple helmfile using the same chart in parallel #2295 (pkg/state/oci_chart_lock_test.go)
Unit tests added for OCI registry login (Test_extractRegistryHost, TestHelmState_SyncRepos_OCI)
Integration test added for issue helmfile 1.2.1 fails with multiple release files using local chart and transformers #2297 (test/integration/test-cases/issue-2297-local-chart-transformers.sh)
Integration test added for issue Cache conflicts running multiple helmfile using the same chart in parallel #2295 (test/integration/test-cases/oci-parallel-pull.sh)
All existing unit tests pass (including TestLint)
Manual testing of issue helmfile 1.2.1 fails with multiple release files using local chart and transformers #2297 fix verified transformer annotations are applied

Breaking Changes

None. All changes are backwards compatible bug fixes.

Fixes #2295
Fixes #2296
Fixes #2297

yxxhero · 2025-11-26T14:54:06Z

@aditmeno split 3 PRs are better next time. thanks so much.

aditmeno · 2025-11-26T16:27:42Z

@yxxhero PR is ready for review

aditmeno · 2025-11-26T20:12:19Z

@champtar This concern is addressed in the merged changes from PR #2301. The new implementation does not force SkipRefresh=True - it still supports refresh (delete + re-download) while adding coordination for parallel processes.

How it works:

The acquireChartLock() function tracks needsRefresh = !opts.SkipDeps && !opts.SkipRefresh, so refresh behavior is preserved when the user hasn't disabled it.

When needsRefresh=true and the chart directory exists:

First process: Deletes the existing chart, re-downloads it, and creates a refresh marker file
Parallel processes within 60 seconds: Use the cached version (prevents race conditions like "file already exists" errors)
After 60 seconds: The marker is considered stale, so the next process will refresh again

This means:

--skip-refresh=false (default): Charts are refreshed once per ~60s window (can make it configurable if it makes sense), with parallel processes coordinated
--skip-refresh=true: Always uses cached version (existing behavior)

The 60-second TTL is chosen to cover a typical helmfile run duration while still allowing fresh pulls on subsequent runs. This should handle changing tags like latest correctly - each new helmfile invocation (after 60s) will pull the latest version.

champtar · 2025-11-26T21:26:38Z

@aditmeno thanks for working on this
60s is completely arbitrary, deploy that trigger scaling up/down often take more than that, so you might launch helm while the chart is deleted, or have the chart deleted while helm run.
The right way to do it IMO is to use exclusive lock when you want to refresh, and shared lock while you use the the chart.

Skipping refresh when the chart is recent is a separate issue, this is a good improvement, but the time should not be hardcoded

@champtar

Address PR review feedback from @champtar about the OCI chart caching mechanism. The previous implementation used a 60-second timeout which was arbitrary and caused race conditions when helm deployments took longer (e.g., deployments triggering scaling up/down). Changes: - Replace 60s refresh marker timeout with proper reader-writer locks - Use shared locks (RLock) when using cached charts (allows concurrent reads) - Use exclusive locks (Lock) when refreshing/downloading charts - Hold locks during entire helm operation lifecycle (not just during download) - Add getNamedRWMutex() for in-process RW coordination - Update PrepareCharts() to return locks map for lifecycle management - Add chartLockReleaser in run.go to release locks after helm callback - Remove unused mutexMap and getNamedMutex (replaced by RW versions) - Add comprehensive tests for shared/exclusive lock behavior This eliminates the race condition where one process could delete a cached chart while another process's helm command was still using it. Fixes review comment on PR helmfile#2298 Signed-off-by: Aditya Menon <[email protected]>

aditmeno · 2025-11-26T22:49:30Z

@champtar Thank you for the feedback! You're absolutely right about the 60s timeout being problematic. I've addressed your concerns.

Changes made:

Replaced 60s timeout with proper reader-writer locks using flock's built-in support:
- TryRLockContext() for shared/read locks (using cached chart)
- TryLockContext() for exclusive/write locks (refresh/download)
Shared locks allow concurrent reads - multiple processes can use the same cached chart simultaneously
Exclusive locks for refresh operations - only one process can delete + re-download at a time
Simplified lock lifecycle - Locks are released immediately after download completes. This is safe because:
- The tempDir cleanup is deferred until after helm operations complete in withPreparedCharts()
- Charts won't be deleted mid-operation since the temp directory persists for the entire helm lifecycle
Callback-based deduplication within a process - The key fix for preventing redundant downloads:
- acquireChartLock() accepts an optional skipRefreshCheck callback
- Before deleting a cached chart for refresh, the callback checks the in-memory cache
- If in-memory cache is populated (another worker downloaded it this run) → use cached, don't delete
- If in-memory cache is empty (chart from a previous run) → respect needsRefresh flag
This correctly handles both scenarios:
- Within-process: Multiple releases with same chart → only one download
- Cross-run: Floating tags with --skip-refresh=false → re-downloads as expected
Removed the hardcoded 60s refresh marker timeout entirely - lock-based synchronization provides proper coordination

Why release locks immediately?

Holding locks for the entire helm operation caused deadlocks when multiple releases shared the same OCI chart:

Worker 1 acquires lock, downloads chart, waits for PrepareCharts to finish
Worker 2 tries to acquire same lock, blocks
PrepareCharts waits for Worker 2 → deadlock

Test coverage:

Added TestOCIChartSharedExclusiveLocks verifying reader-writer lock behavior
Added TestOCIChartDoubleCheckLocking verifying cache deduplication

Copilot

Pull request overview

This PR fixes four related bugs affecting chart preparation, caching, and OCI registry authentication in Helmfile. The changes implement filesystem-level locking to prevent race conditions during parallel chart downloads (issue #2295), ensure helmDefaults settings are properly respected for repo synchronization (issue #2296), fix path normalization for local charts with transformers (issue #2297), and correct OCI registry login URL handling.

Key changes:

Added cross-process reader-writer locking using github.com/gofrs/flock for safe parallel OCI chart downloads
Extended repo skip logic to check both CLI options and helmDefaults.skipDeps/skipRefresh
Normalized local chart paths to absolute before chartification to prevent "repo not found" errors

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
pkg/state/state.go	Core locking implementation with `acquireChartLock`, `acquireSharedLock`, `acquireExclusiveLock` functions; added `extractRegistryHost` for OCI registry login; fixed local chart path normalization before chartification; exported Logger() method
pkg/app/run.go	Updated `withPreparedCharts` and `Deps` to check `helmDefaults.skipDeps`/`skipRefresh` when deciding to skip repo sync; added chartLockReleaser type and logic (currently no-op since locks released immediately)
pkg/state/oci_chart_lock_test.go	Comprehensive unit tests for file locking behavior including shared/exclusive lock interactions, double-check locking pattern, and timeout handling
pkg/state/issue_2296_test.go	Unit tests verifying helmDefaults settings are respected in skip logic
pkg/state/issue_2297_test.go	Unit tests for local chart path normalization with transformers
pkg/state/state_test.go	Unit tests for `extractRegistryHost` and OCI registry login behavior
pkg/exectest/helm.go	Added `RegistryLoginHost` field to capture host passed to RegistryLogin for testing
test/integration/test-cases/oci-parallel-pull.sh	Integration test verifying parallel OCI chart pulls don't cause race conditions
test/integration/test-cases/issue-2297-local-chart-transformers.sh	Integration test for local chart with transformers scenario
test/integration/test-cases/oci-parallel-pull/input/helmfile.yaml	Test input file defining OCI chart for parallel pull testing
test/integration/test-cases/issue-2297-local-chart-transformers/input/*	Test input files with helmfile.d structure and local chart for issue #2297
test/integration/run.sh	Added new integration tests to test suite execution
go.mod	Promoted `github.com/gofrs/flock` from indirect to direct dependency

This commit addresses the automated review comments from GitHub Copilot: 1. pkg/state/state.go: Add nil check for logger in Release() method to prevent potential nil pointer dereference when logger is nil. 2. pkg/state/state.go: Fix misleading comment about "external callers" to accurately reflect that Logger() is used by the app package. 3. pkg/state/issue_2296_test.go: Add comment noting that boolPtr helper is already defined in skip_test.go (shared across test files). 4. test/integration/test-cases/oci-parallel-pull.sh: Replace hardcoded /tmp paths with a dedicated temp directory for test outputs. Add cleanup for the output directory in the cleanup function. 5. test/integration/test-cases/issue-2297-local-chart-transformers.sh: Add cleanup trap to remove temp directory on exit, preventing leftover files from accumulating. Signed-off-by: Aditya Menon <[email protected]>

When multiple workers concurrently process releases using the same chart, they need to coordinate to avoid redundant downloads. The previous fix set SkipRefresh=true for OCI charts, which prevented legitimate refresh scenarios (e.g., floating tags). This commit implements a better solution using a callback mechanism: 1. acquireChartLock() now accepts an optional skipRefreshCheck callback 2. Before deleting a cached chart for refresh, the callback is invoked 3. If the callback returns true (in-memory cache has the chart), skip refresh 4. This allows deduplication within a process while respecting cross-run refresh The flow is now: - Worker 1 downloads chart, adds to in-memory cache, releases lock - Worker 2 acquires lock, sees needsRefresh=true, but callback sees in-memory cache is populated → uses cached instead of deleting This correctly handles: - Within-process deduplication: only one download per chart - Cross-run refresh: respects --skip-refresh flag for floating tags - Immutable versions: cached and reused as expected Changes: - Add skipRefreshCheck callback parameter to acquireChartLock() - Update getOCIChart() to pass in-memory cache check callback - Update forcedDownloadChart() to pass in-memory cache check callback - Remove SkipRefresh=true workaround for OCI charts Signed-off-by: Aditya Menon <[email protected]>

This commit addresses the automated review comments from GitHub Copilot: 1. pkg/state/state.go: Add nil check for logger in Release() method to prevent potential nil pointer dereference when logger is nil. 2. pkg/state/state.go: Fix misleading comment about "external callers" to accurately reflect that Logger() is used by the app package. 3. pkg/state/issue_2296_test.go: Add comment noting that boolPtr helper is already defined in skip_test.go (shared across test files). 4. test/integration/test-cases/oci-parallel-pull.sh: Replace hardcoded /tmp paths with a dedicated temp directory for test outputs. Add cleanup for the output directory in the cleanup function. 5. test/integration/test-cases/issue-2297-local-chart-transformers.sh: Add cleanup trap to remove temp directory on exit, preventing leftover files from accumulating. 6. Remove dead code: The chartLocks map in PrepareCharts was always empty since locks are released immediately after download. Removed the unused return value and corresponding handling in run.go to improve code clarity and maintainability. Signed-off-by: Aditya Menon <[email protected]>

The integration test was intermittently failing in CI due to Docker Hub rate limiting or network issues. These failures are not helmfile bugs. Changes: - Add is_registry_error() function to detect external registry issues (rate limits, network timeouts, connection refused, etc.) - Check for the race condition bug (issue helmfile#2295) first and fail fast - If other failures occur, check if they're registry-related - Skip test gracefully when registry issues are detected instead of failing CI on external infrastructure problems This ensures the test still catches the actual race condition bug while not causing false failures due to Docker Hub rate limits in CI. Signed-off-by: Aditya Menon <[email protected]>

The integration test was failing in CI for two reasons: 1. Docker Hub rate limiting or network issues causing helmfile to fail 2. The test script exits early due to `set -e` when `wait` returns non-zero Changes: - Use `wait $pid || exit=$?` pattern to capture exit codes without triggering set -e. When wait returns non-zero, the || branch captures the exit code into the variable, preventing script termination. - Add is_registry_error() function to detect external registry issues (rate limits, network timeouts, connection refused, etc.) - Check for the race condition bug (issue helmfile#2295) first and fail fast - Skip test gracefully when registry issues are detected instead of failing CI on external infrastructure problems This ensures the test still catches the actual race condition bug while not causing false failures due to Docker Hub rate limits in CI. Signed-off-by: Aditya Menon <[email protected]>

…lease Address Copilot review comments: 1. pkg/state/state.go: Reinitialize fileLock after releasing shared lock When upgrading from shared to exclusive lock, the fileLock needs to be reinitialized with flock.New() after calling Release(). This ensures a fresh flock object is used for the exclusive lock acquisition. 2. test/integration/test-cases/oci-parallel-pull.sh: Add lock file verification warning if no lock files are found, to ensure the locking mechanism is actually being tested. Signed-off-by: Aditya Menon <[email protected]>

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.

Address 8 Copilot review comments: 1. pkg/state/state.go: Release in-process mutex during retry backoff to avoid blocking other goroutines for up to 90 seconds. 2. pkg/state/state.go: Include chartPath in shared lock error message for better debugging. 3. pkg/state/state.go: Document that extractRegistryHost does not handle URLs with query parameters or fragments (uncommon for OCI registries). 4. pkg/state/state.go: Document that skipRefreshCheck callback should be fast and non-blocking since it runs while holding exclusive lock. 5. oci-parallel-pull.sh: Use case-insensitive grep (-i flag) to catch error variations like "I/O timeout". 6. helmfile.yaml: Expand comment explaining why library charts can't be used for this test (they can't be templated by Helm). Skipped (with justification): - PrepareChartKey helper: Only 2 usages with different source structs - Context reuse in retry: Per-attempt contexts provide clearer semantics Signed-off-by: Aditya Menon <[email protected]>

Copilot

Pull request overview

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

1. Make race condition detection grep more robust (oci-parallel-pull.sh) - Use case-insensitive extended regex (-iqE) - Add multiple pattern variations to catch different tar/helm versions 2. Remove unused Logger() method from HelmState (state.go) - Method was never called; all lock releases use st.logger directly 3. Add clarifying comments for lock retry behavior (state.go) - Document why file system errors are retried but timeouts are not - Explain flock returns (false, nil) on context deadline exceeded Signed-off-by: Aditya Menon <[email protected]>

Copilot

Pull request overview

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

Lock files are ephemeral and may be cleaned up immediately after helmfile processes complete. Update comments and warning message to make clear their absence doesn't indicate locking wasn't used. Signed-off-by: Aditya Menon <[email protected]>

Copilot

Pull request overview

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

The helm-git plugin requires HELM_BIN environment variable to be set. Without it, the plugin fails with "HELM_BIN: parameter not set". Add HELM_BIN=/usr/local/bin/helm to all Dockerfile variants. Fixes helmfile#2303 Signed-off-by: Aditya Menon <[email protected]>

aditmeno force-pushed the fix/issues-2295-2296-2297 branch 2 times, most recently from 2bbcc3a to 2762123 Compare November 26, 2025 14:53

aditmeno marked this pull request as ready for review November 26, 2025 16:27

aditmeno requested review from itscaro, mumoshu, xiaomudk, yxxhero and zhaque44 as code owners November 26, 2025 16:27

aditmeno force-pushed the fix/issues-2295-2296-2297 branch from 34125ac to 36290c3 Compare November 26, 2025 18:31

champtar reviewed Nov 26, 2025

View reviewed changes

Comment thread pkg/state/state.go

aditmeno force-pushed the fix/issues-2295-2296-2297 branch 3 times, most recently from 161444c to e1e0416 Compare November 26, 2025 20:09

aditmeno force-pushed the fix/issues-2295-2296-2297 branch from e1e0416 to 751cda6 Compare November 26, 2025 20:13

aditmeno requested a review from champtar November 26, 2025 22:52

aditmeno marked this pull request as draft November 27, 2025 00:22

aditmeno force-pushed the fix/issues-2295-2296-2297 branch from 8198420 to 0077ab9 Compare November 27, 2025 01:09

aditmeno marked this pull request as ready for review November 27, 2025 01:34

yxxhero requested review from Copilot and removed request for champtar November 27, 2025 05:48

Copilot started reviewing on behalf of yxxhero November 27, 2025 05:49 View session

Copilot finished reviewing on behalf of yxxhero November 27, 2025 05:52