fix(world): resolve chunk-processing-pipeline deadlock on unrequested neighbours (#5150) - #5348
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change updates chunk pipeline progress, preserves loaded chunk stores across asynchronous processing, filters region readiness, adds a multi-user chunk reload diagnostic, stabilizes local-provider tests, and increases the test JVM heap limit. ChangesChunk Loading Reliability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ManyUsersChunkLoadTest
participant ChunkRegionFuture
participant LocalChunkProvider
participant ChunkProcessingPipeline
ManyUsersChunkLoadTest->>ChunkRegionFuture: request region readiness
ChunkRegionFuture->>LocalChunkProvider: count in-region relevant chunks
LocalChunkProvider->>ChunkProcessingPipeline: process chunk stages
ChunkProcessingPipeline->>LocalChunkProvider: advance blocked positions after idle polling
LocalChunkProvider-->>ManyUsersChunkLoadTest: report chunk readiness and reload completion
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![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
🤖 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/src/main/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProvider.java`:
- Around line 95-99: Update unloadChunkInternal() so its early-cancellation path
removes the position from both loadedChunkStores and generateQueuedEntities
after stopping processing. Ensure cancellation cleanup occurs before returning,
preventing orphaned ChunkStore entries and stale entity restoration on later
requests.
In
`@engine/src/main/java/org/terasology/engine/world/chunks/pipeline/ChunkProcessingPipeline.java`:
- Around line 51-78: Replace the pipeline-wide consecutiveIdlePolls mechanism in
ChunkProcessingPipeline with per-position blocked-duration tracking for entries
in blockedPositions. Record when each position first becomes blocked, and update
skipBlockedStages() to skip a position once its own blocked time reaches the
configured idle threshold, regardless of completions from unrelated stages;
remove or stop using the global idle counter and clear tracking when positions
unblock or are removed.
🪄 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: 44a974e4-ba53-4991-b753-24f26c52b871
📒 Files selected for processing (6)
build-logic/src/main/kotlin/terasology-metrics.gradle.ktsengine-tests/src/main/java/org/terasology/engine/integrationenvironment/ChunkRegionFuture.javaengine-tests/src/test/java/org/terasology/engine/integrationenvironment/ManyUsersChunkLoadTest.javaengine-tests/src/test/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProviderTest.javaengine/src/main/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProvider.javaengine/src/main/java/org/terasology/engine/world/chunks/pipeline/ChunkProcessingPipeline.java
| // The ChunkStore loaded (from disk) for a chunk in createOrLoadChunk()'s async task, carried | ||
| // through to processReadyChunk() so it isn't read from disk, decompressed, and re-parsed a | ||
| // second time there just to reach restoreEntities() - mirrors how generateQueuedEntities | ||
| // above already carries a freshly *generated* chunk's entities across the same handoff. | ||
| private final Map<Vector3ic, ChunkStore> loadedChunkStores = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
loadedChunkStores isn't cleaned up when in-flight loading is cancelled — leak and possible stale-entity restoration.
dispose()/purgeWorld() now clear loadedChunkStores, but the much more common cancellation path — unloadChunkInternal() calling loadingPipeline.stopProcessingAt(pos) when a position becomes irrelevant while still being asynchronously loaded — never removes the entry. If createOrLoadChunk() already ran loadedChunkStores.put(chunk.getPosition(), chunkStore) (line 148) before cancellation, that entry is now orphaned: chunkProcessingInfoMap no longer tracks the position, so processReadyChunk() (the only other place that removes it) will never run for it.
Two consequences:
- A retained
ChunkStoreleaks for the life of the provider every time a load-in-progress is cancelled before completion (common during normal exploration/view-distance changes). - If that position is later re-requested and this time takes the generate branch (
chunkStore == null),processReadyChunk()will pick up the stale entry vialoadedChunkStores.remove(chunkPos)(line 187), restore entities from the old, unrelated store, and skip processing the freshly-generated entities ingenerateQueuedEntitiesentirely.
generateQueuedEntities has the identical pre-existing gap, so ideally both should be cleaned up together in unloadChunkInternal()'s early-cancel branch:
🐛 Proposed fix
private boolean unloadChunkInternal(Vector3ic pos) {
if (loadingPipeline.isPositionProcessing(pos)) {
// Chunk hasn't been finished or changed, so just drop it.
loadingPipeline.stopProcessingAt(pos);
+ loadedChunkStores.remove(pos);
+ generateQueuedEntities.remove(pos);
return false;
}Also applies to: 148-148, 180-187
🤖 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/world/chunks/localChunkProvider/LocalChunkProvider.java`
around lines 95 - 99, Update unloadChunkInternal() so its early-cancellation
path removes the position from both loadedChunkStores and generateQueuedEntities
after stopping processing. Ensure cancellation cleanup occurs before returning,
preventing orphaned ChunkStore entries and stale entity restoration on later
requests.
| /** | ||
| * How long the reactor waits for a stage to complete before checking whether the pipeline has | ||
| * gone idle. See {@link #skipBlockedStages()}. | ||
| */ | ||
| private static final long POLL_INTERVAL_MS = 5000; | ||
| /** | ||
| * Consecutive idle polls (i.e. {@code POLL_INTERVAL_MS * IDLE_POLLS_BEFORE_SKIP} of pipeline-wide | ||
| * silence) before positions with an unmet requirement are forced past their current stage. | ||
| */ | ||
| private static final int IDLE_POLLS_BEFORE_SKIP = 2; | ||
|
|
||
| private final List<ChunkTaskProvider> stages = Lists.newArrayList(); | ||
| private final Thread reactor; | ||
| private final ChunkExecutorCompletionService chunkProcessor; | ||
| private final ThreadPoolExecutor executor; | ||
| private final Function<Vector3ic, Chunk> chunkProvider; | ||
| private final Map<Vector3ic, ChunkProcessingInfo> chunkProcessingInfoMap = Maps.newConcurrentMap(); | ||
| /** | ||
| * Positions currently waiting on a requirement that isn't available yet. A position lands here | ||
| * whether the requirement is merely late (still being processed, or not yet requested but about | ||
| * to be) or will genuinely never arrive (outside anything ever requested) - those two cases look | ||
| * identical at the moment a requirement is found missing. {@link #skipBlockedStages()} is what | ||
| * tells them apart, by waiting to see whether the whole pipeline stays quiet long enough that | ||
| * "still coming" stops being plausible. | ||
| */ | ||
| private final Set<Vector3ic> blockedPositions = Sets.newConcurrentHashSet(); | ||
| /** Reactor-thread-only; counts consecutive poll timeouts with nothing completing anywhere. */ | ||
| private int consecutiveIdlePolls; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Idle detection is pipeline-wide, not per-position — may never fire under sustained load.
consecutiveIdlePolls resets to 0 whenever any future completes anywhere in the pipeline (line 118), and only increments when chunkProcessor.poll() times out with nothing completing at all (lines 112-117). Under continuous churn — many concurrent users repeatedly triggering loads/reloads (the exact scenario ManyUsersChunkLoadTest exercises) — some task somewhere is very likely to complete within every 5s poll window, so the pipeline may never register IDLE_POLLS_BEFORE_SKIP consecutive idle polls even though a specific boundary chunk has been sitting in blockedPositions the entire time. In that case skipBlockedStages() never runs and the position stalls indefinitely — reproducing the original deadlock precisely under the busy conditions this fix is meant to address.
Consider tracking idleness per blocked position (e.g., a timestamp of when each position was first found blocked) rather than a single pipeline-wide idle counter, so a position can be skipped once it has been stuck long enough, independent of unrelated activity elsewhere.
Also applies to: 110-118, 197-244
🤖 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/world/chunks/pipeline/ChunkProcessingPipeline.java`
around lines 51 - 78, Replace the pipeline-wide consecutiveIdlePolls mechanism
in ChunkProcessingPipeline with per-position blocked-duration tracking for
entries in blockedPositions. Record when each position first becomes blocked,
and update skipBlockedStages() to skip a position once its own blocked time
reaches the configured idle threshold, regardless of completions from unrelated
stages; remove or stop using the global idle counter and clear tracking when
positions unblock or are removed.
2da61b7 to
577eff6
Compare
createOrLoadChunk() already loads a chunk's ChunkStore from disk (gzip + protobuf) inside the async pipeline task to build the Chunk, then discarded it. processReadyChunk() loaded it a second time on the main thread purely to call restoreEntities(), doubling the I/O/decompress/ parse cost for every loaded chunk. Carry the already-loaded ChunkStore from the async task to processReadyChunk() via a new loadedChunkStores map, mirroring the existing generateQueuedEntities handoff used for freshly generated chunks. Refs #5150
Unlike generateQueuedEntities (which is always overwritten by the same branch before it's read again), loadedChunkStores is only written on the "loaded from disk" branch. A position loaded before purgeWorld() that never got consumed (early return in processReadyChunk, or a canceled/interrupted load) leaves a stale entry behind. After purgeWorld() deletes the world, the same position re-enters via the "generate fresh" branch instead - which never touches this map - so the stale ChunkStore survives and gets wrongly matched to the new chunk, resurrecting entities from the deleted world. Remove the entry on the early-return path, and clear both loadedChunkStores and generateQueuedEntities in purgeWorld()/dispose() before any new loads can be scheduled.
Simulates 8 clients + host forcing 8 well-separated ~5x5-chunk regions relevant concurrently, populates persistent entities in each, then bulk-reloads all 200 chunk positions at once - the scenario reported in #5150 as causing multi-second main-thread stalls after loading a saved world. Not a pass/fail regression test; it reports timings via TestReporter for manual comparison across runs. Refs #5150
… unrequested neighbours Root cause: multi-chunk stages (e.g. LightMerger, which needs the full 3x3x3 neighbourhood to merge light) only submit once every required neighbour position has been assigned to that stage or later. At the edge of any loaded region, some of those neighbours were never requested by anyone - not by the chunk cache, not by this pipeline - so they can never be assigned. The chunk waiting on them sat in chunkProcessingInfoMap forever. Widening the loaded region does not fix this: the wider region just gets its own outer shell with the same problem. Confirmed this is not limited to bulk reloads - it also happens during completely normal client-spawn loading - and that it is a genuine structural boundary, not wasted recompute: light data is never persisted to disk (only blockData/extraData round-trip through ChunkSerializer), so a reloaded chunk needs a full light-merge same as a freshly generated one. Fix: track positions blocked on a missing requirement in blockedPositions. The reactor no longer blocks indefinitely on chunkProcessor.take(); it polls with a timeout and counts consecutive empty polls. Only once the whole pipeline has been idle for IDLE_POLLS_BEFORE_SKIP consecutive polls (~10s) does it force every still-blocked position past its current stage via skipStage(), which runs a pass-through task instead of the real stage task. For light merging that means the chunk's outward faces keep whatever lighting the earlier per-chunk stages produced; when a neighbour loads later, that neighbour's own merge propagates across the shared face and corrects it. The idle gate (rather than skipping the instant a requirement is found missing) is necessary because "will never arrive" and "hasn't been requested yet, but will be in a moment" look identical at a single point in time - see ChunkProcessingPipelineTest below. Verified with ManyUsersChunkLoadTest: 200/200 bulk-reloaded chunks across 8 regions now complete in ~14s; before this fix the pipeline deadlocked and never completed. Refs #5150
…requested region onChunkRelevant() counted every relevant chunk towards loadedChunks.size() >= chunks.volume(), regardless of whether that chunk was actually inside the requested region. For a single-chunk request (volume 1, e.g. forceAndWaitForGeneration(new Vector3i())), any other relevant chunk - a padding/neighbour chunk outside the request - could satisfy the count and resolve the future before the actually-requested chunk was in the chunk cache. This was previously masked: before the ChunkProcessingPipeline fix, padding/neighbour chunks near the edge of a region could never complete their own stages, so they could never win this race. Once that fix let those chunks complete, ExampleTest.testWorldProvider started failing - the future resolved on a neighbour, and the actual requested chunk was asserted on before it necessarily existed. Fix: only count a chunk towards completion if it is inside the requested region.
…er completes requestCreatingOrLoadingArea() requests the full 3x3x3 neighbourhood a chunk needs for its own light merge, so more than just the centre chunk can become ready, generate world/block events, or get unloaded in a single update(). These tests implicitly assumed otherwise: - A single chunkProvider.update() call was assumed sufficient to make the requested chunk ready. update() only processes as many ready chunks as fit its per-tick time budget, so with more chunks in flight one call is not always enough. Replaced with updateUntilChunkReady(), which loops update() until the specific chunk under test is ready. - World/block lifecycle events were asserted by fixed list index (.get(0), .get(1)), assuming the requested chunk's event always arrived first. Replaced with filtering by chunk position (for events that carry one) or by existence of a matching event (for block lifecycle events, which carry none). - testUnloadChunkAndDeactivationBlock additionally waited for the first BeforeDeactivateBlocks/BeforeChunkUnload from *any* chunk, which - with the empty RelevanceSystem this test relies on to make every cached chunk unload-eligible - could be a neighbour chunk instead of the requested one. Split into two waits: first for the requested chunk's own BeforeChunkUnload (position-filterable), then for block deactivation from any chunk (not attributable to a position, per issue #3244). These assumptions held only because, before the ChunkProcessingPipeline fix, neighbour chunks near a region's edge could never actually finish processing - so in practice only the centre chunk ever completed, which is exactly what stopped being true once that fix let neighbours complete too.
Running many MTE integration tests together in one Test task JVM (each spins up a full TerasologyEngine, some create multiple) hit Java heap space OOM well before running out of the machine's actual RAM, because tasks.withType<Test> hardcoded -Xmx1024m. Kept at 4096m rather than higher in case CI runners are more heap-constrained than a local dev machine.
577eff6 to
bb93624
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
build-logic/src/main/kotlin/terasology-metrics.gradle.kts (1)
73-73: 🧹 Nitpick | 🔵 TrivialHeap bump applies to every
Testtask, incl. any parallel workers.This applies uniformly to
unitTest,integrationTest, andintegrationTestFlakytasks. If Gradle test workers run in parallel (max-workers/maxParallelForks), each JVM can now claim up to 4GB, so total CI memory demand scales accordingly. Worth confirming CI runners have headroom for the expected parallelism.🤖 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 `@build-logic/src/main/kotlin/terasology-metrics.gradle.kts` at line 73, Review the jvmArgs configuration applied to every Test task in terasology-metrics.gradle.kts, including unitTest, integrationTest, and integrationTestFlaky, and avoid imposing a 4GB maximum heap on each parallel worker. Scope the increased heap only to the task(s) that require it, or use a lower/shared limit consistent with CI runner capacity while preserving required test execution.engine-tests/src/test/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProviderTest.java (1)
147-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated event-filtering assertion pattern across 4 tests.
The "filter captured events by type +
chunkPos, thenfindFirst/assert present" block is repeated near-verbatim intestGenerateSingleChunk,testGenerateSingleChunkWithBlockLifeCycle,testLoadSingleChunk, andtestLoadSingleChunkWithBlockLifecycle. Consider extracting a small generic helper, e.g.assertEventForChunk(List<Event> events, Class<T> type, Vector3ic pos, String message), to reduce duplication and keep future changes to this filtering logic in one place.♻️ Sketch of a shared helper
private static <T extends ChunkEvent> void assertEventForChunk( List<Event> events, Class<T> type, Vector3ic chunkPosition, String message) { boolean present = events.stream() .filter(type::isInstance) .map(type::cast) .anyMatch(e -> e.getChunkPos().equals(chunkPosition)); Assertions.assertTrue(present, message); }Also applies to: 182-202, 238-246, 268-276
🤖 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-tests/src/test/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProviderTest.java` around lines 147 - 167, Extract the repeated event-filtering logic from the affected tests into a generic helper such as assertEventForChunk, accepting captured events, event type, chunk position, and assertion message. Use the helper in testGenerateSingleChunk, testGenerateSingleChunkWithBlockLifeCycle, testLoadSingleChunk, and testLoadSingleChunkWithBlockLifecycle while preserving each test’s existing event types and messages.
🤖 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/src/main/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProvider.java`:
- Around line 177-182: Update the duplicate-ready branch in processReadyChunk so
it also removes the generateQueuedEntities entry for chunkPos before returning,
alongside the existing loadedChunkStores cleanup.
In
`@engine/src/main/java/org/terasology/engine/world/chunks/pipeline/ChunkProcessingPipeline.java`:
- Around line 176-195: Update stopProcessingAt to remove each unloaded or
cancelled position from blockedPositions when its ChunkProcessingInfo is
removed, ensuring stale entries cannot accumulate. Keep processChunkInfo’s
blocked-position tracking unchanged.
---
Nitpick comments:
In `@build-logic/src/main/kotlin/terasology-metrics.gradle.kts`:
- Line 73: Review the jvmArgs configuration applied to every Test task in
terasology-metrics.gradle.kts, including unitTest, integrationTest, and
integrationTestFlaky, and avoid imposing a 4GB maximum heap on each parallel
worker. Scope the increased heap only to the task(s) that require it, or use a
lower/shared limit consistent with CI runner capacity while preserving required
test execution.
In
`@engine-tests/src/test/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProviderTest.java`:
- Around line 147-167: Extract the repeated event-filtering logic from the
affected tests into a generic helper such as assertEventForChunk, accepting
captured events, event type, chunk position, and assertion message. Use the
helper in testGenerateSingleChunk, testGenerateSingleChunkWithBlockLifeCycle,
testLoadSingleChunk, and testLoadSingleChunkWithBlockLifecycle while preserving
each test’s existing event types and messages.
🪄 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: c8a69cab-c9a7-4a2d-9e1b-de55a6ffe50a
📒 Files selected for processing (6)
build-logic/src/main/kotlin/terasology-metrics.gradle.ktsengine-tests/src/main/java/org/terasology/engine/integrationenvironment/ChunkRegionFuture.javaengine-tests/src/test/java/org/terasology/engine/integrationenvironment/ManyUsersChunkLoadTest.javaengine-tests/src/test/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProviderTest.javaengine/src/main/java/org/terasology/engine/world/chunks/localChunkProvider/LocalChunkProvider.javaengine/src/main/java/org/terasology/engine/world/chunks/pipeline/ChunkProcessingPipeline.java
| private void processReadyChunk(final Chunk chunk) { | ||
| Vector3ic chunkPos = chunk.getPosition(); | ||
| if (chunkCache.get(chunkPos) != null) { | ||
| loadedChunkStores.remove(chunkPos); | ||
| return; // TODO move it in pipeline; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Duplicate-ready branch cleans up loadedChunkStores but not generateQueuedEntities.
When chunkCache.get(chunkPos) != null, the code removes the loadedChunkStores entry before returning, but leaves any generateQueuedEntities entry for the same position (populated when createOrLoadChunk took the generate branch) orphaned. It will leak until the next dispose()/purgeWorld() call.
🛠️ Proposed fix
private void processReadyChunk(final Chunk chunk) {
Vector3ic chunkPos = chunk.getPosition();
if (chunkCache.get(chunkPos) != null) {
loadedChunkStores.remove(chunkPos);
+ generateQueuedEntities.remove(chunkPos);
return; // TODO move it in pipeline;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void processReadyChunk(final Chunk chunk) { | |
| Vector3ic chunkPos = chunk.getPosition(); | |
| if (chunkCache.get(chunkPos) != null) { | |
| loadedChunkStores.remove(chunkPos); | |
| return; // TODO move it in pipeline; | |
| } | |
| private void processReadyChunk(final Chunk chunk) { | |
| Vector3ic chunkPos = chunk.getPosition(); | |
| if (chunkCache.get(chunkPos) != null) { | |
| loadedChunkStores.remove(chunkPos); | |
| generateQueuedEntities.remove(chunkPos); | |
| return; // TODO move it in pipeline; | |
| } |
🤖 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/world/chunks/localChunkProvider/LocalChunkProvider.java`
around lines 177 - 182, Update the duplicate-ready branch in processReadyChunk
so it also removes the generateQueuedEntities entry for chunkPos before
returning, alongside the existing loadedChunkStores cleanup.
| boolean blocked = false; | ||
| for (Vector3ic pos : requirements) { | ||
| Chunk chunk = getChunkBy(info.getChunkTaskProvider(), pos); | ||
| if (chunk != null) { | ||
| requiredChunks.add(chunk); | ||
| } else { | ||
| return; | ||
| // Could be being processed but not far enough along yet, could be not requested yet | ||
| // but about to be, or could be a position nothing will ever request. Those look | ||
| // identical here; skipBlockedStages() is what tells them apart. | ||
| blocked = true; | ||
| break; | ||
| } | ||
| } | ||
| if (blocked) { | ||
| blockedPositions.add(info.getPosition()); | ||
| return; | ||
| } | ||
| blockedPositions.remove(info.getPosition()); | ||
| info.setCurrentFuture(runTask(chunkTask, requiredChunks)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
blockedPositions entries aren't removed when a blocked position is unloaded/cancelled.
processChunkInfo adds to blockedPositions (line 190) when a requirement is missing, but the only other place a position leaves chunkProcessingInfoMap mid-flight — stopProcessingAt (lines 342-359, unchanged) — never calls blockedPositions.remove(pos). Until skipBlockedStages() next fires (which clears the entire set unconditionally), a stale position sits in blockedPositions even though its ChunkProcessingInfo is already gone.
Combined with the pipeline-wide idle-detection gap above, a server with continuous chunk churn (players moving through the world, triggering frequent loads/unloads at region edges) may never see skipBlockedStages() fire, so this set can grow without bound for the life of the pipeline.
🛠️ Proposed fix
public void stopProcessingAt(Vector3ic pos) {
ChunkProcessingInfo removed = chunkProcessingInfoMap.remove(pos);
if (removed == null) {
return;
}
+ blockedPositions.remove(pos);
removed.getExternalFuture().cancel(true);🤖 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/world/chunks/pipeline/ChunkProcessingPipeline.java`
around lines 176 - 195, Update stopProcessingAt to remove each unloaded or
cancelled position from blockedPositions when its ChunkProcessingInfo is
removed, ensuring stale entries cannot accumulate. Keep processChunkInfo’s
blocked-position tracking unchanged.
|
Going to test and tinker with this a bit, due to a potential connection made with something else :-) Self-assigned in an attempt to start showing better state that way. |
|
Independent verification on different hardware — Dionysus, an aged Win10 desktop running a 55-module workspace on JDK 17. (For cross-reference: the MTE chunk-stall notes we left on #5352 came from a different and weaker machine of ours, a laptop called Phoenix. Same people, different boxes — the hardware gap turns out to matter below.) To vary the test and the fix separately, I cherry-picked
The first three phases agree within noise, so the runs are comparable right up to the phase of interest, and the bulk reload is the only thing that differs: it deadlocks without the fix and completes with it. That is #5150 reproduced and fixed on a machine other than yours. With the fix, the backlog also drains monotonically ( Worth noting the failure mode is Two notes on
|
|
Again talking to myself via agent reply, but reading that last bit reminds me of past efforts to fan out heavy tests in Jenkins to parallel build agents. Build the initial code and do some basics -> Save the workspace state (could be a quick local Docker one-off build stored in a cluster-local image registry) -> Spin up multiple agents using that image and assign a chunk of a test suite each -> Fan back in when all are done ("diamond" job/parallelism pattern) and continue to write the analytics to storage. It would take some manual tuning over time to not wildly mismatch test suites with dedicated agent time, but on the other hand in Kubernetes with the new node pools with dedicated heavy agent machines that spin down more rapidly rather than get stuck it should be fairly doable and not add a lot of cost. |
|
Following up on my own verification above, because the Jenkins result on this PR (PR-5348 build 3) is more interesting than a plain flaky red, and it partly qualifies what I said. CI is not reaching the scenario this PR fixes. The failure is at The shape looks like a stall, not slowness. Comparing the phases against my Dionysus run on this same branch:
The agent is ~5x slower on the baseline and only ~1.2x slower on connect, so a phase that takes 4.7s here should be nowhere near the 60s real-time Which raises the question I cannot answer from the CI log. With roughly a minute spent in that phase and an idle gate of I could not distinguish those from the build log, and want to be explicit about why rather than guess: Cheap way to settle it: enable Two smaller suggestions. The reload phase — the actual #5150 repro — is gated behind two expensive setup phases, so anything that makes connect or concurrent-regions slow prevents it from ever running. Splitting the reload into its own test (or making the setup cheaper, e.g. fewer users for the reachability path) would let CI verify the thing this PR is for, even on a slow agent. And since this is a diagnostic rather than a pass/fail regression test, it probably wants an explicit generous Worth noting it has now failed consistently for three builds, so this is not flakiness in the usual sense. A permanently red test inside a non-blocking bucket is arguably worse than an unreliable one, since it trains people to stop reading the bucket — which bears on the scheduling question in your description. |
|
ManyUsersChunkLoadTest is not supposed to be run regularly, but on demand. i used it in the hope to make it slow - but it got a deadlock. increasing the time permitted to load considerable exposed the deadlock. |
|
I think we should move such tests to a new "benchmark" testing dir then, just to make it obvious - those are good for isolating local test runs specific to performance, and optionally that sort of weekly dedicated run in Jenkins 👍 That sort of testing categorization has been on my list for ages, but never quite makes it near the top (same with the overly elaborate parallel test running with dedicated agents). I think we might have a few other old benchmark tests scattered about too. |
manySimulatedUsersLoadDifferentAreasConcurrently was tagged "flaky", which puts it in integrationTestFlaky - the bucket Jenkins runs (non-blocking) on every single build. It's been failing there on every recent build (UncheckedTimeoutException at a 30s game-time wait with no override, hit under CI contention that doesn't reproduce locally - a capacity gap, not flakiness), while its own Javadoc already says it's "not a pass/fail regression test - it reports timings for manual comparison". Replaced @tag("flaky") with a new @tag("diagnostic") category, reusable for future tests with the same "manual tool, not a CI gate" nature: - excludeTags(diagnostic) added to test, unitTest, integrationTest, and integrationTestFlaky, so it no longer runs automatically anywhere - new integrationTestOnDemand-style task, integrationTestDiagnostic, mirroring the existing filesystemSideEffectTest opt-in pattern (includeTags(diagnostic), same "task nobody calls automatically" shape) Verified: `integrationTestFlaky --tests "*ManyUsersChunkLoadTest*"` now finds no tests; `integrationTestDiagnostic --tests "*ManyUsersChunkLoadTest*"` runs it standalone (1 test, 0 failures, 1m9s locally). Co-Authored-By: soloturn <soloturn@gmail.com>
… branch Real mistake this session: a local branch named "soloturn" shared history with MovingBlocks/Terasology#5348's actual head branch ("soloturn-performance") but had a different name. `ws push` derives the remote branch name from the local one, so pushing there created a brand-new unrelated branch instead of updating the PR - silently, with no warning anywhere in the flow. `ws review <comp> <cr#>` now compares the locally checked-out branch against the CR's real head branch (via the existing gp_review_head_branch provider function) and warns on mismatch, before the branch-drift check below it - drift against the wrong branch isn't meaningful anyway. Same best-effort posture as the drift check: an unresolvable head branch degrades to silence. Verified against the actual incident: checking out "soloturn" and running `ws review terasology 5348` now warns immediately; checking out the correct "soloturn-performance" stays silent. Co-Authored-By: soloturn <soloturn@gmail.com>
|
Re-validated on Dionysus after rebasing all 8 commits onto current
That is a 4.3x improvement on the bulk reload, landing on roughly the ~14s you reported. It also makes sense mechanically: that phase reloads chunks with persistent entities, and entity deserialization goes through module attribution — exactly what #5352 repaired. So the 62s I measured earlier was carrying that bug, and the numbers in my first comment understate this PR against a fixed baseline. Full-suite time came down with it, 13m30s to 11m10s. Two caveats I would not want read past. These are single runs, so No blocking findings from me. The two small things from my review above still stand as optional (the idle gate being bounded by pipeline silence rather than elapsed time, and the missing null guard on |
Cervator
left a comment
Cervator
left a comment
There was a problem hiding this comment.
Did more local testing and review, including a pre-merge test with a rebase to save us some Jenkins. All seems good and fixes are confirmed 👍
@testinstance(PER_CLASS) was added to dodge a chunk-generation stall: every test method built its own engine, and only the first one in a class could finish generating its world — chunk delivery reached 27/27 for the first and then stopped at 7/27 and 4/27 for the next two. MovingBlocks/Terasology#5348 fixed that at the source: multi-chunk pipeline stages waited on neighbours nobody had requested, so chunks at a region edge sat blocked forever. With it in, all three tests pass without sharing an engine — verified three times. Sharing one engine per class is still a reasonable thing to want, since building one costs about 20 seconds. Removing it here anyway: it was written as a workaround, it says so, and leaving workarounds in place after their cause is fixed is how a codebase stops being able to tell you anything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019pXw6g6JWNGP6eAyDJ1Cf8
3 participants
Summary
Investigates and fixes the chunk-processing-pipeline deadlock behind #5150 (chunks that never finish loading after a bulk reload), plus two pre-existing test bugs the fix exposed, and an unrelated test-infra OOM found while verifying.
Root cause
Multi-chunk pipeline stages (e.g.
LightMerger, which needs the full 3x3x3 neighbourhood to merge light) only submit a chunk once every required neighbour has been assigned to that stage or later. At the edge of any loaded region, some neighbours were never requested by anyone - not in the chunk cache, not in the pipeline - so they can never be assigned, and the chunk waiting on them sat inchunkProcessingInfoMapforever. This isn't limited to bulk reloads (it happens during ordinary client-spawn loading too), and widening the loaded region doesn't help - the wider region just gets its own outer shell with the same problem. It's also not wasted recompute: light data is never persisted to disk, so a reloaded chunk genuinely needs a full light-merge, same as a fresh one.Fix
ChunkProcessingPipelinenow tracks positions blocked on a missing requirement. The reactor thread polls with a timeout instead of blocking indefinitely, and counts consecutive empty polls. Only once the whole pipeline has been idle for ~10s does it force still-blocked positions past their current stage with a pass-through task, instead of the real stage task. For light merging, a skipped chunk keeps whatever lighting its earlier per-chunk stages produced; when a neighbour loads later, that neighbour's own merge corrects the shared face.The idle gate (rather than skipping the instant a requirement is found missing) matters because "will never arrive" and "hasn't been requested yet, but will be in a moment" look identical at a single point in time - see the next section.
Verified with the new
ManyUsersChunkLoadTest: 200/200 bulk-reloaded chunks across 8 regions now complete in ~14s; before this fix, the pipeline deadlocked and never completed.Two pre-existing test bugs this uncovered
Both were masked before this fix, because neighbour chunks near a region's edge could never actually finish processing - so in practice only the requested chunk ever completed, which stopped being true once the fix let neighbours complete too.
ChunkRegionFuture.onChunkRelevant()counted any relevant chunk toward region completion, not just chunks inside the requested region. For a single-chunk request, a neighbour/padding chunk becoming relevant could resolve the future before the actually-requested chunk existed in the chunk cache. Fixed to only count chunks inside the requested region (ExampleTest.testWorldProviderwas failing on this).LocalChunkProviderTest(5 of its test methods) assumed only the requested chunk would ever complete in a givenupdate()call, and asserted on world/block lifecycle events by fixed list index. SincerequestCreatingOrLoadingArea()requests a full 3x3x3 neighbourhood, other chunks can complete, generate events, or unload in the same window. Rewrote assertions to wait for and filter on the specific chunk/position under test instead of assuming ordering.Also included
LocalChunkProvider: avoid loading each chunk'sChunkStorefrom disk twice (once in the async pipeline task, again on the main thread forrestoreEntities()).LocalChunkProvider: clear staleloadedChunkStores/generateQueuedEntitiesentries inpurgeWorld()/dispose(), and on the early-return path inprocessReadyChunk(), so a world-purge can't resurrect entities from a deleted world via a leftover entry.terasology-metrics.gradle.kts). Running many MTE integration tests together (each spins up a fullTerasologyEngine) hitJava heap spacewell before running out of the machine's actual RAM - the limit was just a stale hardcoded value, unrelated to available memory.New diagnostic test, and a new
diagnostictag/task to keep it off CIManyUsersChunkLoadTestsimulates 8 clients + host (9TerasologyEngines total), each forcing a distinct region relevant, then bulk-reloads all 200 chunks at once with persistent entities in them - the scenario #5150 describes. It isn't a pass/fail regression test by design - it reports timings viaTestReporterfor manual comparison.It was originally tagged
flaky, which put it inintegrationTestFlaky- the bucketJenkinsfileruns (non-blocking, viawarnError) on every single build. In practice it's been failing there on every recent CI run:UncheckedTimeoutExceptionfrom the plainhelper.runUntil(ListenableFuture)overload, which has no way to override its hardcoded 30-second game-time budget (a separate mechanism from thesetSafetyTimeoutMs()wall-clock safety net this test does raise, for its later reload-polling loop). Making 8 regions concurrently relevant under CI contention apparently doesn't fit that fixed 30s game-time window, even though it comfortably does locally - a capacity gap, not flakiness, and not something this PR's fix touches.Rather than leave a diagnostic tool failing Jenkins on every build (or just deleting the signal), added a new
diagnostictag/task pair inengine-tests/build.gradle.kts, mirroring the existingfilesystemSideEffectTestopt-in pattern:@Tag("diagnostic")(replacing@Tag("flaky")) onManyUsersChunkLoadTestexcludeTags(diagnostic)added totest,unitTest,integrationTest, andintegrationTestFlaky, so it no longer runs automatically anywhereintegrationTestDiagnostictask (includeTags(diagnostic)) for manual/local runs:gradlew :engine-tests:integrationTestDiagnostic --tests ManyUsersChunkLoadTestThis also answers the CI-scheduling question this PR originally raised about the
flakybucket running on every build - reusable for any future test with the same "manual tool, not a CI gate" nature, without needing a scheduled-job change toJenkinsfile.Test plan
ChunkProcessingPipelineTest(5/5, includingmultiRequirementsChunksWillGeneratedSuccess, which regressed under an earlier immediate-skip version of this fix and drove the idle-gate design)ExampleTest(5/5)LocalChunkProviderTest(5/5)ManyUsersChunkLoadTeststandalone viaintegrationTestDiagnostic --tests "*ManyUsersChunkLoadTest*"(1/1, 0 failures, 1m9s)integrationTestFlaky --tests "*ManyUsersChunkLoadTest*"now finds no matching tests (confirms the exclusion)engine-testsregression sweep (test+integrationTest): 1730 tests, 0 failures, 0 errors