-
Notifications
You must be signed in to change notification settings - Fork 3.3k
feat(timeout): add API block timeout configuration #3053
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
426ccb2 to
fe99521
Compare
fe99521 to
64f7b84
Compare
Greptile OverviewGreptile SummaryThis PR fixes timeout configuration for API blocks by adding support for internal routes and fixing type coercion issues. Key Changes:
Additional Changes:
The timeout implementation correctly handles both internal and external routes, with proper error handling and cleanup. Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as API Block UI
participant Block as api.ts
participant Utils as tools/utils.ts
participant Index as tools/index.ts
participant Fetch as fetch/secureFetch
UI->>Block: User sets timeout (string)
Block->>Utils: formatRequestParams(tool, params)
Note over Utils: Convert string to number<br/>Validate timeout > 0
Utils->>Index: executeToolRequest(requestParams)
alt Internal Route (/api/*)
Note over Index: Create AbortController
Index->>Index: setTimeout(timeout || 300000)
Index->>Fetch: fetch(url, { signal })
alt Timeout occurs
Index->>Index: controller.abort()
Fetch-->>Index: AbortError
Index-->>Block: Error: "Request timed out after Xms"
else Success
Fetch-->>Index: Response
Index->>Index: clearTimeout()
Index-->>Block: Response data
end
else External URL
Index->>Fetch: secureFetchWithPinnedIP(url, { timeout })
Note over Fetch: Native timeout support<br/>via requestOptions.timeout
alt Timeout occurs
Fetch-->>Index: Timeout error
Index-->>Block: Error
else Success
Fetch-->>Index: Response
Index-->>Block: Response data
end
end
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5 files reviewed, 1 comment
ec06ab6 to
e2023dc
Compare
Negative timeout values would cause immediate request abort since JavaScript treats negative setTimeout delays as 0.
970228e to
d57fa9f
Compare
|
@greptile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, no comments
...im/app/workspace/[workspaceId]/w/components/sidebar/components/search-modal/search-modal.tsx
Show resolved
Hide resolved
...im/app/workspace/[workspaceId]/w/components/sidebar/components/search-modal/search-modal.tsx
Show resolved
Hide resolved
|
@greptile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 1 comment
- Internal routes now use same 5-minute default as external routes - Added Number.isFinite() check to reject Infinity values
accae9b to
8104f29
Compare
|
@greptile |
- Clamp timeout to max 600000ms (10 minutes) as documented - External routes now report timeout value in error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, no comments
...im/app/workspace/[workspaceId]/w/components/sidebar/components/search-modal/search-modal.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
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 and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| ? Math.min(timeout, MAX_TIMEOUT_MS) | ||
| : undefined | ||
|
|
||
| return { url, method, headers, body, timeout: validTimeout } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timeout parameter incorrectly applied to all tools
Medium Severity
The new timeout handling in formatRequestParams extracts params.timeout for ALL tools and uses it as the HTTP request timeout. However, tools like firecrawl, apify, and elasticsearch have their own timeout parameter that serves a different purpose—it's an API-level parameter (e.g., how long firecrawl should spend scraping), not an HTTP request timeout. When a user sets timeout=60000 for firecrawl intending 60 seconds of scrape time, the HTTP request will now also abort after 60 seconds, potentially failing before the API completes its work and returns a response.
Additional Locations (1)
| }) | ||
| // Set up AbortController for timeout support on internal routes | ||
| const controller = new AbortController() | ||
| const timeout = requestParams.timeout || 300000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated magic number for default timeout value
Low Severity
The magic number 300000 (5 minute timeout) is duplicated in two locations added by this PR. The codebase already has centralized timeout constants like A2A_DEFAULT_TIMEOUT in lib/a2a/constants.ts and CHAT_REQUEST_TIMEOUT_MS in app/chat/constants.ts with the same value. A shared constant like DEFAULT_REQUEST_TIMEOUT_MS would ensure consistency and make future changes easier to manage across the codebase.
Additional Locations (1)
| nativeInputValueSetter.call(inputRef.current, '') | ||
| inputRef.current.dispatchEvent(new Event('input', { bubbles: true })) | ||
| } | ||
| inputRef.current.focus() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overly complex DOM manipulation to clear input
Low Severity
The new approach uses Object.getOwnPropertyDescriptor(window.HTMLInputElement.prototype, 'value')?.set to clear the search input, which is more complex than the previous controlled React state pattern (setSearch('')). This DOM manipulation technique relies on browser implementation details and is harder to understand and maintain than standard React state management.
Summary
Fixes timeout configuration not being honored for API blocks and adds proper validation.
Changes
1. Internal Routes Timeout Support (
apps/sim/tools/index.ts)AbortControllerto support timeouts for internal routes (/api/*)2. Type Coercion Fix (
apps/sim/tools/utils.ts)short-inputsubBlock returns a string but code expected a numberNumber()with validation3. Timeout Validation (
apps/sim/tools/utils.ts)Infinity, andNaN4. Consistent Error Messages (
apps/sim/lib/core/security/input-validation.ts)Request timed out after Xms5. Cleanup (
apps/sim/tools/index.ts)clearTimeoutin catch block (finally always executes)Test Plan
Fixes #2786
Fixes #2242