Skip to content

Commit 87d2c3a

Browse files
adietishcursoragent
andcommitted
fix: DevWorkspacePods.exec to fail on non-zero exit, timeout, and cancel cleanly (CRW-12138)
- Treat Int.MAX_VALUE and non-zero exit as IOException (with truncated stderr) - close IOTrio streams on cancel - use a single joiner gated by streamsReady so pre-open errors and mid-stream failures complete once without racing stream readers. - extracted PodExecSession Signed-off-by: Andre Dietisheim <adietish@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 14bce0f commit 87d2c3a

4 files changed

Lines changed: 684 additions & 141 deletions

File tree

src/main/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePods.kt

Lines changed: 20 additions & 141 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,7 @@ package com.redhat.devtools.gateway.openshift
1313

1414
import com.intellij.openapi.diagnostic.logger
1515
import com.redhat.devtools.gateway.util.isCancellationException
16-
import com.redhat.devtools.gateway.openshift.apiclient.ApiClientUtils
1716
import io.kubernetes.client.PortForward
18-
import io.kubernetes.client.custom.IOTrio
1917
import io.kubernetes.client.openapi.ApiClient
2018
import io.kubernetes.client.openapi.ApiException
2119
import io.kubernetes.client.openapi.apis.CoreV1Api
@@ -27,8 +25,6 @@ import java.io.IOException
2725
import java.io.InputStream
2826
import java.io.OutputStream
2927
import java.net.*
30-
import kotlin.coroutines.resume
31-
import kotlin.coroutines.resumeWithException
3228

3329
class DevWorkspacePods(private val client: ApiClient) {
3430

@@ -60,141 +56,19 @@ class DevWorkspacePods(private val client: ApiClient) {
6056
container: String,
6157
timeout: Long = 60,
6258
checkCancelled: (() -> Unit)? = null
63-
): String = suspendCancellableCoroutine { cont ->
64-
val metadata = pod.metadata
65-
?: throw IllegalArgumentException("Pod metadata is missing")
66-
val namespace = metadata.namespace
67-
?: throw IllegalArgumentException("Pod namespace is missing")
68-
val podName = metadata.name
69-
?: throw IllegalArgumentException("Pod name is missing")
70-
71-
val closed = CompletableDeferred<Unit>()
72-
val stdout = StringBuilder()
73-
val stderr = StringBuilder()
74-
75-
val scope = CoroutineScope(Dispatchers.IO + SupervisorJob())
76-
77-
val execClientApi = createIsolatedExecClient(client)
78-
var stdoutJob: Job? = null
79-
var stderrJob: Job? = null
80-
lateinit var stdoutStream: InputStream
81-
lateinit var stderrStream: InputStream
82-
83-
try {
84-
val execHandle = ContainerAwareExec(execClientApi).containerAwareExec(
85-
namespace = namespace,
86-
pod = podName,
87-
container = container,
88-
command = command,
89-
onOpen = { io ->
90-
launchCheckCancelled(checkCancelled, scope, io)
91-
92-
stdoutJob = scope.launch { readStream(io.stdout, stdout, checkCancelled) }
93-
stderrJob = scope.launch { readStream(io.stderr, stderr, checkCancelled) }
94-
launchJoinStdOutStdErr(scope, stdoutJob, stderrJob, checkCancelled, closed, cont, stdout, execClientApi)
95-
},
96-
onClosed = { _, _ ->
97-
closed.complete(Unit)
98-
},
99-
onError = { err, _ ->
100-
closed.complete(Unit)
101-
shutdownExecClient(execClientApi)
102-
cont.resumeWithException(err)
103-
},
104-
timeoutMs = timeout * 1000,
105-
tty = false
106-
)
107-
108-
cont.invokeOnCancellation { cause ->
109-
try { stdoutStream.close() } catch (_: Throwable) {}
110-
try { stderrStream.close() } catch (_: Throwable) {}
111-
try {
112-
execHandle.job.cancel(CancellationException("Pods.exec cancellation"))
113-
execHandle.future.cancel(true)
114-
} catch (_: Throwable) {}
115-
scope.cancel()
116-
shutdownExecClient(execClientApi)
117-
}
118-
} catch (e: Exception) {
119-
shutdownExecClient(execClientApi)
120-
if (cont.isActive) cont.resumeWithException(e)
121-
}
59+
): String {
60+
val metadata = pod.metadata ?: throw IOException("Pod metadata is missing")
61+
return PodExecSession(
62+
client = client,
63+
namespace = metadata.namespace,
64+
podName = metadata.name,
65+
container = container,
66+
command = command,
67+
timeout = timeout,
68+
checkCancelled = checkCancelled
69+
).execute()
12270
}
12371

124-
private fun launchJoinStdOutStdErr(
125-
scope: CoroutineScope,
126-
stdoutJob: Job,
127-
stderrJob: Job,
128-
checkCancelled: (() -> Unit)?,
129-
closed: CompletableDeferred<Unit>,
130-
cont: CancellableContinuation<String>,
131-
stdout: StringBuilder,
132-
execClientApi: ApiClient
133-
) {
134-
scope.launch {
135-
try {
136-
listOfNotNull(stdoutJob, stderrJob).joinAll()
137-
checkCancelled?.invoke()
138-
closed.await()
139-
140-
checkCancelled?.invoke()
141-
if (cont.isActive) cont.resume(stdout.toString())
142-
} catch (e: Throwable) {
143-
if (e.isCancellationException()) cont.cancel(e)
144-
else if (cont.isActive) cont.resumeWithException(e)
145-
} finally {
146-
scope.cancel()
147-
shutdownExecClient(execClientApi)
148-
}
149-
}
150-
}
151-
152-
private fun launchCheckCancelled(
153-
checkCancelled: (() -> Unit)?,
154-
scope: CoroutineScope,
155-
io: IOTrio
156-
) {
157-
if (checkCancelled == null) {
158-
return
159-
}
160-
scope.launch {
161-
try {
162-
while (isActive) {
163-
checkCancelled.invoke()
164-
delay(200)
165-
}
166-
} catch (_: Throwable) {
167-
runCatching { io.stdout.close() }
168-
runCatching { io.stderr.close() }
169-
}
170-
}
171-
}
172-
173-
private fun shutdownExecClient(client: ApiClient) {
174-
runCatching { client.httpClient.dispatcher.executorService.shutdownNow() }
175-
runCatching { client.httpClient.connectionPool.evictAll() }
176-
}
177-
178-
private fun readStream(
179-
input: InputStream,
180-
output: StringBuilder,
181-
checkCancelled: (() -> Unit)?
182-
) {
183-
try {
184-
while (true) {
185-
checkCancelled?.invoke()
186-
val b = input.read()
187-
if (b == -1) break
188-
output.append(b.toChar())
189-
}
190-
} catch (_: IOException) {
191-
// Stream was closed (possibly due to cancellation)
192-
}
193-
}
194-
195-
private fun createIsolatedExecClient(base: ApiClient): ApiClient =
196-
ApiClientUtils.cloneForExec(base)
197-
19872
@Throws(IOException::class)
19973
fun forward(pod: V1Pod, localPort: Int, remotePort: Int): Closeable {
20074
val serverSocket = ServerSocket(localPort, 50, InetAddress.getLoopbackAddress())
@@ -204,7 +78,7 @@ class DevWorkspacePods(private val client: ApiClient) {
20478
)
20579
scope.acceptConnections(serverSocket, pod, localPort, remotePort)
20680
return Closeable {
207-
runCatching { serverSocket.close() }
81+
closeQuietly(serverSocket)
20882
scope.cancel()
20983
}
21084
}
@@ -282,7 +156,7 @@ class DevWorkspacePods(private val client: ApiClient) {
282156
"Could not port forward to pod ${pod.metadata?.name} using port $localPort -> $remotePort",
283157
e)
284158
} finally {
285-
runCatching { clientSocket.close() }
159+
closeQuietly(clientSocket)
286160
}
287161
}
288162

@@ -314,8 +188,13 @@ class DevWorkspacePods(private val client: ApiClient) {
314188
}
315189

316190
private fun closeStreams(port: Int, forwardResult: PortForward.PortForwardResult?) {
317-
runCatching { forwardResult?.getInputStream(port)?.close() }
318-
runCatching { forwardResult?.getOutboundStream(port)?.close() }
191+
// getInputStream/getOutboundStream can throw; isolate so one failure does not skip the other close
192+
runCatching { forwardResult?.getInputStream(port) }
193+
.onSuccess { closeQuietly(it) }
194+
.onFailure { logger.debug("Could not get input stream for port $port while closing port-forward", it) }
195+
runCatching { forwardResult?.getOutboundStream(port) }
196+
.onSuccess { closeQuietly(it) }
197+
.onFailure { logger.debug("Could not get outbound stream for port $port while closing port-forward", it) }
319198
}
320199

321200
private fun InputStream.copyToAndFlush(destination: OutputStream) {

0 commit comments

Comments
 (0)