Fix locker unlock for destroy (#492)

## Changes
Adds ability for allowing unlock to succeed even if the deploy file is
missing.
 
## Tests
Using integration tests and manually

bundle/deploy/lock/release.go

@@ -2,15 +2,26 @@ package lock
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/databricks/cli/bundle"
+	"github.com/databricks/cli/libs/locker"
 	"github.com/databricks/cli/libs/log"
 )
 
-type release struct{}
+type Goal string
 
-func Release() bundle.Mutator {
+const (
-	return &release{}
+	GoalDeploy  = Goal("deploy")
+	GoalDestroy = Goal("destroy")
+)
+
+type release struct {
+	goal Goal
+}
+
+func Release(goal Goal) bundle.Mutator {
+	return &release{goal}
 }
 
 func (m *release) Name() string {
@@ -32,11 +43,12 @@ func (m *release) Apply(ctx context.Context, b *bundle.Bundle) error {
 	}
 
 	log.Infof(ctx, "Releasing deployment lock")
-	err := b.Locker.Unlock(ctx)
+	switch m.goal {
-	if err != nil {
+	case GoalDeploy:
-		log.Errorf(ctx, "Failed to release deployment lock: %v", err)
+		return b.Locker.Unlock(ctx)
-		return err
+	case GoalDestroy:
+		return b.Locker.Unlock(ctx, locker.AllowLockFileNotExist)
+	default:
+		return fmt.Errorf("unknown goal for lock release: %s", m.goal)
 	}
-
-	return nil
 }

bundle/deployer/deployer.go

@@ -108,6 +108,7 @@ func (d *Deployer) LoadTerraformState(ctx context.Context) error {
 	if err != nil {
 		return err
 	}
+	defer r.Close()
 	err = os.MkdirAll(d.DefaultTerraformRoot(), os.ModeDir)
 	if err != nil {
 		return err

bundle/phases/deploy.go

@@ -22,7 +22,7 @@ func Deploy() bundle.Mutator {
 				terraform.Apply(),
 				terraform.StatePush(),
 			),
-			lock.Release(),
+			lock.Release(lock.GoalDeploy),
 		),
 	)
 

bundle/phases/destroy.go

@@ -20,7 +20,7 @@ func Destroy() bundle.Mutator {
 				terraform.StatePush(),
 				files.Delete(),
 			),
-			lock.Release(),
+			lock.Release(lock.GoalDestroy),
 		),
 	)
 

internal/locker_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/databricks/cli/libs/filer"
 	lockpkg "github.com/databricks/cli/libs/locker"
 	"github.com/databricks/databricks-sdk-go"
 	"github.com/databricks/databricks-sdk-go/service/workspace"
@@ -126,6 +127,7 @@ func TestAccLock(t *testing.T) {
 	// read active locker file
 	r, err := lockers[indexOfActiveLocker].Read(ctx, "foo.json")
 	require.NoError(t, err)
+	defer r.Close()
 	b, err := io.ReadAll(r)
 	require.NoError(t, err)
 
@@ -159,3 +161,67 @@ func TestAccLock(t *testing.T) {
 	assert.NoError(t, err)
 	assert.True(t, lockers[indexOfAnInactiveLocker].Active)
 }
+
+func setupLockerTest(ctx context.Context, t *testing.T) (*lockpkg.Locker, filer.Filer) {
+	t.Log(GetEnvOrSkipTest(t, "CLOUD_ENV"))
+
+	w, err := databricks.NewWorkspaceClient()
+	require.NoError(t, err)
+
+	// create temp wsfs dir
+	tmpDir := temporaryWorkspaceDir(t, w)
+	f, err := filer.NewWorkspaceFilesClient(w, tmpDir)
+	require.NoError(t, err)
+
+	// create locker
+	locker, err := lockpkg.CreateLocker("redfoo@databricks.com", tmpDir, w)
+	require.NoError(t, err)
+
+	return locker, f
+}
+
+func TestAccLockUnlockWithoutAllowsLockFileNotExist(t *testing.T) {
+	ctx := context.Background()
+	locker, f := setupLockerTest(ctx, t)
+	var err error
+
+	// Acquire lock on tmp directory
+	err = locker.Lock(ctx, false)
+	require.NoError(t, err)
+
+	// Assert lock file is created
+	_, err = f.Stat(ctx, "deploy.lock")
+	assert.NoError(t, err)
+
+	// Manually delete lock file
+	err = f.Delete(ctx, "deploy.lock")
+	assert.NoError(t, err)
+
+	// Assert error, because lock file does not exist
+	err = locker.Unlock(ctx)
+	assert.ErrorIs(t, err, fs.ErrNotExist)
+}
+
+func TestAccLockUnlockWithAllowsLockFileNotExist(t *testing.T) {
+	ctx := context.Background()
+	locker, f := setupLockerTest(ctx, t)
+	var err error
+
+	// Acquire lock on tmp directory
+	err = locker.Lock(ctx, false)
+	require.NoError(t, err)
+	assert.True(t, locker.Active)
+
+	// Assert lock file is created
+	_, err = f.Stat(ctx, "deploy.lock")
+	assert.NoError(t, err)
+
+	// Manually delete lock file
+	err = f.Delete(ctx, "deploy.lock")
+	assert.NoError(t, err)
+
+	// Assert error, because lock file does not exist
+	err = locker.Unlock(ctx, lockpkg.AllowLockFileNotExist)
+	assert.NoError(t, err)
+	assert.False(t, locker.Active)
+}

libs/locker/locker.go

@@ -13,8 +13,17 @@ import (
 	"github.com/databricks/cli/libs/filer"
 	"github.com/databricks/databricks-sdk-go"
 	"github.com/google/uuid"
+	"golang.org/x/exp/slices"
 )
 
+type UnlockOption int
+
+const (
+	AllowLockFileNotExist UnlockOption = iota
+)
+
+const LockFileName = "deploy.lock"
+
 // Locker object enables exclusive access to TargetDir's scope for a client. This
 // enables multiple clients to deploy to the same scope (ie TargetDir) in an atomic
 // manner
@@ -65,10 +74,11 @@ type LockState struct {
 
 // GetActiveLockState returns current lock state, irrespective of us holding it.
 func (locker *Locker) GetActiveLockState(ctx context.Context) (*LockState, error) {
-	reader, err := locker.filer.Read(ctx, locker.RemotePath())
+	reader, err := locker.filer.Read(ctx, LockFileName)
 	if err != nil {
 		return nil, err
 	}
+	defer reader.Close()
 
 	bytes, err := io.ReadAll(reader)
 	if err != nil {
@@ -89,7 +99,7 @@ func (locker *Locker) GetActiveLockState(ctx context.Context) (*LockState, error
 func (locker *Locker) assertLockHeld(ctx context.Context) error {
 	activeLockState, err := locker.GetActiveLockState(ctx)
 	if errors.Is(err, fs.ErrNotExist) {
-		return fmt.Errorf("no active lock on target dir: %s", err)
+		return fmt.Errorf("no active lock on target dir: %w", err)
 	}
 	if err != nil {
 		return err
@@ -140,7 +150,7 @@ func (locker *Locker) Lock(ctx context.Context, isForced bool) error {
 		modes = append(modes, filer.OverwriteIfExists)
 	}
 
-	err = locker.filer.Write(ctx, locker.RemotePath(), bytes.NewReader(buf), modes...)
+	err = locker.filer.Write(ctx, LockFileName, bytes.NewReader(buf), modes...)
 	if err != nil {
 		// If the write failed because the lock file already exists, don't return
 		// the error and instead fall through to [assertLockHeld] below.
@@ -161,15 +171,24 @@ func (locker *Locker) Lock(ctx context.Context, isForced bool) error {
 	return nil
 }
 
-func (locker *Locker) Unlock(ctx context.Context) error {
+func (locker *Locker) Unlock(ctx context.Context, opts ...UnlockOption) error {
 	if !locker.Active {
 		return fmt.Errorf("unlock called when lock is not held")
 	}
+
+	// if allowLockFileNotExist is set, do not throw an error if the lock file does
+	// not exist. This is helpful when destroying a bundle in which case the lock
+	// file will be deleted before we have a chance to unlock
+	if _, err := locker.filer.Stat(ctx, LockFileName); errors.Is(err, fs.ErrNotExist) && slices.Contains(opts, AllowLockFileNotExist) {
+		locker.Active = false
+		return nil
+	}
+
 	err := locker.assertLockHeld(ctx)
 	if err != nil {
-		return fmt.Errorf("unlock called when lock is not held: %s", err)
+		return fmt.Errorf("unlock called when lock is not held: %w", err)
 	}
-	err = locker.filer.Delete(ctx, locker.RemotePath())
+	err = locker.filer.Delete(ctx, LockFileName)
 	if err != nil {
 		return err
 	}
@@ -177,11 +196,6 @@ func (locker *Locker) Unlock(ctx context.Context) error {
 	return nil
 }
 
-func (locker *Locker) RemotePath() string {
-	// Note: remote paths are scoped to `targetDir`. Also see [CreateLocker].
-	return "deploy.lock"
-}
-
 func CreateLocker(user string, targetDir string, w *databricks.WorkspaceClient) (*Locker, error) {
 	filer, err := filer.NewWorkspaceFilesClient(w, targetDir)
 	if err != nil {