Don't report a failed deploy-lock read as "unlock called when lock is not held"

.nextchanges/bundles/locker-unlock-error.md

@@ -0,0 +1 @@
+* Fixed a failure to read the deploy lock (for example a timeout) being reported as `unlock called when lock is not held`. ([#7257](https://github.com/databricks/cli/pull/7257))

libs/locker/locker.go

@@ -15,6 +15,7 @@ import (
 	"github.com/databricks/cli/libs/filer"
 	"github.com/databricks/cli/libs/retry"
 	"github.com/databricks/databricks-sdk-go"
+	"github.com/databricks/databricks-sdk-go/apierr"
 )
 
 type UnlockOption int
@@ -95,27 +96,36 @@ func (locker *Locker) GetActiveLockState(ctx context.Context) (*LockState, error
 	return &remoteLock, nil
 }
 
+// lockNotHeldError reports that the lock file is missing or held by someone else, as opposed
+// to a failure to read it.
+type lockNotHeldError struct{ err error }
+
+func (e lockNotHeldError) Error() string { return e.err.Error() }
+func (e lockNotHeldError) Unwrap() error { return e.err }
+
 // asserts whether lock is held by locker. Returns descriptive error with current
 // holder details if locker does not hold the lock
 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: %w", err)
+	// The workspace filer stats the lock file before downloading it, so a lock deleted in between
+	// surfaces as the SDK's not-found error from the download rather than as fs.ErrNotExist.
+	if errors.Is(err, fs.ErrNotExist) || errors.Is(err, apierr.ErrNotFound) {
+		return lockNotHeldError{fmt.Errorf("no active lock on target dir: %w", err)}
 	}
 	if err != nil {
 		return err
 	}
 	if activeLockState.ID != locker.State.ID && !activeLockState.IsForced {
-		return fmt.Errorf("deploy lock acquired by %s at %v.\n"+
+		return lockNotHeldError{fmt.Errorf("deploy lock acquired by %s at %v.\n"+
 			"Another deployment may be in progress. Use --force-lock to override, but this may\n"+
 			"conflict with the other deployment if it is still active",
-			activeLockState.User, activeLockState.AcquisitionTime)
+			activeLockState.User, activeLockState.AcquisitionTime)}
 	}
 	if activeLockState.ID != locker.State.ID && activeLockState.IsForced {
-		return fmt.Errorf("deploy lock force-acquired by %s at %v.\n"+
+		return lockNotHeldError{fmt.Errorf("deploy lock force-acquired by %s at %v.\n"+
 			"Another deployment may be in progress. Use --force-lock to override, but this may\n"+
 			"conflict with the other deployment if it is still active",
-			activeLockState.User, activeLockState.AcquisitionTime)
+			activeLockState.User, activeLockState.AcquisitionTime)}
 	}
 	return nil
 }
@@ -192,9 +202,12 @@ func (locker *Locker) Unlock(ctx context.Context, opts ...UnlockOption) error {
 	}
 
 	err := locker.assertLockHeld(ctx)
-	if err != nil {
+	if _, ok := errors.AsType[lockNotHeldError](err); ok {
 		return fmt.Errorf("unlock called when lock is not held: %w", err)
 	}
+	if err != nil {
+		return fmt.Errorf("failed to check the deploy lock before releasing it: %w", err)
+	}
 	err = locker.filer.Delete(ctx, LockFileName)
 	if err != nil {
 		return err

libs/locker/locker_test.go

@@ -0,0 +1,59 @@
+package locker
+
+import (
+	"context"
+	"errors"
+	"io"
+	"testing"
+	"uuid"
+
+	"github.com/databricks/cli/libs/fakefs"
+	"github.com/databricks/cli/libs/filer"
+	"github.com/databricks/databricks-sdk-go/apierr"
+	"github.com/stretchr/testify/assert"
+)
+
+// failingReadFiler fails every Read, standing in for a lock-file read that timed out.
+type failingReadFiler struct {
+	filer.Filer
+	err error
+}
+
+func (f failingReadFiler) Read(context.Context, string) (io.ReadCloser, error) {
+	return nil, f.err
+}
+
+func TestUnlockReportsMissingLockAsNotHeld(t *testing.T) {
+	l := &Locker{
+		filer:  filer.NewFakeFiler(map[string]fakefs.FileInfo{}),
+		Active: true,
+		State:  &LockState{ID: uuid.New()},
+	}
+	err := l.Unlock(t.Context())
+	assert.ErrorContains(t, err, "unlock called when lock is not held: no active lock on target dir")
+}
+
+func TestUnlockReportsReadFailureAsCheckFailure(t *testing.T) {
+	readErr := errors.New(`Get "https://example.com/api/2.0/workspace/export": request timed out after 1m30s of inactivity`)
+	l := &Locker{
+		filer:  failingReadFiler{Filer: filer.NewFakeFiler(map[string]fakefs.FileInfo{LockFileName: {}}), err: readErr},
+		Active: true,
+		State:  &LockState{ID: uuid.New()},
+	}
+	err := l.Unlock(t.Context())
+	assert.ErrorIs(t, err, readErr)
+	assert.ErrorContains(t, err, "failed to check the deploy lock before releasing it")
+	assert.NotContains(t, err.Error(), "not held")
+}
+
+func TestUnlockReportsLockDeletedBeforeDownloadAsNotHeld(t *testing.T) {
+	// The workspace filer's stat succeeded, then the export found nothing.
+	downloadErr := &apierr.APIError{StatusCode: 404, ErrorCode: "RESOURCE_DOES_NOT_EXIST", Message: "Path does not exist"}
+	l := &Locker{
+		filer:  failingReadFiler{Filer: filer.NewFakeFiler(map[string]fakefs.FileInfo{LockFileName: {}}), err: downloadErr},
+		Active: true,
+		State:  &LockState{ID: uuid.New()},
+	}
+	err := l.Unlock(t.Context())
+	assert.ErrorContains(t, err, "unlock called when lock is not held: no active lock on target dir")
+}