Fix episodes queued in Up Next being auto-archived by per-podcast episode limit

[joashrajin]

Description

checkPodcastForEpisodeLimitBlocking filtered out only the currently-playing episode (playbackManager.getCurrentEpisode(), which is the top of Up Next). Any episode queued behind the current one in Up Next could still be archived when a newer episode arrived for the same podcast and tripped the per-podcast limit during a background refresh.

User-visible effect: when a user has a podcast set to Auto add to Up Next + a download/episode limit of 1, an episode queued for later is silently marked as played the next time a new episode arrives via background refresh. The Zendesk reporter perceived this as "the episode I was listening to" being marked played mid-listening.

Fix: replace the getCurrentEpisode().uuid != it.uuid filter with !playbackManager.upNextQueue.contains(it.uuid). The full-queue check is already the pattern used by episodeCanBeCleanedUp in the same file (line 886), so this aligns the over-limit path with the existing precedent. The new filter is null-safe to preserve the existing test-only playbackManager = null behaviour.

Added an androidTest regression that mirrors the bug: episode limit = 1, two episodes for the same podcast, the older one queued behind a newer one in Up Next. The legacy filter archives the older episode; with this change it stays.

Fixes PCDROID-571. User-reported via Zendesk #11253136, with the customer's debug log showing the exact Auto archiving episode over limit 1 … → Download deleted … Source: UNKNOWN sequence inside a background RefreshPodcastsTask cycle.

Testing Instructions

1. Subscribe to a podcast that publishes regularly (e.g. Marketplace Morning Report).
2. In that podcast's settings, enable Auto Download and Auto add to Up Next.
3. Set the podcast's Episode Limit / Auto Download limit to 1.
4. Trigger a refresh (pull-to-refresh on the Podcasts tab) so a downloaded episode lands in Up Next.
5. Reorder Up Next so the downloaded episode is not at the top (queue an unrelated episode in front of it).
6. Wait for or force another refresh that brings a newer episode of the same podcast.
7. ✅ Verify the older episode that was queued in Up Next is still present (not archived, not marked played).

Or, run the new androidTest:

./gradlew :app:connectedAndroidTest --tests "au.com.shiftyjelly.pocketcasts.models.db.AutoArchiveTest.testEpisodeLimitRespectsUpNext"

Checklist

• If this is a user-facing change, I have added an entry in CHANGELOG.md
• Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
• I have considered whether it makes sense to add tests for my changes
• All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
• Any jetpack compose components I added or changed are covered by compose previews
• I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

[Copilot]

Pull request overview

Fixes an auto-archive edge case where episodes queued in Up Next (but not currently playing) could be archived when a per-podcast episode limit is exceeded during background refresh, causing queued episodes to disappear unexpectedly.

Changes:

• Update per-podcast episode-limit auto-archive logic to exclude any episode currently present in the Up Next queue (not just the current episode).
• Add an androidTest regression test to ensure queued (non-current) Up Next episodes are not archived under the per-podcast limit.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/podcast/EpisodeManagerImpl.kt	Extends the episode-limit exclusion check to cover the full Up Next queue to prevent archiving queued episodes.
app/src/androidTest/java/au/com/shiftyjelly/pocketcasts/models/db/AutoArchiveTest.kt	Adds a regression test reproducing the queued-episode auto-archive scenario under a per-podcast limit of 1.

[joashrajin]

Good catch, addressed in 549b0c7 — dropped the runBlocking wrappers and call playLast directly from the runTest coroutine.

[joashrajin]

Update: had to revert this in 2cd7c78 — the instrumented-tests CI job failed after dropping the wrappers. upNext.playLast does withContext(Dispatchers.Default) internally, and the runTest scope here uses an UnconfinedTestDispatcher only as the ioDispatcher — it doesn't control Dispatchers.Default, so the queue can be observed not-yet-populated when the archive check runs immediately after. Matching the existing testAddingInactiveEpisodeToUpNext pattern with runBlocking keeps the ordering reliable.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[claude]

Claude finished @joashrajin's task in 1m 41s —— View job

---

PR Review: Fix Up Next episodes being auto-archived by per-podcast limit

• Read the changed files and surrounding context
• Evaluate correctness, performance, and the open review threads
• Post review feedback

---

Thanks for the fix and the detailed writeup — the root cause analysis is spot on. The bug was real: the old getCurrentEpisode().uuid != it.uuid guard only protected the top of Up Next, leaving queued-behind episodes vulnerable to the over-limit sweep. I reviewed the current state of the branch (not just the original diff), and both earlier Copilot threads are already addressed:

• runBlocking in the test — removed; the two assertEquals calls on upNext.currentEpisode/upNext.queueEpisodes (lines 539-544) now act as a synchronization barrier that forces the queue to be populated (blockingFirst()) before the archive check runs, so the ordering is reliable without the wrappers. Nicer than the revert.
• Per-candidate contains() cost — resolved; EpisodeManagerImpl.kt:858-862 now snapshots the Up Next UUIDs into a Set<String> once and does it.uuid !in upNextEpisodeUuids set-membership per candidate.

Correctness ✅

• UpNextQueue.allEpisodes = currentEpisode + queueEpisodes (UpNextQueue.kt:33), so the new filter protects both the currently-playing episode and everything queued behind it. This is a strict superset of the old behaviour — no regression for the currently-playing case, and it matches the !upNextQueue.contains(...) precedent used by episodeCanBeCleanedUp/userHasInteractedWithEpisode (lines 886/891).
• Null-safety (playbackManager?...orEmpty()) preserves the test-only playbackManager = null path — nothing is protected, which is the pre-existing behaviour. ✅
• The set is built lazily inside the episodeLimit < allEpisodes.size guard, so it's only computed when there's actually over-limit work to do. ✅
• The regression test is well-constructed — the comment at lines 533-535 correctly notes that placing the older episode behind the current one is what distinguishes this from the legacy guard (otherwise the test would pass even with the buggy filter).

Minor observations (non-blocking)

1. CHANGELOG — the checklist item is unchecked and the 8.13 section in CHANGELOG.md is currently empty. Since this is a user-facing behaviour fix, it's worth adding an entry, e.g.:

   8.13
   -----
   *   Bug Fixes
       *   Don't auto-archive episodes queued in Up Next when a podcast is over its episode limit
           ([#5342](https://github.com/Automattic/pocket-casts-android/pull/5342))
   

   Add CHANGELOG entry →

2. Naming (optional) — the local val allEpisodes (DAO result, EpisodeManagerImpl.kt:856) shadows the conceptual name of upNextQueue.allEpisodes. No actual bug since the queue access is qualified, but a name like podcastEpisodes would read a touch clearer. Purely cosmetic.

3. Spotless — the formatting checkbox is unchecked; just make sure ./gradlew spotlessCheck is green before merge (I can't run it here).

Verdict

The change is correct, minimal, well-tested, and follows an existing pattern in the same file. The only thing I'd genuinely suggest before merge is the CHANGELOG entry. Nice work. 👍

Note: I reviewed against the current branch HEAD. I wasn't able to run the Gradle build/tests in this environment, so I did not execute spotlessCheck or the instrumented test.

[claude]

Claude finished @joashrajin's task in 47s —— View job

---

Re-review: Fix Up Next episodes being auto-archived by per-podcast limit

• Read the changed files at current branch HEAD
• Re-check both Copilot threads + my earlier CHANGELOG note
• Evaluate correctness, performance, security
• Post feedback

---

I re-reviewed the branch after the latest push (138f382, 1e67f18, and the merge from main). Everything I flagged last time is now resolved, and the change is in good shape.

What's resolved since the last round

• CHANGELOG ✅ — added under 8.13 Bug Fixes with the correct PR link (CHANGELOG.md).
• Per-candidate contains() cost ✅ — EpisodeManagerImpl.kt:867-871 now snapshots Up Next UUIDs into a Set<String> once, then does it.uuid !in upNextEpisodeUuids per candidate. Set-membership instead of repeated blockingFirst() reads.
• runBlocking in the test — kept intentionally, with 2cd7c78 reverting the earlier removal. The rationale in the thread is correct: UpNextQueueImpl.playLast hops to Dispatchers.Default via withContext, which the runTest scope's UnconfinedTestDispatcher (wired only as ioDispatcher) does not control, so without the barrier the queue can be observed unpopulated. The two assertEquals on currentEpisode/queueEpisodes (test lines 543-548) also force blockingFirst() before the archive check. Reliable ordering. ✅
• Test-only mock gap — the 138f382 addition stubbing trackingAutoPlaySource/lastAutoPlaySource (AutoArchiveTest.kt:108-110) is needed because playLast reads those settings when the queue already has a current episode. Good catch to add them.

Correctness ✅

• UpNextQueue.allEpisodes = currentEpisode + queueEpisodes, so the new filter protects the currently-playing episode and everything queued behind it — a strict superset of the old getCurrentEpisode().uuid != it.uuid guard, so no regression for the current-episode case. It also aligns with the !upNextQueue.contains(...) precedent in userHasInteractedWithEpisode/episodeCanBeCleanedUp (lines 895/899).
• Null-safety (playbackManager?...orEmpty()) preserves the test-only playbackManager = null path. ✅
• The set is built lazily inside the episodeLimit < allEpisodes.size guard, so it's only computed when there's actual over-limit work. ✅

Security / performance

• No security surface — pure local archive-filtering logic, no new I/O, network, or user input.
• Performance is now better than the original approach (single snapshot vs. per-candidate observable reads).

Minor (non-blocking, unchanged from last time)

• Naming — the local val allEpisodes (DAO result, line 865) shares a name with the conceptual upNextQueue.allEpisodes. No bug since the queue access is qualified; podcastEpisodes would read a touch clearer. Purely cosmetic.
• Spotless — I can't run Gradle in this environment, so please confirm ./gradlew spotlessCheck is green before merge.

Verdict

Correct, minimal, well-tested, and consistent with existing patterns in the file. All review feedback addressed. LGTM — nothing blocking from me. 👍

Note: I reviewed against the current branch HEAD and could not run the Gradle build/tests here, so I did not execute spotlessCheck or the instrumented test.

[claude]

Claude finished @joashrajin's task in 45s —— View job

---

Re-review: Fix Up Next episodes being auto-archived by per-podcast limit

• Read the changed files at current branch HEAD
• Re-check prior review threads (Copilot + earlier CHANGELOG note)
• Evaluate correctness, performance, security
• Post feedback

---

I re-reviewed after the latest push. Since my last pass the only change is 5d4686c, which moves the CHANGELOG entry from 8.13 to the 8.18 section to match the PR milestone. The fix itself is unchanged and remains in good shape.

Correctness ✅

• EpisodeManagerImpl.kt:867-875 — the over-limit filter now excludes anything in upNextQueue.allEpisodes (= currentEpisode + queueEpisodes), which is a strict superset of the old getCurrentEpisode().uuid != it.uuid guard. This protects both the currently-playing episode and everything queued behind it, directly fixing PCDROID-571 with no regression for the current-episode case.
• Aligns with the existing !upNextQueue.contains(...) precedent in userHasInteractedWithEpisode/episodeCanBeCleanedUp (lines 895/899). ✅
• Null-safety: playbackManager?...orEmpty() yields an empty set when playbackManager == null, preserving the pre-existing test-only path (nothing protected). ✅
• The UUID set is built lazily inside the episodeLimit < allEpisodes.size guard, so it's only computed when over-limit work actually exists. ✅

Performance ✅

• Single snapshot of Up Next UUIDs into a Set<String> (via mapTo(mutableSetOf())), then O(1) it.uuid !in upNextEpisodeUuids per candidate — no repeated blockingFirst() observable reads. Better than the original per-candidate contains() approach and than the old baseline.

Security ✅

• No new surface: pure local archive-filtering logic, no network/IO/user input.

Test ✅

• AutoArchiveTest.testEpisodeLimitRespectsUpNext faithfully reproduces the bug: limit = 1, older episode queued behind the newer current one. The comment (lines ~528-530) correctly notes that this ordering is what distinguishes the fix from the legacy guard — with the old filter the older episode would be archived. The runBlocking in playLast and the two assertEquals barriers on currentEpisode/queueEpisodes are intentional and correctly explained in the thread (the runTest scope's UnconfinedTestDispatcher only backs ioDispatcher, not the Dispatchers.Default that playLast hops to). ✅

Minor (non-blocking, unchanged)

• Naming — the local val allEpisodes (line 865, DAO result) shares its name with the conceptual upNextQueue.allEpisodes. No bug since the queue access is qualified; podcastEpisodes would read slightly clearer. Purely cosmetic.
• Spotless — I can't run Gradle in this environment, so please confirm ./gradlew spotlessCheck is green before merge.

Verdict

Correct, minimal, well-tested, and consistent with existing patterns in the file. All prior review feedback (both Copilot threads + CHANGELOG) is resolved. LGTM — nothing blocking from me. 👍

Note: I reviewed against the current branch HEAD and could not run the Gradle build/tests here, so I did not execute spotlessCheck or the instrumented test.

[joashrajin]

✅ Verified on-device

Ran the connected instrumented test on a physical device (not just the Buildkite emulator):

• Device: Samsung Galaxy S21 FE (SM-G990E), Android 16
• Command: ./gradlew :app:connectedDebugAndroidTest -Pandroid.testInstrumentationRunnerArguments.class=…AutoArchiveTest
• Result: 18 tests, 0 failures — BUILD SUCCESSFUL
• testEpisodeLimitRespectsUpNext (the PCDROID-571 regression) passed in 1.05s

So the older episode queued behind the currently-playing one survives the over-limit archive sweep on real hardware, and none of the other archive paths (inactive / played / starred / episode-limit) regressed.

[wpmobilebot]

Version 8.18 has now entered code-freeze, so the milestone of this PR has been updated to 8.19.