starred/tty7
1
0
Fork 0
mirror of https://github.com/l0ng-ai/tty7.git synced 2026-09-21 16:02:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(windows): keep a restored screen out of ConPTY's viewport, and stop Restart Server crashing the window (#657)

Browse Source 
* fix(restore): keep a restored screen out of ConPTY's viewport

On Windows a restored pane came back with its shell drawing in the wrong
place: the prompt stopped responding where it stood and the restored text
filled with fragments of whatever was being typed.

A ConPTY does not hand the terminal a stream, it hands it a rendering of a
screen buffer conhost owns, addressed absolutely and counted from that
buffer's top-left, which starts blank with the cursor at (0,0). PSReadLine
redraws the line being typed as `ESC[6;20H ... ESC[6;26H` on every
keystroke, and conhost frames what it paints the same way. Those row
numbers are only right if the client's viewport is conhost's buffer, row
for row.

Restored output is output conhost never produced and knows nothing about.
Left on screen it shifts every row conhost names, so the first repaint of
the input line lands on the old text. Nothing the client can do fixes it
afterwards: the offset is not constant, and it would have to be unpicked
from every absolute address in the stream.

So the restore preamble now ends by scrolling the restored screen out of
the way. `ESC[2J` on the primary screen scrolls the viewport into history
rather than erasing it, so the screen the daemon restored is one scroll up
rather than gone, and `ESC[H` leaves the cursor where a fresh ConPTY
expects to find it. Unix keeps the old behaviour: a shell there positions
itself relatively, so the restored screen can stay where it can be seen.

* fix(restart): stop Restart Server taking the window with it

Clicking Restart Server made the whole app disappear, with a double-lease
panic in the crash log: cannot read Tty7App while it is already being
updated.

The work that puts the window back together after the restart ran inside
`update_in` on this window's own entity, and it ends by rebuilding every
local window from the machine tree. The first thing that rebuild asks each
window is which tabs it is showing, which it reads back out of the window
registry — so 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.

Split into `settle_after_restart`: the window's own state first, then the
resync outside the lease, then the focus. The resync still runs either way
the restart went, because a refused handoff leaves the daemon serving the
panes this window already dropped (#554).

---------

Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com>
This commit is contained in:
l0ng-ai
2026-08-16 17:49:54 +08:00
committed by GitHub
co-authored by l0ng-ai
parent ac3c95a647
commit ccd21fe97d
3 changed files with 294 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/tty7-core/src/daemon/pane.rs
+63 -1
View File
@@ -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<u8> {
///
/// 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<u8> {
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<u8> {
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();
src/terminal/remote.rs
+105 -1
View File
@@ -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::<String>()
.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<String> = (-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))]
src/ui/app.rs
+126 -45
View File
@@ -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<Self>,
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<Self>) {
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::<Tty7App>()
.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;