Fixed cache eviction for TLRU cache entries that were overwritten. Changed default ttl of TLRU assignment cache.

Author: maya-the-mango

src/cache/lru-cache.ts

@@ -97,22 +97,3 @@ export class LRUCache implements Map<string, string> {
     return this;
   }
 }
-
-/**
- * Time-aware, least-recently-used (TLRU), variant of LRU where entries have valid lifetime.
- * @param {number} maxSize - Maximum cache size
- * @param {number} ttl - Time in milliseconds after which cache entry will evict itself
- **/
-export class TLRUCache extends LRUCache {
-  constructor(readonly maxSize: number, readonly ttl: number) {
-    super(maxSize);
-  }
-
-  set(key: string, value: string): this {
-    const cache = super.set(key, value);
-    setTimeout(() => {
-      this.delete(key);
-    }, this.ttl);
-    return cache;
-  }
-}

src/cache/tlru-cache.spec.ts

@@ -1,18 +1,19 @@
-import { TLRUCache } from './lru-cache';
+import { TLRUCache } from './tlru-cache';
 
 describe('TLRU Cache', () => {
   let cache: TLRUCache;
-  const expectedCacheTimeoutMs = 50;
+  const expectedCacheTimeoutMs = 10;
 
   beforeEach(async () => {
     cache = new TLRUCache(2, expectedCacheTimeoutMs);
   });
 
-  afterAll(async () => {
+  afterEach(async () => {
     jest.restoreAllMocks();
+    jest.clearAllTimers();
   });
 
-  it('should evict cache after timeout', async () => {
+  it('should evict cache after timeout', () => {
     jest.useFakeTimers();
     jest.spyOn(global, 'setTimeout');
 
@@ -23,4 +24,38 @@ describe('TLRU Cache', () => {
     expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), expectedCacheTimeoutMs);
     expect(cache.get('a')).toBeUndefined();
   });
+
+  /**
+   * This test assumes implementation which is not ideal, but that's
+   * the only way I know of how to go around timers in jest
+   **/
+  it('should overwrite existing cache entry', () => {
+    jest.useFakeTimers();
+    jest.spyOn(global, 'setTimeout');
+    jest.spyOn(global, 'clearTimeout');
+
+    cache.set('a', 'apple');
+    jest.advanceTimersByTime(expectedCacheTimeoutMs - 1);
+    cache.set('a', 'avocado');
+
+    expect(setTimeout).toHaveBeenCalledTimes(2);
+    expect(setTimeout).toHaveBeenLastCalledWith(expect.any(Function), expectedCacheTimeoutMs);
+    expect(clearTimeout).toHaveBeenCalledTimes(1);
+    // spin the clock by 5sec. After that time cache entry should be still valid.
+    jest.advanceTimersByTime(expectedCacheTimeoutMs / 2);
+
+    // setting assertion in a weird way because calling cache.get()
+    // will reset eviction timer which will mess up next assertion
+    let avocadoInCache = false;
+    cache.forEach((value, key) => {
+      if (key === 'a' && value === 'avocado') {
+        avocadoInCache = true;
+      }
+    });
+    expect(avocadoInCache).toBe(true);
+
+    // after another spin of 5 sec, cache entry should evict itself
+    jest.advanceTimersByTime(expectedCacheTimeoutMs / 2);
+    expect(cache.has('a')).toBeFalsy();
+  });
 });

src/cache/tlru-cache.ts

@@ -6,15 +6,51 @@ import { LRUCache } from './lru-cache';
  * @param {number} ttl - Time in milliseconds after which cache entry will evict itself
  **/
 export class TLRUCache extends LRUCache {
+  private readonly cacheEntriesTimoutIds = new Map<string, ReturnType<typeof setTimeout>>();
   constructor(readonly maxSize: number, readonly ttl: number) {
     super(maxSize);
   }
 
-  set(key: string, value: string): this {
-    const cache = super.set(key, value);
-    setTimeout(() => {
+  private clearCacheEntryTimeout(key: string): void {
+    const timeoutId = this.cacheEntriesTimoutIds.get(key);
+    clearTimeout(timeoutId);
+    this.cacheEntriesTimoutIds.delete(key);
+  }
+
+  private setCacheEntryTimout(key: string): void {
+    const timeOutId = setTimeout(() => {
       this.delete(key);
     }, this.ttl);
+
+    this.cacheEntriesTimoutIds.set(key, timeOutId);
+  }
+
+  delete(key: string): boolean {
+    this.clearCacheEntryTimeout(key);
+    return super.delete(key);
+  }
+
+  get(key: string): string | undefined {
+    const value = super.get(key);
+
+    // Whenever we get a cache hit, we need to reset the timer
+    // for eviction, because it is now considered most recently
+    // accessed thus the timer should start over. Not doing that
+    // will cause a de-sync that will stop proper eviction
+    this.clearCacheEntryTimeout(key);
+    if (value) {
+      this.setCacheEntryTimout(key);
+    }
+    return value;
+  }
+
+  set(key: string, value: string): this {
+    const cache = super.set(key, value);
+    if (this.cacheEntriesTimoutIds.has(key)) {
+      this.clearCacheEntryTimeout(key);
+    }
+    this.setCacheEntryTimout(key);
+
     return cache;
   }
 }

src/cache/tlru-in-memory-assignment-cache.spec.ts

@@ -2,36 +2,40 @@ import { TLRUInMemoryAssignmentCache } from './tlru-in-memory-assignment-cache';
 
 describe('ExpiringLRUInMemoryAssignmentCache', () => {
   let cache: TLRUInMemoryAssignmentCache;
-  const defaultTimout = 60_000; // 10 minutes
+  const defaultTimout = 600_000; // 10 minutes
 
   beforeAll(() => {
     jest.useFakeTimers();
     cache = new TLRUInMemoryAssignmentCache(2);
   });
 
+  afterAll(() => {
+    jest.clearAllTimers();
+  });
+
   it(`assignment cache's timeout should default to 10 minutes `, () => {
-    const key1 = { subjectKey: 'a', flagKey: 'b', banditKey: 'c', actionKey: 'd' };
-    cache.set(key1);
+    const cacheEntry = { subjectKey: 'a', flagKey: 'b', banditKey: 'c', actionKey: 'd' };
+    cache.set(cacheEntry);
     jest.advanceTimersByTime(defaultTimout);
-    expect(cache.has(key1)).toBeFalsy();
+    expect(cache.has(cacheEntry)).toBeFalsy();
   });
 
   it(`assignment cache's timeout value is used on construction`, () => {
     const expectedTimout = 88;
     cache = new TLRUInMemoryAssignmentCache(2, expectedTimout);
-    const key1 = { subjectKey: 'a', flagKey: 'b', banditKey: 'c', actionKey: 'd' };
-    cache.set(key1);
+    const cacheEntry = { subjectKey: 'a', flagKey: 'b', banditKey: 'c', actionKey: 'd' };
+    cache.set(cacheEntry);
     jest.advanceTimersByTime(expectedTimout);
-    expect(cache.has(key1)).toBeFalsy();
+    expect(cache.has(cacheEntry)).toBeFalsy();
   });
 
   it(`cache shouldn't be invalidated before timeout`, () => {
-    const key1 = { subjectKey: 'a', flagKey: 'b', banditKey: 'c', actionKey: 'd' };
-    cache.set(key1);
+    const cacheEntry = { subjectKey: 'a', flagKey: 'b', banditKey: 'c', actionKey: 'd' };
+    cache.set(cacheEntry);
 
-    expect(cache.has(key1)).toBeTruthy();
+    expect(cache.has(cacheEntry)).toBeTruthy();
 
     jest.advanceTimersByTime(defaultTimout);
-    expect(cache.has(key1)).toBeFalsy();
+    expect(cache.has(cacheEntry)).toBeFalsy();
   });
 });

src/cache/tlru-in-memory-assignment-cache.ts

@@ -11,7 +11,7 @@ import { TLRUCache } from './tlru-cache';
  * @param {number} ttl - Time in milliseconds after cache will expire.
  */
 export class TLRUInMemoryAssignmentCache extends AbstractAssignmentCache<TLRUCache> {
-  constructor(maxSize: number, ttl = 60_000) {
+  constructor(maxSize: number, ttl = 600_000) {
     super(new TLRUCache(maxSize, ttl));
   }
 }

src/client/eppo-client.ts

@@ -253,6 +253,25 @@ export default class EppoClient {
     return this.getBooleanAssignment(flagKey, subjectKey, subjectAttributes, defaultValue);
   }
 
+  /**
+   * Maps a subject to a boolean variation for a given experiment.
+   *
+   * @param flagKey feature flag identifier
+   * @param subjectKey an identifier of the experiment subject, for example a user ID.
+   * @param subjectAttributes optional attributes associated with the subject, for example name and email.
+   * @param defaultValue default value to return if the subject is not part of the experiment sample
+   * @returns a boolean variation value if the subject is part of the experiment sample, otherwise the default value
+   */
+  public getBooleanAssignment(
+    flagKey: string,
+    subjectKey: string,
+    subjectAttributes: Attributes,
+    defaultValue: boolean,
+  ): boolean {
+    return this.getBooleanAssignmentDetails(flagKey, subjectKey, subjectAttributes, defaultValue)
+      .variation;
+  }
+
   /**
    * Maps a subject to a boolean variation for a given experiment and provides additional details about the
    * variation assigned and the reason for the assignment.
@@ -637,25 +656,6 @@ export default class EppoClient {
     return result;
   }
 
-  /**
-   * Maps a subject to a boolean variation for a given experiment.
-   *
-   * @param flagKey feature flag identifier
-   * @param subjectKey an identifier of the experiment subject, for example a user ID.
-   * @param subjectAttributes optional attributes associated with the subject, for example name and email.
-   * @param defaultValue default value to return if the subject is not part of the experiment sample
-   * @returns a boolean variation value if the subject is part of the experiment sample, otherwise the default value
-   */
-  public getBooleanAssignment(
-    flagKey: string,
-    subjectKey: string,
-    subjectAttributes: Attributes,
-    defaultValue: boolean,
-  ): boolean {
-    return this.getBooleanAssignmentDetails(flagKey, subjectKey, subjectAttributes, defaultValue)
-      .variation;
-  }
-
   private ensureActionsWithContextualAttributes(
     actions: BanditActions,
   ): Record<string, ContextAttributes> {