From d68b42af7dc2596f2af4eaa6f775ad674a90bcdf Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Thu, 13 Aug 2026 19:51:03 +0800 Subject: [PATCH] fix(remote): recheck a parked link, and name a downgrade a downgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the dialect-refusal parking. `is_dialect_refusal` was a substring sniff on the marker while every reader of a `true` went on to parse the whole shape. Two predicates for one question, and the weaker one decided whether to park a link that only a person could free. It is the parse now. A parked link never looked again. `RouteLost`, the state it was modelled on, is re-tested every tick and comes back by itself; this one could not, so a machine somebody else updated — or one that rebooted onto a build that does speak to us — sat there claiming to be broken for the rest of the session. It looks again every five minutes: a slow clock, deliberately two orders of magnitude off the reconnect one, and the strip says nothing while it does. `retry_now` cleared `last_error` for every caller, so pressing Retry Now on an unreachable machine cost the user the reason why until the next attempt finished. Only leaving a park clears it. The one button read Update Server in both directions, including the one where installing our server takes the far end back a version. That direction reads Replace Server, and the confirmation it opens offers the same word the button did rather than renaming the act between the click and the prompt. The switcher's band gates that button on `hosts_our_server` as well now, the way the workspace strip already did. `a_dialect_refusal_parks_the_link_instead_of_counting_attempts` passed without reaching what it named: an `Alias` resolves only if the machine running the tests has that name in its ssh config, and the pump drops an unresolvable route before it reaches any parking. It uses a target that always resolves now, and both new pump tests were checked against a mutation that removes the recheck. --- crates/tty7-core/src/daemon/control.rs | 10 +- src/ui/app.rs | 10 +- src/ui/i18n/en.rs | 5 + src/ui/i18n/ja.rs | 1 + src/ui/i18n/mod.rs | 1 + src/ui/i18n/zh.rs | 1 + src/ui/remote_workspace.rs | 257 +++++++++++++++++++++---- src/ui/switcher.rs | 13 +- 8 files changed, 248 insertions(+), 50 deletions(-) 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, );