Repository navigation
Breaking: return Google ADK Go MCP tool errors to the model instead of retrying them #326
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -199,7 +199,15 @@ the model SDK's own retries (see below), or override the fallbacks. | |
| toolset name — the toolset is cached and shared across calls — and the plugin | ||
| closes any cached toolset that implements `Close() error` at worker stop | ||
| automatically; with manual wiring, call `Activities.Close` yourself after | ||
| `worker.Run` returns. A tool's own error, such as an MCP `isError` result, | ||
| reaches the model once, as it does without Temporal; `CallMcpTool` succeeds | ||
| and is not retried. The plugin counts an error as the tool's own when it | ||
| wraps no other error and is not an `ApplicationError`. ADK's MCP tools wrap | ||
| the error of a failed MCP request, so that error fails `CallMcpTool` with a | ||
| retryable `googleadk.McpError`, as does an error returned after the Worker | ||
| stops the call. To have a failure in your own toolset or a `FakeMCPServer` | ||
| handler retried, wrap its cause with `%w` or return a retryable | ||
| `temporal.ApplicationError`. | ||
| - **Remote A2A agents.** Build ADK's remote agent (`remoteagent.NewA2A`) with | ||
| `AgentCardProvider: googleadk.NewA2AAgentCardProvider(cardURL, activityOptions)` | ||
| and `ClientProvider: googleadk.NewA2AClientProvider(activityOptions)`. The card | ||
|
|
@@ -249,7 +257,9 @@ the model SDK's own retries (see below), or override the fallbacks. | |
| serial fallback. | ||
| - **Typed failures.** Model / tool / MCP / A2A failures surface as Temporal | ||
| `ApplicationError`s tagged `googleadk.ModelError` / `.ToolError` / `.McpError` | ||
| / `.A2AError`. An MCP tool's own error is not one of these: ADK, its | ||
| tool-error callbacks and the model get the tool's error, as without | ||
|
Collaborator
There was a problem hiding this comment. P3 (non-blocking): callbacks get an error that carries the tool's message, not the error the tool returned, so "get the tool's error, as without Temporal" holds only for ADK's own
A callback written to the body's upgrade note ("match the tool's error, as they would without Temporal") with |
||
| Temporal, not a `googleadk.McpError` (see **MCP, statelessly** above). | ||
| Classify with `IsNonRetryable(err)`; never string-match. Upstream HTTP status | ||
| drives retryability (`408`/`409`/`429`/`5xx` retryable, other `4xx` not). | ||
| - **Disable model-SDK retries in your `ModelFactory`.** `InvokeModel` already runs | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -146,6 +146,12 @@ func (t *mcpProxyTool) Run(ctx agent.Context, args any) (map[string]any, error) | |
| } | ||
| return nil, fmt.Errorf("error tool %q %w", t.Name(), tool.ErrConfirmationRequired) | ||
| } | ||
| if out.Error != nil { | ||
|
Collaborator
There was a problem hiding this comment. P3 (tests, non-blocking): nothing in the suite exercises this branch, the Workflow-side half of the change. With lines 149-154 deleted, the full suite still passes, because the model gets the same
|
||
| // The tool's own error, such as an MCP isError result. Give ADK the | ||
| // error the tool returned, so the model sees it as it does without | ||
| // Temporal. | ||
| return nil, errors.New(*out.Error) | ||
| } | ||
| if out.Result == nil { | ||
| return map[string]any{}, nil | ||
| } | ||
|
|
@@ -168,10 +174,14 @@ type mcpCallInput struct { | |
| // mcpCallOutput is the serializable result of CallMcpTool. Confirmation is | ||
| // non-nil when the tool paused on ADK's tool.ErrConfirmationRequired protocol | ||
| // instead of running; the workflow-side proxy re-records it so the runner | ||
| // surfaces the pause exactly like an in-workflow tool. Error is the message of | ||
| // the tool's own error, which the proxy hands to ADK; see CallMcpTool. Result | ||
| // then holds {"error": message}, the response ADK builds from a tool error, so | ||
| // a workflow worker without the Error field gives the model the same response. | ||
| type mcpCallOutput struct { | ||
| Result map[string]any | ||
| Confirmation *toolconfirmation.ToolConfirmation | ||
| Error *string | ||
|
Collaborator
There was a problem hiding this comment. P1: during a rolling upgrade, a Workflow Worker of the current release reads a tool error returned by an upgraded Worker's main's Reproduced on a dev server with this setup:
(a) No callback. History records (b) An Fix: make the new output opt-in per call. The head proxy sets a field in
Contributor
Author
There was a problem hiding this comment. Thanks for reproducing this end to end. Fixed the I didn't add the opt-in field, for two reasons:
For 1.0 we're also not adding compatibility switches to these fixes; we document the deployment steps instead. The body now limits "open runs need no draining" to apps without a tool-error callback (or an after-tool callback that treats errors differently). Apps with one keep each run on one version: a new Task Queue, stopping every old Worker before starting new ones, or Worker Versioning (Pinned), and they roll back only new runs. I also added your |
||
| } | ||
|
|
||
| // ListMcpTools returns the full declarations (name + description + parameters) | ||
|
|
@@ -211,6 +221,15 @@ func (a *Activities) ListMcpTools(ctx context.Context, in mcpListInput) ([]*gena | |
| // confirmation protocol (e.g. mcptoolset's RequireConfirmation option) the | ||
| // Activity succeeds with the pending confirmation in its output rather than | ||
| // failing, so the workflow can pause the agent for the human. | ||
| // | ||
| // A tool's own error, one that wraps no other error and is not an | ||
| // ApplicationError, also goes in the output, so the model sees it once, as it | ||
| // does without Temporal. ADK's MCP tools return an isError result (and, before | ||
| // ADK v2.4.0, a result with no text) that way. They wrap the error of a failed | ||
| // MCP request, which still fails the Activity with a retryable McpError, as | ||
| // does an error returned after the Activity's context is done, such as when | ||
| // the Worker stops. An ApplicationError fails the Activity with its own type | ||
| // and retryability. | ||
|
Collaborator
There was a problem hiding this comment. P3 (comment, non-blocking): "An ApplicationError fails the Activity with its own type and retryability" holds only when the tool returns the
ff14931 behaves the same way, so only the new sentence is wrong. Suggested: "An ApplicationError returned as is fails the Activity with its own type and retryability. Wrapped in another error, it fails as a retryable |
||
| func (a *Activities) CallMcpTool(ctx context.Context, in mcpCallInput) (*mcpCallOutput, error) { | ||
| log := activity.GetLogger(ctx) | ||
| // No-op unless the user set a HeartbeatTimeout on the toolset's Activity | ||
|
|
@@ -256,6 +275,12 @@ func (a *Activities) CallMcpTool(ctx context.Context, in mcpCallInput) (*mcpCall | |
| return nil, newApplicationError(ErrorTypeMCP, false, runErr, | ||
| "tool %q failed: %v", in.Tool, runErr) | ||
| } | ||
| // An error after ctx is done, such as ctx.Err() when the Worker | ||
| // stops, is not the tool's answer, whatever it wraps. | ||
| if ctx.Err() == nil && isToolAnswer(runErr) { | ||
| msg := runErr.Error() | ||
| return &mcpCallOutput{Result: map[string]any{"error": msg}, Error: &msg}, nil | ||
| } | ||
| return nil, classifyToolError(ErrorTypeMCP, in.Tool, runErr) | ||
| } | ||
| return &mcpCallOutput{Result: res}, nil | ||
|
|
@@ -264,6 +289,23 @@ func (a *Activities) CallMcpTool(ctx context.Context, in mcpCallInput) (*mcpCall | |
| "tool %q not found in MCP toolset %q", in.Tool, in.Toolset) | ||
| } | ||
|
|
||
| // isToolAnswer reports whether err, returned by an MCP tool's Run, is the | ||
| // tool's own answer: an error that wraps no other error and is not an | ||
| // ApplicationError. | ||
| func isToolAnswer(err error) bool { | ||
| var appErr interface{ NonRetryable() bool } | ||
| if errors.As(err, &appErr) { | ||
| return false | ||
| } | ||
| switch e := err.(type) { | ||
| case interface{ Unwrap() error }: | ||
| return e.Unwrap() == nil | ||
| case interface{ Unwrap() []error }: | ||
| return len(e.Unwrap()) == 0 | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| // mcpToolset resolves the named toolset, running its registered factory at most | ||
| // once and caching the result for the life of the Activities value. The cache | ||
| // is what bounds the resource cost: a live MCP toolset lazily opens and retains | ||
|
|
||
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.
P3 (non-blocking): the deployment restriction for applications with tool-error callbacks is only in the PR body, so users will not see it. Release notes are generated from commit subjects:
release-notesreads only--format=%h%x1f%s(release_tool.py:456). The restriction is the one thing such an application must do differently. Without it, a rolling deploy can close a run asFailedor leave it stuck on a[TMPRL1100]Workflow Task failure, as in the reruns in the review body. This repository keeps such notes in the README: the remote A2A paragraph in this README, from #152, and the Strands, LangGraph and OpenAI Agents READMEs. Suggested, at the end of this bullet: