fix(actions): retry 504 and time out fetches in dispatch-run - #89
fix(actions): retry 504 and time out fetches in dispatch-run#89saturninoabril wants to merge 2 commits into
Conversation
The dispatch-run worker's end-of-run shard upload (register + multipart) only retried network-layer errors, so a 504 from the load balancer on a large multipart upload failed the job immediately — the weakest link on the orchestrated path. It also set no request timeout, so a hung request waited on the LB idle timeout before surfacing. - auth.ts: retry transient HTTP statuses (408/429/502/503/504) with exponential backoff + Retry-After, matching the report-upload action; treat request timeouts / aborts as retryable network errors; export request-timeout constants, timeoutSignal(), and isTransientHTTPStatus(). - upload.ts / main.ts: attach AbortSignal.timeout to every fetch (30s for JSON control calls, 120s for multipart uploads) so a hung request fails fast and is retried instead of waiting on the gateway. - main.ts: /checkout and /complete no longer re-loop on the transient statuses the auth layer now handles, avoiding double-retry amplification while still covering the other 5xx for these idempotent calls. - Rebuilt dist/index.js (tsup). Co-authored-by: saturnino <saturnino@mattermost.com>
|
@coderabbitai review |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRetry handling now supports transient HTTP responses, ChangesRetry and timeout controls
Sequence Diagram(s)sequenceDiagram
participant Action
participant fetchWithRetry
participant HTTPEndpoint
participant RetryDelay
Action->>fetchWithRetry: Send authenticated request
fetchWithRetry->>HTTPEndpoint: Execute request
HTTPEndpoint-->>fetchWithRetry: Return transient response or network error
fetchWithRetry->>RetryDelay: Wait using backoff and Retry-After
RetryDelay-->>fetchWithRetry: Resume retry
fetchWithRetry-->>Action: Return response or final error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/actions/test-system-io-dispatch-run/src/auth.ts:
- Around line 70-83: Cap the delay returned by parseRetryAfterMs to an
action-appropriate maximum before it is used for sleeping. Apply the same upper
bound to both numeric seconds and parsed HTTP-date values, while preserving the
existing zero handling for missing, invalid, or past Retry-After values.
- Around line 103-113: Update fetchWithAuthRetry in
.github/actions/test-system-io-dispatch-run/src/auth.ts at lines 103-113 and its
callers in .github/actions/test-system-io-dispatch-run/src/main.ts at lines
362-365 and .github/actions/test-system-io-dispatch-run/src/upload.ts at lines
201-204 so response body consumption occurs inside the retried request, or
expose a helper returning parsed JSON. Ensure stalls while reading the body
remain covered by the 30s abort and trigger retries for checkout, complete, and
report registration flows; callers should use the new result without performing
a separate res.text() outside the retry loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b645e96f-9963-4ace-9970-d27b66ad4869
⛔ Files ignored due to path filters (1)
.github/actions/test-system-io-dispatch-run/dist/index.jsis excluded by!**/dist/**
📒 Files selected for processing (3)
.github/actions/test-system-io-dispatch-run/src/auth.ts.github/actions/test-system-io-dispatch-run/src/main.ts.github/actions/test-system-io-dispatch-run/src/upload.ts
…etry - parseRetryAfterMs now clamps to MAX_RETRY_AFTER_MS (30s) so an upstream 429/503 with a huge delay/date can't turn a retry into an unbounded stall. - Add fetchTextWithAuthRetry, which consumes the response body inside the retried scope. Previously callers did res.text() after fetchWithAuthRetry returned on headers, so a stall mid-body let the request timeout escape the retry loop and hard-fail /checkout, /complete, or report registration on the first stalled response. main.ts and upload.ts postJSON now use it. - Extract isRetryableFetchError (shared by both retry loops); add a timeout to the best-effort screenshot upload too. Rebuilt dist/index.js. Co-authored-by: saturnino <saturnino@mattermost.com>
2 participants
Summary
Hardens the reusable
test-system-io-dispatch-runaction — the orchestrated CI worker — against gateway timeouts, which is where the orchestrated upload path was weakest./reports/register+ multipart/reports/upload/...) went throughfetchWithAuthRetry, which only retried network-layer errors. An HTTP504from the load balancer on a large multipart upload failed the job immediately. This brings the action to parity withtest-system-io-report-upload: retry408/429/502/503/504with exponential backoff +Retry-After.fetchhad a request timeout, so a hung request waited on the load balancer's idle timeout before surfacing. Everyfetchnow carries anAbortSignal.timeout(30s for JSON control calls, 120s for multipart uploads); a timeout is classified as a retryable transient failure, so it fails fast and retries instead of hanging./checkoutand/complete(main.tspostJSON) previously re-looped on all 5xx. Since the auth layer now retries the transient gateway statuses itself, the call-site loop skips those (via the newisTransientHTTPStatus) to avoid double-retry amplification, while still covering the remaining 5xx for these idempotent calls.dist/index.jsbundle (tsup).No
action.ymlinput/output changes; behavior change is limited to retry/timeout resilience.Test Plan
tsc -b,oxlint,oxfmt --checkall clean;tsupbuild succeeds and the rebuilt bundle contains the new logic (AbortSignal.timeout,retry-after, transient-status retry).504 → 504 → 200sequence was retried to a final200(3 server hits);isTransientHTTPStatusclassifies504transient /500not; a request to an always-slow endpoint with a 200ms timeout threwTimeoutErrorafter being retried and gave up at ~4.3s (well before the 5s server delay), proving the timeout aborts and retries rather than hanging.Release Note