From 0698ee1fa7183117c9b238a4a35526596dd75c6c Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 18 Mar 2026 23:14:19 -0400 Subject: [PATCH] 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 --- .../2026-03-18-comprehensive-review-design.md | 263 ++++++++++++++++++ 1 file changed, 263 insertions(+) create mode 100644 docs/superpowers/specs/2026-03-18-comprehensive-review-design.md diff --git a/docs/superpowers/specs/2026-03-18-comprehensive-review-design.md b/docs/superpowers/specs/2026-03-18-comprehensive-review-design.md new file mode 100644 index 0000000..beac118 --- /dev/null +++ b/docs/superpowers/specs/2026-03-18-comprehensive-review-design.md @@ -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)