From 1a3e6f0685bab61752a0221730f95a93c2a96f34 Mon Sep 17 00:00:00 2001 From: Chuck Date: Tue, 12 May 2026 13:25:12 -0400 Subject: [PATCH] fix: address five review findings (NM retry loop, start_display message, code quality) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - wifi_monitor_daemon: reset _consecutive_internet_failures = 0 in both NM-restart exception handlers; previously both left the counter at threshold, causing an immediate retry on the next iteration instead of waiting another full backoff period - api_v3: fix start_display failure message — when mode is set and systemctl returns non-zero, message now includes the failure reason and a hint rather than always reporting success phrasing - wifi_manager: move _redirect_backend from class variable to instance variable in __init__ alongside _ap_enabled_at; class-level default shadowed correctly in practice (single instance) but was misleading - wifi_manager: narrow broad except Exception in _check_internet_connectivity to (subprocess.SubprocessError, OSError) for ping and OSError for HTTP (urllib.error.URLError is an OSError subclass in Python 3) - wifi_manager: remove redundant local 'import re as _re' in _validate_ap_config; re is already imported at module level (line 37) Co-Authored-By: Claude Sonnet 4.6 --- scripts/utils/wifi_monitor_daemon.py | 6 ++++-- src/wifi_manager.py | 13 +++++-------- web_interface/blueprints/api_v3.py | 3 ++- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/scripts/utils/wifi_monitor_daemon.py b/scripts/utils/wifi_monitor_daemon.py index 3afd212d..05ed429c 100755 --- a/scripts/utils/wifi_monitor_daemon.py +++ b/scripts/utils/wifi_monitor_daemon.py @@ -153,10 +153,12 @@ class WiFiMonitorDaemon: self.wifi_manager._disconnected_checks = 0 except subprocess.CalledProcessError as e: logger.error(f"NetworkManager restart failed (rc={e.returncode}); " - "keeping failure counter unchanged") + "resetting failure counter to avoid tight retry loop") + self._consecutive_internet_failures = 0 except Exception as e: logger.error(f"NetworkManager restart error: {e}; " - "keeping failure counter unchanged") + "resetting failure counter to avoid tight retry loop") + self._consecutive_internet_failures = 0 else: self._consecutive_internet_failures = 0 else: diff --git a/src/wifi_manager.py b/src/wifi_manager.py index bbb7a543..733d5f6c 100644 --- a/src/wifi_manager.py +++ b/src/wifi_manager.py @@ -144,6 +144,8 @@ class WiFiManager: # Timestamp set when AP mode is enabled; used for the idle-timeout check self._ap_enabled_at: Optional[float] = None + # Which redirect backend was used (iptables/nftables/None); set per-instance + self._redirect_backend: Optional[str] = None logger.info(f"WiFi Manager initialized - nmcli: {self.has_nmcli}, iwlist: {self.has_iwlist}, " f"hostapd: {self.has_hostapd}, dnsmasq: {self.has_dnsmasq}, " @@ -691,9 +693,8 @@ class WiFiManager: def _validate_ap_config(self) -> Tuple[str, int]: """Return a sanitized (ssid, channel) pair from config, falling back to defaults.""" - import re as _re ssid = str(self.config.get("ap_ssid", DEFAULT_AP_SSID)) - if not ssid or len(ssid) > 32 or not _re.match(r'^[\x20-\x7E]+$', ssid): + if not ssid or len(ssid) > 32 or not re.match(r'^[\x20-\x7E]+$', ssid): logger.warning(f"AP SSID '{ssid}' is invalid, falling back to default") ssid = DEFAULT_AP_SSID try: @@ -705,10 +706,6 @@ class WiFiManager: channel = DEFAULT_AP_CHANNEL return ssid, channel - # Tracks which redirect backend was used so teardown uses the same one. - # Value is "iptables", "nftables", or None (not set up). - _redirect_backend: Optional[str] = None - def _setup_iptables_redirect(self) -> bool: """ Add port 80 → 5000 redirect rules for the captive portal. @@ -936,14 +933,14 @@ class WiFiManager: if r.returncode == 0: logger.debug("Internet connectivity confirmed via ping 8.8.8.8") return True - except Exception: + except (subprocess.SubprocessError, OSError): pass try: import urllib.request as _ureq _ureq.urlopen("http://connectivity-check.ubuntu.com/", timeout=timeout) logger.debug("Internet connectivity confirmed via HTTP check") return True - except Exception: + except OSError: pass logger.debug("Internet connectivity check failed (both ping and HTTP)") return False diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index dca2f33a..aa2c5616 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -1720,7 +1720,8 @@ def execute_system_action(): capture_output=True, text=True, timeout=15) return jsonify({ 'status': 'success' if result.returncode == 0 else 'error', - 'message': f'Started display in {mode} mode', + 'message': f'Started display in {mode} mode' if result.returncode == 0 + else f'Failed to start display in {mode} mode: {result.stderr.strip() or "check sudo systemctl status ledmatrix.service"}', 'returncode': result.returncode, 'stdout': result.stdout, 'stderr': result.stderr