-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Standalone activities to support for ID conflict use existing #8814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…nflict policy fail.
|
cursor review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: UseExisting policy returns incorrect Started=true for existing activity
When ACTIVITY_ID_CONFLICT_POLICY_USE_EXISTING is used and an activity with the same ID already exists, the response incorrectly returns Started: true. The callback that creates the response with Started: true is always executed, even when the engine returns an existing activity via the UseExisting conflict policy. Per the established convention (seen in workflow's similar USE_EXISTING behavior in telemetry tests), Started should be false when returning an existing execution rather than creating a new one. This could mislead callers into thinking a new activity was started when it wasn't.
chasm/lib/activity/handler.go#L68-L84
temporal/chasm/lib/activity/handler.go
Lines 68 to 84 in 76033ba
| }, | |
| func(mutableContext chasm.MutableContext, request *workflowservice.StartActivityExecutionRequest) (*Activity, *workflowservice.StartActivityExecutionResponse, error) { | |
| newActivity, err := NewStandaloneActivity(mutableContext, request) | |
| if err != nil { | |
| return nil, nil, err | |
| } | |
| err = TransitionScheduled.Apply(newActivity, mutableContext, nil) | |
| if err != nil { | |
| return nil, nil, err | |
| } | |
| return newActivity, &workflowservice.StartActivityExecutionResponse{ | |
| Started: true, | |
| // EagerTask: TODO when supported, need to call the same code that would handle the HandleStarted API | |
| }, nil | |
| }, |
That behavior would seem confusing as the activity is in Started state. WHere are you deducing this for workflows? Give me evidence |
tests/standalone_activity_test.go
Outdated
| StartToCloseTimeout: durationpb.New(1 * time.Minute), | ||
| IdConflictPolicy: enumspb.ACTIVITY_ID_CONFLICT_POLICY_USE_EXISTING, | ||
| }) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should check that the returned runID is the same as the first one, and that started is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added runID assertion.
As discussed, added a TODO. We need a way for chasm engine to tell us execution is already created. I'll dig through the code and ping chasm crew
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with that suggestion to change the error and some minor comments.
## What changed? Added support for ID conflict use existing. Added error details on conflict policy fail. ## Why? We now have the use existing policy support on chasm engine, so we can support it. Also need to pass back error details on conflict policy fail in struct ActivityExecutionAlreadyStartedError ## How did you test it? - [X] built - [X] run locally and tested manually - [X] covered by existing tests - [ ] added new unit test(s) - [X] added new functional test(s) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > <sup>[Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) is generating a summary for commit f264bba. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
What changed?
Added support for ID conflict use existing. Added error details on conflict policy fail.
Why?
We now have the use existing policy support on chasm engine, so we can support it. Also need to pass back error details on conflict policy fail in struct ActivityExecutionAlreadyStartedError
How did you test it?
Note
Cursor Bugbot is generating a summary for commit f264bba. Configure here.