From 4cdc851cb5c2d1a179e693fedfefe8ed2fa2a3a3 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sun, 8 Nov 2026 07:53:05 -0500 Subject: [PATCH] fix: a game build is recognised before the archive integrity test, so re-extract no longer stalls on it The archive probe read and checksummed every member before the importer looked at what the archive held. For a multi-gigabyte game build that is minutes of work on a file that is kept whole and never extracted, and it held a re-extract chunk far past its time budget. - The importer checks the archive's directory for a software build first, for the outer archive and for one nested inside an art pack, and only probes archives it may extract. - The probe rejects an archive over the size cap from its directory totals, before the integrity test decompresses anything. - Re-extract logs each archive as it starts and finishes, with its size and how long it took, so an archive that stalls a chunk is named in the log. Co-Authored-By: Claude Opus 5.5 --- backend/app/services/cleanup_service.py | 10 ++++- backend/app/services/importer.py | 57 ++++++++++++++----------- backend/app/utils/safe_probe.py | 16 ++++++- tests/test_importer_archive.py | 21 +++++++++ tests/test_safe_probe.py | 17 ++++++++ 5 files changed, 93 insertions(+), 28 deletions(-) diff --git a/backend/app/services/cleanup_service.py b/backend/app/services/cleanup_service.py index bbf2087..23cbab8 100644 --- a/backend/app/services/cleanup_service.py +++ b/backend/app/services/cleanup_service.py @@ -1205,6 +1205,11 @@ def reextract_archive_attachments( ) if post is None: summary["without_post"] += 1 + # Logged on the way in as well as out, so an archive that stalls a + # chunk is named in the log even if the worker dies holding it. + size_mb = (att.size_bytes or 0) / 1e6 + log.info("re-extract: attachment %s (%.0f MB) starting", att.id, size_mb) + archive_start = time.monotonic() try: ids = _reextract_archive_to_post( importer, stored, post, source_row, artist, images_root, @@ -1214,8 +1219,11 @@ def reextract_archive_attachments( except Exception as exc: # one bad archive must not strand the rest session.rollback() summary["errors"] += 1 - log.warning("re-extract failed for attachment %s: %s", att.id, exc) + log.warning("re-extract failed for attachment %s after %.1fs: %s", + att.id, time.monotonic() - archive_start, exc) continue + log.info("re-extract: attachment %s done in %.1fs, %d new image(s)", + att.id, time.monotonic() - archive_start, len(ids)) if ids: summary["members_imported"] += len(ids) if post is not None: diff --git a/backend/app/services/importer.py b/backend/app/services/importer.py index 7363125..8a7afa6 100644 --- a/backend/app/services/importer.py +++ b/backend/app/services/importer.py @@ -594,29 +594,22 @@ class Importer: # lays files out under /images//...). Filesystem # import still calls bare _import_archive(source) and falls # back to the path-walk derivation as before. - probe = safe_probe.probe_archive(source) - if not probe.ok: - if probe.crashed: - return ImportResult( - status="failed", - error=f"archive probe crashed/timed out: {probe.reason}", - ) - # Clean rejection (bomb cap exceeded, integrity mismatch): - # still preserve the archive file itself as an attachment so - # nothing silently vanishes, matching extract_archive's - # fail-soft contract. - artist_use = artist if artist is not None else self._resolve_artist(source) - post = self._post_for_sidecar(source, artist_use, source_row=source_row) - self._capture_attachment( - source, post=post, artist=artist_use, resolved=True, + # + # A game or app build is recognised first, from the archive's directory + # alone. The probe reads and checksums every member, which for a + # multi-gigabyte build is minutes of work on a file that is kept whole + # and never extracted anyway. + marker = software_marker(source) + probe = None if marker is not None else safe_probe.probe_archive(source) + if probe is not None and probe.crashed: + return ImportResult( + status="failed", + error=f"archive probe crashed/timed out: {probe.reason}", ) - reason = f"archive probe rejected, captured unextracted: {probe.reason}" - log.warning("%s: %s", source.name, reason) - return ImportResult(status="attached", error=reason) artist_use = artist if artist is not None else self._resolve_artist(source) post = self._post_for_sidecar(source, artist_use, source_row=source_row) - if (marker := software_marker(source)) is not None: + if marker is not None: # A game or app build: kept whole, its bundled images left alone. self._capture_attachment( source, post=post, artist=artist_use, resolved=True, @@ -624,6 +617,16 @@ class Importer: log.info("%s: a software build (holds %s), kept unextracted", source.name, marker) return ImportResult(status="attached") + if not probe.ok: + # Clean rejection (bomb cap exceeded, integrity mismatch): the + # archive file itself is still kept as an attachment so nothing + # silently vanishes, matching extract_archive's fail-soft contract. + self._capture_attachment( + source, post=post, artist=artist_use, resolved=True, + ) + reason = f"archive probe rejected, captured unextracted: {probe.reason}" + log.warning("%s: %s", source.name, reason) + return ImportResult(status="attached", error=reason) # Committed before extraction, so a member that fails (and is rolled # back on its own) can never take the archive's post with it. self.session.commit() @@ -718,6 +721,15 @@ class Importer: _ARCHIVE_MAX_DEPTH, member_path.name, ) continue + # Recognised from its directory before the probe reads + # every member, as for the outer archive. + if (marker := software_marker(member_path)) is not None: + counts["nested_software"] += 1 + log.info( + "nested archive is a software build (holds %s), " + "skipped: %s", marker, member_path.name, + ) + continue probe = safe_probe.probe_archive(member_path) if not probe.ok: counts["nested_rejected"] += 1 @@ -726,13 +738,6 @@ class Importer: probe.reason, member_path.name, ) continue - if (marker := software_marker(member_path)) is not None: - counts["nested_software"] += 1 - log.info( - "nested archive is a software build (holds %s), " - "skipped: %s", marker, member_path.name, - ) - continue self._collect_archive_members( member_path, attribution=attribution, source_row=source_row, depth=depth + 1, diff --git a/backend/app/utils/safe_probe.py b/backend/app/utils/safe_probe.py index 4b1af6c..05a114f 100644 --- a/backend/app/utils/safe_probe.py +++ b/backend/app/utils/safe_probe.py @@ -175,12 +175,20 @@ def _run_probe(path_str: str) -> tuple[str, str | None]: return ("ok", None) +def _over_cap(total) -> bool: + return total is not None and total > MAX_ARCHIVE_UNCOMPRESSED_BYTES + + def _inspect_archive(path: Path): """Return (total_uncompressed_bytes | None, first_bad_member | None) for the archive. Format detected by extension OR magic bytes (so a mis-named archive is still bomb-guarded + integrity-tested, matching the extractor's gate); raises on a structurally-broken container (caught by the - child as a clean rejection).""" + child as a clean rejection). + + The size total comes from the archive's directory, so an archive over the + cap is rejected before the integrity test decompresses anything: on a + multi-gigabyte archive that test alone outlasts the probe's timeout.""" from ..services.archive_extractor import detect_archive_format fmt = detect_archive_format(path) @@ -189,12 +197,16 @@ def _inspect_archive(path: Path): with zipfile.ZipFile(path) as zf: total = sum(zi.file_size for zi in zf.infolist()) + if _over_cap(total): + return total, None return total, zf.testzip() if fmt == "rar": import rarfile with rarfile.RarFile(path) as rf: total = sum(getattr(ri, "file_size", 0) for ri in rf.infolist()) + if _over_cap(total): + return total, None rf.testrar() return total, None if fmt == "7z": @@ -203,6 +215,8 @@ def _inspect_archive(path: Path): with py7zr.SevenZipFile(path, "r") as zf: info = zf.archiveinfo() total = getattr(info, "uncompressed", None) + if _over_cap(total): + return total, None ok = zf.test() # True / None when all members pass return total, (None if ok in (True, None) else "7z test reported corruption") # Not a recognised archive — nothing to test; treat as clean. diff --git a/tests/test_importer_archive.py b/tests/test_importer_archive.py index 2c35552..95df5d2 100644 --- a/tests/test_importer_archive.py +++ b/tests/test_importer_archive.py @@ -305,3 +305,24 @@ def test_a_game_build_inside_an_art_pack_is_skipped_and_the_art_imported(importe assert r.status == "imported" assert len(r.member_image_ids) == 1 assert importer.session.execute(select(func.count()).select_from(ImageRecord)).scalar_one() == 1 + + +def test_a_game_build_is_recognised_before_the_integrity_test( + importer, import_layout, monkeypatch, +): + """A multi-gigabyte build would spend minutes in the probe's read-everything + test, only to be kept whole anyway, so its directory decides first.""" + from backend.app.utils import safe_probe + + def _no_probe(_path, **_kw): + raise AssertionError("a software build went through the archive probe") + monkeypatch.setattr(safe_probe, "probe_archive", _no_probe) + import_root, _ = import_layout + arc = import_root / "Bob" / "MyGame-2.0-pc.zip" + arc.parent.mkdir(parents=True, exist_ok=True) + with zipfile.ZipFile(arc, "w") as zf: + zf.writestr("MyGame/MyGame.exe", b"MZ") + zf.writestr("MyGame/game/images/cg01.jpg", _split_bytes("v")) + + assert importer.import_one(arc).status == "attached" + assert importer.session.execute(select(func.count()).select_from(ImageRecord)).scalar_one() == 0 diff --git a/tests/test_safe_probe.py b/tests/test_safe_probe.py index 85e5c85..dd5aaaa 100644 --- a/tests/test_safe_probe.py +++ b/tests/test_safe_probe.py @@ -67,6 +67,23 @@ def test_run_probe_bomb_guard(tmp_path, monkeypatch): assert "bomb-guard cap" in detail +def test_an_archive_over_the_cap_is_rejected_without_an_integrity_test( + tmp_path, monkeypatch, +): + """The size total is read from the directory, so an oversize archive never + reaches the decompress-everything test.""" + monkeypatch.setattr(safe_probe, "MAX_ARCHIVE_UNCOMPRESSED_BYTES", 10) + + def _no_test(_self): + raise AssertionError("integrity test ran on an archive over the cap") + monkeypatch.setattr(zipfile.ZipFile, "testzip", _no_test) + z = tmp_path / "big.zip" + _zip(z, {"big.txt": b"x" * 5000}) + status, detail = safe_probe._run_probe(str(z)) + assert status == "error" + assert "bomb-guard cap" in detail + + def test_probe_video_non_video_is_not_ok(tmp_path): """A text file is not a decodable video. Whether ffprobe is present (returncode != 0) or absent (OSError → 'unavailable'), the result is