fix: one index instance and one map wrapper per name, and a drop that cannot run twice (#1309)

The first read of a non-unique index after a store reopens runs the lazy layout
migration in SingleFieldIndex, which drops the legacy map. That migration is
guarded per SingleFieldIndex instance, but ComparableIndexer.findNitriteIndex
created instances with an unsynchronized check-then-act, so several threads
arriving at once each got their own instance and each ran the migration.
NitriteMVMap.drop() likewise tested its dropped flag and then ignored the result
of its compare-and-set, and NitriteMVStore.openMap handed out one wrapper per
caller. Once the first thread had removed the map, MVMap.getName() answered null
for the rest, and NitriteMVStore.removeMap(null) died in ConcurrentHashMap.remove:

  NullPointerException: Cannot invoke "Object.hashCode()" because "key" is null
    at NitriteMVStore.removeMap
    at NitriteMVMap.drop
    at SingleFieldIndex.migrateLegacyIndex

A second shape of the same race is a NullPointerException from Attributes.set
via NitriteMap.updateLastModifiedTime, which read getName() several times.

Seen on a production system on the first multi-threaded lookup after every
restart, since every close left an empty map under the legacy name (fixed by
#1295) and every start had to drop it again.

- ComparableIndexer registers the index with computeIfAbsent, so the per-instance
  guard in SingleFieldIndex is the guard.
- NitriteMVStore.openMap and openRTree register the wrapper with computeIfAbsent,
  so every holder shares one dropped flag; removeMap ignores a null name and does
  not open (and thereby create) a map that is no longer in the store.
- NitriteMVMap captures its name at open and acts only when its compare-and-set
  succeeds, so drop() and close() run once and never ask MVMap for a name it no
  longer has.
- NitriteMap.updateLastModifiedTime reads the name once.

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

Author: brettwooldridge

nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVMap.java

@@ -46,6 +46,9 @@
 class NitriteMVMap<Key, Value> implements NitriteMap<Key, Value> {
 
     private final MVMap<Key, Value> mvMap;
+    // captured at open: MVMap.getName() answers null once the map is removed from the store,
+    // which is exactly when the registry and the catalog still have to be told the name
+    private final String name;
     private final NitriteStore<?> nitriteStore;
     private final MVStore mvStore;
     private final AtomicBoolean droppedFlag;
@@ -54,6 +57,7 @@ class NitriteMVMap<Key, Value> implements NitriteMap<Key, Value> {
 
     NitriteMVMap(final MVMap<Key, Value> mvMap, final NitriteStore<?> nitriteStore) {
         this.mvMap = mvMap;
+        this.name = mvMap.getName();
         this.nitriteStore = nitriteStore;
         this.mvStore = mvMap.getStore();
         this.closedFlag = new AtomicBoolean(false);
@@ -89,7 +93,7 @@ public void clear() {
 
     @Override
     public String getName() {
-        return mvMap.getName();
+        return name;
     }
 
     @Override
@@ -254,15 +258,16 @@ public boolean isEmpty() {
 
     @Override
     public void drop() {
-        if (!droppedFlag.get()) {
-            droppedFlag.compareAndSet(false, true);
-            closedFlag.compareAndSet(false, true);
+        // the compare-and-set is the guard: two threads that both saw the flag clear must not
+        // both remove the map
+        if (droppedFlag.compareAndSet(false, true)) {
+            closedFlag.set(true);
             releaseVersionUsages();
 
             final MVStore.TxCounter txCounter = mvStore.registerVersionUsage();
             try {
-                nitriteStore.closeMap(mvMap.getName());
-                nitriteStore.removeMap(mvMap.getName());
+                nitriteStore.closeMap(name);
+                nitriteStore.removeMap(name);
             } finally {
                 mvStore.deregisterVersionUsage(txCounter);
             }
@@ -276,10 +281,9 @@ public boolean isDropped() {
 
     @Override
     public void close() {
-        if (!closedFlag.get() && !droppedFlag.get()) {
-            closedFlag.compareAndSet(false, true);
+        if (!droppedFlag.get() && closedFlag.compareAndSet(false, true)) {
             releaseVersionUsages();
-            nitriteStore.closeMap(mvMap.getName());
+            nitriteStore.closeMap(name);
         }
     }
 

nitrite-mvstore-adapter/src/main/java/org/dizitart/no2/mvstore/NitriteMVStore.java

@@ -147,14 +147,12 @@ public boolean hasMap(String mapName) {
     @Override
     @SuppressWarnings("unchecked")
     public <Key, Value> NitriteMap<Key, Value> openMap(String mapName, Class<?> keyType, Class<?> valueType) {
-        if (nitriteMapRegistry.containsKey(mapName)) {
-            return (NitriteMVMap<Key, Value>) nitriteMapRegistry.get(mapName);
-        }
-
-        MVMap<Key, Value> mvMap = openMVMap(mapName, null);
-        NitriteMVMap<Key, Value> nitriteMVMap = new NitriteMVMap<>(mvMap, this);
-        nitriteMapRegistry.put(mapName, nitriteMVMap);
-        return nitriteMVMap;
+        // one wrapper per map, however many threads open it at once, so a drop() or close()
+        // through any holder is the drop or close every holder sees
+        return (NitriteMVMap<Key, Value>) nitriteMapRegistry.computeIfAbsent(mapName, name -> {
+            MVMap<Key, Value> mvMap = openMVMap(name, null);
+            return new NitriteMVMap<>(mvMap, this);
+        });
     }
 
     @Override
@@ -173,8 +171,15 @@ public void closeRTree(String rTreeName) {
 
     @Override
     public void removeMap(String name) {
-        MVMap<?, ?> mvMap = openMVMap(name, null);
-        mvStore.removeMap(mvMap);
+        if (StringUtils.isNullOrEmpty(name)) {
+            return;
+        }
+        // a map another thread has already removed is simply gone; openMVMap would create an
+        // empty map of that name only to remove it again
+        if (mvStore.hasMap(name)) {
+            MVMap<?, ?> mvMap = openMVMap(name, null);
+            mvStore.removeMap(mvMap);
+        }
         getCatalog().remove(name);
         nitriteMapRegistry.remove(name);
     }
@@ -191,14 +196,10 @@ public void removeRTree(String rTreeName) {
     @Override
     @SuppressWarnings({"unchecked", "rawtypes"})
     public <Key extends BoundingBox, Value> NitriteRTree<Key, Value> openRTree(String mapName, Class<?> keyType, Class<?> valueType) {
-        if (nitriteRTreeMapRegistry.containsKey(mapName)) {
-            return (NitriteMVRTreeMap) nitriteRTreeMapRegistry.get(mapName);
-        }
-
-        MVRTreeMap<Value> map = (MVRTreeMap<Value>) openMVMap(mapName, new MVRTreeMap.Builder<>());
-        NitriteMVRTreeMap<Key, Value> nitriteMVRTreeMap = new NitriteMVRTreeMap(map, this);
-        nitriteRTreeMapRegistry.put(mapName, nitriteMVRTreeMap);
-        return nitriteMVRTreeMap;
+        return (NitriteMVRTreeMap) nitriteRTreeMapRegistry.computeIfAbsent(mapName, name -> {
+            MVRTreeMap<Value> map = (MVRTreeMap<Value>) openMVMap(name, new MVRTreeMap.Builder<>());
+            return new NitriteMVRTreeMap(map, this);
+        });
     }
 
     @Override

nitrite/src/main/java/org/dizitart/no2/index/ComparableIndexer.java

@@ -105,17 +105,11 @@ private NitriteIndex findNitriteIndex(IndexDescriptor indexDescriptor, NitriteCo
             throw new IndexingException("Index descriptor cannot be null");
         }
 
-        if (indexRegistry.containsKey(indexDescriptor)) {
-            return indexRegistry.get(indexDescriptor);
-        }
-
-        NitriteIndex nitriteIndex;
-        if (indexDescriptor.isCompoundIndex()) {
-            nitriteIndex = new CompoundIndex(indexDescriptor, nitriteConfig.getNitriteStore());
-        } else {
-            nitriteIndex = new SingleFieldIndex(indexDescriptor, nitriteConfig.getNitriteStore());
-        }
-        indexRegistry.put(indexDescriptor, nitriteIndex);
-        return nitriteIndex;
+        // One instance per descriptor, however many threads ask for it at once. The lazy layout
+        // migration in SingleFieldIndex is guarded per instance, so two instances for the same
+        // index would each migrate and drop the legacy map, and the second drop fails.
+        return indexRegistry.computeIfAbsent(indexDescriptor, descriptor -> descriptor.isCompoundIndex()
+            ? new CompoundIndex(descriptor, nitriteConfig.getNitriteStore())
+            : new SingleFieldIndex(descriptor, nitriteConfig.getNitriteStore()));
     }
 }

nitrite/src/main/java/org/dizitart/no2/store/NitriteMap.java

@@ -243,15 +243,16 @@ default void setAttributes(Attributes attributes) {
      */
     default void updateLastModifiedTime() {
         if (!isDropped()) {
-            if (isNullOrEmpty(getName())
-                || META_MAP_NAME.equals(getName())) return;
+            // read once: an adapter may answer null as soon as the map is removed from the store
+            String name = getName();
+            if (isNullOrEmpty(name) || META_MAP_NAME.equals(name)) return;
 
             NitriteMap<String, Attributes> metaMap = getStore().openMap(META_MAP_NAME, String.class, Attributes.class);
             if (metaMap != null) {
-                Attributes attributes = metaMap.get(getName());
+                Attributes attributes = metaMap.get(name);
                 if (attributes == null) {
-                    attributes = new Attributes(getName());
-                    metaMap.put(getName(), attributes);
+                    attributes = new Attributes(name);
+                    metaMap.put(name, attributes);
                 }
                 attributes.set(Attributes.LAST_MODIFIED_TIME, Long.toString(System.currentTimeMillis()));
             }