fix(connection): stop reconnect storms, unbounded poll cycles and event-loop stalls - #618
Open
greggo74 wants to merge 2 commits into
Open
fix(connection): stop reconnect storms, unbounded poll cycles and event-loop stalls#618greggo74 wants to merge 2 commits into
greggo74 wants to merge 2 commits into
Conversation
Review of the connect/poll/reconnect paths for defects that cause or worsen repeated loss of connection to the panel. 13 findings, ranked by likely contribution, with file:line references against be1e46e. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…p stalls Addresses the 13 findings in CONNECTION_STABILITY_REVIEW.md. Tier 1: - IO_TIMEOUT was bound as a default argument, evaluated at import time -- before main() runs cfg.load() -- so every request path silently used the built-in 0.5 s and configuring it did nothing. Resolve it per call. - Bound the status poll cycle with a panel-owned budget and floor the idle gap between cycles, so an overrunning cycle no longer re-polls a struggling panel back to back with zero delay. - Reconnect backoff used '2 ^ retry' (XOR): 3, 0, 1, 6, 7, ... seconds, so the second attempt reconnected instantly and it never reached the cap. - Failed IP connect attempts left their socket open and unowned; the retry overwrote _protocol and PAI competed with its own orphans for the module's single session slot. Close each attempt, and pause between them. Tier 2 (STUN/paradoxmyhome): - The TURN refresh ran inline in write(), doing blocking socket I/O on the event loop with no socket timeout. Run it in an executor, bound the sockets, and drop the link when it fails. - Replaced a blocking time.sleep(5) in an async function, and bounded the SWAN site lookup. - receive_response() assumed one recv() returned a whole STUN message. Tier 3: - busy.release() ran in a finally that could not have acquired the lock. - gather left siblings running after the first status request failed. - Connection.close() skipped its state reset when the protocol raised -- which is the normal case when closing after a fault. - An unparsable status block took the whole cycle down with it. - disconnect() built a Connection just to ask whether one was open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Collaborator
|
Did you try |
Author
|
Next on my list of todo. I'll do it today and let you know. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
My IP150/paradoxmyhome setup was dropping its panel connection several times a
day. Rather than patch around it locally I worked through the connection and
polling paths and ended up with 13 findings, most of which reproduce without any
hardware. This PR fixes them and adds regression tests for each.
Happy to split this into smaller PRs if that is easier to review — say the word
and I will break it up by tier.
Tier 1 — most likely to cause the drops
IO_TIMEOUTfrompai.confwas silently ignored.cfg.IO_TIMEOUTwascaptured into default arguments at import time, so the configured value never
reached any request path. Anyone who raised it to work around timeouts has
been running the default all along.
replies started arriving late. Now bounded by
Panel.status_cycle_budget; acycle that exceeds it is cancelled and counted as a missing reply, so it
surfaces as "Replies missing" and a reconnect instead of silence.
2 ^ retry— bitwise XOR, not exponentiation.The real sequence was 3, 0, 1, 6, 7… so the second retry fired immediately.
Now 2, 4, 8, 16, 30, 30 s.
IP150's single session slot so the retry could not get in.
Tier 2 — STUN / paradoxmyhome path
refresh_session_if_required()did blocking socket I/O on the event loop.time.sleep(5)inside an async function, plus an unbounded HTTP call.recv()returns a whole message.Tier 3 — smaller, still real
busy.release()could be called without holding the lock.asyncio.gatherabandoned sibling requests on first failure.disconnect()could construct a connection object during shutdown.Behaviour changes worth knowing about
outage, because PAI stops hammering the module.
CONNECT_RETRY_DELAY), so a failingconnect()takes ~10 s longer before handing back to the main retry loop.IO_TIMEOUTnow actually takes effect — existing configs shouldre-check their value.
SWAN site info instead of reusing a possibly stale
xoraddr. Costs one extraHTTPS round trip per retry.
into one request per area and zone.
Testing
21 new regression tests across
tests/connection/,tests/lib/,tests/paradox/andtests/test_main_uptime.py. Full suite for the touchedareas: 319 passed. Each finding has a test that fails before its fix.
Note on
CONNECTION_STABILITY_REVIEW.mdThe first commit adds the full write-up as a document in the repo root, with the
reasoning and file/line references behind each finding. I have kept it because it
makes the second commit reviewable, but I am happy to drop that commit if you
would rather it lived only in this PR description.