* backend/local: push responsibility for unlocking state into individ…

…ual operations

* unlock the state if Context() has an error, exactly as backend/remote does today
* terraform console and terraform import will exit before unlocking state in case of error in Context()
* responsibility for unlocking state in the local backend is pushed down the stack, out of backend.go and into each individual state operation
* add tests confirming that state is not locked after apply and plan

* backend/local: add checks that the state is unlocked after operations

This adds tests to plan, apply and refresh which validate that the state
is unlocked after all operations, regardless of exit status. I've also
added specific tests that force Context() to fail during each operation
to verify that locking behavior specifically.

backend/local/backend.go

@@ -336,16 +336,6 @@ func (b *Local) Operation(ctx context.Context, op *backend.Operation) (*backend.
 		defer stop()
 		defer cancel()
 
-		// the state was locked during context creation, unlock the state when
-		// the operation completes
-		defer func() {
-			err := op.StateLocker.Unlock(nil)
-			if err != nil {
-				b.ShowDiagnostics(err)
-				runningOp.Result = backend.OperationFailure
-			}
-		}()
-
 		defer b.opLock.Unlock()
 		f(stopCtx, cancelCtx, op, runningOp)
 	}()

backend/local/backend_apply.go

@@ -56,6 +56,15 @@ func (b *Local) opApply(
 		b.ReportResult(runningOp, diags)
 		return
 	}
+	// the state was locked during succesfull context creation; unlock the state
+	// when the operation completes
+	defer func() {
+		err := op.StateLocker.Unlock(nil)
+		if err != nil {
+			b.ShowDiagnostics(err)
+			runningOp.Result = backend.OperationFailure
+		}
+	}()
 
 	// Before we do anything else we'll take a snapshot of the prior state
 	// so we can use it for some fixups to our detection of whether the plan

backend/local/backend_apply_test.go

@@ -62,6 +62,7 @@ test_instance.foo:
   provider = provider["registry.terraform.io/hashicorp/test"]
   ami = bar
 `)
+
 }
 
 func TestLocal_applyEmptyDir(t *testing.T) {
@@ -90,6 +91,9 @@ func TestLocal_applyEmptyDir(t *testing.T) {
 	if _, err := os.Stat(b.StateOutPath); err == nil {
 		t.Fatal("should not exist")
 	}
+
+	// the backend should be unlocked after a run
+	assertBackendStateUnlocked(t, b)
 }
 
 func TestLocal_applyEmptyDirDestroy(t *testing.T) {
@@ -179,6 +183,9 @@ test_instance.foo:
   provider = provider["registry.terraform.io/hashicorp/test"]
   ami = bar
 	`)
+
+	// the backend should be unlocked after a run
+	assertBackendStateUnlocked(t, b)
 }
 
 func TestLocal_applyBackendFail(t *testing.T) {
@@ -229,6 +236,9 @@ test_instance.foo:
   provider = provider["registry.terraform.io/hashicorp/test"]
   ami = bar
 	`)
+
+	// the backend should be unlocked after a run
+	assertBackendStateUnlocked(t, b)
 }
 
 type backendWithFailingState struct {

backend/local/backend_local.go

@@ -49,6 +49,18 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, *configload.
 		diags = diags.Append(errwrap.Wrapf("Error locking state: {{err}}", err))
 		return nil, nil, nil, diags
 	}
+
+	defer func() {
+		// If we're returning with errors, and thus not producing a valid
+		// context, we'll want to avoid leaving the workspace locked.
+		if diags.HasErrors() {
+			err := op.StateLocker.Unlock(nil)
+			if err != nil {
+				diags = diags.Append(errwrap.Wrapf("Error unlocking state: {{err}}", err))
+			}
+		}
+	}()
+
 	log.Printf("[TRACE] backend/local: reading remote state for workspace %q", op.Workspace)
 	if err := s.RefreshState(); err != nil {
 		diags = diags.Append(errwrap.Wrapf("Error loading state: {{err}}", err))

backend/local/backend_local_test.go

@@ -0,0 +1,57 @@
+package local
+
+import (
+	"testing"
+
+	"github.com/hashicorp/terraform/backend"
+	"github.com/hashicorp/terraform/internal/initwd"
+)
+
+func TestLocalContext(t *testing.T) {
+	configDir := "./testdata/empty"
+	b, cleanup := TestLocal(t)
+	defer cleanup()
+
+	_, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir)
+	defer configCleanup()
+
+	op := &backend.Operation{
+		ConfigDir:    configDir,
+		ConfigLoader: configLoader,
+		Workspace:    backend.DefaultStateName,
+		LockState:    true,
+	}
+
+	_, _, diags := b.Context(op)
+	if diags.HasErrors() {
+		t.Fatalf("unexpected error: %s", diags.Err().Error())
+	}
+
+	// Context() retains a lock on success
+	assertBackendStateLocked(t, b)
+}
+
+func TestLocalContext_error(t *testing.T) {
+	configDir := "./testdata/apply"
+	b, cleanup := TestLocal(t)
+	defer cleanup()
+
+	_, configLoader, configCleanup := initwd.MustLoadConfigForTests(t, configDir)
+	defer configCleanup()
+
+	op := &backend.Operation{
+		ConfigDir:    configDir,
+		ConfigLoader: configLoader,
+		Workspace:    backend.DefaultStateName,
+		LockState:    true,
+	}
+
+	_, _, diags := b.Context(op)
+	if !diags.HasErrors() {
+		t.Fatal("unexpected success")
+	}
+
+	// Context() unlocks the state on failure
+	assertBackendStateUnlocked(t, b)
+
+}

backend/local/backend_plan.go

@@ -75,6 +75,15 @@ func (b *Local) opPlan(
 		b.ReportResult(runningOp, diags)
 		return
 	}
+	// the state was locked during succesfull context creation; unlock the state
+	// when the operation completes
+	defer func() {
+		err := op.StateLocker.Unlock(nil)
+		if err != nil {
+			b.ShowDiagnostics(err)
+			runningOp.Result = backend.OperationFailure
+		}
+	}()
 
 	// Before we do anything else we'll take a snapshot of the prior state
 	// so we can use it for some fixups to our detection of whether the plan

backend/local/backend_plan_test.go

@@ -41,6 +41,9 @@ func TestLocal_planBasic(t *testing.T) {
 	if !p.PlanResourceChangeCalled {
 		t.Fatal("PlanResourceChange should be called")
 	}
+
+	// the backend should be unlocked after a run
+	assertBackendStateUnlocked(t, b)
 }
 
 func TestLocal_planInAutomation(t *testing.T) {
@@ -128,6 +131,33 @@ func TestLocal_planNoConfig(t *testing.T) {
 	if !strings.Contains(output, "configuration") {
 		t.Fatalf("bad: %s", err)
 	}
+
+	// the backend should be unlocked after a run
+	assertBackendStateUnlocked(t, b)
+}
+
+// This test validates the state lacking behavior when the inner call to
+// Context() fails
+func TestLocal_plan_context_error(t *testing.T) {
+	b, cleanup := TestLocal(t)
+	defer cleanup()
+
+	op, configCleanup := testOperationPlan(t, "./testdata/plan")
+	defer configCleanup()
+	op.PlanRefresh = true
+
+	// we coerce a failure in Context() by omitting the provider schema
+	run, err := b.Operation(context.Background(), op)
+	if err != nil {
+		t.Fatalf("bad: %s", err)
+	}
+	<-run.Done()
+	if run.Result != backend.OperationFailure {
+		t.Fatalf("plan operation succeeded")
+	}
+
+	// the backend should be unlocked after a run
+	assertBackendStateUnlocked(t, b)
 }
 
 func TestLocal_planOutputsChanged(t *testing.T) {

backend/local/backend_refresh.go

@@ -50,6 +50,16 @@ func (b *Local) opRefresh(
 		return
 	}
 
+	// the state was locked during succesfull context creation; unlock the state
+	// when the operation completes
+	defer func() {
+		err := op.StateLocker.Unlock(nil)
+		if err != nil {
+			b.ShowDiagnostics(err)
+			runningOp.Result = backend.OperationFailure
+		}
+	}()
+
 	// Set our state
 	runningOp.State = opState.State()
 	if !runningOp.State.HasResources() {

backend/local/backend_refresh_test.go

@@ -46,6 +46,9 @@ test_instance.foo:
   ID = yes
   provider = provider["registry.terraform.io/hashicorp/test"]
 	`)
+
+	// the backend should be unlocked after a run
+	assertBackendStateUnlocked(t, b)
 }
 
 func TestLocal_refreshNoConfig(t *testing.T) {
@@ -202,6 +205,28 @@ test_instance.foo:
 	`)
 }
 
+// This test validates the state lacking behavior when the inner call to
+// Context() fails
+func TestLocal_refresh_context_error(t *testing.T) {
+	b, cleanup := TestLocal(t)
+	defer cleanup()
+	testStateFile(t, b.StatePath, testRefreshState())
+	op, configCleanup := testOperationRefresh(t, "./testdata/apply")
+	defer configCleanup()
+
+	// we coerce a failure in Context() by omitting the provider schema
+
+	run, err := b.Operation(context.Background(), op)
+	if err != nil {
+		t.Fatalf("bad: %s", err)
+	}
+	<-run.Done()
+	if run.Result == backend.OperationSuccess {
+		t.Fatal("operation succeeded; want failure")
+	}
+	assertBackendStateUnlocked(t, b)
+}
+
 func testOperationRefresh(t *testing.T, configDir string) (*backend.Operation, func()) {
 	t.Helper()
 
@@ -211,6 +236,7 @@ func testOperationRefresh(t *testing.T, configDir string) (*backend.Operation, f
 		Type:         backend.OperationTypeRefresh,
 		ConfigDir:    configDir,
 		ConfigLoader: configLoader,
+		LockState:    true,
 	}, configCleanup
 }
 

backend/local/testing.go

@@ -225,3 +225,29 @@ func mustResourceInstanceAddr(s string) addrs.AbsResourceInstance {
 	}
 	return addr
 }
+
+// assertBackendStateUnlocked attempts to lock the backend state. Failure
+// indicates that the state was indeed locked and therefore this function will
+// return true.
+func assertBackendStateUnlocked(t *testing.T, b *Local) bool {
+	t.Helper()
+	stateMgr, _ := b.StateMgr(backend.DefaultStateName)
+	if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil {
+		t.Errorf("state is already locked: %s", err.Error())
+		return false
+	}
+	return true
+}
+
+// assertBackendStateLocked attempts to lock the backend state. Failure
+// indicates that the state was already locked and therefore this function will
+// return false.
+func assertBackendStateLocked(t *testing.T, b *Local) bool {
+	t.Helper()
+	stateMgr, _ := b.StateMgr(backend.DefaultStateName)
+	if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil {
+		return true
+	}
+	t.Error("unexpected success locking state")
+	return true
+}

backend/remote/backend.go

@@ -18,8 +18,8 @@ import (
 	"github.com/hashicorp/terraform-svchost/disco"
 	"github.com/hashicorp/terraform/backend"
 	"github.com/hashicorp/terraform/configs/configschema"
-	"github.com/hashicorp/terraform/state"
 	"github.com/hashicorp/terraform/state/remote"
+	"github.com/hashicorp/terraform/states/statemgr"
 	"github.com/hashicorp/terraform/terraform"
 	"github.com/hashicorp/terraform/tfdiags"
 	tfversion "github.com/hashicorp/terraform/version"
@@ -591,7 +591,7 @@ func (b *Remote) DeleteWorkspace(name string) error {
 }
 
 // StateMgr implements backend.Enhanced.
-func (b *Remote) StateMgr(name string) (state.State, error) {
+func (b *Remote) StateMgr(name string) (statemgr.Full, error) {
 	if b.workspace == "" && name == backend.DefaultStateName {
 		return nil, backend.ErrDefaultWorkspaceNotSupported
 	}

backend/remote/backend_apply_test.go

@@ -15,6 +15,7 @@ import (
 	"github.com/hashicorp/terraform/backend"
 	"github.com/hashicorp/terraform/internal/initwd"
 	"github.com/hashicorp/terraform/plans/planfile"
+	"github.com/hashicorp/terraform/states/statemgr"
 	"github.com/hashicorp/terraform/terraform"
 	"github.com/mitchellh/cli"
 )
@@ -75,6 +76,12 @@ func TestRemote_applyBasic(t *testing.T) {
 	if !strings.Contains(output, "1 added, 0 changed, 0 destroyed") {
 		t.Fatalf("expected apply summery in output: %s", output)
 	}
+
+	stateMgr, _ := b.StateMgr(backend.DefaultStateName)
+	// An error suggests that the state was not unlocked after apply
+	if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil {
+		t.Fatalf("unexpected error locking state after apply: %s", err.Error())
+	}
 }
 
 func TestRemote_applyCanceled(t *testing.T) {
@@ -98,6 +105,11 @@ func TestRemote_applyCanceled(t *testing.T) {
 	if run.Result == backend.OperationSuccess {
 		t.Fatal("expected apply operation to fail")
 	}
+
+	stateMgr, _ := b.StateMgr(backend.DefaultStateName)
+	if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil {
+		t.Fatalf("unexpected error locking state after cancelling apply: %s", err.Error())
+	}
 }
 
 func TestRemote_applyWithoutPermissions(t *testing.T) {
@@ -386,6 +398,12 @@ func TestRemote_applyNoConfig(t *testing.T) {
 	if !strings.Contains(errOutput, "configuration files found") {
 		t.Fatalf("expected configuration files error, got: %v", errOutput)
 	}
+
+	stateMgr, _ := b.StateMgr(backend.DefaultStateName)
+	// An error suggests that the state was not unlocked after apply
+	if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil {
+		t.Fatalf("unexpected error locking state after failed apply: %s", err.Error())
+	}
 }
 
 func TestRemote_applyNoChanges(t *testing.T) {