diff --git a/backend/app/services/github_restore.py b/backend/app/services/github_restore.py index 6cd0ba1f1..d3a05c7d0 100644 --- a/backend/app/services/github_restore.py +++ b/backend/app/services/github_restore.py @@ -1486,12 +1486,19 @@ class GitHubRestoreService: profile_dicts = [] 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: if not isinstance(p, dict): continue - match = self._match_kprofile(p, current) + match = self._match_kprofile(p, current, claimed) if match is None: unmatched += 1 + else: + claimed.add(match.slot_id) profile_dicts.append( { "filament_id": p.get("filament_id", ""), @@ -1554,7 +1561,7 @@ class GitHubRestoreService: return [] @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. ``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. Both are scoped by ``filament_id`` — the same preset on a different 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") if not filament_id: return None 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 setting_id = entry.get("setting_id") if setting_id: - for c in candidates: + for c in available: if c.setting_id == setting_id: return c name = entry.get("name") if name: - for c in candidates: + for c in available: 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 + # 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 diff --git a/backend/tests/unit/test_github_restore.py b/backend/tests/unit/test_github_restore.py index 7a97e4317..9c3f525ef 100644 --- a/backend/tests/unit/test_github_restore.py +++ b/backend/tests/unit/test_github_restore.py @@ -1340,6 +1340,106 @@ class TestRestoreKprofiles: 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_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 async def test_unknown_serial_is_skipped_with_reason(self, db_session): tally = _CategoryTally()