From 79c29243044672437596f428392c5982bd67b579 Mon Sep 17 00:00:00 2001 From: Brett Wooldridge Date: Fri, 4 Sep 2026 19:29:41 +0900 Subject: [PATCH] fix: skip documents removed between an index lookup and the fetch An index scan is two steps that are not atomic under concurrent writes: the index yields the matching ids, then each document is fetched from the collection map. A document removed in between came back as a (id, null) row. With a residual filter that row reached Filter.apply and threw NullPointerException from the filter's document.get(); without one the cursor handed the caller a null element. The by-id fast path had the same window between containsKey and get. - IndexedStream now prefetches and drops ids whose document is gone; skip() still walks ids without fetching them. - FilteredStream treats a row without a document as a non-match. - The by-id path does one get and yields an empty stream when it is null. The test removes a document from the collection's map behind the index's back, which is what a concurrent remove looks like to a reader that already holds the ids; on the unfixed code its indexed-plus-residual case throws and the others return null rows. Co-Authored-By: Claude Fable 5.1 --- .../collection/operation/ReadOperations.java | 11 +-- .../no2/common/streams/FilteredStream.java | 3 +- .../no2/common/streams/IndexedStream.java | 33 ++++++- .../IndexScanRemovedDocumentTest.java | 91 +++++++++++++++++++ 4 files changed, 127 insertions(+), 11 deletions(-) create mode 100644 nitrite/src/test/java/org/dizitart/no2/collection/IndexScanRemovedDocumentTest.java diff --git a/nitrite/src/main/java/org/dizitart/no2/collection/operation/ReadOperations.java b/nitrite/src/main/java/org/dizitart/no2/collection/operation/ReadOperations.java index 7d9c838e7..0602f8d06 100644 --- a/nitrite/src/main/java/org/dizitart/no2/collection/operation/ReadOperations.java +++ b/nitrite/src/main/java/org/dizitart/no2/collection/operation/ReadOperations.java @@ -222,12 +222,11 @@ private RecordStream> findSuitableStream(FindPlan find NitriteId nitriteId = idValue instanceof Long ? NitriteId.createId((long) idValue) : NitriteId.createId(String.valueOf(idValue)); - if (nitriteMap.containsKey(nitriteId)) { - Document document = nitriteMap.get(nitriteId); - rawStream = RecordStream.single(pair(nitriteId, document)); - } else { - rawStream = RecordStream.empty(); - } + // one lookup: a document removed between containsKey and get would be a null row + Document document = nitriteMap.get(nitriteId); + rawStream = document == null + ? RecordStream.empty() + : RecordStream.single(pair(nitriteId, document)); } else { IndexDescriptor indexDescriptor = findPlan.getIndexDescriptor(); if (indexDescriptor != null) { diff --git a/nitrite/src/main/java/org/dizitart/no2/common/streams/FilteredStream.java b/nitrite/src/main/java/org/dizitart/no2/common/streams/FilteredStream.java index 9ee59bf4e..8f99bfce8 100644 --- a/nitrite/src/main/java/org/dizitart/no2/common/streams/FilteredStream.java +++ b/nitrite/src/main/java/org/dizitart/no2/common/streams/FilteredStream.java @@ -89,7 +89,8 @@ public void remove() { private boolean setNextId() { while (iterator.hasNext()) { final Pair pair = iterator.next(); - if (filter.apply(pair)) { + // a row whose document is gone is not a match for anything + if (pair.getSecond() != null && filter.apply(pair)) { nextPair = pair; nextPairSet = true; return true; diff --git a/nitrite/src/main/java/org/dizitart/no2/common/streams/IndexedStream.java b/nitrite/src/main/java/org/dizitart/no2/common/streams/IndexedStream.java index be6c2335a..285d8f7c8 100644 --- a/nitrite/src/main/java/org/dizitart/no2/common/streams/IndexedStream.java +++ b/nitrite/src/main/java/org/dizitart/no2/common/streams/IndexedStream.java @@ -24,6 +24,7 @@ import org.dizitart.no2.store.NitriteMap; import java.util.Iterator; +import java.util.NoSuchElementException; import java.util.Set; /** @@ -49,6 +50,7 @@ private static class IndexedStreamIterator implements Iterator iterator; private final NitriteMap nitriteMap; + private Pair next; IndexedStreamIterator(Iterator iterator, NitriteMap nitriteMap) { @@ -58,7 +60,7 @@ private static class IndexedStreamIterator implements Iterator 0) { + next = null; + skipped++; + } while (skipped < count && iterator.hasNext()) { iterator.next(); skipped++; @@ -77,9 +83,28 @@ public long skip(long count) { @Override public Pair next() { - NitriteId id = iterator.next(); - Document document = nitriteMap.get(id); - return new Pair<>(id, document); + if (next == null && !advance()) { + throw new NoSuchElementException(); + } + Pair current = next; + next = null; + return current; + } + + /** + * A document removed between the index lookup and the fetch is no longer a row: it is + * skipped rather than handed downstream as a null for a residual filter to dereference. + */ + private boolean advance() { + while (iterator.hasNext()) { + NitriteId id = iterator.next(); + Document document = nitriteMap.get(id); + if (document != null) { + next = new Pair<>(id, document); + return true; + } + } + return false; } } } diff --git a/nitrite/src/test/java/org/dizitart/no2/collection/IndexScanRemovedDocumentTest.java b/nitrite/src/test/java/org/dizitart/no2/collection/IndexScanRemovedDocumentTest.java new file mode 100644 index 000000000..04dad0731 --- /dev/null +++ b/nitrite/src/test/java/org/dizitart/no2/collection/IndexScanRemovedDocumentTest.java @@ -0,0 +1,91 @@ +/* + * Copyright (c) 2017-2020. Nitrite author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.dizitart.no2.collection; + +import org.dizitart.no2.Nitrite; +import org.dizitart.no2.filters.Filter; +import org.dizitart.no2.index.IndexOptions; +import org.dizitart.no2.index.IndexType; +import org.dizitart.no2.store.NitriteMap; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.util.List; + +import static org.dizitart.no2.filters.FluentFilter.where; +import static org.dizitart.no2.integration.TestUtil.createDb; +import static org.junit.Assert.*; + +/** + * An index lookup and the fetch of the documents it names are not atomic: a document removed in + * between must simply be absent from the result, never a null row that a residual filter, or the + * caller, dereferences. + *

+ * The window is reproduced by removing the document from the collection's map behind the index's + * back, which is exactly what a concurrent remove looks like to a reader that already holds the ids. + */ +public class IndexScanRemovedDocumentTest { + private Nitrite db; + private NitriteCollection collection; + private NitriteId removedId; + + @Before + public void setUp() { + db = createDb(); + collection = db.getCollection("race"); + collection.createIndex(IndexOptions.indexOptions(IndexType.NON_UNIQUE), "group"); + for (int n = 1; n <= 3; n++) { + collection.insert(Document.createDocument("group", "a").put("n", n)); + } + removedId = collection.find(where("n").eq(2)).firstOrNull().getId(); + + NitriteMap map = db.getStore().openMap("race", NitriteId.class, Document.class); + assertNotNull(map.remove(removedId)); + } + + @After + public void tearDown() { + db.close(); + } + + @Test + public void testIndexScanWithResidualFilterSkipsRemovedDocument() { + List found = collection.find(where("group").eq("a").and(where("n").gte(1))).toList(); + assertEquals(2, found.size()); + assertTrue(found.stream().noneMatch(d -> d == null)); + assertTrue(found.stream().noneMatch(d -> d.getId().equals(removedId))); + } + + @Test + public void testIndexScanWithoutResidualFilterSkipsRemovedDocument() { + List found = collection.find(where("group").eq("a")).toList(); + assertEquals(2, found.size()); + assertTrue(found.stream().noneMatch(d -> d == null)); + } + + @Test + public void testIndexScanSizeExcludesRemovedDocument() { + assertEquals(2, collection.find(where("group").eq("a").and(where("n").gte(1))).size()); + } + + @Test + public void testByIdOfRemovedDocumentIsEmpty() { + assertNull(collection.find(Filter.byId(removedId)).firstOrNull()); + assertNull(collection.getById(removedId)); + } +}