From 315399c4e55337f3fb60ddce5fe0c5bc8f84dd69 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 6 Nov 2026 15:48:14 -0500 Subject: [PATCH] fix: an unfinished download is never stored as an attachment, and the temp sweep keeps any file a library row points at The importer refuses a staging file (.part, .partial, .megatmp.*) at both entry points, the same way it refuses a sidecar .json, and discard_after_attach never deletes one: the age-gated temp sweep owns them. The sweep checks its stale candidates against image and attachment rows and keeps any a row points at. A Google Drive fetch's half-written file had been stored as .part; the sweep then deleted it by name and left the row with no file. Co-Authored-By: Claude Opus 5.5 --- backend/app/services/importer.py | 36 ++++++++++++++------ backend/app/tasks/maintenance.py | 46 +++++++++++++++++++++++--- tests/test_importer_attach_in_place.py | 25 ++++++++++++++ tests/test_maintenance.py | 42 +++++++++++++++++------ 4 files changed, 122 insertions(+), 27 deletions(-) diff --git a/backend/app/services/importer.py b/backend/app/services/importer.py index 9715053..e5960ca 100644 --- a/backend/app/services/importer.py +++ b/backend/app/services/importer.py @@ -43,6 +43,7 @@ from ..utils.paths import ( from ..utils.phash import compute_phash, find_similar, fingerprint_path, fingerprints_match from ..utils.sidecar import find_sidecar, parse_sidecar from ..utils.slug import slugify +from ..utils.staging import is_temp_staging_file from .archive_extractor import extract_archive, is_archive from .attachment_store import AttachmentStore from .audits import single_color @@ -115,6 +116,21 @@ def is_video(path: Path) -> bool: return path.suffix.lower() in VIDEO_EXTS +def _not_content(path: Path) -> ImportResult | None: + """The skip result for a file that is never library content: a sidecar + `.json` (metadata) or a downloader's staging file (an unfinished download, + which becomes content only under its final name). None for anything else.""" + if path.suffix.lower() == ".json": + error = "sidecar json is metadata, not content" + elif is_temp_staging_file(path): + error = "unfinished download (staging file), not content" + else: + return None + return ImportResult( + status="skipped", skip_reason=SkipReason.invalid_image, error=error, + ) + + def _safe_ext(path: Path) -> str: """Conservatively extract a file extension for PostAttachment.ext (varchar(32)). Thin wrapper over the shared `utils.paths.safe_ext` (kept for @@ -410,11 +426,8 @@ class Importer: media members (one Post via the archive-adjacent sidecar) and preserve the archive itself. Other non-media → PostAttachment. Nothing is skipped for being non-media (FC-2d-iii).""" - if source.suffix.lower() == ".json": - return ImportResult( - status="skipped", skip_reason=SkipReason.invalid_image, - error="sidecar json is metadata, not content", - ) + if (refused := _not_content(source)) is not None: + return refused if is_archive(source): return self._import_archive(source) if not is_supported(source): @@ -1219,11 +1232,8 @@ class Importer: caller may delete the on-disk original (mirrors duplicate_hash) - failed → file untouched; caller decides """ - if path.suffix.lower() == ".json": - return ImportResult( - status="skipped", skip_reason=SkipReason.invalid_image, - error="sidecar json is metadata, not content", - ) + if (refused := _not_content(path)) is not None: + return refused if is_archive(path): return self._import_archive( path, artist=artist, source_row=source, @@ -1420,7 +1430,11 @@ class Importer: An imported or superseded media file is kept: it IS the record's file. So is any path an ImageRecord points at, whatever the status, because a - re-fetch can write identical bytes over a record's own file.""" + re-fetch can write identical bytes over a record's own file. A staging + file is kept too: it may be a download still being written, and the + temp-file sweep removes it once it is old enough to be abandoned.""" + if is_temp_staging_file(path): + return False if result.status in ("imported", "superseded"): redundant = is_archive(path) else: diff --git a/backend/app/tasks/maintenance.py b/backend/app/tasks/maintenance.py index 767065a..a6651f2 100644 --- a/backend/app/tasks/maintenance.py +++ b/backend/app/tasks/maintenance.py @@ -20,6 +20,7 @@ from ..models import ( ImportSettings, ImportTask, LibraryAuditRun, + PostAttachment, Source, TaskRun, ) @@ -372,23 +373,58 @@ def cleanup_old_tasks() -> int: return result.rowcount or 0 +def _paths_with_rows(paths: list[str]) -> set[str]: + """The subset of `paths` that an image (its file or thumbnail) or an + attachment row points at.""" + owned: set[str] = set() + SessionLocal = _sync_session_factory() + with SessionLocal() as session: + for i in range(0, len(paths), 500): + chunk = paths[i:i + 500] + owned.update(session.execute( + select(ImageRecord.path).where(ImageRecord.path.in_(chunk)) + ).scalars()) + owned.update(session.execute( + select(ImageRecord.thumbnail_path).where(ImageRecord.thumbnail_path.in_(chunk)) + ).scalars()) + owned.update(session.execute( + select(PostAttachment.path).where(PostAttachment.path.in_(chunk)) + ).scalars()) + return owned + + @celery.task(name="backend.app.tasks.maintenance.cleanup_orphaned_temp_files") def cleanup_orphaned_temp_files() -> int: """Delete orphaned staging files (.part, .partial, megatools' .megatmp.*) under the images root, left by a download/import killed mid-write. Only removes files older than ORPHAN_TEMP_MIN_AGE_HOURS so an in-flight - download's staging file is never pulled out from under it. Returns the - count removed.""" + download's staging file is never pulled out from under it, and never a + file an image or attachment row points at, whatever its name: that file + is library content. Returns the count removed.""" if not IMAGES_ROOT.is_dir(): return 0 cutoff = datetime.now(UTC).timestamp() - ORPHAN_TEMP_MIN_AGE_HOURS * 3600 - removed = 0 + stale: list[Path] = [] for path in IMAGES_ROOT.rglob("*"): if not is_temp_staging_file(path) or not path.is_file(): continue try: - if path.stat().st_mtime >= cutoff: - continue # too fresh — may be an active download + if path.stat().st_mtime < cutoff: # fresher may be an active download + stale.append(path) + except OSError as exc: + log.warning("cleanup_orphaned_temp_files: %s: %s", path, exc) + if not stale: + return 0 + owned = _paths_with_rows([str(p) for p in stale]) + removed = 0 + for path in stale: + if str(path) in owned: + log.warning( + "cleanup_orphaned_temp_files: keeping %s: a library row points at it", + path, + ) + continue + try: path.unlink() removed += 1 except OSError as exc: diff --git a/tests/test_importer_attach_in_place.py b/tests/test_importer_attach_in_place.py index 47ae24d..33ec48b 100644 --- a/tests/test_importer_attach_in_place.py +++ b/tests/test_importer_attach_in_place.py @@ -410,6 +410,31 @@ def test_attach_in_place_invalid_image_returns_skipped(importer): assert result.skip_reason.value == "invalid_image" +@pytest.mark.parametrize("name", ["film.mp4abc123_.part", "img.jpg.partial", ".megatmp.Xy12"]) +def test_an_unfinished_download_is_refused_and_left_on_disk(importer, db_sync, name): + """A staging file is a download still being written (or abandoned): it is + never stored as an attachment, and the importer never deletes it, so a + download in progress is not pulled out from under its writer.""" + from backend.app.models import PostAttachment + + artist = Artist(name="Hal", slug="hal") + db_sync.add(artist) + db_sync.flush() + staging = importer.images_root / "hal" / "patreon" / "post" / name + staging.parent.mkdir(parents=True, exist_ok=True) + staging.write_bytes(b"half of a video") + + for result in (importer.attach_in_place(staging, artist=artist), + importer.import_one(staging)): + assert result.status == "skipped" + assert "unfinished download" in result.error + assert importer.discard_after_attach(staging, result) is False + assert staging.exists() + assert db_sync.execute( + select(func.count()).where(PostAttachment.original_filename == name) + ).scalar() == 0 + + def test_attach_in_place_non_media_routes_to_attachment(importer, db_sync): """FC-2d-iii dispatch parity for the download path. A non-media file (.pdf, .txt etc.) downloaded by gallery-dl must become a PostAttachment, diff --git a/tests/test_maintenance.py b/tests/test_maintenance.py index cae9705..0738d61 100644 --- a/tests/test_maintenance.py +++ b/tests/test_maintenance.py @@ -26,30 +26,50 @@ def _make_batch(session) -> int: return batch.id -def test_cleanup_orphaned_temp_files_removes_stale_only(tmp_path, monkeypatch): +def test_cleanup_orphaned_temp_files_removes_stale_only(db_sync, tmp_path, monkeypatch): import os + from backend.app.models import ImageRecord, PostAttachment from backend.app.tasks import maintenance as m monkeypatch.setattr(m, "IMAGES_ROOT", tmp_path) - stale = tmp_path / "artist" / "img.jpg.part" # killed download → orphan - stale.parent.mkdir(parents=True) - stale.write_bytes(b"x") old = datetime.now(UTC).timestamp() - 8 * 3600 # older than the 6h guard - os.utime(stale, (old, old)) - fresh = tmp_path / "in_progress.jpg.partial" # active download → keep - fresh.write_bytes(b"x") - keep = tmp_path / "real.jpg" # a real image → keep + + def staged(rel: str, *, aged: bool = True): + p = tmp_path / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_bytes(b"x") + if aged: + os.utime(p, (old, old)) + return p + + stale = staged("artist/img.jpg.part") # killed download → orphan + mega = staged("artist/.megatmp.AbCd123") # abandoned megatools stage + fresh = staged("in_progress.jpg.partial", aged=False) # active download → keep + keep = tmp_path / "real.jpg" # a real image → keep keep.write_bytes(b"x") - mega = tmp_path / "artist" / ".megatmp.AbCd123" # abandoned megatools stage - mega.write_bytes(b"x") - os.utime(mega, (old, old)) + # Staging-named files a library row points at are library content. + attached = staged("attachments/ab/abc.part") + imaged = staged("artist/video.mp4.part") + db_sync.add(PostAttachment( + post_id=None, artist_id=None, sha256="tempsweep-att".ljust(64, "x"), + path=str(attached), original_filename="film.mp4.part", ext=".part", + size_bytes=1, + )) + db_sync.add(ImageRecord( + path=str(imaged), sha256="tempsweep-img".ljust(64, "x"), size_bytes=1, + mime="video/mp4", width=1, height=1, origin="downloaded", + integrity_status="ok", + )) + db_sync.commit() assert m.cleanup_orphaned_temp_files() == 2 assert not stale.exists() assert not mega.exists() assert fresh.exists() assert keep.exists() + assert attached.exists() + assert imaged.exists() def test_recover_interrupted_only_old(db_sync, monkeypatch):