From 62dddbad348eec439b17906d1960ca8eea2320e9 Mon Sep 17 00:00:00 2001 From: Guilherme Costa Date: Wed, 21 Oct 2026 15:06:31 +0100 Subject: [PATCH 1/6] fix: keep SEGV traces, seed variables.cfg keys via SAVE_VARIABLE, re-enable networking at boot --- BlocksScreen/BlocksScreen.py | 21 +++++++ BlocksScreen/lib/network/worker.py | 2 + BlocksScreen/logger.py | 3 +- .../configuration_manager/configurator.py | 32 ++++++++++- tests/network/test_worker_unit.py | 3 + tests/util/test_configurator_unit.py | 33 +++++++++-- tests/widgets/test_seed_variables.py | 55 +++++++++++++++++++ 7 files changed, 142 insertions(+), 7 deletions(-) create mode 100644 tests/widgets/test_seed_variables.py diff --git a/BlocksScreen/BlocksScreen.py b/BlocksScreen/BlocksScreen.py index 9cc399e7..e4eeffae 100644 --- a/BlocksScreen/BlocksScreen.py +++ b/BlocksScreen/BlocksScreen.py @@ -1,5 +1,6 @@ import logging import os +import shlex import shutil import signal import socket @@ -181,6 +182,25 @@ 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])}" + ) + + main_window.printer.save_variables_update.connect(_on_update) + + def _sd_notify(msg: str) -> None: sock_path = os.environ.get("NOTIFY_SOCKET", "") if not sock_path: @@ -321,6 +341,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. diff --git a/BlocksScreen/lib/network/worker.py b/BlocksScreen/lib/network/worker.py index aa995a69..351e4ba0 100644 --- a/BlocksScreen/lib/network/worker.py +++ b/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(): diff --git a/BlocksScreen/logger.py b/BlocksScreen/logger.py index 7b3b7d50..2ce42c84 100644 --- a/BlocksScreen/logger.py +++ b/BlocksScreen/logger.py @@ -349,7 +349,8 @@ 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") faulthandler.enable(file=self._fault_file, all_threads=True) # Also dump traceback on SIGUSR1 (useful for debugging hangs) diff --git a/BlocksScreen/tools/configuration_manager/configurator.py b/BlocksScreen/tools/configuration_manager/configurator.py index cd619be3..610b7cb1 100644 --- a/BlocksScreen/tools/configuration_manager/configurator.py +++ b/BlocksScreen/tools/configuration_manager/configurator.py @@ -1,4 +1,5 @@ #!/usr/bin/python3 +import ast import configparser import hashlib import logging @@ -626,9 +627,12 @@ 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 + 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) @@ -639,6 +643,32 @@ def _cpy_cfg_files(self) -> None: except Exception as e: _logger.info("Caught exception while cpy files: %s" % e) + 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, ';' starts 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) diff --git a/tests/network/test_worker_unit.py b/tests/network/test_worker_unit.py index ff4e4ad5..69e3d9be 100644 --- a/tests/network/test_worker_unit.py +++ b/tests/network/test_worker_unit.py @@ -3145,6 +3145,7 @@ async def _test(): class TestAsyncInitializeFull: def test_happy_path_full_init(self, qapp): w = _make(qapp, running=False) + w._ensure_networking_enabled = AsyncMock(return_value=True) w._detect_interfaces = AsyncMock() w._ensure_wired_autoconnect = AsyncMock() w._is_ethernet_connected = AsyncMock(return_value=False) @@ -3162,6 +3163,8 @@ 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() # Boot must not re-arm: NM's latch is how "ethernet off" survives a reboot. w._ensure_wired_autoconnect.assert_not_awaited() diff --git a/tests/util/test_configurator_unit.py b/tests/util/test_configurator_unit.py index e67dd1ef..2c51d908 100644 --- a/tests/util/test_configurator_unit.py +++ b/tests/util/test_configurator_unit.py @@ -820,17 +820,40 @@ 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" + + +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() == {} # ========================================================================= diff --git a/tests/widgets/test_seed_variables.py b/tests/widgets/test_seed_variables.py new file mode 100644 index 00000000..0ab33891 --- /dev/null +++ b/tests/widgets/test_seed_variables.py @@ -0,0 +1,55 @@ +"""_seed_variables: repo variables.cfg keys reach Klipper via SAVE_VARIABLE, once each.""" + +import ast +import shlex +from unittest.mock import MagicMock + +import pytest + +from .test_sigterm_shutdown import _load_entry_module + + +@pytest.fixture(scope="module") +def entry_mod(): + return _load_entry_module() + + +def _seed(entry_mod, defaults: dict[str, str]): + window = MagicMock() + manager = MagicMock() + manager.variable_defaults.return_value = defaults + entry_mod._seed_variables(window, manager) + on_update = window.printer.save_variables_update.connect.call_args.args[0] + return on_update, window.run_gcode_signal.emit + + +def test_only_missing_keys_are_saved(entry_mod): + on_update, emit = _seed(entry_mod, {"etemp": "0", "printcore_type": "'Standard'"}) + on_update({"variables": {"etemp": 5}}) + emit.assert_called_once_with( + "SAVE_VARIABLE VARIABLE=printcore_type VALUE=" + shlex.quote("'Standard'") + ) + + +def test_value_survives_klipper_param_parsing(entry_mod): + on_update, emit = _seed(entry_mod, {"material_type": "'PLA #1'"}) + on_update({"variables": {}}) + # Klipper splits extended params with posix shlex and '#;' commenters + lex = shlex.shlex(emit.call_args.args[0].split(" ", 1)[1], posix=True) + lex.whitespace_split = True + lex.commenters = "#;" + params = dict(arg.split("=", 1) for arg in lex) + assert ast.literal_eval(params["VALUE"]) == "PLA #1" + + +def test_key_is_sent_once_even_before_klipper_reports_it(entry_mod): + on_update, emit = _seed(entry_mod, {"heating": "False"}) + on_update({"variables": {}}) + on_update({"variables": {"etemp": 0}}) + assert emit.call_count == 1 + + +def test_update_without_variables_is_ignored(entry_mod): + on_update, emit = _seed(entry_mod, {"heating": "False"}) + on_update({}) + emit.assert_not_called() From f41d9996c7548368d007779e435d4353449bbe5e Mon Sep 17 00:00:00 2001 From: Guilherme Costa Date: Wed, 21 Oct 2026 15:48:56 +0100 Subject: [PATCH 2/6] fix: retry variable seeds after Klippy restart, mark each run in the fault log --- BlocksScreen/BlocksScreen.py | 6 ++++++ BlocksScreen/logger.py | 4 ++++ tests/network/test_worker_unit.py | 9 +++++++-- tests/widgets/test_seed_variables.py | 21 ++++++++++++++++++++- 4 files changed, 37 insertions(+), 3 deletions(-) diff --git a/BlocksScreen/BlocksScreen.py b/BlocksScreen/BlocksScreen.py index e4eeffae..667fec45 100644 --- a/BlocksScreen/BlocksScreen.py +++ b/BlocksScreen/BlocksScreen.py @@ -198,7 +198,13 @@ def _on_update(status: dict) -> None: f"SAVE_VARIABLE VARIABLE={key} VALUE={shlex.quote(defaults[key])}" ) + def _on_state(state: str) -> None: + # A send lost to a restart or shutdown is retried once Klippy is ready again. + if state != "ready": + 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: diff --git a/BlocksScreen/logger.py b/BlocksScreen/logger.py index 2ce42c84..a6a03454 100644 --- a/BlocksScreen/logger.py +++ b/BlocksScreen/logger.py @@ -351,6 +351,10 @@ def _install(self) -> None: try: # 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) diff --git a/tests/network/test_worker_unit.py b/tests/network/test_worker_unit.py index 69e3d9be..f87f2e9d 100644 --- a/tests/network/test_worker_unit.py +++ b/tests/network/test_worker_unit.py @@ -3145,8 +3145,11 @@ async def _test(): class TestAsyncInitializeFull: def test_happy_path_full_init(self, qapp): w = _make(qapp, running=False) - w._ensure_networking_enabled = AsyncMock(return_value=True) - 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() @@ -3166,6 +3169,8 @@ def test_happy_path_full_init(self, qapp): # 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() diff --git a/tests/widgets/test_seed_variables.py b/tests/widgets/test_seed_variables.py index 0ab33891..fdac742e 100644 --- a/tests/widgets/test_seed_variables.py +++ b/tests/widgets/test_seed_variables.py @@ -14,11 +14,16 @@ def entry_mod(): return _load_entry_module() -def _seed(entry_mod, defaults: dict[str, str]): +def _window(entry_mod, defaults: dict[str, str]) -> MagicMock: window = MagicMock() manager = MagicMock() manager.variable_defaults.return_value = defaults entry_mod._seed_variables(window, manager) + return window + + +def _seed(entry_mod, defaults: dict[str, str]): + window = _window(entry_mod, defaults) on_update = window.printer.save_variables_update.connect.call_args.args[0] return on_update, window.run_gcode_signal.emit @@ -49,6 +54,20 @@ def test_key_is_sent_once_even_before_klipper_reports_it(entry_mod): assert emit.call_count == 1 +def test_unsaved_key_is_resent_after_klippy_restart(entry_mod): + window = _window(entry_mod, {"heating": "False"}) + on_update = window.printer.save_variables_update.connect.call_args.args[0] + on_state = window.ws.klippy_state_signal.connect.call_args.args[0] + emit = window.run_gcode_signal.emit + on_update({"variables": {}}) + on_state("ready") + on_update({"variables": {}}) + assert emit.call_count == 1 + on_state("startup") + on_update({"variables": {}}) + assert emit.call_count == 2 + + def test_update_without_variables_is_ignored(entry_mod): on_update, emit = _seed(entry_mod, {"heating": "False"}) on_update({}) From 2a284daccea8a855e28c7b0c73442205e5d4ee9d Mon Sep 17 00:00:00 2001 From: Guilherme Costa Date: Wed, 21 Oct 2026 16:00:53 +0100 Subject: [PATCH 3/6] fix(logger): keep the fault log open so a teardown SEGV is still recorded --- BlocksScreen/logger.py | 6 +--- tests/util/test_fault_log_unit.py | 47 +++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 5 deletions(-) create mode 100644 tests/util/test_fault_log_unit.py diff --git a/BlocksScreen/logger.py b/BlocksScreen/logger.py index a6a03454..aedfa4f3 100644 --- a/BlocksScreen/logger.py +++ b/BlocksScreen/logger.py @@ -563,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 diff --git a/tests/util/test_fault_log_unit.py b/tests/util/test_fault_log_unit.py new file mode 100644 index 00000000..23d90d5c --- /dev/null +++ b/tests/util/test_fault_log_unit.py @@ -0,0 +1,47 @@ +"""CrashHandler fault log: a SEGV after shutdown still lands in the fault log.""" + +import subprocess +import sys +import textwrap +from pathlib import Path + +_REPO = Path(__file__).resolve().parents[2] + +_CHILD = textwrap.dedent( + """ + import faulthandler, sys + from BlocksScreen.logger import CrashHandler + fault, other = sys.argv[1], sys.argv[2] + CrashHandler.install(crash_log_path=fault + ".crash", fault_log_path=fault) + CrashHandler._instance.uninstall() + with open(other, "w") as f: + f.write("unrelated\\n") + f.flush() + faulthandler._sigsegv() + """ +) + + +def _crash_after_uninstall(tmp_path: Path) -> tuple[str, str]: + fault, other = tmp_path / "fault.log", tmp_path / "other.log" + fault.write_text("previous run\n") + proc = subprocess.run( + [sys.executable, "-c", _CHILD, str(fault), str(other)], + cwd=_REPO, + capture_output=True, + timeout=20, + check=False, + ) + assert proc.returncode != 0 + return fault.read_text(), other.read_text() + + +def test_segv_after_uninstall_goes_to_fault_log(tmp_path): + fault, other = _crash_after_uninstall(tmp_path) + assert "Segmentation fault" in fault + assert "Segmentation fault" not in other + + +def test_fault_log_keeps_previous_runs_and_marks_each_start(tmp_path): + fault, _ = _crash_after_uninstall(tmp_path) + assert fault.startswith("previous run\n--- run started ") From e45eb94bb402fb9672c93bd82d475b9fc6adbdda Mon Sep 17 00:00:00 2001 From: Guilherme Costa Date: Wed, 21 Oct 2026 16:42:24 +0100 Subject: [PATCH 4/6] fix(configurator): repair a variables.cfg Klipper cannot load, reseed after any Klippy state --- BlocksScreen/BlocksScreen.py | 7 ++- .../configuration_manager/configurator.py | 40 ++++++++++++++++- tests/util/test_configurator_unit.py | 45 +++++++++++++++++++ tests/widgets/test_seed_variables.py | 4 +- 4 files changed, 89 insertions(+), 7 deletions(-) diff --git a/BlocksScreen/BlocksScreen.py b/BlocksScreen/BlocksScreen.py index 667fec45..0b425d28 100644 --- a/BlocksScreen/BlocksScreen.py +++ b/BlocksScreen/BlocksScreen.py @@ -198,10 +198,9 @@ def _on_update(status: dict) -> None: f"SAVE_VARIABLE VARIABLE={key} VALUE={shlex.quote(defaults[key])}" ) - def _on_state(state: str) -> None: - # A send lost to a restart or shutdown is retried once Klippy is ready again. - if state != "ready": - sent.clear() + 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) diff --git a/BlocksScreen/tools/configuration_manager/configurator.py b/BlocksScreen/tools/configuration_manager/configurator.py index 610b7cb1..950d8b7c 100644 --- a/BlocksScreen/tools/configuration_manager/configurator.py +++ b/BlocksScreen/tools/configuration_manager/configurator.py @@ -629,6 +629,7 @@ def _cpy_cfg_files(self) -> None: ok = False if f == "variables.cfg": # Klipper rewrites it from memory: new keys go in via SAVE_VARIABLE + self._repair_variables(target) continue if f == "printer.cfg": ok = self.merge_cfg(src_file, target, marker=SV_CONFIG_MARKER) @@ -643,6 +644,43 @@ 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: + if "%" in raw: + raise ValueError("'%' breaks save_variables' parser") + ast.literal_eval(raw) + 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())] + target.write_text("[Variables]\n" + "".join(lines) + "\n", encoding="utf-8") + _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", []) @@ -658,7 +696,7 @@ def variable_defaults(self) -> dict[str, str]: return {} defaults = {} for key, raw in cfg.items("Variables"): - # HH owns mmu_*; '%' breaks save_variables' parser, ';' starts a gcode comment + # HH owns mmu_*; '%' breaks save_variables' parser, ';' is a gcode comment if key.startswith("mmu_") or "%" in raw or ";" in raw: continue try: diff --git a/tests/util/test_configurator_unit.py b/tests/util/test_configurator_unit.py index 2c51d908..df3bde39 100644 --- a/tests/util/test_configurator_unit.py +++ b/tests/util/test_configurator_unit.py @@ -830,6 +830,51 @@ def test_existing_variables_cfg_left_to_klipper(self, tmp_path): # 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" + class TestVariableDefaults: def _mgr(self, tmp_path, text: str) -> ConfigManager: diff --git a/tests/widgets/test_seed_variables.py b/tests/widgets/test_seed_variables.py index fdac742e..b9662675 100644 --- a/tests/widgets/test_seed_variables.py +++ b/tests/widgets/test_seed_variables.py @@ -60,10 +60,10 @@ def test_unsaved_key_is_resent_after_klippy_restart(entry_mod): on_state = window.ws.klippy_state_signal.connect.call_args.args[0] emit = window.run_gcode_signal.emit on_update({"variables": {}}) - on_state("ready") on_update({"variables": {}}) assert emit.call_count == 1 - on_state("startup") + # Fast restart: disconnect resets the stored state, so only "ready" is emitted + on_state("ready") on_update({"variables": {}}) assert emit.call_count == 2 From e6067abd091665146e89ae69e0457c9ee8e24435 Mon Sep 17 00:00:00 2001 From: Guilherme Costa Date: Wed, 21 Oct 2026 17:07:08 +0100 Subject: [PATCH 5/6] fix(configurator): restart Klipper after a variables.cfg repair, keep escaped % --- BlocksScreen/tools/configuration_manager/configurator.py | 9 ++++++--- .../tools/configuration_manager/device_configurator.py | 6 ++++-- tests/util/test_configurator_unit.py | 8 ++++++++ tests/util/test_device_configurator_unit.py | 8 ++++++++ 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/BlocksScreen/tools/configuration_manager/configurator.py b/BlocksScreen/tools/configuration_manager/configurator.py index 950d8b7c..0a29076a 100644 --- a/BlocksScreen/tools/configuration_manager/configurator.py +++ b/BlocksScreen/tools/configuration_manager/configurator.py @@ -225,6 +225,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() @@ -629,7 +631,7 @@ def _cpy_cfg_files(self) -> None: ok = False if f == "variables.cfg": # Klipper rewrites it from memory: new keys go in via SAVE_VARIABLE - self._repair_variables(target) + self.variables_repaired = self._repair_variables(target) continue if f == "printer.cfg": ok = self.merge_cfg(src_file, target, marker=SV_CONFIG_MARKER) @@ -667,9 +669,10 @@ def _repair_variables(self, target: pathlib.Path) -> bool: loose.items("Variables") if loose.has_section("Variables") else [] ): try: - if "%" in raw: + # Loose read skips interpolation: '%%' is Klipper's escaped '%', kept raw + if "%" in raw.replace("%%", ""): raise ValueError("'%' breaks save_variables' parser") - ast.literal_eval(raw) + ast.literal_eval(raw.replace("%%", "%")) except (ValueError, SyntaxError, TypeError) as e: _logger.warning("Dropping variables.cfg %s = %r: %s", key, raw, e) continue diff --git a/BlocksScreen/tools/configuration_manager/device_configurator.py b/BlocksScreen/tools/configuration_manager/device_configurator.py index a7cfbc10..3b25a8ee 100644 --- a/BlocksScreen/tools/configuration_manager/device_configurator.py +++ b/BlocksScreen/tools/configuration_manager/device_configurator.py @@ -127,8 +127,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" @@ -193,6 +193,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""" diff --git a/tests/util/test_configurator_unit.py b/tests/util/test_configurator_unit.py index df3bde39..993b3dee 100644 --- a/tests/util/test_configurator_unit.py +++ b/tests/util/test_configurator_unit.py @@ -875,6 +875,14 @@ def test_unparseable_file_left_as_is(self, tmp_path): 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 + class TestVariableDefaults: def _mgr(self, tmp_path, text: str) -> ConfigManager: diff --git a/tests/util/test_device_configurator_unit.py b/tests/util/test_device_configurator_unit.py index b4cd40fd..1d8d2aa2 100644 --- a/tests/util/test_device_configurator_unit.py +++ b/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): From 9c31fc1248b283ef753b7ce3a752aff96442a4f5 Mon Sep 17 00:00:00 2001 From: Guilherme Costa Date: Wed, 21 Oct 2026 17:17:51 +0100 Subject: [PATCH 6/6] fix(configurator): write the variables.cfg repair atomically --- .../tools/configuration_manager/configurator.py | 2 +- tests/util/test_configurator_unit.py | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/BlocksScreen/tools/configuration_manager/configurator.py b/BlocksScreen/tools/configuration_manager/configurator.py index 4aab10d7..c47b679a 100644 --- a/BlocksScreen/tools/configuration_manager/configurator.py +++ b/BlocksScreen/tools/configuration_manager/configurator.py @@ -712,7 +712,7 @@ def _repair_variables(self, target: pathlib.Path) -> bool: backup = self.backup_dir / f"variables.cfg.{_timestamp()}" shutil.copy2(target, backup) lines = [f"{k} = {v}\n" for k, v in sorted(keep.items())] - target.write_text("[Variables]\n" + "".join(lines) + "\n", encoding="utf-8") + atomic_write_text(target, "[Variables]\n" + "".join(lines) + "\n") _logger.info("Repaired variables.cfg, original kept at %s", backup) return True diff --git a/tests/util/test_configurator_unit.py b/tests/util/test_configurator_unit.py index 0da5d2dd..64d7284c 100644 --- a/tests/util/test_configurator_unit.py +++ b/tests/util/test_configurator_unit.py @@ -953,6 +953,19 @@ def test_escaped_percent_kept(self, tmp_path): 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: