fix: Respect the data warning when caching the entire playing episode

[joashrajin]

Description

When a non-downloaded episode starts playing (and cacheEntirePlayingEpisode is enabled — its Firebase Remote Config default is true), ExoPlayerDataSourceFactory enqueues CacheWorker.startCachingEntireEpisode, which downloads the entire episode in the background with NetworkType.CONNECTED hardcoded. On a metered connection this transfers the full episode (hundreds of MB for long shows) over mobile data regardless of the user's Warn before using data setting — and the 416-error recovery path (SimplePlayer.onPlayerError → resetEpisodeCaching) wipes the cache and downloads the whole episode again, multiplying usage. The playback path itself does not write to the cache (setCacheWriteDataSinkFactory(null)), so this is an additional transfer on top of what the user actually streams.

buildPrefetchRequest already handles this correctly by picking UNMETERED when warnOnMeteredNetwork is on; CacheWorker never got the same treatment.

Changes:

• CacheWorker.startCachingEntireEpisode now takes a networkConstraint: NetworkType parameter (mirroring PrefetchWorker.prefetchNextEpisode) instead of hardcoding NetworkType.CONNECTED.
• ExoPlayerDataSourceFactory.startCachingEntireEpisodeIfNeeded passes UNMETERED when settings.warnOnMeteredNetwork.value is true, else CONNECTED — the same rule as buildPrefetchRequest. The resetEpisodeCaching path inherits the fix via the shared helper.

Net behavior: on WiFi nothing changes; on metered networks the full-episode cache job waits for an unmetered network instead of silently consuming mobile data. Streaming playback is unaffected (the stream is a separate data path).

Addresses PCDROID-442 — likely explains the reports where affected users (ZD 11401496, 11307725, 11383273) never manually manipulate Up Next: any episode change during playback (including auto-advance) creates a new media source and enqueues a full-episode CacheWorker job with the unconstrained network type. Not using a Fixes keyword because the DownloadEpisodeWorker (UserTriggered(waitForWifi = false)) angle in the original report may be a separate contributor and needs its own verification.

Testing Instructions

Preconditions: Settings → Storage & data use → Warn before using data = ON, episode caching enabled (Settings → Advanced → cache entire playing episode), a podcast with non-downloaded episodes.

• On WiFi, play a non-downloaded episode
• Verify the log shows "Caching complete for episode id: …" (unchanged behavior)
• Switch to mobile data (WiFi off), play another non-downloaded episode
• Verify playback streams normally but NO full-episode caching starts; adb shell dumpsys jobscheduler | grep -A5 pocket_casts_cache_worker (or WorkManager inspection) shows the job constrained to UNMETERED and deferred
• Verify mobile data usage for the session is roughly the streamed portion only, not the full episode size (compare in Android Settings → Apps → Pocket Casts → Data usage)
• Turn Warn before using data OFF, play a non-downloaded episode on mobile data
• Verify full-episode caching runs over mobile data (opt-out behavior preserved)

⚠️ TODO (manual): On-device verification of the 416-reset path (force a 416 by seeking in a partially cached episode on mobile data) — confirm the re-cache also waits for WiFi.

Screenshots or Screencast

⚠️ TODO (manual): n/a expected — no UI changes; add a WorkManager inspector screenshot showing the UNMETERED constraint if reviewers want proof.

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) (ran spotlessCheck — passed)
• I have considered whether it makes sense to add tests for my changes (no unit test added: CacheWorker/ExoPlayerDataSourceFactory have no existing test harness and the change is WorkManager constraint plumbing; covered by the manual steps above — same situation as fix: Halt playback for the data warning and respect it on stream retry #5533)
• All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml — n/a, no new strings
• Any jetpack compose components I added or changed are covered by compose previews — n/a, no UI components changed
• I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics. — n/a, no analytics changes

I have tested any UI changes...

n/a — no UI changes

[claude]

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

---

Code Review

I reviewed the diff against origin/main, focusing on correctness, consistency with the existing prefetch path, and edge cases.

Verdict: LGTM. The fix is correct, well-scoped, and faithfully mirrors the established buildPrefetchRequest pattern (PlaybackManager.kt:2802-2806). The single caller (ExoPlayerDataSourceFactory.startCachingEntireEpisodeIfNeeded) is updated, PrefetchWorker.prefetchNextEpisode already had the same networkConstraint parameter shape, and the resetEpisodeCaching 416-recovery path correctly inherits the fix through the shared helper. WorkManager re-evaluates the UNMETERED constraint at runtime, so a job enqueued on mobile data will correctly defer until WiFi.

Correctness ✅

• On WiFi, UNMETERED is satisfied → caching runs as before. ✔️
• On metered data with Warn before using data ON → UNMETERED defers the job. ✔️
• Warning OFF → CONNECTED preserves opt-out behavior. ✔️
• enqueueUniqueWork(..., REPLACE, ...) means each new media source re-enqueues with a freshly-computed constraint, so a mid-session settings toggle is picked up on the next episode change. ✔️

Suggestions (non-blocking)

1. Duplicated network-constraint logic / testability. The if (warnOnMeteredNetwork) UNMETERED else CONNECTED ternary now lives in two places — buildPrefetchRequest (PlaybackManager.kt:2802) and startCachingEntireEpisodeIfNeeded (ExoPlayerDataSourceFactory.kt:126-130). Extracting it into a tiny shared pure function (e.g. fun cacheNetworkConstraint(warnOnMeteredNetwork: Boolean): NetworkType) would remove the duplication and — more importantly — give this path the same unit-test coverage that PrefetchNextEpisodeTest gives the prefetch path. Since the PR checklist notes there's no test harness for CacheWorker/ExoPlayerDataSourceFactory, a pure helper is the cheapest way to lock the behavior in. Fix this →

2. Platform asymmetry (pre-existing, worth a note). buildPrefetchRequest short-circuits on AppPlatform.WearOs (PlaybackManager.kt:2791), but startCachingEntireEpisodeIfNeeded has no platform guard — full-episode caching is gated only by settings.cacheEntirePlayingEpisode.value. This isn't a regression from this PR, but if entire-episode caching can run on Wear/Automotive it's now also subject to the metered-network deferral there. Just confirm that's the intended behavior on those platforms.

3. CHANGELOG. This is a user-facing behavior change (mobile-data usage). The checklist notes the entry is added after PR creation — just a reminder to land it before merge, referencing the PR number.

Testing

Agreed that a device-level check of the metered/deferred path and the 416-reset re-cache (both flagged as manual TODOs in the description) is the right validation here given the WorkManager plumbing nature of the change.

---

Nice, targeted fix — the reasoning in the description about auto-advance re-enqueuing an unconstrained job is convincing and matches what the code does.

[claude]

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

---

Code Review

• Read changed files
• Compare against buildPrefetchRequest reference implementation
• Check for bugs / correctness / consistency
• Post review

The change is correct, minimal, and well-reasoned. It closes a real gap: CacheWorker.startCachingEntireEpisode hardcoded NetworkType.CONNECTED, so the full-episode background cache ran over metered data even with Warn before using data on, while buildPrefetchRequest already did the right thing. The new networkConstraint parameter and the UNMETERED/CONNECTED selection are a faithful mirror of the prefetch path.

Verification

• Constraint logic matches the reference. PlaybackManager.buildPrefetchRequest (lines 2802–2806) uses exactly if (warnOnMeteredNetwork) UNMETERED else CONNECTED — the new code in ExoPlayerDataSourceFactory is identical. ✅
• All callers updated. startCachingEntireEpisode has a single call site (ExoPlayerDataSourceFactory:131); the new required parameter is supplied there. No other callers, no compile break. ✅
• resetEpisodeCaching (416 path) inherits the fix. It routes through the shared startCachingEntireEpisodeIfNeeded helper (line 153), so the re-cache also respects the constraint. ✅
• Placement is correct. The constraint is computed inside the if (cacheFactory != null) block, only when caching will actually be enqueued. ✅
• CHANGELOG entry is under the unreleased 8.17 section with the correct PR link. ✅

Minor suggestions (non-blocking)

1. Duplicated constraint logic → extract a shared helper. The if (warnOnMeteredNetwork) UNMETERED else CONNECTED rule now lives in two places (PlaybackManager.buildPrefetchRequest and ExoPlayerDataSourceFactory:126-130). This is a small policy that's easy to let drift. Extracting a single function (e.g. fun meteredNetworkConstraint(warnOnMeteredNetwork: Boolean): NetworkType) would keep the two paths in lockstep and, importantly, make the rule unit-testable — PrefetchNextEpisodeTest already covers the prefetch side, but the cache side is currently untestable because the logic is inlined. Fix this →

2. settings.warnOnMeteredNetwork.value is read as a snapshot at createMediaSource/resetEpisodeCaching time. This is fine — WorkManager enforces the constraint independently once enqueued, and this matches how buildPrefetchRequest reads it. Just worth noting there's no re-evaluation if the setting changes while a deferred job is queued (again, consistent with prefetch, so no action needed).

On testing

Agree with the PR note that there's no existing harness for CacheWorker/ExoPlayerDataSourceFactory. If suggestion #1 is adopted, the extracted helper is trivially testable and would be worth a small test mirroring PrefetchNextEpisodeTest.

Verdict: Looks good to merge. The two suggestions are optional polish; the fix itself is sound and behavior-preserving on WiFi. Note the two manual verification TODOs in the description (metered-data deferral + 416 re-cache path) are still worth completing before release since there's no automated coverage.

[claude]

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

---

Code Review — updated diff

• Read changed files & diff vs origin/main
• Verify the prior review feedback was addressed
• Check correctness / consistency / edge cases
• Post review

This synchronize pushed the follow-up that both prior reviews asked for: the if (warnOnMeteredNetwork) UNMETERED else CONNECTED rule is now a single shared function and it's covered by a unit test. LGTM — ready to merge.

What changed since the last review

• Duplication removed (prev. suggestion Update issue templates #1). cacheNetworkConstraint(warnOnMeteredNetwork: Boolean): NetworkType is now a top-level internal function in PlaybackManager.kt:2869. Both buildPrefetchRequest (PlaybackManager.kt:2863) and ExoPlayerDataSourceFactory.startCachingEntireEpisodeIfNeeded (ExoPlayerDataSourceFactory.kt:127) call it, so the two paths can no longer drift. ✅
• Now unit-tested. CacheNetworkConstraintTest covers both branches (UNMETERED when the warning is on, CONNECTED when off) — this is the coverage the prefetch path had via PrefetchNextEpisodeTest and the cache path lacked. ✅

Correctness ✅

• Same package (…repositories.playback), same module, so the internal visibility resolves for both call sites — no compile/visibility issue. ✔️
• Behavior is byte-for-byte identical to the previous inline logic; pure refactor + the original CacheWorker fix. ✔️
• resetEpisodeCaching (the 416 recovery path) still routes through startCachingEntireEpisodeIfNeeded, so it inherits the constraint. ✔️
• Constraint is computed inside the if (cacheFactory != null) block — only when a job is actually enqueued. ✔️

Non-blocking notes (unchanged from before)

1. Platform asymmetry (pre-existing). buildPrefetchRequest short-circuits on AppPlatform.WearOs (PlaybackManager.kt:2849), but startCachingEntireEpisodeIfNeeded has no platform guard — entire-episode caching is gated only by settings.cacheEntirePlayingEpisode.value. Not a regression from this PR; just confirm the metered deferral is the intended behavior wherever entire-episode caching can run.
2. Snapshot read of the setting. settings.warnOnMeteredNetwork.value is read once at enqueue time; a deferred job won't re-evaluate if the setting flips while queued. This matches the prefetch path, so no action needed.
3. Manual TODOs still open. The metered/deferred verification and the 416 re-cache path are flagged as manual in the description — worth completing before release given there's no automated coverage of the WorkManager plumbing itself.

Nice, clean follow-up — the extraction landed exactly where it should and the test locks the policy in.

[claude]

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

---

Code Review — latest sync (buildCacheWorkRequest extraction + test)

• Read changed files & diff vs origin/main
• Verify the new commit (Add test for cache work request network constraint)
• Check correctness / consistency / edge cases
• Post review

This synchronize adds the piece that closes the last gap from earlier reviews: the actual WorkManager constraint plumbing in CacheWorker is now unit-tested, not just the cacheNetworkConstraint policy helper. LGTM — ready to merge.

What changed since the last review

• CacheWorker.buildCacheWorkRequest extracted (CacheWorker.kt:96). Request construction (input data + Constraints + OneTimeWorkRequest) is split out from startCachingEntireEpisode, which keeps the observe + enqueueUniqueWork side effects. Pure refactor — enqueue path is byte-for-byte identical to before. ✔️
• CacheWorkerRequestTest added. Asserts requiredNetworkType() is UNMETERED/CONNECTED for the respective inputs by reading workSpec.constraints.requiredNetworkType. This is the coverage the WorkManager-plumbing side previously lacked; CacheNetworkConstraintTest covers the policy, this covers that the policy actually lands on the request. ✔️

Correctness ✅

• networkConstraint flows end-to-end: settings.warnOnMeteredNetwork.value → cacheNetworkConstraint(...) (ExoPlayerDataSourceFactory.kt:136) → startCachingEntireEpisode → buildCacheWorkRequest → Constraints.setRequiredNetworkType(...). ✔️
• Single caller of startCachingEntireEpisode is updated; resetEpisodeCaching (416 path) still routes through startCachingEntireEpisodeIfNeeded, so it inherits the constraint. ✔️
• internal visibility of buildCacheWorkRequest resolves for the test (same module/package). The @Suppress("RestrictedApi") on the workSpec accessor is the standard way to introspect a WorkRequest in tests — fine. ✔️

Non-blocking notes (carried over, no action needed)

1. Platform asymmetry (pre-existing). buildPrefetchRequest short-circuits on AppPlatform.WearOs, but startCachingEntireEpisodeIfNeeded has no platform guard — entire-episode caching is gated only by settings.cacheEntirePlayingEpisode.value. Not a regression here; just confirm the metered deferral is intended wherever entire-episode caching can run.
2. Snapshot read of the setting. warnOnMeteredNetwork.value is read once at enqueue time; a deferred job won't re-evaluate if the setting flips while queued — consistent with the prefetch path.
3. Manual TODOs still open. Metered/deferred verification and the 416 re-cache path are flagged manual in the description; worth completing before release since there's no automated coverage of the runtime WorkManager behavior itself.

Clean, well-tested follow-up. The extraction was done as a pure refactor and the new test locks in exactly the behavior the fix is about.