Fix concurrent corruption of PlayerLoader account cache (#1795)

[claude]

Fixes #1795

The race

PlayerLoader.cachedPlayers was a plain TreeMap mutated from three threads with no synchronisation:

• server main thread, on join — MainListener#onJoin → Fabled#getData → PlayerLoader#getPlayerAccounts
• async Bukkit thread, on quit — Fabled#unloadPlayerData dispatches PlayerLoader#unloadPlayer via runTaskAsynchronously
• Fabled's own MainThread, running SaveTask when auto-save: true

When a put and a remove interleaved, the red-black tree could be corrupted into a cycle. A later lookup then span forever inside TreeMap#getEntry on the main thread, and the server stopped ticking permanently — the watchdog fired at 180s and shutdown deadlocked. The same corruption surfaced earlier as NullPointerException inside TreeMap#fixAfterDeletion on the async thread.

Both mutating paths are compound, not single operations — containsKey → loadPlayer → get on join, and containsKey → get → saveData → remove on quit — and that saveData is a YAML file write sitting between the read and the remove, which is what makes the window wide enough to hit in practice.

The fix

ConcurrentHashMap alone would not be enough: it makes each operation atomic but not the sequences above. And a single synchronized around everything would serialise every save behind one monitor and stall the main thread across disk I/O — trading a hang for a different hang.

So: ConcurrentHashMap plus per-player lock striping. 128 stripes, chosen by UUID, each held only for the duration of that player's load or save. That makes load-then-put and get-save-remove mutually exclusive for the same player — which also stops a fast rejoin from reading a data file that the unload is still writing — while unrelated players keep loading and saving in parallel. Only one stripe is ever held at a time, so there is no lock-ordering deadlock, and no global lock is ever held across I/O. The worst case for a caller that collides on a stripe is waiting out one file write.

computeIfAbsent was deliberately avoided: its mapping function runs while holding a bin lock, and here that function performs disk I/O.

unloadPlayer ordering

Kept as save-then-remove, under the per-player lock. Under a correct lock either order is safe for the cache itself. Remove-then-save would slightly narrow the window in which a bulk save could pick the same player up — but the bulk savers that write files (IOManager#saveAll, ConfigIO#saveAll) only run from Fabled#onDisable, and by then disabling is already true, which makes Fabled#unloadPlayerData short-circuit — so no async unload can be in flight alongside them. Remove-then-save would, on the other hand, turn a failed write into data lost from memory, where save-then-remove leaves the entry cached for a later save pass to retry. Existing durability behaviour preserved.

Second defect, not in the original report

getAllPlayerAccounts() returned the backing map directly, and five call sites iterated it while the async unloader could be mutating it:

• data/io/ConfigIO.java:114
• data/sql/tables/FabledPlayersSQL.java:142
• data/io/IOManager.java:117
• Fabled.java:696
• com/sucy/skill/SkillAPI.java:382 (deprecated)

It now returns an unmodifiable snapshot. All five callers are read-only, so nothing breaks. This also removes a latent NPE in ConfigIO#saveAll, which snapshotted the key set and then re-read each key from the live map — a key unloaded in between yielded a null passed straight into saveData.

PlayerLoader#saveAllPlayerAccounts likewise now iterates a snapshot and takes each player's stripe individually, releasing it before moving to the next, rather than iterating the live map's values().

One behaviour change worth flagging

ConcurrentHashMap rejects null values, and FabledPlayersSQL#loadPlayerAccounts returns null for a player with no known name (FabledPlayersSQL.java:43). A straight type swap would therefore have thrown an NPE from inside the map on SQL setups. Loads that return null are now simply left out of the cache, so callers see the same null they saw before.

Ordering is not relied on anywhere — the audit that no code casts this map to SortedMap/NavigableMap holds, so dropping TreeMap is safe.

What I verified, and what I could not

Could not: run mvn clean install, or the existing test suite. This environment's egress policy returns 403 for every Minecraft-ecosystem Maven repository — repo.papermc.io, hub.spigotmc.org, repo.codemc.io, jitpack.io, mvn.lib.co.nz, repo.md-5.net, maven.enginehub.org, mvn.lumine.io and others. Only Maven Central and repo.travja.dev are reachable, and neither carries paper-api/spigot-api, so not a single project dependency resolves and nothing can be compiled through Maven. I am not claiming a green build. CI on this PR is the real check.

Did verify, by compiling the unmodified PlayerLoader.java and the unmodified new test against minimal stand-ins for the handful of types they touch (OfflinePlayer, Player, Bukkit, Fabled, PlayerAccounts, IOManager), with JUnit 5.13.4 and Mockito 5.19.0 from Central — the same versions the parent POM pins:

• both files compile cleanly under javac -Xlint:all, no warnings;
• the new test passes 10/10 runs against this branch;
• the new test fails 5/5 runs against the pre-fix PlayerLoader, so it is a real regression test rather than a test written to match the new code.

The observed pre-fix failures are exactly the compound-sequence race: getPlayerAccounts returning null for a player it had just seen via containsKey, and unloadPlayer calling saveData with accounts that had already been removed.

The gap in this method is that the stubs stand in for the real classes; that they match the real signatures was checked by reading the source, but only a real build proves it.

The test

src/test/java/studio/magemonkey/fabled/data/io/PlayerLoaderConcurrencyTest.java. No live Bukkit server: Fabled.singleton is replaced reflectively with a mock whose IOManager is a fake that spends measurable time "on disk" and records whether two calls were ever inside the IO layer for the same player at once; players are java.lang.reflect.Proxy instances that only answer getUniqueId().

Twenty threads — 8 loading, 8 unloading, 4 iterating and running full save passes — hammer 16 overlapping UUIDs for 1500 rounds each, asserting no exceptions, that every lookup returns the accounts belonging to the player asked for, that no load and save overlap for a single player, and (via @Timeout) that no thread spins forever. A second, deterministic test covers the snapshot semantics of getAllPlayerAccounts.

Credit

Diagnosis, watchdog stack traces, the live fixAfterDeletion NPE capture, and the ~1364-rejoin reproduction are all @qhashofficial's work in #1795. Their patch was referenced in the report as an attachment but no file or link came through on the issue, so this is an independent implementation of the fix they described.

---

Generated by Claude Code

[qhashofficial]

I'm the one who filed #1795. This is a better fix than the patch I described there, so I'd rather see this go in than mine. Two things it does that mine didn't: the per-player stripes mean no lock is held across the actual file or database write, and getAllPlayerAccounts finally hands out a copy. My version took one global lock around everything. It stopped the hang, but it also made saveAllPlayerAccounts block every player at once.

Three things I noticed reading it.

The clear() at the end of saveAllPlayerAccounts(true) isn't inside any stripe. The saves are, the clear isn't:

for (UUID id : ids) {
    synchronized (lockFor(id)) { ... }
}
if (clearCache) cachedPlayers.clear();

If someone joins after their stripe has already been visited, load() puts them in the map and then clear() wipes them while they're still online. When they quit, unloadPlayer finds nothing cached and skips the save, so it comes out as lost data rather than a stale read. Narrow if clearCache is only ever true on disable, but saveAllPlayerAccounts() is also the periodic path, so it's worth either clearing per stripe inside the loop or writing the constraint down somewhere.

hasPlayerAccounts still reads the map with no lock. That's at odds with the reasoning in the getPlayerAccounts javadoc, which says the lock has to be taken even on a cache hit, because an unlocked read can return an instance that a concurrent unload is about to evict and write out. Both can't be right. If the unlocked read is actually fine, then getPlayerAccounts could keep a fast path instead of taking a stripe on every single data access.

The last one is a question rather than a finding. Does anything under Fabled.getIO() come back into PlayerLoader for a different player? Holding one stripe at a time is only safe if nothing below loadData or saveData grabs a second one. A global lock is reentrant and hides that; striping isn't, so a nested call for another UUID would be two threads taking stripes in opposite order. I haven't read every IOManager implementation. The SQL path is the one I'd look at first.

On numbers, to be clear about what they cover: these are from my own single-lock patch, not from this branch.

build	joins	TreeMap corruptions	outcome
unpatched, spread out	1115	2	survived
unpatched, burst	1364	+2	hang
patched, single global lock	4038	0	fine, TPS 20.0

The repro is fast rejoins across a Velocity network, roughly 1.5 s per hub-to-game cycle, on fresh accounts so loadPlayer takes the creation path. Corruption counts are NullPointerExceptions thrown inside TreeMap itself, and they turn up long before anything actually hangs.

The harness is still set up here. Happy to run it against this branch if that's useful.

[claude]

Thanks — all three are worth having, and one of them was a live bug rather than a nitpick. Pushed as a283560.

1. The clear() outside the stripes — you're right, and it's worse than stale data

Confirmed and fixed. Your reasoning about the failure mode is exactly right: load() puts the joiner in, clear() wipes them while they're online, and because unloadPlayer skips players it finds nothing cached for, the whole session is gone on quit rather than merely stale.

Each entry is now dropped under the same stripe that saved it, instead of a bulk clear() at the end:

for (UUID id : ids) {
    synchronized (lockFor(id)) {
        PlayerAccounts accounts = cachedPlayers.get(id);
        if (accounts != null) {
            Fabled.getIO().saveData(accounts);
            if (clearCache) cachedPlayers.remove(id);
        }
    }
}

So only what was actually saved gets removed, and anything appearing after its stripe was visited just stays cached for the next pass.

One correction to the framing, since it affects how urgent this is: the periodic path is not affected. SaveTask calls saveAllPlayerAccounts(false), and the no-arg overload also passes false. Nothing in the tree passes true at all — I re-checked. So this was latent rather than live. It's public API, so it still needed fixing, but nobody was losing data to it today.

Covered by saveAllWithClear_doesNotDiscardAPlayerThatJoinedDuringThePass, which fails by assertion against the previous commit.

2. hasPlayerAccounts — the asymmetry is intentional, but the javadoc didn't say so

Here I'd push back on "both can't be right", though the doc absolutely invited the reading.

The two aren't the same operation. getPlayerAccounts hands back a reference. An unguarded read there can return an instance that a concurrent unload is about to save and evict — the caller then holds an object that is no longer the cached one, and the next getData loads a second instance from disk. Changes to the first are silently written over. That's the reason it can't keep a fast path.

hasPlayerAccounts returns a boolean. There's nothing to detach. And every caller is doing check-then-act — Fabled#unloadPlayerData checks it, then dispatches an async task that runs long after any stripe would have been released — so guarding the read buys precisely nothing; the race is in the gap, not in the read.

So: no fast path for the reference-returning one, no lock for the boolean one. I've rewritten both javadocs to state which case applies and why, so the asymmetry reads as deliberate rather than as an oversight.

3. The nested-stripe question — this one deadlocks, and I could reproduce it

This was the real find. Two parts to the answer.

Nothing in Fabled does it. The entire data/ package contains no call to Fabled#getData or Fabled#getPlayerAccounts, and the only PlayerLoader method the I/O layer reaches is getAllPlayerAccounts, which takes no stripe. The one Fabled.getData in api/player is PlayerSkillSlot#updateItem, reached only from CastItemListener, and for the same player anyway. You were right to look at the SQL path, but it's clean too — FabledPlayersSQL#loadPlayerAccounts does call saveData on the creation path, but that's the same UUID and saveData takes no stripe of its own.

But the I/O layer isn't a leaf, so third-party code can. Here's the chain:

IOManager.load           (IOManager.java:157)
  -> PlayerData.setClass(null, fabledClass, true)
  -> updatePlayerStat(getPlayer())
  -> callEvent(PlayerMaxManaChangeEvent)   (PlayerData.java:2015)

That fires while the stripe is held. Any addon listening on PlayerMaxManaChangeEvent (or PlayerClassChangeEvent, or PlayerSkillUnlockEvent) that looks up another player's data takes a second stripe. Two threads doing that for two players in opposite orders deadlock — and both hold their first stripe across disk I/O, so the window is wide, not a hairline.

I wrote a test for it and it deadlocks against the previous commit, timing out at 60s rather than failing. So this was not theoretical. You were also right about the mechanism: your global lock was reentrant and absorbed it; striping doesn't.

Fix: a thread never holds more than one stripe. A nested request runs without taking a second one.

if (heldStripe.get() != null) {
    return getOrLoad(player, id);   // nested - do not take a second stripe
}
synchronized (stripe) {
    heldStripe.set(stripe);
    try { return getOrLoad(player, id); }
    finally { heldStripe.remove(); }
}

The trade is explicit: that nested call loses the compound-sequence guarantee. But it's no worse than the behaviour before striping existed, and against a ConcurrentHashMap it cannot corrupt anything — so it's a narrow, non-fatal race in exchange for making lock-ordering deadlock structurally impossible rather than merely unlikely. Covered by reentrantLookupForAnotherPlayer_fromInsideTheIoLayer_doesNotDeadlock.

Verification, same caveat as before

Still no Maven build — this environment's egress policy 403s every Minecraft-ecosystem repo, so no project dependency resolves. Unchanged from the PR description, and CI remains the real check.

What I did run: the four tests, against stand-ins for the six types PlayerLoader touches. 10/10 clean runs on this branch. Against the previous commit, the two new tests fail — one by assertion, one by deadlocking to its timeout — so neither is a test written to match the code it's testing.

On your offer to run the harness

Yes, please — that's the piece I can't produce here, and it's worth more than anything I can assert from a stub harness. Two things it would cover that my tests can't: real TreeMap-scale rejoin volume against real YAML I/O, and whether taking a stripe on every getPlayerAccounts costs anything measurable in TPS under load. That second one is my main open question about this design, since Fabled.getData is called constantly and I've only reasoned about the cost, not measured it.

If you have an addon on that server listening to PlayerMaxManaChangeEvent or PlayerClassChangeEvent, a run with it enabled would be especially interesting — that's exactly the shape that would have hit the deadlock in point 3.

---

Generated by Claude Code

[claude]

One thing I found while working on this and should have noted in the description: there is a second, unrelated data-loss path in the quit sequence that this PR does not fix.

Fabled#unloadPlayerData decides whether to unload a player on the main thread, then dispatches the actual work asynchronously. If the player rejoins in that gap, the join takes the still-cached accounts and the unload then saves and removes that instance, leaving an online player detached from the cache. The per-player locking added here cannot help: the decision predates any lock, and unloadPlayer correctly re-checks the map but never re-checks whether the player should still be unloaded.

Written up separately as #1809 with the two orderings that produce it and two suggested fixes. It is pre-existing, predates #1795, and is narrower than the hang — but it is real, and it is adjacent enough to this change that it should not stay buried in a review thread.

Not proposing to fold it in here. This PR is already doing one thing; that one needs a maintainer decision about what "unload" should mean for a player who has come back.

---

Generated by Claude Code