Make update caller receive update failed error on workflow cancellation #653

dandavison · 2024-09-27T09:33:54Z

Fixes #575

Today, if a workflow terminates via cancellation with an in-progress update, the update caller sees a gRPC NOT FOUND error (because the server aborts the update after the workflow is closed).

This PR causes the caller to see WorkflowUpdateFailedError, which is what the caller sees in this situation with all other Temporal SDKs.

It does so by cancelling the asyncio Task representing the update handler execution; the resulting exception in the update handler coroutine causes the update to fail.

How this was tested

Integration test in the PR

dandavison · 2024-09-27T11:58:39Z

temporalio/worker/_workflow_instance.py

-                )
+                self._in_progress_updates[
+                    job.id
+                ].unfinished_policy = defn.unfinished_policy


We are now forced to create the _in_progress_updates entry before this point (when we create the task), but we update the policy from the defn here, since defn is not known until handler coroutine execution time.

dandavison · 2024-09-27T11:59:36Z

temporalio/worker/_workflow_instance.py

+            task,
+            temporalio.workflow.HandlerUnfinishedPolicy.WARN_AND_ABANDON,
+            job.id,
+        )


We are now storing Task in the HandlerExecution dataclass, in order to get hold of unfinished update handler tasks for cancellation.

dandavison · 2024-09-27T12:00:49Z

temporalio/worker/_workflow_instance.py

        task.add_done_callback(done_callback)
+        self._in_progress_signals[id] = HandlerExecution(
+            job.signal_name, task, defn.unfinished_policy
+        )


We don't actually do anything with the signal tasks currently, but we add the Task to the HandlerExecution for consistency with update.

dandavison · 2024-09-27T12:02:00Z

temporalio/worker/_workflow_instance.py

@@ -1845,6 +1856,14 @@ async def _run_top_level_workflow_function(self, coro: Awaitable[None]) -> None:
                err
            ):
                self._add_command().cancel_workflow_execution.SetInParent()
+                self._warn_if_unfinished_handlers()


In order to maintain our rules regarding unfinished handler warnings, we are forced to issue the warnings here in the case of cancellation (for the other workflow termination types, we continue to do it at the end of activate())

cretz · 2024-09-27T12:00:51Z

temporalio/worker/_workflow_instance.py

@@ -1845,6 +1856,14 @@ async def _run_top_level_workflow_function(self, coro: Awaitable[None]) -> None:
                err
            ):
                self._add_command().cancel_workflow_execution.SetInParent()
+                self._warn_if_unfinished_handlers()


I don't think we should warn before sending the cancellation. A user should have the opportunity to receive and react to cancellation without warning.

cretz · 2024-09-27T12:02:47Z

temporalio/worker/_workflow_instance.py

+        # We do also warn in the case of workflow cancellation, but this is done
+        # when handling the workflow cancellation, since we also cancel update
+        # handlers at that time.


IMO it should be at workflow completion time where we warn always, regardless of reason for completion. This includes cancellation. Users should ensure that their handlers complete even during cancellation, but that doesn't need to be checked until the actual workflow completion is being recorded (primary workflow method could be swallowing cancellation, throwing some other error, etc).

cretz · 2024-09-27T12:05:10Z

tests/worker/test_workflow.py

+        )
+
+
+async def test_update_cancellation(client: Client):


May be worth having a test for doing a cleanup step on cancellation (e.g. in a finally). Can do this in the update or the workflow. This is common in Temporal code and I fear cleanup code in an update with issue a warning.

I see, good point. So to restate what you're saying:

If a workflow cancellation request comes in, and a handler is unfinished, the workflow author should be able to allow the workflow to be cancelled, and to have it gracefully terminate the handler, and not to be criticized by any warning.

That is true today: we don't cancel signals or updates on workflow cancellation, but a user could write a workflow that does, and they would escape the warning, because the handler would not be alive at check time.

But my PR breaks it, by attempting to make the check at workflow cancellation time: it would classify a handler as unfinished when in fact it's about to be gracefully terminated.

The reason my PR does that is that it wanted to continue to warn the workflow author when workflow cancellation causes an update cancellation. And the reason it wants to do that is to maintain consistency with signals: when the workflow is canceled, if we're going to warn on unfinished signal handlers (which we do) then we should warn on unfinished update handlers also.

I think that these desires are mutually incompatible. We have to allow a user to clean up their update gracefully without the warning, as you point out. I see two solutions:

to change things so that we no longer issue unfinished handler warnings for either signal or update, in the case of workflow cancellation.

to automatically cancel signals also (with a workflow logic flag)

There is a strong incentive (caller UX) for automatically canceling update handlers on workflow cancellation. We have never automatically cancelled signal handlers in the Python SDK, and doing so would require a flag as it may be backwards incompatible with replay of existing histories. The inconsistency in cancelling update handlers but not signal handlers would only be a problem if there is a potential for observable side effects.

I'll carry on thinking about it but for now I've updated the PR to stop issuing the unfinished warning on workflow cancellation, for both signal and update.

to change things so that we no longer issue unfinished handler warnings for either signal or update, in the case of workflow cancellation.
[...]
I'll carry on thinking about it but for now I've updated the PR to stop issuing the unfinished warning on workflow cancellation, for both signal and update.

I think we should warn on unfinished handlers upon workflow completion regardless of reason for completion and regardless of reason handlers are unfinished. I think the problem with the PR before was it was warning before cancellation is processed and I think the previous behavior of warning after cancellation/completion is processed is best. So I think the line at #653 (comment) should not be removed.

It does mean it's a pain for users doing "cleanup" in update handlers, but IMO that's an education issue. We have to educate them that they should not exit the primary run function until they have done cleanup in handlers. This is similar to process exiting before an async thing has been given a chance to clean up.

You're suggesting that we continue to do the check late, and warn on workflow cancellation.

We can't do that, because in the common case we will then warn on unfinished signals (since these are not automatically cancelled, and hence the handlers will still be alive) but not warn on unfinished updates (since these are automatically cancelled, and hence the handlers will be dead).

(I did try to explain that in the comment above #653 (comment), but it is turning out to be a slightly tricky subject! And my comment isn't very clear).

This is why I've switched the PR to no longer warn on cancellation.

To confirm from our off-PR discussion, we are still warning on unfinished handler regardless of reason (i.e. cancellation)?

Yes, still thinking about the design here. I've put the PR into draft mode for now.

temporalio/worker/_workflow_instance.py

This reverts commit a9600e3.

This reverts commit 87f540e.

dandavison force-pushed the sdk-2453-cancellation branch 2 times, most recently from 1af52e4 to 6b4d5ca Compare September 27, 2024 11:55

dandavison marked this pull request as ready for review September 27, 2024 11:55

dandavison requested a review from a team as a code owner September 27, 2024 11:55

dandavison force-pushed the sdk-2453-cancellation branch from 6b4d5ca to c721ec9 Compare September 27, 2024 11:56

dandavison commented Sep 27, 2024

View reviewed changes

dandavison force-pushed the sdk-2453-cancellation branch from c721ec9 to 87f540e Compare September 27, 2024 12:03

cretz reviewed Sep 27, 2024

View reviewed changes

cretz reviewed Sep 30, 2024

View reviewed changes

temporalio/worker/_workflow_instance.py Show resolved Hide resolved

dandavison force-pushed the sdk-2453-cancellation branch from be8ec97 to 2d13fa6 Compare October 1, 2024 11:14

Add documentation of signal and update handlers

77382c0

dandavison force-pushed the sdk-2453-cancellation branch from 2d13fa6 to 423d2d5 Compare October 1, 2024 12:05

dandavison added 11 commits October 1, 2024 08:05

SDK-2453 Failing test of update and workflow cancellation

722d9ed

Refactor and add asyncio.Task to HandlerExecution dataclass

b5b7292

Cancel update tasks on workflow cancellation

2e38833

Emit unfinished handler warnings separately for cancellation

c45d9ce

Strengthen test assertion (count instead of presence)

a6d41a5

Revert "Strengthen test assertion (count instead of presence)"

da6566e

This reverts commit a9600e3.

Revert "Emit unfinished handler warnings separately for cancellation"

d8a05ab

This reverts commit 87f540e.

Don't emit unfinished warnings on workflow cancellation

43ad2c8

Fix mypy errors

b485802

3.8 compatibility

a8ba197

Document cancellation of update handlers

423d2d5

dandavison marked this pull request as draft October 1, 2024 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make update caller receive update failed error on workflow cancellation #653

Make update caller receive update failed error on workflow cancellation #653

dandavison commented Sep 27, 2024

dandavison Sep 27, 2024

dandavison Sep 27, 2024

dandavison Sep 27, 2024 •

edited

Loading

dandavison Sep 27, 2024

cretz Sep 27, 2024

cretz Sep 27, 2024

cretz Sep 27, 2024

dandavison Sep 27, 2024 •

edited

Loading

cretz Sep 30, 2024

dandavison Sep 30, 2024 •

edited

Loading

cretz Oct 1, 2024

dandavison Oct 1, 2024

Make update caller receive update failed error on workflow cancellation #653

Are you sure you want to change the base?

Make update caller receive update failed error on workflow cancellation #653

Conversation

dandavison commented Sep 27, 2024

How this was tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dandavison Sep 27, 2024 •

edited

Loading

dandavison Sep 27, 2024 •

edited

Loading

dandavison Sep 30, 2024 •

edited

Loading