From 810293542adfd0a28c7a5ac10dbe4e07f82637a5 Mon Sep 17 00:00:00 2001 From: Andrew Wong <42793301+md5sha256@users.noreply.github.com> Date: Mon, 26 Oct 2026 23:40:13 +1100 Subject: [PATCH] docs(modules): define cleanup contract for a failed initialize The manager closes a module's class loader when initialize throws but never calls shutdown, and nothing said so. A module that registered a listener before failing left it live; this was observed in Keystone on Paper 26.3. Document that cleanup after a failed initialize is the module's job and that shutdown only follows a successful start, on PluginModule, SimplePluginModule, README and AGENTS.md. Pin it with a test. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_011DvdDwRoC7qHZ7yKUv1ufw --- AGENTS.md | 4 +++ README.md | 5 ++++ .../modules/PluginModule.java | 26 +++++++++++++++++++ .../modules/SimplePluginModule.java | 12 +++++++++ .../modules/ModuleLifecycleManagerTest.java | 16 ++++++++++++ 5 files changed, 63 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 4bde0b4..406270e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,6 +55,10 @@ outcome that cannot occur. - **Module reload = config refresh, not a jar swap.** `ModuleLifecycleManager.reloadAsync` calls the module's `PluginModule#reload`; it does **not** unload or replace the jar. Changed module code is only picked up by restarting the host plugin, and only manifests with `reloadable = true` refresh. +- **A failed `initialize` is never followed by `shutdown`.** The manager only logs and closes the + class loader, so a module must clean up its own partial registrations before throwing (documented + on `PluginModule#initialize`). `SimplePluginModule` tracks listeners but unregisters nothing by + itself. Changing this is a contract change for every module, not a bug fix. - **`ModuleLifecycleManager` is not thread-safe.** Its state must be touched from one thread (the server main thread on Paper). - **`SchemaMigrator` is abstract and dialect-agnostic.** It provides no SQL of its own — subclasses diff --git a/README.md b/README.md index 0a8a9ce..798bc40 100644 --- a/README.md +++ b/README.md @@ -51,6 +51,11 @@ by a `ModuleManifest` and loaded at runtime by its own `URLClassLoader`. - `ModuleLifecycleManager` — owns the runtime lifecycle: `start()`, `stop()`, `reloadAsync(moduleName)`, `getActiveModules()`. +> **Failed start:** if `initialize` throws, the manager closes the module's class loader but does +> **not** call `shutdown`. A module must do its fallible work before registering anything, or undo +> its registrations before rethrowing; otherwise its listeners and tasks stay live. `shutdown` is only +> called for modules that started. + > **Reload semantics:** a module reload is a **config refresh** (`PluginModule#reload`), not a jar > unload/reswap. Changed module code is only picked up by restarting the host plugin. Only modules > whose manifest sets `reloadable = true` are refreshed. diff --git a/src/main/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/PluginModule.java b/src/main/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/PluginModule.java index 947846c..9510487 100644 --- a/src/main/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/PluginModule.java +++ b/src/main/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/PluginModule.java @@ -8,8 +8,34 @@ public interface PluginModule { + /** + * Starts this module: reads its configuration and registers its listeners, tasks and commands + * with the host. + * + *

Failure is the module's to clean up. If this method throws, the + * {@link ModuleLifecycleManager} logs the failure and closes the module's class loader, but it + * does not call {@link #shutdown}. Anything registered before the failure stays live: + * a listener keeps firing with classes from a closed class loader, a task keeps running. An + * implementation must therefore either do all fallible work (reading config, resolving + * services) before registering anything, or undo what it registered before rethrowing. + * + * @param plugin the host plugin + * @param dataFolder this module's own data folder + * @throws ModuleInitializationException if the module cannot start; it must have released + * everything it registered before throwing + */ void initialize(@NotNull T plugin, @NotNull Path dataFolder) throws ModuleInitializationException; + /** + * Stops this module and releases everything {@link #initialize} registered. + * + *

Called once, by {@link ModuleLifecycleManager#stop()}, and only for a module whose + * {@code initialize} returned normally. It is never called after a failed {@code initialize}, + * so it may assume a fully started module. + * + * @param plugin the host plugin + * @throws ModuleShutdownException if releasing fails; the class loader is closed regardless + */ void shutdown(@NotNull T plugin) throws ModuleShutdownException; /** diff --git a/src/main/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/SimplePluginModule.java b/src/main/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/SimplePluginModule.java index f41418c..0d758b1 100644 --- a/src/main/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/SimplePluginModule.java +++ b/src/main/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/SimplePluginModule.java @@ -9,6 +9,18 @@ import java.util.ArrayList; import java.util.Collection; +/** + * A {@link PluginModule} base that keeps the host plugin and data folder, and tracks listeners + * registered through {@link #registerListener}. + * + *

Tracking is not cleanup: neither this class's {@link #initialize} nor its {@link #shutdown} + * unregisters anything. Following {@link PluginModule#initialize}'s contract, a subclass whose + * {@code initialize} fails after {@code registerListener} must call {@link #unregisterListeners()} + * before rethrowing, and its {@code shutdown} must call {@code unregisterListeners()} before + * {@code super.shutdown(plugin)}. + * + * @param the host plugin type + */ public abstract class SimplePluginModule implements PluginModule { private final Collection listeners = new ArrayList<>(); diff --git a/src/test/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/ModuleLifecycleManagerTest.java b/src/test/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/ModuleLifecycleManagerTest.java index fd60a0c..4ea58ec 100644 --- a/src/test/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/ModuleLifecycleManagerTest.java +++ b/src/test/java/com/minecraftcitiesnetwork/pluginInfrastructure/modules/ModuleLifecycleManagerTest.java @@ -175,6 +175,22 @@ void initializationFailureUnloads() throws Exception { assertTrue(logger.hasRecord(Level.SEVERE, "Failed to initialize module bad")); } + @Test + @DisplayName("a failed initialize is not followed by shutdown, neither at once nor on stop()") + void failedInitializeIsNeverShutDown() throws Exception { + // Pins the PluginModule contract: cleaning up after a failed initialize is the module's job. + List shutdowns = new ArrayList<>(); + FakeModule bad = new FakeModule("bad", shutdowns); + bad.initFailure = new RuntimeException("boom"); + stubLoader(List.of(loaded("bad", false, bad))); + + ModuleLifecycleManager manager = manager(); + manager.start(); + manager.stop(); + + assertTrue(shutdowns.isEmpty()); + } + @Test @DisplayName("stop() shuts modules down in reverse initialization order and clears state") void stopShutsDownInReverseOrder() throws Exception {