diff --git a/backend/app/services/github_restore.py b/backend/app/services/github_restore.py index ed256d216..8087ce430 100644 --- a/backend/app/services/github_restore.py +++ b/backend/app/services/github_restore.py @@ -70,6 +70,20 @@ _SENSITIVE_SETTING_KEYS = {"bambu_cloud_token", "auth_secret_key"} # skipped even if it isn't in the explicit denylist above. _SECRET_KEY_HINTS = ("token", "secret", "password", "access_code", "api_key", "passphrase") +# Settings the MQTT relay reads only when it is (re)configured, so restoring the +# rows is not enough on its own. Mirrors the set the settings PUT handler +# watches. mqtt_password is in here for the configure() payload's sake — the +# credential blocklist means a restore never writes it. +_MQTT_SETTING_KEYS = { + "mqtt_enabled", + "mqtt_broker", + "mqtt_port", + "mqtt_username", + "mqtt_password", + "mqtt_topic_prefix", + "mqtt_use_tls", +} + # Keys that decide *who can reach the instance* rather than how it behaves. The # backup collector writes them like any other Settings row, so a backup taken # before auth was turned on carries auth_enabled=false — and a restore reaches @@ -407,9 +421,17 @@ class GitHubRestoreService: if error: raise RuntimeError(error) - results = await self._apply(db, payload, categories, overwrite_existing) + settings_keys_written: set[str] = set() + results = await self._apply(db, payload, categories, overwrite_existing, settings_keys_written) await db.commit() + # After the commit: this reconnects the relay, which is not + # something to do on values that could still roll back. + settings_tally = results.get(RestoreCategory.SETTINGS.value) + if settings_tally is not None: + self._progress = "Reconnecting the MQTT relay..." + await self._reconfigure_mqtt_relay(db, settings_keys_written, settings_tally) + total_restored = sum(tally.restored for tally in results.values()) any_failed = any(tally.failed for tally in results.values()) @@ -484,8 +506,14 @@ class GitHubRestoreService: payload: dict, categories: list[RestoreCategory], overwrite: bool, + settings_keys_written: set[str] | None = None, ) -> dict[str, _CategoryTally]: - """Apply categories in dependency order and return per-category tallies.""" + """Apply categories in dependency order and return per-category tallies. + + ``settings_keys_written``, if given, collects the setting keys actually + written, for the caller's post-commit side effects (see + ``_reconfigure_mqtt_relay``). + """ results: dict[str, _CategoryTally] = {} archive_id_map: dict[int, int] = {} @@ -512,7 +540,9 @@ class GitHubRestoreService: if RestoreCategory.SETTINGS in categories: self._progress = "Restoring app settings..." tally = _CategoryTally() - await self._restore_settings(db, payload.get(SETTINGS_PATH), overwrite, tally) + await self._restore_settings( + db, payload.get(SETTINGS_PATH), overwrite, tally, keys_written=settings_keys_written + ) results[RestoreCategory.SETTINGS.value] = tally # Last, because it leaves the database and publishes over MQTT. @@ -879,7 +909,14 @@ class GitHubRestoreService: "spool list, so there is nothing to attach them to." ) - async def _restore_settings(self, db: AsyncSession, payload, overwrite: bool, tally: _CategoryTally) -> None: + async def _restore_settings( + self, + db: AsyncSession, + payload, + overwrite: bool, + tally: _CategoryTally, + keys_written: set[str] | None = None, + ) -> None: values = payload.get("settings") if isinstance(payload, dict) else None if not isinstance(values, dict): tally.note("No settings data in this backup") @@ -911,10 +948,14 @@ class GitHubRestoreService: continue existing.value = str(value) tally.restored += 1 + if keys_written is not None: + keys_written.add(key) continue db.add(Settings(key=key, value=str(value))) tally.restored += 1 + if keys_written is not None: + keys_written.add(key) if blocked: tally.note(f"{blocked} credential-like key(s) skipped — re-enter secrets manually") @@ -924,6 +965,51 @@ class GitHubRestoreService: "Authentication so the lockout checks still run" ) + async def _reconfigure_mqtt_relay(self, db: AsyncSession, keys_written: set[str], tally: _CategoryTally) -> None: + """Push restored mqtt_* settings into the live relay. + + The relay reads its broker config once, at configure() time — the + settings PUT handler reconfigures it for exactly this reason + (api/routes/settings.py). Writing the rows alone left the relay on the + pre-restore broker until the next backend restart while the UI showed + the restored values, which is the one way a restore could look applied + and not be. + + Called after the commit, never before: configure() tears the connection + down and rebuilds it, so it must not run against values a later failure + could roll back. Only mqtt_password can't come back this way (the + credential blocklist skips it) — the row already in the database is + reused, so an unchanged broker keeps working. + """ + if not _MQTT_SETTING_KEYS & keys_written: + return + + try: + from backend.app.services.mqtt_relay import mqtt_relay + + rows = await db.execute(select(Settings).where(Settings.key.in_(_MQTT_SETTING_KEYS))) + stored = {s.key: s.value for s in rows.scalars().all()} + + # Same shape and defaults the settings PUT handler builds. + await mqtt_relay.configure( + { + "mqtt_enabled": (stored.get("mqtt_enabled") or "false") == "true", + "mqtt_broker": stored.get("mqtt_broker") or "", + "mqtt_port": int(stored.get("mqtt_port") or "1883"), + "mqtt_username": stored.get("mqtt_username") or "", + "mqtt_password": stored.get("mqtt_password") or "", + "mqtt_topic_prefix": stored.get("mqtt_topic_prefix") or "bambuddy", + "mqtt_use_tls": (stored.get("mqtt_use_tls") or "false") == "true", + } + ) + except Exception: + # Same call is best-effort in the settings PUT handler: the rows are + # committed either way, and a broker that refuses the new config + # must not turn a successful restore into a failed one. Noted rather + # than swallowed silently, so the user knows to restart. + logger.warning("Could not reconfigure the MQTT relay after a settings restore", exc_info=True) + tally.note("MQTT settings restored, but the relay could not be reconnected — restart Bambuddy") + async def _restore_kprofiles(self, db: AsyncSession, payload: dict, tally: _CategoryTally) -> None: by_serial: dict[str, list[tuple[str, dict]]] = {} for path, content in payload.items(): diff --git a/backend/tests/unit/test_github_restore.py b/backend/tests/unit/test_github_restore.py index 12103c0f6..61068211f 100644 --- a/backend/tests/unit/test_github_restore.py +++ b/backend/tests/unit/test_github_restore.py @@ -1090,6 +1090,98 @@ class TestMutex: assert "restore is currently running" in result["message"] +class TestMqttRelayReconfigure: + """Restoring mqtt_* rows has to reach the live relay, not just the table.""" + + @pytest.mark.asyncio + async def test_reconfigures_from_the_committed_rows(self, db_session): + db_session.add(Settings(key="mqtt_enabled", value="true")) + db_session.add(Settings(key="mqtt_broker", value="restored.local")) + db_session.add(Settings(key="mqtt_port", value="8883")) + db_session.add(Settings(key="mqtt_use_tls", value="true")) + # Never restorable (credential blocklist), so it comes from the row that + # was already there. + db_session.add(Settings(key="mqtt_password", value="kept")) + await db_session.commit() + tally = _CategoryTally() + relay = MagicMock() + relay.configure = AsyncMock(return_value=True) + + with patch("backend.app.services.mqtt_relay.mqtt_relay", relay): + await _service()._reconfigure_mqtt_relay(db_session, {"mqtt_broker"}, tally) + + relay.configure.assert_awaited_once() + sent = relay.configure.await_args.args[0] + assert sent["mqtt_enabled"] is True + assert sent["mqtt_broker"] == "restored.local" + assert sent["mqtt_port"] == 8883 + assert sent["mqtt_use_tls"] is True + assert sent["mqtt_password"] == "kept" + assert sent["mqtt_topic_prefix"] == "bambuddy" + assert tally.notes == [] + + @pytest.mark.asyncio + async def test_no_reconnect_when_no_mqtt_key_was_written(self, db_session): + """configure() tears the connection down, so don't call it for a theme change.""" + tally = _CategoryTally() + relay = MagicMock() + relay.configure = AsyncMock() + + with patch("backend.app.services.mqtt_relay.mqtt_relay", relay): + await _service()._reconfigure_mqtt_relay(db_session, {"currency", "theme"}, tally) + + relay.configure.assert_not_awaited() + + @pytest.mark.asyncio + async def test_broker_failure_is_noted_not_fatal(self, db_session): + tally = _CategoryTally() + relay = MagicMock() + relay.configure = AsyncMock(side_effect=OSError("no route to broker")) + + with patch("backend.app.services.mqtt_relay.mqtt_relay", relay): + await _service()._reconfigure_mqtt_relay(db_session, {"mqtt_enabled"}, tally) + + assert any("restart Bambuddy" in note for note in tally.notes) + + @pytest.mark.asyncio + async def test_restore_settings_reports_the_keys_it_wrote(self, db_session): + db_session.add(Settings(key="mqtt_broker", value="old.local")) + await db_session.commit() + written: set[str] = set() + payload = { + "settings": { + "mqtt_broker": "new.local", + "currency": "EUR", + "mqtt_password": "leaked", + "auth_enabled": "false", + } + } + + await _service()._restore_settings( + db_session, payload, overwrite=True, tally=_CategoryTally(), keys_written=written + ) + + # Skipped keys are not "written", or a blocked mqtt_password would + # trigger a pointless reconnect. + assert written == {"mqtt_broker", "currency"} + + @pytest.mark.asyncio + async def test_keys_skipped_for_overwrite_off_are_not_reported(self, db_session): + db_session.add(Settings(key="mqtt_broker", value="old.local")) + await db_session.commit() + written: set[str] = set() + + await _service()._restore_settings( + db_session, + {"settings": {"mqtt_broker": "new.local"}}, + overwrite=False, + tally=_CategoryTally(), + keys_written=written, + ) + + assert written == set() + + class TestApplyOrdering: """_apply must not hold SQLite's write transaction across the MQTT phase."""