fix: race condition inside an expanded step regarding dependencies (#277)

Children steps Dependencies array was shared between all the child steps
and the parent step, every modification to one child steps was
propagated to the other child. Impact was detected on template steps
with `foreach_strategy` set to `sequence` as the child steps might be
launched all together, instead of waiting the previous one.

Signed-off-by: Romain Beuque <556072+rbeuque74@users.noreply.github.com>

Author: rbeuque74

engine/engine.go

@@ -20,6 +20,7 @@ import (
 
 	"github.com/ovh/utask"
 	"github.com/ovh/utask/engine/step"
+	"github.com/ovh/utask/engine/step/condition"
 	"github.com/ovh/utask/engine/values"
 	"github.com/ovh/utask/models/resolution"
 	"github.com/ovh/utask/models/runnerinstance"
@@ -738,6 +739,18 @@ func expandStep(s *step.Step, res *resolution.Resolution) {
 	// generate all children steps
 	for i, item := range items {
 		childStepName := fmt.Sprintf("%s-%d", s.Name, i)
+
+		// copy all slices from the parent step to prevent array pointer
+		// to be shared between multiple steps
+		dependencies := make([]string, len(s.Dependencies))
+		customStates := make([]string, len(s.CustomStates))
+		conditions := make([]*condition.Condition, len(s.Conditions))
+		resources := make([]string, len(s.Resources))
+		copy(dependencies, s.Dependencies)
+		copy(customStates, s.CustomStates)
+		copy(conditions, s.Conditions)
+		copy(resources, s.Resources)
+
 		res.Steps[childStepName] = &step.Step{
 			Name:         childStepName,
 			Description:  fmt.Sprintf("%d - %s", i, s.Description),
@@ -747,10 +760,10 @@ func expandStep(s *step.Step, res *resolution.Resolution) {
 			State:        step.StateTODO,
 			RetryPattern: s.RetryPattern,
 			MaxRetries:   s.MaxRetries,
-			Dependencies: s.Dependencies,
-			CustomStates: s.CustomStates,
-			Conditions:   s.Conditions,
-			Resources:    s.Resources,
+			Dependencies: dependencies,
+			CustomStates: customStates,
+			Conditions:   conditions,
+			Resources:    resources,
 			Item:         item,
 		}
 

engine/engine_test.go

@@ -702,6 +702,58 @@ func TestForeachWithChainedIterations(t *testing.T) {
 	td.Cmp(t, res.Steps["generateItems-4"].Dependencies, []string{"generateItems-3"})
 }
 
+func TestForeachWithChainedIterationsWithDepOnParent(t *testing.T) {
+	_, require := td.AssertRequire(t)
+	res, err := createResolution("foreach.yaml", map[string]interface{}{
+		"list": []interface{}{"a", "b", "c", "d", "e"},
+	}, nil)
+	require.Nil(err)
+	require.NotNil(res)
+
+	res.Steps["generateItems"].Dependencies = []string{"emptyLoop"}
+	res.Steps["generateItems"].Conditions[0].Then["this"] = "DONE"
+	res.Steps["generateItems"].Conditions = append(
+		res.Steps["generateItems"].Conditions,
+		&condition.Condition{
+			Type: condition.CHECK,
+			If: []*condition.Assert{
+				{
+					Value:    "{{.iterator}}",
+					Operator: condition.EQ,
+					Expected: "d",
+				},
+			},
+			Then: map[string]string{
+				"this": "SERVER_ERROR",
+			},
+		},
+	)
+	res.Steps["generateItems"].ForEachStrategy = "sequence"
+	err = updateResolution(res)
+	require.Nil(err)
+
+	res, err = runResolution(res)
+	require.NotNil(res)
+	require.Nil(err)
+	require.Cmp(res.State, resolution.StateError)
+
+	td.Cmp(t, res.Steps["emptyLoop"].State, step.StateDone) // running on empty collection is ok
+	td.Cmp(t, res.Steps["concatItems"].State, step.StateTODO)
+	td.Cmp(t, res.Steps["finalStep"].State, step.StateTODO)
+	td.Cmp(t, res.Steps["bStep"].State, "B")
+	td.Cmp(t, res.Steps["generateItems-0"].State, step.StateDone)
+	td.Cmp(t, res.Steps["generateItems-1"].State, step.StateDone)
+	td.Cmp(t, res.Steps["generateItems-2"].State, step.StateDone)
+	td.Cmp(t, res.Steps["generateItems-3"].State, step.StateServerError)
+	td.Cmp(t, res.Steps["generateItems-4"].State, step.StateTODO)
+	td.Cmp(t, res.Steps["generateItems"].Dependencies, []string{"emptyLoop", "generateItems-0:ANY", "generateItems-1:ANY", "generateItems-2:ANY", "generateItems-3:ANY", "generateItems-4:ANY"})
+	td.Cmp(t, res.Steps["generateItems-0"].Dependencies, []string{"emptyLoop"})
+	td.Cmp(t, res.Steps["generateItems-1"].Dependencies, []string{"emptyLoop", "generateItems-0"})
+	td.Cmp(t, res.Steps["generateItems-2"].Dependencies, []string{"emptyLoop", "generateItems-1"})
+	td.Cmp(t, res.Steps["generateItems-3"].Dependencies, []string{"emptyLoop", "generateItems-2"})
+	td.Cmp(t, res.Steps["generateItems-4"].Dependencies, []string{"emptyLoop", "generateItems-3"})
+}
+
 func TestForeachWithPreRun(t *testing.T) {
 	input := map[string]interface{}{}
 	res, err := createResolution("foreachAndPreRun.yaml", input, nil)