fix(updater): keep moonraker.conf, clean shutdown, safe updates, Spoolman pin

BlocksScreen/devices/storage/usb_controller.py

@@ -53,6 +53,7 @@ def __init__(self, parent: QtCore.QObject, gcodes_dir: str | None) -> None:
         )
         self.banner = BannerPopup()
         self._restart_type: ResType = "always"
+        self._closing: bool = False
         self.udisks.start(self.udisks.Priority.InheritPriority)
         self.udisks.hardware_detected.connect(self.handle_new_hardware)
         self.udisks.hardware_detected.connect(self.usb_hardware_detected)
@@ -75,6 +76,8 @@ def __init__(self, parent: QtCore.QObject, gcodes_dir: str | None) -> None:
 
     def restart(self) -> None:
         """Restart usb monitoring tool"""
+        if self._closing:
+            return
         if not self.udisks.active:
             self.udisks.start(self.udisks.Priority.InheritPriority)
             return
@@ -83,11 +86,13 @@ def restart(self) -> None:
 
     def close(self) -> None:
         """Close usb monitoring tool"""
+        # A queued `finished` must not restart the thread deleteLater destroys (qFatal).
+        self._closing = True
         self.udisks.close()
         self.deleteLater()
 
     def _handle_full_restart(self) -> None:
-        if self.need_restart:
+        if self.need_restart and not self._closing:
             self.udisks.start(self.udisks.Priority.InheritPriority)
             self.need_restart = False
 

BlocksScreen/tools/configuration_manager/configurator.py

@@ -21,10 +21,17 @@
 VARIABLES_FILE: pathlib.Path = KLIPPER_CONFIG_DIR / "variables.cfg"
 PRINTER_CONFIG: pathlib.Path = KLIPPER_CONFIG_DIR / "printer.cfg"
 SV_CONFIG_MARKER = "#*# <---------------------- SAVE_CONFIG ---------------------->"
+# Copied only when missing; bs_migrate_moonraker_conf and Mainsail own it after that.
+SEED_ONLY_FILES: frozenset[str] = frozenset({"moonraker.conf"})
 
 _logger = logging.getLogger(__name__)
 
 
+def _opt_line(opt: str, val: str) -> str:
+    """Option line; continuation lines indented so a multi-line value stays one option."""
+    return f"{opt}= " + val.replace("\n", "\n    ")
+
+
 def _timestamp() -> str:
     """Return the current timestamp in ISO8601 format
 
@@ -542,12 +549,13 @@ def merge_cfg(
                         target_cfg.read_string(_tfl)
 
                     appendix = []
+                    inserts: dict[str, list[tuple[str, str]]] = {}
                     for section in src_cfg.sections():
                         if section.startswith("mcu") or section.startswith("beacon"):
                             continue
                         if not target_cfg.has_section(section):
                             appendix.append(
-                                (section, True, list(src_cfg.items(section, raw=True)))
+                                (section, list(src_cfg.items(section, raw=True)))
                             )
                         else:
                             sec_missing = [
@@ -556,33 +564,48 @@ def merge_cfg(
                                 if not target_cfg.has_option(section, o)
                             ]
                             if sec_missing:
-                                appendix.append((section, False, sec_missing))
+                                inserts[section] = sec_missing
+
+                    base = _tfl
+                    if inserts:
+                        # A repeated header breaks Klipper save_variables' parser.
+                        tlines = _tfl.splitlines(keepends=True)
+                        for i in range(len(tlines) - 1, -1, -1):
+                            m = configparser.ConfigParser.SECTCRE.match(
+                                tlines[i].strip()
+                            )
+                            if m and m.group("header") in inserts:
+                                opts = inserts.pop(m.group("header"))
+                                tlines[i] = tlines[i].rstrip("\n") + "\n"
+                                tlines[i + 1 : i + 1] = [
+                                    _opt_line(o, v) + "\n" for o, v in opts
+                                ]
+                        base = "".join(tlines)
 
                     for opt, val in src_cfg.defaults().items():
                         if not target_cfg.defaults().get(opt):
                             items = (
-                                appendix[-1][2]
+                                appendix[-1][1]
                                 if appendix and appendix[-1][0] == "DEFAULT"
                                 else None
                             )
                             if items is not None:
                                 items.append((opt, val))
                             else:
-                                appendix.append(("DEFAULT", True, [(opt, val)]))
+                                appendix.append(("DEFAULT", [(opt, val)]))
 
                     if appendix:
                         lines = []
-                        for section, is_new, opts in appendix:
-                            if is_new:
-                                lines.append(f"[{section}]")
+                        for section, opts in appendix:
+                            lines.append(f"[{section}]")
                             for opt, val in opts:
-                                lines.append(f"{opt}= {val}")
+                                lines.append(_opt_line(opt, val))
                         text = "\n".join(lines)
-                        if _tfl and not _tfl.endswith("\n"):
-                            _tfl += "\n"
-                        merged = _tfl + text + "\n"
+                        if base and not base.endswith("\n"):
+                            base += "\n"
+                        merged = base + text + "\n"
                     else:
-                        merged = _tfl
+                        merged = base
 
             merged = self._fix_beacon_serial(merged)
             if _tfl != merged:
@@ -617,6 +640,8 @@ def _cpy_cfg_files(self) -> None:
                     _logger.info("File created")
                     continue
 
+                if f in SEED_ONLY_FILES:
+                    continue
                 target = min(target_files, key=lambda p: len(p.parents))
                 if target.exists():
                     chk_src = get_file_checksum(src_file)

scripts/bs-bootstrap.sh

@@ -46,6 +46,8 @@ _deps_broken() {
     REINSTALL="--force-reinstall --no-cache-dir"
     rm -f "$SENTINEL" "$ATTEMPT" 2>/dev/null || true
 }
+# Every venv writer (also install-updater.sh, post-merge) flocks the venv dir: a probe mid-replace forces a reinstall.
+{ exec 201<"$BSENV" && flock -w 900 201; } || echo "[bs-bootstrap] venv lock not acquired (busy 900s or no venv) - continuing unlocked"
 # Repair the daemon's own dependency up front so an update can still run if the full set below fails.
 if ! _probe yaml; then
     _deps_broken PyYAML
@@ -66,41 +68,52 @@ if [ -z "$REINSTALL" ] && [ -n "$(find "$BSENV/lib" -name '*.so' -size 0 -print
 fi
 
 # 3. Full pinned requirements when the set changed (also done by post-merge).
-REQS_HASH=$(md5sum "$REQS" | cut -d' ' -f1)
-if [ ! -f "$SENTINEL" ] || [ "$(cat "$SENTINEL")" != "$REQS_HASH" ]; then
-    # shellcheck disable=SC2086
-    if _as_blocks "$BSENV/bin/pip" install --quiet $REINSTALL \
-        --only-binary :all: \
-        --no-binary sdbus,sdbus-networkmanager \
-        --upgrade-strategy=only-if-needed \
-        -r "$BS_PATH/scripts/requirements.txt"; then
-        echo "$REQS_HASH" >"$SENTINEL"
-        chown "$_BSENV_USER" "$SENTINEL" 2>/dev/null || true
-        rm -f "$ATTEMPT" 2>/dev/null || true
-    else
-        # pip resolves -r atomically: a single dep without an aarch64 wheel
-        # rejects the whole set, the sentinel never advances, and bootstrap
-        # re-thrashes this install on every boot. Bound the retries - keep
-        # trying for a few boots (covers transient/offline failures), then
-        # accept the partial venv so a deterministically-failing set stops
-        # thrashing the boot path.
-        _prev=$(cut -d' ' -f1 "$ATTEMPT" 2>/dev/null || true)
-        _cnt=$(cut -d' ' -f2 "$ATTEMPT" 2>/dev/null || true)
-        case "$_cnt" in '' | *[!0-9]*) _cnt=0 ;; esac
-        if [ "$_prev" = "$REQS_HASH" ]; then _cnt=$((_cnt + 1)); else _cnt=1; fi
-        echo "$REQS_HASH $_cnt" >"$ATTEMPT"
-        chown "$_BSENV_USER" "$ATTEMPT" 2>/dev/null || true
-        if [ "$_cnt" -ge 3 ]; then
-            echo "[bs-bootstrap] requirements failed ${_cnt}x; giving up to stop boot thrash (venv left as-is)"
+_install_reqs() {
+    REQS_HASH=$(md5sum "$REQS" | cut -d' ' -f1)
+    if [ ! -f "$SENTINEL" ] || [ "$(cat "$SENTINEL")" != "$REQS_HASH" ]; then
+        # shellcheck disable=SC2086
+        if _as_blocks "$BSENV/bin/pip" install --quiet $REINSTALL \
+            --only-binary :all: \
+            --no-binary sdbus,sdbus-networkmanager \
+            --upgrade-strategy=only-if-needed \
+            -r "$BS_PATH/scripts/requirements.txt"; then
             echo "$REQS_HASH" >"$SENTINEL"
             chown "$_BSENV_USER" "$SENTINEL" 2>/dev/null || true
+            rm -f "$ATTEMPT" 2>/dev/null || true
+        else
+            # pip resolves -r atomically: a single dep without an aarch64 wheel
+            # rejects the whole set, the sentinel never advances, and bootstrap
+            # re-thrashes this install on every boot. Bound the retries - keep
+            # trying for a few boots (covers transient/offline failures), then
+            # accept the partial venv so a deterministically-failing set stops
+            # thrashing the boot path.
+            _prev=$(cut -d' ' -f1 "$ATTEMPT" 2>/dev/null || true)
+            _cnt=$(cut -d' ' -f2 "$ATTEMPT" 2>/dev/null || true)
+            case "$_cnt" in '' | *[!0-9]*) _cnt=0 ;; esac
+            if [ "$_prev" = "$REQS_HASH" ]; then _cnt=$((_cnt + 1)); else _cnt=1; fi
+            echo "$REQS_HASH $_cnt" >"$ATTEMPT"
+            chown "$_BSENV_USER" "$ATTEMPT" 2>/dev/null || true
+            if [ "$_cnt" -ge 3 ]; then
+                echo "[bs-bootstrap] requirements failed ${_cnt}x; giving up to stop boot thrash (venv left as-is)"
+                echo "$REQS_HASH" >"$SENTINEL"
+                chown "$_BSENV_USER" "$SENTINEL" 2>/dev/null || true
+            fi
         fi
     fi
+}
+_install_reqs
+# post-merge skips pip while we hold the venv lock: install what it merged meanwhile.
+if [ "$(md5sum "$REQS" | cut -d' ' -f1)" != "$REQS_HASH" ]; then
+    echo "[bs-bootstrap] requirements changed during install - installing again"
+    REINSTALL=""
+    _install_reqs
 fi
 # pip -r never removes a dropped pin; the glob keeps this free on every start once the stale backport is gone.
 if compgen -G "$BSENV/lib/python3*/site-packages/typing-*-info" >/dev/null; then
     _as_blocks "$BSENV/bin/pip" uninstall --quiet --yes typing || true
 fi
+# Released before step 4: install-updater.sh takes the venv lock itself and would wait on ours.
+exec 201<&-
 
 # 4. Updater stack (local files only - works offline).
 # Verify each critical artifact: an old-installer box can have the service but no apt helper.

scripts/install-updater.sh

@@ -228,6 +228,8 @@ echo_ok "post-merge hook installed"
 echo_info "Installing Python requirements ..."
 # Skip pip's PyPI self-check: it stalls every install on an offline box and only prints a notice.
 export PIP_DISABLE_PIP_VERSION_CHECK=1
+# Shared with bs-bootstrap and post-merge: a concurrent pip crashes when the pip pin below replaces pip under it.
+{ exec 201<"$BSENV" && flock -w 300 201; } || echo_warn "Venv lock not acquired (busy 300s or no venv); installing unlocked"
 # Older runs of this script ran pip as root; hand those files back or every pip-as-blocks run fails.
 [ -d "$BSENV" ] && sudo find "$BSENV" ! -user "$_BSENV_USER" -exec chown -h "$_BSENV_USER" {} + || true
 # Pinned so every box runs the pip CI tested; best-effort, the venv's stock pip installs the pins too.
@@ -254,6 +256,7 @@ if _as_blocks "$BSENV/bin/pip" install --quiet uv==0.12.24; then
 else
     echo_warn "uv install FAILED; Spoolman provisioning fails until it is installed"
 fi
+exec 201<&-
 
 echo_info "Pre-generating framebuffer splash cache ..."
 _as_blocks "$BSENV/bin/python3.11" "$SCRIPT_PATH/bs-splash.py" --precompute 2>/dev/null || true

scripts/post-merge

@@ -29,8 +29,12 @@ REQS_HASH=$(md5sum "$REQS" | cut -d' ' -f1)
 SENTINEL="$BSENV/.blockscreen-reqs-hash"
 ATTEMPT="$BSENV/.blockscreen-reqs-attempt"
 REINSTALL=""
+# Shared with bs-bootstrap and install-updater.sh; released before step 3, which runs install-updater.sh.
+# Busy = skip pip, not wait: the updater kills this hook at HOOK_TIMEOUT (600s); bs-bootstrap retries next boot.
+_VENV_LOCKED=0
+{ exec 201<"$BSENV" && flock -w 60 201 && _VENV_LOCKED=1; } || echo "[post-merge] venv lock not acquired (busy 60s or no venv) - skipping pip"
 # A truncated package (interrupted pip, power cut) still imports and yields only its dunders.
-if ! _as_blocks "$BSENV/bin/python3.11" -c "import yaml as _m; assert len(dir(_m)) > 15" >/dev/null 2>&1; then
+if [ "$_VENV_LOCKED" = 1 ] && ! _as_blocks "$BSENV/bin/python3.11" -c "import yaml as _m; assert len(dir(_m)) > 15" >/dev/null 2>&1; then
     echo "[post-merge] PyYAML missing or broken - repairing before the pinned set"
     # --force-reinstall: a gutted package keeps intact dist-info, so a plain install reports it already satisfied.
     _as_blocks "$BSENV/bin/pip" install --quiet --force-reinstall --no-cache-dir \
@@ -39,12 +43,12 @@ if ! _as_blocks "$BSENV/bin/python3.11" -c "import yaml as _m; assert len(dir(_m
     rm -f "$SENTINEL" "$ATTEMPT" 2>/dev/null || true
 fi
 # A zero-length .so is never legitimate: it means the filesystem lost writes mid-flush.
-if [ -z "$REINSTALL" ] && [ -n "$(find "$BSENV/lib" -name '*.so' -size 0 -print -quit 2>/dev/null)" ]; then
+if [ "$_VENV_LOCKED" = 1 ] && [ -z "$REINSTALL" ] && [ -n "$(find "$BSENV/lib" -name '*.so' -size 0 -print -quit 2>/dev/null)" ]; then
     echo "[post-merge] truncated extension module - reinstalling venv"
     REINSTALL="--force-reinstall --no-cache-dir"
     rm -f "$SENTINEL" "$ATTEMPT" 2>/dev/null || true
 fi
-if [ ! -f "$SENTINEL" ] || [ "$(cat "$SENTINEL")" != "$REQS_HASH" ]; then
+if [ "$_VENV_LOCKED" = 1 ] && { [ ! -f "$SENTINEL" ] || [ "$(cat "$SENTINEL")" != "$REQS_HASH" ]; }; then
     echo "[post-merge] requirements changed - pre-installing deps (this may take a few minutes) ..."
     # shellcheck disable=SC2086
     if _as_blocks "$BSENV/bin/pip" install --quiet $REINSTALL \
@@ -71,6 +75,7 @@ if [ ! -f "$SENTINEL" ] || [ "$(cat "$SENTINEL")" != "$REQS_HASH" ]; then
         fi
     fi
 fi
+exec 201<&-
 
 # 2. Sync all service files so start.sh finds them already in place.
 bs_sync_service_files "$SCRIPT_PATH" post-merge

tests/storage/test_usb_controller_unit.py

@@ -0,0 +1,91 @@
+"""USBManager.close must not let the queued `finished` restart the monitor."""
+
+import importlib.util
+import pathlib
+import sys
+import types
+from unittest.mock import MagicMock
+
+import pytest
+from PyQt6 import QtCore
+
+_STORAGE = pathlib.Path(__file__).resolve().parents[2] / "BlocksScreen/devices/storage"
+_PKG = "_usb_controller_under_test"
+
+
+class _FakeUdisks(QtCore.QObject):
+    """UDisksDBusAsync signal surface, no D-Bus or thread."""
+
+    Priority = QtCore.QThread.Priority
+    hardware_detected = QtCore.pyqtSignal(str)
+    hardware_removed = QtCore.pyqtSignal(str)
+    device_added = QtCore.pyqtSignal(str, dict)
+    device_removed = QtCore.pyqtSignal(str, str)
+    device_mounted = QtCore.pyqtSignal(str, str)
+    device_unmounted = QtCore.pyqtSignal(str)
+    started = QtCore.pyqtSignal()
+    finished = QtCore.pyqtSignal()
+
+    def __init__(self, parent=None, gcodes_dir=None):
+        super().__init__(parent)
+        self.active = False
+        self.starts = 0
+
+    def start(self, _priority=None):
+        self.starts += 1
+        self.active = True
+
+    def close(self):
+        self.active = False
+
+
+@pytest.fixture
+def usb_controller(monkeypatch):
+    """Load usb_controller by path with udisks2 and BannerPopup stubbed."""
+    pkg = types.ModuleType(_PKG)
+    pkg.__path__ = [str(_STORAGE)]
+    monkeypatch.setitem(sys.modules, _PKG, pkg)
+    monkeypatch.setitem(
+        sys.modules,
+        f"{_PKG}.udisks2",
+        types.SimpleNamespace(UDisksDBusAsync=_FakeUdisks),
+    )
+    monkeypatch.setitem(
+        sys.modules,
+        "lib.panels.widgets.Common.bannerPopup",
+        types.SimpleNamespace(BannerPopup=MagicMock),
+    )
+    spec = importlib.util.spec_from_file_location(
+        f"{_PKG}.usb_controller", _STORAGE / "usb_controller.py"
+    )
+    assert spec is not None and spec.loader is not None
+    module = importlib.util.module_from_spec(spec)
+    spec.loader.exec_module(module)
+    return module
+
+
+def test_finished_after_close_does_not_restart(qapp, usb_controller):
+    parent = QtCore.QObject()
+    mgr = usb_controller.USBManager(parent, gcodes_dir=None)
+    assert mgr.udisks.starts == 1
+    mgr.close()
+    mgr.udisks.finished.emit()
+    assert mgr.udisks.starts == 1
+
+
+def test_finished_without_close_restarts(qapp, usb_controller):
+    parent = QtCore.QObject()
+    mgr = usb_controller.USBManager(parent, gcodes_dir=None)
+    mgr.udisks.close()
+    mgr.udisks.finished.emit()
+    assert mgr.udisks.starts == 2
+
+
+def test_pending_full_restart_dropped_on_close(qapp, usb_controller):
+    parent = QtCore.QObject()
+    mgr = usb_controller.USBManager(parent, gcodes_dir=None)
+    mgr.restart()
+    assert mgr.need_restart
+    mgr.close()
+    mgr.udisks.finished.emit()
+    assert mgr.udisks.starts == 1