Skip to content

Conversation

@kiwina
Copy link
Contributor

@kiwina kiwina commented Jun 2, 2025

PR for RooIgnoreController FileSystemWatcher Leak (RooIgnoreController_41)

This PR addresses the FileSystemWatcher leak associated with RooIgnoreController as identified in leak-report/guaranteed/RooIgnoreController_41.md.

Description

RooIgnoreController instances, created by Task objects, manage a FileSystemWatcher. If a Task is not properly disposed of (i.e., Task.abortTask() or a new Task.dispose() method isn't called reliably), the RooIgnoreController's dispose method isn't called, leading to its FileSystemWatcher leaking.

This patch focuses on ensuring the Task class has a robust dispose method that cleans up its RooIgnoreController, and that Task.abortTask() reliably calls this dispose method.

Related Bug Report

Closes: #4232

Changes

  • Added/Augmented dispose() method in src/core/task/Task.ts to explicitly call this.rooIgnoreController?.dispose().
  • Ensured Task.abortTask() calls the new/augmented this.dispose() method.
  • Verified that ClineProvider.removeClineFromStack() correctly calls task.abortTask() or task.dispose().
  • Add/update unit tests if applicable to verify the disposal chain for Task and RooIgnoreController.

How to Test

  1. Create scenarios where Task instances are created and then their parent ClineProvider is disposed of or individual tasks are removed from the stack.
  2. Verify through logging or debugging that RooIgnoreController.dispose() is called for each RooIgnoreController instance associated with a cleaned-up Task.
  3. Monitor for any "Can't perform a React state update on an unmounted component" warnings related to FileSystemWatcher callbacks if test components are involved.

Important

Fixes FileSystemWatcher leak in RooIgnoreController by ensuring Task.dispose() is called for cleanup in Task.ts.

  • Behavior:
    • Fixes FileSystemWatcher leak in RooIgnoreController by ensuring dispose() is called.
    • Task.dispose() method in Task.ts now cleans up RooIgnoreController and other resources.
    • Task.abortTask() in Task.ts calls dispose() to ensure cleanup.
  • Testing:
    • Verify RooIgnoreController.dispose() is called when Task is disposed or aborted.
    • Monitor for React state update warnings related to FileSystemWatcher during tests.

This description was created by Ellipsis for f826ade. You can customize this summary. It will automatically update as commits are pushed.

@kiwina kiwina requested review from cte and mrubens as code owners June 2, 2025 05:28
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. bug Something isn't working labels Jun 2, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Jun 2, 2025
@daniel-lxs
Copy link
Member

daniel-lxs commented Jun 2, 2025

LGTM, I tested normal tasks and subtask, everything seems to be working as expected and no shared resources are being disposed.

One test failed, however doesn't seem to be related to this PR, I reran it but it got cancelled.

Thank you @kiwina!

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jun 2, 2025
@daniel-lxs daniel-lxs changed the title fix: auto patch for RooIgnoreController_41 fix: Prevent FileSystemWatcher leak in RooIgnoreController Jun 3, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 4, 2025
@mrubens mrubens merged commit 6f21103 into RooCodeInc:main Jun 4, 2025
31 of 34 checks passed
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Jun 4, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Jun 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer PR - Needs Review size:M This PR changes 30-99 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

fix: Undisposed FileSystemWatcher in RooIgnoreController (RooIgnoreController_41)

3 participants