Stabilize terminal frame recovery

Cargo.lock

@@ -436,7 +436,7 @@ dependencies = [
 
 [[package]]
 name = "dosh"
-version = "0.1.11"
+version = "0.1.14"
 dependencies = [
  "anyhow",
  "base64",

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "dosh"
-version = "0.1.11"
+version = "0.1.14"
 edition = "2024"
 license = "MIT"
 

install.sh

@@ -429,15 +429,6 @@ ExecStart=$bindir/dosh-server serve
 Restart=on-failure
 RestartSec=1
 KillMode=process
-NoNewPrivileges=true
-PrivateTmp=true
-ProtectSystem=strict
-ProtectHome=read-only
-ReadWritePaths=$config_dir $data_dir
-RestrictAddressFamilies=AF_INET AF_INET6 AF_UNIX
-RestrictRealtime=true
-LockPersonality=true
-MemoryDenyWriteExecute=true
 
 [Install]
 WantedBy=default.target

packaging/systemd/dosh-server.service

@@ -9,15 +9,6 @@ ExecStart=%h/.local/bin/dosh-server serve
 Restart=on-failure
 RestartSec=1
 KillMode=process
-NoNewPrivileges=true
-PrivateTmp=true
-ProtectSystem=strict
-ProtectHome=read-only
-ReadWritePaths=%h/.config/dosh %h/.local/share/dosh
-RestrictAddressFamilies=AF_INET AF_INET6 AF_UNIX
-RestrictRealtime=true
-LockPersonality=true
-MemoryDenyWriteExecute=true
 
 [Install]
 WantedBy=default.target

src/bin/dosh-client.rs

@@ -4271,7 +4271,6 @@ async fn run_terminal(
     let mut status_tick = tokio::time::interval(Duration::from_secs(1));
     let mut resize_tick = tokio::time::interval(Duration::from_millis(250));
     let mut stream_retransmit_tick = tokio::time::interval(Duration::from_millis(200));
-    let mut frame_gap_tick = tokio::time::interval(Duration::from_millis(100));
     let mut last_size = terminal_size();
     // React to terminal resize the instant it happens via SIGWINCH (mosh-style),
     // instead of waiting up to one `resize_tick`. The 250ms poll below stays as a
@@ -4499,36 +4498,6 @@ async fn run_terminal(
             _ = resize_tick.tick() => {
                 maybe_send_resize(&socket, addr, &cred, &mut send_seq, &mut last_size).await?;
             }
-            _ = frame_gap_tick.tick() => {
-                if frame_buffer.gap_wait_elapsed(Duration::from_millis(FRAME_GAP_RESYNC_AFTER_MS))
-                    && let Some(frame) = reconnect(
-                        &socket,
-                        &mut cred,
-                        &mut send_seq,
-                        last_size,
-                        &mut frame_buffer,
-                        &mut predictor,
-                    )
-                    .await?
-                {
-                    refresh_live_addr(&mut addr, &cred)?;
-                    if !forward_only {
-                        render_frame(&frame)?;
-                        predictor.observe_output(&frame.bytes);
-                    }
-                    last_packet_at = Instant::now();
-                    flush_pending_user_input(
-                        &socket,
-                        addr,
-                        &cred,
-                        &mut send_seq,
-                        &mut predictor,
-                        &mut pending_user_input,
-                        &mut pending_user_input_bytes,
-                    )
-                    .await?;
-                }
-            }
             recv = socket.recv_from(&mut recv_buf) => {
                 let (n, _) = recv?;
                 let Ok(packet) = protocol::decode(&recv_buf[..n]) else {
@@ -6592,18 +6561,14 @@ fn contains_bytes(haystack: &[u8], needle: &[u8]) -> bool {
         .any(|window| window == needle)
 }
 
-const FRAME_GAP_RESYNC_AFTER_MS: u64 = 500;
-
 #[derive(Default)]
 struct FrameBuffer {
     pending: BTreeMap<u64, Frame>,
-    gap_since: Option<Instant>,
 }
 
 impl FrameBuffer {
     fn clear(&mut self) {
         self.pending.clear();
-        self.gap_since = None;
     }
 
     fn accept(&mut self, frame: Frame, last_rendered_seq: &mut u64) -> Vec<Frame> {
@@ -6612,7 +6577,6 @@ impl FrameBuffer {
                 return Vec::new();
             }
             self.pending.clear();
-            self.gap_since = None;
             *last_rendered_seq = frame.output_seq;
             return vec![frame];
         }
@@ -6626,30 +6590,8 @@ impl FrameBuffer {
             *last_rendered_seq = frame.output_seq;
             ready.push(frame);
         }
-        self.refresh_gap(*last_rendered_seq);
         ready
     }
-
-    fn refresh_gap(&mut self, last_rendered_seq: u64) {
-        let waiting_on_missing = self
-            .pending
-            .first_key_value()
-            .is_some_and(|(&seq, _)| seq > last_rendered_seq.saturating_add(1));
-        if waiting_on_missing {
-            self.gap_since.get_or_insert_with(Instant::now);
-        } else {
-            self.gap_since = None;
-        }
-    }
-
-    fn gap_wait_elapsed(&self, wait: Duration) -> bool {
-        self.gap_since.is_some_and(|since| since.elapsed() >= wait)
-    }
-
-    #[cfg(test)]
-    fn backdate_gap_for_test(&mut self, elapsed: Duration) {
-        self.gap_since = Some(Instant::now() - elapsed);
-    }
 }
 
 /// Read the current terminal size and, if it changed since `last_size`, send a
@@ -7061,10 +7003,10 @@ const TERMINAL_CLEANUP: &[u8] = concat!(
 #[cfg(test)]
 mod tests {
     use super::{
-        CachedCredential, DisconnectStatus, DynamicForward, FRAME_GAP_RESYNC_AFTER_MS, FrameBuffer,
-        LocalForward, MAX_PENDING_USER_INPUT_BYTES, POST_SUBMIT_ALL_INPUT_HOLD, PendingStreamOpen,
-        PredictMode, Predictor, RESTART_STATUS_SCRIPT, RemoteForward, STARTUP_INPUT_HOLD,
-        SshConfig, StartupGateMode, StatusAction, auth_allows, cache_key, cache_server_prefix,
+        CachedCredential, DisconnectStatus, DynamicForward, FrameBuffer, LocalForward,
+        MAX_PENDING_USER_INPUT_BYTES, POST_SUBMIT_ALL_INPUT_HOLD, PendingStreamOpen, PredictMode,
+        Predictor, RESTART_STATUS_SCRIPT, RemoteForward, STARTUP_INPUT_HOLD, SshConfig,
+        StartupGateMode, StatusAction, auth_allows, cache_key, cache_server_prefix,
         clear_cached_credentials, ensure_tui_safe_status_overlay, input_matches_escape,
         is_local_status_target, is_resume_response_for_client, latest_release_download_url,
         load_first_native_identity_with_prompt, parse_dynamic_forward, parse_escape_key,
@@ -7483,14 +7425,12 @@ mod tests {
 
         assert!(buffer.accept(test_frame(12, false), &mut last).is_empty());
         assert_eq!(last, 10);
-        assert!(!buffer.gap_wait_elapsed(Duration::from_secs(60)));
 
         let ready = buffer.accept(test_frame(11, false), &mut last);
         assert_eq!(ready.len(), 2);
         assert_eq!(ready[0].output_seq, 11);
         assert_eq!(ready[1].output_seq, 12);
         assert_eq!(last, 12);
-        assert!(!buffer.gap_wait_elapsed(Duration::from_secs(0)));
     }
 
     #[test]
@@ -7509,28 +7449,39 @@ mod tests {
         let mut last = 10;
 
         assert!(buffer.accept(test_frame(12, false), &mut last).is_empty());
-        buffer.backdate_gap_for_test(Duration::from_secs(10));
-        assert!(buffer.gap_wait_elapsed(Duration::from_millis(1)));
         let ready = buffer.accept(test_frame(20, true), &mut last);
 
         assert_eq!(ready.len(), 1);
         assert_eq!(ready[0].output_seq, 20);
         assert_eq!(last, 20);
-        assert!(!buffer.gap_wait_elapsed(Duration::from_millis(1)));
         assert!(buffer.accept(test_frame(12, false), &mut last).is_empty());
     }
 
     #[test]
-    fn frame_buffer_marks_gap_ready_for_resync_after_wait() {
+    fn frame_buffer_keeps_gap_buffered_without_snapshot_resync() {
         let mut buffer = FrameBuffer::default();
         let mut last = 10;
 
         assert!(buffer.accept(test_frame(13, false), &mut last).is_empty());
         assert_eq!(last, 10);
-        assert!(!buffer.gap_wait_elapsed(Duration::from_secs(60)));
+        assert!(buffer.accept(test_frame(14, false), &mut last).is_empty());
+        assert_eq!(last, 10);
 
-        buffer.backdate_gap_for_test(Duration::from_secs(1));
-        assert!(buffer.gap_wait_elapsed(Duration::from_millis(FRAME_GAP_RESYNC_AFTER_MS)));
+        let ready = buffer.accept(test_frame(11, false), &mut last);
+        assert_eq!(ready.len(), 1);
+        assert_eq!(ready[0].output_seq, 11);
+        assert_eq!(last, 11);
+
+        let ready = buffer.accept(test_frame(12, false), &mut last);
+        assert_eq!(ready.len(), 3);
+        assert_eq!(
+            ready
+                .iter()
+                .map(|frame| frame.output_seq)
+                .collect::<Vec<_>>(),
+            vec![12, 13, 14]
+        );
+        assert_eq!(last, 14);
     }
 
     #[test]

src/bin/dosh-server.rs

@@ -512,12 +512,13 @@ impl ServerState {
 
     /// Whether a session named `name` should be backed by a persistent holder.
     ///
-    /// With persistence enabled, every PTY session gets a detached holder. That
-    /// includes implicit `term-<millis>-<pid>` sessions: a reconnecting client has
-    /// the exact session name in its ticket cache, so it can reattach after a
-    /// quick server restart without forcing users to name sessions manually.
-    fn should_persist_session(&self, _name: &str) -> bool {
-        self.config.persist_sessions
+    /// With persistence enabled, explicitly named sessions get a detached
+    /// holder. Implicit `term-<millis>-<pid>` sessions are one-off terminals
+    /// created by `dosh host`; users cannot intentionally reattach to them by
+    /// name, so persisting them only leaves stale holders that can be
+    /// accidentally re-adopted after restarts.
+    fn should_persist_session(&self, name: &str) -> bool {
+        self.config.persist_sessions && !protocol::is_implicit_session_name(name)
     }
 
     /// Spawn (or, in the persistent case, spawn-and-adopt) a PTY for a session.
@@ -605,6 +606,20 @@ impl ServerState {
             if self.sessions.contains_key(&name) {
                 continue;
             }
+            if !self.should_persist_session(&name) {
+                eprintln!(
+                    "discarding stale implicit persistent session {name} (shell pid {})",
+                    meta.shell_pid
+                );
+                persist::request_shutdown(&sessions_dir, &name, None);
+                if meta.shell_pid > 0 {
+                    let _ = unsafe { libc::kill(meta.shell_pid, libc::SIGHUP) };
+                    let _ = unsafe { libc::kill(meta.shell_pid, libc::SIGTERM) };
+                    std::thread::sleep(Duration::from_millis(50));
+                    let _ = unsafe { libc::kill(meta.shell_pid, libc::SIGKILL) };
+                }
+                continue;
+            }
             let (pty, control) = match self.adopt_persistent_pty(&name) {
                 Ok(pair) => pair,
                 Err(err) => {
@@ -3504,11 +3519,9 @@ async fn retransmit_pending(
                     if pending.output_seq <= client.last_acked {
                         continue;
                     }
-                    if now.duration_since(pending.last_sent) >= Duration::from_millis(200)
-                        && pending.attempts < 8
-                    {
+                    if now.duration_since(pending.last_sent) >= Duration::from_millis(200) {
                         pending.last_sent = now;
-                        pending.attempts += 1;
+                        pending.attempts = pending.attempts.saturating_add(1);
                         sends.push((client.endpoint, pending.packet.clone()));
                     }
                 }
@@ -3841,7 +3854,7 @@ mod tests {
     use super::*;
 
     #[test]
-    fn persists_all_sessions_when_enabled() {
+    fn persists_named_sessions_when_enabled() {
         let (pty_tx, _rx) = mpsc::unbounded_channel();
         let config = ServerConfig {
             persist_sessions: true,
@@ -3851,7 +3864,7 @@ mod tests {
         let state = ServerState::new(config, [0u8; 32], pty_tx);
         assert!(state.should_persist_session("default")); // prewarmed
         assert!(state.should_persist_session("work")); // explicitly named
-        assert!(state.should_persist_session("term-1781470634216-76685")); // implicit reconnect
+        assert!(!state.should_persist_session("term-1781470634216-76685")); // one-off terminal
     }
 
     #[test]

src/persist.rs

@@ -435,6 +435,10 @@ pub fn run_holder(
         let mut cmd = [0u8; 1];
         match stream.read(&mut cmd) {
             Ok(1) if cmd[0] == HOLDER_CMD_SHUTDOWN => {
+                if shell_pid > 0 {
+                    let _ = unsafe { libc::kill(shell_pid, libc::SIGHUP) };
+                    let _ = unsafe { libc::kill(shell_pid, libc::SIGTERM) };
+                }
                 let _ = std::fs::remove_dir_all(runtime_dir);
                 std::process::exit(0);
             }

tests/integration_smoke.rs

@@ -1392,18 +1392,10 @@ fn server_retransmits_unacked_output_frames() {
     );
 
     let mut seen = Vec::new();
-    let mut duplicate = None;
-    let deadline = std::time::Instant::now() + Duration::from_secs(3);
+    let deadline = std::time::Instant::now() + Duration::from_secs(4);
     while std::time::Instant::now() < deadline {
         if let Some((header, frame)) = recv_frame(&socket, &bootstrap.session_key) {
             if String::from_utf8_lossy(&frame.bytes).contains("DOSH_RETRANSMIT") {
-                if seen
-                    .iter()
-                    .any(|(_, output_seq)| *output_seq == frame.output_seq)
-                {
-                    duplicate = Some((header.seq, frame.output_seq));
-                    break;
-                }
                 seen.push((header.seq, frame.output_seq));
             }
         }
@@ -1412,9 +1404,18 @@ fn server_retransmits_unacked_output_frames() {
     let _ = server.kill();
     let _ = server.wait();
 
+    let max_same_output_seq = seen
+        .iter()
+        .map(|(_, output_seq)| {
+            seen.iter()
+                .filter(|(_, candidate)| candidate == output_seq)
+                .count()
+        })
+        .max()
+        .unwrap_or(0);
     assert!(
-        duplicate.is_some(),
-        "expected retransmitted same output seq, seen={seen:?}"
+        max_same_output_seq >= 10,
+        "expected retransmission to continue past the old 8-attempt ceiling, seen={seen:?}"
     );
 }
 
@@ -2518,6 +2519,9 @@ fn write_persistent_server_config(dir: &tempfile::TempDir, port: u16) -> std::pa
     let mut raw = fs::read_to_string(&config).unwrap();
     // The base writer hardcodes `persist_sessions = false`; flip it on.
     raw = raw.replace("persist_sessions = false", "persist_sessions = true");
+    // Persistence tests attach the sessions they need explicitly. Leaving the
+    // base prewarm on here creates a detached default holder in every test.
+    raw = raw.replace("prewarm_sessions = [\"default\"]", "prewarm_sessions = []");
     fs::write(&config, raw).unwrap();
     config
 }
@@ -2553,6 +2557,45 @@ fn kill_holder(sessions_dir: &Path, session: &str) {
         .status();
 }
 
+/// Create a holder the way older Dosh builds did for implicit sessions, so
+/// startup migration can be tested without depending on the new spawn policy.
+fn spawn_legacy_holder(sessions_dir: &Path, session: &str) {
+    let server = env!("CARGO_BIN_EXE_dosh-server");
+    let runtime_dir = dosh::persist::ensure_runtime_dir(sessions_dir, session).unwrap();
+    let mut launcher = Command::new(server)
+        .arg("hold")
+        .arg("--runtime-dir")
+        .arg(&runtime_dir)
+        .arg("--session")
+        .arg(session)
+        .arg("--shell")
+        .arg("/bin/sh")
+        .arg("--cols")
+        .arg("80")
+        .arg("--rows")
+        .arg("24")
+        .stdout(Stdio::null())
+        .stderr(Stdio::null())
+        .spawn()
+        .unwrap();
+    let deadline = std::time::Instant::now() + Duration::from_secs(5);
+    while std::time::Instant::now() < deadline {
+        if holder_shell_pid(sessions_dir, session).is_some()
+            && runtime_dir.join("holder.sock").exists()
+        {
+            let _ = launcher.wait();
+            return;
+        }
+        if let Some(status) = launcher.try_wait().unwrap() {
+            panic!("legacy holder launcher exited before ready: {status}");
+        }
+        thread::sleep(Duration::from_millis(20));
+    }
+    let _ = launcher.kill();
+    let _ = launcher.wait();
+    panic!("legacy holder did not become ready");
+}
+
 /// Send one encrypted Input line and give the shell a moment to act.
 fn type_line(socket: &UdpSocket, port: u16, ok: &AttachOk, key: &[u8; 32], seq: u64, line: &str) {
     let input = Input {
@@ -2690,7 +2733,7 @@ fn session_survives_server_restart_same_shell_and_screen() {
 }
 
 #[test]
-fn implicit_session_survives_server_restart_for_same_ticket_holder() {
+fn implicit_session_does_not_create_restart_holder() {
     let dir = tempfile::tempdir().unwrap();
     let sessions_dir = dir.path().join("sessions");
     let port = free_udp_port();
@@ -2713,20 +2756,14 @@ fn implicit_session_survives_server_restart_for_same_ticket_holder() {
 
     let shell_pid_before = holder_shell_pid(&sessions_dir, &session);
     assert!(
-        shell_pid_before.is_some(),
-        "implicit sessions should get a persistent holder when persistence is enabled"
+        shell_pid_before.is_none(),
+        "implicit sessions must not get persistent holders when persistence is enabled"
     );
 
     let _ = server.kill();
     let _ = server.wait();
     thread::sleep(Duration::from_millis(300));
 
-    let pid = shell_pid_before.unwrap();
-    assert!(
-        unsafe { libc::kill(pid, 0) } == 0,
-        "implicit session shell pid {pid} must survive server restart"
-    );
-
     let mut server = start_server(&dir, &config);
     let (socket2, bootstrap2, ok2) = direct_attach_session(&config, port, "read-write", &session);
     let key2 = bootstrap2.session_key;
@@ -2743,15 +2780,44 @@ fn implicit_session_survives_server_restart_for_same_ticket_holder() {
 
     let _ = server.kill();
     let _ = server.wait();
-    kill_holder(&sessions_dir, &session);
 
-    assert_eq!(
-        shell_pid_after, shell_pid_before,
-        "implicit session should re-adopt the same shell pid"
+    assert!(
+        shell_pid_after.is_none(),
+        "implicit restart attach must stay ephemeral"
     );
     assert!(
-        post.contains("IMPLICIT_MARK=implicit_restart_42"),
-        "implicit session state must survive restart, got {post:?}"
+        post.contains("IMPLICIT_MARK=") && !post.contains("implicit_restart_42"),
+        "implicit session must start fresh after server restart, got {post:?}"
+    );
+}
+
+#[test]
+fn startup_discards_legacy_implicit_holders() {
+    let dir = tempfile::tempdir().unwrap();
+    let sessions_dir = dir.path().join("sessions");
+    let port = free_udp_port();
+    let config = write_persistent_server_config(&dir, port);
+    let session = protocol::generate_implicit_session_name();
+
+    spawn_legacy_holder(&sessions_dir, &session);
+    let shell_pid = holder_shell_pid(&sessions_dir, &session).expect("legacy holder shell pid");
+    assert!(
+        unsafe { libc::kill(shell_pid, 0) } == 0,
+        "legacy holder shell must be live before startup"
+    );
+
+    let mut server = start_server(&dir, &config);
+    thread::sleep(Duration::from_millis(300));
+
+    let _ = server.kill();
+    let _ = server.wait();
+    assert!(
+        holder_shell_pid(&sessions_dir, &session).is_none(),
+        "startup must remove old implicit holder metadata"
+    );
+    assert!(
+        unsafe { libc::kill(shell_pid, 0) } != 0,
+        "startup must shut down old implicit holder shell"
     );
 }
 

tests/release_scripts.rs

@@ -18,3 +18,31 @@ fn release_scripts_skip_stale_artifacts_when_auto_discovering() {
     assert!(publish.contains("skipping stale artifact"));
     assert!(publish.contains("actual=\"$(artifact_version \"$artifact\" || true)\""));
 }
+
+#[test]
+fn systemd_service_does_not_sandbox_remote_shells() {
+    let service = include_str!("../packaging/systemd/dosh-server.service");
+    let install = include_str!("../install.sh");
+    for raw in [service, install] {
+        assert!(raw.contains("KillMode=process"));
+        for directive in [
+            "NoNewPrivileges=",
+            "PrivateTmp=",
+            "ProtectSystem=",
+            "ProtectHome=",
+            "RestrictAddressFamilies=",
+            "RestrictRealtime=",
+            "LockPersonality=",
+            "MemoryDenyWriteExecute=",
+        ] {
+            assert!(
+                !raw.contains(directive),
+                "dosh sessions are user shells; systemd sandbox directive {directive} changes terminal/app behavior"
+            );
+        }
+        assert!(
+            !raw.contains("ReadWritePaths="),
+            "remote shells must not be restricted to dosh config/data paths"
+        );
+    }
+}