From 09f90904ce8a7adeab07c9f0e8714d11948ed691 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Fri, 22 May 2026 11:45:27 +0000 Subject: [PATCH 01/10] bundle: extract DeploymentLock interface + workspace filesystem impl Pure code-movement refactor. Wraps the existing workspace-filesystem lock behavior behind a DeploymentLock interface so a follow-up PR can introduce an alternative metadata-service-backed lock implementation without touching deploy/destroy/bind callers again. What changed: - New bundle/deploy/lock/lock.go: DeploymentLock interface, Goal enum (moved from release.go), DeploymentStatus enum, and a NewDeploymentLock factory that unconditionally returns the workspace filesystem implementation. - New bundle/deploy/lock/workspace_filesystem.go: workspaceFilesystemLock struct that implements DeploymentLock. Preserves the historical behavior of the deleted acquire.go / release.go mutators: lock-disabled short-circuit, locker.CreateLocker initialization, the permissions.ReportPossiblePermissionDenied branch on fs.ErrPermission / fs.ErrNotExist, and the destroy-mode locker.AllowLockFileNotExist unlock quirk. - Deleted bundle/deploy/lock/acquire.go and bundle/deploy/lock/release.go. - Updated bundle/phases/{deploy,destroy,bind}.go to construct the lock once via NewDeploymentLock and call Acquire / Release directly instead of through bundle.ApplyContext. The deferred Release now reports DeploymentSuccess / DeploymentFailure based on logdiag.HasError so a future DMS-backed implementation can record the outcome. Behavior is preserved end-to-end: lock-related acceptance goldens (pipelines/{deploy,destroy}/force-lock, bundle/help/bundle-{deploy, destroy}) all pass unchanged. --- bundle/deploy/lock/acquire.go | 69 ------------------ bundle/deploy/lock/lock.go | 41 +++++++++++ bundle/deploy/lock/release.go | 58 --------------- bundle/deploy/lock/workspace_filesystem.go | 84 ++++++++++++++++++++++ bundle/phases/bind.go | 26 +++++-- bundle/phases/deploy.go | 20 ++++-- bundle/phases/destroy.go | 13 +++- 7 files changed, 169 insertions(+), 142 deletions(-) delete mode 100644 bundle/deploy/lock/acquire.go create mode 100644 bundle/deploy/lock/lock.go delete mode 100644 bundle/deploy/lock/release.go create mode 100644 bundle/deploy/lock/workspace_filesystem.go diff --git a/bundle/deploy/lock/acquire.go b/bundle/deploy/lock/acquire.go deleted file mode 100644 index 6e4844ca5ff..00000000000 --- a/bundle/deploy/lock/acquire.go +++ /dev/null @@ -1,69 +0,0 @@ -package lock - -import ( - "context" - "errors" - "io/fs" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/permissions" - "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/locker" - "github.com/databricks/cli/libs/log" -) - -type acquire struct{} - -func Acquire() bundle.Mutator { - return &acquire{} -} - -func (m *acquire) Name() string { - return "lock:acquire" -} - -func (m *acquire) init(ctx context.Context, b *bundle.Bundle) error { - user := b.Config.Workspace.CurrentUser.UserName - dir := b.Config.Workspace.StatePath - l, err := locker.CreateLocker(user, dir, b.WorkspaceClient(ctx)) - if err != nil { - return err - } - - b.Locker = l - return nil -} - -func (m *acquire) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - // Return early if locking is disabled. - if !b.Config.Bundle.Deployment.Lock.IsEnabled() { - log.Infof(ctx, "Skipping; locking is disabled") - return nil - } - - err := m.init(ctx, b) - if err != nil { - return diag.FromErr(err) - } - - force := b.Config.Bundle.Deployment.Lock.Force - log.Infof(ctx, "Acquiring deployment lock (force: %v)", force) - err = b.Locker.Lock(ctx, force) - if err != nil { - log.Errorf(ctx, "Failed to acquire deployment lock: %v", err) - - if errors.Is(err, fs.ErrPermission) { - return permissions.ReportPossiblePermissionDenied(ctx, b, b.Config.Workspace.StatePath) - } - - if errors.Is(err, fs.ErrNotExist) { - // If we get a "doesn't exist" error from the API this indicates - // we either don't have permissions or the path is invalid. - return permissions.ReportPossiblePermissionDenied(ctx, b, b.Config.Workspace.StatePath) - } - - return diag.FromErr(err) - } - - return nil -} diff --git a/bundle/deploy/lock/lock.go b/bundle/deploy/lock/lock.go new file mode 100644 index 00000000000..6e3339d6fdb --- /dev/null +++ b/bundle/deploy/lock/lock.go @@ -0,0 +1,41 @@ +package lock + +import ( + "context" + + "github.com/databricks/cli/bundle" +) + +// Goal describes the purpose of a deployment operation. +type Goal string + +const ( + GoalBind = Goal("bind") + GoalUnbind = Goal("unbind") + GoalDeploy = Goal("deploy") + GoalDestroy = Goal("destroy") +) + +// DeploymentStatus indicates whether the deployment operation succeeded or failed. +type DeploymentStatus int + +const ( + DeploymentSuccess DeploymentStatus = iota + DeploymentFailure +) + +// DeploymentLock manages the deployment lock lifecycle. +type DeploymentLock interface { + // Acquire acquires the deployment lock. + Acquire(ctx context.Context) error + + // Release releases the deployment lock with the given deployment status. + Release(ctx context.Context, status DeploymentStatus) error +} + +// NewDeploymentLock returns a DeploymentLock backed by the workspace +// filesystem. This factory exists so a future change can swap in alternative +// lock implementations without touching callers. +func NewDeploymentLock(b *bundle.Bundle, goal Goal) DeploymentLock { + return newWorkspaceFilesystemLock(b, goal) +} diff --git a/bundle/deploy/lock/release.go b/bundle/deploy/lock/release.go deleted file mode 100644 index 26f95edfc95..00000000000 --- a/bundle/deploy/lock/release.go +++ /dev/null @@ -1,58 +0,0 @@ -package lock - -import ( - "context" - - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/libs/diag" - "github.com/databricks/cli/libs/locker" - "github.com/databricks/cli/libs/log" -) - -type Goal string - -const ( - GoalBind = Goal("bind") - GoalUnbind = Goal("unbind") - 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 { - return "lock:release" -} - -func (m *release) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnostics { - // Return early if locking is disabled. - if !b.Config.Bundle.Deployment.Lock.IsEnabled() { - log.Infof(ctx, "Skipping; locking is disabled") - return nil - } - - // Return early if the locker is not set. - // It is likely an error occurred prior to initialization of the locker instance. - if b.Locker == nil { - log.Warnf(ctx, "Unable to release lock if locker is not configured") - return nil - } - - log.Infof(ctx, "Releasing deployment lock") - switch m.goal { - case GoalDeploy: - return diag.FromErr(b.Locker.Unlock(ctx)) - case GoalBind, GoalUnbind: - return diag.FromErr(b.Locker.Unlock(ctx)) - case GoalDestroy: - return diag.FromErr(b.Locker.Unlock(ctx, locker.AllowLockFileNotExist)) - default: - return diag.Errorf("unknown goal for lock release: %s", m.goal) - } -} diff --git a/bundle/deploy/lock/workspace_filesystem.go b/bundle/deploy/lock/workspace_filesystem.go new file mode 100644 index 00000000000..55da52d6a2e --- /dev/null +++ b/bundle/deploy/lock/workspace_filesystem.go @@ -0,0 +1,84 @@ +package lock + +import ( + "context" + "errors" + "io/fs" + + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/permissions" + "github.com/databricks/cli/libs/locker" + "github.com/databricks/cli/libs/log" +) + +// workspaceFilesystemLock implements DeploymentLock using a lock file in the +// bundle's workspace state path. This preserves the historical behavior of +// the previous lock.Acquire / lock.Release mutators. +type workspaceFilesystemLock struct { + b *bundle.Bundle + goal Goal +} + +func newWorkspaceFilesystemLock(b *bundle.Bundle, goal Goal) *workspaceFilesystemLock { + return &workspaceFilesystemLock{b: b, goal: goal} +} + +func (l *workspaceFilesystemLock) Acquire(ctx context.Context) error { + b := l.b + + // Return early if locking is disabled. + if !b.Config.Bundle.Deployment.Lock.IsEnabled() { + log.Infof(ctx, "Skipping; locking is disabled") + return nil + } + + user := b.Config.Workspace.CurrentUser.UserName + dir := b.Config.Workspace.StatePath + lk, err := locker.CreateLocker(user, dir, b.WorkspaceClient(ctx)) + if err != nil { + return err + } + + b.Locker = lk + + force := b.Config.Bundle.Deployment.Lock.Force + log.Infof(ctx, "Acquiring deployment lock (force: %v)", force) + err = lk.Lock(ctx, force) + if err != nil { + log.Errorf(ctx, "Failed to acquire deployment lock: %v", err) + + // If we get a permission or "doesn't exist" error from the API this + // indicates we either don't have permissions or the path is invalid. + if errors.Is(err, fs.ErrPermission) || errors.Is(err, fs.ErrNotExist) { + diags := permissions.ReportPossiblePermissionDenied(ctx, b, b.Config.Workspace.StatePath) + return diags.Error() + } + + return err + } + + return nil +} + +func (l *workspaceFilesystemLock) Release(ctx context.Context, _ DeploymentStatus) error { + b := l.b + + // Return early if locking is disabled. + if !b.Config.Bundle.Deployment.Lock.IsEnabled() { + log.Infof(ctx, "Skipping; locking is disabled") + return nil + } + + // Return early if the locker is not set. + // It is likely an error occurred prior to initialization of the locker instance. + if b.Locker == nil { + log.Warnf(ctx, "Unable to release lock if locker is not configured") + return nil + } + + log.Infof(ctx, "Releasing deployment lock") + if l.goal == GoalDestroy { + return b.Locker.Unlock(ctx, locker.AllowLockFileNotExist) + } + return b.Locker.Unlock(ctx) +} diff --git a/bundle/phases/bind.go b/bundle/phases/bind.go index 48ba7755714..7b3ce12df64 100644 --- a/bundle/phases/bind.go +++ b/bundle/phases/bind.go @@ -23,13 +23,20 @@ import ( func Bind(ctx context.Context, b *bundle.Bundle, opts *terraform.BindOptions, engine engine.EngineType) { log.Info(ctx, "Phase: bind") - bundle.ApplyContext(ctx, b, lock.Acquire()) - if logdiag.HasError(ctx) { + dl := lock.NewDeploymentLock(b, lock.GoalBind) + if err := dl.Acquire(ctx); err != nil { + logdiag.LogError(ctx, err) return } defer func() { - bundle.ApplyContext(ctx, b, lock.Release(lock.GoalBind)) + status := lock.DeploymentSuccess + if logdiag.HasError(ctx) { + status = lock.DeploymentFailure + } + if err := dl.Release(ctx, status); err != nil { + log.Warnf(ctx, "Failed to release deployment lock: %v", err) + } }() if engine.IsDirect() { @@ -119,13 +126,20 @@ func jsonDump(ctx context.Context, v any, field string) string { func Unbind(ctx context.Context, b *bundle.Bundle, bundleType, tfResourceType, resourceKey string, engine engine.EngineType) { log.Info(ctx, "Phase: unbind") - bundle.ApplyContext(ctx, b, lock.Acquire()) - if logdiag.HasError(ctx) { + dl := lock.NewDeploymentLock(b, lock.GoalUnbind) + if err := dl.Acquire(ctx); err != nil { + logdiag.LogError(ctx, err) return } defer func() { - bundle.ApplyContext(ctx, b, lock.Release(lock.GoalUnbind)) + status := lock.DeploymentSuccess + if logdiag.HasError(ctx) { + status = lock.DeploymentFailure + } + if err := dl.Release(ctx, status); err != nil { + log.Warnf(ctx, "Failed to release deployment lock: %v", err) + } }() if engine.IsDirect() { diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index 1db4b2e02da..d23b9ac2c91 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -125,19 +125,27 @@ func Deploy(ctx context.Context, b *bundle.Bundle, outputHandler sync.OutputHand // Core mutators that CRUD resources and modify deployment state. These // mutators need informed consent if they are potentially destructive. - bundle.ApplySeqContext(ctx, b, - scripts.Execute(config.ScriptPreDeploy), - lock.Acquire(), - ) - + bundle.ApplyContext(ctx, b, scripts.Execute(config.ScriptPreDeploy)) if logdiag.HasError(ctx) { // lock is not acquired here return } + dl := lock.NewDeploymentLock(b, lock.GoalDeploy) + if err := dl.Acquire(ctx); err != nil { + logdiag.LogError(ctx, err) + return + } + // lock is acquired here defer func() { - bundle.ApplyContext(ctx, b, lock.Release(lock.GoalDeploy)) + status := lock.DeploymentSuccess + if logdiag.HasError(ctx) { + status = lock.DeploymentFailure + } + if err := dl.Release(ctx, status); err != nil { + log.Warnf(ctx, "Failed to release deployment lock: %v", err) + } }() uploadLibraries(ctx, b, libs) diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index 98e6f7fee2a..741a30c99c0 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -120,13 +120,20 @@ func Destroy(ctx context.Context, b *bundle.Bundle, engine engine.EngineType) { return } - bundle.ApplyContext(ctx, b, lock.Acquire()) - if logdiag.HasError(ctx) { + dl := lock.NewDeploymentLock(b, lock.GoalDestroy) + if err := dl.Acquire(ctx); err != nil { + logdiag.LogError(ctx, err) return } defer func() { - bundle.ApplyContext(ctx, b, lock.Release(lock.GoalDestroy)) + status := lock.DeploymentSuccess + if logdiag.HasError(ctx) { + status = lock.DeploymentFailure + } + if err := dl.Release(ctx, status); err != nil { + log.Warnf(ctx, "Failed to release deployment lock: %v", err) + } }() if !engine.IsDirect() { From 915430b277013869113a7b21e3b20807b11dba7a Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 28 May 2026 09:47:37 +0000 Subject: [PATCH 02/10] bundle/deploy/lock: own *locker.Locker on the lock object, not on bundle.Bundle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-refactor the locker was stashed on b.Locker so the two mutators (acquire then release) could share it via the Bundle. After PR #5314 both lifecycle methods live on a single struct, so the cross-method state-passing through bundle.Bundle is redundant — grep finds zero external consumers of b.Locker. Move the *locker.Locker to a field on workspaceFilesystemLock and delete the now-unused Locker field on bundle.Bundle. Co-authored-by: Isaac --- bundle/bundle.go | 4 ---- bundle/deploy/lock/workspace_filesystem.go | 19 ++++++++++--------- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/bundle/bundle.go b/bundle/bundle.go index e7eef14b907..72d2fe8824a 100644 --- a/bundle/bundle.go +++ b/bundle/bundle.go @@ -21,7 +21,6 @@ import ( "github.com/databricks/cli/libs/auth" "github.com/databricks/cli/libs/cache" "github.com/databricks/cli/libs/fileset" - "github.com/databricks/cli/libs/locker" "github.com/databricks/cli/libs/log" "github.com/databricks/cli/libs/logdiag" libsync "github.com/databricks/cli/libs/sync" @@ -129,9 +128,6 @@ type Bundle struct { // Stores an initialized copy of this bundle's Terraform wrapper. Terraform *tfexec.Terraform - // Stores the locker responsible for acquiring/releasing a deployment lock. - Locker *locker.Locker - // TerraformPlanPath is the path to the plan from the terraform CLI TerraformPlanPath string diff --git a/bundle/deploy/lock/workspace_filesystem.go b/bundle/deploy/lock/workspace_filesystem.go index 55da52d6a2e..c56bb9d2586 100644 --- a/bundle/deploy/lock/workspace_filesystem.go +++ b/bundle/deploy/lock/workspace_filesystem.go @@ -15,8 +15,11 @@ import ( // bundle's workspace state path. This preserves the historical behavior of // the previous lock.Acquire / lock.Release mutators. type workspaceFilesystemLock struct { - b *bundle.Bundle - goal Goal + // b is retained for the workspace client and the permissions reporter on + // the Acquire error path; lock state itself lives on the struct. + b *bundle.Bundle + locker *locker.Locker + goal Goal } func newWorkspaceFilesystemLock(b *bundle.Bundle, goal Goal) *workspaceFilesystemLock { @@ -39,7 +42,7 @@ func (l *workspaceFilesystemLock) Acquire(ctx context.Context) error { return err } - b.Locker = lk + l.locker = lk force := b.Config.Bundle.Deployment.Lock.Force log.Infof(ctx, "Acquiring deployment lock (force: %v)", force) @@ -61,24 +64,22 @@ func (l *workspaceFilesystemLock) Acquire(ctx context.Context) error { } func (l *workspaceFilesystemLock) Release(ctx context.Context, _ DeploymentStatus) error { - b := l.b - // Return early if locking is disabled. - if !b.Config.Bundle.Deployment.Lock.IsEnabled() { + if !l.b.Config.Bundle.Deployment.Lock.IsEnabled() { log.Infof(ctx, "Skipping; locking is disabled") return nil } // Return early if the locker is not set. // It is likely an error occurred prior to initialization of the locker instance. - if b.Locker == nil { + if l.locker == nil { log.Warnf(ctx, "Unable to release lock if locker is not configured") return nil } log.Infof(ctx, "Releasing deployment lock") if l.goal == GoalDestroy { - return b.Locker.Unlock(ctx, locker.AllowLockFileNotExist) + return l.locker.Unlock(ctx, locker.AllowLockFileNotExist) } - return b.Locker.Unlock(ctx) + return l.locker.Unlock(ctx) } From 7ccf788a6300055488316ee50cf3a44b66707613 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 28 May 2026 09:54:39 +0000 Subject: [PATCH 03/10] bundle/deploy/lock: decouple workspaceFilesystemLock from bundle.Bundle The struct now holds only the primitives + workspace client + a permission-error callback it needs. The bundle-aware wiring lives in NewDeploymentLock, which captures everything from the *bundle.Bundle at construction time. Why: keeps the type-level dependency surface narrow (the impl no longer imports bundle or bundle/permissions), removes a class of accidental coupling that would make alternative lock implementations awkward, and forecloses the possibility of a future bundle <-> lock import cycle. NewDeploymentLock now takes ctx so it can call b.WorkspaceClient(ctx) at construction; the three callers in bundle/phases are updated in one line each. Co-authored-by: Isaac --- bundle/deploy/lock/lock.go | 20 ++++++++-- bundle/deploy/lock/workspace_filesystem.go | 44 +++++++++++----------- bundle/phases/bind.go | 4 +- bundle/phases/deploy.go | 2 +- bundle/phases/destroy.go | 2 +- 5 files changed, 41 insertions(+), 31 deletions(-) diff --git a/bundle/deploy/lock/lock.go b/bundle/deploy/lock/lock.go index 6e3339d6fdb..7d4e0c4a2cf 100644 --- a/bundle/deploy/lock/lock.go +++ b/bundle/deploy/lock/lock.go @@ -4,6 +4,7 @@ import ( "context" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/permissions" ) // Goal describes the purpose of a deployment operation. @@ -34,8 +35,19 @@ type DeploymentLock interface { } // NewDeploymentLock returns a DeploymentLock backed by the workspace -// filesystem. This factory exists so a future change can swap in alternative -// lock implementations without touching callers. -func NewDeploymentLock(b *bundle.Bundle, goal Goal) DeploymentLock { - return newWorkspaceFilesystemLock(b, goal) +// filesystem. The factory captures everything the lock needs from the bundle +// at construction time so the lock implementation itself does not depend on +// bundle.Bundle. +func NewDeploymentLock(ctx context.Context, b *bundle.Bundle, goal Goal) DeploymentLock { + return &workspaceFilesystemLock{ + client: b.WorkspaceClient(ctx), + user: b.Config.Workspace.CurrentUser.UserName, + statePath: b.Config.Workspace.StatePath, + enabled: b.Config.Bundle.Deployment.Lock.IsEnabled(), + force: b.Config.Bundle.Deployment.Lock.Force, + goal: goal, + reportPermissionError: func(ctx context.Context, path string) error { + return permissions.ReportPossiblePermissionDenied(ctx, b, path).Error() + }, + } } diff --git a/bundle/deploy/lock/workspace_filesystem.go b/bundle/deploy/lock/workspace_filesystem.go index c56bb9d2586..786326bc638 100644 --- a/bundle/deploy/lock/workspace_filesystem.go +++ b/bundle/deploy/lock/workspace_filesystem.go @@ -5,56 +5,54 @@ import ( "errors" "io/fs" - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/permissions" "github.com/databricks/cli/libs/locker" "github.com/databricks/cli/libs/log" + "github.com/databricks/databricks-sdk-go" ) // workspaceFilesystemLock implements DeploymentLock using a lock file in the -// bundle's workspace state path. This preserves the historical behavior of -// the previous lock.Acquire / lock.Release mutators. +// bundle's workspace state path. Holds only the primitives it needs so the +// type doesn't pin a *bundle.Bundle; the bundle-aware wiring lives in the +// NewDeploymentLock factory. type workspaceFilesystemLock struct { - // b is retained for the workspace client and the permissions reporter on - // the Acquire error path; lock state itself lives on the struct. - b *bundle.Bundle + client *databricks.WorkspaceClient + user string + statePath string + enabled bool + force bool + + // reportPermissionError explains an FS permission error from the lock + // path back to the user. Injected so this struct doesn't import + // bundle/permissions. + reportPermissionError func(ctx context.Context, path string) error + locker *locker.Locker goal Goal } -func newWorkspaceFilesystemLock(b *bundle.Bundle, goal Goal) *workspaceFilesystemLock { - return &workspaceFilesystemLock{b: b, goal: goal} -} - func (l *workspaceFilesystemLock) Acquire(ctx context.Context) error { - b := l.b - // Return early if locking is disabled. - if !b.Config.Bundle.Deployment.Lock.IsEnabled() { + if !l.enabled { log.Infof(ctx, "Skipping; locking is disabled") return nil } - user := b.Config.Workspace.CurrentUser.UserName - dir := b.Config.Workspace.StatePath - lk, err := locker.CreateLocker(user, dir, b.WorkspaceClient(ctx)) + lk, err := locker.CreateLocker(l.user, l.statePath, l.client) if err != nil { return err } l.locker = lk - force := b.Config.Bundle.Deployment.Lock.Force - log.Infof(ctx, "Acquiring deployment lock (force: %v)", force) - err = lk.Lock(ctx, force) + log.Infof(ctx, "Acquiring deployment lock (force: %v)", l.force) + err = lk.Lock(ctx, l.force) if err != nil { log.Errorf(ctx, "Failed to acquire deployment lock: %v", err) // If we get a permission or "doesn't exist" error from the API this // indicates we either don't have permissions or the path is invalid. if errors.Is(err, fs.ErrPermission) || errors.Is(err, fs.ErrNotExist) { - diags := permissions.ReportPossiblePermissionDenied(ctx, b, b.Config.Workspace.StatePath) - return diags.Error() + return l.reportPermissionError(ctx, l.statePath) } return err @@ -65,7 +63,7 @@ func (l *workspaceFilesystemLock) Acquire(ctx context.Context) error { func (l *workspaceFilesystemLock) Release(ctx context.Context, _ DeploymentStatus) error { // Return early if locking is disabled. - if !l.b.Config.Bundle.Deployment.Lock.IsEnabled() { + if !l.enabled { log.Infof(ctx, "Skipping; locking is disabled") return nil } diff --git a/bundle/phases/bind.go b/bundle/phases/bind.go index 7b3ce12df64..1e13cb2931a 100644 --- a/bundle/phases/bind.go +++ b/bundle/phases/bind.go @@ -23,7 +23,7 @@ import ( func Bind(ctx context.Context, b *bundle.Bundle, opts *terraform.BindOptions, engine engine.EngineType) { log.Info(ctx, "Phase: bind") - dl := lock.NewDeploymentLock(b, lock.GoalBind) + dl := lock.NewDeploymentLock(ctx, b, lock.GoalBind) if err := dl.Acquire(ctx); err != nil { logdiag.LogError(ctx, err) return @@ -126,7 +126,7 @@ func jsonDump(ctx context.Context, v any, field string) string { func Unbind(ctx context.Context, b *bundle.Bundle, bundleType, tfResourceType, resourceKey string, engine engine.EngineType) { log.Info(ctx, "Phase: unbind") - dl := lock.NewDeploymentLock(b, lock.GoalUnbind) + dl := lock.NewDeploymentLock(ctx, b, lock.GoalUnbind) if err := dl.Acquire(ctx); err != nil { logdiag.LogError(ctx, err) return diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index d23b9ac2c91..a16e13a7a3d 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -131,7 +131,7 @@ func Deploy(ctx context.Context, b *bundle.Bundle, outputHandler sync.OutputHand return } - dl := lock.NewDeploymentLock(b, lock.GoalDeploy) + dl := lock.NewDeploymentLock(ctx, b, lock.GoalDeploy) if err := dl.Acquire(ctx); err != nil { logdiag.LogError(ctx, err) return diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index 741a30c99c0..b8a6c811b7a 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -120,7 +120,7 @@ func Destroy(ctx context.Context, b *bundle.Bundle, engine engine.EngineType) { return } - dl := lock.NewDeploymentLock(b, lock.GoalDestroy) + dl := lock.NewDeploymentLock(ctx, b, lock.GoalDestroy) if err := dl.Acquire(ctx); err != nil { logdiag.LogError(ctx, err) return From 00a7e9a42dde2d8527a00606455e37dee7d97186 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 28 May 2026 10:01:45 +0000 Subject: [PATCH 04/10] bundle/deploy/lock: drop the permission-error callback indirection The lock and workspace_filesystem files live in the same package, so the package-level import of bundle/permissions exists regardless of whether the struct hides the call behind a function value. The callback only added a layer of indirection; switch to a direct permissions.ReportPossiblePermissionDenied call and keep a narrow b field purely for that error path. Co-authored-by: Isaac --- bundle/deploy/lock/lock.go | 10 +++------- bundle/deploy/lock/workspace_filesystem.go | 17 +++++++++-------- 2 files changed, 12 insertions(+), 15 deletions(-) diff --git a/bundle/deploy/lock/lock.go b/bundle/deploy/lock/lock.go index 7d4e0c4a2cf..03db85befba 100644 --- a/bundle/deploy/lock/lock.go +++ b/bundle/deploy/lock/lock.go @@ -4,7 +4,6 @@ import ( "context" "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/permissions" ) // Goal describes the purpose of a deployment operation. @@ -35,9 +34,8 @@ type DeploymentLock interface { } // NewDeploymentLock returns a DeploymentLock backed by the workspace -// filesystem. The factory captures everything the lock needs from the bundle -// at construction time so the lock implementation itself does not depend on -// bundle.Bundle. +// filesystem. Captures everything the lock needs from the bundle at +// construction time. func NewDeploymentLock(ctx context.Context, b *bundle.Bundle, goal Goal) DeploymentLock { return &workspaceFilesystemLock{ client: b.WorkspaceClient(ctx), @@ -45,9 +43,7 @@ func NewDeploymentLock(ctx context.Context, b *bundle.Bundle, goal Goal) Deploym statePath: b.Config.Workspace.StatePath, enabled: b.Config.Bundle.Deployment.Lock.IsEnabled(), force: b.Config.Bundle.Deployment.Lock.Force, + b: b, goal: goal, - reportPermissionError: func(ctx context.Context, path string) error { - return permissions.ReportPossiblePermissionDenied(ctx, b, path).Error() - }, } } diff --git a/bundle/deploy/lock/workspace_filesystem.go b/bundle/deploy/lock/workspace_filesystem.go index 786326bc638..f934c52a658 100644 --- a/bundle/deploy/lock/workspace_filesystem.go +++ b/bundle/deploy/lock/workspace_filesystem.go @@ -5,15 +5,16 @@ import ( "errors" "io/fs" + "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/permissions" "github.com/databricks/cli/libs/locker" "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go" ) // workspaceFilesystemLock implements DeploymentLock using a lock file in the -// bundle's workspace state path. Holds only the primitives it needs so the -// type doesn't pin a *bundle.Bundle; the bundle-aware wiring lives in the -// NewDeploymentLock factory. +// bundle's workspace state path. Holds only the primitives it needs from the +// bundle plus a reference for the permission-error path. type workspaceFilesystemLock struct { client *databricks.WorkspaceClient user string @@ -21,10 +22,9 @@ type workspaceFilesystemLock struct { enabled bool force bool - // reportPermissionError explains an FS permission error from the lock - // path back to the user. Injected so this struct doesn't import - // bundle/permissions. - reportPermissionError func(ctx context.Context, path string) error + // b is retained only for permissions.ReportPossiblePermissionDenied on + // the Acquire error path. + b *bundle.Bundle locker *locker.Locker goal Goal @@ -52,7 +52,8 @@ func (l *workspaceFilesystemLock) Acquire(ctx context.Context) error { // If we get a permission or "doesn't exist" error from the API this // indicates we either don't have permissions or the path is invalid. if errors.Is(err, fs.ErrPermission) || errors.Is(err, fs.ErrNotExist) { - return l.reportPermissionError(ctx, l.statePath) + diags := permissions.ReportPossiblePermissionDenied(ctx, l.b, l.statePath) + return diags.Error() } return err From 0433a83a5405a1681fde3335e46d173a8111f1ab Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 28 May 2026 10:02:40 +0000 Subject: [PATCH 05/10] bundle/deploy/lock: drop redundant Errorf on failed lock acquire The error is returned to the caller; logging it here just produces a duplicate line in the user-facing output. Drop the log; preserve the permission-denied branch and the bare return. Co-authored-by: Isaac --- bundle/deploy/lock/workspace_filesystem.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/bundle/deploy/lock/workspace_filesystem.go b/bundle/deploy/lock/workspace_filesystem.go index f934c52a658..19a5f4c952f 100644 --- a/bundle/deploy/lock/workspace_filesystem.go +++ b/bundle/deploy/lock/workspace_filesystem.go @@ -47,8 +47,6 @@ func (l *workspaceFilesystemLock) Acquire(ctx context.Context) error { log.Infof(ctx, "Acquiring deployment lock (force: %v)", l.force) err = lk.Lock(ctx, l.force) if err != nil { - log.Errorf(ctx, "Failed to acquire deployment lock: %v", err) - // If we get a permission or "doesn't exist" error from the API this // indicates we either don't have permissions or the path is invalid. if errors.Is(err, fs.ErrPermission) || errors.Is(err, fs.ErrNotExist) { From 8f541a0c1db86b712c57757dbd3103b00fe6b649 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 28 May 2026 10:06:15 +0000 Subject: [PATCH 06/10] bundle/phases: surface Release errors via logdiag (restore pre-refactor semantics) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original release.go mutator returned diag.FromErr on unlock failure, which surfaced as an error diagnostic to the user. The defer pattern introduced in #5314 dropped it to log.Warnf — a silent demotion that would hide a stuck lock from the user (who normally has to recover with --force-lock). Switch the defer to logdiag.LogError so the unlock failure shows up as a proper diagnostic, matching the pre-refactor behavior. The deploy/destroy/ bind/unbind phases all share the same fix. Co-authored-by: Isaac --- bundle/phases/bind.go | 4 ++-- bundle/phases/deploy.go | 2 +- bundle/phases/destroy.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bundle/phases/bind.go b/bundle/phases/bind.go index 1e13cb2931a..ca2091e8bbb 100644 --- a/bundle/phases/bind.go +++ b/bundle/phases/bind.go @@ -35,7 +35,7 @@ func Bind(ctx context.Context, b *bundle.Bundle, opts *terraform.BindOptions, en status = lock.DeploymentFailure } if err := dl.Release(ctx, status); err != nil { - log.Warnf(ctx, "Failed to release deployment lock: %v", err) + logdiag.LogError(ctx, err) } }() @@ -138,7 +138,7 @@ func Unbind(ctx context.Context, b *bundle.Bundle, bundleType, tfResourceType, r status = lock.DeploymentFailure } if err := dl.Release(ctx, status); err != nil { - log.Warnf(ctx, "Failed to release deployment lock: %v", err) + logdiag.LogError(ctx, err) } }() diff --git a/bundle/phases/deploy.go b/bundle/phases/deploy.go index a16e13a7a3d..ca215b93b84 100644 --- a/bundle/phases/deploy.go +++ b/bundle/phases/deploy.go @@ -144,7 +144,7 @@ func Deploy(ctx context.Context, b *bundle.Bundle, outputHandler sync.OutputHand status = lock.DeploymentFailure } if err := dl.Release(ctx, status); err != nil { - log.Warnf(ctx, "Failed to release deployment lock: %v", err) + logdiag.LogError(ctx, err) } }() diff --git a/bundle/phases/destroy.go b/bundle/phases/destroy.go index b8a6c811b7a..a35380f88aa 100644 --- a/bundle/phases/destroy.go +++ b/bundle/phases/destroy.go @@ -132,7 +132,7 @@ func Destroy(ctx context.Context, b *bundle.Bundle, engine engine.EngineType) { status = lock.DeploymentFailure } if err := dl.Release(ctx, status); err != nil { - log.Warnf(ctx, "Failed to release deployment lock: %v", err) + logdiag.LogError(ctx, err) } }() From 6cad232aabca24c1c4b8cc3f0a815e0c8a3e305a Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 28 May 2026 10:13:19 +0000 Subject: [PATCH 07/10] bundle/deploy/lock: lift the *bundle.Bundle reference off workspaceFilesystemLock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReportPossiblePermissionDenied has a second caller (bundle/deploy/files/ upload.go) and walks bundle.Config.RunAs / Permissions / Resources via analyzeBundlePermissions, so the lock can't simply inline it or call a simplified variant. But the struct doesn't need to pin a *bundle.Bundle field — the bundle-aware wiring can live in a callback closure that NewDeploymentLock builds at construction time. After this commit workspaceFilesystemLock holds only primitives, a workspace client, and the callback; no bundle types appear in its field list. Co-authored-by: Isaac --- bundle/deploy/lock/lock.go | 9 +++++++-- bundle/deploy/lock/workspace_filesystem.go | 15 +++++++-------- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/bundle/deploy/lock/lock.go b/bundle/deploy/lock/lock.go index 03db85befba..bd49a4fa48a 100644 --- a/bundle/deploy/lock/lock.go +++ b/bundle/deploy/lock/lock.go @@ -4,6 +4,8 @@ import ( "context" "github.com/databricks/cli/bundle" + "github.com/databricks/cli/bundle/permissions" + "github.com/databricks/cli/libs/diag" ) // Goal describes the purpose of a deployment operation. @@ -35,7 +37,8 @@ type DeploymentLock interface { // NewDeploymentLock returns a DeploymentLock backed by the workspace // filesystem. Captures everything the lock needs from the bundle at -// construction time. +// construction time so the lock implementation itself does not retain a +// *bundle.Bundle reference. func NewDeploymentLock(ctx context.Context, b *bundle.Bundle, goal Goal) DeploymentLock { return &workspaceFilesystemLock{ client: b.WorkspaceClient(ctx), @@ -43,7 +46,9 @@ func NewDeploymentLock(ctx context.Context, b *bundle.Bundle, goal Goal) Deploym statePath: b.Config.Workspace.StatePath, enabled: b.Config.Bundle.Deployment.Lock.IsEnabled(), force: b.Config.Bundle.Deployment.Lock.Force, - b: b, goal: goal, + reportPermissionError: func(ctx context.Context, path string) diag.Diagnostics { + return permissions.ReportPossiblePermissionDenied(ctx, b, path) + }, } } diff --git a/bundle/deploy/lock/workspace_filesystem.go b/bundle/deploy/lock/workspace_filesystem.go index 19a5f4c952f..290d5866ae6 100644 --- a/bundle/deploy/lock/workspace_filesystem.go +++ b/bundle/deploy/lock/workspace_filesystem.go @@ -5,8 +5,7 @@ import ( "errors" "io/fs" - "github.com/databricks/cli/bundle" - "github.com/databricks/cli/bundle/permissions" + "github.com/databricks/cli/libs/diag" "github.com/databricks/cli/libs/locker" "github.com/databricks/cli/libs/log" "github.com/databricks/databricks-sdk-go" @@ -14,7 +13,7 @@ import ( // workspaceFilesystemLock implements DeploymentLock using a lock file in the // bundle's workspace state path. Holds only the primitives it needs from the -// bundle plus a reference for the permission-error path. +// bundle. type workspaceFilesystemLock struct { client *databricks.WorkspaceClient user string @@ -22,9 +21,10 @@ type workspaceFilesystemLock struct { enabled bool force bool - // b is retained only for permissions.ReportPossiblePermissionDenied on - // the Acquire error path. - b *bundle.Bundle + // reportPermissionError produces the user-facing permission diagnostic + // when the workspace API returns ErrPermission/ErrNotExist from Lock. + // Lifted to a callback so this struct does not pin a *bundle.Bundle. + reportPermissionError func(ctx context.Context, path string) diag.Diagnostics locker *locker.Locker goal Goal @@ -50,8 +50,7 @@ func (l *workspaceFilesystemLock) Acquire(ctx context.Context) error { // If we get a permission or "doesn't exist" error from the API this // indicates we either don't have permissions or the path is invalid. if errors.Is(err, fs.ErrPermission) || errors.Is(err, fs.ErrNotExist) { - diags := permissions.ReportPossiblePermissionDenied(ctx, l.b, l.statePath) - return diags.Error() + return l.reportPermissionError(ctx, l.statePath).Error() } return err From 209c9a4a0e6f4e822a88c7fc8cfd5f8a092f02df Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 28 May 2026 11:17:45 +0000 Subject: [PATCH 08/10] acceptance/pipelines: drop duplicate "Error: Failed to acquire deployment lock" goldens Earlier in this PR we removed the log.Errorf wrapper in workspaceFilesystemLock because the same error is already surfaced via the returned diag. That removed the duplicate "Error: Failed to acquire deployment lock: ..." line from the CLI's stderr output, which two force-lock acceptance goldens were still expecting. Update both goldens to match the single-line shape. Co-authored-by: Isaac --- acceptance/pipelines/deploy/force-lock/output.txt | 3 --- acceptance/pipelines/destroy/force-lock/output.txt | 3 --- 2 files changed, 6 deletions(-) diff --git a/acceptance/pipelines/deploy/force-lock/output.txt b/acceptance/pipelines/deploy/force-lock/output.txt index b1092d3f523..b885965a6d2 100644 --- a/acceptance/pipelines/deploy/force-lock/output.txt +++ b/acceptance/pipelines/deploy/force-lock/output.txt @@ -4,9 +4,6 @@ === test deployment without force-lock (should fail) >>> errcode [CLI] pipelines deploy -Error: Failed to acquire deployment lock: deploy lock acquired by user-with-lock@databricks.com at [TIMESTAMP] +0000 UTC. -Another deployment may be in progress. Use --force-lock to override, but this may -conflict with the other deployment if it is still active Error: deploy lock acquired by user-with-lock@databricks.com at [TIMESTAMP] +0000 UTC. Another deployment may be in progress. Use --force-lock to override, but this may conflict with the other deployment if it is still active diff --git a/acceptance/pipelines/destroy/force-lock/output.txt b/acceptance/pipelines/destroy/force-lock/output.txt index 5579565b06f..24a60bb8f3f 100644 --- a/acceptance/pipelines/destroy/force-lock/output.txt +++ b/acceptance/pipelines/destroy/force-lock/output.txt @@ -11,9 +11,6 @@ View your pipeline foo here: [DATABRICKS_URL]/pipelines/[UUID]?o=[NUMID] === test deployment without force-lock (should fail) >>> errcode [CLI] pipelines destroy --auto-approve -Error: Failed to acquire deployment lock: deploy lock acquired by user-with-lock@databricks.com at [TIMESTAMP] +0000 UTC. -Another deployment may be in progress. Use --force-lock to override, but this may -conflict with the other deployment if it is still active Error: deploy lock acquired by user-with-lock@databricks.com at [TIMESTAMP] +0000 UTC. Another deployment may be in progress. Use --force-lock to override, but this may conflict with the other deployment if it is still active From e5c90cbfc4fb263e7c3990617849af68755251aa Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 28 May 2026 11:35:51 +0000 Subject: [PATCH 09/10] Revert: restore "Failed to acquire deployment lock" log line + goldens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The log.Errorf in workspaceFilesystemLock.Acquire isn't redundant with the downstream logdiag.LogError — it emits a separate user-visible "Error: Failed to acquire deployment lock: ..." stderr line that the force-lock acceptance goldens depend on. Removing it (commit 0433a83a5) was a real behavior change; this PR is supposed to be a pure refactor. Restore the log call and the two golden files. Co-authored-by: Isaac --- acceptance/pipelines/deploy/force-lock/output.txt | 3 +++ acceptance/pipelines/destroy/force-lock/output.txt | 3 +++ bundle/deploy/lock/workspace_filesystem.go | 2 ++ 3 files changed, 8 insertions(+) diff --git a/acceptance/pipelines/deploy/force-lock/output.txt b/acceptance/pipelines/deploy/force-lock/output.txt index b885965a6d2..b1092d3f523 100644 --- a/acceptance/pipelines/deploy/force-lock/output.txt +++ b/acceptance/pipelines/deploy/force-lock/output.txt @@ -4,6 +4,9 @@ === test deployment without force-lock (should fail) >>> errcode [CLI] pipelines deploy +Error: Failed to acquire deployment lock: deploy lock acquired by user-with-lock@databricks.com at [TIMESTAMP] +0000 UTC. +Another deployment may be in progress. Use --force-lock to override, but this may +conflict with the other deployment if it is still active Error: deploy lock acquired by user-with-lock@databricks.com at [TIMESTAMP] +0000 UTC. Another deployment may be in progress. Use --force-lock to override, but this may conflict with the other deployment if it is still active diff --git a/acceptance/pipelines/destroy/force-lock/output.txt b/acceptance/pipelines/destroy/force-lock/output.txt index 24a60bb8f3f..5579565b06f 100644 --- a/acceptance/pipelines/destroy/force-lock/output.txt +++ b/acceptance/pipelines/destroy/force-lock/output.txt @@ -11,6 +11,9 @@ View your pipeline foo here: [DATABRICKS_URL]/pipelines/[UUID]?o=[NUMID] === test deployment without force-lock (should fail) >>> errcode [CLI] pipelines destroy --auto-approve +Error: Failed to acquire deployment lock: deploy lock acquired by user-with-lock@databricks.com at [TIMESTAMP] +0000 UTC. +Another deployment may be in progress. Use --force-lock to override, but this may +conflict with the other deployment if it is still active Error: deploy lock acquired by user-with-lock@databricks.com at [TIMESTAMP] +0000 UTC. Another deployment may be in progress. Use --force-lock to override, but this may conflict with the other deployment if it is still active diff --git a/bundle/deploy/lock/workspace_filesystem.go b/bundle/deploy/lock/workspace_filesystem.go index 290d5866ae6..07da42558f3 100644 --- a/bundle/deploy/lock/workspace_filesystem.go +++ b/bundle/deploy/lock/workspace_filesystem.go @@ -47,6 +47,8 @@ func (l *workspaceFilesystemLock) Acquire(ctx context.Context) error { log.Infof(ctx, "Acquiring deployment lock (force: %v)", l.force) err = lk.Lock(ctx, l.force) if err != nil { + log.Errorf(ctx, "Failed to acquire deployment lock: %v", err) + // If we get a permission or "doesn't exist" error from the API this // indicates we either don't have permissions or the path is invalid. if errors.Is(err, fs.ErrPermission) || errors.Is(err, fs.ErrNotExist) { From cab0dfe422e8ea3a87008cfc009df310215cdd01 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Thu, 28 May 2026 12:02:55 +0000 Subject: [PATCH 10/10] bundle/deploy/lock: only init WorkspaceClient when locking is enabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original acquire.go mutator never called b.WorkspaceClient(ctx) when lock.enabled was false — the disabled check returned early in Apply before init() ran. After moving primitives to construction time the client was being eagerly initialized regardless, which adds a network call for the common dev-mode case where locking is disabled. Gate the client init on the captured enabled flag. Co-authored-by: Isaac --- bundle/deploy/lock/lock.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/bundle/deploy/lock/lock.go b/bundle/deploy/lock/lock.go index bd49a4fa48a..28a250d0e42 100644 --- a/bundle/deploy/lock/lock.go +++ b/bundle/deploy/lock/lock.go @@ -38,17 +38,22 @@ type DeploymentLock interface { // NewDeploymentLock returns a DeploymentLock backed by the workspace // filesystem. Captures everything the lock needs from the bundle at // construction time so the lock implementation itself does not retain a -// *bundle.Bundle reference. +// *bundle.Bundle reference. The workspace client is only initialized when +// locking is enabled to match the original lazy-init behavior. func NewDeploymentLock(ctx context.Context, b *bundle.Bundle, goal Goal) DeploymentLock { - return &workspaceFilesystemLock{ - client: b.WorkspaceClient(ctx), + enabled := b.Config.Bundle.Deployment.Lock.IsEnabled() + l := &workspaceFilesystemLock{ user: b.Config.Workspace.CurrentUser.UserName, statePath: b.Config.Workspace.StatePath, - enabled: b.Config.Bundle.Deployment.Lock.IsEnabled(), + enabled: enabled, force: b.Config.Bundle.Deployment.Lock.Force, goal: goal, reportPermissionError: func(ctx context.Context, path string) diag.Diagnostics { return permissions.ReportPossiblePermissionDenied(ctx, b, path) }, } + if enabled { + l.client = b.WorkspaceClient(ctx) + } + return l }