Skip to content

Conversation

@dblinkhorn
Copy link

This PR introduces a singleton cache for Octokit clients, authentication objects, and GitHub App configuration, so we can avoid making redundant SSM lookups and client creation.

This PR represents the first step in what will be a broader effort to modularize GitHub interactions. Future PRs will build on this one by adding runner management, job operations, advanced caching for runner and job metadata, etc.

@dblinkhorn dblinkhorn requested review from a team as code owners November 18, 2025 22:47
@dblinkhorn dblinkhorn changed the title feat: Add centralized GitHub service module with caching for clients, auth, and config feat(control-plane): Centralize GitHub client/auth/config caching Nov 18, 2025
@npalm
Copy link
Member

npalm commented Nov 19, 2025

@dblinkhorn thx, not digged in the PR it self. But please can you run yarn format to fix the prettier errors in CI.

@dblinkhorn dblinkhorn force-pushed the dblinkhorn/github-service-module branch 3 times, most recently from 14bd0be to 5f34e10 Compare November 20, 2025 22:43
@npalm npalm requested a review from Copilot November 21, 2025 21:30
Copilot finished reviewing on behalf of npalm November 21, 2025 21:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a singleton caching layer for GitHub API clients, authentication objects, and app configuration to optimize performance by avoiding redundant SSM parameter lookups and client instantiations. The caching implementation uses in-memory Maps for storing Octokit clients, app/installation authentication objects, and GitHub App configuration.

Key changes:

  • Created a new cache module (github/cache.ts) with functions to cache and retrieve Octokit clients, authentication objects, and app configs
  • Refactored existing authentication functions in auth.ts to utilize the new caching layer
  • Added cache reset functionality in test setup to ensure test isolation

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
lambdas/functions/control-plane/src/github/types.ts Defines GithubAppConfig interface for GitHub App configuration with appId, privateKey, and optional installationId
lambdas/functions/control-plane/src/github/index.ts Exports cache and types modules to make them available to other parts of the application
lambdas/functions/control-plane/src/github/cache.ts Implements singleton cache using Map structures with cache key generators and getter functions for clients, auth objects, and configs
lambdas/functions/control-plane/src/github/auth.ts Integrates caching into existing authentication and client creation functions to avoid redundant SSM lookups
lambdas/functions/control-plane/src/github/auth.test.ts Adds cache reset call in test setup and updates import statement to use type imports
lambdas/functions/control-plane/src/local.ts Removes an eslint-disable comment that is no longer needed

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +46 to +27
export async function getInstallationAuthObject(
key: string,
creator: () => Promise<InstallationAccessTokenAuthentication>,
): Promise<InstallationAccessTokenAuthentication> {
if (installationAuthObjects.has(key)) {
return installationAuthObjects.get(key)!;
}

const authObj = await creator();
installationAuthObjects.set(key, authObj);
return authObj;
}
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

Installation access tokens have expiration times (typically 1 hour) and should not be cached indefinitely. The current implementation caches InstallationAccessTokenAuthentication objects without considering token expiration, which could lead to using expired tokens.

Consider either:

  1. Not caching installation auth objects at all, or
  2. Implementing TTL-based caching that respects the token's expiresAt field from the authentication object

Copilot uses AI. Check for mistakes.
Copy link
Member

@npalm npalm Nov 21, 2025

Choose a reason for hiding this comment

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

@copilot Lambda max TTL is 15 minutes

Comment on lines 10 to 12
export function createTokenCacheKey(token: string, ghesApiUrl: string = '') {
return `octokit-${token}-${ghesApiUrl}`;
}
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

Storing tokens directly in cache keys is a security concern as it could expose sensitive credentials in memory dumps or logs. The full token value should not be part of the cache key.

Consider using a hash of the token instead:

import { createHash } from 'crypto';

export function createTokenCacheKey(token: string, ghesApiUrl: string = '') {
  const tokenHash = createHash('sha256').update(token).digest('hex').substring(0, 16);
  return `octokit-${tokenHash}-${ghesApiUrl}`;
}

Copilot uses AI. Check for mistakes.
export interface GithubAppConfig {
appId: number;
privateKey: string;
installationId?: number;
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The installationId field is defined in the GithubAppConfig interface but is never populated or used. It should either be:

  1. Removed from the interface if not needed, or
  2. Populated in the createAuth function (line 109-112) if it's intended to be part of the config

Currently, the installationId is handled separately in createAuth (line 119) rather than being part of the cached config.

Suggested change
installationId?: number;

Copilot uses AI. Check for mistakes.
Comment on lines 10 to 67
export function createTokenCacheKey(token: string, ghesApiUrl: string = '') {
return `octokit-${token}-${ghesApiUrl}`;
}

export function createAuthCacheKey(type: 'app' | 'installation', installationId?: number, ghesApiUrl: string = '') {
const id = installationId || 'none';
return `${type}-auth-${id}-${ghesApiUrl}`;
}

export function createAuthConfigCacheKey(ghesApiUrl: string = '') {
return `auth-config-${ghesApiUrl}`;
}

export async function getClient(key: string, creator: () => Promise<Octokit>): Promise<Octokit> {
if (clients.has(key)) {
return clients.get(key)!;
}

const client = await creator();
clients.set(key, client);
return client;
}

export async function getAppAuthObject(
key: string,
creator: () => Promise<AppAuthentication>,
): Promise<AppAuthentication> {
if (appAuthObjects.has(key)) {
return appAuthObjects.get(key)!;
}

const authObj = await creator();
appAuthObjects.set(key, authObj);
return authObj;
}

export async function getInstallationAuthObject(
key: string,
creator: () => Promise<InstallationAccessTokenAuthentication>,
): Promise<InstallationAccessTokenAuthentication> {
if (installationAuthObjects.has(key)) {
return installationAuthObjects.get(key)!;
}

const authObj = await creator();
installationAuthObjects.set(key, authObj);
return authObj;
}

export async function getAuthConfig(key: string, creator: () => Promise<GithubAppConfig>) {
if (authConfigs.has(key)) {
return authConfigs.get(key)!;
}

const config = await creator();
authConfigs.set(key, config);
return config;
}
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The cache functions and key generation functions lack JSDoc documentation. Adding documentation would help other developers understand:

  • The caching strategy and behavior
  • When cached values are returned vs when the creator function is called
  • The cache key format and what parameters affect it
  • The purpose of the reset() function

Example:

/**
 * Creates a cache key for Octokit clients based on token and API URL.
 * @param token - The authentication token
 * @param ghesApiUrl - Optional GitHub Enterprise Server API URL
 * @returns A unique cache key for this token/URL combination
 */
export function createTokenCacheKey(token: string, ghesApiUrl: string = '') {
  return `octokit-${token}-${ghesApiUrl}`;
}

Copilot uses AI. Check for mistakes.
}

export function createAuthCacheKey(type: 'app' | 'installation', installationId?: number, ghesApiUrl: string = '') {
const id = installationId || 'none';
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

The cache key generation uses installationId || 'none' which treats falsy values (0, null, undefined) the same way. While 0 is likely not a valid GitHub installation ID, this could lead to cache key collisions for different falsy values.

Consider using nullish coalescing instead for more precise handling:

const id = installationId ?? 'none';

This will only treat null and undefined as 'none', while preserving 0 if it were ever a valid value.

Suggested change
const id = installationId || 'none';
const id = installationId ?? 'none';

Copilot uses AI. Check for mistakes.
)
.toString()
.replace('/[\\n]/g', String.fromCharCode(10)),
.replace('/[\\n]/g', String.fromCharCode(10));
Copy link

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

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

String argument /[\\n]/g looks like a regular expression, but it will be interpreted as a string.

Suggested change
.replace('/[\\n]/g', String.fromCharCode(10));
.replace(/\\n/g, String.fromCharCode(10));

Copilot uses AI. Check for mistakes.
@dblinkhorn dblinkhorn force-pushed the dblinkhorn/github-service-module branch from f8d8165 to 7e9cae3 Compare November 22, 2025 01:48
@dblinkhorn dblinkhorn force-pushed the dblinkhorn/github-service-module branch from 7e9cae3 to 8a5d1d0 Compare November 22, 2025 01:50
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.

2 participants