fix(spoolbuddy): writing a tag takes it off the spool that held it before (#3312)

- write-result cleared the previous holder with {"tag": ""}; Spoolman
  stores extra values as JSON and refused it with HTTP 400, so both
  spools kept the tag and the next scan could resolve to the old one.
- The cleanup now uses _clear_stale_tag_links, as Assign and Link do:
  JSON-encoded, and on every other spool holding the tag. The old
  lookup found only the first holder, and returned the target itself
  when it already held the tag as a native Spoolman 0.27 tag.

CHANGELOG.md

@@ -174,6 +174,7 @@ All notable changes to Bambuddy will be documented in this file.
 - **The frontend build no longer warns about `path` and `crypto` being externalized for the STEP previewer (#2976)** — `occt-import-js`, the Emscripten build behind STEP previews, requires both modules, but only inside its `ENVIRONMENT_IS_NODE` branches; in the browser it loads its `.wasm` from the URL the preview worker passes and draws randomness from `crypto.getRandomValues`. Vite still externalized both and printed two warnings on every build. `vite.config.ts` now drops exactly those two warnings for that one package through `build.rolldownOptions.onLog`, so an externalization anywhere else, or of any other module, still shows.
 
 ### Fixed
+- **Writing a SpoolBuddy tag takes it off the spool that held it before (#3312, reported by @sgiffhorn)** — With Spoolman, writing a tag that another spool already carried was meant to clear it from that spool first, but Spoolman refused the empty value Bambuddy sent. Both spools kept the tag, and the next scan could show the old spool. The tag is now cleared properly, from every other spool that carries it, including when the spool being written already held it as a Spoolman 0.27 tag.
 - **A large 3MF that times out at print start is not downloaded a second time (#3308, reported by @elizabethburg)** — On a slow link, such as an A1 in its start-up routine, a 3MF too large to finish in time was downloaded twice: once by name, and again when the folder listing found the same file. Each attempt took up to 5 minutes, holding the printer's only file connection with the cover images queued behind it. It is now tried once. The archive starts without the 3MF and the recovery retry fills it in, with up to 15 minutes for a large file.
 - **"Print again" on the printer's screen is archived with its 3MF (#3307, reported by @elizabethburg)** — A print that archived in full the first time got an empty archive (no 3MF, thumbnail or filament) when started again with **Print again** on the printer. On a reprint the printer doesn't name the file on the card: it reports its own internal copy under a generic name, plus the model's title from inside the 3MF. When that title differs from the file name, as with a project saved under another name, Bambuddy couldn't find the file.
   - **Now:** when the file can't be found, Bambuddy looks for an earlier archive of the same job on the same printer and archives the new run with that 3MF. The earlier run must have been sent from the slicer or started on the printer, with the same title and plate, and the same layer count when the printer has already reported one. The log names the archive the 3MF came from.

backend/app/api/routes/spoolbuddy.py

@@ -14,6 +14,7 @@ from fastapi import APIRouter, Depends, HTTPException
 from sqlalchemy import func, select
 from sqlalchemy.ext.asyncio import AsyncSession
 
+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
@@ -833,28 +834,13 @@ async def nfc_write_result(
                 # one Spoolman spool, otherwise find_spool_by_tag returns
                 # whichever spool comes first in the cached list (usually the
                 # older one) and the dashboard shows the wrong spool when the
-                # tag is scanned. Before binding the new owner, clear the tag
-                # from any other spool that currently has it. Best-effort:
-                # cleanup failure does not block the write itself, but the
-                # warning surfaces in logs so a stale duplicate can be tracked
-                # down manually.
+                # tag is scanned. Before binding the new owner, clear extra.tag
+                # on every other spool that holds it -- JSON-encoded, as Spoolman
+                # refuses a bare "" (#3312). Best-effort: cleanup failure does
+                # not block the write itself; each failed spool is logged.
                 try:
-                    cached_spools = await sm_client.get_spools()
-                    duplicate = await sm_client.find_spool_by_tag(req.tag_uid, cached_spools=cached_spools)
-                    if duplicate is not None and duplicate.get("id") != req.spool_id:
-                        await sm_client.merge_spool_extra(int(duplicate["id"]), {"tag": ""})
-                        logger.info(
-                            "Spoolman: cleared tag %s from previous holder spool %d before binding to spool %d",
-                            req.tag_uid,
-                            duplicate["id"],
-                            req.spool_id,
-                        )
-                except (SpoolmanNotFoundError, SpoolmanUnavailableError, SpoolmanClientError) as cleanup_exc:
-                    logger.warning(
-                        "Spoolman: failed to clear duplicate tag %s before binding to spool %d (proceeding anyway): %s",
-                        req.tag_uid,
-                        req.spool_id,
-                        cleanup_exc,
+                    await _clear_stale_tag_links(
+                        sm_client, tag=req.tag_uid, keep_spool_id=req.spool_id, log_context="SpoolBuddy tag write"
                     )
                 except Exception:
                     logger.exception(

backend/tests/integration/test_spoolbuddy.py

@@ -552,12 +552,13 @@ class TestNfcEndpoints:
 
         assert resp.status_code == 200
         # merge_spool_extra was called twice:
-        #   1. clear tag from spool A (id=11) — set tag to ""
+        #   1. clear tag from spool A (id=11) — JSON-encoded "", as Spoolman
+        #      refuses a bare "" (#3312)
         #   2. set tag on spool B (id=22) — set tag to "DEADBEEF" (JSON-encoded)
         assert mock_client.merge_spool_extra.await_count == 2
         clear_call, bind_call = mock_client.merge_spool_extra.await_args_list
         assert clear_call.args[0] == 11
-        assert clear_call.args[1] == {"tag": ""}
+        assert clear_call.args[1] == {"tag": _json.dumps("")}
         assert bind_call.args[0] == 22
         assert bind_call.args[1] == {"tag": '"DEADBEEF"'}
 

backend/tests/integration/test_spoolbuddy_write_clears_previous_tag_3312.py

@@ -0,0 +1,156 @@
+"""A SpoolBuddy tag write takes the tag off every other spool (#3312).
+
+Before binding the written UID to the target spool, write-result cleared it
+from the previous holder with ``{"tag": ""}``. Spoolman stores ``extra``
+values as JSON and answers a bare ``""`` with HTTP 400; the handler logged a
+warning and bound the tag anyway, so two spools carried it and the next scan
+could resolve to the old one. The lookup it used also found only the first
+holder, and on Spoolman 0.27+ asked native tags first -- so when the target
+already held the tag natively, the old ``extra.tag`` holder was never looked
+at.
+
+The fake Spoolman below refuses a non-JSON ``extra`` value the way the real
+one does.
+"""
+
+import json
+from datetime import datetime, timezone
+from unittest.mock import AsyncMock, patch
+
+import pytest
+from httpx import AsyncClient
+
+from backend.app.models.settings import Settings
+from backend.app.models.spoolbuddy_device import SpoolBuddyDevice
+from backend.app.services.spoolman import SpoolmanClient, SpoolmanClientError
+
+pytestmark = [pytest.mark.asyncio, pytest.mark.integration]
+
+API = "/api/v1/spoolbuddy"
+UID = "04DBD727B62A81"
+TARGET = 3
+
+
+class FakeSpoolman:
+    """In-memory Spoolman: spools by id, ``extra`` checked like the server does."""
+
+    def __init__(self, spools, *, tag_api=False):
+        self.spools = {s["id"]: s for s in spools}
+        self.tag_api = tag_api
+        self.writes: list[tuple[int, dict]] = []
+
+    async def get_spools(self):
+        return [dict(s) for s in self.spools.values()]
+
+    async def get_spool(self, spool_id):
+        return dict(self.spools[spool_id])
+
+    async def has_tag_api(self):
+        return self.tag_api
+
+    async def find_spool_by_tag(self, tag_uid, cached_spools=None):
+        return await SpoolmanClient.find_spool_by_tag(self, tag_uid, cached_spools=cached_spools)  # type: ignore[arg-type]
+
+    async def link_native_tag(self, spool_id, uid):
+        self.spools[spool_id].setdefault("tags", []).append({"uid": uid})
+        return None
+
+    async def unlink_native_tag(self, spool_id, uid):
+        self.spools[spool_id]["tags"] = [t for t in self.spools[spool_id].get("tags", []) if t["uid"] != uid]
+
+    async def merge_spool_extra(self, spool_id, new_fields):
+        self.writes.append((spool_id, dict(new_fields)))
+        for value in new_fields.values():
+            try:
+                json.loads(value)
+            except (TypeError, ValueError):
+                raise SpoolmanClientError(f"Spoolman rejected update for spool {spool_id} (HTTP 400)", 400) from None
+        spool = self.spools[spool_id]
+        spool["extra"] = {**(spool.get("extra") or {}), **new_fields}
+        return dict(spool)
+
+    def holders(self):
+        """Ids of the spools whose extra.tag still names UID."""
+        return sorted(
+            sid for sid, s in self.spools.items() if (s.get("extra") or {}).get("tag", "").strip('"').upper() == UID
+        )
+
+
+def _spool(spool_id, *, tag=None, native=()):
+    return {
+        "id": spool_id,
+        "filament": {"material": "PLA", "name": "PLA Basic", "color_hex": "000000", "weight": 1000.0},
+        "extra": {"tag": json.dumps(tag)} if tag is not None else {},
+        "tags": [{"uid": u} for u in native],
+    }
+
+
+async def _write(async_client: AsyncClient, db_session, fake: FakeSpoolman):
+    db_session.add(Settings(key="spoolman_enabled", value="true"))
+    db_session.add(Settings(key="spoolman_url", value="http://127.0.0.1:7912"))
+    db_session.add(
+        SpoolBuddyDevice(
+            device_id="sb-3312",
+            hostname="spoolbuddy",
+            ip_address="10.0.0.9",
+            firmware_version="1.0.0",
+            has_nfc=True,
+            has_scale=True,
+            tare_offset=0,
+            calibration_factor=1.0,
+            last_seen=datetime.now(timezone.utc),
+            pending_command="write_tag",
+            pending_write_payload=json.dumps({"spool_id": TARGET, "ndef_data_hex": "DEAD", "data_origin": "spoolman"}),
+        )
+    )
+    await db_session.commit()
+    with (
+        patch("backend.app.api.routes.spoolbuddy.ws_manager") as ws,
+        patch("backend.app.api.routes.spoolbuddy._get_spoolman_client_or_none", new=AsyncMock(return_value=fake)),
+    ):
+        ws.broadcast = AsyncMock()
+        resp = await async_client.post(
+            f"{API}/nfc/write-result",
+            json={"device_id": "sb-3312", "spool_id": TARGET, "tag_uid": UID.lower(), "success": True},
+        )
+    assert resp.status_code == 200, resp.text
+    return resp
+
+
+async def test_the_reported_case_leaves_one_holder(async_client: AsyncClient, db_session):
+    fake = FakeSpoolman([_spool(1, tag=UID), _spool(TARGET)])
+    await _write(async_client, db_session, fake)
+    assert fake.holders() == [TARGET]
+    assert fake.spools[1]["extra"]["tag"] == json.dumps("")
+    # The target is written once, with its new tag -- never cleared first.
+    assert [w for w in fake.writes if w[0] == TARGET] == [(TARGET, {"tag": json.dumps(UID)})]
+
+
+async def test_every_previous_holder_is_cleared(async_client: AsyncClient, db_session):
+    fake = FakeSpoolman([_spool(1, tag=UID), _spool(2, tag=UID.lower()), _spool(TARGET)])
+    await _write(async_client, db_session, fake)
+    assert fake.holders() == [TARGET]
+
+
+async def test_cleared_when_the_target_already_holds_the_tag_natively(async_client: AsyncClient, db_session):
+    fake = FakeSpoolman([_spool(1, tag=UID), _spool(TARGET, native=(UID,))], tag_api=True)
+    await _write(async_client, db_session, fake)
+    assert fake.holders() == [TARGET]
+
+
+async def test_other_spools_and_other_extra_fields_are_left_alone(async_client: AsyncClient, db_session):
+    other = _spool(2, tag="AABBCCDD")
+    other["extra"]["note"] = json.dumps("keep")
+    first = _spool(1, tag=UID)
+    first["extra"]["note"] = json.dumps("mine")
+    fake = FakeSpoolman([first, other, _spool(TARGET)])
+    await _write(async_client, db_session, fake)
+    assert fake.spools[2]["extra"] == {"tag": json.dumps("AABBCCDD"), "note": json.dumps("keep")}
+    assert fake.spools[1]["extra"]["note"] == json.dumps("mine")
+
+
+async def test_rewriting_the_targets_own_tag_does_not_clear_it_first(async_client: AsyncClient, db_session):
+    fake = FakeSpoolman([_spool(1, tag=UID), _spool(TARGET, tag=UID)])
+    await _write(async_client, db_session, fake)
+    assert fake.holders() == [TARGET]
+    assert [w for w in fake.writes if w[0] == TARGET] == [(TARGET, {"tag": json.dumps(UID)})]