fix(proxyFetch): retry once on undici dispatcher failure before native fallback - #2222
Conversation
…e fallback (#13) * fix(proxyFetch): retry once on undici dispatcher failure before native fallback Observed 19x [ProxyFetch] Undici dispatcher failed warnings per hour on 2026-05-12 against direct connections (ModelSync internal self-fetch path). P330 proxy itself confirmed healthy; failures were in the direct-connection path where getDefaultDispatcher() hits transient undici socket errors. Native fetch fallback worked correctly, but the volume was excessive noise and indicated the dispatcher was not given a second chance. Adds a single retry on undici failure with 25-75ms jittered backoff before falling back to native. Tests cover both the retry-succeeds and retry-also-fails paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(proxyFetch): tests assert retry count; ReadableStream body guard; preserve log phrase Addresses code review feedback on PR #13: 1. Tests previously asserted log wording, not actual retry count. Rewrote with Approach B dependency injection: proxyFetch.ts now exports a named proxyFetch(url, opts, deps) that accepts injected undiciFetch/nativeFetch. Tests inject mocks and assert exact dispatcher call counts (==2 both-fail, ==2 retry-success, ==1 ReadableStream). Sanity check confirmed: removing the retry loop causes tests 1 and 2 to fail with 1 !== 2. Note: mock.module() is unavailable in Node 25.9.0 / tsx/ESM setup — Approach B (DI refactor) was used as specified in fallback instructions. 2. Warning message now contains original literal phrase intact: Undici dispatcher failed, falling back to native fetch — the (after retry) marker is appended as a parenthetical suffix so existing substring-based and equals-based monitoring matchers continue to fire. Monitoring audit found no hard-string matchers in /etc, /opt/uptime-kuma, /opt/grafana, /opt/prometheus, or alertmanager — only coverage JSON files referenced the string. 3. Added ReadableStream/Blob body guard: when options.body has a .getReader() method (ReadableStream) or .stream() method (Blob), maxAttempts is set to 1 to avoid silently sending a drained empty body on the retry attempt. New test confirms undiciCalls===1 and nativeCalls===1 for stream bodies. Casts use Record<string, unknown> to stay within T11 explicit-any budget. --------- Co-authored-by: OmniRoute Ops <ops@nomenak.dev> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a retry mechanism for undici fetch operations within proxyFetch.ts, including a guard to prevent retrying non-replayable request bodies such as ReadableStreams. It also implements a dependency injection pattern to facilitate unit testing, accompanied by a new test suite. Feedback focuses on enhancing type safety by utilizing existing type aliases and refining mock signatures in tests to avoid using 'as any'. Additionally, it is recommended to transition from string-based error checking to more robust error code validation for the retry logic.
|
|
||
| /** Injectable dependencies for testability (Approach B DI). */ | ||
| export type ProxyFetchDeps = { | ||
| undiciFetch?: (...args: unknown[]) => Promise<Response>; |
There was a problem hiding this comment.
The type for undiciFetch is (...args: unknown[]) => Promise<Response>, which is not type-safe. You can use the FetchWithDispatcher type alias defined in this same file to make it more specific and improve type safety. This will also allow for better type inference and reduce the need for type assertions in patchedFetch.
| undiciFetch?: (...args: unknown[]) => Promise<Response>; | |
| undiciFetch?: FetchWithDispatcher; |
There was a problem hiding this comment.
Done in 6c73fa1 — ProxyFetchDeps.undiciFetch now uses the local FetchWithDispatcher type alias. That also let me drop the three as any casts on nativeFetch in the test file. Good catch.
| // Only retry/fallback for connection/dispatcher errors, not HTTP errors | ||
| if ( | ||
| msg.includes("fetch failed") || | ||
| msg.includes("ECONNREFUSED") || | ||
| msg.includes("UND_ERR") | ||
| ) { |
There was a problem hiding this comment.
Relying on error message substrings for control flow can be brittle, as these messages might change in future library versions. It's more robust to check for specific error codes when they are available.
undici errors typically have a code property (e.g., UND_ERR_*), and system errors like ECONNREFUSED also expose a code. Checking for these codes directly will make the retry logic more resilient.
// Only retry/fallback for connection/dispatcher errors, not HTTP errors
const code = (dispatcherError as { code?: string })?.code;
if (
msg.includes("fetch failed") || // Keep for generic undici errors without a code
code === "ECONNREFUSED" ||
(code && code.startsWith("UND_ERR"))
) {There was a problem hiding this comment.
Done in 6c73fa1 — classifier now checks (err as { code?: string }).code first (ECONNREFUSED / UND_ERR* prefix) and falls back to the substring match for errors that lack a structured code. Kept the substring branch as a safety net since some undici versions throw plain Error("fetch failed") without a code — but agreed that code-based matching is more stable as the primary path.
| const mockUndici = async (..._args: unknown[]): Promise<Response> => { | ||
| undiciCalls++; | ||
| throw makeUndiciError("fetch failed"); | ||
| }; | ||
|
|
||
| const mockNative = async (): Promise<Response> => { | ||
| nativeCalls++; | ||
| return new Response("native-fallback-body", { status: 200 }); | ||
| }; | ||
|
|
||
| const res = await proxyFetch( | ||
| "https://example.invalid/test", | ||
| { method: "GET" }, | ||
| { undiciFetch: mockUndici, nativeFetch: mockNative as any } |
There was a problem hiding this comment.
To improve type safety and avoid using as any, the mock functions mockUndici and mockNative should be defined with the full function signature they are mocking, even if the arguments are unused. This makes the tests more robust and easier to understand.
This principle applies to all tests in this file.
const mockUndici = async (
_input: RequestInfo | URL,
_options?: RequestInit
): Promise<Response> => {
undiciCalls++;
throw makeUndiciError("fetch failed");
};
const mockNative = async (
_input: RequestInfo | URL,
_init?: RequestInit
): Promise<Response> => {
nativeCalls++;
return new Response("native-fallback-body", { status: 200 });
};
const res = await proxyFetch(
"https://example.invalid/test",
{ method: "GET" },
{ undiciFetch: mockUndici, nativeFetch: mockNative }
);There was a problem hiding this comment.
Done in 6c73fa1 — all 6 mocks (3 mockUndici, 3 mockNative) now use the full (input: RequestInfo | URL, init?: RequestInit) => Promise<Response> signature, and the as any casts at the three call sites are gone. Tests still pass.
…ching - `ProxyFetchDeps.undiciFetch` now uses the local `FetchWithDispatcher` type alias instead of `(...args: unknown[]) => Promise<Response>`. This preserves the dispatcher slot in the signature and removes the need for `as any` at the call site. - Error classifier now prefers the `.code` property of the dispatcher error (ECONNREFUSED / UND_ERR_*) over message-substring matching, and keeps the substring match as a fallback for errors that lack a structured code. More resilient to undici message-string changes. - Test mocks now use the full `(input: RequestInfo | URL, init?: RequestInit)` signature instead of `(..._args: unknown[])`, which lets us drop all three `as any` casts on `nativeFetch`. 3 unit tests still pass.
29b9f27
into
diegosouzapw:release/v3.8.0
|
Thanks @NomenAK! Your contribution has been integrated into The PR branch was synced with the latest Reviewed and merged via the |
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: #2135 (1proxy settings), #2227 (antigravity project ID), #2238 (Z.AI Search), #2240 (CLI Suite) - fix: #2217, #2218, #2219, #2221, #2222, #2223, #2224, #2231, #2233, #2236, #2242, #2243 - security: #2209 (stack trace exposure) - chore: #2228, #2234 Total contributors updated from 50+ to 55+.
…e fallback (diegosouzapw#2222) Integrated into release/v3.8.0
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: diegosouzapw#2135 (1proxy settings), diegosouzapw#2227 (antigravity project ID), diegosouzapw#2238 (Z.AI Search), diegosouzapw#2240 (CLI Suite) - fix: diegosouzapw#2217, diegosouzapw#2218, diegosouzapw#2219, diegosouzapw#2221, diegosouzapw#2222, diegosouzapw#2223, diegosouzapw#2224, diegosouzapw#2231, diegosouzapw#2233, diegosouzapw#2236, diegosouzapw#2242, diegosouzapw#2243 - security: diegosouzapw#2209 (stack trace exposure) - chore: diegosouzapw#2228, diegosouzapw#2234 Total contributors updated from 50+ to 55+.
…e fallback (diegosouzapw#2222) Integrated into release/v3.8.0
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: diegosouzapw#2135 (1proxy settings), diegosouzapw#2227 (antigravity project ID), diegosouzapw#2238 (Z.AI Search), diegosouzapw#2240 (CLI Suite) - fix: diegosouzapw#2217, diegosouzapw#2218, diegosouzapw#2219, diegosouzapw#2221, diegosouzapw#2222, diegosouzapw#2223, diegosouzapw#2224, diegosouzapw#2231, diegosouzapw#2233, diegosouzapw#2236, diegosouzapw#2242, diegosouzapw#2243 - security: diegosouzapw#2209 (stack trace exposure) - chore: diegosouzapw#2228, diegosouzapw#2234 Total contributors updated from 50+ to 55+.
What
Add a single retry with 25-75ms jittered backoff in
proxyFetch.patchedFetch()when the undici dispatcher fails, before falling back to native fetch. Bypass the retry when the request body is aReadableStreamorBlob(non-replayable).Why
On our fork we observed 19
[ProxyFetch] Undici dispatcher failed, falling back to native fetchwarnings per hour. After tracing, most originated from a 17-connection concurrent burst at container boot (the same race that the ModelSync readiness-gate PR addresses). The undici dispatcher transient failure was real — a second attempt typically succeeded — but the existing code went straight to native fetch on the first error, producing noise and bypassing connection pooling benefits.How
patchedFetch()in the direct-connection branch now loops at most 2× when the error matchesfetch failed,ECONNREFUSED, orUND_ERR_*codes. 25-75ms jitter between attempts to avoid synchronized retry storms from concurrent callers.onRequestStartremain immediate throws — not retried.bodyIsStreamguard: ifoptions.bodyis aReadableStream(has.getReader()) orBlob(has.stream()),maxAttempts=1so the body isn't silently emptied on a retry."Undici dispatcher failed, falling back to native fetch"verbatim (adds(after retry)as a parenthetical) so existing substring-based monitoring continues to fire.tests/unit/proxyfetch-undici-retry.test.ts: retry-succeeds, retry-also-fails-falls-to-native, no-retry-on-streamed-body. Uses a DI pattern (proxyFetch(url, opts, deps?: ProxyFetchDeps)) so tests can inject mocked undici/native fetches and assert exact call counts.Notes
The DI signature change (
deps?3rd arg) is backward compatible — existing two-arg callers work unchanged. Happy to revise the jitter window or the error classifier if you have observed different transient patterns.