diff --git a/src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnection.kt b/src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnection.kt index 8eb538ef..6745033c 100644 --- a/src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnection.kt +++ b/src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnection.kt @@ -54,7 +54,15 @@ import java.util.concurrent.atomic.AtomicBoolean * Still clear [DevSpacesContext.activeWorkspaces] when the connection ends (before optional * remote stop) for tooltips / bookkeeping. */ +class ConnectWaitTimeoutException(message: String) : IllegalStateException(message) + class DevSpacesConnection(private val devSpacesContext: DevSpacesContext) { + + companion object { + const val CONNECT_TIMEOUT: Long = 120 * 1000 // millis + private const val CONNECT_POLL: Long = 200 // millis + } + /** Ensures [tearDownConnection] runs at most once for this connect attempt. */ private val tearDownStarted = AtomicBoolean(false) @@ -87,21 +95,21 @@ class DevSpacesConnection(private val devSpacesContext: DevSpacesContext) { checkCancelled?.invoke() onProgress?.invoke(ProgressCountdown.ProgressEvent( - message = "Waiting for the workspace IDE client to start...")) + message = "Waiting for the workspace IDE client to start (first-time download may take several minutes)...")) val (fwd, localPort) = setupPortForwarding(remoteIdeServer.pod) forwarder = fwd val effectiveJoinLink = joinLink.replace(":5990", ":$localPort") - val connectWaitDone = AtomicBoolean(false) + val connectFailed = AtomicBoolean(false) checkCancelled?.invoke() client = startThinClient( URI(effectiveJoinLink), workspace, onConnected, onConnectionEnded, onDevWorkspaceStopped, - remoteIdeServer, forwarder, connectWaitDone, connectionLive + remoteIdeServer, forwarder, connectFailed, connectionLive ) - waitForThinClientConnect(client, connectWaitDone, checkCancelled) + waitForThinClientConnect(client, connectFailed, checkCancelled) if (registerRestartWatcher == true) { watchRestartAnnotation( @@ -116,7 +124,9 @@ class DevSpacesConnection(private val devSpacesContext: DevSpacesContext) { onConnected() client } catch (e: Exception) { - runCatching { client?.close() } + if (e !is ConnectWaitTimeoutException || connectionLive.get()) { + runCatching { client?.close() } + } tearDownConnection( client, workspace, onConnectionEnded, onDevWorkspaceStopped, remoteIdeServer, forwarder ) @@ -130,8 +140,8 @@ class DevSpacesConnection(private val devSpacesContext: DevSpacesContext) { * already live; failures during connect are cleaned up by [connect]'s catch. */ @Suppress("UnstableApiUsage") - private fun onThinClientClosed( - connectWaitDone: AtomicBoolean, + internal fun onThinClientClosed( + connectFailed: AtomicBoolean, connectionLive: AtomicBoolean, thinClient: ThinClientHandle, workspace: DevWorkspace, @@ -140,7 +150,7 @@ class DevSpacesConnection(private val devSpacesContext: DevSpacesContext) { remoteIdeServer: RemoteIDEServer?, forwarder: Closeable?, ) { - connectWaitDone.set(true) + connectFailed.set(true) if (connectionLive.get()) { tearDownConnection( thinClient, @@ -215,11 +225,10 @@ class DevSpacesConnection(private val devSpacesContext: DevSpacesContext) { val workspacePatch = DevWorkspacePatch( workspace.namespace, workspace.name, - devSpacesContext.client, - { - DevWorkspaces(devSpacesContext.client).get(workspace.namespace, workspace.name) - } - ) + devSpacesContext.client + ) { + DevWorkspaces(devSpacesContext.client).get(workspace.namespace, workspace.name) + } try { if (workspacePatch.hasRestartAnnotation()) { closeAllProjects() @@ -339,7 +348,7 @@ class DevSpacesConnection(private val devSpacesContext: DevSpacesContext) { onDevWorkspaceStopped: () -> Unit, remoteIdeServer: RemoteIDEServer?, forwarder: Closeable?, - connectWaitDone: AtomicBoolean, + connectFailed: AtomicBoolean, connectionLive: AtomicBoolean, ): ThinClientHandle { val thinClient = LinkedClientManager @@ -352,11 +361,9 @@ class DevSpacesConnection(private val devSpacesContext: DevSpacesContext) { false ) - thinClient.onClientPresenceChanged.advise(thinClient.lifetime) { connectWaitDone.set(true) } - fun notifyThinClientClosed() { onThinClientClosed( - connectWaitDone, + connectFailed, connectionLive, thinClient, workspace, @@ -372,22 +379,25 @@ class DevSpacesConnection(private val devSpacesContext: DevSpacesContext) { return thinClient } - private suspend fun waitForThinClientConnect( + @Suppress("UnstableApiUsage") + internal suspend fun waitForThinClientConnect( thinClient: ThinClientHandle, - connectWaitDone: AtomicBoolean, - checkCancelled: (() -> Unit)? + connectFailed: AtomicBoolean, + checkCancelled: (() -> Unit)?, + timeoutMs: Long = CONNECT_TIMEOUT ) { @Suppress("ConvertLongToDuration") - val success = withTimeoutOrNull(60_000L) { - while (!connectWaitDone.get()) { + val connected = withTimeoutOrNull(timeoutMs) { + // Keep polling while the client is not present and no failure was reported: + // a transient absence must not fail the wait + while (!thinClient.clientPresent && !connectFailed.get()) { checkCancelled?.invoke() - delay(200L) + delay(CONNECT_POLL) } - true + thinClient.clientPresent && !connectFailed.get() } ?: false - - check(success && thinClient.clientPresent) { - "Could not connect, workspace IDE is not ready." + if (!connected) { + throw ConnectWaitTimeoutException("Could not connect, workspace IDE is not ready.") } } } diff --git a/src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnectionProvider.kt b/src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnectionProvider.kt index 1a5e1bd3..06fa4355 100644 --- a/src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnectionProvider.kt +++ b/src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnectionProvider.kt @@ -99,7 +99,7 @@ class DevSpacesConnectionProvider : GatewayConnectionProvider { } runBlocking { - withTimeoutOrNull(60_000L) { ready.await() } ?: run { + withTimeoutOrNull(DevSpacesConnection.CONNECT_TIMEOUT) { ready.await() } ?: run { if (ready.isActive) { indicator.text = "Workspace IDE did not report readiness in time." ready.completeExceptionally( diff --git a/src/test/kotlin/com/redhat/devtools/gateway/devworkspace/DevSpacesConnectionTest.kt b/src/test/kotlin/com/redhat/devtools/gateway/devworkspace/DevSpacesConnectionTest.kt new file mode 100644 index 00000000..5a88c462 --- /dev/null +++ b/src/test/kotlin/com/redhat/devtools/gateway/devworkspace/DevSpacesConnectionTest.kt @@ -0,0 +1,164 @@ +/* + * Copyright (c) 2026 Red Hat, Inc. + * This program and the accompanying materials are made + * available under the terms of the Eclipse Public License 2.0 + * which is available at https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Red Hat, Inc. - initial API and implementation + */ +package com.redhat.devtools.gateway.devworkspace + +import com.jetbrains.gateway.thinClientLink.ThinClientHandle +import com.redhat.devtools.gateway.DevSpacesConnection +import com.redhat.devtools.gateway.DevSpacesContext +import io.mockk.* +import kotlinx.coroutines.test.runTest +import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import java.util.concurrent.atomic.AtomicBoolean + +class DevSpacesConnectionTest { + + private lateinit var devSpacesContext: DevSpacesContext + private lateinit var thinClient: ThinClientHandle + + private val namespace = "test-namespace" + private val workspaceName = "test-workspace" + + private lateinit var connection: DevSpacesConnection + + @BeforeEach + fun beforeEach() { + devSpacesContext = mockk(relaxed = true) { + every { devWorkspace.namespace } returns namespace + every { devWorkspace.name } returns workspaceName + } + thinClient = mockk(relaxed = true) { + every { clientPresent } returns false + every { lifetime } returns mockk(relaxed = true) + } + + connection = DevSpacesConnection(devSpacesContext) + + // Mock DevWorkspaces.get() for tearDownConnection's DevWorkspacePatch + mockkConstructor(DevWorkspaces::class) + every { anyConstructed().get(any(), any()) } returns mockk(relaxed = true) { + every { annotations } returns emptyMap() + } + } + + @Test + fun `waitForThinClientConnect succeeds when client is present`() = runTest { + // given + val connectFailed = AtomicBoolean(false) + every { thinClient.clientPresent } returns true + + // when + connection.waitForThinClientConnect(thinClient, connectFailed, null) + + // then — no exception means success + } + + @Test + fun `waitForThinClientConnect times out when client is never present`() = runTest { + // given + val connectFailed = AtomicBoolean(false) + every { thinClient.clientPresent } returns false + + // when/then — short timeout so the test completes quickly + var thrown: Throwable? = null + try { + connection.waitForThinClientConnect(thinClient, connectFailed, null, timeoutMs = 500L) + } catch (e: Throwable) { + thrown = e + } + assertThat(thrown).isInstanceOf(IllegalStateException::class.java) + assertThat(thrown?.message).contains("Could not connect") + } + + @Test + fun `waitForThinClientConnect tolerates transient presence absence`() = runTest { + // given — simulate transient absence: absent for first N polls, then present + val connectFailed = AtomicBoolean(false) + var callCount = 0 + every { thinClient.clientPresent } answers { + callCount++ + callCount > 3 + } + + // when — generous timeout so the loop can recover + connection.waitForThinClientConnect(thinClient, connectFailed, null, timeoutMs = 10_000L) + + // then — loop polled at least 4 times (3 absent + 1 present) + assertThat(callCount).isGreaterThan(3) + } + + @Test + fun `waitForThinClientConnect fails when connectFailed is set`() = runTest { + // given — connectFailed already set before calling waitForThinClientConnect + val connectFailed = AtomicBoolean(true) + every { thinClient.clientPresent } returns false + + // when/then + var thrown: Throwable? = null + try { + connection.waitForThinClientConnect(thinClient, connectFailed, null, timeoutMs = 1_000L) + } catch (e: Throwable) { + thrown = e + } + assertThat(thrown).isInstanceOf(IllegalStateException::class.java) + assertThat(thrown?.message).contains("Could not connect") + } + + // -- onThinClientClosed tests (covers: onThinClientClosed) -- + + @Test + fun `onThinClientClosed sets connectFailed and tears down when live`() { + // given + val connectFailed = AtomicBoolean(false) + val connectionLive = AtomicBoolean(true) + + // when + connection.onThinClientClosed( + connectFailed, + connectionLive, + thinClient, + devSpacesContext.devWorkspace, + mockk<() -> Unit>(relaxed = true), + {}, + null, + null + ) + + // then + assertThat(connectFailed.get()).isTrue() + verify { devSpacesContext.removeWorkspace(devSpacesContext.devWorkspace) } + } + + @Test + fun `onThinClientClosed does not tear down when not live`() { + // given + val connectFailed = AtomicBoolean(false) + val connectionLive = AtomicBoolean(false) + + // when + connection.onThinClientClosed( + connectFailed, + connectionLive, + thinClient, + devSpacesContext.devWorkspace, + mockk<() -> Unit>(relaxed = true), + {}, + null, + null + ) + + // then + assertThat(connectFailed.get()).isTrue() + verify(exactly = 0) { devSpacesContext.removeWorkspace(any()) } + } +}