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

Remove cancelation commands when underlying futures are closed #275

Merged
merged 3 commits into from
Jan 4, 2024

Conversation

jeffschoner
Copy link
Contributor

Summary

This change filters out activity and timer cancellation commands before sending back the result of the workflow task to Temporal server. It does this when those commands are associated with activities or timers that have already been canceled.

Motivation

Existing code in future.rb already ensures that cancellation is a no-op on activity and timer futures that have already completed at that point in workflow execution. However, this doesn't cover all cases where an activity or timer is being canceled in the same workflow task where it is completing or failing. Consider the following scenario:

In an earlier workflow task:

  • An activity is scheduled by starting an activity in workflow code

In a later workflow task:

  • A signal is received, which has a handler that cancels the activity. This will produce a RequestCancelActivityTaskCommand which is put in a list to be sent back to the Temporal server.
  • The activity completes or fails

Because the future will still not be complete when the signal is received, it will be canceled and the command produced. However, later in the processing of the history window, we encounter a history event indicating that cancellation is not valid because the activity has already finished. When the RequestCancelActivityTaskCommand is sent to Temporal server, it will reject it as invalid and retry the workflow task. This will continue indefinitely, putting the workflow in a "stuck" state.

Testing

  • Adds a new integration spec that reproduces this race condition
  • Adds some new unit specs for the state manager that precisely tests this filtering behavior

@DeRauk DeRauk merged commit cfcbdd3 into coinbase:master Jan 4, 2024
2 checks passed
@DeRauk
Copy link
Contributor

DeRauk commented Jan 4, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants