diff --git a/backend/app/api/routes/printers.py b/backend/app/api/routes/printers.py index d2b76b953..9eb1203da 100644 --- a/backend/app/api/routes/printers.py +++ b/backend/app/api/routes/printers.py @@ -2935,15 +2935,27 @@ async def configure_ams_slot( # tell, and on those machines extruder 0 is the only one there is. kp_extruder = resolved_extruder if resolved_extruder is not None else 0 - # Spoolman SlotAssignment first — has UniqueConstraint, idempotent. - sm_result = await db.execute( - select(SpoolmanSlotAssignment).where( - SpoolmanSlotAssignment.printer_id == printer_id, - SpoolmanSlotAssignment.ams_id == ams_id, - SpoolmanSlotAssignment.tray_id == tray_id, + # Only the active mode's assignment table decides where this + # K-profile is stored. Reading Spoolman first and falling through + # was safe while the inactive table was emptied on every mode + # toggle; nothing is emptied since #2812, so a leftover Spoolman + # row in built-in mode would file the calibration against a spool + # the printer is not using and never write the local profile — + # the calibration would appear to succeed and then not apply. + from backend.app.services.inventory_mode import spoolman_owns_assignments + + spoolman_mode = await spoolman_owns_assignments(db) + sm_assignment = None + if spoolman_mode: + # Spoolman SlotAssignment — has UniqueConstraint, idempotent. + sm_result = await db.execute( + select(SpoolmanSlotAssignment).where( + SpoolmanSlotAssignment.printer_id == printer_id, + SpoolmanSlotAssignment.ams_id == ams_id, + SpoolmanSlotAssignment.tray_id == tray_id, + ) ) - ) - sm_assignment = sm_result.scalar_one_or_none() + sm_assignment = sm_result.scalar_one_or_none() if sm_assignment: existing = await db.execute( select(SpoolmanKProfile).where( @@ -2981,8 +2993,11 @@ async def configure_ams_slot( tray_id, cali_idx, ) - else: - # Local SpoolAssignment + SpoolKProfile (no UNIQUE — use .first()) + elif not spoolman_mode: + # Local SpoolAssignment + SpoolKProfile (no UNIQUE — use .first()). + # Skipped in Spoolman mode even when a local row survives: the + # profile would be filed against a spool this printer is not + # drawing on, and the mode's own table has nothing to bind to. local_result = await db.execute( select(SpoolAssignment) .options(selectinload(SpoolAssignment.spool)) diff --git a/backend/app/api/routes/settings.py b/backend/app/api/routes/settings.py index cb511c3ed..a5155f953 100644 --- a/backend/app/api/routes/settings.py +++ b/backend/app/api/routes/settings.py @@ -8,7 +8,7 @@ from pathlib import Path from fastapi import APIRouter, Depends, File, HTTPException, UploadFile from fastapi.responses import FileResponse, JSONResponse from pydantic import BaseModel, Field -from sqlalchemy import delete, func, select +from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession from backend.app.core.auth import RequirePermissionIfAuthEnabled, caller_is_api_key, require_energy_cost_update @@ -552,22 +552,24 @@ async def update_spoolman_settings( now_enabled = new_val == "true" await set_setting(db, "spoolman_enabled", new_val) - # Switching to Spoolman: clear built-in inventory slot assignments - if not was_enabled and now_enabled: - from backend.app.models.spool_assignment import SpoolAssignment - - result = await db.execute(delete(SpoolAssignment)) - logger.info("Cleared %d spool assignments on switch to Spoolman mode", result.rowcount) - # Switching back to internal mode: clear Spoolman slot assignments — the - # symmetric counterpart of the clear above. Without this, stale - # spoolman_slot_assignments rows linger and would wrongly count as - # "assigned" in any mode-agnostic check (e.g. the missing-spool- - # assignment notification, which unions both tables — #1473). - elif was_enabled and not now_enabled: - from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment - - result = await db.execute(delete(SpoolmanSlotAssignment)) - logger.info("Cleared %d Spoolman slot assignments on switch to internal mode", result.rowcount) + # Nothing is deleted on a mode change (#2812). Each mode keeps its slot + # assignments in its own table, so both can hold rows at once and the + # toggle is reversible: switching to Spoolman to see what it does, then + # switching back, returns you to the assignments you had. + # + # This used to empty the other mode's table on every toggle. The reason + # was real -- checks that read both tables would let a row in the mode + # you are not using answer for the mode you are -- but the cost was that + # inspecting a mode destroyed your configuration, with no confirmation + # and no way back, and the deletion was unfiltered across every printer. + # The readers that could be confused now ask which mode is active + # (``spoolman_owns_assignments``), which is where that decision belongs: + # the mode is a property of the install, not of the rows. + if was_enabled != now_enabled: + logger.info( + "Inventory mode switched to %s; slot assignments in both tables kept", + "Spoolman" if now_enabled else "built-in", + ) if "spoolman_url" in settings: await set_setting(db, "spoolman_url", normalize_str_setting("spoolman_url", settings["spoolman_url"])) if "spoolman_sync_mode" in settings: diff --git a/backend/app/main.py b/backend/app/main.py index fb233f1b3..27b34de51 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -1953,17 +1953,26 @@ async def on_ams_change(printer_id: int, ams_data: list): from backend.app.api.routes.inventory import _find_tray_in_ams_data from backend.app.models.spool import Spool as _Spool from backend.app.models.spool_assignment import SpoolAssignment as SA + from backend.app.services.inventory_mode import spoolman_owns_assignments - result = await db.execute( - select(SA) - .where(SA.printer_id == printer_id) - .options(selectinload(SA.spool).selectinload(_Spool.k_profiles)) - ) + # Built-in assignments only. Since #2812 they survive a switch to + # Spoolman mode rather than being deleted by it, and this pass ends + # in ``db.delete`` — left ungated it would unlink them one slot at a + # time as the AMS contents changed under the other mode, undoing the + # preservation more slowly but just as completely. + assignments = [] + if not await spoolman_owns_assignments(db): + result = await db.execute( + select(SA) + .where(SA.printer_id == printer_id) + .options(selectinload(SA.spool).selectinload(_Spool.k_profiles)) + ) + assignments = result.scalars().all() # ``printing_now`` (top of this function) keeps a runout from # unlinking the spool that fed the print — the next idle-time pass # unlinks it if the user really did take it out. stale = [] - for assignment in result.scalars().all(): + for assignment in assignments: # External spool assignments (ams_id=255) live in vt_tray, not AMS data if assignment.ams_id == 255: ps = printer_manager.get_status(printer_id) @@ -2521,21 +2530,35 @@ async def on_ams_change(printer_id: int, ams_data: list): from backend.app.models.spool_assignment import SpoolAssignment from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment + from backend.app.services.inventory_mode import spoolman_owns_assignments + # Built-in remaining weight, used by sync_ams_tray only when the + # firmware reports an unusable remain%/tray_weight for a slot. + # + # Left empty since #2812. This block runs in Spoolman mode only, + # and until then the built-in table was emptied on the switch, so + # there was never anything here to read and the fallback was inert. + # Preserving those rows makes it live again, and it is keyed by slot + # rather than by spool: after a mode switch the tray may well hold + # different filament, and ``create_spool`` writes ``remaining_weight`` + # unconditionally, so a stale figure would be seeded into a brand new + # Spoolman spool. Deliberately kept inert rather than deleted, so the + # intent survives for whoever revisits the cross-mode fallback. inventory_weights: dict[tuple[int, int], float] = {} - try: - assign_result = await db.execute( - select(SpoolAssignment) - .options(selectinload(SpoolAssignment.spool)) - .where(SpoolAssignment.printer_id == printer_id) - ) - for assignment in assign_result.scalars().all(): - spool = assignment.spool - if spool and spool.label_weight > 0: - remaining = max(0.0, spool.label_weight - (spool.weight_used or 0)) - inventory_weights[(assignment.ams_id, assignment.tray_id)] = remaining - except Exception as e: - logger.warning("Could not load inventory weights for printer %s: %s", printer_id, e) + if not await spoolman_owns_assignments(db): + try: + assign_result = await db.execute( + select(SpoolAssignment) + .options(selectinload(SpoolAssignment.spool)) + .where(SpoolAssignment.printer_id == printer_id) + ) + for assignment in assign_result.scalars().all(): + spool = assignment.spool + if spool and spool.label_weight > 0: + remaining = max(0.0, spool.label_weight - (spool.weight_used or 0)) + inventory_weights[(assignment.ams_id, assignment.tray_id)] = remaining + except Exception as e: + logger.warning("Could not load inventory weights for printer %s: %s", printer_id, e) # Load existing Spoolman slot assignments for the no-RFID fallback path spoolman_slot_map: dict[tuple[int, int], int] = {} diff --git a/backend/app/services/inventory_mode.py b/backend/app/services/inventory_mode.py new file mode 100644 index 000000000..191f80803 --- /dev/null +++ b/backend/app/services/inventory_mode.py @@ -0,0 +1,42 @@ +"""Which table holds a printer's slot assignments. + +Bambuddy keeps AMS slot assignments in two places: ``spool_assignment`` for the +built-in inventory and ``spoolman_slot_assignments`` for Spoolman. Exactly one +of them describes reality at any moment, and which one is a user setting. + +Until #2812 the two were kept from overlapping by emptying the inactive table +whenever the mode toggled, which made merely looking at the other mode destroy +the configuration you had. Nothing is deleted now, so both tables can hold rows +at once and every reader has to say which one it means. + +This is deliberately a module of its own rather than a helper on the settings +routes: the readers are services, and importing an API route module from a +service to answer a one-key question invites an import cycle. Several call +sites already carry their own private copy of this predicate for that reason +(``filament_deficit``, ``print_scheduler``, ``inventory``); those are unchanged +and correct, and are only worth folding in here if they are touched anyway. +""" + +import logging + +from sqlalchemy.ext.asyncio import AsyncSession + +logger = logging.getLogger(__name__) + + +async def spoolman_owns_assignments(db: AsyncSession) -> bool: + """True when ``spoolman_slot_assignments`` is the table that counts. + + Fails closed to the built-in inventory: a setting that cannot be read is + not evidence that the user switched modes, and treating an unreadable + setting as "Spoolman" would make a built-in install look as though every + tray were unassigned. + """ + try: + from backend.app.api.routes.settings import get_setting + + value = await get_setting(db, "spoolman_enabled") + return bool(value) and value.lower() == "true" + except Exception as exc: # noqa: BLE001 — a mode probe must not raise into its callers + logger.debug("Could not read spoolman_enabled, assuming built-in inventory: %s", exc) + return False diff --git a/backend/app/services/print_cost_estimate.py b/backend/app/services/print_cost_estimate.py index 6b0acf1a6..ed6c509fc 100644 --- a/backend/app/services/print_cost_estimate.py +++ b/backend/app/services/print_cost_estimate.py @@ -12,6 +12,7 @@ from backend.app.core.config import settings from backend.app.models.archive import PrintArchive from backend.app.models.library import LibraryFile from backend.app.models.spool_assignment import SpoolAssignment +from backend.app.services.inventory_mode import spoolman_owns_assignments from backend.app.utils import threemf_tools from backend.app.utils.safe_path import safe_join_under @@ -135,7 +136,11 @@ async def estimate_queue_source_cost( default_cost = await _default_cost_per_kg(db) cost_by_tray: dict[int, float | None] = {} mapping = _parse_mapping(ams_mapping) - if printer_id is not None and mapping: + # Built-in spool prices only. In Spoolman mode the built-in table may still + # hold rows from before the user switched -- nothing clears it since #2812 -- + # and pricing an estimate from a spool the printer is not drawing on would + # be worse than the default rate this falls back to. + if printer_id is not None and mapping and not await spoolman_owns_assignments(db): assignments = ( ( await db.execute( diff --git a/backend/app/services/slot_kprofile.py b/backend/app/services/slot_kprofile.py index 67c96b31f..891df582d 100644 --- a/backend/app/services/slot_kprofile.py +++ b/backend/app/services/slot_kprofile.py @@ -22,6 +22,7 @@ from backend.app.models.spool_assignment import SpoolAssignment from backend.app.models.spool_k_profile import SpoolKProfile from backend.app.models.spoolman_k_profile import SpoolmanKProfile from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment +from backend.app.services.inventory_mode import spoolman_owns_assignments @dataclass(frozen=True) @@ -51,18 +52,29 @@ async def find_slot_kprofile_for_extruder( profile for this nozzle — an operator who calibrated only one side should keep the binding they set by hand rather than have it swapped for a guess. - Local spools take priority over Spoolman, matching the rest of the - K-profile cascade. + Only the table the current inventory mode uses is consulted. Before #2812 + the inactive one was emptied on every mode toggle, so reading the built-in + table first and stopping on a hit was safe -- there could be nothing in it + to stop on. Nothing is emptied now, and a leftover built-in row would + otherwise shadow the Spoolman assignment for the slot, returning that + spool's profile or, on the deliberate stop below, no profile at all. That + is the symptom #1556 reported from the other direction. """ + spoolman_mode = await spoolman_owns_assignments(db) + assignment = ( - await db.execute( - select(SpoolAssignment).where( - SpoolAssignment.printer_id == printer_id, - SpoolAssignment.ams_id == ams_id, - SpoolAssignment.tray_id == tray_id, + None + if spoolman_mode + else ( + await db.execute( + select(SpoolAssignment).where( + SpoolAssignment.printer_id == printer_id, + SpoolAssignment.ams_id == ams_id, + SpoolAssignment.tray_id == tray_id, + ) ) - ) - ).scalar_one_or_none() + ).scalar_one_or_none() + ) if assignment is not None: profile = ( @@ -92,6 +104,9 @@ async def find_slot_kprofile_for_extruder( # falling through to Spoolman would answer for a different spool. return None + if not spoolman_mode: + return None + sm_assignment = ( await db.execute( select(SpoolmanSlotAssignment).where( diff --git a/backend/app/services/spool_assignment_notifications.py b/backend/app/services/spool_assignment_notifications.py index 2ad2ea05a..01d506233 100644 --- a/backend/app/services/spool_assignment_notifications.py +++ b/backend/app/services/spool_assignment_notifications.py @@ -6,6 +6,7 @@ from backend.app.models.printer import Printer from backend.app.models.spool_assignment import SpoolAssignment from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment from backend.app.services.bambu_mqtt import PrinterState +from backend.app.services.inventory_mode import spoolman_owns_assignments from backend.app.services.notification_service import notification_service from backend.app.services.printer_manager import printer_manager @@ -131,52 +132,94 @@ async def notify_missing_spool_assignments_on_print_start( printer = await db.get(Printer, printer_id) printer_name = printer.name if printer else f"Printer {printer_id}" - # A tray is "assigned" if it has a row in EITHER table: the legacy - # spool_assignment table (internal-inventory mode) or - # spoolman_slot_assignments (Spoolman mode — the binding - # source-of-truth since #1119). Querying only the legacy table - # flagged every used tray as missing on every Spoolman-mode print - # (#1473). Both tables expose printer_id / ams_id / tray_id in the - # same shape, so _global_tray_from_assignment works on either. - legacy_rows = ( - await db.execute(SpoolAssignment.__table__.select().where(SpoolAssignment.printer_id == printer_id)) - ).fetchall() - spoolman_rows = ( - await db.execute( - SpoolmanSlotAssignment.__table__.select().where(SpoolmanSlotAssignment.printer_id == printer_id) - ) - ).fetchall() - assigned_global_trays = { - _global_tray_from_assignment(row.ams_id, row.tray_id) for row in (*legacy_rows, *spoolman_rows) - } + # A tray is "assigned" if it has a row in the table the current + # mode uses. Both expose printer_id / ams_id / tray_id in the same + # shape, so _global_tray_from_assignment works on either. + # + # This read both tables and unioned them until #2812. That was + # correct while the inactive table was emptied on every mode + # toggle -- it is how #1473 was fixed, where querying only the + # legacy table flagged every tray as missing on a Spoolman print. + # Nothing is emptied now, so a union would let a leftover row in + # the mode you are *not* using vouch for a tray that has no + # assignment in the mode you are, and this notification exists + # precisely to catch that tray. + table = SpoolmanSlotAssignment if await spoolman_owns_assignments(db) else SpoolAssignment + rows = (await db.execute(table.__table__.select().where(table.printer_id == printer_id))).fetchall() + assigned_global_trays = {_global_tray_from_assignment(row.ams_id, row.tray_id) for row in rows} missing_global = sorted(used_global_trays - assigned_global_trays) if not missing_global: return - state = printer_manager.get_status(printer_id) - missing_slots = [] - for global_id in missing_global: - profile, color = _tray_profile_and_color_for_global_id(state, global_id) - missing_slots.append( - { - "slot": _slot_label_from_global_tray(global_id), - "profile": profile, - "color": color, - } - ) - - await ws_manager.send_missing_spool_assignment( - printer_id=printer_id, - printer_name=printer_name, - missing_slots=missing_slots, - ) - - await notification_service.on_print_missing_spool_assignment( - printer_id=printer_id, - printer_name=printer_name, - missing_slots=missing_slots, - db=db, - ) + await _send_missing_assignment_notification(printer_id, printer_name, missing_global, db) except Exception as e: logger.warning("Missing spool-assignment notification failed: %s", e) + + +async def _send_missing_assignment_notification( + printer_id: int, + printer_name: str, + missing_global: list[int], + db, +) -> None: + """Describe the unassigned trays and push them to the UI and the providers.""" + state = printer_manager.get_status(printer_id) + missing_slots = [] + for global_id in missing_global: + profile, color = _tray_profile_and_color_for_global_id(state, global_id) + missing_slots.append( + { + "slot": _slot_label_from_global_tray(global_id), + "profile": profile, + "color": color, + } + ) + + await ws_manager.send_missing_spool_assignment( + printer_id=printer_id, + printer_name=printer_name, + missing_slots=missing_slots, + ) + await notification_service.on_print_missing_spool_assignment( + printer_id=printer_id, + printer_name=printer_name, + missing_slots=missing_slots, + db=db, + ) + + +async def notify_missing_spool_assignments_on_print_complete( + printer_id: int, + missing_global_trays: list[int], + db, + logger: logging.Logger, +) -> None: + """Say so when a finished print could not debit a tray it drew from (#2812). + + The print-start check above is predictive: it reads the mapping before the + job runs and warns about trays that have no assignment yet. It cannot cover + an assignment that disappears *during* a print, and nothing re-checked + afterwards -- so a print whose assignments existed at print start, and were + gone by the time it finished, resolved its 3MF, resolved its grams, + resolved its tray, skipped the debit at INFO, and reported success. The + reporter lost 65.49 g that way and only noticed because a spool's remaining + weight looked wrong. + + This fires on realized loss rather than risk: the trays passed here are the + ones a completed print actually tried to charge and could not. A print that + was already warned at start will notify twice, which is the right trade -- + the first says the weight may not be tracked, the second says it was not. + + Takes the caller's session: this runs inside ``on_print_complete``'s + transaction, and opening a second one to read the printer's name would + deadlock against it on SQLite. + """ + if not missing_global_trays: + return + try: + printer = await db.get(Printer, printer_id) + printer_name = printer.name if printer else f"Printer {printer_id}" + await _send_missing_assignment_notification(printer_id, printer_name, sorted(set(missing_global_trays)), db) + except Exception as e: # noqa: BLE001 — a notification must not fail a completed print + logger.warning("Missing spool-assignment completion notification failed: %s", e) diff --git a/backend/app/services/usage_tracker.py b/backend/app/services/usage_tracker.py index dc6fe6514..ba1df7be8 100644 --- a/backend/app/services/usage_tracker.py +++ b/backend/app/services/usage_tracker.py @@ -1481,6 +1481,10 @@ async def _track_from_3mf( pass # Fall back to linear scaling results = [] + # Trays this print drew from that no longer have an assignment to charge. + # Collected rather than acted on inline so one notification covers the whole + # print instead of one per slot (#2812). + unassigned_global_trays: list[int] = [] for usage in filament_usage: slot_id = usage.get("slot_id", 0) @@ -1707,7 +1711,19 @@ async def _track_from_3mf( print_started_at=print_started_at, ) if spool_id is None: - logger.info("[UsageTracker] 3MF: no spool assignment at printer %d AMS%d-T%d", printer_id, ams_id, tray_id) + # WARNING, not INFO: everything upstream of this line succeeded -- + # the 3MF was found, the grams were read, the tray resolved -- and + # the print will still report success while this filament is never + # deducted. At INFO it was invisible under the default log level and + # absent from the reasoning in support bundles (#2812). + logger.warning( + "[UsageTracker] 3MF: no spool assignment at printer %d AMS%d-T%d — %.1fg not deducted", + printer_id, + ams_id, + tray_id, + used_g, + ) + unassigned_global_trays.append(global_tray_id) continue # Load spool @@ -1817,4 +1833,11 @@ async def _track_from_3mf( ) archive.filament_type = joined_types + if unassigned_global_trays: + from backend.app.services.spool_assignment_notifications import ( + notify_missing_spool_assignments_on_print_complete, + ) + + await notify_missing_spool_assignments_on_print_complete(printer_id, unassigned_global_trays, db, logger) + return results diff --git a/backend/tests/integration/test_mode_switch_preserves_assignments_2812.py b/backend/tests/integration/test_mode_switch_preserves_assignments_2812.py new file mode 100644 index 000000000..1fcc6c345 --- /dev/null +++ b/backend/tests/integration/test_mode_switch_preserves_assignments_2812.py @@ -0,0 +1,126 @@ +"""Preserved built-in assignments must stay preserved (#2812). + +The mode toggle no longer deletes them. That is only worth anything if the rest +of the app leaves them alone while Spoolman mode is active — otherwise they are +destroyed just as completely, only more slowly. + +The auto-unlink pass in ``on_ams_change`` is the one that matters: it ends in +``db.delete`` for every assignment whose slot no longer matches the fingerprint +it recorded, which is precisely what happens once the user starts loading +different filament under the other mode. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from httpx import AsyncClient +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from backend.app.models.settings import Settings +from backend.app.models.spool import Spool +from backend.app.models.spool_assignment import SpoolAssignment + + +def _status(ams_data): + status = MagicMock() + status.raw_data = {"ams": ams_data, "vt_tray": []} + status.gcode_state = "IDLE" + return status + + +async def _run_ams_change(printer_id: int, ams_data: list): + from backend.app.main import on_ams_change + + status = _status(ams_data) + with ( + patch("backend.app.main.printer_manager") as pm_main, + patch("backend.app.services.printer_manager.printer_manager") as pm_inv, + patch("backend.app.main.mqtt_relay") as relay, + patch("backend.app.main.ws_manager") as ws, + ): + pm_main.get_printer.return_value = MagicMock(name="P", serial_number="SER") + pm_main.get_status.return_value = status + pm_main.get_client.return_value = MagicMock() + pm_main.get_model.return_value = "X1C" + pm_inv.get_status.return_value = status + pm_inv.get_client.return_value = MagicMock() + relay.on_ams_change = AsyncMock() + ws.send_printer_status = AsyncMock() + ws.broadcast = AsyncMock() + await on_ams_change(printer_id, ams_data) + + +class TestAutoUnlinkRespectsTheActiveMode: + @pytest.mark.asyncio + @pytest.mark.integration + async def test_spoolman_mode_does_not_unlink_built_in_assignments( + self, async_client: AsyncClient, printer_factory, db_session: AsyncSession + ): + """The slot now holds something else entirely — a stale fingerprint by + every measure. In built-in mode that is a genuine unlink. In Spoolman + mode these rows are the user's preserved configuration, waiting for + them to switch back, and nothing here is entitled to delete them.""" + printer = await printer_factory(name="P1S") + spool = Spool(material="PLA", rgba="FF0000FF") + db_session.add(spool) + await db_session.flush() + db_session.add(Settings(key="spoolman_enabled", value="true")) + db_session.add( + SpoolAssignment( + spool_id=spool.id, + printer_id=printer.id, + ams_id=0, + tray_id=0, + fingerprint_color="FF0000FF", + fingerprint_type="PLA", + ) + ) + await db_session.commit() + + await _run_ams_change( + printer.id, + [{"id": 0, "tray": [{"id": 0, "tray_type": "PETG", "tray_color": "00FF00FF", "state": 11}]}], + ) + + rows = ( + (await db_session.execute(select(SpoolAssignment).where(SpoolAssignment.printer_id == printer.id))) + .scalars() + .all() + ) + assert len(rows) == 1 + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_built_in_mode_still_unlinks_a_stale_assignment( + self, async_client: AsyncClient, printer_factory, db_session: AsyncSession + ): + """The guard must not switch the feature off for the mode that owns it.""" + printer = await printer_factory(name="P1S") + spool = Spool(material="PLA", rgba="FF0000FF") + db_session.add(spool) + await db_session.flush() + db_session.add(Settings(key="spoolman_enabled", value="false")) + db_session.add( + SpoolAssignment( + spool_id=spool.id, + printer_id=printer.id, + ams_id=0, + tray_id=0, + fingerprint_color="FF0000FF", + fingerprint_type="PLA", + ) + ) + await db_session.commit() + + await _run_ams_change( + printer.id, + [{"id": 0, "tray": [{"id": 0, "tray_type": "PETG", "tray_color": "00FF00FF", "state": 11}]}], + ) + + rows = ( + (await db_session.execute(select(SpoolAssignment).where(SpoolAssignment.printer_id == printer.id))) + .scalars() + .all() + ) + assert rows == [] diff --git a/backend/tests/integration/test_printers_api.py b/backend/tests/integration/test_printers_api.py index 60b7b0832..9dc159929 100644 --- a/backend/tests/integration/test_printers_api.py +++ b/backend/tests/integration/test_printers_api.py @@ -2576,9 +2576,15 @@ class TestApplyPaAfterRefresh: async def test_spoolman_kp_when_no_local(self, db_session, printer_factory): """No local assignment + Spoolman SlotAssignment + SpoolmanKProfile → Spoolman cali_idx.""" from backend.app.api.routes.printers import _apply_pa_after_refresh + from backend.app.models.settings import Settings from backend.app.models.spoolman_k_profile import SpoolmanKProfile from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment + # A spoolman_slot_assignments row only exists in Spoolman mode, and + # since #2812 the mode is asked for explicitly rather than inferred + # from which table happens to hold rows. + db_session.add(Settings(key="spoolman_enabled", value="true")) + printer = await printer_factory() db_session.add( SpoolmanSlotAssignment( @@ -2622,8 +2628,14 @@ class TestApplyPaAfterRefresh: async def test_spoolman_no_kp_uses_live(self, db_session, printer_factory): """Spoolman SlotAssignment but no SpoolmanKProfile → live cali_idx (Stage 3).""" from backend.app.api.routes.printers import _apply_pa_after_refresh + from backend.app.models.settings import Settings from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment + # A spoolman_slot_assignments row only exists in Spoolman mode, and + # since #2812 the mode is asked for explicitly rather than inferred + # from which table happens to hold rows. + db_session.add(Settings(key="spoolman_enabled", value="true")) + printer = await printer_factory() db_session.add( SpoolmanSlotAssignment( @@ -3131,9 +3143,15 @@ class TestConfigureAmsSlotPersistsKProfile: printer_factory, ): """SpoolmanSlotAssignment present → SpoolmanKProfile row created with cali_idx + k_value + name.""" + from backend.app.models.settings import Settings from backend.app.models.spoolman_k_profile import SpoolmanKProfile from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment + # A spoolman_slot_assignments row only exists in Spoolman mode, and + # since #2812 the mode is asked for explicitly rather than inferred + # from which table happens to hold rows. + db_session.add(Settings(key="spoolman_enabled", value="true")) + printer = await printer_factory(model="H2D") db_session.add( SpoolmanSlotAssignment( @@ -3309,9 +3327,15 @@ class TestConfigureAmsSlotPersistsKProfile: printer_factory, ): """cali_idx=-1 (no profile selected) → no DB write even when assignment exists.""" + from backend.app.models.settings import Settings from backend.app.models.spoolman_k_profile import SpoolmanKProfile from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment + # A spoolman_slot_assignments row only exists in Spoolman mode, and + # since #2812 the mode is asked for explicitly rather than inferred + # from which table happens to hold rows. + db_session.add(Settings(key="spoolman_enabled", value="true")) + printer = await printer_factory(model="H2D") db_session.add( SpoolmanSlotAssignment( @@ -3368,9 +3392,15 @@ class TestConfigureAmsSlotPersistsKProfile: printer_factory, ): """cali_idx=0 is the first valid profile slot (NOT a sentinel for missing).""" + from backend.app.models.settings import Settings from backend.app.models.spoolman_k_profile import SpoolmanKProfile from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment + # A spoolman_slot_assignments row only exists in Spoolman mode, and + # since #2812 the mode is asked for explicitly rather than inferred + # from which table happens to hold rows. + db_session.add(Settings(key="spoolman_enabled", value="true")) + printer = await printer_factory(model="H2D") db_session.add( SpoolmanSlotAssignment( @@ -3425,9 +3455,15 @@ class TestConfigureAmsSlotPersistsKProfile: printer_factory, ): """Repeated POSTs update the same row (UNIQUE on spool_id+printer+extruder+nozzle_diameter).""" + from backend.app.models.settings import Settings from backend.app.models.spoolman_k_profile import SpoolmanKProfile from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment + # A spoolman_slot_assignments row only exists in Spoolman mode, and + # since #2812 the mode is asked for explicitly rather than inferred + # from which table happens to hold rows. + db_session.add(Settings(key="spoolman_enabled", value="true")) + printer = await printer_factory(model="H2D") db_session.add( SpoolmanSlotAssignment( @@ -3635,8 +3671,14 @@ class TestConfigureAmsSlotPersistsKProfile: so we shouldn't return 500 to the user. The error is logged and the endpoint returns success. """ + from backend.app.models.settings import Settings from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment + # A spoolman_slot_assignments row only exists in Spoolman mode, and + # since #2812 the mode is asked for explicitly rather than inferred + # from which table happens to hold rows. + db_session.add(Settings(key="spoolman_enabled", value="true")) + printer = await printer_factory(model="H2D") db_session.add( SpoolmanSlotAssignment( diff --git a/backend/tests/integration/test_spoolman_slot_assignments.py b/backend/tests/integration/test_spoolman_slot_assignments.py index 226054d88..bd8317562 100644 --- a/backend/tests/integration/test_spoolman_slot_assignments.py +++ b/backend/tests/integration/test_spoolman_slot_assignments.py @@ -579,20 +579,21 @@ class TestCascadeDeletePrinter: assert post.scalars().all() == [] -class TestModeSwitchClearsAssignments: - """#1473 follow-up — the Spoolman mode toggle clears the other mode's - slot-assignment table so stale rows can't bleed across a mode switch.""" +class TestModeSwitchKeepsAssignments: + """#2812 — the mode toggle no longer deletes anything. + + It used to empty the other mode's table on every switch, so opening the + settings page to see what Spoolman mode did destroyed the built-in slot + assignments, with no confirmation and no way back. Both tables are kept now + and the readers that could be confused by a row in the inactive one ask + which mode is active instead (``spoolman_owns_assignments``). + """ @pytest.mark.asyncio @pytest.mark.integration - async def test_switch_to_internal_mode_clears_spoolman_slot_assignments( + async def test_switch_to_internal_mode_keeps_spoolman_slot_assignments( self, async_client: AsyncClient, db_session, test_printer ): - """Switching Spoolman OFF deletes spoolman_slot_assignments rows — the - symmetric counterpart of clearing legacy spool_assignment rows when - switching ON. Stale rows would otherwise wrongly count as 'assigned' - in mode-agnostic checks (e.g. the missing-spool-assignment notification, - which unions both tables).""" from backend.app.models.settings import Settings from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment @@ -606,4 +607,30 @@ class TestModeSwitchClearsAssignments: rows = await db_session.execute( select(SpoolmanSlotAssignment).where(SpoolmanSlotAssignment.printer_id == test_printer.id) ) - assert rows.scalars().all() == [] + assert len(rows.scalars().all()) == 1 + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_switch_to_spoolman_mode_keeps_builtin_assignments( + self, async_client: AsyncClient, db_session, test_printer + ): + """The reported case: four toggles in 85 seconds took the reporter's + built-in assignments and never gave them back.""" + from backend.app.models.settings import Settings + from backend.app.models.spool import Spool + from backend.app.models.spool_assignment import SpoolAssignment + + db_session.add(Settings(key="spoolman_enabled", value="false")) + spool = Spool(material="PLA", rgba="000000FF") + db_session.add(spool) + await db_session.flush() + db_session.add(SpoolAssignment(printer_id=test_printer.id, ams_id=0, tray_id=0, spool_id=spool.id)) + await db_session.commit() + + on = await async_client.put("/api/v1/settings/spoolman", json={"spoolman_enabled": "true"}) + assert on.status_code == 200 + off = await async_client.put("/api/v1/settings/spoolman", json={"spoolman_enabled": "false"}) + assert off.status_code == 200 + + rows = await db_session.execute(select(SpoolAssignment).where(SpoolAssignment.printer_id == test_printer.id)) + assert len(rows.scalars().all()) == 1 diff --git a/backend/tests/unit/services/test_mode_toggle_keeps_assignments_2812.py b/backend/tests/unit/services/test_mode_toggle_keeps_assignments_2812.py new file mode 100644 index 000000000..2bda99767 --- /dev/null +++ b/backend/tests/unit/services/test_mode_toggle_keeps_assignments_2812.py @@ -0,0 +1,235 @@ +"""The inventory mode toggle is no longer destructive (#2812). + +Turning Spoolman mode on ran an unfiltered ``delete(SpoolAssignment)`` across +every printer. Turning it straight back off cleared the *other* table instead, +so the built-in assignments were simply gone -- and the setting auto-saves on a +500 ms debounce with no confirmation, so inspecting the mode destroyed the +configuration. The reporter toggled four times in 85 seconds and never got +their assignments back. + +The deletion had a real reason: checks that read both assignment tables would +otherwise let a row in the mode you are *not* using answer for the mode you +are. The fix is to make those checks ask which mode is active, which is where +that decision belongs, and then stop deleting. +""" + +from types import SimpleNamespace +from unittest.mock import AsyncMock, patch + +import pytest + +from backend.app.services.slot_kprofile import find_slot_kprofile_for_extruder + + +class _Result: + def __init__(self, value): + self._value = value + + def scalar_one_or_none(self): + return self._value + + def scalars(self): + return self + + def first(self): + return self._value + + def all(self): + return [self._value] if self._value is not None else [] + + +class _TableRoutingSession: + """Returns a row per model, so a test can populate either table or both.""" + + def __init__(self, rows: dict): + self._rows = rows + self.queried = [] + + async def execute(self, stmt): + entity = stmt.column_descriptions[0]["entity"] + name = entity.__name__ + self.queried.append(name) + return _Result(self._rows.get(name)) + + +class TestKProfileIgnoresTheInactiveModesTable: + """slot_kprofile checked the built-in table first and, on a hit with no + matching profile, deliberately returned None rather than falling through to + Spoolman. That was safe only while the built-in table was guaranteed empty + in Spoolman mode. A leftover row would otherwise shadow the Spoolman + binding -- the symptom #1556 reported from the other direction. + """ + + @pytest.mark.asyncio + async def test_spoolman_mode_does_not_read_the_built_in_table(self): + session = _TableRoutingSession( + { + # A leftover from before the user switched modes. + "SpoolAssignment": SimpleNamespace(spool_id=7), + "SpoolmanSlotAssignment": None, + } + ) + + with patch( + "backend.app.services.slot_kprofile.spoolman_owns_assignments", + new_callable=AsyncMock, + return_value=True, + ): + result = await find_slot_kprofile_for_extruder( + session, printer_id=1, ams_id=0, tray_id=0, extruder=0, nozzle_diameter="0.4" + ) + + assert result is None + assert "SpoolAssignment" not in session.queried + + @pytest.mark.asyncio + async def test_built_in_mode_does_not_read_the_spoolman_table(self): + session = _TableRoutingSession( + { + "SpoolAssignment": None, + "SpoolmanSlotAssignment": SimpleNamespace(spoolman_spool_id=9), + } + ) + + with patch( + "backend.app.services.slot_kprofile.spoolman_owns_assignments", + new_callable=AsyncMock, + return_value=False, + ): + result = await find_slot_kprofile_for_extruder( + session, printer_id=1, ams_id=0, tray_id=0, extruder=0, nozzle_diameter="0.4" + ) + + assert result is None + assert "SpoolmanSlotAssignment" not in session.queried + + +class TestCostEstimateIgnoresTheInactiveModesTable: + """A leftover built-in assignment must not price a pre-print estimate from + a spool the printer is not drawing on. The default rate is the honest + answer once the mode has moved on.""" + + @staticmethod + async def _estimate(spoolman_mode: bool): + from backend.app.services import print_cost_estimate as pce + + library_file = SimpleNamespace( + file_path="nowhere/never.3mf", + file_metadata={"filament_used_grams": 100.0}, + source_folder=None, + ) + + db = AsyncMock() + db.execute = AsyncMock(side_effect=AssertionError("the built-in table must not be queried")) + + with ( + patch( + "backend.app.services.print_cost_estimate.spoolman_owns_assignments", + new_callable=AsyncMock, + return_value=spoolman_mode, + ), + patch( + "backend.app.services.print_cost_estimate._default_cost_per_kg", + new_callable=AsyncMock, + return_value=25.0, + ), + patch( + "backend.app.services.print_cost_estimate._source_path", + return_value=__import__("pathlib").Path("/nonexistent/never.3mf"), + ), + ): + return await pce.estimate_queue_source_cost( + db, + library_file=library_file, + ams_mapping=[0], + printer_id=1, + ) + + @pytest.mark.asyncio + async def test_spoolman_mode_does_not_price_from_built_in_spools(self): + # 100 g at the 25/kg default. The AsyncMock would raise if the + # built-in assignment table were queried. + assert await self._estimate(spoolman_mode=True) == pytest.approx(2.5) + + @pytest.mark.asyncio + async def test_built_in_mode_still_reads_its_own_table(self): + """The guard must not switch the built-in path off as well.""" + with pytest.raises(AssertionError, match="must not be queried"): + await self._estimate(spoolman_mode=False) + + +class TestCompletionNotifiesTheLostDebit: + """The half that turned a mis-click into lost filament. + + A print whose assignments existed at print start and were gone by the time + it finished resolved its 3MF, resolved its grams, resolved its tray, and + then skipped the debit because the row was missing -- at INFO, with no + notification, while the completion notification fired as usual. The + reporter's 65.49 g was never deducted and nothing surfaced it. + + The print-start check cannot cover this: it runs before the job and was + correct to stay quiet, because at that moment the assignments existed. + """ + + @pytest.mark.asyncio + async def test_a_skipped_debit_notifies_at_completion(self): + from backend.app.services.spool_assignment_notifications import ( + notify_missing_spool_assignments_on_print_complete, + ) + + db = AsyncMock() + db.get = AsyncMock(return_value=SimpleNamespace(name="Printer A")) + logger = __import__("logging").getLogger(__name__) + + with ( + patch( + "backend.app.services.spool_assignment_notifications.printer_manager.get_status", + return_value=None, + ), + patch( + "backend.app.services.spool_assignment_notifications.ws_manager.send_missing_spool_assignment", + new_callable=AsyncMock, + ) as mock_ws, + patch( + "backend.app.services.spool_assignment_notifications.notification_service." + "on_print_missing_spool_assignment", + new_callable=AsyncMock, + ) as mock_notify, + ): + await notify_missing_spool_assignments_on_print_complete(1, [2], db, logger) + + mock_ws.assert_awaited_once() + assert mock_ws.await_args.kwargs["missing_slots"] == [{"slot": "A3", "profile": "Unknown", "color": "Unknown"}] + mock_notify.assert_awaited_once() + + @pytest.mark.asyncio + async def test_a_print_that_debited_everything_stays_quiet(self): + from backend.app.services.spool_assignment_notifications import ( + notify_missing_spool_assignments_on_print_complete, + ) + + db = AsyncMock() + logger = __import__("logging").getLogger(__name__) + + with patch( + "backend.app.services.spool_assignment_notifications.ws_manager.send_missing_spool_assignment", + new_callable=AsyncMock, + ) as mock_ws: + await notify_missing_spool_assignments_on_print_complete(1, [], db, logger) + + mock_ws.assert_not_awaited() + + @pytest.mark.asyncio + async def test_a_failure_here_never_fails_the_completed_print(self): + """The print is already done and its spools already written. A + notification that cannot be sent must not surface as a failed + completion.""" + from backend.app.services.spool_assignment_notifications import ( + notify_missing_spool_assignments_on_print_complete, + ) + + db = AsyncMock() + db.get = AsyncMock(side_effect=RuntimeError("db gone")) + logger = __import__("logging").getLogger(__name__) + + await notify_missing_spool_assignments_on_print_complete(1, [2], db, logger) diff --git a/backend/tests/unit/services/test_spool_assignment_notifications.py b/backend/tests/unit/services/test_spool_assignment_notifications.py index f4aec04cc..2e6c8cd87 100644 --- a/backend/tests/unit/services/test_spool_assignment_notifications.py +++ b/backend/tests/unit/services/test_spool_assignment_notifications.py @@ -89,9 +89,19 @@ async def test_missing_assignment_broadcasts_websocket_event_and_push_notificati assert notify_kwargs["missing_slots"] == [{"slot": "A2", "profile": "Unknown", "color": "Unknown"}] -def _patches(session): - """Common patch set: the fake session + stubbed printer state / emitters.""" +def _patches(session, spoolman_mode: bool = False): + """Common patch set: the fake session + stubbed printer state / emitters. + + ``spoolman_mode`` decides which assignment table the check reads. Since + #2812 it reads one, not both: nothing empties the inactive table on a mode + toggle any more, so a leftover row there must not vouch for a tray. + """ return ( + patch( + "backend.app.services.spool_assignment_notifications.spoolman_owns_assignments", + new_callable=AsyncMock, + return_value=spoolman_mode, + ), patch( "backend.app.services.spool_assignment_notifications.async_session", return_value=session, @@ -122,8 +132,8 @@ async def test_spoolman_only_assignment_suppresses_notification(): legacy=[], spoolman=[SimpleNamespace(ams_id=0, tray_id=0), SimpleNamespace(ams_id=0, tray_id=1)], ) - p_session, p_status, p_ws, p_notify = _patches(session) - with p_session, p_status, p_ws as mock_ws, p_notify as mock_notify: + p_mode, p_session, p_status, p_ws, p_notify = _patches(session, spoolman_mode=True) + with p_mode, p_session, p_status, p_ws as mock_ws, p_notify as mock_notify: await notify_missing_spool_assignments_on_print_start(1, data, logger) mock_ws.assert_not_awaited() @@ -142,8 +152,8 @@ async def test_spoolman_partial_coverage_flags_only_uncovered_tray(): legacy=[], spoolman=[SimpleNamespace(ams_id=0, tray_id=0)], # A1 only ) - p_session, p_status, p_ws, p_notify = _patches(session) - with p_session, p_status, p_ws as mock_ws, p_notify as mock_notify: + p_mode, p_session, p_status, p_ws, p_notify = _patches(session, spoolman_mode=True) + with p_mode, p_session, p_status, p_ws as mock_ws, p_notify as mock_notify: await notify_missing_spool_assignments_on_print_start(1, data, logger) mock_ws.assert_awaited_once() @@ -152,19 +162,47 @@ async def test_spoolman_partial_coverage_flags_only_uncovered_tray(): @pytest.mark.asyncio -async def test_mixed_mode_union_covers_all_used_trays(): - """A1 bound in the legacy table, A2 bound in spoolman_slot_assignments — - the union covers both used trays, so no notification fires.""" +async def test_a_row_in_the_inactive_modes_table_does_not_vouch_for_a_tray(): + """A1 bound in the legacy table, A2 bound in spoolman_slot_assignments. + + This used to union both and stay quiet. That was safe only because the mode + toggle emptied whichever table the current mode was not using, so the two + could never both hold rows -- and that emptying is what destroyed people's + assignments for merely looking at the other mode (#2812). Nothing is + emptied now, so in Spoolman mode the legacy row for A1 is a leftover from + before the switch and says nothing about whether A1 is assigned today. A1 + is exactly the tray this notification exists to flag. + """ logger = logging.getLogger(__name__) data = {"ams_mapping": [0, 1], "raw_data": {}} session = _FakeSession( "Printer A", - legacy=[SimpleNamespace(ams_id=0, tray_id=0)], # A1 - spoolman=[SimpleNamespace(ams_id=0, tray_id=1)], # A2 + legacy=[SimpleNamespace(ams_id=0, tray_id=0)], # A1 — leftover, not current + spoolman=[SimpleNamespace(ams_id=0, tray_id=1)], # A2 — the live binding ) - p_session, p_status, p_ws, p_notify = _patches(session) - with p_session, p_status, p_ws as mock_ws, p_notify as mock_notify: + p_mode, p_session, p_status, p_ws, p_notify = _patches(session, spoolman_mode=True) + with p_mode, p_session, p_status, p_ws as mock_ws, p_notify as mock_notify: + await notify_missing_spool_assignments_on_print_start(1, data, logger) + + mock_ws.assert_awaited_once() + assert mock_ws.await_args.kwargs["missing_slots"] == [{"slot": "A1", "profile": "Unknown", "color": "Unknown"}] + mock_notify.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_built_in_mode_reads_the_built_in_table(): + """The mirror image: in built-in mode a Spoolman row is the leftover.""" + logger = logging.getLogger(__name__) + data = {"ams_mapping": [0, 1], "raw_data": {}} + + session = _FakeSession( + "Printer A", + legacy=[SimpleNamespace(ams_id=0, tray_id=0), SimpleNamespace(ams_id=0, tray_id=1)], + spoolman=[SimpleNamespace(ams_id=0, tray_id=0)], + ) + p_mode, p_session, p_status, p_ws, p_notify = _patches(session, spoolman_mode=False) + with p_mode, p_session, p_status, p_ws as mock_ws, p_notify as mock_notify: await notify_missing_spool_assignments_on_print_start(1, data, logger) mock_ws.assert_not_awaited()