-
Notifications
You must be signed in to change notification settings - Fork 110
fix: Wait for agent completion and ensure all events processed in blo… #431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @kabir, 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 resolves a critical race condition affecting blocking calls, ensuring that 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
@flex-myeonghyeon I moved the work here
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses a critical race condition in blocking calls by introducing a synchronization mechanism to ensure all events are processed before returning a response. The solution is well-structured and the accompanying comments clearly explain the logic. The new unit tests are comprehensive and correctly validate both the fix for blocking calls and the fire-and-forget scenario. I've identified a few minor areas for improvement, mainly related to code style and clarity, such as replacing magic numbers with constants and simplifying some expressions.
server-common/src/main/java/io/a2a/server/requesthandlers/DefaultRequestHandler.java
Show resolved
Hide resolved
server-common/src/main/java/io/a2a/server/requesthandlers/DefaultRequestHandler.java
Outdated
Show resolved
Hide resolved
server-common/src/test/java/io/a2a/server/requesthandlers/DefaultRequestHandlerTest.java
Outdated
Show resolved
Hide resolved
server-common/src/test/java/io/a2a/server/requesthandlers/DefaultRequestHandlerTest.java
Outdated
Show resolved
Hide resolved
server-common/src/test/java/io/a2a/server/requesthandlers/DefaultRequestHandlerTest.java
Outdated
Show resolved
Hide resolved
|
Replaces #430 |
b79aee8 to
c97d236
Compare
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(DefaultRequestHandler.class); | ||
| private static final int AGENT_COMPLETION_TIMEOUT_SECONDS = 5; | ||
| private static final int CONSUMPTION_COMPLETION_TIMEOUT_SECONDS = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some cases, a longer wait time might be necessary.
Would it be possible to make this configurable — for example, by passing it as a parameter or injecting it through the DefaultRequestHandler constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flex-myeonghyeon Good idea, I've added some MP Config properties
f487663 to
6e35990
Compare
|
@kabir |
6e35990 to
413b7ce
Compare
…cking calls Fixes race condition where DefaultRequestHandler.onMessageSend() returns before all task events are fully processed and persisted to TaskStore, resulting in incomplete Task objects being returned to clients (missing artifacts, incorrect state). Root Cause: - Blocking calls interrupted immediately after first event and returned to client before background event consumption completed - Agent execution and event processing happened asynchronously in background - No synchronization to ensure all events were consumed and persisted before returning Task to client Solution (4-step process): 1. Wait for agent to finish enqueueing events (5s timeout) 2. Close the queue to signal consumption can complete (breaks dependency) 3. Wait for consumption to finish processing events (2s timeout) 4. Fetch final task state from TaskStore (has all artifacts and correct state) This ensures blocking calls return complete Task objects with all artifacts and correct state, including support for fire-and-forget tasks that never emit final state events. Added unit tests: - testBlockingFireAndForgetReturnsNonFinalTask: Validates fire-and-forget pattern - testBlockingCallReturnsCompleteTaskWithArtifacts: Ensures all artifacts included
413b7ce to
ece9af8
Compare
server-common/src/main/java/io/a2a/server/requesthandlers/DefaultRequestHandler.java
Show resolved
Hide resolved
2e48d12 to
e2498c7
Compare
e2498c7 to
fab40d7
Compare
#441) …cking calls Fixes race condition where DefaultRequestHandler.onMessageSend() returns before all task events are fully processed and persisted to TaskStore, resulting in incomplete Task objects being returned to clients (missing artifacts, incorrect state). Root Cause: - Blocking calls interrupted immediately after first event and returned to client before background event consumption completed - Agent execution and event processing happened asynchronously in background - No synchronization to ensure all events were consumed and persisted before returning Task to client Solution (4-step process): 1. Wait for agent to finish enqueueing events (5s timeout) 2. Close the queue to signal consumption can complete (breaks dependency) 3. Wait for consumption to finish processing events (2s timeout) 4. Fetch final task state from TaskStore (has all artifacts and correct state) This ensures blocking calls return complete Task objects with all artifacts and correct state, including support for fire-and-forget tasks that never emit final state events. Added unit tests: - testBlockingFireAndForgetReturnsNonFinalTask: Validates fire-and-forget pattern - testBlockingCallReturnsCompleteTaskWithArtifacts: Ensures all artifacts included Upstream: #431
a2aproject#431) …cking calls Fixes race condition where DefaultRequestHandler.onMessageSend() returns before all task events are fully processed and persisted to TaskStore, resulting in incomplete Task objects being returned to clients (missing artifacts, incorrect state). Root Cause: - Blocking calls interrupted immediately after first event and returned to client before background event consumption completed - Agent execution and event processing happened asynchronously in background - No synchronization to ensure all events were consumed and persisted before returning Task to client Solution (4-step process): 1. Wait for agent to finish enqueueing events (configurable timeout) 2. Close the queue to signal consumption can complete (breaks dependency) 3. Wait for consumption to finish processing events (configurable timeout) 4. Fetch final task state from TaskStore (has all artifacts and correct state) This ensures blocking calls return complete Task objects with all artifacts and correct state, including support for fire-and-forget tasks that never emit final state events. Added unit tests: - testBlockingFireAndForgetReturnsNonFinalTask: Validates fire-and-forget pattern - testBlockingCallReturnsCompleteTaskWithArtifacts: Ensures all artifacts included Fixes a2aproject#428 🦕
…cking calls
Fixes race condition where DefaultRequestHandler.onMessageSend() returns before all task events are fully processed and persisted to TaskStore, resulting in incomplete Task objects being returned to clients (missing artifacts, incorrect state).
Root Cause:
Solution (4-step process):
This ensures blocking calls return complete Task objects with all artifacts and correct state, including support for fire-and-forget tasks that never emit final state events.
Added unit tests:
Fixes #428 🦕