fix: eliminate Redis TOCTOU race on step cache ops

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- **Redis TOCTOU safety** (#71): step cache and placeholder Redis ops use call-time client resolution with try/catch helpers (`redisHGetAll` / `redisHSet` / `redisExpire`), so a disconnect or `resetRedis()` between null-check and use no longer crashes step execution. `resetRedis()` now clears the module reference before disconnecting.
+
 ### Added
 
 - **OpenCode Zen gateway support**: set `gateway: "opencodezen"` in `configure()` and provide `OPENCODEZEN_API_KEY` to route all model requests through [OpenCode Zen](https://opencode.ai/docs/ko/zen/) (`https://opencode.ai/zen/v1`), an OpenAI-compatible gateway with 30+ curated models including Claude, Gemini, GPT, Qwen, and more.

src/__tests__/data-cache.test.ts

@@ -1,7 +1,15 @@
 import { describe, it, expect, vi, beforeEach } from "vitest";
 
 vi.mock("../redis", () => ({
-  getRedis: () => ({ hgetall: vi.fn(), hset: vi.fn(), expire: vi.fn() }),
+  getRedis: () => ({
+    hgetall: vi.fn().mockResolvedValue({}),
+    hset: vi.fn().mockResolvedValue("OK"),
+    expire: vi.fn().mockResolvedValue(1),
+  }),
+  resetRedis: vi.fn(),
+  redisHGetAll: vi.fn().mockResolvedValue({}),
+  redisHSet: vi.fn().mockResolvedValue(true),
+  redisExpire: vi.fn().mockResolvedValue(true),
 }));
 
 vi.mock("../email", () => ({

src/__tests__/integration/run-steps.test.ts

@@ -6,14 +6,39 @@ vi.mock("../../instrumentation", () => ({
   initTelemetry: vi.fn(),
 }));
 
-// Mock Redis
+// Mock Redis client + safe helpers (helpers must be mocked: they close over getRedis
+// inside the redis module, so overriding only the getRedis export is not enough).
 const mockRedis = {
   hgetall: vi.fn().mockResolvedValue({}),
   hset: vi.fn().mockResolvedValue("OK"),
   expire: vi.fn().mockResolvedValue(1),
 };
 vi.mock("../../redis", () => ({
   getRedis: () => mockRedis,
+  resetRedis: vi.fn(),
+  redisHGetAll: async (key: string) => {
+    try {
+      return (await mockRedis.hgetall(key)) ?? {};
+    } catch {
+      return {};
+    }
+  },
+  redisHSet: async (key: string, data: Record<string, string>) => {
+    try {
+      await mockRedis.hset(key, data);
+      return true;
+    } catch {
+      return false;
+    }
+  },
+  redisExpire: async (key: string, seconds: number) => {
+    try {
+      await mockRedis.expire(key, seconds);
+      return true;
+    } catch {
+      return false;
+    }
+  },
 }));
 
 // Mock AI SDK

src/__tests__/redis.test.ts

@@ -0,0 +1,110 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
+
+const hgetall = vi.fn();
+const hset = vi.fn();
+const expire = vi.fn();
+const disconnect = vi.fn();
+
+vi.mock("ioredis", () => {
+  class MockRedis {
+    hgetall = hgetall;
+    hset = hset;
+    expire = expire;
+    disconnect = disconnect;
+  }
+  return { default: MockRedis };
+});
+
+vi.mock("../config", () => ({
+  getConfig: vi.fn(() => ({ redis: { url: "redis://localhost:6379" } })),
+}));
+
+vi.mock("../logger", () => ({
+  logger: {
+    warn: vi.fn(),
+    debug: vi.fn(),
+    error: vi.fn(),
+    info: vi.fn(),
+  },
+}));
+
+import {
+  getRedis,
+  resetRedis,
+  redisHGetAll,
+  redisHSet,
+  redisExpire,
+} from "../redis";
+import { getConfig } from "../config";
+import { logger } from "../logger";
+
+describe("redis safe helpers (TOCTOU / error resilience)", () => {
+  beforeEach(() => {
+    resetRedis();
+    hgetall.mockReset();
+    hset.mockReset();
+    expire.mockReset();
+    disconnect.mockReset();
+    vi.mocked(logger.warn).mockClear();
+    vi.mocked(getConfig).mockReturnValue({
+      redis: { url: "redis://localhost:6379" },
+    } as ReturnType<typeof getConfig>);
+  });
+
+  afterEach(() => {
+    resetRedis();
+  });
+
+  it("redisHGetAll returns hash data on success", async () => {
+    hgetall.mockResolvedValue({ locator: "#btn", action: "click" });
+    const result = await redisHGetAll("step:flow:click");
+    expect(result).toEqual({ locator: "#btn", action: "click" });
+    expect(hgetall).toHaveBeenCalledWith("step:flow:click");
+  });
+
+  it("redisHGetAll returns {} and does not throw when hgetall fails", async () => {
+    hgetall.mockRejectedValue(new Error("Connection is closed"));
+    await expect(redisHGetAll("step:flow:x")).resolves.toEqual({});
+    expect(logger.warn).toHaveBeenCalled();
+  });
+
+  it("redisHGetAll returns {} when Redis is not configured", async () => {
+    vi.mocked(getConfig).mockReturnValue({} as ReturnType<typeof getConfig>);
+    resetRedis();
+    await expect(redisHGetAll("k")).resolves.toEqual({});
+    expect(hgetall).not.toHaveBeenCalled();
+  });
+
+  it("redisHSet returns true on success and false on failure without throwing", async () => {
+    hset.mockResolvedValue(1);
+    await expect(redisHSet("k", { a: "1" })).resolves.toBe(true);
+
+    hset.mockRejectedValue(new Error("READONLY"));
+    await expect(redisHSet("k", { a: "1" })).resolves.toBe(false);
+    expect(logger.warn).toHaveBeenCalled();
+  });
+
+  it("redisExpire returns false on failure without throwing", async () => {
+    expire.mockRejectedValue(new Error("timeout"));
+    await expect(redisExpire("k", 60)).resolves.toBe(false);
+  });
+
+  it("resetRedis nulls the client before disconnect (no dangling module ref)", async () => {
+    const client = getRedis();
+    expect(client).toBeTruthy();
+    resetRedis();
+    expect(disconnect).toHaveBeenCalled();
+    hgetall.mockResolvedValue({});
+    await expect(redisHGetAll("after-reset")).resolves.toEqual({});
+  });
+
+  it("safe helpers re-resolve client after resetRedis mid-flight (TOCTOU)", async () => {
+    hgetall.mockImplementation(async () => {
+      resetRedis();
+      return { ok: "1" };
+    });
+    await expect(redisHGetAll("k")).resolves.toEqual({ ok: "1" });
+    hgetall.mockResolvedValue({});
+    await expect(redisHGetAll("k2")).resolves.toEqual({});
+  });
+});

src/data-cache.ts

@@ -4,7 +4,7 @@ import { getConfig } from "./config";
 import { extractEmailContent } from "./email";
 import { GLOBAL_VALUES_TTL_SECONDS } from "./constants";
 import { logger } from "./logger";
-import { getRedis } from "./redis";
+import { redisHGetAll, redisHSet, redisExpire } from "./redis";
 import { Step } from "./types";
 import { generatePhoneNumber } from "./utils";
 
@@ -125,10 +125,8 @@ function getRedisKey(executionId: string): string {
 export async function getGlobalValues(
   executionId: string,
 ): Promise<Partial<GlobalPlaceholders> | null> {
-  const redis = getRedis();
-  if (!redis) return null;
   const key = getRedisKey(executionId);
-  const values = await redis.hgetall(key);
+  const values = await redisHGetAll(key);
 
   if (!values || Object.keys(values).length === 0) {
     return null;
@@ -145,16 +143,14 @@ export async function saveGlobalValues(
   executionId: string,
   values: GlobalPlaceholders,
 ): Promise<void> {
-  const redis = getRedis();
-  if (!redis) return;
-
   const key = getRedisKey(executionId);
 
-  // Save all values as a hash
-  await redis.hset(key, values);
+  // Save all values as a hash (safe helper: no TOCTOU / never throws)
+  const saved = await redisHSet(key, values);
+  if (!saved) return;
 
   // Set TTL
-  await redis.expire(key, GLOBAL_VALUES_TTL_SECONDS);
+  await redisExpire(key, GLOBAL_VALUES_TTL_SECONDS);
 
   logger.debug(`Saved global values to Redis for execution: ${executionId}`);
 }
@@ -175,10 +171,8 @@ function getProjectDataRedisKey(projectId: string): string {
  * Returns an empty object if no data exists.
  */
 export async function getProjectData(projectId: string): Promise<ProjectDataPlaceholders> {
-  const redis = getRedis();
-  if (!redis) return {};
   const key = getProjectDataRedisKey(projectId);
-  const values = await redis.hgetall(key);
+  const values = await redisHGetAll(key);
 
   if (!values || Object.keys(values).length === 0) {
     return {};

src/index.ts

@@ -21,7 +21,7 @@ async function maybeWithSpan<T>(
 }
 import { z } from "zod";
 import { buildRunStepsPrompt, buildRunUserFlowPrompt } from "./prompts";
-import { getRedis } from "./redis";
+import { getRedis, redisHGetAll, redisHSet } from "./redis";
 import { getAItools } from "./tools";
 import { RunStepsOptions, UserFlowOptions } from "./types";
 import {
@@ -352,8 +352,8 @@ export const runSteps = async ({
       continue;
     }
 
-    // First check if the step is cached on redis
-    const cachedStep = redis ? await redis.hgetall(`step:${userFlow}:${step.description}`) : {};
+    // First check if the step is cached on redis (safe helper: no TOCTOU / never throws)
+    const cachedStep = await redisHGetAll(`step:${userFlow}:${step.description}`);
 
     if (
       !bypassCache &&
@@ -532,11 +532,13 @@ export const runSteps = async ({
         .flatMap((s) => s.toolCalls)
         .filter((tool) => ["browser_snapshot", "browser_stop"].indexOf(tool.toolName) === -1);
 
-      if (allToolCalls.length === 1 && redis) {
+      if (allToolCalls.length === 1) {
         const cacheData = getPendingCacheData();
         if (cacheData) {
-          await redis.hset(`step:${userFlow}:${step.description}`, cacheData);
-          logger.debug(`Cached step action: ${step.description}`);
+          const cached = await redisHSet(`step:${userFlow}:${step.description}`, cacheData);
+          if (cached) {
+            logger.debug(`Cached step action: ${step.description}`);
+          }
         }
       }
 

src/redis.ts

@@ -12,6 +12,11 @@ let initialized = false;
  *
  * Lazy: the connection is opened on first call so users can call `configure()`
  * before any Redis-dependent code path runs.
+ *
+ * Do not hold the returned client across `await` boundaries for long-lived work.
+ * Prefer {@link redisHGetAll}, {@link redisHSet}, and {@link redisExpire}, which
+ * re-resolve the client at call time and degrade gracefully on errors (avoids
+ * TOCTOU races with `resetRedis()` / disconnect mid-execution).
  */
 export function getRedis(): Redis | null {
   if (initialized) return client;
@@ -32,7 +37,61 @@ export function getRedis(): Redis | null {
 
 /** @internal Reset the memoized client. Used for testing only. */
 export function resetRedis() {
-  client?.disconnect();
+  // Null the module ref before disconnect so concurrent getRedis()/safe helpers
+  // cannot observe a half-dead client mid-teardown (TOCTOU with in-flight awaits).
+  const prev = client;
   client = null;
   initialized = false;
+  prev?.disconnect();
+}
+
+/**
+ * Safe HGETALL: resolves the client at call time and never throws.
+ * Returns {} when Redis is unavailable, reset mid-flight, or the command fails.
+ */
+export async function redisHGetAll(key: string): Promise<Record<string, string>> {
+  const redis = getRedis();
+  if (!redis) return {};
+  try {
+    const values = await redis.hgetall(key);
+    return values ?? {};
+  } catch (err) {
+    logger.warn({ err, key }, "Redis hgetall failed; treating cache as empty");
+    return {};
+  }
 }
+
+/**
+ * Safe HSET: resolves the client at call time and never throws.
+ * Returns true on success, false when Redis is unavailable or the command fails.
+ */
+export async function redisHSet(
+  key: string,
+  data: Record<string, string>,
+): Promise<boolean> {
+  const redis = getRedis();
+  if (!redis) return false;
+  try {
+    await redis.hset(key, data);
+    return true;
+  } catch (err) {
+    logger.warn({ err, key }, "Redis hset failed; cache write skipped");
+    return false;
+  }
+}
+
+/**
+ * Safe EXPIRE: resolves the client at call time and never throws.
+ * Returns true on success, false when Redis is unavailable or the command fails.
+ */
+export async function redisExpire(key: string, seconds: number): Promise<boolean> {
+  const redis = getRedis();
+  if (!redis) return false;
+  try {
+    await redis.expire(key, seconds);
+    return true;
+  } catch (err) {
+    logger.warn({ err, key }, "Redis expire failed; TTL not applied");
+    return false;
+  }
+}