diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index 67a05d1b..ad6fc8ad 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -1255,7 +1255,11 @@ pub struct Restore { /// for and cannot see. Leaving the alternate screen also does the useful thing /// in the common case: the primary buffer still holds the pre-`vim` scrollback /// from earlier in the same snapshot. -fn restore_preamble(banner: Option<&str>) -> Vec { +/// +/// On Windows it ends by scrolling the restored screen out of the viewport +/// ([`SCROLL_RESTORED_AWAY`]), which is a correctness requirement rather than a +/// matter of taste — see that constant. +pub fn restore_preamble(banner: Option<&str>) -> Vec { let mut out = Vec::new(); out.extend_from_slice(b"\x1b[?1049l\x1b[?25h\x1b[?7h\x1b[0m"); if let Some(banner) = banner.map(str::trim).filter(|b| !b.is_empty()) { @@ -1265,9 +1269,40 @@ fn restore_preamble(banner: Option<&str>) -> Vec { out.extend_from_slice(banner.replace(['\r', '\n'], " ").as_bytes()); out.extend_from_slice(b" \xe2\x94\x80\xe2\x94\x80\x1b[0m\r\n"); } + if cfg!(windows) { + out.extend_from_slice(SCROLL_RESTORED_AWAY); + } out } +/// Push the restored screen into the client's scrollback and put the cursor +/// back at the top-left, so the incoming shell starts on a blank viewport. +/// +/// A pty on unix hands the terminal a stream; a ConPTY hands it a *rendering of +/// a screen buffer it owns*. That buffer starts blank with its cursor at the +/// top-left, and conhost addresses it absolutely: PSReadLine redrawing the line +/// being typed emits `ESC[6;20H`, meaning row 6 of conhost's buffer, and every +/// frame conhost paints is positioned the same way. Those row numbers are only +/// correct if the client's viewport is conhost's buffer, row for row. +/// +/// Restored output breaks exactly that. It is output conhost never produced and +/// knows nothing about, so leaving it on screen puts the shell's first prompt +/// some rows below where conhost believes it is, and the first keystroke +/// repaints the input line *over the restored text* — the prompt stops +/// responding and the old screen fills with fragments of what is being typed. +/// Nothing the client can do fixes that after the fact: the offset is not a +/// constant (the screen scrolls) and it would have to be unpicked from every +/// absolute address in the stream. +/// +/// So the restored screen goes where it can be kept without claiming a row: +/// `ESC[2J` on the primary screen scrolls the viewport into history rather than +/// erasing it, so it is a scroll away, and `ESC[H` leaves the cursor where a +/// fresh ConPTY expects to find it. +/// +/// Not done on unix, where the shell positions itself relatively and the +/// restored screen can simply stay where the user can see it. +pub const SCROLL_RESTORED_AWAY: &[u8] = b"\x1b[2J\x1b[H"; + impl DaemonPane { pub fn spawn( id: u64, @@ -3626,6 +3661,33 @@ mod tests { assert!(text.contains("this shell is new")); } + /// The ConPTY constraint, from the daemon's side. A pane whose shell runs + /// on a ConPTY must open with an empty viewport and the cursor at the + /// top-left, because that is the state conhost's own screen buffer starts + /// in and every row it names afterwards is counted from there. Restored + /// output left on screen shifts all of them, and the shell's first repaint + /// of the line being typed lands on the old text — see + /// [`SCROLL_RESTORED_AWAY`]. + #[test] + fn the_preamble_clears_the_way_for_conpty_and_only_for_conpty() { + let text = String::from_utf8(restore_preamble(Some("this shell is new"))).unwrap(); + if cfg!(windows) { + assert!( + text.ends_with("\x1b[2J\x1b[H"), + "the restored screen has to be scrolled into history and the cursor \ + homed *last*, after the banner: anything printed afterwards would \ + take back the row conhost counts from. The preamble ends {:?}", + &text[text.len().saturating_sub(16)..] + ); + } else { + assert!( + !text.contains("\x1b[2J"), + "on a real pty the shell positions itself relatively, so the screen \ + the user asked to have back stays where they can see it" + ); + } + } + #[test] fn a_banner_cannot_smuggle_extra_lines_into_the_pane() { let text = String::from_utf8(restore_preamble(Some("first\r\nsecond"))).unwrap(); diff --git a/src/terminal/remote.rs b/src/terminal/remote.rs index a58191c1..15f78582 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -2246,7 +2246,7 @@ fn win_size(size: TermSize, cell_w: u16, cell_h: u16) -> WinSize { } #[cfg(all(test, windows))] -mod windows_teardown_tests { +mod windows_tests { use super::*; fn tcp_pair() -> (std::net::TcpStream, std::net::TcpStream) { @@ -2346,6 +2346,110 @@ mod windows_teardown_tests { "the abandoned link must not tear the adopted pane down" ); } + + /// What the daemon replays into a pane restored from a stored screen, in + /// the frames and the order it sends them: the dead pane's screen, then the + /// preamble, then the new shell's own first output. + fn replay_restore_into(daemon: &mut std::net::TcpStream, old_screen: &[u8], shell: &[u8]) { + let size = WinSize { + cols: 40, + rows: 10, + cell_w: 8, + cell_h: 17, + }; + DaemonMsg::Size(size).encode(daemon).unwrap(); + DaemonMsg::Snapshot(old_screen.to_vec()) + .encode(daemon) + .unwrap(); + DaemonMsg::Size(size).encode(daemon).unwrap(); + DaemonMsg::Snapshot(crate::daemon::pane::restore_preamble(Some( + "the shell below is new", + ))) + .encode(daemon) + .unwrap(); + DaemonMsg::Output(shell.to_vec()).encode(daemon).unwrap(); + } + + fn row(term: &RemoteTerminal, line: i32) -> String { + use alacritty_terminal::grid::Dimensions as _; + use alacritty_terminal::index::{Column, Line}; + let term = term.term.lock(); + let grid = term.grid(); + (0..grid.columns()) + .map(|c| grid[Line(line)][Column(c)].c) + .collect::() + .trim_end() + .to_string() + } + + /// A restored screen must not be sitting in the viewport when the new + /// shell's ConPTY starts drawing on it. + /// + /// conhost addresses its own screen buffer absolutely — PSReadLine repaints + /// the line being typed with `ESC[6;20H` and conhost frames it the same way + /// — and that buffer starts blank with the cursor at the top-left. Restored + /// output is output conhost never produced: left on screen it shifts every + /// row conhost names, so the first keystroke repaints the input line on top + /// of the old text instead of at the prompt. The restored screen belongs in + /// scrollback, where it survives without claiming a row. + #[test] + fn a_restored_screen_leaves_the_new_shell_the_viewport_conpty_thinks_it_has() { + crate::core::config::pin_test_config_dir(); + let (client_side, mut daemon_side) = tcp_pair(); + let term = RemoteTerminal::from_stream(client_side, TermSize::new(40, 10)).unwrap(); + + // Five lines of the dead pane, then the shell painting its prompt the + // way conhost does: at row 1 of a buffer it believes is blank. + replay_restore_into( + &mut daemon_side, + b"line one\r\nline two\r\nline three\r\nline four\r\nline five\r\n", + b"\x1b[?25l\x1b[1;1HPS C:\\> \x1b[?25h", + ); + + let mut top = String::new(); + for _ in 0..400 { + top = row(&term, 0); + if top.starts_with("PS C:") { + break; + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + assert_eq!( + top, "PS C:\\>", + "the shell's prompt paints where conhost put it" + ); + + for line in 1..10 { + assert_eq!( + row(&term, line), + "", + "row {line} still holds restored output, so conhost and the client \ + disagree about which row is which: the next repaint of the input \ + line lands on the old screen instead of at the prompt" + ); + } + + // Kept, not erased: `ESC[2J` on the primary screen scrolls the viewport + // into history, so the screen the daemon restored is one scroll away. + let depth = { + use alacritty_terminal::grid::Dimensions as _; + term.term.lock().grid().history_size() as i32 + }; + let history: Vec = (-depth..0).map(|line| row(&term, line)).collect(); + for wanted in [ + "line one", + "line two", + "line three", + "line four", + "line five", + ] { + assert!( + history.iter().any(|row| row == wanted), + "{wanted:?} is not in the scrollback; the restored screen was erased \ + rather than scrolled away. History holds {history:?}" + ); + } + } } #[cfg(all(test, unix))] diff --git a/src/ui/app.rs b/src/ui/app.rs index abb601d9..f618b2d3 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -1778,55 +1778,73 @@ impl Tty7App { } }) .await; - let _ = this.update_in(cx, |this, window, cx| { - match &restarted { - Ok(()) => { - // The link we held pointed at the server we just killed; - // the reconnect finds a new process whose registry knows - // nothing about these panes. The helper drops the dead - // link first — a pull sent down it dies on a dead socket - // before the reader notices — and rebuilds every local - // window from the tree. - crate::ui::tree_sync::resync_after_local_daemon_change(cx); - } - Err(e) => { - // A refused handoff leaves the daemon exactly as it was, - // still serving the panes this window just dropped. Leaving - // it at the error here strands them: every restore path is - // tree-driven, and the next sync of this emptied window - // would diff into "close every tab" against the mirror — - // deleting the pane records under the still-running shells, - // or the whole workspace if the user simply closes the - // window first (#554). Pull the layout back instead; where - // the failure really did take the daemon away (an exec that - // never re-listened), the pull misses and the rehydration - // debt keeps the empty window from being pushed up. - // - // The invalidating helper, not the one the reconnect uses: - // nothing here handshaked a link. Half of `hand_off`'s - // failures happen *after* the exec — a daemon that never - // started listening again is gone, and the client we still - // hold points at its socket, which `is_connected` keeps - // calling good until its reader sees the EOF. - log::error!( - "restart background service failed, resyncing from the tree: {e}" - ); - let text = t_fmt( - L10nKey::AppRestartServerFailed, - &[("error", &e.to_string())], - ); - this.startup_error = Some(gpui::SharedString::from(text.clone())); - window.push_notification(text, cx); - crate::ui::tree_sync::resync_after_local_daemon_change(cx); - } - } - this.focus_active(window, cx); - cx.notify(); - }); + Self::settle_after_restart(this, restarted, cx).await; }) .detach(); } + /// Put the window back together once the restart has been attempted, either + /// way it went. + /// + /// Split into three steps because the middle one must not run with this + /// window's entity leased. `resync_after_local_daemon_change` takes the + /// whole `App` and rebuilds *every* local window from the tree, and the + /// first thing it asks each one is which tabs it is showing — which it gets + /// by reading that window's `Tty7App` back out of the registry. Called from + /// inside `update_in`, the window it reaches for first is the one already + /// leased to the closure, and gpui's answer to a double lease is a panic + /// that takes the process with it: clicking Restart Server made the whole + /// app vanish. + async fn settle_after_restart( + this: gpui::WeakEntity, + restarted: anyhow::Result<()>, + cx: &mut gpui::AsyncApp, + ) { + // A refused handoff leaves the daemon exactly as it was, still serving + // the panes this window just dropped. Leaving it at the error here + // strands them: every restore path is tree-driven, and the next sync of + // this emptied window would diff into "close every tab" against the + // mirror — deleting the pane records under the still-running shells, or + // the whole workspace if the user simply closes the window first (#554). + // So the resync below runs either way; this step only says so on screen. + if this + .update_in(cx, |this, window, cx| { + if let Err(e) = &restarted { + log::error!("restart background service failed, resyncing from the tree: {e}"); + let text = t_fmt( + L10nKey::AppRestartServerFailed, + &[("error", &e.to_string())], + ); + this.startup_error = Some(gpui::SharedString::from(text.clone())); + window.push_notification(text, cx); + } + }) + .is_err() + { + return; + } + + // The link we held pointed at the server we just killed; the reconnect + // finds a new process whose registry knows nothing about these panes. + // The helper drops the dead link first — a pull sent down it dies on a + // dead socket before the reader notices — and rebuilds every local + // window from the tree. Where the restart failed and the daemon is + // really gone, the pull misses and the rehydration debt keeps the empty + // window from being pushed back up. + // + // The invalidating helper, not the one the reconnect uses: nothing here + // handshaked a link. Half of `hand_off`'s failures happen *after* the + // exec — a daemon that never started listening again is gone, and the + // client we still hold points at its socket, which `is_connected` keeps + // calling good until its reader sees the EOF. + let _ = cx.update(crate::ui::tree_sync::resync_after_local_daemon_change); + + let _ = this.update_in(cx, |this, window, cx| { + this.focus_active(window, cx); + cx.notify(); + }); + } + fn set_font_size(&mut self, size: f32, cx: &mut Context) { let size = size.clamp(FONT_SIZE_MIN, FONT_SIZE_MAX); self.font_size = size; @@ -9253,6 +9271,69 @@ mod keybinding_gpui_tests { } } +#[cfg(test)] +mod restart_server_gpui_tests { + use crate::core::config::Config; + use crate::core::session::Session; + use crate::ui::app::Tty7App; + use gpui::{AppContext, TestAppContext}; + + /// Clicking Restart Server made the whole app disappear. + /// + /// The work that puts the window back together after the restart ends by + /// rebuilding every local window from the machine tree, and the first thing + /// that rebuild asks each window is which tabs it is showing — which it + /// reads back out of the window registry. Run from inside `update_in` on + /// this window's own entity, the first window it reaches for is the one the + /// closure already holds leased, and gpui answers a double lease by + /// panicking, which on the main thread is the process. + /// + /// Driven through `settle_after_restart` with a restart that "succeeded", + /// because the crash is in the part that runs either way, not in the + /// restart itself. + #[gpui::test] + async fn settling_after_a_restart_does_not_lease_the_window_twice(cx: &mut TestAppContext) { + crate::core::config::pin_test_config_dir(); + cx.executor().allow_parking(); + cx.update(|cx| { + gpui_component::init(cx); + cx.set_global(Config::default()); + crate::ui::keymap::init(cx); + crate::ui::windows::WindowRegistry::init(cx); + }); + let window = cx.add_window(|window, cx| { + let app = + cx.new(|cx| Tty7App::with_session(None, Some(Session::default()), window, cx)); + gpui_component::Root::new(app, window, cx) + }); + let app = window + .update(cx, |root, _, _| { + root.view() + .clone() + .downcast::() + .ok() + .expect("window root wraps a Tty7App") + }) + .unwrap(); + + // Registered the way an opened window registers itself: without this + // the rebuild finds no window to ask and never reaches for the entity, + // which is the whole thing under test. + let handle = window.into(); + let weak = app.downgrade(); + app.update(cx, |app, cx| { + crate::ui::windows::WindowRegistry::register(cx, app.workspace, handle, weak); + }); + + Tty7App::settle_after_restart(app.downgrade(), Ok(()), &mut cx.to_async()).await; + + assert!( + app.update(cx, |app, _| app.startup_error.is_none()), + "a restart reported as successful must not leave an error banner" + ); + } +} + #[cfg(test)] mod shell_menu_gpui_tests { use crate::core::config::Config;