Skip to content

Commit bd1a717

Browse files
easonliang28easonliang28
committed
fix(api): chain request config cleanup
1 parent 7335967 commit bd1a717

2 files changed

Lines changed: 104 additions & 46 deletions

File tree

‎src/api/providers/__tests__/request-config-builder.spec.ts‎

Lines changed: 85 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, expect, test } from "vitest"
1+
import { describe, expect, test, vi } from "vitest"
22

33
import type { ApiHandlerCreateMessageMetadata } from "../../index"
44
import { RequestConfigBuilder } from "../config-builder/request-config-builder"
@@ -53,7 +53,7 @@ describe("RequestConfigBuilder", () => {
5353
const builder = new RequestConfigBuilder({ initial: "value" })
5454
builder.addAbortSignal(undefined)
5555

56-
const config = builder.build() as Record<string, any>
56+
const config = builder.build() as { signal?: AbortSignal }
5757
expect(config.signal).toBeUndefined()
5858
})
5959

@@ -65,7 +65,7 @@ describe("RequestConfigBuilder", () => {
6565
const builder = new RequestConfigBuilder({ initial: "value" })
6666
builder.addAbortSignal(metadata)
6767

68-
const config = builder.build() as Record<string, any>
68+
const config = builder.build() as { signal?: AbortSignal }
6969
expect(config.signal).toBeUndefined()
7070
})
7171

@@ -115,7 +115,7 @@ describe("RequestConfigBuilder", () => {
115115
const result = builder.addHeaders()
116116

117117
expect(result).toBe(builder) // chainable
118-
const config = builder.build() as Record<string, any>
118+
const config = builder.build() as { headers?: Record<string, string> }
119119
expect(config.headers).toBeUndefined()
120120
})
121121

@@ -124,7 +124,7 @@ describe("RequestConfigBuilder", () => {
124124
const result = builder.addHeaders({})
125125

126126
expect(result).toBe(builder) // chainable
127-
const config = builder.build() as Record<string, any>
127+
const config = builder.build() as { headers?: Record<string, string> }
128128
expect(config.headers).toBeUndefined()
129129
})
130130

@@ -173,9 +173,9 @@ describe("RequestConfigBuilder", () => {
173173

174174
test("should do nothing when value is undefined", () => {
175175
const builder = new RequestConfigBuilder({ initial: "value" })
176-
builder.setOption("initial", undefined as any)
176+
builder.setOption("initial", undefined as unknown as string)
177177

178-
const config = builder.build() as Record<string, any>
178+
const config = builder.build() as { initial?: string }
179179
// When setOption receives undefined, it should NOT modify the existing value
180180
expect(config.initial).toBe("value")
181181
})
@@ -196,7 +196,12 @@ describe("RequestConfigBuilder", () => {
196196
builder.setOption("booleanKey", true)
197197
builder.setOption("objectKey", { nested: true })
198198

199-
const config = builder.build() as Record<string, any>
199+
const config = builder.build() as {
200+
stringKey?: string
201+
numberKey?: number
202+
booleanKey?: boolean
203+
objectKey?: { nested: boolean }
204+
}
200205
expect(config.stringKey).toBe("stringValue")
201206
expect(config.numberKey).toBe(42)
202207
expect(config.booleanKey).toBe(true)
@@ -208,7 +213,7 @@ describe("RequestConfigBuilder", () => {
208213
const result = builder.setOption("key1", "value1").setOption("key2", "value2")
209214

210215
expect(result).toBe(builder)
211-
const config = builder.build() as Record<string, any>
216+
const config = builder.build() as { key1?: string; key2?: string }
212217
expect(config.key1).toBe("value1")
213218
expect(config.key2).toBe("value2")
214219
})
@@ -222,7 +227,7 @@ describe("RequestConfigBuilder", () => {
222227

223228
test("should return undefined for non-existent key", () => {
224229
const builder = new RequestConfigBuilder()
225-
expect(builder.getOption("nonExistent" as any)).toBeUndefined()
230+
expect(builder.getOption("nonExistent")).toBeUndefined()
226231
})
227232
})
228233

@@ -243,7 +248,7 @@ describe("RequestConfigBuilder", () => {
243248

244249
test("modifying build result should not affect internal state", () => {
245250
const builder = new RequestConfigBuilder({ key: "value" })
246-
const result = builder.build() as Record<string, any>
251+
const result = builder.build() as { key: string }
247252

248253
result.key = "modified"
249254
expect(builder.getOption("key")).toBe("value")
@@ -259,7 +264,11 @@ describe("RequestConfigBuilder", () => {
259264
const builder = new RequestConfigBuilder()
260265
builder.addAbortSignal(metadata).addHeaders({ "X-Custom": "value" }).setOption("modelId", "test-model")
261266

262-
const config = builder.build() as Record<string, any>
267+
const config = builder.build() as {
268+
signal?: AbortSignal
269+
headers?: Record<string, string>
270+
modelId?: string
271+
}
263272
expect(config.signal).toBe(controller.signal)
264273
expect(config.headers).toEqual({ "X-Custom": "value" })
265274
expect(config.modelId).toBe("test-model")
@@ -279,7 +288,7 @@ describe("RequestConfigBuilder", () => {
279288
abortSignal: controller.signal,
280289
}
281290

282-
const result = RequestConfigBuilder.fromMetadata(metadata) as Record<string, any>
291+
const result = RequestConfigBuilder.fromMetadata(metadata) as { signal?: AbortSignal }
283292
expect(result.signal).toBe(controller.signal)
284293
})
285294

@@ -291,23 +300,30 @@ describe("RequestConfigBuilder", () => {
291300
}
292301
const extraOptions = { modelId: "test-model", customKey: "customValue" }
293302

294-
const result = RequestConfigBuilder.fromMetadata(metadata, extraOptions) as Record<string, any>
303+
const result = RequestConfigBuilder.fromMetadata(metadata, extraOptions) as {
304+
signal?: AbortSignal
305+
modelId?: string
306+
customKey?: string
307+
}
295308
expect(result.signal).toBe(controller.signal)
296309
expect(result.modelId).toBe("test-model")
297310
expect(result.customKey).toBe("customValue")
298311
})
299312

300313
test("should return only extraOptions when metadata is undefined", () => {
301314
const extraOptions = { modelId: "test-model" }
302-
const result = RequestConfigBuilder.fromMetadata(undefined, extraOptions) as Record<string, any>
315+
const result = RequestConfigBuilder.fromMetadata(undefined, extraOptions) as { modelId?: string }
303316
expect(result.modelId).toBe("test-model")
304317
})
305318

306319
test("should not set signal when metadata.abortSignal is undefined", () => {
307320
const metadata: ApiHandlerCreateMessageMetadata = { taskId: "test-task" }
308321
const extraOptions = { modelId: "test-model" }
309322

310-
const result = RequestConfigBuilder.fromMetadata(metadata, extraOptions) as Record<string, any>
323+
const result = RequestConfigBuilder.fromMetadata(metadata, extraOptions) as {
324+
signal?: AbortSignal
325+
modelId?: string
326+
}
311327
expect(result.signal).toBeUndefined()
312328
expect(result.modelId).toBe("test-model")
313329
})
@@ -345,23 +361,61 @@ describe("RequestConfigBuilder", () => {
345361
expect(config.signal?.aborted).toBe(true)
346362
})
347363

348-
test("should merge internal controller signal with timeout", async () => {
364+
test("should clear timeout when cleanup runs before timeout fires", async () => {
349365
vi.useFakeTimers()
350-
const internalController = new AbortController()
351-
const builder = new RequestConfigBuilder()
366+
try {
367+
const internalController = new AbortController()
368+
const builder = new RequestConfigBuilder()
352369

353-
builder.addMergedSignal(internalController, undefined, 100)
370+
builder.addMergedSignal(internalController, undefined, 100)
354371

355-
const config = builder.build() as { signal?: AbortSignal; _cleanup?: () => void }
356-
expect(config.signal).not.toBe(internalController.signal)
357-
expect(config).not.toHaveProperty("_cleanup")
358-
expect(config.signal?.aborted).toBe(false)
372+
const config = builder.build() as { signal?: AbortSignal; _cleanup?: () => void }
373+
expect(config.signal).not.toBe(internalController.signal)
374+
expect(config).not.toHaveProperty("_cleanup")
375+
expect(config.signal?.aborted).toBe(false)
376+
expect(vi.getTimerCount()).toBe(1)
359377

360-
await vi.advanceTimersByTimeAsync(100)
378+
builder.getCleanup()()
361379

380+
expect(vi.getTimerCount()).toBe(0)
381+
await vi.advanceTimersByTimeAsync(100)
382+
expect(config.signal?.aborted).toBe(false)
383+
} finally {
384+
vi.useRealTimers()
385+
}
386+
})
387+
388+
test("should cleanup timeouts from repeated addMergedSignal calls", () => {
389+
vi.useFakeTimers()
390+
try {
391+
const internalController = new AbortController()
392+
const builder = new RequestConfigBuilder()
393+
394+
builder.addMergedSignal(internalController, undefined, 100)
395+
builder.addMergedSignal(internalController, undefined, 200)
396+
397+
expect(vi.getTimerCount()).toBe(2)
398+
builder.getCleanup()()
399+
expect(vi.getTimerCount()).toBe(0)
400+
} finally {
401+
vi.useRealTimers()
402+
}
403+
})
404+
405+
test("should immediately abort when metadata signal is already aborted", () => {
406+
const internalController = new AbortController()
407+
const externalController = new AbortController()
408+
externalController.abort()
409+
const builder = new RequestConfigBuilder()
410+
411+
builder.addMergedSignal(internalController, {
412+
taskId: "test-task",
413+
abortSignal: externalController.signal,
414+
})
415+
416+
const config = builder.build() as { signal?: AbortSignal }
362417
expect(config.signal?.aborted).toBe(true)
363-
builder.getCleanup()()
364-
vi.useRealTimers()
418+
expect(() => builder.getCleanup()()).not.toThrow()
365419
})
366420
})
367421

@@ -393,7 +447,7 @@ describe("RequestConfigBuilder", () => {
393447
expect(result.aborted).toBe(false)
394448
})
395449

396-
test("should abort merged signal when primarySignal is aborted", async () => {
450+
test("should abort merged signal when primarySignal is aborted", () => {
397451
const primaryController = new AbortController()
398452
const secondaryController = new AbortController()
399453

@@ -413,12 +467,10 @@ describe("RequestConfigBuilder", () => {
413467

414468
primaryController.abort()
415469

416-
// Wait for event to propagate
417-
await new Promise((resolve) => setTimeout(resolve, 10))
418470
expect(aborted).toBe(true)
419471
})
420472

421-
test("should abort merged signal when secondarySignal is aborted", async () => {
473+
test("should abort merged signal when secondarySignal is aborted", () => {
422474
const primaryController = new AbortController()
423475
const secondaryController = new AbortController()
424476

@@ -438,12 +490,10 @@ describe("RequestConfigBuilder", () => {
438490

439491
secondaryController.abort()
440492

441-
// Wait for event to propagate
442-
await new Promise((resolve) => setTimeout(resolve, 10))
443493
expect(aborted).toBe(true)
444494
})
445495

446-
test("should not abort merged signal when neither signal is aborted", async () => {
496+
test("should not abort merged signal when neither signal is aborted", () => {
447497
const primaryController = new AbortController()
448498
const secondaryController = new AbortController()
449499

@@ -461,7 +511,6 @@ describe("RequestConfigBuilder", () => {
461511
{ once: true },
462512
)
463513

464-
await new Promise((resolve) => setTimeout(resolve, 10))
465514
expect(aborted).toBe(false)
466515
})
467516

@@ -509,7 +558,7 @@ describe("RequestConfigBuilder", () => {
509558
test("should handle empty builder through full lifecycle", () => {
510559
const builder = new RequestConfigBuilder()
511560
expect(builder.build()).toBeUndefined()
512-
expect(builder.getOption("anyKey" as any)).toBeUndefined()
561+
expect(builder.getOption("anyKey")).toBeUndefined()
513562
})
514563

515564
test("should work with custom default options type", () => {

‎src/api/providers/config-builder/request-config-builder.ts‎

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,17 @@ import { mergeAbortSignalAndTimeout, mergeAbortSignals } from "../utils/abort-si
1111
* - Header merging
1212
* - Static factory methods
1313
*/
14-
export class RequestConfigBuilder<TOptions extends Record<string, any> = Record<string, any>> {
15-
protected options: TOptions
14+
type RequestConfigOptions = {
15+
headers?: Record<string, string>
16+
signal?: AbortSignal
17+
} & Record<string, unknown>
18+
19+
export class RequestConfigBuilder<TOptions extends RequestConfigOptions = RequestConfigOptions> {
20+
protected options: Partial<TOptions>
1621
private cleanupFn: () => void = () => {}
1722

1823
constructor(defaultOptions?: Partial<TOptions>) {
19-
this.options = (defaultOptions ? { ...defaultOptions } : {}) as TOptions
24+
this.options = defaultOptions ? { ...defaultOptions } : {}
2025
}
2126

2227
/**
@@ -30,7 +35,7 @@ export class RequestConfigBuilder<TOptions extends Record<string, any> = Record<
3035
return this
3136
}
3237

33-
this.options = { ...this.options, signal: metadata.abortSignal } as TOptions
38+
this.options = { ...this.options, signal: metadata.abortSignal }
3439
return this
3540
}
3641

@@ -45,8 +50,8 @@ export class RequestConfigBuilder<TOptions extends Record<string, any> = Record<
4550
return this
4651
}
4752

48-
const existingHeaders = (this.options as any).headers ?? {}
49-
this.options = { ...this.options, headers: { ...existingHeaders, ...headers } } as TOptions
53+
const existingHeaders = this.options.headers ?? {}
54+
this.options = { ...this.options, headers: { ...existingHeaders, ...headers } }
5055
return this
5156
}
5257

@@ -69,8 +74,12 @@ export class RequestConfigBuilder<TOptions extends Record<string, any> = Record<
6974
const merged = mergeAbortSignalAndTimeout(metadata?.abortSignal, timeoutMs)
7075
const signal = mergeAbortSignals(internalController.signal, merged.signal)
7176

72-
this.options = { ...this.options, signal } as TOptions
73-
this.cleanupFn = merged.cleanup
77+
this.options = { ...this.options, signal }
78+
const previousCleanup = this.cleanupFn
79+
this.cleanupFn = () => {
80+
previousCleanup()
81+
merged.cleanup()
82+
}
7483
return this
7584
}
7685

@@ -95,7 +104,7 @@ export class RequestConfigBuilder<TOptions extends Record<string, any> = Record<
95104
return this
96105
}
97106

98-
this.options = { ...this.options, [key]: value } as TOptions
107+
this.options = { ...this.options, [key]: value }
99108
return this
100109
}
101110

@@ -133,7 +142,7 @@ export class RequestConfigBuilder<TOptions extends Record<string, any> = Record<
133142
* @param extraOptions - Additional options to merge
134143
* @returns The built configuration or undefined if empty
135144
*/
136-
static fromMetadata<TOptions extends Record<string, any> = Record<string, any>>(
145+
static fromMetadata<TOptions extends RequestConfigOptions = RequestConfigOptions>(
137146
metadata?: ApiHandlerCreateMessageMetadata,
138147
extraOptions?: Partial<TOptions>,
139148
): TOptions | undefined {

0 commit comments

Comments
 (0)