-
Notifications
You must be signed in to change notification settings - Fork 5.2k
fix: clear tool output and attachments when pruning to prevent memory leak #7049
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
base: dev
Are you sure you want to change the base?
Conversation
|
The following comment was made by an LLM, it may be inaccurate: No duplicate PRs found |
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.
Pull request overview
This PR fixes a memory leak where compacted tool outputs and their attachments were retained indefinitely in storage. When the pruning mechanism marks old tool outputs as compacted, the fix now clears both the output string and attachments array to free memory.
Key changes:
- Clears tool output and attachments during compaction to prevent unbounded memory growth
- Adds comprehensive test coverage for the pruning behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/opencode/src/session/compaction.ts | Sets output to empty string and attachments to undefined when marking tool parts as compacted, with explanatory comment |
| packages/opencode/test/session/compaction.test.ts | Adds two new test cases: one verifying output/attachments are cleared during pruning, and one testing that pruning respects the disabled configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed review comment about test not verifying behavior when pruning is disabled (commit 4881734). The test now:
|
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
this would be really helpful |
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.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
… leak When compaction prunes old tool outputs, only the compacted timestamp was being set - the actual output string and attachments array remained in storage indefinitely. This caused storage files to grow unbounded with large tool outputs (file contents, base64 images/PDFs, etc.). Now prune() clears both output and attachments when marking parts as compacted. The toModelMessage function already replaces compacted outputs with placeholder text, so this is safe. Fixes part of anomalyco#4315
…s to pruning disabled test
This reverts commit b302472.
9d089b6 to
141c413
Compare
Fixes #3013
Summary
Clear tool output and attachments when compaction prunes old tool results, actually freeing memory instead of just flagging.
Evidence
In a real session working on this repo:
Sample tool output sizes from this session:
webfetchof docs page: 10 KBreadof source file: 5-50 KBgit diff/gh pr diff: 10-100 KBbashcommand outputs: 1-50 KBAll of these outputs stay in memory even after compaction marks them "old".
The Problem
In
SessionCompaction.prune(), when tool outputs are pruned:The
compactedtimestamp is used bytoModelMessage()to replace output with placeholder text like "(Old tool result content cleared)" - but the actual data never gets freed.Over a long session:
The Fix
Now when compaction runs, the memory is actually freed. The placeholder text is already shown to the LLM (that logic exists), we just were not clearing the source data.
Testing
Existing compaction tests pass. Added test verifying output/attachments are cleared after prune.