From 8adebdf24deb0752a42df9d902956b01fe62a252 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:54:45 +0800 Subject: [PATCH 01/15] perf(ssh): prove the remote server once per connection, not once per pane (#695) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opening a second tab on a machine tty7 was already connected to and already serving cost the same wait as the first one. The SSH connection is reused, so none of that wait was handshake cost: every route called `ensure_remote_server` unconditionally, and that runs the whole installer probe again — `uname -sm`, an SFTP realpath for the home directory, an SFTP stat, a control probe that spawns the server binary, and `check_running_build`, which walks `/proc/[0-9]*` with a `readlink` per PID and shells out to `ps` on the machines that have no `/proc`. Five serial round trips before the pane's own channel opened, to re-learn what the pane before it had just learned. WSL fixed exactly this in #479 by remembering where a distro's server was last proved to be. SSH now does the same, with one difference that matters: a distro name is the whole identity of a WSL target, but an SSH connection can die and be replaced under the same key, so the note is kept on the `SshConnection` rather than in a map beside its key. Keying by connection generation is then not a discipline anyone has to keep — a reconnect is a new `SshConnection` with an empty slot, and nothing has to remember to forget. Memoizing must not quietly cancel the version check, which is the one thing that could make this a bad trade. Three things keep it honest. The note carries the build mismatch the probe found and re-files it on every hit, because the warning is raised inside `Installer::run` and each route drains its own sink — without that, only the first pane on a connection would ever hear that a different build is serving the machine, and every window after it would attach in silence. `replace_remote_server` and `restart_remote_daemon` forget before they act, not after, so a restart that fails halfway leaves the next pane looking rather than trusting a note written before the upheaval. And the router forgets when a routed link closes without the remote sending a byte, the way it already does for a WSL bridge: `exec` succeeds whatever the command turns out to be, so a binary deleted or moved since the probe is discovered exactly there. A failed probe is deliberately not remembered. A host that was briefly unreachable, or an install the user declined once, must not pin every later pane on that connection into the same failure — the slot is written only when the probe got all the way through. The note carries the binary path and the mismatch and nothing else: `installed`, `launched` and `confirmed` describe an event rather than a state, and serving them again to a later pane would only make the log lie. Left alone on purpose: the probe itself, which is unchanged and still the only thing that decides what a pane runs; the WSL memo, which keeps its own shape; and the macOS-server half of #695, which shipped in v26.9.1. Claude-Session: https://claude.ai/code/session_01UUyWQXzcBAoBzaSX8pc7nU --- crates/tty7-core/src/daemon/install/mod.rs | 154 ++++++++++++--- crates/tty7-core/src/daemon/install/tests.rs | 198 +++++++++++++++++++ crates/tty7-core/src/daemon/router.rs | 19 ++ crates/tty7-core/src/daemon/ssh/session.rs | 37 ++++ 4 files changed, 383 insertions(+), 25 deletions(-) diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index bc73c4d4..157970bd 100644 --- a/crates/tty7-core/src/daemon/install/mod.rs +++ b/crates/tty7-core/src/daemon/install/mod.rs @@ -1341,37 +1341,135 @@ fn connection_label(conn: &SshConnection) -> String { conn.key().as_str().to_string() } +/// What one SSH connection's server probe proved, kept so that the panes after +/// the first one do not pay for proving it again. +/// +/// `Installer::run` is four to six serial round trips — `uname -sm`, an SFTP +/// realpath for the home directory, an SFTP stat, a control probe that spawns +/// the server binary, and `check_running_build`, which walks `/proc//exe` +/// with a `readlink` per PID (or shells out to `ps` where there is no `/proc`). +/// That is a fair price once for a machine and an absurd one per pane: issue +/// #695 is a user watching `connecting to ...` for ten seconds every time they +/// open a tab on a host tty7 was already connected to and already serving. The +/// SSH connection itself is reused, so none of that wait is handshake cost. +/// +/// Deliberately not a global map keyed by host, the way [`wsl`]'s is. A distro +/// name is the whole identity of a WSL target, but an SSH connection can die +/// and be replaced under the same key, and a note about the previous link must +/// not answer for the next one. Keying by connection generation *is* keeping +/// the note on the connection — see `SshConnection::proved_server`. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ProvedServer { + /// The server binary the probe settled on, which is what the route runs. + pub binary: String, + /// The build mismatch the probe found, if it found one. + /// + /// Kept because the warning is raised inside `Installer::run`, and the + /// whole point of the memo is that `run` does not happen again: without + /// this, only the first pane on a connection would ever hear that a + /// different build is serving the machine, and every pane after it — every + /// window, since a connection outlives one — would attach in silence. That + /// is the one way memoizing could quietly cancel the version check, so it + /// is the one thing the note carries besides the path. + pub mismatch: Option, +} + +impl ProvedServer { + fn from_report(report: InstallReport) -> ProvedServer { + ProvedServer { + binary: report.paths.binary, + mismatch: report.mismatch, + } + } +} + +/// Answer from the note if there is one, otherwise prove it and leave a note. +/// +/// Two rules the callers depend on: +/// +/// - A failed probe is not remembered. `prove` returning an error leaves the +/// slot exactly as it found it, so a host that was briefly unreachable, or an +/// install the user declined once, is retried by the next pane rather than +/// pinned into permanent failure for the life of the connection. +/// - A remembered mismatch is re-filed on every hit. Each route carries its own +/// mismatch sink (`RouteSetup::mismatches`), drained into a prompt as the +/// route is set up, so re-filing is what makes the *n*th pane's client hear +/// what the first pane's probe found. +fn proved_or_prove( + slot: &mut Option, + prove: impl FnOnce() -> io::Result, +) -> io::Result { + if let Some(known) = slot.as_ref() { + if let Some(mismatch) = known.mismatch.clone() { + record_remote_mismatches(vec![mismatch]); + } + return Ok(known.binary.clone()); + } + let proved = prove()?; + let binary = proved.binary.clone(); + *slot = Some(proved); + Ok(binary) +} + pub fn ensure_remote_server(conn: &Arc) -> io::Result { let host = connection_label(conn); ensure_remote_server_labeled(conn, &host) } pub fn ensure_remote_server_labeled(conn: &Arc, host: &str) -> io::Result { - let ops = ssh_ops::SshRemoteOps::new(conn.clone()); - let fetch = default_fetcher(); - let confirm = install_confirm(); - let source = BundledOrRelease::discover(fetch.as_ref()); - let report = Installer::with_source(&ops, &source, confirm.as_ref(), host).run()?; - log::info!( - "remote {host}: {} at {} ({}{})", - if report.installed { - "installed tty7-server" - } else { - "tty7-server already present" - }, - report.paths.binary, - if report.launched { - "daemon launched" - } else { - "daemon already running" - }, - if report.mismatch.is_some() { - ", build mismatch recorded" - } else { - "" - }, - ); - Ok(report.paths.binary) + // The lock is held across the probe, not just across the read: two panes + // opening at once on a cold connection would otherwise both install, and + // the loser would be uploading over the very file the winner is renaming + // into place. Waiting out an install is what the second pane wants to do + // anyway — it needs the same answer. + let mut slot = conn.proved_server(); + if let Some(known) = slot.as_ref() { + log::debug!( + "remote {host}: tty7-server was already proved at {} on this connection", + known.binary, + ); + } + proved_or_prove(&mut slot, || { + let ops = ssh_ops::SshRemoteOps::new(conn.clone()); + let fetch = default_fetcher(); + let confirm = install_confirm(); + let source = BundledOrRelease::discover(fetch.as_ref()); + let report = Installer::with_source(&ops, &source, confirm.as_ref(), host).run()?; + log::info!( + "remote {host}: {} at {} ({}{})", + if report.installed { + "installed tty7-server" + } else { + "tty7-server already present" + }, + report.paths.binary, + if report.launched { + "daemon launched" + } else { + "daemon already running" + }, + if report.mismatch.is_some() { + ", build mismatch recorded" + } else { + "" + }, + ); + Ok(ProvedServer::from_report(report)) + }) +} + +/// Drop what we thought we knew about a connection's server, so that the next +/// `ensure_remote_server` on it proves the whole thing again. +/// +/// Three callers, and between them they are the memo's correctness argument: +/// [`restart_remote_daemon`] and [`replace_remote_server`], which change which +/// build is serving the machine and therefore what the note claims, and the +/// router, which calls this when a routed link closes without the remote ever +/// sending a byte — the only way a path that has stopped working is found out. +/// A reconnect needs no caller at all: the note lives on the connection, and a +/// new connection has none. +pub fn forget_remote_server(conn: &SshConnection) { + *conn.proved_server() = None; } pub fn restart_remote_daemon(conn: &Arc) -> io::Result<()> { @@ -1379,6 +1477,11 @@ pub fn restart_remote_daemon(conn: &Arc) -> io::Result<()> { let ops = ssh_ops::SshRemoteOps::new(conn.clone()); let fetch = default_fetcher(); let confirm = install_confirm(); + // Forget first, not afterwards: this deliberately changes what is running + // over there, which is most of what the note claims, and a restart that + // fails halfway must leave the next pane looking rather than trusting a + // note written before the upheaval. + forget_remote_server(conn); Installer::new(&ops, fetch.as_ref(), confirm.as_ref(), host).restart_daemon()?; Ok(()) } @@ -1389,6 +1492,7 @@ pub fn replace_remote_server(conn: &Arc) -> io::Result<()> { let fetch = default_fetcher(); let confirm = install_confirm(); let source = BundledOrRelease::discover(fetch.as_ref()); + forget_remote_server(conn); Installer::with_source(&ops, &source, confirm.as_ref(), host).replace()?; Ok(()) } diff --git a/crates/tty7-core/src/daemon/install/tests.rs b/crates/tty7-core/src/daemon/install/tests.rs index dbb59194..1422e5bc 100644 --- a/crates/tty7-core/src/daemon/install/tests.rs +++ b/crates/tty7-core/src/daemon/install/tests.rs @@ -59,6 +59,10 @@ struct FakeRemote { /// a login shell that could not read the script, which is the shape the /// no-`/proc` bug took on every Mac. stop_fails: bool, + /// SFTP metadata reads — the realpath behind `home_dir` and every `stat`. + /// The journal carries commands and writes; these are the other half of + /// what a probe spends on the wire, and #695 is a count of both. + sftp_reads: Mutex, } impl FakeRemote { @@ -87,6 +91,7 @@ impl FakeRemote { speaks: Mutex::new(HashMap::new()), installed_speaks: Some(ours()), stop_fails: false, + sftp_reads: Mutex::new(0), } } @@ -183,10 +188,28 @@ impl FakeRemote { .filter(|j| !matches!(j, Journal::Exec(_))) .collect() } + + /// The commands this remote was asked to run, in order. + fn execs(&self) -> Vec { + self.journal() + .into_iter() + .filter_map(|j| match j { + Journal::Exec(cmd) => Some(cmd), + _ => None, + }) + .collect() + } + + /// Everything that would have crossed the wire: commands, SFTP metadata + /// reads, and the writes an install makes. + fn round_trips(&self) -> usize { + self.journal().len() + *self.sftp_reads.lock().unwrap() + } } impl RemoteOps for FakeRemote { fn home_dir(&self) -> Result { + *self.sftp_reads.lock().unwrap() += 1; Ok(HOME.to_string()) } @@ -290,6 +313,7 @@ impl RemoteOps for FakeRemote { } fn stat(&self, path: &str) -> Result, String> { + *self.sftp_reads.lock().unwrap() += 1; Ok(self.file(path).map(|f| RemoteStat { size: f.bytes.len() as u64, mode: f.mode, @@ -2256,3 +2280,177 @@ fn replacing_overwrites_a_published_binary_that_does_not_serve_us() { ); assert!(!release.fetched().is_empty(), "which means downloading it"); } + +/// The probe every pane used to pay for, and the note that spares the second +/// one — issue #695. See [`ProvedServer`]. +mod proving_the_server_once_per_connection { + use super::*; + + /// A warm machine: this build's server is installed and already serving. + /// Every pane after the first on a connection to it finds exactly this. + fn warm() -> FakeRemote { + FakeRemote::new().with_previous_install().serving(BINARY) + } + + fn prove(remote: &FakeRemote, user: &FakeUser, host: &str) -> io::Result { + let release = FakeRelease::new(); + Ok(ProvedServer::from_report( + installer(remote, &release, user, host).run()?, + )) + } + + /// The measurement the issue asks for, from the fake's own books: what the + /// first pane on a connection spends, and what the second one spends after + /// it. The chain is asserted by name rather than by count so that a probe + /// growing a step is a failure here and not a slow tab somewhere. + #[test] + fn the_second_pane_on_a_connection_spends_nothing() { + let remote = warm(); + let user = FakeUser::approving(); + let mut slot = None; + + let first = proved_or_prove(&mut slot, || prove(&remote, &user, "me@warm-box:22")) + .expect("the server is there and serving"); + assert_eq!(first, BINARY); + assert_eq!( + remote.execs(), + vec![ + "uname -sm".to_string(), + format!("{} --stdio --bridge < /dev/null", shell_quote(BINARY)), + RUNNING_EXE_COMMAND.to_string(), + ], + "the probe: what to install, is a daemon answering, and what build is serving" + ); + assert_eq!( + remote.round_trips(), + 5, + "three commands and two SFTP reads — the realpath for $HOME and the stat" + ); + + let paid = remote.round_trips(); + let second = proved_or_prove(&mut slot, || { + panic!("the second pane must not probe again"); + }) + .expect("the note answers"); + assert_eq!(second, BINARY); + assert_eq!( + remote.round_trips(), + paid, + "the second pane pays nothing for what the first one proved" + ); + } + + /// A transient failure must not pin every later pane on the connection into + /// the same failure. Nothing is written to the note unless the probe got + /// all the way through, so the next pane goes and asks again. + #[test] + fn a_probe_that_failed_is_not_remembered() { + let remote = FakeRemote::new(); + let user = FakeUser::declining(); + let mut slot = None; + + let refused = proved_or_prove(&mut slot, || prove(&remote, &user, "me@shy-box:22")) + .expect_err("the user said no"); + assert!( + format!("{refused}").contains("was not confirmed"), + "the refusal is the install prompt's, not something else: {refused}" + ); + assert_eq!(slot, None, "a failure leaves the slot exactly as it was"); + assert_eq!(user.asked().len(), 1); + + let _ = proved_or_prove(&mut slot, || prove(&remote, &user, "me@shy-box:22")); + assert_eq!( + user.asked().len(), + 2, + "the pane after a refusal asks again rather than inheriting the refusal" + ); + } + + /// The one thing memoizing could quietly cancel: the version check. The + /// probe is what notices that a different build is serving the machine, and + /// the note has to keep filing that warning for the panes that never run + /// the probe — each route drains its own sink, so a warning filed only once + /// would reach only the first pane's client. + #[test] + fn a_remembered_mismatch_is_filed_again_for_every_pane() { + let (remote, legacy) = FakeRemote::new().with_legacy_install("26.7.4"); + let remote = remote.serving(&legacy).speaking( + &legacy, + RemoteProtocol { + control: CONTROL - 1, + protocol: PROTOCOL, + build: "26.7.4".to_string(), + }, + ); + let user = FakeUser::approving(); + let mut slot = None; + + let first_route: Arc>> = Arc::new(Mutex::new(Vec::new())); + with_mismatch_sink(first_route.clone(), || { + proved_or_prove(&mut slot, || prove(&remote, &user, "me@old-box:22")) + .expect("an old daemon is kept, not a failure") + }); + assert_eq!( + first_route.lock().unwrap().len(), + 1, + "the probe found the mismatch" + ); + + let spent = remote.round_trips(); + let second_route: Arc>> = + Arc::new(Mutex::new(Vec::new())); + with_mismatch_sink(second_route.clone(), || { + proved_or_prove(&mut slot, || panic!("the note answers this one")).expect("remembered") + }); + + let filed = second_route.lock().unwrap().clone(); + assert_eq!(filed.len(), 1, "the second pane's client hears it too"); + assert_eq!(filed[0].running_version.as_deref(), Some("26.7.4")); + assert_eq!(filed[0].wanted_version, VERSION); + assert_eq!( + remote.round_trips(), + spent, + "and hears it without a round trip" + ); + } + + /// The wiring, over a real SSH connection: `ensure_remote_server` reads the + /// note off the connection it was handed, and `forget_remote_server` takes + /// it away again. The fake sshd counts session channels, so "no round trip" + /// is measured here rather than argued. + #[tokio::test] + async fn a_proved_connection_answers_the_next_pane_off_the_wire() { + use crate::daemon::ssh::test_support::{Exec, FakeSshd}; + + let sshd = FakeSshd::connect(Exec::Exits, None).await; + assert_eq!( + sshd.conn.remembered_server(), + None, + "a new link knows nothing" + ); + + *sshd.conn.proved_server() = Some(ProvedServer { + binary: BINARY.to_string(), + mismatch: None, + }); + assert_eq!( + ensure_remote_server(&sshd.conn).expect("the note answers"), + BINARY + ); + assert_eq!( + sshd.opened(), + 0, + "a proved connection opens no channel for the next pane" + ); + assert_eq!(sshd.conn.remembered_server().as_deref(), Some(BINARY)); + + // What `replace_remote_server`, `restart_remote_daemon` and a routed + // link that closed without answering all do before they act. + forget_remote_server(&sshd.conn); + assert_eq!( + sshd.conn.remembered_server(), + None, + "the next pane proves it again the long way" + ); + } +} diff --git a/crates/tty7-core/src/daemon/router.rs b/crates/tty7-core/src/daemon/router.rs index 4c759568..9585cc85 100644 --- a/crates/tty7-core/src/daemon/router.rs +++ b/crates/tty7-core/src/daemon/router.rs @@ -622,6 +622,25 @@ async fn drive(local: Stream, header: &RouteHeader) -> io::Result<()> { log::info!("wsl:{distro}: the bridge closed without answering; proving it again next time"); crate::daemon::install::wsl::forget_wsl_server(distro); } + // The same reasoning over SSH, where the note is the one this connection's + // probe left behind. `exec` on a session channel succeeds whatever the + // command turns out to be, so a server binary that has been deleted or + // moved since the probe proved it is discovered exactly here, by a link + // that opened and then said nothing. Forget it and the next pane on this + // connection pays for a fresh probe once; leave it and every pane on the + // connection repeats the same silent failure. + if let (RouteTarget::Ssh(_), Some(conn)) = (&header.target, conn.as_ref()) + && header.server_command.is_none() + && !copied + .as_ref() + .is_ok_and(|(_, from_remote)| *from_remote > 0) + { + log::info!( + "ssh {}: the routed link closed without answering; proving the server again next time", + conn.key().as_str(), + ); + crate::daemon::install::forget_remote_server(conn); + } let (to_remote, to_local) = copied?; log::debug!("routed connection closed after {to_remote} up / {to_local} down bytes"); diff --git a/crates/tty7-core/src/daemon/ssh/session.rs b/crates/tty7-core/src/daemon/ssh/session.rs index dd25437d..fe38f8fe 100644 --- a/crates/tty7-core/src/daemon/ssh/session.rs +++ b/crates/tty7-core/src/daemon/ssh/session.rs @@ -5,6 +5,7 @@ use std::sync::{Arc, Mutex, Weak}; use russh::client::Msg; use russh::{Channel, ChannelMsg}; +use crate::daemon::install::ProvedServer; use crate::daemon::protocol::WinSize; use crate::daemon::remote_link::RemoteEntry; @@ -230,6 +231,9 @@ pub struct SshConnection { remote_forwards: RemoteForwardTable, alive: AtomicBool, remote_entry: tokio::sync::Mutex>, + /// What this connection's server probe proved, once it has. See + /// [`SshConnection::proved_server`]. + proved_server: Mutex>, } impl SshConnection { @@ -244,6 +248,7 @@ impl SshConnection { remote_forwards, alive: AtomicBool::new(true), remote_entry: tokio::sync::Mutex::new(None), + proved_server: Mutex::new(None), }) } @@ -334,6 +339,38 @@ impl SshConnection { *self.remote_entry.lock().await = Some(entry); } + /// Where this connection's server was proved to be, and the lock that + /// makes the second pane wait for the first rather than prove it again. + /// + /// The memo lives on the connection rather than beside its key, and that + /// is the whole of the invalidation story for a reconnect: a dropped or + /// evicted link is a dropped `SshConnection`, and the one dialled in its + /// place starts with an empty slot. Nothing has to remember to forget. + /// What does have to remember is anything that changes the server *over + /// there* while the link stays up — see + /// [`crate::daemon::install::forget_remote_server`]. + /// + /// A blocking `Mutex` on purpose: the probe behind it is a chain of + /// blocking SSH round trips run on a blocking thread, and a pane that + /// arrives mid-install wants to wait for that install rather than start a + /// second one. + pub(crate) fn proved_server(&self) -> std::sync::MutexGuard<'_, Option> { + self.proved_server + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + } + + /// Where this connection's server was last proved to be, or `None` if the + /// next pane would have to go and ask — including while it is being asked, + /// since this never waits. A hint for callers deciding whether a failure is + /// worth re-proving; the answer itself comes from `ensure_remote_server`. + pub fn remembered_server(&self) -> Option { + self.proved_server + .try_lock() + .ok() + .and_then(|slot| slot.as_ref().map(|proved| proved.binary.clone())) + } + pub async fn add_remote_forward( &self, bind_host: &str, From dbef42a03d70adb8830e02a7f54320522b95fa38 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:00:28 +0800 Subject: [PATCH 02/15] fix(machine): keep earlier generations of the machine tree (#716) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `MachineStore::persist` serialises the whole document and lands it with `write_atomic_private`, so the file on disk is never torn — but the document it replaces is simply gone. The tree is rewritten whole on every mutation, which means the write that loses a layout is also the write that erases the only copy of it. That is what turned #716 from an annoyance into a lost afternoon: a client's arrival queued nineteen `TabClose`s, each one persisted, and by the time anyone read `machine.json` there was nothing anywhere on the machine that remembered what the workspace had looked like. `persist` now rotates the document it is about to replace into a small ring of backups beside it — `machine.json.bak`, then `.bak.1` and `.bak.2` behind it — before the new one is written. Two things decide whether such a ring is worth anything. The first is what "previous good" means. It cannot mean "the last document that parsed": an emptied tree parses perfectly and is exactly the state you want to recover *from*, so validity is no signal at all. Age is the only signal available, so the ring is spaced: a generation is taken only when the newest one is at least five minutes old. Without that spacing the failure mode above — a burst of writes seconds apart — would have rolled three copies of the damage through the whole ring before a human noticed. With it, the oldest generation is a quarter of an hour of history, and three generations is where the ring stops so a file rewritten every few seconds does not grow a history without bound. The second is the atomicity of the rotation itself, since a rotation that can lose both copies is worse than none. The live file is never renamed, only read: at every point in `keep_a_generation` the tree is still completely at its own path, and the new generation lands through `write_atomic_private` — a sibling temporary renamed into place, which also means the copies inherit the 0600 the live tree is written under rather than widening anything. A crash mid-rotation costs at most one backup generation and never the tree. A backup that cannot be written is logged and the new document is persisted anyway; a machine with no backup still has to work. `load_machine` now also falls back to the newest generation that parses when the live document is unreadable or corrupt, instead of starting from an empty tree. Deliberately only for those two cases: a tree that parses always wins, however empty it is, because that is the case a human has to judge — which is why the files are plain JSON under obvious names, ready to be copied back by hand. Claude-Session: https://claude.ai/code/session_01UUyWQXzcBAoBzaSX8pc7nU --- crates/tty7-core/src/core/machine.rs | 234 +++++++++++++++++++++++++-- 1 file changed, 225 insertions(+), 9 deletions(-) diff --git a/crates/tty7-core/src/core/machine.rs b/crates/tty7-core/src/core/machine.rs index 4e754afa..5479709c 100644 --- a/crates/tty7-core/src/core/machine.rs +++ b/crates/tty7-core/src/core/machine.rs @@ -26,6 +26,28 @@ pub const FACT_FLUSH_INTERVAL: Duration = Duration::from_secs(2); #[cfg(test)] pub const FACT_FLUSH_INTERVAL: Duration = Duration::from_secs(600); +/// How many earlier generations of the machine tree are kept beside it. +/// +/// Bounded on purpose: the tree is rewritten whole on every mutation, so an +/// unbounded history would be a new file every few seconds forever. Three is +/// what the spacing below has to work with. +const BACKUP_GENERATIONS: usize = 3; + +/// How far apart those generations are taken. +/// +/// The point of the spacing is that "the previous write" is worthless as a +/// backup here. The tree is written whole on every mutation, and the failure +/// worth recovering from arrives as a burst — #716 emptied a workspace with +/// nineteen `TabClose`s in a row, each of them a persist. A backup taken on +/// every write would have rolled the damage through all three generations +/// before anyone looked. Spaced out, the oldest generation is a quarter of an +/// hour of history, which is longer than any burst. +/// +/// Note that "previous good" cannot mean "the last document that parsed": +/// an emptied tree parses perfectly and is exactly what you want to recover +/// *from*. Age is the only signal available here, so age is what is used. +const BACKUP_SPACING: Duration = Duration::from_secs(300); + #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] #[serde(transparent)] pub struct TabId(uuid::Uuid); @@ -1116,6 +1138,15 @@ impl MachineStore { if let Some(parent) = self.path.parent() { std::fs::create_dir_all(parent)?; } + // Before the overwrite, never after: the backup is the document about + // to be replaced. A failure here is not a reason to stop writing the + // new one — a machine with no backup still has to keep working. + if let Err(e) = keep_a_generation(&self.path, BACKUP_SPACING) { + log::warn!( + "could not keep an earlier generation of {}: {e}", + self.path.display() + ); + } crate::core::config::write_atomic_private(&self.path, &bytes) } @@ -1210,24 +1241,96 @@ fn load_machine(path: &Path) -> Machine { Err(e) => { log::warn!("could not read {}; quarantining it: {e}", path.display()); crate::core::config::quarantine_by_rename(path); - return Machine::default(); + return load_a_backup(path).unwrap_or_default(); } }; - match serde_json::from_str::(crate::core::config::strip_bom(&text)) { - Ok(mut machine) => { - for pane in &mut machine.panes { - pane.live = false; - } - machine - } + match parse_machine(&text) { + Ok(machine) => machine, Err(e) => { log::warn!("{} does not parse ({e}); quarantining it", path.display()); crate::core::config::quarantine(path); - Machine::default() + load_a_backup(path).unwrap_or_default() } } } +fn parse_machine(text: &str) -> serde_json::Result { + let mut machine = serde_json::from_str::(crate::core::config::strip_bom(text))?; + for pane in &mut machine.panes { + pane.live = false; + } + Ok(machine) +} + +/// The newest kept generation that still parses. +/// +/// Only reached when the live document is gone or unreadable — a tree that +/// parses is always preferred to a backup, however empty it turns out to be. +/// This is the automatic half of [`keep_a_generation`]; the manual half is a +/// human copying a `.bak` over `machine.json`, which is why the files are +/// plain JSON under obvious names. +fn load_a_backup(path: &Path) -> Option { + (0..BACKUP_GENERATIONS).find_map(|generation| { + let kept = backup_path(path, generation); + let machine = parse_machine(&std::fs::read_to_string(&kept).ok()?).ok()?; + log::warn!("recovered the machine tree from {}", kept.display()); + Some(machine) + }) +} + +/// Where generation `n` of `path` is kept — `machine.json.bak` for the newest, +/// `machine.json.bak.1` and `.bak.2` behind it. +fn backup_path(path: &Path, generation: usize) -> PathBuf { + let name = path + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(MACHINE_FILE); + match generation { + 0 => path.with_file_name(format!("{name}.bak")), + n => path.with_file_name(format!("{name}.bak.{n}")), + } +} + +/// Rotates the document currently at `path` into the backup ring, if the ring's +/// newest entry is at least `spacing` old. +/// +/// The live file is never renamed, only read: at every point in this function +/// `path` still holds a complete document, so a crash part-way through costs at +/// most one *backup* generation and never the tree itself. The new generation +/// lands through `write_atomic_private`, which writes a sibling temporary and +/// renames it into place — so a reader never sees a half-written `.bak` either, +/// and the 0600 the live tree is written under is the mode the copies get. +/// +/// Returns `Ok(())` when there was nothing to do: no tree yet, or the newest +/// generation is younger than `spacing`. +fn keep_a_generation(path: &Path, spacing: Duration) -> io::Result<()> { + let bytes = match std::fs::read(path) { + Ok(bytes) => bytes, + // Nothing has been written yet, so there is no previous generation. + Err(e) if e.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(e), + }; + let newest = backup_path(path, 0); + let too_soon = std::fs::metadata(&newest) + .and_then(|meta| meta.modified()) + .map(|at| at.elapsed().unwrap_or_default() < spacing) + .unwrap_or(false); + if too_soon { + return Ok(()); + } + // Oldest first, so nothing is overwritten before it has been moved down. + // A generation that is not there yet simply has nothing to move. + for generation in (1..BACKUP_GENERATIONS).rev() { + let from = backup_path(path, generation - 1); + match std::fs::rename(&from, backup_path(path, generation)) { + Ok(()) => {} + Err(e) if e.kind() == io::ErrorKind::NotFound => {} + Err(e) => return Err(e), + } + } + crate::core::config::write_atomic_private(&newest, &bytes) +} + static OBSERVED: Mutex>> = Mutex::new(None); pub fn publish_observations(store: &Arc) { @@ -2569,6 +2672,119 @@ mod tests { ); } + /// One tree with `tabs` tabs in it, written out the way `persist` writes. + fn document(tabs: u64) -> Vec { + let machine = Machine { + workspaces: vec![Workspace { + tabs: (0..tabs).map(Tab::leaf).collect(), + ..Workspace::default() + }], + panes: Vec::new(), + }; + serde_json::to_vec_pretty(&machine).unwrap() + } + + fn tabs_in(path: &Path) -> usize { + let text = std::fs::read_to_string(path).expect("a generation must be there to read"); + parse_machine(&text).expect("and it must parse").workspaces[0] + .tabs + .len() + } + + /// A machine tree is written whole on every mutation, so the write that + /// loses the layout also erases the only copy of it (#716). One generation + /// back is the least that makes a bad write survivable by hand. + #[test] + fn the_document_being_replaced_is_kept_beside_it() { + let dir = tempfile::TempDir::new().unwrap(); + let path = dir.path().join(MACHINE_FILE); + let store = MachineStore::open(&path); + + let ws = store.workspace_create(None, None, None).unwrap(); + store.tab_create(ws.id, None, seed(1, "/work"), None, None).unwrap(); + store.tab_create(ws.id, None, seed(2, "/work"), None, None).unwrap(); + + let kept = backup_path(&path, 0); + assert!( + kept.exists(), + "the second write has a first write to keep: {}", + kept.display() + ); + assert_eq!( + tabs_in(&kept), + 0, + "and what it kept is the document that write replaced" + ); + } + + /// The whole reason the generations are spaced. The failure this came from + /// arrived as a burst — nineteen `TabClose`s, nineteen persists, seconds + /// apart — and a ring that rotated on every write would have held three + /// copies of the damage by the time anyone looked at it. + #[test] + fn a_burst_of_writes_cannot_roll_the_history_away() { + let dir = tempfile::TempDir::new().unwrap(); + let path = dir.path().join(MACHINE_FILE); + std::fs::write(&path, document(19)).unwrap(); + keep_a_generation(&path, Duration::ZERO).unwrap(); + + for remaining in (0..19).rev() { + std::fs::write(&path, document(remaining)).unwrap(); + keep_a_generation(&path, BACKUP_SPACING).unwrap(); + } + + assert_eq!(tabs_in(&path), 0, "the burst emptied the live tree"); + assert_eq!( + tabs_in(&backup_path(&path, 0)), + 19, + "and the kept generation is from before it, not from one write ago" + ); + } + + #[test] + fn the_ring_keeps_three_generations_and_no_more() { + let dir = tempfile::TempDir::new().unwrap(); + let path = dir.path().join(MACHINE_FILE); + + for tabs in 1..=6 { + std::fs::write(&path, document(tabs)).unwrap(); + keep_a_generation(&path, Duration::ZERO).unwrap(); + } + + assert_eq!(tabs_in(&backup_path(&path, 0)), 6); + assert_eq!(tabs_in(&backup_path(&path, 1)), 5); + assert_eq!(tabs_in(&backup_path(&path, 2)), 4); + assert!( + !backup_path(&path, 3).exists(), + "the ring is bounded; a tree rewritten every few seconds must not \ + grow a file per write forever" + ); + } + + /// The automatic half of the recovery. A tree that parses always wins — + /// including an empty one, which is why the manual `.bak` copy still + /// matters — but a tree that does not parse used to mean starting over. + #[test] + fn a_tree_that_does_not_parse_is_loaded_from_the_newest_backup() { + let dir = tempfile::TempDir::new().unwrap(); + let path = dir.path().join(MACHINE_FILE); + std::fs::write(&path, document(3)).unwrap(); + keep_a_generation(&path, Duration::ZERO).unwrap(); + std::fs::write(&path, b"{ not json").unwrap(); + + let store = MachineStore::open(&path); + + assert_eq!( + store.machine().workspaces[0].tabs.len(), + 3, + "the layout comes back from the kept generation" + ); + assert!( + path.with_extension("json.corrupt").exists(), + "and the unparseable document is still kept for inspection" + ); + } + #[test] fn a_sparse_document_decodes_with_defaults() { let machine: Machine = From a6e12716975bbe09040f4578c5f77241906a0b7e Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:07:58 +0800 Subject: [PATCH 03/15] fix(terminal): decide the parked-cursor repair per pty, not per build (#774) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repair that puts back the cursor a conhost repaint parks was gated on `const REPAIR_PARKED_CURSOR: bool = cfg!(windows)` — a compile-time answer about the platform the client was built for, when the question it is standing in for is what kind of pty this pane is attached to. Those are not the same question, and a Windows client's panes are not all alike. A local shell is a ConPTY and so is `wsl.exe` or an `ssh` client running inside one, because they are ordinary programs in the same conhost-rendered buffer. A pane on a remote `tty7-server` is not: a remote workspace only installs onto Linux or macOS (`asset_for_uname` refuses everything else), so its pty is raw. Nor is a native-SSH pane, which the local daemon dials but opens no pty for — it bridges an ssh channel straight through. The repair is written for what conhost does: it brackets a frame in `?25l` … `?25h` and, on the frames where it did not paint the cursor, commits the show wherever the last erase left it. Applied to a raw pty it is not a correction but a corruption, because there the application owns the cursor and may deliberately end a repaint on the text it just wrote. Vim opens its `:` command line exactly that way and then echoes the following keystrokes as bare bytes, so moving the cursor back to where the repaint hid it drops the `wq` two rows up, onto the line being edited. That is #430 again, on the path the Mac fix never covered: a Windows client talking to a Linux host. So the decision moves to a per-pane `PtySource`, resolved at runtime from the route the pane was spawned or attached on and carried to the reader as a shared flag. `RemoteContext` is a second source of the answer, because the route cannot supply it: a native-SSH pane is spawned and attached through the local daemon like any other, so a window reopening onto an existing one by pane id has nothing else to go on. Learning `RemoteKind::NativeSsh` turns the repair off for good — one way, since the far end of an ssh channel never becomes a local pty later, and since an `ssh` *command* (`RemoteKind::Ssh`) is a program inside a ConPTY and must keep the repair. A relink reuses the pane's existing answer rather than recomputing one, for the same reason: the pty on the far side is the pty it always was. Nothing about when a cursor counts as parked changed — `ParkedCursorScanner` and `ParkedCursorRepair` are untouched, and the Windows behaviour they were written for is unchanged for every pane that really is on a ConPTY. The tests that covered them move out of the `cfg(unix)` module and onto the socket-pair harness that already runs on both platforms, so both answers are now driven directly instead of one of them being whatever the host happened to compile: a conhost frame on a ConPTY still parks, the same frame off a raw pty does not, vim's `:wq!` lands on the command line, and a native-SSH context turns the repair off mid-stream. Claude-Session: https://claude.ai/code/session_01UUyWQXzcBAoBzaSX8pc7nU --- src/terminal/parked_cursor.rs | 7 +- src/terminal/remote.rs | 519 ++++++++++++++++++++++++---------- 2 files changed, 372 insertions(+), 154 deletions(-) diff --git a/src/terminal/parked_cursor.rs b/src/terminal/parked_cursor.rs index facad126..66f3628c 100644 --- a/src/terminal/parked_cursor.rs +++ b/src/terminal/parked_cursor.rs @@ -16,13 +16,14 @@ //! invisible, which is the cell the correcting frame would have moved it back //! to anyway. //! -//! Only conhost parks a cursor, so the reader runs this on Windows alone -//! (`RemoteTerminal::REPAIR_PARKED_CURSOR`). Off it the pty is raw and the +//! Only conhost parks a cursor, so the reader runs this for panes on a ConPTY +//! alone — see [`crate::terminal::remote::PtySource`], which answers that per +//! pane rather than per build. Everywhere else the pty is raw and the //! application's cursor is the real one: a TUI is free to end a repaint on the //! text it just wrote and then echo the next keystroke straight after it, with //! no positioning of its own — vim opens its `:` command line exactly that way, //! which a repair on a raw pty turns into `wq!` landing on the row being edited -//! (#430). +//! (#430; #774 for the Windows client whose Linux panes were repaired too). use std::time::{Duration, Instant}; diff --git a/src/terminal/remote.rs b/src/terminal/remote.rs index b2f67038..01650704 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -23,8 +23,9 @@ use crate::core::osc::OscTokenizer; use crate::daemon::protocol::{ AuthPromptKind, AuthResponse, ClientMsg, DaemonMsg, KnownHostEntry, KnownHostId, LoopbackForward, LoopbackForwardRequest, ManagedForward, NativeSshSpec, PaneProcs, - RemoteContext, RestoreFrom, SftpEntry, SftpJobProgress, SftpOp, SftpOpResult, SftpTransferSpec, - ShellSpec, SshForwardRule, SshPhase, SshTestReport, WinSize, WorkspaceOp, WorkspaceRequest, + RemoteContext, RemoteKind, RestoreFrom, SftpEntry, SftpJobProgress, SftpOp, SftpOpResult, + SftpTransferSpec, ShellSpec, SshForwardRule, SshPhase, SshTestReport, WinSize, WorkspaceOp, + WorkspaceRequest, }; use crate::daemon::transport::{self, Stream}; use gpui::EntityId; @@ -97,6 +98,60 @@ struct ReaderSignals { /// [`crate::terminal::agent_marks`]. The daemon reads the same events for /// the status dot, but only the client holds the rows they point into. turns: AgentTurns, + /// Whether this pane's pty is one a conhost renders into, and so whether + /// the reader puts back the cursor a repaint parked. Decided per pane from + /// its [`PtySource`], and shared rather than copied because the reader can + /// learn better mid-stream — see the `RemoteContext` arm. + repair_cursor: Arc, +} + +/// What kind of pty is at the far end of a pane's link, which is what decides +/// whether a conhost stands between the application and us. +/// +/// The distinction is not the platform this client was built for. A Windows +/// client's panes are a mix: a local shell — or `wsl.exe`, or an `ssh` client, +/// which are ordinary programs inside the same ConPTY — is rendered by conhost, +/// while a pane on a remote `tty7-server` (Linux or macOS only, see +/// [`tty7_core::daemon::install::asset::asset_for_uname`]) or on a native-SSH +/// channel is a raw unix pty whose bytes reach us untouched. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PtySource { + /// A pty this machine's Windows daemon opened: a ConPTY, with conhost + /// painting frames into it. + LocalConpty, + /// A pty nothing repaints on our behalf — every pty on unix, and every pty + /// reached over a link. + Raw, +} + +impl PtySource { + /// The source a pane spawned or attached on `route` reads from. The + /// platform enters here and nowhere else: a local route means a pty this + /// machine opened, and only on Windows is that a ConPTY. + /// + /// `Unroutable` is a route that could not be resolved, so it never gets a + /// pty at all; answering as if it were local costs nothing and keeps the + /// match total. + fn for_route(route: &PaneRoute) -> PtySource { + match route { + PaneRoute::Local | PaneRoute::Unroutable(_) if cfg!(windows) => PtySource::LocalConpty, + _ => PtySource::Raw, + } + } + + /// Whether the cursor a repaint parked has to be put back — see + /// [`crate::terminal::parked_cursor`]. + /// + /// Only conhost parks one. On a raw pty the application owns the cursor and + /// is free to end a repaint on the text it just wrote and then echo the + /// next keystroke straight after it, with no positioning of its own: vim + /// opens its command line that way, and putting the cursor back on the cell + /// the repaint hid it on drops the `wq!` typed next onto the row being + /// edited (#430, and #774 for the Windows client that reached a Linux host + /// and was repaired anyway). + fn repairs_parked_cursor(self) -> bool { + self == PtySource::LocalConpty + } } #[derive(Clone, Debug, PartialEq)] @@ -553,6 +608,12 @@ pub struct RemoteTerminal { /// flag under the term lock before every grid mutation, so once it is set /// the abandoned thread can only exit, never write. reader_quit: Arc, + /// Whether this pane's pty is a ConPTY, and so whether the reader repairs + /// the cursor a repaint parks. Held here so a relink hands the same answer + /// to the reader it starts: a pane's pty does not change kind when the link + /// to it is rebuilt, and the route a relink carries cannot tell a + /// native-SSH pane from a local shell. + repair_cursor: Arc, } /// The workspace id a spawn carries, so the pane's shell gets `$TTY7_WS` and a @@ -705,7 +766,8 @@ impl RemoteTerminal { } }; - let mut term = Self::from_stream(stream, size)?; + let mut term = + Self::from_stream_with(stream, size, Vec::new(), PtySource::for_route(route))?; term.route = route.clone(); term.seed_cwd(spawned_in); Ok((term, pane_id)) @@ -775,7 +837,7 @@ impl RemoteTerminal { } Err(e) => return Err(e), }; - let mut term = Self::from_stream_with(stream, size, buffered)?; + let mut term = Self::from_stream_with(stream, size, buffered, PtySource::for_route(route))?; term.route = route.clone(); Ok(term) } @@ -862,6 +924,11 @@ impl RemoteTerminal { clipboard_writes: self.clipboard_writes.clone(), clipboard_write_busy: self.clipboard_write_busy.clone(), turns: self.turns.clone(), + // Deliberately the pane's existing answer rather than one + // rebuilt from `route`: the pty on the far side is the same pty + // it was before the link dropped, and only this value still + // remembers what a `RemoteContext` taught the old reader. + repair_cursor: self.repair_cursor.clone(), }, ); self.reader_thread = Some(reader); @@ -876,14 +943,22 @@ impl RemoteTerminal { Ok(()) } + /// A pane on a pty of this machine's own — what the tests build, and what + /// `spawn_on` narrows with the route it dialled. pub(super) fn from_stream(stream: Stream, size: TermSize) -> anyhow::Result { - Self::from_stream_with(stream, size, Vec::new()) + Self::from_stream_with( + stream, + size, + Vec::new(), + PtySource::for_route(&PaneRoute::Local), + ) } pub(super) fn from_stream_with( stream: Stream, size: TermSize, buffered: Vec, + pty: PtySource, ) -> anyhow::Result { let read_half = stream.try_clone()?; let write_half = stream; @@ -918,6 +993,7 @@ impl RemoteTerminal { let turns = AgentTurns::new(); let reader_quit = Arc::new(AtomicBool::new(false)); + let repair_cursor = Arc::new(AtomicBool::new(pty.repairs_parked_cursor())); let reader_thread = Self::spawn_reader( term.clone(), proxy.clone(), @@ -941,6 +1017,7 @@ impl RemoteTerminal { clipboard_writes: clipboard_writes.clone(), clipboard_write_busy: clipboard_write_busy.clone(), turns: turns.clone(), + repair_cursor: repair_cursor.clone(), }, ); @@ -981,6 +1058,7 @@ impl RemoteTerminal { proxy, reader_thread: Some(reader_thread), reader_quit, + repair_cursor, }) } @@ -1023,17 +1101,6 @@ impl RemoteTerminal { term.set_options(terminal_config_from_user(user_config)); } - /// Whether this build puts back the cursor a repaint parked — see - /// [`crate::terminal::parked_cursor`]. - /// - /// Only conhost parks one, so like `conpty_resize` the repair is Windows' - /// alone. On a raw pty the application owns the cursor and is free to end a - /// repaint on the text it just wrote and then echo the next keystroke - /// straight after it, with no positioning of its own: vim opens its command - /// line that way, and putting the cursor back on the cell the repaint hid it - /// on drops the `wq!` typed next onto the row being edited (#430). - const REPAIR_PARKED_CURSOR: bool = cfg!(windows); - fn spawn_reader( term: Arc>>, proxy: EventProxy, @@ -1062,6 +1129,7 @@ impl RemoteTerminal { clipboard_writes, clipboard_write_busy, turns, + repair_cursor, } = signals; crate::core::threads::promote_to_user_interactive(); let mut stream = read_half; @@ -1128,7 +1196,7 @@ impl RemoteTerminal { // emulator to the cut, act on the state that // sequence left behind, carry on. let mut cuts: Vec<(usize, Cut)> = Vec::new(); - if Self::REPAIR_PARKED_CURSOR { + if repair_cursor.load(Ordering::Relaxed) { cursor_scan .feed(&out_batch, |off, c| cuts.push((off, Cut::Cursor(c)))); } @@ -1451,6 +1519,24 @@ impl RemoteTerminal { if let Ok(mut guard) = cwd.lock() { *guard = None; } + // A native-SSH pane's pty is the far host's, + // however local the daemon that dialled it: the + // daemon bridges an ssh channel straight through + // and opens no ConPTY of its own. The route + // cannot say so — such a pane is spawned and + // attached through the local daemon like any + // other — so this frame is where a client that + // reopened onto an existing one finds out. A + // one-way latch: the far end of an ssh channel + // never becomes a local pty later, while an + // `ssh` *command* (RemoteKind::Ssh) is a program + // inside a ConPTY and keeps the repair. + if ctx + .as_ref() + .is_some_and(|c| c.kind == RemoteKind::NativeSsh) + { + repair_cursor.store(false, Ordering::Relaxed); + } if let Ok(mut guard) = remote.lock() { *guard = ctx; } @@ -1842,7 +1928,10 @@ impl RemoteTerminal { } }; - let mut term = Self::from_stream(stream, size)?; + // The local daemon dialled this one, but it opened no pty for it: the + // pane is an ssh channel bridged straight through, so the bytes are the + // far host's raw pty and no conhost ever sees them. + let mut term = Self::from_stream_with(stream, size, Vec::new(), PtySource::Raw)?; term.ssh_endpoint = Some(endpoint); term.ssh_user = Some(user); term.auto_supplied_password = auto_supplied_password; @@ -3146,8 +3235,13 @@ mod replay_tests { !buffered.is_empty(), "the handshake read nothing, so it proves nothing" ); - let term = - RemoteTerminal::from_stream_with(client_side, TermSize::new(80, 24), buffered).unwrap(); + let term = RemoteTerminal::from_stream_with( + client_side, + TermSize::new(80, 24), + buffered, + PtySource::Raw, + ) + .unwrap(); let text = settled(&term, &["BIRTH-BANNER", "LAST-ROW-OF-THE-RING"]); assert!(text.contains("BIRTH-BANNER"), "grid held:\n{text}"); @@ -3723,6 +3817,253 @@ mod route_header_tests { } } +/// Issue #430, and #774 for the half of it that was still open. +/// +/// Whether the reader puts back a parked cursor is a property of the pty this +/// pane is attached to, not of the platform the client was compiled for — so +/// these drive both answers on every platform, over a real socket pair. Before +/// #774 the decision was `cfg!(windows)`, which meant a Windows client talking +/// to a Linux host repaired a cursor no conhost had parked, and the `wq` typed +/// after vim's `:` landed on the row being edited. +#[cfg(test)] +mod parked_cursor_tests { + use super::replay_tests::socket_pair; + use super::*; + use std::io::Write as _; + + fn terminal_on(pty: PtySource, size: TermSize) -> (RemoteTerminal, Stream) { + crate::core::config::pin_test_config_dir(); + let (client_side, daemon_side) = socket_pair(); + let term = RemoteTerminal::from_stream_with(client_side, size, Vec::new(), pty) + .expect("a terminal over a socket pair"); + (term, daemon_side) + } + + /// Feeds one conhost-shaped repaint and reports the cell the cursor ends on, + /// waiting for the `X` the frame paints so the reader is known to be done. + fn cursor_after_conpty_frame(pty: PtySource, frame: &[u8]) -> (i32, usize) { + let (term, mut daemon_side) = terminal_on(pty, TermSize::new(80, 24)); + + // Where the TUI put the cursor before conhost repainted over it. + DaemonMsg::Output(b"\x1b[6;4H".to_vec()) + .encode(&mut daemon_side) + .unwrap(); + DaemonMsg::Output(frame.to_vec()) + .encode(&mut daemon_side) + .unwrap(); + daemon_side.flush().unwrap(); + + for _ in 0..600 { + { + let t = term.term.lock(); + let painted = t.grid()[alacritty_terminal::index::Line(19)] + [alacritty_terminal::index::Column(1)] + .c; + if painted == 'X' { + let point = t.grid().cursor.point; + return (point.line.0, point.column.0); + } + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + panic!("the reader never applied the frame"); + } + + #[test] + fn a_local_route_is_a_conpty_only_where_conhost_exists() { + assert_eq!( + PtySource::for_route(&PaneRoute::Local), + if cfg!(windows) { + PtySource::LocalConpty + } else { + PtySource::Raw + }, + ); + assert!(PtySource::LocalConpty.repairs_parked_cursor()); + assert!(!PtySource::Raw.repairs_parked_cursor()); + } + + #[test] + fn a_routed_pane_reads_a_raw_pty_whatever_the_client_was_built_for() { + let spec: NativeSshSpec = serde_json::from_str( + r#"{"host":"linux-box","port":22,"user":"dev","auth_mode":"auto"}"#, + ) + .unwrap(); + // A remote workspace only ever installs a `tty7-server` on Linux or + // macOS (`install::asset::asset_for_uname`), and a WSL workspace is a + // Linux server too, so a routed pane's pty is a raw one — the far end + // is never a conhost, whoever is dialling it. + for header in [ + crate::daemon::router::RouteHeader::ssh(spec), + crate::daemon::router::RouteHeader::wsl("Ubuntu-22.04"), + ] { + let route = PaneRoute::Remote { + header: Box::new(header), + resize_echo: false, + }; + assert_eq!(PtySource::for_route(&route), PtySource::Raw); + } + } + + #[test] + fn a_conpty_frame_that_shows_the_cursor_over_an_erase_keeps_the_cell_it_hid_on() { + assert_eq!( + cursor_after_conpty_frame( + PtySource::LocalConpty, + b"\x1b[?25l\x1b[20;2HX\x1b[K\x1b[m\x1b[22;42H\x1b[K\x1b[?25h", + ), + (5, 3), + "conhost parked the cursor on the cell it erased last; the cursor \ + belongs where it was when the repaint hid it" + ); + } + + #[test] + fn the_same_frame_off_a_raw_pty_leaves_the_cursor_where_the_frame_left_it() { + assert_eq!( + cursor_after_conpty_frame( + PtySource::Raw, + b"\x1b[?25l\x1b[20;2HX\x1b[K\x1b[m\x1b[22;42H\x1b[K\x1b[?25h", + ), + (21, 41), + "with no conhost in between the stream is the application's own, \ + and the cell it left the cursor on is the cell it meant" + ); + } + + #[test] + fn a_conpty_frame_that_moves_the_cursor_before_showing_it_is_obeyed() { + assert_eq!( + cursor_after_conpty_frame( + PtySource::LocalConpty, + b"\x1b[?25l\x1b[20;2HX\x1b[K\x1b[m\x1b[22;42H\x1b[K\x1b[9;9H\x1b[?25h" + ), + (8, 8), + "the frame painted the cursor somewhere on purpose" + ); + } + + /// Vim opens its command line with exactly the shape the parked-cursor + /// scanner calls parked — hide, move around to paint, end on the `:` it + /// wrote — and then echoes every following keystroke as a bare byte at + /// wherever that left the cursor. Putting the cursor back on a raw pty + /// therefore does not straighten out a stray caret, it drops `wq!` onto the + /// row vim was editing. Bytes below are a capture of vim 9 on a 20x11 pty. + #[test] + fn a_raw_pty_repaint_keeps_the_cursor_the_frame_left_so_the_echo_lands_on_it() { + let (term, mut daemon_side) = terminal_on(PtySource::Raw, TermSize::new(20, 11)); + + let mut stream: Vec = Vec::new(); + // `vim test.md`: the alternate screen, the file, cursor home. + stream.extend_from_slice(b"\x1b[?1049h\x1b[H\x1b[2J\x1b[1;1H123456789\x1b[1;1H"); + // Esc, then `:` — two bracketed repaints, the second ending on the `:` + // vim wrote at the head of the command line. + stream.extend_from_slice(b"\x1b[?25l\x1b[m\x1b[11;10H^[\x1b[1;1H\x1b[?25h"); + stream.extend_from_slice(b"\x1b[?25l\x1b[11;10H \x1b[1;1H\x07\x1b[?25h"); + stream.extend_from_slice( + b"\x1b[?25l\x1b[11;10H:\x1b[1;1H\x1b[11;1H\x1b[K\x1b[11;1H:\x1b[?25h", + ); + // `w`, `q`, `!`: vim echoes them with no positioning of their own. + stream.extend_from_slice(b"wq!"); + DaemonMsg::Output(stream).encode(&mut daemon_side).unwrap(); + daemon_side.flush().unwrap(); + + let row = |t: &Term, line: i32| -> String { + (0..20) + .map(|col| { + t.grid()[alacritty_terminal::index::Line(line)] + [alacritty_terminal::index::Column(col)] + .c + }) + .collect::() + .trim_end() + .to_string() + }; + + // The whole batch is applied under one lock, so the `:` landing on the + // command line means every byte after it landed too. + let mut command_line = String::new(); + let mut edited = String::new(); + for _ in 0..600 { + { + let t = term.term.lock(); + command_line = row(&t, 10); + edited = row(&t, 0); + } + if command_line.starts_with(':') { + break; + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + assert_eq!( + command_line, ":wq!", + "the keystrokes belong after the `:` the repaint ended on" + ); + assert_eq!( + edited, "123456789", + "and nothing of them belongs on the row vim was editing" + ); + } + + /// The route cannot tell a native-SSH pane from a local shell: both are + /// spawned through this machine's daemon. What tells them apart is the + /// `RemoteContext` the daemon sends — and it has to, because a window + /// reopening onto an existing native-SSH pane attaches by id and has + /// nothing else to go on. + #[test] + fn a_native_ssh_context_turns_the_repair_off_mid_stream() { + let (term, mut daemon_side) = terminal_on(PtySource::LocalConpty, TermSize::new(80, 24)); + + DaemonMsg::RemoteContext(Some(RemoteContext { + kind: RemoteKind::NativeSsh, + argv: Vec::new(), + target: "dev@linux-box".into(), + })) + .encode(&mut daemon_side) + .unwrap(); + daemon_side.flush().unwrap(); + for _ in 0..600 { + if term.remote_context().is_some() { + break; + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + assert!( + term.remote_context().is_some(), + "the reader never applied the context" + ); + + DaemonMsg::Output(b"\x1b[6;4H".to_vec()) + .encode(&mut daemon_side) + .unwrap(); + DaemonMsg::Output(b"\x1b[?25l\x1b[20;2HX\x1b[K\x1b[m\x1b[22;42H\x1b[K\x1b[?25h".to_vec()) + .encode(&mut daemon_side) + .unwrap(); + daemon_side.flush().unwrap(); + + let mut cursor = (0, 0); + for _ in 0..600 { + { + let t = term.term.lock(); + let painted = t.grid()[alacritty_terminal::index::Line(19)] + [alacritty_terminal::index::Column(1)] + .c; + if painted == 'X' { + let point = t.grid().cursor.point; + cursor = (point.line.0, point.column.0); + break; + } + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + assert_eq!( + cursor, + (21, 41), + "an ssh channel's bytes are the far host's own; nothing parked that cursor" + ); + } +} + #[cfg(all(test, unix))] mod tests { use super::*; @@ -4152,8 +4493,13 @@ mod tests { !buffered.is_empty(), "the classification read the Snapshot frame; it must come back" ); - let term = - RemoteTerminal::from_stream_with(client_side, TermSize::new(80, 24), buffered).unwrap(); + let term = RemoteTerminal::from_stream_with( + client_side, + TermSize::new(80, 24), + buffered, + PtySource::Raw, + ) + .unwrap(); let mut got = String::new(); for _ in 0..200 { @@ -4815,135 +5161,6 @@ mod tests { ); } - /// Feeds one conhost-shaped repaint and reports the cell the cursor ends on, - /// waiting for the `X` the frame paints so the reader is known to be done. - fn cursor_after_conpty_frame(frame: &[u8]) -> (i32, usize) { - crate::core::config::pin_test_config_dir(); - let (client_side, mut daemon_side) = UnixStream::pair().unwrap(); - let term = RemoteTerminal::from_stream(client_side, TermSize::new(80, 24)).unwrap(); - - // Where the TUI put the cursor before conhost repainted over it. - DaemonMsg::Output(b"\x1b[6;4H".to_vec()) - .encode(&mut daemon_side) - .unwrap(); - DaemonMsg::Output(frame.to_vec()) - .encode(&mut daemon_side) - .unwrap(); - daemon_side.flush().unwrap(); - - for _ in 0..600 { - { - let t = term.term.lock(); - let painted = t.grid()[alacritty_terminal::index::Line(19)] - [alacritty_terminal::index::Column(1)] - .c; - if painted == 'X' { - let point = t.grid().cursor.point; - return (point.line.0, point.column.0); - } - } - std::thread::sleep(std::time::Duration::from_millis(5)); - } - panic!("the reader never applied the frame"); - } - - #[test] - fn a_conpty_frame_that_shows_the_cursor_over_an_erase_keeps_the_cell_it_hid_on() { - let got = cursor_after_conpty_frame( - b"\x1b[?25l\x1b[20;2HX\x1b[K\x1b[m\x1b[22;42H\x1b[K\x1b[?25h", - ); - if RemoteTerminal::REPAIR_PARKED_CURSOR { - assert_eq!( - got, - (5, 3), - "conhost parked the cursor on the cell it erased last; the cursor \ - belongs where it was when the repaint hid it" - ); - } else { - assert_eq!( - got, - (21, 41), - "with no conhost in between the stream is the application's own, \ - and the cell it left the cursor on is the cell it meant" - ); - } - } - - #[test] - fn a_conpty_frame_that_moves_the_cursor_before_showing_it_is_obeyed() { - assert_eq!( - cursor_after_conpty_frame( - b"\x1b[?25l\x1b[20;2HX\x1b[K\x1b[m\x1b[22;42H\x1b[K\x1b[9;9H\x1b[?25h" - ), - (8, 8), - "the frame painted the cursor somewhere on purpose" - ); - } - - /// Issue #430. Vim opens its command line with exactly the shape the parked - /// -cursor scanner calls parked — hide, move around to paint, end on the `:` - /// it wrote — and then echoes every following keystroke as a bare byte at - /// wherever that left the cursor. Putting the cursor back on a raw pty - /// therefore does not straighten out a stray caret, it drops `wq!` onto the - /// row vim was editing. Bytes below are a capture of vim 9 on a 20x11 pty. - #[test] - fn a_raw_pty_repaint_keeps_the_cursor_the_frame_left_so_the_echo_lands_on_it() { - crate::core::config::pin_test_config_dir(); - let (client_side, mut daemon_side) = UnixStream::pair().unwrap(); - let term = RemoteTerminal::from_stream(client_side, TermSize::new(20, 11)).unwrap(); - - let mut stream: Vec = Vec::new(); - // `vim test.md`: the alternate screen, the file, cursor home. - stream.extend_from_slice(b"\x1b[?1049h\x1b[H\x1b[2J\x1b[1;1H123456789\x1b[1;1H"); - // Esc, then `:` — two bracketed repaints, the second ending on the `:` - // vim wrote at the head of the command line. - stream.extend_from_slice(b"\x1b[?25l\x1b[m\x1b[11;10H^[\x1b[1;1H\x1b[?25h"); - stream.extend_from_slice(b"\x1b[?25l\x1b[11;10H \x1b[1;1H\x07\x1b[?25h"); - stream.extend_from_slice( - b"\x1b[?25l\x1b[11;10H:\x1b[1;1H\x1b[11;1H\x1b[K\x1b[11;1H:\x1b[?25h", - ); - // `w`, `q`, `!`: vim echoes them with no positioning of their own. - stream.extend_from_slice(b"wq!"); - DaemonMsg::Output(stream).encode(&mut daemon_side).unwrap(); - daemon_side.flush().unwrap(); - - let row = |t: &Term, line: i32| -> String { - (0..20) - .map(|col| { - t.grid()[alacritty_terminal::index::Line(line)] - [alacritty_terminal::index::Column(col)] - .c - }) - .collect::() - .trim_end() - .to_string() - }; - - // The whole batch is applied under one lock, so the `:` landing on the - // command line means every byte after it landed too. - let mut command_line = String::new(); - let mut edited = String::new(); - for _ in 0..600 { - { - let t = term.term.lock(); - command_line = row(&t, 10); - edited = row(&t, 0); - } - if command_line.starts_with(':') { - break; - } - std::thread::sleep(std::time::Duration::from_millis(5)); - } - assert_eq!( - command_line, ":wq!", - "the keystrokes belong after the `:` the repaint ended on" - ); - assert_eq!( - edited, "123456789", - "and nothing of them belongs on the row vim was editing" - ); - } - #[test] fn layout_resize_reasserts_geometry_after_a_late_size_frame() { use alacritty_terminal::grid::Dimensions as _; From 237b2a14c44fafaffcabc9b5d976d2c8a04cafd5 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:09:15 +0800 Subject: [PATCH 04/15] fix(daemon): restore a pane's terminal modes on re-attach (#774) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pane's screen comes back on re-attach out of the replay ring, and the ring is a window: eight megabytes wide, dropped from the front as it fills. That is the right shape for text, which is only worth what is still on screen, and the wrong one for modes. A full-screen program announces itself exactly once — `btop` sends `?1049h` and its mouse-reporting modes when it starts and then does nothing but refresh — so a long enough run of refreshes pushes the only copy of that announcement out of the front of the ring. What the client replays is then a screenful of alternate-buffer frames with nothing left to say they belong on the alternate buffer: it paints them onto its primary screen with reporting off, and `wheel_route`, which reads exactly those modes, sends the wheel to the scrollback of a screen that has none. That is the "a screen that should not scroll starts scrolling" in the report. `replay_state` already refuses to rely on the ring for anything that matters — cwd, prompt state, the remote context, the agent, the exit — because all of those are facts about the pane rather than bytes on it. The modes are the same kind of fact and were the exception, so the daemon now folds the bytes it hands the ring into a small tracker (`core::term_modes`) and `replay_state` re-sends what is still on. Tracked are the modes that decide input routing or which buffer is on screen: the alternate screen in its three spellings, the mouse reporting level and its encodings, alternate scroll, DECCKM, focus reporting and bracketed paste. They are replayed in the order the application set them, because the emulator treats the reporting modes as a level and not as independent bits, so the last one set has to be last here too. The frame goes *ahead* of the ring rather than after it. That way the replayed frames are painted into the buffer they were drawn for, and re-entering an alternate screen the ring turns out to still carry is a no-op in the emulator, so a prefix and a ring that both carry the mode cannot fight. Where the ring does still carry a toggle it wins on its own terms, since the fold runs over every byte the pane ever wrote and therefore agrees with the ring's last word on any mode the ring still mentions. Cursor visibility (`?25`) and autowrap (`?7`) are deliberately not tracked. Any frame of a running TUI repaints them within milliseconds, whereas restoring a stale `?25l` would leave a shell with an invisible cursor — a worse failure than the one being fixed, and one the existing `restore_preamble` already goes out of its way to avoid. A daemon handoff starts the fold empty rather than carrying it, so a pane adopted across a daemon restart is no worse off than it is today; the ring it carries is all it ever had. Claude-Session: https://claude.ai/code/session_01UUyWQXzcBAoBzaSX8pc7nU --- crates/tty7-core/src/core/mod.rs | 1 + crates/tty7-core/src/core/term_modes.rs | 276 ++++++++++++++++++++++++ crates/tty7-core/src/daemon/pane.rs | 90 +++++++- src/terminal/remote.rs | 52 +++++ 4 files changed, 418 insertions(+), 1 deletion(-) create mode 100644 crates/tty7-core/src/core/term_modes.rs diff --git a/crates/tty7-core/src/core/mod.rs b/crates/tty7-core/src/core/mod.rs index f2226e07..43a72d97 100644 --- a/crates/tty7-core/src/core/mod.rs +++ b/crates/tty7-core/src/core/mod.rs @@ -20,6 +20,7 @@ pub mod shells; #[allow(dead_code)] pub mod ssh_profile; pub mod tab_view; +pub mod term_modes; pub mod threads; pub mod window_state; pub mod worktree; diff --git a/crates/tty7-core/src/core/term_modes.rs b/crates/tty7-core/src/core/term_modes.rs new file mode 100644 index 00000000..cc9dea8b --- /dev/null +++ b/crates/tty7-core/src/core/term_modes.rs @@ -0,0 +1,276 @@ +//! Tracks the DEC private modes a pane's output has switched on, so a +//! re-attaching client can be told about them instead of having to find them +//! in the replayed bytes. +//! +//! A pane's screen comes back on re-attach as raw bytes out of the replay ring, +//! and the ring is a *window*: it holds the last few megabytes and drops the +//! rest from the front. That is fine for text, which is only worth what is +//! still on screen, and wrong for modes, which a full-screen program sets +//! exactly once — `btop` sends `?1049h` and its mouse modes at startup and then +//! never again, so a day of refreshes pushes the only copy of them out of the +//! ring. The client that replays what is left ends up painting an alternate +//! screen onto its primary buffer with mouse reporting off, and its wheel falls +//! back to scrolling the scrollback of a screen that should not scroll (#774). +//! +//! So the daemon folds the same bytes into this tracker as they pass, and +//! `replay_state` re-sends what is still on ahead of the ring — the same +//! treatment cwd, the prompt state and the agent already get. +//! +//! Only modes that change how input is routed or which buffer is on screen are +//! tracked. Cursor visibility (`?25`) and autowrap (`?7`) are deliberately left +//! out: any frame of a running TUI paints them back within milliseconds, while +//! restoring them from a stale fold could leave a shell with an invisible +//! cursor, which is a worse failure than the one being fixed. + +/// The modes worth restoring. +/// +/// `47`, `1047` and `1049` are the alternate screen in its three spellings — +/// the mode the wheel consults before it decides the pane has a scrollback to +/// move at all, and the one that decides which buffer the replayed frames are +/// painted into. `1000`, `1002` and `1003` are the mouse reporting level and +/// `1005`, `1006`, `1015` and `1016` its encodings: a program that negotiated +/// SGR and comes back without it reads every wheel report as a click at a +/// wrong, truncated coordinate. `1007` is alternate scroll, which is what turns +/// the wheel into arrow keys inside a full-screen program, and `1` (DECCKM) +/// decides whether those arrows are `ESC O A` or `ESC [ A`. `1004` is focus +/// reporting, which the client stops sending without it, and `2004` bracketed +/// paste — without it a paste into a restored TUI arrives as plain keystrokes, +/// which is how a paste turns into commands. +const TRACKED: &[u16] = &[ + 1, 47, 1047, 1049, 1000, 1002, 1003, 1004, 1005, 1006, 1007, 1015, 1016, 2004, +]; + +/// A CSI longer than this is not a mode set; keep the buffer bounded. +const MAX_PARAMS: usize = 64; + +/// The private modes currently on, in the order they were last switched on. +/// +/// Order matters because the emulator treats some of these as levels rather +/// than as independent bits: setting `?1002` clears the other mouse-reporting +/// modes. Replaying them in the order the application set them therefore lands +/// on the same state the application asked for, whatever it asked for. +#[derive(Debug, Default, Clone)] +pub struct TerminalModes { + on: Vec, + state: State, + params: Vec, +} + +#[derive(Default, Debug, Clone, Copy, PartialEq, Eq)] +enum State { + #[default] + Text, + Esc, + /// A CSI whose parameter bytes are being read. `private` records the `?` + /// that makes it a DEC private mode rather than an ANSI one. + Csi { + private: bool, + }, + Osc, + OscEsc, +} + +impl TerminalModes { + pub fn new() -> Self { + Self::default() + } + + pub fn is_empty(&self) -> bool { + self.on.is_empty() + } + + /// The modes currently on, oldest set first. + pub fn active(&self) -> &[u16] { + &self.on + } + + /// The bytes that put a freshly reset terminal back into these modes, or + /// `None` when there is nothing to restore. + pub fn restore_bytes(&self) -> Option> { + if self.on.is_empty() { + return None; + } + let mut out = Vec::with_capacity(self.on.len() * 8); + for mode in &self.on { + out.extend_from_slice(b"\x1b[?"); + out.extend_from_slice(mode.to_string().as_bytes()); + out.push(b'h'); + } + Some(out) + } + + /// Folds one chunk of pty output into the tracked state. Sequences split + /// across chunks are carried, so the caller may feed whatever sizes the pty + /// hands it. + pub fn feed(&mut self, bytes: &[u8]) { + let mut i = 0; + while i < bytes.len() { + if self.state == State::Text { + let Some(off) = memchr::memchr(0x1b, &bytes[i..]) else { + return; + }; + self.state = State::Esc; + i += off + 1; + continue; + } + let b = bytes[i]; + match self.state { + State::Text => unreachable!(), + State::Esc => match b { + b'[' => { + self.params.clear(); + self.state = State::Csi { private: false }; + } + b']' => self.state = State::Osc, + // RIS. Everything this tracker knows goes back to default, + // exactly as it does in the client's emulator. + b'c' => { + self.on.clear(); + self.state = State::Text; + } + 0x1b => {} + _ => self.state = State::Text, + }, + State::Csi { private } => match b { + b'?' if self.params.is_empty() => self.state = State::Csi { private: true }, + b'0'..=b'9' | b';' => { + self.params.push(b); + if self.params.len() > MAX_PARAMS { + self.state = State::Text; + } + } + b'h' | b'l' => { + if private { + self.apply(b == b'h'); + } + self.state = State::Text; + } + // Any other final byte — or an intermediate such as the `$` + // of a DECRQM query — ends a sequence that is not a mode + // set. Intermediates are lumped in with finals on purpose: + // `?…$p` is a *request*, and answering it is the emulator's + // job, not ours. + _ => self.state = State::Text, + }, + State::Osc => match b { + 0x07 => self.state = State::Text, + 0x1b => self.state = State::OscEsc, + _ => {} + }, + State::OscEsc => match b { + b'\\' => self.state = State::Text, + 0x1b => {} + _ => self.state = State::Osc, + }, + } + i += 1; + } + } + + fn apply(&mut self, on: bool) { + for param in self.params.split(|b| *b == b';') { + let Ok(text) = std::str::from_utf8(param) else { + continue; + }; + let Ok(mode) = text.parse::() else { + continue; + }; + if !TRACKED.contains(&mode) { + continue; + } + // Removed either way: switching a mode on again moves it to the + // back, so the replay repeats the application's own order. + self.on.retain(|m| *m != mode); + if on { + self.on.push(mode); + } + } + self.params.clear(); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn tracks_the_alternate_screen_and_mouse_modes_a_full_screen_tool_sets() { + let mut modes = TerminalModes::new(); + modes.feed(b"\x1b[?1049h\x1b[?1002h\x1b[?1006h"); + assert_eq!(modes.active(), &[1049, 1002, 1006]); + assert_eq!( + modes.restore_bytes().unwrap(), + b"\x1b[?1049h\x1b[?1002h\x1b[?1006h".to_vec() + ); + } + + #[test] + fn a_mode_switched_off_is_forgotten() { + let mut modes = TerminalModes::new(); + modes.feed(b"\x1b[?1049h\x1b[?1006h"); + modes.feed(b"\x1b[?1049l"); + assert_eq!(modes.active(), &[1006]); + + modes.feed(b"\x1b[?1006l"); + assert!(modes.is_empty()); + assert!(modes.restore_bytes().is_none()); + } + + #[test] + fn one_csi_may_carry_several_modes() { + let mut modes = TerminalModes::new(); + modes.feed(b"\x1b[?1000;1002;1006h"); + assert_eq!(modes.active(), &[1000, 1002, 1006]); + modes.feed(b"\x1b[?1000;1002l"); + assert_eq!(modes.active(), &[1006]); + } + + #[test] + fn re_setting_a_mode_moves_it_behind_the_ones_set_since() { + let mut modes = TerminalModes::new(); + // The emulator treats the reporting modes as a level, so the last one + // set is the one that wins — the replay has to end on it too. + modes.feed(b"\x1b[?1002h\x1b[?1003h\x1b[?1002h"); + assert_eq!(modes.active(), &[1003, 1002]); + } + + #[test] + fn a_sequence_split_across_chunks_is_still_seen() { + let mut modes = TerminalModes::new(); + modes.feed(b"\x1b[?10"); + modes.feed(b"49"); + modes.feed(b"h"); + assert_eq!(modes.active(), &[1049]); + } + + #[test] + fn untracked_modes_and_ansi_mode_sets_are_ignored() { + let mut modes = TerminalModes::new(); + // `?25` (cursor) and `?2026` (synchronised update) are not restored, + // and `[4h` is ANSI insert mode, not a private one. + modes.feed(b"\x1b[?25l\x1b[?2026h\x1b[4h\x1b[?1049h"); + assert_eq!(modes.active(), &[1049]); + } + + #[test] + fn a_mode_query_is_not_a_mode_set() { + let mut modes = TerminalModes::new(); + modes.feed(b"\x1b[?1049$p"); + assert!(modes.is_empty()); + } + + #[test] + fn an_osc_payload_that_looks_like_a_mode_set_is_not_one() { + let mut modes = TerminalModes::new(); + modes.feed(b"\x1b]0;\x1b[?1049h\x07\x1b[?1002h"); + assert_eq!(modes.active(), &[1002]); + } + + #[test] + fn a_full_reset_clears_everything() { + let mut modes = TerminalModes::new(); + modes.feed(b"\x1b[?1049h\x1b[?1006h"); + modes.feed(b"\x1bc"); + assert!(modes.is_empty()); + } +} diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index 3954a7e0..232677a9 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -15,6 +15,7 @@ use crate::core::clipboard::{ }; use crate::core::kitty_graphics::{GraphicsSniffer, Segment, Sniffed}; use crate::core::osc::OscTokenizer; +use crate::core::term_modes::TerminalModes; use crate::daemon::protocol::{ AuthResponse, DaemonMsg, MAX_FRAME, NativeSshSpec, PaneInfo, RemoteContext, RemoteKind, ShellSpec, WinSize, @@ -693,6 +694,12 @@ struct PaneState { /// record. See [`crate::core::machine::PaneRecord::osc_title`]. osc_title: Option, shell: ShellState, + /// The private modes the pane's output has switched on — the alternate + /// screen and mouse reporting above all. Folded from the same bytes the + /// ring gets, because the ring cannot be trusted to still hold them: a + /// full-screen tool sets them once at startup and the ring drops its front + /// (#774). See [`TerminalModes`]. + modes: TerminalModes, /// What this pane is running, for the machine tree to record. Distinct from /// `shell` above, which is the shell-integration state. shell_spec: Option, @@ -1461,6 +1468,7 @@ impl DaemonPane { cwd: spawn.initial_cwd, osc_title: restored_title, shell: ShellState::default(), + modes: TerminalModes::default(), shell_spec: spawn.shell.clone(), remote: spawn.remote.clone(), agent: None, @@ -1683,6 +1691,7 @@ impl DaemonPane { last_exit_code: carried.last_exit, command: None, }, + modes: TerminalModes::default(), remote: carried.remote, agent: carried.agent, agent_session: carried.agent_session, @@ -1735,6 +1744,7 @@ impl DaemonPane { cwd: None, osc_title: None, shell: ShellState::default(), + modes: TerminalModes::default(), remote: Some(remote), agent: None, agent_session: None, @@ -2035,7 +2045,7 @@ impl DaemonPane { || probed_cwd.is_some(); let mut st = state.lock().unwrap(); let facts_before = may_change_facts.then(|| observed_facts(&st)); - st.ring.append(bytes); + record_output(&mut st, bytes); fan_out_output(&mut st, bytes, frames, &gate); apply_signals(&mut st, signals); if let Some(remote) = remote { @@ -2575,7 +2585,26 @@ impl ReplayRing { } } +/// Everything the pane remembers about a chunk of output: the bytes +/// themselves, and the modes they switched on. One function so the two can +/// never drift apart — the modes are only worth anything if they were folded +/// from exactly the bytes the ring was given. +fn record_output(st: &mut PaneState, bytes: &[u8]) { + st.ring.append(bytes); + st.modes.feed(bytes); +} + fn replay_state(st: &PaneState, subscriber: &Sender) { + // Ahead of the ring, not after it: a client that is put into the alternate + // screen first paints the replayed frames into the buffer they belong to, + // and re-entering an alternate screen the ring turns out to still carry is + // a no-op in the emulator, so the two cannot fight. What the ring does + // carry always wins on its own terms — the fold is over every byte the + // pane ever wrote, so any mode the ring still toggles ends where the fold + // says it ends. + if let Some(modes) = st.modes.restore_bytes() { + let _ = subscriber.send(DaemonMsg::Snapshot(modes)); + } st.ring.replay(subscriber); if let Some(cwd) = &st.cwd { let _ = subscriber.send(DaemonMsg::Cwd(cwd.clone())); @@ -4376,6 +4405,7 @@ mod tests { cwd: None, osc_title: None, shell: ShellState::default(), + modes: TerminalModes::default(), remote: None, agent: None, agent_session: None, @@ -4770,6 +4800,64 @@ mod tests { ); } + /// Issue #774: `btop` sends its alternate-screen and mouse-reporting + /// prefix once, when it starts, and then refreshes for hours. The ring + /// holds the last few megabytes of those refreshes and nothing of the + /// prefix, so a client that rebuilt its terminal from replayed bytes alone + /// came back on the primary screen with reporting off — and its wheel, + /// reading those modes, scrolled the scrollback of a screen that has none. + #[test] + fn attach_restores_modes_whose_bytes_the_ring_has_dropped() { + let mut st = test_state(true); + record_output(&mut st, b"\x1b[?1049h\x1b[?1002h\x1b[?1006h"); + // A long enough run of refreshes to push the prefix out of the front. + record_output(&mut st, &vec![b'.'; RING_CAP]); + assert!( + !st.ring.flatten().windows(8).any(|w| w == b"\x1b[?1049h"), + "the point of the test is that the prefix is gone from the ring" + ); + + let (tx, rx) = mpsc::channel(); + attach_subscriber(&mut st, tx); + + // Ahead of the replayed screen, so the frames land in the buffer the + // modes put the client on. + assert!( + matches!(rx.try_recv(), Ok(DaemonMsg::Snapshot(b)) if b == b"\x1b[?1049h\x1b[?1002h\x1b[?1006h"), + "the modes the ring lost must be re-sent, in the order they were set" + ); + assert!(matches!(rx.try_recv(), Ok(DaemonMsg::Size(_)))); + assert!(matches!(rx.try_recv(), Ok(DaemonMsg::Snapshot(_)))); + } + + #[test] + fn a_pane_that_left_the_alternate_screen_restores_no_modes() { + let mut st = test_state(true); + record_output(&mut st, b"\x1b[?1049h\x1b[?1002hvim\x1b[?1002l\x1b[?1049l"); + record_output(&mut st, b"$ "); + + let (tx, rx) = mpsc::channel(); + attach_subscriber(&mut st, tx); + + // Straight to the ring: a shell prompt is not owed a mode frame, and + // sending one would put the pane on a screen it had left. + assert!(matches!(rx.try_recv(), Ok(DaemonMsg::Size(_)))); + assert!(matches!(rx.try_recv(), Ok(DaemonMsg::Snapshot(_)))); + assert!(rx.try_recv().is_err()); + } + + /// An observer joins mid-session too, and reads the same pane state. + #[test] + fn observers_are_told_the_pane_modes_as_well() { + let mut st = test_state(true); + record_output(&mut st, b"\x1b[?1049h"); + record_output(&mut st, &vec![b'.'; RING_CAP]); + + let (tx, rx) = mpsc::channel(); + observe_subscriber(&mut st, tx, Arc::new(OutputGate::new())); + assert!(matches!(rx.try_recv(), Ok(DaemonMsg::Snapshot(b)) if b == b"\x1b[?1049h")); + } + #[test] fn attach_to_a_dead_pane_replays_exited() { let mut st = test_state(false); diff --git a/src/terminal/remote.rs b/src/terminal/remote.rs index 01650704..6d5f8888 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -4064,6 +4064,58 @@ mod parked_cursor_tests { } } +/// Issue #774, from the client's end. A full-screen tool sets its modes once +/// and the replay ring drops them, so the daemon re-sends them from what it +/// folded out of the stream ([`tty7_core::core::term_modes`]). This is the +/// other half of that: the bytes it re-sends have to land the emulator back +/// where the application left it, because those are the modes `wheel_route` +/// reads before it decides the pane has a scrollback to move at all. +#[cfg(test)] +mod replayed_mode_tests { + use super::replay_tests::socket_pair; + use super::*; + use std::io::Write as _; + use tty7_core::core::term_modes::TerminalModes; + + #[test] + fn a_replayed_mode_frame_puts_the_client_back_on_the_alternate_screen() { + crate::core::config::pin_test_config_dir(); + // `btop`'s startup prefix, folded the way the daemon folds it out of + // the pty — and then re-sent from the fold, the ring having dropped + // the bytes themselves hours ago. + let mut modes = TerminalModes::new(); + modes.feed(b"\x1b[?1049h\x1b[?1002h\x1b[?1006h"); + + let (client_side, mut daemon_side) = socket_pair(); + let term = RemoteTerminal::from_stream(client_side, TermSize::new(80, 24)).unwrap(); + DaemonMsg::Snapshot(modes.restore_bytes().expect("a fold with modes in it")) + .encode(&mut daemon_side) + .unwrap(); + daemon_side.flush().unwrap(); + + for _ in 0..600 { + if term.term.lock().mode().contains(TermMode::ALT_SCREEN) { + break; + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + + let mode = *term.term.lock().mode(); + assert!( + mode.contains(TermMode::ALT_SCREEN), + "the pane belongs on the alternate screen it never left: {mode:?}" + ); + // Reporting first, SGR encoding with it: `wheel_route` sends the wheel + // to the application on the first and encodes the report with the + // second — see `wheel_routes_by_negotiated_mode_with_reporting_first`. + assert!( + mode.intersects(TermMode::MOUSE_MODE), + "mouse reporting is what keeps the wheel off the scrollback: {mode:?}" + ); + assert!(mode.contains(TermMode::SGR_MOUSE), "{mode:?}"); + } +} + #[cfg(all(test, unix))] mod tests { use super::*; From 97c1cb9b44710acd1640ec704cd37eb59daf40be Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:10:09 +0800 Subject: [PATCH 05/15] feat(cli): re-home an orphaned pane with `tab new --pane` (#716) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pane can come out from under its tab with its shell still running — an interrupted `tty7 run`, a `ws rm` that could not hang everything up, or a client that closed nineteen tabs whose shells were all alive (#716). `pane ls --all` has been able to *show* those for a while, but everything the CLI offered to do about one was to kill it: `pane close %`, or `pane close --orphans` for the lot. The shells were fine. There was simply no verb that put one back on screen, so recovering meant recreating tabs by hand and reaping the originals. `tab new` grows a `--pane` that builds the tab around a pane that is already running instead of spawning a shell for it. Nothing new had to be invented on the wire: `ControlRequest::TabCreate` has always taken a `PaneSeed` with a pane id in it, which is how `run --keep` files its pane into a tab. The part that needed designing is where the seed comes from. `tab_close` retains the orphaned panes out of `m.panes` at the same moment it drops the tab, so by the time anyone wants a pane back the tree has already forgotten its record — cwd, title, shell. Reading the seed off the tree would therefore work for an interrupted `run` and fail for exactly the case this verb exists for. It is rebuilt from the live pane registry instead, which still has the pane because the pane is still running, and which is the same list `pane ls --all` walks. That does mean `ssh_spec`, `agent` and `shell` are not recovered — the registry never carried them. They cost nothing while the shell lives, since the tab is a view onto a pty that is already there, and only matter if the pane later dies and something tries to restore it from the seed. Reconstructing them from a running pty is a different problem; a tab you can see beats a shell nobody can reach. Two refusals rather than one guess: a pane the server is not running cannot be re-homed, and neither can one a tab already holds — that is what `pane split` is for, and accepting it would put a single pane in two places in the tree. With no workspace named the pane goes back to the one it was spawned for, which is the `owner` that `pane ls --all` already prints; `$TTY7_WS` cannot help here, because a shell recovering from this is by definition not inside tty7. The `pane ls --all` footer now names the way back as well as the two ways to kill, since the listing is where an orphan is found and so is where the recovery has to be written down. Deliberately not done here: the switcher's orphan rows still carry only a Close button. Adopting from the GUI is not the same one-line change — the row lists orphans machine-wide while a window speaks for one workspace, and a pane may only be attached by the workspace that owns it, so the button has to decide where the tab goes before it can build one. That is its own piece of work. Claude-Session: https://claude.ai/code/session_01UUyWQXzcBAoBzaSX8pc7nU --- crates/tty7-cli/src/cli.rs | 23 +++- crates/tty7-cli/src/commands.rs | 226 +++++++++++++++++++++++++++++++- docs/cli/reference.mdx | 17 +++ 3 files changed, 259 insertions(+), 7 deletions(-) diff --git a/crates/tty7-cli/src/cli.rs b/crates/tty7-cli/src/cli.rs index bc216a6d..d266d890 100644 --- a/crates/tty7-cli/src/cli.rs +++ b/crates/tty7-cli/src/cli.rs @@ -407,7 +407,7 @@ pub enum TabCmd { ws: Option, }, - #[command(about = "Add a tab with a fresh shell")] + #[command(about = "Add a tab with a fresh shell, or around a pane already running")] New { #[arg(value_name = "WORKSPACE")] ws: Option, @@ -417,6 +417,20 @@ pub enum TabCmd { help = "Working directory for the tab's shell" )] cwd: Option, + + // The recovery half of `pane ls --all`. Until this existed, a pane that + // came out from under its tab — an interrupted `run`, or a client that + // closed tabs whose shells were still alive (#716) — could only be + // listed and killed. The shell is fine; it just has no tab, and + // `TabCreate` has always been able to take an existing pane id. + #[arg( + long, + value_name = "%PANE", + help = "Re-home a pane that is already running instead of spawning a shell — \ + for the orphans `tty7 pane ls --all` lists. Defaults the workspace to \ + the one the pane was spawned for" + )] + pane: Option, }, #[command(about = "Close a tab and every pane in it")] @@ -742,7 +756,12 @@ mod tests { )); assert!(matches!( parse(&["tty7", "tab", "new", "api", "--cwd", "C:\\proj"]).command, - Some(Command::Tab(TabCmd::New { ws: Some(w), cwd: Some(c) })) if w == "api" && c == "C:\\proj" + Some(Command::Tab(TabCmd::New { ws: Some(w), cwd: Some(c), pane: None })) + if w == "api" && c == "C:\\proj" + )); + assert!(matches!( + parse(&["tty7", "tab", "new", "--pane", "%37"]).command, + Some(Command::Tab(TabCmd::New { ws: None, cwd: None, pane: Some(p) })) if p == "%37" )); assert!(matches!( parse(&["tty7", "tab", "close", "@7"]).command, diff --git a/crates/tty7-cli/src/commands.rs b/crates/tty7-cli/src/commands.rs index c31f2c1d..10b88fc3 100644 --- a/crates/tty7-cli/src/commands.rs +++ b/crates/tty7-cli/src/commands.rs @@ -81,7 +81,9 @@ pub fn execute(cli: Cli, ctx: &Context, backend: &mut dyn Backend) -> Result capture(args, ctx, backend), Some(Command::Procs { target }) => procs(target.as_deref(), ctx, backend), Some(Command::Tab(TabCmd::Ls { ws })) => tab_ls(ws.as_deref(), ctx, backend), - Some(Command::Tab(TabCmd::New { ws, cwd })) => tab_new(ws.as_deref(), cwd, ctx, backend), + Some(Command::Tab(TabCmd::New { ws, cwd, pane })) => { + tab_new(ws.as_deref(), cwd, pane.as_deref(), ctx, backend) + } Some(Command::Tab(TabCmd::Close { tab })) => tab_close(&tab, backend), Some(Command::Tab(TabCmd::Rename { tab, name })) => tab_rename(&tab, name, backend), Some(Command::Tab(TabCmd::Move { tab, index })) => tab_move(&tab, index, backend), @@ -668,12 +670,19 @@ fn tab_ls(explicit: Option<&str>, ctx: &Context, backend: &mut dyn Backend) -> R fn tab_new( explicit: Option<&str>, cwd: Option, + adopt: Option<&str>, ctx: &Context, backend: &mut dyn Backend, ) -> Result { let machine = fetch_machine(backend)?; - let id = resolve_ws(explicit, ctx, &machine)?; - let pane = backend.spawn_shell(id, cwd.clone())?; + let (id, pane, cwd) = match adopt { + Some(spec) => adopt_pane(spec, explicit, cwd, ctx, &machine, backend)?, + None => { + let id = resolve_ws(explicit, ctx, &machine)?; + let pane = backend.spawn_shell(id, cwd.clone())?; + (id, pane, cwd) + } + }; let tab = match backend.control(ControlRequest::TabCreate { workspace: id, at: None, @@ -695,6 +704,83 @@ fn tab_new( ) } +/// Works out which workspace a pane that is already running goes into, and what +/// to seed the tab around it with. +/// +/// The seed is rebuilt from the **live pane registry**, not from the tree, and +/// that is the whole design of this verb. `tab_close` retains the panes it +/// orphaned out of `m.panes` at the same moment it drops the tab, so by the +/// time anyone wants a pane back the tree has forgotten its record — its cwd, +/// its title, the shell it was started with. The registry still has the pane, +/// because the pane is still running; it is the only place left that knows +/// anything about it. +/// +/// What the registry does not carry is `ssh_spec`, `agent` or `shell`, so a +/// re-homed pane is seeded without them. That costs nothing while the shell +/// lives — the tab is a view onto a pty that is already there — and only shows +/// up if the pane later dies and something tries to restore it from the seed. +/// Reconstructing those from a running pty is a separate problem; a tab you can +/// see and close beats a shell nobody can reach. +fn adopt_pane( + spec: &str, + explicit: Option<&str>, + cwd: Option, + ctx: &Context, + machine: &Machine, + backend: &mut dyn Backend, +) -> Result<(WorkspaceId, u64, Option)> { + let pane = address::parse_pane(spec)?; + let running = backend.list_panes()?; + let info = running + .iter() + .find(|info| info.pane_id == pane) + .ok_or_else(|| { + anyhow::anyhow!( + "no pane %{pane} is running on this machine — \ + `tty7 pane ls --all` lists every pane the server holds" + ) + })?; + if let Ok(holder) = resolve::workspace_of_pane(machine, pane) { + bail!( + "%{pane} is already in a tab of workspace {} — `tty7 pane split` adds \ + to that tab, and `tty7 tab new --pane` is for panes no tab holds", + resolve::short_id(&holder.id) + ); + } + // A pane the tree still knows nothing about, addressed with no workspace, + // goes back to the one it was spawned for: that is what `pane ls --all` + // prints as its owner, and the shell being recovered from is by definition + // not inside tty7, so `$TTY7_WS` is not going to answer here. + let id = match (explicit, ctx.ws.as_deref()) { + (None, None) => owner_of(info, machine).ok_or_else(|| { + anyhow::anyhow!( + "%{pane} does not name a workspace that still exists — \ + say which one to re-home it into: `tty7 tab new --pane %{pane}`" + ) + })?, + _ => resolve_ws(explicit, ctx, machine)?, + }; + // The recorded cwd is a courtesy for a later restore, not something the + // running shell is moved to; an explicit `--cwd` overrides it. + let cwd = cwd.or_else(|| { + info.cwd + .as_ref() + .map(|dir| dir.display().to_string()) + .filter(|dir| !dir.is_empty()) + }); + Ok((id, pane, cwd)) +} + +/// The workspace a pane was spawned for, if it is still on this machine. +fn owner_of(info: &tty7_core::daemon::protocol::PaneInfo, machine: &Machine) -> Option { + let owner = info.owner.as_deref()?; + machine + .workspaces + .iter() + .find(|ws| ws.id.to_string() == owner) + .map(|ws| ws.id) +} + fn tab_close(tab: &str, backend: &mut dyn Backend) -> Result { let addr = address::parse_tab(tab)?; let machine = fetch_machine(backend)?; @@ -818,8 +904,9 @@ fn pane_ls_all(backend: &mut dyn Backend) -> Result { let mut human = output::registry_table(&running, &|pane| holder(pane).map(|ws| ws.to_string())); if orphans > 0 { human.push_str(&format!( - "\n{orphans} pane(s) held by no workspace — `tty7 pane close %` stops one, \ - `tty7 pane close --orphans` stops all of them\n" + "\n{orphans} pane(s) held by no workspace — `tty7 tab new --pane %` puts one \ + back in a tab, `tty7 pane close %` stops one, `tty7 pane close --orphans` \ + stops all of them\n" )); } report(human, json!({ "panes": panes, "orphans": orphans })) @@ -2060,6 +2147,135 @@ mod tests { ); } + /// The recovery verb from #716. The pane is running and no tab holds it; + /// the tab is built around it and no new shell is started. + #[test] + fn tab_new_with_a_pane_re_homes_an_orphan_instead_of_spawning() { + let mut backend = mock(); + let api = backend.machine.workspaces[0].clone(); + let mut orphan = pane_info(37, Some(&api.id.to_string())); + orphan.cwd = Some("C:\\work".into()); + backend.registry = vec![orphan]; + backend + .replies + .push_back(ReplyOk::TabTree(Box::new(Tab::leaf(37)))); + + let out = run_cli( + &["tty7", "tab", "new", "--pane", "%37"], + &Context::default(), + &mut backend, + ); + + assert_eq!( + backend.control_calls[1], + ControlRequest::TabCreate { + workspace: api.id, + at: None, + pane: PaneSeed { + pane: 37, + // Rebuilt from the registry: the tree dropped this pane's + // record when the tab holding it closed. + cwd: Some("C:\\work".into()), + ssh_spec: None, + agent: None, + shell: None, + }, + tab: None, + }, + "with no workspace named, the pane goes back to the one it was spawned for" + ); + assert!( + backend.spawned.is_empty(), + "re-homing must not start a second shell — the point is the one still running" + ); + assert_eq!(human(out), "%37"); + } + + #[test] + fn tab_new_with_a_pane_takes_an_explicit_workspace_and_cwd() { + let mut backend = mock(); + let web = backend.machine.workspaces[1].id; + backend.registry = vec![pane_info(37, None)]; + backend + .replies + .push_back(ReplyOk::TabTree(Box::new(Tab::leaf(37)))); + + run_cli( + &["tty7", "tab", "new", "web", "--pane", "%37", "--cwd", "C:\\else"], + &Context::default(), + &mut backend, + ); + + assert_eq!( + backend.control_calls[1], + ControlRequest::TabCreate { + workspace: web, + at: None, + pane: PaneSeed { + pane: 37, + cwd: Some("C:\\else".into()), + ssh_spec: None, + agent: None, + shell: None, + }, + tab: None, + }, + "a pane with no owner still re-homes wherever it is told to" + ); + } + + #[test] + fn tab_new_refuses_a_pane_that_is_not_running() { + let mut backend = mock(); + let error = execute( + cli(&["tty7", "tab", "new", "api", "--pane", "%99"]), + &Context::default(), + &mut backend, + ) + .expect_err("a pane the server does not hold cannot be re-homed"); + assert!( + error.to_string().contains("no pane %99 is running"), + "{error:#}" + ); + assert!( + !backend + .control_calls + .iter() + .any(|call| matches!(call, ControlRequest::TabCreate { .. })), + "and nothing is written to the tree" + ); + } + + /// A pane a tab already holds is not an orphan, and putting it in a second + /// tab would leave the tree with one pane in two places. + #[test] + fn tab_new_refuses_a_pane_a_tab_already_holds() { + let mut backend = mock(); + backend.registry = vec![pane_info(2, None)]; + let error = execute( + cli(&["tty7", "tab", "new", "api", "--pane", "%2"]), + &Context::default(), + &mut backend, + ) + .expect_err("%2 is in a tab of api"); + assert!(error.to_string().contains("already in a tab"), "{error:#}"); + } + + /// The listing is where an orphan is found, so it is where the way out of + /// being one has to be written down. + #[test] + fn pane_ls_all_points_at_the_way_back_as_well_as_the_way_out() { + let mut backend = mock(); + backend.registry = vec![pane_info(37, None)]; + let out = human(run_cli( + &["tty7", "pane", "ls", "--all"], + &Context::default(), + &mut backend, + )); + assert!(out.contains("tty7 tab new --pane %"), "{out}"); + assert!(out.contains("tty7 pane close --orphans"), "{out}"); + } + #[test] fn pane_split_builds_the_split_and_spawns_the_new_shell() { let mut backend = mock(); diff --git a/docs/cli/reference.mdx b/docs/cli/reference.mdx index a7cf88b5..1270f24f 100644 --- a/docs/cli/reference.mdx +++ b/docs/cli/reference.mdx @@ -270,10 +270,22 @@ resolve it immediately before use. A full tab UUID also works: `@`. |---|---|---| | `tab ls [WORKSPACE]` | Tabs of a workspace | `{"workspace","tabs":[{"ordinal","id","name","label","agent","group","panes":[…]}]}` | | `tab new [WORKSPACE] [--cwd DIR]` | Add a tab with a fresh shell | `{"tab","pane"}` | +| `tab new [WORKSPACE] --pane %PANE` | Add a tab around a pane already running | `{"tab","pane"}` | | `tab close @TAB` | Close the tab and every pane in it | `{"closed"}` | | `tab rename @TAB NAME` | Name or rename | `{"tab","name"}` | | `tab move @TAB INDEX` | Reposition within its workspace | `{"tab","to"}` | +`--pane` re-homes instead of spawning: it builds the tab around a pane the +server is already running, which is how an orphan gets back on screen. Only a +pane no tab holds is accepted — use `pane split` to add to a tab that exists. +With no `WORKSPACE` and no `TTY7_WS`, the pane goes back to the workspace it was +spawned for, which is the `owner` that `pane ls --all` prints. The seed is +rebuilt from the pane registry, so the tab gets the pane's recorded cwd (unless +`--cwd` overrides it) but not the shell or SSH spec the pane started with: the +tree drops a pane's record when the tab holding it closes, and the registry is +what is left. That only matters if the shell later dies, since a restore would +then have nothing to restore from. + `GROUP` is the heading the GUI's sidebar files the tab under, shown by its last segment. Read-only from here: with the default repo grouping the GUI recomputes it from the tab's working directory. @@ -299,6 +311,11 @@ of the workspace that may attach to the pane (absent when none may), and `orphan: true` means no workspace holds it. An interrupted `run` leaves orphans here, as does a `ws rm` that reported panes it could not hang up. +An orphan is not necessarily rubbish — its shell may still be doing real work — +so there are two ways out of the list. `tab new --pane %` puts one back into +a tab, which is the recovery path when the panes came out from under a workspace +rather than out of an interrupted `run`. `pane close` is the other. + `--orphans` is the reaper for exactly those. It closes what `pane ls --all` lists as orphaned and nothing else — panes a workspace holds are untouched — and reports an empty list rather than an error when there is nothing to clean From 29312d5bb8d47cf46965a09b0310dc55c1d7db22 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:13:51 +0800 Subject: [PATCH 06/15] fix(procinfo): stop the listening-port probe failing silently (#731) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Ports section says nothing when a pane has no listeners, and it said exactly the same thing when the code that looks for listeners never ran. #731 is a report from inside that gap: a Go service started with `go run main.go` on macOS serves requests, and tty7 shows no port. The reported shape itself holds up. `snapshot()` walks the whole descendant tree from the pane's shell, `go run`'s compiled binary sits at depth 2, and the `lsof` invocation is the one that finds it. What did not hold up is everything around that. The walk stopped at 64 processes, and it is depth-first over children in ascending pid order. A shell whose earlier children brought a crowd — a build, a container runtime, an agent's worker pool — could spend the whole budget before the traversal reached the newest child, and the newest child, highest pid and visited last, is precisely the server someone started ten seconds ago. The walk now runs to a far larger bound and the probe is asked about all of it; only the list handed to the panel is cut back to 64 rows. Every way the probe can fail arrived as the same empty vector. `lsof` missing from the daemon's PATH read as "nothing is listening" — and the daemon's PATH is not the shell's, while macOS keeps `lsof` in /usr/sbin, the sort of entry a hand-written `export PATH=...` drops. So did a probe that hung: `Command::output` has no deadline and this runs on the thread answering `QueryProcs`, so one `lsof` wedged on a dead mount takes the whole pane's process list with it, permanently. The probe now falls back to the absolute paths, is bounded at three seconds, and says which of those happened. A server under `sudo` is visible as a process and invisible as a socket: `lsof` running as this user cannot read another user's fds. The walk now carries each process's effective uid, and a tree holding someone else's process says so rather than claiming the pane is quiet. `PaneProcs` grew a `probe` verdict, `serde(default)` so an older `tty7-server` at the far end of a remote workspace still parses and its silence still reads as a complete answer. The panel spends it on the one muted line where it used to write "None", and `tty7 procs` — the command the issue asks reporters to run — prints a note under the empty PORTS table. No banner and no new colour: an honest empty state, not a warning. Deliberately left alone: `lsof`'s exit status. It returns 1 for a pid it could not locate, and a pane's tree loses processes between the walk and the probe as a matter of course, so reading that as a broken probe would put a doubt on screen every time a command finished. A non-zero exit that also found nothing gets a debug log line and no more. The Windows path is untouched beyond its new return type — `GetExtendedTcpTable` has no tool to be missing and no subprocess to hang. I could not reproduce #731, and none of these is proven to be the reporter's bug. Each is a way the panel could be silently wrong, and the verdict is what will make the next report say which one. Claude-Session: https://claude.ai/code/session_01UUyWQXzcBAoBzaSX8pc7nU --- crates/tty7-cli/src/commands.rs | 2 + crates/tty7-cli/src/output.rs | 84 +++- crates/tty7-core/src/daemon/procinfo.rs | 522 +++++++++++++++++++++--- crates/tty7-core/src/daemon/protocol.rs | 65 +++ crates/tty7-core/src/host/server.rs | 1 + src/ui/i18n/en.rs | 4 + src/ui/i18n/ja.rs | 6 + src/ui/i18n/mod.rs | 2 + src/ui/i18n/zh.rs | 2 + src/ui/right_panel.rs | 41 +- 10 files changed, 661 insertions(+), 68 deletions(-) diff --git a/crates/tty7-cli/src/commands.rs b/crates/tty7-cli/src/commands.rs index c31f2c1d..931a5e67 100644 --- a/crates/tty7-cli/src/commands.rs +++ b/crates/tty7-cli/src/commands.rs @@ -2881,6 +2881,7 @@ mod tests { tty7_core::daemon::protocol::PaneProcs { procs: vec![proc_entry(100, "zsh", 0, true)], ports: Vec::new(), + probe: Default::default(), } } @@ -2892,6 +2893,7 @@ mod tests { proc_entry(101, "cargo", 1, true), ], ports: Vec::new(), + probe: Default::default(), } } diff --git a/crates/tty7-cli/src/output.rs b/crates/tty7-cli/src/output.rs index f6381c6e..5d3294c2 100644 --- a/crates/tty7-cli/src/output.rs +++ b/crates/tty7-cli/src/output.rs @@ -4,7 +4,7 @@ use tty7_core::core::machine::{Machine, PaneNode, Workspace}; use tty7_core::core::session::WorkspaceId; use tty7_core::core::tab_view::{TabLabel, TabView, strip_host_prefix, tab_views_of}; use tty7_core::daemon::control::{PaneAgentState, RouteInfo, ServerStatus}; -use tty7_core::daemon::protocol::{PaneInfo, PaneProcs}; +use tty7_core::daemon::protocol::{PaneInfo, PaneProcs, PortProbe}; use crate::resolve; @@ -236,9 +236,34 @@ fn render_node(out: &mut String, node: &PaneNode, machine: &Machine, depth: usiz } } +/// What the port probe has to say for itself, when it has something to say. +/// +/// This is the line that makes `tty7 procs` a diagnostic rather than another +/// place to read an empty PORTS table. An empty table means "nothing is +/// listening" only when the probe actually ran, and until it said so there was +/// no way — from a screenshot, from a bug report, from the JSON — to tell that +/// case from a probe that never got off the ground. +fn probe_note(probe: &PortProbe) -> Option { + match probe { + PortProbe::Ok => None, + PortProbe::Restricted => Some( + "note: some processes here belong to another user; a probe running as you \ + cannot see their sockets" + .to_string(), + ), + PortProbe::Unavailable(detail) => Some(format!( + "note: could not check for listening ports ({detail})" + )), + } +} + pub fn procs_tables(procs: &PaneProcs) -> String { + let note = probe_note(&procs.probe); if procs.procs.is_empty() && procs.ports.is_empty() { - return "nothing running in this pane\n".to_string(); + return match note { + Some(note) => format!("nothing running in this pane\n{note}\n"), + None => "nothing running in this pane\n".to_string(), + }; } let rows: Vec> = procs .procs @@ -261,6 +286,11 @@ pub fn procs_tables(procs: &PaneProcs) -> String { .collect(); out.push_str(&table(&["PORT", "PID", "NAME"], &rows)); } + if let Some(note) = note { + out.push('\n'); + out.push_str(¬e); + out.push('\n'); + } out } @@ -551,6 +581,7 @@ mod tests { addr: "*".into(), name: "node".into(), }], + probe: PortProbe::Ok, }; let rendered = procs_tables(&procs); assert!( @@ -562,5 +593,54 @@ mod tests { "the foreground process is marked: {rendered}" ); assert!(rendered.contains("3000"), "{rendered}"); + assert!( + !rendered.contains("note:"), + "a probe that worked says nothing: {rendered}" + ); + } + + /// #731's diagnostic. Someone whose ports are missing runs `tty7 procs` + /// and pastes what it says; an empty PORTS table is only worth pasting if + /// it distinguishes a quiet pane from a probe that never ran. + #[test] + fn a_probe_that_could_not_run_says_so_next_to_the_empty_table() { + let mut procs = PaneProcs { + procs: vec![ProcEntry { + pid: 100, + name: "zsh".into(), + depth: 0, + foreground: true, + }], + ports: Vec::new(), + probe: PortProbe::Unavailable("lsof: program not found".into()), + }; + let rendered = procs_tables(&procs); + assert!( + rendered.contains("could not check for listening ports"), + "{rendered}" + ); + assert!( + rendered.contains("lsof: program not found"), + "the reason is the whole point of the line: {rendered}" + ); + + procs.probe = PortProbe::Restricted; + assert!( + procs_tables(&procs).contains("another user"), + "{}", + procs_tables(&procs) + ); + + // And on a pane with nothing running at all, where the tables are not + // drawn, the note still has to come out. + let empty = PaneProcs { + probe: PortProbe::Unavailable("lsof: program not found".into()), + ..Default::default() + }; + assert!( + procs_tables(&empty).contains("could not check"), + "{}", + procs_tables(&empty) + ); } } diff --git a/crates/tty7-core/src/daemon/procinfo.rs b/crates/tty7-core/src/daemon/procinfo.rs index 30a8fe85..c937ec37 100644 --- a/crates/tty7-core/src/daemon/procinfo.rs +++ b/crates/tty7-core/src/daemon/procinfo.rs @@ -1,21 +1,95 @@ use std::collections::HashMap; -use crate::daemon::protocol::{PaneProcs, PortEntry, ProcEntry}; +use crate::daemon::protocol::{PaneProcs, PortEntry, PortProbe, ProcEntry}; const MAX_DEPTH: u8 = 6; +/// How many processes the panel is asked to draw. const MAX_PROCS: usize = 64; +/// How far the walk itself goes before it calls the tree pathological. +/// +/// This is deliberately far above `MAX_PROCS`, and the gap is the point. The +/// walk is depth-first over children sorted by ascending pid, so capping it at +/// the number of rows the panel wants meant one busy early branch — a build, +/// a container runtime, an agent's worker pool — could consume the whole +/// budget before the traversal ever reached the pane's newest child. A server +/// someone just started is the *last* pid in that ordering, so the one process +/// the Ports section exists for was the one most likely to fall off the end, +/// and it fell off silently. The port probe is asked about everything the walk +/// found; only the list handed to the panel is cut back to `MAX_PROCS`. +const MAX_TREE: usize = 512; + pub fn snapshot(shell_pid: u32, fg_pgid: Option) -> PaneProcs { let table = process_table(); let procs = walk(&table, shell_pid, fg_pgid); - let ports = listening_ports(&procs); - PaneProcs { procs, ports } + let (ports, probe) = listening_ports(&procs); + let probe = match probe.is_ok() && tree_has_foreign_uid(&table, &procs, current_uid()) { + true => PortProbe::Restricted, + false => probe, + }; + if let PortProbe::Unavailable(detail) = &probe { + log::warn!("listening-port probe failed for pane shell {shell_pid}: {detail}"); + } + finish(procs, ports, probe) +} + +/// The answer as the panel gets it: the process list trimmed to what a sidebar +/// can show, and the ports left whole. +/// +/// Trimming here rather than in `walk` is what keeps a port owned by the 100th +/// process in the tree on screen — the row names its owner out of the full +/// list, so cutting the list afterwards costs the panel a process row it had +/// no room for and costs the Ports section nothing. +fn finish(mut procs: Vec, ports: Vec, probe: PortProbe) -> PaneProcs { + procs.truncate(MAX_PROCS); + PaneProcs { + procs, + ports, + probe, + } +} + +/// Whether any process in the pane's tree belongs to a user other than `me`. +/// +/// `sudo go run main.go` is the shape this is about. The process tree still +/// walks — the kernel will name another user's processes — but the sockets +/// they hold are readable only by their owner or by root, so `lsof` running as +/// this user answers "nothing is listening" about a server that plainly is. +/// Saying "some of these are another user's" is the difference between a panel +/// that is wrong and a panel that is honest. +fn tree_has_foreign_uid(table: &HashMap, procs: &[ProcEntry], me: u32) -> bool { + // Root sees everyone's sockets, so nothing is hidden from a daemon that is + // already root and there is nothing to warn about. + if me == 0 { + return false; + } + procs + .iter() + .any(|p| table.get(&p.pid).is_some_and(|row| row.uid != me)) +} + +#[cfg(unix)] +fn current_uid() -> u32 { + // SAFETY: `getuid` reads the calling process's own credentials and cannot + // fail. + unsafe { libc::getuid() } +} + +/// Windows has no uid, and its port probe is a kernel table rather than a +/// subprocess with an identity — `Row::uid` is 0 there and so is this, so the +/// check above is a constant false. +#[cfg(not(unix))] +fn current_uid() -> u32 { + 0 } struct Row { ppid: u32, pgid: u32, + /// The effective uid of the process, which is what decides whether this + /// daemon may look at its sockets. 0 on platforms that have no such thing. + uid: u32, name: String, } @@ -32,7 +106,7 @@ fn walk(table: &HashMap, shell_pid: u32, fg_pgid: Option) -> Vec< let mut stack = vec![(shell_pid, 0u8)]; while let Some((pid, depth)) = stack.pop() { let Some(row) = table.get(&pid) else { continue }; - if out.len() >= MAX_PROCS { + if out.len() >= MAX_TREE { break; } out.push(ProcEntry { @@ -96,6 +170,10 @@ fn process_table() -> HashMap { Row { ppid: info.pbi_ppid, pgid: info.pbi_pgid, + // The effective uid, not the real one: a setuid `sudo` still + // runs as the user who typed it, and it is the effective uid + // that decides whose sockets `lsof` may read. + uid: info.pbi_uid, name, }, ); @@ -146,7 +224,23 @@ fn process_table() -> HashMap { .rfind('(') .map_or_else(|| String::new(), |open| stat[open + 1..close].to_string()) }); - table.insert(pid, Row { ppid, pgid, name }); + // `/proc/` is owned by the process's effective uid, which is the + // one that governs who may read its sockets. A stat that fails on a + // pid whose `stat` file just parsed is a race with the process + // exiting; calling that "mine" keeps a dying process from being + // mistaken for another user's. + let uid = std::fs::metadata(format!("/proc/{pid}")) + .map(|m| std::os::unix::fs::MetadataExt::uid(&m)) + .unwrap_or_else(|_| current_uid()); + table.insert( + pid, + Row { + ppid, + pgid, + uid, + name, + }, + ); } table } @@ -161,6 +255,7 @@ fn process_table() -> HashMap { Row { ppid: p.parent, pgid: 0, + uid: 0, name: p.name, }, ) @@ -221,20 +316,42 @@ pub(super) fn proc_name(pid: i32) -> Option { (!comm.is_empty()).then(|| comm.to_string()) } +/// Where to look for `lsof`, in order. +/// +/// `PATH` first, and then the two absolute paths it actually lives at, because +/// the daemon's `PATH` is not the shell's. macOS ships `lsof` in `/usr/sbin`, +/// which is on the default login `PATH` and is exactly the kind of entry a +/// hand-written `export PATH=...` in a dotfile drops on the floor — and the +/// daemon inherits whatever the app that launched it had. Falling back to the +/// absolute path costs one failed `execvp` in the case that used to end with +/// the panel quietly claiming nothing was listening. #[cfg(unix)] -fn listening_ports(procs: &[ProcEntry]) -> Vec { - use std::process::{Command, Stdio}; +const LSOF_CANDIDATES: [&str; 3] = ["lsof", "/usr/sbin/lsof", "/usr/bin/lsof"]; +/// How long the probe gets before it is declared hung. +/// +/// `lsof` is famous for blocking on a wedged network mount, and this one runs +/// on the daemon's connection thread: without a bound, one stuck call does not +/// merely lose a port, it stops the pane answering `QueryProcs` at all, for as +/// long as the mount stays wedged. The Info panel re-polls every two seconds, +/// so a probe still running after three has already missed its slot. +#[cfg(unix)] +const PROBE_BUDGET: std::time::Duration = std::time::Duration::from_secs(3); + +#[cfg(unix)] +fn listening_ports(procs: &[ProcEntry]) -> (Vec, PortProbe) { if procs.is_empty() { - return Vec::new(); + return (Vec::new(), PortProbe::Ok); } let pid_list = procs .iter() .map(|p| p.pid.to_string()) .collect::>() .join(","); - let out = Command::new("lsof") - .args([ + let mut first_err = String::new(); + for tool in LSOF_CANDIDATES { + let mut cmd = std::process::Command::new(tool); + cmd.args([ "-nP", "-iTCP", "-sTCP:LISTEN", @@ -242,13 +359,113 @@ fn listening_ports(procs: &[ProcEntry]) -> Vec { "-p", &pid_list, "-Fpn", - ]) - .stdin(Stdio::null()) - .stderr(Stdio::null()) - .output(); - let Ok(out) = out else { return Vec::new() }; - let text = String::from_utf8_lossy(&out.stdout); + ]); + match run_bounded(cmd, PROBE_BUDGET) { + Ok(Run::Finished(out)) => { + let ports = parse_lsof(&String::from_utf8_lossy(&out.stdout), procs); + // The exit status is deliberately not read as failure. `lsof` + // returns 1 for a pid it could not locate, and a pane's tree + // grows and loses processes between the walk and this call as + // a matter of course — treating that as a broken probe would + // put a doubt on screen every time a `ls` finished. What the + // status is worth is a log line when the run both complained + // and came back with nothing. + if ports.is_empty() && !out.status.success() { + if let Some(line) = String::from_utf8_lossy(&out.stderr) + .lines() + .find(|l| !l.trim().is_empty()) + { + log::debug!("{tool} found no listeners and said: {line}"); + } + } + return (ports, PortProbe::Ok); + } + Ok(Run::TimedOut) => { + return ( + Vec::new(), + PortProbe::Unavailable(format!( + "{tool} did not answer within {}s", + PROBE_BUDGET.as_secs() + )), + ); + } + // Try the next candidate: this one is not there, or is not + // runnable. The complaint kept is the first one, about the name as + // the daemon's `PATH` sees it, since that is the failure worth + // reading — the others are fallbacks nobody asked for. + Err(e) => { + if first_err.is_empty() { + first_err = format!("{tool}: {e}"); + } + } + } + } + ( + Vec::new(), + PortProbe::Unavailable(format!( + "{first_err} (also tried {})", + LSOF_CANDIDATES[1..].join(", ") + )), + ) +} +/// What became of a probe process. +/// +/// Compiled everywhere and used by the unix probe and by the tests, which is +/// how a parser and a timeout that only ever run on macOS and Linux get +/// exercised on a Windows machine. +#[cfg_attr(not(unix), allow(dead_code))] +enum Run { + Finished(std::process::Output), + TimedOut, +} + +/// Run `cmd` to completion, or kill it once `budget` is up. +/// +/// `Command::output` has no deadline, and the caller is a daemon thread that a +/// hung child would own forever. Both pipes are read after the wait rather +/// than while it runs, which is safe for a probe whose whole output is a few +/// hundred bytes and which is killed if it ever stops making progress. +/// +/// A killed run is reaped and its output abandoned unread. Reading it would +/// reintroduce the hang this exists to prevent: a child that spawned something +/// of its own hands the write end of the pipe on, and waiting for end-of-file +/// then means waiting for a grandchild nobody killed. +#[cfg_attr(not(unix), allow(dead_code))] +fn run_bounded( + mut cmd: std::process::Command, + budget: std::time::Duration, +) -> std::io::Result { + use std::process::Stdio; + + let mut child = cmd + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn()?; + let deadline = std::time::Instant::now() + budget; + loop { + if child.try_wait()?.is_some() { + return Ok(Run::Finished(child.wait_with_output()?)); + } + if std::time::Instant::now() >= deadline { + let _ = child.kill(); + // Reaps the process this call started, so a timed-out probe leaves + // no zombie behind; the pipes close as `child` drops. + let _ = child.wait(); + return Ok(Run::TimedOut); + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } +} + +/// The listeners in one `lsof -Fpn` report, named after the processes that own +/// them. +/// +/// Split out from the call so the format can be tested off a Mac: this parser +/// is the half of the probe that has no platform in it. +#[cfg_attr(not(unix), allow(dead_code))] +fn parse_lsof(text: &str, procs: &[ProcEntry]) -> Vec { let by_pid: HashMap = procs.iter().map(|p| (p.pid, p.name.as_str())).collect(); let mut ports: Vec = Vec::new(); let mut current = 0u32; @@ -262,32 +479,13 @@ fn listening_ports(procs: &[ProcEntry]) -> Vec { let Some((addr, port)) = parse_listen_addr(rest) else { continue; }; - // One process listening on the same port over IPv4 and IPv6 is - // one port to show. Which of the two lines survives used to be - // whichever lsof printed first; now that the address is carried - // through to a clickable URL, the reachable one wins — a - // process bound to both `192.168.1.5` and `*` is on localhost, - // and the row should say so. - if let Some(seen) = ports - .iter_mut() - .find(|e| e.port == port && e.pid == current) - { - if !PortEntry::reaches_loopback(&seen.addr) && PortEntry::reaches_loopback(addr) - { - seen.addr = addr.to_string(); - } - continue; - } - ports.push(PortEntry { + record_listener( + &mut ports, + by_pid.get(¤t).copied().unwrap_or_default(), port, - pid: current, - addr: addr.to_string(), - name: by_pid - .get(¤t) - .copied() - .unwrap_or_default() - .to_string(), - }); + current, + addr.to_string(), + ); } _ => {} } @@ -311,8 +509,12 @@ fn listening_ports(procs: &[ProcEntry]) -> Vec { /// second call when its table outgrew that. The Info tab re-polls every two /// seconds while it is open, so this is a fixed handful of microseconds, with /// no process spawn and nothing allocated per pid. +/// +/// The probe state is always `Ok` here. There is no tool to be missing and no +/// subprocess to hang: `GetExtendedTcpTable` either answers or the family is +/// skipped, and a machine with IPv6 off still gets its IPv4 ports. #[cfg(windows)] -fn listening_ports(procs: &[ProcEntry]) -> Vec { +fn listening_ports(procs: &[ProcEntry]) -> (Vec, PortProbe) { use std::net::{Ipv4Addr, Ipv6Addr}; use windows_sys::Win32::NetworkManagement::IpHelper::{ @@ -321,7 +523,7 @@ fn listening_ports(procs: &[ProcEntry]) -> Vec { }; if procs.is_empty() { - return Vec::new(); + return (Vec::new(), PortProbe::Ok); } let by_pid: HashMap = procs.iter().map(|p| (p.pid, p.name.as_str())).collect(); let mut ports: Vec = Vec::new(); @@ -378,7 +580,7 @@ fn listening_ports(procs: &[ProcEntry]) -> Vec { } ports.sort_by_key(|e| (e.port, e.pid)); - ports + (ports, PortProbe::Ok) } /// The two Winsock address families, named here rather than by switching on @@ -494,11 +696,11 @@ fn spell_v6(addr: std::net::Ipv6Addr) -> String { /// Adds one listening socket to the list, merging it with a row already there /// for the same port and pid. /// -/// The merge rule is the unix path's, for the same reason: a process bound to -/// both `192.168.1.5` and `*` is on localhost, and the row the panel turns into -/// a clickable URL should say so rather than whichever address the kernel -/// happened to list first. -#[cfg(windows)] +/// One rule for both platforms — it was written twice, once inline in the +/// `lsof` parser and once here, and two copies of a merge rule is one too +/// many. A process bound to both `192.168.1.5` and `*` is on localhost, and +/// the row the panel turns into a clickable URL should say so rather than +/// keeping whichever address the kernel or `lsof` happened to list first. fn record_listener(ports: &mut Vec, name: &str, port: u16, pid: u32, addr: String) { if let Some(seen) = ports.iter_mut().find(|e| e.port == port && e.pid == pid) { if !PortEntry::reaches_loopback(&seen.addr) && PortEntry::reaches_loopback(&addr) { @@ -515,8 +717,11 @@ fn record_listener(ports: &mut Vec, name: &str, port: u16, pid: u32, } #[cfg(not(any(unix, windows)))] -fn listening_ports(_procs: &[ProcEntry]) -> Vec { - Vec::new() +fn listening_ports(_procs: &[ProcEntry]) -> (Vec, PortProbe) { + ( + Vec::new(), + PortProbe::Unavailable("no port probe on this platform".to_string()), + ) } /// The address and port `lsof -Fn` reports a listener on — `*:3000`, @@ -538,10 +743,14 @@ fn parse_listen_addr(name: &str) -> Option<(&str, u16)> { mod tests { use super::*; + /// The uid every fabricated row belongs to unless a test says otherwise. + const ME: u32 = 501; + fn row(ppid: u32, name: &str) -> Row { Row { ppid, pgid: 0, + uid: ME, name: name.to_string(), } } @@ -589,7 +798,216 @@ mod tests { .into_iter() .collect(); let got = walk(&table, 100, None); - assert!(got.len() <= MAX_PROCS, "bounded, not infinite"); + assert!(got.len() <= MAX_TREE, "bounded, not infinite"); + } + + /// #731. The walk is depth-first over children in ascending pid order, so + /// a shell whose earlier children brought a crowd used to exhaust the row + /// budget before the traversal reached the newest child — and the newest + /// child, highest pid and visited last, is precisely the `go run` someone + /// started ten seconds ago and is looking for the port of. + #[test] + fn the_newest_child_survives_a_shell_crowded_with_older_ones() { + let mut table: HashMap = [(100, row(1, "zsh"))].into_iter().collect(); + // 80 older children, each with a child of its own: 160 processes, well + // past the 64 the panel draws. + for i in 0..80u32 { + table.insert(200 + i * 2, row(100, "node")); + table.insert(201 + i * 2, row(200 + i * 2, "esbuild")); + } + table.insert(9000, row(100, "go")); + table.insert(9001, row(9000, "main")); + + let walked = walk(&table, 100, None); + assert!( + walked.iter().any(|p| p.pid == 9001 && p.name == "main"), + "the process holding the listener must reach the probe, got {} rows", + walked.len() + ); + + let ports = vec![PortEntry { + port: 8080, + pid: 9001, + addr: "*".into(), + name: "main".into(), + }]; + let out = finish(walked, ports, PortProbe::Ok); + assert_eq!( + out.procs.len(), + MAX_PROCS, + "the panel still gets a short list" + ); + assert_eq!( + out.ports.first().map(|p| p.port), + Some(8080), + "and the port survives the trim that dropped its owner's row" + ); + } + + #[test] + fn a_pathological_tree_still_stops() { + let mut table: HashMap = [(100, row(1, "zsh"))].into_iter().collect(); + for pid in 200..2000u32 { + table.insert(pid, row(100, "fork-bomb")); + } + assert_eq!(walk(&table, 100, None).len(), MAX_TREE); + } + + /// A `sudo go run` is a server the panel can see and a socket it cannot. + /// Saying nothing is listening is the one answer that is certainly wrong. + #[test] + fn another_users_process_in_the_tree_is_noticed() { + let mut table: HashMap = [ + (100, row(1, "zsh")), + (200, row(100, "sudo")), + (300, row(200, "main")), + ] + .into_iter() + .collect(); + let procs = walk(&table, 100, None); + assert!( + !tree_has_foreign_uid(&table, &procs, ME), + "everything is mine until sudo takes over" + ); + + table.get_mut(&200).unwrap().uid = 0; + table.get_mut(&300).unwrap().uid = 0; + assert!(tree_has_foreign_uid(&table, &procs, ME)); + assert!( + !tree_has_foreign_uid(&table, &procs, 0), + "a daemon already running as root is shown everyone's sockets" + ); + } + + /// The `go run` shape from #731, as `lsof -Fpn` reports it: the shell is in + /// the pid list and holds nothing, the compiled binary under `$TMPDIR` + /// holds the listener, and it is bound over both address families. + #[test] + fn parses_a_go_run_report() { + let procs = vec![ + ProcEntry { + pid: 100, + name: "zsh".into(), + depth: 0, + foreground: false, + }, + ProcEntry { + pid: 9000, + name: "go".into(), + depth: 1, + foreground: true, + }, + ProcEntry { + pid: 9001, + name: "main".into(), + depth: 2, + foreground: true, + }, + ]; + let report = "p9001\nf3\nn*:8080\nf5\nn[::]:8080\np100\n"; + let ports = parse_lsof(report, &procs); + assert_eq!( + ports.len(), + 1, + "one server, not one row per family: {ports:?}" + ); + assert_eq!(ports[0].port, 8080); + assert_eq!(ports[0].pid, 9001); + assert_eq!( + ports[0].name, "main", + "the row names the binary `go run` built, not `go`" + ); + assert_eq!(ports[0].authority(), "localhost:8080"); + } + + #[test] + fn a_report_about_nobody_we_asked_about_still_parses() { + // A pid that vanished between the walk and the probe leaves a row with + // no name rather than dropping the port someone can still click. + let ports = parse_lsof("p4242\nn127.0.0.1:5173\n", &[]); + assert_eq!(ports.len(), 1); + assert_eq!(ports[0].name, ""); + } + + #[test] + fn a_quick_probe_comes_back_whole() { + let got = run_bounded(echo_command(), std::time::Duration::from_secs(30)); + let out = match got { + Ok(Run::Finished(ref out)) => out, + ref other => panic!("expected a finished run, got {}", describe(other)), + }; + assert!(String::from_utf8_lossy(&out.stdout).contains("tty7")); + } + + /// The bound is the whole point: this probe runs on the daemon thread that + /// answers `QueryProcs`, and an `lsof` wedged on a dead mount used to own + /// it forever. + /// + /// The sleeper is deliberately a wrapper around a second process, which is + /// the shape that makes this hard: killing the child does not close the + /// pipe its own child inherited, so a `wait_with_output` after the kill + /// would sit on that pipe for the full sleep and hand the caller a timeout + /// that took as long as no timeout at all. + #[test] + fn a_probe_that_never_finishes_is_killed_and_named() { + let cmd = sleeper_command(); + let started = std::time::Instant::now(); + let got = run_bounded(cmd, std::time::Duration::from_millis(300)); + assert!( + matches!(got, Ok(Run::TimedOut)), + "expected a timeout, got {}", + describe(&got) + ); + assert!( + started.elapsed() < std::time::Duration::from_secs(10), + "the caller was released long before the child would have exited" + ); + } + + #[test] + fn a_missing_probe_is_an_error_and_not_an_empty_answer() { + let cmd = std::process::Command::new("tty7-no-such-probe-b0rk"); + assert!( + run_bounded(cmd, std::time::Duration::from_secs(5)).is_err(), + "a tool that is not there has to be distinguishable from one that found nothing" + ); + } + + fn describe(run: &std::io::Result) -> String { + match run { + Ok(Run::Finished(out)) => format!("finished with {}", out.status), + Ok(Run::TimedOut) => "a timeout".to_string(), + Err(e) => format!("an error: {e}"), + } + } + + #[cfg(windows)] + fn echo_command() -> std::process::Command { + let mut c = std::process::Command::new("cmd"); + c.args(["/c", "echo", "tty7"]); + c + } + + #[cfg(not(windows))] + fn echo_command() -> std::process::Command { + let mut c = std::process::Command::new("echo"); + c.arg("tty7"); + c + } + + #[cfg(windows)] + fn sleeper_command() -> std::process::Command { + // `ping` is the sleep every Windows image has. + let mut c = std::process::Command::new("cmd"); + c.args(["/c", "ping", "-n", "20", "127.0.0.1"]); + c + } + + #[cfg(not(windows))] + fn sleeper_command() -> std::process::Command { + let mut c = std::process::Command::new("sleep"); + c.arg("60"); + c } #[test] diff --git a/crates/tty7-core/src/daemon/protocol.rs b/crates/tty7-core/src/daemon/protocol.rs index 0eaa807e..e29d7d4c 100644 --- a/crates/tty7-core/src/daemon/protocol.rs +++ b/crates/tty7-core/src/daemon/protocol.rs @@ -464,10 +464,46 @@ impl PortEntry { } } +/// Whether the answer in `PaneProcs::ports` can be believed. +/// +/// An empty port list used to mean two very different things at once: nothing +/// in this pane is listening, or the thing that looks for listeners never got +/// to say. On unix that look is an `lsof` subprocess, and every way it can go +/// wrong — absent from the daemon's `PATH`, killed, hung on a wedged mount, +/// pointed at sockets it has no permission to read — arrived as the same empty +/// vector as a genuinely quiet pane. Whoever reads the list gets to know which +/// one it is. +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "state", content = "detail", rename_all = "snake_case")] +pub enum PortProbe { + /// The probe ran and the list is its whole answer. + #[default] + Ok, + /// The probe ran, but at least one process in this pane belongs to another + /// user — `sudo go run`, a root-owned server on a low port — and a probe + /// running as this user cannot see that process's sockets. The list holds + /// what could be seen, which may be nothing. + Restricted, + /// The probe could not be run at all. The string is for a log line or for + /// `tty7 procs`, not for the panel: it names the tool and what went wrong. + Unavailable(String), +} + +impl PortProbe { + pub fn is_ok(&self) -> bool { + matches!(self, PortProbe::Ok) + } +} + #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct PaneProcs { pub procs: Vec, pub ports: Vec, + /// `serde(default)` because a daemon from before this field existed + /// answers `QueryProcs` without it, and its silence is read the way that + /// daemon's callers read every answer it gives: as a complete one. + #[serde(default)] + pub probe: PortProbe, } fn default_term() -> String { @@ -2192,6 +2228,35 @@ mod tests { assert!(ClientMsg::read(&mut empty2).is_err()); } + /// The port probe's verdict travels over the same wire as the ports, and + /// the two ends of that wire are regularly different builds: a remote + /// workspace's panes are answered for by whatever `tty7-server` is + /// installed on the peer. An older one says nothing about the probe, and + /// its silence has to read as "this list is the whole answer" rather than + /// failing the frame or, worse, arriving as a doubt the panel then shows. + #[test] + fn a_procs_answer_without_a_probe_verdict_is_a_complete_one() { + let old = r#"{"procs":[],"ports":[{"port":3000,"pid":7,"name":"node"}]}"#; + let procs: PaneProcs = serde_json::from_str(old).unwrap(); + assert_eq!(procs.probe, PortProbe::Ok); + assert!(procs.probe.is_ok()); + assert_eq!(procs.ports[0].addr, ""); + + for probe in [ + PortProbe::Ok, + PortProbe::Restricted, + PortProbe::Unavailable("lsof: not found".into()), + ] { + let wire = serde_json::to_string(&PaneProcs { + probe: probe.clone(), + ..Default::default() + }) + .unwrap(); + let back: PaneProcs = serde_json::from_str(&wire).unwrap(); + assert_eq!(back.probe, probe, "round trip through {wire}"); + } + } + #[test] fn pane_info_deserializes_with_defaults() { let info: PaneInfo = serde_json::from_str(r#"{"pane_id": 5, "alive": true}"#).unwrap(); diff --git a/crates/tty7-core/src/host/server.rs b/crates/tty7-core/src/host/server.rs index 1f2fa862..9db770c8 100644 --- a/crates/tty7-core/src/host/server.rs +++ b/crates/tty7-core/src/host/server.rs @@ -1711,6 +1711,7 @@ mod aggregate_tests { name: "node".into(), addr: "*".into(), }], + probe: Default::default(), } } diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 42b04b3d..6d5aafca 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -1073,6 +1073,10 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::PanelProcessesSubtitle => "Processes", L10nKey::PanelPortsSubtitle => "Ports", L10nKey::PanelPortsUnsupported => "That machine's tty7-server is too old to list ports.", + L10nKey::PanelPortsProbeFailed => "Couldn't check what this pane is listening on.", + L10nKey::PanelPortsRestricted => { + "Something here runs as another user, whose ports aren't visible." + } L10nKey::PortAutoForwarded => "Remote :{port} is now http://localhost:{local}", L10nKey::PanelCwd => "cwd", L10nKey::PanelShell => "shell", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 6560bf5a..f1c1c41a 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -1137,6 +1137,12 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::PanelProcessesSubtitle => "プロセス", L10nKey::PanelPortsSubtitle => "ポート", L10nKey::PanelPortsUnsupported => "リモートの tty7-server が古く、ポートを列挙できません。", + L10nKey::PanelPortsProbeFailed => { + "このペインが何をリッスンしているか確認できませんでした。" + } + L10nKey::PanelPortsRestricted => { + "他のユーザーで動いているプロセスがあり、そのポートは見えません。" + } L10nKey::PortAutoForwarded => "リモートの :{port} は http://localhost:{local} で開けます", L10nKey::PanelCwd => "作業ディレクトリ", L10nKey::PanelShell => "シェル", diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 0cc6d32d..3d51402f 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -789,6 +789,8 @@ l10n_keys! { PanelProcessesSubtitle, PanelPortsSubtitle, PanelPortsUnsupported, + PanelPortsProbeFailed, + PanelPortsRestricted, PortAutoForwarded, PanelCwd, PanelShell, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index 83d55ae5..eee42a1e 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -1024,6 +1024,8 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::PanelProcessesSubtitle => "进程", L10nKey::PanelPortsSubtitle => "端口", L10nKey::PanelPortsUnsupported => "对端的 tty7-server 太旧,列不出端口。", + L10nKey::PanelPortsProbeFailed => "没能查出这个窗格在监听什么。", + L10nKey::PanelPortsRestricted => "这里有以其他用户身份运行的进程,看不到它们的端口。", L10nKey::PortAutoForwarded => "远程 :{port} 现在是 http://localhost:{local}", L10nKey::PanelCwd => "工作目录", L10nKey::PanelShell => "shell", diff --git a/src/ui/right_panel.rs b/src/ui/right_panel.rs index da037237..15689140 100644 --- a/src/ui/right_panel.rs +++ b/src/ui/right_panel.rs @@ -8,7 +8,7 @@ use gpui_component::{ use std::path::PathBuf; use crate::core::config::{Config, RightPanelTab}; -use crate::daemon::protocol::{ManagedForward, PaneProcs}; +use crate::daemon::protocol::{ManagedForward, PaneProcs, PortProbe}; use crate::ui::app::{ CONTENT_INSET, TILE_GLYPH_SM, TILE_GLYPH_XS, TILE_SIZE_SM, TILE_SIZE_XS, Tty7App, tile_trailing_inset, tile_trailing_inset_sm, @@ -1248,9 +1248,12 @@ impl Tty7App { ) -> Option { let ctx = ctx?; let pane_id = ctx.pane_id; - let ports = self + // No answer yet for this pane defaults to a probe that is fine, not a + // broken one: the panel has nothing to doubt until it has been told + // something. + let (ports, probe) = self .procs(Some(pane_id)) - .map(|p| p.ports.clone()) + .map(|p| (p.ports.clone(), p.probe.clone())) .unwrap_or_default(); let forwards: Vec = self .loopback_panel @@ -1262,7 +1265,13 @@ impl Tty7App { let form_open = self.loopback_panel.form_pane_id == Some(pane_id); // A pane that cannot hold a forward and is serving nothing has no // section: the heading alone would be an empty promise. - if ports.is_empty() && forwards.is_empty() && ctx.route.is_none() { + // + // Unless the reason it is serving nothing is that nobody managed to + // look. Then the heading and one muted line under it are the only + // place the panel can admit it does not know, and a silently absent + // section is the bug (#731): someone whose server is plainly up reads + // the missing section as tty7 saying there is no server. + if ports.is_empty() && forwards.is_empty() && ctx.route.is_none() && probe.is_ok() { return None; } @@ -1423,22 +1432,26 @@ impl Tty7App { |this| { // "Nothing is listening" and "nobody could tell us" // look identical on screen unless the panel says which - // one it means — and the second is a fixable thing: - // the far end is running a server too old to answer. - let (text, tone) = match self.right_panel.procs_unsupported { - true => ( - t(L10nKey::PanelPortsUnsupported), - cx.theme().muted_foreground, - ), - false => (t(L10nKey::None), cx.theme().muted_foreground), + // one it means — and every one of the second kind is a + // fixable thing: a far end running a server too old to + // answer, a probe that could not be run at all, a + // server started under `sudo` whose sockets this user + // is not allowed to see. Still one muted line in the + // place the word "None" would have gone; the panel is + // reporting what it knows, not raising an alarm. + let key = match (self.right_panel.procs_unsupported, &probe) { + (true, _) => L10nKey::PanelPortsUnsupported, + (false, PortProbe::Unavailable(_)) => L10nKey::PanelPortsProbeFailed, + (false, PortProbe::Restricted) => L10nKey::PanelPortsRestricted, + (false, PortProbe::Ok) => L10nKey::None, }; this.child( div() .px(px(CONTENT_INSET)) .py(px(2.)) .text_size(rems(TEXT)) - .text_color(tone) - .child(text), + .text_color(cx.theme().muted_foreground) + .child(t(key)), ) }, ) From 425bf32e01aefd7924deddfe12ee7915e4709730 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:17:34 +0800 Subject: [PATCH 07/15] fix(tree-sync): spend a typed workspace name only on the create it rode with (#716) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A name typed into the create form cannot be sent as a rename: the workspace does not exist on the machine yet, so the rename is answered `NotFound` and dropped, and the create that follows names the workspace whatever `fresh_workspace_name` rolled. That is #618, and the fix was to park the name on the window and have the pull spend it — offering it to the create, and, if the machine came back saying something else, sending it afterwards as the rename it had become. #604 then wired that answer straight to the switcher chip. The trouble is that "the machine came back saying something else" is also what walking into somebody else's workspace looks like. `settle_chosen_name` had two facts to work with — a parked string and the name the machine answered with — and neither of them says which workspace the name was meant for, or whether anything was created at all. So it fired the rename at whatever workspace the window had landed on. In #716 that was a workspace already holding nineteen panes on another machine, and it came back named after the arriving client's login. The report reads as one failure; it was two, and this is the second. The parked name now carries the workspace it was typed for, and the pull now says how the workspace got there. `pull_or_create` and `pull_workspace` answer `Arrival::Created` when a create ran and `Arrival::Adopted` when the tree simply had the workspace already, and the name is spent only when both agree: same workspace, and a create to ride along with. Losing the create race still counts as `Created` — a create did run for that workspace a moment ago, it just was not this one, and its rolled codename is precisely what #618 exists to beat. A name that does not match is left parked rather than dropped. Opening a workspace runs two pulls at once, `start_prime`'s and `hydrate`'s, and only one of them creates; taking the name on the adopting one would let the loser of that race swallow it before the winner could spend it, which is the regression this is trying not to reintroduce. Nothing leaks: `forget` drops the whole window state when the window leaves the workspace. `a_workspace_the_machine_already_had_still_takes_the_typed_name` is reshaped rather than deleted. What it was really pinning is the create that answers under another name, and that is still asserted, under a name that says so. The half it asserted wrongly — that an adopted workspace takes the name too — is now pinned the other way, once through `finish_prime` and once through `settle_hydration`, which is the path the report actually came in through. Left alone: the daemon still executes a `WorkspaceRename` from any client without asking whether that client has ever pulled the tree. Like the tab-tree half of #716, this fix is client-side, and an older build can still do it to a machine running current main. Claude-Session: https://claude.ai/code/session_01UUyWQXzcBAoBzaSX8pc7nU --- src/ui/tree_sync.rs | 346 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 291 insertions(+), 55 deletions(-) diff --git a/src/ui/tree_sync.rs b/src/ui/tree_sync.rs index f78b025d..524e1b60 100644 --- a/src/ui/tree_sync.rs +++ b/src/ui/tree_sync.rs @@ -843,6 +843,38 @@ enum SyncPhase { Primed(WsMirror), } +/// A name the user typed, and the workspace they typed it for. +/// +/// The workspace id is the whole point. Parked on its own, a name is just "the +/// last thing somebody typed into a create form", and `settle_chosen_name` had +/// no way to tell a workspace it had named into existence from one the window +/// had since walked into — so it renamed whichever workspace the window +/// happened to land on (#716). Carrying the id it was chosen for makes that +/// question answerable. +#[derive(Clone, Debug)] +struct ChosenName { + /// The workspace on the machine — `tree_workspace_id`, not the client's own + /// id — that this name was typed for. + workspace: WorkspaceId, + name: String, +} + +/// How the workspace a pull answered for got there. +/// +/// The name a user types belongs to a create. When one runs — this client's, or +/// a create of its own that raced it and won with a rolled codename — the typed +/// name is owed and goes out as a rename if the machine came back with anything +/// else (#618, #604). When the workspace was simply already on the machine, +/// nothing was created, the window walked into somebody else's workspace, and +/// the name it is called is its own. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum Arrival { + /// A create ran for this workspace as part of this pull. + Created, + /// The workspace was on the machine before this window asked for it. + Adopted, +} + struct WsState { sync: SyncPhase, queue: VecDeque, @@ -917,7 +949,7 @@ struct WsState { /// names it whatever `fresh_workspace_name` rolled (#618). Consumed by /// `start_prime`, which spends it instead of the generated name, and /// cleared by `finish_prime` once the machine has confirmed a name. - chosen_name: Option, + chosen_name: Option, /// Whether this window has already been told why it opened empty. /// /// The retry is as quiet as the failure was, so a window whose machine @@ -1213,6 +1245,10 @@ fn unsendable(request: &ControlRequest, why: &str) { /// A window already synced with its machine has no create coming, so there is /// nothing to ride along with and the rename goes out as usual. pub(crate) fn name_new_workspace(cx: &mut App, client_ws: WorkspaceId, name: String) { + // Read before the state is borrowed, and read *now* rather than at settle + // time: this is the moment the user named a particular workspace, and it is + // the only moment at which the pairing is certain. + let workspace = tree_workspace_id(cx, client_ws); // A window tree-sync has never heard of is as unprimed as one it is // priming right now: either way the create is still ahead of us. let state = cx @@ -1221,7 +1257,7 @@ pub(crate) fn name_new_workspace(cx: &mut App, client_ws: WorkspaceId, name: Str .entry(client_ws) .or_default(); if matches!(state.sync, SyncPhase::Unprimed { .. }) { - state.chosen_name = Some(name); + state.chosen_name = Some(ChosenName { workspace, name }); return; } rename_workspace(cx, client_ws, Some(name)); @@ -1234,7 +1270,8 @@ pub(crate) fn chosen_name_for(cx: &mut App, client_ws: WorkspaceId) -> Option() .windows .get(&client_ws) - .and_then(|state| state.chosen_name.clone()) + .and_then(|state| state.chosen_name.as_ref()) + .map(|chosen| chosen.name.clone()) } pub(crate) fn rename_workspace(cx: &mut App, client_ws: WorkspaceId, name: Option) { @@ -1281,7 +1318,9 @@ fn start_prime(cx: &mut App, client_ws: WorkspaceId) { cx.default_global::() .windows .get(&client_ws) - .and_then(|state| state.chosen_name.clone()) + .and_then(|state| state.chosen_name.as_ref()) + .filter(|chosen| chosen.workspace == machine_ws) + .map(|chosen| chosen.name.clone()) .unwrap_or_else(|| fresh_workspace_name(cx, host)) }); let outcome = cx @@ -1329,38 +1368,69 @@ pub(crate) fn fresh_workspace_name(cx: &App, host: HostId) -> String { /// /// `answered` is the machine's answer. A chosen name it read back was spent by /// the create that carried it, and there is nothing left to do. One it did not -/// means the create never ran — the workspace was already there, or the other -/// create won the race with a stale idea of the name — so it goes out as the -/// rename it has become. Either way the name is owed only once. +/// means the create ran under a different name — this client's create lost the +/// race to a create of its own that had rolled a codename before the user had +/// finished typing — so it goes out as the rename it has become (#618, #604). +/// Either way the name is owed only once. +/// +/// `arrival` is what stops that override from firing at a workspace nobody +/// asked to create. A name is spent only on a create it actually rode along +/// with: the workspace it was typed for, and a pull that had to make that +/// workspace. Walking into a workspace the machine already had renamed it to +/// whatever the arriving client had parked — which is how a remote client's +/// login name ended up on somebody else's workspace (#716). +/// +/// A name that does not match this arrival is left parked rather than dropped. +/// Two pulls run at once when a window opens a workspace (`start_prime`'s and +/// `hydrate`'s) and only one of them creates; taking the name on the adopting +/// one would let the loser of that race swallow it before the winner could +/// spend it, which is the #618 regression this is trying not to reintroduce. +/// A parked name outlives nothing: `forget` drops the whole state when the +/// window leaves the workspace. fn settle_chosen_name( cx: &mut App, client_ws: WorkspaceId, answered: Option, + arrival: Arrival, ) -> Option { - let chosen = cx - .default_global::() - .windows - .get_mut(&client_ws) - .and_then(|state| state.chosen_name.take()); - match chosen { - Some(chosen) if answered.as_deref() == Some(chosen.as_str()) => answered, - Some(chosen) => { - rename_workspace(cx, client_ws, Some(chosen.clone())); - Some(chosen) + let machine_ws = tree_workspace_id(cx, client_ws); + let Some(state) = cx.default_global::().windows.get_mut(&client_ws) else { + return answered; + }; + let owed = state + .chosen_name + .as_ref() + .is_some_and(|chosen| chosen.workspace == machine_ws && arrival == Arrival::Created); + if !owed { + if let Some(parked) = &state.chosen_name { + log::debug!( + "workspace {client_ws}: keeping the name {} answered, not the parked \ + '{}' — nothing was created here", + answered.as_deref().unwrap_or(""), + parked.name + ); } - None => answered, + return answered; } + let chosen = state.chosen_name.take().expect("checked just above").name; + if answered.as_deref() == Some(chosen.as_str()) { + return answered; + } + rename_workspace(cx, client_ws, Some(chosen.clone())); + Some(chosen) } fn pull_or_create( client: &ControlClient, machine_ws: WorkspaceId, fresh: String, -) -> io::Result<(WsMirror, Option)> { +) -> io::Result<(WsMirror, Option, Arrival)> { match client.call(ControlRequest::WorkspaceTree { workspace: machine_ws, }) { - Ok(ReplyOk::WorkspaceTree(ws)) => Ok(primed(*ws)), + // The tree answered, so nothing was created here — this window walked + // into a workspace that was already on the machine. + Ok(ReplyOk::WorkspaceTree(ws)) => Ok(primed(*ws, Arrival::Adopted)), Ok(other) => Err(io::Error::other(format!( "WorkspaceTree answered {other:?}" ))), @@ -1369,7 +1439,7 @@ fn pull_or_create( name: Some(fresh), workspace: Some(machine_ws), })? { - ReplyOk::WorkspaceTree(ws) => Ok(primed(*ws)), + ReplyOk::WorkspaceTree(ws) => Ok(primed(*ws, Arrival::Created)), other => Err(io::Error::other(format!( "WorkspaceCreate answered {other:?}" ))), @@ -1379,13 +1449,14 @@ fn pull_or_create( } } -fn primed(ws: Workspace) -> (WsMirror, Option) { +fn primed(ws: Workspace, arrival: Arrival) -> (WsMirror, Option, Arrival) { ( WsMirror { tabs: ws.tabs, active: ws.active_tab, }, ws.name, + arrival, ) } @@ -1393,7 +1464,7 @@ fn finish_prime( cx: &mut App, client_ws: WorkspaceId, epoch: u64, - outcome: io::Result<(WsMirror, Option)>, + outcome: io::Result<(WsMirror, Option, Arrival)>, ) { let Some(state) = cx.default_global::().windows.get_mut(&client_ws) else { return; @@ -1404,12 +1475,12 @@ fn finish_prime( } let was_dirty = matches!(state.sync, SyncPhase::Unprimed { dirty: true, .. }); let landed = match outcome { - Ok((mirror, name)) => { + Ok((mirror, name, arrival)) => { state.informed |= mirror.tabs.is_empty(); // The machine answered, which is the only thing the retry was // waiting to find out, so the next failure starts its backoff over. state.rehydrate_attempts = 0; - let landed = (mirror.tabs.clone(), mirror.active, name); + let landed = (mirror.tabs.clone(), mirror.active, name, arrival); state.sync = SyncPhase::Primed(mirror); landed } @@ -1429,7 +1500,7 @@ fn finish_prime( ); // The pull above is the only place this window will hear the workspace's // name — it is left out of the deltas its own create raises (#604). - let name = settle_chosen_name(cx, client_ws, landed.2); + let name = settle_chosen_name(cx, client_ws, landed.2, landed.3); crate::ui::machine_mirror::MachineMirrors::note_workspace_name(cx, host, machine_ws, name); if !was_dirty { return; @@ -1855,7 +1926,9 @@ fn hydrate_with(cx: &mut App, client_ws: WorkspaceId, adopt: Adopt, showing: Vec cx.default_global::() .windows .get(&client_ws) - .and_then(|state| state.chosen_name.clone()) + .and_then(|state| state.chosen_name.as_ref()) + .filter(|chosen| chosen.workspace == machine_ws) + .map(|chosen| chosen.name.clone()) }); let outcome = cx .background_executor() @@ -2005,9 +2078,13 @@ fn pull_workspace( client: &ControlClient, machine_ws: WorkspaceId, chosen: Option, -) -> io::Result<(Machine, WsMirror, Session)> { +) -> io::Result<(Machine, WsMirror, Session, Arrival)> { + // The workspace was on the machine before this pull touched it: adopted, + // whatever name anyone has parked for it. let mut machine = match layout_of(machine_get(client)?, machine_ws) { - Ok(pulled) => return Ok(pulled), + Ok((machine, mirror, session)) => { + return Ok((machine, mirror, session, Arrival::Adopted)); + } Err(machine) => machine, }; // The whole tree is already in hand, so the taken names can be read @@ -2033,9 +2110,19 @@ fn pull_workspace( Ok(ReplyOk::WorkspaceTree(created)) => { machine.workspaces.retain(|w| w.id != created.id); machine.workspaces.push(*created); - Ok((machine, WsMirror::default(), Session::default())) + Ok(( + machine, + WsMirror::default(), + Session::default(), + Arrival::Created, + )) } - Ok(_) => Ok((machine, WsMirror::default(), Session::default())), + Ok(_) => Ok(( + machine, + WsMirror::default(), + Session::default(), + Arrival::Created, + )), // Losing this create is not a failed hydration. Opening a remote // workspace runs two pulls at once — this one and `start_prime`'s — // and both create when the tree they read did not hold it yet, so the @@ -2054,8 +2141,14 @@ fn pull_workspace( "workspace {machine_ws} could not be created ({refused}); reading the tree \ again in case something else created it first" ); + // Still `Created`, and deliberately: a create did run for this + // workspace a moment ago, it just was not this one. That is the + // race #618 came from — the winner rolled a codename before the + // user had finished typing — and the typed name is owed against it. match machine_get(client) { - Ok(machine) => layout_of(machine, machine_ws).map_err(|_| refused), + Ok(machine) => layout_of(machine, machine_ws) + .map(|(machine, mirror, session)| (machine, mirror, session, Arrival::Created)) + .map_err(|_| refused), Err(_) => Err(refused), } } @@ -2091,7 +2184,7 @@ fn finish_hydration( client_ws: WorkspaceId, epoch: u64, adopt: Adopt, - outcome: io::Result<(Machine, WsMirror, Session)>, + outcome: io::Result<(Machine, WsMirror, Session, Arrival)>, ) { if settle_hydration(cx, client_ws, epoch, adopt, outcome) { open_parked_path(cx, client_ws); @@ -2107,7 +2200,7 @@ fn settle_hydration( client_ws: WorkspaceId, epoch: u64, adopt: Adopt, - outcome: io::Result<(Machine, WsMirror, Session)>, + outcome: io::Result<(Machine, WsMirror, Session, Arrival)>, ) -> bool { let current = cx .default_global::() @@ -2118,7 +2211,7 @@ fn settle_hydration( log::debug!("workspace {client_ws}: dropping a superseded hydration"); return false; } - let (machine, mirror, session) = match outcome { + let (machine, mirror, session, arrival) = match outcome { Ok(pulled) => pulled, Err(e) => { let failures = cx @@ -2144,7 +2237,7 @@ fn settle_hydration( .find(|w| w.id == machine_ws) .and_then(|w| w.name.clone()); crate::ui::machine_mirror::MachineMirrors::install(cx, host, machine); - let name = settle_chosen_name(cx, client_ws, answered); + let name = settle_chosen_name(cx, client_ws, answered, arrival); crate::ui::machine_mirror::MachineMirrors::note_workspace_name(cx, host, machine_ws, name); let machine_was_empty = mirror.tabs.is_empty(); let was_dirty = { @@ -2809,12 +2902,14 @@ mod tests { name_new_workspace(cx, ws, "deploy".into()); + let parked = cx.default_global::().windows[&ws] + .chosen_name + .clone() + .expect("the name rides along with the create instead of chasing it"); + assert_eq!(parked.name, "deploy"); assert_eq!( - cx.default_global::().windows[&ws] - .chosen_name - .as_deref(), - Some("deploy"), - "the name rides along with the create instead of chasing it" + parked.workspace, ws, + "and it is parked against the workspace it was typed for, so a window that walks into a different one cannot spend it (#716)" ); }); } @@ -2831,7 +2926,7 @@ mod tests { cx, ws, epoch, - Ok((WsMirror::default(), Some("deploy".into()))), + Ok((WsMirror::default(), Some("deploy".into()), Arrival::Created)), ); assert!( @@ -2848,12 +2943,17 @@ mod tests { }); } - /// The other branch of `pull_or_create`: the workspace was already on the - /// machine, so the pull answered and the create never ran — nobody was ever - /// offered the typed name. It has to go out as a rename, and it has to beat - /// the name the pull came back with, which #604 wired straight to the chip. + /// A create ran and came back under a different name — this window's create + /// lost the race to the other pull's, which had rolled a codename before + /// the user finished typing. The typed name is still owed and still has to + /// beat what the pull answered with, which is #618 and the chip #604 wired. + /// + /// Reshaped from `a_workspace_the_machine_already_had_still_takes_the_typed_name`, + /// which asserted this override for *every* answer, adopted workspaces + /// included. The override is right; the blanket was not (#716) — see the + /// test below for the half that was wrong. #[gpui::test] - fn a_workspace_the_machine_already_had_still_takes_the_typed_name( + fn a_create_that_answered_with_another_name_still_takes_the_typed_one( cx: &mut gpui::TestAppContext, ) { cx.update(|cx| { @@ -2864,7 +2964,11 @@ mod tests { cx, ws, epoch, - Ok((WsMirror::default(), Some("keen-marten".into()))), + Ok(( + WsMirror::default(), + Some("keen-marten".into()), + Arrival::Created, + )), ); assert!( @@ -2876,7 +2980,73 @@ mod tests { assert_eq!( crate::ui::machine_mirror::display_name(cx, &view).as_deref(), Some("deploy"), - "the name the user typed outranks the one the pull answered with" + "the name the user typed outranks the one the create answered with" + ); + }); + } + + /// The other half of that split, and #716's second failure. A client + /// opening a workspace that was already on the machine renamed it to + /// whatever that client had parked — a workspace holding nineteen panes + /// came back named after the arriving user. Nothing was created here, so + /// nothing is owed: the workspace keeps the name it has. + #[gpui::test] + fn a_workspace_the_window_walked_into_keeps_its_own_name(cx: &mut gpui::TestAppContext) { + cx.update(|cx| { + let (ws, view) = primed_window(cx, Some("sher1")); + let epoch = cx.default_global::().windows[&ws].epoch; + + finish_prime( + cx, + ws, + epoch, + Ok(( + WsMirror::default(), + Some("keen-marten".into()), + Arrival::Adopted, + )), + ); + + assert_eq!( + crate::ui::machine_mirror::display_name(cx, &view).as_deref(), + Some("keen-marten"), + "the machine's name stands; an arriving client does not rename it" + ); + }); + } + + /// The identity check, independently of how the pull arrived. A name typed + /// for one workspace must not be spendable on another even when a create + /// did run: the pairing is what makes the name mean anything. + #[gpui::test] + fn a_name_typed_for_another_workspace_is_never_spent_here(cx: &mut gpui::TestAppContext) { + cx.update(|cx| { + let (ws, view) = primed_window(cx, Some("deploy")); + cx.default_global::() + .windows + .get_mut(&ws) + .expect("primed just above") + .chosen_name = Some(ChosenName { + workspace: WorkspaceId::new(), + name: "deploy".into(), + }); + let epoch = cx.default_global::().windows[&ws].epoch; + + finish_prime( + cx, + ws, + epoch, + Ok(( + WsMirror::default(), + Some("keen-marten".into()), + Arrival::Created, + )), + ); + + assert_eq!( + crate::ui::machine_mirror::display_name(cx, &view).as_deref(), + Some("keen-marten"), + "a name owed to another workspace is not this one's to take" ); }); } @@ -2906,7 +3076,12 @@ mod tests { ws, epoch, Adopt::IfEmpty, - Ok((pulled, WsMirror::default(), Session::default())), + Ok(( + pulled, + WsMirror::default(), + Session::default(), + Arrival::Created, + )), ); assert_eq!( @@ -2923,6 +3098,46 @@ mod tests { }); } + /// The same window arriving at a workspace it did not make, which is the + /// path #716 came in through: `switch_workspace` orders the hydration, the + /// pull finds the workspace already there, and the name parked on the + /// window used to land on it as a rename. + #[gpui::test] + fn a_hydration_that_adopted_leaves_the_name_it_found(cx: &mut gpui::TestAppContext) { + cx.update(|cx| { + crate::ui::windows::WindowRegistry::init(cx); + let (ws, view) = primed_window(cx, Some("sher1")); + let epoch = cx.default_global::().windows[&ws].epoch; + let pulled = Machine { + workspaces: vec![tty7_core::core::machine::Workspace { + id: ws, + name: Some("keen-marten".into()), + ..Default::default() + }], + panes: Vec::new(), + }; + + settle_hydration( + cx, + ws, + epoch, + Adopt::IfEmpty, + Ok(( + pulled, + WsMirror::default(), + Session::default(), + Arrival::Adopted, + )), + ); + + assert_eq!( + crate::ui::machine_mirror::display_name(cx, &view).as_deref(), + Some("keen-marten"), + "walking in is not naming" + ); + }); + } + /// A window with no name owed reads whatever the machine says, which is /// the whole of #604 and must survive the arbitration above. #[gpui::test] @@ -2935,7 +3150,11 @@ mod tests { cx, ws, epoch, - Ok((WsMirror::default(), Some("keen-marten".into()))), + Ok(( + WsMirror::default(), + Some("keen-marten".into()), + Arrival::Created, + )), ); assert_eq!( @@ -2973,7 +3192,10 @@ mod tests { dirty: false, priming: true, }; - state.chosen_name = chosen.map(str::to_string); + state.chosen_name = chosen.map(|name| ChosenName { + workspace: ws, + name: name.to_string(), + }); (ws, view) } @@ -3492,7 +3714,11 @@ mod tests { cx, ws, epoch, - Ok((WsMirror::default(), Some("keen-marten".to_string()))), + Ok(( + WsMirror::default(), + Some("keen-marten".to_string()), + Arrival::Created, + )), ); assert_eq!( @@ -3548,7 +3774,12 @@ mod tests { // The machine answered. Whatever it was, it is over. unprimed(cx); - finish_prime(cx, ws, epoch, Ok((WsMirror::default(), None))); + finish_prime( + cx, + ws, + epoch, + Ok((WsMirror::default(), None, Arrival::Created)), + ); assert_eq!( attempts(cx), 0, @@ -4028,7 +4259,12 @@ mod tests { state.sync = SyncPhase::Primed(advanced.clone()); } - finish_prime(cx, ws, stale_epoch, Ok((WsMirror::default(), None))); + finish_prime( + cx, + ws, + stale_epoch, + Ok((WsMirror::default(), None, Arrival::Created)), + ); match &cx.default_global::().windows[&ws].sync { SyncPhase::Primed(mirror) => assert_eq!( From e320f70fb05068471882c012b787164863798e73 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Wed, 9 Sep 2026 22:36:17 +0800 Subject: [PATCH 08/15] fix(terminal): encode the function keys and stop swallowing them (#834) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1 through F12 produced no bytes at all. `functional_key` in `src/terminal/input.rs` is the one place a named key becomes an escape sequence, and it knew the cursor and editing keys and nothing else; the `key_char` fallback below it cannot help, because a function key arrives with no character, and `text_key_code` gives up the moment a key name is longer than one character. Both encoders funnel through that function, so the kitty path was equally blank. This was never a Windows or a PowerShell problem — it was every platform, every shell — but it shows up first on Windows because PSReadLine keeps CharacterSearch on F3, HistorySearch on F8 and ClearHistory on Alt+F7, so the missing keys are part of the shell's ordinary editing surface rather than something exotic. The table is `xterm-256color`'s, since that is what we advertise in `$TERM`: `kf1`..`kf4` are the SS3 letters, `kf5`..`kf12` the numbered tilde forms whose numbering starts at 15 and skips both 16 and 22. Modified, they take the same `CSI 1;` and `CSI ;` shapes the cursor keys already use, which reproduces `kf13` onwards exactly — `kf13` is Shift+F1, `kf25` is Ctrl+F1. DECCKM is deliberately not consulted: unlike `kcuu1`, `kf1` is SS3 under both `smkx` and `rmkx`. It stops at F12 on purpose. In the entry we advertise there is no capability left for a thirteenth key — `kf13` and up are already the modified forms of F1..F8 — so sending the VT220 `\E[25~` for a physical F13 would hand ncurses a sequence its own table reads back as Shift+F1. Silence is the more honest answer, and F13+ keys are close to nonexistent on the keyboards this bug was filed from. Encoding alone would not have reached the shell, because two things upstream were eating the keys first. The inline prompt editor had no arm for a named key it does not bind, so F8 fell out of the bottom of `handle_editor_key` and died on a `cx.notify()`; it now takes the same route an unknown Ctrl chord takes, handing the line to the shell before sending the key, which is what PSReadLine's history and character searches need in order to act on it. And Find Next / Previous sit on F3 and Shift+F3 off macOS, where gpui matches the binding before the pane's key handler ever runs; with no find bar open there is no match to step to, so those listeners now give the keystroke back the way `EditorSave` gives back Ctrl+S. F11 keeps fullscreen. It is the chord Windows Terminal, GNOME Terminal and konsole all use, no shell binds it, the modified forms still reach the PTY, and it is one line of config away from being retired — but it is now a stated choice with a test standing on it rather than an accident. The reporter's second clause is explained rather than fixed: `prompt_editor` is a real gate, and turning it off could not help while the bytes did not exist, which is exactly what they saw. Claude-Session: https://claude.ai/code/session_01UUyWQXzcBAoBzaSX8pc7nU --- src/terminal/input.rs | 252 +++++++++++++++++++++++++++++++++++++++++- src/terminal/view.rs | 115 +++++++++++++++++++ src/ui/keymap.rs | 55 +++++++++ 3 files changed, 419 insertions(+), 3 deletions(-) diff --git a/src/terminal/input.rs b/src/terminal/input.rs index 0fe392ac..e198bc77 100644 --- a/src/terminal/input.rs +++ b/src/terminal/input.rs @@ -171,7 +171,7 @@ fn csi_u(code: u32, mods: u32, text: Option<&[u32]>) -> Vec { s.into_bytes() } -/// The cursor and editing keys, encoded the way `xterm-256color`'s terminfo +/// The cursor, editing and function keys, encoded the way `xterm-256color`'s terminfo /// says they are — which is what we advertise in `$TERM`, and what ncurses /// matches against byte for byte. /// @@ -197,6 +197,19 @@ fn functional_key(key: &str, mods: u32, app_cursor: bool) -> Option> { }; return Some(s.into_bytes()); } + if let Some(form) = function_key(key) { + let s = match form { + // `kf1=\EOP` .. `kf4=\EOS`, and modified the `CSI 1;` form + // the cursor keys use — terminfo spells Shift+F1 `kf13=\E[1;2P`. + // DECCKM has no say here: unlike `kcuu1`, `kf1` is SS3 under both + // `smkx` and `rmkx`. + FunctionKey::Ss3(l) if mods == 1 => format!("\x1bO{l}"), + FunctionKey::Ss3(l) => format!("\x1b[1;{mods}{l}"), + FunctionKey::Tilde(n) if mods == 1 => format!("\x1b[{n}~"), + FunctionKey::Tilde(n) => format!("\x1b[{n};{mods}~"), + }; + return Some(s.into_bytes()); + } let num = match key { "insert" => Some(2u32), "delete" => Some(3), @@ -215,6 +228,53 @@ fn functional_key(key: &str, mods: u32, app_cursor: bool) -> Option> { None } +/// The two shapes `xterm-256color` gives a function key. +enum FunctionKey { + /// `SS3 ` unmodified, `CSI 1; ` with a modifier. + Ss3(char), + /// `CSI ~`, or `CSI ; ~` with a modifier. + Tilde(u32), +} + +/// `f1`..`f12` — the name gpui gives these keys on all three platforms, and +/// the range `xterm-256color` gives a key of its own. +/// +/// The numbering is the PC-style table xterm has used since patch #94 and the +/// one `kf5`..`kf12` spell: it starts at 15 and skips both 16 and 22, because +/// those two were DEC's "do" and "help" on the VT220 keypad. Guessing a +/// contiguous run here is the classic way to make F6 arrive as F5. +/// +/// This deliberately stops at F12. In the entry we advertise, `kf13` onwards +/// are not further keys — they are the *modified* forms of F1..F8 (`kf13` is +/// `\E[1;2P`, Shift+F1), which the `mods` parameter above already produces. +/// A physical F13 therefore has no encoding of its own under this `$TERM`; +/// sending the VT220 `\E[25~` for it would hand ncurses a sequence its own +/// table reads back as Shift+F1, which is worse than sending nothing. +fn function_key(key: &str) -> Option { + Some(match key { + "f1" => FunctionKey::Ss3('P'), + "f2" => FunctionKey::Ss3('Q'), + "f3" => FunctionKey::Ss3('R'), + "f4" => FunctionKey::Ss3('S'), + "f5" => FunctionKey::Tilde(15), + "f6" => FunctionKey::Tilde(17), + "f7" => FunctionKey::Tilde(18), + "f8" => FunctionKey::Tilde(19), + "f9" => FunctionKey::Tilde(20), + "f10" => FunctionKey::Tilde(21), + "f11" => FunctionKey::Tilde(23), + "f12" => FunctionKey::Tilde(24), + _ => return None, + }) +} + +/// Whether a key name is one `functional_key` turns into a function-key +/// sequence. The inline editor asks this to know a keystroke it holds no +/// meaning for but the shell does. +pub(crate) fn is_function_key(key: &str) -> bool { + function_key(key).is_some() +} + fn text_key_code(ks: &gpui::Keystroke) -> Option { match ks.key.as_str() { "space" => Some(0x20), @@ -682,6 +742,123 @@ mod tests { } } + /// Issue #834: the function keys produced no bytes at all, on any + /// platform. PSReadLine puts CharacterSearch on F3 and HistorySearch on + /// F8, so on Windows this took part of the shell's normal editing surface + /// away. + /// + /// The table is `xterm-256color`'s `kf1`..`kf12` verbatim, gaps included: + /// F5 is 15 and F6 is 17, F10 is 21 and F11 is 23. + #[test] + fn keystroke_to_bytes_encodes_f1_through_f12_like_terminfo() { + let none = Modifiers::default(); + let cases: &[(&str, &[u8])] = &[ + ("f1", b"\x1bOP"), + ("f2", b"\x1bOQ"), + ("f3", b"\x1bOR"), + ("f4", b"\x1bOS"), + ("f5", b"\x1b[15~"), + ("f6", b"\x1b[17~"), + ("f7", b"\x1b[18~"), + ("f8", b"\x1b[19~"), + ("f9", b"\x1b[20~"), + ("f10", b"\x1b[21~"), + ("f11", b"\x1b[23~"), + ("f12", b"\x1b[24~"), + ]; + for (key, seq) in cases { + assert_eq!( + legacy(&ks(none, key, None)).as_deref(), + Some(*seq), + "{key} unmodified" + ); + // gpui hands a function key over with no `key_char`, but the + // Windows backend has been known to attach an empty one; neither + // may reach the `key_char` fallback and send nothing. + assert_eq!( + legacy(&ks(none, key, Some(""))).as_deref(), + Some(*seq), + "{key} with an empty key_char" + ); + } + } + + /// The modified forms are the same ones terminfo lists under `kf13` + /// onwards: `kf13=\E[1;2P` is Shift+F1, `kf17=\E[15;2~` is Shift+F5, + /// `kf25=\E[1;5P` is Ctrl+F1. Alt is xterm's 3, which PSReadLine wants for + /// Alt+F7 (ClearHistory). + #[test] + fn keystroke_to_bytes_modifies_function_keys_like_terminfo() { + let shift = Modifiers { + shift: true, + ..Default::default() + }; + let alt = Modifiers { + alt: true, + ..Default::default() + }; + let ctrl = Modifiers { + control: true, + ..Default::default() + }; + let ctrl_shift = Modifiers { + control: true, + shift: true, + ..Default::default() + }; + // kf13, kf16 + assert_eq!(legacy(&ks(shift, "f1", None)), Some(b"\x1b[1;2P".to_vec())); + assert_eq!(legacy(&ks(shift, "f4", None)), Some(b"\x1b[1;2S".to_vec())); + // kf25 + assert_eq!(legacy(&ks(ctrl, "f1", None)), Some(b"\x1b[1;5P".to_vec())); + // kf37 + assert_eq!( + legacy(&ks(ctrl_shift, "f1", None)), + Some(b"\x1b[1;6P".to_vec()) + ); + // kf49 + assert_eq!(legacy(&ks(alt, "f1", None)), Some(b"\x1b[1;3P".to_vec())); + // kf20 -- Shift+F8, PSReadLine's HistorySearchForward + assert_eq!(legacy(&ks(shift, "f8", None)), Some(b"\x1b[19;2~".to_vec())); + // kf55 -- Alt+F7, PSReadLine's ClearHistory + assert_eq!(legacy(&ks(alt, "f7", None)), Some(b"\x1b[18;3~".to_vec())); + // kf35 -- Ctrl+F11 + assert_eq!(legacy(&ks(ctrl, "f11", None)), Some(b"\x1b[23;5~".to_vec())); + } + + /// The `mods` parameter is what makes F13 onwards ambiguous: in + /// `xterm-256color` those capability names are already spoken for by the + /// modified F1..F8, so a physical F13 has no sequence of its own to send. + #[test] + fn function_keys_stop_at_f12() { + let none = Modifiers::default(); + let ctrl = Modifiers { + control: true, + ..Default::default() + }; + for key in ["f13", "f14", "f20", "f24"] { + assert_eq!(legacy(&ks(none, key, None)), None, "{key} is unencodable"); + assert_eq!( + keystroke_to_bytes(&ks(none, key, None), kitty()), + None, + "{key} is unencodable under kitty too" + ); + } + // Ctrl+F13 must not fold into a C0 byte on its first letter either. + assert_eq!(legacy(&ks(ctrl, "f13", None)), None); + } + + /// Cmd is not an xterm modifier, so it must not turn a bare F-key into a + /// modified one — and on macOS Cmd+F-keys belong to the window anyway. + #[test] + fn cmd_does_not_modify_a_function_key() { + let cmd = Modifiers { + platform: true, + ..Default::default() + }; + assert_eq!(legacy(&ks(cmd, "f5", None)), Some(b"\x1b[15~".to_vec())); + } + fn app_cursor() -> KeyFlags { KeyFlags { app_cursor: true, @@ -689,6 +866,21 @@ mod tests { } } + /// DECCKM governs `kcuu1` and friends, not `kf1`: terminfo spells `kf1` + /// `\EOP` under both `smkx` and `rmkx`, so the F keys must not move when + /// an ncurses app turns application cursor keys on. + #[test] + fn app_cursor_mode_leaves_the_function_keys_alone() { + let none = Modifiers::default(); + for key in ["f1", "f4", "f5", "f12"] { + assert_eq!( + keystroke_to_bytes(&ks(none, key, None), app_cursor()), + legacy(&ks(none, key, None)), + "{key} does not follow DECCKM" + ); + } + } + /// The bug behind issue #361: htop turns on DECCKM, `xterm-256color` spells /// `kcuu1` as `\EOA`, and ncurses matches nothing else. Sending `\E[A` there /// left htop reading the bytes one at a time -- and `[` is bound to "lower @@ -801,9 +993,11 @@ mod tests { ..Default::default() }; assert_eq!(legacy(&ks(alt, "b", Some("b"))), Some(b"\x1bb".to_vec())); + // `f13` stands in for "a named key with nothing behind it" — it used + // to be `f7`, back when no function key encoded at all (#834). let none = Modifiers::default(); - assert_eq!(legacy(&ks(none, "f7", Some(""))), None); - assert_eq!(legacy(&ks(none, "f7", None)), None); + assert_eq!(legacy(&ks(none, "f13", Some(""))), None); + assert_eq!(legacy(&ks(none, "f13", None)), None); } #[test] @@ -984,6 +1178,58 @@ mod tests { ); } + /// The kitty encoder shares `functional_key`, so the F keys have to come + /// out of it byte-identical to the legacy path — kitty's own spec keeps + /// the legacy CSI/SS3 forms for F1..F12 and only appends the modifier + /// parameter, which is what that shared table already does. + /// + /// `report_all_keys` changes nothing here either: the F keys are already + /// escape sequences, so there is no bare byte for it to promote. + #[test] + fn kitty_encodes_function_keys_like_the_legacy_path() { + let none = Modifiers::default(); + let shift = Modifiers { + shift: true, + ..Default::default() + }; + let ctrl = Modifiers { + control: true, + ..Default::default() + }; + let full = KeyFlags { + disambiguate: true, + report_all_keys: true, + report_text: true, + app_cursor: false, + }; + for key in ["f1", "f2", "f3", "f4", "f5", "f7", "f10", "f11", "f12"] { + for mods in [none, shift, ctrl] { + let want = legacy(&ks(mods, key, None)); + assert!(want.is_some(), "{key} encodes on the legacy path"); + assert_eq!( + keystroke_to_bytes(&ks(mods, key, None), kitty()), + want, + "{key} under kitty disambiguate" + ); + assert_eq!( + keystroke_to_bytes(&ks(mods, key, None), full), + want, + "{key} under kitty report-all-keys" + ); + } + } + // Spot-check the actual bytes, so a change to `legacy` cannot quietly + // move both sides at once. + assert_eq!( + keystroke_to_bytes(&ks(none, "f7", None), full), + Some(b"\x1b[18~".to_vec()) + ); + assert_eq!( + keystroke_to_bytes(&ks(shift, "f1", None), full), + Some(b"\x1b[1;2P".to_vec()) + ); + } + #[test] fn kitty_report_all_keys_escapes_plain_text_with_associated_text() { let full = KeyFlags { diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 50f6c259..e5c2f206 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -2482,6 +2482,19 @@ impl TerminalView { self.close_completion(); + // A function key means nothing to this editor and everything to the + // shell — PSReadLine puts CharacterSearch on F3 and HistorySearch on + // F8, and both of those act on the line that is currently on the + // prompt. So it takes the same route an unknown Ctrl chord takes: + // hand the line over first, then send the key, with every modifier + // combination going the same way (Alt+F7 is ClearHistory). + if super::input::is_function_key(key) && !m.platform { + if let Some(bytes) = super::input::keystroke_to_bytes(ks, self.key_flags()) { + self.handoff_line_to_shell(&bytes, cx); + return; + } + } + if m.control && !m.platform && !m.alt { if cfg!(not(target_os = "macos")) { match key { @@ -6859,10 +6872,24 @@ impl Render for TerminalView { .on_action( cx.listener(|this, _: &FindInTerminal, window, cx| this.open_search(window, cx)), ) + // Off macOS these two live on F3 and Shift+F3, which is also where + // PSReadLine keeps CharacterSearch and readline users put their + // own widgets. With no find bar open there is no next match to + // step to, so the keystroke is given back the way `EditorSave` + // gives back Ctrl+S — otherwise the action swallows the key and + // the shell never sees it (#834). .on_action(cx.listener(|this, _: &FindNext, _w, cx| { + if this.search.is_none() { + cx.propagate(); + return; + } this.step_match(Direction::Right, cx); })) .on_action(cx.listener(|this, _: &FindPrevious, _w, cx| { + if this.search.is_none() { + cx.propagate(); + return; + } this.step_match(Direction::Left, cx); })) .on_action(cx.listener(|this, _: &ClearScrollback, _w, cx| this.clear_scrollback(cx))) @@ -11960,6 +11987,59 @@ mod gpui_tests { .unwrap(); } + /// The whole chain for #834, through the real dispatch tree: F3 is bound + /// to Find Next off macOS, and gpui matches bindings before the pane's key + /// handler. With no find bar open the action gives the key back, the pane + /// encodes it, and PSReadLine's CharacterSearch gets its `\EOR`. + /// + /// F7 has no binding at all and is the control: it takes the same route + /// with nothing to fall through. + #[cfg(not(target_os = "macos"))] + #[gpui::test] + fn an_unused_find_binding_gives_f3_back_to_the_shell(cx: &mut TestAppContext) { + let (window, mut daemon) = harness(cx); + cx.update(|cx| crate::ui::keymap::init(cx)); + prompt_ready(&window, cx, &mut daemon); + window + .update(cx, |view, window, cx| { + window.activate_window(); + view.focus_handle.focus(window, cx); + view.commit_text("echo a", cx); + }) + .unwrap(); + + let mut vcx = gpui::VisualTestContext::from_window(window.into(), cx); + for (chord, seq) in [("f3", b"\x1bOR".to_vec()), ("f7", b"\x1b[18~".to_vec())] { + window + .update(cx, |view, _, _| { + // The previous handoff gave this prompt to the shell for + // good; take it back so both keys are tested from the + // same starting state. + view.editor_handoff = None; + view.cmd.set("echo a"); + assert!(view.search.is_none(), "no find bar is open"); + }) + .unwrap(); + vcx.simulate_keystrokes(chord); + window + .update(cx, |view, _, _| { + assert!(view.search.is_none(), "{chord} did not open the find bar"); + assert_eq!(view.cmd.text(), "", "{chord} handed the line over"); + }) + .unwrap(); + assert_eq!( + next_input_until_timeout(&mut daemon), + Some(b"echo a".to_vec()), + "{chord} puts the line on the shell's prompt first" + ); + assert_eq!( + next_input_until_timeout(&mut daemon), + Some(seq), + "{chord} reaches the PTY" + ); + } + } + #[gpui::test] fn ctrl_r_fuzzy_search_accepts_into_the_editor(cx: &mut TestAppContext) { let (window, _daemon) = harness(cx); @@ -12027,6 +12107,41 @@ mod gpui_tests { ); } + /// The second half of #834. Even once the encoder knew the F keys, the + /// inline editor still ate them: `handle_editor_key` had no arm for a + /// named key it does not bind, so F8 fell out of the bottom of the match + /// and died on a `cx.notify()`. PSReadLine's HistorySearchBackward acts on + /// the line that is on the prompt, so the fix is the unknown-chord route — + /// the line goes over first, then the key. + #[gpui::test] + fn function_keys_hand_the_line_to_the_shell(cx: &mut TestAppContext) { + let (window, mut daemon) = harness(cx); + for (chord, seq) in [ + ("f8", b"\x1b[19~".to_vec()), + ("shift-f8", b"\x1b[19;2~".to_vec()), + ("alt-f7", b"\x1b[18;3~".to_vec()), + ("f3", b"\x1bOR".to_vec()), + ] { + window + .update(cx, |view, _, cx| { + view.cmd.set("git st"); + view.handle_editor_key(&key(chord), cx); + assert_eq!(view.cmd.text(), "", "{chord} handed the line over"); + }) + .unwrap(); + assert_eq!( + next_input_until_timeout(&mut daemon), + Some(b"git st".to_vec()), + "{chord} puts the line on the shell's prompt first" + ); + assert_eq!( + next_input_until_timeout(&mut daemon), + Some(seq), + "{chord} follows the line" + ); + } + } + #[gpui::test] fn ctrl_j_and_ctrl_m_submit_the_line_like_enter(cx: &mut TestAppContext) { crate::core::config::pin_test_config_dir(); diff --git a/src/ui/keymap.rs b/src/ui/keymap.rs index 615b63e0..6fb2d808 100644 --- a/src/ui/keymap.rs +++ b/src/ui/keymap.rs @@ -402,6 +402,15 @@ pub(crate) fn default_bindings() -> Vec<(&'static str, &'static str)> { per_platform("secondary-shift-t", "alt-shift-t"), ), ("ToggleMaximizePane", "secondary-shift-enter"), + // The one default that sits on a bare function key, and it stays + // there: F11 is the fullscreen chord Windows Terminal, GNOME Terminal + // and konsole all train their users on, and no shell binds it — the + // keys PSReadLine actually wants are F3, F7 and F8. gpui matches this + // binding before the pane's key handler runs, so the terminal never + // sees a bare F11; Shift/Ctrl/Alt+F11 are not bound and still reach + // the PTY as `\E[23;~`, and the whole chord is one line of + // config away from being retired. See + // `f11_is_the_only_default_on_a_bare_function_key`. ("ToggleFullscreen", per_platform("secondary-enter", "f11")), ("ToggleTabSidebar", ""), ( @@ -1873,6 +1882,52 @@ mod tests { } } + /// F1..F12 now encode (#834), so a default sitting on one takes it away + /// from the shell the same way a Ctrl binding takes a control code: gpui + /// matches bindings before the pane's key handler runs, so a bound + /// function key never reaches the PTY at all. + /// + /// The three that do are named here rather than left to be discovered, + /// and each answers for the shell key it stands on: + /// + /// * F11 keeps fullscreen outright. It is the chord Windows Terminal, + /// GNOME Terminal and konsole all use, and no shell binds it — + /// PSReadLine's F keys are F3, F7 and F8. + /// * F3 and Shift+F3 are Find Next / Previous, and they fall through: + /// with no find bar open the listener in `terminal::view` calls + /// `cx.propagate()`, so PSReadLine's CharacterSearch still gets the key. + /// + /// A fourth needs a fall-through of its own to join them. + #[test] + fn f11_is_the_only_default_on_a_bare_function_key() { + let mut bound = Vec::new(); + for (action, spec) in default_bindings() { + for chord in spec.split_whitespace() { + let ks = Keystroke::parse(chord).expect("default chords parse"); + if crate::terminal::input::is_function_key(&ks.key) { + bound.push((action, chord)); + } + } + } + let expected: &[(&str, &str)] = if cfg!(target_os = "macos") { + &[] + } else { + &[ + ("FindNext", "f3"), + ("FindPrevious", "shift-f3"), + ("ToggleFullscreen", "f11"), + ] + }; + bound.sort(); + let mut expected = expected.to_vec(); + expected.sort(); + assert_eq!( + bound, expected, + "a default on a function key hides it from the shell; \ + PSReadLine wants F3, F7 and F8, readline's `bind -x` any of them" + ); + } + #[test] fn the_control_code_rule_knows_what_the_shell_needs() { // The keys with a C0 byte behind them, and the modifier shape that From 8a4f08e368af6a97a87e15933347042ac6afcf43 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 10 Sep 2026 11:28:04 +0800 Subject: [PATCH 09/15] fix(ui): remember the focused pane as focus arrives, not at switch time (#843) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A tab's `last_focused` was written by one sample taken on the way out of `activate`, and that sample asks which leaf holds focus at that instant. By then focus is routinely somewhere else: the switcher's own search input, a palette that just closed, the tab strip, the file tree `activate` focuses itself, or a pane restored and never clicked. With no leaf focused the write was skipped, the field kept a stale pane or the `None` it was born with, and `focus_target` fell through `leaf_matching_or_first` to the tab's first leaf. Two panes, click the right one, switch away and back, land in the left one. `watch_pane_focus` already subscribed to every pane's focus-in — it existed to repaint the chrome that marks the focused pane — so the record now rides on that callback instead of a new one. Focus-in is the only moment that knows the answer without having to guess when to look, and the subscription's cost and lifetime were already paid for. Which tab gets the write is asked of the layout rather than assumed to be the active one, so a pane dragged into another tab is remembered by the tab holding it now; a leaf no tab holds is recorded nowhere. Connecting slots are watched too, since a pane can be focused while it is still coming up. `remember_active_pane` stays. It never writes a wrong answer, only sometimes none, and its callers want the field settled at a named moment — before a pane is detached, before a tab is torn down — while the layout can still answer. Deliberately left alone: a restored tab still opens on its first leaf until someone touches a pane. That is a different hole — persistence, not a focus race — and closing it means giving a leaf an identity that survives a restart. `SessionTab` has none, and an ordinal into the tree is not one either, because `session_to_pane` drops leaves that cannot be respawned and shifts every index after them. The report's third suggestion, preferring a most-recently-active leaf recorded elsewhere, was checked and dropped: `last_used` exists on `Tab` for the switcher's MRU column and there is no per-leaf equivalent anywhere. No timestamp was invented to create one, and with focus-in authoritative there is nothing left for it to break a tie about. Claude-Session: https://claude.ai/code/session_01UUyWQXzcBAoBzaSX8pc7nU --- src/ui/app.rs | 242 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 236 insertions(+), 6 deletions(-) diff --git a/src/ui/app.rs b/src/ui/app.rs index d2f043e9..a0c40a49 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -3455,6 +3455,16 @@ impl Tty7App { } } + /// Sample which pane holds focus right now and record it against the + /// active tab. + /// + /// A sample only ever writes the truth, but it can only write it when + /// there is one to read: with focus one handle off the panes it finds no + /// leaf and leaves the field alone. That is why it is no longer the only + /// writer (see [`Tty7App::remember_focused_leaf`]) — it stays because the + /// callers below want the answer settled at a named moment, before a pane + /// is detached or a tab is torn down and the layout stops being able to + /// answer at all. pub(crate) fn remember_active_pane(&mut self, window: &Window, cx: &App) { let active = self.active; if let Some(tab) = self.tabs.get_mut(active) { @@ -3464,6 +3474,24 @@ impl Tty7App { } } + /// Record `leaf` as the pane its tab comes back to, as focus arrives in it. + /// + /// Sampling at switch time asks which leaf holds focus *at that instant*, + /// and by then focus is routinely somewhere else: the switcher's own + /// search input, a palette that just closed, the tab strip, a pane + /// restored and never clicked. The sample then wrote nothing and the tab + /// kept a stale pane — or the `None` it was born with — and came back to + /// its first leaf instead of the one the reader was working in (#843). + /// + /// Focus-in is the one moment that knows the answer without having to + /// guess when to look, so it is the primary writer now. The tab is found + /// by the leaf rather than assumed to be the active one: a pane dragged + /// into another tab is focused after the move, and it is the tab holding + /// it now that has to remember it. + pub(crate) fn remember_focused_leaf(&mut self, leaf: gpui::EntityId) { + remember_leaf_in(&mut self.tabs, leaf); + } + fn focus_leaf(&self, leaf: &PaneSlot, window: &mut Window, cx: &mut App) { let handle = leaf.focus_handle(cx); window.focus(&handle, cx); @@ -8525,6 +8553,8 @@ pub(crate) fn new_terminal( }, ) .detach(); + let handle = pending.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, pending.entity_id(), window, cx); start_pane_spawn(pending.clone(), window, cx); Ok(PaneSlot::Connecting(pending)) } @@ -8590,7 +8620,8 @@ fn build_terminal_view( ) .detach(); watch_open_file_requests(&view, window, cx); - watch_pane_focus(&view, window, cx); + let handle = view.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, view.entity_id(), window, cx); view } @@ -8618,13 +8649,42 @@ fn watch_open_file_requests( .detach(); } -fn watch_pane_focus(view: &Entity, window: &mut Window, cx: &mut Context) { - let handle = view.read(cx).focus_handle.clone(); +/// Record `leaf` as the pane the tab holding it comes back to. +/// +/// Which tab that is gets asked of the layout rather than assumed to be the +/// active one: a pane dragged into another tab takes focus with it, and it is +/// the tab holding it now whose memory the arrival should change. A leaf no +/// tab holds — one that has just closed, or arrived after its slot went away — +/// is recorded nowhere. +fn remember_leaf_in(tabs: &mut [Tab], leaf: gpui::EntityId) { + let held = tabs + .iter_mut() + .find(|tab| tab.pane.leaves().iter().any(|l| l.entity_id() == leaf)); + if let Some(tab) = held { + tab.last_focused = Some(leaf); + } +} + +/// Repaint the chrome that marks the focused pane, and record the leaf as the +/// one its tab returns to (#843). +/// +/// Every leaf is watched, connecting slots included: a pane can be focused +/// while it is still coming up, and if the tab is left in that moment the +/// answer has to already be written down. +fn watch_pane_focus( + handle: &gpui::FocusHandle, + leaf: gpui::EntityId, + window: &mut Window, + cx: &mut Context, +) { let app = cx.weak_entity(); window - .on_focus_in(&handle, cx, move |_window, cx| { + .on_focus_in(handle, cx, move |_window, cx| { if let Some(app) = app.upgrade() { - app.update(cx, |_, cx| cx.notify()); + app.update(cx, |app, cx| { + app.remember_focused_leaf(leaf); + cx.notify(); + }); } }) .detach(); @@ -8656,7 +8716,8 @@ pub(crate) fn new_terminal_native( ) .detach(); watch_open_file_requests(&view, window, cx); - watch_pane_focus(&view, window, cx); + let handle = view.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, view.entity_id(), window, cx); Ok(view) } @@ -11060,3 +11121,172 @@ mod close_window_action_tests { ); } } + +/// #843: which pane a tab comes back to. +#[cfg(test)] +mod tab_focus_memory_tests { + use super::{Pane, PaneSlot, Tab, remember_leaf_in}; + use crate::ui::pending_pane::{PendingPane, PendingSpawn}; + use gpui::{ + AppContext as _, Axis, Context, Entity, IntoElement, Render, Styled as _, TestAppContext, + Window, div, + }; + + /// A window has to exist for focus to live in, but nothing this file asks + /// is about what a pane paints. + struct Blank; + impl Render for Blank { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div().size_full() + } + } + + /// A leaf that owns a real focus handle without owning a shell. Focus + /// tracking asks the slot, not the terminal behind it, so a connecting + /// pane answers every question here exactly as a running one would — and + /// connecting panes are watched for focus now too, so this is not a + /// stand-in for the case under test but one of its cases. + fn leaf(cx: &mut Context) -> Entity { + cx.new(|cx| { + PendingPane::new( + "test", + PendingSpawn { + workspace: None, + working_directory: None, + restore_pane: None, + shell: None, + agent: None, + agent_session_id: None, + agent_launch_argv: None, + owner: None, + font_size: 14., + }, + cx, + ) + }) + } + + fn two_pane_tab(a: &Entity, b: &Entity) -> Tab { + Tab::new(Pane::split_node( + Axis::Horizontal, + 0.5, + Pane::Leaf(PaneSlot::Connecting(a.clone())), + Pane::Leaf(PaneSlot::Connecting(b.clone())), + )) + } + + fn one_pane_tab(a: &Entity) -> Tab { + Tab::new(Pane::Leaf(PaneSlot::Connecting(a.clone()))) + } + + /// The mechanism behind the report: `remember_active_pane`'s sample asks + /// which leaf holds focus *at that instant*, and a switch begun with focus + /// one handle away — the switcher's own search input, a palette closing, + /// the tab strip — finds no leaf and has nothing to write. This is why the + /// sample cannot be the only writer, and it is pinned here so that a change + /// making `focused_leaf` tolerant would have to argue with a test rather + /// than silently make this fix look unnecessary. + #[gpui::test] + fn a_switch_begun_off_the_panes_samples_nothing(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b, elsewhere) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), cx.focus_handle())) + .unwrap(); + let tab = two_pane_tab(&a, &b); + + window + .update(cx, |_, window, cx| { + let on_b = b.read(cx).focus_handle.clone(); + window.focus(&on_b, cx); + assert_eq!( + tab.pane.focused_leaf(window, cx).map(|l| l.entity_id()), + Some(b.entity_id()), + "with focus in the pane the sample would have found it" + ); + + window.focus(&elsewhere, cx); + assert!( + tab.pane.focused_leaf(window, cx).is_none(), + "one handle off the pane and the switch-away sample has \ + nothing to write" + ); + }) + .unwrap(); + } + + /// So focus-in writes instead, and the tab comes back to the pane focus + /// was last in even though it had wandered off the panes before the switch + /// ever started. + #[gpui::test] + fn a_tab_comes_back_to_the_pane_focus_was_last_in(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b) = window.update(cx, |_, _, cx| (leaf(cx), leaf(cx))).unwrap(); + let mut tabs = vec![two_pane_tab(&a, &b)]; + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(a.entity_id()), + "a tab nobody has worked in yet still opens on its first leaf" + ); + + // The reader clicks into the right-hand pane: focus arrives, and that + // is the moment the tab is told. + remember_leaf_in(&mut tabs, b.entity_id()); + + // Focus then leaves the panes — the switcher opens, a palette closes — + // and the tab is switched away from. `remember_active_pane` finds no + // focused leaf and writes nothing, which is now harmless. + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(b.entity_id()), + "#843: the tab has to come back to the pane the reader was in" + ); + } + + /// A pane dragged into another tab is focused where it lands, so the + /// arrival has to change that tab's memory and not the one it left — the + /// reason the tab is found by the leaf rather than taken to be the active + /// one. + #[gpui::test] + fn a_moved_pane_is_remembered_by_the_tab_that_holds_it_now(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b, c) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), leaf(cx))) + .unwrap(); + // Tab 0 is the active one and holds `a`; `b` and `c` live in tab 1. + let mut tabs = vec![one_pane_tab(&a), two_pane_tab(&b, &c)]; + + remember_leaf_in(&mut tabs, c.entity_id()); + + assert_eq!( + tabs[1].focus_target().map(|l| l.entity_id()), + Some(c.entity_id()), + "the tab holding the focused pane is the one that remembers it" + ); + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(a.entity_id()), + "and no other tab's memory is touched" + ); + } + + /// A pane that arrives after its slot has gone — a spawn landing on a + /// closed tab, a leaf killed mid-flight — belongs to no tab, and must not + /// leave a memory behind for the first tab that happens to be looked at. + #[gpui::test] + fn a_leaf_no_tab_holds_is_recorded_nowhere(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, b, gone) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), leaf(cx))) + .unwrap(); + let mut tabs = vec![two_pane_tab(&a, &b)]; + + remember_leaf_in(&mut tabs, b.entity_id()); + remember_leaf_in(&mut tabs, gone.entity_id()); + + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(b.entity_id()), + "a stranger's arrival leaves the tab's own answer alone" + ); + } +} From a249379bf8878a7c8cabdf6d40eb033c7b096495 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 10 Sep 2026 13:57:25 +0800 Subject: [PATCH 10/15] fix(machine): format the three items and stop reading the tree per persist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rustfmt` is a required check and was red on three hunks of this branch: `owner_of`'s signature, the `--cwd` argv in `tab_new`'s adopt test, and the two `tab_create` calls in `the_document_being_replaced_is_kept_beside_it`. `keep_a_generation` also read the whole document before asking whether it was going to keep anything. It runs on every persist — pane facts alone flush every couple of seconds — and answers "too soon" on almost all of them, so that was a full read of `machine.json` per write to produce one copy every five minutes. The spacing check moves ahead of the read; the `NotFound` arm still covers the machine that has never written a tree. And a line continuation was missing from an assertion message in `a_typed_name_waits_for_the_create_rather_than_racing_it`, so the failure would have printed eighteen spaces mid-sentence. Claude-Session: https://claude.ai/code/session_01JRqYZ9E153WpSHGS2AW3BM --- crates/tty7-cli/src/commands.rs | 9 +++++++-- crates/tty7-core/src/core/machine.rs | 23 +++++++++++++++-------- src/ui/tree_sync.rs | 3 ++- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/crates/tty7-cli/src/commands.rs b/crates/tty7-cli/src/commands.rs index 10b88fc3..ce9b8d04 100644 --- a/crates/tty7-cli/src/commands.rs +++ b/crates/tty7-cli/src/commands.rs @@ -772,7 +772,10 @@ fn adopt_pane( } /// The workspace a pane was spawned for, if it is still on this machine. -fn owner_of(info: &tty7_core::daemon::protocol::PaneInfo, machine: &Machine) -> Option { +fn owner_of( + info: &tty7_core::daemon::protocol::PaneInfo, + machine: &Machine, +) -> Option { let owner = info.owner.as_deref()?; machine .workspaces @@ -2201,7 +2204,9 @@ mod tests { .push_back(ReplyOk::TabTree(Box::new(Tab::leaf(37)))); run_cli( - &["tty7", "tab", "new", "web", "--pane", "%37", "--cwd", "C:\\else"], + &[ + "tty7", "tab", "new", "web", "--pane", "%37", "--cwd", "C:\\else", + ], &Context::default(), &mut backend, ); diff --git a/crates/tty7-core/src/core/machine.rs b/crates/tty7-core/src/core/machine.rs index 5479709c..2a0fdb29 100644 --- a/crates/tty7-core/src/core/machine.rs +++ b/crates/tty7-core/src/core/machine.rs @@ -1304,12 +1304,9 @@ fn backup_path(path: &Path, generation: usize) -> PathBuf { /// Returns `Ok(())` when there was nothing to do: no tree yet, or the newest /// generation is younger than `spacing`. fn keep_a_generation(path: &Path, spacing: Duration) -> io::Result<()> { - let bytes = match std::fs::read(path) { - Ok(bytes) => bytes, - // Nothing has been written yet, so there is no previous generation. - Err(e) if e.kind() == io::ErrorKind::NotFound => return Ok(()), - Err(e) => return Err(e), - }; + // Asked before the tree is read: this runs on every persist and answers + // "nothing to do" on almost all of them, so reading the whole document + // first would be a full read per write for one copy every five minutes. let newest = backup_path(path, 0); let too_soon = std::fs::metadata(&newest) .and_then(|meta| meta.modified()) @@ -1318,6 +1315,12 @@ fn keep_a_generation(path: &Path, spacing: Duration) -> io::Result<()> { if too_soon { return Ok(()); } + let bytes = match std::fs::read(path) { + Ok(bytes) => bytes, + // Nothing has been written yet, so there is no previous generation. + Err(e) if e.kind() == io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(e), + }; // Oldest first, so nothing is overwritten before it has been moved down. // A generation that is not there yet simply has nothing to move. for generation in (1..BACKUP_GENERATIONS).rev() { @@ -2701,8 +2704,12 @@ mod tests { let store = MachineStore::open(&path); let ws = store.workspace_create(None, None, None).unwrap(); - store.tab_create(ws.id, None, seed(1, "/work"), None, None).unwrap(); - store.tab_create(ws.id, None, seed(2, "/work"), None, None).unwrap(); + store + .tab_create(ws.id, None, seed(1, "/work"), None, None) + .unwrap(); + store + .tab_create(ws.id, None, seed(2, "/work"), None, None) + .unwrap(); let kept = backup_path(&path, 0); assert!( diff --git a/src/ui/tree_sync.rs b/src/ui/tree_sync.rs index 524e1b60..be7f0ef3 100644 --- a/src/ui/tree_sync.rs +++ b/src/ui/tree_sync.rs @@ -2909,7 +2909,8 @@ mod tests { assert_eq!(parked.name, "deploy"); assert_eq!( parked.workspace, ws, - "and it is parked against the workspace it was typed for, so a window that walks into a different one cannot spend it (#716)" + "and it is parked against the workspace it was typed for, so a window \ + that walks into a different one cannot spend it (#716)" ); }); } From e7145eabd89ad5b205440b177999d2e38f64e5cb Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:00:41 +0800 Subject: [PATCH 11/15] fix(terminal): give the kitty protocol its own F3 and F13..F24 The shared table sent F3 as `SS3 R` / `CSI 1;R` to a client that had negotiated the kitty keyboard protocol. Kitty's first version allowed that letter form and then removed it: `CSI 1;2R` is also a Cursor Position Report for row 1, column 2, so a client cannot tell Shift+F3 from an answer to its own DSR. The protocol's table gives F3 as `CSI 13~` alone -- the VT220 `kf3` -- and alacritty special-cases exactly this. Only the kitty path moves; everyone reading terminfo still gets the `kf3=\EOR` we advertise. F13..F24 were left unencodable on both paths for a terminfo reason that holds only on the legacy one: `kf13` onwards are already the modified F1..F8, so there is nothing to send. Kitty has no such clash -- it names them in the private use area, `CSI 57376 u` for F13 -- so a client that asked for the protocol now gets them, and the legacy path still sends nothing. Claude-Session: https://claude.ai/code/session_01JRqYZ9E153WpSHGS2AW3BM --- src/terminal/input.rs | 137 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 126 insertions(+), 11 deletions(-) diff --git a/src/terminal/input.rs b/src/terminal/input.rs index e198bc77..03ec4344 100644 --- a/src/terminal/input.rs +++ b/src/terminal/input.rs @@ -142,10 +142,31 @@ fn encode_kitty(ks: &gpui::Keystroke, kitty: KeyFlags) -> Option> { return Some(csi_u(code, mods, None)); } + // F3 is the one function key the kitty protocol does not share with + // terminfo. Its first version allowed both `CSI R` and `CSI 13~`, then + // dropped the letter form outright: `CSI 1;2R` is also a Cursor Position + // Report for row 1, column 2, so a client that negotiated the protocol + // cannot tell Shift+F3 from an answer to its own DSR. The table gives F3 + // as `CSI 13~` alone -- the VT220 `kf3` -- so that is what an app that + // asked for the protocol is told, while the legacy path below keeps the + // `\EOR` our `$TERM` spells. + if ks.key.as_str() == "f3" { + let s = if mods == 1 { + "\x1b[13~".to_string() + } else { + format!("\x1b[13;{mods}~") + }; + return Some(s.into_bytes()); + } + if let Some(seq) = functional_key(ks.key.as_str(), mods, kitty.app_cursor()) { return Some(seq); } + if let Some(code) = kitty_function_key(ks.key.as_str()) { + return Some(csi_u(code, mods, None)); + } + let modified = m.control || m.alt; if modified || kitty.report_all_keys { if let Some(code) = text_key_code(ks) { @@ -250,6 +271,8 @@ enum FunctionKey { /// A physical F13 therefore has no encoding of its own under this `$TERM`; /// sending the VT220 `\E[25~` for it would hand ncurses a sequence its own /// table reads back as Shift+F1, which is worse than sending nothing. +/// `kitty_function_key` picks them up for the one protocol that *can* name +/// them without that clash. fn function_key(key: &str) -> Option { Some(match key { "f1" => FunctionKey::Ss3('P'), @@ -268,11 +291,24 @@ fn function_key(key: &str) -> Option { }) } -/// Whether a key name is one `functional_key` turns into a function-key -/// sequence. The inline editor asks this to know a keystroke it holds no -/// meaning for but the shell does. +/// `f13`..`f24`, encodable only once the kitty protocol is negotiated. Kitty +/// gives them codepoints of its own in the private use area — `CSI 57376 u` is +/// F13, up to `CSI 57387 u` for F24 — so the ambiguity that stops +/// `function_key` at F12 does not arise: nothing else in that protocol spells +/// 57376. An app that never asked for the protocol still gets nothing, because +/// there is nothing in `xterm-256color` to send it. +fn kitty_function_key(key: &str) -> Option { + let n: u32 = key.strip_prefix('f')?.parse().ok()?; + (13..=24).contains(&n).then(|| 57376 + (n - 13)) +} + +/// Whether a key name is one of the function keys — the range that means +/// nothing to a text editor and everything to a shell. The inline editor asks +/// this to know a keystroke it holds no meaning for but the shell does; it +/// still only hands over what actually encodes, which for F13 and up is the +/// kitty path alone. pub(crate) fn is_function_key(key: &str) -> bool { - function_key(key).is_some() + function_key(key).is_some() || kitty_function_key(key).is_some() } fn text_key_code(ks: &gpui::Keystroke) -> Option { @@ -829,23 +865,100 @@ mod tests { /// The `mods` parameter is what makes F13 onwards ambiguous: in /// `xterm-256color` those capability names are already spoken for by the /// modified F1..F8, so a physical F13 has no sequence of its own to send. + /// + /// The kitty protocol has no such clash — it puts F13..F24 in the private + /// use area, `CSI 57376 u` upwards — so a client that negotiated it does + /// get those keys, and only those twelve: F25 is past the end of the range + /// gpui names. #[test] - fn function_keys_stop_at_f12() { + fn terminfo_stops_at_f12_and_kitty_carries_on_to_f24() { let none = Modifiers::default(); + let shift = Modifiers { + shift: true, + ..Default::default() + }; let ctrl = Modifiers { control: true, ..Default::default() }; - for key in ["f13", "f14", "f20", "f24"] { - assert_eq!(legacy(&ks(none, key, None)), None, "{key} is unencodable"); + let kitty_cases: &[(&str, &[u8])] = &[ + ("f13", b"\x1b[57376u"), + ("f14", b"\x1b[57377u"), + ("f20", b"\x1b[57383u"), + ("f24", b"\x1b[57387u"), + ]; + for (key, seq) in kitty_cases { assert_eq!( - keystroke_to_bytes(&ks(none, key, None), kitty()), + legacy(&ks(none, key, None)), None, - "{key} is unencodable under kitty too" + "{key} has no terminfo capability" + ); + assert_eq!( + keystroke_to_bytes(&ks(none, key, None), kitty()).as_deref(), + Some(*seq), + "{key} under kitty" ); } + assert_eq!( + keystroke_to_bytes(&ks(shift, "f13", None), kitty()), + Some(b"\x1b[57376;2u".to_vec()) + ); // Ctrl+F13 must not fold into a C0 byte on its first letter either. assert_eq!(legacy(&ks(ctrl, "f13", None)), None); + // And the range ends where gpui's names do. + assert_eq!(keystroke_to_bytes(&ks(none, "f25", None), kitty()), None); + assert_eq!(legacy(&ks(none, "f25", None)), None); + } + + /// The one function key where the two protocols disagree. Kitty's spec + /// allowed `CSI R` for F3 in its first version and then removed it, + /// because `CSI 1;2R` is also a Cursor Position Report for row 1, column + /// 2 — so a client that negotiated the protocol is given `CSI 13~`, the + /// VT220 `kf3`, while `$TERM`'s own `kf3=\EOR` still goes to everyone + /// else. alacritty draws the same line. + #[test] + fn kitty_spells_f3_thirteen_because_csi_r_is_a_cursor_report() { + let none = Modifiers::default(); + let shift = Modifiers { + shift: true, + ..Default::default() + }; + let ctrl = Modifiers { + control: true, + ..Default::default() + }; + assert_eq!(legacy(&ks(none, "f3", None)), Some(b"\x1bOR".to_vec())); + assert_eq!(legacy(&ks(shift, "f3", None)), Some(b"\x1b[1;2R".to_vec())); + let full = KeyFlags { + disambiguate: true, + report_all_keys: true, + report_text: true, + app_cursor: false, + }; + for flags in [kitty(), full] { + assert_eq!( + keystroke_to_bytes(&ks(none, "f3", None), flags), + Some(b"\x1b[13~".to_vec()) + ); + assert_eq!( + keystroke_to_bytes(&ks(shift, "f3", None), flags), + Some(b"\x1b[13;2~".to_vec()) + ); + assert_eq!( + keystroke_to_bytes(&ks(ctrl, "f3", None), flags), + Some(b"\x1b[13;5~".to_vec()) + ); + } + // Cmd is not a kitty modifier either, and the platform key sends the + // keystroke down the legacy path in the first place. + let cmd = Modifiers { + platform: true, + ..Default::default() + }; + assert_eq!( + keystroke_to_bytes(&ks(cmd, "f3", None), kitty()), + Some(b"\x1bOR".to_vec()) + ); } /// Cmd is not an xterm modifier, so it must not turn a bare F-key into a @@ -1181,7 +1294,9 @@ mod tests { /// The kitty encoder shares `functional_key`, so the F keys have to come /// out of it byte-identical to the legacy path — kitty's own spec keeps /// the legacy CSI/SS3 forms for F1..F12 and only appends the modifier - /// parameter, which is what that shared table already does. + /// parameter, which is what that shared table already does. F3 is the sole + /// exception and has a test of its own: + /// `kitty_spells_f3_thirteen_because_csi_r_is_a_cursor_report`. /// /// `report_all_keys` changes nothing here either: the F keys are already /// escape sequences, so there is no bare byte for it to promote. @@ -1202,7 +1317,7 @@ mod tests { report_text: true, app_cursor: false, }; - for key in ["f1", "f2", "f3", "f4", "f5", "f7", "f10", "f11", "f12"] { + for key in ["f1", "f2", "f4", "f5", "f7", "f10", "f11", "f12"] { for mods in [none, shift, ctrl] { let want = legacy(&ks(mods, key, None)); assert!(want.is_some(), "{key} encodes on the legacy path"); From 16b663a407ab23cce66d5ec5f7245000ff518223 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:02:08 +0800 Subject: [PATCH 12/15] fix(procinfo): report a Windows table that would not answer, and stop crying wolf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things the new probe verdict still got wrong. `GetExtendedTcpTable` refusing came back as an empty buffer, which read exactly like a family with no listeners, so the Windows branch answered `Ok` about ports it had never looked for — the silence #731 is about, on the platform the panel was written on. `tcp_table` says `None` now, and only "neither family answered" is `Unavailable`: a machine with IPv6 off keeps its `Ok` and its IPv4 ports. The `Unavailable` warning was written on every `QueryProcs`, and the Info panel sends one every two seconds — thirty identical lines a minute into a log that truncates itself at 4 MiB, which costs a reporter the rest of the session they turned logging on to capture. Same reason, once a minute. And `Restricted` on Linux took `/proc/`'s owner for the process's uid. The kernel hands that directory to root whenever it clears a process's dumpable attribute, which is what executing a set-user-ID binary or one carrying file capabilities does, so a plain `ping` in a pane had the panel apologising for sockets it can read perfectly well — the opposite mistake, and just as wrong. The `Uid:` line of `/proc//status` settles the few rows that look foreign and are in a pane's tree, so an ordinary pane pays nothing for it. Claude-Session: https://claude.ai/code/session_01JRqYZ9E153WpSHGS2AW3BM --- crates/tty7-core/src/daemon/procinfo.rs | 256 +++++++++++++++++++++--- 1 file changed, 227 insertions(+), 29 deletions(-) diff --git a/crates/tty7-core/src/daemon/procinfo.rs b/crates/tty7-core/src/daemon/procinfo.rs index c937ec37..04897d8e 100644 --- a/crates/tty7-core/src/daemon/procinfo.rs +++ b/crates/tty7-core/src/daemon/procinfo.rs @@ -29,11 +29,52 @@ pub fn snapshot(shell_pid: u32, fg_pgid: Option) -> PaneProcs { false => probe, }; if let PortProbe::Unavailable(detail) = &probe { - log::warn!("listening-port probe failed for pane shell {shell_pid}: {detail}"); + note_probe_failure(shell_pid, detail); } finish(procs, ports, probe) } +/// How long the same probe failure waits before it is written down again. +/// +/// `snapshot` answers one `QueryProcs`, and the Info panel sends one every two +/// seconds for as long as it is open — a probe that cannot run now will not +/// have started working two seconds later. Logging every attempt turns one +/// standing fact into thirty lines a minute in a file that truncates itself at +/// 4 MiB, which costs the reporter the rest of the session they turned logging +/// on to capture. Once a minute still leaves a trail for a failure that +/// outlives the panel. +const PROBE_LOG_GAP: std::time::Duration = std::time::Duration::from_secs(60); + +fn note_probe_failure(shell_pid: u32, detail: &str) { + static LAST: std::sync::Mutex> = + std::sync::Mutex::new(None); + let line = format!("listening-port probe failed for pane shell {shell_pid}: {detail}"); + let Ok(mut last) = LAST.lock() else { return }; + if probe_log_due(&mut last, &line, std::time::Instant::now(), PROBE_LOG_GAP) { + log::warn!("{line}"); + } +} + +/// Whether `line` is worth a log entry now, given what was written last. +/// +/// A line that has not been said before is always worth saying — a probe that +/// starts failing for a second reason, or a second pane failing for the same +/// one, is news. Repeating one is worth it only once per `gap`. +fn probe_log_due( + last: &mut Option<(String, std::time::Instant)>, + line: &str, + now: std::time::Instant, + gap: std::time::Duration, +) -> bool { + if let Some((said, at)) = last.as_ref() { + if said == line && now.duration_since(*at) < gap { + return false; + } + } + *last = Some((line.to_string(), now)); + true +} + /// The answer as the panel gets it: the process list trimmed to what a sidebar /// can show, and the ports left whole. /// @@ -64,9 +105,58 @@ fn tree_has_foreign_uid(table: &HashMap, procs: &[ProcEntry], me: u32) if me == 0 { return false; } - procs - .iter() - .any(|p| table.get(&p.pid).is_some_and(|row| row.uid != me)) + procs.iter().any(|p| { + table + .get(&p.pid) + .is_some_and(|row| row.uid != me && confirm_foreign(p.pid, me)) + }) +} + +/// A second opinion on a row that looks like another user's. +/// +/// Asked only about the pane's own tree, and only about the rows that already +/// look foreign, so an ordinary pane pays nothing for it and a `sudo` pays one +/// file read. +/// +/// Linux needs it because the owner of `/proc/` is not always the process's +/// uid: the kernel makes that directory `root:root` whenever a process's +/// dumpable attribute has been cleared, which is what executing a set-user-ID +/// binary or one carrying file capabilities does. A plain `ping` in a pane is +/// the user's own process behind a root-owned `/proc` entry, and taking the +/// directory's word for it would have the panel apologise for sockets it can +/// read perfectly well — the opposite mistake to the one this file is fixing, +/// and just as wrong. `Uid:` in `/proc//status` is the real answer and is +/// readable either way. +#[cfg(target_os = "linux")] +fn confirm_foreign(pid: u32, me: u32) -> bool { + match std::fs::read_to_string(format!("/proc/{pid}/status")) { + Ok(text) => status_euid(&text).is_none_or(|uid| uid != me), + // Gone, or unreadable: keep the directory's verdict rather than + // inventing a second one out of a failed read. + Err(_) => true, + } +} + +/// macOS reads the effective uid straight out of `PROC_PIDTBSDINFO`, and +/// Windows has no uid to be wrong about, so there is nothing to confirm. +#[cfg(not(target_os = "linux"))] +fn confirm_foreign(_pid: u32, _me: u32) -> bool { + true +} + +/// The effective uid on the `Uid:` line of a `/proc//status`, which reads +/// `Uid:\t\t\t\t`. +/// +/// The effective one is the second: it is the credential the kernel checks when +/// something asks to read the process's sockets. +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +fn status_euid(text: &str) -> Option { + text.lines() + .find_map(|line| line.strip_prefix("Uid:"))? + .split_whitespace() + .nth(1)? + .parse() + .ok() } #[cfg(unix)] @@ -88,7 +178,9 @@ struct Row { ppid: u32, pgid: u32, /// The effective uid of the process, which is what decides whether this - /// daemon may look at its sockets. 0 on platforms that have no such thing. + /// daemon may look at its sockets. 0 on platforms that have no such thing, + /// and on Linux the cheapest reading of it rather than the last word — see + /// `confirm_foreign`. uid: u32, name: String, } @@ -224,11 +316,14 @@ fn process_table() -> HashMap { .rfind('(') .map_or_else(|| String::new(), |open| stat[open + 1..close].to_string()) }); - // `/proc/` is owned by the process's effective uid, which is the - // one that governs who may read its sockets. A stat that fails on a - // pid whose `stat` file just parsed is a race with the process - // exiting; calling that "mine" keeps a dying process from being - // mistaken for another user's. + // `/proc/` is normally owned by the process's effective uid, which + // is the one that governs who may read its sockets. Normally: the + // kernel hands the directory to `root` for a process whose dumpable + // attribute it cleared, so this is a cheap first pass over every pid on + // the machine and `confirm_foreign` settles the few that look foreign + // and are in a pane's tree. A stat that fails on a pid whose `stat` + // file just parsed is a race with the process exiting; calling that + // "mine" keeps a dying process from being mistaken for another user's. let uid = std::fs::metadata(format!("/proc/{pid}")) .map(|m| std::os::unix::fs::MetadataExt::uid(&m)) .unwrap_or_else(|_| current_uid()); @@ -510,9 +605,12 @@ fn parse_lsof(text: &str, procs: &[ProcEntry]) -> Vec { /// seconds while it is open, so this is a fixed handful of microseconds, with /// no process spawn and nothing allocated per pid. /// -/// The probe state is always `Ok` here. There is no tool to be missing and no -/// subprocess to hang: `GetExtendedTcpTable` either answers or the family is -/// skipped, and a machine with IPv6 off still gets its IPv4 ports. +/// There is no tool to be missing here and no subprocess to hang, so the state +/// is `Ok` whenever the kernel answered at all — including for a machine with +/// IPv6 off, which is answered for by its IPv4 table alone. The one case left +/// is a `GetExtendedTcpTable` that would not answer for either family, and that +/// is `Unavailable` for the same reason a missing `lsof` is: nothing looked, so +/// the empty list is not an answer. #[cfg(windows)] fn listening_ports(procs: &[ProcEntry]) -> (Vec, PortProbe) { use std::net::{Ipv4Addr, Ipv6Addr}; @@ -529,12 +627,13 @@ fn listening_ports(procs: &[ProcEntry]) -> (Vec, PortProbe) { let mut ports: Vec = Vec::new(); let v4 = tcp_table(AF_INET); - // SAFETY: `tcp_table` hands back either an empty buffer or one the kernel - // filled with a `MIB_TCPTABLE_OWNER_PID`; the `Vec` gives it the 4-byte - // alignment every field of that struct wants, and `rows` is clamped to what - // the buffer can actually hold before anything is read out of it. + // SAFETY: `tcp_table` hands back a buffer the kernel filled with a + // `MIB_TCPTABLE_OWNER_PID`, or nothing at all; the `Vec` gives it the + // 4-byte alignment every field of that struct wants, and `rows` is clamped + // to what the buffer can actually hold before anything is read out of it. unsafe { - if let Some((rows, count)) = table_rows::(&v4) + if let Some((rows, count)) = + table_rows::(v4.as_deref().unwrap_or(&[])) { for i in 0..count { let row = &*rows.add(i); @@ -559,9 +658,9 @@ fn listening_ports(procs: &[ProcEntry]) -> (Vec, PortProbe) { let v6 = tcp_table(AF_INET6); // SAFETY: as above, for the IPv6 shape of the same table. unsafe { - if let Some((rows, count)) = - table_rows::(&v6) - { + if let Some((rows, count)) = table_rows::( + v6.as_deref().unwrap_or(&[]), + ) { for i in 0..count { let row = &*rows.add(i); let Some(name) = by_pid.get(&row.dwOwningPid) else { @@ -580,7 +679,25 @@ fn listening_ports(procs: &[ProcEntry]) -> (Vec, PortProbe) { } ports.sort_by_key(|e| (e.port, e.pid)); - (ports, PortProbe::Ok) + (ports, windows_probe(v4.is_some(), v6.is_some())) +} + +/// What a Windows probe is worth, given whether each family's table could be +/// read. +/// +/// One family refusing is not a broken probe: a machine with IPv6 switched off +/// is an ordinary machine, and its IPv4 listeners are the whole truth about it. +/// Both refusing is the failure this file is about — nothing was looked at, and +/// an empty list then means "we do not know", which is the one thing #731 says +/// the panel must not spell as "None". +#[cfg(windows)] +fn windows_probe(v4: bool, v6: bool) -> PortProbe { + match v4 || v6 { + true => PortProbe::Ok, + false => PortProbe::Unavailable( + "GetExtendedTcpTable would not answer for either address family".to_string(), + ), + } } /// The two Winsock address families, named here rather than by switching on @@ -592,13 +709,17 @@ const AF_INET: u32 = 2; const AF_INET6: u32 = 23; /// One `GetExtendedTcpTable` snapshot of the listening sockets in `family`, as -/// the raw buffer the kernel filled, or an empty buffer if it would not answer. +/// the raw buffer the kernel filled, or `None` if it would not answer. /// -/// Failure is soft, the way an absent `lsof` is soft on unix: the panel shows no -/// ports rather than an error. A machine with IPv6 disabled takes that path for -/// `AF_INET6` alone and still gets its IPv4 ports. +/// A machine with IPv6 disabled takes the `None` path for `AF_INET6` alone and +/// still gets its IPv4 ports. What `None` must not do is disappear: it used to +/// come back as an empty buffer that read exactly like a family with no +/// listeners, so a table the kernel refused twice — or refused outright, which +/// a filter driver sitting on `iphlpapi` is enough to cause — left the panel +/// saying "None" about ports it never looked for. The caller turns "neither +/// family answered" into `PortProbe::Unavailable` instead. #[cfg(windows)] -fn tcp_table(family: u32) -> Vec { +fn tcp_table(family: u32) -> Option> { use windows_sys::Win32::Foundation::{ERROR_INSUFFICIENT_BUFFER, NO_ERROR}; use windows_sys::Win32::NetworkManagement::IpHelper::{ GetExtendedTcpTable, TCP_TABLE_OWNER_PID_LISTENER, @@ -627,14 +748,14 @@ fn tcp_table(family: u32) -> Vec { ) }; match rc { - NO_ERROR => return buf, + NO_ERROR => return Some(buf), ERROR_INSUFFICIENT_BUFFER => { buf = vec![0u32; (size as usize).div_ceil(std::mem::size_of::()) + 64] } _ => break, } } - Vec::new() + None } /// Where the rows of a `MIB_*TABLE_OWNER_PID` start in `buf`, and how many of @@ -973,6 +1094,78 @@ mod tests { ); } + /// A failure that stands still is one fact, and `snapshot` is asked again + /// every two seconds for as long as the Info panel is open. + #[test] + fn a_standing_probe_failure_is_logged_once_a_minute_not_once_a_poll() { + let mut last = None; + let t0 = std::time::Instant::now(); + let gap = std::time::Duration::from_secs(60); + let line = "shell 100: lsof: program not found"; + assert!( + probe_log_due(&mut last, line, t0, gap), + "the first one talks" + ); + for poll in 1..30u64 { + let now = t0 + std::time::Duration::from_secs(poll * 2); + assert!( + !probe_log_due(&mut last, line, now, gap), + "the same reason again at +{}s", + poll * 2 + ); + } + assert!( + probe_log_due(&mut last, "shell 100: lsof: permission denied", t0, gap), + "a different reason is news even in the same breath" + ); + assert!( + probe_log_due(&mut last, line, t0 + gap, gap), + "and a failure that outlives the gap still leaves a trail" + ); + } + + /// A `/proc/` the kernel handed to root is not evidence of another + /// user: `ping`, and anything else carrying file capabilities, is the + /// caller's own process behind one. + #[test] + fn the_status_files_effective_uid_is_the_one_that_counts() { + let ping = "Name:\tping\nState:\tS (sleeping)\nTgid:\t4242\n\ + Uid:\t501\t501\t501\t501\nGid:\t20\t20\t20\t20\n"; + assert_eq!(status_euid(ping), Some(501)); + + let sudo = "Name:\tmain\nUid:\t501\t0\t0\t0\n"; + assert_eq!( + status_euid(sudo), + Some(0), + "the effective uid, not the real one that typed the password" + ); + + assert_eq!(status_euid("Name:\tzsh\n"), None); + assert_eq!( + status_euid("Uid:\t501\n"), + None, + "a truncated line is no answer" + ); + } + + /// Windows has no tool to be missing, but it does have a kernel call that + /// can refuse, and a refusal used to arrive as an empty table — the same + /// silence #731 is about, on the platform the panel was written on. + #[cfg(windows)] + #[test] + fn a_windows_table_that_would_not_answer_is_not_an_empty_one() { + assert_eq!(windows_probe(true, true), PortProbe::Ok); + assert_eq!( + windows_probe(true, false), + PortProbe::Ok, + "IPv6 switched off is an ordinary machine, not a broken probe" + ); + assert!( + matches!(windows_probe(false, false), PortProbe::Unavailable(_)), + "neither family answered, so the empty list is not an answer" + ); + } + fn describe(run: &std::io::Result) -> String { match run { Ok(Run::Finished(out)) => format!("finished with {}", out.status), @@ -1197,6 +1390,11 @@ mod windows_tests { "the chain must be walked past its root: {:?}", got.procs ); + assert!( + got.probe.is_ok(), + "a kernel that answered has nothing to apologise for: {:?}", + got.probe + ); let found = got .ports .iter() From 8ee9474045b7f1e10d242e408f5be5cb1ff083ff Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:02:13 +0800 Subject: [PATCH 13/15] fix(daemon): leave the ring the modes it still carries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `replay_state` sends the mode fold ahead of the ring so the replayed frames land in the buffer they were drawn for, and it sent every mode that was on — including the ones the ring itself still switches on. `?1049h` is a no-op in the emulator once the mode is already set, so the ring's own copy then stopped clearing the alternate screen: everything the ring holds *ahead* of that sequence, which is the shell scrollback the user had behind the program, was painted into the alternate buffer instead. That buffer keeps no history, so those lines were thrown away, and the primary buffer the program's exit returns the client to was left empty. Reconnecting a minute after opening `vim` is the ordinary case, and it lost the prompt the user left behind. Only the modes a replay of the ring cannot speak for go ahead of it now, and they are read from a fold over the bytes the ring actually still holds — so a sequence the front cut in half counts as lost, exactly as it will for the emulator that reads the same bytes. Claude-Session: https://claude.ai/code/session_01JRqYZ9E153WpSHGS2AW3BM --- crates/tty7-core/src/core/term_modes.rs | 83 +++++++++++++++++++++++-- crates/tty7-core/src/daemon/pane.rs | 81 +++++++++++++++++++++--- 2 files changed, 153 insertions(+), 11 deletions(-) diff --git a/crates/tty7-core/src/core/term_modes.rs b/crates/tty7-core/src/core/term_modes.rs index cc9dea8b..b23763df 100644 --- a/crates/tty7-core/src/core/term_modes.rs +++ b/crates/tty7-core/src/core/term_modes.rs @@ -14,7 +14,8 @@ //! //! So the daemon folds the same bytes into this tracker as they pass, and //! `replay_state` re-sends what is still on ahead of the ring — the same -//! treatment cwd, the prompt state and the agent already get. +//! treatment cwd, the prompt state and the agent already get. Only what the +//! ring itself no longer carries, though: see [`TerminalModes::restore_bytes_beyond`]. //! //! Only modes that change how input is routed or which buffer is on screen are //! tracked. Cursor visibility (`?25`) and autowrap (`?7`) are deliberately left @@ -87,11 +88,40 @@ impl TerminalModes { /// The bytes that put a freshly reset terminal back into these modes, or /// `None` when there is nothing to restore. pub fn restore_bytes(&self) -> Option> { - if self.on.is_empty() { + Self::bytes_for(&self.on) + } + + /// The same, minus every mode `replayed` switches on by itself. + /// + /// `replayed` is a fold over the bytes that are about to be sent after + /// these, and whatever it carries has to be left to it — a mode sequence in + /// a stream does more than set a bit. `?1049h` clears the alternate screen + /// and takes the cursor there, and the emulator makes it a *no-op* once the + /// mode is already on, so restoring such a mode ahead of a replay that + /// still contains it does not harmlessly double up: it paints everything + /// the replay wrote before its own `?1049h` into the alternate screen, + /// which has no scrollback to hold it, and leaves the primary buffer the + /// program's exit returns the client to empty. + /// + /// What is left over is exactly what the replay can no longer speak for, + /// and it is a prefix of `on`: the replay is a suffix of the stream, so any + /// mode it sets was set later than one it does not. + pub fn restore_bytes_beyond(&self, replayed: &TerminalModes) -> Option> { + let missing: Vec = self + .on + .iter() + .copied() + .filter(|mode| !replayed.on.contains(mode)) + .collect(); + Self::bytes_for(&missing) + } + + fn bytes_for(modes: &[u16]) -> Option> { + if modes.is_empty() { return None; } - let mut out = Vec::with_capacity(self.on.len() * 8); - for mode in &self.on { + let mut out = Vec::with_capacity(modes.len() * 8); + for mode in modes { out.extend_from_slice(b"\x1b[?"); out.extend_from_slice(mode.to_string().as_bytes()); out.push(b'h'); @@ -266,6 +296,51 @@ mod tests { assert_eq!(modes.active(), &[1002]); } + #[test] + fn modes_the_replay_still_carries_are_left_to_the_replay() { + let mut modes = TerminalModes::new(); + modes.feed(b"\x1b[?1049h\x1b[?1002h\x1b[?1006h"); + + // A ring that still holds the whole prefix speaks for all three, and + // has to: its own `?1049h` is what clears the alternate screen and + // decides which buffer the bytes around it are painted into. + let mut whole = TerminalModes::new(); + whole.feed(b"\x1b[?1049h\x1b[?1002h\x1b[?1006h"); + assert!(modes.restore_bytes_beyond(&whole).is_none()); + + // One that holds only the tail speaks for the tail; the rest comes back + // ahead of it, in the order the application set it. + let mut tail = TerminalModes::new(); + tail.feed(b"\x1b[?1006h"); + assert_eq!( + modes.restore_bytes_beyond(&tail).unwrap(), + b"\x1b[?1049h\x1b[?1002h".to_vec() + ); + + // And a ring with nothing left of the prefix is the #774 case: all of + // it is re-sent. + assert_eq!( + modes.restore_bytes_beyond(&TerminalModes::new()).unwrap(), + modes.restore_bytes().unwrap() + ); + } + + /// A ring drops from its front mid-sequence, so its first bytes can be the + /// tail of a mode set. The emulator will not act on that, so neither does + /// the fold that stands in for it. + #[test] + fn a_mode_set_the_replay_only_half_carries_is_still_restored() { + let mut modes = TerminalModes::new(); + modes.feed(b"\x1b[?1049h"); + + let mut replayed = TerminalModes::new(); + replayed.feed(b"049h and the rest of the screen"); + assert_eq!( + modes.restore_bytes_beyond(&replayed).unwrap(), + b"\x1b[?1049h".to_vec() + ); + } + #[test] fn a_full_reset_clears_everything() { let mut modes = TerminalModes::new(); diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index 232677a9..5e784300 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -2575,6 +2575,25 @@ impl ReplayRing { } } + /// The modes a replay of this ring switches on by itself — the same fold + /// the pane keeps, over the bytes that are actually left. + /// + /// Folded rather than remembered per mode because the front of the ring cuts + /// wherever the cap fell, possibly through a sequence: the client's emulator + /// will not act on half a `?1049h` either, and the answer here has to be the + /// one the emulator will reach. + fn modes(&self) -> TerminalModes { + let mut modes = TerminalModes::new(); + for seg in &self.segments { + // Both halves of the deque, in order: `feed` carries a sequence + // across calls, so the split is invisible to the fold. + let (a, b) = seg.bytes.as_slices(); + modes.feed(a); + modes.feed(b); + } + modes + } + #[cfg(test)] fn flatten(&self) -> Vec { let mut out = Vec::with_capacity(self.len); @@ -2596,13 +2615,17 @@ fn record_output(st: &mut PaneState, bytes: &[u8]) { fn replay_state(st: &PaneState, subscriber: &Sender) { // Ahead of the ring, not after it: a client that is put into the alternate - // screen first paints the replayed frames into the buffer they belong to, - // and re-entering an alternate screen the ring turns out to still carry is - // a no-op in the emulator, so the two cannot fight. What the ring does - // carry always wins on its own terms — the fold is over every byte the - // pane ever wrote, so any mode the ring still toggles ends where the fold - // says it ends. - if let Some(modes) = st.modes.restore_bytes() { + // screen first paints the replayed frames into the buffer they belong to. + // + // Only the modes the ring cannot switch on itself, though. Re-entering an + // alternate screen the ring still carries is not a harmless duplicate — + // the emulator makes `?1049h` a no-op when the mode is already on, so the + // ring's own copy stops clearing the alternate screen, and everything the + // ring holds from *before* that sequence (the shell scrollback the user + // had behind the program) is painted into the alternate buffer, which has + // no history to keep it and is left behind when the program exits. What + // the ring carries always wins on its own terms. + if let Some(modes) = st.modes.restore_bytes_beyond(&st.ring.modes()) { let _ = subscriber.send(DaemonMsg::Snapshot(modes)); } st.ring.replay(subscriber); @@ -4846,6 +4869,50 @@ mod tests { assert!(rx.try_recv().is_err()); } + /// The other side of the same coin, and the common case: reconnect a minute + /// after opening `vim` and the ring still holds the whole session, prefix + /// included. Re-sending the prefix then would turn the ring's own `?1049h` + /// into a no-op, so the shell scrollback the ring holds ahead of it would be + /// painted into the alternate screen — which keeps no history and is thrown + /// away when the program exits, leaving the user back on a blank primary + /// buffer instead of the prompt they left behind. + #[test] + fn a_prefix_the_ring_still_carries_is_left_to_the_ring() { + let mut st = test_state(true); + record_output(&mut st, b"$ vim notes.md\r\n"); + record_output(&mut st, b"\x1b[?1049h\x1b[?1002h\x1b[?1006hthe file\r\n"); + + let (tx, rx) = mpsc::channel(); + attach_subscriber(&mut st, tx); + + assert!( + matches!(rx.try_recv(), Ok(DaemonMsg::Size(_))), + "the ring speaks for its own modes; nothing goes ahead of it" + ); + assert!(matches!(rx.try_recv(), Ok(DaemonMsg::Snapshot(_)))); + assert!(rx.try_recv().is_err()); + } + + /// A mode the ring half-carries is a mode the ring cannot set: the front + /// cuts wherever the cap fell, and the emulator will not act on the tail of + /// a sequence any more than the fold does. + #[test] + fn a_prefix_the_ring_cut_in_half_is_restored() { + let mut st = test_state(true); + record_output(&mut st, b"\x1b[?1049h"); + // Four bytes over the cap, so the front eats exactly the `ESC [ ? 1` + // the sequence opens with and leaves the rest of it in place. + record_output(&mut st, &vec![b'.'; RING_CAP - 4]); + assert!(st.ring.flatten().starts_with(b"049h")); + + let (tx, rx) = mpsc::channel(); + attach_subscriber(&mut st, tx); + assert!( + matches!(rx.try_recv(), Ok(DaemonMsg::Snapshot(b)) if b == b"\x1b[?1049h"), + "the bytes left in the ring put no client on the alternate screen" + ); + } + /// An observer joins mid-session too, and reads the same pane state. #[test] fn observers_are_told_the_pane_modes_as_well() { From 8c508a5282b898efda93cf17a69495b967400ab8 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:03:37 +0800 Subject: [PATCH 14/15] fix(ssh): hold the note's lock across a restart or a replace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The memo made the connection's `proved_server` slot both the note and the install gate, and `ensure_remote_server` holds it across the probe for a reason the two operations that change the server share: a pane arriving mid-install must wait rather than upload over the file the winner is renaming into place. `restart_remote_daemon` and `replace_remote_server` only cleared the note and then let go, so a pane opening during a replace still probed against a half-moved binary — and now *kept* that answer for the life of the connection, where before the memo it cost that one pane and no other. The sharp end is the mismatch the note re-files on every hit: probe the machine while the replace has uploaded but not yet restarted, and the outgoing daemon is filed as a mismatch that every later pane on that connection hears about, long after the replace the user asked for fixed it. Both now run inside the guard, the note dropped first, the way `wsl::replace_wsl_server` holds its distro's install lock. That makes the hold as long as a replace, so the router's forget on a silently closed link moves off the thread polling the route: it took the same non-reentrant lock, and waiting there kept the client's half of a link that was already gone open for as long as whatever held it. Claude-Session: https://claude.ai/code/session_01JRqYZ9E153WpSHGS2AW3BM --- crates/tty7-core/src/daemon/install/mod.rs | 63 +++++++++++++----- crates/tty7-core/src/daemon/install/tests.rs | 68 ++++++++++++++++++++ crates/tty7-core/src/daemon/router.rs | 10 ++- 3 files changed, 123 insertions(+), 18 deletions(-) diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index 157970bd..8614556e 100644 --- a/crates/tty7-core/src/daemon/install/mod.rs +++ b/crates/tty7-core/src/daemon/install/mod.rs @@ -1461,29 +1461,57 @@ pub fn ensure_remote_server_labeled(conn: &Arc, host: &str) -> io /// Drop what we thought we knew about a connection's server, so that the next /// `ensure_remote_server` on it proves the whole thing again. /// -/// Three callers, and between them they are the memo's correctness argument: -/// [`restart_remote_daemon`] and [`replace_remote_server`], which change which -/// build is serving the machine and therefore what the note claims, and the -/// router, which calls this when a routed link closes without the remote ever -/// sending a byte — the only way a path that has stopped working is found out. -/// A reconnect needs no caller at all: the note lives on the connection, and a -/// new connection has none. +/// The router's call, and between it and [`while_changing_the_server`] they are +/// the memo's correctness argument: this one is made when a routed link closes +/// without the remote ever sending a byte — the only way a path that has stopped +/// working is found out — and that one covers [`restart_remote_daemon`] and +/// [`replace_remote_server`], which change which build is serving the machine +/// and therefore what the note claims. A reconnect needs no caller at all: the +/// note lives on the connection, and a new connection has none. +/// +/// Takes the lock and gives it straight back, so it is safe to call from the +/// reactor — unlike anything that holds it across a probe or a replace. pub fn forget_remote_server(conn: &SshConnection) { *conn.proved_server() = None; } +/// Run something that changes which server is running over there, with the note +/// dropped first and its lock held for the whole operation. +/// +/// Forget first, not afterwards: both callers deliberately change what is +/// running over there, which is most of what the note claims, and one that +/// fails halfway must leave the next pane looking rather than trusting a note +/// written before the upheaval. +/// +/// Held throughout for the same reason `ensure_remote_server` holds it across +/// its probe. A pane opening while a replace is in flight would otherwise probe +/// against a half-moved binary — installing over the very upload this call is +/// renaming into place, or filing the outgoing daemon as the mismatch — and +/// then *keep* that answer for the life of the connection, where before the +/// memo it cost that one pane and no other. Waiting the replace out is what the +/// pane wants anyway: the answer it is asking for is the one this call is in +/// the business of changing. +/// +/// The note is cleared inside the guard rather than by calling +/// [`forget_remote_server`], which wants this same non-reentrant lock. +fn while_changing_the_server( + conn: &SshConnection, + change: impl FnOnce() -> io::Result<()>, +) -> io::Result<()> { + let mut slot = conn.proved_server(); + *slot = None; + change() +} + pub fn restart_remote_daemon(conn: &Arc) -> io::Result<()> { let host = connection_label(conn); let ops = ssh_ops::SshRemoteOps::new(conn.clone()); let fetch = default_fetcher(); let confirm = install_confirm(); - // Forget first, not afterwards: this deliberately changes what is running - // over there, which is most of what the note claims, and a restart that - // fails halfway must leave the next pane looking rather than trusting a - // note written before the upheaval. - forget_remote_server(conn); - Installer::new(&ops, fetch.as_ref(), confirm.as_ref(), host).restart_daemon()?; - Ok(()) + while_changing_the_server(conn, || { + Installer::new(&ops, fetch.as_ref(), confirm.as_ref(), host).restart_daemon()?; + Ok(()) + }) } pub fn replace_remote_server(conn: &Arc) -> io::Result<()> { @@ -1492,9 +1520,10 @@ pub fn replace_remote_server(conn: &Arc) -> io::Result<()> { let fetch = default_fetcher(); let confirm = install_confirm(); let source = BundledOrRelease::discover(fetch.as_ref()); - forget_remote_server(conn); - Installer::with_source(&ops, &source, confirm.as_ref(), host).replace()?; - Ok(()) + while_changing_the_server(conn, || { + Installer::with_source(&ops, &source, confirm.as_ref(), host).replace()?; + Ok(()) + }) } #[cfg(feature = "remote-install")] diff --git a/crates/tty7-core/src/daemon/install/tests.rs b/crates/tty7-core/src/daemon/install/tests.rs index 1422e5bc..765ceaad 100644 --- a/crates/tty7-core/src/daemon/install/tests.rs +++ b/crates/tty7-core/src/daemon/install/tests.rs @@ -2453,4 +2453,72 @@ mod proving_the_server_once_per_connection { "the next pane proves it again the long way" ); } + + /// What `restart_remote_daemon` and `replace_remote_server` do to the note: + /// drop it before they start, so that one which fails halfway leaves the + /// next pane looking instead of trusting a note written before the upheaval. + #[tokio::test] + async fn a_change_that_failed_halfway_leaves_no_note() { + use crate::daemon::ssh::test_support::{Exec, FakeSshd}; + + let sshd = FakeSshd::connect(Exec::Exits, None).await; + *sshd.conn.proved_server() = Some(ProvedServer { + binary: BINARY.to_string(), + mismatch: None, + }); + + let failed = while_changing_the_server(&sshd.conn, || { + Err(io::Error::other("the daemon would not stop")) + }) + .expect_err("the change failed"); + assert!(format!("{failed}").contains("would not stop")); + assert_eq!( + sshd.conn.remembered_server(), + None, + "the note went first, so the next pane proves it again" + ); + } + + /// And they hold the lock while they run: a pane that arrives in the middle + /// of a replace waits for it rather than proving a binary the replace is in + /// the middle of moving — and then keeping that answer for the life of the + /// connection. + #[tokio::test] + async fn a_pane_arriving_mid_change_waits_for_it() { + use crate::daemon::ssh::test_support::{Exec, FakeSshd}; + use std::sync::atomic::{AtomicBool, Ordering}; + + let sshd = FakeSshd::connect(Exec::Exits, None).await; + let proved = Arc::new(AtomicBool::new(false)); + let mut pane = None; + + while_changing_the_server(&sshd.conn, || { + let conn = sshd.conn.clone(); + let raced = proved.clone(); + pane = Some(std::thread::spawn(move || { + *conn.proved_server() = Some(ProvedServer { + binary: "/home/me/.tty7/bin/proved-mid-change".to_string(), + mismatch: None, + }); + raced.store(true, Ordering::SeqCst); + })); + // Long enough for the other thread to reach the lock. It cannot + // pass it, so this can only fail if the lock is not being held. + std::thread::sleep(Duration::from_millis(50)); + assert!( + !proved.load(Ordering::SeqCst), + "a pane must not write a note while the server is being changed" + ); + Ok(()) + }) + .expect("the change itself succeeded"); + + pane.expect("the pane raced") + .join() + .expect("it got through"); + assert!( + proved.load(Ordering::SeqCst), + "and it goes through as soon as the change is done" + ); + } } diff --git a/crates/tty7-core/src/daemon/router.rs b/crates/tty7-core/src/daemon/router.rs index 9585cc85..aa64431b 100644 --- a/crates/tty7-core/src/daemon/router.rs +++ b/crates/tty7-core/src/daemon/router.rs @@ -639,7 +639,15 @@ async fn drive(local: Stream, header: &RouteHeader) -> io::Result<()> { "ssh {}: the routed link closed without answering; proving the server again next time", conn.key().as_str(), ); - crate::daemon::install::forget_remote_server(conn); + // Off this thread, which is the one polling the route: the note's lock + // is also the connection's install gate, so a pane that is mid-probe or + // a replace that is mid-upload holds it for as long as that takes, and + // waiting here would keep the client's half of a link that is already + // gone open for the same span. Landing after whatever holds it is right + // either way — a note written by a probe that started before this link + // failed is exactly as suspect as the one it replaced. + let conn = conn.clone(); + tokio::task::spawn_blocking(move || crate::daemon::install::forget_remote_server(&conn)); } let (to_remote, to_local) = copied?; From eb419128ca02a0084c2db1599fddc171edc2104b Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 10 Sep 2026 14:05:02 +0800 Subject: [PATCH 15/15] fix(ui): carry a tab's focus memory onto the pane its pending slot became MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Watching connecting slots is what lets a pane focused while it is still coming up be recorded, but the record names the *pending* slot, and that id dies the moment `land_pane` swaps the slot for the pane that came up in it. The memory then names an entity no tab holds, `focus_target` falls through `leaf_matching_or_first`, and the tab comes back to its first leaf — the bug this branch is about, one landing later. Nothing else writes the answer down in that window. `land_pane` re-focuses the pane it built only when the pending slot still held focus, and with focus off the panes the switch-away sample has nothing to read either. So: split a pane, let focus wander off the panes while the new one is still connecting, switch away and back, and you land in the pane you did not ask for. `replace_leaf_in` does the swap and carries the memory with it, and `respawn_native_ssh_in_place` — the other place a live pane is substituted for a dead one — uses it instead of repeating the walk. Also pins the wiring this branch changed. The tests here drove `remember_leaf_in` directly and never the subscription that calls it, so deleting the one line that records a focus arrival left all four green. `test_window::harness` makes the round trip reachable — real panes, real gpui focus, a real `activate` there and back — and both new tests fail without the code they are about. Claude-Session: https://claude.ai/code/session_01JRqYZ9E153WpSHGS2AW3BM --- src/ui/app.rs | 152 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 140 insertions(+), 12 deletions(-) diff --git a/src/ui/app.rs b/src/ui/app.rs index a0c40a49..9830f652 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -3543,9 +3543,7 @@ impl Tty7App { view.read(cx).run_command_line(&cmd); } let slot = PaneSlot::Ready(view.clone()); - self.tabs - .iter_mut() - .any(|tab| tab.pane.replace_leaf(slot_id, slot.clone())); + replace_leaf_in(&mut self.tabs, slot_id, slot.clone()); if was_focused { self.focus_leaf(&slot, window, cx); } @@ -3749,14 +3747,11 @@ impl Tty7App { return; } }; - for tab in &mut self.tabs { - if tab - .pane - .replace_leaf(dead.entity_id(), PaneSlot::Ready(fresh.clone())) - { - break; - } - } + replace_leaf_in( + &mut self.tabs, + dead.entity_id(), + PaneSlot::Ready(fresh.clone()), + ); self.maximized = None; self.focus_leaf(&PaneSlot::Ready(fresh), window, cx); self.save_session(cx); @@ -8665,6 +8660,30 @@ fn remember_leaf_in(tabs: &mut [Tab], leaf: gpui::EntityId) { } } +/// Put `new` where the slot `old` named stood, carrying that tab's focus +/// memory across with it. +/// +/// The memory has to move because the id it holds does not survive the swap. +/// Focus arriving in a pane that is still coming up is recorded against the +/// *pending* slot — that is why connecting slots are watched at all — and that +/// slot's id dies the moment the pane lands. Left behind, the memory names an +/// entity no tab holds, `focus_target` falls through `leaf_matching_or_first`, +/// and the tab comes back to its first leaf: #843 again, one landing later. +/// +/// Nothing else writes the answer down in that case. `land_pane` re-focuses +/// the pane it built only when the pending slot still held focus, and with +/// focus off the panes the switch-away sample has nothing to read either. +fn replace_leaf_in(tabs: &mut [Tab], old: gpui::EntityId, new: PaneSlot) { + for tab in tabs.iter_mut() { + if tab.pane.replace_leaf(old, new.clone()) { + if tab.last_focused == Some(old) { + tab.last_focused = Some(new.entity_id()); + } + break; + } + } +} + /// Repaint the chrome that marks the focused pane, and record the leaf as the /// one its tab returns to (#843). /// @@ -11125,7 +11144,7 @@ mod close_window_action_tests { /// #843: which pane a tab comes back to. #[cfg(test)] mod tab_focus_memory_tests { - use super::{Pane, PaneSlot, Tab, remember_leaf_in}; + use super::{Pane, PaneSlot, Tab, remember_leaf_in, replace_leaf_in}; use crate::ui::pending_pane::{PendingPane, PendingSpawn}; use gpui::{ AppContext as _, Axis, Context, Entity, IntoElement, Render, Styled as _, TestAppContext, @@ -11289,4 +11308,113 @@ mod tab_focus_memory_tests { "a stranger's arrival leaves the tab's own answer alone" ); } + + /// A pane focused while it was still coming up is remembered under its + /// *pending* slot, and that id dies the moment the pane lands in its + /// place. The memory has to come along with the swap, or the landing is + /// itself what puts the tab back on its first leaf. + /// + /// What lands here is another slot rather than a running pane: the swap has + /// to move an id from one slot to another, and which kind of slot arrived + /// is no part of the question. + #[gpui::test] + fn a_landing_pane_inherits_what_its_pending_slot_was_told(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| Blank); + let (a, connecting, landed, other) = window + .update(cx, |_, _, cx| (leaf(cx), leaf(cx), leaf(cx), leaf(cx))) + .unwrap(); + let mut tabs = vec![two_pane_tab(&a, &connecting)]; + + // Focus arrives while the pane is still connecting, then the pane it + // was waiting for lands in that slot. + remember_leaf_in(&mut tabs, connecting.entity_id()); + replace_leaf_in( + &mut tabs, + connecting.entity_id(), + PaneSlot::Connecting(landed.clone()), + ); + + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(landed.entity_id()), + "#843: the memory follows the pane, not the slot it arrived in" + ); + + // A landing somewhere else in the tab is not an answer to this + // question and does not touch it. + replace_leaf_in(&mut tabs, a.entity_id(), PaneSlot::Connecting(other)); + assert_eq!( + tabs[0].focus_target().map(|l| l.entity_id()), + Some(landed.entity_id()), + "another pane landing leaves the tab's answer alone" + ); + } + + /// The wiring, end to end. `watch_pane_focus` is the subscription that + /// writes the record, and a real round trip through `activate` has to come + /// back to the pane focus last arrived in — with focus off the panes well + /// before the switch was made, which is the moment the switch-away sample + /// cannot see (#843). + #[gpui::test] + fn a_tab_switch_returns_to_the_pane_focus_arrived_in(cx: &mut TestAppContext) { + use super::{test_window::harness, watch_pane_focus}; + use crate::terminal::view::quiet_test_pane; + + let (app, mut vcx) = harness(cx); + let (left, right, elsewhere, _held) = app.update_in(&mut vcx, |app, window, cx| { + let (left, left_stream) = quiet_test_pane(1, window, cx); + let (right, right_stream) = quiet_test_pane(2, window, cx); + let (only, only_stream) = quiet_test_pane(3, window, cx); + app.tabs.push(Tab::new(Pane::split_node( + Axis::Horizontal, + 0.5, + Pane::leaf(PaneSlot::Ready(left.clone())), + Pane::leaf(PaneSlot::Ready(right.clone())), + ))); + app.tabs.push(Tab::new(Pane::leaf(PaneSlot::Ready(only)))); + app.active = 0; + // The subscription every spawn path registers for the pane it + // built. + for view in [&left, &right] { + let handle = view.read(cx).focus_handle.clone(); + watch_pane_focus(&handle, view.entity_id(), window, cx); + } + cx.notify(); + ( + left, + right, + cx.focus_handle(), + (left_stream, right_stream, only_stream), + ) + }); + vcx.background_executor.run_until_parked(); + + // The reader clicks into the right-hand pane. + app.update_in(&mut vcx, |_, window, cx| { + let handle = right.read(cx).focus_handle.clone(); + handle.focus(window, cx); + }); + vcx.background_executor.run_until_parked(); + + // Focus then leaves the panes altogether — a palette closing, the tab + // strip, the switcher's own search input — before the tab is left. + app.update_in(&mut vcx, |_, window, cx| elsewhere.focus(window, cx)); + vcx.background_executor.run_until_parked(); + + app.update_in(&mut vcx, |app, window, cx| app.activate(1, window, cx)); + vcx.background_executor.run_until_parked(); + app.update_in(&mut vcx, |app, window, cx| app.activate(0, window, cx)); + vcx.background_executor.run_until_parked(); + + app.update_in(&mut vcx, |_, window, cx| { + assert!( + right.read(cx).focus_handle.is_focused(window), + "#843: the tab has to come back to the pane the reader was in" + ); + assert!( + !left.read(cx).focus_handle.is_focused(window), + "and not to the first leaf" + ); + }); + } }