From 84f9d54ad672eec4e6454a225988df4c63f5cd98 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:20:43 +0800 Subject: [PATCH] fix(remote): finish the create a server update interrupted, and retire the note it answers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Creating a workspace on a machine whose server is the other side of a dialect bump took two creates and two update clicks in different places. Two holes in one flow: The create parked on the connect died with the refusal — finish_connect's Err arm dropped pending_create — so the update the refusal band offered ran to completion and then nothing happened: no workspace, and nothing left for any reconnect to finish. A create refused for the dialect now moves aside to parked_create, the replacement's success connects at the machine again, and whichever connect finally lands spends it, name and all. Dismissing the refusal or disconnecting the machine still calls the create off; every other failure does too. The mismatch note recorded during that failed attempt outlived the very replacement that answered it: the queue is only drained after a successful connect, so the next one raised "update this server?" about a server that was already updated — and confirming killed the fresh daemon all over again, sessions and all. reconnect_after_restart now retires the origin's notes the moment a restart or replacement lands. Three tests, each confirmed to fail without the change it guards. The end-to-end one drives finish_connect against a real control server over a socketpair, so the parked create is spent by the same code path a live reconnect uses. --- crates/tty7-core/src/daemon/install/mod.rs | 9 + src/ui/app.rs | 8 + src/ui/remote_workspace.rs | 333 ++++++++++++++++++++- src/ui/switcher.rs | 21 ++ 4 files changed, 360 insertions(+), 11 deletions(-) diff --git a/crates/tty7-core/src/daemon/install/mod.rs b/crates/tty7-core/src/daemon/install/mod.rs index d5753fa9..82b4eaa1 100644 --- a/crates/tty7-core/src/daemon/install/mod.rs +++ b/crates/tty7-core/src/daemon/install/mod.rs @@ -449,6 +449,15 @@ pub fn record_remote_mismatches(entries: Vec) { } } +/// Retires the notes a machine earned before its server was restarted or +/// replaced into this build. Keyed the way the notes are — by the route origin +/// that discovered them — so notes about other machines stay owed. +pub fn forget_remote_mismatch(host: &str) { + if let Ok(mut slot) = MISMATCHED.lock() { + slot.retain(|e| e.host != host); + } +} + pub fn take_mismatched_remote_daemons() -> Vec { MISMATCHED .lock() diff --git a/src/ui/app.rs b/src/ui/app.rs index d63f4f30..267c1e1c 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -594,6 +594,13 @@ pub struct Tty7App { /// A create asked of a machine that was not connected yet; the connect /// finishing is what completes it (see `Tty7App::finish_connect`). pub(crate) pending_create: Option, + /// A create whose connect was refused for the control dialect — the state + /// the "update server" button answers. Set aside rather than dropped, so + /// the update the user runs next still ends in the workspace they asked + /// for. Held apart from `pending_create` on purpose: only a connect to the + /// same machine may spend it, and giving the machine up (dismissing the + /// refusal, disconnecting) discards it. + pub(crate) parked_create: Option, /// Why the window opened with no terminal in it. Shown on the home screen, /// which is otherwise indistinguishable from having closed everything. pub(crate) startup_error: Option, @@ -1137,6 +1144,7 @@ impl Tty7App { remote_host_errors: std::collections::HashMap::new(), parked_dismissed: std::collections::HashSet::new(), pending_create: None, + parked_create: None, startup_error, }; if !cfg!(test) && crate::ui::windows::WindowRegistry::count(cx) == 0 { diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index a89aed67..2a4f6012 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -530,12 +530,7 @@ impl Tty7App { // A create that was waiting on this link (the switcher's form, // asked of a machine that was not connected yet) can now run: // the link just told us the home directory to root it at. - if self - .pending_create - .as_ref() - .is_some_and(|p| p.target == choice.target) - { - let pending = self.pending_create.take().expect("checked above"); + if let Some(pending) = self.take_create_waiting_on(&choice.target) { self.close_switcher(window, cx); self.create_remote_workspace(pending.target, home, window, cx); self.name_fresh_workspace(pending.name, window, cx); @@ -548,7 +543,17 @@ impl Tty7App { .as_ref() .is_some_and(|p| p.target == choice.target) { - self.pending_create = None; + // A dialect refusal is the one failure with a button on + // it — the "update server" this window is about to offer. + // The create moves aside to wait for that answer instead + // of dying with the attempt, or the update would end in + // nothing and the user would have to ask all over again. + // Every other failure still calls the create off. + if crate::daemon::control::is_dialect_refusal(&error) { + self.parked_create = self.pending_create.take(); + } else { + self.pending_create = None; + } } self.connect = Some(ConnectFlow::Failed { choice, error }); } @@ -556,6 +561,51 @@ impl Tty7App { cx.notify(); } + /// The create waiting on this machine's link, wherever it waits: parked on + /// the app by the form (`pending_create`), or set aside by a dialect + /// refusal until the server over there was updated (`parked_create`). + /// Either way it is spent by the connect that finally lands. + fn take_create_waiting_on( + &mut self, + target: &RemoteTarget, + ) -> Option { + for slot in [&mut self.pending_create, &mut self.parked_create] { + if slot.as_ref().is_some_and(|p| &p.target == target) { + return slot.take(); + } + } + None + } + + /// The far end was just cycled into a server this build speaks to — the + /// answer a create refused for the dialect has been waiting on. Reconnects + /// whatever the restart cut, and connects at the machine again if a create + /// is still parked on it, so the workspace the user asked for finally + /// exists; `finish_connect` finds the create via `take_create_waiting_on`. + fn server_replaced( + &mut self, + target: &RemoteTarget, + label: &str, + origin: &str, + cx: &mut Context, + ) { + reconnect_after_restart(origin, cx); + if self + .parked_create + .as_ref() + .is_some_and(|p| &p.target == target) + { + self.connect_to_host( + HostChoice { + target: target.clone(), + label: label.to_string(), + detail: String::new(), + }, + cx, + ); + } + } + pub(crate) fn open_remote_workspace( &mut self, target: RemoteTarget, @@ -745,7 +795,7 @@ impl Tty7App { let _ = this.update_in(cx, |this, window, cx| match outcome { Ok(()) => { log::info!("{label} is now serving this client's build"); - reconnect_after_restart(&host, cx); + this.server_replaced(&target_for_error, &label, &host, cx); } Err(e) => { log::warn!("could not restart tty7's server on {label}: {e}"); @@ -819,7 +869,7 @@ impl Tty7App { let _ = this.update_in(cx, |this, window, cx| match outcome { Ok(()) => { log::info!("{label} is now serving this client's build"); - reconnect_after_restart(&host, cx); + this.server_replaced(&target_for_error, &label, &host, cx); } Err(e) => { log::warn!("could not replace tty7's server on {label}: {e}"); @@ -1343,7 +1393,11 @@ fn pump_tick(cx: &mut gpui::App) -> bool { // Whatever we told the far end went down with the link. link.attach_sent.clear(); match link.next_attempt { - None => link.next_attempt = Some(now + link.backoff.advance()), + // Scheduling is not failing: the counter moves in + // `finish_attempt` when an attempt actually comes back + // wrong, so the strip's "attempt N" stays the number of + // tries that really happened, not one ahead of it. + None => link.next_attempt = Some(now + link.backoff.delay()), Some(at) if at <= now => { due = true; link.next_attempt = None; @@ -1603,6 +1657,11 @@ pub(crate) fn drain_events(cx: &mut gpui::App) { } fn reconnect_after_restart(origin: &str, cx: &mut gpui::App) { + // The mismatch note this machine's old daemon earned is answered now — + // the restart just put this build's server there. Left in the queue it + // outlives the fix, and the next successful connect raises it again as a + // second "update this server?" about a server that was already updated. + crate::daemon::install::forget_remote_mismatch(origin); let Some(host) = remote_connect::origin_host(origin) else { return; }; @@ -1686,7 +1745,32 @@ fn finish_attempt( match outcome { Ok((connected, sent)) => { let restarted = server_restarted(cx, host, &connected.host); + let rows = connected.rows.clone(); remote_connect::HostLinks::insert(cx, connected.host, connected.home); + // Every successful attach carries the machine's own workspace + // listing, not just the switcher's explicit connect: a workspace + // another client created on this machine only becomes visible + // here if this path merges the listing too. + let listing: Vec<(WorkspaceId, String, u64)> = rows + .iter() + .map(|r| (r.id, r.name.clone(), r.last_active)) + .collect(); + WorkspaceStore::sync_remote(cx, target, &listing); + for (_, app) in crate::ui::windows::WindowRegistry::open_windows(cx) { + let Some(app) = app.upgrade() else { + continue; + }; + app.update(cx, |app, cx| { + app.host_snapshots.insert( + host, + crate::ui::switcher::HostSnapshot { + target: target.clone(), + rows: rows.clone(), + }, + ); + cx.notify(); + }); + } for (id, key) in workspaces_on(cx, host) { let reclaimed = { let links = cx.default_global::(); @@ -1734,6 +1818,13 @@ fn finish_attempt( } else { LinkState::Reconnecting }; + if !parked { + // The counter is the number of attempts that came back + // wrong. It moves here, not when the pump schedules one: + // a first try still in flight is attempt 1 on the strip, + // not attempt 2. A parked look stays off the backoff. + link.backoff.advance(); + } link.next_attempt = None; link.last_error = Some(e.clone()); }); @@ -2024,6 +2115,178 @@ mod tests { }); } + /// A refusal as the daemon writes it — what `finish_connect` receives when + /// the machine's server is the other side of a dialect bump. + fn a_dialect_refusal() -> String { + "build-box answered, but not as a tty7 server: control peer (build 26.7.7-nightly) \ + speaks control v4, this build speaks v5" + .to_string() + } + + /// A live `Connected` over a socketpair, served by a real control server in + /// a thread — what a connect that finally landed hands `finish_connect`. + #[cfg(unix)] + fn fake_connected(connection_key: &str) -> remote_connect::Connected { + use tty7_core::daemon::control::ControlHello; + use tty7_core::host::local::LocalHost; + use tty7_core::host::server::{Services, serve_with}; + + let (server, client) = std::os::unix::net::UnixStream::pair().unwrap(); + std::thread::spawn(move || { + let _ = serve_with(server, LocalHost::new(), Services::none()); + }); + let hello = ControlHello::host_rpc("test-token", "test-client"); + let host = RemoteHost::over_unix(client, connection_key, &hello) + .expect("the fake server answers the hello"); + remote_connect::Connected { + host, + home: std::path::PathBuf::from("/tmp"), + rows: Vec::new(), + } + } + + /// The regression behind "I clicked update and nothing happened": a create + /// whose connect was refused for the dialect used to die with the attempt, + /// so the update the refusal button ran had nothing left to finish and the + /// user had to create the workspace all over again. + #[cfg(unix)] + #[gpui::test] + fn a_create_refused_for_dialect_runs_once_the_machine_connects(cx: &mut gpui::TestAppContext) { + use gpui::VisualContext as _; + + let (app, mut vcx) = crate::ui::app::test_window::harness(cx); + // Entering the created workspace walks the registry, so the window has + // to be in it — the same setup the create form's own test needs. + let handle = vcx.window_handle(); + let weak = app.downgrade(); + app.update(cx, |app, cx| { + crate::core::session::WorkspaceStore::install_for_test( + cx, + crate::core::session::WindowViews::default(), + ); + crate::ui::windows::WindowRegistry::init(cx); + crate::ui::windows::WindowRegistry::register(cx, app.workspace, handle, weak); + }); + let target = RemoteTarget::Alias { + alias: "tty7-test-refused-box".into(), + }; + let choice = HostChoice { + target: target.clone(), + label: "refused-box".into(), + detail: String::new(), + }; + + app.update_in(&mut vcx, |app, window, cx| { + app.pending_create = Some(crate::ui::switcher::PendingCreate { + target: target.clone(), + name: Some("deploy".into()), + }); + app.connect = Some(ConnectFlow::Connecting { + choice: choice.clone(), + }); + app.finish_connect(Err(a_dialect_refusal()), window, cx); + }); + + // The server over there was updated and the machine finally connected — + // whether over `server_replaced`'s kick or the band's own retry, it + // ends in the same place. + app.update_in(&mut vcx, |app, window, cx| { + app.finish_connect(Ok(fake_connected("tty7-test-refused-box")), window, cx); + }); + + app.update(cx, |app, cx| { + let own = WorkspaceStore::remote_ref(cx, app.workspace) + .expect("the parked create ran and this window entered its workspace"); + assert_eq!(own.target, target); + assert_eq!( + crate::ui::tree_sync::chosen_name_for(cx, app.workspace).as_deref(), + Some("deploy"), + "the name travels with the create it was typed for" + ); + }); + } + + /// The moment the replacement lands the machine is connected at again + /// without waiting for the user — the parked create stays put for that + /// connect to spend, not for a click that will never come. + #[gpui::test] + fn a_server_replacement_connects_back_for_the_parked_create(cx: &mut gpui::TestAppContext) { + let (app, mut vcx) = crate::ui::app::test_window::harness(cx); + let target = RemoteTarget::Alias { + alias: "tty7-test-refused-box".into(), + }; + let choice = HostChoice { + target: target.clone(), + label: "refused-box".into(), + detail: String::new(), + }; + + app.update_in(&mut vcx, |app, window, cx| { + app.pending_create = Some(crate::ui::switcher::PendingCreate { + target: target.clone(), + name: None, + }); + app.connect = Some(ConnectFlow::Connecting { + choice: choice.clone(), + }); + app.finish_connect(Err(a_dialect_refusal()), window, cx); + }); + + app.update(cx, |app, cx| { + app.server_replaced(&target, "refused-box", "tty7-test-unknown-origin", cx); + // The old refusal is still `Failed { target }` too, so the target + // alone proves nothing — what has to change is the failure itself: + // a fresh attempt ran and left its own outcome in its place. + match app.connect.as_ref() { + Some(ConnectFlow::Connecting { choice }) => assert_eq!(choice.target, target), + Some(ConnectFlow::Failed { choice, error }) => { + assert_eq!(choice.target, target); + assert!( + !crate::daemon::control::is_dialect_refusal(error), + "the refusal was never retried: {error}" + ); + } + None => panic!("the replacement kicks a connect at the machine"), + } + assert!( + app.parked_create.is_some(), + "the create keeps waiting for that connect to land" + ); + }); + } + + /// The second half of the double-prompt: the note a mismatched daemon + /// earned used to outlive the very replacement that answered it, so the + /// next successful connect asked to update a server that was already + /// updated — and confirming killed the fresh daemon all over again. + #[gpui::test] + fn a_restart_retires_the_mismatch_note_it_answers(cx: &mut gpui::TestAppContext) { + cx.update(|cx| { + let origin = "tty7-test-stale-origin"; + crate::daemon::install::record_remote_mismatches(vec![ + crate::daemon::install::MismatchedRemoteDaemon { + host: origin.into(), + running_version: Some("0.8.0".into()), + running_exe: None, + wanted_version: "0.9.0".into(), + }, + ]); + + reconnect_after_restart(origin, cx); + + let drained = crate::daemon::install::take_mismatched_remote_daemons(); + let (ours, others): (Vec<_>, Vec<_>) = + drained.into_iter().partition(|m| m.host == origin); + // Notes about other machines are still owed; put back what the + // drain took. + crate::daemon::install::record_remote_mismatches(others); + assert!( + ours.is_empty(), + "the restart just answered this note; raising it again is the double prompt" + ); + }); + } + #[test] fn the_status_strip_speaks_unless_everything_is_working() { crate::ui::i18n::set_locale("en"); @@ -2317,6 +2580,49 @@ mod tests { (target.host_id(), target) } + /// The strip's "attempt N" is the number of tries that actually came back + /// wrong. The pump scheduling a try is not one: a first connect still in + /// flight used to read "attempt 2" on a link that had never failed. + #[gpui::test] + fn scheduling_a_try_is_not_a_failed_attempt(cx: &mut gpui::TestAppContext) { + cx.update(|cx| { + crate::core::config::pin_test_config_dir(); + cx.set_global(crate::core::config::Config::default()); + crate::ui::windows::WindowRegistry::init(cx); + + let (host, target) = resolvable_machine("build-box"); + let mut entry = crate::core::session::WindowView::on_remote(RemoteRef::new( + target.clone(), + WorkspaceId::new(), + )); + entry.open = true; + WorkspaceStore::install_for_test( + cx, + crate::core::session::WindowViews { + views: vec![entry], + active: None, + }, + ); + + pump_tick(cx); + let attempt = |cx: &mut gpui::App| { + cx.default_global::() + .machines + .get(&host) + .expect("the machine is known") + .backoff + .attempt() + }; + assert_eq!(attempt(cx), 0, "the first try is scheduled, not failed"); + + finish_attempt(cx, host, &target, Err("connection refused".into())); + assert_eq!(attempt(cx), 1, "a try that came back wrong is one"); + + pump_tick(cx); + assert_eq!(attempt(cx), 1, "rescheduling the next try adds nothing"); + }); + } + /// A parked machine with one open workspace on it, wound forward to the /// moment after the refusal. fn parked_on_a_refusal(cx: &mut gpui::App) -> (HostId, RemoteTarget, WorkspaceId) { @@ -2411,7 +2717,12 @@ mod tests { "four ticks later it is still parked, not back on the backoff" ); assert!(!link.attempting); - assert_eq!(link.backoff.attempt(), 0, "no attempt was ever scheduled"); + assert_eq!( + link.backoff.attempt(), + 1, + "the counter still reads the one ordinary failure before the \ + park; neither the refusal nor the parked ticks moved it" + ); let wait = link .next_attempt .expect("a parked link still looks again eventually") diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index da2431ac..82b24140 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -1059,6 +1059,8 @@ impl Tty7App { // A create still waiting on this machine's link dies with the link: // calling the connect off is calling the create off, or the next // successful connect would grow a workspace nobody asked it for. + // One parked on a dialect refusal is waiting all the same, so it + // dies here too. if self .pending_create .as_ref() @@ -1066,6 +1068,13 @@ impl Tty7App { { self.pending_create = None; } + if self + .parked_create + .as_ref() + .is_some_and(|p| &p.target == target) + { + self.parked_create = None; + } cx.notify(); } @@ -1976,6 +1985,18 @@ impl Tty7App { { this.connect = None; } + // Dismissing the refusal is giving up on the + // machine, and the create parked on it goes + // with it — an update run weeks later must + // not revive a workspace nobody remembers + // asking for. + if this + .parked_create + .as_ref() + .is_some_and(|p| Some(&p.target) == dismiss_target.as_ref()) + { + this.parked_create = None; + } cx.notify(); }, )),