Add comprehensive review design spec
Covers four confirmed bugs, superseded-at-write-time refactor, error classification improvements, and code quality cleanup. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,263 @@
|
||||
# GallerySubscriber Comprehensive Review — Design Spec
|
||||
|
||||
**Date:** 2026-03-18
|
||||
**Approach:** Layered fixes (bugs → superseded logic → error classification → code quality)
|
||||
**Scope:** Confirmed bugs, retry/superseded behaviour, error classification, code quality
|
||||
|
||||
---
|
||||
|
||||
## Section 1: Bug Fixes
|
||||
|
||||
### Bug 1 — `process_download` retry silently skips
|
||||
|
||||
**File:** `backend/app/tasks/downloads.py:574`
|
||||
|
||||
`process_download(download_id)` fetches the download record, gets its `source_id`, then calls
|
||||
`_download_source_async(source_id)` without passing `download_id`. Since `download_id` is `None`,
|
||||
the function enters the `not download_id or download is None` branch (line 172) and queries for any
|
||||
`QUEUED` or `RUNNING` download for that source. The API retry endpoint already reset that record to
|
||||
`QUEUED`, so the guard at line 180 finds it and returns `source_already_active`. The retry never
|
||||
runs. Recovery eventually happens via the 30-minute stale-job maintenance task.
|
||||
|
||||
**Fix:** Pass `download_id` through:
|
||||
```python
|
||||
return await _download_source_async(download.source_id, download_id=download_id)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Bug 2 — Orphaned job reset message always says "None"
|
||||
|
||||
**File:** `backend/app/tasks/maintenance.py:177-178`
|
||||
|
||||
```python
|
||||
job.started_at = None # cleared on line 177
|
||||
job.error_message = f"Reset from orphaned running state (was running since {job.started_at})" # now None on line 178
|
||||
```
|
||||
|
||||
**Fix:** Capture `started_at` before clearing it:
|
||||
```python
|
||||
original_started_at = job.started_at
|
||||
job.started_at = None
|
||||
job.error_message = f"Reset from orphaned running state (was running since {original_started_at})"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### Bug 3 — Deprecated `asyncio.get_event_loop()`
|
||||
|
||||
**File:** `backend/app/services/gallery_dl.py:616`
|
||||
|
||||
`asyncio.get_event_loop()` is fragile inside a running async context in Python 3.10+. The safer and
|
||||
more explicit replacement is `asyncio.get_running_loop()`, which raises `RuntimeError` rather than
|
||||
silently returning a potentially wrong loop if the call path changes.
|
||||
|
||||
**Fix:** Replace with `asyncio.get_running_loop()`.
|
||||
|
||||
---
|
||||
|
||||
### Bug 4 — `datetime.utcnow()` inconsistency
|
||||
|
||||
**File:** `backend/app/services/gallery_dl.py:561, 628`
|
||||
|
||||
Two uses of `datetime.utcnow()` (naive datetime, deprecated in Python 3.12) while the rest of the
|
||||
codebase uses `utcnow()` from `app.models.base` (timezone-aware).
|
||||
|
||||
**Fix:** Import and use `utcnow()` from `app.models.base` in `gallery_dl.py`.
|
||||
|
||||
---
|
||||
|
||||
## Section 2: Superseded Logic (Write-time)
|
||||
|
||||
### Problem
|
||||
|
||||
`_not_superseded_condition()` is a correlated subquery evaluated at query time on every call to
|
||||
`/api/downloads/stats`, `/api/downloads/recent-activity`, and filtered download lists. It is
|
||||
duplicated across multiple call sites and adds complexity to every query.
|
||||
|
||||
### Model Change
|
||||
|
||||
Add a `superseded` boolean column to the `Download` model and `downloads` table (default `False`).
|
||||
Add the mapped column to `backend/app/models/download.py` alongside the other columns, and add
|
||||
`"superseded": self.superseded` to `Download.to_dict()` so the field is visible in API responses.
|
||||
|
||||
### Write Logic
|
||||
|
||||
When a download completes successfully (status `COMPLETED`, regardless of `file_count`), bulk-update
|
||||
all prior `FAILED` downloads for the same source to `superseded = True`. This write happens in Phase
|
||||
3 of `_download_source_async`, after setting `download.status = COMPLETED` but **before** calling
|
||||
`publish_event("download.completed", ...)`. Placing it before the event ensures the database state
|
||||
is already correct if any event consumer queries it before the session commits:
|
||||
|
||||
```python
|
||||
download.status = DownloadStatus.COMPLETED
|
||||
# Mark prior failures as superseded before publishing the event
|
||||
await session.execute(
|
||||
update(Download)
|
||||
.where(
|
||||
Download.source_id == source_id,
|
||||
Download.status == DownloadStatus.FAILED,
|
||||
)
|
||||
.values(superseded=True)
|
||||
)
|
||||
# Now safe to publish — subscribers see updated state
|
||||
publish_event("download.completed", { ... })
|
||||
```
|
||||
|
||||
This only affects `FAILED` records. `QUEUED` retries of a failed download are not touched — they
|
||||
will either run and complete or be superseded by the next successful run. There is no meaningful
|
||||
race risk: if two tasks complete for the same source concurrently, both will mark prior failures as
|
||||
superseded, which is idempotent and correct.
|
||||
|
||||
### Migration
|
||||
|
||||
One Alembic migration with three steps:
|
||||
|
||||
```sql
|
||||
-- 1. Add column
|
||||
ALTER TABLE downloads ADD COLUMN superseded BOOLEAN NOT NULL DEFAULT FALSE;
|
||||
|
||||
-- 2. Backfill: mark FAILED downloads as superseded where a later COMPLETED download
|
||||
-- exists for the same source. Required — omitting this causes all historic
|
||||
-- superseded failures to reappear in the dashboard immediately after deployment.
|
||||
UPDATE downloads d
|
||||
SET superseded = TRUE
|
||||
WHERE d.status = 'failed'
|
||||
AND EXISTS (
|
||||
SELECT 1 FROM downloads d2
|
||||
WHERE d2.source_id = d.source_id
|
||||
AND d2.status = 'completed'
|
||||
AND d2.created_at > d.created_at
|
||||
);
|
||||
|
||||
-- 3. Partial index covering the dominant query pattern (status + superseded = FALSE).
|
||||
-- A single-column boolean index would be ignored by the planner in favour of
|
||||
-- status-based indexes; this form is used by all three affected query sites.
|
||||
CREATE INDEX idx_downloads_not_superseded ON downloads (status)
|
||||
WHERE superseded = FALSE;
|
||||
```
|
||||
|
||||
The backfill is **required**, not optional. Without it, all previously-superseded failures will
|
||||
appear as active failures immediately after the migration runs.
|
||||
|
||||
### Query Changes
|
||||
|
||||
All uses of `_not_superseded_condition()` are replaced with `Download.superseded == False`.
|
||||
The `_not_superseded_condition()` function is removed.
|
||||
|
||||
Affected sites:
|
||||
- `GET /api/downloads/stats` — `active_failed` count
|
||||
- `GET /api/downloads/recent-activity` — failed downloads filter
|
||||
- `GET /api/downloads` with `exclude_superseded=true` — list filter
|
||||
|
||||
### Impact on UI
|
||||
|
||||
- "Retry All Failed" in Dashboard — only sees unsuperseded failures
|
||||
- "Sources Needing Attention" — clears naturally when source recovers
|
||||
- Downloads list `exclude_superseded` filter — works via column instead of subquery
|
||||
|
||||
---
|
||||
|
||||
## Section 3: Error Classification Improvements
|
||||
|
||||
### 3a — Debug logging for pattern matches
|
||||
|
||||
Add `logger.debug()` output when `_categorize_error()` returns a result, identifying which pattern
|
||||
triggered the classification:
|
||||
|
||||
```python
|
||||
logger.debug(f"Error classified as {error_type} via pattern: '{matched_pattern}'")
|
||||
```
|
||||
|
||||
This is zero-cost in production (DEBUG level) and makes the export-failed-logs analysis tool much
|
||||
more useful for diagnosing misclassifications.
|
||||
|
||||
### 3b — Extract pattern lists to class-level constants
|
||||
|
||||
Move inline pattern lists (`auth_error_patterns`, `rate_limit_patterns`, `not_found_patterns`, etc.)
|
||||
from inside `_categorize_error()` to named class-level constants on `GalleryDLService`. No logic
|
||||
change. Makes patterns easy to scan, extend, and test in isolation.
|
||||
|
||||
### 3c — Fix `no_new_content` for empty output on return code 1
|
||||
|
||||
**File:** `backend/app/services/gallery_dl.py:393-394`
|
||||
|
||||
```python
|
||||
if return_code in (0, 1) and not stdout.strip():
|
||||
return ErrorType.NO_NEW_CONTENT, "No new content to download"
|
||||
```
|
||||
|
||||
Gallery-dl return code `1` means "some extractions failed". Treating it as `no_new_content` when
|
||||
stdout is empty can mask real failures.
|
||||
|
||||
**Fix:** Only treat empty stdout as `NO_NEW_CONTENT` for return code `0`:
|
||||
```python
|
||||
if return_code == 0 and not stdout.strip():
|
||||
return ErrorType.NO_NEW_CONTENT, "No new content to download"
|
||||
```
|
||||
|
||||
**Trade-off:** Gallery-dl may also return code `1` with empty stdout for genuinely empty sources
|
||||
(user with no posts, empty gallery). After this fix those runs will fall through to `UNKNOWN_ERROR`
|
||||
rather than `NO_NEW_CONTENT`. This is acceptable: it is better to surface these as needing attention
|
||||
than to silently swallow potential errors. If this becomes noisy in practice, a specific
|
||||
gallery-dl "no URLs extracted" pattern can be added to the `no_new_content` detection phase.
|
||||
|
||||
---
|
||||
|
||||
## Section 4: Code Quality
|
||||
|
||||
### 4a — Dashboard `checkAllSources` serializes requests
|
||||
|
||||
**File:** `frontend/src/views/Dashboard.vue:464-471`
|
||||
|
||||
Sequential `await` inside a `for` loop means sources are checked one at a time. With many sources
|
||||
this blocks the UI.
|
||||
|
||||
**Fix:** Use `Promise.allSettled()` to fire all `triggerCheck` calls in parallel, then count
|
||||
resolved vs rejected results. Note: with a large number of sources (> 20), the simultaneous burst
|
||||
of requests may be worth rate-limiting or batching; the existing `source_already_active` guard on
|
||||
the backend prevents duplicate records regardless.
|
||||
|
||||
### 4b — `AsyncSession` not closed in API endpoints
|
||||
|
||||
**File:** `backend/app/api/downloads.py` — all six route handlers use the same unclosed-session
|
||||
pattern: `list_downloads` (line 45), `get_download` (line 105), `retry_download` (line 139),
|
||||
`recent_activity` (line 179), `get_stats` (line 304), `export_failed_logs` (line 373).
|
||||
|
||||
Other API blueprint files (`sources.py`, `subscriptions.py`, `credentials.py`) should also be
|
||||
checked and fixed if they use the same pattern.
|
||||
|
||||
**Fix:** Replace `session = AsyncSession(bind=conn)` with `async with AsyncSession(bind=conn) as session:`
|
||||
in all affected handlers to ensure proper cleanup on all paths.
|
||||
|
||||
### 4c — `_count_downloaded_files` over-counts
|
||||
|
||||
**File:** `backend/app/services/gallery_dl.py:517-531`
|
||||
|
||||
The current heuristic (exclude lines starting with `#` or containing "skip") still counts
|
||||
gallery-dl log lines and, with `--verbose` enabled, HTTP request/response lines. The most robust
|
||||
fix is to count only lines that look like absolute file paths (starting with `/`), which is the
|
||||
actual gallery-dl output format for downloaded files. This replaces the current approach entirely
|
||||
and handles the `--verbose` output correctly.
|
||||
|
||||
### 4d — Duplicate DB helpers across task files
|
||||
|
||||
**Files:** `backend/app/tasks/downloads.py` and `backend/app/tasks/maintenance.py`
|
||||
|
||||
Both files define identical `get_async_session()` and `_cleanup_engine()` functions.
|
||||
|
||||
**Fix:** Extract to `backend/app/tasks/db.py` and import from both task files.
|
||||
|
||||
**Constraint:** `db.py` must only import from `app.config` and SQLAlchemy — no imports from
|
||||
`downloads.py` or `maintenance.py` — to avoid creating a circular import. The existing late import
|
||||
in `maintenance.py` (`from app.tasks.downloads import download_source`) is already there to prevent
|
||||
a circular dependency and must be preserved.
|
||||
|
||||
---
|
||||
|
||||
## Delivery Order
|
||||
|
||||
1. Section 1 bugs (all four — small, safe, independently verifiable)
|
||||
2. Section 2 superseded migration + model + write logic + query cleanup
|
||||
3. Section 3 error classification improvements
|
||||
4. Section 4 code quality (can be done alongside or after sections 2–3)
|
||||
Reference in New Issue
Block a user