fix(http): retry dropped provider connections by default

[lavaman131]

Summary

Eval runs against github-copilot intermittently failed mid-run with {"error":"Connection error"} after a long thinking=xhigh turn. Root cause: a multi-minute no-token window (model thinking) lets the eval sandbox proxy / prod Copilot edge close the idle stream → undici socket reset → OpenAI SDK throws a fatal APIConnectionError, because provider/SDK retries were pinned to 0. Disabling the client idle timeout (httpIdleTimeoutMs:0) didn't help — it only lets Atomic wait on a socket the server already severed. The fix is to make socket-drop recovery the default.

Changes

• retry.provider.maxRetries default 0 → 5 so status-undefined connection drops retry instead of aborting. The existing retry.provider.maxRetryDelayMs (60s) cap still fails fast on long quota/rate-limit waits, so this does not block on usage limits.
• Evals: removed hard-coded httpIdleTimeoutMs:0 from atomic_pier.py and atomic_harbor.py; both now inherit the standard 10-minute idle timeout default.
• Evals: atomic_harbor.py install mirrors pier — installs fd-find/ripgrep (+ git, curl) and symlinks fdfind→fd.
• Updated docs/settings.md and CHANGELOG.md.

The idle timeout default is unchanged (10 min / 600000).

Notes

typecheck, lint, check:file-length, and test:unit all pass via prek. Recovery now comes from retrying drops, not from waiting forever on a severed socket; the finite idle timeout still reclaims stale connections.

[mintlify]

Preview deployment for your docs. Learn more about Mintlify Previews.

Project	Status	Preview	Updated (UTC)
bastani	🟢 Ready	View Preview	Jun 29, 2026, 7:46 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

[claude]

Code Review — fix(http): retry dropped connections and widen idle timeout defaults

Thanks for the clear write-up — the root-cause analysis (idle stream close → undici reset → fatal APIConnectionError because retries were pinned to 0) is well-reasoned, and pivoting from "wait forever" (httpIdleTimeoutMs:0) to "tolerate drops" (retries) is the right direction. The change is small and focused. A few things worth a look before merge.

1. The maxRetries 0 → 5 default is a global behavior change (main thing to confirm)

This isn't scoped to evals/Copilot — getProviderRetrySettings() feeds every provider via sdk.ts:362, so all users get provider-level retries on by default now. The previous docs explicitly warned against exactly this:

"Setting it above 0 can make SDK/provider retries handle out-of-usage-limit errors before Atomic sees them, which may block the agent until the provider quota resets."

The PR's safety argument is that maxRetryDelayMs (60s) fails fast on long quota waits. That's sound only if the provider SDK checks maxRetryDelayMs against the Retry-After header before consuming a retry attempt and sleeping — i.e. a 429 with a multi-hour Retry-After must bail immediately, not back off internally first. Worth confirming this holds for each SDK path (OpenAI/Copilot, Anthropic, Gemini), since the whole safety story rests on it. If any SDK ignores the cap for 429s, this reintroduces the "agent blocks until quota resets" footgun the old default was protecting against.

2. Retry amplification on expensive turns

Provider retries (5) compose with the agent-level retry loop (retry.maxRetries, default 3). A dropped connection on a long thinking=xhigh turn could now re-issue an expensive request several times (provider-internal × agent-level). For connection drops that's the intended resilience, but worth being aware the worst case multiplies latency/token cost. Not a blocker — just flagging the compounding.

3. No regression test locking in the new defaults

bun test:unit passing doesn't cover the changed values — I don't see any test asserting getProviderRetrySettings().maxRetries === 5 (when unset) or DEFAULT_HTTP_IDLE_TIMEOUT_MS === 700_000. Given this is a subtle defaults change with a non-obvious safety rationale, a couple of small assertions (including that an explicit maxRetries: 0 still disables retries via the ?? 5 fallback) would prevent a silent future regression. Recommended.

4. Minor: ~12 min label vs. value

700_000 ms = 11.67 min, labeled ~12 min. Harmless, but if you want the preset to be exactly what it says, 720_000 is a true 12 min. The 700000 choice also reads as somewhat arbitrary — a one-line comment on why ~700s specifically (e.g. observed worst-case first-token latency) would help future readers.

5. Eval harness changes — looks good, one small asymmetry

• Removing the hard-coded httpIdleTimeoutMs:0 from both adapters so they inherit the new finite default is consistent with the PR's thesis.
• atomic_harbor.py install now mirrors pier (--no-install-recommends, fd-find/ripgrep/git, fdfind→fd symlink) — nice consistency win.
• Asymmetry: atomic_pier.py branches on apk (Alpine) vs apt-get, but atomic_harbor.py only does the apt-get path. Fine if harbor's base image is always Debian/Ubuntu; otherwise it'd break on a non-apt image. Worth a one-line confirmation.
• Verified import json is still used in both files after removing _atomic_settings_config_command, and the command-string concatenations (heredoc → next statement) remain valid.

Minor nit

• The getProviderRetrySettings() interface still types maxRetries?: number (optional), but the impl now always returns a number via ?? 5. Harmless, but the ? is now slightly misleading.

Summary

Direction and implementation are solid; the eval changes are clean. The one item I'd want verified before merge is #1 — that maxRetryDelayMs genuinely short-circuits long Retry-After/quota responses across all provider SDKs — since the default flip to 5 only stays safe if that guarantee holds. Adding the regression test (#3) would also be cheap insurance for a defaults change like this.

[claude]

Review: retry dropped connections and widen idle timeout defaults

Thanks for the detailed write-up and root-cause analysis — the reasoning for preferring drop-tolerance via retries over an unbounded idle timeout is sound. The core change (maxRetries default 0 → 5) is small and correct, and the eval-harness simplification is clean. A few things to address before merge.

🔴 PR description is out of sync with the diff

The description's "Core defaults" section claims a change to http-dispatcher.ts:

DEFAULT_HTTP_IDLE_TIMEOUT_MS 600000 → 700000 (~12 min) … Also adds a ~12 min preset to the /settings picker.

Neither change exists in the diff — http-dispatcher.ts is not in the 5 changed files, and DEFAULT_HTTP_IDLE_TIMEOUT_MS is still 600_000. The CHANGELOG.md entry actually contradicts the body and correctly says the evals "now inherit the standard 10-minute idle timeout default."

This matters beyond bookkeeping: the eval harnesses previously set httpIdleTimeoutMs: 0 (unbounded) precisely because, per the removed comments, "long-context + thinking=xhigh requests near the model's prompt cap can take >5 min to first token." This PR removes that override without bumping the default, so eval runs now hard-cap idle at 10 min. If a turn genuinely needs a long time to first token (a slow request, not a dropped socket), retries don't help — each attempt restarts the same slow request and re-hits the 10-min cap. Please either (a) land the 700000 bump the description promised, or (b) confirm 10 min is deliberately sufficient and fix the description. Right now reviewers can't tell which behavior was intended.

🟡 No test coverage for the changed default

There are zero tests referencing getProviderRetrySettings (and none for the existing maxRetryDelayMs ?? 60000 default either). Since this PR changes a user-visible default, a small unit test would lock it in and prevent silent regressions:

• default returns maxRetries: 5 when unset
• an explicit maxRetries: 0 is still honored (the ?? 5 nullish-coalescing correctly preserves 0, but it's worth pinning so a future refactor to || doesn't break "disable retries")

🟡 Two retry layers now stack — confirm the interplay

maxRetries: 5 feeds the provider/SDK retry (sdk.ts:362), but there is also an agent-level retry loop (agent-session-retry.ts, retry.maxRetries default 3) whose _isRetryableError regex already matches connection.?error, socket hang up, terminated, etc. So a persistent connection failure can now be retried up to 5× by the SDK and then, on exhaustion surfacing as an error message, up to 3× more at the agent level — each agent-level retry re-arming 5 SDK retries. Worth a sentence in the PR confirming this multiplication is acceptable (worst-case wall-clock on a hard-down endpoint) and that the SDK retries are what actually fix the fatal throw the agent loop couldn't catch.

🟡 Verify the maxRetryDelayMs safety claim for connection errors

The docs/changelog justify flipping the old "keep at 0" guidance with:

The maxRetryDelayMs cap keeps this safe: connection errors back off briefly, while quota/rate-limit replies asking for a long delay still fail fast.

This is the crux of why raising the default is now safe (the old doc warned SDK retries could swallow quota errors and block until reset). maxRetryDelayMs is passed through to the pi-ai SDK (sdk.ts:363-364), but the guarantee depends on the SDK applying that cap to status-undefined connection errors with brief backoff while still failing fast on long server-requested Retry-After. Please confirm the SDK honors it that way for connection-level (no-response) errors, since that's the assumption protecting against the original "blocked until quota resets" failure mode.

🟢 Minor

• PR body's 700000 (~12 min) contradicts the code (600_000) and the changelog (10-minute). Pick one.
• evals/atomic_harbor.py: import json is still used elsewhere, so no dead import after removing _atomic_settings_config_command — 👍. Same for atomic_pier.py.
• Harbor install now mirrors pier (fd-find/ripgrep/git + fdfind→fd symlink) — nice parity win, though it's unrelated to the connection-error fix and arguably deserves its own changelog line.

Overall: the substance is right and well-justified. The blocker is reconciling the description/changelog with what actually shipped (the missing idle-timeout bump) so the eval behavior is unambiguous, plus a small test for the new default.