[JEP-222] Add experimental WebSocket support + Jetty update in Winstone + Remoting 4.0

[Chi-Song-Owen]

See JEP-222. Downstream of jenkinsci/winstone#79 + jenkinsci/remoting#357 + jenkinsci/jenkins-test-harness#183.

Proposed Changelog Entries

• Added experimental support for connecting inbound agents using a WebSocket transport over HTTP(S) rather than the TCP port, for easier network setup. See JEP-222 for details.
• Remoting 4.0 release, mostly to highlight the binary incompatibility in the last 3.x release: https://github.com/jenkinsci/remoting/releases/tag/remoting-4.0
• Jetty update in Winstone: https://github.com/jenkinsci/winstone/releases/tag/winstone-5.5

[gemini-code-assist]

Code Review

This pull request introduces WebSocket support for inbound agents and the CLI (JEP-222), separates cloud configuration into a standalone page, modernizes class loading and exception handling, and removes legacy JNLP protocols. The review feedback highlights several critical and high-severity issues, including a bug in Slave.java that causes 404 errors when downloading the CLI jar, potential NullPointerExceptions in finally blocks within WorkUnitContext that could swallow primary exceptions, a potential ConcurrentModificationException in UpdateCenter's background thread, and a missing frame size limit in PlainCLIProtocol that could lead to OutOfMemory errors. Additionally, the reviewer recommends marking shared fields as volatile in WebSocketSession, replacing printStackTrace() with proper logging, and adding defensive null checks across several components.

[gemini-code-assist]

This refactoring introduces a critical bug for jenkins-cli.jar. If cliJar.isFile() is true, name is changed to lib/cli-<version>.jar. However, the webapp only serves the CLI jar under /jnlpJars/jenkins-cli.jar. Changing the name to lib/cli-<version>.jar will cause servletContext.getResource to return null, resulting in a 404 error when users try to download the CLI jar. Keep the name as jenkins-cli.jar for CLI.class.

Suggested change

	File jar = Which.jarFile(owner);
	if (jar.isFile()) {
	name = "lib/" + jar.getName();
	File jar = Which.jarFile(owner);
	if (jar.isFile()) {
	if (owner == CLI.class) {
	name = "jenkins-cli.jar";
	} else {
	name = "lib/" + jar.getName();
	}
	} else {

[gemini-code-assist]

To prevent integer overflow and potential OutOfMemoryError (DoS) when reading very large frame lengths, add a check to limit the maximum allowed frame size (e.g., 100MB).

                    if (framelen < 0) {
                        throw new IOException("corrupt stream: negative frame length");
                    }
                    if (framelen > 100 * 1024 * 1024) {
                        throw new IOException("Frame length " + framelen + " exceeds maximum allowed size");
                    }

[gemini-code-assist]

If an exception is thrown during channel building or setup (e.g., IOException from build), it will be swallowed by the thread pool because the Future is not checked. Wrap the setup logic in a try-catch block to log the failure and close the session properly.

Suggested change

	Computer.threadPoolForRemoting.submit(() -> {
	LOGGER.fine(() -> "setting up channel for " + agent);
	state.fireBeforeChannel(new ChannelBuilder(agent, Computer.threadPoolForRemoting));
	state.fireAfterChannel(state.getChannelBuilder().build(new Transport()));
	LOGGER.fine(() -> "set up channel for " + agent);
	return null;
	});
	Computer.threadPoolForRemoting.submit(() -> {
	try {
	LOGGER.fine(() -> "setting up channel for " + agent);
	state.fireBeforeChannel(new ChannelBuilder(agent, Computer.threadPoolForRemoting));
	state.fireAfterChannel(state.getChannelBuilder().build(new Transport()));
	LOGGER.fine(() -> "set up channel for " + agent);
	} catch (Exception x) {
	LOGGER.log(Level.WARNING, "failed to set up channel for " + agent, x);
	close();
	}
	return null;
	});

[gemini-code-assist]

The fields session, remoteEndpoint, and pings are accessed and modified by different threads (e.g., during connection, close, and scheduled ping tasks). They should be marked as volatile to ensure thread visibility and prevent potential resource leaks where a ping task continues running after the session is closed.

Suggested change

	private Object session;
	private Object remoteEndpoint;
	private ScheduledFuture<?> pings;
	private volatile Object session;
	private volatile Object remoteEndpoint;
	private volatile ScheduledFuture<?> pings;

[gemini-code-assist]

Avoid using x.printStackTrace() as it prints directly to standard error and bypasses the logging framework. Use LOGGER.log(Level.WARNING, ...) or simply wrap and throw the exception.

                } catch (Exception x) {
                    throw new IOException(x);
                }

[gemini-code-assist]

If Executor.currentExecutor() or e.getCurrentWorkUnit() returns null, a NullPointerException will be thrown in the finally block. This will suppress/swallow the primary InterruptedException thrown by startLatch.synchronize(), making debugging extremely difficult. Add defensive null checks.

            Executor e = Executor.currentExecutor();
            if (e != null) {
                WorkUnit wu = e.getCurrentWorkUnit();
                if (wu != null && wu.isMainWork()) {
                    future.start.set(e.getCurrentExecutable());
                }
            }

[gemini-code-assist]

Defensively check if wu is null before calling wu.isMainWork() to prevent a NullPointerException in the finally block, which would swallow any primary exception or interruption.

Suggested change

	WorkUnit wu = e.getCurrentWorkUnit();
	if (wu.isMainWork()) {
	if (problems == null) {
	future.set(executable);
	e.getOwner().taskCompleted(e, task, duration);
	} else {
	future.set(problems);
	e.getOwner().taskCompletedWithProblems(e, task, duration, problems);
	}
	}
	WorkUnit wu = e.getCurrentWorkUnit();
	if (wu != null && wu.isMainWork()) {
	if (problems == null) {
	future.set(executable);
	e.getOwner().taskCompleted(e, task, duration);
	} else {
	future.set(problems);
	e.getOwner().taskCompletedWithProblems(e, task, duration, problems);
	}
	}

[gemini-code-assist]

Iterating over sites (which is a PersistedList) in a background thread can throw a ConcurrentModificationException if another thread modifies the list of update sites concurrently. Create a copy of the list before submitting the task to the executor.

                List<UpdateSite> targets = new ArrayList<>(sites);
                Timer.get().submit(() -> {
                    targets.forEach(UpdateSite::getData);
                    siteDataLoading = false;
                });

[gemini-code-assist]

Defensively check if depPluginTR.jenkinsPluginMetadata and enableInput are defined before accessing checked to prevent potential JavaScript errors on the plugin manager page.

                    if (depPluginTR && depPluginTR.jenkinsPluginMetadata && depPluginTR.jenkinsPluginMetadata.enableInput) {
                        enabled = depPluginTR.jenkinsPluginMetadata.enableInput.checked;
                    }

[gemini-code-assist]

Defensively check for null elements in the exceptions list before calling addSuppressed to prevent a NullPointerException.

Suggested change

	exceptions.forEach(this::addSuppressed);
	exceptions.forEach(ex -> {
	if (ex != null) {
	addSuppressed(ex);
	}
	});