From bf781e05665fa0b412a98764f638673f2a3c4d23 Mon Sep 17 00:00:00 2001 From: maziggy Date: Thu, 11 Jun 2026 09:23:03 +0200 Subject: [PATCH] fix(notifications): reprint-from-archive sent original print's finish photo (#1707) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reprints reuse the source archive row via the expected-print promotion branch, but that branch never reset archive.timelapse_path. The stale path made _scan_for_timelapse_with_retries early-return (so the new run's MP4 was never downloaded), and _capture_finish_photo_from_timelapse then extracted the *original* run's last frame and shipped it to Telegram as the new run's finish photo. Surface was specific to the timelapse-prefer path (data.timelapse_was_active=true and no external camera) — fallback live-camera paths were unaffected, which is why this took a P2S reporter to surface. Clear archive.timelapse_path at promotion and unlink the stale on-disk MP4 (best-effort; missing files are logged and skipped). The orphan unlink also kills a long-standing file-leak: every reprint used to leave its predecessor's timelapse on disk forever. archive.photos is deliberately left alone — accumulating one finish photo per run is correct. --- CHANGELOG.md | 2 + backend/app/main.py | 27 ++ .../test_reprint_clears_stale_timelapse.py | 326 ++++++++++++++++++ 3 files changed, 355 insertions(+) create mode 100644 backend/tests/unit/test_reprint_clears_stale_timelapse.py diff --git a/CHANGELOG.md b/CHANGELOG.md index b0e51f87a..fd6b3d557 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,8 @@ All notable changes to Bambuddy will be documented in this file. - **Print Log page: per-row delete (#1687 part 1, reported by @IndividualGhost1905)** — Reporter noted that the existing "Also remove this print from Quick Stats" toggle on archive delete is one-shot: if you tick "keep stats" at delete time, there was no later way to drop the row from /stats; and rows that aren't tied to an archive (errors, aborts, manual entries) had no delete affordance at all. **Fix:** every row in the Archives → Print Log table now has a trash icon next to the filament cell, gated on `archives:delete_own` (own rows) or `archives:delete_all` (any row), matching the archive-delete permission shape. Click → confirm modal → row is gone, and because /archives/stats aggregates over `PrintLogEntry` the filament / time / cost contribution drops out of Quick Stats in the same response cycle. The matching archive (if any) is untouched — the log row is a sibling, not a child. **Backend:** new `DELETE /print-log/{entry_id}` mirrors `delete_archive`'s ownership flow via `require_ownership_permission(ARCHIVES_DELETE_ALL, ARCHIVES_DELETE_OWN)`; owners can drop their own rows, admins can drop any row, missing IDs return 404 rather than 200-silently. **Frontend:** new `deletePrintLogEntry` API helper, per-row mutation that invalidates both `print-log` and `archives-stats` query keys so the totals re-render without a manual refresh. **i18n:** 4 new keys (`deleteEntryTitle`, `deleteEntryConfirm`, `entryDeleted`, `entryDeleteFailed`) translated across all 11 locales (de / en / es / fr / it / ja / ko / pt-BR / tr / zh-CN / zh-TW). **Tests:** 3 backend integration cases — delete drops the row from /stats while keeping the linked archive listed, missing ID returns 404, delete-one does not touch siblings (regression guard against an accidental `delete(PrintLogEntry)` without a `where`). Frontend ArchivesPage / PrintLogModal vitests stay green (31 / 31). i18n parity green (5099 leaves × 11 locales). Issue #1687 also asks for per-row tagging (already covered by `EditArchiveModal`'s tags field) and per-row filament-usage-history edits (deferred — see the issue thread for the reasoning). ### Fixed +- **Telegram (and other image-bearing) finish notification on a reprint-from-archive showed the original print's finish photo instead of the new run's (#1707, reported by @kycrna)** — P2S user reprinted an archived job and observed the Telegram notification arriving with the photo of the *original* print (white box) attached to the completion message for the *new* run (black box). **Root cause:** reprints reuse the source archive row — `register_expected_print` stores the source `archive_id` in `_expected_prints`, and the on-print-start expected-archive promotion branch at `main.py:2207-2245` updates the row's status / started_at / printer_id / subtask_id but never reset `archive.timelapse_path`. Two failure modes cascaded from the stale path: (a) `_scan_for_timelapse_with_retries` early-returns at `main.py:3062` with `if archive.timelapse_path: return` — the reprint's new timelapse MP4 sitting on the printer's SD card was never downloaded, the archive's `timelapse_path` kept pointing at the original run's local file; (b) `_capture_finish_photo_from_timelapse` polls `archive.timelapse_path` and immediately found the *original* video, extracted ITS last frame as `finish__.jpg`, and handed those bytes to `_background_notifications` as `image_data` — which then went out to Telegram via the `sendPhoto` path. The filename was new (so log lines and the archive's `photos` list looked correct), but the pixels were the original run's finish frame. Surface was specific to the timelapse-prefer path: with `data.timelapse_was_active` true and no external camera, `prefer_timelapse_source` was True, which is the exact configuration on P2S with timelapse-on for both runs. External-camera, buffered-frame, and fresh-RTSP fallback paths grab the *current* camera frame, so users on those paths saw correct photos and the bug stayed hidden. **Fix:** at expected-archive promotion, capture and clear `archive.timelapse_path` to None before the commit, and `os.unlink` the stale on-disk video so reprints don't accumulate orphaned MP4s in the archive directory. Photos list is left alone — accumulating one finish photo per run across the archive's lifetime is the right behaviour. The unlink is wrapped in `OSError`-catching best-effort logging so a missing file (manual delete, archive purge, container rebuild with bind-mount drift) doesn't break promotion. The clear-and-unlink runs unconditionally when `timelapse_path` is set, so even if a user has been reprinting under the buggy build for months, the next reprint self-heals. **Tests:** 3 new cases in `test_reprint_clears_stale_timelapse.py` exercise the full `on_print_start` callback through the expected-archive branch — happy path (path cleared + file unlinked), no-prior-timelapse (no-op, promotion still succeeds), missing-stale-file (best-effort unlink doesn't raise). Full `test_print_start_expected_promotion.py` + `test_print_start_assigns_printer_id_to_vp_archive.py` suite (28/28) stays green; ruff clean. + - **Connection diagnostic no longer flags `external_storage: fail` on A1 / A1 Mini, which physically have no MicroSD slot (#1703, reported by @MartinNYHC)** — Bug report from an A1 user complained that BambuStudio and OrcaSlicer don't have an "external storage" tick box (correct — there's nothing to toggle, the A1 series ships without a SD card slot at all) while the Bambuddy support bundle simultaneously reported `external_storage: fail` in the printer's connection diagnostic. The two together left the user thinking Bambuddy was wrong about a setting their hardware doesn't have. **Root cause:** the `external_storage` check at `services/printer_diagnostic.py:179-189` reads `state.store_to_sdcard`, which is parsed from MQTT `home_flag` bit 11. On A1 and A1 Mini that bit is never set (no hardware slot, no firmware-side toggle, no slicer-side equivalent), so the value pushes as `False` and the check fell through to `fail` instead of `skip`. **Fix:** new `NO_EXTERNAL_STORAGE_MODELS` frozenset in `utils/printer_models.py` enumerating A1, A1 Mini, and their internal codes (N1, N2S, A04, A11, A12), plus a `has_external_storage(model)` helper that returns False for those and True for everything else (unknown models default to True so the check stays active for any future Bambu model that ships *with* a slot — new no-slot models must be added to the set explicitly). The diagnostic now short-circuits to `skip` before reading `store_to_sdcard` when `printer.model` is in the set. **What this does NOT change:** X1 / X1E / P1S / P1P / P2S / H2D / H2D Pro / H2C / H2S / X2D continue to evaluate `store_to_sdcard` exactly as before — the home-flag-bit-off → `fail` path is still the right signal for them. **The companion FTP-upload-timeout symptom in the same bug report (ftp code 28 from BambuStudio when sending to the proxy VP) is a separate Docker-bridge-mode networking constraint, not addressed by this change.** **Tests:** 8 new cases — `TestHasExternalStorage` (5 cases) pins the model list, internal-code aliasing, case/whitespace normalisation, unknown-defaults-true, and null/empty-defaults-true; `TestExternalStorageCheck` gains `test_skips_on_a1_no_external_storage_slot`, `test_skips_on_a1_mini_no_external_storage_slot`, and `test_still_fails_on_x1c_when_toggle_off` (regression guard that the model-aware skip doesn't accidentally silence the genuine signal on slotted models). Full `test_printer_models.py` + `test_printer_diagnostic.py` + archives integration suite green (172/172); ruff clean. - **AMS slot card surfaced the previous spool's preset name after RFID auto-assigned a new spool (reported with H2D-1 / AMS-B3 / PLA-CF showing as "Bambu PLA Silk+")** — Reporter inserted a fresh Bambu PLA-CF spool into AMS-B3, RFID identified it correctly, but the slot card kept showing "Bambu PLA Silk+" (the name from a PLA Silk+ spool that had occupied the slot back in March). Confirmed in the live data: `slot_preset_mappings` row for `(printer_id=1, ams_id=1, tray_id=2)` was `preset_id=GFSA06_09, preset_name='Bambu PLA Silk+', updated_at=2026-03-15` — three months stale. **Root cause:** `slot_preset_mappings.preset_name` is first in the PrintersPage display chain (`PrintersPage.tsx:3624`) and overrides the spool's own `slicer_filament_name` plus the cloud catalog `cloudInfo.name`. The internal-mode manual-assign path (`inventory.apply_spool_to_slot_via_mqtt`) kept this row in sync, but the internal-mode RFID auto-assign path (`spool_tag_matcher.auto_assign_spool`) skipped it entirely. The Spoolman-mode sync path (`main.auto_sync_spoolman_ams_trays`) also skipped it — same bug shape, latent for Spoolman users who'd never manually configured a slot preset, active for those who had. **Fix — three writers in lockstep via one shared helper.** New `backend/app/services/slot_preset_writer.py` exposes a primitive `upsert_slot_preset` plus two convenience wrappers: `upsert_slot_preset_for_spool` for internal `Spool` ORM objects (local-preset numeric ids → `local_{n}`, cloud ids run through `filament_id_to_setting_id`) and `upsert_slot_preset_for_spoolman_spool` for Spoolman dicts (filament.name → preset_name, tray_info_idx → preset_id). All three call sites — the manual-assign block in `inventory.py:396-438`, the RFID auto-assign tail in `spool_tag_matcher.py:auto_assign_spool`, and the per-tray-sync branch in `main.py:auto_sync_spoolman_ams_trays` — now go through the helper. **Self-heal:** existing stale rows from past spool swaps get rewritten the next time a fresh spool is detected on the same slot. No migration script needed. **What this also covers per `feedback_inventory_modes_parity`:** the bug shape exists in both internal and Spoolman modes, so the patch ships fixes for both inventory paths in the same drop — a Spoolman user with a manually-configured slot preset would have seen the same stale-name behavior after every RFID swap until the row was overwritten through Configure Slot. **Tests:** new `test_slot_preset_writer.py` (6 cases) pins the helper contracts — no-op on empty preset_id, upsert idempotency, Spoolman filament.name → preset_name, fallback to material → tray_sub_brands → tray_type, stale-row overwrite from the Spoolman path, skip when tray_info_idx is unknown. New `test_spool_tag_matcher.py` cases (3) pin the internal RFID-auto-assign path — stale-row overwrite (the exact reporter shape: PLA Silk+ → PLA-CF), fresh insert when no row exists, `local_{n}` formatting for numeric local-preset ids. Total touched-area suite 69/69 green; broader related suite (inventory + spoolman + spool_tag + auto_sync) 767/767 green; ruff clean. diff --git a/backend/app/main.py b/backend/app/main.py index 589f84d7a..206b43418 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -2216,6 +2216,33 @@ async def on_print_start(printer_id: int, data: dict): # Update archive status to printing archive.status = "printing" archive.started_at = datetime.now(timezone.utc) + + # Reprint of an archive reuses the source row. Without resetting + # ``timelapse_path`` _scan_for_timelapse_with_retries early-returns + # ("already has timelapse") and _capture_finish_photo_from_timelapse + # extracts the *original* print's last frame, which then ships in + # the completion notification (#1707). Clear the path so the + # scanner runs fresh; also unlink the old video file so reprints + # don't accumulate orphans in the archive directory. Photos list + # is left alone — accumulating one finish photo per run is fine. + stale_timelapse_relpath = archive.timelapse_path + if stale_timelapse_relpath: + archive.timelapse_path = None + try: + stale_path = app_settings.base_dir / stale_timelapse_relpath + if stale_path.is_file(): + stale_path.unlink() + logger.info( + "Deleted stale timelapse %s on reprint of archive %s", + stale_timelapse_relpath, + expected_archive_id, + ) + except OSError as e: + logger.warning( + "Failed to delete stale timelapse %s on reprint: %s", + stale_timelapse_relpath, + e, + ) # Persist a restart-stable id so a later restart resumes this # archive by subtask_id instead of name-matching + duplicating # it (#1485). The printer often hasn't echoed subtask_id back diff --git a/backend/tests/unit/test_reprint_clears_stale_timelapse.py b/backend/tests/unit/test_reprint_clears_stale_timelapse.py new file mode 100644 index 000000000..82c36e01f --- /dev/null +++ b/backend/tests/unit/test_reprint_clears_stale_timelapse.py @@ -0,0 +1,326 @@ +"""Regression for #1707: Telegram (and any image-bearing) notification on a +reprint from archive showed the *original* print's finish photo because the +expected-archive branch never reset ``archive.timelapse_path``. + +The source archive row is reused for reprints. With ``timelapse_path`` still +pointing at the original run's downloaded MP4: + - ``_scan_for_timelapse_with_retries`` early-returns ("already has timelapse") + and never downloads the reprint's video. + - ``_capture_finish_photo_from_timelapse`` reads the stale path, extracts the + *original* last frame, and ships it as the reprint's finish photo. + +The fix clears ``archive.timelapse_path`` (and unlinks the stale file) at +expected-archive promotion so the scan + photo path run fresh. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from backend.app.core.config import settings as app_settings +from backend.app.main import ( + _active_prints, + _expected_print_creators, + _expected_print_registered_at, + _expected_prints, + _print_ams_mappings, + _timelapse_baselines, + register_expected_print, +) + + +@pytest.fixture(autouse=True) +def _clear_dicts(): + _expected_prints.clear() + _expected_print_registered_at.clear() + _expected_print_creators.clear() + _print_ams_mappings.clear() + _active_prints.clear() + _timelapse_baselines.clear() + yield + _expected_prints.clear() + _expected_print_registered_at.clear() + _expected_print_creators.clear() + _print_ams_mappings.clear() + _active_prints.clear() + _timelapse_baselines.clear() + + +def _patches(): + """Common patches for driving on_print_start without side effects.""" + return ( + patch("backend.app.main.async_session"), + patch("backend.app.main.notification_service"), + patch("backend.app.main.smart_plug_manager"), + patch("backend.app.main.ws_manager"), + patch("backend.app.main.printer_manager"), + patch("backend.app.main.mqtt_relay"), + patch("backend.app.main._record_energy_start", new_callable=AsyncMock), + patch("backend.app.main._load_objects_from_archive"), + patch("backend.app.main._store_spoolman_print_data", new_callable=AsyncMock), + patch("backend.app.main._send_print_start_notification", new_callable=AsyncMock), + patch( + "backend.app.main._list_timelapse_videos", + new=AsyncMock(return_value=([], "/timelapse")), + ), + ) + + +def _build_mocks(mock_printer, mock_archive): + def execute_router(stmt, *args, **kwargs): + sql = str(stmt).lower() + if "from printers" in sql or "from printer " in sql: + return MagicMock( + scalar_one_or_none=MagicMock(return_value=mock_printer), + scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[mock_printer]))), + ) + if "from print_archives" in sql or "from print_archive" in sql: + return MagicMock( + scalar_one_or_none=MagicMock(return_value=mock_archive), + scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[mock_archive]))), + ) + return MagicMock( + scalar_one_or_none=MagicMock(return_value=None), + scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[]))), + ) + + mock_session = AsyncMock() + mock_session.__aenter__ = AsyncMock(return_value=mock_session) + mock_session.__aexit__ = AsyncMock() + mock_session.execute = AsyncMock(side_effect=execute_router) + mock_session.commit = AsyncMock() + return mock_session + + +@pytest.mark.asyncio +async def test_reprint_clears_timelapse_path_and_unlinks_stale_file(tmp_path): + """On reprint promotion, timelapse_path must be reset to None and the old + on-disk video unlinked, so the completion-time scanner and finish-photo + extractor don't reuse the original run's frame.""" + mock_printer = MagicMock() + mock_printer.id = 1 + mock_printer.auto_archive = True + mock_printer.external_camera_enabled = False + mock_printer.external_camera_url = None + mock_printer.name = "TestP2S" + + # Lay down a fake stale timelapse under a tmp base_dir so the unlink + # actually has a file to remove. + relpath = "archives/42/timelapse/original.mp4" + stale_file = tmp_path / relpath + stale_file.parent.mkdir(parents=True, exist_ok=True) + stale_file.write_bytes(b"old timelapse bytes") + assert stale_file.exists() + + mock_archive = MagicMock() + mock_archive.id = 42 + mock_archive.filename = "MyModel.3mf" + mock_archive.subtask_id = None + mock_archive.print_time_seconds = None + mock_archive.created_by_id = None + mock_archive.printer_id = 1 + mock_archive.print_name = "MyModel" + mock_archive.status = "archived" + mock_archive.file_path = "archives/42/MyModel.3mf" + mock_archive.energy_start_kwh = None + mock_archive.timelapse_path = relpath # stale from the original run + + register_expected_print(1, "MyModel.3mf", archive_id=42, ams_mapping=None) + + mock_session = _build_mocks(mock_printer, mock_archive) + + ( + async_session_p, + notif_p, + plug_p, + ws_p, + pm_p, + relay_p, + _energy, + _load_obj, + _store_spoolman, + _send_start, + _list_tl, + ) = _patches() + + with ( + async_session_p as mock_session_maker, + notif_p as mock_notif, + plug_p as mock_plug, + ws_p as mock_ws, + pm_p as mock_pm, + relay_p as mock_relay, + _energy, + _load_obj, + _store_spoolman, + _send_start, + _list_tl, + patch.object(app_settings, "base_dir", tmp_path), + ): + mock_session_maker.return_value = mock_session + mock_notif.on_print_start = AsyncMock() + mock_plug.on_print_start = AsyncMock() + mock_ws.send_print_start = AsyncMock() + mock_ws.send_archive_updated = AsyncMock() + mock_relay.on_print_start = AsyncMock() + mock_pm.get_printer = MagicMock(return_value=MagicMock(name="Test", serial_number="TEST123")) + + from backend.app.main import on_print_start + + await on_print_start(1, {"filename": "MyModel.3mf", "subtask_name": "MyModel"}) + + assert mock_archive.timelapse_path is None, ( + "expected-archive branch must clear timelapse_path on reprint so " + "_scan_for_timelapse_with_retries doesn't early-return and " + "_capture_finish_photo_from_timelapse doesn't extract the original " + "run's last frame (#1707)" + ) + assert not stale_file.exists(), ( + "old timelapse file must be unlinked at reprint promotion to avoid orphans in the archive directory" + ) + + +@pytest.mark.asyncio +async def test_reprint_with_no_timelapse_path_is_noop(tmp_path): + """When archive has no prior timelapse_path (first print, or already + cleared), promotion must still succeed and not raise on the unlink path.""" + mock_printer = MagicMock() + mock_printer.id = 1 + mock_printer.auto_archive = True + mock_printer.external_camera_enabled = False + mock_printer.external_camera_url = None + mock_printer.name = "TestP2S" + + mock_archive = MagicMock() + mock_archive.id = 99 + mock_archive.filename = "FreshFile.3mf" + mock_archive.subtask_id = None + mock_archive.print_time_seconds = None + mock_archive.created_by_id = None + mock_archive.printer_id = 1 + mock_archive.print_name = "FreshFile" + mock_archive.status = "archived" + mock_archive.file_path = "archives/99/FreshFile.3mf" + mock_archive.energy_start_kwh = None + mock_archive.timelapse_path = None # nothing to clean up + + register_expected_print(1, "FreshFile.3mf", archive_id=99, ams_mapping=None) + + mock_session = _build_mocks(mock_printer, mock_archive) + + ( + async_session_p, + notif_p, + plug_p, + ws_p, + pm_p, + relay_p, + _energy, + _load_obj, + _store_spoolman, + _send_start, + _list_tl, + ) = _patches() + + with ( + async_session_p as mock_session_maker, + notif_p as mock_notif, + plug_p as mock_plug, + ws_p as mock_ws, + pm_p as mock_pm, + relay_p as mock_relay, + _energy, + _load_obj, + _store_spoolman, + _send_start, + _list_tl, + patch.object(app_settings, "base_dir", tmp_path), + ): + mock_session_maker.return_value = mock_session + mock_notif.on_print_start = AsyncMock() + mock_plug.on_print_start = AsyncMock() + mock_ws.send_print_start = AsyncMock() + mock_ws.send_archive_updated = AsyncMock() + mock_relay.on_print_start = AsyncMock() + mock_pm.get_printer = MagicMock(return_value=MagicMock(name="Test", serial_number="TEST123")) + + from backend.app.main import on_print_start + + await on_print_start(1, {"filename": "FreshFile.3mf", "subtask_name": "FreshFile"}) + + assert mock_archive.timelapse_path is None + assert mock_archive.status == "printing" + + +@pytest.mark.asyncio +async def test_reprint_with_missing_stale_file_does_not_raise(tmp_path): + """If the stale file referenced by timelapse_path no longer exists on + disk (user deleted, archive purge, container rebuilt with bind-mount + drift), promotion must still clear the field cleanly without raising.""" + mock_printer = MagicMock() + mock_printer.id = 1 + mock_printer.auto_archive = True + mock_printer.external_camera_enabled = False + mock_printer.external_camera_url = None + mock_printer.name = "TestP2S" + + mock_archive = MagicMock() + mock_archive.id = 7 + mock_archive.filename = "Ghost.3mf" + mock_archive.subtask_id = None + mock_archive.print_time_seconds = None + mock_archive.created_by_id = None + mock_archive.printer_id = 1 + mock_archive.print_name = "Ghost" + mock_archive.status = "archived" + mock_archive.file_path = "archives/7/Ghost.3mf" + mock_archive.energy_start_kwh = None + # Path points at a file that doesn't exist under tmp_path. + mock_archive.timelapse_path = "archives/7/timelapse/vanished.mp4" + + register_expected_print(1, "Ghost.3mf", archive_id=7, ams_mapping=None) + + mock_session = _build_mocks(mock_printer, mock_archive) + + ( + async_session_p, + notif_p, + plug_p, + ws_p, + pm_p, + relay_p, + _energy, + _load_obj, + _store_spoolman, + _send_start, + _list_tl, + ) = _patches() + + with ( + async_session_p as mock_session_maker, + notif_p as mock_notif, + plug_p as mock_plug, + ws_p as mock_ws, + pm_p as mock_pm, + relay_p as mock_relay, + _energy, + _load_obj, + _store_spoolman, + _send_start, + _list_tl, + patch.object(app_settings, "base_dir", tmp_path), + ): + mock_session_maker.return_value = mock_session + mock_notif.on_print_start = AsyncMock() + mock_plug.on_print_start = AsyncMock() + mock_ws.send_print_start = AsyncMock() + mock_ws.send_archive_updated = AsyncMock() + mock_relay.on_print_start = AsyncMock() + mock_pm.get_printer = MagicMock(return_value=MagicMock(name="Test", serial_number="TEST123")) + + from backend.app.main import on_print_start + + await on_print_start(1, {"filename": "Ghost.3mf", "subtask_name": "Ghost"}) + + assert mock_archive.timelapse_path is None + assert mock_archive.status == "printing"