Skip to content

Commit 623003e

Browse files
authored
fix(providers): name the failing phase of a stalled OpenAI call, and reject a failed generation (#6283)
* fix(providers): name the failing phase of a stalled OpenAI call, and reject a failed generation An agent block hung ~4.5 minutes with an empty trace and surfaced only the runtime's own `TimeoutError: The operation timed out.` The cause was a runaway generation: the model repeated one tool call until it consumed the whole 128,000-token output budget, which takes minutes, and `/v1/responses` withholds its 200 until generation finishes — so the client waited, bounded only by an undocumented runtime socket deadline, and gave up before the response existed. Nothing in the trace could distinguish that from a request the provider never answered, or from one whose body never arrived. - Name the phase a transport failure died in — `awaiting-response-headers` vs `reading-response-body` — with status, ttfb, content-length and `x-request-id`. undici draws the same line as two error types (UND_ERR_HEADERS_TIMEOUT / UND_ERR_BODY_TIMEOUT); the OpenAI SDK captures `x-request-id` for the same reason. It rides the error message because that reaches the trace span, which survives when a task stops shipping logs. - Carry the cause through `ProviderError` so a transport timeout still classifies after wrapping overwrites `name`. - Reject a 200 that reports a failed or unusable generation instead of returning empty content with billed tokens, and stop truncated tool calls from executing. Matches `streamResponsesTurn`, which already did this, and `@ai-sdk/openai`, which throws on the same condition. - Bound non-JSON error bodies so a gateway error page cannot become the user-facing block error. Deliberately not included: a response-body deadline (the observed failure is in the headers phase, and the body transfers in ~1ms) and status-based retries (worth doing, unrelated to this, and separable). * test(providers): pin that a structured provider error survives the error-body bound * chore(providers): trim comments to the non-obvious why * fix(providers): let a deadline while reading an error body propagate * fix(providers): name the body phase when an error body read fails
1 parent 5718def commit 623003e

8 files changed

Lines changed: 811 additions & 21 deletions

File tree

.claude/rules/sim-ui-copy.md

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
---
2+
paths:
3+
- "apps/sim/**/*.tsx"
4+
- "apps/sim/components/emcn/**"
5+
---
6+
7+
# UI Copy
8+
9+
**Do not add subtitles, helper text, or descriptive copy beneath headings, labels, cards, or settings by default.** Prefer one concise, self-explanatory heading or label. Only add supporting copy when the user explicitly asks for it, or when it is necessary to prevent misunderstanding or error — and never use it to restate the heading.
10+
11+
This applies to product surfaces: settings rows, modals, panels, cards, list rows, empty states, form fields, and section headers. Marketing surfaces (`app/(landing)`, docs) are governed by `constitution.md` instead.
12+
13+
**Carve-out — settings section metadata.** `SettingsNavigationItem.description` in `components/settings/navigation.ts` stays required, and `SettingsPanel` keeps rendering it as the page subtitle. Settings sections are reached through a nav list where the description is the only thing distinguishing adjacent sections, so it earns its place by the "prevents misunderstanding" test. Keep those descriptions verb-first and one line, per `sim-settings-pages.md`. Everything else on a settings page — inline `<p>` blurbs under section headings, field hints, modal bodies, row subtitles — follows the default rule above.
14+
15+
## The default is no description
16+
17+
```tsx
18+
// ✗ Bad — the subtitle restates the heading
19+
<h3>API Keys</h3>
20+
<p className='text-[var(--text-muted)] text-caption'>Manage your API keys.</p>
21+
22+
// ✗ Bad — decorative filler under a field label
23+
<ChipModalField title='Workspace name' hint='The name of your workspace.' />
24+
25+
// ✓ Good — the label carries the whole meaning
26+
<h3>API Keys</h3>
27+
<ChipModalField title='Workspace name' />
28+
```
29+
30+
If a heading needs a subtitle to be understood, the heading is wrong. Fix the heading — don't append a second line.
31+
32+
## When supporting copy earns its place
33+
34+
Keep (or add) a description only when it carries information the label cannot, and its absence would cause a mistake:
35+
36+
- **Irreversible or destructive consequences** — "Deleting this workspace removes every workflow and log. This cannot be undone."
37+
- **A non-obvious format, unit, or bound** — "Comma-separated. Max 50 domains.", "Cost per 1M input tokens."
38+
- **A security or access implication** — "This key is shown once and grants full workspace access."
39+
- **A state the user cannot otherwise see** — "Inherited from your organization's policy."
40+
- **Instructional copy that advances a flow** — "We sent a 6-digit code to you@example.com."
41+
42+
Everything else — restatements, "Manage your X", "Configure your Y", feature blurbs, encouragement — gets deleted.
43+
44+
## Component APIs
45+
46+
Description/hint slots on shared components are **optional**, never required, and must reserve no layout space when omitted. A component that forces every consumer to supply a subtitle forces every consumer to violate this rule. When adding a new shared component, ship it without a description slot and add one only once a real caller meets the bar above.

.cursor/rules/sim-ui-copy.mdc

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
description: UI copy conventions — no default subtitles or helper text under headings, labels, cards, or settings
3+
globs: ["apps/sim/**/*.tsx"]
4+
---
5+
# UI Copy
6+
7+
**Do not add subtitles, helper text, or descriptive copy beneath headings, labels, cards, or settings by default.** Prefer one concise, self-explanatory heading or label. Only add supporting copy when the user explicitly asks for it, or when it is necessary to prevent misunderstanding or error — and never use it to restate the heading.
8+
9+
This applies to product surfaces: settings rows, modals, panels, cards, list rows, empty states, form fields, and section headers. Marketing surfaces (`app/(landing)`, docs) are governed by `constitution.mdc` instead.
10+
11+
**Carve-out — settings section metadata.** `SettingsNavigationItem.description` in `components/settings/navigation.ts` stays required, and `SettingsPanel` keeps rendering it as the page subtitle. Settings sections are reached through a nav list where the description is the only thing distinguishing adjacent sections. Everything else on a settings page — inline `<p>` blurbs under section headings, field hints, modal bodies, row subtitles — follows the default rule above.
12+
13+
## The default is no description
14+
15+
```tsx
16+
// ✗ Bad — the subtitle restates the heading
17+
<h3>API Keys</h3>
18+
<p className='text-[var(--text-muted)] text-caption'>Manage your API keys.</p>
19+
20+
// ✗ Bad — decorative filler under a field label
21+
<ChipModalField title='Workspace name' hint='The name of your workspace.' />
22+
23+
// ✓ Good — the label carries the whole meaning
24+
<h3>API Keys</h3>
25+
<ChipModalField title='Workspace name' />
26+
```
27+
28+
If a heading needs a subtitle to be understood, the heading is wrong. Fix the heading — don't append a second line.
29+
30+
## When supporting copy earns its place
31+
32+
Keep (or add) a description only when it carries information the label cannot, and its absence would cause a mistake:
33+
34+
- **Irreversible or destructive consequences** — "Deleting this workspace removes every workflow and log. This cannot be undone."
35+
- **A non-obvious format, unit, or bound** — "Comma-separated. Max 50 domains.", "Cost per 1M input tokens."
36+
- **A security or access implication** — "This key is shown once and grants full workspace access."
37+
- **A state the user cannot otherwise see** — "Inherited from your organization's policy."
38+
- **Instructional copy that advances a flow** — "We sent a 6-digit code to you@example.com."
39+
40+
Everything else — restatements, "Manage your X", "Configure your Y", feature blurbs, encouragement — gets deleted.
41+
42+
## Component APIs
43+
44+
Description/hint slots on shared components are **optional**, never required, and must reserve no layout space when omitted. A component that forces every consumer to supply a subtitle forces every consumer to violate this rule. When adding a new shared component, ship it without a description slot and add one only once a real caller meets the bar above.

apps/sim/executor/handlers/agent/agent-handler.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -979,6 +979,49 @@ describe('AgentBlockHandler', () => {
979979
)
980980
})
981981

982+
/**
983+
* A stalled model call reaches here as the runtime's own `TimeoutError`, whose bare
984+
* message ("The operation timed out.") names nothing. It must become a Sim-level
985+
* message WITHOUT discarding the phase detail the provider attached — that detail is
986+
* the only thing distinguishing "never answered" from "body never completed".
987+
*/
988+
it('maps a provider TimeoutError to a Sim message while keeping the phase detail', async () => {
989+
const inputs = { model: 'gpt-4o', userPrompt: 'hi', apiKey: 'test-api-key' }
990+
mockGetProviderFromModel.mockReturnValue('openai')
991+
992+
// Faithful to production: providers rewrap the transport failure in a
993+
// ProviderError, which overwrites `name` — so only the cause still classifies it.
994+
const transport = new Error(
995+
'The operation timed out. [phase=reading-response-body elapsedMs=60001 status=200 contentLength=32116]'
996+
)
997+
transport.name = 'TimeoutError'
998+
const wrapped = new Error(transport.message, { cause: transport })
999+
wrapped.name = 'ProviderError'
1000+
mockExecuteProviderRequest.mockRejectedValueOnce(wrapped)
1001+
1002+
const error = await handler.execute(mockContext, mockBlock, inputs).catch((e) => e)
1003+
1004+
expect(error.message).toContain('Provider request timed out')
1005+
expect(error.message).toContain('phase=reading-response-body')
1006+
expect(error.message).toContain('status=200')
1007+
})
1008+
1009+
it('maps a provider AbortError the same way', async () => {
1010+
const inputs = { model: 'gpt-4o', userPrompt: 'hi', apiKey: 'test-api-key' }
1011+
mockGetProviderFromModel.mockReturnValue('openai')
1012+
1013+
const aborted = new Error('aborted [phase=awaiting-response-headers elapsedMs=12]')
1014+
aborted.name = 'AbortError'
1015+
const wrapped = new Error(aborted.message, { cause: aborted })
1016+
wrapped.name = 'ProviderError'
1017+
mockExecuteProviderRequest.mockRejectedValueOnce(wrapped)
1018+
1019+
const error = await handler.execute(mockContext, mockBlock, inputs).catch((e) => e)
1020+
1021+
expect(error.message).toContain('Provider request timed out')
1022+
expect(error.message).toContain('phase=awaiting-response-headers')
1023+
})
1024+
9821025
it('should handle streaming responses with text/event-stream content type', async () => {
9831026
const mockStreamBody = new ReadableStream({
9841027
start(controller) {

apps/sim/executor/handlers/agent/agent-handler.ts

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,22 @@ import { getToolAsync } from '@/tools/utils.server'
7171

7272
const logger = createLogger('AgentBlockHandler')
7373

74+
/**
75+
* True when a failure originated from a transport deadline or abort, at any depth of the
76+
* cause chain.
77+
*
78+
* Providers rewrap transport failures (`ProviderError` overwrites `name`), so a check on
79+
* the top-level `name` alone misses every wrapped case. Bounded to a short walk so a
80+
* self-referential cause cannot loop.
81+
*/
82+
function isTransportTimeout(error: unknown): boolean {
83+
for (let current = error, depth = 0; current instanceof Error && depth < 5; depth++) {
84+
if (current.name === 'AbortError' || current.name === 'TimeoutError') return true
85+
current = current.cause
86+
}
87+
return false
88+
}
89+
7490
/**
7591
* Handler for Agent blocks that process LLM requests with optional tools.
7692
*/
@@ -1299,8 +1315,15 @@ export class AgentBlockHandler implements BlockHandler {
12991315
timestamp: new Date().toISOString(),
13001316
})
13011317

1302-
if (error.name === 'AbortError') {
1303-
throw new Error('Provider request timed out - the API took too long to respond')
1318+
/**
1319+
* The original message is appended rather than replaced: providers annotate it with
1320+
* the request phase they died in, which is the only thing separating a request that
1321+
* was never answered from one whose body stalled.
1322+
*/
1323+
if (isTransportTimeout(error)) {
1324+
throw new Error(
1325+
`Provider request timed out - the API took too long to respond (${error.message})`
1326+
)
13041327
}
13051328
if (error.name === 'TypeError' && error.message.includes('fetch')) {
13061329
throw new Error(

0 commit comments

Comments
 (0)