resolved: fix spurious BrowseServices add/remove flapping with ifindex=0 (#42982)

## Problem A `BrowseServices` subscription with `"ifindex":0` (browse all interfaces) receives a continuous flap of `added`/`removed` events for a service that is still present — within a second, and with no goodbye packet involved. ## Root cause For `ifindex==0`, `mdns_browser_revisit_cache()` looked up each mDNS scope's cache separately and called `mdns_manage_services_answer()` **once per scope**. That function derives `removed` events by diffing the browser's *global* discovered-service list (all interfaces, filtered only by owner family) against the single answer it's handed. So with ≥2 mDNS-relevant interfaces of the same family, a service present on interface A isn't in interface B's answer and is spuriously removed while B is reconciled, then re-added on the next revisit tick. ## Fix Accumulate the pruned cache answers from every matching mDNS scope into one combined `DnsAnswer` and reconcile **once**, so removals diff the global list against the union across interfaces. Items are merged with `dns_answer_add_full()` (not `dns_answer_extend()`, which defaults each item's `until` to `USEC_INFINITY` and would skew the RFC 6762 §5.2 TTL-maintenance schedule). The single-interface (`ifindex>0`) path is unchanged. ## Test `TEST-89-RESOLVED-MDNS.sh` gains `testcase_browse_ifindex_zero_no_flap`: it adds a service-less dummy mDNS link to guarantee ≥2 same-family scopes (the flap precondition), browses `ifindex=0`, waits for discovery, then asserts **zero** `removed` events while every publisher stays up. The subscription uses `varlinkctl --timeout=infinity`, since it sits idle after discovery and the default 45s idle timeout would sever it (and the assertion) mid-observation. ## Testing status Builds clean; `shellcheck -x` clean. First CI round: `TEST-89` (including this testcase) passed on all mkosi platforms; the failing jobs all traced to unrelated flakes/infra.
2026-07-14 23:58:34 +02:00
parent d26de063cd 4235dea97a
commit e6acf0bab7
2 changed files with 101 additions and 5 deletions
@@ -489,10 +489,21 @@ int mdns_browser_revisit_cache(DnsServiceBrowser *sb, int owner_family) {
        assert(sb);
        assert(sb->manager);

-        /* ifindex=0 means "all interfaces" */
+        /* ifindex=0 means "all interfaces". Collect the cached answers from
+         * every matching mDNS scope into a single combined answer and reconcile
+         * once. Reconciling per-scope would be wrong: mdns_manage_services_answer()
+         * derives removals by diffing the browser's global service list against
+         * the answer it is handed, so a single scope's answer would spuriously
+         * "remove" (and then, on the next scope/pass, re-"add") services that are
+         * still present on other interfaces — resulting in a continuous
+         * added/removed event flap for services seen on more than the current
+         * scope. */
        if (sb->ifindex == 0) {
+                _cleanup_(dns_answer_unrefp) DnsAnswer *combined = NULL;
+
                LIST_FOREACH(scopes, scope, sb->manager->dns_scopes) {
                        _cleanup_(dns_answer_unrefp) DnsAnswer *answer = NULL;
+                        DnsAnswerItem *item;

                        if (scope->protocol != DNS_PROTOCOL_MDNS)
                                continue;
@@ -515,11 +526,24 @@ int mdns_browser_revisit_cache(DnsServiceBrowser *sb, int owner_family) {
                                return log_error_errno(r, "Failed to look up DNS cache for service browser key on scope %s: %m",
                                                       dns_scope_ifname(scope) ?: "global");

-                        r = mdns_manage_services_answer(sb, answer, owner_family);
-                        if (r < 0)
-                                return log_error_errno(r, "Failed to manage mDNS services after cache lookup on scope %s: %m",
-                                                       dns_scope_ifname(scope) ?: "global");
+                        /* Merge preserving each item's ifindex, flags, rrsig and
+                         * cache-expiry 'until'. (dns_answer_extend()/merge() would
+                         * reset 'until' to USEC_INFINITY, which would skew the RFC
+                         * 6762 §5.2 TTL-maintenance schedule that
+                         * mdns_manage_services_answer() derives from item->until.) */
+                        DNS_ANSWER_FOREACH_ITEM(item, answer) {
+                                r = dns_answer_add_extend_full(&combined, item->rr, item->ifindex,
+                                                               item->flags, item->rrsig, item->until);
+                                if (r < 0)
+                                        return log_error_errno(r, "Failed to merge mDNS cache answer from scope %s: %m",
+                                                               dns_scope_ifname(scope) ?: "global");
+                        }
                }
+
+                r = mdns_manage_services_answer(sb, combined, owner_family);
+                if (r < 0)
+                        return log_error_errno(r, "Failed to manage mDNS services after cache lookup for all interfaces: %m");
+
                return 0;
        }

@@ -237,6 +237,78 @@ testcase_browse_all_interfaces_ifindex_zero() {
    run_and_check_services_with_ifindex 0 check_both 0
 }

+testcase_browse_ifindex_zero_no_flap() {
+    : "ifindex=0 browse must not emit spurious 'removed' events while publishers stay up"
+    resolvectl flush-caches
+
+    local out_file unit_name service_type added removed
+    local dummy="ravc-noflap"
+
+    out_file="$(mktemp)"
+    unit_name="varlinkctl-noflap-$SRANDOM.service"
+    service_type="_testService5._udp"
+
+    # The flap only manifests when the browser reconciles >=2 same-family mDNS
+    # scopes: the pre-fix code diffed the browser's global service list against
+    # each scope's partial answer, spuriously removing services absent from that
+    # one scope. The host normally has only the container bridge as an mDNS
+    # interface, so add a service-less dummy link with mDNS enabled to guarantee a
+    # second (empty) scope that the ifindex=0 reconciliation must combine. This
+    # must succeed -- without the second scope the testcase asserts nothing.
+    # A previously interrupted run may have leaked the fixed-name link: RETURN traps
+    # do not fire when set -e aborts a function mid-flight, so clear it first to keep
+    # re-runs self-healing instead of tripping over EEXIST here.
+    ip link del "$dummy" 2>/dev/null || :
+    ip link add "$dummy" type dummy
+    # Arm the cleanup before anything else can fail, so the fixed-name link never
+    # leaks into later testcases. The browse unit may not exist yet, hence the
+    # best-effort stop.
+    # shellcheck disable=SC2064
+    trap "trap - RETURN; systemctl stop $unit_name 2>/dev/null || :; ip link del $dummy 2>/dev/null || :" RETURN
+    ip link set "$dummy" up multicast on
+    ip address add 169.254.171.171/16 dev "$dummy"
+    resolvectl mdns "$dummy" yes
+    [[ "$(resolvectl mdns "$dummy")" =~ :\ yes$ ]]
+    sleep 2  # let resolved create the scope before we start browsing
+
+    # Long-running browse across *all* interfaces (ifindex=0). With the
+    # combined-answer reconciliation there must be no 'removed' event as long as
+    # every publisher stays up. Use --timeout=infinity: the subscription goes idle
+    # once everything is discovered, and varlinkctl's default 45s idle timeout
+    # would sever it (and the assertion) mid-observation.
+    systemd-run --unit="$unit_name" --service-type=exec -p StandardOutput="file:$out_file" \
+        varlinkctl call --more --timeout=infinity /run/systemd/resolve/io.systemd.Resolve io.systemd.Resolve.BrowseServices \
+        "{ \"domain\": \"$service_type.local\", \"type\": \"\", \"ifindex\": 0, \"flags\": 16785432 }"
+
+    # Wait until both containers' services (20 each, 40 total) have been
+    # discovered. Count occurrences, not lines: varlinkctl --more emits compact
+    # JSON-SEQ and one notify batches many entries onto a single line.
+    for _ in {0..14}; do
+        added="$( { grep -o '"updateFlag":"added"' "$out_file" || :; } | wc -l)"
+        [[ "$added" -ge 40 ]] && break
+        sleep 2
+    done
+    if [[ "${added:-0}" -lt 40 ]]; then
+        echo >&2 "Did not discover the expected services on ifindex=0"
+        cat "$out_file" >&2
+        return 1
+    fi
+
+    # Observe a further window during which several continuous-query revisits
+    # happen; a correct ifindex=0 browse emits zero 'removed' events while every
+    # publisher stays up.
+    sleep 12
+
+    removed="$( { grep -o '"updateFlag":"removed"' "$out_file" || :; } | wc -l)"
+    if [[ "${removed:-0}" -ne 0 ]]; then
+        echo >&2 "Got $removed spurious 'removed' event(s) on ifindex=0 while all publishers were up:"
+        grep -oE '"updateFlag":"removed"[^}]*"name":"[^"]*"' "$out_file" >&2 || :
+        return 1
+    fi
+
+    echo testcase_end
+}
+
 testcase_second_unreachable() {
    : "Test each service type while the second container is unreachable"
    systemd-run -M "$CONTAINER_2" --wait --pipe -- networkctl down host0