From 7125d2cd650ee3c8c7a929efcabab2e0e57b10b2 Mon Sep 17 00:00:00 2001 From: Coco Sheng Date: Thu, 30 Apr 2026 16:11:38 -0400 Subject: [PATCH] fix(core): ensure tool output cleanup on session deletion for legacy files (#26263) --- packages/cli/src/utils/sessionCleanup.test.ts | 94 +++++++++++++++++++ packages/cli/src/utils/sessionCleanup.ts | 77 +++++++++++---- .../src/services/chatRecordingService.test.ts | 46 +++++++++ .../core/src/services/chatRecordingService.ts | 61 ++++++++---- 4 files changed, 244 insertions(+), 34 deletions(-) diff --git a/packages/cli/src/utils/sessionCleanup.test.ts b/packages/cli/src/utils/sessionCleanup.test.ts index c1473dc633..d34ac4739d 100644 --- a/packages/cli/src/utils/sessionCleanup.test.ts +++ b/packages/cli/src/utils/sessionCleanup.test.ts @@ -280,6 +280,100 @@ describe('Session Cleanup (Refactored)', () => { expect(existsSync(path.join(chatsDir, sessions[1].id))).toBe(false); // Subagent chats directory should be deleted }); + it('should delete legacy pretty-printed session files and their artifacts', async () => { + const twoWeeksAgo = new Date(Date.now() - 14 * 24 * 60 * 60 * 1000); + const sessionId = 'legacy-uuid'; + const shortId = 'legacy12'; + const fileName = `session-20250110-${shortId}.json`; + const filePath = path.join(chatsDir, fileName); + + // Write pretty-printed JSON + await fs.writeFile( + filePath, + JSON.stringify( + { + sessionId, + lastUpdated: twoWeeksAgo.toISOString(), + startTime: twoWeeksAgo.toISOString(), + messages: [{ type: 'user', content: 'hello legacy' }], + }, + null, + 2, + ), + ); + + await writeArtifacts(sessionId); + + 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(filePath)).toBe(false); + // Artifacts should be gone because we extracted sessionId from legacy JSON + expect( + existsSync(path.join(toolOutputsDir, `session-${sessionId}`)), + ).toBe(false); + }); + + it('should delete expired JSONL session files and their artifacts', async () => { + const twoWeeksAgo = new Date(Date.now() - 14 * 24 * 60 * 60 * 1000); + const sessionId = 'jsonl-uuid'; + const shortId = 'jsonl123'; + const fileName = `session-20250110-${shortId}.jsonl`; + const filePath = path.join(chatsDir, fileName); + + // Write JSONL + const metadata = { + sessionId, + lastUpdated: twoWeeksAgo.toISOString(), + startTime: twoWeeksAgo.toISOString(), + kind: 'main', + }; + const message = { id: '1', type: 'user', content: 'hello jsonl' }; + await fs.writeFile( + filePath, + JSON.stringify(metadata) + '\n' + JSON.stringify(message) + '\n', + ); + + await writeArtifacts(sessionId); + + 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(filePath)).toBe(false); + // Artifacts should be gone because we extracted sessionId from JSONL first line + expect( + existsSync(path.join(toolOutputsDir, `session-${sessionId}`)), + ).toBe(false); + }); + + it('should delete corrupted session files even if sessionId cannot be extracted', async () => { + const shortId = 'corrupt1'; + const fileName = `session-20250110-${shortId}.json`; + const filePath = path.join(chatsDir, fileName); + + await fs.writeFile(filePath, 'completely 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(filePath)).toBe(false); + }); + it('should NOT delete sessions within the cutoff date', async () => { const sessions = await seedSessions(); // [current, 14d, 30d] const config = createMockConfig(); diff --git a/packages/cli/src/utils/sessionCleanup.ts b/packages/cli/src/utils/sessionCleanup.ts index dde926674c..fd79f379fd 100644 --- a/packages/cli/src/utils/sessionCleanup.ts +++ b/packages/cli/src/utils/sessionCleanup.ts @@ -30,10 +30,28 @@ const MULTIPLIERS = { }; /** - * Matches a trailing hyphen followed by exactly 8 alphanumeric characters before the .json extension. + * Matches a trailing hyphen followed by exactly 8 alphanumeric characters before the .json or .jsonl extension. * Example: session-20250110-abcdef12.json -> captures "abcdef12" */ -const SHORT_ID_REGEX = /-([a-zA-Z0-9]{8})\.json$/; +const SHORT_ID_REGEX = /-([a-zA-Z0-9]{8})\.jsonl?$/; + +function hasProperty( + obj: unknown, + prop: T, +): obj is { [key in T]: unknown } { + return obj !== null && typeof obj === 'object' && prop in obj; +} + +function isStringProperty( + obj: unknown, + prop: T, +): obj is { [key in T]: string } { + return hasProperty(obj, prop) && typeof obj[prop] === 'string'; +} + +function isSessionIdRecord(record: unknown): record is { sessionId: string } { + return isStringProperty(record, 'sessionId'); +} /** * Result of session cleanup operation @@ -54,7 +72,10 @@ export interface CleanupResult { * Derives an 8-character shortId from a session filename. */ function deriveShortIdFromFileName(fileName: string): string | null { - if (fileName.startsWith(SESSION_FILE_PREFIX) && fileName.endsWith('.json')) { + if ( + fileName.startsWith(SESSION_FILE_PREFIX) && + (fileName.endsWith('.json') || fileName.endsWith('.jsonl')) + ) { const match = fileName.match(SHORT_ID_REGEX); return match ? match[1] : null; } @@ -141,7 +162,8 @@ export async function cleanupExpiredSessions( .filter( (f) => f.startsWith(SESSION_FILE_PREFIX) && - f.endsWith(`-${shortId}.json`), + (f.endsWith(`-${shortId}.json`) || + f.endsWith(`-${shortId}.jsonl`)), ); for (const file of matchingFiles) { @@ -151,21 +173,44 @@ export async function cleanupExpiredSessions( 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; + const CHUNK_SIZE = 4096; + const buffer = Buffer.alloc(CHUNK_SIZE); + let fd: fs.FileHandle | undefined; + try { + fd = await fs.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'); + const firstLine = + newlineIndex !== -1 + ? contentChunk.substring(0, newlineIndex) + : contentChunk; + + try { + const record: unknown = JSON.parse(firstLine); + if (isSessionIdRecord(record)) { + fullSessionId = record.sessionId; + } + } catch { + // Ignore first line parse error, try full parse for legacy pretty-printed JSON + } + } + } finally { + if (fd !== undefined) { + await fd.close(); + } + } + + if (!fullSessionId) { + const fileContent = await fs.readFile(filePath, 'utf8'); + const content: unknown = JSON.parse(fileContent); + if (isSessionIdRecord(content)) { + fullSessionId = content.sessionId; } } } catch { - // If read/parse fails, skip getting sessionId, just delete the file + // If read/parse fails, skip getting sessionId, just delete the file below } // Delete the session file diff --git a/packages/core/src/services/chatRecordingService.test.ts b/packages/core/src/services/chatRecordingService.test.ts index d6588945e1..7af8380a5a 100644 --- a/packages/core/src/services/chatRecordingService.test.ts +++ b/packages/core/src/services/chatRecordingService.test.ts @@ -602,6 +602,52 @@ describe('ChatRecordingService', () => { expect(fs.existsSync(sessionDir)).toBe(false); }); + it('should delete legacy pretty-printed session files and their artifacts', async () => { + const sessionId = 'legacy-uuid'; + const shortId = 'legacy12'; + const chatsDir = path.join(testTempDir, 'chats'); + const toolOutputsDir = path.join(testTempDir, 'tool-outputs'); + + fs.mkdirSync(chatsDir, { recursive: true }); + fs.mkdirSync(toolOutputsDir, { recursive: true }); + + const sessionFile = path.join( + chatsDir, + `session-2023-01-01T00-00-${shortId}.json`, + ); + // Pretty-printed JSON (not JSONL) + fs.writeFileSync( + sessionFile, + JSON.stringify({ sessionId, messages: [] }, null, 2), + ); + + const toolOutputDir = path.join(toolOutputsDir, `session-${sessionId}`); + fs.mkdirSync(toolOutputDir, { recursive: true }); + fs.writeFileSync(path.join(toolOutputDir, 'output.txt'), 'data'); + + await chatRecordingService.deleteSession(shortId); + + expect(fs.existsSync(sessionFile)).toBe(false); + expect(fs.existsSync(toolOutputDir)).toBe(false); + }); + + it('should delete the session file even if it is corrupted (invalid JSON)', async () => { + const shortId = 'corrupt1'; + const chatsDir = path.join(testTempDir, 'chats'); + + fs.mkdirSync(chatsDir, { recursive: true }); + + const sessionFile = path.join( + chatsDir, + `session-2023-01-01T00-00-${shortId}.jsonl`, + ); + fs.writeFileSync(sessionFile, 'not-json'); + + await chatRecordingService.deleteSession(shortId); + + expect(fs.existsSync(sessionFile)).toBe(false); + }); + it('should delete subagent files and their logs when parent is deleted', async () => { const parentSessionId = '12345678-session-id'; const shortId = '12345678'; diff --git a/packages/core/src/services/chatRecordingService.ts b/packages/core/src/services/chatRecordingService.ts index c7cf7ef95e..e070a1c542 100644 --- a/packages/core/src/services/chatRecordingService.ts +++ b/packages/core/src/services/chatRecordingService.ts @@ -744,6 +744,8 @@ export class ChatRecordingService { tempDir: string, ): Promise { const filePath = path.join(chatsDir, file); + let fullSessionId: string | undefined; + try { const CHUNK_SIZE = 4096; const buffer = Buffer.alloc(CHUNK_SIZE); @@ -752,32 +754,43 @@ export class ChatRecordingService { try { fd = await fs.promises.open(filePath, 'r'); const { bytesRead } = await fd.read(buffer, 0, CHUNK_SIZE, 0); - if (bytesRead === 0) { - await fd.close(); - await fs.promises.unlink(filePath); - return; + 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. + } } - const contentChunk = buffer.toString('utf8', 0, bytesRead); - const newlineIndex = contentChunk.indexOf('\n'); - firstLine = - newlineIndex !== -1 - ? contentChunk.substring(0, newlineIndex) - : contentChunk; } finally { if (fd !== undefined) { await fd.close(); } } - const content = JSON.parse(firstLine) as unknown; - let fullSessionId: string | undefined; - if (isSessionIdRecord(content)) { - fullSessionId = content['sessionId']; + // 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 + } } - // Delete the session file - await fs.promises.unlink(filePath); - if (fullSessionId) { // Delegate to shared utility! await deleteSessionArtifactsAsync(fullSessionId, tempDir); @@ -788,7 +801,19 @@ export class ChatRecordingService { ); } } catch (error) { - debugLogger.error(`Error deleting associated file ${file}:`, 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); + } + } } }