Skip to content

Add keep_max_files_sort_by option for playlist-index-based retention - #1484

Merged
jmbannon merged 8 commits into
jmbannon:masterfrom
michaeldyrynda:feat/keep-max-files-sort-by
Jul 16, 2026
Merged

Add keep_max_files_sort_by option for playlist-index-based retention#1484
jmbannon merged 8 commits into
jmbannon:masterfrom
michaeldyrynda:feat/keep-max-files-sort-by

Conversation

@michaeldyrynda

@michaeldyrynda michaeldyrynda commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Adds a keep_max_files_sort_by config option that controls how entries are ordered when pruning with keep_max_files.

Previously, pruning always sorted by upload_date (keeping the most recent). This adds support for sorting by playlist_index (keeping the lowest indices), which is useful for playlists where position matters more than upload date e.g. keeping the first N episodes of a series.

Changes

  • keep_max_files_sort_by - new output_options field accepting "upload_date" (default, preserving existing behaviour) or "playlist_index"
  • playlist_index on DownloadMapping - persisted in the archive JSON so it's available at prune time; old archives without the field gracefully default to None
  • Fallback behaviour - if sort_by is "playlist_index" but no entries have an index (e.g. old archives), logs a warning and falls back to upload_date (current behaviour)
  • Prebuilt preset support - only_recent_sort_by override wired into the download_deletion_options helper

Test plan

  • Unit tests cover playlist-index sort, upload-date sort, None-first pruning, all-None fallback, zero-max no-op, old archive backward compat
  • Manual integration test with a real playlist subscription using keep_max_files_sort_by: "playlist_index"

Closes #1461

…es_sort_by config option

- Add playlist_index field to DownloadMapping dataclass with backward-compatible deserialisation
- Create KeepMaxFilesSortByValidator accepting "upload_date" or "playlist_index"
- Register keep_max_files_sort_by in OutputOptions with default "upload_date"
- Add unit tests for serialisation, deserialisation, and validator behaviour
…move_stale_files

- Add sort_by parameter to remove_stale_files() with playlist_index ascending sort
- None playlist indices sort last (pruned first) when mixed with populated values
- Fall back to upload_date with warning when all entries have None playlist index
- Wire sort_by resolution from OutputOptions through subscription_download
- Add only_recent_sort_by override to "Only Recent" prebuilt preset
- Verify default sort_by="upload_date" keeps most recent N entries by date descending
- Verify old archives without playlist_index sort identically to upload_date mode
- Verify keep_max_files=0 skips pruning entirely
@jmbannon

jmbannon commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Most of the failing tests are due to hash mismatch of the download archive file. We will need to update all of them via setting the REGENERATE_FIXTURES global variable to True, rerunning, and committing.

I can help with this and the integration test if you'd like.

@jmbannon

jmbannon commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Also, I feel we should make the order support both playlist_index ascending and descending. Right now from the doc:

When set to playlist_index, entries with the lowest playlist indices are kept

It seems its playlist_index ASC. I see both cases where authors add new vids to playlists at the front or back. We should support the DESC case too and make it explicit in the name.

…t orders

- Rename 'playlist_index' to 'playlist_index_asc' and add 'playlist_index_desc'
  for explicit ascending/descending playlist index sorting
- Regenerate test fixtures to include new keep_max_files_sort_by field
- Regenerate plugins.rst from docstrings
- Add descending sort tests and update existing tests
@michaeldyrynda

Copy link
Copy Markdown
Contributor Author

Thanks @jmbannon - regenerated the hashes and added support for both directions for playlist_index sorting.

ffmpeg on Windows does not produce bit-identical output even with
-bitexact, causing unchanged media files to appear as 'modified'
in the transaction log during reformat steps. This matches the
existing pattern in expected_download.py which skips all MD5 hash
checks on Windows.

Also improves the assertion error message to show actual vs expected
line counts instead of just 'True'.
@michaeldyrynda

Copy link
Copy Markdown
Contributor Author

sorry about that @jmbannon - broke it. again in a similar but different way.

have tested it out in my fork and made sure all tests are passing this time - https://github.com/michaeldyrynda/ytdl-sub/pull/1/checks

i also added a fix for the windows failures in ecba1d3 - same fix as is used in expected_download.py where the tests are skipped on Windows.

looks like ffmpeg isn't deterministic on Windows, which causes the ordering to change. if you'd prefer those failing tests stay in place, let me know and i'll drop the commit.

@jmbannon
jmbannon merged commit 3379235 into jmbannon:master Jul 16, 2026
32 checks passed
@jmbannon

Copy link
Copy Markdown
Owner

Ran a few of the disabled e2e tests manually, notably test_full_channel_existing_archive_keep_max_files, download archive and everything else looked good. Thanks again @michaeldyrynda , especially the CI issue. Shame about the Windows tests but honestly not surprised non-determinism slipped in there.

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.

Incorrect handling of archive persistence with only recent

2 participants