feat: add async_update_interfaces to rescan network interfaces at runtime - #1797

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.81%. Comparing base (5be5d60) to head (c2df902).
⚠️ Report is 1 commits behind head on master.

@@            Coverage Diff             @@
##           master    #1797      +/-   ##
==========================================
+ Coverage   99.77%   99.81%   +0.03%     
==========================================
  Files          33       33              
  Lines        3540     3750     +210     
  Branches      498      533      +35     
==========================================
+ Hits         3532     3743     +211     
  Misses          5        5              
+ Partials        3        2       -1     

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

Merging this PR will not alter performance

✅ 21 untouched benchmarks

_{Comparing feat/async-update-interfaces (c2df902) with master (797fdc0)}

Previous review — superseded by a newer review below.

Thanks, both review points addressed in the latest push:

IPv6 link-local diff key collision (bluetoothbot + Copilot): _interface_bind_address and _wrapped_bind_address now return (address, scope_id) instead of the bare address string, so the same link-local address on two interfaces no longer collapses to one key. The desired side reads the scope_id from the normalized interface tuple and the current side reads it from getsockname()[3], which are the same value the socket binds with, so an unchanged interface still matches itself across a rescan. Helper tests updated to assert the tuple key and a non-collision case.

drop_multicast_member WSAEINVAL (bluetoothbot + Copilot): now mirrors add_multicast_member and adds WSAEINVAL to the benign set on win32. New tests/utils/test_net.py::test_drop_multicast_member_wsaeinval covers it.

Also added test_update_interfaces_keeps_unchanged_sender_untouched, which pins the core invariant: an interface present in both the old and new set keeps its exact transport object (no teardown, no rebind), while only the gone interface's transport is closed.

@bdraco: Thanks, both review points addressed in the latest push:

Verified. Scope_id keying sound. Desired reads interface[0][2]; current reads getsockname()[3]. Same value socket binds with. Link-local collision resolved. Dual-use guard and rollback correct. Three minor suggestions left inline: missing log on IPv6 inet_pton failure, unsurfaced per-interface add failure, silent duplicate sender on Default-to-list switch.

Previous review — superseded by a newer review below.

Previous review — superseded by a newer review below.

Good catch on the IPv6 membership asymmetry — fixed in the latest push.

Important #1 (IPv6 leave used scope_id where join used the index): the responder socket already holds the join index in IPV6_MULTICAST_IF (set by new_respond_socket), so I capture it at wrap time. _WrappedTransport now carries multicast_index, read via getsockopt(IPPROTO_IPV6, IPV6_MULTICAST_IF) in make_wrapped_transport, and multicast_interface uses it for the v6 leave instead of the bound scope_id. Leave and join are now symmetric for global IPv6 (scope_id 0, index N) as well as link-local, and it works for both startup-created and rescan-added senders since both go through make_wrapped_transport. The diff key (interface_key) stays scope_id-based since both sides derive from the same bind, as you noted.

New tests:

• test_wrapped_multicast_interface now pins that the leave value carries multicast_index (9), not the bound scope_id (5).
• test_make_wrapped_transport_captures_v6_multicast_index confirms the index is read via getsockopt.
• test_update_interfaces_reconciles_mixed_set drops a global-style v6 sender and asserts the leave uses its join index.

Suggestion #1: drop_multicast_member now logs the same IPv6-disabled info line its inverse emits before returning False.

Suggestion #3 / silent partial failure: _async_add_interface now logs (debug) naming the interface when a join fails or no responder socket is created. Surfacing the failed set to the caller via the return type would be a larger API change; left as-is to match construction semantics (the constructor also skips unusable interfaces), but the per-interface skip is now observable.

Good catch on the startup blast radius. Fixed in 37f6b1a:

• make_wrapped_transport no longer re-raises a failed IPV6_MULTICAST_IF read on non-Windows. It runs on the startup/connection path (connection_made for every transport), and the index it protects only selects the interface for a future group leave that drop_multicast_member already treats as benign, so a read failure now falls back to the default index 0 with a debug log instead of aborting setup.
• _listen_socket_supports gets the same graceful fallback (assume dual-stack + debug log) so a getsockopt failure can't abort a rescan, and the two paths now agree. This supersedes the re-raise from the previous round; degrading is the right trade given make_wrapped_transport can't safely abort startup.
• Softened the _core config-commit comment, which overstated the invariant: a mid-reconcile failure may have changed some sockets, so the comment now just says the stored config is left unchanged rather than claiming it matches the bound sockets.

On the two silent-failure notes (surface failed/degraded interfaces to the caller): keeping that best-effort by design for now; the per-interface warnings name the interface, and an aggregate failed-interface return is a deliberate API addition I'd do as a follow-up rather than fold into this PR.

Previous review — superseded by a newer review below.

Addressed in c93f537:

• The IPV6_V6ONLY read-failure fallback now logs at warning, not debug. You're right that assume-dual-stack is the risky direction (skips a needed rebuild, strands a v4 family) and that interface updates may never recur; I kept assume-dual-stack rather than force-rebuild because returning False could loop rebuilds when the rebuilt dual-stack socket's read also fails, but warning makes a real failure visible. Comment updated to spell out that trade.
• Documented best-effort bring-up in the async_update_interfaces docstring: a requested interface that fails to bind or re-join is logged, not raised (alongside the existing best-effort re-announce note).
• Added the clarifying comment for the benign group leave on the rebuilt listen socket in the close loop.

The programmatic partial-reconcile signal (return failed interfaces) stays a deliberate follow-up, as you noted; no API change in this PR.

Previous review — superseded by a newer review below.

All three addressed in 423f875:

• Important Fix ServiceInfo __repr__ #1 (transient empty set): an All/Default instance that momentarily resolves to zero addresses during adapter churn made normalize_interface_choice raise straight into the caller's handler. Now caught as a logged no-op (current sockets left intact), so a transient down state doesn't crash the adapter-change handler. Test added.
• Suggestion Fix ServiceInfo __repr__ #1 (endpoint-creation failure aborting the reconcile): _async_add_interface now rolls back and skips with log_warning_once instead of re-raising, so other interfaces still come up and get re-announced. This matches the documented best-effort contract and the add_interface-is-None path. The rebuild's listen-socket wrap failure still raises (can't continue without a listen socket). Tests updated.
• Silent-failure (make_wrapped_transport debug log): a non-Windows IPV6_MULTICAST_IF read failure now logs at warning (Windows WSAEINVAL stays debug via a platform-branched level), since a wrong-index leave silently leaks the membership.

Previous review — superseded by a newer review below.

On the two notes:

MEDIUM #1 (bare Exception) - fixed in 55ca5ea. Good catch; that was a regression from making the wrap failure best-effort. _async_add_interface now rolls back on any failure but only swallows OSError; a non-OSError (TypeError/AttributeError etc.) propagates so a real bug isn't downgraded to a deduped 'interface not added' warning. Added a test that a TypeError propagates after rollback.

MEDIUM #2 (_listen_socket_supports assume-dual-stack) - holding deliberately. This exact line has now been flagged in four directions across reviews (silent True, re-raise on non-win32, degrade to True, warning, now force-rebuild). force-rebuild is not strictly better: returning False on a getsockopt failure rebuilds to a dual-stack AF_INET6 socket whose IPV6_V6ONLY read fails the same way, so the next rescan rebuilds again - churning the listen socket on every reconcile. getsockopt(IPV6_V6ONLY) on a valid AF_INET6 socket effectively never fails on any platform, so this branch is defensive against the near-impossible; assume-dual-stack is the non-looping choice and the warning makes the rare event visible. A real fix (cache V6ONLY at socket creation, or a caller-visible degraded-family signal) is the deferred API follow-up, not a log-level or fallback-direction tweak. Leaving as-is unless a maintainer wants the churn tradeoff.

@bdraco: On the two notes:

Agree on holding MEDIUM #2. force-rebuild loops: rebuilt AF_INET6 socket fails same getsockopt, churns every rescan. assume-dual-stack non-looping; warning surfaces the rare failure. Correct call. Real fix is caching V6ONLY at socket creation, a follow-up.

@bdraco: On the cross-family finding (Important #1): rather than reject family changes, this now supports...

Verified rebuild rollback and staying-interface re-join — sound. One gap: rebuild failure raises through the best-effort API after demoting the dual-use socket from senders. Leaves degraded partial state, crashes adapter-change handler. See inline comment. Demote after rebuild succeeds, or log-and-return-False like the transient empty-set branch.

Previous review — superseded by a newer review below.

Important #1 - fixed in f0090e4. Genuinely good catch. The Default dual-use conversion demoted the socket from senders before the fallible rebuild, so a rebuild failure left it pulled-but-not-replaced (instance goes silent) and the exception crashed the caller's handler.

The fix: that explicit demote was redundant. _async_rebuild_listen_socket already removes the old transport from senders via _async_remove_transport, but only after the new socket is successfully created and wrapped. So I dropped the pre-demote (keeping only the local diff-view current.pop) and wrapped the rebuild in try/except -> log + return False. Now a rebuild failure (transient bind/endpoint failure during churn) leaves senders untouched (no degraded state) and is a logged no-op rather than a crash, consistent with the transient-empty-set branch. Added tests for both the family-rebuild failure and the dual-use-conversion failure leaving the dual-use sender intact.

Thanks for confirming the MEDIUM #2 hold (assume-dual-stack) - agreed, force-rebuild would churn. The remaining silent-failure note (send-only after a failed re-join) and the partial-failure return value stay the documented best-effort follow-up.

Previous review — superseded by a newer review below.

Addressed the two actionable items in c2df902:

• Silent-failure Redesign listener system #3 (new): gather(return_exceptions=True) also captures BaseExceptions like CancelledError, which the isinstance(result, Exception) check skipped, so a cancelled re-announce vanished. Now a captured BaseException is re-raised rather than swallowed (and a real Exception is still logged per-service). Test added that a CancelledError propagates.
• Suggestion Redesign listener system #3: documented on update_interfaces / async_update_interfaces / the AsyncZeroconf delegate that apple_p2p on a non-Apple platform raises RuntimeError (input validation, matching the constructor).

MEDIUM #1 (assume-dual-stack) and #2 (send-only after failed re-join) remain held as the documented best-effort tradeoffs we agreed on; the aggregate partial-failure return is the post-PR follow-up.

PR Review — feat: add async_update_interfaces to rescan network interfaces at runtime

Solid, carefully-reconciled implementation. No new blocking issues found; the prior review rounds already caught and fixed the substantive bugs.

Specific things done well:

• The (address, scope_id) keying via interface_key/_interface_key correctly disambiguates link-local IPv6 addresses that repeat across interfaces, and the join/leave symmetry (capturing IPV6_MULTICAST_IF into multicast_index so the leave uses the index the join did, not the bound scope_id) is the right fix for a genuinely subtle asymmetry.
• Failure handling is consistently rollback-oriented: _async_add_interface and _async_rebuild_listen_socket both close the unadopted socket and drop the just-joined membership on wrap failure, re-raising non-OSError so real bugs aren't masked.
• cancel_pending_timers closes a real hazard (a TC-reassembly timer firing against a closed transport); I verified the stated invariant — every _timers addr also has a _deferred entry, so iterating _deferred covers all timers — holds against the timer-arming code.
• The _drop_deferred refactor with its matching _listener.pxd cdef declaration is correct; _transport.py is not in TO_CYTHONIZE so the new __slots__ entry needs no .pxd change.
• Re-entrancy lock scoped to the reconcile only (re-announce runs unlocked), config committed only after the engine reconcile succeeds, and the BaseException re-raise in the gather loop are all reasonable.

What to be aware of (non-blocking):

• Persistently-degraded interfaces (failed re-join after rebuild; assume-dual-stack on IPV6_V6ONLY read failure) are surfaced once via log_warning_once and then invisible — documented best-effort tradeoffs with a follow-up planned.

Note on scope: net.py (drop_multicast_member, add_interface), asyncio.py, and the test files were omitted from the visible diff, so I could not independently verify them — the test plan is thorough on paper.

🟢 Suggestions

if bind_address in current and not add_multicast_member(new_listen, interface):
    self.zc.log_warning_once(
        f"Interface {interface!r} could not re-join the multicast group "
        "on the rebuilt listen socket"
    )

Checklist

• IPv6 link-local diffing disambiguated by scope_id
• Multicast join/leave symmetry (index vs scope_id)
• Resource cleanup on error paths (sockets/memberships)
• Cython .pxd kept in sync with layout/signature changes
• Concurrency / re-entrancy safety
• Error handling does not swallow real bugs
• Degraded-state observability — suggestion ＃1
• net.py / asyncio.py / tests verified — omitted from visible diff — unverified

Silent Failure Analysis

try:
    normalized = normalize_interface_choice(interfaces, ip_version)
except RuntimeError as exc:
    log.warning("Skipping interface update; no interfaces available: %s", exc)
    return False

supported = True
try:
    supported = not listen_socket.getsockopt(socket.IPPROTO_IPV6, socket.IPV6_V6ONLY)
except OSError as exc:
    log.warning("Unable to read IPV6_V6ONLY, assuming dual-stack: %s", exc)
return supported

Automated review by Kōan (Claude) HEAD=c2df902 5 min 44s