From 22d0b6dfea744affece2b24246896d924062185e Mon Sep 17 00:00:00 2001
From: ennsharma <ennsharma@dropyacht.com>
Date: Thu, 11 Jun 2026 16:12:01 -0700
Subject: [PATCH] Clean up session ownership keys in Redis

session:${sessionId}:owner keys were written without a TTL and never
deleted, so they accumulated forever (#21).

- Delete the ownership key in ServerRedisTransport.close(), which runs
  on client DELETE, SHUTDOWN control messages, and inactivity timeout
- Set a 30-minute safety-net TTL on the key, refreshed on each
  authorized request, so keys orphaned by a crashed process still
  expire

Fixes #21

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---
 .../mcp/services/redisTransport.test.ts       | 46 +++++++++++++++++++
 src/modules/mcp/services/redisTransport.ts    | 31 +++++++++++--
 2 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/src/modules/mcp/services/redisTransport.test.ts b/src/modules/mcp/services/redisTransport.test.ts
index 3c478bf..df4e8e4 100644
--- a/src/modules/mcp/services/redisTransport.test.ts
+++ b/src/modules/mcp/services/redisTransport.test.ts
@@ -297,6 +297,52 @@ describe('Redis Transport', () => {
       expect(await validateSessionOwnership(sessionId, 'different-user')).toBe(false);
     });
 
+    it('should set the ownership key with a TTL', async () => {
+      const setSpy = jest.spyOn(mockRedis, 'set');
+
+      await setSessionOwner(sessionId, userId);
+
+      expect(setSpy).toHaveBeenCalledWith(
+        `session:${sessionId}:owner`,
+        userId,
+        expect.objectContaining({ EX: expect.any(Number) })
+      );
+    });
+
+    it('should refresh the ownership TTL on successful validation only', async () => {
+      const expireSpy = jest.spyOn(mockRedis, 'expire');
+      await setSessionOwner(sessionId, userId);
+
+      expect(await validateSessionOwnership(sessionId, userId)).toBe(true);
+      expect(expireSpy).toHaveBeenCalledWith(`session:${sessionId}:owner`, expect.any(Number));
+
+      expireSpy.mockClear();
+      expect(await validateSessionOwnership(sessionId, 'different-user')).toBe(false);
+      expect(expireSpy).not.toHaveBeenCalled();
+    });
+
+    it('should remove the ownership key when the server transport closes', async () => {
+      await setSessionOwner(sessionId, userId);
+      expect(await getSessionOwner(sessionId)).toBe(userId);
+
+      const transport = new ServerRedisTransport(sessionId);
+      await transport.start();
+      await transport.close();
+
+      expect(await getSessionOwner(sessionId)).toBeNull();
+    });
+
+    it('should remove the ownership key when the session is shut down via control message', async () => {
+      await setSessionOwner(sessionId, userId);
+
+      const transport = new ServerRedisTransport(sessionId);
+      await transport.start();
+      await shutdownSession(sessionId);
+      await new Promise(resolve => setTimeout(resolve, 10));
+
+      expect(await getSessionOwner(sessionId)).toBeNull();
+    });
+
     it('should check if session is owned by user including liveness', async () => {
       // Session not live yet
       expect(await isSessionOwnedBy(sessionId, userId)).toBe(false);
diff --git a/src/modules/mcp/services/redisTransport.ts b/src/modules/mcp/services/redisTransport.ts
index 906f34c..259356c 100644
--- a/src/modules/mcp/services/redisTransport.ts
+++ b/src/modules/mcp/services/redisTransport.ts
@@ -8,6 +8,11 @@ import { logger } from "../../shared/logger.js";
 let redisTransportCounter = 0;
 const notificationStreamId = "__GET_stream";
 
+// Safety-net TTL for session ownership keys. Live sessions refresh it on every
+// authorized request, and ServerRedisTransport.close() deletes the key, so the
+// TTL only ever fires for keys orphaned by a crashed process.
+const SESSION_OWNER_TTL_SECONDS = 30 * 60;
+
 // Message types for Redis transport
 type RedisMessage = 
   | {
@@ -69,18 +74,33 @@ export async function isLive(sessionId: string): Promise<boolean> {
   return numSubs > 0;
 }
 
+function getSessionOwnerKey(sessionId: string): string {
+  return `session:${sessionId}:owner`;
+}
+
 export async function setSessionOwner(sessionId: string, userId: string): Promise<void> {
   logger.debug('Setting session owner', { sessionId, userId });
-  await redisClient.set(`session:${sessionId}:owner`, userId);
+  await redisClient.set(getSessionOwnerKey(sessionId), userId, { EX: SESSION_OWNER_TTL_SECONDS });
 }
 
 export async function getSessionOwner(sessionId: string): Promise<string | null> {
-  return await redisClient.get(`session:${sessionId}:owner`);
+  return await redisClient.get(getSessionOwnerKey(sessionId));
+}
+
+export async function deleteSessionOwner(sessionId: string): Promise<void> {
+  logger.debug('Deleting session owner', { sessionId });
+  await redisClient.del(getSessionOwnerKey(sessionId));
 }
 
 export async function validateSessionOwnership(sessionId: string, userId: string): Promise<boolean> {
   const owner = await getSessionOwner(sessionId);
-  return owner === userId;
+  if (owner !== userId) {
+    return false;
+  }
+  // Sliding expiration: each authorized request keeps the ownership key alive,
+  // so the TTL only reaps keys whose session never got cleaned up.
+  await redisClient.expire(getSessionOwnerKey(sessionId), SESSION_OWNER_TTL_SECONDS);
+  return true;
 }
 
 export async function isSessionOwnedBy(sessionId: string, userId: string): Promise<boolean> {
@@ -325,7 +345,10 @@ export class ServerRedisTransport implements Transport {
       await this.controlCleanup();
       this.controlCleanup = undefined;
     }
-    
+
+    // The session is finished — remove its ownership key (#21)
+    await deleteSessionOwner(this._sessionId);
+
     this.onclose?.();
   }
 }