diff --git a/backend/app/services/github_restore.py b/backend/app/services/github_restore.py index 13b0dd405..2c33f0f84 100644 --- a/backend/app/services/github_restore.py +++ b/backend/app/services/github_restore.py @@ -14,6 +14,12 @@ Design notes worth knowing before editing: on natural keys instead, inserted without an explicit id, and an ``old_id -> new_id`` map is threaded through so foreign keys in dependent tables (spool usage history) still line up. + + The printer-side ``cali_idx`` behaves the same way and gets the same + treatment. Editing a K-profile in Bambuddy is a delete-then-add on a + single-nozzle printer, which re-keys it, and ``extrusion_cali_set`` aimed at a + slot that no longer exists is silently dropped — so the live index is read + back and matched before writing, never taken from the backup. * **Categories are applied archives -> spools -> settings -> kprofiles.** Archives first because spool usage history references ``archive_id``; K-profiles last because they leave the database and talk to hardware. @@ -866,7 +872,7 @@ class GitHubRestoreService: # the profile occupying a slot, so writing is always an overwrite on the # printer side. tally.note("K-profiles always overwrite the matching slot on the printer") - tally.note("Profiles are published over MQTT without acknowledgement — verify on the printer") + tally.note("The printer's acknowledgement is not reliable — verify the profiles on the printer") for serial, entries in sorted(by_serial.items()): profile_total = sum(len(c.get("profiles") or []) for _, c in entries) @@ -890,21 +896,48 @@ class GitHubRestoreService: if nozzle not in _KNOWN_NOZZLES: tally.note(f"Unexpected nozzle diameter {nozzle} for {serial} — sent as-is") - profile_dicts = [ - { - "filament_id": p.get("filament_id", ""), - "name": p.get("name", ""), - "k_value": p.get("k_value", "0.020000"), - "nozzle_id": p.get("nozzle_id"), - "extruder_id": p.get("extruder_id", 0), - "setting_id": p.get("setting_id"), - "slot_id": p.get("slot_id", 0), - } - for p in profiles - if isinstance(p, dict) - ] + # The backup's slot_id is a cali_idx, and cali_idx is as + # unstable as the autoincrement ids we already refuse to reuse + # for spools and archives: editing a profile in Bambuddy is a + # delete-then-add on a single-nozzle printer, which re-keys it. + # Addressing extrusion_cali_set at a slot that no longer exists + # is a silent no-op — the printer drops it and we would still + # report the profile restored. So resolve the live index first. + current = await self._current_kprofile_index(client, nozzle, serial) + + profile_dicts = [] + unmatched = 0 + for p in profiles: + if not isinstance(p, dict): + continue + match = self._match_kprofile(p, current) + if match is None: + unmatched += 1 + profile_dicts.append( + { + "filament_id": p.get("filament_id", ""), + "name": p.get("name", ""), + "k_value": p.get("k_value", "0.020000"), + "nozzle_id": p.get("nozzle_id"), + "extruder_id": p.get("extruder_id", 0), + # Prefer the live setting_id when we matched: it is + # what the printer currently associates with the slot. + "setting_id": (match.setting_id if match else None) or p.get("setting_id"), + # cali_idx -1 tells the printer to add a new profile + # rather than address a slot that isn't there. + "cali_idx": match.slot_id if match else -1, + # Only consulted for the generated-setting_id + # fallback; cali_idx above takes precedence. + "slot_id": 0, + } + ) if not profile_dicts: continue + if unmatched: + tally.note( + f"{unmatched} profile(s) for {nozzle} had no counterpart on {printer.name} " + "— added as new profiles" + ) try: sent = client.set_kprofiles_batch(profile_dicts, nozzle) @@ -918,6 +951,53 @@ class GitHubRestoreService: tally.failed += len(profile_dicts) tally.note(f"Failed to send {nozzle} profiles to {printer.name} ({serial})") + @staticmethod + async def _current_kprofile_index(client, nozzle: str, serial: str) -> list: + """Read the printer's live profiles for one nozzle. + + Best-effort: a read failure degrades to "nothing matched", which makes + every profile an add rather than aborting the restore. + """ + try: + return list(await client.get_kprofiles(nozzle_diameter=nozzle) or []) + except Exception as e: + logger.warning("Could not read live K-profiles for %s nozzle %s: %s", serial, nozzle, e) + return [] + + @staticmethod + def _match_kprofile(entry: dict, current: list): + """Find the live profile a backed-up entry corresponds to. + + ``setting_id`` is the filament preset the profile was calibrated for and + is the strongest signal; a delete-then-add edit regenerates it, so fall + back to the display name, which Bambuddy's own editor preserves. + Both are scoped by ``filament_id`` — the same preset on a different + filament is a different profile. + """ + filament_id = entry.get("filament_id") + if not filament_id: + return None + + candidates = [c for c in current if c.filament_id == filament_id] + if not candidates: + return None + + setting_id = entry.get("setting_id") + if setting_id: + for c in candidates: + if c.setting_id == setting_id: + return c + + name = entry.get("name") + if name: + for c in candidates: + if c.name == name: + return c + + # Exactly one profile for this filament and no better discriminator: + # treat it as the same profile rather than duplicating it. + return candidates[0] if len(candidates) == 1 else None + # Singleton instance github_restore_service = GitHubRestoreService() diff --git a/backend/tests/unit/test_github_restore.py b/backend/tests/unit/test_github_restore.py index 02ce3e0d4..39878c59e 100644 --- a/backend/tests/unit/test_github_restore.py +++ b/backend/tests/unit/test_github_restore.py @@ -7,6 +7,7 @@ K-profile paths that depend on live printers. """ from datetime import datetime +from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -656,6 +657,18 @@ class TestRestoreArchives: class TestRestoreKprofiles: + @staticmethod + def _live(slot_id, filament_id="GFA00", name="Bambu PLA", setting_id="PFUS123"): + """One profile as the printer currently reports it.""" + return SimpleNamespace(slot_id=slot_id, filament_id=filament_id, name=name, setting_id=setting_id) + + def _client(self, live=None, sent=True): + client = MagicMock() + client.state.connected = True + client.set_kprofiles_batch = MagicMock(return_value=sent) + client.get_kprofiles = AsyncMock(return_value=list(live or [])) + return client + def _payload(self, serial="00M09A123456789", nozzle="0.4"): return { f"kprofiles/{serial}/{nozzle}.json": { @@ -697,20 +710,143 @@ class TestRestoreKprofiles: assert manager.get_client.call_args.args == (printer.id,) @pytest.mark.asyncio - async def test_always_warns_that_mqtt_is_unacknowledged(self, db_session, printer_factory): + async def test_always_warns_to_verify_on_the_printer(self, db_session, printer_factory): await printer_factory(serial_number="00M09A123456789") - client = MagicMock() - client.state.connected = True - client.set_kprofiles_batch = MagicMock(return_value=True) + client = self._client() tally = _CategoryTally() with patch("backend.app.services.github_restore.printer_manager") as manager: manager.get_client = MagicMock(return_value=client) await _service()._restore_kprofiles(db_session, self._payload(), tally) - assert any("without acknowledgement" in note for note in tally.notes) + # The printer does answer extrusion_cali_set, but it reports "fail" on + # writes that land, so the note must not promise either way. + assert any("verify the profiles on the printer" in note for note in tally.notes) + assert not any("without acknowledgement" in note for note in tally.notes) assert any("always overwrite" in note for note in tally.notes) + # --- cali_idx is resolved live, never taken from the backup ------------- + # + # Regression cover for the silent no-op found testing on an X1E: the backup + # stored cali_idx 8151, a Bambuddy edit re-keyed the profile to 4606, and + # the restore aimed extrusion_cali_set at 8151. The printer dropped it and + # the tally still said "1 restored". + + @pytest.mark.asyncio + async def test_uses_the_live_cali_idx_not_the_backed_up_slot(self, db_session, printer_factory): + await printer_factory(serial_number="00M09A123456789") + payload = self._payload() + payload["kprofiles/00M09A123456789/0.4.json"]["profiles"][0]["slot_id"] = 8151 + client = self._client(live=[self._live(slot_id=4606)]) + tally = _CategoryTally() + + with patch("backend.app.services.github_restore.printer_manager") as manager: + manager.get_client = MagicMock(return_value=client) + await _service()._restore_kprofiles(db_session, payload, tally) + + client.get_kprofiles.assert_awaited_once_with(nozzle_diameter="0.4") + profiles, _ = client.set_kprofiles_batch.call_args.args + assert profiles[0]["cali_idx"] == 4606, "must address the slot that exists now" + assert profiles[0]["cali_idx"] != 8151, "must not reuse the backup's cali_idx" + assert tally.restored == 1 + + @pytest.mark.asyncio + async def test_matches_on_name_when_setting_id_was_regenerated(self, db_session, printer_factory): + # A delete-then-add edit mints a fresh setting_id, so the name carries + # the match instead. + await printer_factory(serial_number="00M09A123456789") + client = self._client(live=[self._live(slot_id=4606, setting_id="PF9999999999")]) + tally = _CategoryTally() + + with patch("backend.app.services.github_restore.printer_manager") as manager: + manager.get_client = MagicMock(return_value=client) + await _service()._restore_kprofiles(db_session, self._payload(), tally) + + profiles, _ = client.set_kprofiles_batch.call_args.args + assert profiles[0]["cali_idx"] == 4606 + # The live setting_id wins: it is what the printer associates with the slot. + assert profiles[0]["setting_id"] == "PF9999999999" + + @pytest.mark.asyncio + async def test_unmatched_profile_is_added_rather_than_aimed_at_a_dead_slot(self, db_session, printer_factory): + await printer_factory(serial_number="00M09A123456789") + client = self._client(live=[]) # printer has nothing for this nozzle + tally = _CategoryTally() + + with patch("backend.app.services.github_restore.printer_manager") as manager: + manager.get_client = MagicMock(return_value=client) + await _service()._restore_kprofiles(db_session, self._payload(), tally) + + profiles, _ = client.set_kprofiles_batch.call_args.args + assert profiles[0]["cali_idx"] == -1, "-1 tells the printer to add a new profile" + assert profiles[0]["setting_id"] == "PFUS123", "falls back to the backed-up preset" + assert any("added as new profiles" in note for note in tally.notes) + + @pytest.mark.asyncio + async def test_different_filament_is_not_treated_as_a_match(self, db_session, printer_factory): + # Same slot, different filament — matching on slot alone would clobber + # an unrelated profile. + await printer_factory(serial_number="00M09A123456789") + client = self._client(live=[self._live(slot_id=4606, filament_id="GFB99", name="Bambu PLA")]) + tally = _CategoryTally() + + with patch("backend.app.services.github_restore.printer_manager") as manager: + manager.get_client = MagicMock(return_value=client) + await _service()._restore_kprofiles(db_session, self._payload(), tally) + + profiles, _ = client.set_kprofiles_batch.call_args.args + assert profiles[0]["cali_idx"] == -1 + + @pytest.mark.asyncio + async def test_unreadable_live_index_degrades_to_adding(self, db_session, printer_factory): + # A failed read must not abort the restore. + await printer_factory(serial_number="00M09A123456789") + client = self._client() + client.get_kprofiles = AsyncMock(side_effect=RuntimeError("mqtt timeout")) + tally = _CategoryTally() + + with patch("backend.app.services.github_restore.printer_manager") as manager: + manager.get_client = MagicMock(return_value=client) + await _service()._restore_kprofiles(db_session, self._payload(), tally) + + profiles, _ = client.set_kprofiles_batch.call_args.args + assert profiles[0]["cali_idx"] == -1 + assert tally.restored == 1 + + @pytest.mark.asyncio + async def test_sole_profile_for_a_filament_matches_without_setting_id_or_name(self, db_session, printer_factory): + await printer_factory(serial_number="00M09A123456789") + payload = self._payload() + entry = payload["kprofiles/00M09A123456789/0.4.json"]["profiles"][0] + entry["setting_id"] = None + entry["name"] = "" + client = self._client(live=[self._live(slot_id=4606, setting_id="PFOTHER", name="Renamed")]) + tally = _CategoryTally() + + with patch("backend.app.services.github_restore.printer_manager") as manager: + manager.get_client = MagicMock(return_value=client) + await _service()._restore_kprofiles(db_session, payload, tally) + + profiles, _ = client.set_kprofiles_batch.call_args.args + assert profiles[0]["cali_idx"] == 4606 + + @pytest.mark.asyncio + async def test_ambiguous_filament_without_discriminator_is_added_not_guessed(self, db_session, printer_factory): + await printer_factory(serial_number="00M09A123456789") + payload = self._payload() + entry = payload["kprofiles/00M09A123456789/0.4.json"]["profiles"][0] + entry["setting_id"] = None + entry["name"] = "" + client = self._client(live=[self._live(slot_id=1, setting_id="A"), self._live(slot_id=2, setting_id="B")]) + tally = _CategoryTally() + + with patch("backend.app.services.github_restore.printer_manager") as manager: + manager.get_client = MagicMock(return_value=client) + await _service()._restore_kprofiles(db_session, payload, tally) + + profiles, _ = client.set_kprofiles_batch.call_args.args + assert profiles[0]["cali_idx"] == -1, "two candidates and nothing to tell them apart" + @pytest.mark.asyncio async def test_unknown_serial_is_skipped_with_reason(self, db_session): tally = _CategoryTally()