-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: Address memory leaks in API provider clients and Bedrock handler cache #4239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,8 @@ import { countTokens } from "../../utils/countTokens" | |
| * Base class for API providers that implements common functionality. | ||
| */ | ||
| export abstract class BaseProvider implements ApiHandler { | ||
| // constructor remains empty, no new properties added here | ||
|
|
||
| abstract createMessage( | ||
| systemPrompt: string, | ||
| messages: Anthropic.Messages.MessageParam[], | ||
|
|
@@ -32,4 +34,25 @@ export abstract class BaseProvider implements ApiHandler { | |
|
|
||
| return countTokens(content, { useWorker: true }) | ||
| } | ||
|
|
||
| /** | ||
| * Disposes of any resources held by the provider. | ||
| * Attempts common disposal methods on the client if it exists. | ||
| */ | ||
| public dispose(): void { | ||
| // Use reflection to find any property named 'client' on the instance | ||
| const clientProperty = (this as any).client | ||
| if (clientProperty) { | ||
| // Try common disposal methods that SDKs might have | ||
| if (typeof clientProperty.close === "function") { | ||
| clientProperty.close() | ||
| } else if (typeof clientProperty.destroy === "function") { | ||
| clientProperty.destroy() | ||
| } else if (typeof clientProperty.dispose === "function") { | ||
| clientProperty.dispose() | ||
| } | ||
| // Clear the reference on the instance | ||
| ;(this as any).client = undefined | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reflection-based approach using
Consider a more type-safe approach: // Option 1: Make dispose abstract and let each provider implement it
export abstract class BaseProvider implements ApiHandler {
abstract dispose(): void;
}
// Option 2: Define a client contract
export abstract class BaseProvider implements ApiHandler {
protected abstract getClient(): { dispose?: () => void } | undefined;
public dispose(): void {
const client = this.getClient();
client?.dispose?.();
}
}This would ensure each provider explicitly handles its own cleanup logic with proper type checking. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -1001,6 +1001,7 @@ export class Task extends EventEmitter<ClineEvents> { | |||
| this.browserSession.closeBrowser() | ||||
| this.rooIgnoreController?.dispose() | ||||
| this.fileContextTracker.dispose() | ||||
| this.api?.dispose?.() // Added this line | ||||
|
|
||||
| // If we're not streaming then `abortStream` (which reverts the diff | ||||
| // view changes) won't be called, so we need to revert the changes here. | ||||
|
|
@@ -1012,6 +1013,25 @@ export class Task extends EventEmitter<ClineEvents> { | |||
| await this.saveClineMessages() | ||||
| } | ||||
|
|
||||
| // Added new dispose method as per leak report suggestion | ||||
| public dispose(): void { | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This dispose method will directly conflict with your PR #4233.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, there are several PR that touch into the same concept of the Task class needing to dispose of everything it launches. |
||||
| console.log(`[subtasks] disposing task ${this.taskId}.${this.instanceId}`) | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think generally we have way to little logging to trace problems. I did mention this before on discord. |
||||
| this.abortTask(true) // Call abortTask to ensure all resources are released | ||||
|
|
||||
| // Explicitly call dispose on the api handler again, in case abortTask didn't catch it | ||||
| // or if dispose is called directly without abortTask. | ||||
| this.api?.dispose?.() | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that these are being called more than once.
This could lead to issues if the disposal method isn't idempotent. |
||||
|
|
||||
| // Clear any other task-specific resources if they weren't handled by abortTask | ||||
| if (this.pauseInterval) { | ||||
| clearInterval(this.pauseInterval) | ||||
| this.pauseInterval = undefined | ||||
| } | ||||
| // Ensure other disposables are handled if not already by abortTask | ||||
| // this.rooIgnoreController?.dispose(); // Already called in abortTask | ||||
| // this.fileContextTracker.dispose(); // Already called in abortTask | ||||
| } | ||||
|
|
||||
| // Used when a sub-task is launched and the parent task is waiting for it to | ||||
| // finish. | ||||
| // TBD: The 1s should be added to the settings, also should add a timeout to | ||||
|
|
||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several providers in the codebase have
clientproperties but don't overridedispose():AnthropicHandlerhasprivate client: AnthropicOpenAiHandlerhasprivate client: OpenAIBaseOpenAiCompatibleProviderhasprivate client: OpenAIThese will rely on this reflection-based disposal, which:
privateproperties (they're not accessible via reflection)As I said before, I think we should probably have each provider implement its own
dispose()method to properly clean up its specific resources, otherwise this implementation doesn't actually do much for providers that don't have any of the 3 disposal methods.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets go with a dispose method on the provider, like i mentioned on another pr this would streamline the codebase to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to get it done in the next couple of days