fix(cli): make the deploy lock all-or-nothing across hosts and expand its metadata

changelog.d/deploy-lock-correctness.fixed.md

@@ -0,0 +1,2 @@
+- `wheels deploy` lock acquisition is now all-or-nothing across the fleet: the lock is acquired on every (deduped) host sequentially in config order with failures surfaced, already-acquired locks are rolled back on a partial failure (`Wheels.Deploy.LockAcquireFailed` names the contended host; the contended host's own lock is never touched), and release fans out to every acquired host. Previously the first-success-wins `onAny` dispatch swallowed contention on one host and silently re-acquired on another, so concurrent deploys were only mutually excluded on single-host configs — and release could target a different host than acquire, stranding stale locks. The manual `wheels deploy lock acquire/release/status` verbs follow the same fleet-wide semantics (#2957)
+- Deploy lock metadata now actually expands `$(hostname)` and `$(date --iso-8601=seconds)` on the remote: the symlink target double-quotes the substitution segment while keeping the user and message inert via `shellEscape` single-quoting — previously the whole target was single-quoted, which suppressed command substitution and recorded the literal `$(hostname)` text (#2957)

cli/lucli/services/deploy/cli/DeployLockCli.cfc

@@ -26,7 +26,10 @@ component {
             user: $currentUser(),
             message: arguments.opts.message ?: "manual acquire"
         });
-        $dispatchAny($allHosts(cfg), cmd, dryRun);
+        // The deploy flow holds the lock on EVERY host (##2957 DEP-1), so a
+        // manual acquire must match — locking a single host would let a
+        // concurrent deploy that probes another host proceed.
+        $acquireLockAllOrNothing($uniqueHosts($allHosts(cfg)), cmd, lock, dryRun);
         return $renderResult(arguments.opts, "Acquired deploy lock for " & cfg.service());
     }
 
@@ -37,7 +40,9 @@ component {
         var lock = new modules.wheels.services.deploy.commands.LockCommands(cfg);
         // rm -f is idempotent; surfacing a failure here only obscures the
         // operator's intent ("clear the lock if it's there"). #2696.
-        $dispatchAny($allHosts(cfg), lock.release(), dryRun, true);
+        // Fan out to every host — the lock lives fleet-wide (##2957 DEP-1),
+        // so clearing one host would strand stale locks on the rest.
+        $dispatch($uniqueHosts($allHosts(cfg)), lock.release(), dryRun, true);
         return $renderResult(arguments.opts, "Released deploy lock for " & cfg.service());
     }
 
@@ -48,8 +53,9 @@ component {
         var lock = new modules.wheels.services.deploy.commands.LockCommands(cfg);
         // readlink exits nonzero when the lock file is missing — which is
         // exactly what the operator wants to learn from `status`. Treat that
-        // as advisory output, not a thrown error. #2696.
-        $dispatchAny($allHosts(cfg), lock.status(), dryRun, true);
+        // as advisory output, not a thrown error. #2696. Checked on every
+        // host since the lock lives fleet-wide (##2957 DEP-1).
+        $dispatch($uniqueHosts($allHosts(cfg)), lock.status(), dryRun, true);
         return $renderResult(arguments.opts, "Checked deploy lock status for " & cfg.service());
     }
 
@@ -73,18 +79,90 @@ component {
         return out;
     }
 
-    private void function $dispatchAny(required array hosts, required string cmd, required boolean dryRun, boolean allowFail = false) {
+    /**
+     * All-or-nothing lock acquisition across every host, in config order,
+     * with rollback of already-acquired locks on the first failure.
+     *
+     * MIRROR: DeployMainCli.$acquireLockAllOrNothing is the deploy-flow
+     * twin of this contract (##2957 DEP-1) — keep them in lockstep.
+     */
+    private void function $acquireLockAllOrNothing(
+        required array hosts,
+        required string acquireCmd,
+        required any lock,
+        required boolean dryRun
+    ) {
         if (arguments.dryRun) {
-            if (arrayLen(arguments.hosts)) {
-                arrayAppend(variables.dryRunBuffer, "[" & arguments.hosts[1] & "] " & arguments.cmd);
+            for (var h in arguments.hosts) {
+                arrayAppend(variables.dryRunBuffer, "[" & h & "] " & arguments.acquireCmd);
+            }
+            return;
+        }
+        var c = arguments.acquireCmd;
+        // Shared struct so the callback can record progress — closures can't
+        // reliably mutate outer scalars across engines (anti-pattern ##10).
+        var state = {acquired: [], lastHost: ""};
+        try {
+            variables.sshPool.sequential(arguments.hosts, function(ssh, host) {
+                state.lastHost = host;
+                ssh.run(c, {raise: true});
+                arrayAppend(state.acquired, host);
+            });
+        } catch (any e) {
+            $rollbackAcquiredLocks(state.acquired, arguments.lock);
+            throw(
+                type = "Wheels.Deploy.LockAcquireFailed",
+                message = "Could not acquire the deploy lock on " & state.lastHost
+                    & " — another deploy may hold it. Rolled back "
+                    & arrayLen(state.acquired) & " already-acquired lock(s). "
+                    & "Inspect with 'wheels deploy lock status'; clear a stale lock with "
+                    & "'wheels deploy lock release'. Cause: " & e.message,
+                detail = e.detail ?: ""
+            );
+        }
+    }
+
+    /**
+     * Best-effort release of the locks a partially-failed acquire already
+     * placed. A rollback failure must never shadow the LockAcquireFailed
+     * the caller is about to throw.
+     */
+    private void function $rollbackAcquiredLocks(required array hosts, required any lock) {
+        if (!arrayLen(arguments.hosts)) return;
+        var releaseCmd = arguments.lock.release();
+        try {
+            variables.sshPool.onEach(arguments.hosts, function(ssh, host) {
+                ssh.run(releaseCmd, {raise: false});
+            });
+        } catch (any e) {
+            // Swallowed deliberately — the acquire error is the one the
+            // operator needs to see.
+        }
+    }
+
+    /** Order-preserving dedupe — a host serving several roles appears once. */
+    private array function $uniqueHosts(required array hosts) {
+        var seen = {};
+        var out = [];
+        for (var h in arguments.hosts) {
+            if (!structKeyExists(seen, h)) {
+                seen[h] = true;
+                arrayAppend(out, h);
+            }
+        }
+        return out;
+    }
+
+    private void function $dispatch(required array hosts, required string cmd, required boolean dryRun, boolean allowFail = false) {
+        if (arguments.dryRun) {
+            for (var h in arguments.hosts) {
+                arrayAppend(variables.dryRunBuffer, "[" & h & "] " & arguments.cmd);
             }
             return;
         }
-        // Lock ops target just one host (the lock file lives on one path; any host works).
-        // #2696: acquire stays strict (contention should surface); release/status tolerate.
         var c = arguments.cmd;
         var doRaise = !arguments.allowFail;
-        variables.sshPool.onAny(arguments.hosts, function(ssh, host) { ssh.run(c, {raise: doRaise}); });
+        variables.sshPool.onEach(arguments.hosts, function(ssh, host) { ssh.run(c, {raise: doRaise}); });
     }
 
     private string function $currentUser() {

cli/lucli/services/deploy/cli/DeployMainCli.cfc

@@ -84,10 +84,16 @@ component {
 
         $fireHook(hooks, "pre-deploy", hookEnv, dryRun);
 
+        // The lock lives on EVERY host (deduped — a host serving two roles
+        // must not contend with itself) so a concurrent deploy collides with
+        // it no matter which host it probes (##2957 DEP-1).
+        var lockHosts = $uniqueHosts(hosts);
+
         try {
-            $dispatchAny(
-                hosts,
+            $acquireLockAllOrNothing(
+                lockHosts,
                 lock.acquire({user: $currentUser(), message: "deploy " & ver}),
+                lock,
                 dryRun
             );
 
@@ -110,7 +116,9 @@ component {
                 // original deploy exception inside this finally block. rm -f
                 // is idempotent; if it genuinely fails on a remote, the deploy
                 // already has a real error to surface from the try body.
-                $dispatchAny(hosts, lock.release(), dryRun, true);
+                // Release fans out to the exact hosts the lock was acquired
+                // on, so no host is left holding a stale lock (##2957 DEP-1b).
+                $dispatch(lockHosts, lock.release(), dryRun, true);
             }
 
             hookEnv.KAMAL_RUNTIME = int((getTickCount() - deployStart) / 1000);
@@ -449,10 +457,97 @@ component {
         });
     }
 
+    /**
+     * All-or-nothing deploy-lock acquisition (##2957 DEP-1).
+     *
+     * The lock only provides mutual exclusion if a concurrent deploy is
+     * guaranteed to collide with it. The old $dispatchAny acquire was
+     * first-success-wins: contention on host 1 was swallowed by
+     * SshPool.onAny and a fresh lock was acquired on host 2, so two deploys
+     * could run side by side on any multi-host fleet. Instead: acquire on
+     * EVERY host, in deterministic (config) order, sequentially — two
+     * concurrent deploys probe hosts in the same order, so exactly one wins
+     * the first host and the other aborts there. On a partial failure, roll
+     * back ONLY the locks already acquired (the contended host's lock
+     * belongs to the other deploy) and surface the per-host error.
+     *
+     * MIRROR: DeployLockCli.$acquireLockAllOrNothing implements the same
+     * contract for the manual lock verbs — keep them in lockstep.
+     */
+    private void function $acquireLockAllOrNothing(
+        required array hosts,
+        required string acquireCmd,
+        required any lock,
+        required boolean dryRun
+    ) {
+        if (arguments.dryRun) {
+            for (var h in arguments.hosts) {
+                arrayAppend(variables.dryRunBuffer, "[" & h & "] " & arguments.acquireCmd);
+            }
+            return;
+        }
+        var c = arguments.acquireCmd;
+        // Shared struct so the callback can record progress — closures can't
+        // reliably mutate outer scalars across engines (anti-pattern ##10).
+        var state = {acquired: [], lastHost: ""};
+        try {
+            variables.sshPool.sequential(arguments.hosts, function(ssh, host) {
+                state.lastHost = host;
+                ssh.run(c, {raise: true});
+                arrayAppend(state.acquired, host);
+            });
+        } catch (any e) {
+            $rollbackAcquiredLocks(state.acquired, arguments.lock);
+            throw(
+                type = "Wheels.Deploy.LockAcquireFailed",
+                message = "Could not acquire the deploy lock on " & state.lastHost
+                    & " — another deploy may hold it. Rolled back "
+                    & arrayLen(state.acquired) & " already-acquired lock(s). "
+                    & "Inspect with 'wheels deploy lock status'; clear a stale lock with "
+                    & "'wheels deploy lock release'. Cause: " & e.message,
+                detail = e.detail ?: ""
+            );
+        }
+    }
+
+    /**
+     * Best-effort release of the locks a partially-failed acquire already
+     * placed. A rollback failure must never shadow the LockAcquireFailed
+     * the caller is about to throw.
+     */
+    private void function $rollbackAcquiredLocks(required array hosts, required any lock) {
+        if (!arrayLen(arguments.hosts)) return;
+        var releaseCmd = arguments.lock.release();
+        try {
+            variables.sshPool.onEach(arguments.hosts, function(ssh, host) {
+                ssh.run(releaseCmd, {raise: false});
+            });
+        } catch (any e) {
+            // Swallowed deliberately — the acquire error is the one the
+            // operator needs to see; stale locks are recoverable via
+            // `wheels deploy lock release`.
+        }
+    }
+
+    /** Order-preserving dedupe — a host serving several roles appears once. */
+    private array function $uniqueHosts(required array hosts) {
+        var seen = {};
+        var out = [];
+        for (var h in arguments.hosts) {
+            if (!structKeyExists(seen, h)) {
+                seen[h] = true;
+                arrayAppend(out, h);
+            }
+        }
+        return out;
+    }
+
     /**
      * Dispatch a single command to "any one" host — used for operations
-     * that only need to happen once across the fleet (lock acquire/release,
-     * proxy boot check). FakeSshPool.onAny records exactly one call.
+     * that only need to happen once across the fleet (proxy boot check).
+     * NOT suitable for the deploy lock: onAny swallows per-host failures,
+     * which is exactly the multi-host lock bypass fixed in ##2957 DEP-1.
+     * FakeSshPool.onAny records exactly one call.
      */
     private void function $dispatchAny(
         required array hosts,

cli/lucli/services/deploy/commands/LockCommands.cfc

@@ -29,14 +29,17 @@ component extends="Base" {
     public string function acquire(required struct opts) {
         var user = arguments.opts.user ?: "unknown";
         var message = arguments.opts.message ?: "";
-        // $(hostname) and $(date ...) resolved by the remote shell; user and
-        // message are escaped by surrounding the whole target in single
-        // quotes so shell metacharacters don't blow up the ln command.
-        // Inner single quotes in either value are closed and re-opened
-        // ( "'\''").
-        var safeUser = replace(user, "'", "'\''", "all");
-        var safeMessage = replace(message, "'", "'\''", "all");
-        var target = "'" & safeUser & "@$(hostname)/$(date --iso-8601=seconds)/" & safeMessage & "'";
+        // The symlink target is three concatenated shell words: the single-
+        // quoted (inert) user, a double-quoted middle segment whose
+        // $(hostname) and $(date ...) ARE resolved by the remote shell, and
+        // the single-quoted (inert) message. Adjacent quoted segments join
+        // into one argument, so metacharacters in user/message can't execute
+        // while the metadata substitutions still expand. Previously the whole
+        // target was single-quoted, which suppressed command substitution and
+        // recorded the literal "$(hostname)" text (##2957 DEP-10).
+        var target = shellEscape(user)
+            & """@$(hostname)/$(date --iso-8601=seconds)/"""
+            & shellEscape(message);
         return "ln -s " & target & " " & lockPath();
     }
 

cli/lucli/tests/_fixtures/deploy/configs/multi-host.yml

@@ -0,0 +1,9 @@
+service: demo
+image: acme/demo
+servers:
+  - 10.0.0.1
+  - 10.0.0.2
+registry:
+  username: demo
+  password:
+    - REGISTRY_PASSWORD

cli/lucli/tests/_fixtures/deploy/configs/shared-host.yml

@@ -0,0 +1,11 @@
+service: demo
+image: acme/demo
+servers:
+  web:
+    - 10.0.0.5
+  job:
+    - 10.0.0.5
+registry:
+  username: demo
+  password:
+    - REGISTRY_PASSWORD