feat(dashboard): smarter Retry All Failed with per-source dedup and recent-success skip
The Dashboard's "Retry All Failed" button used to fetch every non-superseded
failure, then fire one retry HTTP call per row. Two missing guards:
- No per-source dedup — a source with N historical failures enqueued N jobs
for the same URL.
- No "recent success" check — a source that succeeded two hours ago could
still have stale un-superseded failures from last week that got re-queued.
Adds POST /api/downloads/retry-failed-bulk:
• Selects non-superseded failures, ordered (source_id ASC, created_at DESC)
• Per-source dedup keeps only the most recent failure per source
• Skips sources whose last_success is within the configurable recent window
(default 24h, via recent_window_hours body param)
• Resets + enqueues in a single DB transaction, single Celery dispatch loop
• Returns a per-reason skip breakdown: duplicate_source, recent_success,
no_source (orphaned rows)
The Dashboard button now calls the bulk endpoint and surfaces the full
skip breakdown in the toast, so the user knows exactly what was (and
wasn't) queued. Logic is extracted into a pure _plan_bulk_retry helper
and unit-tested against SimpleNamespace fixtures — 7 new tests covering
the dedup, window, and orphan paths.
This commit is contained in:
@@ -114,6 +114,109 @@ async def get_download(download_id: int):
|
|||||||
return jsonify(response)
|
return jsonify(response)
|
||||||
|
|
||||||
|
|
||||||
|
def _plan_bulk_retry(sorted_failures, sources_by_id, recent_cutoff):
|
||||||
|
"""Decide which failed downloads to retry given per-source dedup + recent-success filter.
|
||||||
|
|
||||||
|
Args:
|
||||||
|
sorted_failures: Download rows sorted by (source_id ASC, created_at DESC) —
|
||||||
|
the first occurrence of each source_id is the most recent failure.
|
||||||
|
sources_by_id: dict mapping source_id -> Source (for `last_success` lookup).
|
||||||
|
recent_cutoff: datetime — any source whose last_success is >= this is skipped.
|
||||||
|
|
||||||
|
Returns (retry_ids, summary) where summary has skip counts per reason.
|
||||||
|
"""
|
||||||
|
seen_sources = set()
|
||||||
|
retry_ids = []
|
||||||
|
skipped_dup = 0
|
||||||
|
skipped_recent = 0
|
||||||
|
skipped_no_source = 0
|
||||||
|
|
||||||
|
for failure in sorted_failures:
|
||||||
|
if failure.source_id is None:
|
||||||
|
skipped_no_source += 1
|
||||||
|
continue
|
||||||
|
if failure.source_id in seen_sources:
|
||||||
|
skipped_dup += 1
|
||||||
|
continue
|
||||||
|
seen_sources.add(failure.source_id)
|
||||||
|
|
||||||
|
source = sources_by_id.get(failure.source_id)
|
||||||
|
if source and source.last_success and source.last_success >= recent_cutoff:
|
||||||
|
skipped_recent += 1
|
||||||
|
continue
|
||||||
|
|
||||||
|
retry_ids.append(failure.id)
|
||||||
|
|
||||||
|
return retry_ids, {
|
||||||
|
"retried": len(retry_ids),
|
||||||
|
"skipped_duplicate_source": skipped_dup,
|
||||||
|
"skipped_recent_success": skipped_recent,
|
||||||
|
"skipped_no_source": skipped_no_source,
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
@bp.route("/retry-failed-bulk", methods=["POST"])
|
||||||
|
async def retry_failed_bulk():
|
||||||
|
"""Bulk-retry non-superseded failed downloads, with smart dedup.
|
||||||
|
|
||||||
|
Applies two filters that the old per-row Dashboard loop missed:
|
||||||
|
1. Per-source dedup — a source with N historical failures enqueues once.
|
||||||
|
2. Recent-success skip — a source whose `last_success` is within the
|
||||||
|
recent window (default 24h, overridable via `recent_window_hours`)
|
||||||
|
is considered healthy; we leave its stale failures alone rather
|
||||||
|
than pile on redundant retry work.
|
||||||
|
"""
|
||||||
|
data = await request.get_json() or {}
|
||||||
|
recent_window_hours = max(0, int(data.get("recent_window_hours", 24)))
|
||||||
|
recent_cutoff = utcnow() - timedelta(hours=recent_window_hours)
|
||||||
|
|
||||||
|
async with current_app.db_engine.connect() as conn:
|
||||||
|
async with AsyncSession(bind=conn) as session:
|
||||||
|
failures_result = await session.execute(
|
||||||
|
select(Download)
|
||||||
|
.where(
|
||||||
|
Download.status == DownloadStatus.FAILED,
|
||||||
|
Download.superseded == False,
|
||||||
|
)
|
||||||
|
.order_by(Download.source_id.asc(), Download.created_at.desc())
|
||||||
|
)
|
||||||
|
failures = list(failures_result.scalars().all())
|
||||||
|
|
||||||
|
source_ids = {f.source_id for f in failures if f.source_id is not None}
|
||||||
|
sources_by_id = {}
|
||||||
|
if source_ids:
|
||||||
|
sources_result = await session.execute(
|
||||||
|
select(Source).where(Source.id.in_(source_ids))
|
||||||
|
)
|
||||||
|
sources_by_id = {s.id: s for s in sources_result.scalars().all()}
|
||||||
|
|
||||||
|
retry_ids, summary = _plan_bulk_retry(failures, sources_by_id, recent_cutoff)
|
||||||
|
|
||||||
|
for failure in failures:
|
||||||
|
if failure.id in retry_ids:
|
||||||
|
failure.status = DownloadStatus.QUEUED
|
||||||
|
failure.error_type = None
|
||||||
|
failure.error_message = None
|
||||||
|
failure.started_at = None
|
||||||
|
failure.completed_at = None
|
||||||
|
|
||||||
|
await session.commit()
|
||||||
|
|
||||||
|
for download_id in retry_ids:
|
||||||
|
process_download.delay(download_id)
|
||||||
|
|
||||||
|
current_app.logger.info(
|
||||||
|
f"retry-failed-bulk: {summary} (recent_window_hours={recent_window_hours})"
|
||||||
|
)
|
||||||
|
|
||||||
|
return jsonify({
|
||||||
|
"message": f"Queued {summary['retried']} retries",
|
||||||
|
**summary,
|
||||||
|
"total_failures_considered": len(failures),
|
||||||
|
"recent_window_hours": recent_window_hours,
|
||||||
|
})
|
||||||
|
|
||||||
|
|
||||||
@bp.route("/<int:download_id>/retry", methods=["POST"])
|
@bp.route("/<int:download_id>/retry", methods=["POST"])
|
||||||
async def retry_download(download_id: int):
|
async def retry_download(download_id: int):
|
||||||
"""Retry a failed download."""
|
"""Retry a failed download."""
|
||||||
|
|||||||
@@ -0,0 +1,124 @@
|
|||||||
|
"""Unit tests for the planner helper behind POST /downloads/retry-failed-bulk.
|
||||||
|
|
||||||
|
The helper decides which non-superseded failed downloads get re-queued, with
|
||||||
|
per-source dedup and a "recent success" skip window. These tests exercise the
|
||||||
|
dedup + filter logic on plain objects (no DB).
|
||||||
|
"""
|
||||||
|
from datetime import datetime, timedelta, timezone
|
||||||
|
from types import SimpleNamespace
|
||||||
|
|
||||||
|
from app.api.downloads import _plan_bulk_retry
|
||||||
|
|
||||||
|
|
||||||
|
def _failure(failure_id, source_id, created_at):
|
||||||
|
return SimpleNamespace(id=failure_id, source_id=source_id, created_at=created_at)
|
||||||
|
|
||||||
|
|
||||||
|
def _source(source_id, last_success):
|
||||||
|
return SimpleNamespace(id=source_id, last_success=last_success)
|
||||||
|
|
||||||
|
|
||||||
|
# Tests expect failures already sorted (source_id ASC, created_at DESC) — same
|
||||||
|
# ordering the endpoint's SQL query produces.
|
||||||
|
|
||||||
|
def test_no_failures_returns_empty_plan():
|
||||||
|
retry_ids, summary = _plan_bulk_retry([], {}, datetime.now(timezone.utc))
|
||||||
|
assert retry_ids == []
|
||||||
|
assert summary == {
|
||||||
|
"retried": 0,
|
||||||
|
"skipped_duplicate_source": 0,
|
||||||
|
"skipped_recent_success": 0,
|
||||||
|
"skipped_no_source": 0,
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
def test_single_failure_retried_when_source_never_succeeded():
|
||||||
|
now = datetime.now(timezone.utc)
|
||||||
|
failures = [_failure(1, source_id=10, created_at=now - timedelta(hours=48))]
|
||||||
|
sources = {10: _source(10, last_success=None)}
|
||||||
|
|
||||||
|
retry_ids, summary = _plan_bulk_retry(failures, sources, now - timedelta(hours=24))
|
||||||
|
|
||||||
|
assert retry_ids == [1]
|
||||||
|
assert summary["retried"] == 1
|
||||||
|
|
||||||
|
|
||||||
|
def test_dedup_keeps_only_most_recent_per_source():
|
||||||
|
"""Given 3 failures for same source (sorted by created_at DESC), only the first survives."""
|
||||||
|
now = datetime.now(timezone.utc)
|
||||||
|
failures = [
|
||||||
|
_failure(301, source_id=10, created_at=now - timedelta(hours=1)), # most recent
|
||||||
|
_failure(302, source_id=10, created_at=now - timedelta(hours=24)),
|
||||||
|
_failure(303, source_id=10, created_at=now - timedelta(days=7)),
|
||||||
|
]
|
||||||
|
sources = {10: _source(10, last_success=None)}
|
||||||
|
|
||||||
|
retry_ids, summary = _plan_bulk_retry(failures, sources, now - timedelta(hours=24))
|
||||||
|
|
||||||
|
assert retry_ids == [301]
|
||||||
|
assert summary["skipped_duplicate_source"] == 2
|
||||||
|
|
||||||
|
|
||||||
|
def test_skip_source_with_recent_success():
|
||||||
|
"""Source that succeeded 2 hours ago should be skipped against a 24h window."""
|
||||||
|
now = datetime.now(timezone.utc)
|
||||||
|
failures = [_failure(1, source_id=10, created_at=now - timedelta(days=7))]
|
||||||
|
sources = {10: _source(10, last_success=now - timedelta(hours=2))}
|
||||||
|
|
||||||
|
retry_ids, summary = _plan_bulk_retry(failures, sources, now - timedelta(hours=24))
|
||||||
|
|
||||||
|
assert retry_ids == []
|
||||||
|
assert summary["skipped_recent_success"] == 1
|
||||||
|
|
||||||
|
|
||||||
|
def test_retry_source_with_old_success():
|
||||||
|
"""Source that succeeded 48h ago should still retry against a 24h window."""
|
||||||
|
now = datetime.now(timezone.utc)
|
||||||
|
failures = [_failure(1, source_id=10, created_at=now - timedelta(hours=1))]
|
||||||
|
sources = {10: _source(10, last_success=now - timedelta(hours=48))}
|
||||||
|
|
||||||
|
retry_ids, summary = _plan_bulk_retry(failures, sources, now - timedelta(hours=24))
|
||||||
|
|
||||||
|
assert retry_ids == [1]
|
||||||
|
assert summary["skipped_recent_success"] == 0
|
||||||
|
|
||||||
|
|
||||||
|
def test_orphaned_failure_without_source_id_is_skipped_separately():
|
||||||
|
"""Failures with source_id=None shouldn't explode — tallied under a distinct bucket."""
|
||||||
|
now = datetime.now(timezone.utc)
|
||||||
|
failures = [
|
||||||
|
_failure(1, source_id=None, created_at=now),
|
||||||
|
_failure(2, source_id=10, created_at=now),
|
||||||
|
]
|
||||||
|
sources = {10: _source(10, last_success=None)}
|
||||||
|
|
||||||
|
retry_ids, summary = _plan_bulk_retry(failures, sources, now - timedelta(hours=24))
|
||||||
|
|
||||||
|
assert retry_ids == [2]
|
||||||
|
assert summary["skipped_no_source"] == 1
|
||||||
|
assert summary["skipped_duplicate_source"] == 0
|
||||||
|
|
||||||
|
|
||||||
|
def test_mixed_scenario_combines_all_rules():
|
||||||
|
"""One healthy source (recent success, skipped), one broken source (multiple failures, dedup'd)."""
|
||||||
|
now = datetime.now(timezone.utc)
|
||||||
|
failures = [
|
||||||
|
_failure(101, source_id=10, created_at=now - timedelta(hours=1)), # healthy source, skip
|
||||||
|
_failure(201, source_id=20, created_at=now - timedelta(hours=2)), # broken, pick this one
|
||||||
|
_failure(202, source_id=20, created_at=now - timedelta(days=1)), # broken, dedup
|
||||||
|
_failure(203, source_id=20, created_at=now - timedelta(days=7)), # broken, dedup
|
||||||
|
]
|
||||||
|
sources = {
|
||||||
|
10: _source(10, last_success=now - timedelta(hours=3)), # healthy
|
||||||
|
20: _source(20, last_success=None), # never succeeded
|
||||||
|
}
|
||||||
|
|
||||||
|
retry_ids, summary = _plan_bulk_retry(failures, sources, now - timedelta(hours=24))
|
||||||
|
|
||||||
|
assert retry_ids == [201]
|
||||||
|
assert summary == {
|
||||||
|
"retried": 1,
|
||||||
|
"skipped_duplicate_source": 2,
|
||||||
|
"skipped_recent_success": 1,
|
||||||
|
"skipped_no_source": 0,
|
||||||
|
}
|
||||||
@@ -35,6 +35,7 @@ export const downloadsApi = {
|
|||||||
list: (params = {}) => api.get('/downloads', { params }),
|
list: (params = {}) => api.get('/downloads', { params }),
|
||||||
get: (id) => api.get(`/downloads/${id}`),
|
get: (id) => api.get(`/downloads/${id}`),
|
||||||
retry: (id) => api.post(`/downloads/${id}/retry`),
|
retry: (id) => api.post(`/downloads/${id}/retry`),
|
||||||
|
retryFailedBulk: (recentWindowHours = 24) => api.post('/downloads/retry-failed-bulk', { recent_window_hours: recentWindowHours }),
|
||||||
stats: (params = {}) => api.get('/downloads/stats', { params }),
|
stats: (params = {}) => api.get('/downloads/stats', { params }),
|
||||||
recentActivity: (params = {}) => api.get('/downloads/recent-activity', { params }),
|
recentActivity: (params = {}) => api.get('/downloads/recent-activity', { params }),
|
||||||
resetOrphaned: (thresholdMinutes = 0) => api.post('/downloads/reset-orphaned', { threshold_minutes: thresholdMinutes }),
|
resetOrphaned: (thresholdMinutes = 0) => api.post('/downloads/reset-orphaned', { threshold_minutes: thresholdMinutes }),
|
||||||
|
|||||||
@@ -43,6 +43,10 @@ export const useDownloadsStore = defineStore('downloads', () => {
|
|||||||
return await downloadsApi.retry(id)
|
return await downloadsApi.retry(id)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
async function retryFailedBulk(recentWindowHours = 24) {
|
||||||
|
return await downloadsApi.retryFailedBulk(recentWindowHours)
|
||||||
|
}
|
||||||
|
|
||||||
async function resetOrphaned(thresholdMinutes = 0) {
|
async function resetOrphaned(thresholdMinutes = 0) {
|
||||||
return await downloadsApi.resetOrphaned(thresholdMinutes)
|
return await downloadsApi.resetOrphaned(thresholdMinutes)
|
||||||
}
|
}
|
||||||
@@ -63,6 +67,7 @@ export const useDownloadsStore = defineStore('downloads', () => {
|
|||||||
fetchRecentActivity,
|
fetchRecentActivity,
|
||||||
fetchStats,
|
fetchStats,
|
||||||
retryDownload,
|
retryDownload,
|
||||||
|
retryFailedBulk,
|
||||||
resetOrphaned,
|
resetOrphaned,
|
||||||
requeueStale,
|
requeueStale,
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -592,18 +592,20 @@ async function checkAllSources() {
|
|||||||
async function retryAllFailed() {
|
async function retryAllFailed() {
|
||||||
retryingAll.value = true
|
retryingAll.value = true
|
||||||
try {
|
try {
|
||||||
const response = await downloadsStore.fetchDownloads({ status: 'failed', per_page: 100, exclude_superseded: true })
|
const response = await downloadsStore.retryFailedBulk()
|
||||||
const failed = response.items
|
const {
|
||||||
let retried = 0
|
retried,
|
||||||
for (const dl of failed) {
|
skipped_duplicate_source: dupSkipped = 0,
|
||||||
try {
|
skipped_recent_success: recentSkipped = 0,
|
||||||
await downloadsStore.retryDownload(dl.id)
|
skipped_no_source: noSourceSkipped = 0,
|
||||||
retried++
|
} = response.data
|
||||||
} catch (e) {
|
|
||||||
console.error(`Failed to retry ${dl.id}:`, e)
|
const parts = [`Queued ${retried} ${retried === 1 ? 'retry' : 'retries'}`]
|
||||||
}
|
if (dupSkipped) parts.push(`${dupSkipped} duplicate-source skipped`)
|
||||||
}
|
if (recentSkipped) parts.push(`${recentSkipped} recently succeeded, skipped`)
|
||||||
notifications.success(`Queued retry for ${retried} downloads`)
|
if (noSourceSkipped) parts.push(`${noSourceSkipped} orphaned, skipped`)
|
||||||
|
notifications.success(parts.join(' · '))
|
||||||
|
|
||||||
await downloadsStore.fetchStats()
|
await downloadsStore.fetchStats()
|
||||||
await fetchActiveDownloads()
|
await fetchActiveDownloads()
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
|||||||
Reference in New Issue
Block a user