fix(mcp): add size-based log rotation to prevent unbounded MCP log growth#9308
fix(mcp): add size-based log rotation to prevent unbounded MCP log growth#9308OthmanAdi wants to merge 2 commits intowarpdotdev:masterfrom
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @OthmanAdi on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
I'm starting a first review of this pull request. You can follow along in the session on Warp. I requested changes on this pull request and posted feedback. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR adds size-based rotation to SimpleLogger so MCP log files are rotated after roughly 50 MB and older files are retained with .old.N suffixes.
Concerns
- A single oversized MCP log message can still exceed the intended per-file cap because rotation is skipped when the active file is empty; this leaves disk usage unbounded by message size.
- Untouched-code concern:
app/src/ai/mcp/templatable_manager/native.rsappends stderr into the sameStringwithread_line(&mut buf)without clearing it, so each stderr line can re-log all prior stderr and create the oversized messages that bypass this cap.
Security
- Untrusted MCP server output can still drive unbounded disk growth with a single large log message; chunk or truncate log messages before writing.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| log_line | ||
| ); | ||
| let line_bytes = line.len() as u64; | ||
| if bytes_written > 0 && bytes_written + line_bytes > MAX_LOG_FILE_SIZE { |
There was a problem hiding this comment.
bytes_written == 0), so untrusted server output can write an arbitrarily large active/rotated file; truncate or chunk messages before writing.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @OthmanAdi on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
Updated to address the security concern:
The stderr accumulation bug in |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
…owth MCP server log files in the SimpleLogger grew without any size limit or rotation. A single log file could reach 16+ GB, with total MCP log directories consuming 18+ GB of disk space on Windows. The main Warp application log already has rotation logic in warp_logging, but SimpleLogger (used for MCP server logs) had none. Add size-based rotation to SimpleLogger's async writer: when a log file exceeds 50 MB, it is renamed to .old.0 and older rotated files are shifted (.old.0 -> .old.1, etc.), up to a maximum of 5 rotated files. The oldest file beyond the limit is deleted. A new empty log file is created to continue logging. Rotation is synchronous (std::fs::rename/remove_file) since it happens once per 50 MB and must complete atomically before the new file is opened. The async file handle is dropped first to release the lock. The constants MAX_LOG_FILE_SIZE (50 MB) and MAX_ROTATED_FILES (5) keep total MCP log storage bounded at approximately 300 MB per server. Closes warpdotdev#8993
…ty file Oz review found that a single oversized MCP log message bypasses the per-file cap when bytes_written is 0 (fresh file after rotation or startup), because the guard checked bytes_written > 0 before triggering rotation. A malicious MCP server could emit a single multi-GB log line that gets written without any size check. Two fixes: 1. Remove the bytes_written > 0 guard. Rotation now triggers even when the active file is empty if the incoming line would exceed the cap. The empty file gets rotated immediately and a fresh one opened. 2. Truncate individual log lines at 1 MB (MAX_LOG_LINE_BYTES). Any line exceeding the limit is written with its first 1 MB of content followed by ... [truncated]. This bounds the maximum size of any single write regardless of what the MCP server sends.
OthmanAdi
force-pushed
the
othmanadi/fix-mcp-log-rotation
branch
from
f410f84 to
ae011cb
Compare
1 participant
Description
Fixes #8993
MCP server log files grow without any size limit or rotation. A single log file was reported at 16.4 GB, with total MCP log storage consuming 18.3 GB across 1,423 files on Windows. The only cleanup mechanism is a full directory purge on application startup, meaning logs accumulate unbounded during a single session.
Root cause:
SimpleLogger(incrates/simple_logger/) opens log files with truncate on creation and writes every line via an unbounded async channel to a background task. There is zero size checking, zero rotation, and zero max file size logic. The main Warp application log (incrates/warp_logging/) already has rotation (up to 5 files for GUI, 10 for CLI), but this was never implemented forSimpleLogger.Fix: Add size-based log rotation to
SimpleLogger's async writer task. When the current log file exceeds 50 MB, the writer:.old.4).old.3->.old.4,.old.2->.old.3, etc.).old.0This mirrors the rotation pattern in
crates/warp_logging/src/native.rs(lines 132-184).Bounds
MAX_LOG_FILE_SIZEMAX_ROTATED_FILESWhat this changes
Testing
cargo check -p simple_logger -p warppasses cleanwarp_logging/src/native.rs::rotate_files(lines 154-184), which is already in productionstd::fs::renameandstd::fs::remove_fileare synchronous, which is correct here: rotation is rare (once per 50 MB) and must complete atomically before the new file is openedmetadata()per line, avoiding unnecessary syscalls on every log writeServer API dependencies
Agent Mode
Changelog Entries for Stable
CHANGELOG-BUG-FIX: MCP server log files now rotate at 50 MB with a maximum of 5 rotated files per server, preventing unbounded disk usage on Windows and all platforms.