Skip to content

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

Merged
zbigniewsobiecki merged 1 commit intodevfrom
fix/github-adapter-n-plus-1-caching
Mar 23, 2026
Merged

fix(router): eliminate N+1 DB/API calls in GitHubRouterAdapter#989
zbigniewsobiecki merged 1 commit intodevfrom
fix/github-adapter-n-plus-1-caching

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Mar 23, 2026

Summary

Addresses a Sentry-reported N+1 query issue in the GitHub webhook processing pipeline. Each webhook was triggering 3x resolvePersonaIdentities() calls (2 DB + 2 GitHub API each), 2-3x findProjectByRepo() calls, and 3x loadProjectConfig() calls — all for the same request.

Expected impact: Reduces DB queries per GitHub webhook from ~10–15 to ~3–4, and GitHub API calls from 4–6 to 2 (globally cached with 60s TTL).

  • Per-request instance caching in GitHubRouterAdapterfindProjectByRepo() and resolvePersonaIdentities() results are cached in private instance fields. The adapter is instantiated once per webhook request so this is naturally request-scoped, preventing redundant calls across isSelfAuthored, sendReaction, dispatchWithCredentials, and postAck.
  • cachedFullProject field in the adapter — populated by dispatchWithCredentials, reused in postAck to skip a second loadProjectConfig() call.
  • 60s TTL cache added to resolvePersonaIdentities() in src/github/personas.ts — prevents redundant DB + GitHub API lookups across rapid successive webhooks for the same project. Errors are re-thrown (unlike BotIdentityCache which silences them) to preserve caller error semantics.
  • Optional project parameter on resolveGitHubTokenForAckByAgent() — when the adapter passes its cached project config, the findProjectByRepo() DB lookup in the token resolver is skipped entirely.
  • Parallelized credential lookups in resolvePersonaIdentities() — implementer and reviewer token lookups now use Promise.all instead of sequential awaits, halving round-trip latency.

Test plan

  • All 6300 existing unit tests pass
  • New adapter caching tests verify findProjectByRepo and resolvePersonaIdentities called once per pipeline
  • New token resolver tests verify findProjectByRepo skipped when pre-resolved project provided
  • New persona TTL cache tests: hit/miss, expiry after 60s, per-project isolation, manual reset
  • Lint and type checking pass

Card

https://trello.com/c/ZSiSPvKM/515-address-sentry-reported-issue

🤖 Generated with Claude Code

🕵️ claude-code · claude-sonnet-4-6 · run details

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 23, 2026

Codecov Report

❌ Patch coverage is 87.93103% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/router/adapters/github.ts 80.55% 6 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

LGTM — well-designed performance optimization that eliminates N+1 DB/API calls in the GitHub webhook pipeline through three complementary caching layers: per-request instance caching, module-level TTL cache, and pass-through of pre-resolved objects.

Verification

Correctness:

  • The adapter is confirmed to be instantiated once per webhook request (new GitHubRouterAdapter() in src/router/index.ts:108), so instance fields are properly request-scoped.
  • findProjectCached and resolvePersonaCached correctly use sentinel values (undefined for "not yet fetched" vs null for "fetched but not found") to distinguish cache states.
  • Failed persona resolutions are NOT cached (the catch block leaves cachedPersonas as undefined), allowing retries within the same request — this is correct.
  • Error semantics are preserved: resolvePersonaCached swallows errors (matching the old try/catch in dispatchWithCredentials), while the module-level resolvePersonaIdentities cache re-throws errors (matching its documented behavior and differing intentionally from BotIdentityCache).

Architecture:

  • The 60s TTL cache in personas.ts follows the same pattern as the existing BotIdentityCache class (same TTL, same Map<string, {value, expiresAt}> structure). Consistent with established codebase patterns.
  • The optional project parameter on resolveGitHubTokenForAckByAgent is a clean backward-compatible extension.
  • The Promise.all parallelization of credential lookups is a straightforward win.

Testing:

  • New tests cover all caching behaviors: hit/miss, TTL expiry, per-project isolation, manual reset, and cross-method caching within the adapter.
  • All CI checks pass.

🕵️ claude-code · claude-opus-4-6 · run details

@zbigniewsobiecki zbigniewsobiecki merged commit f030c8f into dev Mar 23, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants