Skip to content

fix(queue): recover a Redis connection whose client phpredis marked failed - #258

Merged
loks0n merged 3 commits into
mainfrom
fix/queue-redis-recovers-failed-client
Sep 11, 2026
Merged

fix(queue): recover a Redis connection whose client phpredis marked failed#258
loks0n merged 3 commits into
mainfrom
fix/queue-redis-recovers-failed-client

Conversation

@loks0n

@loks0n loks0n commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

What

Connection\Redis runs every command through a call() wrapper: on a phpredis transport error ("went away", "Connection lost", "Connection closed", "Connection refused", "read error on connection") it drops the cached \Redis client and runs the command once more on a fresh connection. Any other RedisException (WRONGTYPE, OOM, ...) still surfaces as before, and a second transport failure propagates.

New e2e test RedisConnectionRecoveryTest, registered in the e2e suite against the compose redis on 16379.

Why

phpredis retries a dropped socket by itself, but once the peer stays away past its retry budget it sets the client to REDIS_SOCK_STATUS_FAILED for good, after which every command throws Redis server <host>:<port> went away. getRedis() cached that object and returned it forever.

On Appwrite Cloud this hit worker-executions when a Dragonfly primary moved during a rollout: the consume side recovered because Broker\Redis::receive() already closes and reconnects, but the command connection behind Locking never did, so every execution job failed at ack with the trace below for 30 minutes until the pod was restarted.

RedisException: Redis server queue-dragonfly:6379 went away
  src/Queue/Connection/Redis.php:126  del
  src/Queue/Connection/Locking.php:119  remove

TDD

The test reproduces the terminal phpredis state deterministically without an outage: a client with OPT_MAX_RETRIES 0 whose own socket the server closes via CLIENT KILL ID, then a command.

  • Before the change: RedisException: Connection lost at Redis.php:126, the production line.
  • After: the command runs on a fresh connection and later commands keep working. A second test checks that an unreachable server still throws.

composer test (113 unit tests) and SwooleTest against a live worker pass. bin/monorepo check queue --fix applied; the remaining PHPStan findings in Broker/Nats.php and Broker/Pool.php are from registry-resolved siblings in my local vendor and are not in this diff (main CI is green).

Notes

  • RedisCluster caches its client the same way and would benefit from the same wrapper; left out to keep this to the reported path.
  • A retried write can in principle double-apply if the server executed it before the read failed. phpredis checks EOF before writing, so the failed-state case retries an unsent command; the "read error" case carries the same risk any client has.

🤖 Generated with Claude Code

…ailed

phpredis retries a dropped socket on its own, but once the peer stays away
past that budget it parks the client in a failed state for good. Connection\
Redis cached that object and handed it back for every later command, so a
worker that lived through a broker failover longer than those retries
failed every ack with "Redis server ... went away" until the process was
restarted. Seen on Appwrite Cloud when a Dragonfly primary moved: the
consume side reconnected (the broker already handles that around receive())
while the command connection behind Locking never did, and every execution
job was marked failed for half an hour.

Every command now goes through call(), which drops the cached client and
runs the command once more on a fresh one when phpredis reports a
transport error. Server-side errors still surface unchanged, and a second
transport failure propagates.

RedisConnectionRecoveryTest reproduces the failed state deterministically:
a client with no retry budget whose socket the server closes, then the
next command. Red before this change at Redis.php:126, the production
stack's exact line; green after.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; the previous replay and test-effectiveness findings are resolved, and no new actionable failure was established.

Summary

  • Routes Redis operations through centralized transport-error handling.
  • Prevents automatic replay of pushes, pops, and counters when execution is uncertain.
  • Adds a TCP proxy that can simulate an outage or discard a command reply.
  • Registers the Redis recovery scenario in the queue E2E suite.

Reviews (3) · Last reviewed commit: "test(queue): lose the reply, not the ser..."

Comment thread packages/queue/src/Queue/Connection/Redis.php
Comment thread packages/queue/tests/Queue/E2E/Adapter/RedisConnectionRecoveryTest.php Outdated
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown

Benchmark results

queue — workload shapes across both concurrency axes (4 cores, 600 messages, median of 3)

workload shape redis msg/s nats msg/s
io 1p x 1c 38 38
io 1p x 4c 152 154
io 4p x 1c 152 153
cpu 1p x 1c 203 208
cpu 1p x 4c 210 213
cpu 4p x 1c 486 506
mixed 1p x 1c 66 67
mixed 1p x 4c 181 188
mixed 4p x 1c 260 263

Np x Mc = N consumer processes x M handler coroutines. io yields and should follow
coroutines; cpu does not and should follow processes; mixed is in between.

Shared CI runners — treat absolute numbers as rough, compare modes within a run. Commit 8c4d4b2.

…th a real outage

Review: a blind retry could double-apply a push, pop or counter whose
reply was lost after the server ran it. call() now always drops the failed
client so the next command heals, but replays only commands that are safe
to repeat (del, lrem, get, llen, lrange, set); pops, pushes and counters
surface the error and leave the decision to the broker.

The test no longer subclasses the adapter or touches phpredis options. It
talks to the compose redis through a TCP proxy it stops and starts, which
is what a broker failover looks like from a worker, and asserts on
observable behaviour only: a command fails while the server is gone, the
first command after it is back succeeds, and a push that failed is not
replayed. Endpoint comes from REDIS_URL like the NATS tests.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Comment thread packages/queue/tests/Queue/E2E/Adapter/RedisConnectionRecoveryTest.php Outdated
…replayed

Stopping the proxy before the push meant Redis never ran it, so a replaying
implementation and a non-replaying one both left the list at one element
and the test could not tell them apart. The proxy can now, on request,
forward one client chunk upstream and close both sockets before the reply:
the server runs the command, the client only sees a read error. With that
the list holds two entries after the failed push, and a replaying
implementation is caught at three.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@loks0n
loks0n merged commit d7a0029 into main Sep 11, 2026
7 checks passed
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