Honor X-RateLimit-Reset header and extend retry budget to recover from AI provider 429s.#8
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
CI Agent Review
The refactored createHttpFetch in source/ai.mts discards the AbortSignal parameter (named _signal), breaking in-flight HTTP request cancellation. The original createFetch forwarded signal to the native fetch() call, allowing abort to cancel ongoing network requests. In the new code, only the retry sleep respects the signal — actual in-flight requests run to completion even after abort fires. This is a functional regression that can leave connections open and consume resources after the caller has moved on (e.g., when the agentLoop idle timer fires).
| const url = `${configuration.apiUrl.replace(/\/$/, "")}/chat/completions` | ||
| const requestHeaders: Record<string, string> = { ...headers } | ||
| if (configuration.apiKey) requestHeaders["Authorization"] = `Bearer ${configuration.apiKey}` | ||
| return fetch(url, { method: "POST", headers: requestHeaders, body }) |
There was a problem hiding this comment.
The AbortSignal is received as _signal (explicitly unused) and never forwarded to fetch(). The original code passed signal to the native fetch() call, enabling in-flight request cancellation on abort. Without it, the signal only works during retry sleep phases — in-flight HTTP requests continue running even after abort, consuming resources and leaving connections open. Fix: pass signal to the fetch() call: return fetch(url, { method: "POST", headers: requestHeaders, body, signal })
…m AI provider 429s. Also extracts AI fetch retry logic into testable orchestration with injected leaf dependencies.
|
/review |
There was a problem hiding this comment.
CI Agent Review
The refactoring of createFetch from createFetch(configuration: AiConfiguration) to createFetch(dependencies: FetchDependencies) leaks retry-implementation details through the abstraction boundary, forcing the composition root to wire up internals (sleep, random, now, httpFetch) that were previously encapsulated within createFetch.
| const spawnGit = createSpawnGit(workspaceDirectory) | ||
| const aiConfiguration = parseAiConfiguration(Bun.env) | ||
| const fetch = createFetch(aiConfiguration) | ||
| const fetch = createFetch({ httpFetch: createHttpFetch(aiConfiguration), sleep: createSleep(), random: createRandom(), now: createNow() }) |
There was a problem hiding this comment.
The composition root is now coupled to retry timing details (sleep, random, now) that are implementation details of the retry mechanism inside ai.mts. Previously, createFetch(aiConfiguration) encapsulated all retry behavior; now every call site must construct four dependencies. Consider keeping the convenience signature createFetch(aiConfiguration) as the production entry point (wiring up defaults internally) and exposing a separate createFetchWithDependencies(dependencies) for tests, so changes to retry internals don't ripple outward to index.mts.
1 participant
Also extracts AI fetch retry logic into testable orchestration with injected leaf dependencies.