Skip to content

Add timeouts and bounded reconnect to Redis client#120

Merged
masnwilliams merged 5 commits into
mainfrom
hypeship/redis-timeouts
Jul 20, 2026
Merged

Add timeouts and bounded reconnect to Redis client#120
masnwilliams merged 5 commits into
mainfrom
hypeship/redis-timeouts

Conversation

@masnwilliams

@masnwilliams masnwilliams commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

The Redis client retried reconnecting forever with no connect or command timeout. When Redis was unreachable, any command (e.g. the OAuth org-context writes in /token) blocked until the serverless function limit instead of throwing — so kernel login hung after the browser showed "authentication successful" rather than failing with an error.

This bounds the failure:

  • Bounded reconnectreconnectStrategy now returns an Error after MAX_RECONNECT_ATTEMPTS (10) instead of retrying indefinitely, so queued commands reject when Redis stays down.
  • Connect timeoutconnectTimeout (5s) on the socket so a black-holed connection fails fast.
  • Per-command timeout — every op runs through withTimeout (5s ceiling) so a stalled-but-connected command surfaces as an error.

Net effect: a Redis outage returns a clean error in seconds instead of hanging the caller.

Test plan

  • tsc --noEmit passes
  • prettier clean
  • Manual: confirm normal OAuth login still succeeds against a healthy Redis (behavior unchanged on the happy path)

Follow-up companion PR on kernel/cli adds a timeout on the CLI token exchange so it also fails fast.


Note

Medium Risk
Touches all Redis-backed org-context paths used during token exchange; mis-tuned timeouts could cause false failures under slow Redis, though happy-path behavior should stay the same.

Overview
Redis failures now surface in seconds instead of hanging OAuth/token handlers (e.g. /token org-context writes and kernel login after the browser succeeds).

The shared redis client adds a 5s per-command ceiling via withTimeout, 3s socket connectTimeout, and a reconnect strategy that stops after two connect attempts (returns an error instead of retrying forever). Connection handling drops the manual isConnected / event listeners in favor of client.isReady, shares one in-flight connect() via openConnection, and introduces resetClient() (destroy() on a stuck open socket) so failed connects, command timeouts, and transient socket errors can retry on a clean client instead of hitting “Socket already opened.”

Reviewed by Cursor Bugbot for commit 1ff018b. Bugbot is set up for automated code reviews on this repo. Configure here.

Previously the Redis client retried reconnecting forever with no connect or
command timeout, so an unreachable Redis made callers (e.g. the OAuth token
exchange) block indefinitely instead of failing. Bound the reconnect attempts,
set a connect timeout, and cap each command so Redis outages surface as errors.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 13, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
mcp Ready Ready Preview, Comment Jul 20, 2026 6:59pm

@masnwilliams
masnwilliams marked this pull request as ready for review July 13, 2026 20:45
@masnwilliams
masnwilliams requested a review from sjmiller609 July 13, 2026 20:46
Comment thread src/lib/redis.ts
Comment thread src/lib/redis.ts
Comment thread src/lib/redis.ts

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

Additional Suggestion:

The 5s command timeout does not cover ensureConnected()/client.connect(), so an unreachable Redis blocks the caller for ~60s+ instead of the intended ~5s budget.

Fix on Vercel

sjmiller609
sjmiller609 previously approved these changes Jul 20, 2026
Cap ensureConnected() with the same ~5s ceiling as commands (connect drives
the reconnect loop, so an unreachable Redis blocked callers for the full
reconnect budget), destroy the client on connect timeout so the next call
starts clean, swallow late command settlements to avoid unhandled rejections,
and fix the reconnect-attempt off-by-one.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment thread src/lib/redis.ts
Comment thread src/lib/redis.ts Outdated
Comment thread src/lib/redis.ts
Fixes raised on the prior hardening commit:
- centralize teardown in resetClient() so a failed connect or stalled command
  clears the half-open socket instead of destroying a shared client out from
  under concurrent callers (the in-flight connect is already shared)
- drop the isConnected flag in favor of client.isReady so state can't get
  stuck true after the socket is torn down
- treat a command timeout as a reset trigger so the next call reconnects
  rather than reusing a stalled-but-ready link

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursor cursor 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.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 29d6903. Configure here.

Comment thread src/lib/redis.ts
@masnwilliams
masnwilliams requested a review from sjmiller609 July 20, 2026 18:53
The external connect deadline + destroy() approach kept spawning edge cases
(orphaned connect loop when the deadline won mid-backoff). Drop it entirely and
let node-redis bound connect() itself: connectTimeout caps each attempt and a
bounded reconnectStrategy caps the attempt count, so an unreachable Redis fails
in ~6.5s with a clear error and no in-flight connect is ever force-destroyed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@masnwilliams
masnwilliams merged commit 74b476d into main Jul 20, 2026
10 checks passed
@masnwilliams
masnwilliams deleted the hypeship/redis-timeouts branch July 20, 2026 19:03
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.

2 participants