Skip to content

Fix failed downloads being marked as downloaded, add rip repair - #1023

Open
sjbrownrigg wants to merge 4 commits into
nathom:devfrom
sjbrownrigg:fix/failed-downloads-marked-complete
Open

Fix failed downloads being marked as downloaded, add rip repair#1023
sjbrownrigg wants to merge 4 commits into
nathom:devfrom
sjbrownrigg:fix/failed-downloads-marked-complete

Conversation

@sjbrownrigg

@sjbrownrigg sjbrownrigg commented Aug 13, 2026

Copy link
Copy Markdown

The problem

When a track fails to download twice, it gets logged to the failed-downloads database — and then marked as successfully downloaded.

Track.download() retried once and, on a second failure, called db.set_failed(...) but then returned normally. Media.rip() runs preprocess → download → postprocess unconditionally, so postprocess() went on to call db.set_downloaded(...). From that point on, PendingTrack.resolve() / PendingSingle.resolve() see the track in the downloads database and skip it, permanently. Re-running the same URL doesn't help, and it survives restarts. The only recovery is to find the missing tracks by hand.

I hit this on a real library. Of 629 rows in downloads.db, 18 were also present in failed_downloads.db — every single failed track had been recorded as downloaded.

The failures also leave truncated files behind. Because downloads always restart from byte 0 (BasicDownloadable opens the path "wb" and sends no Range header), a partial is never resumed — it just stays in the library as a file with correct tags and a plausible size that fails to decode partway through. I found 8 of these, silently unplayable past the truncation point.

There's a second, quieter version of the same bug: PendingTrack.resolve() has four failure paths and only one of them recorded anything. If the metadata request failed, if building metadata raised, or if fetching the downloadable URL failed, it logged a line and returned None — no file, no downloads row, nothing in the failed database. The track just went missing from the album with no trace anywhere. PendingSingle.resolve() had the same gap on three paths.

Finally, config.toml has always said "Then, rip repair can be called to retry the downloads", but no such command existed. The failed-downloads database was write-only — nothing ever read it back.

What this changes

Don't mark failed downloads as downloaded — raise TrackDownloadFailedError on persistent failure so Media.rip() stops before postprocess() and set_downloaded() is never reached for a track that failed. Also delete the partial file, since it can never be resumed. Album.download() and Playlist.download() already wrap each track.rip() in its own try/except and continue, so raising cannot abort a whole album or playlist.

Record tracks that fail while resolving — call set_failed() on the resolve paths that previously returned None silently, so those failures are retryable instead of invisible.

Add rip repair to retry failed downloads — implements the command config.toml already documents. It reads the failed database, retries each item through the normal pipeline, and clears the ones that succeed.

Two details worth flagging for review:

  • Success is determined by membership in the downloads database, not by diffing the failed database before/after. Nothing in the pipeline removes rows from the failed database, so a before/after diff always reports zero repaired. Checking the downloads database is sound precisely because of the first commit: set_downloaded() is now only reachable from postprocess(), which a failed track never gets to.
  • Repaired items are placed in their album's folder, because a failed item is usually one track missing from an otherwise complete album, and retrying resolves it as a single. --flat restores the old root-folder behaviour.

Interaction with #1009: that PR adds proper Range-based resumable downloads. If it lands, deleting the partial file here becomes actively wrong — it would throw away exactly the bytes #1009 wants to resume from. The deletion is only correct while downloads restart from byte 0, which is true on dev today. Happy to make the deletion conditional, drop it, or rebase on #1009, whichever the maintainers prefer. The rest of this PR is independent of that question.

Retrying through the album

Repair originally re-added each failed track by id, which resolves as a single. That builds album metadata from the track response — and on Tidal a track response carries almost nothing about its album:

album object keys: ['cover', 'id', 'title', 'vibrantColor', 'videoCover']

No track count, so tracktotal falls back to 1, and the artist is the track's artists rather than the album artist. Where folder_format uses {albumartist} or {tracktotal}, repaired tracks therefore land somewhere other than the album they belong to:

Simon Carter/Beautiful Destruction (2021) [MP4] ... [11 tracks]      the album
Simon Carter, Fabsi/Beautiful Destruction (2021) [FLAC] ... [1 tracks]

Running it against a real library produced 18 such folders holding 107 files.

It now looks up each failed track's album and retries through that. The metadata is then the album's own, so the folder, disc subfolders and cover art are all correct, and nothing extra is downloaded — tracks already in the downloads database are skipped, so only the missing ones are fetched. Albums are de-duplicated, so several failures from one album cost one retry rather than several. Anything whose album cannot be determined is still retried on its own.

Testing

  • Drove a real Track.rip() against a downloadable that writes a partial file then raises, with temporary databases: confirmed it raises, logs to the failed database, does not mark downloaded, removes the partial, and leaves no stale progress-bar title.
  • Ran rip repair against a real Qobuz account and recovered all 18 stuck tracks. Roughly half the large files hit genuine IncompleteRead errors along the way, which made a good test: on every pass the downloaded and failed databases stayed disjoint. Under the old code those would have become permanently lost tracks.
  • Every commit compiles standalone.

Happy to split this further or adjust anything.

sjbrownrigg and others added 3 commits August 13, 2026 11:59
Track.download() retried once and, on a second failure, logged the track
to the failed-downloads database but then returned normally. Media.rip()
runs preprocess -> download -> postprocess unconditionally, so
postprocess() went on to call db.set_downloaded(), permanently marking a
track that had never downloaded. Every later run's resolve() then saw it
in the downloads database and skipped it, so the track could never be
recovered -- including across restarts.

Raise TrackDownloadFailedError instead, so Media.rip() stops before
postprocess() and set_downloaded() is never reached for a track that
actually failed.

Also delete the partial file. Downloads always restart from byte 0
(BasicDownloadable opens the path "wb" and sends no Range header), so a
partial is never resumed -- it just sits in the library as a truncated
file that has valid tags and the right size, plays in any client, and
fails to decode partway through.

Album.download() and Playlist.download() already wrap each track.rip()
in its own try/except and continue, so raising cannot abort a whole
album or playlist.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 78d76be)
PendingTrack.resolve() had four failure paths and only one of them
recorded anything. If the metadata request failed, if building the
metadata raised, or if fetching the downloadable URL failed, it logged a
line and returned None. Nothing else happened: no file was written, no
downloads-database row was added, and nothing went into the failed
database. The track simply went missing from the album, and because it
was never recorded as failed, `rip repair` could not find it either.

PendingSingle.resolve() had the same gap on three of its paths.

Call db.set_failed() on all of them so a resolve-time failure is
retryable rather than invisible.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit bcebf57)
The failed-downloads database was write-only: entries went in and
nothing ever read them back. config.toml has always told users that
"`rip repair` can be called to retry the downloads", but no such command
existed.

Add it. It reads the failed database, retries each item through the
normal resolve/rip pipeline, and clears the ones that succeed, leaving
the rest logged for a future run.

Success is determined by membership in the downloads database rather
than by diffing the failed database before and after, because nothing in
the pipeline removes rows from the failed database -- a before/after
diff would always report zero repaired. Checking the downloads database
is sound because set_downloaded() is only reached from postprocess(),
which a failed track no longer gets to.

Failed items are cleared from the downloads database before retrying, so
a repair still works on a database left inconsistent by an older
version.

Repaired tracks are placed in their album's folder, since a failed item
is usually one track missing from an otherwise complete album, and
retrying resolves it as a single. --flat restores the old behaviour of
downloading it to the root.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 6c54ab0)
berettavexee pushed a commit to berettavexee/streamrip that referenced this pull request Aug 18, 2026
Cherry-picks the parts of upstream nathom#1023 the fork still
needs. The core of that PR -- a failed download recorded as downloaded --
is already fixed here by a different route: download() raises after two
attempts, so postprocess() and its set_downloaded() are never reached.
The PR's download() rework is therefore dropped; the fork's version
retries and runs an integrity check on top.

What was missing:

- resolve() logged six failure paths without recording them. A track
  dying there produces no file and no downloads row either, so it left no
  trace at all: it silently went missing from its album with nothing for
  a retry to find. Both PendingTrack and PendingSingle now call
  set_failed() on every exception path.
- a failed download left its truncated file at the final path, in the
  library among the good ones. It is now removed; a cleanup error is
  logged rather than masking the download error being raised.
- a single whose download failed kept its title in the progress header
  for the rest of the run, since postprocess() never ran to clear it.

`rip repair` retries everything in the failed-downloads database and
clears what succeeds, relying on the invariant above to tell success from
failure. Repaired tracks land in their album folder by default (--flat
opts out), since a repaired track is nearly always one missing from an
otherwise complete album. Stale downloads rows are cleared first, or
resolve() would skip the retry as already-downloaded.

db.remove() carried a "NOT TESTED" warning and repair depends on it, so
it now has coverage on the Failed table, including the no-op case.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Repair re-added each failed track by id, which resolves as a single.
That builds album metadata from the track response -- and on Tidal a
track response carries only the album's id, title and cover:

    album object keys: ['cover', 'id', 'title', 'vibrantColor', 'videoCover']

No track count, so tracktotal falls back to 1, and the artist is the
track's artists rather than the album artist. Where folder_format uses
{albumartist} or {tracktotal}, repaired tracks therefore land in a
different folder from the album they belong to. One repair of a real
library produced 18 such folders holding 107 files.

Look up each failed track's album and retry through that instead. The
metadata is then the album's own, so the folder, disc subfolders and
cover art are right, and nothing extra is downloaded: tracks already in
the downloads database are skipped. Albums are de-duplicated, so several
failures from one album cost one retry rather than several.

Anything whose album cannot be determined is still retried on its own.
berettavexee pushed a commit to berettavexee/streamrip that referenced this pull request Aug 20, 2026
types-click and types-Pillow were stubs for a type checker this project does
not have — no mypy dependency, no typing step in CI — and both sat on
obsolete majors, click 7 stubs against the click 8 actually used.

vulture had the opposite problem: its configuration was committed
([tool.vulture] plus a curated whitelist) but the tool itself was never
declared, so poetry install did not provide it and it only lived in whichever
virtualenv someone had pip-installed it into. Predictably the whitelist had
drifted — _album_cache and _album_tasks were dropped when _TaskCache was
encapsulated, while _TaskCache.has_pending and the repair command added by
nathom#1023 were missing from it. Declare vulture, resync the whitelist, and add a
Dead code workflow running the same `poetry run vulture` as locally.

Move the dev dependencies to [tool.poetry.group.dev.dependencies] while here:
the old form is deprecated in Poetry 2.x and warns on every invocation, and
the group name is what lets the new workflow install dev deps alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.

1 participant