From ff54d702cd713b942465e6b0ec7a95960f52e0ec Mon Sep 17 00:00:00 2001 From: Jochen Delabie Date: Tue, 28 Jul 2026 21:37:04 +0200 Subject: [PATCH] Don't call System.exit() when the tunnel is embedded App is used as a library as well as a command line client. The TeamCity agent plugin, for example, constructs an App and calls boot() inside the build agent JVM. When the tunnel failed to start, boot() called System.exit(1), which terminated the whole build agent: the running build hung indefinitely and the agent never re-registered until it was restarted manually. A try/catch around boot() cannot defend against this, because System.exit() does not throw. Replace the exits on the embedding path with a TunnelFailedException that carries the exit code, and let main() catch it and exit with exactly the same codes as before, so command line behaviour is unchanged. - boot() throws instead of exiting when tunnel creation fails or the API returns an error (including 401) - startProxies() throws instead of exiting when the local proxy cannot be started; tunnelReady() rethrows it so the failure still reaches main() - doctor() throws instead of exiting - trackPid() moves from boot() to main(). The pid file exists so an external supervisor can stop the process, which is a command line concern, and PidPoller calls System.exit(0) from a timer thread when the file disappears. Embedders no longer get that background thread. - stop() removes the shutdown hook registered by init(), cancels the pid poller and tolerates a null api, so starting a tunnel per job no longer leaks a hook per App instance TunnelFailedException extends RuntimeException so no existing signature changes and nothing that compiles today stops compiling. --- src/main/java/com/testingbot/tunnel/App.java | 66 ++++++--- .../tunnel/TunnelFailedException.java | 30 ++++ src/main/java/ssh/TunnelPoller.java | 6 + .../testingbot/tunnel/AppEmbeddedTest.java | 128 ++++++++++++++++++ .../tunnel/TunnelFailedExceptionTest.java | 40 ++++++ 5 files changed, 252 insertions(+), 18 deletions(-) create mode 100644 src/main/java/com/testingbot/tunnel/TunnelFailedException.java create mode 100644 src/test/java/com/testingbot/tunnel/AppEmbeddedTest.java create mode 100644 src/test/java/com/testingbot/tunnel/TunnelFailedExceptionTest.java diff --git a/src/main/java/com/testingbot/tunnel/App.java b/src/main/java/com/testingbot/tunnel/App.java index f9ebe3b..c4eed8e 100644 --- a/src/main/java/com/testingbot/tunnel/App.java +++ b/src/main/java/com/testingbot/tunnel/App.java @@ -463,14 +463,21 @@ public static void main(String... args) throws Exception { app.init(); app.boot(); + // The pid file lets an external supervisor stop this process; it is + // only meaningful when running as a command line client. + app.trackPid(); } catch (ParseException parseException) { System.err.println(parseException.getMessage()); System.exit(2); + } catch (TunnelFailedException tunnelFailedException) { + System.err.println(tunnelFailedException.getMessage()); + System.exit(tunnelFailedException.getExitCode()); } } private PidPoller pidPoller; private TunnelPoller poller; private HttpForwarder httpForwarder; + private Thread cleanupThread; private String[] getUserData() { if (System.getenv("TESTINGBOT_KEY") != null && System.getenv("TESTINGBOT_SECRET") != null) { @@ -492,7 +499,7 @@ private String[] getUserData() { } public void init() { - Thread cleanupThread = new Thread() { + cleanupThread = new Thread() { @Override public void run() { if (readyFile != null) { @@ -518,28 +525,28 @@ public void run() { Runtime.getRuntime().addShutdownHook(cleanupThread); } + Api createApi() { + return new Api(this); + } + public void boot() throws Exception { - api = new Api(this); + api = createApi(); JsonNode tunnelData = null; try { tunnelData = api.createTunnel(); } catch (Exception e) { - System.err.println("Creating a new tunnel failed, please make sure you're supplying correct credentials and that you can connect to the TestingBot network.\nUse --doctor to verify if everything is set up correctly."); - System.err.println(e.getMessage()); - System.exit(1); + throw new TunnelFailedException("Creating a new tunnel failed, please make sure you're supplying correct credentials and that you can connect to the TestingBot network.\nUse --doctor to verify if everything is set up correctly.\n" + e.getMessage(), 1, e); } if (tunnelData.has("error")) { - System.err.println("An error ocurred: " + tunnelData.get("error").asText()); + String error = "An error ocurred: " + tunnelData.get("error").asText(); if (tunnelData.get("error").asText().contains("401")) { - System.err.println("Missing required arguments API_KEY API_SECRET\nYou can get these two values from https://testingbot.com/members/user/edit"); + error += "\nMissing required arguments API_KEY API_SECRET\nYou can get these two values from https://testingbot.com/members/user/edit"; } - System.exit(1); + throw new TunnelFailedException(error, 1); } - trackPid(); - startInsightServer(); if (tunnelData.has("id")) { @@ -585,11 +592,31 @@ public void stop() { poller.cancel(); } - try { - System.out.println("Shutting down your personal Tunnel Server."); - api.destroyTunnel(); - } catch (Exception ex) { - Logger.getLogger(App.class.getName()).log(Level.SEVERE, null, ex); + if (pidPoller != null) { + pidPoller.cancel(); + pidPoller = null; + } + + // Without this, an embedder that starts a tunnel per job leaks one + // shutdown hook per App instance for the lifetime of the JVM. + if (cleanupThread != null) { + try { + Runtime.getRuntime().removeShutdownHook(cleanupThread); + } catch (IllegalStateException alreadyShuttingDown) { + // the JVM is on its way down and will run the hook itself + } + cleanupThread = null; + } + + // api is null when stop() is called after boot() failed, which is the + // normal path for an embedder cleaning up in a finally block. + if (api != null) { + try { + System.out.println("Shutting down your personal Tunnel Server."); + api.destroyTunnel(); + } catch (Exception ex) { + Logger.getLogger(App.class.getName()).log(Level.SEVERE, null, ex); + } } } @@ -607,6 +634,10 @@ public void tunnelReady(JsonNode apiResponse) { Logger.getLogger(App.class.getName()).log(Level.INFO, "The Tunnel is ready, ip: {0}\nYou may start your tests.", _serverIP); Logger.getLogger(App.class.getName()).log(Level.INFO, "To stop the tunnel, press CTRL+C"); } + } catch (TunnelFailedException tunnelFailedException) { + // fatal: let it reach the caller of boot() so the command line + // client can exit and an embedder can handle it + throw tunnelFailedException; } catch (Exception ex) { Logger.getLogger(App.class.getName()).log(Level.INFO, "Something went wrong while setting up the Tunnel."); Logger.getLogger(App.class.getName()).log(Level.SEVERE, null, ex); @@ -624,8 +655,7 @@ private void startProxies() { try { this.httpProxy = new HttpProxy(this); } catch (HttpProxy.HttpProxyStartException ex) { - Logger.getLogger(App.class.getName()).log(Level.SEVERE, ex.getMessage()); - System.exit(1); + throw new TunnelFailedException(ex.getMessage(), 1, ex); } if (this.getProxy() == null && !this.httpProxy.testProxy()) { Logger.getLogger(App.class.getName()).log(Level.INFO, "! Tunnel might not work properly, test failed"); @@ -650,7 +680,7 @@ private void startProxies() { public void doctor() { Doctor doctor = new Doctor(this); if (doctor.hasFailures()) { - System.exit(1); + throw new TunnelFailedException("Doctor detected one or more problems, see the output above.", 1); } } diff --git a/src/main/java/com/testingbot/tunnel/TunnelFailedException.java b/src/main/java/com/testingbot/tunnel/TunnelFailedException.java new file mode 100644 index 0000000..34afb92 --- /dev/null +++ b/src/main/java/com/testingbot/tunnel/TunnelFailedException.java @@ -0,0 +1,30 @@ +package com.testingbot.tunnel; + +/** + * Thrown when the tunnel cannot be started or has to be aborted. + * + * Carries the exit code the command line client should terminate with, so that + * {@link App#main(String...)} keeps its existing exit codes while embedders can + * catch this instead of having their JVM terminated. + */ +public class TunnelFailedException extends RuntimeException { + private final int exitCode; + + public TunnelFailedException(String message) { + this(message, 1); + } + + public TunnelFailedException(String message, int exitCode) { + super(message); + this.exitCode = exitCode; + } + + public TunnelFailedException(String message, int exitCode, Throwable cause) { + super(message, cause); + this.exitCode = exitCode; + } + + public int getExitCode() { + return exitCode; + } +} diff --git a/src/main/java/ssh/TunnelPoller.java b/src/main/java/ssh/TunnelPoller.java index 6ded544..44cdc51 100644 --- a/src/main/java/ssh/TunnelPoller.java +++ b/src/main/java/ssh/TunnelPoller.java @@ -2,6 +2,7 @@ import com.testingbot.tunnel.Api; import com.testingbot.tunnel.App; +import com.testingbot.tunnel.TunnelFailedException; import java.util.Timer; import java.util.TimerTask; import java.util.logging.Level; @@ -51,6 +52,11 @@ public void run() { this.counter += 1; Logger.getLogger(TunnelPoller.class.getName()).log(Level.INFO, "Current tunnel status: {0}", response.get("state").asText()); } + } catch (TunnelFailedException tunnelFailedException) { + // the tunnel became ready but could not be set up; this runs on a + // timer thread so there is nobody to propagate to, report it here + timer.cancel(); + Logger.getLogger(TunnelPoller.class.getName()).log(Level.SEVERE, tunnelFailedException.getMessage()); } catch (Exception ex) { timer.cancel(); Logger.getLogger(TunnelPoller.class.getName()).log(Level.SEVERE, "Unable to poll for tunnel status."); diff --git a/src/test/java/com/testingbot/tunnel/AppEmbeddedTest.java b/src/test/java/com/testingbot/tunnel/AppEmbeddedTest.java new file mode 100644 index 0000000..ef894b4 --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/AppEmbeddedTest.java @@ -0,0 +1,128 @@ +package com.testingbot.tunnel; + +import com.github.tomakehurst.wiremock.WireMockServer; +import com.github.tomakehurst.wiremock.client.WireMock; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import java.lang.reflect.Field; + +import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.options; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Verifies that App can be embedded in a host process: a failing tunnel must + * report the failure to the caller instead of terminating the JVM. + * + * These tests would not merely fail but crash the surefire JVM if App went back + * to calling System.exit on the boot path. + */ +class AppEmbeddedTest { + + private WireMockServer wireMockServer; + private App app; + + @BeforeEach + void setUp() { + wireMockServer = new WireMockServer(options().dynamicPort()); + wireMockServer.start(); + WireMock.configureFor("localhost", wireMockServer.port()); + + app = new App() { + @Override + Api createApi() { + Api api = new Api(this); + api.setApiScheme("http"); + api.setApiHost("localhost:" + wireMockServer.port()); + return api; + } + }; + app.setClientKey("test_key"); + app.setClientSecret("test_secret"); + } + + @AfterEach + void tearDown() { + if (wireMockServer != null) { + wireMockServer.stop(); + } + } + + @Test + void boot_whenCredentialsAreRejected_shouldThrowInsteadOfExiting() { + // Given: the API reports an authentication failure + wireMockServer.stubFor(post(urlPathEqualTo("/v1/tunnel/create")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody("{\"error\":\"401 Unauthorized\"}"))); + + // When & Then: the caller gets an exception, the JVM survives + assertThatThrownBy(() -> app.boot()) + .isInstanceOf(TunnelFailedException.class) + .hasMessageContaining("401"); + } + + @Test + void boot_whenTunnelCreationFails_shouldThrowInsteadOfExiting() { + // Given: the API is unreachable / returns something unusable + wireMockServer.stubFor(post(urlPathEqualTo("/v1/tunnel/create")) + .willReturn(aResponse().withStatus(500).withBody("nope"))); + + // When & Then + assertThatThrownBy(() -> app.boot()) + .isInstanceOf(TunnelFailedException.class); + } + + @Test + void boot_shouldNotStartPidPolling() throws Exception { + // Given: pid tracking is a command line concern; a background timer that + // calls System.exit has no business running inside a host process + wireMockServer.stubFor(post(urlPathEqualTo("/v1/tunnel/create")) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody("{\"error\":\"401 Unauthorized\"}"))); + + // When + assertThatThrownBy(() -> app.boot()).isInstanceOf(TunnelFailedException.class); + + // Then + assertThat(readField(app, "pidPoller")).isNull(); + } + + @Test + void stop_shouldUnregisterTheShutdownHook() throws Exception { + // Given + app.init(); + assertThat(readField(app, "cleanupThread")).isNotNull(); + + // When + app.stop(); + + // Then: an embedder starting a tunnel per job must not leak a hook each time + assertThat(readField(app, "cleanupThread")).isNull(); + } + + @Test + void stop_shouldBeSafeToCallRepeatedly() { + // Given + app.init(); + + // When & Then + assertThatCode(() -> { + app.stop(); + app.stop(); + }).doesNotThrowAnyException(); + } + + private Object readField(App target, String name) throws Exception { + Field field = App.class.getDeclaredField(name); + field.setAccessible(true); + return field.get(target); + } +} diff --git a/src/test/java/com/testingbot/tunnel/TunnelFailedExceptionTest.java b/src/test/java/com/testingbot/tunnel/TunnelFailedExceptionTest.java new file mode 100644 index 0000000..3c54f00 --- /dev/null +++ b/src/test/java/com/testingbot/tunnel/TunnelFailedExceptionTest.java @@ -0,0 +1,40 @@ +package com.testingbot.tunnel; + +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +class TunnelFailedExceptionTest { + + @Test + void defaultExitCode_shouldBeOne() { + // Given & When + TunnelFailedException exception = new TunnelFailedException("boom"); + + // Then + assertThat(exception.getMessage()).isEqualTo("boom"); + assertThat(exception.getExitCode()).isEqualTo(1); + } + + @Test + void exitCode_shouldBePreserved() { + // Given & When + TunnelFailedException exception = new TunnelFailedException("boom", 2); + + // Then + assertThat(exception.getExitCode()).isEqualTo(2); + } + + @Test + void cause_shouldBePreserved() { + // Given + Exception cause = new IllegalStateException("underlying"); + + // When + TunnelFailedException exception = new TunnelFailedException("boom", 1, cause); + + // Then + assertThat(exception).hasCause(cause); + assertThat(exception.getExitCode()).isEqualTo(1); + } +}