-
Notifications
You must be signed in to change notification settings - Fork 2
feat: auto retry on deployment-related 499 errors #265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
aadcd27
feat: add conditional failure tracking to evaluation fetch retries
duyhungtnn 412ff89
Fix suite name typo in EvaluationTask.spec.ts
duyhungtnn 8c25903
Refactor options handling in fetchEvaluationsInternal
duyhungtnn 05562df
Refactor formatting in BKTClientImpl methods
duyhungtnn 637f365
Ignore storage layer errors in evaluation logging
duyhungtnn 0690d13
Refactor fetchEvaluations options handling
duyhungtnn cf642f9
Remove redundant nullish coalescing in options destructure
duyhungtnn cc7edb2
Refactor test to use fetchEvaluationsInternal parameter types
duyhungtnn 846f491
feat: add generic retry utility with tests
duyhungtnn 61bf9bc
Rename FutureRetriable to PromiseRetriable and update usage
duyhungtnn e04739c
Add backoff strategy to promiseRetriable retries
duyhungtnn 87915fb
Add tests for ApiClientImpl and postInternal retry logic
duyhungtnn bfc8636
Refactor evaluation retry logic and failure tracking
duyhungtnn 69c03c9
Refine retry logic in post and EvaluationTask
duyhungtnn 5915cb8
Skip retries when polling interval is short
duyhungtnn 7b18504
Improve timer handling in postInternal test
duyhungtnn 71dcd62
fix(scheduler): prevent EvaluationTask from stopping after fetch
duyhungtnn a915fa9
Add test for repeated timer execution in EvaluationTask
duyhungtnn 1c3b790
chore: remove redundant code
duyhungtnn e82cf2c
revert: restore code for EvaluationTask
duyhungtnn e24753a
Add debug logging for storage layer errors
duyhungtnn a53ba5b
Change storage error logging from debug to error
duyhungtnn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| export interface RetryPolicy { | ||
| /** Maximum number of retry attempts */ | ||
| maxRetries: number | ||
| /** Base delay before the first retry attempt in milliseconds */ | ||
| delay: number | ||
| /** Strategy for calculating retry delays. Defaults to linear. */ | ||
| backoffStrategy?: BackoffStrategy | ||
| } | ||
|
|
||
| export type BackoffStrategy = 'constant' | 'linear' | ||
|
|
||
| /** | ||
| * Function to determine if an error should trigger a retry | ||
| */ | ||
| export type ShouldRetryFn = (error: Error) => boolean | ||
|
|
||
| /** | ||
| * A generic retry utility that executes a function with configurable retry logic | ||
| * @param fn The function to execute | ||
| * @param retryPolicy The retry configuration | ||
| * @param shouldRetry Function to determine if error should trigger retry | ||
| * @returns Promise resolving to the result or throwing the last error | ||
| */ | ||
| export async function promiseRetriable<T>( | ||
| fn: () => Promise<T>, | ||
| retryPolicy: RetryPolicy, | ||
| shouldRetry: ShouldRetryFn | ||
| // Future improvement: Allow cancellation in the future by adding an AbortSignal parameter | ||
| ): Promise<T> { | ||
| const { maxRetries } = retryPolicy | ||
| let attempts = 0 | ||
|
|
||
| while (attempts <= maxRetries) { | ||
| attempts++ | ||
|
|
||
| try { | ||
| return await fn() | ||
| } catch (error) { | ||
| const lastError = error instanceof Error ? error : new Error(String(error)) | ||
|
|
||
| // If this was the last attempt or we shouldn't retry this error, throw | ||
| if (attempts > maxRetries || !shouldRetry(lastError)) { | ||
| throw lastError | ||
| } | ||
|
|
||
| // Wait before next attempt | ||
| const waitTime = getBackoffDelay(attempts, retryPolicy) | ||
| if (waitTime > 0) { | ||
| await sleep(waitTime) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // This should never be reached due to the logic above | ||
| throw new Error('Unexpected end of retry loop') | ||
| } | ||
|
|
||
| /** | ||
| * Sleep utility function | ||
| */ | ||
| function sleep(ms: number): Promise<void> { | ||
| return new Promise(resolve => setTimeout(resolve, ms)) | ||
| } | ||
|
|
||
| function getBackoffDelay(attempt: number, retryPolicy: RetryPolicy): number { | ||
| const baseDelay = Math.max(0, retryPolicy.delay) | ||
| if (baseDelay === 0) { | ||
| return 0 | ||
| } | ||
|
|
||
| const strategy = retryPolicy.backoffStrategy ?? 'linear' | ||
|
|
||
| switch (strategy) { | ||
| case 'constant': | ||
| return baseDelay | ||
| case 'linear': | ||
| default: | ||
| return baseDelay * Math.max(1, attempt) | ||
| } | ||
| } | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| import { describe, it, expect, vi } from 'vitest' | ||
|
|
||
| import { ApiClientImpl } from '../../../src/internal/remote/ApiClient' | ||
| import { FetchResponseLike } from '../../../src/internal/remote/fetch' | ||
| import { SourceId } from '../../../src/internal/model/SourceId' | ||
| import { SDK_VERSION } from '../../../src/internal/version' | ||
| import { user1 } from '../../mocks/users' | ||
| import * as postModule from '../../../src/internal/remote/post' | ||
|
|
||
| describe('ApiClientImpl', () => { | ||
| it('should call postInternal', async () => { | ||
| vi.restoreAllMocks() | ||
|
|
||
| const mockResponse: FetchResponseLike = { | ||
| ok: true, | ||
| status: 200, | ||
| statusText: 'OK', | ||
| headers: { | ||
| get: (name: string) => name === 'Content-Length' ? '10' : null, | ||
| }, | ||
| json: () => Promise.resolve({ | ||
| evaluations: [], | ||
| userEvaluationsId: 'test_id', | ||
| }), | ||
| text: () => Promise.resolve(''), | ||
| } | ||
|
|
||
| const spy = vi.spyOn(postModule, 'postInternal').mockResolvedValue(mockResponse) | ||
|
|
||
| const apiClient = new ApiClientImpl( | ||
| 'https://api.example.com', | ||
| 'test_api_key', | ||
| fetch, | ||
| SourceId.JAVASCRIPT, | ||
| SDK_VERSION, | ||
| ) | ||
|
|
||
| await apiClient.getEvaluations({ | ||
| user: user1, | ||
| userEvaluationsId: 'test_id', | ||
| tag: 'test_tag', | ||
| userEvaluationCondition: { | ||
| evaluatedAt: '0', | ||
| userAttributesUpdated: false, | ||
| }, | ||
| }) | ||
|
|
||
| expect(spy).toHaveBeenCalledTimes(1) | ||
| expect(spy).toHaveBeenCalledWith( | ||
| 'https://api.example.com/get_evaluations', | ||
| { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: 'test_api_key', | ||
| }, | ||
| { | ||
| user: user1, | ||
| userEvaluationsId: 'test_id', | ||
| tag: 'test_tag', | ||
| sourceId: SourceId.JAVASCRIPT, | ||
| sdkVersion: SDK_VERSION, | ||
| userEvaluationCondition: { | ||
| evaluatedAt: '0', | ||
| userAttributesUpdated: false, | ||
| }, | ||
| }, | ||
| fetch, | ||
| 30000, // default timeout | ||
| ) | ||
| }) | ||
| }) |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.