Optimizations

[kibertoad]

Summary

Memory leak fixes and performance optimizations across the codebase, identified through static analysis.

---

Fix 1: Remove event listeners on Redis consumer close()

Files: lib/redis/RedisNotificationConsumer.ts, lib/redis/RedisGroupNotificationConsumer.ts

Both RedisNotificationConsumer and RedisGroupNotificationConsumer attached an anonymous function to redis.on('message', ...) during subscribe(), but never removed it in close(). Since the handler holds a closure over this.targetCache (the in-memory cache), the listener prevented garbage collection of the entire cache tree after the consumer was closed.

Before:

subscribe(): Promise<void> {
  return this.redis.subscribe(this.channel).then(() => {
    this.redis.on('message', (channel, message) => { /* ... */ })
  })
}

async close(): Promise<void> {
  // listener never removed
  await this.redis.unsubscribe(this.channel)
}

After:

private messageHandler?: (channel: string, message: string) => void

subscribe(): Promise<void> {
  return this.redis.subscribe(this.channel).then(() => {
    this.messageHandler = (channel, message) => { /* ... */ }
    this.redis.on('message', this.messageHandler)
  })
}

async close(): Promise<void> {
  if (this.messageHandler) {
    this.redis.removeListener('message', this.messageHandler)
  }
  await this.redis.unsubscribe(this.channel)
}

---

Fix 2: Clean up empty Sets in GroupLoader.groupRefreshFlags

File: lib/GroupLoader.ts

When a background async refresh completed for a grouped key, the key was deleted from the group's Set in groupRefreshFlags, but the now-empty Set itself was never removed from the Map. Over time, every group that ever triggered a background refresh would leave behind an empty Set permanently in memory.

Before:

.finally(() => {
  groupSet!.delete(key)
})

After:

.finally(() => {
  groupSet!.delete(key)
  if (groupSet!.size === 0) {
    this.groupRefreshFlags.delete(group)
  }
})

This matches the cleanup pattern already used in AbstractGroupCache.deleteGroupRunningLoad.

---

Fix 3: Avoid creating empty group load Maps in getInMemoryOnly

File: lib/AbstractGroupCache.ts

getInMemoryOnly() called this.resolveGroupLoads(group) to check whether a key had a running load. resolveGroupLoads creates and inserts a new empty Map if none exists for the group — a side effect that's unnecessary when the intent is a read-only check. Every call to getInMemoryOnly for a group without active loads would allocate a Map that persists until the group's loads are cleaned up.

Before:

const groupLoads = this.resolveGroupLoads(group)  // creates empty Map as side effect
if (!groupLoads.has(key)) {

After:

if (!this.runningLoads.get(group)?.has(key)) {

Returns undefined (falsy) when no Map exists — semantically identical, without the allocation side effect. getAsyncOnly() still calls resolveGroupLoads() when it actually needs to store a load.

---

Fix 4: Optimize InMemoryGroupCache.getManyFromGroup

File: lib/memory/InMemoryGroupCache.ts

getManyFromGroup called this.getFromGroup(keys[i], group) inside a loop, which called this.resolveGroup(groupId) on every iteration — repeatedly looking up the same group cache from the LRU.

Before:

for (let i = 0; i < keys.length; i++) {
  const resolvedValue = this.getFromGroup(keys[i], group)  // resolveGroup() called N times

After:

const groupCache = this.resolveGroup(group)  // resolved once
for (let i = 0; i < keys.length; i++) {
  const resolvedValue = groupCache.get(keys[i])

Reduces N hash lookups to 1.

---

Fix 5: Optimize unique() to skip allocation when input has no duplicates

File: lib/util/unique.ts

The original unique() always created a new array via Array.from(new Set(arr)), even when the input had no duplicates (the common case in getMany calls).

Before:

export const unique = <T>(arr: T[]): T[] => Array.from(new Set(arr))

After:

export const unique = <T>(arr: T[]): T[] => {
  const set = new Set(arr)
  return set.size === arr.length ? arr : Array.from(set)
}

Returns the original array reference when all elements are already unique, avoiding an unnecessary array allocation on the hot path.

---

Fix 6: Remove redundant has() before delete() in Loader

File: lib/Loader.ts

forceSetValue and forceRefresh both guarded this.runningLoads.delete(key) with if (this.runningLoads.has(key)). Map.delete() is already a no-op for missing keys, so the guard was redundant. The lines were also marked with /* v8 ignore next -- @preserve */ confirming the branch was unreachable in tests.

Before:

/* v8 ignore next -- @preserve */
if (this.runningLoads.has(key)) {
  this.runningLoads.delete(key)
}

After:

this.runningLoads.delete(key)

---

Test plan

• Added test: RedisNotificationPublisher.spec.ts — verifies listenerCount('message') drops to 0 after close()
• Added test: RedisGroupNotificationPublisher.spec.ts — same for group consumer
• Added test: GroupLoader-async-refresh.spec.ts — verifies groupRefreshFlags Map has no empty Set after refresh completes
• Added test: GroupLoader-main.spec.ts — verifies getInMemoryOnly does not create empty runningLoads entry
• Added tests: unique.spec.ts — verifies same-reference return when no duplicates, new-array return when duplicates exist
• All 261 tests pass
• 100% line coverage maintained

Summary by CodeRabbit

• Bug Fixes

  • Improved cleanup of message listeners to prevent resource leaks when notification consumers close
  • Better removal of stale refresh tracking entries

• Performance

  • Optimized retrieval of grouped cached data by reducing repeated lookups

• Tests

  • Added test coverage for proper listener cleanup on shutdown
  • Added tests for deduplication edge cases

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR refactors memory management and listener cleanup across loader and cache layers. Changes include removing empty refresh tracking entries, simplifying load deletion logic, optimizing cache resolution, and properly removing Redis message listeners on close. New tests verify these cleanup behaviors.

Changes

Cohort / File(s)	Summary
In-Memory Refresh and Load State Management lib/AbstractGroupCache.ts, lib/GroupLoader.ts, lib/Loader.ts	Simplifies in-memory refresh checks by removing conditional guards on runningLoads deletion and adds cleanup to remove empty refresh tracking entries from groupRefreshFlags after refresh completes.
Cache Resolution Optimization lib/memory/InMemoryGroupCache.ts	Optimizes getManyFromGroup to resolve the group cache once at method start rather than per-iteration, reducing repeated resolution calls.
Redis Listener Lifecycle Management lib/redis/RedisGroupNotificationConsumer.ts, lib/redis/RedisNotificationConsumer.ts	Adds stored messageHandler field to both consumers and implements proper listener cleanup on close by removing the 'message' listener before unsubscribing, replacing inline anonymous listeners with stored handler references.
Utility Function Enhancement lib/util/unique.ts	Refactors unique function from one-liner to multi-line implementation that returns original array when no duplicates exist, otherwise returns deduplicated Array.from(set).
Utility Function Tests lib/util/unique.spec.ts	Adds two new tests verifying that unique returns the same array reference when there are no duplicates and returns a new array when duplicates exist.
Refresh Tracking Cleanup Test test/GroupLoader-async-refresh.spec.ts	Adds test case verifying that groupRefreshFlags entry is removed after background refresh completes.
In-Memory Load State Test test/GroupLoader-main.spec.ts	Adds test case verifying that getInMemoryOnly does not create or populate runningLoads entries for groups not requiring refresh.
Redis Listener Removal Tests test/redis/RedisGroupNotificationPublisher.spec.ts, test/redis/RedisNotificationPublisher.spec.ts	Each adds test case verifying that closing notification consumers removes Redis 'message' listeners.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• migrate to vitest 4 #376: Modifies the same Redis notification consumer classes (RedisGroupNotificationConsumer/RedisNotificationConsumer) with related close/subscribe logic changes for listener management.

Suggested labels

patch

Suggested reviewers

• andrewi-wd

Poem

🐰 Empty sets are cleared away, listeners removed when done,
Cache refreshes cleaned with care, no stale maps left to run,
Optimized loops, handlers stored—our memory runs lean,
The finest refactor the layered loader's ever seen! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Optimizations' is vague and overly broad, using a non-descriptive term that fails to convey the specific nature of the changes (memory-leak fixes, cache cleanup, listener removal, and performance optimizations across multiple components).	Consider using a more specific title such as 'Fix memory leaks and optimize cache operations' or 'Clean up Redis listeners and optimize group cache lookups' to better communicate the primary changes.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/perf

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

lib/memory/InMemoryGroupCache.ts (1)

91-95: Pre-existing: null cached values are treated as unresolved.

if (resolvedValue) is falsy for both null and undefined. Since null represents "resolved to empty" elsewhere in the codebase, a null-cached entry will be reported as unresolved here, potentially triggering a redundant async fetch. This isn't introduced by this PR but worth noting if null group entries are expected.

lib/redis/RedisGroupNotificationConsumer.ts (1)

41-66: Minor: repeated subscribe() calls would leak the previous listener.

If subscribe() is ever called more than once, the prior messageHandler is overwritten without being removed first. Consider removing the existing listener (if any) at the top of subscribe().

Defensive cleanup suggestion

   subscribe(): Promise<void> {
     return this.redis.subscribe(this.channel).then(() => {
+      if (this.messageHandler) {
+        this.redis.removeListener('message', this.messageHandler)
+      }
       this.messageHandler = (channel, message) => {

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}