Repository navigation
fix(redis): time out unresponsive Redis connections - #14540
HarshMN2345 wants to merge 2 commits into
Conversation
…es open phpredis keeps the socket open when the reply to EVAL, EVALSHA or PING times out, so the next command on that client reads the late reply as its own. The cache adapter swallows lease and ping failures and Appwrite's lock wrapper swallows release failures, which put such a client back in the pool answering every later command with the previous command's reply.
Pooled Redis clients waited forever for a reply, so a Redis that stopped answering held every request until Swoole hit max_concurrency and answered 503 to everything. Connections now connect and read within _APP_REDIS_TIMEOUT (3 s by default, above the 2 s a queue retry blocks on a pop), set before AUTH so a reconnect cannot hang on it either. Pub/sub subscribers keep waiting indefinitely for the next message.
Security rulesNo new WARNING or ERROR findings from security rules. 19 existing findings tracked in
|
🔵 Tier A · Mergeable after minor fixes
Adds a configurable connect and read timeout to Redis clients used by the application pools, queue publishers, and MQTT rate limiting, while keeping pub/sub subscribers waiting indefinitely. Cache and lock adapters also close timed-out sockets to prevent late replies from desynchronizing later commands.
Note Not approving while a bug finding is open: Do not replay ambiguous timed-out purge scripts
Fix with agent prompt### Issue 1
packages/cache/src/Adapter/Redis.php:411
**Do not replay ambiguous timed-out purge scripts**
This makes `execute()` retry a script after a protocol error even though Redis may already have executed it and only its reply timed out. In particular, `LUA_PURGE_FIELD` and `LUA_PURGE_BUMP` increment the generation on every run; replaying a successful purge can return `false` and, if another cache write lands between attempts, leave that write behind despite invalidating its generation.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 6
🔇 Filtered out · 1Findings Hansi considered but did not post.
Reviewed |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
| 'went away', | ||
| 'socket', | ||
| 'read error on connection', | ||
| 'protocol error', // how phpredis reports a timed-out script reply |
There was a problem hiding this comment.
Do not replay ambiguous timed-out purge scripts
This makes execute() retry a script after a protocol error even though Redis may already have executed it and only its reply timed out. In particular, LUA_PURGE_FIELD and LUA_PURGE_BUMP increment the generation on every run; replaying a successful purge can return false and, if another cache write lands between attempts, leave that write behind despite invalidating its generation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/cache/src/Adapter/Redis.php
Line: 411
Comment:
**Do not replay ambiguous timed-out purge scripts**
This makes `execute()` retry a script after a protocol error even though Redis may already have executed it and only its reply timed out. In particular, `LUA_PURGE_FIELD` and `LUA_PURGE_BUMP` increment the generation on every run; replaying a successful purge can return `false` and, if another cache write lands between attempts, leave that write behind despite invalidating its generation.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · bug · Reply if this doesn't apply.
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds a bounded Redis connect/read timeout (_APP_REDIS_TIMEOUT, default 3s) to pooled and publisher connections, plus close-on-connection-error handling in the cache and lock adapters.
Key findings
- 🟡 Publisher pool misses timed-out replies as transport errors — registers.php:262
There was a problem hiding this comment.
🟡 bug · medium
Publisher pool misses timed-out replies as transport errors
The new 3s read timeout on publisher-pool connections (app/init/registers.php:262) can surface as a phpredis 'protocol error' or 'timed out' exception, but isTransportError() in packages/queue/src/Connection/Redis.php does not match either string. call() then rethrows without $this->close(), leaving the dirty socket in the Swoole pool, so the next borrower reads the stale reply as its own response — the same failure mode this PR fixes for the cache and lock packages. Corrupted replies can silently misroute queue publishes and acks.
📋 Prompt for AI Agents
In packages/queue/src/Connection/Redis.php around lines 320-326, update the regex in isTransportError() from '/went away|Connection (lost|closed|refused)|read error on connection/' to also match 'protocol error' and 'timed out'/'timeout' (case-insensitively), matching the connection-error list just added to packages/cache/src/Adapter/Redis.php isConnectionError(). Reason: app/init/registers.php:262 now applies a 3s read timeout to publisher Redis connections, and a timed-out script reply leaves the socket holding an unread reply that must trigger $this->close() in call() or the pooled connection serves a stale reply to the next command.
This branch has not been deployed
1 participant
Pooled Redis clients were opened with no connect timeout and
OPT_READ_TIMEOUT = -1, so a Redis that stopped answering held every request until Swoole'smax_concurrencyanswered 503 to everything. Cache, lock, abuse, pub/sub and publisher connections (and the MQTT rate-limit client) now connect and read within_APP_REDIS_TIMEOUT(default 3 s, above the 2 sBroker::retry()blocks on BRPOP). The read timeout is set withsetOptionbefore AUTH, since a socket reused from phpredis' persistent pool ignores thepconnectargument. Pub/sub subscribers switch back to waiting indefinitely. The worker consumer is unchanged.phpredis 6.3 leaves the socket open when an EVAL/EVALSHA/PING reply times out, so the next command reads the late reply. The cache adapter (lease saves, ping) and the lock wrapper (release) swallow those errors and returned the client to the pool. The cache adapter and
Distributednow close such a socket.Verified against a throwaway
redis:7.4.7with a password andCLIENT PAUSE ... ALL, driving the real pools fromapp/init/registers.phpin the dev image:retry()'s 2 s BRPOP still succeeds.pint --testand PHPStan on the touched files,bin/monorepo check cache|lock|queue, and the unit suites of those three packagesCloses #14490