feat: add abort signal pass-through to ~19 providers#677
Closed
easonLiangWorldedtech wants to merge 1 commit into
Closed
Conversation
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
📝 WalkthroughWalkthroughAdds ChangesAbortSignal propagation to all streaming API providers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 19
🤖 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 `@src/api/providers/__tests__/anthropic-vertex.spec.ts`:
- Around line 1651-1683: The test "should not pass signal when abortSignal is
undefined" in the AnthropicVertexHandler spec currently only validates the
stream output but does not assert that the request construction itself omits the
signal. Add an expectation on the mocked handler["client"].messages.create
function to verify it was called and then inspect the arguments passed to that
mock call to assert that the options object does not include a signal property.
This validates the actual request construction logic rather than just the
output.
- Around line 1616-1649: The test "should handle abort signal triggered during
request" never actually triggers the abort path because controller.abort() is
never called. To fix this, add a call to controller.abort() within the loop that
iterates through the stream chunks (the for await loop iterating over stream) to
actually trigger the abort signal and validate that the handler properly handles
the aborted signal. Consider aborting the controller after collecting one or
more chunks to test the cancellation behavior, then verify that the AbortError
is properly caught or that the streaming stops as expected.
In `@src/api/providers/__tests__/anthropic.spec.ts`:
- Around line 1109-1113: The test assertion in the mockCreate implementation is
overly strict by checking that the entire requestOptions object is undefined,
when it should only verify that the signal property is absent. Modify the expect
statement to assert that requestOptions.signal is undefined rather than
asserting that requestOptions itself is undefined, as other request option
fields (like headers) may still be present even when abortSignal is not
provided.
In `@src/api/providers/__tests__/base-openai-compatible-provider.spec.ts`:
- Around line 560-590: The test for abort signal handling in the createMessage
method flow (around mockCreate and handler.createMessage) validates that abort
behavior triggers an error, but does not assert that the abortSignal was
actually passed through to the mock implementation. Add an assertion after the
stream consumption to verify that mockCreate was called with the correct
abortSignal in its options parameter, ensuring the signal is truly forwarded to
the SDK and not just that abort behavior happens to work. This hardens the test
to catch regressions in signal pass-through.
In `@src/api/providers/__tests__/lmstudio.spec.ts`:
- Around line 164-236: The abort tests verify abort behavior but don't confirm
that the abortSignal is actually forwarded to the underlying SDK call. Add
explicit assertions after the stream consumption in both the "should abort when
signal is aborted" and "should abort immediately if signal is already aborted"
test cases to verify that mockCreate was called with the abortSignal in its
second argument (the options/metadata parameter). This ensures
handler.createMessage is truly passing metadata.abortSignal to
chat.completions.create() rather than relying on the test's outer controller
behavior.
In `@src/api/providers/__tests__/mimo.spec.ts`:
- Around line 1009-1038: The abort signal test for the handler.createMessage
call is missing an explicit assertion to verify that the abortSignal is actually
being forwarded to the mockCreate method call. Add a mock assertion after the
stream consumption (after the for-await loop) to verify that mockCreate was
called with an options object that includes the signal property matching
controller.signal. This ensures the abort signal is explicitly being passed to
the create call, rather than relying on closure behavior alone.
In `@src/api/providers/__tests__/minimax.spec.ts`:
- Around line 489-569: The abort tests in the test suite do not actually
validate that the abortSignal is being forwarded to the SDK call
(this.client.messages.create). Instead of relying only on the mocked iterator
checking the signal internally, add direct assertions on the mockCreate mock to
verify that its second argument contains the signal property with the correct
AbortSignal object. Add these assertions in both the first abort test and the
"should abort immediately if signal is already aborted" test to ensure the
signal is properly passed through to the SDK.
In `@src/api/providers/__tests__/moonshot.spec.ts`:
- Around line 479-556: The abort behavior tests rely on mockFullStream throwing
the abort error, but they do not verify that the handler's createMessage method
actually passes the abortSignal to the streamText call. Add assertions in both
the "should abort after setup" test and the "should abort immediately if signal
is already aborted" test to verify that mockStreamText was called with an object
containing the abortSignal in its metadata parameter. This ensures the abort
signal wiring happens in the actual handler implementation, not just in the mock
generator.
In `@src/api/providers/__tests__/openai.spec.ts`:
- Around line 1298-1312: The abort test mocks in mockCreate.mockImplementation
are using the outer controller.signal directly instead of verifying that the
abortSignal is actually being passed through the options to mockCreate, which
means the test could pass even if createMessage stops forwarding
metadata.abortSignal. Fix this by capturing the abortSignal from the options
parameter passed to mockCreate and using that captured signal in the async
iterator logic instead of relying on the outer controller.signal. Apply this
same correction to both mockCreate.mockImplementation blocks referenced in the
comment.
In `@src/api/providers/__tests__/openrouter.spec.ts`:
- Around line 724-816: The abort test suite does not verify that the abort
signal is actually forwarded to the OpenAI API. The mocked stream reads the
controller signal directly from the test scope rather than receiving it through
the options parameter passed to mockCreate. Add assertions after consuming each
stream to verify that mockCreate was called with options containing the expected
abort signal. For each test case (the main abort test, the "should work normally
without abortSignal" test, and the "should abort immediately if signal is
already aborted" test), use expect(mockCreate).toHaveBeenCalledWith() to assert
that when abortSignal is provided to handler.createMessage(), it is properly
forwarded in the chat.completions.create() call options.
In `@src/api/providers/__tests__/poe.spec.ts`:
- Around line 319-341: The test mock for createMessage is timing-fragile because
the async generator function yields only once and completes before the abort
signal is processed. To make the test deterministic, modify the mockStreamText
mock's async iterator to yield multiple times in a loop (or use a longer
sequence of yields) so that the stream is still actively iterating when the
abort is triggered at the 50ms timeout. This ensures the abort signal check
within the generator function actually encounters the aborted state during
iteration rather than after the stream has already completed naturally.
In `@src/api/providers/__tests__/qwen-code-native-tools.spec.ts`:
- Around line 455-485: The test verifies that the stream rejects when aborted,
but does not assert that the abort signal was actually forwarded to the
underlying create method call. Add an assertion after the stream consumption
test that verifies mockCreate was called and that the options passed to it
include the abortSignal that was provided to handler.createMessage. This ensures
the implementation properly forwards the abort signal through to the underlying
API client.
In `@src/api/providers/__tests__/requesty.spec.ts`:
- Around line 544-631: The abort signal tests are not properly verifying that
the abortSignal is being passed through to the underlying SDK call. In all three
test cases within the "abort signal" describe block, modify the
mockCreate.mockImplementation functions to extract and assert that the signal
from the options argument matches the controller.signal passed to createMessage,
then base the abort behavior check on the signal argument received rather than
the controller.signal from closure. This ensures createMessage is actually
forwarding the abortSignal to chat.completions.create instead of just checking a
variable in scope.
In `@src/api/providers/__tests__/unbound.spec.ts`:
- Around line 206-317: The abort signal tests for UnboundHandler do not verify
that the abortSignal is actually being propagated to the OpenAI client's create
method. In the "should handle abort signal triggered during request" and "should
abort immediately if signal is already aborted" test cases, update the
mockCreateLocal function to capture the second argument (requestOptions) passed
to it, then add an assertion to verify that requestOptions.signal matches the
controller.signal that was passed to createMessage(). This ensures that the
UnboundHandler implementation actually forwards the abortSignal through the
createMessage call down to the underlying OpenAI client creation rather than
relying solely on the mock's internal logic.
In `@src/api/providers/__tests__/xai.spec.ts`:
- Around line 300-380: The abort signal tests do not actually verify that the
abort signal is being passed to mockResponsesCreate, only that the outer
controller.signal aborts the stream. In each test ("should handle abort signal
triggered during request", "should abort immediately if signal is already
aborted"), add an assertion to verify that mockResponsesCreate was called with
options containing the abort signal (e.g.,
expect(mockResponsesCreate).toHaveBeenCalledWith(..., expect.objectContaining({
signal: controller.signal }))). Additionally, modify the mock iterator
implementation to check if the signal passed in the request options is aborted,
rather than checking the outer controller.signal directly, so the test truly
validates that the signal forwarding is working correctly.
In `@src/api/providers/__tests__/zai.spec.ts`:
- Around line 892-982: The abort signal tests verify functional behavior but
don't assert that the abortSignal is being forwarded to the mockCreate call
options. In the test "should handle abort signal triggered during request", add
an assertion after the stream consumption to verify that mockCreate was called
with options containing the abortSignal property matching the controller.signal.
Similarly, in the test "should abort immediately if signal is already aborted",
add an assertion to verify mockCreate was called with the aborted signal in its
options. These assertions will catch any regressions in signal forwarding from
handler.createMessage to the underlying API request.
In `@src/api/providers/__tests__/zoo-gateway.spec.ts`:
- Around line 639-725: The abort signal tests in the "abort signal" describe
block verify that abort behavior works through the mock stream, but they do not
assert that createMessage actually forwards the abortSignal into the options
passed to mockCreate. Add assertions in each test case to verify that mockCreate
was called with options containing the abortSignal property (for the cases where
abortSignal is provided), ensuring that signal-plumbing is correctly implemented
and not just working at the mock stream level.
In `@src/api/providers/lm-studio.ts`:
- Around line 101-107: The catch block around the chat.completions.create() call
immediately wraps all errors with handleOpenAIError(), but when an AbortSignal
cancels the request, the OpenAI SDK throws APIUserAbortError which loses its
type information when wrapped. Add a check in the catch block to detect if the
error is an instance of APIUserAbortError and throw it directly without
wrapping; only call handleOpenAIError() for other error types. This ensures the
APIUserAbortError reaches the outer catch block at line 179 where it can be
properly identified and handled as an abort scenario rather than falling through
to the generic error handler.
In `@src/api/providers/openai.ts`:
- Around line 178-181: The abort signal forwarding is incomplete for O3-family
model requests. While the main stream creation path now forwards the abortSignal
from metadata, the handleO3FamilyMessage function still calls
chat.completions.create without forwarding the same signal. Locate the
handleO3FamilyMessage function and find its chat.completions.create call, then
add the same abort signal handling logic by spreading the conditional
abortSignal check (checking if metadata?.abortSignal exists and passing it as {
signal: metadata?.abortSignal }) to ensure requests for o1/o3/o4 models are
properly cancelled when the user stops them.
🪄 Autofix (Beta)
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: c4b66c08-bd7c-4a0f-8781-254f74ff7917
📒 Files selected for processing (41)
src/api/index.tssrc/api/providers/__tests__/anthropic-vertex.spec.tssrc/api/providers/__tests__/anthropic.spec.tssrc/api/providers/__tests__/base-openai-compatible-provider.spec.tssrc/api/providers/__tests__/deepseek.spec.tssrc/api/providers/__tests__/gemini.spec.tssrc/api/providers/__tests__/lite-llm.spec.tssrc/api/providers/__tests__/lmstudio.spec.tssrc/api/providers/__tests__/mimo.spec.tssrc/api/providers/__tests__/minimax.spec.tssrc/api/providers/__tests__/moonshot.spec.tssrc/api/providers/__tests__/openai.spec.tssrc/api/providers/__tests__/openrouter.spec.tssrc/api/providers/__tests__/poe.spec.tssrc/api/providers/__tests__/qwen-code-native-tools.spec.tssrc/api/providers/__tests__/requesty.spec.tssrc/api/providers/__tests__/unbound.spec.tssrc/api/providers/__tests__/xai.spec.tssrc/api/providers/__tests__/zai.spec.tssrc/api/providers/__tests__/zoo-gateway.spec.tssrc/api/providers/anthropic-vertex.tssrc/api/providers/anthropic.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/deepseek.tssrc/api/providers/gemini.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/mimo.tssrc/api/providers/minimax.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai.tssrc/api/providers/openrouter.tssrc/api/providers/poe.tssrc/api/providers/qwen-code.tssrc/api/providers/requesty.tssrc/api/providers/unbound.tssrc/api/providers/xai.tssrc/api/providers/zai.tssrc/api/providers/zoo-gateway.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.ts
Comment on lines
+1616
to
+1649
| it("should handle abort signal triggered during request", async () => { | ||
| const controller = new AbortController() | ||
| const handler = new AnthropicVertexHandler({ | ||
| apiModelId: "claude-3-sonnet", | ||
| vertexProjectId: "test-project", | ||
| vertexRegion: "us-central1", | ||
| }) | ||
|
|
||
| const mockStream = async function* () { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| if (controller.signal.aborted) { | ||
| throw new Error("AbortError: The operation was aborted") | ||
| } | ||
| yield { | ||
| type: "message_start", | ||
| message: { usage: { input_tokens: 10, output_tokens: 0 } }, | ||
| } | ||
| } | ||
|
|
||
| ;(handler["client"].messages as any).create = vitest.fn().mockResolvedValue(mockStream()) | ||
|
|
||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }], { | ||
| taskId: "test", | ||
| tools: [], | ||
| abortSignal: controller.signal, | ||
| }) | ||
|
|
||
| const chunks: any[] = [] | ||
| for await (const chunk of stream) { | ||
| chunks.push(chunk) | ||
| } | ||
|
|
||
| expect(chunks.length).toBeGreaterThan(0) | ||
| }) |
Contributor
There was a problem hiding this comment.
Abort test never triggers the abort path.
This test never calls controller.abort(), so it validates normal streaming, not cancellation behavior.
Suggested fix
it("should handle abort signal triggered during request", async () => {
const controller = new AbortController()
const handler = new AnthropicVertexHandler({
@@
const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }], {
@@
abortSignal: controller.signal,
})
-
- const chunks: any[] = []
- for await (const chunk of stream) {
- chunks.push(chunk)
- }
-
- expect(chunks.length).toBeGreaterThan(0)
+ setTimeout(() => controller.abort(), 20)
+ await expect(async () => {
+ for await (const _chunk of stream) {
+ // consume stream
+ }
+ }).rejects.toThrow(/abort/i)
})📝 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.
Suggested change
| it("should handle abort signal triggered during request", async () => { | |
| const controller = new AbortController() | |
| const handler = new AnthropicVertexHandler({ | |
| apiModelId: "claude-3-sonnet", | |
| vertexProjectId: "test-project", | |
| vertexRegion: "us-central1", | |
| }) | |
| const mockStream = async function* () { | |
| await new Promise((resolve) => setTimeout(resolve, 10)) | |
| if (controller.signal.aborted) { | |
| throw new Error("AbortError: The operation was aborted") | |
| } | |
| yield { | |
| type: "message_start", | |
| message: { usage: { input_tokens: 10, output_tokens: 0 } }, | |
| } | |
| } | |
| ;(handler["client"].messages as any).create = vitest.fn().mockResolvedValue(mockStream()) | |
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }], { | |
| taskId: "test", | |
| tools: [], | |
| abortSignal: controller.signal, | |
| }) | |
| const chunks: any[] = [] | |
| for await (const chunk of stream) { | |
| chunks.push(chunk) | |
| } | |
| expect(chunks.length).toBeGreaterThan(0) | |
| }) | |
| it("should handle abort signal triggered during request", async () => { | |
| const controller = new AbortController() | |
| const handler = new AnthropicVertexHandler({ | |
| apiModelId: "claude-3-sonnet", | |
| vertexProjectId: "test-project", | |
| vertexRegion: "us-central1", | |
| }) | |
| const mockStream = async function* () { | |
| await new Promise((resolve) => setTimeout(resolve, 10)) | |
| if (controller.signal.aborted) { | |
| throw new Error("AbortError: The operation was aborted") | |
| } | |
| yield { | |
| type: "message_start", | |
| message: { usage: { input_tokens: 10, output_tokens: 0 } }, | |
| } | |
| } | |
| ;(handler["client"].messages as any).create = vitest.fn().mockResolvedValue(mockStream()) | |
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }], { | |
| taskId: "test", | |
| tools: [], | |
| abortSignal: controller.signal, | |
| }) | |
| setTimeout(() => controller.abort(), 20) | |
| await expect(async () => { | |
| for await (const _chunk of stream) { | |
| // consume stream | |
| } | |
| }).rejects.toThrow(/abort/i) | |
| }) |
🤖 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__/anthropic-vertex.spec.ts` around lines 1616 -
1649, The test "should handle abort signal triggered during request" never
actually triggers the abort path because controller.abort() is never called. To
fix this, add a call to controller.abort() within the loop that iterates through
the stream chunks (the for await loop iterating over stream) to actually trigger
the abort signal and validate that the handler properly handles the aborted
signal. Consider aborting the controller after collecting one or more chunks to
test the cancellation behavior, then verify that the AbortError is properly
caught or that the streaming stops as expected.
Comment on lines
+1109
to
+1113
| it("should not pass signal when abortSignal is undefined", async () => { | ||
| mockCreate.mockImplementation(async (options, requestOptions) => { | ||
| // When no abortSignal is provided, requestOptions should be undefined or not have signal | ||
| expect(requestOptions).toBeUndefined() | ||
| return { |
Contributor
There was a problem hiding this comment.
Assert “no signal”, not “no options object”.
Line 1112 is brittle: provider request options may include non-signal fields (e.g., headers) even when abortSignal is absent. Assert only that signal is missing.
Suggested test fix
- expect(requestOptions).toBeUndefined()
+ expect(requestOptions?.signal).toBeUndefined()As per coding guidelines, "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."
📝 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.
Suggested change
| it("should not pass signal when abortSignal is undefined", async () => { | |
| mockCreate.mockImplementation(async (options, requestOptions) => { | |
| // When no abortSignal is provided, requestOptions should be undefined or not have signal | |
| expect(requestOptions).toBeUndefined() | |
| return { | |
| it("should not pass signal when abortSignal is undefined", async () => { | |
| mockCreate.mockImplementation(async (options, requestOptions) => { | |
| // When no abortSignal is provided, requestOptions should be undefined or not have signal | |
| expect(requestOptions?.signal).toBeUndefined() | |
| return { |
🤖 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__/anthropic.spec.ts` around lines 1109 - 1113, The
test assertion in the mockCreate implementation is overly strict by checking
that the entire requestOptions object is undefined, when it should only verify
that the signal property is absent. Modify the expect statement to assert that
requestOptions.signal is undefined rather than asserting that requestOptions
itself is undefined, as other request option fields (like headers) may still be
present even when abortSignal is not provided.
Source: Coding guidelines
Comment on lines
+560
to
+590
| mockCreate.mockImplementation(async (options: unknown) => { | ||
| return { | ||
| async *[Symbol.asyncIterator]() { | ||
| while (!controller.signal.aborted) { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| if (controller.signal.aborted) { | ||
| throw new Error("AbortError: The operation was aborted") | ||
| } | ||
| yield { | ||
| choices: [{ delta: { content: "response" } }], | ||
| usage: null, | ||
| } | ||
| } | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, { | ||
| taskId: "test", | ||
| tools: [], | ||
| abortSignal: controller.signal, | ||
| }) | ||
|
|
||
| setTimeout(() => controller.abort(), 50) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // consume stream | ||
| } | ||
| }).rejects.toThrow(/abort/i) | ||
| }) |
Contributor
There was a problem hiding this comment.
Abort tests don't currently prove signal pass-through.
Lines 560-590 validate abort behavior via a closure, but they never assert the SDK call received the signal. This can pass even if forwarding regresses.
Suggested test hardening
-mockCreate.mockImplementation(async (options: unknown) => {
+mockCreate.mockImplementation(async (options: unknown, requestOptions?: { signal?: AbortSignal }) => {
+ expect(requestOptions?.signal).toBe(controller.signal)
return {
async *[Symbol.asyncIterator]() {
while (!controller.signal.aborted) {As per coding guidelines, "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."
📝 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.
Suggested change
| mockCreate.mockImplementation(async (options: unknown) => { | |
| return { | |
| async *[Symbol.asyncIterator]() { | |
| while (!controller.signal.aborted) { | |
| await new Promise((resolve) => setTimeout(resolve, 10)) | |
| if (controller.signal.aborted) { | |
| throw new Error("AbortError: The operation was aborted") | |
| } | |
| yield { | |
| choices: [{ delta: { content: "response" } }], | |
| usage: null, | |
| } | |
| } | |
| }, | |
| } | |
| }) | |
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, { | |
| taskId: "test", | |
| tools: [], | |
| abortSignal: controller.signal, | |
| }) | |
| setTimeout(() => controller.abort(), 50) | |
| await expect(async () => { | |
| for await (const _chunk of stream) { | |
| // consume stream | |
| } | |
| }).rejects.toThrow(/abort/i) | |
| }) | |
| mockCreate.mockImplementation(async (options: unknown, requestOptions?: { signal?: AbortSignal }) => { | |
| expect(requestOptions?.signal).toBe(controller.signal) | |
| return { | |
| async *[Symbol.asyncIterator]() { | |
| while (!controller.signal.aborted) { | |
| await new Promise((resolve) => setTimeout(resolve, 10)) | |
| if (controller.signal.aborted) { | |
| throw new Error("AbortError: The operation was aborted") | |
| } | |
| yield { | |
| choices: [{ delta: { content: "response" } }], | |
| usage: null, | |
| } | |
| } | |
| }, | |
| } | |
| }) | |
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, { | |
| taskId: "test", | |
| tools: [], | |
| abortSignal: controller.signal, | |
| }) | |
| setTimeout(() => controller.abort(), 50) | |
| await expect(async () => { | |
| for await (const _chunk of stream) { | |
| // consume stream | |
| } | |
| }).rejects.toThrow(/abort/i) | |
| } |
🤖 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__/base-openai-compatible-provider.spec.ts` around
lines 560 - 590, The test for abort signal handling in the createMessage method
flow (around mockCreate and handler.createMessage) validates that abort behavior
triggers an error, but does not assert that the abortSignal was actually passed
through to the mock implementation. Add an assertion after the stream
consumption to verify that mockCreate was called with the correct abortSignal in
its options parameter, ensuring the signal is truly forwarded to the SDK and not
just that abort behavior happens to work. This hardens the test to catch
regressions in signal pass-through.
Source: Coding guidelines
Comment on lines
+164
to
+236
| mockCreate.mockImplementation(async (options: unknown) => { | ||
| return { | ||
| async *[Symbol.asyncIterator]() { | ||
| while (!controller.signal.aborted) { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| if (controller.signal.aborted) { | ||
| throw new DOMException("The operation was aborted.", "AbortError") | ||
| } | ||
| yield { | ||
| choices: [{ delta: { content: "response" }, index: 0 }], | ||
| usage: null, | ||
| } | ||
| } | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, { | ||
| taskId: "test", | ||
| tools: [], | ||
| abortSignal: controller.signal, | ||
| }) | ||
|
|
||
| setTimeout(() => controller.abort(), 50) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // consume stream | ||
| } | ||
| }).rejects.toThrow(/abort/i) | ||
| }) | ||
|
|
||
| it("should work normally without abortSignal", async () => { | ||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any) | ||
|
|
||
| const chunks: any[] = [] | ||
| for await (const chunk of stream) { | ||
| chunks.push(chunk) | ||
| } | ||
|
|
||
| expect(chunks.length).toBeGreaterThan(0) | ||
| }) | ||
|
|
||
| it("should abort immediately if signal is already aborted", async () => { | ||
| const controller = new AbortController() | ||
| controller.abort() | ||
|
|
||
| mockCreate.mockImplementation(async (options: unknown) => { | ||
| return { | ||
| async *[Symbol.asyncIterator]() { | ||
| if (controller.signal.aborted) { | ||
| throw new DOMException("The operation was aborted.", "AbortError") | ||
| } | ||
| yield { | ||
| choices: [{ delta: { content: "response" }, index: 0 }], | ||
| usage: null, | ||
| } | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, { | ||
| taskId: "test", | ||
| tools: [], | ||
| abortSignal: controller.signal, | ||
| }) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // consume stream | ||
| } | ||
| }).rejects.toThrow(/abort/i) | ||
| }) |
Contributor
There was a problem hiding this comment.
Abort tests don’t prove SDK signal forwarding.
The abort behavior here is driven by the test’s outer controller, so these cases still pass even if createMessage stops passing metadata.abortSignal to chat.completions.create(...). Add explicit call-arg assertions for the second argument.
Suggested test hardening
@@
const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, {
taskId: "test",
tools: [],
abortSignal: controller.signal,
})
+ expect(mockCreate).toHaveBeenCalled()
+ const [, requestOptions] = mockCreate.mock.calls[0] ?? []
+ expect(requestOptions).toEqual(expect.objectContaining({ signal: controller.signal }))
@@
const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any)
+ expect(mockCreate).toHaveBeenCalled()
+ const [, requestOptions] = mockCreate.mock.calls[0] ?? []
+ expect(requestOptions).toBeUndefined()
@@
const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, {
taskId: "test",
tools: [],
abortSignal: controller.signal,
})
+ expect(mockCreate).toHaveBeenCalled()
+ const [, requestOptions] = mockCreate.mock.calls[0] ?? []
+ expect(requestOptions).toEqual(expect.objectContaining({ signal: controller.signal }))🤖 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__/lmstudio.spec.ts` around lines 164 - 236, The
abort tests verify abort behavior but don't confirm that the abortSignal is
actually forwarded to the underlying SDK call. Add explicit assertions after the
stream consumption in both the "should abort when signal is aborted" and "should
abort immediately if signal is already aborted" test cases to verify that
mockCreate was called with the abortSignal in its second argument (the
options/metadata parameter). This ensures handler.createMessage is truly passing
metadata.abortSignal to chat.completions.create() rather than relying on the
test's outer controller behavior.
Comment on lines
+300
to
+380
|
|
||
| describe("abort signal", () => { | ||
| it("should handle abort signal triggered during request", async () => { | ||
| const controller = new AbortController() | ||
|
|
||
| mockResponsesCreate.mockResolvedValueOnce({ | ||
| [Symbol.asyncIterator]: async function* () { | ||
| while (true) { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| if (controller.signal.aborted) { | ||
| throw new Error("AbortError: The operation was aborted") | ||
| } | ||
| yield { type: "response.output_text.delta", delta: "chunk" } | ||
| } | ||
| }, | ||
| }) | ||
|
|
||
| handler = new XAIHandler({ xaiApiKey: "test-key" }) | ||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, { | ||
| taskId: "test", | ||
| tools: [], | ||
| abortSignal: controller.signal, | ||
| }) | ||
|
|
||
| setTimeout(() => controller.abort(), 50) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // consume stream | ||
| } | ||
| }).rejects.toThrow(/abort/i) | ||
| }) | ||
|
|
||
| it("should work normally without abortSignal", async () => { | ||
| handler = new XAIHandler({ xaiApiKey: "test-key" }) | ||
|
|
||
| mockResponsesCreate.mockResolvedValueOnce({ | ||
| [Symbol.asyncIterator]: async function* () { | ||
| yield { type: "response.output_text.delta", delta: "Hello" } | ||
| yield { type: "response.output_text.delta", delta: " world!" } | ||
| yield { type: "response.completed", response: { id: "test" }, usage: {} } | ||
| }, | ||
| }) | ||
|
|
||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any) | ||
|
|
||
| const chunks: any[] = [] | ||
| for await (const chunk of stream) { | ||
| chunks.push(chunk) | ||
| } | ||
|
|
||
| expect(chunks.length).toBeGreaterThan(0) | ||
| }) | ||
|
|
||
| it("should abort immediately if signal is already aborted", async () => { | ||
| const controller = new AbortController() | ||
| controller.abort() | ||
|
|
||
| mockResponsesCreate.mockResolvedValueOnce({ | ||
| [Symbol.asyncIterator]: async function* () { | ||
| if (controller.signal.aborted) { | ||
| throw new Error("AbortError: The operation was aborted") | ||
| } | ||
| yield { type: "text", text: "response" } | ||
| }, | ||
| }) | ||
|
|
||
| handler = new XAIHandler({ xaiApiKey: "test-key" }) | ||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, { | ||
| taskId: "test", | ||
| tools: [], | ||
| abortSignal: controller.signal, | ||
| }) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // consume stream | ||
| } | ||
| }).rejects.toThrow(/abort/i) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Abort-path tests don’t prove responses.create receives the abort signal.
The iterator throws based on outer controller.signal, not on the second argument passed to mockResponsesCreate, so this suite can still pass if signal forwarding is accidentally removed. Assert the call includes { signal: controller.signal } and drive abort from request options in the mock.
Suggested test hardening
- mockResponsesCreate.mockResolvedValueOnce({
+ mockResponsesCreate.mockImplementationOnce(async (_body: unknown, requestOptions?: { signal?: AbortSignal }) => {
+ const signal = requestOptions?.signal
+ return {
[Symbol.asyncIterator]: async function* () {
while (true) {
await new Promise((resolve) => setTimeout(resolve, 10))
- if (controller.signal.aborted) {
+ if (signal?.aborted) {
throw new Error("AbortError: The operation was aborted")
}
yield { type: "response.output_text.delta", delta: "chunk" }
}
},
- })
+ }
+ })
+ expect(mockResponsesCreate).toHaveBeenCalledWith(
+ expect.anything(),
+ expect.objectContaining({ signal: controller.signal }),
+ )As per coding guidelines, **/{__tests__,tests,test}/**/*.{test,spec}.{ts,tsx,js}: Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling.
🤖 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__/xai.spec.ts` around lines 300 - 380, The abort
signal tests do not actually verify that the abort signal is being passed to
mockResponsesCreate, only that the outer controller.signal aborts the stream. In
each test ("should handle abort signal triggered during request", "should abort
immediately if signal is already aborted"), add an assertion to verify that
mockResponsesCreate was called with options containing the abort signal (e.g.,
expect(mockResponsesCreate).toHaveBeenCalledWith(..., expect.objectContaining({
signal: controller.signal }))). Additionally, modify the mock iterator
implementation to check if the signal passed in the request options is aborted,
rather than checking the outer controller.signal directly, so the test truly
validates that the signal forwarding is working correctly.
Source: Coding guidelines
Comment on lines
+892
to
+982
| describe("abort signal", () => { | ||
| beforeEach(() => { | ||
| vitest.clearAllMocks() | ||
| }) | ||
|
|
||
| it("should handle abort signal triggered during request", async () => { | ||
| const controller = new AbortController() | ||
| handler = new ZAiHandler({ zaiApiKey: "test-zai-api-key", zaiApiLine: "international_coding" }) | ||
|
|
||
| mockCreate.mockImplementation(async (options: unknown) => { | ||
| return { | ||
| async *[Symbol.asyncIterator]() { | ||
| while (!controller.signal.aborted) { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| if (controller.signal.aborted) { | ||
| throw new Error("AbortError: The operation was aborted") | ||
| } | ||
| yield { | ||
| choices: [{ delta: { content: "response" } }], | ||
| usage: null, | ||
| } | ||
| } | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, { | ||
| taskId: "test", | ||
| tools: [], | ||
| abortSignal: controller.signal, | ||
| }) | ||
|
|
||
| setTimeout(() => controller.abort(), 50) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // consume stream | ||
| } | ||
| }).rejects.toThrow(/abort/i) | ||
| }) | ||
|
|
||
| it("should work normally without abortSignal", async () => { | ||
| handler = new ZAiHandler({ zaiApiKey: "test-zai-api-key", zaiApiLine: "international_coding" }) | ||
|
|
||
| mockCreate.mockResolvedValue({ | ||
| async *[Symbol.asyncIterator]() { | ||
| yield { choices: [{ delta: { content: "Hello" } }], usage: null } | ||
| yield { choices: [{ delta: {} }], usage: { prompt_tokens: 10, completion_tokens: 5 } } | ||
| }, | ||
| }) | ||
|
|
||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any) | ||
|
|
||
| const chunks: any[] = [] | ||
| for await (const chunk of stream) { | ||
| chunks.push(chunk) | ||
| } | ||
|
|
||
| expect(chunks.length).toBeGreaterThan(0) | ||
| }) | ||
|
|
||
| it("should abort immediately if signal is already aborted", async () => { | ||
| const controller = new AbortController() | ||
| controller.abort() | ||
|
|
||
| handler = new ZAiHandler({ zaiApiKey: "test-zai-api-key", zaiApiLine: "international_coding" }) | ||
|
|
||
| mockCreate.mockImplementation(async (options: unknown) => { | ||
| return { | ||
| async *[Symbol.asyncIterator]() { | ||
| if (controller.signal.aborted) { | ||
| throw new Error("AbortError: The operation was aborted") | ||
| } | ||
| yield { choices: [{ delta: { content: "response" } }], usage: null } | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, { | ||
| taskId: "test", | ||
| tools: [], | ||
| abortSignal: controller.signal, | ||
| }) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // consume stream | ||
| } | ||
| }).rejects.toThrow(/abort/i) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Abort suite validates behavior, but not signal forwarding.
The mocks abort via a captured controller and never assert the request-options signal passed to mockCreate. Add a forwarding assertion so regressions in metadata.abortSignal plumbing are caught.
As per coding guidelines, "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."
🤖 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__/zai.spec.ts` around lines 892 - 982, The abort
signal tests verify functional behavior but don't assert that the abortSignal is
being forwarded to the mockCreate call options. In the test "should handle abort
signal triggered during request", add an assertion after the stream consumption
to verify that mockCreate was called with options containing the abortSignal
property matching the controller.signal. Similarly, in the test "should abort
immediately if signal is already aborted", add an assertion to verify mockCreate
was called with the aborted signal in its options. These assertions will catch
any regressions in signal forwarding from handler.createMessage to the
underlying API request.
Source: Coding guidelines
Comment on lines
+639
to
+725
| describe("abort signal", () => { | ||
| it("should handle abort signal triggered during request", async () => { | ||
| const controller = new AbortController() | ||
| const handler = new ZooGatewayHandler(mockOptions) | ||
|
|
||
| mockCreate.mockImplementation(async (options: unknown) => { | ||
| return { | ||
| async *[Symbol.asyncIterator]() { | ||
| while (!controller.signal.aborted) { | ||
| await new Promise((resolve) => setTimeout(resolve, 10)) | ||
| if (controller.signal.aborted) { | ||
| throw new Error("AbortError: The operation was aborted") | ||
| } | ||
| yield { | ||
| choices: [{ delta: { content: "response" } }], | ||
| usage: null, | ||
| } | ||
| } | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, { | ||
| taskId: "test", | ||
| tools: [], | ||
| abortSignal: controller.signal, | ||
| }) | ||
|
|
||
| setTimeout(() => controller.abort(), 50) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // consume stream | ||
| } | ||
| }).rejects.toThrow(/abort/i) | ||
| }) | ||
|
|
||
| it("should work normally without abortSignal", async () => { | ||
| const handler = new ZooGatewayHandler(mockOptions) | ||
|
|
||
| mockCreate.mockResolvedValue({ | ||
| async *[Symbol.asyncIterator]() { | ||
| yield { choices: [{ delta: { content: "Hello" } }], usage: null } | ||
| yield { choices: [{ delta: {} }], usage: { prompt_tokens: 10, completion_tokens: 5 } } | ||
| }, | ||
| }) | ||
|
|
||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any) | ||
|
|
||
| const chunks: any[] = [] | ||
| for await (const chunk of stream) { | ||
| chunks.push(chunk) | ||
| } | ||
|
|
||
| expect(chunks.length).toBeGreaterThan(0) | ||
| }) | ||
|
|
||
| it("should abort immediately if signal is already aborted", async () => { | ||
| const controller = new AbortController() | ||
| controller.abort() | ||
|
|
||
| const handler = new ZooGatewayHandler(mockOptions) | ||
|
|
||
| mockCreate.mockImplementation(async (options: unknown) => { | ||
| return { | ||
| async *[Symbol.asyncIterator]() { | ||
| if (controller.signal.aborted) { | ||
| throw new Error("AbortError: The operation was aborted") | ||
| } | ||
| yield { choices: [{ delta: { content: "response" } }], usage: null } | ||
| }, | ||
| } | ||
| }) | ||
|
|
||
| const stream = handler.createMessage("system", [{ role: "user", content: "Hello" }] as any, { | ||
| taskId: "test", | ||
| tools: [], | ||
| abortSignal: controller.signal, | ||
| }) | ||
|
|
||
| await expect(async () => { | ||
| for await (const _chunk of stream) { | ||
| // consume stream | ||
| } | ||
| }).rejects.toThrow(/abort/i) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Abort tests miss request-option signal assertions.
Current tests only observe a local controller in the mock stream. They do not verify that createMessage forwards abortSignal into the client call config, so signal-plumbing regressions may go undetected.
As per coding guidelines, "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."
🤖 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__/zoo-gateway.spec.ts` around lines 639 - 725, The
abort signal tests in the "abort signal" describe block verify that abort
behavior works through the mock stream, but they do not assert that
createMessage actually forwards the abortSignal into the options passed to
mockCreate. Add assertions in each test case to verify that mockCreate was
called with options containing the abortSignal property (for the cases where
abortSignal is provided), ensuring that signal-plumbing is correctly implemented
and not just working at the mock stream level.
Source: Coding guidelines
Comment on lines
+101
to
107
| results = await this.client.chat.completions.create( | ||
| params, | ||
| metadata?.abortSignal ? { signal: metadata?.abortSignal } : undefined, | ||
| ) | ||
| } catch (error) { | ||
| throw handleOpenAIError(error, this.providerName) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the openainpm package version5.12.2, what exact error type/name is thrown when chat.completions.create(...)is canceled viaAbortSignal? Please include recommended cancellation checks for provider code.
💡 Result:
When a request to chat.completions.create(...) is canceled via an AbortSignal in the OpenAI Node.js SDK (including version 5.12.2), the error thrown is an instance of APIUserAbortError [1]. This error type is a specialized class provided by the SDK to represent a request intentionally aborted by the consumer, typically via an AbortController [1]. Recommended Cancellation Checks To handle cancellation in your provider code, you should wrap your API calls in a try-catch block and check if the caught error is an instance of the APIUserAbortError class [1]. It is also standard practice to check if the error is a general APIError or a native JavaScript AbortError depending on your runtime environment [2][1]. Example implementation: import OpenAI from 'openai'; const client = new OpenAI; const controller = new AbortController; try { const completion = await client.chat.completions.create( { model: 'gpt-4o', messages: [{ role: 'user', content: 'Hello' }], }, { signal: controller.signal }); console.log(completion); } catch (err) { if (err instanceof OpenAI.APIUserAbortError) { console.log('Request was aborted by the user.'); } else if (err instanceof OpenAI.APIError) { // Handle other API-related errors (e.g., rate limits, server errors) console.error('API Error:', err.status, err.name); } else { // Handle unexpected or other error types throw err; } } // To trigger the cancellation: controller.abort; In addition to handling the error during the initial request, if you are using streaming, you can verify cancellation by monitoring the AbortSignal status on the stream controller or by breaking out of the for-await-of loop when your application logic determines the stream is no longer needed [3][4][5].
Citations:
- 1: https://deepwiki.com/openai/openai-node/5.2-error-handling
- 2: https://github.com/openai/openai-node?tab=readme-ov-file
- 3: How to stop streaming openai/openai-node#682
- 4: https://github.com/openai/openai-node/blob/e228aaad/src/lib/ChatCompletionStream.ts
- 5: https://github.com/openai/openai-node/tree/8438c2c79229448b21f9b92b07e37b98ba88ea80
🏁 Script executed:
cat -n src/api/providers/lm-studio.ts | head -200Repository: Zoo-Code-Org/Zoo-Code
Length of output: 7522
🏁 Script executed:
find . -name "*openai-error*" -o -name "*error-handler*" | grep -i openaiRepository: Zoo-Code-Org/Zoo-Code
Length of output: 283
🏁 Script executed:
cat -n ./src/api/providers/utils/openai-error-handler.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 931
🏁 Script executed:
cat -n ./src/api/providers/utils/error-handler.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 4700
🌐 Web query:
OpenAI Node.js SDK v5.12.2 APIUserAbortError - how to import and detect abort cancellation errors
💡 Result:
To import and detect an APIUserAbortError in the OpenAI Node.js SDK (v5.x), you can import the error class directly from the main package and use standard instance checking [1][2]. Importing the Error Class You can import APIUserAbortError directly from the openai package [1][2]: import { APIUserAbortError } from 'openai'; Detecting the Error When you abort a request (for example, using an AbortController signal), the SDK throws an APIUserAbortError [3][4]. You can detect this by catching the error in a try/catch block and checking if the error is an instance of this class [5]: try { await openai.chat.completions.create({ model: 'gpt-4o', messages: [{ role: 'user', content: 'Say hello' }], }, { signal: abortController.signal }); } catch (err) { if (err instanceof APIUserAbortError) { console.log('Request was aborted by the user.'); } else { console.error('An unexpected error occurred:', err); } } Handling Streaming Aborts If you are using the streaming helpers (like ChatCompletionStreamingRunner), the standard.on('error') event might not trigger for an abort [6]. Instead, you should: 1. Use the.on('abort') event, which is specifically designed to handle cancellation signals [6][7]. 2. Alternatively, catch the error from the.finalChatCompletion promise if you are using that pattern [8]. Example using.on('abort'): const runner = openai.beta.chat.completions.stream({... }); runner.on('abort', => { console.log('Stream was aborted.'); }); runner.on('error', (err) => { // Handles non-abort errors }); Summary of Best Practices - Import Error Classes: Import directly from the top-level openai package for access to all SDK error types [1][2]. - Use instanceof: The most reliable way to identify specific SDK errors is using the instanceof operator [5]. - Event Listeners: When dealing with streaming, prefer.on('abort') over.on('error') to handle user-initiated cancellations gracefully [6][7].
Citations:
- 1: https://github.com/openai/openai-node/blob/master/src/index.ts
- 2: https://github.com/openai/openai-node/blob/e228aaad/src/index.ts
- 3: https://github.com/openai/openai-node/blob/5436f42d/src/client.ts
- 4: https://github.com/openai/openai-node/blob/3fdc7d4f67a6ceea51723684dcc0bc1895088259/src/core.ts
- 5: https://deepwiki.com/openai/openai-node/5.2-error-handling
- 6: on('error', () => ...) event does not fire on APIUserAbortError openai/openai-node#526
- 7: https://github.com/openai/openai-node/blob/54e4f643256b497bdbd5e61637cf0782393b89c8/helpers.md
- 8: How to stop streaming openai/openai-node#682
🏁 Script executed:
cat -n ./src/api/providers/utils/__tests__/openai-error-handler.spec.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 7239
🏁 Script executed:
rg "APIUserAbortError" --type ts --type tsxRepository: Zoo-Code-Org/Zoo-Code
Length of output: 93
🏁 Script executed:
rg "APIUserAbortError" --type tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 47
🏁 Script executed:
rg "DOMException" --type ts -B 2 -A 2Repository: Zoo-Code-Org/Zoo-Code
Length of output: 3602
🏁 Script executed:
cat -n src/api/providers/__tests__/lmstudio.spec.ts | head -100Repository: Zoo-Code-Org/Zoo-Code
Length of output: 3145
🏁 Script executed:
rg "abort" src/api/providers/__tests__/lmstudio.spec.ts -B 3 -A 10Repository: Zoo-Code-Org/Zoo-Code
Length of output: 2506
🏁 Script executed:
rg "import.*OpenAI" src/api/providers/lm-studio.tsRepository: Zoo-Code-Org/Zoo-Code
Length of output: 158
🏁 Script executed:
cat -n src/api/providers/__tests__/lmstudio.spec.ts | grep -A 40 "abort signal triggered"Repository: Zoo-Code-Org/Zoo-Code
Length of output: 1562
Check for APIUserAbortError before wrapping with handleOpenAIError().
When chat.completions.create(...) is canceled via AbortSignal, the OpenAI SDK (v5.12.2) throws APIUserAbortError (not DOMException). At line 105, this error is immediately wrapped by handleOpenAIError(), which creates a new generic Error and discards the original error type. The outer catch block at line 179 checks for DOMException with name "AbortError", but the wrapped error won't match—it falls through to the generic error handler at line 184.
Add a check for APIUserAbortError before calling handleOpenAIError():
Proposed fix
+ import { APIUserAbortError } from "openai"
+
let results
try {
results = await this.client.chat.completions.create(
params,
metadata?.abortSignal ? { signal: metadata?.abortSignal } : undefined,
)
} catch (error) {
+ if (error instanceof APIUserAbortError) {
+ throw error
+ }
throw handleOpenAIError(error, this.providerName)
}Also applies to: 179-183
🤖 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/lm-studio.ts` around lines 101 - 107, The catch block
around the chat.completions.create() call immediately wraps all errors with
handleOpenAIError(), but when an AbortSignal cancels the request, the OpenAI SDK
throws APIUserAbortError which loses its type information when wrapped. Add a
check in the catch block to detect if the error is an instance of
APIUserAbortError and throw it directly without wrapping; only call
handleOpenAIError() for other error types. This ensures the APIUserAbortError
reaches the outer catch block at line 179 where it can be properly identified
and handled as an abort scenario rather than falling through to the generic
error handler.
easonLiangWorldedtech
force-pushed
the
feat/abort-signal-provider-pass-through-pr
branch
from
37cff20 to
5d6682d
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/api/providers/__tests__/anthropic-vertex.spec.ts (2)
1619-1652:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAbort-path test never actually triggers abort.
This case is named as mid-request cancellation, but
controller.abort()is never called, so it only verifies normal streaming. Add an explicit abort during iteration (or shortly after stream start) and assert rejection with an abort error.
As per coding guidelines, "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."🤖 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__/anthropic-vertex.spec.ts` around lines 1619 - 1652, The test for abort signal handling never actually triggers the abort since controller.abort() is never called. To fix this, call controller.abort() at an appropriate point during the stream iteration (for example, after collecting the first chunk or shortly after starting the iteration in the for await loop), and then assert that the stream throws or rejects with an abort error instead of expecting successful chunk collection. This ensures the abort path is actually exercised by the test.Source: Coding guidelines
1654-1686:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win“No abortSignal” test does not verify request construction.
The test only checks emitted chunks; it should also assert that
client.messages.createis called without asignalin request options to validate the no-signal contract.
As per coding guidelines, "Use package-local unit tests for pure logic, parsing, state transitions, validation, serialization, request construction, retry decisions, and error handling."🤖 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__/anthropic-vertex.spec.ts` around lines 1654 - 1686, The test "should not pass signal when abortSignal is undefined" for the AnthropicVertexHandler only verifies that chunks are emitted from the stream, but does not validate that the client.messages.create method was called with the correct request construction (specifically without a signal parameter). After the for-await loop completes in the test, add an assertion using vitest matchers to verify that the mocked handler["client"].messages method was called, and that the request options passed to create do not include a signal property to properly validate the no-signal contract.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@src/api/providers/__tests__/anthropic-vertex.spec.ts`:
- Around line 1619-1652: The test for abort signal handling never actually
triggers the abort since controller.abort() is never called. To fix this, call
controller.abort() at an appropriate point during the stream iteration (for
example, after collecting the first chunk or shortly after starting the
iteration in the for await loop), and then assert that the stream throws or
rejects with an abort error instead of expecting successful chunk collection.
This ensures the abort path is actually exercised by the test.
- Around line 1654-1686: The test "should not pass signal when abortSignal is
undefined" for the AnthropicVertexHandler only verifies that chunks are emitted
from the stream, but does not validate that the client.messages.create method
was called with the correct request construction (specifically without a signal
parameter). After the for-await loop completes in the test, add an assertion
using vitest matchers to verify that the mocked handler["client"].messages
method was called, and that the request options passed to create do not include
a signal property to properly validate the no-signal contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fa9b51e9-86a6-4083-90b5-2e8c0ead972b
📒 Files selected for processing (38)
src/api/providers/__tests__/anthropic-vertex.spec.tssrc/api/providers/__tests__/anthropic.spec.tssrc/api/providers/__tests__/base-openai-compatible-provider.spec.tssrc/api/providers/__tests__/deepseek.spec.tssrc/api/providers/__tests__/gemini.spec.tssrc/api/providers/__tests__/lite-llm.spec.tssrc/api/providers/__tests__/lmstudio.spec.tssrc/api/providers/__tests__/mimo.spec.tssrc/api/providers/__tests__/minimax.spec.tssrc/api/providers/__tests__/moonshot.spec.tssrc/api/providers/__tests__/openai.spec.tssrc/api/providers/__tests__/openrouter.spec.tssrc/api/providers/__tests__/poe.spec.tssrc/api/providers/__tests__/qwen-code-native-tools.spec.tssrc/api/providers/__tests__/requesty.spec.tssrc/api/providers/__tests__/unbound.spec.tssrc/api/providers/__tests__/xai.spec.tssrc/api/providers/__tests__/zai.spec.tssrc/api/providers/__tests__/zoo-gateway.spec.tssrc/api/providers/anthropic-vertex.tssrc/api/providers/anthropic.tssrc/api/providers/base-openai-compatible-provider.tssrc/api/providers/deepseek.tssrc/api/providers/gemini.tssrc/api/providers/lite-llm.tssrc/api/providers/lm-studio.tssrc/api/providers/mimo.tssrc/api/providers/minimax.tssrc/api/providers/openai-compatible.tssrc/api/providers/openai.tssrc/api/providers/openrouter.tssrc/api/providers/poe.tssrc/api/providers/qwen-code.tssrc/api/providers/requesty.tssrc/api/providers/unbound.tssrc/api/providers/xai.tssrc/api/providers/zai.tssrc/api/providers/zoo-gateway.ts
✅ Files skipped from review due to trivial changes (2)
- src/api/providers/mimo.ts
- src/api/providers/zoo-gateway.ts
🚧 Files skipped from review as they are similar to previous changes (35)
- src/api/providers/zai.ts
- src/api/providers/gemini.ts
- src/api/providers/deepseek.ts
- src/api/providers/xai.ts
- src/api/providers/openrouter.ts
- src/api/providers/tests/unbound.spec.ts
- src/api/providers/tests/base-openai-compatible-provider.spec.ts
- src/api/providers/tests/deepseek.spec.ts
- src/api/providers/requesty.ts
- src/api/providers/openai-compatible.ts
- src/api/providers/tests/moonshot.spec.ts
- src/api/providers/poe.ts
- src/api/providers/lm-studio.ts
- src/api/providers/tests/zoo-gateway.spec.ts
- src/api/providers/tests/mimo.spec.ts
- src/api/providers/tests/openai.spec.ts
- src/api/providers/openai.ts
- src/api/providers/qwen-code.ts
- src/api/providers/lite-llm.ts
- src/api/providers/anthropic-vertex.ts
- src/api/providers/tests/requesty.spec.ts
- src/api/providers/tests/openrouter.spec.ts
- src/api/providers/tests/minimax.spec.ts
- src/api/providers/tests/lite-llm.spec.ts
- src/api/providers/tests/anthropic.spec.ts
- src/api/providers/minimax.ts
- src/api/providers/anthropic.ts
- src/api/providers/tests/gemini.spec.ts
- src/api/providers/tests/lmstudio.spec.ts
- src/api/providers/unbound.ts
- src/api/providers/base-openai-compatible-provider.ts
- src/api/providers/tests/xai.spec.ts
- src/api/providers/tests/qwen-code-native-tools.spec.ts
- src/api/providers/tests/poe.spec.ts
- src/api/providers/tests/zai.spec.ts
Contributor
Author
|
Closing to recreate from correct branch (feat/abort-signal-provider-pass-through with core plumbing + providers) |
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
Complete abort signal pass-through across 19 HTTP-making API providers. When user clicks stop, the AbortController.signal from Task.ts flows through metadata → provider → HTTP client, cancelling the in-flight request and preventing wasted tokens/compute.
Changes (41 files, +2170/-48)
Core Plumbing
abortSignal?: AbortSignaltoApiHandlerCreateMessageMetadataAbortController.signalinto createMessage metadataProvider Pass-Through (19 providers)
Providers updated to thread
metadata.abortSignalthrough to their HTTP clients:Tests Added (~1800 lines)
Comprehensive test coverage for all updated providers verifying:
Related
Closes #434
Summary by CodeRabbit
New Features
Tests