Skip to content

fix: prune dead transports from the engine on connection loss#1796

Closed
bdraco wants to merge 2 commits into
masterfrom
fix/self-heal-dead-transport
Closed

fix: prune dead transports from the engine on connection loss#1796
bdraco wants to merge 2 commits into
masterfrom
fix/self-heal-dead-transport

Conversation

@bdraco

@bdraco bdraco commented Jun 21, 2026

Copy link
Copy Markdown
Member

Summary

When an interface goes down or changes IP at runtime, its per interface sender transport stays in the engine forever; every multicast send then raises EHOSTUNREACH (No route to host), logged once then retried silently for the life of the instance, so the only recovery is to recreate the whole Zeroconf. This makes AsyncListener.connection_lost prune itself, dropping its reader and sender wrappers and its protocol from the engine lists so a dead socket stops being used.

Details

  • New AsyncEngine._async_remove_listener is the first incremental remove path; it mirrors the list semantics of _async_shutdown and matches both wrapper objects by the shared underlying transport identity (wrapper.transport is dead_transport), since one per interface socket is wrapped twice (once into readers, once into senders).
  • Runs on the loop thread only, so it is free threading safe; guards make it a no op if the listener was already gone or never registered.
  • This only self heals on real transport death; a transient route flap is not pruned here, that is left to a later interface rescan.

Test plan

  • tests/test_engine.py::test_connection_lost_prunes_transport asserts readers, senders, and protocols shrink and no longer reference the lost transport.
  • Full tests/test_engine.py green under both the compiled Cython build (REQUIRE_CYTHON=1) and pure Python (SKIP_CYTHON=1).
  • pre-commit run clean.

@codecov

codecov Bot commented Jun 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.77%. Comparing base (868f28f) to head (530b7ff).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1796   +/-   ##
=======================================
  Coverage   99.77%   99.77%           
=======================================
  Files          33       33           
  Lines        3540     3549    +9     
  Branches      498      500    +2     
=======================================
+ Hits         3532     3541    +9     
  Misses          5        5           
  Partials        3        3           

☔ 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.

@codspeed-hq

codspeed-hq Bot commented Jun 21, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 21 untouched benchmarks


Comparing fix/self-heal-dead-transport (530b7ff) with master (868f28f)1

Open in CodSpeed

Footnotes

  1. No successful run was found on master (5333fc5) during the generation of this report, so 868f28f was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@bdraco
bdraco force-pushed the fix/self-heal-dead-transport branch from 37e3ceb to 530b7ff Compare June 21, 2026 23:29
@bluetoothbot

Copy link
Copy Markdown
Contributor

PR Review — fix: prune dead transports from the engine on connection loss

Solid, well-scoped pruning mechanism; main open question is whether its trigger fires in the case it targets.

Strengths:

  • Matching readers/senders by shared underlying transport identity (w.transport is not transport) is exactly right given each socket's transport is wrapped three times (reader, sender, and listener.transport).
  • Rebuilding self.readers/self.senders via list comprehension (new list object) rather than in-place .remove() means any in-flight iterator over the old list — e.g. async_send's for send_transport in self.engine.senders — is unaffected.
  • Loop-thread-only execution keeps it free-threading safe; no-op guards handle the never-registered / already-removed cases, and the noop test pins that.
  • Coverage is complete and CodSpeed shows no regression.

Needs attention:

  • warning Fix ServiceInfo __repr__ #1: the motivating scenario (interface down → EHOSTUNREACH on send) routes through asyncio's error_received, not connection_lost, so the dead transport may never reach this hook — verify the real trigger fires or prune from error_received.
  • suggestion: trim the _async_remove_listener docstring to a single line per CLAUDE.md style.
  • The added test exercises connection_lost directly, which validates the pruning logic but not that connection_lost is actually invoked on interface-down.

🟡 Important

1. connection_lost likely never fires for the EHOSTUNREACH scenario this PR targets
src/zeroconf/_listener.py:358-360

The mechanism here is correct, but the hook point may not fire for the exact failure mode the PR describes.

The PR motivates this as: interface goes down/changes IP → every multicast send raises EHOSTUNREACH → logged once, retried silently forever. In CPython's asyncio (_SelectorDatagramTransport), a send that raises OSError (which EHOSTUNREACH is) is delivered to the protocol's error_received(), not connection_lost():

except OSError as exc:
    self._protocol.error_received(exc)
    return
except Exception as exc:
    self._fatal_error(exc, 'Fatal write error on datagram transport')

The read path (_read_ready) is the same — OSErrorerror_received, only non-OSError exceptions escalate to _fatal_errorconnection_lost. For a downed interface, reads generally don't error and sends raise EHOSTUNREACH into error_received. So connection_lost will essentially only fire on explicit transport.close() (normal shutdown) and truly unexpected exceptions — not on the interface-down case driving this PR.

Why it matters: as written, the self-heal path is wired to a callback that won't be invoked in the primary scenario, so a dead sender transport can still linger and keep raising EHOSTUNREACH — the bug the PR sets out to fix. The pruning code itself is sound; it's the trigger that looks misaligned.

This repo's _listener.py is Cythonized and I'm reasoning from stdlib asyncio behavior — uvloop or platform specifics could differ. Worth confirming before merge:

  • Does connection_lost actually fire on interface-down in a real repro (not just the synthetic protocol.connection_lost(None) call in the test)? An integration/manual repro would settle it.
  • If not, consider pruning from error_received() after N consecutive EHOSTUNREACH for the same socket, or lean on the "later interface rescan" the PR mentions as the real recovery path and scope the description accordingly.
def connection_lost(self, exc: Exception | None) -> None:
    """Prune this transport from the engine so a dead socket is not reused."""
    self.zc.engine._async_remove_listener(self)

🟢 Suggestions

1. Docstring carries rationale that CLAUDE.md says belongs in the PR/commit
src/zeroconf/_engine.py:130-135

Per this repo's CLAUDE.md code-style rules, docstrings should be terse and default to single-line, and rationale/motivation ("so a transport that dies ... instead of raising EHOSTUNREACH ...") belongs in the PR description and commit message, not the docstring.

Consider collapsing to a one-liner, e.g. """Drop a listener and its wrapped transports from the engine lists.""". Minor — does not affect behavior.


Checklist

  • Logic correct for connection-loss pruning
  • Trigger fires in the targeted failure mode — warning #1
  • Thread/free-threading safety
  • No mutation-during-iteration hazard
  • Cython .pxd updated where needed (_engine not cythonized; new listener methods not in .pxd)
  • Tests cover new branches
  • Docstring/comment style per CLAUDE.md — suggestion

To rebase specific severity levels, mention me: @bluetoothbot rebase critical (fixes 🔴 only), @bluetoothbot rebase important (fixes 🔴 + 🟡), or just @bluetoothbot rebase for all.


Automated review by Kōan (Claude) HEAD=37e3ceb 3 min 3s

@bluetoothbot bluetoothbot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issues found.

  • connection_lost likely never fires for the EHOSTUNREACH scenario this PR targets

@bdraco

bdraco commented Jun 21, 2026

Copy link
Copy Markdown
Member Author

Closing per maintainer review. As Kōan correctly flagged, EHOSTUNREACH/ENETUNREACH on send route to asyncio's error_received(), not connection_lost() (verified against CPython _SelectorDatagramTransport), so this hook never fires for the targeted scenario. These errnos are transient route conditions and should not trigger pruning of an otherwise-good interface. The runtime rescan API (#1797) and the optional monitor (#1798) cover genuine interface add/remove/IP-change, and are being reworked to stand alone on master without this change.

@bdraco bdraco closed this Jun 21, 2026
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