Skip to content

Apply Folia inventory rollbacks on each player's scheduler - #1018

Merged
Intelli merged 4 commits into
PlayPro:masterfrom
tricrotism:for-upstream/folia-inventory-rollback
Oct 2, 2026
Merged

Intelli merged 4 commits into
PlayPro:masterfrom
tricrotism:for-upstream/folia-inventory-rollback

Conversation

@tricrotism

Copy link
Copy Markdown
Contributor

Summary

On Folia, /co rollback ... #inventory changes player inventories from the region thread that owns chunk 0,0, not from the thread that owns the player. This change moves those inventory writes onto each player's entity scheduler and makes the existing per-batch wait cover them. Paper and Spigot keep the inline path unchanged.

The problem

An inventory rollback sets inventoryRollback and gives every row chunk key 0 (Rollback.java:244), so all rows for a world land in one "chunk" at 0,0. On Folia, processFoliaChunks runs each batch through scheduleFoliaChunkBatchTask, which schedules the work with Scheduler.scheduleSyncDelayedTask(..., chunkLocation, 0) (Rollback.java:610). That is the region thread that owns chunk 0,0 in the target world.

RollbackProcessor.processChunk then applies each row inline: player.getEnderChest() and player.getInventory() are modified at RollbackProcessor.java:283-285, and armor slots are re-sorted at line 373. Players are almost never in the region that owns chunk 0,0. Folia requires a player's state to be touched only from the thread that ticks that player, so this write is not allowed on Folia and it races the player's own tick (item pickup, inventory clicks, the player's save).

Scenario: a griefer steals items and logs off, an admin at spawn runs /co rollback u:Griefer t:1h #inventory, and the victim is online 3000 blocks away. The spawn region's thread writes the victim's inventory while the victim's own region is ticking them. The result is undefined: items can go missing, or reappear after the player's own tick overwrites the change.

The fix

Rollback.java

  • processChunkWorld: on Folia it creates a list, passes it to RollbackProcessor.processChunk, and hands every future in it to entitySpawnContext.addPending(...). Upstream's loop already calls awaitChunkTasks(entitySpawnContext.drainPending(), preview) after each batch, so the next batch and the final count read at line 387 wait for the player tasks. On Paper and Spigot it passes null.
  • prepareChunkCounters and completeChunk write rollbackHash with computeIfPresent instead of get then put. The item count is now added from the player's thread while the region thread may still be in these methods for the same batch. With get then put, either side could overwrite the other's update and the admin would see a wrong item total. rollbackHash is a Collections.synchronizedMap, whose computeIfPresent holds the map lock for the whole update.

RollbackProcessor.java

  • processChunk takes a List<CompletableFuture<Boolean>> inventoryTasks. When it is not null and this is an inventory rollback, rows are grouped per player in a LinkedHashMap (row order per player is kept) instead of being applied.
  • After the item loop, one task per player is scheduled with PaperAdapter.ADAPTER.executeEntityTask. The task applies that player's rows in order, re-sorts armor slots like the inline path, adds the item count, then completes the future.
  • The row-apply code moved into applyInventoryRow so both paths share it. It passes null as the container type. That is what the inline path already passed: containerType is only assigned in the container branch, which is skipped for the whole call when inventoryRollback is true.
  • updateRollbackHash uses computeIfPresent, and addRollbackItems is the item-only atomic update the player task uses. Both use upstream's existing key, the user string.

Offline and retired players: the inline path skips a row when getPlayer(uuid) returns null. If the player logs out after grouping, Folia runs the retired callback or execute returns false. Both complete the future with true and add no items, so the player is skipped the same way and the total the admin sees counts only applied items.

Failure: if the player task throws, it reports the error, adds the items applied so far, and completes false. The batch wait then returns false, the loop logs ROLLBACK_ABORTED and cancels, and the sender gets the aborted message. On the inline path, an exception sets status 2, which also aborts.

Alternatives I rejected:

  • Running each batch on the target player's scheduler instead of the chunk's region. A batch can hold rows for many players, so this would need one batch per player and a larger change to the Folia loop.
  • Passing the entity spawn context into RollbackProcessor. A plain list keeps RollbackProcessor free of the context type and leaves the Bukkit path at null.
  • Setting the status-2 abort flag from the player thread. prepareChunkCounters resets that slot before each chunk, so the flag could be lost. The future result is not lost.

Behaviour change

  • Paper and Spigot: none.
  • Folia, inventory rollbacks only: inventory changes happen on the player's own thread, up to one tick after the batch that read them. The next batch starts only after they finish, as with entity spawn tasks today.

Risk

  • Counter writes: every rollbackHash writer that can run during a Folia batch is now atomic. The status check in completeChunk is still a read followed by a write. Only the region and rollback threads set status 2, and the player task never does, so that check is not racing the new code.
  • Late tasks: if a batch fails before its pending futures are awaited, or the 300 second wait in awaitChunkTasks expires, a player task can still run afterwards. This is the same as upstream's entity spawn tasks. The rows were already flagged as rolled back before any world work started, so applying them late matches the database.
  • Row lifetime: the grouped rows hold references to the row arrays, not to the list that processChunk clears, so itemData.clear() does not affect them.
  • Checked: executeEntityTask returns false off Folia and is only called when the list is non-null, which is Folia only.

Testing

Build: mvn package passes.

Row parity: the 47-step scenario (with a block rollback and restore) on Paper 26.2 and Folia 1.21.11 with SQLite matches upstream apart from the random plant that bone meal grows, with no new errors, and the world fingerprints after its rollback and restore steps differ from upstream's only in that same plant and in loose item entities (zombie drops, which are random). The scenario has no #inventory rollback step, so this shows the shared rollback code is unaffected, not the inventory path itself.

Suggested live test on Folia with two players far apart (for example spawn and 3000 blocks out):

  1. Player B picks up and drops known items, moves some to the ender chest, and equips armor. Player B stays online far from spawn.
  2. From spawn, run /co rollback u:B t:5m #inventory, then /co restore u:B t:5m #inventory.
  3. Expect no thread ownership errors in the console, B's inventory and ender chest to match the pre-change state after the rollback, armor back in armor slots, and the item count in the result message to equal the rows applied.
  4. Repeat with B logging out between the command and completion. Expect no error and B to be skipped.
  5. On Paper, repeat step 2 and confirm identical results to upstream.

Inventory rollbacks run in the region that owns chunk 0,0 and changed every player's inventory from there, which Folia does not allow. On Folia the rows are now grouped per player and applied on that player's entity scheduler, the batch waits for those tasks, and the rollback counters are updated atomically. Paper and Spigot are unchanged.
@netlify

netlify Bot commented Sep 23, 2026

Copy link
Copy Markdown

❌ Deploy Preview for coreprotect failed. Why did it fail? →

Name Link
🔨 Latest commit 30beeb5
🔍 Latest deploy log https://app.netlify.com/projects/coreprotect/deploys/6ab3ef83d642ea0008a173bb

@Intelli

Intelli commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Thanks -- automated review is requesting the following changes:

  1. Before merging, please integrate these tasks with the rollback operation’s cancellation and mutation lifecycle. Currently, scheduleInventoryRows can still apply inventory changes after a batch failure or timeout has ended the operation. Its subsequent addRollbackItems(userString, ...) can also update a newer rollback started by the same sender.
  2. Please prevent queued callbacks from beginning mutations after cancellation, and ensure already-started inventory mutations finish before operation cleanup releases the rollback. Counter updates must remain associated with the originating operation. Reusing the existing context’s cancellation and active-mutation mechanism would be appropriate; merely completing or cancelling a future won’t stop its scheduled callback.

The per-player inventory tasks now go through the rollback's entity context. A task checks for cancellation before it changes anything, runs as an active mutation so cleanup waits for it to finish, and adds its item count to that context instead of the shared counter for the sender, so a late task cannot change a rollback that already ended or a newer one from the same sender. The counter helpers go back to their upstream form since player tasks no longer write to them.
@Intelli
Intelli merged commit 48c9bf3 into PlayPro:master Oct 2, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants