diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt index ca99103..14ea7dd 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt @@ -6,14 +6,14 @@ import de.jeanlucmakiola.agendula.data.tasks.room.TaskListEntity import de.jeanlucmakiola.agendula.data.tasks.room.TasksDatabase import de.jeanlucmakiola.caldav.CalDavDiscovery import de.jeanlucmakiola.caldav.TaskCollection +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.withContext -import kotlinx.coroutines.withTimeoutOrNull import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import javax.inject.Inject import javax.inject.Singleton -import kotlin.time.Duration.Companion.seconds /** * The one thing the sign-in flow needs from [AccountRepository]. @@ -235,22 +235,38 @@ class AccountRepository @Inject constructor( withContext(io) { syncTrigger.cancel(displayName) revokeAppPassword(accountId) - // The lists survive as device-only lists, so their cursors must not: - // a re-added account would otherwise inherit a "reconciled recently" - // that was true of a different account's data. - cadence.forget( - database.taskLists().syncedForAccount(accountId).map { it.id }.toSet(), - ) - // Play's Account Deletion policy does not apply to us — there is no - // Agendula account to delete — but "I want it gone from this device - // too" is a reasonable thing to want, and it is the only way to get - // the tasks off the device without also uninstalling. - if (deleteLocalData) database.taskLists().deleteForAccount(accountId) - accountState.setNeedsSignIn(accountId, false) - credentials.clear(accountId) - database.accounts().delete(accountId) - accounts.find(displayName)?.let { accounts.remove(it) } + // ⚠️ Uncancellable from here. Everything below is destructive and + // spread over four stores that cannot share a transaction, and the + // caller is a viewModelScope tied to the Settings destination — the + // user taps Remove, the screen slides away, and a couple of back + // gestures kill the scope mid-sequence. Only the DataStore writes can + // observe cancellation (every Room DAO here is blocking), so the + // realistic landing point is `cadence.forget`: the app password is + // already revoked server-side while the row survives holding it, and + // the account reads "sign in again" for a credential we ourselves + // invalidated. Land further in and the tasks are gone with the row + // still there. The tail is three DataStore writes, two deletes and an + // AccountManager call — bounded and sub-second, so finishing it is + // strictly better than stopping anywhere inside it. + withContext(NonCancellable) { + // The lists survive as device-only lists, so their cursors must + // not: a re-added account would otherwise inherit a "reconciled + // recently" that was true of a different account's data. + cadence.forget( + database.taskLists().syncedForAccount(accountId).map { it.id }.toSet(), + ) + // Play's Account Deletion policy does not apply to us — there is + // no Agendula account to delete — but "I want it gone from this + // device too" is a reasonable thing to want, and it is the only + // way to get the tasks off the device without also uninstalling. + if (deleteLocalData) database.taskLists().deleteForAccount(accountId) + + accountState.setNeedsSignIn(accountId, false) + credentials.clear(accountId) + database.accounts().delete(accountId) + accounts.find(displayName)?.let { accounts.remove(it) } + } } /** @@ -266,17 +282,19 @@ class AccountRepository @Inject constructor( val origin = account.principalUrl?.toHttpUrlOrNull() ?: return val password = (credentials.get(accountId) as? CredentialStore.Secret.Present)?.value ?: return - runCatching { - // ⚠️ Bounded well below the shared client's 30 s connect / 120 s read. - // The user pressed "remove"; an unreachable server must not leave the - // account sitting on screen for half a minute with nothing happening. - // Losing the revocation is the lesser failure, and it is the one the - // user can still fix by hand on the server. - withTimeoutOrNull(REVOCATION_TIMEOUT) { - gateway.revokeAppPassword( - CalDavGateway.Credentials(username, password, origin), - ) - } + // ⚠️ The budget lives on the request itself, in AppPassword.revoke. + // Wrapping this in withTimeoutOrNull only *looked* bounded: the call + // parks on a socket read that no cancellation can break, and withContext + // returns when its block does, so the deadline passed and we waited + // anyway — minutes, on a multi-homed host that stalls. + try { + gateway.revokeAppPassword( + CalDavGateway.Credentials(username, password, origin), + ) + } catch (_: CancellationException) { + // Deliberately swallowed. If the caller went away mid-revoke we still + // want the removal to finish rather than stop half-done; the tail + // below runs uncancellable for the same reason. } } @@ -284,7 +302,5 @@ class AccountRepository @Inject constructor( /** M3 primary-ish blue; the user recolours a list from its own screen. */ const val DEFAULT_LIST_COLOR = 0xFF4C6FFF.toInt() - /** Long enough for a reachable server, short enough not to feel stuck. */ - val REVOCATION_TIMEOUT = 5.seconds } } diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt index 4d305e1..aad10ae 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt @@ -4,6 +4,9 @@ import okhttp3.HttpUrl import okhttp3.OkHttpClient import okhttp3.Request import java.io.IOException +import kotlin.time.Duration +import kotlin.time.Duration.Companion.seconds +import kotlin.time.toJavaDuration /** * Gives an app password back when the account is removed. @@ -58,12 +61,33 @@ object AppPassword { } /** + * ⚠️ The budget is enforced here because nothing above can enforce it. + * `execute()` parks on a socket read, which no coroutine cancellation and no + * `Thread.interrupt` can break — only closing the socket does, which is what + * `callTimeout` does. Wrapping this call in `withTimeoutOrNull` instead + * returns only once the read has finished anyway, so the caller waits out + * the shared client's own budget: 30s *per resolved address*, doubled by the + * authenticator's retry, plus up to 120s of read timeout. Minutes, for a + * step whose own doc says it must not block the removal. + * + * Not set on the shared client on purpose: `callTimeout` bounds the whole + * exchange including the body, and a multiget of a large list over a slow + * link legitimately runs long. + * * @return true when the server confirmed the revocation. False means the * credential may still exist server-side — the caller carries on regardless. */ - fun revoke(httpClient: OkHttpClient, principal: HttpUrl): Boolean = try { + fun revoke( + httpClient: OkHttpClient, + principal: HttpUrl, + timeout: Duration = REVOCATION_TIMEOUT, + ): Boolean = try { val url = ocsRootFor(principal).newBuilder().addPathSegments(PATH).build() - httpClient.newCall( + httpClient.newBuilder() + // Shares the pool and dispatcher, so this costs nothing. + .callTimeout(timeout.toJavaDuration()) + .build() + .newCall( Request.Builder() .url(url) .delete() @@ -73,10 +97,14 @@ object AppPassword { .header("OCS-APIRequest", "true") .header("Accept", "application/json") .build(), - ).execute().use { it.isSuccessful } + ).execute().use { it.isSuccessful } } catch (_: IOException) { + // callTimeout throws InterruptedIOException, which lands here. false } catch (_: IllegalArgumentException) { false } + + /** What a best-effort courtesy call is worth waiting for. */ + val REVOCATION_TIMEOUT: Duration = 5.seconds } diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt index bd49093..1115006 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt @@ -5,9 +5,13 @@ import okhttp3.HttpUrl.Companion.toHttpUrl import okhttp3.OkHttpClient import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer +import okhttp3.mockwebserver.SocketPolicy import org.junit.After import org.junit.Before import org.junit.Test +import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Duration.Companion.nanoseconds +import kotlin.time.Duration.Companion.seconds class AppPasswordTest { @@ -38,6 +42,30 @@ class AppPasswordTest { .isEqualTo("https://baikal.example.com/") } + @Test fun `a server that never answers does not hold the removal`() { + // Accepts the connection and says nothing — the off-VPN homelab shape. + server.enqueue(MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE)) + + val startedAt = System.nanoTime() + val revoked = AppPassword.revoke( + httpClient, + server.url("/remote.php/dav/principals/users/alice/"), + timeout = 200.milliseconds, + ) + val elapsed = (System.nanoTime() - startedAt).nanoseconds + + // ⚠️ The budget has to live on the call. execute() parks on a socket read + // that neither coroutine cancellation nor Thread.interrupt can break, so + // a timeout imposed from outside returns only once the read has finished + // anyway — after the shared client's own minutes-long budget. + assertThat(revoked).isFalse() + assertThat(elapsed).isLessThan(2.seconds) + } + + @Test fun `the default budget is what the removal is willing to wait`() { + assertThat(AppPassword.REVOCATION_TIMEOUT).isEqualTo(5.seconds) + } + @Test fun `the revocation is a DELETE with the OCS header`() { server.enqueue(MockResponse().setResponseCode(200)) diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt index f3cc412..eb362a8 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavHttpTest.kt @@ -202,6 +202,14 @@ class CalDavHttpTest { .header("WWW-Authenticate", "$scheme realm=\"dav\"") .build() + @Test + fun `the shared client has no call timeout`() { + // ⚠️ Deliberate. A callTimeout bounds the whole exchange including the + // body, and a multiget of a large list over a slow link legitimately runs + // long. The one call that needs a budget sets its own. + assertThat(CalDavHttp.anonymous("Agendula").callTimeoutMillis).isEqualTo(0) + } + @Test fun `derived clients share one connection pool`() { // A fresh OkHttpClient per probe gives each its own pool and dispatcher