server/world: fix Loader deadlocking the transaction goroutine against itself

[schphe]

What happens

Loader.viewChunk can re-enter itself on one goroutine and block on a lock that goroutine already holds. That goroutine is the world's owner, so the world stops permanently while the process stays up: nobody on it receives anything further, inbound packets are not handled, and goroutines elsewhere keep running and logging, which makes it read as a delivery bug rather than a stall.

On how it is triggered, please read the note at the end before this lands. The cycle is demonstrated by the test in this change, which hangs without it. I have not managed to show an ordinary player triggering it.

Why

Loader.viewChunk held the Loader's mutex across both of its call-outs — the Viewer's ViewChunk and World.addViewer.

Showing an entity to a Viewer runs arbitrary code. A session encoding a player's metadata calls Player.Breathing, which reads the liquid at the player's eye, and that position need not be in the chunk being viewed. Reading a block from a chunk that has a background load in flight takes the chunkFromAsyncPool path, where chunkRequest.doImmediate runs the request's callbacks inline on the calling goroutine. The callback a Loader registers in Load is viewChunk, so it re-entered itself and blocked forever on a lock it already held.

sync.(*RWMutex).Lock
world.(*Loader).viewChunk          loader.go:127   <- blocks
world.(*chunkRequest).signal       chunk_request.go:141
world.(*Tx).liquid                 world.go:571
world.showEntity                   world.go:1342
world.(*World).addViewer           world.go:1305
world.(*Loader).viewChunk          loader.go:145   <- lock already held
world.(*World).handleTransactions  world.go:200    <- world owner, dead

Worth knowing: every existing test in this repository sets Config.Synchronous, and a synchronous world never registers a chunk request, so it is immune. That is why nothing caught this.

What changed

The bookkeeping is done under the lock and the lock is released before the Viewer is called. A re-entrant call for the same position returns at the loaded check; one for a different position takes an uncontended lock, so recursion terminates after at most one entry per chunk. This requires recording the chunk as loaded before the Viewer is called rather than after.

addViewer and removeViewer now take the Viewer explicitly instead of reading it off the Loader — that read was previously protected by the caller holding the lock. Releasing the lock also means the Loader may be closed or moved to another World mid-call, which viewChunk now detects, undoing the addition rather than leaking the Viewer into the old world.

Verification

Regression test drives the cycle deterministically through both call-outs. Without the fix both subtests hang and fail with viewChunk did not return: the Loader deadlocked against itself; with it they pass in 0.02s. The test needs a non-synchronous world for exactly the reason above.

What has and has not been shown

The deadlock itself is not in question: the test added here drives the cycle deterministically and hangs on master, and the stack below is from that run.

What I could not show is a player reaching it. I originally believed a teleport on join was enough — a player sent to a spawn point or a lobby, landing in a chunk inside their render distance that is not yet loaded. A harness driving a real client against a real server did not reproduce it that way: the background chunk loading kept up, and the only world freeze it produced came from Loader.ChangeWorld blocking on a full transaction queue, which is a different defect with its own fix.

So the honest position is that this is a real re-entrancy defect in viewChunk with a demonstrated mechanism and no demonstrated player-facing trigger. It may be that the trigger needs a slower generator, a fuller queue or more players than the harness had; it may be that the ordering makes it much rarer than I first thought. I would rather say that plainly than assert a frequency I cannot back.

Worth knowing either way: every existing test in this repository sets Config.Synchronous, and a synchronous world never registers a chunk request, so none of them can reach this path.