* backend/local: push responsibility for unlocking state into individual 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) {

backend/remote/backend_context_test.go

@@ -8,6 +8,7 @@ import (
 	"github.com/hashicorp/terraform/backend"
 	"github.com/hashicorp/terraform/configs"
 	"github.com/hashicorp/terraform/internal/initwd"
+	"github.com/hashicorp/terraform/states/statemgr"
 	"github.com/zclconf/go-cty/cty"
 )
 
@@ -186,6 +187,7 @@ func TestRemoteContextWithVars(t *testing.T) {
 				ConfigDir:    configDir,
 				ConfigLoader: configLoader,
 				Workspace:    backend.DefaultStateName,
+				LockState:    true,
 			}
 
 			v := test.Opts
@@ -205,10 +207,21 @@ func TestRemoteContextWithVars(t *testing.T) {
 				if errStr != test.WantError {
 					t.Fatalf("wrong error\ngot:  %s\nwant: %s", errStr, test.WantError)
 				}
+				// When Context() returns an error, it should unlock the state,
+				// so re-locking it is expected to succeed.
+				stateMgr, _ := b.StateMgr(backend.DefaultStateName)
+				if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil {
+					t.Fatalf("unexpected error locking state: %s", err.Error())
+				}
 			} else {
 				if diags.HasErrors() {
 					t.Fatalf("unexpected error\ngot:  %s\nwant: <no error>", diags.Err().Error())
 				}
+				// When Context() succeeds, this should fail w/ "workspace already locked"
+				stateMgr, _ := b.StateMgr(backend.DefaultStateName)
+				if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err == nil {
+					t.Fatal("unexpected success locking state after Context")
+				}
 			}
 		})
 	}

backend/remote/backend_plan_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"
 )
@@ -62,6 +63,12 @@ func TestRemote_planBasic(t *testing.T) {
 	if !strings.Contains(output, "1 to add, 0 to change, 0 to destroy") {
 		t.Fatalf("expected plan summary in output: %s", output)
 	}
+
+	stateMgr, _ := b.StateMgr(backend.DefaultStateName)
+	// An error suggests that the state was not unlocked after the operation finished
+	if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil {
+		t.Fatalf("unexpected error locking state after successful plan: %s", err.Error())
+	}
 }
 
 func TestRemote_planCanceled(t *testing.T) {
@@ -85,6 +92,12 @@ func TestRemote_planCanceled(t *testing.T) {
 	if run.Result == backend.OperationSuccess {
 		t.Fatal("expected plan operation to fail")
 	}
+
+	stateMgr, _ := b.StateMgr(backend.DefaultStateName)
+	// An error suggests that the state was not unlocked after the operation finished
+	if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil {
+		t.Fatalf("unexpected error locking state after cancelled plan: %s", err.Error())
+	}
 }
 
 func TestRemote_planLongLine(t *testing.T) {

command/console.go

@@ -92,8 +92,13 @@ func (c *ConsoleCommand) Run(args []string) int {
 
 	// Get the context
 	ctx, _, ctxDiags := local.Context(opReq)
+	diags = diags.Append(ctxDiags)
+	if ctxDiags.HasErrors() {
+		c.showDiagnostics(diags)
+		return 1
+	}
 
-	// Creating the context can result in a lock, so ensure we release it
+	// Successfully creating the context can result in a lock, so ensure we release it
 	defer func() {
 		err := opReq.StateLocker.Unlock(nil)
 		if err != nil {
@@ -101,12 +106,6 @@ func (c *ConsoleCommand) Run(args []string) int {
 		}
 	}()
 
-	diags = diags.Append(ctxDiags)
-	if ctxDiags.HasErrors() {
-		c.showDiagnostics(diags)
-		return 1
-	}
-
 	// Setup the UI so we can output directly to stdout
 	ui := &cli.BasicUi{
 		Writer:      wrappedstreams.Stdout(),

command/import.go

@@ -200,8 +200,13 @@ func (c *ImportCommand) Run(args []string) int {
 
 	// Get the context
 	ctx, state, ctxDiags := local.Context(opReq)
+	diags = diags.Append(ctxDiags)
+	if ctxDiags.HasErrors() {
+		c.showDiagnostics(diags)
+		return 1
+	}
 
-	// Creating the context can result in a lock, so ensure we release it
+	// Successfully creating the context can result in a lock, so ensure we release it
 	defer func() {
 		err := opReq.StateLocker.Unlock(nil)
 		if err != nil {
@@ -209,12 +214,6 @@ func (c *ImportCommand) Run(args []string) int {
 		}
 	}()
 
-	diags = diags.Append(ctxDiags)
-	if ctxDiags.HasErrors() {
-		c.showDiagnostics(diags)
-		return 1
-	}
-
 	// Perform the import. Note that as you can see it is possible for this
 	// API to import more than one resource at once. For now, we only allow
 	// one while we stabilize this feature.