From 5e709a31b68559ff6d6d3701e883f6651d8d9fa3 Mon Sep 17 00:00:00 2001 From: LironEr Date: Fri, 9 Oct 2026 07:44:54 +0200 Subject: [PATCH 1/3] refactor(service): optimize serverless app initialization and add cache --- apps/service/src/app.ts | 12 +-- apps/service/src/controllers/utils/auth.ts | 18 ++++ apps/service/src/framework/github.ts | 82 ++++++++++++++++++- .../src/utils/__tests__/ttlCache.spec.ts | 63 ++++++++++++++ apps/service/src/utils/ttlCache.ts | 46 +++++++++++ apps/service/vercel/serverless.ts | 24 +++++- 6 files changed, 234 insertions(+), 11 deletions(-) create mode 100644 apps/service/src/utils/__tests__/ttlCache.spec.ts create mode 100644 apps/service/src/utils/ttlCache.ts diff --git a/apps/service/src/app.ts b/apps/service/src/app.ts index 5fae18a..4ad8193 100644 --- a/apps/service/src/app.ts +++ b/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); } diff --git a/apps/service/src/controllers/utils/auth.ts b/apps/service/src/controllers/utils/auth.ts index 34eb28e..2ef0a10 100644 --- a/apps/service/src/controllers/utils/auth.ts +++ b/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(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 { 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 }; } diff --git a/apps/service/src/framework/github.ts b/apps/service/src/framework/github.ts index 8193f77..aaa45aa 100644 --- a/apps/service/src/framework/github.ts +++ b/apps/service/src/framework/github.ts @@ -7,9 +7,18 @@ import { CommitRecordGitHubOutputs, getReportConclusionText, OutputResponse, Rep import type { RequestError } from '@octokit/types'; import type { FastifyBaseLogger } from 'fastify'; import { UserSessionData } from '@/types/auth'; +import { TtlCache } from '@/utils/ttlCache'; const WRITE_PERMISSIONS = ['admin', 'write']; +// In-memory caches, live as long as the process +// They avoid signing a JWT / minting an installation token / calling GitHub for every request. +const HOUR_MS = 60 * 60 * 1000; +const installationIdCache = new TtlCache>(HOUR_MS, 500); +const installationOctokitCache = new TtlCache(HOUR_MS, 200); +// a CI job sends a few requests with the same run id within seconds, and monorepo sub projects send them in parallel +const workflowRunAuthCache = new TtlCache>(2 * 60 * 1000, 500); + let _app: Octokit | undefined; function getAppAuth() { @@ -47,7 +56,36 @@ export const getGithubApp = () => { return _app; }; -export const getInstallationId = async (owner: string, repo: string): Promise => { +export const getInstallationId = (owner: string, repo: string): Promise => { + const key = `${owner}/${repo}`.toLowerCase(); + const cached = installationIdCache.get(key); + + if (cached) { + return cached; + } + + // the promise is cached to also dedupe parallel requests + const promise = fetchInstallationId(owner, repo).then( + (installationId) => { + if (installationId === undefined) { + // the app might be installed later + installationIdCache.delete(key); + } + + return installationId; + }, + (err) => { + installationIdCache.delete(key); + throw err; + } + ); + + installationIdCache.set(key, promise); + + return promise; +}; + +const fetchInstallationId = async (owner: string, repo: string): Promise => { try { const { data } = await getGithubApp().apps.getRepoInstallation({ owner, repo }); @@ -83,7 +121,38 @@ type CreateOctokitClientByActionResponse = } | { authenticated: true; installationOctokit: Octokit }; -export async function createOctokitClientByAction( +export function createOctokitClientByAction( + params: { owner: string; repo: string; commitSha?: string; runId: string }, + log: FastifyBaseLogger +): Promise { + const key = `${params.owner}/${params.repo}/${params.runId}`.toLowerCase(); + const cached = workflowRunAuthCache.get(key); + + if (cached) { + return cached; + } + + // only successful authentications are cached + const promise = authenticateWorkflowRun(params, log).then( + (result) => { + if (!result.authenticated) { + workflowRunAuthCache.delete(key); + } + + return result; + }, + (err) => { + workflowRunAuthCache.delete(key); + throw err; + } + ); + + workflowRunAuthCache.set(key, promise); + + return promise; +} + +async function authenticateWorkflowRun( { owner, repo, runId }: { owner: string; repo: string; commitSha?: string; runId: string }, log: FastifyBaseLogger ): Promise { @@ -137,6 +206,13 @@ export async function createOctokitClientByAction( } export function createOctokitClientByInstallationId(installationId: number) { + const cachedClient = installationOctokitCache.get(installationId); + + if (cachedClient) { + return cachedClient; + } + + // auth-app caches the installation token inside the client, so reusing the client reuses the token const client = new Octokit({ authStrategy: createAppAuth, auth: { @@ -145,6 +221,8 @@ export function createOctokitClientByInstallationId(installationId: number) { }, }); + installationOctokitCache.set(installationId, client); + return client; } diff --git a/apps/service/src/utils/__tests__/ttlCache.spec.ts b/apps/service/src/utils/__tests__/ttlCache.spec.ts new file mode 100644 index 0000000..10c24a3 --- /dev/null +++ b/apps/service/src/utils/__tests__/ttlCache.spec.ts @@ -0,0 +1,63 @@ +import { TtlCache } from '../ttlCache'; + +describe('TtlCache', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + test('returns cached value until it expires', () => { + const cache = new TtlCache(1000, 10); + cache.set('a', 1); + + expect(cache.get('a')).toBe(1); + + jest.advanceTimersByTime(999); + expect(cache.get('a')).toBe(1); + + jest.advanceTimersByTime(1); + expect(cache.get('a')).toBeUndefined(); + }); + + test('evicts the oldest entry when exceeding max size', () => { + const cache = new TtlCache(1000, 2); + cache.set('a', 1); + cache.set('b', 2); + cache.set('c', 3); + + expect(cache.get('a')).toBeUndefined(); + expect(cache.get('b')).toBe(2); + expect(cache.get('c')).toBe(3); + }); + + test('overriding a key refreshes its TTL and position', () => { + const cache = new TtlCache(1000, 2); + cache.set('a', 1); + cache.set('b', 2); + + jest.advanceTimersByTime(500); + cache.set('a', 10); + cache.set('c', 3); + + expect(cache.get('b')).toBeUndefined(); + + jest.advanceTimersByTime(600); + expect(cache.get('a')).toBe(10); + }); + + test('delete and clear', () => { + const cache = new TtlCache(1000, 10); + cache.set('a', 1); + cache.set('b', 2); + + cache.delete('a'); + expect(cache.get('a')).toBeUndefined(); + expect(cache.get('b')).toBe(2); + + cache.clear(); + expect(cache.get('b')).toBeUndefined(); + }); +}); diff --git a/apps/service/src/utils/ttlCache.ts b/apps/service/src/utils/ttlCache.ts new file mode 100644 index 0000000..0e4fd04 --- /dev/null +++ b/apps/service/src/utils/ttlCache.ts @@ -0,0 +1,46 @@ +/** + * Minimal in-memory cache with TTL and a max size (oldest entry is evicted first). + * Lives as long as the process (on Vercel - as long as the Fluid instance). + */ +export class TtlCache { + private readonly entries = new Map(); + + constructor( + private readonly ttlMs: number, + private readonly maxSize: number + ) {} + + get(key: K): V | undefined { + const entry = this.entries.get(key); + + if (!entry) { + return undefined; + } + + if (entry.expiresAt <= Date.now()) { + this.entries.delete(key); + return undefined; + } + + return entry.value; + } + + set(key: K, value: V): void { + // re-insert to move the key to the end of the insertion order + this.entries.delete(key); + this.entries.set(key, { value, expiresAt: Date.now() + this.ttlMs }); + + if (this.entries.size > this.maxSize) { + const oldestKey = this.entries.keys().next().value as K; + this.entries.delete(oldestKey); + } + } + + delete(key: K): void { + this.entries.delete(key); + } + + clear(): void { + this.entries.clear(); + } +} diff --git a/apps/service/vercel/serverless.ts b/apps/service/vercel/serverless.ts index 0d3c951..b1b9338 100644 --- a/apps/service/vercel/serverless.ts +++ b/apps/service/vercel/serverless.ts @@ -1,9 +1,27 @@ import init from '../src/app'; -export default async (req: any, res: any) => { - const app = await init({ isServerless: true }); +import type { FastifyInstance } from 'fastify'; + +// The app is built once per function instance (not per request), building it is expensive +let appPromise: Promise | undefined; + +function getApp() { + appPromise ??= init({ isServerless: true }) + .then(async (app) => { + await app.ready(); + return app; + }) + .catch((err) => { + // allow the next request to retry the initialization + appPromise = undefined; + throw err; + }); - await app.ready(); + return appPromise; +} + +export default async (req: any, res: any) => { + const app = await getApp(); app.server.emit('request', req, res); }; From 4e704378f42cd3940717dc89360672773a89a87c Mon Sep 17 00:00:00 2001 From: LironEr Date: Fri, 9 Oct 2026 08:07:07 +0200 Subject: [PATCH 2/3] fix --- .../controllers/utils/__tests__/auth.spec.ts | 82 ++++++++ .../src/framework/__tests__/github.spec.ts | 193 ++++++++++++++++++ apps/service/src/framework/github.ts | 29 ++- apps/service/src/utils/ttlCache.ts | 10 + apps/service/tests/commitRecordUtils.ts | 8 +- 5 files changed, 313 insertions(+), 9 deletions(-) create mode 100644 apps/service/src/controllers/utils/__tests__/auth.spec.ts create mode 100644 apps/service/src/framework/__tests__/github.spec.ts diff --git a/apps/service/src/controllers/utils/__tests__/auth.spec.ts b/apps/service/src/controllers/utils/__tests__/auth.spec.ts new file mode 100644 index 0000000..fe2ec46 --- /dev/null +++ b/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); + }); +}); diff --git a/apps/service/src/framework/__tests__/github.spec.ts b/apps/service/src/framework/__tests__/github.spec.ts new file mode 100644 index 0000000..629f34b --- /dev/null +++ b/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); + }); + }); +}); diff --git a/apps/service/src/framework/github.ts b/apps/service/src/framework/github.ts index aaa45aa..5884953 100644 --- a/apps/service/src/framework/github.ts +++ b/apps/service/src/framework/github.ts @@ -13,6 +13,8 @@ const WRITE_PERMISSIONS = ['admin', 'write']; // In-memory caches, live as long as the process // They avoid signing a JWT / minting an installation token / calling GitHub for every request. +// The installation id and workflow run caches hold promises (not results), so parallel requests +// (e.g. monorepo sub projects) that arrive before the first lookup finishes share it instead of all missing the cache. const HOUR_MS = 60 * 60 * 1000; const installationIdCache = new TtlCache>(HOUR_MS, 500); const installationOctokitCache = new TtlCache(HOUR_MS, 200); @@ -56,8 +58,10 @@ export const getGithubApp = () => { return _app; }; +const getInstallationIdCacheKey = (owner: string, repo: string) => `${owner}/${repo}`.toLowerCase(); + export const getInstallationId = (owner: string, repo: string): Promise => { - const key = `${owner}/${repo}`.toLowerCase(); + const key = getInstallationIdCacheKey(owner, repo); const cached = installationIdCache.get(key); if (cached) { @@ -69,13 +73,13 @@ export const getInstallationId = (owner: string, repo: string): Promise { if (installationId === undefined) { // the app might be installed later - installationIdCache.delete(key); + installationIdCache.deleteIfValue(key, promise); } return installationId; }, (err) => { - installationIdCache.delete(key); + installationIdCache.deleteIfValue(key, promise); throw err; } ); @@ -136,13 +140,13 @@ export function createOctokitClientByAction( const promise = authenticateWorkflowRun(params, log).then( (result) => { if (!result.authenticated) { - workflowRunAuthCache.delete(key); + workflowRunAuthCache.deleteIfValue(key, promise); } return result; }, (err) => { - workflowRunAuthCache.delete(key); + workflowRunAuthCache.deleteIfValue(key, promise); throw err; } ); @@ -156,14 +160,18 @@ async function authenticateWorkflowRun( { owner, repo, runId }: { owner: string; repo: string; commitSha?: string; runId: string }, log: FastifyBaseLogger ): Promise { + let installationId: number | undefined; + try { - const octokit = await createOctokitClientByRepo(owner, repo); + installationId = await getInstallationId(owner, repo); - if (!octokit) { + if (!installationId) { log.info({ owner, repo }, 'missing installation id'); return { authenticated: false, error: `BundleMon GitHub app is not installed for this repo (${owner}/${repo})` }; } + const octokit = createOctokitClientByInstallationId(installationId); + const res = await octokit.actions.getWorkflowRun({ owner, repo, run_id: Number(runId) }); // check job status @@ -192,6 +200,13 @@ async function authenticateWorkflowRun( installationOctokit: octokit, }; } catch (err) { + // the cached installation might be stale (e.g. the app was reinstalled), the next request will look it up again + installationIdCache.delete(getInstallationIdCacheKey(owner, repo)); + + if (installationId) { + installationOctokitCache.delete(installationId); + } + let errorMsg = 'forbidden'; if ((err as any).status === 404) { diff --git a/apps/service/src/utils/ttlCache.ts b/apps/service/src/utils/ttlCache.ts index 0e4fd04..137e682 100644 --- a/apps/service/src/utils/ttlCache.ts +++ b/apps/service/src/utils/ttlCache.ts @@ -40,6 +40,16 @@ export class TtlCache { this.entries.delete(key); } + /** + * Deletes the entry only if it still holds `value`, + * so a cleanup of an old (expired) entry won't delete a newer entry stored under the same key. + */ + deleteIfValue(key: K, value: V): void { + if (this.entries.get(key)?.value === value) { + this.entries.delete(key); + } + } + clear(): void { this.entries.clear(); } diff --git a/apps/service/tests/commitRecordUtils.ts b/apps/service/tests/commitRecordUtils.ts index a4ffe41..0491050 100644 --- a/apps/service/tests/commitRecordUtils.ts +++ b/apps/service/tests/commitRecordUtils.ts @@ -1,13 +1,17 @@ import { createCommitRecord as createCommitRecordInDB } from '@/framework/mongo/commitRecords'; /** - * Creates a commit record and waits until the next millisecond, + * Creates a commit record and waits until the clock moves past its `creationDate`, * so records created one after another always have a distinct `creationDate` (tests rely on the order). + * A fixed sleep is not enough, timers can fire earlier than requested. */ export const createCommitRecord: typeof createCommitRecordInDB = async (...args) => { const record = await createCommitRecordInDB(...args); + const creationTime = new Date(record.creationDate).getTime(); - await new Promise((resolve) => setTimeout(resolve, 2)); + while (Date.now() <= creationTime) { + await new Promise((resolve) => setTimeout(resolve, 1)); + } return record; }; From 3f1ba05b4c93bae660beb711ae0fddbf08aefeac Mon Sep 17 00:00:00 2001 From: LironEr Date: Fri, 9 Oct 2026 08:16:58 +0200 Subject: [PATCH 3/3] fix serverless build --- apps/service/project.json | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/apps/service/project.json b/apps/service/project.json index 40cda2d..9038cfe 100644 --- a/apps/service/project.json +++ b/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",