mirror of
https://github.com/RooVetGit/Roo-Code.git
synced 2026-09-05 08:10:14 +00:00
fix: improve memory leak prevention in Task and ClineProvider disposal
- Ensure RooIgnoreController is always disposed even if errors occur - Add finally block to guarantee reference cleanup - Improve event listener cleanup order in ClineProvider - Add defensive error handling for all disposal operations - Clear array references to help garbage collection - Fix TypeScript error in removeClineFromStack method Fixes #7561
This commit is contained in:
parent
63b71d8299
commit
a2030b8caf
3 changed files with 131 additions and 41 deletions
|
|
@ -1425,11 +1425,11 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
public dispose(): void {
|
||||
console.log(`[Task#dispose] disposing task ${this.taskId}.${this.instanceId}`)
|
||||
|
||||
// Remove all event listeners to prevent memory leaks.
|
||||
// Remove all event listeners first to prevent any callbacks during disposal
|
||||
try {
|
||||
this.removeAllListeners()
|
||||
} catch (error) {
|
||||
console.error("Error removing event listeners:", error)
|
||||
console.error(`[Task#dispose] Error removing event listeners for task ${this.taskId}:`, error)
|
||||
}
|
||||
|
||||
// Stop waiting for child task completion.
|
||||
|
|
@ -1438,60 +1438,78 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike {
|
|||
this.pauseInterval = undefined
|
||||
}
|
||||
|
||||
// Unsubscribe from bridge if enabled
|
||||
if (this.enableBridge) {
|
||||
BridgeOrchestrator.getInstance()
|
||||
?.unsubscribeFromTask(this.taskId)
|
||||
.catch((error) =>
|
||||
console.error(
|
||||
`[Task#dispose] BridgeOrchestrator#unsubscribeFromTask() failed: ${error instanceof Error ? error.message : String(error)}`,
|
||||
`[Task#dispose] BridgeOrchestrator#unsubscribeFromTask() failed for task ${this.taskId}: ${error instanceof Error ? error.message : String(error)}`,
|
||||
),
|
||||
)
|
||||
}
|
||||
|
||||
// Release any terminals associated with this task.
|
||||
try {
|
||||
// Release any terminals associated with this task.
|
||||
TerminalRegistry.releaseTerminalsForTask(this.taskId)
|
||||
} catch (error) {
|
||||
console.error("Error releasing terminals:", error)
|
||||
console.error(`[Task#dispose] Error releasing terminals for task ${this.taskId}:`, error)
|
||||
}
|
||||
|
||||
// Close browsers
|
||||
try {
|
||||
this.urlContentFetcher.closeBrowser()
|
||||
} catch (error) {
|
||||
console.error("Error closing URL content fetcher browser:", error)
|
||||
console.error(`[Task#dispose] Error closing URL content fetcher browser for task ${this.taskId}:`, error)
|
||||
}
|
||||
|
||||
try {
|
||||
this.browserSession.closeBrowser()
|
||||
} catch (error) {
|
||||
console.error("Error closing browser session:", error)
|
||||
console.error(`[Task#dispose] Error closing browser session for task ${this.taskId}:`, error)
|
||||
}
|
||||
|
||||
try {
|
||||
if (this.rooIgnoreController) {
|
||||
// CRITICAL: Always dispose RooIgnoreController to prevent memory leaks
|
||||
// This must be done even if it throws an error
|
||||
if (this.rooIgnoreController) {
|
||||
try {
|
||||
this.rooIgnoreController.dispose()
|
||||
} catch (error) {
|
||||
console.error(
|
||||
`[Task#dispose] CRITICAL: Error disposing RooIgnoreController for task ${this.taskId}:`,
|
||||
error,
|
||||
)
|
||||
} finally {
|
||||
// Always clear the reference to allow garbage collection
|
||||
this.rooIgnoreController = undefined
|
||||
}
|
||||
} catch (error) {
|
||||
console.error("Error disposing RooIgnoreController:", error)
|
||||
// This is the critical one for the leak fix.
|
||||
}
|
||||
|
||||
// Dispose file context tracker
|
||||
try {
|
||||
this.fileContextTracker.dispose()
|
||||
} catch (error) {
|
||||
console.error("Error disposing file context tracker:", error)
|
||||
console.error(`[Task#dispose] Error disposing file context tracker for task ${this.taskId}:`, error)
|
||||
}
|
||||
|
||||
// Revert any pending diff changes
|
||||
try {
|
||||
// If we're not streaming then `abortStream` won't be called.
|
||||
if (this.isStreaming && this.diffViewProvider.isEditing) {
|
||||
this.diffViewProvider.revertChanges().catch(console.error)
|
||||
this.diffViewProvider
|
||||
.revertChanges()
|
||||
.catch((error) =>
|
||||
console.error(`[Task#dispose] Error reverting diff changes for task ${this.taskId}:`, error),
|
||||
)
|
||||
}
|
||||
} catch (error) {
|
||||
console.error("Error reverting diff changes:", error)
|
||||
console.error(`[Task#dispose] Error checking/reverting diff changes for task ${this.taskId}:`, error)
|
||||
}
|
||||
|
||||
// Clear any remaining references to help garbage collection
|
||||
this.assistantMessageContent = []
|
||||
this.userMessageContent = []
|
||||
this.apiConversationHistory = []
|
||||
this.clineMessages = []
|
||||
}
|
||||
|
||||
public async abortTask(isAbandoned = false) {
|
||||
|
|
|
|||
|
|
@ -117,8 +117,11 @@ describe("Task dispose method", () => {
|
|||
// Call dispose - should not throw
|
||||
expect(() => task.dispose()).not.toThrow()
|
||||
|
||||
// Verify error was logged
|
||||
expect(consoleErrorSpy).toHaveBeenCalledWith("Error removing event listeners:", expect.any(Error))
|
||||
// Verify error was logged with the improved format
|
||||
expect(consoleErrorSpy).toHaveBeenCalledWith(
|
||||
`[Task#dispose] Error removing event listeners for task ${task.taskId}:`,
|
||||
expect.any(Error),
|
||||
)
|
||||
|
||||
// Restore
|
||||
task.removeAllListeners = originalRemoveAllListeners
|
||||
|
|
|
|||
|
|
@ -350,10 +350,33 @@ export class ClineProvider
|
|||
}
|
||||
|
||||
// Pop the top Cline instance from the stack.
|
||||
let task = this.clineStack.pop()
|
||||
const task = this.clineStack.pop()
|
||||
|
||||
if (task) {
|
||||
task.emit(RooCodeEventName.TaskUnfocused)
|
||||
const taskId = task.taskId
|
||||
const instanceId = task.instanceId
|
||||
|
||||
// Emit unfocused event before cleanup
|
||||
try {
|
||||
task.emit(RooCodeEventName.TaskUnfocused)
|
||||
} catch (error) {
|
||||
this.log(`[ClineProvider#removeClineFromStack] Error emitting TaskUnfocused for ${taskId}: ${error}`)
|
||||
}
|
||||
|
||||
// Remove event listeners BEFORE aborting to prevent any callbacks during cleanup
|
||||
const cleanupFunctions = this.taskEventListeners.get(task)
|
||||
if (cleanupFunctions) {
|
||||
cleanupFunctions.forEach((cleanup) => {
|
||||
try {
|
||||
cleanup()
|
||||
} catch (error) {
|
||||
this.log(
|
||||
`[ClineProvider#removeClineFromStack] Error cleaning up event listener for ${taskId}: ${error}`,
|
||||
)
|
||||
}
|
||||
})
|
||||
this.taskEventListeners.delete(task)
|
||||
}
|
||||
|
||||
try {
|
||||
// Abort the running task and set isAbandoned to true so
|
||||
|
|
@ -361,21 +384,9 @@ export class ClineProvider
|
|||
await task.abortTask(true)
|
||||
} catch (e) {
|
||||
this.log(
|
||||
`[ClineProvider#removeClineFromStack] abortTask() failed ${task.taskId}.${task.instanceId}: ${e.message}`,
|
||||
`[ClineProvider#removeClineFromStack] abortTask() failed ${taskId}.${instanceId}: ${e.message}`,
|
||||
)
|
||||
}
|
||||
|
||||
// Remove event listeners before clearing the reference.
|
||||
const cleanupFunctions = this.taskEventListeners.get(task)
|
||||
|
||||
if (cleanupFunctions) {
|
||||
cleanupFunctions.forEach((cleanup) => cleanup())
|
||||
this.taskEventListeners.delete(task)
|
||||
}
|
||||
|
||||
// Make sure no reference kept, once promises end it will be
|
||||
// garbage collected.
|
||||
task = undefined
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -418,6 +429,21 @@ export class ClineProvider
|
|||
async dispose() {
|
||||
this.log("Disposing ClineProvider...")
|
||||
|
||||
// Clear all event listeners for all tasks first
|
||||
for (const task of this.clineStack) {
|
||||
const cleanupFunctions = this.taskEventListeners.get(task)
|
||||
if (cleanupFunctions) {
|
||||
cleanupFunctions.forEach((cleanup) => {
|
||||
try {
|
||||
cleanup()
|
||||
} catch (error) {
|
||||
this.log(`[ClineProvider#dispose] Error cleaning up event listener: ${error}`)
|
||||
}
|
||||
})
|
||||
this.taskEventListeners.delete(task)
|
||||
}
|
||||
}
|
||||
|
||||
// Clear all tasks from the stack.
|
||||
while (this.clineStack.length > 0) {
|
||||
await this.removeClineFromStack()
|
||||
|
|
@ -425,6 +451,9 @@ export class ClineProvider
|
|||
|
||||
this.log("Cleared all tasks")
|
||||
|
||||
// Clear the task event listeners map completely
|
||||
this.taskEventListeners = new WeakMap()
|
||||
|
||||
if (this.view && "dispose" in this.view) {
|
||||
this.view.dispose()
|
||||
this.log("Disposed webview")
|
||||
|
|
@ -441,21 +470,61 @@ export class ClineProvider
|
|||
const x = this.disposables.pop()
|
||||
|
||||
if (x) {
|
||||
x.dispose()
|
||||
try {
|
||||
x.dispose()
|
||||
} catch (error) {
|
||||
this.log(`[ClineProvider#dispose] Error disposing disposable: ${error}`)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Clean up workspace tracker
|
||||
if (this._workspaceTracker) {
|
||||
try {
|
||||
this._workspaceTracker.dispose()
|
||||
} catch (error) {
|
||||
this.log(`[ClineProvider#dispose] Error disposing workspace tracker: ${error}`)
|
||||
}
|
||||
this._workspaceTracker = undefined
|
||||
}
|
||||
|
||||
// Unregister from MCP hub
|
||||
if (this.mcpHub) {
|
||||
try {
|
||||
await this.mcpHub.unregisterClient()
|
||||
} catch (error) {
|
||||
this.log(`[ClineProvider#dispose] Error unregistering MCP client: ${error}`)
|
||||
}
|
||||
this.mcpHub = undefined
|
||||
}
|
||||
|
||||
// Clean up marketplace manager
|
||||
if (this.marketplaceManager) {
|
||||
try {
|
||||
this.marketplaceManager.cleanup()
|
||||
} catch (error) {
|
||||
this.log(`[ClineProvider#dispose] Error cleaning up marketplace manager: ${error}`)
|
||||
}
|
||||
}
|
||||
|
||||
// Dispose custom modes manager
|
||||
if (this.customModesManager) {
|
||||
try {
|
||||
this.customModesManager.dispose()
|
||||
} catch (error) {
|
||||
this.log(`[ClineProvider#dispose] Error disposing custom modes manager: ${error}`)
|
||||
}
|
||||
}
|
||||
|
||||
this._workspaceTracker?.dispose()
|
||||
this._workspaceTracker = undefined
|
||||
await this.mcpHub?.unregisterClient()
|
||||
this.mcpHub = undefined
|
||||
this.marketplaceManager?.cleanup()
|
||||
this.customModesManager?.dispose()
|
||||
this.log("Disposed all disposables")
|
||||
ClineProvider.activeInstances.delete(this)
|
||||
|
||||
// Clean up any event listeners attached to this provider
|
||||
this.removeAllListeners()
|
||||
try {
|
||||
this.removeAllListeners()
|
||||
} catch (error) {
|
||||
this.log(`[ClineProvider#dispose] Error removing provider event listeners: ${error}`)
|
||||
}
|
||||
|
||||
McpServerManager.unregisterProvider(this)
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue