Просмотр исходного кода

Never delete the SD-card file the printer is printing (#3009)

The post-print cleanup picks what to delete from the finished archive.
When a print finished while Bambuddy was stopped and the file was then
reprinted from the printer's screen, startup reconciliation closed the
old archive and the cleanup deleted the file the printer was printing.

The cleanup now skips any candidate that matches the printer's current
gcode_file or job name while the printer is busy. Finished, failed and
idle states hold nothing back, so the ghost-print cleanup is unchanged;
a ghost replay's file is deleted by its own end-of-print cleanup.
The cleanup moves out of on_print_complete into its own function.
maziggy 2 дней назад
Родитель
Сommit
cdb78f1321
3 измененных файлов с 267 добавлено и 100 удалено
  1. 1 0
      CHANGELOG.md
  2. 156 100
      backend/app/main.py
  3. 110 0
      backend/tests/unit/test_sd_cleanup_file_in_use_3009.py

+ 1 - 0
CHANGELOG.md

@@ -117,6 +117,7 @@ All notable changes to Bambuddy will be documented in this file.
 - **The frontend build no longer warns about `path` and `crypto` being externalized for the STEP previewer (#2976)** — `occt-import-js`, the Emscripten build behind STEP previews, requires both modules, but only inside its `ENVIRONMENT_IS_NODE` branches; in the browser it loads its `.wasm` from the URL the preview worker passes and draws randomness from `crypto.getRandomValues`. Vite still externalized both and printed two warnings on every build. `vite.config.ts` now drops exactly those two warnings for that one package through `build.rolldownOptions.onLog`, so an externalization anywhere else, or of any other module, still shows.
 - **The frontend build no longer warns about `path` and `crypto` being externalized for the STEP previewer (#2976)** — `occt-import-js`, the Emscripten build behind STEP previews, requires both modules, but only inside its `ENVIRONMENT_IS_NODE` branches; in the browser it loads its `.wasm` from the URL the preview worker passes and draws randomness from `crypto.getRandomValues`. Vite still externalized both and printed two warnings on every build. `vite.config.ts` now drops exactly those two warnings for that one package through `build.rolldownOptions.onLog`, so an externalization anywhere else, or of any other module, still shows.
 
 
 ### Fixed
 ### Fixed
+- **The SD-card cleanup could delete a file the printer was printing (#3009, reported by @bdwilson)** — If a print finished while Bambuddy was stopped and you then reprinted the same file from the printer's screen, Bambuddy closed the old print when it came back and deleted its file from the SD card, even though the printer was printing that file right then. The cleanup now leaves a file alone while the printer is printing it.
 - **Permissions taken away from a group came back after a restart (#3238, reported by @Minebuddy)** — Turning off **View MakerWorld** and **Import MakerWorld** for a group and restarting Bambuddy turned them back on. Several permissions added in past releases were given to every matching group on each start, not just once on the upgrade that introduced them: MakerWorld, **Clear plate**, stock forecasting and slicer pipelines. Each is now given once, and a permission you remove stays removed. Updating doesn't re-enable anything you have already turned off. The built-in Administrators, Operators and Viewers groups, whose permissions can't be edited, are still kept complete on each start.
 - **Permissions taken away from a group came back after a restart (#3238, reported by @Minebuddy)** — Turning off **View MakerWorld** and **Import MakerWorld** for a group and restarting Bambuddy turned them back on. Several permissions added in past releases were given to every matching group on each start, not just once on the upgrade that introduced them: MakerWorld, **Clear plate**, stock forecasting and slicer pipelines. Each is now given once, and a permission you remove stays removed. Updating doesn't re-enable anything you have already turned off. The built-in Administrators, Operators and Viewers groups, whose permissions can't be edited, are still kept complete on each start.
 - **A virtual printer with Save AMS mapping lost the slicer's external-spool pick (#3237, reported by @erabti)** — The slicer marks a filament fed from the external spool as -1 in its slot list, the same as a filament with no slot, and says which spool it is in a second list. Bambuddy saved only the first list, so the queued print sent that filament as unassigned. On an H2C the printer then stopped before the first layer with 0700-8012, "Failed to get AMS mapping table". The saved mapping now keeps the external spool, left or right on dual-nozzle printers, for the queued print and for reprints from the archive.
 - **A virtual printer with Save AMS mapping lost the slicer's external-spool pick (#3237, reported by @erabti)** — The slicer marks a filament fed from the external spool as -1 in its slot list, the same as a filament with no slot, and says which spool it is in a second list. Bambuddy saved only the first list, so the queued print sent that filament as unassigned. On an H2C the printer then stopped before the first layer with 0700-8012, "Failed to get AMS mapping table". The saved mapping now keeps the external spool, left or right on dual-nozzle printers, for the queued print and for reprints from the archive.
 - **A smart plug that reports energy in watt-seconds could not be set up (reported by @CLKRUN in #1251)** — A myStrom Switch reports energy in watt-seconds, and converting that to kWh needs a multiplier of about 0.000000278. The multiplier fields refused anything below 0.0001, so saving the plug failed. Any multiplier above zero is now accepted, for MQTT and REST plugs alike.
 - **A smart plug that reports energy in watt-seconds could not be set up (reported by @CLKRUN in #1251)** — A myStrom Switch reports energy in watt-seconds, and converting that to kWh needs a multiplier of about 0.000000278. The multiplier fields refused anything below 0.0001, so saving the plug failed. Any multiplier above zero is now accepted, for MQTT and REST plugs alike.

+ 156 - 100
backend/app/main.py

@@ -6376,6 +6376,46 @@ async def prime_kprofile_table(printer_id: int) -> int:
     return primed
     return primed
 
 
 
 
+def _sd_files_in_use(state) -> set[str]:
+    """Names of the SD-card files the printer may be printing right now (#3009).
+
+    The post-print cleanup works out what to delete from the finished archive,
+    not from the printer. When reconciliation closes an old archive because the
+    printer has moved on to a new job, and that job is the same file reprinted
+    from the printer's screen, the cleanup would delete the file mid-print.
+
+    Empty unless the printer is busy: on an ordinary completion the state is
+    FINISH or FAILED and nothing is held back. ``gcode_file`` arrives as a bare
+    name, a path or a URL depending on firmware, and some report only the
+    plate's G-code, so the job name is matched as well, in the same spellings
+    the cleanup tries. Names are lower-cased; the card's FAT filesystem
+    ignores case.
+    """
+
+    def _text(value) -> str:
+        return value.strip() if isinstance(value, str) else ""
+
+    if state is None:
+        return set()
+    current_state = _text(getattr(state, "state", None)).upper()
+    if current_state in ("", "UNKNOWN", "IDLE", "FINISH", "FAILED"):
+        return set()
+    in_use: set[str] = set()
+    gcode_file = _text(getattr(state, "gcode_file", None))
+    if gcode_file:
+        # Drop a scheme by hand: urlparse would cut a name at "#" or "?",
+        # both legal in a file name, and raises on some inputs.
+        name = PurePosixPath(gcode_file.split("://", 1)[-1]).name
+        if name:
+            in_use.add(name.lower())
+    subtask_name = _text(getattr(state, "subtask_name", None))
+    if subtask_name:
+        for name in (subtask_name, subtask_name.replace(" ", "_")):
+            for ext in (".3mf", ".gcode"):
+                in_use.add(f"{name}{ext}".lower())
+    return in_use
+
+
 async def reconcile_stale_active_prints(printer_id: int) -> int:
 async def reconcile_stale_active_prints(printer_id: int) -> int:
     """Synthesise ``on_print_complete`` for archives whose print can't be
     """Synthesise ``on_print_complete`` for archives whose print can't be
     running on the printer anymore.
     running on the printer anymore.
@@ -7029,6 +7069,121 @@ async def _recover_fallback_from_cache_before_eviction(printer_id: int, data: di
             logger.debug("[RECOVER] Pre-eviction recovery for %s failed: %s", name, e)
             logger.debug("[RECOVER] Pre-eviction recovery for %s failed: %s", name, e)
 
 
 
 
+async def _cleanup_sd_card_after_print(
+    printer_id: int, subtask_name: str | None, archive_id: int | None, logger
+) -> None:
+    """Delete a finished print's file from the printer's SD card."""
+    # Cleanup: delete uploaded file from printer SD card to prevent phantom prints (Issue #374, #1542)
+    # The print scheduler uploads files to the SD card root (/). Some printers (e.g. P1S, A1)
+    # auto-start files found in root on power cycle, causing ghost prints.
+    # Must run before the archive_id early-return so it executes even when archiving is disabled.
+    try:
+        if subtask_name:
+            archive_filename: str | None = None
+            async with async_session() as db:
+                from backend.app.models.archive import PrintArchive
+                from backend.app.models.printer import Printer
+
+                result = await db.execute(select(Printer).where(Printer.id == printer_id))
+                printer = result.scalar_one_or_none()
+                if archive_id:
+                    archive_row = await db.execute(select(PrintArchive.filename).where(PrintArchive.id == archive_id))
+                    archive_filename = archive_row.scalar_one_or_none()
+
+            if printer:
+                from backend.app.services.bambu_ftp import DeleteResult, delete_file_async
+                from backend.app.utils.filename import derive_remote_filename
+
+                # Primary candidate: the exact path the dispatcher uploaded to
+                # (derived from archive.filename via the same rule as upload).
+                # Without it, a library row that ended up with a doubled
+                # .gcode.3mf (#1542) leaves the real file behind because the
+                # subtask_name + ext fallbacks below don't match what's on the
+                # SD card. Fallbacks remain for archive-less prints (subtask
+                # never resolved to an archive) and for older naming variants.
+                candidate_paths: list[str] = []
+                if archive_filename:
+                    candidate_paths.append(f"/{derive_remote_filename(archive_filename)}")
+                for ext in (".3mf", ".gcode"):
+                    fallback = f"/{subtask_name}{ext}"
+                    if fallback not in candidate_paths:
+                        candidate_paths.append(fallback)
+
+                # Three outcomes track across all candidates so the final log
+                # line reflects what actually happened. The A1 in #1721 always
+                # ends here with ``any_not_found=True`` and the others False
+                # — its firmware auto-cleans the SD card before our cleanup
+                # runs, every candidate FTP-DELE returns 550, and the old
+                # code burned 3 retries × 2 s × 3 candidates per print
+                # logging a misleading "may linger" WARNING on a successful
+                # print.
+                any_deleted = False
+                any_real_failure = False
+                any_not_found = False
+
+                files_in_use = _sd_files_in_use(printer_manager.get_status(printer_id))
+
+                for remote_path in candidate_paths:
+                    if PurePosixPath(remote_path).name.lower() in files_in_use:
+                        logger.info(
+                            "SD card cleanup: keeping %s on printer %s, which is printing it now",
+                            remote_path,
+                            printer.name,
+                        )
+                        continue
+                    # Retry only the FAILED case — 550 NOT_FOUND will never
+                    # recover by waiting, so a "file isn't here" answer
+                    # advances immediately to the next candidate without
+                    # consuming the retry budget.
+                    for attempt in range(1, 4):
+                        try:
+                            delete_result = await delete_file_async(
+                                printer.ip_address,
+                                printer.access_code,
+                                remote_path,
+                                printer_model=printer.model,
+                            )
+                        except Exception as e:
+                            delete_result = DeleteResult.FAILED
+                            logger.warning(
+                                "SD card cleanup attempt %d/3 raised for %s: %s",
+                                attempt,
+                                remote_path,
+                                e,
+                            )
+
+                        if delete_result == DeleteResult.DELETED:
+                            any_deleted = True
+                            logger.info("Deleted %s from printer %s SD card", remote_path, printer.name)
+                            break
+                        if delete_result == DeleteResult.NOT_FOUND:
+                            any_not_found = True
+                            break  # 550 will not recover; try next candidate
+                        # FAILED: real error — retry with backoff, then give up
+                        if attempt < 3:
+                            await asyncio.sleep(2)
+                        else:
+                            any_real_failure = True
+                            logger.warning(
+                                "SD card cleanup failed after 3 attempts for %s "
+                                "(network/auth/transient error — file may linger on SD card)",
+                                remote_path,
+                            )
+
+                if not any_deleted and not any_real_failure and any_not_found:
+                    # Every candidate said "not here." Either the printer
+                    # firmware swept the SD card itself (common on A1) or the
+                    # dispatcher's upload path doesn't match our candidate
+                    # rule. Either way: nothing to clean up, no warning.
+                    logger.debug(
+                        "SD card cleanup: nothing to delete on %s — every candidate returned 550 "
+                        "(printer likely self-cleaned)",
+                        printer.name,
+                    )
+    except Exception as e:
+        logger.warning("SD card file cleanup failed for printer %s: %s", printer_id, e)
+
+
 async def on_print_complete(printer_id: int, data: dict):
 async def on_print_complete(printer_id: int, data: dict):
     """Handle print completion - update the archive status."""
     """Handle print completion - update the archive status."""
     import time
     import time
@@ -7222,106 +7377,7 @@ async def on_print_complete(printer_id: int, data: dict):
                 if archive:
                 if archive:
                     archive_id = archive.id
                     archive_id = archive.id
 
 
-    # Cleanup: delete uploaded file from printer SD card to prevent phantom prints (Issue #374, #1542)
-    # The print scheduler uploads files to the SD card root (/). Some printers (e.g. P1S, A1)
-    # auto-start files found in root on power cycle, causing ghost prints.
-    # Must run before the archive_id early-return so it executes even when archiving is disabled.
-    try:
-        if subtask_name:
-            archive_filename: str | None = None
-            async with async_session() as db:
-                from backend.app.models.archive import PrintArchive
-                from backend.app.models.printer import Printer
-
-                result = await db.execute(select(Printer).where(Printer.id == printer_id))
-                printer = result.scalar_one_or_none()
-                if archive_id:
-                    archive_row = await db.execute(select(PrintArchive.filename).where(PrintArchive.id == archive_id))
-                    archive_filename = archive_row.scalar_one_or_none()
-
-            if printer:
-                from backend.app.services.bambu_ftp import DeleteResult, delete_file_async
-                from backend.app.utils.filename import derive_remote_filename
-
-                # Primary candidate: the exact path the dispatcher uploaded to
-                # (derived from archive.filename via the same rule as upload).
-                # Without it, a library row that ended up with a doubled
-                # .gcode.3mf (#1542) leaves the real file behind because the
-                # subtask_name + ext fallbacks below don't match what's on the
-                # SD card. Fallbacks remain for archive-less prints (subtask
-                # never resolved to an archive) and for older naming variants.
-                candidate_paths: list[str] = []
-                if archive_filename:
-                    candidate_paths.append(f"/{derive_remote_filename(archive_filename)}")
-                for ext in (".3mf", ".gcode"):
-                    fallback = f"/{subtask_name}{ext}"
-                    if fallback not in candidate_paths:
-                        candidate_paths.append(fallback)
-
-                # Three outcomes track across all candidates so the final log
-                # line reflects what actually happened. The A1 in #1721 always
-                # ends here with ``any_not_found=True`` and the others False
-                # — its firmware auto-cleans the SD card before our cleanup
-                # runs, every candidate FTP-DELE returns 550, and the old
-                # code burned 3 retries × 2 s × 3 candidates per print
-                # logging a misleading "may linger" WARNING on a successful
-                # print.
-                any_deleted = False
-                any_real_failure = False
-                any_not_found = False
-
-                for remote_path in candidate_paths:
-                    # Retry only the FAILED case — 550 NOT_FOUND will never
-                    # recover by waiting, so a "file isn't here" answer
-                    # advances immediately to the next candidate without
-                    # consuming the retry budget.
-                    for attempt in range(1, 4):
-                        try:
-                            delete_result = await delete_file_async(
-                                printer.ip_address,
-                                printer.access_code,
-                                remote_path,
-                                printer_model=printer.model,
-                            )
-                        except Exception as e:
-                            delete_result = DeleteResult.FAILED
-                            logger.warning(
-                                "SD card cleanup attempt %d/3 raised for %s: %s",
-                                attempt,
-                                remote_path,
-                                e,
-                            )
-
-                        if delete_result == DeleteResult.DELETED:
-                            any_deleted = True
-                            logger.info("Deleted %s from printer %s SD card", remote_path, printer.name)
-                            break
-                        if delete_result == DeleteResult.NOT_FOUND:
-                            any_not_found = True
-                            break  # 550 will not recover; try next candidate
-                        # FAILED: real error — retry with backoff, then give up
-                        if attempt < 3:
-                            await asyncio.sleep(2)
-                        else:
-                            any_real_failure = True
-                            logger.warning(
-                                "SD card cleanup failed after 3 attempts for %s "
-                                "(network/auth/transient error — file may linger on SD card)",
-                                remote_path,
-                            )
-
-                if not any_deleted and not any_real_failure and any_not_found:
-                    # Every candidate said "not here." Either the printer
-                    # firmware swept the SD card itself (common on A1) or the
-                    # dispatcher's upload path doesn't match our candidate
-                    # rule. Either way: nothing to clean up, no warning.
-                    logger.debug(
-                        "SD card cleanup: nothing to delete on %s — every candidate returned 550 "
-                        "(printer likely self-cleaned)",
-                        printer.name,
-                    )
-    except Exception as e:
-        logger.warning("SD card file cleanup failed for printer %s: %s", printer_id, e)
+    await _cleanup_sd_card_after_print(printer_id, subtask_name, archive_id, logger)
 
 
     log_timing("SD card cleanup")
     log_timing("SD card cleanup")
 
 

+ 110 - 0
backend/tests/unit/test_sd_cleanup_file_in_use_3009.py

@@ -0,0 +1,110 @@
+"""The post-print SD cleanup never deletes the file the printer is printing (#3009).
+
+The cleanup works out what to delete from the finished archive. When
+reconciliation closes an old archive because the printer has started a new
+job, and that job is the same file reprinted from the printer's screen, the
+cleanup deleted the file of the print that was running.
+"""
+
+import logging
+from pathlib import PurePosixPath
+from types import SimpleNamespace
+from unittest.mock import AsyncMock, MagicMock, patch
+
+import pytest
+
+from backend.app.main import _cleanup_sd_card_after_print, _sd_files_in_use
+from backend.app.services.bambu_ftp import DeleteResult
+
+
+def _state(state="RUNNING", gcode_file="", subtask_name=""):
+    return SimpleNamespace(state=state, gcode_file=gcode_file, subtask_name=subtask_name)
+
+
+class TestFilesInUse:
+    @pytest.mark.parametrize("state", ["IDLE", "FINISH", "FAILED", "", "unknown"])
+    def test_nothing_is_held_back_when_the_printer_is_not_busy(self, state):
+        assert _sd_files_in_use(_state(state, "cube.3mf", "cube")) == set()
+
+    def test_no_state(self):
+        assert _sd_files_in_use(None) == set()
+
+    @pytest.mark.parametrize("state", ["RUNNING", "PAUSE", "PREPARE", "SLICING"])
+    def test_busy_states(self, state):
+        assert "cube.3mf" in _sd_files_in_use(_state(state, "cube.3mf"))
+
+    @pytest.mark.parametrize(
+        "gcode_file",
+        [
+            "Cube_Part.3mf",
+            "/Cube_Part.3mf",
+            "/sdcard/Cube_Part.3mf",
+            "ftp://Cube_Part.3mf",
+            "file:///sdcard/Cube_Part.3mf",
+        ],
+    )
+    def test_gcode_file_spellings(self, gcode_file):
+        assert "cube_part.3mf" in _sd_files_in_use(_state(gcode_file=gcode_file))
+
+    @pytest.mark.parametrize("name", ["Part #2.3mf", "What?.3mf", "ftp://[odd.3mf"])
+    def test_awkward_names_are_kept_whole(self, name):
+        assert PurePosixPath(name.split("://", 1)[-1]).name.lower() in _sd_files_in_use(_state(gcode_file=name))
+
+    def test_unexpected_values_do_not_raise(self):
+        assert _sd_files_in_use(SimpleNamespace(state="RUNNING", gcode_file=None, subtask_name=None)) == set()
+        assert _sd_files_in_use(SimpleNamespace(state=MagicMock(), gcode_file=MagicMock(), subtask_name=1)) == set()
+
+    def test_job_name_covers_firmware_that_reports_only_the_plate_gcode(self):
+        in_use = _sd_files_in_use(_state(gcode_file="/data/Metadata/plate_1.gcode", subtask_name="My Cube"))
+        assert {"my cube.3mf", "my_cube.3mf", "my cube.gcode", "my_cube.gcode"} <= in_use
+
+
+async def _run_cleanup(state, *, archive_filename, subtask_name):
+    printer = SimpleNamespace(id=1, name="A1", ip_address="192.0.2.10", access_code="12345678", model="A1")
+    db = AsyncMock()
+    db.execute = AsyncMock(
+        side_effect=[
+            MagicMock(scalar_one_or_none=MagicMock(return_value=printer)),
+            MagicMock(scalar_one_or_none=MagicMock(return_value=archive_filename)),
+        ]
+    )
+    session_ctx = AsyncMock()
+    session_ctx.__aenter__ = AsyncMock(return_value=db)
+    session_ctx.__aexit__ = AsyncMock(return_value=False)
+    manager = MagicMock()
+    manager.get_status = MagicMock(return_value=state)
+    delete = AsyncMock(return_value=DeleteResult.DELETED)
+    with (
+        patch("backend.app.main.async_session", MagicMock(return_value=session_ctx)),
+        patch("backend.app.main.printer_manager", manager),
+        patch("backend.app.services.bambu_ftp.delete_file_async", delete),
+    ):
+        await _cleanup_sd_card_after_print(1, subtask_name, 342, logging.getLogger("test"))
+    return [call.args[2] for call in delete.await_args_list]
+
+
+@pytest.mark.asyncio
+async def test_reprint_from_the_printer_keeps_its_file():
+    """The reporter's case: the A1 reprints the file from its screen, startup
+    reconciliation closes the old archive and runs the cleanup."""
+    state = _state("RUNNING", "A1-Siraya_Tech_TPU-64D-18JAN26.3mf", "A1-Siraya Tech TPU-64D-18JAN26")
+    deleted = await _run_cleanup(
+        state,
+        archive_filename="A1-Siraya Tech TPU-64D-18JAN26.gcode.3mf",
+        subtask_name="A1-Siraya Tech TPU-64D-18JAN26",
+    )
+    assert "/A1-Siraya_Tech_TPU-64D-18JAN26.3mf" not in deleted
+
+
+@pytest.mark.asyncio
+async def test_a_finished_print_is_still_deleted():
+    state = _state("FINISH", "cube.3mf", "cube")
+    deleted = await _run_cleanup(state, archive_filename="cube.gcode.3mf", subtask_name="cube")
+    assert "/cube.3mf" in deleted
+
+
+@pytest.mark.asyncio
+async def test_a_different_running_job_does_not_protect_the_finished_file():
+    state = _state("RUNNING", "other.3mf", "other")
+    deleted = await _run_cleanup(state, archive_filename="cube.gcode.3mf", subtask_name="cube")
+    assert "/cube.3mf" in deleted