fix(patch): cherry-pick 02995ba to release/v0.41.1-pr-26568 to patch version v0.41.1 and create version 0.41.2#26589
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a significant refactor of the tool execution synchronization logic within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Size Change: -4 B (0%) Total Size: 33.9 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Code Review
This pull request refactors the Task class in packages/a2a-server to use an EventEmitter for managing tool execution states, replacing the previous promise-based synchronization. This change improves the handling of complex tool lifecycles and race conditions. New test suites were introduced to verify these improvements, including scenarios for concurrent tool scheduling and cancellation. Review feedback highlighted a race condition in waitForPendingTools where concurrent waiters might fail to catch a cancellation error if it is cleared prematurely, and recommended resetting the cancellation state when new tool calls are registered to ensure the task can proceed with subsequent work.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/a2a-server/src/agent/task.ts (213-230)
There is a race condition in waitForPendingTools when multiple concurrent callers are waiting. If cancelPendingTools is called, this.cancellationError is set and an update event is emitted. All concurrent waiters will wake up and exit the loop. However, the first waiter to reach line 216 or 228 will clear this.cancellationError by setting it to undefined. Subsequent waiters will then see undefined and resolve successfully instead of throwing the cancellation error.
To fix this, avoid clearing the error within waitForPendingTools. Instead, you should clear it when new work is initiated (e.g., in _registerToolCall). Additionally, per repository rules, this asynchronous operation should ideally accept and propagate an AbortSignal to ensure consistent cancellation handling.
while (this.pendingToolCalls.size > 0 && !this.isAwaitingApprovalOnly()) {
if (this.cancellationError) {
throw this.cancellationError;
}
logger.info(
'[Task] Waiting for ' + this.pendingToolCalls.size + ' pending tool(s)...',
);
await new Promise((resolve) =>
this.toolUpdateEmitter.once('update', resolve),
);
}
if (this.cancellationError) {
throw this.cancellationError;
}References
- Asynchronous operations that can be cancelled by the user should accept and propagate an AbortSignal to ensure cancellability and prevent dangling processes or network requests.
- When managing the state of asynchronous operations, rely on an explicit state variable rather than checking for the existence of a promise object.
packages/a2a-server/src/agent/task.ts (179-181)
When new tool calls are registered, any previous cancellation error should be cleared to allow the task to proceed with new work. This also ensures that waitForPendingTools doesn't immediately throw an old error from a previous turn. This ensures explicit state management for the asynchronous operation's lifecycle.
private _registerToolCall(toolCallId: string, status: string): void {
this.cancellationError = undefined;
this.pendingToolCalls.set(toolCallId, status);
this.toolUpdateEmitter.emit('update');
}References
- When managing the state of asynchronous operations, rely on an explicit state variable rather than checking for the existence of a promise object.
This PR automatically cherry-picks commit 02995ba to patch version v0.41.1 in the stable release to create version 0.41.2.