TS-45056 Rework

Author: stahlbauer

CHANGELOG.md

@@ -5,8 +5,7 @@ We use [semantic versioning](http://semver.org/):
 - PATCH version when you make backwards compatible bug fixes.
 
 # 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
-- [feature] _agent_: Automatically detects application server reloads (for example, JBoss reload) and clears stale class files from the dump directory
+- [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
 - [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/Agent.java

@@ -21,17 +21,13 @@
 import org.glassfish.jersey.server.ResourceConfig;
 import org.glassfish.jersey.server.ServerProperties;
 
-import org.jacoco.core.data.ExecutionData;
-
 import java.io.File;
 import java.io.IOException;
 import java.lang.instrument.Instrumentation;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.time.Duration;
-import java.util.HashMap;
 import java.util.List;
-import java.util.Map;
 import java.util.Properties;
 import java.util.stream.Stream;
 
@@ -50,12 +46,6 @@ public class Agent extends AgentBase {
 	/** Stores the XML files. */
 	protected final IUploader uploader;
 
-	/**
-	 * Tracks class IDs (CRC64) by class name across dumps to detect application server reloads. When a known class
-	 * name appears with a different ID, we know the class was reloaded with different bytecode.
-	 */
-	private final Map<String, Long> previousClassIds = new HashMap<>();
-
 	/** Constructor. */
 	public Agent(AgentOptions options, Instrumentation instrumentation)
 			throws IllegalStateException, UploaderException {
@@ -192,8 +182,6 @@ private void dumpReportUnsafe() {
 			return;
 		}
 
-		clearClassDumpDirectoryIfReloadDetected(dump);
-
 		JaCoCoXmlReportGenerator generator = new JaCoCoXmlReportGenerator(
 				options.getClassDirectoriesOrZips(), options.getLocationIncludeFilter(),
 				options.getDuplicateClassFileBehavior(), options.shouldIgnoreUncoveredClasses(), wrap(logger));
@@ -208,40 +196,4 @@ private void dumpReportUnsafe() {
 			logger.error("No coverage was collected. " + e.getMessage(), e);
 		}
 	}
-
-	/**
-	 * Detects application server reloads by comparing class IDs (CRC64) across dumps. When a class that was
-	 * previously seen with one ID now appears with a different ID, it means the class was reloaded with different
-	 * bytecode (for example, an application server reload such as JBoss reload). In that case, the auto-created class
-	 * dump directory is cleared so that stale class files from the old deployment are removed.
-	 *
-	 * <p>This correctly distinguishes reloads from normal multi-classloader setups: in a multi-classloader setup,
-	 * both class IDs are present from the first dump onward, so no ID change is detected on subsequent dumps.
-	 */
-	private void clearClassDumpDirectoryIfReloadDetected(Dump dump) {
-		boolean reloadDetected = false;
-		for (ExecutionData data : dump.getStore().getContents()) {
-			Long previousId = previousClassIds.put(data.getName(), data.getId());
-			if (previousId != null && previousId != data.getId()) {
-				reloadDetected = true;
-			}
-		}
-
-		if (!reloadDetected) {
-			return;
-		}
-
-		logger.info("Detected application server reload: class files were reloaded with different bytecode. "
-				+ "Clearing the class dump directory to remove stale class files from the previous deployment.");
-
-		Path classDumpDir = options.getClassDumpDirectory();
-		if (classDumpDir != null) {
-			FileSystemUtils.deleteRecursively(classDumpDir.toFile());
-			try {
-				Files.createDirectories(classDumpDir);
-			} catch (IOException e) {
-				logger.error("Failed to recreate class dump directory after reload detection", e);
-			}
-		}
-	}
 }

agent/src/main/java/com/teamscale/jacoco/agent/options/AgentOptions.java

@@ -110,12 +110,6 @@ public class AgentOptions {
 	 */
 	/* package */ List<File> classDirectoriesOrZips = new ArrayList<>();
 
-	/**
-	 * The auto-created class dump directory, or null if the user specified class-dir explicitly.
-	 * Used to clear stale class files when an application server reload is detected.
-	 */
-	/* package */ Path classDumpDirectory = null;
-
 	/**
 	 * The logging configuration file.
 	 */
@@ -633,13 +627,6 @@ public List<File> getClassDirectoriesOrZips() {
 		return classDirectoriesOrZips;
 	}
 
-	/**
-	 * @see #classDumpDirectory
-	 */
-	public Path getClassDumpDirectory() {
-		return classDumpDirectory;
-	}
-
 	/** @see #teamscaleServer */
 	public TeamscaleServer getTeamscaleServerOptions() {
 		return teamscaleServer;

agent/src/main/java/com/teamscale/jacoco/agent/options/JacocoAgentOptionsBuilder.java

@@ -32,17 +32,14 @@ public String createJacocoAgentOptions() throws AgentOptionParseException, IOExc
 			builder.append(",excludes=").append(agentOptions.jacocoExcludes);
 		}
 
-		// In EXEC_FILE mode, the agent writes raw execution data directly and doesn't
-		// convert it to a coverage report, so it doesn't need class files for probe-to-source mapping.
-		// The conversion happens externally, where class files are already available.
+		// Don't dump class files in testwise mode when coverage is written to an exec file
 		boolean needsClassFiles = agentOptions.mode == EMode.NORMAL || agentOptions.testwiseCoverageMode != ETestwiseCoverageMode.EXEC_FILE;
 		if (agentOptions.classDirectoriesOrZips.isEmpty() && needsClassFiles) {
 			Path classDumpDirectory = createTemporaryDumpDirectory();
 			classDumpDirectory.toFile().deleteOnExit();
 			builder.append(",classdumpdir=").append(classDumpDirectory.toAbsolutePath());
 
 			agentOptions.classDirectoriesOrZips = Collections.singletonList(classDumpDirectory.toFile());
-			agentOptions.classDumpDirectory = classDumpDirectory;
 		}
 
 		agentOptions.additionalJacocoOptions

report-generator/src/main/kotlin/com/teamscale/report/jacoco/JaCoCoBasedReportGenerator.kt

@@ -74,21 +74,10 @@ abstract class JaCoCoBasedReportGenerator<Visitor : ICoverageVisitor>(
 	 * Analyzes the structure of the class files in [codeDirectoriesOrArchives] and builds an in-memory coverage
 	 * report with the coverage in the given store.
 	 *
-	 * [codeDirectoriesOrArchives] is populated in one of two ways:
-	 * - **Auto-created dump directory**: When the user does not specify `class-dir`, JaCoCo is configured with a
-	 *   `classdumpdir` pointing to a temporary directory (for example,
-	 *   `teamscale-java-profiler-<pid>-<random>/jacoco-class-dump`). JaCoCo writes each instrumented class file there
-	 *   at load time, using the class's internal name as the file path (for example, `com/example/MyClass.class`). If a
-	 *   class is reloaded (for example, after an application server reload), the new version overwrites the old file. This
-	 *   typically results in a single directory entry.
-	 * - **User-specified `class-dir`**: The user points directly at directories or archives (JARs, WARs, EARs)
-	 *   containing their application's class files. This may result in multiple entries, and the same class can appear
-	 *   in several archives, for example, when both an old and a new WAR coexist during an application server reload.
-	 *
 	 * We share a single [EnhancedCoverageVisitor] across all entries so that its duplicate-detection map tracks classes
-	 * globally. Without this, the same class appearing in two different archives (for example, old and new WAR) would
-	 * bypass our duplicate handling and hit `CoverageBuilder` directly, which always throws `IllegalStateException`
-	 * regardless of the configured [duplicateClassFileBehavior].
+	 * globally. Without this, the same class appearing in two different archives (for example, old and new WAR after an
+	 * application server reload) would bypass our duplicate handling and hit `CoverageBuilder` directly, which always
+	 * throws `IllegalStateException` regardless of the configured [duplicateClassFileBehavior].
 	 */
 	@Throws(IOException::class)
 	private fun analyzeStructureAndAnnotateCoverage(store: ExecutionDataStore) {

report-generator/src/test/kotlin/com/teamscale/report/jacoco/JaCoCoXmlReportGeneratorTest.kt

@@ -77,6 +77,38 @@ class JaCoCoXmlReportGeneratorTest : TestDataBase() {
 		}
 	}
 
+	/**
+	 * Ensures that two non-identical, duplicate classes in separate archives (directories) do not cause an exception
+	 * with the default WARN behavior. This tests the scenario where the same class exists in multiple JARs/WARs,
+	 * for example, during an application server reload where both old and new deployments coexist.
+	 */
+	@Test
+	fun testDuplicateClassesAcrossSeparateArchivesShouldNotCrashWithWarn() {
+		try {
+			runGeneratorWithMultipleArchives(
+				"different-duplicate-classes",
+				EDuplicateClassFileBehavior.WARN
+			)
+		} catch (e: EmptyReportException) {
+			// Expected: we don't configure matching class IDs. We only verify no IllegalStateException is thrown.
+		}
+	}
+
+	/**
+	 * Ensures that two non-identical, duplicate classes in separate archives (directories) cause an exception
+	 * when FAIL behavior is configured.
+	 */
+	@Test
+	fun testDuplicateClassesAcrossSeparateArchivesShouldThrowWithFail() {
+		assertThatThrownBy {
+			runGeneratorWithMultipleArchives(
+				"different-duplicate-classes",
+				EDuplicateClassFileBehavior.FAIL
+			)
+		}.isExactlyInstanceOf(IOException::class.java)
+			.hasCauseExactlyInstanceOf(IllegalStateException::class.java)
+	}
+
 	@Test
 	fun testEmptyCoverageFileThrowsException() {
 		val testFolderName = "empty-report-handling"
@@ -166,6 +198,31 @@ class JaCoCoXmlReportGeneratorTest : TestDataBase() {
 		).convertSingleDumpToReport(dump, Paths.get(outputFilePath).toFile())
 	}
 
+	/**
+	 * Runs the report generator with each subdirectory of the test data folder as a separate archive entry.
+	 * This simulates the case where the same class exists in multiple JARs/WARs (for example, after an application
+	 * server reload where both old and new deployments coexist on the classpath).
+	 */
+	@Throws(IOException::class, EmptyReportException::class)
+	private fun runGeneratorWithMultipleArchives(
+		testDataFolder: String,
+		duplicateClassFileBehavior: EDuplicateClassFileBehavior,
+		dump: Dump = createDummyDump()
+	): CoverageFile {
+		val parentFolder = useTestFile(testDataFolder)
+		val subdirectories = parentFolder.listFiles { file -> file.isDirectory }?.toList()
+			?: error("No subdirectories found in $parentFolder")
+		val currentTime = System.currentTimeMillis()
+		val outputFilePath = "test-coverage-multi-archive-$currentTime.xml"
+		return JaCoCoXmlReportGenerator(
+			subdirectories,
+			ClasspathWildcardIncludeFilter(null, null),
+			duplicateClassFileBehavior,
+			false,
+			Mockito.mock()
+		).convertSingleDumpToReport(dump, Paths.get(outputFilePath).toFile())
+	}
+
 	companion object {
 		/**
 		 * Creates a fake dump with the specified class ID. The class ID can currently be calculated with [org.jacoco.core.internal.data.CRC64.classId].

system-tests/reload-detection-test/src/test/kotlin/com/teamscale/systemtest/ReloadDetectionSystemTest.kt

@@ -9,20 +9,22 @@ import org.junit.jupiter.api.Test
 import systemundertest.SystemUnderTest
 
 /**
- * Tests that the agent handles application server reloads correctly:
- * 1. Multiple sequential dumps work without crashing (fresh CoverageBuilder per dump).
- * 2. When a class is reloaded with different bytecode, the agent detects this and clears the class dump directory.
+ * Tests that the agent handles class reloads (for example, JBoss reload) correctly. When a class is reloaded with
+ * different bytecode, JaCoCo assigns a different class ID (CRC64). With a shared report generator, this would cause
+ * an `IllegalStateException: Can't add different class with same name` because the `CoverageBuilder` still holds the
+ * old class ID from a previous dump. The fix is to create a fresh report generator per dump so the `CoverageBuilder`
+ * never carries state across dumps.
  *
  * The reload is simulated by loading a modified copy of the [SystemUnderTest] class through a custom classloader,
- * which causes JaCoCo to create execution data with a different class ID (CRC64) for the same class name.
+ * which causes JaCoCo to create execution data with a different class ID for the same class name.
  */
 class ReloadDetectionSystemTest {
 
 	@Test
-	fun multipleDumpsAndReloadDetection() {
+	fun dumpAfterClassReloadDoesNotCrash() {
 		val teamscaleMockServer = TeamscaleMockServer(SystemTestUtils.TEAMSCALE_PORT).acceptingReportUploads()
 
-		// First dump: generate coverage and establish baseline class IDs
+		// First dump: generate coverage
 		SystemUnderTest().foo()
 		dumpCoverage(SystemTestUtils.AGENT_PORT)
 
@@ -31,8 +33,8 @@ class ReloadDetectionSystemTest {
 		// entry with a different CRC64 for the same class name.
 		loadModifiedSystemUnderTest()
 
-		// Generate more coverage and dump again. The agent should detect the reload
-		// (different class ID for the same name) and handle it without crashing.
+		// Generate more coverage and dump again. Because the agent creates a fresh report generator
+		// (and thus a fresh CoverageBuilder) per dump, the changed class ID does not cause a crash.
 		SystemUnderTest().bar()
 		dumpCoverage(SystemTestUtils.AGENT_PORT)
 
@@ -65,7 +67,6 @@ class ReloadDetectionSystemTest {
 		modifiedBytes[markerIndex + marker.size - 1] = 'x'.code.toByte()
 
 		val classLoader = object : ClassLoader(ClassLoader.getSystemClassLoader()) {
-			/** Defines the modified class bytecode in this class loader, triggering a class reload. */
 			fun defineModifiedClass(): Class<*> {
 				return defineClass("systemundertest.SystemUnderTest", modifiedBytes, 0, modifiedBytes.size)
 			}