Skip to content

fix(core): retry after clock skew correction #1170

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 7 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/giant-pianos-lie.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@smithy/service-error-classification": patch
"@smithy/middleware-retry": patch
"@smithy/types": patch
---

make clock skew correcting errors transient
8 changes: 7 additions & 1 deletion packages/middleware-retry/src/configurations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,20 @@ export interface RetryInputConfig {
retryStrategy?: RetryStrategy | RetryStrategyV2;
}

interface PreviouslyResolved {
/**
* @internal
*/
export interface PreviouslyResolved {
/**
* Specifies provider for retry algorithm to use.
* @internal
*/
retryMode: string | Provider<string>;
}

/**
* @internal
*/
export interface RetryResolvedConfig {
/**
* Resolved value for input config {@link RetryInputConfig.maxAttempts}
Expand Down
3 changes: 3 additions & 0 deletions packages/middleware-retry/src/retryDecider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import {
} from "@smithy/service-error-classification";
import { SdkError } from "@smithy/types";

/**
* @deprecated this is only used in the deprecated StandardRetryStrategy. Do not use in new code.
*/
export const defaultRetryDecider = (error: SdkError) => {
if (!error) {
return false;
Expand Down
7 changes: 7 additions & 0 deletions packages/middleware-retry/src/retryMiddleware.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,7 @@ describe(retryMiddleware.name, () => {
(isThrottlingError as jest.Mock).mockReturnValue(true);
const next = jest.fn().mockRejectedValue(requestError);
const errorInfo = {
error: requestError,
errorType: "THROTTLING",
};
const mockRetryStrategy = {
Expand Down Expand Up @@ -172,6 +173,7 @@ describe(retryMiddleware.name, () => {
(isThrottlingError as jest.Mock).mockReturnValue(true);
const next = jest.fn().mockRejectedValueOnce(mockError).mockResolvedValueOnce(mockSuccess);
const errorInfo = {
error: mockError,
errorType: "THROTTLING",
};
const { response, output } = await retryMiddleware({
Expand All @@ -193,6 +195,7 @@ describe(retryMiddleware.name, () => {
(isThrottlingError as jest.Mock).mockReturnValue(false);
const next = jest.fn().mockRejectedValueOnce(mockError).mockResolvedValueOnce(mockSuccess);
const errorInfo = {
error: mockError,
errorType: "TRANSIENT",
};
const { response, output } = await retryMiddleware({
Expand All @@ -215,6 +218,7 @@ describe(retryMiddleware.name, () => {
(isThrottlingError as jest.Mock).mockReturnValue(false);
const next = jest.fn().mockRejectedValueOnce(mockError).mockResolvedValueOnce(mockSuccess);
const errorInfo = {
error: mockError,
errorType: "SERVER_ERROR",
};
const { response, output } = await retryMiddleware({
Expand All @@ -237,6 +241,7 @@ describe(retryMiddleware.name, () => {
(isThrottlingError as jest.Mock).mockReturnValue(false);
const next = jest.fn().mockRejectedValueOnce(mockError).mockResolvedValueOnce(mockSuccess);
const errorInfo = {
error: mockError,
errorType: "CLIENT_ERROR",
};
const { response, output } = await retryMiddleware({
Expand Down Expand Up @@ -265,6 +270,7 @@ describe(retryMiddleware.name, () => {
});
const next = jest.fn().mockRejectedValueOnce(mockError).mockResolvedValueOnce(mockSuccess);
const errorInfo = {
error: mockError,
errorType: "CLIENT_ERROR",
};
const { response, output } = await retryMiddleware({
Expand All @@ -289,6 +295,7 @@ describe(retryMiddleware.name, () => {
const { isInstance } = HttpResponse;
((isInstance as unknown) as jest.Mock).mockReturnValue(true);
const errorInfo = {
error: mockError,
errorType: "CLIENT_ERROR",
retryAfterHint: retryAfterDate,
};
Expand Down
3 changes: 2 additions & 1 deletion packages/middleware-retry/src/retryMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export const retryMiddleware = (options: RetryResolvedConfig) => <Output extends
output.$metadata.attempts = attempts + 1;
output.$metadata.totalRetryDelay = totalRetryDelay;
return { response, output };
} catch (e) {
} catch (e: any) {
const retryErrorInfo = getRetryErrorInfo(e);
lastError = asSdkError(e);

Expand Down Expand Up @@ -97,6 +97,7 @@ const isRetryStrategyV2 = (retryStrategy: RetryStrategy | RetryStrategyV2) =>

const getRetryErrorInfo = (error: SdkError): RetryErrorInfo => {
const errorInfo: RetryErrorInfo = {
error,
errorType: getRetryErrorType(error),
};
const retryAfterHint = getRetryAfterHint(error.$response);
Expand Down
9 changes: 9 additions & 0 deletions packages/service-error-classification/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,16 @@ import {

export const isRetryableByTrait = (error: SdkError) => error.$retryable !== undefined;

/**
* @deprecated use isClockSkewCorrectedError. This is only used in deprecated code.
*/
export const isClockSkewError = (error: SdkError) => CLOCK_SKEW_ERROR_CODES.includes(error.name);

/**
* @returns whether the error resulted in a systemClockOffset aka clock skew correction.
*/
export const isClockSkewCorrectedError = (error: SdkError) => error.$metadata?.clockSkewCorrected;

export const isThrottlingError = (error: SdkError) =>
error.$metadata?.httpStatusCode === 429 ||
THROTTLING_ERROR_CODES.includes(error.name) ||
Expand All @@ -24,6 +32,7 @@ export const isThrottlingError = (error: SdkError) =>
* the name "TimeoutError" to be checked by the TRANSIENT_ERROR_CODES condition.
*/
export const isTransientError = (error: SdkError) =>
isClockSkewCorrectedError(error) ||
TRANSIENT_ERROR_CODES.includes(error.name) ||
NODEJS_TIMEOUT_ERROR_CODES.includes((error as { code?: string })?.code || "") ||
TRANSIENT_ERROR_STATUS_CODES.includes(error.$metadata?.httpStatusCode || 0);
Expand Down
9 changes: 8 additions & 1 deletion packages/types/src/retry.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { SdkError } from "./shapes";

/**
* @public
*/
Expand Down Expand Up @@ -33,14 +35,19 @@ export type RetryErrorType =
* @public
*/
export interface RetryErrorInfo {
/**
* The error thrown during the initial request, if available.
*/
error?: SdkError;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users have requested that the thrown exception is available in the retry strategy implementation. Otherwise, they cannot even check (for example) the http status code of an error when deciding whether to retry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, they are forced to write a verbose retry wrapper instead of using a RetryStrategy.


errorType: RetryErrorType;

/**
* Protocol hint. This could come from Http's 'retry-after' header or
* something from MQTT or any other protocol that has the ability to convey
* retry info from a peer.
*
* @returns the Date after which a retry should be attempted.
* The Date after which a retry should be attempted.
*/
retryAfterHint?: Date;
}
Expand Down
13 changes: 12 additions & 1 deletion packages/types/src/shapes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,15 @@ export interface SmithyException {
* the base exception for the service should be used. Each client exports
* a base ServiceException prefixed with the service name.
*/
export type SdkError = Error & Partial<SmithyException> & Partial<MetadataBearer>;
export type SdkError = Error &
Partial<SmithyException> &
Partial<MetadataBearer> & {
$metadata?: Partial<MetadataBearer>["$metadata"] & {
/**
* If present, will have value of true and indicates that the error resulted in a
* correction of the clock skew, a.k.a. config.systemClockOffset.
* This is specific to AWS SDK and sigv4.
*/
readonly clockSkewCorrected?: true;
};
};