fix(retry-count): don't leak retryCount between requests: updated (#539)

Co-authored-by: Jarek Radosz <jradosz@gmail.com>

Author: mutantcornholio

README.md

@@ -52,12 +52,12 @@ const MyOctokit = Octokit.plugin(throttling);
 const octokit = new MyOctokit({
   auth: `secret123`,
   throttle: {
-    onRateLimit: (retryAfter, options, octokit) => {
+    onRateLimit: (retryAfter, options, octokit, retryCount) => {
       octokit.log.warn(
         `Request quota exhausted for request ${options.method} ${options.url}`
       );
 
-      if (options.request.retryCount === 0) {
+      if (retryCount < 1) {
         // only retries once
         octokit.log.info(`Retrying after ${retryAfter} seconds!`);
         return true;

src/index.ts

@@ -104,6 +104,7 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
           octokit.log.warn(
             "[@octokit/plugin-throttling] `onAbuseLimit()` is deprecated and will be removed in a future release of `@octokit/plugin-throttling`, please use the `onSecondaryRateLimit` handler instead"
           );
+          // @ts-expect-error
           return state.onAbuseLimit(...args);
         }
       : state.onSecondaryRateLimit
@@ -117,7 +118,7 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
 
   // @ts-expect-error
   state.retryLimiter.on("failed", async function (error, info) {
-    const options = info.args[info.args.length - 1];
+    const [state, request, options] = info.args;
     const { pathname } = new URL(options.url, "http://github.test");
     const shouldRetryGraphQL =
       pathname.startsWith("/graphql") && error.status !== 401;
@@ -126,7 +127,10 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
       return;
     }
 
-    const retryCount = ~~options.request.retryCount;
+    const retryCount = ~~request.retryCount;
+    request.retryCount = retryCount;
+
+    // backward compatibility
     options.request.retryCount = retryCount;
 
     const { wantRetry, retryAfter = 0 } = await (async function () {
@@ -144,7 +148,8 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
           "secondary-limit",
           retryAfter,
           options,
-          octokit
+          octokit,
+          retryCount
         );
         return { wantRetry, retryAfter };
       }
@@ -167,15 +172,16 @@ export function throttling(octokit: Octokit, octokitOptions: OctokitOptions) {
           "rate-limit",
           retryAfter,
           options,
-          octokit
+          octokit,
+          retryCount
         );
         return { wantRetry, retryAfter };
       }
       return {};
     })();
 
     if (wantRetry) {
-      options.request.retryCount++;
+      request.retryCount++;
       return retryAfter * state.retryAfterBaseValue;
     }
   });

src/types.ts

@@ -8,9 +8,10 @@ declare module "@octokit/core/dist-types/types.d" {
 }
 
 type LimitHandler = (
-  retryAfter?: number,
-  options?: object,
-  octokit?: Octokit
+  retryAfter: number,
+  options: object,
+  octokit: Octokit,
+  retryCount: number
 ) => void;
 
 export type AbuseLimitHandler = {

src/wrap-request.ts

@@ -12,7 +12,7 @@ async function doRequest(state, request, options) {
   const isSearch = options.method === "GET" && pathname.startsWith("/search/");
   const isGraphQL = pathname.startsWith("/graphql");
 
-  const retryCount = ~~options.request.retryCount;
+  const retryCount = ~~request.retryCount;
   const jobOptions = retryCount > 0 ? { priority: 0, weight: 0 } : {};
   if (state.clustering) {
     // Remove a job from Redis if it has not completed or failed within 60s

test/retry.test.ts

@@ -1,5 +1,9 @@
 import Bottleneck from "bottleneck";
 import { TestOctokit } from "./octokit";
+import { Octokit } from "@octokit/core";
+import { throttling } from "../src";
+import { AddressInfo } from "net";
+import { createServer } from "http";
 
 describe("Retry", function () {
   describe("REST", function () {
@@ -114,6 +118,55 @@ describe("Retry", function () {
       expect(ms2).toBeGreaterThan(80);
     });
 
+    it("Should not leak retryCount between requests", async function () {
+      let counter = 1;
+
+      const server = createServer((req, res) => {
+        if (counter++ % 3 === 0) {
+          res
+            .writeHead(200, { "Content-Type": "application/json" })
+            .end(JSON.stringify({ message: "Success!" }));
+        } else {
+          res
+            .writeHead(403, {
+              "Content-Type": "application/json",
+              "retry-after": "1",
+            })
+            .end(
+              JSON.stringify({
+                message: "You have exceeded a secondary rate limit",
+              })
+            );
+        }
+      });
+
+      server.listen(0);
+      const { port } = server.address() as AddressInfo;
+
+      const ThrottledOctokit = Octokit.plugin(throttling);
+      const octokit = new ThrottledOctokit({
+        baseUrl: `http://localhost:${port}`,
+        throttle: {
+          minimumSecondaryRateRetryAfter: 0,
+          retryAfterBaseValue: 50,
+          onRateLimit: () => true,
+          onSecondaryRateLimit: (retryAfter, options, octokit, retryCount) => {
+            if (retryCount < 5) {
+              return true;
+            }
+          },
+        },
+      });
+
+      try {
+        await octokit.request("GET /nope-nope-ok");
+        await octokit.request("GET /nope-nope-ok");
+        await octokit.request("GET /nope-nope-ok");
+      } finally {
+        server.close();
+      }
+    });
+
     it("Should retry 'rate-limit' and succeed", async function () {
       let eventCount = 0;
       const octokit = new TestOctokit({