Skip to content

Real workflow cancel#218

Merged
manojdbos merged 14 commits intomainfrom
real-cancel
Feb 27, 2025
Merged

Real workflow cancel#218
manojdbos merged 14 commits intomainfrom
real-cancel

Conversation

@manojdbos
Copy link
Copy Markdown
Contributor

No description provided.

dbos/_core.py Outdated
output = wf_handle.get_result()
return output
except DBOSWorkflowCancelledError as error:
if status["queue_name"] is not None:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We discussed this is the TS PR as well.
I think this is needed. From here the error goes to the client.
If we don'nt remove it like the other error handlers do, I think it will get retried.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it needed? Doesn't the main cancel handler already remove it from the queue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What main handler are you referring to ?

My understanding of the code is that , once the error is re thrown here , it goes to the caller. There is no other except clause to catch it.

Copy link
Copy Markdown
Member

@kraftp kraftp Feb 26, 2025

Choose a reason for hiding this comment

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

The actual cancel handler, in https://github.com/dbos-inc/dbos-transact-py/blob/main/dbos/_workflow_commands.py, which calls

def cancel_workflow(
, which already does this

Copy link
Copy Markdown
Member

@kraftp kraftp left a comment

Choose a reason for hiding this comment

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

Looks good!

@manojdbos manojdbos merged commit ce1e445 into main Feb 27, 2025
5 checks passed
@manojdbos manojdbos deleted the real-cancel branch February 27, 2025 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants