Don't call System.exit() when the tunnel is embedded - #29
Merged
Conversation
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.
jochen-testingbot
added a commit
to testingbot/testingbot-teamcity-plugin
that referenced
this pull request
Jul 29, 2026
com.testingbot.tunnel.App is a command line client that calls System.exit() on failure and installs its own shutdown hooks. The agent plugin ran it inside the build agent JVM, so a tunnel that could not start took the whole agent down with it. Reproduced on TeamCity 2024.12 with invalid credentials: the agent logged "JVM process got terminate signal" 224ms after "Starting TestingBot Tunnel", the build hung in running state indefinitely and the agent never re-registered until it was restarted. The existing try/catch around boot() could not help, because System.exit() does not throw. This is long standing rather than new; TestingBotTunnel 2.4 has the same exit inside boot(). A fix for the library side is proposed in testingbot/Testingbot-Tunnel#29, but running the tunnel out of process protects the agent regardless of which tunnel version is installed. TunnelProcess launches the tunnel with the plugin's own lib directory as the classpath, waits for the --readyfile it touches when it is usable, and streams its output into the build log. Credentials are passed through the environment rather than the command line so the secret does not appear in the process list. Stopping asks politely first so the tunnel can tear down its server side, then kills it after 30 seconds. The tunnel jar drops to runtimeOnly, since nothing compiles against it any more. TunnelProcessTest launches the real tunnel jar with unusable credentials. If this ever regresses to running in process, the tests do not fail, they kill the Gradle test JVM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Appis used as a library as well as a command line client. The TeamCity agent plugin constructs anAppand callsboot()inside the build agent JVM.When the tunnel fails to start,
boot()calledSystem.exit(1). Embedded, that terminates the host process. Observed on a TeamCity 2024.12 agent with deliberately invalid credentials:The running build then hung in "running" indefinitely, and the agent never re-registered — it needed a manual restart. The calling plugin already wraps
boot()in a try/catch, but that cannot help, becauseSystem.exit()does not throw.This is long-standing rather than new:
boot()has had aSystem.exitsince at least 2.4.The fix
Replace the exits on the embedding path with a
TunnelFailedExceptioncarrying the exit code, and letmain()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 port cannot be opened.tunnelReady()rethrowsTunnelFailedException(while still swallowing/logging everything else as before) so the failure reachesmain()on the synchronous pathdoctor()throws instead of exitingtrackPid()moves fromboot()intomain(). The pid file exists so an external supervisor can stop the process — a command line concern — andPidPollercallsSystem.exit(0)from a timer thread when the file disappears. That was a background thread able to kill a host JVM at any moment, e.g. if a CI workspace cleaner removed the file. Embedders no longer get it.stop()now removes the shutdown hook registered byinit(), cancels the pid poller, and tolerates a nullapi. Starting a tunnel per job previously leaked one shutdown hook perAppfor the lifetime of the JVM, andstop()after a failedboot()logged a confusing NPE stack trace.After this, the only remaining
System.exitcalls are inmain(), plusPidPoller, which is now only reachable frommain().Compatibility
TunnelFailedException extends RuntimeException, so no existing signature changes and nothing that compiles today stops compiling. This also matches the existingHttpProxyStartExceptionidiom in this codebase.boot()creating the pid file will no longer get one. That seemed clearly desirable, but say the word if you'd rather keep it and gate it on a flag instead.Tests
mvn test: 192 passing, 0 failures (184 before, 8 added).AppEmbeddedTestdrivesboot()against a WireMock-stubbed API and asserts the failure surfaces as an exception. These tests are somewhat self-enforcing: ifboot()ever goes back to callingSystem.exit, they don't just fail, they crash the surefire JVM.To make that testable I added a package-private
App.createApi()seam so a test subclass can point the API at WireMock —boot()previously constructednew Api(this)inline with no way to intercept it. Happy to change the approach if you'd prefer a different seam.Known limitation
On the asynchronous path (
TunnelPollertimer thread callingtunnelReady()), there is no caller to propagate to, so a proxy setup failure is now logged with its real message instead of terminating the process. Previously it calledSystem.exit(1)from that timer thread. Surfacing it properly would need a failure callback from the poller back to the caller, which felt like a design decision for you rather than something to slip into this PR.