fix(yaml-functions): detect cross-component !terraform.state cycles instead of stack-overflowing#2533
Merged
Andriy Knysh (aknysh) merged 5 commits intoMay 28, 2026
Conversation
…nstead of stack-overflowing Two components that reference each other via !terraform.state (A → B, B → A) drove atmos into infinite recursion ending in a goroutine stack overflow on `describe affected` / `describe component` / `terraform plan`. The YAML-function cycle detector existed and worked for cycles inside a single ProcessCustomYamlTags walk, but ProcessCustomYamlTags was installing a fresh, scoped ResolutionContext on every entry via scopedResolutionContext(). Resolving !terraform.state recurses through GetTerraformState → ExecuteDescribeComponent → ProcessStacks → ProcessCustomYamlTags, and each re-entry started over with an empty Visited map, so the outer walk's in-progress components were invisible to the inner walk and the cycle was unrecoverable. The fix: - ProcessCustomYamlTags now reuses the goroutine-local ResolutionContext so the Visited map survives across nested walks. The Push/Pop discipline in processTagTerraformState*/processTagTerraformOutput* already pairs every successful Push with a deferred Pop, so the context is empty when the top-level walk returns. - Added MaxResolutionDepth (= 64) as defense-in-depth: if any future re-entry path bypasses the cycle detector, Push refuses to grow past 64 frames and returns ErrYamlFuncMaxResolutionDepth rather than letting the Go runtime stack overflow. - GetTerraformState's describe-error wrap now uses double %w so errors.Is can match the propagated sentinel (e.g., ErrCircularDependency) through the descriptive wrapper. Tests: - New regression test tests/yaml_functions_circular_deps_integration_test.go exercises the full ExecuteDescribeComponent path on a fixture with an A↔B cycle and asserts ErrCircularDependency comes back (and not the depth safety net, which would indicate the cycle detector regressed). - Removed internal/exec/yaml_func_circular_deps_test.go — all four tests in it were t.Skip()-ed placeholders referencing fixtures that don't exist; the new integration test replaces them. Closes cloudposse#2457
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a MaxResolutionDepth constant and ErrYamlFuncMaxResolutionDepth sentinel; enforces the depth guard in ResolutionContext.Push; removes the scoped resolution context so goroutine-local cycle-detection state persists across nested YAML-tag resolution; preserves inner sentinel errors when wrapping ExecuteDescribeComponent failures; and adds fixtures + an integration test reproducing a cross-component terraform.state cycle. ChangesCircular dependency safeguard for YAML function resolution
Sequence Diagram(s)sequenceDiagram
participant Test
participant ExecuteDescribeComponent
participant YAMLFunctionResolver
participant ResolutionContext
participant TerraformStateStore
Test->>ExecuteDescribeComponent: describe component-a with YAML tags
ExecuteDescribeComponent->>YAMLFunctionResolver: resolve !terraform.state
YAMLFunctionResolver->>ResolutionContext: Push(node)
ResolutionContext->>TerraformStateStore: fetch outputs of other component
TerraformStateStore-->>YAMLFunctionResolver: outputs (may trigger nested resolves)
YAMLFunctionResolver->>ResolutionContext: Push(nested node)
ResolutionContext-->>YAMLFunctionResolver: ErrCircularDependency or ErrYamlFuncMaxResolutionDepth
YAMLFunctionResolver-->>ExecuteDescribeComponent: return wrapped error
ExecuteDescribeComponent-->>Test: propagate wrapped error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 2
🤖 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 `@tests/fixtures/scenarios/yaml-functions-circular-deps/atmos.yaml`:
- Line 14: In atmos.yaml replace the hardcoded Unix-only entry file:
"/dev/stderr" (seen on line containing file: "/dev/stderr") with a
platform-neutral logging configuration: either remove the file key so the
fixture uses the default logger, or set it to a cross-platform target (e.g., a
logical "stderr" sink) or an environment-driven value; update the fixture so
tests don’t rely on the literal "/dev/stderr".
In `@tests/yaml_functions_circular_deps_integration_test.go`:
- Line 33: Replace the hard-coded path string passed to t.Chdir with an
OS-neutral path built using filepath.Join: locate the test call to
t.Chdir("./fixtures/scenarios/yaml-functions-circular-deps") and change it to
use filepath.Join(".", "fixtures", "scenarios", "yaml-functions-circular-deps");
also import the "path/filepath" package if not already imported so the test
compiles.
🪄 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
Run ID: b4347e4d-7944-4389-ab59-43d27a8e67b7
📒 Files selected for processing (8)
errors/errors.gointernal/exec/terraform_state_utils.gointernal/exec/yaml_func_circular_deps_test.gointernal/exec/yaml_func_resolution_context.gointernal/exec/yaml_func_utils.gotests/fixtures/scenarios/yaml-functions-circular-deps/atmos.yamltests/fixtures/scenarios/yaml-functions-circular-deps/stacks/test.yamltests/yaml_functions_circular_deps_integration_test.go
💤 Files with no reviewable changes (1)
- internal/exec/yaml_func_circular_deps_test.go
…loudposse#2457 regression test - Drop the Unix-only `/dev/stderr` log target from the fixture's atmos.yaml; the regression test doesn't depend on log routing. - Use `filepath.Join` for the `t.Chdir` fixture path so the test runs on Windows runners as well as Linux/macOS (per CLAUDE.md cross-platform rule). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
May 28, 2026
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (11.11%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2533 +/- ##
=======================================
Coverage 78.59% 78.59%
=======================================
Files 1145 1145
Lines 110311 110305 -6
=======================================
- Hits 86699 86698 -1
+ Misses 18804 18798 -6
- Partials 4808 4809 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Andriy Knysh (aknysh)
merged commit 7ada7db
into
cloudposse:main
62 of 66 checks passed
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.220.0. |
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
Fix the goroutine stack overflow reported in #2457: two components that reference each other via
!terraform.state(A → B, B → A) droveatmos describe affected/describe component/terraform planinto infinite recursion until the Go runtime stack overflowed.The YAML-function cycle detector already existed and worked within a single
ProcessCustomYamlTagswalk, but it didn't survive the recursive describe path that!terraform.statetriggers.why
When a component is being processed and the resolver encounters
!terraform.state, it does:ProcessCustomYamlTagswas wrapping every entry withscopedResolutionContext(), which saved the parent's context and installed a fresh, empty one. So when the inner walk found B's!terraform.state a ..., the cycle detector'sVisitedmap had no record that A was already in progress, and it pushed A → B → A → B forever until the goroutine stack hit its 1 GB cap.The cycle detector unit tests pass because they exercise
Push/Popon a single context; the only integration tests that would have caught this weret.Skip()-ed placeholders ininternal/exec/yaml_func_circular_deps_test.goreferencing fixtures that don't exist.how
Three coordinated changes:
internal/exec/yaml_func_utils.go—ProcessCustomYamlTagsnow reuses the goroutine-localResolutionContextviaGetOrCreateResolutionContext()and drops thescopedResolutionContext()wrap. ThePush/Popdiscipline inprocessTagTerraformStateWithContext/trackOutputDependencyalready pairs every successfulPushwith a deferredPop, so the context is empty when the top-level walk returns. Removed the now-unusedscopedResolutionContexthelper.internal/exec/yaml_func_resolution_context.go— AddedMaxResolutionDepth = 64and a depth check inPushthat returnsErrYamlFuncMaxResolutionDepthif any future re-entry path slips past the cycle detector. This is belt-and-suspenders: real cycles are caught by theVisitedcheck; the depth bound exists so atmos surfaces a clean error instead of stack-overflowing if the detector regresses.internal/exec/terraform_state_utils.go—GetTerraformState's describe-error wrap now uses double%wsoerrors.Iscan match a propagated sentinel likeErrCircularDependencythrough the descriptive wrapper. Without this, the cycle error message is human-readable buterrors.Is(err, ErrCircularDependency)returnsfalse, breaking callers that try to handle the error programmatically.tests
tests/yaml_functions_circular_deps_integration_test.goplus fixture attests/fixtures/scenarios/yaml-functions-circular-deps/— exercises the fullExecuteDescribeComponentpath on an A↔B cycle and assertsErrCircularDependencycomes back (and not the depth safety net, which would indicate the cycle detector regressed). Test completes in ~20 ms instead of running forever.internal/exec/yaml_func_circular_deps_test.go— all four tests in it weret.Skip()-ed placeholders referencing fixtures that don't exist. The new integration test replaces them with one that actually runs.TestResolutionContext*unit tests still pass unchanged.references
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Fixtures