diff --git a/README.md b/README.md index 34c5c76b..794d90e8 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,22 @@ const octokit = new MyOctokit({ }); ``` +You can override the default predicate that determines whether to retry a request based on the outcome of the previous attempt. For example, to retry if the response message includes a particular string. Note that the `doNotRetry` option from the constructor is ignored in this case. + +```typescript +const octokit = new MyOctokit({ + auth: "secret123", + retry: { + shouldRetry: (retryState: RetryState, error: any) => { + if (isRequestError(error)) { + return error.message.includes("Intermittent problem"); + } + return false; + }, + }, +}); +``` + To override the number of retries: ```js diff --git a/src/error-request.ts b/src/error-request.ts index 25af8a65..a1de3e30 100644 --- a/src/error-request.ts +++ b/src/error-request.ts @@ -1,28 +1,45 @@ -import { type RetryPlugin, type RetryState } from "./types.js"; -import type { RequestRequestOptions } from "@octokit/types"; -import type { RequestError } from "@octokit/request-error"; +import { + type RequestOptionsWithRequest, + type RetryPlugin, + type RetryState, +} from "./types.js"; +import { RequestError } from "@octokit/request-error"; export function isRequestError(error: any): error is RequestError { return error.request !== undefined; } -export async function errorRequest( +export function defaultShouldRetry( state: RetryState, - octokit: RetryPlugin, error: RequestError | Error, - options: { request: RequestRequestOptions }, -): Promise { +): boolean { if (!isRequestError(error) || !error?.request.request) { // address https://github.com/octokit/plugin-retry.js/issues/8 throw error; } - // retry all >= 400 && not doNotRetry - if (error.status >= 400 && !state.doNotRetry.includes(error.status)) { - const retries = - options.request.retries != null ? options.request.retries : state.retries; - const retryAfter = Math.pow((options.request.retryCount || 0) + 1, 2); - throw octokit.retry.retryRequest(error, retries, retryAfter); + return error.status >= 400 && !state.doNotRetry.includes(error.status); +} + +export async function errorRequest( + state: RetryState, + octokit: RetryPlugin, + error: RequestError | Error, + options: RequestOptionsWithRequest, +): Promise { + if (state.shouldRetry(state, error)) { + const retries: number = + options.request?.retries != null + ? options.request.retries + : state.retries; + const retryAfter = Math.pow((options.request?.retryCount || 0) + 1, 2); + throw new RequestError( + error.message, + isRequestError(error) ? error.status : 500, + { + request: octokit.retry.retryRequest(options, retries, retryAfter), + }, + ); } // Maybe eventually there will be more cases here diff --git a/src/index.ts b/src/index.ts index cbba4182..3227655c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,40 +1,50 @@ import type { Octokit, OctokitOptions } from "@octokit/core"; -import type { RequestError } from "@octokit/request-error"; import { VERSION } from "./version.js"; -import { errorRequest } from "./error-request.js"; +import { defaultShouldRetry, errorRequest } from "./error-request.js"; import { wrapRequest } from "./wrap-request.js"; -import type { RetryOptions, RetryPlugin, RetryState } from "./types.js"; +import type { + RequestOptionsWithRequest, + RetryOptions, + RetryPlugin, + RetryRequestOptions, + RetryState, +} from "./types.js"; import type { RequestRequestOptions } from "@octokit/types"; export { VERSION } from "./version.js"; +export const defaultRetryState: RetryState = { + enabled: true, + retryAfterBaseValue: 1000, + doNotRetry: [400, 401, 403, 404, 410, 422, 451], + retries: 3, + shouldRetry: defaultShouldRetry, +}; + export function retry( octokit: Octokit, octokitOptions: OctokitOptions, ): RetryPlugin { const state: RetryState = Object.assign( - { - enabled: true, - retryAfterBaseValue: 1000, - doNotRetry: [400, 401, 403, 404, 410, 422, 451], - retries: 3, - } satisfies RetryState, + {}, + defaultRetryState, octokitOptions.retry, ); const retryPlugin: RetryPlugin = { retry: { retryRequest: ( - error: RequestError, + request: RequestOptionsWithRequest, retries: number, retryAfter: number, ) => { - error.request.request = Object.assign({}, error.request.request, { + const newRequest: RequestRequestOptions & + Required = Object.assign({}, request.request, { retries: retries, retryAfter: retryAfter, - } satisfies RequestRequestOptions); + } as Required); - return error; + return { ...request, request: newRequest }; }, }, }; @@ -54,4 +64,11 @@ declare module "@octokit/core/types" { } } +declare module "@octokit/types" { + interface RequestRequestOptions { + retries?: number; + retryAfter?: number; + } +} + export type { RetryPlugin, RetryOptions }; diff --git a/src/types.ts b/src/types.ts index 65598812..0ad06110 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,12 +1,24 @@ import type { RequestError } from "@octokit/request-error"; +import type { RequestOptions, RequestRequestOptions } from "@octokit/types"; + +export interface RetryRequestOptions { + retries?: number; + retryAfter?: number; +} + +export type RequestOptionsWithRequest = RequestOptions & { + request: RequestRequestOptions & RetryRequestOptions; +}; export interface RetryPlugin { retry: { retryRequest: ( - error: RequestError, + request: RequestOptionsWithRequest, retries: number, retryAfter: number, - ) => RequestError; + ) => RequestOptions & { + request: RequestRequestOptions & Required; + }; }; } @@ -15,6 +27,7 @@ export interface RetryOptions { retryAfterBaseValue?: number; doNotRetry?: number[]; retries?: number; + shouldRetry?: (state: RetryState, error: RequestError | Error) => boolean; } export type RetryState = Required; diff --git a/src/wrap-request.ts b/src/wrap-request.ts index e4127773..65ebd755 100644 --- a/src/wrap-request.ts +++ b/src/wrap-request.ts @@ -17,8 +17,8 @@ export async function wrapRequest( const limiter = new Bottleneck(); limiter.on("failed", function (error: RequestError, info: RetryableInfo) { - const maxRetries = ~~error.request.request?.retries; - const after = ~~error.request.request?.retryAfter; + const maxRetries = error.request.request?.retries || 0; + const after = error.request.request?.retryAfter || 0; options.request.retryCount = info.retryCount + 1; if (maxRetries > info.retryCount) { @@ -26,6 +26,9 @@ export async function wrapRequest( // the request after that number of milliseconds have passed return after * state.retryAfterBaseValue; } + + // Do not retry. + return undefined; }); return limiter.schedule( diff --git a/test/error-request.test.ts b/test/error-request.test.ts new file mode 100644 index 00000000..43daa2c7 --- /dev/null +++ b/test/error-request.test.ts @@ -0,0 +1,92 @@ +import { describe, it, expect } from "vitest"; +import { defaultShouldRetry, errorRequest } from "../src/error-request.ts"; +import { defaultRetryState } from "../src/index.ts"; +import { RequestError } from "@octokit/request-error"; +import { TestOctokit } from "./octokit.ts"; +import { RetryState } from "../src/types.ts"; +import type { RequestMethod, RequestOptions } from "@octokit/types"; + +describe("defaultShouldRetry", function () { + it("should re-throw non-RequestError errors", function () { + try { + defaultShouldRetry(defaultRetryState, new Error("Re-throw me")); + throw new Error("Should not reach this point"); + } catch (err: any) { + expect(err.message).toEqual("Re-throw me"); + } + }); + + it("should re-throw errors without RequestRequestOptions", function () { + try { + defaultShouldRetry( + defaultRetryState, + new RequestError("Re-throw me", 500, { + request: { method: "GET", url: "/something", headers: {} }, + }), + ); + throw new Error("Should not reach this point"); + } catch (err: any) { + expect(err.message).toEqual("Re-throw me"); + } + }); + + it("returns false for doNotRetry status codes", function () { + for (const statusCode of defaultRetryState.doNotRetry) { + const result = defaultShouldRetry( + defaultRetryState, + new RequestError("Re-throw me", statusCode, { + request: { + method: "GET", + url: "/something", + headers: {}, + request: {}, + }, + }), + ); + expect(result).toBe(false); + } + }); + + it("returns true for 500 errors", function () { + const result = defaultShouldRetry( + defaultRetryState, + new RequestError("Re-throw me", 500, { + request: { + method: "GET", + url: "/something", + headers: {}, + request: {}, + }, + }), + ); + expect(result).toBe(true); + }); +}); + +describe("errorRequest", function () { + it("allows non-RequestErrors to be retried", async function () { + const state: RetryState = { + ...defaultRetryState, + shouldRetry: (_state, _error) => true, + }; + const requestOptions = { + method: "GET" as RequestMethod, + url: "/issues", + headers: {}, + request: {}, + } satisfies RequestOptions; + + try { + await errorRequest( + state, + new TestOctokit(), + new Error("Some non-RequestError"), + requestOptions, + ); + throw new Error("Should not reach this point"); + } catch (error: any) { + expect(error.message).toBe("Some non-RequestError"); + expect(error.request.request.retries).toBe(3); + } + }); +}); diff --git a/test/retry.test.ts b/test/retry.test.ts index acc0c369..f82d9357 100644 --- a/test/retry.test.ts +++ b/test/retry.test.ts @@ -1,6 +1,10 @@ import { describe, it, expect } from "vitest"; import { TestOctokit } from "./octokit.ts"; -import { errorRequest, isRequestError } from "../src/error-request.ts"; +import { + defaultShouldRetry, + errorRequest, + isRequestError, +} from "../src/error-request.ts"; import { RequestError } from "@octokit/request-error"; import type { RequestMethod, @@ -58,6 +62,43 @@ describe("Automatic Retries", function () { ).toBeLessThan(20); }); + it("Should retry and pass with custom shouldRetry function", async function () { + const octokit = new TestOctokit({ + shouldRetry: (_retryState: RetryState, error: any) => { + if (isRequestError(error)) { + return error.message.includes("Intermittent problem"); + } + return false; + }, + }); + + const response = await octokit.request("GET /route", { + request: { + responses: [ + { + status: 403, + headers: {}, + data: { message: "Intermittent problem" }, + }, + { status: 200, headers: {}, data: { message: "Success!" } }, + ], + retries: 1, + }, + }); + + expect(response.status).toEqual(200); + expect(response.data).toStrictEqual({ message: "Success!" }); + expect(octokit.__requestLog).toStrictEqual([ + "START GET /route", + "START GET /route", + "END GET /route", + ]); + + expect( + octokit.__requestTimings[1] - octokit.__requestTimings[0], + ).toBeLessThan(20); + }); + it("Should retry twice and fail", async function () { const octokit = new TestOctokit(); @@ -414,6 +455,7 @@ describe("errorRequest", function () { retryAfterBaseValue: 1000, doNotRetry: [400, 401, 403, 404, 422], retries: 3, + shouldRetry: defaultShouldRetry, } satisfies RetryState; const requestOptions = { method: "GET" as RequestMethod, @@ -448,6 +490,7 @@ describe("errorRequest", function () { retryAfterBaseValue: 1000, doNotRetry: [400, 401, 403, 404, 422], retries: 3, + shouldRetry: defaultShouldRetry, } satisfies RetryState; const requestOptions = { method: "GET" as RequestMethod,