core: If create leg of create_before_destroy fails, restore deposed

I misunderstood the logic here on the first pass of porting to the new
provider and state types: EvalUndeposeState is supposed to return the
deposed object back to being current again, so we can undo the deposing
in the case where the create leg fails.

If we don't do this, we end up leaving the instance with no current object
at all and with its prior object deposed, and then the later destroy
node deletes that deposed object, leaving the user with no object at all.

For safety we skip this restoration if there _is_ a new current object,
since a failed create can still produce a partial result which we need
to keep to avoid losing track of any remote objects that were successfully
created.

states/module.go

@@ -181,6 +181,28 @@ func (ms *Module) deposeResourceInstanceObject(addr addrs.ResourceInstance, forc
 	return is.deposeCurrentObject(forceKey)
 }
 
+// maybeRestoreResourceInstanceDeposed is the real implementation of
+// SyncState.MaybeRestoreResourceInstanceDeposed.
+func (ms *Module) maybeRestoreResourceInstanceDeposed(addr addrs.ResourceInstance, key DeposedKey) bool {
+	rs := ms.Resource(addr.Resource)
+	if rs == nil {
+		return false
+	}
+	is := rs.Instance(addr.Key)
+	if is == nil {
+		return false
+	}
+	if is.Current != nil {
+		return false
+	}
+	if len(is.Deposed) == 0 {
+		return false
+	}
+	is.Current = is.Deposed[key]
+	delete(is.Deposed, key)
+	return true
+}
+
 // SetOutputValue writes an output value into the state, overwriting any
 // existing value of the same name.
 func (ms *Module) SetOutputValue(name string, value cty.Value, sensitive bool) *OutputValue {

states/sync.go

@@ -361,6 +361,30 @@ func (s *SyncState) ForgetResourceInstanceDeposed(addr addrs.AbsResourceInstance
 	s.maybePruneModule(addr.Module)
 }
 
+// MaybeRestoreResourceInstanceDeposed will restore the deposed object with the
+// given key on the specified resource as the current object for that instance
+// if and only if that would not cause us to forget an existing current
+// object for that instance.
+//
+// Returns true if the object was restored to current, or false if no change
+// was made at all.
+func (s *SyncState) MaybeRestoreResourceInstanceDeposed(addr addrs.AbsResourceInstance, key DeposedKey) bool {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+
+	if key == NotDeposed {
+		panic("MaybeRestoreResourceInstanceDeposed called without DeposedKey")
+	}
+
+	ms := s.state.Module(addr.Module)
+	if ms == nil {
+		// Nothing to do, since the specified deposed object cannot exist.
+		return false
+	}
+
+	return ms.maybeRestoreResourceInstanceDeposed(addr.Resource, key)
+}
+
 // RemovePlannedResourceInstanceObjects removes from the state any resource
 // instance objects that have the status ObjectPlanned, indiciating that they
 // are just transient placeholders created during planning.

terraform/context_apply_test.go

@@ -4600,7 +4600,7 @@ func TestContext2Apply_error_createBeforeDestroy(t *testing.T) {
 	actual := strings.TrimSpace(state.String())
 	expected := strings.TrimSpace(testTerraformApplyErrorCreateBeforeDestroyStr)
 	if actual != expected {
-		t.Fatalf("bad: \n%s\n\nExpected:\n\n%s", actual, expected)
+		t.Fatalf("wrong final state\ngot:\n%s\n\nwant:\n%s", actual, expected)
 	}
 }
 

terraform/eval_state.go

@@ -343,13 +343,24 @@ func (n *EvalDeposeState) Eval(ctx EvalContext) (interface{}, error) {
 	return nil, nil
 }
 
-// EvalUndeposeState is an EvalNode implementation that forgets a particular
-// deposed object from the state, causing Terraform to no longer track it.
+// EvalMaybeRestoreDeposedObject is an EvalNode implementation that will
+// restore a particular deposed object of the specified resource instance
+// to be the "current" object if and only if the instance doesn't currently
+// have a current object.
 //
-// Users of this must ensure that the upstream object that the object was
-// tracking has been deleted in the remote system before this node is
-// evaluated.
-type EvalUndeposeState struct {
+// This is intended for use when the create leg of a create before destroy
+// fails with no partial new object: if we didn't take any action, the user
+// would be left in the unfortunate situation of having no current object
+// and the previously-workign object now deposed. This EvalNode causes a
+// better outcome by restoring things to how they were before the replace
+// operation began.
+//
+// The create operation may have produced a partial result even though it
+// failed and it's important that we don't "forget" that state, so in that
+// situation the prior object remains deposed and the partial new object
+// remains the current object, allowing the situation to hopefully be
+// improved in a subsequent run.
+type EvalMaybeRestoreDeposedObject struct {
 	Addr addrs.ResourceInstance
 
 	// Key is a pointer to the deposed object key that should be forgotten
@@ -358,12 +369,17 @@ type EvalUndeposeState struct {
 }
 
 // TODO: test
-func (n *EvalUndeposeState) Eval(ctx EvalContext) (interface{}, error) {
+func (n *EvalMaybeRestoreDeposedObject) Eval(ctx EvalContext) (interface{}, error) {
 	absAddr := n.Addr.Absolute(ctx.Path())
+	dk := *n.Key
 	state := ctx.State()
 
-	state.ForgetResourceInstanceDeposed(absAddr, *n.Key)
-	log.Printf("[TRACE] EvalDeposeState: %s deposed object %s is forgotten", absAddr, *n.Key)
+	restored := state.MaybeRestoreResourceInstanceDeposed(absAddr, dk)
+	if restored {
+		log.Printf("[TRACE] EvalMaybeRestoreDeposedObject: %s deposed object %s was restored as the current object", absAddr, dk)
+	} else {
+		log.Printf("[TRACE] EvalMaybeRestoreDeposedObject: %s deposed object %s remains deposed", absAddr, dk)
+	}
 
 	return nil, nil
 }

terraform/node_resource_apply_instance.go

@@ -384,7 +384,7 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe
 				If: func(ctx EvalContext) (bool, error) {
 					return createBeforeDestroyEnabled && err != nil, nil
 				},
-				Then: &EvalUndeposeState{
+				Then: &EvalMaybeRestoreDeposedObject{
 					Addr: addr.Resource,
 					Key:  &deposedKey,
 				},

terraform/provider_mock.go

@@ -388,20 +388,22 @@ func (p *MockProvider) ApplyResourceChange(r providers.ApplyResourceChangeReques
 		if err != nil {
 			resp.Diagnostics = resp.Diagnostics.Append(err)
 		}
-		var newVal cty.Value
 		if newState != nil {
-			var err error
-			newVal, err = newState.AttrsAsObjectValue(schema.Block.ImpliedType())
-			if err != nil {
-				resp.Diagnostics = resp.Diagnostics.Append(err)
+			var newVal cty.Value
+			if newState != nil {
+				var err error
+				newVal, err = newState.AttrsAsObjectValue(schema.Block.ImpliedType())
+				if err != nil {
+					resp.Diagnostics = resp.Diagnostics.Append(err)
+				}
+			} else {
+				// If apply returned a nil new state then that's the old way to
+				// indicate that the object was destroyed. Our new interface calls
+				// for that to be signalled as a null value.
+				newVal = cty.NullVal(schema.Block.ImpliedType())
 			}
-		} else {
-			// If apply returned a nil new state then that's the old way to
-			// indicate that the object was destroyed. Our new interface calls
-			// for that to be signalled as a null value.
-			newVal = cty.NullVal(schema.Block.ImpliedType())
+			resp.NewState = newVal
 		}
-		resp.NewState = newVal
 
 		return resp
 	}