Honour consoleproxy.session.timeout for noVNC sessions (fixes #12810)#13002
Open
dheeraj12347 wants to merge 3 commits intoapache:mainfrom
Open
Honour consoleproxy.session.timeout for noVNC sessions (fixes #12810)#13002dheeraj12347 wants to merge 3 commits intoapache:mainfrom
dheeraj12347 wants to merge 3 commits intoapache:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the console-proxy server to honor the consoleproxy.session.timeout configuration for noVNC WebSocket sessions and for server-side idle-session cleanup, addressing issue #12810 where noVNC sessions did not time out as expected.
Changes:
- Introduces a shared
ConsoleProxy.sessionTimeoutMillis(default 5 minutes) loaded fromconsoleproxy.session.timeout. - Uses the configured timeout for noVNC WebSocket idle timeouts and for GC-based idle session eviction.
- Improves noVNC WebSocket error handling/logging to avoid null viewer access.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java | Adds and loads sessionTimeoutMillis from configuration; wires GC thread startup. |
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java | Replaces hardcoded idle timeout with a derived value from sessionTimeoutMillis. |
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java | Sets Jetty WebSocket idle timeout from sessionTimeoutMillis; hardens frame/error handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13002 +/- ##
=========================================
Coverage 18.01% 18.01%
- Complexity 16607 16609 +2
=========================================
Files 6029 6029
Lines 542160 542170 +10
Branches 66451 66462 +11
=========================================
+ Hits 97682 97687 +5
- Misses 433461 433466 +5
Partials 11017 11017
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
|
@dheeraj12347 , can you asses the co-pilot’s comments (and close or apply as applicable)? |
dheeraj12347
force-pushed
the
fix-12810-consoleproxy-timeout
branch
from
4a7a6ee to
6eef242
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make noVNC console sessions respect the global consoleproxy.session.timeout setting (issue #12810) by wiring the configured timeout into the console-proxy process and applying it to both WebSocket sessions and idle-session cleanup.
Changes:
- Refactors logging/validation in the noVNC WebSocket handler and console-proxy bootstrap code.
- Refactors GC thread logging/loop variables and keeps an explicit idle-session constant.
- (Per PR description) intended: read/apply
consoleproxy.session.timeoutand use it for noVNC/GC timeouts — but this wiring is not present in the current diffs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java | Improves properties handling/logging and resource closing, but does not currently read/apply consoleproxy.session.timeout. |
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java | Refactors logging/loop code; still uses a hard-coded 180s idle timeout. |
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java | Improves error handling/logging and viewer-null handling; does not currently set WebSocket session idle timeout. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+160
to
169
| s = conf != null ? conf.getProperty("consoleproxy.readTimeoutSeconds") : null; | ||
| if (s != null) { | ||
| readTimeoutSeconds = Integer.parseInt(s); | ||
| LOGGER.info("Setting readTimeoutSeconds=" + readTimeoutSeconds); | ||
| } | ||
|
|
||
| s = conf.getProperty("consoleproxy.defaultBufferSize"); | ||
| s = conf != null ? conf.getProperty("consoleproxy.defaultBufferSize") : null; | ||
| if (s != null) { | ||
| defaultBufferSize = Integer.parseInt(s); | ||
| LOGGER.info("Setting defaultBufferSize=" + defaultBufferSize); |
Comment on lines
+37
to
+41
| /** | ||
| * Maximum time (in seconds) a console session is allowed to be idle before it is closed. | ||
| * This value should be kept in sync with ConsoleProxy.VIEWER_LINGER_SECONDS. | ||
| */ | ||
| private static final int MAX_SESSION_IDLE_SECONDS = 180; |
Comment on lines
155
to
156
| viewer = ConsoleProxy.getNoVncViewer(param, ajaxSessionIdStr, session); | ||
| logger.info("Viewer has been created successfully [session UUID: {}, client IP: {}].", sessionUuid, clientIp); |
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
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.
Fixes #12810.
Changes:
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java