From fd1b9ec5405207ba1aab79baa5f16d5372e5f2e4 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 7 Jul 2026 19:44:28 -0400 Subject: [PATCH 1/5] Change deleteStore to check StoreHeader before bumping MetaDataVersionStamp In particular the catalog uses the MetaDataVersion-based store cache, and when you delete a schema (as part of a test cleanup), the old deleteStore would invalidate the metadataversion cache, which would cause any catalog operations (which opened the store) to conflict. By being more restrictive, deleting stores that aren't caching the header means that we won't force a refresh on stores that are using the store header cache. Closes #4335 --- .../provider/foundationdb/FDBRecordStore.java | 38 ++++-- .../FDBRecordStoreStateCacheTest.java | 126 ++++++++++++++++++ 2 files changed, 155 insertions(+), 9 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java index 2a135af001..8ea85bef91 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java @@ -1799,12 +1799,16 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { * the store existed. * *

- * This method does not read the underlying record store, so it does not validate - * that a record store exists in the given subspace. As it might be the case that - * this record store has a cacheable store state (see {@link #setStateCacheability(boolean)}), - * this method resets the database's - * {@linkplain FDBRecordContext#getMetaDataVersionStamp(IsolationLevel) meta-data version-stamp}. - * As a result, calling this method may cause other clients to invalidate their caches needlessly. + * This method reads only the record store's header key (see {@link #STORE_INFO_KEY}) in + * order to decide whether it needs to invalidate cached state. If a header is present and + * marks the store as {@linkplain #setStateCacheability(boolean) cacheable}, the database's + * {@linkplain FDBRecordContext#getMetaDataVersionStamp(IsolationLevel) meta-data + * version-stamp} is reset so that other clients drop their cached copies; if the header is + * missing (store doesn't exist) or marks the store as non-cacheable, the version-stamp is + * not touched. This means callers who only ever operate on non-cacheable stores do not + * contend on the single meta-data version-stamp key. The header read uses snapshot + * isolation and does not add a read conflict, so it does not narrow the write-conflict + * behavior of the clear that follows. *

* * @param context the transactional context in which to delete the record store @@ -1812,9 +1816,25 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { */ @SuppressWarnings("PMD.CloseResource") public static void deleteStore(FDBRecordContext context, Subspace subspace) { - // In theory, we only need to set the meta-data version stamp if the record store's - // meta-data is cacheable, but we can't know that from here. - context.setMetaDataVersionStamp(); + // Read the store header at snapshot isolation so we don't add a needless read conflict: + // the clear below already creates a write conflict on the whole range, which is what + // actually serialises us against any concurrent writer of this store's header. + final byte[] headerKey = subspace.pack(STORE_INFO_KEY); + final byte[] headerBytes = context.asyncToSync(FDBStoreTimer.Waits.WAIT_LOAD_RECORD_STORE_STATE, + context.readTransaction(true).get(headerKey)); + if (headerBytes != null) { + boolean shouldBump = true; + try { + shouldBump = RecordMetaDataProto.DataStoreInfo.parseFrom(headerBytes).getCacheable(); + } catch (InvalidProtocolBufferException e) { + // If we can't parse the header, fall back to the pre-change conservative + // behavior and bump. This preserves the "delete a store I know nothing about" + // contract this method advertises. + } + if (shouldBump) { + context.setMetaDataVersionStamp(); + } + } context.setDirtyStoreState(true); context.clear(subspace.range()); } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 2e866a2a47..76b1e13599 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -45,6 +45,7 @@ import com.apple.foundationdb.record.provider.foundationdb.RecordStoreStaleMetaDataVersionException; import com.apple.foundationdb.record.provider.foundationdb.keyspace.KeySpacePath; import com.apple.foundationdb.record.test.FakeClusterFileUtil; +import com.apple.foundationdb.subspace.Subspace; import com.apple.foundationdb.tuple.ByteArrayUtil; import com.apple.foundationdb.tuple.ByteArrayUtil2; import com.apple.foundationdb.tuple.Tuple; @@ -766,6 +767,131 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte } } + /** + * Deleting a non-cacheable store must NOT bump the meta-data version stamp: no other + * client can possibly hold a cached copy of a non-cacheable header, so the version stamp + * (a JVM-wide bottleneck on the {@code \xff/metadataVersion} key) shouldn't be touched. + * This is the low-level property that lets parallel tests share the SYS catalog without + * conflicting on catalog teardown. + */ + @Test + void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp() throws Exception { + // Bootstrap: ensure the meta-data version stamp key has a value before the test runs, + // so we can distinguish "unchanged" from "was never set". The store below is opened + // with cacheability disabled (which is the default from setStateCacheability(false)). + try (FDBRecordContext context = fdb.openContext()) { + if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { + context.setMetaDataVersionStamp(); + } + commit(context); + } + + Subspace subspace; + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + assertFalse(recordStore.getRecordStoreState().getStoreHeader().getCacheable(), + "test presumes the store is non-cacheable by default"); + subspace = recordStore.getSubspace(); + commit(context); + } + + // Snapshot the version stamp before deletion — reading via SNAPSHOT so this txn doesn't + // conflict with the delete-txn below and skew the result. + final byte[] beforeStamp; + try (FDBRecordContext context = fdb.openContext()) { + beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + } + assertNotNull(beforeStamp, "bootstrap should have populated the meta-data version stamp"); + + try (FDBRecordContext context = openContext()) { + FDBRecordStore.deleteStore(context, subspace); + commit(context); + } + + try (FDBRecordContext context = fdb.openContext()) { + final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + assertArrayEquals(beforeStamp, afterStamp, + "deleting a non-cacheable store should not have bumped the meta-data version stamp"); + } + } + + /** + * Complement of {@link #deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp()}: deleting + * a cacheable store MUST bump the stamp — otherwise sibling clients could keep serving + * reads out of a stale cached header long after the store is gone. + */ + @Test + void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { + try (FDBRecordContext context = fdb.openContext()) { + if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { + context.setMetaDataVersionStamp(); + } + commit(context); + } + + Subspace subspace; + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + assertTrue(recordStore.setStateCacheability(true), "flipping to cacheable should have changed something"); + subspace = recordStore.getSubspace(); + commit(context); + } + // Commit above already bumped the stamp (transition to cacheable). Snapshot after that. + final byte[] beforeStamp; + try (FDBRecordContext context = fdb.openContext()) { + beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + } + assertNotNull(beforeStamp); + + try (FDBRecordContext context = openContext()) { + FDBRecordStore.deleteStore(context, subspace); + commit(context); + } + + try (FDBRecordContext context = fdb.openContext()) { + final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + assertNotNull(afterStamp); + assertFalse(java.util.Arrays.equals(beforeStamp, afterStamp), + "deleting a cacheable store should have bumped the meta-data version stamp"); + } + } + + /** + * Deleting an empty subspace (no store header present) must not bump the stamp either — + * there's no cached header to invalidate. + */ + @Test + void deleteMissingStoreDoesNotBumpMetaDataVersionStamp() throws Exception { + try (FDBRecordContext context = fdb.openContext()) { + if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { + context.setMetaDataVersionStamp(); + } + commit(context); + } + + // Use the test's per-instance path, but never open a store there. + final Subspace subspace; + try (FDBRecordContext context = openContext()) { + subspace = path.toSubspace(context); + } + final byte[] beforeStamp; + try (FDBRecordContext context = fdb.openContext()) { + beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + } + assertNotNull(beforeStamp); + + try (FDBRecordContext context = openContext()) { + FDBRecordStore.deleteStore(context, subspace); + commit(context); + } + + try (FDBRecordContext context = fdb.openContext()) { + final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + assertArrayEquals(beforeStamp, afterStamp, + "deleting an empty subspace (no header) should not have bumped the meta-data version stamp"); + } + } + /** * Verify that updating a header user field will be updated if the store state is cached. */ From fb747b2ed9f52a52edac60e1b321559102fba27a Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Mon, 13 Jul 2026 16:36:21 -0400 Subject: [PATCH 2/5] deleteStore: guard against concurrent setStateCacheability flip deleteStore reads STORE_INFO_KEY at snapshot isolation to avoid adding a read conflict on every delete. This leaves a race with a concurrent commit that flips the store to cacheable via setStateCacheability(true) and bumps the meta-data version stamp: our snapshot read misses the flip, we skip our own bump, and the delete lands. Sibling clients that populated their caches from the flipper committed state then keep serving reads out of a header for a store that no longer exists. Fix: only on the branches where we decide NOT to bump (header absent, or header parses as non-cacheable), add an explicit point read conflict on STORE_INFO_KEY. That way: - the common case (delete of a store that was and stays non-cacheable) still contributes zero read conflicts, and - the racy case (concurrent flip to cacheable) is detected: the writer SET on STORE_INFO_KEY overlaps our added read conflict and we lose the commit race. The cacheable-header branch and the invalid-header fallback continue to bump unconditionally without a read conflict -- bumping is safe regardless of what a concurrent writer does. Also updates the javadoc to describe the new semantics. Adds a regression test (concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump) that fails against the pre-fix implementation: it runs the two racing transactions with pinned read versions, forces the writer to commit first, and asserts that if the delete then commits the stamp must have advanced past the writer post-commit value. Closes #4335 --- .../provider/foundationdb/FDBRecordStore.java | 66 +++++-- .../FDBRecordStoreStateCacheTest.java | 166 ++++++++++++++++++ 2 files changed, 219 insertions(+), 13 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java index 8ea85bef91..64b77c140d 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java @@ -1806,9 +1806,19 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { * version-stamp} is reset so that other clients drop their cached copies; if the header is * missing (store doesn't exist) or marks the store as non-cacheable, the version-stamp is * not touched. This means callers who only ever operate on non-cacheable stores do not - * contend on the single meta-data version-stamp key. The header read uses snapshot - * isolation and does not add a read conflict, so it does not narrow the write-conflict - * behavior of the clear that follows. + * contend on the single meta-data version-stamp key. + *

+ * + *

+ * The header read uses snapshot isolation to avoid adding a read-conflict range on every + * delete. Only when the on-disk header says the store is non-cacheable (or is missing) — + * i.e., the case in which we would skip the bump — do we add an explicit point read + * conflict on {@link #STORE_INFO_KEY}. That closes the race with a concurrent + * {@link #setStateCacheability(boolean)} that flips the store to cacheable: if such a + * writer commits before us, our commit fails with a serialisation error and the caller + * retries with a fresh snapshot that sees the new header and bumps the stamp. In the + * common case (delete of a store that was and stays non-cacheable), no read conflict is + * added at all. *

* * @param context the transactional context in which to delete the record store @@ -1816,25 +1826,55 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { */ @SuppressWarnings("PMD.CloseResource") public static void deleteStore(FDBRecordContext context, Subspace subspace) { - // Read the store header at snapshot isolation so we don't add a needless read conflict: - // the clear below already creates a write conflict on the whole range, which is what - // actually serialises us against any concurrent writer of this store's header. + // Read the store header at snapshot isolation so we don't add a read conflict on + // every delete. The clear below already creates a write conflict on the whole range, + // which serialises us against any concurrent writer that lands INSIDE this subspace. + // The single race the snapshot read leaves open is a concurrent commit that writes + // STORE_INFO_KEY (e.g. a setStateCacheability that flips cacheable=true and bumps + // the meta-data version stamp). If that writer commits first, our snapshot read + // misses the flip and we would skip our own bump — leaving sibling caches populated + // from the writer's committed state stale after our delete. + // + // We close that race narrowly: only on the branches where we decide NOT to bump + // (header absent, or header parses as non-cacheable), we add an explicit point read + // conflict on STORE_INFO_KEY. That way: + // - the common case (delete of a store that was and stays non-cacheable) still + // contributes zero read conflicts, and + // - the racy case (concurrent flip to cacheable) is detected: the writer's SET on + // STORE_INFO_KEY overlaps our added read conflict and we lose the commit race. final byte[] headerKey = subspace.pack(STORE_INFO_KEY); final byte[] headerBytes = context.asyncToSync(FDBStoreTimer.Waits.WAIT_LOAD_RECORD_STORE_STATE, context.readTransaction(true).get(headerKey)); - if (headerBytes != null) { - boolean shouldBump = true; + boolean shouldBump; + if (headerBytes == null) { + // Header absent: no cached state exists to invalidate — no bump needed. But guard + // against a concurrent creator/enabler flipping the store to cacheable underneath + // us: adding a read conflict on STORE_INFO_KEY makes such a writer's commit + // exclude ours. + context.ensureActive().addReadConflictKey(headerKey); + shouldBump = false; + } else { + boolean cacheable; try { - shouldBump = RecordMetaDataProto.DataStoreInfo.parseFrom(headerBytes).getCacheable(); + cacheable = RecordMetaDataProto.DataStoreInfo.parseFrom(headerBytes).getCacheable(); } catch (InvalidProtocolBufferException e) { // If we can't parse the header, fall back to the pre-change conservative - // behavior and bump. This preserves the "delete a store I know nothing about" - // contract this method advertises. + // behaviour and bump. Also skip the read conflict — bumping unconditionally + // is safe regardless of what a concurrent writer does. + cacheable = true; } - if (shouldBump) { - context.setMetaDataVersionStamp(); + if (cacheable) { + shouldBump = true; + } else { + // Header says non-cacheable AND parsed cleanly. If a concurrent writer flips + // it to cacheable, we must lose the commit race so the caller retries. + context.ensureActive().addReadConflictKey(headerKey); + shouldBump = false; } } + if (shouldBump) { + context.setMetaDataVersionStamp(); + } context.setDirtyStoreState(true); context.clear(subspace.range()); } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 76b1e13599..80f041b080 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -76,6 +76,7 @@ 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 static org.junit.jupiter.api.Assertions.fail; /** * Tests to make sure that caching {@link FDBRecordStoreStateCacheEntry} objects work. @@ -892,6 +893,123 @@ void deleteMissingStoreDoesNotBumpMetaDataVersionStamp() throws Exception { } } + /** + * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)}. Three + * transactions in this sequence — {@code T_flipper} and {@code T_deleter} race, then + * {@code T_writer} exercises the observable inconsistency: + * + *
    + *
  1. {@code T_deleter} pins its read version before any flip has committed.
    2. + *
  2. {@code T_flipper} (a separate transaction) flips the store to cacheable and + * commits.
    3. + *
  3. A warm-up transaction opens the store so the JVM-wide state cache is + * populated with a {@code (cacheable=true)} entry.
    4. + *
  4. {@code T_deleter} calls {@link FDBRecordStore#deleteStore} and commits. Its + * snapshot-isolation header read at the pinned pre-flip version sees + * {@code cacheable=false} and (in the buggy code) skips both the bump and any + * read conflict on {@code STORE_INFO_KEY}.
    5. + *
  5. {@code T_writer} — a separate transaction — opens the store, hits + * the (stale) cache entry, and saves a record. Because the writer never reads + * the disk header itself and only trusts the cache, it commits happily.
    6. + *
+ * + *

Acceptable outcomes:

+ *
    + *
  1. {@code T_deleter} conflicts with the flipper's {@code STORE_INFO_KEY} write + * and does not land. The store still exists on disk; the cache is consistent + * with reality.
    2. + *
  2. {@code T_deleter} commits but the meta-data version stamp advances (it also + * bumped) so the writer's cache lookup misses and reloads the fresh, absent + * header — reporting "no store" and refusing to save the orphan record.
    3. + *
+ * + *

The bug we're guarding against: {@code T_deleter} commits without bumping the + * stamp, the writer's cache lookup hits the stale entry, and the writer inserts a record + * into a subspace whose store header has been deleted. On-disk end state: no header, but + * records present — an orphan set of rows nothing can safely read again.

+ */ + @Test + void concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump() throws Exception { + fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); + ensureMetaDataVersionStampInitialized(); + + // Setup: create the store as NON-cacheable. That is the trap for the deleter's + // snapshot header read. + FDBRecordStore.Builder storeBuilder; + Subspace subspace; + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + assertFalse(recordStore.getRecordStoreState().getStoreHeader().getCacheable(), + "test presumes the store is non-cacheable by default"); + storeBuilder = recordStore.asBuilder(); + subspace = recordStore.getSubspace(); + commit(context); + } + + final boolean deleterCommitted; + try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { + // Pin the deleter's read version BEFORE the flip commits, so the deleter's + // snapshot header read sees the pre-flip (non-cacheable) state. + deleterContext.getReadVersion(); + + // Separate transaction: flip the store to cacheable. Kept separate from the + // record insert below so that the record-inserting transaction has no reason to + // read the header itself — it will pick up the cacheable header via the state + // cache. + try (FDBRecordContext flipperContext = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore flipperStore = storeBuilder.copyBuilder().setContext(flipperContext).open(); + assertTrue(flipperStore.setStateCacheability(true), + "flipping to cacheable should have changed the header"); + commit(flipperContext); + } + + // Warm the JVM-wide state cache: a fresh open goes through the cache's + // get() → cache-miss → load-fresh → addToCache(cacheable=true). Without this + // the writer below would miss the cache and read STORE_INFO_KEY directly (which + // would see the deleter's clear and correctly treat the store as absent), + // masking the bug. + try (FDBRecordContext warmup = fdb.openContext(null, new FDBStoreTimer())) { + storeBuilder.copyBuilder().setContext(warmup).open(); + } + + // T_deleter: reads STORE_INFO_KEY at snapshot at its pinned pre-flip RV, sees + // non-cacheable. In the fixed code, this branch also adds a point read conflict + // on STORE_INFO_KEY — which the flipper's committed write will conflict with. + FDBRecordStore.deleteStore(deleterContext, subspace); + deleterCommitted = tryCommitOrDetectConflict(deleterContext); + } + + if (deleterCommitted) { + // Bug reproduction: a separate transaction opens the store via the cache and + // saves a record. The writer never reads STORE_INFO_KEY itself — the cache + // entry (populated during warmup) still validates because the deleter did not + // bump the stamp. The record write commits successfully, producing an on-disk + // orphan: no store header, but records present. + try (FDBRecordContext writerContext = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore writerStore = storeBuilder.copyBuilder().setContext(writerContext).open(); + writerStore.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() + .setRecNo(1) + .setStrValueIndexed("orphaned-writer") + .build()); + commit(writerContext); + } + // At this point without the fix: STORE_INFO_KEY is absent AND the record is + // present. Fail loudly and describe the failure mode — this is the exact + // "no header but not empty" orphan the fix is meant to prevent. With the fix, + // the deleter would have conflicted, we would not be here at all. + fail("delete committed after a concurrent setStateCacheability(true) flip " + + "and did not bump the meta-data version stamp; a subsequent writer " + + "trusted its (now-stale) cache entry and inserted a record into a " + + "subspace whose store header has been deleted — an on-disk orphan. " + + "This is the correctness bug deleteStore's read-conflict guard is meant " + + "to prevent."); + } else { + // Deleter correctly conflicted with the flipper. The store still exists (the + // flipper's cacheable header); caches remain consistent with disk. + assertStoreHeaderPresent(subspace); + } + } + /** * Verify that updating a header user field will be updated if the store state is cached. */ @@ -1380,4 +1498,52 @@ private void assertNotCacheable() { private boolean isStoreCachable() { return recordStore.getRecordStoreState().getStoreHeader().getCacheable(); } + + // ---- helpers for the concurrent-deleteStore race test ------------------------------- + + /** + * Bootstrap that guarantees the JVM-wide meta-data version stamp key exists, so callers + * can compare before/after snapshots without special-casing null. + */ + private void ensureMetaDataVersionStampInitialized() throws Exception { + try (FDBRecordContext context = fdb.openContext()) { + if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { + context.setMetaDataVersionStamp(); + } + commit(context); + } + } + + /** + * Attempt to commit the given context. Returns {@code true} on success, {@code false} if + * the commit failed with a conflict (any nested cause). Any other exception is re-raised + * so an unexpected failure mode doesn't silently masquerade as "conflict". + */ + private boolean tryCommitOrDetectConflict(@Nonnull FDBRecordContext context) throws Exception { + try { + commit(context); + return true; + } catch (Exception ex) { + for (Throwable cause = ex; cause != null; cause = cause.getCause()) { + if (cause instanceof FDBExceptions.FDBStoreTransactionConflictException) { + return false; + } + } + throw new AssertionError("unexpected exception from commit: " + ex, ex); + } + } + + /** + * Assert the store header is still present at the given subspace (used when the deleter + * loses the commit race and the store should still exist on disk). + */ + private void assertStoreHeaderPresent(@Nonnull Subspace subspace) throws Exception { + try (FDBRecordContext peek = fdb.openContext()) { + final byte[] header = peek.readTransaction(true) + .get(subspace.pack(FDBRecordStoreKeyspace.STORE_INFO.key())) + .join(); + assertNotNull(header, + "when the deleter conflicts, the store header should still be present on disk"); + } + } } From 756a08ddb595869b5bf222019b32cf06d2cfe617 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 14 Jul 2026 13:13:11 -0400 Subject: [PATCH 3/5] Simplify tests and inverted version of conflicting one A bunch of DRY and removing unnecessary logic. Also have a version of the conflict test with the commits in the other order --- .../FDBRecordStoreStateCacheTest.java | 276 ++++++++---------- 1 file changed, 116 insertions(+), 160 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 80f041b080..8a0629099c 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -25,8 +25,8 @@ import com.apple.foundationdb.record.RecordCoreException; import com.apple.foundationdb.record.RecordMetaData; import com.apple.foundationdb.record.RecordMetaDataBuilder; -import com.apple.foundationdb.record.expressions.RecordKeyExpressionProto; import com.apple.foundationdb.record.TestRecords1Proto; +import com.apple.foundationdb.record.expressions.RecordKeyExpressionProto; import com.apple.foundationdb.record.metadata.Key; import com.apple.foundationdb.record.metadata.expressions.KeyExpression; import com.apple.foundationdb.record.provider.foundationdb.FDBDatabase; @@ -57,6 +57,7 @@ import org.junit.jupiter.params.provider.MethodSource; import javax.annotation.Nonnull; +import javax.annotation.Nullable; import java.util.Arrays; import java.util.Collections; import java.util.UUID; @@ -772,48 +773,49 @@ public void storeDeletionAcrossContexts(@Nonnull StateCacheTestContext testConte * Deleting a non-cacheable store must NOT bump the meta-data version stamp: no other * client can possibly hold a cached copy of a non-cacheable header, so the version stamp * (a JVM-wide bottleneck on the {@code \xff/metadataVersion} key) shouldn't be touched. - * This is the low-level property that lets parallel tests share the SYS catalog without - * conflicting on catalog teardown. + * This means that deleting a store won't cause any interaction with any cacheable store to conflict. */ @Test void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp() throws Exception { - // Bootstrap: ensure the meta-data version stamp key has a value before the test runs, - // so we can distinguish "unchanged" from "was never set". The store below is opened - // with cacheability disabled (which is the default from setStateCacheability(false)). - try (FDBRecordContext context = fdb.openContext()) { - if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { - context.setMetaDataVersionStamp(); - } - commit(context); - } + ensureMetaDataVersionStampInitialized(); Subspace subspace; try (FDBRecordContext context = openContext()) { openSimpleRecordStore(context); - assertFalse(recordStore.getRecordStoreState().getStoreHeader().getCacheable(), - "test presumes the store is non-cacheable by default"); + assertNotCacheable(); subspace = recordStore.getSubspace(); commit(context); } - // Snapshot the version stamp before deletion — reading via SNAPSHOT so this txn doesn't - // conflict with the delete-txn below and skew the result. - final byte[] beforeStamp; - try (FDBRecordContext context = fdb.openContext()) { - beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - } - assertNotNull(beforeStamp, "bootstrap should have populated the meta-data version stamp"); + deleteStoreDoesNotBumpMetaDataVersion(subspace); + } + + /** + * Deleting an empty subspace (no store header present) must not bump the stamp either — + * there's no cached header to invalidate. + */ + @Test + void deleteMissingStoreDoesNotBumpMetaDataVersionStamp() { + ensureMetaDataVersionStampInitialized(); + // Use the test's per-instance path, but never open a store there. + final Subspace subspace; try (FDBRecordContext context = openContext()) { - FDBRecordStore.deleteStore(context, subspace); - commit(context); + subspace = path.toSubspace(context); } + deleteStoreDoesNotBumpMetaDataVersion(subspace); + } - try (FDBRecordContext context = fdb.openContext()) { - final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - assertArrayEquals(beforeStamp, afterStamp, - "deleting a non-cacheable store should not have bumped the meta-data version stamp"); - } + private void deleteStoreDoesNotBumpMetaDataVersion(final Subspace subspace) { + final byte[] beforeStamp = getMetaDataVersionStamp(); + assertNotNull(beforeStamp); + + deleteStore(subspace); + + final byte[] afterStamp = getMetaDataVersionStamp(); + assertNotNull(afterStamp); + assertArrayEquals(beforeStamp, afterStamp, + "deleting a cacheable store should have bumped the meta-data version stamp"); } /** @@ -823,12 +825,7 @@ void deleteNonCacheableStoreDoesNotBumpMetaDataVersionStamp() throws Exception { */ @Test void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { - try (FDBRecordContext context = fdb.openContext()) { - if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { - context.setMetaDataVersionStamp(); - } - commit(context); - } + ensureMetaDataVersionStampInitialized(); Subspace subspace; try (FDBRecordContext context = openContext()) { @@ -838,98 +835,27 @@ void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { commit(context); } // Commit above already bumped the stamp (transition to cacheable). Snapshot after that. - final byte[] beforeStamp; - try (FDBRecordContext context = fdb.openContext()) { - beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - } + final byte[] beforeStamp = getMetaDataVersionStamp(); assertNotNull(beforeStamp); - try (FDBRecordContext context = openContext()) { - FDBRecordStore.deleteStore(context, subspace); - commit(context); - } + deleteStore(subspace); try (FDBRecordContext context = fdb.openContext()) { final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); assertNotNull(afterStamp); - assertFalse(java.util.Arrays.equals(beforeStamp, afterStamp), + assertFalse(Arrays.equals(beforeStamp, afterStamp), "deleting a cacheable store should have bumped the meta-data version stamp"); } } /** - * Deleting an empty subspace (no store header present) must not bump the stamp either — - * there's no cached header to invalidate. - */ - @Test - void deleteMissingStoreDoesNotBumpMetaDataVersionStamp() throws Exception { - try (FDBRecordContext context = fdb.openContext()) { - if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { - context.setMetaDataVersionStamp(); - } - commit(context); - } - - // Use the test's per-instance path, but never open a store there. - final Subspace subspace; - try (FDBRecordContext context = openContext()) { - subspace = path.toSubspace(context); - } - final byte[] beforeStamp; - try (FDBRecordContext context = fdb.openContext()) { - beforeStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - } - assertNotNull(beforeStamp); - - try (FDBRecordContext context = openContext()) { - FDBRecordStore.deleteStore(context, subspace); - commit(context); - } - - try (FDBRecordContext context = fdb.openContext()) { - final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - assertArrayEquals(beforeStamp, afterStamp, - "deleting an empty subspace (no header) should not have bumped the meta-data version stamp"); - } - } - - /** - * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)}. Three - * transactions in this sequence — {@code T_flipper} and {@code T_deleter} race, then - * {@code T_writer} exercises the observable inconsistency: - * - *
    - *
  1. {@code T_deleter} pins its read version before any flip has committed.
    2. - *
  2. {@code T_flipper} (a separate transaction) flips the store to cacheable and - * commits.
    3. - *
  3. A warm-up transaction opens the store so the JVM-wide state cache is - * populated with a {@code (cacheable=true)} entry.
    4. - *
  4. {@code T_deleter} calls {@link FDBRecordStore#deleteStore} and commits. Its - * snapshot-isolation header read at the pinned pre-flip version sees - * {@code cacheable=false} and (in the buggy code) skips both the bump and any - * read conflict on {@code STORE_INFO_KEY}.
    5. - *
  5. {@code T_writer} — a separate transaction — opens the store, hits - * the (stale) cache entry, and saves a record. Because the writer never reads - * the disk header itself and only trusts the cache, it commits happily.
    6. - *
- * - *

Acceptable outcomes:

- *
    - *
  1. {@code T_deleter} conflicts with the flipper's {@code STORE_INFO_KEY} write - * and does not land. The store still exists on disk; the cache is consistent - * with reality.
    2. - *
  2. {@code T_deleter} commits but the meta-data version stamp advances (it also - * bumped) so the writer's cache lookup misses and reloads the fresh, absent - * header — reporting "no store" and refusing to save the orphan record.
    3. - *
- * - *

The bug we're guarding against: {@code T_deleter} commits without bumping the - * stamp, the writer's cache lookup hits the stale entry, and the writer inserts a record - * into a subspace whose store header has been deleted. On-disk end state: no header, but - * records present — an orphan set of rows nothing can safely read again.

+ * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)}. + *

The bug we're guarding against: {@code deleteStore} commits without bumping the + * stamp, concurrently with another transaction that sets the store as cacheable. + * The delete should conflict.

*/ @Test - void concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump() throws Exception { + void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); ensureMetaDataVersionStampInitialized(); @@ -939,8 +865,7 @@ void concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump() throws Exception { Subspace subspace; try (FDBRecordContext context = openContext()) { openSimpleRecordStore(context); - assertFalse(recordStore.getRecordStoreState().getStoreHeader().getCacheable(), - "test presumes the store is non-cacheable by default"); + assertNotCacheable(); storeBuilder = recordStore.asBuilder(); subspace = recordStore.getSubspace(); commit(context); @@ -963,40 +888,14 @@ void concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump() throws Exception { commit(flipperContext); } - // Warm the JVM-wide state cache: a fresh open goes through the cache's - // get() → cache-miss → load-fresh → addToCache(cacheable=true). Without this - // the writer below would miss the cache and read STORE_INFO_KEY directly (which - // would see the deleter's clear and correctly treat the store as absent), - // masking the bug. - try (FDBRecordContext warmup = fdb.openContext(null, new FDBStoreTimer())) { - storeBuilder.copyBuilder().setContext(warmup).open(); - } - - // T_deleter: reads STORE_INFO_KEY at snapshot at its pinned pre-flip RV, sees - // non-cacheable. In the fixed code, this branch also adds a point read conflict + // deleterContext: reads STORE_INFO_KEY at snapshot at its pinned pre-flip RV, sees + // non-cacheable. The deleteStore also adds a point read conflict // on STORE_INFO_KEY — which the flipper's committed write will conflict with. FDBRecordStore.deleteStore(deleterContext, subspace); deleterCommitted = tryCommitOrDetectConflict(deleterContext); } if (deleterCommitted) { - // Bug reproduction: a separate transaction opens the store via the cache and - // saves a record. The writer never reads STORE_INFO_KEY itself — the cache - // entry (populated during warmup) still validates because the deleter did not - // bump the stamp. The record write commits successfully, producing an on-disk - // orphan: no store header, but records present. - try (FDBRecordContext writerContext = fdb.openContext(null, new FDBStoreTimer())) { - FDBRecordStore writerStore = storeBuilder.copyBuilder().setContext(writerContext).open(); - writerStore.saveRecord(TestRecords1Proto.MySimpleRecord.newBuilder() - .setRecNo(1) - .setStrValueIndexed("orphaned-writer") - .build()); - commit(writerContext); - } - // At this point without the fix: STORE_INFO_KEY is absent AND the record is - // present. Fail loudly and describe the failure mode — this is the exact - // "no header but not empty" orphan the fix is meant to prevent. With the fix, - // the deleter would have conflicted, we would not be here at all. fail("delete committed after a concurrent setStateCacheability(true) flip " + "and did not bump the meta-data version stamp; a subsequent writer " + "trusted its (now-stale) cache entry and inserted a record into a " + @@ -1006,7 +905,64 @@ void concurrentSetCacheabilityAndDeleteDoesNotLoseTheBump() throws Exception { } else { // Deleter correctly conflicted with the flipper. The store still exists (the // flipper's cacheable header); caches remain consistent with disk. - assertStoreHeaderPresent(subspace); + try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { + // ensure that we can open the store, guaranteeing the store header still exists + recordStore = storeBuilder.copyBuilder().setContext(contextUsingCache).open(); + assertCacheable(); + } + } + } + + /** + * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)}. + *

The bug we're guarding against: {@code deleteStore} commits without bumping the + * stamp, concurrently with another transaction that sets the store as cacheable. + * The context trying to set cacheability should conflict.

+ */ + @Test + void concurrentSetCacheabilityConflictsWithDeleteStore() throws Exception { + fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); + ensureMetaDataVersionStampInitialized(); + + // Setup: create the store as NON-cacheable. That is the trap for the deleter's + // snapshot header read. + FDBRecordStore.Builder storeBuilder; + Subspace subspace; + try (FDBRecordContext context = openContext()) { + openSimpleRecordStore(context); + assertNotCacheable(); + storeBuilder = recordStore.asBuilder(); + subspace = recordStore.getSubspace(); + commit(context); + } + + final boolean flipperCommitted; + try (FDBRecordContext flipperContext = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore flipperStore = storeBuilder.copyBuilder().setContext(flipperContext).open(); + try (FDBRecordContext deleterContext = fdb.openContext(null, new FDBStoreTimer())) { + FDBRecordStore.deleteStore(deleterContext, subspace); + commit(deleterContext); + } + assertTrue(flipperStore.setStateCacheability(true), + "flipping to cacheable should have changed the header"); + flipperCommitted = tryCommitOrDetectConflict(flipperContext); + } + + if (flipperCommitted) { + fail("delete committed after a concurrent setStateCacheability(true) flip " + + "and did not bump the meta-data version stamp; a subsequent writer " + + "trusted its (now-stale) cache entry and inserted a record into a " + + "subspace whose store header has been deleted — an on-disk orphan. " + + "This is the correctness bug deleteStore's read-conflict guard is meant " + + "to prevent."); + } else { + // Deleter correctly conflicted with the flipper. The store still should have been deleted. + try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { + assertTrue(contextUsingCache.ensureActive().getRange(subspace.range()).asList().join().isEmpty(), + "The store should have been deleted"); + recordStore = storeBuilder.copyBuilder().setContext(contextUsingCache).create(); + assertNotCacheable(); + } } } @@ -1502,10 +1458,10 @@ private boolean isStoreCachable() { // ---- helpers for the concurrent-deleteStore race test ------------------------------- /** - * Bootstrap that guarantees the JVM-wide meta-data version stamp key exists, so callers + * Bootstrap that guarantees the cluster-wide meta-data version stamp key exists, so callers * can compare before/after snapshots without special-casing null. */ - private void ensureMetaDataVersionStampInitialized() throws Exception { + private void ensureMetaDataVersionStampInitialized() { try (FDBRecordContext context = fdb.openContext()) { if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { context.setMetaDataVersionStamp(); @@ -1514,6 +1470,20 @@ private void ensureMetaDataVersionStampInitialized() throws Exception { } } + @Nullable + private byte[] getMetaDataVersionStamp() { + try (FDBRecordContext context = fdb.openContext()) { + return context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); + } + } + + private void deleteStore(final Subspace subspace) { + try (FDBRecordContext context = openContext()) { + FDBRecordStore.deleteStore(context, subspace); + commit(context); + } + } + /** * Attempt to commit the given context. Returns {@code true} on success, {@code false} if * the commit failed with a conflict (any nested cause). Any other exception is re-raised @@ -1532,18 +1502,4 @@ private boolean tryCommitOrDetectConflict(@Nonnull FDBRecordContext context) thr throw new AssertionError("unexpected exception from commit: " + ex, ex); } } - - /** - * Assert the store header is still present at the given subspace (used when the deleter - * loses the commit race and the store should still exist on disk). - */ - private void assertStoreHeaderPresent(@Nonnull Subspace subspace) throws Exception { - try (FDBRecordContext peek = fdb.openContext()) { - final byte[] header = peek.readTransaction(true) - .get(subspace.pack(FDBRecordStoreKeyspace.STORE_INFO.key())) - .join(); - assertNotNull(header, - "when the deleter conflicts, the store header should still be present on disk"); - } - } } From 5dc8ed31f7080dc409411b561b81d4073e2e6613 Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 14 Jul 2026 14:50:26 -0400 Subject: [PATCH 4/5] Cleanup comments / assertion messages --- .../provider/foundationdb/FDBRecordStore.java | 8 +++--- .../FDBRecordStoreStateCacheTest.java | 28 ++++++++----------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java index 64b77c140d..e67bc86d44 100644 --- a/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java +++ b/fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/provider/foundationdb/FDBRecordStore.java @@ -1815,7 +1815,7 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { * i.e., the case in which we would skip the bump — do we add an explicit point read * conflict on {@link #STORE_INFO_KEY}. That closes the race with a concurrent * {@link #setStateCacheability(boolean)} that flips the store to cacheable: if such a - * writer commits before us, our commit fails with a serialisation error and the caller + * writer commits before us, our commit fails with a serialization error and the caller * retries with a fresh snapshot that sees the new header and bumps the stamp. In the * common case (delete of a store that was and stays non-cacheable), no read conflict is * added at all. @@ -1828,7 +1828,7 @@ public static void deleteStore(FDBRecordContext context, KeySpacePath path) { public static void deleteStore(FDBRecordContext context, Subspace subspace) { // Read the store header at snapshot isolation so we don't add a read conflict on // every delete. The clear below already creates a write conflict on the whole range, - // which serialises us against any concurrent writer that lands INSIDE this subspace. + // which serializes us against any concurrent writer that lands INSIDE this subspace. // The single race the snapshot read leaves open is a concurrent commit that writes // STORE_INFO_KEY (e.g. a setStateCacheability that flips cacheable=true and bumps // the meta-data version stamp). If that writer commits first, our snapshot read @@ -1858,8 +1858,8 @@ public static void deleteStore(FDBRecordContext context, Subspace subspace) { try { cacheable = RecordMetaDataProto.DataStoreInfo.parseFrom(headerBytes).getCacheable(); } catch (InvalidProtocolBufferException e) { - // If we can't parse the header, fall back to the pre-change conservative - // behaviour and bump. Also skip the read conflict — bumping unconditionally + // If we can't parse the header, fall back to the conservative + // behavior and bump. Also skip the read conflict — bumping unconditionally // is safe regardless of what a concurrent writer does. cacheable = true; } diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index 8a0629099c..eddf156ac7 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -815,7 +815,7 @@ private void deleteStoreDoesNotBumpMetaDataVersion(final Subspace subspace) { final byte[] afterStamp = getMetaDataVersionStamp(); assertNotNull(afterStamp); assertArrayEquals(beforeStamp, afterStamp, - "deleting a cacheable store should have bumped the meta-data version stamp"); + "deleting a cacheable store should not have bumped the meta-data version stamp"); } /** @@ -898,7 +898,7 @@ void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { if (deleterCommitted) { fail("delete committed after a concurrent setStateCacheability(true) flip " + "and did not bump the meta-data version stamp; a subsequent writer " + - "trusted its (now-stale) cache entry and inserted a record into a " + + "could have trusted its (now-stale) cache entry and inserted a record into a " + "subspace whose store header has been deleted — an on-disk orphan. " + "This is the correctness bug deleteStore's read-conflict guard is meant " + "to prevent."); @@ -914,18 +914,19 @@ void concurrentSetCacheabilityPreventsDeleteStore() throws Exception { } /** - * Race check for {@link FDBRecordStore#deleteStore(FDBRecordContext, Subspace)}. - *

The bug we're guarding against: {@code deleteStore} commits without bumping the - * stamp, concurrently with another transaction that sets the store as cacheable. - * The context trying to set cacheability should conflict.

+ * Companion invariant to {@link #concurrentSetCacheabilityPreventsDeleteStore()}: with the + * commit order flipped (deleter first, then flipper), the flipper must conflict. Unlike its + * companion, this test relies on general read/write conflict machinery — specifically, that + * setStateCacheability's own SERIALIZABLE header read (via open() → loadRecordStoreStateAsync + * or handleCachedState) puts STORE_INFO_KEY in the flipper's read set, so any concurrent + * write to that key by a preceding delete forces a conflict. */ @Test void concurrentSetCacheabilityConflictsWithDeleteStore() throws Exception { fdb.setStoreStateCache(metaDataVersionStampCacheFactory.getCache(fdb)); ensureMetaDataVersionStampInitialized(); - // Setup: create the store as NON-cacheable. That is the trap for the deleter's - // snapshot header read. + // Setup: create the store as NON-cacheable (matches the companion test's starting state). FDBRecordStore.Builder storeBuilder; Subspace subspace; try (FDBRecordContext context = openContext()) { @@ -949,12 +950,9 @@ void concurrentSetCacheabilityConflictsWithDeleteStore() throws Exception { } if (flipperCommitted) { - fail("delete committed after a concurrent setStateCacheability(true) flip " + - "and did not bump the meta-data version stamp; a subsequent writer " + - "trusted its (now-stale) cache entry and inserted a record into a " + - "subspace whose store header has been deleted — an on-disk orphan. " + - "This is the correctness bug deleteStore's read-conflict guard is meant " + - "to prevent."); + fail("a setStateCacheability(true) flip committed after a successful deleteStore; " + + "another transaction could have used the cached state to write after the " + + "delete store without there being a store header"); } else { // Deleter correctly conflicted with the flipper. The store still should have been deleted. try (FDBRecordContext contextUsingCache = fdb.openContext(null, new FDBStoreTimer())) { @@ -1455,8 +1453,6 @@ private boolean isStoreCachable() { return recordStore.getRecordStoreState().getStoreHeader().getCacheable(); } - // ---- helpers for the concurrent-deleteStore race test ------------------------------- - /** * Bootstrap that guarantees the cluster-wide meta-data version stamp key exists, so callers * can compare before/after snapshots without special-casing null. From 0f1eefd89ef14ff2fc1b2a85a0bbd26dbb98d57b Mon Sep 17 00:00:00 2001 From: Scott Dugas Date: Tue, 14 Jul 2026 15:07:29 -0400 Subject: [PATCH 5/5] Slightly more test DRY --- .../FDBRecordStoreStateCacheTest.java | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java index eddf156ac7..e66f47849f 100644 --- a/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java +++ b/fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/storestate/FDBRecordStoreStateCacheTest.java @@ -840,12 +840,10 @@ void deleteCacheableStoreBumpsMetaDataVersionStamp() throws Exception { deleteStore(subspace); - try (FDBRecordContext context = fdb.openContext()) { - final byte[] afterStamp = context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT); - assertNotNull(afterStamp); - assertFalse(Arrays.equals(beforeStamp, afterStamp), - "deleting a cacheable store should have bumped the meta-data version stamp"); - } + final byte[] afterStamp = getMetaDataVersionStamp(); + assertNotNull(afterStamp); + assertFalse(Arrays.equals(beforeStamp, afterStamp), + "deleting a cacheable store should have bumped the meta-data version stamp"); } /** @@ -1180,12 +1178,7 @@ void setCacheabilityDuringStoreOpening() { .getCache(fdb); fdb.setStoreStateCache(storeStateCache); - try (FDBRecordContext context = fdb.openContext()) { - if (context.getMetaDataVersionStamp(IsolationLevel.SNAPSHOT) == null) { - context.setMetaDataVersionStamp(); - } - commit(context); - } + ensureMetaDataVersionStampInitialized(); // Create the store, initially not cacheable FDBStoreTimer timer = new FDBStoreTimer();