refactor(printers): every user stop goes through one function that also marks it, including the HMS dialog's Stop Printing

.basedpyright/baseline.json

@@ -2983,22 +2983,6 @@
                     "lineCount": 1
                 }
             },
-            {
-                "code": "reportArgumentType",
-                "range": {
-                    "startColumn": 47,
-                    "endColumn": 57,
-                    "lineCount": 1
-                }
-            },
-            {
-                "code": "reportArgumentType",
-                "range": {
-                    "startColumn": 37,
-                    "endColumn": 47,
-                    "lineCount": 1
-                }
-            },
             {
                 "code": "reportArgumentType",
                 "range": {

CHANGELOG.md

@@ -173,6 +173,8 @@ All notable changes to Bambuddy will be documented in this file.
 - **Stopping or cancelling a print through the API failed with an error** — `POST /api/v1/webhook/printer/{id}/cancel` answered with an error and left the print running. `POST /api/v1/webhook/printer/{id}/stop` stopped the print but still answered with an error. This affected Home Assistant automations and scripts that use an API key; the Stop button in Bambuddy was not affected.
   - **Now both stop the print and answer normally.** If the printer drops off before the command can be sent, they answer "Printer not connected" (503).
   - **A print stopped this way now counts as stopped, not failed,** as with the Stop button: the "print stopped" notification goes out instead of "print failed", and the print is no longer mislabelled with a printer error such as a layer shift.
+  - **`…/stop` now stops a paused print too,** as `…/cancel` and the Stop button on the printer card do; it used to answer that no print was in progress.
+  - **"Stop Printing" in the HMS error dialog** now also counts as stopped rather than failed.
 - **Starting the next plate of a Send All deleted the previous plate's timelapse (#3275, reported by @dovmesiz)** — All plates sent together with **Send All** share one archive. When a print starts on an archive that already has a timelapse, Bambuddy clears it so the new run gets its own video and completion photo. It also deleted the old video file, so starting plate 2 deleted plate 1's. With **delete from printer after archiving** on, that was the only copy. The video is now deleted only when the same plate prints again. Another plate's video, or one attached by hand or before this release, stays in the archive folder and is removed with the archive. The card still shows the latest plate's video. An external-camera timelapse no longer overwrites a kept video of the same name.
   - **A queued print still running 2 hours after it was sent lost what Bambuddy stored for it at dispatch:** its plate, AMS mapping and cost centre, which the completion uses (for example for the per-plate totals in the notification). A clean-up of unused print registrations removed them mid-print. They are now kept until the print completes.
 - **A PP spool without a slicer preset was sent to the AMS slot with no filament ID (#3273, reported by @sgiffhorn)** — When a spool has no preset of its own, Bambuddy fills in Bambu's generic preset for its material. The table used for spool assignment had no entry for PP, PE, PCTG, PLA Silk or PLA High Speed, so those slots got an empty filament ID, while **Configure AMS Slot** used a separate, longer table. The backend now has one table with all five, matching the one the frontend already used. A slot already set to Generic PLA Silk or PLA High Speed still keeps it when a PLA spool without a preset is assigned, as before.

backend/app/api/routes/print_queue.py

@@ -2391,8 +2391,6 @@ async def stop_queue_item(
     holding only _OWN saw the Stop button in the queue UI but got 403 on click.
     """
 
-    from backend.app.services.printer_manager import printer_manager
-
     user, can_modify_all = auth_result
 
     result = await db.execute(select(PrintQueueItem).where(PrintQueueItem.id == item_id))
@@ -2414,26 +2412,23 @@ async def stop_queue_item(
     printer_id = item.printer_id
     auto_off_after = item.auto_off_after
 
-    # Try to send stop command to printer
+    # Send the stop and mark the printer as user-stopped, even when the stop
+    # couldn't be sent: the job is closed below either way, so a "failed" the
+    # printer reports later is the user's cancel. Marked before the first await
+    # so an on_print_complete arriving during the db.commit() yield already
+    # sees the flag (no spurious "print failed" notification).
+    from backend.app.services.print_control import stop_print_by_user
+
     stop_sent = False
     try:
-        stop_sent = printer_manager.stop_print(printer_id)
+        # A printing job always has a printer; without one there is nothing to stop.
+        if printer_id is not None:
+            stop_sent = stop_print_by_user(printer_id, mark_when_unsent=True)
         if not stop_sent:
             logger.warning("stop_print returned False for printer %s - printer may not be connected", printer_id)
     except Exception as e:
         logger.error("Error sending stop command for queue item %s: %s", item_id, e)
 
-    # Mark this printer as user-stopped BEFORE the first await so that if the
-    # MQTT on_print_complete callback fires during the db.commit() yield the flag
-    # is already set and the "failed" status will be correctly overridden to
-    # "cancelled" (preventing a spurious "print failed" notification).
-    try:
-        from backend.app.main import mark_printer_stopped_by_user
-
-        mark_printer_stopped_by_user(printer_id)
-    except Exception as _mark_err:
-        logger.warning("Failed to mark printer %s as user-stopped: %s", printer_id, _mark_err)
-
     # Update queue item status regardless - if printer is off, print is already stopped
     item.status = "cancelled"
     item.completed_at = datetime.now(timezone.utc)

backend/app/api/routes/printers.py

@@ -62,6 +62,7 @@ from backend.app.services.bambu_ftp import (
     get_storage_info_async,
     list_files_result_async,
 )
+from backend.app.services.hms_actions import HMSAction
 from backend.app.services.print_storage import ftp_probe_paths, print_file_reachable_over_ftp
 from backend.app.services.printer_diagnostic import run_connection_diagnostic
 from backend.app.services.printer_manager import (
@@ -3572,20 +3573,12 @@ async def stop_print(
     if not client:
         raise HTTPException(400, "Printer not connected")
 
-    success = client.stop_print()
-    if not success:
-        raise HTTPException(500, "Failed to stop print")
-
-    # Mark this printer as user-stopped so on_print_complete reclassifies
-    # the resulting "failed"/"aborted" MQTT status as "cancelled" — otherwise
-    # the HMS heuristic in _dispatch_archive_update mislabels user-cancels
-    # (e.g. the H2D's cancel-sequence module-0x0C HMS) as "Layer shift".
-    try:
-        from backend.app.main import mark_printer_stopped_by_user
+    # Sends the stop and marks the printer as stopped by the user, so the
+    # resulting "failed"/"aborted" status ends the print as cancelled.
+    from backend.app.services.print_control import stop_print_by_user
 
-        mark_printer_stopped_by_user(printer_id)
-    except Exception as _mark_err:
-        logger.warning("Failed to mark printer %s as user-stopped: %s", printer_id, _mark_err)
+    if not stop_print_by_user(printer_id):
+        raise HTTPException(500, "Failed to stop print")
 
     return {"success": True, "message": "Print stop command sent"}
 
@@ -4801,6 +4794,13 @@ async def execute_hms_action(
     if not success:
         raise HTTPException(400, "Failed to execute HMS action")
 
+    if body.action == HMSAction.STOP_PRINTING:
+        # A user stop like the Stop button: mark it before the wait below, so a
+        # completion arriving meanwhile ends the print as cancelled, not failed.
+        from backend.app.services.print_control import mark_stopped_by_user
+
+        mark_stopped_by_user(printer_id)
+
     # Give the printer time to push a state update. The dispatch helper already
     # publishes a pushall after every command, so a fresh status should arrive
     # within ~1s; the default 2.5s covers slower firmware variants without

backend/app/api/routes/webhook.py

@@ -21,6 +21,7 @@ from backend.app.models.print_queue import PrintQueueItem
 from backend.app.models.printer import Printer
 from backend.app.schemas.printer import HMSErrorResponse, hms_error_responses
 from backend.app.services.print_confirmation import confirm_outcome_for_new_queue_item
+from backend.app.services.print_control import stop_print_by_user
 from backend.app.services.printer_manager import printer_manager
 from backend.app.services.queue_position import next_queue_position
 
@@ -235,25 +236,21 @@ async def webhook_start_print(
     return {"message": "Print started", "queue_item_id": queue_item.id}
 
 
-def _stop_current_print(printer_id: int) -> None:
-    """Send the stop command and mark the printer as stopped by the user.
+# States in which a print can be stopped, as with the Stop button on the
+# printer card: printing or paused.
+_STOPPABLE_STATES = ("RUNNING", "PAUSE")
 
-    Cancel and stop are the same MQTT command on Bambu printers. The mark makes
-    the print end as stopped rather than failed, as a stop from the printer
-    card or the queue does (mark_printer_stopped_by_user).
-    """
+
+def _stop_current_print(printer_id: int) -> None:
+    """Stop the print on the user's behalf (cancel and stop are the same command)."""
     try:
-        sent = printer_manager.stop_print(printer_id)
+        sent = stop_print_by_user(printer_id)
     except Exception as e:
         logger.error("Failed to stop print on printer %s: %s", printer_id, e)
         raise HTTPException(status_code=500, detail=str(e))
     if not sent:
         raise HTTPException(status_code=503, detail="Printer not connected")
 
-    from backend.app.main import mark_printer_stopped_by_user
-
-    mark_printer_stopped_by_user(printer_id)
-
 
 @router.post("/printer/{printer_id}/stop")
 async def webhook_stop_print(
@@ -275,7 +272,7 @@ async def webhook_stop_print(
     if not status or not status.connected:
         raise HTTPException(status_code=503, detail="Printer not connected")
 
-    if status.state != "RUNNING":
+    if status.state not in _STOPPABLE_STATES:
         raise HTTPException(status_code=409, detail="No print in progress")
 
     _stop_current_print(printer_id)
@@ -301,7 +298,7 @@ async def webhook_cancel_print(
     if not status or not status.connected:
         raise HTTPException(status_code=503, detail="Printer not connected")
 
-    if status.state not in ["RUNNING", "PAUSE"]:
+    if status.state not in _STOPPABLE_STATES:
         raise HTTPException(status_code=409, detail="No print to cancel")
 
     _stop_current_print(printer_id)

backend/app/services/print_control.py

@@ -0,0 +1,45 @@
+"""Stopping a print on a user's behalf.
+
+Every place a user can stop a print goes through here, so none can send the
+stop without the "stopped by the user" mark: the printer card, the queue and
+the webhook routes call stop_print_by_user; the HMS error dialog's "Stop
+Printing" sends its own stop payload and calls mark_stopped_by_user. Without
+the mark the printer's resulting "failed"/"aborted" status is taken at face
+value: a "print failed" notification goes out, and on some printers the
+cancel sequence's own error is reported as a fault such as a layer shift.
+"""
+
+import logging
+
+from backend.app.services.printer_manager import printer_manager
+
+logger = logging.getLogger(__name__)
+
+
+def mark_stopped_by_user(printer_id: int) -> None:
+    """Record that the user stopped the print on this printer (see module docstring)."""
+    try:
+        from backend.app.main import mark_printer_stopped_by_user
+
+        mark_printer_stopped_by_user(printer_id)
+    except Exception as e:
+        logger.warning("Failed to mark printer %s as user-stopped: %s", printer_id, e)
+
+
+def stop_print_by_user(printer_id: int, *, mark_when_unsent: bool = False) -> bool:
+    """Send the stop command; returns whether it was sent.
+
+    The printer is marked as stopped by the user when the command was sent,
+    or always with ``mark_when_unsent`` (the queue, which closes the job even
+    when the printer is offline, so a "failed" it reports later still reads
+    as the user's cancel). Errors sending the command propagate to the caller.
+    """
+    try:
+        sent = printer_manager.stop_print(printer_id)
+    except Exception:
+        if mark_when_unsent:
+            mark_stopped_by_user(printer_id)
+        raise
+    if sent or mark_when_unsent:
+        mark_stopped_by_user(printer_id)
+    return sent

backend/tests/integration/test_printers_api.py

@@ -1204,17 +1204,23 @@ class TestPrintControlAPI:
         """Verify successful stop print request."""
         printer = await printer_factory(name="Printing Printer")
 
-        mock_client = MagicMock()
-        mock_client.stop_print.return_value = True
+        from backend.app.main import _user_stopped_printers
 
-        with patch("backend.app.api.routes.printers.printer_manager") as mock_pm:
-            mock_pm.get_client.return_value = mock_client
+        _user_stopped_printers.discard(printer.id)
+        with (
+            patch("backend.app.api.routes.printers.printer_manager") as mock_pm,
+            patch("backend.app.services.print_control.printer_manager.stop_print", return_value=True) as send,
+        ):
+            mock_pm.get_client.return_value = MagicMock()
 
             response = await async_client.post(f"/api/v1/printers/{printer.id}/print/stop")
 
             assert response.status_code == 200
             assert response.json()["success"] is True
-            mock_client.stop_print.assert_called_once()
+            send.assert_called_once_with(printer.id)
+        # Stopped by the user, so the printer's "failed" ends the print as cancelled.
+        assert printer.id in _user_stopped_printers
+        _user_stopped_printers.discard(printer.id)
 
     # ========================================================================
     # Pause print endpoint
@@ -5214,3 +5220,57 @@ class TestCoverUsesTheRunningPrintsArchive:
             await async_client.get(f"/api/v1/printers/{printer.id}/cover")
 
         assert threemf.is_file(), "the cover flow deleted the running print's archived 3MF"
+
+
+class TestHMSStopCountsAsUserStop:
+    """ "Stop Printing" in the HMS error dialog is a user stop like the Stop button.
+
+    It sends its own stop payload, but used to skip the stopped-by-user mark,
+    so the print ended as failed ("print failed" notification, and the cancel
+    sequence's own error read as a fault).
+    """
+
+    @staticmethod
+    async def _act(async_client, printer_id: int, action: str):
+        mock_client = MagicMock()
+        mock_client._last_message_time = 100.0
+
+        def _send(*_a, **_kw):
+            mock_client._last_message_time = 100.5
+            return True
+
+        mock_client.execute_hms_action.side_effect = _send
+        with (
+            patch("backend.app.api.routes.printers.printer_manager") as mock_pm,
+            patch("backend.app.api.routes.printers.HMS_ACTION_ACK_WAIT_SECONDS", 0.01),
+        ):
+            mock_pm.get_client.return_value = mock_client
+            return await async_client.post(
+                f"/api/v1/printers/{printer_id}/hms/execute-action",
+                json={"print_error": "0300800A", "action": action, "job_id": None},
+            )
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_stop_printing_marks_the_printer(self, async_client: AsyncClient, printer_factory):
+        from backend.app.main import _user_stopped_printers
+
+        printer = await printer_factory(name="HMS Printer")
+        _user_stopped_printers.discard(printer.id)
+        try:
+            response = await self._act(async_client, printer.id, "STOP_PRINTING")
+            assert response.status_code == 200, response.text
+            assert printer.id in _user_stopped_printers
+        finally:
+            _user_stopped_printers.discard(printer.id)
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_other_hms_actions_do_not_mark(self, async_client: AsyncClient, printer_factory):
+        from backend.app.main import _user_stopped_printers
+
+        printer = await printer_factory(name="HMS Printer 2")
+        _user_stopped_printers.discard(printer.id)
+        response = await self._act(async_client, printer.id, "RESUME_PRINTING")
+        assert response.status_code == 200, response.text
+        assert printer.id not in _user_stopped_printers

backend/tests/integration/test_webhook_printer_status.py

@@ -437,7 +437,10 @@ class TestWebhookStopAndCancelSendTheStop:
 
     @pytest.mark.asyncio
     @pytest.mark.integration
-    @pytest.mark.parametrize(("action", "state"), [("stop", "RUNNING"), ("cancel", "RUNNING"), ("cancel", "PAUSE")])
+    @pytest.mark.parametrize(
+        ("action", "state"),
+        [(action, state) for action in ("stop", "cancel") for state in ("RUNNING", "PAUSE")],
+    )
     async def test_sends_the_stop_and_marks_the_printer(self, async_client, api_key_data, printer_row, action, state):
         from backend.app.main import _user_stopped_printers
 
@@ -458,3 +461,13 @@ class TestWebhookStopAndCancelSendTheStop:
         assert resp.status_code == 503, resp.text
         stop_print.assert_called_once_with(printer_row.id)
         assert printer_row.id not in _user_stopped_printers
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    @pytest.mark.parametrize("action", ["stop", "cancel"])
+    async def test_a_print_still_preparing_is_not_stopped(self, async_client, api_key_data, printer_row, action):
+        """As on the printer card, whose Stop works only while printing or paused."""
+        resp, stop_print = await _send(async_client, api_key_data, printer_row.id, action, "PREPARE", stop_result=True)
+
+        assert resp.status_code == 409
+        stop_print.assert_not_called()

backend/tests/unit/test_print_control.py

@@ -0,0 +1,88 @@
+"""One way to stop a print on a user's behalf.
+
+The printer card, the queue and the two webhook routes each used to send the
+stop and set the "stopped by the user" mark themselves, and one of them forgot
+the mark: the print then ended as failed, with a "print failed" notification
+and, on some printers, a mislabelled error such as a layer shift. They now all
+call print_control.stop_print_by_user, and only it (and the automatic kill
+switch, which is not a user stop) may send a stop command.
+"""
+
+import ast
+from pathlib import Path
+from unittest.mock import patch
+
+import pytest
+
+from backend.app.services.print_control import stop_print_by_user
+
+BACKEND_APP = Path(__file__).resolve().parents[2] / "app"
+
+
+@pytest.fixture
+def marks():
+    from backend.app.main import _user_stopped_printers
+
+    _user_stopped_printers.clear()
+    yield _user_stopped_printers
+    _user_stopped_printers.clear()
+
+
+def test_a_sent_stop_marks_the_printer(marks):
+    with patch("backend.app.services.print_control.printer_manager.stop_print", return_value=True) as send:
+        assert stop_print_by_user(4) is True
+    send.assert_called_once_with(4)
+    assert 4 in marks
+
+
+def test_an_unsent_stop_leaves_no_mark(marks):
+    with patch("backend.app.services.print_control.printer_manager.stop_print", return_value=False):
+        assert stop_print_by_user(4) is False
+    assert 4 not in marks
+
+
+def test_the_queue_can_mark_an_unsent_stop(marks):
+    """An offline printer that later reports the job as failed must still read
+    it as cancelled by the user (the queue closes the job at once)."""
+    with patch("backend.app.services.print_control.printer_manager.stop_print", return_value=False):
+        assert stop_print_by_user(4, mark_when_unsent=True) is False
+    assert 4 in marks
+
+
+def test_a_failing_send_raises_and_marks_only_when_asked(marks):
+    with patch("backend.app.services.print_control.printer_manager.stop_print", side_effect=OSError("mqtt down")):
+        with pytest.raises(OSError):
+            stop_print_by_user(4)
+        assert 4 not in marks
+        with pytest.raises(OSError):
+            stop_print_by_user(4, mark_when_unsent=True)
+    assert 4 in marks
+
+
+# Files allowed to send a stop command themselves: the shared function, the
+# printer manager and MQTT client that implement it, and main.py's kill switch,
+# which stops unauthorised prints automatically (not a user stop, no mark).
+ALLOWED = {
+    "services/print_control.py",
+    "services/printer_manager.py",
+    "services/bambu_mqtt.py",
+    "main.py",
+}
+
+
+def test_no_other_code_sends_a_stop():
+    """Catches a new ``.stop_print(`` call. It cannot see a stop sent as a raw
+    MQTT payload (the HMS dialog's "Stop Printing" in bambu_mqtt.py, whose
+    route marks the stop itself); such a path must call mark_stopped_by_user."""
+    offenders = []
+    for path in sorted(BACKEND_APP.rglob("*.py")):
+        rel = path.relative_to(BACKEND_APP).as_posix()
+        if rel in ALLOWED:
+            continue
+        for node in ast.walk(ast.parse(path.read_text(encoding="utf-8"))):
+            if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr == "stop_print":
+                offenders.append(f"{rel}:{node.lineno}")
+    assert not offenders, (
+        "Stop a print through services.print_control.stop_print_by_user, which also sets the "
+        f"stopped-by-user mark: {offenders}"
+    )