fix: keep SEGV traces, seed variables.cfg keys via SAVE_VARIABLE, re-enable networking at boot

BlocksScreen/BlocksScreen.py

@@ -1,5 +1,6 @@
 import logging
 import os
+import shlex
 import shutil
 import signal
 import socket
@@ -181,6 +182,30 @@ def initialize_conf_manager() -> None:
         )
 
 
+def _seed_variables(main_window: MainWindow, manager: DeviceConfigManager) -> None:
+    """SAVE_VARIABLE repo keys Klipper lacks: a file append is lost on its next save."""
+    defaults = manager.variable_defaults()
+    sent: set[str] = set()
+
+    def _on_update(status: dict) -> None:
+        live = status.get("variables")
+        if live is None:
+            return
+        for key in defaults.keys() - live.keys() - sent:
+            sent.add(key)
+            logging.info("Seeding Klipper variable %s = %s", key, defaults[key])
+            main_window.run_gcode_signal.emit(
+                f"SAVE_VARIABLE VARIABLE={key} VALUE={shlex.quote(defaults[key])}"
+            )
+
+    def _on_state(_state: str) -> None:
+        # Emits are change-only and a fast restart emits just "ready": clear on any.
+        sent.clear()
+
+    main_window.printer.save_variables_update.connect(_on_update)
+    main_window.ws.klippy_state_signal.connect(_on_state)
+
+
 def _sd_notify(msg: str) -> None:
     sock_path = os.environ.get("NOTIFY_SOCKET", "")
     if not sock_path:
@@ -321,6 +346,7 @@ def eventFilter(self, a0: QtCore.QObject | None, a1: QtCore.QEvent | None) -> bo
             # Klipper may have started before the boot sync rewrote
             # printer.cfg; restarted once Moonraker is up and no job runs.
             main_window.request_klipper_restart()
+        _seed_variables(main_window, conf_man)
         # Live side of the device templates: printer.cfg follows devices
         # plugged in while running (after the user's OK for templates that
         # ask for it), and Klipper is restarted to load it.

BlocksScreen/lib/network/worker.py

@@ -360,6 +360,8 @@ async def _async_initialize(self) -> None:
                 return
 
             self._running = True
+            # The UI has no networking toggle, so `nmcli networking off` would lock the user out.
+            await self._ensure_networking_enabled()
             await self._detect_interfaces()
 
             if await self._is_ethernet_connected():

BlocksScreen/logger.py

@@ -349,7 +349,12 @@ def _install(self) -> None:
         """Install exception hooks."""
         # Setup faulthandler for C-level crashes (segfaults, etc.)
         try:
-            self._fault_file = open(self._fault_log_path, "w")
+            # Append: the systemd restart right after a SEGV would otherwise wipe its trace
+            self._fault_file = open(self._fault_log_path, "a")
+            self._fault_file.write(
+                f"--- run started {datetime.now().isoformat()} pid {os.getpid()} ---\n"
+            )
+            self._fault_file.flush()
             faulthandler.enable(file=self._fault_file, all_threads=True)
 
             # Also dump traceback on SIGUSR1 (useful for debugging hangs)
@@ -558,11 +563,7 @@ def uninstall(self) -> None:
         if self._original_threading_excepthook and hasattr(threading, "excepthook"):
             threading.excepthook = self._original_threading_excepthook
 
-        if self._fault_file:
-            try:
-                self._fault_file.close()
-            except OSError:
-                pass  # File already closed; nothing to recover
+        # Kept open: faulthandler writes teardown SEGVs here; a closed fd gets reused.
 
         CrashHandler._installed = False
         CrashHandler._instance = None

BlocksScreen/tools/configuration_manager/configurator.py

@@ -1,4 +1,5 @@
 #!/usr/bin/python3
+import ast
 import configparser
 import hashlib
 import logging
@@ -238,6 +239,8 @@ def __init__(self, config) -> None:
         self.config_fi_name, self.config_fi_relpath = self._build_file_index(
             self.config_dir
         )
+        # Set by sync(): a Klipper that refused the old variables.cfg must restart.
+        self.variables_repaired = False
 
         self.sync()
 
@@ -658,9 +661,13 @@ def _cpy_cfg_files(self) -> None:
 
                 # Files are different so they need merging
                 ok = False
+                if f == "variables.cfg":
+                    # Klipper rewrites it from memory: new keys go in via SAVE_VARIABLE
+                    self.variables_repaired = self._repair_variables(target)
+                    continue
                 if f == "printer.cfg":
                     ok = self.merge_cfg(src_file, target, marker=SV_CONFIG_MARKER)
-                elif f in ["variables.cfg", "BlocksScreen.cfg"]:
+                elif f == "BlocksScreen.cfg":
                     ok = self.merge_cfg(src_file, target)
                 else:
                     ok = copy_file_simple(src_file, target)
@@ -671,6 +678,70 @@ def _cpy_cfg_files(self) -> None:
         except Exception as e:
             _logger.info("Caught exception while cpy files: %s" % e)
 
+    def _repair_variables(self, target: pathlib.Path) -> bool:
+        """Rewrite a variables.cfg save_variables cannot load, in its own format."""
+        strict = configparser.ConfigParser()
+        try:
+            # Same checks as save_variables.loadVariables, which fails Klippy's config
+            strict.read(target, encoding="utf-8")
+            if strict.has_section("Variables"):
+                for _, raw in strict.items("Variables"):
+                    ast.literal_eval(raw)
+            return False
+        except (configparser.Error, ValueError, SyntaxError, TypeError) as e:
+            _logger.warning("variables.cfg unloadable by Klipper, repairing: %s", e)
+        loose = configparser.ConfigParser(strict=False, interpolation=None)
+        try:
+            loose.read(target, encoding="utf-8")
+        except (configparser.Error, ValueError) as e:
+            _logger.error("variables.cfg beyond repair, left as is: %s", e)
+            return False
+        keep = {}
+        for key, raw in (
+            loose.items("Variables") if loose.has_section("Variables") else []
+        ):
+            try:
+                # Loose read skips interpolation: '%%' is Klipper's escaped '%', kept raw
+                if "%" in raw.replace("%%", ""):
+                    raise ValueError("'%' breaks save_variables' parser")
+                ast.literal_eval(raw.replace("%%", "%"))
+            except (ValueError, SyntaxError, TypeError) as e:
+                _logger.warning("Dropping variables.cfg %s = %r: %s", key, raw, e)
+                continue
+            keep[key] = raw
+        backup = self.backup_dir / f"variables.cfg.{_timestamp()}"
+        shutil.copy2(target, backup)
+        lines = [f"{k} = {v}\n" for k, v in sorted(keep.items())]
+        atomic_write_text(target, "[Variables]\n" + "".join(lines) + "\n")
+        _logger.info("Repaired variables.cfg, original kept at %s", backup)
+        return True
+
+    def variable_defaults(self) -> dict[str, str]:
+        """Repo variables.cfg keys Klipper's SAVE_VARIABLE can take, as raw literals."""
+        src_files = self.repo_fi_name.get("variables.cfg", [])
+        if not src_files:
+            return {}
+        cfg = configparser.ConfigParser(interpolation=None)
+        try:
+            cfg.read(min(src_files, key=lambda p: len(p.parents)), encoding="utf-8")
+        except configparser.Error as e:
+            _logger.error("Unreadable repo variables.cfg: %s", e)
+            return {}
+        if not cfg.has_section("Variables"):
+            return {}
+        defaults = {}
+        for key, raw in cfg.items("Variables"):
+            # HH owns mmu_*; '%' breaks save_variables' parser, ';' is a gcode comment
+            if key.startswith("mmu_") or "%" in raw or ";" in raw:
+                continue
+            try:
+                ast.literal_eval(raw)
+            except (ValueError, SyntaxError):
+                _logger.warning("variables.cfg %s is not a literal: %r", key, raw)
+                continue
+            defaults[key] = raw
+        return defaults
+
     def cmp_file(self, src, target) -> bool:
         src = pathlib.Path(src)
         target = pathlib.Path(target)

BlocksScreen/tools/configuration_manager/device_configurator.py

@@ -99,8 +99,8 @@ def __init__(
         self._consent = consent if consent is not None else DeviceConsent()
         self._devices: list[Device] = []
         self._merging: str | None = None
-        # Set by sync(): printer.cfg differs from what it was before this
-        # boot's sync, so a Klipper that already loaded it must restart.
+        # Set by sync(): printer.cfg changed or variables.cfg was repaired during
+        # this boot's sync, so a Klipper that already loaded them must restart.
         self.boot_changed = False
         _logger.info(
             "Device profiles: %s", ", ".join(p.name for p in self._profiles) or "none"
@@ -165,6 +165,8 @@ def sync(self) -> None:
         self.boot_changed = self._read_printer_cfg() != before
         if self.boot_changed:
             _logger.info("printer.cfg changed during boot sync")
+        if self.variables_repaired:
+            self.boot_changed = True
 
     def merge_cfg(self, src_file, target_file, marker="") -> bool:
         """ConfigManager.merge_cfg, remembering which file is being merged"""

tests/network/test_worker_unit.py

@@ -3145,7 +3145,11 @@ async def _test():
 class TestAsyncInitializeFull:
     def test_happy_path_full_init(self, qapp):
         w = _make(qapp, running=False)
-        w._detect_interfaces = AsyncMock()
+        order: list[str] = []
+        w._ensure_networking_enabled = AsyncMock(
+            side_effect=lambda: order.append("enable") or True
+        )
+        w._detect_interfaces = AsyncMock(side_effect=lambda: order.append("detect"))
         w._ensure_wired_autoconnect = AsyncMock()
         w._is_ethernet_connected = AsyncMock(return_value=False)
         w._activate_saved_vlans = AsyncMock()
@@ -3162,7 +3166,11 @@ def test_happy_path_full_init(self, qapp):
         _run(w._async_initialize())
 
         assert w._running is True
+        # Boot clears `nmcli networking off`, not just the Wi-Fi preflight.
+        w._ensure_networking_enabled.assert_awaited_once()
         w._detect_interfaces.assert_awaited_once()
+        # Detection against a disabled NM finds no devices, so enabling must come first.
+        assert order == ["enable", "detect"]
         # Boot must not re-arm: NM's latch is how "ethernet off" survives a reboot.
         w._ensure_wired_autoconnect.assert_not_awaited()
         w._start_signal_listeners.assert_awaited_once()

tests/util/test_configurator_unit.py

@@ -890,17 +890,106 @@ def test_printer_cfg_differs_merge_called(self, tmp_path):
         assert SV_CONFIG_MARKER in merged
         assert "#*# user: saved" in merged
 
-    def test_variables_cfg_differs_merge_called(self, tmp_path):
+    def test_existing_variables_cfg_left_to_klipper(self, tmp_path):
         mgr = _make_manager(tmp_path, cpy_files=["variables.cfg"])
         src = _write(mgr.repo / "variables.cfg", "[g]\nopt_a: repo\nopt_b: repo\n")
         tgt = _write(mgr.config_dir / "variables.cfg", "[g]\nopt_a: existing\n")
         mgr.repo_fi_name = {"variables.cfg": [src]}
         mgr.config_fi_name = {"variables.cfg": [tgt]}
         mgr._cpy_cfg_files()
-        result = configparser.ConfigParser()
-        result.read_string(tgt.read_text())
-        assert result.get("g", "opt_a") == "existing"
-        assert result.get("g", "opt_b") == "repo"
+        # Klipper's next SAVE_VARIABLE would drop an append; the seeder adds keys instead
+        assert tgt.read_text() == "[g]\nopt_a: existing\n"
+
+    def test_unloadable_variables_cfg_repaired(self, tmp_path):
+        mgr = _make_manager(tmp_path, cpy_files=["variables.cfg"])
+        src = _write(mgr.repo / "variables.cfg", "[Variables]\na = 1\n")
+        tgt = _write(
+            mgr.config_dir / "variables.cfg",
+            "[Variables]\nb = 2\na = 3\n[Variables]\nc = 'x'\n",
+        )
+        mgr.repo_fi_name = {"variables.cfg": [src]}
+        mgr.config_fi_name = {"variables.cfg": [tgt]}
+        mgr._cpy_cfg_files()
+        assert tgt.read_text() == "[Variables]\na = 3\nb = 2\nc = 'x'\n\n"
+
+
+class TestRepairVariables:
+    BROKEN = "[Variables]\nb = 2\n[Variables]\nb = 5\npct = '50%'\nbare = PLA\n"
+
+    def test_loadable_file_untouched(self, tmp_path):
+        mgr = _make_manager(tmp_path)
+        text = "[Variables]\nb = 2\nok = '50%%'\n"
+        tgt = _write(mgr.config_dir / "variables.cfg", text)
+        assert mgr._repair_variables(tgt) is False
+        assert tgt.read_text() == text
+        assert not list(mgr.backup_dir.iterdir())
+
+    def test_rewrites_as_klipper_loads_it(self, tmp_path):
+        mgr = _make_manager(tmp_path)
+        tgt = _write(mgr.config_dir / "variables.cfg", self.BROKEN)
+        assert mgr._repair_variables(tgt) is True
+        # Later duplicate wins; values Klipper's parser rejects are dropped
+        assert tgt.read_text() == "[Variables]\nb = 5\n\n"
+        assert mgr._repair_variables(tgt) is False
+
+    def test_original_backed_up(self, tmp_path):
+        mgr = _make_manager(tmp_path)
+        tgt = _write(mgr.config_dir / "variables.cfg", self.BROKEN)
+        mgr._repair_variables(tgt)
+        (backup,) = mgr.backup_dir.iterdir()
+        assert backup.read_text() == self.BROKEN
+
+    def test_unparseable_file_left_as_is(self, tmp_path):
+        mgr = _make_manager(tmp_path)
+        tgt = _write(mgr.config_dir / "variables.cfg", "a = 1\n")
+        assert mgr._repair_variables(tgt) is False
+        assert tgt.read_text() == "a = 1\n"
+
+    def test_escaped_percent_kept(self, tmp_path):
+        mgr = _make_manager(tmp_path)
+        text = "[Variables]\nbare = PLA\nok = '50%%'\n"
+        tgt = _write(mgr.config_dir / "variables.cfg", text)
+        assert mgr._repair_variables(tgt) is True
+        assert tgt.read_text() == "[Variables]\nok = '50%%'\n\n"
+        assert mgr._repair_variables(tgt) is False
+
+    def test_failed_repair_keeps_original(self, tmp_path, monkeypatch):
+        mgr = _make_manager(tmp_path)
+        tgt = _write(mgr.config_dir / "variables.cfg", self.BROKEN)
+
+        def power_cut(*_):
+            raise OSError("power cut")
+
+        monkeypatch.setattr(os, "replace", power_cut)
+        with pytest.raises(OSError):
+            mgr._repair_variables(tgt)
+        assert tgt.read_text() == self.BROKEN
+        assert not list(mgr.config_dir.glob(".variables.cfg.*"))
+
+
+class TestVariableDefaults:
+    def _mgr(self, tmp_path, text: str) -> ConfigManager:
+        mgr = _make_manager(tmp_path)
+        mgr.repo_fi_name = {"variables.cfg": [_write(mgr.repo / "variables.cfg", text)]}
+        return mgr
+
+    def test_returns_literal_keys(self, tmp_path):
+        mgr = self._mgr(tmp_path, "[Variables]\netemp=0\nprintcore_type: 'Standard'\n")
+        assert mgr.variable_defaults() == {"etemp": "0", "printcore_type": "'Standard'"}
+
+    def test_skips_hh_owned_and_unsendable_values(self, tmp_path):
+        mgr = self._mgr(
+            tmp_path,
+            "[Variables]\nmmu_state_gate_selected = 0\npct = '50%'\n"
+            "semi = 'a;b'\nbare = PLA\nok = True\n",
+        )
+        assert mgr.variable_defaults() == {"ok": "True"}
+
+    def test_no_repo_file_or_section(self, tmp_path):
+        mgr = _make_manager(tmp_path)
+        mgr.repo_fi_name = {}
+        assert mgr.variable_defaults() == {}
+        assert self._mgr(tmp_path, "[Other]\na = 1\n").variable_defaults() == {}
 
     def test_seed_only_file_kept_when_present(self, tmp_path):
         mgr = _make_manager(tmp_path, cpy_files=["moonraker.conf"])

tests/util/test_device_configurator_unit.py

@@ -237,6 +237,14 @@ def test_amu_removed_while_off(self, dirs, monkeypatch):
         _boot(dirs, monkeypatch, [AMU])
         assert _boot(dirs, monkeypatch, []).boot_changed is True
 
+    def test_variables_cfg_repaired(self, dirs, monkeypatch):
+        _, config_dir, _ = dirs
+        _boot(dirs, monkeypatch, [AMU])
+        # Klipper already refused this file, only a restart loads the repair
+        (config_dir / "variables.cfg").write_text("[Variables]\na = 1\nbare = PLA\n")
+        assert _boot(dirs, monkeypatch, [AMU]).boot_changed is True
+        assert _boot(dirs, monkeypatch, [AMU]).boot_changed is False
+
 
 class TestScanDevices:
     def test_by_id_listing_fills_in_what_the_daemon_missed(self, monkeypatch):