Ver código fonte

fix(backup): resolve K-profile cali_idx live instead of reusing the backup's (#2656)

Restoring K-profiles addressed extrusion_cali_set at the cali_idx recorded in
the backup. If that slot no longer existed on the printer the write was
silently dropped and the restore still reported the profile restored.

Not an edge case: Bambuddy's own K-profile editor is what re-keys the slot.
On a single-nozzle printer an edit is delete-then-add, so any edit between
backup and restore reproduces it.

Found testing on an X1E. Backup held cali_idx 8151; an edit through the UI
re-keyed the profile to 4606; the restore published cali_idx 8151, the printer
ignored it, and the tally read "1 restored" while the k-value stayed put.
Resending the identical payload with cali_idx 4606 applied, isolating the
stale index as the cause.

Fix mirrors the natural-key matching spools and archives already use, which
the module docstring already promised but scoped to spool.id and
print_archives.id. Before writing, read the live profiles for the nozzle and
match on filament_id + setting_id, falling back to filament_id + name, then to
the sole candidate for that filament. Send that profile's current cali_idx;
where nothing matches send -1 so the printer adds a new profile instead of
addressing a dead slot, and say so in the tally. A failed read degrades to
adding rather than aborting.

Also corrects the tally note. The printer does acknowledge extrusion_cali_set
-- it answers with a result/reason pair -- so "published without
acknowledgement" was false. It reports "fail" on writes that land, though, so
the note now says the acknowledgement is unreliable rather than absent.
Consuming result is left to a follow-up.

Re-verified on the same X1E: perturbed to k=0.061, restored from the commit
carrying the stale slot, payload went out with cali_idx 4606 and the printer
read back 0.027.
jmoore-skild 1 mês atrás
pai
commit
cbe412607f

+ 94 - 14
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
   on natural keys instead, inserted without an explicit id, and an
   ``old_id -> new_id`` map is threaded through so foreign keys in dependent
   ``old_id -> new_id`` map is threaded through so foreign keys in dependent
   tables (spool usage history) still line up.
   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.**
 * **Categories are applied archives -> spools -> settings -> kprofiles.**
   Archives first because spool usage history references ``archive_id``;
   Archives first because spool usage history references ``archive_id``;
   K-profiles last because they leave the database and talk to hardware.
   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
         # the profile occupying a slot, so writing is always an overwrite on the
         # printer side.
         # printer side.
         tally.note("K-profiles always overwrite the matching slot on the printer")
         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()):
         for serial, entries in sorted(by_serial.items()):
             profile_total = sum(len(c.get("profiles") or []) for _, c in entries)
             profile_total = sum(len(c.get("profiles") or []) for _, c in entries)
@@ -890,21 +896,48 @@ class GitHubRestoreService:
                 if nozzle not in _KNOWN_NOZZLES:
                 if nozzle not in _KNOWN_NOZZLES:
                     tally.note(f"Unexpected nozzle diameter {nozzle} for {serial} — sent as-is")
                     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:
                 if not profile_dicts:
                     continue
                     continue
+                if unmatched:
+                    tally.note(
+                        f"{unmatched} profile(s) for {nozzle} had no counterpart on {printer.name} "
+                        "— added as new profiles"
+                    )
 
 
                 try:
                 try:
                     sent = client.set_kprofiles_batch(profile_dicts, nozzle)
                     sent = client.set_kprofiles_batch(profile_dicts, nozzle)
@@ -918,6 +951,53 @@ class GitHubRestoreService:
                     tally.failed += len(profile_dicts)
                     tally.failed += len(profile_dicts)
                     tally.note(f"Failed to send {nozzle} profiles to {printer.name} ({serial})")
                     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
 # Singleton instance
 github_restore_service = GitHubRestoreService()
 github_restore_service = GitHubRestoreService()

+ 141 - 5
backend/tests/unit/test_github_restore.py

@@ -7,6 +7,7 @@ K-profile paths that depend on live printers.
 """
 """
 
 
 from datetime import datetime
 from datetime import datetime
+from types import SimpleNamespace
 from unittest.mock import AsyncMock, MagicMock, patch
 from unittest.mock import AsyncMock, MagicMock, patch
 
 
 import pytest
 import pytest
@@ -656,6 +657,18 @@ class TestRestoreArchives:
 
 
 
 
 class TestRestoreKprofiles:
 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"):
     def _payload(self, serial="00M09A123456789", nozzle="0.4"):
         return {
         return {
             f"kprofiles/{serial}/{nozzle}.json": {
             f"kprofiles/{serial}/{nozzle}.json": {
@@ -697,20 +710,143 @@ class TestRestoreKprofiles:
         assert manager.get_client.call_args.args == (printer.id,)
         assert manager.get_client.call_args.args == (printer.id,)
 
 
     @pytest.mark.asyncio
     @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")
         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()
         tally = _CategoryTally()
 
 
         with patch("backend.app.services.github_restore.printer_manager") as manager:
         with patch("backend.app.services.github_restore.printer_manager") as manager:
             manager.get_client = MagicMock(return_value=client)
             manager.get_client = MagicMock(return_value=client)
             await _service()._restore_kprofiles(db_session, self._payload(), tally)
             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)
         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
     @pytest.mark.asyncio
     async def test_unknown_serial_is_skipped_with_reason(self, db_session):
     async def test_unknown_serial_is_skipped_with_reason(self, db_session):
         tally = _CategoryTally()
         tally = _CategoryTally()