Add comprehensive review implementation plan
12 tasks covering all spec items: 4 bug fixes, superseded column migration, error classification refactor, and code quality cleanup across backend and frontend. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,862 @@
|
||||
# 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
|
||||
```
|
||||
Reference in New Issue
Block a user