Repository navigation
fix(helm): serialize repository state during parallel setup - #8113
Conversation
Share a cancelable acquisition gate across repository registration and chart reads, preserving every entry and parallel installation after acquisition. Part of #8112.
Validation at The complete Helm race suite passed in 47.831s, the installer race suites passed, vet passed, focused configured lint reported zero issues, and independent static review found no remaining correctness/security findings. The three source hashes were stable during validation. Native cluster CI and current-head provider review remain pending; this is still a draft. The gate protects clients in one process, while unrelated external Helm processes remain outside its scope. No retry budget, cache format, timeout or installation assertion was changed. |
✅
|
Code coverage overviewLanguages: Go Go / code-coverage/go
View coverage of the 6 most impacted files.
ℹ️ Calculations represent line coverage. Results may change as calculations are updated. |
@coderabbitai review |
Action performed
|
Native validation at The earlier deterministic real HTTP/filesystem tests establish the lost-registration and overlapping-access failures and their correction; complete Helm and installer race suites remain green. The published commit is verified signed, independently rechecked with the existing verification backend. MegaLinter's non-blocking executable-bit warning concerns an unchanged baseline helper: identical mode and blob at base/head, with both production callers explicitly invoking Bash. It is outside this three-file Helm change and is not a failed native gate. Current-head provider review has been requested; the PR remains a draft until that gate is satisfied. |
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes serializing Helm repository state during parallel setup, which is the main change. Description check The description directly explains the repository-state corruption problem and the change that serializes repository updates and chart reads while preserving cancellation and parallel installation. Linked Issues check Directly linked issue #8112is the only active target. Its coding requirements are covered at head 219d6f0:
- Shared gate across clients: AddRepository (pkg/client/helm/repository.go) calls acquireCh…Out of Scope Changes check All three changed files serve #8112:
- pkg/client/helm/chart_location.go: changes chartAcquireMu from sync.Mutex to a weighted semaphore. This is needed for context-aware acquisition. It also adds acq…Full details: Linked Issues check
-
Explanation
Directly linked issue
#8112is the only active target. Its coding requirements are covered at head 219d6f0:- Shared gate across clients: AddRepository (pkg/client/helm/repository.go) calls acquireChartState after request validation and holds chartAcquireMu through config load, cache setup, index download, and repositories.yaml write. chartAcquireMu is a package-level semaphore, so it covers distinct clients in one process.
- Registration and chart acquisition exclude each other in both directions. loadChartAndValues (pkg/client/helm/chart_location.go) takes the same gate for locate, load, and values reads.
- No nested locking: AddRepository does not call loadChartAndValues or acquireChartState again. The gate is released by defer when loadChartAndValues returns, so install and render run outside it and stay parallel.
- Cancellation: acquireChartState returns a wrapped context error when the context is already canceled or while waiting. It releases the token when cancellation races a successful acquisition.
- Regression tests (pkg/client/helm/repository_state_test.go): TestRepositoryRegistrationsPreserveEntries, TestRepositoryRegistrationAndChartAcquisitionExcludeEachOther, and TestRepositoryStateWaitCancellation. They use isolated HELM_* paths, an httptest server, and a generated local chart. They cover lost updates, reader/writer exclusion, and waiter cancellation without shared-state access.
- Preserved behavior: validateRepositoryRequest and its error text are unchanged. Retry constants and downloadRepositoryIndex retry logic are unchanged at head. The author reports that retry, cache, timeout, and installation assertions did not change. I could not diff against the base because the diff operation failed, so this relies on head content and the author's report.
- Non-coding items (full CI, native full-stack leg, current-head review, publication) were excluded.
Uncertainty: test pass results are author-reported. I did not execute tests. The new cancellation path returns 'acquire Helm chart state: ...' wrapping context.Canceled. Earlier validation keeps 'add repository context cancelled'. Both wrap with %w, so errors.Is still matches.
Full details: Out of Scope Changes check
-
Explanation
All three changed files serve
#8112:- pkg/client/helm/chart_location.go: changes chartAcquireMu from sync.Mutex to a weighted semaphore. This is needed for context-aware acquisition. It also adds acquireChartState and updates lock documentation to include registration.
- pkg/client/helm/repository.go: adds the shared gate to AddRepository. This is the core fix.
- pkg/client/helm/repository_state_test.go: adds regression tests for the required behaviors.
The reviewed summary shows no unrelated refactoring, dependency, or public API change. Exported declarations are unchanged.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 5 functions across 3 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
The docstring-percentage warning is refuted on its merits. The public CodeRabbit's actual review reports no actionable comments and no inline findings. This disclosed resolution starts a new ordered review round at the same head |
@coderabbitai review |
Action not completed
|
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Ready at The real repository/chart HTTP and filesystem fixtures reproduced the original races before the fix and now pass with cancellation and later-progress controls. Complete Helm and installer race suites, vet and focused lint passed. The actual native full-stack Vanilla/Kyverno journey at this complete source tree installed components, persisted its admission probe and cleaned up successfully. The signed head was verified locally and by GitHub. Scope remains shared Helm state within one process; installation execution stays parallel outside chart acquisition. Promoting for normal protected merge. |
Why
Parallel component setup can lose or corrupt Helm repository information, failing cluster creation even when the charts are available.
What
Protect repository changes and chart reads as one shared operation, while letting canceled requests stop promptly. Component installation remains parallel after chart loading.
Fixes #8112