Преглед изворни кода

fix(backup): stop two backed-up K-profiles claiming one live slot (#2656)

`_match_kprofile` ends in a single-candidate fallback, and the per-nozzle
loop called it once per entry with no record of which live profiles were
already taken. Two backup entries sharing a `filament_id` and matching on
neither `setting_id` nor `name` both resolved to the same live profile, both
got the same `cali_idx`, and both went into the batch — so the second
overwrote the first on the printer while the tally counted two restored.

Reachable in the ordinary way: the user deletes one of a pair after the
backup, and the delete-then-add re-key this code already reasons about is
exactly what strips the `setting_id` match.

Fix: thread a `claimed` set of slot ids through the loop; a live profile can
only stand in for one entry. A displaced entry falls through to
`cali_idx: -1` — add-as-new is the safe outcome — and folds into the
existing `kprofilesUnmatched` note rather than earning a new code.

The single-candidate fallback is still judged against every candidate rather
than the unclaimed ones. Two live profiles for one filament are ambiguous
whether or not another entry has taken one, and narrowing to "available"
would turn a guess the code deliberately refuses into a match.

Tests: 2 regression (the displaced entry is added rather than aliased, and
keeps its own setting_id) + 2 controls (two genuine matches keep their own
slots; a claimed slot does not make an ambiguous pair matchable). Both
regressions confirmed failing against the pre-fix service.
jmoore-skild пре 1 месец
родитељ
комит
cb6a4e6d88
2 измењених фајлова са 127 додато и 7 уклоњено
  1. 27 7
      backend/app/services/github_restore.py
  2. 100 0
      backend/tests/unit/test_github_restore.py

+ 27 - 7
backend/app/services/github_restore.py

@@ -1486,12 +1486,19 @@ class GitHubRestoreService:
 
 
                 profile_dicts = []
                 profile_dicts = []
                 unmatched = 0
                 unmatched = 0
+                # A live profile can only stand in for one backed-up entry. Two
+                # entries resolving to the same cali_idx both go into the batch,
+                # the second overwrites the first on the printer, and the tally
+                # counts two restored where one landed.
+                claimed: set[int] = set()
                 for p in profiles:
                 for p in profiles:
                     if not isinstance(p, dict):
                     if not isinstance(p, dict):
                         continue
                         continue
-                    match = self._match_kprofile(p, current)
+                    match = self._match_kprofile(p, current, claimed)
                     if match is None:
                     if match is None:
                         unmatched += 1
                         unmatched += 1
+                    else:
+                        claimed.add(match.slot_id)
                     profile_dicts.append(
                     profile_dicts.append(
                         {
                         {
                             "filament_id": p.get("filament_id", ""),
                             "filament_id": p.get("filament_id", ""),
@@ -1554,7 +1561,7 @@ class GitHubRestoreService:
             return []
             return []
 
 
     @staticmethod
     @staticmethod
-    def _match_kprofile(entry: dict, current: list):
+    def _match_kprofile(entry: dict, current: list, claimed: set[int]):
         """Find the live profile a backed-up entry corresponds to.
         """Find the live profile a backed-up entry corresponds to.
 
 
         ``setting_id`` is the filament preset the profile was calibrated for and
         ``setting_id`` is the filament preset the profile was calibrated for and
@@ -1562,30 +1569,43 @@ class GitHubRestoreService:
         back to the display name, which Bambuddy's own editor preserves.
         back to the display name, which Bambuddy's own editor preserves.
         Both are scoped by ``filament_id`` — the same preset on a different
         Both are scoped by ``filament_id`` — the same preset on a different
         filament is a different profile.
         filament is a different profile.
+
+        ``claimed`` holds the slot ids already taken by earlier entries in this
+        nozzle's loop, and no live profile may be claimed twice. Without it, two
+        backed-up entries sharing a ``filament_id`` and matching on neither
+        ``setting_id`` nor ``name`` both fell through to the single-candidate
+        arm and both took the same slot — reachable whenever the user has since
+        deleted one of a pair, because the delete-then-add re-key is what strips
+        the ``setting_id`` match. Returning None for the displaced entry means
+        ``cali_idx: -1``, i.e. add-as-new, which is the safe outcome.
         """
         """
         filament_id = entry.get("filament_id")
         filament_id = entry.get("filament_id")
         if not filament_id:
         if not filament_id:
             return None
             return None
 
 
         candidates = [c for c in current if c.filament_id == filament_id]
         candidates = [c for c in current if c.filament_id == filament_id]
-        if not candidates:
+        available = [c for c in candidates if c.slot_id not in claimed]
+        if not available:
             return None
             return None
 
 
         setting_id = entry.get("setting_id")
         setting_id = entry.get("setting_id")
         if setting_id:
         if setting_id:
-            for c in candidates:
+            for c in available:
                 if c.setting_id == setting_id:
                 if c.setting_id == setting_id:
                     return c
                     return c
 
 
         name = entry.get("name")
         name = entry.get("name")
         if name:
         if name:
-            for c in candidates:
+            for c in available:
                 if c.name == name:
                 if c.name == name:
                     return c
                     return c
 
 
         # Exactly one profile for this filament and no better discriminator:
         # 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
+        # treat it as the same profile rather than duplicating it. Judged
+        # against every candidate rather than the unclaimed ones, because two
+        # live profiles for one filament are ambiguous whether or not another
+        # entry has already taken one of them.
+        return available[0] if len(candidates) == 1 else None
 
 
 
 
 # Singleton instance
 # Singleton instance

+ 100 - 0
backend/tests/unit/test_github_restore.py

@@ -1340,6 +1340,106 @@ class TestRestoreKprofiles:
         profiles, _ = client.set_kprofiles_batch.call_args.args
         profiles, _ = client.set_kprofiles_batch.call_args.args
         assert profiles[0]["cali_idx"] == -1, "two candidates and nothing to tell them apart"
         assert profiles[0]["cali_idx"] == -1, "two candidates and nothing to tell them apart"
 
 
+    @pytest.mark.asyncio
+    async def test_two_entries_cannot_claim_the_same_live_slot(self, db_session, printer_factory):
+        """One live profile cannot stand in for two backed-up ones (#2656).
+
+        Both entries fell through to the single-candidate arm, both took
+        cali_idx 4606, both went into the batch — so the second overwrote the
+        first on the printer while the tally counted two restored. Reachable
+        whenever the user has deleted one of a pair since the backup, because
+        the delete-then-add re-key is what strips the setting_id match.
+        """
+        await printer_factory(serial_number="00M09A123456789")
+        payload = self._payload()
+        entries = payload["kprofiles/00M09A123456789/0.4.json"]["profiles"]
+        entries[0].update(setting_id="PFGONE1", name="PLA Basic")
+        entries.append({**entries[0], "setting_id": "PFGONE2", "name": "PLA Matte"})
+        client = self._client(live=[self._live(slot_id=4606, setting_id="PFUS123", 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, payload, tally)
+
+        profiles, _ = client.set_kprofiles_batch.call_args.args
+        assert [p["cali_idx"] for p in profiles] == [4606, -1], "the displaced entry has to be added, not aliased"
+        assert sum(1 for p in profiles if p["cali_idx"] == 4606) == 1
+        assert any("added as new profiles" in note for note in _messages(tally))
+
+    @pytest.mark.asyncio
+    async def test_the_displaced_entry_does_not_inherit_the_claimed_setting_id(self, db_session, printer_factory):
+        """An add-as-new keeps its own preset, or it lands on top of the match anyway.
+
+        cali_idx -1 is only safe if the rest of the payload doesn't point at the
+        profile the first entry just claimed — the generated-setting_id fallback
+        reads setting_id when cali_idx is -1.
+        """
+        await printer_factory(serial_number="00M09A123456789")
+        payload = self._payload()
+        entries = payload["kprofiles/00M09A123456789/0.4.json"]["profiles"]
+        entries[0].update(setting_id="PFGONE1", name="PLA Basic")
+        entries.append({**entries[0], "setting_id": "PFGONE2", "name": "PLA Matte"})
+        client = self._client(live=[self._live(slot_id=4606, setting_id="PFUS123", name="Bambu PLA")])
+
+        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, _CategoryTally())
+
+        profiles, _ = client.set_kprofiles_batch.call_args.args
+        assert profiles[0]["setting_id"] == "PFUS123", "the match prefers the live preset"
+        assert profiles[1]["setting_id"] == "PFGONE2", "the displaced entry keeps its own"
+
+    @pytest.mark.asyncio
+    async def test_two_entries_matching_two_live_profiles_keep_their_own_slots(self, db_session, printer_factory):
+        """Control: the guard must not displace a legitimate second match."""
+        await printer_factory(serial_number="00M09A123456789")
+        payload = self._payload()
+        entries = payload["kprofiles/00M09A123456789/0.4.json"]["profiles"]
+        entries.append({**entries[0], "setting_id": "PFUS456", "name": "Bambu PETG"})
+        client = self._client(
+            live=[
+                self._live(slot_id=4606, setting_id="PFUS123", name="Bambu PLA"),
+                self._live(slot_id=4607, setting_id="PFUS456", name="Bambu PETG"),
+            ]
+        )
+        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 [p["cali_idx"] for p in profiles] == [4606, 4607]
+        assert not any("added as new profiles" in note for note in _messages(tally))
+
+    @pytest.mark.asyncio
+    async def test_a_claimed_slot_does_not_make_an_ambiguous_pair_matchable(self, db_session, printer_factory):
+        """Two live profiles for one filament stay ambiguous after one is taken.
+
+        The single-candidate fallback is judged against every candidate, not the
+        unclaimed ones — otherwise claiming the first would leave exactly one
+        "available" and turn a guess the code deliberately refuses into a match.
+        """
+        await printer_factory(serial_number="00M09A123456789")
+        payload = self._payload()
+        entries = payload["kprofiles/00M09A123456789/0.4.json"]["profiles"]
+        entries[0].update(setting_id="PFUS123", name="Bambu PLA")
+        entries.append({**entries[0], "setting_id": None, "name": ""})
+        client = self._client(
+            live=[
+                self._live(slot_id=1, setting_id="PFUS123", name="Bambu PLA"),
+                self._live(slot_id=2, setting_id="PFOTHER", name="Renamed"),
+            ]
+        )
+
+        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, _CategoryTally())
+
+        profiles, _ = client.set_kprofiles_batch.call_args.args
+        assert [p["cali_idx"] for p in profiles] == [1, -1]
+
     @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()