Replace custom retry() with @octokit/plugin-retry#947
Conversation
…etAccessPolicy Co-authored-by: qoomon <3963394+qoomon@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves resilience against transient GitHub API connection closures by fixing the shared retry() helper and applying consistent retry behavior to GitHub API calls involved in access-policy reads and installation token creation.
Changes:
- Fix
retry()to correctly rethrow the final error and toawaitasynconRetry/onErrorcallbacks. - Add Jest unit tests covering
retry()success, failure, and callback short-circuiting behavior. - Add retry wrappers around access policy file reads and installation access token creation in
access-token-manager.ts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
server/test/common/common-utils.test.ts |
Adds unit tests validating retry boundaries and async callback behavior. |
server/src/common/common-utils.ts |
Fixes retry() loop boundary and awaits async onRetry/onError callbacks; avoids sleeping after final attempt. |
server/src/access-token-manager.ts |
Adds retry around access policy fetch and installation access token creation to mitigate transient GitHub API connection issues. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
server/src/access-token-manager.ts
Outdated
| return retry( | ||
| () => getRepositoryFileContent(client, {owner, repo, path, maxSize: ACCESS_POLICY_MAX_SIZE}), | ||
| {retries: 3, delay: 1000}, | ||
| ).catch((error) => { | ||
| logger.error({owner, repo, path, error: String(error)}, 'Failed to get access policy file content'); | ||
| return null; | ||
| } | ||
| }); |
There was a problem hiding this comment.
getRepositoryFileContent() already returns null for 404 and throws for non-404 errors. This .catch(() => null) converts any remaining non-404 error (after retries) into null, which later becomes Access policy not found and hides real GitHub/API failures. Consider only mapping "not found" to null and propagating other errors (or tracking an error and throwing it if all paths fail due to non-404 errors).
server/src/access-token-manager.ts
Outdated
| }); | ||
| }); | ||
| if (!policyValue) { | ||
| throw new GithubAccessPolicyError(`Access policy not found`); |
There was a problem hiding this comment.
if (!policyValue) treats an empty access policy file ('') the same as null/missing, which will throw Access policy not found instead of Invalid access policy. Prefer an explicit null check here (and ensure the findFirstNotNull callback doesn’t drop empty strings due to truthiness).
server/src/access-token-manager.ts
Outdated
| return retry( | ||
| () => client.rest.apps.createInstallationAccessToken({ | ||
| installation_id: installation.id, | ||
| // BE AWARE that an empty object will result in a token with all app installation permissions | ||
| permissions: ensureHasEntries(mapObjectEntries(permissions, ([scope, permission]) => [ |
There was a problem hiding this comment.
Wrapping createInstallationAccessToken in retry() without an onError predicate will retry non-transient failures (e.g., 401/403/422) as well, adding ~2s latency and extra GitHub API calls before returning the same error. Consider restricting retries to clearly transient conditions (connection resets/"line closed", 5xx, etc.) and short-circuiting on non-retryable status codes.
Co-authored-by: qoomon <3963394+qoomon@users.noreply.github.com>
Co-authored-by: qoomon <3963394+qoomon@users.noreply.github.com>
@octokit/plugin-retryas a production dependencyretryPluginto the Octokit plugin chain@octokit/plugin-retrywith a localoctokitRetryPluginthat readsrequest.{ retries, retryAfter, doNotRetry }from per-request options via Octokit'shook.errorAPIOCTOKIT_RETRY_OPTIONSshared constantrequest: { retries, retryAfter, doNotRetry }with semantically correct per-calldoNotRetrylists:getUserInstallation→doNotRetry: [Status.NOT_FOUND]createInstallationAccessToken→doNotRetry: [Status.UNPROCESSABLE_ENTITY]getContent→doNotRetry: [Status.NOT_FOUND]@octokit/plugin-retry💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.