Audit fixes: dupe/economy exploits, race conditions, DB bugs, cleanups

[Jakubk15]

Full audit of src/ and fixes for every finding, one commit each. See AUDIT.md for the detailed report and resolution status. ./gradlew compileJava and ./gradlew test pass.

🔴 Critical

• C1 Refund the parcel fee when persistence fails (was charged-then-lost).
• C2 rollbackSend deletes the parcel and its content and refunds the fee, instead of orphaning content + losing money.
• C3 Consume the locker item on creation (the cancelled place left the item in hand → unlimited lockers).
• C4 Cancel the locker break in-tick via a synchronous cache lookup (the late async restore allowed chest/contents duplication).

🟠 High

• H1 Collection re-checks inventory space on the main thread and deletes before handing items back safely.
• H2 Close the locker-fullness TOCTOU: occupancy counts in-transit parcels and dispatches are serialized per destination locker.
• H3 Page lockers from the repository instead of an arbitrary partial cache.
• H4 Cancel the chest interaction in the same tick so the vanilla chest no longer opens under the locker GUI.
• H5/H6 User conflict checks consult the database; read misses now populate the cache.

🟡 Medium

M1 (chain delivery delete after status update), M2 (deterministic event-firing threads — fixes two real IllegalStateException paths), M3 (no reentrant cache loader), M4 (surface item-storage save failures, re-give items), M5 (fail fast on table create), M6 (rename upsert/insertIfAbsent), M7 (atomic DAO cache), M8 (configurable connection-pool tuning).

🔵 Low

L1, L3, L4, L5. (L2 was folded into the H2 fix.)

Duplication

• D1 Shared createTable helper — also fixes the same swallowed table-creation failure in the Delivery and User repos.
• D2 Shared queryPage helper — also fixes a latent always-false "has next page" in UserRepository.fetchPage.
• D4 CancellableEvent base for shared cancel handling.
• D5 Collapse the duplicate createActiveItem overloads.

Intentionally deferred: D3 (generic cache-or-fetch helper) — the managers use their caches differently enough that one abstraction risks masking per-cache nuance, and the concrete inconsistency it could hide was already fixed by H6.

Note: this is a static review; the Critical items (esp. C3/C4 duplication and C1/C2 refunds) should be validated with targeted in-game tests before merge.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request implements comprehensive fixes for critical and high-severity issues identified in a code audit of the ParcelLockers service. Key changes include resolving TOCTOU races in locker fullness checks, preventing item duplication exploits during locker placement and breaking, ensuring proper fee refunds on send failures, and refactoring database repositories to reduce duplication. The review feedback highlights several crucial edge cases where asynchronous database operations or failures could lead to cache-DB desyncs, item duplication, or permanent item loss. Specifically, the reviewer recommends invalidating or reverting caches if saves fail in ItemStorageManager and UserManagerImpl, handling exceptional failures during item storage deletion in ParcelDispatchService to guarantee rollbacks, ensuring items are still delivered even if content deletion fails during collection, and verifying item consumption in LockerPlaceController.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[Jakubk15]

@claude review

[claude]

Claude finished @Jakubk15's task in 6m 4s —— View job

---

Code Review

• Gather PR context
• Review critical fixes (C1–C4)
• Review high-severity fixes (H1–H6)
• Review Gemini follow-up fixes (commit f859387)
• Review medium/low/duplication fixes
• Post final review

---

Summary

Overall, this is a thorough and well-structured audit with correct fixes for the most dangerous issues. The Gemini follow-up (f859387) addressed all five flagged regressions properly. A few issues remain — one medium-severity cache-DB desync that was missed, and a couple of minor problems in changeName.

---

What's Solid

C1 — Fee refund on persistence failure (ParcelServiceImpl:122–137): refundableFee is correctly captured before the async chain so the value is stable. The exceptionally handler is positioned to cover both the parcel-save and content-save failure paths.

C2 — rollbackSend (ParcelServiceImpl:141–153): Correctly deletes both the parcel row and its content, and refunds the fee via the bypass-permission check.

C3 — Item consumption (LockerPlaceController:131–167): The boolean-returning consumeLockerItem abort-before-create pattern is correct. The item-refund-on-failed-creation path (exceptionally block) cleans up both the consumed item and the placed block.

C4 / H4 — Synchronous cancel via cache (LockerBreakController:49–54, LockerInteractionController:47–53): Fast-path cancellation in the same tick is the right fix. The acknowledged slow-path limitation (cache miss → block breaks before async result arrives) is inherent to the architecture and acceptable given that the cache is warm for all known lockers.

H1 — Collection TOCTOU (ParcelServiceImpl:224–266): Main-thread re-check + delete-before-give is correct. Content deletion is correctly treated as best-effort once the parcel row is deleted (Gemini's fix properly landed here).

H2 — Locker fullness serialization (ParcelDispatchService:58–66): The per-locker chain via ConcurrentHashMap.compute() is correct and race-free. The conditional remove(lockerId, chained) safely handles concurrent dispatches without prematurely evicting a newer chain tail.

H3 — Page from repository (LockerManager:108–119): Cache-only pagination was correctly moved to repository.

H5/H6 — User conflict checks (UserManagerImpl:91–99, 117–127): DB-first validation with cache-miss population on getOrCreate, and the Gemini-flagged optimistic-cache rollback on failed save in create, are both correct.

AbstractRepositoryOrmLite.queryPage: The +1 fetch / removeLast hasNext logic is clean and avoids the always-false bug it replaces in UserRepository.

Gemini's 5 flagged items (f859387): ItemStorageManager.create cache revert, consumeLockerItem boolean return, parcelContentRepository best-effort delete, UserManagerImpl.create cache invalidate on failure, itemStorageManager.delete().exceptionally(t -> false) — all correctly applied.

---

Issues Found

Medium — LockerManager.create() cache not reverted on save failure

File: src/main/java/com/eternalcode/parcellockers/locker/LockerManager.java:149–153

this.lockersByUUID.put(uniqueId, locker);
this.lockersByPosition.put(position, locker);

return this.lockerRepository.save(locker);   // ← if this fails...

If lockerRepository.save() fails, both cache maps hold a locker entry that was never persisted. The caller in LockerPlaceController refunds the item and clears the block, but does not clear the cache. Consequences:

• LockerBreakController fast path sees the phantom entry and tries to delete a non-existent locker
• LockerInteractionController fast path opens the GUI for a position with no DB record
• A subsequent attempt to place a locker at the same position hits the conflict check (lockersByPosition cache) and is rejected with "already exists" until the cache expires (2 h)

This is the same pattern Gemini caught in UserManagerImpl and ItemStorageManager — but it was missed here. The fix mirrors those cases:

this.lockersByUUID.put(uniqueId, locker);
this.lockersByPosition.put(position, locker);

return this.lockerRepository.save(locker)
    .whenComplete((saved, throwable) -> {
        if (throwable != null) {
            this.lockersByUUID.invalidate(uniqueId);
            this.lockersByPosition.invalidate(position);
        }
    });

Fix this →

---

Minor — UserManagerImpl.changeName() only consults the cache

File: src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java:149–155

User oldUser = this.usersByUUID.getIfPresent(uuid);
if (oldUser == null) {
    throw new ValidationException("User not found with UUID: " + uuid);
}

If the cache has evicted the user (e.g. 2 h idle), changeName fails with ValidationException even though the user exists in the database. The fix is to fall back to userRepository.fetch(uuid) on a cache miss, consistent with how get(UUID) handles it.

---

Minor — UserManagerImpl.changeName() cache updated before DB save succeeds

File: src/main/java/com/eternalcode/parcellockers/user/UserManagerImpl.java:162–167

User updatedUser = new User(uuid, newName);
this.usersByUUID.put(uuid, updatedUser);        // ← cache updated first
this.usersByName.invalidate(oldName);
this.usersByName.put(newName, updatedUser);

return this.userRepository.changeName(uuid, newName); // ← if this fails...

If userRepository.changeName() fails, the cache holds the new name but the database still has the old name — a desync that persists until the entry expires or the server restarts. Same pattern as UserManagerImpl.create() pre-fix. Add a whenComplete rollback that reinstates oldName in both caches on failure.

---

Minor UX — Double "cannotSend" notification on storage-delete failure

File: src/main/java/com/eternalcode/parcellockers/parcel/service/ParcelDispatchService.java:97–98

When itemStorageManager.delete() fails:

1. parcelService.send() has already sent "Parcel sent!" (at ParcelServiceImpl:126)
2. dispatchInternal then sends "Cannot send parcel." before calling rollbackSend

The player sees a success message immediately followed by a failure message for the same transaction. This is confusing even though the rollback is correct. Consider suppressing the "sent" notification inside send() and only issuing it once the storage delete has confirmed success — or at minimum document this as a known edge case.

---

Nit

LockerBreakController.handleDamagedLocker() (fire/explosion handlers) also restores the block asynchronously and doesn't cancel the explosion event, which can drop a vanilla chest item from the locker position before the restore. This pre-dates the PR and is the same slow-path issue as C4, just in a different handler — worth a follow-up.

---

Note: ./gradlew compileJava and ./gradlew test pass per the PR description. The Critical/High items should still receive targeted in-game testing as flagged in the PR body before merge.

[Jakubk15]

Addressed the second review round in d6ec8a539587339cdb06d8cc201fb0ed2bfe8e9a. All five items were valid:

• Medium — LockerManager.create cache desync: correct, same pattern missed here. Now reverts both lockersByUUID/lockersByPosition via whenComplete if the save fails, so the break/interaction fast paths and the re-placement conflict check never see a phantom locker.
• Minor — changeName cache-only lookup: fixed — falls back to userRepository.fetch(uuid) on a cache miss, consistent with get(UUID).
• Minor — changeName cache-before-save: fixed — reverts both caches (old name/user) via whenComplete if userRepository.changeName fails. Used thenComposeAsync so the async UserChangeNameEvent still fires off the main thread on a cache hit.
• Minor UX — double notification: fixed — moved the "sent" notice out of ParcelServiceImpl.send into the dispatcher's success branch, so it only fires once the whole send (incl. storage clear) succeeds.
• Nit — handleDamagedLocker explosion/fire handlers: fixed — explosion now removes cached locker blocks from blockList() synchronously, and ignite/burn/damage cancel in-tick via getCached, with the async restore kept as the uncached fallback (mirrors the C4 fix).

./gradlew compileJava and ./gradlew test pass.