Skip to content

Commit b7559e1

Browse files
ostermanclaudeaknysh
authored
fix(dag): stop concurrent map crash in bulk terraform commands (#2831)
* fix(dag): stop concurrent map iteration/write crash in bulk terraform commands FindStacksMap returns its cached stack config maps by reference, and ProcessComponentConfig handed the shared component section straight to callers. DAG-scheduled bulk commands (terraform --all/--affected/--query) run ProcessStacks concurrently across workers, so one worker's writes (atmos_component, workspace, sources, deps, merged auth, ...) raced with another worker's reads of the same cached section, crashing with `fatal error: concurrent map iteration and map write` at higher --max-concurrency. Shallow-clone the component section before mutating it, and apply the same fix to two adjacent cache-corruption sites in the describe-stacks processor that deleted keys from cache-owned maps in place. Add regression tests that fail pre-fix both deterministically and under -race. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(security): bump brace-expansion to patch CVE-2026-14257 (GHSA-mh99-v99m-4gvg) brace-expansion <=5.0.7 lets expand() accumulate unbounded output length from chained brace groups, causing an uncatchable OOM crash (high severity, Dependabot alert #261). Bump the existing pnpm overrides for both major lines in use here (transitive via minimatch, pulled in by serve-handler/docusaurus and docusaurus-plugin-llms) to the patched releases: 1.1.18 and 2.1.4, both published today with the EXPANSION_MAX_LENGTH bound backported from the 5.0.8 fix. Verified by diffing the published tarballs against the vulnerable versions. Website builds clean with the bump; no Go code is affected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(fixes): add fix-log record for DAG concurrent map crash Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): allowlist GHSA-mh99-v99m-4gvg pending advisory data update The Dependency Review job still flags brace-expansion@1.1.18/2.1.4 (added in the prior brace-expansion bump) because GHSA-mh99-v99m-4gvg's recorded vulnerable range ("<=5.0.7") doesn't distinguish between brace-expansion's parallel 1.x/2.x/5.x release lines. Both versions we use already contain the same EXPANSION_MAX_LENGTH bound backported from the 5.0.8 fix, verified by diffing the published tarballs against the CVE fix commit. Upgrading further to the only version the advisory recognizes as patched (5.0.8+) isn't safe here: brace-expansion 5.x's CommonJS build switched from a callable default export to a named `exports.expand`, which breaks minimatch@3.1.5's `require('brace-expansion')(...)` call convention (transitive via serve-handler/@docusaurus/core) — a genuine breaking API change, not just a semver-major label. Allowlisted following the existing GHSA-fxhp-mv3v-67qp precedent in this same file, with the reasoning recorded inline for removal once GitHub's advisory data or minimatch's dependency catches up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(describe): compute spacelift_stack/atlantis_project before templates Acceptance tests started failing after the ProcessComponentConfig clone (previous commit): describe component's tags.spacelift_stack/ tags.atlantis_project rendered as `<no value>` instead of the resolved name, and path-resolved describe component (`.`/`./component`) showed an empty imports list instead of the real one. Both were pre-existing bugs masked by the exact cache-mutation issue just fixed. `describe component` runs ProcessStacks twice per invocation (once via resolveAuthManager's preliminary ExecuteDescribeComponent call, once for the real result); pre-fix, the first call's completed computation leaked into the second call's shared, unprotected cache entry, making `{{ .spacelift_stack }}`/`{{ .atlantis_project }}` template references resolve "by accident" and making a describe-stacks preliminary pass's `delete(stackMap, "imports")` corruption invisible. Once ProcessStacks stopped mutating the shared cache, each call started from a clean slate and both latent bugs became visible and deterministic. BuildSpaceliftStackNameFromComponentConfig/BuildAtlantisProjectNameFromComponentConfig only depend on data already populated before template processing (ComponentSettingsSection, ComponentVarsSection, ComponentFromArg, Stack), so move both calls before the template-processing block instead of after it. This makes `.spacelift_stack`/`.atlantis_project` genuinely available to templates in a single pass, matching what the working "describe component <name>" (backward-compatibility) golden snapshot already showed, and consistent regardless of how many times ProcessStacks runs per invocation. Regenerated the 3 affected golden snapshots via `-regenerate-snapshots` per repo convention (never hand-edited) — verified each diff against the now-consistent, already-correct behavior on the other snapshot. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): scope the brace-expansion GHSA allowlist to verified versions Address CodeRabbit review feedback on PR #2831: allow-ghsas allowlists GHSA-mh99-v99m-4gvg by ID only, so any future brace-expansion resolution (an unpatched 3.x/4.x, or a 1.x/2.x release without the backported fix) would silently bypass the vulnerability check. Add a CI step that parses website/pnpm-lock.yaml and fails the job if any resolved brace-expansion version isn't in the explicit verified-patched set (1.1.18, 2.1.4). Also add regression tests closing the patch-coverage gap Codecov flagged on the two Spacelift/Atlantis error-propagation lines moved earlier in ProcessStacks: real, reachable error paths (a malformed name_template for BuildSpaceliftStackNameFromComponentConfig; a type-mismatched settings.atlantis.project_template for BuildAtlantisProjectNameFromComponentConfig) that ProcessStacks must surface rather than silently swallow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): make the brace-expansion guard's no-match diagnostic reachable Address CodeRabbit review feedback on PR #2831: under `set -euo pipefail`, grep's exit status 1 for "no matches" was propagating through the pipeline into the `found=` assignment, aborting the step with a bare exit code before the intended "No brace-expansion entries found" error message ever printed. Convert grep's expected no-match status into empty input while still preserving failures from a genuinely unreadable/missing file (verified locally: exit 2 still aborts, exit 1 now reaches the diagnostic). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(ci): correct override-pin wording in brace-expansion comment Address CodeRabbit review feedback on PR #2831: the pnpm override selectors (brace-expansion@^1/@^2) always carry a caret; only their pinned target values (1.1.18/2.1.4) are exact. The prior comment conflated the two, documenting an inaccurate contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): drop brace-expansion lockfile re-verification step The bash step re-parsing website/pnpm-lock.yaml on every PR was pure defense-in-depth on top of the allow-ghsas suppression and has cost four follow-up commits with no functional benefit over the one-line allowlist entry. Investigated a real fix (traced the dependency chain to serve-handler/minimatch, checked dependency-review-action's suppression options, confirmed upstream status via isaacs/minimatch#314 and #310): there isn't one currently reachable from this repo, so the suppression is structurally required, not a maintenance debt we're choosing to carry. Rewrote the allow-ghsas comment with the upstream citations and a concrete removal condition instead of dropping the extra verification step silently. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
1 parent b69113d commit b7559e1

9 files changed

Lines changed: 478 additions & 42 deletions

.github/workflows/dependency-review.yml

Lines changed: 39 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -65,26 +65,49 @@ jobs:
6565
# currently the latest tagged v2 release. Remove after Helm/ORAS ships a
6666
# fixed version for GHSA-fxhp-mv3v-67qp.
6767
#
68-
# Temporary allowlist: brace-expansion's advisory (GHSA-mh99-v99m-4gvg /
69-
# CVE-2026-14257) lists "<=5.0.7" as vulnerable and "5.0.8" as the first
70-
# patched version, but that range predates the maintainer's later
71-
# backport of the same EXPANSION_MAX_LENGTH fix into the 1.x/2.x lines
72-
# (verified by unpacking the exact 1.1.18/2.1.4 tarballs pinned -- no
73-
# `^` range -- in website/package.json's pnpm overrides: the fix is
74-
# present, and the `module.exports = expandTop` callable export is
75-
# unchanged). Jumping to 5.0.8+ is not an option: its CommonJS build
76-
# switched to a named `exports.expand` export, which breaks the
77-
# `require('brace-expansion')` usage in minimatch@3.1.5/9.0.9.
68+
# GHSA-mh99-v99m-4gvg (brace-expansion, CVE-2026-14257): the advisory's
69+
# recorded vulnerable range ("<=5.0.7") doesn't distinguish between
70+
# brace-expansion's parallel 1.x/2.x/5.x release lines, so it still flags
71+
# our pinned 1.1.18/2.1.4 even though both contain the same
72+
# EXPANSION_MAX_LENGTH bound backported from the 5.0.8 fix (verified by
73+
# diffing the published tarballs). The only version the advisory
74+
# recognizes as patched, 5.0.8+, is not a safe upgrade here: brace-expansion
75+
# 5.x's CommonJS build changed from a callable default export
76+
# (`module.exports = expand`) to a named export (`exports.expand`), which
77+
# breaks minimatch@3.1.5's `require('brace-expansion')(...)` call
78+
# convention (transitive via serve-handler/@docusaurus/core).
79+
#
80+
# This is a known, already-closed-as-wontfix situation upstream, not
81+
# something fixable from this repo: see isaacs/minimatch#314 and #310.
82+
# The ReDoS fix was backported into brace-expansion's 1.x/2.x lines, but
83+
# minimatch's maintainer declined to bump the older 3.x/5.x/8.x/9.x
84+
# release lines to brace-expansion 5.x, since those lines exist to
85+
# support CJS-only consumers. minimatch's current release (10.2.6) does
86+
# depend on brace-expansion ^5.0.8 (the patched line the advisory
87+
# recognizes), but nothing in our tree can reach it: serve-handler@6.1.7
88+
# (used by @docusaurus/core for `docusaurus serve`) still hard-pins
89+
# minimatch to 3.1.5, and everything else here resolves minimatch@9.0.9,
90+
# which depends on brace-expansion ^2.0.1.
7891
#
7992
# NOTE: allow-ghsas suppresses this GHSA for every resolved
8093
# brace-expansion version, not just 1.1.18/2.1.4 -- the action has no
8194
# package/version-scoped vulnerability exception (only
82-
# allow-dependencies-licenses is purl-scoped, and that's license-only).
83-
# The exact (non-`^`) pnpm override pins are what keep this exception
84-
# scoped in practice: revisit this entry if those pins ever change.
85-
# Remove once GitHub's advisory data reflects the backport, or once
86-
# minimatch ships a version compatible with brace-expansion 5.x's
87-
# export shape.
95+
# allow-dependencies-licenses is purl-scoped, and that's license-only),
96+
# and dependency-review-action has no SARIF/code-scanning output either,
97+
# so this is the only suppression mechanism it offers -- not a
98+
# workaround chosen over some cleaner supported path. The scoping that
99+
# exists is the exact override values in website/package.json's
100+
# pnpm.overrides (`brace-expansion@^1`/`@^2` selectors pinned to the
101+
# exact "1.1.18"/"2.1.4", not a range).
102+
#
103+
# Remove this entry once either: GHSA-mh99-v99m-4gvg's advisory data is
104+
# corrected to distinguish brace-expansion's 1.x/2.x/5.x release lines
105+
# (see https://github.com/advisories/GHSA-mh99-v99m-4gvg for a
106+
# firstPatchedVersion per major line instead of one "<=5.0.7" range), or
107+
# serve-handler and whatever still depends on minimatch@9.0.9 both move
108+
# to a minimatch version that depends on brace-expansion ^5 -- at that
109+
# point also remove the brace-expansion overrides in
110+
# website/package.json.
88111
allow-ghsas: GHSA-fxhp-mv3v-67qp, GHSA-mh99-v99m-4gvg
89112

90113
# Comment on PR with details
Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
# Fix: DAG-scheduled bulk terraform commands crash with `concurrent map iteration and map write`
2+
3+
**Date:** 2026-07-30
4+
5+
## Summary
6+
7+
Users running DAG-scheduled bulk terraform commands (`terraform <cmd> --all`, `--affected`, `--query`) with
8+
`--max-concurrency` set above 1 could hit a fatal `concurrent map iteration and map write` crash. The crash
9+
went away at low concurrency, which was the tell: it was a data race, not a logic bug. Fixed by shallow-cloning
10+
the shared, cached component-config map before any downstream code mutates it.
11+
12+
## Context
13+
14+
`FindStacksMap` (`internal/exec/utils.go`) caches fully-processed stack config in a package-global map and,
15+
on a cache hit, returns the cached tree **by reference**. The guarding `RWMutex` protects only the cache
16+
lookup, not the nested maps it hands out.
17+
18+
`ProcessComponentConfig` then extracted a component's section from that shared tree and assigned it (and its
19+
sub-sections) directly into the per-node `ConfigAndStacksInfo`, still aliasing the cache. Two things wrote into
20+
that shared section:
21+
22+
- `ProcessStacks` injecting `atmos_component`, `atmos_stack`, `workspace`, `sources`, `deps`, `deps_all`, etc.
23+
- `mergeGlobalAuthConfig`, which installs the merged `auth` section — and runs once per candidate stack, not
24+
just the eventual winner.
25+
26+
Meanwhile, `findComponentInStacks` makes every DAG worker evaluate the target component in *every* stack
27+
(not just its own), iterating each candidate's shared section along the way (env-filter loop, `componentConfigsEqual`
28+
deep-compare, template-context range, `ConvertToYAMLPreservingDelimiters` walk). With `--max-concurrency > 1`,
29+
the scheduler (`pkg/scheduler`) runs these workers concurrently, so one worker's write into a shared section
30+
raced with another worker's read/iteration of that same section — the exact crash reported.
31+
32+
The describe-stacks processor (`internal/exec/describe_stacks_component_processor.go`) already had one instance
33+
of the same class of bug fixed via a documented shallow clone; the audit for this fix found two further
34+
un-cloned in-place deletes there that corrupt the cache for later callers in the same process (not part of the
35+
crash itself, since describe runs during the serial planning phase, but a real bug once the cache is polluted).
36+
37+
## Changes
38+
39+
- `internal/exec/utils.go`: `ProcessComponentConfig` now shallow-clones (`maps.Clone`) the extracted component
40+
section immediately after extraction, before any sub-section derivation, `mergeGlobalAuthConfig`, or later
41+
mutation. All top-level writers now land on a private clone instead of the cache's own map. Documented the
42+
sharing contract on `FindStacksMap`'s doc comment (returned maps are shared by reference on a cache hit and
43+
must be treated as read-only).
44+
- `internal/exec/describe_stacks_component_processor.go`: applied the same shallow-clone-before-mutate fix to
45+
two adjacent sites that deleted keys (`imports`, `terraform_workspace_pattern`/`terraform_workspace_template`)
46+
from cache-owned maps in place.
47+
- `internal/exec/process_stacks_shared_cache_test.go` (new): two regression tests —
48+
`TestProcessStacksDoesNotMutateSharedStacksMapCache` (deterministic snapshot-equality check against the cache,
49+
no race detector required) and `TestProcessStacksConcurrentSharedCacheAccess` (16 goroutines running
50+
`ProcessStacks` across two stacks that share a component, reproducing the production interleaving under
51+
`-race`).
52+
53+
## Validation
54+
55+
- `go test ./internal/exec -run TestProcessStacksDoesNotMutateSharedStacksMapCache -count=1` — confirmed FAIL
56+
before the fix (cache polluted with `atmos_component`/`workspace`/etc.), PASS after.
57+
- `go test -race ./internal/exec -run TestProcessStacksConcurrentSharedCacheAccess -count=1` — confirmed FAIL
58+
(data race reported) before the fix, PASS after.
59+
- `go build ./...` — clean.
60+
- `go test ./internal/exec ./pkg/scheduler/... -count=1` — all pass (includes existing describe-stacks golden
61+
snapshot tests, confirming the two hardening clones don't change output).
62+
- `atmos lint --changed` — 0 issues.
63+
- `atmos test` (full suite): the `tests` package hit an unrelated, pre-existing local-only hang — this sandbox's
64+
podman auto-boots a vfkit VM during an emulator-preflight CLI test when Docker isn't running, which doesn't
65+
happen in CI (Docker present there). Verified pre-existing on this machine in a prior session; `pkg/container`
66+
is untouched by this change.
67+
68+
## Follow-ups
69+
70+
None.

internal/exec/describe_stacks_component_processor.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package exec
44
import (
55
"errors"
66
"fmt"
7+
"maps"
78
"slices"
89
"strings"
910

@@ -259,6 +260,11 @@ func (p *describeStacksProcessor) processStackFile(stackFileName string, stackMa
259260
stackManifestName := getStackManifestName(stackMap)
260261

261262
// Delete the stack-wide imports section (not needed in output).
263+
// Shallow-clone first: stackMap is owned by the shared FindStacksMap cache, and
264+
// deleting from it in place would strip `imports` (and thus `deps`) from every
265+
// subsequent ProcessStacks call in the same process (e.g., DAG-scheduled bulk
266+
// commands, which run ExecuteDescribeStacks before executing components).
267+
stackMap = maps.Clone(stackMap)
262268
delete(stackMap, "imports")
263269

264270
// When includeEmptyStacks is true, pre-create an entry in the result map so that
@@ -974,6 +980,11 @@ func applyTerraformMetadataInheritance(
974980
// regardless of whether an inherit list is present. This matches the original behaviour:
975981
// the cleanup ran unconditionally (outside the inheritList guard) in the old monolith.
976982
if _, hasExplicitWorkspace := metadataSection["terraform_workspace"].(string); hasExplicitWorkspace {
983+
// Shallow-clone first: unless the inheritance merge above already produced a
984+
// fresh map, metadataSection aliases the nested metadata map owned by the
985+
// shared FindStacksMap cache, and deleting from it in place would corrupt the
986+
// cache for every subsequent caller in the same process.
987+
metadataSection = maps.Clone(metadataSection)
977988
delete(metadataSection, "terraform_workspace_pattern")
978989
delete(metadataSection, "terraform_workspace_template")
979990
}
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
package exec
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"testing"
7+
8+
"github.com/stretchr/testify/require"
9+
10+
cfg "github.com/cloudposse/atmos/pkg/config"
11+
"github.com/cloudposse/atmos/pkg/schema"
12+
)
13+
14+
// writeMinimalStackFixture writes a minimal atmos.yaml plus a single stack
15+
// manifest into the current directory (expected to already be a t.TempDir()),
16+
// enough for ProcessStacks to resolve component config without needing real
17+
// component/terraform files.
18+
func writeMinimalStackFixture(t *testing.T, atmosYAML, stackYAML string) {
19+
t.Helper()
20+
21+
require.NoError(t, os.WriteFile("atmos.yaml", []byte(atmosYAML), 0o644))
22+
require.NoError(t, os.MkdirAll("stacks", 0o755))
23+
require.NoError(t, os.MkdirAll(filepath.Join("components", "terraform", "mock"), 0o755))
24+
require.NoError(t, os.WriteFile(filepath.Join("stacks", "dev.yaml"), []byte(stackYAML), 0o644))
25+
}
26+
27+
// TestProcessStacksPropagatesSpaceliftStackNameBuildError verifies that
28+
// ProcessStacks surfaces an error when BuildSpaceliftStackNameFromComponentConfig
29+
// fails, instead of silently ignoring it.
30+
//
31+
// The fixture needs a `name_pattern` (so the Directory-branch ContextPrefix
32+
// computation, which only consults the pattern, succeeds) and an `http` backend
33+
// (so BuildTerraformWorkspace short-circuits on `isWorkspacesEnabled` before its
34+
// own, separate `NameTemplate` check). `NameTemplate` is poisoned only after
35+
// InitCliConfig, so it never affects the stack file discovery that InitCliConfig
36+
// itself performs — only BuildSpaceliftStackNameFromComponentConfig's later,
37+
// independent `ProcessTmpl` call sees the malformed value.
38+
func TestProcessStacksPropagatesSpaceliftStackNameBuildError(t *testing.T) {
39+
t.Chdir(t.TempDir())
40+
t.Setenv("ATMOS_CLI_CONFIG_PATH", ".")
41+
42+
require.NoError(t, os.WriteFile("atmos.yaml", []byte(`
43+
base_path: "./"
44+
stacks:
45+
base_path: stacks
46+
included_paths:
47+
- "**/*"
48+
name_pattern: "{stage}"
49+
components:
50+
terraform:
51+
base_path: components/terraform
52+
`), 0o644))
53+
require.NoError(t, os.MkdirAll(filepath.Join("stacks", "dev"), 0o755))
54+
require.NoError(t, os.MkdirAll(filepath.Join("components", "terraform", "mock"), 0o755))
55+
require.NoError(t, os.WriteFile(filepath.Join("stacks", "dev", "main.yaml"), []byte(`
56+
vars:
57+
stage: dev
58+
components:
59+
terraform:
60+
mock:
61+
backend_type: http
62+
settings:
63+
spacelift:
64+
workspace_enabled: true
65+
vars:
66+
stage: dev
67+
`), 0o644))
68+
69+
info := schema.ConfigAndStacksInfo{
70+
ComponentFromArg: "mock",
71+
Stack: "dev/main",
72+
ComponentType: cfg.TerraformComponentType,
73+
}
74+
atmosConfig, err := cfg.InitCliConfig(info, true)
75+
require.NoError(t, err)
76+
require.Equal(t, "Directory", atmosConfig.StackType)
77+
78+
atmosConfig.Stacks.NameTemplate = "{{ .vars.broken"
79+
80+
ClearFindStacksMapCache()
81+
t.Cleanup(ClearFindStacksMapCache)
82+
83+
_, err = ProcessStacks(&atmosConfig, info, true, true, false, nil, nil)
84+
require.ErrorContains(t, err, "unclosed action")
85+
}
86+
87+
// TestProcessStacksPropagatesAtlantisProjectNameBuildError verifies that
88+
// ProcessStacks surfaces an error when BuildAtlantisProjectNameFromComponentConfig
89+
// fails to decode `settings.atlantis.project_template` (a string where a bool
90+
// field is expected), instead of silently ignoring it.
91+
func TestProcessStacksPropagatesAtlantisProjectNameBuildError(t *testing.T) {
92+
t.Chdir(t.TempDir())
93+
t.Setenv("ATMOS_CLI_CONFIG_PATH", ".")
94+
95+
writeMinimalStackFixture(t, `
96+
base_path: "./"
97+
stacks:
98+
base_path: stacks
99+
included_paths:
100+
- "**/*"
101+
components:
102+
terraform:
103+
base_path: components/terraform
104+
`, `
105+
vars:
106+
stage: dev
107+
components:
108+
terraform:
109+
mock:
110+
settings:
111+
atlantis:
112+
project_template:
113+
name: "mock-project"
114+
delete_source_branch_on_merge: "not-a-bool"
115+
vars:
116+
stage: dev
117+
`)
118+
119+
info := schema.ConfigAndStacksInfo{
120+
ComponentFromArg: "mock",
121+
Stack: "dev",
122+
ComponentType: cfg.TerraformComponentType,
123+
}
124+
atmosConfig, err := cfg.InitCliConfig(info, true)
125+
require.NoError(t, err)
126+
127+
ClearFindStacksMapCache()
128+
t.Cleanup(ClearFindStacksMapCache)
129+
130+
_, err = ProcessStacks(&atmosConfig, info, true, true, false, nil, nil)
131+
require.ErrorContains(t, err, "delete_source_branch_on_merge")
132+
}

0 commit comments

Comments
 (0)