Przeglądaj źródła

fix(inventory): broadcast assignment change when auto-unlink clears a stale slot (#2575)

The #2575 reconciliation correctly deletes a stale external-spool
assignment in on_ams_change, but did so silently: spool_assignment_changed
was only broadcast by the manual REST assign/unassign endpoints, and the
frontend's spool-assignments cache is invalidated only by that event. So
after an external-spool type swap the DB was correct but every open browser
kept rendering the unlinked spool on the slot until an unrelated refetch —
which the reporter read as "the fix didn't work" (a browser refresh showed
the right state all along).

Broadcast spool_assignment_changed for each auto-unlinked slot after the
commit. No frontend change — the handler already invalidates the cache.
maziggy 1 miesiąc temu
rodzic
commit
da128e50c7

+ 1 - 1
CHANGELOG.md

@@ -8,7 +8,7 @@ All notable changes to Bambuddy will be documented in this file.
 - **P2S RTSP timeout could leave the fan-out camera stream permanently stalled (#2580, reported and diagnosed by @ronaldheft, fix shape from PR #2581)** — After an RTSP read timeout, the stream cleanup killed the stalled ffmpeg and then waited *unbounded* for it to be reaped. A SIGKILLed ffmpeg stuck in uninterruptible I/O on a dead RTSP socket can take arbitrarily long to exit, so the fan-out stream coroutine sat parked in that wait — in the reported case for 12 hours — while every new viewer attached to the stalled broadcaster and got no frames (snapshots and diagnostics kept working, since those open fresh connections). The post-kill wait is now bounded (2 s): on timeout the stream abandons the zombie — the orphan janitor's /proc scan reaps it on its next pass — and proceeds to its normal reconnect, so live view recovers by itself. The same unbounded wait hid in two more places, both bounded too: the camera *Stop* endpoint (which would hang the very request a user makes to recover a stuck stream) and the periodic orphan-cleanup janitor itself (which is the safety net that recovers stalled streams, and so can least afford to block).
 - **Queue edit showed the sliced-for model as the scheduler target, and a cross-model queue row could dispatch G-code to an incompatible printer (#2578, reporter @Jostxxl)** — Two bugs with one root. The "Any \<model\>" assignment button labeled itself from the file's slice metadata while the scheduler actually used the row's `target_model`, so an X1C-sliced item targeting H2D read "Any X1C" above "Scheduler will assign to first available idle H2D printer". Worse, the mismatch could be *created* silently: the sliced-for model loads asynchronously, and clicking "Any Model" before it arrived pre-selected the first model alphabetically — on a mixed X1C/P1S/H2D farm that's H2D — after which the model dropdown hid itself, leaving no way to see or fix the wrong target. Nothing downstream checked compatibility, so the scheduler would happily hand X1C G-code to an H2D. Now: the target model is never silently defaulted (the dropdown stays visible in model mode, pre-selected to the sliced-for model when available, and back-fills once the metadata loads); the button reflects the actual target; a warning shows when the target differs from the sliced-for model. Compatibility is enforced end-to-end with an explicit G-code interchange family table (X1/X1C/X1E/P1P/P1S interchange; everything else exact-match — files without slice metadata are never blocked): incompatible models are disabled in the dropdown, queue create/update reject a mismatch with a clear 400 (so API-created rows can't sneak in), and the scheduler holds back pre-existing mismatched rows with an actionable waiting reason instead of dispatching them — fix the target via edit and the job flows again.
 - **Manual jog could drive an axis past its travel limit into a collision (#2579, reporter @R3play210)** — Jog the bed up from Bambuddy and, instead of stopping at the travel limit, it keeps going until the nozzle hits the plate; X/Y overrun too, on every model. Instrumenting the exact bytes sent to an H2D showed Bambuddy issuing a clean, correct move at the limit — `G91` / `G1 Z-1.00 F600` / `G90`, no endstop manipulation — that the printer executed straight past the stop, while the machine's **own touchscreen refuses the identical motion**. **This is a Bambu firmware bug: the firmware does not enforce its soft endstops on G-code received over MQTT** (the path every remote tool, Bambuddy included, must use), and it reports no axis position, so Bambuddy cannot know where the bed is to stop it either. It is not fixable from our side. Two things change here: (1) the jog no longer wraps moves in `M211 S0`/`S1` — the old code disabled the firmware's soft endstops *globally* around every jog, which also broke the **touchscreen's** limits until the printer was power-cycled; it now sends a bare move and never touches `M211`, so the touchscreen stays protected. (2) The jog panel now shows a prominent warning that travel limits are **not** enforced during manual moves because of this firmware bug, so nobody trusts the control to stop at the limit. Client-side travel-limit enforcement (dead-reckoning from a home) is tracked separately as the only real mitigation. If your printer currently overruns even from its touchscreen, power-cycle it once to restore the endstops an older Bambuddy build disabled.
-- **External spool kept its old inventory filament after the type was changed on the printer (#2575, reporter @ajbastien)** — Assigning a new filament to the external spool (e.g. generic ABS in place of generic TPU) left the previous inventory spool assigned, so an ABS spool stayed mapped to TPU. The reconciliation that unlinks a stale external-spool assignment lives in `on_ams_change`, but that callback only fired on changes to the regular AMS units — its change-hash never included the external spool (`vt_tray`/`vir_slot`), and the external-spool data is stored after the AMS handler runs. External-spool identity changes (type, colour, tag, or a reset to empty) now re-trigger the callback so the stale assignment is unlinked; the fill-percentage (`remain`) is deliberately excluded from the fingerprint so a running print doesn't fire it on every push.
+- **External spool kept its old inventory filament after the type was changed on the printer (#2575, reporter @ajbastien)** — Assigning a new filament to the external spool (e.g. generic ABS in place of generic TPU) left the previous inventory spool assigned, so an ABS spool stayed mapped to TPU. The reconciliation that unlinks a stale external-spool assignment lives in `on_ams_change`, but that callback only fired on changes to the regular AMS units — its change-hash never included the external spool (`vt_tray`/`vir_slot`), and the external-spool data is stored after the AMS handler runs. External-spool identity changes (type, colour, tag, or a reset to empty) now re-trigger the callback so the stale assignment is unlinked; the fill-percentage (`remain`) is deliberately excluded from the fingerprint so a running print doesn't fire it on every push. Follow-up: the auto-unlink now also broadcasts `spool_assignment_changed` for each cleared slot — previously only the manual assign/unassign endpoints did, so an open browser kept rendering the now-unlinked spool on the slot until an unrelated refetch, which read as "the fix didn't work" even though the server state was already correct (reporter confirmed a browser refresh showed the right state all along).
 - **Progress-milestone and HMS-error notifications held a DB connection across the camera snapshot (#2572, reporter @Jostxxl)** — Both notification paths inside `on_printer_status_change` (the 25/50/75% milestone push and the new-HMS-error push) opened a database session, then captured a camera snapshot for the notification image — an up-to-15s RTSP grab — and sent the notification, all with the session held. So a pooled connection sat idle for the whole grab, per milestone/error, per printer; on a farm those fire constantly. The snapshot needs no database, so it now runs between two short sessions: one to read the printer name, then the grab with no connection held, then a fresh session for the notification send. Behaviour is unchanged; pinned by a test that fails if the snapshot ever runs while a session is open. The AMS-change notification path was left as-is for now (it holds a per-printer lock across its write and needs separate care). Continues the #2572 effort (camera stream, timelapse scan, finish photo).
 - **Finish-photo capture held a DB connection open across the whole camera grab (#2572, reporter @Jostxxl)** — When a print finishes, the background finish-photo task reads a couple of rows (the capture setting, the printer, the archive) and then runs a capture pipeline that can take tens of seconds — timelapse last-frame extraction, waiting up to 20s for the stage-22 producer, an external-camera HTTP grab, or a fresh RTSP shot. It held one database session open across that entire pipeline, so a pooled connection sat `idle in transaction` for the full capture, once per finishing print — and finishes cluster on a farm. It now reads what it needs in a short session, releases the connection, runs the capture with no session held, and re-opens a fresh short session only to append the photo to the archive. Behaviour is unchanged. Continues the #2572 effort (camera stream, timelapse scan) to stop holding sessions across slow I/O.
 - **Timelapse scan held a DB connection open across every FTP round-trip (#2572, reporter @Jostxxl)** — After a print completes, `_scan_for_timelapse_with_retries` polls the printer's FTP server for the new timelapse file (up to 4 retry attempts, plus a name-match fallback). Each attempt opened one database session and held it across the FTP directory listing *and* the multi-MB video download — so a pooled connection sat `idle in transaction` for the whole transfer, once per attempt, per completed print. When several prints finish together on a farm that adds up. The scan now reads the archive + printer in a short session, releases the connection, does the FTP list/download with no session held, and re-opens a fresh short session only to attach the downloaded file. Behaviour is unchanged; the existing scan tests already exercise the read→download→attach path. Continues the #2572 effort (after the camera-stream fix) to stop holding sessions across slow I/O; the scheduler paths were reviewed and found already bounded (single loop + capped concurrent uploads, with an explicit pre-dispatch commit) so they were left as-is.

+ 18 - 0
backend/app/main.py

@@ -1608,12 +1608,30 @@ async def on_ams_change(printer_id: int, ams_data: list):
                             spool.material if spool else "?",
                         )
                         stale.append(assignment)  # Spool changed
+            # Snapshot slots before delete — ORM attribute access after the
+            # commit would refresh against a deleted row.
+            unlinked_slots = [(a.ams_id, a.tray_id) for a in stale]
             for a in stale:
                 await db.delete(a)
             if stale:
                 logger.info("Auto-unlinked %d stale spool assignments for printer %d", len(stale), printer_id)
             # Commit any changes (stale deletions and/or fingerprint updates)
             await db.commit()
+            # Tell open browsers the assignment is gone (#2575). Only the manual
+            # REST assign/unassign endpoints broadcast this event; without it the
+            # frontend's spool-assignments cache keeps rendering the unlinked
+            # spool on the slot until an unrelated refetch — which reads exactly
+            # like "the fix didn't work" (reporter verified: a browser refresh
+            # after the swap showed the correct state all along).
+            for ams_id, tray_id in unlinked_slots:
+                await ws_manager.broadcast(
+                    {
+                        "type": "spool_assignment_changed",
+                        "printer_id": printer_id,
+                        "ams_id": ams_id,
+                        "tray_id": tray_id,
+                    }
+                )
     except Exception as e:
         logger.warning("Spool assignment cleanup failed: %s", e, exc_info=True)
 

+ 83 - 0
backend/tests/integration/test_inventory_assign.py

@@ -885,6 +885,89 @@ class TestAssignSpoolEmptySlotPreConfig:
         await db_session.refresh(pre_assignment)
         assert pre_assignment.fingerprint_type == "PLA"
 
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_auto_unlink_broadcasts_assignment_change(
+        self, async_client: AsyncClient, printer_factory, spool_factory, db_session: AsyncSession
+    ):
+        """#2575 follow-up: when on_ams_change auto-unlinks a stale external-spool
+        assignment, it must broadcast spool_assignment_changed. Only the manual
+        REST endpoints did, so open browsers kept rendering the unlinked spool —
+        the reporter read that as "the fix didn't work" when the DB was correct."""
+        from unittest.mock import AsyncMock
+
+        from sqlalchemy import select
+
+        from backend.app.main import on_ams_change
+        from backend.app.models.spool_assignment import SpoolAssignment
+
+        printer = await printer_factory(name="X1C")
+        spool = await spool_factory(slicer_filament="GFU01", material="TPU")
+
+        # TPU inventory spool assigned to the external slot (ams_id=255, tray 0)
+        assignment = SpoolAssignment(
+            spool_id=spool.id,
+            printer_id=printer.id,
+            ams_id=255,
+            tray_id=0,
+            fingerprint_color="000000FF",
+            fingerprint_type="TPU",
+        )
+        db_session.add(assignment)
+        await db_session.commit()
+
+        # The printer's external spool now reports ABS — the assignment is stale.
+        vt_tray = [
+            {
+                "id": "254",
+                "tray_type": "ABS",
+                "tray_color": "000000FF",
+                "tag_uid": "0000000000000000",
+                "tray_uuid": "00000000000000000000000000000000",
+            }
+        ]
+        status = _make_mock_status(ams_data=[], vt_tray=vt_tray)
+        printer_info = MagicMock(name="X1C", serial_number="00M00A391800004")
+
+        with (
+            patch("backend.app.main.printer_manager") as mock_pm_main,
+            patch("backend.app.services.printer_manager.printer_manager") as mock_pm_inv,
+            patch("backend.app.main.mqtt_relay") as mock_relay,
+            patch("backend.app.main.ws_manager") as mock_ws,
+        ):
+            mock_pm_main.get_printer.return_value = printer_info
+            mock_pm_main.get_status.return_value = status
+            mock_pm_main.get_client.return_value = MagicMock()
+            mock_pm_main.get_model.return_value = "X1C"
+            mock_pm_inv.get_client.return_value = MagicMock()
+            mock_pm_inv.get_status.return_value = status
+            mock_relay.on_ams_change = AsyncMock()
+            mock_ws.send_printer_status = AsyncMock()
+            mock_ws.broadcast = AsyncMock()
+
+            await on_ams_change(printer.id, [])
+
+            # The stale TPU assignment on the now-ABS external slot was unlinked...
+            gone = await db_session.execute(
+                select(SpoolAssignment).where(
+                    SpoolAssignment.printer_id == printer.id,
+                    SpoolAssignment.ams_id == 255,
+                    SpoolAssignment.tray_id == 0,
+                )
+            )
+            assert gone.scalar_one_or_none() is None
+
+            # ...and the frontend was told about it.
+            change_events = [
+                c.args[0]
+                for c in mock_ws.broadcast.await_args_list
+                if c.args and isinstance(c.args[0], dict) and c.args[0].get("type") == "spool_assignment_changed"
+            ]
+            assert change_events, "auto-unlink must broadcast spool_assignment_changed"
+            assert change_events[0]["printer_id"] == printer.id
+            assert change_events[0]["ams_id"] == 255
+            assert change_events[0]["tray_id"] == 0
+
 
 class TestAssignSpoolEmptyDetection:
     """Bambu firmware reports tray.state — 11=loaded, 9=empty, 10=spool present