Skip to content

Commit 5833652

Browse files
authored
fix(server): await _handleProcessingError so blocking drains surface errors (#579)
# Description `_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"*: ```ts } catch (error) { console.error(`Event processing loop failed for task ${taskId}:`, error); this._handleProcessingError( // <-- not awaited error, resultManager, firstResultSent, taskId, options?.firstResultRejector, ); } finally { eventQueue.stop(); } ``` ```ts // Blocking: re-throw so the caller's await catches it. if (!firstResultRejector) { throw error; } ``` The call is **not awaited**. `_handleProcessingError` is `async`, so its `throw` becomes a floating rejection instead of propagating out of `_processEvents`. The generator resolves as if the drain had succeeded. The one blocking caller that passes no `firstResultRejector` is `cancelTask`'s drain: ```ts await this._processEvents( taskId, new ResultManager(this.taskStore, context), eventQueue, context, ); ``` So when the drain throws (e.g. the task store fails while persisting the cancellation), `cancelTask` silently swallows the real error, reloads the task, sees a non-canceled state, and throws a misleading `TaskNotCancelableError` — while the true error surfaces as an unhandled promise rejection. ## Fix Await `_handleProcessingError`. 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: `sendMessage`/`sendMessageStream` pass a `firstResultRejector`, so `_handleProcessingError` rejects or persists a FAILED status rather than throwing. ## Tests Added a regression test asserting `cancelTask` surfaces a drain-time persistence error instead of masking it with `TaskNotCancelableError`. Verified it fails without the fix (throws `TaskNotCancelableError`) and passes with it. Full suite green (1368 tests). - [x] Follows the `CONTRIBUTING` guide - [x] PR title uses Conventional Commits (`fix:`) - [x] Tests and linter pass - [ ] Docs updated (not necessary)
1 parent 477e394 commit 5833652

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)