core: Keep old value on error even for delete

When an operation fails, providers may return a null new value rather than
returning a partial state. In that case, we'd prefer to keep the old value
so that we stand the best chance of being able to retry on a subsequent
run.

Previously we were making an exception for the delete action, allowing
the result of that to be null even when an error is returned. In practice
that was a bad idea because it would cause Terraform to lose track of the
object even though it might not actually have been deleted.

Now we'll retain the old object even in the delete case. Providers can
still return partial new objects if they were able to partially complete
a delete operation, in which case we'll discard what we had before, but
if the result is null with errors then we'll assume the delete failed
entirely and so just keep the old state as-is, giving us the opportunity
to refresh it on the next run to see if anything actually happened after
all.

(This also includes a new resource in the test provider which isn't used
by the patch but was useful for some manual UX testing here, so I thought
I'd include it in case it's similarly useful in future, given how simple
its implementation is.)

builtin/providers/test/provider.go

@@ -36,6 +36,7 @@ func Provider() terraform.ResourceProvider {
 			"test_resource_computed_set":     testResourceComputedSet(),
 			"test_resource_config_mode":      testResourceConfigMode(),
 			"test_resource_nested_id":        testResourceNestedId(),
+			"test_undeleteable":              testResourceUndeleteable(),
 		},
 		DataSourcesMap: map[string]*schema.Resource{
 			"test_data_source":    testDataSource(),

builtin/providers/test/resource_undeletable.go

@@ -0,0 +1,30 @@
+package test
+
+import (
+	"fmt"
+
+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func testResourceUndeleteable() *schema.Resource {
+	return &schema.Resource{
+		Create: testResourceUndeleteableCreate,
+		Read:   testResourceUndeleteableRead,
+		Delete: testResourceUndeleteableDelete,
+
+		Schema: map[string]*schema.Schema{},
+	}
+}
+
+func testResourceUndeleteableCreate(d *schema.ResourceData, meta interface{}) error {
+	d.SetId("placeholder")
+	return testResourceUndeleteableRead(d, meta)
+}
+
+func testResourceUndeleteableRead(d *schema.ResourceData, meta interface{}) error {
+	return nil
+}
+
+func testResourceUndeleteableDelete(d *schema.ResourceData, meta interface{}) error {
+	return fmt.Errorf("test_undeleteable always fails deletion (use terraform state rm if you really want to delete it)")
+}

terraform/context_apply_test.go

@@ -7097,6 +7097,80 @@ func TestContext2Apply_error(t *testing.T) {
 	}
 }
 
+func TestContext2Apply_errorDestroy(t *testing.T) {
+	m := testModule(t, "empty")
+	p := testProvider("test")
+
+	p.GetSchemaReturn = &ProviderSchema{
+		ResourceTypes: map[string]*configschema.Block{
+			"test_thing": {
+				Attributes: map[string]*configschema.Attribute{
+					"id": {Type: cty.String, Optional: true},
+				},
+			},
+		},
+	}
+	p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse {
+		// Should actually be called for this test, because Terraform Core
+		// constructs the plan for a destroy operation itself.
+		return providers.PlanResourceChangeResponse{
+			PlannedState: req.ProposedNewState,
+		}
+	}
+	p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse {
+		// The apply (in this case, a destroy) always fails, so we can verify
+		// that the object stays in the state after a destroy fails even though
+		// we aren't returning a new state object here.
+		return providers.ApplyResourceChangeResponse{
+			Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("failed")),
+		}
+	}
+
+	ctx := testContext2(t, &ContextOpts{
+		Config: m,
+		State: states.BuildState(func(ss *states.SyncState) {
+			ss.SetResourceInstanceCurrent(
+				addrs.Resource{
+					Mode: addrs.ManagedResourceMode,
+					Type: "test_thing",
+					Name: "foo",
+				}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
+				&states.ResourceInstanceObjectSrc{
+					Status:    states.ObjectReady,
+					AttrsJSON: []byte(`{"id":"baz"}`),
+				},
+				addrs.ProviderConfig{
+					Type: "test",
+				}.Absolute(addrs.RootModuleInstance),
+			)
+		}),
+		ProviderResolver: providers.ResolverFixed(
+			map[string]providers.Factory{
+				"test": testProviderFuncFixed(p),
+			},
+		),
+	})
+
+	if _, diags := ctx.Plan(); diags.HasErrors() {
+		t.Fatalf("plan errors: %s", diags.Err())
+	}
+
+	state, diags := ctx.Apply()
+	if diags == nil {
+		t.Fatal("should have error")
+	}
+
+	actual := strings.TrimSpace(state.String())
+	expected := strings.TrimSpace(`
+test_thing.foo:
+  ID = baz
+  provider = provider.test
+`) // test_thing.foo is still here, even though provider returned no new state along with its error
+	if actual != expected {
+		t.Fatalf("expected:\n%s\n\ngot:\n%s", expected, actual)
+	}
+}
+
 func TestContext2Apply_errorCreateInvalidNew(t *testing.T) {
 	m := testModule(t, "apply-error")
 

terraform/eval_apply.go

@@ -247,11 +247,11 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) {
 	}
 
 	// Sometimes providers return a null value when an operation fails for some
-	// reason, but for any action other than delete we'd rather keep the prior
-	// state so that the error can be corrected on a subsequent run. We must
-	// only do this for null new value though, or else we may discard partial
-	// updates the provider was able to complete.
-	if change.Action != plans.Delete && diags.HasErrors() && newVal.IsNull() {
+	// reason, but we'd rather keep the prior state so that the error can be
+	// corrected on a subsequent run. We must only do this for null new value
+	// though, or else we may discard partial updates the provider was able to
+	// complete.
+	if diags.HasErrors() && newVal.IsNull() {
 		// Otherwise, we'll continue but using the prior state as the new value,
 		// making this effectively a no-op. If the item really _has_ been
 		// deleted then our next refresh will detect that and fix it up.