fix: keep large MCP response files off tmpfs - #9308
Conversation
Signed-off-by: Bright Zheng <bzqzheng@gmail.com>
a3456eb to
aec12d6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aec12d6012
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let _guard = | ||
| env_lock::lock_env([("GOOSE_PATH_ROOT", Some(temp_root.path().to_str().unwrap()))]); |
There was a problem hiding this comment.
Serialize all tests that read GOOSE_PATH_ROOT
When Rust runs these module tests in parallel, this guard mutates the process-wide GOOSE_PATH_ROOT while sibling large-response tests in the same file also call process_tool_response without taking env_lock. Those calls can spill into this temporary root and then try to read the generated file after this test drops temp_root, making the test suite flaky; please either take the same env lock around every test that reaches Paths::in_data_dir here or avoid changing global env for this assertion.
Useful? React with 👍 / 👎.
|
Thanks for the PR — the Docker tmpfs issue in #9153 is real, but we're not sure moving spill files to The current code writes to Moving them to Could you think about an approach that keeps the temporary nature of these files? For example:
We'd want to see a cleanup story before moving these into a persistent directory. |
|
Going to close this since no answer to the concern. it's a genuine issue though, so if you come up with a better idea, let me know. |
Summary
When an MCP tool response exceeds the large-response threshold, goose writes the full text to a spill file and returns the file path to the model. That path previously lived under the process temp directory (
/tmp/goose_mcp_responseson Unix-like systems). In Docker,/tmpis often tmpfs-backed, so large responses can still consume container memory even after goose has moved them out of the model context.This keeps the existing large-response behavior, but moves the spill files to goose's data directory and keeps the path private:
Paths::in_data_dir("mcp_responses")instead of the OS temp directory0700permissions on Unixtempfile, which gives the file a randomized name and0600permissionsTesting
cargo fmt --package goosecargo test -p goose agents::large_response_handler::tests -- --nocapturecargo clippy -p goose --all-targets -- -D warningsgit diff --checkRelated Issues
Related to #9153. This addresses the tmpfs-backed
/tmpspill path described there by moving large MCP response files into goose's data directory. It does not add lifecycle cleanup for old spill files.Screenshots/Demos (for UX changes)
Before: N/A
After: N/A