stackeval: When being destroyed, component instance result comes from plan

Because we treat dependency edges as reversed when a component instance
is being destroyed, the final result (an object representing output values)
for a component instance being destroyed must not depend on anything else
in the evaluation graph, or else we'd cause a promise self-reference as
the downstream component tries to configure itself based on our outputs.

As a special case then, for a component instance being destroyed we take
the planned output values directly from the plan, relying on the fact that
the plan phase sets them to the prior state output values in that case,
and therefore the result for such a component is available immediately
without blocking on any other expression evaluation during the apply phase.

This combines with several previous commits to create a first pass at
handling ordering correctly when planning and applying a full destroy.

This commit also incorporates some fixes and improvements to stackeval's
apply-time testing helpers, which had some quirks and bugs when first
added in a recent commit. One of those problems also revealed that the
raw state loader was not resilient to a buggy caller setting a state
entry to nil instead of removing it altogether, and that mistake seems
relatively easy to make (as I did here in the test helper) so we'll
tolerate it to make it possible to recover if such a bug does end up
occurring in real code too.

internal/stacks/stackruntime/internal/stackeval/applying_test.go

@@ -12,9 +12,9 @@ import (
 	"testing"
 
 	"github.com/davecgh/go-spew/spew"
-	"github.com/golang/protobuf/proto"
 	"github.com/google/go-cmp/cmp"
 	"github.com/zclconf/go-cty/cty"
+	"google.golang.org/protobuf/encoding/prototext"
 	"google.golang.org/protobuf/types/known/anypb"
 
 	"github.com/hashicorp/terraform/internal/addrs"
@@ -154,12 +154,12 @@ func TestApply_componentOrdering(t *testing.T) {
 	// then hopefully at least one of the planning-specific tests is also
 	// failing, and will give some more clues as to what's gone wrong here.
 	if !plan.Applyable {
-		m := proto.TextMarshaler{
-			Compact:   false,
-			ExpandAny: true,
+		m := prototext.MarshalOptions{
+			Multiline: true,
+			Indent:    "  ",
 		}
 		for _, raw := range rawPlan {
-			t.Log(m.Text(raw))
+			t.Log(m.Format(raw))
 		}
 		t.Fatalf("plan is not applyable")
 	}
@@ -184,6 +184,7 @@ func TestApply_componentOrdering(t *testing.T) {
 
 	type applyResultData struct {
 		NewRawState    map[string]*anypb.Any
+		NewState       *stackstate.State
 		VisitedMarkers []string
 	}
 
@@ -225,11 +226,19 @@ func TestApply_componentOrdering(t *testing.T) {
 
 		assertNoDiagnostics(t, outpTester.Diags())
 
+		rawState := outpTester.RawUpdatedState(t)
+		state, diags := outpTester.Close(t)
+		assertNoDiagnostics(t, diags)
+
 		return applyResultData{
-			NewRawState:    outpTester.RawUpdatedState(t),
+			NewRawState:    rawState,
+			NewState:       state,
 			VisitedMarkers: visitedMarkers,
 		}, nil
 	})
+	if err != nil {
+		t.Fatal(err)
+	}
 
 	{
 		if len(applyResult.VisitedMarkers) != 5 {
@@ -264,51 +273,128 @@ func TestApply_componentOrdering(t *testing.T) {
 	// If the initial plan and apply was successful and made its changes in
 	// the correct order, then we'll also test creating and applying a
 	// destroy-mode plan.
-	// FIXME: We can't actually do this yet, because we don't have the logic
-	// in place for handling component results correctly when destroying.
-	// We'll complete this in a future commit.
-	/*
-		t.Log("destroy plan")
-		planOutput, err = promising.MainTask(ctx, func(ctx context.Context) (*planOutputTester, error) {
-			main := NewForPlanning(cfg, stackstate.NewState(), PlanOpts{
-				PlanningMode: plans.DestroyMode,
-				ProviderFactories: ProviderFactories{
-					testProviderAddr: func() (providers.Interface, error) {
-						return &terraform.MockProvider{
-							GetProviderSchemaResponse: &testProviderSchema,
-							PlanResourceChangeFn: func(prcr providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
-								return providers.PlanResourceChangeResponse{
-									PlannedState: prcr.ProposedNewState,
-								}
-							},
-						}, nil
-					},
+	t.Log("destroy plan")
+	planOutput, err = promising.MainTask(ctx, func(ctx context.Context) (*planOutputTester, error) {
+		main := NewForPlanning(cfg, applyResult.NewState, PlanOpts{
+			PlanningMode: plans.DestroyMode,
+			ProviderFactories: ProviderFactories{
+				testProviderAddr: func() (providers.Interface, error) {
+					return &terraform.MockProvider{
+						GetProviderSchemaResponse: &testProviderSchema,
+						PlanResourceChangeFn: func(prcr providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
+							return providers.PlanResourceChangeResponse{
+								PlannedState: prcr.ProposedNewState,
+							}
+						},
+					}, nil
 				},
-			})
+			},
+		})
 
-			outp, outpTester := testPlanOutput(t)
-			main.PlanAll(ctx, outp)
+		outp, outpTester := testPlanOutput(t)
+		main.PlanAll(ctx, outp)
 
-			return outpTester, nil
-		})
+		return outpTester, nil
+	})
+	if err != nil {
+		t.Fatalf("planning failed: %s", err)
+	}
+
+	rawPlan = planOutput.RawChanges(t)
+	plan, diags = planOutput.Close(t)
+	assertNoDiagnostics(t, diags)
+	if !plan.Applyable {
+		m := prototext.MarshalOptions{
+			Multiline: true,
+			Indent:    "  ",
+		}
+		for _, raw := range rawPlan {
+			t.Log(m.Format(raw))
+		}
+		t.Fatalf("plan is not applyable")
+	}
+
+	// When we apply the destroy plan, the components should be visited in
+	// reverse dependency order to ensure that dependencies outlive their
+	// dependents.
+	t.Log("destroy apply")
+	applyResult, err = promising.MainTask(ctx, func(ctx context.Context) (applyResultData, error) {
+		var visitedMarkers []string
+		var visitedMarkersMu sync.Mutex
+
+		outp, outpTester := testApplyOutput(t, nil)
+
+		main, err := ApplyPlan(ctx, cfg, rawPlan, ApplyOpts{
+			ProviderFactories: ProviderFactories{
+				testProviderAddr: func() (providers.Interface, error) {
+					return &terraform.MockProvider{
+						GetProviderSchemaResponse: &testProviderSchema,
+						ApplyResourceChangeFn: func(arcr providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse {
+							markerStr := arcr.PriorState.GetAttr("marker").AsString()
+							log.Printf("[TRACE] TestApply_componentOrdering: visiting %q", markerStr)
+							visitedMarkersMu.Lock()
+							visitedMarkers = append(visitedMarkers, markerStr)
+							visitedMarkersMu.Unlock()
+
+							return providers.ApplyResourceChangeResponse{
+								NewState: arcr.PlannedState,
+							}
+						},
+					}, nil
+				},
+			},
+		}, outp)
+		if main != nil {
+			defer main.DoCleanup(ctx)
+		}
 		if err != nil {
-			t.Fatalf("planning failed: %s", err)
+			t.Fatal(err)
 		}
 
-		rawPlan = planOutput.RawChanges(t)
-		plan, diags = planOutput.Close(t)
+		assertNoDiagnostics(t, outpTester.Diags())
+
+		rawState := outpTester.RawUpdatedState(t)
+		state, diags := outpTester.Close(t)
 		assertNoDiagnostics(t, diags)
-		if !plan.Applyable {
-			m := proto.TextMarshaler{
-				Compact:   false,
-				ExpandAny: true,
-			}
-			for _, raw := range rawPlan {
-				t.Log(m.Text(raw))
-			}
-			t.Fatalf("plan is not applyable")
+
+		return applyResultData{
+			NewRawState:    rawState,
+			NewState:       state,
+			VisitedMarkers: visitedMarkers,
+		}, nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	{
+		if len(applyResult.VisitedMarkers) != 5 {
+			t.Fatalf("apply didn't visit all of the resources\n%s", spew.Sdump(applyResult.VisitedMarkers))
 		}
-	*/
+		assertSliceElementsInRelativeOrder(
+			t, applyResult.VisitedMarkers,
+			"b.i", "a",
+		)
+		assertSliceElementsInRelativeOrder(
+			t, applyResult.VisitedMarkers,
+			"b.ii", "a",
+		)
+		assertSliceElementsInRelativeOrder(
+			t, applyResult.VisitedMarkers,
+			"b.iii", "a",
+		)
+		assertSliceElementsInRelativeOrder(
+			t, applyResult.VisitedMarkers,
+			"c", "b.i",
+		)
+		assertSliceElementsInRelativeOrder(
+			t, applyResult.VisitedMarkers,
+			"c", "b.ii",
+		)
+		assertSliceElementsInRelativeOrder(
+			t, applyResult.VisitedMarkers,
+			"c", "b.iii",
+		)
+	}
 }
 
 func sliceElementsInRelativeOrder[S ~[]E, E comparable](s S, v1, v2 E) bool {

internal/stacks/stackruntime/internal/stackeval/component_instance.go

@@ -957,7 +957,6 @@ func (c *ComponentInstance) ResultValue(ctx context.Context, phase EvalPhase) ct
 				}
 				attrs[os.Addr.OutputValue.Name] = v
 			}
-			return cty.ObjectVal(attrs)
 		}
 		if decl := c.call.Config(ctx).ModuleTree(ctx); decl != nil {
 			// If the plan only ran partially then we might be missing
@@ -974,10 +973,52 @@ func (c *ComponentInstance) ResultValue(ctx context.Context, phase EvalPhase) ct
 					attrs[name] = cty.DynamicVal
 				}
 			}
+			// In the DestroyMode case above we might also find ourselves
+			// with some remnant additional output values that have since
+			// been removed from the configuration, but yet remain in the
+			// state. Destroying with a different configuration than was
+			// most recently applied is not guaranteed to work, but we
+			// can make it more likely to work by dropping anything that
+			// isn't currently declared, since referring directly to these
+			// would be a static validation error anyway, and including
+			// them might cause aggregate operations like keys(component.foo)
+			// to produce broken results.
+			for name := range attrs {
+				_, declared := decl.Module.Outputs[name]
+				if !declared {
+					// (deleting map elements during iteration is valid in Go,
+					// unlike some other languages.)
+					delete(attrs, name)
+				}
+			}
 		}
 		return cty.ObjectVal(attrs)
 
 	case ApplyPhase, InspectPhase:
+		// As a special case, if we're applying and the planned action is
+		// to destroy then we'll just return the planned output values
+		// verbatim without waiting for anything, so that downstreams can
+		// begin their own destroy phases before we start ours.
+		if phase == ApplyPhase {
+			fullPlan := c.main.PlanBeingApplied()
+			ourPlan := fullPlan.Components.Get(c.Addr())
+			if ourPlan == nil {
+				// Weird, but we'll tolerate it.
+				return cty.DynamicVal
+			}
+			if ourPlan.PlannedAction == plans.Delete {
+				// In this case our result was already decided during the
+				// planning phase, because we can't block on anything else
+				// here to make sure we don't create a self-dependency
+				// while our downstreams are trying to destroy themselves.
+				attrs := make(map[string]cty.Value, len(ourPlan.PlannedOutputValues))
+				for addr, val := range ourPlan.PlannedOutputValues {
+					attrs[addr.Name] = val
+				}
+				return cty.ObjectVal(attrs)
+			}
+		}
+
 		var state *states.State
 		switch phase {
 		case ApplyPhase:

internal/stacks/stackruntime/internal/stackeval/testing_test.go

@@ -242,7 +242,11 @@ func (aot *applyOutputTester) RawUpdatedState(t *testing.T) map[string]*anypb.An
 			t.Fatalf("failed to encode %T: %s", change, err)
 		}
 		for _, protoRaw := range protoChange.Raw {
-			msgs[protoRaw.Key] = protoRaw.Value
+			if protoRaw.Value != nil {
+				msgs[protoRaw.Key] = protoRaw.Value
+			} else {
+				delete(msgs, protoRaw.Key)
+			}
 		}
 	}
 

internal/stacks/stackstate/from_proto.go

@@ -5,6 +5,12 @@ package stackstate
 
 import (
 	"fmt"
+	"log"
+
+	"github.com/zclconf/go-cty/cty"
+	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/reflect/protoreflect"
+	"google.golang.org/protobuf/types/known/anypb"
 
 	"github.com/hashicorp/terraform/internal/addrs"
 	"github.com/hashicorp/terraform/internal/lang/marks"
@@ -13,10 +19,6 @@ import (
 	"github.com/hashicorp/terraform/internal/stacks/stackstate/statekeys"
 	"github.com/hashicorp/terraform/internal/stacks/tfstackdata1"
 	"github.com/hashicorp/terraform/internal/states"
-	"github.com/zclconf/go-cty/cty"
-	"google.golang.org/protobuf/proto"
-	"google.golang.org/protobuf/reflect/protoreflect"
-	"google.golang.org/protobuf/types/known/anypb"
 )
 
 // LoadFromProto produces a [State] object by decoding a raw state map.
@@ -44,6 +46,16 @@ func LoadFromProto(msgs map[string]*anypb.Any) (*State, error) {
 			continue
 		}
 
+		if rawMsg == nil {
+			// This suggests a state mutation bug where a deleted object was
+			// written as a map entry without a value, as opposed to deleting
+			// the value. We tolerate this here just because otherwise it
+			// would be harder to recover once a state has been mutated
+			// incorrectly.
+			log.Panicf("[WARN] stackstate.LoadFromProto: key %s has no associated object; ignoring", rawKey)
+			continue
+		}
+
 		msg, err := anypb.UnmarshalNew(rawMsg, proto.UnmarshalOptions{})
 		if err != nil {
 			return nil, fmt.Errorf("invalid raw value for raw state key %q: %w", rawKey, err)