Recover from transient read failures instead of aborting the client - #38
MultivitaminJuice wants to merge 1 commit into
Conversation
sdb9696
left a comment
There was a problem hiding this comment.
Hi @MultivitaminJuice thanks for the PR! I have some questions and change requests inline. Also this needs some kind of exponential backoff and a separate counter as it can't just keep trying every three seconds forever. Probably some extra config as well to control the behaviour.
| ## [Unreleased] | ||
|
|
||
| **Fixed bugs:** | ||
|
|
||
| - Treat `asyncio.IncompleteReadError` as a transient read failure: log at warning level, reconnect, and do not advance the connection error counter toward abort. | ||
| - Recover from unexpected `ValueError` in the MCS read loop via reset when the sequential NOTIFY error limit allows, instead of always terminating the client immediately. |
|
|
||
| except ValueError as ex: | ||
| _logger.error( | ||
| "Recoverable value error in FcmPushClient: %s\n%s", | ||
| ex, | ||
| traceback.format_exc(), | ||
| ) | ||
| if self._try_increment_error_count(ErrorType.NOTIFY): | ||
| await self._reset() | ||
|
|
There was a problem hiding this comment.
Its not clear how this relates to this PR
| if self.do_listen: | ||
| await self._reset() |
| ) | ||
| if self.do_listen: | ||
| await self._reset() | ||
| elif isinstance(osex, TimeoutError): |
There was a problem hiding this comment.
This can be combined with above as the only difference is the log message
| # Transient stream read (e.g. 0 bytes of an | ||
| # expected 1): reconnect without advancing the | ||
| # CONNECTION abort counter. | ||
| _logger.warning( |
There was a problem hiding this comment.
Look at _log_warn_with_limit() to avoid spamming the log.
|
Thanks for the detailed review! I'll address the inline points — combine the two branches, switch to On the backoff, before I implement it — does this match what you have in mind?
Two things I'd like your steer on:
|
Addresses review feedback on sdb9696#38. - Merge the IncompleteReadError / TimeoutError read-loop branches into one and reconnect with exponential backoff (capped at reconnect_backoff_max), driven by a dedicated _transient_error_count, instead of warning + resetting on every iteration. Uses _log_warn_with_limit and drops the redundant do_listen guard (_reset checks it). The counter resets on the next successful read. - Make _reset() self-heal: when _connect_with_retry() exhausts its tries during a sustained outage it no longer terminates the client; it keeps retrying with backoff so push recovers automatically once connectivity returns. The new abort_on_connection_failure flag restores the legacy shut-down behaviour. - Tests for transient-read reconnect (no CONNECTION counter, no terminate), self-heal on repeated connect failure, and the abort_on_connection_failure opt-out. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
5578487 to
f722b83
Compare
|
Thanks for the detailed review — and apologies in advance: I'm coming at this as a downstream Home Assistant / Ring user who ran into the push receiver dying, not as a firebase-messaging expert. So please reshape, rename, re-default, split, or drop anything that doesn't fit the library's direction — I trust your judgement on the right form far more than my own here. Rebased onto Your inline points:
Backoff + counter: transient read failures now reconnect with exponential backoff The part I'm least sure about (very much your call): in my setup the "push dies and never comes back" actually happens one level down — when I'm genuinely unsure this is the right layer, shape, or default for the library — it's simply what reliably keeps push alive on my HA/Ring box. I'd completely understand if you'd rather keep the terminate and leave recovery to the caller, flip the default the other way, or split the self-heal into its own PR. Happy to do whatever makes it easiest for you to review (or to drop the self-heal entirely and keep just the read-loop changes). Tests cover the transient reconnect (no CONNECTION counter, no terminate), the self-heal on repeated connect failure, and the |
|
Downstream data point from a Home Assistant / Ring user, and a small release request. The gap: the last PyPI release is 0.4.5 (2025-05-10). What that looks like in practice, from my own install:
That last point is why the workarounds circulating in the HA issue don't help: they reload the config entry when the event entity goes The request: would you consider cutting a 0.4.6 with #36 as-is? That alone would get the fix to downstream users, independent of when #38 lands. If you'd rather ship both together, that's of course fine too — this is really just a note that the fix is currently stuck behind the release, not behind the code. Related HA Core issue with 21 comments from affected users: home-assistant/core#157764. Happy to test a pre-release or a git ref against a live Ring doorbell if that's useful — I have a reproducible dead-listener install right now. |
Addresses #33. That issue's traceback shows
TimeoutError: SSL shutdown timed outbeing counted asErrorType.CONNECTION, so after 3 occurrences the client hitsabort_on_sequential_error_countand shuts down for good. This PR treats that timeout (andIncompleteReadError) as transient and reconnects without advancing the abort counter. Complementary to #36 — the two merge cleanly. Reported downstream in home-assistant/core#157764 and home-assistant/core#134431 (theSSL shutdown timed out→ 3×ErrorType.CONNECTION→ push-receiver shutdown that users have to reload the Ring integration to recover from).Summary
Makes the MCS read loop recover from transient stream failures instead of counting them toward the fatal abort threshold and terminating the client.
Previously
asyncio.IncompleteReadErrorandTimeoutError(when not already in a reset) fell into the generic "unexpected exception" branch, advanced theCONNECTIONerror counter, and afterabort_on_sequential_error_countfailures terminated the client for good. In practice these are transient stream hiccups — a 0-byte read, or"SSL shutdown timed out"after a stuck writer close — from which a simple reconnect recovers. On a long-running Home Assistant / Ring setup this manifested as push silently stopping until a full restart.Changes
asyncio.IncompleteReadErrorandTimeoutErroras transient: log a warning and_reset()the connection without advancing theCONNECTIONabort counter.ValueErrorin the read loop and_reset()(subject to theNOTIFYsequential-error limit) instead of letting it bubble up to the outer handler that immediately terminates the client.The existing reset-state handling for these exception types is unchanged; the new branches only apply when the client is not already resetting.
Testing
uv run pytest tests/test_fcmpushclient.py– all green, with new tests asserting that anIncompleteReadError/TimeoutErrortriggers a reconnect (LoginRequestre-sent) without incrementing theCONNECTIONcounter and without terminating.ruff check/ruff format --checkclean.Notes
This is the second of two independent PRs; the first (#37) hardens encrypted-payload decoding. They touch different code paths and can be reviewed/merged separately.
🤖 Generated with Claude Code