Skip to content

fix(spigot): support ViaVersion 5.x legacy initializer unwrapping#67

Merged
robinbraemer merged 4 commits into
mainfrom
fm/connect-java-spigot-via5x-getoriginal-fix
Jul 26, 2026
Merged

fix(spigot): support ViaVersion 5.x legacy initializer unwrapping#67
robinbraemer merged 4 commits into
mainfrom
fm/connect-java-spigot-via5x-getoriginal-fix

Conversation

@robinbraemer

Copy link
Copy Markdown
Member

Intent

Fix a real, currently-shipping connect-java bug proven by a prior triage: on plain Spigot/CraftBukkit with ViaVersion >= 5.0, Connect fails to enable at all and NO player can join.

Root cause (already diagnosed and reproduced against the shipped 0.12.3 bytecode with Via 5.11.0, three independent proofs): SpigotInjector.getChildHandler() called ((BukkitChannelInitializer) childHandler).getOriginal() to unwrap ViaVersion's legacy (non-Paper) channel-initializer wrapper so Connect does not double-inject Via's handlers. ViaVersion 5.0 renamed that accessor to original() and pulled it up into the WrappedChannelInitializer interface / ViaChannelInitializer. connect-java compiled against the compile-only Via 4.0.0 pin (Versions.kt), so it built fine and threw NoSuchMethodError at runtime. NoSuchMethodError is an Error, not an Exception, so it escaped ConnectPlatform.enable()'s catch (Exception), escaped onEnable(), and Bukkit disabled the plugin outright. Paper and Paper forks are NOT affected: there Via registers a Paper ChannelInitializeListener instead of wrapping the child handler, so the unwrap branch is dead code. Latent since Via 5.0, present in every release.

Deliberate decisions made (so these are intended, not oversights):

  • NOT a literal getOriginal() -> original() rename. Connect supports a wide server/plugin range, so a literal rename would fix Via 5.x and break Via 4.x. Instead the accessor is resolved reflectively by name, newest first (original(), then getOriginal()), in a new private unwrapViaInitializer() helper. Reflection is deliberately preferred over an instanceof/cast against Via 5's WrappedChannelInitializer interface, because that interface does not exist on Via 4.x and the instanceof would raise NoClassDefFoundError there, i.e. it would reintroduce the same class of bug in the other direction.
  • The helper deliberately catches Throwable and degrades to 'unwrap skipped, warning logged' instead of propagating. Rationale: a future Via rename must leave Connect starting (worst case Via handlers double-injected) rather than refusing to enable, which is the far worse outcome. This is intentional broad exception handling, requested by the triage.
  • Recommended hardening included because it prevents the whole failure class: ConnectPlatform.enable()'s catch (Exception exception) widened to catch (Throwable throwable), so any injector Error becomes a logged, contained injection failure instead of a plugin disable. This is an intentional catch-Throwable, documented with an inline comment.
  • Compile-only Via dep bumped 4.0.0 -> 5.11.0 in build-logic/src/main/kotlin/Versions.kt, plus a new provided()/testImplementation entry for viaversion-common. viaversion-common is needed because since Via 5.0 BukkitChannelInitializer extends ViaChannelInitializer, which lives in that artifact, and the viaversion-bukkit POM declares no transitive dependencies. Both are provided() (platform-supplied, excluded from the shaded jar), so this does not change what the jar ships nor the minimum supported Via at runtime - verified: 0 viaversion classes in connect-spigot.jar and no getOriginal() Methodref left in its constant pool.

Mandatory captain condition - a signature-level regression test that provably fails pre-fix and passes post-fix, matching the repo convention beside BedrockVelocityGuice7ProvisioningTest:

  • spigot/src/test/java/com/minekube/connect/inject/spigot/SpigotInjectorViaLegacyPathTest.java: real ViaVersion 5.11.0 on the test classpath, a real BukkitChannelInitializer wrapping a stub ChannelInitializer, a mocked ChannelFuture -> Channel -> ChannelPipeline whose handler declares a childHandler field (stand-in for Netty's package-private ServerBootstrapAcceptor), new SpigotInjector(logger, isViaVersion=true), and a reflective call of the real private getChildHandler(ChannelFuture) asserting the returned handler IS the unwrapped stub. Explicitly verified fail-pre/pass-post: with the pre-fix injector restored and compile against Via 4.0.0 while testing against 5.11.0, it FAILED with NoSuchMethodError: 'io.netty.channel.ChannelInitializer com.viaversion.viaversion.bukkit.handlers.BukkitChannelInitializer.getOriginal()'; it passes on the fix. It also carries an isViaVersion=false control, a degrade-on-unknown-API test, and testClasspathViaIsThePostRenameApi() which asserts the test classpath really is post-rename Via so the guard cannot go vacuously green if the dependency is ever downgraded.
  • core/src/test/java/com/minekube/connect/ConnectPlatformEnableFailureContainmentTest.java guards the catch-Throwable hardening; also verified failing on the pre-hardening catch (Exception) and passing after.
  • Extra out-of-band validation: the built connect-spigot.jar was executed against BOTH Via 4.0.0 and Via 5.11.0 via a scratch harness; both unwrap to the server's own initializer, confirming the 4.x path still works. ./gradlew build is green locally.

Deliberately OUT OF SCOPE per the captain, do not flag as missing work: the triage's secondary observations are separate follow-up tickets - PacketHandlerAddon's unguarded pipeline.addAfter("encoder", ...) landmine, DebugAddon being a silent no-op on modern Paper, and adding ViaVersion to spigot plugin.yml softdepend. Also intentionally NOT referenced or closed: minekube/connect issue GeyserMC#96 (the triage could not reproduce that Paper-specific report and closing it is an outward-facing decision the captain owns), so no issue reference belongs in the commit or PR. AGENTS.md (CLAUDE.md symlink) gained a concise 'Third-party platform APIs (ViaVersion & friends)' section recording the reflective-boundary convention, the Paper-vs-Spigot injection split, and the catch-Throwable containment rule.

What Changed

  • Resolve ViaVersion’s legacy initializer accessor reflectively (original() first, then getOriginal()) so plain Spigot supports Via 5.x while retaining Via 4.x compatibility.
  • Contain injector Errors during platform enablement with logged failures, preventing reflective API drift from escaping onEnable().
  • Update compile-only ViaVersion dependencies, add focused regression coverage, and document the cross-version injector conventions.

Risk Assessment

✅ Low: The wording fixes are now coherent and the full source change remains bounded with no additional material issues identified.

Testing

Exercised the real ViaVersion 5.11 legacy unwrap path, unknown-API degradation, non-Via control, and catch-Throwable containment; inspected the shaded Spigot artifact, which contains no ViaVersion classes or direct getOriginal() bytecode call. Evidence was saved outside the worktree; no UI screenshot applies to this server-side plugin behavior.

Evidence: Focused validation log
Connect Java ViaVersion 5.x focused validation
=============================================
SpigotInjectorViaLegacyPathTest:
<testsuite name="com.minekube.connect.inject.spigot.SpigotInjectorViaLegacyPathTest" tests="5" skipped="0" failures="0" errors="0" timestamp="2026-07-26T12:12:49" hostname="Robins-MacBook-Pro.fritz.box" time="0.192">
  <testcase name="returnsPlainChildHandlerUntouched()" classname="com.minekube.connect.inject.spigot.SpigotInjectorViaLegacyPathTest" time="0.184"/>
  <testcase name="unwrapsViaVersionLegacyChannelInitializer()" classname="com.minekube.connect.inject.spigot.SpigotInjectorViaLegacyPathTest" time="0.001"/>
  <testcase name="unknownViaInitializerApiDegradesToSkippingTheUnwrap()" classname="com.minekube.connect.inject.spigot.SpigotInjectorViaLegacyPathTest" time="0.004"/>
  <testcase name="keepsViaInitializerWhenViaVersionIsNotDetected()" classname="com.minekube.connect.inject.spigot.SpigotInjectorViaLegacyPathTest" time="0.001"/>
  <testcase name="testClasspathViaIsThePostRenameApi()" classname="com.minekube.connect.inject.spigot.SpigotInjectorViaLegacyPathTest" time="0.001"/>
  <system-out><![CDATA[]]></system-out>
  <system-err><![CDATA[SLF4J: No SLF4J providers were found.
ConnectPlatformEnableFailureContainmentTest:
<testsuite name="com.minekube.connect.ConnectPlatformEnableFailureContainmentTest" tests="2" skipped="0" failures="0" errors="0" timestamp="2026-07-26T12:12:49" hostname="Robins-MacBook-Pro.fritz.box" time="0.165">
  <testcase name="enableReportsAFailedInjection()" classname="com.minekube.connect.ConnectPlatformEnableFailureContainmentTest" time="0.161"/>
  <testcase name="enableContainsAnInjectorError()" classname="com.minekube.connect.ConnectPlatformEnableFailureContainmentTest" time="0.003"/>
  <system-out><![CDATA[]]></system-out>
  <system-err><![CDATA[]]></system-err>
Shaded Spigot artifact:
jar=spigot/build/libs/connect-spigot.jar
packaged ViaVersion classes=0
direct getOriginal Methodref entries in SpigotInjector=0
- Evidence: Built Spigot plugin artifact (local file: /var/folders/1y/cjgf53nj31n_dxsspqnjfjvc0000gn/T/no-mistakes-evidence/01KYF4RTB3NYAN6G4FGYHZTX2A/connect-spigot.jar)

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 1 issue found → auto-fixed (2) ✅
  • 🚨 core/src/main/java/com/minekube/connect/ConnectPlatform.java:136 - The new catch (Throwable) at core/src/main/java/com/minekube/connect/ConnectPlatform.java:136 returns false, but SpigotPlatform.enable() immediately calls disablePlugin(plugin) whenever super.enable() returns false (spigot/src/main/java/com/minekube/connect/SpigotPlatform.java:54-58). Thus a future injector Error is caught and logged but still disables Connect, contradicting the required criterion that it be “a logged, contained injection failure instead of a plugin disable.”

🔧 Fix: Clarified injector-failure wording and verified focused core test
2 warnings still open:

  • ⚠️ core/src/main/java/com/minekube/connect/ConnectPlatform.java:139 - The updated inline comment leaves the old fragment “Letting that escape onEnable() makes the platform” immediately before “Catch it here…”, producing a malformed and misleading explanation of the catch behavior. Rewrite the paragraph as one coherent statement.
  • ⚠️ spigot/src/main/java/com/minekube/connect/inject/spigot/SpigotInjector.java:339 - This Javadoc says every failure degrades, but then claims a missing accessor surfaces as NoSuchMethodError and causes enablement failure. With reflective lookup, missing names produce NoSuchMethodException, are skipped, and the wrapper is returned with a warning. Mark the NoSuchMethodError statement as the pre-fix behavior or remove it to keep the operational documentation accurate.

🔧 Fix: Fixed malformed catch comment and stale unwrap Javadoc
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • ./gradlew :spigot:test --tests com.minekube.connect.inject.spigot.SpigotInjectorViaLegacyPathTest :core:test --tests com.minekube.connect.ConnectPlatformEnableFailureContainmentTest
  • ./gradlew :spigot:shadowJar --console=plain
  • Shaded JAR inspection for packaged ViaVersion classes and direct getOriginal() Methodref entries
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

On plain Spigot/CraftBukkit, ViaVersion wraps the server's child handler with
BukkitChannelInitializer (its legacy injector; on Paper it registers a
ChannelInitializeListener instead, which Connect picks up for free). SpigotInjector
unwrapped that with getOriginal(), which ViaVersion 5.0 renamed to original() and
pulled up into WrappedChannelInitializer. Connect compiled against Via 4.0.0, so
this built fine and threw NoSuchMethodError at runtime with any Via >= 5.0.

That NoSuchMethodError is an Error, so it escaped the catch (Exception) in
ConnectPlatform.enable(), escaped onEnable(), and Bukkit disabled the plugin:
Connect never bound its local channel and no Connect player could join at all.
Latent since ViaVersion 5.0 and present in every release since.

Resolve the accessor reflectively by name (original(), then getOriginal()) so both
Via majors keep working, and degrade to skipping the unwrap - with a warning - if a
future rename removes both, rather than refusing to enable. Also widen the catch in
ConnectPlatform.enable() to Throwable so any injector Error stays a contained,
logged injection failure instead of a plugin disable.

The compile-only Via dep moves 4.0.0 -> 5.11.0 (plus viaversion-common, which now
holds the ViaChannelInitializer superclass); it is platform-provided, so this does
not change what the shaded jar ships or the minimum supported Via.

Verified: the shipped connect-spigot.jar unwraps correctly against both Via 4.0.0
and Via 5.11.0, and no longer references getOriginal() in its constant pool.
@robinbraemer
robinbraemer merged commit 0b44a3e into main Jul 26, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant