From 81a97e78f1f371fbf4ea63f480aeaa12a74e3068 Mon Sep 17 00:00:00 2001 From: Abhi <43648792+abhipatel12@users.noreply.github.com> Date: Wed, 18 Mar 2026 10:42:15 -0400 Subject: [PATCH] refactor(cli): group subagent trajectory deletion and use native filesystem testing (#22890) --- .../utils/sessionCleanup.integration.test.ts | 150 ++ packages/cli/src/utils/sessionCleanup.test.ts | 2269 ++++++----------- packages/cli/src/utils/sessionCleanup.ts | 214 +- 3 files changed, 1081 insertions(+), 1552 deletions(-) diff --git a/packages/cli/src/utils/sessionCleanup.integration.test.ts b/packages/cli/src/utils/sessionCleanup.integration.test.ts index eec9a12592..871e30f669 100644 --- a/packages/cli/src/utils/sessionCleanup.integration.test.ts +++ b/packages/cli/src/utils/sessionCleanup.integration.test.ts @@ -252,4 +252,154 @@ describe('Session Cleanup Integration', () => { await fs.rm(tempDir, { recursive: true, force: true }); } }); + + it('should delete subagent files and their artifacts when parent expires', async () => { + // Create a temporary directory with test sessions + const fs = await import('node:fs/promises'); + const path = await import('node:path'); + const os = await import('node:os'); + + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'gemini-test-')); + const chatsDir = path.join(tempDir, 'chats'); + const logsDir = path.join(tempDir, 'logs'); + const toolOutputsDir = path.join(tempDir, 'tool-outputs'); + + await fs.mkdir(chatsDir, { recursive: true }); + await fs.mkdir(logsDir, { recursive: true }); + await fs.mkdir(toolOutputsDir, { recursive: true }); + + const now = new Date(); + const oldDate = new Date(now.getTime() - 5 * 24 * 60 * 60 * 1000); // 5 days ago + + // The shortId that ties them together + const sharedShortId = 'abcdef12'; + + const parentSessionId = 'parent-uuid-123'; + const parentFile = path.join( + chatsDir, + `${SESSION_FILE_PREFIX}2024-01-01T10-00-00-${sharedShortId}.json`, + ); + await fs.writeFile( + parentFile, + JSON.stringify({ + sessionId: parentSessionId, + messages: [], + startTime: oldDate.toISOString(), + lastUpdated: oldDate.toISOString(), + }), + ); + + const subagentSessionId = 'subagent-uuid-456'; + const subagentFile = path.join( + chatsDir, + `${SESSION_FILE_PREFIX}2024-01-01T10-05-00-${sharedShortId}.json`, + ); + await fs.writeFile( + subagentFile, + JSON.stringify({ + sessionId: subagentSessionId, + messages: [], + startTime: oldDate.toISOString(), + lastUpdated: oldDate.toISOString(), + }), + ); + + const parentLogFile = path.join( + logsDir, + `session-${parentSessionId}.jsonl`, + ); + await fs.writeFile(parentLogFile, '{"log": "parent"}'); + + const parentToolOutputsDir = path.join( + toolOutputsDir, + `session-${parentSessionId}`, + ); + await fs.mkdir(parentToolOutputsDir, { recursive: true }); + await fs.writeFile( + path.join(parentToolOutputsDir, 'some-output.txt'), + 'data', + ); + + const subagentLogFile = path.join( + logsDir, + `session-${subagentSessionId}.jsonl`, + ); + await fs.writeFile(subagentLogFile, '{"log": "subagent"}'); + + const subagentToolOutputsDir = path.join( + toolOutputsDir, + `session-${subagentSessionId}`, + ); + await fs.mkdir(subagentToolOutputsDir, { recursive: true }); + await fs.writeFile( + path.join(subagentToolOutputsDir, 'some-output.txt'), + 'data', + ); + + const currentShortId = 'current1'; + const currentFile = path.join( + chatsDir, + `${SESSION_FILE_PREFIX}2025-01-20T10-00-00-${currentShortId}.json`, + ); + await fs.writeFile( + currentFile, + JSON.stringify({ + sessionId: 'current-session', + messages: [ + { + type: 'user', + content: [{ type: 'text', text: 'hello' }], + timestamp: now.toISOString(), + }, + ], + startTime: now.toISOString(), + lastUpdated: now.toISOString(), + }), + ); + + // Configure test + const config: Config = { + storage: { + getProjectTempDir: () => tempDir, + }, + getSessionId: () => 'current-session', // Mock CLI instance ID + getDebugMode: () => false, + initialize: async () => undefined, + } as unknown as Config; + + const settings: Settings = { + general: { + sessionRetention: { + enabled: true, + maxAge: '1d', // Expire things older than 1 day + }, + }, + }; + + try { + const result = await cleanupExpiredSessions(config, settings); + + // Verify the cleanup result object + // It scanned 3 files. It should delete 2 (parent + subagent), and keep 1 (current) + expect(result.disabled).toBe(false); + expect(result.scanned).toBe(3); + expect(result.deleted).toBe(2); + expect(result.skipped).toBe(1); + + // Verify on-disk file states + const chats = await fs.readdir(chatsDir); + expect(chats).toHaveLength(1); + expect(chats).toContain( + `${SESSION_FILE_PREFIX}2025-01-20T10-00-00-${currentShortId}.json`, + ); // Only current is left + + const logs = await fs.readdir(logsDir); + expect(logs).toHaveLength(0); // Both parent and subagent logs were deleted + + const tools = await fs.readdir(toolOutputsDir); + expect(tools).toHaveLength(0); // Both parent and subagent tool output dirs were deleted + } finally { + await fs.rm(tempDir, { recursive: true, force: true }); + } + }); }); diff --git a/packages/cli/src/utils/sessionCleanup.test.ts b/packages/cli/src/utils/sessionCleanup.test.ts index bcd55953e8..b014159e08 100644 --- a/packages/cli/src/utils/sessionCleanup.test.ts +++ b/packages/cli/src/utils/sessionCleanup.test.ts @@ -6,138 +6,145 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import * as fs from 'node:fs/promises'; +import { existsSync, unlinkSync } from 'node:fs'; import * as path from 'node:path'; +import * as os from 'node:os'; import { - SESSION_FILE_PREFIX, type Config, debugLogger, + TOOL_OUTPUTS_DIR, + Storage, } from '@google/gemini-cli-core'; import type { Settings } from '../config/settings.js'; -import { cleanupExpiredSessions } from './sessionCleanup.js'; -import { type SessionInfo, getAllSessionFiles } from './sessionUtils.js'; - -// Mock the fs module -vi.mock('node:fs/promises'); -vi.mock('./sessionUtils.js', () => ({ - getAllSessionFiles: vi.fn(), -})); +import { + cleanupExpiredSessions, + cleanupToolOutputFiles, +} from './sessionCleanup.js'; vi.mock('@google/gemini-cli-core', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - Storage: class MockStorage { - getProjectTempDir() { - return '/tmp/test-project'; - } + debugLogger: { + error: vi.fn(), + warn: vi.fn(), + debug: vi.fn(), + info: vi.fn(), }, }; }); -const mockFs = vi.mocked(fs); -const mockGetAllSessionFiles = vi.mocked(getAllSessionFiles); +describe('Session Cleanup (Refactored)', () => { + let testTempDir: string; + let chatsDir: string; + let logsDir: string; + let toolOutputsDir: string; -// Create mock config -function createMockConfig(overrides: Partial = {}): Config { - return { - storage: { - getProjectTempDir: vi.fn().mockReturnValue('/tmp/test-project'), - }, - getSessionId: vi.fn().mockReturnValue('current123'), - getDebugMode: vi.fn().mockReturnValue(false), - initialize: vi.fn().mockResolvedValue(undefined), - ...overrides, - } as unknown as Config; -} - -// Create test session data -function createTestSessions(): SessionInfo[] { - const now = new Date(); - const oneWeekAgo = new Date(now.getTime() - 7 * 24 * 60 * 60 * 1000); - const twoWeeksAgo = new Date(now.getTime() - 14 * 24 * 60 * 60 * 1000); - const oneMonthAgo = new Date(now.getTime() - 30 * 24 * 60 * 60 * 1000); - - return [ - { - id: 'current123', - file: `${SESSION_FILE_PREFIX}2025-01-20T10-30-00-current12`, - fileName: `${SESSION_FILE_PREFIX}2025-01-20T10-30-00-current12.json`, - startTime: now.toISOString(), - lastUpdated: now.toISOString(), - messageCount: 5, - displayName: 'Current session', - firstUserMessage: 'Current session', - isCurrentSession: true, - index: 1, - }, - { - id: 'recent456', - file: `${SESSION_FILE_PREFIX}2025-01-18T15-45-00-recent45`, - fileName: `${SESSION_FILE_PREFIX}2025-01-18T15-45-00-recent45.json`, - startTime: oneWeekAgo.toISOString(), - lastUpdated: oneWeekAgo.toISOString(), - messageCount: 10, - displayName: 'Recent session', - firstUserMessage: 'Recent session', - isCurrentSession: false, - index: 2, - }, - { - id: 'old789abc', - file: `${SESSION_FILE_PREFIX}2025-01-10T09-15-00-old789ab`, - fileName: `${SESSION_FILE_PREFIX}2025-01-10T09-15-00-old789ab.json`, - startTime: twoWeeksAgo.toISOString(), - lastUpdated: twoWeeksAgo.toISOString(), - messageCount: 3, - displayName: 'Old session', - firstUserMessage: 'Old session', - isCurrentSession: false, - index: 3, - }, - { - id: 'ancient12', - file: `${SESSION_FILE_PREFIX}2024-12-25T12-00-00-ancient1`, - fileName: `${SESSION_FILE_PREFIX}2024-12-25T12-00-00-ancient1.json`, - startTime: oneMonthAgo.toISOString(), - lastUpdated: oneMonthAgo.toISOString(), - messageCount: 15, - displayName: 'Ancient session', - firstUserMessage: 'Ancient session', - isCurrentSession: false, - index: 4, - }, - ]; -} - -describe('Session Cleanup', () => { - beforeEach(() => { + beforeEach(async () => { vi.clearAllMocks(); - vi.spyOn(debugLogger, 'error').mockImplementation(() => {}); - vi.spyOn(debugLogger, 'warn').mockImplementation(() => {}); - // By default, return all test sessions as valid - const sessions = createTestSessions(); - mockGetAllSessionFiles.mockResolvedValue( - sessions.map((session) => ({ - fileName: session.fileName, - sessionInfo: session, - })), + testTempDir = await fs.mkdtemp( + path.join(os.tmpdir(), 'gemini-cli-cleanup-test-'), ); + chatsDir = path.join(testTempDir, 'chats'); + logsDir = path.join(testTempDir, 'logs'); + toolOutputsDir = path.join(testTempDir, TOOL_OUTPUTS_DIR); + + await fs.mkdir(chatsDir, { recursive: true }); + await fs.mkdir(logsDir, { recursive: true }); + await fs.mkdir(toolOutputsDir, { recursive: true }); }); - afterEach(() => { + afterEach(async () => { vi.restoreAllMocks(); + if (testTempDir && existsSync(testTempDir)) { + await fs.rm(testTempDir, { recursive: true, force: true }); + } }); - describe('cleanupExpiredSessions', () => { + function createMockConfig(overrides: Partial = {}): Config { + return { + storage: { + getProjectTempDir: () => testTempDir, + }, + getSessionId: () => 'current123', + getDebugMode: () => false, + initialize: async () => {}, + ...overrides, + } as unknown as Config; + } + + async function writeSessionFile(session: { + id: string; + fileName: string; + lastUpdated: string; + }) { + const filePath = path.join(chatsDir, session.fileName); + await fs.writeFile( + filePath, + JSON.stringify({ + sessionId: session.id, + lastUpdated: session.lastUpdated, + startTime: session.lastUpdated, + messages: [{ type: 'user', content: 'hello' }], + }), + ); + } + + async function writeArtifacts(sessionId: string) { + // Log file + await fs.writeFile( + path.join(logsDir, `session-${sessionId}.jsonl`), + 'log content', + ); + // Tool output directory + const sessionOutputDir = path.join(toolOutputsDir, `session-${sessionId}`); + await fs.mkdir(sessionOutputDir, { recursive: true }); + await fs.writeFile( + path.join(sessionOutputDir, 'output.txt'), + 'tool output', + ); + // Session directory + await fs.mkdir(path.join(testTempDir, sessionId), { recursive: true }); + } + + async function seedSessions() { + const now = new Date(); + const twoWeeksAgo = new Date(now.getTime() - 14 * 24 * 60 * 60 * 1000); + const oneMonthAgo = new Date(now.getTime() - 30 * 24 * 60 * 60 * 1000); + + const sessions = [ + { + id: 'current123', + fileName: 'session-20250101-current1.json', + lastUpdated: now.toISOString(), + }, + { + id: 'old789abc', + fileName: 'session-20250110-old789ab.json', + lastUpdated: twoWeeksAgo.toISOString(), + }, + { + id: 'ancient12', + fileName: 'session-20241225-ancient1.json', + lastUpdated: oneMonthAgo.toISOString(), + }, + ]; + + for (const session of sessions) { + await writeSessionFile(session); + await writeArtifacts(session.id); + } + return sessions; + } + + describe('Configuration boundaries & early exits', () => { it('should return early when cleanup is disabled', async () => { const config = createMockConfig(); const settings: Settings = { general: { sessionRetention: { enabled: false } }, }; - const result = await cleanupExpiredSessions(config, settings); - expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); expect(result.deleted).toBe(0); @@ -147,246 +154,99 @@ describe('Session Cleanup', () => { it('should return early when sessionRetention is not configured', async () => { const config = createMockConfig(); - const settings: Settings = {}; - + const settings: Settings = { general: {} }; const result = await cleanupExpiredSessions(config, settings); - expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); expect(result.deleted).toBe(0); - }); - - it('should handle invalid maxAge configuration', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: 'invalid-format', - }, - }, - }; - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(result.deleted).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining( - 'Session cleanup disabled: Error: Invalid retention period format', - ), - ); - }); - - it('should delete sessions older than maxAge', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '10d', // 10 days - }, - }, - }; - - // Mock successful file operations - mockFs.access.mockResolvedValue(undefined); - mockFs.readFile.mockResolvedValue( - JSON.stringify({ - sessionId: 'test', - messages: [], - startTime: '2025-01-01T00:00:00Z', - lastUpdated: '2025-01-01T00:00:00Z', - }), - ); - mockFs.unlink.mockResolvedValue(undefined); - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(4); - expect(result.deleted).toBe(2); // Should delete the 2-week-old and 1-month-old sessions - expect(result.skipped).toBe(2); // Current session + recent session should be skipped - expect(result.failed).toBe(0); - }); - - it('should never delete current session', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '1d', // Very short retention - }, - }, - }; - - // Mock successful file operations - mockFs.access.mockResolvedValue(undefined); - mockFs.readFile.mockResolvedValue( - JSON.stringify({ - sessionId: 'test', - messages: [], - startTime: '2025-01-01T00:00:00Z', - lastUpdated: '2025-01-01T00:00:00Z', - }), - ); - mockFs.unlink.mockResolvedValue(undefined); - - const result = await cleanupExpiredSessions(config, settings); - - // Should delete all sessions except the current one - expect(result.disabled).toBe(false); - expect(result.deleted).toBe(3); - - // Verify that unlink was never called with the current session file - const unlinkCalls = mockFs.unlink.mock.calls; - const currentSessionPath = path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}2025-01-20T10-30-00-current12.json`, - ); - expect( - unlinkCalls.find((call) => call[0] === currentSessionPath), - ).toBeUndefined(); - }); - - it('should handle count-based retention', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxCount: 2, // Keep only 2 most recent sessions - }, - }, - }; - - // Mock successful file operations - mockFs.access.mockResolvedValue(undefined); - mockFs.readFile.mockResolvedValue( - JSON.stringify({ - sessionId: 'test', - messages: [], - startTime: '2025-01-01T00:00:00Z', - lastUpdated: '2025-01-01T00:00:00Z', - }), - ); - mockFs.unlink.mockResolvedValue(undefined); - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(4); - expect(result.deleted).toBe(2); // Should delete 2 oldest sessions (after skipping the current one) - expect(result.skipped).toBe(2); // Current session + 1 recent session should be kept - }); - - it('should handle file system errors gracefully', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '1d', - }, - }, - }; - - // Mock file operations to succeed for access and readFile but fail for unlink - mockFs.access.mockResolvedValue(undefined); - mockFs.readFile.mockResolvedValue( - JSON.stringify({ - sessionId: 'test', - messages: [], - startTime: '2025-01-01T00:00:00Z', - lastUpdated: '2025-01-01T00:00:00Z', - }), - ); - mockFs.unlink.mockRejectedValue(new Error('Permission denied')); - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(4); - expect(result.deleted).toBe(0); - expect(result.failed).toBeGreaterThan(0); - }); - - it('should handle empty sessions directory', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '30d', - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.deleted).toBe(0); expect(result.skipped).toBe(0); expect(result.failed).toBe(0); }); - it('should handle global errors gracefully', async () => { + it('should require either maxAge or maxCount', async () => { const config = createMockConfig(); const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '30d', - }, - }, + general: { sessionRetention: { enabled: true } }, }; - - // Mock getSessionFiles to throw an error - mockGetAllSessionFiles.mockRejectedValue( - new Error('Directory access failed'), - ); - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(false); - expect(result.failed).toBe(1); - expect(debugLogger.warn).toHaveBeenCalledWith( - 'Session cleanup failed: Directory access failed', - ); - }); - - it('should respect minRetention configuration', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '12h', // Less than 1 day minimum - minRetention: '1d', - }, - }, - }; - - const result = await cleanupExpiredSessions(config, settings); - - // Should disable cleanup due to minRetention violation expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(result.deleted).toBe(0); + expect(debugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('Either maxAge or maxCount must be specified'), + ); }); + it.each([0, -1, -5])( + 'should validate maxCount range (rejecting %i)', + async (invalidCount) => { + const config = createMockConfig(); + const settings: Settings = { + general: { + sessionRetention: { enabled: true, maxCount: invalidCount }, + }, + }; + const result = await cleanupExpiredSessions(config, settings); + expect(result.disabled).toBe(true); + expect(debugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('maxCount must be at least 1'), + ); + }, + ); + + it('should reject if both maxAge and maxCount are invalid', async () => { + const config = createMockConfig(); + const settings: Settings = { + general: { + sessionRetention: { enabled: true, maxAge: 'invalid', maxCount: 0 }, + }, + }; + const result = await cleanupExpiredSessions(config, settings); + expect(result.disabled).toBe(true); + expect(debugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('Invalid retention period format'), + ); + }); + + it('should reject if maxAge is invalid even when maxCount is valid', async () => { + const config = createMockConfig(); + const settings: Settings = { + general: { + sessionRetention: { enabled: true, maxAge: 'invalid', maxCount: 5 }, + }, + }; + const result = await cleanupExpiredSessions(config, settings); + expect(result.disabled).toBe(true); + expect(debugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('Invalid retention period format'), + ); + }); + }); + + describe('Logging and Debug Mode', () => { it('should log debug information when enabled', async () => { + await seedSessions(); const config = createMockConfig({ getDebugMode: vi.fn().mockReturnValue(true), }); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxCount: 1 } }, + }; + + const debugSpy = vi + .spyOn(debugLogger, 'debug') + .mockImplementation(() => {}); + await cleanupExpiredSessions(config, settings); + + expect(debugSpy).toHaveBeenCalledWith( + expect.stringContaining('Session cleanup: deleted'), + ); + debugSpy.mockRestore(); + }); + }); + + describe('Basic retention rules', () => { + it('should delete sessions older than maxAge', async () => { + const sessions = await seedSessions(); + const config = createMockConfig(); const settings: Settings = { general: { sessionRetention: { @@ -396,1304 +256,723 @@ describe('Session Cleanup', () => { }, }; - // Mock successful file operations - mockFs.access.mockResolvedValue(undefined); - mockFs.readFile.mockResolvedValue( - JSON.stringify({ - sessionId: 'test', - messages: [], - startTime: '2025-01-01T00:00:00Z', - lastUpdated: '2025-01-01T00:00:00Z', - }), - ); - mockFs.unlink.mockResolvedValue(undefined); - - const debugSpy = vi - .spyOn(debugLogger, 'debug') - .mockImplementation(() => {}); - - await cleanupExpiredSessions(config, settings); - - expect(debugSpy).toHaveBeenCalledWith( - expect.stringContaining('Session cleanup: deleted'), - ); - expect(debugSpy).toHaveBeenCalledWith( - expect.stringContaining('Deleted expired session:'), - ); - - debugSpy.mockRestore(); - }); - }); - - describe('Specific cleanup scenarios', () => { - it('should delete sessions that exceed the cutoff date', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '7d', // Keep sessions for 7 days - }, - }, - }; - - // Create sessions with specific dates - const now = new Date(); - const fiveDaysAgo = new Date(now.getTime() - 5 * 24 * 60 * 60 * 1000); - const eightDaysAgo = new Date(now.getTime() - 8 * 24 * 60 * 60 * 1000); - const fifteenDaysAgo = new Date(now.getTime() - 15 * 24 * 60 * 60 * 1000); - - const testSessions: SessionInfo[] = [ - { - id: 'current', - file: `${SESSION_FILE_PREFIX}current`, - fileName: `${SESSION_FILE_PREFIX}current.json`, - startTime: now.toISOString(), - lastUpdated: now.toISOString(), - messageCount: 1, - displayName: 'Current', - firstUserMessage: 'Current', - isCurrentSession: true, - index: 1, - }, - { - id: 'session5d', - file: `${SESSION_FILE_PREFIX}5d`, - fileName: `${SESSION_FILE_PREFIX}5d.json`, - startTime: fiveDaysAgo.toISOString(), - lastUpdated: fiveDaysAgo.toISOString(), - messageCount: 1, - displayName: '5 days old', - firstUserMessage: '5 days', - isCurrentSession: false, - index: 2, - }, - { - id: 'session8d', - file: `${SESSION_FILE_PREFIX}8d`, - fileName: `${SESSION_FILE_PREFIX}8d.json`, - startTime: eightDaysAgo.toISOString(), - lastUpdated: eightDaysAgo.toISOString(), - messageCount: 1, - displayName: '8 days old', - firstUserMessage: '8 days', - isCurrentSession: false, - index: 3, - }, - { - id: 'session15d', - file: `${SESSION_FILE_PREFIX}15d`, - fileName: `${SESSION_FILE_PREFIX}15d.json`, - startTime: fifteenDaysAgo.toISOString(), - lastUpdated: fifteenDaysAgo.toISOString(), - messageCount: 1, - displayName: '15 days old', - firstUserMessage: '15 days', - isCurrentSession: false, - index: 4, - }, - ]; - - mockGetAllSessionFiles.mockResolvedValue( - testSessions.map((session) => ({ - fileName: session.fileName, - sessionInfo: session, - })), - ); - - // Mock successful file operations - mockFs.access.mockResolvedValue(undefined); - mockFs.readFile.mockResolvedValue( - JSON.stringify({ - sessionId: 'test', - messages: [], - startTime: '2025-01-01T00:00:00Z', - lastUpdated: '2025-01-01T00:00:00Z', - }), - ); - mockFs.unlink.mockResolvedValue(undefined); - const result = await cleanupExpiredSessions(config, settings); - // Should delete sessions older than 7 days (8d and 15d sessions) - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(4); + expect(result.scanned).toBe(3); expect(result.deleted).toBe(2); - expect(result.skipped).toBe(2); // Current + 5d session + expect(result.skipped).toBe(1); + expect(result.failed).toBe(0); + expect(existsSync(path.join(chatsDir, sessions[0].fileName))).toBe(true); + expect(existsSync(path.join(chatsDir, sessions[1].fileName))).toBe(false); + expect(existsSync(path.join(chatsDir, sessions[2].fileName))).toBe(false); - // Verify which files were deleted - const unlinkCalls = mockFs.unlink.mock.calls.map((call) => call[0]); - expect(unlinkCalls).toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}8d.json`, - ), - ); - expect(unlinkCalls).toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}15d.json`, - ), - ); - expect(unlinkCalls).not.toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}5d.json`, - ), - ); + // Verify artifacts for an old session are gone + expect( + existsSync(path.join(logsDir, `session-${sessions[1].id}.jsonl`)), + ).toBe(false); + expect( + existsSync(path.join(toolOutputsDir, `session-${sessions[1].id}`)), + ).toBe(false); + expect(existsSync(path.join(testTempDir, sessions[1].id))).toBe(false); // Session directory should be deleted }); it('should NOT delete sessions within the cutoff date', async () => { + const sessions = await seedSessions(); // [current, 14d, 30d] const config = createMockConfig(); const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '14d', // Keep sessions for 14 days - }, - }, + general: { sessionRetention: { enabled: true, maxAge: '60d' } }, }; - // Create sessions all within the retention period - const now = new Date(); - const oneDayAgo = new Date(now.getTime() - 1 * 24 * 60 * 60 * 1000); - const sevenDaysAgo = new Date(now.getTime() - 7 * 24 * 60 * 60 * 1000); - const thirteenDaysAgo = new Date( - now.getTime() - 13 * 24 * 60 * 60 * 1000, - ); - - const testSessions: SessionInfo[] = [ - { - id: 'current', - file: `${SESSION_FILE_PREFIX}current`, - fileName: `${SESSION_FILE_PREFIX}current.json`, - startTime: now.toISOString(), - lastUpdated: now.toISOString(), - messageCount: 1, - displayName: 'Current', - firstUserMessage: 'Current', - isCurrentSession: true, - index: 1, - }, - { - id: 'session1d', - file: `${SESSION_FILE_PREFIX}1d`, - fileName: `${SESSION_FILE_PREFIX}1d.json`, - startTime: oneDayAgo.toISOString(), - lastUpdated: oneDayAgo.toISOString(), - messageCount: 1, - displayName: '1 day old', - firstUserMessage: '1 day', - isCurrentSession: false, - index: 2, - }, - { - id: 'session7d', - file: `${SESSION_FILE_PREFIX}7d`, - fileName: `${SESSION_FILE_PREFIX}7d.json`, - startTime: sevenDaysAgo.toISOString(), - lastUpdated: sevenDaysAgo.toISOString(), - messageCount: 1, - displayName: '7 days old', - firstUserMessage: '7 days', - isCurrentSession: false, - index: 3, - }, - { - id: 'session13d', - file: `${SESSION_FILE_PREFIX}13d`, - fileName: `${SESSION_FILE_PREFIX}13d.json`, - startTime: thirteenDaysAgo.toISOString(), - lastUpdated: thirteenDaysAgo.toISOString(), - messageCount: 1, - displayName: '13 days old', - firstUserMessage: '13 days', - isCurrentSession: false, - index: 4, - }, - ]; - - mockGetAllSessionFiles.mockResolvedValue( - testSessions.map((session) => ({ - fileName: session.fileName, - sessionInfo: session, - })), - ); - - // Mock successful file operations - mockFs.access.mockResolvedValue(undefined); - mockFs.readFile.mockResolvedValue( - JSON.stringify({ - sessionId: 'test', - messages: [], - startTime: '2025-01-01T00:00:00Z', - lastUpdated: '2025-01-01T00:00:00Z', - }), - ); - mockFs.unlink.mockResolvedValue(undefined); - + // 60d cutoff should keep everything that was seeded const result = await cleanupExpiredSessions(config, settings); - // Should NOT delete any sessions as all are within 14 days - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(4); expect(result.deleted).toBe(0); - expect(result.skipped).toBe(4); - expect(result.failed).toBe(0); - - // Verify no files were deleted - expect(mockFs.unlink).not.toHaveBeenCalled(); - }); - - it('should keep N most recent deletable sessions', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxCount: 3, // Keep only 3 most recent sessions - }, - }, - }; - - // Create 6 sessions with different timestamps - const now = new Date(); - const sessions: SessionInfo[] = [ - { - id: 'current', - file: `${SESSION_FILE_PREFIX}current`, - fileName: `${SESSION_FILE_PREFIX}current.json`, - startTime: now.toISOString(), - lastUpdated: now.toISOString(), - messageCount: 1, - displayName: 'Current (newest)', - firstUserMessage: 'Current', - isCurrentSession: true, - index: 1, - }, - ]; - - // Add 5 more sessions with decreasing timestamps - for (let i = 1; i <= 5; i++) { - const daysAgo = new Date(now.getTime() - i * 24 * 60 * 60 * 1000); - sessions.push({ - id: `session${i}`, - file: `${SESSION_FILE_PREFIX}${i}d`, - fileName: `${SESSION_FILE_PREFIX}${i}d.json`, - startTime: daysAgo.toISOString(), - lastUpdated: daysAgo.toISOString(), - messageCount: 1, - displayName: `${i} days old`, - firstUserMessage: `${i} days`, - isCurrentSession: false, - index: i + 1, - }); - } - - mockGetAllSessionFiles.mockResolvedValue( - sessions.map((session) => ({ - fileName: session.fileName, - sessionInfo: session, - })), - ); - - // Mock successful file operations - mockFs.access.mockResolvedValue(undefined); - mockFs.readFile.mockResolvedValue( - JSON.stringify({ - sessionId: 'test', - messages: [], - startTime: '2025-01-01T00:00:00Z', - lastUpdated: '2025-01-01T00:00:00Z', - }), - ); - mockFs.unlink.mockResolvedValue(undefined); - - const result = await cleanupExpiredSessions(config, settings); - - // Should keep current + 2 most recent (1d and 2d), delete 3d, 4d, 5d - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(6); - expect(result.deleted).toBe(3); expect(result.skipped).toBe(3); - - // Verify which files were deleted (should be the 3 oldest) - const unlinkCalls = mockFs.unlink.mock.calls.map((call) => call[0]); - expect(unlinkCalls).toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}3d.json`, - ), - ); - expect(unlinkCalls).toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}4d.json`, - ), - ); - expect(unlinkCalls).toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}5d.json`, - ), - ); - - // Verify which files were NOT deleted - expect(unlinkCalls).not.toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}current.json`, - ), - ); - expect(unlinkCalls).not.toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}1d.json`, - ), - ); - expect(unlinkCalls).not.toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}2d.json`, - ), - ); + for (const session of sessions) { + expect(existsSync(path.join(chatsDir, session.fileName))).toBe(true); + } }); - it('should handle combined maxAge and maxCount retention (most restrictive wins)', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '10d', // Keep sessions for 10 days - maxCount: 2, // But also keep only 2 most recent - }, - }, - }; + it('should handle count-based retention (keeping N most recent)', async () => { + const sessions = await seedSessions(); // [current, 14d, 30d] - // Create sessions where maxCount is more restrictive + // Seed two additional granular files to prove sorting works const now = new Date(); const threeDaysAgo = new Date(now.getTime() - 3 * 24 * 60 * 60 * 1000); const fiveDaysAgo = new Date(now.getTime() - 5 * 24 * 60 * 60 * 1000); - const sevenDaysAgo = new Date(now.getTime() - 7 * 24 * 60 * 60 * 1000); - const twelveDaysAgo = new Date(now.getTime() - 12 * 24 * 60 * 60 * 1000); - const testSessions: SessionInfo[] = [ - { - id: 'current', - file: `${SESSION_FILE_PREFIX}current`, - fileName: `${SESSION_FILE_PREFIX}current.json`, - startTime: now.toISOString(), - lastUpdated: now.toISOString(), - messageCount: 1, - displayName: 'Current', - firstUserMessage: 'Current', - isCurrentSession: true, - index: 1, - }, - { - id: 'session3d', - file: `${SESSION_FILE_PREFIX}3d`, - fileName: `${SESSION_FILE_PREFIX}3d.json`, - startTime: threeDaysAgo.toISOString(), - lastUpdated: threeDaysAgo.toISOString(), - messageCount: 1, - displayName: '3 days old', - firstUserMessage: '3 days', - isCurrentSession: false, - index: 2, - }, - { - id: 'session5d', - file: `${SESSION_FILE_PREFIX}5d`, - fileName: `${SESSION_FILE_PREFIX}5d.json`, - startTime: fiveDaysAgo.toISOString(), - lastUpdated: fiveDaysAgo.toISOString(), - messageCount: 1, - displayName: '5 days old', - firstUserMessage: '5 days', - isCurrentSession: false, - index: 3, - }, - { - id: 'session7d', - file: `${SESSION_FILE_PREFIX}7d`, - fileName: `${SESSION_FILE_PREFIX}7d.json`, - startTime: sevenDaysAgo.toISOString(), - lastUpdated: sevenDaysAgo.toISOString(), - messageCount: 1, - displayName: '7 days old', - firstUserMessage: '7 days', - isCurrentSession: false, - index: 4, - }, - { - id: 'session12d', - file: `${SESSION_FILE_PREFIX}12d`, - fileName: `${SESSION_FILE_PREFIX}12d.json`, - startTime: twelveDaysAgo.toISOString(), - lastUpdated: twelveDaysAgo.toISOString(), - messageCount: 1, - displayName: '12 days old', - firstUserMessage: '12 days', - isCurrentSession: false, - index: 5, - }, - ]; + await writeSessionFile({ + id: 'recent3', + fileName: 'session-20250117-recent3.json', + lastUpdated: threeDaysAgo.toISOString(), + }); + await writeArtifacts('recent3'); + await writeSessionFile({ + id: 'recent5', + fileName: 'session-20250115-recent5.json', + lastUpdated: fiveDaysAgo.toISOString(), + }); + await writeArtifacts('recent5'); - mockGetAllSessionFiles.mockResolvedValue( - testSessions.map((session) => ({ - fileName: session.fileName, - sessionInfo: session, - })), - ); - - // Mock successful file operations - mockFs.access.mockResolvedValue(undefined); - mockFs.readFile.mockResolvedValue( - JSON.stringify({ - sessionId: 'test', - messages: [], - startTime: '2025-01-01T00:00:00Z', - lastUpdated: '2025-01-01T00:00:00Z', - }), - ); - mockFs.unlink.mockResolvedValue(undefined); - - const result = await cleanupExpiredSessions(config, settings); - - // Should delete: - // - session12d (exceeds maxAge of 10d) - // - session7d and session5d (exceed maxCount of 2, keeping current + 3d) - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(5); - expect(result.deleted).toBe(3); - expect(result.skipped).toBe(2); // Current + 3d session - - // Verify which files were deleted - const unlinkCalls = mockFs.unlink.mock.calls.map((call) => call[0]); - expect(unlinkCalls).toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}5d.json`, - ), - ); - expect(unlinkCalls).toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}7d.json`, - ), - ); - expect(unlinkCalls).toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}12d.json`, - ), - ); - - // Verify which files were NOT deleted - expect(unlinkCalls).not.toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}current.json`, - ), - ); - expect(unlinkCalls).not.toContain( - path.join( - '/tmp/test-project', - 'chats', - `${SESSION_FILE_PREFIX}3d.json`, - ), - ); - }); - - it('should delete the session-specific directory', async () => { const config = createMockConfig(); const settings: Settings = { general: { sessionRetention: { enabled: true, - maxAge: '1d', // Very short retention to trigger deletion of all but current + maxCount: 3, // Keep current + 2 most recent (which should be 3d and 5d) }, }, }; - // Mock successful file operations - mockFs.access.mockResolvedValue(undefined); - mockFs.unlink.mockResolvedValue(undefined); - mockFs.rm.mockResolvedValue(undefined); + const result = await cleanupExpiredSessions(config, settings); - await cleanupExpiredSessions(config, settings); + expect(result.scanned).toBe(5); + expect(result.deleted).toBe(2); // Should only delete the 14d and 30d old sessions + expect(result.skipped).toBe(3); + expect(result.failed).toBe(0); - // Verify that fs.rm was called with the session directory for the deleted session that has sessionInfo - // recent456 should be deleted and its directory removed - expect(mockFs.rm).toHaveBeenCalledWith( - path.join('/tmp/test-project', 'recent456'), - expect.objectContaining({ recursive: true, force: true }), + // Verify specifically WHICH files survived + expect(existsSync(path.join(chatsDir, sessions[0].fileName))).toBe(true); // current + expect( + existsSync(path.join(chatsDir, 'session-20250117-recent3.json')), + ).toBe(true); // 3d + expect( + existsSync(path.join(chatsDir, 'session-20250115-recent5.json')), + ).toBe(true); // 5d + + // Verify the older ones were deleted + expect(existsSync(path.join(chatsDir, sessions[1].fileName))).toBe(false); // 14d + expect(existsSync(path.join(chatsDir, sessions[2].fileName))).toBe(false); // 30d + }); + + it('should delete subagent files sharing the same shortId', async () => { + const now = new Date(); + const twoWeeksAgo = new Date(now.getTime() - 14 * 24 * 60 * 60 * 1000); + + // Parent session (expired) + await writeSessionFile({ + id: 'parent-uuid', + fileName: 'session-20250110-abc12345.json', + lastUpdated: twoWeeksAgo.toISOString(), + }); + await writeArtifacts('parent-uuid'); + + // Subagent session (different UUID, same shortId) + await writeSessionFile({ + id: 'sub-uuid', + fileName: 'session-20250110-subagent-abc12345.json', + lastUpdated: twoWeeksAgo.toISOString(), + }); + await writeArtifacts('sub-uuid'); + + const config = createMockConfig(); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '10d' } }, + }; + + const result = await cleanupExpiredSessions(config, settings); + + expect(result.deleted).toBe(2); // Both files should be deleted + expect( + existsSync(path.join(chatsDir, 'session-20250110-abc12345.json')), + ).toBe(false); + expect( + existsSync( + path.join(chatsDir, 'session-20250110-subagent-abc12345.json'), + ), + ).toBe(false); + + // Artifacts for both should be gone + expect(existsSync(path.join(logsDir, 'session-parent-uuid.jsonl'))).toBe( + false, ); + expect(existsSync(path.join(logsDir, 'session-sub-uuid.jsonl'))).toBe( + false, + ); + }); + + it('should delete corrupted session files', async () => { + // Write a corrupted file (invalid JSON) + const corruptPath = path.join(chatsDir, 'session-corrupt.json'); + await fs.writeFile(corruptPath, 'invalid json'); + + const config = createMockConfig(); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '10d' } }, + }; + + const result = await cleanupExpiredSessions(config, settings); + + expect(result.deleted).toBe(1); + expect(existsSync(corruptPath)).toBe(false); + }); + + it('should safely delete 8-character sessions containing invalid JSON', async () => { + const config = createMockConfig(); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '1d' } }, + }; + + const badJsonPath = path.join(chatsDir, 'session-20241225-badjson1.json'); + await fs.writeFile(badJsonPath, 'This is raw text, not JSON'); + + const result = await cleanupExpiredSessions(config, settings); + + expect(result.deleted).toBe(1); + expect(result.failed).toBe(0); + expect(existsSync(badJsonPath)).toBe(false); + }); + + it('should safely delete legacy non-8-character sessions', async () => { + const config = createMockConfig(); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '1d' } }, + }; + + const legacyPath = path.join(chatsDir, 'session-20241225-legacy.json'); + // Create valid JSON so the parser succeeds, but shortId derivation fails + await fs.writeFile( + legacyPath, + JSON.stringify({ + sessionId: 'legacy-session-id', + lastUpdated: '2024-12-25T00:00:00.000Z', + messages: [], + }), + ); + + const result = await cleanupExpiredSessions(config, settings); + + expect(result.deleted).toBe(1); + expect(result.failed).toBe(0); + expect(existsSync(legacyPath)).toBe(false); + }); + + it('should silently ignore ENOENT if file is already deleted before unlink', async () => { + await seedSessions(); // Seeds older 2024 and 2025 sessions + const targetFile = path.join(chatsDir, 'session-20241225-ancient1.json'); + let getSessionIdCalls = 0; + + const config = createMockConfig({ + getSessionId: () => { + getSessionIdCalls++; + // First call is for `getAllSessionFiles`. + // Subsequent calls are right before `fs.unlink`! + if (getSessionIdCalls > 1) { + try { + unlinkSync(targetFile); + } catch { + /* ignore */ + } + } + return 'mock-session-id'; + }, + }); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '1d' } }, + }; + + const result = await cleanupExpiredSessions(config, settings); + + // `failed` should not increment because ENOENT is silently swallowed + expect(result.failed).toBe(0); + }); + + it('should respect minRetention configuration', async () => { + await seedSessions(); + const config = createMockConfig(); + const settings: Settings = { + general: { + sessionRetention: { + enabled: true, + maxAge: '12h', // Less than 1 day minRetention + minRetention: '1d', + }, + }, + }; + + const result = await cleanupExpiredSessions(config, settings); + + // Should return early and not delete anything + expect(result.disabled).toBe(true); + expect(result.deleted).toBe(0); + }); + + it('should handle combined maxAge and maxCount (most restrictive wins)', async () => { + const sessions = await seedSessions(); // [current, 14d, 30d] + + // Seed 3d and 5d to mirror the granular sorting test + const now = new Date(); + const threeDaysAgo = new Date(now.getTime() - 3 * 24 * 60 * 60 * 1000); + const fiveDaysAgo = new Date(now.getTime() - 5 * 24 * 60 * 60 * 1000); + + await writeSessionFile({ + id: 'recent3', + fileName: 'session-20250117-recent3.json', + lastUpdated: threeDaysAgo.toISOString(), + }); + await writeArtifacts('recent3'); + await writeSessionFile({ + id: 'recent5', + fileName: 'session-20250115-recent5.json', + lastUpdated: fiveDaysAgo.toISOString(), + }); + await writeArtifacts('recent5'); + + const config = createMockConfig(); + const settings: Settings = { + general: { + sessionRetention: { + enabled: true, + // 20d deletes 30d. + // maxCount: 2 keeps current and 3d. + // Restrictive wins: 30d deleted by maxAge. 14d, 5d deleted by maxCount. + maxAge: '20d', + maxCount: 2, + }, + }, + }; + + const result = await cleanupExpiredSessions(config, settings); + + expect(result.scanned).toBe(5); + expect(result.deleted).toBe(3); // deletes 5d, 14d, 30d + expect(result.skipped).toBe(2); // keeps current, 3d + expect(result.failed).toBe(0); + + // Assert kept + expect(existsSync(path.join(chatsDir, sessions[0].fileName))).toBe(true); // current + expect( + existsSync(path.join(chatsDir, 'session-20250117-recent3.json')), + ).toBe(true); // 3d + + // Assert deleted + expect( + existsSync(path.join(chatsDir, 'session-20250115-recent5.json')), + ).toBe(false); // 5d + expect(existsSync(path.join(chatsDir, sessions[1].fileName))).toBe(false); // 14d + expect(existsSync(path.join(chatsDir, sessions[2].fileName))).toBe(false); // 30d + }); + + it('should handle empty sessions directory', async () => { + const config = createMockConfig(); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '30d' } }, + }; + const result = await cleanupExpiredSessions(config, settings); + expect(result.disabled).toBe(false); + expect(result.scanned).toBe(0); + expect(result.deleted).toBe(0); + expect(result.skipped).toBe(0); + expect(result.failed).toBe(0); }); }); - describe('parseRetentionPeriod format validation', () => { - // Test all supported formats + describe('Error handling & resilience', () => { + it.skipIf(process.platform === 'win32')( + 'should handle file system errors gracefully (e.g., EACCES)', + async () => { + const sessions = await seedSessions(); + const config = createMockConfig(); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '1d' } }, + }; + + // Make one of the files read-only and its parent directory read-only to simulate EACCES during unlink + const targetFile = path.join(chatsDir, sessions[1].fileName); + await fs.chmod(targetFile, 0o444); + // Wait we want unlink to fail, so we make the directory read-only temporarily + await fs.chmod(chatsDir, 0o555); + + try { + const result = await cleanupExpiredSessions(config, settings); + + // It shouldn't crash + expect(result.disabled).toBe(false); + // It should have tried and failed to delete the old session + expect(result.failed).toBeGreaterThan(0); + } finally { + // Restore permissions so cleanup can proceed in afterEach + await fs.chmod(chatsDir, 0o777); + await fs.chmod(targetFile, 0o666); + } + }, + ); + + it.skipIf(process.platform === 'win32')( + 'should handle global read errors gracefully', + async () => { + const config = createMockConfig(); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '1d' } }, + }; + + // Make the chats directory unreadable + await fs.chmod(chatsDir, 0o000); + + try { + const result = await cleanupExpiredSessions(config, settings); + + // It shouldn't crash, but it should fail + expect(result.disabled).toBe(false); + expect(result.failed).toBe(1); + expect(debugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('Session cleanup failed'), + ); + } finally { + await fs.chmod(chatsDir, 0o777); + } + }, + ); + + it('should NOT delete tempDir if safeSessionId is empty', async () => { + const config = createMockConfig(); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '1d' } }, + }; + + const sessions = await seedSessions(); + const targetFile = path.join(chatsDir, sessions[1].fileName); + + // Write a session ID that sanitizeFilenamePart will turn into an empty string "" + await fs.writeFile(targetFile, JSON.stringify({ sessionId: '../../..' })); + + const tempDir = config.storage.getProjectTempDir(); + expect(existsSync(tempDir)).toBe(true); + + await cleanupExpiredSessions(config, settings); + + // It must NOT delete the tempDir root + expect(existsSync(tempDir)).toBe(true); + }); + + it('should handle unexpected errors without throwing (e.g. string errors)', async () => { + await seedSessions(); + const config = createMockConfig({ + getSessionId: () => { + const stringError = 'String error' as unknown as Error; + throw stringError; // Throw a non-Error string without triggering no-restricted-syntax + }, + }); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxCount: 1 } }, + }; + + const result = await cleanupExpiredSessions(config, settings); + + expect(result.disabled).toBe(false); + expect(result.failed).toBeGreaterThan(0); + }); + + it('should never run on the current session', async () => { + await seedSessions(); + const config = createMockConfig(); + const settings: Settings = { + general: { + sessionRetention: { + enabled: true, + maxCount: 1, // Keep only 1 session (which will be the current one) + }, + }, + }; + + const result = await cleanupExpiredSessions(config, settings); + + expect(result.deleted).toBe(2); + expect(result.skipped).toBe(1); // The current session + const currentSessionFile = (await fs.readdir(chatsDir)).find((f) => + f.includes('current1'), + ); + expect(currentSessionFile).toBeDefined(); + }); + }); + + describe('Format parsing & validation', () => { + // Valid formats it.each([ - ['1h', 60 * 60 * 1000], - ['24h', 24 * 60 * 60 * 1000], - ['168h', 168 * 60 * 60 * 1000], - ['1d', 24 * 60 * 60 * 1000], - ['7d', 7 * 24 * 60 * 60 * 1000], - ['30d', 30 * 24 * 60 * 60 * 1000], - ['365d', 365 * 24 * 60 * 60 * 1000], - ['1w', 7 * 24 * 60 * 60 * 1000], - ['2w', 14 * 24 * 60 * 60 * 1000], - ['4w', 28 * 24 * 60 * 60 * 1000], - ['52w', 364 * 24 * 60 * 60 * 1000], - ['1m', 30 * 24 * 60 * 60 * 1000], - ['3m', 90 * 24 * 60 * 60 * 1000], - ['6m', 180 * 24 * 60 * 60 * 1000], - ['12m', 360 * 24 * 60 * 60 * 1000], - ])('should correctly parse valid format %s', async (input) => { + ['1h'], + ['24h'], + ['168h'], + ['1d'], + ['7d'], + ['30d'], + ['365d'], + ['1w'], + ['2w'], + ['4w'], + ['52w'], + ['1m'], + ['3m'], + ['12m'], + ['9999d'], + ])('should accept valid maxAge format %s', async (input) => { const config = createMockConfig(); const settings: Settings = { general: { sessionRetention: { enabled: true, maxAge: input, - // Set minRetention to 1h to allow testing of hour-based maxAge values minRetention: '1h', }, }, }; - mockGetAllSessionFiles.mockResolvedValue([]); - - // If it parses correctly, cleanup should proceed without error const result = await cleanupExpiredSessions(config, settings); expect(result.disabled).toBe(false); expect(result.failed).toBe(0); }); - // Test invalid formats - it.each([ - '30', // Missing unit - '30x', // Invalid unit - 'd', // No number - '1.5d', // Decimal not supported - '-5d', // Negative number - '1 d', // Space in format - '1dd', // Double unit - 'abc', // Non-numeric - '30s', // Unsupported unit (seconds) - '30y', // Unsupported unit (years) - '0d', // Zero value (technically valid regex but semantically invalid) - ])('should reject invalid format %s', async (input) => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); + it('should accept maxAge equal to minRetention', async () => { + const config = createMockConfig(); const settings: Settings = { general: { - sessionRetention: { - enabled: true, - maxAge: input, - }, + sessionRetention: { enabled: true, maxAge: '1d', minRetention: '1d' }, }, }; - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining( - input === '0d' - ? 'Invalid retention period: 0d. Value must be greater than 0' - : `Invalid retention period format: ${input}`, - ), - ); + expect(result.disabled).toBe(false); }); - // Test special case - empty string - it('should reject empty string', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); + it('should accept maxCount = 1000 (maximum valid)', async () => { + const config = createMockConfig(); const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '', - }, - }, + general: { sessionRetention: { enabled: true, maxCount: 1000 } }, }; - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - // Empty string means no valid retention method specified - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('Either maxAge or maxCount must be specified'), - ); + expect(result.disabled).toBe(false); }); - // Test edge cases - it('should handle very large numbers', async () => { + it('should reject maxAge less than default minRetention (1d)', async () => { + await seedSessions(); const config = createMockConfig(); const settings: Settings = { general: { sessionRetention: { enabled: true, - maxAge: '9999d', // Very large number + maxAge: '12h', + // Note: No minRetention provided here, should default to 1d }, }, }; - mockGetAllSessionFiles.mockResolvedValue([]); + const result = await cleanupExpiredSessions(config, settings); + + expect(result.disabled).toBe(true); + expect(debugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('maxAge cannot be less than minRetention'), + ); + }); + + it('should reject maxAge less than custom minRetention', async () => { + const config = createMockConfig(); + const settings: Settings = { + general: { + sessionRetention: { + enabled: true, + maxAge: '2d', + minRetention: '3d', // maxAge < minRetention + }, + }, + }; const result = await cleanupExpiredSessions(config, settings); - expect(result.disabled).toBe(false); - expect(result.failed).toBe(0); + expect(result.disabled).toBe(true); + expect(debugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('maxAge cannot be less than minRetention (3d)'), + ); + }); + + it('should reject zero value with a specific error message', async () => { + const config = createMockConfig(); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '0d' } }, + }; + + const result = await cleanupExpiredSessions(config, settings); + expect(result.disabled).toBe(true); + expect(debugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('Value must be greater than 0'), + ); + }); + + // Invalid formats + it.each([ + ['30'], + ['30x'], + ['d'], + ['1.5d'], + ['-5d'], + ['1 d'], + ['1dd'], + ['abc'], + ['30s'], + ['30y'], + ])('should reject invalid maxAge format %s', async (input) => { + const config = createMockConfig(); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: input } }, + }; + + const result = await cleanupExpiredSessions(config, settings); + expect(result.disabled).toBe(true); + expect(debugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining(`Invalid retention period format: ${input}`), + ); + }); + + it('should reject empty string for maxAge', async () => { + const config = createMockConfig(); + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '' } }, + }; + + const result = await cleanupExpiredSessions(config, settings); + expect(result.disabled).toBe(true); + expect(debugLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('Either maxAge or maxCount must be specified'), + ); }); it('should validate minRetention format', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); + const config = createMockConfig(); const settings: Settings = { general: { sessionRetention: { enabled: true, maxAge: '5d', - minRetention: 'invalid-format', // Invalid minRetention + minRetention: 'invalid-format', }, }, }; - mockGetAllSessionFiles.mockResolvedValue([]); - // Should fall back to default minRetention and proceed const result = await cleanupExpiredSessions(config, settings); - - // Since maxAge (5d) > default minRetention (1d), this should succeed expect(result.disabled).toBe(false); - expect(result.failed).toBe(0); }); }); - describe('Configuration validation', () => { - it('should require either maxAge or maxCount', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - // Neither maxAge nor maxCount specified - }, - }, - }; + describe('Tool Output Cleanup', () => { + let toolOutputDir: string; - const result = await cleanupExpiredSessions(config, settings); + beforeEach(async () => { + toolOutputDir = path.join(testTempDir, TOOL_OUTPUTS_DIR); + await fs.mkdir(toolOutputDir, { recursive: true }); + }); + + async function seedToolOutputs() { + const now = new Date(); + const oldTime = new Date(now.getTime() - 10 * 24 * 60 * 60 * 1000); // 10 days ago + + const file1 = path.join(toolOutputDir, 'output1.json'); + await fs.writeFile(file1, '{}'); + + const file2 = path.join(toolOutputDir, 'output2.json'); + await fs.writeFile(file2, '{}'); + + // Manually backdate file1 + await fs.utimes(file1, oldTime, oldTime); + + // Create an old session subdirectory + const oldSubdir = path.join(toolOutputDir, 'session-old'); + await fs.mkdir(oldSubdir); + await fs.utimes(oldSubdir, oldTime, oldTime); + + return { file1, file2, oldSubdir }; + } + + it('should return early if cleanup is disabled', async () => { + const settings: Settings = { + general: { sessionRetention: { enabled: false } }, + }; + const result = await cleanupToolOutputFiles(settings, false, testTempDir); expect(result.disabled).toBe(true); expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('Either maxAge or maxCount must be specified'), - ); + expect(result.deleted).toBe(0); }); - it('should validate maxCount range', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); + it('should gracefully handle missing tool-outputs directory', async () => { + await fs.rm(toolOutputDir, { recursive: true, force: true }); const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxCount: 0, // Invalid count - }, - }, + general: { sessionRetention: { enabled: true, maxAge: '1d' } }, }; - const result = await cleanupExpiredSessions(config, settings); + const result = await cleanupToolOutputFiles(settings, false, testTempDir); - expect(result.disabled).toBe(true); + expect(result.disabled).toBe(false); expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('maxCount must be at least 1'), - ); }); - describe('maxAge format validation', () => { - it('should reject invalid maxAge format - no unit', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '30', // Missing unit - }, - }, - }; - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('Invalid retention period format: 30'), - ); - }); - it('should reject invalid maxAge format - invalid unit', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '30x', // Invalid unit 'x' - }, - }, - }; - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('Invalid retention period format: 30x'), - ); - }); - it('should reject invalid maxAge format - no number', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: 'd', // No number - }, - }, - }; - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('Invalid retention period format: d'), - ); - }); - it('should reject invalid maxAge format - decimal number', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '1.5d', // Decimal not supported - }, - }, - }; - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('Invalid retention period format: 1.5d'), - ); - }); - it('should reject invalid maxAge format - negative number', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '-5d', // Negative not allowed - }, - }, - }; - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('Invalid retention period format: -5d'), - ); - }); - it('should accept valid maxAge format - hours', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '48h', // Valid: 48 hours - maxCount: 10, // Need at least one valid retention method - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - const result = await cleanupExpiredSessions(config, settings); - - // Should not reject the configuration - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.failed).toBe(0); - }); - - it('should accept valid maxAge format - days', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '7d', // Valid: 7 days - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - const result = await cleanupExpiredSessions(config, settings); - - // Should not reject the configuration - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.failed).toBe(0); - }); - - it('should accept valid maxAge format - weeks', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '2w', // Valid: 2 weeks - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - const result = await cleanupExpiredSessions(config, settings); - - // Should not reject the configuration - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.failed).toBe(0); - }); - - it('should accept valid maxAge format - months', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '3m', // Valid: 3 months - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - const result = await cleanupExpiredSessions(config, settings); - - // Should not reject the configuration - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.failed).toBe(0); - }); - }); - - describe('minRetention validation', () => { - it('should reject maxAge less than default minRetention (1d)', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '12h', // Less than default 1d minRetention - }, - }, - }; - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining( - 'maxAge cannot be less than minRetention (1d)', - ), - ); - }); - it('should reject maxAge less than custom minRetention', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '2d', - minRetention: '3d', // maxAge < minRetention - }, - }, - }; - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining( - 'maxAge cannot be less than minRetention (3d)', - ), - ); - }); - it('should accept maxAge equal to minRetention', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '2d', - minRetention: '2d', // maxAge == minRetention (edge case) - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - const result = await cleanupExpiredSessions(config, settings); - - // Should not reject the configuration - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.failed).toBe(0); - }); - - it('should accept maxAge greater than minRetention', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '7d', - minRetention: '2d', // maxAge > minRetention - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - const result = await cleanupExpiredSessions(config, settings); - - // Should not reject the configuration - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.failed).toBe(0); - }); - - it('should handle invalid minRetention format gracefully', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '5d', - minRetention: 'invalid', // Invalid format - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - // When minRetention is invalid, it should default to 1d - // Since maxAge (5d) > default minRetention (1d), this should be valid - const result = await cleanupExpiredSessions(config, settings); - - // Should not reject due to minRetention (falls back to default) - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.failed).toBe(0); - }); - }); - - describe('maxCount boundary validation', () => { - it('should accept maxCount = 1 (minimum valid)', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxCount: 1, // Minimum valid value - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - const result = await cleanupExpiredSessions(config, settings); - - // Should accept the configuration - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.failed).toBe(0); - }); - - it('should accept maxCount = 1000 (maximum valid)', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxCount: 1000, // Maximum valid value - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - const result = await cleanupExpiredSessions(config, settings); - - // Should accept the configuration - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.failed).toBe(0); - }); - - it('should reject negative maxCount', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxCount: -1, // Negative value - }, - }, - }; - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('maxCount must be at least 1'), - ); - }); - it('should accept valid maxCount in normal range', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxCount: 50, // Normal valid value - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - const result = await cleanupExpiredSessions(config, settings); - - // Should accept the configuration - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.failed).toBe(0); - }); - }); - - describe('combined configuration validation', () => { - it('should accept valid maxAge and maxCount together', async () => { - const config = createMockConfig(); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '30d', - maxCount: 10, - }, - }, - }; - - mockGetAllSessionFiles.mockResolvedValue([]); - - const result = await cleanupExpiredSessions(config, settings); - - // Should accept the configuration - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(0); - expect(result.failed).toBe(0); - }); - - it('should reject if both maxAge and maxCount are invalid', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: 'invalid', - maxCount: 0, - }, - }, - }; - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - // Should fail on first validation error (maxAge format) - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('Invalid retention period format'), - ); - }); - it('should reject if maxAge is invalid even when maxCount is valid', async () => { - const config = createMockConfig({ - getDebugMode: vi.fn().mockReturnValue(true), - }); - const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: 'invalid', // Invalid format - maxCount: 5, // Valid count - }, - }, - }; - - // The validation logic rejects invalid maxAge format even if maxCount is valid - const result = await cleanupExpiredSessions(config, settings); - - // Should reject due to invalid maxAge format - expect(result.disabled).toBe(true); - expect(result.scanned).toBe(0); - expect(debugLogger.warn).toHaveBeenCalledWith( - expect.stringContaining('Invalid retention period format'), - ); - }); - }); - - it('should never throw an exception, always returning a result', async () => { - const config = createMockConfig(); + it('should delete flat files and subdirectories based on maxAge', async () => { + const { file1, file2, oldSubdir } = await seedToolOutputs(); const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '7d', - }, - }, + general: { sessionRetention: { enabled: true, maxAge: '5d' } }, }; - // Mock getSessionFiles to throw an error - mockGetAllSessionFiles.mockRejectedValue( - new Error('Failed to read directory'), - ); + const result = await cleanupToolOutputFiles(settings, false, testTempDir); - // Should not throw, should return a result with errors - const result = await cleanupExpiredSessions(config, settings); - - expect(result).toBeDefined(); - expect(result.disabled).toBe(false); - expect(result.failed).toBe(1); + // file1 and oldSubdir should be deleted. + expect(result.deleted).toBe(2); + expect(existsSync(file1)).toBe(false); + expect(existsSync(oldSubdir)).toBe(false); + expect(existsSync(file2)).toBe(true); }); - it('should delete corrupted session files', async () => { - const config = createMockConfig(); + it('should delete oldest-first flat files based on maxCount when maxAge does not hit', async () => { + const { file1, file2 } = await seedToolOutputs(); const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '30d', - }, - }, + general: { sessionRetention: { enabled: true, maxCount: 1 } }, }; - // Mock getAllSessionFiles to return both valid and corrupted files - const validSession = createTestSessions()[0]; - mockGetAllSessionFiles.mockResolvedValue([ - { fileName: validSession.fileName, sessionInfo: validSession }, - { - fileName: `${SESSION_FILE_PREFIX}2025-01-02T10-00-00-corrupt1.json`, - sessionInfo: null, - }, - { - fileName: `${SESSION_FILE_PREFIX}2025-01-03T10-00-00-corrupt2.json`, - sessionInfo: null, - }, - ]); + const result = await cleanupToolOutputFiles(settings, false, testTempDir); - mockFs.unlink.mockResolvedValue(undefined); - - const result = await cleanupExpiredSessions(config, settings); - - expect(result.disabled).toBe(false); - expect(result.scanned).toBe(3); // 1 valid + 2 corrupted - expect(result.deleted).toBe(2); // Should delete the 2 corrupted files - expect(result.skipped).toBe(1); // The valid session is kept - - // Verify corrupted files were deleted - expect(mockFs.unlink).toHaveBeenCalledWith( - expect.stringContaining('corrupt1.json'), - ); - expect(mockFs.unlink).toHaveBeenCalledWith( - expect.stringContaining('corrupt2.json'), - ); + // Excess is 1. Oldest is file1. So file1 is deleted. + expect(result.deleted).toBe(1); + expect(existsSync(file1)).toBe(false); + expect(existsSync(file2)).toBe(true); }); - it('should handle unexpected errors without throwing', async () => { - const config = createMockConfig(); + it('should skip tool-output subdirectories with unsafe names', async () => { const settings: Settings = { - general: { - sessionRetention: { - enabled: true, - maxAge: '7d', - }, - }, + general: { sessionRetention: { enabled: true, maxAge: '1d' } }, }; - // Mock getSessionFiles to throw a non-Error object - mockGetAllSessionFiles.mockRejectedValue('String error'); + // Create a directory with a name that is semantically unsafe for sanitization rules + const unsafeSubdir = path.join(toolOutputDir, 'session-unsafe@name'); + await fs.mkdir(unsafeSubdir); - // Should not throw, should return a result with errors - const result = await cleanupExpiredSessions(config, settings); + // Backdate it so it WOULD be deleted if it were safely named + const oldTime = new Date(Date.now() - 10 * 24 * 60 * 60 * 1000); + await fs.utimes(unsafeSubdir, oldTime, oldTime); - expect(result).toBeDefined(); - expect(result.disabled).toBe(false); - expect(result.failed).toBe(1); + const result = await cleanupToolOutputFiles(settings, false, testTempDir); + + // Must be scanned but actively skipped from deletion due to sanitization mismatch + expect(result.deleted).toBe(0); + expect(existsSync(unsafeSubdir)).toBe(true); + }); + + it('should initialize Storage when projectTempDir is not explicitly provided', async () => { + const getProjectTempDirSpy = vi + .spyOn(Storage.prototype, 'getProjectTempDir') + .mockReturnValue(testTempDir); + const initializeSpy = vi + .spyOn(Storage.prototype, 'initialize') + .mockResolvedValue(undefined); + + const settings: Settings = { + general: { sessionRetention: { enabled: true, maxAge: '1d' } }, + }; + const { oldSubdir } = await seedToolOutputs(); + + // Call explicitly without third parameter + const result = await cleanupToolOutputFiles(settings, false); + + expect(initializeSpy).toHaveBeenCalled(); + expect(result.deleted).toBeGreaterThan(0); + expect(existsSync(oldSubdir)).toBe(false); + + getProjectTempDirSpy.mockRestore(); + initializeSpy.mockRestore(); }); }); }); diff --git a/packages/cli/src/utils/sessionCleanup.ts b/packages/cli/src/utils/sessionCleanup.ts index 57f2fdd189..5ed4547604 100644 --- a/packages/cli/src/utils/sessionCleanup.ts +++ b/packages/cli/src/utils/sessionCleanup.ts @@ -9,6 +9,7 @@ import * as path from 'node:path'; import { debugLogger, sanitizeFilenamePart, + SESSION_FILE_PREFIX, Storage, TOOL_OUTPUTS_DIR, type Config, @@ -26,6 +27,12 @@ const MULTIPLIERS = { m: 30 * 24 * 60 * 60 * 1000, // months (30 days) to ms }; +/** + * Matches a trailing hyphen followed by exactly 8 alphanumeric characters before the .json extension. + * Example: session-20250110-abcdef12.json -> captures "abcdef12" + */ +const SHORT_ID_REGEX = /-([a-zA-Z0-9]{8})\.json$/; + /** * Result of session cleanup operation */ @@ -37,6 +44,65 @@ export interface CleanupResult { failed: number; } +/** + * Helpers for session cleanup. + */ + +/** + * Derives an 8-character shortId from a session filename. + */ +function deriveShortIdFromFileName(fileName: string): string | null { + if (fileName.startsWith(SESSION_FILE_PREFIX) && fileName.endsWith('.json')) { + const match = fileName.match(SHORT_ID_REGEX); + return match ? match[1] : null; + } + return null; +} + +/** + * Gets the log path for a session ID. + */ +function getSessionLogPath(tempDir: string, safeSessionId: string): string { + return path.join(tempDir, 'logs', `session-${safeSessionId}.jsonl`); +} + +/** + * Cleans up associated artifacts (logs, tool-outputs, directory) for a session. + */ +async function deleteSessionArtifactsAsync( + sessionId: string, + config: Config, +): Promise { + const tempDir = config.storage.getProjectTempDir(); + + // Cleanup logs + const logsDir = path.join(tempDir, 'logs'); + const safeSessionId = sanitizeFilenamePart(sessionId); + const logPath = getSessionLogPath(tempDir, safeSessionId); + if (logPath.startsWith(logsDir)) { + await fs.unlink(logPath).catch(() => {}); + } + + // Cleanup tool outputs + const toolOutputDir = path.join( + tempDir, + TOOL_OUTPUTS_DIR, + `session-${safeSessionId}`, + ); + const toolOutputsBase = path.join(tempDir, TOOL_OUTPUTS_DIR); + if (toolOutputDir.startsWith(toolOutputsBase)) { + await fs + .rm(toolOutputDir, { recursive: true, force: true }) + .catch(() => {}); + } + + // Cleanup session directory + const sessionDir = path.join(tempDir, safeSessionId); + if (safeSessionId && sessionDir.startsWith(tempDir + path.sep)) { + await fs.rm(sessionDir, { recursive: true, force: true }).catch(() => {}); + } +} + /** * Main entry point for session cleanup during CLI startup */ @@ -72,7 +138,6 @@ export async function cleanupExpiredSessions( return { ...result, disabled: true }; } - // Get all session files (including corrupted ones) for this project const allFiles = await getAllSessionFiles(chatsDir, config.getSessionId()); result.scanned = allFiles.length; @@ -86,78 +151,110 @@ export async function cleanupExpiredSessions( retentionConfig, ); + const processedShortIds = new Set(); + // Delete all sessions that need to be deleted for (const sessionToDelete of sessionsToDelete) { try { - const sessionPath = path.join(chatsDir, sessionToDelete.fileName); - await fs.unlink(sessionPath); + const shortId = deriveShortIdFromFileName(sessionToDelete.fileName); - // ALSO cleanup Activity logs in the project logs directory - const sessionId = sessionToDelete.sessionInfo?.id; - if (sessionId) { - const logsDir = path.join(config.storage.getProjectTempDir(), 'logs'); - const logPath = path.join(logsDir, `session-${sessionId}.jsonl`); - try { - await fs.unlink(logPath); - } catch { - /* ignore if log doesn't exist */ + if (shortId) { + if (processedShortIds.has(shortId)) { + continue; } + processedShortIds.add(shortId); - // ALSO cleanup tool outputs for this session - const safeSessionId = sanitizeFilenamePart(sessionId); - const toolOutputDir = path.join( - config.storage.getProjectTempDir(), - TOOL_OUTPUTS_DIR, - `session-${safeSessionId}`, - ); - try { - await fs.rm(toolOutputDir, { recursive: true, force: true }); - } catch { - /* ignore if doesn't exist */ - } - - // ALSO cleanup the session-specific directory (contains plans, tasks, etc.) - const sessionDir = path.join( - config.storage.getProjectTempDir(), - sessionId, - ); - try { - await fs.rm(sessionDir, { recursive: true, force: true }); - } catch { - /* ignore if doesn't exist */ - } - } - - if (config.getDebugMode()) { - if (sessionToDelete.sessionInfo === null) { - debugLogger.debug( - `Deleted corrupted session file: ${sessionToDelete.fileName}`, + const matchingFiles = allFiles + .map((f) => f.fileName) + .filter( + (f) => + f.startsWith(SESSION_FILE_PREFIX) && + f.endsWith(`-${shortId}.json`), ); - } else { + + for (const file of matchingFiles) { + const filePath = path.join(chatsDir, file); + let fullSessionId: string | undefined; + + try { + // Try to read file to get full sessionId + try { + const fileContent = await fs.readFile(filePath, 'utf8'); + const content: unknown = JSON.parse(fileContent); + if ( + content && + typeof content === 'object' && + 'sessionId' in content + ) { + const record = content as Record; + const id = record['sessionId']; + if (typeof id === 'string') { + fullSessionId = id; + } + } + } catch { + // If read/parse fails, skip getting sessionId, just delete the file + } + + // Delete the session file + if (!fullSessionId || fullSessionId !== config.getSessionId()) { + await fs.unlink(filePath); + + if (fullSessionId) { + await deleteSessionArtifactsAsync(fullSessionId, config); + } + result.deleted++; + } else { + result.skipped++; + } + } catch (error) { + // Ignore ENOENT (file already deleted) + if ( + error instanceof Error && + 'code' in error && + error.code === 'ENOENT' + ) { + // File already deleted, do nothing. + } else { + debugLogger.warn( + `Failed to delete matching file ${file}: ${error instanceof Error ? error.message : 'Unknown error'}`, + ); + result.failed++; + } + } + } + } else { + // Fallback to old logic + const sessionPath = path.join(chatsDir, sessionToDelete.fileName); + await fs.unlink(sessionPath); + + const sessionId = sessionToDelete.sessionInfo?.id; + if (sessionId) { + await deleteSessionArtifactsAsync(sessionId, config); + } + + if (config.getDebugMode()) { debugLogger.debug( - `Deleted expired session: ${sessionToDelete.sessionInfo.id} (${sessionToDelete.sessionInfo.lastUpdated})`, + `Deleted fallback session: ${sessionToDelete.fileName}`, ); } + result.deleted++; } - result.deleted++; } catch (error) { - // Ignore ENOENT errors (file already deleted) + // Ignore ENOENT (file already deleted) if ( error instanceof Error && 'code' in error && error.code === 'ENOENT' ) { - // File already deleted, do nothing. + // File already deleted } else { - // Log error directly to console const sessionId = sessionToDelete.sessionInfo === null ? sessionToDelete.fileName : sessionToDelete.sessionInfo.id; - const errorMessage = - error instanceof Error ? error.message : 'Unknown error'; debugLogger.warn( - `Failed to delete session ${sessionId}: ${errorMessage}`, + `Failed to delete session ${sessionId}: ${error instanceof Error ? error.message : 'Unknown error'}`, ); result.failed++; } @@ -182,9 +279,6 @@ export async function cleanupExpiredSessions( return result; } -/** - * Identifies sessions that should be deleted (corrupted or expired based on retention policy) - */ /** * Identifies sessions that should be deleted (corrupted or expired based on retention policy) */ @@ -248,13 +342,19 @@ export async function identifySessionsToDelete( let shouldDelete = false; // Age-based retention check - if (cutoffDate && new Date(session.lastUpdated) < cutoffDate) { - shouldDelete = true; + if (cutoffDate) { + const lastUpdatedDate = new Date(session.lastUpdated); + const isExpired = lastUpdatedDate < cutoffDate; + if (isExpired) { + shouldDelete = true; + } } // Count-based retention check (keep only N most recent deletable sessions) - if (maxDeletableSessions !== undefined && i >= maxDeletableSessions) { - shouldDelete = true; + if (maxDeletableSessions !== undefined) { + if (i >= maxDeletableSessions) { + shouldDelete = true; + } } if (shouldDelete) {