Skip to content

fix(http): disable proxy connect timeout - #1571

Merged
lavaman131 merged 2 commits into
mainfrom
fix/headless-proxy-connect-timeout
Jul 1, 2026
Merged

fix(http): disable proxy connect timeout#1571
lavaman131 merged 2 commits into
mainfrom
fix/headless-proxy-connect-timeout

Conversation

@flora131

@flora131 flora131 commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Disables Undici's implicit 10-second connect timeout on Atomic's global proxy-aware HTTP dispatcher (EnvHttpProxyAgent). Under policy/proxy layers such as Pier, the CONNECT tunnel can legitimately take longer than 10 seconds to establish; without this change, Undici aborts the connection mid-handshake and the failure surfaces as a spurious provider Connection error. instead of going through normal provider/agent retry handling.

Changes

  • packages/coding-agent/src/core/http-dispatcher.ts: extract dispatcher construction options into a new exported createHttpDispatcherOptions(timeoutMs) helper and pass connectTimeout: 0 to undici.EnvHttpProxyAgent, disabling the fixed connect-phase timeout while keeping the existing configurable bodyTimeout/headersTimeout idle timeout behavior. configureHttpDispatcher now delegates to the helper.
  • packages/coding-agent/test/http-dispatcher.test.ts: new regression test asserting createHttpDispatcherOptions returns connectTimeout: 0 alongside the configured idle timeouts. The helper is asserted directly rather than mocking undici.EnvHttpProxyAgent, avoiding brittle module-mocking of undici.
  • packages/coding-agent/CHANGELOG.md: document the fix under [Unreleased] / Fixed.

Validation

  • bun run --cwd packages/coding-agent test -- test/http-dispatcher.test.ts
  • bun run typecheck
  • bun run check:file-length
  • Pre-commit/pre-push hooks (bun run lint, bun run check:file-length, bun run test:unit)

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

PR Review: fix(http): disable proxy connect timeout

Thanks for tackling this — the diagnosis (spurious "Connection error." while a slow CONNECT tunnel is still establishing behind a policy proxy) is well-reasoned, and the changelog plus inline comment clearly explain the intent. A couple of things are worth verifying before merge.

🔴 High priority — is connectTimeout actually a recognized undici option?

The change passes a top-level connectTimeout: 0 to EnvHttpProxyAgent. I believe undici configures the connect-phase timeout via the nested connect options (buildConnectors timeout, default 10000ms), NOT a top-level connectTimeout. Evidence:

  1. This repo already does it that way — src/core/tools/fetch-url.ts:178 sets connect-phase behavior through new Agent({ connect: { lookup: ... } }). The idiomatic, codebase-consistent way to disable the connect timeout would be connect: { timeout: 0 }.
  2. undicis public API for the connect timeout is connect.timeout; I am not aware of a top-level connectTimeout on Client/Agent/ProxyAgent/EnvHttpProxyAgent options.

If connectTimeout is not a real option, undici will silently ignore it and the default 10s connect timeout stays in effect — i.e. the fix is a no-op and the original bug remains. undici is not installed in my review sandbox so I could not confirm at runtime; please double-check against undici 8.5.0:

  • Confirm connect.timeout (not connectTimeout) is the correct knob, and switch to connect: { timeout: 0 } if so.
  • Verify timeout: 0 truly disables the timer (undicis setupTimeout treats a falsy timeout as "no timer") rather than meaning "0ms / immediate".

🟠 The test cannot catch the above — it asserts the mock, not behavior

Because undici is fully mocked (vi.mock("undici", ...)), the test only proves that we pass whatever key we wrote to the constructor — it passes regardless of whether undici actually honors connectTimeout. That is the exact failure mode from the point above, so this test gives false confidence. Consider an assertion closer to real behavior (construct against real undici and inspect the effective connect options, or an integration test against a slow-CONNECT stub proxy). At minimum, verify the option name against real undici and keep this as a fast regression guard.

🟢 Minor / nits

  • configureHttpDispatcher also calls installCopilotGeminiReasoningInterceptor() (unmocked) as a test side effect — fine since it is idempotent, just noting it.
  • Changelog entry and inline comment are clear and appropriately detailed — nice.
  • Test runner (vitest) is correct for packages/coding-agent (upstream pi layout), so no bun:test concern here.

Summary

Intent and scope are good and the change is minimal. The blocker is confirming the undici option name: if it is connect.timeout rather than connectTimeout, both the fix and its test need updating. Everything else is minor.

@claude

claude Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review: fix(http): disable proxy connect timeout

Thanks for the focused fix and the clean refactor. Extracting createHttpDispatcherOptions to make the dispatcher config unit-testable (instead of mocking undici) is the right call, the ### Fixed changelog entry follows the repo convention, and the doc-comment on configureHttpDispatcher was kept in sync with the behavior change. Using vitest here is correct — that's the established convention for coding-agent (the bun:test rule in CLAUDE.md applies to the raw-TS workspace packages, not this upstream-pi fork).

A few things worth considering:

1. connectTimeout: 0 disables the connect timeout globally, not just behind a proxy

configureHttpDispatcher installs one global dispatcher for all egress. The comment and PR framing are about the Pier/proxy CONNECT path, but this change also removes the 10s connect timeout for direct, non-proxied connections. For a genuinely dead/black-holed endpoint with no proxy in the path, undici will no longer fail fast at 10s — the request will hang until the OS-level TCP connect timeout (platform-dependent, commonly ~20s–2min+, and effectively very long for black-holed SYNs) or until the caller's own abort/bodyTimeout fires. The PR says failures "should surface through the provider/agent retry path," but with no connect-phase ceiling there's nothing to surface quickly on a stuck connect. Worth confirming that's acceptable, or consider a generous-but-finite value (e.g. 60s) that accommodates slow Pier CONNECT while still bounding dead connects, rather than fully disabling.

2. The test proves the options shape, not that the bug is fixed

http-dispatcher.test.ts asserts the object passed to EnvHttpProxyAgent contains connectTimeout: 0 — good as a regression guard against the value being dropped. But it doesn't demonstrate that undici's EnvHttpProxyAgent actually honors connectTimeout: 0 for the proxy CONNECT tunnel phase specifically (vs. only the direct socket connect). In undici, connectTimeout maps to the connector's timeout, and timeout ? setTimeout(...) : null means 0 does disable it — so the mechanism is sound — but ProxyAgent/EnvHttpProxyAgent tunnel establishment routes options through a nested client, and it's worth a one-line confirmation (comment or PR note) that the option propagates to the CONNECT leg and not just the TCP-to-proxy leg. That's the exact behavior this fix depends on.

3. Minor

  • The extracted function's return type ConstructorParameters<typeof undici.EnvHttpProxyAgent>[0] is a nice typed contract — no any/unknown, consistent with the style guide.

None of these block the fix; #1 is the one I'd most want an explicit answer on, since it silently changes failure timing for the common non-proxy path. Nice, tightly-scoped PR overall.

Automated review — verify against your own judgment.

@lavaman131
lavaman131 merged commit b001b49 into main Jul 1, 2026
11 checks passed
@lavaman131
lavaman131 deleted the fix/headless-proxy-connect-timeout branch July 1, 2026 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants