CancelTimer activity expects "activity-" prefix

[JoschaMetze]

Hi,

I did notice that the QuartzTimers-example fails within the CancelWorkflow example. If you execute the example the string "Should not be executed" will be written to the console although the timer should have been cancelled. After debugging around I found out, that the CancelTimer activity tries to get the blocking activity by comparing it to it's activity id. Unfortunately the activityId of the blocking activity is "activity-timer-1" although the WithId method while setting up the timer specifies "timer-1" as it's id. So when cancelling with "timer-1" the activity is not found and the cancellation silently fails. Changing the call to CancelTimer("activity-timer-1") fixes the example. Is this by design so should we prepend the activityId with "activity-" when using the CancelTimer activity or is there a bug somewhere?

Regards Joscha

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[sfmskywalker]

Hi @JoschaMetze , I believe this is a bug. From the user's point of view, if you give the timer activity a name of e.g. "timer-1", then one should be able to refer to it by that name.

[jdevillard]

Hello,

It seems to be by design because to avoid conflict when combining activities in CompositeActivities.

https://github.com/elsa-workflows/elsa-core/blob/0183a77d28df39b75ec97ace35e9dc2b3eb0da6e/src/core/Elsa.Core/Builders/CompositeActivityBuilder.cs#L198-L202

We Prefix the Id with activityIdPrefix (by default equal to activity).

Then we build Composite : https://github.com/elsa-workflows/elsa-core/blob/0183a77d28df39b75ec97ace35e9dc2b3eb0da6e/src/core/Elsa.Core/Builders/CompositeActivityBuilder.cs#L212-L214

and in the Composite Build we use the id of the composite activity (in fact activity-activityId if defined) : https://github.com/elsa-workflows/elsa-core/blob/0183a77d28df39b75ec97ace35e9dc2b3eb0da6e/src/core/Elsa.Core/Builders/CompositeActivityBuilder.cs#L240-L241

So if we remove the prefix like this :

 activityBuilder.ActivityId = string.IsNullOrWhiteSpace(activityId) ? $"{activityIdPrefix}-{++index}" : activityId;

• this can cause breaking change for people who refer to activity-*
• this can create conflict when using composite activity where we can have in a workflow multiple time the same activityId.

[Bergam64]

Hello ELSA Team,

I've also faced the same bug in one of my workflows with ELSA 2.8.2. I also luckily discovered the above-mentioned workaround, but it took time.

In fact, I see 2 distinct issues here:

1. The activity id specified in .Timer(...).WithId(...) is not the same as the activity id actually expected by .CancelTimer(...). From an API point or view, this is clearly a bug.
   The developer expects this to work:

   .Timer(Duration.FromSeconds(30))
   .WithId("timer-1")
   
   // ...
   
   .CancelTimer("timer-1")
   

   but he actually needs to do this to get the expected result:

   .Timer(Duration.FromSeconds(30))
   .WithId("timer-1")
   
   // ...
   
   .CancelTimer("activity-timer-1") // <--
   

2. When activity ClearTimer (which is behind extension method .CancelTimer(...)) can't find the expected timer by id, it silently fails (no exception, not even a log), which is nothing but a booby-trap: neither the workflow nor the developer has a way to know that activity CancelTimer has failed and that the timer will still fire at some point! Very nasty!
   Clearly, not finding the expected timer should throw a meaningful exception.

So, please fix both issues.

Thanks in advance.