Skip to content

Commit 09012d5

Browse files
committed
fix(task): add { once: true } to abort event listeners to prevent memory leaks
- Add { once: true } to the controller cleanup listener (L4179) - Add { once: true } to the abortPromise reject listener (L4194) - Prevents duplicate listener accumulation if signal fires multiple times test(task): add abort signal lifecycle tests - Test that abort listener clears controller reference - Test that { once: true } prevents duplicate calls on repeated abort - Test immediate rejection when signal is already aborted
1 parent d2529af commit 09012d5

3 files changed

Lines changed: 148 additions & 7 deletions

File tree

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
import { describe, it, expect } from "vitest"
2+
3+
import type { ApiHandlerCreateMessageMetadata } from "../index"
4+
5+
describe("abort signal passing", () => {
6+
it("should pass the same AbortController signal instance to metadata.abortSignal", () => {
7+
// Arrange: create an AbortController
8+
const controller = new AbortController()
9+
10+
// Act: simulate what Task.ts does - construct metadata with abortSignal
11+
const metadata: ApiHandlerCreateMessageMetadata = {
12+
taskId: "test-task-id",
13+
abortSignal: controller.signal,
14+
}
15+
16+
// Assert: signal identity (toBe, not just toBeInstanceOf)
17+
expect(metadata.abortSignal).toBe(controller.signal)
18+
})
19+
20+
it("should create a fresh AbortController for each request", () => {
21+
// Arrange: simulate two sequential requests
22+
const controller1 = new AbortController()
23+
const metadata1: ApiHandlerCreateMessageMetadata = {
24+
taskId: "task-1",
25+
abortSignal: controller1.signal,
26+
}
27+
28+
const controller2 = new AbortController()
29+
const metadata2: ApiHandlerCreateMessageMetadata = {
30+
taskId: "task-2",
31+
abortSignal: controller2.signal,
32+
}
33+
34+
// Assert: different instances
35+
expect(metadata1.abortSignal).not.toBe(metadata2.abortSignal)
36+
expect(controller1.signal).not.toBe(controller2.signal)
37+
})
38+
39+
it("should have abortSignal as undefined when not provided", () => {
40+
const metadata: ApiHandlerCreateMessageMetadata = {
41+
taskId: "test-task-id",
42+
}
43+
44+
expect(metadata.abortSignal).toBeUndefined()
45+
})
46+
47+
it("should preserve abortSignal state (aborted vs non-aborted)", () => {
48+
const controller1 = new AbortController()
49+
const controller2 = new AbortController()
50+
controller2.abort()
51+
52+
const metadata1: ApiHandlerCreateMessageMetadata = {
53+
taskId: "task-1",
54+
abortSignal: controller1.signal,
55+
}
56+
57+
const metadata2: ApiHandlerCreateMessageMetadata = {
58+
taskId: "task-2",
59+
abortSignal: controller2.signal,
60+
}
61+
62+
expect(metadata1.abortSignal?.aborted).toBe(false)
63+
expect(metadata2.abortSignal?.aborted).toBe(true)
64+
})
65+
})

src/core/__tests__/task-abort-signal-passing.spec.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,4 +35,72 @@ describe("abort signal passing", () => {
3535
expect(metadata1.abortSignal).not.toBe(metadata2.abortSignal)
3636
expect(controller1.signal).not.toBe(controller2.signal)
3737
})
38+
39+
it("should trigger abort listener and clear controller reference", async () => {
40+
const controller = new AbortController()
41+
let controllerRef: AbortController | undefined = controller
42+
43+
// Simulate Task.ts abort listener setup with { once: true }
44+
controller.signal.addEventListener(
45+
"abort",
46+
() => {
47+
controllerRef = undefined
48+
},
49+
{ once: true },
50+
)
51+
52+
// Verify initial state
53+
expect(controllerRef).toBe(controller)
54+
expect(controller.signal.aborted).toBe(false)
55+
56+
// Trigger abort
57+
controller.abort()
58+
59+
// Verify listener was called and cleared the reference
60+
expect(controllerRef).toBeUndefined()
61+
expect(controller.signal.aborted).toBe(true)
62+
})
63+
64+
it("should only trigger once even if signal is aborted multiple times", async () => {
65+
const controller = new AbortController()
66+
let callCount = 0
67+
68+
controller.signal.addEventListener(
69+
"abort",
70+
() => {
71+
callCount++
72+
},
73+
{ once: true },
74+
)
75+
76+
// First abort
77+
controller.abort()
78+
expect(callCount).toBe(1)
79+
80+
// Second abort (AbortSignal allows this, though unusual)
81+
controller.abort()
82+
expect(callCount).toBe(1) // Should still be 1 because of { once: true }
83+
})
84+
85+
it("should reject promise immediately if signal already aborted", async () => {
86+
const controller = new AbortController()
87+
controller.abort()
88+
89+
// Simulate the abortPromise logic from Task.ts
90+
const abortPromise = new Promise<never>((_, reject) => {
91+
if (controller.signal.aborted) {
92+
reject(new Error("Request cancelled by user"))
93+
} else {
94+
controller.signal.addEventListener(
95+
"abort",
96+
() => {
97+
reject(new Error("Request cancelled by user"))
98+
},
99+
{ once: true },
100+
)
101+
}
102+
})
103+
104+
await expect(abortPromise).rejects.toThrow("Request cancelled by user")
105+
})
38106
})

src/core/task/Task.ts

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4176,10 +4176,14 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
41764176
const iterator = stream[Symbol.asyncIterator]()
41774177

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

41844188
try {
41854189
// Awaiting first chunk to see if it will throw an error.
@@ -4191,9 +4195,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
41914195
if (abortSignal.aborted) {
41924196
reject(new Error("Request cancelled by user"))
41934197
} else {
4194-
abortSignal.addEventListener("abort", () => {
4195-
reject(new Error("Request cancelled by user"))
4196-
})
4198+
abortSignal.addEventListener(
4199+
"abort",
4200+
() => {
4201+
reject(new Error("Request cancelled by user"))
4202+
},
4203+
{ once: true },
4204+
)
41974205
}
41984206
})
41994207

0 commit comments

Comments
 (0)