From 568835c586ffbbb2bd41911b2dac9d8d5f7c0792 Mon Sep 17 00:00:00 2001
From: maziggy <mz@v8w.de>
Date: Sat, 25 Apr 2026 12:49:00 +0200
Subject: [PATCH]   fix(#918): RFID auto-match handles Quick-Add and rejects
 non-Bambu brands
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

  `find_matching_untagged_spool` is supposed to attach an incoming Bambu
  RFID UUID to a pre-existing manually-logged spool of the same
  material/color so users who log inventory before scanning don't end up
  with duplicate rows. Two bugs meant it almost never worked for the
  actual reporting workflow:

  1. Subtype filter was strict. AMS reports `tray_sub_brands="PLA Basic"`
     → matcher required `Spool.subtype = 'Basic'` exactly. The form's
     Quick-Add mode only requires `material`, so bulk-logged rows have
     `subtype=NULL` and were always excluded → duplicate on first AMS
     read.
  2. Brand wasn't filtered. The docstring claimed brand was matched but
     the WHERE clause didn't include it, so a same-color Polymaker (or
     any non-Bambu) untagged row could acquire a Bambu UUID — silent
     data corruption.

  Fix in the same query: subtype prefers exact match but accepts NULL as
  fallback (CASE in ORDER BY ensures exact wins when both exist); brand
  restricted to NULL or LOWER(brand) LIKE '%bambu%' (covers 'Bambu',
  'Bambu Lab', 'BambuLab', 'bambu lab' — the spellings users actually
  type).

  6 regression tests added in test_spool_tag_matcher.py.
---
 CHANGELOG.md                                  |   1 +
 backend/app/services/spool_tag_matcher.py     |  42 ++++-
 .../unit/services/test_spool_tag_matcher.py   | 168 ++++++++++++++++++
 3 files changed, 202 insertions(+), 9 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7f1b9150a..ac0ab3dfc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -24,6 +24,7 @@ All notable changes to Bambuddy will be documented in this file.
 - **Settings page: permission-gated instead of admin-only** — the Settings sidebar entry has always been visible to any user holding `settings:read`, but the route guard required admin role, so a non-admin with `settings:read` would see the entry, click it, and get silently redirected back to the dashboard. The route guard now matches the sidebar: any user with `settings:read` can open the page, and the individual tabs / cards continue to enforce their own per-feature permissions (`users:read`, `groups:update`, `oidc:*`, etc. — many of them admin-only, some not). Group editor routes moved to permission-based guards too (`groups:create` for `/groups/new`, `groups:update` for `/groups/:id/edit`), so permission delegation works end-to-end. Admins retain full access since admins implicitly hold every permission.
 
 ### Fixed
+- **Bambu RFID auto-match created duplicate inventory rows for Quick-Add and non-Bambu-branded spools** ([#918](https://github.com/maziggy/bambuddy/issues/918)) — `find_matching_untagged_spool` is supposed to attach a Bambu RFID UID to a pre-existing manually-logged spool of the same material/color so users who log inventory before scanning don't end up with a duplicate row on first AMS read. Two bugs in the matcher meant it almost never worked for the actual reporting workflow: **(1)** the subtype filter was strict — when the AMS tray reports `tray_sub_brands="PLA Basic"` the matcher required `Spool.subtype = 'Basic'` exactly, so any Quick-Add row (Quick-Add only requires `material`, leaving `subtype=NULL`) was excluded and duplicated on first AMS read. **(2)** the docstring claimed it filtered on brand but the WHERE clause didn't, so a same-color *Polymaker* untagged spool would silently acquire a Bambu Lab tray UUID, leaving the user with `brand="Polymaker"` but a Bambu UUID — silent data corruption. Both bugs are addressed in the same query: subtype now prefers an exact match but accepts a NULL-subtype row as fallback (with a `CASE` in `ORDER BY` so an exact match still wins when both exist), and brand is now restricted to "contains 'bambu' (case-insensitive)" or NULL — matching `'Bambu'` (the form's `DEFAULT_BRANDS` value), `'Bambu Lab'` (the catalog value), `'BambuLab'`, `'bambu lab'`, etc., while rejecting any explicitly-named third-party brand. 6 new regression tests in `test_spool_tag_matcher.py` cover the NULL-subtype fallback, exact-subtype-wins-over-NULL ordering, non-Bambu brand rejection, NULL brand acceptance, all four Bambu brand spelling variants, and the full Quick-Add scenario (`brand=NULL` + `subtype=NULL`). The broader UI proposals in #918 (manual override / merge / disambiguation prompt) are intentionally out of scope — once the matcher works, the duplicate-on-RFID complaint that motivated those proposals goes away. Thanks to @ViridityCorn for the report and pointing at the right function, and to @Arn0uDz for confirming with a 20-spool repro.
 - **Swagger UI link in Settings → API Keys rendered a blank page** — the global CSP applied by `security_headers_middleware` set `script-src 'self'` and `style-src 'self' 'unsafe-inline' https://fonts.googleapis.com`, which blocked both the inline `<script>` that boots Swagger and the `cdn.jsdelivr.net` URL that ships `swagger-ui-bundle.js` / `swagger-ui.css`. FastAPI's `/docs` page therefore loaded a 1 KB shell with no JS executed, leaving an empty white page. The middleware now emits a docs-scoped CSP for `/docs`, `/redoc`, and `/docs/oauth2-redirect` that allows `https://cdn.jsdelivr.net` for scripts + styles, the FastAPI/Redoc favicon hosts for images, and `'unsafe-inline'` for the Swagger boot script — every other route keeps the unchanged stricter SPA policy.
 - **Camera stream second viewer fails / kicks the first off** ([#1089](https://github.com/maziggy/bambuddy/issues/1089)) — Most Bambu Lab printers only allow one concurrent camera connection (RTSP socket on X1/H2/P2, port-6000 chamber-image socket on A1/P1), but `GET /printers/{id}/camera/stream` opened a fresh upstream per viewer keyed on a per-request `stream_id`. Two browser tabs / two dashboard cards → the second viewer either failed silently or kicked the first one off. New `services/camera_fanout.py::MjpegBroadcaster` owns a single upstream per printer and fans pre-formatted MJPEG chunks out to N subscriber queues; new viewers tap the existing connection. When the last subscriber leaves, the upstream stays alive for a 5 s grace window so a tab refresh or "open in new tab" doesn't pay an ffmpeg/RTSP reconnect, then tears down cleanly. Per-subscriber queues are bounded (depth 4) so a slow viewer drops frames for itself rather than blocking the broadcaster — live video, old frames have no value. Stop endpoint and app-shutdown both call into the broadcaster's force-shutdown path so subscribers wake up via an upstream-gone sentinel instead of hanging on `queue.get()`. External-camera path is unchanged (user-supplied MJPEG/RTSP servers handle multi-viewer themselves). The upstream uses a deterministic `{printer_id}-fanout` stream id so every existing prefix-match in `cleanup_orphaned_streams`, `camera_status`, the snapshot fall-through in `main.py`, and the `stop` endpoint continues to find it without changes. Two follow-up correctness fixes from the audit pass: (1) `_stream_start_times[printer_id]` is now set with `setdefault()` so `/camera/status` reports the SHARED upstream's age — previously each new viewer overwrote it, making `stream_uptime` jump backward whenever a second viewer attached; (2) the route now retries `subscribe()` once on `RuntimeError` to close a tiny race where the grace teardown can flip the broadcaster to `stopped` between the registry lookup and the subscribe call (the retry forces the registry to mint a fresh broadcaster). Detach log line shows the post-unsubscribe count returned atomically by `unsubscribe()` — no more two viewers leaving simultaneously both reporting `subscribers=0`. Permission gates unchanged: `/camera/stream` still requires the existing token (minted by `POST /camera/stream-token` with `CAMERA_VIEW`); `/camera/stop` still requires `CAMERA_VIEW`; the broadcaster is internal infra with no FastAPI surface. 13 unit tests for the broadcaster (single subscriber, multi-subscriber-shares-one-pump, slow-subscriber-doesn't-block-fast, grace-window teardown, grace-cancelled-on-rejoin, force-shutdown sentinel, `iter_subscriber` exits on upstream-gone and on client-disconnect, registry replaces stopped broadcasters, `subscribe()` raises on stopped broadcaster, `unsubscribe()` returns post-removal count atomically across concurrent leavers, double-unsubscribe is idempotent, and the route's force-shutdown-then-fresh-subscribe retry path) plus 2 new integration tests on the stop endpoint covering the deterministic fan-out stream id and the `shutdown_broadcaster` wiring. Thanks to @swheettaos for the diagnosis and broadcaster sketch.
 - **Uploads to writable external folders silently landed in internal storage** ([#1112](https://github.com/maziggy/bambuddy/issues/1112)) — `LibraryFolder` has an `external_readonly` flag, so the model already distinguishes writable from read-only external mounts, but `POST /library/files` rejected only the read-only branch and then unconditionally wrote to `get_library_files_dir()` with a UUID-scoped filename. The resulting `LibraryFile` row linked back to the external folder via `folder_id`, so the file showed up in the Bambuddy UI and could be printed, but the bytes physically lived in `archive/library/files/` and never touched the mount — invisible from any other machine accessing the same NAS/SMB share. New `_resolve_upload_destination()` helper detects writable external targets and writes through to `<external_path>/<filename>` (keeping the original filename so the file is recognisable on the mount), with guards for missing/inaccessible path (400), non-writable mount (400), pre-existing filename on the mount (409 — no silent overwrite; the user is expected to rename and retry, matching how scan treats external files as externally-owned bytes), and a `resolve + relative_to` path-traversal guard on the joined destination. DB row now matches what scan produces: `is_external=True`, `file_path=<absolute external path>`, so the existing download / delete / dedupe paths work unchanged (`to_absolute_path` already fast-paths `is_absolute()` inputs, and external-file deletion already bypasses trash and only drops the DB row + internal thumbnail). `POST /library/files/extract-zip` is now rejected against *any* external folder (not just read-only) with a clear "extract the ZIP on the external mount and run Scan" message — the nested-subfolder creation path would need to `mkdir` on the mount and create matching `is_external=True` `LibraryFolder` rows, which is a separate design round, and the Scan flow already handles that shape. 7 new integration tests cover: bytes land on the mount; DB row has `is_external=True` + absolute `file_path`; filename collision → 409 with prior bytes preserved; vanished external path → 400; path-traversal filename never escapes the external dir; extract-zip into writable external rejected with the Scan hint; root uploads unchanged.
diff --git a/backend/app/services/spool_tag_matcher.py b/backend/app/services/spool_tag_matcher.py
index a7b9c3ce2..63e11100f 100644
--- a/backend/app/services/spool_tag_matcher.py
+++ b/backend/app/services/spool_tag_matcher.py
@@ -2,7 +2,7 @@
 
 import logging
 
-from sqlalchemy import func, or_, select
+from sqlalchemy import case, func, or_, select
 from sqlalchemy.ext.asyncio import AsyncSession
 from sqlalchemy.orm import selectinload
 
@@ -191,11 +191,22 @@ async def create_spool_from_tray(db: AsyncSession, tray_data: dict) -> Spool:
 
 
 async def find_matching_untagged_spool(db: AsyncSession, tray_data: dict) -> Spool | None:
-    """Find an existing untagged inventory spool matching brand/material/color.
+    """Find an existing untagged Bambu inventory spool matching material/color.
 
     When a Bambu Lab spool is detected in the AMS but no tag match exists,
     check if the user has a manually-added spool with the same properties
-    that hasn't been linked to a tag yet. Returns the oldest match (FIFO).
+    that hasn't been linked to a tag yet. Returns the best match (#918):
+
+    - **Brand**: only consider spools whose brand is unspecified or contains
+      "bambu" (case-insensitive — covers both "Bambu" and "Bambu Lab" as
+      stored by the form's brand dropdown). This prevents a same-color
+      Polymaker / generic spool from accidentally attracting a Bambu UUID.
+    - **Subtype**: prefer an exact match (e.g. AMS "Basic" → spool subtype
+      "Basic"), but fall back to a NULL-subtype spool — the form's Quick Add
+      mode leaves subtype empty, so bulk-logged spools rely on this fallback
+      to attract their RFID tag instead of duplicating on first AMS read.
+    - **FIFO** within each preference group (user likely logged in purchase
+      order).
     """
     tray_type = tray_data.get("tray_type", "")
     tray_sub_brands = tray_data.get("tray_sub_brands", "")
@@ -229,7 +240,7 @@ async def find_matching_untagged_spool(db: AsyncSession, tray_data: dict) -> Spo
         elif color_code and color_code[0] == "T":
             subtype = "Tri Color"
 
-    # Build query: active spools with no tag, matching brand + material + color
+    # Active, untagged spools matching material + color + Bambu-or-unset brand.
     query = (
         select(Spool)
         .options(selectinload(Spool.k_profiles), selectinload(Spool.assignments))
@@ -239,17 +250,30 @@ async def find_matching_untagged_spool(db: AsyncSession, tray_data: dict) -> Spo
             Spool.tray_uuid.is_(None),
             func.upper(Spool.material) == material.upper(),
             func.upper(Spool.rgba) == tray_color.upper(),
+            or_(
+                Spool.brand.is_(None),
+                func.lower(Spool.brand).like("%bambu%"),
+            ),
         )
     )
 
-    # Match subtype if parsed (e.g. "Basic", "Matte")
     if subtype:
-        query = query.where(func.upper(Spool.subtype) == subtype.upper())
+        # Exact subtype OR NULL fallback. The CASE in ORDER BY ensures an
+        # exact-subtype row beats a NULL-subtype row when both exist; FIFO
+        # within each group.
+        query = query.where(
+            or_(
+                func.upper(Spool.subtype) == subtype.upper(),
+                Spool.subtype.is_(None),
+            )
+        ).order_by(
+            case((func.upper(Spool.subtype) == subtype.upper(), 0), else_=1),
+            Spool.created_at.asc(),
+        )
     else:
-        query = query.where(Spool.subtype.is_(None))
+        query = query.where(Spool.subtype.is_(None)).order_by(Spool.created_at.asc())
 
-    # FIFO: oldest spool first (user likely added in purchase order)
-    query = query.order_by(Spool.created_at.asc()).limit(1)
+    query = query.limit(1)
 
     result = await db.execute(query)
     spool = result.scalar_one_or_none()
diff --git a/backend/tests/unit/services/test_spool_tag_matcher.py b/backend/tests/unit/services/test_spool_tag_matcher.py
index abd43f497..88945b864 100644
--- a/backend/tests/unit/services/test_spool_tag_matcher.py
+++ b/backend/tests/unit/services/test_spool_tag_matcher.py
@@ -690,6 +690,174 @@ async def test_find_matching_untagged_spool_relationships_loaded(db_session):
     assert _relationship_is_loaded(found, "assignments")
 
 
+# -- find_matching_untagged_spool: #918 regressions ------------------------
+
+
+@pytest.mark.asyncio
+async def test_find_matching_untagged_spool_null_subtype_fallback(db_session):
+    """#918: Quick-Add spool (subtype=NULL) matches when AMS reports a subtype.
+
+    The form's Quick-Add mode only requires `material`, so bulk-logged spools
+    have subtype=NULL. Before the fix, the strict `subtype = 'Basic'` filter
+    excluded these rows and the system created duplicates on first AMS read.
+    """
+    spool = Spool(
+        material="PLA",
+        subtype=None,  # Quick-Add bulk entry
+        rgba="FFFFFFFF",
+        brand="Bambu Lab",
+        label_weight=1000,
+        core_weight=250,
+    )
+    db_session.add(spool)
+    await db_session.commit()
+
+    # Tray reports "PLA Basic" → subtype parsed as "Basic"
+    found = await find_matching_untagged_spool(db_session, SAMPLE_TRAY)
+    assert found is not None
+    assert found.id == spool.id
+
+
+@pytest.mark.asyncio
+async def test_find_matching_untagged_spool_prefers_exact_subtype_over_null(db_session):
+    """#918: When both an exact-subtype and a NULL-subtype row match, exact wins.
+
+    The NULL fallback exists only as a backstop for Quick-Add bulk-logged
+    spools — if the user did the work to record subtype="Basic", it must
+    take precedence over a vague "PLA" record, even if the latter is older.
+    """
+    import asyncio
+
+    null_spool = Spool(
+        material="PLA",
+        subtype=None,  # Older but vague — should NOT win
+        rgba="FFFFFFFF",
+        brand="Bambu Lab",
+        label_weight=1000,
+        core_weight=250,
+    )
+    db_session.add(null_spool)
+    await db_session.flush()
+
+    await asyncio.sleep(0.05)
+
+    exact_spool = Spool(
+        material="PLA",
+        subtype="Basic",  # Newer but specific — should win
+        rgba="FFFFFFFF",
+        brand="Bambu Lab",
+        label_weight=1000,
+        core_weight=250,
+    )
+    db_session.add(exact_spool)
+    await db_session.commit()
+
+    found = await find_matching_untagged_spool(db_session, SAMPLE_TRAY)
+    assert found is not None
+    assert found.id == exact_spool.id
+
+
+@pytest.mark.asyncio
+async def test_find_matching_untagged_spool_rejects_non_bambu_brand(db_session):
+    """#918: A same-color non-Bambu spool must NOT attract a Bambu UUID.
+
+    Without the brand filter, a Polymaker untagged spool of matching
+    material/color would silently acquire a Bambu RFID UUID, leaving the
+    user with brand="Polymaker" but a Bambu Lab tray UUID — corrupt data.
+    """
+    spool = Spool(
+        material="PLA",
+        subtype="Basic",
+        rgba="FFFFFFFF",
+        brand="Polymaker",  # NOT Bambu — must be rejected
+        label_weight=1000,
+        core_weight=250,
+    )
+    db_session.add(spool)
+    await db_session.commit()
+
+    found = await find_matching_untagged_spool(db_session, SAMPLE_TRAY)
+    assert found is None
+
+
+@pytest.mark.asyncio
+async def test_find_matching_untagged_spool_accepts_null_brand(db_session):
+    """#918: Quick-Add spools with brand=NULL still match a Bambu RFID read.
+
+    Quick-Add doesn't require brand, so a user bulk-logging Bambu spools may
+    leave it empty. The matcher allows NULL brand because the alternative
+    (forcing every Quick-Add spool to be tagged "Bambu") is the exact
+    friction the auto-matcher exists to remove.
+    """
+    spool = Spool(
+        material="PLA",
+        subtype="Basic",
+        rgba="FFFFFFFF",
+        brand=None,  # Quick-Add left brand blank
+        label_weight=1000,
+        core_weight=250,
+    )
+    db_session.add(spool)
+    await db_session.commit()
+
+    found = await find_matching_untagged_spool(db_session, SAMPLE_TRAY)
+    assert found is not None
+    assert found.id == spool.id
+
+
+@pytest.mark.asyncio
+async def test_find_matching_untagged_spool_accepts_bambu_brand_variants(db_session):
+    """#918: Both 'Bambu' (form dropdown) and 'Bambu Lab' (catalog) match.
+
+    DEFAULT_BRANDS in the form lists 'Bambu'; the catalog uses 'Bambu Lab'.
+    Users can pick either. The fuzzy %bambu% LIKE handles both, plus
+    'BambuLab', 'bambu lab', etc.
+    """
+    for brand_value in ("Bambu", "Bambu Lab", "BambuLab", "bambu lab"):
+        spool = Spool(
+            material="PLA",
+            subtype="Basic",
+            rgba="FFFFFFFF",
+            brand=brand_value,
+            label_weight=1000,
+            core_weight=250,
+        )
+        db_session.add(spool)
+        await db_session.commit()
+
+        found = await find_matching_untagged_spool(db_session, SAMPLE_TRAY)
+        assert found is not None, f"brand={brand_value!r} should match"
+        assert found.id == spool.id
+
+        # Clean up so the next iteration starts fresh.
+        await db_session.delete(spool)
+        await db_session.commit()
+
+
+@pytest.mark.asyncio
+async def test_find_matching_untagged_spool_null_subtype_with_null_brand(db_session):
+    """#918: Pure Quick-Add row (brand=NULL, subtype=NULL) matches.
+
+    This is the exact scenario from Arn0uDz's report: 20 spools logged via
+    Quick Add, then placed in the AMS one at a time. Before the fix every
+    insertion duplicated; after the fix the first matching row is reused.
+    """
+    spool = Spool(
+        material="PLA",
+        subtype=None,
+        rgba="FFFFFFFF",
+        brand=None,
+        label_weight=1000,
+        core_weight=250,
+    )
+    db_session.add(spool)
+    await db_session.commit()
+
+    found = await find_matching_untagged_spool(db_session, SAMPLE_TRAY)
+    assert found is not None
+    assert found.id == spool.id
+
+
 # -- link_tag_to_inventory_spool -------------------------------------------