Skip to content

fix(providers): bound a non-streaming request with an explicit 10-minute deadline - #6304

Open
waleedlatif1 wants to merge 1 commit into
stagingfrom
fix/provider-request-deadline
Open

fix(providers): bound a non-streaming request with an explicit 10-minute deadline#6304
waleedlatif1 wants to merge 1 commit into
stagingfrom
fix/provider-request-deadline

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

Sim sets no request timeout on provider calls, so every one inherits whatever the runtime imposes. Under Bun that is an undocumented ~300s idle timer — half of OpenAI's own documented default, chosen by nobody, and not configurable.

This sets an explicit 600,000 ms deadline on non-streaming provider requests, matching the vendor default verbatim: node_modules/openai/client.d.ts:179[opts.timeout=10 minutes].

Why 600s and not a number I made up

It is the OpenAI client's own default, in the SDK version this repo vendors. Both measured production failures — 295,823 ms and 278,920 ms, on gpt-5.4 and gpt-5.6-luna — were generations still in progress, not stalled connections. Both would have completed under the vendor default.

This is not hypothetical or one customer: four models across two providers (OpenAI and Gemini), and the Gemini case recurs on the same workflow roughly daily for at least two weeks.

Streaming is deliberately excluded

I measured Bun's default with a raw-TCP probe on 1.3.14, the production version:

trickle (a byte every 10s): COMPLETED after 330,054ms
silent  (zero bytes):       TimeoutError after 300,015ms

The timer is idle-based — any received byte resets it. So a streaming response, which emits continuously, is already bounded correctly by the runtime, and a total deadline there would cut off a long answer still arriving normally. The carve-out keys on payload.stream === true, i.e. the request's own semantics.

Caveat recorded honestly: the probe is HTTP/1.1 plaintext to localhost, and production is h2 over TLS. It explains the mechanism but does not explain why the two production failures fired early (4.2s and 21.1s before 300s). Leading hypothesis is that Bun's idle timer is connection-anchored and not reset on reuse of a pooled h2 connection, which would predict exactly those margins. Unproven. It does not change this fix — the runtime timer is demonstrably not anchored to our request, which is the reason to arm our own.

No regressions

  • The caller's abortSignal is preserved via AbortSignal.any, so a user pressing Stop still wins.
  • AbortSignal.timeout aborts with a TimeoutError, which the existing phase annotation already classifies — diagnostics keep working unchanged.
  • The constant lives in its own providers/timeouts.ts rather than the @/providers barrel. That barrel is replaced wholesale by vi.mock in 21 test files, so an export added there resolves to undefined in all of them. I hit this: the first version broke 30 tests across 4 files.

Type of Change

  • Bug fix

Testing

4 tests: the constant matches the vendor default, a deadline is armed on non-streaming, no deadline on streaming, and the caller signal still aborts. Each verified fail-detectable by breaking it and watching it go red.

Providers + agent-handler suites: 110 files / 1420 tests passing. Typecheck, lint, and check:api-validation clean.

Follow-up

A survey of default timeout and retry policy across all 25 providers is running. Sim also performs zero retries on the OpenAI path, where the vendor client does 2 with exponential backoff on 408/409/429/5xx and connection errors — including retrying timeouts. That gap is bigger day-to-day than this one and is the natural next PR.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel

vercel Bot commented Aug 6, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 6, 2026 2:11am

Request Review

@cursor

cursor Bot commented Aug 6, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes timeout behavior on a critical provider path; streaming is intentionally excluded and user abort is preserved, but misclassification of stream payloads could still affect long runs.

Overview
Non-streaming OpenAI Responses fetch calls now use an explicit 10-minute deadline (PROVIDER_REQUEST_TIMEOUT_MS in providers/timeouts.ts), aligned with the OpenAI SDK default, instead of inheriting Bun’s undocumented ~300s idle limit that was cutting off long generations still in progress.

withRequestDeadline in openai/core.ts applies AbortSignal.timeout for non-streaming payloads only; when stream: true, the request keeps the caller’s signal alone so long streaming answers are not capped by a total timeout. Caller abort (e.g. Stop) is merged via AbortSignal.any when both apply.

New tests in core.deadline.test.ts cover the constant, non-streaming vs streaming signal behavior, and caller abort precedence.

Reviewed by Cursor Bugbot for commit db40573. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR gives non-streaming OpenAI Responses requests an explicit ten-minute deadline while preserving caller cancellation and leaving streaming requests governed by idle timeout behavior.

  • Adds a shared 600,000 ms provider request-timeout constant.
  • Combines the deadline with caller-provided abort signals for non-streaming requests.
  • Adds tests covering deadline selection, streaming exclusion, and caller cancellation.

Confidence Score: 5/5

The PR appears safe to merge with no actionable correctness or security issues identified.

The deadline is applied to every reachable non-streaming Responses request, streaming payloads consistently bypass it, caller cancellation remains composed into the resulting signal, and the pinned server runtime supports the APIs used.

Important Files Changed

Filename Overview
apps/sim/providers/openai/core.ts Applies the explicit deadline at the shared Responses fetch boundary and correctly distinguishes literal streaming payloads.
apps/sim/providers/timeouts.ts Defines and documents the ten-minute non-streaming provider-request timeout.
apps/sim/providers/openai/core.deadline.test.ts Covers the timeout constant, non-streaming signal creation, streaming exclusion, and composition with caller cancellation.

Reviews (1): Last reviewed commit: "fix(providers): bound a non-streaming re..." | Re-trigger Greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

Correction from the 25-provider survey (73 agents, adversarially verified)

The number is right; my characterisation of it was not. Eight "600000ms" claims failed adversarial verification — in every case the value was exact and the scope was wrong. The SDK's 600s is per-attempt and time-to-headers: the timer is armed before fetch and cleared in a finally the moment headers arrive (openai/client.js:531-556, @anthropic-ai/sdk/client.js:661-673 — literally "Arm the timeout around the underlying fetch only"). With maxRetries=2 the vendor's real non-streaming ceiling is ~3 × 600s ≈ 30 min, and streaming bodies are bounded by nothing in any SDK surveyed.

This PR arms a total deadline (AbortSignal.timeout) covering headers and body. For the non-streaming Responses API the two are nearly identical — headers only arrive when generation completes, and I measured the subsequent body read at 7ms — so the fix behaves as intended. But the PR body's "matching the vendor default verbatim" overstates it: the number matches, the scope does not. Recording that rather than quietly leaving it.

What the survey found that this PR does not fix

  • Google / Vertex ship no timeout and no retries at all. @google/genai is constructed with no httpOptions, so apiCall is a bare fetch (dist/index.mjs:8355-8357). That is the direct explanation for the 5 Gemini TimeoutErrors found in 14 days of production logs, recurring on one workflow roughly daily. Untouched by this PR.
  • Bedrock also has no request timeout (defaultsMode:"legacy"DEFAULT_REQUEST_TIMEOUT=0), though it has by far the best retry policy in the table: full jitter plus a retry-quota token bucket, which no other client has.
  • Azure OpenAI is internally inconsistent — Chat Completions goes through the SDK (600s / 2 retries), while the Responses API goes through this same raw-fetch path (previously 0 / 0). This PR fixes the Responses half by construction.
  • 19 providers construct new OpenAI({ baseURL }) with no overrides, so they already inherit 600s / 2 retries. The gap was never platform-wide — it is specifically our raw-fetch paths plus Google/Vertex and Bedrock.

Where I disagree with the survey's own recommendation

It proposes a 60s TTFB deadline, on the grounds that 60s is the tightest vendor consensus (Groq, Cerebras, Together, Fireworks, Mistral). For streaming that is sound — TTFB genuinely means time-to-first-token.

For non-streaming it would be a serious regression: headers don't arrive until generation completes, so a 60s TTFB caps every non-streaming generation at 60 seconds. Both production failures we're fixing here ran 279s and 296s and were legitimate long generations ("Build Story Bible", "Build Slide Render Specification"). A 60s cap would fail them faster rather than let them finish. The survey flags this in its own risks section but still carries 60s into the headline recommendation.

I'd keep 600s for non-streaming as shipped here, and treat 60s TTFB as a streaming-only control alongside the stream-idle deadline — which is the genuinely novel finding, since no vendor implements an inter-chunk timeout at all.

Confirmed as the bigger gap

Retry is near-unanimous at maxRetries=2 across 8 independent vendors, on connection errors / 408 / 429 / 5xx. Sim does zero on this path. That remains the larger day-to-day deviation and the natural follow-up.

Two traps the survey correctly refused: a widely-repeated "Z.ai 30s/3 retries" traces to a third-party MCP server's config, not Z.ai; and Sakana's stream_idle_timeout_ms is a Codex CLI config key, not an API default. Neither should be adopted.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit db40573. Configure here.

headers: config.headers,
body: JSON.stringify(payload),
signal: abortSignal,
signal: withRequestDeadline(abortSignal, payload.stream === true),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Deadline ignored by Bun idle timer

High Severity

On Bun 1.3.14, AbortSignal.timeout does not outrank the runtime's ~300s idle fetch limit, so silent non-streaming generations can still die around five minutes—the failure mode this change aims to fix. The new 600s deadline never becomes the effective bound unless that idle timer is disarmed on the request.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit db40573. Configure here.

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