mirror of
https://github.com/jlengrand/Maestro.git
synced 2026-03-10 08:31:19 +00:00
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
This commit is contained in:
@@ -32,7 +32,7 @@ describe('Stats Listener', () => {
|
|||||||
|
|
||||||
mockStatsDB = {
|
mockStatsDB = {
|
||||||
isReady: vi.fn(() => true),
|
isReady: vi.fn(() => true),
|
||||||
insertQueryEvent: vi.fn(() => 1),
|
insertQueryEvent: vi.fn(() => 'event-id-123'),
|
||||||
} as unknown as StatsDB;
|
} as unknown as StatsDB;
|
||||||
|
|
||||||
mockProcessManager = {
|
mockProcessManager = {
|
||||||
@@ -52,7 +52,7 @@ describe('Stats Listener', () => {
|
|||||||
expect(mockProcessManager.on).toHaveBeenCalledWith('query-complete', expect.any(Function));
|
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, {
|
setupStatsListener(mockProcessManager, {
|
||||||
safeSend: mockSafeSend,
|
safeSend: mockSafeSend,
|
||||||
getStatsDB: () => mockStatsDB,
|
getStatsDB: () => mockStatsDB,
|
||||||
@@ -73,6 +73,8 @@ describe('Stats Listener', () => {
|
|||||||
|
|
||||||
handler?.(testSessionId, testQueryData);
|
handler?.(testSessionId, testQueryData);
|
||||||
|
|
||||||
|
// Wait for async processing
|
||||||
|
await vi.waitFor(() => {
|
||||||
expect(mockStatsDB.isReady).toHaveBeenCalled();
|
expect(mockStatsDB.isReady).toHaveBeenCalled();
|
||||||
expect(mockStatsDB.insertQueryEvent).toHaveBeenCalledWith({
|
expect(mockStatsDB.insertQueryEvent).toHaveBeenCalledWith({
|
||||||
sessionId: testQueryData.sessionId,
|
sessionId: testQueryData.sessionId,
|
||||||
@@ -85,6 +87,7 @@ describe('Stats Listener', () => {
|
|||||||
});
|
});
|
||||||
expect(mockSafeSend).toHaveBeenCalledWith('stats:updated');
|
expect(mockSafeSend).toHaveBeenCalledWith('stats:updated');
|
||||||
});
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it('should not record event when stats database is not ready', () => {
|
it('should not record event when stats database is not ready', () => {
|
||||||
vi.mocked(mockStatsDB.isReady).mockReturnValue(false);
|
vi.mocked(mockStatsDB.isReady).mockReturnValue(false);
|
||||||
@@ -113,7 +116,7 @@ describe('Stats Listener', () => {
|
|||||||
expect(mockSafeSend).not.toHaveBeenCalled();
|
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(() => {
|
vi.mocked(mockStatsDB.insertQueryEvent).mockImplementation(() => {
|
||||||
throw new Error('Database error');
|
throw new Error('Database error');
|
||||||
});
|
});
|
||||||
@@ -137,16 +140,26 @@ describe('Stats Listener', () => {
|
|||||||
|
|
||||||
handler?.('session-789', testQueryData);
|
handler?.('session-789', testQueryData);
|
||||||
|
|
||||||
|
// Wait for all retries to complete (100ms + 200ms + final attempt)
|
||||||
|
await vi.waitFor(
|
||||||
|
() => {
|
||||||
expect(mockLogger.error).toHaveBeenCalledWith(
|
expect(mockLogger.error).toHaveBeenCalledWith(
|
||||||
expect.stringContaining('Failed to record query event'),
|
expect.stringContaining('Failed to record query event after 3 attempts'),
|
||||||
'[Stats]',
|
'[Stats]',
|
||||||
expect.objectContaining({
|
expect.objectContaining({
|
||||||
sessionId: 'session-789',
|
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, {
|
setupStatsListener(mockProcessManager, {
|
||||||
safeSend: mockSafeSend,
|
safeSend: mockSafeSend,
|
||||||
getStatsDB: () => mockStatsDB,
|
getStatsDB: () => mockStatsDB,
|
||||||
@@ -166,6 +179,8 @@ describe('Stats Listener', () => {
|
|||||||
|
|
||||||
handler?.('session-abc', testQueryData);
|
handler?.('session-abc', testQueryData);
|
||||||
|
|
||||||
|
// Wait for async processing
|
||||||
|
await vi.waitFor(() => {
|
||||||
expect(mockLogger.debug).toHaveBeenCalledWith(
|
expect(mockLogger.debug).toHaveBeenCalledWith(
|
||||||
expect.stringContaining('Recorded query event'),
|
expect.stringContaining('Recorded query event'),
|
||||||
'[Stats]',
|
'[Stats]',
|
||||||
@@ -178,3 +193,47 @@ describe('Stats Listener', () => {
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
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)
|
||||||
|
);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -7,9 +7,70 @@ import type { ProcessManager } from '../process-manager';
|
|||||||
import type { QueryCompleteData } from '../process-manager/types';
|
import type { QueryCompleteData } from '../process-manager/types';
|
||||||
import type { ProcessListenerDependencies } from './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<ProcessListenerDependencies['getStatsDB']>,
|
||||||
|
queryData: QueryCompleteData,
|
||||||
|
logger: ProcessListenerDependencies['logger']
|
||||||
|
): Promise<string | null> {
|
||||||
|
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.
|
* 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(
|
export function setupStatsListener(
|
||||||
processManager: ProcessManager,
|
processManager: ProcessManager,
|
||||||
@@ -20,18 +81,16 @@ export function setupStatsListener(
|
|||||||
// Handle query-complete events for stats tracking
|
// Handle query-complete events for stats tracking
|
||||||
// This is emitted when a batch mode AI query completes (user or auto)
|
// This is emitted when a batch mode AI query completes (user or auto)
|
||||||
processManager.on('query-complete', (_sessionId: string, queryData: QueryCompleteData) => {
|
processManager.on('query-complete', (_sessionId: string, queryData: QueryCompleteData) => {
|
||||||
try {
|
|
||||||
const db = getStatsDB();
|
const db = getStatsDB();
|
||||||
if (db.isReady()) {
|
if (!db.isReady()) {
|
||||||
const id = db.insertQueryEvent({
|
return;
|
||||||
sessionId: queryData.sessionId,
|
}
|
||||||
agentType: queryData.agentType,
|
|
||||||
source: queryData.source,
|
// Use async IIFE to handle retry logic without blocking
|
||||||
startTime: queryData.startTime,
|
void (async () => {
|
||||||
duration: queryData.duration,
|
const id = await insertQueryEventWithRetry(db, queryData, logger);
|
||||||
projectPath: queryData.projectPath,
|
|
||||||
tabId: queryData.tabId,
|
if (id !== null) {
|
||||||
});
|
|
||||||
logger.debug(`Recorded query event: ${id}`, '[Stats]', {
|
logger.debug(`Recorded query event: ${id}`, '[Stats]', {
|
||||||
sessionId: queryData.sessionId,
|
sessionId: queryData.sessionId,
|
||||||
agentType: queryData.agentType,
|
agentType: queryData.agentType,
|
||||||
@@ -41,10 +100,6 @@ export function setupStatsListener(
|
|||||||
// Broadcast stats update to renderer for real-time dashboard refresh
|
// Broadcast stats update to renderer for real-time dashboard refresh
|
||||||
safeSend('stats:updated');
|
safeSend('stats:updated');
|
||||||
}
|
}
|
||||||
} catch (error) {
|
})();
|
||||||
logger.error(`Failed to record query event: ${error}`, '[Stats]', {
|
|
||||||
sessionId: queryData.sessionId,
|
|
||||||
});
|
|
||||||
}
|
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -4,6 +4,13 @@
|
|||||||
*/
|
*/
|
||||||
|
|
||||||
import type { ProcessManager } from '../process-manager';
|
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
|
// Constants
|
||||||
@@ -15,13 +22,6 @@ import type { ProcessManager } from '../process-manager';
|
|||||||
* Session IDs starting with this prefix belong to group chat sessions.
|
* Session IDs starting with this prefix belong to group chat sessions.
|
||||||
*/
|
*/
|
||||||
export const GROUP_CHAT_PREFIX = 'group-chat-';
|
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
|
// Re-export types from their canonical locations
|
||||||
export type { UsageStats, QueryCompleteData, ToolExecution } from '../process-manager/types';
|
export type { UsageStats, QueryCompleteData, ToolExecution } from '../process-manager/types';
|
||||||
|
|||||||
Reference in New Issue
Block a user