diff --git a/crates/tty7-core/src/daemon/control.rs b/crates/tty7-core/src/daemon/control.rs index b1cf4a20..4172676c 100644 --- a/crates/tty7-core/src/daemon/control.rs +++ b/crates/tty7-core/src/daemon/control.rs @@ -42,8 +42,16 @@ fn dialect_refusal(peer_build: &str, peer: u32, ours: u32) -> String { format!("control peer (build {peer_build}) {DIALECT_MARKER}{peer}, this build speaks v{ours}") } +/// Whether this is a refusal over the control dialect — the whole shape of one, +/// not just the marker. +/// +/// Every reader of a `true` here goes on to restate the refusal with +/// [`parse_dialect_refusal`], and one of them parks a link in a state only a +/// person can leave. Two predicates for one question is how those two come +/// apart: a message carrying the marker in some other shape would be parked on +/// and then shown as the raw protocol wording it was supposed to replace. pub fn is_dialect_refusal(message: &str) -> bool { - message.contains(DIALECT_MARKER) + parse_dialect_refusal(message).is_some() } #[derive(Clone, Debug, PartialEq, Eq)] diff --git a/src/ui/app.rs b/src/ui/app.rs index cb39d412..d63f4f30 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -1544,11 +1544,11 @@ impl Tty7App { // there, and the installer now refuses rather than killing the one // that is running. Updating is the move that works, so offer that, // under its own name and with its own warning about ending sessions. - if matches!( - self.remote_status(cx), - Some(crate::ui::remote_workspace::RemoteStatus::ServerMismatch(_)) - ) { - self.confirm_replace_remote_server(target, label, window, cx); + if let Some(crate::ui::remote_workspace::RemoteStatus::ServerMismatch(refusal)) = + self.remote_status(cx) + { + let action = crate::ui::remote_workspace::mismatch_action_key(&refusal); + self.confirm_replace_remote_server(target, label, action, window, cx); return; } self.confirm_restart_remote_server(target, label, window, cx); diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 0b98fe5f..8fb1e1e3 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -1251,6 +1251,11 @@ pub fn translate_en(key: L10nKey) -> &'static str { {cancel}\u{2003}leaves {machine} exactly as it is. This window will not connect." } L10nKey::RemoteMismatchReplaceServer => "Update Server", + // Same button, opposite direction: the machine is ahead of this build, + // so putting our server there takes it back a version. Calling that an + // update would be a lie, and it is the kind that ends other people's + // sessions on the way through. + L10nKey::RemoteMismatchDowngradeServer => "Replace Server", L10nKey::RemoteMismatchUnknownBuild => "an unknown build", L10nKey::RemoteMismatchUnknownBuildFromExe => "an unknown build (from {exe})", L10nKey::RemoteServerOutdated => { diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 3e9b3a28..8e0b1680 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -1283,6 +1283,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { "{machine} は {running} から tty7 セッションを提供していますが、このクライアント（{wanted}）はそのプロトコルを理解できません。tty7 は対応するサーバーをそこにインストール済みですが、セッションは実行中のサーバー上にあります。\n\n{replace_server}\u{2003}を選ぶと {wanted} に置き換えられ、そのサーバー上のセッションはすべて終了します。\n{cancel}\u{2003}を選ぶと {machine} はそのままです。このウィンドウは接続しません" } L10nKey::RemoteMismatchReplaceServer => "サーバーを更新", + L10nKey::RemoteMismatchDowngradeServer => "サーバーを置き換え", L10nKey::RemoteMismatchUnknownBuild => "不明なビルド", L10nKey::RemoteMismatchUnknownBuildFromExe => "不明なビルド（{exe} から）", L10nKey::RemoteServerOutdated => { diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 48efe14e..355fa435 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -1017,6 +1017,7 @@ l10n_keys! { RemoteMismatchUnknownBuild, RemoteMismatchUnknownBuildFromExe, RemoteMismatchReplaceServer, + RemoteMismatchDowngradeServer, RemoteServerOutdated, RemoteServerTooNew, RemoteDaemonStartFailed, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index aced2acc..e227882c 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -1174,6 +1174,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { {cancel}\u{2003}会保持 {machine} 现状不变。此窗口将不会连接。" } L10nKey::RemoteMismatchReplaceServer => "更新 server", + L10nKey::RemoteMismatchDowngradeServer => "替换 server", L10nKey::RemoteMismatchUnknownBuild => "未知构建", L10nKey::RemoteMismatchUnknownBuildFromExe => "未知构建（来自 {exe}）", L10nKey::RemoteServerOutdated => { diff --git a/src/ui/remote_workspace.rs b/src/ui/remote_workspace.rs index 79842aa6..a89aed67 100644 --- a/src/ui/remote_workspace.rs +++ b/src/ui/remote_workspace.rs @@ -100,9 +100,10 @@ impl RemoteStatus { )), // The protocol layer's wording reads like the far end is not tty7 // at all, and it is 20 words of dialect numbers. Say which side is - // behind instead. The fallback cannot be reached from the pump — - // nothing becomes `ServerMismatch` unless the refusal parsed — but - // it costs nothing and beats an empty strip. + // behind instead. Nothing becomes `ServerMismatch` unless the + // refusal parsed — `is_dialect_refusal` is that same parse — so the + // fallback is unreachable; it costs nothing and beats an empty + // strip if that ever stops being true. RemoteStatus::ServerMismatch(e) => Some( remote_connect::dialect_complaint(e, machine).unwrap_or_else(|| { t_fmt( @@ -136,7 +137,7 @@ impl RemoteStatus { // Not "retry" — retrying is what the strip used to offer here, and // it can only fail the same way. The only move that changes the // answer is installing the server this build speaks to. - RemoteStatus::ServerMismatch(_) => Some(t(L10nKey::RemoteMismatchReplaceServer)), + RemoteStatus::ServerMismatch(e) => Some(t(mismatch_action_key(e))), // A retry on a dead route fails deterministically; the switcher // row carries the honest actions (forget the entry, or dismiss). RemoteStatus::RouteLost => None, @@ -152,6 +153,21 @@ impl RemoteStatus { } } +/// Which way the one button points. +/// +/// Both directions do the same thing — put *this* build's server on that +/// machine — and that is an update only while the machine is behind. On one +/// that is ahead it is a downgrade, and the copy beside the button says so: +/// updating tty7 here is the first suggestion, replacing the server there the +/// second. The button is the second one, so it should not wear the first one's +/// word. +pub(crate) fn mismatch_action_key(refusal: &str) -> L10nKey { + match crate::daemon::control::parse_dialect_refusal(refusal) { + Some(r) if r.peer > r.ours => L10nKey::RemoteMismatchDowngradeServer, + _ => L10nKey::RemoteMismatchReplaceServer, + } +} + /// What the remote strip's button is for, once the status has been read. #[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum StripAction { @@ -161,12 +177,23 @@ pub(crate) enum StripAction { UpdateServer { target: RemoteTarget, label: String, + /// The word for it, carried rather than recomputed: the confirmation + /// this ends in must offer the same one the button did. + action: L10nKey, }, } pub const RECONNECT_FIRST: std::time::Duration = std::time::Duration::from_secs(1); pub const RECONNECT_CAP: std::time::Duration = std::time::Duration::from_secs(30); +/// How long a link parked on a dialect refusal waits before looking again. +/// +/// Not a backoff — it never grows, and it is two orders of magnitude off the +/// reconnect clock on purpose. The refusal only stops being true when something +/// happens on the far end that nobody here is told about, so this is a slow +/// question about someone else's machine, not a retry of our own failure. +pub const PARKED_RECHECK: std::time::Duration = std::time::Duration::from_secs(300); + #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] pub struct Backoff { attempt: u32, @@ -355,7 +382,7 @@ impl Tty7App { cx: &gpui::App, ) -> Option<(&'static str, StripAction)> { let label = status.action_label()?; - let RemoteStatus::ServerMismatch(_) = status else { + let RemoteStatus::ServerMismatch(refusal) = status else { return Some((label, StripAction::Retry)); }; // Only a machine whose server is ours to install can be updated from @@ -368,6 +395,7 @@ impl Tty7App { StripAction::UpdateServer { target: own.target.clone(), label: self.remote_machine_label(cx), + action: mismatch_action_key(refusal), }, ) }) @@ -381,9 +409,11 @@ impl Tty7App { ) { match action { StripAction::Retry => self.remote_retry(cx), - StripAction::UpdateServer { target, label } => { - self.confirm_replace_remote_server(target, label, window, cx) - } + StripAction::UpdateServer { + target, + label, + action, + } => self.confirm_replace_remote_server(target, label, action, window, cx), } } @@ -726,10 +756,15 @@ impl Tty7App { .detach(); } + /// `action` is the word the thing that led here used on its own button — + /// Update or Replace, depending on which side is behind. A confirmation + /// that renames the act between the click and the prompt is asking about + /// something the user did not choose. pub(crate) fn confirm_replace_remote_server( &mut self, target: RemoteTarget, label: String, + action: L10nKey, window: &mut Window, cx: &mut Context, ) { @@ -737,10 +772,7 @@ impl Tty7App { PromptLevel::Warning, &t_fmt(L10nKey::RemoteMismatchTitle, &[("machine", &label)]), Some(&t_fmt(L10nKey::RemoteReplaceBody, &[("machine", &label)])), - &crate::ui::confirm_answers( - t(L10nKey::RemoteMismatchReplaceServer), - t(L10nKey::Cancel), - ), + &crate::ui::confirm_answers(t(action), t(L10nKey::Cancel)), cx, ); cx.spawn(async move |this, cx| { @@ -1142,11 +1174,15 @@ impl RemoteLinks { }); link.backoff.reset(); link.next_attempt = Some(Instant::now()); - // Someone asked for this by hand, which is also the only way out of a - // park — and the reason it was parked is now the *previous* answer, not - // the current one. Keeping it would put the old complaint back on the - // strip underneath a fresh attempt. - link.last_error = None; + // Leaving a park, the refusal that caused it is the *previous* answer: + // carrying it over would put "that server is too old" back on the strip + // underneath a fresh attempt, as though this one had failed too. Every + // other reason is still the last thing that actually happened, and the + // strip says so while the attempt is in flight — pressing Retry Now on + // an unreachable machine should not cost the user the reason why. + if matches!(link.state, LinkState::Mismatched(_)) { + link.last_error = None; + } if !link.attempting { link.state = LinkState::Reconnecting; } @@ -1274,23 +1310,32 @@ fn pump_tick(cx: &mut gpui::App) -> bool { continue; } - // Parked on a dialect refusal, for the same reason and in the same - // shape as an unresolvable route: the attempt is known to fail. Two - // things still get out of it, and both need a human to say so — - // `retry_now`, which the Update Server flow ends in, and a manual - // connect from the switcher, whose link the `live` branch above adopts - // before this ever runs. - let parked = cx - .try_global::() - .and_then(|l| l.machines.get(&host)) - .is_some_and(|l| matches!(l.state, LinkState::Mismatched(_))); - if parked { - continue; - } - let due = { let mut due = false; RemoteLinks::mark(cx, host, |link| { + // Parked on a dialect refusal: the answer is known, so no + // backoff and no attempt counter. What gets out of it is + // normally a person — `retry_now`, which the Update Server flow + // ends in, or a manual connect the `live` branch above adopts. + // + // But this machine's build is not ours to know. Someone else's + // client can update that server, and the machine can come back + // from a reboot on a build that speaks to us; nothing tells us + // when. So look again on a slow clock — far enough apart that + // it is not the retry loop this replaced, close enough that a + // machine which quietly got fixed does not sit here claiming to + // be broken for the rest of the session. + if matches!(link.state, LinkState::Mismatched(_)) { + match link.next_attempt { + None => link.next_attempt = Some(now + PARKED_RECHECK), + Some(at) if at <= now => { + due = true; + link.next_attempt = None; + } + Some(_) => {} + } + return; + } if !matches!(link.state, LinkState::Reconnecting) { changed = true; link.state = LinkState::Reconnecting; @@ -2203,6 +2248,12 @@ mod tests { Some(t(L10nKey::RemoteNoticeDisconnected)), Some(t(L10nKey::RemoteMismatchReplaceServer)), ), + ( + RemoteStatus::ServerMismatch(a_refusal_between(7, 6)), + false, + Some(t(L10nKey::RemoteNoticeDisconnected)), + Some(t(L10nKey::RemoteMismatchDowngradeServer)), + ), ]; for (status, accepts, notice, action) in cases { assert_eq!(status.accepts_input(), accepts, "{status:?}"); @@ -2253,6 +2304,44 @@ mod tests { (target.host_id(), target) } + /// A machine the pump will actually think about. + /// + /// An `Alias` is only resolvable while that name is in the ssh config of + /// whoever is running the tests, and the pump drops an unresolvable route + /// before it reaches anything else — so a test that asserts on what the + /// pump does to a link must not use one, or it passes by not getting there. + fn resolvable_machine(name: &str) -> (HostId, RemoteTarget) { + let target = RemoteTarget::Wsl { + distro: name.to_string(), + }; + (target.host_id(), target) + } + + /// 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) { + 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; + let id = entry.id; + WorkspaceStore::install_for_test( + cx, + crate::core::session::WindowViews { + views: vec![entry], + active: None, + }, + ); + finish_attempt(cx, host, &target, Err(a_refusal())); + (host, target, id) + } + #[test] fn a_disconnect_ends_when_the_last_window_on_that_machine_closes() { let (build, build_t) = machine("build-box"); @@ -2279,7 +2368,7 @@ mod tests { cx.set_global(crate::core::config::Config::default()); crate::ui::windows::WindowRegistry::init(cx); - let (host, target) = machine("build-box"); + let (host, target) = resolvable_machine("build-box"); let mut entry = crate::core::session::WindowView::on_remote(RemoteRef::new( target.clone(), WorkspaceId::new(), @@ -2311,7 +2400,7 @@ mod tests { ); // Whatever the pump does with a parked machine, it must not be to - // schedule another attempt — that is the loop this replaced. + // put it back on the backoff — that is the loop this replaced. for _ in 0..4 { pump_tick(cx); } @@ -2321,10 +2410,19 @@ mod tests { matches!(link.state, LinkState::Mismatched(_)), "four ticks later it is still parked, not back on the backoff" ); - assert!(link.next_attempt.is_none() && !link.attempting); + assert!(!link.attempting); assert_eq!(link.backoff.attempt(), 0, "no attempt was ever scheduled"); + let wait = link + .next_attempt + .expect("a parked link still looks again eventually") + .saturating_duration_since(Instant::now()); + assert!( + wait > RECONNECT_CAP, + "the next look is on the slow clock, not the reconnect one: {wait:?}" + ); - // The one way out, which is what Update Server ends in. + // The one way out that does not involve waiting, and the one Update + // Server ends in. RemoteLinks::retry_now(cx, id); assert!( matches!( @@ -2336,6 +2434,64 @@ mod tests { }); } + #[gpui::test] + fn a_parked_link_looks_again_when_the_slow_clock_runs_out(cx: &mut gpui::TestAppContext) { + cx.update(|cx| { + let (host, _, _) = parked_on_a_refusal(cx); + pump_tick(cx); + + // Standing in for five minutes of nobody touching anything. The + // machine's build is not ours to know: someone else's client can + // update that server, and nothing over here is told when. + cx.default_global::() + .machines + .get_mut(&host) + .expect("the machine is still known") + .next_attempt = Some(Instant::now() - std::time::Duration::from_secs(1)); + pump_tick(cx); + + let link = cx.default_global::().machines.get(&host); + let link = link.expect("the machine is still known"); + assert!( + link.attempting, + "the slow clock ran out, so it looked again" + ); + assert!( + matches!(link.state, LinkState::Mismatched(_)), + "and said nothing new on the strip while it did: a look that fails \ + the same way must not read as a fresh problem" + ); + assert_eq!( + link.backoff.attempt(), + 0, + "looking again is not the backoff coming back" + ); + }); + } + + #[gpui::test] + fn a_park_that_is_still_true_goes_straight_back_on_the_slow_clock( + cx: &mut gpui::TestAppContext, + ) { + cx.update(|cx| { + let (host, target, _) = parked_on_a_refusal(cx); + // The look the slow clock bought, answered the same way as before. + finish_attempt(cx, host, &target, Err(a_refusal())); + pump_tick(cx); + + let link = cx.default_global::().machines.get(&host); + let link = link.expect("the machine is still known"); + let wait = link + .next_attempt + .expect("still parked, so still looking again later") + .saturating_duration_since(Instant::now()); + assert!( + wait > RECONNECT_CAP, + "a refusal that repeats resets the slow clock rather than tightening it: {wait:?}" + ); + }); + } + #[gpui::test] fn disconnecting_rests_at_not_connected_and_connect_undoes_it(cx: &mut gpui::TestAppContext) { cx.update(|cx| { @@ -2409,18 +2565,35 @@ mod tests { /// What `connect_blocking` hands back when the far end answers the hello /// with another dialect, localised wrapper and all. fn a_refusal() -> String { + a_refusal_between(5, 6) + } + + fn a_refusal_between(peer: u32, ours: u32) -> String { + let error = format!( + "control peer (build 26.8.1) speaks control v{peer}, this build speaks v{ours}" + ); t_fmt( L10nKey::RemoteHostNotTty7, - &[ - ("machine", "build-box"), - ( - "error", - "control peer (build 26.8.1) speaks control v5, this build speaks v6", - ), - ], + &[("machine", "build-box"), ("error", &error)], ) } + #[test] + fn the_button_calls_a_downgrade_a_downgrade() { + crate::ui::i18n::set_locale("en"); + assert_eq!( + t(mismatch_action_key(&a_refusal_between(5, 6))), + "Update Server", + "that machine is behind, so putting our server there moves it forward" + ); + assert_eq!( + t(mismatch_action_key(&a_refusal_between(7, 6))), + "Replace Server", + "that machine is ahead: the same button takes it back a version, and \ + the copy beside it offers updating *this* computer first" + ); + } + #[test] fn a_hand_run_connect_refused_on_the_dialect_gets_the_same_answer() { let target = RemoteTarget::Alias { diff --git a/src/ui/switcher.rs b/src/ui/switcher.rs index 484c4283..25b67e49 100644 --- a/src/ui/switcher.rs +++ b/src/ui/switcher.rs @@ -1879,6 +1879,7 @@ impl Tty7App { // already names it. let shown = remote_connect::dialect_complaint(error, &group.label) .unwrap_or_else(|| format!("{}: {error}", group.label)); + let replace_action = crate::ui::remote_workspace::mismatch_action_key(error); let theme = cx.theme(); v_flex() .gap(px(4.)) @@ -1921,14 +1922,21 @@ impl Tty7App { ) .when( crate::daemon::control::is_dialect_refusal(error) - && replace.target.is_some(), + // Same gate as the workspace strip's: a machine + // whose server is not ours to install cannot be + // helped by this button, and a click that can only + // fail is worse than no button. + && replace + .target + .as_ref() + .is_some_and(|t| t.hosts_our_server()), |row| { row.child( Button::new(gpui::SharedString::from(format!( "switcher-replace:{}", group.key ))) - .label(t(L10nKey::RemoteMismatchReplaceServer)) + .label(t(replace_action)) .ghost() .xsmall() .on_click(cx.listener( @@ -1938,6 +1946,7 @@ impl Tty7App { this.confirm_replace_remote_server( target, replace.label.clone(), + replace_action, window, cx, );