feat: auto retry on deployment-related 499 errors#114
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces retry logic to the ApiClientImpl to handle transient HTTP 499 (client closed connection) errors by automatically retrying failed requests up to 3 times with exponential backoff.
- Adds
sendRetriablemethod that wraps the existing request logic with retry capability - Refactors existing
sendlogic intosendInternalto support retries - Introduces comprehensive test coverage with
ApiClientRetriableTestsfor various 499 response scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
Bucketeer/Sources/Internal/Remote/ApiClientImpl.swift |
Implements retry logic with exponential backoff for HTTP 499 errors, refactors request handling into sendInternal and sendRetriable methods |
BucketeerTests/Mock/MockSession.swift |
Adds dynamic response capability with responseProvider and MockSessionCounter to support testing sequential response scenarios |
BucketeerTests/ApiClientRetriableTests.swift |
Comprehensive test suite covering 499 retry behavior, non-retriable status codes, and sequential response scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
duyhungtnn
changed the title
|
@cre8ivejp please help me to take a look |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Updated all instances of mockDispatchQueue.sync to mockDispatchQueue.async in ApiClientTests.swift to ensure asynchronous execution in test cases. This change aligns the test behavior with expected asynchronous patterns and may help prevent potential deadlocks or blocking issues during test execution.
Wrap RetrierTests test cases in dispatchQueue.async to ensure retrier logic is executed on the correct queue. Also fix minor typos in ApiClientImpl test-only method comments for clarity.
Improved documentation in ApiClientImpl for clarity on requestId and blocking behavior. Updated RetrierTests to use [weak self] instead of [unowned self] in async closures to prevent potential crashes due to deallocated self.
Renamed the parameter and related usages from 'defaultRequestTimeoutMills' to 'defaultRequestTimeoutMillis' in ApiClientImpl and associated test files for consistency and correctness.
duyhungtnn
force-pushed
the
feat/retry-on-499-status-code
branch
from
731729e to
880d1c6
Compare
Updated ApiClientRequestIdTests and ApiClientRetriableTests to include the sdkInfo parameter with sourceId set to .ios and sdkVersion set to Version.current. This ensures tests reflect the latest ApiClient initialization requirements.
Enhances memory management in ApiClientImpl by using strong references in async tasks and adding DEBUG guards for test-only methods. Updates logging for request IDs, clarifies exponential backoff comments, and ensures BKTClient.destroy executes reliably. Adds a unit test to verify that retries stop when Retrier is deallocated.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Moved the exponential backoff delay logic into a dedicated method `nextExponentialBackoffDelay` for clarity and reusability. Also updated the closure to use a strong reference to self after checking for deallocation, improving code safety and readability.
Corrected 'intenal' to 'internal' in a comment to improve code clarity.
Added unit tests to verify the nextExponentialBackoffDelay calculation and to ensure the condition check is called the correct number of times during retries.
Refactored attemptRecursive to safely handle cases where self may be deallocated by using optional chaining and weak self in closures. This prevents potential retain cycles and ensures no further retries are attempted if Retrier is deallocated.
Simplified the weak self handling in the retrier task closure by directly checking for self deallocation and using optional chaining. This improves code clarity and safety when ApiClientImpl is deallocated before the request completes.
Eliminated duplicate or unnecessary debug log statements in fetchEvaluation and registerEvents methods. Updated one log message in registerEvents to include the API path for improved clarity.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Updated the destroy method to capture a strong reference to the component before executing the destroy logic, ensuring proper destruction even when the client deallocates.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Related bucketeer-io/javascript-client-sdk#269
This pull request introduces a robust retry mechanism for API requests in the Bucketeer SDK, focusing on improved reliability and error handling for transient failures (specifically HTTP 499 errors). The changes include the implementation of a new
Retrierclass with exponential backoff, integration of request tracking to avoid race conditions, and several refactorings for clarity and maintainability.Key changes:
Retry Logic & Reliability Improvements
Retrierclass (Retrier.swift) that handles asynchronous retries with exponential backoff, to be used on a specifiedDispatchQueue. This ensures that transient errors (like HTTP 499) are retried up to 3 times with increasing delays.RetrierintoApiClientImpl, updating API request methods to use the new retry mechanism. Retries are now only triggered for HTTP 499 errors, and all retry scheduling occurs on the SDK's queue. [1] [2] [3] [4] [5] [6] [7] [8]Request Tracking & Cancellation
getEvaluationsRequestId,registerEventsRequestId) to ensure only the latest request is processed, preventing race conditions and accidental handling of outdated responses. [1] [2] [3] [4]Refactoring & Code Quality
ApiPathsenum to centralize and clarify API endpoint references. [1] [2] [3] [4]