fix: cancel orphaned NewbieTask before overwrite to prevent memory leak

[xBuhari]

🐛 Memory Leak Fix: Orphaned NewbieTask accumulation

Root Cause

Heap dump analysis (Eclipse MAT) revealed 5,312 NewbieTask instances accumulating
in the ScheduledThreadPoolExecutor queue, retaining ~4.85 GB of heap (26% of total).

setNewbie(true) was creating a new NewbieTask without cancelling the previous one.
The orphaned task remained in the executor queue (potentially for hours) while holding
a strong reference chain:

NewbieTask → CombatPlayer → BasePlayer.player (CraftPlayer)

Easy trigger: running /newbie add on a player already under newbie protection,
since there was no isNewbie() guard before calling setNewbie(true).

---

Fix

• CombatPlayer.setNewbie() — Cancel existing newbieTask before overwriting the reference
• Newbie.java (AddNewbieCommand) — Guard with isNewbie() check to prevent repeated scheduling
• CombatPlayer.cleanForRemoval() — Null out newbieTask after cancel to release the reference immediately

---

Description

Memory leak fix for orphaned NewbieTask instances causing excessive heap retention.
No behavioral changes to newbie protection logic.

• I tested these changes or am certain they have no issues
• I accept that my changes may also be merged into the premium version of PvPManager

[deepsource-io]

DeepSource Code Review

We reviewed changes in e554441...03aa8c7 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

PR Report Card

Overall Grade

Security   Reliability   Complexity   Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
Java		May 18, 2026 10:42a.m.	Review ↗

---

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

[xBuhari]

PvPManager — Heap Dump Analysis

Source: Eclipse Memory Analyzer leak report (heapdump.hprof)
Captured: 18 May 2026, 09:38 GMT+3
Process heap (used): 17.2 GB · 352,470,433 objects
JVM: OpenJDK 21.0.10 (64-bit, Ubuntu 22.04)

This report covers only PvPManager (me.chancesd.pvpmanager). Other plugins are out of scope.

---

Summary

PvPManager shows up as a major retainer in Leak Suspect 2 (orphaned CraftPlayer instances). The shortest GC path runs through a ScheduledThreadPoolExecutor work queue: pending NewbieTask runnables still reference CombatPlayer wrappers, which in turn hold CraftPlayer (and underlying ServerPlayer) objects.

Rough scale:

Item	Count
CombatPlayer on main retention path	5,312
NewbieTask (scheduled, not collected)	5,312
CraftPlayer reachable via that path	5,499
Extra CombatPlayer groups (other paths)	92 + 53

Using the suspect set average (~720 KB retained per CraftPlayer), the PvPManager-linked player graph is on the order of ~3.6–3.8 GB of retained heap. That is about 79% of the 6,731 leaked CraftPlayer instances (4.85 GB total in this suspect).

This pattern matches newbie-protection timers not being cancelled when players disconnect, so tasks and CombatPlayer entries accumulate instead of being released.

---

Classes (PvPManager only)

Only three PvPManager types appear in the exported dominator / reference data:

Class	Role
me.chancesd.pvpmanager.player.CombatPlayer	Per-player state; holds Bukkit Player
me.chancesd.pvpmanager.tasks.NewbieTask	Scheduled newbie-protection work unit
me.chancesd.pvpmanager.sdutils.scheduler.ScheduleUtils$ExceptionRunnable	Wrapper submitted to the plugin scheduler

No other me.chancesd.pvpmanager.* types surfaced in the MAT HTML export (histogram/top-consumer pages did not list the package separately).

---

Retention path (main leak chain)

MAT Reference Pattern for the CraftPlayer suspect set:

java.lang.Thread (7)
  └─ ScheduledThreadPoolExecutor$DelayedWorkQueue (2)
       └─ RunnableScheduledFuture[] (2)
            └─ ScheduledThreadPoolExecutor$ScheduledFutureTask (5,312)
                 └─ Executors$RunnableAdapter (5,312)
                      └─ ScheduleUtils$ExceptionRunnable (5,312)
                           └─ NewbieTask (5,312)
                                └─ CombatPlayer (5,312)
                                     └─ CraftPlayer (5,312)  →  ServerPlayer / world graph

Interpretation: Thousands of delayed tasks are still sitting in the executor queue. Each task keeps its CombatPlayer alive; each CombatPlayer keeps a CraftPlayer that should have been collectable after the player left.

Secondary references (same dump, smaller branches):

• CombatPlayer × 92 → 175 referenced objects (~49 KB shallow)
• CombatPlayer × 53 → 83 referenced objects (~23 KB shallow)

---

Instance counts (reference pattern)

Class	Instances	Referenced objects	Ref. shallow heap
ScheduleUtils$ExceptionRunnable	5,312	5,499	1,539,720 B (~1.47 MB)
NewbieTask	5,312	5,499	1,539,720 B
CombatPlayer (main chain)	5,312	5,499	1,539,720 B
CombatPlayer (branch A)	92	175	49,000 B
CombatPlayer (branch B)	53	83	23,240 B

Shallow heap on the path is small; almost all impact is retained CraftPlayer / NMS player data (inventories, advancements, maps, etc.) visible in the full retained set for the suspect.

---

Threads

PvPManager threads in the dump (context class loader: java.net.URLClassLoader @ 0x7fb81c002600):

Thread	Shallow	Retained (approx.)
PvPManager Worker Thread - 0	168 B	111 KB
PvPManager Worker Thread - 4	168 B	85 KB
PvPManager Worker Thread - 1	168 B	84 KB
PvPManager Worker Thread - 14	168 B	84 KB
… (workers 0–15, mixed retained sizes)	168–216 B	1 KB – 85 KB
PvPManager housekeeper	168 B	< 1 KB

32 thread records match PvPManager Worker Thread, but only 16 distinct worker indices (0–15). Names repeat with different object IDs and retained sizes — consistent with two scheduler pools (reload without shutdown, or two plugin instances registering executors).

Combined retained heap on these thread objects is about 1.4 MB (thread locals + queue heads), excluding the bulk of player data held through queued tasks.

---

Context in Leak Suspect 2

MAT flags 6,731 org.bukkit.craftbukkit.entity.CraftPlayer instances as top-level dominators:

• Retained: 4,847,484,704 B (26.29% of 17.2 GB heap)
• Shallow: 1,884,680 B

PvPManager is not the only possible referrer (other threads also point at CraftPlayer), but the scheduled NewbieTask chain accounts for 5,312 of those players and is the clearest, repeatable path in the report.

---

Likely cause

1. Missing cleanup on quit — CombatPlayer / NewbieTask not removed when PlayerQuitEvent fires (or quit handling bypassed).
2. Task backlog — ScheduledThreadPoolExecutor delayed queue grows with one pending task per affected player; tasks retain players until they run or are cancelled.
3. Possible double scheduler — duplicate worker thread sets suggest the plugin scheduler may have been initialized more than once without tearing down the old pool.

Newbie protection duration, re-log behaviour, and async quit handling should be checked against PvPManager config and version release notes.

---

Recommendations

1. Upgrade PvPManager to the latest build for your server version and check changelog/issues for memory leaks around newbie protection or CombatPlayer cleanup.
2. Verify quit cleanup — ensure no soft-ignore of quit events; confirm CombatPlayer removal and pending task cancellation on disconnect.
3. Audit newbie settings — long or permanent newbie timers increase how long each CombatPlayer stays referenced if cleanup fails.
4. Avoid reload without restart — if /reload or plugman-style reloads are used, prefer full restarts so old ScheduledThreadPoolExecutor instances are not left alive.
5. Re-dump after fix — take a second MAT report and confirm NewbieTask / CombatPlayer counts drop after mass disconnect or restart.

---

Raw identifiers (for MAT follow-up)

Object	Address
CraftPlayer (class)	0x7fb8238f7738
CombatPlayer (class)	0x7fb83f04f910
NewbieTask (class)	0x7fb81e08f0f0
ScheduleUtils$ExceptionRunnable (class)	0x7fb83f400728
Server / plugin URLClassLoader	0x7fb81c002600

[ChanceSD]

This seems like it would only happen when running /newbie add on players already protected.
Out of curiosity how did this issue occur? To have 5000+ instances I assume it was something automated.
Either way I will be pushing a fix for this