From 198a7267c2a5e6692513ff66d326eaac49748d62 Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Wed, 21 Oct 2026 13:30:47 +0000 Subject: [PATCH 1/4] fix(push): retire a removed server's Android push installation Removing a saved server, or signing out of it, made one 8-second attempt at the session DELETE and then cleared the login. When that request failed, the removal could never be resent, as it needed the login that was gone, and the server kept pushing to the phone. Removal and sign-out now allocate the removal at the record's next generation and persist it as a pending retirement, in the same write, before anything is sent. The retirement, POST /api/v2/notifications/push/devices/{device_id}/retire (retirePushDevice), carries only the installation key and generation, with no session, and is sent first. Only a server that answers 404 or 405 gets the session DELETE, at the same key and generation, while the login still exists. Pending retirements are retried on app start and foreground, and when an FCM message arrives, at most three at a time with back-off. 200, 403, 409 and other request refusals drop the entry. 404 or 405 keeps it for 30 days, and other failures keep it for up to 90. Signing in again, re-adding the server, or restoring after a sign-out that stopped registers at a higher generation, which drops the pending retirement. Outcomes go to SiloLog as PushRetire breadcrumbs. Co-Authored-By: Claude Opus 5.5 --- .../silo/android/auth/SignOutTeardown.kt | 10 +- .../silo/android/push/AndroidPushRegistrar.kt | 246 ++++++++++++++++-- .../push/AndroidPushRegistrationStarter.kt | 3 + .../android/push/PushInstallationStore.kt | 11 + .../push/SiloFirebaseMessagingService.kt | 27 +- .../ui/screens/servers/ServerListViewModel.kt | 8 +- .../android/push/AndroidPushRegistrarTest.kt | 88 ++++++- docs/api-v2/android-push-registration.md | 20 +- .../silo/network/api/PushRegistrationApi.kt | 19 ++ .../repository/PushRegistrationRepository.kt | 2 + .../network/api/PushRegistrationApiTest.kt | 25 ++ 11 files changed, 420 insertions(+), 39 deletions(-) diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/auth/SignOutTeardown.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/auth/SignOutTeardown.kt index aa537dda6c..32c279f7da 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/auth/SignOutTeardown.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/auth/SignOutTeardown.kt @@ -74,10 +74,12 @@ class SignOutTeardown( fun stillActive() = serverRegistry.activeServerId.value == signedOutOf if (flushFirst) flushPendingSettings() if (!stillActive()) return false - // While the credentials removing it needs still exist. A registration - // left behind keeps the server pushing this device for an account it - // no longer holds, but failing to remove it never stops the sign-out. - // Cancellation still does, before anyone is signed out. + // Before the credentials are cleared: a server without the retirement + // route can only be told with them. A retirement the server has not + // answered stays queued and is sent again without them, as a + // registration left behind keeps the server pushing this device for an + // account it no longer holds. Failing to remove it never stops the + // sign-out; cancellation still does, before anyone is signed out. var removal: PushRemoval? = null if (signedOutOf != null) tolerateFailure { removal = unregisterPush(signedOutOf) } if (!stillActive()) { diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt index 1f585af218..be984e2f25 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt @@ -4,11 +4,18 @@ import java.security.SecureRandom import java.util.Base64 import java.util.concurrent.ConcurrentHashMap import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.ensureActive +import kotlinx.coroutines.launch import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.Semaphore import kotlinx.coroutines.sync.withLock +import kotlinx.coroutines.sync.withPermit import kotlinx.coroutines.withTimeoutOrNull +import org.siloserver.silo.common.diagnostics.SiloLog +import org.siloserver.silo.common.diagnostics.SiloLogAttribute +import org.siloserver.silo.model.diagnostics.DiagnosticsLogCategory import org.siloserver.silo.model.notifications.* import org.siloserver.silo.network.* import org.siloserver.silo.repository.PushRegistrationRepository @@ -161,14 +168,20 @@ class AndroidPushRegistrar internal constructor( } /** * Removes this device's registration on [serverId], active or not, before that - * server's credentials are cleared. Acts as the login and profile it was - * registered for: the server refuses a removal from any other profile, and a - * refusal is kept for good. The record stays, as the server binds its key to - * this device; a removal intent is no push target. + * server's credentials are cleared. The record stays, as the server binds its + * key to this device; a removal intent is no push target. + * + * The removal takes the record's next generation and is persisted as a + * pending retirement before anything is sent. A retirement needs only the + * installation key, so it needs no login and is sent first. Only a server + * without the retirement route (404 or 405) gets the session removal instead, + * at the same key and generation, as the login and profile it was registered + * for. A retirement the server does not answer for good stays queued and + * [retryPendingRetirements] sends it again after the login is gone. * * Bounded so an unreachable server cannot hold up the sign-out or removal * waiting on it, and never throws, so unreadable records cannot abort it - * either. A removal cut off mid-flight stays pending, like any uncertain one. + * either. */ suspend fun unregisterDevice(serverId: String): PushRemoval { val attempt = beginRequest(serverId) @@ -184,9 +197,11 @@ class AndroidPushRegistrar internal constructor( /** * Undoes [removal] after the sign-out or server removal that made it stopped, - * leaving that server's login in place: registers again as the login and - * profile it was registered for, with the current token. Only that removal - * is undone, and only while it is still the newest operation on that server, + * leaving that server's login in place: drops its pending retirement and + * registers again as the login and profile it was registered for, with the + * current token, at a newer generation that a retirement already sent loses + * to. Only that removal is undone, and only while it is still the newest + * operation on that server, * so a later removal (another sign-out, the server's removal) stands; and * nothing happens once that login is gone, so a removal whose login really * was cleared stays removed. Bounded and never throws, like [unregisterDevice]. @@ -206,13 +221,16 @@ class AndroidPushRegistrar internal constructor( val attempt = removal.attempt if (latestRequests[serverId] !== attempt) return failure("push_request_superseded") val device = deviceIdProvider() - val token = tokenProvider.token()?.trim()?.takeIf { it.length in 64..512 } ?: return failure("invalid_push_token") val found = installations().firstOrNull { it.intent.serverId == serverId && it.deviceId == device && it.refusal == null } ?: return failure("push_not_registered") return serverLock(found.serverUrl).withLock { val record = recordOf(found.serverUrl, device)?.takeIf { it.intent.serverId == serverId && it.refusal == null } ?: return@withLock failure("push_not_registered") val owner = registeredOwnerOf(record) ?: return@withLock failure("push_authority_changed") + // The server stays saved and signed in, so its retirement must not go out. + cancelRetirement(record, "restored") + val token = tokenProvider.token()?.trim()?.takeIf { it.length in 64..512 } + ?: return@withLock failure("invalid_push_token") // The registration it held, or the one register() sends, with the current token. val request = record.intent.registration?.copy(token = token) ?: PushDeviceRegisterRequest("android", token, device) applyOnServer(owner, request, attempt, mayReconcile = true) @@ -227,16 +245,186 @@ class AndroidPushRegistrar internal constructor( // record then holds belongs to a login about to be cleared. markOwnerLeaving(found.serverUrl, device) return serverLock(found.serverUrl).withLock { - // Read again under the server's lock: a registration finishing first can - // change the registered profile, and the server refuses a removal from any - // other profile, for good. - val registered = recordOf(found.serverUrl, device)?.takeIf { it.intent.serverId == serverId && it.removable } - ?: return@withLock failure("push_not_registered") - val owner = registeredOwnerOf(registered) ?: return@withLock failure("push_authority_changed") - applyOnServer(owner, null, attempt, mayReconcile = true).map { Unit } + if (latestRequests[serverId] !== attempt) return@withLock failure("push_request_superseded") + // Read again under the server's lock, as a registration finishing first + // changes the record: the removal is allocated over whatever it left. + val retiring = queueRetirement(found.serverUrl, device, serverId) ?: return@withLock failure("push_not_registered") + val retired = sendRetirement(retiring) + if (retired.outcome != RetirementOutcome.UNSUPPORTED) return@withLock retired.result + // A server without the retirement route takes only the session removal, + // at the same generation, from the profile that registered: it refuses + // one from any other profile, for good. Unless that removal lands, the + // retirement stays queued for when the server is updated. + val owner = registeredOwnerOf(retiring) ?: return@withLock retired.result + val removed = applyOnServer(owner, null, attempt, mayReconcile = false).map { Unit } + if (removed is ApiResult.Success) recordRetirement("cancelled", "session_removed", retiring.generation) + removed } } + /** + * Allocates the removal of [serverUrl]'s registration for [device] and queues + * it as a retirement, in one write, before anything is sent. A removal not yet + * confirmed keeps its generation, so it is replayed exactly; a registration, + * confirmed or not, is superseded at the next generation, which the server + * orders above it whether or not it ever arrived. Null when nothing is left to + * remove. + */ + private suspend fun queueRetirement(serverUrl: String, device: String, serverId: String): PushInstallation? { + var queued: PushInstallation? = null + updateRecord(serverUrl, device) { stored -> + if (stored == null || stored.intent.serverId != serverId || !stored.removable) return@updateRecord stored + val at = clock() + val next = if (stored.intent.registration == null) { + // The lifetime counts from the first queueing. + stored.copy(ownerLeaving = true, retirement = PushRetirement(stored.generation, stored.retirement?.queuedAtMs ?: at)) + } else { + check(stored.generation < Long.MAX_VALUE) { "push_generation_exhausted" } + val generation = stored.generation + 1 + stored.copy(generation = generation, intent = stored.intent.copy(registration = null), acknowledged = false, + receipt = null, ownerLeaving = true, retirement = PushRetirement(generation, at)) + } + queued = next + next + } + if (queued != null) retirementFailures.remove(serverUrl) + return queued + } + + /** + * Sends the retirements still queued for removed or signed-out servers whose + * back-off has passed, a few at a time. They need no login, so this runs on + * app start and foreground, before anyone signs in, and when a push arrives: + * [afterPush] skips the back-off, as a push from a retired server is the sign + * it is still sending them. Never throws. + */ + suspend fun retryPendingRetirements(afterPush: Boolean = false) { + val pending = try { + installations().filter { it.retirement != null } + } catch (cancelled: CancellationException) { + throw cancelled + } catch (_: Exception) { + return + } + val at = clock() + val due = pending.filter { record -> + val failed = retirementFailures[record.serverUrl] ?: return@filter true + at - failed.atMs >= if (afterPush) RETIREMENT_MIN_RETRY_MS else retirementBackOffMs(failed.count) + } + if (due.isEmpty()) return + val permits = Semaphore(RETIREMENT_CONCURRENCY) + coroutineScope { + for (record in due) launch { + permits.withPermit { withTimeoutOrNull(PUSH_WORK_BUDGET_MS) { retryRetirement(record) } } + } + } + } + + private suspend fun retryRetirement(found: PushInstallation) { + val lock = serverLock(found.serverUrl) + // Another command on that server is under way: it settles the record + // (a registration supersedes the retirement), or the next pass retries. + if (!lock.tryLock()) return + try { + val record = recordOf(found.serverUrl, found.deviceId) ?: return + val retirement = record.retirement ?: return + if (record.generation != retirement.generation || record.intent.registration != null) { + cancelRetirement(record, "superseded") + return + } + sendRetirement(record) + } catch (cancelled: CancellationException) { + throw cancelled + } catch (_: Exception) { + // Unreadable records: nothing was settled, so the entry stays queued. + } finally { + lock.unlock() + } + } + + private enum class RetirementOutcome { RETIRED, DROPPED, UNSUPPORTED, KEPT } + private class SentRetirement(val outcome: RetirementOutcome, val result: ApiResult) + private class RetirementFailure(val count: Int, val atMs: Long) + /** Per server URL, in memory: failed retirements in a row and when the last one ran. A restart starts over. */ + private val retirementFailures = ConcurrentHashMap() + private val clock: () -> Long = System::currentTimeMillis + + /** + * Sends [record]'s queued retirement; the caller holds that server's lock. + * + * - 200 naming the generation: the server holds no registration for this + * device at or below it. The removal is confirmed and the entry dropped. + * - 403 (another key), 409 (a newer generation: the server was registered + * again), or another refusal of the request itself: dropped, as the same + * request gets the same answer. + * - 404 or 405: a server without the route. Kept, and dropped once queued + * [RETIREMENT_UNSUPPORTED_LIFETIME_MS] ago. + * - No answer, 5xx, 429 and the rest: kept for a later attempt after a + * back-off, and dropped unsent once queued [RETIREMENT_LIFETIME_MS] ago. + */ + private suspend fun sendRetirement(record: PushInstallation): SentRetirement { + val retirement = checkNotNull(record.retirement) + val age = clock() - retirement.queuedAtMs + if (age >= RETIREMENT_LIFETIME_MS) { + return settleRetirement(record, RetirementOutcome.DROPPED, "expired", failure("push_retirement_expired")) + } + val result = repository.retireDevice(record.serverUrl, record.deviceId, record.key, retirement.generation) + val (outcome, reason) = when (result) { + is ApiResult.Success -> RetirementOutcome.RETIRED to null + is ApiResult.Error -> when { + result.code in RETIREMENT_REFUSALS -> RetirementOutcome.DROPPED to "http_${result.code}" + result.code in RETIREMENT_UNSUPPORTED && age >= RETIREMENT_UNSUPPORTED_LIFETIME_MS -> + RetirementOutcome.DROPPED to "unsupported_expired" + result.code in RETIREMENT_UNSUPPORTED -> RetirementOutcome.UNSUPPORTED to "http_${result.code}" + // A 200 that does not name this generation: sending it again changes nothing. + result.code == 0 && result.error == "invalid_response" -> RetirementOutcome.DROPPED to "invalid_receipt" + else -> RetirementOutcome.KEPT to (if (result.code == 0) result.error else "http_${result.code}") + } + is ApiResult.NetworkError -> RetirementOutcome.KEPT to "network" + } + return settleRetirement(record, outcome, reason, result) + } + + private suspend fun settleRetirement(record: PushInstallation, outcome: RetirementOutcome, reason: String?, + result: ApiResult): SentRetirement { + when (outcome) { + RetirementOutcome.RETIRED, RetirementOutcome.DROPPED -> { + retirementFailures.remove(record.serverUrl) + updateRecord(record.serverUrl, record.deviceId) { stored -> + stored?.takeIf { it.generation == record.generation && it.key == record.key && it.retirement != null }?.let { + if (outcome == RetirementOutcome.RETIRED) { + it.copy(acknowledged = true, receipt = null, ownerLeaving = false, retirement = null) + } else it.copy(retirement = null) + } ?: stored + } + } + RetirementOutcome.UNSUPPORTED, RetirementOutcome.KEPT -> retirementFailures.compute(record.serverUrl) { _, failed -> + RetirementFailure((failed?.count ?: 0) + 1, clock()) + } + } + recordRetirement(outcome.name.lowercase(), reason, record.generation) + return SentRetirement(outcome, result) + } + + /** Drops [record]'s queued retirement while the store still holds it at that generation. */ + private suspend fun cancelRetirement(record: PushInstallation, reason: String) { + if (record.retirement == null) return + retirementFailures.remove(record.serverUrl) + updateRecord(record.serverUrl, record.deviceId) { stored -> + stored?.takeIf { it.generation == record.generation && it.retirement != null }?.copy(retirement = null) ?: stored + } + recordRetirement("cancelled", reason, record.generation) + } + + /** One breadcrumb per outcome, so a report can say why a removed server's pushes kept arriving. */ + private fun recordRetirement(outcome: String, reason: String?, generation: Long) { + val attributes = buildMap { + put("outcome", SiloLogAttribute.Text(outcome)) + reason?.let { put("reason", SiloLogAttribute.Text(it)) } + } + SiloLog.breadcrumb(DiagnosticsLogCategory.LIFECYCLE, "PushRetire", "push retirement $outcome at generation $generation", attributes) + } + /** * A registration, or a removal not yet confirmed, which is then sent again * exactly as persisted: one cut off before it reached the server must not @@ -317,8 +505,13 @@ class AndroidPushRegistrar internal constructor( if (!current(owner)) return failure("push_authority_changed") // Allocation reaches durable storage before any mutation can leave the process. // A login marked leaving meanwhile stays marked on the intent it allocates next. - if (entry !== previous) updateRecord(serverUrl, device) { stored -> - entry.copy(ownerLeaving = stored?.ownerLeaving == true && stored.intent.loginId == entry.intent.loginId) + if (entry !== previous) { + updateRecord(serverUrl, device) { stored -> + entry.copy(ownerLeaving = stored?.ownerLeaving == true && stored.intent.loginId == entry.intent.loginId) + } + // The server, added or signed in to again, orders this generation above + // the pending retirement, which the record no longer holds. + if (previous?.retirement != null) recordRetirement("cancelled", "registered", previous.generation) } currentCoroutineContext().ensureActive() if (!current(owner)) return failure("push_authority_changed") @@ -331,7 +524,7 @@ class AndroidPushRegistrar internal constructor( stored?.takeIf { it.generation == entry.generation && it.key == entry.key }?.let(outcome) ?: stored } when (result) { - is ApiResult.Success -> settle { it.copy(acknowledged = true, receipt = result.data, ownerLeaving = false) } + is ApiResult.Success -> settle { it.copy(acknowledged = true, receipt = result.data, ownerLeaving = false, retirement = null) } is ApiResult.Error -> if (result.code in setOf(401, 403, 409, 422)) { // No generation rebase or replacement secret after a definite refusal. settle { it.copy(refusal = "push_registration_refused") } @@ -353,6 +546,21 @@ class AndroidPushRegistrar internal constructor( private companion object { const val UNREGISTER_BUDGET_MS = 8_000L + const val DAY_MS = 24 * 60 * 60 * 1000L + /** The contract's limit for a server without the retirement route. */ + const val RETIREMENT_UNSUPPORTED_LIFETIME_MS = 30 * DAY_MS + /** A server that never answers is not tried forever. */ + const val RETIREMENT_LIFETIME_MS = 90 * DAY_MS + const val RETIREMENT_CONCURRENCY = 3 + /** The first retry waits this long, doubling per failure up to [RETIREMENT_MAX_BACK_OFF_MS]. */ + const val RETIREMENT_INITIAL_BACK_OFF_MS = 30_000L + const val RETIREMENT_MAX_BACK_OFF_MS = 6 * 60 * 60 * 1000L + /** A push skips the back-off, but not this. */ + const val RETIREMENT_MIN_RETRY_MS = 30_000L + val RETIREMENT_REFUSALS = setOf(400, 403, 409, 413, 415, 422) + val RETIREMENT_UNSUPPORTED = setOf(404, 405) + fun retirementBackOffMs(failures: Int): Long = + (RETIREMENT_INITIAL_BACK_OFF_MS shl (failures - 1).coerceIn(0, 20)).coerceAtMost(RETIREMENT_MAX_BACK_OFF_MS) } } diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrationStarter.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrationStarter.kt index 1c1e6e282b..8d9f3a7f2f 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrationStarter.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrationStarter.kt @@ -24,6 +24,9 @@ class AndroidPushRegistrationStarter( if (scope != null) return val nextScope = CoroutineScope(SupervisorJob() + Dispatchers.IO) scope = nextScope + // A removed or signed-out server keeps pushing until its retirement lands. + // It needs no login, so it goes out on app start and every foreground. + nextScope.launch { runCatching { registrar.retryPendingRetirements() } } nextScope.registerOnce() nextScope.launch { notificationsRepository.resetSignals.collect { diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/PushInstallationStore.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/PushInstallationStore.kt index e14d1a82d9..bd9efaf945 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/PushInstallationStore.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/PushInstallationStore.kt @@ -28,8 +28,19 @@ internal data class PushInstallation( * registration supersedes it instead. */ val ownerLeaving: Boolean = false, + /** + * The removal intent at this record's generation still has to reach the + * server as a retirement, which needs only [key]: it is sent again after + * the login that registered is gone, until the server answers for good. + * Any newer generation supersedes it. + */ + val retirement: PushRetirement? = null, ) { override fun toString() = "PushInstallation()" } +/** A queued retirement of [generation], first queued at [queuedAtMs] (epoch milliseconds). */ +@Serializable +internal data class PushRetirement(val generation: Long, val queuedAtMs: Long) + @Serializable internal data class PushInstallationIntent( val serverId: String, diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/SiloFirebaseMessagingService.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/SiloFirebaseMessagingService.kt index ecc59b8f48..a678ec9939 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/SiloFirebaseMessagingService.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/SiloFirebaseMessagingService.kt @@ -3,6 +3,7 @@ package org.siloserver.silo.android.push import com.google.firebase.messaging.FirebaseMessagingService import com.google.firebase.messaging.RemoteMessage import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withTimeoutOrNull import org.koin.java.KoinJavaComponent.get @@ -116,18 +117,24 @@ class SiloFirebaseMessagingService : FirebaseMessagingService() { override fun onMessageReceived(message: RemoteMessage) { val handler = runCatching { get(PushMessageHandler::class.java) }.getOrNull() - ?: return - // Budgeted inside the presenter rather than here, so a stalled server - // still ends in a posted notification rather than none at all. The catch - // is a boundary net: nothing may escape into the Firebase SDK. + val registrar = runCatching { get(AndroidPushRegistrar::class.java) }.getOrNull() runBlocking { - runCatching { - handler.handle( - data = message.data, - fallbackTitle = message.notification?.title, - fallbackBody = message.notification?.body, - ) + // The push may come from a server this device retired: its pending + // retirement goes out again beside the delivery, inside the same budget. + val retirements = registrar?.let { launch { withinPushBudget { it.retryPendingRetirements(afterPush = true) } } } + // Budgeted inside the presenter rather than here, so a stalled server + // still ends in a posted notification rather than none at all. The catch + // is a boundary net: nothing may escape into the Firebase SDK. + handler?.let { + runCatching { + it.handle( + data = message.data, + fallbackTitle = message.notification?.title, + fallbackBody = message.notification?.body, + ) + } } + retirements?.join() } } diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/servers/ServerListViewModel.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/servers/ServerListViewModel.kt index 318826e11e..a3d7f07e29 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/servers/ServerListViewModel.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/servers/ServerListViewModel.kt @@ -124,9 +124,11 @@ class ServerListViewModel( fun onRemove(serverId: String) { viewModelScope.launch { val wasActive = serverRegistry.activeServerId.value == serverId - // While the server's credentials still exist: removal wipes them. - // Failing to remove the registration never stops the removal; - // cancellation still does, before anything is removed. + // Before removal wipes the server's credentials, which a server + // without the retirement route needs; a retirement it has not + // answered is sent again later without them. Failing to remove the + // registration never stops the removal; cancellation still does, + // before anything is removed. var removal: PushRemoval? = null tolerateFailure { removal = unregisterPush(serverId) } try { diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt index a67495dbf5..48445b94e5 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt @@ -1,13 +1,20 @@ package org.siloserver.silo.android.push +import android.app.Application import kotlinx.coroutines.* import kotlinx.coroutines.test.runTest +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config import org.siloserver.silo.model.notifications.* import org.siloserver.silo.network.* import org.siloserver.silo.network.api.PushRegistrationApi import org.siloserver.silo.repository.PushRegistrationRepository import kotlin.test.* +// Robolectric for the diagnostics log lines the registrar writes. +@RunWith(RobolectricTestRunner::class) +@Config(application=Application::class, sdk=[28]) class AndroidPushRegistrarTest { private var owner = DurableLoginAuthority("login", AuthScopeSnapshot("server","profile","https://example.invalid","pin",identityGeneration=1)) private val token = "t".repeat(80) @@ -44,6 +51,10 @@ class AndroidPushRegistrarTest { private var unreachableUrl: String?=null private var available=true private var onAvailable: suspend (AuthScopeSnapshot) -> Unit = {} + /** The retirement route's answer: 200 retires, null is no answer. A server without the route by default. */ + private var retireStatus: Int? = 404 + /** Each retirement sent: server URL, key and generation. It carries no login at all. */ + private val retirements=mutableListOf>() private val api=object : PushRegistrationApi { override suspend fun available(owner: AuthScopeSnapshot): ApiResult { onAvailable(owner); return ApiResult.Success(available) } override suspend fun register(request: PushDeviceRegisterRequest,key:String,generation:Long,owner:AuthScopeSnapshot):ApiResult { @@ -61,6 +72,17 @@ class AndroidPushRegistrarTest { if(serverProfiles[owner.serverUrl].let { it!=null && it!=owner.profileId }) return ApiResult.Error(409,"conflict","") return if(uncertain) ApiResult.NetworkError(IllegalStateException("lost reply")) else ApiResult.Success(Unit) } + override suspend fun retire(serverUrl:String,deviceId:String,key:String,generation:Long):ApiResult { + val record=store.entries.single { it.serverUrl==serverUrl } + assertEquals(generation,record.retirement?.generation) // durable before dispatch + assertNull(record.intent.registration); assertEquals("device",deviceId) + retirements+=Triple(serverUrl,key,generation) + return when(val status=retireStatus) { + null -> ApiResult.NetworkError(IllegalStateException("no answer")) + 200 -> ApiResult.Success(Unit) + else -> ApiResult.Error(status,"","") + } + } } private fun registrar(provider: AndroidPushTokenProvider=object:AndroidPushTokenProvider { override suspend fun token()=token }) = AndroidPushRegistrar(provider,PushRegistrationRepository(api),{"device"},authorities,store,{proof}) @@ -147,7 +169,8 @@ class AndroidPushRegistrarTest { available=true owner=owner.copy(loginId="next",scope=owner.scope.copy(identityGeneration=2)) assertIs>(r.registerToken(token)) - assertEquals(listOf(1L,2L),generations); assertEquals("next",store.entries.single().intent.loginId) + // The removal took generation 2 before anything was sent, so the next login registers above it. + assertEquals(listOf(1L,3L),generations); assertEquals("next",store.entries.single().intent.loginId) } @Test fun theNextForegroundGivesOtherServersARotatedTokenTheyMissed()=runTest { val r=registrar(); r.registerToken(token) @@ -278,6 +301,69 @@ class AndroidPushRegistrarTest { savedLogins.remove("server") // the sign-out went through after all assertIs(r.restoreRegistration(removal)); assertNull(store.entries.single().intent.registration) } + @Test fun aRemovalIsRetiredWithoutALoginAtTheGenerationItPersisted()=runTest { + val r=registrar(); r.registerToken(token) + retireStatus=200 + savedLogins.remove("server") // no login can act for that server any more + assertIs>(r.unregisterDevice("server").result) + assertEquals(listOf(Triple("https://example.invalid",proof,2L)),retirements) + assertEquals(listOf(1L),generations) // no session request + val record=store.entries.single() + assertNull(record.intent.registration); assertTrue(record.acknowledged); assertNull(record.retirement) + } + @Test fun aRemovalTheSessionCouldNotSendIsRetiredAfterARestartAtTheSameKeyAndGeneration()=runTest { + registrar().registerToken(token) + // A server without the retirement route, whose session removal gets no answer. + uncertain=true + assertFalse(registrar().unregisterDevice("server").result is ApiResult.Success) + assertEquals(listOf(1L,2L),generations) + savedLogins.remove("server") // the sign-out goes ahead and clears the login + // After a restart, the updated server answers. + uncertain=false; retireStatus=200 + registrar().retryPendingRetirements() + assertEquals(listOf(2L,2L),retirements.map { it.third }); assertEquals(setOf(proof),retirements.map { it.second }.toSet()) + assertEquals(listOf(1L,2L),generations) + val record=store.entries.single(); assertTrue(record.acknowledged); assertNull(record.retirement) + registrar().retryPendingRetirements(); assertEquals(2,retirements.size) + } + @Test fun onlyAFinalAnswerDropsAQueuedRetirement()=runTest { + val day=24*60*60*1000L + // status to whether the entry is queued 31 days ago, and whether it is then dropped. + val cases=listOf(Triple(200,false,true),Triple(403,false,true),Triple(409,false,true),Triple(503,false,false), + Triple(null,false,false),Triple(404,false,false),Triple(404,true,true)) + for ((status,aged,dropped) in cases) { + store.entries=emptyList(); savedLogins["server"]="login" + retireStatus=503; registrar().registerToken(token); registrar().unregisterDevice("server") + savedLogins.remove("server") + if (aged) store.entries=store.entries.map { it.copy(retirement=it.retirement!!.copy(queuedAtMs=it.retirement!!.queuedAtMs-31*day)) } + retireStatus=status; val sent=retirements.size + registrar().retryPendingRetirements() + assertEquals(sent+1,retirements.size,"$status") + val record=store.entries.single() + assertEquals(dropped,record.retirement==null,"$status aged=$aged") + assertEquals(status==200,record.acknowledged,"$status") + assertNull(record.intent.registration) + } + } + @Test fun signingInAgainOrARestoreDropsTheRetirementAndRegistersAboveIt()=runTest { + val r=registrar(); r.registerToken(token) + retireStatus=null + assertFalse(r.unregisterDevice("server").result is ApiResult.Success) + assertEquals(2L,store.entries.single().retirement?.generation) + owner=owner.copy(loginId="next",scope=owner.scope.copy(identityGeneration=2)); savedLogins["server"]="next" + assertIs>(r.registerToken(token)) + assertEquals(listOf(1L,3L),generations) + assertNull(store.entries.single().retirement) + registrar().retryPendingRetirements(); assertEquals(1,retirements.size) + + // A sign-out that stops undoes its removal, retirement included. + val removal=r.unregisterDevice("server") + assertEquals(4L,store.entries.single().retirement?.generation) + assertIs>(r.restoreRegistration(removal)) + assertEquals(listOf(1L,3L,5L),generations) + val record=store.entries.single(); assertNull(record.retirement); assertTrue(record.acknowledged) + registrar().retryPendingRetirements(); assertEquals(2,retirements.size) + } @Test fun conflictStaysDurableAndNeverRotatesProofOrGeneration()=runTest { failCode=409; registrar().registerToken(token) failCode=null diff --git a/docs/api-v2/android-push-registration.md b/docs/api-v2/android-push-registration.md index 0ad5c0fe20..dc6a1cdd3a 100644 --- a/docs/api-v2/android-push-registration.md +++ b/docs/api-v2/android-push-registration.md @@ -3,8 +3,8 @@ The phone's existing foreground/reset registration and Firebase token callback use `ordered_android_v1`. Registration capability describes storage support, not provider availability or successful delivery. TV has no FCM registration caller. -The registrar's existing removal method uses the ordered DELETE contract, but no -production invocation of that method or new removal UI is claimed. +Signing out of a server and removing a saved server remove this device's +registration there first, as described below. The registrar captures durable login authority before requesting a Firebase token. A later callback supersedes an older token lookup. A mutex serializes @@ -25,6 +25,22 @@ restart. If the token or verb changed, at most one exact pending replay precedes the new intent; allocation occurs only after acknowledgment. Replacement authority cannot replay the original intent or publish its late result. +Removal allocates the next generation as a removal intent and records it, in the +same write, as a pending retirement (generation and time queued) before anything +is sent; a removal not yet confirmed keeps its generation. The retirement, +`POST /api/v2/notifications/push/devices/{device_id}/retire` +(`retirePushDevice`), carries only the installation key and generation headers, +with no session, so it still works after sign-out or removal has cleared the +login. It is sent first. Only a server that answers 404 or 405 gets the session +DELETE instead, at the same key and generation, while the registering login +still exists. A retirement without a final answer stays queued and is sent again +on app start, on every foreground and when an FCM message arrives, at most three +at a time with an in-memory back-off. 200 naming the generation confirms the +removal; 403, 409 and other refusals of the request drop the entry; 404 or 405 +keeps it for 30 days from queueing; anything else keeps it for up to 90 days. +Signing in again, re-adding the server or undoing a sign-out that stopped +registers at a higher generation, which drops the pending retirement. + Conflicts and definite authority/validation refusals persist a stopped state. There is no automatic generation rebase, secret reset, legacy fallback, counter exhaustion recovery, or recovery UI. Corrupt storage or a missing file with an diff --git a/shared/src/commonMain/kotlin/org/siloserver/silo/network/api/PushRegistrationApi.kt b/shared/src/commonMain/kotlin/org/siloserver/silo/network/api/PushRegistrationApi.kt index c490d2cc6b..5fdab9486d 100644 --- a/shared/src/commonMain/kotlin/org/siloserver/silo/network/api/PushRegistrationApi.kt +++ b/shared/src/commonMain/kotlin/org/siloserver/silo/network/api/PushRegistrationApi.kt @@ -12,6 +12,13 @@ interface PushRegistrationApi { suspend fun available(owner: AuthScopeSnapshot): ApiResult suspend fun register(request: PushDeviceRegisterRequest, key: String, generation: Long, owner: AuthScopeSnapshot): ApiResult suspend fun delete(deviceId: String, key: String, generation: Long, owner: AuthScopeSnapshot): ApiResult + /** + * `retirePushDevice`: tells [serverUrl] to drop this device's registration + * at or below [generation], proven by the installation [key] alone. No + * session, so it still works once the server's login is gone. Success is + * 200 naming [generation], whether or not the server held a registration. + */ + suspend fun retire(serverUrl: String, deviceId: String, key: String, generation: Long): ApiResult } class DefaultPushRegistrationApi( @@ -67,5 +74,17 @@ class DefaultPushRegistrationApi( } }) { } } + override suspend fun retire(serverUrl: String, deviceId: String, key: String, generation: Long): ApiResult = + ownedV2Call(ApiV2Gate.Unrestricted, null, null, OwnerPolicy.FULL, HttpStatusCode.OK, { + client.post("${serverUrl.trimEnd('/')}$DEVICES_PATH/${deviceId.encodeURLPathPart()}/retire") { + // The key proves the device; no credentials of any login go with it. + skipSiloAuth(); singleAttempt() + header("X-Push-Installation-Key", key); header("X-Push-Generation", generation.toString()) + } + }) { body -> + require(body["generation"] == JsonPrimitive(generation.toString()) && body["removed"] == JsonPrimitive(true)) { + "Push retirement acknowledgment is invalid." + } + } companion object { const val DEVICES_PATH = "/api/v2/notifications/push/devices" } } diff --git a/shared/src/commonMain/kotlin/org/siloserver/silo/repository/PushRegistrationRepository.kt b/shared/src/commonMain/kotlin/org/siloserver/silo/repository/PushRegistrationRepository.kt index 7f7568aacb..00634df6ee 100644 --- a/shared/src/commonMain/kotlin/org/siloserver/silo/repository/PushRegistrationRepository.kt +++ b/shared/src/commonMain/kotlin/org/siloserver/silo/repository/PushRegistrationRepository.kt @@ -11,4 +11,6 @@ class PushRegistrationRepository(private val api: PushRegistrationApi) { api.register(request, key, generation, owner) suspend fun unregisterDevice(deviceId: String, key: String, generation: Long, owner: AuthScopeSnapshot) = api.delete(deviceId, key, generation, owner) + suspend fun retireDevice(serverUrl: String, deviceId: String, key: String, generation: Long) = + api.retire(serverUrl, deviceId, key, generation) } diff --git a/shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PushRegistrationApiTest.kt b/shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PushRegistrationApiTest.kt index ec07633f0d..6b343d6696 100644 --- a/shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PushRegistrationApiTest.kt +++ b/shared/src/commonTest/kotlin/org/siloserver/silo/network/api/PushRegistrationApiTest.kt @@ -91,6 +91,31 @@ class PushRegistrationApiTest { assertIs(api.register(request,key,9007199254740993,captured)); assertEquals(2,sends) } finally { client.close() } } + @Test fun retirementCarriesOnlyTheKeyAndGenerationAndRequiresAReceiptForThatGeneration() = runTest { + var body = """{"generation":"7","removed":true}""" + var status = HttpStatusCode.OK + val client = HttpClient(MockEngine { + assertEquals(HttpMethod.Post, it.method) + assertEquals("https://cabin.invalid/api/v2/notifications/push/devices/device%2Fone/retire", it.url.toString()) + assertEquals(key, it.headers["X-Push-Installation-Key"]); assertEquals("7", it.headers["X-Push-Generation"]) + // No login of any kind: the auth plugin attaches no credentials to it. + assertTrue(it.attributes[SkipSiloAuthAttributeKey]); assertNull(it.attributes.getOrNull(AuthScopeAttributeKey)) + assertTrue(it.attributes[SingleAttemptAttributeKey]) + respond(body, status, headersOf(HttpHeaders.ContentType, "application/json")) + }) { install(ContentNegotiation) { json(SiloJson) } } + try { + val api = DefaultPushRegistrationApi(client, tokens, ApiV2Gate.Unrestricted) + suspend fun retire() = api.retire("https://cabin.invalid/", "device/one", key, 7) + assertIs>(retire()) + for (bad in listOf("""{"generation":"6","removed":true}""", """{"generation":7,"removed":true}""", """{"generation":"7","removed":false}""", "{}")) { + body = bad; assertEquals("invalid_response", assertIs(retire()).error, bad) + } + body = "{}" + for (refused in listOf(HttpStatusCode.NotFound, HttpStatusCode.Forbidden, HttpStatusCode.Conflict)) { + status = refused; assertEquals(refused.value, assertIs(retire()).code) + } + } finally { client.close() } + } @Test fun capabilityRequiresOrderedRevisionPlatformAndStorage() = runTest { var body="""{"revision":"4bc701e0","state":"available","allowed":true,"registration_available":true,"platforms":["android"]}""" val client=HttpClient(MockEngine { From 252294042d13a9ee1e0e3ee5dbcfcbabfe489c57 Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Wed, 21 Oct 2026 23:31:07 +0000 Subject: [PATCH 2/4] fix(push): retire removals that never queued a retirement A removal that ran out of time waiting on a registration holding the server's lock persisted only ownerLeaving, and one made by the previous release has no retirement field at all. Neither was ever retried, so the server kept pushing once the login was cleared. Retries now queue and send a retirement for such a record once its login is gone, at the generation a removal nobody confirmed already holds. A retry that runs out of time now counts as a failure, so pushes respect the back-off. Every retry pass shares one three-permit limit. Restore rechecks that it is still the newest operation under the server's lock before it drops a retirement, so it can't cancel one a newer removal queued. Co-Authored-By: Claude Opus 5.5 --- .../silo/android/push/AndroidPushRegistrar.kt | 38 ++++++++++++++----- .../android/push/AndroidPushRegistrarTest.kt | 38 ++++++++++++++++++- docs/api-v2/android-push-registration.md | 5 ++- 3 files changed, 70 insertions(+), 11 deletions(-) diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt index be984e2f25..9244e1d425 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt @@ -227,6 +227,8 @@ class AndroidPushRegistrar internal constructor( val record = recordOf(found.serverUrl, device)?.takeIf { it.intent.serverId == serverId && it.refusal == null } ?: return@withLock failure("push_not_registered") val owner = registeredOwnerOf(record) ?: return@withLock failure("push_authority_changed") + // A removal started while this waited owns the retirement now. + if (latestRequests[serverId] !== attempt) return@withLock failure("push_request_superseded") // The server stays saved and signed in, so its retirement must not go out. cancelRetirement(record, "restored") val token = tokenProvider.token()?.trim()?.takeIf { it.length in 64..512 } @@ -293,14 +295,15 @@ class AndroidPushRegistrar internal constructor( /** * Sends the retirements still queued for removed or signed-out servers whose - * back-off has passed, a few at a time. They need no login, so this runs on - * app start and foreground, before anyone signs in, and when a push arrives: + * back-off has passed, a few at a time, and queues one for a removal that + * never got that far once its login is gone. They need no login, so this runs + * on app start and foreground, before anyone signs in, and when a push arrives: * [afterPush] skips the back-off, as a push from a retired server is the sign * it is still sending them. Never throws. */ suspend fun retryPendingRetirements(afterPush: Boolean = false) { val pending = try { - installations().filter { it.retirement != null } + installations().filter { it.retirement != null || it.unqueuedRemoval } } catch (cancelled: CancellationException) { throw cancelled } catch (_: Exception) { @@ -312,10 +315,12 @@ class AndroidPushRegistrar internal constructor( at - failed.atMs >= if (afterPush) RETIREMENT_MIN_RETRY_MS else retirementBackOffMs(failed.count) } if (due.isEmpty()) return - val permits = Semaphore(RETIREMENT_CONCURRENCY) coroutineScope { for (record in due) launch { - permits.withPermit { withTimeoutOrNull(PUSH_WORK_BUDGET_MS) { retryRetirement(record) } } + retirementPermits.withPermit { + // Out of time is a failure like any other, so the next push waits out the back-off. + withTimeoutOrNull(PUSH_WORK_BUDGET_MS) { retryRetirement(record) } ?: noteRetirementFailure(record.serverUrl) + } } } } @@ -327,7 +332,14 @@ class AndroidPushRegistrar internal constructor( if (!lock.tryLock()) return try { val record = recordOf(found.serverUrl, found.deviceId) ?: return - val retirement = record.retirement ?: return + val retirement = record.retirement + if (retirement == null) { + // A removal that ran out of time before queueing its retirement, or one + // an older version could not send: with the login gone, nothing else will. + if (!record.unqueuedRemoval || registeredOwnerOf(record) != null) return + queueRetirement(record.serverUrl, record.deviceId, record.intent.serverId)?.let { sendRetirement(it) } + return + } if (record.generation != retirement.generation || record.intent.registration != null) { cancelRetirement(record, "superseded") return @@ -347,6 +359,8 @@ class AndroidPushRegistrar internal constructor( private class RetirementFailure(val count: Int, val atMs: Long) /** Per server URL, in memory: failed retirements in a row and when the last one ran. A restart starts over. */ private val retirementFailures = ConcurrentHashMap() + /** Shared by every pass, so a foreground and a push retrying at once still send only a few. */ + private val retirementPermits = Semaphore(RETIREMENT_CONCURRENCY) private val clock: () -> Long = System::currentTimeMillis /** @@ -398,14 +412,16 @@ class AndroidPushRegistrar internal constructor( } ?: stored } } - RetirementOutcome.UNSUPPORTED, RetirementOutcome.KEPT -> retirementFailures.compute(record.serverUrl) { _, failed -> - RetirementFailure((failed?.count ?: 0) + 1, clock()) - } + RetirementOutcome.UNSUPPORTED, RetirementOutcome.KEPT -> noteRetirementFailure(record.serverUrl) } recordRetirement(outcome.name.lowercase(), reason, record.generation) return SentRetirement(outcome, result) } + private fun noteRetirementFailure(serverUrl: String) { + retirementFailures.compute(serverUrl) { _, failed -> RetirementFailure((failed?.count ?: 0) + 1, clock()) } + } + /** Drops [record]'s queued retirement while the store still holds it at that generation. */ private suspend fun cancelRetirement(record: PushInstallation, reason: String) { if (record.retirement == null) return @@ -433,6 +449,10 @@ class AndroidPushRegistrar internal constructor( private val PushInstallation.removable: Boolean get() = refusal == null && (intent.registration != null || !acknowledged) + /** A removal began here ([markOwnerLeaving]) but queued no retirement. */ + private val PushInstallation.unqueuedRemoval: Boolean + get() = ownerLeaving && retirement == null && removable + /** * Marks [serverUrl]'s record as belonging to a login on its way out, so the * next registration may supersede whatever it leaves uncertain: once that diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt index 48445b94e5..5827adfbe4 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt @@ -2,6 +2,7 @@ package org.siloserver.silo.android.push import android.app.Application import kotlinx.coroutines.* +import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner @@ -55,6 +56,7 @@ class AndroidPushRegistrarTest { private var retireStatus: Int? = 404 /** Each retirement sent: server URL, key and generation. It carries no login at all. */ private val retirements=mutableListOf>() + private var onRetire: suspend () -> Unit = {} private val api=object : PushRegistrationApi { override suspend fun available(owner: AuthScopeSnapshot): ApiResult { onAvailable(owner); return ApiResult.Success(available) } override suspend fun register(request: PushDeviceRegisterRequest,key:String,generation:Long,owner:AuthScopeSnapshot):ApiResult { @@ -76,7 +78,7 @@ class AndroidPushRegistrarTest { val record=store.entries.single { it.serverUrl==serverUrl } assertEquals(generation,record.retirement?.generation) // durable before dispatch assertNull(record.intent.registration); assertEquals("device",deviceId) - retirements+=Triple(serverUrl,key,generation) + retirements+=Triple(serverUrl,key,generation); onRetire() return when(val status=retireStatus) { null -> ApiResult.NetworkError(IllegalStateException("no answer")) 200 -> ApiResult.Success(Unit) @@ -364,6 +366,40 @@ class AndroidPushRegistrarTest { val record=store.entries.single(); assertNull(record.retirement); assertTrue(record.acknowledged) registrar().retryPendingRetirements(); assertEquals(2,retirements.size) } + @Test fun aRemovalThatRanOutOfTimeWaitingOnARegistrationIsRetiredOnceTheLoginIsGone()=runTest { + val r=registrar(); r.registerToken(token) + // A registration holds the server's lock past the removal's budget. + val gate=CompletableDeferred(); onAvailable={ gate.await() } + val registering=launch { r.registerToken(token) }; runCurrent() + assertFalse(r.unregisterDevice("server").result is ApiResult.Success) + assertNull(store.entries.single().retirement) + savedLogins.remove("server") // the sign-out goes ahead and clears the login + onAvailable={}; gate.complete(Unit); registering.join() + retireStatus=200 + registrar().retryPendingRetirements() + assertEquals(listOf(Triple("https://example.invalid",proof,2L)),retirements) + val record=store.entries.single(); assertNull(record.intent.registration); assertTrue(record.acknowledged); assertNull(record.retirement) + } + @Test fun aRemovalAnOlderVersionCouldNotSendIsRetiredAtItsOwnGeneration()=runTest { + registrar().registerToken(token) + // What a failed session removal left behind before retirements existed. + store.entries=store.entries.map { it.copy(generation=2, intent=it.intent.copy(registration=null), acknowledged=false, receipt=null, ownerLeaving=true) } + savedLogins.remove("server") + retireStatus=200 + registrar().retryPendingRetirements() + assertEquals(listOf(Triple("https://example.invalid",proof,2L)),retirements) + assertTrue(store.entries.single().acknowledged) + } + @Test fun aRetirementThatStallsWaitsOutTheBackOffLikeOneThatFailed()=runTest { + registrar().registerToken(token) + retireStatus=null; registrar().unregisterDevice("server"); savedLogins.remove("server") + val r=registrar() // a restart: no failures remembered + onRetire={ awaitCancellation() } + r.retryPendingRetirements(afterPush=true) + r.retryPendingRetirements(afterPush=true) + assertEquals(2,retirements.size) // the removal's and the stalled retry's; the next push waits + assertEquals(2L,store.entries.single().retirement?.generation) + } @Test fun conflictStaysDurableAndNeverRotatesProofOrGeneration()=runTest { failCode=409; registrar().registerToken(token) failCode=null diff --git a/docs/api-v2/android-push-registration.md b/docs/api-v2/android-push-registration.md index dc6a1cdd3a..f12a4699fe 100644 --- a/docs/api-v2/android-push-registration.md +++ b/docs/api-v2/android-push-registration.md @@ -35,7 +35,10 @@ login. It is sent first. Only a server that answers 404 or 405 gets the session DELETE instead, at the same key and generation, while the registering login still exists. A retirement without a final answer stays queued and is sent again on app start, on every foreground and when an FCM message arrives, at most three -at a time with an in-memory back-off. 200 naming the generation confirms the +at a time with an in-memory back-off; an attempt that runs out of time counts as +a failure. A removal that never queued its retirement, because it ran out of +time waiting on that server or an older version made it, is queued then, once +its login is gone. 200 naming the generation confirms the removal; 403, 409 and other refusals of the request drop the entry; 404 or 405 keeps it for 30 days from queueing; anything else keeps it for up to 90 days. Signing in again, re-adding the server or undoing a sign-out that stopped From 6ca97f84d33657459cfc605d59061659dcd704e0 Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Thu, 22 Oct 2026 00:04:09 +0000 Subject: [PATCH 3/4] fix(push): don't queue again a retirement the server refused A dropped retirement (refused, or expired unsent) cleared only its retirement and left ownerLeaving set on an unconfirmed removal, so the next retry took it for a removal that never queued one, sent it again, and restarted its lifetime. Dropping now clears ownerLeaving too; a later registration still supersedes the removal, as it holds no registration. Co-Authored-By: Claude Opus 5.5 --- .../silo/android/push/AndroidPushRegistrar.kt | 5 ++++- .../android/push/AndroidPushRegistrarTest.kt | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt index 9244e1d425..90a3fd6d6e 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt @@ -408,7 +408,10 @@ class AndroidPushRegistrar internal constructor( stored?.takeIf { it.generation == record.generation && it.key == record.key && it.retirement != null }?.let { if (outcome == RetirementOutcome.RETIRED) { it.copy(acknowledged = true, receipt = null, ownerLeaving = false, retirement = null) - } else it.copy(retirement = null) + } else { + // Settled for good: no later retry may take it for a removal never queued. + it.copy(ownerLeaving = false, retirement = null) + } } ?: stored } } diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt index 5827adfbe4..0e22c6d438 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt @@ -390,6 +390,23 @@ class AndroidPushRegistrarTest { assertEquals(listOf(Triple("https://example.invalid",proof,2L)),retirements) assertTrue(store.entries.single().acknowledged) } + @Test fun aDroppedRetirementIsNotQueuedAgain()=runTest { + val day=24*60*60*1000L + for (aged in listOf(false,true)) { + store.entries=emptyList(); retirements.clear(); savedLogins["server"]="login" + registrar().registerToken(token) + retireStatus=if (aged) null else 403 + registrar().unregisterDevice("server"); savedLogins.remove("server") + if (aged) { + store.entries=store.entries.map { it.copy(retirement=it.retirement!!.copy(queuedAtMs=it.retirement!!.queuedAtMs-91*day)) } + registrar().retryPendingRetirements() // expires unsent + } + assertNull(store.entries.single().retirement,"aged=$aged") + val sent=retirements.size + registrar().retryPendingRetirements() + assertEquals(sent,retirements.size,"aged=$aged"); assertNull(store.entries.single().retirement,"aged=$aged") + } + } @Test fun aRetirementThatStallsWaitsOutTheBackOffLikeOneThatFailed()=runTest { registrar().registerToken(token) retireStatus=null; registrar().unregisterDevice("server"); savedLogins.remove("server") From 37ca6461fcb41788909b2803c977b9a2e96a1a2d Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Thu, 22 Oct 2026 01:01:07 +0000 Subject: [PATCH 4/4] fix(push): leave the session removal time, and send it when the retirement is unanswered The removal gave the retirement and the session removal one 8-second budget. A server without the retirement route that took 7 seconds to answer 404 left the session removal about a second, so it timed out and the login was cleared; later retries without a login only got 404 until the entry expired, while the server kept pushing. The first retirement attempt now gets at most 2 seconds. One cut off there stays queued. Only 404 or 405 fell back to the session removal. A retirement with no answer, a 429 or a 5xx now also gets it while the login still exists, at the same key and generation; unless it lands, the retirement stays queued. Co-Authored-By: Claude Opus 5.5 --- .../silo/android/push/AndroidPushRegistrar.kt | 32 +++++++++++------ .../android/push/AndroidPushRegistrarTest.kt | 34 +++++++++++++++---- docs/api-v2/android-push-registration.md | 7 ++-- 3 files changed, 52 insertions(+), 21 deletions(-) diff --git a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt index 90a3fd6d6e..91156f7964 100644 --- a/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt +++ b/androidApp/src/androidMain/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrar.kt @@ -173,10 +173,11 @@ class AndroidPushRegistrar internal constructor( * * The removal takes the record's next generation and is persisted as a * pending retirement before anything is sent. A retirement needs only the - * installation key, so it needs no login and is sent first. Only a server - * without the retirement route (404 or 405) gets the session removal instead, - * at the same key and generation, as the login and profile it was registered - * for. A retirement the server does not answer for good stays queued and + * installation key, so it needs no login and is sent first, for at most + * [RETIREMENT_FIRST_ATTEMPT_MS]. A server without the retirement route (404 or + * 405), or one that has not answered it, also gets the session removal, at the + * same key and generation, as the login and profile it was registered for. + * A retirement the server does not answer for good stays queued and * [retryPendingRetirements] sends it again after the login is gone. * * Bounded so an unreachable server cannot hold up the sign-out or removal @@ -251,13 +252,20 @@ class AndroidPushRegistrar internal constructor( // Read again under the server's lock, as a registration finishing first // changes the record: the removal is allocated over whatever it left. val retiring = queueRetirement(found.serverUrl, device, serverId) ?: return@withLock failure("push_not_registered") - val retired = sendRetirement(retiring) - if (retired.outcome != RetirementOutcome.UNSUPPORTED) return@withLock retired.result - // A server without the retirement route takes only the session removal, - // at the same generation, from the profile that registered: it refuses - // one from any other profile, for good. Unless that removal lands, the - // retirement stays queued for when the server is updated. - val owner = registeredOwnerOf(retiring) ?: return@withLock retired.result + // Capped, so a slow server still leaves the session removal time to run + // before the credentials are cleared. One cut off here stays queued. + val retired = withTimeoutOrNull(RETIREMENT_FIRST_ATTEMPT_MS) { sendRetirement(retiring) } + if (retired == null) noteRetirementFailure(retiring.serverUrl) + if (retired != null && retired.outcome in setOf(RetirementOutcome.RETIRED, RetirementOutcome.DROPPED)) { + return@withLock retired.result + } + // A server without the retirement route, or one that did not answer it, + // gets the session removal while the login still exists, at the same + // generation, from the profile that registered: the server refuses one + // from any other profile, for good. Unless that removal lands, the + // retirement stays queued and is sent again after the login is gone. + val unanswered = retired?.result ?: failure("push_retirement_unanswered") + val owner = registeredOwnerOf(retiring) ?: return@withLock unanswered val removed = applyOnServer(owner, null, attempt, mayReconcile = false).map { Unit } if (removed is ApiResult.Success) recordRetirement("cancelled", "session_removed", retiring.generation) removed @@ -569,6 +577,8 @@ class AndroidPushRegistrar internal constructor( private companion object { const val UNREGISTER_BUDGET_MS = 8_000L + /** The removal's first retirement attempt, leaving the session removal most of [UNREGISTER_BUDGET_MS]. */ + const val RETIREMENT_FIRST_ATTEMPT_MS = 2_000L const val DAY_MS = 24 * 60 * 60 * 1000L /** The contract's limit for a server without the retirement route. */ const val RETIREMENT_UNSUPPORTED_LIFETIME_MS = 30 * DAY_MS diff --git a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt index 0e22c6d438..3d8e9ec1ff 100644 --- a/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt +++ b/androidApp/src/androidUnitTest/kotlin/org/siloserver/silo/android/push/AndroidPushRegistrarTest.kt @@ -335,7 +335,8 @@ class AndroidPushRegistrarTest { Triple(null,false,false),Triple(404,false,false),Triple(404,true,true)) for ((status,aged,dropped) in cases) { store.entries=emptyList(); savedLogins["server"]="login" - retireStatus=503; registrar().registerToken(token); registrar().unregisterDevice("server") + retireStatus=503; registrar().registerToken(token) + uncertain=true; registrar().unregisterDevice("server"); uncertain=false // the session removal gets no answer either savedLogins.remove("server") if (aged) store.entries=store.entries.map { it.copy(retirement=it.retirement!!.copy(queuedAtMs=it.retirement!!.queuedAtMs-31*day)) } retireStatus=status; val sent=retirements.size @@ -349,20 +350,21 @@ class AndroidPushRegistrarTest { } @Test fun signingInAgainOrARestoreDropsTheRetirementAndRegistersAboveIt()=runTest { val r=registrar(); r.registerToken(token) - retireStatus=null + retireStatus=null; uncertain=true // neither the retirement nor the session removal is answered assertFalse(r.unregisterDevice("server").result is ApiResult.Success) + uncertain=false assertEquals(2L,store.entries.single().retirement?.generation) owner=owner.copy(loginId="next",scope=owner.scope.copy(identityGeneration=2)); savedLogins["server"]="next" assertIs>(r.registerToken(token)) - assertEquals(listOf(1L,3L),generations) + assertEquals(listOf(1L,2L,3L),generations) assertNull(store.entries.single().retirement) registrar().retryPendingRetirements(); assertEquals(1,retirements.size) // A sign-out that stops undoes its removal, retirement included. - val removal=r.unregisterDevice("server") + uncertain=true; val removal=r.unregisterDevice("server"); uncertain=false assertEquals(4L,store.entries.single().retirement?.generation) assertIs>(r.restoreRegistration(removal)) - assertEquals(listOf(1L,3L,5L),generations) + assertEquals(listOf(1L,2L,3L,4L,5L),generations) val record=store.entries.single(); assertNull(record.retirement); assertTrue(record.acknowledged) registrar().retryPendingRetirements(); assertEquals(2,retirements.size) } @@ -396,7 +398,7 @@ class AndroidPushRegistrarTest { store.entries=emptyList(); retirements.clear(); savedLogins["server"]="login" registrar().registerToken(token) retireStatus=if (aged) null else 403 - registrar().unregisterDevice("server"); savedLogins.remove("server") + uncertain=aged; registrar().unregisterDevice("server"); uncertain=false; savedLogins.remove("server") if (aged) { store.entries=store.entries.map { it.copy(retirement=it.retirement!!.copy(queuedAtMs=it.retirement!!.queuedAtMs-91*day)) } registrar().retryPendingRetirements() // expires unsent @@ -409,7 +411,7 @@ class AndroidPushRegistrarTest { } @Test fun aRetirementThatStallsWaitsOutTheBackOffLikeOneThatFailed()=runTest { registrar().registerToken(token) - retireStatus=null; registrar().unregisterDevice("server"); savedLogins.remove("server") + retireStatus=null; uncertain=true; registrar().unregisterDevice("server"); uncertain=false; savedLogins.remove("server") val r=registrar() // a restart: no failures remembered onRetire={ awaitCancellation() } r.retryPendingRetirements(afterPush=true) @@ -417,6 +419,24 @@ class AndroidPushRegistrarTest { assertEquals(2,retirements.size) // the removal's and the stalled retry's; the next push waits assertEquals(2L,store.entries.single().retirement?.generation) } + @Test fun aSlowRetirementLeavesTheSessionRemovalTimeBeforeTheLoginIsCleared()=runTest { + val r=registrar(); r.registerToken(token) + // A server without the route that takes 7 seconds to say so, and a session removal that takes 1.5. + onRetire={ delay(7_000) }; send={ delay(1_500) } + assertIs>(r.unregisterDevice("server").result) + assertEquals(listOf(1L,2L),generations) + val record=store.entries.single(); assertTrue(record.acknowledged); assertNull(record.retirement) + } + @Test fun aRetirementWithoutAnAnswerFallsBackToTheSessionRemoval()=runTest { + for (status in listOf(null,429,503)) { + store.entries=emptyList(); generations.clear() + registrar().registerToken(token) + retireStatus=status + assertIs>(registrar().unregisterDevice("server").result,"$status") + assertEquals(listOf(1L,2L),generations,"$status") + val record=store.entries.single(); assertTrue(record.acknowledged,"$status"); assertNull(record.retirement,"$status") + } + } @Test fun conflictStaysDurableAndNeverRotatesProofOrGeneration()=runTest { failCode=409; registrar().registerToken(token) failCode=null diff --git a/docs/api-v2/android-push-registration.md b/docs/api-v2/android-push-registration.md index f12a4699fe..365c334710 100644 --- a/docs/api-v2/android-push-registration.md +++ b/docs/api-v2/android-push-registration.md @@ -31,9 +31,10 @@ is sent; a removal not yet confirmed keeps its generation. The retirement, `POST /api/v2/notifications/push/devices/{device_id}/retire` (`retirePushDevice`), carries only the installation key and generation headers, with no session, so it still works after sign-out or removal has cleared the -login. It is sent first. Only a server that answers 404 or 405 gets the session -DELETE instead, at the same key and generation, while the registering login -still exists. A retirement without a final answer stays queued and is sent again +login. It is sent first and given at most 2 of the removal's 8 seconds. A server +that answers 404 or 405, or does not answer in time (no answer, 429, 5xx), also +gets the session DELETE, at the same key and generation, while the registering +login still exists. A retirement without a final answer stays queued and is sent again on app start, on every foreground and when an FCM message arrives, at most three at a time with an in-memory back-off; an attempt that runs out of time counts as a failure. A removal that never queued its retirement, because it ran out of