Skip to content

Conversation

@UnlimitedBytes
Copy link
Contributor

Summary

  • Fixes "Cannot move entity off-main" error when using /spreadplayers command
  • Uses teleportAsync instead of synchronous teleportTo to properly handle cross-region teleportation

Problem

The /spreadplayers command was causing thread errors because it attempted to teleport entities synchronously to random positions that could be in different regions than the current thread owns. This resulted in moonrise thread check failures during the position update.

Solution

Replaced the synchronous entity.teleportTo() call with entity.getBukkitEntity().teleportAsync(), which schedules the teleport on the entity's task scheduler. This ensures the teleport executes when the entity is being ticked by the appropriate thread that owns both the source and destination regions.

Test plan

  • Build compiles successfully
  • Unit tests pass
  • Manual testing: /spreadplayers ~ ~ 5 1000 false @a executes without errors

@PureGero
Copy link
Contributor

Considering whether SpreadPlayersCommand.Position.isSafe also needs to be made thread safe. Currently it's getting block states in other regions

public boolean isSafe(BlockGetter level, int y) {
    BlockPos blockPos = BlockPos.containing(this.x, this.getSpawnY(level, y) - 1, this.z);
    BlockState blockState = level.getBlockState(blockPos);
    return blockPos.getY() < y && !blockState.liquid() && !blockState.is(BlockTags.FIRE);
}

Use teleportAsync to properly handle teleporting entities to positions
that may be in different regions. This prevents "Cannot move entity
off-main" errors when using /spreadplayers command.
@UnlimitedBytes
Copy link
Contributor Author

I investigated this. isSafe() does have a thread-safety concern since it reads block states from potentially different regions without holding the appropriate locks.

However, we cannot practically synchronize it because:

  • The command runs on a region tick thread that already holds locks for certain regions
  • spreadplayers with a large range checks positions across many regions
  • Blocking to synchronize (via CompletableFuture.join()) causes a deadlock - the current thread holds locks that prevent other region threads from executing the scheduled task

I tested this and confirmed the deadlock - the server hung for 20+ seconds until the watchdog killed it.

Resolution: Accept the minor race condition for isSafe() since it's read-only block access. Worst case is slightly stale data leading to a suboptimal spawn position. This matches vanilla behavior where blocks can change between the safety check and teleportation anyway.

For getSpawnY() in setPlayerPositions(), we can fix it using fire-and-forget scheduling (ShreddedPaper.runSync()) since we don't need a return value - we schedule the Y computation and teleport together on the target region thread.

Options to make this thread-safe would require either:

  1. changes to how commands are dispatched in ShreddedPaper (e.g. moving commands onto their own Executor Pool that doesn't acquired region locks)
  2. changes to the command to make it fully async with callbacks instead of synchronous loops which significantly complicates the algorithm and command response timing

@UnlimitedBytes UnlimitedBytes force-pushed the fix/spreadplayers-off-main-thread branch from 57314e0 to cd1df24 Compare January 10, 2026 14:12
@PureGero
Copy link
Contributor

Many commands are already fully asynchronous such as /setblock, the same would be done to make isSafe thread safe. However, I've decided it's not worth it, I will merge commit 74ab001 as is

@PureGero PureGero closed this Jan 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants