sync: put the revocation's timeout on the request itself
withTimeoutOrNull around the revoke bounded nothing. The call parks on a socket read that neither coroutine cancellation nor Thread.interrupt can break, and withContext returns only when its block does -- so the deadline passed and we waited anyway, for the shared client's own budget: 30s per resolved address, doubled by the authenticator's retry, plus up to 120s of read timeout. Removing a homelab account off the VPN sat there for minutes with nothing visibly happening. Only closing the socket ends it, which is what callTimeout does. The budget moves to AppPassword.revoke, where the "best effort, must not block the removal" contract is already written down, and where it can be enforced. Not on the shared client: callTimeout covers the whole exchange including the body, and a multiget of a large list over a slow link legitimately runs long. CalDavHttpTest pins that decision. The destructive tail is now uncancellable. It spans four stores that cannot share a transaction, and the caller is a viewModelScope tied to the Settings destination, so a couple of back gestures used to kill it mid-sequence. Only the DataStore writes can observe cancellation -- every Room DAO here is blocking -- so the landing point was cadence.forget: the app password already revoked while the row survives holding it, and the account asking the user to sign in again for a credential we invalidated ourselves. Further in, the tasks are gone and the row stays. The tail is bounded and sub-second, so finishing it always beats stopping inside it.
This commit is contained in:
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user