fix(net): try normal fetch() first, fail back to public-DNS IPv4 - #2
Conversation
|
Warning Review limit reached
More reviews will be available in 2 minutes and 6 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthrough
ChangesfetchResilient Control Flow Inversion
Sequence Diagram(s)sequenceDiagram
participant Caller
participant fetchResilient
participant SystemFetch as System fetch()
participant PublicDNS as dns.Resolver (public DNS)
participant IPv4Fetch as fetchOverIpv4
Caller->>fetchResilient: fetchResilient(url, init)
fetchResilient->>SystemFetch: fetch(url, init)
alt success or HTTP error (non-connection)
SystemFetch-->>fetchResilient: Response / non-connection Error
fetchResilient-->>Caller: return Response / rethrow
else connection-class failure
SystemFetch-->>fetchResilient: throws connection error
fetchResilient->>PublicDNS: resolve4(hostname)
alt DNS resolves
PublicDNS-->>fetchResilient: IPv4 address
fetchResilient->>IPv4Fetch: fetchOverIpv4(url, address, init)
IPv4Fetch-->>fetchResilient: Response
fetchResilient-->>Caller: return Response
else DNS fails
PublicDNS-->>fetchResilient: ESERVFAIL / no result
fetchResilient-->>Caller: rethrow original connection error
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates fetchResilient() (used for MikroTik download/upgrade endpoints) to prefer the system resolver (fetch(url)) first, and only fail back to a public-DNS A-record + IPv4-literal retry on connection-class errors—so local DNS overrides (hosts/VPN/split-horizon) remain effective in the common case while retaining CI resilience.
Changes:
- Reordered
fetchResilient()logic: normal fetch first; public-DNS IPv4 retry only on connection-class failures. - Reworked unit tests to match the new ordering and added a “surface original failure if public DNS has no answer” case.
- Updated DESIGN/backlog/instructions docs to reflect the new “normal-first, failback” behavior and guidance.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/net.ts |
Implements system-resolver-first behavior with a connection-failure-triggered public-DNS IPv4 retry; updates module docs accordingly. |
test/unit/net.test.ts |
Updates tests for new primary/failback ordering and adds an explicit “public DNS no answer” behavior test. |
DESIGN.md |
Reframes decision #9 to “normal fetch first, public-DNS IPv4 failback” and clarifies trigger/intent. |
BACKLOG.md |
Updates completed item description to reflect the reordered logic and rationale. |
.github/instructions/bun-http.instructions.md |
Updates HTTP guidance: external URLs should use fetchResilient(); adjusts mocking guidance for tests. |
| // Only a connection-class failure (e.g. a broken/slow stub resolver, or an | ||
| // unreachable AAAA) is worth the public-DNS workaround. Aborts/timeouts and | ||
| // everything else propagate unchanged. | ||
| if (!isConnectionFailure(err)) throw err; |
There was a problem hiding this comment.
| test("uses a plain fetch on the system resolver as the primary path", async () => { | ||
| const resolveSpy = spyOn(dns.Resolver.prototype, "resolve4"); | ||
| const fetchSpy = spyOn(globalThis, "fetch").mockImplementation((async ( |
There was a problem hiding this comment.
Good catch — fixed in a2ae707. These now use mockResolve4({ reject: … }) so resolve4 fails fast instead of calling through to real DNS, keeping the suite strictly network-free even under a regression. The toHaveBeenCalledTimes(0) assertions remain.
| test("returns the HTTP response as-is (no fallback) on a 5xx", async () => { | ||
| const resolveSpy = spyOn(dns.Resolver.prototype, "resolve4"); | ||
| const fetchSpy = spyOn(globalThis, "fetch").mockResolvedValue( | ||
| new Response("nope", { status: 503 }), | ||
| ); |
There was a problem hiding this comment.
Good catch — fixed in a2ae707. These now use mockResolve4({ reject: … }) so resolve4 fails fast instead of calling through to real DNS, keeping the suite strictly network-free even under a regression. The toHaveBeenCalledTimes(0) assertions remain.
| test("rethrows non-connection errors (e.g. timeouts) without a fallback", async () => { | ||
| mockResolve4({ resolve: ["203.0.113.7"] }); | ||
| const resolveSpy = spyOn(dns.Resolver.prototype, "resolve4"); | ||
| const fetchSpy = spyOn(globalThis, "fetch").mockImplementation((async () => { |
There was a problem hiding this comment.
Good catch — fixed in a2ae707. These now use mockResolve4({ reject: … }) so resolve4 fails fast instead of calling through to real DNS, keeping the suite strictly network-free even under a regression. The toHaveBeenCalledTimes(0) assertions remain.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/net.ts (1)
56-60:⚠️ Potential issue | 🟠 MajorNarrow
TypeErrorretry gating to connection-class failures only
isConnectionFailure()at line 59 returnstruefor anyTypeError, which is broader than the documented contract. Per the module docstring (line 15), Bun surfaces actual connection failures witherrno: 0and specific codes; a bareTypeErrorwithout this errno marker may not indicate a connection failure and should not unconditionally trigger the DNS fallback.Proposed fix
export function isConnectionFailure(err: unknown): boolean { if (!err || typeof err !== "object") return false; - const e = err as { name?: string; code?: string; cause?: { code?: string } }; + const e = err as { + name?: string; + message?: string; + code?: string; + errno?: number; + cause?: { code?: string }; + }; if (e.name === "AbortError") return false; const code = e.code ?? e.cause?.code; if ( code === "ConnectionRefused" || code === "FailedToOpenSocket" || code === "ConnectionClosed" || code === "ECONNREFUSED" || code === "ECONNRESET" || code === "EHOSTUNREACH" || code === "ENETUNREACH" || code === "ETIMEDOUT" ) { return true; } - // Bun wraps low-level connect failures as a bare TypeError ("Unable to - // connect…"). Treat those as retriable too; a wrong guess only costs one - // normal fetch, which then surfaces the real error if it also fails. - return e.name === "TypeError"; + // Bun may surface connect failures as TypeError with errno 0 / connect wording. + if (e.name === "TypeError") { + const msg = e.message ?? ""; + return e.errno === 0 && /connect|socket|refused|unreach|network/i.test(msg); + } + return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/net.ts` around lines 56 - 60, The isConnectionFailure() function currently returns true for any TypeError, which is overly broad and inconsistent with the module's documented behavior. According to the module docstring, Bun surfaces actual connection failures with specific error characteristics including errno: 0 and specific error codes. Narrow the TypeError check in isConnectionFailure() to only return true when the error object has these specific connection-related properties (errno value and connection-specific error codes), not for all TypeErrors, to ensure the DNS fallback is only triggered for genuine connection failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit/net.test.ts`:
- Around line 108-117: Add a new test case after the existing timeout error test
that verifies fetchResilient does not retry on plain TypeError exceptions that
lack connection-class markers. In this new test, mock the fetch function to
throw a plain TypeError (not an AbortError or connection-related error), call
fetchResilient with a sample URL, and assert that the promise rejects with the
TypeError, the fetchSpy is called only once (indicating no retry occurred), and
the resolveSpy is called zero times (confirming no DNS fallback retry was
attempted). This ensures the retry behavior remains restricted to
connection-class errors only.
---
Outside diff comments:
In `@src/lib/net.ts`:
- Around line 56-60: The isConnectionFailure() function currently returns true
for any TypeError, which is overly broad and inconsistent with the module's
documented behavior. According to the module docstring, Bun surfaces actual
connection failures with specific error characteristics including errno: 0 and
specific error codes. Narrow the TypeError check in isConnectionFailure() to
only return true when the error object has these specific connection-related
properties (errno value and connection-specific error codes), not for all
TypeErrors, to ensure the DNS fallback is only triggered for genuine connection
failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1d81d08c-0284-4ce8-bd00-ab9bf15bb36d
📒 Files selected for processing (5)
.github/instructions/bun-http.instructions.mdBACKLOG.mdDESIGN.mdsrc/lib/net.tstest/unit/net.test.ts
… wording Review follow-ups on PR #2 (Copilot + CodeRabbit): - isConnectionFailure(): gate the TypeError net on `errno === 0` (Bun's connect-failure marker) instead of matching any TypeError, so an unrelated TypeError (a real bug) surfaces immediately rather than triggering a pointless DNS retry. Confirmed by probe that Bun 1.3 surfaces real DNS/connect failures as Error+code:"ConnectionRefused"+errno:0 (caught by the typed-code list); declined CodeRabbit's message-regex variant as version-fragile (would miss "failed to lookup"-style wording). - Tests: mock dns.Resolver.prototype.resolve4 to reject in the three primary-path/no-retry cases so the suite stays strictly network-free even if a regression accidentally invokes it (Copilot); add a case asserting a plain TypeError without the errno marker does not retry (CodeRabbit). - Docs: correct "aborts/timeouts pass through" wording in net.ts and DESIGN.md — AbortError (incl. AbortSignal.timeout) passes through, but a low-level connect ETIMEDOUT is connection-class and is retried (Copilot). Fix a stale isConnectionFailure doc line that described the old IPv4-first direction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review follow-ups — pushed in a2ae707Thanks both. Summary, including one place I diverged from the suggested fix. Copilot
CodeRabbit
Validation: |
| 9. **Resilient downloads: normal `fetch` first, public-DNS IPv4 as a failback** — All fetches to MikroTik's `upgrade`/`download` hosts go through `fetchResilient()` (`src/lib/net.ts`), never bare `fetch()`. The intent is narrow: a **plain `fetch` is the path** — dual-stack (happy eyeballs), on par with curl/most tools, and honoring local DNS (`/etc/hosts` pins, VPN/split-horizon, mirror redirects, IPv6-only egress) — and we layer one **failback** beneath it to ride out *DNS misconfiguration somewhere in the environment*, wherever it comes from. We deliberately do **not** invert this (public-DNS+IPv4 first) — that would override local DNS on every machine and could introduce its own failures (e.g. an IPv4-only connect on an IPv6-only network). Normal first keeps us on par with most tools; the failback only adds smarts when the normal path actually breaks. | ||
|
|
||
| Measured on the CI runner: `lookup({family:4})` → `ESERVFAIL` ~9 s; `lookup({all})` → `ESERVFAIL`/`ETIMEOUT` 22–26 s; `resolve4` (resolv.conf) → `ESERVFAIL` 2–22 s; `Resolver([1.1.1.1,8.8.8.8]).resolve4` → **OK ~10 ms**. | ||
| The trigger is precise: only when the normal `fetch` throws a **connection-class error** (`isConnectionFailure()` — the `errno: 0` / bare-`TypeError` / `ECONN*` family, *not* aborts/timeouts or any HTTP response) does `fetchResilient` retry. It resolves the A record by querying public DNS **directly** (a `dns.Resolver` with `setServers([1.1.1.1, 8.8.8.8, 1.0.0.1])`, bounded by a 3 s timeout so a blocked resolver doesn't stall), then connects to the IPv4 literal preserving the `Host` header and TLS SNI so certificate validation still passes. If public DNS also has no answer it surfaces the original failure. HTTP responses (incl. 5xx) and aborts (`AbortError`, e.g. from `AbortSignal.timeout`) pass through unchanged, never retried; a `TypeError` retries only with Bun's `errno: 0` connect-failure marker, so an unrelated `TypeError` (a real bug) surfaces immediately. |
There was a problem hiding this comment.
Fixed in 8a837b5 — the trigger description now reads "or a TypeError carrying Bun's errno: 0 connect-failure marker," matching isConnectionFailure() and the tests. Thanks for catching the leftover "bare TypeError" wording.
Second-pass review follow-ups on PR #2: - DESIGN.md #9: the trigger description still said "bare-TypeError"; align it with the implementation — a TypeError counts only with Bun's errno:0 marker (Copilot). - net.ts: add a docstring to fetchOverIpv4 (the failback transport) explaining why Host + TLS SNI are pinned to the original hostname when connecting to an IPv4 literal — clears CodeRabbit's docstring-coverage pre-merge warning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| * Primary path: a plain `fetch` on the system resolver. This is tried first, | ||
| * even at a small latency cost when it fails. It is dual-stack (happy eyeballs) | ||
| * and on par with curl/most tools, and it honors local DNS configuration — |
There was a problem hiding this comment.
Fixed in a3902b5 — dropped "small latency cost"; the header now reads "accepting the latency cost when it fails — usually small, but up to the resolver's own failure time (the broken CI stub below took 2–26 s)."
| * Fetch `url` but connect to an explicit IPv4 `address`, keeping HTTP and TLS | ||
| * pointed at the original host: the `Host` header and TLS SNI (`serverName`) are | ||
| * set to the URL's hostname so virtual-hosting routing and certificate | ||
| * validation still pass against the IP literal. The failback's transport. |
There was a problem hiding this comment.
Fixed in a3902b5 — tls: { ...init?.tls, serverName: host } so the forced hostname wins. Added a regression guard in the failback test (conflicting caller serverName).
| }) as unknown as typeof fetch); | ||
| const res = await fetchResilient("https://h.example/x"); | ||
|
|
||
| const res = await fetchResilient("https://upgrade.mikrotik.com/x"); |
There was a problem hiding this comment.
Done in a3902b5 — the test now calls fetchResilient(url, { tls: { serverName: "attacker.example" } }) and asserts the IPv4 attempt still uses upgrade.mikrotik.com. Confirmed it fails against the pre-fix merge order, so it genuinely guards the behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/net.ts`:
- Around line 101-106: The SNI pinning guarantee documented in the comment block
is being violated because caller-provided init.tls.serverName can override the
required hostname pinning. Locate the code that merges the caller-provided TLS
options with the failback transport configuration (likely immediately following
this comment block) and ensure that after all merges and option processing, the
serverName property is explicitly set to the URL's hostname to enforce SNI
pinning and guarantee certificate validation against the IP literal address,
preventing the caller from breaking this security requirement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 566133cb-13a6-4241-81fa-ba02f529bec3
📒 Files selected for processing (3)
DESIGN.mdsrc/lib/net.tstest/unit/net.test.ts
fetchResilient() previously resolved via public DNS and connected over the IPv4 literal as its *primary* path, falling back to a normal fetch only when public DNS was unavailable. That overrode local DNS on every machine (hosts pins, VPN/split-horizon, mirror redirects) and risked introducing its own failure modes off-CI (e.g. an IPv4-only connect on an IPv6-only network). Reverse the order: a plain fetch() on the system resolver is now the path (dual-stack, on par with curl/most tools), and the public-DNS + IPv4-literal attempt is a failback triggered only by a connection-class error (isConnectionFailure: errno-0 / bare-TypeError / ECONN* family — never aborts, timeouts, or HTTP responses). Host header + TLS SNI are preserved on the failback so cert validation still passes; if public DNS also has no answer, the original failure is surfaced. This keeps the CI ESERVFAIL recovery (now the exception path, not the default) while framing the behavior as generic resilience to a misconfigured/transient resolver rather than a CI-specific patch — the deeper root cause of those ESERVFAILs is still unknown, so we don't over-fit to one incident. Docs: rewrite DESIGN.md decision #9 (failback framing, softened sister-project note to best-effort), update bun-http.instructions.md (external URLs go through fetchResilient, not bare fetch) and the BACKLOG entry. Tests in test/unit/net.test.ts rewritten for system-first ordering. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… wording Review follow-ups on PR #2 (Copilot + CodeRabbit): - isConnectionFailure(): gate the TypeError net on `errno === 0` (Bun's connect-failure marker) instead of matching any TypeError, so an unrelated TypeError (a real bug) surfaces immediately rather than triggering a pointless DNS retry. Confirmed by probe that Bun 1.3 surfaces real DNS/connect failures as Error+code:"ConnectionRefused"+errno:0 (caught by the typed-code list); declined CodeRabbit's message-regex variant as version-fragile (would miss "failed to lookup"-style wording). - Tests: mock dns.Resolver.prototype.resolve4 to reject in the three primary-path/no-retry cases so the suite stays strictly network-free even if a regression accidentally invokes it (Copilot); add a case asserting a plain TypeError without the errno marker does not retry (CodeRabbit). - Docs: correct "aborts/timeouts pass through" wording in net.ts and DESIGN.md — AbortError (incl. AbortSignal.timeout) passes through, but a low-level connect ETIMEDOUT is connection-class and is retried (Copilot). Fix a stale isConnectionFailure doc line that described the old IPv4-first direction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Second-pass review follow-ups on PR #2: - DESIGN.md #9: the trigger description still said "bare-TypeError"; align it with the implementation — a TypeError counts only with Bun's errno:0 marker (Copilot). - net.ts: add a docstring to fetchOverIpv4 (the failback transport) explaining why Host + TLS SNI are pinned to the original hostname when connecting to an IPv4 literal — clears CodeRabbit's docstring-coverage pre-merge warning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Third-pass review (CodeRabbit Major + Copilot) caught a real bug surfaced by
the fetchOverIpv4 docstring: the TLS merge order `{ serverName: host, ...init?.tls }`
let a caller-provided init.tls.serverName override the forced hostname, breaking
cert validation against the IP literal that the docstring promises. Flip to
`{ ...init?.tls, serverName: host }` so serverName always pins to the real host.
- net.test.ts: the failback test now passes a conflicting tls.serverName
("attacker.example") and asserts the IPv4 attempt still uses the real host —
verified to fail under the old merge order (Copilot).
- net.ts header: drop the "small latency cost" claim that contradicted the
documented 2–26 s slow-resolver case; state the cost is usually small but up
to the resolver's own failure time (Copilot).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve `bun run check` cspell findings introduced by this PR: - add "failback" to project-words.txt (deliberate term, distinct from fallback) - net.ts: "failback's transport" -> "failback transport" (avoid possessive) - DESIGN.md: "ECONN*" -> "ECONNREFUSED-family" (avoid bare ECONN token) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
a3902b5 to
1133a36
Compare
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Reverses the order in
fetchResilient()(src/lib/net.ts): a normalfetch()on the system resolver is now the primary path, and the public-DNS + IPv4-literal attempt is a failback triggered only by a connection-class error.Previously it was public-DNS-first — resolve via
1.1.1.1/8.8.8.8and connect over the IPv4 literal on every call, falling back to a normal fetch only when public DNS was unavailable. That overrode local DNS everywhere (/etc/hostspins, VPN/split-horizon, mirror redirects) and risked introducing its own failure modes off-CI (e.g. an IPv4-only connect on an IPv6-only network).Behavior now
fetch(url)— system resolver, dual-stack (happy eyeballs), on par with curl/most tools.isConnectionFailure:errno: 0/ bare-TypeError/ECONN*family — never aborts, timeouts, or HTTP responses) → resolve A record via public DNS directly (3 s timeout) and retry over the IPv4 literal withHostheader + TLS SNI preserved.This keeps the CI
ESERVFAILrecovery (now the exception path, not the default) while framing the behavior as generic resilience to a misconfigured/transient resolver rather than a CI-specific patch. The deeper root cause of thoseESERVFAILs is still unknown, so we deliberately don't over-fit to one incident.Docs
fetchfirst, public-DNS IPv4 as a failback"; demotes the CI runner to a "motivating incident (don't over-fit)" note; softens the sister-project guidance to best-effort.fetchResilient(), not barefetch(); updated the unit-mocking note.Test plan
bun test test/unit/net.test.ts— 9 pass (rewritten for system-first ordering: primary path asserts no public-DNS detour; failback asserts the IPv4 retry with Host+SNI; added "surfaces original failure when public DNS has no answer").bun test(full) — 555 pass / 0 fail / 154 skip.tsc --noEmitclean; Biome clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Refactor