Repository: octokit/auth-app.js (ts)

Issue:
[FEAT]: coalesce concurrent authentication calls
### Describe the need

#### Context
This is somewhat related to https://github.com/octokit/octokit.js/issues/2800. Although installation access tokens are cached for an hour, this caching only takes effect once a token is acquired. If a token is not yet cached (or is expired), and you trigger authentication multiple times concurrently, then multiple identical requests will be made to `/app/installations/.../access_tokens`. This is redundant and inefficient, and can negatively impact other API calls, depending on your throttling configuration. This suboptimal behavior was observed in writing tests for https://github.com/octokit/plugin-throttling.js/pull/779.

#### Proposal
If an authentication request is already in flight (i.e. `getInstallationAuthentication` has been called but the promise has not yet resolved), subsequent calls with the same options should return the same promise. This way we ensure that we only query and cache the token once.

### SDK Version

_No response_

### API Version

_No response_

### Relevant log output

```shell

```

### Code of Conduct

- [x] I agree to follow this project's Code of Conduct

Pull request description:
feat: coalesce concurrent authentication calls
<!-- Please refer to our contributing docs for any questions on submitting a pull request -->
<!-- Issues are required for both bug fixes and features. -->
Resolves #688

----

### Before the change?
<!-- Please describe the current behavior that you are modifying. -->

* Concurrent identical calls to `getInstallationAuthentication` would result in redundant token acquisition (generate a JWT, post to `/app/installations/.../access_tokens`, and cache the token, with the last one winning)

### After the change?

* Concurrent identical calls to `getInstallationAuthentication` will all resolve to the same promise, ensuring that the token is only acquired once.

### Pull request checklist
- [x] Tests for the changes have been added (for bug fixes / features)
- [x] Docs have been reviewed and added / updated if needed (for bug fixes / features)
  - N/A, since this is really just a performance optimization

### Does this introduce a breaking change?
<!-- If this introduces a breaking change make sure to note it here any what the impact might be -->

Please see our docs on [breaking changes](https://github.com/octokit/.github/blob/master/community/breaking_changes.md) to help!

- [ ] Yes
- [x] No

No breaking changes. The only observable impact should be that API usage goes down and request throughput goes up when concurrent requests are made that need authentication. The improvement is most noticeable without https://github.com/octokit/plugin-throttling.js/pull/779, since currently authentications are treated as writes, meaning they incur an automatic [1 second penalty](https://github.com/octokit/plugin-throttling.js/blob/main/src/wrap-request.ts#L37-L41) between them.

----



Patch:
diff --git a/src/cache.ts b/src/cache.ts
index ecfe999..3022b9f 100644
--- a/src/cache.ts
+++ b/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}!`))
diff --git a/src/get-installation-authentication.ts b/src/get-installation-authentication.ts
index 889e5ee..08758f6 100644
--- a/src/get-installation-authentication.ts
+++ b/src/get-installation-authentication.ts
@@ -1,4 +1,4 @@
-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 {
@@ -6,6 +6,7 @@ import type {
   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,
diff --git a/src/types.ts b/src/types.ts
index 6f7d1d3..5134a52 100644
--- a/src/types.ts
+++ b/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 patch:
diff --git a/test/index.test.ts b/test/index.test.ts
index 612aec3..a5a9c6b 100644
--- a/test/index.test.ts
+++ b/test/index.test.ts
@@ -1,4 +1,4 @@
-import fetchMock, { type MockMatcherFunction } from "fetch-mock";
+import fetchMock from "fetch-mock";
 
 import { request } from "@octokit/request";
 import { vi, beforeEach, test, expect } from "vitest";
@@ -6,6 +6,8 @@ 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,