Repository navigation
fix(mdns): advertise only served addresses on Linux - #5914
Conversation
ReenigneArcher
left a comment
There was a problem hiding this comment.
I'll review fully after docs build errors (not gh-pages preview) and sonar issues are resolved.
ReenigneArcher
left a comment
There was a problem hiding this comment.
The current head resolves the earlier Sonar findings, but the address-family change does not filter the service's advertised host addresses, and the documentation build still fails on duplicate parameter documentation. Please address the two substantive inline findings; the test include indentation also needs the formatter's small correction.
Validation: compiled the actual changed publish.cpp on macOS and ran both PublishProtocolTest cases against that object, with a wrapper enabling the Linux test guard after loading the headers. Both passed. Whitespace checks passed; clang-format 23.1.0 flags the test include block. Inspected the current-head Read the Docs log and Avahi's upstream implementation. Current master still uses PROTO_UNSPEC, so this fix has not been superseded.
Verification limits: no full Sunshine build/suite or live Avahi reproduction. The local Linux containers could not start. GitHub CI/lint/CodeQL await maintainer approval, and the GH-Pages preview timed out waiting for those checks.
Head 71bfc2ac26e5724ea8adf6f2b5cd983b6c055f73 is mergeable with current master (d492aa5688470b2e1642118403bcfe1167cf5b8f).
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
|
Addressed all three findings in commit 55061f4. P1 — filter the advertised host addresses, not the transport. The service now publishes a service-specific host name ( P2 — Doxygen duplicate. Resolved by removal: the helper whose definition duplicated the declaration's P3 — clang-format. Applied the suggested include-block indentation in Tests. Replaced the two mapping assertions with unit tests for the new selection layer: IPv4-only selection contains no IPv6 address (the regression core for #5860 — no IPv6 selected means no AAAA record published for the service host), Verification done / limits. Disclosure. This work was done by an AI agent on Daniel's behalf; Daniel did not personally write it. No application or program goal is mentioned here because none applies to this change — it is just the mDNS fix. |
|
SonarCloud follow-up in 1d6457f: fixed the mechanical findings, left the ones that can't hold without changing behavior. No behavior change intended. Fixed in this commit
Left as-is
Verification done
Verification limits
This work was done by an AI agent on Daniel's behalf; Daniel did not personally write it. |
|
Correction to my comment above (self-review): I misread the Sonar API list. The 5 issues without line numbers (S7035 x3, S6177, S3642) are CLOSED — created during earlier analyses of this PR and closed 2026-10-13 18:51–22:00 UTC, before the 55061f4 push. They are not open findings on the current head. The open set is exactly the 11 line-numbered findings, matching the bot's "11 new issues". What this changes:
This work was done by an AI agent on Daniel's behalf; Daniel did not personally write it. |
|
SonarCloud re-analysis on
The other 4 are pre-existing patterns this PRs diff surfaced, left intentionally:
So the Quality Gate will still show those 4. Happy to address them differently if you prefer — e.g. a follow-up restructuring the hooks/global state — but I did not want to churn them inside this fix. Verification limits: no full build or clang-format binary in this environment (CI/Sonar is the gate); diff re-read, rename-only.
|
ReenigneArcher
left a comment
There was a problem hiding this comment.
Re-reviewed 73c5cf8ad6f944bc219dac1a7811a9e7e62df911, including all commits since 71bfc2a and the complete PR changes against current master (81a677c). The previous transport-selector, duplicate-Doxygen, and formatting findings are addressed. The new address-registration design introduces three regressions described inline: reverse-PTR collisions prevent registration, interface scope is lost, and address records become stale after network changes.
Validation: all 9 current helper gtests passed against the actual publish.cpp/network.cpp objects on macOS, with a wrapper enabling the Linux test guard after loading the headers. All three changed files pass clang-format 23.1.0 and whitespace checks. A harness using Avahi's actual upstream conflict checker reproduced the reverse-PTR collision. A separate harness using the current Sunshine sources, synthetic interfaces, injected Avahi calls, and redirected logging reproduced the registration failure, unrestricted interface arguments, and lack of refresh after an address change. The PUBLISH_NO_REVERSE suggestion compiled and allowed registration in that harness. These are focused native harnesses, not a full Sunshine build/suite or a live-daemon Linux test.
The Sunshine documentation check now passes. The Sonar quality gate still fails on the four reported mutable-global/POSIX-cast code smells; the contributor's explanations match the runtime assignments and family-guarded casts, and their recorded gate disposition remains outstanding. GitHub CI/lint/CodeQL remain awaiting maintainer approval, and the GH-Pages preview log confirms it timed out waiting for those checks; hosted build/test verification is therefore incomplete.
GitHub confirms conflicts with master. Please rebase and resolve the conflicts before merging, and update the PR description to describe the current host-address design.
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
73c5cf8 to
ff98a49
Compare
|
All three findings addressed in [P1] Reverse-PTR collision — fixed. [P1] Interface scope lost — fixed. New [P2] Stale records after network change — fixed. SonarCloud gate disposition (recorded, not claimed as passing). The gate still fails on code smells left as-is, and the fresh analysis on the new head was still pending when I checked, so I am not claiming it passes:
Verification (honest limits). A standalone harness driving the real Implemented with AI assistance (Claude, via an agent working on behalf of the contributor). Review was a self-review pass over the final diff, not an independent review. |
…, emplace_back) SonarCloud Quality Gate on 27f1d01 reported 32 new issues. This commit fixes the 3 that are genuinely correct without changing behavior: - S3642: enum WatchEvent -> enum class WatchEvent : int (explicit int underlying type keeps the AvahiWatchEvent layout); single use site qualified as WatchEvent::WATCH_IN. - S6003 (x2): push_back({...}) -> emplace_back(...) in enumerate_interface_addresses. The remaining 29 findings are left as-is with per-finding reasons in the PR reply: S5421 x25 (loader-table pointers assigned at runtime by dyn::load, or state mutated after init -- none can be const), S3642 x1 (PublishFlags moved verbatim from upstream; enum-class conversion would refactor upstream code), S3630 x2 (family-checked sockaddr downcast; no safer C++ cast compiles), S5008 x1 (void* required by the Avahi watch-callback signature). AI-assisted: drafted with AI assistance and self-reviewed; no independent reviewer was available in this environment. No behavior change intended. Full build/gtest/live-daemon not run here; CI is the gate.
|
SonarCloud follow-up in Line numbers below are Sonar's on head Fixed in this commit (3)
Left as-is (29) S5421 x25 — none of these can be
S3642 x1 — publish.h:60 S3630 x2 — publish.cpp:368, 373 S5008 x1 — publish.cpp:605 Verification (honest limits)
|
5020c08 to
de1db21
Compare
…, emplace_back) SonarCloud Quality Gate on 27f1d01 reported 32 new issues. This commit fixes the 3 that are genuinely correct without changing behavior: - S3642: enum WatchEvent -> enum class WatchEvent : int (explicit int underlying type keeps the AvahiWatchEvent layout); single use site qualified as WatchEvent::WATCH_IN. - S6003 (x2): push_back({...}) -> emplace_back(...) in enumerate_interface_addresses. The remaining 29 findings are left as-is with per-finding reasons in the PR reply: S5421 x25 (loader-table pointers assigned at runtime by dyn::load, or state mutated after init -- none can be const), S3642 x1 (PublishFlags moved verbatim from upstream; enum-class conversion would refactor upstream code), S3630 x2 (family-checked sockaddr downcast; no safer C++ cast compiles), S5008 x1 (void* required by the Avahi watch-callback signature). AI-assisted: drafted with AI assistance and self-reviewed; no independent reviewer was available in this environment. No behavior change intended. Full build/gtest/live-daemon not run here; CI is the gate.
ReenigneArcher
left a comment
There was a problem hiding this comment.
Rereviewed de1db212baa9f5924c698eb1eea5e8219aa319a6, including the complete target-branch diff and the changes since my previous review. The reverse-PTR fix, ordinary per-interface publication, and address-change subscription address the earlier broad scenarios. Two narrower runtime cases and the documentation error remain below.
Validation: all 16 PR tests pass in a focused native Linux component harness compiling the current publisher and network source with real Boost headers. Three added regressions fail on this head; the proposed runtime corrections pass all 19. A live Avahi 0.8 smoke test resolves the IPv4 service alias on the container's network interface, exposes no IPv6 endpoint for that alias, and verifies netlink-watch installation and cleanup. Formatting and whitespace checks pass. The harness supplies platform/configuration support and replaces logging; this is not a full Sunshine build, full test suite, or remote Moonlight session.
Workflow status: Sunshine documentation fails on the callback below. Sonar's actual analysis gate still fails with 29 open code smells; the rationale in the PR discussion has not changed their disposition in Sonar. Those findings need resolution or an accepted maintainer disposition. Actions awaiting maintainer approval and the dependent preview timeout limit hosted verification.
The PR is currently mergeable against master (178a0467da1dde750b182be4ae10ff8f8245ad01); GitHub reports it behind, with no conflicts.
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
… invalidation, docs) Address the three P2 findings from the 2026-10-15 re-review: - Preserve the IPv6 zone (sin6_scope_id) when enumerating interface addresses, so a scoped link-local bind (e.g. fe80::1%7) resolves to its owning interface instead of falling back to IF_UNSPEC. - Track the committed snapshot as std::optional: reset it before each entry-group reset and at the start of a fresh registration attempt, so a failed refresh followed by restoring the previous snapshot republishes instead of early-returning on stale state. This also distinguishes a withdrawn group from a successfully published empty snapshot (daemon-hostname fallback). - Document address_change_callback() with Doxygen so the docs build can pass. Add three regressions: scoped link-local bind resolution, A -> failed B -> A republication (nonempty A), and A -> failed B -> A republication (empty A). The 29 previously-dispositioned SonarCloud code smells are unchanged by this commit and still await maintainer disposition. This work is AI-assisted; the author did not personally write it.
|
Thanks for the re-review. This push (7cb9f82) addresses all three P2 findings: 1. IPv6 zone preserved on enumeration — 2. Snapshot invalidated on withdrawal — 3. Poll callback documented — Sonar: the 29 previously-dispositioned code smells are untouched by this commit; their disposition is unchanged and still awaits maintainer disposition. Verification, with honest limits — disclosure: this work is AI-assisted; it was not personally written by the account owner. I verified the logic with a focused harness replicating the changed-function logic (real |
|
SonarCloud re-analysis on I checked the T42 diff against the flagged lines: no new finding is attributable to this push — the flagged declarations (loader table, AI-assisted (Muse), self-reviewed; full build/gtest/live-daemon not run locally — CI is the gate. |
With address_family = ipv4, Avahi was registered with PROTO_UNSPEC, so it also published an IPv6 link-local record nothing was listening on and IPv6-preferring Moonlight clients stalled on connect. Map the configured (effective) address family to the Avahi protocol selector so the announcement only contains addresses Sunshine actually serves. Fixes LizardByte#5860.
SonarCloud (cpp:S3642) flagged the plain enum in publish.h. Convert proto to a scoped enum and update the three return sites and the unit test to qualify enumerators / cast to avahi::Protocol.
Addresses SonarCloud cpp:S7035 (use std::to_underlying to cast enums) introduced by the enum class conversion, and cpp:S6177 via using enum in protocol_for_address_family.
The PROTO_INET mapping only restricted mDNS transport: with a null SRV target the daemon still answered with both A and AAAA records for its shared host name (publish-aaaa-on-ipv4), so an IPv4-only configuration could still hand clients an unserved IPv6 endpoint (LizardByte#5860). Publish a service-specific host name (<host>-sunshine.local) with only the addresses Sunshine serves, via avahi_entry_group_add_address, and use it as the SRV target. Discovery transport returns to PROTO_UNSPEC on both the service and the address records. The served addresses are the explicit bind address when configured, otherwise the selected local interface addresses for the effective family (family-filtered, no loopback/unspecified, deduplicated). Host-name collisions regenerate the service host name alongside the service name. Also drop protocol_for_address_family (superseded by the redesign, which also removes the duplicated @PARAM docs), and fix the test include-block indentation per clang-format 23.1.0.
Fix the mechanical SonarCloud findings on the service-specific host-name code: explicit lambda capture, std::format for the collision suffix, if init-statement for MAX_LABEL, const-ref candidates, std::ranges::copy range overload, using enum for avahi::proto, and flatten the served_addresses / create_services nesting with early returns and an add_service_addresses helper. Left as-is with reasons in the PR comment: the function-pointer globals (assigned at runtime via dlsym), the collision counter (incremented at runtime), the sockaddr reinterpret_casts (required for POSIX sockaddr interop), the S7035 hits (no enum-to-underlying cast exists in the file), and S3642 (the only enum in publish.h is already enum class).
…s, refresh on network change
P1 reverse-PTR collision: publish the service-host alias with PUBLISH_NO_REVERSE so the daemon's default publish-addresses=yes reverse PTR does not collide (ERR_COLLISION) and kill the poll loop. Regression: address-call stub rejects a competing reverse PTR unless the flag is present; asserts registration + commit reached and the poll loop stays active.
P1 interface scope lost: carry the owning interface index in a new platf::publish::ServedAddress {interface, address} through enumeration, selection (dedup by interface+address), and publication; enumerate populates it with if_nametoindex(ifa_name) (IF_UNSPEC fallback), explicit bind resolves its owning interface the same way, and add_service_addresses() passes address.interface. Discovery protocol stays PROTO_UNSPEC. Regression: two synthetic interfaces incl. distinct link-local IPv6 assert each call uses its owning index.
P2 stale records after network change: refresh_service_addresses() compares the current served snapshot against the last published snapshot and, on change, resets the entry group (withdrawing stale records) and republishes; unchanged snapshots are a no-op. The trigger is a netlink route socket (RTMGRP_LINK | RTMGRP_IPV4_IFADDR | RTMGRP_IPV6_IFADDR) registered as an Avahi poll watch, so the callback runs on the existing poll thread; Sunshine has no reusable network-change notification (only UPnP's unrelated WAN-IP renewal). Install is best-effort and shutdown frees the watch + socket after the poll thread joins. A refresh failure does not quit the poll loop (create_services gains quit_on_failure, threaded through the collision retry), so a transient failure cannot leave the service withdrawn with no recovery. Regression: register .10, change snapshot to .20, refresh, verify withdrawal/republication with no stale .10, plus a collision-during-refresh-retry case.
Also: rebased onto master 81a677c (conflict in the init_client loader table resolved as master's reformat + the PR's entry_group_add_address line); Avahi function-pointer declarations moved to publish.h so registration tests can stub them; SUNSHINE_TESTS-gated seams expose the publisher state for those tests; Doxygen stays on declarations only.
Implemented with AI assistance (Claude, via an agent working on behalf of the contributor). Verified with a standalone harness driving the real publish.cpp logic with stubbed Avahi calls (44 checks, 0 failures; fail-before confirmed 11 failures on the unfixed logic) and clang-format 23.1.3 clean. Full CMake build, gtest suite, and live-daemon test NOT run in this environment; CI is the verification gate.
…, emplace_back) SonarCloud Quality Gate on 27f1d01 reported 32 new issues. This commit fixes the 3 that are genuinely correct without changing behavior: - S3642: enum WatchEvent -> enum class WatchEvent : int (explicit int underlying type keeps the AvahiWatchEvent layout); single use site qualified as WatchEvent::WATCH_IN. - S6003 (x2): push_back({...}) -> emplace_back(...) in enumerate_interface_addresses. The remaining 29 findings are left as-is with per-finding reasons in the PR reply: S5421 x25 (loader-table pointers assigned at runtime by dyn::load, or state mutated after init -- none can be const), S3642 x1 (PublishFlags moved verbatim from upstream; enum-class conversion would refactor upstream code), S3630 x2 (family-checked sockaddr downcast; no safer C++ cast compiles), S5008 x1 (void* required by the Avahi watch-callback signature). AI-assisted: drafted with AI assistance and self-reviewed; no independent reviewer was available in this environment. No behavior change intended. Full build/gtest/live-daemon not run here; CI is the gate.
… invalidation, docs) Address the three P2 findings from the 2026-10-15 re-review: - Preserve the IPv6 zone (sin6_scope_id) when enumerating interface addresses, so a scoped link-local bind (e.g. fe80::1%7) resolves to its owning interface instead of falling back to IF_UNSPEC. - Track the committed snapshot as std::optional: reset it before each entry-group reset and at the start of a fresh registration attempt, so a failed refresh followed by restoring the previous snapshot republishes instead of early-returning on stale state. This also distinguishes a withdrawn group from a successfully published empty snapshot (daemon-hostname fallback). - Document address_change_callback() with Doxygen so the docs build can pass. Add three regressions: scoped link-local bind resolution, A -> failed B -> A republication (nonempty A), and A -> failed B -> A republication (empty A). The 29 previously-dispositioned SonarCloud code smells are unchanged by this commit and still await maintainer disposition. This work is AI-assisted; the author did not personally write it.
7cb9f82 to
587397c
Compare
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5914 +/- ##
==========================================
+ Coverage 48.72% 48.90% +0.18%
==========================================
Files 112 113 +1
Lines 24576 24769 +193
Branches 10622 10702 +80
==========================================
+ Hits 11974 12113 +139
+ Misses 10191 10184 -7
- Partials 2411 2472 +61
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Harness.
|
Screenshot ComparisonPR #5914 screenshots vs Matrix:
|
|
Looks like some platform dependent logic is needed for FreeBSD. https://github.com/LizardByte/Sunshine/actions/runs/38825784043/job/116734075473?pr=5914#step:6:134 |
The FreeBSD CI job failed at the Build step compiling src/platform/linux/publish.cpp: 'linux/netlink.h' file not found. Netlink is a Linux-kernel API, so guard the netlink includes, members, callback, install body, and deinit cleanup with \#ifdef __linux__. On other platforms install_address_change_watch() returns false (best-effort, no watch): discovery still works and records just don't auto-refresh on network changes, exactly the pre-watch behavior. Linux behavior is unchanged. The publish.h Doxygen for install_address_change_watch() now notes it is Linux-only. This work is AI-assisted; the author did not personally write it. Verification limits: no FreeBSD machine or full Sunshine build was available locally. Verified by preprocessing the real publish.cpp with __linux__ undefined and poisoned netlink headers (succeeds, zero netlink symbols in output, install body is 'return false'), plus controls showing the Linux path still includes netlink and keeps the full watch logic. clang-format checked with the repo's .clang-format. FreeBSD CI is the gate for the real build.
|
Thanks for flagging this — and a correction to my earlier reading of the failure. Correction: I previously described this as a packaging-step failure. That was wrong. Re-checking the log (CI run 38825784043, FreeBSD/amd64 job, Build step 6), it is a Build-step compile error: Root cause: the address-change watch added in this PR uses netlink, which is a Linux-kernel API, but Fix (pushed as Verification (honest limits): I have no FreeBSD machine and did not run a full Sunshine build locally. What I did verify: preprocessing the real A possible follow-up (not implemented here): a FreeBSD
|
|
SonarCloud re-analysis on head I checked each flagged line against the T44 diff: none is a line T44 added or changed (the T44 lines are the No code change for this — the QG failure is expected on the same left-as-is items. Full build/gtest/live-daemon not run locally; CI is the gate. (AI-assisted; self-reviewed — no independent reviewer.) |
ReenigneArcher
left a comment
There was a problem hiding this comment.
Rereviewed 44fb71150e17712f99c9e4e33f262a7f7f7d5912, including the rebase against current master and the changes since the approved revision. The guards remove netlink dependencies from the non-Linux code path. One FreeBSD runtime regression remains: disabling updates while retaining fixed custom-host address records makes discovery stale after an interface-address change. A tested option to preserve the existing FreeBSD behavior is included inline.
Validation: all 19 PR tests plus three independent regressions pass on the Linux path, with controlled scoped-IPv6 data. The added non-Linux fallback regression fails on this head and passes with the proposed correction; its Linux path still passes all 22 checks. Current files and the proposed patch pass formatting checks; whitespace checks pass. These are focused component builds with configuration/platform support and redirected logging, not a full Sunshine or native FreeBSD build.
The original FreeBSD log confirms a compile failure at linux/netlink.h. Current Sunshine documentation passes. CI/lint/CodeQL await maintainer approval, and the preview log times out waiting for their checks, limiting hosted verification. Sonar now reports 26 intentional maintainability findings from the previously assessed categories; the earlier exception assessment still applies.
GitHub reports the current head mergeable with master (55ff34e21b7c4ee760b990f25c0778050c1d6035), with no conflicts.
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
…h is unavailable On non-Linux platforms install_address_change_watch() returns false (no netlink), but create_services() still published the service-specific host name (<host>-sunshine.local) plus fixed address records from a one-time snapshot. With no refresh, a DHCP change left discovery pointing at the stale address until restart. Keep the custom host name and its fixed records Linux-only, where the watch refreshes them. Other platforms keep the null SRV target, so the service uses the Avahi daemon-managed host name and the daemon owns the interface address records: it updates and withdraws them when addresses change. The non-Linux fallback comment, the start() comment, and the install_address_change_watch() Doxygen now describe this delegated address maintenance. served_addresses() is still computed on non-Linux (it compiles there); the committed-snapshot line is unchanged. Adds a platform regression: the test file is no longer entirely Linux-gated, and on non-Linux the new test asserts the watch is unsupported, the service call receives a null host, and no fixed alias address records are published. The lenient address stub is now Linux-gated (only the Linux tests install it). This work is AI-assisted; the author did not personally write it. Verification limits: no FreeBSD machine, full Sunshine build, gtest, or clang-format binary was available locally. Verified with a focused harness compiling the real publish.cpp + test file against stubbed dependencies: Linux config 19/19 pass; non-Linux config (__linux__ undefined, poisoned netlink headers) new test passes; the new test fails on the pre-fix code (fixed alias addresses published). No netlink symbols in the non-Linux object. Full build / gtest / FreeBSD CI is the gate.
|
Addressed the re-review finding (review 5542042465, inline comment 4290068153). Finding: [P2] Preserve FreeBSD address updates when disabling the watchImplemented the suggested option (b) — retain the daemon-managed hostname on non-Linux while keeping this fix Linux-scoped. No FreeBSD routing-socket watcher was added: the daemon-managed hostname delegates address maintenance to the Avahi daemon, which updates/withdraws the interface records, so a watcher is unnecessary.
VerificationDisclosure: this work is AI-assisted; the author did not personally write it. Review was self-review, not independent review (Grok Bot unavailable). Focused harness compiling the real
Head: |
ReenigneArcher
left a comment
There was a problem hiding this comment.
Rereviewed ecce8ab065a1e2e43792325b05c4190b200de495. The functional suggestion was applied correctly: non-Linux builds keep a null SRV target and do not publish fixed alias addresses. Continuing to calculate the served-address snapshot there does not change the advertised hostname or create address records. The previous FreeBSD stale-address finding is resolved. The only remaining correction is the required formatting of two new test directives, described inline.
Validation: all 19 PR tests plus three independent regressions pass on Linux with controlled scoped-interface data. The contributor's new non-Linux gtest and the independent fallback regression both pass against the actual publisher using real Boost and gtest. The non-Linux branch is selected after dependency headers are loaded; this is a focused component test, not a native FreeBSD build or full Sunshine suite. Source/header formatting and whitespace checks pass; the test file fails formatting only at the two reported guards, and the formatter's correction passes.
Sunshine documentation passes. CI/lint/CodeQL await maintainer approval, so hosted build/test verification remains incomplete. The earlier Sonar exception assessment still applies. Current head is mergeable with master (55ff34e21b7c4ee760b990f25c0778050c1d6035), with no conflicts.
Review completed by Codex | Model: gpt-6.1-sol | Thinking: xhigh
Address reviewer feedback (review 5542281177, inline comment
4290244919, [P3]): the '#if defined(__linux__)' guard and its matching
'#endif' around FakeEntryGroupAddAddressLenient are nested inside
'namespace { }' and failed 'clang-format --dry-run --Werror'. Give both
directives the two leading spaces the repository configuration expects.
Whitespace-only change; no functional change.
AI-assisted with disclosure.
|
Addressed the re-review finding (review 5542281177, inline comment 4290244919, [P3]) in commit 1c800cc. Finding: format both newly nested Linux guardsApplied the reviewer's exact correction: the Verification
AI-assisted with disclosure. Self-review only (no independent reviewer available). |
|
SonarCloud re-analysis on Verified 0 attributable to T46 ( AI-assisted triage; no independent reviewer available in this environment (self-review only). Full build/gtest/live-daemon not run — CI is the gate. |
|




























































Description
Fixes #5860.
With
address_family = ipv4, Sunshine's mDNS registration passedPROTO_UNSPECto Avahi, telling the daemon to publish the service for all protocols — so it advertised an IPv6 link-local record that nothing was listening on. IPv6-preferring Moonlight clients picked that record, TCP connect hung, and discovery stalled with no fallback to IPv4.This PR publishes the service under a service-specific host name (
<host>-sunshine.local, distinct from the daemon's shared host name) with only the addresses Sunshine actually serves:select_served_addresses()keeps only addresses of the effective family (which accounts forbind_address, matching the listener setup), drops unspecified/loopback addresses, and deduplicates by interface + address. Discovery transport staysPROTO_UNSPECon both protocols: transport only decides which multicast groups carry the announcement, it does not filter the advertised host addresses.ServedAddress {interface, address}), populated withif_nametoindex()during enumeration; an explicit bind address resolves its owning interface the same way (IF_UNSPECwhen no local interface owns it). Records are published on their owning interface, so LAN clients no longer receive a Docker-bridge address (or a foreign link-local IPv6 address) under the same SRV target.PUBLISH_NO_REVERSE: with the daemon's defaultpublish-addresses=yesthe daemon already owns the reverse PTR for each local IP, and a competing unique reverse PTR for the alias would collide (ERR_COLLISION) and kill the poll loop before registration.RTMGRP_LINK | RTMGRP_IPV4_IFADDR | RTMGRP_IPV6_IFADDR) is registered as an Avahi poll watch, so address/link changes dispatch a refresh on the existing poll thread (no new thread, no cross-thread Avahi calls). The refresh compares the current served snapshot against the last published one and, on change, resets the entry group (withdrawing stale records) and republishes; unchanged snapshots are a no-op. Install is best-effort — if the watch cannot be installed, discovery works as before except records go stale after a network change. A refresh failure does not quit the poll loop (a transient failure must not leave the service withdrawn with no recovery short of restarting Sunshine).Deliberate scope calls for reviewer attention: macOS/Windows are untouched (their APIs offer no equivalent per-interface address publication). The SonarCloud quality gate still fails on code smells left as-is with reasons (see the review reply):
S5421on the mutable Avahi function-pointer globals (assigned by the dynamic loader and by tests) and on thegroupglobal (mutated by Avahi callbacks), andS3630on the two family-guardedsockaddrdowncasts (the family is checked immediately before; no safer operation exists). The three new globals in this round (published_addresses,netlink_fd,netlink_watch) fall under the sameS5421disposition.Screenshot
Issues Fixed or Closed
Roadmap Issues
Testing
Standalone harness driving the real
publish.cpplogic with stubbed Avahi calls (Boost address shim; full CMake build unavailable in this environment): 44 checks, 0 failures. Fail-before was confirmed on the unfixed logic (11 failures across the three findings). Coverage:PUBLISH_NO_REVERSEpresent on the address call (stub rejects a competing reverse PTR otherwise); each captured address call uses its owning interface (LAN + Docker bridge + distinct link-local IPv6); register.10→ change snapshot to.20→ refresh withdraws/republishes with no stale.10, unchanged-snapshot refresh is a no-op, and a collision during a refresh retry does not quit the poll loop; netlink-watch install + callback drain + dispatch verified. Newtests/unit/test_publish.cppregressions mirror the harness (registration tests with stubbed Avahi calls, including the collision-retry case).clang-format --dry-run --Werrorclean (23.1.3). The full CMake/Ninja build, gtest suite, and live-Avahi end-to-end test were NOT run in this environment, so please treat CI as the verification gate.Type of Change
Checklist
AI Usage
See our AI usage policy.
This change was implemented with AI assistance (Claude, via an agent working on behalf of the contributor). Logic verified with a standalone harness driving the real publish.cpp logic with stubbed Avahi calls (44 checks, 0 failures; fail-before confirmed); the full local build, gtest suite, and live-Avahi test were not run, so CI is the verification gate.