diff --git a/CODE_QUALITY_ANALYSIS.md b/CODE_QUALITY_ANALYSIS.md new file mode 100644 index 00000000..1b753c64 --- /dev/null +++ b/CODE_QUALITY_ANALYSIS.md @@ -0,0 +1,584 @@ +# Code Quality Analysis Report - src/cli + +**Date:** 2025-12-30 +**Analyzed Directory:** `/src/cli` +**Total Files:** ~481 TypeScript files +**Total Lines of Code:** ~191,000 LOC + +--- + +## Executive Summary + +This analysis identifies critical code quality issues across the CLI codebase, focusing on: +- Memory leaks from uncleaned resources +- Try-catch-finally blocks with improper error handling +- Missing listener cleanup patterns +- Resource disposal issues +- Interval/timeout cleanup problems +- Async operation handling issues + +--- + +## 1. CRITICAL: Memory Leaks + +### 1.1 setInterval Without Cleanup (streaming-orchestrator.ts:1254-1302) + +**File:** `src/cli/streaming-orchestrator.ts` +**Severity:** HIGH + +```typescript +private startMessageProcessor(): void { + // BUG: 4 setInterval calls created but NEVER stored for cleanup + setInterval(() => { + if (!this.processingMessage) { + this.processNextMessage() + } + this.updateContextCounter() + }, 100) + + setInterval(() => { + this.absorbCompletedMessages() + }, 5000) + + setInterval(() => { + this.processQueuedInputs() + }, 2000) + + setInterval(() => { + if (this.cognitiveEnabled) { + // token optimization + } + }, 30000) +} +``` + +**Problem:** Four intervals are created but never stored or cleaned up. When the orchestrator is destroyed, these intervals continue running, causing: +- Memory leaks (closures hold references) +- CPU usage from zombie intervals +- Potential errors accessing destroyed objects + +**Fix Required:** +```typescript +private messageProcessorIntervals: NodeJS.Timeout[] = [] + +private startMessageProcessor(): void { + this.messageProcessorIntervals.push( + setInterval(() => { ... }, 100), + setInterval(() => { ... }, 5000), + setInterval(() => { ... }, 2000), + setInterval(() => { ... }, 30000) + ) +} + +private cleanup(): void { + this.messageProcessorIntervals.forEach(clearInterval) + this.messageProcessorIntervals = [] +} +``` + +### 1.2 EventBus Subscriber Accumulation (event-bus.ts) + +**File:** `src/cli/automation/agents/event-bus.ts` +**Severity:** MEDIUM + +The EventBus uses `Map>` for subscribers. While `cleanup()` exists, issues include: + +1. **Event history grows unbounded** until reaching 1000 events, then trims +2. **Subscribers not auto-removed** when components unmount +3. **No weak references** - subscribers hold strong references to handlers + +**Current cleanup:** +```typescript +cleanup(): void { + this.subscribers.clear() + this.eventHistory = [] + this.removeAllListeners() +} +``` + +**Problem:** Application must explicitly call `cleanup()` on shutdown, which may not happen in all exit paths. + +### 1.3 Singleton Instantiation Memory (agent-service.ts:1820) + +**File:** `src/cli/services/agent-service.ts` + +```typescript +export const agentService = new AgentService() +``` + +**Problem:** Module-level singleton instantiation means: +- Service is created immediately on import +- Resources allocated before app is ready +- No lazy initialization +- Memory allocated even if service is never used + +--- + +## 2. CRITICAL: Try-Catch-Finally Issues + +### 2.1 Silent Error Swallowing Pattern + +Found **50+ instances** of empty or near-empty catch blocks: + +**Example 1 - Complete silence:** `streaming-orchestrator.ts:240-242` +```typescript +} catch { + // ignore +} +``` + +**Example 2 - Partial logging:** `main-orchestrator.ts:151-153` +```typescript +} catch (_error) { + // Silent fallback - cache cleanup is non-critical +} +``` + +**Example 3 - Error swallowed in disposal:** `agent-service.ts:630` +```typescript +} catch { + // ignore +} +``` + +### 2.2 Missing Finally Blocks for Resource Cleanup + +**File:** `src/cli/services/agent-service.ts:442-495` + +```typescript +const executionPromise = (async () => { + try { + for await (const update of generator as any) { + // ... processing + } + } catch (streamError: any) { + throw new Error(`Stream processing error: ${streamError.message}`) + } +})() +// NO FINALLY - generator may not be properly closed on error +``` + +**Better pattern:** +```typescript +try { + for await (const update of generator) { ... } +} catch (error) { + throw error +} finally { + await generator.return?.() // Always close generator +} +``` + +### 2.3 Locations with Empty Catch Blocks + +| File | Line | Pattern | +|------|------|---------| +| `streaming-orchestrator.ts` | 240 | `catch { // ignore }` | +| `streaming-orchestrator.ts` | 1086 | `catch {}` | +| `streaming-orchestrator.ts` | 1401 | `catch {}` | +| `agent-service.ts` | 449-451 | `catch { /* ignore */ }` | +| `agent-service.ts` | 587-589 | `catch { /* ignore */ }` | +| `agent-service.ts` | 630 | `catch { // ignore }` | +| `main-orchestrator.ts` | 151-153 | Silent cache cleanup | +| `context/workspace-cache-manager.ts` | 83-85 | `return false` | +| `context/workspace-cache-manager.ts` | 104-106 | `return null` | +| `chat/chat-manager.ts` | 151-154 | Silent fallback | + +--- + +## 3. HIGH: Missing Listener Cleanup + +### 3.1 Event Listeners Registered But Never Removed + +**File:** `src/cli/streaming-orchestrator.ts:245-301` + +```typescript +private setupServiceListeners(): void { + // These listeners are NEVER removed + agentService.on('task_start', (task) => { ... }) + agentService.on('task_progress', (task, update) => { ... }) + agentService.on('tool_use', (task, update) => { ... }) + agentService.on('task_complete', (task) => { ... }) +} +``` + +**Problem:** When StreamingOrchestrator is destroyed, these listeners remain attached to agentService, causing: +- Memory leak (closures hold orchestrator reference) +- Potential errors when handlers access destroyed orchestrator +- Double event handling if new orchestrator is created + +**Fix Required:** +```typescript +private boundHandlers: Map = new Map() + +private setupServiceListeners(): void { + const taskStartHandler = (task) => { ... } + this.boundHandlers.set('task_start', taskStartHandler) + agentService.on('task_start', taskStartHandler) + // ... repeat for other handlers +} + +private cleanup(): void { + this.boundHandlers.forEach((handler, event) => { + agentService.removeListener(event, handler) + }) + this.boundHandlers.clear() +} +``` + +### 3.2 Process Signal Handlers Without Cleanup + +**File:** `src/cli/streaming-orchestrator.ts:1469` + +```typescript +process.on('SIGINT', () => this.gracefulExit()) +``` + +**Problem:** Signal handlers are registered but never removed. Multiple instances will stack handlers. + +### 3.3 Files with Listener Cleanup Issues + +| File | Issue | +|------|-------| +| `streaming-orchestrator.ts` | agentService listeners never removed | +| `index.ts` | Global process handlers accumulate | +| `main-orchestrator.ts` | vmOrchestrator event listeners | +| `services/streamtty/safety-guard.ts` | Stream timeout maps may leak | + +--- + +## 4. HIGH: Resource Disposal Issues + +### 4.1 Services Without Dispose Method + +Several key services lack proper `dispose()` implementation: + +| Service | File | Issue | +|---------|------|-------| +| `StreamingOrchestrator` | streaming-orchestrator.ts | No dispose, only partial teardown | +| `ChatManager` | chat/chat-manager.ts | No dispose, LRU cache persists | +| `ConfigManager` | core/config-manager.ts | No cleanup mechanism | +| `InputQueue` | core/input-queue.ts | No disposal | + +### 4.2 Incomplete Resource Cleanup Chain + +**File:** `src/cli/main-orchestrator.ts:135-155` + +```typescript +private async cleanup(): Promise { + // Stop LSP servers + const lspServers = lspService.getServerStatus() + for (const server of lspServers) { + if (server.status === 'running') { + await lspService.stopServer(server.name.toLowerCase()) + } + } + + // Clear caches - BUT THESE ARE OPTIONAL + try { + if ('clearAllCaches' in memoryService) + (memoryService as any).clearAllCaches() + // ... more optional cleanup + } catch (_error) { + // Silent fallback - cache cleanup is non-critical + } +} +``` + +**Problem:** Cache cleanup is wrapped in try-catch with silent failure. If one cache fails, others may not be cleared. + +### 4.3 Missing readline.close() in Some Paths + +**File:** `src/cli/streaming-orchestrator.ts` + +The `teardownInterface()` method removes listeners but doesn't close the readline interface in all paths: + +```typescript +private teardownInterface(): void { + try { + if (this.keypressHandler) { + process.stdin.removeListener('keypress', this.keypressHandler) + this.keypressHandler = undefined + } + // NOTE: this.rl is never closed here! + } catch { + // ignore + } +} +``` + +--- + +## 5. MEDIUM: Interval/Timeout Cleanup Issues + +### 5.1 Tracked Intervals/Timeouts Per File + +| File | Intervals | Properly Cleaned | +|------|-----------|------------------| +| `streaming-orchestrator.ts` | 4 | NO | +| `autonomous-claude-interface.ts` | 2 | YES | +| `ui/advanced-cli-ui.ts` | 2 | YES | +| `ui/vm-status-indicator.ts` | 1 | YES | +| `ui/vm-keyboard-controls.ts` | 2 | YES | +| `lsp/lsp-manager.ts` | 1 | YES | +| `services/streamtty/stream-batcher.ts` | 2 | YES | +| `utils/memory-manager.ts` | 1 | YES | +| `persistence/work-session-manager.ts` | 1 | YES | +| `context/vector-store-abstraction.ts` | 1 | YES | +| `providers/nikdrive/nikdrive-provider.ts` | 1 | YES | + +### 5.2 Good Cleanup Pattern Example + +**File:** `src/cli/chat/autonomous-claude-interface.ts:1596-1604` + +```typescript +// Clear intervals - CORRECT PATTERN +if (this.streamOptimizationInterval) { + clearInterval(this.streamOptimizationInterval) + this.streamOptimizationInterval = undefined +} +if (this.tokenOptimizationInterval) { + clearInterval(this.tokenOptimizationInterval) + this.tokenOptimizationInterval = undefined +} +``` + +--- + +## 6. MEDIUM: Async Operation Issues + +### 6.1 Fire-and-Forget Async Calls + +**File:** `src/cli/streaming-orchestrator.ts:1112-1119` + +```typescript +import('./core/input-queue') + .then(({ inputQueue }) => inputQueue.disableBypass()) + .then(() => { + try { + ;(global as any).__nikCLI?.resumePromptAndRender?.() + } catch {} + }) + .catch(() => {}) +``` + +**Problems:** +1. Dynamic import without error handling +2. Chained `.catch(() => {})` silently swallows all errors +3. No way to know if bypass was disabled + +### 6.2 Unhandled Promise Rejection Risk + +**File:** `src/cli/streaming-orchestrator.ts:1405` + +```typescript +if (this.activeVMAgent) { + this.cleanupVMAgent().catch(() => {}) +} +``` + +### 6.3 Missing await in Critical Paths + +**File:** `src/cli/streaming-orchestrator.ts:347-354` + +```typescript +// If locked, queue asynchronously (fire and forget) +this.stateLocks.acquire('messageQueue').then((release) => { + try { + this.messageQueue.push(message) + } finally { + release() + } +}) +// No await - message order not guaranteed +``` + +--- + +## 7. Additional Quality Issues + +### 7.1 Global State Pollution + +**File:** `src/cli/streaming-orchestrator.ts:110-111` + +```typescript +// Expose streaming orchestrator globally for VM agent communications +;(global as any).__streamingOrchestrator = this +``` + +Multiple files use global state: +- `__streamingOrchestrator` +- `__nikCLI` +- `__shouldInterrupt` + +This causes: +- Testing difficulties +- Memory leaks (globals never garbage collected) +- Race conditions in concurrent contexts + +### 7.2 Type Safety Issues + +**Pattern:** Excessive use of `any` type + +```typescript +private activeVMAgent?: any +private cliInstance: any +const metrics = Object.fromEntries(this.adaptiveMetrics) // loses type info +``` + +### 7.3 Console Logging in Production Code + +Found **100+ console.log/console.error** calls in production code paths that should use structured logging. + +--- + +## 8. Recommendations Summary + +### Immediate Actions (Critical) + +1. **Fix streaming-orchestrator intervals** - Store and cleanup the 4 intervals in `startMessageProcessor()` + +2. **Add dispose methods** to: + - `StreamingOrchestrator` + - `ChatManager` + - All singleton services + +3. **Fix listener leaks** in `setupServiceListeners()` - Store handlers and remove on cleanup + +### Short-term Actions (High) + +4. **Replace empty catch blocks** with proper error handling or logging + +5. **Add finally blocks** for generator cleanup in agent execution + +6. **Remove global state** pollution - use dependency injection + +### Long-term Actions (Medium) + +7. **Implement WeakMap/WeakRef** for subscriber management in EventBus + +8. **Add TypeScript strict mode** and reduce `any` usage + +9. **Implement structured logging** to replace console.log + +10. **Add automated memory leak detection** in test suite + +--- + +## 9. Files Requiring Immediate Attention + +| Priority | File | Issues | +|----------|------|--------| +| CRITICAL | `streaming-orchestrator.ts` | 4 leaked intervals, listener leaks, empty catches | +| HIGH | `services/agent-service.ts` | Empty catches, generator cleanup | +| HIGH | `main-orchestrator.ts` | Silent error handling, incomplete cleanup | +| HIGH | `automation/agents/event-bus.ts` | Subscriber accumulation | +| MEDIUM | `chat/autonomous-claude-interface.ts` | Fire-and-forget async | +| MEDIUM | `index.ts` | Process handler accumulation | + +--- + +## 10. Testing Recommendations + +1. **Add memory leak tests** using Node.js `--expose-gc` flag +2. **Test cleanup paths** for all services +3. **Verify no zombie intervals** after shutdown +4. **Test event listener counts** before/after operations +5. **Add integration tests** for graceful shutdown + +--- + +## 11. Additional Findings - Subfolder Analysis + +### 11.1 Empty Catch Blocks in nik-cli.ts + +**File:** `src/cli/nik-cli.ts` +**Severity:** MEDIUM +**Count:** 60+ instances + +The main CLI file contains numerous empty catch blocks that silently swallow errors: + +| Line | Pattern | +|------|---------| +| 388 | `} catch { }` | +| 485 | `} catch {` | +| 497 | `} catch {` | +| 812, 817, 849, 876, 916, 921 | Multiple empty catches | +| 1128, 1131, 1137, 1140 | Clustered empty catches | +| 1979, 2103, 2670, 2682 | Silent error handling | +| ... | 50+ more instances | + +### 11.2 setInterval Without Cleanup + +Multiple files create intervals that are never stored or cleaned up: + +| File | Line | Interval Description | +|------|------|---------------------| +| `providers/memory/mem0-provider.ts` | 712 | Hourly cleanup - NEVER stored | +| `services/ai-completion-service.ts` | 67 | Cache cleanup every minute - NEVER stored | +| `browser/browser-container-manager.ts` | 547 | Container cleanup every 5 min - NEVER stored | +| `middleware/performance-middleware.ts` | 375 | Performance metrics - NEVER stored | +| `automation/agents/agent-router.ts` | 556, 564 | Route cleanup + metrics - NEVER stored | +| `middleware/middleware-manager.ts` | 282 | Middleware cleanup - NEVER stored | +| `virtualized-agents/security/token-manager.ts` | 327 | Token cleanup - NEVER stored | +| `core/enhanced-tool-router.ts` | 69 | Cache cleanup - NEVER stored | +| `ui/theme-manager.ts` | 71 | Theme sync - NEVER stored | +| `monitoring/alerting/deduplicator.ts` | 60 | Alert cleanup - NEVER stored | +| `core/mcp-client.ts` | 822 | Health check - NEVER stored | +| `monitoring/metrics/prometheus-exporter.ts` | 112 | Metrics collection - NEVER stored | + +### 11.3 Files with Proper Cleanup (Good Examples) + +These files correctly handle interval cleanup: + +| File | Pattern | +|------|---------| +| `chat/autonomous-claude-interface.ts` | Stores intervals, clears in cleanup() | +| `index.ts` | Stores messageProcessorInterval, clears in cleanup() | +| `persistence/work-session-manager.ts` | Stores autoSaveTimer, clears properly | +| `lsp/lsp-manager.ts` | Stores cleanupInterval, clears properly | +| `services/ad-display-manager.ts` | Stores updateInterval/syncInterval | +| `providers/nikdrive/nikdrive-provider.ts` | Stores healthCheckInterval | +| `context/vector-store-abstraction.ts` | Stores healthCheckInterval | + +--- + +## 12. Fixes Applied + +### 12.1 streaming-orchestrator.ts (FIXED) + +- ✅ Added `messageProcessorIntervals: NodeJS.Timeout[]` for interval storage +- ✅ Added `serviceListenerHandlers: Map` for listener cleanup +- ✅ Added `stopMessageProcessor()` to clear all intervals +- ✅ Added `removeServiceListeners()` to remove event listeners +- ✅ Added comprehensive `dispose()` method +- ✅ Fixed all empty catch blocks with proper logging +- ✅ Stored SIGINT handler for cleanup + +### 12.2 chat-manager.ts (FIXED) + +- ✅ Added `dispose()` method for LRU cache cleanup + +### 12.3 agent-service.ts (FIXED) + +- ✅ Fixed empty catch blocks with debug logging +- ✅ Improved dispose() error logging + +--- + +## 13. Remaining Work + +### High Priority +1. Fix interval leaks in 12+ files listed in section 11.2 +2. Address 60+ empty catch blocks in nik-cli.ts +3. Add dispose() methods to singleton services + +### Medium Priority +4. Add WeakMap/WeakRef for subscriber management +5. Implement structured logging to replace console.log +6. Add automated memory leak detection in tests + +--- + +*Report updated: 2025-12-30* +*Fixes applied to streaming-orchestrator.ts, chat-manager.ts, agent-service.ts* diff --git a/src/cli/automation/agents/agent-router.ts b/src/cli/automation/agents/agent-router.ts index 76c9b66c..4cd6e5e8 100644 --- a/src/cli/automation/agents/agent-router.ts +++ b/src/cli/automation/agents/agent-router.ts @@ -19,6 +19,9 @@ export class AgentRouter { averageRoutingTime: 0, agentUtilization: new Map(), } + // FIXED: Store intervals for proper cleanup + private cleanupInterval?: NodeJS.Timeout + private metricsInterval?: NodeJS.Timeout constructor() { this.eventBus = EventBus.getInstance() @@ -552,8 +555,9 @@ export class AgentRouter { * Setup performance optimizations */ private setupPerformanceOptimizations(): void { + // FIXED: Store interval references for cleanup // Periodic cleanup of completed routes - setInterval( + this.cleanupInterval = setInterval( () => { this.cleanupCompletedRoutes() }, @@ -561,11 +565,28 @@ export class AgentRouter { ) // Every 5 minutes // Performance metrics optimization - setInterval(() => { + this.metricsInterval = setInterval(() => { this.optimizeAgentUtilization() }, 30 * 1000) // Every 30 seconds } + /** + * FIXED: Dispose method to clean up resources + */ + public dispose(): void { + if (this.cleanupInterval) { + clearInterval(this.cleanupInterval) + this.cleanupInterval = undefined + } + if (this.metricsInterval) { + clearInterval(this.metricsInterval) + this.metricsInterval = undefined + } + this.agents.clear() + this.taskQueue = [] + this.activeRoutes.clear() + } + /** * Clean up completed routes and old metrics */ diff --git a/src/cli/browser/browser-container-manager.ts b/src/cli/browser/browser-container-manager.ts index 18e99fdd..15dc2675 100644 --- a/src/cli/browser/browser-container-manager.ts +++ b/src/cli/browser/browser-container-manager.ts @@ -24,6 +24,8 @@ export class BrowserContainerManager extends ContainerManager { private readonly baseNoVNCPort = 6080 private readonly basePlaywrightPort = 9222 private readonly baseVNCPort = 5900 + // FIXED: Store cleanup interval for proper disposal + private cleanupInterval?: NodeJS.Timeout constructor() { super() @@ -543,11 +545,24 @@ export class BrowserContainerManager extends ContainerManager { advancedUI.logFunctionUpdate('info', `Container stopped: ${containerId.slice(0, 12)}`, '⏹️') }) + // FIXED: Store interval reference for cleanup // Periodic cleanup of inactive containers - setInterval(() => { + this.cleanupInterval = setInterval(() => { this.cleanupInactiveBrowserContainers() }, 300000) // Every 5 minutes } + + /** + * FIXED: Dispose method to clean up resources + */ + public dispose(): void { + if (this.cleanupInterval) { + clearInterval(this.cleanupInterval) + this.cleanupInterval = undefined + } + this.activeBrowserContainers.clear() + this.removeAllListeners() + } } // Type definitions diff --git a/src/cli/chat/chat-manager.ts b/src/cli/chat/chat-manager.ts index 11ab721f..83dcdbf5 100644 --- a/src/cli/chat/chat-manager.ts +++ b/src/cli/chat/chat-manager.ts @@ -313,6 +313,15 @@ export class ChatManager { currentSessionMessages: this.currentSession?.messages.length || 0, } } + + /** + * FIXED: Added dispose method to clean up resources + * Prevents memory leaks from LRU cache and session data + */ + dispose(): void { + this.sessions.clear() + this.currentSession = null + } } export const chatManager = new ChatManager() diff --git a/src/cli/core/enhanced-tool-router.ts b/src/cli/core/enhanced-tool-router.ts index cfa1de6f..e2b2b04f 100644 --- a/src/cli/core/enhanced-tool-router.ts +++ b/src/cli/core/enhanced-tool-router.ts @@ -55,6 +55,8 @@ export class EnhancedToolRouter extends ToolRouter { private projectContextCache: ProjectContext | null = null private lastAnalysisTime: number = 0 private smartSuggestionCache: Map = new Map() + // FIXED: Store cleanup interval for proper disposal + private cleanupInterval?: NodeJS.Timeout constructor() { super() @@ -65,8 +67,9 @@ export class EnhancedToolRouter extends ToolRouter { // Initialize user pattern learning this.loadUserPatterns() + // FIXED: Store interval reference for cleanup // Set up cache cleanup - setInterval(() => this.cleanupCache(), 5 * 60 * 1000) // 5 minutes + this.cleanupInterval = setInterval(() => this.cleanupCache(), 5 * 60 * 1000) // 5 minutes } /** @@ -635,6 +638,19 @@ export class EnhancedToolRouter extends ToolRouter { toKeep.forEach(([key, value]) => this.smartSuggestionCache.set(key, value)) } } + + /** + * FIXED: Dispose method to clean up resources + */ + public dispose(): void { + if (this.cleanupInterval) { + clearInterval(this.cleanupInterval) + this.cleanupInterval = undefined + } + this.smartSuggestionCache.clear() + this.userPatterns.clear() + this.recentCommands = [] + } } export const enhancedToolRouter = new EnhancedToolRouter() diff --git a/src/cli/providers/memory/mem0-provider.ts b/src/cli/providers/memory/mem0-provider.ts index 8769d71a..da4ad061 100644 --- a/src/cli/providers/memory/mem0-provider.ts +++ b/src/cli/providers/memory/mem0-provider.ts @@ -67,6 +67,8 @@ export class Mem0Provider extends EventEmitter { private isInitialized = false private memoriesDir: string private currentUserId: string | null = null + // FIXED: Store cleanup interval for proper disposal + private cleanupInterval?: NodeJS.Timeout constructor() { super() @@ -708,8 +710,9 @@ export class Mem0Provider extends EventEmitter { } private setupAutoCleanup(): void { + // FIXED: Store interval reference for cleanup // Run cleanup every hour - setInterval( + this.cleanupInterval = setInterval( async () => { await this.cleanupOldMemories() }, @@ -717,6 +720,18 @@ export class Mem0Provider extends EventEmitter { ) } + /** + * FIXED: Dispose method to clean up resources + */ + public dispose(): void { + if (this.cleanupInterval) { + clearInterval(this.cleanupInterval) + this.cleanupInterval = undefined + } + this.memories.clear() + this.removeAllListeners() + } + private async cleanupOldMemories(): Promise { const cutoffTime = Date.now() - this.config.importance_decay_days * 24 * 60 * 60 * 1000 let cleanedCount = 0 diff --git a/src/cli/services/agent-service.ts b/src/cli/services/agent-service.ts index 55931090..d295715d 100644 --- a/src/cli/services/agent-service.ts +++ b/src/cli/services/agent-service.ts @@ -447,8 +447,9 @@ export class AgentService extends EventEmitter { // Attempt to close the generator gracefully try { await (generator as any)?.return?.() - } catch { - /* ignore */ + } catch (err: any) { + // FIXED: Log generator cleanup errors for debugging + console.debug?.('Generator cleanup during cancel:', err?.message || 'unknown') } throw new Error('Cancelled by user') } @@ -584,8 +585,9 @@ export class AgentService extends EventEmitter { if (gen && typeof (gen as any).return === 'function') { try { ;(gen as any).return() - } catch { - /* ignore */ + } catch (err: any) { + // FIXED: Log generator cleanup errors for debugging + console.debug?.('Generator return cleanup:', err?.message || 'unknown') } } console.log(chalk.yellow(`⏹️ Cancellation requested for task ${taskId}`)) @@ -627,8 +629,9 @@ export class AgentService extends EventEmitter { this.activeTasks.clear() this.taskQueue = [] this.removeAllListeners() - } catch { - // ignore + } catch (err: any) { + // FIXED: Log instead of silently ignoring dispose errors + console.error('AgentService dispose warning:', err?.message || 'unknown') } } diff --git a/src/cli/services/ai-completion-service.ts b/src/cli/services/ai-completion-service.ts index 517ed146..85cdef4b 100644 --- a/src/cli/services/ai-completion-service.ts +++ b/src/cli/services/ai-completion-service.ts @@ -61,10 +61,13 @@ export class AICompletionService { private readonly cacheExpiryMs = 5 * 60 * 1000 // 5 minutes private readonly maxCacheSize = 100 private readonly completionTimeout = 3000 // 3 seconds max for AI completion + // FIXED: Store interval for cleanup + private cleanupInterval?: NodeJS.Timeout constructor() { // Clear expired cache entries periodically - setInterval(() => this.cleanCache(), 60 * 1000) // Every minute + // FIXED: Store interval reference for proper cleanup + this.cleanupInterval = setInterval(() => this.cleanCache(), 60 * 1000) // Every minute } /** @@ -493,6 +496,17 @@ Provide 3-8 most relevant completions, ranked by relevance and confidence.` public clearCache(): void { this.cache.clear() } + + /** + * FIXED: Dispose method to clean up interval + */ + public dispose(): void { + if (this.cleanupInterval) { + clearInterval(this.cleanupInterval) + this.cleanupInterval = undefined + } + this.cache.clear() + } } export const aiCompletionService = new AICompletionService() diff --git a/src/cli/streaming-orchestrator.ts b/src/cli/streaming-orchestrator.ts index da47808f..d21a8eb0 100644 --- a/src/cli/streaming-orchestrator.ts +++ b/src/cli/streaming-orchestrator.ts @@ -88,6 +88,12 @@ class StreamingOrchestratorImpl extends EventEmitter { // 🔒 FIXED: Locks for preventing state corruption private stateLocks = new AsyncLock() + // 🔒 FIXED: Store interval/handler references for proper cleanup + private messageProcessorIntervals: NodeJS.Timeout[] = [] + private serviceListenerHandlers: Map void> = new Map() + private sigintHandler?: () => void + private isDisposed = false + constructor() { super() @@ -237,14 +243,15 @@ class StreamingOrchestratorImpl extends EventEmitter { if (process.stdin.isTTY && typeof this.originalRawMode === 'boolean') { ;(process.stdin as any).setRawMode(this.originalRawMode) } - } catch { - // ignore + } catch (err: any) { + // FIXED: Log instead of silently ignoring - teardown errors are non-critical + advancedUI.logError('info', `Interface teardown warning: ${err?.message || 'unknown'}`) } } private setupServiceListeners(): void { - // Agent events - agentService.on('task_start', (task) => { + // FIXED: Store handler references for cleanup + const taskStartHandler = (task: any) => { this.activeAgents.set(task.id, task) // Avoid duplicate logging if NikCLI is active @@ -256,9 +263,9 @@ class StreamingOrchestratorImpl extends EventEmitter { metadata: { agentId: task.id, agentType: task.agentType }, }) } - }) + } - agentService.on('task_progress', (task, update) => { + const taskProgressHandler = (task: any, update: any) => { this.queueMessage({ type: 'agent', content: `📊 ${task.agentType}: ${update.progress}% ${update.description || ''}`, @@ -266,17 +273,17 @@ class StreamingOrchestratorImpl extends EventEmitter { agentId: task.id, progress: update.progress, }) - }) + } - agentService.on('tool_use', (task, update) => { + const toolUseHandler = (task: any, update: any) => { this.queueMessage({ type: 'tool', content: ` ${task.agentType} using ${update.tool}: ${update.description}`, metadata: { agentId: task.id, tool: update.tool }, }) - }) + } - agentService.on('task_complete', (task) => { + const taskCompleteHandler = (task: any) => { this.activeAgents.delete(task.id) if (task.status === 'completed') { this.queueMessage({ @@ -297,7 +304,29 @@ class StreamingOrchestratorImpl extends EventEmitter { // Check if all background tasks are complete and show prompt this.checkAndReturnToDefaultMode() + } + + // Store handlers for cleanup + this.serviceListenerHandlers.set('task_start', taskStartHandler) + this.serviceListenerHandlers.set('task_progress', taskProgressHandler) + this.serviceListenerHandlers.set('tool_use', toolUseHandler) + this.serviceListenerHandlers.set('task_complete', taskCompleteHandler) + + // Register listeners + agentService.on('task_start', taskStartHandler) + agentService.on('task_progress', taskProgressHandler) + agentService.on('tool_use', toolUseHandler) + agentService.on('task_complete', taskCompleteHandler) + } + + /** + * FIXED: Remove service listeners on cleanup + */ + private removeServiceListeners(): void { + this.serviceListenerHandlers.forEach((handler, event) => { + agentService.removeListener(event, handler) }) + this.serviceListenerHandlers.clear() } private async queueUserInput(input: string): Promise { @@ -1083,7 +1112,10 @@ class StreamingOrchestratorImpl extends EventEmitter { try { process.env.NIKCLI_COMPACT = '1' process.env.NIKCLI_SUPER_COMPACT = '1' - } catch {} + } catch (err: any) { + // FIXED: Non-critical env var operation - log for debugging + advancedUI.logError('info', `Mode switch env warning: ${err?.message || 'unknown'}`) + } console.log(chalk.green('\\n✓ plan mode on ') + chalk.dim('(shift+tab to cycle)')) } else if (this.context.planMode && !this.context.autoAcceptEdits && !this.context.vmMode) { // Plan → Auto-accept @@ -1106,7 +1138,10 @@ class StreamingOrchestratorImpl extends EventEmitter { try { delete (process.env as any).NIKCLI_COMPACT delete (process.env as any).NIKCLI_SUPER_COMPACT - } catch {} + } catch (err: any) { + // FIXED: Non-critical env var cleanup - log for debugging + advancedUI.logError('info', `Mode switch env cleanup warning: ${err?.message || 'unknown'}`) + } // Reset processing and show prompt to accept new input this.processingMessage = false import('./core/input-queue') @@ -1114,9 +1149,15 @@ class StreamingOrchestratorImpl extends EventEmitter { .then(() => { try { ;(global as any).__nikCLI?.resumePromptAndRender?.() - } catch {} + } catch (err: any) { + // FIXED: Non-critical UI callback - log for debugging + advancedUI.logError('info', `Prompt resume warning: ${err?.message || 'unknown'}`) + } + }) + .catch((err: any) => { + // FIXED: Non-critical import/bypass operation - log for debugging + advancedUI.logError('info', `Input queue operation warning: ${err?.message || 'unknown'}`) }) - .catch(() => {}) this.showPrompt() // Cleanup VM agent when exiting VM mode @@ -1252,53 +1293,75 @@ class StreamingOrchestratorImpl extends EventEmitter { } private startMessageProcessor(): void { + // FIXED: Store interval references for proper cleanup // Process messages every 100ms - setInterval(() => { - // Check for interruption - const shouldInterrupt = (global as any).__shouldInterrupt - if (shouldInterrupt?.()) { - return - } + this.messageProcessorIntervals.push( + setInterval(() => { + if (this.isDisposed) return + // Check for interruption + const shouldInterrupt = (global as any).__shouldInterrupt + if (shouldInterrupt?.()) { + return + } - if (!this.processingMessage) { - this.processNextMessage() - } - this.updateContextCounter() - }, 100) + if (!this.processingMessage) { + this.processNextMessage() + } + this.updateContextCounter() + }, 100) + ) // Auto-absorb messages every 5 seconds - setInterval(() => { - // Check for interruption - const shouldInterrupt = (global as any).__shouldInterrupt - if (shouldInterrupt?.()) { - return - } + this.messageProcessorIntervals.push( + setInterval(() => { + if (this.isDisposed) return + // Check for interruption + const shouldInterrupt = (global as any).__shouldInterrupt + if (shouldInterrupt?.()) { + return + } - this.absorbCompletedMessages() - }, 5000) + this.absorbCompletedMessages() + }, 5000) + ) // Process queued inputs every 2 seconds - setInterval(() => { - // Check for interruption - const shouldInterrupt = (global as any).__shouldInterrupt - if (shouldInterrupt?.()) { - return - } + this.messageProcessorIntervals.push( + setInterval(() => { + if (this.isDisposed) return + // Check for interruption + const shouldInterrupt = (global as any).__shouldInterrupt + if (shouldInterrupt?.()) { + return + } - this.processQueuedInputs() - }, 2000) + this.processQueuedInputs() + }, 2000) + ) // ⚡︎ Optimize token allocation every 30 seconds - setInterval(() => { - // Check for interruption - const shouldInterrupt = (global as any).__shouldInterrupt - if (shouldInterrupt?.()) { - return - } + this.messageProcessorIntervals.push( + setInterval(() => { + if (this.isDisposed) return + // Check for interruption + const shouldInterrupt = (global as any).__shouldInterrupt + if (shouldInterrupt?.()) { + return + } - if (this.cognitiveEnabled) { - } - }, 30000) + if (this.cognitiveEnabled) { + // Token optimization logic + } + }, 30000) + ) + } + + /** + * FIXED: Clear all message processor intervals + */ + private stopMessageProcessor(): void { + this.messageProcessorIntervals.forEach((interval) => clearInterval(interval)) + this.messageProcessorIntervals = [] } private async processQueuedInputs(): Promise { @@ -1398,11 +1461,17 @@ class StreamingOrchestratorImpl extends EventEmitter { this.context.vmMode = false try { diffManager.setAutoAccept(false) - } catch {} + } catch (err: any) { + // FIXED: Non-critical diff manager operation - log for debugging + advancedUI.logError('info', `Diff manager reset warning: ${err?.message || 'unknown'}`) + } // Cleanup VM agent if any if (this.activeVMAgent) { - this.cleanupVMAgent().catch(() => {}) + this.cleanupVMAgent().catch((err: any) => { + // FIXED: Non-critical VM cleanup - log for debugging + advancedUI.logError('info', `VM cleanup warning: ${err?.message || 'unknown'}`) + }) } // Show prompt after a small delay to ensure messages are processed @@ -1428,16 +1497,58 @@ class StreamingOrchestratorImpl extends EventEmitter { // In production, you'd wait for agents to complete } + // FIXED: Call comprehensive dispose for all cleanup + this.dispose() + + console.log(chalk.green('✓ Goodbye!')) + process.exit(0) + } + + /** + * FIXED: Comprehensive dispose method to clean up all resources + * Prevents memory leaks from intervals, listeners, and handlers + */ + public dispose(): void { + if (this.isDisposed) return + this.isDisposed = true + + // Stop all message processor intervals + this.stopMessageProcessor() + + // Remove service listeners + this.removeServiceListeners() + // Cleanup keyboard handlers and raw mode this.teardownInterface() + // Close readline interface + if (this.rl) { + this.rl.close() + this.rl = undefined + } + + // Remove SIGINT handler + if (this.sigintHandler) { + process.removeListener('SIGINT', this.sigintHandler) + this.sigintHandler = undefined + } + // Cleanup VM agent if active if (this.activeVMAgent) { - this.cleanupVMAgent().catch(() => {}) + this.cleanupVMAgent().catch((err) => { + console.error('VM cleanup error:', err.message) + }) } - console.log(chalk.green('✓ Goodbye!')) - process.exit(0) + // Clear global reference + if ((global as any).__streamingOrchestrator === this) { + delete (global as any).__streamingOrchestrator + } + + // Clear message queue and panels + this.messageQueue = [] + this.panels.clear() + this.activeAgents.clear() } async start(): Promise { @@ -1465,8 +1576,9 @@ class StreamingOrchestratorImpl extends EventEmitter { // Start the interface this.showPrompt() - // Ensure SIGINT exits gracefully even when TTY is not controlling - process.on('SIGINT', () => this.gracefulExit()) + // FIXED: Store SIGINT handler for cleanup + this.sigintHandler = () => this.gracefulExit() + process.on('SIGINT', this.sigintHandler) return new Promise((resolve) => { this.rl?.on('close', resolve)