Skip to content

Fix StorageException for users with multi-path offline content - #295

Open
ziad-halabi9 wants to merge 1 commit into
mainfrom
fix-storage-exception
Open

Fix StorageException for users with multi-path offline content#295
ziad-halabi9 wants to merge 1 commit into
mainfrom
fix-storage-exception

Conversation

@ziad-halabi9

Copy link
Copy Markdown
Contributor

Summary

  • OfflineDataSourceFactoryHelper.getExternal/getInternal cached one factory per storage type and ignored the path argument after the first call. Because the helper is @Reusable in Dagger, that stale factory persisted for the whole app session, so users whose offline content lived at multiple paths within one storage class (e.g. they switched download location, hit an OS-level path migration, or use a different SD-card root) hit StorageException / ERROR_CODE_IO_UNSPECIFIED on later tracks at any path other than the first-seen one.
  • Switch the per-storage-type field to a path-keyed Map<String, T> so each distinct path memoizes its own factory. Replace !! with checkNotNull on the nullable provider.
  • Add OfflineDataSourceFactoryHelperTest (5 cases): per-path memoization for both getExternal and getInternal, independence of internal/external maps, and IllegalStateException when the provider is null.

Test plan

  • ./gradlew :player:playback-engine:testDebugUnitTest (full module suite passes locally on JBR 21)
  • New OfflineDataSourceFactoryHelperTest — all 5 cases pass

🤖 Generated with Claude Code

The helper cached a single factory per storage type (internal/external),
ignoring the path argument after the first call. Because the helper is
@reusable in Dagger, the stale factory persisted for the app's lifetime,
so users with offline content at multiple paths within one storage class
hit StorageException / ERROR_CODE_IO_UNSPECIFIED on later tracks whose
path didn't match the first-seen one.

Switch the per-storage-type field to a path-keyed map so each distinct
path gets its own factory. Replace !! with checkNotNull on the nullable
provider while we're here.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@ziad-halabi9
ziad-halabi9 requested a review from a team as a code owner May 1, 2026 10:09

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd love if we could use more descriptive naming and given/when/then in the tests, to make them more clear, something like:

@Test
fun `getExternal returns same instance when called multiple times with same path`() {
    // given
    val cacheA = mock<Cache>()
    whenever(offlineCacheProvider.getExternal("a")).thenReturn(cacheA)
    
    // when
    val firstA = helper.getExternal("a")
    val secondA = helper.getExternal("a")
    
    // then
    assertThat(secondA).isSameInstanceAs(firstA)
    ...
}

@michpohl michpohl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with 1 comment. I leave it up to you if you want to change or not.

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.

2 participants