fix(redis): keep the existing admission when a reservation refresh throws

The refresh extends the local reservation and the pointer as separate Redis
mutations, so an exception does not prove the slot went unrefreshed — and a
client-side command timeout can abandon a call the server still applied.
Re-admitting on that ambiguity spends another rate-limit token and can reject
a run that still holds a valid slot. Only a `false` return proves the
reservation is gone, so only that repeats admission.

Also corrects the timeout comments: the multi-second gap before a connection
becomes usable is the connect callback waiting on a saturated event loop, not
a slow handshake — the INFO round-trip that follows completes in ~10ms.

Author: waleedlatif1

apps/sim/background/async-preprocessing-correlation.test.ts

@@ -771,4 +771,49 @@ describe('async preprocessing correlation threading', () => {
       })
     )
   })
+
+  it('keeps the enqueuer admission when a reservation refresh throws', async () => {
+    // A throw is ambiguous: the refresh mutates the local reservation and the
+    // pointer separately, so it can fail after the slot's TTL was extended.
+    mockRefreshExecutionSlotExpiry.mockRejectedValueOnce(new Error('Command timed out'))
+    mockPreprocessExecution.mockResolvedValueOnce({
+      success: true,
+      actorUserId: 'actor-1',
+      workflowRecord: {
+        id: 'workflow-1',
+        userId: 'owner-1',
+        workspaceId: 'workspace-1',
+        variables: {},
+      },
+      billingAttribution,
+      executionTimeout: {},
+    })
+    mockExecuteWorkflowCore.mockResolvedValueOnce({
+      success: true,
+      status: 'success',
+      output: { ok: true },
+      metadata: { duration: 10, userId: 'actor-1' },
+    })
+
+    await expect(
+      executeWorkflowJob({
+        principal,
+        workflowId: 'workflow-1',
+        userId: 'actor-1',
+        workspaceId: 'workspace-1',
+        billingAttribution,
+        triggerType: 'api',
+        executionId: 'execution-refresh-throw',
+        requestId: 'request-refresh-throw',
+        admissionCompleted: true,
+        executionTimeoutMs: 60_000,
+      })
+    ).resolves.toEqual(expect.objectContaining({ success: true }))
+
+    // Re-admitting would spend another rate-limit token and could reject a run
+    // that still holds a valid slot.
+    expect(mockPreprocessExecution).toHaveBeenCalledWith(
+      expect.objectContaining({ checkRateLimit: false, skipUsageLimits: true })
+    )
+  })
 })

apps/sim/background/workflow-execution.ts

@@ -165,15 +165,6 @@ export async function executeWorkflowJob(
     const executionDeadlineAt = getExecutionDeadlineAt(timeoutController.signal)?.getTime()
     let admissionCompleted = payload.admissionCompleted === true
     if (admissionCompleted && executionDeadlineAt !== undefined) {
-      /**
-       * A refresh that *returns* false already degrades into repeating usage
-       * admission below. A refresh that *throws* — a Redis timeout on the first
-       * command this fresh worker process issues — used to kill the run here,
-       * before the logging session exists, so the execution left no log row at
-       * all and simply vanished from the workspace's logs. Route both outcomes
-       * into the same re-admission path: preprocessing owns the reservation
-       * retry, and any failure it hits is recorded against a live session.
-       */
       try {
         admissionCompleted = await refreshExecutionSlotExpiry(
           executionId,
@@ -186,8 +177,23 @@ export async function executeWorkflowJob(
           })
         }
       } catch (error) {
-        admissionCompleted = false
-        logger.warn('Reservation refresh failed; repeating usage admission', {
+        /**
+         * Only a `false` return proves the reservation is gone, and that is the
+         * one outcome re-admission is right for. A throw does not: the refresh
+         * is several separate Redis mutations, so it can fail after the slot's
+         * TTL was already extended, and a client-side command timeout abandons
+         * a call the server may still have applied. Re-admitting on that
+         * ambiguity would spend another rate-limit token and could reject a run
+         * that still holds a perfectly valid slot, so keep the admission the
+         * enqueuing surface already completed.
+         *
+         * Worst case the slot lapses before the run ends, which under-counts
+         * concurrency for this one execution; the later release is already a
+         * no-op when the reservation is gone. Previously this threw before the
+         * logging session existed, so the run left no log row at all and simply
+         * vanished from the workspace's logs.
+         */
+        logger.warn('Reservation refresh failed; continuing on the existing admission', {
           workflowId,
           executionId,
           error: toError(error).message,

apps/sim/lib/core/config/redis.ts

@@ -49,9 +49,13 @@ const REDIS_CONNECT_TIMEOUT_MS = 10_000
  * never received the command, with a stack containing only ioredis timer
  * frames.
  *
- * Production handshakes to ElastiCache measure 2-6s when several connections
- * are opened at once, so a 5s command timeout failed roughly 3% of cold-start
- * commands on Trigger.dev workers, which open a fresh connection per run.
+ * The gap between opening a connection and servicing its `connect` callback is
+ * measured in seconds during an initialization burst — not because the network
+ * or the server is slow (the `INFO` round-trip that follows completes in ~10ms,
+ * and the server sits near-idle), but because the callback cannot run while the
+ * main thread is saturated. This deadline is wall-clock, so it spans that delay
+ * whether the cause is the network or a busy event loop, which is exactly why it
+ * has to leave room beyond the connect budget.
  */
 export const REDIS_COMMAND_TIMEOUT_MS = 15_000
 
@@ -262,18 +266,20 @@ export function getRedisClient(): Redis | null {
  * issued: ioredis arms it in `sendCommand` before it checks whether the socket
  * is writable, so the budget covers handshake and offline-queue wait as well as
  * execution. A process whose first command lands on a cold client therefore
- * spends most of that budget on the TLS handshake, which measures 2-6s against
- * ElastiCache when several connections open at once.
+ * spends that budget waiting for the connection rather than running the command.
  *
- * Warming at process start moves that cost off the first command's clock, which
- * is the connection-reuse practice AWS recommends: establishing a TCP+TLS
- * connection is far more expensive than the commands that run over it, so it
- * should be paid once per process rather than once per unit of work.
+ * Warming separates the two: the wait becomes connection setup, bounded by
+ * `connectTimeout`, instead of eating a command's deadline. It also follows the
+ * connection-reuse practice AWS recommends — establishing a connection costs far
+ * more than the commands that run over it, so it belongs once per process rather
+ * than once per unit of work. Note this does not make the connection ready any
+ * sooner when the delay is a saturated event loop rather than the network; it
+ * only stops that delay from being charged to a command.
  *
  * Best effort by design — resolves rather than rejects on failure, and is
  * bounded by the connect budget, so neither a degraded Redis nor a missing
  * configuration can stop a process from starting. Callers that skip warming
- * still work; they just pay the handshake on their first command as before.
+ * still work; they just wait on their first command as before.
  *
  * Memoized per client: a warm process returns immediately, and a forced
  * reconnect clears it so the replacement connection is warmed in turn.