Add transaction observability and task scheduling guards#72
Merged
Conversation
#63) - Add re-schedule guard to RepeatableCoreTask.runTask() that logs a warning and returns when the task is already scheduled - Add volatile to cross-thread state fields in CoreTask (bukkitTaskId, taskStartTime, taskRunningAsync, taskExecuted), RepeatableCoreTask (currentInterval, delayExpired, intervalStartTime, paused), and CancelableCoreTask (cancelled) - Remove self-scheduling from PlayerUnloadTask constructor — callers must now call runTask(true) explicitly after construction - Add RepeatableCoreTaskTest cases for double-scheduling prevention - Update PlayerUnloadTaskTest to verify constructor does not schedule - Update CLAUDE.md with anti-pattern and task scheduling notes Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX
…rvability (#62) - Add TransactionState enum (PENDING, COMMITTED, ROLLED_BACK) with volatile fields for cross-thread visibility - Add getTransactionState() and getFailureCause() to FailSafeTransaction so callers can inspect outcome without breaking fail-safe semantics - Replace e.printStackTrace() calls with Logger.log(Level.SEVERE, ..., e) to route stack traces through the plugin logger - Remove unused SLF4J import and field from FailSafeTransaction and BatchTransaction - Add TransactionStateTest for enum round-trip verification - Add state assertions to existing FailSafeTransactionTest cases - Update CLAUDE.md domain terminology with TransactionState entry Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX
…NE fallback warning (#65) - Add isVanilla() and isCustom() instance methods to CustomBlockWrapper and CustomEntityWrapper for checking whether a wrapper represents a vanilla or custom block/entity - Add isVanillaBlock(String) and isVanillaEntity(String) static methods for checking identifiers against the Paper registry without constructing a wrapper - Add dedup-guarded warning to ItemPluginType when material resolution fails and falls back to STONE (logs once per unique string) - Add IsVanilla and IsCustom nested test classes to both wrapper tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX
Add missing entries to .cursor/rules/core.mdc that were added to CLAUDE.md across the three wave-1 tickets: e.printStackTrace() anti-pattern, task scheduling notes (volatile fields, re-schedule guard), and TransactionState observability section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX
|
Claude finished @DiamondDagger590's task in 1m 48s —— View job Claude PR Review — in progress
|
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds transaction outcome tracking and failure accessors, strengthens task scheduling and cross-thread state handling, changes player unload startup to explicit scheduling, de-duplicates unresolved-material warnings, and adds vanilla/custom classification APIs for block and entity wrappers with tests and documentation. ChangesTransaction outcome tracking
Task lifecycle and concurrency
Item resolution and wrapper queries
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Remove the static WARNED_MATERIALS dedup set and private helper method. Log the warning directly on every unresolvable material — the spam makes misconfigured materials obvious to server owners. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.java`:
- Around line 95-96: Bound the WARNED_MATERIALS warning de-duplication cache so
unresolved material strings cannot accumulate for the JVM lifetime. Replace the
unbounded set with a bounded or expiring cache, or add lifecycle-based eviction,
while preserving suppression of repeated warnings within the configured
retention period.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro
Run ID: c1485209-7a85-4c52-a3f4-32c8fd9c9ead
📒 Files selected for processing (18)
.cursor/rules/core.mdcCLAUDE.mdsrc/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.javasrc/main/java/com/diamonddagger590/mccore/database/transaction/BatchTransaction.javasrc/main/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransaction.javasrc/main/java/com/diamonddagger590/mccore/database/transaction/TransactionState.javasrc/main/java/com/diamonddagger590/mccore/task/core/CancelableCoreTask.javasrc/main/java/com/diamonddagger590/mccore/task/core/CoreTask.javasrc/main/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTask.javasrc/main/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTask.javasrc/main/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapper.javasrc/main/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapper.javasrc/test/java/com/diamonddagger590/mccore/database/transaction/FailSafeTransactionTest.javasrc/test/java/com/diamonddagger590/mccore/database/transaction/TransactionStateTest.javasrc/test/java/com/diamonddagger590/mccore/task/core/RepeatableCoreTaskTest.javasrc/test/java/com/diamonddagger590/mccore/task/player/PlayerUnloadTaskTest.javasrc/test/java/com/diamonddagger590/mccore/util/item/CustomBlockWrapperTest.javasrc/test/java/com/diamonddagger590/mccore/util/item/CustomEntityWrapperTest.java
💤 Files with no reviewable changes (1)
- src/main/java/com/diamonddagger590/mccore/database/transaction/BatchTransaction.java
Comment on lines
+95
to
+96
| private static final Set<String> WARNED_MATERIALS = ConcurrentHashMap.newKeySet(); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the warning de-duplication cache.
WARNED_MATERIALS retains every distinct unresolved string for the lifetime of the JVM. If invalid material values can vary across plugin data or reloads, this becomes an unbounded memory leak and can eventually cause resource exhaustion. Use a bounded/expiring cache or provide lifecycle-based eviction while preserving warning de-duplication.
Also applies to: 128-136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/com/diamonddagger590/mccore/builder/item/ItemPluginType.java`
around lines 95 - 96, Bound the WARNED_MATERIALS warning de-duplication cache so
unresolved material strings cannot accumulate for the JVM lifetime. Replace the
unbounded set with a bounded or expiring cache, or add lifecycle-based eviction,
while preserving suppression of repeated warnings within the configured
retention period.
…ow permissions - Extract private lookupVanillaBlock/lookupVanillaEntityType helpers in CustomBlockWrapper and CustomEntityWrapper to consolidate FQN usage - Update core.mdc e.printStackTrace anti-pattern to reference CorePlugin.getInstance().getLogger() instead of Logger.log - Add Bash(git diff/log/show:*) to allowedTools in claude-review.yml so review sub-agents can run git commands Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR improves observability and safety across the transaction and task scheduling subsystems by:
FailSafeTransactionso callers can inspect outcomesRepeatableCoreTaskinstancesKey Changes
Transaction Observability
TransactionStateenum (PENDING,COMMITTED,ROLLED_BACK) to track transaction outcomesgetTransactionState()andgetFailureCause()methods toFailSafeTransactionfor post-execution inspectionLoggerand improved exception logging to useLogger.log(Level.SEVERE, msg, exception)instead ofprintStackTrace()volatilefor thread-safe visibilityTask Scheduling Guards
RepeatableCoreTask.runTask()to prevent double-scheduling; logs warning and returns if task already executedCoreTaskhierarchyvolatile(taskExecuted,bukkitTaskId,taskStartTime,taskRunningAsync,paused,delayExpired,currentInterval,cancelled)PlayerUnloadTaskto not self-schedule in constructor; callers must explicitly invokerunTask(true)after constructionWrapper Type Checking
isVanilla()andisCustom()instance methods toCustomBlockWrapperandCustomEntityWrapperisVanillaBlock(String)andisVanillaEntity(String)methods for identifier-based checkingLogging & Error Handling
e.printStackTrace()calls withLogger.log(Level.SEVERE, message, exception)inFailSafeTransactionandBatchTransactionItemPluginTypeusingConcurrentHashMap.newKeySet()RepeatableCoreTaskTestsetupDocumentation
.cursor/rules/core.mdcwith new guidelines on constructor task scheduling and logging practicesCLAUDE.mdwith transaction observability and task scheduling patternsTesting
TransactionStateTestwith enum value and round-trip verificationFailSafeTransactionTestto verify state transitions and failure cause captureisVanilla()andisCustom()test suites toCustomBlockWrapperTestandCustomEntityWrapperTestRepeatableCoreTaskTestPlayerUnloadTaskTestto verify constructor does not schedule andrunTask()doesImplementation Notes
volatileto ensure visibility across main and async scheduler threadsFailSafeTransactionstate is captured atomically at commit/rollback boundariesConcurrentHashMap.newKeySet()to avoid duplicate log spamrunTask()calls for testability and clarityhttps://claude.ai/code/session_017q76Se6Yvwmjow3t3FSLEX
Summary by CodeRabbit