fix(redis): stop a slow handshake from failing as a command timeout - #7186
fix(redis): stop a slow handshake from failing as a command timeout#7186waleedlatif1 wants to merge 3 commits into
Conversation
ioredis arms the commandTimeout timer in sendCommand before it checks whether the socket is writable, so the budget covers connection setup and offline-queue wait as well as execution. With commandTimeout below connectTimeout, any handshake slower than the command deadline surfaced as "Command timed out" from a Redis that never received the command. - Raise the command deadline above the connect budget and derive both from named constants so the invariant cannot drift - Give the PING health check its own deadline so the wider command budget does not slow failover detection - Warm the shared connection at process start (Trigger.dev init, Next instrumentation) so a run's first command does not pay the handshake inside its own deadline - Record an execution log when admission infrastructure is unreachable, so those runs show as failed instead of disappearing - Re-admit rather than abort when a reservation refresh throws
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThe PR separates Redis connection, command, and health-probe deadlines, warms shared connections during process initialization, and improves execution behavior around admission infrastructure failures.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/background/workflow-execution.ts | Handles ambiguous reservation-refresh errors without repeating admission, resolving the previously reported duplicate-gating failure. |
| apps/sim/lib/core/config/redis.ts | Centralizes Redis deadlines, adds an independent PING timeout, and provides bounded per-client connection warm-up. |
| apps/sim/lib/execution/preprocessing.ts | Records admission-infrastructure failures unless retryable failure logging is intentionally suppressed. |
| apps/sim/trigger.config.ts | Awaits best-effort Redis warm-up during Trigger.dev process initialization. |
| apps/sim/instrumentation-node.ts | Starts non-blocking Redis warm-up during Next.js instrumentation registration. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Queued workflow starts] --> B{Admission already completed?}
B -- No --> E[Run preprocessing admission]
B -- Yes --> C[Refresh reservation expiry]
C -- true --> D[Keep existing admission]
C -- false --> E
C -- throws --> D
D --> F[Preprocess with usage and rate-limit gates skipped]
E --> G[Run usage, rate-limit, and reservation gates]
F --> H[Execute workflow]
G --> H
Reviews (3): Last reviewed commit: "fix(execution): honor retryable-failure ..." | Re-trigger Greptile
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…rows 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.
…dmission The new admission-infrastructure log row ignored suppressRetryableFailureLogs, so a webhook with a setup retry still available recorded a terminal failure before the same execution was requeued. That option exists precisely for this shape: statusCode >= 500 and retryable, which RESERVATION_INFRASTRUCTURE (503, retryable) matches. The denial branch nearby is unaffected because its descriptors are 402/429. Builds the failure once and reuses it for both the suppression check and the returned error rather than duplicating the shape.
|
Closing this while we root-cause properly. The timeout invariant this changes is real, but it treats a symptom: it makes the system tolerate a multi-second stall before a connection becomes usable rather than explaining why that stall happens. Reopening or re-cutting once the underlying cause is understood, so the fix can be aimed at it. |
Summary
commandTimeoutwas set belowconnectTimeout. ioredis arms that timer insendCommandbefore it checks whether the socket is writable and before theenableOfflineQueuebranch, so the budget covers handshake and offline-queue wait as well as execution — any handshake slower than the command deadline failed asCommand timed outfrom a Redis that never received the command (stack contains only ioredis timer frames)execution-signal.ts, which was hardcoding its owninit, Nextinstrumentation) so the first command doesn't pay the handshake inside its own deadline. Establishing TCP+TLS is far more expensive than the commands that run over it, so it should cost once per process, not once per unit of workType of Change
Testing
Verified the mechanism against ioredis source (
Redis.jssendCommand,Command.jstimer lifecycle — armed once, cleared only on settle, never re-armed on queue flush).New tests cover the timeout invariant, the decoupled probe deadline, and warm-up (already-ready, resolves on ready, gives up at the connect deadline, memoized per client, no-throw when Redis is unconfigured). Confirmed each new guard fails when its fix is reverted and passes when restored.
bun run type-checkclean. 3756 tests pass acrosslib/core,lib/execution,lib/logs,lib/billing,background.bun run lint, block-registry check, and all 37 audits incheck:auditspass.Checklist