From 0e3fa8ac60868ad427d21af093e32f3f971eec6c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 01:12:39 +0000 Subject: [PATCH] fix: start the AUTH prompt timeout when the dialog is shown, not when it arrives The host renders one dialog at a time, but every prompt's 60s deadline started when its challenge arrived. When several relays challenged at once, prompts 2..N counted down while queued and invisible, then expired unseen -- a silent deny the user had no way to act on. Reaching one after it expired was worse than useless: complete() is a no-op on a resolved deferred, so the click did nothing at all. No auth sent, no "always allow" rule written, no feedback. RelayAuthPrompt.markShown() now opens the answer window. It is gated on a host actually collecting the flow -- with no UI nobody can ever answer, which is what the timeout has always existed for, so that case keeps the arrival clock -- and capped by queueWaitMs so a host that stops rendering mid-queue cannot suspend a relay coroutine forever. A second challenge for the same (relay, account) now rides along on the owner's answer with no deadline of its own. Giving it one would let it complete the shared deferred and tear the dialog away while the user was still reading it. Three regression tests cover the queued-clock, the no-host timeout, and the rider. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01USYujXpjNrQdEyK39Q48Z1 --- .../2026-08-03-auth-permissions-redesign.md | 20 +++++- .../compose/RelayAuthPromptHost.kt | 3 + .../authCommand/model/RelayAuthPromptBus.kt | 69 +++++++++++++++++-- .../model/RelayAuthPromptBusTest.kt | 62 +++++++++++++++++ 4 files changed, 147 insertions(+), 7 deletions(-) diff --git a/amethyst/plans/2026-08-03-auth-permissions-redesign.md b/amethyst/plans/2026-08-03-auth-permissions-redesign.md index 02c2c519f0..ae69a30b65 100644 --- a/amethyst/plans/2026-08-03-auth-permissions-redesign.md +++ b/amethyst/plans/2026-08-03-auth-permissions-redesign.md @@ -38,9 +38,23 @@ Shipped as designed. Where it diverged or went further: where the new sentence belongs — or silently dropped a new `%1$s`. New keys fall back to the new English until Crowdin catches up. The 401 now-orphaned translations were deleted from the 11 locale files that carried them. -- **Not done:** the 60-second silent `DISMISS` (item 6 below) is still a silent - deny with the event left pending in the outbox. It needs a decision about what - the user should see, not just a layout. +- **The 60s timeout now runs from display, not from arrival.** Found while + answering "what happens if several auths are requested inside one 60s window?". + The host shows one dialog at a time but every prompt's deadline started when its + challenge arrived, so a burst of relays meant prompt 2..N counted down while + invisible. They expired unseen — a silent deny — and if the user did reach one + after it expired, the click was swallowed whole: `complete()` is a no-op on a + resolved deferred, so no auth was sent and not even the "always allow" rule was + written. `RelayAuthPrompt.markShown()` now starts the window, gated on a host + actually collecting (no UI → the old arrival clock, which is what the timeout was + always for) and capped by `queueWaitMs` so a stuck queue can't suspend a + connection forever. A second challenge for the same (relay, account) rides along + on the owner's answer with no deadline of its own — running one would let it + resolve the shared deferred and tear down a dialog mid-read. +- **Still not done:** what a timeout should *look like*. It is now an honest 60s of + visible time rather than a clock the user never saw, but it is still a dialog + that vanishes and an event left pending in the outbox with no feedback. That + needs a product decision, not a layout. Verified: `:amethyst:testFdroidDebugUnitTest` 1096 tests green (38 in the relay-auth suites, 5 of them new), `:commons:jvmTest` 1446 green, `spotlessApply` clean. diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/compose/RelayAuthPromptHost.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/compose/RelayAuthPromptHost.kt index 7315dde3ad..244f9b043a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/compose/RelayAuthPromptHost.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/compose/RelayAuthPromptHost.kt @@ -113,7 +113,10 @@ fun RelayAuthPromptHost( } } + // One dialog at a time. Everything else waits its turn, and tells the bus when its turn comes so + // its answer window starts from the moment it is visible rather than from the challenge. queue.firstOrNull { !it.isResolved }?.let { prompt -> + LaunchedEffect(prompt) { prompt.markShown() } RelayAuthPromptDialog(prompt, accountViewModel, nav) { choice -> prompt.respond(choice) queue.remove(prompt) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBus.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBus.kt index 61cdeeb943..a8ad8ea46a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBus.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBus.kt @@ -26,6 +26,7 @@ import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.SharedFlow +import kotlinx.coroutines.flow.first import kotlinx.coroutines.withTimeoutOrNull /** What the user chose when asked whether to authenticate with a relay. */ @@ -64,6 +65,20 @@ class RelayAuthPrompt( reply.complete(choice) } + private val shown = CompletableDeferred() + + /** + * Called by the host the moment this prompt is actually put on screen. The answer window is + * measured from here, not from when the challenge arrived — the host shows one dialog at a time, + * so a prompt can sit queued behind another for a long while, and its clock must not be running + * during that. + */ + fun markShown() { + shown.complete(Unit) + } + + internal suspend fun awaitShown() = shown.await() + /** True once answered by the user or resolved by the bus (e.g. timed out). */ val isResolved: Boolean get() = reply.isCompleted @@ -82,6 +97,7 @@ class RelayAuthPrompt( */ class RelayAuthPromptBus( private val timeoutMs: Long = DEFAULT_TIMEOUT_MS, + private val queueWaitMs: Long = DEFAULT_QUEUE_WAIT_MS, ) { // replay so a challenge raised *before* the UI host subscribes — cold start, an account switch, // any moment no RelayAuthPromptHost is collecting — isn't dropped (which would stall the auth @@ -112,16 +128,58 @@ class RelayAuthPromptBus( ?: CompletableDeferred().also { inFlight[key] = it } to true } - if (isOwner) mutablePrompts.emit(RelayAuthPrompt(relayUrl, purposes, askingAccount, isMyOwnRelay, deferred)) + // Only the owner surfaces a dialog and owns its deadline. A second challenge for the same + // (relay, account) just rides along on the owner's answer — it must NOT run a deadline of its + // own, because resolving the shared deferred would tear down a dialog the user is still + // looking at. + if (!isOwner) return rideAlong(deferred) + + val prompt = RelayAuthPrompt(relayUrl, purposes, askingAccount, isMyOwnRelay, deferred) + mutablePrompts.emit(prompt) return try { - awaitOrTimeout(deferred) + awaitOrTimeout(prompt, deferred) } finally { - if (isOwner) synchronized(inFlight) { inFlight.remove(key) } + synchronized(inFlight) { inFlight.remove(key) } } } - private suspend fun awaitOrTimeout(deferred: CompletableDeferred): UserAuthChoice { + /** + * Waits for the owner's answer without imposing a deadline that could resolve the shared prompt. + * The cap exists only so a cancelled owner can't strand this caller forever; it deliberately + * returns [UserAuthChoice.DISMISS] *locally* rather than completing the deferred. + */ + private suspend fun rideAlong(deferred: CompletableDeferred): UserAuthChoice = withTimeoutOrNull(queueWaitMs + 2 * timeoutMs) { deferred.await() } ?: UserAuthChoice.DISMISS + + /** + * Waits for the user's answer, giving them [timeoutMs] **from the moment the dialog is on screen** + * rather than from the moment the challenge arrived. + * + * That distinction is the whole point. The host renders one dialog at a time, so when several + * relays challenge at once every prompt but the first is queued and invisible — and with a single + * deadline measured from arrival, those queued prompts expired without ever being shown. The user + * saw nothing, the relay was silently denied, and a click on a dialog that had already expired was + * swallowed whole: `complete()` is a no-op on a resolved deferred, so the auth was never sent and + * not even the "always allow" rule was written. + */ + private suspend fun awaitOrTimeout( + prompt: RelayAuthPrompt, + deferred: CompletableDeferred, + ): UserAuthChoice { + // Is anything able to display this? With no host collecting — a headless background process, + // or a cold start before the UI subscribes — nobody can ever answer, and the relay coroutine + // must not hang. That case is what the timeout has always been for, so it keeps the old clock. + val hasHost = withTimeoutOrNull(timeoutMs) { mutablePrompts.subscriptionCount.first { it > 0 } } != null + + if (hasHost) { + // Wait for this prompt's turn at the front of the host's queue. Capped, so a host that + // stops rendering (backgrounded mid-queue) still can't suspend the connection forever. + withTimeoutOrNull(queueWaitMs) { prompt.awaitShown() } + } + + // The answer window proper. If the user already answered while we were waiting above, this + // returns immediately. withTimeoutOrNull(timeoutMs) { deferred.await() }?.let { return it } + // Timed out: resolve the deferred so any UI still showing this prompt can drop it, and so a // concurrent waiter on the same deferred gets an answer too. complete() is a no-op if a late // user response already won the race. @@ -131,5 +189,8 @@ class RelayAuthPromptBus( companion object { const val DEFAULT_TIMEOUT_MS = 60_000L + + /** How long a prompt may wait its turn behind other dialogs before we give up on it. */ + const val DEFAULT_QUEUE_WAIT_MS = 5 * 60_000L } } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBusTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBusTest.kt index 176cdeb684..d1931883bf 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBusTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBusTest.kt @@ -23,6 +23,7 @@ package com.vitorpamplona.amethyst.service.relayClient.authCommand.model import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.async +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.take import kotlinx.coroutines.flow.toList @@ -113,4 +114,65 @@ class RelayAuthPromptBusTest { bus.prompts.first().respond(UserAuthChoice.ALLOW_ONCE) assertEquals(UserAuthChoice.ALLOW_ONCE, caller.await()) } + + @Test + fun aQueuedPromptsClockStartsWhenItIsShown() = + runTest { + val bus = RelayAuthPromptBus(timeoutMs = 1_000L) + val relayA = NormalizedRelayUrl("wss://a.relay.test") + val relayB = NormalizedRelayUrl("wss://b.relay.test") + + val surfaced = async { bus.prompts.take(2).toList() } + val callerA = async { bus.requestDecision(relayA, emptyList(), alice, isMyOwnRelay = false) } + val callerB = async { bus.requestDecision(relayB, emptyList(), alice, isMyOwnRelay = false) } + val prompts = surfaced.await() + + // The host renders one dialog at a time, so B sits queued and invisible while the user + // works through A. Only A is on screen, so only A's clock is running. + prompts[0].markShown() + delay(600) + prompts[0].respond(UserAuthChoice.ALLOW_ONCE) + assertEquals(UserAuthChoice.ALLOW_ONCE, callerA.await()) + + // B's turn. Its own window opens now — well past the point where a single deadline + // measured from the challenge would already have expired it unseen. + prompts[1].markShown() + delay(600) + prompts[1].respond(UserAuthChoice.ALWAYS_ALLOW) + + assertEquals(UserAuthChoice.ALWAYS_ALLOW, callerB.await()) + } + + @Test + fun aPromptNoHostCanEverShowStillTimesOut() = + runTest { + // Nothing collects the flow, so nobody can answer. The relay coroutine must not hang + // waiting for a dialog that will never appear. + val bus = RelayAuthPromptBus(timeoutMs = 1_000L, queueWaitMs = 60_000L) + + assertEquals(UserAuthChoice.DISMISS, bus.ask()) + } + + @Test + fun aSecondChallengeNeverResolvesTheDialogTheUserIsLookingAt() = + runTest { + val bus = RelayAuthPromptBus(timeoutMs = 1_000L) + + val surfaced = async { bus.prompts.first() } + val owner = async { bus.ask() } + val rider = async { bus.ask() } + val prompt = surfaced.await() + + // The prompt is queued behind another dialog well past the rider's old deadline. The + // rider has no dialog of its own, so it must simply wait: if it ran its own clock it + // would complete the shared deferred at 1000ms and yank this prompt away before the + // user ever saw it. + delay(1_500) + prompt.markShown() + delay(300) + prompt.respond(UserAuthChoice.ALWAYS_ALLOW) + + assertEquals(UserAuthChoice.ALWAYS_ALLOW, owner.await()) + assertEquals(UserAuthChoice.ALWAYS_ALLOW, rider.await()) + } }