Browse Source

fix(spoolman): clear stale fallback-tag links on assign + link, prefer slot-assignment over tag-link in UI (#1457)

  Reporter on a P1S with non-RFID spools saw an old, almost-empty spool in
  the AMS hover card's "Spulen-ID" block while the "Zugewiesen" block
  correctly showed the freshly assigned full spool. Two layers compounded:

    (1) Non-RFID slots fall back to a deterministic per-slot tag
        (hash(serial) + ams_id + tray_id). The Link / Assign routes wrote
        that tag to Spoolman extra.tag but never cleared it from the
        previous holder on re-binding.

    (2) The frontend's hover-card resolver preferred the (stale) tag-link
        over the user's explicit slot-assignment. Same precedence bug in
        SpoolBuddy's fill-bar resolver and slot-action picker.

  Frontend: swap precedence at 5 sites — slot-assignment outranks tag-link
  everywhere. FilamentHoverCard's existing match-dedupe then collapses the
  two "Open in Inventory" buttons back into one.

  Backend: new _clear_stale_tag_links() in spoolman_inventory.py, called
  from POST /spoolman/inventory/slot-assignments (with the slot's
  deterministic fallback tag) and POST /spoolman/spools/{id}/link (with
  the literal tag being bound — works for RFID and fallback). Best-effort:
  Spoolman 5xx and per-spool patch failures log + continue, never wedge
  the bind. get_fallback_spool_tag_for_slot promoted to a public helper
  mirroring the frontend's signature exactly.
maziggy 1 week ago
parent
commit
12b0c138f7

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 16 - 0
backend/app/api/routes/spoolman.py

@@ -11,6 +11,7 @@ from sqlalchemy.ext.asyncio import AsyncSession
 from sqlalchemy.orm import selectinload
 
 from backend.app.api.routes._spoolman_helpers import _map_spoolman_spool
+from backend.app.api.routes.spoolman_inventory import _clear_stale_tag_links
 from backend.app.core.auth import RequirePermissionIfAuthEnabled
 from backend.app.core.database import get_db
 from backend.app.core.permissions import Permission
@@ -842,6 +843,21 @@ async def link_spool(
 
     logger.info("Linked Spoolman spool %s to tag %s", spool_id, spool_tag)
 
+    # #1457: clear stale tag links on OTHER spools still claiming this exact tag.
+    # A given AMS-slot tag (RFID or deterministic fallback) belongs to one
+    # physical spool; without this cleanup the previous holder's extra.tag
+    # keeps it visible in the hover card / fill-level lookup.
+    await _clear_stale_tag_links(
+        client,
+        tag=spool_tag,
+        keep_spool_id=spool_id,
+        log_context=(
+            f"printer={printer_context[0]} ams={printer_context[1]} tray={printer_context[2]}"
+            if printer_context
+            else "via /spools/{id}/link"
+        ),
+    )
+
     # Auto-configure AMS slot via MQTT (best-effort; tag link and slot assignment already persisted)
     if printer_context:
         p_id, a_id, t_id = printer_context

+ 97 - 0
backend/app/api/routes/spoolman_inventory.py

@@ -51,6 +51,7 @@ from backend.app.services.spoolman import (
     get_spoolman_client,
     init_spoolman_client,
 )
+from backend.app.services.spoolman_tracking import get_fallback_spool_tag_for_slot
 from backend.app.utils.filament_ids import (
     GENERIC_FILAMENT_IDS,
     MATERIAL_TEMPS,
@@ -73,6 +74,89 @@ def _tag_cleared(val: str | None) -> bool:
     return val is None
 
 
+async def _clear_stale_tag_links(
+    client: SpoolmanClient,
+    *,
+    tag: str,
+    keep_spool_id: int,
+    log_context: str,
+) -> int:
+    """Clear extra.tag on OTHER spools still claiming the given tag (#1457).
+
+    A given AMS slot tag — whether a real RFID (tray_uuid/tag_uid) or the
+    deterministic fallback derived from (printer_serial, ams_id, tray_id) for
+    non-RFID slots — uniquely identifies one physical slot. When a spool is
+    (re)bound to that slot via Assign or Link, any other Spoolman spool whose
+    extra.tag still holds the same value is stale and would resurface in the
+    hover card / fill-level lookup.
+
+    Best-effort: per-spool patch failures are logged and skipped, never raised.
+    Returns the number of spools cleared.
+    """
+    if not tag:
+        return 0
+    tag_upper = tag.upper()
+
+    try:
+        spools = await client.get_spools()
+    except (SpoolmanClientError, SpoolmanUnavailableError) as exc:
+        logger.warning("Could not enumerate spools for stale-tag cleanup: %s", exc)
+        return 0
+
+    cleared = 0
+    for spool in spools:
+        spool_id = spool.get("id")
+        if not spool_id or spool_id == keep_spool_id:
+            continue
+        extra = spool.get("extra") or {}
+        raw_tag = extra.get("tag", "")
+        if not raw_tag:
+            continue
+        clean_tag = raw_tag.strip('"').upper()
+        if clean_tag != tag_upper:
+            continue
+        try:
+            await client.merge_spool_extra(spool_id, {"tag": json.dumps("")})
+            cleared += 1
+            logger.info(
+                "Cleared stale tag '%s' from Spoolman spool %s (%s; reassigned to spool %s)",
+                tag_upper[:16],
+                spool_id,
+                log_context,
+                keep_spool_id,
+            )
+        except (SpoolmanClientError, SpoolmanUnavailableError, SpoolmanNotFoundError) as exc:
+            logger.warning(
+                "Failed to clear stale tag on Spoolman spool %s: %s",
+                spool_id,
+                exc,
+            )
+    return cleared
+
+
+async def _clear_stale_slot_fallback_tag_links(
+    client: SpoolmanClient,
+    *,
+    printer_serial: str,
+    ams_id: int,
+    tray_id: int,
+    keep_spool_id: int,
+) -> int:
+    """Convenience wrapper: compute the slot's fallback tag and clear it from
+    other spools. Used by the assign route, which identifies the slot by
+    (printer, ams, tray) rather than by an explicit tag value.
+    """
+    fallback_tag = get_fallback_spool_tag_for_slot(printer_serial, ams_id, tray_id)
+    if not fallback_tag:
+        return 0
+    return await _clear_stale_tag_links(
+        client,
+        tag=fallback_tag,
+        keep_spool_id=keep_spool_id,
+        log_context=f"printer={printer_serial} ams={ams_id} tray={tray_id}",
+    )
+
+
 async def _get_client(db: AsyncSession) -> SpoolmanClient:
     """Return a validated Spoolman client (URL checked, health-checked) or raise an HTTP error."""
     result = await db.execute(select(Settings))
@@ -1170,6 +1254,19 @@ async def assign_spoolman_slot(
         logger.error("Failed to persist slot assignment: %s", exc)
         raise HTTPException(status_code=500, detail="Failed to save slot assignment") from exc
 
+    # #1457: clear stale fallback-tag links on OTHER spools still bound to this
+    # slot. Without this, a non-RFID slot's deterministic fallback tag stays
+    # attached to the previous spool in Spoolman's extra.tag and re-surfaces in
+    # the hover card whenever the local slot assignment is removed.
+    if printer.serial_number:
+        await _clear_stale_slot_fallback_tag_links(
+            client,
+            printer_serial=printer.serial_number,
+            ams_id=body.ams_id,
+            tray_id=body.tray_id,
+            keep_spool_id=body.spoolman_spool_id,
+        )
+
     mapped = _map_spoolman_spool(spool)
 
     # Fetch K-profiles before the MQTT try block so we can use async DB access.

+ 11 - 0
backend/app/services/spoolman_tracking.py

@@ -67,6 +67,17 @@ def _get_fallback_spool_tag(printer_serial: str, global_tray_id: int) -> str:
     if not printer_serial:
         return ""
     ams_id, tray_id = _global_tray_id_to_ams_slot(global_tray_id)
+    return get_fallback_spool_tag_for_slot(printer_serial, ams_id, tray_id)
+
+
+def get_fallback_spool_tag_for_slot(printer_serial: str, ams_id: int, tray_id: int) -> str:
+    """Public helper matching frontend getFallbackSpoolTag(serial, amsId, trayId).
+
+    Used by stale-tag cleanup (#1457) to detect Spoolman spools still holding
+    this slot's deterministic fallback tag in extra.tag.
+    """
+    if not printer_serial:
+        return ""
     return f"{_hash_serial_to_hex32(printer_serial)}{_to_fixed_hex(ams_id, 4)}{_to_fixed_hex(tray_id, 4)}"
 
 

+ 2 - 0
backend/tests/integration/test_spoolman_api.py

@@ -1159,6 +1159,8 @@ class TestLinkSpoolMqttConfigure:
         mock_client.merge_spool_extra = AsyncMock(
             return_value={"id": 5, "extra": {"tag": '"A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4"'}}
         )
+        # #1457 stale-tag cleanup enumerates spools; default to empty so it's a no-op.
+        mock_client.get_spools = AsyncMock(return_value=[])
         mock_client.get_spool = AsyncMock(
             return_value={
                 "id": 5,

+ 3 - 0
backend/tests/integration/test_spoolman_slot_assignment_mqtt.py

@@ -65,6 +65,9 @@ def mock_spoolman_client():
     client.base_url = "http://localhost:7912"
     client.health_check = AsyncMock(return_value=True)
     client.get_spool = AsyncMock(return_value=SAMPLE_SPOOL)
+    # #1457: assign route enumerates spools to clear stale fallback-tag links.
+    client.get_spools = AsyncMock(return_value=[])
+    client.merge_spool_extra = AsyncMock(return_value={"id": 0, "extra": {}})
 
     with patch(
         "backend.app.api.routes.spoolman_inventory._get_client",

+ 4 - 0
backend/tests/integration/test_spoolman_slot_assignments.py

@@ -70,6 +70,10 @@ def mock_client():
     client.base_url = "http://localhost:7912"
     client.health_check = AsyncMock(return_value=True)
     client.get_spool = AsyncMock(return_value=SAMPLE_SPOOL)
+    # #1457: assign route enumerates spools to clear stale fallback-tag links.
+    # Default to empty so the cleanup is a no-op for tests that don't exercise it.
+    client.get_spools = AsyncMock(return_value=[])
+    client.merge_spool_extra = AsyncMock(return_value={"id": 0, "extra": {}})
 
     with patch(
         "backend.app.api.routes.spoolman_inventory._get_client",

+ 3 - 0
backend/tests/integration/test_spoolman_slot_concurrency.py

@@ -61,6 +61,9 @@ def mock_client():
     client.base_url = "http://localhost:7912"
     client.health_check = AsyncMock(return_value=True)
     client.get_spool = AsyncMock(return_value=SAMPLE_SPOOL)
+    # #1457: assign route enumerates spools to clear stale fallback-tag links.
+    client.get_spools = AsyncMock(return_value=[])
+    client.merge_spool_extra = AsyncMock(return_value={"id": 0, "extra": {}})
 
     with patch(
         "backend.app.api.routes.spoolman_inventory._get_client",

+ 135 - 0
backend/tests/unit/test_spoolman_stale_tag_cleanup.py

@@ -0,0 +1,135 @@
+"""Unit tests for #1457 stale fallback-tag cleanup.
+
+When a user (re)assigns or (re)links a Spoolman spool to an AMS slot, any OTHER
+Spoolman spool whose extra.tag still holds the same value is stale — the hover
+card and fill-level lookup would surface the wrong spool. The cleanup helper
+clears extra.tag on those orphans.
+"""
+
+import json
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+
+from backend.app.api.routes.spoolman_inventory import (
+    _clear_stale_slot_fallback_tag_links,
+    _clear_stale_tag_links,
+)
+from backend.app.services.spoolman import SpoolmanClientError
+from backend.app.services.spoolman_tracking import get_fallback_spool_tag_for_slot
+
+
+def _make_client(spools, *, merge_side_effect=None):
+    client = MagicMock()
+    client.get_spools = AsyncMock(return_value=spools)
+    client.merge_spool_extra = AsyncMock(
+        side_effect=merge_side_effect,
+        return_value={"id": 0, "extra": {}},
+    )
+    return client
+
+
+@pytest.mark.asyncio
+class TestClearStaleTagLinks:
+    async def test_clears_other_spools_with_same_tag(self):
+        target_tag = "AABBCCDDEEFF0011"
+        client = _make_client(
+            [
+                {"id": 7, "extra": {"tag": json.dumps(target_tag)}},  # stale orphan -> clear
+                {"id": 8, "extra": {"tag": json.dumps("UNRELATED12345678")}},  # different tag -> keep
+                {"id": 9, "extra": {"tag": json.dumps(target_tag)}},  # newly bound -> keep_spool_id
+                {"id": 10, "extra": {}},  # no tag -> keep
+            ]
+        )
+
+        cleared = await _clear_stale_tag_links(client, tag=target_tag, keep_spool_id=9, log_context="test")
+
+        assert cleared == 1
+        client.merge_spool_extra.assert_called_once_with(7, {"tag": json.dumps("")})
+
+    async def test_case_insensitive_match(self):
+        target_tag = "aabbccdd11223344"
+        client = _make_client([{"id": 5, "extra": {"tag": json.dumps(target_tag.upper())}}])
+
+        cleared = await _clear_stale_tag_links(client, tag=target_tag, keep_spool_id=99, log_context="test")
+
+        assert cleared == 1
+
+    async def test_empty_tag_no_op(self):
+        client = _make_client([{"id": 1, "extra": {"tag": json.dumps("AABB")}}])
+
+        cleared = await _clear_stale_tag_links(client, tag="", keep_spool_id=99, log_context="test")
+
+        assert cleared == 0
+        client.get_spools.assert_not_called()
+        client.merge_spool_extra.assert_not_called()
+
+    async def test_skips_keep_spool_id(self):
+        target_tag = "AABB00112233CCDD"
+        client = _make_client([{"id": 42, "extra": {"tag": json.dumps(target_tag)}}])
+
+        cleared = await _clear_stale_tag_links(client, tag=target_tag, keep_spool_id=42, log_context="test")
+
+        assert cleared == 0
+        client.merge_spool_extra.assert_not_called()
+
+    async def test_swallows_get_spools_error(self):
+        client = MagicMock()
+        client.get_spools = AsyncMock(side_effect=SpoolmanClientError("boom", status_code=500))
+        client.merge_spool_extra = AsyncMock()
+
+        cleared = await _clear_stale_tag_links(client, tag="AABB00112233CCDD", keep_spool_id=99, log_context="test")
+
+        assert cleared == 0
+        client.merge_spool_extra.assert_not_called()
+
+    async def test_continues_when_one_patch_fails(self):
+        target_tag = "AABB00112233CCDD"
+        client = _make_client(
+            [
+                {"id": 1, "extra": {"tag": json.dumps(target_tag)}},
+                {"id": 2, "extra": {"tag": json.dumps(target_tag)}},
+            ],
+            merge_side_effect=[SpoolmanClientError("first fails", status_code=500), {"id": 2, "extra": {}}],
+        )
+
+        cleared = await _clear_stale_tag_links(client, tag=target_tag, keep_spool_id=99, log_context="test")
+
+        assert cleared == 1
+        assert client.merge_spool_extra.call_count == 2
+
+
+@pytest.mark.asyncio
+class TestClearStaleSlotFallbackTagLinks:
+    async def test_computes_fallback_tag_and_clears(self):
+        serial = "ABCDEF123456"
+        ams_id, tray_id = 0, 2
+        fallback_tag = get_fallback_spool_tag_for_slot(serial, ams_id, tray_id)
+        assert fallback_tag, "sanity: helper must produce a tag for a real serial"
+
+        client = _make_client([{"id": 11, "extra": {"tag": json.dumps(fallback_tag)}}])
+
+        cleared = await _clear_stale_slot_fallback_tag_links(
+            client,
+            printer_serial=serial,
+            ams_id=ams_id,
+            tray_id=tray_id,
+            keep_spool_id=22,
+        )
+
+        assert cleared == 1
+        client.merge_spool_extra.assert_called_once_with(11, {"tag": json.dumps("")})
+
+    async def test_empty_serial_no_op(self):
+        client = _make_client([{"id": 1, "extra": {"tag": json.dumps("AABB")}}])
+
+        cleared = await _clear_stale_slot_fallback_tag_links(
+            client,
+            printer_serial="",
+            ams_id=0,
+            tray_id=0,
+            keep_spool_id=1,
+        )
+
+        assert cleared == 0
+        client.get_spools.assert_not_called()

+ 11 - 6
frontend/src/pages/PrintersPage.tsx

@@ -3733,8 +3733,11 @@ function PrinterCard({
                                         data={filamentData}
                                         spoolman={{
                                           enabled: spoolmanEnabled,
-                                          linkedSpoolId: (trayTag ? linkedSpools?.[trayTag]?.id : undefined)
-                                            ?? slotAssignmentForFill?.spoolman_spool_id,
+                                          // #1457: slot assignment is the user's most explicit action — it must
+                                          // outrank the tag-link, which can be stale when a non-RFID slot's
+                                          // fallback tag is still attached to a previous spool in Spoolman.
+                                          linkedSpoolId: slotAssignmentForFill?.spoolman_spool_id
+                                            ?? (trayTag ? linkedSpools?.[trayTag]?.id : undefined),
                                           spoolmanUrl,
                                           syncMode: spoolmanSyncMode,
                                           // Suppress Link button when slot is already occupied by ANY assignment
@@ -4134,8 +4137,9 @@ function PrinterCard({
                                     data={filamentData}
                                     spoolman={{
                                       enabled: spoolmanEnabled,
-                                      linkedSpoolId: (htTrayTag ? linkedSpools?.[htTrayTag]?.id : undefined)
-                                        ?? htSlotAssignmentForFill?.spoolman_spool_id,
+                                      // #1457: slot assignment outranks tag-link (see top-level slot block).
+                                      linkedSpoolId: htSlotAssignmentForFill?.spoolman_spool_id
+                                        ?? (htTrayTag ? linkedSpools?.[htTrayTag]?.id : undefined),
                                       spoolmanUrl,
                                       syncMode: spoolmanSyncMode,
                                       // Suppress Link button when slot is occupied by ANY assignment (Phase 13 P13-6d)
@@ -4449,8 +4453,9 @@ function PrinterCard({
                                       data={extFilamentData}
                                       spoolman={{
                                         enabled: spoolmanEnabled,
-                                        linkedSpoolId: (extTrayTag ? linkedSpools?.[extTrayTag]?.id : undefined)
-                                          ?? extSlotAssignmentForFill?.spoolman_spool_id,
+                                        // #1457: slot assignment outranks tag-link (see top-level slot block).
+                                        linkedSpoolId: extSlotAssignmentForFill?.spoolman_spool_id
+                                          ?? (extTrayTag ? linkedSpools?.[extTrayTag]?.id : undefined),
                                         spoolmanUrl,
                                         syncMode: spoolmanSyncMode,
                                         // Suppress Link button when slot is occupied by ANY assignment (Phase 13 P13-6d)

+ 18 - 12
frontend/src/pages/spoolbuddy/SpoolBuddyAmsPage.tsx

@@ -136,16 +136,11 @@ export function SpoolBuddyAmsPage() {
   // Look up Spoolman fill level for a given tray
   const printerSerial = printer?.serial_number ?? '';
   const getSpoolmanFillForSlot = useCallback((amsId: number, trayId: number, tray: AMSTray | null): number | null => {
-    // Stage 1: tag-linked spool (linkedSpools map keyed by tag/UUID).
-    if (linkedSpools && printerSerial) {
-      const tag = (tray?.tray_uuid || tray?.tag_uid || getFallbackSpoolTag(printerSerial, amsId, trayId))?.toUpperCase();
-      const linkedSpool = tag ? linkedSpools[tag] : undefined;
-      const tagFill = getSpoolmanFillLevel(linkedSpool);
-      if (tagFill !== null) return tagFill;
-    }
-    // Stage 2: slot-assigned-only Spoolman spool (no tag link). Bug #6 in
-    // maintainer screenshot: spool was assigned to slot via AssignToAmsModal
-    // but never tag-linked, so the fill bar stayed empty.
+    // Stage 1: slot-assigned Spoolman spool. The user's explicit, recent
+    // action — must outrank the tag-link to avoid #1457, where a non-RFID
+    // slot's deterministic fallback tag stayed bound to the previous spool
+    // in Spoolman's extra.tag and the fill bar reported the old (stale)
+    // spool's remaining weight instead of the freshly assigned one.
     if (selectedPrinterId !== null && spoolmanSlotAssignmentsAll.length && spoolmanInventorySpoolsCache.length) {
       const slotAssign = spoolmanSlotAssignmentsAll.find(a =>
         a.printer_id === selectedPrinterId &&
@@ -159,6 +154,13 @@ export function SpoolBuddyAmsPage() {
         }
       }
     }
+    // Stage 2: tag-linked spool (linkedSpools map keyed by tag/UUID).
+    if (linkedSpools && printerSerial) {
+      const tag = (tray?.tray_uuid || tray?.tag_uid || getFallbackSpoolTag(printerSerial, amsId, trayId))?.toUpperCase();
+      const linkedSpool = tag ? linkedSpools[tag] : undefined;
+      const tagFill = getSpoolmanFillLevel(linkedSpool);
+      if (tagFill !== null) return tagFill;
+    }
     return null;
   }, [linkedSpools, printerSerial, selectedPrinterId, spoolmanSlotAssignmentsAll, spoolmanInventorySpoolsCache]);
 
@@ -757,7 +759,11 @@ export function SpoolBuddyAmsPage() {
                     </div>
                   </div>
                 )}
-                {spoolmanEnabled && linked && (
+                {/* #1457: Assigned-spool block is rendered FIRST when a slot
+                    assignment exists, regardless of whether a (possibly stale)
+                    tag-link also exists. The tag-link block is the fallback
+                    for slots that have only a tag-link. */}
+                {spoolmanEnabled && linked && !spoolmanAssignedSpool && (
                   <div className="p-2.5 bg-bambu-dark rounded-lg border border-bambu-dark-tertiary mb-3">
                     <p className="text-xs text-bambu-gray mb-1">{t('spoolman.linkedSpool', 'Linked spool')}</p>
                     <div className="flex items-center gap-2">
@@ -768,7 +774,7 @@ export function SpoolBuddyAmsPage() {
                     </div>
                   </div>
                 )}
-                {spoolmanEnabled && !linked && spoolmanAssignedSpool && (
+                {spoolmanEnabled && spoolmanAssignedSpool && (
                   <div className="p-2.5 bg-bambu-dark rounded-lg border border-bambu-dark-tertiary mb-3">
                     <p className="text-xs text-bambu-gray mb-1">{t('inventory.assignedSpool', 'Assigned spool')}</p>
                     <div className="flex items-center gap-2">

File diff suppressed because it is too large
+ 0 - 0
static/assets/index-CMHVQrcW.js


+ 1 - 1
static/index.html

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

Some files were not shown because too many files changed in this diff