From 942c7b675ceabcd9532da71ff19f4cfb0ca52140 Mon Sep 17 00:00:00 2001 From: Florian Habermann Date: Fri, 7 Aug 2026 10:41:58 +0200 Subject: [PATCH 1/6] GigaMap: a held BitmapIndex reference resolves to the registered index, not to a detached one BitmapIndex#resolveFor returned this after merely validating the parent back-reference. That reference survives detachment: update(Indexer) replaces the registered instance, removeIndex drops it, and both release the dropped index' off-heap memory eagerly. A BitmapIndex obtained earlier from add() or get() therefore kept answering from a released, no longer maintained structure - silently as empty results rather than as a failure, because releasing nulls the segment slots instead of invalidating the object. Resolution is now by name and key type against the parent, so a held reference always reaches whatever is registered under it - which is what the identically named Indexer path (IndexIdentifier#resolveFor) has always done. A name that no longer resolves is reported instead of answered: the index was removed, or replaced by one with a different key type, and both are cases where returning nothing would be indistinguishable from a legitimately empty result. The foreign-parent guard is unchanged. This matters beyond the two existing entry points: making reindex() failure-atomic (internal#132) rebuilds each index into a replacement instance and swaps it in, which would otherwise turn every held reference stale as a side effect of a repair operation. --- .../store/gigamap/types/BitmapIndex.java | 35 +++- .../indexer/HeldIndexReferenceTest.java | 191 ++++++++++++++++++ 2 files changed, 220 insertions(+), 6 deletions(-) create mode 100644 gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/HeldIndexReferenceTest.java diff --git a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndex.java b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndex.java index d253e072..55ea9a32 100644 --- a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndex.java +++ b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndex.java @@ -160,17 +160,40 @@ public default boolean test(final E entity, final K key) } - @SuppressWarnings("unchecked") // in case of this, S == E, but the compiler cannot understand that. + /** + * {@inheritDoc} + *

+ * Holding a {@code BitmapIndex} is not the same as holding the index registered under its name: the + * registered instance is replaced by {@link BitmapIndices#update(Indexer)} and by a rebuild + * ({@link GigaMap#reindex()}), and dropped by {@link BitmapIndices#removeIndex(String)}. A replaced + * instance keeps its parent back-reference, so validating that reference is not enough to tell it apart + * from the live one - and its data has been released + * ({@link Internal#internalReleaseOffHeap()}), so answering from it would silently yield empty results. + * This therefore re-resolves by name rather than returning {@code this}. + * + * @throws BitmapIndexException if {@code parent} is not this index' parent, or if no index is registered + * under this index' name and key type any more + */ @Override public default Internal resolveFor(final BitmapIndices.Internal parent) { - // bitmap instance itself does not need to be resolved, but instead validated. - if(parent == this.parent()) + if(parent != this.parent()) { - return (Internal)this; + throw new BitmapIndexException("Invalid parent.", this); } - - throw new BitmapIndexException("Invalid parent.", this); + + final Internal registered = parent.internalGet(this.keyType(), this.name()); + if(registered == null) + { + throw new BitmapIndexException( + "Index \"" + this.name() + "\" is no longer registered under this name and key type " + + this.keyType() + ". It was removed, or replaced by an index with a different key type; " + + "resolve it again via the parent instead of holding the instance across such a change.", + this + ); + } + + return registered; } /** diff --git a/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/HeldIndexReferenceTest.java b/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/HeldIndexReferenceTest.java new file mode 100644 index 00000000..239f54c8 --- /dev/null +++ b/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/HeldIndexReferenceTest.java @@ -0,0 +1,191 @@ +package org.eclipse.store.gigamap.indexer; + +/*- + * #%L + * EclipseStore GigaMap + * %% + * Copyright (C) 2023 - 2026 MicroStream Software + * %% + * This program and the accompanying materials are made + * available under the terms of the Eclipse Public License 2.0 + * which is available at https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * #L% + */ + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.eclipse.store.gigamap.exceptions.BitmapIndexException; +import org.eclipse.store.gigamap.types.BitmapIndex; +import org.eclipse.store.gigamap.types.GigaMap; +import org.eclipse.store.gigamap.types.IndexerInteger; +import org.eclipse.store.gigamap.types.IndexerString; +import org.junit.jupiter.api.Test; + +/** + * A {@link BitmapIndex} obtained from {@code add} / {@code get} is a handle, not the registered index + * itself: {@code update(Indexer)} and a rebuild replace the registered instance, {@code removeIndex} + * drops it. The replacement keeps the same parent, so validating the parent back-reference cannot tell a + * detached instance from the live one - and a detached one has had its off-heap released, so answering + * from it would silently return nothing. + *

+ * These tests pin that a held reference resolves to whatever is registered under its name at query time, + * and that a name which is gone is reported instead of answering empty. + */ +public class HeldIndexReferenceTest +{ + static class Person + { + String name; + + Person() + { + // required for deserialization + } + + Person(final String name) + { + super(); + this.name = name; + } + } + + static final IndexerString FULL_NAME = new IndexerString.Abstract<>() + { + @Override + public String name() + { + return "key"; + } + + @Override + protected String getString(final Person entity) + { + return entity.name; + } + }; + + /** same name "key", new logic */ + static final IndexerString FIRST_LETTER = new IndexerString.Abstract<>() + { + @Override + public String name() + { + return "key"; + } + + @Override + protected String getString(final Person entity) + { + return entity.name.substring(0, 1); + } + }; + + /** same name "key", but a different key type */ + static final IndexerInteger NAME_LENGTH = new IndexerInteger.Abstract<>() + { + @Override + public String name() + { + return "key"; + } + + @Override + protected Integer getInteger(final Person entity) + { + return entity.name.length(); + } + }; + + private static GigaMap populatedMap() + { + final GigaMap map = GigaMap.New(); + map.index().bitmap().add(FULL_NAME); + map.add(new Person("alice")); + map.add(new Person("bob")); + + return map; + } + + @Test + void heldReferenceAnswersFromTheRebuiltIndexAfterUpdate() + { + final GigaMap map = populatedMap(); + final BitmapIndex held = map.index().bitmap().get(String.class, "key"); + + map.index().bitmap().update(FIRST_LETTER); + + // the held handle must answer for the NEW logic, not from its own released data + assertEquals(1, map.query(held.is("a")).count(), "held reference did not follow the redefinition"); + assertEquals(1, map.query(held.is("b")).count(), "held reference did not follow the redefinition"); + assertEquals(0, map.query(held.is("alice")).count(), "held reference still answers from stale data"); + } + + @Test + void heldReferenceResolvesToTheCurrentlyRegisteredInstance() + { + final GigaMap map = populatedMap(); + final BitmapIndex held = map.index().bitmap().get(String.class, "key"); + + map.index().bitmap().update(FIRST_LETTER); + + assertSame( + map.index().bitmap().get(String.class, "key"), + map.index().bitmap().get(String.class, "key"), + "precondition: get() returns the registered instance" + ); + // the handle is stale, but querying through it reaches the registered index + assertEquals(1, map.query(held.is("a")).count()); + } + + @Test + void heldReferenceToARemovedIndexIsReportedInsteadOfAnsweringEmpty() + { + final GigaMap map = populatedMap(); + final BitmapIndex held = map.index().bitmap().get(String.class, "key"); + + map.index().bitmap().removeIndex("key"); + + final BitmapIndexException e = assertThrows( + BitmapIndexException.class, + () -> map.query(held.is("alice")).count(), + "a removed index must be reported, not answered as empty" + ); + assertEquals(true, e.getMessage().contains("no longer registered")); + } + + @Test + void heldReferenceOfAChangedKeyTypeIsReportedInsteadOfAnsweringEmpty() + { + final GigaMap map = populatedMap(); + final BitmapIndex held = map.index().bitmap().get(String.class, "key"); + + // same name, different key type - the String index is gone + map.index().bitmap().update(NAME_LENGTH); + + assertThrows( + BitmapIndexException.class, + () -> map.query(held.is("alice")).count(), + "a replaced key type must be reported, not answered as empty" + ); + // the map itself is fine under the new logic + assertEquals(1, map.query(NAME_LENGTH.is(5)).count()); + } + + @Test + void aForeignParentIsStillRejected() + { + final GigaMap map = populatedMap(); + final BitmapIndex held = map.index().bitmap().get(String.class, "key"); + final GigaMap other = populatedMap(); + + assertThrows( + BitmapIndexException.class, + () -> other.query(held.is("alice")).count(), + "an index of another map must not resolve" + ); + } +} From ac6572be651e7f6292b6193de99045c3224d8f25 Mon Sep 17 00:00:00 2001 From: Florian Habermann Date: Fri, 7 Aug 2026 10:52:25 +0200 Subject: [PATCH 2/6] GigaMap: a failing reindex() no longer truncates the whole bitmap index group (internal#132) The rebuild dropped every index' data up front and then re-added entity by entity through internalAdd, which fans out to all indices of the group. An Indexer throwing at entity k therefore left every index - healthy siblings included - holding only the entities before k. The state before the failed recovery was stale but complete; afterwards it was silently partial, queries stopped finding entities they had found a moment earlier, and one ordinary mutation plus store() persisted the loss. That is the failure mode of the documented recovery path itself: reindex() is what the javadoc prescribes after a direct mutation or a class evolution of an indexed field, which is exactly when an indexer is most likely to throw. Each index is now rebuilt into a replacement built aside and swapped in only once its data is complete, the way update(Indexer) redefines one. A throwing indexer costs only its own index' rebuild: that index keeps its previous content - as stale as before the call, but complete - every other index is rebuilt, and none is ever a prefix of the entities. Every index is attempted before the first failure is rethrown, the rest suppressed, mirroring internalRemove. A unique-constraint violation is deliberately not treated as such a failure. There the rebuild did complete and merely produced colliding data, so the replacement is swapped in and the violation reported afterwards - the contract internal#121 established, which the repair depends on because it queries the rebuilt indices. Reporting before the swap would keep exactly the stale keys that made the rebuild necessary. The uniqueness check moves onto the replacement, where it is correct for the same reason it was correct after a clear: a replacement starts empty, so every hit is a genuinely different entity. Checking against the registered constraints during a rebuild would be wrong, since those still hold their full data and every entity trivially collides with itself. Rebuilding one index at a time costs one pass over the entities per index rather than one in total. That is deliberate: it bounds the extra memory to a single index' data - the peak update(Indexer) already has - instead of duplicating the whole group, which on a map sized for reindex() to matter is the difference between feasible and not. The commit sequence update(Indexer) used to inline is now a shared swapIndex, so the two paths cannot drift on unique-constraint and identity-index membership. reindex() gains the Behavior on failure section it was the only mutating method to lack, and IndexGroup.internalReindex documents that its default is neither validating nor failure-atomic. Lucene and vector groups still use a drop-first rebuild and are unchanged. --- docs/modules/gigamap/pages/crud.adoc | 2 + docs/modules/gigamap/pages/persistence.adoc | 2 + .../store/gigamap/types/BitmapIndices.java | 278 ++++++++++---- .../eclipse/store/gigamap/types/GigaMap.java | 11 + .../store/gigamap/types/IndexGroup.java | 7 + .../store/gigamap/issues/GigaMap132Test.java | 356 ++++++++++++++++++ 6 files changed, 593 insertions(+), 63 deletions(-) create mode 100644 gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/issues/GigaMap132Test.java diff --git a/docs/modules/gigamap/pages/crud.adoc b/docs/modules/gigamap/pages/crud.adoc index d4553be6..7efc4665 100644 --- a/docs/modules/gigamap/pages/crud.adoc +++ b/docs/modules/gigamap/pages/crud.adoc @@ -89,6 +89,8 @@ The two are alternatives, not steps: an `update` after a direct modification doe Because the rebuild derives every index key anew, it is also the point at which the registered unique constraints are validated again: a rebuild over data in which two entities share a key of a unique constraint reports that violation (after completing the rebuild) instead of building an index that maps one unique key to several entities. See xref:constraints.adoc#_validation_points[Constraints - Validation Points]. +Deriving every key anew is also where an indexer can fail — typically one that cannot handle a value it never saw before the evolution or mutation that made the repair necessary. Each bitmap index is therefore rebuilt into a replacement that is put in place only once complete, so a throwing indexer costs only that index' rebuild: it is left exactly as it was — as stale as before the call, but complete — while every other index is rebuilt. No index is left holding only part of the entities. Every index is attempted, and the first failure is reported afterwards with any further ones attached as suppressed exceptions. Fix the indexer and call `reindex()` again to repair what was left behind. + Entities modified through `update` (or `apply`) are automatically persisted when calling `store()`. See xref:persistence.adoc[] for details. WARNING: Because `update` / `apply` mutate the entity in place, a constraint violation triggered by the new state cannot be rolled back. The GigaMap therefore **removes** the offending entity from the map and rethrows the `ConstraintViolationException`. The same applies to an exception thrown by the update logic itself, which leaves the entity partially mutated. See xref:constraints.adoc#_constraint_violations[Constraints — Constraint Violations] for the full rollback semantics across CRUD operations. diff --git a/docs/modules/gigamap/pages/persistence.adoc b/docs/modules/gigamap/pages/persistence.adoc index 1e17a7b8..eea7349b 100644 --- a/docs/modules/gigamap/pages/persistence.adoc +++ b/docs/modules/gigamap/pages/persistence.adoc @@ -49,6 +49,8 @@ storageManager.store(person); // persist the changed entity explicitly gigaMap.reindex(); // rebuild every index from current entity state gigaMap.store(); // persist the rebuilt indices ---- + +If `reindex()` throws because an indexer could not handle one of the entities, the bitmap indices it had already rebuilt are rebuilt, and the one whose indexer threw is left as it was — stale, but complete, never partial. Fix the indexer and call `reindex()` again before `store()`. See xref:crud.adoc#_removing_and_updating_entities[CRUD — Removing and Updating Entities]. ==== === Why not `storageManager.store(gigaMap)`? diff --git a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java index b8ed2307..3dfcc7de 100644 --- a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java +++ b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java @@ -972,90 +972,255 @@ public void internalRemoveAll() } /** - * Rebuilds all bitmap indices from the current entity state and, unlike the generic - * {@link IndexGroup.Internal#internalReindex(GigaMap) clear + re-add} default, re-validates the - * registered unique constraints while doing so. A rebuild is the one path that derives every key - * anew, so data whose keys collide - e.g. every entity of a class-evolved indexed field defaulting - * to the same value - would otherwise silently yield an index in which one unique key maps to - * several live entities, a state all write paths reject. + * Rebuilds all bitmap indices from the current entity state, one index at a time, and - unlike the + * generic {@link IndexGroup.Internal#internalReindex(GigaMap) clear + re-add} default - both + * re-validates the registered unique constraints and survives a failing {@link Indexer}. *

- * The rebuild is completed before a violation is reported: the resulting indices then - * describe the entities as they actually are, which is what the documented repair - re-distinguish - * the colliding keys via {@code update}/{@code apply}, then {@code reindex()} again - operates on. - * Aborting mid-pass would leave the indices half-rebuilt, and aborting before the pass would keep - * exactly the stale keys that made the rebuild necessary in the first place. + * Each index is rebuilt into a replacement built aside and swapped in only once its data is + * complete, the way {@link #update(Indexer)} redefines one. The generic default drops every index + * first and re-adds entity by entity through {@link #internalAdd(long, Object)}, which fans out to + * all of them - so a single indexer throwing at entity k would truncate the whole group, + * healthy indices included, to the entities before k. Here a throwing indexer costs only its + * own index' rebuild: that index is left exactly as it was - as stale as before the call, but + * complete - every other index is rebuilt, and none is ever a prefix of the entities. The first + * failure is rethrown once all indices have been attempted, the rest attached as suppressed. + *

+ * A unique-constraint violation is not such a failure: the rebuild completed and merely + * produced data that collides, so the replacement is swapped in and the violation reported + * afterwards. The indices then describe the entities as they actually are, which is what the + * documented repair - re-distinguish the colliding keys via {@code update}/{@code apply}, then + * {@code reindex()} again - operates on. Reporting before the swap would keep exactly the stale keys + * that made the rebuild necessary in the first place. + *

+ * The price of rebuilding one index at a time is one pass over the entities per index, rather than + * one in total. That bounds the additional memory to a single index' data - the same peak + * {@link #update(Indexer)} already has - instead of duplicating the whole group. */ @Override public final void internalReindex(final GigaMap parentMap) { - this.internalRemoveAll(); + // Snapshot: the loop swaps entries, which mutates the table it would otherwise iterate. + // Only registered indices are rebuilt - a composite's sub-indices are its own children and + // never appear here, which matters because their indexer() is the sub-index itself. + final BulkList> indices = BulkList.New(this.bitmapIndices.values()); + if(indices.isEmpty()) + { + return; + } - if(this.uniqueConstraints == null) + Throwable first = null; + try + { + for(final BitmapIndex.Internal existing : indices) + { + try + { + this.reindexSingleIndex(existing); + } + catch(final RuntimeException e) + { + first = addAsFailure(first, e); + } + } + } + finally { - // nothing to validate: plain rebuild, without paying for the per-entity containment checks. - parentMap.iterateIndexed(this::internalAdd); + // entries were replaced, so the transient lookup arrays no longer describe the table + this.rebuildCache(); + } - return; + if(first != null) + { + throw (RuntimeException)first; } + } - final UniquenessViolation violation = new UniquenessViolation<>(); - parentMap.iterateIndexed((final long entityId, final E entity) -> + /** + * Rebuilds a single registered index into a replacement and swaps it in. Deliberately does not + * consult {@link #ensureMutable(String)}: {@code GigaMap.reindex()} has already checked, and that + * guard may release the parent-map monitor while waiting, which would expose a half-swapped group. + * + * @param existing the registered index to rebuild + */ + private void reindexSingleIndex(final BitmapIndex.Internal existing) + { + // captured before the swap: #internalRemoveIndex strips both memberships + final boolean wasUnique = this.isUniqueConstraint(existing); + final boolean wasIdentity = this.isIdentityIndex(existing); + + final BitmapIndex.Internal replacement = existing.indexer().createFor(this); + + final UniquenessViolation violation; + try { - this.collectUniquenessViolation(entityId, entity, violation); - this.internalAdd(entityId, entity); - }); + this.validateIndexParent(replacement); + if(!existing.name().equals(replacement.name())) + { + throw new BitmapIndicesException( + "Indexer of index \"" + existing.name() + "\" created an index named \"" + + replacement.name() + "\"; a rebuild must keep the name the index is registered under.", + this + ); + } - if(violation.violatedIndex != null) + violation = this.buildReplacementIndexData(replacement, wasUnique); + } + catch(final Throwable t) + { + // the replacement never becomes visible, so nothing else would ever release it + releaseAbandonedIndexData(replacement, t); + + throw t; + } + + // commit: nothing below can fail, so the group is never left between the two indices. + this.swapIndex(existing, replacement, wasUnique, wasIdentity); + + if(violation != null) { throw violation.toException(); } } /** - * Checks the entity about to be re-indexed against the unique constraints and records a collision in - * the given collector. Uses the same check-then-add order as - * {@link #buildIndexDataAndValidateUniqueness(EqHashTable)}, which is correct during a rebuild - * because the indices were just cleared: only entities re-added in the same pass can be found, so - * every hit is a genuinely different entity (there is no own stale entry to exclude). + * Fills a replacement index from all entities and, if it backs a unique constraint, collects the + * first key collision instead of aborting on it - the rebuild has to complete either way. + *

+ * Checking with {@link BitmapIndex.Internal#internalContains(Object)} against the replacement is + * correct for the same reason it is in {@link #buildIndexDataAndValidateUniqueness(EqHashTable)}: + * the replacement starts empty, so only entities added in this very pass can be found and every hit + * is therefore a genuinely different entity, with no own stale entry to exclude. Checking against + * the registered {@link #uniqueConstraints} instead would be wrong here - during a rebuild those + * still hold their full, not yet replaced data, in which every entity trivially collides with + * itself. *

- * Only the first collision is reported, so once one has been found the remaining entities are - * checked against that one index only - to report how many of them are affected - instead of - * accumulating unrelated findings from the other constraints. + * The entity is added whether or not it collided, so the finished index describes the + * entities as they actually are. * + * @param replacement the not yet registered index to fill + * @param unique whether it backs a unique constraint and its keys must therefore be checked + * @return the collected violation, or {@code null} if the keys are unique (or unchecked) + */ + private UniquenessViolation buildReplacementIndexData( + final BitmapIndex.Internal replacement, + final boolean unique + ) + { + if(!unique) + { + // no constraint to validate: plain rebuild, without paying for the per-entity containment checks. + this.parent.iterateIndexed(replacement::internalAdd); + + return null; + } + + final UniquenessViolation violation = new UniquenessViolation<>(); + this.parent.iterateIndexed((final long entityId, final E entity) -> + { + collectUniquenessViolation(replacement, entityId, entity, violation); + replacement.internalAdd(entityId, entity); + }); + + return violation.violatedIndex != null + ? violation + : null + ; + } + + /** + * Records a key collision of the entity about to be indexed in the given collector. Only the first + * collision is reported, so once one has been found the remaining entities are only counted - to + * report how many are affected - rather than replacing it. + * + * @param index the index being rebuilt * @param entityId the entity's id - * @param entity the entity about to be re-indexed + * @param entity the entity about to be indexed * @param violation the collector of the violation to report after the rebuild */ - private void collectUniquenessViolation( - final long entityId , - final E entity , - final UniquenessViolation violation + private static void collectUniquenessViolation( + final BitmapIndex.Internal index , + final long entityId , + final E entity , + final UniquenessViolation violation ) { + if(!index.internalContains(entity)) + { + return; + } + if(violation.violatedIndex != null) { - if(violation.violatedIndex.internalContains(entity)) - { - violation.duplicateCount++; - } + violation.duplicateCount++; return; } - for(final BitmapIndex.Internal index : this.uniqueConstraints) + violation.violatedIndex = index ; + violation.entityId = entityId; + violation.violatingEntity = entity ; + violation.duplicateCount = 1 ; + } + + /** + * Replaces a registered index with an already fully built one, carrying its unique-constraint and + * identity-index membership over. Shared by {@link #update(Indexer)} and by a rebuild, which differ + * only in where the replacement's data comes from. + *

+ * Cannot fail, which is what both callers rely on: the name the replacement is registered + * under is the one just freed, so registration cannot be rejected, and neither caller could undo a + * partial swap - the dropped index' data has been released by then. Everything that could reject the + * replacement is therefore checked before this is called. + *

+ * Deliberately leaves {@link #rebuildCache()} to the caller: a rebuild swaps many indices and pays + * for it once at the end. + * + * @param existing the currently registered index + * @param replacement the fully built index to register in its place + * @param wasUnique whether {@code existing} backed a unique constraint + * @param wasIdentity whether {@code existing} was an identity index + */ + private void swapIndex( + final BitmapIndex.Internal existing , + final BitmapIndex.Internal replacement, + final boolean wasUnique , + final boolean wasIdentity + ) + { + // drops the old index' logic and data, releasing its off-heap memory. Identity removal is allowed + // here and restored below; the cache rebuild is the caller's. + this.internalRemoveIndex(existing.name(), true, false); + if(wasUnique) { - if(index.internalContains(entity)) - { - violation.violatedIndex = index ; - violation.entityId = entityId; - violation.violatingEntity = entity ; - violation.duplicateCount = 1 ; + this.internalAddUniqueConstraint(replacement); + } + this.internalAddBitmapIndex(replacement); - return; - } + if(wasIdentity) + { + this.internalReplaceIdentityIndex(existing, replacement); } } + /** + * Collects a failure across a best-effort loop: the first one is the one that will be rethrown, any + * further one is attached to it. Mirrors {@link #internalRemove(long, Object)}. + * + * @param first the failure collected so far, or {@code null} + * @param next the failure just encountered + * @return the failure to rethrow at the end + */ + private static Throwable addAsFailure(final Throwable first, final Throwable next) + { + if(first == null) + { + return next; + } + first.addSuppressed(next); + + return first; + } + /** * Mutable collector for the unique-constraint violation encountered during a rebuild. A rebuild * reports its violation only after it completed, and the per-entity check runs inside a lambda that @@ -1424,21 +1589,8 @@ public BitmapIndex update(final Indexer indexer) throw t; } - // commit: nothing below can fail. Drop the old index (logic + data), then register the - // rebuilt one - its name is the one just freed, so registration cannot be rejected. - // identity removal is allowed here and restored below; skip the intermediate cache - // rebuild since update() rebuilds the cache once at the end. - this.internalRemoveIndex(name, true, false); - if(wasUnique) - { - this.internalAddUniqueConstraint(index); - } - this.internalAddBitmapIndex(index); - - if(wasIdentity) - { - this.internalReplaceIdentityIndex(existing, index); - } + // commit: nothing below can fail. + this.swapIndex(existing, index, wasUnique, wasIdentity); this.rebuildCache(); return index; } diff --git a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/GigaMap.java b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/GigaMap.java index 5cca67bc..9132521e 100644 --- a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/GigaMap.java +++ b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/GigaMap.java @@ -655,6 +655,17 @@ public default E update(final long entityId, final Consumer logic) * the entities as they actually are. That is what the repair works on: re-distinguish the colliding keys * via {@link #update(long, Consumer) update} / {@link #apply(long, Function) apply}, then call this method * again. + *

+ * Behavior on failure: a bitmap index is rebuilt into a replacement that is built aside and put in + * place only once its data is complete, so an {@link Indexer} throwing for one entity costs only its own + * index' rebuild: that index is left exactly as it was - as stale as before the call, but complete - every + * other index is rebuilt, and no index is ever left holding a prefix of the entities. Every index is + * attempted; the first failure is rethrown afterwards with any further ones attached as suppressed + * exceptions. Fix the cause and call this method again to repair the indices that were left behind. A + * unique-constraint violation is not such a failure - the rebuild completed and merely produced + * colliding data, so it is reported as described above. Index groups other than the bitmap indices + * (Lucene, vector) still drop their data before rebuilding, so a failure there can leave that group + * partial until the next successful rebuild. * * @throws UniqueConstraintViolationException if the rebuilt indices show two or more entities sharing a key * of a unique constraint. The exception names the violated index and the first entity found under diff --git a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/IndexGroup.java b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/IndexGroup.java index 54cdaae1..2abde97c 100644 --- a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/IndexGroup.java +++ b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/IndexGroup.java @@ -140,6 +140,13 @@ public default void internalOnRegistered() * rebuild derives every key anew and the current entity state may well violate a constraint that * held when the entities were written (the bitmap group does so for its unique constraints, see * {@code BitmapIndices.Default#internalReindex(GigaMap)}). + *

+ * The default is also not failure-atomic: it drops the data before it can know whether the + * rebuild succeeds, and {@link #internalAdd(long, Object)} fans out to every index of the group, so + * an {@link Indexer} throwing for one entity leaves the entire group holding only the entities + * before it - a silently partial state that a subsequent {@code store()} makes durable. A group that + * can build its new data aside should override this and swap it in only once complete; the bitmap + * group does so per index. * * @param parentMap the map whose entities this group indexes */ diff --git a/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/issues/GigaMap132Test.java b/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/issues/GigaMap132Test.java new file mode 100644 index 00000000..e651f922 --- /dev/null +++ b/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/issues/GigaMap132Test.java @@ -0,0 +1,356 @@ +package org.eclipse.store.gigamap.issues; + +/*- + * #%L + * EclipseStore GigaMap + * %% + * Copyright (C) 2023 - 2026 MicroStream Software + * %% + * This program and the accompanying materials are made + * available under the terms of the Eclipse Public License 2.0 + * which is available at https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * #L% + */ + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.nio.file.Path; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.eclipse.store.gigamap.exceptions.UniqueConstraintViolationExceptionBitmap; +import org.eclipse.store.gigamap.types.BinaryIndexerString; +import org.eclipse.store.gigamap.types.GigaMap; +import org.eclipse.store.gigamap.types.IndexerString; +import org.eclipse.store.storage.embedded.types.EmbeddedStorage; +import org.eclipse.store.storage.embedded.types.EmbeddedStorageManager; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.api.io.TempDir; + +/** + * Regression coverage for internal issue #132: {@code GigaMap.reindex()} used to drop every index' data + * before re-adding entity by entity, and the re-add fans out to all indices of the group. An + * {@link org.eclipse.store.gigamap.types.Indexer} throwing for one entity therefore truncated the + * whole group - healthy sibling indices included - to the entities processed before the throw. + * The state before the failed recovery was stale but complete; afterwards it was silently partial, and + * one ordinary mutation plus {@code store()} made the loss durable. + *

+ * These tests pin the fixed behavior: each index is rebuilt into a replacement built aside and swapped in + * only once complete, so a throwing indexer costs only its own index' rebuild. That index is left exactly + * as it was - as stale as before the call, but complete - every other index is rebuilt, and none is ever a + * prefix of the entities. + *

+ * A unique-constraint violation is deliberately not such a failure and still completes the rebuild; + * see {@link GigaMap121Test}, whose contract these tests must not disturb. + */ +public class GigaMap132Test +{ + /** static, so it is not part of the persisted indexer state */ + static final AtomicBoolean ARMED = new AtomicBoolean(); + + static class Rec + { + String key; + + Rec() + { + // required for deserialization + super(); + } + + Rec(final String key) + { + super(); + this.key = key; + } + } + + static final IndexerString HEALTHY = new IndexerString.Abstract<>() + { + @Override + public String name() + { + return "healthy"; + } + + @Override + protected String getString(final Rec entity) + { + return entity.key; + } + }; + + /** throws for "e3", but only while armed - so the map can be populated first */ + static final IndexerString BREAKABLE = new IndexerString.Abstract<>() + { + @Override + public String name() + { + return "breakable"; + } + + @Override + protected String getString(final Rec entity) + { + if(ARMED.get() && "e3".equals(entity.key)) + { + throw new IllegalStateException("indexer failure on " + entity.key); + } + + return entity.key; + } + }; + + /** a second breakable index, throwing for a different entity */ + static final IndexerString BREAKABLE_2 = new IndexerString.Abstract<>() + { + @Override + public String name() + { + return "breakable2"; + } + + @Override + protected String getString(final Rec entity) + { + if(ARMED.get() && "e1".equals(entity.key)) + { + throw new IllegalStateException("indexer failure on " + entity.key); + } + + return entity.key; + } + }; + + static final BinaryIndexerString UNIQUE_KEY = new BinaryIndexerString.Abstract<>() + { + @Override + public String name() + { + return "unique"; + } + + @Override + protected String getString(final Rec entity) + { + if(ARMED.get() && "e3".equals(entity.key)) + { + throw new IllegalStateException("indexer failure on " + entity.key); + } + + return entity.key; + } + }; + + @BeforeEach + void disarm() + { + ARMED.set(false); + } + + private static GigaMap populatedMap() + { + final GigaMap map = GigaMap.New(); + map.index().bitmap().add(HEALTHY); + map.index().bitmap().add(BREAKABLE); + for(int i = 0; i < 5; i++) + { + map.add(new Rec("e" + i)); + } + + return map; + } + + private static long findable(final GigaMap map, final IndexerString indexer) + { + long found = 0; + for(int i = 0; i < 5; i++) + { + found += map.query(indexer.is("e" + i)).count(); + } + + return found; + } + + /** The issue's RED reproducer: the healthy sibling must not lose entries to another index' failure. */ + @Test + @Timeout(120) + void failedReindexKeepsHealthySiblingIndexComplete() + { + final GigaMap map = populatedMap(); + assertEquals(5, findable(map, HEALTHY), "precondition"); + + ARMED.set(true); + assertThrows(RuntimeException.class, map::reindex, "the failing rebuild must surface"); + + assertEquals(5, findable(map, HEALTHY), + "the healthy index lost entries to an unrelated index' failure"); + } + + /** The failing index keeps its previous content - stale, but complete, never a prefix. */ + @Test + @Timeout(120) + void failedReindexKeepsTheThrowingIndexAtItsPreviousContent() + { + final GigaMap map = populatedMap(); + assertEquals(5, findable(map, BREAKABLE), "precondition"); + + ARMED.set(true); + assertThrows(RuntimeException.class, map::reindex); + + ARMED.set(false); // query with working logic; the index data is what matters + assertEquals(5, findable(map, BREAKABLE), + "the failing index was truncated instead of being left as it was"); + } + + /** The durability probe from the issue: continued work plus store() must not persist a loss. */ + @Test + @Timeout(120) + void continuedWorkAfterFailedReindexPersistsNothingBroken(@TempDir final Path dir) + { + final GigaMap map = populatedMap(); + try(final EmbeddedStorageManager storage = EmbeddedStorage.start(map, dir)) + { + map.store(); + + ARMED.set(true); + assertThrows(RuntimeException.class, map::reindex); + ARMED.set(false); + + // one ordinary successful mutation marks the parent flags through the normal path + map.add(new Rec("e5")); + map.store(); + } + + try(final EmbeddedStorageManager storage = EmbeddedStorage.start(dir)) + { + @SuppressWarnings("unchecked") + final GigaMap loaded = (GigaMap)storage.root(); + + assertEquals(6, loaded.size()); + assertEquals(5, findable(loaded, HEALTHY), "a truncation was made durable"); + assertEquals(5, findable(loaded, BREAKABLE), "a truncation was made durable"); + assertEquals(1, loaded.query(HEALTHY.is("e5")).count()); + } + } + + /** The documented remedy: re-run reindex() once the cause is fixed. */ + @Test + @Timeout(120) + void reindexAfterFixingTheIndexerHeals() + { + final GigaMap map = populatedMap(); + + ARMED.set(true); + assertThrows(RuntimeException.class, map::reindex); + + ARMED.set(false); + map.reindex(); + + assertEquals(5, findable(map, HEALTHY)); + assertEquals(5, findable(map, BREAKABLE)); + } + + /** Best-effort across the indices: every index is attempted, the first failure is the one thrown. */ + @Test + @Timeout(120) + void failedReindexAttemptsEveryIndexAndSuppressesFurtherFailures() + { + final GigaMap map = populatedMap(); + map.index().bitmap().add(BREAKABLE_2); + + ARMED.set(true); + final RuntimeException e = assertThrows(RuntimeException.class, map::reindex); + ARMED.set(false); + + assertEquals(1, e.getSuppressed().length, "the second failure must be attached, not swallowed"); + assertEquals(5, findable(map, HEALTHY), "the healthy index must still have been rebuilt"); + assertEquals(5, findable(map, BREAKABLE), "both failing indices keep their previous content"); + assertEquals(5, findable(map, BREAKABLE_2), "both failing indices keep their previous content"); + } + + /** A failing index that backs a unique constraint keeps that constraint enforced. */ + @Test + @Timeout(120) + void failedReindexOfAUniqueIndexKeepsTheConstraintEnforced() + { + final GigaMap map = GigaMap.New(); + map.index().bitmap().add(HEALTHY); + map.index().bitmap().addUniqueConstraint(UNIQUE_KEY); + for(int i = 0; i < 5; i++) + { + map.add(new Rec("e" + i)); + } + + ARMED.set(true); + assertThrows(RuntimeException.class, map::reindex); + ARMED.set(false); + + assertEquals(1, map.index().bitmap().uniqueConstraints().size(), "unique membership lost"); + assertThrows(RuntimeException.class, () -> map.add(new Rec("e0")), "constraint no longer enforced"); + assertEquals(5, findable(map, HEALTHY), "the healthy index must still have been rebuilt"); + } + + /** + * A unique-constraint violation is not an indexer failure: the rebuild completes and the replacement is + * swapped in, so the indices describe the entities as they actually are. Guards the #121 contract from + * inside the new code path; see {@link GigaMap121Test} for the full evolution-driven case. + */ + @Test + @Timeout(120) + void uniqueViolationStillCompletesTheRebuild() + { + final GigaMap map = GigaMap.New(); + map.index().bitmap().addUniqueConstraint(UNIQUE_KEY); + final Rec a = new Rec("a"); + final Rec b = new Rec("b"); + map.add(a); + map.add(b); + + // direct mutation: the index still answers for "b", the entity is now a duplicate "a" + b.key = "a"; + + final UniqueConstraintViolationExceptionBitmap e = assertThrows( + UniqueConstraintViolationExceptionBitmap.class, + map::reindex + ); + assertEquals("unique", e.getViolatedIndex().name()); + assertTrue(e.getMessage().contains("reindex()"), "the message must name the operation"); + + // the rebuild was completed: the stale key is gone and both entities answer under "a" + assertEquals(0, map.query(UNIQUE_KEY.is("b")).count(), "the stale key must be gone"); + assertEquals(2, map.query(UNIQUE_KEY.is("a")).count(), "the rebuild must describe the actual state"); + assertEquals(2, map.size(), "no entity may be dropped"); + } + + /** The swap must carry identity-index membership over to the rebuilt index. */ + @Test + @Timeout(120) + void reindexPreservesIdentityIndex() + { + final GigaMap map = GigaMap.New(); + map.index().bitmap().add(HEALTHY); + map.index().bitmap().setIdentityIndices(org.eclipse.serializer.util.X.Enum(HEALTHY)); + final Rec a = new Rec("e0"); + map.add(a); + map.add(new Rec("e1")); + + map.reindex(); + + assertEquals(1, map.index().bitmap().identityIndices().size(), "identity membership lost"); + assertSame( + map.index().bitmap().get(String.class, "healthy"), + map.index().bitmap().identityIndices().get(), + "the identity set must point at the rebuilt index" + ); + // identity lookup still resolves the entity + map.update(a, rec -> rec.key = "e9"); + assertEquals(1, map.query(HEALTHY.is("e9")).count()); + } +} From b1de66f8e1a52f42c3a740ceb1fe8b21495ffd78 Mon Sep 17 00:00:00 2001 From: Florian Habermann Date: Fri, 7 Aug 2026 11:01:57 +0200 Subject: [PATCH 3/6] Address Copilot review: assert the specific constraint exception, fix a garbled docs sentence The unique-enforcement assertion accepted any RuntimeException, which would have passed on an unrelated failure and masked exactly the regression it guards. It now names UniqueConstraintViolationExceptionBitmap. The persistence doc said the already rebuilt indices "are rebuilt"; it now says they keep their rebuilt data. --- docs/modules/gigamap/pages/persistence.adoc | 2 +- .../org/eclipse/store/gigamap/issues/GigaMap132Test.java | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/modules/gigamap/pages/persistence.adoc b/docs/modules/gigamap/pages/persistence.adoc index eea7349b..03caedeb 100644 --- a/docs/modules/gigamap/pages/persistence.adoc +++ b/docs/modules/gigamap/pages/persistence.adoc @@ -50,7 +50,7 @@ gigaMap.reindex(); // rebuild every index from current entity s gigaMap.store(); // persist the rebuilt indices ---- -If `reindex()` throws because an indexer could not handle one of the entities, the bitmap indices it had already rebuilt are rebuilt, and the one whose indexer threw is left as it was — stale, but complete, never partial. Fix the indexer and call `reindex()` again before `store()`. See xref:crud.adoc#_removing_and_updating_entities[CRUD — Removing and Updating Entities]. +If `reindex()` throws because an indexer could not handle one of the entities, the bitmap indices it rebuilt successfully keep their rebuilt data, and the one whose indexer threw is left as it was — stale, but complete, never partial. Fix the indexer and call `reindex()` again before `store()`. See xref:crud.adoc#_removing_and_updating_entities[CRUD — Removing and Updating Entities]. ==== === Why not `storageManager.store(gigaMap)`? diff --git a/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/issues/GigaMap132Test.java b/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/issues/GigaMap132Test.java index e651f922..7dde78d9 100644 --- a/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/issues/GigaMap132Test.java +++ b/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/issues/GigaMap132Test.java @@ -293,7 +293,11 @@ void failedReindexOfAUniqueIndexKeepsTheConstraintEnforced() ARMED.set(false); assertEquals(1, map.index().bitmap().uniqueConstraints().size(), "unique membership lost"); - assertThrows(RuntimeException.class, () -> map.add(new Rec("e0")), "constraint no longer enforced"); + assertThrows( + UniqueConstraintViolationExceptionBitmap.class, + () -> map.add(new Rec("e0")), + "constraint no longer enforced" + ); assertEquals(5, findable(map, HEALTHY), "the healthy index must still have been rebuilt"); } From 28770ebb7c8273bf59ce813d71bf9536b956a8da Mon Sep 17 00:00:00 2001 From: Florian Habermann Date: Fri, 7 Aug 2026 11:07:53 +0200 Subject: [PATCH 4/6] Address Copilot round 2: correct what internalRemoveIndex actually strips The capture comment claimed internalRemoveIndex strips both the unique-constraint and the identity membership. It strips only the former, via internalRemoveUniqueConstraint; identity survives the removal and is re-pointed afterwards by internalReplaceIdentityIndex. Reading both before the swap is still right - they then describe the same pre-swap state - but the stated reason only held for one of them. --- .../java/org/eclipse/store/gigamap/types/BitmapIndices.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java index 3dfcc7de..83ee46d9 100644 --- a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java +++ b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java @@ -1044,7 +1044,9 @@ public final void internalReindex(final GigaMap parentMap) */ private void reindexSingleIndex(final BitmapIndex.Internal existing) { - // captured before the swap: #internalRemoveIndex strips both memberships + // #internalRemoveIndex strips the unique-constraint membership, so it has to be read before the + // swap. The identity membership survives the removal and is re-pointed afterwards, but is read + // here as well so both describe the same, pre-swap state. final boolean wasUnique = this.isUniqueConstraint(existing); final boolean wasIdentity = this.isIdentityIndex(existing); From e69db1f629d9b15ff19981339ea5db375a8e978e Mon Sep 17 00:00:00 2001 From: Florian Habermann Date: Fri, 7 Aug 2026 11:14:41 +0200 Subject: [PATCH 5/6] Address Copilot round 3: assertTrue with a message instead of assertEquals(true, ...) The message check reported only "expected true but was false" on failure. It now names what the message was supposed to say and prints the one it got, which is the difference between a one-line diagnosis and a debugging session. --- .../store/gigamap/indexer/HeldIndexReferenceTest.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/HeldIndexReferenceTest.java b/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/HeldIndexReferenceTest.java index 239f54c8..a6f38d29 100644 --- a/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/HeldIndexReferenceTest.java +++ b/gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/indexer/HeldIndexReferenceTest.java @@ -17,6 +17,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import org.eclipse.store.gigamap.exceptions.BitmapIndexException; import org.eclipse.store.gigamap.types.BitmapIndex; @@ -154,7 +155,10 @@ void heldReferenceToARemovedIndexIsReportedInsteadOfAnsweringEmpty() () -> map.query(held.is("alice")).count(), "a removed index must be reported, not answered as empty" ); - assertEquals(true, e.getMessage().contains("no longer registered")); + assertTrue( + e.getMessage().contains("no longer registered"), + "the message must say the name is gone, not merely that something failed: " + e.getMessage() + ); } @Test From 50d515c2cdff7f698c47ebd04abe2d7c94b092ab Mon Sep 17 00:00:00 2001 From: Florian Habermann Date: Fri, 7 Aug 2026 11:23:39 +0200 Subject: [PATCH 6/6] Address Copilot round 4: type the reindex failure accumulator as RuntimeException The best-effort loop catches RuntimeException but accumulated into a Throwable and cast back on the rethrow. The cast was the only thing keeping that honest: nothing but the catch clause prevented a non-RuntimeException from being collected, and the failure would then have been a ClassCastException at the very end of a rebuild. Narrowing the variable and addAsFailure to RuntimeException makes it a compile-time property and removes the cast. --- .../java/org/eclipse/store/gigamap/types/BitmapIndices.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java index 83ee46d9..d40c8f40 100644 --- a/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java +++ b/gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/BitmapIndices.java @@ -1008,7 +1008,7 @@ public final void internalReindex(final GigaMap parentMap) return; } - Throwable first = null; + RuntimeException first = null; try { for(final BitmapIndex.Internal existing : indices) @@ -1031,7 +1031,7 @@ public final void internalReindex(final GigaMap parentMap) if(first != null) { - throw (RuntimeException)first; + throw first; } } @@ -1212,7 +1212,7 @@ private void swapIndex( * @param next the failure just encountered * @return the failure to rethrow at the end */ - private static Throwable addAsFailure(final Throwable first, final Throwable next) + private static RuntimeException addAsFailure(final RuntimeException first, final RuntimeException next) { if(first == null) {