From f306f36a5f8ced71a6c34ab419cf788ed4a8852a Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Thu, 8 Oct 2026 01:44:31 +0200 Subject: [PATCH 1/6] fix(login): use a new connection for each Login Flow poll Login Flow v2 polls disable OkHttp connection-failure recovery so a one-time approval response is never replayed, but they shared the platform connection pool. A pooled connection that died while the app was in the background, suspended, or past a proxy keep-alive window still passed the reuse health check, so the next poll failed only after its request had started. That failure was classified as an ambiguous post-exchange result and ended sign-in with "could not be confirmed safely". Derive the poll client from the base client with a non-retaining connection pool so no connection outlives the wait between polls. Retry connection-setup failures that happen before a request is sent, with a distinct "could not be reached" status. Failures after the exchange starts remain terminal. Advances #488 --- .../AndroidNextcloudServices.kt | 6 +- .../488-login-poll-fresh-connections.md | 7 + docs/login-diagnostics.md | 25 +- tools/kotlin-file-size-baseline.txt | 2 +- .../nextcloudnative/app/LoginAttemptState.kt | 4 + .../nextcloudnative/app/LoginPolling.kt | 9 +- .../nextcloudnative/app/LoginPollingTest.kt | 27 ++ .../app/DesktopNextcloudServices.kt | 2 +- .../app/JvmLoginFlowTransportTest.kt | 29 +++ .../app/JvmLoginPollConnectionTest.kt | 231 ++++++++++++++++++ .../app/LoginEndpointSecurityTest.kt | 40 ++- .../app/JvmLoginFlowTransport.kt | 19 ++ .../app/LoginPollNetworkFailure.kt | 26 +- 13 files changed, 407 insertions(+), 20 deletions(-) create mode 100644 changes/unreleased/488-login-poll-fresh-connections.md create mode 100644 ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt index 9ad3dae42..c500b54ae 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt @@ -31,6 +31,7 @@ import dev.obiente.nextcloudnative.app.LoginTransportSecurity import dev.obiente.nextcloudnative.app.LOGIN_FLOW_RESPONSE_MAX_BYTES import dev.obiente.nextcloudnative.app.LoginPollHttpResponse import dev.obiente.nextcloudnative.app.executeLoginPollHttp +import dev.obiente.nextcloudnative.app.newLoginPollHttpClient import dev.obiente.nextcloudnative.app.interpretLoginChallengeHttpResponse import dev.obiente.nextcloudnative.app.loginPollEndpointFallbackDiagnostic import dev.obiente.nextcloudnative.app.normalizeServerUrl @@ -347,7 +348,7 @@ internal class AndroidNextcloudServices( .useAndroidNextcloudCertificateTrust(appContext) .trackJvmNetworkFailures() .build() - private val loginPollHttpClient = httpClient.newBuilder().retryOnConnectionFailure(false).build() + private val loginPollHttpClient = httpClient.newLoginPollHttpClient() private val loginPollFallbackTokens = ConcurrentHashMap.newKeySet() private val loginPollPendingTokens = ConcurrentHashMap.newKeySet() private val noRedirectHttpClient = httpClient.newBuilder() @@ -3730,9 +3731,6 @@ internal class AndroidNextcloudServices( LoginTransportSecurity.Tls } - private val LoginTransportSecurity.diagnosticValue: String - get() = if (this == LoginTransportSecurity.PlainHttp) "plaintext" else "tls" - private fun JSONArray.toAppEntries(): List = buildList { for (index in 0 until length()) { val item = optJSONObject(index) ?: continue diff --git a/changes/unreleased/488-login-poll-fresh-connections.md b/changes/unreleased/488-login-poll-fresh-connections.md new file mode 100644 index 000000000..7a4290314 --- /dev/null +++ b/changes/unreleased/488-login-poll-fresh-connections.md @@ -0,0 +1,7 @@ +category: fix +issue: 488 +pull: none +platforms: android, desktop +user-facing: yes + +Keep browser sign-in working after returning to the app during approval: each check uses a new connection, and connection failures before a request is sent are retried. diff --git a/docs/login-diagnostics.md b/docs/login-diagnostics.md index ca98132fd..5deda07cc 100644 --- a/docs/login-diagnostics.md +++ b/docs/login-diagnostics.md @@ -1,6 +1,6 @@ # Sign-in recovery and login diagnostics -**Last reviewed: 2026-09-28.** This describes repository implementation, which may +**Last reviewed: 2026-10-08.** This describes repository implementation, which may have changed. Check the [current source](https://github.com/obiente/native/blob/main/ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginAttemptState.kt) and the release notes for the installed build before assuming a released version includes these controls. @@ -24,20 +24,28 @@ the URL parser's error text. ## Connection problems A name-resolution message means a DNS lookup failed before any request reached -the server. It does not prove the server is down, or that the failure is still -happening. Check the address and the device's network or VPN connection. +the server. A "could not be reached" message means the app could not open a +connection to the server, so no sign-in request was sent. Neither proves the +server is down, or that the failure is still happening. Check the address and +the device's network or VPN connection. -The app retries this kind of failure after a bounded delay. When the server +The app retries both kinds of failure after a bounded delay. When the server answers again, the browser-approval status returns. You can also cancel, change the address or connection, and start a fresh attempt. +Each approval check opens a new connection and closes it after the response. +Leaving the app for the browser can suspend it or end its network connections. +Because no connection is kept between checks, returning to the app does not +reuse one that ended while the app was in the background. + ## Time limit and uncertain results Each attempt has a five-minute deadline, including network waits. If the result became uncertain after the one-time approval exchange, start a new sign-in. Do not try to replay that exchange. The app does not retry this -case automatically. +case automatically. A request is uncertain only when the connection failed +after the app had started sending it. ## Save a login report @@ -85,8 +93,11 @@ Account publication failures retain the existing credential rollback policy. Synthetic regression tests in `LoginPollingTest` and `LoginAttemptStateTest` exercise retry phases, timeout, cancellation, browser-handoff failure, and late -completion after a replacement attempt. They do not establish connectivity to -any particular server or device network. +completion after a replacement attempt. `JvmLoginPollConnectionTest` runs +approval checks against a local mock server while simulating connections that +end during a suspension. These tests run on the desktop JVM. They do not +establish connectivity to any particular server or device network, and they do +not reproduce Android process freezing. ## Evidence and release status diff --git a/tools/kotlin-file-size-baseline.txt b/tools/kotlin-file-size-baseline.txt index a875c8a79..9082606fd 100644 --- a/tools/kotlin-file-size-baseline.txt +++ b/tools/kotlin-file-size-baseline.txt @@ -1,4 +1,4 @@ -androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt|4132 +androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt|4130 androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidProjectContentClient.kt|985 androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt|900 contractAcquisition/src/main/kotlin/dev/obiente/nextcloudnative/contracts/SignedAppStoreContractAcquirer.kt|1212 diff --git a/ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginAttemptState.kt b/ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginAttemptState.kt index 987436aeb..6c14d3d7b 100644 --- a/ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginAttemptState.kt +++ b/ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginAttemptState.kt @@ -19,6 +19,10 @@ internal enum class LoginAttemptPhase(val message: String, val buttonLabel: Stri "The server name could not be resolved. Check your network or VPN. Retrying...", "Retrying connection...", ), + ReconnectingToServer( + "The server could not be reached. Check your network or VPN. Retrying...", + "Retrying connection...", + ), Completing("Finishing sign-in...", "Finishing sign-in..."), } diff --git a/ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginPolling.kt b/ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginPolling.kt index 7fb0cf0a5..b79f71988 100644 --- a/ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginPolling.kt +++ b/ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginPolling.kt @@ -4,6 +4,7 @@ import kotlin.math.min internal const val LOGIN_POLL_PENDING_DELAY_MILLIS = 2_000L internal const val LOGIN_POLL_MAX_RETRY_DELAY_MILLIS = 15_000L +private const val LOGIN_POLL_DNS_UNRESOLVED_CODE = "NETWORK_DNS_UNRESOLVED" internal suspend fun pollLoginUntilApproved( poll: suspend () -> LoginPollResult, @@ -23,7 +24,13 @@ internal suspend fun pollLoginUntilApproved( is LoginPollResult.RetryablePreExchangeFailure -> { transientFailures += 1 val delayMillis = loginPollRetryDelayMillis(transientFailures) - onStatus(LoginAttemptPhase.WaitingForNetwork) + onStatus( + if (result.code == LOGIN_POLL_DNS_UNRESOLVED_CODE) { + LoginAttemptPhase.WaitingForNetwork + } else { + LoginAttemptPhase.ReconnectingToServer + }, + ) waitBeforeNextPoll(delayMillis, true) } is LoginPollResult.FatalFailure -> error(result.message) diff --git a/ui/src/commonTest/kotlin/dev/obiente/nextcloudnative/app/LoginPollingTest.kt b/ui/src/commonTest/kotlin/dev/obiente/nextcloudnative/app/LoginPollingTest.kt index 355b483e5..9d8696652 100644 --- a/ui/src/commonTest/kotlin/dev/obiente/nextcloudnative/app/LoginPollingTest.kt +++ b/ui/src/commonTest/kotlin/dev/obiente/nextcloudnative/app/LoginPollingTest.kt @@ -51,6 +51,33 @@ class LoginPollingTest { assertEquals(LoginAttemptPhase.AwaitingApproval, statuses.last()) } + @Test + fun connectionFailuresBeforeTheExchangeRetryWithoutClaimingDnsFailure() = runBlocking { + val session = NextcloudSession("https://cloud.example.com", "person", "secret") + val results = ArrayDeque().apply { + add(LoginPollResult.Pending) + add(LoginPollResult.RetryablePreExchangeFailure("NETWORK_CONNECT_FAILED")) + add(LoginPollResult.Approved(session)) + } + val waits = mutableListOf>() + val statuses = mutableListOf() + + val approved = pollLoginUntilApproved( + poll = { results.removeFirst() }, + waitBeforeNextPoll = { delay, awaitNetwork -> waits += delay to awaitNetwork }, + hasTimedOut = { false }, + onStatus = statuses::add, + ) + + assertEquals(session, approved) + assertEquals(listOf(2_000L to false, 2_000L to true), waits) + assertEquals( + listOf(LoginAttemptPhase.AwaitingApproval, LoginAttemptPhase.ReconnectingToServer), + statuses, + ) + assertTrue("resolved" !in LoginAttemptPhase.ReconnectingToServer.message) + } + @Test fun anAmbiguousPostExchangeFailureIsNeverRetried() = runBlocking { var polls = 0 diff --git a/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopNextcloudServices.kt b/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopNextcloudServices.kt index 50184f8d4..c93485f3d 100644 --- a/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopNextcloudServices.kt +++ b/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopNextcloudServices.kt @@ -708,7 +708,7 @@ class DesktopNextcloudServices( environment = desktopSupportDiagnosticsEnvironment(), client = httpClient.newBuilder().retryOnConnectionFailure(false).build(), ) - private val loginPollHttpClient = httpClient.newBuilder().retryOnConnectionFailure(false).build() + private val loginPollHttpClient = httpClient.newLoginPollHttpClient() private val loginPollFallbackTokens = ConcurrentHashMap.newKeySet() private val loginPollPendingTokens = ConcurrentHashMap.newKeySet() private val fileMutationHttpExecutor = DesktopHttpMutationExecutor(httpClient) diff --git a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransportTest.kt b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransportTest.kt index f942950d8..8240c48c3 100644 --- a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransportTest.kt +++ b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransportTest.kt @@ -211,6 +211,35 @@ class JvmLoginFlowTransportTest { } } + @Test + fun `compatibility probe that cannot connect leaves the pending advertised route active`() = runBlocking { + var diagnostic: JvmNetworkFailureDiagnostic? = null + val execution = executeLoginPollHttp( + challenge = challenge( + pollEndpoint = "https://cloud.example.test/login/v2/poll", + fallbackEndpoint = "https://cloud.example.test/index.php/login/v2/poll", + ), + fallbackAlreadySelected = false, + poll = { endpoint -> + diagnostic = null + if ("/index.php/" in endpoint) { + diagnostic = dnsFailure().copy( + code = "NETWORK_CONNECT_FAILED", + phase = JvmNetworkFailurePhase.Connect, + ) + throw IOException("synthetic connect failure") + } + LoginPollHttpResponse(status = 404, body = "[]") + }, + networkFailure = { diagnostic }, + ) + + assertEquals(LoginPollResult.Pending, execution.interpretation.result) + assertNull(execution.selectedFallbackReason) + assertEquals(false, execution.usedFallback) + assertEquals(false, execution.responseUsedFallback) + } + @Test fun `failure after compatibility exchange is ambiguous`() = runBlocking { var diagnostic: JvmNetworkFailureDiagnostic? = null diff --git a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt new file mode 100644 index 000000000..240330343 --- /dev/null +++ b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt @@ -0,0 +1,231 @@ +package dev.obiente.nextcloudnative.app + +import java.io.FilterInputStream +import java.io.FilterOutputStream +import java.io.IOException +import java.io.InputStream +import java.io.OutputStream +import java.net.InetAddress +import java.net.Socket +import java.net.SocketException +import java.net.SocketTimeoutException +import java.util.concurrent.CopyOnWriteArrayList +import javax.net.SocketFactory +import kotlinx.coroutines.runBlocking +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertIs +import kotlin.test.assertNotSame +import kotlin.test.assertTrue +import mockwebserver3.MockResponse +import mockwebserver3.MockWebServer +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.RequestBody.Companion.toRequestBody + +/** + * Reproduces a connection that dies while the app is suspended between Login Flow v2 polls. + * + * A destroyed or silently dropped socket still passes OkHttp's pooled-connection health check, + * so the next request is written before the failure surfaces. That failure is correctly + * ambiguous, so the poll client must never reuse a connection across the wait between polls. + */ +class JvmLoginPollConnectionTest { + @Test + fun `a poll written to a reused dead connection stays ambiguous`() = runBlocking { + MockWebServer().use { server -> + server.start() + server.enqueue(challengeResponse()) + server.enqueue(pendingResponse()) + server.enqueue(server.approvedResponse()) + val sockets = SuspendableSocketFactory() + val base = baseClient(sockets) + val sharedPoolClient = base.newBuilder().retryOnConnectionFailure(false).build() + val challenge = challenge(server.url("/login/v2/poll").toString()) + + beginLogin(base, server.url("/index.php/login/v2").toString()) + assertEquals(LoginPollResult.Pending, poll(sharedPoolClient, challenge)) + assertEquals(1, sockets.created.size) + sockets.suspendAll() + + val failure = assertIs( + poll(sharedPoolClient, challenge), + ) + assertEquals("NETWORK_SOCKET_FAILED", failure.code) + assertTrue(failure.message.contains("could not be confirmed safely")) + } + } + + @Test + fun `login polls survive connections that died while the app was suspended`() = runBlocking { + MockWebServer().use { server -> + server.start() + server.enqueue(challengeResponse()) + server.enqueue(pendingResponse()) + server.enqueue(pendingResponse()) + server.enqueue(server.approvedResponse()) + val sockets = SuspendableSocketFactory() + val base = baseClient(sockets) + val pollClient = base.newLoginPollHttpClient() + val challenge = challenge(server.url("/login/v2/poll").toString()) + + beginLogin(base, server.url("/index.php/login/v2").toString()) + sockets.suspendAll() + assertEquals(LoginPollResult.Pending, poll(pollClient, challenge)) + sockets.suspendAll() + assertEquals(LoginPollResult.Pending, poll(pollClient, challenge)) + sockets.suspendAll() + val approved = assertIs(poll(pollClient, challenge)) + + assertEquals("person", approved.session.loginName) + assertEquals(4, sockets.created.size) + assertEquals(4, server.requestCount) + } + } + + @Test + fun `login poll client never replays a request and keeps no idle connection`() { + val base = OkHttpClient.Builder().trackJvmNetworkFailures().build() + val pollClient = base.newLoginPollHttpClient() + + assertFalse(pollClient.retryOnConnectionFailure) + assertNotSame(base.connectionPool, pollClient.connectionPool) + assertEquals(0, pollClient.connectionPool.idleConnectionCount()) + assertEquals(base.eventListenerFactory, pollClient.eventListenerFactory) + assertEquals(base.socketFactory, pollClient.socketFactory) + } + + private fun baseClient(sockets: SuspendableSocketFactory): OkHttpClient = OkHttpClient.Builder() + .socketFactory(sockets) + // Racing IPv4 and IPv6 loopback addresses would create a losing socket per connection. + .fastFallback(false) + .trackJvmNetworkFailures() + .build() + + private fun beginLogin(client: OkHttpClient, url: String) { + val request = Request.Builder().url(url).post(ByteArray(0).toRequestBody(null)).build() + client.newCall(request).execute().use { response -> + assertEquals(200, response.code) + response.body.string() + } + } + + private suspend fun poll(client: OkHttpClient, challenge: LoginChallenge): LoginPollResult { + var networkFailure: JvmNetworkFailureDiagnostic? = null + return executeLoginPollHttp( + challenge = challenge, + fallbackAlreadySelected = false, + poll = { endpoint -> + networkFailure = null + val attempt = JvmNetworkRequestAttempt() + val request = Request.Builder() + .url(endpoint) + .post("token=synthetic-one-time-token".toRequestBody(FORM)) + .tag(JvmNetworkRequestAttempt::class.java, attempt) + .build() + try { + client.newCall(request).execute().use { response -> + LoginPollHttpResponse(response.code, response.body.string()) + } + } catch (failure: IOException) { + networkFailure = failure.toJvmNetworkFailureDiagnostic( + attempt = attempt, + readOnlyRequest = false, + replayableRequest = false, + ) + throw failure + } + }, + networkFailure = { networkFailure }, + ).interpretation.result + } + + private fun challenge(pollEndpoint: String) = LoginChallenge( + enteredServerUrl = pollEndpoint.substringBefore("/login/v2/poll"), + pollEndpoint = pollEndpoint, + pollFallbackEndpoint = null, + token = "synthetic-one-time-token", + loginUrl = pollEndpoint.replace("/poll", "/flow/synthetic"), + transportSecurity = LoginTransportSecurity.PlainHttp, + ) + + private fun challengeResponse() = MockResponse.Builder().code(200).body("{}").build() + + private fun pendingResponse() = MockResponse.Builder().code(404).body("[]").build() + + private fun MockWebServer.approvedResponse() = MockResponse.Builder() + .code(200) + .body( + """{"server":"${url("/").toString().trimEnd('/')}","loginName":"person",""" + + """"appPassword":"synthetic-app-password"}""", + ) + .build() + + private companion object { + val FORM = "application/x-www-form-urlencoded".toMediaType() + } +} + +/** + * Creates sockets that can be marked dead without the client observing a close, like a socket + * destroyed while its process was frozen or a connection whose network path was dropped. + */ +private class SuspendableSocketFactory : SocketFactory() { + val created = CopyOnWriteArrayList() + + fun suspendAll() = created.forEach(SuspendableSocket::markDead) + + override fun createSocket(): Socket = SuspendableSocket().also(created::add) + + override fun createSocket(host: String, port: Int): Socket = unsupported() + + override fun createSocket(host: String, port: Int, localHost: InetAddress, localPort: Int): Socket = + unsupported() + + override fun createSocket(host: InetAddress, port: Int): Socket = unsupported() + + override fun createSocket(address: InetAddress, port: Int, localAddress: InetAddress, localPort: Int): Socket = + unsupported() + + private fun unsupported(): Nothing = throw UnsupportedOperationException("OkHttp connects unconnected sockets") +} + +private class SuspendableSocket : Socket() { + @Volatile + private var dead = false + + fun markDead() { + dead = true + } + + override fun getInputStream(): InputStream = object : FilterInputStream(super.getInputStream()) { + override fun read(): Int { + if (dead) throw SocketTimeoutException("no data arrived") + return super.read() + } + + override fun read(buffer: ByteArray, offset: Int, length: Int): Int { + if (dead) throw SocketTimeoutException("no data arrived") + return super.read(buffer, offset, length) + } + } + + override fun getOutputStream(): OutputStream = object : FilterOutputStream(super.getOutputStream()) { + override fun write(value: Int) { + if (dead) throw SocketException("Software caused connection abort") + out.write(value) + } + + override fun write(buffer: ByteArray, offset: Int, length: Int) { + if (dead) throw SocketException("Software caused connection abort") + out.write(buffer, offset, length) + } + + override fun flush() { + if (dead) throw SocketException("Software caused connection abort") + out.flush() + } + } +} diff --git a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/LoginEndpointSecurityTest.kt b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/LoginEndpointSecurityTest.kt index fcc5ffd64..2d1dfd8d6 100644 --- a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/LoginEndpointSecurityTest.kt +++ b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/LoginEndpointSecurityTest.kt @@ -137,7 +137,7 @@ class LoginEndpointSecurityTest { } @Test - fun onlyPreExchangeDnsFailureIsSafeToRetry() { + fun onlyTransientConnectionSetupFailuresAreSafeToRetry() { val dns = JvmNetworkFailureDiagnostic( code = "NETWORK_DNS_UNRESOLVED", phase = JvmNetworkFailurePhase.Dns, @@ -146,10 +146,40 @@ class LoginEndpointSecurityTest { exchangeStarted = false, protocol = null, ) - val afterExchange = dns.copy(exchangeStarted = true) - - assertIs(classifyLoginPollNetworkFailure(dns)) - assertIs(classifyLoginPollNetworkFailure(afterExchange)) + val connectionSetupFailures = listOf( + dns, + dns.copy(code = "NETWORK_CONNECT_FAILED", phase = JvmNetworkFailurePhase.Connect), + dns.copy(code = "NETWORK_CONNECT_TIMEOUT", phase = JvmNetworkFailurePhase.Connect), + dns.copy(code = "NETWORK_UNREACHABLE", phase = JvmNetworkFailurePhase.Connect), + dns.copy(code = "NETWORK_TLS_TIMEOUT", phase = JvmNetworkFailurePhase.Tls), + dns.copy(code = "NETWORK_SOCKET_FAILED", phase = JvmNetworkFailurePhase.Connect), + dns.copy(code = "NETWORK_CONNECTION_RESET", phase = JvmNetworkFailurePhase.Connect), + ) + connectionSetupFailures.forEach { failure -> + val result = assertIs( + classifyLoginPollNetworkFailure(failure), + ) + assertEquals(failure.code, result.code) + // A request that may have reached the server is never retried, whatever its cause. + val afterExchange = failure.copy( + exchangeStarted = true, + phase = JvmNetworkFailurePhase.ResponseHeaders, + ) + val ambiguous = assertIs( + classifyLoginPollNetworkFailure(afterExchange), + ) + assertEquals(failure.code, ambiguous.code) + } + listOf( + dns.copy(code = "NETWORK_CERTIFICATE_REJECTED", phase = JvmNetworkFailurePhase.Tls), + dns.copy(code = "NETWORK_TLS_HANDSHAKE", phase = JvmNetworkFailurePhase.Tls), + dns.copy(code = "NETWORK_PROTOCOL_FAILED", phase = JvmNetworkFailurePhase.Connect), + dns.copy(code = "NETWORK_UNKNOWN_FAILED", phase = JvmNetworkFailurePhase.Unknown), + dns.copy(code = "NETWORK_SOCKET_FAILED", phase = JvmNetworkFailurePhase.Tls), + ).forEach { failure -> + assertIs(classifyLoginPollNetworkFailure(failure)) + } + assertIs(classifyLoginPollNetworkFailure(null)) assertEquals( false, loginResultOriginMatchesEntered("https://cloud.example.com", "https://other.example.com"), diff --git a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt index a216d8ddc..01524ec1f 100644 --- a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt +++ b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt @@ -1,6 +1,9 @@ package dev.obiente.nextcloudnative.app +import java.util.concurrent.TimeUnit import kotlinx.coroutines.CancellationException +import okhttp3.ConnectionPool +import okhttp3.OkHttpClient data class LoginPollHttpResponse( val status: Int, @@ -21,6 +24,22 @@ data class LoginPollHttpExecution( val selectedFallbackReason: LoginPollFallbackReason? = null, ) +/** + * Derives the client used for one-time Login Flow v2 polls from the platform's base client. + * + * The HTTP stack must never replay a poll after its request may have reached the server, so + * connection-failure recovery stays disabled. Without that recovery, a pooled connection that + * died while the app was suspended or in the background, or that a proxy closed after its + * keep-alive window, fails only after the request has started and is indistinguishable from a + * lost one-time approval response. Each poll therefore opens a new connection and closes it + * after the response, so no connection outlives the wait between polls. Failures while that + * connection is being established remain provably pre-exchange. + */ +fun OkHttpClient.newLoginPollHttpClient(): OkHttpClient = newBuilder() + .retryOnConnectionFailure(false) + .connectionPool(ConnectionPool(0, 1, TimeUnit.SECONDS)) + .build() + suspend fun executeLoginPollHttp( challenge: LoginChallenge, fallbackAlreadySelected: Boolean, diff --git a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/LoginPollNetworkFailure.kt b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/LoginPollNetworkFailure.kt index df10b70ff..5758c9a81 100644 --- a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/LoginPollNetworkFailure.kt +++ b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/LoginPollNetworkFailure.kt @@ -1,9 +1,17 @@ package dev.obiente.nextcloudnative.app +/** + * Classifies a failed poll request by whether the one-time exchange could have reached the server. + * + * Only failures that occurred while the connection was being established, before OkHttp started + * writing the request, are retried. Certificate, TLS handshake, protocol, and unknown failures + * remain fatal because waiting does not resolve them, and any failure after the exchange started + * stays ambiguous because the server may already have issued its one-time response. + */ fun classifyLoginPollNetworkFailure( diagnostic: JvmNetworkFailureDiagnostic?, ): LoginPollResult = when { - diagnostic?.code == "NETWORK_DNS_UNRESOLVED" && !diagnostic.exchangeStarted -> + diagnostic != null && diagnostic.isTransientBeforeExchange() -> LoginPollResult.RetryablePreExchangeFailure(diagnostic.code) diagnostic?.exchangeStarted == true -> LoginPollResult.AmbiguousAfterExchangeFailure( message = "The login response could not be confirmed safely after contacting the server. " + @@ -21,3 +29,19 @@ fun ambiguousLoginPollResponse(message: String): LoginPollResult = message = "$message Restart sign-in so the app does not reuse a one-time approval response.", code = "LOGIN_POLL_RESPONSE_INVALID", ) + +private fun JvmNetworkFailureDiagnostic.isTransientBeforeExchange(): Boolean = + !exchangeStarted && ( + code in TRANSIENT_CONNECTION_SETUP_CODES || + (code in TRANSIENT_SOCKET_CODES && phase in CONNECTION_SETUP_PHASES) + ) + +private val TRANSIENT_CONNECTION_SETUP_CODES = setOf( + "NETWORK_DNS_UNRESOLVED", + "NETWORK_CONNECT_FAILED", + "NETWORK_CONNECT_TIMEOUT", + "NETWORK_UNREACHABLE", + "NETWORK_TLS_TIMEOUT", +) +private val TRANSIENT_SOCKET_CODES = setOf("NETWORK_SOCKET_FAILED", "NETWORK_CONNECTION_RESET") +private val CONNECTION_SETUP_PHASES = setOf(JvmNetworkFailurePhase.Dns, JvmNetworkFailurePhase.Connect) From 5a93966db0cd53a582b60dbd1fa72f5fa6dbd234 Mon Sep 17 00:00:00 2001 From: "obiente-automations[bot]" <311907242+obiente-automations[bot]@users.noreply.github.com> Date: Thu, 8 Oct 2026 00:39:38 +0000 Subject: [PATCH 2/6] chore(website): refresh marketing captures --- website/public/screenshots/capture-manifest.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/website/public/screenshots/capture-manifest.json b/website/public/screenshots/capture-manifest.json index 5976f9d17..1b4488b67 100644 --- a/website/public/screenshots/capture-manifest.json +++ b/website/public/screenshots/capture-manifest.json @@ -678,10 +678,10 @@ "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LivePhotoDetection.kt": "d4afebb50e544a7e1ff8de73be5a48e60b657f85f127172f4b6d59213f0100f5", "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LivePhotos.kt": "380a1f135dd50b1746967a6805b69dd4e48ab77dc864aeef394440c021c223d7", "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LocalFileUpload.kt": "71b363286921b21fb43bae04832ddb15627f12fbcc62c58262ce474481833658", - "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginAttemptState.kt": "5f8088ae7861aceccdadbdb0700483bf7d13710ff49ccb297d029b36ce107f45", + "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginAttemptState.kt": "1ba0d22d310b92f8b1b106851437f547f3f86a1a5530d60c2d8ae6186bbe61b4", "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginDiagnostics.kt": "4eac0172e469d673d594c9fd83a85bf275e7c945ec1bd85cf8ea18679f9444bc", "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginDiagnosticsView.kt": "42031d385d274da6a96871977511683d1842d8e92823f91de3957c3511dcf750", - "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginPolling.kt": "8e63644cc3a6eba20356477ed19f5b4277001182e31c7a977344d7e34ed8a274", + "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginPolling.kt": "49cf95635e24f3660ecc8a98dbaf83dc4855fa6140efd2e2b9d24cbb5115308a", "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/LoginScreen.kt": "3200fefe482fbc765e5e0cd4f22f837379faba038cb0c110ee1675a3f003230a", "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/MarkdownEditing.kt": "74ed912eef556a53b7eb0008946aa5b4425cb69456bea75368d309a39052a331", "ui/src/commonMain/kotlin/dev/obiente/nextcloudnative/app/MediaBackupLedger.kt": "dd48146115c292d81cb58bba56c138960499b4b5702bfc7b0401234bfdde3d30", From c22283a50f9a9eb564f38082f9e27aafef28ea91 Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Thu, 8 Oct 2026 18:56:45 +0200 Subject: [PATCH 3/6] docs(login): list invalid approval responses as uncertain results --- docs/login-diagnostics.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/login-diagnostics.md b/docs/login-diagnostics.md index 5deda07cc..04e83eeb5 100644 --- a/docs/login-diagnostics.md +++ b/docs/login-diagnostics.md @@ -44,8 +44,11 @@ Each attempt has a five-minute deadline, including network waits. If the result became uncertain after the one-time approval exchange, start a new sign-in. Do not try to replay that exchange. The app does not retry this -case automatically. A request is uncertain only when the connection failed -after the app had started sending it. +case automatically. The result is uncertain when the connection failed after +the app had started sending a request, or when the server reported an approval +but its one-time response was malformed, incomplete, or contained invalid +account values. A connection failure before the request was sent is retried +instead. ## Save a login report From 663db6dc10fa56b5f4b280ec13375080e4f26c23 Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Thu, 8 Oct 2026 20:06:30 +0200 Subject: [PATCH 4/6] fix(login): isolate and cancel each Login Flow poll The service-level poll client kept one connection pool for every attempt. Its zero idle limit only evicts idle connections, so a poll from a replacement sign-in attempt could multiplex onto the active HTTP/2 connection of a poll that was still in flight. That poll was also never cancelled: the blocking OkHttp call ignored coroutine cancellation and ran until its timeout after sign-in was cancelled. Give each poll its own client with a private pool and dispatcher, and cancel that client's calls as soon as the polling coroutine is cancelled. A call that fails because it was cancelled now propagates cancellation instead of being classified as a network result. --- .../AndroidNextcloudServices.kt | 7 +- .../488-login-poll-fresh-connections.md | 2 +- docs/login-diagnostics.md | 11 +- tools/kotlin-file-size-baseline.txt | 4 +- .../app/DesktopNextcloudServices.kt | 5 +- .../app/JvmLoginFlowTransportTest.kt | 30 ++++ .../app/JvmLoginPollConnectionTest.kt | 10 +- .../app/JvmLoginPollIsolationTest.kt | 168 ++++++++++++++++++ .../app/JvmLoginFlowTransport.kt | 43 +++++ 9 files changed, 262 insertions(+), 18 deletions(-) create mode 100644 ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt diff --git a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt index c500b54ae..9a1c60e3f 100644 --- a/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt +++ b/androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt @@ -31,7 +31,7 @@ import dev.obiente.nextcloudnative.app.LoginTransportSecurity import dev.obiente.nextcloudnative.app.LOGIN_FLOW_RESPONSE_MAX_BYTES import dev.obiente.nextcloudnative.app.LoginPollHttpResponse import dev.obiente.nextcloudnative.app.executeLoginPollHttp -import dev.obiente.nextcloudnative.app.newLoginPollHttpClient +import dev.obiente.nextcloudnative.app.withLoginPollHttpClient import dev.obiente.nextcloudnative.app.interpretLoginChallengeHttpResponse import dev.obiente.nextcloudnative.app.loginPollEndpointFallbackDiagnostic import dev.obiente.nextcloudnative.app.normalizeServerUrl @@ -348,7 +348,6 @@ internal class AndroidNextcloudServices( .useAndroidNextcloudCertificateTrust(appContext) .trackJvmNetworkFailures() .build() - private val loginPollHttpClient = httpClient.newLoginPollHttpClient() private val loginPollFallbackTokens = ConcurrentHashMap.newKeySet() private val loginPollPendingTokens = ConcurrentHashMap.newKeySet() private val noRedirectHttpClient = httpClient.newBuilder() @@ -1203,7 +1202,7 @@ internal class AndroidNextcloudServices( override suspend fun pollLogin(challenge: LoginChallenge): LoginPollResult = withContext(Dispatchers.IO) { val formBody = "token=" + URLEncoder.encode(challenge.token, StandardCharsets.UTF_8.name()) var networkFailure: JvmNetworkFailureDiagnostic? = null - val execution = executeLoginPollHttp( + val execution = withLoginPollHttpClient(httpClient) { loginPollHttpClient -> executeLoginPollHttp( challenge = challenge, fallbackAlreadySelected = challenge.token in loginPollFallbackTokens, poll = { endpoint -> @@ -1220,7 +1219,7 @@ internal class AndroidNextcloudServices( ).let { LoginPollHttpResponse(it.status, it.text) } }, networkFailure = { networkFailure }, - ) + ) } execution.selectedFallbackReason?.let { reason -> loginPollFallbackTokens += challenge.token runCatching { diff --git a/changes/unreleased/488-login-poll-fresh-connections.md b/changes/unreleased/488-login-poll-fresh-connections.md index 7a4290314..c43416d6a 100644 --- a/changes/unreleased/488-login-poll-fresh-connections.md +++ b/changes/unreleased/488-login-poll-fresh-connections.md @@ -4,4 +4,4 @@ pull: none platforms: android, desktop user-facing: yes -Keep browser sign-in working after returning to the app during approval: each check uses a new connection, and connection failures before a request is sent are retried. +Keep browser sign-in working after returning to the app during approval: each check uses a new connection, and connection failures before a request is sent are retried. Cancelling sign-in stops a check in flight. diff --git a/docs/login-diagnostics.md b/docs/login-diagnostics.md index 04e83eeb5..09d27ac0c 100644 --- a/docs/login-diagnostics.md +++ b/docs/login-diagnostics.md @@ -13,8 +13,9 @@ setup. While the app waits for browser approval or retries the connection, **Cancel sign-in** stops the attempt and lets you edit the server address -again. On Android, the system Back action does the same. Cancelling does not -revoke credentials the server has already issued. +again. On Android, the system Back action does the same. Cancelling also stops +an approval check that is still waiting for the server. It does not revoke +credentials the server has already issued. The form scrolls when a short window, landscape layout or larger text makes it taller than the screen. In landscape it keeps a bounded width. If the server @@ -36,7 +37,8 @@ the address or connection, and start a fresh attempt. Each approval check opens a new connection and closes it after the response. Leaving the app for the browser can suspend it or end its network connections. Because no connection is kept between checks, returning to the app does not -reuse one that ended while the app was in the background. +reuse one that ended while the app was in the background. A new sign-in attempt +also never shares a connection with a check from an earlier attempt. ## Time limit and uncertain results @@ -98,7 +100,8 @@ Synthetic regression tests in `LoginPollingTest` and `LoginAttemptStateTest` exercise retry phases, timeout, cancellation, browser-handoff failure, and late completion after a replacement attempt. `JvmLoginPollConnectionTest` runs approval checks against a local mock server while simulating connections that -end during a suspension. These tests run on the desktop JVM. They do not +end during a suspension. `JvmLoginPollIsolationTest` holds a check open on an +HTTP/2 mock server to cover cancellation and a replacement check. These tests run on the desktop JVM. They do not establish connectivity to any particular server or device network, and they do not reproduce Android process freezing. diff --git a/tools/kotlin-file-size-baseline.txt b/tools/kotlin-file-size-baseline.txt index 9082606fd..7f421c62c 100644 --- a/tools/kotlin-file-size-baseline.txt +++ b/tools/kotlin-file-size-baseline.txt @@ -1,4 +1,4 @@ -androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt|4130 +androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidNextcloudServices.kt|4129 androidApp/src/main/kotlin/dev/obiente/nextcloudnative/AndroidProjectContentClient.kt|985 androidApp/src/main/kotlin/dev/obiente/nextcloudnative/NextcloudDocumentsProvider.kt|900 contractAcquisition/src/main/kotlin/dev/obiente/nextcloudnative/contracts/SignedAppStoreContractAcquirer.kt|1212 @@ -48,7 +48,7 @@ ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopAppUpdates.kt|9 ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopFileReadCache.kt|1536 ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopFileSyncEngine.kt|884 ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopFileSyncRemoteTree.kt|857 -ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopNextcloudServices.kt|6169 +ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopNextcloudServices.kt|6168 ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopVirtualRangeCache.kt|2759 ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/LinuxVirtualFileSystem.kt|1697 ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/WindowsCloudFilesJna.kt|1069 diff --git a/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopNextcloudServices.kt b/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopNextcloudServices.kt index c93485f3d..ffab0d414 100644 --- a/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopNextcloudServices.kt +++ b/ui/src/desktopMain/kotlin/dev/obiente/nextcloudnative/app/DesktopNextcloudServices.kt @@ -708,7 +708,6 @@ class DesktopNextcloudServices( environment = desktopSupportDiagnosticsEnvironment(), client = httpClient.newBuilder().retryOnConnectionFailure(false).build(), ) - private val loginPollHttpClient = httpClient.newLoginPollHttpClient() private val loginPollFallbackTokens = ConcurrentHashMap.newKeySet() private val loginPollPendingTokens = ConcurrentHashMap.newKeySet() private val fileMutationHttpExecutor = DesktopHttpMutationExecutor(httpClient) @@ -4004,7 +4003,7 @@ class DesktopNextcloudServices( override suspend fun pollLogin(challenge: LoginChallenge): LoginPollResult = withContext(Dispatchers.IO) { var networkFailure: JvmNetworkFailureDiagnostic? = null - val execution = executeLoginPollHttp( + val execution = withLoginPollHttpClient(httpClient) { loginPollHttpClient -> executeLoginPollHttp( challenge = challenge, fallbackAlreadySelected = challenge.token in loginPollFallbackTokens, poll = { endpoint -> @@ -4021,7 +4020,7 @@ class DesktopNextcloudServices( ).let { LoginPollHttpResponse(it.status, it.text) } }, networkFailure = { networkFailure }, - ) + ) } execution.selectedFallbackReason?.let { reason -> loginPollFallbackTokens += challenge.token runCatching { diff --git a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransportTest.kt b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransportTest.kt index 8240c48c3..f4d4a8e10 100644 --- a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransportTest.kt +++ b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransportTest.kt @@ -2,10 +2,14 @@ package dev.obiente.nextcloudnative.app import java.io.IOException import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.async +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.job import kotlinx.coroutines.runBlocking import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith +import kotlin.test.assertFalse import kotlin.test.assertIs import kotlin.test.assertNull @@ -307,6 +311,32 @@ class JvmLoginFlowTransportTest { Unit } + @Test + fun `a call failing because its poll was cancelled is not classified as a network result`() = runBlocking { + var classified = false + val execution = async { + executeLoginPollHttp( + challenge = challenge( + pollEndpoint = "https://cloud.example.test/login/v2/poll", + fallbackEndpoint = "https://cloud.example.test/index.php/login/v2/poll", + ), + fallbackAlreadySelected = false, + poll = { + // OkHttp reports a call cancelled with its coroutine as an I/O failure. + currentCoroutineContext().job.cancel() + throw IOException("Canceled") + }, + networkFailure = { + classified = true + null + }, + ) + } + + assertFailsWith { execution.await() } + assertFalse(classified) + } + private fun challenge(pollEndpoint: String, fallbackEndpoint: String?) = LoginChallenge( enteredServerUrl = "https://cloud.example.test/nextcloud", pollEndpoint = pollEndpoint, diff --git a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt index 240330343..5f0c09830 100644 --- a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt +++ b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt @@ -68,16 +68,16 @@ class JvmLoginPollConnectionTest { server.enqueue(server.approvedResponse()) val sockets = SuspendableSocketFactory() val base = baseClient(sockets) - val pollClient = base.newLoginPollHttpClient() val challenge = challenge(server.url("/login/v2/poll").toString()) + suspend fun pollOnce() = withLoginPollHttpClient(base) { client -> poll(client, challenge) } beginLogin(base, server.url("/index.php/login/v2").toString()) sockets.suspendAll() - assertEquals(LoginPollResult.Pending, poll(pollClient, challenge)) + assertEquals(LoginPollResult.Pending, pollOnce()) sockets.suspendAll() - assertEquals(LoginPollResult.Pending, poll(pollClient, challenge)) + assertEquals(LoginPollResult.Pending, pollOnce()) sockets.suspendAll() - val approved = assertIs(poll(pollClient, challenge)) + val approved = assertIs(pollOnce()) assertEquals("person", approved.session.loginName) assertEquals(4, sockets.created.size) @@ -92,6 +92,8 @@ class JvmLoginPollConnectionTest { assertFalse(pollClient.retryOnConnectionFailure) assertNotSame(base.connectionPool, pollClient.connectionPool) + assertNotSame(base.dispatcher, pollClient.dispatcher) + assertNotSame(pollClient.connectionPool, base.newLoginPollHttpClient().connectionPool) assertEquals(0, pollClient.connectionPool.idleConnectionCount()) assertEquals(base.eventListenerFactory, pollClient.eventListenerFactory) assertEquals(base.socketFactory, pollClient.socketFactory) diff --git a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt new file mode 100644 index 000000000..b260d352b --- /dev/null +++ b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt @@ -0,0 +1,168 @@ +package dev.obiente.nextcloudnative.app + +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicReference +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.async +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeout +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFalse +import kotlin.test.assertNotEquals +import kotlin.test.assertTrue +import mockwebserver3.MockResponse +import mockwebserver3.MockWebServer +import mockwebserver3.RecordedRequest +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.OkHttpClient +import okhttp3.Protocol +import okhttp3.Request +import okhttp3.RequestBody.Companion.toRequestBody + +/** + * Covers a poll that is still in flight when its sign-in attempt is cancelled or replaced. + * + * The server holds the first poll open, so the second poll starts while the first one's HTTP/2 + * connection is still active and therefore not subject to idle-connection eviction. + */ +class JvmLoginPollIsolationTest { + @Test + fun `a shared poll client multiplexes a replacement poll onto the active connection`() = runBlocking { + withHeldFirstPoll { server, gate, base -> + val shared = base.newLoginPollHttpClient() + val first = async(Dispatchers.IO) { poll(shared, server.challenge()) } + assertTrue(gate.firstArrived.await(10, TimeUnit.SECONDS)) + + val second = withContext(Dispatchers.IO) { poll(shared, server.challenge()) } + gate.release.countDown() + + assertEquals(LoginPollResult.Pending, second) + assertEquals(LoginPollResult.Pending, first.await()) + assertEquals(1, gate.connectionIndexes.distinct().size) + } + } + + @Test + fun `a replacement poll never shares the connection of a poll still in flight`() = runBlocking { + withHeldFirstPoll { server, gate, base -> + val first = async(Dispatchers.IO) { + withLoginPollHttpClient(base) { client -> poll(client, server.challenge()) } + } + assertTrue(gate.firstArrived.await(10, TimeUnit.SECONDS)) + + val second = withContext(Dispatchers.IO) { + withLoginPollHttpClient(base) { client -> poll(client, server.challenge()) } + } + gate.release.countDown() + + assertEquals(LoginPollResult.Pending, second) + assertEquals(LoginPollResult.Pending, first.await()) + assertEquals(2, gate.connectionIndexes.size) + assertNotEquals(gate.connectionIndexes[0], gate.connectionIndexes[1]) + } + } + + @Test + fun `cancelling the poll coroutine cancels its blocked call`() = runBlocking { + withHeldFirstPoll { server, gate, base -> + val attempt = AtomicReference(null) + var completed = false + val job = launch(Dispatchers.IO) { + withLoginPollHttpClient(base) { client -> + poll(client, server.challenge(), onAttempt = attempt::set) + } + completed = true + } + assertTrue(gate.firstArrived.await(10, TimeUnit.SECONDS)) + + job.cancel() + // The server holds the response far longer than this, so only a cancelled call ends early. + withTimeout(5_000) { job.join() } + + assertTrue(job.isCancelled) + assertFalse(completed) + assertTrue(requireNotNull(attempt.get()).cancelled) + } + } + + private suspend fun withHeldFirstPoll( + test: suspend (MockWebServer, HeldFirstPoll, OkHttpClient) -> Unit, + ) { + val gate = HeldFirstPoll() + MockWebServer().use { server -> + server.protocols = listOf(Protocol.H2_PRIOR_KNOWLEDGE) + server.dispatcher = gate + server.start() + val base = OkHttpClient.Builder() + .protocols(listOf(Protocol.H2_PRIOR_KNOWLEDGE)) + .trackJvmNetworkFailures() + .build() + try { + test(server, gate, base) + } finally { + gate.release.countDown() + } + } + } + + private suspend fun poll( + client: OkHttpClient, + challenge: LoginChallenge, + onAttempt: (JvmNetworkRequestAttempt) -> Unit = {}, + ): LoginPollResult = executeLoginPollHttp( + challenge = challenge, + fallbackAlreadySelected = false, + poll = { endpoint -> + val attempt = JvmNetworkRequestAttempt().also(onAttempt) + val request = Request.Builder() + .url(endpoint) + .post("token=synthetic-one-time-token".toRequestBody(FORM)) + .tag(JvmNetworkRequestAttempt::class.java, attempt) + .build() + client.newCall(request).execute().use { response -> + LoginPollHttpResponse(response.code, response.body.string()) + } + }, + networkFailure = { null }, + ).interpretation.result + + private fun MockWebServer.challenge(): LoginChallenge { + val pollEndpoint = url("/login/v2/poll").toString() + return LoginChallenge( + enteredServerUrl = pollEndpoint.substringBefore("/login/v2/poll"), + pollEndpoint = pollEndpoint, + pollFallbackEndpoint = null, + token = "synthetic-one-time-token", + loginUrl = pollEndpoint.replace("/poll", "/flow/synthetic"), + transportSecurity = LoginTransportSecurity.PlainHttp, + ) + } + + private companion object { + val FORM = "application/x-www-form-urlencoded".toMediaType() + } +} + +/** Answers every poll as pending, but holds the first one until the test releases it. */ +private class HeldFirstPoll : mockwebserver3.Dispatcher() { + val firstArrived = CountDownLatch(1) + val release = CountDownLatch(1) + val connectionIndexes = CopyOnWriteArrayList() + + override fun dispatch(request: RecordedRequest): MockResponse { + val isFirst = synchronized(connectionIndexes) { + connectionIndexes += request.connectionIndex + connectionIndexes.size == 1 + } + if (isFirst) { + firstArrived.countDown() + release.await(30, TimeUnit.SECONDS) + } + return MockResponse.Builder().code(404).body("[]").build() + } +} diff --git a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt index 01524ec1f..40c038dbd 100644 --- a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt +++ b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt @@ -2,7 +2,15 @@ package dev.obiente.nextcloudnative.app import java.util.concurrent.TimeUnit import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineStart +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.awaitCancellation +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.currentCoroutineContext +import kotlinx.coroutines.ensureActive +import kotlinx.coroutines.launch import okhttp3.ConnectionPool +import okhttp3.Dispatcher import okhttp3.OkHttpClient data class LoginPollHttpResponse( @@ -34,12 +42,45 @@ data class LoginPollHttpExecution( * lost one-time approval response. Each poll therefore opens a new connection and closes it * after the response, so no connection outlives the wait between polls. Failures while that * connection is being established remain provably pre-exchange. + * + * The pool and dispatcher belong to this client alone. Another client's in-flight poll, such as + * one from a cancelled or replaced sign-in attempt, can therefore never share its HTTP/2 + * connection, and cancelling this client's calls cannot affect unrelated requests. A dispatcher + * that only runs synchronous calls starts no threads. */ fun OkHttpClient.newLoginPollHttpClient(): OkHttpClient = newBuilder() .retryOnConnectionFailure(false) .connectionPool(ConnectionPool(0, 1, TimeUnit.SECONDS)) + .dispatcher(Dispatcher()) .build() +/** + * Runs one poll with a new [newLoginPollHttpClient] derived from [baseClient]. + * + * OkHttp's blocking calls do not observe coroutine cancellation, so the client's calls are + * cancelled as soon as the calling coroutine is cancelled. A cancelled or replaced sign-in attempt + * therefore cannot leave a poll running against the server. + */ +suspend fun withLoginPollHttpClient( + baseClient: OkHttpClient, + block: suspend (OkHttpClient) -> T, +): T = coroutineScope { + val client = baseClient.newLoginPollHttpClient() + // Runs on its own dispatcher so cancellation is delivered while [block] blocks a thread. + val cancellation = launch(Dispatchers.IO, start = CoroutineStart.UNDISPATCHED) { + try { + awaitCancellation() + } finally { + client.dispatcher.cancelAll() + } + } + try { + block(client) + } finally { + cancellation.cancel() + } +} + suspend fun executeLoginPollHttp( challenge: LoginChallenge, fallbackAlreadySelected: Boolean, @@ -62,6 +103,8 @@ suspend fun executeLoginPollHttp( poll(endpoint) } catch (failure: Throwable) { if (failure is CancellationException) throw failure + // A call cancelled with its coroutine fails as an I/O error; it is not a network outcome. + currentCoroutineContext().ensureActive() throw LoginPollRequestFailure(classifyLoginPollNetworkFailure(networkFailure()), failure) } From bd6a4fa03b5a4dd240f1d8e87baed5d80bb226f1 Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Thu, 8 Oct 2026 21:21:45 +0200 Subject: [PATCH 5/6] fix(login): tie poll cancellation to each concrete call Cancelling a poll swept the dispatcher's running calls once. A call that registered just after that sweep was never cancelled, and the blocked thread could still send the poll and wait for its response. Register every call of a poll in a first application interceptor that shares a lock with cancellation. A call registered before cancellation is cancelled by it, and a call that starts afterwards is cancelled before any network work. Calls stay registered for the poll's lifetime so a response body that is still being read is also stopped. --- .../app/JvmLoginPollConnectionTest.kt | 1 - .../app/JvmLoginPollIsolationTest.kt | 82 +++++++++++++++++-- .../app/JvmLoginFlowTransport.kt | 66 ++++++++++++--- 3 files changed, 129 insertions(+), 20 deletions(-) diff --git a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt index 5f0c09830..7388bbd6f 100644 --- a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt +++ b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollConnectionTest.kt @@ -92,7 +92,6 @@ class JvmLoginPollConnectionTest { assertFalse(pollClient.retryOnConnectionFailure) assertNotSame(base.connectionPool, pollClient.connectionPool) - assertNotSame(base.dispatcher, pollClient.dispatcher) assertNotSame(pollClient.connectionPool, base.newLoginPollHttpClient().connectionPool) assertEquals(0, pollClient.connectionPool.idleConnectionCount()) assertEquals(base.eventListenerFactory, pollClient.eventListenerFactory) diff --git a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt index b260d352b..e73ce9138 100644 --- a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt +++ b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt @@ -1,18 +1,22 @@ package dev.obiente.nextcloudnative.app +import java.io.IOException import java.util.concurrent.CopyOnWriteArrayList import java.util.concurrent.CountDownLatch import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicReference import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.async +import kotlinx.coroutines.job import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeout import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFailsWith import kotlin.test.assertFalse +import kotlin.test.assertIs import kotlin.test.assertNotEquals import kotlin.test.assertTrue import mockwebserver3.MockResponse @@ -90,6 +94,77 @@ class JvmLoginPollIsolationTest { } } + @Test + fun `a call that starts after cancellation is never sent`() = runBlocking { + withHeldFirstPoll { server, gate, base -> + val calls = LoginPollCallCancellation() + val client = base.newBuilder().addInterceptor(calls).build() + calls.cancel() + + val attempt = JvmNetworkRequestAttempt() + assertFailsWith { + client.newCall(pollRequest(server.challenge().pollEndpoint, attempt)).execute() + } + + assertTrue(attempt.cancelled) + assertFalse(attempt.exchangeStarted) + assertTrue(gate.connectionIndexes.isEmpty()) + assertEquals(0, server.requestCount) + } + } + + @Test + fun `a call registered before cancellation is cancelled while blocked`() = runBlocking { + withHeldFirstPoll { server, gate, base -> + val calls = LoginPollCallCancellation() + val client = base.newBuilder().addInterceptor(calls).build() + val attempt = JvmNetworkRequestAttempt() + val blocked = async(Dispatchers.IO) { + runCatching { + client.newCall(pollRequest(server.challenge().pollEndpoint, attempt)).execute().close() + } + } + assertTrue(gate.firstArrived.await(10, TimeUnit.SECONDS)) + + calls.cancel() + val outcome = withTimeout(5_000) { blocked.await() } + + assertIs(outcome.exceptionOrNull()) + assertTrue(attempt.cancelled) + } + } + + @Test + fun `cancellation just before a poll is sent stops it in either order`() = runBlocking { + withHeldFirstPoll { server, gate, base -> + val attempt = AtomicReference(null) + var completed = false + val job = launch(Dispatchers.IO) { + val attemptJob = coroutineContext.job + withLoginPollHttpClient(base) { client -> + // The cancellation sweep races the call's registration from here on. + attemptJob.cancel() + poll(client, server.challenge(), onAttempt = attempt::set) + } + completed = true + } + + // The server would hold a sent poll far longer than this. + withTimeout(5_000) { job.join() } + + assertTrue(job.isCancelled) + assertFalse(completed) + val sent = gate.connectionIndexes.isNotEmpty() + assertTrue(!sent || requireNotNull(attempt.get()).cancelled) + } + } + + private fun pollRequest(endpoint: String, attempt: JvmNetworkRequestAttempt): Request = Request.Builder() + .url(endpoint) + .post("token=synthetic-one-time-token".toRequestBody(FORM)) + .tag(JvmNetworkRequestAttempt::class.java, attempt) + .build() + private suspend fun withHeldFirstPoll( test: suspend (MockWebServer, HeldFirstPoll, OkHttpClient) -> Unit, ) { @@ -119,12 +194,7 @@ class JvmLoginPollIsolationTest { fallbackAlreadySelected = false, poll = { endpoint -> val attempt = JvmNetworkRequestAttempt().also(onAttempt) - val request = Request.Builder() - .url(endpoint) - .post("token=synthetic-one-time-token".toRequestBody(FORM)) - .tag(JvmNetworkRequestAttempt::class.java, attempt) - .build() - client.newCall(request).execute().use { response -> + client.newCall(pollRequest(endpoint, attempt)).execute().use { response -> LoginPollHttpResponse(response.code, response.body.string()) } }, diff --git a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt index 40c038dbd..39ea20564 100644 --- a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt +++ b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt @@ -1,5 +1,6 @@ package dev.obiente.nextcloudnative.app +import java.io.IOException import java.util.concurrent.TimeUnit import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineStart @@ -9,9 +10,11 @@ import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.ensureActive import kotlinx.coroutines.launch +import okhttp3.Call import okhttp3.ConnectionPool -import okhttp3.Dispatcher +import okhttp3.Interceptor import okhttp3.OkHttpClient +import okhttp3.Response data class LoginPollHttpResponse( val status: Int, @@ -43,35 +46,37 @@ data class LoginPollHttpExecution( * after the response, so no connection outlives the wait between polls. Failures while that * connection is being established remain provably pre-exchange. * - * The pool and dispatcher belong to this client alone. Another client's in-flight poll, such as - * one from a cancelled or replaced sign-in attempt, can therefore never share its HTTP/2 - * connection, and cancelling this client's calls cannot affect unrelated requests. A dispatcher - * that only runs synchronous calls starts no threads. + * The pool belongs to this client alone. Another client's in-flight poll, such as one from a + * cancelled or replaced sign-in attempt, can therefore never share its HTTP/2 connection. */ -fun OkHttpClient.newLoginPollHttpClient(): OkHttpClient = newBuilder() +fun OkHttpClient.newLoginPollHttpClient(): OkHttpClient = loginPollHttpClientBuilder().build() + +private fun OkHttpClient.loginPollHttpClientBuilder(): OkHttpClient.Builder = newBuilder() .retryOnConnectionFailure(false) .connectionPool(ConnectionPool(0, 1, TimeUnit.SECONDS)) - .dispatcher(Dispatcher()) - .build() /** * Runs one poll with a new [newLoginPollHttpClient] derived from [baseClient]. * - * OkHttp's blocking calls do not observe coroutine cancellation, so the client's calls are - * cancelled as soon as the calling coroutine is cancelled. A cancelled or replaced sign-in attempt - * therefore cannot leave a poll running against the server. + * OkHttp's blocking calls do not observe coroutine cancellation. Every call made through the + * client is therefore registered with a [LoginPollCallCancellation] that the calling coroutine's + * cancellation triggers, so a cancelled or replaced sign-in attempt cannot send a poll or leave + * one running against the server. */ suspend fun withLoginPollHttpClient( baseClient: OkHttpClient, block: suspend (OkHttpClient) -> T, ): T = coroutineScope { - val client = baseClient.newLoginPollHttpClient() + val calls = LoginPollCallCancellation() + val client = baseClient.loginPollHttpClientBuilder() + .apply { interceptors().add(0, calls) } + .build() // Runs on its own dispatcher so cancellation is delivered while [block] blocks a thread. val cancellation = launch(Dispatchers.IO, start = CoroutineStart.UNDISPATCHED) { try { awaitCancellation() } finally { - client.dispatcher.cancelAll() + calls.cancel() } } try { @@ -81,6 +86,41 @@ suspend fun withLoginPollHttpClient( } } +/** + * Cancels every call of one poll, including calls that start after cancellation was requested. + * + * As the first application interceptor it sees each concrete [Call] before any network work. + * Registration and cancellation share one lock: a call registered before [cancel] is cancelled by + * it, and a call that arrives afterwards is cancelled before its request can be sent. Calls stay + * registered for the poll's lifetime so cancellation also stops a response body still being read. + */ +internal class LoginPollCallCancellation : Interceptor { + private val lock = Any() + private var cancelled = false + private val calls = mutableListOf() + + fun cancel() { + val registered = synchronized(lock) { + cancelled = true + calls.toList() + } + registered.forEach(Call::cancel) + } + + override fun intercept(chain: Interceptor.Chain): Response { + val call = chain.call() + val alreadyCancelled = synchronized(lock) { + if (!cancelled) calls += call + cancelled + } + if (alreadyCancelled) { + call.cancel() + throw IOException("Canceled") + } + return chain.proceed(chain.request()) + } +} + suspend fun executeLoginPollHttp( challenge: LoginChallenge, fallbackAlreadySelected: Boolean, From 853771fa3fa874e81a3e6a9c96ead0f6d54dd3ad Mon Sep 17 00:00:00 2001 From: veryCrunchy Date: Thu, 8 Oct 2026 22:07:04 +0200 Subject: [PATCH 6/6] fix(login): cancel poll calls without a dispatcher hop The poll cancellation watcher resumed on Dispatchers.IO before cancelling its calls. When blocking work saturated that dispatcher, a dismissed or replaced sign-in could keep its poll running until an IO thread became free. Register the cancellation with the watcher's continuation instead, so the thread that cancels the attempt cancels the calls synchronously. Add Android instrumented coverage for new TLS connections per poll, cancellation of a held poll, and retryable refused connections. Update the browser sign-in architecture contract to the current pre-exchange retry boundary and poll isolation rules. --- ADAPTER_ARCHITECTURE.md | 15 +- ...droidLoginPollTransportInstrumentedTest.kt | 210 ++++++++++++++++++ docs/login-diagnostics.md | 10 +- .../app/JvmLoginPollIsolationTest.kt | 2 + .../app/JvmLoginFlowTransport.kt | 15 +- 5 files changed, 239 insertions(+), 13 deletions(-) create mode 100644 androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidLoginPollTransportInstrumentedTest.kt diff --git a/ADAPTER_ARCHITECTURE.md b/ADAPTER_ARCHITECTURE.md index c84f06dee..cb892ca2b 100644 --- a/ADAPTER_ARCHITECTURE.md +++ b/ADAPTER_ARCHITECTURE.md @@ -294,9 +294,18 @@ retrying a connection, and completing sign-in. - Cancellation releases challenge bookkeeping and rejects late results before they can persist credentials. The existing authenticated-session transition still owns the final credential commit. -- Only a classified DNS failure before an HTTP exchange may retry - automatically. An ambiguous response after the one-time exchange requires a - new sign-in attempt. +- Only a classified failure while the connection is being established, before + an HTTP exchange starts, may retry automatically: unresolved DNS, a refused + or timed-out connection, an unreachable network, a TLS timeout, or a socket + failure during DNS lookup or connect. Certificate, TLS handshake, protocol, + and unclassified failures end the attempt. +- A failure after the one-time exchange starts, or an invalid one-time approval + response, is ambiguous. It is never retried and requires a new sign-in + attempt. +- Each poll uses its own HTTP client with OkHttp connection-failure retry + disabled and a connection pool that keeps no idle connection, so a poll never + reuses a connection from an earlier poll or attempt. Cancelling the attempt + cancels the poll's calls, including one that has not been sent yet. - Retry copy describes the last observed failure. It cannot report the current health of DNS, the server, or a VPN. diff --git a/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidLoginPollTransportInstrumentedTest.kt b/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidLoginPollTransportInstrumentedTest.kt new file mode 100644 index 000000000..c22890db5 --- /dev/null +++ b/androidApp/src/androidTest/kotlin/dev/obiente/nextcloudnative/AndroidLoginPollTransportInstrumentedTest.kt @@ -0,0 +1,210 @@ +package dev.obiente.nextcloudnative + +import androidx.test.ext.junit.runners.AndroidJUnit4 +import dev.obiente.nextcloudnative.app.JvmNetworkFailureDiagnostic +import dev.obiente.nextcloudnative.app.JvmNetworkRequestAttempt +import dev.obiente.nextcloudnative.app.LoginChallenge +import dev.obiente.nextcloudnative.app.LoginPollHttpResponse +import dev.obiente.nextcloudnative.app.LoginPollResult +import dev.obiente.nextcloudnative.app.executeLoginPollHttp +import dev.obiente.nextcloudnative.app.toJvmNetworkFailureDiagnostic +import dev.obiente.nextcloudnative.app.trackJvmNetworkFailures +import dev.obiente.nextcloudnative.app.withLoginPollHttpClient +import java.io.IOException +import java.net.ServerSocket +import java.util.concurrent.CopyOnWriteArrayList +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import java.util.concurrent.atomic.AtomicReference +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.launch +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import mockwebserver3.Dispatcher +import mockwebserver3.MockResponse +import mockwebserver3.MockWebServer +import mockwebserver3.RecordedRequest +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.RequestBody.Companion.toRequestBody +import okhttp3.tls.HandshakeCertificates +import okhttp3.tls.HeldCertificate +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith + +/** + * Runs Login Flow v2 poll isolation and cancellation on the Android runtime and its TLS stack. + * + * All servers are local synthetic fixtures; no request leaves the device. + */ +@RunWith(AndroidJUnit4::class) +class AndroidLoginPollTransportInstrumentedTest { + private val certificate = HeldCertificate.Builder() + .commonName("localhost") + .addSubjectAlternativeName("localhost") + .build() + + @Test + fun everyPollUsesANewTlsConnectionUntilApproval() = runBlocking { + val connections = CopyOnWriteArrayList() + withServer( + dispatcher = { request, server -> + connections += request.connectionIndex + if (connections.size < 3) pending() else server.approved() + }, + ) { server, base -> + val challenge = server.challenge() + val results = List(3) { + withLoginPollHttpClient(base) { client -> poll(client, challenge) } + } + + assertEquals(LoginPollResult.Pending, results[0]) + assertEquals(LoginPollResult.Pending, results[1]) + assertTrue(results[2] is LoginPollResult.Approved) + assertEquals(3, connections.distinct().size) + } + } + + @Test + fun cancellingTheAttemptCancelsAPollHeldByTheServer() = runBlocking { + val arrived = CountDownLatch(1) + val release = CountDownLatch(1) + withServer( + dispatcher = { _, _ -> + arrived.countDown() + release.await(30, TimeUnit.SECONDS) + pending() + }, + onClose = release::countDown, + ) { server, base -> + val attempt = AtomicReference(null) + var completed = false + val job = launch(Dispatchers.IO) { + withLoginPollHttpClient(base) { client -> + poll(client, server.challenge(), onAttempt = attempt::set) + } + completed = true + } + assertTrue(arrived.await(10, TimeUnit.SECONDS)) + + job.cancel() + assertTrue(requireNotNull(attempt.get()).cancelled) + withTimeout(5_000) { job.join() } + + assertTrue(job.isCancelled) + assertFalse(completed) + } + } + + @Test + fun refusedConnectionBeforeTheExchangeIsRetryable() = runBlocking { + val closedPort = ServerSocket(0).use { it.localPort } + val pollEndpoint = "https://localhost:$closedPort/login/v2/poll" + val challenge = LoginChallenge( + enteredServerUrl = "https://localhost:$closedPort", + pollEndpoint = pollEndpoint, + pollFallbackEndpoint = null, + token = "synthetic-one-time-token", + loginUrl = "https://localhost:$closedPort/login/v2/flow/synthetic", + ) + + val result = withLoginPollHttpClient(trustingClient()) { client -> poll(client, challenge) } + + assertTrue(result is LoginPollResult.RetryablePreExchangeFailure) + assertEquals("NETWORK_CONNECT_FAILED", (result as LoginPollResult.RetryablePreExchangeFailure).code) + } + + private suspend fun withServer( + dispatcher: (RecordedRequest, MockWebServer) -> MockResponse, + onClose: () -> Unit = {}, + test: suspend (MockWebServer, OkHttpClient) -> Unit, + ) { + MockWebServer().use { server -> + server.useHttps( + HandshakeCertificates.Builder().heldCertificate(certificate).build().sslSocketFactory(), + ) + server.dispatcher = object : Dispatcher() { + override fun dispatch(request: RecordedRequest): MockResponse = dispatcher(request, server) + } + server.start() + try { + test(server, trustingClient()) + } finally { + onClose() + } + } + } + + private fun trustingClient(): OkHttpClient { + val trusted = HandshakeCertificates.Builder() + .addTrustedCertificate(certificate.certificate) + .build() + return OkHttpClient.Builder() + .sslSocketFactory(trusted.sslSocketFactory(), trusted.trustManager) + .trackJvmNetworkFailures() + .build() + } + + private suspend fun poll( + client: OkHttpClient, + challenge: LoginChallenge, + onAttempt: (JvmNetworkRequestAttempt) -> Unit = {}, + ): LoginPollResult { + var networkFailure: JvmNetworkFailureDiagnostic? = null + return executeLoginPollHttp( + challenge = challenge, + fallbackAlreadySelected = false, + poll = { endpoint -> + networkFailure = null + val attempt = JvmNetworkRequestAttempt().also(onAttempt) + val request = Request.Builder() + .url(endpoint) + .post("token=synthetic-one-time-token".toRequestBody(FORM)) + .tag(JvmNetworkRequestAttempt::class.java, attempt) + .build() + try { + client.newCall(request).execute().use { response -> + LoginPollHttpResponse(response.code, response.body.string()) + } + } catch (failure: IOException) { + networkFailure = failure.toJvmNetworkFailureDiagnostic( + attempt = attempt, + readOnlyRequest = false, + replayableRequest = false, + ) + throw failure + } + }, + networkFailure = { networkFailure }, + ).interpretation.result + } + + private fun MockWebServer.challenge(): LoginChallenge { + val origin = url("/").toString().trimEnd('/') + return LoginChallenge( + enteredServerUrl = origin, + pollEndpoint = "$origin/login/v2/poll", + pollFallbackEndpoint = null, + token = "synthetic-one-time-token", + loginUrl = "$origin/login/v2/flow/synthetic", + ) + } + + private fun pending() = MockResponse.Builder().code(404).body("[]").build() + + private fun MockWebServer.approved() = MockResponse.Builder() + .code(200) + .body( + """{"server":"${url("/").toString().trimEnd('/')}","loginName":"person",""" + + """"appPassword":"synthetic-app-password"}""", + ) + .build() + + private companion object { + val FORM = "application/x-www-form-urlencoded".toMediaType() + } +} diff --git a/docs/login-diagnostics.md b/docs/login-diagnostics.md index 09d27ac0c..c3fad7475 100644 --- a/docs/login-diagnostics.md +++ b/docs/login-diagnostics.md @@ -101,9 +101,13 @@ exercise retry phases, timeout, cancellation, browser-handoff failure, and late completion after a replacement attempt. `JvmLoginPollConnectionTest` runs approval checks against a local mock server while simulating connections that end during a suspension. `JvmLoginPollIsolationTest` holds a check open on an -HTTP/2 mock server to cover cancellation and a replacement check. These tests run on the desktop JVM. They do not -establish connectivity to any particular server or device network, and they do -not reproduce Android process freezing. +HTTP/2 mock server to cover cancellation and a replacement check. These tests +run on the desktop JVM. `AndroidLoginPollTransportInstrumentedTest` repeats the +new-connection, cancellation and refused-connection checks against a local TLS +mock server on the Android runtime. It runs only through the instrumented +emulator workflow in [CONTRIBUTING.md](../CONTRIBUTING.md), not in pull request +CI. None of these tests establish connectivity to any particular server or +device network, and they do not reproduce Android process freezing. ## Evidence and release status diff --git a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt index e73ce9138..9068c3a7e 100644 --- a/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt +++ b/ui/src/desktopTest/kotlin/dev/obiente/nextcloudnative/app/JvmLoginPollIsolationTest.kt @@ -85,6 +85,8 @@ class JvmLoginPollIsolationTest { assertTrue(gate.firstArrived.await(10, TimeUnit.SECONDS)) job.cancel() + // The cancelling thread cancels the call itself, without waiting for a dispatcher thread. + assertTrue(requireNotNull(attempt.get()).cancelled) // The server holds the response far longer than this, so only a cancelled call ends early. withTimeout(5_000) { job.join() } diff --git a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt index 39ea20564..87f556b36 100644 --- a/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt +++ b/ui/src/jvmMain/kotlin/dev/obiente/nextcloudnative/app/JvmLoginFlowTransport.kt @@ -5,11 +5,11 @@ import java.util.concurrent.TimeUnit import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineStart import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.awaitCancellation import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.ensureActive import kotlinx.coroutines.launch +import kotlinx.coroutines.suspendCancellableCoroutine import okhttp3.Call import okhttp3.ConnectionPool import okhttp3.Interceptor @@ -62,6 +62,10 @@ private fun OkHttpClient.loginPollHttpClientBuilder(): OkHttpClient.Builder = ne * client is therefore registered with a [LoginPollCallCancellation] that the calling coroutine's * cancellation triggers, so a cancelled or replaced sign-in attempt cannot send a poll or leave * one running against the server. + * + * The calls are cancelled synchronously by the thread that cancels the coroutine. Cancellation + * never waits for a dispatcher thread, so it still works while blocking work saturates + * [kotlinx.coroutines.Dispatchers.IO]. */ suspend fun withLoginPollHttpClient( baseClient: OkHttpClient, @@ -71,12 +75,9 @@ suspend fun withLoginPollHttpClient( val client = baseClient.loginPollHttpClientBuilder() .apply { interceptors().add(0, calls) } .build() - // Runs on its own dispatcher so cancellation is delivered while [block] blocks a thread. - val cancellation = launch(Dispatchers.IO, start = CoroutineStart.UNDISPATCHED) { - try { - awaitCancellation() - } finally { - calls.cancel() + val cancellation = launch(Dispatchers.Unconfined, start = CoroutineStart.UNDISPATCHED) { + suspendCancellableCoroutine { continuation -> + continuation.invokeOnCancellation { calls.cancel() } } } try {