Jelajahi Sumber

Fix Spoolman location not cleared on auto-sync when spool removed from AMS (#921)

  The on_ams_change auto-sync callback set locations for new spools but
  never called clear_location_for_removed_spools(), leaving stale locations
  that caused double-booked slots. Also pass synced_spool_ids in the
  single-printer sync route to match the sync-all endpoint behavior.
maziggy 1 bulan lalu
induk
melakukan
813d9dde38

+ 1 - 0
CHANGELOG.md

@@ -6,6 +6,7 @@ All notable changes to Bambuddy will be documented in this file.
 
 ### Fixed
 - **"Build Plate Cleared" Button Unclickable After Second Print** ([#912](https://github.com/maziggy/bambuddy/issues/912)) — After completing the first queued print and confirming the plate was cleared, the "Build plate cleared — ready for next print" button became unresponsive after the second print finished. The React Query mutation's `isSuccess` state persisted from the first plate-clear confirmation, causing the component to render the static "Plate Ready" confirmation instead of the clickable button. The mutation state is now reset when the printer leaves the FINISH/FAILED state, so the button works correctly on every print cycle.
+- **Spoolman Location Not Cleared When Spool Removed from AMS** ([#921](https://github.com/maziggy/bambuddy/issues/921)) — When Spoolman auto-sync was enabled and a spool was removed from an AMS slot, its location in Spoolman was never cleared, causing "double-booked" slots where multiple spools shared the same location. The auto-sync callback set locations for newly inserted spools but skipped the cleanup step that clears stale locations. The location clearing logic now runs after every auto-sync cycle. Also fixed the single-printer manual sync endpoint which didn't track synced spool IDs, risking incorrect location clearing for location-matched (non-RFID) spools.
 
 ## [0.2.3b2] - 2026-04-08
 

+ 4 - 2
backend/app/api/routes/spoolman.py

@@ -197,6 +197,7 @@ async def sync_printer_ams(
     errors = []
     # Track tray UUIDs currently in the AMS (for clearing removed spools)
     current_tray_uuids: set[str] = set()
+    synced_spool_ids: set[int] = set()
 
     # Handle different AMS data structures
     # Traditional AMS: list of {"id": N, "tray": [...]} dicts
@@ -299,8 +300,9 @@ async def sync_printer_ams(
                 )
                 if sync_result:
                     synced += 1
-                    # Add newly created spool to cache
+                    # Add newly created spool to cache and track synced ID
                     if sync_result.get("id"):
+                        synced_spool_ids.add(sync_result["id"])
                         spool_exists = any(s.get("id") == sync_result["id"] for s in cached_spools)
                         if not spool_exists:
                             cached_spools.append(sync_result)
@@ -319,7 +321,7 @@ async def sync_printer_ams(
     # Clear location for spools that were removed from this printer's AMS
     try:
         cleared = await client.clear_location_for_removed_spools(
-            printer.name, current_tray_uuids, cached_spools=cached_spools
+            printer.name, current_tray_uuids, cached_spools=cached_spools, synced_spool_ids=synced_spool_ids
         )
         if cleared > 0:
             logger.info("Cleared location for %s spools removed from %s", cleared, printer.name)

+ 25 - 3
backend/app/main.py

@@ -1002,8 +1002,10 @@ async def on_ams_change(printer_id: int, ams_data: list):
             except Exception as e:
                 logger.debug("Could not load inventory weights for printer %s: %s", printer_id, e)
 
-            # Sync each AMS tray
+            # Sync each AMS tray, tracking UUIDs and spool IDs for cleanup
             synced = 0
+            current_tray_uuids: set[str] = set()
+            synced_spool_ids: set[int] = set()
             for ams_unit in ams_data:
                 ams_id = int(ams_unit.get("id", 0))
                 trays = ams_unit.get("tray", [])
@@ -1013,6 +1015,15 @@ async def on_ams_change(printer_id: int, ams_data: list):
                     if not tray:
                         continue  # Empty tray
 
+                    # Track this spool's UUID as currently present in the AMS
+                    spool_tag = (
+                        tray.tray_uuid
+                        if tray.tray_uuid and tray.tray_uuid != "00000000000000000000000000000000"
+                        else tray.tag_uid
+                    )
+                    if spool_tag:
+                        current_tray_uuids.add(spool_tag.upper())
+
                     try:
                         inv_remaining = inventory_weights.get((ams_id, tray.tray_id))
                         result = await client.sync_ams_tray(
@@ -1024,9 +1035,10 @@ async def on_ams_change(printer_id: int, ams_data: list):
                         )
                         if result:
                             synced += 1
-                            # If a new spool was created, add it to the cache
-                            # so subsequent trays can find it if they reference the same tag
                             if result.get("id"):
+                                synced_spool_ids.add(result["id"])
+                                # If a new spool was created, add it to the cache
+                                # so subsequent trays can find it if they reference the same tag
                                 # Check if this spool already exists in cache
                                 spool_exists = any(s.get("id") == result["id"] for s in cached_spools)
                                 if not spool_exists:
@@ -1042,6 +1054,16 @@ async def on_ams_change(printer_id: int, ams_data: list):
             if synced > 0:
                 logger.info("Auto-synced %s AMS trays to Spoolman for printer %s", synced, printer_id)
 
+            # Clear location for spools no longer in this printer's AMS
+            try:
+                cleared = await client.clear_location_for_removed_spools(
+                    printer_name, current_tray_uuids, cached_spools=cached_spools, synced_spool_ids=synced_spool_ids
+                )
+                if cleared > 0:
+                    logger.info("Auto-cleared location for %s spools removed from printer %s", cleared, printer_id)
+            except Exception as e:
+                logger.error("Error clearing locations for removed spools on printer %s: %s", printer_id, e)
+
     except Exception as e:
         logging.getLogger(__name__).warning(f"Spoolman AMS sync failed: {e}")
 

+ 186 - 0
backend/tests/unit/test_spoolman_clear_location.py

@@ -0,0 +1,186 @@
+"""Unit tests for Spoolman location clearing when spools are removed from AMS.
+
+Tests the clear_location_for_removed_spools method to verify that stale
+Spoolman locations are cleared during both auto-sync and manual sync,
+preventing the "double-booked" slot bug (#921).
+"""
+
+from unittest.mock import AsyncMock, patch
+
+import pytest
+
+from backend.app.services.spoolman import SpoolmanClient
+
+BAMBU_UUID_A = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+BAMBU_UUID_B = "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"
+BAMBU_UUID_C = "CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC"
+PRINTER_NAME = "My Printer"
+LOCATION_PREFIX = f"{PRINTER_NAME} - "
+
+
+def _make_spool(spool_id: int, location: str, tag: str = "", extra: dict | None = None) -> dict:
+    """Create a mock Spoolman spool dict."""
+    return {
+        "id": spool_id,
+        "location": location,
+        "extra": extra or {"tag": tag},
+    }
+
+
+@pytest.fixture
+def client():
+    """Create a SpoolmanClient without connecting."""
+    return SpoolmanClient("http://localhost:7912")
+
+
+class TestClearLocationForRemovedSpools:
+    """Test the clear_location_for_removed_spools method."""
+
+    @pytest.mark.asyncio
+    async def test_clears_spool_no_longer_in_ams(self, client):
+        """A spool whose UUID is not in current_tray_uuids should have its location cleared."""
+        cached_spools = [
+            _make_spool(1, f"{LOCATION_PREFIX}AMS A Slot 1", BAMBU_UUID_A),
+        ]
+
+        with patch.object(client, "update_spool", new_callable=AsyncMock, return_value=True) as mock_update:
+            cleared = await client.clear_location_for_removed_spools(
+                PRINTER_NAME, current_tray_uuids=set(), cached_spools=cached_spools
+            )
+
+        assert cleared == 1
+        mock_update.assert_called_once_with(spool_id=1, clear_location=True)
+
+    @pytest.mark.asyncio
+    async def test_keeps_spool_still_in_ams(self, client):
+        """A spool whose UUID is in current_tray_uuids should not be cleared."""
+        cached_spools = [
+            _make_spool(1, f"{LOCATION_PREFIX}AMS A Slot 1", BAMBU_UUID_A),
+        ]
+
+        with patch.object(client, "update_spool", new_callable=AsyncMock) as mock_update:
+            cleared = await client.clear_location_for_removed_spools(
+                PRINTER_NAME, current_tray_uuids={BAMBU_UUID_A}, cached_spools=cached_spools
+            )
+
+        assert cleared == 0
+        mock_update.assert_not_called()
+
+    @pytest.mark.asyncio
+    async def test_skips_non_bambu_spools(self, client):
+        """Spools without a 32-char RFID tag should not be cleared (non-Bambu / third-party)."""
+        cached_spools = [
+            _make_spool(1, f"{LOCATION_PREFIX}AMS A Slot 1", "SHORT_TAG"),
+            _make_spool(2, f"{LOCATION_PREFIX}AMS A Slot 2", ""),
+        ]
+
+        with patch.object(client, "update_spool", new_callable=AsyncMock) as mock_update:
+            cleared = await client.clear_location_for_removed_spools(
+                PRINTER_NAME, current_tray_uuids=set(), cached_spools=cached_spools
+            )
+
+        assert cleared == 0
+        mock_update.assert_not_called()
+
+    @pytest.mark.asyncio
+    async def test_skips_spools_from_other_printers(self, client):
+        """Spools with locations for a different printer should not be touched."""
+        cached_spools = [
+            _make_spool(1, "Other Printer - AMS A Slot 1", BAMBU_UUID_A),
+        ]
+
+        with patch.object(client, "update_spool", new_callable=AsyncMock) as mock_update:
+            cleared = await client.clear_location_for_removed_spools(
+                PRINTER_NAME, current_tray_uuids=set(), cached_spools=cached_spools
+            )
+
+        assert cleared == 0
+        mock_update.assert_not_called()
+
+    @pytest.mark.asyncio
+    async def test_synced_spool_ids_protects_location_matched_spools(self, client):
+        """Spools in synced_spool_ids should not be cleared even if UUID doesn't match."""
+        cached_spools = [
+            _make_spool(1, f"{LOCATION_PREFIX}AMS A Slot 1", BAMBU_UUID_A),
+        ]
+
+        with patch.object(client, "update_spool", new_callable=AsyncMock) as mock_update:
+            cleared = await client.clear_location_for_removed_spools(
+                PRINTER_NAME,
+                current_tray_uuids=set(),
+                cached_spools=cached_spools,
+                synced_spool_ids={1},
+            )
+
+        assert cleared == 0
+        mock_update.assert_not_called()
+
+    @pytest.mark.asyncio
+    async def test_clears_only_removed_spools_in_mixed_set(self, client):
+        """With multiple spools at a printer, only clear the one that was removed."""
+        cached_spools = [
+            _make_spool(1, f"{LOCATION_PREFIX}AMS A Slot 1", BAMBU_UUID_A),  # Still in AMS
+            _make_spool(2, f"{LOCATION_PREFIX}AMS A Slot 2", BAMBU_UUID_B),  # Removed
+            _make_spool(3, f"{LOCATION_PREFIX}AMS A Slot 3", BAMBU_UUID_C),  # Still in AMS
+        ]
+
+        with patch.object(client, "update_spool", new_callable=AsyncMock, return_value=True) as mock_update:
+            cleared = await client.clear_location_for_removed_spools(
+                PRINTER_NAME,
+                current_tray_uuids={BAMBU_UUID_A, BAMBU_UUID_C},
+                cached_spools=cached_spools,
+            )
+
+        assert cleared == 1
+        mock_update.assert_called_once_with(spool_id=2, clear_location=True)
+
+    @pytest.mark.asyncio
+    async def test_uuid_comparison_is_case_insensitive(self, client):
+        """UUID matching should work regardless of case."""
+        cached_spools = [
+            _make_spool(1, f"{LOCATION_PREFIX}AMS A Slot 1", BAMBU_UUID_A.lower()),
+        ]
+
+        with patch.object(client, "update_spool", new_callable=AsyncMock) as mock_update:
+            cleared = await client.clear_location_for_removed_spools(
+                PRINTER_NAME,
+                current_tray_uuids={BAMBU_UUID_A},  # Uppercase
+                cached_spools=cached_spools,
+            )
+
+        assert cleared == 0
+        mock_update.assert_not_called()
+
+    @pytest.mark.asyncio
+    async def test_returns_zero_when_no_spools_at_printer(self, client):
+        """When no spools have locations for this printer, nothing is cleared."""
+        with patch.object(client, "update_spool", new_callable=AsyncMock) as mock_update:
+            cleared = await client.clear_location_for_removed_spools(
+                PRINTER_NAME, current_tray_uuids=set(), cached_spools=[]
+            )
+
+        assert cleared == 0
+        mock_update.assert_not_called()
+
+    @pytest.mark.asyncio
+    async def test_double_booking_scenario(self, client):
+        """Reproduce #921: two spools assigned to the same printer location.
+
+        When SpoolA is removed and SpoolB takes its slot, SpoolA's old location
+        should be cleared because its UUID is no longer in current_tray_uuids.
+        """
+        cached_spools = [
+            _make_spool(1, f"{LOCATION_PREFIX}AMS A Slot 1", BAMBU_UUID_A),  # OLD — was removed
+            _make_spool(2, f"{LOCATION_PREFIX}AMS A Slot 1", BAMBU_UUID_B),  # NEW — just inserted
+        ]
+
+        with patch.object(client, "update_spool", new_callable=AsyncMock, return_value=True) as mock_update:
+            cleared = await client.clear_location_for_removed_spools(
+                PRINTER_NAME,
+                current_tray_uuids={BAMBU_UUID_B},  # Only SpoolB is in AMS now
+                cached_spools=cached_spools,
+                synced_spool_ids={2},  # SpoolB was just synced
+            )
+
+        assert cleared == 1
+        mock_update.assert_called_once_with(spool_id=1, clear_location=True)

+ 1 - 1
frontend/src/components/PrinterQueueWidget.tsx

@@ -48,7 +48,7 @@ export function PrinterQueueWidget({ printerId, printerModel, printerState, plat
     if (printerState !== 'FINISH' && printerState !== 'FAILED') {
       clearPlateMutation.reset();
     }
-  }, [printerState]);
+  }, [printerState, clearPlateMutation]);
 
   // Filter queue to items this printer can actually print (filament type + color check)
   const compatibleQueue = queue ? filterCompatibleQueueItems(queue, loadedFilamentTypes, loadedFilaments) : undefined;

File diff ditekan karena terlalu besar
+ 0 - 0
static/assets/index-BluaTs0d.js


+ 1 - 1
static/index.html

@@ -23,7 +23,7 @@
 
     <!-- Splash screens for iOS -->
     <link rel="apple-touch-startup-image" href="/img/android-chrome-512x512.png" />
-    <script type="module" crossorigin src="/assets/index-CNOL7dUq.js"></script>
+    <script type="module" crossorigin src="/assets/index-BluaTs0d.js"></script>
     <link rel="stylesheet" crossorigin href="/assets/index-CA19cW_f.css">
   </head>
   <body>

Beberapa file tidak ditampilkan karena terlalu banyak file yang berubah dalam diff ini