fix(jellyfin): typed HTTP exception instead of a status code buried in a string - #28
Merged
Merged
Conversation
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).
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.
…itory + authenticateByName 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).
PonceGL
added a commit
that referenced
this pull request
Sep 11, 2026
…te cancellation-test comment code-review on PR #35 found four issues: - fetchPlayStoreAnnouncement() used .mapCatching { it.toPlayStoreAnnouncementRemoteConfig() }, which catches Throwable, not just Exception — silently turning a real bug or an OutOfMemoryError in that pure mapping into an ordinary Result.failure. The pre-refactor code built the config inside a catch (e: Exception) branch, so an Error would have propagated as intended. Switched to .map (non-catching), matching that behavior. - The cancellation test's own comment claimed cancelling before the call starts 'exercises exactly' the catch (e: CancellationException) { throw e } inside fetchRawProperties — it doesn't: withContext's pre-flight ensureActive() throws before the function body runs, same issue found on PR #28's JellyfinApiService tests. Unlike that OkHttp case, HttpURLConnection has no interceptor seam to throw an exception mid-call, so there's currently no way to write a test that actually reaches this catch. Reworded the comment to say so plainly instead of claiming coverage it doesn't have. - A third finding (HttpURLConnection is fundamentally not cancellable; the real fix is migrating to the app's injectable OkHttpClient) was already disclosed in this PR's own KDoc as an accepted, deferred limitation — left as-is, not re-litigated. Verified: compiles, full JVM suite green (398 tests, same 5 pre-existing baseline failures as master, none new).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Staged internally first (was chore/p4-jellyfin-typed-exception on PixelPlayerHQ#2819, closed — see that PR's closing comment). Will be proposed upstream again once the downloads feature it's part of is further along.