⏪ undo LocalStack legacy CloudFormation engine workaround by sodre · Pull Request #86 · zeroae/zae-limiter

sodre · 2026-01-12T21:05:59Z

Summary

This reverts commit 1bbc649 (PR #85) which added the legacy CloudFormation engine workaround.

Rationale: Prefer using LocalStack's v2 CloudFormation engine (the default) rather than the legacy engine. While v2 has a stack deletion bug with conditional resources, the legacy engine has its own bug with CloudWatch Alarm Threshold parameters.

The Dilemma

LocalStack has two CloudFormation engine implementations, each with different bugs:

Engine	Stack Deletion	CloudWatch Alarms
v2 (default)	❌ Fails with "Unresolved resource dependencies"	✅ Works
Legacy	✅ Works	❌ Fails (Threshold passed as string)

Decision

The v2 engine is the future of LocalStack CloudFormation support. Rather than permanently work around the v2 bug with the legacy engine (which has its own issues), we should:

Track the upstream bug: bug: CloudFormation v2 engine fails to delete stack with conditional resource references localstack/localstack#13609
Use try/except workarounds in test cleanup until LocalStack fixes v2
Accept that some integration test cleanup may fail gracefully

Re-opens

This PR re-opens #81 for tracking until LocalStack fixes the v2 engine bug.

Test plan

Unit tests pass (no LocalStack dependency)
Integration tests may have cleanup failures (expected with v2 bug)

🤖 Generated with Claude Code

codecov · 2026-01-12T21:07:49Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.17%. Comparing base (1bbc649) to head (cfa2cb7).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #86      +/-   ##
==========================================
- Coverage   87.37%   87.17%   -0.21%     
==========================================
  Files          18       18              
  Lines        1933     1933              
==========================================
- Hits         1689     1685       -4     
- Misses        244      248       +4

Flag	Coverage Δ
integration	`56.75% <ø> (+0.20%)`	⬆️

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.

github-actions

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.30.

Benchmark suite	Current: `41a577f`	Previous: `1bbc649`	Ratio
`tests/test_performance.py::TestCascadeOverheadBenchmarks::test_acquire_with_cascade`	`27.218195583238636` iter/sec (`stddev: 0.07316920017406905`)	`35.73282947749318` iter/sec (`stddev: 0.09186254420360597`)	`1.31`

This comment was automatically generated by workflow using github-action-benchmark.

This reverts commit 1bbc649. Prefer using LocalStack's v2 CloudFormation engine (the default) rather than the legacy engine. While v2 has a stack deletion bug with conditional resources, the legacy engine has its own bug with CloudWatch Alarm Threshold parameters. The v2 engine is the future of LocalStack CloudFormation support, and we should track the upstream bug rather than permanently work around it. Upstream issue: localstack/localstack#13609 Re-opens #81 for tracking until LocalStack fixes the v2 engine. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

claude · 2026-01-12T21:13:03Z

Code Review

Found 3 issues that need attention. See inline comments below.

claude · 2026-01-12T21:13:17Z

Issue 1: CloudFormation Bug - Condition Mismatch

File: src/zae_limiter/infra/cfn_template.yaml (line 497)

The AggregatorDLQAlarmName output has Condition: DeployAggregator but references !Ref AggregatorDLQAlarm which has Condition: DeployAggregatorAlarms (alarm only created when BOTH aggregator AND alarms enabled).

Bug scenario:

When EnableAggregator=true and EnableAlarms=false
The output will exist (DeployAggregator=true)
But references a non-existent alarm (DeployAggregatorAlarms=false)
CloudFormation deployment fails with "Unresolved resource dependencies"

Fix: Change line 497 back to Condition: DeployAggregatorAlarms

References:

Condition definitions:

zae-limiter/src/zae_limiter/infra/cfn_template.yaml

Lines 106 to 113 in a2f4f85

    
           Conditions: 
        
             DeployAggregator: !Equals [!Ref EnableAggregator, 'true'] 
        
             UsePITRRecoveryPeriod: !Not [!Equals [!Ref PITRRecoveryPeriodDays, '']] 
        
             DeployAlarms: !Equals [!Ref EnableAlarms, 'true'] 
        
             DeployAggregatorAlarms: !And 
        
               - !Condition DeployAggregator 
        
               - !Condition DeployAlarms 
        
             HasSNSTopic: !Not [!Equals [!Ref AlarmSNSTopicArn, '']]

AggregatorDLQAlarm resource:

zae-limiter/src/zae_limiter/infra/cfn_template.yaml

Lines 197 to 213 in a2f4f85

    
           AggregatorDLQAlarm: 
        
             Type: AWS::CloudWatch::Alarm 
        
             Condition: DeployAggregatorAlarms 
        
             Properties: 
        
               AlarmName: !Sub ${TableName}-aggregator-dlq-alarm 
        
               AlarmDescription: Alert when messages appear in the aggregator DLQ 
        
               MetricName: ApproximateNumberOfMessagesVisible 
        
               Namespace: AWS/SQS 
        
               Statistic: Sum 
        
               Period: 300  # 5 minutes 
        
               EvaluationPeriods: 1 
        
               Threshold: 1 
        
               ComparisonOperator: GreaterThanOrEqualToThreshold 
        
               Dimensions: 
        
                 - Name: QueueName 
        
                   Value: !GetAtt AggregatorDLQ.QueueName 
        
               TreatMissingData: notBreaching

CLAUDE.md Infrastructure guidelines:

zae-limiter/CLAUDE.md

Line 242 in a2f4f85

claude · 2026-01-12T21:13:27Z

Issue 2: Test Coverage Reduction

File: tests/test_integration_localstack.py (line 34)

The test changed from using aggregator_stack_options (aggregator=true, alarms=false) to full_stack_options (aggregator=true, alarms=true). This removes test coverage for the aggregator-without-alarms scenario, which is a valid configuration that should be tested.

Impact:

The aggregator_stack_options fixture still exists in conftest.py but is no longer used by any test
This edge case (aggregator enabled, alarms disabled) is now untested
Would have caught the CloudFormation bug in Issue ⬆️ Bump astral-sh/setup-uv from 4 to 7 #1

Fix: Either:

Keep this test using aggregator_stack_options and add a separate test for full_stack_options, OR
Add a new test that uses aggregator_stack_options

CLAUDE.md Reference:

Test Coverage (changes to src/): Verify corresponding tests exist in tests/ ... Check edge cases: negative values, empty collections, None

Ref:

zae-limiter/CLAUDE.md

Lines 226 to 229 in a2f4f85

    
           ### Token Bucket Algorithm 
        
           - Buckets can go **negative** for post-hoc reconciliation 
        
           - Refill is calculated lazily on each access

claude · 2026-01-12T21:13:42Z

Issue 3: Test Deletion Without Replacement

File: tests/test_stack_manager.py (deleted test_stack_create_and_delete_minimal)

The test test_stack_create_and_delete_minimal was completely removed. This was the only integration test that verified full stack lifecycle (create + delete) with LocalStack.

Analysis of remaining coverage:

test_delete_stack_nonexistent: Only tests deleting non-existent stacks (edge case)
TestDeleteStack class: Unit tests with mocks, not integration tests
test_create_and_delete_stack_real_aws: Marked as @pytest.mark.skip, never runs in CI
test_cloudformation_stack_deployment: Only tests creation via async with limiter (doesn't delete stacks, only closes connections)

Issue:
The PR description mentions handling "cleanup failures gracefully," but instead of adding graceful error handling (try/except), the test was deleted entirely, eliminating coverage for stack deletion.

Fix: Instead of deleting the test, wrap the delete_stack() call in try/except to handle the known v2 engine deletion bug gracefully:

try:
    await manager.delete_stack(stack_name, wait=True)
    assert not await manager.stack_exists(stack_name)
except Exception as e:
    # Known issue: LocalStack v2 engine has deletion bugs
    # See: https://github.com/localstack/localstack/issues/13609
    pytest.skip(f"Stack deletion failed (known LocalStack v2 bug): {e}")

CLAUDE.md Reference:

Infrastructure (changes to infra/, aggregator/): Validate CloudFormation template syntax
Test Coverage (changes to src/): Verify corresponding tests exist in tests/

Ref:

zae-limiter/CLAUDE.md

Lines 226 to 229 in a2f4f85

    
           ### Token Bucket Algorithm 
        
           - Buckets can go **negative** for post-hoc reconciliation 
        
           - Refill is calculated lazily on each access

claude · 2026-01-12T21:17:32Z

Code Review

Found 2 issues requiring attention:

Issue 1: CloudFormation Template Bug - Output References Non-Existent Resource

File: src/zae_limiter/infra/cfn_template.yaml (line 497)

Problem: The AggregatorDLQAlarmName output's condition was changed from DeployAggregatorAlarms to DeployAggregator. However, the resource it references (AggregatorDLQAlarm) is conditioned on DeployAggregatorAlarms (line 199).

Impact: When EnableAggregator=true but EnableAlarms=false:

DeployAggregator = true (output will be created)
DeployAggregatorAlarms = false (resource will NOT be created)

The output will attempt to reference a resource that doesn't exist, causing CloudFormation deployment to fail.

Suggested Fix:

    Condition: DeployAggregatorAlarms

Reference:

Resource definition:

zae-limiter/src/zae_limiter/infra/cfn_template.yaml

Lines 197 to 200 in a2f4f85

    
           AggregatorDLQAlarm: 
        
             Type: AWS::CloudWatch::Alarm 
        
             Condition: DeployAggregatorAlarms 
        
             Properties:

Conditions:

zae-limiter/src/zae_limiter/infra/cfn_template.yaml

Lines 107 to 112 in a2f4f85

    
           DeployAggregator: !Equals [!Ref EnableAggregator, 'true'] 
        
           UsePITRRecoveryPeriod: !Not [!Equals [!Ref PITRRecoveryPeriodDays, '']] 
        
           DeployAlarms: !Equals [!Ref EnableAlarms, 'true'] 
        
           DeployAggregatorAlarms: !And 
        
             - !Condition DeployAggregator 
        
             - !Condition DeployAlarms

Issue 2: CLAUDE.md References Non-Existent Pytest Marker

File: pyproject.toml (lines 76-79)

Problem: The PR removes the e2e and aws pytest markers from configuration, but CLAUDE.md still documents using pytest -m e2e -v to run E2E tests.

Impact: The documented command will either fail or return no tests, creating confusion for developers following the documentation.

Suggested Fix Options:

Re-add the e2e marker to pyproject.toml if you plan to add E2E tests in the future, OR
Update CLAUDE.md to use pytest -m integration -v instead of pytest -m e2e -v

Reference: CLAUDE.md testing section at

zae-limiter/CLAUDE.md

Lines 283 to 296 in a2f4f85

    
           # Run E2E tests (full stack lifecycle) 
        
           AWS_ENDPOINT_URL=http://localhost:4566 \ 
        
           AWS_ACCESS_KEY_ID=test \ 
        
           AWS_SECRET_ACCESS_KEY=test \ 
        
           AWS_DEFAULT_REGION=us-east-1 \ 
        
           pytest -m e2e -v 
        
           ``` 
        
           **Important:** The Docker socket mount is required for Lambda execution in LocalStack. 
        
           **Testing Strategy:** 
        
           - Unit tests use moto for speed (no Docker required) 
        
           - Integration tests use LocalStack for full AWS service testing 
        
           - E2E tests validate complete stack lifecycle (deploy → use → delete)

1. Fix CloudFormation condition mismatch: AggregatorDLQAlarmName output now uses DeployAggregatorAlarms condition (not DeployAggregator) to match the resource it references. 2. Add test_cloudformation_stack_deployment_no_alarms: Tests the edge case where EnableAggregator=true but EnableAlarms=false. 3. Add test_stack_create_and_delete_minimal: Integration test for full stack lifecycle with graceful handling of LocalStack v2 deletion bug. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

The e2e marker was removed by the revert. Update CLAUDE.md to reflect that integration tests now include full stack lifecycle tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

sodre mentioned this pull request Jan 12, 2026

🐛 LocalStack: CloudFormation stack deletion fails with template validation error #81

Closed

github-actions Bot reviewed Jan 12, 2026

View reviewed changes

sodre force-pushed the revert/pr-85-legacy-engine branch from 41a577f to a2f4f85 Compare January 12, 2026 21:10

sodre and others added 2 commits January 12, 2026 16:24

📝 docs: remove references to non-existent e2e pytest marker

cfa2cb7

The e2e marker was removed by the revert. Update CLAUDE.md to reflect that integration tests now include full stack lifecycle tests. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

sodre merged commit 1e632cb into main Jan 12, 2026
11 of 12 checks passed

sodre deleted the revert/pr-85-legacy-engine branch January 12, 2026 21:54

sodre added this to the v0.2.0 milestone Jan 14, 2026

sodre changed the title ~~revert: undo LocalStack legacy CloudFormation engine workaround (#85)~~ ⏪ undo LocalStack legacy CloudFormation engine workaround Jan 15, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⏪ undo LocalStack legacy CloudFormation engine workaround#86

⏪ undo LocalStack legacy CloudFormation engine workaround#86
sodre merged 3 commits into
mainfrom
revert/pr-85-legacy-engine

sodre commented Jan 12, 2026

Uh oh!

codecov Bot commented Jan 12, 2026 •

edited

Loading

Uh oh!

github-actions Bot left a comment

Uh oh!

claude Bot commented Jan 12, 2026

Uh oh!

claude Bot commented Jan 12, 2026

Uh oh!

claude Bot commented Jan 12, 2026

Uh oh!

claude Bot commented Jan 12, 2026

Uh oh!

claude Bot commented Jan 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

sodre commented Jan 12, 2026

Summary

The Dilemma

Decision

Re-opens

Test plan

Uh oh!

codecov Bot commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

⚠️ Performance Alert ⚠️

Uh oh!

claude Bot commented Jan 12, 2026

Code Review

Uh oh!

claude Bot commented Jan 12, 2026

Issue 1: CloudFormation Bug - Condition Mismatch

Uh oh!

claude Bot commented Jan 12, 2026

Issue 2: Test Coverage Reduction

Uh oh!

claude Bot commented Jan 12, 2026

Issue 3: Test Deletion Without Replacement

Uh oh!

claude Bot commented Jan 12, 2026

Code Review

Issue 1: CloudFormation Template Bug - Output References Non-Existent Resource

Issue 2: CLAUDE.md References Non-Existent Pytest Marker

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codecov Bot commented Jan 12, 2026 •

edited

Loading