Skip to content

Commit 76c3c23

Browse files
authored
feat: Drop persistent-store cache after FDv2 in-memory store init (#274)
## 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 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 `IDisableableCache` capability interface and a `DisableCache()` method on `PersistentStoreWrapper`. The method atomically swaps each cache field to null via `Interlocked.Exchange`, then clears and disposes the captured instances. `WriteThroughStore.MaybeSwitchStore()` probes for the interface and invokes the method inside the existing `_activeStoreLock`, immediately after the active read store is flipped to the in-memory store. Existing null-guards at every cache touch site already short-circuit when a cache field is null (the "caching disabled" config path nulls them at construction), so no read/write call sites needed to be rewritten. The drop is mode-independent: reads bypass the persistent store in both `ReadOnly` and `ReadWrite` modes after the flip, so the cache is unused in either case. `Dispose(bool)` is refactored to call `DisableCache()` in place of disposing the three caches individually, making the cache lifecycle idempotent. The `PersistentDataStoreBuilder` cache-config setters (`NoCaching`, `CacheTime`, `CacheMillis`, `CacheSeconds`, `CacheMaximumEntries`, `CacheForever`) remain functional during the bootstrap window for backward compatibility. XML documentation on the class explains that under FDv2 these only govern the window before the in-memory store has received its first payload. Mirrors the same change Matthew Keeler shipped for Python (launchdarkly/python-server-sdk#426), Ruby (launchdarkly/ruby-server-sdk#384), and Go (launchdarkly/go-server-sdk#373). Naming differs from the ticket text: the method is `DisableCache` rather than `DiscardCache` so the verb conveys that the cache is off from this point forward (matching Python and Ruby's `disable_cache`); the interface is `IDisableableCache` to mirror the existing `ISettableCache` precedent in the same `Subsystems/` namespace. Tests follow the indirect-observation pattern used in the reference SDKs: prime the cache, drop, mutate the core directly, assert the next read sees the new value. A new spy `MockDisableableCachePersistentStore` (with a `DisableCacheCallCount` counter) drives the `WriteThroughStore` probe-and-invoke coverage, including the FDv1 `Init` path, the FDv2 `Apply` path, the delta-does-not-redrop case, the `ReadOnly`-mode-still-drops case, and the non-disabler-store no-op case. No `Thread.Sleep`. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches core data-store read/write paths and the FDv2 store handoff; behavior change is scoped to post-init cache bypass with broad test coverage, but incorrect timing could cause stale reads if the switch ran too early. > > **Overview** > Under **FDv2**, once the in-memory store has taken the first full payload, the persistent wrapper’s item/all/init caches are redundant and can roughly **double flag memory** (worse with `CacheForever()`). This PR adds internal **`IDisableableCache`** / **`DisableCache()`** on **`PersistentStoreWrapper`**, which sets a volatile bypass flag, clears caches, and routes **`Get`**, **`GetAll`**, **`Init`**, **`Upsert`**, and **`Initialized`** straight to the core when disabled. > > **`WriteThroughStore.MaybeSwitchStore()`** invokes **`DisableCache()`** once (inside **`_activeStoreLock`**) when reads flip to the memory store—on first **`Init`** or **`Apply`**, including **ReadOnly** mode; stores without the interface are unchanged. > > **`PersistentDataStoreBuilder`** docs now state cache options only matter during the **bootstrap** window before that handoff; options remain for backward compatibility. > > Tests cover idempotent disable, read-through after disable, and **`WriteThroughStore`** calling disable exactly once. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 81f1b82. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 3c38378 commit 76c3c23

6 files changed

Lines changed: 294 additions & 9 deletions

File tree

pkgs/sdk/server/src/Integrations/PersistentDataStoreBuilder.cs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,17 @@ namespace LaunchDarkly.Sdk.Server.Integrations
2525
/// .DataStore(myStore)
2626
/// .Build();
2727
/// </code>
28+
/// <para>
29+
/// Note: under the FDv2 data system, the cache settings configured here
30+
/// (<see cref="CacheTime(TimeSpan)"/>, <see cref="CacheSeconds(int)"/>,
31+
/// <see cref="CacheMillis(int)"/>, <see cref="CacheMaximumEntries(int?)"/>,
32+
/// <see cref="CacheForever"/>, <see cref="NoCaching"/>) only govern the
33+
/// brief bootstrap window before the in-memory store has received its
34+
/// first full payload. Once the in-memory store takes over as the active
35+
/// read source, the persistent-store cache is released and these settings
36+
/// have no further effect. These options are kept for backward compatibility
37+
/// and may be deprecated in a future major version.
38+
/// </para>
2839
/// </remarks>
2940
public class PersistentDataStoreBuilder : IComponentConfigurer<IDataStore>, IDiagnosticDescription
3041
{

pkgs/sdk/server/src/Internal/DataStores/PersistentStoreWrapper.cs

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ namespace LaunchDarkly.Sdk.Server.Internal.DataStores
2222
/// class adds the caching behavior that we normally want for any persistent data store.
2323
/// </para>
2424
/// </remarks>
25-
internal sealed class PersistentStoreWrapper : IDataStore, ISettableCache
25+
internal sealed class PersistentStoreWrapper : IDataStore, ISettableCache, IDisableableCache
2626
{
2727
private readonly IPersistentDataStore _core;
2828
private readonly DataStoreCacheConfig _caching;
@@ -39,6 +39,13 @@ internal sealed class PersistentStoreWrapper : IDataStore, ISettableCache
3939
private ICacheExporter _externalCache;
4040

4141
private volatile bool _inited;
42+
43+
// Once true, the cache is bypassed on reads and writes; entries already in
44+
// the cache have been Clear()-ed by DisableCache(). The cache objects
45+
// themselves remain alive until Dispose() so that any reader currently
46+
// holding a reference does not observe a disposed cache. Volatile so the
47+
// bypass becomes visible to other threads without an explicit lock.
48+
private volatile bool _cacheDisabled;
4249

4350
internal PersistentStoreWrapper(
4451
IPersistentDataStoreAsync coreAsync,
@@ -112,7 +119,7 @@ public bool Initialized()
112119
return true;
113120
}
114121
bool result;
115-
if (_initCache != null)
122+
if (_initCache != null && !_cacheDisabled)
116123
{
117124
result = _initCache.Get();
118125
}
@@ -163,7 +170,7 @@ public void Init(FullDataSet<ItemDescriptor> items)
163170
kindAndItems => PersistentDataStoreConverter.SerializeAll(kindAndItems.Key, kindAndItems.Value.Items)
164171
);
165172
Exception failure = InitCore(new FullDataSet<SerializedItemDescriptor>(serializedItems));
166-
if (_itemCache != null && _allCache != null)
173+
if (_itemCache != null && _allCache != null && !_cacheDisabled)
167174
{
168175
_itemCache.Clear();
169176
_allCache.Clear();
@@ -199,7 +206,7 @@ public void Init(FullDataSet<ItemDescriptor> items)
199206
{
200207
try
201208
{
202-
var ret = _itemCache is null ? GetAndDeserializeItem(kind, key) :
209+
var ret = (_itemCache is null || _cacheDisabled) ? GetAndDeserializeItem(kind, key) :
203210
_itemCache.Get(new CacheKey(kind, key));
204211
ProcessError(null);
205212
return ret;
@@ -215,7 +222,7 @@ public KeyedItems<ItemDescriptor> GetAll(DataKind kind)
215222
{
216223
try
217224
{
218-
var ret = new KeyedItems<ItemDescriptor>(_allCache is null ?
225+
var ret = new KeyedItems<ItemDescriptor>((_allCache is null || _cacheDisabled) ?
219226
GetAllAndDeserialize(kind) : _allCache.Get(kind));
220227
ProcessError(null);
221228
return ret;
@@ -250,7 +257,7 @@ public bool Upsert(DataKind kind, string key, ItemDescriptor item)
250257
}
251258
failure = e;
252259
}
253-
if (_itemCache != null)
260+
if (_itemCache != null && !_cacheDisabled)
254261
{
255262
var cacheKey = new CacheKey(kind, key);
256263
if (failure is null)
@@ -284,7 +291,7 @@ public bool Upsert(DataKind kind, string key, ItemDescriptor item)
284291
}
285292
}
286293
}
287-
if (_allCache != null)
294+
if (_allCache != null && !_cacheDisabled)
288295
{
289296
// If the cache has a finite TTL, then we should remove the "all items" cache entry to force
290297
// a reread the next time All is called. However, if it's an infinite TTL, we need to just
@@ -315,6 +322,24 @@ public bool Upsert(DataKind kind, string key, ItemDescriptor item)
315322
return updated;
316323
}
317324

325+
/// <summary>
326+
/// Disables the internal cache. After this call, reads and writes go straight
327+
/// to the underlying persistent store; subsequent invocations are no-ops.
328+
/// </summary>
329+
/// <remarks>
330+
/// At the time of writing, it is likely that the cache was disabled when
331+
/// another store took over as the active store and so reads may not even get
332+
/// to this layer anymore. If they do, they do go straight to persistence if
333+
/// this layer's DisableCache method was called.
334+
/// </remarks>
335+
public void DisableCache()
336+
{
337+
if (_cacheDisabled) return;
338+
_cacheDisabled = true;
339+
_itemCache?.Clear();
340+
_allCache?.Clear();
341+
}
342+
318343
public void Dispose()
319344
{
320345
Dispose(true);
@@ -437,8 +462,9 @@ private bool PollAvailabilityAfterOutage()
437462
}
438463

439464
// Fall back to cache-based recovery if external store is not available/initialized
440-
// and we're in infinite cache mode
441-
if (_cacheIndefinitely && _allCache != null)
465+
// and we're in infinite cache mode. Under FDv2 this branch is dead once
466+
// DisableCache has run: the ICacheExporter path above supersedes it.
467+
if (_cacheIndefinitely && _allCache != null && !_cacheDisabled)
442468
{
443469
// If we're in infinite cache mode, then we can assume the cache has a full set of current
444470
// flag data (since presumably the data source has still been running) and we can just

pkgs/sdk/server/src/Internal/DataSystem/WriteThroughStore.cs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,10 @@ private void MaybeSwitchStore()
127127
lock (_activeStoreLock)
128128
{
129129
_activeReadStore = _memoryStore;
130+
if (_persistentStore is IDisableableCache disableable)
131+
{
132+
disableable.DisableCache();
133+
}
130134
}
131135
}
132136

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
namespace LaunchDarkly.Sdk.Server.Subsystems
2+
{
3+
/// <summary>
4+
/// Optional interface for data stores that can disable their internal cache.
5+
/// </summary>
6+
/// <remarks>
7+
/// This is currently for internal implementations only.
8+
/// </remarks>
9+
internal interface IDisableableCache
10+
{
11+
/// <summary>
12+
/// Disables the internal cache. After this call, the cache is no longer
13+
/// consulted on reads and no longer populated by writes.
14+
/// </summary>
15+
/// <remarks>
16+
/// Implementations should clear, dispose, and dereference cache instances
17+
/// so the memory can be reclaimed. The call must be idempotent: subsequent
18+
/// invocations should be safe and have no further effect.
19+
/// </remarks>
20+
void DisableCache();
21+
}
22+
}

pkgs/sdk/server/test/Internal/DataStores/PersistentStoreWrapperTestBase.cs

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -640,6 +640,109 @@ public void StatusRemainsUnavailableIfStoreSaysItIsAvailableButInitFails()
640640
}
641641
}
642642

643+
[Fact]
644+
public void DisableCacheIsIdempotent()
645+
{
646+
var wrapper = MakeWrapper(Cached);
647+
wrapper.DisableCache();
648+
wrapper.DisableCache(); // second call must not throw
649+
}
650+
651+
[Fact]
652+
public void DisableCacheIsSafeOnUncachedWrapper()
653+
{
654+
var wrapper = MakeWrapper(Uncached);
655+
wrapper.DisableCache(); // no caches to release, must not throw
656+
}
657+
658+
[Fact]
659+
public void GetAfterDisableCacheReturnsCurrentCoreState()
660+
{
661+
var wrapper = MakeWrapper(Cached);
662+
var key = "flag";
663+
var itemv1 = new TestItem("itemv1");
664+
var itemv2 = new TestItem("itemv2");
665+
666+
_core.ForceSet(TestDataKind, key, 1, itemv1);
667+
// Prime the cache.
668+
Assert.Equal(itemv1.WithVersion(1), wrapper.Get(TestDataKind, key));
669+
670+
wrapper.DisableCache();
671+
672+
// Mutate the core behind the wrapper's back. If the cache is still
673+
// serving reads we would see the stale itemv1.
674+
_core.ForceSet(TestDataKind, key, 2, itemv2);
675+
Assert.Equal(itemv2.WithVersion(2), wrapper.Get(TestDataKind, key));
676+
}
677+
678+
[Fact]
679+
public void GetAllAfterDisableCacheReturnsCurrentCoreState()
680+
{
681+
var wrapper = MakeWrapper(Cached);
682+
var itemA = new TestItem("itemA");
683+
var itemB = new TestItem("itemB");
684+
685+
_core.ForceSet(TestDataKind, "keyA", 1, itemA);
686+
// Prime the cache.
687+
var primed = wrapper.GetAll(TestDataKind);
688+
Assert.Single(primed.Items);
689+
690+
wrapper.DisableCache();
691+
692+
_core.ForceSet(TestDataKind, "keyB", 1, itemB);
693+
var afterDrop = wrapper.GetAll(TestDataKind);
694+
Assert.Equal(2, afterDrop.Items.Count());
695+
}
696+
697+
[Fact]
698+
public void UpsertAfterDisableCacheWritesThroughToCoreOnly()
699+
{
700+
var wrapper = MakeWrapper(Cached);
701+
var key = "flag";
702+
var itemv1 = new TestItem("itemv1");
703+
704+
wrapper.DisableCache();
705+
706+
Assert.True(wrapper.Upsert(TestDataKind, key, itemv1.WithVersion(1)));
707+
// The write must have landed in the core, and a subsequent Get
708+
// must reach the core (not a repopulated cache).
709+
Assert.True(_core.Data[TestDataKind].ContainsKey(key));
710+
Assert.Equal(itemv1.WithVersion(1), wrapper.Get(TestDataKind, key));
711+
}
712+
713+
[Fact]
714+
public void InitAfterDisableCacheWritesThroughToCoreWithoutRepopulatingCache()
715+
{
716+
var wrapper = MakeWrapper(Cached);
717+
var itemA = new TestItem("itemA");
718+
var itemB = new TestItem("itemB");
719+
720+
wrapper.DisableCache();
721+
722+
var allData = new TestDataBuilder()
723+
.Add(TestDataKind, "keyA", 1, itemA)
724+
.Build();
725+
wrapper.Init(allData);
726+
727+
// The init must have written to the core.
728+
Assert.True(_core.Data[TestDataKind].ContainsKey("keyA"));
729+
730+
// If the cache had repopulated, the next ForceRemove behind the
731+
// wrapper's back would still yield itemA on Get. With cache
732+
// disabled, the new core state is what we observe.
733+
_core.ForceRemove(TestDataKind, "keyA");
734+
_core.ForceSet(TestDataKind, "keyA", 2, itemB);
735+
Assert.Equal(itemB.WithVersion(2), wrapper.Get(TestDataKind, "keyA"));
736+
}
737+
738+
[Fact]
739+
public void DisposeIsSafeAfterDisableCache()
740+
{
741+
var wrapper = MakeWrapper(Cached);
742+
wrapper.DisableCache();
743+
wrapper.Dispose(); // must not throw, even though caches are already gone
744+
}
745+
643746
private PersistentStoreWrapper MakeWrapper(CacheMode cacheMode) =>
644747
MakeWrapper(new TestParams { CacheMode = cacheMode, PersistMode = PersistWithMetadata });
645748

0 commit comments

Comments
 (0)