Skip to content

fix: clean up cache by removing stale keys #252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
izadoesdev merged 2 commits into from
Jan 2, 2026
Merged

fix: clean up cache by removing stale keys #252

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce4a905
refactor: Extract flags cache key generation.
sbansal1999 Jan 2, 2026
d1f140e
feat: Remove stale cache keys for flags.
sbansal1999 Jan 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions packages/sdk/src/core/flags/flags-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,14 @@ export class CoreFlagsManager implements FlagsManager {
};
}

private removeStaleKeys(validCacheKeys: Set<string>): void {
for (const key of this.cache.keys()) {
if (!validCacheKeys.has(key)) {
this.cache.delete(key);
}
}
}

Comment on lines +109 to +116
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Multi-user cache corruption.

The method deletes all cache keys not present in validCacheKeys, but when called from fetchAllFlags (line 315), validCacheKeys only contains keys for the current user. If the cache holds entries for multiple users, fetching flags for User A will incorrectly delete all cached flags for User B.

Scenario:

  1. User A fetches → cache: "flag1:userA", "flag2:userA"
  2. User B fetches → cache: "flag1:userA", "flag2:userA", "flag1:userB", "flag2:userB"
  3. User A re-fetches → validCacheKeys = {"flag1:userA", "flag2:userA"}
  4. All User B entries are deleted because they're not in validCacheKeys
🔎 Proposed fix to scope cleanup to current user 
-private removeStaleKeys(validCacheKeys: Set<string>): void {
-  for (const key of this.cache.keys()) {
-    if (!validCacheKeys.has(key)) {
-      this.cache.delete(key);
+private removeStaleKeys(validCacheKeys: Set<string>, currentUser?: UserContext): void {
+  const userSuffix = getCacheKey("", currentUser ?? this.config.user).slice(1); // Get ":userContext" part
+  for (const key of this.cache.keys()) {
+    // Only consider keys belonging to the current user
+    if (key.endsWith(userSuffix) && !validCacheKeys.has(key)) {
+      this.cache.delete(key);
    }
  }
}

Then update the call site at line 315:

 this.removeStaleKeys(
-  new Set(flagCacheEntries.map(({ cacheKey }) => cacheKey))
+  new Set(flagCacheEntries.map(({ cacheKey }) => cacheKey)),
+  user
 );

Committable suggestion skipped: line range outside the PR's diff.

✅ Addressed in commits ce4a905 to d1f140e

private async initialize(): Promise<void> {
// Load from persistent storage first (instant hydration)
if (!this.config.skipStorage && this.storage) {
Expand Down Expand Up @@ -294,16 +302,22 @@ export class CoreFlagsManager implements FlagsManager {
const apiUrl = this.config.apiUrl ?? "https://api.databuddy.cc";
const params = buildQueryParams(this.config, user);

const ttl = this.config.cacheTtl ?? 60_000;
const staleTime = this.config.staleTime ?? ttl / 2;

try {
const flags = await fetchAllFlagsApi(apiUrl, params);
const flagCacheEntries = Object.entries(flags).map(([key, result]) => ({
cacheKey: getCacheKey(key, user ?? this.config.user),
cacheEntry: createCacheEntry(result, ttl, staleTime),
}));

// Update cache with all flags
const ttl = this.config.cacheTtl ?? 60_000;
const staleTime = this.config.staleTime ?? ttl / 2;
this.removeStaleKeys(
new Set(flagCacheEntries.map(({ cacheKey }) => cacheKey))
);

for (const [key, result] of Object.entries(flags)) {
const cacheKey = getCacheKey(key, user ?? this.config.user);
this.cache.set(cacheKey, createCacheEntry(result, ttl, staleTime));
for (const { cacheKey, cacheEntry } of flagCacheEntries) {
this.cache.set(cacheKey, cacheEntry);
}
Comment on lines +305 to 321
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Partially fixes the original issue but introduces multi-user bug.

What's fixed:

  • Lines 310-313 correctly construct cache keys using getCacheKey(key, user ?? this.config.user), which fixes the cache key format mismatch from the previous review.
  • The batch preparation approach (lines 310-313, 319-321) is cleaner and more efficient.

New issue:

  • Line 315-317 calls removeStaleKeys with only the current user's cache keys, but the method deletes entries for ALL users not in that set. See the critical issue flagged in the removeStaleKeys method above.


this.ready = true;
Expand Down
Loading