Skip to content

SMOODEV-2513: Add connect timeout to TS fetch SDK (default-off) - #92

Closed
brentrager wants to merge 1 commit into
mainfrom
SMOODEV-2513-connect-timeout-ts
Closed

SMOODEV-2513: Add connect timeout to TS fetch SDK (default-off)#92
brentrager wants to merge 1 commit into
mainfrom
SMOODEV-2513-connect-timeout-ts

Conversation

@brentrager

Copy link
Copy Markdown
Contributor

What

Adds an optional connect timeout to the TypeScript SmooAI fetch SDK — parity with the Rust PR #88 (with_connect_timeout, SMOODEV-2513).

  • New RequestOptions.connectTimeoutMs option (same options surface as the existing whole-request timeout: { timeoutMs }).
  • New builder method FetchBuilder.withConnectTimeout(ms), mirroring withTimeout(...) and the Rust builder.

Why

api-prime's ~16s stalls (SMOODEV-2498 / SMOODEV-2481) were fresh SYNs to dead pod IPs still lingering in a ClusterIP's iptables. The whole-request timeout only fires after the entire request budget elapses, so a black-holed connect stalls for the full window before retry can even try a live endpoint. A connect timeout bounds only the connection-establishment phase: a dead connect fails in ~the configured window and the configured retry lands on a live pod. Slow-but-alive handlers are unaffected.

How

Node's global fetch is undici under the hood. A connect timeout requires a dispatcher — an undici Agent({ connect: { timeout: ms } }) — passed on the fetch init. When connectTimeoutMs is set we lazily build (and cache, keyed by ms) such an Agent and attach it as dispatcher.

  • undici added as a dependency, pinned to ^6 to match Node 22's bundled undici handler interface (npm undici@8's Agent is rejected by Node 22's built-in fetch with UND_ERR_INVALID_ARG / invalid onRequestStart method — a real cross-version footgun). It's imported via a guarded await import('undici'), so it never loads unless a connect timeout is actually requested, and it stays an external dynamic import in both the node and browser builds (not bundled).

Default-off (behavior-identical)

When connectTimeoutMs is unset, no dispatcher is attached — the fetch call is byte-identical to today. The undici import is never reached. In browser/worker environments the option is ignored (no connect-timeout knob there; getConnectTimeoutDispatcher returns undefined before importing undici).

Test

src/fetch.connect-timeout.spec.ts mirrors the Rust connect_timeout_tests.rs: a connect to the non-routable black-hole http://10.255.255.1:80/ with connectTimeoutMs: 500 and a 10x-larger whole-request timeout of 5000ms fails in ~1s (well under 3s), and the error is not the mollitia whole-request TimeoutError. Plus a builder-path test and an unset-option (default-behavior) test. All 51 TS tests pass (48 existing + 3 new); typecheck, lint, format, and both builds are green.

🤖 Generated with Claude Code

…i dispatcher)

Bounds only the connection-establishment phase so a black-holed connect
(a SYN to a dead pod IP still lingering in a ClusterIP's iptables) fails
fast and retry can land on a live endpoint, instead of stalling until the
whole-request timeout. Node only, via a lazily-loaded undici Agent
dispatcher; ignored in browser/worker builds. Default-off: unset preserves
the previous behavior exactly. Parity with Rust with_connect_timeout (#88).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 22ba327

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

brentrager added a commit that referenced this pull request Aug 20, 2026
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 added a commit that referenced this pull request Aug 20, 2026
…103)

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.


Claude-Session: https://claude.ai/code/session_0152bbE1veqfG1SVJdyLCBxC

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@brentrager

Copy link
Copy Markdown
Contributor Author

Landed via #103, which consolidates all five ports onto current main. The work here was sound — it had gone stale against traceparent injection (#95) and the credential-redaction fix (#98), both of which touch the same single-request site.

Two changes from this PR in the consolidated version:

  • undici is an optional peer dependency, not a runtime dependency (this only affected SMOODEV-2513: Add connect timeout to TS fetch SDK (default-off) #92). Putting a full HTTP stack in every consumer's tree — browser bundles included — for an opt-in Node-only feature ran against the runtime-dependency eviction in Evict two runtime dependencies from a published HTTP client #101. It now follows the @opentelemetry/api pattern the package already uses.
  • Requesting a connect timeout with undici absent 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 now read their knobs (black-hole URL, connect timeout, whole timeout, elapsed ceiling) from spec/connect-timeout-corpus.json instead of each hard-coding them, so the thresholds cannot drift apart per language.

Closing in favour of #103.

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