fix(downloads): wait for post-processing result

Do not mark a monitored transfer as successful as soon as slskd reports completion. The monitor now only submits the post-processing worker; that worker reports the real success or failure after finding, verifying, and importing the file. If post-processing cannot be scheduled, mark the task failed and release the batch slot. Add a regression test for the premature success path.
pull/666/head
Broque Thomas 5 days ago
parent f02ef4dd6a
commit 6e5ea1d490

@ -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'.

@ -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:

@ -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)

Loading…
Cancel
Save