Skip to content

fix(redis): stop a slow handshake from failing as a command timeout - #7186

Closed
waleedlatif1 wants to merge 3 commits into
stagingfrom
fix/redis-connect-command-timeout-race
Closed

fix(redis): stop a slow handshake from failing as a command timeout#7186
waleedlatif1 wants to merge 3 commits into
stagingfrom
fix/redis-connect-command-timeout-race

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • commandTimeout was set below connectTimeout. ioredis arms that timer in sendCommand before it checks whether the socket is writable and before the enableOfflineQueue branch, so the budget covers handshake and offline-queue wait as well as execution — any handshake slower than the command deadline failed as Command timed out from a Redis that never received the command (stack contains only ioredis timer frames)
  • Derive both deadlines from named constants so the invariant can't drift, and apply the same constant in execution-signal.ts, which was hardcoding its own
  • Give the PING health check its own deadline — a probe only runs on an established connection, so it shouldn't inherit a budget sized for handshakes. Keeps failover detection at ~2 intervals
  • Warm the shared connection at process start (Trigger.dev init, Next instrumentation) 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 work
  • Record an execution log when admission infrastructure is unreachable. That branch returned a failure without one, unlike the denial branch above it, so affected runs vanished from the workspace's logs instead of showing as failed
  • Re-admit rather than abort when a reservation refresh throws — a throw previously killed the run before the logging session existed

Type of Change

  • Bug fix

Testing

Verified the mechanism against ioredis source (Redis.js sendCommand, Command.js timer 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-check clean. 3756 tests pass across lib/core, lib/execution, lib/logs, lib/billing, background. bun run lint, block-registry check, and all 37 audits in check:audits pass.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

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

vercel Bot commented Aug 28, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Aug 28, 2026 1:12am

Request Review

@greptile-apps

greptile-apps Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR separates Redis connection, command, and health-probe deadlines, warms shared connections during process initialization, and improves execution behavior around admission infrastructure failures.

  • Raises the shared command deadline above the connection deadline and centralizes the value.
  • Adds bounded, memoized Redis connection warm-up for Next.js and Trigger.dev processes.
  • Uses a dedicated health-probe deadline to preserve timely reconnect detection.
  • Records terminal admission-infrastructure failures while suppressing logs for executions that will retry.
  • Preserves completed admission when reservation refresh has an ambiguous error, while repeating admission only on a definitive false result.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

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
Loading

Reviews (3): Last reviewed commit: "fix(execution): honor retryable-failure ..." | Re-trigger Greptile

Comment thread apps/sim/background/workflow-execution.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 9 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread apps/sim/lib/core/config/redis.ts
Comment thread apps/sim/lib/execution/preprocessing.ts Outdated
Comment thread apps/sim/background/workflow-execution.ts Outdated
Comment thread apps/sim/background/workflow-execution.ts Outdated
…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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

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

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

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.

@waleedlatif1
waleedlatif1 deleted the fix/redis-connect-command-timeout-race branch August 28, 2026 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant