refactor(service): optimize serverless app initialization and add cache

apps/service/project.json

@@ -76,6 +76,10 @@
           "main": "{projectRoot}/vercel/serverless.ts",
           "thirdParty": true,
           "minify": true,
+          "esbuildOptions": {
+            "sourcemap": true,
+            "outfile": "{projectRoot}/api/serverless.js"
+          },
           "assets": [
             {
               "glob": "tasks.js",

apps/service/src/app.ts

@@ -196,14 +196,14 @@ async function init({ isServerless }: InitParams) {
     await closeMongoClient();
   });
 
-  process.on('SIGTERM', () => _gracefulShutdown(app, 'SIGTERM'));
-  process.on('SIGINT', () => _gracefulShutdown(app, 'SIGINT'));
+  if (!isServerless) {
+    process.on('SIGTERM', () => _gracefulShutdown(app, 'SIGTERM'));
+    process.on('SIGINT', () => _gracefulShutdown(app, 'SIGINT'));
 
-  if (!isServerless && shouldRunDbInit) {
-    await initDb(app.log);
-  }
+    if (shouldRunDbInit) {
+      await initDb(app.log);
+    }
 
-  if (!isServerless) {
     // starts only if at least one task has a schedule
     stopScheduler = startScheduler(app.log);
   }

apps/service/src/controllers/utils/__tests__/auth.spec.ts

@@ -0,0 +1,82 @@
+import { checkAuth } from '../auth';
+import { getProject } from '../../../framework/mongo/projects';
+import { verifyHash } from '../../../utils/hashUtils';
+import { CreateCommitRecordAuthType } from '../../../consts/commitRecords';
+
+import type { FastifyBaseLogger } from 'fastify';
+
+jest.mock('../../../framework/mongo/projects');
+jest.mock('../../../framework/github');
+jest.mock('../../../utils/hashUtils');
+
+const log = { warn: jest.fn(), info: jest.fn() } as unknown as FastifyBaseLogger;
+
+// the cache is module level, each test uses its own project id
+let testCounter = 0;
+
+const mockProject = (hash = 'salt:hash') => {
+  const id = `project-${++testCounter}`;
+
+  jest.mocked(getProject).mockResolvedValue({ id, apiKey: { hash } } as any);
+
+  return id;
+};
+
+const auth = (projectId: string, token: string) =>
+  checkAuth(projectId, { authType: CreateCommitRecordAuthType.ProjectApiKey, token }, undefined, log);
+
+describe('checkAuth - API key', () => {
+  beforeEach(() => {
+    jest.resetAllMocks();
+  });
+
+  test('same key twice runs verifyHash once', async () => {
+    const projectId = mockProject();
+    jest.mocked(verifyHash).mockResolvedValue(true);
+
+    expect(await auth(projectId, 'key')).toEqual({ authenticated: true });
+    expect(await auth(projectId, 'key')).toEqual({ authenticated: true });
+
+    expect(verifyHash).toHaveBeenCalledTimes(1);
+  });
+
+  test('wrong key is never cached', async () => {
+    const projectId = mockProject();
+    jest.mocked(verifyHash).mockResolvedValue(false);
+
+    expect(await auth(projectId, 'wrong-key')).toEqual({ authenticated: false, error: 'forbidden' });
+    expect(await auth(projectId, 'wrong-key')).toEqual({ authenticated: false, error: 'forbidden' });
+
+    expect(verifyHash).toHaveBeenCalledTimes(2);
+  });
+
+  test('a cached key does not authenticate a different key', async () => {
+    const projectId = mockProject();
+    jest.mocked(verifyHash).mockResolvedValueOnce(true).mockResolvedValueOnce(false);
+
+    expect(await auth(projectId, 'key')).toEqual({ authenticated: true });
+    expect(await auth(projectId, 'other-key')).toEqual({ authenticated: false, error: 'forbidden' });
+  });
+
+  test('a cached key does not authenticate another project', async () => {
+    const projectId1 = mockProject();
+    jest.mocked(verifyHash).mockResolvedValueOnce(true).mockResolvedValueOnce(false);
+
+    expect(await auth(projectId1, 'key')).toEqual({ authenticated: true });
+
+    const projectId2 = mockProject();
+    expect(await auth(projectId2, 'key')).toEqual({ authenticated: false, error: 'forbidden' });
+  });
+
+  test('rotated API key (new stored hash) invalidates the cached key', async () => {
+    const projectId = mockProject('salt:old-hash');
+    jest.mocked(verifyHash).mockResolvedValueOnce(true).mockResolvedValueOnce(false);
+
+    expect(await auth(projectId, 'old-key')).toEqual({ authenticated: true });
+
+    jest.mocked(getProject).mockResolvedValue({ id: projectId, apiKey: { hash: 'salt:new-hash' } } as any);
+
+    expect(await auth(projectId, 'old-key')).toEqual({ authenticated: false, error: 'forbidden' });
+    expect(verifyHash).toHaveBeenCalledTimes(2);
+  });
+});

apps/service/src/controllers/utils/auth.ts

@@ -1,9 +1,11 @@
+import { createHash } from 'node:crypto';
 import { Octokit } from '@octokit/rest';
 import { getProject, Project } from '../../framework/mongo/projects';
 import { verifyHash } from '../../utils/hashUtils';
 import { createOctokitClientByAction } from '../../framework/github';
 import { CreateCommitRecordAuthType } from '../../consts/commitRecords';
 import { isGitHubProject } from '../../utils/projectUtils';
+import { TtlCache } from '../../utils/ttlCache';
 
 import type { FastifyBaseLogger } from 'fastify';
 import type { CreateCommitRecordRequestQuery } from '../../types/schemas';
@@ -51,15 +53,31 @@ export async function checkAuth(
   return { authenticated: false, error: 'forbidden' };
 }
 
+// scrypt is intentionally expensive, so successful verifications are cached for a short time.
+// The key is a digest of (project id, stored hash, API key): the API key is never stored as is,
+// and rotating the project API key (new stored hash) invalidates the cached entry.
+const verifiedApiKeysCache = new TtlCache<string, true>(10 * 60 * 1000, 1000);
+
+function getVerifiedApiKeyCacheKey(projectId: string, storedHash: string, apiKey: string) {
+  return createHash('sha256').update(`${projectId}\0${storedHash}\0${apiKey}`).digest('hex');
+}
+
 async function handleApiKeyAuth(project: Project, apiKey: string, log: FastifyBaseLogger): Promise<CheckAuthResponse> {
   if (!('apiKey' in project)) {
     log.warn({ projectId: project.id }, 'API key sent, but project dont have API key');
     return { authenticated: false, error: 'forbidden' };
   }
 
+  const cacheKey = getVerifiedApiKeyCacheKey(project.id, project.apiKey.hash, apiKey);
+
+  if (verifiedApiKeysCache.get(cacheKey)) {
+    return { authenticated: true };
+  }
+
   const isAuthenticated = await verifyHash(apiKey, project.apiKey.hash);
 
   if (isAuthenticated) {
+    verifiedApiKeysCache.set(cacheKey, true);
     return { authenticated: true };
   }
 

apps/service/src/framework/__tests__/github.spec.ts

@@ -0,0 +1,193 @@
+import { createOctokitClientByAction, createOctokitClientByRepo, getInstallationId } from '../github';
+
+import type { FastifyBaseLogger } from 'fastify';
+
+const mockGetRepoInstallation = jest.fn();
+const mockGetWorkflowRun = jest.fn();
+let mockOctokitCreatedCount = 0;
+
+jest.mock('../env', () => ({
+  githubAppId: '1',
+  githubAppPrivateKey: 'private-key',
+  githubAppClientId: 'client-id',
+  githubAppClientSecret: 'client-secret',
+}));
+jest.mock('../mongo/commitRecords', () => ({}));
+jest.mock('@octokit/auth-app', () => ({ createAppAuth: jest.fn(), createOAuthUserAuth: jest.fn() }));
+jest.mock('@octokit/rest', () => ({
+  Octokit: class {
+    id = ++mockOctokitCreatedCount;
+    apps = { getRepoInstallation: mockGetRepoInstallation };
+    actions = { getWorkflowRun: mockGetWorkflowRun };
+  },
+}));
+
+const log = { info: jest.fn(), warn: jest.fn() } as unknown as FastifyBaseLogger;
+
+// the caches are module level, each test uses its own repo / run id
+let testCounter = 0;
+const uniqueRepo = () => ({ owner: 'owner', repo: `repo-${++testCounter}` });
+
+const flushPromises = () => new Promise((resolve) => setImmediate(resolve));
+
+describe('github caches', () => {
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+
+  describe('getInstallationId', () => {
+    test('parallel calls trigger one fetch', async () => {
+      const { owner, repo } = uniqueRepo();
+      mockGetRepoInstallation.mockResolvedValue({ data: { id: 123 } });
+
+      const results = await Promise.all([
+        getInstallationId(owner, repo),
+        getInstallationId(owner, repo),
+        getInstallationId(owner, repo),
+      ]);
+
+      expect(results).toEqual([123, 123, 123]);
+      expect(mockGetRepoInstallation).toHaveBeenCalledTimes(1);
+
+      // cached for later calls
+      expect(await getInstallationId(owner, repo)).toBe(123);
+      expect(mockGetRepoInstallation).toHaveBeenCalledTimes(1);
+    });
+
+    test('cache key is case insensitive', async () => {
+      const { owner, repo } = uniqueRepo();
+      mockGetRepoInstallation.mockResolvedValue({ data: { id: 123 } });
+
+      await getInstallationId(owner, repo);
+      await getInstallationId(owner.toUpperCase(), repo.toUpperCase());
+
+      expect(mockGetRepoInstallation).toHaveBeenCalledTimes(1);
+    });
+
+    test('undefined (app not installed) is not cached', async () => {
+      const { owner, repo } = uniqueRepo();
+      mockGetRepoInstallation.mockRejectedValueOnce({ status: 404 });
+
+      expect(await getInstallationId(owner, repo)).toBeUndefined();
+
+      mockGetRepoInstallation.mockResolvedValueOnce({ data: { id: 456 } });
+
+      expect(await getInstallationId(owner, repo)).toBe(456);
+      expect(mockGetRepoInstallation).toHaveBeenCalledTimes(2);
+    });
+
+    test('errors are not cached', async () => {
+      const { owner, repo } = uniqueRepo();
+      mockGetRepoInstallation.mockRejectedValueOnce({ status: 500 });
+
+      await expect(getInstallationId(owner, repo)).rejects.toEqual({ status: 500 });
+
+      mockGetRepoInstallation.mockResolvedValueOnce({ data: { id: 789 } });
+
+      expect(await getInstallationId(owner, repo)).toBe(789);
+      expect(mockGetRepoInstallation).toHaveBeenCalledTimes(2);
+    });
+
+    test('cleanup of an expired entry does not delete a newer entry', async () => {
+      jest.useFakeTimers({ doNotFake: ['setImmediate', 'nextTick'] });
+
+      try {
+        const { owner, repo } = uniqueRepo();
+
+        let rejectFirst: (err: unknown) => void = () => {};
+        mockGetRepoInstallation.mockReturnValueOnce(new Promise((_, reject) => (rejectFirst = reject)));
+
+        const first = getInstallationId(owner, repo).catch(() => undefined);
+
+        // the first entry expires while still in flight, a newer one replaces it
+        jest.advanceTimersByTime(60 * 60 * 1000 + 1);
+        mockGetRepoInstallation.mockResolvedValueOnce({ data: { id: 1000 } });
+        expect(await getInstallationId(owner, repo)).toBe(1000);
+
+        rejectFirst({ status: 500 });
+        await first;
+        await flushPromises();
+
+        // the newer entry is still cached
+        expect(await getInstallationId(owner, repo)).toBe(1000);
+        expect(mockGetRepoInstallation).toHaveBeenCalledTimes(2);
+      } finally {
+        jest.useRealTimers();
+      }
+    });
+  });
+
+  describe('createOctokitClientByRepo', () => {
+    test('reuses the client of the same installation', async () => {
+      const first = uniqueRepo();
+      const second = uniqueRepo();
+      mockGetRepoInstallation.mockResolvedValue({ data: { id: 2000 + testCounter } });
+
+      const client1 = await createOctokitClientByRepo(first.owner, first.repo);
+      const client2 = await createOctokitClientByRepo(first.owner, first.repo);
+
+      expect(client1).toBe(client2);
+
+      // another repo with a different installation gets its own client
+      mockGetRepoInstallation.mockResolvedValue({ data: { id: 3000 + testCounter } });
+      expect(await createOctokitClientByRepo(second.owner, second.repo)).not.toBe(client1);
+    });
+  });
+
+  describe('createOctokitClientByAction', () => {
+    test('successful authentication is cached, parallel calls trigger one request', async () => {
+      const { owner, repo } = uniqueRepo();
+      mockGetRepoInstallation.mockResolvedValue({ data: { id: 4000 + testCounter } });
+      mockGetWorkflowRun.mockResolvedValue({ data: { status: 'in_progress' } });
+
+      const params = { owner, repo, runId: '1' };
+      const results = await Promise.all([
+        createOctokitClientByAction(params, log),
+        createOctokitClientByAction(params, log),
+      ]);
+      const another = await createOctokitClientByAction(params, log);
+
+      expect(results[0].authenticated).toBe(true);
+      expect(results[1]).toBe(results[0]);
+      expect(another).toBe(results[0]);
+      expect(mockGetWorkflowRun).toHaveBeenCalledTimes(1);
+    });
+
+    test('failed authentication is not cached', async () => {
+      const { owner, repo } = uniqueRepo();
+      mockGetRepoInstallation.mockResolvedValue({ data: { id: 5000 + testCounter } });
+      mockGetWorkflowRun.mockResolvedValueOnce({ data: { status: 'completed' } });
+
+      const params = { owner, repo, runId: '2' };
+
+      expect((await createOctokitClientByAction(params, log)).authenticated).toBe(false);
+
+      mockGetWorkflowRun.mockResolvedValueOnce({ data: { status: 'in_progress' } });
+
+      expect((await createOctokitClientByAction(params, log)).authenticated).toBe(true);
+      expect(mockGetWorkflowRun).toHaveBeenCalledTimes(2);
+    });
+
+    test('stale installation id is evicted on error', async () => {
+      const { owner, repo } = uniqueRepo();
+      const staleInstallationId = 6000 + testCounter;
+
+      mockGetRepoInstallation.mockResolvedValueOnce({ data: { id: staleInstallationId } });
+      mockGetWorkflowRun.mockRejectedValueOnce({ status: 404 });
+
+      const result = await createOctokitClientByAction({ owner, repo, runId: '3' }, log);
+
+      expect(result).toMatchObject({ authenticated: false });
+
+      // the app was reinstalled, the next request looks up the new installation id
+      mockGetRepoInstallation.mockResolvedValueOnce({ data: { id: staleInstallationId + 1 } });
+      mockGetWorkflowRun.mockResolvedValueOnce({ data: { status: 'queued' } });
+
+      const retry = await createOctokitClientByAction({ owner, repo, runId: '4' }, log);
+
+      expect(retry.authenticated).toBe(true);
+      expect(mockGetRepoInstallation).toHaveBeenCalledTimes(2);
+      expect(await getInstallationId(owner, repo)).toBe(staleInstallationId + 1);
+    });
+  });
+});