Implement GetWorkflowExecutionResult by long-nt-tran · Pull Request #10173 · temporalio/temporal

long-nt-tran · 2026-05-04T20:52:47Z

What changed?

Server PR for API counterpart temporalio/api#778

This API allows users to get the result of a workflow execution and optionally attach callbacks to be fired when the workflow completes (if the workflow is running). It follows continue-as-new chain up to GetWorkflowExecutionResultMaxCANChainDepth iterations (for now defined to be 100 in dynamic config).

A lot of boilerplate from making a new API (proto + mock generated lines). Business logic to note are in:

Frontend handler: service/frontend/workflow_handler.go
History handler: service/history/handler.go
Actual API Invoke(...) implementation: service/history/api/getworkflowexecutionresult/api.go

Why?

Allows Nexus handler to map to this operation more ergonomically for callers.

How did you test it?

Potential risks

New API -- need more test across repos (load testing, etc...). Gated behind dynamic config.

long-nt-tran · 2026-05-07T22:44:49Z

+	req.Service = cmp.Or(req.Service, "test-service")
+	req.Operation = cmp.Or(req.Operation, "test-operation")


opportunistic refactor from nexus_standalone_test.go, but maybe slightly overboard with still keeping these defaults here :) LMK if folks prefer to not have defaults at all

works for me 👍

long-nt-tran · 2026-05-07T22:45:41Z

+		leafErr := pollResp.GetFailure()
+		for leafErr.GetCause() != nil {
+			leafErr = leafErr.GetCause()
+		}
+		protorequire.ProtoEqual(s.T(),
+			&failurepb.Failure{Message: intentionalFailureMsg},
+			leafErr,
+			protorequire.IgnoreFields("source", "stack_trace", "encoded_attributes", "application_failure_info"),
+		)


this seemed slightly clunky to me but I wasn't sure there was another way to unwrap and assert on this base error, LMK if there is

Quinn-With-Two-Ns · 2026-05-08T13:29:59Z

+
+	// Walk the CAN chain until we reach the head. At head, we will return the result of the head's workflow run,
+	// and optionally attach callbacks if hasCallbacks.
+	for range maxCANChainDepth {


This was what I did in my prototype, but I question if this is the right approach to have an arb. limit here. I wonder if we could be smarter but it would require a larger effort. I was thinking if we only support targeting the latest run ID then we don't need to tackle this problem yet.

commenting for posterity per offline discussion: will always get result for latest (empty) run id for now to make this simpler

should we remove the code if it's effectively dead?

I think this code is already deleted

- Always default to latest runID (empty) - Incorporate latest API proto changes

stephanos · 2026-05-08T16:16:35Z

+	req.Service = cmp.Or(req.Service, "test-service")
+	req.Operation = cmp.Or(req.Operation, "test-operation")


works for me 👍

stephanos · 2026-05-13T00:22:23Z

+	if namespaceID == "" {
+		return nil, h.convertError(errNamespaceNotSet)
+	}


This is redundant; ValidateNamespaceUUID already contains that check.

stephanos · 2026-05-13T00:29:23Z

+	history := env.SdkClient().GetWorkflowHistory(env.Context(), run.GetID(), "", false, enumspb.HISTORY_EVENT_FILTER_TYPE_ALL_EVENT)
+	foundEvent := false
+	for history.HasNext() {
+		event, err := history.Next()
+		s.NoError(err)
+		if event.EventType != enumspb.EVENT_TYPE_WORKFLOW_EXECUTION_OPTIONS_UPDATED {
+			continue
+		}
+		foundEvent = true
+		protorequire.ProtoSliceEqual(s.T(), links, event.Links)
+	}
+	s.True(foundEvent)


could use HistoryRequire's RequireHistoryEvent

stephanos · 2026-05-13T21:35:00Z

+	if err := validateRequestId(&request.RequestId, wh.config.MaxIDLengthLimit()); err != nil {
+		return nil, err
+	}
+


WDYT about adding this?

if req.GetExecution().GetRunId() != "" { return nil, serviceerror.NewInvalidArgument("RunId is not supported") }

stephanos · 2026-05-13T22:44:29Z

+			// Write path: there are completion callbacks or links, and this workflow execution is running:
+			// 1. Attach callbacks and links via AddWorkflowExecutionOptionsUpdatedEvent
+			// 2. Set response for caller with backlinks
+			// 3. Return with Noop=false so the attachments get persisted.


create comments in this closure!

I assumed you meant Return with Noop=false so the attachments get persisted. right above where it is

stephanos · 2026-05-13T23:16:18Z

+		testcore.WithDynamicConfig(dynamicconfig.EnableChasm, true),
+		testcore.WithDynamicConfig(nexusoperation.Enabled, true),
+	)
+}


inline if only used once?

stephanos · 2026-05-13T23:17:03Z

+	}
+}
+
+func newGetResultNexusEnv(t *testing.T, opts ...testcore.TestOption) *NexusTestEnv {


the other suites often follow the pattern of having a newTestEnv method

stephanos · 2026-05-13T23:17:36Z

+// runViaCallerWF runs the scenario via a caller workflow that uses
+// workflow.NewNexusClient to invoke the operation. The caller workflow itself is
+// opaque about what target it's querying; that's wired into the handler.
+func runViaCallerWF(s *NexusGetWorkflowResultTestSuite, spec *targetWFSpec) {


This can be a method, too (no?)

I was running tests via a runner so had this as a helper so i pass down the right s. Elevated this and runViaStandalone(...) to be suite test functions, copying pattern from temporal/tests/task_queue_stats_test.go

stephanos · 2026-05-13T23:18:42Z

+// Each test runs both WorkflowWrapped and Standalone subtests.
+// =============================================================================
+
+func (s *NexusGetWorkflowResultTestSuite) TestGetResult() {


check out temporal/tests/task_queue_stats_test.go to do this a little better IMO by leveraging suites fully

stephanos · 2026-05-13T23:36:22Z

+		)
+	}
+	return resp, nil
+}


I think some of these code paths aren't tested; might be good to do (unit test is likely easier than func test)

added service/history/api/getworkflowexecutionresult/api_test.go

spkane31 · 2026-05-15T17:57:38Z

+	if err := validateRequestId(&request.RequestId, wh.config.MaxIDLengthLimit()); err != nil {
+		return nil, err
+	}
+	if request.GetExecution().GetRunId() != "" {


Why not support getting the result of a specific run id?

Because if the workflow run continued as new we would have to follow continue as new errors to get the result

I think we want to support the ability to follow continue as new errors. Also there's no guarantee that they want the result for the most recent run of a workflow. The user might not know if the run that they want was the most recent instance of a workflow.

long-nt-tran force-pushed the get-workflow-result branch 10 times, most recently from 71e932b to 9ec54e6 Compare May 6, 2026 21:40

long-nt-tran added 2 commits May 7, 2026 18:19

GetWorkflowExecutionResult implementation

004e24f

Add integration tests

807f96b

long-nt-tran force-pushed the get-workflow-result branch from 9ec54e6 to 807f96b Compare May 7, 2026 22:19

long-nt-tran mentioned this pull request May 7, 2026

Add API for GetWorkflowExecutionResult temporalio/api#778

Open

long-nt-tran requested review from Quinn-With-Two-Ns and stephanos May 7, 2026 22:39

long-nt-tran marked this pull request as ready for review May 7, 2026 22:43

long-nt-tran requested review from a team as code owners May 7, 2026 22:43

long-nt-tran commented May 7, 2026

View reviewed changes

Comment thread service/history/api/getworkflowexecutionresult/api.go

Quinn-With-Two-Ns reviewed May 8, 2026

View reviewed changes

Comment thread common/api/metadata.go Outdated

Quinn-With-Two-Ns reviewed May 8, 2026

View reviewed changes

long-nt-tran added 3 commits May 8, 2026 13:26

PR feedback

702e41d

- Always default to latest runID (empty) - Incorporate latest API proto changes

Merge remote-tracking branch 'upstream/main' into get-workflow-result

d5ac2e1

Change metadata access to write

641e68a

long-nt-tran requested a review from Quinn-With-Two-Ns May 8, 2026 17:46

stephanos reviewed May 13, 2026

View reviewed changes

long-nt-tran added 5 commits May 14, 2026 16:38

PR feedback

cf2b61c

Merge remote-tracking branch 'upstream/main' into get-workflow-result

167d837

Fix fmt

f2d1b65

Update to latest protos

49c50a7

Merge remote-tracking branch 'upstream/main' into get-workflow-result

de6f821

long-nt-tran requested a review from stephanos May 15, 2026 15:13

Merge remote-tracking branch 'upstream/main' into get-workflow-result

4085af0

spkane31 reviewed May 15, 2026

View reviewed changes

		req.Service = cmp.Or(req.Service, "test-service")
		req.Operation = cmp.Or(req.Operation, "test-operation")

Conversation

long-nt-tran commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed?

Why?

How did you test it?

Potential risks

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

long-nt-tran commented May 4, 2026 •

edited

Loading