Fix workflow variable scope inconsistency (#5558)

* Refactor variable handling in ExpressionExecutionContextExtensions

The core change in this commit is the refactoring of the handling of variables within the ExpressionExecutionContextExtensions. The methods GetVariable, CreateVariable, and GetVariableBlock have been altered for clarity and simplified reducing redundant code. Unnecessary parameters and returns in method documentation have been removed, and overall code formatting has been improved to enhance readability.

* Simplify MemoryRegister creation in Workflow.cs

The creation of the MemoryRegister object in the file Workflow.cs was simplified to one line. The previous method, which declared a new object then called the Declare method before returning, was removed.

* Map controller routes in server web program

Added a line of code in the Elsa.Server.Web program.cs file to map controller routes. This change ensures that HTTP requests are correctly directed to their corresponding controller actions.

* Update workflow state extraction logic

The logic in the WorkflowStateExtractor has been updated to retain the root Workflow activity context even if it's completed. This change is necessary to keep workflow-level variables accessible.

* Remove RequiresUnreferencedCode attribute from ConvertTo method

The RequiresUnreferencedCode attribute was removed from the ConvertTo method in the ObjectConverter class.

* Remove unused services and rename test file

Unused services in the AutoUpdateTests.cs class were removed, reducing clutter and improving code readability. Additionally, the DeleteWorkflow_Clustered.cs test file has been renamed to DeleteWorkflowClustered.cs for better naming consistency.

* Add CountdownStep activity and CountdownWorkflow for testing

This commit introduces new component tests for simulations involving counters. It includes a new CountdownStep activity that decrements a counter variable, as well as a CountdownWorkflow which consists of a loop based on the aforementioned activity. It also involves a CountdownWorkflowTests class for testing counter persistence across workflow runs.

* Remove unnecessary whitespace in CountdownWorkflowTests

This commit eliminates the superfluous whitespace in the CountdownWorkflowTests.cs file. It maintains the proper formatting and ensures code consistency across the test component.

* Refactor CountdownWorkflowTests constructor

Simplified the constructor of the CountdownWorkflowTests class. The changes remove the unnecessary constructor body and pass the 'app' object directly to the base AppComponentTest class, enhancing the code's readability and maintainability.

* Add application roles and configure them in MassTransit

A new enum ApplicationRole has been added for distinguishing among different roles (Hybrid, Api, Worker) an application can take. In the configuration of MassTransit, it is now possible to disable the consumers based on application role, which can help optimize the usage of resources and increase application efficiency.

* Update Program.cs

Switch to Memory broker

* Update Program.cs

Simplify DisableConsumers assignment.

* Update ApplicationRole.cs

Rename Hybrid to Default.

* Update appsettings.json

src/bundles/Elsa.Server.Web/Enums/ApplicationRole.cs

@@ -2,7 +2,7 @@ namespace Elsa.Server.Web;
 
 public enum ApplicationRole
 {
-    Hybrid,
+    Default,
     Api,
     Worker
-}
+}

src/bundles/Elsa.Server.Web/Program.cs

@@ -326,8 +326,7 @@
         {
             elsa.UseMassTransit(massTransit =>
             {
-                if (appRole == ApplicationRole.Api)
-                    massTransit.DisableConsumers = true;
+                massTransit.DisableConsumers = appRole == ApplicationRole.Api;
 
                 if (useMassTransitBroker == MassTransitBroker.AzureServiceBus)
                 {
@@ -410,6 +409,8 @@
 // Elsa HTTP Endpoint activities.
 app.UseWorkflows();
 
+app.MapControllers();
+
 // Swagger API documentation.
 if (app.Environment.IsDevelopment())
 {
@@ -432,4 +433,4 @@ public partial class Program
     /// Set by the test runner to configure the module for testing.
     /// </summary>
     public static Action<IModule>? ConfigureForTest { get; set; }
-}
+}

src/bundles/Elsa.Server.Web/appsettings.json

@@ -96,7 +96,7 @@
       }
     ]
   },
-  "AppRole": "Hybrid",
+  "AppRole": "Default",
   "Runtime": {
     "WorkflowInboxCleanup": {
       "SweepInterval": "00:00:10:00",
@@ -126,4 +126,4 @@
       ]
     }
   }
-}
+}

src/modules/Elsa.Expressions/Helpers/ObjectConverter.cs

@@ -51,7 +51,6 @@ public static Result TryConvertTo(this object? value, Type targetType, ObjectCon
     /// <summary>
     /// Attempts to convert the source value into the destination type.
     /// </summary>
-    [RequiresUnreferencedCode("The JsonSerializer type is not trim-compatible.")]
     public static T? ConvertTo<T>(this object? value, ObjectConverterOptions? converterOptions = null) => value != null ? (T?)value.ConvertTo(typeof(T), converterOptions) : default;
 
     private static JsonSerializerOptions? _defaultSerializerOptions;

src/modules/Elsa.Workflows.Core/Activities/Workflow.cs

@@ -117,9 +117,7 @@ public Workflow()
     /// </summary>
     public MemoryRegister CreateRegister()
     {
-        var register = new MemoryRegister();
-        register.Declare(Variables);
-        return register;
+        return new MemoryRegister();
     }
 
     /// <inheritdoc />

src/modules/Elsa.Workflows.Core/Extensions/ExpressionExecutionContextExtensions.cs

@@ -66,8 +66,7 @@ public static IDictionary<object, object> CreateTriggerIndexingPropertiesFrom(Wo
     /// <summary>
     /// Returns the <see cref="Workflow"/> of the specified <see cref="ExpressionExecutionContext"/>
     /// </summary>
-    public static bool TryGetWorkflowExecutionContext(this ExpressionExecutionContext context, out WorkflowExecutionContext workflowExecutionContext) =>
-        context.TransientProperties.TryGetValue(WorkflowExecutionContextKey, out workflowExecutionContext!);
+    public static bool TryGetWorkflowExecutionContext(this ExpressionExecutionContext context, out WorkflowExecutionContext workflowExecutionContext) => context.TransientProperties.TryGetValue(WorkflowExecutionContextKey, out workflowExecutionContext!);
 
     /// <summary>
     /// Returns the <see cref="WorkflowExecutionContext"/> of the specified <see cref="ExpressionExecutionContext"/>
@@ -77,16 +76,11 @@ public static bool TryGetWorkflowExecutionContext(this ExpressionExecutionContex
     /// <summary>
     /// Returns the <see cref="ActivityExecutionContext"/> of the specified <see cref="ExpressionExecutionContext"/>
     /// </summary>
-    /// <param name="context"></param>
-    /// <returns></returns>
     public static ActivityExecutionContext GetActivityExecutionContext(this ExpressionExecutionContext context) => (ActivityExecutionContext)context.TransientProperties[ActivityExecutionContextKey];
 
     /// <summary>
     /// Returns the <see cref="ActivityExecutionContext"/> of the specified <see cref="ExpressionExecutionContext"/> 
     /// </summary>
-    /// <param name="context"></param>
-    /// <param name="activityExecutionContext"></param>
-    /// <returns></returns>
     public static bool TryGetActivityExecutionContext(this ExpressionExecutionContext context, out ActivityExecutionContext activityExecutionContext) => context.TransientProperties.TryGetValue(ActivityExecutionContextKey, out activityExecutionContext!);
 
     /// <summary>
@@ -104,31 +98,41 @@ public static bool TryGetWorkflowExecutionContext(this ExpressionExecutionContex
     /// </summary>
     public static object? Get(this ExpressionExecutionContext context, Output output) => context.GetBlock(output.MemoryBlockReference).Value;
 
-
     /// <summary>
     /// Returns the value of the variable with the specified name.
     /// </summary>
-    public static T? GetVariable<T>(this ExpressionExecutionContext context, string name) => (T?)context.GetVariable(name)?.Value;
+    public static T? GetVariable<T>(this ExpressionExecutionContext context, string name)
+    {
+        var block = context.GetVariableBlock(name);
+        return (T?)block?.Value;
+    }
 
     /// <summary>
     /// Returns the variable with the specified name.
     /// </summary>
     public static Variable? GetVariable(this ExpressionExecutionContext context, string name, bool localScopeOnly = false)
+    {
+        var block = context.GetVariableBlock(name, localScopeOnly);
+        return block?.Metadata is VariableBlockMetadata metadata ? metadata.Variable : default;
+    }
+
+    private static MemoryBlock? GetVariableBlock(this ExpressionExecutionContext context, string name, bool localScopeOnly = false)
     {
         foreach (var block in context.Memory.Blocks.Where(b => b.Value.Metadata is VariableBlockMetadata))
         {
             var metadata = block.Value.Metadata as VariableBlockMetadata;
             if (metadata!.Variable.Name == name)
-                return metadata.Variable;
+                return block.Value;
         }
 
-        return localScopeOnly ? null : context.ParentContext?.GetVariable(name);
+        return localScopeOnly ? null : context.ParentContext?.GetVariableBlock(name);
     }
 
     /// <summary>
     /// Creates a named variable in the context.
     /// </summary>
-    public static Variable CreateVariable<T>(this ExpressionExecutionContext context, string name, T? value, Type? storageDriverType = null, Action<MemoryBlock>? configure = default)
+    public static Variable CreateVariable<T>(this ExpressionExecutionContext context, string name, T? value, Type? storageDriverType = null,
+        Action<MemoryBlock>? configure = default)
     {
         var existingVariable = context.GetVariable(name, localScopeOnly: true);
 
@@ -174,7 +178,6 @@ public static Variable SetVariable<T>(this ExpressionExecutionContext context, s
         var contextWithVariable = context.FindContextContainingBlock(variable.Id) ?? context;
 
         // Set the value on the variable.
-        variable.Value = value;
         variable.Set(contextWithVariable, value, configure);
 
         // Return the variable.
@@ -354,14 +357,14 @@ public static IEnumerable<Variable> EnumerateVariablesInScope(this ExpressionExe
     {
         return context.GetInput<T>(inputDefinition.Name);
     }
-    
+
     private static JsonSerializerOptions? _serializerOptions;
 
     private static JsonSerializerOptions GetSerializerOptions(ExpressionExecutionContext context)
     {
-        if(_serializerOptions != null)
+        if (_serializerOptions != null)
             return _serializerOptions;
-        
+
         var serializerOptions = context.GetRequiredService<IJsonSerializer>().GetOptions().Clone();
         serializerOptions.ReferenceHandler = ReferenceHandler.Preserve;
         _serializerOptions = serializerOptions;

src/modules/Elsa.Workflows.Core/Services/WorkflowStateExtractor.cs

@@ -260,9 +260,9 @@ private void ExtractScheduledActivities(WorkflowState state, WorkflowExecutionCo
 
     private static IEnumerable<ActivityExecutionContext> GetActiveActivityExecutionContexts(IEnumerable<ActivityExecutionContext> activityExecutionContexts)
     {
-        // Filter out completed activity execution contexts.
+        // Filter out completed activity execution contexts, except for the root Workflow activity context, which stores workflow-level variables.
         // This will currently break scripts accessing activity output directly, but there's a workaround for that via variable capturing.
         // We may ultimately restore direct output access, but in a different way.
-        return activityExecutionContexts.Where(x => !x.IsCompleted).ToList();
+        return activityExecutionContexts.Where(x => !x.IsCompleted || x.ParentActivityExecutionContext == null).ToList();
     }
 }

test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/Activities/CountdownStep.cs

@@ -0,0 +1,13 @@
+using Elsa.Extensions;
+
+namespace Elsa.Workflows.ComponentTests.Scenarios.Variables.Activities;
+
+public class CountdownStep : Activity
+{
+    protected override void Execute(ActivityExecutionContext context)
+    {
+        var counter = context.GetVariable<int>("Counter");
+        context.SetVariable("Counter", counter - 1);
+        context?.CreateBookmark();
+    }
+}

test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/CountdownWorkflowTests.cs

@@ -0,0 +1,53 @@
+using Elsa.Expressions.Helpers;
+using Elsa.Extensions;
+using Elsa.Workflows.ComponentTests.Scenarios.Variables.Workflows;
+using Elsa.Workflows.Management.Contracts;
+using Elsa.Workflows.Models;
+using Elsa.Workflows.Runtime.Contracts;
+using Elsa.Workflows.Runtime.Parameters;
+using Elsa.Workflows.Services;
+using Elsa.Workflows.State;
+using Microsoft.Extensions.DependencyInjection;
+
+namespace Elsa.Workflows.ComponentTests.Scenarios.Variables;
+
+public class CountdownWorkflowTests(App app) : AppComponentTest(app)
+{
+    [Fact(DisplayName = "Variable is persisted across workflow runs")]
+    public async Task VariableIsPersistedAcrossWorkflowRuns()
+    {
+        var workflowRuntime = Scope.ServiceProvider.GetRequiredService<IWorkflowRuntime>();
+        var workflowInstanceStore = Scope.ServiceProvider.GetRequiredService<IWorkflowInstanceStore>();
+        var startParams = new StartWorkflowRuntimeParams();
+        var result = await workflowRuntime.StartWorkflowAsync(CountdownWorkflow.DefinitionId, startParams);
+        var workflowInstanceId = result.WorkflowInstanceId;
+        var bookmarks = new Stack<Bookmark>(result.Bookmarks);
+        var expectedCounter = 3;
+
+        while (bookmarks.Any())
+        {
+            var workflowInstance = await workflowInstanceStore.FindAsync(workflowInstanceId);
+            var workflowState = workflowInstance!.WorkflowState;
+            var rootWorkflowActivityExecutionContext = workflowState.ActivityExecutionContexts.Single(x => x.ParentContextId == null);
+            var variables = GetVariablesDictionary(rootWorkflowActivityExecutionContext);
+            var actualCounter = variables["Workflow1:variable-1"].ConvertTo<int>();
+            Assert.Equal(--expectedCounter, actualCounter);
+
+            var bookmark = bookmarks.Pop();
+            var resumeWorkflowRuntimeOptions = new ResumeWorkflowRuntimeParams
+            {
+                BookmarkId = bookmark?.Id,
+            };
+
+            result = await workflowRuntime.ResumeWorkflowAsync(workflowInstanceId, resumeWorkflowRuntimeOptions);
+
+            if (result == null)
+                break;
+
+            foreach (var newBookmark in result.Bookmarks) bookmarks.Push(newBookmark);
+        }
+    }
+
+    private IDictionary<string, object> GetVariablesDictionary(ActivityExecutionContextState context) =>
+        context.Properties.GetOrAdd(WorkflowStorageDriver.VariablesDictionaryStateKey, () => new Dictionary<string, object>());
+}

test/component/Elsa.Workflows.ComponentTests/Scenarios/Variables/Workflows/CountdownWorkflow.cs

@@ -0,0 +1,35 @@
+using Elsa.Extensions;
+using Elsa.Workflows.Activities;
+using Elsa.Workflows.ComponentTests.Scenarios.Variables.Activities;
+using Elsa.Workflows.Contracts;
+
+namespace Elsa.Workflows.ComponentTests.Scenarios.Variables.Workflows;
+
+public class CountdownWorkflow : WorkflowBase
+{
+    public static readonly string DefinitionId = Guid.NewGuid().ToString();
+
+    protected override void Build(IWorkflowBuilder builder)
+    {
+        builder.WithDefinitionId(DefinitionId);
+        var counter = builder.WithVariable("Counter", 3).WithWorkflowStorage();
+
+        builder.Root = new Sequence
+        {
+            Activities =
+            {
+                new While(context => counter.Get(context) > 0)
+                {
+                    Body = new Sequence
+                    {
+                        Activities =
+                        {
+                            new WriteLine(context => $"Counter: {counter.Get(context)}"),
+                            new CountdownStep()
+                        }
+                    }
+                }
+            }
+        };
+    }
+}

test/component/Elsa.Workflows.ComponentTests/Scenarios/WorkflowActivities/AutoUpdateTests.cs

@@ -1,6 +1,3 @@
-using Elsa.Common.Models;
-using Elsa.Http;
-using Elsa.Http.Bookmarks;
 using Elsa.Http.Contracts;
 using Elsa.Workflows.Contracts;
 using Elsa.Workflows.Management.Contracts;
@@ -18,9 +15,6 @@ public class AutoUpdateTests : AppComponentTest
     private readonly IWorkflowDefinitionPublisher _publisher;
     private readonly ISignalManager _signalManager;
     private readonly ITriggerChangeTokenSignalEvents _changeTokenEvents;
-    private readonly IWorkflowDefinitionManager _definitionManager;
-    private readonly IWorkflowDefinitionService _workflowDefinitionService;
-
     private readonly IHttpWorkflowsCacheManager _httpCacheManager;
     private readonly IWorkflowDefinitionCacheManager _workflowCacheManager;
 
@@ -42,8 +36,6 @@ public AutoUpdateTests(App app) : base(app)
         _hasher = Scope.ServiceProvider.GetRequiredService<IHasher>();
         _definitionCacheManager = Scope.ServiceProvider.GetRequiredService<IWorkflowDefinitionCacheManager>();
         _publisher = Scope.ServiceProvider.GetRequiredService<IWorkflowDefinitionPublisher>();
-        _definitionManager = Scope.ServiceProvider.GetRequiredService<IWorkflowDefinitionManager>();
-        _workflowDefinitionService = Scope.ServiceProvider.GetRequiredService<IWorkflowDefinitionService>();
 
         _httpCacheManager = Scope.ServiceProvider.GetRequiredService<IHttpWorkflowsCacheManager>();
         _workflowCacheManager = Scope.ServiceProvider.GetRequiredService<IWorkflowDefinitionCacheManager>();