terraform: CBD edge transpositions must happen atomically

terraform/test-fixtures/transform-create-before-destroy-twice/main.tf

@@ -0,0 +1,9 @@
+resource "aws_lc" "foo" {
+    lifecycle { create_before_destroy = true }
+}
+
+resource "aws_autoscale" "bar" {
+    lc = "${aws_lc.foo.id}"
+
+    lifecycle { create_before_destroy = true }
+}

terraform/test-fixtures/transform-destroy-deps/main.tf

@@ -0,0 +1,5 @@
+resource "aws_lc" "foo" {}
+
+resource "aws_asg" "bar" {
+  lc = "${aws_lc.foo.id}"
+}

terraform/transform_destroy.go

@@ -110,6 +110,13 @@ func (t *DestroyTransformer) Transform(g *Graph) error {
 type CreateBeforeDestroyTransformer struct{}
 
 func (t *CreateBeforeDestroyTransformer) Transform(g *Graph) error {
+	// We "stage" the edge connections/destroys in these slices so that
+	// while we're doing the edge transformations (transpositions) in
+	// the graph, we're not affecting future edge transpositions. These
+	// slices let us stage ALL the changes that WILL happen so that all
+	// of the transformations happen atomically.
+	var connect, destroy []dag.Edge
+
 	for _, v := range g.Vertices() {
 		// We only care to use the destroy nodes
 		dn, ok := v.(GraphNodeDestroy)
@@ -125,7 +132,7 @@ func (t *CreateBeforeDestroyTransformer) Transform(g *Graph) error {
 		// Get the creation side of this node
 		cn := dn.CreateNode()
 
-		// Take all the things which depend on the web creation and
+		// Take all the things which depend on the creation node and
 		// make them dependencies on the destruction. Clarifying this
 		// with an example: if you have a web server and a load balancer
 		// and the load balancer depends on the web server, then when we
@@ -138,13 +145,20 @@ func (t *CreateBeforeDestroyTransformer) Transform(g *Graph) error {
 		// This ensures that.
 		for _, sourceRaw := range g.UpEdges(cn).List() {
 			source := sourceRaw.(dag.Vertex)
-			g.Connect(dag.BasicEdge(dn, source))
+			connect = append(connect, dag.BasicEdge(dn, source))
 		}
 
 		// Swap the edge so that the destroy depends on the creation
 		// happening...
-		g.Connect(dag.BasicEdge(dn, cn))
-		g.RemoveEdge(dag.BasicEdge(cn, dn))
+		connect = append(connect, dag.BasicEdge(dn, cn))
+		destroy = append(destroy, dag.BasicEdge(cn, dn))
+	}
+
+	for _, edge := range connect {
+		g.Connect(edge)
+	}
+	for _, edge := range destroy {
+		g.RemoveEdge(edge)
 	}
 
 	return nil

terraform/transform_destroy_test.go

@@ -30,6 +30,31 @@ func TestDestroyTransformer(t *testing.T) {
 	}
 }
 
+func TestDestroyTransformer_deps(t *testing.T) {
+	mod := testModule(t, "transform-destroy-deps")
+
+	g := Graph{Path: RootModulePath}
+	{
+		tf := &ConfigTransformer{Module: mod}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	{
+		tf := &DestroyTransformer{}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTransformDestroyDepsStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\n%s", actual)
+	}
+}
+
 func TestCreateBeforeDestroyTransformer(t *testing.T) {
 	mod := testModule(t, "transform-create-before-destroy-basic")
 
@@ -62,6 +87,38 @@ func TestCreateBeforeDestroyTransformer(t *testing.T) {
 	}
 }
 
+func TestCreateBeforeDestroyTransformer_twice(t *testing.T) {
+	mod := testModule(t, "transform-create-before-destroy-twice")
+
+	g := Graph{Path: RootModulePath}
+	{
+		tf := &ConfigTransformer{Module: mod}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	{
+		tf := &DestroyTransformer{}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	{
+		tf := &CreateBeforeDestroyTransformer{}
+		if err := tf.Transform(&g); err != nil {
+			t.Fatalf("err: %s", err)
+		}
+	}
+
+	actual := strings.TrimSpace(g.String())
+	expected := strings.TrimSpace(testTransformCreateBeforeDestroyTwiceStr)
+	if actual != expected {
+		t.Fatalf("bad:\n\n%s", actual)
+	}
+}
+
 const testTransformDestroyBasicStr = `
 aws_instance.bar
   aws_instance.bar (destroy)
@@ -73,6 +130,17 @@ aws_instance.foo (destroy)
   aws_instance.bar (destroy)
 `
 
+const testTransformDestroyDepsStr = `
+aws_asg.bar
+  aws_asg.bar (destroy)
+  aws_lc.foo
+aws_asg.bar (destroy)
+aws_lc.foo
+  aws_lc.foo (destroy)
+aws_lc.foo (destroy)
+  aws_asg.bar (destroy)
+`
+
 const testTransformCreateBeforeDestroyBasicStr = `
 aws_instance.web
 aws_instance.web (destroy)
@@ -84,3 +152,15 @@ aws_load_balancer.lb
   aws_load_balancer.lb (destroy)
 aws_load_balancer.lb (destroy)
 `
+
+const testTransformCreateBeforeDestroyTwiceStr = `
+aws_autoscale.bar
+  aws_lc.foo
+aws_autoscale.bar (destroy)
+  aws_autoscale.bar
+aws_lc.foo
+aws_lc.foo (destroy)
+  aws_autoscale.bar
+  aws_autoscale.bar (destroy)
+  aws_lc.foo
+`