diff --git a/crates/tty7-cli/src/cli.rs b/crates/tty7-cli/src/cli.rs index 0663650d..7cfd0b73 100644 --- a/crates/tty7-cli/src/cli.rs +++ b/crates/tty7-cli/src/cli.rs @@ -433,7 +433,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, @@ -443,6 +443,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")] @@ -768,7 +782,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 53140d84..e251c486 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), @@ -714,12 +716,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, @@ -741,6 +750,86 @@ 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)?; @@ -864,8 +953,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 })) @@ -2275,6 +2365,137 @@ 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(); @@ -3199,6 +3420,7 @@ mod tests { tty7_core::daemon::protocol::PaneProcs { procs: vec![proc_entry(100, "zsh", 0, true)], ports: Vec::new(), + probe: Default::default(), context: Some(local_context(None)), } } @@ -3211,6 +3433,7 @@ mod tests { proc_entry(101, "cargo", 1, true), ], ports: Vec::new(), + probe: Default::default(), context: Some(local_context(None)), } } @@ -3239,6 +3462,7 @@ mod tests { proc_entry(101, "ssh", 1, true), ], ports: Vec::new(), + probe: Default::default(), context: Some(tty7_core::daemon::protocol::PaneContext { remote: Some(RemoteContext { kind: RemoteKind::Ssh, @@ -3259,6 +3483,7 @@ mod tests { tty7_core::daemon::protocol::PaneProcs { procs: Vec::new(), ports: Vec::new(), + probe: Default::default(), context: Some(tty7_core::daemon::protocol::PaneContext { remote: Some(RemoteContext { kind: RemoteKind::NativeSsh, diff --git a/crates/tty7-cli/src/output.rs b/crates/tty7-cli/src/output.rs index 8c657e9c..c44e3219 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, context: None, }; let rendered = procs_tables(&procs); @@ -563,5 +594,55 @@ 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()), + context: None, + }; + 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/core/machine.rs b/crates/tty7-core/src/core/machine.rs index 4e754afa..2a0fdb29 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,99 @@ 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<()> { + // 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()) + .map(|at| at.elapsed().unwrap_or_default() < spacing) + .unwrap_or(false); + 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() { + 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 +2675,123 @@ 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 = 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..b23763df --- /dev/null +++ b/crates/tty7-core/src/core/term_modes.rs @@ -0,0 +1,351 @@ +//! 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 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 +//! 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> { + 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(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'); + } + 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 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(); + modes.feed(b"\x1b[?1049h\x1b[?1006h"); + modes.feed(b"\x1bc"); + assert!(modes.is_empty()); + } +} diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index bc73c4d4..8614556e 100644 --- a/crates/tty7-core/src/daemon/install/mod.rs +++ b/crates/tty7-core/src/daemon/install/mod.rs @@ -1341,37 +1341,166 @@ 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. +/// +/// 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<()> { @@ -1379,8 +1508,10 @@ 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(); - 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<()> { @@ -1389,8 +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()); - 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 dbb59194..765ceaad 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,245 @@ 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" + ); + } + + /// 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/pane.rs b/crates/tty7-core/src/daemon/pane.rs index bc41147b..29a0e542 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, @@ -703,6 +704,12 @@ struct PaneState { /// and will never be superseded. Cleared whenever `remote` changes, so a /// second hop is proved on its own terms. remote_prompt_seen: bool, + /// 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, @@ -1518,6 +1525,7 @@ impl DaemonPane { osc_title: restored_title, shell: ShellState::default(), remote_prompt_seen: false, + modes: TerminalModes::default(), shell_spec: spawn.shell.clone(), remote: spawn.remote.clone(), agent: None, @@ -1747,6 +1755,7 @@ impl DaemonPane { mark_at_prompt: false, }, remote_prompt_seen: false, + modes: TerminalModes::default(), remote: carried.remote, agent: carried.agent, agent_session: carried.agent_session, @@ -1800,6 +1809,7 @@ impl DaemonPane { osc_title: None, shell: ShellState::default(), remote_prompt_seen: false, + modes: TerminalModes::default(), remote: Some(remote), agent: None, agent_session: None, @@ -2103,7 +2113,7 @@ impl DaemonPane { signals.shell.iter().any(|s| s.mark_at_prompt); 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 { @@ -2679,6 +2689,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); @@ -2689,6 +2718,15 @@ 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); +} + /// Everything a client needs to rebuild the pane's screen and status, in the /// order it has to be applied. /// @@ -2697,6 +2735,20 @@ impl ReplayRing { /// stored state. It gates the prompt report, and that gate is not cosmetic — /// see [`replayed_at_prompt`]. fn replay_state(st: &PaneState, subscriber: &Sender, foreground_command: bool) { + // 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. + // + // 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); if let Some(cwd) = &st.cwd { let _ = subscriber.send(DaemonMsg::Cwd(cwd.clone())); @@ -4801,6 +4853,7 @@ mod tests { osc_title: None, shell: ShellState::default(), remote_prompt_seen: false, + modes: TerminalModes::default(), remote: None, agent: None, agent_session: None, @@ -5283,6 +5336,108 @@ 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()); + } + + /// 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() { + 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()), false); + 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/crates/tty7-core/src/daemon/procinfo.rs b/crates/tty7-core/src/daemon/procinfo.rs index 54ee4780..217a530b 100644 --- a/crates/tty7-core/src/daemon/procinfo.rs +++ b/crates/tty7-core/src/daemon/procinfo.rs @@ -1,27 +1,190 @@ 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); - // The caller fills `context`: only the pane knows where its session lives, - // and this module only ever walks *this* machine's table. + 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 { + 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. +/// +/// 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, + // The caller fills `context`: only the pane knows where its session + // lives, and this module only ever walks *this* machine's table. context: None, } } +/// 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 && 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)] +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, + /// and on Linux the cheapest reading of it rather than the last word — see + /// `confirm_foreign`. + uid: u32, name: String, } @@ -38,7 +201,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 { @@ -102,6 +265,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, }, ); @@ -152,7 +319,26 @@ 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 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()); + table.insert( + pid, + Row { + ppid, + pgid, + uid, + name, + }, + ); } table } @@ -167,6 +353,7 @@ fn process_table() -> HashMap { Row { ppid: p.parent, pgid: 0, + uid: 0, name: p.name, }, ) @@ -227,20 +414,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", @@ -248,13 +457,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; @@ -268,32 +577,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(), + ); } _ => {} } @@ -317,8 +607,15 @@ 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. +/// +/// 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 { +fn listening_ports(procs: &[ProcEntry]) -> (Vec, PortProbe) { use std::net::{Ipv4Addr, Ipv6Addr}; use windows_sys::Win32::NetworkManagement::IpHelper::{ @@ -327,18 +624,19 @@ 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(); 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); @@ -363,9 +661,9 @@ fn listening_ports(procs: &[ProcEntry]) -> Vec { 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 { @@ -384,7 +682,25 @@ fn listening_ports(procs: &[ProcEntry]) -> Vec { } ports.sort_by_key(|e| (e.port, e.pid)); - ports + (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 @@ -396,13 +712,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, @@ -431,14 +751,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 @@ -500,11 +820,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) { @@ -521,8 +841,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`, @@ -544,10 +867,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(), } } @@ -595,7 +922,288 @@ 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" + ); + } + + /// 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), + 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] @@ -785,6 +1393,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() diff --git a/crates/tty7-core/src/daemon/protocol.rs b/crates/tty7-core/src/daemon/protocol.rs index 234720ee..8bdad16f 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, /// What the pane can say about itself that the process list cannot — see /// [`PaneContext`]. `None` from a daemon built before the field existed, /// which reads as "this daemon cannot say", never as a set of falses. @@ -2232,6 +2268,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/daemon/router.rs b/crates/tty7-core/src/daemon/router.rs index 4c759568..aa64431b 100644 --- a/crates/tty7-core/src/daemon/router.rs +++ b/crates/tty7-core/src/daemon/router.rs @@ -622,6 +622,33 @@ 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(), + ); + // 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?; 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, diff --git a/crates/tty7-core/src/host/server.rs b/crates/tty7-core/src/host/server.rs index b59f5631..6913a5e8 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(), context: None, } } diff --git a/docs/cli/reference.mdx b/docs/cli/reference.mdx index dee98fe1..6c0fac7f 100644 --- a/docs/cli/reference.mdx +++ b/docs/cli/reference.mdx @@ -326,10 +326,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. @@ -355,6 +367,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 diff --git a/src/terminal/input.rs b/src/terminal/input.rs index 0fe392ac..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) { @@ -171,7 +192,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 +218,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 +249,68 @@ 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. +/// `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'), + "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, + }) +} + +/// `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() || kitty_function_key(key).is_some() +} + fn text_key_code(ks: &gpui::Keystroke) -> Option { match ks.key.as_str() { "space" => Some(0x20), @@ -682,6 +778,200 @@ 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. + /// + /// 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 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() + }; + 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!( + legacy(&ks(none, key, None)), + None, + "{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 + /// 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 +979,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 +1106,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 +1291,60 @@ 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. 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. + #[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", "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/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 e67a8ee4..be4cee3e 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -22,8 +22,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; @@ -82,6 +83,60 @@ struct ReaderSignals { images: crate::terminal::images::ImageStore, clipboard_writes: Arc>>, clipboard_write_busy: Arc, + /// 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)] @@ -535,6 +590,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 @@ -687,7 +748,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)) @@ -757,7 +819,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) } @@ -839,6 +901,11 @@ impl RemoteTerminal { images: self.images.clone(), clipboard_writes: self.clipboard_writes.clone(), clipboard_write_busy: self.clipboard_write_busy.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); @@ -853,14 +920,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; @@ -894,6 +969,7 @@ impl RemoteTerminal { let clipboard_write_busy = Arc::new(AtomicBool::new(false)); 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(), @@ -916,6 +992,7 @@ impl RemoteTerminal { images: images.clone(), clipboard_writes: clipboard_writes.clone(), clipboard_write_busy: clipboard_write_busy.clone(), + repair_cursor: repair_cursor.clone(), }, ); @@ -955,6 +1032,7 @@ impl RemoteTerminal { proxy, reader_thread: Some(reader_thread), reader_quit, + repair_cursor, }) } @@ -997,17 +1075,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, @@ -1035,6 +1102,7 @@ impl RemoteTerminal { images, clipboard_writes, clipboard_write_busy, + repair_cursor, } = signals; crate::core::threads::promote_to_user_interactive(); let mut stream = read_half; @@ -1100,7 +1168,7 @@ impl RemoteTerminal { // emulator to the cut, act on the state that // sequence left behind, carry on. let mut cuts: Vec<(usize, CursorCut)> = Vec::new(); - if Self::REPAIR_PARKED_CURSOR { + if repair_cursor.load(Ordering::Relaxed) { cursor_scan.feed(&out_batch, |off, c| cuts.push((off, c))); } { @@ -1395,6 +1463,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; } @@ -1780,7 +1866,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; @@ -3209,8 +3298,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}"); @@ -3870,6 +3964,305 @@ 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" + ); + } +} + +/// 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::*; @@ -4299,8 +4692,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 { @@ -4962,135 +5360,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 _; 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/app.rs b/src/ui/app.rs index e8449b40..8409a7c5 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -3489,6 +3489,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) { @@ -3498,6 +3508,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); @@ -3549,9 +3577,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); } @@ -3755,14 +3781,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); @@ -8573,6 +8596,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)) } @@ -8638,7 +8663,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 } @@ -8666,13 +8692,66 @@ 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); + } +} + +/// 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). +/// +/// 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(); @@ -8704,7 +8783,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) } @@ -11146,3 +11226,281 @@ 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, replace_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" + ); + } + + /// 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" + ); + }); + } +} diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index f3572312..4884935d 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -1066,6 +1066,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 b0aa4296..d4e30d71 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -1130,6 +1130,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 ec9e2ac3..627f7c2c 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -786,6 +786,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 f4b90a1e..8b53c7a7 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -1021,6 +1021,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/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 diff --git a/src/ui/right_panel.rs b/src/ui/right_panel.rs index 10580ae9..43e21b74 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, @@ -1149,9 +1149,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 @@ -1163,7 +1166,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; } @@ -1324,22 +1333,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)), ) }, ) diff --git a/src/ui/tree_sync.rs b/src/ui/tree_sync.rs index f78b025d..be7f0ef3 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,15 @@ 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 +2927,7 @@ mod tests { cx, ws, epoch, - Ok((WsMirror::default(), Some("deploy".into()))), + Ok((WsMirror::default(), Some("deploy".into()), Arrival::Created)), ); assert!( @@ -2848,12 +2944,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 +2965,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 +2981,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 +3077,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 +3099,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 +3151,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 +3193,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 +3715,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 +3775,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 +4260,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!(