From 08a58b1ef45265ce8a5ed7e7a54de3946b2beaa4 Mon Sep 17 00:00:00 2001 From: maziggy Date: Tue, 25 Aug 2026 17:17:26 +0200 Subject: [PATCH] Keep both modes' slot assignments across an inventory mode switch (issue #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 two directions were symmetric in code and one-way in effect, and the setting auto-saves on a 500 ms debounce with no save button and no confirmation. Opening the settings page to see what the option did was enough to destroy the configuration: the reporter's log shows four toggles in 85 seconds, which is someone looking and reverting, and the assignments never came back. The deletion was not careless. Checks that read both assignment tables would otherwise let a row in the mode you are not using answer for the mode you are, which is how #1473 was fixed, and emptying the inactive table made that impossible by construction. The cost was that the guarantee was bought with the user's data. That decision belongs to the readers -- the mode is a property of the install, not of the rows -- so spoolman_owns_assignments now answers it and nothing is deleted on a toggle. Each mode keeps its own assignments and switching is reversible. Existing installs need no migration: their inactive table is already empty, because it was being emptied. Six sites had to be told which mode they meant, and only two of them are the reads you would guess at, the missing-assignment notification and the queue cost estimate. The per-slot K-profile lookup consults the built-in table first and, on a hit with no matching profile, deliberately stops rather than falling through to Spoolman, so a leftover row would have shadowed the Spoolman binding for that slot -- the symptom #1556 reported from the other direction. configure_ams_slot *writes* a K-profile against whichever table answers first, so the same leftover would have filed a calibration against a spool the printer is not drawing on and never written the local one, leaving a calibration that appeared to succeed and then did not apply. The auto-unlink pass in on_ams_change is the one that would have made this change worthless. It drops any assignment whose tray no longer matches the fingerprint it recorded, and it ends in db.delete. Ungated, it would have removed the preserved rows one slot at a time as the AMS contents changed under the other mode -- the same loss, arriving slowly enough not to be connected to the toggle that caused it. The sixth is the built-in remaining-weight fallback inside the Spoolman AMS sync, and it is deliberately left inert rather than woken up. It could never fire while the table it reads was being emptied, it is keyed by slot rather than by spool, and create_spool writes remaining_weight unconditionally where the update path does not -- so preserving the rows would have seeded a stale figure into a brand new Spoolman spool the first time a tray reported an unusable remain%. The query stays, gated off, so the intent survives for whoever revisits the cross-mode fallback. Separately, a print that could not debit a spool said nothing about it, and that is what turned a mis-click into lost filament. The reporter's print was already running when they toggled. At completion it resolved its 3MF, read its per-filament grams, resolved its tray, and then skipped the debit because the assignment row no longer existed -- logged at INFO, invisible under the default log level, while the completion notification fired as usual. 65.49 g was never deducted and they only noticed because a spool's remaining weight looked wrong. _resolve_spool_id_for_tray has no tag or fingerprint fallback, so there was nothing else to catch it. The skip is now a warning naming the grams, and a completed print that failed to charge a tray it drew from raises the missing-spool-assignment notification. The print-start check cannot cover this and was right to stay quiet: the assignments existed when it ran. The two are different statements -- the first says the weight may not be tracked, the second says it was not -- so a print warned at start will notify twice, which is the right trade. Collected across the print rather than fired per slot, and given the caller's session, because 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. This is independent of the toggle and catches any other cause of an assignment disappearing mid-print. --- CHANGELOG.md | 2 + backend/app/api/routes/printers.py | 35 ++- backend/app/api/routes/settings.py | 36 +-- backend/app/main.py | 61 +++-- backend/app/services/inventory_mode.py | 42 ++++ backend/app/services/print_cost_estimate.py | 7 +- backend/app/services/slot_kprofile.py | 33 ++- .../spool_assignment_notifications.py | 127 ++++++---- backend/app/services/usage_tracker.py | 25 +- ..._mode_switch_preserves_assignments_2812.py | 126 ++++++++++ .../tests/integration/test_printers_api.py | 42 ++++ .../test_spoolman_slot_assignments.py | 47 +++- ...test_mode_toggle_keeps_assignments_2812.py | 235 ++++++++++++++++++ .../test_spool_assignment_notifications.py | 64 ++++- 14 files changed, 760 insertions(+), 122 deletions(-) create mode 100644 backend/app/services/inventory_mode.py create mode 100644 backend/tests/integration/test_mode_switch_preserves_assignments_2812.py create mode 100644 backend/tests/unit/services/test_mode_toggle_keeps_assignments_2812.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9526d39cd..fd1bacf2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ All notable changes to Bambuddy will be documented in this file. - **The Windows installer build is split in two so a signing request can wait for a human (SignPath Foundation)** — Release tags are Authenticode-signed through the SignPath Foundation OSS programme, and the production certificate does not sign on demand the way the self-signed test certificate does: every request has to be approved by hand in the SignPath UI, because the Foundation verifies what is being signed and which build it came from. The submitting action waits for that approval with a default timeout of 600 seconds, which is ample when the test policy approves automatically in seconds and far too short once the wait is a person noticing a tag went out. A tag pushed at night would have failed the run ten minutes later with the installer already compiled and thrown away. The compile now ends in its own job that uploads the unsigned artifact and stops; a second job downloads it, signs it, and does the release-facing work, with the wait raised to an hour. Because the artifact is uploaded before the wait begins and is addressed by id, a missed approval window is recovered by re-running the second job alone rather than rebuilding the installer — which is the reason to separate them rather than simply raise the timeout in place. The second job runs for unsigned builds too, so the daily prereleases that are deliberately left unsigned to preserve the signing quota keep going out through exactly one set of alias, artifact and release steps. The property that matters is unchanged and now recorded next to the steps that depend on it: none of the alias, upload or release-attach steps carry `always()`, so GitHub skips all three when signing fails or times out, and an unsigned `.exe` cannot reach a release. Nothing about the signed output changes, and the restructure behaves identically under the test policy — the request simply completes immediately instead of waiting — so it can be proven green before the production certificate arrives. ### Fixed +- **Turning Spoolman mode on deleted every built-in slot assignment, and turning it back off did not restore them (#2812, reported by @chitrangdesign)** — The toggle ran an unfiltered `delete(SpoolAssignment)` across every printer. Switching straight back cleared the *other* table instead, so the two directions were symmetric in code and one-way in effect, and the setting auto-saves on a 500 ms debounce with no save button and no confirmation. Opening the settings page to see what the option did was enough to destroy the configuration — the reporter's log shows four toggles in 85 seconds and the assignments never came 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, which is how #1473 was fixed. That reason has moved to where it belongs: every place that could be confused now asks which mode is active rather than reading whichever table happens to have rows, and nothing is deleted on a toggle. Each mode keeps its own assignments, so switching is reversible and inspecting a mode costs nothing. Six sites needed it, and only two are reads you would guess at. The per-slot K-profile lookup consults the built-in table first and, on a hit with no matching profile, deliberately stops rather than falling through to Spoolman, so a leftover row would have shadowed the Spoolman binding — the symptom #1556 reported from the other direction. Configuring a slot *writes* its K-profile against whichever table answers first, so the same leftover would have filed a calibration against a spool the printer is not using and never written the local one, leaving a calibration that appeared to succeed and then did not apply. And the auto-unlink pass that drops an assignment whose tray no longer matches its fingerprint ends in a delete: left ungated it would have removed the preserved rows one slot at a time as the AMS contents changed under the other mode, undoing the whole point of this more slowly but just as completely. The remaining two are the missing-assignment notification and the queue cost estimate. A sixth, the built-in remaining-weight fallback inside the Spoolman AMS sync, is deliberately left inert: it was unreachable while the table was being emptied, it is keyed by slot rather than by spool, and `create_spool` writes `remaining_weight` unconditionally, so waking it up would seed a stale figure into a brand new Spoolman spool. Existing installs need no migration: their inactive table is already empty, because it was being emptied. +- **A print that could not debit a spool said nothing about it (#2812)** — This is what turned the toggle above from an annoyance into lost filament. A print whose assignments existed at print start and were gone by the time it finished resolved its 3MF, read its per-filament grams, resolved its tray, and then skipped the debit because the row no longer existed — logged at INFO, invisible under the default log level, with the completion notification firing as usual. 65.49 g was never deducted and the reporter only noticed because a spool's remaining weight looked wrong. The skip is now a warning that names the grams, and a completed print that failed to charge a tray it drew from raises the missing-spool-assignment notification, not only the print-start check that runs before the job and was correct to stay quiet. The two are different statements — the first says the weight may not be tracked, the second says it was not — so a print warned at start may notify twice, which is the right trade. This is independent of the toggle and catches any other cause of an assignment disappearing mid-print. - **Print cost ignored the linked Spoolman spool's price and always used the default rate (#2591, reported by @khaosdoctor)** — Spoolman holds per-spool pricing, and #261 gave that as the reason for integrating with it, but nothing ever read it. A print's cost is set once, at archive time, from the built-in Filament catalogue matched on the *primary* type, falling back to a global default rate — and in Spoolman mode nothing revisited that figure afterwards. The per-spool recompute that would have fixed it runs only over rows the built-in inventory writes, and Spoolman mode hands the usage tracker `spoolman_owns_usage` at print start so it writes none. On an install with an empty catalogue, which is what the reporter had, every print came out at the default no matter what the spool cost. Multi-material was wrong twice over: the primary type's rate was applied to the whole print's weight, so a slot of expensive PA was billed at the price of the PLA beside it. Each slot is now priced from the spool it was actually charged to, as the charge is made, and the per-slot costs are summed — which is what fixes the multi-material case rather than a separate change. The rate is the spool's own `price` when set, else the filament's, divided by `filament.weight`; that is the same net-grams field the remain%-delta path already divides by, so a spool that can be charged by percentage can always be priced. A missing or zero price is read as unpriced rather than free, and those grams — along with a tray that has no Spoolman row and any filament the sliced file never attributed — are covered at the default rate in one subtraction against the archive's own total, so a partially priced print still reports a whole-print figure instead of a fraction of one (#1344 in the other inventory mode). Only the first run writes the archive's cost, matching the built-in writer (#1378). Pricing is applied even when the slot-to-tray mapping was a positional guess, unlike the colour and material rewrites beside it: those overwrite what the slicer recorded, whereas the grams have already been deducted from these spools and the archive should say what that deduction was worth. - **A rescan or a cost recalculation quietly replaced a Spoolman-derived cost with a default-rate one** — Both rebuild an archive's cost from the built-in inventory's usage rows and fall back to the catalogue or the global default when there are none. In Spoolman mode there are never any, so the fallback was not a recalculation but a downgrade, and it would have undone the pricing above on the next rescan. The spool-to-slot resolution a price is derived from only exists while a print is being completed and cannot be rebuilt from the archive row afterwards, so both now leave a cost alone rather than replacing it with a worse one, and the bulk endpoint reports how many it kept. An archive that has no cost yet is still priced, and with Spoolman off both behave exactly as before. - **A print sent from Bambu Studio charged the wrong Spoolman spool, and rewrote the archive to match (#2953, reported by @bitelvl1)** — A sliced file numbers its filaments 1..4; which AMS tray each came from is decided when the job is sent, and #2768 gave the Spoolman writer two ways to recover that decision when the print did not come through Bambuddy: the printer's own `mapping` field, and a colour match of the 3MF's slots against the loaded trays. An A1 can satisfy neither. It publishes no `mapping` field, and it drops the MQTT connection when Bambuddy subscribes to its request topic, so the slicer's instruction never arrives either — which leaves the colour match, and that compares hex strings exactly. The reporter sliced with a generic black profile against a tray they had set to #111111. No match, so every print fell through to the positional default and charged slot 1 to whatever sat in the first tray: 2.17 g onto a grey PLA+ spool, while the print was fed from tray 3. The printer had already said so. `Tray change during print: tray=3 at layer=0` is in their log, recorded 90 seconds in, and read further down the same completion pass to decide which slots the print had touched — the same pass then charged tray 0 on a guess. A single-slot print is now pinned to the tray the printer reported feeding from, using the ladder the built-in inventory writer has consulted all along: the mid-print tray-change log, then the tray loaded at print start, then the current one, then the last real tray seen. Spoolman users were the only ones not getting it, which is why an install with the internal inventory in use never showed this. Gated on exactly one slot with usage, because a multi-colour print moves `tray_now` on every change and one reading cannot then be attributed to one slot; and it declines when the log holds more than one switch, so an AMS-backup runout still splits per segment (#1793) rather than landing whole on one spool. Where nothing at all names a tray the positional default still stands — it is right for an AMS loaded in slicer order — but it now says so at warning level, and no longer restamps the archive's filament colour and material from a spool it picked by position. That restamp is what made the fault look like data loss: the grams can be put back, whereas overwriting what the slicer recorded leaves nothing to compare against, and the reporter's archive had already been rewritten from #000000 to the wrong spool's grey. 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()