Skip to content

fix(sse): surface undici err.cause on dispatcher failure (#4252) - #4281

Merged
diegosouzapw merged 1 commit into
release/v3.8.30from
fix/proxyfetch-surface-cause
Jun 19, 2026
Merged

diegosouzapw merged 1 commit into
release/v3.8.30from
fix/proxyfetch-surface-cause

Conversation

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Summary

#4252 reports frequent 502 bursts from ProxyFetch Undici dispatcher failed, falling back to native fetch: fetch failed where curl and native fetch() work fine. The reason it's been undiagnosable: proxyFetch.ts logged only the bare "fetch failed" message and discarded err.cause — so the real reason (ECONNRESET / UND_ERR_CONNECT_TIMEOUT / ENETUNREACH / Happy-Eyeballs AggregateError) was invisible. When the native fallback also failed, the job sat in the concurrency queue until the 30s semaphore timeout, then tripped the circuit breaker — the cascade the report describes.

What this does (and does NOT do)

Does not fix the underlying network failure (environment-specific, not reproducible locally). Ruled out during investigation: autoSelectFamily (the custom Agent already inherits net.getDefaultAutoSelectFamily()=true, undici 8.5.0) and stale keep-alive socket (repro recovered 3/3).

Makes the failure diagnosable + fast-propagating:

  • describeFetchCause(err) flattens the cause chain + AggregateError sub-errors into one line (code/syscall/errno/address:port + truncated message). Never a stack trace (Rule fix(ui): fix Select dropdown dark theme inconsistency #12). Pure + testable.
  • The dispatcher-failure warn now appends describeFetchCause(dispatcherError).
  • When undici AND native both fail: log both causes + tag the propagated error with proxyFetchDetail so the combo executor sees a diagnosable error immediately, not a bare "fetch failed".

Validation (TDD — Rule #18)

4 new tests RED→GREEN (cause-chain flatten, AggregateError expansion, String() fallback, both-fail propagation). 8/8 in the file, sibling proxyFetch suites 18/18, typecheck:core + eslint + check:file-size (incl. #4273 test-file cap) clean.

Next step (gated on this): once a reporter's logs show the real err.cause, target the actual network fix. Tracked in #4252.

#4252 reports frequent 502 bursts from "ProxyFetch Undici dispatcher failed,
falling back to native fetch: fetch failed" where curl + native fetch work fine.
The blocker to diagnosing it is that proxyFetch logged only the bare "fetch failed"
message and DISCARDED err.cause — so the real reason (ECONNRESET vs
UND_ERR_CONNECT_TIMEOUT vs ENETUNREACH vs a Happy-Eyeballs AggregateError) was
invisible in the field, and when the native fallback ALSO failed the job was left
sitting in the concurrency queue until the 30s semaphore timeout (then tripped the
circuit breaker — the cascade the report describes).

This does NOT yet fix the underlying network failure (which is environment-specific
and not reproducible locally — ruled out: autoSelectFamily/Happy-Eyeballs is already
inherited by the custom Agent, and a stale-keepalive-socket repro recovered cleanly).
It makes the failure DIAGNOSABLE and propagate fast:

- describeFetchCause(err): flattens the cause chain + AggregateError sub-errors into
  one line (code/syscall/errno/address:port + truncated message). Never a stack (Rule #12).
- The dispatcher-failure warn now appends describeFetchCause(dispatcherError).
- When undici AND native both fail, log both causes and tag the propagated error with
  `proxyFetchDetail` so the combo executor sees a diagnosable error immediately instead
  of a bare "fetch failed".

TDD: 4 new tests (cause-chain flatten, AggregateError expansion, String() fallback,
both-fail propagation) RED before -> GREEN after; 8/8 in the file, sibling proxyFetch
suites 18/18, typecheck + eslint + file-size clean. proxyFetch.ts not frozen.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@diegosouzapw
diegosouzapw merged commit 98b0d5e into release/v3.8.30 Jun 19, 2026
4 checks passed

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the describeFetchCause utility to flatten and format fetch error cause chains (including AggregateError sub-errors) into a single diagnostic line, and integrates it into patchedFetch to provide detailed logs when both Undici and native fetch fallbacks fail. The feedback is highly constructive and aims to make the error parsing more robust. Specifically, it suggests handling primitive values in the error cause chain and AggregateError sub-errors, as well as replacing the instanceof Error check with a more inclusive object check to ensure diagnostic details are attached to non-standard or environment-specific exceptions.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +46 to +48
for (let depth = 0; cur && depth < 5 && !seen.has(cur); depth++) {
seen.add(cur);
const e = cur as Record<string, unknown>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If cur is a primitive value (such as a string or number), typeof cur !== 'object' will be true. Currently, the loop does not handle primitive causes, meaning that if an error has a string as its cause (e.g., new Error('fail', { cause: 'auth failed' })), the cause is completely discarded and not included in the diagnostics. We can fix this by checking if cur is a primitive, appending its string representation, and breaking the loop.

  for (let depth = 0; cur && depth < 5 && !seen.has(cur); depth++) {
    seen.add(cur);
    if (typeof cur !== "object") {
      parts.push(String(cur));
      break;
    }
    const e = cur as Record<string, unknown>;

Comment on lines +63 to +64
for (const sub of (e.errors as unknown[]).slice(0, 4)) {
const s = (sub ?? {}) as Record<string, unknown>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If e.errors contains primitive values (like strings) instead of structured objects, they will be silently ignored because they don't have properties like code, syscall, or message. We should handle primitive sub-errors by appending them directly to parts.

      for (const sub of (e.errors as unknown[]).slice(0, 4)) {
        if (sub && typeof sub !== "object") {
          parts.push("↳ " + String(sub));
          continue;
        }
        const s = (sub ?? {}) as Record<string, unknown>;

Comment on lines +490 to +492
if (nativeError instanceof Error) {
(nativeError as Error & { proxyFetchDetail?: string }).proxyFetchDetail = detail;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using nativeError instanceof Error might fail to match non-standard error objects, custom exception classes, or certain environment-specific exceptions (like some DOMExceptions) that do not inherit from the global Error class. To ensure the diagnostic proxyFetchDetail is always attached when possible, we should check if nativeError is a non-null object instead.

Suggested change
if (nativeError instanceof Error) {
(nativeError as Error & { proxyFetchDetail?: string }).proxyFetchDetail = detail;
}
if (nativeError && typeof nativeError === "object") {
(nativeError as Record<string, unknown>).proxyFetchDetail = detail;
}

diegosouzapw added a commit that referenced this pull request Jun 19, 2026
…tcher (#4252) (#4319)

The default direct dispatcher pools keep-alive sockets for up to
fetchKeepAliveTimeoutMs (4s). Edges like nvidia / opencode-zen silently
close idle keep-alive sockets within that window, so the next request
reusing a pooled socket fails with UND_ERR_SOCKET ("other side closed") —
in bursts. proxyFetch retried once, but the retry reused the SAME pooled
dispatcher and could grab another stale socket, then fell through to native
fetch (which also pools) → the job sat in the rate-limit queue until the
30s timeout → 502 + circuit breaker open.

Add getRetryDispatcher() (no keep-alive, no pipelining, mirrors the proxy
dispatcher mitigation) and use it for the retry attempt so it opens a fresh
socket that can't be a dead pooled one. The first attempt still uses the
pooled dispatcher, preserving healthy keep-alive reuse.

Regression test (DI mock) asserts the retry uses getRetryDispatcher(), not
getDefaultDispatcher() (RED before, GREEN after).

Refs #4281 (describeFetchCause diagnostics). Closes #4252
@diegosouzapw diegosouzapw mentioned this pull request Jun 20, 2026
@diegosouzapw
diegosouzapw deleted the fix/proxyfetch-surface-cause branch June 21, 2026 12:33
tkgo11 pushed a commit to tkgo11/OmniRoute that referenced this pull request Sep 23, 2026
…erence#4281)

Surface err.cause + propagate diagnosable error fast on undici dispatcher failure. TDD. cheaper-inference#4252.
tkgo11 pushed a commit to tkgo11/OmniRoute that referenced this pull request Sep 23, 2026
…tcher (cheaper-inference#4252) (cheaper-inference#4319)

The default direct dispatcher pools keep-alive sockets for up to
fetchKeepAliveTimeoutMs (4s). Edges like nvidia / opencode-zen silently
close idle keep-alive sockets within that window, so the next request
reusing a pooled socket fails with UND_ERR_SOCKET ("other side closed") —
in bursts. proxyFetch retried once, but the retry reused the SAME pooled
dispatcher and could grab another stale socket, then fell through to native
fetch (which also pools) → the job sat in the rate-limit queue until the
30s timeout → 502 + circuit breaker open.

Add getRetryDispatcher() (no keep-alive, no pipelining, mirrors the proxy
dispatcher mitigation) and use it for the retry attempt so it opens a fresh
socket that can't be a dead pooled one. The first attempt still uses the
pooled dispatcher, preserving healthy keep-alive reuse.

Regression test (DI mock) asserts the retry uses getRetryDispatcher(), not
getDefaultDispatcher() (RED before, GREEN after).

Refs cheaper-inference#4281 (describeFetchCause diagnostics). Closes cheaper-inference#4252
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.

1 participant