Skip to content

fix(mdns): advertise only served addresses on Linux - #5914

Merged
ReenigneArcher merged 12 commits into
LizardByte:masterfrom
MrBeldum:fix/mdns-ipv6-filter-5860
Oct 16, 2026
Merged

ReenigneArcher merged 12 commits into
LizardByte:masterfrom
MrBeldum:fix/mdns-ipv6-filter-5860

Conversation

@MrBeldum

@MrBeldum MrBeldum commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

Description

Fixes #5860.

With address_family = ipv4, Sunshine's mDNS registration passed PROTO_UNSPEC to 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:

  • Address selection. select_served_addresses() keeps only addresses of the effective family (which accounts for bind_address, matching the listener setup), drops unspecified/loopback addresses, and deduplicates by interface + address. Discovery transport stays PROTO_UNSPEC on both protocols: transport only decides which multicast groups carry the announcement, it does not filter the advertised host addresses.
  • Per-interface scope. Each published address carries its owning interface index (ServedAddress {interface, address}), populated with if_nametoindex() during enumeration; an explicit bind address resolves its owning interface the same way (IF_UNSPEC when 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.
  • Forward records only. Address records are published with PUBLISH_NO_REVERSE: with the daemon's default publish-addresses=yes the 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.
  • Refresh on network change. A netlink route socket (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): S5421 on the mutable Avahi function-pointer globals (assigned by the dynamic loader and by tests) and on the group global (mutated by Avahi callbacks), and S3630 on the two family-guarded sockaddr downcasts (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 same S5421 disposition.

Screenshot

Issues Fixed or Closed

Roadmap Issues

Testing

Standalone harness driving the real publish.cpp logic 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_REVERSE present 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. New tests/unit/test_publish.cpp regressions mirror the harness (registration tests with stubbed Avahi calls, including the collision-retry case). clang-format --dry-run --Werror clean (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

  • feat: New feature (non-breaking change which adds functionality)
  • fix: Bug fix (non-breaking change which fixes an issue)
  • docs: Documentation only changes
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semicolons, etc.)
  • refactor: Code change that neither fixes a bug nor adds a feature
  • perf: Code change that improves performance
  • test: Adding missing tests or correcting existing tests
  • build: Changes that affect the build system or external dependencies
  • ci: Changes to CI configuration files and scripts
  • chore: Other changes that don't modify src or test files
  • revert: Reverts a previous commit
  • BREAKING CHANGE: Introduces a breaking change (can be combined with any type above)

Checklist

  • Code follows the style guidelines of this project
  • Code has been self-reviewed
  • Code has been commented, particularly in hard-to-understand areas
  • Code docstring/documentation-blocks for new or existing methods/components have been added or updated
  • Tests have been added or updated for new or modified functionality, including regression tests for bug fixes

AI Usage

See our AI usage policy.

  • AI tools were used while preparing this PR

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.

@ReenigneArcher ReenigneArcher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'll review fully after docs build errors (not gh-pages preview) and sonar issues are resolved.

@ReenigneArcher ReenigneArcher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread src/platform/linux/publish.cpp Outdated
Comment thread src/platform/linux/publish.cpp Outdated
Comment thread tests/unit/test_publish.cpp Outdated
@MrBeldum

Copy link
Copy Markdown
Contributor Author

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 (<host>-sunshine.local, sanitized label + .local) with only the addresses Sunshine serves, via avahi_entry_group_add_address (newly loaded from libavahi-client alongside the existing function pointers), and uses it as the SRV target. Discovery transport is back to PROTO_UNSPEC on both the service and the address records, matching avahi-publish -a behavior. The served addresses are the explicit bind address when one is configured (non-wildcard), otherwise the local interface addresses selected for the effective family: family-filtered, loopback/unspecified dropped, deduplicated, IPv4-mapped IPv6 normalized. Host-name collisions regenerate the service host name (suffixed -N) alongside the service-name rename, so a retry does not collide on the same host name again. If no served address can be determined, it falls back to the daemon host name with an explicit warning log rather than publishing an unresolvable SRV target. The old protocol_for_address_family helper is removed as superseded by this redesign. One judgment call: in both mode, link-local IPv6 interface addresses are published (same as the daemon's previous behavior); only the family filter is new there.

P2 — Doxygen duplicate. Resolved by removal: the helper whose definition duplicated the declaration's @param docs no longer exists. The new helpers are documented once, on their declarations in publish.h; the publish.cpp definitions use ordinary comments.

P3 — clang-format. Applied the suggested include-block indentation in tests/unit/test_publish.cpp; verified with clang-format 23.1.0 --dry-run --Werror on all three changed files.

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), both keeps both families, loopback/unspecified/duplicates are dropped and v4-mapped addresses normalize, host-name sanitization/truncation/collision-suffix behavior, and the Avahi address-record byte layout (including network-order IPv4 bytes). I did not add a live-daemon mDNS resolution test: the unit-test harness has no Avahi daemon, and a test shelling out to one would be flaky in CI. The selection-layer tests cover exactly what determines which records get published.

Verification done / limits. avahi_entry_group_add_address signature and the AvahiAddress layout were verified against Avahi upstream source; the symbol is confirmed present in libavahi-client.so.3 (checked locally: avahi_entry_group_add_address exported). The real publish.cpp type-checks clean under g++ -std=c++23 -Wall -Wextra -Werror (against stubbed Sunshine headers, since Boost is not installed here), and the header helpers were executed in a standalone harness replicating every new unit assertion — all passed. Not verified: no full Sunshine build/test-suite run, no live Avahi daemon reproduction, no CI/SonarCloud re-check — CI remains the real gate for those.

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.

@MrBeldum

Copy link
Copy Markdown
Contributor Author

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

  • S3608 (publish.cpp) — fail_guard lambda now captures [&interfaces] explicitly; it only uses interfaces.
  • S134 (publish.cpp, served_addresses) — flattened with early returns: empty/unparsable/unspecified bind → interface enumeration; IPv4-only mismatch → {}; otherwise {bind_address}. Same five paths as before.
  • S134 (publish.cpp, create_services) — address-adding loop extracted into add_service_addresses(); on failure the caller still resets the group and returns, as before.
  • S6177 (publish.cpp) — using enum avahi::proto; in create_services and the new helper; both PROTO_UNSPEC uses are now unqualified. No qualified avahi::proto:: use remains in publish.cpp.
  • S6185 (publish.h) — collision suffix now via std::format("-{}", collision_suffix) (+ #include <format>).
  • S6004 (publish.h) — MAX_LABEL moved into the if init-statement.
  • S1238 (publish.h) — select_served_addresses takes candidates by const ref; the body never mutates the vector, and existing call sites (including temporaries) still bind.
  • S6197 (publish.h) — std::copy(...) → std::ranges::copy(bytes, result.data.begin()).

Left as-is

  • S5421 (publish.cpp:282) — entry_group_add_address is a namespace-scope function-pointer global assigned at runtime via the dyn::load dlsym table (same pattern as all sibling pointers); const cannot hold.
  • S5421 (publish.cpp:414) — host_name_collision_count is incremented at runtime on collisions (two ++ sites); const cannot hold.
  • S3630 (publish.cpp:434, 439) — the reinterpret_casts are the standard POSIX sockaddr downcast after the sa_family == AF_INET / AF_INET6 check. static_cast cannot convert between the unrelated sockaddr / sockaddr_in{,6} types, so there is no safer operation.
  • S7035 (publish.cpp, x3, no line) — unactionable as far as I can tell: the file contains no enum-to-underlying cast. The only enum-related casts are PublishFlags(0) (x2, int→enum, the opposite direction) and the pre-existing ClientFlags(0); the reported text ranges point at lines with no cast (a doc comment, a using-declaration, a variable declaration). If Sonar re-flags these after this push, the specific locations will need a closer look.
  • S3642 (publish.h, no line) — the only enum declared in publish.h is already enum class proto : int; there is nothing to convert. The pre-existing plain enums in publish.cpp (err_e, ServerState, etc.) are untouched and were not flagged on this PR.

Verification done

  • Re-read the diff: each fixed item maps to a real change; left items are untouched.
  • New constructs (using-enum, if-init constexpr, std::format, ranges::copy, const-ref param, explicit capture, early returns, helper extraction) syntax-checked standalone with g++ 13 -std=c++23 -Wall -Wextra -Werror (clean).
  • Commit identity verified: author and committer are the noreply identity; single append-only commit on top of 55061f4, no force-push, no rebase.

Verification limits

  • No full Sunshine build here (project dependencies unavailable in this environment), no live Avahi daemon, and lb-update-clang-format could not run (uv unavailable). No new unit tests: the restructured .cpp paths aren't directly covered by the existing unit-test harness, and the header helpers' existing tests are unaffected in behavior.
  • Sonar re-analysis after this push is the real gate — I am not claiming a green Quality Gate.

This work was done by an AI agent on Daniel's behalf; Daniel did not personally write it.

@MrBeldum

Copy link
Copy Markdown
Contributor Author

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:

  • The "left as-is" entries for S7035 and S3642 in my previous comment are withdrawn — there is nothing open to leave. If Sonar re-opens them on the next analysis, they'll need fresh locations.
  • The S6177 change in 1d6457f still stands: 55061f4 removed the old using enum avahi::proto; and reintroduced two qualified avahi::proto::PROTO_UNSPEC uses, so the using-enum cleanup addresses the live code even though the old S6177 issue itself is closed.
  • Corrected tally for the 11 open findings: 7 fixed in 1d6457f (S3608, S134 x2, S6185, S6004, S1238, S6197), 4 left with reasons (S5421 x2, S3630 x2). Everything else in the previous comment stands.
  • Sonar has not yet re-analyzed 1d6457f (the open issues still carry the 55061f4 analysis timestamp), so the re-analysis remains the real gate.

This work was done by an AI agent on Daniel's behalf; Daniel did not personally write it.

@MrBeldum

Copy link
Copy Markdown
Contributor Author

SonarCloud re-analysis on 1d6457f left 5 new issues (down from 11). Fixed the one my previous commit introduced, in append-only commit 73c5cf8:

  • S1117 (publish.cpp:487) — fixed: the add_service_addresses parameter group shadowed the file-scope avahi::EntryGroup *group. Renamed the parameter to entry_group (definition, Doxygen @param, and its single use). The call site still passes the global unchanged. Pure rename, no behavior change, no new test needed.

The other 4 are pre-existing patterns this PRs diff surfaced, left intentionally:

  • S5421 x2 (publish.cpp:282, 414): the function-pointer table entries are test-injection hooks (making them const would break injection), and host_name_collision_count is a counter reassigned on collisions — neither can be const.
  • S3630 x2 (publish.cpp:434, 439): reinterpret_cast of sockaddr to sockaddr_in/sockaddr_in6 is the only valid cast for this in C++; there is no safer operation.

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.

This comment was generated by Muse (Meta AI) with AI assistance, reviewed by the same agent (self-review, no independent reviewer).

@ReenigneArcher ReenigneArcher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread src/platform/linux/publish.cpp Outdated
Comment thread src/platform/linux/publish.cpp Outdated
Comment thread src/platform/linux/publish.cpp
@MrBeldum
MrBeldum force-pushed the fix/mdns-ipv6-filter-5860 branch from 73c5cf8 to ff98a49 Compare October 14, 2026 16:36
@MrBeldum

Copy link
Copy Markdown
Contributor Author

All three findings addressed in 27f1d01 (rebased onto master 81a677c, conflicts resolved — the only conflict was the init_client loader table: master's reformat + this PR's entry_group_add_address line). Self-review pass done on the final diff (no independent reviewer available in this environment).

[P1] Reverse-PTR collision — fixed. add_service_addresses() now passes avahi::PUBLISH_NO_REVERSE, so the alias publishes forward A/AAAA records only and cannot collide with the daemon-owned reverse PTR under default publish-addresses=yes. Regression: AddressPublicationSkipsReversePtr — the address-call stub returns ERR_COLLISION unless the flag is present, and the test asserts the call succeeds, registration + commit are reached, and the poll loop is not quit.

[P1] Interface scope lost — fixed. New platf::publish::ServedAddress {interface, address} is carried through enumeration → selection → publication. Enumeration populates the interface with if_nametoindex(ifa->ifa_name) (falling back to IF_UNSPEC if the name vanishes mid-enumeration); an explicit bind address resolves its owning interface the same way (IF_UNSPEC when no local interface owns it); selection deduplicates by interface + address; add_service_addresses() passes address.interface. Discovery protocol stays PROTO_UNSPEC. Regression: AddressPublicationUsesOwningInterface — LAN + Docker-bridge addresses plus distinct link-local IPv6 addresses on two synthetic interfaces, asserting each captured call uses its owning index and encodes correctly; plus bind-resolution tests (live-interface owner, and IF_UNSPEC fallback for an unowned TEST-NET-1 address).

[P2] Stale records after network change — fixed. 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; an unchanged snapshot is a no-op. I checked for an existing notification mechanism to reuse: there is none — the only related code is UPnP's WAN-IP renewal, which is unrelated to mDNS — so I implemented the smallest correct one: a netlink route socket (RTMGRP_LINK | RTMGRP_IPV4_IFADDR | RTMGRP_IPV6_IFADDR) registered as an Avahi poll watch, so the callback (drain + refresh) runs on the existing poll thread. No new thread, no cross-thread Avahi calls, no polling. Install is best-effort (failure degrades to the old behavior instead of failing startup); shutdown frees the watch + socket after the poll thread joins. One deliberate deviation from the sketch: create_services() gained a quit_on_failure parameter (default true, preserving the existing fail-fast behavior for initial registration), and refresh passes false, threaded through the collision retry — otherwise a transient failure during a refresh would quit the poll loop and leave the service withdrawn with no recovery short of restarting Sunshine. Regression: RefreshRepublishesOnAddressChange (register .10, change snapshot to .20, refresh, verify withdrawal/republication with no stale .10, plus unchanged-snapshot no-op) and RefreshCollisionRetryDoesNotQuitPollLoop.

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:

  • S5421 on the Avahi function-pointer globals (e.g. entry_group_add_address): they must stay mutable — the dynamic loader assigns them at startup and the registration tests stub them. Making them const would break both.
  • S5421 on the group global: mutated by the Avahi entry-group callback; cannot be const.
  • S3630 ×2 on the sockaddr downcasts in enumeration: sa_family is checked immediately before each cast and no safer downcast exists for sockaddr.
  • The three new globals in this round (published_addresses, netlink_fd, netlink_watch) fall under the same S5421 disposition — mutable state required by the callback/watch architecture — and will predictably appear as new S5421 lines on the fresh analysis. I did not mark anything won't-fix in SonarCloud (no permission to do so); this reply is the recorded disposition.

Verification (honest limits). A standalone harness driving the real publish.cpp logic with stubbed Avahi calls: 44 checks, 0 failures; fail-before confirmed on the unfixed logic (11 failures across the three findings). clang-format --dry-run --Werror clean (23.1.3). The full CMake/Ninja build, the gtest suite, and a live-daemon test were NOT run in this environment (Sunshine dependencies unavailable), so please treat CI as the verification gate for those. The new tests/unit/test_publish.cpp regressions mirror the harness.

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.

MrBeldum added a commit to MrBeldum/Sunshine that referenced this pull request Oct 14, 2026
…, 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.
@MrBeldum

Copy link
Copy Markdown
Contributor Author

SonarCloud follow-up in 5020c08: fixed the 3 findings that are genuinely correct without changing behavior; the other 29 are left as-is with per-finding reasons below. No behavior change intended. (Disclosure: this work was drafted with AI assistance and self-reviewed; no independent reviewer was available in this environment.)

Line numbers below are Sonar's on head 27f1d01.

Fixed in this commit (3)

  • S3642 (publish.cpp:206) — enum WatchEvent → enum class WatchEvent : int. The explicit int underlying type keeps the documented AvahiWatchEvent layout; the single use site is now WatchEvent::WATCH_IN. Verified: no other use of the enumerators, no implicit int conversions.
  • S6003 (publish.cpp:371, 376) — push_back({...}) → emplace_back(...) in enumerate_interface_addresses. ServedAddress is an aggregate; C++23 parenthesized aggregate init in emplace_back constructs the same values in place.

Left as-is (29)

S5421 x25 — none of these can be const:

  • publish.h:172–185 (x14, loader-table extern declarations) — 13 are moved verbatim from upstream publish.cpp definitions; entry_group_add_address (publish.h:178) is new in this PR because upstream never used avahi_entry_group_add_address. All 14 are assigned at runtime by dyn::load in init_client/init_common (e.g. &entry_group_add_address → "avahi_entry_group_add_address"), so const would break the loader.
  • publish.cpp:192 (x1, entry_group_add_address definition) — same runtime-loaded pointer; also the only definition Sonar flags because it is the only one on a changed hunk — its 18 neighboring loader-table definitions are identical in form but sit on unchanged lines.
  • publish.h:390–395 (x6, #ifdef SUNSHINE_TESTS extern declarations) — mirror state that is mutated: group/poll/client reassigned on publish/teardown, name updated on collision, host_name_collision_count incremented (publish.cpp:470, 554), published_addresses reassigned (publish.cpp:575).
  • publish.cpp:335, 337, 339, 340 (x4, new state) — host_name_collision_count incremented, published_addresses assigned, netlink_fd/netlink_watch assigned on install and reset to -1/nullptr on teardown (publish.cpp:658–659, 722, 726). File-scope mutable state matches the surrounding upstream design (group/poll/client are file-scope in upstream too); moving it into a struct would be a refactor beyond this PR.

S3642 x1 — publish.h:60 enum PublishFlags: moved byte-identically from upstream publish.cpp (verified by diff). Converting to enum class would require qualifying every enumerator use and adding casts where values flow to Avahi as ints — a refactor of upstream code to chase Sonar, so leaving it.

S3630 x2 — publish.cpp:368, 373 reinterpret_cast<const sockaddr_in*> / sockaddr_in6*: each follows an sa_family == AF_INET / AF_INET6 check. static_cast cannot downcast between unrelated pointer types, a C-style cast is strictly worse, and bit_cast does not apply to pointers. This is the standard, guarded pattern; no safer C++ cast compiles here.

S5008 x1 — publish.cpp:605 void * in address_change_callback: required by the Avahi watch-callback signature (void (*)(AvahiWatch*, int, AvahiWatchEvent, void*)). Same as the two existing // NOSONAR(cpp:S5008) comments on entry_group_callback / client_callback upstream.

Verification (honest limits)

  • clang-format 23.1.3: publish.cpp clean (file was also clean at 27f1d01).
  • Compile check: a standalone harness using the verbatim WatchEvent enum and ServedAddress struct from this PR verified the enum-class change (int size, poll values, qualified use through the declared function-pointer type) and the emplace_back calls (same values, operator== intact), with -Wall -Wextra -Werror under C++23. Boost headers are not available in this environment, so the real boost::asio::ip::address converting constructors were stubbed; the conversion sequence in emplace_back is identical to the previous push_back braced-init.
  • Not run here: full build, gtest, live daemon. CI is the gate.
  • Sonar re-analysis on 5020c08 is pending; I expect the QG to still fail on the 29 documented left-as-is items unless maintainers prefer suppressions or a different treatment.

@MrBeldum
MrBeldum force-pushed the fix/mdns-ipv6-filter-5860 branch from 5020c08 to de1db21 Compare October 15, 2026 16:13
MrBeldum added a commit to MrBeldum/Sunshine that referenced this pull request Oct 15, 2026
…, 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 ReenigneArcher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread src/platform/linux/publish.cpp Outdated
Comment thread src/platform/linux/publish.cpp Outdated
Comment thread src/platform/linux/publish.cpp Outdated
MrBeldum added a commit to MrBeldum/Sunshine that referenced this pull request Oct 15, 2026
… 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.
@MrBeldum

Copy link
Copy Markdown
Contributor Author

Thanks for the re-review. This push (7cb9f82) addresses all three P2 findings:

1. IPv6 zone preserved on enumeration — enumerate_interface_addresses() now constructs the v6 address with sin6_scope_id, so a scoped bind like fe80::1%7 equals its enumerated owner and resolves to the owning interface instead of IF_UNSPEC. Added ServedAddressesTest.ExplicitBindScopedLinkLocalResolvesOwningInterface, which derives its input from raw getifaddrs() (not the enumeration) so it detects the mismatch.

2. Snapshot invalidated on withdrawal — published_addresses is now std::optional<std::vector<ServedAddress>> (declaration in publish.h, definition in publish.cpp). refresh_service_addresses() uses the prescribed published_addresses && current == *published_addresses comparison, reset() is called before every entry_group_reset(group) (5 sites, including the client-callback collision/registering path) and at the start of a fresh entry_group_is_empty registration attempt, and assignment after successful commit is unchanged. Fixture .clear() calls updated to .reset(). Added RefreshFailureThenRestoreRepublishes (nonempty A) and RefreshFailureThenRestoreEmptySnapshotRepublishes (empty A, daemon-hostname fallback): both assert a second commit, a nonempty group, and no poll-loop quit.

3. Poll callback documented — address_change_callback() now carries exactly the suggested Doxygen block.

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 getifaddrs() data, stubbed Avahi, stub address type with Boost-identical equality/constructor semantics): the three regression scenarios fail on pre-fix logic and pass post-fix, and the pre-existing refresh/collision/no-op behaviors pass in both. I also verified against real Boost headers that make_address("fe80::1%7") retains scope 7, address_v6(bytes, scope) stores it, and equality distinguishes scope. What I did NOT run: the full Sunshine build, the gtest suite, a live daemon, or a full ReadTheDocs build (no Doxygen available here) — CI is the gate for those.

@MrBeldum

Copy link
Copy Markdown
Contributor Author

SonarCloud re-analysis on 7cb9f82 (this push) reports 29 new issues — the same 29 already dispositioned in my Sonar follow-up comment (#issuecomment-6178743613): S5421 x25, S3630 x2, S5008 x1, S3642 x1 (PublishFlags).

I checked the T42 diff against the flagged lines: no new finding is attributable to this push — the flagged declarations (loader table, published_addresses, callback signatures) are the same ones from before, and the changed lines introduce no new rule violation. The Quality Gate failure on these 29 is therefore expected, same as on the previous head. No code change for Sonar in this push.

AI-assisted (Muse), self-reviewed; full build/gtest/live-daemon not run locally — CI is the gate.

ReenigneArcher
ReenigneArcher previously approved these changes Oct 15, 2026
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).
…al group

SonarCloud S1117 on head 1d6457f: the parameter 'group' shadowed the
file-scope 'avahi::EntryGroup *group'. Rename the parameter to
'entry_group' (definition, Doxygen @PARAM, and single use). Call site
passes the global unchanged. Pure rename, no behavior change.
…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.
@ReenigneArcher
ReenigneArcher force-pushed the fix/mdns-ipv6-filter-5860 branch from 7cb9f82 to 587397c Compare October 15, 2026 23:48
@ReenigneArcher ReenigneArcher changed the title fix(mdns): only advertise addresses matching configured address family fix(mdns): advertise only served addresses on Linux Oct 15, 2026
@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Bundle Report

Bundle size has no change ✅

@codecov

codecov Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 61.92893% with 75 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.90%. Comparing base (55ff34e) to head (1c800cc).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/platform/linux/publish.cpp 52.70% 50 Missing and 20 partials ⚠️
src/platform/linux/publish.h 89.79% 1 Missing and 4 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            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     
Flag Coverage Δ
Alpine-aarch64 30.19% <54.64%> (+0.35%) ⬆️
Alpine-x86_64 30.31% <54.64%> (+0.35%) ⬆️
Archlinux 28.01% <56.28%> (+0.40%) ⬆️
FreeBSD-amd64 29.87% <34.12%> (+0.07%) ⬆️
Homebrew-macos-15 46.67% <ø> (ø)
Homebrew-macos-26 47.06% <ø> (+0.03%) ⬆️
Homebrew-ubuntu-24.04 31.25% <45.25%> (+0.23%) ⬆️
Linux-AppImage-aarch64 33.10% <59.01%> (+0.35%) ⬆️
Linux-AppImage-x86_64 33.35% <59.01%> (+0.36%) ⬆️
PowerShell 100.00% <ø> (ø)
Web-UI 56.65% <ø> (ø)
macOS-arm64 38.48% <ø> (ø)
macOS-x86_64 39.06% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/platform/linux/publish.h 89.79% <89.79%> (ø)
src/platform/linux/publish.cpp 31.35% <52.70%> (+31.35%) ⬆️

... and 12 files with indirect coverage changes


Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55ff34e...1c800cc. Read the comment docs.

@github-actions

github-actions Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Last Updated 2026-10-16 03:31:58 UTC
Source Run CI Run #6329
Commit 1c800cca162fee232824df2937f289498d30d9f4

Screenshot Comparison

PR #5914 screenshots vs screenshots baseline.

Matrix: AppImage-aarch64

Image Baseline PR
sunshine_tray_initial.png
sunshine_tray_menu.png
sunshine_tray_pairing_request.png
sunshine_tray_paused.png
sunshine_tray_stopped.png
sunshine_tray_streaming.png
sunshine_tray_tooltip.png

Matrix: AppImage-x86_64

Image Baseline PR
sunshine_tray_initial.png
sunshine_tray_menu.png
sunshine_tray_pairing_request.png
sunshine_tray_paused.png
sunshine_tray_stopped.png
sunshine_tray_streaming.png
sunshine_tray_tooltip.png

Matrix: macOS-arm64

Image Baseline PR
sunshine_tray_initial.png
sunshine_tray_menu.png
sunshine_tray_pairing_request.png
sunshine_tray_paused.png
sunshine_tray_stopped.png
sunshine_tray_streaming.png
sunshine_tray_tooltip.png

Matrix: macOS-x86_64

Image Baseline PR
sunshine_tray_initial.png
sunshine_tray_menu.png
sunshine_tray_pairing_request.png
sunshine_tray_paused.png
sunshine_tray_stopped.png
sunshine_tray_streaming.png
sunshine_tray_tooltip.png

@ReenigneArcher

Copy link
Copy Markdown
Member

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.
@MrBeldum

Copy link
Copy Markdown
Contributor Author

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: src/platform/linux/publish.cpp:11:10: fatal error: 'linux/netlink.h' file not found. Sorry for the mischaracterization.

Root cause: the address-change watch added in this PR uses netlink, which is a Linux-kernel API, but src/platform/linux/*.cpp is also compiled on FreeBSD (with SUNSHINE_PLATFORM=freebsd), where the <linux/...> headers don't exist.

Fix (pushed as 44fb711): guard the Linux-only netlink code in src/platform/linux/publish.cpp with #ifdef __linux__ — the includes, the netlink_fd / netlink_watch members, address_change_callback, the install body, and the deinit cleanup. I used __linux__ rather than #ifndef __FreeBSD__ (the form in misc.cpp) because netlink is Linux-specific, not "everything except FreeBSD". On non-Linux platforms install_address_change_watch() now returns false (best-effort, no watch): discovery still works, records just don't auto-refresh on network changes — exactly the pre-watch behavior. The call site is unchanged and still treats it as best-effort. Linux behavior is unchanged. The publish.h Doxygen for install_address_change_watch() now notes it is Linux-only. No test changes: tests/unit/test_publish.cpp has no netlink references.

Verification (honest limits): I have no FreeBSD machine and did not run a full Sunshine build locally. What I did verify: preprocessing the real publish.cpp with __linux__ undefined and poisoned netlink headers succeeds, with zero netlink symbols in the output and the install body reducing to return false; controls confirm the Linux path still includes netlink and keeps the full watch logic. clang-format checked against the repo's .clang-format. FreeBSD CI is the gate for the real build.

A possible follow-up (not implemented here): a FreeBSD AF_ROUTE routing-socket watcher could restore auto-refresh there. Happy to do that as a separate change if you'd like it.

This work is AI-assisted (Muse Spark 1.3, implementation + self-review pass; no independent reviewer was available). The author did not personally write it.

@MrBeldum

Copy link
Copy Markdown
Contributor Author

SonarCloud re-analysis on head 44fb711 is done: Quality Gate still fails, with 26 new issues (down from 29 on 7cb9f82) — S5421 x23, S3630 x2, S3642 x1.

I checked each flagged line against the T44 diff: none is a line T44 added or changed (the T44 lines are the #ifdef __linux__ guards, the guarded netlink members/callback/install/deinit code, and the Doxygen note). The 26 are the previously dispositioned set: moved upstream loader decls, test-seam externs, the family-checked sockaddr downcasts, and the byte-identical moved PublishFlags enum. The S5008 void* finding from the previous head is no longer reported.

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 ReenigneArcher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread src/platform/linux/publish.cpp
…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.
@MrBeldum

Copy link
Copy Markdown
Contributor Author

Addressed the re-review finding (review 5542042465, inline comment 4290068153).

Finding: [P2] Preserve FreeBSD address updates when disabling the watch

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

  • src/platform/linux/publish.cpp (create_services): served_addresses() is still computed on all platforms (it compiles on non-Linux: getifaddrs / if_nametoindex / Boost.Asio are portable). The service-specific host name and the add_service_addresses() call are now inside #ifdef __linux__; on non-Linux service_target stays nullptr, so the service uses the daemon-managed host name and no fixed alias address records are published. The committed-snapshot line (published_addresses = addresses) is deliberately unchanged: the refresh only ever fires from the watch callback, and if it were ever called on non-Linux it would still compare + republish with a null host, which is correct.
  • Fallback comment/documentation updated to describe delegated address maintenance: the non-Linux branch of install_address_change_watch(), the start() best-effort comment (which previously claimed records go stale on all platforms without a watch — now only Linux), and the install_address_change_watch() Doxygen in publish.h.
  • tests/unit/test_publish.cpp: no longer entirely Linux-gated. The file's outer guard is now !WIN32 && !APPLE, matching the platform set publish.cpp is built for (cmake/compile_definitions/linux.cmake is included for all non-Apple Unix); the existing plain unit tests stay Linux-gated. New PublishRegistrationTest.NonLinuxFallbackUsesDaemonHostName (non-Linux only) 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), keeping the non-Linux build warning-free.

Verification

Disclosure: 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 publish.cpp + test_publish.cpp against stubbed dependencies (no Boost / gtest / clang-format binary available locally):

  • Linux config: 19/19 pass (all pre-existing tests + controls).
  • Non-Linux config (__linux__ undefined, poisoned netlink headers): new test passes; zero netlink symbols in the object; -Wall -Wextra clean.
  • Fail-before: the new test fails on the pre-fix code at the fixed-alias-addresses assertion.
  • Not verified: full Sunshine build, real gtest run, native FreeBSD build, live daemon. CI is the gate.

Head: ecce8ab065a1e2e43792325b05c4190b200de495 (the commit message carries the same AI disclosure + limits).

@ReenigneArcher ReenigneArcher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Comment thread tests/unit/test_publish.cpp Outdated
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.
@MrBeldum

Copy link
Copy Markdown
Contributor Author

Addressed the re-review finding (review 5542281177, inline comment 4290244919, [P3]) in commit 1c800cc.

Finding: format both newly nested Linux guards

Applied the reviewer's exact correction: the #if defined(__linux__) guard and its matching #endif around FakeEntryGroupAddAddressLenient in tests/unit/test_publish.cpp now each carry two leading spaces. No other line changed.

Verification

  • git diff on the commit is exactly 2 insertions / 2 deletions (the two directives); git diff -w is empty, confirming a whitespace-only change with no functional effect.
  • Preprocessor nesting still balanced (5 #if / 5 #endif), and all depth-2 directives in the file now uniformly carry two leading spaces.
  • Honest limits: no clang-format binary was available in this environment, so I applied the reviewer's stated two-whitespace-change correction directly rather than running the formatter; no full build, gtest run, or live-daemon test was run on this whitespace-only commit. CI remains the gate.

AI-assisted with disclosure. Self-review only (no independent reviewer available).

@MrBeldum

Copy link
Copy Markdown
Contributor Author

SonarCloud re-analysis on 1c800cc reports 26 open issues — identical composition to the triage on 44fb711 (T45): S5421 x23, S3630 x2, S3642 x1.

Verified 0 attributable to T46 (ecce8ab) or T47 (1c800cc): checked every flagged line against the 44fb711...ecce8ab compare diff — no flagged line falls inside any T46 added-line range (publish.cpp hunks at 522–542, 550–557, 692–702, 803–812; publish.h hunk at 381–389; test file has zero flagged lines). T47 was whitespace-only (two-space indent on two preprocessor directives). Flagged lines are the same pre-existing/moved categories dispositioned in T39/T45: moved upstream loader declarations, test-seam externs, the family-checked sockaddr downcast (S3630), and the moved PublishFlags enum (S3642, byte-identical moved upstream code). No code change made; QG failure is expected on the same left-as-is set.

AI-assisted triage; no independent reviewer available in this environment (self-review only). Full build/gtest/live-daemon not run — CI is the gate.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
26 New issues
26 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@ReenigneArcher
ReenigneArcher merged commit 3d890ea into LizardByte:master Oct 16, 2026
79 of 84 checks passed
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.

mDNS advertises an IPv6 address Sunshine is not listening on (address_family = ipv4) — Moonlight stalls on "Connecting"

2 participants