Repository navigation
https-dns-proxy: enable the existing post-start health check - #30787
EmreYavuzalp wants to merge 1 commit into
Conversation
| if ! is_resolver_working; then | ||
| output "Health check failed, falling back to plain DNS " | ||
| rc_procd stop_service 'on_failed_health_check' && service_stopped 'on_failed_health_check' | ||
| output_okn | ||
| fi |
There was a problem hiding this comment.
Blocking: start_service runs inside rc_procd in rc.common:129-135, so the instances are only handed to procd by procd_close_service after this function returns — the health check queries a proxy that is not running yet and will fail on every start. The nested rc_procd stop_service then calls json_init in _procd_open_service, wiping the pending instance JSON, so the outer close registers no instances and DoH is effectively disabled for everyone with a non-- heartbeat_domain. The check needs to run after the service has been registered (e.g. a backgrounded/deferred check that invokes /etc/init.d/https-dns-proxy stop), not inline here — likely why it was commented out.
Generated by Claude Code
There was a problem hiding this comment.
Good catch, confirmed by reading rc.common/procd.sh - the inline call plus the nested rc_procd stop_service was exactly the bug, almost certainly why it was commented out to begin with.
Fixed: the check now runs in a backgrounded subshell after start_service() returns (so procd has actually submitted and started the instance JSON by the time it runs), and the fallback goes through a plain top-level /etc/init.d/https-dns-proxy stop instead of a nested rc_procd call, so there's no _procd_open_service/json_init collision with the outer transaction. Pushed as b1c1faa.
1e4cf8b to
b1c1faa
Compare
| ( | ||
| if ! is_resolver_working; then | ||
| logger -t "$packageName" "Health check failed, falling back to plain DNS" | ||
| "${IPKG_INSTROOT}/etc/init.d/${packageName}" stop 'on_failed_health_check' >/dev/null 2>&1 |
There was a problem hiding this comment.
rc.common's stop() forwards $1 to procd_kill as the instance name (rc.common:159), so this asks procd to delete a non-existent instance on_failed_health_check. dnsmasq gets restored, but the broken proxy instances stay registered and keep respawning. Drop the argument so the whole service is killed.
| "${IPKG_INSTROOT}/etc/init.d/${packageName}" stop 'on_failed_health_check' >/dev/null 2>&1 | |
| "${IPKG_INSTROOT}/etc/init.d/${packageName}" stop >/dev/null 2>&1 |
Generated by Claude Code
| PKG_NAME:=https-dns-proxy | ||
| PKG_VERSION:=2026.05.06 | ||
| PKG_RELEASE:=1 | ||
| PKG_RELEASE:=3 |
There was a problem hiding this comment.
nit: this is a single change on top of PKG_RELEASE:=1, so it should go to 2, not 3.
| PKG_RELEASE:=3 | |
| PKG_RELEASE:=2 |
Generated by Claude Code
There was a problem hiding this comment.
Good catch, fixed - was double-bumping it across the amend. Pushed as 3f4ae18.
start_service() already has is_resolver_working() and an on_failed_health_check path to fall back to plain DNS, but the actual call was left commented out, so an instance that starts but never successfully resolves anything just sits there broken. This is a real, reproducible failure mode on boards with no RTC: at boot the clock is wrong, so the DoH TLS handshake fails and the proxy never comes up; meanwhile sysntpd is trying to fix the clock by resolving pool.ntp.org by hostname, through a dnsmasq that https-dns-proxy has already set to noresolv + single upstream (the dead proxy) - DNS never recovers and the NTP fix the clock depends on never completes either, so it stays broken until a reboot or a manual restart. Reproduced and confirmed on two different RTC-less boards. Enabling the existing check breaks that deadlock: if the resolver isn't healthy a few seconds after start, the service stops itself and the previous working DNS config (saved by dhcp_backup) is restored, so the device always has working DNS even when DoH can't come up. Anyone who doesn't want this can already opt out via the existing heartbeat_domain='-' option. The check has to run deferred and out-of-process, not inline: start_service() runs inside the outer rc_procd transaction, and the instance JSON built by procd_open_instance/procd_close_instance in start_instance() is only handed to procd once start_service() returns. Calling is_resolver_working() inline tests a proxy procd hasn't actually started yet (so it fails every time), and the nested `rc_procd stop_service` that followed went through _procd_open_service, which does json_init - wiping that still-pending JSON before it was ever submitted, so no instance got registered with procd at all. That's almost certainly why this call was commented out originally. Backgrounding the check and falling back through a fresh top-level `/etc/init.d/https-dns-proxy stop` avoids both problems. Bumped PKG_RELEASE since files/ changed. Signed-off-by: Emre Yavuzalp <emreyavuzalp2@gmail.com>
b1c1faa to
3f4ae18
Compare
`start_service()` already has `is_resolver_working()` and an `on_failed_health_check` path that falls back to plain DNS, but the actual call to it is commented out, so an instance that starts but never successfully resolves anything just sits there broken with nothing to catch it.
This is a real, reproducible failure mode on boards with no RTC: at boot the clock is wrong, so the DoH TLS handshake fails and the proxy never comes up; meanwhile `sysntpd` is trying to fix the clock by resolving `pool.ntp.org` by hostname, through a dnsmasq that https-dns-proxy has already set to `noresolv` + single upstream (the dead proxy) - DNS never recovers, and the NTP fix the clock depends on never completes either, so it stays broken until a reboot or a manual service restart. We've reproduced and confirmed this on two different RTC-less boards.
Enabling the existing check breaks that deadlock: if the resolver isn't healthy a few seconds after start, the service stops itself and the previous working DNS config (saved by `dhcp_backup`) is restored, so the device always ends up with working DNS even when DoH itself can't come up. Anyone who doesn't want this can already opt out via the existing `heartbeat_domain='-'` option, so this doesn't change behavior for anyone who's deliberately configured around it.
Bumped `PKG_RELEASE` since `files/` changed.