Update episode thumbnail status when embedded artwork is extracted during playback

[joashrajin]

Description

Fixes PCDROID-79

Supersedes #5303 by @lromero16 (author is away; this re-lands her fix with the review feedback addressed). Root-cause analysis is hers:

Root cause: When embedded artwork is extracted from a downloaded audio file during playback and saved to the artwork cache file, the episode's DB record was never updated — PlaybackManager.onMetadataAvailable() never set PodcastEpisode.thumbnailStatus. Because Room didn't re-emit, DiffUtil detected no change and the filters page ViewHolder was never rebound, so the newly extracted artwork never appeared.

Fix: After saving chapters in onMetadataAvailable(), resolve and persist thumbnailStatus (only when still THUMBNAIL_STATUS_UNKNOWN). The DB write triggers Room → DiffUtil → rebind → Coil loads the cached artwork file.

Change from #5303: the original set THUMBNAIL_STATUS_EMBEDDED_NOT_AVAILABLE even when the "use episode artwork" setting is off — but in that case EpisodeFileMetadata.read() never attempts extraction, so the episode would be permanently marked as having no artwork and never re-evaluated if the user enabled the setting later. The status is now only resolved when extraction was actually attempted (resolveThumbnailStatus() returns null otherwise), with unit tests covering all three cases.

Testing Instructions

1. Ensure Settings → Appearance → Use Episode Artwork is enabled
2. Find and download a podcast episode with embedded artwork
3. Open the Filters tab — note the episode shows podcast-level artwork
4. Play the episode for a few seconds (letting the player read file metadata)
5. Return to the filters page

• Verify the episode now shows its embedded artwork without scrolling away and back
• With Use Episode Artwork disabled, play an episode, then enable the setting — verify embedded artwork can still appear afterwards

Screenshots or Screencast

⚠️ TODO (manual): Add a before/after screenshot of the filters page if required; no UI code changed.

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 (added ResolveThumbnailStatusTest)
• All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml — n/a
• Any jetpack compose components I added or changed are covered by compose previews — n/a
• I have updated (or requested that someone edit) the Event Horizon schema to reflect any new or changed analytics. — n/a

I have tested any UI changes...

• with different themes
• with a landscape orientation
• with the device set to have a large display and font size
• for accessibility with TalkBack

[claude]

Claude finished @joashrajin's task in 3m 2s —— View job

---

Code Review

I reviewed the change against origin/main and traced how thumbnailStatus, EpisodeFileMetadata, and the image request factory interact. Overall this is a clean, well-targeted fix with a nicely extracted, unit-tested pure function. The root-cause analysis is sound and the guard on THUMBNAIL_STATUS_UNKNOWN makes it idempotent across the repeated onTracksChanged callbacks. A few things worth considering below.

🟡 1. TOCTOU on the useEpisodeArtwork setting (same bug class this PR fixes)

useEpisodeArtwork is read in two independent places:

• SimplePlayer.onTracksChanged — EpisodeFileMetadata.read(tracks, settings.artworkConfiguration.value.useEpisodeArtwork, …) decides whether extraction is attempted (SimplePlayer.kt:226).
• PlaybackManager.onMetadataAvailable — resolveThumbnailStatus(…, useEpisodeArtwork = settings.artworkConfiguration.value.useEpisodeArtwork) re-reads the setting (PlaybackManager.kt:1746).

If the user toggles the setting in the (small) window between those two reads, you can reproduce the exact failure mode #5303 was trying to avoid: setting off at read() time (no extraction → embeddedArtworkPath == null) → toggled on before onMetadataAvailable runs → resolveThumbnailStatus returns EMBEDDED_NOT_AVAILABLE, permanently marking an episode whose file was never inspected.

The window is narrow and severity is low, but the cleaner fix removes it entirely: capture what actually happened on the metadata object instead of re-reading settings. e.g. store the flag inside read():

// EpisodeFileMetadata
var useEpisodeArtwork: Boolean = false
    private set

fun read(tracks: Tracks?, useEpisodeArtwork: Boolean, context: Context) {
    this.useEpisodeArtwork = useEpisodeArtwork
    …
}

…and have onMetadataAvailable pass episodeMetadata.useEpisodeArtwork rather than re-reading settings. Both decision points then share a single source of truth. Fix this →

🟡 2. thumbnailStatus is effectively write-only — the DB write is only a re-emit trigger

Tracing the readers: PocketCastsImageRequestFactory chooses episode artwork purely by whether the cache file exists, not by thumbnailStatus:

// PocketCastsImageRequestFactory.kt:98
episode.imageUrl ?: EpisodeFileMetadata.artworkCacheFile(context, episode.uuid).takeIf(File::exists) ?: episode.podcastArtworkUrl(context)

Grepping the codebase, nothing reads PodcastEpisode.thumbnailStatus — this PR is its only writer. So the field's whole purpose here is to "touch" the row and force Room → DiffUtil → rebind, as the PR description notes. It works, but it's an implicit contract that's easy to accidentally break (someone could later "optimize away" a write to a field with no readers). Two suggestions:

• Add a short comment at the call site (PlaybackManager.kt:1748) stating the write exists to trigger re-emission/rebind, since the value itself isn't consumed elsewhere.
• Note that the EMBEDDED_NOT_AVAILABLE branch triggers a rebind that changes nothing on screen (no artwork file to show) — a harmless but unnecessary DB write. If the only goal is to surface newly available artwork, you could skip persisting NOT_AVAILABLE, though keeping it does prevent re-attempts. Your call; just flagging the trade-off.

🟢 3. Non-atomic read-modify-write (minor)

findEpisodeByUuid → check == UNKNOWN → episodeManager.update(episode) writes the entire row (PlaybackManager.kt:1749-1752). A concurrent update to the same episode between the read and the write would be clobbered. Very low risk in this path, but a targeted DAO update would be both safer and more expressive, and could fold the guard into SQL:

UPDATE podcast_episodes SET thumbnail_status = :status
WHERE uuid = :uuid AND thumbnail_status = 0 -- THUMBNAIL_STATUS_UNKNOWN

Optional — the current approach matches existing patterns in this class.

🟢 4. Tests (nit)

ResolveThumbnailStatusTest covers the pure function well (all three branches). The integration logic — the UNKNOWN guard and the as? PodcastEpisode cast (UserEpisodes are correctly skipped) — isn't covered, which is understandable given it lives in PlaybackManager. No action needed, just noting.

🟢 5. CHANGELOG (nit)

This is a user-facing bug fix and the checklist box for CHANGELOG.md is unchecked — worth adding an entry.

---

Summary: Solid, minimal fix that correctly addresses the root cause and improves on #5303. The only thing I'd genuinely encourage before merge is #1 (removing the setting TOCTOU by threading the flag through EpisodeFileMetadata), since it's the same bug class this PR set out to fix. The rest are optional polish.

[claude]

Claude finished @joashrajin's task in 2m 35s —— View job

---

Review 🔍

• Read changed files
• Analyze onMetadataAvailable() changes
• Review resolveThumbnailStatus() logic & tests
• Check for concurrency / threading concerns
• Post review feedback

---

Nice, focused fix. The root-cause analysis is correct: PocketCastsImageRequestFactory already resolves the cached artwork file at bind time (EpisodeFileMetadata.artworkCacheFile(...).takeIf(File::exists), line 99), so the only thing missing was a DB change to make Room re-emit and DiffUtil rebind. Writing thumbnailStatus accomplishes exactly that. The resolveThumbnailStatus() split + tests are clean, and the improvement over #5303 (returning null when extraction wasn't attempted) is the right call.

A couple of things worth considering before merge:

🟠 Full-row read‑modify‑write can clobber concurrent updates

val episode = episodeManager.findEpisodeByUuid(playbackState.episodeUuid) as? PodcastEpisode
if (episode != null && episode.thumbnailStatus == PodcastEpisode.THUMBNAIL_STATUS_UNKNOWN) {
    episode.thumbnailStatus = thumbnailStatus
    episodeManager.update(episode)   // @Update → writes the ENTIRE row
}

episodeManager.update() maps to episodeDao.update(episode) (a Room @Update, i.e. full-row write). This runs from onTracksChanged — i.e. right at playback start, exactly when position / playing-status writes are also happening. Between the findEpisodeByUuid read and this write, another coroutine can update a different column (e.g. updateCurrentPositionInDatabase, playing status), and this full-row write would silently revert it to the value read a moment earlier.

EpisodeDao already has the codebase's established pattern of targeted column updates (updateFileTypeBlocking, updateEpisodeStatus, updatePlayErrorDetailsBlocking, …). A dedicated query avoids the read entirely and eliminates the lost-update window:

@Query("UPDATE podcast_episodes SET thumbnail_status = :thumbnailStatus WHERE uuid = :uuid AND thumbnail_status = ${PodcastEpisode.THUMBNAIL_STATUS_UNKNOWN}")
abstract suspend fun updateThumbnailStatusIfUnknown(thumbnailStatus: Int, uuid: String)

Folding the THUMBNAIL_STATUS_UNKNOWN guard into the WHERE clause also makes the update atomic. Fix this →

🟡 useEpisodeArtwork is read from settings twice

EpisodeFileMetadata.read() is called with settings.artworkConfiguration.value.useEpisodeArtwork in SimplePlayer.onTracksChanged (SimplePlayer.kt:226), and then resolveThumbnailStatus() reads the current value again inside the launch{} coroutine. These are milliseconds apart so the risk is tiny, but they're two independent reads of the same flag. If the setting is toggled in that window, embeddedArtworkPath (computed with the read-time value) and useEpisodeArtwork (callback-time value) can disagree — e.g. setting flips off→on after read() skipped extraction, and the episode gets marked EMBEDDED_NOT_AVAILABLE even though extraction was never attempted, which is the exact "permanently mis-marked" case this PR set out to avoid. Carrying the actual flag used by read() on EpisodeFileMetadata (or passing it through) would make "was extraction attempted" a single source of truth. Minor, non-blocking.

🟢 Notes / nits

• thumbnailStatus is currently a pure rebind trigger — PocketCastsImageRequestFactory never reads it, so EMBEDDED_AVAILABLE vs EMBEDDED_NOT_AVAILABLE produce identical rendering (both just force a re-diff). That's fine, but a one-line comment at the write site would save the next reader from hunting for where the status is consumed.
• The resolveThumbnailStatus KDoc and the three-case unit test are good coverage for the extracted function. Consider one small integration-flavored test on the onMetadataAvailable guard (only writes when UNKNOWN) if the logic moves into the DAO query above.
• CHANGELOG entry looks correctly placed under the unreleased section. 👍

Overall: correct fix for the reported bug, and it supersedes #5303 cleanly. The full-row update is the one item I'd recommend addressing before merge; the rest are optional polish.
• fix/pcdroid-79-filters-episode-artwork

[Copilot]

Pull request overview

Ensures episode artwork can update on the Filters page after embedded artwork is extracted during playback by persisting an episode-level thumbnailStatus change so Room re-emits and the UI rebinds.

Changes:

• Update PlaybackManager.onMetadataAvailable() to resolve and persist PodcastEpisode.thumbnailStatus (when still UNKNOWN) after metadata is read.
• Add resolveThumbnailStatus() helper and unit tests covering the “setting off / extracted / attempted but none found” cases.
• Add a changelog entry for the user-visible bug fix.

Reviewed changes

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

File	Description
modules/services/repositories/src/main/java/au/com/shiftyjelly/pocketcasts/repositories/playback/PlaybackManager.kt	Resolves and persists thumbnailStatus after metadata extraction to trigger Room/UI updates.
modules/services/repositories/src/test/java/au/com/shiftyjelly/pocketcasts/repositories/playback/ResolveThumbnailStatusTest.kt	Unit tests for thumbnail status resolution behavior.
CHANGELOG.md	Documents the bug fix in release notes.

[joashrajin]

Fixed in 245e03c — replaced the full-row update with a targeted conditional query (UPDATE podcast_episodes SET thumbnail_status = :thumbnailStatus WHERE uuid = :uuid AND thumbnail_status = 0) exposed via EpisodeManager.updateThumbnailStatusIfUnknown(). The same commit also addresses the setting-toggle race by capturing artworkExtractionAttempted on EpisodeFileMetadata during read() instead of re-reading the setting.

[claude]

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

---

Re-review (commit 245e03c) 🔍

• Read updated diff
• Verify targeted DAO update fix
• Verify artworkExtractionAttempted TOCTOU fix
• Post review

---

Thanks for the quick turnaround — I traced the updated flow end-to-end and both concerns from the earlier reviews are now properly resolved.

✅ Full-row read-modify-write → atomic conditional query

onMetadataAvailable now calls episodeManager.updateThumbnailStatusIfUnknown(...), which routes to the new EpisodeDao.updateThumbnailStatusIfUnknown():

@Query("UPDATE podcast_episodes SET thumbnail_status = :thumbnailStatus WHERE uuid = :uuid AND thumbnail_status = ${PodcastEpisode.THUMBNAIL_STATUS_UNKNOWN}")

This eliminates the read entirely and folds the UNKNOWN guard into the WHERE clause, so the update is atomic and can no longer clobber concurrent played_up_to / playing-status writes. THUMBNAIL_STATUS_UNKNOWN is a const val 0 (PodcastEpisode.kt:92), so the interpolation compiles fine into the Room query. Parameter ordering checks out across all three layers (call site (uuid, status) → manager (episodeUuid, thumbnailStatus) → DAO (thumbnailStatus, uuid)). 👍

✅ Setting TOCTOU → single source of truth

artworkExtractionAttempted is now captured on the EpisodeFileMetadata instance inside read() (EpisodeFileMetadata.kt:52), and the same instance flows straight into onMetadataAvailable (SimplePlayer.kt:225-227). So embeddedArtworkPath and artworkExtractionAttempted are both derived from one read() call — toggling the setting afterward can no longer produce the "marked EMBEDDED_NOT_AVAILABLE even though extraction was never attempted" case. This is exactly the fix I'd hoped for.

🟢 Optional polish (non-blocking)

• Write-only field: thumbnailStatus still has no reader in the codebase — PocketCastsImageRequestFactory resolves artwork purely by cache-file existence, so EMBEDDED_AVAILABLE vs EMBEDDED_NOT_AVAILABLE render identically and the write exists solely to trigger Room re-emit → DiffUtil → rebind. Worth a one-line comment at PlaybackManager.kt:1748 so a future reader doesn't "optimize away" a write to a field with no consumers.
• Naming nuance: artworkExtractionAttempted = useEpisodeArtwork reads as "the setting was on so extraction would be attempted for any APIC frame" rather than "an APIC frame was actually processed." That's the correct semantic for resolveThumbnailStatus, just slightly loose naming — fine as-is.

Summary: The two substantive issues (lost-update race + setting TOCTOU) are both fixed correctly, and the extracted resolveThumbnailStatus + its three-case test remain clean. This looks good to merge; the remaining items are purely optional polish. ✅
• fix/pcdroid-79-filters-episode-artwork

[claude]

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

---

Review 🔍

• Read changed files & diff vs origin/main
• Review resolveThumbnailStatus() + DAO update
• Check concurrency / correctness / performance
• Post review feedback

---

I re-reviewed against origin/main after the merge (3a87db6). The change is clean and both substantive issues from earlier reviews are still correctly resolved. The fix is minimal, well-scoped, and the root-cause reasoning holds up.

✅ Atomic conditional update (lost-update race resolved)

onMetadataAvailable calls episodeManager.updateThumbnailStatusIfUnknown(...) → EpisodeDao.updateThumbnailStatusIfUnknown():

@Query("UPDATE podcast_episodes SET thumbnail_status = :thumbnailStatus WHERE uuid = :uuid AND thumbnail_status = ${PodcastEpisode.THUMBNAIL_STATUS_UNKNOWN}")

The read-modify-write is gone and the UNKNOWN guard is folded into the WHERE, so this can no longer clobber concurrent played_up_to / playing-status writes that also fire at playback start. THUMBNAIL_STATUS_UNKNOWN is a const val 0 (PodcastEpisode.kt:92), so the interpolation is compile-time constant and valid in the Room query. Parameter order is consistent across the three layers: call site (episodeUuid, thumbnailStatus) → manager (episodeUuid, thumbnailStatus) → DAO (thumbnailStatus, uuid). 👍

✅ Setting TOCTOU resolved

artworkExtractionAttempted is captured on the EpisodeFileMetadata instance inside read() (EpisodeFileMetadata.kt:52) and the same instance flows into onMetadataAvailable, so embeddedArtworkPath and artworkExtractionAttempted derive from a single read() call. Toggling the setting afterward can no longer produce the "marked EMBEDDED_NOT_AVAILABLE even though extraction was never attempted" case that #5303 hit. resolveThumbnailStatus returning null when extraction wasn't attempted keeps the row re-evaluable. 👍

🟢 One correction to the earlier "write-only field" note

Earlier reviews called thumbnailStatus purely a rebind trigger with no reader. That's imprecise: PlaylistDao.kt:547 selects podcast_episode.thumbnail_status AS p_thumbnail_status into the PodcastEpisode entity that backs the filters list, so the field is part of the object DiffUtil compares — which is exactly why writing it forces the rebind. PocketCastsImageRequestFactory doesn't read it for rendering (artwork is resolved by cache-file existence), so EMBEDDED_AVAILABLE vs EMBEDDED_NOT_AVAILABLE render identically today — but the write isn't a no-op field poke; it changes a projected column that participates in list diffing. A one-line comment at PlaybackManager.kt:1861 explaining that the write's purpose is to trigger Room re-emit → DiffUtil rebind would still help the next reader.

🟢 Optional polish (non-blocking)

• Naming: artworkExtractionAttempted = useEpisodeArtwork reads as "the setting was on" rather than "an APIC frame was actually inspected." It's the correct semantic for resolveThumbnailStatus, just slightly loose. Fine as-is.
• Test coverage: ResolveThumbnailStatusTest covers all three branches of the pure function cleanly. The onMetadataAvailable glue (the != null guard + DAO call) isn't unit-tested, which is reasonable given where it lives.
• CHANGELOG: correctly placed under the unreleased 8.18 → Bug Fixes section with the right PR link. 👍

Summary: Correct, minimal fix for the reported bug; supersedes #5303 cleanly and both prior race concerns remain fixed after the merge. Nothing blocking — the only suggestion is the clarifying comment at the write site. ✅
• fix/pcdroid-79-filters-episode-artwork

[geekygecko]

Would you mind sharing which podcast you used to test? One with embedded artwork but it isn't in the feed?

[joashrajin]

Thanks for the ping, @geekygecko

I never dig into it too much, but was testing using https://pca.st/upgrade and https://pca.st/moreorless (although this one generally has the same podcast artwork as the episode artwork)

I found a reliable fixture and was able to reproduce the intermittent behavior naturally on the pre-fix build.

Fixture

• More or Less — “WS More or Less: How Louis Bachelier Scooped Economists by Half a Century”
• About 8.4 MiB / 9 minutes
• The feed/API episode image is empty.
• The podcast artwork is the current blue Tim Harford portrait.
• The MP3 contains an 800×800 turquoise BBC Radio 4 “More or Less: Behind the Statistics” APIC image.
• Before playback, the episode had thumbnail_status = UNKNOWN and no artwork cache. I did not mutate the database or throttle the network.

Natural reproduction

1. Enable Use Episode Artwork.
2. Add the episode to a manual playlist/filter and keep that row visible.
3. Press Play normally from the row; do not navigate or scroll.
4. Watch the row during the first ~8 seconds.

On the pre-fix build (eff297ca9):

• Playback opened at 12:37:12.110.
• Metadata and the embedded-artwork cache arrived at 12:37:14.544.
• The visible row still showed the blue podcast portrait.
• The first normal playback-position save occurred at 12:37:19.467.
• Only after that save did the row switch to the turquoise embedded artwork.

This means the failure can be a short stale interval rather than a permanently wrong image. The old build eventually appears correct because the unrelated position save changes the episode row, Room emits it again, and DiffUtil rebinds the artwork. The episode's thumbnail_status remained UNKNOWN, confirming that metadata extraction itself had not triggered the update.

On the fixed build (3a87db69c), I verified separately with a fresh untouched embedded-artwork episode that the visible row switched when metadata arrived—about 3.8 seconds before its first routine playback-position save—and thumbnail_status changed from UNKNOWN to EMBEDDED_AVAILABLE.

[wpmobilebot]

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