core: fix deadlock when dependable node replaced with non-dependable one

In #2884, Terraform would hang on graphs with an orphaned resource
depended on an orphaned module.

This is because orphan module nodes (which are dependable) were getting
expanded (replaced) with GraphNodeBasicSubgraph nodes (which are _not_
dependable).

The old `graph.Replace()` code resulted in GraphNodeBasicSubgraph being
entered into the lookaside table, even though it is not dependable.

This resulted in an untraversable edge in the graph, so the graph would
hang and wait forever.

Now, we remove entries from the lookaside table when a dependable node
is being replaced with a non-dependable node. This means we lose an
edge, but we can move forward. It's ~probably~ never correct to be
replacing depenable nodes with non-dependable ones, but this tweak
seemed preferable to tossing a panic in there.

terraform/context_refresh_test.go

@@ -4,6 +4,7 @@ import (
 	"reflect"
 	"sort"
 	"strings"
+	"sync"
 	"testing"
 )
 
@@ -685,6 +686,100 @@ func TestContext2Refresh_vars(t *testing.T) {
 	}
 }
 
+func TestContext2Refresh_orphanModule(t *testing.T) {
+	p := testProvider("aws")
+	m := testModule(t, "refresh-module-orphan")
+
+	// Create a custom refresh function to track the order they were visited
+	var order []string
+	var orderLock sync.Mutex
+	p.RefreshFn = func(
+		info *InstanceInfo,
+		is *InstanceState) (*InstanceState, error) {
+		orderLock.Lock()
+		defer orderLock.Unlock()
+
+		order = append(order, is.ID)
+		return is, nil
+	}
+
+	state := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+				Resources: map[string]*ResourceState{
+					"aws_instance.foo": &ResourceState{
+						Primary: &InstanceState{
+							ID: "i-abc123",
+							Attributes: map[string]string{
+								"childid":      "i-bcd234",
+								"grandchildid": "i-cde345",
+							},
+						},
+						Dependencies: []string{
+							"module.child",
+							"module.child",
+						},
+					},
+				},
+			},
+			&ModuleState{
+				Path: append(rootModulePath, "child"),
+				Resources: map[string]*ResourceState{
+					"aws_instance.bar": &ResourceState{
+						Primary: &InstanceState{
+							ID: "i-bcd234",
+							Attributes: map[string]string{
+								"grandchildid": "i-cde345",
+							},
+						},
+						Dependencies: []string{
+							"module.grandchild",
+						},
+					},
+				},
+				Outputs: map[string]string{
+					"id":            "i-bcd234",
+					"grandchild_id": "i-cde345",
+				},
+			},
+			&ModuleState{
+				Path: append(rootModulePath, "child", "grandchild"),
+				Resources: map[string]*ResourceState{
+					"aws_instance.baz": &ResourceState{
+						Primary: &InstanceState{
+							ID: "i-cde345",
+						},
+					},
+				},
+				Outputs: map[string]string{
+					"id": "i-cde345",
+				},
+			},
+		},
+	}
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		State: state,
+	})
+
+	testCheckDeadlock(t, func() {
+		_, err := ctx.Refresh()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		// TODO: handle order properly for orphaned modules / resources
+		// expected := []string{"i-abc123", "i-bcd234", "i-cde345"}
+		// if !reflect.DeepEqual(order, expected) {
+		// 	t.Fatalf("expected: %#v, got: %#v", expected, order)
+		// }
+	})
+}
+
 func TestContext2Validate(t *testing.T) {
 	p := testProvider("aws")
 	m := testModule(t, "validate-good")

terraform/graph.go

@@ -74,7 +74,11 @@ func (g *Graph) Replace(o, n dag.Vertex) bool {
 	// Go through and update our lookaside to point to the new vertex
 	for k, v := range g.dependableMap {
 		if v == o {
-			g.dependableMap[k] = n
+			if _, ok := n.(GraphNodeDependable); ok {
+				g.dependableMap[k] = n
+			} else {
+				delete(g.dependableMap, k)
+			}
 		}
 	}
 

terraform/graph_test.go

@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"reflect"
 	"strings"
 	"testing"
 )
@@ -20,7 +21,7 @@ func TestGraphAdd(t *testing.T) {
 
 func TestGraphConnectDependent(t *testing.T) {
 	var g Graph
-	g.Add(&testGraphDependable{VertexName: "a", Mock: []string{"a"}})
+	g.Add(&testGraphDependable{VertexName: "a"})
 	b := g.Add(&testGraphDependable{
 		VertexName:      "b",
 		DependentOnMock: []string{"a"},
@@ -37,10 +38,40 @@ func TestGraphConnectDependent(t *testing.T) {
 	}
 }
 
+func TestGraphReplace_DependableWithNonDependable(t *testing.T) {
+	var g Graph
+	a := g.Add(&testGraphDependable{VertexName: "a"})
+	b := g.Add(&testGraphDependable{
+		VertexName:      "b",
+		DependentOnMock: []string{"a"},
+	})
+	newA := "non-dependable-a"
+
+	if missing := g.ConnectDependent(b); len(missing) > 0 {
+		t.Fatalf("bad: %#v", missing)
+	}
+
+	if !g.Replace(a, newA) {
+		t.Fatalf("failed to replace")
+	}
+
+	c := g.Add(&testGraphDependable{
+		VertexName:      "c",
+		DependentOnMock: []string{"a"},
+	})
+
+	// This should fail by reporting missing, since a node with dependable
+	// name "a" is no longer in the graph.
+	missing := g.ConnectDependent(c)
+	expected := []string{"a"}
+	if !reflect.DeepEqual(expected, missing) {
+		t.Fatalf("expected: %#v, got: %#v", expected, missing)
+	}
+}
+
 type testGraphDependable struct {
 	VertexName      string
 	DependentOnMock []string
-	Mock            []string
 }
 
 func (v *testGraphDependable) Name() string {
@@ -48,7 +79,7 @@ func (v *testGraphDependable) Name() string {
 }
 
 func (v *testGraphDependable) DependableName() []string {
-	return v.Mock
+	return []string{v.VertexName}
 }
 
 func (v *testGraphDependable) DependentOn() []string {

terraform/test-fixtures/refresh-module-orphan/child/grandchild/main.tf

@@ -0,0 +1,3 @@
+resource "aws_instance" "baz" {}
+
+output "id" { value = "${aws_instance.baz.id}" }

terraform/test-fixtures/refresh-module-orphan/child/main.tf

@@ -0,0 +1,10 @@
+module "grandchild" {
+  source = "./grandchild"
+}
+
+resource "aws_instance" "bar" {
+  grandchildid = "${module.grandchild.id}"
+}
+
+output "id" { value = "${aws_instance.bar.id}" }
+output "grandchild_id" { value = "${module.grandchild.id}" }

terraform/test-fixtures/refresh-module-orphan/main.tf

@@ -0,0 +1,10 @@
+/*
+module "child" {
+  source = "./child"
+}
+
+resource "aws_instance" "bar" {
+  childid      = "${module.child.id}"
+  grandchildid = "${module.child.grandchild_id}"
+}
+*/