Honour consoleproxy.session.timeout for noVNC sessions (fixes #12810)#13002
Open
dheeraj12347 wants to merge 1 commit intoapache:mainfrom
Open
Honour consoleproxy.session.timeout for noVNC sessions (fixes #12810)#13002dheeraj12347 wants to merge 1 commit intoapache:mainfrom
dheeraj12347 wants to merge 1 commit 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.
| if (ConsoleProxy.sessionTimeoutMillis <= 0) { | ||
| return DEFAULT_MAX_SESSION_IDLE_SECONDS; | ||
| } | ||
| return ConsoleProxy.sessionTimeoutMillis / 1000; |
Comment on lines
+48
to
+54
| private int getMaxSessionIdleSeconds() { | ||
| if (ConsoleProxy.sessionTimeoutMillis <= 0) { | ||
| return DEFAULT_MAX_SESSION_IDLE_SECONDS; | ||
| } | ||
| return ConsoleProxy.sessionTimeoutMillis / 1000; | ||
| } | ||
|
|
Comment on lines
+172
to
+181
| // New: read consoleproxy.session.timeout (milliseconds) | ||
| s = conf.getProperty("consoleproxy.session.timeout"); | ||
| if (s != null) { | ||
| try { | ||
| sessionTimeoutMillis = Integer.parseInt(s); | ||
| LOGGER.info("Setting consoleproxy.session.timeout=" + sessionTimeoutMillis); | ||
| } catch (NumberFormatException e) { | ||
| LOGGER.warn("Invalid value for consoleproxy.session.timeout: " + s + | ||
| ", using default " + sessionTimeoutMillis, e); | ||
| } |
Comment on lines
127
to
+131
| try { | ||
| session.setIdleTimeout(ConsoleProxy.sessionTimeoutMillis); | ||
| logger.debug("Set noVNC WebSocket idle timeout to {} ms for session UUID: {}.", | ||
| ConsoleProxy.sessionTimeoutMillis, sessionUuid); | ||
|
|
| } | ||
|
|
||
| ConsoleProxyGCThread cthread = new ConsoleProxyGCThread(connectionMap, removedSessionsSet); | ||
| ConsoleProxyGCThread cthread = new ConsoleProxyGCThread(connectionMap, removedSessionsSet /*, sessionTimeoutMillis */); |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13002 +/- ##
============================================
- Coverage 18.00% 18.00% -0.01%
- Complexity 16464 16472 +8
============================================
Files 5977 5976 -1
Lines 537726 537881 +155
Branches 66026 66053 +27
============================================
+ Hits 96839 96862 +23
- Misses 429968 430097 +129
- Partials 10919 10922 +3
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:
|
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