fix: improve error handling and logging for Redis stream connections - #1029
fix: improve error handling and logging for Redis stream connections#1029wakonig wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Improves Redis stream and pub/sub logging by suppressing repeated connection-loss messages and reporting recovery.
Changes:
- Tracks stream connection failures.
- Adds pub/sub recovery logging.
- Adds outage and recovery tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Summary | Findings |
|---|---|---|
bec_lib/tests/test_managed_redis_connection.py |
Adds connection logging tests. | No final comments. |
bec_lib/bec_lib/redis_connector/managed_redis_connection.py |
Updates stream and pub/sub error handling. | Two moderate issues remain involving failure-type preservation and validating successful reads before logging recovery. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
bec_lib/bec_lib/redis_connector/managed_redis_connection.py:560
- Using
orhere discardsnormal_errorwhenever a from-start read also fails. With ACLs that deny different stream keys, the loop will log only the from-start topics forever and hide the normal subscription failure, even though the error text is intentionally topic-specific. Aggregate both errors (deduplicating identical connection errors) before applying the once-per-error logging logic.
stream_error = from_start_error or normal_error
bec_lib/bec_lib/redis_connector/managed_redis_connection.py:568
- If the last stream subscription is removed (or garbage-collected) after an error, this iteration can have no
from_start_errorornormal_errorand perform no Redis read at all. The branch then reports that Redis reconnected even though connectivity was never verified, so a still-down server gets a false recovery log. Only clear the error and emit this message after a successful stream read.
elif error is not None:
error = None
logger.info(f"{self.name} reconnected to redis ({self.host}:{self.port}).")
0dc162a to
3c66d96
Compare
3c66d96 to
35c1261
Compare
Description
Streams were logging at 1 Hz if the connection got lost. While this is unlikely to happen in production, the development machines may not be able to connect to Atlas from the outside of PSI, leading to massive log file.
Here, I'm changing the logging behavior to log once on connection loss and once when the connection is re-established.
The pub sub was already only logging once but was lacking the recovery message, which I added now as well.