feat(push): Add AWS throttling retry and duplicate path validation #109
Conversation
…ndHandler Implement exponential backoff with jitter for AWS SSM throttling errors. Enhance error logging to provide more context on failures. Add tests to verify retry behavior and error handling.
- Refactored the logic to validate and group environment variables by their SSM paths, ensuring consistent values for duplicate paths. - Updated the push logic to handle grouped parameters, improving clarity and maintainability of the code. - Enhanced error handling for conflicting values.
macalbert
temporarily deployed
to
github-pages
GitHub Actions
Inactive
Summary of ChangesHello @macalbert, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the reliability and robustness of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
WalkthroughDocumentation YAML string literals standardized; PushEnvToSsm handler refactored to validate and group environment variables by SSM path, perform batch parallel pushes with exponential-backoff retry and normalized error handling; tests expanded for validation, retries, parallel processing, and diverse error cases. Changes
Sequence DiagramsequenceDiagram
actor Handler as PushEnvToSsmCommandHandler
participant Config as Configuration
participant Validation as Validation & Grouping
participant Retry as Retry Logic
participant SSM as AWS SSM
Handler->>Config: loadConfiguration()
Config-->>Handler: config
Handler->>Validation: validateAndGroupByPath(envVariables, paramMap)
Validation->>Validation: build Map<ssmPath, {value, sourceKeys}>
alt Conflict detected
Validation-->>Handler: throw Error
else Valid map
Validation-->>Handler: pathToValueMap
end
loop For each unique SSM path (parallel)
Handler->>Retry: pushParameter(path, value, sourceKeys)
Retry->>SSM: PutParameter
alt Throttling-like error
Retry->>Retry: exponential backoff + jitter
Retry->>SSM: retry PutParameter
else Success
SSM-->>Retry: OK
end
Retry-->>Handler: result / error
end
Handler->>Handler: log summary (total vars, unique paths, failures)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 enhances the robustness of Envilder's push operation by adding AWS throttling retry logic with exponential backoff and duplicate path validation to prevent data corruption.
Changes:
- Implements retry logic with exponential backoff + jitter for AWS SSM throttling errors (
TooManyUpdates,ThrottlingException) - Adds validation to detect conflicting values when multiple environment variables map to the same SSM path
- Groups parameters by SSM path to push each unique path only once, reducing API calls
- Enhances error logging to handle all error types comprehensively
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/envilder/application/pushEnvToSsm/PushEnvToSsmCommandHandler.ts |
Core implementation of retry logic, duplicate path validation, parameter grouping optimization, and enhanced error handling |
tests/envilder/application/pushEnvToSsm/PushEnvToSsmCommandHandler.test.ts |
Comprehensive test coverage for retry behavior, duplicate path validation, and error handling scenarios |
github-action/dist/index.js |
Bundled GitHub Action with new implementation |
e2e/cli.test.ts |
E2E test for successful push operation |
.github/workflows/*.yml |
Style formatting improvements (quote standardization, trailing space removal) |
docs/github-action.md, github-action/README.md |
Documentation style formatting improvements |
macalbert
had a problem deploying
to
github-pages
GitHub Actions
Failure
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the robustness of the push operation by introducing exponential backoff for AWS throttling and validation for duplicate SSM paths. However, a security audit identified critical issues related to sensitive data handling in error paths. Specifically, secret values are explicitly included in error messages when conflicts are detected, and the general error logging mechanism stringifies entire error objects, which may contain secrets, risking accidental leakage into application logs. Additionally, consider restoring a helpful warning for missing environment variables to improve user experience.
src/envilder/application/pushEnvToSsm/PushEnvToSsmCommandHandler.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/envilder/application/pushEnvToSsm/PushEnvToSsmCommandHandler.ts`:
- Around line 94-99: The error thrown in PushEnvToSsmCommandHandler exposes raw
secret values (existing.value and envValue); change the error message to use
masked values via EnvironmentVariable.maskedValue(...) for both existing.value
and envValue (keep ssmPath and source keys as-is), e.g., call
EnvironmentVariable.maskedValue(existing.value) and
EnvironmentVariable.maskedValue(envValue) when composing the message to avoid
leaking secrets; ensure no other logs or thrown messages in this handler include
the raw values.
🧹 Nitpick comments (3)
src/envilder/application/pushEnvToSsm/PushEnvToSsmCommandHandler.ts (2)
116-134: ConsiderPromise.allSettledfor comprehensive error reporting.Using
Promise.allcauses the handler to fail fast on the first error, potentially hiding other failures. If comprehensive error reporting is desired (e.g., showing all failed parameters), considerPromise.allSettled.However, the current fail-fast behavior is consistent with the existing error handling pattern and may be intentional.
152-193: Consider logging retry attempts for observability.The retry logic is correct, but retries happen silently. Adding a debug/info log when retrying would improve observability and help diagnose throttling issues in production.
📊 Proposed fix to add retry logging
// Calculate delay with exponential backoff + jitter const exponentialDelay = baseDelayMs * 2 ** attempt; const jitter = Math.random() * exponentialDelay * 0.5; // 0-50% jitter const delayMs = exponentialDelay + jitter; + this.logger.info( + `Throttled by AWS SSM, retrying in ${Math.round(delayMs)}ms (attempt ${attempt + 1}/${maxRetries})`, + ); + await new Promise((resolve) => setTimeout(resolve, delayMs));tests/envilder/application/pushEnvToSsm/PushEnvToSsmCommandHandler.test.ts (1)
82-106: Test name could be more descriptive.The test name
Should_Warning_When_ImportsNotMatchesKeyis misleading since no warning is logged. The test verifies that missing variables are skipped silently. Consider renaming for clarity.✏️ Suggested rename
- it('Should_Warning_When_ImportsNotMatchesKey', async () => { + it('Should_SkipMissingVariable_When_EnvVariableNotInEnvFile', async () => {
macalbert
temporarily deployed
to
github-pages
GitHub Actions
Inactive
macalbert
temporarily deployed
to
github-pages
GitHub Actions
Inactive
macalbert
had a problem deploying
to
github-pages
GitHub Actions
Failure
macalbert
temporarily deployed
to
github-pages
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/envilder/application/pushEnvToSsm/PushEnvToSsmCommandHandler.test.ts (1)
82-106: Test name has grammatical issue and is missing warning assertion.The test name
Should_Warning_When_ImportsNotMatchesKeyshould follow proper grammar (e.g.,Should_LogWarning_When_EnvironmentVariableNotFound). Additionally, the test verifies only thatsetSecretwas called once but doesn't assert thatlogger.warnwas called for the missing variable, even though the test name implies warning behavior is being tested.💡 Proposed fix
- it('Should_Warning_When_ImportsNotMatchesKey', async () => { + it('Should_LogWarningAndSkipVariable_When_EnvironmentVariableNotFound', async () => { // Arrange mockVariableStore.getMapping.mockResolvedValue({ TEST_ENV_VAR: '/path/to/ssm/test', MISSING_VAR: '/path/to/ssm/missing', }); mockVariableStore.getEnvironment.mockResolvedValue({ TEST_ENV_VAR: 'test-value', // MISSING_VAR is not present }); const command = PushEnvToSsmCommand.create(mockMapPath, mockEnvFilePath); // Act await sut.handle(command); // Assert // Only TEST_ENV_VAR should be pushed (MISSING_VAR is skipped) expect(mockSecretProvider.setSecret).toHaveBeenCalledTimes(1); expect(mockSecretProvider.setSecret).toHaveBeenCalledWith( '/path/to/ssm/test', 'test-value', ); + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('MISSING_VAR'), + ); });
🧹 Nitpick comments (3)
tests/envilder/application/pushEnvToSsm/PushEnvToSsmCommandHandler.test.ts (1)
176-181: Redundant assertion can be simplified.The second assertion on lines 179-181 is redundant since if
mockLogger.errorwas never called (line 178), it couldn't have been called with any specific argument. Consider removing the redundant check.♻️ Proposed simplification
// Assert // Should NOT show any "Failed..." error message expect(mockLogger.error).not.toHaveBeenCalled(); - expect(mockLogger.error).not.toHaveBeenCalledWith( - expect.stringContaining('Failed to push environment file'), - ); // Should show success messagesrc/envilder/application/pushEnvToSsm/PushEnvToSsmCommandHandler.ts (2)
94-123: Past review addressed: secrets are now properly masked.The conflicting values error message now uses
EnvironmentVariable.maskedValueinstead of raw values.However,
totalVariableson line 117 counts all mappings including those where the environment variable was not found, making the log message potentially misleading when variables are skipped.,
♻️ Optional: More accurate logging
+ let validatedCount = 0; for (const [envKey, ssmPath] of Object.entries(paramMap)) { const envValue = envVariables[envKey]; if (envValue === undefined) { this.logger.warn( `Warning: Environment variable ${envKey} not found in environment file`, ); continue; } + validatedCount++; // ... rest of the loop } const uniquePaths = pathToValueMap.size; - const totalVariables = Object.keys(paramMap).length; this.logger.info( - `Validated ${totalVariables} environment variables mapping to ${uniquePaths} unique SSM parameters`, + `Validated ${validatedCount} environment variables mapping to ${uniquePaths} unique SSM parameters`, );
125-143: Parallel processing with Promise.all may mask individual failures.Using
Promise.allmeans if multiple parameters fail, only the first rejection is surfaced. Consider whetherPromise.allSettledfollowed by error aggregation would provide better visibility into all failures.For now this is acceptable since the test confirms all parameters are attempted, but worth considering for future improvement if detailed error reporting per parameter becomes needed.
Pull Request
What does this PR do?
This PR significantly improves the robustness and reliability of the push operation in Envilder by addressing multiple error handling and operational issues:
TooManyUpdates,ThrottlingException) automaticallyRelated issues
Fixes issues with push command failures when encountering AWS throttling or duplicate path configurations.
Type of change
Checklist
Notes for reviewer
Key Changes by Commit:
9855fe1: Style improvements in workflow files (quotes standardization)61d4fc2: Initial error handling improvements and better error message formattingc5384d6: Core feature - retry logic with exponential backoff for AWS throttlingd55d416: Validation logic to detect conflicting values for duplicate SSM paths3ee3bbf: Code cleanup - removed unused parameterTesting Coverage:
The PR includes comprehensive test coverage for:
Architecture Notes:
This follows the established Command/Handler pattern in the application layer. The changes are isolated to
PushEnvToSsmCommandHandlerand its tests, maintaining the hexagonal architecture boundaries (no changes to domain or infrastructure layers).Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.