Skip to content

Breaking: return Google ADK Go MCP tool errors to the model instead of retrying them - #326

Open
campoy wants to merge 4 commits into
mainfrom
fix/google-adk-go-mcp-tool-error-result-retried
Open

campoy wants to merge 4 commits into
mainfrom
fix/google-adk-go-mcp-tool-error-result-retried

Conversation

@campoy

@campoy campoy commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

What was changed

An MCP tool's own error now reaches the model once, as it does without Temporal. Before, CallMcpTool failed with a retryable googleadk.McpError: under the default options the tool ran again on every attempt and the run never moved on, and with a retry limit the model got a wrapped Activity error.

  • CallMcpTool returns a tool error that wraps no other error in its output, and the Workflow-side MCP proxy returns the same error to ADK. ADK's mcptoolset returns an isError result (and, on ADK v2.3.0, a result with no text) as such an error, so the model now gets {"error":"Tool execution failed. Details: …"}, as without Temporal. A FakeMCPServer handler's plain error takes the same path.
  • The output also carries the error as Result: {"error": …}, the response ADK builds from a tool error. A Workflow Worker on an earlier version reads only Result, so during a rolling upgrade (an earlier Workflow Worker with an Activity Worker on this version) or after a rollback, the model still gets the same {"error": …} response. On such a Worker the earlier proxy hands ADK that response as a result, so tool-error callbacks do not run for it; after-tool callbacks see it.
  • A failed MCP request still fails the Activity with a retryable googleadk.McpError, because ADK wraps the client's error (failed to call MCP tool … with err: %w). So does an error the tool returns after the Activity's context is done, such as context.Canceled when the Worker stops during the call, whatever it wraps; the call then runs again. An error that is already an ApplicationError still fails the Activity with its own type and retryability.
  • The README's MCP bullet says which errors reach the model, which are retried, and how a custom toolset or a FakeMCPServer handler gets a retry: wrap the cause with %w, or return a retryable temporal.ApplicationError. MCPFactory and FakeMCPServer.AddTool say the same, and the README's "Typed failures" note says an MCP tool's own error reaches ADK as the tool's error, not as a googleadk.McpError.
  • TestMcpToolErrorReachesModel, in a new mcp_error_test.go, runs ADK's mcptoolset over an in-memory go-sdk MCP server, so go.mod now requires github.com/modelcontextprotocol/go-sdk v1.7.0 (the version ADK v2.3.0 already selects) and the modules it needs. It is a new file because Breaking: list each Google ADK Go MCP toolset once per model step #306 and Breaking: resolve the Google ADK Worker model by the TemporalModel's name in Go and Python #319 both change mcp_test.go's imports. TestMcpToolErrorAfterWorkerStops, in the same file, runs CallMcpTool with a tool that returns its context's error after the Worker stops.

Breaking change: no public name or option changes, but these tool errors no longer fail the Activity:

  • For a tool error result, ADK's OnToolErrorCallbacks and a runner plugin's OnToolErrorCallback now get ADK's own error, as without Temporal, instead of an ActivityError that wraps googleadk.McpError (the README's "Typed failures" note). A callback that matches googleadk.McpError to supply a fallback for these errors stops matching, and the model gets the tool's error instead of the fallback.
  • A RetryPolicy that names googleadk.McpError now applies only to failed MCP requests.
  • A tool in a custom MCPFactory toolset whose Run returns an error that wraps no other error is no longer retried.

Runs recorded before this change replay and continue, because the recorded CallMcpTool failure is reused. An open run whose CallMcpTool is retrying a tool error finishes that call on its next attempt after the upgrade, and the model gets the tool's error. After a rollback, runs that recorded a tool error on this version replay without an error, and the earlier version reads the error from Result, so later model calls in those runs get the same tool response, unless the application has a tool-error callback (below).

Upgrade: change error callbacks that match googleadk.McpError to match the tool's error, as they would without Temporal. A custom toolset tool that wants a retry for its error returns a retryable temporal.ApplicationError, or wraps the cause. Without a tool-error callback, open runs need no draining, and Workflow and Activity Workers of the two versions can run on one Task Queue during the rollout.

Deployment restriction for applications with tool-error callbacks: for this version's tool-error output, the earlier Workflow-side proxy hands ADK a result, so ADK skips OnToolErrorCallbacks and a runner plugin's OnToolErrorCallback and gives after-tool callbacks no error; this version runs them. For a run whose history holds that output, the two Workflow builds disagree on whatever the callback does: its response (a fallback, or ADK's retryandreflect plugin's reflection response), session state it writes, ADK actions it sets (SkipSummarization, a transfer, an escalation) and Workflow commands it issues through googleadk.WorkflowContext. A callback that changes the run's commands, such as one that starts a timer or one that only sets SkipSummarization, makes the run replay only on the Workflow build that read the output: on the other it fails with [TMPRL1100], and a moved live run gets a non-deterministic Workflow Task failure and stays Running. A callback that only returns a fallback replays without an error, but the moved run's later turns rebuild a different tool response from the one the model got. This applies to every tool-error callback, agent or plugin, and to an after-tool callback that treats errors differently from results, unless it only logs. Review also saw such a run close as Failed, with unable to decode on a shifted Activity result, instead of stalling. Such applications must not run Workers of both versions on one Task Queue at once:

  • deploy this version on a new Task Queue and keep the earlier Workers until their runs close, or stop every earlier Worker on the Task Queue before starting Workers of this version. Worker Versioning with the Pinned behavior, with the MCP Activities on the Workflow's Task Queue, is meant to do the same; it was not tested here;
  • roll back only new runs: runs that used this version stay on its Workers until they close, or are reset or terminated.

Runs recorded before this version, including runs left retrying a tool error, can move to Workflow and Activity Workers of this version together: a recorded failure gives the callback the same ActivityError, and a pending call finishes on this version and the callback runs there.

Why?

See #325. ADK hands a tool's error to the model once (base_flow.go callTool), and its mcptoolset turns an isError result into the error Tool execution failed. Details: …, which ADK's tests pin on main. The MCP Go SDK says tool errors belong in the result so the model can see them and self-correct, and its server returns one for every handler error and invalid argument. The plugin passed every such error to classifyToolError, which made it a retryable googleadk.McpError.

Checklist

  1. Closes [Bug] googleadk (Go): an MCP tool's error result is retried instead of reaching the model #325

  2. How was this tested:

    • TestMcpToolErrorReachesModel in the new mcp_error_test.go: on main at ff14931, with only the test and its go.mod lines added, three of its four cases fail. The model gets activity error (type: googleadk.CallMcpTool, …): tool "lookup" failed: Tool execution failed. Details: order A-1 not found (type: googleadk.McpError, retryable: true): … instead of Tool execution failed. Details: order A-1 not found, and the FakeMCPServer handler runs twice. The ApplicationError case passes on both, as it should. All four pass with the change. The test also decodes CallMcpTool's output the way an earlier Workflow Worker does, reading only Result, and checks that it holds the same error; that check fails if Result is left empty.
    • TestMcpToolErrorAfterWorkerStops: with only the ctx.Err() check removed, CallMcpTool returns the tool's answer and the test fails; it passes with the change.
    • Locally on this change, in Docker with Go 1.27.2: make sync, make lint, make test, make build and make test TEST_ARGS=-race pass, and scripts/ci/check_conventions.py passes. CI's Go 1.26.6 cells were not run. go mod tidy -diff also wants go.temporal.io/api moved from indirect to direct, as it does on main; that line is left as on main.
    • [Bug] googleadk (Go): an MCP tool's error result is retried instead of reaching the model #325's reproduction on a dev server: on v0.1.0 and main the default-options run is still Running after 10 s with CallMcpTool at attempt 5, and the 3-attempt run calls the tool 3 times. With the change, both runs call the tool once, the default-options run is Completed, and the model gets {"error":"Tool execution failed. Details: order A-1 not found"}, as with ADK alone. In the issue's second program, results with only an image or only a resource, and a typed tool called with a wrong argument type, now run once and give the model the same error as ADK alone; a result with text and an image is unchanged.
    • The reproduction above and the replay check below ran on the tree of 737b6ea. The later commits change only how CallMcpTool treats an error returned after the Activity's context is done, which none of those runs returns, and docs.
    • Replay check on a dev server, with Workers with no Workflow cache (worker.SetStickyWorkflowCacheSize(0)) and each replay in a new process, for main and this change. Turn 1 calls an MCP tool, the run waits for a signal, and turn 2 asks a model that repeats the tool response the session holds. Three cases: a tool that answers with text, a tool that returns isError (at most 2 attempts), and the same tool with the default options, which main retries without end. Each was recorded finished and left waiting. A Workflow Worker of one version and an Activity Worker of the other also served runs on split Task Queues. The isError runs were repeated with each of three OnToolErrorCallbacks: one that starts a timer, one that sets SkipSummarization and returns a fallback, and one that only returns a fallback. All 188 checks held:
      • every history without the callback replays on both versions, including a run main left retrying;
      • a control that adds an Activity at Workflow start fails every history with [TMPRL1100];
      • waiting runs moved to a new Worker of the same version completed;
      • upgrade: a run left retrying on main finished its tool call on the next attempt on this change, and the model got the tool's error. The other waiting runs completed;
      • rollback, without the callback: runs that recorded a tool error on this change replay on main and complete, and turn 2's request carried {"error":"Tool execution failed. Details: order A-1 not found"};
      • mixed Workers: with a main Workflow Worker and an Activity Worker on this change, the isError and default-options runs called the tool once, and both turns got {"error":"Tool execution failed. Details: order A-1 not found"}. With this change's Workflow Worker and a main Activity Worker, the model got the wrapped Activity error, as on main. Same-version pairs matched their version. Without the Result copy, the main-Workflow pairs gave tool said null | hi, earlier tool said null, and the rollback runs gave null in turn 2.
      • callbacks: with a main Workflow Worker and an Activity Worker on this change, the callback did not run, and the model got the tool's error. For the timer and SkipSummarization callbacks, that history fails on this change and a history recorded entirely on this change fails on main, each with [TMPRL1100]; the same two runs moved live after turn 1 (to this change's Workers, and rolled back to main's) stayed Running with a [TMPRL1100] Workflow Task failure. For the fallback-only callback, both replay without an error, but the moved runs completed with turn 2 rebuilding a different tool response from turn 1's (the tool's error in place of the fallback after the rollback, and the reverse after the upgrade). For every callback, the main-only and this-change-Workflow, main-Activity histories replay on both, and with every main Worker stopped before this change's Workers started, a main run that finished turn 1 and a run main left retrying both completed on this change with the callback's result, and replay there.
    • Merging with Breaking: list each Google ADK Go MCP toolset once per model step #306: git merge-tree with its branch (a55a102) merges cleanly; mcp.go auto-merges, since Breaking: list each Google ADK Go MCP toolset once per model step #306 changes the listing code and this change the call path. On that merge the new test, go vet and the full suite pass. With Breaking: resolve the Google ADK Worker model by the TemporalModel's name in Go and Python #319 (12351bd) it also merges cleanly (README.md auto-merges). On this change, git merge-tree with Breaking: list each Google ADK Go MCP toolset once per model step #306 (eb36121), Breaking: fail the Google ADK model Activity when its last stream publish fails (Go and TypeScript) #308 (d5a3e66) and Breaking: resolve the Google ADK Worker model by the TemporalModel's name in Go and Python #319 (54a4d26) is clean.
  3. Any docs updates needed?
    The README's MCP bullet and "Typed failures" note are updated here. Release notes should list the callback and retry changes under "Breaking change", with the deployment restriction for applications with tool-error callbacks.

Not covered:

  • A confirmation the human rejects still fails CallMcpTool without a retry, so the model gets the ActivityError text instead of ADK's rejection message. Not changed here.
  • go.temporal.io/sdk/contrib/googleadk v0.3.0, the plugin's earlier home, has the same defect. No backport here.

The line between a tool's answer and a failed request is "the error wraps no other error and is not an ApplicationError, and the Activity's context is not done". It relies on ADK building isError and no-text errors with errors.New and wrapping client errors with %w, which review checked on ADK v2.3.0, v2.5.0 and v2.6.0; the new test fails if ADK changes either. Matching ADK's Tool execution failed. text instead would leave a FakeMCPServer handler's error, and a custom toolset's, retried.

…f retrying them

CallMcpTool failed with a retryable googleadk.McpError when an MCP tool
returned isError (or, before ADK v2.4.0, no text), or when a FakeMCPServer
handler returned an error. Under the default options the tool ran again
on every attempt and the run never moved on; with a retry limit the model
got a wrapped Activity error.

Now an error that wraps no other error is the tool's own answer:
CallMcpTool returns it in its output and the Workflow-side proxy gives
ADK the same error, so the model sees it once, as without Temporal. A
failed MCP request, which ADK wraps, and an ApplicationError still fail
the Activity.
@campoy
campoy marked this pull request as ready for review October 31, 2026 01:35
@campoy
campoy requested a review from a team as a code owner October 31, 2026 01:35

@DABH DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review, at head b2b28a9 (one commit over ff14931; main has not touched go/googleadk since, and the branch merges cleanly with it). In go/googleadk with Go 1.27.1, make sync lint test build pass. The new TestMcpToolErrorReachesModel, run on ff14931 with only its file, go.mod and go.sum added, fails 3 of its 4 subtests as the body says (the wrapped ActivityError text, two tool runs) and the ApplicationError case passes. After make sync-latest (ADK v2.6.0, go-sdk v1.8.0, SDK v1.50.0) the MCP tests still pass, so nightly stays green. Checked and dropped: the rule misreading ADK's own errors (at v2.3.0, v2.5.0 and v2.6.0 isError is built with errors.New and CallTool failures are wrapped with %w; v2.6.0's new reserved-_meta-key error is unwrapped and deterministic, so handing it to the model matches ADK alone), and the rebuilt error losing anything ADK's flow uses (callTool reads only err.Error()). Replay was checked with real Workers on a dev server and worker.NewWorkflowReplayer, each version in its own process. Histories recorded on ff14931 all replay on the head: a tool error with two attempts, the same with an OnToolErrorCallbacks callback that runs a fallback Activity when it matches googleadk.McpError and with one that does so for any error, a success, and a run left retrying under the default options. Head histories replayed on ff14931 reproduce the body's rollback note exactly: the any-error callback history fails with [TMPRL1100] lookup failed for scheduledEventID to activityID: scheduleEventID: 35, activityID: 35, and the others replay. Across languages, the Python and TypeScript Google ADK plugins already give an isError result to the model (their ADKs return it instead of raising, and the TypeScript <name>-callTool Activity returns the CallToolResult), so this change brings Go in line. The P1 and P2 below were reproduced end to end with real Workers; nothing material was left unverified.

Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.

Where the problems cluster: the rule is right for ADK's own mcptoolset, and histories from main replay. Both defects sit where the new output meets code that does not expect it: a Workflow Worker of the current release reads it during a rollout, and a cancelled Activity produces it. On the body's open question, I would keep the 'wraps no other error' rule, since it holds from ADK v2.3.0 to v2.6.0 and the new test pins it. I would add the cancellation guard, and document the rule so that custom toolsets know how to ask for a retry.

Requesting changes because of the rolling-upgrade item: an ordinary rollout drops MCP tool errors on Workflow Workers of the current release and can fail open runs whose error handling issues commands, while the body tells operators that no draining is needed.

PR body: it still ends with 'Draft while the breaking change is open', but the PR is not marked as a draft. Also, the release-notes guidance ('with the rollback note above') needs the rolling-upgrade correction from the first item.

Comment thread go/googleadk/mcp.go
type mcpCallOutput struct {
Result map[string]any
Confirmation *toolconfirmation.ToolConfirmation
Error *string

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: during a rolling upgrade, a Workflow Worker of the current release reads a tool error returned by an upgraded Worker's CallMcpTool as an empty result. A run whose error handling issues commands then fails when an upgraded Worker replays it, although the body says 'Open runs need no draining for the upgrade'.

main's mcpCallOutput has no Error field, so the JSON converter drops it and the old proxy returns map[string]any{} (mcp.go:149 at ff14931). With sticky execution, a run's Workflow Tasks stay on the Worker that last ran them, while CallMcpTool goes to any Worker on the queue. During a rollout, old Workflows therefore routinely consume new Activities' results.

Reproduced on a dev server with this setup:

  • a head Worker serving only the MCP Activities (the toolset's TaskQueue set to its own queue to make the placement deterministic);
  • a ff14931 Worker running the Workflow and InvokeModel;
  • a FakeMCPServer handler returning errors.New("order A-1 not found"), MaximumAttempts: 2, and a signal wait after turn 1.

(a) No callback. History records CallMcpTool completed with {"Result":null,"Confirmation":null,"Error":"order A-1 not found"}, and in turn 1 the model got that tool response as null. After the old Worker stopped and a head Worker took the signal, in turn 2 the model got {"error":"order A-1 not found"} for that same call.

(b) An OnToolErrorCallbacks callback that runs a fallback Activity on a tool error. This is the body's own example, and also what its upgrade advice produces once a callback matches the tool's error. The old Worker scheduled no fallback. The head Worker's replay scheduled it, and the shifted Activity results failed the run with process request by toolset "orders-err": payload item 0: unable to decode: json: cannot unmarshal object into Go value of type []*genai.FunctionDeclaration. The Execution closed as Failed: a live Worker skips the history check once the workflow function returns (SDK v1.48.0 internal/internal_task_handlers.go:1286). worker.NewWorkflowReplayer on the saved history reports [TMPRL1100] nondeterministic workflow: history event is ActivityTaskScheduled for googleadk.ListMcpTools (ActivityId 29) against a replay command that schedules the fallback Activity. Histories recorded on a single version replay cleanly, so this is the body's rollback hazard, occurring on the way up.

Fix: make the new output opt-in per call. The head proxy sets a field in mcpCallInput (say ToolErrorInOutput: true), and CallMcpTool returns Error only when that field is set; otherwise it fails with the retryable McpError. A Workflow of the current release never sets the field, so every reader in a mixed deployment agrees. Replay compares only Activity ID and type (internal_task_handlers.go:1653-1663), so the new input field is replay-safe. The cost is that a call scheduled by the old code keeps retrying after the upgrade, since its input is fixed, so runs that are already stuck need a reset; the body should say so. A cheaper partial step is to also return Result: map[string]any{"error": msg}, so an old reader gives the model the JSON ADK builds for an error; callbacks still diverge, though. Either way, replace 'Open runs need no draining for the upgrade' with the rollback section's advice for deployments whose callbacks or Workflow code act on tool errors: use Worker Versioning, or let runs that may record a tool error close first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reproducing this end to end. Fixed the null part in 737b6ea: CallMcpTool now also sets Result: {"error": msg}, so a Workflow Worker of the current release gives the model the same {"error": …} response as this one. TestMcpToolErrorReachesModel decodes the output the way the old proxy does and checks it.

I didn't add the opt-in field, for two reasons:

  • A call scheduled by the current release keeps its input, so runs stuck retrying today would stay stuck after the upgrade and need a reset. Letting those runs finish is the main thing [Bug] googleadk (Go): an MCP tool's error result is retried instead of reaching the model #325 fixes.
  • It still wouldn't make a plain rolling upgrade safe for apps with tool-error callbacks. During the rollout, a run can move from a new Workflow Worker back to an old one (a sticky timeout, a restart) after a new Activity recorded the error, and the old Worker then skips the callback. Those apps would need the restriction anyway.

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 Failed outcome (the unable to decode error) next to the stalled one.

Comment thread go/googleadk/mcp.go Outdated
return nil, newApplicationError(ErrorTypeMCP, false, runErr,
"tool %q failed: %v", in.Tool, runErr)
}
if isToolAnswer(runErr) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: a tool that returns its context's bare error counts as the tool's own answer. If a Worker stops in the middle of a custom-toolset call, the model gets {"error":"context canceled"} and the call is not retried on another Worker.

context.Canceled is a plain errors.New value and context.DeadlineExceeded has no Unwrap, so isToolAnswer returns true for both. The SDK reports any nil-error return as completed, whatever cancelled the Activity (SDK v1.48.0 internal/internal_task_pollers.go:1603). With the default WorkerStopTimeout of 0, Worker.Stop cancels running Activities with ErrWorkerShutdown right away (internal/internal_worker_base.go:930).

Reproduced with real Workers. The MCPFactory toolset's tool waits on <-ctx.Done() and returns ctx.Err() on its first run, and returns {"status":"found"} after that. Worker A was stopped mid-call and Worker B started.

  • On ff14931 the attempt fails with a retryable McpError and reruns on B; the model sees {"status":"found"} (two runs).
  • On the head, CallMcpTool completes with "Error":"context canceled" and the model sees {"error":"context canceled"} (one run).

By the same code path, an Activity pause, which reaches the Activity when a HeartbeatTimeout is set, completes the call instead of pausing it. ADK's mcptoolset is not affected, because it wraps every CallTool error.

Fix: if ctx.Err() == nil && isToolAnswer(runErr). An error returned after the Activity's context is done is not the tool's answer, whatever it wraps. Add a subtest with a tool that returns ctx.Err().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixed in 662da82: an error the tool returns after the Activity's context is done now fails with a retryable McpError, whatever it wraps (ctx.Err() == nil && isToolAnswer(runErr)). The workflow test environment has no Worker stop, so the new TestMcpToolErrorAfterWorkerStops runs CallMcpTool in the Activity test environment with a BackgroundActivityContext that the tool cancels, as a2a_internal_test.go does. It fails without the check (CallMcpTool returns the tool's answer) and passes with it.

Comment thread go/googleadk/README.md Outdated
`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. A failed MCP request fails `CallMcpTool` with a
retryable `googleadk.McpError`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: the README states the outcome but not the rule, and the 'Typed failures' note still promises googleadk.McpError for MCP failures.

For an error from a custom MCPFactory toolset or a FakeMCPServer handler, retry versus model depends on whether it wraps another error. Only the body and a code comment say so. A test that simulated a transient MCP failure with errors.New in a FakeMCPServer handler now sees one run, with the message reaching the model.

Suggested:

  • One sentence here: an error that wraps no other error and is not an ApplicationError is the tool's answer; to have a failure retried, wrap the cause with %w or return a retryable temporal.ApplicationError.
  • The same sentence on FakeMCPServer.AddTool and MCPFactory.
  • At README.md:253: say that an MCP tool's own error reaches ADK and its error callbacks as the tool's error, not as a googleadk.McpError. This is the first change the body lists under Breaking change.

Also, the closing sentence of the CallMcpTool comment, 'So does an ApplicationError' (mcp.go:227), reads as if an ApplicationError became a retryable McpError. In fact it passes through with its own type and retryability.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 915f6b9. The README's MCP item now states the rule and how to get a retry (wrap the cause with %w, or return a retryable temporal.ApplicationError). The same sentence is on MCPFactory and FakeMCPServer.AddTool, and "Typed failures" now says an MCP tool's own error reaches ADK, its tool-error callbacks and the model as the tool's error, not as a googleadk.McpError. The CallMcpTool comment now says an ApplicationError fails the Activity with its own type and retryability; that change is in 662da82, which rewrote the paragraph.

A workflow worker from before this change reads only Result and
Confirmation from CallMcpTool's output. With the tool's error only in
Error, it handed ADK an empty result, so during a rolling upgrade or
after a rollback the model got the tool response as null. Result now
holds {"error": message}, the response ADK builds from a tool error, so
that worker gives the model the same response.
When the Worker stops during a call, a tool can return its context's
error, such as context.Canceled. That error wraps nothing, so it counted
as the tool's answer: CallMcpTool completed and the model got
{"error":"context canceled"}. An error returned after the Activity's
context is done is not the tool's answer, so it now fails the Activity
with a retryable McpError and the call runs again, as before.
The README's MCP item now states the rule: an error that wraps no other
error and is not an ApplicationError is the tool's own answer, and a
failure is retried when its cause is wrapped with %w or it is a
retryable temporal.ApplicationError. The same rule is on MCPFactory and
FakeMCPServer.AddTool, and the "Typed failures" item says an MCP tool's
own error reaches ADK as the tool's error, not as a googleadk.McpError.
@campoy

campoy commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Round 1 is in 737b6ea (Result copy), 662da82 (context check) and 915f6b9 (docs), with replies in each thread. On the summary: the body no longer tells apps with tool-error callbacks that open runs need no draining, the release-notes line points to the deployment restriction, and the "Draft while…" line is gone. On the open question, I kept the "wraps no other error" rule as you suggested; an error returned after the Activity's context is done no longer counts as the tool's answer.

@DABH DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at head 915f6b9, three commits over b2b28a9 (my first round). The branch is still based on ff14931 and merges cleanly with main at 88745dc, #319 included. In go/googleadk with Go 1.27.1, make sync lint test build passes on the head, and make sync lint test passes on a local merge with main. On ff14931, with only the new test file, go.mod and go.sum added, TestMcpToolErrorReachesModel still fails 3 of its 4 subtests. On the head, removing the Result copy fails the three answered cases, and removing the ctx.Err() guard fails TestMcpToolErrorAfterWorkerStops. After make sync-latest (ADK v2.6.0, go-sdk v1.8.0, SDK v1.50.0) the MCP tests pass. Two A2A tests fail with a2a-go v2.7.0, and they fail the same way on main, so they are not from this PR. I reran the first round's reproductions with real Workers on a dev server, one process per build, and replayed every history with worker.NewWorkflowReplayer on both builds. ff14931 has the same mcp.go as v0.1.0 and current main, so these results also hold for an upgrade from the release. A second, independent pass over the code and docs found the last three items below, and I reproduced each one before keeping it. Checked and dropped:

  • Misclassification of ADK's own errors from v2.3.0 to v2.6.0: isError and v2.3.0's no-text result are built with errors.New, and every client error is wrapped with %w. v2.4.0 already renders a no-text result as text, so the comment's "before ADK v2.4.0" is right.
  • Old readers of the new output: they ignore Error.
  • The HITL branch order: Confirmation is checked first.
  • go.mod: go mod tidy -diff differs only in the go.temporal.io/api line, as on main.
  • Cross-language consistency: the Python and TypeScript plugins still hand an isError result to the model.

Nothing material was left unverified.

Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.

Previous rounds.

  • Rolling upgrade (P1): fixed for applications without a tool-error callback. With a ff14931 Workflow Worker and a head Activity Worker, the model now gets {"error":"order A-1 not found"} in turn 1 (round one saw null). After the run moves to a head Workflow Worker, turn 2 gets the same response. Both histories replay on both builds. I accept the decline of the opt-in input field. The runs left retrying today are the ones #325 is about, and the field would keep them stuck. A run that moves back to an earlier Workflow Worker would diverge anyway. The body now restricts deployments for applications with tool-error callbacks. My rerun of the fallback-Activity callback matches what it describes: the moved run closes as Failed with unable to decode, and its turn-1 history fails on the head with [TMPRL1100].
  • Worker stop during a call (P2): fixed. The same custom tool returns ctx.Err() when Worker A stops. Attempt 1 now fails with a retryable googleadk.McpError, and the call runs again on Worker B. The model gets {"status":"found"} after two runs, as on ff14931.
  • Docs (P3): fixed. The README states the rule and how to get a retry, and MCPFactory and FakeMCPServer.AddTool say the same. "Typed failures" and the CallMcpTool comment are corrected.
  • PR body: the draft line is gone, and the release-notes guidance now includes the restriction.

The body's upgrade and rollback claims also hold. ff14931 left a run retrying under the default options (attempt 3). Its call finished on the head's next attempt, with and without a callback, and both runs replay on the head. Two head runs recorded a tool error and then moved to ff14931 Workers. The run without a callback completed, and turn 2 got the same tool response. The run with the fallback callback closed as Failed, as the body says.

Where the remaining items cluster: the code is right. What is left is how the new contract is written down and pinned. The deployment restriction exists only in the PR body. The README promises callbacks the tool's error, but they get only its message. Nothing in the suite would catch removal of the Workflow-side branch that makes callbacks run.

Approving: every item from my first round is fixed or declined for a reason I accept, CI is green, and the four items below are non-blocking documentation and test points.

Comment thread go/googleadk/README.md
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`.

Copy link
Copy Markdown
Collaborator

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-notes reads 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 as Failed or 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:

If an agent or a runner plugin has a tool-error callback, or an after-tool callback that treats errors differently from results, do not run Workers of an earlier version and this one on the same Task Queue. An earlier Workflow Worker hands this version's tool error to ADK as a result and skips the callback, so the run fails to replay on the other version, or closes as Failed. Deploy this version on a new Task Queue, or stop every earlier Worker before you start this version's. If you roll back, keep runs that ran on this version on its Workers until they close.

Comment thread go/googleadk/README.md
`ApplicationError`s tagged `googleadk.ModelError` / `.ToolError` / `.McpError`
/ `.A2AError`.
/ `.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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mcptoolset. CallMcpTool keeps only runErr.Error() (mcp.go:281), and the proxy rebuilds it with errors.New (mcp.go:153). For ADK's isError errors nothing is lost, because ADK also builds them with errors.New. A custom toolset or a FakeMCPServer handler that returns a sentinel or a typed error does lose something. Probe, with the same OnToolErrorCallback with and without Temporal:

  • For a sentinel, errors.Is is true with ADK alone and false through the plugin.
  • For a *NotFound type, errors.As is true with ADK alone and false through the plugin, where %T is *errors.errorString.
  • The model's response is the same.

A callback written to the body's upgrade note ("match the tool's error, as they would without Temporal") with errors.Is or errors.As never matches, and nothing reports it. The model then gets the error instead of the fallback. ADK's retryandreflect plugin also reports a different error_type, because it takes it from %T. Suggested wording: "get an error with the tool's message, as without Temporal. Its Go type and identity do not cross the Activity boundary, so match on the message." Make the same change in the proxy comment at mcp.go:150-152 and in the body's upgrade note. Main could not match these either, because there callbacks got an ActivityError, so this is a wording fix, not a regression.

Comment thread go/googleadk/mcp.go
}
return nil, fmt.Errorf("error tool %q %w", t.Name(), tool.ErrConfirmationRequired)
}
if out.Error != nil {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {"error": …} through the Result copy. Without the branch, tool-error callbacks never run and after-tool callbacks get no error, which undoes the PR's main breaking change. My dev-server runs show the branch working at this head: a callback that runs a fallback Activity ran once, and one that matches googleadk.McpError no longer matched. No test would catch its removal, though. Likewise, with len(e.Unwrap()) == 0 at line 304 replaced by true, the suite still passes, so the errors.Join and multi-%w case is untested. Suggested:

  • A variant of mcpLookupWorkflow with an OnToolErrorCallbacks entry that records the error it gets and returns a fallback. Assert that it ran once with Tool execution failed. Details: order A-1 not found and that the fallback reached the model.
  • A FakeMCPServer case that returns errors.Join(errA, errB) and expects two CallMcpTool attempts.

Comment thread go/googleadk/mcp.go
// 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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (comment, non-blocking): "An ApplicationError fails the Activity with its own type and retryability" holds only when the tool returns the ApplicationError as is. classifyToolError passes a wrapped one through unchanged (tool.go:79-85). The SDK's failure converter then encodes the outer *fmt.wrapError as a retryable wrapError failure. Probe, with a FakeMCPServer handler and MaximumAttempts: 2:

  • temporal.NewNonRetryableApplicationError("order A-1 not found", "NotFound", nil) gives one CallMcpTool attempt.
  • fmt.Errorf("lookup: %w", <that error>) gives two attempts and two tool runs. Under the default options, it would be retried without limit.

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 wrapError." Alternatively, when err is not itself an *ApplicationError, classifyToolError could build newApplicationError(errType, !appErr.NonRetryable(), err, …). That would be a separate behaviour change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] googleadk (Go): an MCP tool's error result is retried instead of reaching the model

2 participants