Backport: Honour consoleproxy.session.timeout for noVNC console sessions

[dheeraj12347]

Description

This PR backports the console proxy/noVNC timeout fix to the 4.20 branch.

It ensures that consoleproxy.session.timeout is honoured for noVNC console
sessions, so idle sessions are cleaned up correctly and do not linger
indefinitely.

Key points:

• Wire consoleproxy.session.timeout through the console proxy server for
  noVNC-based console sessions.
• Apply the timeout in the WebSocket handler and GC thread so idle sessions
  are closed after the configured period.
• Keep the change minimal and compatible with 4.20: do not introduce the
  newer sessionRequiresNewViewer API on ConsoleProxyClientParam, only
  remove the references that exist in later branches.

Related work

• Backport of the console proxy timeout fix from the main branch.
• Intended to address the same behaviour as the upstream change that fixed
  idle noVNC sessions not respecting consoleproxy.session.timeout.

Testing

• Compiled the console proxy server module successfully:

  mvn -pl services/console-proxy/server -am -DskipTests compile

• Verified that the code builds cleanly with checkstyle on the 4.20 branch.
  Fix Global setting "consoleproxy.session.timeout " is not honoured #12810

[Copilot]

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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 4.14%. Comparing base (549daae) to head (bf60c05).

❗ There is a different number of reports uploaded between BASE (549daae) and HEAD (bf60c05). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (549daae)	HEAD (bf60c05)
unittests	1	0

Additional details and impacted files

@@              Coverage Diff              @@
##               4.20   #13058       +/-   ##
=============================================
- Coverage     16.26%    4.14%   -12.12%     
=============================================
  Files          5667      405     -5262     
  Lines        500731    33025   -467706     
  Branches      60803     5898    -54905     
=============================================
- Hits          81455     1370    -80085     
+ Misses       410172    31479   -378693     
+ Partials       9104      176     -8928     

Flag	Coverage Δ
uitests	4.14% <ø> (ø)
unittests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[dheeraj12347]

Thanks for the detailed review and suggestions from Copilot.
I’ve read and understood all of the comments, especially around the 4.20 authenticateConsoleAccess signature/invocation, idle timeout handling in the GC thread, noVNC viewer activity tracking, and the unused sessionRequiresNewViewer flag.
I’ll work through each point and push updates to address them as soon as possible.

[DaanHoogland]

@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?

[weizhouapache]

@dheeraj12347
only one of #13002 and #13058 is needed.

once this is merged into 4.20, it will be included in main as well after forward-merge

[dheeraj12347]

@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?

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
mvn -pl services/console-proxy/server -am clean compile

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.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@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.

[DaanHoogland]

@nvazquez , can you please review (and maybe test)?

[DaanHoogland]

#13858 opened for possible follow-up for non-noVNC consoles.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (3)

services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java:98

• Indentation around the connMap key snapshot loop is inconsistent (extra leading spaces on the assignment/brace/for lines), which is likely to violate checkstyle and makes the synchronization scope harder to read.

            List<String> keys;
            synchronized (connMap) {
                 keys = new ArrayList<>(connMap.keySet());
             }
             for (String key : keys) {
                ConsoleProxyClient client;

services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyGCThread.java:120

• Potential race: this code removes by key even if another thread has replaced the viewer for the same key between the earlier get() and this remove(). Since clientKey can be reused (e.g., per VM tag), this can drop an active session from the map. Remove only if the current map value is still the same client instance you evaluated as idle.

                synchronized (connMap) {
                    connMap.remove(key);
                    bReportLoad = true;
                }

services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java:219

• New behavior (parsing and applying consoleproxy.session.timeout) isn’t covered by tests in this module. Adding unit tests for: (1) defaulting on missing/invalid/<=0 values, and (2) using the configured value when valid, would help prevent regressions in future backports.

        // New: read consoleproxy.session.timeout (milliseconds)
        s = conf != null ? conf.getProperty("consoleproxy.session.timeout") : null;
        if (s != null) {
            try {
                long value = Long.parseLong(s);
                if (value <= 0) {
                    LOGGER.warn("consoleproxy.session.timeout={} is <= 0, using default {} ms",
                            value, DEFAULT_SESSION_TIMEOUT_MILLIS);
                    sessionTimeoutMillis = DEFAULT_SESSION_TIMEOUT_MILLIS;
                } else {
                    sessionTimeoutMillis = value;
                }
            } catch (NumberFormatException e) {
                LOGGER.warn("Invalid value for consoleproxy.session.timeout: {}, using default {} ms",
                        s, DEFAULT_SESSION_TIMEOUT_MILLIS, e);
                sessionTimeoutMillis = DEFAULT_SESSION_TIMEOUT_MILLIS;
            }
        }
        LOGGER.info("Effective consoleproxy.session.timeout={} ms", sessionTimeoutMillis);

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18836