Document GigaMap index staleness on class evolution and fail loud on stale-key removal - #762
Merged
Merged
Conversation
…stale-key removal
GigaMap's persisted bitmap indices are a derived cache: their keys are stored verbatim and restored on load without ever being re-derived from, or revalidated against, the entities. When an indexed field's class layout evolves between releases without a value-preserving refactoring mapping, the loaded entities carry shifted/defaulted values while the persisted index keeps the pre-evolution keys. Nothing detects this: queries return stale results, and a subsequent update/apply/set can fail deep inside the index update or (for a unique-constrained field) trigger a phantom constraint violation whose destructive apply() fallback removes the committed entity. GigaMap.reindex() already repairs all of this, but it was documented only as recovery from direct in-place mutation, and class evolution was mentioned nowhere in the GigaMap docs.
This change does not alter the intentional destructive apply() semantics; it attacks the root cause (undocumented and silent) by documenting the caveat and by turning the one raw, uncatchable failure on this path into a clear, actionable exception.
Documentation: add a "Class evolution and indexed fields" section to the GigaMap persistence guide describing the stale-index condition and the reindex()-then-store() recovery, and cross-link the same note from the reindex() and both apply(...) javadocs.
Fail-loud guard: AbstractBitmapIndexHashing.NewKeyChangeChandler.removeFromIndex no longer throws a raw java.lang.Error ("Removing an entityId for a new key may never be required."). It now throws a descriptive BitmapIndexException (or GigaIndexException for composite sub-indices, which are a GigaIndex but not a BitmapIndex) that names the likely stale-index / class-evolution cause and points the caller to GigaMap.reindex(). Reaching this branch means an existing entity is being de-indexed under a key that has no entry, which for a genuine new key is impossible by construction and therefore signals a stale index.
Regression test: GigaMap88Test simulates class evolution by rewriting the persisted type dictionary (rename + retype of the indexed field to a dissimilar, same-width member, so automatic member matching skips it and the runtime field loads defaulted). One test pins reindex() as the sanctioned recovery for a stale unique (binary) index so a legitimate apply() no longer deletes the committed entity and the fix survives a restart; the other pins that a write against a stale hashing index now raises the descriptive BitmapIndexException instead of a raw Error.
Note: making the key-type-mismatch lookup (BitmapIndices.internalGet) throw was considered but rejected — get(Class, String) is a documented public API that returns null on a miss and IndexIdentifier.resolveFor relies on that null, so this consequence is covered by documentation only.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR documents and hardens GigaMap’s behavior when persisted bitmap index keys become stale relative to loaded entities after class evolution (renamed/retyped indexed fields without value-preserving legacy mapping), and ensures the failure mode is catchable and actionable.
Changes:
- Adds documentation describing stale index behavior after class evolution, and prescribes
reindex()+store()as recovery. - Replaces an internal raw
java.lang.Errorwith a descriptive, catchableBitmapIndexException(orGigaIndexExceptionfor composite sub-indices) when stale-key removal is detected. - Adds regression tests that simulate class evolution by rewriting the persisted type dictionary and verify both recovery via
reindex()and the new fail-loud exception.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| gigamap/gigamap/src/test/java/org/eclipse/store/gigamap/issues/GigaMap88Test.java | Adds regression coverage simulating class evolution and validating reindex() recovery + descriptive exception behavior. |
| gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/GigaMap.java | Updates apply(...) and reindex() Javadocs to explicitly document stale-index risk on class evolution and recommended remediation. |
| gigamap/gigamap/src/main/java/org/eclipse/store/gigamap/types/AbstractBitmapIndexHashing.java | Converts an internal Error on impossible “remove-from-new-key” into actionable domain exceptions pointing to reindex(). |
| docs/modules/gigamap/pages/persistence.adoc | Adds a new persistence guide section warning about index staleness after class evolution and prescribing reindex() + store(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zdenek-jonas
approved these changes
Jul 15, 2026
fh-ms
added a commit
that referenced
this pull request
Jul 22, 2026
* Prevent GigaMap from deleting a committed entity on a stale index After an indexed field's class evolves without a value-preserving refactoring mapping, GigaMap's persisted bitmap indices load verbatim while the entities load with defaulted/shifted values, so the indices are stale relative to the entities. PR #762 documented this and made the stale-key removal fail loud, but deliberately left apply()'s behavior unchanged. As a result a legitimate apply()/update() after such an evolution could still trigger a phantom unique-constraint violation (the entity colliding only with its own stale index entry), and apply()'s destructive fallback then removed the committed entity, which the next store() persisted. That data-loss path is closed here without weakening the real unique-constraint guarantee and without changing the persisted format. Auto-detecting class evolution at load time to rebuild the indices was ruled out: legacy type handlers are keyed only by the old type-id (a Class always resolves to the current handler), there is no queryable "was this type legacy-mapped" API reachable from a type handler's complete(), and GigaMap tracks no entity type-ids. reindex() is also O(all entities) and force-loads every lazy segment. So the fix is reactive: never destroy a still-valid committed entity because the derived index is stale. Id-aware unique check: the unique-constraint check now asks "does any entity OTHER THAN the one being updated hold this key" instead of "does any entity hold this key". A key held only by the entity's own (stale) entry is not a duplicate, so no phantom violation is raised; a different entity holding the key is still a real violation and is rejected exactly as before. Implemented as a new BitmapIndex.Internal.internalContains(entity, excludedEntityId) overload (default delegates to the id-agnostic check) with real implementations on both unique-capable binary hierarchies (AbstractBitmapIndexBinary and AbstractCompositeBitmapIndex), reusing the existing bitmap iteration with a new ContainsOtherBreaker that skips the excluded id. Both update-time check sites in BitmapIndices (internalCheckViolation and internalUpdateIndices) pass the entity id. Never delete on a stale-index failure: a hashing index (which cannot back a unique constraint) can still hit the stale-key removal guard during an update. That guard now throws a BitmapIndexStaleException, marked with the new StaleIndexException interface. GigaMap.internalApply detects a StaleIndexException in the failure's cause/suppressed chain and retains the committed entity, rethrowing so the caller can reindex(), instead of removing it. Genuine exceptions from user update logic and real (different-entity) unique violations keep the existing destructive behavior. The stale-key guard's this.index is always a top-level HashingBitmapIndex (a BitmapIndex), so it throws BitmapIndexStaleException directly. Docs/javadoc: the apply(...) javadocs and the "Class evolution and indexed fields" section of the persistence guide now describe the refined contract - a committed entity is never destroyed by a phantom/stale-index violation; such a write either succeeds or throws a StaleIndexException without removing the entity, and reindex() restores correct query results. Out of scope: stale query visibility after evolution (is(oldValue) still matches, is(actualValue) misses) is not reactively fixable and still requires reindex(); this is a visibility issue, not data loss. Tracked as a follow-up. * Make isStaleIndexFailure traverse the full throwable graph cycle-safely (#88) The previous implementation walked the cause chain but only inspected the directly attached suppressed throwables (not their own causes), and could loop forever on a pathological throwable graph with cyclic causes. The JDK offers no ready-made utility for this - Throwable exposes only getCause() and getSuppressed() - so the traversal is done here explicitly: an iterative DFS over both cause and suppressed, guarded by an identity-based visited set, mirroring how Throwable.printStackTrace() avoids infinite loops internally. ArrayDeque forbids null elements, so only non-null throwables are pushed (getCause() may be null). * De-index the previous key before creating the new entry on a key change (#88) NewKeyChangeChandler.changeInIndex() created and registered the new-key BitmapEntry (marking the index changed) before de-indexing the entity from its previous key. If that removal threw - notably the stale-index path - the update failed but the index had already been mutated, leaving a stray empty entry for the new key that lingers until the next reindex() and makes recovery noisier. The order is now reversed: remove the entity from the previous key first, and create the new entry only after that succeeds (passing a no-op previous handler to the entry's own changeInIndex, since the removal is already done). On success the behavior is unchanged. * Persist the in-place mutation retained on a stale-index apply failure (#88) When apply()/update() fails because an index is stale, the update logic has already mutated the entity in place and we keep it. That mutation must be persisted like any other in-place mutation, otherwise the documented reindex() + store() recovery persists the rebuilt index while silently skipping the (same object identity) entity, reintroducing entity/index divergence after a restart - the rebuilt index would reference a value the persisted entity no longer carries. internalApply now gives the retained entity the same bookkeeping as a successful mutation through a shared retainMutatedEntity(entityId, current) helper: it pins the owning level1 segment (so it cannot be evicted before store) and tracks the entity in pendingEntityStores so store() re-persists it. GigaMap88Test is extended to assert the full recovery: after the stale update() throws, reindex() + store(), restart, and verify the entity is present with its mutated value and reachable by that value, with no stale entry remaining.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GigaMap's persisted bitmap indices are a derived cache whose keys are stored verbatim and restored on load without ever being re-derived from, or revalidated against, the entities. When an indexed field's class layout evolves between releases without a value-preserving refactoring mapping, the loaded entities carry shifted/defaulted values while the persisted index keeps the pre-evolution keys. Nothing detects this divergence: queries return stale results, and a subsequent
update/apply/setcan fail deep inside the index update or — for a unique-constrained field — trigger a phantom constraint violation whose destructiveapply()fallback removes the committed entity, which the nextstore()then persists.GigaMap.reindex()already fully repairs this, but it was documented only as recovery from direct in-place mutation, and class evolution was mentioned nowhere in the GigaMap docs. This PR keeps the intentional destructiveapply()semantics unchanged and instead attacks the root cause of the issue (the failure was undocumented and, on one path, silent/uncatchable): it documents the caveat and turns the raw failure into a clear, actionable exception.Changes
reindex()-then-store()recovery, cross-linked from thereindex()and bothapply(...)javadocs.AbstractBitmapIndexHashing.NewKeyChangeChandler.removeFromIndexno longer throws a raw, uncatchablejava.lang.Error("Removing an entityId for a new key may never be required."). It now throws a descriptiveBitmapIndexException(orGigaIndexExceptionfor composite sub-indices, which are aGigaIndexbut not aBitmapIndex) that names the likely stale-index / class-evolution cause and points the caller toGigaMap.reindex(). Reaching this branch means an existing entity is being de-indexed under a key that has no entry, which for a genuine new key is impossible by construction and therefore signals a stale index.GigaMap88Testsimulates class evolution by rewriting the persisted type dictionary (rename + retype of the indexed field to a dissimilar, same-width member, so automatic member matching skips it and the runtime field loads defaulted). One test pinsreindex()as the sanctioned recovery for a stale unique (binary) index so a legitimateapply()no longer deletes the committed entity and the fix survives a restart; the other pins that a write against a stale hashing index now raises the descriptiveBitmapIndexExceptioninstead of a rawError.Deliberately out of scope
apply()removal on constraint violation is intentional and documented; it is not changed here.BitmapIndices.internalGet) throw was considered but rejected:get(Class, String)is a documented public API that returnsnullon a miss andIndexIdentifier.resolveForrelies on thatnull, so throwing there would break the public contract. That consequence is covered by documentation only.Testing
GigaMap88Test: 2/2 pass.gigamapmodule suite: 1075 tests, 0 failures (1 pre-existing skip), BUILD SUCCESS.