From ef779daedf185ac2b7b00b79f492b660469e9f21 Mon Sep 17 00:00:00 2001 From: Shirone Date: Sun, 25 Jan 2026 14:57:23 +0100 Subject: [PATCH] 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. --- apps/server/src/index.ts | 7 +- .../server/src/routes/features/routes/list.ts | 25 ++-- apps/server/src/services/auto-mode-service.ts | 16 +++ .../unit/services/auto-mode-service.test.ts | 129 +++++++++++++++++- 4 files changed, 156 insertions(+), 21 deletions(-) diff --git a/apps/server/src/index.ts b/apps/server/src/index.ts index 06040100..ab3d60f5 100644 --- a/apps/server/src/index.ts +++ b/apps/server/src/index.ts @@ -787,11 +787,8 @@ const gracefulShutdown = async (signal: string) => { // Mark all running features as interrupted before shutdown // This ensures they can be resumed when the server restarts - try { - await autoModeService.markAllRunningFeaturesInterrupted(`${signal} signal received`); - } catch (error) { - logger.error('Failed to mark running features as interrupted:', error); - } + // Note: markAllRunningFeaturesInterrupted handles errors internally and never rejects + await autoModeService.markAllRunningFeaturesInterrupted(`${signal} signal received`); terminalService.cleanup(); server.close(() => { diff --git a/apps/server/src/routes/features/routes/list.ts b/apps/server/src/routes/features/routes/list.ts index 7920db73..40c35966 100644 --- a/apps/server/src/routes/features/routes/list.ts +++ b/apps/server/src/routes/features/routes/list.ts @@ -28,25 +28,20 @@ export function createListHandler(featureLoader: FeatureLoader, autoModeService? // Run orphan detection in background when project is loaded // This detects features whose branches no longer exist (e.g., after merge/delete) // We don't await this to keep the list response fast + // Note: detectOrphanedFeatures handles errors internally and always resolves if (autoModeService) { - autoModeService - .detectOrphanedFeatures(projectPath) - .then((orphanedFeatures) => { - if (orphanedFeatures.length > 0) { + autoModeService.detectOrphanedFeatures(projectPath).then((orphanedFeatures) => { + if (orphanedFeatures.length > 0) { + logger.info( + `[ProjectLoad] Detected ${orphanedFeatures.length} orphaned feature(s) in ${projectPath}` + ); + for (const { feature, missingBranch } of orphanedFeatures) { 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 }); diff --git a/apps/server/src/services/auto-mode-service.ts b/apps/server/src/services/auto-mode-service.ts index 64ab7ee6..d6aa180b 100644 --- a/apps/server/src/services/auto-mode-service.ts +++ b/apps/server/src/services/auto-mode-service.ts @@ -3095,6 +3095,10 @@ Format your response as a structured markdown document.`; * restart, process crash, or manual stop). Features with this status can be * 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 featureId - ID of the feature to mark as interrupted * @param reason - Optional reason for the interruption (logged for debugging) @@ -3104,6 +3108,18 @@ Format your response as a structured markdown document.`; featureId: string, reason?: string ): Promise { + // 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) { logger.info(`Marking feature ${featureId} as interrupted: ${reason}`); } else { diff --git a/apps/server/tests/unit/services/auto-mode-service.test.ts b/apps/server/tests/unit/services/auto-mode-service.test.ts index 1de26bae..7f3f9af0 100644 --- a/apps/server/tests/unit/services/auto-mode-service.test.ts +++ b/apps/server/tests/unit/services/auto-mode-service.test.ts @@ -483,8 +483,15 @@ describe('auto-mode-service.ts', () => { (svc as any).updateFeatureStatus = mockFn; }; - it('should call updateFeatureStatus with interrupted status', async () => { + // Helper to mock loadFeature + const mockLoadFeature = (svc: AutoModeService, mockFn: ReturnType) => { + (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); + mockLoadFeature(service, loadMock); mockUpdateFeatureStatus(service, updateMock); 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 () => { + const loadMock = vi.fn().mockResolvedValue({ id: 'feature-123', status: 'in_progress' }); const updateMock = vi.fn().mockResolvedValue(undefined); + mockLoadFeature(service, loadMock); mockUpdateFeatureStatus(service, updateMock); 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 () => { + const loadMock = vi.fn().mockResolvedValue({ id: 'feature-123', status: 'in_progress' }); const updateMock = vi.fn().mockRejectedValue(new Error('Update failed')); + mockLoadFeature(service, loadMock); mockUpdateFeatureStatus(service, updateMock); await expect(service.markFeatureInterrupted('/test/project', 'feature-123')).rejects.toThrow( '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', () => { @@ -524,6 +593,11 @@ describe('auto-mode-service.ts', () => { (svc as any).updateFeatureStatus = mockFn; }; + // Helper to mock loadFeature + const mockLoadFeature = (svc: AutoModeService, mockFn: ReturnType) => { + (svc as any).loadFeature = mockFn; + }; + it('should do nothing when no features are running', async () => { const updateMock = vi.fn().mockResolvedValue(undefined); mockUpdateFeatureStatus(service, updateMock); @@ -541,7 +615,9 @@ describe('auto-mode-service.ts', () => { isAutoMode: true, }); + const loadMock = vi.fn().mockResolvedValue({ id: 'feature-1', status: 'in_progress' }); const updateMock = vi.fn().mockResolvedValue(undefined); + mockLoadFeature(service, loadMock); mockUpdateFeatureStatus(service, updateMock); await service.markAllRunningFeaturesInterrupted(); @@ -567,7 +643,9 @@ describe('auto-mode-service.ts', () => { isAutoMode: true, }); + const loadMock = vi.fn().mockResolvedValue({ status: 'in_progress' }); const updateMock = vi.fn().mockResolvedValue(undefined); + mockLoadFeature(service, loadMock); mockUpdateFeatureStatus(service, updateMock); await service.markAllRunningFeaturesInterrupted(); @@ -588,11 +666,13 @@ describe('auto-mode-service.ts', () => { }); } + const loadMock = vi.fn().mockResolvedValue({ status: 'in_progress' }); const callOrder: string[] = []; const updateMock = vi.fn().mockImplementation(async (_path: string, featureId: string) => { callOrder.push(featureId); await new Promise((resolve) => setTimeout(resolve, 10)); }); + mockLoadFeature(service, loadMock); mockUpdateFeatureStatus(service, updateMock); const startTime = Date.now(); @@ -618,10 +698,12 @@ describe('auto-mode-service.ts', () => { isAutoMode: false, }); + const loadMock = vi.fn().mockResolvedValue({ status: 'in_progress' }); const updateMock = vi .fn() .mockResolvedValueOnce(undefined) .mockRejectedValueOnce(new Error('Failed to update')); + mockLoadFeature(service, loadMock); mockUpdateFeatureStatus(service, updateMock); // Should not throw even though one feature failed @@ -638,7 +720,9 @@ describe('auto-mode-service.ts', () => { isAutoMode: true, }); + const loadMock = vi.fn().mockResolvedValue({ id: 'feature-1', status: 'in_progress' }); const updateMock = vi.fn().mockResolvedValue(undefined); + mockLoadFeature(service, loadMock); mockUpdateFeatureStatus(service, updateMock); await service.markAllRunningFeaturesInterrupted('manual stop'); @@ -654,13 +738,56 @@ describe('auto-mode-service.ts', () => { isAutoMode: true, }); + const loadMock = vi.fn().mockResolvedValue({ id: 'feature-1', status: 'in_progress' }); const updateMock = vi.fn().mockResolvedValue(undefined); + mockLoadFeature(service, loadMock); mockUpdateFeatureStatus(service, updateMock); await service.markAllRunningFeaturesInterrupted(); 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', () => {