Skip to content

fix(state): cap retry attempts for pending downloads - #49

Open
rfsbraz wants to merge 1 commit into
mainfrom
fix/pending-retry-accounting
Open

fix(state): cap retry attempts for pending downloads#49
rfsbraz wants to merge 1 commit into
mainfrom
fix/pending-retry-accounting

Conversation

@rfsbraz

@rfsbraz rfsbraz commented Jun 12, 2026

Copy link
Copy Markdown
Owner

Summary

  • Fix an infinite loop in run_check: failed downloads stayed in the pending queue, and the download loop immediately pulled the exact same IDs back out with get_oldest. A single permanently failing message (deleted media, persistent server error) made the check iteration spin forever, hammering Telegram through the full retry/backoff budget on every pass and never letting the check complete.
  • Add an attempts column to pending_downloads with automatic schema migration for existing databases (ALTER TABLE, no manual steps).
  • Failed message IDs now get their attempt counter incremented after each batch; get_oldest and count exclude entries that reached the limit, so each failing message is retried a bounded number of times and then dead-lettered.
  • Dead-lettered entries stay in the table for inspection and are reported with an error log line per run.
  • New max_download_attempts config option (default 5, each attempt still carries the existing per-download retry budget).
  • Unit tests cover attempt counting, exclusion at the limit, per-source isolation, and legacy database migration.

Copilot AI review requested due to automatic review settings June 12, 2026 21:24
@github-actions

Copy link
Copy Markdown
Contributor

Testing this PR

Option 1 — Docker Compose override:

Create a docker-compose.override.yml:

services:
  telegram-downloader:
    build: https://github.com/rfsbraz/telegram-downloader.git#refs/pull/49/head

Then run:

docker compose up --build

Option 2 — Direct build and run:

docker build https://github.com/rfsbraz/telegram-downloader.git#refs/pull/49/head -t telegram-downloader:pr-49
docker run --rm -it telegram-downloader:pr-49

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds bounded retry/dead-letter behavior for the persistent pending download queue so a permanently failing message ID can’t cause run_check to spin indefinitely.

Changes:

  • Add an attempts column (with auto-migration) to pending_downloads, plus APIs to increment attempts and count exhausted entries.
  • Update run_check to exclude exhausted entries from scheduling and to increment attempts for failed downloads.
  • Introduce max_download_attempts configuration (default 5) and add unit tests for attempt accounting and legacy DB migration.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/state/pending.py Adds attempts column + migration, filters get_oldest/count by attempt budget, and introduces increment_attempts/count_exhausted.
src/main.py Uses attempt budget when pulling pending batches, increments attempts for failed IDs, and reports exhausted entries.
src/config/schema.py Adds max_download_attempts config option with validation.
tests/unit/state/test_pending.py Adds unit tests covering attempt counting, exclusion at limit, isolation, and legacy schema migration.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/state/pending.py
Comment on lines +65 to +69
self._connection.execute(
"ALTER TABLE pending_downloads ADD COLUMN attempts INTEGER NOT NULL DEFAULT 0"
)
except sqlite3.OperationalError:
pass # Column already exists
Comment thread src/main.py
Comment on lines +378 to +381
log.warning(
f"{len(failed_ids)} download(s) failed, will retry "
f"(up to {cfg.max_download_attempts} attempts per message)"
)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants