Cache GitHub installation tokens in memory#4912
Conversation
vansh-commits
requested review from
Flo4604,
MichaelUnkey,
chronark,
imeyer,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
@vansh-commits is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds an in-process per-installation cache for GitHub installation tokens, wires it into Client initialization, and changes GetInstallationToken to use a cache-first SWR loader that generates a JWT and requests an installation token from GitHub on cache miss. Also removes DownloadRepoTarball and adjusts return types. Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant Client as GitHubClient
participant Cache as InstallationTokenCache
participant GitHub as GitHub API
Caller->>Client: GetInstallationToken(installationID)
Client->>Cache: SWR Lookup/Load(installationID)
alt Cache Hit (fresh)
Cache-->>Client: InstallationToken
Client-->>Caller: InstallationToken
else Cache Miss or Stale
Client->>Client: Generate JWT
Client->>GitHub: POST /app/installations/:id/access_tokens (Bearer <JWT>)
GitHub-->>Client: 201 Created + token payload
Client->>Cache: Store(installationID, InstallationToken)
Client-->>Caller: InstallationToken
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR introduces in-memory caching for GitHub App installation tokens to reduce unnecessary GitHub API calls. Installation tokens are valid for approximately one hour but were previously fetched on every request. The implementation adds a global cache that reuses tokens until expiry, reducing API calls while maintaining functional correctness.
Changes:
- Added global cache initialization in
NewClientwith 55-minute freshness/staleness windows - Modified
GetInstallationTokento check cache before making GitHub API calls - Added cache write operation after successful token retrieval from GitHub
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@svc/ctrl/worker/github/client.go`:
- Around line 23-25: The global installationTokenCache is initialized with a
nil-check in NewClient which races when NewClient is called concurrently; add a
package-level sync.Once (e.g., installationTokenCacheOnce) and move the cache
creation into installationTokenCacheOnce.Do(...) so installationTokenCache is
created exactly once; update NewClient to call that Once.Do before using
installationTokenCache (and apply the same pattern to the related initialization
at lines 57-70), referencing the installationTokenCache and NewClient symbols.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@svc/ctrl/worker/github/client.go`:
- Around line 124-137: The cached-token branch for installationTokenCache
currently treats any non-Miss hit as valid; update the logic in the block that
calls installationTokenCache.Get(…) to verify the cached.ExpiresAt is still in
the future before returning it: after retrieving cached for installationID,
check that cached.ExpiresAt.After(time.Now()) (or equivalent) and only then log
via c.logger.Debug and return a copy of token; if expired, treat it as a cache
miss (do not return) so the code will fallthrough to refresh the token. Ensure
you still return a copy of the token (token := cached; return &token, nil) when
valid.
🧹 Nitpick comments (1)
svc/ctrl/worker/github/client.go (1)
29-29: Rename toerrInstallationTokenCacheper Go conventions.Static analysis flags this: sentinel error names should follow
errXxxformat.Proposed fix
var ( installationTokenCache cache.Cache[int64, InstallationToken] installationTokenCacheOnce sync.Once - installationTokenCacheErr error + errInstallationTokenCache error )Update usage at lines 63 and 75:
- installationTokenCache, installationTokenCacheErr = cache.New( + installationTokenCache, errInstallationTokenCache = cache.New( - if installationTokenCacheErr != nil { - return nil, installationTokenCacheErr + if errInstallationTokenCache != nil { + return nil, errInstallationTokenCache
|
Thanks for the review! I’ve addressed the requested changes:
Let me know if you’d like any further adjustments. |
|
also please run make bazel so the CI is happy |
|
Done — ran make bazel and pushed the changes |
The linter isn't happy please run make build locally it should show you any errors from bazel linting |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@svc/ctrl/worker/github/client.go`:
- Around line 110-173: GetInstallationToken currently returns a cached
InstallationToken from tokenCache.SWR without validating
InstallationToken.ExpiresAt, which can return expired tokens; update
GetInstallationToken to check the returned value's ExpiresAt against time.Now()
plus a small buffer (e.g., 5 minutes) and if the token is expired or within the
buffer, invalidate or bypass the cache and fetch a fresh token (e.g., call
tokenCache.Delete/Invalidate for installationID or re-run the SWR fetcher to
force refresh) rather than returning the stale value; reference the
GetInstallationToken function, tokenCache.SWR call, InstallationToken.ExpiresAt
field, and generateJWT flow when implementing the validation and refresh logic.
|
Welcome! Always happy to Contribute... |
3 participants
What does this PR do?
This PR introduces an in-memory cache for GitHub App installation tokens to avoid
fetching a new token on every deployment-related call.
GitHub installation tokens are valid for ~1 hour, but were previously fetched
on each request. This change reuses tokens until expiry, reducing unnecessary
GitHub API calls while preserving existing behavior.
Fixes #4901
Type of change
How should this be tested?
Checklist
Required
pnpm build(not applicable)pnpm fmt(not applicable)make fmton/godirectoryconsole.logs(not applicable)git pull origin main