TS-45056 Rework

Author: stahlbauer

CHANGELOG.md

@@ -6,6 +6,7 @@ We use [semantic versioning](http://semver.org/):
 
 # Next version
 - [fix] _agent_: Fixed `IllegalStateException: Can't add different class with same name` when application servers like JBoss perform a reload without full restart or when duplicate class files exist across multiple archives
+- [fix] _agent_: The profiler no longer aborts the profiled application when its options are malformed or startup fails for any other reason. The startup failure is logged and the application continues to run without coverage collection
 - [feature] _teamscale-gradle-plugin_: Annotated tasks with `@DisableCachingByDefault` where caching can't be applied
 
 # 36.4.0

agent/src/main/java/com/teamscale/jacoco/agent/PreMain.java

@@ -35,7 +35,17 @@
 
 import static com.teamscale.jacoco.agent.logging.LoggingUtils.getLoggerContext;
 
-/** Container class for the premain entry point for the agent. */
+/**
+ * Container class for the premain entry point for the agent.
+ * <p>
+ * <b>Design decision: the agent must never crash the profiled application.</b>
+ * Even when the agent is misconfigured, startup must not propagate any exception out of
+ * {@link #premain(String, Instrumentation)}. An exception escaping {@code premain} would be re-thrown by the JVM as
+ * {@code IllegalAgentException}, which aborts startup of the profiled application. The profiler is an observational
+ * tool, so a failure to start up the profiler must degrade to "no coverage is collected" rather than taking down the
+ * application. All failure paths in {@code premain} are therefore logged and swallowed; see the top-level
+ * {@code try}/{@code catch} in {@link #premain(String, Instrumentation)}.
+ */
 public class PreMain {
 
 	private static LoggingUtils.LoggingResources loggingResources = null;
@@ -60,8 +70,26 @@ public class PreMain {
 
 	/**
 	 * Entry point for the agent, called by the JVM.
+	 * <p>
+	 * This method never throws. Any exception raised during startup is logged (to the file log and, if configured,
+	 * to Teamscale) and swallowed, and the JVM continues without coverage collection. See the class-level Javadoc for
+	 * the rationale.
 	 */
-	public static void premain(String options, Instrumentation instrumentation) throws Exception {
+	public static void premain(String options, Instrumentation instrumentation) {
+		try {
+			startProfiler(options, instrumentation);
+		} catch (Throwable t) {
+			// Top-level safety net: an exception escaping premain would be re-thrown by the JVM as
+			// IllegalAgentException and abort the profiled application. See class-level Javadoc.
+			logStartupFailure(t);
+		}
+	}
+
+	/**
+	 * Starts the profiler. Contains all logic that may legitimately throw during startup; every caller must route
+	 * through {@link #premain(String, Instrumentation)} which catches these exceptions.
+	 */
+	private static void startProfiler(String options, Instrumentation instrumentation) throws Exception {
 		if (System.getProperty(LOCKING_SYSTEM_PROPERTY) != null) {
 			return;
 		}
@@ -81,8 +109,8 @@ public static void premain(String options, Instrumentation instrumentation) thro
 					environmentConfigFile);
 			agentOptions = parseResult.getFirst();
 
-			// After parsing everything and configuring logging, we now
-			// can throw the caught exceptions.
+			// After parsing everything and configuring logging, we now can throw the caught exceptions. They are
+			// handled locally below and never propagate out of premain (see class-level Javadoc).
 			for (Exception exception : parseResult.getSecond()) {
 				throw exception;
 			}
@@ -97,7 +125,9 @@ public static void premain(String options, Instrumentation instrumentation) thro
 				agentOptions.configurationViaTeamscale.unregisterProfiler();
 			}
 
-			throw e;
+			// Previously rethrown. We now swallow the exception so a misconfigured agent cannot abort the profiled
+			// application. The error has been logged above.
+			return;
 		} catch (AgentOptionReceiveException e) {
 			// When Teamscale is not available, we don't want to fail hard to still allow for testing even if no
 			// coverage is collected (see TS-33237)
@@ -118,6 +148,29 @@ public static void premain(String options, Instrumentation instrumentation) thro
 		agent.registerShutdownHook();
 	}
 
+	/**
+	 * Logs a startup failure on a best-effort basis. Must not throw: even logging failures are swallowed so that
+	 * {@link #premain(String, Instrumentation)} keeps its no-throw contract.
+	 */
+	private static void logStartupFailure(Throwable t) {
+		try {
+			LoggingUtils.getLogger(PreMain.class).error(
+					"Teamscale Java Profiler failed to start up. The profiled application will continue to run "
+							+ "without coverage collection.", t);
+		} catch (Throwable loggingFailure) {
+			// The logger may not be initialized yet or may itself be broken. Fall back to stderr so the failure is
+			// at least visible somewhere, then give up silently.
+			try {
+				System.err.println(
+						"[Teamscale Java Profiler] Failed to start up and will not collect coverage: " + t);
+				loggingFailure.addSuppressed(t);
+				loggingFailure.printStackTrace();
+			} catch (Throwable ignored) {
+				// Nothing we can do; the alternative is to crash the profiled application, which we must not do.
+			}
+		}
+	}
+
 	private static Pair<AgentOptions, List<Exception>> getAndApplyAgentOptions(String options,
 			String environmentConfigId,
 			String environmentConfigFile) throws AgentOptionParseException, IOException, AgentOptionReceiveException {
@@ -204,9 +257,14 @@ private static void initializeLogging(AgentOptions agentOptions, DelayedLogger l
 		}
 	}
 
-	/** Closes the opened logging contexts. */
+	/**
+	 * Closes the opened logging contexts. Safe to call before logging has been initialized, in which case this is a
+	 * no-op.
+	 */
 	static void closeLoggingResources() {
-		loggingResources.close();
+		if (loggingResources != null) {
+			loggingResources.close();
+		}
 	}
 
 	/**

agent/src/test/java/com/teamscale/jacoco/agent/PreMainTest.java

@@ -0,0 +1,26 @@
+package com.teamscale.jacoco.agent;
+
+import org.junit.jupiter.api.Test;
+
+import java.lang.instrument.Instrumentation;
+
+import static org.assertj.core.api.Assertions.assertThatCode;
+
+/**
+ * Verifies the cross-cutting design invariant documented on {@link PreMain}: a failure to start up the profiler
+ * (misconfiguration, missing files, environment issues, ...) must never propagate an exception out of
+ * {@link PreMain#premain(String, Instrumentation)}, because the JVM would turn that into an
+ * {@code IllegalAgentException} and abort the profiled application.
+ */
+class PreMainTest {
+
+	/**
+	 * Invalid agent options produce an {@code AgentOptionParseException} internally. The agent must swallow it so the
+	 * profiled application keeps running.
+	 */
+	@Test
+	void premainDoesNotThrowOnInvalidOptions() {
+		assertThatCode(() -> PreMain.premain("this=is=not=a=valid=option,bogus-key=value", null))
+				.doesNotThrowAnyException();
+	}
+}

report-generator/src/main/kotlin/com/teamscale/report/EDuplicateClassFileBehavior.kt

@@ -2,6 +2,11 @@ package com.teamscale.report
 
 /**
  * Behavior when two non-identical class files with the same package name are found.
+ *
+ * Note for use inside the agent: the agent must never crash the profiled application (see
+ * `PreMain` class Javadoc). [FAIL] therefore only aborts the individual dump; `Agent.dumpReport`
+ * catches the resulting exception and logs it. Outside the agent (for example, in the CLI
+ * converter or the Gradle plugin) [FAIL] surfaces as an exception to the caller as usual.
  */
 enum class EDuplicateClassFileBehavior {
 	/** Completely ignores it.  */
@@ -10,6 +15,6 @@ enum class EDuplicateClassFileBehavior {
 	/** Prints a warning to the logger.  */
 	WARN,
 
-	/** Fails and stops further processing.  */
+	/** Fails and aborts the current report generation. See class-level note on agent semantics.  */
 	FAIL
 }