TS-45056 Class hot reloading detected and coverage dump fixed #881
TS-45056 Class hot reloading detected and coverage dump fixed #881stahlbauer wants to merge 12 commits intomasterfrom
Conversation
DreierF
left a comment
DreierF
left a comment
There was a problem hiding this comment.
The approach looks good on the surface, but has some serious problems in some cases. I think we either need to more specifically detect the reload for individual jar files or at least ensure that if no duplicates is set, that the default warn does not crash the report generation, but just warn about the issue.
| /** | ||
| * 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<>(); |
There was a problem hiding this comment.
I feel like the agent itself is the wrong level of abstraction at where to track this
| Long previousId = previousClassIds.put(data.getName(), data.getId()); | ||
| if (previousId != null && previousId != data.getId()) { | ||
| reloadDetected = true; |
There was a problem hiding this comment.
This doesn't necessarily detect a reload. I could also just be two conflicting class files, i.e., a third-party dependency that is on the classpath in two different versions.
|
|
||
| Path classDumpDir = options.getClassDumpDirectory(); | ||
| if (classDumpDir != null) { | ||
| FileSystemUtils.deleteRecursively(classDumpDir.toFile()); |
There was a problem hiding this comment.
Classes are only dumped to that directory by jacoco once. Some all class files from other jar files are also gone afterwards and hence will not produce any coverage anymore, but warnings, that the class files are missing.
|
@DreierF Thanks for the review. I guess these are the pitfalls of Agentic Coding. Now a variant where primarily the duplication-detection is kept alive by sharing the visitor instance. |
README.md
Outdated
| │ │ | ||
| └──────────────────────────────────────────────────────────────────────────┘ | ||
| │ | ||
| │ Periodically (default: every 10 min) or on HTTP request |
There was a problem hiding this comment.
I thought the default is 8h 🤔
| // (and thus a fresh CoverageBuilder) per dump, the changed class ID does not cause a crash. | ||
| SystemUnderTest().bar() | ||
| dumpCoverage(SystemTestUtils.AGENT_PORT) | ||
|
|
There was a problem hiding this comment.
Can we also check that the log file does contain the expected warning? (we have some other tests that already check the log file)
| class ReloadDetectionSystemTest { | ||
|
|
||
| @Test | ||
| fun dumpAfterClassReloadDoesNotCrash() { |
There was a problem hiding this comment.
The test also passes on master without any changes to the agent, so it does not really reproduce the issue.
| Path tempDir = createTemporaryDumpDirectory(); | ||
| tempDir.toFile().deleteOnExit(); | ||
| builder.append(",classdumpdir=").append(tempDir.toAbsolutePath()); | ||
| Path classDumpDirectory = createTemporaryDumpDirectory(); |
There was a problem hiding this comment.
Just a variable renaming
| // Created fresh per dump so its CoverageBuilder does not carry class IDs from previous dumps. | ||
| // Without this, application server reloads (for example, JBoss) cause an IllegalStateException | ||
| // because the reloaded class has a different CRC64 than what CoverageBuilder stored earlier. | ||
| JaCoCoXmlReportGenerator generator = new JaCoCoXmlReportGenerator( |
There was a problem hiding this comment.
Core of the bug fix
| * Base class for generating reports based on the binary JaCoCo exec dump files. | ||
| * | ||
| * It takes care of ignoring non-identical classes with the same fully qualified name and classes without coverage. | ||
| * JaCoCo's execution data only contains per-class boolean arrays indicating which probes fired. It does not contain |
There was a problem hiding this comment.
Just better JavaDocs
| │ │ | ||
| └──────────────────────────────────────────────────────────────────────────┘ | ||
| │ | ||
| │ Periodically (default: every 480 min) or on HTTP request |
There was a problem hiding this comment.
Now, the 8h are here. The cause was an inconsistency in CLAUDE.md
|
|
||
| **Coverage modes:** | ||
| - **Interval-based** (`Agent` class) - Periodically dumps coverage (default every 10 min) | ||
| - **Interval-based** (`Agent` class) - Periodically dumps coverage (default every 480 min) |
There was a problem hiding this comment.
This was inconsistent to the implementation
2 participants
Addresses issue TS-45056
Please respect the vote of the Teamscale bot or flag irrelevant findings as tolerated or false positives. If you feel that the Teamscale config needs adjustment, please state so in a comment and discuss this with your reviewer.