GallerySubscriber/docs/superpowers/specs/2026-03-18-comprehensive-review-design.md

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