mirror of
https://github.com/l0ng-ai/tty7.git
synced 2026-09-22 08:02:24 +00:00
fix(workspace): stop switching workspaces from destroying live sessions (#410)
* fix(workspace): stop switching workspaces from destroying live sessions Switching workspaces rebuilt every pane it was asked to restore, and a window that rebuilt nothing then deleted the workspace outright — tree and store both. Three separate guesses, each one authorizing an irreversible act: - `session_from_tree` erased a pane's id when the tree said `live: false`. That flag is a cached observation from another process, reloaded as false on every server start, so a quiet pane read as dead while its shell was running. The restore had nothing to attach to and spawned a fresh shell over it. - Two servers could start against one config dir. `run_with` decided another server was dead by failing to connect once, then unlinked its socket and bound its own. The loser kept `control.sock` with an empty pane registry, so `MachineGet` reported every pane dead and nothing logged an error. - `finish_hydration` marked a window informed before the rebuild and without looking at the result. `tabs_from_session` drops any tab whose panes all fail to start, which is every tab when the pane socket is unreachable — leaving a window that was empty and authoritative at once, and the next switch deleted a workspace with ten live tabs. Each is now settled by whoever holds the truth: attaching decides whether a pane is there, an advisory lock decides which process is the server, and a deletion needs the machine's own mirror to agree that the workspace is empty. * fix(state): quarantine a corrupt views.json instead of silently discarding it machine.json already sets a corrupt file aside before falling back to defaults; views.json just logged and returned None, and the next save overwrote whatever the file held. Move the quarantine helpers to config so both loaders share them. Also make the no-lock-primitive fallback in the daemon singleton report Unavailable rather than Taken, so a platform without flock still gets a server instead of one that refuses to start. * fix(restore): a failed List no longer reads as every pane being dead Review follow-ups on #410, all three the same shape the PR exists to stamp out: - alive_panes_on flattened a failed List RPC into an empty alive-set, which made pane_attachable respawn every pane in the batch over its running session. The failure now surfaces as None and the attach itself decides, the way session_from_tree already leaves it to. - pub fn run() bypassed the singleton lock entirely; it had no callers, so a future one would have silently reintroduced the split-brain. Removed. - Singleton::path() and the field behind it were unused. Removed. --------- Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com>
This commit is contained in:
@@ -622,6 +622,39 @@ pub fn strip_bom(text: &str) -> &str {
|
||||
text.strip_prefix('\u{FEFF}').unwrap_or(text)
|
||||
}
|
||||
|
||||
/// Sets a corrupt state file aside (copied, the original left in place) so the
|
||||
/// caller can fall back to defaults without silently destroying what was there.
|
||||
pub(crate) fn quarantine(path: &std::path::Path) {
|
||||
let aside = quarantine_path(path);
|
||||
match std::fs::copy(path, &aside) {
|
||||
Ok(_) => log::warn!("the previous contents were kept at {}", aside.display()),
|
||||
Err(e) => log::warn!("could not keep a copy at {}: {e}", aside.display()),
|
||||
}
|
||||
}
|
||||
|
||||
/// Like [`quarantine`], but moves the file out of the way — for files that
|
||||
/// cannot even be read, where copying would fail too.
|
||||
pub(crate) fn quarantine_by_rename(path: &std::path::Path) {
|
||||
let aside = quarantine_path(path);
|
||||
match std::fs::rename(path, &aside) {
|
||||
Ok(()) => log::warn!("the previous contents were moved to {}", aside.display()),
|
||||
Err(e) => log::warn!("could not move the file to {}: {e}", aside.display()),
|
||||
}
|
||||
}
|
||||
|
||||
fn quarantine_path(path: &std::path::Path) -> PathBuf {
|
||||
const MAX_QUARANTINED: u32 = 8;
|
||||
|
||||
let base = path.with_extension("json.corrupt");
|
||||
if !base.exists() {
|
||||
return base;
|
||||
}
|
||||
(1..MAX_QUARANTINED)
|
||||
.map(|n| path.with_extension(format!("json.corrupt.{n}")))
|
||||
.find(|candidate| !candidate.exists())
|
||||
.unwrap_or(base)
|
||||
}
|
||||
|
||||
pub fn write_atomic(path: &std::path::Path, bytes: &[u8]) -> std::io::Result<()> {
|
||||
write_atomic_mode(path, bytes, false)
|
||||
}
|
||||
|
||||
@@ -1158,7 +1158,7 @@ fn load_machine(path: &Path) -> Machine {
|
||||
Err(e) if e.kind() == io::ErrorKind::NotFound => return Machine::default(),
|
||||
Err(e) => {
|
||||
log::warn!("could not read {}; quarantining it: {e}", path.display());
|
||||
quarantine_by_rename(path);
|
||||
crate::core::config::quarantine_by_rename(path);
|
||||
return Machine::default();
|
||||
}
|
||||
};
|
||||
@@ -1171,7 +1171,7 @@ fn load_machine(path: &Path) -> Machine {
|
||||
}
|
||||
Err(e) => {
|
||||
log::warn!("{} does not parse ({e}); quarantining it", path.display());
|
||||
quarantine(path);
|
||||
crate::core::config::quarantine(path);
|
||||
Machine::default()
|
||||
}
|
||||
}
|
||||
@@ -1202,35 +1202,6 @@ pub(crate) fn withdraw_observations() {
|
||||
#[cfg(test)]
|
||||
pub(crate) static OBSERVE_SLOT: Mutex<()> = Mutex::new(());
|
||||
|
||||
fn quarantine(path: &Path) {
|
||||
let aside = quarantine_path(path);
|
||||
match std::fs::copy(path, &aside) {
|
||||
Ok(_) => log::warn!("the previous contents were kept at {}", aside.display()),
|
||||
Err(e) => log::warn!("could not keep a copy at {}: {e}", aside.display()),
|
||||
}
|
||||
}
|
||||
|
||||
fn quarantine_by_rename(path: &Path) {
|
||||
let aside = quarantine_path(path);
|
||||
match std::fs::rename(path, &aside) {
|
||||
Ok(()) => log::warn!("the previous contents were moved to {}", aside.display()),
|
||||
Err(e) => log::warn!("could not move the file to {}: {e}", aside.display()),
|
||||
}
|
||||
}
|
||||
|
||||
fn quarantine_path(path: &Path) -> PathBuf {
|
||||
const MAX_QUARANTINED: u32 = 8;
|
||||
|
||||
let base = path.with_extension("json.corrupt");
|
||||
if !base.exists() {
|
||||
return base;
|
||||
}
|
||||
(1..MAX_QUARANTINED)
|
||||
.map(|n| path.with_extension(format!("json.corrupt.{n}")))
|
||||
.find(|candidate| !candidate.exists())
|
||||
.unwrap_or(base)
|
||||
}
|
||||
|
||||
pub fn default_machine_path() -> io::Result<PathBuf> {
|
||||
Ok(data_dir()?.join(MACHINE_FILE))
|
||||
}
|
||||
|
||||
@@ -285,7 +285,14 @@ impl WindowViews {
|
||||
match serde_json::from_str(crate::core::config::strip_bom(&text)) {
|
||||
Ok(loaded) => Some(loaded),
|
||||
Err(e) => {
|
||||
log::warn!("failed to parse views at {}: {e}; ignoring", path.display());
|
||||
// The next `save` overwrites this file wholesale, so ignoring a
|
||||
// corrupt one quietly discards whatever it held. Keep a copy
|
||||
// aside first, the way `load_machine` does.
|
||||
log::warn!(
|
||||
"failed to parse views at {}: {e}; quarantining it",
|
||||
path.display()
|
||||
);
|
||||
crate::core::config::quarantine(&path);
|
||||
None
|
||||
}
|
||||
}
|
||||
@@ -418,6 +425,30 @@ mod tests {
|
||||
assert_eq!(loaded.active, Some(id));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_corrupt_views_file_is_kept_aside_before_being_ignored() {
|
||||
let _file = lock_session_file();
|
||||
let dir = pin_config_dir();
|
||||
let path = dir.join("views.json");
|
||||
let aside = dir.join("views.json.corrupt");
|
||||
std::fs::remove_file(&aside).ok();
|
||||
std::fs::write(&path, "{ not json").unwrap();
|
||||
|
||||
assert!(
|
||||
WindowViews::load().is_none(),
|
||||
"a corrupt file yields nothing rather than a guess"
|
||||
);
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(&aside).as_deref().ok(),
|
||||
Some("{ not json"),
|
||||
"the next save overwrites views.json wholesale, so the old contents \
|
||||
must already be parked beside it"
|
||||
);
|
||||
|
||||
std::fs::remove_file(&path).ok();
|
||||
std::fs::remove_file(&aside).ok();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn an_empty_or_partial_file_decodes_to_defaults() {
|
||||
let empty: WindowViews = serde_json::from_str("{}").unwrap();
|
||||
|
||||
@@ -9,6 +9,7 @@ pub(crate) mod remote;
|
||||
pub mod remote_link;
|
||||
pub mod router;
|
||||
pub mod server;
|
||||
pub mod singleton;
|
||||
pub mod spawn;
|
||||
pub mod ssh;
|
||||
pub mod transport;
|
||||
|
||||
@@ -165,6 +165,27 @@ macro_rules! startup_note {
|
||||
}
|
||||
|
||||
pub fn run_daemon() -> anyhow::Result<()> {
|
||||
// Before either endpoint, and before the machine tree is opened. Whoever
|
||||
// holds this is the server; everyone else stands down while it lives.
|
||||
//
|
||||
// The endpoints cannot decide this between themselves. They are bound in
|
||||
// sequence, so a second server can take one of them in the window before
|
||||
// the first takes the other, and the two then serve half a machine each —
|
||||
// panes on one socket, an empty pane registry answering the machine tree on
|
||||
// the other. That is how a workspace switch came to rebuild live sessions:
|
||||
// no error anywhere, just every pane reading as dead. See `singleton`.
|
||||
let _seat = match crate::daemon::singleton::claim() {
|
||||
crate::daemon::singleton::Claim::Held(seat) => Some(seat),
|
||||
crate::daemon::singleton::Claim::Taken => {
|
||||
startup_note!("tty7-server: another server already serves this config dir; exiting");
|
||||
return Ok(());
|
||||
}
|
||||
crate::daemon::singleton::Claim::Unavailable(why) => {
|
||||
startup_note!("tty7-server: starting without the single-server lock ({why})");
|
||||
None
|
||||
}
|
||||
};
|
||||
|
||||
let registry = Arc::new(Registry::new());
|
||||
|
||||
#[cfg(any(unix, windows))]
|
||||
@@ -200,10 +221,6 @@ pub fn control_services() -> crate::host::server::Services {
|
||||
}
|
||||
}
|
||||
|
||||
pub fn run() -> anyhow::Result<()> {
|
||||
run_with(Arc::new(Registry::new()))
|
||||
}
|
||||
|
||||
/// Say which pseudoconsole this daemon's panes will run on.
|
||||
///
|
||||
/// `portable-pty` loads a sideloaded `conpty.dll` if one sits beside the
|
||||
|
||||
@@ -0,0 +1,174 @@
|
||||
//! One server per config directory, decided by the kernel.
|
||||
//!
|
||||
//! Two servers against one config dir is not a degraded mode, it is a silent
|
||||
//! one. They do not fight over both endpoints — they split them. The one that
|
||||
//! keeps `control.sock` answers `MachineGet` out of its own pane registry,
|
||||
//! which is empty because the panes are all in the *other* process, so every
|
||||
//! pane in the machine tree reads as dead. Nothing logs an error; the window
|
||||
//! just quietly rebuilds every session it was asked to restore.
|
||||
//!
|
||||
//! That split was reachable because "is a server already running?" was answered
|
||||
//! by connecting to its socket and treating one failed connect as proof of
|
||||
//! death — after which the newcomer unlinked the endpoint and bound its own.
|
||||
//! A connect can fail for a server that is merely slow to reach `accept`, and
|
||||
//! the loser of that race never learns it was replaced: it holds a listener on
|
||||
//! an unlinked path and serves nobody, forever.
|
||||
//!
|
||||
//! An advisory lock has no such gap. Holding it is the definition of being the
|
||||
//! server, the kernel drops it when the holder dies however it dies, and there
|
||||
//! is no stale state to interpret — which is the property the socket probe
|
||||
//! could never have.
|
||||
//!
|
||||
//! Take it *before* either endpoint. Then a stale endpoint can only ever be
|
||||
//! removed by a process that already knows it is alone, and the removal is safe
|
||||
//! by construction rather than by timing.
|
||||
|
||||
use std::fs::File;
|
||||
use std::path::PathBuf;
|
||||
|
||||
/// Proof that this process is the server for its config directory.
|
||||
///
|
||||
/// Released when dropped, and by the kernel if the process never gets to drop
|
||||
/// it. Hold it for as long as the server serves.
|
||||
#[derive(Debug)]
|
||||
pub struct Singleton {
|
||||
_file: File,
|
||||
}
|
||||
|
||||
/// The outcome of asking to be the server.
|
||||
#[derive(Debug)]
|
||||
pub enum Claim {
|
||||
/// This process holds it. Serve.
|
||||
Held(Singleton),
|
||||
/// Another live server holds it. Stand down and talk to that one.
|
||||
Taken,
|
||||
/// The lock could not be evaluated (no config dir, unwritable path). The
|
||||
/// caller carries on unprotected rather than refusing to start: a server
|
||||
/// that will not run is worse than one that might race.
|
||||
Unavailable(String),
|
||||
}
|
||||
|
||||
fn lock_path() -> Option<PathBuf> {
|
||||
crate::core::config::config_path("daemon.lock")
|
||||
}
|
||||
|
||||
/// Claims the right to be this machine's server.
|
||||
pub fn claim() -> Claim {
|
||||
let Some(path) = lock_path() else {
|
||||
return Claim::Unavailable("no config directory".into());
|
||||
};
|
||||
if let Some(parent) = path.parent() {
|
||||
let _ = std::fs::create_dir_all(parent);
|
||||
}
|
||||
match open_exclusive(&path) {
|
||||
Ok(Some(file)) => Claim::Held(Singleton { _file: file }),
|
||||
Ok(None) => Claim::Taken,
|
||||
Err(e) => Claim::Unavailable(format!("{} could not be locked: {e}", path.display())),
|
||||
}
|
||||
}
|
||||
|
||||
/// `Ok(Some(file))` when the lock is ours, `Ok(None)` when someone else holds
|
||||
/// it, `Err` when the question could not be put to the kernel at all.
|
||||
#[cfg(unix)]
|
||||
fn open_exclusive(path: &std::path::Path) -> std::io::Result<Option<File>> {
|
||||
use std::os::unix::io::AsRawFd as _;
|
||||
|
||||
let file = File::options()
|
||||
.create(true)
|
||||
.read(true)
|
||||
.write(true)
|
||||
.truncate(false)
|
||||
.open(path)?;
|
||||
// LOCK_NB so a running server answers "taken" instead of parking this
|
||||
// process on a lock it will hold until it exits.
|
||||
let locked = unsafe { libc::flock(file.as_raw_fd(), libc::LOCK_EX | libc::LOCK_NB) };
|
||||
if locked == 0 {
|
||||
return Ok(Some(file));
|
||||
}
|
||||
let e = std::io::Error::last_os_error();
|
||||
match e.raw_os_error() {
|
||||
Some(libc::EWOULDBLOCK) => Ok(None),
|
||||
_ => Err(e),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
fn open_exclusive(path: &std::path::Path) -> std::io::Result<Option<File>> {
|
||||
use std::os::windows::fs::OpenOptionsExt as _;
|
||||
|
||||
// share_mode(0) is the whole mechanism: the file stays open for as long as
|
||||
// the server runs, and every other open of it fails until this handle is
|
||||
// closed — by `drop`, or by Windows when the process ends.
|
||||
match File::options()
|
||||
.create(true)
|
||||
.read(true)
|
||||
.write(true)
|
||||
.share_mode(0)
|
||||
.open(path)
|
||||
{
|
||||
Ok(file) => Ok(Some(file)),
|
||||
Err(e) if e.raw_os_error() == Some(32) => Ok(None), // ERROR_SHARING_VIOLATION
|
||||
Err(e) => Err(e),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(not(any(unix, windows)))]
|
||||
fn open_exclusive(_path: &std::path::Path) -> std::io::Result<Option<File>> {
|
||||
// No lock primitive here. `Ok(None)` would read as "taken" and stop the
|
||||
// server from ever starting; not being able to ask is `Unavailable`.
|
||||
Err(std::io::Error::new(
|
||||
std::io::ErrorKind::Unsupported,
|
||||
"no single-server lock on this platform",
|
||||
))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
/// `set_config_dir` is process-wide, so these cases cannot run beside each
|
||||
/// other: one would be claiming the very seat the other is asserting is
|
||||
/// free, and which failed would depend on the scheduler.
|
||||
static CONFIG_DIR: std::sync::Mutex<()> = std::sync::Mutex::new(());
|
||||
|
||||
fn pin_dir(name: &str) -> (PathBuf, std::sync::MutexGuard<'static, ()>) {
|
||||
let guard = CONFIG_DIR.lock().unwrap_or_else(|e| e.into_inner());
|
||||
let dir =
|
||||
std::env::temp_dir().join(format!("tty7-singleton-{}-{name}", std::process::id()));
|
||||
std::fs::create_dir_all(&dir).ok();
|
||||
crate::core::config::set_config_dir(dir.clone());
|
||||
(dir, guard)
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_second_claim_is_refused_while_the_first_is_held() {
|
||||
let _pinned = pin_dir("basic");
|
||||
let first = match claim() {
|
||||
Claim::Held(s) => s,
|
||||
other => panic!("the first claim must be granted, got {other:?}"),
|
||||
};
|
||||
assert!(
|
||||
matches!(claim(), Claim::Taken),
|
||||
"a second server must be told the seat is taken, not race for the endpoints"
|
||||
);
|
||||
drop(first);
|
||||
assert!(
|
||||
matches!(claim(), Claim::Held(_)),
|
||||
"releasing it hands the seat to the next server — no stale file to interpret"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_lock_left_behind_by_a_dead_holder_is_claimable() {
|
||||
let (dir, _guard) = pin_dir("stale");
|
||||
let path = dir.join("daemon.lock");
|
||||
// The file surviving is exactly what a killed server leaves. It must
|
||||
// not read as "taken" — that was the old socket probe's failure mode,
|
||||
// in reverse.
|
||||
std::fs::write(&path, b"").unwrap();
|
||||
assert!(
|
||||
matches!(claim(), Claim::Held(_)),
|
||||
"an unlocked file is not a holder, however it came to exist"
|
||||
);
|
||||
}
|
||||
}
|
||||
+56
-18
@@ -838,7 +838,7 @@ impl Tty7App {
|
||||
let answered = WorkspaceStore::machine_is_connected(cx, self.workspace);
|
||||
if self.tabs.is_empty()
|
||||
&& answered
|
||||
&& crate::ui::tree_sync::window_is_informed(cx, self.workspace)
|
||||
&& crate::ui::tree_sync::workspace_is_disposable(cx, self.workspace)
|
||||
{
|
||||
crate::ui::tree_sync::fire_workspace_op(cx, self.workspace, |ws| {
|
||||
tty7_core::daemon::control::ControlRequest::WorkspaceRemove { workspace: ws }
|
||||
@@ -939,7 +939,7 @@ impl Tty7App {
|
||||
}
|
||||
// Anything parked for the workspace we are leaving is now meaningless.
|
||||
self.pending_tab = None;
|
||||
if self.tabs.is_empty() && crate::ui::tree_sync::window_is_informed(cx, previous) {
|
||||
if self.tabs.is_empty() && crate::ui::tree_sync::workspace_is_disposable(cx, previous) {
|
||||
crate::ui::tree_sync::fire_workspace_op(cx, previous, |ws| {
|
||||
tty7_core::daemon::control::ControlRequest::WorkspaceRemove { workspace: ws }
|
||||
});
|
||||
@@ -999,7 +999,7 @@ impl Tty7App {
|
||||
pane_ws.as_ref(),
|
||||
self.workspace,
|
||||
&st.pane,
|
||||
&alive,
|
||||
alive.as_ref(),
|
||||
self.font_size,
|
||||
window,
|
||||
cx,
|
||||
@@ -5697,24 +5697,47 @@ fn pane_to_session(pane: &Pane, cx: &App) -> SessionPane {
|
||||
}
|
||||
}
|
||||
|
||||
/// The daemon's account of which panes are alive, or `None` when it could not
|
||||
/// be asked at all.
|
||||
///
|
||||
/// The distinction is the point: a pane absent from a *successful* listing is
|
||||
/// genuinely gone and may be respawned, while a failed `List` says nothing
|
||||
/// about any pane. Flattening the failure into an empty map made one transient
|
||||
/// RPC error read as "every pane is dead", and the restore then spawned fresh
|
||||
/// shells over all of them — the same destruction-by-inference this file's
|
||||
/// restore path is built to avoid.
|
||||
pub(crate) fn alive_panes_on(
|
||||
route: &crate::terminal::PaneRoute,
|
||||
) -> std::collections::HashMap<u64, Option<String>> {
|
||||
) -> Option<std::collections::HashMap<u64, Option<String>>> {
|
||||
if !matches!(route, crate::terminal::PaneRoute::Local) {
|
||||
return std::collections::HashMap::new();
|
||||
return Some(std::collections::HashMap::new());
|
||||
}
|
||||
match crate::terminal::RemoteTerminal::try_list_panes_on(route) {
|
||||
Ok(list) => Some(
|
||||
list.into_iter()
|
||||
.filter(|p| p.alive)
|
||||
.map(|p| (p.pane_id, p.owner))
|
||||
.collect(),
|
||||
),
|
||||
Err(e) => {
|
||||
log::warn!("could not list panes ({e}); leaving each attach to decide");
|
||||
None
|
||||
}
|
||||
}
|
||||
crate::terminal::RemoteTerminal::list_panes_on(route)
|
||||
.into_iter()
|
||||
.filter(|p| p.alive)
|
||||
.map(|p| (p.pane_id, p.owner))
|
||||
.collect()
|
||||
}
|
||||
|
||||
fn pane_attachable(
|
||||
alive: &std::collections::HashMap<u64, Option<String>>,
|
||||
alive: Option<&std::collections::HashMap<u64, Option<String>>>,
|
||||
id: u64,
|
||||
owner: crate::core::session::WorkspaceId,
|
||||
) -> bool {
|
||||
let Some(alive) = alive else {
|
||||
// No listing to consult. Attaching is the safe guess in both
|
||||
// directions: if the daemon is really unreachable the attach fails and
|
||||
// the pane falls to the fresh-spawn path anyway, while spawning fresh
|
||||
// on a hunch destroys a session that was merely hard to reach.
|
||||
return true;
|
||||
};
|
||||
match alive.get(&id) {
|
||||
None => false,
|
||||
Some(None) => true,
|
||||
@@ -5745,8 +5768,15 @@ fn tabs_from_session(
|
||||
let alive = alive_panes_on(&crate::terminal::PaneRoute::for_workspace(workspace));
|
||||
let mut tabs: Vec<Tab> = Vec::with_capacity(session.tabs.len());
|
||||
for st in &session.tabs {
|
||||
let Some(pane) = session_to_pane(workspace, owner, &st.pane, &alive, font_size, window, cx)
|
||||
else {
|
||||
let Some(pane) = session_to_pane(
|
||||
workspace,
|
||||
owner,
|
||||
&st.pane,
|
||||
alive.as_ref(),
|
||||
font_size,
|
||||
window,
|
||||
cx,
|
||||
) else {
|
||||
log::error!("dropping a restored tab: no pane in it could be started");
|
||||
continue;
|
||||
};
|
||||
@@ -5777,7 +5807,7 @@ fn session_to_pane(
|
||||
workspace: Option<&crate::terminal::PaneWorkspace>,
|
||||
owner: WorkspaceId,
|
||||
sp: &SessionPane,
|
||||
alive: &std::collections::HashMap<u64, Option<String>>,
|
||||
alive: Option<&std::collections::HashMap<u64, Option<String>>>,
|
||||
font_size: f32,
|
||||
window: &mut Window,
|
||||
cx: &mut Context<Tty7App>,
|
||||
@@ -6448,19 +6478,27 @@ mod tests {
|
||||
.into_iter()
|
||||
.collect();
|
||||
|
||||
assert!(pane_attachable(&alive, 1, ours), "our own pane attaches");
|
||||
assert!(
|
||||
!pane_attachable(&alive, 2, ours),
|
||||
pane_attachable(Some(&alive), 1, ours),
|
||||
"our own pane attaches"
|
||||
);
|
||||
assert!(
|
||||
!pane_attachable(Some(&alive), 2, ours),
|
||||
"another workspace's pane must spawn fresh instead"
|
||||
);
|
||||
assert!(
|
||||
pane_attachable(&alive, 3, ours),
|
||||
pane_attachable(Some(&alive), 3, ours),
|
||||
"an unowned pane is legacy"
|
||||
);
|
||||
assert!(
|
||||
!pane_attachable(&alive, 4, ours),
|
||||
!pane_attachable(Some(&alive), 4, ours),
|
||||
"a dead id never attaches"
|
||||
);
|
||||
assert!(
|
||||
pane_attachable(None, 4, ours),
|
||||
"a failed List says nothing about pane 4; the attach itself must decide, \
|
||||
because respawning on a transient RPC error destroys a live session"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
+177
-13
@@ -770,10 +770,27 @@ fn take_rehydrate(cx: &mut App, client_ws: WorkspaceId, window_is_empty: bool) -
|
||||
(window_is_empty || adopt == Adopt::IfEmpty).then_some(adopt)
|
||||
}
|
||||
|
||||
pub(crate) fn window_is_informed(cx: &App, client_ws: WorkspaceId) -> bool {
|
||||
cx.try_global::<TreeSync>()
|
||||
/// Whether a window with no tabs may delete `client_ws` outright — from the
|
||||
/// machine's tree and from the store both.
|
||||
///
|
||||
/// Two independent things have to agree, because the window's own emptiness
|
||||
/// cannot tell them apart: a workspace is empty when it genuinely holds
|
||||
/// nothing, and equally when its layout failed to rebuild. Only the first is a
|
||||
/// reason to delete anything, and the second has already cost a workspace with
|
||||
/// ten live tabs in it.
|
||||
///
|
||||
/// So the window must be informed (it pulled a layout and put it up), *and* the
|
||||
/// mirror — the machine's own account, which no local failure can empty — must
|
||||
/// agree there is nothing there. An unprimed mirror knows nothing and answers
|
||||
/// no: "I don't know" may never authorize a deletion.
|
||||
pub(crate) fn workspace_is_disposable(cx: &App, client_ws: WorkspaceId) -> bool {
|
||||
let Some(state) = cx
|
||||
.try_global::<TreeSync>()
|
||||
.and_then(|t| t.windows.get(&client_ws))
|
||||
.is_some_and(|s| s.informed)
|
||||
else {
|
||||
return false;
|
||||
};
|
||||
state.informed && matches!(&state.sync, SyncPhase::Primed(mirror) if mirror.tabs.is_empty())
|
||||
}
|
||||
|
||||
pub(crate) fn mark_window_informed(cx: &mut App, client_ws: WorkspaceId) {
|
||||
@@ -1122,7 +1139,6 @@ fn session_pane_from_node(node: &PaneNode, panes: &[PaneRecord]) -> SessionPane
|
||||
match node {
|
||||
PaneNode::Leaf { pane } => {
|
||||
let record = panes.iter().find(|p| p.id == *pane);
|
||||
let live = record.is_some_and(|r| r.live);
|
||||
let (cwd, ssh_spec, agent) = match record {
|
||||
Some(r) => (
|
||||
r.cwd.clone().map(std::path::PathBuf::from),
|
||||
@@ -1133,7 +1149,21 @@ fn session_pane_from_node(node: &PaneNode, panes: &[PaneRecord]) -> SessionPane
|
||||
};
|
||||
SessionPane::Leaf {
|
||||
cwd,
|
||||
pane_id: live.then_some(*pane),
|
||||
// The id goes down whatever `live` says. That flag is a cached
|
||||
// fact about another process, written by whoever last observed
|
||||
// the pane and reloaded from disk as `false` on every server
|
||||
// start — so a quiet pane that nobody has observed since reads
|
||||
// as dead while its shell is very much alive. Believing it here
|
||||
// is what threw away live sessions on a workspace switch: the
|
||||
// id was erased, and the restore below had nothing to attach
|
||||
// to, so it spawned a fresh shell over a running one.
|
||||
//
|
||||
// Attaching is the thing that actually knows. `spawn_shell_
|
||||
// terminal_in` attaches when the pane is there and spawns fresh
|
||||
// when it is not, which is the same answer this filter was
|
||||
// trying to guess — except it is right. `live` stays a hint for
|
||||
// what to show, never the judge of what to destroy.
|
||||
pane_id: Some(*pane),
|
||||
ssh_spec,
|
||||
agent: agent.as_ref().map(|a| a.agent),
|
||||
agent_session_id: agent.as_ref().and_then(|a| a.session_id.clone()),
|
||||
@@ -1370,16 +1400,39 @@ fn finish_hydration(
|
||||
let Some(handle) = crate::ui::windows::WindowRegistry::window_for(cx, client_ws) else {
|
||||
return;
|
||||
};
|
||||
log::info!(
|
||||
"rebuilding {} tab(s) of workspace {client_ws} from its machine's tree",
|
||||
session.tabs.len()
|
||||
);
|
||||
mark_window_informed(cx, client_ws);
|
||||
let wanted = session.tabs.len();
|
||||
log::info!("rebuilding {wanted} tab(s) of workspace {client_ws} from its machine's tree");
|
||||
let _ = handle.update(cx, move |_, window, cx| {
|
||||
app.update(cx, |app, cx| {
|
||||
app.adopt_workspace(client_ws, session, window, cx)
|
||||
});
|
||||
});
|
||||
|
||||
// Informed *after* the rebuild, and only if the rebuild produced something.
|
||||
//
|
||||
// The licence means "this window knows what belongs in this workspace", and
|
||||
// `switch_workspace` / `detach_workspace` read it as permission to delete a
|
||||
// workspace that has no tabs — from the machine tree and from the store
|
||||
// both. Granting it before the rebuild handed that permission to a window
|
||||
// whose rebuild had not happened yet, and a rebuild can produce nothing:
|
||||
// `tabs_from_session` drops any tab whose panes all fail to start, which is
|
||||
// what every tab does when the pane socket is unreachable. The window then
|
||||
// sat there, empty and authoritative, and the next switch deleted a
|
||||
// workspace with ten live tabs in it.
|
||||
//
|
||||
// Emptiness that came from a failure has to stay indistinguishable from not
|
||||
// knowing, because that is what it is.
|
||||
let rebuilt = crate::ui::windows::WindowRegistry::app_for(cx, client_ws)
|
||||
.and_then(|app| app.upgrade())
|
||||
.is_some_and(|app| !app.read(cx).tabs.is_empty());
|
||||
if rebuilt || wanted == 0 {
|
||||
mark_window_informed(cx, client_ws);
|
||||
} else {
|
||||
log::warn!(
|
||||
"workspace {client_ws}: none of its {wanted} tab(s) could be rebuilt; leaving the \
|
||||
window uninformed so the layout is not mistaken for an empty workspace"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn on_layout_delta(cx: &mut App, host: HostId, key: &str, delta: LayoutDelta) {
|
||||
@@ -1824,6 +1877,72 @@ mod tests {
|
||||
});
|
||||
}
|
||||
|
||||
/// The rule that stops a failed rebuild from being read as "empty".
|
||||
///
|
||||
/// A window with no tabs may delete its workspace outright — tree and store
|
||||
/// both — so the two ways of having no tabs must not look alike. Genuinely
|
||||
/// empty is a reason; "the panes would not start" is not, and it is what
|
||||
/// every tab looks like when the pane socket has gone away.
|
||||
#[gpui::test]
|
||||
fn only_a_mirror_that_agrees_lets_an_empty_window_delete_its_workspace(
|
||||
cx: &mut gpui::TestAppContext,
|
||||
) {
|
||||
cx.update(|cx| {
|
||||
let ws = WorkspaceId::new();
|
||||
let set = |cx: &mut App, informed: bool, sync: SyncPhase| {
|
||||
let state = cx
|
||||
.default_global::<TreeSync>()
|
||||
.windows
|
||||
.entry(ws)
|
||||
.or_default();
|
||||
state.informed = informed;
|
||||
state.sync = sync;
|
||||
};
|
||||
let unprimed = || SyncPhase::Unprimed {
|
||||
dirty: false,
|
||||
priming: false,
|
||||
};
|
||||
let primed_with =
|
||||
|tabs: Vec<TreeTab>| SyncPhase::Primed(WsMirror { tabs, active: None });
|
||||
let a_tab = || TreeTab {
|
||||
id: TabId::new(),
|
||||
name: None,
|
||||
sidebar_group: None,
|
||||
root: PaneNode::Leaf { pane: 1 },
|
||||
};
|
||||
|
||||
assert!(
|
||||
!workspace_is_disposable(cx, WorkspaceId::new()),
|
||||
"a workspace nothing is tracking is not a workspace to delete"
|
||||
);
|
||||
|
||||
set(cx, true, unprimed());
|
||||
assert!(
|
||||
!workspace_is_disposable(cx, ws),
|
||||
"an unpulled mirror knows nothing, and not knowing must never authorize this"
|
||||
);
|
||||
|
||||
set(cx, true, primed_with(vec![a_tab()]));
|
||||
assert!(
|
||||
!workspace_is_disposable(cx, ws),
|
||||
"this is the regression: the window came up empty because the rebuild failed, \
|
||||
while the machine still held the tabs. Deleting here destroyed them."
|
||||
);
|
||||
|
||||
set(cx, false, primed_with(vec![]));
|
||||
assert!(
|
||||
!workspace_is_disposable(cx, ws),
|
||||
"a window that never put up a layout does not get to say what belongs here"
|
||||
);
|
||||
|
||||
set(cx, true, primed_with(vec![]));
|
||||
assert!(
|
||||
workspace_is_disposable(cx, ws),
|
||||
"informed, and the machine agrees it holds nothing — the one case that is"
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
fn a_hydration_that_died_on_a_stale_link_is_owed_back(cx: &mut gpui::TestAppContext) {
|
||||
cx.update(|cx| {
|
||||
@@ -2550,7 +2669,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_live_leaf_keeps_its_pane_id_and_a_dead_one_lowers_to_a_revival_leaf() {
|
||||
fn a_lowered_leaf_carries_its_pane_id_and_its_agent_whatever_live_says() {
|
||||
use tty7_core::core::cli_agent::CLIAgent;
|
||||
let tab_id = TabId::new();
|
||||
let ws = tty7_core::core::machine::Workspace {
|
||||
@@ -2619,8 +2738,12 @@ mod tests {
|
||||
..
|
||||
} => {
|
||||
assert_eq!(
|
||||
*pane_id, None,
|
||||
"a dead pane's leaf takes the fresh-spawn path — that is the revival"
|
||||
*pane_id,
|
||||
Some(2),
|
||||
"a pane the tree calls dead still goes down by its id: the flag is a \
|
||||
cached observation from another process — reloaded as false on every \
|
||||
server start — and attaching is what settles it. Believing the flag \
|
||||
here spawned fresh shells over running sessions."
|
||||
);
|
||||
assert_eq!(cwd.as_deref(), Some(std::path::Path::new("/work/api")));
|
||||
assert_eq!(*agent, Some(CLIAgent::Claude));
|
||||
@@ -2630,6 +2753,47 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
/// The regression behind "switching workspaces threw away every session".
|
||||
///
|
||||
/// Two servers had started against one config dir — one holding the control
|
||||
/// socket with an empty pane registry, the other holding the panes — so
|
||||
/// `MachineGet` answered with every `live` still `false`, the value
|
||||
/// `load_machine` stamps on a cold read. Erasing the id on that made the
|
||||
/// restore spawn a fresh shell over each running one, and nineteen live
|
||||
/// agent sessions went out with it.
|
||||
///
|
||||
/// The id has to survive a `live: false`, because nothing here is entitled
|
||||
/// to declare a pane dead. Attaching is.
|
||||
#[test]
|
||||
fn a_pane_the_tree_calls_dead_still_goes_down_by_its_id() {
|
||||
let tab_id = TabId::new();
|
||||
let ws = tty7_core::core::machine::Workspace {
|
||||
tabs: vec![TreeTab {
|
||||
id: tab_id,
|
||||
name: None,
|
||||
sidebar_group: None,
|
||||
root: PaneNode::Leaf { pane: 7 },
|
||||
}],
|
||||
active_tab: Some(tab_id),
|
||||
..Default::default()
|
||||
};
|
||||
let panes = vec![PaneRecord {
|
||||
id: 7,
|
||||
cwd: Some("/work".into()),
|
||||
live: false,
|
||||
..PaneRecord::new(7)
|
||||
}];
|
||||
|
||||
match &session_from_tree(&ws, &panes).tabs[0].pane {
|
||||
SessionPane::Leaf { pane_id, .. } => assert_eq!(
|
||||
*pane_id,
|
||||
Some(7),
|
||||
"the attach decides whether pane 7 is still there; this must not pre-empt it"
|
||||
),
|
||||
_ => panic!("leaf"),
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_dangling_active_tab_in_the_pulled_tree_falls_back_to_the_first() {
|
||||
let ws = tty7_core::core::machine::Workspace {
|
||||
|
||||
Reference in New Issue
Block a user