* fix: resolve issues #2295, #2296, #2297 and OCI registry login
This PR fixes four related bugs affecting chart preparation, caching,
and OCI registry authentication.
Issue #2295: OCI chart cache conflicts with parallel helmfile processes
- Added filesystem-level locking using flock for cross-process sync
- Implements double-check locking pattern for efficiency
- Retry logic with 5-minute timeout and 3 retries
- Refactored into reusable acquireChartLock() helper function
- Added refresh marker coordination for cross-process cache management
Issue #2296: helmDefaults.skipDeps and helmDefaults.skipRefresh ignored
- Check both CLI options AND helmDefaults when deciding to skip repo sync
Issue #2297: Local chart + transformers causes panic
- Normalize local chart paths to absolute before calling chartify
OCI Registry Login URL Fix:
- Added extractRegistryHost() to extract just the registry host from URLs
- Fixed SyncRepos to use extracted host for OCI registry login
- e.g., "account.dkr.ecr.region.amazonaws.com/charts" ->
"account.dkr.ecr.region.amazonaws.com"
Test Plan:
- Unit tests for issues #2295, #2296, #2297
- Unit tests for OCI registry login (extractRegistryHost, SyncRepos_OCI)
- Integration tests for issues #2295 and #2297
- All existing unit tests pass (including TestLint)
Fixes#2295Fixes#2296Fixes#2297
Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
* fix: replace 60s timeout with reader-writer locks for OCI chart caching
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 #2298
Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
* fix: prevent deadlock when multiple releases share the same chart
When multiple releases use the same OCI chart (e.g., same chart different
values), workers in PrepareCharts would deadlock:
1. Worker 1 acquires lock for chart/path, downloads, adds to cache
2. Worker 2 finds chart in cache, tries to acquire lock on same path
3. Worker 2 blocks waiting for Worker 1's lock
4. Collector waits for Worker 2's result
5. Worker 1's lock held until PrepareCharts finishes -> deadlock
The fix: when using the in-memory chart cache (which means another worker
in the same process already downloaded the chart), don't acquire another
lock. This is safe because:
- The in-memory cache is only used within a single helmfile process
- The tempDir cleanup is deferred until after helm callback completes
- Cross-process coordination is still handled by file locks during downloads
This fixes the "signal: killed" test failures in CI for:
- oci_chart_pull_direct
- oci_chart_pull_once
- oci_chart_pull_once2
Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
* fix: resolve deadlock by releasing OCI chart locks immediately after download
This commit simplifies the OCI chart locking mechanism to fix deadlock
issues that occurred when multiple releases shared the same chart.
Problem:
When multiple releases used the same OCI chart, workers in PrepareCharts
would deadlock because:
1. Worker 1 acquires lock for chart/path, downloads chart
2. Worker 2 tries to acquire lock on same path, blocks waiting
3. PrepareCharts waits for all workers to complete
4. Worker 1's lock held until PrepareCharts finishes -> deadlock
Solution:
Release locks immediately after chart 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
- The in-memory chart cache prevents redundant downloads within a process
- Cross-process coordination via file locks still works during download
Changes:
- Remove chartLock field from chartPrepareResult struct
- Release locks immediately in getOCIChart() and forcedDownloadChart()
- Simplify PrepareCharts() by removing lock collection and release logic
- Update function signatures to return only (path, error)
This also fixes the "signal: killed" test failures in CI for:
- oci_chart_pull_direct
- oci_chart_pull_once
- oci_chart_pull_once2
Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
* fix: add double-check locking for in-memory chart cache
When multiple workers concurrently process releases using the same chart,
they all check the in-memory cache before acquiring locks. If none have
populated the cache yet, all workers miss and try to download.
Previously, even after acquiring the exclusive lock, the code would
re-download the chart when needsRefresh=true (the default). This caused
multiple "Pulling" messages in tests like oci_chart_pull_once.
The fix adds a second in-memory cache check AFTER acquiring the lock.
This implements proper double-check locking:
1. Check cache (outside lock) → miss
2. Acquire lock
3. Check cache again (inside lock) → hit if another worker populated it
4. If still miss, download and add to cache
This ensures only one worker downloads the chart, while others use
the cached version populated by the first worker.
Changes:
- Add in-memory cache double-check in getOCIChart() after acquiring lock
- Add in-memory cache double-check in forcedDownloadChart() after acquiring lock
This fixes the oci_chart_pull_once and oci_chart_pull_direct test failures
where charts were being pulled multiple times instead of once.
Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
* fix: use callback to prevent redundant chart downloads within a process
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 <amenon@canarytechnologies.com>
* fix: address Copilot review comments on PR #2298
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 <amenon@canarytechnologies.com>
* fix: make oci-parallel-pull test resilient to registry issues
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 #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 <amenon@canarytechnologies.com>
* fix: make oci-parallel-pull test resilient to registry issues
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 #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 <amenon@canarytechnologies.com>
* fix: address PR #2298 review - reinitialize fileLock after release
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 <amenon@canarytechnologies.com>
* fix: address PR #2298 Copilot review comments (round 4)
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 <amenon@canarytechnologies.com>
* fix: address PR #2298 Copilot review comments (round 5)
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 <amenon@canarytechnologies.com>
* fix: clarify lock file check is informational only
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 <amenon@canarytechnologies.com>
* fix: add HELM_BIN env var to Dockerfiles
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#2303
Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
---------
Signed-off-by: Aditya Menon <amenon@canarytechnologies.com>
9c70adc038