fix(lsp): size-rotate per-server LSP logs (#10877) by david-engelmann · Pull Request #10975 · warpdotdev/warp

david-engelmann · 2026-05-15T01:31:46Z

Wires the LSP namespace into the size-based rotation primitives shipped for MCP in #10874 (#7723). Same cap as MCP: 10 MiB per file × 5 rotated copies = ~60 MiB per LSP server per workspace.

Fixes #10877. Depends on #10874 landing first — LogManager::register_with_rotation is introduced there.

What changes

crates/lsp/src/model.rs:272 — replaces the unbounded manager.register("lsp", ...) with manager.register_with_rotation("lsp", ..., logs::lsp_log_rotation_config()). Identical pattern to the MCP wire-up at app/src/ai/mcp/templatable_manager/native.rs:810.
crates/lsp/src/logs.rs (new) — lsp_log_rotation_config() mirroring app/src/ai/mcp/logs.rs::mcp_log_rotation_config().
crates/lsp/src/lib.rs — registers the new mod logs.

Net diff: 49 lines added, 1 removed.

Why mirror MCP's policy

simple_logger's rotation primitives are namespace-agnostic — the cap constants are the only knobs. Choosing the same 10 MiB × 5 cap as MCP keeps both namespaces well below the multi-GB unbounded growth observed for verbose servers (rust-analyzer in #10877, MCP servers in #7723) while preserving a useful multi-day debugging window. If a future change wants per-namespace caps the constants are already isolated in crates/lsp/src/logs.rs.

Tests

Two unit tests pin the cap constants and document the intentional sync with MCP:

$ cargo test -p lsp --lib logs::
test logs::tests::lsp_log_rotation_config_uses_expected_caps ... ok
test logs::tests::lsp_log_rotation_caps_match_mcp_namespace ... ok

Full lsp suite still green (18/18), rustfmt + clippy -D warnings clean.

Why no integration / live-rotation evidence

The rotation logic itself lives in simple_logger and is fully covered by the existing manager_tests.rs cases shipped in #10874. This PR is strictly the wire-up: changing one register call site and adding the config helper. The unit tests pin that the helper produces the policy the wire-up depends on; runtime correctness of rotation is already proven for the MCP namespace and is identical here.

A demonstration of the on-disk rotation shape (active log + .log.1...log.N + .rotations.jsonl sidecar) is available in the warp-taper sample tape — the LSP path produces the same artifacts.

…dev#7723) Warp captures every MCP server's stderr/stdout to a per-server log file under the `mcp/` namespace. Once a logger was registered, the file was opened with `truncate(true)` and then grew without bound for the lifetime of the app session — a verbose server like iMCP could reach 20 GB in five days (warpdotdev#7723). Adds size-based rotation to the `simple_logger` crate that backs MCP log capture: - `RotationConfig { max_file_size_bytes, max_rotation }` (in crates/simple_logger/src/lib.rs). `RotationConfig::new` returns `Option<Self>`, mapping any zero to `None` so callers reading from config can statically express "rotation disabled". - `SimpleLogger::new` now takes `rotation: Option<RotationConfig>`. When `None`, behavior is unchanged (one file per logger lifetime, truncate-on-create). When `Some`, the writer tracks bytes written and, after any write that brings the counter to or above the threshold, drops the file handle, shifts existing `.N` rotations up by one (discarding `.{max_rotation}` first), renames the active file to `.1`, and reopens a fresh active file. Log lines are never split across files; the active file may briefly overshoot the threshold by one line. - `LogManager::register_with_rotation` is a sibling of `register` that threads the config through. The existing `register` becomes a thin wrapper that passes `None`, so all current callers other than MCP are unaffected. - The MCP call site in `templatable_manager/native.rs` now uses `register_with_rotation` with a policy of 10 MiB × 5 rotations = 60 MiB cap per server (1 active + 5 rotated). Generous enough to keep a useful debugging window; small enough that even a chatty server can't fill a multi-day session. Rotation operates on `path.1` … `path.{max_rotation}` suffixes rather than rewriting extensions, so compound names like `server.stderr.log` get rotated to `server.stderr.log.1` rather than `server.stderr.1` (which is what `Path::set_extension` would produce). Rename failures for intermediate `.N` files are tolerated — the file may not exist yet if fewer than `max_rotation` rotations have happened. Only the rename of the active file surfaces an error, and even then the writer reopens the active path truncated and keeps going, so a transient rotation failure (full disk, permission flip) degrades to "oldest rotation discarded, current session continues" rather than "logging stops". Tests ===== $ cargo nextest run -p simple_logger 19 tests run: 19 passed $ cargo nextest run -p warp -E 'test(mcp::) or test(file_based_manager) or test(file_mcp_watcher) or test(simple_logger)' 74 tests run: 74 passed 15 new file-level unit tests in `crates/simple_logger/src/lib_tests.rs` cover `RotationConfig::new`, `path_with_suffix`, and `perform_rotation` across the boundary cases (empty file, missing intermediates, cap, max rotation = 1, three consecutive rotations to verify shift order). 2 new end-to-end tests in `crates/simple_logger/src/manager_tests.rs` drive a live `SimpleLogger` on a real background executor: one asserts that high-volume writes with rotation enabled produce a `.1` rotated file; the other pins the backward-compatibility guarantee that the same write volume *without* rotation produces no `.1` file. $ cargo clippy -p simple_logger --tests -- -D warnings # clean $ cargo clippy -p warp --tests -- -D warnings # clean $ cargo fmt -p simple_logger -p warp -- --check # clean Fixes warpdotdev#7723 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Wires the LSP namespace into the size-based rotation primitives shipped for MCP in warpdotdev#10874 (warpdotdev#7723). Cap is identical: 10 MiB per file × 5 rotated copies = 60 MiB per LSP server per workspace. Replaces the unbounded LogManager::register call at crates/lsp/src/model.rs:272 with register_with_rotation, mirroring the MCP wire-up in app/src/ai/mcp/templatable_manager/native.rs:810. A new crates/lsp/src/logs.rs hosts lsp_log_rotation_config(), parallel to app/src/ai/mcp/logs.rs::mcp_log_rotation_config(). Two unit tests pin the cap constants and document that they're intentionally kept in sync with the MCP policy. Depends on warpdotdev#10874 landing first — register_with_rotation is introduced there. Fixes warpdotdev#10877.

oz-for-oss · 2026-05-15T01:32:03Z

@david-engelmann

I'm starting a first review of this pull request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

oz-for-oss

Overview

This PR adds size-based rotation support to simple_logger, wires MCP and LSP server logs to use the new rotation policy, and adds unit/integration coverage for the rotation helpers.

Concerns

The rotation failure path can still truncate the active log after a failed rename, which loses the log data the rotation was supposed to preserve.

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

oz-for-oss · 2026-05-15T01:38:21Z

+                                        &log_path,
+                                    );
+                                }
+                                log_file = match open_truncated(&log_path).await {


⚠️ [IMPORTANT] If perform_rotation fails after the active handle is dropped, this reopens log_path with truncation and erases the current log even though it was never preserved. Reopen without truncation or keep appending/retry rotation when the rename fails.

`perform_rotation`'s only Err path is step 3 — the rename of the active file into its `.1` slot. When that rename fails, the original log content is still on disk at `log_path`. The callback in `SimpleLogger` was nevertheless calling `open_truncated` unconditionally, which destroyed the data the rotation was meant to preserve. Fix: on rotation failure, open the file in append mode and seed `written_bytes` from its current size so the next threshold check is accurate. On success, behavior is unchanged (truncate + reset counter). A new manager test pre-stages `server.log.1` as a non-empty directory with `max_rotation = 1` (so step 2's shift loop is a no-op and doesn't quietly move the blocker), drives enough log volume to trigger rotation, and asserts both that the active log keeps the pre-rotation content and that the blocking directory is untouched. Addresses Oz's "rotation failure path can still truncate the active log after a failed rename" concern on warpdotdev#10975.

…otation

david-engelmann · 2026-05-15T13:20:56Z

Addressed in the latest push, which lands on the underlying simple_logger branch (#10874) and is now merged into this stacked PR.

The fix is in crates/simple_logger/src/lib.rs: on rotation failure, the callback now opens the active file in append mode (rather than open_truncated) and seeds written_bytes from the file's current size so the next threshold check stays accurate. This preserves the log data on the only path through perform_rotation that can return Err — step 3's rename of the active file to its .1 slot. On success the behavior is unchanged: truncate + reset.

New test manager::tests::simple_logger_rotation_failure_preserves_active_log_content pre-stages server.log.1 as a non-empty directory with max_rotation = 1 (so step 2's shift loop is a no-op and can't quietly relocate the blocker), drives enough volume to trigger rotation, and asserts both that the active log retains its pre-rotation content and that the blocking directory is untouched.

cargo test -p simple_logger: 20/20 pass. cargo test -p lsp: 18/18 pass.

/oz-review

oz-for-oss · 2026-05-15T13:21:15Z

@david-engelmann

I'm re-reviewing this pull request in response to a review request.

You can view the conversation on Warp.

I reviewed this pull request and requested human review from: @kevinyang372.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Oz no longer requests changes for this pull request after the latest automated review.

oz-for-oss

Overview

This PR adds size-based rotation support to simple_logger, wires the rotation config into MCP and LSP per-server logs, and adds coverage for rotation behavior and namespace-specific caps.

Concerns

No blocking correctness, security, or error-handling concerns found in the annotated diff.

Verdict

Found: 0 critical, 0 important, 0 suggestions

Approve

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

david-engelmann · 2026-05-19T01:18:48Z

/oz-review

oz-for-oss · 2026-05-19T01:19:05Z

@david-engelmann

I'm re-reviewing this pull request in response to a review request.

You can view the conversation on Warp.

I completed the review and no human review was requested for this pull request.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

oz-for-oss

Overview

Reviewed the size-based log rotation primitives and the MCP/LSP wire-ups. The rotation implementation and tests look scoped to the log-growth issue, and no supplemental security findings were identified.

Concerns

The new LSP logs module is compiled unconditionally even though it imports simple_logger, which is only a non-wasm dependency of crates/lsp; this will break wasm builds unless the module is gated like the call site.

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

oz-for-oss · 2026-05-19T01:21:26Z

 mod language_server_candidate;
 pub use language_server_candidate::LanguageServerCandidate;
 pub mod install;
+mod logs;


⚠️ [IMPORTANT] This module is compiled on wasm too, but it imports simple_logger, which crates/lsp only depends on under cfg(not(target_arch = "wasm32")); gate it like the call site or wasm builds will fail.

Suggested change

mod logs;

#[cfg(not(target_arch = "wasm32"))]

mod logs;

…dotdev#10877) Address Oz reviewer feedback on warpdotdev#10975: the new `logs` module imports `simple_logger`, which is only a dependency of `crates/lsp` for non-wasm targets. The module was compiled unconditionally, breaking `cargo check -p lsp --target wasm32-unknown-unknown`. Gate the module declaration with `#[cfg(not(target_arch = "wasm32"))]` matching the call site at model.rs:269 and the existing pattern on `mod transport;`. Verified locally: cargo check -p lsp # clean cargo check -p lsp --target wasm32-unknown-unknown # clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

david-engelmann · 2026-05-19T02:44:10Z

Addressed the wasm-gate concern in d96b6b84. The mod logs; declaration is now #[cfg(not(target_arch = "wasm32"))] to match its only call site at crates/lsp/src/model.rs:269 and the existing pattern on mod transport;.

Verified locally:

cargo check -p lsp                                  # clean
cargo check -p lsp --target wasm32-unknown-unknown  # clean

/oz-review

oz-for-oss · 2026-05-19T02:44:27Z

@david-engelmann

I'm re-reviewing this pull request in response to a review request.

You can view the conversation on Warp.

I reviewed this pull request and requested human review from: @peicodes.

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

Oz no longer requests changes for this pull request after the latest automated review.

oz-for-oss

Overview

This PR adds size-based rotation support to simple_logger, wires the rotation policy into MCP and LSP server logs, and adds focused unit coverage for the rotation helper behavior and configured caps.

Concerns

No blocking correctness, security, or spec-alignment concerns found in the annotated diff.

Verdict

Found: 0 critical, 0 important, 0 suggestions

Approve

Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).

Powered by Oz

kevinyang372

@david-engelmann Can you re-request review once you land #10874? Currently there are a lot of overlap between the two PRs

david-engelmann · 2026-05-21T23:51:17Z

@kevinyang372 for sure!

david-engelmann and others added 2 commits May 13, 2026 20:47

cla-bot Bot added the cla-signed label May 15, 2026

github-actions Bot added the external-contributor Indicates that a PR has been opened by someone outside the Warp team. label May 15, 2026

oz-for-oss Bot previously requested changes May 15, 2026

View reviewed changes

david-engelmann mentioned this pull request May 15, 2026

feat(logging): in-session size rotation for warp.log (#10879) #11000

Open

david-engelmann added 2 commits May 15, 2026 09:18

Merge branch 'david/7723-mcp-log-rotation' into david/10877-lsp-log-r…

8cded61

…otation

oz-for-oss Bot reviewed May 15, 2026

View reviewed changes

oz-for-oss Bot requested a review from kevinyang372 May 15, 2026 13:27

oz-for-oss Bot previously requested changes May 19, 2026

View reviewed changes

oz-for-oss Bot reviewed May 19, 2026

View reviewed changes

oz-for-oss Bot requested a review from peicodes May 19, 2026 02:48

kevinyang372 reviewed May 21, 2026

View reviewed changes

Conversation

david-engelmann commented May 15, 2026

What changes

Why mirror MCP's policy

Tests

Why no integration / live-rotation evidence

Uh oh!

oz-for-oss Bot commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oz-for-oss Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Uh oh!

oz-for-oss Bot May 15, 2026

Choose a reason for hiding this comment

Uh oh!

david-engelmann commented May 15, 2026

Uh oh!

oz-for-oss Bot commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oz-for-oss Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Uh oh!

david-engelmann commented May 19, 2026

Uh oh!

oz-for-oss Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oz-for-oss Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Uh oh!

oz-for-oss Bot May 19, 2026

Choose a reason for hiding this comment

Uh oh!

david-engelmann commented May 19, 2026

Uh oh!

oz-for-oss Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oz-for-oss Bot left a comment

Choose a reason for hiding this comment

Overview

Concerns

Verdict

Uh oh!

kevinyang372 left a comment

Choose a reason for hiding this comment

Uh oh!

david-engelmann commented May 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oz-for-oss Bot commented May 15, 2026 •

edited

Loading

oz-for-oss Bot commented May 15, 2026 •

edited

Loading

oz-for-oss Bot commented May 19, 2026 •

edited

Loading

oz-for-oss Bot commented May 19, 2026 •

edited

Loading