Repository navigation
fix(dag): stop concurrent map crash in bulk terraform commands - #2831
Merged
Merged
Conversation
… 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>
…9-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>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
July 30, 2026 13:36
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Contributor
📝 WalkthroughWalkthroughProcessStacks and describe-stacks now avoid mutating shared cached maps, preserve non-templated sections, and populate derived fields before template processing. Regression tests cover cache safety, concurrency, and builder errors. Dependency-review documentation is expanded while executable version checking is removed. ChangesShared cache safety
Dependency review documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProcessStacks
participant FindStacksMap
participant ProcessComponentConfig
participant TemplateProcessor
ProcessStacks->>FindStacksMap: read cached stack configuration
FindStacksMap-->>ProcessStacks: return shared map
ProcessStacks->>ProcessComponentConfig: clone component section
ProcessComponentConfig-->>ProcessStacks: return isolated component data
ProcessStacks->>TemplateProcessor: render templated sections
TemplateProcessor-->>ProcessStacks: restore non-templated sections
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 30, 2026 13:36 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 30, 2026 13:37 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 30, 2026 13:38 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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>
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 30, 2026 13:45 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
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>
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 30, 2026 15:20 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/exec/process_stacks_shared_cache_test.go (1)
20-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the fixture path with
filepath.Join.Proposed fix
import ( + "path/filepath" "sync" "testing" @@ - testDir := "../../tests/fixtures/scenarios/stack-manifest-name-template" + testDir := filepath.Join("..", "..", "tests", "fixtures", "scenarios", "stack-manifest-name-template")As per coding guidelines, tests must use
filepath.Join()for filesystem paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/exec/process_stacks_shared_cache_test.go` around lines 20 - 21, Update the test fixture path assignment before t.Chdir in the relevant test to construct the path with filepath.Join instead of a hardcoded slash-separated string, and ensure the filepath package is imported.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/dependency-review.yml:
- Line 81: Update the dependency-review configuration around the allow-ghsas
entry to scope the GHSA-mh99-v99m-4gvg exception to the verified brace-expansion
versions covered by the pinned ^1 and ^2 overrides. Add a lock/version assertion
or equivalent validation so future brace-expansion 3.x/4.x resolutions cannot
bypass the vulnerability check.
---
Nitpick comments:
In `@internal/exec/process_stacks_shared_cache_test.go`:
- Around line 20-21: Update the test fixture path assignment before t.Chdir in
the relevant test to construct the path with filepath.Join instead of a
hardcoded slash-separated string, and ensure the filepath package is imported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bbfc93ca-d5b4-461d-8e02-3c025968fc33
⛔ Files ignored due to path filters (1)
website/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/workflows/dependency-review.ymldocs/fixes/2026-07-30-dag-concurrent-map-crash.mdinternal/exec/describe_stacks_component_processor.gointernal/exec/process_stacks_shared_cache_test.gointernal/exec/utils.gotests/snapshots/TestCLICommands_describe_component_with_component_name_(backward_compatibility).stdout.goldentests/snapshots/TestCLICommands_describe_component_with_current_directory_(.).stdout.goldentests/snapshots/TestCLICommands_describe_component_with_relative_path.stdout.goldenwebsite/package.json
💤 Files with no reviewable changes (1)
- tests/snapshots/TestCLICommands_describe_component_with_component_name_(backward_compatibility).stdout.golden
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2831 +/- ##
==========================================
+ Coverage 81.90% 81.94% +0.03%
==========================================
Files 1798 1798
Lines 173914 173917 +3
==========================================
+ Hits 142450 142513 +63
+ Misses 23688 23634 -54
+ Partials 7776 7770 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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>
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 30, 2026 17:10 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/dependency-review.yml:
- Around line 41-45: Update the found-assignment pipeline in the
dependency-review workflow so grep’s expected status 1 for no matches is
converted to empty input, allowing the existing “No brace-expansion entries
found” diagnostic to run. Preserve nonzero failures caused by unreadable or
otherwise invalid input, while keeping the current allowlist guidance unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e1a3d7f-0df6-47a4-a221-d30fa7d835f5
📒 Files selected for processing (2)
.github/workflows/dependency-review.ymlinternal/exec/process_stacks_builder_errors_test.go
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>
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 30, 2026 18:16 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
…nt-map-fix # Conflicts: # .github/workflows/dependency-review.yml # website/package.json # website/pnpm-lock.yaml
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 30, 2026 22:13 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/dependency-review.yml (1)
41-45: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCheck every brace-expansion lockfile version, not only
X.Y.Zversions.The current regex skips valid keys like
brace-expansion@1.1.18-beta.0; sinceallow-ghsassuppresses the advisory for every resolved version, such a key would not fail this check and can bypass the scoped pin. Extract everybrace-expansion@...key and compare the full version against the allowlist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/dependency-review.yml around lines 41 - 45, The version extraction in the found assignment must match every brace-expansion lockfile key, including prerelease and other valid semver forms, rather than only numeric X.Y.Z versions. Update the grep pattern and normalization around found to capture the complete version after brace-expansion@, then compare those full versions against the existing allowlist while preserving deduplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/dependency-review.yml:
- Around line 112-121: Update the comment near the brace-expansion exception to
replace “exact (non-^) pnpm override pins” with “exact override values,”
accurately distinguishing the selectors from their pinned target values while
preserving the rest of the scoping explanation.
---
Outside diff comments:
In @.github/workflows/dependency-review.yml:
- Around line 41-45: The version extraction in the found assignment must match
every brace-expansion lockfile key, including prerelease and other valid semver
forms, rather than only numeric X.Y.Z versions. Update the grep pattern and
normalization around found to capture the complete version after
brace-expansion@, then compare those full versions against the existing
allowlist while preserving deduplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5be0723b-af84-4cb0-8ea9-005716db9398
📒 Files selected for processing (3)
.github/workflows/dependency-review.ymlinternal/exec/describe_stacks_component_processor.gointernal/exec/utils.go
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>
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 30, 2026 22:48 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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.
Erik Osterman (Cloud Posse) (osterman)
temporarily deployed
to
screengrabs
July 31, 2026 00:44 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/dependency-review.yml (1)
68-111: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAutomated brace-expansion version check removed — unscoped suppression now has no guardrail.
The prior fix added an executable step verifying
website/pnpm-lock.yamlresolves only the audited1.1.18/2.1.4versions before trustingallow-ghsas. This PR removes that check and keeps only documentation. Sinceallow-ghsas: GHSA-mh99-v99m-4gvgsuppresses the advisory for any resolved brace-expansion version (as the comment itself notes on lines 92-97), a future dependency bump resolving an unpatched version would now silently pass CI — the only thing catching that regression was the removed script.Consider restoring the version-assertion step (with the earlier no-match fix from commit 0a487b3 applied) alongside the documentation, so the suppression stays backed by an automated guarantee rather than just a comment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/dependency-review.yml around lines 68 - 111, Restore the executable dependency-review guard that inspects website/pnpm-lock.yaml and fails unless brace-expansion resolves only the audited 1.1.18 and 2.1.4 versions, applying the no-match handling from commit 0a487b3. Keep the existing allow-ghsas documentation, and place the assertion alongside the allow-ghsas configuration so future unpatched resolutions cannot be silently suppressed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.github/workflows/dependency-review.yml:
- Around line 68-111: Restore the executable dependency-review guard that
inspects website/pnpm-lock.yaml and fails unless brace-expansion resolves only
the audited 1.1.18 and 2.1.4 versions, applying the no-match handling from
commit 0a487b3. Keep the existing allow-ghsas documentation, and place the
assertion alongside the allow-ghsas configuration so future unpatched
resolutions cannot be silently suppressed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 17f72619-a9b3-407d-a040-b762174f765b
📒 Files selected for processing (1)
.github/workflows/dependency-review.yml
Andriy Knysh (aknysh)
temporarily deployed
to
screengrabs
July 31, 2026 03:18 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Andriy Knysh (aknysh)
approved these changes
Jul 31, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.225.0-rc.4. |
This was referenced Aug 6, 2026
This branch was successfully deployed
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
2 participants
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.
what
fatal error: concurrent map iteration and map writecrash in DAG-scheduled bulk terraform commands (terraform <cmd> --all/--affected/--query) at higher--max-concurrency.ProcessComponentConfignow shallow-clones the component section before any downstream code mutates it, so concurrent workers never write into the map tree owned by the sharedFindStacksMapcache.imports, andterraform_workspace_pattern/terraform_workspace_template, from cache-owned maps in place).internal/exec/process_stacks_shared_cache_test.go) that fail pre-fix both deterministically and under-race.brace-expansionpnpm.overrides(website) to1.1.18/2.1.4, patchingCVE-2026-14257/GHSA-mh99-v99m-4gvg(high-severity DoS via unbounded expansion length), reported by Dependabot alert Documented ADRs for Atmos #261.why
FindStacksMapcaches processed stack config and returns it by reference on cache hits, shared across all goroutines within a process.ProcessStacksandmergeGlobalAuthConfigwrite top-level keys into that shared component section, whilefindComponentInStackshas every DAG worker iterate every stack's component section (not just its own) looking for a match — so one worker's write races with another worker's read/iteration of the same cached map, crashing exactly as reported.ProcessStackscall in the same process even outside the crash path.brace-expansionbump addresses an open, high-severity Dependabot alert; deferring it risks a DoS crash if attacker-influenced input reaches an affected glob/brace-pattern code path in the docs site tooling.references
Summary by CodeRabbit
Bug Fixes
describeoutput to include referenced imports consistently.Documentation