Repository navigation
Conversation
Extract a shared `githubRequest` utility in `scripts/github-request.ts` that wraps `fetch()` with: - Exponential backoff retry (3 attempts) for transient errors (ECONNRESET, ECONNREFUSED, EPIPE, ETIMEDOUT, etc.) - Retry on retryable HTTP status codes (408, 429, 500, 502, 503, 504) - Respect for `Retry-After` headers from GitHub's rate limiter - Per-request timeout (30s default) via AbortController - 204 No Content handling - Jitter to avoid thundering herd on concurrent retries Migrate all 4 scripts to use the shared utility: - auto-close-duplicates.ts - backfill-duplicate-comments.ts - lifecycle-comment.ts - sweep.ts Previously, a single ECONNRESET or GitHub 502 would crash these CI scripts with no recovery. This is especially problematic for the sweep and auto-close scripts which make hundreds of sequential API calls during batch processing.
24 tests covering: - Basic GET/POST functionality and 204 No Content handling - Non-retryable HTTP errors (401, 403, 422) — no retry, immediate throw - Transient network error retry (ECONNRESET, EPIPE, ETIMEDOUT, ECONNREFUSED, Bun socket close) - Retryable HTTP status codes (429, 500, 502, 503) - Retry exhaustion — throws after max attempts - maxRetries=0 disables all retry - Mixed error scenarios (ECONNRESET → 502 → success) - Request timeout via AbortController - Custom User-Agent header
Test architecture improvements: - Export isTransientError, calculateDelay, parseRetryAfter for direct unit testing — follows common practice of testing internal logic independently from the integration layer - Split tests into unit tests (internal helpers) and integration tests (githubRequest end-to-end) - Nest describe blocks by concern: basic requests, non-retryable errors, transient network errors, retryable HTTP codes, maxRetries=0, mixed scenarios, timeout - Use test.each() for parameterized tests — reduces boilerplate for error code and status code coverage - Move beforeEach/afterEach inside the githubRequest describe block - Extract mockFetch() and failThenSucceed() helpers to eliminate repeated globalThis.fetch reassignment Test count: 24 → 52 (added unit tests for internal helpers)
|
New evidence from the official repository's scheduled sweep, and one replay-safety case to consider here. Issue Sweep run 41812092234 failed on main I checked this PR's head One concern: both retry branches also replay POSTs. Callers here include issue comments and workflow dispatches. A timeout, reset, or gateway error can leave the write outcome unknown; the server may already have accepted the first request. Retrying can then create a second comment or dispatch. I added two mocked cases: the first simulated write is accepted, then its response is lost ( A self-contained test beside import { afterEach, expect, test } from "bun:test";
import { githubRequest } from "./github-request.ts";
const originalFetch = globalThis.fetch;
afterEach(() => { globalThis.fetch = originalFetch; });
for (const failure of ["lost-response", "http-502"] as const) {
test(`does not replay a comment POST after ${failure}`, async () => {
let acceptedWrites = 0;
globalThis.fetch = (async () => {
acceptedWrites++;
if (acceptedWrites === 1) {
if (failure === "lost-response") {
throw new DOMException("The operation timed out.", "TimeoutError");
}
return new Response("Bad Gateway", { status: 502 });
}
return Response.json({ id: acceptedWrites });
}) as typeof fetch;
let caught: unknown;
try {
await githubRequest("/repos/example/example/issues/1/comments", "dummy", {
method: "POST", body: { body: "example" }, maxRetries: 1,
});
} catch (error) { caught = error; }
expect(acceptedWrites).toBe(1);
expect(caught).toBeDefined();
});
}Would restricting automatic retries to GET by default be a reasonable minimum, with any write retry requiring an explicit replay-safety decision? As a local proposal, adding It would also help to retain the method, endpoint, and original cause when the final attempt fails, so the next production timeout identifies the operation that stopped the sweep. The underlying network cause of today's failure remains unconfirmed. |
Summary
githubRequestutility (scripts/github-request.ts) that wrapsfetch()with retry, timeout, and rate-limit handlinggithubRequestfunctionsscripts/github-request.test.ts)ECONNRESETor GitHub 502 would crash these CI scripts with no recoveryWhat's included
The shared utility adds:
ECONNRESET,ECONNREFUSED,EPIPE,ETIMEDOUT,EAI_AGAIN, etc.)Retry-Afterheaders from GitHub's rate limiterAbortController{}instead of crashing on.json()parseTest architecture
Tests use Bun's native test runner (
bun:test) and are structured as:Unit tests (internal helpers exported for testability):
isTransientError— parameterized viatest.each()across all 10 transient error codes + Bun socket message + AbortError/TimeoutError + negative casescalculateDelay— exponential backoff curve, jitter range, cap at 32sparseRetryAfter— numeric seconds, HTTP-date, edge cases (0, negative, >300s cap, missing header)Integration tests (end-to-end
githubRequest):test.each([401, 403, 422])— immediate throw, 1 attempttest.each(["ECONNRESET", "EPIPE", "ETIMEDOUT", "ECONNREFUSED"])— recovers after N failurestest.each([429, 500, 502, 503, 504])— retries and recoversmaxRetries=0— disables retry for both network and HTTP errorsScripts updated
auto-close-duplicates.tsbackfill-duplicate-comments.tslifecycle-comment.tssweep.tsTest plan
bun test scripts/github-request.test.ts— 52/52 passingbun run scripts/sweep.ts --dry-run— verify no behavioral change in normal path[RETRY]messages appear with backoff delays