Support for a circuit breaker to avoid recursive calls to atmos commands (infinite loop) #906
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a circuit breaker mechanism to prevent infinite loops in Atmos CLI custom commands. A new constant Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/exec/shell_utils.go (2)
27-46: Great approach for handling shell levels.
The logic cleanly enforces the depth limit. However, consider reusinggetNextShellLevel()inexecTerraformShellCommandto avoid duplicating the environment variable parsing.
58-63: Solid environment variable update.
UpdatingATMOS_SHLVLhere is consistent with the new approach. Also unify with the Terraform shell logic for consistency.examples/demo-custom-command/atmos.yaml (1)
73-79: Neat demonstration of recursion.
The new “loop” command clearly shows how to trigger and test the circuit breaker. Consider adding a quick note in documentation to warn about potential infinite loops.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/demo-custom-command/atmos.yaml(1 hunks)internal/exec/shell_utils.go(4 hunks)
🔇 Additional comments (3)
internal/exec/shell_utils.go (3)
24-26: Nicely done with the constant definition.
A dedicated constant clarifies the maximum allowed depth.
116-121: Straightforward usage.
Happy to see consistency in retrieving the shell level.
143-148: Consistent shell level handling.
UsinggetNextShellLevel()keeps the recursion check uniform across methods.
|
💥 This pull request now has conflicts. Could you fix it @Listener430? 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/tests/atmos.yaml (1)
99-105: Improve command documentation and fix typo.The command's documentation could be more informative, and there's a typo in the description.
Apply this diff to enhance the documentation:
# test for infinite loop - name: loop - description: This command tests curcuit breaker for infinite loop + description: | + This command tests the circuit breaker mechanism by creating an infinite loop. + WARNING: This command intentionally creates an infinite loop that will be + terminated by the circuit breaker when shell depth exceeds 10. steps: - "echo Hello world!" - atmos loop
|
These changes were released in v1.136.0. |
what
This PR contains the code that counts the number of recursive calls, and in case it exceeds the threshold, terminates the processes (parent + all children).
Add-on to atmos.yaml
The circuit breaker in use
why
Avoid infinite loops
references
Summary by CodeRabbit
New Features
loopfor testing infinite loops in the CLI.Tests