Add new constructor for WorkflowStub to target correct execution #2709
Conversation
Quinn-With-Two-Ns
force-pushed
the
issue-2651
branch
from
790dbd6 to
ae470f8
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Inconsistent Update Options: Missing firstExecutionRunId Handling
The startUpdate(String updateName, WorkflowUpdateStage waitForStage, Class<R> resultClass, Object... args) method creates UpdateOptions without setting the firstExecutionRunId field, while the update(String updateName, Class<R> resultClass, Object... args) method (line 349) does set it from the stub's firstExecutionRunId field. This inconsistency means that updates started via the convenience method won't respect the firstExecutionRunId targeting, potentially causing incorrect workflow targeting behavior when the workflow has continued-as-new.
temporal-sdk/src/main/java/io/temporal/client/WorkflowStubImpl.java#L360-L370
sdk-java/temporal-sdk/src/main/java/io/temporal/client/WorkflowStubImpl.java
Lines 360 to 370 in ae50fa9
![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.
ℹ️ 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".
temporal-sdk/src/main/java/io/temporal/client/WorkflowClientInternalImpl.java
Show resolved
Hide resolved
|
|
||
| private void populateExecutionAfterStart(WorkflowExecution startedExecution) { | ||
| this.startedExecution.set(startedExecution); | ||
| // this.firstExecutionRunId.set(startedExecution.getRunId()); |
There was a problem hiding this comment.
Bug: Follow-Through Failure: Immutable First Execution Run ID
The firstExecutionRunId field is declared as final but needs to be populated after workflow start. The commented-out line // this.firstExecutionRunId.set(startedExecution.getRunId()); in populateExecutionAfterStart() reveals this issue - it attempts to call .set() on a String which would fail. Because firstExecutionRunId remains null after starting a workflow, subsequent operations (cancel, terminate, update) will not properly follow the workflow chain using firstExecutionRunId, breaking the intended "follow first run ID" functionality for workflows that continue-as-new.
There was a problem hiding this comment.
LGTM, especially knowing that no code in use today will break (though users may get a bit annoyed when these deprecation warnings pop up). Usually would leave this PR up to other Java reviewers, but thought I'd put my approval on too since I had discussions on the concerns previously.
| * @param workflowTargetOptions options that specify target workflow execution. | ||
| * @return Stub that implements workflowInterface and can be used to signal or query it. | ||
| */ | ||
| <T> T newWorkflowStub(Class<T> workflowInterface, WorkflowTargetOptions workflowTargetOptions); |
There was a problem hiding this comment.
Pedantic, but I like WorkflowStubOptions name myself in case we ever want an option unrelated to "workflow target"
There was a problem hiding this comment.
Yeah my reason for not using WorkflowStubOptions was because these options are specifically for creating a stub that is attached to a running workflow, instead of a stub that can start a workflow
maciejdudko
left a comment
maciejdudko
left a comment
There was a problem hiding this comment.
One small comment, I'll approve after it gets resolved.
temporal-sdk/src/main/java/io/temporal/client/WorkflowStubImpl.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
3 participants
Add new constructor for WorkflowStub to target correct execution. Before the PR
WorkflowStubwas very inconsistent in what workflow execution it targets, sometimes it would ignore the RunID, sometimes it wouldn't. This PR keeps the old inconsistent behaviour if a user uses the old constructor and adds a new constructor that always uses the execution the user specified. It also adds the ability to target byFirstExecutionRunIDto target a workflow chain.Note
Introduces WorkflowTargetOptions-based APIs and updates stubs to target the specified workflow/run (with optional firstExecutionRunId), deprecating legacy constructors and updating requests and tests accordingly.
WorkflowTargetOptions(withworkflowId,runId,firstExecutionRunId).newWorkflowStub(Class, WorkflowTargetOptions)andnewUntypedWorkflowStub(WorkflowTargetOptions[, Optional<String>]).newWorkflowStub(Class, String, Optional<String>)and untyped variants takingWorkflowExecution/runId.runId; addlegacyTargetingflag to preserve old behavior for deprecated paths.firstExecutionRunIdthroughWorkflowStubImplandWorkflowInvocationHandlerfor signals/cancel/terminate/updates.runIdto follow chains.WorkflowClientCallsInterceptor.CancelInput/TerminateInputand Root invoker to sendfirstExecutionRunId.firstExecutionRunIdin Update request inputs.WorkflowStubRespectRunIdTestvalidating signal/query/cancel/terminate behavior across continue-as-new.(workflowId, runId).Written by Cursor Bugbot for commit 1776365. This will update automatically on new commits. Configure here.