Inconsistent behavior

[ssougnez]

Hello,

I'm currently experiencing an inconsistent behavior with wfc and I'm starting to run out of idea. I keep telling myself I did something wrong but I can figure out what... First of all, I'm using

• WorkflowCore 1.9.0
• WorkflowCore.Persistence.SqlServer 1.8.1

Here is the definition of my workflow:

builder
    .StartWith<NopStep>()
    .Parallel()
        .Do(then => then
            .StartWith<NopStep>()
            .Then<CreateTaskForUserStep>()
                .Input((step, data) =>
                {
                    step.Concerned = data.Concerned.Value;
                    step.Type = CreateTaskForUserStep.UserType.Manager;
                })
                .Output((step, data) => data.TaskOne = step.Output)
            .WaitFor(Constants.WorkflowEvent.WaitForApproval, (data, context) => data.TaskOne.Id.ToString())
                .Output((step, data) => data.TaskOne = step.EventData as TaskInfo)
        )
        .Do(then => then
            .StartWith<NopStep>()
            .Then<CreateTaskForUserStep>()
                .Input((step, data) =>
                {
                    step.Concerned = data.Concerned.Value;
                    step.Type = CreateTaskForUserStep.UserType.Manager;
                    step.Level = 2;
                })
                .Output(data => data.TaskTwo, step => step.Output)
            .WaitFor(Constants.WorkflowEvent.WaitForApproval, (data, context) => data.TaskTwo.Id.ToString())
                .Output((step, data) => data.TaskTwo = step.EventData as TaskInfo)
        )
    .Join()
        .CancelCondition(data => data.TaskOne.IsRejected || data.TaskTwo.IsRejected, true)
    .Then(_ =>
    {
        return ExecutionResult.Next();
    });

This workflow is not complete because of the issue I'm experiencing. It is pretty simple. When it is started by someone, two parallel tasks are started and each of them call the step "CreateTaskForUserStep" to create a task in the database. The step is registered as transient and use a regular "DbContext" to create the task. Once the task are created, I'm just waiting for them to get approved. If one of the two task gets rejected, the workflow must continue and cancel the other parallel branch (which is why I'm using the "CancelCondition").

Like this, this workflow works correctly. If I set a breakpoint at the last line of code, it gets hit correctly. However, if I modify the two parallel branches by adding:

.Then<NopStep>()

Just before the "WaitFor", then the breakpoint I set at the last line gets hit twice... Three remarks:

1. The NopStep is a simple step that returns ExecutionResult.Next()
2. The behavior is deterministic. The way I'm testing it is that I trigger the workflow, the two tasks get created, I accept the first one but reject the second one. If I accept both tasks or reject the first one, the breakpoint gets it only once.
3. All my step (even the "NopStep") is registered as transient. I made an extension method that analyze my assembly to register all the classes that inherit from "StepBody" and "StepBodyAsync" as transient. This way, I can easily inject dependency without having to manually register the step as transient.

Something even weirder is that if I add another

.Then<NopStep>()

Just after the previous one, then the breakpoint gets hits only once... Unfortunately, I can't upload the project somewhere as it is a pretty big project interconnected with other systems.

The more I think about it, the more I think there is an issue with "CancelCondition". Indeed, I tried to follow the same steps (so accept the first task and reject the other one) with the "CancelCondition" line commented and in this case, the breakpoint only gets hit once. It looks like when both parallel branches terminate by themselves and that the "CancelCondition" evaluates to true, the rest of the workflow is executed twice (once by the "CancelCondition" and once by the normal flow of the workflow). To validate this supposition, I replaced the "CancelCondition" line by this:

.CancelCondition(data => (data.TaskOne.IsRejected && data.TaskTwo.State == TaskApprovalState.WaitingForApproval) || (data.TaskTwo.IsRejected && data.TaskOne.State == TaskApprovalState.WaitingForApproval), true)

This way, if the two parallel branches terminate by themselves, the condition of the "CancelCondition" does not evaluate to "true". With this, the breakpoint only gets hit once, which tend to validate my theory. The only thing I don't understand is the fact that the number of .Then<NopStep>() leverage the workflow behavior.

I can use the aforementioned workaround for now, but I fear that there is an underlying issue that could arise in other situations that this one (when using "CancelCondition" I guess). Don't hesitate if you have any question, I really think that library is awesome and I hope that you'll find a solution to this issue ;-)

[danielgerlag]

Are you able to create a failing integration test that demonstrates the issue?
Also, I would try putting the cancel condition on each WaitFor with continue set to false, rather than on the Join? Does that work? (If there is an ussie, I suspect it might be related to canceling a "container" of steps rather than a single step)

[ssougnez]

I tried what you just mentioned and I used CancelCondition on the two WaitFor and that works as expected so your theory that the problem (if there is one) could be related to the container might be true.

Unfortunately, I couldn't reproduce this behavior in a simple solution. Maybe the fact I'm using a StepBodyAsync to create the task might have an impact. I'll check if I can build wfc from the source and attach the dotnet process to it to maybe see what happens in my particular case. If you want me to test anything else, don't hesitate ;-)

[ssougnez]

TL;DR; See the next comment ;-)

So, I've been digging a bit and I'm kind of lost so far. I'll edit this post to give new information I find out.

I'm currently checking at the "CancellationProcessor.ProcessCancellations" behavior and here is what's happening. When I configure the workflow to run correctly (so adding a NopStep in each branch), the method seems to behave normally:

• Finds a step with a cancel condition that evaluates to true (the one applied on the Join)
• Finds an EP to cancel that corresponds to step 1 ("Sequence", so the parallel I guess)
• Enters the condition to proceed on cancel
• The foreach does not find any pending EP (which makes sense I guess as both parallel branch completed)
• That's it...

However, when I configure the workflow without the NopStep, here is what happens in this method:

• Finds a step with a cancel condition that evaluates to true (the one applied on the Join)
• Finds an EP to cancel that corresponds to step 1 ("Sequence", so the parallel I guess)
• Enters the condition to proceed on cancel
• The foreach does not find any pending EP
• That's not it... Indeed, for I don't know what reason, but:
• Finds a step with a cancel condition that evaluates to true (the one applied on the Join) <= correct
• Finds an EP to cancel that corresponds to step 1 ("Sequence", so the parallel I guess) <= incorrect, it is supposed to be cancelled so the condition on line 38 should not evaluate to true
• Enters the condition to proceed on cancel <= Same than before
• After this methods, the foreach finds 2 pending EP <= Different than before when no EP was found. Moreover, I tried to find these EP in the database while debugging and there were nowhere to be found in the database. Maybe the changes were not yet committed...
• Both EP get cancelled by the foreach <= Did not happen before as no EP was found
• That's it...

So it seems that, for whatever reason, the method processing cancellations finds pending execution pointers even after it ran once for that workflow. So far, it seems to be the issue but I still have not the slightest idea why adding/removing a step in the branches leverage this behavior.

EDIT 1

Mmmmmh, the behavior seemed so strange to me that I decided to check if the issue also occurs when using

services.AddWorkflow();

Instead of

services.AddWorkflow(options => options.UseSqlServer(_configuration.GetConnectionString("WorkflowCore"), true, true));

And... It does not, so maybe the issue is not in wfc but in the WorkflowCore.Persistence.SqlServer, which would explain why I couldn't reproduce it in a sample application as I was using the memory provider. I'll try to edit a sample with a db connection instead.

EDIT 2

Ok, I could reproduce it with a sample application. To do so, open "Sample04" and:

• Use services.AddWorkflow(x => x.UseSqlServer(@"Server=.\SQLEXPRESS;Database=WorkflowsSample;Trusted_Connection=True;", true, true));
• Add a property called Value2 to MyDataClass
• Update the workflow definition like so:

builder
    .StartWith(context => ExecutionResult.Next())
    .Parallel()
        .Do(then => then
            .StartWith(_ => ExecutionResult.Next())
            .Then(_ => ExecutionResult.Next())
            .Then(_ => ExecutionResult.Next())
            .WaitFor("MyEvent1", (data, context) => context.Workflow.Id)
                .Output(data => data.Value1, step => step.EventData))
        .Do(then => then
            .StartWith(_ => ExecutionResult.Next())
            .Then(_ =>  ExecutionResult.Next())
            .Then(_ =>  ExecutionResult.Next())
            .WaitFor("MyEvent2", (data, context) => context.Workflow.Id)
                .Output(data => data.Value2, step => step.EventData))
    .Join()
        .CancelCondition(d => d.Value1 == "stop" || d.Value2 == "stop", true)
    .Then<CustomMessage>()
        .Input(step => step.Message, data => "The data from the event is " + data.Value1 + " - " + data.Value2)
    .Then(context => Console.WriteLine("workflow complete"));

• Start the console
• Enter something for the first value
• Enter "stop" for the second value

The output will be:

As you can see, the CustomMessage step got executed twice.

EDIT 3

So far, I could fix the issue by changing the line https://github.com/danielgerlag/workflow-core/blob/master/src/WorkflowCore/Services/WorkflowExecutor.cs#L44 by

var exePointers = new List<ExecutionPointer>(workflow.ExecutionPointers.Where(x => x.Active && (!x.SleepUntil.HasValue || x.SleepUntil < _datetimeProvider.Now.ToUniversalTime()))).OrderBy(s => s.StepId);

To ensure order in the step. I don't know yet why but depending on the number of step in the branches, the step with id 1 (the sequence) appears before the WaitFor step (which is correct) or the other way around (which is incorrect). I'll continue investigating to understand why the order is different.

EDIT 4

So, that's the issue. Workflow Core is doing everything fine but when this line is called: https://github.com/danielgerlag/workflow-core/blob/master/src/providers/WorkflowCore.Persistence.EntityFramework/Services/EntityFrameworkPersistenceProvider.cs#L132

It seems that Entity Framwork does not respect the order of the execution pointers. I'll check if it's a known issue at EFCore side.

[ssougnez]

The issue is actually due to the fact that EFCore does not ensure that the order you add entities to the DbSet is preserved when saving the data into the database. That's exactly what happens here... When this method is called: https://github.com/danielgerlag/workflow-core/blob/master/src/providers/WorkflowCore.Persistence.EntityFramework/ExtensionMethods.cs#L15

The execution pointers are re-inserted in the database in a bad order, meaning that when they are treated by https://github.com/danielgerlag/workflow-core/blob/master/src/WorkflowCore/Services/WorkflowExecutor.cs#L45, they are in a incorrect order causing inconsistent behaviors.

I have two solution in mind that I'll propose below but as you're the owner of the project and have a complete understanding of it, you'll probably take the best solution (probably one I didn't think about ^^).

Note that, as you know very well, the EFCore provider being used by all SQL-related persistence provider, they are all probably affected by the issue.

The "good"

Adding a field "Order" in the "ExecutionPointer" table would allow you to execute an OrderBy to ensure that the execution pointers order is always the correct one.

Spoiler alert. I tried this solution by adding a migration to the EFCore provider and blah blah... And it works as expected. At the end of the day, as you clear the execution pointer every time you persist them, the order field will always have the correct value. If you're OK with this solution, I'd be glad to submit a PR :-D

The really really bad (more like evil)

You could call db.SaveChangeAsync() after every addition of an execution pointer in the workflow but I can't think about something worse for performances ^^

[danielgerlag]

I don't think that it should matter the order that the persistence provider returns the steps in.... the core engine should be robust enough. That being said... the order shouldn't matter either, as it should execute all active steps. I think the issue may be a bit deeper than this,

[danielgerlag]

How would you determine the correct order and can you distill why it matters?

[ssougnez]

I'll finish what I'm currently working on then I'll dig deeper to understand why order matters ;-)

[ssougnez]

I checked a bit and I think the order is important because of an underlying problem. Consider the edited sample that I described in here: #273 (comment).

Say that you enter "ok", then "stop". When you validate "stop", the WorkflowExecutor will fetch all the EP that needs to be executed. In the bad scenario, the first one would concern the step 9 (the WaitFor) then the second one would concern the step 1 (the Parallel).

Now, the WorkflowExecutor executes the handling of the event, that's normal. Then, before executing the Parallel, it executes the CancellationProcessor. This one fetches all the step that have a CancelCondition that are not completed or cancelled, checks if the condition evaluates to true and if that's the case, it cancels the EP and its children. In our case, it would fetch the EP corresponding to the step 1 and would cancel it, as expected. However, the CancelCondition is set to continue on cancel, so the ExecutionResultProcessor is kicked off and continues the execution. Basically, it will add an EP corresponding the the CustomMessage step as it's the one after the Join. However, the issue is that it will also set the EP status to Complete overriding the previous status (Canceled).

This causes an issue as the WorkflowExecutor will now handles the next EP, which corresponds to the step 1. This executor only handles the EP if its status is not Cancelled, however, the status of the EP has been wrongly changed to Complete via the CancellationProcessor => ExecutionResultProcessor. This means that the EP corresponding to the step 1 will execute normally and register another EP for CustomMessage.

At the end of the day, we end up with two new EP leading to CustomMessage.

This also explain why the issue does not occur when the order is correct. In that case, the step 1 is handled first and cancels all its children, so the step 9 does not get executed.

I could solve the issue in two different way. First one is by modifying this line: https://github.com/danielgerlag/workflow-core/blob/master/src/WorkflowCore/Services/WorkflowExecutor.cs#L54 in

if (pointer.Status == PointerStatus.Cancelled || pointer.Status == PointerStatus.Complete)

It seems logic not to execute an EP if it's already completed but there might be some cases where it's wanted.

Another, and maybe better, solution would be to move line 42-45 of this file: https://github.com/danielgerlag/workflow-core/blob/master/src/WorkflowCore/Services/CancellationProcessor.cs after the foreach. This way, the EP for CustomMessage is correctly added, the EP gets the Complete status and then its status gets updates to Cancelled, preventing the WorkflowExecutor to execute it.

Now that you have all these information, maybe you have a better solution in mind? I'd be glad to continue investigating this issue but as I don't know the solution well enough, it might take a long time and I think that's a pretty "important" issue, so it might get fixed faster if you had a look ;-)

[danielgerlag]

I really appreciate this level of engagement :)

I will try spend some time drilling into this on the weekend

[danielgerlag]

Do you think this change makes sense? #281
Also, can you think of how we could best write a test for this scenario?

[ssougnez]

The change makes sense to me. The condition in the foreach loop adds an additional security against step that should not be executed.

For the test, I'll have to think about it because it's clearly ont my specialty hehe. Off th top of my mind, I'd say that the issue occurred when an action "unlocks" two different path of the workflow. In that case, depending on the order of the EPs, the two different path could be executed when the CancellationProcessor does not behave as it is supposed to do.

[ssougnez]

Hi,

Would it be possible for you to create a release with this bug fix? I'm gonna need a bug free version of the CancelCondition pretty soon in my project so it would be nice to have it :-)

As for the tests, I still didn't have time to think about it and I feel that I won't find a reliable way very fast (mainly because I suck at testing haha)

[danielgerlag]

I published a release this last weekend, sorry I have not sent the release notes yet.

[ssougnez]

Cool, thanks.

Just a remark about that. I noticed that the release for the night package don't always match the ones on GitHub. It would be nice to align them.

Sometimes, I see there is an update and when I go on GitHub to see what changed, I don't see the new release. It's just a detail but it's always interesting to know what changed in new releases ;-)

[danielgerlag]

You are 100% right, sorry I dropped the ball. 😀