search

sartography / SpiffWorkflow

A powerful workflow engine implemented in pure Python
GNU Lesser General Public License v3.0
1.69k stars 313 forks source link

Sub-workflow tasks should be marked as "Future" when resetting to a task before the sub-process. #327

Closed danfunk closed 1 year ago

danfunk commented 1 year ago

Found that tasks within a sub-workflow were left in a state of "READY… after resetting to task before the sub-workflow.

burnettk commented 1 year ago

if you are resetting to, say, task2, another option that might resolve this is dropping all children (and subchildren) of task2 and then re-predicting, which would create any tasks that are necessary.

essweine commented 1 year ago

We should simply drop the children, delete, the subprocess, and call predict.

Reusing the tasks is messy. There's no guarantee the task tree will be the same, so trying to continually keep it in sync with the state of the workflow is going to cause problems, and we'll end up having to reimplement predict in a second set of methods.

A month or six weeks from now, some random problem will happen and we'll add a special case, and then another month will go by the same thing will happen and we'll add another because we found another "bug", when the real bug is trying to reuse the tasks.

danfunk commented 1 year ago

@essweine perhaps you could take a crack at doing this?

essweine commented 1 year ago

I changed the reset method to drop the existing tasks and create new ones. The method now returns the list of tasks that were deleted.

I also changed the name of the method toreset_from_task_id, which I think makes more sense, as we're not resetting the task, we're resetting the workflow. Though we're actually not resetting the workflow, just the branch (that's all we were doing before though, so not a behavior change). However, this does mean that tasks completed after this one that were not descendants will be unaffected; IMO, this behavior makes sense, but wanted to throw this out there for discussion. Not entirely sure how this affects the backend.

I also updated the catch method on catching events to update the last updated time (catching an event could change the internal data without changing the task state, so without this, you might not detect a change in the caught event).

danfunk commented 1 year ago

Thank you @essweine. This certainly clears out some crufty code - I'm looking forward to testing it out tomorrow and seeing if it resolves the issues we are encountering in the admin tools.