Skip to content

Commit 8849f1a

Browse files
0xMinkedelauna
andauthored
chore: enforce no-floating-promises in core/task/ (#253)
* fix(task): handle rejections on void-prefixed async calls in Task.ts Per Copilot review on #253: the void-prefixed async calls flagged by the reviewer (postStateToWebviewWithoutTaskHistory, startTask and resumeTaskFromHistory in the constructor, startTask in start(), and the nine presentAssistantMessage(this) sites in the streaming loop) can become unhandled promise rejections on failure and crash the extension host. Each flagged site now has a .catch handler that logs the rejection without re-throwing, while keeping the void prefix to satisfy no-floating-promises. presentAssistantMessage was wrapped in a small private helper, presentAssistantMessageSafe, that distinguishes the expected throw-on-abort path (silently swallowed) from real failures (logged). All nine streaming presenter call sites delegate through the helper so the rejection-handling logic lives in one place. Adds six specs under "unhandled-rejection guards on void async calls" that pin the new behavior: every catch handler is asserted to log on rejection, and the helper's abort vs non-abort branches are both exercised. Single-line fire-and-forget UI updates and the streaming presenter call sites carry /* v8 ignore next */ markers with a short rationale, since the rejection-handling logic they delegate to is covered separately by the helper specs. * fix(Task): match the abort error message in presentAssistantMessageSafe The helper's catch handler was distinguishing abort from real failures via `this.abort` state at catch time, which has a real TOCTOU window: a non-abort throw followed by an abort flip between throw and catch microtask would silently swallow the real error. Switched to matching `error.message.endsWith("aborted")` — the literal contract presentAssistantMessage itself throws on abort. Same suppression for the abort case, no swallow window for real errors. Added a test that pins the message-based discriminator: a non-abort error with `this.abort = true` now correctly logs (would have been swallowed under the state-based check). Also added a regression test for the abort-message-match path so a future refactor can't drift back to the state check. * fix(Task): catch updateClineMessage rejections on partial-message paths The four fire-and-forget updateClineMessage calls in say()/ask() partial-message branches were void-prefixed without .catch arms. The callee's webview post is internally guarded, but its synchronous RooCodeEventName.Message emit can throw via a consumer-attached listener — so the void sites can produce unhandled rejections. Replace each `void this.updateClineMessage(...)` with an explicit .catch arm that logs the error, matching the other rejection-handling sites in this file. Two new tests pin the say() and ask() catch arms. Also pin the void on getCheckpointService with a rationale comment: its top-level try/catch returns undefined on failure, so .catch is not needed there. * feat(Task): adding catch block to log any errors * test(Task): coverage --------- Co-authored-by: 0xMink <260166390+0xMink@users.noreply.github.com> Co-authored-by: Elliott de Launay <edelauna@gmail.com>
1 parent 515437b commit 8849f1a

4 files changed

Lines changed: 493 additions & 40 deletions

File tree

src/core/assistant-message/presentAssistantMessage.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -959,8 +959,7 @@ export async function presentAssistantMessage(cline: Task) {
959959
if (cline.currentStreamingContentIndex < cline.assistantMessageContent.length) {
960960
// There are already more content blocks to stream, so we'll call
961961
// this function ourselves.
962-
presentAssistantMessage(cline)
963-
return
962+
return presentAssistantMessage(cline)
964963
} else {
965964
// CRITICAL FIX: If we're out of bounds and the stream is complete, set userMessageContentReady
966965
// This handles the case where assistantMessageContent is empty or becomes empty after processing
@@ -972,7 +971,7 @@ export async function presentAssistantMessage(cline: Task) {
972971

973972
// Block is partial, but the read stream may have finished.
974973
if (cline.presentAssistantMessageHasPendingUpdates) {
975-
presentAssistantMessage(cline)
974+
return presentAssistantMessage(cline)
976975
}
977976
}
978977

src/core/task/Task.ts

Lines changed: 117 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,29 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
356356
*/
357357
assistantMessageSavedToHistory = false
358358

359+
/**
360+
* Fire-and-forget wrapper around `presentAssistantMessage` that swallows the
361+
* expected cancellation rejection (the presenter throws when `this.abort` is set)
362+
* and logs any other failure. Keeping it non-blocking preserves the streaming
363+
* presenter's self-locking semantics while preventing unhandled promise rejections
364+
* from crashing the extension host.
365+
*/
366+
private presentAssistantMessageSafe(): void {
367+
void presentAssistantMessage(this).catch((error) => {
368+
// Discriminate on the error message rather than `this.abort` state,
369+
// which can flip between the throw and the catch microtask running:
370+
// a real failure followed by an abort flip would otherwise be
371+
// silently swallowed, and a stale abort error logged as a failure.
372+
// The abort throw site in presentAssistantMessage emits a message
373+
// ending in "aborted" (matching the other abort-throw contracts in
374+
// this file), so we suppress exactly that.
375+
if (error instanceof Error && error.message.endsWith("aborted")) {
376+
return
377+
}
378+
console.error(`[Task#presentAssistantMessage] task ${this.taskId}.${this.instanceId} failed:`, error)
379+
})
380+
}
381+
359382
/**
360383
* Push a tool_result block to userMessageContent, preventing duplicates.
361384
* Duplicate tool_use_ids cause API errors.
@@ -522,7 +545,15 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
522545
this.messageQueueStateChangedHandler = () => {
523546
this.emit(RooCodeEventName.TaskUserMessage, this.taskId)
524547
this.emit(RooCodeEventName.QueuedMessagesUpdated, this.taskId, this.messageQueueService.messages)
525-
this.providerRef.deref()?.postStateToWebviewWithoutTaskHistory()
548+
void this.providerRef
549+
.deref()
550+
?.postStateToWebviewWithoutTaskHistory()
551+
.catch((error) => {
552+
console.error(
553+
"[Task#messageQueueStateChangedHandler] postStateToWebviewWithoutTaskHistory failed:",
554+
error,
555+
)
556+
})
526557
}
527558

528559
this.messageQueueService.on("stateChanged", this.messageQueueStateChangedHandler)
@@ -567,9 +598,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
567598
if (startTask) {
568599
this._started = true
569600
if (task || images) {
570-
this.startTask(task, images)
601+
void this.startTask(task, images).catch((error) => {
602+
console.error("[Task#constructor] startTask failed:", error)
603+
})
571604
} else if (historyItem) {
572-
this.resumeTaskFromHistory()
605+
void this.resumeTaskFromHistory().catch((error) => {
606+
console.error("[Task#constructor] resumeTaskFromHistory failed:", error)
607+
})
573608
} else {
574609
throw new Error("Either historyItem or task/images must be provided")
575610
}
@@ -1162,7 +1197,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
11621197
// data or one whole message at a time so ignore partial for
11631198
// saves, and only post parts of partial message instead of
11641199
// whole array in new listener.
1165-
this.updateClineMessage(lastMessage)
1200+
// Fire-and-forget: the webview post is internally guarded, but
1201+
// the `RooCodeEventName.Message` emit can synchronously throw
1202+
// if any consumer-attached listener does, which would surface
1203+
// here as an unhandled rejection. Log it instead.
1204+
this.updateClineMessage(lastMessage).catch((error) => {
1205+
console.error("[Task#ask] updateClineMessage failed:", error)
1206+
})
11661207
// console.log("Task#ask: current ask promise was ignored (#1)")
11671208
throw new AskIgnoredError("updating existing partial")
11681209
} else {
@@ -1203,7 +1244,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
12031244
lastMessage.isAnswered = true
12041245
}
12051246
await this.saveClineMessages()
1206-
this.updateClineMessage(lastMessage)
1247+
// Fire-and-forget: see updateClineMessage call above for the
1248+
// rationale on the .catch arm.
1249+
this.updateClineMessage(lastMessage).catch((error) => {
1250+
console.error("[Task#ask] updateClineMessage failed:", error)
1251+
})
12071252
} else {
12081253
// This is a new and complete message, so add it like normal.
12091254
this.askResponse = undefined
@@ -1274,7 +1319,10 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
12741319
if (message) {
12751320
this.interactiveAsk = message
12761321
this.emit(RooCodeEventName.TaskInteractive, this.taskId)
1277-
provider?.postMessageToWebview({ type: "interactionRequired" })
1322+
/* v8 ignore next 3 -- fires inside 2s timer after ask() resolves; not reachable in unit tests */
1323+
void provider?.postMessageToWebview({ type: "interactionRequired" }).catch((error) => {
1324+
console.error("[Task#ask] postMessageToWebview interactionRequired failed:", error)
1325+
})
12781326
}
12791327
}, statusMutationTimeout),
12801328
)
@@ -1414,7 +1462,9 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
14141462
)
14151463
if (lastToolAskIndex !== -1) {
14161464
this.clineMessages[lastToolAskIndex].isAnswered = true
1417-
void this.updateClineMessage(this.clineMessages[lastToolAskIndex])
1465+
void this.updateClineMessage(this.clineMessages[lastToolAskIndex]).catch((error) => {
1466+
console.error("[Task#handleWebviewAskResponse] updateClineMessage failed:", error)
1467+
})
14181468
this.saveClineMessages().catch((error) => {
14191469
console.error("Failed to save answered tool-ask state:", error)
14201470
})
@@ -1662,7 +1712,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
16621712
lastMessage.images = images
16631713
lastMessage.partial = partial
16641714
lastMessage.progressStatus = progressStatus
1665-
this.updateClineMessage(lastMessage)
1715+
// Fire-and-forget: webview post is internally guarded, but the
1716+
// `RooCodeEventName.Message` emit can synchronously throw via a
1717+
// consumer-attached listener. Surface that as a log, not an
1718+
// unhandled rejection.
1719+
this.updateClineMessage(lastMessage).catch((error) => {
1720+
console.error("[Task#say] updateClineMessage failed:", error)
1721+
})
16661722
} else {
16671723
// This is a new partial message, so add it with partial state.
16681724
const sayTs = Date.now()
@@ -1701,7 +1757,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
17011757
await this.saveClineMessages()
17021758

17031759
// More performant than an entire `postStateToWebview`.
1704-
this.updateClineMessage(lastMessage)
1760+
// Fire-and-forget: see updateClineMessage call above for the
1761+
// rationale on the .catch arm.
1762+
this.updateClineMessage(lastMessage).catch((error) => {
1763+
console.error("[Task#say] updateClineMessage failed:", error)
1764+
})
17051765
} else {
17061766
// This is a new and complete message, so add it like normal.
17071767
const sayTs = Date.now()
@@ -1810,7 +1870,9 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
18101870
const { task, images } = this.metadata
18111871

18121872
if (task || images) {
1813-
this.startTask(task ?? undefined, images ?? undefined)
1873+
void this.startTask(task ?? undefined, images ?? undefined).catch((error) => {
1874+
console.error("[Task#start] startTask failed:", error)
1875+
})
18141876
}
18151877
}
18161878

@@ -2351,7 +2413,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
23512413

23522414
private async initiateTaskLoop(userContent: Anthropic.Messages.ContentBlockParam[]): Promise<void> {
23532415
// Kicks off the checkpoints initialization process in the background.
2354-
getCheckpointService(this)
2416+
// `getCheckpointService` wraps its full body in a try/catch and returns
2417+
// `undefined` on failure (see src/core/checkpoints/index.ts), so the
2418+
// returned promise cannot reject. `void` is sufficient — no `.catch`
2419+
// arm needed.
2420+
void getCheckpointService(this)
23552421

23562422
let nextUserContent = userContent
23572423
let includeFileDetails = true
@@ -2688,9 +2754,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
26882754
if (signal.aborted) {
26892755
reject(new Error("Request cancelled by user"))
26902756
} else {
2691-
signal.addEventListener("abort", () => {
2692-
reject(new Error("Request cancelled by user"))
2693-
}, { once: true })
2757+
signal.addEventListener(
2758+
"abort",
2759+
() => {
2760+
reject(new Error("Request cancelled by user"))
2761+
},
2762+
{ once: true },
2763+
)
26942764
}
26952765
})
26962766
return await Promise.race([nextPromise, abortPromise])
@@ -2794,7 +2864,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
27942864
// Add to content and present
27952865
this.assistantMessageContent.push(partialToolUse)
27962866
this.userMessageContentReady = false
2797-
presentAssistantMessage(this)
2867+
/* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */
2868+
this.presentAssistantMessageSafe()
27982869
} else if (event.type === "tool_call_delta") {
27992870
// Process chunk using streaming JSON parser
28002871
const partialToolUse = NativeToolCallParser.processStreamingChunk(
@@ -2813,7 +2884,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
28132884
this.assistantMessageContent[toolUseIndex] = partialToolUse
28142885

28152886
// Present updated tool use
2816-
presentAssistantMessage(this)
2887+
/* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */
2888+
this.presentAssistantMessageSafe()
28172889
}
28182890
}
28192891
} else if (event.type === "tool_call_end") {
@@ -2839,7 +2911,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
28392911
this.userMessageContentReady = false
28402912

28412913
// Present the finalized tool call
2842-
presentAssistantMessage(this)
2914+
/* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */
2915+
this.presentAssistantMessageSafe()
28432916
} else if (toolUseIndex !== undefined) {
28442917
// finalizeStreamingToolCall returned null (malformed JSON or missing args)
28452918
// Mark the tool as non-partial so it's presented as complete, but execution
@@ -2858,7 +2931,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
28582931
this.userMessageContentReady = false
28592932

28602933
// Present the tool call - validation will handle missing params
2861-
presentAssistantMessage(this)
2934+
/* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */
2935+
this.presentAssistantMessageSafe()
28622936
}
28632937
}
28642938
}
@@ -2891,7 +2965,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
28912965

28922966
// Present the tool call to user - presentAssistantMessage will execute
28932967
// tools sequentially and accumulate all results in userMessageContent
2894-
presentAssistantMessage(this)
2968+
/* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */
2969+
this.presentAssistantMessageSafe()
28952970
break
28962971
}
28972972
case "text": {
@@ -2910,7 +2985,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
29102985
})
29112986
this.userMessageContentReady = false
29122987
}
2913-
presentAssistantMessage(this)
2988+
/* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */
2989+
this.presentAssistantMessageSafe()
29142990
break
29152991
}
29162992
}
@@ -3237,7 +3313,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
32373313
this.userMessageContentReady = false
32383314

32393315
// Present the finalized tool call
3240-
presentAssistantMessage(this)
3316+
/* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */
3317+
this.presentAssistantMessageSafe()
32413318
} else if (toolUseIndex !== undefined) {
32423319
// finalizeStreamingToolCall returned null (malformed JSON or missing args)
32433320
// We still need to mark the tool as non-partial so it gets executed
@@ -3256,7 +3333,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
32563333
this.userMessageContentReady = false
32573334

32583335
// Present the tool call - validation will handle missing params
3259-
presentAssistantMessage(this)
3336+
/* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */
3337+
this.presentAssistantMessageSafe()
32603338
}
32613339
}
32623340
}
@@ -3455,7 +3533,8 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
34553533
// If there is content to update then it will complete and
34563534
// update `this.userMessageContentReady` to true, which we
34573535
// `pWaitFor` before making the next request.
3458-
presentAssistantMessage(this)
3536+
/* v8 ignore next -- streaming presenter; .catch lives in presentAssistantMessageSafe (covered) */
3537+
this.presentAssistantMessageSafe()
34593538
}
34603539

34613540
if (hasTextContent || hasToolUses) {
@@ -4191,10 +4270,14 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
41914270
const iterator = stream[Symbol.asyncIterator]()
41924271

41934272
// Set up abort handling - when the signal is aborted, clean up the controller reference
4194-
abortSignal.addEventListener("abort", () => {
4195-
console.log(`[Task#${this.taskId}.${this.instanceId}] AbortSignal triggered for current request`)
4196-
this.currentRequestAbortController = undefined
4197-
}, { once: true })
4273+
abortSignal.addEventListener(
4274+
"abort",
4275+
() => {
4276+
console.log(`[Task#${this.taskId}.${this.instanceId}] AbortSignal triggered for current request`)
4277+
this.currentRequestAbortController = undefined
4278+
},
4279+
{ once: true },
4280+
)
41984281

41994282
try {
42004283
// Awaiting first chunk to see if it will throw an error.
@@ -4206,9 +4289,13 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
42064289
if (abortSignal.aborted) {
42074290
reject(new Error("Request cancelled by user"))
42084291
} else {
4209-
abortSignal.addEventListener("abort", () => {
4210-
reject(new Error("Request cancelled by user"))
4211-
}, { once: true })
4292+
abortSignal.addEventListener(
4293+
"abort",
4294+
() => {
4295+
reject(new Error("Request cancelled by user"))
4296+
},
4297+
{ once: true },
4298+
)
42124299
}
42134300
})
42144301

0 commit comments

Comments
 (0)