fix(worker): handle 5xx errors in http retry logic

[brendan-kellam]

Summary by CodeRabbit

• New Features

  • Added automatic retry mechanism for 5xx HTTP server errors with exponential backoff strategy
  • Implemented intelligent rate limit handling by detecting and utilizing header-based reset times
  • Enhanced error handling to improve reliability during transient server failures

• Tests

  • Expanded test coverage for retry behavior, exponential backoff calculation, and rate limit detection scenarios

[coderabbitai]

Walkthrough

This PR introduces automatic retry logic for 5xx HTTP errors in backend utilities. It adds an Octokit error type guard, implements header-based rate-limit handling via the x-ratelimit-reset header with exponential backoff fallback, expands test coverage, and adds the http-status-codes dependency.

Changes

Cohort / File(s)	Summary
HTTP Error Retry Logic packages/backend/src/utils.ts, packages/backend/src/github.ts	Modified retry conditions to include 5xx, 403, and 429 status codes. Added header-based rate-limit handling using x-ratelimit-reset with exponential backoff fallback. Introduced isOctokitRequestError type guard to detect Octokit request errors.
Dependencies packages/backend/package.json	Added http-status-codes ^2.3.0 dependency for HTTP status code utilities.
Test Coverage packages/backend/src/utils.test.ts	Expanded fetchWithRetry test suite with scenarios for retryable statuses (5xx, 403, 429), non-retryable statuses (404, 401), exponential backoff verification, rate-limit header handling, max-attempt behavior, and logging on retries.
Documentation CHANGELOG.md	Added entry documenting retry support for 5xx HTTP errors under Unreleased section.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant fetchWithRetry
    participant HTTP as HTTP Request
    participant RateLimitCheck as Rate Limit<br/>Check
    participant BackoffTimer as Backoff<br/>Timer
    
    Caller->>fetchWithRetry: fetchWithRetry(url)
    loop Until success or max attempts
        fetchWithRetry->>HTTP: Make request
        HTTP-->>fetchWithRetry: Response with status
        alt Status is retriable (5xx, 403, 429)
            fetchWithRetry->>RateLimitCheck: Check x-ratelimit-reset header
            alt Header exists
                RateLimitCheck-->>BackoffTimer: Wait until reset time
            else
                RateLimitCheck-->>BackoffTimer: Exponential backoff
            end
            BackoffTimer-->>fetchWithRetry: Wait complete
            fetchWithRetry->>fetchWithRetry: Increment attempts
        else Status not retriable or max attempts reached
            fetchWithRetry-->>Caller: Return response or error
        end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding handling for 5xx HTTP errors in the retry logic, which aligns with the additions across CHANGELOG.md, utils.ts, and utils.test.ts.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch bkellam/fix-SOU-317

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

packages/backend/src/utils.ts (1)

75-98: ⚠️ Potential issue | 🟡 Minor

Log message is misleading for 5xx errors.

The warning message states "Rate limit exceeded" but the function now retries on 5xx server errors as well, which are not rate limit issues. Consider a more accurate message.

📝 Proposed fix

-                logger.warn(`Rate limit exceeded for ${identifier}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...`);
+                logger.warn(`Retriable error (status ${e.status}) for ${identifier}. Waiting ${waitTime}ms before retry ${attempts}/${maxAttempts}...`);

🧹 Nitpick comments (1)

packages/backend/src/github.ts (1)

53-65: Use instanceof RequestError instead of duck typing for more robust type narrowing.

The RequestError class sets name to "HttpError", so the current duck-type check works for actual Octokit errors. However, the Octokit documentation recommends using instanceof RequestError for type narrowing—it's more explicit and avoids potential false positives from non-Octokit errors with matching shape. Since RequestError is already imported, refactor to:

export const isOctokitRequestError = (error: unknown): error is RequestError => {
    return error instanceof RequestError;
};