Ayuriel/openclaw
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(acp): add per-command timeout and remove REDIS_URL fallback

Browse Source 
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.
This commit is contained in:
Dominic Lewis 2026-03-18 19:29:51 -04:00
parent 39d3890278
commit 475141fdd7
2 changed files with 35 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
src/auto-reply/reply/session-lock-manager.test.ts
View File

@ -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.

26
src/auto-reply/reply/session-lock-manager.ts
View File

@ -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;