-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: Add correlation ID support for idempotent task creation #939
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
base: main
Are you sure you want to change the base?
Conversation
- Add correlationIdRequired capability to AgentCapabilities - Add correlationId field to MessageSendParams - Add CorrelationIdAlreadyExistsError (-32008) for duplicate detection - Update all transport formats (JSON-RPC, gRPC, REST) - Add comprehensive spec documentation and examples Fixes a2aproject#928
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.
Summary of Changes
Hello @maeste, 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!
I've implemented support for correlation IDs to enable idempotent task creation within the A2A protocol. This enhancement addresses potential issues with duplicate operations caused by network failures or client crashes, ensuring that message/send requests can be safely retried.
Highlights
- New Capability for Agents: Agents can now declare
correlationIdRequiredin theirAgentCapabilitiesto indicate they require a correlation ID for new task creation requests. - Correlation ID in Message Sending: The
MessageSendParamsnow includes an optionalcorrelationIdfield, allowing clients to provide a unique identifier for idempotent operations. - New Error Type: A new A2A-specific error,
CorrelationIdAlreadyExistsError(-32008), has been introduced to signal when a provided correlation ID is already associated with an active task. - Protocol Specification Updates: The A2A specification has been comprehensively updated across JSON-RPC, gRPC, and REST formats, including detailed documentation on correlation ID behavior for agents, servers, and clients, along with example scenarios.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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 or fill out our survey to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 introduces support for idempotent task creation using a correlationId. The changes are comprehensive, updating the specification documents, gRPC proto files, JSON schema, and TypeScript types. The implementation is well-documented with clear explanations and examples. I have a couple of minor suggestions to improve clarity in the specification and fix a style issue in the proto file.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
I think the same ability can be achieved by |
+1 for using the existing identifier instead of creating a new one. Implementation-wise, I believe it's easier if we also allow users to specify
My hope is that we can arrive at a behavior which is optional, straightforward to implement, and can be gradually adopted as agents & clients mature without breaking either party (or adding onerous complexity) in the meantime. |
@pstephengoogle I've originally created a new field to avoid semantic overload, considering messaId currently serves conversations history. But re-thinking it I see there is more minus than plus on having a new field, so I've just pushed a change to use mesageId b) the only spec change, in theory, would be to add an 'is_idempotent_task' capability boolean (or similar) c) I would recommend doing this via an extension profile initially. The biggest concern is the burden that is placed on agent developers to implement this guarantee. Doing this as an extension, and at the very least, providing a reference implementation, will go a long way toward determining if and how to support as a core spec change |
|
@pstephengoogle I agree with @maeste on making this an optional server capability just like streaming and push notifications is optional. For servers that support it, then clients should expect the correlation identifier in responses from the server (including callback push notifications). If we make it an optional capability on the agent card then only server implementations that want to support it will have to do it, thus minimizing the initial burden of one chooses to implement the spec version. @maeste I think the push notification coming from the server after a message/send for servers that support push notifications should also include this correlation identifier in the callback operation so that it can be reconciled with the original message that triggered it. You can call this out explicitly in your docs as the payload structure for push notifications is currently not clearly defined. You may also want to review all the other interactions to see where this identifier could also be relevant in the response from the servers. If idempotency is support I think we will need the identifier beyond the message/send interaction alone. It's a great idea to use the original message identifier from the client as the correlation identifier. This was my initial thought as well. |
|
@izzymsft Thanks for the comment, but I'm not sure I'm following: the idempotency proposed here is about task creation. And by spec (see also chapter 9.5 with the example) the client receive a message confirming the task is created as usual. Then when you have taskId the idempotency is guaranteed by the taskId. The problem of idempotency solved here is on task creation. Am I missing something? |
|
@maeste I was only agreeing with you and I just added that we would include the correlation id in the push notifications as well. This whole idempotency discussion is primarily an issue in scenarios where there is a disconnection of the client from the server before the server response was received and recorded by the client If the client sends a message id then the server should include that in the push notifications as well as correlation id so that we can link it back to the original message id if it disconnected from the server before a response with the task id or context id was received and recorded by the client to be used in future interactions. This will also help us to associate this correlation id to the task and context identifiers. I hope this explains what I meant |
727386b to
ba7780c
Compare
ba7780c to
f895259
Compare
docs/specification.md
Outdated
| **Server Behavior:** | ||
|
|
||
| - **MessageId scope**: MessageId tracking is scoped to the authenticated user/session to prevent cross-user conflicts. | ||
| - **Active task collision**: If a `messageId` (for new task creation) matches an existing task in a non-terminal state (`submitted`, `working`, `input-required`), the server **MUST** return a [`MessageIdAlreadyExistsError`](#82-a2a-specific-errors) (-32008). |
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.
Can we call out what happens for the other transports (e.g. how is the error information propagated so that the client get the necessary task id)
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.
I agree with the comment. I'd like to include it in a better error handling and equivalence as proposed in #976. If that one would get merged I can rebase this one on it, including other transports error mappings for this new error in the same way I'm doing I'm that PR. Does it make sense to you?
docs/specification.md
Outdated
| **Server Behavior:** | ||
|
|
||
| - **MessageId scope**: MessageId tracking is scoped to the authenticated user/session to prevent cross-user conflicts. | ||
| - **Active task collision**: If a `messageId` (for new task creation) matches an existing task in a non-terminal state (`submitted`, `working`, `input-required`), the server **MUST** return a [`MessageIdAlreadyExistsError`](#82-a2a-specific-errors) (-32008). |
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.
I'm not so sure about erroring in this case. What if the incoming message was exactly what was previously sent? It seems like a better client experience would be to just return the ongoing task, rather than having them inspect the error and fetch it themselves. That also matches with the idempotency goal: you can send the same request twice and get exactly the same response.
Getting slightly more specific, I feel the best behavior for message/send would be:
- IF messageId matches a value previously seen, THEN:
- IF the request message exactly matches the previous message
- AND the matched message is the most recent message for the Task/Message
- THEN:
- If the request is non-blocking, OR the task is in a terminal state, OR the response was a Message: immediately return the current state of the Task/Message.
- If the request is blocking and the task is in a non-terminal state: block until the Task transitions to an interrupted state, and return the Task.
- ELSE: fail with
409 Conflict/BadRequest/MesageIdAlreadyExistsError
It looks complicated, but I think it's actually what you would naturally expect.
This allows clients to write very simple retry loops -- they can just write an outer loop that keeps sending the same request until it succeeds and not worry about handling collision errors. It also calls out more bizarre situations, like a client sending old messages that have since had follow-ups (indicating the client has seen the response, given it used details from the response in its request).
I am assuming that a client would never "accidentally" reuse a messageId (meaning: "I really wanted this to be a new task, even though the entirety of my request content is identical and this agent supports idempotency! Why did I get an old task back??"), but I actually think that's a fair assumption. We already specify that messageIds must be unique, so if you are breaking that requirement you may see strange results.
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.
I see your point, but it is a burden on the server side because the implementation would need to store both the IDs and the full messages somewhere for some time. It's not ideal for an agent with high traffic. Moreover, we are saying we store on the server side the contents of a message sent from the client, which may have some privacy implications (it's probably out of spec's scope, but just mentioning).
Finally, consider that the retry from the client should happen only in case of network outage or severe errors on the client, so it is already handling an exception for a sporadic case. We can pretend that the standard call is not in a loop, and clients handle the retry properly (ideally by checking for the task before with getTask) in a fallback method or something like that.
Am I losing something?
In the short term, I'm worried about asking the server to store the full message content instead of just an ID.
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.
Moreover, we are saying we store on the server side the contents of a message sent from the client, which may have some privacy implications (it's probably out of spec's scope, but just mentioning).
Only message hashes can be stored.
Finally, consider that the retry from the client should happen only in case of network outage or severe errors on the client, so it is already handling an exception for a sporadic case. We can pretend that the standard call is not in a loop, and clients handle the retry properly (ideally by checking for the task before with getTask) in a fallback method or something like that.
If an a2a server is exposed to a mobile client a retry-in-the-loop becomes a pretty common thing.
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.
Oki I'll add the suggested behaviour, leaving to the implementation what to save (most likely the hash)
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.
ELSE: fail with 409 Conflict
I think that'd be nice to specify something like that as the expected behavior for handling concurrent message requests with different messages. If a client sends a message and then sends a different message referencing the same task, the server should respond with an error.
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.
The commit 9384703 addresses the original comment from @mikeas1
Regarding the latest comment from @yarolegovich, I think it is a more general problem of accepting or not following up on messages depending on the task state. I've opened #1027 and addressed it in commit ec3cdc5 in this PR
docs/specification.md
Outdated
|
|
||
| A2A supports optional messageId-based idempotency to enable idempotent task creation, addressing scenarios where network failures or client crashes could result in duplicate tasks with unintended side effects. | ||
|
|
||
| **Scope**: Idempotency **ONLY** applies to new task creation (when `message.taskId` is not provided). Messages sent to continue existing tasks follow normal message uniqueness rules without task-level deduplication. |
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.
I'm not sure I understand this scope limitation, given that:
scenarios where network failures or client crashes could result in duplicate tasks with unintended side effects.
are equally likely to happen with message follow-ups and might lead to unintended consequences there as well
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.
The message.taskId guarantees follow-ups idempotency
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.
let's consider an example with (messageId, taskId) tuples:
(msg1, nil) -> task1 created, hits input_required
(msg2, task1) -> continues execution of the task
(msg2, task1) -> duplicate request is idempotent, taps into the running execution / returns the current Task state
(msg3, task1) -> a new message id, the request gets executed
Re-reading this comment, more specifically:
AND the matched message is the most recent message for the Task/Message
I'd say the idempotency mechanism scope is broader than:
the new task creation
Or maybe I'm just misunderstanding this statement:
Messages sent to continue existing tasks follow normal message uniqueness rules without task-level deduplication.
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.
I'm a bit lost. message/send and message/stream do not accept taskId because is the server that need to create them. All the other JSONRPC methods on task use the taskId. The idempotency is guaranteed for those methods by the taskId, and the messageId is not considered anymore: It may be used in logging/tracing of course, but it doesn't have any required behaviour in the current spec and this PR is just using it for task creation idempotency, leaving all the rest unchanged
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.
message/send and message/stream do not accept taskId because is the server that need to create them
I'm talking about multi-turn interactions. You're right that Task IDs are generated by the server, but a Task might require a follow-up message (input-required state). And what I'm saying is Messages sent as follow-ups for a Task need to adhere to the same idempotency rules.
Idempotency ONLY applies to new task creation (when
message.taskIdis not provided)
So I'm suggesting to change the wording 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.
@maeste To pile on to what @yarolegovich is saying with a concrete example. Imagine an agent playing the role of a shopping cart.
(msg1, nil) -> I want to go shopping - task1 created, hits input_required
(msg2, task1) -> Add a SuperX4 Laptop to my cart
(msg2, task1) -> Add a SuperX4 Laptop to my cart
(msg3, task1) -> Add a XtraScreen to my card
(msg4, task1) -> I'm done let's pay
Without idempotency during a task update, I get two laptops, not one.
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.
the commit 9384703 address this
specification/grpc/a2a.proto
Outdated
| // Extensions supported by this agent. | ||
| repeated AgentExtension extensions = 3; | ||
| // If the agent supports messageId-based idempotency for task creation | ||
| bool idempotency_supported = 4; |
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.
out of sync with the idempotencyEnforced renaming
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.
good catch, fixing
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.
the commit 697e334 address this
7dc2cb8 to
2a1f957
Compare
…or in case of collisions
2a1f957 to
9384703
Compare
correlationIdRequiredcapability toAgentCapabilitiescorrelationIdfield toMessageSendParamsCorrelationIdAlreadyExistsError(-32008) for duplicate detectionFixes #928
Discussed also in #925