Repository navigation
fix: persist manual GitHub retry failures - #30
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5cdf83455
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Math.max(1, args.maxMinutes), | ||
| Math.max(1, args.baseMinutes) * 2 ** Math.max(0, failureCount - 1), | ||
| ); | ||
| const fallbackRetryAt = new Date(args.now.getTime() + exponentialMinutes * 60_000); |
There was a problem hiding this comment.
When a workspace uses an allowed 1-minute githubSyncBackoffMinutes and a manual/MCP refresh consumes a long githubRequestTimeoutSeconds (up to 60s, or longer across sequential PR/check calls), args.now is the timestamp captured before fetchGitHubSnapshot runs, so fallbackRetryAt can already be at or before the time this failure is persisted. Once the manual cooldown has also elapsed, the next refresh can claim immediately instead of honoring the intended timeout/permission backoff circuit; compute the fallback from the failure/persist time, or clamp it to a future time.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71794d586e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| provider: GITHUB_PROVIDER, | ||
| connectionMappingId: args.connectionMappingId, | ||
| }, | ||
| data: { syncRetryAt: retryAt, syncLastError: message }, |
There was a problem hiding this comment.
When a manual refresh hits a mapping-wide error on one PR while another resource on the same mapping already has a later syncRetryAt (for example a GitHub reset header from a previous 429), this unconditional updateMany replaces the later gate with this failure's retry time. A timeout or auth error with only the fallback backoff can therefore shorten an existing longer rate-limit circuit and let the reconciler/manual refreshes poll that mapping early; only extend rows whose retry is null or earlier than the new value.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f303062a53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (error) { | ||
| // Scheduled reconciliation owns its own retry accounting. Manual/MCP | ||
| // refreshes must persist the provider reset/backoff before returning the | ||
| // error so a short collision lease cannot accidentally become the retry | ||
| // policy and the issue panel retains the real diagnostic. | ||
| if (!args.skipCollisionGuard) { | ||
| await persistGitHubManualSyncFailure({ |
There was a problem hiding this comment.
This only persists retry/backoff when fetchGitHubSnapshot throws, but PR checks/status provider failures do not throw: getGitHubPullRequestChecks settles those calls into metadata.checks.partial and upsertExternalResource only updates the current row. In a manual/MCP refresh where the PR request succeeds but checks are rate-limited, 403, or time out, sibling resources on the same mapping still avoid the new mapping-wide circuit and can immediately keep refreshing against the failing provider; scheduled reconciliation has a separate partial-checks path for this case.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2e619c5dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return new GitHubRequestError( | ||
| message, | ||
| rateLimited ? 429 : timedOut ? 408 : 403, |
There was a problem hiding this comment.
When getGitHubPullRequestChecks returns partial: true for a non-auth reason (for example truncated check-suite pagination or a transient 5xx from one checks/status request, both of which leave rateLimited and timedOut false), this creates a synthetic 403. persistGitHubManualSyncFailure() then treats that 403 as a mapping-wide permission failure, so manually refreshing one unusually large or transiently failing PR blocks every sibling GitHub resource on the mapping until backoff expires; only auth/rate-limit/timeout partials should open that circuit.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3ff55ecc4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }, | ||
| data: { | ||
| syncAttemptedAt: args.now, | ||
| syncRetryAt: retryAt, |
There was a problem hiding this comment.
When two manual/MCP refreshes for different resources on the same GitHub mapping overlap, one failure can extend this row via the mapping-wide update below while this refresh is still in flight. This unconditional assignment then overwrites that later provider reset with this call's shorter fallback/retry time, so the resource can become retryable before the mapping-wide circuit actually expires. Apply the same max/syncRetryAt < retryAt guard to the primary row before writing syncRetryAt.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52b4c04488
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const partialChecksError = gitHubPartialChecksError(snapshot.metadata); | ||
| if (!args.skipCollisionGuard && partialChecksError) { | ||
| const failureNow = new Date(); | ||
| const failure = await persistGitHubManualSyncFailure({ |
There was a problem hiding this comment.
When a manual refresh succeeds on the PR read but returns partial Checks metadata, the transaction above has already called upsertExternalResource, whose checksDiagnostic path increments syncFailureCount and stores the diagnostic. This new persistGitHubManualSyncFailure call increments syncFailureCount again for the same provider failure, so one Checks API 403/timeout/rate-limit is recorded as two failures and subsequent retries skip an exponential-backoff step. Consider either not counting partials in upsertExternalResource for this path or applying the provider retry circuit without another increment.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d023136f5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| **/node_modules | ||
| .next | ||
| .next-e2e | ||
| .next-* |
There was a problem hiding this comment.
In the release worker build path I checked (RELEASE.md builds forge-worker and the Dockerfile's worker stage does COPY ... next-env.d.ts ...), this pattern also matches the generated next-env.d.ts file at the repo root, not just .next-* build directories. Because ignored files are removed from the Docker context, docker compose build forge-worker will fail at that COPY once this line is present; add an exception such as !next-env.d.ts or use a narrower directory-only pattern.
Useful? React with 👍 / 👎.
| id: args.externalResourceId, | ||
| workspaceId: args.workspaceId, | ||
| provider: GITHUB_PROVIDER, | ||
| OR: [{ syncRetryAt: null }, { syncRetryAt: { lt: retryAt } }], |
There was a problem hiding this comment.
When a manual/MCP refresh has already called claimGitHubManualSync, the row's syncRetryAt contains the temporary collision lease. For workspaces with a short configured backoff, such as 1 minute, and a request timeout of 5 seconds or more, the computed failure retryAt can be earlier than that lease, so this extend-only predicate skips the update and leaves the temporary lease as the persisted retry gate returned to the UI. That blocks retries longer than the workspace backoff policy after the provider call has already failed; the primary row needs to distinguish the just-acquired lease from an older provider reset before refusing to shorten it.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Review context
Addresses the valid post-merge Codex finding on #29: #29 (comment)
Verification