Fix concurrent map access in azurekeyvault and httpjson providers - #1250
Merged
Conversation
pathob
requested review from
itscaro,
mumoshu,
xiaomudk and
yxxhero
as code owners
July 22, 2026 19:03
pathob
force-pushed
the
fix/provider-cache-races
branch
2 times, most recently
from
July 22, 2026 19:09
54cbdb4 to
69f62e1
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses data races in provider-level caches that previously caused fatal error: concurrent map writes when refs were resolved concurrently (e.g., Helmfile concurrency > 1). It adds synchronization around lazily populated cache maps in the azurekeyvault and httpjson providers and introduces regression tests aimed at catching the issue under the race detector.
Changes:
- Add mutex protection for the
httpjsonprovider’s lazy JSON document cache. - Add mutex protection for the
azurekeyvaultprovider’s lazy Key Vault client cache. - Add concurrency-focused regression tests for both providers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/providers/httpjson/httpjson.go | Adds a mutex and refactors JSON doc caching to avoid concurrent map writes. |
| pkg/providers/httpjson/httpjson_test.go | Adds a concurrent GetString regression test for the httpjson cache. |
| pkg/providers/azurekeyvault/azurekeyvault.go | Adds a mutex and refactors client caching to avoid concurrent map writes. |
| pkg/providers/azurekeyvault/azurekeyvault_test.go | Adds a concurrent client-creation regression test for the azurekeyvault cache. |
pathob
force-pushed
the
fix/provider-cache-races
branch
from
July 23, 2026 14:25
69f62e1 to
9de1ed7
Compare
Both providers lazily populate an internal cache map (Key Vault clients, fetched JSON documents) without synchronization. When helmfile runs with concurrency > 1, parallel goroutines resolving refs crash the process with 'fatal error: concurrent map writes'. Guard each cache with a mutex that only covers map access. Credential setup and document fetching run outside the lock, so different vaults and URLs can initialize in parallel. Cache stores double-check for entries created concurrently and keep the first one as canonical. Also add regression tests that fail on the unfixed code when run with the Go race detector. Signed-off-by: Patrick Hobusch <patrick@hobusch.net>
pathob
force-pushed
the
fix/provider-cache-races
branch
from
July 23, 2026 14:32
9de1ed7 to
0c184f9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both providers lazily populate an internal cache map (Key Vault clients, fetched JSON documents) without synchronization. When helmfile runs with concurrency > 1, parallel goroutines resolving refs crash the process with 'fatal error: concurrent map writes'. Guard each cache with a mutex that only covers map access. Credential setup and document fetching run outside the lock, so different vaults and URLs can initialize in parallel. Cache stores double-check for entries created concurrently and keep the first one as canonical. Also add regression tests that fail on the unfixed code when run with the Go race detector.
Disclaimer: This change was implemented using Claude Fable 5 based on the stack trace that I provided from one of our CI runs where we extensively use the azurekeyvault provider. This issue has been there "forever", but we lived with it by disabling concurrency in Helmfile and accepting the much longer deployment times. The recent developments in AI motivated me to finally find and fix this issue now. I also asked it to find similar issues and it came up with this fix for the httpjson provider as well.