From 7c1dbc90616ef789bbd103f89f837f7d54e5f2e3 Mon Sep 17 00:00:00 2001
From: Martin Atkins <mart@degeneration.co.uk>
Date: Thu, 27 Jul 2023 09:26:53 -0700
Subject: [PATCH] stackeval: Don't share objects between AsyncTask closures

This is, of course, the classic Go for loop hazard where a closure in the
loop captures the symbol itself rather than the value of the symbol, and
the symbol gets modified directly on each iteration so all of the closures
end up referring to whichever value came last.

This is just an ugly tactical fix for that intentionally not changing the
existing code too much. Hopefully in a future commit we'll rearrange this
a little so that it's harder to accidentally reintroduce this bug later,
and we should also add some more complex test cases with multiple embedded
stack calls and multiple component calls.
---
 .../internal/stackeval/main_plan.go           | 22 +++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/internal/stacks/stackruntime/internal/stackeval/main_plan.go b/internal/stacks/stackruntime/internal/stackeval/main_plan.go
index c6d11f6058..30f82ffd26 100644
--- a/internal/stacks/stackruntime/internal/stackeval/main_plan.go
+++ b/internal/stacks/stackruntime/internal/stackeval/main_plan.go
@@ -136,7 +136,14 @@ func (m *Main) walkPlanChanges(ctx context.Context, walk *planWalk, stack *Stack
 	// we can explore downstream stacks concurrently with this one. Each
 	// stack call can represent zero or more child stacks that we'll analyze
 	// by recursive calls to this function.
-	for _, call := range stack.EmbeddedStackCalls(ctx) {
+	for _, obj := range stack.EmbeddedStackCalls(ctx) {
+		// This must be a local variable inside the loop and _not_ a
+		// loop iterator variable because otherwise the function below
+		// will capture the same variable on every iteration, rather
+		// than a separate value each time.
+		call := obj
+		obj = nil // DO NOT use obj in the rest of this loop
+
 		m.walkPlanValidateConfig(ctx, walk, call.Config(ctx))
 
 		// We need to perform the whole expansion in an overall async task
@@ -160,14 +167,21 @@ func (m *Main) walkPlanChanges(ctx context.Context, walk *planWalk, stack *Stack
 	// We also need to plan all of the other declarations in the current stack.
 
 	for _, obj := range stack.Components(ctx) {
-		m.walkPlanValidateConfig(ctx, walk, obj.Config(ctx))
-		m.walkPlanObjectChanges(ctx, walk, obj)
+		// This must be a local variable inside the loop and _not_ a
+		// loop iterator variable because otherwise the function below
+		// will capture the same variable on every iteration, rather
+		// than a separate value each time.
+		component := obj
+		obj = nil // DO NOT use obj in the rest of this loop
+
+		m.walkPlanValidateConfig(ctx, walk, component.Config(ctx))
+		m.walkPlanObjectChanges(ctx, walk, component)
 
 		// We need to perform the instance expansion in an overall async task
 		// because it involves potentially evaluating a for_each expression.
 		// and that might depend on data from elsewhere in the same stack.
 		walk.AsyncTask(ctx, func(ctx context.Context) {
-			insts := obj.Instances(ctx, PlanPhase)
+			insts := component.Instances(ctx, PlanPhase)
 			for _, inst := range insts {
 				m.walkPlanObjectChanges(ctx, walk, inst)
 			}