refactor: decouple stored session deletion from ChatRecordingService (#22920)

packages/cli/src/utils/sessions.test.ts

@@ -5,7 +5,7 @@
  */
 
 import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
-import { ChatRecordingService, type Config } from '@google/gemini-cli-core';
+import { deleteStoredSession, type Config } from '@google/gemini-cli-core';
 import { listSessions, deleteSession } from './sessions.js';
 import { SessionSelector, type SessionInfo } from './sessionUtils.js';
 
@@ -14,7 +14,7 @@ const mocks = vi.hoisted(() => ({
   writeToStderr: vi.fn(),
 }));
 
-// Mock the SessionSelector and ChatRecordingService
+// Mock the SessionSelector and deleteStoredSession.
 vi.mock('./sessionUtils.js', () => ({
   SessionSelector: vi.fn(),
   formatRelativeTime: vi.fn(() => 'some time ago'),
@@ -24,7 +24,7 @@ vi.mock('@google/gemini-cli-core', async () => {
   const actual = await vi.importActual('@google/gemini-cli-core');
   return {
     ...actual,
-    ChatRecordingService: vi.fn(),
+    deleteStoredSession: vi.fn(),
     generateSummary: vi.fn().mockResolvedValue(undefined),
     writeToStdout: mocks.writeToStdout,
     writeToStderr: mocks.writeToStderr,
@@ -347,7 +347,8 @@ describe('deleteSession', () => {
 
     // Create mock methods
     mockListSessions = vi.fn();
-    mockDeleteSession = vi.fn();
+    mockDeleteSession = vi.mocked(deleteStoredSession);
+    mockDeleteSession.mockReset();
 
     // Mock SessionSelector constructor
     vi.mocked(SessionSelector).mockImplementation(
@@ -356,14 +357,6 @@ describe('deleteSession', () => {
           listSessions: mockListSessions,
         }) as unknown as InstanceType<typeof SessionSelector>,
     );
-
-    // Mock ChatRecordingService
-    vi.mocked(ChatRecordingService).mockImplementation(
-      () =>
-        ({
-          deleteSession: mockDeleteSession,
-        }) as unknown as InstanceType<typeof ChatRecordingService>,
-    );
   });
 
   afterEach(() => {
@@ -411,7 +404,10 @@ describe('deleteSession', () => {
 
     // Assert
     expect(mockListSessions).toHaveBeenCalledOnce();
-    expect(mockDeleteSession).toHaveBeenCalledWith('session-file-123');
+    expect(mockDeleteSession).toHaveBeenCalledWith(
+      mockConfig,
+      'session-file-123',
+    );
     expect(mocks.writeToStdout).toHaveBeenCalledWith(
       'Deleted session 1: Test session (some time ago)',
     );
@@ -458,7 +454,10 @@ describe('deleteSession', () => {
 
     // Assert
     expect(mockListSessions).toHaveBeenCalledOnce();
-    expect(mockDeleteSession).toHaveBeenCalledWith('session-file-2');
+    expect(mockDeleteSession).toHaveBeenCalledWith(
+      mockConfig,
+      'session-file-2',
+    );
     expect(mocks.writeToStdout).toHaveBeenCalledWith(
       'Deleted session 2: Second session (some time ago)',
     );
@@ -641,7 +640,10 @@ describe('deleteSession', () => {
     await deleteSession(mockConfig, '1');
 
     // Assert
-    expect(mockDeleteSession).toHaveBeenCalledWith('session-file-1');
+    expect(mockDeleteSession).toHaveBeenCalledWith(
+      mockConfig,
+      'session-file-1',
+    );
     expect(mocks.writeToStderr).toHaveBeenCalledWith(
       'Failed to delete session: File deletion failed',
     );
@@ -732,7 +734,10 @@ describe('deleteSession', () => {
     await deleteSession(mockConfig, '1');
 
     // Assert
-    expect(mockDeleteSession).toHaveBeenCalledWith('session-file-1');
+    expect(mockDeleteSession).toHaveBeenCalledWith(
+      mockConfig,
+      'session-file-1',
+    );
     expect(mocks.writeToStdout).toHaveBeenCalledWith(
       expect.stringContaining('Oldest session'),
     );

packages/cli/src/utils/sessions.ts

@@ -5,7 +5,7 @@
  */
 
 import {
-  ChatRecordingService,
+  deleteStoredSession,
   generateSummary,
   writeToStderr,
   writeToStdout,
@@ -95,9 +95,7 @@ export async function deleteSession(
   }
 
   try {
-    // Use ChatRecordingService to delete the session
-    const chatRecordingService = new ChatRecordingService(config);
-    await chatRecordingService.deleteSession(sessionToDelete.file);
+    await deleteStoredSession(config, sessionToDelete.file);
 
     const time = formatRelativeTime(sessionToDelete.lastUpdated);
     writeToStdout(

packages/core/src/services/chatRecordingService.ts

@@ -12,7 +12,7 @@ import { sanitizeFilenamePart } from '../utils/fileUtils.js';
 import { isNodeError } from '../utils/errors.js';
 import {
   deleteSessionArtifactsAsync,
-  deleteSubagentSessionDirAndArtifactsAsync,
+  deleteStoredSession,
 } from '../utils/sessionOperations.js';
 import readline from 'node:readline';
 import { randomUUID } from 'node:crypto';
@@ -99,10 +99,6 @@ function isTextPart(part: unknown): part is { text: string } {
   return isStringProperty(part, 'text');
 }
 
-function isSessionIdRecord(record: unknown): record is { sessionId: string } {
-  return isStringProperty(record, 'sessionId');
-}
-
 export async function loadConversationRecord(
   filePath: string,
   options?: LoadConversationOptions,
@@ -681,140 +677,7 @@ export class ChatRecordingService {
    * @throws {Error} If shortId validation fails.
    */
   async deleteSession(sessionIdOrBasename: string): Promise<void> {
-    try {
-      const tempDir = this.context.config.storage.getProjectTempDir();
-      const chatsDir = path.join(tempDir, 'chats');
-      const shortId = this.deriveShortId(sessionIdOrBasename);
-
-      // Using stat instead of existsSync for async sanity
-      if (!(await fs.promises.stat(chatsDir).catch(() => null))) {
-        return; // Nothing to delete
-      }
-
-      const matchingFiles = await this.getMatchingSessionFiles(
-        chatsDir,
-        shortId,
-      );
-      for (const file of matchingFiles) {
-        await this.deleteSessionAndArtifacts(chatsDir, file, tempDir);
-      }
-    } catch (error) {
-      debugLogger.error('Error deleting session file.', error);
-      throw error;
-    }
-  }
-
-  private deriveShortId(sessionIdOrBasename: string): string {
-    let shortId = sessionIdOrBasename;
-    if (sessionIdOrBasename.startsWith(SESSION_FILE_PREFIX)) {
-      const withoutExt = sessionIdOrBasename.replace(/\.jsonl?$/, '');
-      const parts = withoutExt.split('-');
-      shortId = parts[parts.length - 1];
-    } else if (sessionIdOrBasename.length >= 8) {
-      shortId = sessionIdOrBasename.slice(0, 8);
-    } else {
-      throw new Error('Invalid sessionId or basename provided for deletion');
-    }
-
-    if (shortId.length !== 8) {
-      throw new Error('Derived shortId must be exactly 8 characters');
-    }
-
-    return shortId;
-  }
-
-  private async getMatchingSessionFiles(
-    chatsDir: string,
-    shortId: string,
-  ): Promise<string[]> {
-    const files = await fs.promises.readdir(chatsDir);
-    return files.filter(
-      (f) =>
-        f.startsWith(SESSION_FILE_PREFIX) &&
-        (f.endsWith(`-${shortId}.json`) || f.endsWith(`-${shortId}.jsonl`)),
-    );
-  }
-
-  /**
-   * Deletes a single session file and its associated logs, tool-outputs, and directory.
-   */
-  private async deleteSessionAndArtifacts(
-    chatsDir: string,
-    file: string,
-    tempDir: string,
-  ): Promise<void> {
-    const filePath = path.join(chatsDir, file);
-    let fullSessionId: string | undefined;
-
-    try {
-      const CHUNK_SIZE = 4096;
-      const buffer = Buffer.alloc(CHUNK_SIZE);
-      let firstLine: string;
-      let fd: fs.promises.FileHandle | undefined;
-      try {
-        fd = await fs.promises.open(filePath, 'r');
-        const { bytesRead } = await fd.read(buffer, 0, CHUNK_SIZE, 0);
-        if (bytesRead > 0) {
-          const contentChunk = buffer.toString('utf8', 0, bytesRead);
-          const newlineIndex = contentChunk.indexOf('\n');
-          firstLine =
-            newlineIndex !== -1
-              ? contentChunk.substring(0, newlineIndex)
-              : contentChunk;
-
-          try {
-            const content = JSON.parse(firstLine) as unknown;
-            if (isSessionIdRecord(content)) {
-              fullSessionId = content.sessionId;
-            }
-          } catch {
-            // If first line parse fails, it might be a legacy pretty-printed JSON.
-            // We'll fall back to full file read below.
-          }
-        }
-      } finally {
-        if (fd !== undefined) {
-          await fd.close();
-        }
-      }
-
-      // Fallback for legacy JSON files if we couldn't get sessionId from first line
-      if (!fullSessionId) {
-        try {
-          const fileContent = await fs.promises.readFile(filePath, 'utf8');
-          const parsed = JSON.parse(fileContent) as unknown;
-          if (isSessionIdRecord(parsed)) {
-            fullSessionId = parsed.sessionId;
-          }
-        } catch {
-          // Ignore parse errors, we'll still try to unlink the file
-        }
-      }
-
-      if (fullSessionId) {
-        // Delegate to shared utility!
-        await deleteSessionArtifactsAsync(fullSessionId, tempDir);
-        await deleteSubagentSessionDirAndArtifactsAsync(
-          fullSessionId,
-          chatsDir,
-          tempDir,
-        );
-      }
-    } catch (error) {
-      debugLogger.error(
-        `Error deleting artifacts for session file ${file}:`,
-        error,
-      );
-    } finally {
-      // ALWAYS try to delete the session file itself
-      try {
-        await fs.promises.unlink(filePath);
-      } catch (error) {
-        if (isNodeError(error) && error.code !== 'ENOENT') {
-          debugLogger.error(`Error unlinking session file ${file}:`, error);
-        }
-      }
-    }
+    return deleteStoredSession(this.context.config, sessionIdOrBasename);
   }
 
   /**