Skip to content

feat(api): abort signal core plumbing (#615) #620

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
easonLiangWorldedtech wants to merge 4 commits into from
Closed

feat(api): abort signal core plumbing (#615) #620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a61e13d
feat(api): abort signal core plumbing (#615)
easonliang28 Jun 14, 2026
99a978a
test(utils): add metadata forwarding test for singleCompletionHandler
easonliang28 Jun 14, 2026
d24a90c
fix(task): add { once: true } to abort event listeners to prevent mem…
easonliang28 Jun 14, 2026
12fd25e
test(task): add tests for task abort signal core plumbing
easonliang28 Jun 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions src/api/__tests__/task-abort-signal-passing.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { describe, it, expect } from "vitest"

import type { ApiHandlerCreateMessageMetadata } from "../index"

describe("abort signal passing", () => {
it("should pass the same AbortController signal instance to metadata.abortSignal", () => {
// Arrange: create an AbortController
const controller = new AbortController()

// Act: simulate what Task.ts does - construct metadata with abortSignal
const metadata: ApiHandlerCreateMessageMetadata = {
taskId: "test-task-id",
abortSignal: controller.signal,
}

// Assert: signal identity (toBe, not just toBeInstanceOf)
expect(metadata.abortSignal).toBe(controller.signal)
})

it("should create a fresh AbortController for each request", () => {
// Arrange: simulate two sequential requests
const controller1 = new AbortController()
const metadata1: ApiHandlerCreateMessageMetadata = {
taskId: "task-1",
abortSignal: controller1.signal,
}

const controller2 = new AbortController()
const metadata2: ApiHandlerCreateMessageMetadata = {
taskId: "task-2",
abortSignal: controller2.signal,
}

// Assert: different instances
expect(metadata1.abortSignal).not.toBe(metadata2.abortSignal)
expect(controller1.signal).not.toBe(controller2.signal)
})

it("should have abortSignal as undefined when not provided", () => {
const metadata: ApiHandlerCreateMessageMetadata = {
taskId: "test-task-id",
}

expect(metadata.abortSignal).toBeUndefined()
})

it("should preserve abortSignal state (aborted vs non-aborted)", () => {
const controller1 = new AbortController()
const controller2 = new AbortController()
controller2.abort()

const metadata1: ApiHandlerCreateMessageMetadata = {
taskId: "task-1",
abortSignal: controller1.signal,
}

const metadata2: ApiHandlerCreateMessageMetadata = {
taskId: "task-2",
abortSignal: controller2.signal,
}

expect(metadata1.abortSignal?.aborted).toBe(false)
expect(metadata2.abortSignal?.aborted).toBe(true)
})
})
7 changes: 6 additions & 1 deletion src/api/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import {
import { NativeOllamaHandler } from "./providers/native-ollama"

export interface SingleCompletionHandler {
completePrompt(prompt: string): Promise<string>
completePrompt(prompt: string, metadata?: ApiHandlerCreateMessageMetadata): Promise<string>
}

export interface ApiHandlerCreateMessageMetadata {
Expand Down Expand Up @@ -90,6 +90,11 @@ export interface ApiHandlerCreateMessageMetadata {
* Only applies to providers that support function calling restrictions (e.g., Gemini).
*/
allowedFunctionNames?: string[]
/**
* Abort signal from the Task's AbortController, used by providers to cancel
* in-flight HTTP requests when the user presses Stop.
*/
abortSignal?: AbortSignal
}

export interface ApiHandler {
Expand Down
106 changes: 106 additions & 0 deletions src/core/__tests__/task-abort-signal-passing.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
import { describe, it, expect } from "vitest"

import type { ApiHandlerCreateMessageMetadata } from "../../api"

describe("abort signal passing", () => {
it("should pass the same AbortController signal instance to metadata.abortSignal", async () => {
// Arrange: create an AbortController
const controller = new AbortController()

// Act: simulate what Task.ts does - construct metadata with abortSignal
const metadata: ApiHandlerCreateMessageMetadata = {
taskId: "test-task-id",
abortSignal: controller.signal,
}

// Assert: signal identity (toBe, not just toBeInstanceOf)
expect(metadata.abortSignal).toBe(controller.signal)
})

it("should create a fresh AbortController for each request", async () => {
// Arrange: simulate two sequential requests
const controller1 = new AbortController()
const metadata1: ApiHandlerCreateMessageMetadata = {
taskId: "task-1",
abortSignal: controller1.signal,
}

const controller2 = new AbortController()
const metadata2: ApiHandlerCreateMessageMetadata = {
taskId: "task-2",
abortSignal: controller2.signal,
}

// Assert: different instances
expect(metadata1.abortSignal).not.toBe(metadata2.abortSignal)
expect(controller1.signal).not.toBe(controller2.signal)
})

it("should trigger abort listener and clear controller reference", async () => {
const controller = new AbortController()
let controllerRef: AbortController | undefined = controller

// Simulate Task.ts abort listener setup with { once: true }
controller.signal.addEventListener(
"abort",
() => {
controllerRef = undefined
},
{ once: true },
)

// Verify initial state
expect(controllerRef).toBe(controller)
expect(controller.signal.aborted).toBe(false)

// Trigger abort
controller.abort()

// Verify listener was called and cleared the reference
expect(controllerRef).toBeUndefined()
expect(controller.signal.aborted).toBe(true)
})

it("should only trigger once even if signal is aborted multiple times", async () => {
const controller = new AbortController()
let callCount = 0

controller.signal.addEventListener(
"abort",
() => {
callCount++
},
{ once: true },
)

// First abort
controller.abort()
expect(callCount).toBe(1)

// Second abort (AbortSignal allows this, though unusual)
controller.abort()
expect(callCount).toBe(1) // Should still be 1 because of { once: true }
})

it("should reject promise immediately if signal already aborted", async () => {
const controller = new AbortController()
controller.abort()

// Simulate the abortPromise logic from Task.ts
const abortPromise = new Promise<never>((_, reject) => {
if (controller.signal.aborted) {
reject(new Error("Request cancelled by user"))
} else {
controller.signal.addEventListener(
"abort",
() => {
reject(new Error("Request cancelled by user"))
},
{ once: true },
)
}
})

await expect(abortPromise).rejects.toThrow("Request cancelled by user")
})
})
Comment on lines +1 to +106

@coderabbitai coderabbitai Bot Jun 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Test coverage gap: tests verify JavaScript API behavior but not the actual implementation.

The tests manually construct ApiHandlerCreateMessageMetadata objects and verify that AbortSignal identity is preserved, but they don't test that the actual code changes work correctly:

  1. Missing: Test that Task.attemptApiRequest creates an AbortController and includes its signal in the metadata passed to api.createMessage
  2. Missing: Test that singleCompletionHandler forwards the metadata (with abortSignal) to the underlying handler
  3. Missing: Integration test showing the signal can actually abort an in-flight request

As per coding guidelines, unit tests should cover "request construction" - these tests should verify that the Task and utility functions correctly build and forward the metadata with the abort signal.

Suggested additions:

  • Mock buildApiHandler and verify attemptApiRequest passes metadata with abortSignal to api.createMessage
  • Test that singleCompletionHandler calls handler.completePrompt(prompt, metadata) with the provided metadata
  • Test that aborting the signal during a request triggers cancellation behavior
🤖 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/core/__tests__/task-abort-signal-passing.spec.ts` around lines 1 - 38,
The current tests verify JavaScript API behavior for AbortSignal identity but do
not test the actual implementation. Add three categories of tests: (1) verify
that Task.attemptApiRequest creates a new AbortController and includes its
signal in the metadata passed to api.createMessage by mocking buildApiHandler
and asserting the metadata argument contains the abortSignal, (2) verify that
singleCompletionHandler forwards the complete metadata object (including
abortSignal) to the underlying handler by checking that handler.completePrompt
is called with the correct metadata, and (3) add an integration test
demonstrating that aborting the signal during a request actually triggers
cancellation behavior. Use mocks and spies appropriately to isolate the behavior
being tested.

Source: Coding guidelines

32 changes: 20 additions & 12 deletions src/core/task/Task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4141,10 +4141,15 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {

const shouldIncludeTools = allTools.length > 0

// Create an AbortController FIRST so we can include its signal in metadata
this.currentRequestAbortController = new AbortController()
const abortSignal = this.currentRequestAbortController.signal

const metadata: ApiHandlerCreateMessageMetadata = {
mode: mode,
taskId: this.taskId,
suppressPreviousResponseId: this.skipPrevResponseIdOnce,
abortSignal: abortSignal,
Comment thread
coderabbitai[bot] marked this conversation as resolved.
// Include tools whenever they are present.
...(shouldIncludeTools
? {
Expand All @@ -4157,11 +4162,6 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
}
: {}),
}

// Create an AbortController to allow cancelling the request mid-stream
this.currentRequestAbortController = new AbortController()
const abortSignal = this.currentRequestAbortController.signal
// Reset the flag after using it
this.skipPrevResponseIdOnce = false

// The provider accepts reasoning items alongside standard messages; cast to the expected parameter type.
Expand All @@ -4173,10 +4173,14 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
const iterator = stream[Symbol.asyncIterator]()

// Set up abort handling - when the signal is aborted, clean up the controller reference
abortSignal.addEventListener("abort", () => {
console.log(`[Task#${this.taskId}.${this.instanceId}] AbortSignal triggered for current request`)
this.currentRequestAbortController = undefined
})
abortSignal.addEventListener(
"abort",
() => {
console.log(`[Task#${this.taskId}.${this.instanceId}] AbortSignal triggered for current request`)
this.currentRequestAbortController = undefined
},
{ once: true },
)

try {
// Awaiting first chunk to see if it will throw an error.
Expand All @@ -4188,9 +4192,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
if (abortSignal.aborted) {
reject(new Error("Request cancelled by user"))
} else {
abortSignal.addEventListener("abort", () => {
reject(new Error("Request cancelled by user"))
})
abortSignal.addEventListener(
"abort",
() => {
reject(new Error("Request cancelled by user"))
},
{ once: true },
)
}
})

Expand Down
Loading
Loading