Add FirstExecutionRunID to start response#10873
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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: 92186b1aa5
ℹ️ 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".
| // FirstExecutionRunId was duplicated from ExecutionInfo onto ExecutionState so the dedup / | ||
| // conflict path can surface it without loading ExecutionInfo. Backfill in memory for records | ||
| // written before that change so the next persist writes it through. | ||
| if mutableState.executionState.FirstExecutionRunId == "" && mutableState.executionInfo.FirstExecutionRunId != "" { |
There was a problem hiding this comment.
We did something similar for StartTime when we moved that from ExecutionInfo to executionState
yycptt
left a comment
yycptt
left a comment
There was a problem hiding this comment.
But if you start a workflow today w/ conflict policy of use existing and it doesn't start, that run ID is not acceptable for first execution run ID.
sorry I am bit confused here. Why is that runID not acceptable?
| // Run ID of the first execution in the chain (set on WorkflowExecutionStarted). For workflows | ||
| // that have not been continued-as-new this equals run_id. Empty on records written before this | ||
| // field was added; consumers must not assume run_id in that case. | ||
| string first_execution_run_id = 8; |
There was a problem hiding this comment.
We should also remove this field from WorkflowExecutionInfo, or we will storing an addition 32 bytes on every write op.
(partial write for ExecutionState itself could be a separate optimization foundations team can take I think)
There was a problem hiding this comment.
We can't remove it at least for one release no? Since we need to support rollback
There was a problem hiding this comment.
yeah we can't in this release, just want to confirm we will do that eventually. Can you help mark the existing field as deprecated and comment on when we can stop writing to it? (guess the read part we need to keep forever...)
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
Can you add functional test coverage for workflow retries, resets, and signals with start?
@yycptt because the run ID of the currently running workflow is not always the run ID of the first workflow in the chain, like if the workflow continued as new |
| // TestStartWorkflowExecution_FirstExecutionRunId_AfterReset verifies that after a workflow is | ||
| // reset, a StartWorkflowExecution against the reset workflow (with FAIL or USE_EXISTING) reports | ||
| // the original run id as FirstExecutionRunId — not the reset's new run id. | ||
| func (s *WorkflowTestSuite) TestStartWorkflowExecution_FirstExecutionRunId_AfterReset() { |
There was a problem hiding this comment.
There's actually a separate case here where we reset to a non-current run ID that can have a different first execution run ID. Worth the coverage IMHO.
oh I was thinking we can change the check in the api handler to see if the two firstExecutionRunID are the same. But yeah as long as the existing field in the executionExecutionInfo will eventually be removed I don't have any concern. |
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 4931879. Configure here.
3 participants
What changed?
Add FirstExecutionRunID to start response
Why?
closes #8537
How did you test it?
Note
Medium Risk
Touches workflow start, dedup, and ID-conflict paths plus a new persisted field and public API contract; behavior is heavily tested but mistakes could misreport chain identity to SDKs.
Overview
Exposes
first_execution_run_idon StartWorkflowExecution and SignalWithStartWorkflowExecution responses (including dedup / use-existing paths) and onWorkflowExecutionAlreadyStartedfailures, so clients can bind later calls to the head of a continue-as-new chain whenstartedis false or the currentrun_idis not the chain root.Persistence & mutable state: Adds
WorkflowExecutionState.first_execution_run_idas the canonical store (the field onExecutionInfois deprecated), sets it from the started event, backfills fromExecutionInfoon load, and threads it throughCurrentWorkflowConditionFailedErrorfor conflict handling. When persisted state lacks the field, history loads mutable state and resolves viaGetFirstRunID.API bump:
go.temporal.io/apiis updated for the public response/error fields. Functional tests cover brand-new starts, dedup, FAIL policy, retry, reset, continue-as-new, and signal-with-start.Reviewed by Cursor Bugbot for commit edfde7c. Bugbot is set up for automated code reviews on this repo. Configure here.