fix(avahi): avoid restart on transient iface loss#2660
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new helper function ChangesAvahi interface change optimization
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
elibosley
force-pushed
the
codex/avahi-update-preserve-running
branch
from
0da49e3 to
7b72a8b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
etc/rc.d/rc.avahidaemon (1)
134-144: ⚡ Quick winConsider adding an
iface_activecheck for defensive programming.Currently,
bind_adds_active_ifaceassumes that all interfaces in$NEXT(which is$BIND) are active, relying on the contract thatBINDis populated only with active interfaces byrc.library.source.However,
bind_removes_active_iface(lines 146-156) explicitly verifies that removed interfaces are active usingiface_active. For consistency and defensive programming, consider adding the same check here:♻️ Proposed change to verify added interfaces are active
bind_adds_active_iface(){ local CURRENT="$1" NEXT="$2" IFACE IFS=, read -ra IFACES <<< "$NEXT" for IFACE in "${IFACES[@]}"; do [[ -n $IFACE ]] || continue - if ! csv_has "$CURRENT" "$IFACE"; then + if ! csv_has "$CURRENT" "$IFACE" && iface_active "$IFACE"; then return 0 fi done return 1 }This makes the function more robust against potential races or bugs in the
BINDpopulation logic, and maintains symmetry withbind_removes_active_iface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@etc/rc.d/rc.avahidaemon` around lines 134 - 144, The function bind_adds_active_iface should mirror bind_removes_active_iface by checking that each candidate interface in NEXT is actually active before treating it as an added interface; update bind_adds_active_iface to call iface_active "$IFACE" (and skip non-active interfaces) inside the loop, and only return 0 when an iface is both not present in CURRENT (csv_has "$CURRENT" "$IFACE" is false) and iface_active reports true, otherwise continue and return 1 at the end.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@etc/rc.d/rc.avahidaemon`:
- Around line 134-144: The function bind_adds_active_iface should mirror
bind_removes_active_iface by checking that each candidate interface in NEXT is
actually active before treating it as an added interface; update
bind_adds_active_iface to call iface_active "$IFACE" (and skip non-active
interfaces) inside the loop, and only return 0 when an iface is both not present
in CURRENT (csv_has "$CURRENT" "$IFACE" is false) and iface_active reports true,
otherwise continue and return 1 at the end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eee1429a-4590-4b35-ad3a-da9244aacc72
📒 Files selected for processing (1)
etc/rc.d/rc.avahidaemon
elibosley
force-pushed
the
codex/avahi-update-preserve-running
branch
from
7b72a8b to
5e750d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@etc/rc.d/rc.avahidaemon`:
- Around line 122-131: bind_changed currently treats an empty show dev output as
"inactive" by using -z $(show dev "$IFACE"); update bind_changed to explicitly
interpret the semantics of show dev (as defined in rc.library.source) instead of
relying on emptiness — locate the second for-loop in bind_changed and replace
the -z $(show dev "$IFACE") test with a check that parses show dev "$IFACE"
output for the interface state (e.g., test for "up"/"active" or absence of "up")
so the condition truly reflects an inactive/transiently-down interface according
to show()'s output format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 68f8aeb7-56e3-45ec-851b-59a727812d83
📒 Files selected for processing (1)
etc/rc.d/rc.avahidaemon
| bind_changed(){ | ||
| local CURRENT=",$1," NEXT=",$2," IFACE | ||
| for IFACE in ${2//,/ }; do | ||
| [[ $CURRENT == *",$IFACE,"* ]] || return 0 | ||
| done | ||
| for IFACE in ${1//,/ }; do | ||
| [[ $NEXT == *",$IFACE,"* || -z $(show dev "$IFACE") ]] || return 0 | ||
| done | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the 'show' command exists and check its usage with 'dev'
# Check if 'show' is a function or executable
type show 2>/dev/null || echo "show command not found in PATH"
# Search for 'show' function definition in rc scripts
rg -n "^show\s*\(\)" --type=sh --glob='etc/rc.d/*'
# Search for 'show dev' usage patterns
rg -n -C3 "show dev" --type=sh --glob='etc/rc.d/*'Repository: unraid/webgui
Length of output: 3385
show dev is provided by rc.library.source; align semantics with inactivity checks
etc/rc.d/rc.library.source defines show() (around line 57) and relies on show dev output in -n/-z conditionals (e.g., around line 97 and line 120), so the earlier “command not defined” concern is just an artifact of not sourcing the library in isolation. The key remaining alignment is that show dev <iface> returning empty (which bind_changed tests via -z $(show dev "$IFACE")) truly means the interface is inactive/transiently down—not merely that it currently has no addresses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@etc/rc.d/rc.avahidaemon` around lines 122 - 131, bind_changed currently
treats an empty show dev output as "inactive" by using -z $(show dev "$IFACE");
update bind_changed to explicitly interpret the semantics of show dev (as
defined in rc.library.source) instead of relying on emptiness — locate the
second for-loop in bind_changed and replace the -z $(show dev "$IFACE") test
with a check that parses show dev "$IFACE" output for the interface state (e.g.,
test for "up"/"active" or absence of "up") so the condition truly reflects an
inactive/transiently-down interface according to show()'s output format.
Purpose of the change: - Keep Avahi running when an allowed interface temporarily loses carrier or its address. - Preserve explicit user configuration applies as forced Avahi binding updates. Before this change: - rc.avahidaemon update recomputed active bindings and restarted Avahi whenever the active bind list differed from allow-interfaces. - DHCP/network hooks could therefore restart Avahi while a wireless interface was still down. - A purely heuristic fix could not distinguish transient interface loss from a user applying Network Extra changes. Why that was a problem: - Avahi already observes interface loss and recovery while running. - Restarting during a temporary outage can start Avahi with no suitable network protocol, causing it to exit and remain stopped. - Explicit Network Extra changes still need to force the legacy restart path when the Avahi bind list changes. What the new change accomplishes: - Avoids restarting Avahi from ordinary network-hook updates when the only bind difference is an allowed interface that is currently inactive. - Passes a force mode from Network Extra applies so intentional include/exclude changes retain the old restart behavior. - Whitelists the queued service reload mode before passing it through the delayed at job. How it works: - Network Extra calls update_services with a force argument. - update_services carries the whitelisted mode into reload_services. - reload_services passes force only to rc.avahidaemon update. - rc.avahidaemon update restarts immediately for forced config applies, and otherwise uses bind_changed to ignore transient inactive-interface shrinkage.
elibosley
force-pushed
the
codex/avahi-update-preserve-running
branch
from
5e750d6 to
81847c1
Compare
🧹 PR Test Plugin Cleaned UpThe test plugin and associated files for this PR have been removed from the preview environment. 🤖 This comment is automatically generated when a PR is closed without merging. |
1 participant
Summary
Prevent network-hook service reloads from restarting Avahi when an allowed interface is only temporarily unavailable, while preserving forced restarts for explicit Network Extra configuration changes.
What Changed
rc.avahidaemonto distinguish meaningful runtime binding changes from transient interface loss.updateskips restart when the only runtime difference is a previously allowed interface that is currently inactive.update_services 1 forcethroughupdate.phpargs.update_serviceswhitelists and carries the optionalforcemode into the delayedreload_servicesjob.reload_servicespassesforceonly torc.avahidaemon update, leaving other service update calls unchanged.rc.avahidaemon update forcepreserves the old restart-on-bind-mismatch behavior for explicit user config applies.Why
Avahi already observes interface loss and recovery while it remains running. Restarting it during a temporary carrier/DHCP outage can start the daemon before any suitable interface is available, causing Avahi to exit and remain stopped.
The earlier heuristic alone was not sufficient because it could not distinguish transient interface loss from a user intentionally removing an interface in Network Extra while that interface was inactive. Passing
forcefrom the config-apply path makes that intent explicit.Relationship To Other PR
Alternative to #2661.
updatefinds it stopped.Verification
bash -n etc/rc.d/rc.avahidaemon emhttp/plugins/dynamix/scripts/update_services emhttp/plugins/dynamix/scripts/reload_servicesbind_changedcases:wlan0losswlan0is inactivegit diff --checkReview Notes
update.phpmechanism.rc.avahidaemon, which owns Avahi restart behavior.forcebefore being embedded in the delayed service reload command.