Skip to content

NEXUS-484: Support UpdateWorkflow as a Nexus Operation#1631

Merged
mavemuri merged 3 commits into
mainfrom
mavemuri/update-nexus-op
Jul 22, 2026
Merged

NEXUS-484: Support UpdateWorkflow as a Nexus Operation#1631
mavemuri merged 3 commits into
mainfrom
mavemuri/update-nexus-op

Conversation

@mavemuri

@mavemuri mavemuri commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

What was changed

Adds support for UpdateWorkflow as a Nexus operation

Why?

Part of effort to expose all Temporal primitives via Nexus operations

Checklist

  1. Closes
    NEXUS-484

  2. How was this tested:

  • added integration test
  1. Any docs updates needed?

@CLAassistant

CLAassistant commented Jul 2, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@mavemuri
mavemuri force-pushed the mavemuri/update-nexus-op branch 6 times, most recently from a913bdd to 79d24c1 Compare July 2, 2026 18:52
Comment thread tests/nexus/test_temporal_operation.py Outdated
result = await wf_handle.result()
assert result == "Updated workflow status from Pending to Created"

# await assert_event_subsequence(

@mavemuri mavemuri Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO:

  • check why this is flaky on ubuntu-arm/windows
  • verify links are present
  • check resp is sync on the retried update below

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably obvious, but just to note we should try to match the test coverage we have in Go

@mavemuri
mavemuri force-pushed the mavemuri/update-nexus-op branch 2 times, most recently from ecc3c9e to e38becf Compare July 13, 2026 20:00
Comment thread temporalio/nexus/_temporal_client.py Outdated
result_type: type | None = None,
rpc_metadata: Mapping[str, str | bytes] = {},
rpc_timeout: timedelta | None = None,
# run_id: str | None = None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these commented out

@mavemuri mavemuri Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasnt sure if they were required at the time, fixed it



@dataclass(frozen=True)
class CancelUpdateWorkflowOptions:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing run ID

@mavemuri
mavemuri force-pushed the mavemuri/update-nexus-op branch 6 times, most recently from d8db6d8 to 351568c Compare July 16, 2026 20:03
Comment thread temporalio/client/_workflow.py Outdated
Comment on lines +973 to +977
if run_id == None:
run_id = self._run_id
if first_execution_run_id == None:
first_execution_run_id = self.first_execution_run_id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to keep this, let's make sure to use is None

Suggested change
if run_id == None:
run_id = self._run_id
if first_execution_run_id == None:
first_execution_run_id = self.first_execution_run_id
if run_id is None:
run_id = self._run_id
if first_execution_run_id is None:
first_execution_run_id = self.first_execution_run_id

Comment thread temporalio/nexus/_operation_context.py
Comment thread temporalio/nexus/_temporal_client.py
Comment thread temporalio/nexus/_temporal_client.py Outdated
arg: Any = temporalio.common._arg_unset,
*,
args: Sequence[Any] = [],
id: str | None = None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should consider naming this update_id to more clearly differentiate between workflow_id and the other IDs necessary.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code seems to be using "id" as the defacto operation-relevant unique ID - eg. start_workflow uses "id" for workflowID - this is the case for the primitive start_update/etc as well -

id: ID of the update. If not set, the default is a new UUID.

Was trying to follow that but explicit does make sense. It might look inconsistent if update calls out specific and start_workflow doesnt- because that part is already solidified/shipped iiuc

I agree with changing it update_id - because update has both workflow_id and update_id - but just not sure because of this

Comment thread temporalio/nexus/_temporal_client.py Outdated
Comment on lines +340 to +341
# draft-review: check why run_id and first_execution_run_id are not used
# for update workflow in python sdk

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are sourced from the WorkflowHandle rather than passed as arguments.

Comment thread temporalio/nexus/_temporal_client.py Outdated
first_execution_run_id=first_execution_run_id,
)
# If the update has already completed, return the result synchronously
# This is in-line with the Go implementation as well

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# This is in-line with the Go implementation as well

Comment thread temporalio/client/_workflow.py Outdated
Comment on lines +959 to +960
run_id: str | None = None,
first_execution_run_id: str | None = None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in other comments, I think we should remove these and always use self._run_id and self._first_execution_run_id

Comment thread temporalio/nexus/_temporal_client.py Outdated
Comment on lines +507 to +509
nexus_handle: UpdateHandle[Any] = (
UpdateHandle._unsafe_from_client_workflow_update_handle(update_handle)
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the workflow variant uses a WorkflowHandle here, but I'm not sure it's necessary. I think we should remove the UpdateHandle and just contruct the appropriate OperationToken here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense- will just re-generate from the handle there

Comment thread temporalio/nexus/_operation_handlers.py Outdated
Comment on lines +237 to +238
# draft-review: maybe just move it inline, no need for a function just to error out
# check after review in case theres some other way to override/supply custom cancels

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way to customize cancellation is to inherit from TemporalOperationHandler and override these methods. So we'll either need to rework how that's done or leave these methods in place.

@mavemuri
mavemuri force-pushed the mavemuri/update-nexus-op branch 8 times, most recently from 83ed716 to a750f5f Compare July 17, 2026 05:24
@mavemuri
mavemuri marked this pull request as ready for review July 17, 2026 16:17
@mavemuri
mavemuri requested a review from a team as a code owner July 17, 2026 16:17
@mavemuri
mavemuri force-pushed the mavemuri/update-nexus-op branch from a750f5f to 1d14414 Compare July 17, 2026 22:22
first_execution_run_id=first_execution_run_id,
)
# If the update has already completed, return the result synchronously
if update_handle._known_outcome is not None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this case happens should we clear the flag set in _reserve_async_start() that prevents 2 async operations from being triggered by the client? Thinking through it, it probably makes sense to just always leave the flag set otherwise it could feel a little non-deterministic, but thought I'd pose the question for others to also consider.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with leaving as-is - how it ended - sync/async- isnt strictly related to how it started - and its always starts async because we only support wait for Accepted
Also brought up in this PR discussion - temporalio/sdk-java#2945 (comment) - this is consistent there too

@VegetarianOrc VegetarianOrc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! One question and one double checking for if the new UpdateHandle is used.

Comment thread temporalio/nexus/_token.py Outdated
Comment on lines +214 to +216
@dataclass(frozen=True)
class UpdateHandle(Generic[OutputT]):
"""A handle to a workflow update that is backing a Nexus operation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be removed now? Is it used somewhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed it, will remove

@mavemuri
mavemuri force-pushed the mavemuri/update-nexus-op branch from 1d14414 to aa738a4 Compare July 20, 2026 20:39
@mavemuri
mavemuri force-pushed the mavemuri/update-nexus-op branch from aa738a4 to 5a61843 Compare July 20, 2026 20:39
Comment thread temporalio/client/_interceptor.py Outdated
ret_type: type | None
rpc_metadata: Mapping[str, str | bytes]
rpc_timeout: timedelta | None
# The following options are for Nexus Operation-backed updates. Experimental and unstable

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the comment. "Nexus operation backed updates" sounds like the update is running as a nexus operation or something. This is just the nexus operation invoking an update, right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes thats right

Makes sense, will update and clarify that this is a workflow updates exposed as Nexus ops

Comment thread temporalio/nexus/_operation_handlers.py Outdated
ctx: TemporalCancelOperationContext, # pyright: ignore[reportUnusedParameter]
options: CancelUpdateWorkflowOptions, # pyright: ignore[reportUnusedParameter]
) -> None:
"""Cancels the workflow update backing the Nexus operation. Cancellation is not natively supported for update-workflow Nexus operations.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite follow this guy either. How would a user implement this in a way that we can't by edfault?

@mavemuri mavemuri Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that any accepted update cannot be cancelled anymore by the server. To work around it, the user would have to add some app-specific code that is aware of the update implementation to cancel it - like sending some term/etc to the update handler. Because of this, the user would have to override themselves (and as there cannot be a default "cancel" for update, raise this error instead)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would elaborate just a bit that the custom implementation would need to cancel in some way outside temporal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

@mavemuri
mavemuri force-pushed the mavemuri/update-nexus-op branch from 86eba03 to 6c50143 Compare July 21, 2026 23:39
@mavemuri
mavemuri merged commit 29e0c51 into main Jul 22, 2026
30 of 31 checks passed
@mavemuri
mavemuri deleted the mavemuri/update-nexus-op branch July 22, 2026 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants