backend/local: release lock if there is an error in Context() (#25427)

* command/console: return in case of errors before trying to unlock remote
state

The remote backend `Context` would exit without an active lock if there
was an error, while the local backend `Context` exited *with* a lock. This
caused a problem in `terraform console`, which would call unlock
regardless of error status.

This commit makes the local and remote backend consistently unlock the
state incase of error, and updates terraform console to check for errors
before trying to unlock the state.

* adding tests for remote and local backends

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())
+	}
+
+	// Conext() 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/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())
 				}
+				// If Context() succeeded, 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")
+				}
 			}
 		})
 	}

command/console.go

@@ -92,6 +92,11 @@ 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
 	defer func() {
@@ -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(),