Run the freestyle TestingBot Tunnel on the build's agent, not the controller#29
Conversation
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.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/testingbot/TestingBotBuildWrapper.java`:
- Line 118: Update TestingBotBuildEnvironment construction in the surrounding
wrapper method to persist stable node identity instead of the original
launcher.getChannel(). In tearDown, resolve the current agent Computer and
obtain its live channel via Computer.getChannel() before performing tunnel
cleanup, while preserving the existing cleanup behavior when no channel is
available.
In `@src/main/java/testingbot/TunnelManager.java`:
- Around line 284-299: Update the tunnel startup flow in the callable’s call
method to reserve tunnelIdentifier in the running-tunnel registry before
invoking start, reject collisions without overwriting an existing entry, and
coordinate cleanup so stop waits for or safely handles in-progress startup. On
startup failure, remove only the entry owned by this tunnel and stop the created
App as needed; preserve the registry ownership through successful startup.
- Around line 260-264: Update stopOnChannel’s exception handling around
StopTunnelHandler so InterruptedException restores the thread’s interrupted
status, while other teardown exceptions are logged with relevant failure details
before returning. Preserve the existing best-effort behavior without silently
discarding remoting failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 11b51091-2b66-41db-9c51-bee420a1b5cb
📒 Files selected for processing (3)
src/main/java/testingbot/TestingBotBuildWrapper.javasrc/main/java/testingbot/TunnelManager.javasrc/main/java/testingbot/pipeline/TestingBotTunnelStep.java
| TunnelManager.stopOnChannel(channel, tunnelIdentifier, listener); | ||
| throw new IOException("Failed to start TestingBot tunnel", ex); | ||
| } | ||
| return new TestingBotBuildEnvironment(credentials, buildId, tunnelIdentifier, channel, null); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show file with line numbers around the cited regions
sed -n '1,280p' src/main/java/testingbot/TestingBotBuildWrapper.java | nl -ba | sed -n '1,280p'
# Search for tunnel tracking and channel resolution
rg -n "RUNNING_TUNNELS|getChannel\\(|VirtualChannel|Computer" src/main/java/testingbot/TestingBotBuildWrapper.java src/main/java -SRepository: testingbot/TestingBot-Jenkins-Plugin
Length of output: 214
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant file with line numbers using awk (nl is unavailable here)
awk '{printf "%5d %s\n", NR, $0}' src/main/java/testingbot/TestingBotBuildWrapper.java | sed -n '1,280p'
# Search for channel and tunnel bookkeeping references
rg -n "RUNNING_TUNNELS|getChannel\\(|VirtualChannel|Computer" src/main/java/testingbot/TestingBotBuildWrapper.java src/main/java -SRepository: testingbot/TestingBot-Jenkins-Plugin
Length of output: 16213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the tunnel manager methods around start/stop on channel and RUNNING_TUNNELS usage
awk '{printf "%5d %s\n", NR, $0}' src/main/java/testingbot/TunnelManager.java | sed -n '220,340p'
# Inspect the pipeline step's channel handling for comparison
awk '{printf "%5d %s\n", NR, $0}' src/main/java/testingbot/pipeline/TestingBotTunnelStep.java | sed -n '190,260p'Repository: testingbot/TestingBot-Jenkins-Plugin
Length of output: 8054
Resolve the current agent channel in tearDown. Storing launcher.getChannel() in TestingBotBuildEnvironment ties cleanup to the original remoting connection; if the agent reconnects, teardown can no longer reach the live JVM and the tunnel can remain in RUNNING_TUNNELS until the agent exits. Persist stable node identity and resolve Computer.getChannel() during teardown instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TestingBotBuildWrapper.java` at line 118, Update
TestingBotBuildEnvironment construction in the surrounding wrapper method to
persist stable node identity instead of the original launcher.getChannel(). In
tearDown, resolve the current agent Computer and obtain its live channel via
Computer.getChannel() before performing tunnel cleanup, while preserving the
existing cleanup behavior when no channel is available.
| try { | ||
| channel.call(new StopTunnelHandler(tunnelIdentifier, listener)); | ||
| } catch (Exception ignored) { | ||
| // best effort | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and surrounding methods
git ls-files src/main/java/testingbot/TunnelManager.java
ast-grep outline src/main/java/testingbot/TunnelManager.java --view expanded || true
# Show the target section with line numbers
sed -n '220,320p' src/main/java/testingbot/TunnelManager.java
# Search for teardown/logging helpers and interrupt handling in the file
rg -n "logTeardownFailure|InterruptedException|best effort|StopTunnelHandler|stopOnChannel|interrupt\\(" src/main/java/testingbot/TunnelManager.javaRepository: testingbot/TestingBot-Jenkins-Plugin
Length of output: 6168
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find call sites and related teardown logic
rg -n "stopOnChannel\\(|stop\\(App app, TaskListener listener\\)|channel.call\\(new StopTunnelHandler|RUNNING_TUNNELS.remove\\(" src/main/java src/test/java
# Show the public stop method and nearby context
sed -n '190,250p' src/main/java/testingbot/TunnelManager.javaRepository: testingbot/TestingBot-Jenkins-Plugin
Length of output: 4505
Preserve interruption and report teardown failures. stopOnChannel swallows InterruptedException and all remoting failures, so cancellation state is lost and a remote tunnel stop can fail silently. Restore the interrupt flag and log the failure before returning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TunnelManager.java` around lines 260 - 264, Update
stopOnChannel’s exception handling around StopTunnelHandler so
InterruptedException restores the thread’s interrupted status, while other
teardown exceptions are logged with relevant failure details before returning.
Preserve the existing best-effort behavior without silently discarding remoting
failures.
| @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; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Reserve registry ownership before booting the tunnel.
The tunnel is undiscoverable until start finishes. If channel.call is interrupted while the remote callable continues, cleanup can remove nothing before line 298 subsequently registers a live tunnel. The unconditional put also overwrites an existing user-supplied identifier, making later teardown stop the wrong instance.
Install a collision-aware lifecycle entry before booting, coordinate stop with in-progress startup, and remove it conditionally on failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TunnelManager.java` around lines 284 - 299, Update
the tunnel startup flow in the callable’s call method to reserve
tunnelIdentifier in the running-tunnel registry before invoking start, reject
collisions without overwriting an existing entry, and coordinate cleanup so stop
waits for or safely handles in-progress startup. On startup failure, remove only
the entry owned by this tunnel and stop the created App as needed; preserve the
registry ownership through successful startup.
- 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.
PRs #29 and #30 were merged into the build-report-embed branch, but that branch had already been squash-merged to master as #28 beforehand, so their code never reached master. This brings the stranded changes forward, on top of the current master (which already has #28 and the #31 inline-media feature): - #29: run the freestyle tunnel on the build's agent (TunnelManager.startOnChannel/ stopOnChannel + shared registry; wrapper + testingbotTunnel step delegate to it). - #30: TestingBotChecksPublisher (testingbotChecks) publishing results as a GitHub check, plus the checks-api + display-url-api dependencies and docs. No behavior change versus the already-reviewed #29/#30; only rebased onto master. The inline-media (#31) files are left untouched.
1 participant
What
Fixes distributed-build support: the freestyle TestingBot Tunnel now runs on the build's agent, not the Jenkins controller.
Problem
TestingBotBuildWrapper.setUpconstructednew App()and booted the tunnel directly in the controller JVM — theLauncher(which holds the agent channel) was never used. For a build running on an agent, the tunnel came up on the controller, so a test connecting tolocalhostnever reached it. The pipelinetestingbotTunnelstep already handled this correctly by booting the tunnel inside aMasterToSlaveCallableon the build node's channel.Change
Extract the pipeline step's proven mechanism into
TunnelManagerso both entry points share one implementation and one registry:TunnelManager.startOnChannel(...)/stopOnChannel(...)— boot/stop the tunnel on a given node'sVirtualChannel, keeping the liveAppin a per-JVMRUNNING_TUNNELSmap co-located with the tunnel (so teardown finds it without shipping the handle back).TunnelManager.generateTunnelIdentifier(job, buildNumber)— unique per build/tunnel.TestingBotTunnelStepnow delegates to these (its private callables/registry removed); observable behavior unchanged.TestingBotBuildWrapperruns the tunnel vialauncher.getChannel(), stores the channel + identifier fortearDown, exposesTESTINGBOT_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.Validation
mvn -DskipTests verify(SpotBugs on) — clean.hpi:runscript console: the relocatedMasterToSlaveCallableserializes cleanly (crosses to the agent);testingbotTunnelstep + freestyle wrapper descriptors both register; freestyleuseTunnelconfig round-trips across reload.Note
Stacked on #28 (
build-report-embed) to avoid conflicts inTestingBotBuildWrapper.setUp. Retarget tomasteronce #28 merges.Summary by CodeRabbit
New Features
Bug Fixes