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:

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

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:

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:

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:

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

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

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:

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)