Skip to content
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

Validate workflow task start time when complete #4663

Merged
merged 2 commits into from
Jul 21, 2023

Conversation

yiminc
Copy link
Member

@yiminc yiminc commented Jul 21, 2023

What changed?
Add start time to workflow task token and validate it on close.

Why?
To reject concurrent speculative workflow task with same startedEventID.

How did you test it?
Integration tests.

Potential risks
No

Is hotfix candidate?
Yes

@yiminc yiminc requested a review from a team as a code owner July 21, 2023 00:05
Copy link
Member

@yycptt yycptt left a comment

Choose a reason for hiding this comment

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

I don't see correctness issues so I will approve since this is needed for patch release. The odds of nano second conflicting is probably low enough and can be ignored.

@yiminc yiminc force-pushed the validate_start_time branch from f791cd6 to 742f66f Compare July 21, 2023 00:59
@yiminc yiminc enabled auto-merge (squash) July 21, 2023 01:05
@yiminc yiminc merged commit a834ea4 into temporalio:master Jul 21, 2023
yycptt pushed a commit that referenced this pull request Jul 21, 2023
<!-- Describe what has changed in this PR -->
**What changed?**
Add start time to workflow task token and validate it on close.

<!-- Tell your future self why have you made these changes -->
**Why?**
To reject concurrent speculative workflow task with same startedEventID.

<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->
**How did you test it?**
Integration tests.

<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->
**Potential risks**
No

<!-- Is this PR a hotfix candidate or require that a notification be
sent to the broader community? (Yes/No) -->
**Is hotfix candidate?**
Yes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants