fix(core): prevent Paper startup config binding collision#59
Merged
Conversation
…tup crash The 0.12.0 Bedrock identity work (#57) wired BedrockIdentityEnforcer and BedrockIdentityKeyProvider with a direct dependency on ConnectConfig. Those components are resolved by the platform injector while the platform is constructed, i.e. before ConnectPlatform.init() loads the config. ConnectConfig is only bound in the child injector that init() creates via ConfigLoadedModule, so resolving the Bedrock graph from the parent injector forced Guice to create a just-in-time ConnectConfig binding on the parent, which then collided with the child binding: [Guice/JitBindingAlreadySet]: A just-in-time binding to ConnectConfig was already configured on a parent injector at ConfigLoadedModule.config(...) The plugin crashed in SpigotPlugin.onLoad() before token initialization, so plugins/Connect/token.json was never created. Fix: the injected Bedrock components read ConnectConfig lazily through the parent-bound ConfigHolder (populated by init() before the config is used), so no ConnectConfig binding is ever established on the parent injector. The existing (ConnectConfig, ...) constructors are preserved for tests and direct use, and token precedence (CONNECT_TOKEN, existing token.json, else generate + persist) is unchanged. Verified end-to-end on Paper 26.2 (build 60) + Java 25 with a single plugin jar: - unfixed: JitBindingAlreadySet in onLoad, no token.json (reproduces the report with exactly one jar on a clean data dir, so the duplicate-jar theory is false) - fixed: reaches enabled state and creates token.json; restart reuses the token; an explicit CONNECT_TOKEN does not overwrite token.json Regression guard: core BedrockParentInjectorStartupTest.
…d diff checks pass
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
1 participant
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
Fix the connect-java 0.12.0 Paper startup regression: the plugin crashed in SpigotPlugin.onLoad() with Guice [JitBindingAlreadySet] on ConnectConfig, before token initialization, so plugins/Connect/token.json was never created. Root cause: PR #57's Bedrock identity components (BedrockIdentityEnforcer, BedrockIdentityKeyProvider) are resolved by the platform injector while SpigotPlatform is constructed, i.e. before ConnectPlatform.init() loads the config, yet they injected ConnectConfig directly. ConnectConfig is only bound in the child injector that init() creates via ConfigLoadedModule, so resolving the Bedrock graph from the parent injector forced Guice to create a just-in-time ConnectConfig binding on the parent that then collided with the child binding. Fix: the injected Bedrock components now read ConnectConfig lazily through the parent-bound ConfigHolder (populated by init() before the config is used), so no ConnectConfig binding is ever established on the parent injector. Deliberate decisions a reviewer should know: (1) all existing (ConnectConfig, ...) constructors are intentionally preserved so ~40 existing test call-sites and direct callers keep working; BedrockIdentityKeyProvider/BedrockIdentityEnforcer store a Supplier and the ConnectConfig constructors wrap it as a constant supplier. (2) BedrockIdentityKeyProvider was made @Inject @singleton and its CommonModule @provides removed so Guice binds it just-in-time; the readiness @provides now takes ConfigHolder. (3) Token precedence (CONNECT_TOKEN env, then existing token.json, else generate+persist) is deliberately untouched. (4) Scope was intentionally limited to startup/injector/token init; a separate PRE-EXISTING libp2p runtime reflection error ('Failed to initialize Connect libp2p endpoint runtime') that surfaces later in enable() is gracefully caught (WatchService fallback) and was intentionally left out of scope. Added regression test core/BedrockParentInjectorStartupTest and an AGENTS.md injector-scoping note. Verified end-to-end on Paper 26.2 build 60 + Java 25 with a single plugin jar: unfixed crashes in onLoad with no token.json (falsifying the duplicate-jar theory on a one-jar clean data dir), fixed reaches enabled state and creates token.json, restart reuses the token, and an explicit CONNECT_TOKEN does not overwrite token.json.
What Changed
ConnectConfiglazily throughConfigHolder, while preserving existing constructors for compatibility.CONNECT_TOKENchecks.Risk Assessment
✅ Low: The requested compatibility overload is restored, and the remaining changes are narrowly scoped to lazy injector configuration without other material source risks found.
Testing
Focused and full tests, shaded-jar build, and real Paper 26.2 build 60/Java 25 runs verified token creation, restart reuse, environment-token non-overwrite, and absence of the startup crash; the known later libp2p reflection error was caught while the server reached ready state and was left out of scope, and no linters or static analysis were run.
Evidence: Paper startup evidence
Evidence: Startup symptom check
Evidence: Paper restart console
Evidence: CONNECT_TOKEN console
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
core/src/main/java/com/minekube/connect/bedrock/BedrockIdentityEnforcer.java:45- The intent requires that “all existing (ConnectConfig, ...) constructors are intentionally preserved.” The public constructorBedrockIdentityEnforcer(ConnectConfig, ConnectLogger, BedrockIdentityKeyProvider, BedrockAdmissionCoordinator)was replaced by aConfigHolderoverload at lines 44–51, breaking existing callers. Restore a delegating compatibility overload or confirm this intentional exception.🔧 Fix: Restore legacy Bedrock enforcer constructor compatibility
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
./gradlew :core:test --tests com.minekube.connect.module.BedrockParentInjectorStartupTest --tests com.minekube.connect.bedrock.BedrockIdentityEnforcerTest :spigot:test./gradlew test./gradlew :spigot:shadowJarPaper 26.2 build 60 launched with one shaded plugin jar on Java 25 in a clean data directoryRestarted Paper with the same data directory and compared token contents and mtimeLaunched Paper withCONNECT_TOKEN=explicit-tokenand compared the existing token file SHA-256 before/afterChecked Paper logs forJitBindingAlreadySetand verified no occurrences./gradlew cleanand./gradlew -p build-logic clean✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.