Copilot commented on code in PR #12967:
URL: https://github.com/apache/cloudstack/pull/12967#discussion_r3166846005
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java:
##########
@@ -125,6 +127,14 @@ public void onConnect(final Session session) throws
IOException, InterruptedExce
}
try {
+ if (ConsoleProxy.sessionTimeoutMillis > 0) {
+ session.setIdleTimeout(ConsoleProxy.sessionTimeoutMillis);
+ logger.debug("Set noVNC WebSocket idle timeout to {} ms for
session UUID {}.",
+ ConsoleProxy.sessionTimeoutMillis, sessionUuid);
+ } else {
+ logger.debug("Using default noVNC WebSocket idle timeout for
session UUID {}.", sessionUuid);
+ }
Review Comment:
The new WebSocket idle-timeout behavior (setting Session idle timeout from
ConsoleProxy.sessionTimeoutMillis) isn’t covered by existing console-proxy unit
tests. Since this can affect disconnect behavior in production, please add
focused tests (e.g., verify the timeout is applied when configured and left
default when <=0) using a mocked Jetty Session.
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxy.java:
##########
@@ -165,13 +171,31 @@ private static void configProxy(Properties conf) {
defaultBufferSize = Integer.parseInt(s);
LOGGER.info("Setting defaultBufferSize=" + defaultBufferSize);
}
+
+ // Read consoleproxy.session.timeout in milliseconds.
+ s = conf.getProperty("consoleproxy.session.timeout");
+ if (s != null) {
+ try {
+ int parsedTimeout = Integer.parseInt(s);
+ if (parsedTimeout < 1000) {
+ LOGGER.warn("Invalid value for
consoleproxy.session.timeout: " + s
+ + " ms, must be >= 1000 ms, keeping default " +
sessionTimeoutMillis + " ms");
+ } else {
+ sessionTimeoutMillis = parsedTimeout;
+ LOGGER.info("Setting consoleproxy.session.timeout=" +
sessionTimeoutMillis + " ms");
+ }
+ } catch (NumberFormatException e) {
+ LOGGER.warn("Invalid value for consoleproxy.session.timeout: "
+ s
+ + ", keeping default " + sessionTimeoutMillis + " ms",
e);
+ }
+ }
Review Comment:
This PR is scoped/described as adding timestamps to *.err logs via Log4j
config, but this change introduces a new console-proxy setting
(sessionTimeoutMillis + parsing consoleproxy.session.timeout) and changes
runtime behavior (WebSocket idle timeout / session cleanup timing). Please
either split these console-proxy behavioral changes into a separate PR or
update the PR description/issue linkage to explicitly cover and justify them
(including any compatibility/operational impact).
##########
services/console-proxy/server/src/main/java/com/cloud/consoleproxy/ConsoleProxyNoVNCHandler.java:
##########
@@ -185,12 +195,21 @@ public void onClose(Session session, int statusCode,
String reason) throws IOExc
@OnWebSocketFrame
public void onFrame(Frame f) throws IOException {
+ if (viewer == null) {
+ logger.warn("Ignoring WebSocket frame because viewer is not
initialized yet.");
+ return;
Review Comment:
Logging a WARN for every WebSocket frame received before viewer
initialization could create noisy logs (and potentially amplify log volume
under abusive/buggy clients). Consider lowering this to DEBUG/TRACE, adding
context (e.g., session UUID / remote IP), or rate-limiting so transient startup
races don’t spam warnings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
