Manual backport of #35458 core: do not refresh when forgetting instances (#35467)

* core: do not refresh when forgetting instances

When destroying a resource instance, we always refresh first, to give
the provider a chance to tell us that the instance no longer exists.
This prevents issues with providers that cannot handle delete requests
for already deleted resources.

When forgetting, however, this is unnecessary, since no further requests
to the provider will be made for this instance. Indeed, since a main use
case for the forget functionality is removing instances from state for
which there is no longer a configurable provider, attempting to refresh
is both unnecessary and often problematic.

* update changelog

Author: kmoe

CHANGELOG.md

@@ -4,6 +4,10 @@ ENHANCEMENTS:
 
 * Terraform now returns a more specific error message in the awkward situation where an input variable validation rule is known to have failed (`condition` returned `false`) but the error message is derived from an unknown value. ([#35400](https://github.com/hashicorp/terraform/pull/35400))
 
+BUG FIXES:
+
+* core: Terraform no longer performs an unnecessary refresh when removing an instance targeted by a `removed` block. ([#35458](https://github.com/hashicorp/terraform/pull/35458))
+
 ## 1.9.2 (July 10, 2024)
 
 BUG FIXES:

internal/terraform/context_apply2_test.go

@@ -2671,6 +2671,11 @@ removed {
 		t.Fatalf("diags: %s", diags.Err())
 	}
 
+	// check that the provider was not asked to refresh the resource
+	if p.ReadResourceCalled {
+		t.Fatalf("Expected ReadResource not to be called, but it was called")
+	}
+
 	// check that the provider was not asked to destroy the resource
 	if p.ApplyResourceChangeCalled {
 		t.Fatalf("Expected ApplyResourceChange not to be called, but it was called")
@@ -2717,6 +2722,11 @@ removed {
 		t.Fatalf("diags: %s", diags.Err())
 	}
 
+	// check that the provider was not asked to refresh the resource
+	if p.ReadResourceCalled {
+		t.Fatalf("Expected ReadResource not to be called, but it was called")
+	}
+
 	// check that the provider was not asked to destroy the resource
 	if p.ApplyResourceChangeCalled {
 		t.Fatalf("Expected ApplyResourceChange not to be called, but it was called")

internal/terraform/node_resource_destroy_deposed.go

@@ -108,12 +108,26 @@ func (n *NodePlanDeposedResourceInstanceObject) Execute(ctx EvalContext, op walk
 		return diags
 	}
 
+	var forget bool
+	for _, ft := range n.forgetResources {
+		if ft.Equal(n.ResourceAddr()) {
+			forget = true
+		}
+	}
+	for _, fm := range n.forgetModules {
+		if fm.TargetContains(n.Addr) {
+			forget = true
+		}
+	}
+
 	// We don't refresh during the planDestroy walk, since that is only adding
 	// the destroy changes to the plan and the provider will not be configured
 	// at this point. The other nodes use separate types for plan and destroy,
 	// while deposed instances are always a destroy or forget operation, so the
 	// logic here is a bit overloaded.
-	if !n.skipRefresh && op != walkPlanDestroy {
+	//
+	// We also don't refresh when forgetting instances, as it is unnecessary.
+	if !n.skipRefresh && op != walkPlanDestroy && !forget {
 		// Refresh this object even though it may be destroyed, in
 		// case it's already been deleted outside of Terraform. If this is a
 		// normal plan, providers expect a Read request to remove missing
@@ -149,17 +163,6 @@ func (n *NodePlanDeposedResourceInstanceObject) Execute(ctx EvalContext, op walk
 	}
 
 	if !n.skipPlanChanges {
-		var forget bool
-		for _, ft := range n.forgetResources {
-			if ft.Equal(n.ResourceAddr()) {
-				forget = true
-			}
-		}
-		for _, fm := range n.forgetModules {
-			if fm.TargetContains(n.Addr) {
-				forget = true
-			}
-		}
 		var change *plans.ResourceInstanceChange
 		var pDiags tfdiags.Diagnostics
 		var deferred *providers.Deferred

internal/terraform/node_resource_plan_orphan.go

@@ -110,7 +110,40 @@ func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalCon
 		return diags
 	}
 
-	if !n.skipRefresh {
+	var forget bool
+	for _, ft := range n.forgetResources {
+		if ft.Equal(n.ResourceAddr()) {
+			forget = true
+		}
+	}
+	for _, fm := range n.forgetModules {
+		if fm.TargetContains(n.Addr) {
+			forget = true
+		}
+	}
+	var change *plans.ResourceInstanceChange
+	var pDiags tfdiags.Diagnostics
+	var deferred *providers.Deferred
+	if forget {
+		change, pDiags = n.planForget(ctx, oldState, "")
+		diags = diags.Append(pDiags)
+	} else {
+		change, deferred, pDiags = n.planDestroy(ctx, oldState, "")
+		diags = diags.Append(pDiags)
+
+		if deferred != nil {
+			ctx.Deferrals().ReportResourceInstanceDeferred(n.Addr, deferred.Reason, &plans.ResourceInstanceChange{
+				Addr:   n.Addr,
+				Change: change.Change,
+			})
+			return diags
+		}
+	}
+	if diags.HasErrors() {
+		return diags
+	}
+
+	if !n.skipRefresh && !forget {
 		// Refresh this instance even though it is going to be destroyed, in
 		// order to catch missing resources. If this is a normal plan,
 		// providers expect a Read request to remove missing resources from the
@@ -152,39 +185,6 @@ func (n *NodePlannableResourceInstanceOrphan) managedResourceExecute(ctx EvalCon
 		return diags.Append(n.writeResourceInstanceState(ctx, oldState, workingState))
 	}
 
-	var forget bool
-	for _, ft := range n.forgetResources {
-		if ft.Equal(n.ResourceAddr()) {
-			forget = true
-		}
-	}
-	for _, fm := range n.forgetModules {
-		if fm.TargetContains(n.Addr) {
-			forget = true
-		}
-	}
-	var change *plans.ResourceInstanceChange
-	var pDiags tfdiags.Diagnostics
-	var deferred *providers.Deferred
-	if forget {
-		change, pDiags = n.planForget(ctx, oldState, "")
-		diags = diags.Append(pDiags)
-	} else {
-		change, deferred, pDiags = n.planDestroy(ctx, oldState, "")
-		diags = diags.Append(pDiags)
-
-		if deferred != nil {
-			ctx.Deferrals().ReportResourceInstanceDeferred(n.Addr, deferred.Reason, &plans.ResourceInstanceChange{
-				Addr:   n.Addr,
-				Change: change.Change,
-			})
-			return diags
-		}
-	}
-	if diags.HasErrors() {
-		return diags
-	}
-
 	// We might be able to offer an approximate reason for why we are
 	// planning to delete this object. (This is best-effort; we might
 	// sometimes not have a reason.)