Skip to content

fix(router): eliminate N+1 DB/API calls in GitHubRouterAdapter #989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zbigniewsobiecki merged 1 commit into from
Mar 23, 2026
Merged

fix(router): eliminate N+1 DB/API calls in GitHubRouterAdapter #989

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions src/github/personas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,32 @@ export async function getPersonaToken(projectId: string, agentType: string): Pro
// Identity Resolution
// ============================================================================

const PERSONA_CACHE_TTL_MS = 60_000; // 60 seconds — matches the Trello/JIRA BotIdentityCache TTL

interface CacheEntry {
value: PersonaIdentities;
expiresAt: number;
}

// Per-project TTL cache for persona identities.
// Unlike BotIdentityCache, errors are re-thrown so callers retain error semantics.
const personaIdentityCache = new Map<string, CacheEntry>();

/**
* Resolve both persona GitHub usernames for a project.
* Always queries the database and GitHub API for fresh data.
* Results are cached per-project with a 60s TTL to avoid redundant DB + API calls
* on rapid successive webhooks (e.g. multiple events within the same request batch).
* Errors are re-thrown so callers can handle credential failures.
*/
export async function resolvePersonaIdentities(projectId: string): Promise<PersonaIdentities> {
const implementerToken = await getIntegrationCredential(projectId, 'scm', 'implementer_token');
const reviewerToken = await getIntegrationCredential(projectId, 'scm', 'reviewer_token');
const cached = personaIdentityCache.get(projectId);
if (cached && Date.now() < cached.expiresAt) return cached.value;

// Parallelize credential lookups to halve round-trip latency
const [implementerToken, reviewerToken] = await Promise.all([
getIntegrationCredential(projectId, 'scm', 'implementer_token'),
getIntegrationCredential(projectId, 'scm', 'reviewer_token'),
]);

const [implementerLogin, reviewerLogin] = await Promise.all([
getGitHubUserForToken(implementerToken),
Expand Down Expand Up @@ -88,9 +107,18 @@ export async function resolvePersonaIdentities(projectId: string): Promise<Perso
reviewer: reviewerLogin,
});

personaIdentityCache.set(projectId, {
value: identities,
expiresAt: Date.now() + PERSONA_CACHE_TTL_MS,
});
return identities;
}

/** @internal Visible for testing only */
export function _resetPersonaIdentityCache(): void {
personaIdentityCache.clear();
}

// ============================================================================
// Bot Detection
// ============================================================================
Expand Down
81 changes: 66 additions & 15 deletions src/router/adapters/github.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { withPMCredentials, withPMProvider } from '../../pm/context.js';
import { pmRegistry } from '../../pm/registry.js';
import { captureException } from '../../sentry.js';
import type { TriggerRegistry } from '../../triggers/registry.js';
import type { TriggerContext, TriggerResult } from '../../types/index.js';
import type { ProjectConfig, TriggerContext, TriggerResult } from '../../types/index.js';
import { logger } from '../../utils/logging.js';
import { buildWorkItemRunsLink, getDashboardUrl } from '../../utils/runLink.js';
import { extractGitHubContext, generateAckMessage } from '../ackMessageGenerator.js';
Expand Down Expand Up @@ -70,6 +70,7 @@ async function postPMAck(
* Returns an AckResult, or undefined if not applicable.
*
* @param messageOverride — when provided, skips internal message generation and posts this text instead.
* @param project — Optional pre-resolved project config to skip findProjectByRepo lookup.
*/
async function postGitHubPRAck(
repoFullName: string,
Expand All @@ -78,8 +79,9 @@ async function postGitHubPRAck(
agentType: string,
projectId: string,
messageOverride?: string,
project?: ProjectConfig,
): Promise<AckResult | undefined> {
const resolved = await resolveGitHubTokenForAckByAgent(repoFullName, agentType);
const resolved = await resolveGitHubTokenForAckByAgent(repoFullName, agentType, project);
if (!resolved) return undefined;

const context = extractGitHubContext(payload, eventType);
Expand Down Expand Up @@ -117,6 +119,45 @@ function getGitHubDeliveryId(payload: Record<string, unknown>): string | undefin
export class GitHubRouterAdapter implements RouterPlatformAdapter {
readonly type = 'github' as const;

// -------------------------------------------------------------------------
// Per-request caches
//
// GitHubRouterAdapter is instantiated once per webhook request, so instance
// fields are naturally request-scoped. Caching here avoids redundant DB and
// GitHub API calls across the pipeline steps (isSelfAuthored, sendReaction,
// dispatchWithCredentials, postAck) that all need the same project / persona
// data for a single event.
// -------------------------------------------------------------------------

private cachedProject: ProjectConfig | null | undefined = undefined; // undefined = not yet fetched
private cachedPersonas: PersonaIdentities | undefined;
private cachedFullProject: ProjectConfig | undefined;

/**
* Resolve the ProjectConfig for the given repo, caching the result for the
* lifetime of this request adapter instance.
*/
private async findProjectCached(repoFullName: string): Promise<ProjectConfig | null> {
if (this.cachedProject !== undefined) return this.cachedProject;
const project = (await findProjectByRepo(repoFullName)) ?? null;
this.cachedProject = project;
return project;
}

/**
* Resolve persona identities for the given project, caching the result for
* the lifetime of this request adapter instance.
*/
private async resolvePersonaCached(projectId: string): Promise<PersonaIdentities | undefined> {
if (this.cachedPersonas !== undefined) return this.cachedPersonas;
try {
this.cachedPersonas = await resolvePersonaIdentities(projectId);
} catch {
// Resolution may fail — leave cachedPersonas undefined so callers can decide
}
return this.cachedPersonas;
}

async parseWebhook(payload: unknown): Promise<GitHubParsedEvent | null> {
const p = payload as Record<string, unknown>;
const repo = p.repository as Record<string, unknown> | undefined;
Expand Down Expand Up @@ -161,9 +202,10 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter {
const login = commentUser?.login as string | undefined;
if (!login) return false;
try {
const project = await findProjectByRepo((event as GitHubParsedEvent).repoFullName);
const project = await this.findProjectCached((event as GitHubParsedEvent).repoFullName);
if (!project) return false;
const personas = await resolvePersonaIdentities(project.id);
const personas = await this.resolvePersonaCached(project.id);
if (!personas) return false;
return isCascadeBot(login, personas);
} catch {
return false;
Expand All @@ -175,12 +217,16 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter {
const repoFullName = (event as GitHubParsedEvent).repoFullName;
void (async () => {
try {
const project = await findProjectByRepo(repoFullName);
const project = await this.findProjectCached(repoFullName);
if (!project) {
logger.warn('No project found for repo, skipping GitHub reaction', { repoFullName });
return;
}
const personaIdentities = await resolvePersonaIdentities(project.id);
const personaIdentities = await this.resolvePersonaCached(project.id);
if (!personaIdentities) {
logger.warn('No persona identities resolved, skipping GitHub reaction', { repoFullName });
return;
}
await sendAcknowledgeReaction('github', repoFullName, payload, personaIdentities, project);
} catch (err) {
logger.warn('GitHub reaction error', { error: String(err), repoFullName });
Expand Down Expand Up @@ -215,12 +261,11 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter {
return null;
}

let personaIdentities: PersonaIdentities | undefined;
try {
personaIdentities = await resolvePersonaIdentities(fullProject.id);
} catch {
// Persona resolution may fail — proceed without
}
// Cache fullProject for reuse in postAck (Step 4)
this.cachedFullProject = fullProject;

// Reuse cached personas to avoid a second resolvePersonaIdentities call (Step 4)
const personaIdentities = await this.resolvePersonaCached(fullProject.id);

const githubToken = await getProjectGitHubToken(fullProject);
const pmProvider = pmRegistry.createProvider(fullProject);
Expand Down Expand Up @@ -251,9 +296,13 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter {
triggerResult?: TriggerResult,
): Promise<AckResult | undefined> {
try {
// Load full project config to check run link settings
const config = await loadProjectConfig();
const fullProject = config.fullProjects.find((fp) => fp.id === project.id);
// Reuse the fullProject already resolved in dispatchWithCredentials when available
// (Step 4: avoids a second loadProjectConfig() call per webhook pipeline)
let fullProject = this.cachedFullProject;
if (!fullProject) {
const config = await loadProjectConfig();
fullProject = config.fullProjects.find((fp) => fp.id === project.id);
}
const runLinksEnabled = fullProject?.runLinksEnabled ?? false;

// Helper to append run link footer to a message when enabled
Expand Down Expand Up @@ -296,13 +345,15 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter {
}
}

// Pass cached project to avoid a redundant findProjectByRepo() in the token resolver
return postGitHubPRAck(
(event as GitHubParsedEvent).repoFullName,
event.eventType,
payload,
agentType,
project.id,
githubAckMessage,
fullProject,
);
} catch (err) {
logger.warn('GitHub ack comment failed (non-fatal)', { error: String(err) });
Expand Down
17 changes: 11 additions & 6 deletions src/router/github-token-resolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,21 +43,26 @@ export async function resolveGitHubTokenForAck(
* is posted by the same persona that will run the agent (and can
* later update it via ProgressMonitor). All other agents use the
* implementer token.
*
* @param project — Optional pre-resolved project config. When provided, the
* `findProjectByRepo()` DB lookup is skipped entirely (eliminating a
* redundant query in callers that have already resolved the project).
*/
export async function resolveGitHubTokenForAckByAgent(
repoFullName: string,
agentType: string,
project?: ProjectConfig,
): Promise<ResolvedGitHubToken | null> {
const project = await findProjectByRepo(repoFullName);
if (!project) return null;
const resolvedProject = project ?? (await findProjectByRepo(repoFullName));
if (!resolvedProject) return null;

try {
if (agentType === 'review') {
const token = await getIntegrationCredential(project.id, 'scm', 'reviewer_token');
return { token, project };
const token = await getIntegrationCredential(resolvedProject.id, 'scm', 'reviewer_token');
return { token, project: resolvedProject };
}
const token = await getProjectGitHubToken(project);
return { token, project };
const token = await getProjectGitHubToken(resolvedProject);
return { token, project: resolvedProject };
} catch {
logger.warn('[Ack] Missing GitHub token for repo:', repoFullName);
return null;
Expand Down
116 changes: 110 additions & 6 deletions tests/unit/github/personas.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ vi.mock('../../../src/utils/logging.js', () => ({
import { getIntegrationCredential } from '../../../src/config/provider.js';
import { getGitHubUserForToken } from '../../../src/github/client.js';
import {
_resetPersonaIdentityCache,
getPersonaForAgentType,
getPersonaForLogin,
getPersonaToken,
Expand All @@ -30,6 +31,15 @@ import {
import type { PersonaIdentities } from '../../../src/github/personas.js';

describe('personas', () => {
beforeEach(() => {
// Reset the module-level TTL cache so tests don't bleed into each other
_resetPersonaIdentityCache();
});

afterEach(() => {
_resetPersonaIdentityCache();
});

// ========================================================================
// getPersonaForAgentType
// ========================================================================
Expand Down Expand Up @@ -130,23 +140,21 @@ describe('personas', () => {
});
});

it('fetches fresh data on each call', async () => {
it('returns cached result on second call for same project', async () => {
vi.mocked(getIntegrationCredential)
.mockResolvedValueOnce('ghp_impl')
.mockResolvedValueOnce('ghp_review')
.mockResolvedValueOnce('ghp_impl')
.mockResolvedValueOnce('ghp_review');
vi.mocked(getGitHubUserForToken)
.mockResolvedValueOnce('cascade-impl-bot')
.mockResolvedValueOnce('cascade-review-bot')
.mockResolvedValueOnce('cascade-impl-bot')
.mockResolvedValueOnce('cascade-review-bot');

const first = await resolvePersonaIdentities('project1');
const second = await resolvePersonaIdentities('project1');

expect(first).toEqual(second);
expect(getIntegrationCredential).toHaveBeenCalledTimes(4);
// Only 2 credential lookups total (not 4) because the second call is served from cache
expect(getIntegrationCredential).toHaveBeenCalledTimes(2);
expect(getGitHubUserForToken).toHaveBeenCalledTimes(2);
});

it('resolves separately for different projects', async () => {
Expand Down Expand Up @@ -217,6 +225,102 @@ describe('personas', () => {
'Failed to resolve GitHub identity for reviewer token',
);
});

it('fetches fresh data after TTL expires', async () => {
vi.useFakeTimers();
const now = Date.now();

try {
// First call — populate the cache
vi.mocked(getIntegrationCredential)
.mockResolvedValueOnce('ghp_impl')
.mockResolvedValueOnce('ghp_review');
vi.mocked(getGitHubUserForToken)
.mockResolvedValueOnce('cascade-impl-bot')
.mockResolvedValueOnce('cascade-review-bot');

await resolvePersonaIdentities('project1');
expect(getIntegrationCredential).toHaveBeenCalledTimes(2);

// Advance time past the 60s TTL
vi.setSystemTime(now + 61_000);

// Second call after TTL — should re-fetch
vi.mocked(getIntegrationCredential)
.mockResolvedValueOnce('ghp_impl_new')
.mockResolvedValueOnce('ghp_review_new');
vi.mocked(getGitHubUserForToken)
.mockResolvedValueOnce('cascade-impl-bot-new')
.mockResolvedValueOnce('cascade-review-bot-new');

const result = await resolvePersonaIdentities('project1');
// 4 total calls (2 original + 2 re-fetch)
expect(getIntegrationCredential).toHaveBeenCalledTimes(4);
expect(result.implementer).toBe('cascade-impl-bot-new');
} finally {
vi.useRealTimers();
}
});

it('isolates cache per project ID', async () => {
// Set up project1 in cache
vi.mocked(getIntegrationCredential)
.mockResolvedValueOnce('ghp_impl_1')
.mockResolvedValueOnce('ghp_review_1');
vi.mocked(getGitHubUserForToken)
.mockResolvedValueOnce('bot-impl-1')
.mockResolvedValueOnce('bot-review-1');

const result1a = await resolvePersonaIdentities('project1');

// Populate project2
vi.mocked(getIntegrationCredential)
.mockResolvedValueOnce('ghp_impl_2')
.mockResolvedValueOnce('ghp_review_2');
vi.mocked(getGitHubUserForToken)
.mockResolvedValueOnce('bot-impl-2')
.mockResolvedValueOnce('bot-review-2');

const result2 = await resolvePersonaIdentities('project2');

// project1 second call — served from cache (no additional mock calls needed)
const result1b = await resolvePersonaIdentities('project1');

// project1 identity is preserved across project2 population
expect(result1a.implementer).toBe('bot-impl-1');
expect(result1b.implementer).toBe('bot-impl-1');
expect(result1b).toStrictEqual(result1a);
// project2 has its own separate cached identity
expect(result2.implementer).toBe('bot-impl-2');
// Only 4 credential lookups total (project1 once + project2 once)
expect(getIntegrationCredential).toHaveBeenCalledTimes(4);
});

it('re-fetches after cache is manually reset', async () => {
vi.mocked(getIntegrationCredential)
.mockResolvedValueOnce('ghp_impl')
.mockResolvedValueOnce('ghp_review');
vi.mocked(getGitHubUserForToken)
.mockResolvedValueOnce('cascade-impl-bot')
.mockResolvedValueOnce('cascade-review-bot');

await resolvePersonaIdentities('project1');
expect(getIntegrationCredential).toHaveBeenCalledTimes(2);

// Reset the cache
_resetPersonaIdentityCache();

vi.mocked(getIntegrationCredential)
.mockResolvedValueOnce('ghp_impl')
.mockResolvedValueOnce('ghp_review');
vi.mocked(getGitHubUserForToken)
.mockResolvedValueOnce('cascade-impl-bot')
.mockResolvedValueOnce('cascade-review-bot');

await resolvePersonaIdentities('project1');
// 4 total calls (2 original + 2 after reset)
expect(getIntegrationCredential).toHaveBeenCalledTimes(4);
});
});

// ========================================================================
Expand Down
Loading
Loading