fix DevWorkspacePods.exec to fail on non-zero exit, timeout, and cancel cleanly (CRW-12138) - #356
Open
adietish wants to merge 1 commit into
Open
fix DevWorkspacePods.exec to fail on non-zero exit, timeout, and cancel cleanly (CRW-12138)#356adietish wants to merge 1 commit into
adietish wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesPod command execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DevWorkspacePods
participant PodExecSession
participant KubernetesAPI
participant OutputStreams
DevWorkspacePods->>PodExecSession: execute command with timeout and cancellation callback
PodExecSession->>KubernetesAPI: start container execution
KubernetesAPI-->>OutputStreams: provide stdout and stderr
PodExecSession->>OutputStreams: read and collect output
PodExecSession-->>DevWorkspacePods: return stdout or raise execution failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePodsTest.kt (2)
1093-1098: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for empty stdout with exit code 0.
The PR states that empty output with exit code 0 must be treated as a success. The added tests cover non-empty stdout, non-zero exit, timeout, and stderr, but no test asserts that empty stdout with exit code 0 returns an empty string instead of failing. Add that case to lock the stated behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePodsTest.kt` around lines 1093 - 1098, Add a test near the existing pods.exec assertions in DevWorkspacePodsTest that executes a command producing empty stdout with exit code 0, and assert that exec returns an empty string without failing. Keep the test focused on the successful empty-output behavior.
905-1099: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
ContainerAwareExecstubbing into a helper.The four new tests repeat the same setup:
mockkConstructor(ContainerAwareExec::class), afakeHandle, ananswersblock that castsargs[4]/args[5]and drivesonOpen/onClosed, theApiClientUtils.cloneForExecstub, and thetestPodliteral. The same block also appears in the earlier tests at Lines 556-740. Extract one helper that takes stdout, stderr, and the exit code. The tests then state only the outcome under test, and future signature changes tocontainerAwareExecneed one update instead of eight.♻️ Sketch of the helper
private fun stubExec( stdout: String = "", stderr: String = "", exitCode: Int? = null, error: Throwable? = null, afterOpen: (() -> Unit)? = null ): ApiClient { mockkConstructor(ContainerAwareExec::class) val handle = ContainerAwareExec.ExecHandle( future = CompletableFuture.completedFuture(0), job = mockk(relaxed = true) ) every { anyConstructed<ContainerAwareExec>().containerAwareExec( any(), any(), any(), any(), any(), any(), any(), any(), any() ) } answers { `@Suppress`("UNCHECKED_CAST") val onOpen = it.invocation.args[4] as Consumer<IOTrio> `@Suppress`("UNCHECKED_CAST") val onClosed = it.invocation.args[5] as BiConsumer<Int, IOTrio> `@Suppress`("UNCHECKED_CAST") val onError = it.invocation.args[6] as BiConsumer<Throwable, IOTrio> val io = IOTrio().apply { this.stdout = ByteArrayInputStream(stdout.toByteArray()) this.stderr = ByteArrayInputStream(stderr.toByteArray()) this.stdin = mockk(relaxed = true) } onOpen.accept(io) afterOpen?.invoke() error?.let { e -> onError.accept(e, io) } exitCode?.let { code -> onClosed.accept(code, io) } handle } mockkObject(ApiClientUtils) val execClient = mockk<ApiClient>(relaxed = true) every { ApiClientUtils.cloneForExec(any()) } returns execClient return execClient } private fun testPod(name: String = "test-pod", ns: String = "test-ns") = V1Pod().apply { metadata = V1ObjectMeta().apply { this.name = name; namespace = ns } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePodsTest.kt` around lines 905 - 1099, Extract the duplicated ContainerAwareExec and ApiClientUtils setup from the four new tests and the earlier exec tests into shared test helpers, such as stubExec and testPod. Make stubExec accept stdout, stderr, and exitCode, centralize the callback wiring and handle creation, and update each test to configure only its scenario and assertion; preserve any existing error or after-open behavior required by earlier tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePods.kt`:
- Around line 190-192: Update closeStreams to evaluate both forwardResult stream
getters inside the existing failure-tolerant handling, such as runCatching, so
IllegalArgumentException or connection failures are suppressed during cleanup.
Preserve quiet closing for successfully retrieved streams and ensure cleanup
cannot replace the original port-forward failure or interrupt retries.
In `@src/main/kotlin/com/redhat/devtools/gateway/openshift/PodExecSession.kt`:
- Around line 155-170: Update PodExecSession.readStream to read the input in
buffered blocks and decode the bytes explicitly as UTF-8 before appending to
output. Preserve cancellation checks and IOException handling, while avoiding
per-byte reads and ensuring multi-byte UTF-8 output is decoded correctly.
---
Nitpick comments:
In
`@src/test/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePodsTest.kt`:
- Around line 1093-1098: Add a test near the existing pods.exec assertions in
DevWorkspacePodsTest that executes a command producing empty stdout with exit
code 0, and assert that exec returns an empty string without failing. Keep the
test focused on the successful empty-output behavior.
- Around line 905-1099: Extract the duplicated ContainerAwareExec and
ApiClientUtils setup from the four new tests and the earlier exec tests into
shared test helpers, such as stubExec and testPod. Make stubExec accept stdout,
stderr, and exitCode, centralize the callback wiring and handle creation, and
update each test to configure only its scenario and assertion; preserve any
existing error or after-open behavior required by earlier tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 93a9e83b-5a57-419f-b421-99f4470546d0
📒 Files selected for processing (4)
src/main/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePods.ktsrc/main/kotlin/com/redhat/devtools/gateway/openshift/PodExecSession.ktsrc/main/kotlin/com/redhat/devtools/gateway/openshift/Utils.ktsrc/test/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePodsTest.kt
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #356 +/- ##
==========================================
+ Coverage 0.00% 34.63% +34.63%
==========================================
Files 4 114 +110
Lines 26 4862 +4836
Branches 0 935 +935
==========================================
+ Hits 0 1684 +1684
- Misses 26 2939 +2913
- Partials 0 239 +239 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
adietish
force-pushed
the
improve_pod_exec
branch
2 times, most recently
from
July 31, 2026 11:22
3511d41 to
7720add
Compare
adietish
changed the title
…el 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>
adietish
force-pushed
the
improve_pod_exec
branch
from
July 31, 2026 12:06
7720add to
456ddfc
Compare
vrubezhny
reviewed
Jul 31, 2026
vrubezhny
left a comment
vrubezhny
left a comment
Collaborator
There was a problem hiding this comment.
Small questions inline...
The biggest one is that I couldn't connect in modal context - when running it as Run Plugin (IDEA), then File->Remote Development...
More is that the Cluster Connection wizard page wasn't showing any clusters from my Kube config... I believe the connection failure is of the same kind.
When I ran it as Run Plugin (which is non-modal) - it connected successfully
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Decision order for exec’s result:
Exit code wins for failure/timeout. Partial output is only a problem on the success path, and that path now fails if a stream died mid-read instead of handing back truncated text.
Empty stdout with exit 0 and clean EOF is still a normal success (""); callers like RemoteIDEServer map that to empty status.