Skip to content

fix(providers): pin transport policy and lift the 60s cap on Groq and Cerebras - #6306

Merged
waleedlatif1 merged 6 commits into
stagingfrom
fix/provider-transport-defaults
Aug 6, 2026
Merged

fix(providers): pin transport policy and lift the 60s cap on Groq and Cerebras#6306
waleedlatif1 merged 6 commits into
stagingfrom
fix/provider-transport-defaults

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

Centralises provider transport policy in providers/transport.ts and applies it across all 18 SDK-backed providers.

  • Groq and Cerebras: 60s → 600s. Real behaviour change and the reason this PR exists. Both SDKs default to a 60s time-to-headers budget. On a non-streaming call headers do not arrive until the generation completes, so that caps any generation at 60 seconds — then silently retries it twice, re-billing each time. Neither vendor's latency justifies it; it is an OpenAI-template artifact.
  • 16 OpenAI-compatible providers: pinned, no runtime delta. They already inherit 600s / 2 from openai@7. Stamping it explicitly means an SDK bump cannot move production behaviour silently.

What this deliberately does NOT do

It does not add retries, and that is the finding — not an omission. My working assumption going in was that Sim's zero-retry OpenAI path should adopt the vendor's maxRetries: 2. A 25-provider integration audit rejected that, and the reasoning holds: a chat completion is non-idempotent, carries no idempotency key, and on the non-streaming path the response only exists once the generation is already billed. A replay therefore re-bills completed work — multiplied by every turn of the tool loop. The vendor default is kept and pinned; no hand-rolled loop is introduced.

Also rejected, each for a specific reason: dropping 409 (discards x-should-retry: false, which proxies use to shed load — amplifying the outage the header exists to damp); swapping to full jitter (subtractive jitter is a 25% spread vs backoffWithJitter's 40% — buys nothing); clamping Retry-After (the sleep is inside node_modules/openai/client.js:580-607 and unreachable from Sim); and memoising clients (retains plaintext tenant API keys in a global LRU for 30 idle minutes, and openai@7 uses a process-global connection pool anyway, so it saves no handshake).

It also does not raise a number to fix the production stalls, because that cannot work. Bun's ~300s fetch wall is socket-scoped and reachable from no SDK, undici Agent, or RequestInit option — the audit measured that Bun ignores Agent timeouts entirely (Node throws UND_ERR_HEADERS_TIMEOUT at 1007ms with headersTimeout: 800; Bun returns OK at 3013ms). The 279s/296s failures are that wall, reduced by however long a pooled socket sat idle before reuse — which is why 279 and not 300. Raising a timeout above it changes nothing.

Type of Change

  • Bug fix

Testing

4 tests pinning policy against the vendored SDK rather than a remembered number — PROVIDER_HEADERS_TIMEOUT_MS is asserted equal to OpenAI.DEFAULT_TIMEOUT, so an SDK bump fails the test instead of silently drifting. Verified fail-detectable by drifting the constant (2 red).

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

Follow-up (deliberately not in this PR)

The audit's central conclusion is that keeping bytes on the socket is the only remedy for a long silent generation. That means a stream idle/inter-chunk watchdog in stream-pump.ts plus abort threading through the three stream helpers, so a stalled stream unwinds its socket instead of leaking it (today return() queues behind the pending next() and never runs). That is the highest-value remaining change and the most delicate — it touches every provider's stream path and warrants its own PR and its own review.

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 3:40am

Request Review

@cursor

cursor Bot commented Aug 6, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Wide provider client surface changes timeout/retry behavior (real runtime change on Groq/Cerebras); guardrails cancellation semantics affect workflow guardrail consumers.

Overview
Introduces providers/transport.ts as the shared OpenAI-compatible client policy (600s time-to-headers, 2 max retries) and applies openAICompatTransport() across 18 SDK-backed providers. For most OpenAI-compat clients this pins existing openai@7 defaults; for Groq and Cerebras it raises the effective budget from 60s so long non-streaming generations are not cut off and re-billed via silent retries.

Guardrails now forwards request.signal into hallucination LLM scoring and treats AbortError as client cancellation (HTTP 499, no passed: false verdict), with matching route and validator tests.

Adds transport.test.ts asserting constants match vendored SDK defaults and that options reach constructed clients. Minor editor Text sub-block Tailwind class reorder only.

Reviewed by Cursor Bugbot for commit eeb262e. Configure here.

@greptile-apps

greptile-apps Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR centralizes timeout and retry settings for OpenAI-compatible provider clients, raising Groq and Cerebras timeouts while explicitly preserving the existing policy elsewhere. It also propagates request cancellation through hallucination guardrail scoring and preserves cancellation as a 499 response.

  • Adds shared provider transport constants and applies them across 18 SDK-backed providers.
  • Tests effective timeout and retry settings on constructed OpenAI, Groq, and Cerebras clients.
  • Forwards abort signals through hallucination validation and distinguishes cancellation from a failed guardrail verdict.
  • Normalizes Tailwind utility ordering in the text sub-block component.

Confidence Score: 5/5

The PR appears safe to merge.

The previously reported retry drift test now compares the policy against the vendored OpenAI retry default, and no blocking failure remains.

Important Files Changed

Filename Overview
apps/sim/providers/transport.ts Defines the shared 600-second timeout and two-retry transport policy used by OpenAI-compatible clients.
apps/sim/providers/transport.test.ts The revised assertion compares the pinned retry count with the vendored OpenAI default, resolving the prior drift-detection concern.
apps/sim/providers/groq/index.ts Applies the shared transport policy to Groq, replacing its shorter SDK timeout.
apps/sim/providers/cerebras/index.ts Applies the shared transport policy to Cerebras, replacing its shorter SDK timeout.
apps/sim/lib/guardrails/validate_hallucination.ts Threads cancellation into provider scoring and rethrows abort errors instead of converting them into guardrail failures.
apps/sim/app/api/guardrails/validate/route.ts Passes the request signal into hallucination validation and returns 499 for client cancellation.

Reviews (4): Last reviewed commit: "fix(guardrails): return 499 on a cancell..." | Re-trigger Greptile

Comment thread apps/sim/providers/transport.test.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

Final audit — 49 agents, 18 providers, adversarially verified

Verdict: ship with fixes. Zero dead changes — the failure mode I was most worried about does not occur. All 18 spreads land on constructors that genuinely read timeout/maxRetries: 16 → openai@7 (client.js:163,173), groq → groq-sdk/index.d.ts:31,52, cerebras → cerebras_cloud_sdk/index.d.ts:30,51. No call site clobbers the spread in either direction.

Fixed in 6a82d4109

  1. Dropped PROVIDER_DISCOVERY_TIMEOUT_MS — it had zero production consumers. Shipping a tested-but-inert policy constant is worse than not shipping it.
  2. Tests now construct real clients and read the value back, which guards a genuine silent failure: object spread gets no excess-property checking, so a renamed option in either SDK would become a no-op with a green typecheck.
  3. Corrected the TSDoc. It claimed "two are deliberate divergences, marked below" and marked nothing, and claimed the value was "behaviour-preserving for every provider already on that client" — false for Groq and Cerebras. Also corrected two of my own overstatements: Bun's fetch is native and does not route through undici at all, so my "no undici Agent can reach it" was a category error; and the ~300s wall is now labelled observed-but-unproven rather than asserted as fact.

Still open — a test gap I am not claiming to have closed

The audit's prescribed tests (which I implemented) verify the option names reach each SDK. They do not fail if someone deletes ...openAICompatTransport() from a provider file — I checked by deleting it from groq/index.ts and the suite stayed green. Closing that properly needs per-provider construction tests with 18 mocked SDKs. Flagging rather than papering over.

Side effects of the Groq/Cerebras raise — reviewer should weigh these

Retries fire on timeouts (groq-sdk/core.js:302-307 checks retriesRemaining before the AbortError branch), so it is 3 attempts:

per attempt worst case
Before 60s 181.5s
After, real (transport wall ~300s) ~300s 901.5s (~15 min), 5.0x
  • Free sync = 300s (execution-limits/types.ts): a single failing attempt now consumes the whole budget, so free users get "Execution timed out after 5 minutes" instead of a block-level provider error. Sharpest regression.
  • Pro/team sync = 3000s: two sequential failing agent blocks (3,603s) blow it. Async = 5400s: three (5,404.5s) exceed it, and trigger.config.ts:53 maxDuration: 5400 has zero headroom.
  • Error classification regresses. At 60s the SDK aborted itself → APIConnectionTimeoutError. At the transport wall it rewraps into APIConnectionError ("Connection error"), and isTransportTimeout() in agent-handler.ts:82-88 matches only AbortError/TimeoutError — so a 15-minute stall is no longer classified as a timeout, undoing part of what shipped in v0.7.57.
  • Guardrails become unbounded. lib/guardrails/validate_hallucination.ts:170 calls executeProviderRequest with no abortSignal (contrast agent-handler.ts:1274). Previously capped at 181.5s by the SDK; now it can run 15–30 min and keeps running after the workflow aborts. The audit flagged this as promotable to a blocker if Groq/Cerebras are used in guardrails.
  • Throughput: at WORKFLOW_EXECUTION_CONCURRENCY_LIMIT = 75, a Groq brownout holds each slot 5–10x longer (~25 runs/min → 2.5–5).

Mitigation available without dropping the fix: maxRetries: 1 for those two so a stall is not tripled, or a per-request timeout at their create() sites.

Doc alignment

Nothing contradicts the policy outright. Worth knowing: Baseten documents a 1200s read timeout — our 600s pin is half their recommendation. Groq documents 1 min and "retried twice"; Mistral's own SDK bounds the same endpoint at 300s. Aligned and clean: deepseek, xai, kimi, nvidia, zai, fireworks, openrouter, litellm (their own example is request_timeout: 600), ollama, ollama-cloud, vllm.

Consistency

Three genuine misses, all one-liners and provably no-op since their SDK defaults are byte-identical to the pinned values: anthropic (index.ts:27) plus the two Azure paths. Leaving google/vertex/bedrock/openai-raw-fetch is correctly deferred — they need different fixes.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

Wired the abort signal into hallucination scoring — route.ts:288 now forwards request.signal through executeValidationvalidateHallucinationscoreHallucinationWithLLMexecuteProviderRequest, matching how agent-handler.ts:1274 forwards ctx.abortSignal.

That closes the side effect the audit flagged as promotable to a blocker: the scoring request previously had no signal at all, so it outlived a cancelled request and kept burning a provider slot until the transport gave up — which the Groq/Cerebras raise would have stretched from ~181s to potentially 15+ minutes.

The free-tier budget consequence is being kept as-is by decision: a single failing Groq attempt now consumes the whole 300s sync budget rather than failing at 60s. That makes Groq consistent with every other provider rather than uniquely fast-failing, and the alternative — leaving a 60s cap on any generation — is the bug this PR exists to fix.

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/guardrails/validate_hallucination.ts
…port-defaults

# Conflicts:
#	apps/sim/app/api/guardrails/validate/route.ts
#	apps/sim/lib/guardrails/validate_hallucination.ts
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/app/api/guardrails/validate/route.ts
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@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.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit eeb262e. Configure here.

@waleedlatif1
waleedlatif1 merged commit 721b471 into staging Aug 6, 2026
30 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/provider-transport-defaults branch August 6, 2026 03:58
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