Skip to content

Commit 8b80116

Browse files
RenKoya1claude
andcommitted
fix(server): await _handleProcessingError so blocking drains surface errors
`_processEvents` catches errors from the event-processing loop and delegates to `_handleProcessingError`, which — per its own comment — re-throws on the blocking path (no `firstResultRejector`) "so the caller's await catches it". The call was not awaited. `_handleProcessingError` is async, so its throw became a floating rejection instead of propagating out of `_processEvents`. The generator resolved as if the drain had succeeded, and the one blocking caller with no `firstResultRejector` — `cancelTask`'s drain — silently swallowed the failure. It then reloaded the task, saw a non-canceled state, and threw a misleading `TaskNotCancelableError` (masking the real error), while the true error surfaced as an unhandled promise rejection. Await the call. The blocking re-throw now propagates through the `finally` (which still detaches the queue) and out to the awaiting caller, matching the documented intent. The non-blocking paths are unaffected: they pass a `firstResultRejector`, so `_handleProcessingError` rejects/persists rather than throwing. Adds a regression test asserting `cancelTask` surfaces a drain-time persistence error instead of masking it with `TaskNotCancelableError`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 477e394 commit 8b80116

2 files changed

Lines changed: 64 additions & 1 deletion

File tree

src/server/request_handler/default_request_handler.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,11 @@ export class DefaultRequestHandler implements A2ARequestHandler {
322322
}
323323
} catch (error) {
324324
console.error(`Event processing loop failed for task ${taskId}:`, error);
325-
this._handleProcessingError(
325+
// Must be awaited: `_handleProcessingError` re-throws in the blocking
326+
// (no `firstResultRejector`) case so the caller's `await` catches it.
327+
// Without `await`, that throw escapes as a floating rejection and the
328+
// caller (e.g. `cancelTask`'s drain) resolves as if nothing failed.
329+
await this._handleProcessingError(
326330
error,
327331
resultManager,
328332
firstResultSent,

test/server/default_request_handler.spec.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3277,6 +3277,65 @@ describe('DefaultRequestHandler as A2ARequestHandler', () => {
32773277
}
32783278
});
32793279

3280+
it('cancelTask: should surface an error thrown while draining the cancellation', async () => {
3281+
// Regression: `_processEvents` re-throws (via `_handleProcessingError`)
3282+
// on the blocking drain path used by `cancelTask`. That handler must be
3283+
// awaited, otherwise the throw escapes as a floating rejection, the drain
3284+
// resolves as if it succeeded, and `cancelTask` masks the real failure
3285+
// with a misleading `TaskNotCancelableError`.
3286+
vi.useFakeTimers();
3287+
const cancellableExecutor = new CancellableMockAgentExecutor();
3288+
handler = new DefaultRequestHandler(
3289+
testAgentCard,
3290+
mockTaskStore,
3291+
cancellableExecutor,
3292+
executionEventBusManager
3293+
);
3294+
3295+
const streamParams: SendMessageRequest = {
3296+
message: createTestMessage('msg-cancel-drain', 'Start and cancel'),
3297+
} as SendMessageRequest;
3298+
const streamGenerator = handler.sendMessageStream(streamParams, serverCallContext);
3299+
3300+
const streamEvents: any[] = [];
3301+
(async () => {
3302+
for await (const event of streamGenerator) {
3303+
streamEvents.push(event);
3304+
}
3305+
})().catch(() => {
3306+
// The injected persistence failure also surfaces on the stream; ignore.
3307+
});
3308+
3309+
// Allow the task to be created and reach TASK_STATE_WORKING.
3310+
await vi.advanceTimersByTimeAsync(25);
3311+
3312+
const createdTaskEvent = streamEvents.find((e) => e.payload?.$case === 'task');
3313+
assert.isDefined(createdTaskEvent, 'Task creation event should have been received');
3314+
const taskId = createdTaskEvent.payload.value.id;
3315+
3316+
// Inject a failure when the cancellation (CANCELED) state is persisted
3317+
// during the drain. Earlier SUBMITTED/WORKING saves already succeeded.
3318+
const drainError = new Error('drain persistence failed');
3319+
const realSave = mockTaskStore.save.bind(mockTaskStore);
3320+
vi.spyOn(mockTaskStore, 'save').mockImplementation(async (task, ctx) => {
3321+
if (task.status?.state === TaskState.TASK_STATE_CANCELED) {
3322+
throw drainError;
3323+
}
3324+
return realSave(task, ctx);
3325+
});
3326+
3327+
// The real drain failure must surface, not a masking TaskNotCancelableError.
3328+
// Build the rejection assertion before running timers so the rejection is
3329+
// observed as soon as it happens (no floating unhandled rejection).
3330+
const cancelPromise = handler.cancelTask(
3331+
{ id: taskId, tenant: '', metadata: {} },
3332+
serverCallContext
3333+
);
3334+
const rejectsWithDrainError = expect(cancelPromise).rejects.toBe(drainError);
3335+
await vi.runAllTimersAsync();
3336+
await rejectsWithDrainError;
3337+
});
3338+
32803339
it('cancelTask: should fail for tasks in a terminal state', async () => {
32813340
const taskId = 'task-terminal';
32823341
const fakeTask: Task = {

0 commit comments

Comments
 (0)