fix: do not discard database writes queued during shutdown

[tastybento]

The bug

Queued writes are drained by an asynchronous repeating task. That task stops once BentoBox is disabled, and store(...) then refuses to run a queued write:

if (async && !plugin.isEnabled()) return;   // silently discarded

Pladdons are disabled by the server before BentoBox, so everything a Pladdon persists from onDisable() is queued into something about to be abandoned. From a real shutdown log:

08:03:10  Disabling BentoBox-InvSwitcher   <- saves here, write is queued
08:03:10  Disabling BentoBox
08:03:10  [BentoBox] Closing database.      <- queue never drained
08:03:10  tastybentotoo left the game       <- PlayerQuitEvent fires after the DB is closed

Whether the async task manages a tick in first is a race, which is why this presents as intermittent data loss with nothing in the log.

Scope

Nine addons save state in onDisable, and all nine are Pladdons: Boxed, AOneBlock, Raft, Challenges, Limits, DragonFights, CheckMeOut, ControlPanel, InvSwitcher. Third-party Pladdons doing the same are affected identically and have no way to know.

Reproduced end to end on a live server with InvSwitcher: pick up items on an island, restart while still connected, and the items are gone — the shutdown save never lands, and onPlayerJoin then re-applies the stale stored inventory over the player's real one, so it is an active rollback rather than merely a missed write.

Changes

Drain on the way out — fixes existing addons with no addon changes at all:

• AbstractDatabaseHandler.flush() runs this handler's outstanding writes on the calling thread; flushAll() does it for every handler. Per-write try/catch so one bad write cannot abandon the rest.
• Handlers are tracked in a weak static registry. getHandler() creates one per Database instance, each with its own queue, and nothing else kept a list of them — there was no way to reach them all. Weak so handlers from a discarded Database stay collectable.
• A volatile boolean draining exempts flushed writes from the disabled-plugin guard in the JSON and SQL handlers. Without it the flush would discard exactly what it exists to save.
• BentoBox.onDisable() calls flushAll() after disableAddons() and before anything closes. That covers Pladdons (already disabled by the server) and ordinary addons (disabled a line earlier), while playersManager/islandsManager shutdown saves that run afterwards already take the synchronous path.

Explicit synchronous save — for callers wanting a guarantee rather than a rescue:

• AbstractDatabaseHandler.saveObjectNow(T) and Database.saveObjectNow(T), overridden in every queueing handler (JSON, SQL, SQLite, PostgreSQL, Yaml, Transition delegating onward). Each shares a private save(instance, forceSync) with saveObject so the paths cannot drift. MongoDB needed no override — already synchronous.
• Non-abstract on the base class, defaulting to saveObject(T), so third-party AbstractDatabaseHandler subclasses still compile.

Deprecates Database.saveObject(T) — the reason this was hard to find. It reads as though the write has completed on return, but delegates to saveObjectAsync and always returns true without waiting for or checking the result. Now points at saveObjectAsync(T) for normal saves, and warns that saveObjectNow(T) is not a drop-in replacement because it blocks — a mechanical migration to it would turn island/player/economy writes into synchronous main-thread writes. The nine internal call sites move to saveObjectAsync, so the deprecation does not arrive pre-suppressed by warnings in our own code.

Verification

Full suite: 3401 tests, 0 failures, 0 errors.

10 new tests in SQLDatabaseHandlerTest. Verified they actually detect the regressions — with both fixes reverted:

RESULT tests=50 failures=4 errors=0
  FAILED: testFlush_writeThrows_continuesAndLogs()
  FAILED: testFlush_writesQueuedSavesAfterPluginDisabled()
  FAILED: testSaveObjectNow_pluginEnabled_storesSynchronouslyAndSkipsQueue()
  FAILED: testSaveObjectNow_sqlException_completesFalse()

Those 4 pin the behaviour; the other 6 pass either way by design (null / not-a-DataObject early returns, the empty-queue no-op, the draining-flag lifecycle, and the plugin-disabled saveObjectNow variant which takes the sync path regardless) and are coverage rather than regression detection.

All CompletableFuture.get() calls in the new tests are bounded with a timeout. An earlier draft used unbounded get(), which meant that under a regression the queued write never completed the future and the test hung instead of failing — it would have stalled CI rather than reporting.

Notes for review

• Worth a look at whether flushAll() should bound how long it will spend draining. It is bounded by queue depth today, which in practice is small, but a pathological queue would extend shutdown.
• The draining exemption deliberately weakens a guard that exists to stop writes after teardown. It is only ever true inside flush(), which runs before any connector is closed, and is cleared in a finally.

🤖 Generated with Claude Code

https://claude.ai/code/session_01YXnYDGiASdSUZFtNyS4jbc

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
42.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud