uncomment ECS circuit breaker#666
Merged
Merged
Conversation
ms280690
requested review from
Copilot,
gridcell,
michaelconnor00 and
saanobhaai
Contributor
|
Warning Review limit reached
More reviews will be available in 58 minutes and 23 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughECS deployment circuit breaker was enabled (DeploymentCircuitBreaker enable=True, rollback=True) for ECS services in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 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 |
There was a problem hiding this comment.
Pull request overview
This PR re-enables the ECS deployment circuit breaker on the CDK-defined ECS services in iac/stacks/api.py, allowing failed deployments to automatically trigger rollbacks for improved deployment safety in the Mermaid API infrastructure stack.
Changes:
- Enabled
ecs.DeploymentCircuitBreaker(enable=True, rollback=True)for theSummaryCacheServiceECS service. - Enabled
ecs.DeploymentCircuitBreaker(enable=True, rollback=True)for theApiServiceECS service.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
iac/stacks/api.py (1)
337-364: Consider enabling circuit breaker for QueueWorker services for consistency.
ApiServiceandSummaryCacheServicenow have deployment circuit breakers enabled, but theQueueWorkerconstruct (used for "General" and "ImageProcess" workers) still hascircuit_breakercommented out iniac/stacks/constructs/worker.py:61. This means worker services will remain in a broken state on deployment failure while API services auto-rollback.If the inconsistency is intentional (e.g., different rollback strategies for queue workers), consider documenting the rationale. Otherwise, enabling circuit breakers for workers would provide uniform deployment resilience.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@iac/stacks/api.py` around lines 337 - 364, The QueueWorker instances ("worker" and "image_worker") are missing deployment circuit breaker configuration; update the QueueWorker instantiation for both "General" and "ImageProcess" to enable the circuit breaker by passing the appropriate circuit_breaker/deployment_circuit_breaker argument (matching the construct's parameter name used in worker.py where it's currently commented out, e.g., circuit_breaker=True or deployment_circuit_breaker=ecs.DeploymentCircuitBreaker(enabled=True)); ensure both QueueWorker calls include this argument so worker services auto-rollback consistently with ApiService and SummaryCacheService.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@iac/stacks/api.py`:
- Around line 337-364: The QueueWorker instances ("worker" and "image_worker")
are missing deployment circuit breaker configuration; update the QueueWorker
instantiation for both "General" and "ImageProcess" to enable the circuit
breaker by passing the appropriate circuit_breaker/deployment_circuit_breaker
argument (matching the construct's parameter name used in worker.py where it's
currently commented out, e.g., circuit_breaker=True or
deployment_circuit_breaker=ecs.DeploymentCircuitBreaker(enabled=True)); ensure
both QueueWorker calls include this argument so worker services auto-rollback
consistently with ApiService and SummaryCacheService.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ebbd44a0-9a2e-4564-8955-9d51e32443c0
📒 Files selected for processing (1)
iac/stacks/api.py
Signed-off-by: ms280690 <mehul@sparkgeo.com>
Collaborator
|
Please move the CDK diff to a comment. When I originally added this, it caused an issue with deployment, which is why it was commented out. I am a little disappointed I did not leave a comment as reference, and I can't find an old Trello ticket for it. After merging to dev, we will need to run a test to verify. The test should:
I have a vague memory that the issue was that the ECS containers did not have a health checks and the circuit breaker relys on failed health checks. |
cdk-nag report✅ No unsuppressed errors. See |
|
michaelconnor00
approved these changes
Apr 30, 2026
Collaborator
michaelconnor00
left a comment
michaelconnor00
left a comment
There was a problem hiding this comment.
Merge this one last and run the tests suggested in the comments.
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
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.
https://trello.com/c/Ruklqx62
Summary by CodeRabbit