fix(engine) #5636: capture the departing counters before the removal, not after

Round 3 review on #5640. Point 1 was a real bug, not polish.

unregisterDatabase removed the database from the set FIRST and folded its counters SECOND. A throw from
accumulateMonotonic in between dropped the database out of `databases` with its contribution never folded into
retainedStats, so the next scrape came back lower than the last - which, now that these series are typed as
Prometheus counters, is read as a reset and fabricates exactly the rate() spike the baseline exists to prevent.
The old comment called that "the pre-#5636 behaviour", which understated it: before, a close decreased a GAUGE
and meant nothing. Capture now happens into a temp before the removal, so the window is closed rather than
argued away. The one case that still steps back - the capture itself failing, where the database has to leave
the registry regardless or every later toJSON() throws on the same dead source - is logged at WARNING instead of
FINE, because it is the only remaining path that can produce the artifact and an operator staring at an
unexplained spike needs that line.

Point 2: statOf defaulting a missing key to 0 is right for the scrape path but means a typo in DB_STAT_KEYS
degrades to a silently wrong total, and nothing else would catch it - every other assertion reads through the
same defaulting. Now checked against a real getStats() map, reading Profiler's OWN key list reflectively; a
second copy of the list in the test would have passed happily while production carried the typo. Verified by
planting one.

Point 3: metersKeepReportingAfterTheBinderIsCollected did not pin what it claimed. System.gc() is a hint, and
since the meters read through the static cache the values came back either way - so it would have kept passing
if someone regressed CACHE to an instance field, the exact bug it existed to catch. Replaced with a structural
assertion that no field of the binder is an instance field, which does fail on that regression (verified by
making CACHE non-static).

Points 4 and 5 were confirmations of deliberate choices; no change.

Author: lvca

engine/src/main/java/com/arcadedb/Profiler.java

@@ -102,21 +102,36 @@ public synchronized void registerDatabase(final DatabaseInternal database) {
    * lock-free, so no database lock can be on the other side of the wait.
    */
   public synchronized void unregisterDatabase(final DatabaseInternal database) {
-    if (!databases.remove(database))
+    if (!databases.contains(database))
       // ALREADY UNREGISTERED: DO NOT FOLD ITS COUNTERS IN TWICE
       return;
 
+    // CAPTURE BEFORE REMOVING, NOT AFTER. Removing first and folding second leaves a window in which a throw from
+    // accumulateMonotonic drops the database out of `databases` with its contribution never folded into
+    // retainedStats - so the next scrape is LOWER than the last, which a Prometheus counter reads as a reset and
+    // turns into a fabricated rate() spike. That is the exact artifact this baseline exists to prevent, so the
+    // ordering below closes the window rather than relying on the read not to fail.
+    final long[] departing = new long[STATS_COUNT];
+    boolean captured = false;
     try {
-      final long[] departing = new long[STATS_COUNT];
       accumulateMonotonic(departing, database);
-      for (int i = 0; i < MONOTONIC_STATS; i++)
-        retainedStats[i] += departing[i];
+      captured = true;
     } catch (final Exception e) {
-      // THE CALLER IS MID-CLOSE: A STAT SOURCE ALREADY TORN DOWN MUST NOT TAKE THE CLOSE PATH DOWN WITH IT. THE ONLY
-      // COST IS THAT THIS DATABASE'S CONTRIBUTION IS LOST, WHICH IS THE PRE-#5636 BEHAVIOUR FOR EVERY DATABASE.
+      // THE CALLER IS MID-CLOSE: A STAT SOURCE ALREADY TORN DOWN MUST NOT TAKE THE CLOSE PATH DOWN WITH IT. The
+      // database still has to leave the registry - keeping it would make every later toJSON() throw on the same
+      // dead source - so this one case genuinely does decrease the totals, and on a typed counter that reads as a
+      // reset. Logged at WARNING, not FINE: it is the only path that can still produce the artifact, and an
+      // operator seeing an unexplained rate spike needs this line to explain it.
       LogManager.instance()
-          .log(this, Level.FINE, "Could not retain the profiler counters of a closing database", e);
+          .log(this, Level.WARNING, "Could not retain the profiler counters of a closing database: the engine "
+              + "totals will step back by its contribution, which appears as a counter reset in Prometheus", e);
     }
+
+    databases.remove(database);
+
+    if (captured)
+      for (int i = 0; i < MONOTONIC_STATS; i++)
+        retainedStats[i] += departing[i];
   }
 
   /**

engine/src/test/java/com/arcadedb/Issue5636ProfilerMonotonicTest.java

@@ -28,6 +28,8 @@
 import org.junit.jupiter.api.Test;
 
 import java.io.File;
+import java.lang.reflect.Field;
+import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -97,9 +99,42 @@ void closingADatabaseDoesNotRewindTheJvmWideCounters() {
     }
   }
 
+  /**
+   * {@code statOf} degrades a missing or non-numeric key to 0 so a metrics scrape survives it. That is the right
+   * trade at runtime, but it also means a typo in {@code DB_STAT_KEYS} - or a source key someone renames later -
+   * silently produces a wrong total instead of failing. Nothing else would catch it: every other assertion here
+   * reads through the same defaulting. So the key list is checked against a real stats map.
+   */
+  @Test
+  void everyAccumulatedStatKeyExistsInARealStatsMap() throws Exception {
+    // Read Profiler's OWN key list, not a copy of it: a second copy here would pass happily while the production
+    // list carried the typo. This is the one assertion that has to reach into the class under test.
+    final Field keysField = Profiler.class.getDeclaredField("DB_STAT_KEYS");
+    keysField.setAccessible(true);
+    final String[] dbStatKeys = (String[]) keysField.get(null);
+    assertThat(dbStatKeys).isNotEmpty();
+
+    try (final DatabaseFactory factory = new DatabaseFactory(DB_PATH)) {
+      final DatabaseInternal db = (DatabaseInternal) factory.create();
+      try {
+        final Map<String, Object> dbStats = db.getStats();
+        for (final String key : dbStatKeys)
+          assertThat(dbStats.get(key)).as("Profiler.DB_STAT_KEYS entry '%s' is not in DatabaseInternal.getStats()", key)
+              .isInstanceOf(Number.class);
+
+        final Map<String, Object> walStats = db.getTransactionManager().getStats();
+        for (final String key : new String[] { "pagesWritten", "bytesWritten", "logFiles" })
+          assertThat(walStats.get(key)).as("Profiler reads WAL stat '%s', which TransactionManager does not emit", key)
+              .isInstanceOf(Number.class);
+      } finally {
+        db.drop();
+      }
+    }
+  }
+
   /**
    * A database that is never registered - or is unregistered twice - must not double-count. Guards the
-   * {@code if (!databases.remove(database)) return;} short-circuit that makes the fold idempotent.
+   * {@code if (!databases.contains(database)) return;} short-circuit that makes the fold idempotent.
    */
   @Test
   void unregisteringTwiceFoldsTheCountersOnlyOnce() {

server/src/test/java/com/arcadedb/server/monitor/EngineMetricsBinderTest.java

@@ -23,6 +23,9 @@
 import io.micrometer.core.instrument.simple.SimpleMeterRegistry;
 import org.junit.jupiter.api.Test;
 
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+
 import static org.assertj.core.api.Assertions.assertThat;
 
 /**
@@ -108,11 +111,23 @@ void registersPageMergeCounters() {
   /**
    * Micrometer's {@code (obj, fn)} builders hold a WEAK reference to {@code obj}, and the only production caller does
    * {@code new EngineMetricsBinder().bindTo(registry)} - so anchoring the meters to the binder instance would let
-   * every one of them silently stop reporting at the next GC. The meters read through a static cache instead; this
-   * pins that they survive the binder becoming unreachable.
+   * every one of them silently stop reporting at the next GC.
+   * <p>
+   * Asserted structurally rather than by forcing a collection: {@code System.gc()} is only a hint, and because the
+   * meters read through the static cache the values come back either way - so a GC-based test would keep passing if
+   * someone regressed the cache to an instance field, which is the exact bug it is supposed to catch. What actually
+   * has to hold is that whatever the meters are anchored to outlives the binder, i.e. that no instance field of the
+   * binder can be the anchor.
    */
   @Test
-  void metersKeepReportingAfterTheBinderIsCollected() {
+  void theMetersAreAnchoredToSomethingThatOutlivesTheBinder() {
+    for (final Field field : EngineMetricsBinder.class.getDeclaredFields())
+      assertThat(Modifier.isStatic(field.getModifiers()))
+          .as("EngineMetricsBinder.%s is an instance field. Micrometer holds the meter's read target by WEAK "
+              + "reference and the binder is discarded right after bindTo(), so anchoring a meter here would make "
+              + "every engine metric stop reporting at the next GC.", field.getName())
+          .isTrue();
+
     final SimpleMeterRegistry registry = new SimpleMeterRegistry();
     new EngineMetricsBinder().bindTo(registry);