fix: do not retry GraphQl request in case for 401 (#295)

This updates the bottleneck "failed" handler to not retry GraphQL
requests that receive a 401 response. The old behavior was to retry all
GraphQL error responses, which caused the retry to be throttled for an
hour, due to 401 Unauthorized responses including an
`x-ratelimit-remaining` of `0` and a `x-ratelimit-reset` of now + 3600
seconds. The main issue was that the request wasn't being ratelimited,
but the ratelimit headers caused the plugin to wait.

In testing this out, I noticed that GraphQL responses can also receive a
403 Forbidden response when hitting the abuse rate limit. This commit
keeps the functionality for retrying 403 responses for GraphQL requests,
since they signify abuse rate limiting, where 401 responses indicate bad
credentials.

Author: copperwall

README.md

@@ -5,7 +5,7 @@
 [![@latest](https://img.shields.io/npm/v/@octokit/plugin-throttling.svg)](https://www.npmjs.com/package/@octokit/plugin-throttling)
 [![Build Status](https://github.com/octokit/plugin-throttling.js/workflows/Test/badge.svg)](https://github.com/octokit/plugin-throttling.js/actions?workflow=Test)
 
-Implements all [recommended best practises](https://developer.github.com/v3/guides/best-practices-for-integrators/) to prevent hitting abuse rate limits.
+Implements all [recommended best practises](https://docs.github.com/en/rest/guides/best-practices-for-integrators) to prevent hitting abuse rate limits.
 
 ## Usage
 

src/index.ts

@@ -109,9 +109,10 @@ export function throttling(octokit: Octokit, octokitOptions = {}) {
   // @ts-ignore
   state.retryLimiter.on("failed", async function (error, info) {
     const options = info.args[info.args.length - 1];
-    const isGraphQL = options.url.startsWith("/graphql");
+    const shouldRetryGraphQL =
+      options.url.startsWith("/graphql") && error.status !== 401;
 
-    if (!(isGraphQL || error.status === 403)) {
+    if (!(shouldRetryGraphQL || error.status === 403)) {
       return;
     }
 
@@ -120,8 +121,8 @@ export function throttling(octokit: Octokit, octokitOptions = {}) {
 
     const { wantRetry, retryAfter } = await (async function () {
       if (/\babuse\b/i.test(error.message)) {
-        // The user has hit the abuse rate limit. (REST only)
-        // https://developer.github.com/v3/#abuse-rate-limits
+        // The user has hit the abuse rate limit. (REST and GraphQL)
+        // https://docs.github.com/en/rest/overview/resources-in-the-rest-api#abuse-rate-limits
 
         // The Retry-After header can sometimes be blank when hitting an abuse limit,
         // but is always present after 2-3s, so make sure to set `retryAfter` to at least 5s by default.
@@ -142,7 +143,8 @@ export function throttling(octokit: Octokit, octokitOptions = {}) {
         error.headers["x-ratelimit-remaining"] === "0"
       ) {
         // The user has used all their allowed calls for the current time period (REST and GraphQL)
-        // https://developer.github.com/v3/#rate-limiting
+        // https://docs.github.com/en/rest/reference/rate-limit (REST)
+        // https://docs.github.com/en/graphql/overview/resource-limitations#rate-limit (GraphQL)
 
         const rateLimitReset = new Date(
           ~~error.headers["x-ratelimit-reset"] * 1000

test/retry.test.ts

@@ -251,5 +251,94 @@ describe("Retry", function () {
         "END POST /graphql",
       ]);
     });
+
+    it("Should ignore 401 Unauthorized errors", async function () {
+      let eventCount = 0;
+      const octokit = new TestOctokit({
+        throttle: {
+          write: new Bottleneck.Group({ minTime: 50 }),
+          onRateLimit: (retryAfter: number, options: object) => {
+            eventCount++;
+            return true;
+          },
+          onAbuseLimit: () => 1,
+        },
+      });
+
+      try {
+        await octokit.request("POST /graphql", {
+          request: {
+            responses: [
+              {
+                status: 401,
+                headers: {
+                  "x-ratelimit-remaining": "0",
+                  "x-ratelimit-reset": "123",
+                },
+                data: {
+                  message: "Bad credentials",
+                  documentation_url: "https://docs.github.com/graphql",
+                },
+              },
+            ],
+          },
+        });
+        throw new Error("Should not reach this point");
+      } catch (error) {
+        expect(error.status).toEqual(401);
+        expect(error.message).toEqual("Bad credentials");
+      }
+
+      expect(eventCount).toEqual(0);
+      expect(octokit.__requestLog).toStrictEqual(["START POST /graphql"]);
+    });
+
+    it("Should retry 403 Forbidden errors on abuse limit", async function () {
+      let eventCount = 0;
+      const octokit = new TestOctokit({
+        throttle: {
+          write: new Bottleneck.Group({ minTime: 50 }),
+          onAbuseLimit: (retryAfter: number, options: object) => {
+            eventCount++;
+            return true;
+          },
+          onRateLimit: () => 1,
+          minimumAbuseRetryAfter: 0,
+          retryAfterBaseValue: 50,
+        },
+      });
+      const res = await octokit.request("POST /graphql", {
+        request: {
+          responses: [
+            {
+              status: 403,
+              headers: {
+                "retry-after": 1,
+              },
+              data: {
+                message:
+                  "You have triggered an abuse detection mechanism. Please wait a few minutes before you try again.",
+                documentation_url:
+                  "https://developer.github.com/v3/#abuse-rate-limits",
+              },
+            },
+            { status: 200, headers: {}, data: { message: "Success!" } },
+          ],
+        },
+      });
+
+      expect(res.status).toEqual(200);
+      expect(res.data).toMatchObject({ message: "Success!" });
+      expect(eventCount).toEqual(1);
+      expect(octokit.__requestLog).toStrictEqual([
+        "START POST /graphql",
+        "START POST /graphql",
+        "END POST /graphql",
+      ]);
+
+      const ms = octokit.__requestTimings[1] - octokit.__requestTimings[0];
+      expect(ms).toBeLessThan(80);
+      expect(ms).toBeGreaterThan(20);
+    });
   });
 });