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
2 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
Introduce
WorkflowTargetOptionsto target workflows byworkflowId,runId, orfirstExecutionRunId; update typed/untyped stub creation to use it, deprecate legacy constructors, and ensure signal/cancel/terminate/update honor exact targeting.WorkflowTargetOptions(target byworkflowId,runId,firstExecutionRunId).newWorkflowStub(Class, WorkflowTargetOptions).newUntypedWorkflowStub(WorkflowTargetOptions)andnewUntypedWorkflowStub(WorkflowTargetOptions, Optional<String>).(workflowId, Optional<runId>)and(WorkflowExecution, Optional<workflowType>).WorkflowStubImplnow respects explicitrunIdby default; preserves legacy behavior vialegacyTargetingfor deprecated paths.firstExecutionRunIdto updates, cancel, and terminate; add helpers to choose current vs chain (currentExecutionCheckLegacy).WorkflowClientCallsInterceptor.CancelInputandTerminateInputwithfirstExecutionRunId; service invoker forwards it to server requests.WorkflowInvocationHandlerandWorkflowClientInternalImplto build stubs fromWorkflowTargetOptionsand pass targeting flags.WorkflowTargetOptions.WorkflowStubRespectRunIdTest).SDKTestWorkflowRule: add history fetch/assert helpers that acceptrunId.Written by Cursor Bugbot for commit f70478d. This will update automatically on new commits. Configure here.