fix: increase upload timeout and wrap non-Error abort rejections#534
fix: increase upload timeout and wrap non-Error abort rejections#534taylorhou wants to merge 1 commit intoopenclaw:mainfrom
Conversation
…ance The publish command was timing out because REQUEST_TIMEOUT_MS (15s) was used for all requests including multipart form uploads, which can take much longer depending on skill size and server processing time. Two fixes: 1. Add UPLOAD_TIMEOUT_MS (120s) and use it in apiRequestForm and the corresponding curl code path for form uploads. 2. Wrap non-Error fetch rejections in fetchWithTimeout so pRetry always receives a proper Error instance. In some Node/undici versions, AbortController.abort(reason) can cause fetch to reject with the raw reason value. If that value is not an Error, pRetry surfaces the unhelpful message 'Non-error was thrown: Timeout' instead of a meaningful error. Fixes openclaw#533
|
Someone is attempting to deploy a commit to the Amantus Machina Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryIncreased upload timeout from 15s to 120s and ensured abort rejections are always wrapped in proper Error instances. The changes correctly address the two root causes of
The implementation is clean and follows existing patterns. However, the improved error message format ( Confidence Score: 4/5
Last reviewed commit: 938998d |
| // Ensure we always throw a proper Error instance. Some Node/undici versions | ||
| // propagate the abort reason as-is; if it is not an Error, pRetry will | ||
| // surface "Non-error was thrown" instead of a meaningful message. | ||
| controller.abort(new Error(`Request timed out after ${timeoutMs}ms`)) |
There was a problem hiding this comment.
Updated timeout error message will break existing tests in http.test.ts (lines 264 and 346) that expect .message to be exactly "Timeout". Update test assertions to match the new format:
| controller.abort(new Error(`Request timed out after ${timeoutMs}ms`)) | |
| controller.abort(new Error(`Request timed out after ${timeoutMs}ms`)) |
Then update http.test.ts:
// Line 264
expect((caught as Error).message).toContain('timed out')
// Line 346
expect((caught as Error).message).toContain('timed out')Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/clawdhub/src/http.ts
Line: 220
Comment:
Updated timeout error message will break existing tests in `http.test.ts` (lines 264 and 346) that expect `.message` to be exactly `"Timeout"`. Update test assertions to match the new format:
```suggestion
controller.abort(new Error(`Request timed out after ${timeoutMs}ms`))
```
Then update `http.test.ts`:
```typescript
// Line 264
expect((caught as Error).message).toContain('timed out')
// Line 346
expect((caught as Error).message).toContain('timed out')
```
How can I resolve this? If you propose a fix, please make it concise.
Problem
Fixes #533
clawhub publishconsistently times out with:Two root causes:
1. Timeout too short for form uploads
REQUEST_TIMEOUT_MS(15s) is applied uniformly to all requests including multipart form uploads inapiRequestForm. Uploading skill files to the server can easily exceed 15s depending on skill size and server-side processing time.2. Non-Error abort rejections not re-wrapped
In some Node.js/undici versions,
controller.abort(reason)causesfetchto reject with the raw reason value rather than wrapping it. If the reason is not anErrorinstance (or gets lost in propagation),pRetrysurfaces the unhelpfulNon-error was thrownmessage instead of the underlying cause.Fix
UPLOAD_TIMEOUT_MS = 120_000(2 min) and applied it to both the NodefetchWithTimeoutpath and the Bun/curl--max-timepath inapiRequestForm/fetchJsonFormViaCurl.fetchWithTimeout, wrap any non-Error rejection in a newErrorsopRetryalways receives a properErrorinstance.Testing
Verified locally that
clawhub whoamistill works and the timeout constants are wired correctly.