Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,11 @@ export interface ApiHandler {
* @returns A promise resolving to the token count
*/
countTokens(content: Array<Anthropic.Messages.ContentBlockParam>): Promise<number>

/**
* Optional method to dispose of any resources held by the API handler.
*/
dispose?: () => void
}

export function buildApiHandler(configuration: ProviderSettings): ApiHandler {
Expand Down
23 changes: 23 additions & 0 deletions src/api/providers/base-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[],
Expand All @@ -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 {
Copy link
Member

@daniel-lxs daniel-lxs Jun 2, 2025

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 client properties but don't override dispose():

  • AnthropicHandler has private client: Anthropic
  • OpenAiHandler has private client: OpenAI
  • BaseOpenAiCompatibleProvider has private client: OpenAI
  • Other OpenAI-compatible providers (Groq, Chutes, etc.)

These will rely on this reflection-based disposal, which:

  1. Won't work for private properties (they're not accessible via reflection)
  2. Assumes the SDK has one of the three 3 methods

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

// 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
}
Copy link
Member

@daniel-lxs daniel-lxs Jun 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reflection-based approach using (this as any).client and typeof checks might be problematic for several reasons:

  1. Loss of type safety: TypeScript cannot verify that subclasses have a client property
  2. Runtime dependency: Relies on property names and method names that might change
  3. Brittle assumptions: Assumes all clients will have one of three disposal methods (close, destroy, or dispose)

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.

}
}
10 changes: 10 additions & 0 deletions src/api/providers/bedrock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -976,4 +976,14 @@ Suggestions:
return `Bedrock completion error: ${errorMessage}`
}
}

override dispose(): void {
// Clear custom cache specific to this handler
this.previousCachePointPlacements = {}
logger.debug("AwsBedrockHandler: Cleared previousCachePointPlacements.", { ctx: "bedrock" })

// Call base class dispose for any generic cleanup (like the SDK client)
// The base dispose method handles client disposal reflectively.
super.dispose()
}
}
2 changes: 1 addition & 1 deletion src/api/providers/vscode-lm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ export class VsCodeLmHandler extends BaseProvider implements SingleCompletionHan
* converts the messages to VS Code LM format, and streams the response chunks.
* Tool calls handling is currently a work in progress.
*/
dispose(): void {
override dispose(): void {
if (this.disposable) {
this.disposable.dispose()
}
Expand Down
20 changes: 20 additions & 0 deletions src/core/task/Task.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dispose method will directly conflict with your PR #4233.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
I could potentially put them all togheter, i belive keeping them seperated for now will give a better insight. merging them is trivial.

console.log(`[subtasks] disposing task ${this.taskId}.${this.instanceId}`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log(`[subtasks] disposing task ${this.taskId}.${this.instanceId}`)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
we should have generally some better logging to aid in development, if we don't know whats going on it's has to fix something.

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?.()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that these are being called more than once.

  1. abortTask(true) is called, which internally calls this.api?.dispose?.()
  2. Then this.api?.dispose?.() is called again immediately after

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
Expand Down