Skip to content

fix(redis): subscriber watchdog - #52

Merged
bpolaszek merged 2 commits into
bpolaszek:mainfrom
geonativefr:fix/redis-subscriber-watchdog
Sep 22, 2026
Merged

bpolaszek merged 2 commits into
bpolaszek:mainfrom
geonativefr:fix/redis-subscriber-watchdog

Conversation

@LounisBou

@LounisBou LounisBou commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

The Redis transport opens two connections from the same DSN, one for commands and one for the pub/sub subscription. The periodic ping only reached the command connection, so a half-open subscription socket went unnoticed: the hub kept answering publish requests while every subscriber stopped receiving anything.

The ping now runs on both connections. Paired with a non-zero readTimeout it turns a silent socket into a rejected promise, which reaches Hub::die and lets the supervisor restart the hub.

@LounisBou
LounisBou marked this pull request as draft September 21, 2026 14:33
@codecov

codecov Bot commented Sep 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (ed9d6e4) to head (4bb152e).

Additional details and impacted files
@@             Coverage Diff             @@
##                main       #52   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity       164       166    +2     
===========================================
  Files             27        27           
  Lines            465       471    +6     
===========================================
+ Hits             465       471    +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@LounisBou
LounisBou marked this pull request as ready for review September 21, 2026 14:35
@bpolaszek

Copy link
Copy Markdown
Owner

AFAIK no other command can reach the subscribe connection as soon as subscribe() is called (blocking call).
Does that work for real?

@LounisBou

Copy link
Copy Markdown
Contributor Author

AFAIK no other command can reach the subscribe connection as soon as subscribe() is called (blocking call).

It does, integration suite proves it. Why do you think it doesn't ? redis-react is asynchronous, so subscribe() just writes SUBSCRIBE and returns a promise, and the client keeps accepting commands on that connection.

Redis allows PING while subscribed and answers ["pong", ""].

@LounisBou

LounisBou commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor Author

I just re-check it on a real Redis

subscribe() is not blocking redis-react: it writes the command and returns a promise and a ping() in same tick.

Redis allows PING on a connection in subscribed state, while GET throws "ERR Can't execute 'get'".

It seems SUBSCRIBE / UNSUBSCRIBE / PING / QUIT / RESET are allowed.

@LounisBou

Copy link
Copy Markdown
Contributor Author

Found documentation on it for ping: https://redis.io/docs/latest/commands/ping/

If the client is subscribed to a channel or a pattern, it will instead return a multi-bulk string with PONG in the first position and an empty bulk string in the second position, unless an argument is provided, in which case it returns a copy of the argument.

@misaert
misaert requested a review from bpolaszek September 22, 2026 07:50
@bpolaszek
bpolaszek merged commit ae96581 into bpolaszek:main Sep 22, 2026
18 checks passed
@LounisBou
LounisBou requested a review from bpolaszek September 22, 2026 07:52
@bpolaszek

Copy link
Copy Markdown
Owner

LGTM :-)

@LounisBou

Copy link
Copy Markdown
Contributor Author

@bpolaszek, thanks ! Can you make a new release with this please ?

@bpolaszek

Copy link
Copy Markdown
Owner

Consider it done ✅ https://github.com/bpolaszek/freddie/releases/tag/0.5.1

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