Add Standalone Activities support to Temporal Nexus Operation Handler#748
Add Standalone Activities support to Temporal Nexus Operation Handler#748Quinn-With-Two-Ns wants to merge 7 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f807fc513
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Quinn-With-Two-Ns
force-pushed
the
NEXUS-389
branch
from
3f807fc to
84020ad
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Quinn-With-Two-Ns
force-pushed
the
NEXUS-389
branch
from
4f25c95 to
8520a4b
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 8520a4b. Configure here.
| EventRef = new() { EventId = 1, EventType = EventType.WorkflowExecutionStarted }, | ||
| }.ToNexusLink(); | ||
| NexusOperationExecutionContext.Current.HandlerContext.OutboundLinks.Add(nexusLink); | ||
| } |
There was a problem hiding this comment.
Signal-with-start drops Nexus links
Medium Severity
Outbound Nexus links for child workflow starts are only appended after a plain StartWorkflowExecution call. The SignalWithStartWorkflowExecution path returns without the same logic, so Nexus handlers that start workflows with StartSignal no longer populate OutboundLinks on the operation start response.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8520a4b. Configure here.
| options.RequestId = nexusStartContext.RequestId; | ||
|
|
||
| // Do the start call | ||
| var handle = await client.StartActivityAsync( |
There was a problem hiding this comment.
In the Go implementation, we check for timeout presence to raise a handler error and surface the validation error. Is that necessary here as well?
There was a problem hiding this comment.
The SDK already does some of this validation, Started a discussion in slack on where we want to do the validation.
jmaeagle99
left a comment
jmaeagle99
left a comment
There was a problem hiding this comment.
Mostly rename requests and I think there is opportunity for unifying the token types or at least refactoring to be more symmetrical and untied from handles (which can all be deferred).
| try | ||
| { | ||
| token = NexusWorkflowRunHandle.ParseToken(context.OperationToken); | ||
| tokenType = NexusWorkflowRunHandle.LoadTokenType(context.OperationToken); |
There was a problem hiding this comment.
I think the token parsing and related logic could use a refactor such that we use a discriminated union, something like:
[JsonPolymorphic(TypeDiscriminatorPropertyName = "t")]
[JsonDerivedType(typeof(WorkflowExecutionToken), 1)]
[JsonDerivedType(typeof(ActivityExecutionToken), 2)]
internal abstract record NexusOperationToken(string Namespace, int? Version);
Then when parsing the STJ, you'll get the exact token class out rather than having to pick out the t field yourself and then construct each type separately.
There was a problem hiding this comment.
I would be fine with deferring a refactor of this along with https://github.com/temporalio/sdk-dotnet/pull/748/changes/BASE..8520a4bcc34de23a824cf065f2e377f5f615a09b#r3477893944 as a separate change.
| { | ||
| NexusWorkflowRunHandle.WorkflowRunOperationTokenType => | ||
| CancelWorkflowRunAsync( | ||
| case NexusWorkflowRunHandle.WorkflowRunOperationTokenType: |
There was a problem hiding this comment.
Looking at the naming of the this token and helper methods vs the new one for standalone activities, they feel very asymmetrical when they really share a lot in common. For example, I think that the workflow run operation token should be separated from the handle notion and standard on either the "run" or "execution" name for both token types.
There was a problem hiding this comment.
I would be fine with deferring a refactor on this along with https://github.com/temporalio/sdk-dotnet/pull/748/changes/BASE..8520a4bcc34de23a824cf065f2e377f5f615a09b#r3477886756 as a separate change.
|
I assume that |
| /// <returns>Nexus link.</returns> | ||
| public static NexusLink ToNexusLink(this Api.Common.V1.Link.Types.Activity act) | ||
| { | ||
| var builder = new UriBuilder |
There was a problem hiding this comment.
Might need to fix this based on what was done here: https://github.com/temporalio/sdk-dotnet/pull/743/changes#diff-f041f09e4ee02b38caef08ec16893331f18864d25213ec951945bbc6bf23ea1f
3 participants
What was changed
Add Standalone Activities support to Temporal Nexus Operation Handler
Why?
Allow calling Standalone Activities as a Nexus Operation
Checklist
Closes
How was this tested:
Note
Medium Risk
Touches experimental Nexus/standalone-activity paths and start/cancel RPC wiring; behavior changes for workflow outbound links (moved from helper to client) but coverage includes integration tests.
Overview
Adds standalone activity backing for experimental Nexus
TemporalOperationHandler, alongside existing workflow-run operations.Handlers can call
ITemporalNexusClient.StartActivityAsync(lambda or by name) and get an async activity-execution operation token (typet=2).NexusActivityStartHelpermirrors workflow start plumbing: default task queue from the operation, Nexus completion callbacks withNexus-Operation-Token, inbound links, and request ID for idempotent retries.StartActivityOptionsgains internal fields wired throughStartActivityExecution(on-conflict attach, callbacks, links).Cancel dispatch now peeks token type via
ParseTokenType, then cancels the workflow or standalone activity;CancelActivityExecutionAsyncis overridable like workflow cancel.Shared link/callback logic moves to
NexusOperationStartHelper; workflow start docs now say task queue is optional. Outbound Nexus links after start are set inTemporalClientfrom serverresp.Link(workflow falls back to a syntheticWorkflowExecutionStartedlink on older servers). Activity proto ↔ Nexus link conversion is added inProtoLinkExtensions.Reviewed by Cursor Bugbot for commit 8ad5083. Bugbot is set up for automated code reviews on this repo. Configure here.