mirror of
https://github.com/AutoMaker-Org/automaker.git
synced 2026-02-03 21:03:08 +00:00
refactor: Improve error handling and status preservation in auto-mode service
- Simplified the graceful shutdown process by removing redundant error handling for marking features as interrupted, as it is now managed internally. - Updated orphan detection logging to streamline the process and enhance clarity. - Added logic to preserve specific pipeline statuses when marking features as interrupted, ensuring correct resumption of features after a server restart. - Enhanced unit tests to cover new behavior for preserving pipeline statuses and handling various feature states.
This commit is contained in:
@@ -787,11 +787,8 @@ const gracefulShutdown = async (signal: string) => {
|
|||||||
|
|
||||||
// Mark all running features as interrupted before shutdown
|
// Mark all running features as interrupted before shutdown
|
||||||
// This ensures they can be resumed when the server restarts
|
// This ensures they can be resumed when the server restarts
|
||||||
try {
|
// Note: markAllRunningFeaturesInterrupted handles errors internally and never rejects
|
||||||
await autoModeService.markAllRunningFeaturesInterrupted(`${signal} signal received`);
|
await autoModeService.markAllRunningFeaturesInterrupted(`${signal} signal received`);
|
||||||
} catch (error) {
|
|
||||||
logger.error('Failed to mark running features as interrupted:', error);
|
|
||||||
}
|
|
||||||
|
|
||||||
terminalService.cleanup();
|
terminalService.cleanup();
|
||||||
server.close(() => {
|
server.close(() => {
|
||||||
|
|||||||
@@ -28,25 +28,20 @@ export function createListHandler(featureLoader: FeatureLoader, autoModeService?
|
|||||||
// Run orphan detection in background when project is loaded
|
// Run orphan detection in background when project is loaded
|
||||||
// This detects features whose branches no longer exist (e.g., after merge/delete)
|
// This detects features whose branches no longer exist (e.g., after merge/delete)
|
||||||
// We don't await this to keep the list response fast
|
// We don't await this to keep the list response fast
|
||||||
|
// Note: detectOrphanedFeatures handles errors internally and always resolves
|
||||||
if (autoModeService) {
|
if (autoModeService) {
|
||||||
autoModeService
|
autoModeService.detectOrphanedFeatures(projectPath).then((orphanedFeatures) => {
|
||||||
.detectOrphanedFeatures(projectPath)
|
if (orphanedFeatures.length > 0) {
|
||||||
.then((orphanedFeatures) => {
|
logger.info(
|
||||||
if (orphanedFeatures.length > 0) {
|
`[ProjectLoad] Detected ${orphanedFeatures.length} orphaned feature(s) in ${projectPath}`
|
||||||
|
);
|
||||||
|
for (const { feature, missingBranch } of orphanedFeatures) {
|
||||||
logger.info(
|
logger.info(
|
||||||
`[ProjectLoad] Detected ${orphanedFeatures.length} orphaned feature(s) in ${projectPath}`
|
`[ProjectLoad] Orphaned: ${feature.title || feature.id} - branch "${missingBranch}" no longer exists`
|
||||||
);
|
);
|
||||||
for (const { feature, missingBranch } of orphanedFeatures) {
|
|
||||||
logger.info(
|
|
||||||
`[ProjectLoad] Orphaned: ${feature.title || feature.id} - branch "${missingBranch}" no longer exists`
|
|
||||||
);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
})
|
}
|
||||||
.catch((error: unknown) => {
|
});
|
||||||
const errorMessage = error instanceof Error ? error.message : String(error);
|
|
||||||
logger.warn(`[ProjectLoad] Failed to detect orphaned features: ${errorMessage}`);
|
|
||||||
});
|
|
||||||
}
|
}
|
||||||
|
|
||||||
res.json({ success: true, features });
|
res.json({ success: true, features });
|
||||||
|
|||||||
@@ -3095,6 +3095,10 @@ Format your response as a structured markdown document.`;
|
|||||||
* restart, process crash, or manual stop). Features with this status can be
|
* restart, process crash, or manual stop). Features with this status can be
|
||||||
* resumed later using the resume functionality.
|
* resumed later using the resume functionality.
|
||||||
*
|
*
|
||||||
|
* Note: Features with pipeline_* statuses are preserved rather than overwritten
|
||||||
|
* to 'interrupted'. This ensures that resumePipelineFeature() can pick up from
|
||||||
|
* the correct pipeline step after a restart.
|
||||||
|
*
|
||||||
* @param projectPath - Path to the project
|
* @param projectPath - Path to the project
|
||||||
* @param featureId - ID of the feature to mark as interrupted
|
* @param featureId - ID of the feature to mark as interrupted
|
||||||
* @param reason - Optional reason for the interruption (logged for debugging)
|
* @param reason - Optional reason for the interruption (logged for debugging)
|
||||||
@@ -3104,6 +3108,18 @@ Format your response as a structured markdown document.`;
|
|||||||
featureId: string,
|
featureId: string,
|
||||||
reason?: string
|
reason?: string
|
||||||
): Promise<void> {
|
): Promise<void> {
|
||||||
|
// Load the feature to check its current status
|
||||||
|
const feature = await this.loadFeature(projectPath, featureId);
|
||||||
|
const currentStatus = feature?.status;
|
||||||
|
|
||||||
|
// Preserve pipeline_* statuses so resumePipelineFeature can resume from the correct step
|
||||||
|
if (currentStatus && currentStatus.startsWith('pipeline_')) {
|
||||||
|
logger.info(
|
||||||
|
`Feature ${featureId} was in ${currentStatus}; preserving pipeline status for resume`
|
||||||
|
);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
if (reason) {
|
if (reason) {
|
||||||
logger.info(`Marking feature ${featureId} as interrupted: ${reason}`);
|
logger.info(`Marking feature ${featureId} as interrupted: ${reason}`);
|
||||||
} else {
|
} else {
|
||||||
|
|||||||
@@ -483,8 +483,15 @@ describe('auto-mode-service.ts', () => {
|
|||||||
(svc as any).updateFeatureStatus = mockFn;
|
(svc as any).updateFeatureStatus = mockFn;
|
||||||
};
|
};
|
||||||
|
|
||||||
it('should call updateFeatureStatus with interrupted status', async () => {
|
// Helper to mock loadFeature
|
||||||
|
const mockLoadFeature = (svc: AutoModeService, mockFn: ReturnType<typeof vi.fn>) => {
|
||||||
|
(svc as any).loadFeature = mockFn;
|
||||||
|
};
|
||||||
|
|
||||||
|
it('should call updateFeatureStatus with interrupted status for non-pipeline features', async () => {
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ id: 'feature-123', status: 'in_progress' });
|
||||||
const updateMock = vi.fn().mockResolvedValue(undefined);
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
mockUpdateFeatureStatus(service, updateMock);
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
await service.markFeatureInterrupted('/test/project', 'feature-123');
|
await service.markFeatureInterrupted('/test/project', 'feature-123');
|
||||||
@@ -493,7 +500,9 @@ describe('auto-mode-service.ts', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('should call updateFeatureStatus with reason when provided', async () => {
|
it('should call updateFeatureStatus with reason when provided', async () => {
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ id: 'feature-123', status: 'in_progress' });
|
||||||
const updateMock = vi.fn().mockResolvedValue(undefined);
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
mockUpdateFeatureStatus(service, updateMock);
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
await service.markFeatureInterrupted('/test/project', 'feature-123', 'server shutdown');
|
await service.markFeatureInterrupted('/test/project', 'feature-123', 'server shutdown');
|
||||||
@@ -502,13 +511,73 @@ describe('auto-mode-service.ts', () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
it('should propagate errors from updateFeatureStatus', async () => {
|
it('should propagate errors from updateFeatureStatus', async () => {
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ id: 'feature-123', status: 'in_progress' });
|
||||||
const updateMock = vi.fn().mockRejectedValue(new Error('Update failed'));
|
const updateMock = vi.fn().mockRejectedValue(new Error('Update failed'));
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
mockUpdateFeatureStatus(service, updateMock);
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
await expect(service.markFeatureInterrupted('/test/project', 'feature-123')).rejects.toThrow(
|
await expect(service.markFeatureInterrupted('/test/project', 'feature-123')).rejects.toThrow(
|
||||||
'Update failed'
|
'Update failed'
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should preserve pipeline_implementation status instead of marking as interrupted', async () => {
|
||||||
|
const loadMock = vi
|
||||||
|
.fn()
|
||||||
|
.mockResolvedValue({ id: 'feature-123', status: 'pipeline_implementation' });
|
||||||
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
|
await service.markFeatureInterrupted('/test/project', 'feature-123', 'server shutdown');
|
||||||
|
|
||||||
|
// updateFeatureStatus should NOT be called for pipeline statuses
|
||||||
|
expect(updateMock).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should preserve pipeline_testing status instead of marking as interrupted', async () => {
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ id: 'feature-123', status: 'pipeline_testing' });
|
||||||
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
|
await service.markFeatureInterrupted('/test/project', 'feature-123');
|
||||||
|
|
||||||
|
expect(updateMock).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should preserve pipeline_review status instead of marking as interrupted', async () => {
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ id: 'feature-123', status: 'pipeline_review' });
|
||||||
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
|
await service.markFeatureInterrupted('/test/project', 'feature-123');
|
||||||
|
|
||||||
|
expect(updateMock).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should mark feature as interrupted when loadFeature returns null', async () => {
|
||||||
|
const loadMock = vi.fn().mockResolvedValue(null);
|
||||||
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
|
await service.markFeatureInterrupted('/test/project', 'feature-123');
|
||||||
|
|
||||||
|
expect(updateMock).toHaveBeenCalledWith('/test/project', 'feature-123', 'interrupted');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('should mark feature as interrupted for pending status', async () => {
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ id: 'feature-123', status: 'pending' });
|
||||||
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
|
await service.markFeatureInterrupted('/test/project', 'feature-123');
|
||||||
|
|
||||||
|
expect(updateMock).toHaveBeenCalledWith('/test/project', 'feature-123', 'interrupted');
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('markAllRunningFeaturesInterrupted', () => {
|
describe('markAllRunningFeaturesInterrupted', () => {
|
||||||
@@ -524,6 +593,11 @@ describe('auto-mode-service.ts', () => {
|
|||||||
(svc as any).updateFeatureStatus = mockFn;
|
(svc as any).updateFeatureStatus = mockFn;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
// Helper to mock loadFeature
|
||||||
|
const mockLoadFeature = (svc: AutoModeService, mockFn: ReturnType<typeof vi.fn>) => {
|
||||||
|
(svc as any).loadFeature = mockFn;
|
||||||
|
};
|
||||||
|
|
||||||
it('should do nothing when no features are running', async () => {
|
it('should do nothing when no features are running', async () => {
|
||||||
const updateMock = vi.fn().mockResolvedValue(undefined);
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
mockUpdateFeatureStatus(service, updateMock);
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
@@ -541,7 +615,9 @@ describe('auto-mode-service.ts', () => {
|
|||||||
isAutoMode: true,
|
isAutoMode: true,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ id: 'feature-1', status: 'in_progress' });
|
||||||
const updateMock = vi.fn().mockResolvedValue(undefined);
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
mockUpdateFeatureStatus(service, updateMock);
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
await service.markAllRunningFeaturesInterrupted();
|
await service.markAllRunningFeaturesInterrupted();
|
||||||
@@ -567,7 +643,9 @@ describe('auto-mode-service.ts', () => {
|
|||||||
isAutoMode: true,
|
isAutoMode: true,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ status: 'in_progress' });
|
||||||
const updateMock = vi.fn().mockResolvedValue(undefined);
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
mockUpdateFeatureStatus(service, updateMock);
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
await service.markAllRunningFeaturesInterrupted();
|
await service.markAllRunningFeaturesInterrupted();
|
||||||
@@ -588,11 +666,13 @@ describe('auto-mode-service.ts', () => {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ status: 'in_progress' });
|
||||||
const callOrder: string[] = [];
|
const callOrder: string[] = [];
|
||||||
const updateMock = vi.fn().mockImplementation(async (_path: string, featureId: string) => {
|
const updateMock = vi.fn().mockImplementation(async (_path: string, featureId: string) => {
|
||||||
callOrder.push(featureId);
|
callOrder.push(featureId);
|
||||||
await new Promise((resolve) => setTimeout(resolve, 10));
|
await new Promise((resolve) => setTimeout(resolve, 10));
|
||||||
});
|
});
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
mockUpdateFeatureStatus(service, updateMock);
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
const startTime = Date.now();
|
const startTime = Date.now();
|
||||||
@@ -618,10 +698,12 @@ describe('auto-mode-service.ts', () => {
|
|||||||
isAutoMode: false,
|
isAutoMode: false,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ status: 'in_progress' });
|
||||||
const updateMock = vi
|
const updateMock = vi
|
||||||
.fn()
|
.fn()
|
||||||
.mockResolvedValueOnce(undefined)
|
.mockResolvedValueOnce(undefined)
|
||||||
.mockRejectedValueOnce(new Error('Failed to update'));
|
.mockRejectedValueOnce(new Error('Failed to update'));
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
mockUpdateFeatureStatus(service, updateMock);
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
// Should not throw even though one feature failed
|
// Should not throw even though one feature failed
|
||||||
@@ -638,7 +720,9 @@ describe('auto-mode-service.ts', () => {
|
|||||||
isAutoMode: true,
|
isAutoMode: true,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ id: 'feature-1', status: 'in_progress' });
|
||||||
const updateMock = vi.fn().mockResolvedValue(undefined);
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
mockUpdateFeatureStatus(service, updateMock);
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
await service.markAllRunningFeaturesInterrupted('manual stop');
|
await service.markAllRunningFeaturesInterrupted('manual stop');
|
||||||
@@ -654,13 +738,56 @@ describe('auto-mode-service.ts', () => {
|
|||||||
isAutoMode: true,
|
isAutoMode: true,
|
||||||
});
|
});
|
||||||
|
|
||||||
|
const loadMock = vi.fn().mockResolvedValue({ id: 'feature-1', status: 'in_progress' });
|
||||||
const updateMock = vi.fn().mockResolvedValue(undefined);
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
mockUpdateFeatureStatus(service, updateMock);
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
await service.markAllRunningFeaturesInterrupted();
|
await service.markAllRunningFeaturesInterrupted();
|
||||||
|
|
||||||
expect(updateMock).toHaveBeenCalledWith('/project/path', 'feature-1', 'interrupted');
|
expect(updateMock).toHaveBeenCalledWith('/project/path', 'feature-1', 'interrupted');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('should preserve pipeline statuses for running features', async () => {
|
||||||
|
const runningFeaturesMap = getRunningFeaturesMap(service);
|
||||||
|
runningFeaturesMap.set('feature-1', {
|
||||||
|
featureId: 'feature-1',
|
||||||
|
projectPath: '/project-a',
|
||||||
|
isAutoMode: true,
|
||||||
|
});
|
||||||
|
runningFeaturesMap.set('feature-2', {
|
||||||
|
featureId: 'feature-2',
|
||||||
|
projectPath: '/project-b',
|
||||||
|
isAutoMode: false,
|
||||||
|
});
|
||||||
|
runningFeaturesMap.set('feature-3', {
|
||||||
|
featureId: 'feature-3',
|
||||||
|
projectPath: '/project-c',
|
||||||
|
isAutoMode: true,
|
||||||
|
});
|
||||||
|
|
||||||
|
// feature-1 has in_progress (should be interrupted)
|
||||||
|
// feature-2 has pipeline_testing (should be preserved)
|
||||||
|
// feature-3 has pipeline_implementation (should be preserved)
|
||||||
|
const loadMock = vi
|
||||||
|
.fn()
|
||||||
|
.mockImplementation(async (_projectPath: string, featureId: string) => {
|
||||||
|
if (featureId === 'feature-1') return { id: 'feature-1', status: 'in_progress' };
|
||||||
|
if (featureId === 'feature-2') return { id: 'feature-2', status: 'pipeline_testing' };
|
||||||
|
if (featureId === 'feature-3')
|
||||||
|
return { id: 'feature-3', status: 'pipeline_implementation' };
|
||||||
|
return null;
|
||||||
|
});
|
||||||
|
const updateMock = vi.fn().mockResolvedValue(undefined);
|
||||||
|
mockLoadFeature(service, loadMock);
|
||||||
|
mockUpdateFeatureStatus(service, updateMock);
|
||||||
|
|
||||||
|
await service.markAllRunningFeaturesInterrupted();
|
||||||
|
|
||||||
|
// Only feature-1 should be marked as interrupted
|
||||||
|
expect(updateMock).toHaveBeenCalledTimes(1);
|
||||||
|
expect(updateMock).toHaveBeenCalledWith('/project-a', 'feature-1', 'interrupted');
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe('isFeatureRunning', () => {
|
describe('isFeatureRunning', () => {
|
||||||
|
|||||||
Reference in New Issue
Block a user