# GallerySubscriber Comprehensive Review Implementation Plan > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. **Goal:** Fix four confirmed bugs, replace the query-time superseded subquery with a write-time column, improve error classification, and clean up code quality issues across the backend and frontend. **Architecture:** Layered delivery — bugs first (safe, atomic), then the superseded DB migration + logic, then error classification restructuring, then quality cleanup. Each task produces a working commit. **Tech Stack:** Python 3.10+, Quart, SQLAlchemy 2 async, Alembic, Celery, Vue 3, PostgreSQL 15 **Spec:** `docs/superpowers/specs/2026-03-18-comprehensive-review-design.md` **Note on tests:** This project has no existing test infrastructure. Steps include manual verification commands instead of automated tests. --- ## Files Modified | File | Change | |------|--------| | `backend/app/tasks/downloads.py` | Bug 1 fix (pass download_id); Section 2 write logic | | `backend/app/tasks/maintenance.py` | Bug 2 fix (started_at message); import from db.py | | `backend/app/services/gallery_dl.py` | Bug 3 (get_running_loop); Bug 4 (utcnow); Sec 3a/3b/3c; 4c | | `backend/app/models/download.py` | Add `superseded` column + `to_dict()` entry | | `backend/alembic/versions/003_add_superseded.py` | New migration: column + backfill + index | | `backend/app/api/downloads.py` | Remove `_not_superseded_condition()`; replace with `superseded==False`; fix sessions | | `backend/app/api/credentials.py` | Fix unclosed sessions | | `backend/app/api/settings.py` | Fix unclosed sessions | | `backend/app/api/sources.py` | Fix unclosed sessions | | `backend/app/api/subscriptions.py` | Fix unclosed sessions | | `frontend/src/views/Dashboard.vue` | Parallelize `checkAllSources` | | `backend/app/tasks/db.py` | **New file** — shared `get_async_session` + `_cleanup_engine` | --- ## Task 1: Fix Bug 1 — process_download doesn't pass download_id **Files:** - Modify: `backend/app/tasks/downloads.py:574` - [ ] **Step 1: Open the file and locate the `_process` inner function inside `process_download`** In `backend/app/tasks/downloads.py`, find the `process_download` task (around line 551). Inside it is an `async def _process():` function. Near the end of `_process`, around line 574, is: ```python return await _download_source_async(download.source_id) ``` - [ ] **Step 2: Fix the call to pass download_id** Change that line to: ```python return await _download_source_async(download.source_id, download_id=download_id) ``` The `download_id` variable is the parameter of the outer `process_download(self, download_id: int)` function and is in scope for the inner `_process` closure. - [ ] **Step 3: Verify the change looks correct** The `_process` function body should now end with: ```python if not download.source_id: return {"download_id": download_id, "status": "error", "error": "No source associated"} # Delegate to download_source return await _download_source_async(download.source_id, download_id=download_id) ``` - [ ] **Step 4: Commit** ```bash cd /home/bvandeusen/Nextcloud/Projects/GallerySubscriber git add backend/app/tasks/downloads.py git commit -m "fix: pass download_id in process_download to prevent retry skip" ``` --- ## Task 2: Fix Bug 2 — Orphaned job message always shows None **Files:** - Modify: `backend/app/tasks/maintenance.py:176-178` - [ ] **Step 1: Locate the loop in `_reset_orphaned_running_jobs_async`** Find the `for job in orphaned_jobs:` loop. The current code: ```python for job in orphaned_jobs: job.status = DownloadStatus.QUEUED job.started_at = None job.error_message = f"Reset from orphaned running state (was running since {job.started_at})" ``` - [ ] **Step 2: Capture started_at before clearing it** Replace those four lines with: ```python for job in orphaned_jobs: original_started_at = job.started_at job.status = DownloadStatus.QUEUED job.started_at = None job.error_message = f"Reset from orphaned running state (was running since {original_started_at})" ``` - [ ] **Step 3: Commit** ```bash git add backend/app/tasks/maintenance.py git commit -m "fix: capture started_at before clearing in orphaned job reset message" ``` --- ## Task 3: Fix Bugs 3 and 4 — get_event_loop + datetime.utcnow in gallery_dl.py **Files:** - Modify: `backend/app/services/gallery_dl.py` - [ ] **Step 1: Fix get_event_loop (Bug 3)** Find line 616 (inside the `download` method, in the `try` block): ```python loop = asyncio.get_event_loop() ``` Change to: ```python loop = asyncio.get_running_loop() ``` - [ ] **Step 2: Fix datetime.utcnow (Bug 4)** At the top of the file, add the import after the existing imports: ```python from app.models.base import utcnow ``` Then find the two `datetime.utcnow()` calls in the `download` method (around lines 561 and 628): ```python started_at = datetime.utcnow().isoformat() ``` and: ```python completed_at = datetime.utcnow().isoformat() ``` Change both to: ```python started_at = utcnow().isoformat() ``` and: ```python completed_at = utcnow().isoformat() ``` There is also a `datetime.utcnow().isoformat()` in the `except subprocess.TimeoutExpired` block and the `except Exception` block — change those too. Search for all occurrences: ```bash grep -n "datetime.utcnow" backend/app/services/gallery_dl.py ``` - [ ] **Step 3: Remove the now-dead local datetime import inside the `download` method** The `download` method has a local import block near the top of its body (around line 557): ```python import asyncio from datetime import datetime ``` After replacing all `datetime.utcnow()` calls with `utcnow()`, the `from datetime import datetime` line is no longer used inside this method. Remove it. The `import asyncio` line can stay (it is used for `asyncio.get_running_loop()`). - [ ] **Step 4: Verify no remaining datetime.utcnow calls** ```bash grep -n "datetime.utcnow" backend/app/services/gallery_dl.py # Expected: no output ``` - [ ] **Step 5: Commit** ```bash git add backend/app/services/gallery_dl.py git commit -m "fix: replace get_event_loop with get_running_loop and utcnow consistently" ``` --- ## Task 4: Add superseded column to Download model **Files:** - Modify: `backend/app/models/download.py` - [ ] **Step 1: Add the column import** At the top of the file, `Boolean` is not yet imported from sqlalchemy. Add it to the import: ```python from sqlalchemy import String, Integer, Text, ForeignKey, BigInteger, Boolean ``` - [ ] **Step 2: Add the mapped column to the Download class** After the `total_size` column (around line 56), add: ```python # Superseded: True if a later successful download has run for this source superseded: Mapped[bool] = mapped_column(Boolean, default=False, nullable=False, server_default="false") ``` - [ ] **Step 3: Add to to_dict()** In the `to_dict` method, add `"superseded": self.superseded` after `"completed_at"`: ```python "completed_at": format_datetime(self.completed_at), "superseded": self.superseded, "metadata": self.metadata_, ``` - [ ] **Step 4: Commit** ```bash git add backend/app/models/download.py git commit -m "feat: add superseded column to Download model" ``` --- ## Task 5: Create Alembic migration for superseded column **Files:** - Create: `backend/alembic/versions/003_add_superseded.py` - [ ] **Step 1: Create the migration file** Create `backend/alembic/versions/003_add_superseded.py` with this content: ```python """Add superseded column to downloads Revision ID: 003 Revises: 002 Create Date: 2026-03-18 """ from typing import Sequence, Union from alembic import op import sqlalchemy as sa revision: str = '003' down_revision: Union[str, None] = '002' branch_labels: Union[str, Sequence[str], None] = None depends_on: Union[str, Sequence[str], None] = None def upgrade() -> None: # Add superseded column with default False op.add_column( 'downloads', sa.Column('superseded', sa.Boolean(), nullable=False, server_default='false') ) # Backfill: mark FAILED downloads as superseded where a later COMPLETED # download exists for the same source. REQUIRED — without this, all # previously-superseded failures reappear in the dashboard immediately. op.execute(""" 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 ) """) # Partial index covering the dominant query pattern: status + superseded = FALSE. # A single boolean column index would be ignored in favour of status indexes; # this form matches the WHERE clauses at all three affected query sites. op.create_index( 'idx_downloads_not_superseded', 'downloads', ['status'], postgresql_where=sa.text('superseded = FALSE') ) def downgrade() -> None: op.drop_index('idx_downloads_not_superseded', table_name='downloads') op.drop_column('downloads', 'superseded') ``` - [ ] **Step 2: Run the migration** ```bash cd /home/bvandeusen/Nextcloud/Projects/GallerySubscriber docker compose exec app alembic upgrade head ``` Expected output: `Running upgrade 002 -> 003, Add superseded column to downloads` - [ ] **Step 3: Verify the column and index exist** ```bash docker compose exec db psql -U postgres -d gallerysubscriber -c "\d downloads" | grep superseded # Expected: superseded | boolean | not null | false docker compose exec db psql -U postgres -d gallerysubscriber -c "\di idx_downloads_not_superseded" # Expected: index listed ``` - [ ] **Step 4: Commit** ```bash git add backend/alembic/versions/003_add_superseded.py git commit -m "feat: migration to add superseded column with backfill and partial index" ``` --- ## Task 6: Replace _not_superseded_condition() with superseded==False in api/downloads.py **Files:** - Modify: `backend/app/api/downloads.py` - [ ] **Step 1: Remove `exists` from imports if no longer needed** After removing `_not_superseded_condition()`, the `exists` import in `api/downloads.py` becomes unused. The current import line is: ```python from sqlalchemy import select, func, and_, or_, desc, exists ``` Check whether `exists` is still referenced anywhere in the file: ```bash grep -n "exists" backend/app/api/downloads.py ``` If no remaining uses, remove `exists` from the import. - [ ] **Step 2: Replace the `_not_superseded_condition()` function** Delete the entire `_not_superseded_condition()` function (lines 18-29). It will no longer be used. - [ ] **Step 3: Update `list_downloads` — exclude_superseded filter** Find the block around line 63: ```python if exclude_superseded and status == DownloadStatus.FAILED: filters.append(_not_superseded_condition()) ``` Replace with: ```python if exclude_superseded and status == DownloadStatus.FAILED: filters.append(Download.superseded == False) ``` - [ ] **Step 4: Update `recent_activity` — failed filter** Find the `or_` block around line 188: ```python and_( Download.status == DownloadStatus.FAILED, _not_superseded_condition(), ), ``` Replace with: ```python and_( Download.status == DownloadStatus.FAILED, Download.superseded == False, ), ``` - [ ] **Step 5: Update `get_stats` — active_failed query** Find the `active_failed_query` block around line 331: ```python active_failed_query = select(func.count()).select_from(Download).where( and_( Download.status == DownloadStatus.FAILED, _not_superseded_condition(), ) ) ``` Replace with: ```python active_failed_query = select(func.count()).select_from(Download).where( and_( Download.status == DownloadStatus.FAILED, Download.superseded == False, ) ) ``` - [ ] **Step 6: Remove the unused `exists` import if no longer needed** Check: `grep -n "exists" backend/app/api/downloads.py` If `exists` is no longer referenced, remove it from the sqlalchemy import line. - [ ] **Step 7: Verify no remaining references to _not_superseded_condition** ```bash grep -rn "_not_superseded_condition" backend/ # Expected: no output ``` - [ ] **Step 8: Commit** ```bash git add backend/app/api/downloads.py git commit -m "feat: replace _not_superseded_condition subquery with superseded column filter" ``` --- ## Task 7: Add superseded write logic to download task **Files:** - Modify: `backend/app/tasks/downloads.py` - [ ] **Step 1: Add `update` to imports** The current sqlalchemy import line in `backend/app/tasks/downloads.py` is: ```python from sqlalchemy import select, and_, or_ ``` Change it to: ```python from sqlalchemy import select, and_, or_, update ``` - [ ] **Step 2: Locate the COMPLETED path in Phase 3** In `_download_source_async`, find Phase 3 (around line 297). Find the `if dl_result.success:` block: ```python if dl_result.success: download.status = DownloadStatus.COMPLETED source.last_success = utcnow() source.error_count = 0 logger.info(...) publish_event("download.completed", { ... }) ``` - [ ] **Step 3: Insert the bulk supersede UPDATE before publish_event** Insert the UPDATE after `source.error_count = 0` and before `publish_event`: ```python if dl_result.success: download.status = DownloadStatus.COMPLETED source.last_success = utcnow() source.error_count = 0 logger.info(f"Download completed for {subscription_name}/{source_platform}: {dl_result.files_downloaded} files") # Mark any prior failures for this source as superseded await session.execute( update(Download) .where( Download.source_id == source_id, Download.status == DownloadStatus.FAILED, ) .values(superseded=True) ) publish_event("download.completed", { "download_id": actual_download_id, "source_id": source_id, "subscription_name": subscription_name, "platform": source_platform, "file_count": dl_result.files_downloaded, "duration_seconds": dl_result.duration_seconds, }) ``` - [ ] **Step 4: Verify the ordering is correct** The `if dl_result.success:` block should contain (in order): 1. Set `download.status = COMPLETED` 2. Update source success fields (`last_success`, `error_count`) 3. Log the completion 4. Bulk UPDATE prior failures to superseded 5. `publish_event("download.completed", ...)` **Outside** the `if/else` block (shared by both success and failure paths, unchanged): - `source.last_check = utcnow()` - `await session.commit()` Do not move `source.last_check` or `session.commit()` inside the success block — they currently sit after the entire `if dl_result.success: ... else: ...` structure and must remain there. - [ ] **Step 5: Commit** ```bash git add backend/app/tasks/downloads.py git commit -m "feat: mark prior failures as superseded on successful download" ``` --- ## Task 8: Fix unclosed AsyncSession in all API blueprints The pattern `session = AsyncSession(bind=conn)` is used 31 times across 5 API files. All need changing to `async with AsyncSession(bind=conn) as session:` with the body indented inside the `with` block. **Files:** - Modify: `backend/app/api/downloads.py` - Modify: `backend/app/api/credentials.py` - Modify: `backend/app/api/settings.py` - Modify: `backend/app/api/sources.py` - Modify: `backend/app/api/subscriptions.py` The pattern to apply in each handler: **Before:** ```python async with current_app.db_engine.connect() as conn: session = AsyncSession(bind=conn) # ... session usage ... return jsonify(...) ``` **After:** ```python async with current_app.db_engine.connect() as conn: async with AsyncSession(bind=conn) as session: # ... session usage (indented one level deeper) ... return jsonify(...) ``` - [ ] **Step 1: Fix downloads.py** Apply the pattern to all 6 handlers in `backend/app/api/downloads.py`. Each `async with current_app.db_engine.connect() as conn:` block gets an inner `async with AsyncSession(bind=conn) as session:` with the session body re-indented. - [ ] **Step 2: Fix credentials.py** Apply to all 5 occurrences in `backend/app/api/credentials.py`. - [ ] **Step 3: Fix settings.py** Apply to all 6 occurrences in `backend/app/api/settings.py`. - [ ] **Step 4: Fix sources.py** Apply to all 6 occurrences in `backend/app/api/sources.py`. - [ ] **Step 5: Fix subscriptions.py** Apply to all 8 occurrences in `backend/app/api/subscriptions.py`. - [ ] **Step 6: Verify no bare session assignments remain** ```bash grep -rn "session = AsyncSession(bind=" backend/app/api/ # Expected: no output ``` - [ ] **Step 7: Commit** ```bash git add backend/app/api/ git commit -m "fix: wrap AsyncSession in context manager across all API blueprints" ``` --- ## Task 9: Error classification — extract patterns, add logging, fix return code 1 **Files:** - Modify: `backend/app/services/gallery_dl.py` - [ ] **Step 1: Extract pattern lists to class-level constants** Inside the `GalleryDLService` class, before `PLATFORM_DEFAULTS`, add these class-level constants: ```python # Error classification patterns — used by _categorize_error() AUTH_ERROR_PATTERNS = [ "unauthorized", "login required", "please login", "must be logged in", "authentication required", "authenticationerror", "refresh-token", "session expired", "invalid cookie", "cookies are expired", "cookies have expired", "not logged in", ] RATE_LIMIT_PATTERNS = [ '" 429 ', "rate limit", "too many requests", "ratelimit", "throttl", ] NOT_FOUND_PATTERNS = [ '" 404 ', "error 404", "status: 404", "status code: 404", "404 not found", "page not found", "user not found", "creator not found", "artist not found", "content no longer available", "has been deleted", "account.*deleted", "profile.*not.*exist", ] GENERIC_NOT_FOUND_PATTERNS = ["does not exist", "no longer available"] NETWORK_ERROR_PATTERNS = [ "timed out", "read timed out", "connect timed out", "connection timed out", "connection refused", "connection reset", "network unreachable", "name resolution failed", "name or service not known", "nodename nor servname provided", "ssl: certificate_verify_failed", "ssl: wrong_version_number", "certificate verify failed", "[errno", ] ACCESS_DENIED_PATTERNS = [ '" 403 ', "error 403", "forbidden", "access denied", "permission denied", "tier required", "pledge required", ] ``` - [ ] **Step 2: Rewrite `_categorize_error` to use the constants and add debug logging** Replace the inline lists in `_categorize_error` with references to the class constants (e.g. `self.AUTH_ERROR_PATTERNS`). Wrap each `return` statement with a debug log. Example for auth errors: ```python has_401_error = ( '" 401 ' in combined or "401 unauthorized" in combined or "error 401" in combined or "status: 401" in combined or "status code: 401" in combined ) if has_401_error or any(pattern in combined for pattern in self.AUTH_ERROR_PATTERNS): matched = "401 HTTP response" if has_401_error else next(p for p in self.AUTH_ERROR_PATTERNS if p in combined) logger.debug(f"Error classified as auth_error via pattern: '{matched}'") return ErrorType.AUTH_ERROR, "Authentication failed - cookies may be expired or invalid" ``` Apply the same pattern for all other error types. For `NO_NEW_CONTENT` returns, log the trigger (skip line count, skip text, or empty stdout). - [ ] **Step 3: Fix return code 1 empty stdout (3c)** Find the empty-stdout check in Phase 1 (after the skip detection): ```python if return_code in (0, 1) and not stdout.strip(): return ErrorType.NO_NEW_CONTENT, "No new content to download" ``` Change to: ```python if return_code == 0 and not stdout.strip(): logger.debug("Error classified as no_new_content via: empty stdout with return code 0") return ErrorType.NO_NEW_CONTENT, "No new content to download" ``` - [ ] **Step 4: Commit** ```bash git add backend/app/services/gallery_dl.py git commit -m "refactor: extract error patterns to constants, add debug logging, fix rc1 no_new_content" ``` --- ## Task 10: Fix _count_downloaded_files heuristic **Files:** - Modify: `backend/app/services/gallery_dl.py` - [ ] **Step 1: Locate the method** Find `_count_downloaded_files` (around line 517). Current implementation: ```python for line in stdout.strip().split("\n"): line = line.strip() if line and not line.startswith("#") and "skip" not in line.lower(): count += 1 ``` - [ ] **Step 2: Replace with absolute path heuristic** With `--verbose` enabled, gallery-dl writes downloaded file paths to stdout as absolute paths (starting with `/`). Log lines use `[` as a prefix character. HTTP lines go to stderr. Count only lines starting with `/`: ```python for line in stdout.strip().split("\n"): line = line.strip() if line.startswith("/"): count += 1 ``` - [ ] **Step 3: Commit** ```bash git add backend/app/services/gallery_dl.py git commit -m "fix: count only absolute path lines in _count_downloaded_files" ``` --- ## Task 11: Extract shared DB helpers to tasks/db.py **Files:** - Create: `backend/app/tasks/db.py` - Modify: `backend/app/tasks/downloads.py` - Modify: `backend/app/tasks/maintenance.py` - [ ] **Step 1: Create backend/app/tasks/db.py** ```python """Shared async database helpers for Celery tasks. Provides get_async_session() and _cleanup_engine() used by both downloads.py and maintenance.py. This module must not import from either of those files to avoid circular imports. """ from sqlalchemy.ext.asyncio import create_async_engine, AsyncSession, async_sessionmaker from app.config import get_settings def get_async_session(): """Get async session factory for Celery tasks. Creates a fresh engine and session factory each time to avoid event loop issues when asyncio.run() creates new loops. """ settings = get_settings() engine = create_async_engine(settings.async_database_url, echo=False) session_factory = async_sessionmaker(engine, class_=AsyncSession, expire_on_commit=False) return session_factory, engine async def cleanup_engine(engine): """Properly dispose of the async engine.""" await engine.dispose() ``` - [ ] **Step 2: Update downloads.py to import from db.py** At the top of `backend/app/tasks/downloads.py`, remove the local definitions of `get_async_session` and `_cleanup_engine` (the two functions defined around lines 29-42). Add this import: ```python from app.tasks.db import get_async_session, cleanup_engine as _cleanup_engine ``` - [ ] **Step 3: Update maintenance.py to import from db.py** At the top of `backend/app/tasks/maintenance.py`, remove the local definitions of `get_async_session` and `_cleanup_engine` (around lines 32-41). Add this import: ```python from app.tasks.db import get_async_session, cleanup_engine as _cleanup_engine ``` - [ ] **Step 4: Verify no duplicate definitions remain** ```bash grep -n "def get_async_session\|def _cleanup_engine\|def cleanup_engine" \ backend/app/tasks/downloads.py \ backend/app/tasks/maintenance.py \ backend/app/tasks/db.py # Expected: only db.py shows the definitions ``` - [ ] **Step 5: Commit** ```bash git add backend/app/tasks/db.py backend/app/tasks/downloads.py backend/app/tasks/maintenance.py git commit -m "refactor: extract shared DB helpers to tasks/db.py" ``` --- ## Task 12: Parallelize checkAllSources in Dashboard **Files:** - Modify: `frontend/src/views/Dashboard.vue` - [ ] **Step 1: Locate checkAllSources** Find the `async function checkAllSources()` (around line 460). Current implementation uses a `for...of` loop with `await` inside. - [ ] **Step 2: Replace with Promise.allSettled** Replace the body of `checkAllSources` with: ```javascript async function checkAllSources() { checkingAll.value = true try { const enabledSources = sourcesStore.sources.filter(s => s.enabled) const results = await Promise.allSettled( enabledSources.map(source => sourcesStore.triggerCheck(source.id)) ) const queued = results.filter(r => r.status === 'fulfilled').length const failed = results.filter(r => r.status === 'rejected').length if (failed > 0) { notifications.success(`Queued ${queued} sources (${failed} failed to queue)`) } else { notifications.success(`Queued checks for ${queued} sources`) } await fetchActiveDownloads() await downloadsStore.fetchStats() } catch (error) { notifications.error(`Failed to check sources: ${error.message}`) } finally { checkingAll.value = false } } ``` - [ ] **Step 3: Commit** ```bash git add frontend/src/views/Dashboard.vue git commit -m "perf: parallelize checkAllSources with Promise.allSettled" ``` --- ## Final Verification - [ ] **Start the stack and verify no startup errors** ```bash docker compose up -d --build docker compose logs -f app scheduler worker ``` - [ ] **Run the migration if not already done** ```bash docker compose exec app alembic upgrade head ``` - [ ] **Smoke test the dashboard** 1. Open the app, confirm the dashboard loads without errors 2. Confirm "Sources Needing Attention" shows only active (unsuperseded) failures 3. Trigger a retry on a failed download — confirm it actually runs (no longer silently skips) 4. After a successful download completes, confirm prior failures for that source disappear from dashboard - [ ] **Verify the export-failed-logs endpoint returns `superseded` field** ```bash curl -s "http://localhost:8080/api/downloads?status=failed&per_page=1" | python3 -m json.tool | grep superseded # Expected: "superseded": false ```