elsa-workflows / elsa-workflows/elsa-core

Nested loops shadow their parent memory blocks

Open
#7,908 0 comments 0 reactions 0 assignees View on GitHub
bug core triaged
Dominant language
C#
Stars
7.9k
Forks
1.5k
Avg merge
15h 22m
Merged PRs (30d)
114

Description

## Description
If I nest a `ParallelForEach` within a `ForEach`, it's impossible to access the correct `CurrentValue` and `CurrentIndex`.

## Steps to Reproduce
To help us identify the issue more quickly, please follow these guidelines:

1. **Detailed Steps**:
- Add a `ForEach` to the workflow
- Add a nested `ParallelForEach` to the workflow (didn't try, but any activities that shadow there parent's variables should be a problem)
- Try to access `currentValue`, `currentIndex`
- I've attached a sample workflow json

## Expected Behavior
`CurrentValue` and `CurrentIndex` should point to the scope of the current loop within `ForEach` and not override the parents variables.

## Actual Behavior
`CurrentValue` and `CurrentIndex` point to values of the last iteration in the `ParallelForEach`.

## Screenshots

Image

Image

[workflow-definition-parallel-for-each-test-28dde9b65c54cab9-4fb7b983f8489938.json](https://github.com/user-attachments/files/30869653/workflow-definition-parallel-for-each-test-28dde9b65c54cab9-4fb7b983f8489938.json)

Image

## Environment
- **Elsa Package Version**: 3.6.1, 3.7.1
- **Operating System**: Windows 11

## Troubleshooting Attempts
I've tried to debug the chain and came down to those two conclusions:
1. Accessing the currentIndex/currentValue variables in an Expression will call `ExpressionExecutionContext.GetVariableInScope`. This will look for memory blocks of those variables, which are not available. Interestingly enough though: `ExpressionExecutionContext.GetVariablesInScope` will find them - there is a fallback to the `activityExecutionContext` implemented:
```
if (!currentScope.TryGetActivityExecutionContext(out var activityExecutionContext))
{
var variables = currentScope.Memory.Blocks.Values
.Where(x => x.Metadata is VariableBlockMetadata)
.Select(x => x.Metadata as VariableBlockMetadata)
.Select(x => x!.Variable)
.ToList();

foreach (var variable in variables)
yield return variable;
}
else
{
var variables = activityExecutionContext.Variables;

foreach (var variable in variables)
yield return variable;
}
```
2. I've checked why the memory blocks aren't there. `CreateActivityExecutionContextAsync` in `WorkflowExecutionContext` will iterate through `variablesToDeclare` and call `activityExecutionContext.ExpressionExecutionContext.CreateVariable(variable.Name, variable.Value);`. There, a shadow variable will be created:
```
var variable = new Variable(name, value)
{
StorageDriverType = storageDriverType ?? typeof(WorkflowInstanceStorageDriver)
};
```
As we don't provide an id for the variable, it will be the default value `currentIndexVariable`/`currentValueVariable`. `ExpressionExecutionContext.Set` will then use `GetBlockInternal` to determine if the block exists and as the parent `ForEach` already declares them, it will override the values of those memory-blocks. So I guess the fix needs to be at this place already. I've tried to write my own `ParallelForEach` and schedule the items like this:
```
// Schedule a body of work for each item.
var tag = Guid.NewGuid();
tags.Add(tag);

// For each item, declare a new variable for the work to be scheduled.
// We specify our own variable id, as otherwise there would be clashes with the memory-blocks in nested ForEach/ForEachParallel
var currentValueVariable = new Variable("CurrentValue", item, $"currentValueVariable-{tag}")
{
// TODO: This should be configurable, because this won't work for e.g. file streams and other non-serializable types.
StorageDriverType = typeof(WorkflowInstanceStorageDriver)
};

var currentIndexVariable = new Variable("CurrentIndex", currentIndex++, $"currentIndexVariable-{tag}")
{
StorageDriverType = typeof(WorkflowInstanceStorageDriver)
};
var variables = new List { currentValueVariable, currentIndexVariable };

await context.ScheduleActivityAsync(Body, OnChildCompleted, tag, variables);
```
But due to the shadow variable creation above, the `id` will never be forwarded to the `ExpressionExecutionContext.Set`.

Contributor guide

Open the contributing guide

Research direction

Start with WorkflowExecutionContext.CreateActivityExecutionContextAsync and follow how variables are passed into ExpressionExecutionContext.Set. Compare GetVariableInScope with GetVariablesInScope while tracing nested ForEach and ParallelForEach execution. Done means each loop resolves its own CurrentValue and CurrentIndex without overriding the parent scope.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.