Skip to content

Conversation

@hannesrudolph
Copy link
Collaborator

@hannesrudolph hannesrudolph commented Sep 4, 2025

Summary

  • avoid retaining large command output by trimming buffers in BaseTerminalProcess
  • clear stored output when TerminalProcess and ExecaTerminalProcess finish running commands

Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

Thank you for working on this memory leak fix! I've reviewed the changes and found some critical issues that need to be addressed before this can be merged. The main concern is that the memory leak may not be fully resolved due to the implementation logic.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Sep 4, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Draft / In Progress] in Roo Code Roadmap Sep 4, 2025
@hannesrudolph hannesrudolph added PR - Draft / In Progress and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Sep 4, 2025
@hannesrudolph hannesrudolph marked this pull request as ready for review December 12, 2025 23:11
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. bug Something isn't working size:L This PR changes 100-499 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Dec 12, 2025
@RooCodeInc RooCodeInc deleted a comment from roomote bot Dec 13, 2025
@hannesrudolph hannesrudolph moved this from PR [Draft / In Progress] to PR [Needs Review] in Roo Code Roadmap Dec 13, 2025
- Fix trimRetrievedOutput() logic to only clear when all output has been retrieved
- Fix race condition by moving buffer cleanup after 'continue' event
- Add test coverage for trimRetrievedOutput() edge cases

Addresses review comments:
- Logic issue in BaseTerminalProcess.trimRetrievedOutput()
- Race condition in ExecaTerminalProcess and TerminalProcess
- Missing test coverage for buffer trimming
@hannesrudolph hannesrudolph force-pushed the codex/locate-memory-leak-in-command-line-artifacts branch from d1fb632 to cc4ccc5 Compare December 16, 2025 03:16
@daniel-lxs
Copy link
Member

Hey! I traced through the code and I think there's a timing issue here.

The problem is this line:

this.lastRetrievedIndex = this.fullOutput.length

This marks all output as "retrieved" immediately after the command completes, but getEnvironmentDetails() hasn't had a chance to actually call getUnretrievedOutput() yet.

What happens:

  1. Command completes → process gets added to completedProcesses queue ✓
  2. Buffer gets cleared (PR change)
  3. Later, getEnvironmentDetails() tries to get output from completed processes
  4. cleanCompletedProcessQueue() removes the process because hasUnretrievedOutput() now returns false
  5. The AI never sees the terminal output

This breaks the "Inactive Terminals with Completed Process Output" section in getEnvironmentDetails.ts.

The fix would be to not force lastRetrievedIndex = fullOutput.length - just call trimRetrievedOutput() by itself, which will only clear when output has actually been consumed via getUnretrievedOutput().

@hannesrudolph
Copy link
Collaborator Author

Hey! I traced through the code and I think there's a timing issue here.

The problem is this line:

this.lastRetrievedIndex = this.fullOutput.length

This marks all output as "retrieved" immediately after the command completes, but getEnvironmentDetails() hasn't had a chance to actually call getUnretrievedOutput() yet.

What happens:

  1. Command completes → process gets added to completedProcesses queue ✓
  2. Buffer gets cleared (PR change)
  3. Later, getEnvironmentDetails() tries to get output from completed processes
  4. cleanCompletedProcessQueue() removes the process because hasUnretrievedOutput() now returns false
  5. The AI never sees the terminal output

This breaks the "Inactive Terminals with Completed Process Output" section in getEnvironmentDetails.ts.

The fix would be to not force lastRetrievedIndex = fullOutput.length - just call trimRetrievedOutput() by itself, which will only clear when output has actually been consumed via getUnretrievedOutput().

@roomote address this please.

…rieved output

The previous implementation marked all output as 'retrieved' immediately
after command completion, before getEnvironmentDetails() had a chance to
call getUnretrievedOutput(). This caused hasUnretrievedOutput() to return
false, resulting in completed processes being removed from the queue before
the AI could see their output.

Now trimRetrievedOutput() is called without forcing lastRetrievedIndex,
so the buffer is only cleared after output has actually been consumed
via getUnretrievedOutput().

Addresses review feedback from daniel-lxs.
@hannesrudolph hannesrudolph moved this from PR [Needs Review] to PR [Needs Prelim Review] in Roo Code Roadmap Dec 19, 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 codex PR - Needs Preliminary Review size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Status: PR [Needs Prelim Review]

Development

Successfully merging this pull request may close these issues.

3 participants