Browse Source

fix(archives): clear stale thumbnail paths on log entries when archive deleted

  Archives → Print Log was 404-storming the thumbnail endpoint on every
  render: PrintLogEntry.thumbnail_path is copied by value from the archive
  at write-time, but the FK on archive_id is ON DELETE SET NULL (#1378) so
  log entries survive archive deletion to preserve stats history - and the
  cached path keeps pointing at a file that was removed when the archive's
  directory was deleted. Same shape for failed prints whose extractor never
  wrote the thumbnail.

  Two-part fix:

  1. Route self-heals: get_print_log_thumbnail NULLs thumbnail_path on the
     entry and commits before returning 404 when the file is missing on
     disk. The frontend's <img> tag is gated on entry.thumbnail_path being
     truthy, so the next fetch of the log list skips the request entirely.

  2. Eager clear on archive delete: new _null_print_log_thumbnail_paths()
     helper called from both soft_delete_archive and delete_archive before
     the on-disk files are removed. Avoids the one-time storm for future
     deletes; covers both the manual delete route and the auto-purge
     sweeper at archive_purge.py.

  Regression tests cover soft delete, hard delete via ArchiveService, and
  the route's lazy-NULL for failed-print orphans where the file was never
  written.
maziggy 1 week ago
parent
commit
cad63500a8

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 9 - 0
backend/app/api/routes/print_log.py

@@ -97,6 +97,13 @@ async def get_print_log_thumbnail(
     """Get the thumbnail for a print log entry.
 
     Requires a stream token query param (?token=xxx) when auth is enabled.
+
+    Self-heals stale entries: when thumbnail_path points to a file that no
+    longer exists on disk (archive was deleted, or print failed before the
+    thumbnail was ever written), NULL the path on the entry so subsequent
+    page renders skip the request entirely. The frontend's <img> tag is
+    gated on entry.thumbnail_path being truthy, so the next fetch of the
+    log list will simply not request this thumbnail again.
     """
     entry = await db.get(PrintLogEntry, entry_id)
     if not entry or not entry.thumbnail_path:
@@ -104,6 +111,8 @@ async def get_print_log_thumbnail(
 
     thumb_path = settings.base_dir / entry.thumbnail_path
     if not thumb_path.exists():
+        entry.thumbnail_path = None
+        await db.commit()
         raise HTTPException(404, "Thumbnail file not found")
 
     return FileResponse(

+ 26 - 0
backend/app/services/archive.py

@@ -827,6 +827,24 @@ class ProjectPageParser:
             return False
 
 
+async def _null_print_log_thumbnail_paths(db: AsyncSession, archive_id: int) -> None:
+    """NULL thumbnail_path on PrintLogEntry rows linked to *archive_id*.
+
+    Called from both soft- and hard-delete paths before the archive's files
+    leave disk. The FK on PrintLogEntry.archive_id is ON DELETE SET NULL so
+    log rows survive the archive — without this clear, their cached
+    thumbnail_path would still point at a deleted file and the print-log
+    view would 404-storm on every render (#1348 follow-up). Lazy-NULL on
+    the GET route self-heals stragglers (e.g. failed prints that never had
+    a thumbnail written), but eager clear here avoids the one-time storm.
+    """
+    from sqlalchemy import update as sa_update
+
+    from backend.app.models.print_log import PrintLogEntry
+
+    await db.execute(sa_update(PrintLogEntry).where(PrintLogEntry.archive_id == archive_id).values(thumbnail_path=None))
+
+
 class ArchiveService:
     """Service for archiving print jobs."""
 
@@ -1252,6 +1270,7 @@ class ArchiveService:
 
         dir_to_delete = self._resolve_archive_dir_for_delete(archive)
 
+        await _null_print_log_thumbnail_paths(self.db, archive_id)
         archive.deleted_at = datetime.now(timezone.utc)
         await self.db.commit()
 
@@ -1342,6 +1361,13 @@ class ArchiveService:
                 f"file_path is empty or invalid: '{archive.file_path}'"
             )
 
+        # NULL stale thumbnail_path on linked PrintLogEntries before the FK
+        # SET-NULL cascade fires. The on-disk file is about to be removed by
+        # the rmtree below, so the path on any surviving log entry (archive_id
+        # gets SET NULL by the FK) would otherwise point at a missing file
+        # and produce 404 storms in the print-log view (#1348-followup).
+        await _null_print_log_thumbnail_paths(self.db, archive_id)
+
         # Delete database record FIRST — if the commit fails (e.g. database locked
         # during concurrent bulk deletes), the files stay on disk and nothing is lost.
         await self.db.delete(archive)

+ 109 - 0
backend/tests/integration/test_archives_api.py

@@ -243,6 +243,115 @@ class TestArchivesAPI:
         assert post["total_filament_grams"] == 150.0
         assert post["total_cost"] == 4.50
 
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_soft_delete_clears_thumbnail_path_on_linked_log_entries(
+        self, async_client: AsyncClient, archive_factory, printer_factory, db_session
+    ):
+        """#1348 follow-up: soft-deleting an archive removes its files from disk;
+        the cached thumbnail_path on linked PrintLogEntry rows must be NULLed
+        in the same transaction so the print-log view doesn't 404-storm on the
+        now-deleted thumbnail file."""
+        from sqlalchemy import select
+
+        from backend.app.models.print_log import PrintLogEntry
+
+        printer = await printer_factory()
+        archive = await archive_factory(
+            printer.id,
+            status="completed",
+            thumbnail_path="archives/test/test_print/thumbnail.png",
+        )
+        # The factory's auto-PrintLogEntry doesn't copy thumbnail_path; set it
+        # manually to mirror what the production write_log_entry path stores.
+        run_query = await db_session.execute(select(PrintLogEntry).where(PrintLogEntry.archive_id == archive.id))
+        run = run_query.scalar_one()
+        run.thumbnail_path = "archives/test/test_print/thumbnail.png"
+        await db_session.commit()
+        assert run.thumbnail_path is not None
+
+        resp = await async_client.delete(f"/api/v1/archives/{archive.id}")
+        assert resp.status_code == 200
+        assert resp.json()["purged_from_stats"] is False
+
+        await db_session.refresh(run)
+        assert run.thumbnail_path is None, "soft-delete must NULL thumbnail_path on linked log entry"
+        # The log entry itself survives the soft delete (its filament/cost
+        # contribution still needs to flow into stats per #1343).
+        assert run.id is not None
+        assert run.archive_id == archive.id
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_hard_delete_clears_thumbnail_path_before_fk_cascade(
+        self, async_client: AsyncClient, archive_factory, printer_factory, db_session
+    ):
+        """#1348 follow-up: the auto-purge sweeper (and any caller of
+        ArchiveService.delete_archive) hard-deletes the archive row but leaves
+        PrintLogEntry rows alive via ON DELETE SET NULL. The eager
+        thumbnail_path clear must run inside delete_archive so even orphaned
+        log entries don't surface stale paths."""
+        from sqlalchemy import select
+
+        from backend.app.models.print_log import PrintLogEntry
+        from backend.app.services.archive import ArchiveService
+
+        printer = await printer_factory()
+        archive = await archive_factory(
+            printer.id,
+            status="completed",
+            thumbnail_path="archives/test/test_print/thumbnail.png",
+        )
+        run_query = await db_session.execute(select(PrintLogEntry).where(PrintLogEntry.archive_id == archive.id))
+        run = run_query.scalar_one()
+        run.thumbnail_path = "archives/test/test_print/thumbnail.png"
+        await db_session.commit()
+        run_id = run.id
+
+        service = ArchiveService(db_session)
+        assert await service.delete_archive(archive.id) is True
+
+        # Log entry survives the hard-delete (the FK is ON DELETE SET NULL
+        # in production; SQLite test config doesn't enable foreign_keys=ON
+        # by default so archive_id may still be set, but the row itself
+        # remains for audit). The thumbnail_path was cleared eagerly by
+        # _null_print_log_thumbnail_paths before db.delete(archive).
+        refetch = await db_session.execute(select(PrintLogEntry).where(PrintLogEntry.id == run_id))
+        survivor = refetch.scalar_one()
+        assert survivor.thumbnail_path is None, (
+            "delete_archive must NULL thumbnail_path before removing the archive row"
+        )
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_print_log_thumbnail_route_lazy_nulls_missing_file(
+        self, async_client: AsyncClient, archive_factory, printer_factory, db_session
+    ):
+        """#1348 follow-up: GET /print-log/{id}/thumbnail self-heals when the
+        thumbnail_path on a log entry points at a missing file (failed print
+        whose thumbnail was never written, or a stale path that escaped the
+        delete-time cleanup)."""
+        from sqlalchemy import select
+
+        from backend.app.models.print_log import PrintLogEntry
+
+        printer = await printer_factory()
+        archive = await archive_factory(printer.id, status="failed")
+        run_query = await db_session.execute(select(PrintLogEntry).where(PrintLogEntry.archive_id == archive.id))
+        run = run_query.scalar_one()
+        # Path points at a file that never existed (failed-print case where
+        # archive.thumbnail_path was set but the extractor never produced one).
+        run.thumbnail_path = "archives/missing/never_written/thumbnail.png"
+        await db_session.commit()
+
+        # Auth is disabled in the integration test config, so the stream-token
+        # guard is bypassed — the route runs the lazy-NULL branch directly.
+        resp = await async_client.get(f"/api/v1/print-log/{run.id}/thumbnail")
+        assert resp.status_code == 404
+
+        await db_session.refresh(run)
+        assert run.thumbnail_path is None, "missing file must self-heal to NULL"
+
     @pytest.mark.asyncio
     @pytest.mark.integration
     async def test_purge_stats_drops_archive_from_quick_stats(

Some files were not shown because too many files changed in this diff