Enforce single-thread engine access

[MostCromulent]

@tool4ever this needs some more testing but might be a solution to playing whack-a-mole with network CMEs.

Summary

Network play assumes only one thread touches a game at a time. Nothing enforces that — it holds because threads usually happen to be waiting on each other — and cases that slip through keep turning up, including repeated crashes at the delta-collection site. This makes the assumption a rule the code checks, and reports the places that don't follow it.

The problem

A four-player Commander game over the network crashed twice in one session:

java.util.ConcurrentModificationException
  at forge.trackable.Tracker.getDelayedPropsFor(Tracker.java:98)
  at forge.gamemodes.net.server.DeltaSyncManager.collectDeltas(DeltaSyncManager.java:116)

Both crashes are the same collision. The first hit a card-tap handler and only that action died. The second hit the game loop, which had nothing to catch it, so the thread ended and nobody could take priority for the rest of the game.

The cause is two threads working on the same game at once. The delta walk reads the tracker's queue of delayed property changes while another thread clears it. The other side is a client's yield toggle: it arrives on a new thread, reaches the AI's cost evaluation through tryAutoPassNow, and that runs a speculative freeze bracket ending in clearDelayed().

Why the current code doesn't prevent it

Thread safety is a convention, not something the code enforces. The convention is that a thread doing engine work is either the game loop or a thread the game loop is waiting on, but nothing checks that and nothing stops a second thread going ahead. There's no lock and nothing that can tell you whether another thread is already in the engine.

Two things make it weaker than it looks:

• ThreadUtil.isGameThread() checks whether the thread's name starts with "Game". invokeInBackgroundThread has 67 call sites and only one is engine work — the protocol dispatch. The other 66 are deck loading, downloads, skin loading and similar, and every one of them produces a thread that passes the check, so GameAction.invoke will run engine code inline on it.
• Most client messages are replies to a prompt, so they can't arrive while the game is mid-work. Yield toggles aren't replies, so they can arrive at any time and start engine work on a new thread while the game loop is busy.

What this changes

Each game gets an owner, held in EngineOwner alongside the tracker. A thread claims it before running engine code and releases it whenever it blocks. A waiting thread owns nothing, so one prompt opening inside another still works, and a thread that wakes up takes the lock again before carrying on.

"Engine code" here means whatever runs inside those entry points, not a particular kind of operation. Reads are covered as well as writes, and they have to be: the reported crash is a read of the tracker's queue colliding with a write clearing it, so serialising only writes would leave it intact.

This is opt-in. Sites are wrapped so the threads going through them are ordered against each other; a thread that reaches the game some other way is unaffected and races exactly as it does today. The guarantee is over the threads that take part — which is why the PR also includes reporting since it is the only way to find the ones that don't.

The rule for which sites participate is: take the lock where engine work starts, and release it wherever a thread stops running and waits. Holding while idle is what would block everything else.

Engine work starts in two places today, and this change adds a third:

• GameAction.invoke, which is how the game dispatches its own work, and the only caller of isGameThread()
• the protocol handler's background dispatch, which runs an inbound client message on a fresh thread — this is the path behind the reported crash
• the per-game worker, around each task it runs

It is released around four kinds of blocking call. Three were predictable from reading the code: waiting on an input latch, waiting for a client's reply, and waiting on the UI thread. The fourth, the auto-pass pause, was not — it was found by the reporting described below, after the first enforcing run showed the UI thread waiting on it.

Some threads should not be running engine code at all — the UI thread above all. Rather than doing the work themselves, they hand it to a per-game worker, which waits for the lock, does the work, and releases it.

The calling thread waits for the worker to finish before carrying on. It therefore still waits, but it never executes the work itself, so it can't be the thread that tears the graph and an error there can't take out the interface. Waiting is also what keeps the ordering intact: from the caller's point of view the work happens at exactly the point it always did, just on a different thread, so nothing can overtake it and nothing is dropped. The worker is single-threaded, so work from different callers runs in the order it arrives.

Two things move this way: the delta walk, and the auto-tap evaluation. Both were running on the UI thread.

Why it's safe

• It can't deadlock. A thread that can't get in after three seconds carries on anyway and logs it. That only happens if some blocking call is missing its release, and the result is what the code does today plus a log line saying where.
• The UI thread never runs engine code, and its wait for the worker is capped at three seconds with an inline fallback. Measured with extra instrumentation not included here, over a four-player Commander game that wait was 2.7ms on average and 9.8ms at worst, and 94% of engine work never left the thread it started on.
• Ordering is unchanged, because the caller waits for the worker.
• Offline games pay nothing. The reporting sits behind a check that's only true once a network client is attached, and the AI's simulated games each get their own lock, so they never contend.
• mergeDelayedProps says it's safe because speculative freeze brackets finish before any delta collection happens. That isn't true today, since brackets on different threads can overlap. With the lock it is true.

Reporting

Two things get logged, both aimed at the case where the site list is wrong.

A change made by a thread that never took the lock is counted and reported. The lock can't see those threads, so this is the only place a missed entry point shows up. It sits after the check for whether a network client is attached, so offline games pay nothing for it.

When a thread waits more than 100ms, the stack of whoever is holding the game is logged. Sampling it later is useless because the holder has moved on. This is what found the auto-pass pause.

Why there are three commits

The lock on its own does not work. Each commit exists because the one after it needs it.

Commit 1 — retry the delta walk. The lock has a three-second fallback: a thread that cannot get in carries on without ownership rather than freezing the game. That only makes sense if running without ownership is survivable, and today it is not — the reported crash is what happens when a walk collides during that window, and on the game loop it ends the match. Retrying the walk and shipping what was collected turns that into a dropped packet instead. The fallback contract depends on it.

Commit 2 — wait for the AI evaluation thread. When an evaluation runs over its time limit — or throws, or the caller is interrupted — AiController sets a cancel flag, waits 500ms for the thread to notice, and falls back to Thread.stop() if it is still running. The cancel flag is deliberately tried first, because stopping a thread mid-evaluation can leave the game half-changed. But it is only checked between abilities, so a thread inside a long one does not reach it within 500ms, and stop() is the only thing that would have caught that. On Android, and on Java 20 and later, stop() no longer exists, so nothing catches it and the evaluation keeps running alongside the caller.

With the lock this becomes visible rather than silent: the caller cannot take the game back while the evaluation still holds it, so it waits three seconds and falls back. In a ten-game batch that was 10 fallbacks, 16 evaluations over time, and one game that never finished. Waiting for the ability already running brought all three to zero, and it does what the 500ms was reaching for — the cooperative exit now actually lands, so the stop() path stops mattering.

Commit 3 — the lock, and the auto-pass pause. When a player is auto-passing priority, the game deliberately waits a moment before moving on so the phase change or the spell resolving is visible rather than flashing past. That wait is a Thread.sleep followed by a wait on the UI, and the game thread sits in it doing nothing. Holding the game across that costs nothing today because there is nothing to hold, but with the lock it blocks every other thread for the length of the pause: the first enforcing run had the UI thread waiting up to 1490ms and the fallback firing 8 times. Releasing across the pause brought that to 9.8ms and zero. Unlike the other two this is not a bug on its own — it exists purely as a cost the lock creates, which is why it is here rather than anywhere else.

Both of the first two were invisible before this change. A thread holding a lock that does not exist costs nothing, so neither shows up until something starts enforcing.

Testing completed

• An automated four-player Commander game driven through the human input path, with three clients in separate processes rather than swapped to AI: no fallbacks, no unowned changes, all three clients ended holding the same state as the server
• A desktop network game through the real match screen: no fallbacks, no unowned changes, and one wait over 100ms, during game setup while every card was being built
• Ten-game network batch across 2/3/4 players: 10/10 completed, no checksum mismatches
• Desktop only. Everything here is platform-neutral so mobile gets the same behaviour, but it has not been run there.

[tool4ever]

Thanks

Commit # 1:

Looks like that's what I was after for the time being + sounds like it wourld work on its own so that should be spun-off first

# 2:

I don't see how increasing the extra grace wait time there improves things in a reliable way. If the timeout is reached most likely AI thread has crashed or is stuck in an (almost) infinite loop already. Tbh that whole timeout mess was introduced by another dev against my concerns - it eventually needs a way better implementation anyway. :/

# 3:

This will leave us with one rather complex threading change, something I'll probably only have time for during my next holiday.
I don't like how advanced yield is able to cause this at all though, why would it even be allowed to mutate game state unless that player already has priority? Smells like failure in architecture to me 🤔