Skip to content

SMOODEV-2513: Land the connect-timeout fan-out across all five ports - #103

Merged
brentrager merged 1 commit into
mainfrom
fix/connect-timeout
Aug 20, 2026
Merged

SMOODEV-2513: Land the connect-timeout fan-out across all five ports#103
brentrager merged 1 commit into
mainfrom
fix/connect-timeout

Conversation

@brentrager

Copy link
Copy Markdown
Contributor

Consolidates #88 (Rust), #89 (Go), #90 (Python), #91 (.NET) and #92 (TypeScript) — open since 2026-07-10 — onto current main. I assessed each: the work is sound, it had just gone stale against traceparent injection (#95) and the redaction fix (#98), both of which touch the same single-request site. One consolidated PR rather than five sequential rebases; the five originals are closed pointing here.

The gap is real

Without a connect timeout, a SYN to a dead pod IP still lingering in a ClusterIP's iptables burns the whole-request budget before retry can try a live endpoint. Measured on this machine:

$ node -e "fetch('http://10.255.255.1/', {signal: AbortSignal.timeout(20000)}).catch(e => …)"
no-dispatcher failed after 10553 ms: UND_ERR_CONNECT_TIMEOUT

10.5s — undici's own default — where the client's whole-request timeout was supposed to be the ceiling. Bounded at 500ms it fails in ~1s. Slow-but-alive handlers are unaffected; the knob is off by default in every port.

port entry point mechanism
TypeScript connectTimeoutMs / FetchBuilder.withConnectTimeout undici Agent dispatcher (Node only)
Python TimeoutOptions(connect_timeout_ms=…) httpx.Timeout(connect=…)
Rust FetchOptions::connect_timeout_ms / with_connect_timeout reqwest::ClientBuilder::connect_timeout
Go ClientBuilder.WithConnectTimeout cloned default transport, dialer Timeout
.NET SmooFetchOptions.ConnectTimeout / WithConnectTimeout SocketsHttpHandler.ConnectTimeout

Two changes from the original PRs

1. undici is an optional peer dependency, not a runtime dependency. #92 put it in dependencies, which would have added a full HTTP stack to every consumer's tree — including browser bundles — for an opt-in, Node-only feature, in the same week we are evicting runtime deps (#101). It now follows the @opentelemetry/api pattern this package already uses: devDependency for our own tests, optional peerDependency for consumers, lazy await import() at the call site.

2. A requested connect timeout with undici absent now throws, with an actionable message, instead of silently continuing unbounded. A timeout that quietly isn't applied is the exact failure this feature exists to prevent. (Browsers still no-op, because no such knob exists there — that one is documented on the option.)

Spec C — shared corpus

All five regression tests read their knobs from the committed spec/connect-timeout-corpus.json:

{ "blackHoleUrl": "http://10.255.255.1:80/anything",
  "connectTimeoutMs": 500, "wholeRequestTimeoutMs": 5000, "maxElapsedMs": 3000 }

The behavior under test is timing, not an input/output mapping, so what has to stay in lockstep is the thresholds — raise the ceiling in one language only and that port silently stops catching the regression the other four still catch. Rust binds it with include_str!; .NET copies it next to the test assembly via the csproj; Go/Python/TS read it by path. The TS suite carries a positive control (maxElapsedMs > connectTimeoutMs, wholeRequestTimeoutMs > maxElapsedMs) so a corpus that failed to load reads red rather than trivially green.

Verification (all local, all five)

result
vitest run src/fetch.connect-timeout.spec.ts 4 passed
cargo test --test connect_timeout_tests 1 passed
go test -run TestConnectTimeout ./... 2 passed
uv run poe test 131 passed
dotnet test --filter ConnectTimeout 1 passed

Plus oxfmt/oxlint/tsc, ruff check + format, cargo fmt --check + clippy --all-targets -D warnings, go vet + gofmt, dotnet format --verify-no-changes — all clean.

Closes #88, closes #89, closes #90, closes #91, closes #92.

🤖 Generated with Claude Code

https://claude.ai/code/session_0152bbE1veqfG1SVJdyLCBxC

@changeset-bot

changeset-bot Bot commented Aug 20, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fb2bfb7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@smooai/fetch Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Consolidates PRs #88 (Rust), #89 (Go), #90 (Python), #91 (.NET) and #92
(TypeScript), open since 2026-07-10, onto current main. The work was sound;
it had gone stale against traceparent injection and the redaction fix, both
of which touch the same single-request site.

The gap is real: without a connect timeout, a SYN to a dead pod IP still
lingering in a ClusterIP's iptables burns the WHOLE-request budget before
retry can try a live endpoint. Measured locally, an unbounded connect to a
black hole fails after 10.5s (undici's own default); bounded at 500ms it
fails in ~1s.

Two changes from the original PRs:

  - undici is an OPTIONAL PEER dependency, not a runtime dependency. #92
    added it to `dependencies`, which would have put a full HTTP stack in
    the tree of every consumer -- including browser bundles -- for an
    opt-in Node-only feature. It follows the @opentelemetry/api pattern
    this package already uses: devDependency for our own tests, optional
    peer for consumers, lazy import at the call site.

  - Requesting a connect timeout without undici installed now THROWS with
    an actionable message rather than silently continuing unbounded. A
    timeout that quietly isn't applied is the exact failure this feature
    exists to prevent.

Per Spec C, the five regression tests read their knobs (black-hole URL,
connect timeout, whole timeout, elapsed ceiling) from the committed
spec/connect-timeout-corpus.json instead of each hard-coding them, so the
thresholds cannot drift apart per language. The TS suite carries a positive
control so a corpus that failed to load reads red rather than green.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0152bbE1veqfG1SVJdyLCBxC
@brentrager
brentrager force-pushed the fix/connect-timeout branch from 51b7449 to fb2bfb7 Compare August 20, 2026 18:26
@brentrager
brentrager merged commit a5434b0 into main Aug 20, 2026
1 check passed
@brentrager
brentrager deleted the fix/connect-timeout branch August 20, 2026 18:33
brentrager added a commit that referenced this pull request Aug 20, 2026
…h gate (#114)

* Two green signals that meant nothing: Go's test cache, and the publish gate

Both reported success while proving/doing nothing. Cross-repo findings from
the file and audit agents, both confirmed live in fetch by hand.

GO TEST CACHE. `go test` without -count=1 does NOT invalidate on a fixture
read from OUTSIDE the package directory, and the connect-timeout suite loads
spec/connect-timeout-corpus.json from the repo root. Demonstrated:

    go clean -testcache && go test -run TestConnectTimeout...  -> ok  0.818s
    go test -run TestConnectTimeout...                         -> ok  (cached)
    <corrupt the corpus: maxElapsedMs 3000 -> 1>
    go test -run TestConnectTimeout...                         -> ok  (cached)   <-- proves nothing
    go test -count=1 -run TestConnectTimeout...                -> FAIL           <-- correct

So the green Go lane on #103 verified nothing about the shared corpus. Fixed
in go:test and the CI Go lane, with the reason recorded in the corpus itself
where the next person wiring a Go loader will look. Re-ran the positive
control for all five ports afterwards: Rust, Python, TypeScript and .NET were
already honest -- each goes red on a corrupted corpus. Only Go was lying.

PUBLISH GATE. PyPI, crates.io, the Go tag and NuGet were all gated on
`steps.changesets.outputs.published == 'true'` -- on the npm publish
succeeding in the SAME run. A run dying after npm leaves the follow-up run
with no changesets to consume, so published is 'false', every remaining step
skips, and the check goes GREEN having published nothing. The audit agent
lost two versions from three registries exactly this way.

Fixed by making a rerun able to finish the job rather than skip it: gate on
being a publish run, and make each step idempotent (uv --check-url, crates
tolerating ONLY "already exists", the Go tag checked before it is pushed;
NuGet already had --skip-duplicate). A concurrency group stops the workflow
racing itself -- the 'Auto-Merge Changeset PR' step re-dispatches the
workflow AND the merge is itself a push to main, which is what produced
today's npm E403 on a release that had already succeeded. And a final step
reds the run if the released version is not live on npm, PyPI, crates.io and
the Go tag, so publishing nothing can no longer look like success.

Checked and found ALREADY FIXED: release.yml running `pnpm format` in write
mode (#99 made it a check; version:release ends with oxfmt --write, which
covers the changesets-generated CHANGELOG).

Checked and found CLEAN: no registry is stranded. Every git v3 tag from
3.3.4 on is present on npm, PyPI, crates.io, NuGet and as a go/fetch tag
(NuGet 3.6.1 pushed, in validation). The 3.0.x-3.3.3 gaps predate those
publish steps existing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0152bbE1veqfG1SVJdyLCBxC

* oxfmt owns pyproject.toml too — collapse the poe publish sequence

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0152bbE1veqfG1SVJdyLCBxC

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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