Repository navigation
Backport: Honour consoleproxy.session.timeout for noVNC console sessions - #13058
dheeraj12347 wants to merge 15 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to backport the fix to ensure consoleproxy.session.timeout is honored for noVNC console sessions on the 4.20 branch, so idle sessions are cleaned up and don’t linger indefinitely.
Changes:
- Updates the noVNC WebSocket handler with additional logging, parameter validation, and safer frame/error handling.
- Refactors the console proxy GC thread loop and related logging around idle session cleanup.
- Adjusts console proxy startup/authentication reflection and noVNC viewer creation/replacement logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java | WebSocket connect/frame/error handling changes intended to support correct idle-session cleanup. |
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java | GC loop refactor and idle session timeout constant/comment updates. |
| services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java | Console proxy startup/auth reflection changes and noVNC viewer lifecycle adjustments. |
💡 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 @@
## 4.20 #13058 +/- ##
============================================
+ Coverage 16.26% 16.43% +0.16%
- Complexity 13434 13671 +237
============================================
Files 5667 5669 +2
Lines 500731 501655 +924
Branches 60803 60953 +150
============================================
+ Hits 81455 82423 +968
+ Misses 410172 409998 -174
- Partials 9104 9234 +130
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the detailed review and suggestions from Copilot. |
|
@dheeraj12347 , I set consoleproxy.session.timeout to 30000, expecting my consoles to get lost in half a minute. I restarted the MS. The consoles keep working until about the half an hour, the default setting. (I didn’t measure exactly) Can you have another look at this? |
|
@dheeraj12347 once this is merged into 4.20, it will be included in main as well after forward-merge |
Hi @DaanHoogland , I’ve updated the PR to address the consoleproxy.session.timeout behaviour you described: In ConsoleProxy.java I now read consoleproxy.session.timeout from the configuration (milliseconds), validate it, and store the effective value in ConsoleProxy.sessionTimeoutMillis, with a log line on startup showing the effective timeout. In ConsoleProxyGCThread.java I removed the hardcoded idle timeout and changed the GC logic to compute each viewer’s idle time in milliseconds and compare it against ConsoleProxy.sessionTimeoutMillis, logging the idle duration and configured timeout when a session is removed. To verify my changes, I built the console proxy server module with: bash I’m open to any feedback on this approach, and I’m happy to further adjust or iterate based on your results when you re-test with consoleproxy.session.timeout=30000. |
|
@dheeraj12347 : a ai generated comment to this change:
I think we (as in you ;) ) should add this to this PR. |
|
Thanks @DaanHoogland, I added the missing propagation of consoleproxy.session.timeout to the CPVM boot arguments in ConsoleProxyManagerImpl, as suggested. |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19399 |
|
@blueorangutan test keepEnv |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-17073)
|
|
@kiranchavala can you have a second look? |
|
Hi @DaanHoogland and @Pearl1594, I noticed that the Codecov, Simulator CI and UI Build checks are currently failing on this PR. The Codecov log shows repeated request failures while creating the commit record at |
|
@dheeraj12347 leave the UI. codecov, I'm not sure of, but IU doubt it has anything to do with your code. |
| return; | ||
| } | ||
| logger.trace("Sending client [ID: {}] frame of {} bytes.", viewer.getClientId(), f.getPayloadLength()); | ||
| viewer.updateFrontEndActivityTime(); |
There was a problem hiding this comment.
does an open console that nobody touches ever time out? the browser keeps asking for screen updates on its own, so every frame here looks like activity
@dheeraj12347 still no. every frame from the browser still counts as activity, and so does every screen update sent back to it. can we count only key presses and mouse moves? |
|
@DaanHoogland , @Pearl1594 and @Damans227 I also checked the bundled noVNC implementation: continuous screen updates are enabled through a client message, while the resulting screen updates travel from the server to the browser. I'll verify the idle-session behavior and whether the activity refresh in |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19625 |
|
@blueorangutan test keepEnv |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
Hi @DaanHoogland, @Damans227, and @kiranchavala, I've pushed an additional test in The targeted test passes locally (2 tests, 0 failures), and the console-proxy server module compiles successfully. The activity-tracking changes are also pushed. I'll continue verifying that only actual keyboard and mouse interactions refresh the idle timer, while automatic screen updates do not. Could you please review the latest changes when convenient? Thanks! |
|
[SF] Trillian test result (tid-17181)
|
|
Thanks for the fix @dheeraj12347, can you please reference the PR from which the fix is being backported from? |
|
@dheeraj12347 0f26cc3 covers my thread, only key presses and mouse moves keep the session alive now. when the timer runs out, does the console just freeze until the second timeout closes it? looks like an idle tab can sit frozen for up to 5 more minutes before it disconnects |
|
@Damans227, @kiranchavala did you test this? |
|
@DaanHoogland no, i havent run it on a lab, only read the code. still waiting on @dheeraj12347 for the frozen screen question above |
Description
This PR backports the console proxy/noVNC timeout fix to the 4.20 branch.
It ensures that
consoleproxy.session.timeoutis honoured for noVNC consolesessions, so idle sessions are cleaned up correctly and do not linger
indefinitely.
Key points:
consoleproxy.session.timeoutthrough the console proxy server fornoVNC-based console sessions.
are closed after the configured period.
newer
sessionRequiresNewViewerAPI onConsoleProxyClientParam, onlyremove the references that exist in later branches.
Related work
idle noVNC sessions not respecting
consoleproxy.session.timeout.Testing
Compiled the console proxy server module successfully:
Verified that the code builds cleanly with checkstyle on the 4.20 branch.
Fix Global setting "consoleproxy.session.timeout " is not honoured #12810