fix: propagate executor errors instead of synthesizing failed tasks - #567
fix: propagate executor errors instead of synthesizing failed tasks#567JakubWorek wants to merge 1 commit into
Conversation
…d of masking them as failed tasks
🧪 Code Coverage
Generated by coverage-comment.yml |
There was a problem hiding this comment.
Code Review
This pull request refactors agent execution error handling by introducing an internal 'error' event on the event bus. Instead of synthesizing FAILED events in the handler, the queue rethrows the wrapped error, allowing the drain loop to persist the FAILED status and propagate the original exception to the transport layer for proper error envelope generation. The review feedback correctly identifies a critical issue in both _runExecutor and _runStreamExecutor where the hadError parameter of _settleBus is hardcoded to false in the .finally block, which prevents proper cleanup and can leave the event bus dangling on executor rejection.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| this.agentExecutor | ||
| .execute(requestContext, eventBus) | ||
| .catch((err: unknown) => { | ||
| // Promises can reject with any value, so coerce defensively | ||
| // before reading `.message`. | ||
| const errorMessage = extractErrorMessage(err); | ||
| console.error(`Agent execution failed for message ${finalMessageForAgent.messageId}:`, err); | ||
| // The synthetic Task id MUST be `requestContext.taskId` — the | ||
| // id the bus is registered under and the id we hand back to | ||
| // the client. A fresh uuid would make the returned Task | ||
| // unreachable via `getTask`. | ||
| const errorTask: Task = { | ||
| id: requestContext.taskId, | ||
| contextId: finalMessageForAgent.contextId!, | ||
| status: { | ||
| state: TaskState.TASK_STATE_FAILED, | ||
| message: { | ||
| role: Role.ROLE_AGENT, | ||
| messageId: uuidv4(), | ||
| taskId: requestContext.taskId, | ||
| contextId: finalMessageForAgent.contextId!, | ||
| parts: [ | ||
| { | ||
| content: { $case: 'text', value: `Agent execution error: ${errorMessage}` }, | ||
| mediaType: 'text/plain', | ||
| filename: '', | ||
| metadata: {}, | ||
| }, | ||
| ], | ||
| metadata: {}, | ||
| extensions: [], | ||
| referenceTaskIds: [], | ||
| }, | ||
| timestamp: new Date().toISOString(), | ||
| }, | ||
| artifacts: [], | ||
| history: requestContext.task?.history ? [...requestContext.task.history] : [], | ||
| metadata: {}, | ||
| }; | ||
| if ( | ||
| finalMessageForAgent && | ||
| !errorTask.history?.find((m) => m.messageId === finalMessageForAgent.messageId) | ||
| ) { | ||
| errorTask.history?.push(finalMessageForAgent); | ||
| } | ||
| eventBus.publish(AgentEvent.task(errorTask)); | ||
| eventBus.publish( | ||
| AgentEvent.statusUpdate({ | ||
| taskId: errorTask.id, | ||
| contextId: errorTask.contextId, | ||
| status: errorTask.status, | ||
| metadata: {}, | ||
| }) | ||
| ); | ||
| // Publish an internal error event so the drain loop's | ||
| // try/catch observes the exception via `eventQueue.events()` | ||
| // rethrowing it. The drain loop then persists a FAILED status | ||
| // update and rethrows so the transport can build a proper | ||
| // error envelope. | ||
| eventBus.publish(AgentEvent.error(err)); | ||
| }) | ||
| .finally(() => { | ||
| // Close the bus for terminal tasks; keep it alive for | ||
| // INPUT_REQUIRED / AUTH_REQUIRED so follow-up sends and | ||
| // resubscribers can still attach. | ||
| this._settleBus(taskId, eventBus, stateTracker()); | ||
| // Force-close on error regardless of last observed state: | ||
| // errors are terminal and must not leave the bus dangling for | ||
| // an INPUT_REQUIRED / AUTH_REQUIRED holdover. | ||
| this._settleBus(taskId, eventBus, stateTracker(), false); | ||
| }); |
There was a problem hiding this comment.
The hadError parameter of _settleBus is hardcoded to false here. If the executor rejects with an error, this is a terminal failure and the event bus must be cleaned up immediately, even if the last observed state was an interrupted state (like INPUT_REQUIRED or AUTH_REQUIRED). Hardcoding false prevents the bus from being cleaned up in those cases, leaving it dangling.
We should track whether an error occurred and pass that state to _settleBus.
let hadError = false;
this.agentExecutor
.execute(requestContext, eventBus)
.catch((err: unknown) => {
hadError = true;
console.error('Agent execution failed for message ' + finalMessageForAgent.messageId + ':', err);
// Publish an internal error event so the drain loop's
// try/catch observes the exception via eventQueue.events()
// rethrowing it. The drain loop then persists a FAILED status
// update and rethrows so the transport can build a proper
// error envelope.
eventBus.publish(AgentEvent.error(err));
})
.finally(() => {
// Force-close on error regardless of last observed state:
// errors are terminal and must not leave the bus dangling for
// an INPUT_REQUIRED / AUTH_REQUIRED holdover.
this._settleBus(taskId, eventBus, stateTracker(), hadError);
});| this.agentExecutor | ||
| .execute(requestContext, eventBus) | ||
| .catch((err: unknown) => { | ||
| const errorMessage = extractErrorMessage(err); | ||
| console.error( | ||
| `Agent execution failed for stream message ${finalMessageForAgent.messageId}:`, | ||
| err | ||
| ); | ||
|
|
||
| const latestTask = snapshotTracker().task; | ||
| const errorTaskId = latestTask?.id ?? requestContext.taskId; | ||
| const errorContextId = latestTask?.contextId ?? finalMessageForAgent.contextId!; | ||
|
|
||
| // If no Task event has been published yet, synthesize one | ||
| // first so the SSE consumer's stream pattern transitions into | ||
| // TASK_LIFECYCLE before the statusUpdate(FAILED) lands. | ||
| // Otherwise the executor would silently close an empty stream | ||
| // and the client would have no signal that the request failed. | ||
| if (!latestTask) { | ||
| const errorTask: Task = { | ||
| id: requestContext.taskId, | ||
| contextId: finalMessageForAgent.contextId!, | ||
| status: { | ||
| state: TaskState.TASK_STATE_FAILED, | ||
| message: { | ||
| role: Role.ROLE_AGENT, | ||
| messageId: uuidv4(), | ||
| taskId: requestContext.taskId, | ||
| contextId: finalMessageForAgent.contextId!, | ||
| parts: [ | ||
| { | ||
| content: { $case: 'text', value: `Agent execution error: ${errorMessage}` }, | ||
| mediaType: 'text/plain', | ||
| filename: '', | ||
| metadata: {}, | ||
| }, | ||
| ], | ||
| metadata: {}, | ||
| extensions: [], | ||
| referenceTaskIds: [], | ||
| }, | ||
| timestamp: new Date().toISOString(), | ||
| }, | ||
| artifacts: [], | ||
| history: requestContext.task?.history ? [...requestContext.task.history] : [], | ||
| metadata: {}, | ||
| }; | ||
| if ( | ||
| finalMessageForAgent && | ||
| !errorTask.history?.find((m) => m.messageId === finalMessageForAgent.messageId) | ||
| ) { | ||
| errorTask.history?.push(finalMessageForAgent); | ||
| } | ||
| eventBus.publish(AgentEvent.task(errorTask)); | ||
| } | ||
|
|
||
| const errorTaskStatus: TaskStatusUpdateEvent = { | ||
| taskId: errorTaskId, | ||
| contextId: errorContextId, | ||
| status: { | ||
| state: TaskState.TASK_STATE_FAILED, | ||
| message: { | ||
| role: Role.ROLE_AGENT, | ||
| messageId: uuidv4(), | ||
| taskId: errorTaskId, | ||
| contextId: errorContextId, | ||
| parts: [ | ||
| { | ||
| content: { $case: 'text', value: `Agent execution error: ${errorMessage}` }, | ||
| mediaType: 'text/plain', | ||
| filename: '', | ||
| metadata: {}, | ||
| }, | ||
| ], | ||
| metadata: {}, | ||
| extensions: [], | ||
| referenceTaskIds: [], | ||
| }, | ||
| timestamp: new Date().toISOString(), | ||
| }, | ||
| metadata: {}, | ||
| }; | ||
| eventBus.publish(AgentEvent.statusUpdate(errorTaskStatus)); | ||
| eventBus.publish(AgentEvent.error(err)); | ||
| }) | ||
| .finally(() => { | ||
| this._settleBus(taskId, eventBus, snapshotTracker().state); | ||
| this._settleBus(taskId, eventBus, snapshotTracker().state, false); | ||
| }); |
There was a problem hiding this comment.
Similar to _runExecutor, the hadError parameter of _settleBus is hardcoded to false here. If the streaming executor rejects, the event bus must be cleaned up immediately to avoid leaving it dangling.
We should track whether an error occurred and pass that state to _settleBus.
let hadError = false;
this.agentExecutor
.execute(requestContext, eventBus)
.catch((err: unknown) => {
hadError = true;
console.error(
'Agent execution failed for stream message ' + finalMessageForAgent.messageId + ':',
err
);
eventBus.publish(AgentEvent.error(err));
})
.finally(() => {
this._settleBus(taskId, eventBus, snapshotTracker().state, hadError);
});
Description
Aligns JS SDK with Python SDK: when
AgentExecutor.execute()throws, the framework now persists the task as FAILED and propagates the exception to the transport, instead of silently returning a synthesized FAILED-Task as a successful result.Before: executor throws → framework catches → synthesizes Task{state=FAILED} → publishes on bus → returns as successful JSON-RPC result (HTTP 200).
After: executor throws → framework publishes internal AgentEvent.error(err) → ExecutionEventQueue.events() rethrows → drain loop catches → persists FAILED status to store → rethrows → transport maps to proper error envelope.
Fixes #370 🦕