From de7e13d807b6a99e0bbf3f77b69597b165d60798 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:42:21 +0200 Subject: [PATCH 01/34] Add codebase audit report Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- AUDIT.md | 323 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 323 insertions(+) create mode 100644 AUDIT.md diff --git a/AUDIT.md b/AUDIT.md new file mode 100644 index 00000000..f42ca497 --- /dev/null +++ b/AUDIT.md @@ -0,0 +1,323 @@ +# ParcelLockers โ€” Code Audit + +Audit of `src/` covering duplication, race conditions, optimization opportunities, +and database / unintended-behavior bugs. + +Scope: 134 Java source files. Findings are grouped by severity. Each item lists the +file and the relevant lines so they can be jumped to directly. + +Severity legend: **๐Ÿ”ด Critical** (item/money loss, duplication exploits) ยท +**๐ŸŸ  High** (data-integrity races, broken correctness) ยท **๐ŸŸก Medium** (latent bugs, +inconsistency) ยท **๐Ÿ”ต Low** (style, minor optimization). + +--- + +## ๐Ÿ”ด Critical + +### C1 โ€” Economy fee is charged before persistence and never refunded on failure +`parcel/service/ParcelServiceImpl.java:102` (withdraw) vs `:122-138` (save + rollback) + +`send()` withdraws the fee at line 102, then saves the parcel/content. If +`parcelRepository.save(...)` or content save fails, the `exceptionally`/rollback +branches delete the parcel and notify the player, **but never refund the money**. +The player is charged for a parcel that was never created. + +Fix: only withdraw after a successful save, or `depositPlayer` in every failure path +(content-save failure at `:129`, parcel-save failure at `:135`). + +### C2 โ€” Dispatch rollback loses the fee and orphans parcel content +`parcel/service/ParcelDispatchService.java:68-74` + +After `parcelService.send(...)` succeeds (parcel **and** content persisted, fee +charged), `itemStorageManager.delete(...)` is awaited. If it returns `false`, the code +calls `parcelService.delete(parcel.uuid())` โ€” which deletes only the **parcel row**, not +the `ParcelContent` row saved in `send()`. Result: orphaned content in the DB, the +sender's item-storage still present, and the fee is not refunded. + +Fix: refund on this path and also delete the parcel content; ideally make +"charge โ†’ save parcel โ†’ save content โ†’ clear storage โ†’ schedule" a single rollback unit. + +### C3 โ€” Locker can be duplicated: place is cancelled but the item is never consumed +`locker/controller/LockerPlaceController.java:80, 126-129` + +`onBlockPlace` calls `event.setCancelled(true)` (so vanilla never consumes the item from +the hand), then on dialog confirm it manually re-places the block via +`setType`/`setBlockData`. The locker item is **never removed from the player's +inventory**, so a single locker item creates unlimited lockers. + +Fix: decrement the used item stack by one (main hand or off hand, matching the +`isSimilar` check at `:72`) when the locker is actually created. + +### C4 โ€” Breaking a locker is never cancelled โ†’ chest + contents drop, then block is restored +`locker/controller/LockerBreakController.java:38-78` + +`onBlockBreak` never calls `event.setCancelled(true)`. For a non-admin player the block +is physically broken first (the chest item, and in survival its drops, spawn into the +world), and only on the next tick is the block restored via `setType`/`setBlockData` +(`:53-54`). The already-spawned drops remain โ†’ free chests / item duplication. For the +admin path the block is also left in vanilla-broken state before `delete` runs. + +Fix: cancel the `BlockBreakEvent` for non-admins immediately and return; for admins, +cancel and then run the managed delete + broadcast. + +--- + +## ๐ŸŸ  High + +### H1 โ€” `collect()` deletes the parcel before giving items; inventory check is a TOCTOU +`parcel/service/ParcelServiceImpl.java:171-215` + +`freeSlotsInInventory(player)` is evaluated on the async DB thread (`:191`). The parcel +and content rows are then deleted (`:196-197`), and only afterwards are items handed back +on the main thread, one scheduler task per item (`:204`). Between the slot check and the +delivery the inventory can change, and items are given **after** the only copy in the DB +is destroyed. Any failure (full inventory, server stop, exception in `giveItem`) loses the +items permanently. + +Fix: give items first (and let `ItemUtil.giveItem` drop overflow), then delete only after +delivery is confirmed; re-check free slots on the main thread. + +### H2 โ€” Locker fullness check is a TOCTOU race +`parcel/service/ParcelDispatchService.java:50-61` + `locker/LockerManager.java:176-178` + +`isLockerFull` runs a `COUNT(*)` and `dispatch` proceeds to `send` based on the result. +Two players sending to the same near-full locker concurrently can both pass the check and +both store, exceeding `maxParcelsPerLocker`. There is no atomic guard at the DB level. + +Fix: enforce the cap inside a single transaction, or re-check + compensate after insert. + +### H3 โ€” `LockerManager.get(Page)` returns wrong/incomplete pages from the cache +`locker/LockerManager.java:96-103` + +```java +List cached = List.copyOf(this.lockersByUUID.asMap().values()); +boolean hasNextPage = cached.size() > page.getLimit(); +if (!cached.isEmpty() && page.getOffset() == 0 && !hasNextPage) { + return CompletableFuture.completedFuture(new PageResult<>(cached, false)); +} +``` + +The cache holds an arbitrary, partially-evicted, unordered subset of lockers โ€” it is **not** +the full dataset. When fewer than `limit` lockers happen to be cached, page 0 returns just +those, hiding every locker that isn't currently cached (and in non-deterministic order). +This silently shows users an incomplete locker list. + +Fix: don't serve pagination from a partial cache; query the repository (or maintain a +separate authoritative ordered index). + +### H4 โ€” Locker GUI opens on top of the vanilla chest (cancel happens too late) +`locker/controller/LockerInteractionController.java:41-51` + +The DB lookup is async; `event.setCancelled(true)` is executed inside +`scheduler.run(...)` on a later tick โ€” long after the `PlayerInteractEvent` has finished +and the vanilla chest inventory has already opened. The player gets both the vanilla chest +and the locker GUI, and the cancel is a no-op. + +Fix: resolve locker membership synchronously where possible (e.g. position cache lookup), +cancel the event in the same tick, then open the GUI. + +### H5 โ€” `getOrCreate` / `create` only check the cache for conflicts, not the DB +`user/UserManagerImpl.java:65-117`, and the same pattern in +`locker/LockerManager.java:105-138` + +`getOrCreate` (`:65`) returns from cache or calls `create`. `create` validates "no +conflict" using **cache-only** lookups (`usersByUUID.get(uuid, k -> null)` at `:95-96`), +so an entity that exists in the DB but not in the cache passes validation and a second +"create" is attempted. Combined with `get(...)` not populating the cache on a miss (see +H6), this is reachable in normal operation. + +Fix: consult the repository in the conflict check, or rely on a DB unique constraint and +handle the violation. + +### H6 โ€” `UserManagerImpl.get(...)` never populates the cache on a miss +`user/UserManagerImpl.java:42-62` + +Both `get(UUID)` and `get(String)` return straight from the repository on a cache miss +without putting the result into `usersByUUID` / `usersByName`. Every lookup for an +uncached user is a DB round-trip, and the cache only ever fills via `create`/`changeName`. +This also feeds H5. (Contrast with `LockerManager.get`, `DeliveryManager.get`, +`ItemStorageManager.get`, which all cache on read.) + +Fix: cache the fetched user (by UUID and name) before returning. + +--- + +## ๐ŸŸก Medium + +### M1 โ€” `ParcelSendTask` updates status and deletes the delivery as independent fire-and-forget calls +`parcel/task/ParcelSendTask.java:54-64` + +The "mark DELIVERED" update (`:54`) and the "delete delivery" call (`:60`) are not +chained. If the update fails but the delete succeeds, the parcel is stuck in `SENT` +forever with no delivery row, so it is never re-scheduled on restart +(`ParcelLockers.java:215-228` only reschedules parcels that still have a delivery) and +never becomes collectable. + +Fix: chain โ€” delete the delivery only after the status update completes successfully. + +### M2 โ€” Async-only Bukkit events are fired from mixed threads (latent `IllegalStateException`) +`itemstorage/ItemStorageManager.java:51-66`, `user/UserManagerImpl.java:87-117,120-143`, +`locker/LockerManager.java:105-138` + +`ItemStorageUpdateEvent`, `UserCreateEvent`, `UserChangeNameEvent`, `LockerCreateEvent`, +`ParcelSendEvent`, `ParcelDeliverEvent` are all constructed with `super(true)` +(asynchronous), while `ParcelCollectEvent`, `LockerDeleteEvent`, `UserChangeNameEvent`'s +counterparts use the default sync constructor. Paper throws if an async event is fired +from the primary thread (or a sync event from an async thread). Whether each `callEvent` +is legal currently depends entirely on the calling thread happening to match the flag โ€” +e.g. `ItemStorageManager.getOrCreate` (`:47`) runs its loader synchronously on the caller +thread, and if ever invoked on the main thread it will throw when firing the async +`ItemStorageUpdateEvent`. + +Fix: pick one threading model per event deliberately, and document/guarantee the firing +thread; don't let it depend on the call path. + +### M3 โ€” `ItemStorageManager.getOrCreate` mutates the cache from inside a cache-loader for the same key +`itemstorage/ItemStorageManager.java:47-66` + +`getOrCreate` calls `cache.get(owner, key -> this.create(key, items))`, and `create` +itself calls `this.cache.put(owner, ...)` (`:63`) for the **same key** that is currently +being loaded. Recursive computation/mutation of the same key inside a Caffeine mapping +function is explicitly discouraged and can throw `IllegalStateException` / corrupt the +entry. + +Fix: have the loader return the value and let `cache.get` store it; don't `put` inside the +loader. + +### M4 โ€” Item-storage save/delete failures are silently swallowed (possible item loss) +`gui/implementation/locker/ItemStorageGui.java:106-117`, `gui/GuiManager.java:90-92`, +`itemstorage/ItemStorageManager.java:64` + +On GUI close, items are read from the inventory, the storage row is deleted, then +`saveItemStorage` is called โ€” but `create()`'s `itemStorageRepository.save(...)` return is +ignored (fire-and-forget at `ItemStorageManager.java:64`), and `saveItemStorage` returns +`void`. If the save fails after the delete succeeded, the player's staged items are gone +with no error surfaced. + +Fix: chain delete โ†’ save, propagate failure, and re-give items on failure. + +### M5 โ€” `ItemStorageRepositoryOrmLite` swallows table-creation failure +`itemstorage/repository/ItemStorageRepositoryOrmLite.java:19-23` + +```java +} catch (SQLException ex) { + ex.printStackTrace(); +} +``` + +Every other repository (`ParcelRepositoryOrmLite:34`, `LockerRepositoryOrmLite:26`, +`ParcelContentRepositoryOrmLite:21`) throws `DatabaseException` on a failed +`createTableIfNotExists`. Here it prints and continues, so a missing table only surfaces +later as confusing query errors. + +Fix: throw `DatabaseException` for consistency and fail-fast startup. + +### M6 โ€” Confusing repository `save`/`update` semantics +`parcel/repository/ParcelRepositoryOrmLite.java:40-49` + +`database/wrapper/AbstractRepositoryOrmLite.java:27-33` + +`save(Parcel)` maps to `saveIfNotExist` โ†’ `dao.createIfNotExists` (a no-op if the row +exists), while `update(Parcel)` maps to the base `save` โ†’ `dao.createOrUpdate`. The +method named `save` cannot persist changes to an existing parcel, and the wrapper method +named `save` actually performs an upsert. This inversion is an easy source of "my update +didn't persist" bugs. + +Fix: rename wrapper methods to `upsert`/`insertIfAbsent` and align the repository method +names with their real behavior. + +### M7 โ€” `DatabaseManager.getDao` check-then-put is not atomic +`database/DatabaseManager.java:99-112` + +`cachedDao.get` followed by `cachedDao.put` on a `ConcurrentHashMap` can run twice +concurrently and create/replace the DAO twice. Harmless today (ORMLite caches DAOs +internally) but it defeats the point of the cache. + +Fix: `cachedDao.computeIfAbsent(type, t -> DaoManager.createDao(...))`. + +### M8 โ€” Aggressive/duplicated HikariCP tuning +`database/DatabaseManager.java:47-49` + +`leakDetectionThreshold` (5000 ms) equals `connectionTimeout` (5000 ms), and the pool is +hard-capped at 5 connections regardless of DB type or server size. Any legitimately slow +query will log a false "connection leak" warning. None of these are configurable. + +Fix: raise/relax the leak threshold, make pool size configurable, and consider a larger +default for networked DBs. + +--- + +## ๐Ÿ”ต Low / Optimization + +- **L1 โ€” Redundant enum round-trip.** `database/DatabaseManager.java:53` + `DatabaseType.valueOf(databaseType.toString().toUpperCase())` โ€” `databaseType` is already + a `DatabaseType`; switch on it directly. +- **L2 โ€” Redundant condition.** `locker/LockerManager.java:178` + `count > 0 && count >= maxParcelsPerLocker` โ€” the first clause is redundant unless + `maxParcelsPerLocker <= 0`; clarify the intended zero/negative behavior. +- **L3 โ€” `onDisable` ordering.** `ParcelLockers.java:231-244` closes the datasource before + unregistering anything; in-flight async DB tasks (scheduler pool) will then fail. Consider + draining/awaiting pending writes before `disconnect()`. +- **L4 โ€” Stale "TODO"/placeholder comments.** `locker/controller/LockerPlaceController.java:92` + ("Replace with actual config message" โ€” but the config value *is* used) and hardcoded + English dialog strings throughout `LockerPlaceController` and `SendingGui` + (`:95-160`) bypass `MessageConfig`, contradicting the "all user-facing text goes through + MessageConfig" architecture note. +- **L5 โ€” `freeSlotsInInventory` ignores stacking.** `util/InventoryUtil.java:12-20` counts + only fully empty slots, so `collect()` can wrongly reject a parcel whose items would stack + into partially-filled slots (or accept-then-drop). Account for partial stacks if precise. +- **L6 โ€” `CollectionGui` removes the item from the GUI before collection is confirmed.** + `gui/implementation/locker/CollectionGui.java:142-145` calls `collectParcel` (async) and + `refresher.removeItemBySlot` immediately; on a failed collect the UI desyncs from reality. + (The double-collect dupe is prevented by the `delete`-returns-false guard in `collect`, but + the visual desync remains.) + +--- + +## Duplication + +The codebase is generally well-layered, but several patterns are copy-pasted across every +domain and are worth extracting: + +1. **Repository table bootstrap.** The `try { TableUtils.createTableIfNotExists(...) } + catch { throw new DatabaseException(...) }` block is duplicated in every `*OrmLite` + constructor (parcel, locker, content, itemstorage, delivery, user). Pull into + `AbstractRepositoryOrmLite` as a protected helper (and fix M5 in the process). + +2. **Pagination logic.** `ParcelRepositoryOrmLite.findByPaged` (`:108-128`) and + `LockerRepositoryOrmLite.findPage` (`:61-78`) implement the identical "limit+1, remove + last, compute hasNext" pattern (and `UserRepository.fetchPage` likely a third copy). + Extract a shared paged-query helper. + +3. **Manager cache-or-fetch.** The "check cache โ†’ on miss fetch from repo โ†’ populate cache" + block is reimplemented in `LockerManager`, `DeliveryManager`, `ItemStorageManager`, + `UserManagerImpl`, and `ParcelServiceImpl` โ€” with subtle inconsistencies (UserManager + forgets to populate; H6). A small generic cached-loader would unify behavior. + +4. **Bukkit event boilerplate.** Every event class (`ParcelSendEvent`, `ParcelCollectEvent`, + `LockerCreateEvent`, โ€ฆ) repeats the `HandlerList` / `getHandlers` / `isCancelled` / + `setCancelled` block. A shared abstract `CancellableEvent` base removes ~30 lines each and + would make the async-flag decision (M2) explicit in one place. + +5. **`createActiveItem` overloads.** `SendingGui.java:419-437` has two near-identical + methods differing only in `add` vs `addAll`; collapse to one taking a `List`. + +--- + +## Summary + +| Area | Count | Highest severity | +|---|---|---| +| Item/money loss & duplication | C1โ€“C4 | ๐Ÿ”ด Critical | +| Concurrency / data-integrity races | H1โ€“H6, M1, M3 | ๐ŸŸ  High | +| Correctness & consistency | M2, M4โ€“M8 | ๐ŸŸก Medium | +| Style / minor optimization | L1โ€“L6 | ๐Ÿ”ต Low | +| Duplication | 5 patterns | โ€” | + +The most urgent work is the parcel-send/collect money-and-item flows (C1, C2, H1) and the +two block-controller exploits (C3, C4), all of which are reachable in normal gameplay and +cause real loss or duplication. The async-event threading model (M2) is a latent crash that +should be made deliberate rather than incidental. + +> Note: this is a static review; none of the findings were reproduced at runtime. Validate +> the critical items (especially C3/C4 duplication and C1/C2 refunds) with targeted tests +> before and after fixing. \ No newline at end of file From 62c9e4e8b65397ab8b5849c245029ee785eecbc9 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:43:35 +0200 Subject: [PATCH 02/34] Fix C1: refund parcel fee when persistence fails The economy fee was withdrawn before the parcel was saved and never refunded if the save failed, charging the player for a parcel that was never created. Track the charged amount and deposit it back in the failure path. Also consolidate the rollback so the fee is refunded and the failure notice is sent exactly once. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcel/service/ParcelServiceImpl.java | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java index 8db7f323..dca3eb27 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java @@ -91,6 +91,7 @@ public CompletableFuture send(Player sender, Parcel parcel, List this.config.settings.smallParcelFee; @@ -111,6 +112,7 @@ public CompletableFuture send(Player sender, Parcel parcel, List messages.parcel.feeWithdrawn) .player(sender.getUniqueId()) @@ -119,6 +121,7 @@ public CompletableFuture send(Player sender, Parcel parcel, List this.parcelContentRepository.save(new ParcelContent(parcel.uuid(), itemsCopy)) .thenApply(contentSaved -> { @@ -126,18 +129,23 @@ public CompletableFuture send(Player sender, Parcel parcel, List messages.parcel.sent); return true; }) - .exceptionallyCompose(contentError -> { - this.noticeService.player(sender.getUniqueId(), messages -> messages.parcel.cannotSend); - return this.parcelRepository.delete(parcel.uuid()) - .thenCompose(deleted -> CompletableFuture.failedFuture(new ParcelOperationException("Failed to save parcel content, rolled back parcel", contentError))); - }) + .exceptionallyCompose(contentError -> this.parcelRepository.delete(parcel.uuid()) + .thenCompose(deleted -> CompletableFuture.failedFuture(new ParcelOperationException("Failed to save parcel content, rolled back parcel", contentError)))) ) .exceptionally(throwable -> { + // Persistence failed after the fee was withdrawn - refund it so the player is not charged for a parcel that was never created. + this.refundFee(sender, refundableFee); this.noticeService.player(sender.getUniqueId(), messages -> messages.parcel.cannotSend); throw new ParcelOperationException("Failed to save parcel", throwable); }); } + private void refundFee(Player sender, double fee) { + if (fee > 0) { + this.economy.depositPlayer(sender, fee); + } + } + @Override public CompletableFuture update(Parcel updated) { Objects.requireNonNull(updated, "Updated parcel cannot be null"); From 5ab5f9815891a8eb2ab89625d735b0b0cc0434d9 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:45:16 +0200 Subject: [PATCH 03/34] Fix C2: fully roll back a parcel when storage clearing fails When itemStorageManager.delete failed after the parcel and its content had been persisted (and the fee charged), the dispatch path deleted only the parcel row - leaving the ParcelContent orphaned and the fee lost. Add ParcelService#rollbackSend which deletes the parcel and its content and refunds the fee, and call it from ParcelDispatchService. Extract a shared feeFor(size) helper so the charge and refund cannot diverge. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcel/service/ParcelDispatchService.java | 9 ++++-- .../parcel/service/ParcelService.java | 6 ++++ .../parcel/service/ParcelServiceImpl.java | 30 +++++++++++++++---- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java index 2a4ab9b1..e9d066dd 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java @@ -66,11 +66,13 @@ public void dispatch(Player sender, Parcel parcel, List items) { } return this.itemStorageManager.delete(sender.getUniqueId()) - .thenAccept(deleted -> { + .thenCompose(deleted -> { if (!Boolean.TRUE.equals(deleted)) { - this.parcelService.delete(parcel.uuid()); + // The parcel and its content were already persisted and the fee charged, + // but the sender's staged storage could not be cleared. Fully roll back + // (parcel + content + fee) instead of leaving orphaned content behind. this.noticeService.player(sender.getUniqueId(), messages -> messages.parcel.cannotSend); - return; + return this.parcelService.rollbackSend(sender, parcel); } this.deliveryManager.create(parcel.uuid(), Instant.now().plus(delay)); @@ -82,6 +84,7 @@ public void dispatch(Player sender, Parcel parcel, List items) { ); this.scheduler.runLaterAsync(task, delay); + return CompletableFuture.completedFuture(null); }); }); }) diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelService.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelService.java index f17a4f25..9d58fd50 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelService.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelService.java @@ -16,6 +16,12 @@ public interface ParcelService { CompletableFuture send(Player sender, Parcel parcel, List items); + /** + * Rolls back a parcel that was successfully persisted by {@link #send} but could not be + * fully dispatched. Deletes both the parcel and its content and refunds the send fee. + */ + CompletableFuture rollbackSend(Player sender, Parcel parcel); + CompletableFuture update(Parcel parcel); CompletableFuture collect(Player player, Parcel parcel); diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java index dca3eb27..4b379a6a 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java @@ -9,6 +9,7 @@ import com.eternalcode.parcellockers.content.repository.ParcelContentRepository; import com.eternalcode.parcellockers.notification.NoticeService; import com.eternalcode.parcellockers.parcel.Parcel; +import com.eternalcode.parcellockers.parcel.ParcelSize; import com.eternalcode.parcellockers.parcel.event.ParcelCollectEvent; import com.eternalcode.parcellockers.parcel.event.ParcelSendEvent; import com.eternalcode.parcellockers.parcel.repository.ParcelRepository; @@ -93,11 +94,7 @@ public CompletableFuture send(Player sender, Parcel parcel, List this.config.settings.smallParcelFee; - case MEDIUM -> this.config.settings.mediumParcelFee; - case LARGE -> this.config.settings.largeParcelFee; - }; + double fee = this.feeFor(parcel.size()); if (fee > 0) { boolean success = this.economy.withdrawPlayer(sender, fee).transactionSuccess(); @@ -140,6 +137,29 @@ public CompletableFuture send(Player sender, Parcel parcel, List rollbackSend(Player sender, Parcel parcel) { + Objects.requireNonNull(sender, "Sender cannot be null"); + Objects.requireNonNull(parcel, "Parcel cannot be null"); + + if (!sender.hasPermission(PARCEL_FEE_BYPASS_PERMISSION)) { + this.refundFee(sender, this.feeFor(parcel.size())); + } + this.parcelsByUuid.invalidate(parcel.uuid()); + + return this.parcelRepository.delete(parcel.uuid()) + .thenCompose(deleted -> this.parcelContentRepository.delete(parcel.uuid())) + .thenApply(contentDeleted -> null); + } + + private double feeFor(ParcelSize size) { + return switch (size) { + case SMALL -> this.config.settings.smallParcelFee; + case MEDIUM -> this.config.settings.mediumParcelFee; + case LARGE -> this.config.settings.largeParcelFee; + }; + } + private void refundFee(Player sender, double fee) { if (fee > 0) { this.economy.depositPlayer(sender, fee); From 3e6df1463cf33920863dfe1cca67535430cfffeb Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:46:09 +0200 Subject: [PATCH 04/34] Fix C3: consume the locker item when a locker is created onBlockPlace cancels the placement (so vanilla never consumes the item) and re-places the block manually, but never removed the locker item from the player's inventory - letting one item create unlimited lockers. Consume one matching item from the main/off hand on the main thread when the locker is actually created. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../controller/LockerPlaceController.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java index 21f1206b..7fd083e6 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java @@ -130,6 +130,11 @@ public void onBlockPlace(BlockPlaceEvent event) { this.lockerManager.create(UUID.randomUUID(), description, PositionAdapter.convert(location), player.getUniqueId()) .thenAccept(locker -> { + // The original BlockPlaceEvent was cancelled (so the item was never + // consumed) and the block was placed manually. Consume one locker item + // now so a single item cannot create unlimited lockers. + this.scheduler.run(() -> this.consumeLockerItem(player, parcelLockerItem)); + this.noticeService.create() .player(player.getUniqueId()) .notice(messages -> messages.locker.created) @@ -167,4 +172,19 @@ public void onBlockPlace(BlockPlaceEvent event) { player.showDialog(dialog); } + + private void consumeLockerItem(Player player, ItemStack lockerItem) { + PlayerInventory inventory = player.getInventory(); + + ItemStack mainHand = inventory.getItemInMainHand(); + if (lockerItem.isSimilar(mainHand)) { + mainHand.setAmount(mainHand.getAmount() - 1); + return; + } + + ItemStack offHand = inventory.getItemInOffHand(); + if (lockerItem.isSimilar(offHand)) { + offHand.setAmount(offHand.getAmount() - 1); + } + } } From 364c22325a5ae7687dfc6b2a2d39c1a8e252919d Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:48:21 +0200 Subject: [PATCH 05/34] Fix C4: cancel locker break to prevent chest/contents duplication onBlockBreak never cancelled the BlockBreakEvent, so a non-admin break physically destroyed the block (spawning the chest and its drops) before the block was restored on the next tick - a duplication exploit. Add LockerManager#getCached for a synchronous, cache-only lookup and use it to cancel the break within the same tick when the locker is cached. Admin breaks now clear the block without dropping it. The async fallback (uncached locker) still restores the block as before. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/locker/LockerManager.java | 9 +++ .../controller/LockerBreakController.java | 75 +++++++++++-------- 2 files changed, 53 insertions(+), 31 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java b/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java index b3fe0803..500dfd6f 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java @@ -77,6 +77,15 @@ public CompletableFuture> get(UUID uniqueId) { }); } + /** + * Synchronous, cache-only lookup. Lets event handlers that must run on the main thread + * (e.g. block protection) decide whether to cancel an event within the same tick instead + * of cancelling asynchronously after the event has already been processed. + */ + public Optional getCached(Position position) { + return Optional.ofNullable(this.lockersByPosition.getIfPresent(position)); + } + public CompletableFuture> get(Position position) { Locker locker = this.lockersByPosition.getIfPresent(position); diff --git a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerBreakController.java b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerBreakController.java index 03180901..cc3bfa95 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerBreakController.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerBreakController.java @@ -2,10 +2,12 @@ import com.eternalcode.commons.scheduler.Scheduler; import com.eternalcode.multification.shared.Formatter; +import com.eternalcode.parcellockers.locker.Locker; import com.eternalcode.parcellockers.locker.LockerManager; import com.eternalcode.parcellockers.notification.NoticeService; import com.eternalcode.parcellockers.shared.Position; import com.eternalcode.parcellockers.shared.PositionAdapter; +import java.util.Optional; import org.bukkit.Location; import org.bukkit.Material; import org.bukkit.block.Block; @@ -42,39 +44,50 @@ public void onBlockBreak(BlockBreakEvent event) { Position position = PositionAdapter.convert(location); Player player = event.getPlayer(); - this.lockerManager.get(position).thenAccept((locker) -> { - if (locker.isEmpty()) { - return; - } + // Fast path: the locker is cached, so we can decide synchronously and cancel the break + // before vanilla destroys the block and spawns its drops. + Optional cached = this.lockerManager.getCached(position); + if (cached.isPresent()) { + event.setCancelled(true); + this.applyBreakRules(player, position, cached.get(), block); + return; + } + // Slow path: the locker is not cached, so the break has already been processed by the + // time the async lookup completes. Restore the block to avoid losing the locker. + this.lockerManager.get(position).thenAccept(locker -> locker.ifPresent(value -> this.scheduler.run(() -> { - if (!player.hasPermission("parcellockers.admin.break")) { - // We assume that the event was already processed and the block is gone, - // so we need to restore it manually - location.getBlock().setType(block.getType()); - location.getBlock().setBlockData(blockData); - this.noticeService.player(player.getUniqueId(), messages -> messages.locker.cannotBreak); - return; - } - - this.lockerManager.delete(locker.get().uuid(), player.getUniqueId()); - - this.noticeService.player(player.getUniqueId(), messages -> messages.locker.deleted); - - Formatter formatter = new Formatter() - .register("{X}", position.x()) - .register("{Y}", position.y()) - .register("{Z}", position.z()) - .register("{WORLD}", position.world()) - .register("{PLAYER}", player.getName()); - - this.noticeService.create() - .onlinePlayers() - .notice(messages -> messages.locker.broadcastRemoved) - .formatter(formatter) - .send(); - }); - }); + location.getBlock().setType(block.getType()); + location.getBlock().setBlockData(blockData); + this.applyBreakRules(player, position, value, location.getBlock()); + }))); + } + + private void applyBreakRules(Player player, Position position, Locker locker, Block block) { + if (!player.hasPermission("parcellockers.admin.break")) { + this.noticeService.player(player.getUniqueId(), messages -> messages.locker.cannotBreak); + return; + } + + // Admin removal: delete the managed locker and clear the block without dropping it. + block.setType(Material.AIR); + + this.lockerManager.delete(locker.uuid(), player.getUniqueId()); + + this.noticeService.player(player.getUniqueId(), messages -> messages.locker.deleted); + + Formatter formatter = new Formatter() + .register("{X}", position.x()) + .register("{Y}", position.y()) + .register("{Z}", position.z()) + .register("{WORLD}", position.world()) + .register("{PLAYER}", player.getName()); + + this.noticeService.create() + .onlinePlayers() + .notice(messages -> messages.locker.broadcastRemoved) + .formatter(formatter) + .send(); } @EventHandler From 9107ab5867b7a28897ad368181fc86d3db7324e5 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:50:03 +0200 Subject: [PATCH 06/34] Fix H1: make parcel collection space-check and delivery race-safe freeSlotsInInventory was evaluated on the async DB thread and the parcel was deleted before items were handed back on the main thread, so a full or changing inventory could lose items permanently. Re-check inventory space on the main thread, delete the parcel/content first (so it cannot be collected twice), and only then give the items - guarded by the delete result so a failed delete never destroys items. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcel/service/ParcelServiceImpl.java | 46 ++++++++++++------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java index 4b379a6a..3d12b84f 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java @@ -216,29 +216,43 @@ public CompletableFuture collect(Player player, Parcel parcel) { } List items = optional.get().items(); - if (items.size() > freeSlotsInInventory(player)) { - this.noticeService.player(player.getUniqueId(), messages -> messages.parcel.noInventorySpace); - return CompletableFuture.completedFuture(null); - } + CompletableFuture result = new CompletableFuture<>(); + + // Re-check inventory space on the main thread (the previous async check was a TOCTOU), + // then delete the parcel BEFORE handing the items back so it cannot be collected twice. + // Items are only given once the delete is confirmed, so a failed delete never destroys them. + this.scheduler.run(() -> { + if (items.size() > freeSlotsInInventory(player)) { + this.noticeService.player(player.getUniqueId(), messages -> messages.parcel.noInventorySpace); + result.complete(null); + return; + } - return this.parcelRepository.delete(parcel) - .thenCompose(deleted -> this.parcelContentRepository.delete(parcel.uuid()) - .thenAccept(contentDeleted -> { - if (!deleted || !contentDeleted) { + this.parcelRepository.delete(parcel) + .thenCompose(deleted -> this.parcelContentRepository.delete(parcel.uuid()) + .thenApply(contentDeleted -> deleted && contentDeleted)) + .thenAccept(removed -> { + if (!removed) { this.noticeService.player(player.getUniqueId(), messages -> messages.parcel.databaseError); + result.complete(null); return; } - items.forEach(item -> this.scheduler.run(() -> ItemUtil.giveItem(player, item))); - this.parcelsByUuid.invalidate(parcel.uuid()); - this.noticeService.player(player.getUniqueId(), messages -> messages.parcel.collected); + this.scheduler.run(() -> { + items.forEach(item -> ItemUtil.giveItem(player, item)); + this.noticeService.player(player.getUniqueId(), messages -> messages.parcel.collected); + }); + result.complete(null); }) - ) - .exceptionally(throwable -> { - this.noticeService.player(player.getUniqueId(), messages -> messages.parcel.cannotCollect); - return null; - }); + .exceptionally(throwable -> { + this.noticeService.player(player.getUniqueId(), messages -> messages.parcel.cannotCollect); + result.complete(null); + return null; + }); + }); + + return result; }); } From 11fff9eeee99bdb5e2a7dd8eae736fe3973919e6 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:53:56 +0200 Subject: [PATCH 07/34] Fix H2: close the locker-fullness TOCTOU race Two concurrent sends to the same near-full locker could both pass isLockerFull and both store, exceeding maxParcelsPerLocker. Count all parcels occupying a destination locker (collected parcels are already deleted, so in-transit + delivered = the real occupancy), which reserves a slot at send time, and serialize dispatches per destination locker via a per-locker future chain so the check and the save cannot interleave. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/locker/LockerManager.java | 4 ++-- .../parcel/repository/ParcelRepository.java | 7 +++++- .../repository/ParcelRepositoryOrmLite.java | 6 +---- .../parcel/service/ParcelDispatchService.java | 22 ++++++++++++++++++- 4 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java b/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java index 500dfd6f..2f01f190 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java @@ -183,8 +183,8 @@ public CompletableFuture deleteAll(CommandSender sender, NoticeService not } public CompletableFuture isLockerFull(UUID uniqueId) { - return this.parcelRepository.countDeliveredParcelsByDestinationLocker(uniqueId) - .thenApply(count -> count > 0 && count >= this.config.settings.maxParcelsPerLocker); + return this.parcelRepository.countParcelsByDestinationLocker(uniqueId) + .thenApply(count -> count >= this.config.settings.maxParcelsPerLocker); } private CompletableFuture deleteLocker(UUID uniqueId) { diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepository.java b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepository.java index 1997ace4..b738281a 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepository.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepository.java @@ -29,7 +29,12 @@ public interface ParcelRepository { CompletableFuture> findByReceiver(UUID receiver, Page page); - CompletableFuture countDeliveredParcelsByDestinationLocker(UUID destinationLocker); + /** + * Counts the parcels currently occupying a destination locker. Collected parcels are removed + * from storage, so every parcel addressed to the locker (in-transit or delivered) occupies a + * slot. Counting in-transit parcels reserves a slot at send time and closes the fullness race. + */ + CompletableFuture countParcelsByDestinationLocker(UUID destinationLocker); CompletableFuture delete(Parcel parcel); diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java index 52d944b0..d177f604 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java @@ -4,7 +4,6 @@ import com.eternalcode.parcellockers.database.DatabaseManager; import com.eternalcode.parcellockers.database.wrapper.AbstractRepositoryOrmLite; import com.eternalcode.parcellockers.parcel.Parcel; -import com.eternalcode.parcellockers.parcel.ParcelStatus; import com.eternalcode.parcellockers.shared.Page; import com.eternalcode.parcellockers.shared.PageResult; import com.eternalcode.parcellockers.shared.exception.DatabaseException; @@ -24,7 +23,6 @@ public class ParcelRepositoryOrmLite extends AbstractRepositoryOrmLite implement private static final String RECEIVER_COLUMN = "receiver"; private static final String SENDER_COLUMN = "sender"; private static final String DESTINATION_LOCKER_COLUMN = "destination_locker"; - private static final String STATUS_COLUMN = "status"; public ParcelRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { super(databaseManager, scheduler); @@ -92,14 +90,12 @@ public CompletableFuture> findByReceiver(UUID receiver, Page } @Override - public CompletableFuture countDeliveredParcelsByDestinationLocker(UUID destinationLocker) { + public CompletableFuture countParcelsByDestinationLocker(UUID destinationLocker) { Objects.requireNonNull(destinationLocker, "Destination locker UUID cannot be null"); return this.action(ParcelTable.class, dao -> { long count = dao.queryBuilder() .where() .eq(DESTINATION_LOCKER_COLUMN, destinationLocker) - .and() - .eq(STATUS_COLUMN, ParcelStatus.DELIVERED) .countOf(); return (int) count; }); diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java index e9d066dd..dd5984bf 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java @@ -11,7 +11,9 @@ import java.time.Duration; import java.time.Instant; import java.util.List; +import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Logger; import org.bukkit.entity.Player; import org.bukkit.inventory.ItemStack; @@ -28,6 +30,10 @@ public class ParcelDispatchService { private final PluginConfig config; private final NoticeService noticeService; + // Serializes dispatches per destination locker so that two concurrent sends cannot both pass + // the fullness check before either parcel is persisted (a TOCTOU that could exceed the cap). + private final ConcurrentHashMap> lockerChains = new ConcurrentHashMap<>(); + public ParcelDispatchService( LockerManager lockerManager, ParcelService parcelService, @@ -47,7 +53,21 @@ public ParcelDispatchService( } public void dispatch(Player sender, Parcel parcel, List items) { - this.lockerManager.isLockerFull(parcel.destinationLocker()) + UUID lockerId = parcel.destinationLocker(); + + CompletableFuture chained = this.lockerChains.compute(lockerId, (id, previous) -> { + CompletableFuture predecessor = previous == null + ? CompletableFuture.completedFuture(null) + : previous.exceptionally(throwable -> null); + return predecessor.thenCompose(ignored -> this.dispatchInternal(sender, parcel, items)); + }); + + // Drop the chain entry once it drains so the map does not grow unbounded. + chained.whenComplete((result, throwable) -> this.lockerChains.remove(lockerId, chained)); + } + + private CompletableFuture dispatchInternal(Player sender, Parcel parcel, List items) { + return this.lockerManager.isLockerFull(parcel.destinationLocker()) .thenCompose(isFull -> { if (isFull) { this.noticeService.player(sender.getUniqueId(), messages -> messages.parcel.lockerFull); From ea93d72a4869ba1f396ce116937385a9d77ae769 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:54:47 +0200 Subject: [PATCH 08/34] Fix H3: page lockers from the repository, not the partial cache LockerManager.get(Page) returned an arbitrary, partially-evicted subset of the cache as page 0, hiding any locker that was not currently cached and ordering results non-deterministically. Always query the repository and warm the cache with the results. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/locker/LockerManager.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java b/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java index 2f01f190..370629e1 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java @@ -15,7 +15,6 @@ import com.github.benmanes.caffeine.cache.Caffeine; import eu.okaeri.configs.exception.ValidationException; import java.time.Duration; -import java.util.List; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; @@ -103,12 +102,16 @@ public CompletableFuture> get(Position position) { } public CompletableFuture> get(Page page) { - List cached = List.copyOf(this.lockersByUUID.asMap().values()); - boolean hasNextPage = cached.size() > page.getLimit(); - if (!cached.isEmpty() && page.getOffset() == 0 && !hasNextPage) { - return CompletableFuture.completedFuture(new PageResult<>(cached, false)); - } - return this.lockerRepository.findPage(page); + // The cache holds an arbitrary, partially-evicted subset of lockers - it is not the full + // dataset, so it cannot answer pagination. Always query the repository (warming the cache + // with the results for subsequent single-locker lookups). + return this.lockerRepository.findPage(page).thenApply(result -> { + result.items().forEach(locker -> { + this.lockersByUUID.put(locker.uuid(), locker); + this.lockersByPosition.put(locker.position(), locker); + }); + return result; + }); } public CompletableFuture create(UUID uniqueId, String name, Position position, UUID playerUUID) { From 113e608977d0bb93ab828b1bd8829c521a9900ec Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:55:48 +0200 Subject: [PATCH 09/34] Fix H4: cancel chest interaction in-tick so the vanilla chest stays shut The locker lookup was async and event.setCancelled ran a tick later, so the vanilla chest had already opened underneath the locker GUI. Use the synchronous locker cache to cancel the interaction in the same tick; on a cache miss, close the just-opened inventory before showing the GUI. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../LockerInteractionController.java | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerInteractionController.java b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerInteractionController.java index 9ecfe24b..e843febc 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerInteractionController.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerInteractionController.java @@ -2,8 +2,11 @@ import com.eternalcode.commons.scheduler.Scheduler; import com.eternalcode.parcellockers.gui.implementation.locker.LockerGui; +import com.eternalcode.parcellockers.locker.Locker; import com.eternalcode.parcellockers.locker.LockerManager; +import com.eternalcode.parcellockers.shared.Position; import com.eternalcode.parcellockers.shared.PositionAdapter; +import java.util.Optional; import java.util.UUID; import org.bukkit.Material; import org.bukkit.block.Block; @@ -38,16 +41,23 @@ public void onInventoryOpen(PlayerInteractEvent event) { return; } - this.lockerManager.get(PositionAdapter.convert(block.getLocation())).thenAccept(optionalLocker -> { - if (optionalLocker.isEmpty()) { - return; - } - UUID uuid = optionalLocker.get().uuid(); + Position position = PositionAdapter.convert(block.getLocation()); + // Fast path: cancel the interaction synchronously so the vanilla chest never opens. + Optional cached = this.lockerManager.getCached(position); + if (cached.isPresent()) { + event.setCancelled(true); + UUID uuid = cached.get().uuid(); + this.scheduler.run(() -> this.lockerGUI.show(player, uuid)); + return; + } + + // Slow path: the locker is not cached, so the vanilla chest has already opened by the time + // the async lookup completes. Close it and open the locker GUI instead (warming the cache). + this.lockerManager.get(position).thenAccept(optionalLocker -> optionalLocker.ifPresent(locker -> this.scheduler.run(() -> { - event.setCancelled(true); - this.lockerGUI.show(player, uuid); - }); - }); + player.closeInventory(); + this.lockerGUI.show(player, locker.uuid()); + }))); } } From 6a9723c48c00a4b985dedbe351acff00a79a0a31 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:57:05 +0200 Subject: [PATCH 10/34] Fix H6: populate the user cache on read misses UserManagerImpl.get(UUID)/get(String) returned straight from the repository without caching, so every lookup of an uncached user hit the database and the cache only ever filled via create/changeName. Cache the fetched user (by UUID and name) like the other managers do. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/user/UserManagerImpl.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java index 2ffd77bb..a1b2e867 100644 --- a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java @@ -47,7 +47,10 @@ public CompletableFuture> get(UUID uniqueId) { return CompletableFuture.completedFuture(Optional.of(user)); } - return this.userRepository.fetch(uniqueId); + return this.userRepository.fetch(uniqueId).thenApply(optional -> { + optional.ifPresent(this::cache); + return optional; + }); } @Override @@ -58,7 +61,15 @@ public CompletableFuture> get(String username) { return CompletableFuture.completedFuture(Optional.of(user)); } - return this.userRepository.fetch(username); + return this.userRepository.fetch(username).thenApply(optional -> { + optional.ifPresent(this::cache); + return optional; + }); + } + + private void cache(User user) { + this.usersByUUID.put(user.uuid(), user); + this.usersByName.put(user.name(), user); } @Override From 9355bf3783a89668fdecccb6b270f57bbf68afe2 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:58:11 +0200 Subject: [PATCH 11/34] Fix H5: detect user conflicts against the database, not just the cache getOrCreate and create only checked the in-memory cache for conflicts, so a user present in the database but evicted/absent from the cache passed validation and a duplicate create was attempted. getOrCreate now fetches from the repository before creating, and create validates UUID/name conflicts against repository lookups. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/user/UserManagerImpl.java | 55 +++++++++++-------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java index a1b2e867..4a1c7756 100644 --- a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java @@ -86,7 +86,16 @@ public CompletableFuture getOrCreate(UUID uuid, String name) { return CompletableFuture.completedFuture(userByName); } - return this.create(uuid, name); + // Not cached - consult the repository before creating, otherwise a user that exists in the + // database but not the cache would be created a second time. + return this.userRepository.fetch(uuid).thenCompose(optional -> { + if (optional.isPresent()) { + User user = optional.get(); + this.cache(user); + return CompletableFuture.completedFuture(user); + } + return this.create(uuid, name); + }); } @Override @@ -96,35 +105,35 @@ public CompletableFuture> getPage(Page page) { @Override public CompletableFuture create(UUID uuid, String name) { - return CompletableFuture.supplyAsync(() -> { - ValidationResult validation = this.validationService.validateCreateParameters(uuid, name); + ValidationResult validation = this.validationService.validateCreateParameters(uuid, name); - if (!validation.isValid()) { - throw new ValidationException("Invalid user parameters: " + validation.errorMessage()); - } + if (!validation.isValid()) { + return CompletableFuture.failedFuture( + new ValidationException("Invalid user parameters: " + validation.errorMessage())); + } - Optional existingByUUID = Optional.ofNullable(this.usersByUUID.get(uuid, uniqueId -> null)); - Optional existingByName = Optional.ofNullable(this.usersByName.get(name, username -> null)); + // Check for conflicts against the database, not only the cache, so a name/UUID already + // persisted but not currently cached is still detected. + return this.userRepository.fetch(uuid).thenCombine(this.userRepository.fetch(name), + (existingByUUID, existingByName) -> { + ValidationResult conflictCheck = this.validationService.validateNoConflicts( + uuid, name, existingByUUID, existingByName); - ValidationResult conflictCheck = this.validationService.validateNoConflicts( - uuid, name, existingByUUID, existingByName); + if (!conflictCheck.isValid()) { + throw new ValidationException(conflictCheck.errorMessage()); + } - if (!conflictCheck.isValid()) { - throw new ValidationException(conflictCheck.errorMessage()); - } + User user = new User(uuid, name); - User user = new User(uuid, name); - - // Fire UserCreateEvent - UserCreateEvent event = new UserCreateEvent(user); - this.server.getPluginManager().callEvent(event); + // Fire UserCreateEvent + UserCreateEvent event = new UserCreateEvent(user); + this.server.getPluginManager().callEvent(event); - this.usersByUUID.put(uuid, user); - this.usersByName.put(name, user); - this.userRepository.save(user); + this.cache(user); + this.userRepository.save(user); - return user; - }); + return user; + }); } @Override From dd6289c575ea1700fb2d8a34e98effb9f0baea7d Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:58:52 +0200 Subject: [PATCH 12/34] Fix M1: delete the delivery only after the status update succeeds ParcelSendTask updated the parcel to DELIVERED and deleted the delivery as two independent fire-and-forget calls. If the update failed but the delete succeeded, the parcel was stranded in SENT with no delivery row, so it was never re-scheduled on restart and never delivered. Chain the delete after a successful update. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/parcel/task/ParcelSendTask.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/task/ParcelSendTask.java b/src/main/java/com/eternalcode/parcellockers/parcel/task/ParcelSendTask.java index 98adac43..258c7e7d 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/task/ParcelSendTask.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/task/ParcelSendTask.java @@ -51,15 +51,13 @@ public void run() { return; } + // Delete the delivery only after the status update succeeds. If the update fails, the + // delivery row is left intact so the task is rescheduled on the next startup; deleting it + // unconditionally could strand the parcel in SENT with no delivery, never to be delivered. this.parcelService.update(updated) + .thenCompose(ignored -> this.deliveryManager.delete(updated.uuid())) .exceptionally(throwable -> { - LOGGER.severe("Failed to update parcel " + updated.uuid() + " to DELIVERED status: " + throwable.getMessage()); - return null; - }); - - this.deliveryManager.delete(updated.uuid()) - .exceptionally(throwable -> { - LOGGER.warning("Failed to delete delivery for parcel " + updated.uuid() + ": " + throwable.getMessage()); + LOGGER.severe("Failed to deliver parcel " + updated.uuid() + " (delivery left for retry): " + throwable.getMessage()); return null; }); } From abd3fa521dc0b5629b0b38ee04e93474a7f958e4 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 15:59:40 +0200 Subject: [PATCH 13/34] Fix M3: stop mutating the cache from inside its own loader ItemStorageManager.getOrCreate called create() as the cache.get loader, and create() writes the same key back via cache.put - which Caffeine forbids for the key being computed and can throw or corrupt the entry. Check the cache first and call create() outside the loader instead. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/itemstorage/ItemStorageManager.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java b/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java index 9eb5860e..16543d65 100644 --- a/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java +++ b/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java @@ -45,7 +45,13 @@ public CompletableFuture> get(UUID parcelId) { } public ItemStorage getOrCreate(UUID owner, List items) { - return this.cache.get(owner, key -> this.create(key, items)); + ItemStorage existing = this.cache.getIfPresent(owner); + if (existing != null) { + return existing; + } + // Do not call create() from inside cache.get(owner, loader): create() writes the same key + // back into the cache, and Caffeine forbids mutating the key being computed. + return this.create(owner, items); } public ItemStorage create(UUID owner, List items) { From 5667f23bca4fe5d5b9fd782cc6502926fb6e8883 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:03:46 +0200 Subject: [PATCH 14/34] Fix M4: surface item-storage save failures and return staged items On GUI close the staged items were read, the storage row deleted, and the new storage saved fire-and-forget - if the save failed after the delete, the items were silently lost. Make create/getOrCreate and GuiManager.saveItemStorage return the save future, chain delete -> save in ItemStorageGui, and give the items back to the player if saving fails. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/gui/GuiManager.java | 4 +-- .../implementation/locker/ItemStorageGui.java | 25 +++++++++++-------- .../itemstorage/ItemStorageManager.java | 12 ++++----- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java b/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java index f2c2fade..10415a37 100644 --- a/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java +++ b/src/main/java/com/eternalcode/parcellockers/gui/GuiManager.java @@ -87,8 +87,8 @@ public CompletableFuture getItemStorage(UUID owner) { .thenApply(optional -> optional.orElse(new ItemStorage(owner, List.of()))); } - public void saveItemStorage(UUID player, List items) { - this.itemStorageManager.create(player, items); + public CompletableFuture saveItemStorage(UUID player, List items) { + return this.itemStorageManager.create(player, items); } public CompletableFuture deleteItemStorage(UUID owner) { diff --git a/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/ItemStorageGui.java b/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/ItemStorageGui.java index 4d963b11..0dc1d6ea 100644 --- a/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/ItemStorageGui.java +++ b/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/ItemStorageGui.java @@ -104,17 +104,20 @@ void show(Player player, ParcelSize size) { } this.guiManager.deleteItemStorage(player.getUniqueId()) - .thenAccept(action -> { - this.guiManager.saveItemStorage(player.getUniqueId(), items); - new SendingGui( - this.scheduler, - this.guiSettings, - this.miniMessage, - this.noticeService, - this.guiManager, - this.state - ).show(player); - }).exceptionally(FutureHandler::handleException); + .thenCompose(action -> this.guiManager.saveItemStorage(player.getUniqueId(), items)) + .thenAccept(saved -> new SendingGui( + this.scheduler, + this.guiSettings, + this.miniMessage, + this.noticeService, + this.guiManager, + this.state + ).show(player)) + .exceptionally(throwable -> { + // Persisting the staged items failed; hand them back so they are not lost. + this.scheduler.run(() -> items.forEach(item -> ItemUtil.giveItem(player, item))); + return FutureHandler.handleException(throwable); + }); }); this.guiManager.getItemStorage(player.getUniqueId()).thenAccept(itemStorage -> { diff --git a/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java b/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java index 16543d65..ac8d5d2a 100644 --- a/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java +++ b/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java @@ -44,17 +44,17 @@ public CompletableFuture> get(UUID parcelId) { }); } - public ItemStorage getOrCreate(UUID owner, List items) { + public CompletableFuture getOrCreate(UUID owner, List items) { ItemStorage existing = this.cache.getIfPresent(owner); if (existing != null) { - return existing; + return CompletableFuture.completedFuture(existing); } // Do not call create() from inside cache.get(owner, loader): create() writes the same key // back into the cache, and Caffeine forbids mutating the key being computed. return this.create(owner, items); } - public ItemStorage create(UUID owner, List items) { + public CompletableFuture create(UUID owner, List items) { ItemStorage oldItemStorage = this.cache.getIfPresent(owner); ItemStorage newItemStorage = new ItemStorage(owner, items); @@ -63,12 +63,12 @@ public ItemStorage create(UUID owner, List items) { this.server.getPluginManager().callEvent(event); if (event.isCancelled()) { - throw new IllegalStateException("ItemStorage update was cancelled by event"); + return CompletableFuture.failedFuture(new IllegalStateException("ItemStorage update was cancelled by event")); } this.cache.put(owner, newItemStorage); - this.itemStorageRepository.save(newItemStorage); - return newItemStorage; + // Return the save future so callers can react to a persistence failure instead of losing items. + return this.itemStorageRepository.save(newItemStorage).thenApply(ignored -> newItemStorage); } private void cacheAll() { From 4f127cdedb24ccfa386935128cc074b763418385 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:04:26 +0200 Subject: [PATCH 15/34] Fix M5: fail fast when the item storage table cannot be created ItemStorageRepositoryOrmLite swallowed a failed createTableIfNotExists with printStackTrace, unlike every other repository, so a missing table only surfaced later as confusing query errors. Throw DatabaseException for consistency and fail-fast startup. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../itemstorage/repository/ItemStorageRepositoryOrmLite.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java index 65b3e9c7..7f601c11 100644 --- a/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java @@ -4,6 +4,7 @@ import com.eternalcode.parcellockers.database.DatabaseManager; import com.eternalcode.parcellockers.database.wrapper.AbstractRepositoryOrmLite; import com.eternalcode.parcellockers.itemstorage.ItemStorage; +import com.eternalcode.parcellockers.shared.exception.DatabaseException; import com.j256.ormlite.table.TableUtils; import java.sql.SQLException; import java.util.List; @@ -19,7 +20,7 @@ public ItemStorageRepositoryOrmLite(DatabaseManager databaseManager, Scheduler s try { TableUtils.createTableIfNotExists(databaseManager.connectionSource(), ItemStorageTable.class); } catch (SQLException ex) { - ex.printStackTrace(); + throw new DatabaseException("Failed to initialize item storage table", ex); } } From 1a940b6d2144f904d82bc493c237228c7902fc12 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:05:54 +0200 Subject: [PATCH 16/34] Fix M6: rename ambiguous repository wrapper methods The wrapper method named save() performed createOrUpdate (an upsert) while saveIfNotExist() performed createIfNotExists, an inversion that made "save" unable to persist updates. Rename to upsert() and insertIfAbsent() with doc comments, and update all callers. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../content/repository/ParcelContentRepositoryOrmLite.java | 2 +- .../database/wrapper/AbstractRepositoryOrmLite.java | 6 ++++-- .../delivery/repository/DeliveryRepositoryOrmLite.java | 2 +- .../discord/repository/DiscordLinkRepositoryOrmLite.java | 2 +- .../repository/ItemStorageRepositoryOrmLite.java | 2 +- .../locker/repository/LockerRepositoryOrmLite.java | 2 +- .../parcel/repository/ParcelRepositoryOrmLite.java | 4 ++-- .../user/repository/UserRepositoryOrmLite.java | 2 +- 8 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java index 2ffb5749..9686c3bf 100644 --- a/src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java @@ -25,7 +25,7 @@ public ParcelContentRepositoryOrmLite(DatabaseManager databaseManager, Scheduler @Override public CompletableFuture save(ParcelContent parcelContent) { - return this.saveIfNotExist(ParcelContentTable.class, ParcelContentTable.from(parcelContent)).thenApply(dao -> null); + return this.insertIfAbsent(ParcelContentTable.class, ParcelContentTable.from(parcelContent)).thenApply(dao -> null); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java index eb6db387..ca5fd5bf 100644 --- a/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java @@ -24,11 +24,13 @@ protected AbstractRepositoryOrmLite(DatabaseManager databaseManager, Scheduler s this.scheduler = scheduler; } - protected CompletableFuture save(Class type, T entity) { + /** Inserts the entity, or updates it if a row with the same id already exists. */ + protected CompletableFuture upsert(Class type, T entity) { return this.action(type, dao -> dao.createOrUpdate(entity)); } - protected CompletableFuture saveIfNotExist(Class type, T entity) { + /** Inserts the entity only if no row with the same id exists; an existing row is left untouched. */ + protected CompletableFuture insertIfAbsent(Class type, T entity) { return this.action(type, dao -> dao.createIfNotExists(entity)); } diff --git a/src/main/java/com/eternalcode/parcellockers/delivery/repository/DeliveryRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/delivery/repository/DeliveryRepositoryOrmLite.java index 6f7207f6..89a28ae5 100644 --- a/src/main/java/com/eternalcode/parcellockers/delivery/repository/DeliveryRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/delivery/repository/DeliveryRepositoryOrmLite.java @@ -25,7 +25,7 @@ public DeliveryRepositoryOrmLite(DatabaseManager databaseManager, Scheduler sche @Override public CompletableFuture save(Delivery delivery) { - return this.saveIfNotExist(DeliveryTable.class, DeliveryTable.from(delivery)).thenApply(dao -> null); + return this.insertIfAbsent(DeliveryTable.class, DeliveryTable.from(delivery)).thenApply(dao -> null); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java index b7835fa4..d26b2798 100644 --- a/src/main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java @@ -26,7 +26,7 @@ public DiscordLinkRepositoryOrmLite(DatabaseManager databaseManager, Scheduler s @Override public CompletableFuture save(DiscordLink link) { - return this.save(DiscordLinkEntity.class, DiscordLinkEntity.fromDomain(link)) + return this.upsert(DiscordLinkEntity.class, DiscordLinkEntity.fromDomain(link)) .thenApply(status -> status.isCreated() || status.isUpdated()); } diff --git a/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java index 7f601c11..ee5c178f 100644 --- a/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java @@ -26,7 +26,7 @@ public ItemStorageRepositoryOrmLite(DatabaseManager databaseManager, Scheduler s @Override public CompletableFuture save(ItemStorage itemStorage) { - return this.saveIfNotExist(ItemStorageTable.class, ItemStorageTable.from(itemStorage.owner(), itemStorage.items())).thenApply(dao -> null); + return this.insertIfAbsent(ItemStorageTable.class, ItemStorageTable.from(itemStorage.owner(), itemStorage.items())).thenApply(dao -> null); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java index 6c5e6956..2174cf7c 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java @@ -30,7 +30,7 @@ public LockerRepositoryOrmLite(DatabaseManager databaseManager, Scheduler schedu @Override public CompletableFuture save(Locker locker) { - return this.saveIfNotExist(LockerTable.class, LockerTable.from(locker)).thenApply(LockerTable::toLocker); + return this.insertIfAbsent(LockerTable.class, LockerTable.from(locker)).thenApply(LockerTable::toLocker); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java index d177f604..872f9e27 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java @@ -37,13 +37,13 @@ public ParcelRepositoryOrmLite(DatabaseManager databaseManager, Scheduler schedu @Override public CompletableFuture save(Parcel parcel) { Objects.requireNonNull(parcel, "Parcel cannot be null"); - return this.saveIfNotExist(ParcelTable.class, ParcelTable.from(parcel)).thenApply(dao -> null); + return this.insertIfAbsent(ParcelTable.class, ParcelTable.from(parcel)).thenApply(dao -> null); } @Override public CompletableFuture update(Parcel parcel) { Objects.requireNonNull(parcel, "Parcel cannot be null"); - return this.save(ParcelTable.class, ParcelTable.from(parcel)).thenApply(dao -> null); + return this.upsert(ParcelTable.class, ParcelTable.from(parcel)).thenApply(dao -> null); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java index f33cea14..29138b1c 100644 --- a/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java @@ -44,7 +44,7 @@ public CompletableFuture> fetch(String name) { @Override public CompletableFuture save(User user) { - return this.save(UserTable.class, UserTable.from(user)).exceptionally(ex -> { + return this.upsert(UserTable.class, UserTable.from(user)).exceptionally(ex -> { System.err.println("Failed to save user: " + ex.getMessage()); ex.printStackTrace(); return null; From 73629b0304390ad96b7d42b05d14ece598b564c3 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:06:26 +0200 Subject: [PATCH 17/34] Fix M7: make DAO cache population atomic getDao used a non-atomic get-then-put on the ConcurrentHashMap, so two threads could create and store the DAO twice. Use computeIfAbsent. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/database/DatabaseManager.java | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java b/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java index 52a4b52d..f4f272dc 100644 --- a/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java +++ b/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java @@ -97,18 +97,15 @@ public void disconnect() { @SuppressWarnings("unchecked") public Dao getDao(Class type) { - try { - Dao dao = this.cachedDao.get(type); - - if (dao == null) { - dao = DaoManager.createDao(this.connectionSource, type); - this.cachedDao.put(type, dao); + Dao dao = this.cachedDao.computeIfAbsent(type, key -> { + try { + return DaoManager.createDao(this.connectionSource, key); + } catch (SQLException exception) { + throw new DatabaseException("Failed to get DAO for type: " + key.getSimpleName(), exception); } + }); - return (Dao) dao; - } catch (SQLException exception) { - throw new DatabaseException("Failed to get DAO for type: " + type.getSimpleName(), exception); - } + return (Dao) dao; } public ConnectionSource connectionSource() { From 81fdf004e8cfad1ec1c5577e20bf388d0814cf36 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:07:11 +0200 Subject: [PATCH 18/34] Fix M8: make connection pool tuning configurable The Hikari pool was hard-capped at 5 connections with a leak-detection threshold equal to the connection timeout (5s), which logs false leak warnings for any slow query. Expose connectionPoolSize, connectionTimeoutMillis and leakDetectionThresholdMillis in the config and default the leak threshold to a safer 30s. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../configuration/implementation/PluginConfig.java | 13 +++++++++++++ .../parcellockers/database/DatabaseManager.java | 6 +++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/configuration/implementation/PluginConfig.java b/src/main/java/com/eternalcode/parcellockers/configuration/implementation/PluginConfig.java index 1b893aea..8d642a50 100644 --- a/src/main/java/com/eternalcode/parcellockers/configuration/implementation/PluginConfig.java +++ b/src/main/java/com/eternalcode/parcellockers/configuration/implementation/PluginConfig.java @@ -50,6 +50,19 @@ public static class Settings extends OkaeriConfig { @Comment({ "", "# The database password." }) public String password = ""; + @Comment({ "", "# Maximum number of connections held in the database pool." }) + public int connectionPoolSize = 10; + + @Comment({ "", "# How long (in milliseconds) to wait for a free connection before failing." }) + public long connectionTimeoutMillis = 5000; + + @Comment({ + "", + "# Connection leak detection threshold in milliseconds (0 disables it).", + "# Set this comfortably above your slowest expected query to avoid false warnings." + }) + public long leakDetectionThresholdMillis = 30000; + @Comment({ "", "# The parcel locker item." }) public ConfigItem parcelLockerItem = new ConfigItem() .name("&3Parcel locker") diff --git a/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java b/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java index f4f272dc..be7f590a 100644 --- a/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java +++ b/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java @@ -44,9 +44,9 @@ public void connect() throws SQLException { this.dataSource.addDataSourceProperty("prepStmtCacheSqlLimit", 2048); this.dataSource.addDataSourceProperty("useServerPrepStmts", true); - this.dataSource.setMaximumPoolSize(5); - this.dataSource.setConnectionTimeout(5000); - this.dataSource.setLeakDetectionThreshold(5000); + this.dataSource.setMaximumPoolSize(this.config.settings.connectionPoolSize); + this.dataSource.setConnectionTimeout(this.config.settings.connectionTimeoutMillis); + this.dataSource.setLeakDetectionThreshold(this.config.settings.leakDetectionThresholdMillis); this.dataSource.setUsername(this.config.settings.user); this.dataSource.setPassword(this.config.settings.password); From 32ca84d03b92989d990f3171ef8aad30477abbc8 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:10:50 +0200 Subject: [PATCH 19/34] Fix M2: make domain event firing thread deterministic Whether a callEvent was legal depended on the incidental calling thread: UserChangeNameEvent (a sync event) was fired from the async user futures, and LockerDeleteEvent was fired from either the main thread (cache hit) or an async DB thread (cache miss) - both could throw IllegalStateException. Mark UserChangeNameEvent asynchronous to match its firing context (consistent with UserCreateEvent), and fire LockerDeleteEvent on the main thread deterministically via the scheduler so it stays a valid sync event. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/ParcelLockers.java | 2 +- .../parcellockers/locker/LockerManager.java | 28 ++++++++++++++----- .../locker/event/LockerDeleteEvent.java | 2 +- .../user/event/UserChangeNameEvent.java | 2 ++ 4 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java b/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java index 03c31723..d8fb1c35 100644 --- a/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java +++ b/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java @@ -134,7 +134,7 @@ public void onEnable() { UserValidationService userValidationService = new UserValidator(); UserManager userManager = new UserManagerImpl(userRepository, userValidationService, server); LockerValidationService lockerValidationService = new LockerValidator(); - LockerManager lockerManager = new LockerManager(config, lockerRepository, lockerValidationService, parcelRepository, server); + LockerManager lockerManager = new LockerManager(config, lockerRepository, lockerValidationService, parcelRepository, server, scheduler); ParcelContentManager parcelContentManager = new ParcelContentManager(parcelContentRepository); ItemStorageManager itemStorageManager = new ItemStorageManager(itemStorageRepository, server); DeliveryManager deliveryManager = new DeliveryManager(deliveryRepository); diff --git a/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java b/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java index 370629e1..bf31e3dc 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java @@ -1,5 +1,6 @@ package com.eternalcode.parcellockers.locker; +import com.eternalcode.commons.scheduler.Scheduler; import com.eternalcode.parcellockers.configuration.implementation.PluginConfig; import com.eternalcode.parcellockers.locker.event.LockerCreateEvent; import com.eternalcode.parcellockers.locker.event.LockerDeleteEvent; @@ -29,6 +30,7 @@ public class LockerManager { private final LockerValidationService validationService; private final ParcelRepository parcelRepository; private final Server server; + private final Scheduler scheduler; private final Cache lockersByUUID; private final Cache lockersByPosition; @@ -38,13 +40,15 @@ public LockerManager( LockerRepository lockerRepository, LockerValidationService validationService, ParcelRepository parcelRepository, - Server server + Server server, + Scheduler scheduler ) { this.config = config; this.lockerRepository = lockerRepository; this.validationService = validationService; this.parcelRepository = parcelRepository; this.server = server; + this.scheduler = scheduler; this.lockersByUUID = Caffeine.newBuilder() .expireAfterAccess(Duration.ofHours(2)) @@ -161,15 +165,25 @@ public CompletableFuture delete(UUID uniqueId, UUID playerUUID) { return CompletableFuture.completedFuture(null); } + // The locker may be resolved on either the main thread (cache hit) or an async DB thread + // (cache miss), so fire the synchronous event on the main thread deterministically. + return this.fireDeleteEvent(locker, playerUUID).thenCompose(cancelled -> { + if (cancelled) { + return CompletableFuture.completedFuture(null); + } + return this.deleteLocker(uniqueId); + }); + }); + } + + private CompletableFuture fireDeleteEvent(Locker locker, UUID playerUUID) { + CompletableFuture cancelled = new CompletableFuture<>(); + this.scheduler.run(() -> { LockerDeleteEvent event = new LockerDeleteEvent(locker, playerUUID); this.server.getPluginManager().callEvent(event); - - if (event.isCancelled()) { - return CompletableFuture.completedFuture(null); - } - - return this.deleteLocker(uniqueId); + cancelled.complete(event.isCancelled()); }); + return cancelled; } public CompletableFuture deleteAll(CommandSender sender, NoticeService noticeService) { diff --git a/src/main/java/com/eternalcode/parcellockers/locker/event/LockerDeleteEvent.java b/src/main/java/com/eternalcode/parcellockers/locker/event/LockerDeleteEvent.java index 232dfdbd..e47636e8 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/event/LockerDeleteEvent.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/event/LockerDeleteEvent.java @@ -7,7 +7,7 @@ import org.bukkit.event.HandlerList; import org.jetbrains.annotations.NotNull; -// Called when a locker is deleted +// Called when a locker is deleted. Fired synchronously on the main thread (see LockerManager#delete). // Warning: this event is not called when all lockers are deleted through "/parcel debug delete lockers" command public class LockerDeleteEvent extends Event implements Cancellable { diff --git a/src/main/java/com/eternalcode/parcellockers/user/event/UserChangeNameEvent.java b/src/main/java/com/eternalcode/parcellockers/user/event/UserChangeNameEvent.java index 972adcb9..70d3d9d8 100644 --- a/src/main/java/com/eternalcode/parcellockers/user/event/UserChangeNameEvent.java +++ b/src/main/java/com/eternalcode/parcellockers/user/event/UserChangeNameEvent.java @@ -5,6 +5,7 @@ import org.bukkit.event.HandlerList; import org.jetbrains.annotations.NotNull; +// Fired asynchronously from the user-management futures (consistent with UserCreateEvent). public class UserChangeNameEvent extends Event { private static final HandlerList HANDLER_LIST = new HandlerList(); @@ -13,6 +14,7 @@ public class UserChangeNameEvent extends Event { private final String oldName; public UserChangeNameEvent(User user, String oldName) { + super(true); this.user = user; this.oldName = oldName; } From 0e0523f4029a60bb96028153722a88e16a9ec034 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:11:56 +0200 Subject: [PATCH 20/34] Fix L1: switch on the DatabaseType enum directly databaseType is already a DatabaseType, so DatabaseType.valueOf( databaseType.toString().toUpperCase()) was a redundant round-trip. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../com/eternalcode/parcellockers/database/DatabaseManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java b/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java index be7f590a..7e23c45c 100644 --- a/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java +++ b/src/main/java/com/eternalcode/parcellockers/database/DatabaseManager.java @@ -50,7 +50,7 @@ public void connect() throws SQLException { this.dataSource.setUsername(this.config.settings.user); this.dataSource.setPassword(this.config.settings.password); - switch (DatabaseType.valueOf(databaseType.toString().toUpperCase())) { + switch (databaseType) { case MYSQL -> { this.dataSource.setDriverClassName("com.mysql.cj.jdbc.Driver"); From bfd55ab3b789ba00f88bea8b9c890daaa37f1d21 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:12:52 +0200 Subject: [PATCH 21/34] Fix L3: tear down listeners/tasks before closing the datasource onDisable closed the datasource first, so in-flight async DB tasks failed against a closed pool. Unregister listeners, cancel plugin tasks, and shut down commands/Discord before disconnecting the database. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../com/eternalcode/parcellockers/ParcelLockers.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java b/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java index d8fb1c35..13624ed7 100644 --- a/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java +++ b/src/main/java/com/eternalcode/parcellockers/ParcelLockers.java @@ -71,6 +71,7 @@ import org.bstats.bukkit.Metrics; import org.bukkit.Server; import org.bukkit.command.CommandSender; +import org.bukkit.event.HandlerList; import org.bukkit.plugin.RegisteredServiceProvider; import org.bukkit.plugin.java.JavaPlugin; @@ -230,9 +231,10 @@ public void onEnable() { @Override public void onDisable() { - if (this.databaseManager != null) { - this.databaseManager.disconnect(); - } + // Stop accepting new work before closing the datasource, so fewer in-flight async DB tasks + // run into an already-closed connection pool. + HandlerList.unregisterAll(this); + this.getServer().getScheduler().cancelTasks(this); if (this.liteCommands != null) { this.liteCommands.unregister(); @@ -241,6 +243,10 @@ public void onDisable() { if (this.discordClientManager != null) { this.discordClientManager.shutdown(); } + + if (this.databaseManager != null) { + this.databaseManager.disconnect(); + } } private boolean setupEconomy() { From 054f7d692d7a95317a430e40e66ce50ecdaf0a8c Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:13:20 +0200 Subject: [PATCH 22/34] Fix L4: remove misleading stale comment in LockerPlaceController The "Replace with actual config message" comment was outdated - the locker description prompt already comes from MessageConfig. (The remaining hardcoded English dialog button/input labels are left for a dedicated i18n pass.) Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/locker/controller/LockerPlaceController.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java index 7fd083e6..bec00701 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java @@ -89,7 +89,7 @@ public void onBlockPlace(BlockPlaceEvent event) { this.lockerCreators.put(player.getUniqueId(), true); - Component promptMessage = this.miniMessage.deserialize(this.messages.locker.descriptionPrompt); // Replace with actual config message + Component promptMessage = this.miniMessage.deserialize(this.messages.locker.descriptionPrompt); Dialog dialog = Dialog.create(builder -> builder.empty() .base(DialogBase.builder(promptMessage) From 3da519edea3c48172ecd80ae50f3bc422279b234 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:14:50 +0200 Subject: [PATCH 23/34] Fix L5: account for stacking in the collection space check The collect space check compared item count to fully-empty slots only, so it could refuse a parcel whose items would stack into partially-filled slots. Add InventoryUtil.canHold which simulates placement (topping up matching stacks, then spilling into empty slots) and use it in collect(). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcel/service/ParcelServiceImpl.java | 4 +- .../parcellockers/util/InventoryUtil.java | 53 +++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java index 3d12b84f..91094f69 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java @@ -1,6 +1,6 @@ package com.eternalcode.parcellockers.parcel.service; -import static com.eternalcode.parcellockers.util.InventoryUtil.freeSlotsInInventory; +import static com.eternalcode.parcellockers.util.InventoryUtil.canHold; import com.eternalcode.commons.bukkit.ItemUtil; import com.eternalcode.commons.scheduler.Scheduler; @@ -222,7 +222,7 @@ public CompletableFuture collect(Player player, Parcel parcel) { // then delete the parcel BEFORE handing the items back so it cannot be collected twice. // Items are only given once the delete is confirmed, so a failed delete never destroys them. this.scheduler.run(() -> { - if (items.size() > freeSlotsInInventory(player)) { + if (!canHold(player, items)) { this.noticeService.player(player.getUniqueId(), messages -> messages.parcel.noInventorySpace); result.complete(null); return; diff --git a/src/main/java/com/eternalcode/parcellockers/util/InventoryUtil.java b/src/main/java/com/eternalcode/parcellockers/util/InventoryUtil.java index e469b6c0..15cde11a 100644 --- a/src/main/java/com/eternalcode/parcellockers/util/InventoryUtil.java +++ b/src/main/java/com/eternalcode/parcellockers/util/InventoryUtil.java @@ -1,5 +1,6 @@ package com.eternalcode.parcellockers.util; +import java.util.List; import org.bukkit.entity.Player; import org.bukkit.inventory.ItemStack; @@ -18,4 +19,56 @@ public static int freeSlotsInInventory(Player player) { } return freeSlots; } + + /** + * Returns whether all of the given items would fit into the player's inventory, accounting for + * stacking into partially-filled slots rather than only counting fully empty slots. + */ + public static boolean canHold(Player player, List items) { + ItemStack[] contents = player.getInventory().getStorageContents(); + + // Simulate placement against a snapshot of the current slot amounts. + ItemStack[] slotType = new ItemStack[contents.length]; + int[] slotAmount = new int[contents.length]; + for (int i = 0; i < contents.length; i++) { + if (contents[i] != null) { + slotType[i] = contents[i]; + slotAmount[i] = contents[i].getAmount(); + } + } + + for (ItemStack item : items) { + if (item == null) { + continue; + } + + int remaining = item.getAmount(); + int maxStack = item.getMaxStackSize(); + + // Top up existing matching stacks first. + for (int i = 0; i < contents.length && remaining > 0; i++) { + if (slotType[i] != null && slotType[i].isSimilar(item) && slotAmount[i] < maxStack) { + int added = Math.min(maxStack - slotAmount[i], remaining); + slotAmount[i] += added; + remaining -= added; + } + } + + // Then spill into empty slots. + for (int i = 0; i < contents.length && remaining > 0; i++) { + if (slotType[i] == null) { + slotType[i] = item; + int added = Math.min(maxStack, remaining); + slotAmount[i] = added; + remaining -= added; + } + } + + if (remaining > 0) { + return false; + } + } + + return true; + } } From cb9c16b3f192a6d03ec2450862550041b68daf2b Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:15:46 +0200 Subject: [PATCH 24/34] Refactor D5: collapse duplicate createActiveItem overloads The two SendingGui.createActiveItem overloads differed only in add vs addAll. Have the single-line variant delegate to the list variant. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../gui/implementation/locker/SendingGui.java | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/SendingGui.java b/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/SendingGui.java index 1341c0ef..5019fe67 100644 --- a/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/SendingGui.java +++ b/src/main/java/com/eternalcode/parcellockers/gui/implementation/locker/SendingGui.java @@ -417,13 +417,7 @@ public void updateStorageItem(Player player) { } private ItemStack createActiveItem(ConfigItem item, String appendLore) { - List itemLore = new ArrayList<>(item.lore()); - itemLore.add(appendLore); - - return item.toBuilder() - .lore(itemLore.stream().map(element -> resetItalic(this.miniMessage.deserialize(element))).toList()) - .glow(true) - .build(); + return this.createActiveItem(item, List.of(appendLore)); } private ItemStack createActiveItem(ConfigItem item, List appendLore) { From 6773e022abb7a71350fe49808dabe6518866b01e Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:20:09 +0200 Subject: [PATCH 25/34] Refactor D1: extract repository table bootstrap into the base class Every *OrmLite constructor duplicated the same createTableIfNotExists try/catch. Add AbstractRepositoryOrmLite#createTable and call it from each repository. This also fixes the Delivery and User repositories, which silently swallowed table-creation failures with printStackTrace (the same bug fixed for ItemStorage in M5) - they now fail fast like the others. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../repository/ParcelContentRepositoryOrmLite.java | 10 +--------- .../database/wrapper/AbstractRepositoryOrmLite.java | 10 ++++++++++ .../delivery/repository/DeliveryRepositoryOrmLite.java | 9 +-------- .../repository/DiscordLinkRepositoryOrmLite.java | 10 +--------- .../repository/ItemStorageRepositoryOrmLite.java | 10 +--------- .../locker/repository/LockerRepositoryOrmLite.java | 10 +--------- .../parcel/repository/ParcelRepositoryOrmLite.java | 10 +--------- .../user/repository/UserRepositoryOrmLite.java | 9 +-------- 8 files changed, 17 insertions(+), 61 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java index 9686c3bf..b1fd9df3 100644 --- a/src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/content/repository/ParcelContentRepositoryOrmLite.java @@ -4,9 +4,6 @@ import com.eternalcode.parcellockers.content.ParcelContent; import com.eternalcode.parcellockers.database.DatabaseManager; import com.eternalcode.parcellockers.database.wrapper.AbstractRepositoryOrmLite; -import com.eternalcode.parcellockers.shared.exception.DatabaseException; -import com.j256.ormlite.table.TableUtils; -import java.sql.SQLException; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; @@ -15,12 +12,7 @@ public class ParcelContentRepositoryOrmLite extends AbstractRepositoryOrmLite im public ParcelContentRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { super(databaseManager, scheduler); - - try { - TableUtils.createTableIfNotExists(databaseManager.connectionSource(), ParcelContentTable.class); - } catch (SQLException exception) { - throw new DatabaseException("Failed to create ParcelContent table", exception); - } + this.createTable(ParcelContentTable.class); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java index ca5fd5bf..d473e908 100644 --- a/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java @@ -5,6 +5,7 @@ import com.eternalcode.parcellockers.database.DatabaseManager; import com.eternalcode.parcellockers.shared.exception.DatabaseException; import com.j256.ormlite.dao.Dao; +import com.j256.ormlite.table.TableUtils; import java.sql.SQLException; import java.util.List; import java.util.Optional; @@ -24,6 +25,15 @@ protected AbstractRepositoryOrmLite(DatabaseManager databaseManager, Scheduler s this.scheduler = scheduler; } + /** Creates the backing table if it does not exist, failing fast with a {@link DatabaseException}. */ + protected void createTable(Class tableType) { + try { + TableUtils.createTableIfNotExists(this.databaseManager.connectionSource(), tableType); + } catch (SQLException exception) { + throw new DatabaseException("Failed to initialize table " + tableType.getSimpleName(), exception); + } + } + /** Inserts the entity, or updates it if a row with the same id already exists. */ protected CompletableFuture upsert(Class type, T entity) { return this.action(type, dao -> dao.createOrUpdate(entity)); diff --git a/src/main/java/com/eternalcode/parcellockers/delivery/repository/DeliveryRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/delivery/repository/DeliveryRepositoryOrmLite.java index 89a28ae5..6a6f6c5a 100644 --- a/src/main/java/com/eternalcode/parcellockers/delivery/repository/DeliveryRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/delivery/repository/DeliveryRepositoryOrmLite.java @@ -4,8 +4,6 @@ import com.eternalcode.parcellockers.database.DatabaseManager; import com.eternalcode.parcellockers.database.wrapper.AbstractRepositoryOrmLite; import com.eternalcode.parcellockers.delivery.Delivery; -import com.j256.ormlite.table.TableUtils; -import java.sql.SQLException; import java.util.List; import java.util.Optional; import java.util.UUID; @@ -15,12 +13,7 @@ public class DeliveryRepositoryOrmLite extends AbstractRepositoryOrmLite impleme public DeliveryRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { super(databaseManager, scheduler); - - try { - TableUtils.createTableIfNotExists(databaseManager.connectionSource(), DeliveryTable.class); - } catch (SQLException exception) { - exception.printStackTrace(); - } + this.createTable(DeliveryTable.class); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java index d26b2798..d37e04a1 100644 --- a/src/main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java @@ -4,10 +4,7 @@ import com.eternalcode.parcellockers.database.DatabaseManager; import com.eternalcode.parcellockers.database.wrapper.AbstractRepositoryOrmLite; import com.eternalcode.parcellockers.discord.DiscordLink; -import com.eternalcode.parcellockers.shared.exception.DatabaseException; import com.j256.ormlite.stmt.DeleteBuilder; -import com.j256.ormlite.table.TableUtils; -import java.sql.SQLException; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; @@ -16,12 +13,7 @@ public class DiscordLinkRepositoryOrmLite extends AbstractRepositoryOrmLite impl public DiscordLinkRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { super(databaseManager, scheduler); - - try { - TableUtils.createTableIfNotExists(databaseManager.connectionSource(), DiscordLinkEntity.class); - } catch (SQLException ex) { - throw new DatabaseException("Failed to initialize DiscordLink table", ex); - } + this.createTable(DiscordLinkEntity.class); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java index ee5c178f..9c741685 100644 --- a/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/itemstorage/repository/ItemStorageRepositoryOrmLite.java @@ -4,9 +4,6 @@ import com.eternalcode.parcellockers.database.DatabaseManager; import com.eternalcode.parcellockers.database.wrapper.AbstractRepositoryOrmLite; import com.eternalcode.parcellockers.itemstorage.ItemStorage; -import com.eternalcode.parcellockers.shared.exception.DatabaseException; -import com.j256.ormlite.table.TableUtils; -import java.sql.SQLException; import java.util.List; import java.util.Optional; import java.util.UUID; @@ -16,12 +13,7 @@ public class ItemStorageRepositoryOrmLite extends AbstractRepositoryOrmLite impl public ItemStorageRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { super(databaseManager, scheduler); - - try { - TableUtils.createTableIfNotExists(databaseManager.connectionSource(), ItemStorageTable.class); - } catch (SQLException ex) { - throw new DatabaseException("Failed to initialize item storage table", ex); - } + this.createTable(ItemStorageTable.class); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java index 2174cf7c..1a0bff45 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java @@ -7,9 +7,6 @@ import com.eternalcode.parcellockers.shared.Page; import com.eternalcode.parcellockers.shared.PageResult; import com.eternalcode.parcellockers.shared.Position; -import com.eternalcode.parcellockers.shared.exception.DatabaseException; -import com.j256.ormlite.table.TableUtils; -import java.sql.SQLException; import java.util.List; import java.util.Optional; import java.util.UUID; @@ -20,12 +17,7 @@ public class LockerRepositoryOrmLite extends AbstractRepositoryOrmLite implement public LockerRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { super(databaseManager, scheduler); - - try { - TableUtils.createTableIfNotExists(databaseManager.connectionSource(), LockerTable.class); - } catch (SQLException ex) { - throw new DatabaseException("Failed to initialize locker table", ex); - } + this.createTable(LockerTable.class); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java index 872f9e27..a7ea55fd 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java @@ -6,9 +6,6 @@ import com.eternalcode.parcellockers.parcel.Parcel; import com.eternalcode.parcellockers.shared.Page; import com.eternalcode.parcellockers.shared.PageResult; -import com.eternalcode.parcellockers.shared.exception.DatabaseException; -import com.j256.ormlite.table.TableUtils; -import java.sql.SQLException; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -26,12 +23,7 @@ public class ParcelRepositoryOrmLite extends AbstractRepositoryOrmLite implement public ParcelRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { super(databaseManager, scheduler); - - try { - TableUtils.createTableIfNotExists(databaseManager.connectionSource(), ParcelTable.class); - } catch (SQLException ex) { - throw new DatabaseException("Failed to initialize parcel table", ex); - } + this.createTable(ParcelTable.class); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java index 29138b1c..8b256b86 100644 --- a/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java @@ -6,8 +6,6 @@ import com.eternalcode.parcellockers.shared.Page; import com.eternalcode.parcellockers.shared.PageResult; import com.eternalcode.parcellockers.user.User; -import com.j256.ormlite.table.TableUtils; -import java.sql.SQLException; import java.util.List; import java.util.Optional; import java.util.UUID; @@ -18,12 +16,7 @@ public class UserRepositoryOrmLite extends AbstractRepositoryOrmLite implements public UserRepositoryOrmLite(DatabaseManager databaseManager, Scheduler scheduler) { super(databaseManager, scheduler); - - try { - TableUtils.createTableIfNotExists(databaseManager.connectionSource(), UserTable.class); - } catch (SQLException exception) { - exception.printStackTrace(); - } + this.createTable(UserTable.class); } @Override From 779167a3194240b16431f0b5b7fc1f4711350d8b Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:23:22 +0200 Subject: [PATCH 26/34] Refactor D2: extract shared paginated-query helper Parcel, Locker and User repositories each reimplemented the limit+1 / offset / hasNext / removeLast pagination dance. Add AbstractRepositoryOrmLite#queryPage and route all three through it. This also fixes UserRepository.fetchPage, which fetched only `limit` rows yet checked size > limit, so it never reported a next page. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../wrapper/AbstractRepositoryOrmLite.java | 37 +++++++++++++++++++ .../repository/LockerRepositoryOrmLite.java | 18 +-------- .../repository/ParcelRepositoryOrmLite.java | 26 ++----------- .../repository/UserRepositoryOrmLite.java | 18 +-------- 4 files changed, 43 insertions(+), 56 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java index d473e908..9b736770 100644 --- a/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/database/wrapper/AbstractRepositoryOrmLite.java @@ -3,15 +3,22 @@ import com.eternalcode.commons.ThrowingFunction; import com.eternalcode.commons.scheduler.Scheduler; import com.eternalcode.parcellockers.database.DatabaseManager; +import com.eternalcode.parcellockers.shared.Page; +import com.eternalcode.parcellockers.shared.PageResult; import com.eternalcode.parcellockers.shared.exception.DatabaseException; import com.j256.ormlite.dao.Dao; +import com.j256.ormlite.stmt.QueryBuilder; import com.j256.ormlite.table.TableUtils; import java.sql.SQLException; +import java.util.ArrayList; +import java.util.Collections; import java.util.List; import java.util.Optional; import java.util.concurrent.CompletableFuture; +import java.util.function.Function; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.stream.Collectors; public abstract class AbstractRepositoryOrmLite { @@ -68,6 +75,36 @@ protected CompletableFuture> selectAll(Class type) { return this.action(type, Dao::queryForAll); } + /** + * Runs a paginated query. The {@code configure} callback may apply filters (e.g. a WHERE clause) + * to the query builder; one extra row is fetched to determine whether a following page exists. + */ + protected CompletableFuture> queryPage( + Class type, + Page page, + ThrowingFunction, QueryBuilder, SQLException> configure, + Function mapper + ) { + return this.>action(type, dao -> { + QueryBuilder builder = configure.apply(dao.queryBuilder()); + + List items = builder + .limit((long) page.getLimit() + 1) + .offset((long) page.getOffset()) + .query() + .stream() + .map(mapper) + .collect(Collectors.toCollection(ArrayList::new)); + + boolean hasNext = items.size() > page.getLimit(); + if (hasNext) { + items.removeLast(); + } + + return new PageResult<>(Collections.unmodifiableList(items), hasNext); + }); + } + protected CompletableFuture action(Class type, ThrowingFunction, R, SQLException> action) { CompletableFuture completableFuture = new CompletableFuture<>(); diff --git a/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java index 1a0bff45..dae09470 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/repository/LockerRepositoryOrmLite.java @@ -11,7 +11,6 @@ import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; -import java.util.stream.Collectors; public class LockerRepositoryOrmLite extends AbstractRepositoryOrmLite implements LockerRepository { @@ -51,22 +50,7 @@ public CompletableFuture delete(Locker locker) { @Override public CompletableFuture> findPage(Page page) { - return this.action( - LockerTable.class, dao -> { - List lockers = dao.queryBuilder() - .offset((long) page.getOffset()) - .limit((long) page.getLimit() + 1) - .query() - .stream().map(LockerTable::toLocker) - .collect(Collectors.toList()); - - boolean hasNext = lockers.size() > page.getLimit(); - if (hasNext) { - lockers.removeLast(); - } - - return new PageResult<>(lockers, hasNext); - }); + return this.queryPage(LockerTable.class, page, builder -> builder, LockerTable::toLocker); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java index a7ea55fd..95fda22b 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/repository/ParcelRepositoryOrmLite.java @@ -6,14 +6,11 @@ import com.eternalcode.parcellockers.parcel.Parcel; import com.eternalcode.parcellockers.shared.Page; import com.eternalcode.parcellockers.shared.PageResult; -import java.util.ArrayList; -import java.util.Collections; import java.util.List; import java.util.Objects; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; -import java.util.stream.Collectors; public class ParcelRepositoryOrmLite extends AbstractRepositoryOrmLite implements ParcelRepository { @@ -94,25 +91,10 @@ public CompletableFuture countParcelsByDestinationLocker(UUID destinati } private CompletableFuture> findByPaged(UUID key, Page page, String column) { - return this.action( - ParcelTable.class, dao -> { - List parcels = dao.queryBuilder() - .limit((long) page.getLimit() + 1) - .offset((long) page.getOffset()) - .where() - .eq(column, key) - .query() - .stream() - .map(ParcelTable::toParcel) - .collect(Collectors.toCollection(ArrayList::new)); - - boolean hasNext = parcels.size() > page.getLimit(); - if (hasNext) { - parcels.removeLast(); - } - - return new PageResult<>(Collections.unmodifiableList(parcels), hasNext); - }); + return this.queryPage(ParcelTable.class, page, builder -> { + builder.where().eq(column, key); + return builder; + }, ParcelTable::toParcel); } @Override diff --git a/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java b/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java index 8b256b86..a21fd788 100644 --- a/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java +++ b/src/main/java/com/eternalcode/parcellockers/user/repository/UserRepositoryOrmLite.java @@ -6,11 +6,9 @@ import com.eternalcode.parcellockers.shared.Page; import com.eternalcode.parcellockers.shared.PageResult; import com.eternalcode.parcellockers.user.User; -import java.util.List; import java.util.Optional; import java.util.UUID; import java.util.concurrent.CompletableFuture; -import java.util.stream.Collectors; public class UserRepositoryOrmLite extends AbstractRepositoryOrmLite implements UserRepository { @@ -57,20 +55,6 @@ public CompletableFuture changeName(UUID uuid, String newName) { @Override public CompletableFuture> fetchPage(Page page) { - return this.action( - UserTable.class, dao -> { - List users = dao.queryBuilder() - .offset((long) page.getOffset()) - .limit((long) page.getLimit()) - .query() - .stream().map(UserTable::toUser) - .collect(Collectors.toList()); - - boolean hasNext = users.size() > page.getLimit(); - if (hasNext) { - users.removeLast(); - } - return new PageResult<>(users, hasNext); - }); + return this.queryPage(UserTable.class, page, builder -> builder, UserTable::toUser); } } From d246ff53c66a828132da8c9a9edef421207ac23d Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:25:58 +0200 Subject: [PATCH 27/34] Refactor D4: share cancel handling via a CancellableEvent base The six cancellable plugin events each repeated the cancelled field plus isCancelled/setCancelled. Introduce shared/event/CancellableEvent holding that boilerplate (subclasses still own their HandlerList as Bukkit requires) and extend it from each event. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../event/ItemStorageUpdateEvent.java | 17 ++-------- .../locker/event/LockerCreateEvent.java | 17 ++-------- .../locker/event/LockerDeleteEvent.java | 17 ++-------- .../parcel/event/ParcelCollectEvent.java | 16 ++-------- .../parcel/event/ParcelDeliverEvent.java | 16 ++-------- .../parcel/event/ParcelSendEvent.java | 16 ++-------- .../shared/event/CancellableEvent.java | 31 +++++++++++++++++++ 7 files changed, 43 insertions(+), 87 deletions(-) create mode 100644 src/main/java/com/eternalcode/parcellockers/shared/event/CancellableEvent.java diff --git a/src/main/java/com/eternalcode/parcellockers/itemstorage/event/ItemStorageUpdateEvent.java b/src/main/java/com/eternalcode/parcellockers/itemstorage/event/ItemStorageUpdateEvent.java index bebb56fc..57bdf419 100644 --- a/src/main/java/com/eternalcode/parcellockers/itemstorage/event/ItemStorageUpdateEvent.java +++ b/src/main/java/com/eternalcode/parcellockers/itemstorage/event/ItemStorageUpdateEvent.java @@ -1,20 +1,17 @@ package com.eternalcode.parcellockers.itemstorage.event; import com.eternalcode.parcellockers.itemstorage.ItemStorage; -import org.bukkit.event.Cancellable; -import org.bukkit.event.Event; +import com.eternalcode.parcellockers.shared.event.CancellableEvent; import org.bukkit.event.HandlerList; import org.jetbrains.annotations.NotNull; -public class ItemStorageUpdateEvent extends Event implements Cancellable { +public class ItemStorageUpdateEvent extends CancellableEvent { private static final HandlerList HANDLER_LIST = new HandlerList(); private final ItemStorage oldItemStorage; private final ItemStorage updatedItemStorage; - private boolean cancelled; - public ItemStorageUpdateEvent(ItemStorage oldItemStorage, ItemStorage updatedItemStorage) { super(true); this.oldItemStorage = oldItemStorage; @@ -33,16 +30,6 @@ public ItemStorage getUpdatedItemStorage() { return this.updatedItemStorage; } - @Override - public boolean isCancelled() { - return this.cancelled; - } - - @Override - public void setCancelled(boolean cancel) { - this.cancelled = cancel; - } - @Override public @NotNull HandlerList getHandlers() { return HANDLER_LIST; diff --git a/src/main/java/com/eternalcode/parcellockers/locker/event/LockerCreateEvent.java b/src/main/java/com/eternalcode/parcellockers/locker/event/LockerCreateEvent.java index cc5f5e82..014670d1 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/event/LockerCreateEvent.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/event/LockerCreateEvent.java @@ -1,19 +1,17 @@ package com.eternalcode.parcellockers.locker.event; import com.eternalcode.parcellockers.locker.Locker; +import com.eternalcode.parcellockers.shared.event.CancellableEvent; import java.util.UUID; -import org.bukkit.event.Cancellable; -import org.bukkit.event.Event; import org.bukkit.event.HandlerList; import org.jetbrains.annotations.NotNull; -public class LockerCreateEvent extends Event implements Cancellable { +public class LockerCreateEvent extends CancellableEvent { private static final HandlerList HANDLER_LIST = new HandlerList(); private final Locker locker; private final UUID player; - private boolean cancelled; public LockerCreateEvent(Locker locker, UUID player) { super(true); @@ -33,19 +31,8 @@ public UUID getPlayer() { return this.player; } - @Override - public boolean isCancelled() { - return this.cancelled; - } - - @Override - public void setCancelled(boolean cancel) { - this.cancelled = cancel; - } - @Override public @NotNull HandlerList getHandlers() { return HANDLER_LIST; } - } diff --git a/src/main/java/com/eternalcode/parcellockers/locker/event/LockerDeleteEvent.java b/src/main/java/com/eternalcode/parcellockers/locker/event/LockerDeleteEvent.java index e47636e8..281dfc4c 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/event/LockerDeleteEvent.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/event/LockerDeleteEvent.java @@ -1,21 +1,19 @@ package com.eternalcode.parcellockers.locker.event; import com.eternalcode.parcellockers.locker.Locker; +import com.eternalcode.parcellockers.shared.event.CancellableEvent; import java.util.UUID; -import org.bukkit.event.Cancellable; -import org.bukkit.event.Event; import org.bukkit.event.HandlerList; import org.jetbrains.annotations.NotNull; // Called when a locker is deleted. Fired synchronously on the main thread (see LockerManager#delete). // Warning: this event is not called when all lockers are deleted through "/parcel debug delete lockers" command -public class LockerDeleteEvent extends Event implements Cancellable { +public class LockerDeleteEvent extends CancellableEvent { private static final HandlerList HANDLER_LIST = new HandlerList(); private final Locker locker; private final UUID player; - private boolean cancelled; public LockerDeleteEvent(Locker locker, UUID player) { this.locker = locker; @@ -34,19 +32,8 @@ public UUID getPlayer() { return this.player; } - @Override - public boolean isCancelled() { - return this.cancelled; - } - - @Override - public void setCancelled(boolean cancel) { - this.cancelled = cancel; - } - @Override public @NotNull HandlerList getHandlers() { return HANDLER_LIST; } - } diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelCollectEvent.java b/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelCollectEvent.java index 7cd54e99..45fbc8ff 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelCollectEvent.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelCollectEvent.java @@ -1,17 +1,15 @@ package com.eternalcode.parcellockers.parcel.event; import com.eternalcode.parcellockers.parcel.Parcel; -import org.bukkit.event.Cancellable; -import org.bukkit.event.Event; +import com.eternalcode.parcellockers.shared.event.CancellableEvent; import org.bukkit.event.HandlerList; import org.jetbrains.annotations.NotNull; -public class ParcelCollectEvent extends Event implements Cancellable { +public class ParcelCollectEvent extends CancellableEvent { private static final HandlerList HANDLER_LIST = new HandlerList(); private final Parcel parcel; - private boolean cancelled; public ParcelCollectEvent(Parcel parcel) { this.parcel = parcel; @@ -29,14 +27,4 @@ public Parcel getParcel() { public @NotNull HandlerList getHandlers() { return HANDLER_LIST; } - - @Override - public boolean isCancelled() { - return this.cancelled; - } - - @Override - public void setCancelled(boolean cancel) { - this.cancelled = cancel; - } } diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelDeliverEvent.java b/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelDeliverEvent.java index 47ac5347..dd8a9855 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelDeliverEvent.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelDeliverEvent.java @@ -1,17 +1,15 @@ package com.eternalcode.parcellockers.parcel.event; import com.eternalcode.parcellockers.parcel.Parcel; -import org.bukkit.event.Cancellable; -import org.bukkit.event.Event; +import com.eternalcode.parcellockers.shared.event.CancellableEvent; import org.bukkit.event.HandlerList; import org.jetbrains.annotations.NotNull; -public class ParcelDeliverEvent extends Event implements Cancellable { +public class ParcelDeliverEvent extends CancellableEvent { private static final HandlerList HANDLER_LIST = new HandlerList(); private final Parcel parcel; - private boolean cancelled; public ParcelDeliverEvent(Parcel parcel) { super(true); @@ -30,14 +28,4 @@ public Parcel getParcel() { public @NotNull HandlerList getHandlers() { return HANDLER_LIST; } - - @Override - public boolean isCancelled() { - return this.cancelled; - } - - @Override - public void setCancelled(boolean cancel) { - this.cancelled = cancel; - } } diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelSendEvent.java b/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelSendEvent.java index c1c83d69..68395516 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelSendEvent.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/event/ParcelSendEvent.java @@ -1,17 +1,15 @@ package com.eternalcode.parcellockers.parcel.event; import com.eternalcode.parcellockers.parcel.Parcel; -import org.bukkit.event.Cancellable; -import org.bukkit.event.Event; +import com.eternalcode.parcellockers.shared.event.CancellableEvent; import org.bukkit.event.HandlerList; import org.jetbrains.annotations.NotNull; -public class ParcelSendEvent extends Event implements Cancellable { +public class ParcelSendEvent extends CancellableEvent { private static final HandlerList HANDLER_LIST = new HandlerList(); private final Parcel parcel; - private boolean cancelled; public ParcelSendEvent(Parcel parcel) { super(true); @@ -30,14 +28,4 @@ public Parcel getParcel() { public @NotNull HandlerList getHandlers() { return HANDLER_LIST; } - - @Override - public boolean isCancelled() { - return this.cancelled; - } - - @Override - public void setCancelled(boolean cancel) { - this.cancelled = cancel; - } } diff --git a/src/main/java/com/eternalcode/parcellockers/shared/event/CancellableEvent.java b/src/main/java/com/eternalcode/parcellockers/shared/event/CancellableEvent.java new file mode 100644 index 00000000..56bdcf66 --- /dev/null +++ b/src/main/java/com/eternalcode/parcellockers/shared/event/CancellableEvent.java @@ -0,0 +1,31 @@ +package com.eternalcode.parcellockers.shared.event; + +import org.bukkit.event.Cancellable; +import org.bukkit.event.Event; + +/** + * Base class for the plugin's cancellable Bukkit events. Provides the cancel flag and the + * {@link Cancellable} implementation; subclasses still declare their own {@code HandlerList} + * (and static {@code getHandlerList()}) as Bukkit requires one per event type. + */ +public abstract class CancellableEvent extends Event implements Cancellable { + + private boolean cancelled; + + protected CancellableEvent() { + } + + protected CancellableEvent(boolean async) { + super(async); + } + + @Override + public boolean isCancelled() { + return this.cancelled; + } + + @Override + public void setCancelled(boolean cancel) { + this.cancelled = cancel; + } +} From f565a54a953090368287d936224a7ff85c7345f2 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:27:24 +0200 Subject: [PATCH 28/34] Record audit resolution status in AUDIT.md Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- AUDIT.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/AUDIT.md b/AUDIT.md index f42ca497..dd192fce 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -12,6 +12,25 @@ inconsistency) ยท **๐Ÿ”ต Low** (style, minor optimization). --- +## Resolution status (`audit-fixes` branch) + +All findings below are fixed on the `audit-fixes` branch, one commit each, except where noted: + +- **Critical:** C1, C2, C3, C4 โ€” fixed. +- **High:** H1, H2, H3, H4, H5, H6 โ€” fixed. +- **Medium:** M1โ€“M8 โ€” fixed. +- **Low:** L1, L3, L4, L5 โ€” fixed. **L2** was folded into the H2 fix (the redundant + `count > 0` clause was removed there). +- **Duplication:** D1, D2, D4, D5 โ€” fixed (D1 also fixed the same swallowed table-creation + bug in the Delivery and User repos; D2 also fixed a latent always-false "has next page" in + `UserRepository.fetchPage`). **D3** (generic cache-or-fetch helper) was intentionally left: + the five managers use their caches differently enough that one abstraction would risk + masking per-cache nuance, and the concrete inconsistency it could hide was already fixed by H6. + +`./gradlew compileJava` and `./gradlew test` pass on the branch. + +--- + ## ๐Ÿ”ด Critical ### C1 โ€” Economy fee is charged before persistence and never refunded on failure From f8593879773eea038efab395cf30da55358fd092 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 16:44:59 +0200 Subject: [PATCH 29/34] Address PR review feedback (cache/rollback/item-loss edge cases) - ItemStorageManager.create: revert the optimistic cache entry if the save fails, so the cache never holds an unpersisted storage (which, with items re-given to the player, would duplicate them). - LockerPlaceController: consume the locker item before creating and abort if it is gone; refund the item and clear the block if creation fails. consumeLockerItem now returns whether it consumed. - ParcelServiceImpl.collect: once the parcel is deleted, treat content deletion as best-effort and always give the items back (a failed content delete no longer loses them permanently). - UserManagerImpl.create: chain the save future and invalidate the cache on failure instead of fire-and-forget, avoiding a cache-DB desync. - ParcelDispatchService: map an exceptional itemStorage delete to false so it triggers rollbackSend instead of bypassing it. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../itemstorage/ItemStorageManager.java | 14 +++- .../controller/LockerPlaceController.java | 69 ++++++++++++------- .../parcel/service/ParcelDispatchService.java | 3 + .../parcel/service/ParcelServiceImpl.java | 18 ++++- .../parcellockers/user/UserManagerImpl.java | 17 +++-- 5 files changed, 88 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java b/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java index ac8d5d2a..358d51a2 100644 --- a/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java +++ b/src/main/java/com/eternalcode/parcellockers/itemstorage/ItemStorageManager.java @@ -68,7 +68,19 @@ public CompletableFuture create(UUID owner, List items) this.cache.put(owner, newItemStorage); // Return the save future so callers can react to a persistence failure instead of losing items. - return this.itemStorageRepository.save(newItemStorage).thenApply(ignored -> newItemStorage); + // If the save fails, undo the optimistic cache update so the cache never holds an unpersisted + // storage (which, combined with re-giving the items to the player, would duplicate them). + return this.itemStorageRepository.save(newItemStorage) + .whenComplete((ignored, throwable) -> { + if (throwable != null) { + if (oldItemStorage != null) { + this.cache.put(owner, oldItemStorage); + } else { + this.cache.invalidate(owner); + } + } + }) + .thenApply(ignored -> newItemStorage); } private void cacheAll() { diff --git a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java index bec00701..7e329e86 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java @@ -1,5 +1,6 @@ package com.eternalcode.parcellockers.locker.controller; +import com.eternalcode.commons.bukkit.ItemUtil; import com.eternalcode.commons.scheduler.Scheduler; import com.eternalcode.parcellockers.configuration.implementation.MessageConfig; import com.eternalcode.parcellockers.configuration.implementation.PluginConfig; @@ -124,32 +125,47 @@ public void onBlockPlace(BlockPlaceEvent event) { } this.scheduler.run(() -> { - location.getWorld().getBlockAt(location).setType(type); - location.getWorld().getBlockAt(location).setBlockData(data); - }); - - this.lockerManager.create(UUID.randomUUID(), description, PositionAdapter.convert(location), player.getUniqueId()) - .thenAccept(locker -> { - // The original BlockPlaceEvent was cancelled (so the item was never - // consumed) and the block was placed manually. Consume one locker item - // now so a single item cannot create unlimited lockers. - this.scheduler.run(() -> this.consumeLockerItem(player, parcelLockerItem)); - - this.noticeService.create() - .player(player.getUniqueId()) - .notice(messages -> messages.locker.created) - .send(); - - this.lockerCreators.invalidate(player.getUniqueId()); - }) - .exceptionally(ex -> { + // The original BlockPlaceEvent was cancelled, so the item was never consumed. + // Consume it up front; if the player no longer holds it, abort instead of + // creating a free locker. + if (!this.consumeLockerItem(player, parcelLockerItem)) { this.noticeService.create() .player(player.getUniqueId()) .notice(messages -> messages.locker.cannotCreate) .send(); this.lockerCreators.invalidate(player.getUniqueId()); - return null; - }); + return; + } + + location.getWorld().getBlockAt(location).setType(type); + location.getWorld().getBlockAt(location).setBlockData(data); + + this.lockerManager.create(UUID.randomUUID(), description, PositionAdapter.convert(location), player.getUniqueId()) + .thenAccept(locker -> { + this.noticeService.create() + .player(player.getUniqueId()) + .notice(messages -> messages.locker.created) + .send(); + + this.lockerCreators.invalidate(player.getUniqueId()); + }) + .exceptionally(ex -> { + // Creation failed after the item was consumed and the block placed: + // refund the item and clear the block so nothing is lost. + this.scheduler.run(() -> { + ItemStack refund = parcelLockerItem.clone(); + refund.setAmount(1); + ItemUtil.giveItem(player, refund); + location.getWorld().getBlockAt(location).setType(Material.AIR); + }); + this.noticeService.create() + .player(player.getUniqueId()) + .notice(messages -> messages.locker.cannotCreate) + .send(); + this.lockerCreators.invalidate(player.getUniqueId()); + return null; + }); + }); }); }, ClickCallback.Options.builder() .uses(1) @@ -173,18 +189,21 @@ public void onBlockPlace(BlockPlaceEvent event) { player.showDialog(dialog); } - private void consumeLockerItem(Player player, ItemStack lockerItem) { + private boolean consumeLockerItem(Player player, ItemStack lockerItem) { PlayerInventory inventory = player.getInventory(); ItemStack mainHand = inventory.getItemInMainHand(); - if (lockerItem.isSimilar(mainHand)) { + if (lockerItem.isSimilar(mainHand) && mainHand.getAmount() > 0) { mainHand.setAmount(mainHand.getAmount() - 1); - return; + return true; } ItemStack offHand = inventory.getItemInOffHand(); - if (lockerItem.isSimilar(offHand)) { + if (lockerItem.isSimilar(offHand) && offHand.getAmount() > 0) { offHand.setAmount(offHand.getAmount() - 1); + return true; } + + return false; } } diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java index dd5984bf..e43d6621 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java @@ -86,6 +86,9 @@ private CompletableFuture dispatchInternal(Player sender, Parcel parcel, L } return this.itemStorageManager.delete(sender.getUniqueId()) + // A failed delete must trigger the rollback, not skip straight to the outer + // exceptionally handler (which would leave the parcel sent and the fee charged). + .exceptionally(throwable -> false) .thenCompose(deleted -> { if (!Boolean.TRUE.equals(deleted)) { // The parcel and its content were already persisted and the fee charged, diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java index 91094f69..0db1523f 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java @@ -229,8 +229,22 @@ public CompletableFuture collect(Player player, Parcel parcel) { } this.parcelRepository.delete(parcel) - .thenCompose(deleted -> this.parcelContentRepository.delete(parcel.uuid()) - .thenApply(contentDeleted -> deleted && contentDeleted)) + .thenCompose(deleted -> { + if (!deleted) { + return CompletableFuture.completedFuture(false); + } + // The parcel is gone, so it can never be collected again; deleting its content + // is best-effort cleanup and must not block handing the items back. Otherwise a + // failed content delete would lose the items permanently. + return this.parcelContentRepository.delete(parcel.uuid()) + .handle((contentDeleted, throwable) -> { + if (throwable != null) { + this.server.getLogger().warning("Failed to delete content for collected parcel " + + parcel.uuid() + ": " + throwable.getMessage()); + } + return true; + }); + }) .thenAccept(removed -> { if (!removed) { this.noticeService.player(player.getUniqueId(), messages -> messages.parcel.databaseError); diff --git a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java index 4a1c7756..997d3ca2 100644 --- a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java @@ -123,16 +123,23 @@ public CompletableFuture create(UUID uuid, String name) { throw new ValidationException(conflictCheck.errorMessage()); } - User user = new User(uuid, name); - + return new User(uuid, name); + }).thenCompose(user -> { // Fire UserCreateEvent UserCreateEvent event = new UserCreateEvent(user); this.server.getPluginManager().callEvent(event); this.cache(user); - this.userRepository.save(user); - - return user; + // Chain the save so a persistence failure is surfaced to the caller; if it fails, undo + // the optimistic cache entry so the cache never holds an unpersisted user. + return this.userRepository.save(user) + .whenComplete((ignored, throwable) -> { + if (throwable != null) { + this.usersByUUID.invalidate(uuid); + this.usersByName.invalidate(name); + } + }) + .thenApply(ignored -> user); }); } From d6ec8a539587339cdb06d8cc201fb0ed2bfe8e9a Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 17:14:11 +0200 Subject: [PATCH 30/34] Address second review round (@claude): cache desyncs + UX - LockerManager.create: revert both cache maps if the save fails, so the cache never holds a locker absent from the DB (the same desync pattern fixed earlier for User/ItemStorage, missed here). - UserManagerImpl.changeName: fall back to the repository on a cache miss (no longer fails for an evicted-but-existing user), and revert the cache if the persist fails. Run via thenComposeAsync so the async event never fires on the main thread. - ParcelDispatchService/ParcelServiceImpl: issue the "sent" notice only after the whole dispatch succeeds, avoiding a "sent" immediately followed by "cannot send" on a storage-clear rollback. - LockerBreakController: explosion/fire/ignite/damage handlers now cancel synchronously via the locker cache (removing exploded locker blocks from the blast), instead of only restoring after the fact - closing the same drop/dupe window as C4 in those handlers. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01FyikQbSDrV12b3RTMJvd3S --- .../parcellockers/locker/LockerManager.java | 11 +++++- .../controller/LockerBreakController.java | 29 +++++++++++---- .../parcel/service/ParcelDispatchService.java | 3 ++ .../parcel/service/ParcelServiceImpl.java | 3 +- .../parcellockers/user/UserManagerImpl.java | 37 ++++++++++++------- 5 files changed, 60 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java b/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java index bf31e3dc..c04e1244 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java @@ -149,7 +149,16 @@ public CompletableFuture create(UUID uniqueId, String name, Position pos this.lockersByUUID.put(uniqueId, locker); this.lockersByPosition.put(position, locker); - return this.lockerRepository.save(locker); + // Undo the optimistic cache entries if the persist fails, so the cache never holds a + // locker that is not in the database (which would break the break/interaction fast paths + // and reject re-placement at the same position until the cache expires). + return this.lockerRepository.save(locker) + .whenComplete((saved, throwable) -> { + if (throwable != null) { + this.lockersByUUID.invalidate(uniqueId); + this.lockersByPosition.invalidate(position); + } + }); }).thenCompose(Function.identity()); } diff --git a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerBreakController.java b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerBreakController.java index cc3bfa95..89a944a6 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerBreakController.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerBreakController.java @@ -9,6 +9,7 @@ import com.eternalcode.parcellockers.shared.PositionAdapter; import java.util.Optional; import org.bukkit.Location; +import org.bukkit.event.Cancellable; import org.bukkit.Material; import org.bukkit.block.Block; import org.bukkit.block.data.BlockData; @@ -92,30 +93,44 @@ private void applyBreakRules(Player player, Position position, Locker locker, Bl @EventHandler public void onEntityExplode(EntityExplodeEvent event) { + // Remove cached locker blocks from the explosion synchronously so they are never destroyed + // (and never drop their vanilla chest item). + event.blockList().removeIf(block -> block.getType() == Material.CHEST + && this.lockerManager.getCached(PositionAdapter.convert(block.getLocation())).isPresent()); + + // Uncached lockers cannot be decided synchronously; restore them best-effort after the blast. for (Block block : event.blockList()) { - if (block.getType() != Material.CHEST) { - continue; + if (block.getType() == Material.CHEST) { + this.restoreIfUncachedLocker(block); } - this.handleDamagedLocker(block); } } @EventHandler public void onBlockIgnite(BlockIgniteEvent event) { - this.handleDamagedLocker(event.getBlock()); + this.protectFromDamage(event, event.getBlock()); } @EventHandler public void onBlockBurn(BlockBurnEvent event) { - this.handleDamagedLocker(event.getBlock()); + this.protectFromDamage(event, event.getBlock()); } @EventHandler public void onBlockDamage(BlockDamageEvent event) { - this.handleDamagedLocker(event.getBlock()); + this.protectFromDamage(event, event.getBlock()); + } + + private void protectFromDamage(Cancellable event, Block block) { + // Cancel synchronously when the locker is known, otherwise fall back to an async restore. + if (this.lockerManager.getCached(PositionAdapter.convert(block.getLocation())).isPresent()) { + event.setCancelled(true); + return; + } + this.restoreIfUncachedLocker(block); } - private void handleDamagedLocker(Block block) { + private void restoreIfUncachedLocker(Block block) { BlockData blockData = block.getBlockData(); Location location = block.getLocation(); Position position = PositionAdapter.convert(location); diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java index e43d6621..3dd92024 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java @@ -107,6 +107,9 @@ private CompletableFuture dispatchInternal(Player sender, Parcel parcel, L ); this.scheduler.runLaterAsync(task, delay); + // Only confirm success here, once every step has succeeded, to avoid a + // "sent" notice immediately followed by "cannot send" on a rollback. + this.noticeService.player(sender.getUniqueId(), messages -> messages.parcel.sent); return CompletableFuture.completedFuture(null); }); }); diff --git a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java index 0db1523f..d89b88f6 100644 --- a/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelServiceImpl.java @@ -123,7 +123,8 @@ public CompletableFuture send(Player sender, Parcel parcel, List this.parcelContentRepository.save(new ParcelContent(parcel.uuid(), itemsCopy)) .thenApply(contentSaved -> { this.parcelsByUuid.put(parcel.uuid(), parcel); - this.noticeService.player(sender.getUniqueId(), messages -> messages.parcel.sent); + // The "sent" notice is issued by the dispatcher once the whole send succeeds, so it + // is not shown when a later step (e.g. clearing storage) fails and rolls back. return true; }) .exceptionallyCompose(contentError -> this.parcelRepository.delete(parcel.uuid()) diff --git a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java index 997d3ca2..5dc85ea6 100644 --- a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java @@ -145,27 +145,36 @@ public CompletableFuture create(UUID uuid, String name) { @Override public CompletableFuture changeName(UUID uuid, String newName) { - return CompletableFuture.supplyAsync(() -> { - User oldUser = this.usersByUUID.getIfPresent(uuid); - - if (oldUser == null) { - throw new ValidationException("User not found with UUID: " + uuid); - } - + User cached = this.usersByUUID.getIfPresent(uuid); + CompletableFuture> lookup = cached != null + ? CompletableFuture.completedFuture(Optional.of(cached)) + : this.userRepository.fetch(uuid); + + // thenComposeAsync keeps the body (including the async UserChangeNameEvent) off the main thread + // even on a cache hit, where the lookup completes synchronously. + return lookup.thenComposeAsync(optional -> { + User oldUser = optional.orElseThrow( + () -> new ValidationException("User not found with UUID: " + uuid)); String oldName = oldUser.name(); - + // Fire UserChangeNameEvent UserChangeNameEvent event = new UserChangeNameEvent(oldUser, oldName); this.server.getPluginManager().callEvent(event); - - // Update cache + + // Optimistically update the cache, reverting if the persist fails. User updatedUser = new User(uuid, newName); this.usersByUUID.put(uuid, updatedUser); this.usersByName.invalidate(oldName); this.usersByName.put(newName, updatedUser); - - // Update in repository - return this.userRepository.changeName(uuid, newName); - }).thenCompose(future -> future); + + return this.userRepository.changeName(uuid, newName) + .whenComplete((ignored, throwable) -> { + if (throwable != null) { + this.usersByUUID.put(uuid, oldUser); + this.usersByName.invalidate(newName); + this.usersByName.put(oldName, oldUser); + } + }); + }); } } From d5a200fcac96108f28c9c0b1e74387c41432f21e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20K=C4=99dziora?= <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 19:38:45 +0200 Subject: [PATCH 31/34] Update src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java --- .../java/com/eternalcode/parcellockers/user/UserManagerImpl.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java index 5dc85ea6..2837f12f 100644 --- a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java @@ -125,7 +125,6 @@ public CompletableFuture create(UUID uuid, String name) { return new User(uuid, name); }).thenCompose(user -> { - // Fire UserCreateEvent UserCreateEvent event = new UserCreateEvent(user); this.server.getPluginManager().callEvent(event); From 0d5463edf665a7b961055c0be607f806d20061fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20K=C4=99dziora?= <77227023+Jakubk15@users.noreply.github.com> Date: Sat, 20 Jun 2026 19:38:50 +0200 Subject: [PATCH 32/34] Update src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java --- .../java/com/eternalcode/parcellockers/user/UserManagerImpl.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java index 2837f12f..e6ce13a9 100644 --- a/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java +++ b/src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java @@ -156,7 +156,6 @@ public CompletableFuture changeName(UUID uuid, String newName) { () -> new ValidationException("User not found with UUID: " + uuid)); String oldName = oldUser.name(); - // Fire UserChangeNameEvent UserChangeNameEvent event = new UserChangeNameEvent(oldUser, oldName); this.server.getPluginManager().callEvent(event); From 83b6bdda98a2e170233ce971ee5774991c1705e3 Mon Sep 17 00:00:00 2001 From: Jakubk15 <77227023+Jakubk15@users.noreply.github.com> Date: Sun, 21 Jun 2026 09:15:56 +0200 Subject: [PATCH 33/34] Update build configuration and enhance locker item consumption logic --- build.gradle.kts | 6 +++--- .../locker/controller/LockerPlaceController.java | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index 0098058e..02e51118 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -133,7 +133,7 @@ tasks.withType { tasks.withType { javaLauncher = javaToolchains.launcherFor { vendor = JvmVendorSpec.JETBRAINS - languageVersion = JavaLanguageVersion.of(21) + languageVersion = JavaLanguageVersion.of(25) } } @@ -150,11 +150,11 @@ modrinth { tasks { runServer { - minecraftVersion("1.21.11") + minecraftVersion("26.1.2") downloadPlugins { modrinth("luckperms", "v5.5.17-bukkit") modrinth("vaultunlocked", "2.17.0") - modrinth("essentialsx", "2.21.2") + modrinth("essentialsx", "2.22.0") // modrinth("discordsrv", "1.30.4") // uncomment to test with DiscordSRV integration } jvmArgs("-Dcom.mojang.eula.agree=true", "-XX:+AllowEnhancedClassRedefinition") diff --git a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java index 7e329e86..b046a425 100644 --- a/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java +++ b/src/main/java/com/eternalcode/parcellockers/locker/controller/LockerPlaceController.java @@ -23,6 +23,7 @@ import net.kyori.adventure.text.Component; import net.kyori.adventure.text.event.ClickCallback; import net.kyori.adventure.text.minimessage.MiniMessage; +import org.bukkit.GameMode; import org.bukkit.Location; import org.bukkit.Material; import org.bukkit.block.Block; @@ -190,6 +191,10 @@ public void onBlockPlace(BlockPlaceEvent event) { } private boolean consumeLockerItem(Player player, ItemStack lockerItem) { + if (player.getGameMode() == GameMode.CREATIVE) { + return true; + } + PlayerInventory inventory = player.getInventory(); ItemStack mainHand = inventory.getItemInMainHand(); From cfcc33faac94d8a2b9037ee35561592e879d1f48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20K=C4=99dziora?= <77227023+Jakubk15@users.noreply.github.com> Date: Sun, 21 Jun 2026 09:16:38 +0200 Subject: [PATCH 34/34] Delete AUDIT.md --- AUDIT.md | 342 ------------------------------------------------------- 1 file changed, 342 deletions(-) delete mode 100644 AUDIT.md diff --git a/AUDIT.md b/AUDIT.md deleted file mode 100644 index dd192fce..00000000 --- a/AUDIT.md +++ /dev/null @@ -1,342 +0,0 @@ -# ParcelLockers โ€” Code Audit - -Audit of `src/` covering duplication, race conditions, optimization opportunities, -and database / unintended-behavior bugs. - -Scope: 134 Java source files. Findings are grouped by severity. Each item lists the -file and the relevant lines so they can be jumped to directly. - -Severity legend: **๐Ÿ”ด Critical** (item/money loss, duplication exploits) ยท -**๐ŸŸ  High** (data-integrity races, broken correctness) ยท **๐ŸŸก Medium** (latent bugs, -inconsistency) ยท **๐Ÿ”ต Low** (style, minor optimization). - ---- - -## Resolution status (`audit-fixes` branch) - -All findings below are fixed on the `audit-fixes` branch, one commit each, except where noted: - -- **Critical:** C1, C2, C3, C4 โ€” fixed. -- **High:** H1, H2, H3, H4, H5, H6 โ€” fixed. -- **Medium:** M1โ€“M8 โ€” fixed. -- **Low:** L1, L3, L4, L5 โ€” fixed. **L2** was folded into the H2 fix (the redundant - `count > 0` clause was removed there). -- **Duplication:** D1, D2, D4, D5 โ€” fixed (D1 also fixed the same swallowed table-creation - bug in the Delivery and User repos; D2 also fixed a latent always-false "has next page" in - `UserRepository.fetchPage`). **D3** (generic cache-or-fetch helper) was intentionally left: - the five managers use their caches differently enough that one abstraction would risk - masking per-cache nuance, and the concrete inconsistency it could hide was already fixed by H6. - -`./gradlew compileJava` and `./gradlew test` pass on the branch. - ---- - -## ๐Ÿ”ด Critical - -### C1 โ€” Economy fee is charged before persistence and never refunded on failure -`parcel/service/ParcelServiceImpl.java:102` (withdraw) vs `:122-138` (save + rollback) - -`send()` withdraws the fee at line 102, then saves the parcel/content. If -`parcelRepository.save(...)` or content save fails, the `exceptionally`/rollback -branches delete the parcel and notify the player, **but never refund the money**. -The player is charged for a parcel that was never created. - -Fix: only withdraw after a successful save, or `depositPlayer` in every failure path -(content-save failure at `:129`, parcel-save failure at `:135`). - -### C2 โ€” Dispatch rollback loses the fee and orphans parcel content -`parcel/service/ParcelDispatchService.java:68-74` - -After `parcelService.send(...)` succeeds (parcel **and** content persisted, fee -charged), `itemStorageManager.delete(...)` is awaited. If it returns `false`, the code -calls `parcelService.delete(parcel.uuid())` โ€” which deletes only the **parcel row**, not -the `ParcelContent` row saved in `send()`. Result: orphaned content in the DB, the -sender's item-storage still present, and the fee is not refunded. - -Fix: refund on this path and also delete the parcel content; ideally make -"charge โ†’ save parcel โ†’ save content โ†’ clear storage โ†’ schedule" a single rollback unit. - -### C3 โ€” Locker can be duplicated: place is cancelled but the item is never consumed -`locker/controller/LockerPlaceController.java:80, 126-129` - -`onBlockPlace` calls `event.setCancelled(true)` (so vanilla never consumes the item from -the hand), then on dialog confirm it manually re-places the block via -`setType`/`setBlockData`. The locker item is **never removed from the player's -inventory**, so a single locker item creates unlimited lockers. - -Fix: decrement the used item stack by one (main hand or off hand, matching the -`isSimilar` check at `:72`) when the locker is actually created. - -### C4 โ€” Breaking a locker is never cancelled โ†’ chest + contents drop, then block is restored -`locker/controller/LockerBreakController.java:38-78` - -`onBlockBreak` never calls `event.setCancelled(true)`. For a non-admin player the block -is physically broken first (the chest item, and in survival its drops, spawn into the -world), and only on the next tick is the block restored via `setType`/`setBlockData` -(`:53-54`). The already-spawned drops remain โ†’ free chests / item duplication. For the -admin path the block is also left in vanilla-broken state before `delete` runs. - -Fix: cancel the `BlockBreakEvent` for non-admins immediately and return; for admins, -cancel and then run the managed delete + broadcast. - ---- - -## ๐ŸŸ  High - -### H1 โ€” `collect()` deletes the parcel before giving items; inventory check is a TOCTOU -`parcel/service/ParcelServiceImpl.java:171-215` - -`freeSlotsInInventory(player)` is evaluated on the async DB thread (`:191`). The parcel -and content rows are then deleted (`:196-197`), and only afterwards are items handed back -on the main thread, one scheduler task per item (`:204`). Between the slot check and the -delivery the inventory can change, and items are given **after** the only copy in the DB -is destroyed. Any failure (full inventory, server stop, exception in `giveItem`) loses the -items permanently. - -Fix: give items first (and let `ItemUtil.giveItem` drop overflow), then delete only after -delivery is confirmed; re-check free slots on the main thread. - -### H2 โ€” Locker fullness check is a TOCTOU race -`parcel/service/ParcelDispatchService.java:50-61` + `locker/LockerManager.java:176-178` - -`isLockerFull` runs a `COUNT(*)` and `dispatch` proceeds to `send` based on the result. -Two players sending to the same near-full locker concurrently can both pass the check and -both store, exceeding `maxParcelsPerLocker`. There is no atomic guard at the DB level. - -Fix: enforce the cap inside a single transaction, or re-check + compensate after insert. - -### H3 โ€” `LockerManager.get(Page)` returns wrong/incomplete pages from the cache -`locker/LockerManager.java:96-103` - -```java -List cached = List.copyOf(this.lockersByUUID.asMap().values()); -boolean hasNextPage = cached.size() > page.getLimit(); -if (!cached.isEmpty() && page.getOffset() == 0 && !hasNextPage) { - return CompletableFuture.completedFuture(new PageResult<>(cached, false)); -} -``` - -The cache holds an arbitrary, partially-evicted, unordered subset of lockers โ€” it is **not** -the full dataset. When fewer than `limit` lockers happen to be cached, page 0 returns just -those, hiding every locker that isn't currently cached (and in non-deterministic order). -This silently shows users an incomplete locker list. - -Fix: don't serve pagination from a partial cache; query the repository (or maintain a -separate authoritative ordered index). - -### H4 โ€” Locker GUI opens on top of the vanilla chest (cancel happens too late) -`locker/controller/LockerInteractionController.java:41-51` - -The DB lookup is async; `event.setCancelled(true)` is executed inside -`scheduler.run(...)` on a later tick โ€” long after the `PlayerInteractEvent` has finished -and the vanilla chest inventory has already opened. The player gets both the vanilla chest -and the locker GUI, and the cancel is a no-op. - -Fix: resolve locker membership synchronously where possible (e.g. position cache lookup), -cancel the event in the same tick, then open the GUI. - -### H5 โ€” `getOrCreate` / `create` only check the cache for conflicts, not the DB -`user/UserManagerImpl.java:65-117`, and the same pattern in -`locker/LockerManager.java:105-138` - -`getOrCreate` (`:65`) returns from cache or calls `create`. `create` validates "no -conflict" using **cache-only** lookups (`usersByUUID.get(uuid, k -> null)` at `:95-96`), -so an entity that exists in the DB but not in the cache passes validation and a second -"create" is attempted. Combined with `get(...)` not populating the cache on a miss (see -H6), this is reachable in normal operation. - -Fix: consult the repository in the conflict check, or rely on a DB unique constraint and -handle the violation. - -### H6 โ€” `UserManagerImpl.get(...)` never populates the cache on a miss -`user/UserManagerImpl.java:42-62` - -Both `get(UUID)` and `get(String)` return straight from the repository on a cache miss -without putting the result into `usersByUUID` / `usersByName`. Every lookup for an -uncached user is a DB round-trip, and the cache only ever fills via `create`/`changeName`. -This also feeds H5. (Contrast with `LockerManager.get`, `DeliveryManager.get`, -`ItemStorageManager.get`, which all cache on read.) - -Fix: cache the fetched user (by UUID and name) before returning. - ---- - -## ๐ŸŸก Medium - -### M1 โ€” `ParcelSendTask` updates status and deletes the delivery as independent fire-and-forget calls -`parcel/task/ParcelSendTask.java:54-64` - -The "mark DELIVERED" update (`:54`) and the "delete delivery" call (`:60`) are not -chained. If the update fails but the delete succeeds, the parcel is stuck in `SENT` -forever with no delivery row, so it is never re-scheduled on restart -(`ParcelLockers.java:215-228` only reschedules parcels that still have a delivery) and -never becomes collectable. - -Fix: chain โ€” delete the delivery only after the status update completes successfully. - -### M2 โ€” Async-only Bukkit events are fired from mixed threads (latent `IllegalStateException`) -`itemstorage/ItemStorageManager.java:51-66`, `user/UserManagerImpl.java:87-117,120-143`, -`locker/LockerManager.java:105-138` - -`ItemStorageUpdateEvent`, `UserCreateEvent`, `UserChangeNameEvent`, `LockerCreateEvent`, -`ParcelSendEvent`, `ParcelDeliverEvent` are all constructed with `super(true)` -(asynchronous), while `ParcelCollectEvent`, `LockerDeleteEvent`, `UserChangeNameEvent`'s -counterparts use the default sync constructor. Paper throws if an async event is fired -from the primary thread (or a sync event from an async thread). Whether each `callEvent` -is legal currently depends entirely on the calling thread happening to match the flag โ€” -e.g. `ItemStorageManager.getOrCreate` (`:47`) runs its loader synchronously on the caller -thread, and if ever invoked on the main thread it will throw when firing the async -`ItemStorageUpdateEvent`. - -Fix: pick one threading model per event deliberately, and document/guarantee the firing -thread; don't let it depend on the call path. - -### M3 โ€” `ItemStorageManager.getOrCreate` mutates the cache from inside a cache-loader for the same key -`itemstorage/ItemStorageManager.java:47-66` - -`getOrCreate` calls `cache.get(owner, key -> this.create(key, items))`, and `create` -itself calls `this.cache.put(owner, ...)` (`:63`) for the **same key** that is currently -being loaded. Recursive computation/mutation of the same key inside a Caffeine mapping -function is explicitly discouraged and can throw `IllegalStateException` / corrupt the -entry. - -Fix: have the loader return the value and let `cache.get` store it; don't `put` inside the -loader. - -### M4 โ€” Item-storage save/delete failures are silently swallowed (possible item loss) -`gui/implementation/locker/ItemStorageGui.java:106-117`, `gui/GuiManager.java:90-92`, -`itemstorage/ItemStorageManager.java:64` - -On GUI close, items are read from the inventory, the storage row is deleted, then -`saveItemStorage` is called โ€” but `create()`'s `itemStorageRepository.save(...)` return is -ignored (fire-and-forget at `ItemStorageManager.java:64`), and `saveItemStorage` returns -`void`. If the save fails after the delete succeeded, the player's staged items are gone -with no error surfaced. - -Fix: chain delete โ†’ save, propagate failure, and re-give items on failure. - -### M5 โ€” `ItemStorageRepositoryOrmLite` swallows table-creation failure -`itemstorage/repository/ItemStorageRepositoryOrmLite.java:19-23` - -```java -} catch (SQLException ex) { - ex.printStackTrace(); -} -``` - -Every other repository (`ParcelRepositoryOrmLite:34`, `LockerRepositoryOrmLite:26`, -`ParcelContentRepositoryOrmLite:21`) throws `DatabaseException` on a failed -`createTableIfNotExists`. Here it prints and continues, so a missing table only surfaces -later as confusing query errors. - -Fix: throw `DatabaseException` for consistency and fail-fast startup. - -### M6 โ€” Confusing repository `save`/`update` semantics -`parcel/repository/ParcelRepositoryOrmLite.java:40-49` + -`database/wrapper/AbstractRepositoryOrmLite.java:27-33` - -`save(Parcel)` maps to `saveIfNotExist` โ†’ `dao.createIfNotExists` (a no-op if the row -exists), while `update(Parcel)` maps to the base `save` โ†’ `dao.createOrUpdate`. The -method named `save` cannot persist changes to an existing parcel, and the wrapper method -named `save` actually performs an upsert. This inversion is an easy source of "my update -didn't persist" bugs. - -Fix: rename wrapper methods to `upsert`/`insertIfAbsent` and align the repository method -names with their real behavior. - -### M7 โ€” `DatabaseManager.getDao` check-then-put is not atomic -`database/DatabaseManager.java:99-112` - -`cachedDao.get` followed by `cachedDao.put` on a `ConcurrentHashMap` can run twice -concurrently and create/replace the DAO twice. Harmless today (ORMLite caches DAOs -internally) but it defeats the point of the cache. - -Fix: `cachedDao.computeIfAbsent(type, t -> DaoManager.createDao(...))`. - -### M8 โ€” Aggressive/duplicated HikariCP tuning -`database/DatabaseManager.java:47-49` - -`leakDetectionThreshold` (5000 ms) equals `connectionTimeout` (5000 ms), and the pool is -hard-capped at 5 connections regardless of DB type or server size. Any legitimately slow -query will log a false "connection leak" warning. None of these are configurable. - -Fix: raise/relax the leak threshold, make pool size configurable, and consider a larger -default for networked DBs. - ---- - -## ๐Ÿ”ต Low / Optimization - -- **L1 โ€” Redundant enum round-trip.** `database/DatabaseManager.java:53` - `DatabaseType.valueOf(databaseType.toString().toUpperCase())` โ€” `databaseType` is already - a `DatabaseType`; switch on it directly. -- **L2 โ€” Redundant condition.** `locker/LockerManager.java:178` - `count > 0 && count >= maxParcelsPerLocker` โ€” the first clause is redundant unless - `maxParcelsPerLocker <= 0`; clarify the intended zero/negative behavior. -- **L3 โ€” `onDisable` ordering.** `ParcelLockers.java:231-244` closes the datasource before - unregistering anything; in-flight async DB tasks (scheduler pool) will then fail. Consider - draining/awaiting pending writes before `disconnect()`. -- **L4 โ€” Stale "TODO"/placeholder comments.** `locker/controller/LockerPlaceController.java:92` - ("Replace with actual config message" โ€” but the config value *is* used) and hardcoded - English dialog strings throughout `LockerPlaceController` and `SendingGui` - (`:95-160`) bypass `MessageConfig`, contradicting the "all user-facing text goes through - MessageConfig" architecture note. -- **L5 โ€” `freeSlotsInInventory` ignores stacking.** `util/InventoryUtil.java:12-20` counts - only fully empty slots, so `collect()` can wrongly reject a parcel whose items would stack - into partially-filled slots (or accept-then-drop). Account for partial stacks if precise. -- **L6 โ€” `CollectionGui` removes the item from the GUI before collection is confirmed.** - `gui/implementation/locker/CollectionGui.java:142-145` calls `collectParcel` (async) and - `refresher.removeItemBySlot` immediately; on a failed collect the UI desyncs from reality. - (The double-collect dupe is prevented by the `delete`-returns-false guard in `collect`, but - the visual desync remains.) - ---- - -## Duplication - -The codebase is generally well-layered, but several patterns are copy-pasted across every -domain and are worth extracting: - -1. **Repository table bootstrap.** The `try { TableUtils.createTableIfNotExists(...) } - catch { throw new DatabaseException(...) }` block is duplicated in every `*OrmLite` - constructor (parcel, locker, content, itemstorage, delivery, user). Pull into - `AbstractRepositoryOrmLite` as a protected helper (and fix M5 in the process). - -2. **Pagination logic.** `ParcelRepositoryOrmLite.findByPaged` (`:108-128`) and - `LockerRepositoryOrmLite.findPage` (`:61-78`) implement the identical "limit+1, remove - last, compute hasNext" pattern (and `UserRepository.fetchPage` likely a third copy). - Extract a shared paged-query helper. - -3. **Manager cache-or-fetch.** The "check cache โ†’ on miss fetch from repo โ†’ populate cache" - block is reimplemented in `LockerManager`, `DeliveryManager`, `ItemStorageManager`, - `UserManagerImpl`, and `ParcelServiceImpl` โ€” with subtle inconsistencies (UserManager - forgets to populate; H6). A small generic cached-loader would unify behavior. - -4. **Bukkit event boilerplate.** Every event class (`ParcelSendEvent`, `ParcelCollectEvent`, - `LockerCreateEvent`, โ€ฆ) repeats the `HandlerList` / `getHandlers` / `isCancelled` / - `setCancelled` block. A shared abstract `CancellableEvent` base removes ~30 lines each and - would make the async-flag decision (M2) explicit in one place. - -5. **`createActiveItem` overloads.** `SendingGui.java:419-437` has two near-identical - methods differing only in `add` vs `addAll`; collapse to one taking a `List`. - ---- - -## Summary - -| Area | Count | Highest severity | -|---|---|---| -| Item/money loss & duplication | C1โ€“C4 | ๐Ÿ”ด Critical | -| Concurrency / data-integrity races | H1โ€“H6, M1, M3 | ๐ŸŸ  High | -| Correctness & consistency | M2, M4โ€“M8 | ๐ŸŸก Medium | -| Style / minor optimization | L1โ€“L6 | ๐Ÿ”ต Low | -| Duplication | 5 patterns | โ€” | - -The most urgent work is the parcel-send/collect money-and-item flows (C1, C2, H1) and the -two block-controller exploits (C3, C4), all of which are reachable in normal gameplay and -cause real loss or duplication. The async-event threading model (M2) is a latent crash that -should be made deliberate rather than incidental. - -> Note: this is a static review; none of the findings were reproduced at runtime. Validate -> the critical items (especially C3/C4 duplication and C1/C2 refunds) with targeted tests -> before and after fixing. \ No newline at end of file