Skip to content

Require wait_for_stage parameter for Nexus Workflow Updates - #1883

Open
atol wants to merge 2 commits into
mainfrom
alicelin/NEXUS-765/wait-for-stage
Open

atol wants to merge 2 commits into
mainfrom
alicelin/NEXUS-765/wait-for-stage

Conversation

@atol

@atol atol commented Sep 17, 2026

Copy link
Copy Markdown

What was changed

Expose and require the wait_for_stage parameter to be explicitly set to ACCEPTED for Nexus Workflow Updates, instead of being hardcoded internally.

Why?

wait_for_stage is expected to be a required parameter because ACCEPTED is a non-obvious default that users should be aware of. This also aligns with the behaviour in the Go, Java, and .NET SDKs.

How was this tested

Add new test.

@CLAassistant

CLAassistant commented Sep 17, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@atol
atol marked this pull request as ready for review September 17, 2026 18:40
@atol
atol requested a review from a team as a code owner September 17, 2026 18:40
update: temporalio.workflow.UpdateMethodMultiParam[MultiParamSpec, ReturnType],
*,
args: MultiParamSpec.args, # type: ignore
wait_for_stage: Literal[temporalio.client.WorkflowUpdateStage.ACCEPTED],

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 we requiring they pass a specific literal that we already know?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@Quinn-With-Two-Ns has more context, but the gist is that ACCEPTED is an unintuitive default that has historically confused users, so we want to make them aware of the implications of the option (i.e. the worker needs to be running).

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 think a nonintuitive default is really a good reason for a parameter, it's a good reason for a docstring. I don't think making every user and callsite provide this argument so that some users don't miss a docstring.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Discussed offline. We plan to make ADMITTED the default in the future and want to avoid breaking users later.

Comment thread temporalio/nexus/_operation_context.py Outdated
) -> temporalio.client.WorkflowUpdateHandle[Any]:
# Annotations are not enforced at runtime, so validate anyway. The cast widens the
# narrowed Literal; without it the check reads as unreachable to the type checker.
if cast(Any, wait_for_stage) != temporalio.client.WorkflowUpdateStage.ACCEPTED:

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 cast to the enum rather than Any. Alternatively we can just add # type: ignore instead of casting. We might also consider allowing this internal method to accept the regular enum and requiring the literal on the public API as you've got here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion! Updated the param type to WorkflowUpdateStage.

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.

4 participants