fix(session-storage): bound getSessionMessages cache to prevent unbounded growth

[Ericcc-Ma]

Summary

getSessionMessages was implemented via lodash memoize, which has no size cap. Each unique sessionId adds a Promise<Set<UUID>>
entry that is only cleared by clearSessionMessagesCache() — and that call is only made from postCompactCleanup, which fires once
per compaction.

In long-running daemon / swarm sessions that spawn many agents (each with its own transcript), the cache grows unbounded for the
process lifetime. Per-entry Set<UUID> values are MUCH larger than the file-path strings the adjacent existingSessionFiles cache
stores, so the leak is more severe than the one fixed for that cache at sessionStorage.ts:535.

Root Cause

// src/utils/sessionStorage.ts (before)
const getSessionMessages = memoize(
  async (sessionId: UUID): Promise<Set<UUID>> => {
    const { messages } = await loadSessionFile(sessionId)
    return new Set(messages.keys())
  },
  (sessionId: UUID) => sessionId,
)

lodash memoize exposes no size cap. The cache grows for the lifetime of the process unless clearSessionMessagesCache() is called,
and that only happens once per compaction (postCompactCleanup).

The adjacent existingSessionFiles cache at sessionStorage.ts:1314-1337 already solves this for file paths:

if (this.existingSessionFiles.size >= MAX_CACHED_SESSION_FILES) {
  const oldestKey = this.existingSessionFiles.keys().next().value
  if (oldestKey !== undefined) {
    this.existingSessionFiles.delete(oldestKey)
  }
}

…but getSessionMessages was missed when that fix landed. Likely because lodash memoize looked "obviously correct" and the
per-entry size wasn't obvious until swarm sessions exercised it.

Fix

Mirror the existingSessionFiles pattern: replace lodash memoize with a Map<UUID, Promise<Set<UUID>>> and evict the oldest entry
(FIFO via Map insertion order) when at MAX_CACHED_SESSION_FILES capacity. Re-uses the same constant for consistency.

--- a/src/utils/sessionStorage.ts
+++ b/src/utils/sessionStorage.ts
-const getSessionMessages = memoize(
-  async (sessionId: UUID): Promise<Set<UUID>> => {
-    const { messages } = await loadSessionFile(sessionId)
-    return new Set(messages.keys())
-  },
-  (sessionId: UUID) => sessionId,
-)
-
-export function clearSessionMessagesCache(): void {
-  getSessionMessages.cache.clear?.()
-}
+const sessionMessagesCache = new Map<UUID, Promise<Set<UUID>>>()
+
+export async function getSessionMessages(
+  sessionId: UUID,
+): Promise<Set<UUID>> {
+  const existing = sessionMessagesCache.get(sessionId)
+  if (existing !== undefined) {
+    return existing
+  }
+  // Evict oldest entry when at capacity so the Map stays bounded.
+  if (sessionMessagesCache.size >= MAX_CACHED_SESSION_FILES) {
+    const oldestKey = sessionMessagesCache.keys().next().value
+    if (oldestKey !== undefined) {
+      sessionMessagesCache.delete(oldestKey)
+    }
+  }
+  const promise = (async () => {
+    const { messages } = await loadSessionFile(sessionId)
+    return new Set(messages.keys())
+  })()
+  sessionMessagesCache.set(sessionId, promise)
+  return promise
+}
+
+/** Underlying cache for direct manipulation (priming, clearing, tests). */
+export function getSessionMessagesCache(): Map<UUID, Promise<Set<UUID>>> {
+  return sessionMessagesCache
+}
+
+export function clearSessionMessagesCache(): void {
+  sessionMessagesCache.clear()
+}

The getLastSessionLog priming code at sessionStorage.ts:4027 is updated to use getSessionMessagesCache().has/set directly
instead of getSessionMessages.cache.has/set (which was the lodash-internal .cache property).

getSessionMessages is now exported with a comment noting it's not part of the public API (callers should use
doesMessageExistInSession), but is exported so tests can verify the eviction behavior end-to-end.

Test Coverage

7 new tests in src/utils/__tests__/sessionStorage.test.ts:

Cache mechanics (4 tests):

• getSessionMessagesCache() returns the same Map instance across calls (identity)
• clearSessionMessagesCache() empties a populated cache
• clearSessionMessagesCache() is a no-op on empty cache
• getSessionMessages() dedups concurrent calls for the same sessionId (single in-flight promise)

Bounded cache / memory leak fix (3 tests):

• Cache size stays at MAX_CACHED_ENTRIES (200) after 600 distinct sessionIds
• FIFO eviction: oldest entry is removed first when at capacity
• Cleared cache can be refilled without leaking entries

Tests use real temp directory + CLAUDE_CONFIG_DIR env (no mock.module pollution), reading real .jsonl files for end-to-end
coverage of the eviction logic.

Verification

• bun run precheck on Windows 11: 5899/5900 pass (the 1 fail is a pre-existing flaky toolEventObserver > pruneEmittedTurns > prunes Set entries exceeding SET_MAX keeping most recent test that consistently times out at 5s on Windows runners, unrelated to this
  change)
• 7/7 new tests pass
• No regressions in other test files

Environment Tested

• OS: Windows 11 (10.0.26200)
• Bun 1.3.14

Summary by CodeRabbit

• Bug Fixes

  • Improved Windows path handling so MinGW/Git-Bash-style paths are normalized correctly before permission checks.
  • Closed a sandbox escape edge case where some Windows paths could be validated against the wrong location.
  • Fixed session message caching to keep concurrent requests deduplicated while preventing unbounded cache growth.

• Tests

  • Added coverage for session cache behavior, cache eviction, and concurrent session reads.
  • Added path-validation coverage for Windows path normalization and sandbox-boundary checks.

[coderabbitai]

📝 Walkthrough

Walkthrough

Added session-message cache tests and Windows path-validation tests, while refactoring sessionStorage to use a bounded cache and updating validatePath to normalize MinGW-style Windows paths before resolution.

Changes

Session message cache

Layer / File(s)	Summary
Bounded cache implementation src/utils/sessionStorage.ts	getSessionMessages now uses a bounded Map of in-flight promises, getSessionMessagesCache() exposes that map, clearSessionMessagesCache() clears it, and getLastSessionLog primes the cache through the new accessor.
Cache identity and dedup tests src/utils/__tests__/sessionStorage.test.ts	The test harness pins a temporary config directory and checks cache identity stability, cache clearing, and concurrent getSessionMessages deduplication.
Bounded eviction tests src/utils/__tests__/sessionStorage.test.ts	The test suite verifies the cache stays within MAX_CACHED_ENTRIES, evicts the oldest entry first, and remains bounded after clearing and refilling.

Windows path validation

Layer / File(s)	Summary
MinGW path normalization src/utils/permissions/pathValidation.ts	validatePath applies posixPathToWindowsPath before absolute-path checks and resolution, and the file now documents permission prompt directory-list formatting.
Normalization and escape tests src/utils/permissions/__tests__/pathValidation.test.ts	The test harness mocks runtime dependencies, gates execution to Windows, and asserts MinGW normalization, drive-letter casing, relative-path handling, and sandbox-escape denial.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

A bunny hopped through cache and path,
With tidy maps and Windows math.
/c became C:\ today,
And sneaky escapes hopped far away.
🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: bounding the session message cache to prevent unbounded growth.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

src/utils/permissions/__tests__/pathValidation.test.ts (2)

5-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Move module mocks into the shared beforeAll mock pattern.

These are bottom-layer mocks, but this file registers them at module scope and inlines the bun:bundle mock. Please align with the shared mock setup to avoid cross-file mock pollution. As per coding guidelines, **/__tests__/**/*.test.{ts,tsx} should “Mock only bottom-layer dependencies with side effects” and “Use use-shared-mock pattern: import mock from tests/mocks/ … and pass to mock.module() in beforeAll block”.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/permissions/__tests__/pathValidation.test.ts` around lines 5 - 10,
Move the bottom-layer mocks for src/utils/log.ts, src/utils/debug.ts, and
bun:bundle out of module scope and into the shared beforeAll mock setup used by
pathValidation.test.ts. Update the test to use the existing shared mock pattern
by importing the mock from tests/mocks and registering it with mock.module()
inside beforeAll, so the test avoids cross-file mock pollution while keeping the
same behavior.

Source: Coding guidelines

---

173-184: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Seed an allow-list that would have exposed the old escape.

With makeContext() empty, result.allowed is false even before this fix. To prove the sandbox-escape regression, configure the context so the old wrong resolution under D:\... would be allowed while the normalized C:\Users\foo... path is denied.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/permissions/__tests__/pathValidation.test.ts` around lines 173 -
184, The test in validatePath should not use an empty makeContext(), because
that already denies the path before the regression is exercised. Update the
pathValidation.test.ts case to seed the context allow-list so the old
Windows/MinGW mis-resolution under the D:\... base would have passed, while the
corrected normalized path to C:\Users\foo\sensitive.txt is denied; keep the
assertion focused on validatePath and the allowed/denied outcome for the escape
scenario.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/utils/permissions/pathValidation.ts`:
- Around line 482-502: The Windows path normalization in pathValidation should
be applied before both the glob and non-glob validation flows, not only before
isAbsolute/resolve. Update the path handling around validateGlobPattern and the
later resolve logic so MinGW-style absolute paths are normalized to Windows
paths first, ensuring inputs like /c/Users/foo/*.txt are validated against the
same filesystem location Git Bash uses.

In `@src/utils/sessionStorage.ts`:
- Around line 4030-4032: The session cache priming in
getLastSessionLog()/getSessionMessagesCache() needs the original “cache is
empty” guard restored, not just a per-session has(sessionId) check. Update the
priming logic so it only seeds from disk when the entire messages cache is
empty, preventing stale snapshots from being written into a session that already
has cached entries and avoiding UUID dedup misses for unflushed messages.

---

Nitpick comments:
In `@src/utils/permissions/__tests__/pathValidation.test.ts`:
- Around line 5-10: Move the bottom-layer mocks for src/utils/log.ts,
src/utils/debug.ts, and bun:bundle out of module scope and into the shared
beforeAll mock setup used by pathValidation.test.ts. Update the test to use the
existing shared mock pattern by importing the mock from tests/mocks and
registering it with mock.module() inside beforeAll, so the test avoids
cross-file mock pollution while keeping the same behavior.
- Around line 173-184: The test in validatePath should not use an empty
makeContext(), because that already denies the path before the regression is
exercised. Update the pathValidation.test.ts case to seed the context allow-list
so the old Windows/MinGW mis-resolution under the D:\... base would have passed,
while the corrected normalized path to C:\Users\foo\sensitive.txt is denied;
keep the assertion focused on validatePath and the allowed/denied outcome for
the escape scenario.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5ae03700-a312-467b-a868-1cb7b7487cf5

📥 Commits

Reviewing files that changed from the base of the PR and between 8246ffa and 562da4c.

📒 Files selected for processing (4)

• src/utils/__tests__/sessionStorage.test.ts
• src/utils/permissions/__tests__/pathValidation.test.ts
• src/utils/permissions/pathValidation.ts
• src/utils/sessionStorage.ts

[coderabbitai]

🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

Apply the Windows normalization before the glob branch as well.

Line 498 only affects the non-glob path flow, but read globs return earlier through validateGlobPattern(cleanPath, ...) on Lines 472-478. A Windows input like /c/Users/foo/*.txt can still be resolved/authorized in the wrong path space while Git Bash expands it under C:\Users\foo.

Suggested direction

+  const pathForResolve =
+    getPlatform() === 'windows' ? posixPathToWindowsPath(cleanPath) : cleanPath
+
-  if (GLOB_PATTERN_REGEX.test(cleanPath)) {
+  if (GLOB_PATTERN_REGEX.test(pathForResolve)) {
     if (operationType === 'write' || operationType === 'create') {
       return {
         allowed: false,
-        resolvedPath: cleanPath,
+        resolvedPath: pathForResolve,
         decisionReason: {
           type: 'other',
           reason:
             'Glob patterns are not allowed in write operations. Please specify an exact file path.',
         },
       }
     }

     return validateGlobPattern(
-      cleanPath,
+      pathForResolve,
       cwd,
       toolPermissionContext,
       operationType,
     )
   }

-  const pathForResolve =
-    getPlatform() === 'windows' ? posixPathToWindowsPath(cleanPath) : cleanPath

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/permissions/pathValidation.ts` around lines 482 - 502, The Windows
path normalization in pathValidation should be applied before both the glob and
non-glob validation flows, not only before isAbsolute/resolve. Update the path
handling around validateGlobPattern and the later resolve logic so MinGW-style
absolute paths are normalized to Windows paths first, ensuring inputs like
/c/Users/foo/*.txt are validated against the same filesystem location Git Bash
uses.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Restore the "cache is empty" priming guard.

Line 4031 only checks has(sessionId), but the safety condition here is stronger than that. If the cache already contains other sessions, getLastSessionLog() can still seed this session with a stale disk snapshot, and later UUID dedup will miss unflushed messages.

Suggested fix

   const messagesCache = getSessionMessagesCache()
-  if (!messagesCache.has(sessionId)) {
+  if (messagesCache.size === 0) {
     messagesCache.set(sessionId, Promise.resolve(new Set(messages.keys())))
   }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	const messagesCache = getSessionMessagesCache()
	if (!messagesCache.has(sessionId)) {
	messagesCache.set(sessionId, Promise.resolve(new Set(messages.keys())))
	const messagesCache = getSessionMessagesCache()
	if (messagesCache.size === 0) {
	messagesCache.set(sessionId, Promise.resolve(new Set(messages.keys())))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/sessionStorage.ts` around lines 4030 - 4032, The session cache
priming in getLastSessionLog()/getSessionMessagesCache() needs the original
“cache is empty” guard restored, not just a per-session has(sessionId) check.
Update the priming logic so it only seeds from disk when the entire messages
cache is empty, preventing stale snapshots from being written into a session
that already has cached entries and avoiding UUID dedup misses for unflushed
messages.