Skip to content

Commit 2be2670

Browse files
authored
fix(redis): size cold-connection waits to survive a dead handshake (#7764)
* fix(redis): size cold-connection waits to survive a dead handshake A dead Redis handshake costs two command deadlines, not one. ioredis sends CLIENT SETNAME/SETINFO on connect and dispatches the INFO ready check only once those settle — on a socket that never answers, they settle by timing out. Only then does the ready check start its own deadline, fail, and tear the socket down for retryStrategy to reconnect. Every readiness wait in the codebase was sized to a single deadline, so it expired while the first attempt was still being diagnosed and a configured retry could never be the thing that rescued it. Introduces `coldConnectionBudgetMs`, which derives a wait from the command deadline and the reconnect delays a caller's own retryStrategy would return, and states the 2x in one place. Both waits now derive from it: - The execution-signal subscriber's readiness budget was the same constant as its commandTimeout (5s and 5s), so ioredis's reconnect was decorative on the cold path that failed in production. It now has a 2s command deadline — this client only issues SUBSCRIBE/UNSUBSCRIBE, always after ready — and a budget with room for two dead attempts and a healthy one. - The shared client's warm-up budget was 10s against a 5s deadline, exactly the moment ioredis would first tear a stalled socket down, so on the case warming exists for it gave up just before recovery could land. Warms the execution-signal subscriber alongside the shared client at process start, in parallel, never throwing: it is the second connection a run opens and was paying its handshake inside its own readiness budget. The shared redis-config mock mirrors the pure budget helper, since consumers now evaluate it at module load and would otherwise fail to import under the global mock. * fix(redis): keep the subscriber's live command deadline at 5s ioredis has one `commandTimeout` for the handshake commands and every live command on the socket, so tightening it to diagnose dead handshakes faster also cut the initial SUBSCRIBE from 5s to 2s — on the one path where a rejection fails the whole run. A ready-but-slow server delays SUBSCRIBE too. Restore the 5s tolerance that subscribe has always had and derive the readiness budget from it: one dead handshake recovers in ~10.5s, so an 11.5s budget still lets ioredis's reconnect rescue the case that failed. The local test mock now delegates to the shared, drift-guarded mirror of the budget arithmetic so the derived number under test is the one production derives. * improvement(redis): warm the signal subscriber on intent, not at worker start Only the tasks that execute a workflow ever subscribe to cancellation signals, and they are a minority of runs: over a week, 71% of Trigger.dev runs never subscribe, and a single document-processing task is 61% on its own. Warming the subscriber in the global init hook opened a TLS connection on every one of those runs that nothing would use. Warm it instead at `executeWorkflowCore` — the one path every execution shares — fire-and-forget as the first statement, so the handshake overlaps the custom-block read and preprocessing ahead of the cancellation subscribe rather than being paid inside that subscribe's readiness budget. No task-id list to keep in sync: coverage follows from the funnel. The subscribe and the warm-up share the hub's memoized readiness wait, so their budgets never compound within a run. The global init keeps warming only the shared client, which nearly every task uses; the Next server keeps warming both at boot, one connection per long-lived process. * chore(instrumentation): import the warm-ups through the path alias The dynamic imports in the Redis warm-up block used relative paths; the file already resolves `@/` and the repository rule is absolute imports. * fix(redis): give every readiness waiter its own deadline and correct the budget model Three corrections from an independent review of the branch. The shared readiness wait carried one timer, so a subscribe that joined an in-flight warm-up inherited only the remainder of that warm-up's budget — down to nothing — where before this branch it was guaranteed a full wait of its own. Waiters now share one ready/end listener pair but each runs its own deadline, and the signal is torn down when its last waiter gives up. The budget model claimed a dead handshake always costs two command deadlines. That is true only without a password: with one, ioredis treats a timed-out AUTH as fatal and tears the socket down after a single deadline, and a connect that never completes is bounded by connectTimeout, which the formula had no term for. `coldConnectionBudgetMs` now charges the largest of those so it holds for either URL shape, and moves to @sim/utils/retry: a pure helper evaluated at module load must not live in a module the test setup replaces wholesale, which had forced the shared mock to carry a verbatim mirror of the arithmetic. The subscriber derives its budget in its constructor from the exact options it is built with; the shared client's reconnect delay is a pure function used by both retryStrategy and the warm-up budget. The Next server no longer opens the signal subscriber at boot: the execution entry point already warms it on intent, and an eagerly opened subscriber that cannot reach Redis would reconnect on a five-second cadence in every idle replica for the life of the process. * fix(redis): leave the readiness wait exactly once per waiter A waiter that timed out was still subscribed to the shared readiness signal, so when that signal later settled for another waiter its cleanup ran a second time. The waiter count drifted negative, the last-waiter teardown could never match again, and the settled signal stayed memoized — a later subscribe during a reconnect would have observed a readiness that had already passed. Each waiter now leaves exactly once, and a settling signal clears itself from the memo so the next waiter observes the connection afresh regardless of the count. * test(redis): assert the fresh subscribe by channel, not by call count A reconnect also re-subscribes surviving channels, so the new subscription is identified by its channel rather than by SUBSCRIBE having been issued once. * test(redis): prove waiter accounting by the teardown a timed-out waiter must perform The settle path clears the memo on its own, so a drifted waiter count only shows when a later lone waiter times out and fails to tear the signal down. Exercise that path directly; the test now fails without the per-waiter guard. * test(execution-core): type the warm-up test's fixtures instead of casting to any The partial fixtures are widened through unknown to the real ExecutionSnapshot and LoggingSession types, as the repository's TypeScript rule prescribes. * fix(redis): connect the signal subscriber on intent and complete the budget model Findings from an independent review of the finished branch. Constructing the execution-signal hub is what connects — ioredis dials in its constructor — so the warm-up that observed that handshake and returned a boolean its only caller discarded is gone. The execution entry point now constructs the hub, synchronously and never throwing, and the subscribe that follows waits on its own budget. With no observer to accommodate, the waiter machinery collapses to a race between the shared readiness signal and a per-waiter deadline, with a per-signal count so the last waiter detaches. The budget's dead-attempt term was still short for an unanswered handshake: after the ready check times out, ioredis half-closes the socket and destroys it only after `disconnectTimeout` when a wedged peer never answers with a FIN. Measured against such a peer: an unauthenticated connection reconnects at 12.5s, past the 11.5s the budget allowed. The term is now `max(connectTimeout, 2 * commandTimeout + disconnectTimeout)`, with the disconnect deadline stated in the shared connection defaults so the budget derives from it. The model is specific to ioredis 5's handshake sequence, so it lives beside the pinned client in `lib/core/config/redis-budget.ts` — a module the global test mock does not replace — rather than in a generic package. Corrects two doc claims: no caller warms the subscriber at server boot, and a late joiner never had a full wait of its own before this branch either — the memoized promise shared its single timer.
1 parent c4f0815 commit 2be2670

12 files changed

Lines changed: 488 additions & 46 deletions

File tree

‎apps/sim/instrumentation-node.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -407,6 +407,6 @@ export async function register() {
407407
// Not awaited: the connection is warmed in the background so the first request
408408
// that needs Redis does not pay the handshake inside its own command deadline,
409409
// but boot never waits on Redis to serve requests that do not touch it.
410-
const { warmRedisConnection } = await import('./lib/core/config/redis')
410+
const { warmRedisConnection } = await import('@/lib/core/config/redis')
411411
void warmRedisConnection()
412412
}
Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import { coldConnectionBudgetMs } from '@/lib/core/config/redis-budget'
6+
7+
describe('coldConnectionBudgetMs', () => {
8+
it('charges an unanswered handshake at two command deadlines plus the half-close wait', () => {
9+
// Unauthenticated: SETNAME/SETINFO time out, then INFO times out, then the
10+
// half-closed socket waits for a FIN a wedged peer never sends.
11+
expect(
12+
coldConnectionBudgetMs({
13+
connectTimeoutMs: 1_000,
14+
commandTimeoutMs: 5_000,
15+
disconnectTimeoutMs: 2_000,
16+
reconnectDelayMs: 500,
17+
})
18+
).toBe(2 * 5_000 + 2_000 + 500 + 1_000)
19+
})
20+
21+
it('charges the connect deadline when a connection that never completes is the longer case', () => {
22+
// Lowering the command deadline must not shrink the budget below what a
23+
// connect that never completes costs before retryStrategy can fire.
24+
expect(
25+
coldConnectionBudgetMs({
26+
connectTimeoutMs: 10_000,
27+
commandTimeoutMs: 3_000,
28+
disconnectTimeoutMs: 2_000,
29+
reconnectDelayMs: 500,
30+
})
31+
).toBe(10_000 + 500 + 1_000)
32+
})
33+
34+
it('always leaves room for the healthy attempt that follows the reconnect', () => {
35+
expect(
36+
coldConnectionBudgetMs({
37+
connectTimeoutMs: 0,
38+
commandTimeoutMs: 0,
39+
disconnectTimeoutMs: 0,
40+
reconnectDelayMs: 0,
41+
})
42+
).toBe(1_000)
43+
})
44+
})
Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
/** Generous room for the healthy handshake that follows a recovered stall; a real one takes tens of milliseconds. */
2+
const HEALTHY_HANDSHAKE_ALLOWANCE_MS = 1_000
3+
4+
export interface ColdConnectionBudgetOptions {
5+
/** The client's TCP connect deadline: bounds a connection that never completes at all. */
6+
connectTimeoutMs: number
7+
/** The client's per-command deadline: bounds a handshake command the server never answers. */
8+
commandTimeoutMs: number
9+
/** How long ioredis waits after half-closing a dead socket for the peer's FIN before destroying it. */
10+
disconnectTimeoutMs: number
11+
/** What the client's `retryStrategy` returns for its first reconnect. */
12+
reconnectDelayMs: number
13+
}
14+
15+
/**
16+
* How long a wait for a cold ioredis connection must allow before giving up,
17+
* if it is to survive one dead attempt and still see a healthy one land.
18+
*
19+
* A dead attempt is diagnosed by whichever deadline governs the phase it
20+
* stalls in. A connect that never completes costs `connectTimeoutMs`, and the
21+
* socket is destroyed outright. A connection that opens but whose handshake
22+
* is never answered costs command deadlines — one with a password, because a
23+
* timed-out `AUTH` is fatal; two without, because `CLIENT SETNAME`/`SETINFO`
24+
* must settle, by timing out, before the `INFO` ready check starts its own —
25+
* and then `disconnectTimeoutMs` more, because ioredis half-closes the socket
26+
* and a peer that is wedged never answers with a FIN. The budget takes the
27+
* larger phase so it holds for either URL shape without parsing it. Only then
28+
* does `retryStrategy` run and a fresh attempt begin.
29+
*
30+
* A wait sized to a single deadline expires while the first attempt is still
31+
* being diagnosed, so a configured retry can never be the thing that saves it.
32+
*
33+
* The guarantee is one dead attempt from a fresh or previously-ready client.
34+
* ioredis feeds `retryStrategy` its running attempt count, so a wait that
35+
* begins while the client is already deep in a reconnect loop faces larger
36+
* delays this does not model. The handshake sequence is ioredis 5's; keep this
37+
* beside the pinned client, not in a generic package.
38+
*/
39+
export function coldConnectionBudgetMs(options: ColdConnectionBudgetOptions): number {
40+
const deadAttemptMs = Math.max(
41+
options.connectTimeoutMs,
42+
2 * options.commandTimeoutMs + options.disconnectTimeoutMs
43+
)
44+
return deadAttemptMs + options.reconnectDelayMs + HEALTHY_HANDSHAKE_ALLOWANCE_MS
45+
}

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,14 +47,19 @@ vi.mock('ioredis', () => ({
4747

4848
import {
4949
acquireLock,
50+
CONNECT_TIMEOUT_MS,
5051
closeRedisConnection,
52+
DISCONNECT_TIMEOUT_MS,
5153
describeRedisConnection,
5254
extendLock,
5355
getRedisClient,
5456
onRedisReconnect,
5557
resetForTesting,
58+
SHARED_COMMAND_TIMEOUT_MS,
59+
sharedReconnectDelayMs,
5660
warmRedisConnection,
5761
} from '@/lib/core/config/redis'
62+
import { coldConnectionBudgetMs } from '@/lib/core/config/redis-budget'
5863

5964
describe('redis config', () => {
6065
beforeEach(() => {
@@ -471,7 +476,50 @@ describe('redis config', () => {
471476
})
472477
})
473478

479+
describe('sharedReconnectDelayMs', () => {
480+
it('grows exponentially from the base and caps, with upward-only jitter', () => {
481+
expect(sharedReconnectDelayMs(1, 0)).toBe(1_000)
482+
expect(sharedReconnectDelayMs(2, 0)).toBe(2_000)
483+
expect(sharedReconnectDelayMs(5, 0)).toBe(10_000)
484+
expect(sharedReconnectDelayMs(6, 0)).toBe(10_000)
485+
expect(sharedReconnectDelayMs(1, 1)).toBe(1_300)
486+
})
487+
})
488+
474489
describe('warmRedisConnection', () => {
490+
it('outlasts one dead handshake so the reconnect can be what warms it', async () => {
491+
mockRedisInstance.status = 'connecting'
492+
const warm = warmRedisConnection()
493+
494+
// The dead attempt's own diagnosis and half-close, then the longest first
495+
// reconnect delay: the moment a healthy second attempt can begin.
496+
await vi.advanceTimersByTimeAsync(
497+
Math.max(CONNECT_TIMEOUT_MS, 2 * SHARED_COMMAND_TIMEOUT_MS + DISCONNECT_TIMEOUT_MS) +
498+
sharedReconnectDelayMs(1, 1)
499+
)
500+
const client = getRedisClient()
501+
Object.assign(client ?? {}, { status: 'ready' })
502+
client?.emit('ready')
503+
504+
await expect(warm).resolves.toBe(true)
505+
})
506+
507+
it('still gives up once the budget is spent', async () => {
508+
mockRedisInstance.status = 'connecting'
509+
const warm = warmRedisConnection()
510+
511+
await vi.advanceTimersByTimeAsync(
512+
coldConnectionBudgetMs({
513+
connectTimeoutMs: CONNECT_TIMEOUT_MS,
514+
commandTimeoutMs: SHARED_COMMAND_TIMEOUT_MS,
515+
disconnectTimeoutMs: DISCONNECT_TIMEOUT_MS,
516+
reconnectDelayMs: sharedReconnectDelayMs(1, 1),
517+
})
518+
)
519+
520+
await expect(warm).resolves.toBe(false)
521+
})
522+
475523
it('resolves immediately when the connection is already usable', async () => {
476524
mockRedisInstance.status = 'ready'
477525

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

Lines changed: 45 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,10 @@ import { isIP } from 'node:net'
22
import { createLogger } from '@sim/logger'
33
import { toError } from '@sim/utils/errors'
44
import { randomFloat } from '@sim/utils/random'
5-
import Redis, { type RedisOptions } from 'ioredis'
5+
import Redis from 'ioredis'
66
import { env } from '@/lib/core/config/env'
77
import { getConfiguredCacheProvider } from '@/lib/core/config/env-capabilities.server'
8+
import { coldConnectionBudgetMs } from '@/lib/core/config/redis-budget'
89

910
const logger = createLogger('Redis')
1011

@@ -42,13 +43,24 @@ function resolveRedisTlsOptions(url: string | undefined): { servername: string }
4243
* and TLS SNI when REDIS_URL targets an IP. Every Redis client we open should
4344
* spread this; callers add their own retry / timeout policy on top.
4445
*/
45-
export function getRedisConnectionDefaults(
46-
url: string | undefined
47-
): Pick<RedisOptions, 'keepAlive' | 'connectTimeout' | 'enableOfflineQueue' | 'tls'> {
46+
export interface RedisConnectionDefaults {
47+
keepAlive: number
48+
connectTimeout: number
49+
disconnectTimeout: number
50+
enableOfflineQueue: boolean
51+
tls?: { servername: string }
52+
}
53+
54+
export const CONNECT_TIMEOUT_MS = 10_000
55+
/** ioredis's own default, stated so readiness budgets can be derived from it. */
56+
export const DISCONNECT_TIMEOUT_MS = 2_000
57+
58+
export function getRedisConnectionDefaults(url: string | undefined): RedisConnectionDefaults {
4859
const tls = resolveRedisTlsOptions(url)
4960
return {
5061
keepAlive: 1000,
51-
connectTimeout: 10000,
62+
connectTimeout: CONNECT_TIMEOUT_MS,
63+
disconnectTimeout: DISCONNECT_TIMEOUT_MS,
5264
enableOfflineQueue: true,
5365
...(tls ? { tls } : {}),
5466
}
@@ -203,12 +215,34 @@ export function describeRedisConnection(
203215

204216
const PING_INTERVAL_MS = 15_000
205217
const MAX_PING_FAILURES = 2
218+
export const SHARED_COMMAND_TIMEOUT_MS = 5_000
219+
const RECONNECT_BASE_MS = 1_000
220+
const RECONNECT_MAX_BASE_MS = 10_000
221+
const RECONNECT_JITTER_RATIO = 0.3
222+
223+
/**
224+
* The shared client's reconnect delay for attempt `times`, with `jitter` in
225+
* `[0, 1]` scaling the upward-only jitter band. Pure so the same formula can
226+
* be evaluated for a budget — `sharedReconnectDelayMs(1, 1)` is the longest
227+
* possible first reconnect — as well as from `retryStrategy`, which adds the
228+
* bookkeeping around it.
229+
*/
230+
export function sharedReconnectDelayMs(times: number, jitter: number): number {
231+
const base = Math.min(RECONNECT_BASE_MS * 2 ** (times - 1), RECONNECT_MAX_BASE_MS)
232+
return Math.round(base + jitter * base * RECONNECT_JITTER_RATIO)
233+
}
234+
206235
/**
207-
* Warm-up budget. Sized to outlast a slow handshake rather than a fast one,
208-
* because giving up early just returns the handshake to the first command's
209-
* deadline, which is the thing this exists to avoid.
236+
* Warm-up budget: one dead attempt, its longest possible first reconnect
237+
* delay, then a healthy attempt. Giving up sooner returns the handshake to the
238+
* first command's deadline, which is the thing warming exists to avoid.
210239
*/
211-
const REDIS_WARMUP_TIMEOUT_MS = 10_000
240+
const REDIS_WARMUP_TIMEOUT_MS = coldConnectionBudgetMs({
241+
connectTimeoutMs: CONNECT_TIMEOUT_MS,
242+
commandTimeoutMs: SHARED_COMMAND_TIMEOUT_MS,
243+
disconnectTimeoutMs: DISCONNECT_TIMEOUT_MS,
244+
reconnectDelayMs: sharedReconnectDelayMs(1, 1),
245+
})
212246

213247
export function getConfiguredRedisUrl(): string | null {
214248
if (getConfiguredCacheProvider() === 'database') return null
@@ -296,17 +330,15 @@ export function getRedisClient(): Redis | null {
296330

297331
state.client = new Redis(redisUrl, {
298332
...defaults,
299-
commandTimeout: 5000,
333+
commandTimeout: SHARED_COMMAND_TIMEOUT_MS,
300334
maxRetriesPerRequest: 5,
301335

302336
retryStrategy: (times) => {
303337
if (times > 10) {
304338
logger.error(`Redis reconnection attempt ${times}`, { nextRetryMs: 30000 })
305339
return 30000
306340
}
307-
const base = Math.min(1000 * 2 ** (times - 1), 10000)
308-
const jitter = randomFloat() * base * 0.3
309-
const delay = Math.round(base + jitter)
341+
const delay = sharedReconnectDelayMs(times, randomFloat())
310342
state.reconnects++
311343
logger.warn('Redis reconnecting', { attempt: times, nextRetryMs: delay })
312344
return delay

0 commit comments

Comments
 (0)