Skip to content

fix: auto patch for file-watcher_93 #4231

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: auto patch for file-watcher_93 #4231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion src/services/code-index/manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,11 @@ export class CodeIndexManager {
*/
public dispose(): void {
if (this._orchestrator) {
this.stopWatcher()
this.stopWatcher() // This calls _orchestrator.stopWatcher()
// Ensure orchestrator is fully cleaned up if it has a dispose method
if (typeof (this._orchestrator as any).dispose === "function") {
;(this._orchestrator as any).dispose()
Comment on lines +219 to +222
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.

It looks like _orchestrator.stopWatcher() might be called twice here: once via this.stopWatcher() on line 219, and then again when this._orchestrator.dispose() is called on line 222 (since CodeIndexOrchestrator.dispose() also calls stopWatcher()).

Is this intentional?

I think to make this simpler we could have CodeIndexManager.dispose() rely primarily on this._orchestrator.dispose() to handle its own cleanup:

public dispose(): void {
  if (this._orchestrator) {
    // The orchestrator's dispose should handle its own stopWatcher call
    if (typeof this._orchestrator.dispose === 'function') {
      this._orchestrator.dispose();
    } else {
      // Fallback if for some reason dispose isn't on the orchestrator
      this.stopWatcher(); 
    }
  }
  this._stateManager.dispose();
}

What are your thoughts on this approach?

Copy link
Contributor Author

@kiwina kiwina Jun 2, 2025

Choose a reason for hiding this comment

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

I belive everything should be unified with dispose method, just like some of the other PR.
If that is the case than stopWatcher is actually not even necessay. In the long run that would make contribution/testing across the board more unified

Copy link
Contributor Author

@kiwina kiwina Jun 2, 2025

Choose a reason for hiding this comment

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

Also
public stopWatcher(): void { if (!this.isFeatureEnabled) { return } if (this._orchestrator) { this._orchestrator.stopWatcher() } }
this._orchestrator needs to be disposed 100% stopWatcher() does not guarantee that

Comment on lines +221 to +222
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.

I see this._orchestrator is typed as CodeIndexOrchestrator | undefined (on line 24), and CodeIndexOrchestrator now has a dispose() method.

Given this, the as any cast on line 221 might not be strictly necessary. Was it included for a specific reason?

}
}
this._stateManager.dispose()
}
Expand Down
9 changes: 9 additions & 0 deletions src/services/code-index/orchestrator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,4 +219,13 @@ export class CodeIndexOrchestrator {
public get state(): IndexingState {
return this.stateManager.state
}

/**
* Disposes of resources held by the orchestrator.
*/
public dispose(): void {
this.stopWatcher()
// Dispose other resources if any (e.g., if scanner or vectorStore had disposables not managed elsewhere)
// For now, stopWatcher handles the primary disposable (fileWatcher).
}
}