Visitar URL original
Reset server-side state when reusing persistent connections by KentarouTakeda · Pull Request #2825 · phpredis/phpredis · GitHub
Skip to content

Reset server-side state when reusing persistent connections - #2825

Open
KentarouTakeda wants to merge 4 commits into
phpredis:developfrom
KentarouTakeda:reset-persistent-connection
Open

Reset server-side state when reusing persistent connections#2825
KentarouTakeda wants to merge 4 commits into
phpredis:developfrom
KentarouTakeda:reset-persistent-connection

Conversation

@KentarouTakeda

@KentarouTakeda KentarouTakeda commented Apr 4, 2026

Copy link
Copy Markdown
Contributor

Problem

When a pooled connection is retrieved via pconnect, server-side state from the previous request—SELECT, WATCH, CLIENT SETNAME, etc.—carries over. This causes reads and writes to go to the wrong database, stale watches to break transactions, and other subtle failures.

Fixes #1920. Related: #428.

Solution

Add the Redis 6.2 RESET command to the redis_sock_check_liveness() pipeline so that all server-side state is cleared on pool retrieval.

  • Pipeline order: RESET → AUTH → ECHO (prepended to the existing liveness check)
  • Pre-6.2 servers: the first -ERR response is cached in ConnectionPool.reset_unsupported; subsequent retrievals skip the command entirely
  • No stats pollution: the errorstats section was introduced in 6.2—the same version that added RESET—so no per-error counter is incremented on older servers; after the flag is cached the command is not sent at all

Why RESET

Approach Drawback
Selective commands (DISCARD+UNWATCH+SELECT 0+…) Must be updated whenever Redis adds new per-connection state
Disconnect and reconnect Negates the benefit of persistent connections (TCP/TLS handshake cost)
RESET Server-authoritative, single command, future-proof

6.2 adoption

RESET was added in Redis 6.2 (February 2021). Major managed services already provide it:

  • AWS ElastiCache — 6.2 available in all regions (November 2021)
  • GCP Memorystore — 6.x ships 6.2.13; default is 7.2 (docs)
  • Azure Cache for Redis — 6.2+ on Enterprise tier (docs)

On pre-6.2 servers, RESET returns -ERR but the connection stays alive. After the flag is cached the additional cost drops to zero.

Changes

  • common.h — add RESP_RESET_CMD constant; add reset_unsupported field to ConnectionPool
  • library.c — send and consume RESET in redis_sock_check_liveness(); pass ConnectionPool * to the function
  • tests/RedisTest.php — three regression tests covering SELECT, WATCH, and CLIENT SETNAME state leaks

yatsukhnenko
yatsukhnenko reviewed Apr 7, 2026
View reviewed changes
Comment thread library.c Outdated
yatsukhnenko
yatsukhnenko requested changes Apr 7, 2026
View reviewed changes
Comment thread library.c Outdated
Comment thread redis.c Outdated
@yatsukhnenko

yatsukhnenko commented Apr 8, 2026

Copy link
Copy Markdown
Member

@michael-grunder I'm not sure should we use ECHO to check liveness when RESET command is used but in general I'm fine with this PR

KentarouTakeda added a commit to KentarouTakeda/phpredis-phpredis that referenced this pull request Apr 8, 2026
@KentarouTakeda
Fix flaky testExists by using deterministic key setup
7835afb 
The test used rand() to decide which keys to create, making it
possible for $mkeys to be empty. This caused EXISTS to receive an
empty array, resulting in a sporadic assertion failure:
(false) !== 0

Observed in phpredis#2825 CI and also in an unrelated branch:
- https://github.com/phpredis/phpredis/actions/runs/24132080914
- https://github.com/phpredis/phpredis/actions/runs/23617186872
@KentarouTakeda KentarouTakeda mentioned this pull request Apr 8, 2026
Merged
michael-grunder pushed a commit that referenced this pull request Apr 8, 2026
@KentarouTakeda @michael-grunder
Fix flaky testExists by using deterministic key setup
82bf96c 
The test used rand() to decide which keys to create, making it
possible for $mkeys to be empty. This caused EXISTS to receive an
empty array, resulting in a sporadic assertion failure:
(false) !== 0

Observed in #2825 CI and also in an unrelated branch:
- https://github.com/phpredis/phpredis/actions/runs/24132080914
- https://github.com/phpredis/phpredis/actions/runs/23617186872
@KentarouTakeda
KentarouTakeda force-pushed the reset-persistent-connection branch 3 times, most recently from 90f525f to dda3084 Compare August 9, 2026 03:55
@KentarouTakeda
KentarouTakeda force-pushed the reset-persistent-connection branch from dda3084 to 2d4386d Compare August 9, 2026 03:59
@KentarouTakeda

KentarouTakeda commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the latest develop and resolved the conflicts.

Apart from the textual conflict, the type change from #2847 (smart_string → smart_str) also broke the build here, so I've updated the code to follow the current API as well.

Ready for review again.

@KentarouTakeda

KentarouTakeda commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

I'm not sure should we use ECHO to check liveness when RESET command is used

IMO we should keep ECHO. It also guards against a race where a previous request left unconsumed replies on the socket, and it only costs a few dozen bytes.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yatsukhnenko yatsukhnenko yatsukhnenko requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Multple persistent connections to the same Redis host with different databases: database number changes

2 participants

@KentarouTakeda @yatsukhnenko