[SDK Ergonomics] NEXUS-519: Support Query-backed Nexus Operations - #11274
[SDK Ergonomics] NEXUS-519: Support Query-backed Nexus Operations#11274mavemuri wants to merge 2 commits into
Conversation
c6bd436 to
d0784fb
Compare
| go.opentelemetry.io/otel/sdk/metric v1.43.0 | ||
| go.opentelemetry.io/otel/trace v1.44.0 | ||
| go.temporal.io/api v1.63.4 | ||
| go.temporal.io/api v1.62.15-0.20260622232249-060670b1b866 |
There was a problem hiding this comment.
placeholder until api change is merged
52ae6e5 to
2657210
Compare
|
Claude finished @mavemuri's task in 7m 1s —— View job Review complete
Overall the change looks sound: the |
| // Note: QueryWorkflow should not alter mutable state, so it is safe to ignore error and not clear ms. | ||
| workflowLease.GetReleaseFn()(nil) | ||
| }() | ||
| defer func() { |
There was a problem hiding this comment.
common/links.Validate has no *commonpb.Link_Workflow_ case — it falls into default: and returns InvalidArgument: "unsupported link variant" (common/links/validator.go:60-61).
That makes the link this PR returns a dead end for the main thing links are for: taking a link you got back and attaching it to a follow-up request. A client that does
qResp, _ := c.WorkflowService().QueryWorkflow(ctx, ...)
c.WorkflowService().StartWorkflowExecution(ctx, &workflowservice.StartWorkflowExecutionRequest{
..., Links: []*commonpb.Link{qResp.GetLink()},
})gets InvalidArgument: unsupported link variant. Same for SignalWorkflowExecution, SignalWithStart, and UpdateWorkflowExecution (service/frontend/workflow_handler.go:693, 2241, 2286, 2350, 2445) plus the CHASM validators (chasm/lib/workflow/validator.go:220, chasm/lib/activity/link_validator.go:34).
Strictly this gap predates the PR — updateworkflow already emits Link_Workflow for rejected updates — but this change makes the variant reachable from every query response, so it's worth closing here. Adding the case alongside the Link_Activity_ one is a few lines:
case *commonpb.Link_Workflow_:
if t.Workflow.GetNamespace() == "" {
return serviceerror.NewInvalidArgument("workflow link must not have an empty namespace field")
}
if t.Workflow.GetWorkflowId() == "" {
return serviceerror.NewInvalidArgument("workflow link must not have an empty workflow ID field")
}
if t.Workflow.GetRunId() == "" {
return serviceerror.NewInvalidArgument("workflow link must not have an empty run ID field")
}| go.opentelemetry.io/otel/sdk/metric v1.43.0 | ||
| go.opentelemetry.io/otel/trace v1.44.0 | ||
| go.temporal.io/api v1.63.4 | ||
| go.temporal.io/api v1.62.15-0.20260622232249-060670b1b866 |
There was a problem hiding this comment.
Beyond the replace placeholder you already flagged below: this line also downgrades the require from v1.63.4 (which main just moved to in #11264) to the older v1.62.15-0.20260622232249-060670b1b866 pseudo-version. The replace masks that today, so it's easy to miss — but if the replace is dropped without also restoring the require, the module silently regresses to a pre-v1.63.4 API.
When temporalio/api#842 lands, the cleanup is: delete the replace, set this back to the released tag (>= v1.63.4), and restore the two go.temporal.io/api lines that were removed from go.sum.
2657210 to
ad84e98
Compare
|
High level looks good, but I would address all the AI's comments except for #11274 (comment) of course |
ad84e98 to
90722a6
Compare
| Variant: &commonpb.Link_Activity_{Activity: link}, | ||
| }) | ||
| case string((&commonpb.Link_Workflow{}).ProtoReflect().Descriptor().FullName()): | ||
| link, err := temporalnexus.ConvertNexusLinkToLinkWorkflow(nexusLink) |
There was a problem hiding this comment.
The two sibling cases use this repo's own converters (ConvertNexusLinkToLinkWorkflowEvent, ConvertNexusLinkToLinkActivity in common/nexus/link_converter.go), while this one reaches into go.temporal.io/api/temporalnexus. That's the only use of go.temporal.io/api/temporalnexus anywhere in the repo, and it is the sole reason for the api-go fork pin in go.mod.
common/nexus/link_converter.go opens with an explicit policy for exactly this situation:
This file is duplicated in sdk-go/temporalnexus/link_converter.go. Any changes here or there must be replicated. This is temporary until the temporal repo updates to the most recent SDK version.
Adding a local ConvertNexusLinkToLinkWorkflow next to the existing converters would keep all three URL path formats defined in one place, keep this switch internally consistent, and drop the need for the replace directive entirely. It's a small function and the file already owns the urlPath*Template/regex conventions it would follow.
There was a problem hiding this comment.
rejected
real fix is to eventually point all the converters to the helper from api-go for consistency
This pr intentionally addresses one to keep it scoped while avoid additional duplication. Cleaning up all the duplicates will be a different, more scoped pr
90722a6 to
a672bd2
Compare
| go.opentelemetry.io/otel/sdk/metric v1.43.0 | ||
| go.opentelemetry.io/otel/trace v1.44.0 | ||
| go.temporal.io/api v1.63.4 | ||
| go.temporal.io/api v1.62.15-0.20260622232249-060670b1b866 |
There was a problem hiding this comment.
This pins go.temporal.io/api to an unreleased fork via the replace at the bottom of the file, and at the same time downgrades the require line from v1.63.4 to v1.62.15-0.20260622232249-060670b1b866.
replace directives are only honored in the main module — they are ignored for anyone consuming go.temporal.io/server as a dependency. So downstream consumers will resolve the older v1.62.15-… api, which has neither QueryWorkflowResponse.Link nor temporalnexus.ConvertNexusLinkToLinkWorkflow, and common/nexus/links.go will fail to compile for them. Even with the replace kept in place locally, the require line shouldn't go backwards.
Flagging as blocking merge, consistent with the PR's own TODO on temporalio/api#842: once that's released, drop the replace and set require go.temporal.io/api to the released tag (≥ v1.63.4).
There was a problem hiding this comment.
ignore
this is a placeholder until the relevant api/api-go changes are reviewed and merged(already noted in TODO in this pr conversation)
it is kept this way for now for convenience of reviewing and running CI/tests etc
| if retError != nil || resp.GetResponse() == nil { | ||
| return | ||
| } | ||
| // Add link to Workflow regardless of query status as long as there isn't an error. |
There was a problem hiding this comment.
The "as long as there isn't an error" clause just restates the guard three lines above. What isn't obvious from the code is why this is a Link_Workflow rather than a Link_WorkflowEvent — same rationale as the rejected-update case in updateworkflow/api.go.
| // Add link to Workflow regardless of query status as long as there isn't an error. | |
| // Queries never write a history event, so link to the workflow itself. |
What changed?
Adds server support for WorkflowQuery-backed Nexus Operations
Why?
Part of effort to expose all Temporal primitives as Nexus Operations
How did you test it?
TODO: