diff --git a/core/downloads/lifecycle.py b/core/downloads/lifecycle.py index 43e1e970..2a29feec 100644 --- a/core/downloads/lifecycle.py +++ b/core/downloads/lifecycle.py @@ -173,7 +173,7 @@ def on_download_completed(batch_id: str, task_id: str, success: bool, deps: Life # Guard against double-calling: track which tasks have already been completed # This prevents active_count from being decremented multiple times for the same task - # (e.g. monitor detects completion AND post-processing calls this again) + # (e.g. status polling and post-processing both observe the same terminal task) # NOTE: On duplicate calls, we skip decrement/tracking but STILL check batch completion. # This is critical because the first call may see the task in 'post_processing' (not finished), # and the second call (from post-processing worker) arrives after the task is truly 'completed'. diff --git a/core/downloads/monitor.py b/core/downloads/monitor.py index 79a90d8d..62d515e5 100644 --- a/core/downloads/monitor.py +++ b/core/downloads/monitor.py @@ -227,18 +227,28 @@ class WebUIDownloadMonitor: except Exception as e: logger.error(f"[Deferred] Error executing deferred operation {op[0]}: {e}") - # Handle completed downloads outside the lock to prevent deadlock - # (_on_download_completed acquires tasks_lock internally) + # Handle completed transfers outside the lock. The transfer engine's + # "complete" state only means the remote download finished; the + # post-processing worker still has to find, verify, tag, and move the + # file before it can report real batch success or failure. for batch_id, task_id in completed_tasks: try: # Submit post-processing worker (file move, tagging, AcoustID verification) # This makes batch downloads fully independent of browser polling. logger.info(f"[Monitor] Submitting post-processing worker for task {task_id}") missing_download_executor.submit(_run_post_processing_worker, task_id, batch_id) - # Chain to next download in the batch queue - _on_download_completed(batch_id, task_id, success=True) except Exception as e: logger.error(f"[Monitor] Error handling completed task {task_id}: {e}") + with tasks_lock: + if task_id in download_tasks: + download_tasks[task_id]['status'] = 'failed' + download_tasks[task_id]['error_message'] = f'Post-processing could not be scheduled: {e}' + try: + _on_download_completed(batch_id, task_id, success=False) + except Exception as completion_error: + logger.error( + f"[Monitor] Error marking failed post-processing submit for task {task_id}: {completion_error}" + ) # Handle exhausted retry tasks outside the lock to prevent deadlock for batch_id, task_id in exhausted_tasks: try: diff --git a/tests/test_manual_pick_no_auto_retry.py b/tests/test_manual_pick_no_auto_retry.py index c5b23cfe..efa88359 100644 --- a/tests/test_manual_pick_no_auto_retry.py +++ b/tests/test_manual_pick_no_auto_retry.py @@ -93,3 +93,75 @@ def test_manual_pick_skips_retry_on_errored_state(fake_monitor): assert task['status'] == 'downloading' +def test_monitor_waits_for_post_processing_before_batch_success(monkeypatch): + """Engine completion is not the same as a successful import. + + The monitor should start post-processing when slskd reports a completed + transfer, but the post-processing worker must be the only code path that + reports final success/failure to the batch lifecycle. + """ + monkeypatch.setattr(dm, '_make_context_key', lambda u, f: f"{u}::{f}") + monkeypatch.setattr(dm.WebUIDownloadMonitor, '_validate_worker_counts', lambda self: None) + + submitted = [] + completions = [] + + class FakeExecutor: + def submit(self, func, task_id, batch_id): + submitted.append((func, task_id, batch_id)) + + def fake_post_processing_worker(task_id, batch_id): + return None + + monkeypatch.setattr(dm, 'missing_download_executor', FakeExecutor()) + monkeypatch.setattr(dm, '_run_post_processing_worker', fake_post_processing_worker) + monkeypatch.setattr( + dm, + '_on_download_completed', + lambda batch_id, task_id, success: completions.append((batch_id, task_id, success)), + ) + + with dm.tasks_lock: + previous_tasks = dict(dm.download_tasks) + previous_batches = dict(dm.download_batches) + dm.download_tasks.clear() + dm.download_batches.clear() + dm.download_tasks['task-1'] = { + 'track_info': {'name': 'Test Track'}, + 'username': 'Pinasound', + 'filename': r'@@tmllb\Music\Album\01. Track.flac', + 'status': 'downloading', + 'download_id': 'download-1', + 'status_change_time': time.time(), + } + dm.download_batches['batch-1'] = {'queue': ['task-1']} + + try: + monitor = dm.WebUIDownloadMonitor() + monitor.monitoring = True + monitor.monitored_batches.add('batch-1') + monkeypatch.setattr( + monitor, + '_get_live_transfers', + lambda: { + r'Pinasound::@@tmllb\Music\Album\01. Track.flac': { + 'state': 'Completed, Succeeded', + 'size': 100, + 'bytesTransferred': 100, + } + }, + ) + + monitor._check_all_downloads() + + assert submitted == [(fake_post_processing_worker, 'task-1', 'batch-1')] + assert completions == [] + assert dm.download_tasks['task-1']['status'] == 'post_processing' + finally: + with dm.tasks_lock: + dm.download_tasks.clear() + dm.download_tasks.update(previous_tasks) + dm.download_batches.clear() + dm.download_batches.update(previous_batches) + +