From c92e2143b19518ce2f33a07207ffb42feaf1d337 Mon Sep 17 00:00:00 2001 From: Piyush Date: Wed, 13 May 2026 15:15:52 +0530 Subject: [PATCH] fix(sync): bound recursive incentive retries with exponential backoff (#156) IncentiveRepo.pullAndSaveAllIncentiveActivities() and pullAndSaveAllIncentiveRecords() previously caught SocketTimeoutException and recursively re-invoked themselves. Under sustained poor connectivity each timeout produced a fresh suspending stack frame, eventually surfacing as StackOverflowError on field-worker devices. The recursive path is also exercised on the 401/5002 branches because successful token refresh deliberately throws SocketTimeoutException("Refreshed Token!") to retry the call, compounding the depth. Replace recursion with an iterative retryWithBackoff helper: * helpers/RetryWithBackoff.kt: generic suspending utility that re-runs the block up to maxAttempts (default 3) with multiplicative backoff (default 1s -> 2s -> 4s, capped at 30s). The default retry predicate matches only SocketTimeoutException to preserve historical behaviour for auth, parse and business-logic failures; callers can pass a wider predicate. Uses kotlinx.coroutines.delay so the coroutine remains cancellable while waiting between attempts, unlike Thread.sleep. * IncentiveRepo: both pull methods now wrap their bodies in retryWithBackoff { ... }. Labelled returns target the lambda so the existing success and short-circuit semantics are preserved. The outer withContext block keeps a catch for the final SocketTimeoutException (returned after attempts exhaust) and the catch-all Exception, both of which return false rather than swallowing into an infinite loop. Tests cover: success on first attempt without delay; success on retry; exhaustion rethrowing the last exception; non-retryable exceptions propagating immediately; exponential growth verified via virtual time; maxDelayMs cap; custom predicate; argument-validation guards; default predicate matching only SocketTimeoutException. The same recursive pattern exists in 27 other repository classes (audited via 'catch (e: SocketTimeoutException)'). They are intentionally out of scope for this PR; retryWithBackoff is structured so each can be migrated in a follow-up without further refactoring. --- .../sakhi/helpers/RetryWithBackoff.kt | 93 +++++++++ .../sakhi/repositories/IncentiveRepo.kt | 185 +++++++++--------- .../sakhi/helpers/RetryWithBackoffTest.kt | 168 ++++++++++++++++ 3 files changed, 354 insertions(+), 92 deletions(-) create mode 100644 app/src/main/java/org/piramalswasthya/sakhi/helpers/RetryWithBackoff.kt create mode 100644 app/src/test/java/org/piramalswasthya/sakhi/helpers/RetryWithBackoffTest.kt diff --git a/app/src/main/java/org/piramalswasthya/sakhi/helpers/RetryWithBackoff.kt b/app/src/main/java/org/piramalswasthya/sakhi/helpers/RetryWithBackoff.kt new file mode 100644 index 000000000..dccbbb500 --- /dev/null +++ b/app/src/main/java/org/piramalswasthya/sakhi/helpers/RetryWithBackoff.kt @@ -0,0 +1,93 @@ +package org.piramalswasthya.sakhi.helpers + +import kotlinx.coroutines.delay +import timber.log.Timber +import java.net.SocketTimeoutException + +/** + * Default number of attempts (including the initial call) used when a caller + * does not supply an explicit value. + */ +const val DEFAULT_MAX_RETRY_ATTEMPTS: Int = 3 + +/** + * Default delay before the second attempt, in milliseconds. + */ +const val DEFAULT_INITIAL_RETRY_DELAY_MS: Long = 1_000L + +/** + * Upper bound for the per-attempt delay so a long backoff chain cannot stall + * a sync for an unbounded period. + */ +const val DEFAULT_MAX_RETRY_DELAY_MS: Long = 30_000L + +/** + * Multiplicative growth factor applied between attempts. + */ +const val DEFAULT_BACKOFF_FACTOR: Double = 2.0 + +/** + * Executes [block] with bounded retries and exponential backoff. + * + * This replaces the legacy pattern where a repository function caught a + * transient network exception and recursively re-invoked itself. With sustained + * connectivity failures the unbounded recursion produced StackOverflowErrors + * (AMRIT#156); switching to an iterative backoff also keeps the coroutine + * cancellable while it waits because [delay] is suspending rather than + * blocking. + * + * The default [retryOn] predicate matches only [SocketTimeoutException] to + * preserve the historical scope of which failures are considered transient. + * Callers needing a broader policy can pass their own predicate. + * + * @param maxAttempts total attempts including the initial call; must be >= 1 + * @param initialDelayMs delay before the second attempt; must be >= 0 + * @param maxDelayMs ceiling applied after each multiplicative step; + * must be >= [initialDelayMs] + * @param backoffFactor multiplicative growth applied between attempts; + * must be > 0 + * @param retryOn predicate deciding whether a thrown [Throwable] is retryable + * @param block the suspending body to execute + * @return the result of the first successful invocation of [block] + * @throws Throwable the last exception observed after attempts are exhausted, + * or the first non-retryable exception encountered + */ +suspend fun retryWithBackoff( + maxAttempts: Int = DEFAULT_MAX_RETRY_ATTEMPTS, + initialDelayMs: Long = DEFAULT_INITIAL_RETRY_DELAY_MS, + maxDelayMs: Long = DEFAULT_MAX_RETRY_DELAY_MS, + backoffFactor: Double = DEFAULT_BACKOFF_FACTOR, + retryOn: (Throwable) -> Boolean = ::isTransientNetworkFailure, + block: suspend () -> T +): T { + require(maxAttempts >= 1) { "maxAttempts must be at least 1, was $maxAttempts" } + require(initialDelayMs >= 0) { "initialDelayMs must be non-negative, was $initialDelayMs" } + require(maxDelayMs >= initialDelayMs) { "maxDelayMs must be >= initialDelayMs" } + require(backoffFactor > 0.0) { "backoffFactor must be positive, was $backoffFactor" } + + var currentDelay = initialDelayMs + repeat(maxAttempts - 1) { attempt -> + try { + return block() + } catch (t: Throwable) { + if (!retryOn(t)) throw t + Timber.w( + t, + "retryWithBackoff: attempt %d/%d failed; sleeping %dms before retry", + attempt + 1, maxAttempts, currentDelay + ) + delay(currentDelay) + currentDelay = (currentDelay.toDouble() * backoffFactor) + .toLong() + .coerceAtMost(maxDelayMs) + } + } + return block() +} + +/** + * Default retry predicate. Matches only [SocketTimeoutException]; aligned with + * the pre-existing catch sites in repository classes so behaviour for + * non-timeout failures (auth, parse, business errors) remains identical. + */ +fun isTransientNetworkFailure(t: Throwable): Boolean = t is SocketTimeoutException diff --git a/app/src/main/java/org/piramalswasthya/sakhi/repositories/IncentiveRepo.kt b/app/src/main/java/org/piramalswasthya/sakhi/repositories/IncentiveRepo.kt index 007be10f2..a149111e7 100644 --- a/app/src/main/java/org/piramalswasthya/sakhi/repositories/IncentiveRepo.kt +++ b/app/src/main/java/org/piramalswasthya/sakhi/repositories/IncentiveRepo.kt @@ -12,6 +12,7 @@ import okhttp3.RequestBody.Companion.asRequestBody import org.json.JSONObject import org.piramalswasthya.sakhi.database.room.dao.IncentiveDao import org.piramalswasthya.sakhi.database.shared_preferences.PreferenceDao +import org.piramalswasthya.sakhi.helpers.retryWithBackoff import org.piramalswasthya.sakhi.helpers.setToEndOfTheDay import org.piramalswasthya.sakhi.model.IncentiveActivityListRequest import org.piramalswasthya.sakhi.model.IncentiveActivityNetwork @@ -46,61 +47,61 @@ class IncentiveRepo @Inject constructor( suspend fun pullAndSaveAllIncentiveActivities(user: User): Boolean { return withContext(Dispatchers.IO) { try { - val currentLang = preferenceDao.getCurrentLanguage().symbol - val stateId = user.state.id - val districtId = user.district.id - val requestBody = IncentiveActivityListRequest(stateId, districtId, currentLang) - val response = amritApiService.getAllIncentiveActivities(requestBody = requestBody) - val statusCode = response.code() - if (statusCode == 200) { - val responseString = response.body()?.string() - if (responseString != null) { - val jsonObj = JSONObject(responseString) - - val errorMessage = jsonObj.getString("errorMessage") - val responseStatusCode = jsonObj.getInt("statusCode") - Timber.d("Pull from amrit incentives data : $responseStatusCode") - when (responseStatusCode) { - 200 -> { - try { - val dataObj = jsonObj.getString("data") - saveIncentiveMasterData(dataObj) - } catch (e: Exception) { - Timber.d("Incentive master data not synced $e") - return@withContext false + retryWithBackoff { + val currentLang = preferenceDao.getCurrentLanguage().symbol + val stateId = user.state.id + val districtId = user.district.id + val requestBody = IncentiveActivityListRequest(stateId, districtId, currentLang) + val response = amritApiService.getAllIncentiveActivities(requestBody = requestBody) + val statusCode = response.code() + if (statusCode == 200) { + val responseString = response.body()?.string() + if (responseString != null) { + val jsonObj = JSONObject(responseString) + + val errorMessage = jsonObj.getString("errorMessage") + val responseStatusCode = jsonObj.getInt("statusCode") + Timber.d("Pull from amrit incentives data : $responseStatusCode") + when (responseStatusCode) { + 200 -> { + try { + val dataObj = jsonObj.getString("data") + saveIncentiveMasterData(dataObj) + } catch (e: Exception) { + Timber.d("Incentive master data not synced $e") + return@retryWithBackoff false + } + + return@retryWithBackoff true } - return@withContext true - } - - 401, 5002 -> { - if (userRepo.refreshTokenTmc( - user.userName, user.password - ) - ) throw SocketTimeoutException("Refreshed Token!") - else throw IllegalStateException("User Logged out!!") - } + 401, 5002 -> { + if (userRepo.refreshTokenTmc( + user.userName, user.password + ) + ) throw SocketTimeoutException("Refreshed Token!") + else throw IllegalStateException("User Logged out!!") + } - 5000 -> { - if (errorMessage == "No record found") return@withContext true - } + 5000 -> { + if (errorMessage == "No record found") return@retryWithBackoff true + } - else -> { - throw IllegalStateException("$responseStatusCode received, don't know what todo!?") + else -> { + throw IllegalStateException("$responseStatusCode received, don't know what todo!?") + } } } } + true } - } catch (e: SocketTimeoutException) { - Timber.e("incentives error : $e") - pullAndSaveAllIncentiveActivities(user) - return@withContext true + Timber.e(e, "incentives error after exhausting retries") + false } catch (e: Exception) { Timber.d("Caught $e at incentives!") - return@withContext false + false } - true } } @@ -117,67 +118,67 @@ class IncentiveRepo @Inject constructor( suspend fun pullAndSaveAllIncentiveRecords(user: User): Boolean { return withContext(Dispatchers.IO) { try { - - val requestBody = IncentiveRecordListRequest( - user.userId, - getDateTimeStringFromLong( - preferenceDao.lastIncentivePullTimestamp - )!!, - getDateTimeStringFromLong( - Calendar.getInstance().setToEndOfTheDay().timeInMillis - )!!, - villageID = user.state.id - ) - val response = amritApiService.getAllIncentiveRecords(requestBody = requestBody) - val statusCode = response.code() - if (statusCode == 200) { - val responseString = response.body()?.string() - if (responseString != null) { - val jsonObj = JSONObject(responseString) - - val errorMessage = jsonObj.getString("errorMessage") - val responseStatusCode = jsonObj.getInt("statusCode") - Timber.d("Pull from amrit incentives data : $responseStatusCode") - when (responseStatusCode) { - 200 -> { - try { - val dataObj = jsonObj.getString("data") - saveIncentiveRecordsData(dataObj) - } catch (e: Exception) { - Timber.d("Incentive master data not synced $e") - return@withContext false + retryWithBackoff { + val requestBody = IncentiveRecordListRequest( + user.userId, + getDateTimeStringFromLong( + preferenceDao.lastIncentivePullTimestamp + )!!, + getDateTimeStringFromLong( + Calendar.getInstance().setToEndOfTheDay().timeInMillis + )!!, + villageID = user.state.id + ) + val response = amritApiService.getAllIncentiveRecords(requestBody = requestBody) + val statusCode = response.code() + if (statusCode == 200) { + val responseString = response.body()?.string() + if (responseString != null) { + val jsonObj = JSONObject(responseString) + + val errorMessage = jsonObj.getString("errorMessage") + val responseStatusCode = jsonObj.getInt("statusCode") + Timber.d("Pull from amrit incentives data : $responseStatusCode") + when (responseStatusCode) { + 200 -> { + try { + val dataObj = jsonObj.getString("data") + saveIncentiveRecordsData(dataObj) + } catch (e: Exception) { + Timber.d("Incentive master data not synced $e") + return@retryWithBackoff false + } + + return@retryWithBackoff true } - return@withContext true - } - - 401, 5002 -> { - if (userRepo.refreshTokenTmc( - user.userName, user.password - ) - ) throw SocketTimeoutException("Refreshed Token!") - else throw IllegalStateException("User Logged out!!") - } + 401, 5002 -> { + if (userRepo.refreshTokenTmc( + user.userName, user.password + ) + ) throw SocketTimeoutException("Refreshed Token!") + else throw IllegalStateException("User Logged out!!") + } - 5000 -> { - if (errorMessage == "No record found") return@withContext true - } + 5000 -> { + if (errorMessage == "No record found") return@retryWithBackoff true + } - else -> { - throw IllegalStateException("$responseStatusCode received, don't know what todo!?") + else -> { + throw IllegalStateException("$responseStatusCode received, don't know what todo!?") + } } } } + true } } catch (e: SocketTimeoutException) { - Timber.e("incentives error : $e") - pullAndSaveAllIncentiveRecords(user) - return@withContext true + Timber.e(e, "incentives error after exhausting retries") + false } catch (e: Exception) { Timber.d("Caught $e at incentives!") - return@withContext false + false } - true } } diff --git a/app/src/test/java/org/piramalswasthya/sakhi/helpers/RetryWithBackoffTest.kt b/app/src/test/java/org/piramalswasthya/sakhi/helpers/RetryWithBackoffTest.kt new file mode 100644 index 000000000..02b5f7716 --- /dev/null +++ b/app/src/test/java/org/piramalswasthya/sakhi/helpers/RetryWithBackoffTest.kt @@ -0,0 +1,168 @@ +package org.piramalswasthya.sakhi.helpers + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue +import org.junit.Assert.fail +import org.junit.Test +import java.io.IOException +import java.net.SocketTimeoutException + +@OptIn(ExperimentalCoroutinesApi::class) +class RetryWithBackoffTest { + + @Test + fun `returns the block result on first success without delaying`() = runTest { + var invocations = 0 + val result = retryWithBackoff { + invocations++ + "ok" + } + assertEquals("ok", result) + assertEquals(1, invocations) + assertEquals(0L, currentTime) + } + + @Test + fun `retries on SocketTimeoutException and succeeds on the second attempt`() = runTest { + var invocations = 0 + val result = retryWithBackoff(initialDelayMs = 100L) { + invocations++ + if (invocations < 2) throw SocketTimeoutException("first attempt timed out") + "ok" + } + assertEquals("ok", result) + assertEquals(2, invocations) + assertEquals(100L, currentTime) + } + + @Test + fun `stops after maxAttempts and rethrows the last retryable exception`() = runTest { + var invocations = 0 + var caught: SocketTimeoutException? = null + try { + retryWithBackoff(maxAttempts = 3, initialDelayMs = 100L) { + invocations++ + throw SocketTimeoutException("attempt $invocations timed out") + } + fail("Expected SocketTimeoutException after attempts exhausted") + } catch (e: SocketTimeoutException) { + caught = e + } + assertNotNull(caught) + assertEquals(3, invocations) + assertTrue( + "Last exception message should surface to the caller", + caught!!.message!!.contains("attempt 3") + ) + } + + @Test + fun `non-retryable exceptions propagate immediately without sleeping`() = runTest { + var invocations = 0 + var caught: IllegalStateException? = null + try { + retryWithBackoff(initialDelayMs = 1_000L) { + invocations++ + throw IllegalStateException("auth failure should not retry") + } + fail("Expected IllegalStateException to propagate without retry") + } catch (e: IllegalStateException) { + caught = e + } + assertNotNull(caught) + assertEquals(1, invocations) + assertEquals(0L, currentTime) + } + + @Test + fun `delay grows by backoffFactor between attempts`() = runTest { + var invocations = 0 + try { + retryWithBackoff( + maxAttempts = 4, + initialDelayMs = 100L, + maxDelayMs = 10_000L, + backoffFactor = 2.0 + ) { + invocations++ + throw SocketTimeoutException("still down") + } + fail("Expected SocketTimeoutException after attempts exhausted") + } catch (_: SocketTimeoutException) { + // expected + } + assertEquals(4, invocations) + assertEquals( + "Cumulative virtual time should reflect 100ms + 200ms + 400ms backoff", + 700L, currentTime + ) + } + + @Test + fun `delay is capped at maxDelayMs once the cap is exceeded`() = runTest { + var invocations = 0 + try { + retryWithBackoff( + maxAttempts = 5, + initialDelayMs = 1_000L, + maxDelayMs = 2_500L, + backoffFactor = 2.0 + ) { + invocations++ + throw SocketTimeoutException("still down") + } + fail("Expected SocketTimeoutException after attempts exhausted") + } catch (_: SocketTimeoutException) { + // expected + } + assertEquals(5, invocations) + // 1000 + 2000 + capped 2500 + capped 2500 = 8000 + assertEquals(8_000L, currentTime) + } + + @Test + fun `custom retryOn predicate lets callers expand the retryable set`() = runTest { + var invocations = 0 + val result = retryWithBackoff( + initialDelayMs = 50L, + retryOn = { it is IOException } + ) { + invocations++ + if (invocations < 2) throw IOException("connection reset") + "ok" + } + assertEquals("ok", result) + assertEquals(2, invocations) + } + + @Test(expected = IllegalArgumentException::class) + fun `requires at least one attempt`() = runTest { + retryWithBackoff(maxAttempts = 0) { "never called" } + } + + @Test(expected = IllegalArgumentException::class) + fun `requires non-negative initial delay`() = runTest { + retryWithBackoff(initialDelayMs = -1L) { "never called" } + } + + @Test(expected = IllegalArgumentException::class) + fun `requires maxDelay to be at least initialDelay`() = runTest { + retryWithBackoff(initialDelayMs = 1_000L, maxDelayMs = 500L) { "never called" } + } + + @Test(expected = IllegalArgumentException::class) + fun `requires positive backoff factor`() = runTest { + retryWithBackoff(backoffFactor = 0.0) { "never called" } + } + + @Test + fun `default predicate isTransientNetworkFailure matches only socket timeouts`() { + assertTrue(isTransientNetworkFailure(SocketTimeoutException())) + assertFalse(isTransientNetworkFailure(IOException())) + assertFalse(isTransientNetworkFailure(IllegalStateException())) + } +}