Refactor Nexus dispatch result classification by chrsmith · Pull Request #11852 · temporalio/temporal

chrsmith · 2026-08-28T19:56:52Z

I had the AI rewrite the changes to make them easier to review, consider looking at the changes commit-by-commit to see how things all come together.

What changed?

Refactors the frontend's Nexus code relating to how it classifies DispatchNexusTaskResponse values into a reusable DispatchOutcome/DispatchResult types.

Exposing the exact behavior we have today would carry forward some bugs and deprecated types. So there are several intentional behavior changes with this PR (covered below.)

Context

When making a Nexus request, the last step of the invocation is done via the Matching Service's DispatchNexusTask operation. The returned DispatchNexusTaskResponse proto has an Outcome field with 4x variants (Failure, HandlerError, RequestTimeout, and Response). And those variants then expand into 9x(!) distinct outcomes that each need to be handled differently.

For example, the Nexus task can complete successful, with either a sync or async result.
Or if the handler fails, there are different formats the client can use to send back error responses: handler error or operation error? And both nexuspb.HandlerError and nexuspb.UnsuccessfulOperationError are both deprecated, and have been transitioned to use failurepb.Failure for newer clients!

It is sufficient to say that this isn't the type of code we want to be duplicated anywhere, given how rickety it is today.

This refactoring will allow the worker callbacks feature to reuse the same DispatchNexusTaskResponse classification logic, so that there won't be any discrepancies in how we extract errors arising from Nexus handlers getting invoked. e.g. now there is a single, canonical way that "outcome metric tags" will be labeled.

Note that the metric tags we emit today are a little off. A successful cancelation of a Nexus operation is labeled as "success", whereas a successful synchronous operation is labeled with "sync_success". And an uninterpretable response reports handler_error:EMPTY_OUTCOME despite they not coming from a Nexus handler.

All the existing labels were kept as-is except for addressing existing bugs.

Functional Changes

There are a few behavior changes in this PR:

Metric Tags

The metric tag used for a worker failure without a handler failure info changed from "handler_error:" (empty suffix) to "handler_error:UNKNOWN" instead.
We now map all non-spec handler error types to handler_error:UNKNOWN, instead of previously introducing their own handler_error:${userText}. Without the cap, the cardinality of the outcome metric could be unbounded.

Error Conversion

commonnexus.DispatchResultToError now properly handles the "deprecated" errors from clients sending older response Nexus protos, instead of treating them as nexus.NewHandlerErrorf(nexus.HandlerErrorTypeInternal, "empty or unknown dispatch outcome") today.

The deprecated matchingservice.DispatchNexusTaskResponse_HandlerError and nexuspb.StartOperationResponse_OperationErrortypes are now converted intofailurepb.Failure` protos.

This does change what the Temporal server expects to receive from workers sending the deprecated errors.

A client emitting a failed operation is now seen as ApplicationFailure{Type: "OperationError", NonRetryable: true} wrapping the worker's failure. Whereas today, we'd just see the worker's failure alone. (This however aligns with what the current generation of Workers send now.)

Everything Else

Everything else should be just shuffling the code around and simplifying existing function signatures.

In service/frontend/nexus_handler.go, the DispatchNexusTaskResponse objects were handled by two methods:

func (h *nexusHandler) StartOperation(...) (...)
func (h *nexusHandler) CancelOperation(...) (...)

The logic has been moved into methods in service/frontend/nexus_dispatch_result.go, like func (c *operationContext) startOperationOutcome(...).

Why?

By exposing this classification logic, other parts of the codebase that deal with dispatching Nexus tasks (e.g. worker callbacks) will produce the same results. And emit the same outcome metrics.

Also, having a clearer set of possible outcomes from a DispatchNexusTaskResponse makes it easier to understand at a glance if the code processing the Nexus task's result is handling all the relevant cases. (e.g. multiple sources of failures, deprecated failure types, etc.)

How did you test it?

Potential Risks

If there is some change to how Nexus results are classified or how errors are handled, it could break Nexus. That would be very bad. And AI code reviewers should confirm that there is no behavior changes outside of how metric tags are derived.

github-actions · 2026-08-28T22:14:04Z

Claude finished @chrsmith's task in 7m 4s —— View job

Reviewed 7 files, 4 findings.
• chrsmith/refactor-nexus-dispatch-outcome

Moves the two switches over matching's DispatchNexusTaskResponse out of nexusHandler.StartOperation and nexusHandler.CancelOperation and into operationContext methods in a new file, service/frontend/nexus_dispatch_result.go: - (*operationContext).startOperationOutcome - (*operationContext).cancelOperationOutcome No behavior change. The arms are moved verbatim, including the metric outcome tags, the failure-source response header, and the deprecated response variants. Three things are factored out of the repeated arms: - convertWorkerFailure: the Temporal-failure-to-Nexus-failure conversion that appeared three times, with the same log lines. The two "converting Nexus failure to Nexus HandlerError/OperationError" messages collapse into one. - operationError: building and marking the OperationError envelope. - convertOutcomeToNexusHandlerError moves along with its only callers. Handler links are now returned to the caller rather than attached in place, because nexus.AddHandlerLinks needs the SDK's handler context, which these methods do not take. The new test file pins the behavior being moved: every arm of the response oneof, the error each one produces, the outcome tag, and the failure-source header. It passes both before and after this commit, so the following commits that change this logic have to say so in the assertions they change.

matching's DispatchNexusTaskResponse has four outcome arms, two of which nest another oneof, and the deprecated arms carry the same information in an older encoding. That expands to nine distinct things a caller has to handle. Today the frontend is the only place that does it, and the worker-callbacks path needs the same answers. This adds DispatchOutcome and DispatchResult, which flatten all of it into one set of named outcomes plus the fields that arm carried, and the two entry points that produce them: ClassifyStartOperationDispatch and ClassifyCancelOperationDispatch. Two normalizations happen here, so that callers never see the deprecated shapes: - A deprecated nexuspb.HandlerError becomes a failurepb.Failure carrying NexusHandlerFailureInfo, i.e. what a current worker sends. - A deprecated nexuspb.UnsuccessfulOperationError becomes a failurepb.Failure. The deprecated variant reports the operation state in a field of its own and sends the handler's failure bare; the current format has no state field and instead reports the state through the failure wrapping the handler's failure. So the wrapper is rebuilt here from the state field: a canceled failure, or a non-retryable "OperationError" application failure. DispatchResult.OutcomeTag is the single place the outcome metric tag is derived. The existing tag values are kept as they are, including the ones that read oddly ("success" for an accepted cancel, "handler_error:EMPTY_OUTCOME" for a response that never reached a handler), because dashboards query them. Two exceptions, both existing bugs: - A worker failure carrying no handler failure info tagged "handler_error:" with an empty suffix. It now reports "handler_error:UNKNOWN". - The suffix was the error type string the worker chose, so a worker could mint unbounded time series. Types outside the Nexus spec now collapse to UNKNOWN. Nothing calls this yet, so there is no behavior change in this commit. The two tag fixes take effect where the frontend starts using OutcomeTag, and the normalizations where each caller switches over.

MatchingDispatchResponseToError now classifies the response first and converts the outcome, via the new DispatchResultToError. StartOperationResponseToError had no callers left once the nesting moved into the classifier, so it goes away. Two behavior changes for the one caller, common/workercommands.Dispatcher: - A worker answering with a deprecated response variant used to fall through to "empty or unknown dispatch outcome", a retryable internal handler error. The dispatcher read that as a transport failure and retried the task forever. It now produces the worker's actual failure, which the dispatcher treats as permanent, matching what it does for the current variants. - A deprecated operation error is now reported as the failure the current format sends: an ApplicationError of type "OperationError" wrapping the worker's own failure, rather than the worker's failure alone. Handler failures keep their type and retry behavior in both formats, which is what the dispatcher branches on. The tests are rewritten around that split -- *nexus.HandlerError means the task never got an answer and Retryable() decides whether to re-deliver, anything else is the worker's answer and permanent -- and assert both response formats land on the same side of it.

Both outcome methods now classify the response once up front and hand the result to recordDispatchOutcome, which sets the metrics outcome tag and, for anything that is not a success, the failure-source response header. The per-arm tag and setFailureSource lines go away; the arms still build the errors themselves, which the next commit changes. The tag values are unchanged except for the two handler-error-type fixes that came with DispatchResult.OutcomeTag: - A worker failure with no handler failure info tagged "handler_error:". It now tags "handler_error:UNKNOWN". - A handler error type outside the Nexus spec got its own tag value. It now collapses to "handler_error:UNKNOWN". Both are visible in the assertions this commit changes. The error returned to the caller still carries the worker's real error type; only the metric is bounded.

Both outcome methods now read the classified DispatchResult instead of switching over the response proto themselves. The arms that are identical for a start and a cancel dispatch -- a handler failure, a worker failure, a request timeout, an unrecognized outcome -- move into one shared dispatchFailureToNexusError, leaving startOperationOutcome with only what is specific to starting an operation. convertOutcomeToNexusHandlerError goes away with the last switch that used it. The behavior change is confined to workers that answer in a deprecated format, which the classifier normalizes into a failurepb.Failure: - A deprecated handler error is converted through the Temporal failure converter rather than assembled directly, so its cause reaches the caller in the same shape a current worker's would. The type, retry behavior and cause message are unchanged. - A deprecated operation error is re-encoded into what a current worker sends: the operation state as the wrapping failure, and the worker's own failure underneath it. A failed operation therefore now reaches the caller as an ApplicationFailure{Type: "OperationError", NonRetryable: true} wrapping the worker's failure, where before the worker's failure was sent bare. The worker's message, metadata and details all survive, one level deeper. The functional tests are updated for that extra level. Both the current and the deprecated response format now assert the same cause chain, which is the point: the two formats become indistinguishable to the caller. The legacy-only branch of the operation_error assertions read the worker's metadata and details straight off operationError.Cause, which only held while the deprecated failure was passed through untouched, so it is replaced by the one difference that remains: the rebuilt wrapper repeats the worker's message, where a current worker's wrapper has no message of its own.

bergundy

Looks great. It's a bit of effort to ensure that the semantics were preserved so I might spin up another agent to review that just in case the test coverage is missing.

bergundy · 2026-08-31T22:48:53Z

+func ClassifyCancelOperationDispatch(resp *matchingservice.DispatchNexusTaskResponse) DispatchResult {
+	return baseClassifyDispatchNexusTaskResponse(
+		resp,
+		func(*nexuspb.StartOperationResponse) DispatchResult {


Seems strange that this function receives a StartOperationResponse when that's not a viable outcome. I can see why you did this but it would be cleaner to split the implementation to:

func ClassifyStartOperationDispatch(...) ... { // Is it start success? inline classifyStartOperationResponse // Otherwise, call baseClassifyDispatchNexusTaskResponse but rename it to classifyDispatchNexusTaskFailureReponse } func ClassifyCancelOperationDispatch(...) ... { // Is it cancel success? return what's in the following line. // Otherwise, call baseClassifyDispatchNexusTaskResponse but rename it to classifyDispatchNexusTaskFailureReponse }

The AI has been nagging me about the same thing, so I guess I'll change it :P

FWIW, the rational was to keep all of the variants of DispatchNexusTaskResponse in the same place. Rather than only having partial coverage in a "failures only" classification. But since we have a default block already, it's not really any worse...

stephanos · 2026-08-31T15:32:55Z

+	require.Empty(t, r.Links)
+}
+
+func TestClassifyStartOperationDispatch_AsyncSuccess(t *testing.T) {


WDYT about using a single data table for TestClassifyStartOperationDispatch*? And using a single DispatchResult to match against (instead of separate require.Equals)? It could read really nicely I think. The scattered test methods are hard to put together into a coherent picture (for me).

stephanos · 2026-08-31T16:48:08Z

+				s.Equal(map[string]string{"k": "v"}, workerFailure.Metadata)
+				var details string
+				s.NoError(json.Unmarshal(workerFailure.Details, &details))
+				s.Equal("details", details)


nit: could we add newlines to group things here? I have the hardest time reading this.

stephanos · 2026-08-31T16:48:12Z

+				require.NoError(t, appErr.Details(&workerFailure))
+				require.Equal(t, map[string]string{"k": "v"}, workerFailure.Metadata)
+				var details string
+				require.NoError(t, json.Unmarshal(workerFailure.Details, &details))


nit: could we add newlines to group things here? I have the hardest time reading this.

stephanos · 2026-08-31T16:53:46Z

+//
+// Links are returned to the caller instead of being attached here: nexus.AddHandlerLinks requires the
+// SDK's handler context.
+func (c *operationContext) startOperationOutcome(


WDYT about handleStartOperationResponse? The start* reads like it'll start sth.

stephanos · 2026-08-31T16:54:00Z

+// cancelOperationOutcome converts matching's response to a CancelOperation dispatch into the error the
+// Nexus SDK expects, recording the metrics outcome tag and the failure-source response header along
+// the way. A nil error means the cancel was accepted.
+func (c *operationContext) cancelOperationOutcome(


WDYT about handleCancelOperationResponse?

stephanos · 2026-08-31T16:55:08Z

+// dispatchFailureToNexusError converts the outcomes that mean the task was never handled, or was
+// refused outright, into the error to report to the caller. These arms are identical for every kind of
+// dispatched request, so both handler methods share them.
+func (c *operationContext) dispatchFailureToNexusError(


WDYT about failedDispatchToNexusError?

stephanos · 2026-08-31T17:02:02Z

+	nf, err := commonnexus.TemporalFailureToNexusFailureInPlace(failure)
+	if err != nil {
+		c.logger.Error("error converting Temporal failure to Nexus failure",
+			tag.Error(err), tag.Operation(operation), tag.WorkflowNamespace(c.namespaceName))


Aren't these tags apart from tag.Error already set on c.logger? (same question for all other invocations)

stephanos · 2026-08-31T17:05:40Z

+
+	// SyncPayload is the operation's result. Set for DispatchOutcomeSyncSuccess, where it may still
+	// be nil: an operation is allowed to succeed with no value.
+	SyncPayload *commonpb.Payload


WDYT about calling this OperationResult? I liked the Sync prefix, but if we aren't going to call the other field AsyncOperationToken (which I think we shouldn't), OperationResult makes the connection clearer IMO.

stephanos · 2026-08-31T17:17:16Z

+// With an integer enum, iota made duplicate values impossible. With string values a copy-paste can
+// silently alias two outcomes into one, so assert they stay distinct and non-empty -- the empty string
+// is the zero value and must not name a real outcome.
+func TestDispatchOutcomeValuesAreDistinct(t *testing.T) {


Do we need this test? If we make metricOutcome use a distinct, exhaustive switch statement; the linter will enforce that these are all distinct for us.

chrsmith force-pushed the chrsmith/refactor-nexus-dispatch-outcome branch 3 times, most recently from b5e4810 to dd77619 Compare August 28, 2026 22:11

chrsmith marked this pull request as ready for review August 28, 2026 22:13

chrsmith requested review from a team as code owners August 28, 2026 22:13

chrsmith force-pushed the chrsmith/refactor-nexus-dispatch-outcome branch from dd77619 to 69266ae Compare August 28, 2026 22:13

chrsmith mentioned this pull request Aug 28, 2026

Support Worker-variant callbacks #11589

Open

5 tasks

github-actions Bot reviewed Aug 28, 2026

View reviewed changes

Comment thread common/nexus/dispatch_outcome.go

github-actions Bot reviewed Aug 28, 2026

View reviewed changes

Comment thread common/nexus/dispatch_outcome.go Outdated

github-actions Bot reviewed Aug 28, 2026

View reviewed changes

Comment thread common/nexus/dispatch_outcome_test.go

chrsmith requested review from bergundy and stephanos August 28, 2026 22:32

chrsmith force-pushed the chrsmith/refactor-nexus-dispatch-outcome branch from 1bc7230 to 5759b31 Compare August 30, 2026 19:48

chrsmith requested review from a team as code owners August 30, 2026 19:48

temporalio deleted a comment from github-actions Bot Aug 30, 2026

github-actions Bot mentioned this pull request Aug 30, 2026

🦞 OpenClaw 生态日报 2026-08-31 junlinzhao327-oss/big_model_radar#495

Open

chrsmith force-pushed the chrsmith/refactor-nexus-dispatch-outcome branch 2 times, most recently from 96a2490 to 312ab42 Compare August 31, 2026 01:45

chrsmith added 5 commits August 30, 2026 19:06

chrsmith force-pushed the chrsmith/refactor-nexus-dispatch-outcome branch from 312ab42 to 3289f1e Compare August 31, 2026 15:57

bergundy approved these changes Aug 31, 2026

View reviewed changes

stephanos reviewed Sep 1, 2026

View reviewed changes

Uh oh!

Conversation

chrsmith commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed?

Context

Functional Changes

Why?

How did you test it?

Potential Risks

Uh oh!

github-actions Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bergundy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephanos Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chrsmith commented Aug 28, 2026 •

edited

Loading

github-actions Bot commented Aug 28, 2026 •

edited

Loading

stephanos Aug 31, 2026 •

edited

Loading