fix(spigot): improve diagnostics for NMS mapping drift - #73
Merged
Conversation
…ails
ClassNames resolves every Spigot/CraftBukkit internal in one static
initializer, so a single renamed accessor on a new Minecraft release takes
the whole plugin down. What the operator got back could not be acted on: a
bare NullPointerException("<name> cannot be null") on the first touch, and
"NoClassDefFoundError: Could not initialize class ...ClassNames" on every
touch after that. Neither named the server software or the Minecraft
version, and only the first named the accessor at all.
Failures now report the exact accessor (plus every class name that was
tried), the server software, the server/Bukkit/Minecraft versions and the
Java version, and state that this is a Minecraft mapping change rather than
a Java runtime incompatibility - a misdiagnosis this class of report keeps
attracting.
NmsDiagnostics deliberately lives outside ClassNames so it still
initializes once ClassNames is in the erroneous state; that is what lets
SpigotPlatform.enable() replay the original reason as a plain top-level log
line instead of requiring a walk down a cause chain that only sometimes
carries it.
The ClassNames diff is dominated by re-indenting the initializer body into
the latching try/catch; `git diff -w` shows the actual change.
This is diagnosability only. It does not change which accessors resolve, so
it does not by itself fix any server that is currently failing - it makes
the next report say what to fix.
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
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.
Intent
A user (quick102000) reported in a Discord support thread that the Connect Java plugin throws NoSuchMethodException and cannot be used at all on a Minecraft server running Java 26. A support bot had diagnosed this in-thread as 'Java 26 breaks Connect's reflection-based libp2p classloader isolation'. The captain publicly promised the user a fix, and the task was to diagnose, fix, and regression-test it.
I first reproduced against a real Temurin JDK 26.0.1 and DISPROVED that diagnosis. The libp2p classloader isolation works correctly and identically on JDK 26 and JDK 21: verified end-to-end by loading the actual shipped connect-spigot.jar through a simulated server plugin classloader on both JDKs (both resolve the runtime classes child-first and the reflective Libp2pTunnelTransport constructor lookup succeeds), plus every module's test suite green on 26 (core p2p 70, bedrock 71, startup 3, module 5, velocity 14, spigot 17, bungee 2). javap shows Libp2pRuntimeLoader touches only Class.getClassLoader/getProtectionDomain/URLClassLoader.getURLs, none changed in 26; jdeps shows the only JDK-internal use in the shipped jar is sun.misc.Unsafe from shaded Guava/Guice/protobuf, whose reflective surface is byte-for-byte identical on 21 and 26 (terminal-deprecation warnings only, no failure). So there is NO Java 26 classloader bug to fix.
I escalated rather than ship a fabricated fix. The likely real cause is Minecraft-version NMS drift: spigot ClassNames resolves every server internal in one static initializer whose checkNotNull calls throw, so on a brand-new Minecraft any renamed accessor takes the plugin down; a server new enough to require Java 26 is new enough to have moved its mappings, making Java 26 correlation rather than cause.
The captain then explicitly directed a bounded scope: ship ONLY the diagnosability hardening (their 'option B'). This commit is therefore deliberately diagnosability-only and does NOT attempt to fix any currently-broken server. Decisions the captain explicitly made, which should not be flagged as omissions:
What this change does: ClassNames failures now name the exact missing accessor plus every class name tried, the server software, the server/Bukkit/Minecraft versions, and the Java version, and state this is a Minecraft mapping change rather than a Java runtime incompatibility (the exact misdiagnosis this report attracted). Previously the operator got a bare NullPointerException(' cannot be null') on the first touch and 'NoClassDefFoundError: Could not initialize class ...ClassNames' on every touch after, neither naming server software or MC version.
Deliberate design points:
Regression test (spigot NmsDiagnosticsTest, 6 tests) pins the specific gap rather than 'it loads'. Its end-to-end case loads ClassNames in a throwaway child-first classloader so its initializer runs for real and fails exactly as a drifted mapping fails on a live server, then asserts the latch still hands back the accessor-named message after the JVM has degraded to the uninformative NoClassDefFoundError. I verified this test FAILS on the pre-fix ClassNames (assertion 'the reason ClassNames failed to initialize must survive the burial ==> expected: not ') and passes on the fix. Note I corrected one of my own assertions mid-work: I had asserted the repeat NoClassDefFoundError is causeless, but the JVM does chain the original cause, so the test now asserts the accurate, still-meaningful property that its own top-level message never names the drifted accessor. Full ./gradlew build is green, and the spigot suite including the 6 new tests is also green on JDK 26.
What Changed
Risk Assessment
✅ Low: The point-of-use guard preserves the optional plain-Spigot path, raises the named diagnostic only when Velocity support requires the missing field, and the remaining diff is bounded to the stated diagnosability hardening.
Testing
The focused NmsDiagnostics regression suite passed under Java 21 and in an actual Temurin 26.0.1 forked test JVM. Direct Java 26 output demonstrated the actionable diagnostic message. Gradle 8.5 cannot run itself on Java 26, so the test JVM was forked explicitly; generated worktree outputs were cleaned.
Evidence: Java 26 operator diagnostic
Direct Java 26 simulated-Paper diagnostic output with accessor, candidates, server/MC/Bukkit versions, Java version, and mapping-change diagnosis.Evidence: Java 26 regression run
Gradle trace showing the focused tests launched with Temurin 26.0.1 and completed successfully.Evidence: NmsDiagnostics JDK 26 HTML report
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (2) ✅
spigot/src/main/java/com/minekube/connect/util/ClassNames.java:470- Required intent says: “ClassNames failures now name the exact accessor (plus every class name that was tried), the server software, the server/Bukkit/Minecraft versions, and the Java version.” The newcheckNotNullat line 470 only receives generic labels, andSTART_CLIENT_VERIFICATIONat line 292 passes no tried-class list. Other reachable paths still bypass diagnostics entirely:getMethodcan dereference a null class at line 118, whilegetClassOrFallback/getClassOrThrowat lines 121-124, 264-265, 307-308, and 384-387 retain raw class-not-found failures. The catch at lines 441-444 therefore can still latch an uninformative NPE/ClassNotFoundException, leaving the required diagnosability gap reachable. Normalize these lookups at the shared ClassNames lookup boundary, including all candidate names and member signatures.🔧 Fix: Hardened ClassNames drift diagnostics across all lookup paths
1 error still open:
spigot/src/main/java/com/minekube/connect/util/ClassNames.java:194- The required fix says to “normalize EVERY ClassNames lookup ... so all drift paths produce the named, actionable diagnostic.”VELOCITY_LOGIN_MESSAGE_IDis still assigned fromgetField(...)without validation. If a Paper/NMS mapping renames that field, initialization succeeds withnull; when Velocity support is enabled,SpigotDataHandlerpasses it tosetValue, whose first operation dereferences it inmakeAccessible, producing an unlatched NPE during login instead of an NmsDiagnostics message. Guard this lookup at the ClassNames boundary while preserving the intentionally optional non-Paper path.🔧 Fix: Guarded optional velocity field drift at point of use
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
JAVA_HOME=/Users/robin/.local/share/mise/installs/java/temurin-21.0.11+10.0.LTS ./gradlew :spigot:test --tests com.minekube.connect.util.NmsDiagnosticsTest --no-daemon --console=plain./gradlew -I /var/folders/1y/cjgf53nj31n_dxsspqnjfjvc0000gn/T/no-mistakes-evidence/01KYK0TT5C562GXDDG8SRYYVV7/fork-tests-on-jdk26.init.gradle :spigot:test --tests com.minekube.connect.util.NmsDiagnosticsTest --no-daemon --console=plain --info --rerun-tasksTemurin 26.0.1 compiled and ran the simulated-PaperNmsDiagnosticsEvidenceharness./gradlew clean --no-daemon --console=plainand./gradlew -p build-logic clean --no-daemon --console=plaingit status --short --branch🔧 **Document** - 1 issue found → auto-fixed ✅
/Users/robin/.no-mistakes/worktrees/bd6a9ec68e4b/01KYK0TT5C562GXDDG8SRYYVV7/spigot/src/test/java/com/minekube/connect/util/NmsDiagnosticsTest.java:162- Test method name still says the repeat NoClassDefFoundError is causeless; renaming it would modify test code outside this phase.🔧 Fix: Correct diagnostic test naming
✅ Re-checked - no issues remain.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.