diff --git a/.github/scripts/README.md b/.github/scripts/README.md index 727787170..d20d271ae 100644 --- a/.github/scripts/README.md +++ b/.github/scripts/README.md @@ -16,6 +16,30 @@ between jobs and covered by lightweight unit tests. steps can invoke them with a simple `python .github/scripts/.py` command. +## Retry Logic for GitHub API Calls + +To handle transient failures (rate limits, timeouts, network issues), use the retry helpers from `api-helpers.js`: + +```javascript +const { withBackoff, paginateWithBackoff } = require('./api-helpers'); + +// For single API calls +const result = await withBackoff( + () => github.rest.pulls.get({ owner, repo, pull_number: 123 }), + { core, maxRetries: 3 } +); + +// For paginated calls +const items = await paginateWithBackoff( + github, + github.rest.issues.listComments, + { owner, repo, issue_number: 123 }, + { core, maxRetries: 3 } +); +``` + +These helpers automatically retry transient errors (503, 504, rate limits, timeouts) with exponential backoff and jitter. + ## Tests Minimal Node and Python unit tests live alongside the scripts under diff --git a/.github/scripts/__tests__/api-helpers.test.js b/.github/scripts/__tests__/api-helpers.test.js index 27961a1b1..d73b567ff 100644 --- a/.github/scripts/__tests__/api-helpers.test.js +++ b/.github/scripts/__tests__/api-helpers.test.js @@ -3,7 +3,8 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { calculateBackoffDelay } = require('../api-helpers'); +const { calculateBackoffDelay } = require('../github_api_retry'); +const { withBackoff, paginateWithBackoff } = require('../api-helpers'); function withStubbedRandom(value, fn) { const originalRandom = Math.random; @@ -28,3 +29,48 @@ test('calculateBackoffDelay applies negative jitter within expected range', () = assert.equal(delay, 1500); }); }); + +test('withBackoff retries transient errors (not just rate limits)', async () => { + let attempts = 0; + const result = await withBackoff( + async () => { + attempts += 1; + if (attempts < 3) { + const error = new Error('Service temporarily unavailable'); + error.status = 503; + throw error; + } + return { data: 'success' }; + }, + { maxRetries: 3 } + ); + + assert.equal(attempts, 3); + assert.deepEqual(result, { data: 'success' }); +}); + +test('paginateWithBackoff retries transient errors', async () => { + let attempts = 0; + const mockGithub = { + paginate: async (method, params) => { + attempts += 1; + if (attempts < 2) { + const error = new Error('Network timeout'); + error.status = 504; + throw error; + } + return [{ id: 1 }, { id: 2 }]; + }, + }; + + const mockMethod = 'mockMethod'; + const result = await paginateWithBackoff( + mockGithub, + mockMethod, + { page: 1 }, + { maxRetries: 3 } + ); + + assert.equal(attempts, 2); + assert.deepEqual(result, [{ id: 1 }, { id: 2 }]); +}); diff --git a/.github/scripts/agents_pr_meta_update_body.js b/.github/scripts/agents_pr_meta_update_body.js index 6529d40f0..67c189ff7 100644 --- a/.github/scripts/agents_pr_meta_update_body.js +++ b/.github/scripts/agents_pr_meta_update_body.js @@ -208,13 +208,12 @@ function upsertBlock(body, marker, replacement) { /** * Simple retry wrapper with linear backoff for general API errors. * - * Note: This differs from api-helpers.js `withBackoff` which specifically handles - * rate limit errors (403/429) with exponential backoff and reset time extraction. - * This function retries any error type with simple linear delay, suitable for - * transient network/server errors during PR body updates. + * Note: api-helpers.js now has `withBackoff` which handles all transient errors + * (not just rate limits) with exponential backoff. This function uses linear delay + * which may be preferred for specific use cases in PR body updates. * * @param {Function} fn - Async function to retry - * @param {Object} options - Configuration options + * @param {Object} [options] - Retry configuration options * @param {number} [options.attempts=3] - Number of attempts * @param {number} [options.delayMs=1000] - Base delay between attempts in ms * @param {string} [options.description] - Label for logging diff --git a/.github/scripts/api-helpers.js b/.github/scripts/api-helpers.js index 6fa2a787a..885c2807c 100644 --- a/.github/scripts/api-helpers.js +++ b/.github/scripts/api-helpers.js @@ -7,6 +7,8 @@ * This module addresses Issue R-1 from WorkflowSystemBugReport.md */ +const { withGithubApiRetry, calculateBackoffDelay } = require('./github_api_retry'); + const DEFAULT_MAX_RETRIES = 3; const DEFAULT_BASE_DELAY_MS = 1000; const DEFAULT_MAX_DELAY_MS = 30000; @@ -58,23 +60,6 @@ function sleep(ms) { return new Promise((resolve) => setTimeout(resolve, ms)); } -/** - * Calculate delay with exponential backoff and jitter - * @param {number} attempt - Current attempt number (0-indexed) - * @param {number} baseDelay - Base delay in milliseconds - * @param {number} maxDelay - Maximum delay in milliseconds - * @returns {number} Calculated delay with jitter - */ -function calculateBackoffDelay(attempt, baseDelay = DEFAULT_BASE_DELAY_MS, maxDelay = DEFAULT_MAX_DELAY_MS) { - // Exponential backoff: baseDelay * 2^attempt - const exponentialDelay = baseDelay * Math.pow(2, attempt); - // Cap at max delay - const cappedDelay = Math.min(exponentialDelay, maxDelay); - // Add jitter (±25%) - const jitter = cappedDelay * 0.25 * (Math.random() * 2 - 1); - return Math.round(cappedDelay + jitter); -} - /** * Extract rate limit reset time from error or response headers * @param {Error|Object} errorOrResponse - Error object or API response @@ -160,49 +145,25 @@ async function paginateWithBackoff(github, method, params, options = {}) { core = null, } = options; - let lastError = null; - - for (let attempt = 0; attempt <= maxRetries; attempt++) { - try { - return await github.paginate(method, params); - } catch (error) { - lastError = error; - - // Check if this is a rate limit error we should retry - const isRateLimit = isRateLimitError(error); - const isSecondaryLimit = isSecondaryRateLimitError(error); - - if (!isRateLimit && !isSecondaryLimit) { - // Not a rate limit error, throw immediately - throw error; - } - - // Check if we have retries left - if (attempt >= maxRetries) { - log(core, 'error', `Rate limit exhausted after ${maxRetries + 1} attempts. Giving up.`); - throw error; - } - - // Calculate wait time - let delay; - if (isSecondaryLimit) { - // Secondary rate limits require longer waits - const resetTime = extractRateLimitReset(error); - delay = resetTime ? calculateWaitUntilReset(resetTime) : calculateBackoffDelay(attempt + 2, baseDelay, maxDelay); - } else { - const resetTime = extractRateLimitReset(error); - delay = resetTime ? calculateWaitUntilReset(resetTime) : calculateBackoffDelay(attempt, baseDelay, maxDelay); - } - - const limitType = isSecondaryLimit ? 'secondary rate limit' : 'rate limit'; - log(core, 'warning', `Hit ${limitType}. Retrying in ${Math.round(delay / 1000)}s (attempt ${attempt + 1}/${maxRetries + 1})`); - - await sleep(delay); + // Use withGithubApiRetry for comprehensive transient error handling + return withGithubApiRetry( + () => github.paginate(method, params), + { + operation: 'read', // Pagination is typically a read operation + label: 'GitHub API pagination', + maxRetriesByOperation: { + read: maxRetries, + write: maxRetries, + dispatch: maxRetries, + admin: maxRetries, + unknown: maxRetries, + }, + baseDelay, + maxDelay, + core, + backoffFn: calculateBackoffDelay, } - } - - // Should not reach here, but just in case - throw lastError || new Error('Pagination failed with unknown error'); + ); } /** @@ -225,43 +186,22 @@ async function withBackoff(apiCall, options = {}) { core = null, } = options; - let lastError = null; - - for (let attempt = 0; attempt <= maxRetries; attempt++) { - try { - return await apiCall(); - } catch (error) { - lastError = error; - - const isRateLimit = isRateLimitError(error); - const isSecondaryLimit = isSecondaryRateLimitError(error); - - if (!isRateLimit && !isSecondaryLimit) { - throw error; - } - - if (attempt >= maxRetries) { - log(core, 'error', `Rate limit exhausted after ${maxRetries + 1} attempts. Giving up.`); - throw error; - } - - let delay; - if (isSecondaryLimit) { - const resetTime = extractRateLimitReset(error); - delay = resetTime ? calculateWaitUntilReset(resetTime) : calculateBackoffDelay(attempt + 2, baseDelay, maxDelay); - } else { - const resetTime = extractRateLimitReset(error); - delay = resetTime ? calculateWaitUntilReset(resetTime) : calculateBackoffDelay(attempt, baseDelay, maxDelay); - } - - const limitType = isSecondaryLimit ? 'secondary rate limit' : 'rate limit'; - log(core, 'warning', `Hit ${limitType}. Retrying in ${Math.round(delay / 1000)}s (attempt ${attempt + 1}/${maxRetries + 1})`); - - await sleep(delay); - } - } - - throw lastError || new Error('API call failed with unknown error'); + // Use withGithubApiRetry for comprehensive transient error handling + return withGithubApiRetry(apiCall, { + operation: 'read', // Default to read operation + label: 'GitHub API call', + maxRetriesByOperation: { + read: maxRetries, + write: maxRetries, + dispatch: maxRetries, + admin: maxRetries, + unknown: maxRetries, + }, + baseDelay, + maxDelay, + core, + backoffFn: calculateBackoffDelay, + }); } /** @@ -375,7 +315,6 @@ module.exports = { isRateLimitError, isSecondaryRateLimitError, sleep, - calculateBackoffDelay, extractRateLimitReset, calculateWaitUntilReset, diff --git a/.github/scripts/github_api_retry.js b/.github/scripts/github_api_retry.js index a683acafa..0ca1c951c 100644 --- a/.github/scripts/github_api_retry.js +++ b/.github/scripts/github_api_retry.js @@ -1,7 +1,6 @@ 'use strict'; const { classifyError, ERROR_CATEGORIES } = require('./error_classifier'); -const { calculateBackoffDelay } = require('./api-helpers'); const DEFAULT_RETRY_LIMITS = Object.freeze({ read: 3, @@ -14,6 +13,23 @@ const DEFAULT_RETRY_LIMITS = Object.freeze({ const DEFAULT_BASE_DELAY_MS = 1000; const DEFAULT_MAX_DELAY_MS = 30000; +/** + * Calculate delay with exponential backoff and jitter + * @param {number} attempt - Current attempt number (0-indexed) + * @param {number} baseDelay - Base delay in milliseconds + * @param {number} maxDelay - Maximum delay in milliseconds + * @returns {number} Calculated delay with jitter + */ +function calculateBackoffDelay(attempt, baseDelay = DEFAULT_BASE_DELAY_MS, maxDelay = DEFAULT_MAX_DELAY_MS) { + // Exponential backoff: baseDelay * 2^attempt + const exponentialDelay = baseDelay * Math.pow(2, attempt); + // Cap at max delay + const cappedDelay = Math.min(exponentialDelay, maxDelay); + // Add jitter (±25%) + const jitter = cappedDelay * 0.25 * (Math.random() * 2 - 1); + return Math.round(cappedDelay + jitter); +} + function normaliseHeaders(headers) { if (!headers || typeof headers !== 'object') { return {}; @@ -135,4 +151,5 @@ module.exports = { resolveMaxRetries, computeRetryDelayMs, withGithubApiRetry, + calculateBackoffDelay, }; diff --git a/.github/scripts/keepalive_gate.js b/.github/scripts/keepalive_gate.js index 66b62f307..b7d529503 100644 --- a/.github/scripts/keepalive_gate.js +++ b/.github/scripts/keepalive_gate.js @@ -1,6 +1,6 @@ 'use strict'; -const { paginateWithBackoff } = require('./api-helpers.js'); +const { paginateWithBackoff, withBackoff } = require('./api-helpers.js'); const KEEPALIVE_LABEL = 'agents:keepalive'; const AGENT_LABEL_PREFIX = 'agent:'; @@ -16,7 +16,7 @@ const ORCHESTRATOR_WORKFLOW_FILE = 'agents-70-orchestrator.yml'; const WORKER_WORKFLOW_FILE = 'agents-72-codex-belt-worker.yml'; const RECENT_COMPLETED_LOOKBACK_SECONDS = 300; // 5 minutes -// Rate limit retry configuration +// Rate limit retry configuration - now handled by api-helpers const RATE_LIMIT_MAX_RETRIES = 3; const RATE_LIMIT_BASE_DELAY_MS = 2000; @@ -46,40 +46,6 @@ function isRateLimitError(error) { ); } -/** - * Execute a GitHub API call with exponential backoff retry on rate limit errors. - * @template T - * @param {() => Promise} fn - The API call to execute - * @param {Object} [options] - * @param {number} [options.maxRetries=3] - Maximum retry attempts - * @param {number} [options.baseDelayMs=2000] - Base delay in milliseconds - * @param {Object} [options.core] - GitHub Actions core for logging - * @returns {Promise} - */ -async function withRateLimitRetry(fn, options = {}) { - const maxRetries = options.maxRetries ?? RATE_LIMIT_MAX_RETRIES; - const baseDelayMs = options.baseDelayMs ?? RATE_LIMIT_BASE_DELAY_MS; - const core = options.core; - - let lastError; - for (let attempt = 0; attempt <= maxRetries; attempt++) { - try { - return await fn(); - } catch (error) { - lastError = error; - if (!isRateLimitError(error) || attempt >= maxRetries) { - throw error; - } - const delay = baseDelayMs * Math.pow(2, attempt); - if (core?.info) { - core.info(`Rate limited, retrying in ${delay}ms (attempt ${attempt + 1}/${maxRetries})`); - } - await sleep(delay); - } - } - throw lastError; -} - function toInteger(value) { const parsed = Number.parseInt(value, 10); if (!Number.isFinite(parsed)) { @@ -814,9 +780,9 @@ async function evaluateRunCapForPr({ let pull; try { - const response = await withRateLimitRetry( + const response = await withBackoff( () => github.rest.pulls.get({ owner, repo, pull_number: number }), - { core } + { core, maxRetries: RATE_LIMIT_MAX_RETRIES, baseDelay: RATE_LIMIT_BASE_DELAY_MS } ); pull = response.data; } catch (error) { @@ -927,9 +893,9 @@ async function evaluateKeepaliveGate({ core, github, context, options = {} }) { let pr = pullRequest || null; if (!pr) { try { - const response = await withRateLimitRetry( + const response = await withBackoff( () => github.rest.pulls.get({ owner, repo, pull_number: prNumber }), - { core } + { core, maxRetries: RATE_LIMIT_MAX_RETRIES, baseDelay: RATE_LIMIT_BASE_DELAY_MS } ); pr = response.data; } catch (error) {