Fix plan-diff command

[goruha]

What

Move the logic for reading from stdin (pipe input) into a separate goroutine.

Why

In practice, when Atmos reads large plan files in JSON format from stdin, the output can exceed the pipe buffer capacity. If the main thread does not consume this input concurrently, the buffer may become full, leading to a deadlock or the external command hanging.

By moving the read operation into a goroutine, we ensure that the pipe is drained concurrently, preventing buffer overflow and avoiding the process getting stuck.

Summary by CodeRabbit

• Refactor
  • Improved the reliability and resource management when capturing output from the Terraform command.
  • Minor cleanup for code formatting by removing extra blank lines.

[coderabbitai]

📝 Walkthrough

Walkthrough

The runTerraformShow function in internal/exec/terraform_plan_diff.go was refactored to enhance output capturing from the terraform show command. The update introduces deferred pipe closure, asynchronous buffer copying via a goroutine, and improved resource cleanup. Minor formatting adjustments were also made by removing extra blank lines.

Changes

Cohort / File(s)	Change Summary
Terraform Output Handling Refactor internal/exec/terraform_plan_diff.go	Refactored runTerraformShow to use deferred pipe closure, goroutine-based buffer copying, and improved synchronization; removed extra blank lines.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~7 minutes

Suggested labels

patch

Suggested reviewers

• aknysh

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.

---

📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 88fcc6d and 36442cf.

📒 Files selected for processing (1)

• internal/exec/terraform_plan_diff.go (2 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.go

📄 CodeRabbit Inference Engine (.cursor/rules/atmos-rules.mdc)

**/*.go: Use Viper for managing configuration, environment variables, and flags
Use interfaces for external dependencies to facilitate mocking
All code must pass golangci-lint checks
Follow Go's error handling idioms
Use meaningful error messages
Wrap errors with context using fmt.Errorf("context: %w", err)
Consider using a custom error type for domain-specific errors
Follow standard Go coding style
Use gofmt and goimports to format code
Prefer short, descriptive variable names
Use snake_case for environment variables
Document all exported functions, types, and methods
Document complex logic with inline comments
Follow Go's documentation conventions
Use Viper for configuration management
Support configuration via files, environment variables, and flags
Follow the precedence order: flags > environment variables > config file > defaults

Files:

• internal/exec/terraform_plan_diff.go

🧠 Learnings (2)

📓 Common learnings

Learnt from: Listener430
PR: cloudposse/atmos#934
File: tests/fixtures/scenarios/docs-generate/README.md.gotmpl:99-118
Timestamp: 2025-01-25T03:51:57.689Z
Learning: For the cloudposse/atmos repository, changes to template contents should be handled in dedicated PRs and are typically considered out of scope for PRs focused on other objectives.

Learnt from: osterman
PR: cloudposse/atmos#887
File: internal/exec/workflow_utils.go:167-169
Timestamp: 2024-12-25T20:28:19.618Z
Learning: The user plans to revert the change from `path.Join` to `filepath.Join` in this PR due to testing gaps and will open a new PR to safely handle the migration without breaking `main`.

Learnt from: aknysh
PR: cloudposse/atmos#944
File: go.mod:206-206
Timestamp: 2025-01-17T00:18:57.769Z
Learning: For indirect dependencies with license compliance issues in the cloudposse/atmos repository, the team prefers to handle them in follow-up PRs rather than blocking the current changes, as these issues often require deeper investigation of the dependency tree.

Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Clearly describe the changes in the PR description

Learnt from: aknysh
PR: cloudposse/atmos#768
File: internal/exec/vendor_model_component.go:3-20
Timestamp: 2024-11-18T13:59:10.824Z
Learning: When replacing significant dependencies like `go-getter` that require extensive changes, prefer to address them in separate PRs.

Learnt from: osterman
PR: cloudposse/atmos#808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like `pkg/config/config.go`, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.

Learnt from: osterman
PR: cloudposse/atmos#740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like `checkAtmosConfig` in `cmd/cmd_utils.go`, avoid suggesting refactoring to return errors instead of calling `os.Exit` if such changes would significantly increase the scope due to the need to update multiple call sites.

Learnt from: samtholiya
PR: cloudposse/atmos#914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of `os.Args[2:]` for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.

Learnt from: aknysh
PR: cloudposse/atmos#1185
File: internal/exec/yaml_func_store.go:26-26
Timestamp: 2025-04-04T02:03:23.676Z
Learning: The Atmos codebase currently uses `log.Fatal` for error handling in multiple places. The maintainers are aware this isn't an ideal pattern (should only be used in main() or init() functions) and plan to address it comprehensively in a separate PR. CodeRabbit should not flag these issues or push for immediate changes until that refactoring is complete.

Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Run golangci-lint and fix any issues before submitting a pull request

Learnt from: osterman
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like `confirmDeleteTerraformLocal` in `internal/exec/terraform_clean.go`, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.

Learnt from: aknysh
PR: cloudposse/atmos#863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the `processTagTerraformOutput` function within `internal/exec/yaml_func_terraform_output.go`, parameters are separated by spaces and do not contain spaces. Therefore, using `strings.Fields()` for parsing is acceptable, and there's no need to handle parameters with spaces.

Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2024-11-24T19:13:10.287Z
Learning: In `internal/exec/terraform_clean.go`, when `getStackTerraformStateFolder` returns an error in the `handleCleanSubCommand` function, the error is logged, and the process continues without returning the error.

Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In `internal/exec/terraform_clean.go`, the function `determineCleanPath` is necessary and should not be removed.

Learnt from: aknysh
PR: cloudposse/atmos#759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In `internal/exec/terraform.go`, the workspace cleaning code under both the general execution path and within the `case "init":` block is intentionally duplicated because the code execution paths are different. The `.terraform/environment` file should be deleted before executing `terraform init` in both scenarios to ensure a clean state.

Learnt from: aknysh
PR: cloudposse/atmos#810
File: internal/exec/terraform_utils.go:144-145
Timestamp: 2024-12-03T03:52:02.524Z
Learning: Avoid adding context timeouts to Terraform commands in `execTerraformOutput` because their execution time can vary from seconds to hours, and making it configurable would require redesigning the command interface.

Learnt from: aknysh
PR: cloudposse/atmos#810
File: internal/exec/yaml_func_terraform_output.go:35-40
Timestamp: 2024-11-30T22:07:08.610Z
Learning: In the Go function `processTagTerraformOutput` in `internal/exec/yaml_func_terraform_output.go`, parameters cannot contain spaces. The code splits the input by spaces, and if the parameters contain spaces, `len(parts) != 3` will fail and show an error to the user.

Learnt from: aknysh
PR: cloudposse/atmos#825
File: internal/exec/terraform.go:30-30
Timestamp: 2024-12-07T16:19:01.683Z
Learning: In `internal/exec/terraform.go`, skipping stack validation when help flags are present is not necessary.

Learnt from: haitham911
PR: cloudposse/atmos#727
File: internal/exec/terraform_clean.go:155-175
Timestamp: 2024-10-27T04:28:40.966Z
Learning: In the `CollectDirectoryObjects` function in `internal/exec/terraform_clean.go`, recursive search through all subdirectories is not needed.

internal/exec/terraform_plan_diff.go (16)

Learnt from: aknysh
PR: #863
File: internal/exec/yaml_func_terraform_output.go:34-38
Timestamp: 2024-12-17T07:08:41.288Z
Learning: In the processTagTerraformOutput function within internal/exec/yaml_func_terraform_output.go, parameters are separated by spaces and do not contain spaces. Therefore, using strings.Fields() for parsing is acceptable, and there's no need to handle parameters with spaces.

Learnt from: osterman
PR: #727
File: internal/exec/terraform_clean.go:233-235
Timestamp: 2024-10-31T19:25:41.298Z
Learning: When specifying color values in functions like confirmDeleteTerraformLocal in internal/exec/terraform_clean.go, avoid hardcoding color values. Instead, use predefined color constants or allow customization through configuration settings to improve accessibility and user experience across different terminals and themes.

Learnt from: samtholiya
PR: #914
File: cmd/terraform_commands.go:260-265
Timestamp: 2025-01-09T22:37:01.004Z
Learning: In the terraform commands implementation (cmd/terraform_commands.go), the direct use of os.Args[2:] for argument handling is intentionally preserved to avoid extensive refactoring. While it could be improved to use cobra's argument parsing, such changes should be handled in a dedicated PR to maintain focus and minimize risk.

Learnt from: aknysh
PR: #1327
File: cmd/terraform.go:111-117
Timestamp: 2025-06-23T02:14:30.937Z
Learning: In cmd/terraform.go, flags for the DescribeAffected function are added dynamically at runtime when info.Affected is true. This is intentional to avoid exposing internal flags like "file", "format", "verbose", "include-spacelift-admin-stacks", "include-settings", and "upload" in the terraform command interface, while still providing them for the shared DescribeAffected function used by both atmos describe affected and atmos terraform apply --affected.

Learnt from: haitham911
PR: #727
File: internal/exec/terraform_clean.go:215-223
Timestamp: 2024-10-27T04:41:49.199Z
Learning: In internal/exec/terraform_clean.go, the function determineCleanPath is necessary and should not be removed.

Learnt from: aknysh
PR: #810
File: internal/exec/yaml_func_terraform_output.go:35-40
Timestamp: 2024-11-30T22:07:08.610Z
Learning: In the Go function processTagTerraformOutput in internal/exec/yaml_func_terraform_output.go, parameters cannot contain spaces. The code splits the input by spaces, and if the parameters contain spaces, len(parts) != 3 will fail and show an error to the user.

Learnt from: haitham911
PR: #727
File: internal/exec/terraform_clean.go:407-416
Timestamp: 2024-11-24T19:13:10.287Z
Learning: In internal/exec/terraform_clean.go, when getStackTerraformStateFolder returns an error in the handleCleanSubCommand function, the error is logged, and the process continues without returning the error.

Learnt from: aknysh
PR: #759
File: internal/exec/terraform.go:366-368
Timestamp: 2024-11-02T15:35:09.958Z
Learning: In internal/exec/terraform.go, the workspace cleaning code under both the general execution path and within the case "init": block is intentionally duplicated because the code execution paths are different. The .terraform/environment file should be deleted before executing terraform init in both scenarios to ensure a clean state.

Learnt from: haitham911
PR: #727
File: internal/exec/terraform_clean.go:155-175
Timestamp: 2024-10-27T04:28:40.966Z
Learning: In the CollectDirectoryObjects function in internal/exec/terraform_clean.go, recursive search through all subdirectories is not needed.

Learnt from: aknysh
PR: #825
File: internal/exec/terraform.go:30-30
Timestamp: 2024-12-07T16:19:01.683Z
Learning: In internal/exec/terraform.go, skipping stack validation when help flags are present is not necessary.

Learnt from: CR
PR: cloudposse/atmos#0
File: .cursor/rules/atmos-rules.mdc:0-0
Timestamp: 2025-07-01T02:22:25.901Z
Learning: Applies to **/*_test.go : Test both happy paths and error conditions

Learnt from: Cerebrovinny
PR: #729
File: internal/exec/terraform.go:258-264
Timestamp: 2024-10-19T17:42:29.152Z
Learning: Prefer not to declare variables within 'if' statements in Go code.

Learnt from: osterman
PR: #740
File: cmd/cmd_utils.go:340-359
Timestamp: 2024-10-23T21:36:40.262Z
Learning: In the Go codebase for Atmos, when reviewing functions like checkAtmosConfig in cmd/cmd_utils.go, avoid suggesting refactoring to return errors instead of calling os.Exit if such changes would significantly increase the scope due to the need to update multiple call sites.

Learnt from: osterman
PR: #808
File: pkg/config/config.go:478-483
Timestamp: 2024-12-02T21:26:32.337Z
Learning: In the 'atmos' project, when reviewing Go code like pkg/config/config.go, avoid suggesting file size checks after downloading remote configs if such checks aren't implemented elsewhere in the codebase.

Learnt from: aknysh
PR: #1363
File: internal/exec/template_utils.go:18-18
Timestamp: 2025-07-05T20:59:02.914Z
Learning: In the Atmos project, gomplate v4 is imported with a blank import (_ "github.com/hairyhenderson/gomplate/v4") alongside v3 imports to resolve AWS SDK version conflicts. V3 uses older AWS SDK versions that conflict with newer AWS modules used by Atmos. A full migration to v4 requires extensive refactoring due to API changes and should be handled in a separate PR.

Learnt from: aknysh
PR: #775
File: internal/exec/template_funcs_component.go:157-159
Timestamp: 2024-11-12T03:16:02.910Z
Learning: In the Go code for componentFunc in internal/exec/template_funcs_component.go, the function cleanTerraformWorkspace does not return errors, and it's acceptable if the file does not exist. Therefore, error handling for cleanTerraformWorkspace is not needed.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: Build (macos-latest, macos)
• GitHub Check: Build (windows-latest, windows)
• GitHub Check: Build (ubuntu-latest, linux)
• GitHub Check: Lint (golangci)
• GitHub Check: Analyze (go)
• GitHub Check: Summary

🔇 Additional comments (2)

internal/exec/terraform_plan_diff.go (2)

4-4: LGTM!

The bytes import is appropriately added to support the new buffer-based output capturing.

---

251-289: Excellent refactoring to prevent buffer overflow!

The async pipe reading implementation properly addresses the buffer overflow issue with large JSON plan files. The changes include:

• Proper resource cleanup with deferred pipe closures
• Goroutine-based async reading to prevent blocking
• Clean synchronization using channels
• Separate error handling for command execution and reading

This should resolve the hanging issues described in the PR objectives.

✨ Finishing Touches

• 📝 Generate Docstrings

🧪 Generate unit tests

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch plan-diff

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Explain this complex logic.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai explain this code block.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and explain its main purpose.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
• @coderabbitai generate unit tests to generate unit tests for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary or @auto-summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai or @auto-title anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.01%. Comparing base (c29c2c8) to head (1057886).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1382      +/-   ##
==========================================
- Coverage   54.19%   54.01%   -0.18%     
==========================================
  Files         259      259              
  Lines       27164    27167       +3     
==========================================
- Hits        14721    14674      -47     
- Misses      10736    10796      +60     
+ Partials     1707     1697      -10     

Flag	Coverage Δ
unittests	54.01% <100.00%> (-0.18%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[osterman]

Please add tests, ideally with the original failure case that is validated by fixes in this PR

[goruha]

Please add tests, ideally with the original failure case that is validated by fixes in this PR
@osterman, there are tests for terraform plan-diff. but

https://github.com/cloudposse/atmos/blob/plan-diff/internal/exec/terraform_plan_diff_test.go#L391

I can not write test for pipe buffer overflow easily

[github-actions]

These changes were released in v1.184.0.