Skip to content

Fix CoreTask double-scheduling: PlayerUnloadTask self-schedules, RepeatableCoreTask has no re-schedule guard #63

Description

@DiamondDagger590

Context

From the July 2026 McRPG quest-system audit (adversarially verified: CONFIRMED, severity CRITICAL). Every player quit in McRPG currently registers the unload work as TWO 1-tick repeating Bukkit timers — one async, one on the main thread — producing concurrent duplicate chain flushes, synchronous JDBC on the main thread, and a leaked forever-ticking timer that pins the player object graph per quit.

Problem

  • PlayerUnloadTask's constructor self-schedules with runTask(true) (src/main/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTask.java:31). McRPG's quit listener then calls .runTask() (sync) on the same object.
  • RepeatableCoreTask.runTask(boolean) (task/core/RepeatableCoreTask.java:33) has no re-schedule guard (unlike CoreTask.runTask, which is overridden away), so the same Runnable registers as two timers and the second registration overwrites bukkitTaskId — the first timer's id is lost and can never be cancelled.
  • Both timers share non-volatile state fields (paused, cancelled, delayExpired, intervalStartTime).
  • CancelableCoreTask.cancelTask() (task/core/CancelableCoreTask.java:22) cancels only the tracked bukkitTaskId, so a lost registration outlives cancellation permanently.

Failure scenario: player quits → constructor registers async timer A; listener registers sync timer B (bukkitTaskId=B). Both pass the interval check in the same tick: main thread and async thread run the unload concurrently, racing on chain-state writes and dirty flags. Whichever finishes calls cancelTask() — only B is cancelled; A ticks forever holding the player graph.

Implementation plan

  1. Remove self-scheduling from the PlayerUnloadTask constructor (callers schedule explicitly). Check PlayerLoadTask for the same pattern.
  2. Add an already-scheduled guard to RepeatableCoreTask.runTask(boolean) mirroring CoreTask.runTask()'s taskExecuted guard — a second call must be a no-op (log a WARNING).
  3. Make task state fields (paused, cancelled, delayExpired, and any read across threads) volatile.
  4. In CancelableCoreTask, ensure cancellation covers every registration (track all task ids, or make the guard from step 2 structurally prevent multiple registrations).
  5. Audit other task/player/ and task/core/ classes for constructor-side scheduling.
  6. Coordinate with the McRPG companion ticket that fixes the listener call sites — note the API behavior change in the PR description (downstream plugins relying on constructor self-scheduling would break; this repo's policy is deliberate, documented breaking changes only).

Acceptance criteria

  • Calling runTask twice on any RepeatableCoreTask schedules exactly one Bukkit task.
  • No CoreTask subclass schedules itself from its constructor.
  • Cross-thread-visible task state is volatile.
  • Cancelling a CancelableCoreTask leaves zero live registrations.
  • Unit tests cover the double-schedule guard and cancellation (use ManagedExecutorExtension via @RegisterExtension for any executor needs; plain JUnit where Bukkit isn't required).
  • ./gradlew test passes with zero failures.

Notes for the implementing session

Follow CLAUDE.md test conventions (Given/When/Then @DisplayName). One logical change per PR.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions