imports: never delete a file we couldn't quarantine — leave it for retry
Discord (Shdjfgatdif): "if a track isn't imported it should remain there, not be deleted, so we can retry." He was seeing failed downloads disappear and having to re-download. Normally a rejected file is QUARANTINED (moved to ss_quarantine, preserved + retryable), not deleted. But all four quarantine blocks (integrity / silence / quality / acoustid) had the same fallback: if move_to_quarantine itself raised, os.remove(file_path). On a NAS that move can fail (cross-device / permissions), so the except fired and the user's download was DELETED — the worst outcome, and exactly the re-download pain he reported. fix: on quarantine failure, log and LEAVE the file in place — never delete. The task is still marked failed and the batch still notified (that code runs after the try/except and never touched the deleted file), so the only behaviour change is "preserved instead of destroyed". Reviewed every os.remove in the pipeline: the remaining ones are success-path cleanups (replacing an existing destination, or removing a redundant download when the track is already in the library at equal/better quality) — left untouched. regression test drives the REAL pipeline through integrity-rejection with quarantine forced to raise, and asserts the source file is preserved while the task is still failed + notified. 1311 imports/downloads/quality tests green, ruff clean.
This commit is contained in:
BoulderBadgeDad
parent
551df0c3ca
commit
8abf470018
2 changed files with 97 additions and 21 deletions
|
|
@ -325,11 +325,14 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
|||
_mark_task_quarantined(context, quarantine_path)
|
||||
logger.error(f"File quarantined due to integrity failure: {quarantine_path}")
|
||||
except Exception as quarantine_error:
|
||||
logger.error(f"Quarantine failed ({quarantine_error}), deleting broken file: {file_path}")
|
||||
try:
|
||||
os.remove(file_path)
|
||||
except Exception as del_error:
|
||||
logger.error(f"Could not delete broken file either: {del_error}")
|
||||
# Quarantine MOVE failed (e.g. cross-device / permission on a NAS).
|
||||
# Do NOT delete — destroying a download we couldn't even quarantine is
|
||||
# data loss and forces a re-download. Leave it in place so it can be
|
||||
# retried; the task is still marked failed below either way (#kettui).
|
||||
logger.error(
|
||||
f"Quarantine failed ({quarantine_error}) — leaving file in place "
|
||||
f"for retry (not deleting): {file_path}"
|
||||
)
|
||||
|
||||
with matched_context_lock:
|
||||
if context_key in matched_downloads_context:
|
||||
|
|
@ -383,11 +386,12 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
|||
_mark_task_quarantined(context, quarantine_path)
|
||||
logger.warning("File quarantined — incomplete/silent audio: %s", quarantine_path)
|
||||
except Exception as quarantine_error:
|
||||
logger.error(f"Quarantine failed ({quarantine_error}), deleting file: {file_path}")
|
||||
try:
|
||||
os.remove(file_path)
|
||||
except Exception as del_error:
|
||||
logger.debug("delete broken file fallback: %s", del_error)
|
||||
# Don't delete a file we couldn't quarantine — leave it for retry
|
||||
# instead of forcing a re-download (data loss). See integrity block.
|
||||
logger.error(
|
||||
f"Quarantine failed ({quarantine_error}) — leaving file in place "
|
||||
f"for retry (not deleting): {file_path}"
|
||||
)
|
||||
|
||||
with matched_context_lock:
|
||||
matched_downloads_context.pop(context_key, None)
|
||||
|
|
@ -437,11 +441,12 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
|||
_mark_task_quarantined(context, quarantine_path)
|
||||
logger.info(f"File quarantined due to quality mismatch: {quarantine_path}")
|
||||
except Exception as quarantine_error:
|
||||
logger.error(f"Quarantine failed ({quarantine_error}), deleting file: {file_path}")
|
||||
try:
|
||||
os.remove(file_path)
|
||||
except Exception as e:
|
||||
logger.debug("delete quarantine fallback: %s", e)
|
||||
# Don't delete a file we couldn't quarantine — leave it for retry
|
||||
# instead of forcing a re-download (data loss). See integrity block.
|
||||
logger.error(
|
||||
f"Quarantine failed ({quarantine_error}) — leaving file in place "
|
||||
f"for retry (not deleting): {file_path}"
|
||||
)
|
||||
|
||||
context['_bitdepth_rejected'] = True
|
||||
task_id = context.get('task_id')
|
||||
|
|
@ -535,12 +540,13 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
|||
_mark_task_quarantined(context, quarantine_path)
|
||||
logger.error(f"File quarantined due to verification failure: {quarantine_path}")
|
||||
except Exception as quarantine_error:
|
||||
logger.error(f"Quarantine failed ({quarantine_error}), deleting wrong file: {file_path}")
|
||||
logger.error(f"Quarantine failed, deleting wrong file: {file_path}")
|
||||
try:
|
||||
os.remove(file_path)
|
||||
except Exception as del_error:
|
||||
logger.error(f"Could not delete wrong file either: {del_error}")
|
||||
# Don't delete a file we couldn't quarantine — leave it for
|
||||
# retry instead of forcing a re-download (data loss). The
|
||||
# task is still marked failed / requeued below. See integrity.
|
||||
logger.error(
|
||||
f"Quarantine failed ({quarantine_error}) — leaving file "
|
||||
f"in place for retry (not deleting): {file_path}"
|
||||
)
|
||||
|
||||
context['_acoustid_quarantined'] = True
|
||||
context['_acoustid_failure_msg'] = verification_msg
|
||||
|
|
|
|||
|
|
@ -711,3 +711,73 @@ def test_exhaustive_single_source_exhausted_fails(monkeypatch):
|
|||
assert task["status"] == "failed"
|
||||
assert submitted == []
|
||||
assert completion == [("rbatch", "rtask", False)]
|
||||
|
||||
|
||||
def test_quarantine_failure_preserves_file_instead_of_deleting(tmp_path, monkeypatch):
|
||||
"""REGRESSION: when move_to_quarantine itself FAILS (e.g. a cross-device move on
|
||||
a NAS), the rejected file must be LEFT IN PLACE for retry — never deleted.
|
||||
|
||||
Deleting a download we couldn't even quarantine is data loss that forces a
|
||||
re-download (Discord: Shdjfgatdif). The task is still marked failed + the batch
|
||||
still notified — only the destructive os.remove is gone. Drives the real pipeline
|
||||
through the integrity-rejection path with quarantine forced to raise."""
|
||||
source_path = tmp_path / "source.flac"
|
||||
source_path.write_bytes(b"audio")
|
||||
|
||||
context_key, task_id, batch_id = "ctx-q", "task-q", "batch-q"
|
||||
context = {
|
||||
"search_result": {"is_simple_download": True, "filename": "Album/source.flac", "album": "Album"},
|
||||
"track_info": {}, "original_search_result": {}, "is_album_download": False,
|
||||
"task_id": task_id, "batch_id": batch_id,
|
||||
}
|
||||
completion_calls = []
|
||||
|
||||
snap = (dict(runtime_state.matched_downloads_context), dict(runtime_state.download_tasks),
|
||||
dict(runtime_state.download_batches), set(runtime_state.processed_download_ids),
|
||||
dict(runtime_state.post_process_locks))
|
||||
for d in (runtime_state.matched_downloads_context, runtime_state.download_tasks,
|
||||
runtime_state.download_batches, runtime_state.processed_download_ids,
|
||||
runtime_state.post_process_locks):
|
||||
d.clear()
|
||||
|
||||
runtime = types.SimpleNamespace(
|
||||
automation_engine=None,
|
||||
on_download_completed=lambda b, t, success: completion_calls.append((b, t, success)),
|
||||
web_scan_manager=types.SimpleNamespace(request_scan=lambda r: None),
|
||||
repair_worker=None,
|
||||
)
|
||||
fake_acoustid = types.ModuleType("core.acoustid_verification")
|
||||
fake_acoustid.AcoustIDVerification = _FakeAcoustidVerifier
|
||||
fake_acoustid.VerificationResult = types.SimpleNamespace(FAIL="FAIL")
|
||||
monkeypatch.setitem(sys.modules, "core.acoustid_verification", fake_acoustid)
|
||||
|
||||
from core.imports.file_integrity import IntegrityResult
|
||||
# Integrity FAILS → enters the quarantine block.
|
||||
monkeypatch.setattr(import_pipeline, "check_audio_integrity",
|
||||
lambda *_a, **_k: IntegrityResult(ok=False, reason="broken (test)", checks={}))
|
||||
# The quarantine MOVE itself raises → exercises the except branch (the fix).
|
||||
def _boom(*_a, **_k):
|
||||
raise OSError("cross-device link not permitted (simulated NAS)")
|
||||
monkeypatch.setattr(import_pipeline, "move_to_quarantine", _boom)
|
||||
monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _Config(str(tmp_path / "Transfer")))
|
||||
monkeypatch.setattr(import_pipeline, "add_activity_item", lambda *a, **k: None)
|
||||
|
||||
runtime_state.matched_downloads_context[context_key] = context
|
||||
runtime_state.download_tasks[task_id] = {"track_info": {}, "status": "running"}
|
||||
|
||||
try:
|
||||
import_pipeline.post_process_matched_download_with_verification(
|
||||
context_key, context, str(source_path), task_id, batch_id, runtime)
|
||||
|
||||
# THE regression: a file we couldn't quarantine is preserved, not deleted.
|
||||
assert source_path.exists(), "file must be LEFT IN PLACE when quarantine fails"
|
||||
# Downstream still correct — task failed, batch notified of failure.
|
||||
assert runtime_state.download_tasks[task_id]["status"] == "failed"
|
||||
assert completion_calls == [(batch_id, task_id, False)]
|
||||
finally:
|
||||
for d, original in zip(
|
||||
(runtime_state.matched_downloads_context, runtime_state.download_tasks,
|
||||
runtime_state.download_batches, runtime_state.processed_download_ids,
|
||||
runtime_state.post_process_locks), snap):
|
||||
d.clear()
|
||||
d.update(original)
|
||||
|
|
|
|||
Loading…
Reference in a new issue