Nexus cancellation types #981
Conversation
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dandavison
force-pushed
the
dan-0001-nexus-cancellation-types
branch
from
aac8dcd to
d5039c2
Compare
dandavison
force-pushed
the
dan-0001-nexus-cancellation-types
branch
from
d5039c2 to
134072e
Compare
|
bugbot run |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The test_cancellation_type tests were hanging indefinitely when run with the time-skipping environment, causing CI failures. These tests work fine with the regular environment but have compatibility issues with time-skipping, similar to other Nexus tests. Fixes the CI timeout failures seen on PR #981.
The test_cancellation_type tests were hanging indefinitely when run with the time-skipping environment, causing CI failures. These tests work fine with the regular environment but have compatibility issues with time-skipping, similar to other Nexus tests. Fixes the CI timeout failures seen on PR #981.
dandavison
force-pushed
the
dan-0001-nexus-cancellation-types
branch
from
652f2b7 to
771aa96
Compare
dandavison
force-pushed
the
dan-0001-nexus-cancellation-types
branch
from
aeb1b39 to
e1319f5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dandavison
force-pushed
the
dan-0001-nexus-cancellation-types
branch
2 times, most recently
from
d58f06b to
314b0f3
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dandavison
force-pushed
the
dan-0001-nexus-cancellation-types
branch
from
314b0f3 to
5f1f8c9
Compare
dandavison
force-pushed
the
dan-0001-nexus-cancellation-types
branch
from
5f1f8c9 to
fc9af07
Compare
dandavison
force-pushed
the
dan-0001-nexus-cancellation-types
branch
3 times, most recently
from
0e9143c to
b1090f3
Compare
| # We want the cancel handler to be invoked, so this workflow must not close before | ||
| # then. | ||
| await self.cancel_handler_released.wait() | ||
| # TODO: there is technically a race now between (1) caller server writing |
There was a problem hiding this comment.
We may want to just note somewhere that nobody should use this as a safe example, just in case someone stumbles upon it.
| [ | ||
| (caller_wf, EventType.EVENT_TYPE_NEXUS_OPERATION_CANCEL_REQUESTED), | ||
| (caller_wf, EventType.EVENT_TYPE_NEXUS_OPERATION_CANCEL_REQUEST_FAILED), | ||
| (handler_wf, EventType.EVENT_TYPE_WORKFLOW_EXECUTION_COMPLETED), |
There was a problem hiding this comment.
So if I'm following correctly, even if wait_for_cancellation_completed just works like wait_for_cancellation_requested, this will sometimes succeed, yes?
There was a problem hiding this comment.
Thanks for bringing this up. And also related to your comment above about my comment about the race condition. Basically I hadn't quite pushed hard enough on this corner of the tests. I've now added an additional wait on a future/signal, so that the WAIT_REQUESTED assertion
assert op_cancel_request_failed < caller_op_future_resolved < handler_wf_completednever evaluates equal to the WAIT_COMPLETED assertion
assert handler_wf_completed < caller_op_future_resolvedand removed the race condition TODO.
dandavison
force-pushed
the
dan-0001-nexus-cancellation-types
branch
2 times, most recently
from
c90c07b to
8dad318
Compare
dandavison
force-pushed
the
dan-0001-nexus-cancellation-types
branch
from
8dad318 to
3c6088a
Compare
Implements cancellation types for Python Nexus. This PR is mainly tests. These provide coverage for the state machine changes in temporalio/sdk-core#977. See temporalio/sdk-core#911 (comment)
The gist for anyone who needs a refresher is:
Abandon- caller workflow never even send a request cancel command. The state machine is such that the operation handle future is resolved with a cancellation exception in a second activation in the same WFTTryCancel- same asAbandonregarding the immediate resolution of the future, but do emit the cancel nexus op request command (maybe it could also have been called RequestAsync or something)For the next two, one has to keep two concepts clearly distinct: the Nexus cancel handler invocation, and cancellation of the nexus op itself. The typical case is that the handler issues a
RequestCancelWorkflowExecutionto the workflow that's backing the nexus operation and then when that workflow is eventually cancelled, that triggers cancellation of the nexus op itself (the worfklow reaching a terminal state triggers some nexus stuff server-side)WaitRequested- caller workflow issues the cancel request command, and waits for an event saying that the cancel handler invocation is complete (that event is the new event for which Rust state machine changes were made)WaitCompleted- caller workflow issues the cancel request command and waits for the operation to be cancelled (isn't affected by the new event)And then finally, it's not one new event; there's two because the cancel handler might succeed or fail. Under
WaitRequestedthe fail event also resolves the nexus operation handle future in the caller workflow, but as a normal nexus error, not as cancelled.