feat: coalesce concurrent authentication calls (#689)

* feat: coalesce concurrent authentication calls

* lint fix

* move types

Author: jenseng

src/cache.ts

@@ -2,7 +2,7 @@ import { Lru } from "toad-cache";
 
 /* v8 ignore next */
 import type {
-  InstallationAuthOptions,
+  CacheableInstallationAuthOptions,
   Cache,
   CacheData,
   Permissions,
@@ -21,7 +21,7 @@ export function getCache() {
 
 export async function get(
   cache: Cache,
-  options: InstallationAuthOptions,
+  options: CacheableInstallationAuthOptions,
 ): Promise<InstallationAccessTokenData | void> {
   const cacheKey = optionsToCacheKey(options);
   const result = await cache.get(cacheKey);
@@ -66,7 +66,7 @@ export async function get(
 }
 export async function set(
   cache: Cache,
-  options: InstallationAuthOptions,
+  options: CacheableInstallationAuthOptions,
   data: CacheData,
 ): Promise<void> {
   const key = optionsToCacheKey(options);
@@ -91,12 +91,13 @@ export async function set(
   await cache.set(key, value);
 }
 
-function optionsToCacheKey({
+// TODO: consider baseUrl and appId too, so that we don't incorrectly cache tokens across them
+export function optionsToCacheKey({
   installationId,
   permissions = {},
   repositoryIds = [],
   repositoryNames = [],
-}: InstallationAuthOptions) {
+}: CacheableInstallationAuthOptions) {
   const permissionsString = Object.keys(permissions)
     .sort()
     .map((name) => (permissions[name] === "read" ? name : `${name}!`))

src/get-installation-authentication.ts

@@ -1,11 +1,12 @@
-import { get, set } from "./cache.js";
+import { get, optionsToCacheKey, set } from "./cache.js";
 import { getAppAuthentication } from "./get-app-authentication.js";
 import { toTokenAuthentication } from "./to-token-authentication.js";
 import type {
   InstallationAuthOptions,
   InstallationAccessTokenAuthentication,
   RequestInterface,
   State,
+  CacheableInstallationAuthOptions,
 } from "./types.js";
 
 export async function getInstallationAuthentication(
@@ -30,16 +31,45 @@ export async function getInstallationAuthentication(
     return factory(factoryAuthOptions);
   }
 
-  const optionsWithInstallationTokenFromState = Object.assign(
-    { installationId },
-    options,
+  const request = customRequest || state.request;
+
+  return getInstallationAuthenticationConcurrently(
+    state,
+    { ...options, installationId },
+    request,
   );
+}
 
+const pendingPromises = new Map<
+  string,
+  Promise<InstallationAccessTokenAuthentication>
+>();
+
+function getInstallationAuthenticationConcurrently(
+  state: State,
+  options: CacheableInstallationAuthOptions,
+  request: RequestInterface,
+) {
+  const cacheKey = optionsToCacheKey(options);
+  if (pendingPromises.has(cacheKey)) {
+    return pendingPromises.get(cacheKey)!;
+  }
+  const promise = getInstallationAuthenticationImpl(
+    state,
+    options,
+    request,
+  ).finally(() => pendingPromises.delete(cacheKey));
+  pendingPromises.set(cacheKey, promise);
+  return promise;
+}
+
+async function getInstallationAuthenticationImpl(
+  state: State,
+  options: CacheableInstallationAuthOptions,
+  request: RequestInterface,
+) {
   if (!options.refresh) {
-    const result = await get(
-      state.cache,
-      optionsWithInstallationTokenFromState,
-    );
+    const result = await get(state.cache, options);
 
     if (result) {
       const {
@@ -54,7 +84,7 @@ export async function getInstallationAuthentication(
       } = result;
 
       return toTokenAuthentication({
-        installationId,
+        installationId: options.installationId,
         token,
         createdAt,
         expiresAt,
@@ -68,10 +98,9 @@ export async function getInstallationAuthentication(
   }
 
   const appAuthentication = await getAppAuthentication(state);
-  const request = customRequest || state.request;
 
   const payload = {
-    installation_id: installationId,
+    installation_id: options.installationId,
     mediaType: {
       previews: ["machine-man"],
     },
@@ -136,10 +165,10 @@ export async function getInstallationAuthentication(
     Object.assign(payload, { singleFileName });
   }
 
-  await set(state.cache, optionsWithInstallationTokenFromState, cacheOptions);
+  await set(state.cache, options, cacheOptions);
 
   const cacheData = {
-    installationId,
+    installationId: options.installationId,
     token,
     createdAt,
     expiresAt,

src/types.ts

@@ -58,6 +58,11 @@ export type InstallationAuthOptionsWithFactory<T> = {
   [key: string]: unknown;
 };
 
+// normalized installation auth options that can be used for caching
+export type CacheableInstallationAuthOptions = InstallationAuthOptions & {
+  installationId: number;
+};
+
 export type OAuthAppAuthOptions = OAuthAppAuth.AppAuthOptions;
 export type OAuthWebFlowAuthOptions = OAuthAppAuth.WebFlowAuthOptions;
 export type OAuthDeviceFlowAuthOptions =

test/index.test.ts

@@ -1,11 +1,13 @@
-import fetchMock, { type MockMatcherFunction } from "fetch-mock";
+import fetchMock from "fetch-mock";
 
 import { request } from "@octokit/request";
 import { vi, beforeEach, test, expect } from "vitest";
 
 import { createAppAuth, createOAuthUserAuth } from "../src/index.ts";
 import type { FactoryInstallation } from "../src/types.ts";
 
+type MockMatcherFunction = fetchMock.MockMatcherFunction;
+
 const APP_ID = 1;
 const PRIVATE_KEY = `-----BEGIN RSA PRIVATE KEY-----
 MIIEpAIBAAKCAQEA1c7+9z5Pad7OejecsQ0bu3aozN3tihPmljnnudb9G3HECdnH
@@ -397,6 +399,111 @@ test("repositoryIds auth option", async () => {
   });
 });
 
+test("installationId strategy option coalesces concurrent calls with the same options", async () => {
+  const sandbox = fetchMock
+    .sandbox()
+    .post("https://api.github.com/app/installations/123/access_tokens", {
+      token: "secret123",
+      expires_at: "1970-01-01T01:00:00.000Z",
+      permissions: {
+        metadata: "read",
+      },
+      repository_selection: "all",
+    })
+    .postOnce("https://api.github.com/app/installations/234/access_tokens", {
+      token: "secret234",
+      expires_at: "1970-01-01T01:00:00.000Z",
+      permissions: {
+        metadata: "read",
+      },
+      repository_selection: "all",
+    });
+  const requestMock = request.defaults({
+    headers: {
+      "user-agent": "test",
+    },
+    request: {
+      fetch: sandbox,
+    },
+  });
+
+  const auth = createAppAuth({
+    appId: APP_ID,
+    privateKey: PRIVATE_KEY,
+    installationId: 123,
+    request: requestMock,
+  });
+  const unrelatedAuth = createAppAuth({
+    appId: APP_ID,
+    privateKey: PRIVATE_KEY,
+    installationId: 234,
+    request: requestMock,
+  });
+
+  const authentications = await Promise.all([
+    auth({ type: "installation", refresh: true }),
+    auth({ type: "installation", refresh: true }),
+    unrelatedAuth({ type: "installation", refresh: true }),
+  ]);
+  expect(authentications[0]).toEqual({
+    type: "token",
+    token: "secret123",
+    tokenType: "installation",
+    installationId: 123,
+    permissions: {
+      metadata: "read",
+    },
+    createdAt: "1970-01-01T00:00:00.000Z",
+    expiresAt: "1970-01-01T01:00:00.000Z",
+    repositorySelection: "all",
+  });
+  // same result as the first one
+  expect(authentications[1]).toBe(authentications[0]);
+  // and only request was made
+  expect(
+    sandbox.calls("https://api.github.com/app/installations/123/access_tokens")
+      .length,
+  ).toBe(1);
+
+  // whereas the auth for the unrelated installation got a different token
+  expect(authentications[2]).toEqual({
+    type: "token",
+    token: "secret234",
+    tokenType: "installation",
+    installationId: 234,
+    permissions: {
+      metadata: "read",
+    },
+    createdAt: "1970-01-01T00:00:00.000Z",
+    expiresAt: "1970-01-01T01:00:00.000Z",
+    repositorySelection: "all",
+  });
+
+  // now that the original auth call has resolved, if we do it again it will actually run
+  const laterAuthentication = await auth({
+    type: "installation",
+    refresh: true,
+  });
+  expect(laterAuthentication).toEqual({
+    type: "token",
+    token: "secret123",
+    tokenType: "installation",
+    installationId: 123,
+    permissions: {
+      metadata: "read",
+    },
+    createdAt: "1970-01-01T00:00:00.000Z",
+    expiresAt: "1970-01-01T01:00:00.000Z",
+    repositorySelection: "all",
+  });
+
+  // a fresh request was made
+  expect(
+    sandbox.calls("https://api.github.com/app/installations/123/access_tokens")
+      .length,
+  ).toBe(2);
+});
+
 test("repositoryNames auth option", async () => {
   const matchCreateInstallationAccessToken: MockMatcherFunction = (
     _url,