From b0aa543f260767283a11b772992bc5387ee84bdb Mon Sep 17 00:00:00 2001 From: tapframe <85391825+tapframe@users.noreply.github.com> Date: Wed, 22 Jul 2026 20:19:54 +0530 Subject: [PATCH] fix(simkl): align request retry policies --- .../app/features/simkl/SimklApiClient.kt | 57 +++++++++++------ .../features/simkl/SimklMutationRepository.kt | 4 ++ .../app/features/simkl/SimklApiClientTest.kt | 61 +++++++++++++++++++ .../simkl/SimklMutationRepositoryTest.kt | 25 ++++++++ 4 files changed, 128 insertions(+), 19 deletions(-) diff --git a/composeApp/src/commonMain/kotlin/com/nuvio/app/features/simkl/SimklApiClient.kt b/composeApp/src/commonMain/kotlin/com/nuvio/app/features/simkl/SimklApiClient.kt index a38306602..8768b5102 100644 --- a/composeApp/src/commonMain/kotlin/com/nuvio/app/features/simkl/SimklApiClient.kt +++ b/composeApp/src/commonMain/kotlin/com/nuvio/app/features/simkl/SimklApiClient.kt @@ -7,6 +7,7 @@ import kotlinx.coroutines.CancellationException import kotlinx.coroutines.delay import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock +import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable import kotlinx.serialization.decodeFromString import kotlinx.serialization.json.Json @@ -25,6 +26,7 @@ internal enum class SimklHttpMethod { internal enum class SimklRetryPolicy { TRANSIENT_FAILURES, + SYNC_WRITE, NEVER, } @@ -86,12 +88,14 @@ internal class SimklApiClient( null } - var lastTransportFailure: Throwable? = null - val maxRetries = when (request.retryPolicy) { - SimklRetryPolicy.TRANSIENT_FAILURES -> MAX_RETRIES - SimklRetryPolicy.NEVER -> 0 + val maxAttempts = when (request.retryPolicy) { + SimklRetryPolicy.TRANSIENT_FAILURES, + SimklRetryPolicy.SYNC_WRITE, + -> MAX_ATTEMPTS + SimklRetryPolicy.NEVER -> 1 } - for (attempt in 0..maxRetries) { + var syncWriteLockRetried = false + for (attempt in 0 until maxAttempts) { awaitRateLimit(request.method) val response = try { engine.execute( @@ -106,8 +110,7 @@ internal class SimklApiClient( } catch (error: CancellationException) { throw error } catch (error: Throwable) { - lastTransportFailure = error - if (attempt == maxRetries) { + if (attempt == maxAttempts - 1) { throw SimklApiException( status = null, errorCode = "transport_failure", @@ -125,6 +128,18 @@ internal class SimklApiClient( attempt = attempt + 1, ) + if ( + request.retryPolicy == SimklRetryPolicy.SYNC_WRITE && + response.status == 400 && + !syncWriteLockRetried && + response.errorCode(json) == "rate_limit" && + attempt < maxAttempts - 1 + ) { + syncWriteLockRetried = true + sleep(SYNC_WRITE_LOCK_RETRY_DELAY_MS) + continue + } + when (classifySimklResponse(response.status, request.scrobbleStopConflictIsSuccess)) { SimklResponseAction.SUCCESS -> return@withLock response.toApiResponse() SimklResponseAction.SOFT_SUCCESS -> { @@ -136,7 +151,7 @@ internal class SimklApiClient( } SimklResponseAction.FAIL -> throw response.toApiException(json) SimklResponseAction.RETRY -> { - if (attempt == maxRetries) throw response.toApiException(json) + if (attempt == maxAttempts - 1) throw response.toApiException(json) sleep( retryDelayMs( attempt = attempt, @@ -148,12 +163,7 @@ internal class SimklApiClient( } } - throw SimklApiException( - status = null, - errorCode = "transport_failure", - message = "Simkl request failed", - cause = lastTransportFailure, - ) + error("Simkl request loop completed without a response") } private suspend fun awaitRateLimit(method: SimklHttpMethod) { @@ -173,7 +183,8 @@ internal class SimklApiClient( private companion object { const val GET_INTERVAL_MS = 100L const val POST_INTERVAL_MS = 1_000L - const val MAX_RETRIES = 5 + const val MAX_ATTEMPTS = 5 + const val SYNC_WRITE_LOCK_RETRY_DELAY_MS = 3_000L const val RETRY_JITTER_BOUND_MS = 1_000L } } @@ -252,7 +263,8 @@ internal fun retryDelayMs( ?.coerceAtLeast(0L) ?.times(1_000L) ?: 0L - return max(exponentialDelayMs, retryAfterMs) + jitterMs.coerceIn(0L, 1_000L) + return (max(exponentialDelayMs, retryAfterMs) + jitterMs.coerceIn(0L, 1_000L)) + .coerceAtMost(60_000L) } private fun RawHttpResponse.toApiResponse(isSoftSuccess: Boolean = false): SimklApiResponse = @@ -264,18 +276,24 @@ private fun RawHttpResponse.toApiResponse(isSoftSuccess: Boolean = false): Simkl ) private fun RawHttpResponse.toApiException(json: Json): SimklApiException { - val envelope = body.takeIf(String::isNotBlank)?.let { payload -> - runCatching { json.decodeFromString(payload) }.getOrNull() - } + val envelope = errorEnvelope(json) return SimklApiException( status = status, errorCode = envelope?.error, message = envelope?.message?.takeIf(String::isNotBlank) + ?: envelope?.errorDescription?.takeIf(String::isNotBlank) ?: envelope?.error?.takeIf(String::isNotBlank) ?: "Simkl request failed with HTTP $status", ) } +private fun RawHttpResponse.errorCode(json: Json): String? = errorEnvelope(json)?.error + +private fun RawHttpResponse.errorEnvelope(json: Json): SimklErrorEnvelope? = + body.takeIf(String::isNotBlank)?.let { payload -> + runCatching { json.decodeFromString(payload) }.getOrNull() + } + private fun Map.headerValue(name: String): String? = entries.firstOrNull { (key, _) -> key.equals(name, ignoreCase = true) }?.value @@ -284,4 +302,5 @@ private data class SimklErrorEnvelope( val error: String? = null, val code: Int? = null, val message: String? = null, + @SerialName("error_description") val errorDescription: String? = null, ) diff --git a/composeApp/src/commonMain/kotlin/com/nuvio/app/features/simkl/SimklMutationRepository.kt b/composeApp/src/commonMain/kotlin/com/nuvio/app/features/simkl/SimklMutationRepository.kt index 5aebd8d17..45a8b4f50 100644 --- a/composeApp/src/commonMain/kotlin/com/nuvio/app/features/simkl/SimklMutationRepository.kt +++ b/composeApp/src/commonMain/kotlin/com/nuvio/app/features/simkl/SimklMutationRepository.kt @@ -49,6 +49,7 @@ internal class SimklMutationService( method = SimklHttpMethod.POST, path = "/sync/add-to-list", body = buildSimklListMutationBody(candidates, destination, json), + retryPolicy = SimklRetryPolicy.SYNC_WRITE, ), ) onMutationCommitted() @@ -70,6 +71,7 @@ internal class SimklMutationService( method = SimklHttpMethod.POST, path = "/sync/history", body = buildSimklHistoryMutationBody(candidates, json), + retryPolicy = SimklRetryPolicy.SYNC_WRITE, ), ) onMutationCommitted() @@ -84,6 +86,7 @@ internal class SimklMutationService( method = SimklHttpMethod.POST, path = "/sync/history/remove", body = buildSimklHistoryRemovalBody(candidates, json), + retryPolicy = SimklRetryPolicy.SYNC_WRITE, ), ) onMutationCommitted() @@ -100,6 +103,7 @@ internal class SimklMutationService( method = SimklHttpMethod.POST, path = "/scrobble/${action.wireValue}", body = buildSimklScrobbleBody(event, json), + retryPolicy = SimklRetryPolicy.NEVER, scrobbleStopConflictIsSuccess = action == TrackingScrobbleAction.STOP, ), ) diff --git a/composeApp/src/commonTest/kotlin/com/nuvio/app/features/simkl/SimklApiClientTest.kt b/composeApp/src/commonTest/kotlin/com/nuvio/app/features/simkl/SimklApiClientTest.kt index 8716bfe61..16189f341 100644 --- a/composeApp/src/commonTest/kotlin/com/nuvio/app/features/simkl/SimklApiClientTest.kt +++ b/composeApp/src/commonTest/kotlin/com/nuvio/app/features/simkl/SimklApiClientTest.kt @@ -31,6 +31,7 @@ class SimklApiClientTest { assertEquals(8_000L, retryDelayMs(3, null, 0L)) assertEquals(16_000L, retryDelayMs(4, null, 0L)) assertEquals(30_250L, retryDelayMs(0, "30", 250L)) + assertEquals(60_000L, retryDelayMs(0, "120", 1_000L)) } @Test @@ -149,6 +150,66 @@ class SimklApiClientTest { assertEquals(1, deterministicEngine.requests.size) } + @Test + fun `transient failures stop after five total attempts`() = runBlocking { + val engine = RecordingEngine( + response(503), + response(503), + response(503), + response(503), + response(503), + response(200), + ) + val harness = TestHarness(engine) + + assertFailsWith { + harness.client.execute(SimklApiRequest(SimklHttpMethod.GET, "/unavailable")) + } + + assertEquals(5, engine.requests.size) + assertEquals(listOf(1_000L, 2_000L, 4_000L, 8_000L), harness.sleeps) + } + + @Test + fun `sync write lock is retried once and other bad requests are not`() = runBlocking { + val lockedEngine = RecordingEngine( + response(400, """{"error":"rate_limit","error_description":"Another sync is in progress"}"""), + response(200), + ) + val lockedHarness = TestHarness(lockedEngine) + + lockedHarness.client.execute( + SimklApiRequest( + method = SimklHttpMethod.POST, + path = "/sync/history", + body = "{}", + retryPolicy = SimklRetryPolicy.SYNC_WRITE, + ), + ) + + assertEquals(2, lockedEngine.requests.size) + assertEquals(listOf(3_000L), lockedHarness.sleeps) + + val invalidEngine = RecordingEngine( + response(400, """{"error":"wrong_parameter"}"""), + response(200), + ) + val invalidHarness = TestHarness(invalidEngine) + + assertFailsWith { + invalidHarness.client.execute( + SimklApiRequest( + method = SimklHttpMethod.POST, + path = "/sync/history", + body = "{}", + retryPolicy = SimklRetryPolicy.SYNC_WRITE, + ), + ) + } + + assertEquals(1, invalidEngine.requests.size) + } + @Test fun `retry after and unauthorized handling are applied once`() = runBlocking { val retryEngine = RecordingEngine( diff --git a/composeApp/src/commonTest/kotlin/com/nuvio/app/features/simkl/SimklMutationRepositoryTest.kt b/composeApp/src/commonTest/kotlin/com/nuvio/app/features/simkl/SimklMutationRepositoryTest.kt index 981b73c4c..4ebad69c3 100644 --- a/composeApp/src/commonTest/kotlin/com/nuvio/app/features/simkl/SimklMutationRepositoryTest.kt +++ b/composeApp/src/commonTest/kotlin/com/nuvio/app/features/simkl/SimklMutationRepositoryTest.kt @@ -16,6 +16,7 @@ import kotlinx.serialization.json.jsonObject import kotlinx.serialization.json.jsonPrimitive import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse import kotlin.test.assertNull import kotlin.test.assertTrue @@ -140,6 +141,30 @@ class SimklMutationRepositoryTest { assertEquals(2, committed) } + @Test + fun `service leaves failed scrobble retry to the next player event`() = runBlocking { + val engine = RecordingEngine(response(status = 503), response(status = 200)) + val service = SimklMutationService( + client = SimklApiClient( + engine = engine, + accessToken = { "token" }, + onUnauthorized = {}, + nowEpochMs = { 0L }, + sleep = {}, + retryJitterMs = { 0L }, + ), + ) + + assertFailsWith { + service.scrobble( + action = TrackingScrobbleAction.PAUSE, + event = TrackingScrobbleEvent(movie(), progressPercent = 45.0), + ) + } + + assertEquals(listOf("/scrobble/pause"), engine.paths) + } + private fun String.asObject() = json.parseToJsonElement(this).jsonObject private fun movie() = TrackingMediaReference(