From 31c3c8601098f45ce47f8c5d2e325668c71df5a3 Mon Sep 17 00:00:00 2001 From: zhujiatao <97923091+EcutAtom336@users.noreply.github.com> Date: Fri, 18 Sep 2026 16:34:33 +0000 Subject: [PATCH] terminal: Release the parse buffer once a command finishes (#64405) # Objective Fix memory a terminal never gives back after its command finishes. Each `terminal::Terminal` constructed a `vte::ansi::Processor`, and `Processor::new` reserves `SYNC_BUFFER_SIZE` (2 MiB) up front for synchronized updates. `Terminal::release_pty_resources` tore the PTY down once the command exited but left that reservation in place, so it lived exactly as long as the terminal did. That matters because the terminal outlives the command: the agent panel keeps each tool call's terminal so it can keep rendering the finished command's output (`ToolCallContent::Terminal(Entity<..>)`, `acp_thread.rs:2014`), and the panel's own terminal map is only cleared through `TerminalEvent::CloseTerminal`, which a task terminal never emits because `SpawnInTerminal::default()` sets `hide = HideStrategy::Never`. Measured on a release build running 50 agent commands (`ls`): peak heap 205.34 MB, 200.52 MB still retained at exit, and the heap grew in a staircase that never came back down (75 MB to 192 MB between 288s and 300s, then flat). heaptrack attributed the largest share, 104.00 MiB, to exactly 52 of these reservations (109,051,904 bytes = 52 x 2,097,152), all allocated from `TerminalBuilder::new` (`crates/terminal/src/terminal.rs:1309`) and never freed. So this costs roughly 2 MB per command for as long as its terminal is kept around. ## Solution - `output_processor` is only used by `Terminal::write_output`, which parses bytes *injected* into the terminal (init commands, terminal providers, the REPL). PTY output goes through the `alacritty_terminal` event loop's own parser, not this one. - Make the field an `Option`, create it on the first injected byte instead of at construction, and drop it in `release_pty_resources`, right after the PTY is torn down. - It is recreated if something injects bytes again, so `write_output` semantics do not change for terminal providers or init-command injection. As a side effect, terminals that never receive injected bytes no longer reserve the parser at all, even while they are live. ## Testing Added `tests::test_release_pty_resources_drops_an_allocated_parse_buffer` in `crates/terminal/src/terminal.rs`. It runs `echo` to completion in a task terminal the way an agent tool call does, and asserts the parse buffer's lifecycle directly on the terminal: - a terminal that received no injected bytes holds no parse buffer; - `write_output` builds one; - `release_pty_resources` drops it, while the output the terminal already captured is still readable; - injecting again after the release builds a new one. Asserting on the terminal's own state instead of on process-wide heap keeps PTY reader threads out of the measurement: they reserve parse buffers of their own and are torn down asynchronously, so a heap-based assertion would depend on a timing the test cannot synchronize with. Removing `self.output_processor = None` from `release_pty_resources` fails the test with `releasing the PTY should drop the parse buffer`. - `cargo test -p terminal`: 107 tests, all green. - `./script/clippy -p terminal`: clean. Tested on Linux. The fix and the test are platform-independent Rust (neither is behind a `cfg`, and Windows takes the same `TerminalType::Pty` branch), so I expect the same behaviour on macOS and Windows, but I have not verified it. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable ## Suggested .rules additions Found while working on this, proposed rather than edited inline, per the rules-hygiene section. - `webrtc-sys`'s build script never reuses its prebuilt download: on every run it deletes the extracted package and re-downloads 131 MB from the GitHub release, and every distinct unit gets its own `OUT_DIR` and therefore its own copy (a debug build, a release build, and rust-analyzer's `cargo check --all-targets` are at least three). A build that appears to hang indefinitely at `webrtc-sys(build)` with no output is this. Avoid it by extracting the release asset once outside `target/` and setting `LK_CUSTOM_WEBRTC` to that directory through the user-level cargo config, so rust-analyzer's background checks inherit it too: ```toml # ~/.cargo/config.toml [env] LK_CUSTOM_WEBRTC = "/home//.local/share/libwebrtc/webrtc-0001d84-4/linux-x64-release" ``` `webrtc_sys_build::download_webrtc` returns early whenever that variable is set. If the path goes stale (the prebuilt is tagged, e.g. `webrtc-0001d84-4`), the build fails to link, so the variable has to be updated when the tag changes. --- Release Notes: - Fixed terminals retaining about 2 MB of memory per command after the command had finished --- crates/terminal/src/terminal.rs | 61 ++++++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 5 deletions(-) diff --git a/crates/terminal/src/terminal.rs b/crates/terminal/src/terminal.rs index 37c32e9f97..8a669dd6bc 100644 --- a/crates/terminal/src/terminal.rs +++ b/crates/terminal/src/terminal.rs @@ -1019,7 +1019,7 @@ impl TerminalBuilder { completion_tx: None, term, term_config: config, - output_processor: Processor::::new(), + output_processor: None, title_override: None, events: VecDeque::with_capacity(10), last_content: Content { @@ -1306,7 +1306,7 @@ impl TerminalBuilder { completion_tx, term, term_config: config, - output_processor: Processor::::new(), + output_processor: None, title_override: terminal_title_override, events: VecDeque::with_capacity(10), //Should never get this high. last_content: Default::default(), @@ -1506,7 +1506,7 @@ pub struct Terminal { completion_tx: Option>>, term: Arc, term_config: AlacrittyTermConfig, - output_processor: Processor, + output_processor: Option>, events: VecDeque, /// This is only used for mouse mode cell change detection last_mouse: Option<(Point, SelectionSide)>, @@ -1965,7 +1965,9 @@ impl Terminal { let converted = convert_lf_to_crlf(bytes, &mut previous_byte_was_cr); let mut term = self.term.lock(); - self.output_processor.advance(&mut *term, &converted); + self.output_processor + .get_or_insert_with(Processor::::new) + .advance(&mut *term, &converted); drop(term); self.detect_init_command_startup_marker(); cx.emit(Event::Wakeup); @@ -3051,7 +3053,9 @@ impl Terminal { ) } - /// Releases live PTY resources while retaining process metadata and buffered output. + /// Releases live PTY resources, including the parse buffer used by + /// [`Terminal::write_output`], while retaining process metadata and buffered + /// output. /// /// Calling this method after the resources have already been released is a no-op. pub fn release_pty_resources(&mut self) { @@ -3064,6 +3068,10 @@ impl Terminal { }; let info = info.clone(); + // The terminal is retained past the command, so drop the parse buffer's + // reservation; `write_output` builds a new one if it is called again. + self.output_processor = None; + pty_tx.shutdown(); info.terminate_child_process(); @@ -5678,6 +5686,49 @@ mod tests { ); } + /// The parse buffer `write_output` needs is only built for injected bytes and + /// must not outlive the command, even though the terminal does. + #[gpui::test] + async fn test_release_pty_resources_drops_an_allocated_parse_buffer(cx: &mut TestAppContext) { + cx.executor().allow_parking(); + + let terminal = build_test_terminal(cx, "echo", &["captured_output"]).await; + + assert!( + terminal.read_with(cx, |terminal, _| terminal.output_processor.is_none()), + "a terminal that received no injected output should not hold a parse buffer" + ); + + let exit_status = + terminal.read_with(cx, |terminal, cx| terminal.wait_for_completed_task(cx)); + assert_eq!(exit_status.await, Some(ExitStatus::default())); + assert_content_eventually(&terminal, "captured_output", cx).await; + + terminal.update(cx, |terminal, cx| { + terminal.write_output(b"injected_before_release\n", cx); + }); + assert!( + terminal.read_with(cx, |terminal, _| terminal.output_processor.is_some()), + "injecting output should build the parse buffer" + ); + + terminal.update(cx, |terminal, _| terminal.release_pty_resources()); + assert!( + terminal.read_with(cx, |terminal, _| terminal.output_processor.is_none()), + "releasing the PTY should drop the parse buffer" + ); + + assert_content_eventually(&terminal, "captured_output", cx).await; + terminal.update(cx, |terminal, cx| { + terminal.write_output(b"injected_after_release\n", cx); + }); + assert!( + terminal.read_with(cx, |terminal, _| terminal.output_processor.is_some()), + "injecting output after a release should build a new parse buffer" + ); + assert_content_eventually(&terminal, "injected_after_release", cx).await; + } + mod perf { use super::{super::*, init_terminal_test_with_window}; use gpui::{ScrollDelta, ScrollWheelEvent, TestAppContext, VisualContext, point};