fix: don't deadlock on 10+ concurrent requests needing auth (#779)

* fix: don't deadlock on 10+ concurrent requests needing auth

give auth requests a dedicated queue, so that they don't cause a
deadlock when too many requests are queued

* revert unnecessary change to testPlugin

* update TODO to reference related issue

* bump @octokit/auth-app and simplify test

* add more auth urls

Author: jenseng

package-lock.json

@@ -29,6 +29,7 @@
     "@octokit/core": "^6.1.3"
   },
   "devDependencies": {
+    "@octokit/auth-app": "^7.2.0",
     "@octokit/core": "^6.1.3",
     "@octokit/request-error": "^6.1.6",
     "@octokit/tsconfig": "^4.0.0",

package.json

@@ -30,6 +30,11 @@ const createGroups = function (
     maxConcurrent: 10,
     ...common,
   });
+  groups.auth = new Bottleneck.Group({
+    id: "octokit-auth",
+    maxConcurrent: 1,
+    ...common,
+  });
   groups.search = new Bottleneck.Group({
     id: "octokit-search",
     maxConcurrent: 1,

src/index.ts

@@ -39,6 +39,7 @@ export type ThrottlingOptions =
 
 export type Groups = {
   global?: Bottleneck.Group;
+  auth?: Bottleneck.Group;
   write?: Bottleneck.Group;
   search?: Bottleneck.Group;
   notifications?: Bottleneck.Group;

src/types.ts

@@ -20,8 +20,10 @@ async function doRequest(
   ) => Promise<OctokitResponse<any>>) & { retryCount: number },
   options: Required<EndpointDefaults>,
 ) {
-  const isWrite = options.method !== "GET" && options.method !== "HEAD";
   const { pathname } = new URL(options.url, "http://github.test");
+  const isAuth = isAuthRequest(options.method, pathname);
+  const isWrite =
+    !isAuth && options.method !== "GET" && options.method !== "HEAD";
   const isSearch = options.method === "GET" && pathname.startsWith("/search/");
   const isGraphQL = pathname.startsWith("/graphql");
 
@@ -50,7 +52,7 @@ async function doRequest(
     await state.search.key(state.id).schedule(jobOptions, noop);
   }
 
-  const req = state.global
+  const req = (isAuth ? state.auth : state.global)
     .key(state.id)
     .schedule<
       OctokitResponse<any>,
@@ -73,3 +75,18 @@ async function doRequest(
   }
   return req;
 }
+
+function isAuthRequest(method: string, pathname: string) {
+  return (
+    (method === "PATCH" &&
+      // https://docs.github.com/en/rest/apps/apps?apiVersion=2022-11-28#create-a-scoped-access-token
+      /^\/applications\/[^/]+\/token\/scoped$/.test(pathname)) ||
+    (method === "POST" &&
+      // https://docs.github.com/en/rest/apps/oauth-applications?apiVersion=2022-11-28#reset-a-token
+      (/^\/applications\/[^/]+\/token$/.test(pathname) ||
+        // https://docs.github.com/en/rest/apps/apps?apiVersion=2022-11-28#create-an-installation-access-token-for-an-app
+        /^\/app\/installations\/[^/]+\/access_tokens$/.test(pathname) ||
+        // https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps
+        pathname === "/login/oauth/access_token"))
+  );
+}

src/wrap-request.ts

@@ -1,7 +1,12 @@
 import { describe, it, expect } from "vitest";
 import Bottleneck from "bottleneck";
+import { createAppAuth } from "@octokit/auth-app";
 import { TestOctokit } from "./octokit.ts";
 import { throttling } from "../src/index.ts";
+import { Octokit } from "@octokit/core";
+import * as crypto from "node:crypto";
+import { promisify } from "node:util";
+const generateKeyPair = promisify(crypto.generateKeyPair);
 
 describe("General", function () {
   it("Should be possible to disable the plugin", async function () {
@@ -377,4 +382,85 @@ describe("GitHub API best practices", function () {
       octokit.__requestTimings[12] - octokit.__requestTimings[10],
     ).toBeLessThan(30);
   });
+
+  it("should not deadlock concurrent auth requests", async function () {
+    // instrument a fake fetch rather than using TestOctokit; this way
+    // @octokit/auth-app's request hook will actually run and we can
+    // track all requests (auth ones too, not just the top-level ones
+    // we make)
+    const requestLog: string[] = [];
+    const fakeFetch = async (url: string, init: any) => {
+      requestLog.push(`${init.method.toUpperCase()} ${url}`);
+      let data = {};
+      if (init.method === "POST" && url.includes("/app/installations/")) {
+        data = {
+          token: "token",
+          expires_at: new Date(Date.now() + 60_000).toISOString(),
+          permissions: {},
+          single_file: "",
+        };
+      }
+
+      return Promise.resolve(
+        new Response(JSON.stringify(data), {
+          status: 200,
+          headers: { "content-type": "application/json" },
+        }),
+      );
+    };
+    // jsonwebtoken needs a valid private key to sign the JWT, though the
+    // actual value doesn't matter since nothing will validate it
+    const privateKey = (
+      await generateKeyPair("rsa", {
+        modulusLength: 4096,
+        publicKeyEncoding: { type: "spki", format: "pem" },
+        privateKeyEncoding: { type: "pkcs8", format: "pem" },
+      })
+    ).privateKey;
+
+    const octokit = new (Octokit.plugin(throttling))({
+      authStrategy: createAppAuth,
+      auth: {
+        appId: 123,
+        privateKey,
+        installationId: 456,
+      },
+      throttle: {
+        onSecondaryRateLimit: () => 0,
+        onRateLimit: () => 0,
+      },
+      request: {
+        fetch: fakeFetch,
+      },
+    });
+
+    const routes = [
+      "/route01",
+      "/route02",
+      "/route03",
+      "/route04",
+      "/route05",
+      "/route06",
+      "/route07",
+      "/route08",
+      "/route09",
+      "/route10",
+    ];
+
+    await Promise.all(routes.map((route) => octokit.request(`GET ${route}`)));
+
+    expect(requestLog).toStrictEqual([
+      "POST https://api.github.com/app/installations/456/access_tokens",
+      "GET https://api.github.com/route01",
+      "GET https://api.github.com/route02",
+      "GET https://api.github.com/route03",
+      "GET https://api.github.com/route04",
+      "GET https://api.github.com/route05",
+      "GET https://api.github.com/route06",
+      "GET https://api.github.com/route07",
+      "GET https://api.github.com/route08",
+      "GET https://api.github.com/route09",
+      "GET https://api.github.com/route10",
+    ]);
+  });
 });