fix: cancel button hangs during workspace IDE connection (crw-11729)#338
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughWorkspace connection startup now propagates modality state to dialogs, checks pod readiness before remote IDE status commands, honors cancellation during polling and exec sessions, and performs stream and client cleanup during cancellation. ChangesWorkspace readiness and cancellation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DevSpacesConnectionProvider
participant DevSpacesConnection
participant RemoteIDEServer
participant DevWorkspacePods
participant Dialogs
DevSpacesConnectionProvider->>DevSpacesConnection: connect(modalityState, checkCancelled)
DevSpacesConnection->>RemoteIDEServer: getStatus(checkCancelled)
RemoteIDEServer->>DevWorkspacePods: isPodRunning(pod)
DevWorkspacePods-->>RemoteIDEServer: pod running state
RemoteIDEServer-->>DevSpacesConnection: remote IDE status or empty status
DevSpacesConnection->>Dialogs: ideNotResponding(modalityState)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePods.kt (1)
124-148: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the isolated exec client on early failures
onErrorcan fire beforeonOpeninstalls thefinallycleanup, so the isolated client stays open on exec setup failures. AddshutdownExecClient(execClientApi)there, and keep the outercatchas a fallback for failures that preventcontainerAwareExec(...)from returning anExecHandle.🤖 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/main/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePods.kt` around lines 124 - 148, Update the onError callback in the containerAwareExec setup to call shutdownExecClient(execClientApi) after completing the continuation and before resuming with the exception. Preserve the outer catch in the surrounding exec flow as a fallback for failures that occur before an ExecHandle is returned.
🤖 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.
Outside diff comments:
In `@src/main/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePods.kt`:
- Around line 124-148: Update the onError callback in the containerAwareExec
setup to call shutdownExecClient(execClientApi) after completing the
continuation and before resuming with the exception. Preserve the outer catch in
the surrounding exec flow as a fallback for failures that occur before an
ExecHandle is returned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: b465dbcb-ed91-42e6-b872-dddad6a8cd9b
📒 Files selected for processing (5)
src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnection.ktsrc/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnectionProvider.ktsrc/main/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePods.ktsrc/main/kotlin/com/redhat/devtools/gateway/server/RemoteIDEServer.ktsrc/main/kotlin/com/redhat/devtools/gateway/view/ui/Dialogs.kt
8434b20 to
ba99a1a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #338 +/- ##
==========================================
+ Coverage 0.00% 24.55% +24.55%
==========================================
Files 4 104 +100
Lines 26 4414 +4388
Branches 0 846 +846
==========================================
+ Hits 0 1084 +1084
- Misses 26 3156 +3130
- Partials 0 174 +174 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 107-122: Update the completion coroutine and the exec error
callback to guard both continuation resumes against duplicate completion. In the
shown scope.launch flow, check cont.isActive before resuming stdout, and apply
the same guard to the onError resume path (or use tryResume consistently), while
preserving cancellation and exception propagation behavior.
- Around line 41-50: Update isPodRunning to ensure the readNamespacedPod(name,
namespace).execute() readiness lookup is bounded by an explicit timeout or
observes coroutine cancellation. Preserve the existing false result for missing
metadata and lookup failures, while preventing a hung API call from blocking
startup indefinitely.
🪄 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: Enterprise
Run ID: 5e263fb3-f842-4746-8c43-6e0b04f01b12
📒 Files selected for processing (6)
src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnection.ktsrc/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnectionProvider.ktsrc/main/kotlin/com/redhat/devtools/gateway/openshift/DevWorkspacePods.ktsrc/main/kotlin/com/redhat/devtools/gateway/server/RemoteIDEServer.ktsrc/main/kotlin/com/redhat/devtools/gateway/view/ui/Dialogs.ktsrc/test/kotlin/com/redhat/devtools/gateway/devworkspace/DevWorkspaceRestartTest.kt
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnectionProvider.kt
- src/main/kotlin/com/redhat/devtools/gateway/view/ui/Dialogs.kt
- src/main/kotlin/com/redhat/devtools/gateway/DevSpacesConnection.kt
- src/main/kotlin/com/redhat/devtools/gateway/server/RemoteIDEServer.kt
Pass checkCancelled through getStatus/exec, add cancellation monitor that closes streams to unblock reads, use indicator modality state for dialogs, guard exec with pod-running check, and shut down isolated OkHttp clients on cleanup. Issue: https://redhat.atlassian.net/browse/CRW-11796 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com> Assisted-By: Claude Opus 4.6 <noreply@anthropic.com>
ba99a1a to
ce0c03e
Compare
| scope.launch { | ||
| try { | ||
| listOfNotNull(stdoutJob, stderrJob).joinAll() | ||
| checkCancelled?.invoke() |
There was a problem hiding this comment.
is this check and others still needed if we check it in a background job already (see lines 94-106)?
There was a problem hiding this comment.
Why not? That join might take up to several minutes (usually some 10-20 seconds), so user may decide to cancel at any time during this waiting - and we'll follow with the cancellation immediately after, rather then finishing the operation (its result even if achieved is not needed anymore)
Line 111 (checkCancelled before closed.await()): This is the important one. After streams are closed and reader jobs finish, this catches cancellation before blocking on closed.await() — which could wait indefinitely if onClosed/onError never fires (e.g. the WebSocket setup stalled).
Signed-off-by: Andre Dietisheim <adietish@redhat.com>
Signed-off-by: Andre Dietisheim <adietish@redhat.com>
|
I extracted methods in |
adietish
left a comment
adietish
left a comment
There was a problem hiding this comment.
LGTM
Could replicate the issue. Your PR fixes it. Kudos, thanks!
3 participants
Pass checkCancelled through getStatus/exec, add cancellation monitor that closes streams to unblock reads, use indicator modality state for dialogs, guard exec with pod-running check, and shut down isolated OkHttp clients on cleanup.
Issue: https://redhat.atlassian.net/browse/CRW-11796
Assisted-By: Claude Opus 4.6 noreply@anthropic.com