feat: add configurable connect and read timeouts to Redis connections#70
feat: add configurable connect and read timeouts to Redis connections#70lohanidamodar wants to merge 4 commits intomainfrom
Conversation
Queue Redis connections previously used PHP's default timeout of 0 (blocking indefinitely). When Redis becomes unresponsive due to contention (e.g. Dragonfly TxQueue saturation), worker BRPOP calls would hang until the OS TCP timeout (21-127s), causing fatal "read error on connection" crashes and cascading restarts. Adds `connectTimeout` and `readTimeout` parameters (default 5s) to both `Redis` and `RedisCluster` connection classes so callers can configure fail-fast behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis PR adds per-connection Redis timeouts to both Redis and RedisCluster connection classes: two new protected properties ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
src/Queue/Connection/Redis.php
Outdated
| protected ?\Redis $redis = null; | ||
|
|
||
| public function __construct(string $host, int $port = 6379, ?string $user = null, ?string $password = null) | ||
| public function __construct(string $host, int $port = 6379, ?string $user = null, ?string $password = null, float $connectTimeout = 5, float $readTimeout = 5) |
There was a problem hiding this comment.
sorry, I meant by that we should also note it in the source
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Queue/Connection/Redis.php`:
- Around line 193-194: The Redis client is currently using a 5s OPT_READ_TIMEOUT
which can cause blocking calls to throw and leaves the broken connection reused;
change the default readTimeout to -1 (disable client-side socket timeout) and
update the blocking methods rightPop, leftPop, and rightPopLeftPush to: before
issuing a blocking command temporarily set \Redis::OPT_READ_TIMEOUT to at least
the requested $timeout (or disable if $timeout is -1), call the blocking method
inside a try/catch that catches RedisException, and in the catch set
$this->redis = null (so getRedis() will reconnect) and rethrow or translate the
error as appropriate; also ensure you restore the previous OPT_READ_TIMEOUT
value in a finally block so the option is always reset after the call.
In `@src/Queue/Connection/RedisCluster.php`:
- Line 188: The RedisCluster is constructed with a fixed readTimeout which can
abort blocking commands and corrupt the connection; modify the code so that
blocking methods (rightPop, leftPop, rightPopLeftPush) temporarily set Redis
option OPT_READ_TIMEOUT to max($timeout + 1, $this->readTimeout) before issuing
the blocking call and restore the original OPT_READ_TIMEOUT afterwards, and wrap
the call in a try/catch that nulls $this->redis on RedisException to force
reconnection (getRedis should tolerate a null and recreate the client);
additionally change the default constructor behavior for new \RedisCluster(...)
to use a readTimeout of -1 (infinite) instead of a small fixed value so callers
rely on per-call timeouts unless explicitly overridden.
A fixed readTimeout on the connection caused brPop/blPop/bRPopLPush to throw RedisException whenever the command's block duration exceeded the socket read timeout, regardless of whether Redis responded correctly. Changes: - Default readTimeout to -1 (infinite) to preserve existing behavior; callers can opt in to a stricter timeout when needed (e.g. publishers) - Before each blocking call, temporarily widen OPT_READ_TIMEOUT to max($timeout + 1, $this->readTimeout) so the socket never fires before Redis returns - Restore the previous OPT_READ_TIMEOUT in a finally block - On RedisException in a blocking call, null $this->redis so getRedis() reconnects on the next call instead of reusing a broken connection - Deduplicate leftPopArray/rightPopArray to delegate to leftPop/rightPop Applies to both Redis and RedisCluster connection classes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Queue/Connection/Redis.php`:
- Around line 244-245: getRedis() currently connects but never authenticates
using the stored constructor credentials ($user and $password), so either call
$this->redis->auth($this->password) or $this->redis->auth($this->user,
$this->password) immediately after connect() (and only when credentials are
present), and handle failures by throwing or logging an exception; update
getRedis() to perform this auth step (or remove the unused $user/$password
fields from the constructor if you prefer) so authentication is not silently
ignored.
| $this->redis->connect($this->host, $this->port, $this->connectTimeout); | ||
| $this->redis->setOption(\Redis::OPT_READ_TIMEOUT, $this->readTimeout); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if auth() is called anywhere else or if there's a base class handling it
rg -n 'auth\(' src/Queue/Connection/Redis.php
echo "---"
# Check if Connection interface or any parent class handles auth
rg -n 'auth' src/Queue/Connection/
echo "---"
# Check git history for auth in getRedis
git log --oneline -10 -- src/Queue/Connection/Redis.phpRepository: utopia-php/queue
Length of output: 150
🏁 Script executed:
# Let's examine the Redis.php file to see constructor and getRedis method
cat -n src/Queue/Connection/Redis.php | head -100Repository: utopia-php/queue
Length of output: 4220
🏁 Script executed:
# Continue reading Redis.php to see the rest of the constructor and getRedis method
cat -n src/Queue/Connection/Redis.php | sed -n '100,280p'Repository: utopia-php/queue
Length of output: 5685
🏁 Script executed:
# Check if there's a parent class or interface
rg -n 'class Redis|interface Connection' src/Queue/Connection/Repository: utopia-php/queue
Length of output: 207
🏁 Script executed:
# Check the Connection interface definition
cat -n src/Queue/Connection.phpRepository: utopia-php/queue
Length of output: 1786
🏁 Script executed:
# Check if RedisCluster handles auth differently
rg -n 'auth' src/Queue/Connection/RedisCluster.phpRepository: utopia-php/queue
Length of output: 42
Call auth() or auth(password) in getRedis() to authenticate with stored credentials.
The constructor accepts and stores $user and $password (line 30), but getRedis() never calls the PHP Redis auth() method. This means credentials are accepted but silently ignored, bypassing authentication entirely. Either implement authentication by calling auth() in getRedis() after connecting, or remove the unused parameters from the constructor.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Queue/Connection/Redis.php` around lines 244 - 245, getRedis() currently
connects but never authenticates using the stored constructor credentials ($user
and $password), so either call $this->redis->auth($this->password) or
$this->redis->auth($this->user, $this->password) immediately after connect()
(and only when credentials are present), and handle failures by throwing or
logging an exception; update getRedis() to perform this auth step (or remove the
unused $user/$password fields from the constructor if you prefer) so
authentication is not silently ignored.
2 participants
Queue Redis connections previously used PHP's default timeout of 0 (blocking indefinitely). When Redis becomes unresponsive due to contention (e.g. Dragonfly TxQueue saturation), worker BRPOP calls would hang until the OS TCP timeout (21-127s), causing fatal "read error on connection" crashes and cascading restarts.
Adds
connectTimeoutandreadTimeoutparameters (default 5s) to bothRedisandRedisClusterconnection classes so callers can configure fail-fast behaviour.Summary by CodeRabbit
New Features
Bug Fixes