fix(api): pass abortSignal to streaming API calls for all providers (#404)#434
Open
easonLiangWorldedtech wants to merge 2 commits into
Open
Conversation
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds optional ChangesAbort Signal Support for Request Cancellation
Sequence DiagramsequenceDiagram
participant User
participant Task
participant ApiHandler
participant Provider
participant HTTP
User->>Task: start request / later click stop
Task->>Task: create AbortController -> signal
Task->>ApiHandler: createMessage(metadata includes abortSignal)
ApiHandler->>Provider: start streaming call with options including signal
Provider->>HTTP: open streaming HTTP request using provided signal
User->>Task: stop -> controller.abort()
HTTP-->>Provider: connection aborted (by signal)
Provider-->>ApiHandler: stream terminated / error propagated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/api/providers/__tests__/openai-compatible-abort-signal.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
easonLiangWorldedtech
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/providers/openai.ts (1)
247-250: ⚡ Quick winMain non-streaming path omits
abortSignal.The O3 non-streaming flow (Line 408) and both streaming flows forward
metadata?.abortSignal, but this main non-streaming request still passes onlypath. For consistency and to cancel non-streaming requests too, forward the signal here as well.♻️ Proposed change
- response = await this.client.chat.completions.create( - requestOptions, - this._isAzureAiInference(modelUrl) ? { path: OPENAI_AZURE_AI_INFERENCE_PATH } : {}, - ) + response = await this.client.chat.completions.create(requestOptions, { + ...(this._isAzureAiInference(modelUrl) ? { path: OPENAI_AZURE_AI_INFERENCE_PATH } : {}), + signal: metadata?.abortSignal, + })🤖 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 `@src/api/providers/openai.ts` around lines 247 - 250, The non-streaming call to this.client.chat.completions.create omits the abort signal; update the call site that currently passes only { path: OPENAI_AZURE_AI_INFERENCE_PATH } so it also forwards metadata?.abortSignal (or include abortSignal when not undefined) alongside path when _isAzureAiInference(modelUrl) is true, and ensure the same abortSignal is passed in the alternate (non-Azure) branch as well so requestOptions and the signal are both honored by this.client.chat.completions.create.
🤖 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 `@src/api/providers/openai.ts`:
- Around line 247-250: The non-streaming call to
this.client.chat.completions.create omits the abort signal; update the call site
that currently passes only { path: OPENAI_AZURE_AI_INFERENCE_PATH } so it also
forwards metadata?.abortSignal (or include abortSignal when not undefined)
alongside path when _isAzureAiInference(modelUrl) is true, and ensure the same
abortSignal is passed in the alternate (non-Azure) branch as well so
requestOptions and the signal are both honored by
this.client.chat.completions.create.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a5124a34-2d10-49c1-a063-7e83f8ab99f7
📒 Files selected for processing (18)
src/api/index.tssrc/api/providers/__tests__/openai-compatible-abort-signal.spec.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/deepseek.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/mimo.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai.tssrc/api/providers/opencode-go.tssrc/api/providers/openrouter.tssrc/api/providers/qwen-code.tssrc/api/providers/requesty.tssrc/api/providers/unbound.tssrc/api/providers/vercel-ai-gateway.tssrc/api/providers/zai.tssrc/core/task/Task.tssrc/core/task/__tests__/task-abort-signal-passing.spec.ts
…oo-Code-Org#404) When user clicks stop during streaming, the HTTP request continues running because Vercel AI SDK's streamText() doesn't receive an abort signal. This wastes API tokens/compute on the provider side. Changes: - Add abortSignal?: AbortSignal to ApiHandlerCreateMessageMetadata interface - Pass Task.ts's AbortController.signal through metadata to createMessage() - Use signal in all providers' streamText() options
easonLiangWorldedtech
force-pushed
the
fix/abort-signal-openai-compatible
branch
from
ef0a6de to
ee47fbb
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/api/providers/__tests__/openai-compatible-abort-signal.spec.ts (3)
73-82: ⚡ Quick winRemove unused helper function and variable.
The
createMockStreamfunction (lines 73-80) andmockUsagevariable (line 82) are defined but never used in this test. The mock is set up directly on line 84-87 instead.♻️ Suggested cleanup
- function createMockStream(yieldValue: any) { - return { - fullStream: (async function* () { - yield yieldValue - })(), - usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), - } - } - - const mockUsage = Promise.resolve({ inputTokens: 10, outputTokens: 5 }) - mockStreamText.mockReturnValue({ fullStream: mockFullStream(), - usage: mockUsage, + usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), })🤖 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 `@src/api/providers/__tests__/openai-compatible-abort-signal.spec.ts` around lines 73 - 82, Remove the unused helper and variable: delete the createMockStream function and the mockUsage constant since they are never referenced in the test; ensure any tests still rely on the explicit mock set up later (the inline mock at lines that replace createMockStream) and run tests to confirm nothing else depends on createMockStream or mockUsage.
108-117: ⚡ Quick winRemove duplicate unused helper function.
The
createMockStreamfunction (lines 108-115) is defined but never used. This is a duplicate of the unused helper from the previous test.♻️ Suggested cleanup
- function createMockStream(yieldValue: any) { - return { - fullStream: (async function* () { - yield yieldValue - })(), - usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), - } - } - - const mockUsage = Promise.resolve({ inputTokens: 10, outputTokens: 5 }) - mockStreamText.mockReturnValue({ fullStream: mockFullStream(), - usage: mockUsage, + usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), })🤖 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 `@src/api/providers/__tests__/openai-compatible-abort-signal.spec.ts` around lines 108 - 117, The test contains a duplicate, unused helper function createMockStream (and its adjacent mockUsage Promise) that duplicates a helper from the previous test; remove the createMockStream declaration (and the unused mockUsage Promise if it is also unused) so the test file only keeps one shared helper and avoids dead code; search for createMockStream and mockUsage to ensure no usages remain before deleting.
142-151: ⚡ Quick winRemove third duplicate unused helper function.
Yet another
createMockStreamfunction (lines 142-149) that is defined but never called.♻️ Suggested cleanup
- function createMockStream(yieldValue: any) { - return { - fullStream: (async function* () { - yield yieldValue - })(), - usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), - } - } - - const mockUsage = Promise.resolve({ inputTokens: 10, outputTokens: 5 }) - mockStreamText.mockReturnValue({ fullStream: mockFullStream(), - usage: mockUsage, + usage: Promise.resolve({ inputTokens: 10, outputTokens: 5 }), })🤖 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 `@src/api/providers/__tests__/openai-compatible-abort-signal.spec.ts` around lines 142 - 151, Remove the duplicate, unused helper function createMockStream (the async generator returning fullStream and usage) from the test; leave the existing mockUsage constant in place if tests use it. Locate the duplicate createMockStream definition in the file (the one that returns { fullStream: (async function*(){ yield yieldValue })(), usage: Promise.resolve(...) }) and delete it so there is only the intended helper(s) remaining and no unused symbol shadowing.
🤖 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 `@src/api/providers/__tests__/openai-compatible-abort-signal.spec.ts`:
- Around line 73-82: Remove the unused helper and variable: delete the
createMockStream function and the mockUsage constant since they are never
referenced in the test; ensure any tests still rely on the explicit mock set up
later (the inline mock at lines that replace createMockStream) and run tests to
confirm nothing else depends on createMockStream or mockUsage.
- Around line 108-117: The test contains a duplicate, unused helper function
createMockStream (and its adjacent mockUsage Promise) that duplicates a helper
from the previous test; remove the createMockStream declaration (and the unused
mockUsage Promise if it is also unused) so the test file only keeps one shared
helper and avoids dead code; search for createMockStream and mockUsage to ensure
no usages remain before deleting.
- Around line 142-151: Remove the duplicate, unused helper function
createMockStream (the async generator returning fullStream and usage) from the
test; leave the existing mockUsage constant in place if tests use it. Locate the
duplicate createMockStream definition in the file (the one that returns {
fullStream: (async function*(){ yield yieldValue })(), usage:
Promise.resolve(...) }) and delete it so there is only the intended helper(s)
remaining and no unused symbol shadowing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: efeee00b-056f-4dc5-aadf-f93a7464e61f
📒 Files selected for processing (24)
packages/types/src/tool-params.tssrc/api/index.tssrc/api/providers/__tests__/fireworks.spec.tssrc/api/providers/__tests__/openai-compatible-abort-signal.spec.tssrc/api/providers/__tests__/sambanova.spec.tssrc/api/providers/__tests__/zai.spec.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/deepseek.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/mimo.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai.tssrc/api/providers/opencode-go.tssrc/api/providers/openrouter.tssrc/api/providers/qwen-code.tssrc/api/providers/requesty.tssrc/api/providers/unbound.tssrc/api/providers/vercel-ai-gateway.tssrc/api/providers/zai.tssrc/core/task/Task.tssrc/core/task/__tests__/task-abort-signal-passing.spec.tssrc/core/tools/ReadFileTool.tssrc/core/tools/__tests__/readFileTool.spec.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- src/api/index.ts
- src/api/providers/zai.ts
- src/api/providers/vercel-ai-gateway.ts
- src/api/providers/tests/sambanova.spec.ts
- src/api/providers/opencode-go.ts
- src/api/providers/unbound.ts
- src/api/providers/lite-llm.ts
- src/api/providers/lm-studio.ts
- src/api/providers/mimo.ts
- src/api/providers/tests/fireworks.spec.ts
- src/api/providers/tests/zai.spec.ts
- src/api/providers/deepseek.ts
- src/core/task/Task.ts
- src/api/providers/openrouter.ts
- src/api/providers/qwen-code.ts
- src/core/task/tests/task-abort-signal-passing.spec.ts
- src/api/providers/openai.ts
…tests (Zoo-Code-Org#404) CodeRabbit nitpick: three test cases had duplicate unused helper functions (createMockStream + mockUsage) that were never called. Removed dead code and inlined the usage Promise directly in each mockReturnValue call.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #404 — When the user clicks Stop during streaming, the HTTP request to the provider continues running because the abort signal wasn't being passed through. This wastes API tokens/compute on the provider side.
Changes
Added
{ signal: metadata?.abortSignal }to streaming.chat.completions.create()calls in 13 providers:Also added
abortSignal?: AbortSignaltoApiHandlerCreateMessageMetadatainterface and wired it through Task.ts's AbortController.Test Coverage
p.s All code generated by AI, this is the testing video(I only tested LM Studio)
2026-06-01.230625.mp4
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style/Types