Skip to content

Commit 90f14f1

Browse files
authored
feat: Drop persistent-store cache after FDv2 in-memory store init (#167)
## Summary With FDv2, the in-memory store retains every flag and segment once it has received a full payload. The persistent-store wrapper's three internal Guava caches (item, all-items, init) become dead weight at that point, roughly doubling the in-memory footprint of flag data (or worse, indefinitely, in `cacheForever()` mode). This change introduces an internal `DisableableCache` capability interface and a `disableCache()` method on `PersistentDataStoreWrapper`. The method sets a `volatile boolean cacheDisabled` flag and then invalidates all three Guava caches. Every cache touch site (`isInitialized`, `init`, `get`, `getAll`, `upsert`, `getCacheStats`, `pollAvailabilityAfterOutage`) checks the flag and short-circuits to the core path when set. The flag-check is required because the caches are `LoadingCache` instances and plain `invalidateAll()` would auto-repopulate via the registered `CacheLoader`s on the next `get()`. `WriteThroughStore.maybeSwitchStore()` probes for the interface via `instanceof` and invokes `disableCache()` inside the existing `synchronized (activeStoreLock)` block, immediately after the active read store is flipped to the in-memory store. The probe is unconditional with respect to `DataStoreMode`: reads bypass the persistent store in both `READ_ONLY` and `READ_WRITE` modes after the flip, so the cache is dead weight in either mode. The `PersistentDataStoreBuilder` cache-config setters (`noCaching`, `cacheTime`, `cacheMillis`, `cacheSeconds`, `cacheForever`, `staleValuesPolicy`, `recordCacheStats`) remain functional during the bootstrap window for backward compatibility. Class-level javadoc on the builder explains that under FDv2 these only govern the window before the in-memory store has received its first payload. No `@Deprecated` annotation; the SDK source has no `@Deprecated` precedent and Matthew Keeler's pattern across Python, Ruby, and Go uses doc-only deprecation. Mirrors the same change shipped for Python (launchdarkly/python-server-sdk#426), Ruby (launchdarkly/ruby-server-sdk#384), Go (launchdarkly/go-server-sdk#373), and .NET (launchdarkly/dotnet-core#274). **Naming note:** the ticket text says `CacheClearable` / `clearCache()`. This change uses `DisableableCache` / `disableCache()` instead so the verb conveys that the cache is off going forward (a state change), not just emptied (a one-time imperative). This aligns with Python's `disable_cache` and Ruby's `disable_cache`, and matches the `.NET` sibling PR which made the same naming choice. **Concurrency note:** an in-flight reader that passed the `cacheDisabled` check before `disableCache()` ran can still complete its cache operation and, on a miss, fire the `LoadingCache` loader. The fresh value gets stored back in the cache after our `invalidateAll()`. Those leftover entries are unreachable from any subsequent read (every future caller bypasses) and hold fresh, correct values, so they don't cause stale reads or torn observations. They simply persist in the cache instance until GC reclaims it. See dotnet-core#274 for the full analysis of this trade-off vs. a Go-style `AtomicReference` swap + read-once-per-call refactor. Tests follow the indirect-observation pattern: prime the cache, call `disableCache()`, mutate the core directly, assert the next read sees the new value. A new `MockDisableableCachePersistentStore` spy (extending `MockTransactionalPersistentStore`) drives the `WriteThroughStore` probe-and-invoke coverage, including the FDv1 `init` path, the FDv2 `apply` path, the delta-does-not-redrop case, the `READ_ONLY`-mode-still-drops case, and the non-disableable-store no-op case. No `Thread.sleep`. `./gradlew test` -- 1906 tests, 0 failures. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes the server SDK persistent-store read/write path at the FDv2 handoff; behavior is guarded by tests and matches other language SDKs, but incorrect timing could affect bootstrap reads. > > **Overview** > Adds an internal **`DisableableCache`** / **`disableCache()`** path so the persistent data store wrapper stops using its Guava caches after FDv2 hands reads to the in-memory store. > > **`PersistentDataStoreWrapper`** sets a **`cacheDisabled`** flag, invalidates item/all/init caches, and skips cache on **`get`**, **`getAll`**, **`init`**, **`upsert`**, **`isInitialized`**, **`getCacheStats`**, and outage recovery so **`LoadingCache`** loaders are not repopulated. > > **`WriteThroughStore`** calls **`disableCache()`** once inside **`maybeSwitchStore()`** when the first initializing payload flips the active read store to memory (including **`READ_ONLY`** and legacy **`init`**). > > **`PersistentDataStoreBuilder`** javadoc notes cache options only matter during the bootstrap window before that switch. New unit tests cover bypass behavior and the one-time probe. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6ca393f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent a363777 commit 90f14f1

6 files changed

Lines changed: 270 additions & 12 deletions

File tree

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
package com.launchdarkly.sdk.server;
2+
3+
/**
4+
* Optional interface for data stores that can disable their internal cache.
5+
* <p>
6+
* This is currently for internal implementations only.
7+
*/
8+
interface DisableableCache {
9+
/**
10+
* Disables the internal cache. After this call, the cache is no longer
11+
* consulted on reads and no longer populated by writes.
12+
* <p>
13+
* Implementations should release the cache contents so the memory can be
14+
* reclaimed. The call must be idempotent: subsequent invocations should be
15+
* safe and have no further effect.
16+
*/
17+
void disableCache();
18+
}

lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/PersistentDataStoreWrapper.java

Lines changed: 30 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@
4343
* <p>
4444
* This class is only constructed by {@link PersistentDataStoreBuilder}.
4545
*/
46-
final class PersistentDataStoreWrapper implements DataStore, SettableCache {
46+
final class PersistentDataStoreWrapper implements DataStore, SettableCache, DisableableCache {
4747
private final PersistentDataStore core;
4848
private final LoadingCache<CacheKey, Optional<ItemDescriptor>> itemCache;
4949
private final LoadingCache<DataKind, KeyedItems<ItemDescriptor>> allCache;
@@ -54,9 +54,15 @@ final class PersistentDataStoreWrapper implements DataStore, SettableCache {
5454
private final AtomicBoolean inited = new AtomicBoolean(false);
5555
private final ListeningExecutorService cacheExecutor;
5656
private final LDLogger logger;
57-
57+
5858
private final Object externalStoreLock = new Object();
5959
private volatile CacheExporter externalCache;
60+
61+
// Once true, the cache is bypassed on reads and writes; entries already in
62+
// the cache have been invalidated by disableCache(). The cache instances
63+
// themselves remain alive until GC reclaims them; the LoadingCache loaders
64+
// are short-circuited because every touch site checks this flag first.
65+
private volatile boolean cacheDisabled;
6066

6167
PersistentDataStoreWrapper(
6268
final PersistentDataStore core,
@@ -151,14 +157,26 @@ public void close() throws IOException {
151157
core.close();
152158
}
153159

160+
@Override
161+
public void disableCache() {
162+
if (cacheDisabled) return;
163+
// Volatile write publishes the bypass flag before clearing cache contents.
164+
// Future readers observe cacheDisabled == true and skip the cache call
165+
// sites.
166+
cacheDisabled = true;
167+
if (itemCache != null) itemCache.invalidateAll();
168+
if (allCache != null) allCache.invalidateAll();
169+
if (initCache != null) initCache.invalidateAll();
170+
}
171+
154172
@Override
155173
public boolean isInitialized() {
156174
if (inited.get()) {
157175
return true;
158176
}
159177
boolean result;
160178
try {
161-
if (initCache != null) {
179+
if (initCache != null && !cacheDisabled) {
162180
result = initCache.get("");
163181
} else {
164182
result = core.isInitialized();
@@ -187,7 +205,7 @@ public void init(FullDataSet<ItemDescriptor> allData) {
187205
allBuilder.add(new AbstractMap.SimpleEntry<>(kind, items));
188206
}
189207
RuntimeException failure = initCore(new FullDataSet<>(allBuilder.build(), allData.shouldPersist()));
190-
if (itemCache != null && allCache != null) {
208+
if (itemCache != null && allCache != null && !cacheDisabled) {
191209
itemCache.invalidateAll();
192210
allCache.invalidateAll();
193211
if (failure != null && !cacheIndefinitely) {
@@ -228,7 +246,7 @@ private RuntimeException initCore(FullDataSet<SerializedItemDescriptor> allData)
228246
@Override
229247
public ItemDescriptor get(DataKind kind, String key) {
230248
try {
231-
ItemDescriptor ret = itemCache != null ? itemCache.get(CacheKey.forItem(kind, key)).orNull() :
249+
ItemDescriptor ret = (itemCache != null && !cacheDisabled) ? itemCache.get(CacheKey.forItem(kind, key)).orNull() :
232250
getAndDeserializeItem(kind, key);
233251
processError(null);
234252
return ret;
@@ -242,7 +260,7 @@ public ItemDescriptor get(DataKind kind, String key) {
242260
public KeyedItems<ItemDescriptor> getAll(DataKind kind) {
243261
try {
244262
KeyedItems<ItemDescriptor> ret;
245-
ret = allCache != null ? allCache.get(kind) : getAllAndDeserialize(kind);
263+
ret = (allCache != null && !cacheDisabled) ? allCache.get(kind) : getAllAndDeserialize(kind);
246264
processError(null);
247265
return ret;
248266
} catch (Exception e) {
@@ -281,7 +299,7 @@ public boolean upsert(DataKind kind, String key, ItemDescriptor item) {
281299
}
282300
failure = e;
283301
}
284-
if (itemCache != null) {
302+
if (itemCache != null && !cacheDisabled) {
285303
CacheKey cacheKey = CacheKey.forItem(kind, key);
286304
if (failure == null) {
287305
if (updated) {
@@ -297,7 +315,7 @@ public boolean upsert(DataKind kind, String key, ItemDescriptor item) {
297315
}
298316
}
299317
}
300-
if (allCache != null) {
318+
if (allCache != null && !cacheDisabled) {
301319
// If the cache has a finite TTL, then we should remove the "all items" cache entry to force
302320
// a reread the next time All is called. However, if it's an infinite TTL, we need to just
303321
// update the item within the existing "all items" entry (since we want things to still work
@@ -340,7 +358,7 @@ public void setCacheExporter(CacheExporter externalDataSource) {
340358

341359
@Override
342360
public CacheStats getCacheStats() {
343-
if (itemCache == null || allCache == null) {
361+
if (itemCache == null || allCache == null || cacheDisabled) {
344362
return null;
345363
}
346364
com.google.common.cache.CacheStats itemStats = itemCache.stats();
@@ -443,8 +461,9 @@ private boolean pollAvailabilityAfterOutage() {
443461
}
444462

445463
// Fall back to cache-based recovery if external store is not available/initialized
446-
// and we're in infinite cache mode
447-
if (cacheIndefinitely && allCache != null) {
464+
// and we're in infinite cache mode. Under FDv2 this branch is dead once
465+
// disableCache has run: the externalCache path above supersedes it.
466+
if (cacheIndefinitely && allCache != null && !cacheDisabled) {
448467
// If we're in infinite cache mode, then we can assume the cache has a full set of current
449468
// flag data (since presumably the data source has still been running) and we can just
450469
// write the contents of the cache to the underlying data store.

lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/WriteThroughStore.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,9 @@ private void maybeSwitchStore() {
149149
}
150150
synchronized (activeStoreLock) {
151151
activeReadStore = memoryStore;
152+
if (persistentStore instanceof DisableableCache) {
153+
((DisableableCache) persistentStore).disableCache();
154+
}
152155
}
153156
}
154157

lib/sdk/server/src/main/java/com/launchdarkly/sdk/server/integrations/PersistentDataStoreBuilder.java

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,19 @@
3232
* </code></pre>
3333
*
3434
* In this example, {@code .url()} is an option specifically for the Redis integration, whereas
35-
* {@code cacheSeconds()} is an option that can be used for any persistent data store.
35+
* {@code cacheSeconds()} is an option that can be used for any persistent data store.
3636
* <p>
3737
* Note that this class is abstract; the actual implementation is created by calling
3838
* {@link Components#persistentDataStore(ComponentConfigurer)}.
39+
* <p>
40+
* Under the FDv2 data system, the cache options configured here ({@link #cacheTime(Duration)},
41+
* {@link #cacheSeconds(long)}, {@link #cacheMillis(long)}, {@link #cacheForever()},
42+
* {@link #noCaching()}, {@link #staleValuesPolicy(StaleValuesPolicy)},
43+
* {@link #recordCacheStats(boolean)}) only govern the brief bootstrap window before the in-memory
44+
* store has received its first full payload. Once the in-memory store takes over as the active
45+
* read source, the persistent-store cache is released and these settings have no further effect.
46+
* These options are kept for backward compatibility and may be deprecated in a future major
47+
* version.
3948
* @since 4.12.0
4049
*/
4150
public abstract class PersistentDataStoreBuilder implements ComponentConfigurer<DataStore> {

lib/sdk/server/src/test/java/com/launchdarkly/sdk/server/PersistentDataStoreWrapperTest.java

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import static org.hamcrest.Matchers.greaterThan;
3636
import static org.hamcrest.Matchers.is;
3737
import static org.hamcrest.Matchers.nullValue;
38+
import static org.junit.Assert.assertNotNull;
3839
import static org.junit.Assert.assertNull;
3940
import static org.junit.Assert.fail;
4041
import static org.junit.Assume.assumeThat;
@@ -713,6 +714,115 @@ public void statusRemainsUnavailableIfStoreSaysItIsAvailableButInitFails() throw
713714
assertThat(core.initedCount.get(), greaterThan(initedCount));
714715
}
715716

717+
@Test
718+
public void disableCacheIsIdempotent() {
719+
assumeThat(testMode.isCached(), is(true));
720+
wrapper.disableCache();
721+
wrapper.disableCache(); // must not throw
722+
}
723+
724+
@Test
725+
public void disableCacheIsSafeOnUncachedWrapper() {
726+
assumeThat(testMode.isCached(), is(false));
727+
wrapper.disableCache(); // must not throw
728+
}
729+
730+
@Test
731+
public void getAfterDisableCacheReturnsCurrentCoreState() {
732+
assumeThat(testMode.isCached(), is(true));
733+
TestItem item1v1 = new TestItem("key", 1);
734+
TestItem item1v2 = new TestItem("key", 2);
735+
736+
core.forceSet(TEST_ITEMS, item1v1);
737+
// Prime the cache.
738+
assertThat(wrapper.get(TEST_ITEMS, item1v1.key), equalTo(item1v1.toItemDescriptor()));
739+
740+
wrapper.disableCache();
741+
742+
// Mutate the core behind the wrapper's back; if the cache were still
743+
// serving reads we would see the stale v1.
744+
core.forceSet(TEST_ITEMS, item1v2);
745+
assertThat(wrapper.get(TEST_ITEMS, item1v2.key), equalTo(item1v2.toItemDescriptor()));
746+
}
747+
748+
@Test
749+
public void getAllAfterDisableCacheReturnsCurrentCoreState() {
750+
assumeThat(testMode.isCached(), is(true));
751+
TestItem item1 = new TestItem("keyA", 1);
752+
TestItem item2 = new TestItem("keyB", 1);
753+
754+
core.forceSet(TEST_ITEMS, item1);
755+
// Prime the cache.
756+
Map<String, ItemDescriptor> primed = toItemsMap(wrapper.getAll(TEST_ITEMS));
757+
assertThat(primed.size(), is(1));
758+
759+
wrapper.disableCache();
760+
761+
core.forceSet(TEST_ITEMS, item2);
762+
Map<String, ItemDescriptor> afterDrop = toItemsMap(wrapper.getAll(TEST_ITEMS));
763+
assertThat(afterDrop.size(), is(2));
764+
}
765+
766+
@Test
767+
public void upsertAfterDisableCacheWritesThroughToCoreOnly() {
768+
assumeThat(testMode.isCached(), is(true));
769+
TestItem item = new TestItem("key", 1);
770+
771+
wrapper.disableCache();
772+
773+
assertThat(wrapper.upsert(TEST_ITEMS, item.key, item.toItemDescriptor()), is(true));
774+
// The write must have landed in the core.
775+
assertThat(core.data.get(TEST_ITEMS).get(item.key), equalTo(item.toSerializedItemDescriptor()));
776+
// And subsequent reads must reach the core (no repopulated cache).
777+
assertThat(wrapper.get(TEST_ITEMS, item.key), equalTo(item.toItemDescriptor()));
778+
}
779+
780+
@Test
781+
public void initAfterDisableCacheWritesThroughToCoreWithoutRepopulatingCache() {
782+
assumeThat(testMode.isCached(), is(true));
783+
TestItem itemA = new TestItem("keyA", 1);
784+
TestItem itemB = new TestItem("keyA", 2);
785+
786+
wrapper.disableCache();
787+
788+
wrapper.init(new DataBuilder().add(TEST_ITEMS, itemA).build());
789+
790+
assertThat(core.data.get(TEST_ITEMS).get(itemA.key), equalTo(itemA.toSerializedItemDescriptor()));
791+
792+
// Mutate the core behind the wrapper's back; if the cache had repopulated
793+
// we would still see itemA on the next read.
794+
core.forceSet(TEST_ITEMS, itemB);
795+
assertThat(wrapper.get(TEST_ITEMS, itemB.key), equalTo(itemB.toItemDescriptor()));
796+
}
797+
798+
@Test
799+
public void getCacheStatsAfterDisableCacheReturnsNull() {
800+
assumeThat(testMode.isCached(), is(true));
801+
// Build a wrapper with stats recording enabled so getCacheStats is non-null pre-disable.
802+
PersistentDataStoreWrapper w = new PersistentDataStoreWrapper(
803+
new MockPersistentDataStore(),
804+
testMode.getCacheTtl(),
805+
PersistentDataStoreBuilder.StaleValuesPolicy.EVICT,
806+
true,
807+
this::updateStatus,
808+
sharedExecutor,
809+
testLogger);
810+
try {
811+
assertNotNull(w.getCacheStats());
812+
w.disableCache();
813+
assertNull(w.getCacheStats());
814+
} finally {
815+
try { w.close(); } catch (IOException e) { /* ignore */ }
816+
}
817+
}
818+
819+
@Test
820+
public void closeAfterDisableCacheDoesNotThrow() throws IOException {
821+
assumeThat(testMode.isCached(), is(true));
822+
wrapper.disableCache();
823+
wrapper.close(); // safety belt; tearDown will also call close, which must be safe
824+
}
825+
716826
private void causeStoreError(MockPersistentDataStore core, PersistentDataStoreWrapper w) {
717827
core.unavailable = true;
718828
core.fakeError = new RuntimeException(FAKE_ERROR.getMessage());

0 commit comments

Comments
 (0)