From 96357d210ee1439a32cda95f1e26c290acf7277c Mon Sep 17 00:00:00 2001 From: Raza Rauf Date: Tue, 27 Jan 2026 00:59:08 +0500 Subject: [PATCH] fix: address code review feedback - retry logic and import order - Add retry logic with exponential backoff for stats DB insertions (100ms, 200ms, 400ms delays, max 3 attempts) - Fix import order in types.ts (move constant after imports) - Update stats-listener tests for async retry behavior - Add new test for retry success on transient failure Addresses review recommendations: - High: Stats database error handling with retry logic - Low: Import order consistency --- .../__tests__/stats-listener.test.ts | 115 +++++++++++++----- src/main/process-listeners/stats-listener.ts | 91 +++++++++++--- src/main/process-listeners/types.ts | 14 +-- 3 files changed, 167 insertions(+), 53 deletions(-) diff --git a/src/main/process-listeners/__tests__/stats-listener.test.ts b/src/main/process-listeners/__tests__/stats-listener.test.ts index 41e3836d..ff2a0a30 100644 --- a/src/main/process-listeners/__tests__/stats-listener.test.ts +++ b/src/main/process-listeners/__tests__/stats-listener.test.ts @@ -32,7 +32,7 @@ describe('Stats Listener', () => { mockStatsDB = { isReady: vi.fn(() => true), - insertQueryEvent: vi.fn(() => 1), + insertQueryEvent: vi.fn(() => 'event-id-123'), } as unknown as StatsDB; mockProcessManager = { @@ -52,7 +52,7 @@ describe('Stats Listener', () => { expect(mockProcessManager.on).toHaveBeenCalledWith('query-complete', expect.any(Function)); }); - it('should record query event to stats database when ready', () => { + it('should record query event to stats database when ready', async () => { setupStatsListener(mockProcessManager, { safeSend: mockSafeSend, getStatsDB: () => mockStatsDB, @@ -73,17 +73,20 @@ describe('Stats Listener', () => { handler?.(testSessionId, testQueryData); - expect(mockStatsDB.isReady).toHaveBeenCalled(); - expect(mockStatsDB.insertQueryEvent).toHaveBeenCalledWith({ - sessionId: testQueryData.sessionId, - agentType: testQueryData.agentType, - source: testQueryData.source, - startTime: testQueryData.startTime, - duration: testQueryData.duration, - projectPath: testQueryData.projectPath, - tabId: testQueryData.tabId, + // Wait for async processing + await vi.waitFor(() => { + expect(mockStatsDB.isReady).toHaveBeenCalled(); + expect(mockStatsDB.insertQueryEvent).toHaveBeenCalledWith({ + sessionId: testQueryData.sessionId, + agentType: testQueryData.agentType, + source: testQueryData.source, + startTime: testQueryData.startTime, + duration: testQueryData.duration, + projectPath: testQueryData.projectPath, + tabId: testQueryData.tabId, + }); + expect(mockSafeSend).toHaveBeenCalledWith('stats:updated'); }); - expect(mockSafeSend).toHaveBeenCalledWith('stats:updated'); }); it('should not record event when stats database is not ready', () => { @@ -113,7 +116,7 @@ describe('Stats Listener', () => { expect(mockSafeSend).not.toHaveBeenCalled(); }); - it('should log error when recording fails', () => { + it('should log error when recording fails after retries', async () => { vi.mocked(mockStatsDB.insertQueryEvent).mockImplementation(() => { throw new Error('Database error'); }); @@ -137,16 +140,26 @@ describe('Stats Listener', () => { handler?.('session-789', testQueryData); - expect(mockLogger.error).toHaveBeenCalledWith( - expect.stringContaining('Failed to record query event'), - '[Stats]', - expect.objectContaining({ - sessionId: 'session-789', - }) + // Wait for all retries to complete (100ms + 200ms + final attempt) + await vi.waitFor( + () => { + expect(mockLogger.error).toHaveBeenCalledWith( + expect.stringContaining('Failed to record query event after 3 attempts'), + '[Stats]', + expect.objectContaining({ + sessionId: 'session-789', + }) + ); + }, + { timeout: 1000 } ); + // Should have tried 3 times + expect(mockStatsDB.insertQueryEvent).toHaveBeenCalledTimes(3); + // Should not have broadcasted update on failure + expect(mockSafeSend).not.toHaveBeenCalled(); }); - it('should log debug info when recording succeeds', () => { + it('should log debug info when recording succeeds', async () => { setupStatsListener(mockProcessManager, { safeSend: mockSafeSend, getStatsDB: () => mockStatsDB, @@ -166,15 +179,61 @@ describe('Stats Listener', () => { handler?.('session-abc', testQueryData); - expect(mockLogger.debug).toHaveBeenCalledWith( - expect.stringContaining('Recorded query event'), - '[Stats]', - expect.objectContaining({ - sessionId: 'session-abc', - agentType: 'claude-code', - source: 'user', - duration: 3000, + // Wait for async processing + await vi.waitFor(() => { + expect(mockLogger.debug).toHaveBeenCalledWith( + expect.stringContaining('Recorded query event'), + '[Stats]', + expect.objectContaining({ + sessionId: 'session-abc', + agentType: 'claude-code', + source: 'user', + duration: 3000, + }) + ); + }); + }); + + it('should retry on transient failure and succeed', async () => { + // First call fails, second succeeds + vi.mocked(mockStatsDB.insertQueryEvent) + .mockImplementationOnce(() => { + throw new Error('Transient error'); }) + .mockImplementationOnce(() => 'event-id-456'); + + setupStatsListener(mockProcessManager, { + safeSend: mockSafeSend, + getStatsDB: () => mockStatsDB, + logger: mockLogger, + }); + + const handler = eventHandlers.get('query-complete'); + const testQueryData: QueryCompleteData = { + sessionId: 'session-retry', + agentType: 'claude-code', + source: 'user', + startTime: Date.now(), + duration: 1000, + projectPath: '/test/project', + tabId: 'tab-retry', + }; + + handler?.('session-retry', testQueryData); + + // Wait for retry to complete + await vi.waitFor( + () => { + expect(mockStatsDB.insertQueryEvent).toHaveBeenCalledTimes(2); + expect(mockSafeSend).toHaveBeenCalledWith('stats:updated'); + }, + { timeout: 500 } + ); + // Should have logged warning for first failure + expect(mockLogger.warn).toHaveBeenCalledWith( + expect.stringContaining('Stats DB insert failed'), + '[Stats]', + expect.any(Object) ); }); }); diff --git a/src/main/process-listeners/stats-listener.ts b/src/main/process-listeners/stats-listener.ts index f831ef20..b37a8c18 100644 --- a/src/main/process-listeners/stats-listener.ts +++ b/src/main/process-listeners/stats-listener.ts @@ -7,9 +7,70 @@ import type { ProcessManager } from '../process-manager'; import type { QueryCompleteData } from '../process-manager/types'; import type { ProcessListenerDependencies } from './types'; +/** + * Maximum number of retry attempts for transient database failures. + */ +const MAX_RETRY_ATTEMPTS = 3; + +/** + * Base delay in milliseconds for exponential backoff (doubles each retry). + */ +const RETRY_BASE_DELAY_MS = 100; + +/** + * Attempts to insert a query event with retry logic for transient failures. + * Uses exponential backoff: 100ms, 200ms, 400ms delays between retries. + */ +async function insertQueryEventWithRetry( + db: ReturnType, + queryData: QueryCompleteData, + logger: ProcessListenerDependencies['logger'] +): Promise { + for (let attempt = 1; attempt <= MAX_RETRY_ATTEMPTS; attempt++) { + try { + const id = db.insertQueryEvent({ + sessionId: queryData.sessionId, + agentType: queryData.agentType, + source: queryData.source, + startTime: queryData.startTime, + duration: queryData.duration, + projectPath: queryData.projectPath, + tabId: queryData.tabId, + }); + return id; + } catch (error) { + const isLastAttempt = attempt === MAX_RETRY_ATTEMPTS; + + if (isLastAttempt) { + logger.error( + `Failed to record query event after ${MAX_RETRY_ATTEMPTS} attempts`, + '[Stats]', + { + error: String(error), + sessionId: queryData.sessionId, + } + ); + } else { + const delay = RETRY_BASE_DELAY_MS * Math.pow(2, attempt - 1); + logger.warn( + `Stats DB insert failed (attempt ${attempt}/${MAX_RETRY_ATTEMPTS}), retrying in ${delay}ms`, + '[Stats]', + { + error: String(error), + sessionId: queryData.sessionId, + } + ); + await new Promise((resolve) => setTimeout(resolve, delay)); + } + } + } + + return null; +} + /** * Sets up the query-complete listener for stats tracking. - * Records AI query events to the stats database. + * Records AI query events to the stats database with retry logic for transient failures. */ export function setupStatsListener( processManager: ProcessManager, @@ -20,18 +81,16 @@ export function setupStatsListener( // Handle query-complete events for stats tracking // This is emitted when a batch mode AI query completes (user or auto) processManager.on('query-complete', (_sessionId: string, queryData: QueryCompleteData) => { - try { - const db = getStatsDB(); - if (db.isReady()) { - const id = db.insertQueryEvent({ - sessionId: queryData.sessionId, - agentType: queryData.agentType, - source: queryData.source, - startTime: queryData.startTime, - duration: queryData.duration, - projectPath: queryData.projectPath, - tabId: queryData.tabId, - }); + const db = getStatsDB(); + if (!db.isReady()) { + return; + } + + // Use async IIFE to handle retry logic without blocking + void (async () => { + const id = await insertQueryEventWithRetry(db, queryData, logger); + + if (id !== null) { logger.debug(`Recorded query event: ${id}`, '[Stats]', { sessionId: queryData.sessionId, agentType: queryData.agentType, @@ -41,10 +100,6 @@ export function setupStatsListener( // Broadcast stats update to renderer for real-time dashboard refresh safeSend('stats:updated'); } - } catch (error) { - logger.error(`Failed to record query event: ${error}`, '[Stats]', { - sessionId: queryData.sessionId, - }); - } + })(); }); } diff --git a/src/main/process-listeners/types.ts b/src/main/process-listeners/types.ts index ec8559c2..b80f267d 100644 --- a/src/main/process-listeners/types.ts +++ b/src/main/process-listeners/types.ts @@ -4,6 +4,13 @@ */ import type { ProcessManager } from '../process-manager'; +import type { WebServer } from '../web-server'; +import type { AgentDetector } from '../agent-detector'; +import type { SafeSendFn } from '../utils/safe-send'; +import type { StatsDB } from '../stats-db'; +import type { GroupChat, GroupChatParticipant } from '../group-chat/group-chat-storage'; +import type { GroupChatState } from '../../shared/group-chat-types'; +import type { ParticipantState } from '../ipc/handlers/groupChat'; // ========================================================================== // Constants @@ -15,13 +22,6 @@ import type { ProcessManager } from '../process-manager'; * Session IDs starting with this prefix belong to group chat sessions. */ export const GROUP_CHAT_PREFIX = 'group-chat-'; -import type { WebServer } from '../web-server'; -import type { AgentDetector } from '../agent-detector'; -import type { SafeSendFn } from '../utils/safe-send'; -import type { StatsDB } from '../stats-db'; -import type { GroupChat, GroupChatParticipant } from '../group-chat/group-chat-storage'; -import type { GroupChatState } from '../../shared/group-chat-types'; -import type { ParticipantState } from '../ipc/handlers/groupChat'; // Re-export types from their canonical locations export type { UsageStats, QueryCompleteData, ToolExecution } from '../process-manager/types';