Crash due to infinite recursion

[jaredoconnell]

Describe the bug

If a workflow references itself as a foreach subworkflow, it will infinitely load itself without a valid exit condition.

	/home/tguittet/go/pkg/mod/gopkg.in/yaml.v3@v3.0.1/decode.go:197
gopkg.in/yaml%2ev3.(*parser).mapping(0xc02050f808)
	/home/tguittet/go/pkg/mod/gopkg.in/yaml.v3@v3.0.1/decode.go:288 +0x238 fp=0xc08f3816e8 sp=0xc08f381678 pc=0xcb7558
gopkg.in/yaml%2ev3.(*parser).parse(0xc02050f808)
	/home/tguittet/go/pkg/mod/gopkg.in/yaml.v3@v3.0.1/decode.go:155 +0xe9 fp=0xc08f381730 sp=0xc08f3816e8 pc=0xcb6729
gopkg.in/yaml%2ev3.(*parser).parseChild(...)
	/home/tguittet/go/pkg/mod/gopkg.in/yaml.v3@v3.0.1/decode.go:197
gopkg.in/yaml%2ev3.(*parser).document(0xc02050f808)
	/home/tguittet/go/pkg/mod/gopkg.in/yaml.v3@v3.0.1/decode.go:206 +0x79 fp=0xc08f381790 sp=0xc08f381730 pc=0xcb6c19
gopkg.in/yaml%2ev3.(*parser).parse(0xc02050f808)
	/home/tguittet/go/pkg/mod/gopkg.in/yaml.v3@v3.0.1/decode.go:159 +0xa5 fp=0xc08f3817d8 sp=0xc08f381790 pc=0xcb66e5
gopkg.in/yaml%2ev3.unmarshal({0xc04dd49200, 0x591, 0x592}, {0x1b9bbc0, 0xc0205aa960}, 0x47?)
	/home/tguittet/go/pkg/mod/gopkg.in/yaml.v3@v3.0.1/yaml.go:161 +0x2d9 fp=0xc08f381890 sp=0xc08f3817d8 pc=0xce4499
gopkg.in/yaml%2ev3.Unmarshal(...)
	/home/tguittet/go/pkg/mod/gopkg.in/yaml.v3@v3.0.1/yaml.go:89
go.flow.arcalot.io/engine/internal/yaml.parser.Parse({}, {0xc04dd49200, 0x591, 0x592})
	/home/tguittet/projects/arcaflow-engine/internal/yaml/parser.go:136 +0x4f fp=0xc08f3818d8 sp=0xc08f381890 pc=0x184224f
go.flow.arcalot.io/engine/workflow.yamlConverter.FromYAML({{0xc0db970a08?, 0xc08f381968?}}, {0xc04dd49200?, 0xc08f381bb0?, 0xffffffffffffffff?})
	/home/tguittet/projects/arcaflow-engine/workflow/yaml.go:36 +0x3f fp=0xc08f381940 sp=0xc08f3818d8 pc=0x186753f
go.flow.arcalot.io/engine/workflow.(*yamlConverter).FromYAML(0xc0c30049a8?, {0xc04dd49200?, 0xc08f381bb0?, 0x47?})
	<autogenerated>:1 +0x45 fp=0xc08f381978 sp=0xc08f381940 pc=0x1870365
...
go.flow.arcalot.io/engine.SubworkflowCache(0xc0005ac3c0, {0xc0003e49f0, 0x27}, {0x2042f20, 0xc000056ab0}, {0xc00054d9f0, 0x0, 0x0})
	/home/tguittet/projects/arcaflow-engine/engine.go:174 +0x46b fp=0xc0af37e190 sp=0xc0af37dee8 pc=0x188a44b
go.flow.arcalot.io/engine.SubworkflowCache(0xc000592550, {0xc0003e49f0, 0x27}, {0x2042f20, 0xc000056ab0}, {0xc00054d9f0, 0x0, 0x0})
	/home/tguittet/projects/arcaflow-engine/engine.go:174 +0x46b fp=0xc0af37e438 sp=0xc0af37e190 pc=0x188a44b
go.flow.arcalot.io/engine.SubworkflowCache(0xc0005706e0, {0xc0003e49f0, 0x27}, {0x2042f20, 0xc000056ab0}, {0xc00054d9f0, 0x0, 0x0})
	/home/tguittet/projects/arcaflow-engine/engine.go:174 +0x46b fp=0xc0af37e6e0 sp=0xc0af37e438 pc=0x188a44b
go.flow.arcalot.io/engine.SubworkflowCache(0xc00055e870, {0xc0003e49f0, 0x27}, {0x2042f20, 0xc000056ab0}, {0xc00054d9f0, 0x0, 0x0})
	/home/tguittet/projects/arcaflow-engine/engine.go:174 +0x46b fp=0xc0af37e988 sp=0xc0af37e6e0 pc=0x188a44b
go.flow.arcalot.io/engine.SubworkflowCache(0xc000550a00, {0xc0003e49f0, 0x27}, {0x2042f20, 0xc000056ab0}, {0xc00054d9f0, 0x0, 0x0})
	/home/tguittet/projects/arcaflow-engine/engine.go:174 +0x46b fp=0xc0af37ec30 sp=0xc0af37e988 pc=0x188a44b
go.flow.arcalot.io/engine.SubworkflowCache(0xc000168550, {0xc0003e49f0, 0x27}, {0x2042f20, 0xc000056ab0}, {0xc00054d9f0, 0x0, 0x0})
	/home/tguittet/projects/arcaflow-engine/engine.go:174 +0x46b fp=0xc0af37eed8 sp=0xc0af37ec30 pc=0x188a44b
go.flow.arcalot.io/engine.SubworkflowCache(0xc0004fd400, {0xc0003e49f0, 0x27}, {0x2042f20, 0xc000056ab0}, {0xc00054d9f0, 0x0, 0x0})
	/home/tguittet/projects/arcaflow-engine/engine.go:174 +0x46b fp=0xc0af37f180 sp=0xc0af37eed8 pc=0x188a44b
go.flow.arcalot.io/engine.SubworkflowCache(0xc0004f4910, {0xc0003e49f0, 0x27}, {0x2042f20, 0xc000056ab0}, {0xc00054d9f0, 0x0, 0x0})
	/home/tguittet/projects/arcaflow-engine/engine.go:174 +0x46b fp=0xc0af37f428 sp=0xc0af37f180 pc=0x188a44b
go.flow.arcalot.io/engine.SubworkflowCache(0xc000326f50, {0xc0003e49f0, 0x27}, {0x2042f20, 0xc000056ab0}, {0xc00054d9f0, 0x0, 0x0})
	/home/tguittet/projects/arcaflow-engine/engine.go:174 +0x46b fp=0xc0af37f6d0 sp=0xc0af37f428 pc=0x188a44b
go.flow.arcalot.io/engine.SubworkflowCache(0xc0003d3720, {0xc0003e49f0, 0x27}, {0x2042f20, 0xc000056ab0}, {0xc00054d9f0, 0x0, 0x0})
	/home/tguittet/projects/arcaflow-engine/engine.go:174 +0x46b fp=0xc0af37f978 sp=0xc0af37f6d0 pc=0x188a44b
go.flow.arcalot.io/engine.workflowEngine.Parse({{0x20635c8, 0xc00036b410}, {0x205ff60, 0xc000056a90}, 0xc00013a3c0}, {0x2064f10, 0xc0001ee840}, {0x1d41b7a?, 0x44a7cb?})
	/home/tguittet/projects/arcaflow-engine/engine.go:100 +0x174 fp=0xc0af37fa78 sp=0xc0af37f978 pc=0x1889c94
go.flow.arcalot.io/engine.(*workflowEngine).Parse(0xc00013af00?, {0x2064f10?, 0xc0001ee840?}, {0x1d41b7a?, 0x0?})
	<autogenerated>:1 +0x8c fp=0xc0af37faf8 sp=0xc0af37fa78 pc=0x188cf8c
main.runWorkflow({0x20489f8, 0xc000407170}, {0x2064f10, 0xc0001ee840}, {0x1d41b7a, 0x8}, {0x20635c8, 0xc00036b380}, {0xc0002dca80, 0x36e, ...}, ...)
	/home/tguittet/projects/arcaflow-engine/cmd/arcaflow/main.go:203 +0x209 fp=0xc0af37fbd8 sp=0xc0af37faf8 pc=0x188eee9
main.main()
	/home/tguittet/projects/arcaflow-engine/cmd/arcaflow/main.go:189 +0xf75 fp=0xc0af37ff50 sp=0xc0af37fbd8 pc=0x188ebd5

[webbnh]

It appears that we're trying to populate the cache, and that includes adding the subworkflows of the current (sub)workflow, and, most importantly, we apparently assume that the subworkflows are not already in the cache.

I wonder if this isn't a problem all by itself. That is, if we have a complex workflow which uses multiple subworkflows which themselves use a set of common (sub-)subworkflows, then the leaf nodes of this tree would likely end up in the cache multiple times, and that might be annoying or downright problematic.

However, if we did a cache lookup before adding to the cache, that would prevent the problem in my example. And, if we were clever, we should be able to use the same technique to address the recursive reference causing the problem reported in this issue. That is, if we can complete adding the current subworkflow to the cache before moving on to its subworkflows, then a self-reference would "hit" the cache and the recursion would stop.

This would likely address the problem during load. However, presumably there would then be another problem during execution, where the workflow never finishes because it's waiting for a copy of itself to finish which is, in turn, waiting for a copy of itself. However, it's possible that this is a user error which the engine itself does not actually need to detect or solve, since it should become apparent to the user what has happened. Moreover, preventing this might adversely affect certain implementation approaches -- that is, "recursion" itself is not bad, only bottomless recursion is bad!

But, if we wanted to prevent the recursion problem at execution time, we could probably detect it at load time: if a subworkflow attempts to cache itself and gets a "hit" on the cache, that might be a good point at which to bail out, at least until some user expresses a desire to implement a recursive workflow on purpose. 🙂