fix(phase): improve finish callback handling and allow multiple callbacks

src/main/java/net/theevilreaper/xerus/api/phase/LinearPhaseSeries.java

@@ -113,7 +113,7 @@ public boolean isLastPhase() {
      * Starts the currently selected phase and registers a callback to advance when it finishes.
      */
     public void startCurrentPhase() {
-        currentPhase.setFinishedCallback(this::advance);
+        currentPhase.setSeriesCallback(this::advance);
         currentPhase.start();
     }
 

src/main/java/net/theevilreaper/xerus/api/phase/Phase.java

@@ -1,12 +1,17 @@
 package net.theevilreaper.xerus.api.phase;
 
 import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.ArrayList;
+import java.util.List;
 
 /**
  * Represents a unit of execution within a phase-based system.
  * <p>
  * A {@code Phase} is started externally and can complete at any time. Once finished,
- * it can notify an optional listener via a {@code finishedCallback}.
+ * it notifies all registered finished callbacks.
  * </p>
  * <p>
  * Each phase has a unique name and maintains internal state indicating whether it is running,
@@ -19,11 +24,14 @@
  */
 public abstract class Phase {
 
+    private static final Logger PHASE_LOGGER = LoggerFactory.getLogger(Phase.class);
+
     private final String name;
+    private final List<Runnable> finishedCallbacks;
     private boolean running;
     private boolean finished;
     private boolean skipping;
-    private Runnable finishedCallback;
+    private Runnable seriesCallback;
 
     /**
      * Constructs a new {@code Phase} with the specified name.
@@ -32,6 +40,7 @@ public abstract class Phase {
      */
     protected Phase(@NotNull String name) {
         this.name = name;
+        this.finishedCallbacks = new ArrayList<>();
     }
 
     /**
@@ -64,9 +73,11 @@ public void onSkip() {
     }
 
     /**
-     * Marks the phase as finished and invokes the {@code finishedCallback}, if set.
+     * Marks the phase as finished and invokes all registered finished callbacks in registration order.
      * <p>
      * A phase can only be finished once. Subsequent calls will be ignored.
+     * An exception thrown by a callback is logged and does not prevent the remaining callbacks
+     * or the owning series from being notified.
      * </p>
      */
     public void finish() {
@@ -78,8 +89,29 @@ public void finish() {
         running = false;
         skipping = false;
 
-        if (finishedCallback != null) {
-            finishedCallback.run();
+        notifyFinishedCallbacks();
+
+        if (seriesCallback != null) {
+            seriesCallback.run();
+        }
+    }
+
+    /**
+     * Invokes all registered finished callbacks in registration order.
+     * <p>
+     * Iterates over a copy so callbacks may remove themselves while being invoked.
+     * Exceptions are logged and do not prevent the remaining callbacks from running.
+     * </p>
+     */
+    private void notifyFinishedCallbacks() {
+        if (finishedCallbacks.isEmpty()) return;
+
+        for (Runnable callback : List.copyOf(finishedCallbacks)) {
+            try {
+                callback.run();
+            } catch (Exception exception) {
+                PHASE_LOGGER.error("A finished callback of phase '{}' threw an exception", name, exception);
+            }
         }
     }
 
@@ -150,11 +182,48 @@ public void setSkipping(boolean skipping) {
     }
 
     /**
-     * Sets the callback to be invoked when this phase finishes.
+     * Registers a callback to be invoked when this phase finishes.
+     *
+     * @param callback the {@link Runnable} to be executed on phase completion
+     */
+    public void addFinishedCallback(@NotNull Runnable callback) {
+        this.finishedCallbacks.add(callback);
+    }
+
+    /**
+     * Removes a previously registered finished callback.
+     *
+     * @param callback the {@link Runnable} to remove
+     * @return {@code true} if the callback was registered, otherwise {@code false}
+     */
+    public boolean removeFinishedCallback(@NotNull Runnable callback) {
+        return this.finishedCallbacks.remove(callback);
+    }
+
+    /**
+     * Replaces all registered finished callbacks with the given one.
      *
-     * @param finishedCallback the {@link Runnable} to be executed on phase completion
+     * @param finishedCallback the {@link Runnable} to be executed on phase completion, or {@code null} to clear all callbacks
+     * @deprecated use {@link #addFinishedCallback(Runnable)} and {@link #removeFinishedCallback(Runnable)} instead
      */
+    @Deprecated(forRemoval = true)
     public void setFinishedCallback(Runnable finishedCallback) {
-        this.finishedCallback = finishedCallback;
+        this.finishedCallbacks.clear();
+        if (finishedCallback != null) {
+            this.finishedCallbacks.add(finishedCallback);
+        }
+    }
+
+    /**
+     * Sets the internal callback used by an owning {@link LinearPhaseSeries} to advance once this phase finishes.
+     * <p>
+     * It is kept separate from the user-defined finished callbacks so it can not be removed by them.
+     * The series callback is always invoked after all finished callbacks.
+     * </p>
+     *
+     * @param seriesCallback the {@link Runnable} to be executed by the owning series
+     */
+    void setSeriesCallback(Runnable seriesCallback) {
+        this.seriesCallback = seriesCallback;
     }
 }

src/test/java/net/theevilreaper/xerus/api/phase/CyclicPhaseSeriesTest.java

@@ -39,7 +39,7 @@ void testSetMaxIterations() {
    /* @Test
     void testSingleLoop() {
         var runnable = Mockito.mock(Runnable.class);
-        phaseSeries.setFinishedCallback(runnable);
+        phaseSeries.addFinishedCallback(runnable);
         phaseSeries.setMaxIterations(1);
         assertEquals(2, phaseSeries.getIterations());
 
@@ -57,7 +57,7 @@ void testSingleLoop() {
     @Test
     void testDoubleLoop() {
         var runnable = Mockito.mock(Runnable.class);
-        phaseSeries.setFinishedCallback(runnable);
+        phaseSeries.addFinishedCallback(runnable);
         phaseSeries.setMaxIterations(2);
 
         phaseSeries.start();

src/test/java/net/theevilreaper/xerus/api/phase/GamePhaseTest.java

@@ -0,0 +1,37 @@
+package net.theevilreaper.xerus.api.phase;
+
+import net.minestom.server.event.Event;
+import net.minestom.server.event.EventListener;
+import org.junit.jupiter.api.Test;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+
+class GamePhaseTest {
+
+    private static class TestEvent implements Event {
+    }
+
+    private static class DummyGamePhase extends GamePhase {
+        protected DummyGamePhase() {
+            super("Dummy");
+        }
+
+        @Override
+        protected void onStart() {
+        }
+    }
+
+    @Test
+    void testAddAndRemoveListener() {
+        DummyGamePhase phase = new DummyGamePhase();
+        AtomicBoolean called = new AtomicBoolean(false);
+
+        EventListener<TestEvent> listener = phase.addListener(TestEvent.class, event -> called.set(true));
+        assertNotNull(listener);
+
+        EventListener<? extends Event> removed = phase.removeListener(TestEvent.class);
+        assertNotNull(removed);
+    }
+}

src/test/java/net/theevilreaper/xerus/api/phase/PhaseCallbackTest.java

@@ -0,0 +1,139 @@
+package net.theevilreaper.xerus.api.phase;
+
+import org.junit.jupiter.api.Test;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+class PhaseCallbackTest {
+
+    @Test
+    void testLinearSeriesKeepsUserCallback() {
+        SimplePhase phase1 = new SimplePhase("Phase #1");
+        SimplePhase phase2 = new SimplePhase("Phase #2");
+        LinearPhaseSeries<SimplePhase> series = new LinearPhaseSeries<>("Series", new ArrayList<>(List.of(phase1, phase2)));
+
+        AtomicBoolean called = new AtomicBoolean(false);
+        phase1.addFinishedCallback(() -> called.set(true));
+
+        series.start();
+        phase1.finish();
+
+        assertTrue(called.get());
+        assertEquals(phase2, series.getCurrentPhase());
+    }
+
+    @Test
+    void testUserCallbackRunsBeforeAdvance() {
+        SimplePhase phase1 = new SimplePhase("Phase #1");
+        SimplePhase phase2 = new SimplePhase("Phase #2");
+        LinearPhaseSeries<SimplePhase> series = new LinearPhaseSeries<>("Series", new ArrayList<>(List.of(phase1, phase2)));
+
+        List<String> order = new ArrayList<>();
+        phase1.addFinishedCallback(() -> order.add("phase1"));
+        phase2.addFinishedCallback(() -> order.add("phase2"));
+        series.addFinishedCallback(() -> order.add("series"));
+
+        series.start();
+        phase1.finish();
+        assertFalse(phase2.isFinished());
+        phase2.finish();
+
+        assertEquals(List.of("phase1", "phase2", "series"), order);
+        assertTrue(series.isFinished());
+    }
+
+    @Test
+    void testCyclicSeriesKeepsUserCallback() {
+        SimplePhase phase1 = new SimplePhase("Phase #1");
+        SimplePhase phase2 = new SimplePhase("Phase #2");
+        CyclicPhaseSeries<SimplePhase> series = new CyclicPhaseSeries<>("Series", new ArrayList<>(List.of(phase1, phase2)));
+        series.setMaxIterations(2);
+
+        List<String> calls = new ArrayList<>();
+        phase1.addFinishedCallback(() -> calls.add("phase1"));
+
+        series.start();
+        phase1.finish();
+
+        assertEquals(List.of("phase1"), calls);
+        assertEquals(phase2, series.getCurrentPhase());
+    }
+
+    @Test
+    void testMultipleCallbacksRunInOrder() {
+        SimplePhase phase = new SimplePhase("Phase");
+        List<String> order = new ArrayList<>();
+        phase.addFinishedCallback(() -> order.add("first"));
+        phase.addFinishedCallback(() -> order.add("second"));
+
+        phase.finish();
+
+        assertEquals(List.of("first", "second"), order);
+    }
+
+    @Test
+    void testRemoveFinishedCallback() {
+        SimplePhase phase = new SimplePhase("Phase");
+        AtomicBoolean called = new AtomicBoolean(false);
+        Runnable callback = () -> called.set(true);
+        phase.addFinishedCallback(callback);
+
+        assertTrue(phase.removeFinishedCallback(callback));
+        assertFalse(phase.removeFinishedCallback(callback));
+
+        phase.finish();
+        assertFalse(called.get());
+    }
+
+    @Test
+    void testCallbackCanRemoveItselfDuringFinish() {
+        SimplePhase phase = new SimplePhase("Phase");
+        AtomicBoolean secondCalled = new AtomicBoolean(false);
+        phase.addFinishedCallback(new Runnable() {
+            @Override
+            public void run() {
+                phase.removeFinishedCallback(this);
+            }
+        });
+        phase.addFinishedCallback(() -> secondCalled.set(true));
+
+        assertDoesNotThrow(phase::finish);
+        assertTrue(secondCalled.get());
+    }
+
+    @Test
+    void testFailingCallbackDoesNotBlockOthersOrSeries() {
+        SimplePhase phase1 = new SimplePhase("Phase #1");
+        SimplePhase phase2 = new SimplePhase("Phase #2");
+        LinearPhaseSeries<SimplePhase> series = new LinearPhaseSeries<>("Series", new ArrayList<>(List.of(phase1, phase2)));
+
+        AtomicBoolean secondCalled = new AtomicBoolean(false);
+        phase1.addFinishedCallback(() -> {
+            throw new IllegalStateException("Broken callback");
+        });
+        phase1.addFinishedCallback(() -> secondCalled.set(true));
+
+        series.start();
+        assertDoesNotThrow(phase1::finish);
+
+        assertTrue(secondCalled.get());
+        assertEquals(phase2, series.getCurrentPhase());
+    }
+
+    @Test
+    @SuppressWarnings("removal")
+    void testDeprecatedSetterReplacesCallbacks() {
+        SimplePhase phase = new SimplePhase("Phase");
+        List<String> calls = new ArrayList<>();
+        phase.addFinishedCallback(() -> calls.add("old"));
+        phase.setFinishedCallback(() -> calls.add("new"));
+
+        phase.finish();
+
+        assertEquals(List.of("new"), calls);
+    }
+}

src/test/java/net/theevilreaper/xerus/api/phase/PhaseStructureTest.java

@@ -23,7 +23,7 @@ protected void onStart() {
                 throw new RuntimeException("Start called");
             }
         };
-        this.phase.setFinishedCallback(() -> {
+        this.phase.addFinishedCallback(() -> {
             throw new RuntimeException("Finish callback");
         });
     }
@@ -49,7 +49,7 @@ void testStartMethod() {
     @Order(3)
     @Test
     void testFinishMethod() {
-        assertThrowsExactly(RuntimeException.class, phase::finish, "Finish callback");
+        assertDoesNotThrow(phase::finish);
     }
 
     @Order(4)
@@ -92,6 +92,7 @@ void testSetSkipping() {
 
     @Order(10)
     @Test
+    @SuppressWarnings("removal")
     void testSetFinishCallback() {
         this.phase.setFinishedCallback(null);
         this.phase.finish();

src/test/java/net/theevilreaper/xerus/api/phase/PhaseTest.java

@@ -45,7 +45,7 @@ void testStartDouble() {
     void startFinishCallback() {
         var phase = new SimplePhase("Test=");
         var callback = Mockito.mock(Runnable.class);
-        phase.setFinishedCallback(callback);
+        phase.addFinishedCallback(callback);
 
         phase.start();
         phase.finish();
@@ -94,7 +94,7 @@ void isFinishedFull() {
     void testDoubleFinished() {
         var phase = new SimplePhase("Test=");
         var callback = Mockito.mock(Runnable.class);
-        phase.setFinishedCallback(callback);
+        phase.addFinishedCallback(callback);
 
         phase.finish();
         phase.finish();