Add transaction observability and task scheduling guards

.cursor/rules/core.mdc

@@ -98,7 +98,17 @@ void getStatistic_returnsStatistic_whenKeyIsRegistered() { ... }
 - **No state stored in Registry/Manager objects beyond their scope** — domain state belongs on domain objects
 - **No hard-coded strings for namespaced keys or config routes** — define as constants
 - **No direct entity casting without guard** — use `instanceof` pattern matching: `if (entity instanceof Player player) { ... }`
-- **No decorative section-divider comments** — do not use `// ── Section ──`, `// --- Section ---`, or similar ASCII-art dividers to group methods or tests; rely on class structure, method naming, and `@DisplayName` annotations instead
+- **No decorative section-divider comments** — rely on class structure, method naming, and `@DisplayName` annotations instead of `// --- Section ---` dividers
+- **No constructor-initiated task scheduling** — callers schedule tasks explicitly after construction via `runTask()`; constructors must not call `runTask()` because it couples object creation to the scheduler and complicates testing
+- **No `e.printStackTrace()`** — always use `CorePlugin.getInstance().getLogger().log(Level.SEVERE, "context message", e)` so stack traces route through the server logger
+
+## Task Scheduling
+
+`RepeatableCoreTask.runTask(boolean)` guards against double-scheduling — calling it on an already-running task logs a warning and returns without scheduling a second timer. All cross-thread state fields in the `CoreTask` hierarchy (`taskExecuted`, `bukkitTaskId`, `cancelled`, `paused`, etc.) are `volatile` to ensure visibility across the main and async scheduler threads.
+
+## Transaction Observability
+
+`FailSafeTransaction` tracks its outcome via `TransactionState` (`PENDING`, `COMMITTED`, `ROLLED_BACK`). The `executeTransaction()` method still returns `void` and never throws — callers inspect `getTransactionState()` and `getFailureCause()` after execution to detect failures.
 
 ---
 

.github/workflows/claude-review.yml

@@ -65,7 +65,7 @@ jobs:
           claude_args: |
             --model ${{ env.CLAUDE_MODEL }}
             --max-turns 40
-            --allowedTools "Task,Read,Grep,Glob,Bash(gh pr diff:*),Bash(gh pr view:*),mcp__github_inline_comment__create_inline_comment"
+            --allowedTools "Task,Read,Grep,Glob,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(git diff:*),Bash(git log:*),Bash(git show:*),mcp__github_inline_comment__create_inline_comment"
 
   # On-demand: an @claude mention in a PR comment or review comment. Tag mode
   # (no prompt) — claude-code-action handles the sticky comment natively here.
@@ -101,5 +101,5 @@ jobs:
           claude_args: |
             --model ${{ env.CLAUDE_MODEL }}
             --max-turns 40
-            --allowedTools "Task,Read,Grep,Glob,Bash(gh pr diff:*),Bash(gh pr view:*),mcp__github_inline_comment__create_inline_comment"
+            --allowedTools "Task,Read,Grep,Glob,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(git diff:*),Bash(git log:*),Bash(git show:*),mcp__github_inline_comment__create_inline_comment"
             --append-system-prompt "If asked to review or re-review this PR, follow .github/claude-review-prompt.md including its re-review convergence protocol. Otherwise answer the request directly."

CLAUDE.md

@@ -215,6 +215,7 @@ src/main/java/com/diamonddagger590/mccore/
 | **CreateTableFunction** | Functional interface called once at DB init to create a table if it doesn't exist. |
 | **UpdateTableFunction** | Functional interface called after table creation to apply schema migrations. |
 | **Transaction** | Abstract base for executing an ordered list of `PreparedStatement`s against a single `Connection`. Subclasses define failure semantics: `BatchTransaction` commits whatever succeeds and logs individual failures; `FailSafeTransaction` rolls back everything if any single statement fails. |
+| **TransactionState** | Enum (`PENDING`, `COMMITTED`, `ROLLED_BACK`) tracking the outcome of a `FailSafeTransaction`. Fields are `volatile` for cross-thread visibility. Callers inspect state after `executeTransaction()` returns — the method still returns `void` and never throws. |
 | **DAO** | Static JDBC methods for reading/writing a specific entity. Always takes `Connection` as the first argument. |
 | **ReloadableContent** | A config-backed value that can be refreshed at runtime without a server restart. |
 | **PlayerSetting** | A namespaced, persistent player preference. Stored in the database and loaded with the player. |
@@ -341,6 +342,8 @@ managerRegistry.manager(CoreManagerKey.CORE_DATABASE_MANAGER).getDatabase().init
 
 ### Task Scheduling
 
+`RepeatableCoreTask.runTask(boolean)` guards against double-scheduling — calling it on an already-running task logs a warning and returns without scheduling a second timer. All cross-thread state fields in the `CoreTask` hierarchy (`taskExecuted`, `bukkitTaskId`, `cancelled`, `paused`, etc.) are `volatile` to ensure visibility across the main and async scheduler threads.
+
 ```java
 // One-off sync task
 new CoreTask(plugin) {
@@ -422,6 +425,7 @@ Register with `ReloadableContentManager` so it refreshes automatically on `/relo
 - **No hard-coded strings for namespaced keys or config routes** — define constants on the owning class or a dedicated constants file
 - **No direct entity casting without a null/type guard** — use `instanceof` pattern matching: `if (entity instanceof Player player) { ... }`
 - **No decorative section-divider comments** — do not use `// ── Section ──`, `// --- Section ---`, or similar ASCII-art dividers to group methods or tests; rely on class structure, method naming, and `@DisplayName` annotations to communicate organization
+- **No constructor-initiated task scheduling** — callers schedule tasks explicitly after construction via `runTask()`; constructors must not call `runTask()` because it couples object creation to the scheduler and complicates testing
 
 ---
 

src/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.java

@@ -34,6 +34,7 @@ public enum ItemPluginType {
             try {
                 return fromBase64(customItem);
             } catch (Exception exception) {
+                CorePlugin.getInstance().getLogger().warning("Unresolvable material '" + customItem + "', falling back to STONE");
                 return ItemType.STONE.createItemStack(1);
             }
         }
@@ -54,6 +55,7 @@ public enum ItemPluginType {
             try {
                 return fromBase64(customItem);
             } catch (Exception exception) {
+                CorePlugin.getInstance().getLogger().warning("Unresolvable material '" + customItem + "', falling back to STONE");
                 return ItemType.STONE.createItemStack(1);
             }
         }
@@ -82,6 +84,7 @@ public enum ItemPluginType {
             try {
                 return fromBase64(customItem);
             } catch (Exception exception) {
+                CorePlugin.getInstance().getLogger().warning("Unresolvable material '" + customItem + "', falling back to STONE");
                 return ItemType.STONE.createItemStack(1);
             }
         }

src/main/java/com/diamonddagger590/mccore/database/transaction/BatchTransaction.java

@@ -2,7 +2,6 @@
 
 import com.diamonddagger590.mccore.CorePlugin;
 import org.jetbrains.annotations.NotNull;
-import org.slf4j.LoggerFactory;
 
 import java.sql.Connection;
 import java.sql.PreparedStatement;
@@ -19,8 +18,6 @@
  */
 public class BatchTransaction extends Transaction {
 
-    private static final org.slf4j.Logger log = LoggerFactory.getLogger(BatchTransaction.class);
-
     public BatchTransaction(@NotNull Connection connection) {
         super(connection);
     }

src/main/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransaction.java

@@ -2,23 +2,28 @@
 
 import com.diamonddagger590.mccore.CorePlugin;
 import org.jetbrains.annotations.NotNull;
-import org.slf4j.LoggerFactory;
 
 import java.sql.Connection;
 import java.sql.PreparedStatement;
 import java.sql.SQLException;
 import java.util.List;
+import java.util.Optional;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 
 /**
  * A fail-safe transaction requires all {@link PreparedStatement}s
  * to succeed in order for any of them to be committed.
  * <p>
  * If a single statement fails, then the transaction will instead roll back.
+ * <p>
+ * After execution, callers can inspect the outcome via {@link #getTransactionState()}
+ * and retrieve any failure cause via {@link #getFailureCause()}.
  */
 public class FailSafeTransaction extends Transaction {
 
-    private static final org.slf4j.Logger log = LoggerFactory.getLogger(FailSafeTransaction.class);
+    private volatile TransactionState transactionState = TransactionState.PENDING;
+    private volatile SQLException failureCause;
 
     public FailSafeTransaction(@NotNull Connection connection) {
         super(connection);
@@ -45,16 +50,17 @@ public void executeTransaction() {
                 }
             }
             connection.commit();
+            transactionState = TransactionState.COMMITTED;
         } catch (SQLException e) {
+            transactionState = TransactionState.ROLLED_BACK;
+            failureCause = e;
             logger.severe("Encountered an exception while executing a fail safe transaction... rolling back.");
-            logger.severe(e.getMessage());
-            e.printStackTrace();
+            logger.log(Level.SEVERE, e.getMessage(), e);
             try {
                 connection.rollback();
             } catch (SQLException rollbackException) {
                 logger.severe("Encountered an exception while rolling back transaction...");
-                logger.severe(rollbackException.getMessage());
-                rollbackException.printStackTrace();
+                logger.log(Level.SEVERE, rollbackException.getMessage(), rollbackException);
             }
         }
         finally {
@@ -73,4 +79,25 @@ public void executeTransaction() {
             }
         }
     }
+
+    /**
+     * Gets the current state of this transaction.
+     *
+     * @return The {@link TransactionState} representing the outcome of this transaction.
+     */
+    @NotNull
+    public TransactionState getTransactionState() {
+        return transactionState;
+    }
+
+    /**
+     * Gets the {@link SQLException} that caused this transaction to roll back, if any.
+     *
+     * @return An {@link Optional} containing the failure cause, or empty if the transaction
+     *         has not failed.
+     */
+    @NotNull
+    public Optional<SQLException> getFailureCause() {
+        return Optional.ofNullable(failureCause);
+    }
 }

src/main/java/com/diamonddagger590/mccore/database/transaction/TransactionState.java

@@ -0,0 +1,26 @@
+package com.diamonddagger590.mccore.database.transaction;
+
+/**
+ * Tracks the outcome of a {@link FailSafeTransaction} execution.
+ * <p>
+ * A transaction starts in {@link #PENDING} and transitions to either
+ * {@link #COMMITTED} (all statements succeeded) or {@link #ROLLED_BACK}
+ * (a statement failed and the transaction was rolled back).
+ */
+public enum TransactionState {
+
+    /**
+     * The transaction has not yet been executed.
+     */
+    PENDING,
+
+    /**
+     * All statements succeeded and the transaction was committed.
+     */
+    COMMITTED,
+
+    /**
+     * A statement failed and the transaction was rolled back.
+     */
+    ROLLED_BACK
+}

src/main/java/com/diamonddagger590/mccore/task/core/CancelableCoreTask.java

@@ -10,7 +10,7 @@
  */
 public abstract class CancelableCoreTask extends RepeatableCoreTask {
 
-    protected boolean cancelled;
+    protected volatile boolean cancelled;
 
     public CancelableCoreTask(@NotNull CorePlugin plugin, double taskDelay, double taskFrequency) {
         super(plugin, taskDelay, taskFrequency);

src/main/java/com/diamonddagger590/mccore/task/core/CoreTask.java

@@ -11,10 +11,10 @@
 public abstract class CoreTask implements Runnable {
 
     private final CorePlugin plugin;
-    protected int bukkitTaskId = -1;
-    protected long taskStartTime = -1;
-    protected boolean taskRunningAsync;
-    protected boolean taskExecuted;
+    protected volatile int bukkitTaskId = -1;
+    protected volatile long taskStartTime = -1;
+    protected volatile boolean taskRunningAsync;
+    protected volatile boolean taskExecuted;
 
     public CoreTask(@NotNull CorePlugin plugin) {
         this.plugin = plugin;

src/main/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTask.java

@@ -18,10 +18,10 @@ public abstract class RepeatableCoreTask extends MultiExecutionCoreTask {
 
     protected final double taskDelay;
     protected final double taskFrequency;
-    protected int currentInterval = 0;
-    protected boolean delayExpired;
-    protected long intervalStartTime;
-    protected boolean paused;
+    protected volatile int currentInterval = 0;
+    protected volatile boolean delayExpired;
+    protected volatile long intervalStartTime;
+    protected volatile boolean paused;
 
     public RepeatableCoreTask(@NotNull CorePlugin plugin, double taskDelay, double taskFrequency) {
         super(plugin);
@@ -31,6 +31,11 @@ public RepeatableCoreTask(@NotNull CorePlugin plugin, double taskDelay, double t
 
     @Override
     public void runTask(boolean runAsync) {
+        if (taskExecuted) {
+            getPlugin().getLogger().warning("RepeatableCoreTask (id=" + bukkitTaskId + ") is already scheduled; ignoring duplicate runTask() call");
+            return;
+        }
+
         if (runAsync) {
             bukkitTaskId = Bukkit.getScheduler().runTaskTimerAsynchronously(getPlugin(), this, 0, 1).getTaskId();
             taskRunningAsync = true;

src/main/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTask.java

@@ -23,12 +23,17 @@ public abstract class PlayerUnloadTask extends ExpireableCoreTask {
     private final CompletableFuture<Boolean> result;
     private boolean completed;
 
+    /**
+     * Callers must invoke {@code runTask(true)} after construction to begin the async unload task.
+     *
+     * @param plugin     The {@link CorePlugin} that owns this task.
+     * @param corePlayer The {@link CorePlayer} to unload.
+     */
     public PlayerUnloadTask(@NotNull CorePlugin plugin, @NotNull CorePlayer corePlayer) {
         super(plugin, 0L, 2, 10L);
         this.corePlayer = corePlayer;
         this.result = new CompletableFuture<>();
         completed = false;
-        runTask(true);
     }
 
     private void runUnloadPlayerTask() {

src/main/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapper.java

@@ -47,7 +47,7 @@ public CustomBlockWrapper(@NotNull Material material) {
     }
 
     public CustomBlockWrapper(@NotNull String customBlock) {
-        BlockType blockType = io.papermc.paper.registry.RegistryAccess.registryAccess().getRegistry(io.papermc.paper.registry.RegistryKey.BLOCK).get(Methods.getMinecraftKey(customBlock));
+        BlockType blockType = lookupVanillaBlock(customBlock);
         if (blockType != null) {
             this.material = blockType.asMaterial();
             this.customBlock = null;
@@ -73,6 +73,49 @@ public CustomBlockWrapper(@NotNull Block block) {
         this.customBlock = customBlockResult;
     }
 
+    /**
+     * Checks whether this wrapper represents a vanilla block (resolved via the Paper registry).
+     *
+     * @return {@code true} if this wrapper holds a {@link Material}, meaning the block
+     *         was recognized as a vanilla Minecraft block.
+     */
+    public boolean isVanilla() {
+        return material != null;
+    }
+
+    /**
+     * Checks whether this wrapper represents a custom block (from a model plugin).
+     *
+     * @return {@code true} if this wrapper holds a custom block identifier, meaning the block
+     *         was not recognized as a vanilla Minecraft block.
+     */
+    public boolean isCustom() {
+        return customBlock != null;
+    }
+
+    /**
+     * Checks whether the given identifier resolves to a vanilla block in the Paper block registry.
+     *
+     * @param id The block identifier to check (e.g. {@code "stone"}, {@code "iron_ore"}).
+     * @return {@code true} if the identifier is recognized as a vanilla block.
+     */
+    public static boolean isVanillaBlock(@NotNull String id) {
+        return lookupVanillaBlock(id) != null;
+    }
+
+    /**
+     * Looks up a block identifier in the Paper block registry.
+     *
+     * @param id The block identifier to look up (e.g. {@code "stone"}, {@code "iron_ore"}).
+     * @return The {@link BlockType} if the identifier is a vanilla block, or {@code null} if not found.
+     */
+    @Nullable
+    private static BlockType lookupVanillaBlock(@NotNull String id) {
+        return io.papermc.paper.registry.RegistryAccess.registryAccess()
+                .getRegistry(io.papermc.paper.registry.RegistryKey.BLOCK)
+                .get(Methods.getMinecraftKey(id));
+    }
+
     /**
      * Gets an {@link Optional} containing the {@link Material} represented
      * by this wrapper.

src/main/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapper.java

@@ -38,7 +38,7 @@ public CustomEntityWrapper(@NotNull EntityType entityType) {
     }
 
     public CustomEntityWrapper(@NotNull String customEntity) {
-        EntityType entityType = io.papermc.paper.registry.RegistryAccess.registryAccess().getRegistry(io.papermc.paper.registry.RegistryKey.ENTITY_TYPE).get(Methods.getMinecraftKey(customEntity));
+        EntityType entityType = lookupVanillaEntityType(customEntity);
         if (entityType != null) {
             this.entityType = entityType;
             this.customEntity = null;
@@ -65,6 +65,49 @@ public CustomEntityWrapper(@NotNull Entity entity) {
         this.customEntity = customEntityResult;
     }
 
+    /**
+     * Checks whether this wrapper represents a vanilla entity (resolved via the Paper registry).
+     *
+     * @return {@code true} if this wrapper holds an {@link EntityType}, meaning the entity
+     *         was recognized as a vanilla Minecraft entity.
+     */
+    public boolean isVanilla() {
+        return entityType != null;
+    }
+
+    /**
+     * Checks whether this wrapper represents a custom entity (from a model plugin).
+     *
+     * @return {@code true} if this wrapper holds a custom entity identifier, meaning the entity
+     *         was not recognized as a vanilla Minecraft entity.
+     */
+    public boolean isCustom() {
+        return customEntity != null;
+    }
+
+    /**
+     * Checks whether the given identifier resolves to a vanilla entity in the Paper entity type registry.
+     *
+     * @param id The entity identifier to check (e.g. {@code "zombie"}, {@code "creeper"}).
+     * @return {@code true} if the identifier is recognized as a vanilla entity type.
+     */
+    public static boolean isVanillaEntity(@NotNull String id) {
+        return lookupVanillaEntityType(id) != null;
+    }
+
+    /**
+     * Looks up an entity identifier in the Paper entity type registry.
+     *
+     * @param id The entity identifier to look up (e.g. {@code "zombie"}, {@code "creeper"}).
+     * @return The {@link EntityType} if the identifier is a vanilla entity, or {@code null} if not found.
+     */
+    @Nullable
+    private static EntityType lookupVanillaEntityType(@NotNull String id) {
+        return io.papermc.paper.registry.RegistryAccess.registryAccess()
+                .getRegistry(io.papermc.paper.registry.RegistryKey.ENTITY_TYPE)
+                .get(Methods.getMinecraftKey(id));
+    }
+
     /**
      * Gets an {@link Optional} containing the {@link EntityType} represented
      * by this wrapper.