fix(core): add a default timeout for outbound HTTP requests - #614
Conversation
|
@yuvanvk is attempting to deploy a commit to the corsair Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe request configuration adds an optional timeout. ChangesRequest timeout handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant sendRequest
participant AbortSignal
participant fetch
Caller->>sendRequest: Start request with TIMEOUT
sendRequest->>AbortSignal: Combine timeout and caller signals
sendRequest->>fetch: Send request with combined signal
AbortSignal-->>fetch: Abort on timeout or caller cancellation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
yuvanvk
changed the title
Greptile SummaryThis PR adds a configurable wall-clock timeout to Corsair’s shared outbound HTTP path while retaining explicit caller cancellation.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant Request as request()
participant Fetch
Caller->>Request: Start outbound request
Request->>Fetch: "fetch(signal = cancellation OR timeout)"
alt Caller cancels
Caller->>Request: cancel()
Request-->>Fetch: Abort signal
Request-->>Caller: CancelError
else Timeout expires
Request-->>Fetch: Timeout abort signal
Fetch-->>Caller: Request rejected
else Response arrives
Fetch-->>Request: Response
Request-->>Caller: Parsed result
end
Reviews (2): Last reviewed commit: "Merge branch 'main' into fix/586-core" | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/corsair/async-core/request.ts (1)
234-234: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDeclare the minimum Node.js version. The repository requires Node.js 22+, and the Node build supports both APIs.
packages/corsair/package.jsonhas noenginesfield. Add"engines": { "node": ">=22" }, or provide a fallback if older runtimes are supported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/corsair/async-core/request.ts` at line 234, Add an engines declaration to packages/corsair/package.json requiring Node.js >=22, matching the AbortSignal.any usage in the request implementation. Do not add a fallback unless this package must support older Node.js runtimes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/corsair/async-core/request.ts`:
- Line 234: Update the request timeout handling around the AbortSignal
construction in the request flow to enforce config.TIMEOUT using elapsed
wall-clock time rather than relying solely on AbortSignal.timeout, which pauses
during suspension or bfcache. Preserve cancellation through controller.signal
and ensure the request aborts once the real elapsed duration reaches the
configured timeout.
In `@packages/corsair/tests/request-timeout.test.ts`:
- Around line 59-68: Update the caller-cancellation test around request(config,
options) to use a separate request configuration with a sufficiently long
timeout, ensuring the shared 5 ms timeout cannot fire before promise.cancel()
executes. Preserve the existing wait for the abort signal and the isCancelled
rejection assertion.
---
Nitpick comments:
In `@packages/corsair/async-core/request.ts`:
- Line 234: Add an engines declaration to packages/corsair/package.json
requiring Node.js >=22, matching the AbortSignal.any usage in the request
implementation. Do not add a fallback unless this package must support older
Node.js runtimes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da3a5f97-5cbe-4133-a50f-e077ace0ce64
📒 Files selected for processing (3)
packages/corsair/async-core/OpenAPI.tspackages/corsair/async-core/request.tspackages/corsair/tests/request-timeout.test.ts
| body: body ?? formData, | ||
| method: options.method, | ||
| signal: controller.signal, | ||
| signal: AbortSignal.any([controller.signal, AbortSignal.timeout(timeout)]), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
According to the WHATWG DOM specification or MDN, does AbortSignal.timeout() measure active time or elapsed wall-clock time when a document is suspended or in the back-forward cache?
💡 Result:
Yes, according to MDN and the underlying design of the WHATWG DOM specification, AbortSignal.timeout measures active time rather than wall-clock time [1][2]. When a document is suspended in the back-forward cache (bfcache) or when code is running in a suspended worker, the timer effectively pauses [1][2]. This means that the time spent in these suspended states does not count toward the timeout duration [3]. Consequently, if you set a timeout of 10 seconds, the signal will abort only after the document or worker has spent a total of 10 seconds in an active, non-suspended state [4][5].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout_static
- 2: https://github.com/mdn/content/blob/main/files/en-us/web/api/abortsignal/timeout_static/index.md
- 3: https://dev.to/solvodevnotes/abortcontroller-the-cancellation-bugs-most-javascript-devs-ship-34kn
- 4: https://github.com/whatwg/dom/pull/1032/files/4463234e4b3aa26c9d85fbcfb1c7eeafac672a96..71d00e69743d7d1cc8d9fe4d22ee964967f3f29d
- 5: Introduce AbortSignal.timeout() whatwg/dom#1032
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- request.ts structure ---'
ast-grep outline packages/corsair/async-core/request.ts --view compact
printf '%s\n' '--- request.ts relevant lines ---'
sed -n '190,255p' packages/corsair/async-core/request.ts
printf '%s\n' '--- timeout references ---'
rg -n --glob '!node_modules' 'TIMEOUT|AbortSignal\.(timeout|any)|wall.?clock|active.?time|timeout' packages/corsair package.json packages 2>/dev/null | head -200
printf '%s\n' '--- repository support configuration ---'
fd -HI -t f '(package\.json|tsconfig.*|browserslist|\.nvmrc|README.*|CHANGELOG.*)' . | head -200Repository: corsairdev/corsair
Length of output: 336
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- request.ts structure ---'
ast-grep outline packages/corsair/async-core/request.ts --view signatures
printf '%s\n' '--- request.ts relevant lines ---'
sed -n '190,255p' packages/corsair/async-core/request.ts
printf '%s\n' '--- timeout references ---'
rg -n --glob '!node_modules' 'TIMEOUT|AbortSignal\.(timeout|any)|wall.?clock|active.?time|timeout' packages/corsair package.json packages 2>/dev/null | head -200
printf '%s\n' '--- repository support configuration ---'
fd -HI -t f '(package\.json|tsconfig.*|browserslist|\.nvmrc|README.*|CHANGELOG.*)' . | head -200Repository: corsairdev/corsair
Length of output: 28137
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- package and runtime targets ---'
cat package.json
cat packages/corsair/package.json
cat .nvmrc
printf '%s\n' '--- TypeScript targets ---'
cat packages/corsair/tsconfig.json
cat packages/corsair/tsconfig.build.json
printf '%s\n' '--- timeout test ---'
cat -n packages/corsair/tests/request-timeout.test.ts
printf '%s\n' '--- request call path ---'
sed -n '320,390p' packages/corsair/async-core/request.tsRepository: corsairdev/corsair
Length of output: 10533
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- current change summary ---'
git status --short
git diff --stat
printf '%s\n' '--- relevant diff ---'
git diff -- packages/corsair/async-core/request.ts packages/corsair/tests/request-timeout.test.ts
printf '%s\n' '--- base compiler options ---'
cat tsconfig.base.json
printf '%s\n' '--- browser and client references ---'
rg -n 'async-core|CancelablePromise|OpenAPIConfig|request\(' packages/corsair/README.md packages/corsair/client packages/corsair --glob '!tests/**' | head -200Repository: corsairdev/corsair
Length of output: 6021
Implement a wall-clock timeout for requests. AbortSignal.timeout(timeout) measures active time. A suspended document or bfcache entry can therefore exceed config.TIMEOUT before the signal aborts. Use an elapsed-time strategy, or document active-time semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/corsair/async-core/request.ts` at line 234, Update the request
timeout handling around the AbortSignal construction in the request flow to
enforce config.TIMEOUT using elapsed wall-clock time rather than relying solely
on AbortSignal.timeout, which pauses during suspension or bfcache. Preserve
cancellation through controller.signal and ensure the request aborts once the
real elapsed duration reaches the configured timeout.
| const promise = request(config, options); | ||
|
|
||
| // Wait until fetch has started and has received its abort signal | ||
| while (!signal) { | ||
| await new Promise((res) => setTimeout(res, 0)); | ||
| } | ||
|
|
||
| promise.cancel(); | ||
|
|
||
| await expect(promise).rejects.toMatchObject({ isCancelled: true }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use a long timeout in the caller-cancellation test.
The shared 5 ms timeout starts before fetch() receives its signal. If the scheduler delays this test, the timeout can reject the promise before Line 66 calls promise.cancel(). Use a separate config with a long timeout so this test only verifies caller cancellation.
Proposed fix
- const promise = request(config, options);
+ const promise = request({ ...config, TIMEOUT: 60_000 }, options);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const promise = request(config, options); | |
| // Wait until fetch has started and has received its abort signal | |
| while (!signal) { | |
| await new Promise((res) => setTimeout(res, 0)); | |
| } | |
| promise.cancel(); | |
| await expect(promise).rejects.toMatchObject({ isCancelled: true }); | |
| const promise = request({ ...config, TIMEOUT: 60_000 }, options); | |
| // Wait until fetch has started and has received its abort signal | |
| while (!signal) { | |
| await new Promise((res) => setTimeout(res, 0)); | |
| } | |
| promise.cancel(); | |
| await expect(promise).rejects.toMatchObject({ isCancelled: true }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/corsair/tests/request-timeout.test.ts` around lines 59 - 68, Update
the caller-cancellation test around request(config, options) to use a separate
request configuration with a sufficiently long timeout, ensuring the shared 5 ms
timeout cannot fire before promise.cancel() executes. Preserve the existing wait
for the abort signal and the isCancelled rejection assertion.
There was a problem hiding this comment.
2000ms is good right instead of 5ms?
There was a problem hiding this comment.
@yuvanvk, yes. 2000ms is sufficient for this test and removes the practical race caused by 5ms.
A separate longer timeout is still preferable because it keeps the caller-cancellation test independent from the timeout path. However, 2000ms is a reasonable value if it is the configured default. The passing focused test output supports this change.
You are interacting with an AI system.
|
Core idea LGTM
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/corsair/tests/request-timeout.test.ts (1)
12-12: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse separate timeout values for each test.
TIMEOUT: 2_000makes the timeout test wait about two seconds. Keep a short timeout for the timeout test. Use a long timeout only in the caller-cancellation test. This prevents the cancellation race without slowing the focused test.Proposed change
- TIMEOUT: 2_000, + TIMEOUT: 50,- const promise = request(config, options); + const promise = request({ ...config, TIMEOUT: 60_000 }, options);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/corsair/tests/request-timeout.test.ts` at line 12, Update the timeout test setup in request-timeout.test.ts so each test uses its own timeout value instead of sharing TIMEOUT: 2_000. Keep the focused timeout test on a short timeout, and apply the longer timeout only in the caller-cancellation test path to avoid the race without slowing the other test. Use the existing test-case symbols in this file to separate the per-test timeout configuration cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/corsair/tests/request-timeout.test.ts`:
- Line 12: Update the timeout test setup in request-timeout.test.ts so each test
uses its own timeout value instead of sharing TIMEOUT: 2_000. Keep the focused
timeout test on a short timeout, and apply the longer timeout only in the
caller-cancellation test path to avoid the race without slowing the other test.
Use the existing test-case symbols in this file to separate the per-test timeout
configuration cleanly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1da37deb-d00a-451a-8f8f-a8aa0009dd49
📒 Files selected for processing (1)
packages/corsair/tests/request-timeout.test.ts
@Dhirenderchoudhary changed TIMEOUT from 5ms to 2 seconds |
|
@greptileai review |
ambikeesshh
left a comment
ambikeesshh
left a comment
There was a problem hiding this comment.
lgtm now. thanks @yuvanvk
4 participants
Description
Adds a default wall-clock timeout to Corsair’s shared HTTP request path.
Previously,
sendRequestcreated anAbortController, but fetch requests were only aborted when callers explicitly cancelled. A hung upstreamfetchcould keep the promise pending indefinitely. This change combines the existing cancellation signal with a timeout signal so requests abort automatically after the configured timeout while preserving caller cancellation.Changes include:
TIMEOUTsupport toOpenAPIConfigsendRequestFixes: #586
Checklist
Before submitting your PR, please verify the following:
pnpm lintand all checks passpnpm buildand all packages build successfullyScreenshots / Demos (if applicable)
Additional Notes
Verified the focused test with:
pnpm --filter corsair test -- request-timeout.test.tsSummary by CodeRabbit
Summary by CodeRabbit
New Features
TIMEOUT) with a 20-second default.Bug Fixes
Tests