From 7fa1eedd66e01b36dc8f7ce059500a411ce8c738 Mon Sep 17 00:00:00 2001 From: PonceGL Date: Wed, 9 Sep 2026 20:40:49 -0600 Subject: [PATCH 1/3] =?UTF-8?q?fix(jellyfin):=20P.4=20=E2=80=94=20excepci?= =?UTF-8?q?=C3=B3n=20tipada=20para=20errores=20HTTP?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit request() envolvía cualquier fallo en Exception("HTTP $code: ..."), enterrando el código de estado dentro de un String (GEN-AP-06). Sin él, el clasificador de errores de F3.3 (docs/downloads) no puede distinguir 401/403/404/416/429/5xx sin parsear texto. - JellyfinHttpException(statusCode, retryAfter, message): retryAfter va crudo, sin parsear — los dos formatos de Retry-After los decide quien clasifica. - catch (e: CancellationException) { throw e } antes del catch genérico: el catch (e: Exception) existente tragaba la cancelación (AND-CONC-04). - Cero llamantes en el proyecto parseaban el mensaje anterior; sin regresiones. Self-review (code-review skill, medium effort) surfaced one finding: authenticateByName(), a few lines below, has the exact same CancellationException-swallowing catch (e: Exception) — same bug, same file, just not the method R19 named. Fixed it too, same one-line pattern, with a test mirroring the existing deterministic cancel-before-call case. Left its HTTP-failure paths as plain Exception (not JellyfinHttpException): unlike request(), nothing downstream needs to distinguish auth failure status codes, and R19/F3.3 don't ask for it there. 6 tests JVM nuevos (5 + 1 de la autorevisión), en verde. Baseline: 393 tests, los mismos 5 fallos preexistentes de siempre, ninguno nuevo (check-baseline.sh). --- .../network/jellyfin/JellyfinApiService.kt | 13 +- .../network/jellyfin/JellyfinHttpException.kt | 16 ++ .../JellyfinApiServiceErrorHandlingTest.kt | 154 ++++++++++++++++++ 3 files changed, 182 insertions(+), 1 deletion(-) create mode 100644 app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt create mode 100644 app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt diff --git a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt index 0fb1979be7..0544cba2bd 100644 --- a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt +++ b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt @@ -1,6 +1,7 @@ package com.theveloper.pixelplay.data.network.jellyfin import com.theveloper.pixelplay.data.jellyfin.model.JellyfinCredentials +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.withContext import okhttp3.HttpUrl.Companion.toHttpUrl @@ -107,6 +108,8 @@ class JellyfinApiService @Inject constructor( Timber.d("$TAG: Authentication successful for user $username") Result.success(Pair(accessToken, userId)) } + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Authentication failed") Result.failure(e) @@ -142,12 +145,20 @@ class JellyfinApiService @Inject constructor( if (!response.isSuccessful) { Timber.w("$TAG: <<< HTTP $code for $path") - return@withContext Result.failure(Exception("HTTP $code: ${response.message}")) + return@withContext Result.failure( + JellyfinHttpException( + statusCode = code, + retryAfter = response.header("Retry-After"), + message = "HTTP $code: ${response.message}", + ) + ) } Timber.d("$TAG: <<< HTTP $code for $path, body length: ${body.length}") Result.success(body) } + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: !!! FAILED GET $path") Result.failure(e) diff --git a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt new file mode 100644 index 0000000000..8ec8641a24 --- /dev/null +++ b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt @@ -0,0 +1,16 @@ +package com.theveloper.pixelplay.data.network.jellyfin + +/** + * A typed HTTP failure from [JellyfinApiService], carrying the status code and the raw + * `Retry-After` header instead of burying them inside [message] (`GEN-AP-06`, R19 of the + * downloads plan). + * + * [retryAfter] is passed through unparsed: HTTP allows it as either a number of seconds or an + * HTTP-date, and deciding between the two — and clamping the result — is the caller's job + * (`F3.3` in `docs/downloads/F3.md`), not this transport-level type's. + */ +class JellyfinHttpException( + val statusCode: Int, + val retryAfter: String?, + message: String, +) : Exception(message) diff --git a/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt b/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt new file mode 100644 index 0000000000..34050382ab --- /dev/null +++ b/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt @@ -0,0 +1,154 @@ +package com.theveloper.pixelplay.data.network.jellyfin + +import com.theveloper.pixelplay.data.jellyfin.model.JellyfinCredentials +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.cancel +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.runTest +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.OkHttpClient +import okhttp3.Protocol +import okhttp3.Response +import okhttp3.ResponseBody.Companion.toResponseBody +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test + +/** + * `P.4`: [request] must expose the HTTP status code as a typed [JellyfinHttpException] instead + * of burying it inside a message string (R19), and must never rewrap a [CancellationException] + * into a [Result.failure] (`AND-CONC-04`). + * + * No mock server dependency is added for this: a fake [okhttp3.Interceptor] on the client injected + * into [JellyfinApiService] returns a canned [Response] — real OkHttp objects, no new test + * dependency. It works because `newBuilder()` carries interceptors over (confirmed independently + * by `F1.2`'s finding on this same client). + */ +class JellyfinApiServiceErrorHandlingTest { + + private fun serviceRespondingWith( + code: Int, + headers: Map = emptyMap(), + body: String = "{}", + ): JellyfinApiService { + val client = OkHttpClient.Builder() + .addInterceptor { chain -> + val builder = Response.Builder() + .request(chain.request()) + .protocol(Protocol.HTTP_1_1) + .code(code) + .message("test") + .body(body.toResponseBody("application/json".toMediaType())) + headers.forEach { (name, value) -> builder.addHeader(name, value) } + builder.build() + } + .build() + + return JellyfinApiService(client).apply { + setCredentials( + JellyfinCredentials( + serverUrl = "http://example.invalid", + username = "user", + password = "pw", + accessToken = "test-token", + userId = "user-1", + ) + ) + } + } + + @Test + fun `a non-2xx response fails with a JellyfinHttpException carrying the status code`() = runTest { + val service = serviceRespondingWith(code = 500) + + val result = service.ping() + + assertTrue(result.isFailure) + val error = result.exceptionOrNull() + assertTrue(error is JellyfinHttpException) + assertEquals(500, (error as JellyfinHttpException).statusCode) + } + + @Test + fun `a 429 with Retry-After carries the raw header value, unparsed`() = runTest { + val service = serviceRespondingWith(code = 429, headers = mapOf("Retry-After" to "30")) + + val result = service.ping() + + val error = result.exceptionOrNull() as JellyfinHttpException + assertEquals(429, error.statusCode) + assertEquals("30", error.retryAfter) + } + + @Test + fun `a failure without a Retry-After header leaves it null, not throws`() = runTest { + val service = serviceRespondingWith(code = 404) + + val result = service.ping() + + val error = result.exceptionOrNull() as JellyfinHttpException + assertEquals(404, error.statusCode) + assertNull(error.retryAfter) + } + + @Test + fun `a successful response still succeeds — unchanged happy path`() = runTest { + val service = serviceRespondingWith(code = 200) + + val result = service.ping() + + assertTrue(result.isSuccess) + assertTrue(result.getOrThrow()) + } + + // ─── Case borde: cancellation propagates instead of becoming a Result ─────── + // + // Same deterministic pattern as `GitHubAnnouncementPropertiesServiceTest` (P.9): cancel the + // coroutine before the suspend function is even entered, so the cancellation check inside + // `withContext` fires before any network code runs — no dependency on real socket timing. + + @Test + fun `a coroutine cancelled before the call starts propagates cancellation, not a Result`() = runTest { + val service = serviceRespondingWith(code = 200) + var sawCancellation = false + + val job = launch { + cancel() + try { + service.ping() + } catch (e: CancellationException) { + sawCancellation = true + throw e + } + } + job.join() + + assertTrue(job.isCancelled) + assertTrue(sawCancellation) + } + + // Same bug, same fix, a different method in this file (surfaced by self-review, not R19 — + // authenticateByName() isn't part of F3.3's error taxonomy, but the cancellation-swallowing + // catch is identical and just as real). + + @Test + fun `authenticateByName also propagates cancellation instead of becoming a Result`() = runTest { + val service = serviceRespondingWith(code = 200) + var sawCancellation = false + + val job = launch { + cancel() + try { + service.authenticateByName("http://example.invalid", "user", "pw") + } catch (e: CancellationException) { + sawCancellation = true + throw e + } + } + job.join() + + assertTrue(job.isCancelled) + assertTrue(sawCancellation) + } +} From 7b592756cbcd725a5ed28bef403844fc56694a53 Mon Sep 17 00:00:00 2001 From: PonceGL Date: Thu, 10 Sep 2026 16:43:48 -0600 Subject: [PATCH 2/3] docs: strip internal-planning-doc references from code comments Comments cited local-only planning files and rule IDs (docs/downloads/*.md, GEN-/AND- rule docs) that don't exist outside this machine and are meaningless to anyone reviewing the PR. Reworded to keep the same technical reasoning without the dangling references. --- .../data/network/jellyfin/JellyfinHttpException.kt | 7 +++---- .../jellyfin/JellyfinApiServiceErrorHandlingTest.kt | 9 ++++----- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt index 8ec8641a24..0c58c27281 100644 --- a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt +++ b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinHttpException.kt @@ -2,12 +2,11 @@ package com.theveloper.pixelplay.data.network.jellyfin /** * A typed HTTP failure from [JellyfinApiService], carrying the status code and the raw - * `Retry-After` header instead of burying them inside [message] (`GEN-AP-06`, R19 of the - * downloads plan). + * `Retry-After` header instead of burying them inside [message]. * * [retryAfter] is passed through unparsed: HTTP allows it as either a number of seconds or an - * HTTP-date, and deciding between the two — and clamping the result — is the caller's job - * (`F3.3` in `docs/downloads/F3.md`), not this transport-level type's. + * HTTP-date, and deciding between the two — and clamping the result — is the caller's job, + * not this transport-level type's. */ class JellyfinHttpException( val statusCode: Int, diff --git a/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt b/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt index 34050382ab..5ceca224b6 100644 --- a/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt +++ b/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt @@ -16,14 +16,13 @@ import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test /** - * `P.4`: [request] must expose the HTTP status code as a typed [JellyfinHttpException] instead - * of burying it inside a message string (R19), and must never rewrap a [CancellationException] - * into a [Result.failure] (`AND-CONC-04`). + * [request] must expose the HTTP status code as a typed [JellyfinHttpException] instead + * of burying it inside a message string, and must never rewrap a [CancellationException] + * into a [Result.failure]. * * No mock server dependency is added for this: a fake [okhttp3.Interceptor] on the client injected * into [JellyfinApiService] returns a canned [Response] — real OkHttp objects, no new test - * dependency. It works because `newBuilder()` carries interceptors over (confirmed independently - * by `F1.2`'s finding on this same client). + * dependency. It works because `newBuilder()` carries interceptors over. */ class JellyfinApiServiceErrorHandlingTest { From 50bb16e6943c955d2e98936e38d149d0336421a9 Mon Sep 17 00:00:00 2001 From: PonceGL Date: Thu, 10 Sep 2026 17:48:07 -0600 Subject: [PATCH 3/3] fix(jellyfin): stop swallowing CancellationException in JellyfinRepository + authenticateByName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit code-review on PR #28 found three real issues: - JellyfinRepository's 8 suspend functions caught generic Exception with no CancellationException passthrough, so the cancellation fix in JellyfinApiService never reached any real caller — login() is the direct case (it calls authenticateByName()), but the same pattern wrapped syncPlaylists, syncPlaylistSongs, syncLibrarySongs, searchSongs, getLyrics and both syncAllPlaylistsAndSongs() catches. Added catch (e: CancellationException) { throw e } to all of them. - authenticateByName()'s HTTP-failure branch still returned a plain Exception with the status code buried in the message, unlike request(), which this PR already fixed. Now uses JellyfinHttpException for both. - Both new cancellation tests passed regardless of whether the catch (e: CancellationException) { throw e } lines existed, because withContext's own pre-flight ensureActive() throws before the function body runs when the job is already cancelled — neither test actually reached the code they claimed to cover. Added a test that throws CancellationException from inside the interceptor instead (mid-call, inside the try block), which does exercise the real catch. authenticateByName() couldn't get the same test: this module runs with unitTests.isReturnDefaultValues = true, so unmocked JSONObject returns null before the interceptor is ever reached — documented in the test file, not fixed here (needs Robolectric or a JSON dependency, out of scope for this PR). Verified: compiles, full JVM suite green (395 tests, same 5 pre-existing baseline failures as master, none new). --- .../data/jellyfin/JellyfinRepository.kt | 17 ++++++ .../network/jellyfin/JellyfinApiService.kt | 8 ++- .../JellyfinApiServiceErrorHandlingTest.kt | 61 ++++++++++++++++--- 3 files changed, 78 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/com/theveloper/pixelplay/data/jellyfin/JellyfinRepository.kt b/app/src/main/java/com/theveloper/pixelplay/data/jellyfin/JellyfinRepository.kt index 82f9d4acd1..355d1a9bdc 100644 --- a/app/src/main/java/com/theveloper/pixelplay/data/jellyfin/JellyfinRepository.kt +++ b/app/src/main/java/com/theveloper/pixelplay/data/jellyfin/JellyfinRepository.kt @@ -25,6 +25,7 @@ import com.theveloper.pixelplay.data.preferences.PlaylistPreferencesRepository import com.theveloper.pixelplay.data.stream.BulkSyncResult import com.theveloper.pixelplay.data.stream.CloudMusicUtils import dagger.hilt.android.qualifiers.ApplicationContext +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow @@ -160,6 +161,8 @@ class JellyfinRepository @Inject constructor( _isLoggedInFlow.value = true Timber.d("$TAG: Login successful for $username@$serverUrl") Result.success(username) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Login failed") api.clearCredentials() @@ -251,6 +254,8 @@ class JellyfinRepository @Inject constructor( Timber.d("$TAG: Synced ${entities.size} playlists") Result.success(entities) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to sync playlists") Result.failure(e) @@ -294,6 +299,8 @@ class JellyfinRepository @Inject constructor( Timber.d("$TAG: Synced ${entities.size} songs for playlist $playlistId") Result.success(entities.size) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to sync playlist songs") Result.failure(e) @@ -335,6 +342,8 @@ class JellyfinRepository @Inject constructor( Timber.d("$TAG: Synced ${entities.size} library songs") Result.success(entities.size) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to sync library songs") Result.failure(e) @@ -356,6 +365,8 @@ class JellyfinRepository @Inject constructor( val playlistResult = syncPlaylists().getOrElse { try { syncUnifiedLibrarySongsFromJellyfin() + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to sync unified library after playlist fetch failure") } @@ -377,6 +388,8 @@ class JellyfinRepository @Inject constructor( try { syncUnifiedLibrarySongsFromJellyfin() + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to sync unified library") } @@ -426,6 +439,8 @@ class JellyfinRepository @Inject constructor( } val jellyfinSongs = JellyfinResponseParser.parseSongs(result.getOrThrow()) Result.success(jellyfinSongs.map { it.toDisplaySong() }) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Search failed") Result.failure(e) @@ -460,6 +475,8 @@ class JellyfinRepository @Inject constructor( return@withContext result } Result.failure(Exception("No lyrics found")) + } catch (e: CancellationException) { + throw e } catch (e: Exception) { Timber.e(e, "$TAG: Failed to get lyrics for song $songId") Result.failure(e) diff --git a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt index 0544cba2bd..26f5ea14de 100644 --- a/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt +++ b/app/src/main/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiService.kt @@ -93,7 +93,13 @@ class JellyfinApiService @Inject constructor( okHttpClient.newCall(request).execute().use { response -> if (!response.isSuccessful) { - return@withContext Result.failure(Exception("HTTP ${response.code}: ${response.message}")) + return@withContext Result.failure( + JellyfinHttpException( + statusCode = response.code, + retryAfter = response.header("Retry-After"), + message = "HTTP ${response.code}: ${response.message}", + ) + ) } val responseBody = response.body.string() diff --git a/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt b/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt index 5ceca224b6..c05dcc4003 100644 --- a/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt +++ b/app/src/test/java/com/theveloper/pixelplay/data/network/jellyfin/JellyfinApiServiceErrorHandlingTest.kt @@ -101,11 +101,14 @@ class JellyfinApiServiceErrorHandlingTest { assertTrue(result.getOrThrow()) } - // ─── Case borde: cancellation propagates instead of becoming a Result ─────── + // ─── Cancellation before the call starts ──────────────────────────────────── // - // Same deterministic pattern as `GitHubAnnouncementPropertiesServiceTest` (P.9): cancel the + // Same deterministic pattern as `GitHubAnnouncementPropertiesServiceTest`: cancel the // coroutine before the suspend function is even entered, so the cancellation check inside // `withContext` fires before any network code runs — no dependency on real socket timing. + // Note: this exercises `withContext`'s own pre-flight `ensureActive()`, not the + // `catch (e: CancellationException) { throw e }` lines below it — those are covered + // separately further down, by throwing mid-call instead of cancelling before the call. @Test fun `a coroutine cancelled before the call starts propagates cancellation, not a Result`() = runTest { @@ -127,12 +130,8 @@ class JellyfinApiServiceErrorHandlingTest { assertTrue(sawCancellation) } - // Same bug, same fix, a different method in this file (surfaced by self-review, not R19 — - // authenticateByName() isn't part of F3.3's error taxonomy, but the cancellation-swallowing - // catch is identical and just as real). - @Test - fun `authenticateByName also propagates cancellation instead of becoming a Result`() = runTest { + fun `authenticateByName also propagates cancellation before the call starts`() = runTest { val service = serviceRespondingWith(code = 200) var sawCancellation = false @@ -150,4 +149,52 @@ class JellyfinApiServiceErrorHandlingTest { assertTrue(job.isCancelled) assertTrue(sawCancellation) } + + // ─── Cancellation surfacing mid-call ───────────────────────────────────────── + // + // The tests above cancel *before* `request()`/`authenticateByName()` are even entered, so + // they never actually reach the `catch (e: CancellationException) { throw e }` lines inside + // those functions — deleting those lines wouldn't fail either test above. This one makes + // the interceptor throw `CancellationException` from inside the blocking `execute()` call, + // landing directly in that catch block, so a regression (removing the rethrow, or + // reordering it after the generic `catch (e: Exception)`) actually fails a test. + // + // Only `request()` (via [ping]) is covered this way, not `authenticateByName()`: this + // module runs with `unitTests.isReturnDefaultValues = true`, so the unmocked + // `org.json.JSONObject` used to build `authenticateByName()`'s POST body returns `null` + // from `toString()` before the interceptor is ever reached — no JVM test can exercise + // that function's body beyond the pre-flight cancellation case above. Its catch-block + // ordering was fixed identically to `request()`'s and reviewed by eye; a real test needs + // either Robolectric or a JSON dependency for this module, out of scope here. + + private fun serviceThrowingMidCall(exception: Throwable): JellyfinApiService { + val client = OkHttpClient.Builder() + .addInterceptor { throw exception } + .build() + return JellyfinApiService(client).apply { + setCredentials( + JellyfinCredentials( + serverUrl = "http://example.invalid", + username = "user", + password = "pw", + accessToken = "test-token", + userId = "user-1", + ) + ) + } + } + + @Test + fun `a CancellationException thrown mid-call by request() propagates uncaught, not wrapped in a Result`() = runTest { + val service = serviceThrowingMidCall(CancellationException("simulated mid-call cancellation")) + + var caught: CancellationException? = null + try { + service.ping() + } catch (e: CancellationException) { + caught = e + } + + assertTrue(caught != null) + } }