backend state locking

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_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,64 @@
+package local
+
+import (
+	"testing"
+
+	"github.com/hashicorp/terraform/backend"
+	"github.com/hashicorp/terraform/internal/initwd"
+	"github.com/hashicorp/terraform/states/statemgr"
+)
+
+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, so this should fail.
+	stateMgr, _ := b.StateMgr(backend.DefaultStateName)
+	if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err == nil {
+		t.Fatalf("unexpected success locking state")
+	}
+}
+
+func TestLocalContext_error(t *testing.T) {
+	configDir := "./testdata/apply-error"
+	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")
+	}
+
+	// When Context() returns an error, it also unlocks the state.
+	// This should therefore succeed.
+	stateMgr, _ := b.StateMgr(backend.DefaultStateName)
+	if _, err := stateMgr.Lock(statemgr.NewLockInfo()); err != nil {
+		t.Fatalf("unexpected error locking state: %s", err.Error())
+	}
+}

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_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/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,21 +92,20 @@ 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 {
 			c.Ui.Error(err.Error())
 		}
 	}()
 
-	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,21 +200,20 @@ 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 {
 			c.Ui.Error(err.Error())
 		}
 	}()
 
-	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.