Repository navigation
Conversation
…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.
DABH
left a comment
There was a problem hiding this comment.
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.
| type mcpCallOutput struct { | ||
| Result map[string]any | ||
| Confirmation *toolconfirmation.ToolConfirmation | ||
| Error *string |
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 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
TaskQueueset to its own queue to make the placement deterministic); - a ff14931 Worker running the Workflow and
InvokeModel; - a
FakeMCPServerhandler returningerrors.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.
There was a problem hiding this comment.
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.
| return nil, newApplicationError(ErrorTypeMCP, false, runErr, | ||
| "tool %q failed: %v", in.Tool, runErr) | ||
| } | ||
| if isToolAnswer(runErr) { |
There was a problem hiding this comment.
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
McpErrorand reruns on B; the model sees{"status":"found"}(two runs). - On the head,
CallMcpToolcompletes 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().
There was a problem hiding this comment.
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.
| `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`. |
There was a problem hiding this comment.
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
ApplicationErroris the tool's answer; to have a failure retried, wrap the cause with%wor return a retryabletemporal.ApplicationError. - The same sentence on
FakeMCPServer.AddToolandMCPFactory. - 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.
There was a problem hiding this comment.
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.
|
Thanks for the review. Round 1 is in 737b6ea ( |
DABH
left a comment
There was a problem hiding this comment.
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:
isErrorand v2.3.0's no-text result are built witherrors.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:
Confirmationis checked first. go.mod:go mod tidy -diffdiffers only in thego.temporal.io/apiline, as on main.- Cross-language consistency: the Python and TypeScript plugins still hand an
isErrorresult 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 sawnull). 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 asFailedwithunable 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 retryablegoogleadk.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
MCPFactoryandFakeMCPServer.AddToolsay the same. "Typed failures" and theCallMcpToolcomment 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.
| 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`. |
There was a problem hiding this comment.
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.
| `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 |
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 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.Isis true with ADK alone and false through the plugin. - For a
*NotFoundtype,errors.Asis true with ADK alone and false through the plugin, where%Tis*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.
| } | ||
| return nil, fmt.Errorf("error tool %q %w", t.Name(), tool.ErrConfirmationRequired) | ||
| } | ||
| if out.Error != nil { |
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 {"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
mcpLookupWorkflowwith anOnToolErrorCallbacksentry that records the error it gets and returns a fallback. Assert that it ran once withTool execution failed. Details: order A-1 not foundand that the fallback reached the model. - A
FakeMCPServercase that returnserrors.Join(errA, errB)and expects twoCallMcpToolattempts.
| // 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. |
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 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 oneCallMcpToolattempt.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.
What was changed
An MCP tool's own error now reaches the model once, as it does without Temporal. Before,
CallMcpToolfailed with a retryablegoogleadk.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.CallMcpToolreturns a tool error that wraps no other error in its output, and the Workflow-side MCP proxy returns the same error to ADK. ADK'smcptoolsetreturns anisErrorresult (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. AFakeMCPServerhandler's plain error takes the same path.Result: {"error": …}, the response ADK builds from a tool error. A Workflow Worker on an earlier version reads onlyResult, 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.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 ascontext.Canceledwhen the Worker stops during the call, whatever it wraps; the call then runs again. An error that is already anApplicationErrorstill fails the Activity with its own type and retryability.FakeMCPServerhandler gets a retry: wrap the cause with%w, or return a retryabletemporal.ApplicationError.MCPFactoryandFakeMCPServer.AddToolsay 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 agoogleadk.McpError.TestMcpToolErrorReachesModel, in a newmcp_error_test.go, runs ADK'smcptoolsetover an in-memory go-sdk MCP server, sogo.modnow requiresgithub.com/modelcontextprotocol/go-sdkv1.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 changemcp_test.go's imports.TestMcpToolErrorAfterWorkerStops, in the same file, runsCallMcpToolwith 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:
OnToolErrorCallbacksand a runner plugin'sOnToolErrorCallbacknow get ADK's own error, as without Temporal, instead of anActivityErrorthat wrapsgoogleadk.McpError(the README's "Typed failures" note). A callback that matchesgoogleadk.McpErrorto supply a fallback for these errors stops matching, and the model gets the tool's error instead of the fallback.RetryPolicythat namesgoogleadk.McpErrornow applies only to failed MCP requests.MCPFactorytoolset whoseRunreturns an error that wraps no other error is no longer retried.Runs recorded before this change replay and continue, because the recorded
CallMcpToolfailure is reused. An open run whoseCallMcpToolis 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 fromResult, 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.McpErrorto match the tool's error, as they would without Temporal. A custom toolset tool that wants a retry for its error returns a retryabletemporal.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
OnToolErrorCallbacksand a runner plugin'sOnToolErrorCallbackand 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'sretryandreflectplugin's reflection response), session state it writes, ADK actions it sets (SkipSummarization, a transfer, an escalation) and Workflow commands it issues throughgoogleadk.WorkflowContext. A callback that changes the run's commands, such as one that starts a timer or one that only setsSkipSummarization, 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 staysRunning. 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 asFailed, withunable to decodeon a shifted Activity result, instead of stalling. Such applications must not run Workers of both versions on one Task Queue at once: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.gocallTool), and itsmcptoolsetturns anisErrorresult into the errorTool execution failed. Details: …, which ADK's tests pin onmain. 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 toclassifyToolError, which made it a retryablegoogleadk.McpError.Checklist
Closes [Bug] googleadk (Go): an MCP tool's error result is retried instead of reaching the model #325
How was this tested:
TestMcpToolErrorReachesModelin the newmcp_error_test.go: onmainatff14931, with only the test and itsgo.modlines added, three of its four cases fail. The model getsactivity error (type: googleadk.CallMcpTool, …): tool "lookup" failed: Tool execution failed. Details: order A-1 not found (type: googleadk.McpError, retryable: true): …instead ofTool execution failed. Details: order A-1 not found, and theFakeMCPServerhandler runs twice. TheApplicationErrorcase passes on both, as it should. All four pass with the change. The test also decodesCallMcpTool's output the way an earlier Workflow Worker does, reading onlyResult, and checks that it holds the same error; that check fails ifResultis left empty.TestMcpToolErrorAfterWorkerStops: with only thectx.Err()check removed,CallMcpToolreturns the tool's answer and the test fails; it passes with the change.make sync,make lint,make test,make buildandmake test TEST_ARGS=-racepass, andscripts/ci/check_conventions.pypasses. CI's Go 1.26.6 cells were not run.go mod tidy -diffalso wantsgo.temporal.io/apimoved from indirect to direct, as it does onmain; that line is left as onmain.mainthe default-options run is stillRunningafter 10 s withCallMcpToolat 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 isCompleted, 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.737b6ea. The later commits change only howCallMcpTooltreats an error returned after the Activity's context is done, which none of those runs returns, and docs.worker.SetStickyWorkflowCacheSize(0)) and each replay in a new process, formainand 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 returnsisError(at most 2 attempts), and the same tool with the default options, whichmainretries 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. TheisErrorruns were repeated with each of threeOnToolErrorCallbacks: one that starts a timer, one that setsSkipSummarizationand returns a fallback, and one that only returns a fallback. All 188 checks held:mainleft retrying;[TMPRL1100];mainfinished its tool call on the next attempt on this change, and the model got the tool's error. The other waiting runs completed;mainand complete, and turn 2's request carried{"error":"Tool execution failed. Details: order A-1 not found"};mainWorkflow Worker and an Activity Worker on this change, theisErrorand 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 amainActivity Worker, the model got the wrapped Activity error, as onmain. Same-version pairs matched their version. Without theResultcopy, themain-Workflow pairs gavetool said null | hi, earlier tool said null, and the rollback runs gavenullin turn 2.mainWorkflow Worker and an Activity Worker on this change, the callback did not run, and the model got the tool's error. For the timer andSkipSummarizationcallbacks, that history fails on this change and a history recorded entirely on this change fails onmain, each with[TMPRL1100]; the same two runs moved live after turn 1 (to this change's Workers, and rolled back tomain's) stayedRunningwith 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, themain-only and this-change-Workflow,main-Activity histories replay on both, and with everymainWorker stopped before this change's Workers started, amainrun that finished turn 1 and a runmainleft retrying both completed on this change with the callback's result, and replay there.git merge-treewith its branch (a55a102) merges cleanly;mcp.goauto-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 vetand 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.mdauto-merges). On this change,git merge-treewith 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.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:
CallMcpToolwithout a retry, so the model gets theActivityErrortext instead of ADK's rejection message. Not changed here.go.temporal.io/sdk/contrib/googleadkv0.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 buildingisErrorand no-text errors witherrors.Newand 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'sTool execution failed.text instead would leave aFakeMCPServerhandler's error, and a custom toolset's, retried.