fix(module): attribute classes to the module that actually provides them - #5352
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR documents engine testing patterns, improves diagnostics for unattributed event classes, changes module classpath matching to normalized filesystem paths, restores integration-test contexts, and logs discarded writes to unloaded chunks. ChangesEngine diagnostics and test infrastructure
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect module attribution for Java classes discovered during early engine/module integration test startup by ensuring classes are indexed and attributed to the module that actually provides them. It also improves diagnostics and adds coverage/documentation for diagnosing similar attribution failures.
Changes:
- Fix engine module class indexing to index only the specific classpath entry a class originates from (instead of the whole classloader).
- Fix module class ownership checks to compare normalized filesystem
Paths (via code source) rather than URL strings. - Add a regression test for directory modules whose classes are served from a jar, plus improved failure diagnostics and documentation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| engine/src/main/java/org/terasology/engine/core/module/ModuleManager.java | Restricts engine module indexing to the originating classpath entry to avoid “indexed but unowned” classes. |
| engine/src/main/java/org/terasology/engine/core/module/ClasspathCompromisingModuleFactory.java | Switches class ownership checks from URL-equality to normalized filesystem path comparison. |
| engine/src/main/java/org/terasology/engine/core/bootstrap/EntitySystemSetupUtil.java | Improves attribution failure diagnostics for event registration. |
| engine-tests/src/test/java/org/terasology/engine/core/module/ClasspathCompromisingModuleFactoryTest.java | Adds a test covering attribution when a directory module’s classes are loaded from a jar. |
| docs/Engine-Testing-Patterns.md | Documents module attribution failure modes and diagnostic approach for integration tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (Class<? extends Event> type : environment.getSubtypesOf(Event.class)) { | ||
| if (type.getAnnotation(DoNotAutoRegister.class) == null) { | ||
| Name module = verifyNotNull(environment.getModuleProviding(type), | ||
| "Environment has no module for %s", type.getSimpleName()); | ||
| "Environment has no module for %s", unattributedClassReport(type, environment)); | ||
| eventSystem.registerEvent(new ResourceUrn(module.toString(), type.getSimpleName()), type); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/src/main/java/org/terasology/engine/core/bootstrap/EntitySystemSetupUtil.java (1)
154-162: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winBuild the diagnostic report only when event registration fails.
theunattributedClassReport(type, environment)argument toverifyNotNullis evaluated before the call, sogetProtectionDomain()/getCodeSource()and the ordered module-id list are rebuilt for every successful event subtype registration. Use an explicit null check so the expensive report is computed only whengetModuleProviding(type)is actuallynull.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@engine/src/main/java/org/terasology/engine/core/bootstrap/EntitySystemSetupUtil.java` around lines 154 - 162, Update registerEvents to store the result of environment.getModuleProviding(type) and explicitly check it for null before registration. Only when it is null should you call unattributedClassReport(type, environment) and raise the existing failure with the same diagnostic message; otherwise reuse the resolved module for ResourceUrn creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/Engine-Testing-Patterns.md`:
- Around line 277-280: Specify the fenced code block language as text for the
VerifyException example in the EntitySystemSetupUtil.registerEvents
documentation snippet.
- Around line 327-329: Update the classpath equality guidance in
Engine-Testing-Patterns to use java.net.URI for non-blocking comparisons instead
of java.net.URL.equals, while retaining the recommendation to normalize and
compare code sources as Path values rather than comparing URL spellings.
---
Outside diff comments:
In
`@engine/src/main/java/org/terasology/engine/core/bootstrap/EntitySystemSetupUtil.java`:
- Around line 154-162: Update registerEvents to store the result of
environment.getModuleProviding(type) and explicitly check it for null before
registration. Only when it is null should you call unattributedClassReport(type,
environment) and raise the existing failure with the same diagnostic message;
otherwise reuse the resolved module for ResourceUrn creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0531db2-88d6-46aa-a746-a060194e3555
📒 Files selected for processing (5)
docs/Engine-Testing-Patterns.mdengine-tests/src/test/java/org/terasology/engine/core/module/ClasspathCompromisingModuleFactoryTest.javaengine/src/main/java/org/terasology/engine/core/bootstrap/EntitySystemSetupUtil.javaengine/src/main/java/org/terasology/engine/core/module/ClasspathCompromisingModuleFactory.javaengine/src/main/java/org/terasology/engine/core/module/ModuleManager.java
Module integration tests have been failing with `VerifyException: Environment has no module for <X>`, where X is any event class from a module dependency. Two independent defects. The engine module's class index was built with `UrlClassIndex.byClassLoader(aClass.getClassLoader())`. That is the app class loader, so the index claimed every `META-INF/subtypes` entry on the whole classpath — including every module jar — while the engine module's class predicate still (correctly) refused to own those classes. `registerEvents` runs during `StateHeadlessSetup`, against the pre-game environment that holds only `engine` and `unittest`, so a module's event class was discovered there with no module able to claim it. Index only the classpath entry each class came from, which is what the predicate already matches on. This arrived with the Gestalt 8 upgrade (MovingBlocks#5267) and only bites where module jars are on the classpath, which is why it reads as a module-land-only failure. `ClassesInModule` compared `URL`s. A jar has two valid spellings — `file:...jar` as a code source, `jar:file:...jar!/` as a classpath root — and URL equality is exact-string over them, so a class served from a module's own jar never matched its module. Compare normalised paths instead. This has been latent since 8aef6cc (2021), whose verifying test was left `@Disabled` in the same commit; that test is now implemented and passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019pXw6g6JWNGP6eAyDJ1Cf8
`Environment has no module for GetItemTooltip` names neither where the class was loaded from nor which modules were actually asked, and both are invisible at the point of failure. Reconstructing them took most of a debugging session; the message now carries them. Also documents module attribution in Engine-Testing-Patterns, including the pre-game environment that `registerEvents` runs against, and the world generator choice that makes `unittest:empty` unusable for any test that spawns a player. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019pXw6g6JWNGP6eAyDJ1Cf8
`setBlock` returns null when the target chunk is not loaded, which is documented but easy to ignore — and every caller in practice does ignore it. The write silently does not happen, later reads come back as `engine:unloaded`, and the failure surfaces somewhere else entirely with nothing connecting it to the lost write. Found via ItemPipes' integration tests, where a `@BeforeEach` clearing an 11x11x11 region lost 756 writes into chunks that were never loaded. Nothing in the logs mentioned it; the tests instead failed much later on pipe connection counts. Warn at the point of loss, with the position, the block, and the chunk. Reported for the first three positions per chunk — a caller writing a region into missing chunks loses thousands of blocks at a time, so the log has to stay a diagnostic rather than become the failure. Bounded to 256 chunks so a long session cannot accumulate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019pXw6g6JWNGP6eAyDJ1Cf8
Engines.setup() replaces the process-global CoreRegistry context with a wrapper whose parent is whatever was installed before, and teardown never puts the old one back. Across a test class that accumulates a chain of contexts belonging to shut-down engines, and lookups that miss on the newest can still resolve to them. Captured before createHost, because bringing up the host installs its own context. This is hygiene, not a fix for anything observed: it was written to test whether stale contexts explained ItemPipes' chunk generation stalling in every test after the first, and it did not — that failure is unchanged. Keeping it because symmetric teardown of global state is worth having on its own, and the 44 integrationenvironment tests pass with it. Note the module-side MTE (org.terasology.moduletestingenvironment) does not share this shape: it calls CoreRegistry.put(GameEngine.class, host) rather than replacing the context, so it has no chain to accumulate. The two variants have drifted here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019pXw6g6JWNGP6eAyDJ1Cf8
Reporting three positions each from up to 256 chunks is still hundreds of lines describing one bug. Five positions from at most ten chunks says the same thing. Both limits now announce themselves rather than going quiet, so a log that has stopped complaining is not mistaken for a problem that has stopped happening: "Reporting limit reached - further discarded writes are silent. Fix the underlying cause until this message stops appearing." Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019pXw6g6JWNGP6eAyDJ1Cf8
Build the attribution report only on failure. As an argument to verifyNotNull it was evaluated for every event, so a protection-domain lookup and the ordered module-id list ran once per registered event subtype at startup. Reported independently by Copilot and CodeRabbit; both were right. Correct the claim that URL equality is exact-string. It is not — URL.equals compares protocol, host, port and file components, and resolves host names to do it, which also makes it a blocking call. The conclusion still holds, because `file:...jar` and `jar:file:...jar!/` differ in protocol and file, but the stated reason was wrong in both the code comment and the docs. The docs now point at java.net.URI for cases where a URL comparison really is wanted. Also gives the exception example in the docs a language tag. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019pXw6g6JWNGP6eAyDJ1Cf8 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
45ba830 to
304baa5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@engine-tests/src/main/java/org/terasology/engine/integrationenvironment/Engines.java`:
- Around line 132-136: Move the CoreRegistry restoration and contextBeforeSetup
clearing into a finally block surrounding the engine shutdown and cleanup
operations. Ensure CoreRegistry.setContext(contextBeforeSetup) executes and
contextBeforeSetup is cleared even when shutdown() or cleanup() throws, while
preserving the existing cleanup flow.
In
`@engine/src/main/java/org/terasology/engine/core/module/ClasspathCompromisingModuleFactory.java`:
- Around line 190-193: Update the location-resolution logic in
ClasspathCompromisingModuleFactory to catch FileSystemNotFoundException
alongside the existing URI and argument failures, returning null for unavailable
filesystem providers so attribution continues without aborting.
In
`@engine/src/main/java/org/terasology/engine/world/internal/WorldProviderCoreImpl.java`:
- Around line 231-246: Update logDroppedWrite so checking the global
DROPPED_WRITE_CHUNK_REPORT_LIMIT and inserting a new chunk entry in
droppedWritesPerChunk occur atomically, preventing concurrent new keys from
exceeding the limit. Preserve the existing warning announcement and per-chunk
counter behavior for existing entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a0030d0-5f2b-43ac-988e-36578509c4a3
📒 Files selected for processing (7)
docs/Engine-Testing-Patterns.mdengine-tests/src/main/java/org/terasology/engine/integrationenvironment/Engines.javaengine-tests/src/test/java/org/terasology/engine/core/module/ClasspathCompromisingModuleFactoryTest.javaengine/src/main/java/org/terasology/engine/core/bootstrap/EntitySystemSetupUtil.javaengine/src/main/java/org/terasology/engine/core/module/ClasspathCompromisingModuleFactory.javaengine/src/main/java/org/terasology/engine/core/module/ModuleManager.javaengine/src/main/java/org/terasology/engine/world/internal/WorldProviderCoreImpl.java
| Name module = verifyNotNull(environment.getModuleProviding(type), | ||
| "Environment has no module for %s", type.getSimpleName()); | ||
| Name module = environment.getModuleProviding(type); | ||
| if (module == null) { |
There was a problem hiding this comment.
There seem to be a few other critical call sites in the codebase that call ModuleEnvironment#getModuleProviding (or equivalent). Would it make sense to add logging upon failure is those areas as well?
- (three occurrences)
There was a problem hiding this comment.
Agreed these deserve the same treatment. Doing it as a follow-up PR rather than here - this one has grown a couple of times already, and the report helper needed extracting first (now ModuleAttribution.describeUnattributedClass, per your other comment), which turns the follow-up into a mechanical change instead of a refactor bundled with behaviour.
| int seen = dropped.incrementAndGet(); | ||
| if (seen > DROPPED_WRITE_SAMPLES_PER_CHUNK) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is there any point at which dropped is reset? It is possible that the messages might be missed during a longer testing session if it happens briefly mid-test. Perhaps you could reset it after a certain amount of real time has elapsed (for example, 30s)?
I am assuming here that WorldProviderCoreImpl is only instantiated once during game load and is not a transient object.
There was a problem hiding this comment.
Good catch. Your assumption checks out - WorldProviderCoreImpl is constructed in InitialiseWorld/InitialiseRemoteWorld, so it lasts for the whole world load rather than being transient. That makes it worse than the odd missed message: without a reset, one early burst spends the reporting budget for the rest of the session. Added a 30s quiet-period reset, so a later unrelated burst reports afresh.
From @BenjaminAmos: - The attribution report moves out of EntitySystemSetupUtil into ModuleAttribution, alongside the rest of the module machinery. It was never specific to event registration, and the other call sites that want it are not in that class. - `contextBeforeSetup` becomes `coreRegistryBeforeSetup`, so where it came from is obvious at the use site. Noted there that CoreRegistry is deprecated and anything read from it may be stale. - The path-comparison comment loses the URL.equals digression and keeps the point: normalise so a classpath entry and a code source are comparable. - The reporting-limit note is inlined rather than held in a constant. - Discarded-write counters now reset after 30s of quiet. Checking the assumption behind that comment: WorldProviderCoreImpl is built in InitialiseWorld/InitialiseRemoteWorld, so it does last for the whole world load — which means without a reset a single early burst spent the budget for the rest of the session. From CodeRabbit: - Restore the CoreRegistry context in a finally block, so a throwing shutdown cannot leak it into the next test — the exact failure the restore exists to prevent. - Catch FileSystemNotFoundException when resolving a code source, for schemes with no installed provider. Still to come, as a follow-up: the same failure reporting at the other getModuleProviding call sites @BenjaminAmos listed. Extracting the helper here was the prerequisite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019pXw6g6JWNGP6eAyDJ1Cf8 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Some detail on a separate MTE problem this work kept running into, in case it is useful — it is not part of this PR, but it shaped some of the choices here. Chunk generation stops working once a JVM has built enough engines. Turning on Delivery ceases rather than slowing, so it is not a timeout-tuning problem — raising the game-time budget from 30s to 120s changed nothing except how long the failure took to arrive. It is not module-only. Ruled out so far:
Still suspicious, unproven:
Next step when we pick this up is a thread dump of a stalled second engine — whether In the meantime If any of the above is obviously wrong to you, I would rather hear it before we spend more time down the |
|
Replying to my own agent (oh the times we live in): I wonder if #5348 might relate to the chunk mystery - going to have a different workspace investigate. |
|
Cross-checking this from a second machine of ours. The numbers above came from Phoenix, an older and weaker Win10 laptop; everything below is from Dionysus, an aged but considerably beefier Win10 desktop running a 55-module workspace. Four things to add, one of which changes how the 27/7/4 evidence should be read. 1. The counter that produced those numbers is measuring two different regions.
loadedChunks.add(chunk);
if (chunks.isValid()) {
logger.debug("Got chunk {} / {}", loadedChunks.size(), chunks.volume());So "7/27" is 7 arrivals somewhere across a 125-chunk region, not 7 of the 27 that were asked for. And in the other direction, the future could resolve as soon as any 27 padding chunks arrived, before the requested chunks existed at all. #5348 fixes exactly this — an early 2. #5348 makes MTE wait harder, so it may surface more stalls rather than fewer. Anything that passed because the future resolved early on padding now genuinely waits for all 27. Worth expecting when re-running module suites against it. 3. Could not reproduce the stall here today. Full 4. The pipeline deadlock is real, and it produces exactly the "delivery ceases" signature described here. Verified by cherry-picking #5348's diagnostic test (test-only, 181 lines) onto clean
The first three phases match within noise; only the bulk reload differs. Without the fix, chunks blocked on neighbours nobody ever requested sit in So before more A hypothesis I had and then disproved, recorded so nobody re-runs it. #5348 also raises the test JVM from |
Cervator
left a comment
Cervator
left a comment
There was a problem hiding this comment.
Have done more local review and testing including with a rebase, going to merge this as-is just to avoid another full cycle in Jenkins to confirm the already-confirmed.
Module integration tests have been failing for any module whose dependencies carry Java code, with
VerifyException: Environment has no module for <X>thrown fromEntitySystemSetupUtil.registerEventsduringEngines.createHost— before any test body runs.SimpleFarmingwas 7/7 failing,BlockPickeridentically;Health, whose only dependency is code-free, passed throughout.There are two independent defects here, three years apart.
The regression
ModuleManager.loadAndConfigureEngineModulebuilt the engine module's class index with:That is the app class loader, so the index claimed every
META-INF/subtypesentry on the entire classpath — including every module jar. The engine module's class predicate, a few lines further down, still correctly refused to own those classes. Anything falling in that gap gets discovered bygetSubtypesOfand then has no module to attribute it to.It surfaces in module-land specifically because that is where module jars sit on the classpath while
StateHeadlessSetup.initrunsinitEntityAndComponentManagers()— which happens before the game manifest and the handoff toStateLoading, against a pre-game environment containing onlyengineandunittest.Fixed by indexing only the classpath entry each class actually came from, which is what the predicate already matches on.
Introduced by the Gestalt 8 upgrade (#5267).
The latent bug
ClasspathCompromisingModuleFactory.ClassesInModulecomparedURLs. The same jar has two valid spellings —file:...jaras a code source,jar:file:...jar!/as a classpath root — and those differ in both protocol and file component, so a class served from a module's own jar never matched its own module. (URL.equalsis worth avoiding here regardless: it compares components and resolves host names to do it, making it a blocking call.)Fixed by comparing normalised
Paths.This has been latent since 8aef6cc (2021-04-16), whose title is "allow code from either class or jars of module directories" and which left
@Disabled("TODO: need a jar module alongside a classes directory")on the test that would have caught it — in the same commit. That test is now implemented. It builds thebuild/classes+build/libs/foo.jarlayout, loads the class through an isolatedURLClassLoader, and asserts attribution. It fails against the 2021 code, verified by reverting the file and re-running.Fixing the predicate alone does not fix the regression; that was tested in isolation. Both changes are needed.
Diagnostics
Environment has no module for GetItemTooltipnamed neither where the class was loaded from nor which modules were actually asked, and both are invisible at the point of failure. Reconstructing them by hand took most of a debugging session. The message now carries them, on the failure path only.docs/Engine-Testing-Patterns.mdgains a Module Attribution Failures section covering the index-vs-predicate split, the pre-game environment, and the trap that instrumenting the class predicate tells you nothing when the module isn't in the environment — which reads like broken logging rather than a code path that never ran. It also documents whyunittest:emptyis unusable for any test that spawns a player.Also here
Two things found while chasing this, both small and both in the same area:
setBlocksays when it discards a write. It returnsnullwhen the target chunk is not loaded — documented, and universally ignored. The write silently does not happen, later reads come back asengine:unloaded, and the failure surfaces somewhere unrelated. Found via ItemPipes, which was losing 756 writes per test run with nothing in the log about it. Warns at the point of loss, capped at five positions each from ten chunks, and says so when it reaches either cap.MTE restores the
CoreRegistrycontext on teardown.Engines.setup()replaces the process-global context with a wrapper parented on the previous one and never puts it back, so a test class accumulates a chain of contexts belonging to shut-down engines. This is hygiene rather than a fix for anything observed — it was written to test a hypothesis about chunk generation stalling, which it disproved — but symmetric teardown of global state is worth having.Verification
SimpleFarming:integrationTestBlockPicker:integrationTestHealth:integrationTestengine-testsintegrationenvironment.*engine-testscore.module.*Module-side PRs for
SimpleFarmingandItemPipes(both requesting the facet-freeunittest:emptyworld generator) are held until this merges, since their tests cannot pass without it.