Repository navigation
fix(server): keep follow-up message when the executor re-emits the current task - #1298
marcinbelczewski wants to merge 1 commit into
Conversation
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/server/agent_execution/active_task.py | 96.10% | 96.14% | 🟢 +0.04% |
| src/a2a/server/cluster/database_event_stream.py | 97.27% | 93.64% | 🔴 -3.64% |
| Total | 93.46% | 93.43% | 🔴 -0.04% |
Generated by coverage-comment.yml
b594be3 to
c799aa8
Compare
b8e91b6 to
38c00f8
Compare
| 'Task %s already exists. Ignoring task replacement.', | ||
| self.active_task._task_id, | ||
| ) | ||
| if self.message_to_save is None: |
There was a problem hiding this comment.
Ran this locally with an agent like the one in your test. Two things.
- The leading Task on the follow-up stream still has the old history:
['msg-start'].msg-resumeonly lands with the next status update. The final get_task is right, but the first streamed Task is what 3.1.2 is about. - On a follow-up
message_to_saveis still set for every Task, so enqueueingcurrent_tasktwice drops the second one with no ERROR.
Saving the message here fixes both. The first Task then has both messages, the double enqueue logs once, and tests/integration plus tests/server pass:
if existing_task:
if self.message_to_save is None:
logger.error(
'Task %s already exists. Ignoring task replacement.',
self.active_task._task_id,
)
else:
await self.active_task._task_manager.save_task_event(
self.active_task._task_manager.update_with_message(
self.message_to_save, existing_task
)
)
else:
await self.active_task._task_manager.save_task_event(event)
self.message_to_save = NoneThere was a problem hiding this comment.
@ConnorMoss02 thanks a lot for taking the time to actually run it and for the patch! You are right on both points and sorry for that. My test only checked that the stream starts with a Task and what ends up in history at the end, but never what is inside that first Task, which is the whole point of 3.1.2... And the claim that a second Task still logs the ERROR I based on test_scenario_enqueue_task_twice, which only covers a brand new task, not a follow-up. My bad.
I applied your approach with one small change: the follow-up message now goes through the same message_id check that _handle_task_modification_event already had (from #1106). I moved it to _add_message_to_history so both paths use it. Without it, if the executor enqueues current_task and the client resends the same follow-up message (same message_id), the message lands in history twice, while for an executor that only sends status updates it's saved once. Let me know if you'd rather keep it exactly as you proposed.
Tests:
test_scenario_streaming_resumption_enqueues_current_taskis now parametrized withonce/twice. It checks the history of the first streamedTaskand that the ERROR is logged exactly once when theTaskis enqueued twice.- New
test_scenario_resent_follow_up_message_saved_oncechecks that both executor patterns save a resent message only once.
I checked that the streaming test fails on the previous version and the resend one fails with your patch as is. I also broke each part of the change on purpose to make sure some test catches it. Squashed into a single commit and force pushed.
Thanks again for catching this!
There was a problem hiding this comment.
One more thing I noticed after posting the above. The first streamed Task is right only if the executor enqueues context.current_task itself (as in the migration guide), because that's the same object TaskManager keeps. If it enqueues a copy, history is fine but the first streamed Task still has only ['msg-start']. Subscribers get the raw executor event here:
a2a-python/src/a2a/server/agent_execution/active_task.py
Lines 280 to 286 in aa8e966
Looks like a leftover from #1053. Before it the consumer sent the stored task to subscribers, but maybe I'm missing something. Passing handled_event there fixes it and all tests pass, but it changes what's streamed for every Task event, so I'd rather not sneak it into this PR. Should I add it here or open a separate issue?
There was a problem hiding this comment.
Small update: the test now also has an agent message in the INPUT_REQUIRED status, to check the user 1 → agent → user 2 order.
068b73f to
f0fb030
Compare
f0fb030 to
bd0ecc6
Compare
Description
Problem
Spec 3.1.2: "If the agent returns a Task, the stream MUST begin with the Task object". On a follow-up to an existing task,
on_message_send_streamsubscribes withinclude_initial_task=False, so the leadingTaskhas to come from the executor. The v1.0 migration guide shows how:task = context.current_task or new_task_from_user_message(...), thenenqueue_event(task).When the task already exists,
EventConsumer._handle_initial_tasklogsTask ... already exists. Ignoring task replacement.at ERROR and clearsmessage_to_save. As a result, the follow-up user message never reachesTask.history.Fix
In
_handle_initial_task, when the task already exists:Taskis the first task event of the request), it's added to the stored task's history right away. The leading streamedTaskthen already has it. As with the other path,update_with_messagefirst moves the agent'sstatus.messageinto history, so the order is user 1, agent, user 2.Taskin the same request, the ERROR is logged as before.Both paths now share
_add_message_to_history, which is themessage_idcheck from #1106 moved out of_handle_task_modification_event. So a follow-up resent with the samemessage_idis saved once, no matter whether the executor enqueues theTaskor only status updates.Thanks @ConnorMoss02 for the review that led to this version.
Behaviour changes
Taskenqueued as the first event of a follow-up no longer logs an ERROR. Its content is still ignored, and the stored task is kept.Known limitation
The leading streamed
Taskincludes the follow-up message only when the executor enqueuescontext.current_taskitself, as the migration guide does. That object is the oneTaskManagerkeeps, andupdate_with_messageupdates it in place. If the executor enqueues a copy, history is still saved correctly, but the first streamedTaskis stale, same as onmain. Subscribers get the raw executor event in_process_event_inner. This is discussed in the review thread.Tests
In
tests/integration/test_scenarios.py:test_scenario_streaming_resumption_enqueues_current_task(once/twice): the executor enqueues the current task once or twice on the follow-up. It checks the event order, that the first streamedTaskhas historymsg-start,agent-ask,msg-resumeand nostatus.message, the final history, and that the replacement ERROR is logged 0 or 1 times.test_scenario_resent_follow_up_message_saved_once(status_only/current_task): resending the same follow-up adds it to history once for both executor patterns.On
main, both resumption cases andresent…[current_task]fail.resent…[status_only]passes there, because it covers the existing check that moved into the shared helper.resent…[current_task]also fails without themessage_idcheck on the new path.CONTRIBUTINGGuide.bash scripts/format.shfrom the repository root to format)Fixes #1285 🦕