From fa8f20882d583b2db8557527d5a4594f4bb9dcd9 Mon Sep 17 00:00:00 2001 From: Jochen Delabie Date: Thu, 16 Jul 2026 16:15:10 +0200 Subject: [PATCH 1/2] Run the freestyle tunnel on the build's agent, not the controller TestingBotBuildWrapper.setUp booted the tunnel with new App() directly in the controller JVM (the Launcher was unused), so for builds on an agent the tunnel ran on the wrong node and localhost traffic from the tests never reached it. The pipeline testingbotTunnel step already did this correctly by running the tunnel inside a MasterToSlaveCallable on the build node's channel. Extract that mechanism into TunnelManager (startOnChannel/stopOnChannel, a shared RUNNING_TUNNELS registry, and generateTunnelIdentifier) and use it from both the pipeline step (now delegating) and the freestyle wrapper. The wrapper runs the tunnel via launcher.getChannel(), stores the channel + identifier for teardown, exposes TESTINGBOT_TUNNEL_IDENTIFIER (so parallel freestyle builds on one node stay isolated), and falls back to a controller-local tunnel only when no channel is available. The pipeline step's observable behavior is unchanged. --- .../testingbot/TestingBotBuildWrapper.java | 62 ++++++++-- src/main/java/testingbot/TunnelManager.java | 115 ++++++++++++++++++ .../pipeline/TestingBotTunnelStep.java | 86 +------------ 3 files changed, 168 insertions(+), 95 deletions(-) diff --git a/src/main/java/testingbot/TestingBotBuildWrapper.java b/src/main/java/testingbot/TestingBotBuildWrapper.java index e23a3fb..fcb329c 100644 --- a/src/main/java/testingbot/TestingBotBuildWrapper.java +++ b/src/main/java/testingbot/TestingBotBuildWrapper.java @@ -23,8 +23,10 @@ import hudson.model.Job; import hudson.model.Item; import hudson.model.listeners.ItemListener; +import hudson.remoting.VirtualChannel; import hudson.security.ACL; import hudson.util.DescribableList; +import testingbot.pipeline.TestingBotTunnelStep; import java.util.Map; import hudson.util.ListBoxModel; import java.util.ArrayList; @@ -92,24 +94,45 @@ public Environment setUp(AbstractBuild build, Launcher launcher, BuildListener l if (this.useTunnel) { if (credentials == null) { listener.getLogger().println("No TestingBot key/secret found while trying to start a TestingBot Tunnel"); - return new TestingBotBuildEnvironment(null, null, null); + return new TestingBotBuildEnvironment(null, buildId, null, null, null); } + // Run the tunnel on the build's node (not the controller) so that a test connecting to + // localhost reaches the tunnel. The identifier keeps parallel builds on one node isolated. + String tunnelIdentifier = TunnelManager.generateTunnelIdentifier(build.getParent().getFullName(), build.getNumber()); + String key = credentials.getKey(); + String secret = credentials.getDecryptedSecret(); + VirtualChannel channel = launcher.getChannel(); + + if (channel != null) { + try { + TunnelManager.startOnChannel(channel, key, secret, "", tunnelIdentifier, listener); + } catch (InterruptedException ie) { + TunnelManager.stopOnChannel(channel, tunnelIdentifier, listener); + throw ie; + } catch (Exception ex) { + // Do not continue the build with a broken tunnel: clean up and fail. + TunnelManager.stopOnChannel(channel, tunnelIdentifier, listener); + throw new IOException("Failed to start TestingBot tunnel", ex); + } + return new TestingBotBuildEnvironment(credentials, buildId, tunnelIdentifier, channel, null); + } + + // No agent channel available (unusual launcher); fall back to a controller-local tunnel. final App app = new App(); try { - TunnelManager.start(app, credentials.getKey(), credentials.getDecryptedSecret(), "", null, listener); + TunnelManager.start(app, key, secret, "", tunnelIdentifier, listener); } catch (InterruptedException ie) { TunnelManager.stop(app, listener); throw ie; } catch (Exception ex) { - // Do not continue the build with a broken tunnel: clean up and fail. TunnelManager.stop(app, listener); throw new IOException("Failed to start TestingBot tunnel", ex); } - return new TestingBotBuildEnvironment(credentials, app, buildId); + return new TestingBotBuildEnvironment(credentials, buildId, tunnelIdentifier, null, app); } - return new TestingBotBuildEnvironment(credentials, null, buildId); + return new TestingBotBuildEnvironment(credentials, buildId, null, null, null); } /** @@ -151,14 +174,22 @@ public ListBoxModel doFillCredentialsIdItems(@AncestorInPath final Item context) private class TestingBotBuildEnvironment extends BuildWrapper.Environment { - private final App app; private final TestingBotCredentials credentials; private final String buildId; - - public TestingBotBuildEnvironment(TestingBotCredentials credentials, @Nullable App app, @Nullable String buildId) { + /** Non-null when a tunnel was started for this build. */ + private final String tunnelIdentifier; + /** Agent channel the tunnel runs on; null when the tunnel runs controller-locally (see {@link #localApp}). */ + private final VirtualChannel channel; + /** Controller-local tunnel handle, used only in the no-channel fallback. */ + private final App localApp; + + public TestingBotBuildEnvironment(TestingBotCredentials credentials, @Nullable String buildId, + @Nullable String tunnelIdentifier, @Nullable VirtualChannel channel, @Nullable App localApp) { this.credentials = credentials; - this.app = app; this.buildId = buildId; + this.tunnelIdentifier = tunnelIdentifier; + this.channel = channel; + this.localApp = localApp; } @Override @@ -171,8 +202,9 @@ public void buildEnvVars(Map env) { env.put(TestingBotBuildReportAction.TESTINGBOT_BUILD, buildId); } - if (app != null) { - env.put(TESTINGBOT_TUNNEL,"true"); + if (tunnelIdentifier != null) { + env.put(TESTINGBOT_TUNNEL, "true"); + env.put(TestingBotTunnelStep.TESTINGBOT_TUNNEL_IDENTIFIER, tunnelIdentifier); } super.buildEnvVars(env); @@ -180,7 +212,13 @@ public void buildEnvVars(Map env) { @Override public boolean tearDown(AbstractBuild build, BuildListener listener) throws IOException, InterruptedException { - TunnelManager.stop(app, listener); + if (tunnelIdentifier != null) { + if (channel != null) { + TunnelManager.stopOnChannel(channel, tunnelIdentifier, listener); + } else { + TunnelManager.stop(localApp, listener); + } + } return true; } } diff --git a/src/main/java/testingbot/TunnelManager.java b/src/main/java/testingbot/TunnelManager.java index c3bc2e0..1549dc6 100644 --- a/src/main/java/testingbot/TunnelManager.java +++ b/src/main/java/testingbot/TunnelManager.java @@ -4,12 +4,16 @@ import com.testingbot.tunnel.Api; import com.testingbot.tunnel.App; import hudson.model.TaskListener; +import hudson.remoting.VirtualChannel; import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicInteger; import java.util.regex.Matcher; import java.util.regex.Pattern; +import jenkins.security.MasterToSlaveCallable; /** * Shared TestingBot Tunnel lifecycle helper used by both the freestyle @@ -203,4 +207,115 @@ public static void stop(App app, TaskListener listener) { app.stop(); } } + + // ------------------------------------------------------------------------------------------ + // Remote (agent) tunnel lifecycle. + // + // The tunnel must boot on the same node where the tests run, so that a test connecting to + // {@code localhost} reaches the tunnel. Both the freestyle build wrapper and the pipeline step + // therefore run the tunnel inside a MasterToSlaveCallable on the build node's channel. The live + // App is kept in a per-JVM static registry (co-located with the tunnel on the agent) so teardown + // can find and stop it again without serializing the handle back across the channel. + // ------------------------------------------------------------------------------------------ + + /** Tunnels running in this JVM, keyed by tunnel identifier. Lives in the agent's static space. */ + private static final ConcurrentHashMap RUNNING_TUNNELS = new ConcurrentHashMap<>(); + + /** Monotonic suffix so multiple generated identifiers within one run stay distinct. */ + private static final AtomicInteger TUNNEL_SEQ = new AtomicInteger(); + + /** + * Generates a tunnel identifier unique to a build (and to each tunnel within it), so that + * parallel tunnels on the same node stay isolated. + */ + public static String generateTunnelIdentifier(String jobFullName, int buildNumber) { + return "jenkins-" + jobFullName.replace('/', '-') + "-" + buildNumber + "-" + TUNNEL_SEQ.incrementAndGet(); + } + + /** + * Boots a tunnel on the given channel's node and registers it under {@code tunnelIdentifier}. + * Credentials are passed as already-decrypted strings (only plaintext crosses the encrypted + * remoting link, never the credential object). + */ + public static void startOnChannel(VirtualChannel channel, String key, String secret, String options, + String tunnelIdentifier, TaskListener listener) throws Exception { + channel.call(new StartTunnelHandler(key, secret, options, tunnelIdentifier, listener)); + } + + /** + * Stops the tunnel registered under {@code tunnelIdentifier} on the given channel's node. + * Best-effort: a {@code null} channel (offline agent) or a remoting failure is tolerated so a + * teardown never fails the build; an offline agent is reported to the log. + */ + public static void stopOnChannel(VirtualChannel channel, String tunnelIdentifier, TaskListener listener) { + if (channel == null) { + RUNNING_TUNNELS.remove(tunnelIdentifier); + if (listener != null) { + listener.getLogger().println("[TestingBot] Agent offline: remote teardown of tunnel " + + tunnelIdentifier + " could not be performed. It may still be running on the agent " + + "and should stop when the agent process exits (best-effort cleanup)."); + } + return; + } + try { + channel.call(new StopTunnelHandler(tunnelIdentifier, listener)); + } catch (Exception ignored) { + // best effort + } + } + + private static final class StartTunnelHandler extends MasterToSlaveCallable { + + private static final long serialVersionUID = 1L; + private final String key; + private final String secret; + private final String tunnelOptions; + private final String tunnelIdentifier; + private final TaskListener listener; + + StartTunnelHandler(String key, String secret, String tunnelOptions, String tunnelIdentifier, TaskListener listener) { + this.key = key; + this.secret = secret; + this.tunnelOptions = tunnelOptions; + this.tunnelIdentifier = tunnelIdentifier; + this.listener = listener; + } + + @Override + public Void call() throws Exception { + App app = new App(); + try { + start(app, key, secret, tunnelOptions, tunnelIdentifier, listener); + } catch (Exception e) { + // Startup failed after the App was created; stop it so we don't leak a half-booted tunnel. + try { + app.stop(); + } catch (Exception ignored) { + // best effort + } + throw e; + } + RUNNING_TUNNELS.put(tunnelIdentifier, app); + return null; + } + } + + private static final class StopTunnelHandler extends MasterToSlaveCallable { + + private static final long serialVersionUID = 1L; + private final String tunnelIdentifier; + private final TaskListener listener; + + StopTunnelHandler(String tunnelIdentifier, TaskListener listener) { + this.tunnelIdentifier = tunnelIdentifier; + this.listener = listener; + } + + @Override + public Void call() { + App app = RUNNING_TUNNELS.remove(tunnelIdentifier); + stop(app, listener); + return null; + } + } } diff --git a/src/main/java/testingbot/pipeline/TestingBotTunnelStep.java b/src/main/java/testingbot/pipeline/TestingBotTunnelStep.java index ce98eb4..b5b7c19 100644 --- a/src/main/java/testingbot/pipeline/TestingBotTunnelStep.java +++ b/src/main/java/testingbot/pipeline/TestingBotTunnelStep.java @@ -20,10 +20,7 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.atomic.AtomicInteger; import jenkins.model.Jenkins; -import jenkins.security.MasterToSlaveCallable; import org.jenkinsci.plugins.workflow.steps.BodyExecution; import org.jenkinsci.plugins.workflow.steps.BodyExecutionCallback; import org.jenkinsci.plugins.workflow.steps.BodyInvoker; @@ -46,16 +43,6 @@ public class TestingBotTunnelStep extends Step { /** Env var exposing the (possibly auto-generated) tunnel identifier to the build. */ public static final String TESTINGBOT_TUNNEL_IDENTIFIER = "TESTINGBOT_TUNNEL_IDENTIFIER"; - /** - * Tunnels currently running in this JVM, keyed by tunnel identifier. Replaces the - * previous single static {@code App}, which caused concurrent executions on the same - * node to corrupt and tear down each other's tunnel. - */ - private static final ConcurrentHashMap RUNNING_TUNNELS = new ConcurrentHashMap<>(); - - /** Monotonic suffix so multiple tunnels within one run get distinct generated identifiers. */ - private static final AtomicInteger TUNNEL_SEQ = new AtomicInteger(); - private String options; private String credentialsId; @@ -125,59 +112,6 @@ public ListBoxModel doFillCredentialsIdItems(final @AncestorInPath Item context) } } - private static final class TbStartTunnelHandler extends MasterToSlaveCallable { - - private final String key; - private final String secret; - private final String tunnelOptions; - private final String tunnelIdentifier; - private final TaskListener listener; - - TbStartTunnelHandler(String key, String secret, String tunnelOptions, String tunnelIdentifier, TaskListener listener) { - this.key = key; - this.secret = secret; - this.tunnelOptions = tunnelOptions; - this.tunnelIdentifier = tunnelIdentifier; - this.listener = listener; - } - - @Override - public Void call() throws Exception { - App app = new App(); - try { - TunnelManager.start(app, key, secret, tunnelOptions, tunnelIdentifier, listener); - } catch (Exception e) { - // Startup failed after the App was created; stop it so we don't leak a half-booted tunnel. - try { - app.stop(); - } catch (Exception ignored) { - // best effort - } - throw e; - } - RUNNING_TUNNELS.put(tunnelIdentifier, app); - return null; - } - } - - private static final class TbStopTunnelHandler extends MasterToSlaveCallable { - - private final String tunnelIdentifier; - private final TaskListener listener; - - TbStopTunnelHandler(String tunnelIdentifier, TaskListener listener) { - this.tunnelIdentifier = tunnelIdentifier; - this.listener = listener; - } - - @Override - public Void call() { - App app = RUNNING_TUNNELS.remove(tunnelIdentifier); - TunnelManager.stop(app, listener); - return null; - } - } - public static class TestingBotTunnelStepExecution extends StepExecution { private static final long serialVersionUID = 1L; @@ -228,7 +162,7 @@ public boolean start() throws Exception { String userIdentifier = TunnelManager.extractTunnelIdentifier(options); tunnelIdentifier = (userIdentifier != null && !userIdentifier.isEmpty()) ? userIdentifier - : "jenkins-" + run.getParent().getFullName().replace('/', '-') + "-" + run.getNumber() + "-" + TUNNEL_SEQ.incrementAndGet(); + : TunnelManager.generateTunnelIdentifier(run.getParent().getFullName(), run.getNumber()); // Resolve secrets on the controller; only plaintext crosses the channel (over the encrypted remoting link). String key = tbCredentials.getKey(); @@ -249,7 +183,7 @@ public boolean start() throws Exception { env.put("SELENIUM_HOST", hubHost); env.put("SELENIUM_PORT", hubPort); - requireChannel(computer).call(new TbStartTunnelHandler(key, secret, options, tunnelIdentifier, listener)); + TunnelManager.startOnChannel(requireChannel(computer), key, secret, options, tunnelIdentifier, listener); try { body = getContext().newBodyInvoker() @@ -278,21 +212,7 @@ private static VirtualChannel requireChannel(Computer computer) throws Exception /** Best-effort tunnel teardown that tolerates an offline agent (no channel). */ private static void stopTunnelQuietly(Computer computer, String tunnelIdentifier, TaskListener listener) { - VirtualChannel channel = computer == null ? null : computer.getChannel(); - if (channel == null) { - RUNNING_TUNNELS.remove(tunnelIdentifier); - if (listener != null) { - listener.getLogger().println("[TestingBot] Agent offline: remote teardown of tunnel " - + tunnelIdentifier + " could not be performed. It may still be running on the agent " - + "and should stop when the agent process exits (best-effort cleanup)."); - } - return; - } - try { - channel.call(new TbStopTunnelHandler(tunnelIdentifier, listener)); - } catch (Exception ignored) { - // best effort - } + TunnelManager.stopOnChannel(computer == null ? null : computer.getChannel(), tunnelIdentifier, listener); } @Override From c782a5cf2e20f926d4281673f136c3996a7ad23f Mon Sep 17 00:00:00 2001 From: Jochen Delabie Date: Thu, 16 Jul 2026 17:31:21 +0200 Subject: [PATCH 2/2] Harden node-aware tunnel lifecycle (review feedback on #29) - TestingBotBuildWrapper no longer stores the setUp-time channel; tearDown re-resolves the build node's live channel via build.getBuiltOn().toComputer() so a mid-build agent reconnect can't leave teardown holding a stale channel. The controller-local fallback still stops its own App. - TunnelManager.stopOnChannel restores the interrupt status on InterruptedException and logs other remoting failures instead of silently swallowing them, while staying best-effort (no rethrow). - The start callable reserves the tunnel identifier (putIfAbsent) before booting: a duplicate identifier is rejected instead of overwriting/leaking an existing tunnel, teardown can find an App even mid-boot, and a startup failure removes only this tunnel's own entry. --- .../testingbot/TestingBotBuildWrapper.java | 40 ++++++++++++------- src/main/java/testingbot/TunnelManager.java | 31 ++++++++++++-- 2 files changed, 52 insertions(+), 19 deletions(-) diff --git a/src/main/java/testingbot/TestingBotBuildWrapper.java b/src/main/java/testingbot/TestingBotBuildWrapper.java index fcb329c..d319241 100644 --- a/src/main/java/testingbot/TestingBotBuildWrapper.java +++ b/src/main/java/testingbot/TestingBotBuildWrapper.java @@ -22,6 +22,8 @@ import hudson.model.BuildableItemWithBuildWrappers; import hudson.model.Job; import hudson.model.Item; +import hudson.model.Computer; +import hudson.model.Node; import hudson.model.listeners.ItemListener; import hudson.remoting.VirtualChannel; import hudson.security.ACL; @@ -94,7 +96,7 @@ public Environment setUp(AbstractBuild build, Launcher launcher, BuildListener l if (this.useTunnel) { if (credentials == null) { listener.getLogger().println("No TestingBot key/secret found while trying to start a TestingBot Tunnel"); - return new TestingBotBuildEnvironment(null, buildId, null, null, null); + return new TestingBotBuildEnvironment(null, buildId, null, null); } // Run the tunnel on the build's node (not the controller) so that a test connecting to @@ -115,7 +117,7 @@ public Environment setUp(AbstractBuild build, Launcher launcher, BuildListener l TunnelManager.stopOnChannel(channel, tunnelIdentifier, listener); throw new IOException("Failed to start TestingBot tunnel", ex); } - return new TestingBotBuildEnvironment(credentials, buildId, tunnelIdentifier, channel, null); + return new TestingBotBuildEnvironment(credentials, buildId, tunnelIdentifier, null); } // No agent channel available (unusual launcher); fall back to a controller-local tunnel. @@ -129,10 +131,10 @@ public Environment setUp(AbstractBuild build, Launcher launcher, BuildListener l TunnelManager.stop(app, listener); throw new IOException("Failed to start TestingBot tunnel", ex); } - return new TestingBotBuildEnvironment(credentials, buildId, tunnelIdentifier, null, app); + return new TestingBotBuildEnvironment(credentials, buildId, tunnelIdentifier, app); } - return new TestingBotBuildEnvironment(credentials, buildId, null, null, null); + return new TestingBotBuildEnvironment(credentials, buildId, null, null); } /** @@ -178,17 +180,14 @@ private class TestingBotBuildEnvironment extends BuildWrapper.Environment { private final String buildId; /** Non-null when a tunnel was started for this build. */ private final String tunnelIdentifier; - /** Agent channel the tunnel runs on; null when the tunnel runs controller-locally (see {@link #localApp}). */ - private final VirtualChannel channel; - /** Controller-local tunnel handle, used only in the no-channel fallback. */ + /** Controller-local tunnel handle, used only in the no-channel fallback (otherwise null → runs on the agent). */ private final App localApp; public TestingBotBuildEnvironment(TestingBotCredentials credentials, @Nullable String buildId, - @Nullable String tunnelIdentifier, @Nullable VirtualChannel channel, @Nullable App localApp) { + @Nullable String tunnelIdentifier, @Nullable App localApp) { this.credentials = credentials; this.buildId = buildId; this.tunnelIdentifier = tunnelIdentifier; - this.channel = channel; this.localApp = localApp; } @@ -212,16 +211,27 @@ public void buildEnvVars(Map env) { @Override public boolean tearDown(AbstractBuild build, BuildListener listener) throws IOException, InterruptedException { - if (tunnelIdentifier != null) { - if (channel != null) { - TunnelManager.stopOnChannel(channel, tunnelIdentifier, listener); - } else { - TunnelManager.stop(localApp, listener); - } + if (tunnelIdentifier == null) { + return true; + } + if (localApp != null) { + // Controller-local fallback tunnel. + TunnelManager.stop(localApp, listener); + } else { + // Resolve the agent's live channel now rather than holding a reference captured in + // setUp, which could be stale if the agent reconnected during the build. + TunnelManager.stopOnChannel(currentChannel(build), tunnelIdentifier, listener); } return true; } } + + /** The live channel of the node the build ran on, or null if it is offline/unavailable. */ + private static VirtualChannel currentChannel(AbstractBuild build) { + Node node = build.getBuiltOn(); + Computer computer = node == null ? null : node.toComputer(); + return computer == null ? null : computer.getChannel(); + } protected boolean migrateCredentials(AbstractProject project) { Logger.getLogger(TestingBotBuildWrapper.class.getName()).log(Level.INFO, "TestingBot Plugin: migrateCredentials: " + this.credentialsId); diff --git a/src/main/java/testingbot/TunnelManager.java b/src/main/java/testingbot/TunnelManager.java index 1549dc6..169b7f4 100644 --- a/src/main/java/testingbot/TunnelManager.java +++ b/src/main/java/testingbot/TunnelManager.java @@ -259,8 +259,17 @@ public static void stopOnChannel(VirtualChannel channel, String tunnelIdentifier } try { channel.call(new StopTunnelHandler(tunnelIdentifier, listener)); - } catch (Exception ignored) { - // best effort + } catch (InterruptedException ie) { + // Restore the interrupt status; teardown stays best-effort and does not rethrow. + Thread.currentThread().interrupt(); + if (listener != null) { + listener.getLogger().println("[TestingBot] Interrupted while stopping tunnel " + tunnelIdentifier); + } + } catch (Exception e) { + // Best-effort, but do not silently discard the failure — record why teardown could not complete. + if (listener != null) { + listener.getLogger().println("[TestingBot] Failed to stop tunnel " + tunnelIdentifier + ": " + e); + } } } @@ -284,10 +293,25 @@ private static final class StartTunnelHandler extends MasterToSlaveCallable