From 475141fdd78214d7e1b2125e2f26cb7b69fab683 Mon Sep 17 00:00:00 2001
From: Dominic Lewis <dominic.x.lewis@gmail.com>
Date: Wed, 18 Mar 2026 19:29:51 -0400
Subject: [PATCH] fix(acp): add per-command timeout and remove REDIS_URL
 fallback

C-1: sendCommand now races against a 10s per-command timeout so a Redis
instance that accepts the TCP connection but then stalls will reject and
flow into the existing fail-closed path instead of hanging dispatch
indefinitely.

C-3: resolveRedisLockUrl no longer falls back to REDIS_URL. ACP Redis
locking now activates only when OPENCLAW_ACP_SESSION_LOCK_REDIS_URL is
explicitly set, preventing unrelated Redis deployments from being
silently opted into fail-closed ACP locking.

Tests: added two cases to session-lock-manager.test.ts confirming that
REDIS_URL alone selects the local backend, and that
OPENCLAW_ACP_SESSION_LOCK_REDIS_URL takes precedence and triggers
fail-closed when the URL is invalid.
---
 .../reply/session-lock-manager.test.ts        | 21 +++++++++++++++
 src/auto-reply/reply/session-lock-manager.ts  | 26 ++++++++++---------
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/src/auto-reply/reply/session-lock-manager.test.ts b/src/auto-reply/reply/session-lock-manager.test.ts
index cf13bbc8065..2ae2696f809 100644
--- a/src/auto-reply/reply/session-lock-manager.test.ts
+++ b/src/auto-reply/reply/session-lock-manager.test.ts
@@ -195,4 +195,25 @@ describe("session lock manager selection", () => {
     expect(resolveAcpSessionLockTtlMs({ OPENCLAW_ACP_SESSION_LOCK_TTL_MS: "nope" })).toBe(120_000);
     expect(resolveAcpSessionLockTtlMs({ OPENCLAW_ACP_SESSION_LOCK_TTL_MS: "2000" })).toBe(2_000);
   });
+
+  // C-3: REDIS_URL fallback removed — generic REDIS_URL must not activate Redis-backed locking
+  it("does not activate Redis backend when only REDIS_URL is set", () => {
+    const manager = getAcpSessionLockManager({ REDIS_URL: "redis://localhost:6379" });
+    expect(manager).toBeInstanceOf(LocalSessionLockManager);
+  });
+
+  it("activates Redis backend only when OPENCLAW_ACP_SESSION_LOCK_REDIS_URL is set", () => {
+    const manager = getAcpSessionLockManager({
+      REDIS_URL: "redis://localhost:6379",
+      OPENCLAW_ACP_SESSION_LOCK_REDIS_URL: "http://redis.example",
+    });
+    // init will fail (bad scheme) but it must not fall back to local — must be fail-closed
+    expect(manager).not.toBeInstanceOf(LocalSessionLockManager);
+  });
 });
+
+// C-1: sendCommand per-command timeout lives inside RedisSocketConnection, which is private
+// and only reachable via createRedisCommandRunner (real TCP path). That path is not exercised
+// in unit tests because RedisSessionLockManager accepts a runRedisCommand injection that
+// bypasses socket I/O entirely. The Promise.race timeout is covered by code inspection; a
+// network-level integration test would require a real (or mock) TCP server.
diff --git a/src/auto-reply/reply/session-lock-manager.ts b/src/auto-reply/reply/session-lock-manager.ts
index 36388e0bc9f..98d804019f0 100644
--- a/src/auto-reply/reply/session-lock-manager.ts
+++ b/src/auto-reply/reply/session-lock-manager.ts
@@ -63,7 +63,7 @@ class RedisSocketConnection {
     });
   }
 
-  async sendCommand(args: string[]): Promise<RedisReply> {
+  async sendCommand(args: string[], timeoutMs = 10_000): Promise<RedisReply> {
     if (this.pending) {
       throw new Error("Redis command pipelining is not supported by this connection.");
     }
@@ -77,9 +77,17 @@ class RedisSocketConnection {
       }
       return parsed.value;
     }
-    return await new Promise<RedisReply>((resolve, reject) => {
-      this.pending = { resolve, reject };
-    });
+    return await Promise.race([
+      new Promise<RedisReply>((resolve, reject) => {
+        this.pending = { resolve, reject };
+      }),
+      new Promise<never>((_, reject) => {
+        setTimeout(
+          () => reject(new Error(`Redis command timed out after ${timeoutMs}ms.`)),
+          timeoutMs,
+        );
+      }),
+    ]);
   }
 
   close(): void {
@@ -458,14 +466,8 @@ export function resolveAcpSessionLockTtlMs(env: NodeJS.ProcessEnv = process.env)
 }
 
 function resolveRedisLockUrl(env: NodeJS.ProcessEnv = process.env): string | null {
-  const candidates = [env.OPENCLAW_ACP_SESSION_LOCK_REDIS_URL, env.REDIS_URL];
-  for (const candidate of candidates) {
-    const normalized = candidate?.trim();
-    if (normalized) {
-      return normalized;
-    }
-  }
-  return null;
+  const normalized = env.OPENCLAW_ACP_SESSION_LOCK_REDIS_URL?.trim();
+  return normalized || null;
 }
 
 let ACP_SESSION_LOCK_MANAGER_SINGLETON: SessionLockManager | null = null;