From 9ed8d2398c27a65bc308d4ed294120c663e9a202 Mon Sep 17 00:00:00 2001 From: "denis.bilenko" Date: Thu, 29 Oct 2026 17:47:57 +0000 Subject: [PATCH 1/4] locker: don't report a failed lock read as 'lock is not held' Co-authored-by: Isaac --- libs/locker/locker.go | 22 +++++++++++++----- libs/locker/locker_test.go | 46 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 6 deletions(-) create mode 100644 libs/locker/locker_test.go diff --git a/libs/locker/locker.go b/libs/locker/locker.go index 857139c79b4..dd5d6f7d5a6 100644 --- a/libs/locker/locker.go +++ b/libs/locker/locker.go @@ -95,27 +95,34 @@ 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) + 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 +199,12 @@ func (locker *Locker) Unlock(ctx context.Context, opts ...UnlockOption) error { } err := locker.assertLockHeld(ctx) - if err != nil { + if errors.As(err, &lockNotHeldError{}) { 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 diff --git a/libs/locker/locker_test.go b/libs/locker/locker_test.go new file mode 100644 index 00000000000..7f97f442e91 --- /dev/null +++ b/libs/locker/locker_test.go @@ -0,0 +1,46 @@ +package locker + +import ( + "context" + "errors" + "io" + "testing" + "uuid" + + "github.com/databricks/cli/libs/fakefs" + "github.com/databricks/cli/libs/filer" + "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") +} From fc9ae6973b3b5e07e661a5eaf2fbdbd6c9a26034 Mon Sep 17 00:00:00 2001 From: "denis.bilenko" Date: Thu, 29 Oct 2026 17:48:26 +0000 Subject: [PATCH 2/4] Add changelog entry Co-authored-by: Isaac --- .nextchanges/bundles/locker-unlock-error.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 .nextchanges/bundles/locker-unlock-error.md diff --git a/.nextchanges/bundles/locker-unlock-error.md b/.nextchanges/bundles/locker-unlock-error.md new file mode 100644 index 00000000000..caabc1cbe5a --- /dev/null +++ b/.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)) From 121a882baa6c0774ae232509eec5c41daa1856e3 Mon Sep 17 00:00:00 2001 From: "denis.bilenko" Date: Thu, 29 Oct 2026 18:21:09 +0000 Subject: [PATCH 3/4] locker: use errors.AsType Co-authored-by: Isaac --- libs/locker/locker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/locker/locker.go b/libs/locker/locker.go index dd5d6f7d5a6..85e89026a56 100644 --- a/libs/locker/locker.go +++ b/libs/locker/locker.go @@ -199,7 +199,7 @@ func (locker *Locker) Unlock(ctx context.Context, opts ...UnlockOption) error { } err := locker.assertLockHeld(ctx) - if errors.As(err, &lockNotHeldError{}) { + if _, ok := errors.AsType[lockNotHeldError](err); ok { return fmt.Errorf("unlock called when lock is not held: %w", err) } if err != nil { From 8908e985e959127e046c5db65fb82755c5f99814 Mon Sep 17 00:00:00 2001 From: "denis.bilenko" Date: Mon, 2 Nov 2026 12:41:03 +0000 Subject: [PATCH 4/4] locker: treat a lock deleted between stat and download as not held Co-authored-by: Isaac --- libs/locker/locker.go | 5 ++++- libs/locker/locker_test.go | 13 +++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/libs/locker/locker.go b/libs/locker/locker.go index 85e89026a56..d3996b46914 100644 --- a/libs/locker/locker.go +++ b/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 @@ -106,7 +107,9 @@ func (e lockNotHeldError) Unwrap() error { return e.err } // 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) { + // 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 { diff --git a/libs/locker/locker_test.go b/libs/locker/locker_test.go index 7f97f442e91..7e445f4dd95 100644 --- a/libs/locker/locker_test.go +++ b/libs/locker/locker_test.go @@ -9,6 +9,7 @@ import ( "github.com/databricks/cli/libs/fakefs" "github.com/databricks/cli/libs/filer" + "github.com/databricks/databricks-sdk-go/apierr" "github.com/stretchr/testify/assert" ) @@ -44,3 +45,15 @@ func TestUnlockReportsReadFailureAsCheckFailure(t *testing.T) { 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") +}