diff --git a/Cargo.lock b/Cargo.lock index 3b0374f0a4..3a3f778427 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4976,6 +4976,7 @@ dependencies = [ "futures", "git", "globwalk", + "libc", "portable-pty", "preview-proxy", "relay-control", diff --git a/crates/local-deployment/Cargo.toml b/crates/local-deployment/Cargo.toml index ffeb017eb9..a20375e541 100644 --- a/crates/local-deployment/Cargo.toml +++ b/crates/local-deployment/Cargo.toml @@ -38,6 +38,9 @@ futures = "0.3" tokio = { workspace = true } globwalk = "0.9" portable-pty = "0.8" +# Used directly on Unix to disarm the pty master's VEOF before portable-pty's +# writer Drop fires (see pty.rs `disarm_master_eof`). +libc = "0.2" dirs = "5.0" toml = "0.8" chrono = { version = "0.4", features = ["serde"] } diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index 4647f84850..6eda563d69 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -891,9 +891,65 @@ pub enum PtyError { SessionClosed, } +/// Disarm the pty master's `VEOF` (Ctrl-D / end-of-transmission) control char so +/// that dropping `portable-pty`'s writer does NOT inject a keystroke into the +/// terminal. +/// +/// `portable-pty` 0.8.1's `UnixMasterWriter::drop` reads the master's termios +/// and, if `c_cc[VEOF]` is non-zero, writes `[b'\n', VEOF]` — LF + Ctrl-D — into +/// the pty. That is the crate's documented "dropping the writer sends EOF to the +/// slave" behavior (registry source `unix.rs:351-363`), and nothing in this +/// crate's own source reveals it — hence this comment. It bites us because our +/// pty master drives a *tmux client's* tty: on teardown the tmux server reads +/// those 2 bytes as client keyboard input and forwards them to the active pane. +/// In the Claude TUI composer the LF inserts a stray newline (never submits) and +/// the Ctrl-D is a no-op; in a bare shell the Enter+Ctrl-D EXITS the shell and +/// kills the (persistent, `vk_`-named) session. Reproduced live 4-6/6 teardowns. +/// +/// This is a TEARDOWN-ONLY fix: a live shell-mode terminal legitimately needs +/// Ctrl-D/VEOF, so we clear it only right before the writer drops. The writer +/// holds a `dup(2)` of the master fd (`take_writer` → `self.fd.try_clone()`) and +/// termios is a property of the pty device shared across dup'd fds, so clearing +/// `VEOF` here — on the master fd — is seen by the writer's own `tcgetattr`, +/// making its `if eot != 0` guard skip the write. +#[cfg(unix)] +fn disarm_master_eof(master: &dyn portable_pty::MasterPty) { + let Some(fd) = master.as_raw_fd() else { + // Runs inside Drop, so never panic — but with no fd there is no + // disarm, and the writer's Drop may inject; leave a trace so the + // attach-window tripwire (server side) can be correlated. + tracing::debug!("pty master has no raw fd; VEOF disarm skipped"); + return; + }; + unsafe { + let mut termios: libc::termios = std::mem::zeroed(); + if libc::tcgetattr(fd, &mut termios) == 0 { + termios.c_cc[libc::VEOF] = 0; + if libc::tcsetattr(fd, libc::TCSANOW, &termios) != 0 { + tracing::debug!( + "tcsetattr failed disarming pty VEOF; writer teardown may \ + inject \\n+EOT" + ); + } + } else { + tracing::debug!( + "tcgetattr failed disarming pty VEOF; writer teardown may \ + inject \\n+EOT" + ); + } + } +} + +#[cfg(not(unix))] +fn disarm_master_eof(_master: &dyn portable_pty::MasterPty) {} + struct PtySession { /// Per-session writer behind its own lock so a blocking PTY write never /// holds up the global session registry (see `write`). + /// + /// Drop safety lives on `Drop for PtySession` below; the + /// `writer`-before-`master` declaration order here is only + /// defense-in-depth, not the load-bearing guarantee. writer: Arc>>, master: Box, /// Kills the PTY child (tmux client / shell) on teardown. Required because @@ -904,11 +960,43 @@ struct PtySession { /// ends the ephemeral shell. child_killer: Box, /// Set by the reader thread once it has reaped the child via `wait()`. - /// `close_session` checks this before signalling so it never targets a PID - /// that was already reaped (and possibly recycled) on the natural-exit path. + /// `Drop` checks this before signalling so it never targets a PID that + /// was already reaped (and possibly recycled) on the natural-exit path. child_reaped: Arc, _output_handle: thread::JoinHandle<()>, - closed: bool, +} + +impl Drop for PtySession { + fn drop(&mut self) { + // Disarm the master's VEOF BEFORE any field is dropped: a custom + // `Drop::drop` runs ahead of field drops, and the disarm writes + // persistent pty-device state, so every later writer Drop — the field + // drop here or an outliving `Arc` clone — reads `VEOF == 0` and skips + // its `\n` + Ctrl-D injection. See `disarm_master_eof` for why the + // injection happens at all. + disarm_master_eof(self.master.as_ref()); + // Kill the PTY child so a read-parked reader thread sees EOF, exits, + // and reaps it. Without this the reader blocks on its cloned reader + // forever (dropping `master` doesn't close that clone), leaking a + // thread + an unreaped child per disconnect. For CLI mode this + // detaches the tmux CLIENT — the persistent `vk_` server session + // survives. The `child_reaped` gate skips the signal once the reader + // has reaped on the natural-exit path; the residual load-then-kill + // window (reader completes `wait()` between our load and the kill) + // is a few instructions wide and was accepted in the original + // close_session design — signaling requires the OS to recycle the + // PID inside that window. + // + // The session is constructed inside `create_session`'s blocking task, + // so this Drop is the single teardown point EVERY path funnels + // through: `close_session` (map remove), service shutdown (the + // sessions `HashMap` dropping), creation failures after spawn, and + // the caller's future being cancelled at the `.await` (the runtime + // drops the returned session). + if !self.child_reaped.load(Ordering::Acquire) { + let _ = self.child_killer.kill(); + } + } } #[derive(Clone)] @@ -973,7 +1061,8 @@ impl PtyService { if matches!(&command, PtyCommand::TmuxCli { .. }) { match &tmux_session { Some(session_name) => tracing::info!( - "CLI terminal attaching tmux session {session_name} in {}", + "CLI terminal attaching tmux session {session_name} at \ + {cols}x{rows} in {}", working_dir.display() ), None => { @@ -1071,27 +1160,26 @@ impl PtyService { .spawn_command(cmd) .map_err(|e| PtyError::CreateFailed(e.to_string()))?; - // Independent kill handle so close_session can unblock the reader. - let child_killer = child.clone_killer(); + // Independent kill handle so teardown can unblock the reader. + let mut child_killer = child.clone_killer(); let child_reaped = Arc::new(AtomicBool::new(false)); let child_reaped_reader = child_reaped.clone(); - let mut writer = pty_pair - .master - .take_writer() - .map_err(|e| PtyError::CreateFailed(e.to_string()))?; - - if shell_name == "zsh" { - let _ = writer.write_all(b" PROMPT='$ '; RPROMPT=''\n"); - let _ = writer.flush(); - let _ = writer.write_all(b"\x0c"); - let _ = writer.flush(); - } - - let mut reader = pty_pair - .master - .try_clone_reader() - .map_err(|e| PtyError::CreateFailed(e.to_string()))?; + // Reader + reaper thread BEFORE the writer exists: from here on, + // every failure path can kill the child and rely on this thread to + // reap it, and no failure can strand a VEOF-armed writer. + let mut reader = match pty_pair.master.try_clone_reader() { + Ok(reader) => reader, + Err(e) => { + // No reaper thread yet, but the child is still owned here + // and this is a blocking task: kill and reap inline so + // repeated create failures can't accumulate zombies. + let mut child = child; + let _ = child.kill(); + let _ = child.wait(); + return Err(PtyError::CreateFailed(e.to_string())); + } + }; let output_handle = thread::spawn(move || { let mut child = child; @@ -1114,36 +1202,49 @@ impl PtyService { // unreaped PTY child leaves one zombie per disconnect until // the server exits (observed live as a defunct tmux client). let _ = child.wait(); - // Mark reaped so close_session won't signal a freed/recycled PID. + // Mark reaped so teardown won't signal a freed/recycled PID. child_reaped_reader.store(true, Ordering::Release); }); - Ok::<_, PtyError>(( - pty_pair.master, - writer, + let mut writer = match pty_pair.master.take_writer() { + Ok(writer) => writer, + Err(e) => { + // The reaper thread owns the child; kill so it unblocks + // and reaps. No writer exists, so nothing can inject. + let _ = child_killer.kill(); + return Err(PtyError::CreateFailed(e.to_string())); + } + }; + + if shell_name == "zsh" { + let _ = writer.write_all(b" PROMPT='$ '; RPROMPT=''\n"); + let _ = writer.flush(); + let _ = writer.write_all(b"\x0c"); + let _ = writer.flush(); + } + + // Construct the session INSIDE the blocking task, with no fallible + // step between `take_writer` above and this point: the writer is + // never alive outside a `PtySession`, so every teardown — early + // return, panic-unwind, cancellation of the caller at the `.await` + // (the runtime then drops this returned session), or normal + // close — funnels through `Drop for PtySession`, which disarms + // VEOF and kills the child. + Ok::<_, PtyError>(PtySession { + writer: Arc::new(Mutex::new(writer)), + master: pty_pair.master, child_killer, child_reaped, - output_handle, - )) + _output_handle: output_handle, + }) }) .await .map_err(|e| PtyError::CreateFailed(e.to_string()))??; - let (master, writer, child_killer, child_reaped, output_handle) = result; - - let session = PtySession { - writer: Arc::new(Mutex::new(writer)), - master, - child_killer, - child_reaped, - _output_handle: output_handle, - closed: false, - }; - self.sessions .lock() .map_err(|e| PtyError::CreateFailed(e.to_string()))? - .insert(session_id, session); + .insert(session_id, result); Ok((session_id, output_rx)) } @@ -1164,10 +1265,6 @@ impl PtyService { .get(&session_id) .ok_or(PtyError::SessionNotFound(session_id))?; - if session.closed { - return Err(PtyError::SessionClosed); - } - session.writer.clone() }; @@ -1195,10 +1292,6 @@ impl PtyService { .get(&session_id) .ok_or(PtyError::SessionNotFound(session_id))?; - if session.closed { - return Err(PtyError::SessionClosed); - } - session .master .resize(PtySize { @@ -1213,24 +1306,17 @@ impl PtyService { } pub async fn close_session(&self, session_id: Uuid) -> Result<(), PtyError> { - if let Some(mut session) = self + // Dropping the removed session runs the full teardown — VEOF disarm + // then child kill — in `Drop for PtySession`. Bound OUTSIDE the lock + // scope so the teardown syscalls never run under the global registry + // lock. (A send-parked reader is additionally released when the + // caller drops the output receiver after this returns.) + let session = self .sessions .lock() .map_err(|_| PtyError::SessionClosed)? - .remove(&session_id) - { - // Kill the PTY child so a read-parked reader sees EOF, exits, and - // reaps it. Without this the reader blocks on its cloned reader - // forever (dropping the master here doesn't close that clone), - // leaking a thread + an unreaped child per disconnect. (A - // send-parked reader is instead released when the caller drops the - // output receiver after this returns.) Skip the signal if the - // reader already reaped on the natural-exit path, so we never - // SIGHUP a freed/recycled PID. - if !session.child_reaped.load(Ordering::Acquire) { - let _ = session.child_killer.kill(); - } - } + .remove(&session_id); + drop(session); Ok(()) } } @@ -1466,4 +1552,132 @@ mod tests { // Non-vk session names are ignored entirely. assert!(parse_cli_session_line("misc\t0\t900", 1000).is_none()); } + + /// Read the master fd's `VEOF` control char, or `None` if it has no fd / + /// `tcgetattr` fails. Test-only mirror of the read side of + /// `disarm_master_eof`. + #[cfg(unix)] + fn master_veof(master: &dyn portable_pty::MasterPty) -> Option { + let fd = master.as_raw_fd()?; + unsafe { + let mut termios: libc::termios = std::mem::zeroed(); + if libc::tcgetattr(fd, &mut termios) == 0 { + Some(termios.c_cc[libc::VEOF]) + } else { + None + } + } + } + + /// Same `openpty` path as `create_session`. + #[cfg(unix)] + fn open_test_pty() -> portable_pty::PtyPair { + NativePtySystem::default() + .openpty(PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + .expect("openpty") + } + + /// The core of the stray-newline/EOT fix: after `disarm_master_eof`, + /// `portable-pty`'s writer-Drop reads `VEOF == 0` and skips its `\n` + + /// Ctrl-D injection. + #[cfg(unix)] + #[test] + fn disarm_master_eof_zeroes_veof() { + let pair = open_test_pty(); + + // Guard against the helper silently no-oping: a fresh pty must have + // VEOF armed (Ctrl-D == 4 by default) for the disarm to be meaningful. + let before = master_veof(pair.master.as_ref()).expect("tcgetattr before"); + assert_ne!(before, 0, "a fresh pty master should have VEOF armed"); + + disarm_master_eof(pair.master.as_ref()); + + let after = master_veof(pair.master.as_ref()).expect("tcgetattr after"); + assert_eq!(after, 0, "VEOF must be disarmed after disarm_master_eof"); + } + + /// `Drop for PtySession` is the single teardown point every path funnels + /// through (close_session, shutdown, creation failure, cancelled create): + /// dropping a session must disarm VEOF on the pty device (so the writer's + /// own Drop injects nothing) AND kill the child so the reader thread + /// reaps it. + #[cfg(unix)] + #[test] + fn pty_session_drop_disarms_veof_and_reaps_child() { + use std::os::fd::{AsRawFd as _, BorrowedFd}; + + let portable_pty::PtyPair { master, slave } = open_test_pty(); + + // Independent dup of the master so the pty device (and its termios) + // stays observable after the session — and its master fd — drops. + let probe = { + let raw = master.as_raw_fd().expect("master raw fd"); + let borrowed = unsafe { BorrowedFd::borrow_raw(raw) }; + borrowed.try_clone_to_owned().expect("dup master fd") + }; + + let mut cmd = CommandBuilder::new("sleep"); + cmd.arg("30"); + let child = slave.spawn_command(cmd).expect("spawn child"); + // Mirror create_session: the slave fd is not held beyond spawn, so + // the child's exit closes the last slave and EOFs the reader. + drop(slave); + + let child_killer = child.clone_killer(); + let child_reaped = Arc::new(AtomicBool::new(false)); + let child_reaped_reader = child_reaped.clone(); + let mut reader = master.try_clone_reader().expect("clone reader"); + let output_handle = thread::spawn(move || { + let mut child = child; + let mut buf = [0u8; 256]; + loop { + match reader.read(&mut buf) { + Ok(0) | Err(_) => break, + Ok(_) => {} + } + } + let _ = child.wait(); + child_reaped_reader.store(true, Ordering::Release); + }); + let writer = master.take_writer().expect("take writer"); + + let session = PtySession { + writer: Arc::new(Mutex::new(writer)), + master, + child_killer, + child_reaped: child_reaped.clone(), + _output_handle: output_handle, + }; + drop(session); + + // VEOF disarmed on the device (visible through the probe dup). + let veof = unsafe { + let mut termios: libc::termios = std::mem::zeroed(); + assert_eq!( + libc::tcgetattr(probe.as_raw_fd(), &mut termios), + 0, + "probe tcgetattr" + ); + termios.c_cc[libc::VEOF] + }; + assert_eq!(veof, 0, "session drop must disarm VEOF"); + + // Child killed -> reader EOFs -> thread reaps. Bounded wait; the + // `sleep 30` bounds a kill failure to a test failure, not a hang. + for _ in 0..200 { + if child_reaped.load(Ordering::Acquire) { + break; + } + thread::sleep(std::time::Duration::from_millis(25)); + } + assert!( + child_reaped.load(Ordering::Acquire), + "session drop must kill the child so the reader thread reaps it" + ); + } } diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index 750eea83ef..8ef7d83528 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -46,10 +46,14 @@ enum TerminalMode { #[derive(Debug, Deserialize)] struct TerminalQuery { pub workspace_id: Uuid, - #[serde(default = "default_cols")] - pub cols: u16, - #[serde(default = "default_rows")] - pub rows: u16, + /// `None` (fell back to 80x24) is distinguishable from an explicit value + /// so the stray-newline regression tripwire below can warn on true + /// absence without false-positiving on a pane that genuinely measures + /// 80x24. + #[serde(default)] + pub cols: Option, + #[serde(default)] + pub rows: Option, #[serde(default)] mode: TerminalMode, /// VibeKanban session whose claude conversation CLI mode should resume, @@ -58,13 +62,8 @@ struct TerminalQuery { session_id: Option, } -fn default_cols() -> u16 { - 80 -} - -fn default_rows() -> u16 { - 24 -} +const DEFAULT_COLS: u16 = 80; +const DEFAULT_ROWS: u16 = 24; #[derive(Debug, Deserialize)] #[serde(tag = "type", rename_all = "snake_case")] @@ -190,6 +189,23 @@ async fn terminal_ws( let (working_dir, command) = match query.mode { TerminalMode::Cli => { + // Regression tripwire for the stray-newline bug: a CLI attach must + // always carry the pane's real fitted grid. Absent cols/rows mean + // the frontend connected without measuring, which makes claude + // reflow and stack blank lines on the follow-up resize. After the + // frontend fix (never connect at an unmeasured size, URL always + // carries the fitted grid) this must never fire. An EXPLICIT + // 80x24 is legitimate (a pane can really measure that); a + // literal-default regression stays observable via the per-attach + // cols x rows log in pty.rs. + if query.cols.is_none() || query.rows.is_none() { + tracing::warn!( + "CLI terminal attaching without cols/rows for workspace {} — \ + frontend connected unmeasured (stray-newline regression)", + query.workspace_id + ); + } + let pool = &deployment.db().pool; // Resolve the uix session driving the handover. A mid-switch @@ -349,8 +365,8 @@ async fn terminal_ws( socket, deployment, working_dir, - query.cols, - query.rows, + query.cols.unwrap_or(DEFAULT_COLS), + query.rows.unwrap_or(DEFAULT_ROWS), command, prompt_session_to_clear, ) @@ -366,6 +382,14 @@ async fn handle_terminal_ws( command: PtyCommand, prompt_session_to_clear: Option, ) { + // FIX 4 tripwire label: the pty session name, captured before `command` is + // moved into `create_session`. For CLI mode this is the tmux `vk_` + // name, so the logged bytes line up with tmux server logs. + let tripwire_session = match &command { + PtyCommand::TmuxCli { session_name, .. } => session_name.clone(), + PtyCommand::Shell => "shell".to_string(), + }; + let (session_id, mut output_rx) = match deployment .pty() .create_session(working_dir, cols, rows, command) @@ -379,6 +403,9 @@ async fn handle_terminal_ws( } }; + // FIX 4 input tripwire — bounds and rationale live on `AttachInputTripwire`. + let mut tripwire = AttachInputTripwire::new(tripwire_session, session_id); + // The tmux session is now created and its bootstrap (carrying the parked // CLI prompt) is running; only now is it safe to clear the prompt, so a // failure before this point leaves it parked for the next attach. If the @@ -423,6 +450,7 @@ async fn handle_terminal_ws( match cmd { TerminalCommand::Input { data } => { if let Ok(bytes) = BASE64.decode(&data) { + tripwire.observe(&bytes); let _ = pty_service.write(session_id_for_input, &bytes).await; } } @@ -447,6 +475,86 @@ async fn handle_terminal_ws( let _ = deployment.pty().close_session(session_id).await; } +/// FIX 4 input tripwire: for a strictly bounded window right after each +/// attach, hex-dump every client→pty input chunk. This is the production +/// canary for the stray-newline / EOT injection bug (portable-pty's +/// writer-Drop wrote `\n` + Ctrl-D into the tmux client's tty on teardown, +/// killing the pane) and for any FUTURE client-side injector: it makes the +/// exact bytes the browser sent — with an ms-since-attach stamp and the +/// per-attach id — visible in server logs, so a client-origin injection can +/// be told from a server-side one. Bounded to the first `Self::WINDOW` OR +/// `Self::MAX_BYTES` (whichever first); once spent, `observe` is a single +/// integer compare with no allocation, so it is permanently safe to leave +/// enabled. +struct AttachInputTripwire { + /// The pty session name (tmux `vk_` for CLI mode) so the logged + /// bytes line up with tmux server logs. + session: String, + /// The per-attach PTY session id. + attach_id: Uuid, + started: std::time::Instant, + bytes_logged: usize, +} + +impl AttachInputTripwire { + const WINDOW: std::time::Duration = std::time::Duration::from_secs(2); + const MAX_BYTES: usize = 128; + + fn new(session: String, attach_id: Uuid) -> Self { + Self { + session, + attach_id, + started: std::time::Instant::now(), + bytes_logged: 0, + } + } + + /// Log (redacted — see `hex_dump`) input bytes while the attach window is + /// open. Expiry latches: the monotonic clock never goes backwards, so + /// marking the byte budget spent on the first out-of-window observation + /// is behavior-identical and skips even the clock read afterwards. + fn observe(&mut self, bytes: &[u8]) { + if bytes.is_empty() || self.bytes_logged >= Self::MAX_BYTES { + return; + } + let elapsed = self.started.elapsed(); + if elapsed >= Self::WINDOW { + self.bytes_logged = Self::MAX_BYTES; + return; + } + let take = (Self::MAX_BYTES - self.bytes_logged).min(bytes.len()); + tracing::info!( + session = %self.session, + attach = %self.attach_id, + ms_since_attach = elapsed.as_millis() as u64, + bytes = %hex_dump(&bytes[..take]), + "terminal input (attach-window tripwire)" + ); + self.bytes_logged += take; + } +} + +/// Space-separated redacted hex of a byte slice for the attach-window input +/// tripwire: control bytes (< 0x20, or 0x7f) are shown verbatim (e.g. `0a 04`, +/// `1b`) because they are the injection/escape-sequence signature; every other +/// byte is masked as `..` so real keystrokes (passwords, pasted secrets) never +/// reach the logs — only their count and timing do. +/// The caller bounds the slice length, so this +/// never allocates more than a fixed maximum. +fn hex_dump(bytes: &[u8]) -> String { + use std::fmt::Write as _; + let mut out = String::with_capacity(bytes.len() * 3); + for byte in bytes { + if *byte < 0x20 || *byte == 0x7f { + let _ = write!(out, "{byte:02x} "); + } else { + out.push_str(".. "); + } + } + out.pop(); // trailing space + out +} + async fn send_error(socket: &mut MaybeSignedWebSocket, message: &str) -> anyhow::Result<()> { let msg = TerminalMessage::Error { message: message.to_string(), @@ -460,3 +568,77 @@ async fn send_error(socket: &mut MaybeSignedWebSocket, message: &str) -> anyhow: pub(super) fn router() -> Router { Router::new().route("/terminal/ws", get(terminal_ws)) } + +#[cfg(test)] +mod tripwire_tests { + use uuid::Uuid; + + use super::{AttachInputTripwire, hex_dump}; + + fn tripwire() -> AttachInputTripwire { + AttachInputTripwire::new("vk_test".to_string(), Uuid::new_v4()) + } + + /// The byte budget: chunks are truncated to the remaining budget, and a + /// spent budget stops all logging state changes. + #[test] + fn observe_caps_logged_bytes_at_budget() { + let mut tw = tripwire(); + tw.observe(&[0x0a; 100]); + assert_eq!(tw.bytes_logged, 100); + // 100 + 100 crosses the 128 cap: only the remainder is taken. + tw.observe(&[0x0a; 100]); + assert_eq!(tw.bytes_logged, AttachInputTripwire::MAX_BYTES); + // Spent: further input changes nothing. + tw.observe(&[0x0a; 4]); + assert_eq!(tw.bytes_logged, AttachInputTripwire::MAX_BYTES); + // Empty chunks never count. + let mut fresh = tripwire(); + fresh.observe(&[]); + assert_eq!(fresh.bytes_logged, 0); + } + + /// The time window: the first out-of-window observation latches the + /// budget as spent (behavior-identical to checking the clock forever, + /// but cheaper), and nothing is logged after expiry. + #[test] + fn observe_latches_after_window_expires() { + let mut tw = tripwire(); + // Simulate an expired window without sleeping. + tw.started = std::time::Instant::now() - (AttachInputTripwire::WINDOW * 2); + tw.observe(&[0x0a]); + assert_eq!( + tw.bytes_logged, + AttachInputTripwire::MAX_BYTES, + "expiry must latch the budget as spent without logging" + ); + } + + #[test] + fn hex_dump_shows_control_bytes_and_masks_printables() { + // The \n+EOT injection signature must stay fully visible… + assert_eq!(hex_dump(&[0x0a, 0x04]), "0a 04"); + // …while printable payload (keystrokes, secrets) is masked to `..`, + // keeping only count and position. + assert_eq!(hex_dump(b"hi"), ".. .."); + assert_eq!(hex_dump(&[0x1b, b'[', b'A']), "1b .. .."); + assert_eq!(hex_dump(&[0x7f]), "7f"); + assert_eq!(hex_dump(&[]), ""); + } + + /// Locks the redaction invariant over the whole byte range: ONLY the C0 + /// controls (0x00-0x1f) and DEL (0x7f) may appear verbatim; every + /// printable AND high/UTF-8 byte (0x20-0x7e, 0x80-0xff) must be masked so + /// no keystroke content can ever reach the logs. + #[test] + fn hex_dump_masks_every_non_control_byte() { + for byte in 0x00u8..=0xff { + let dumped = hex_dump(&[byte]); + if byte < 0x20 || byte == 0x7f { + assert_eq!(dumped, format!("{byte:02x}"), "control byte {byte:#04x}"); + } else { + assert_eq!(dumped, "..", "byte {byte:#04x} must be masked"); + } + } + } +} diff --git a/packages/web-core/src/shared/components/XTermInstance.tsx b/packages/web-core/src/shared/components/XTermInstance.tsx index 67fa9d80aa..beabe8bb1b 100644 --- a/packages/web-core/src/shared/components/XTermInstance.tsx +++ b/packages/web-core/src/shared/components/XTermInstance.tsx @@ -1,4 +1,10 @@ -import { useCallback, useEffect, useRef, useState } from 'react'; +import { + useCallback, + useEffect, + useLayoutEffect, + useRef, + useState, +} from 'react'; import { Terminal } from '@xterm/xterm'; import { FitAddon } from '@xterm/addon-fit'; import { WebLinksAddon } from '@xterm/addon-web-links'; @@ -8,7 +14,7 @@ import { TERMINAL_BACKGROUND, getTerminalTheme, } from '@/shared/lib/terminalTheme'; -import { buildTerminalWsUrl } from '@/shared/lib/terminalWsUrl'; +import { resolveTerminalEndpoint } from '@/shared/lib/terminalWsUrl'; import { cancelActiveTerminalGesture } from '@/shared/lib/terminalTouchGestures'; import { installTerminalTouchLayers } from '@/shared/lib/terminalTouchLayers'; import { applyStickyCtrl } from '@/shared/lib/terminalKeySequences'; @@ -68,31 +74,132 @@ export function XTermInstance({ getTerminalConnection, } = useTerminal(); - // Built with the terminal's CURRENT grid size (see buildTerminalWsUrl): a - // fresh attach must open the PTY at the real size, not the 80x24 default, or - // claude reflows on the follow-up onopen resize and stacks blank lines every - // time the CLI pane is reopened. - const buildEndpoint = useCallback( - (cols: number, rows: number) => - buildTerminalWsUrl({ + // Latest sessionId / onClose for the connection callbacks. The provider + // stores those callbacks for the connection's whole lifetime, which spans + // session switches and component remounts (a live connection is never torn + // down on either). Reading the props through refs keeps every later + // reconnect on the session the UI currently shows — a sessionId captured at + // connect time could resume the wrong conversation if the tmux session was + // meanwhile reaped and the reconnect had to recreate it — and keeps the + // unstable onClose prop identity out of the callback deps (it would + // otherwise churn the ResizeObserver effect on every parent render). + const sessionIdRef = useRef(sessionId); + const onCloseRef = useRef(onClose); + // Layout effect, not passive: it must run synchronously inside the commit, + // BEFORE any pending websocket-open promise continuation can execute — + // otherwise an open resolving in the commit→passive-effect gap would read + // the previous session's value (see the heal effect below, same reason). + useLayoutEffect(() => { + sessionIdRef.current = sessionId; + onCloseRef.current = onClose; + }); + + // Registry lookup + re-fit shared by every stored connection callback. The + // callbacks live as long as the connection, which survives XTermInstance + // remounts (CliMainPane gate flips, hidden side tabs) — so they resolve the + // terminal through the provider REGISTRY (same lifetime), never through this + // component's refs, which are nulled on unmount. + const fitInstance = useCallback(() => { + const instance = getTerminalInstance(tabId); + instance?.fitAddon.fit(); + return instance; + }, [tabId, getTerminalInstance]); + + // Re-fit and resolve the WS endpoint at the terminal's CURRENT grid. Returns + // null when the pane is unmeasurable — never connect then: the URL would + // carry a placeholder/garbage size and claude would reflow (stacking blank + // lines that read as a stray Enter) on the follow-up resize once the pane + // is shown. Called fresh on every (re)connect attempt so a reconnect + // attaches at the pane's present size, not the creation-time one. + // + // Measurability is gated on real DOM boxes, not just proposeDimensions(): + // FitAddon clamps its result to >=1 cell even for a hidden-but-rendered + // (0-height) container, which would otherwise pass the dims check with a + // garbage 2x1-ish grid and resurrect the tiny-then-resize bounce this fix + // exists to kill. Both boxes matter: display:none / detached read as 0x0 + // on the element, while a collapsed-but-visible container (splitter + // dragged shut) zeroes the PARENT — which is the box FitAddon actually + // measures — while the element keeps its intrinsic screen height. + const getEndpoint = useCallback((): string | null => { + const instance = fitInstance(); + if (!instance) return null; + const element = instance.terminal.element; + const parent = element?.parentElement; + if ( + !element || + !parent || + !element.isConnected || + element.offsetWidth === 0 || + element.offsetHeight === 0 || + parent.offsetWidth === 0 || + parent.offsetHeight === 0 + ) { + return null; + } + return resolveTerminalEndpoint( + { workspaceId, - cols, - rows, protocol: window.location.protocol, host: window.location.host, mode, - sessionId, - }), - [workspaceId, mode, sessionId] - ); + sessionId: sessionIdRef.current, + }, + instance.fitAddon.proposeDimensions() + ); + }, [fitInstance, workspaceId, mode]); + + // Open the backend connection for this tab — but only once the pane is + // actually measurable. A CLI pane can mount inside a hidden (display:none) + // mobile tab where fit() no-ops; connecting then bakes cols=80&rows=24 into + // the URL. When unmeasured we DEFER: fitTerminal() re-runs this on the first + // non-zero ResizeObserver tick (i.e. when the tab is shown). NEVER tears down + // a live connection — hidden side panes keep theirs by design; this only ever + // creates the INITIAL connection for a tab that has none. + const ensureConnection = useCallback(() => { + // The whole connect policy lives in the provider: + // createTerminalConnection is idempotent while a live/in-flight + // connection owns the tab (it then only refreshes the stored callbacks + // to this mount's closures — load-bearing after a remount, whose + // predecessor's closures read refs that its cleanup nulled) and it + // defers, creating nothing, while the pane is unmeasurable. So calling + // this repeatedly — mount, ResizeObserver ticks, session switches — is + // always safe. + createTerminalConnection( + tabId, + getEndpoint, + // Registry-resolved for the same remount-survival reason as fitInstance: + // a closure over this component's refs would silently drop all output + // after a reattach. + (data) => getTerminalInstance(tabId)?.terminal.write(data), + () => onCloseRef.current?.(), + () => { + // Re-fit and report the current grid so the PTY/tmux is sized to the + // pane on every (re)connect (see TerminalProvider ws.onopen). + const instance = fitInstance(); + if (!instance) return null; + return { cols: instance.terminal.cols, rows: instance.terminal.rows }; + } + ); + }, [ + tabId, + getEndpoint, + fitInstance, + getTerminalInstance, + createTerminalConnection, + ]); const fitTerminal = useCallback(() => { - fitAddonRef.current?.fit(); - if (terminalRef.current) { - const conn = getTerminalConnection(tabId); - conn?.resize(terminalRef.current.cols, terminalRef.current.rows); + const instance = fitInstance(); + if (!instance) return; + const conn = getTerminalConnection(tabId); + if (conn) { + conn.resize(instance.terminal.cols, instance.terminal.rows); + } else { + // The initial connect was deferred because the pane was unmeasured at + // mount; now that the ResizeObserver reports a real size, open it. + ensureConnection(); } - }, [tabId, getTerminalConnection]); + }, [tabId, fitInstance, getTerminalConnection, ensureConnection]); // Terminal + connection lifecycle. Every run of this effect MUST register // the same cleanup: an early return without one leaves `terminalRef` @@ -256,24 +363,10 @@ export function XTermInstance({ // Ensure a backend connection exists for this tab — also on the reattach // path, so a tab whose connection died (e.g. reconnect gave up, server - // restarted) heals on the next mount instead of staying dead. - if (!getTerminalConnection(tabId)) { - // Connect at the fitted size (set by fit() above) so the backend opens - // the PTY at the real dimensions — no 80x24-then-resize reflow. - createTerminalConnection( - tabId, - buildEndpoint(terminal.cols, terminal.rows), - (data) => terminal.write(data), - onClose, - () => { - // Re-fit and report the current grid so the PTY/tmux is sized to the - // pane on every (re)connect (see TerminalProvider ws.onopen). - fitAddonRef.current?.fit(); - const t = terminalRef.current; - return t ? { cols: t.cols, rows: t.rows } : null; - } - ); - } + // restarted) heals on the next mount instead of staying dead. Connects at + // the fitted size, or defers if the pane isn't measurable yet (hidden tab); + // fitTerminal() opens it on the first non-zero resize once shown. + ensureConnection(); return () => { // A D-pad gesture running right now would never see its touchend once @@ -297,14 +390,27 @@ export function XTermInstance({ }; }, [ tabId, - buildEndpoint, - onClose, + ensureConnection, getTerminalInstance, registerTerminalInstance, - createTerminalConnection, getTerminalConnection, ]); + // A session switch must also heal a dead/given-up connection: without this, + // a pane whose connection died would only recover on a remount or a resize + // tick. ensureConnection is idempotent — with a live connection it merely + // refreshes the stored callbacks (which also re-syncs them after the + // switch). Layout effect on purpose: the refresh bumps the generation's + // endpointEpoch, and that must happen synchronously in the SAME commit as + // the sessionId change — a websocket open resolving in the gap before a + // passive effect would see an unbumped epoch and register the previous + // session's socket. (At first mount this runs before the terminal exists + // in the registry and defers harmlessly; the lifecycle effect below opens + // the connection.) + useLayoutEffect(() => { + ensureConnection(); + }, [ensureConnection, sessionId]); + useEffect(() => { if (!resizeRef.current) return; // Debounce: a pane-divider drag fires dozens of observations per second, diff --git a/packages/web-core/src/shared/hooks/useTerminal.ts b/packages/web-core/src/shared/hooks/useTerminal.ts index f9c0c88758..f9ffa3a52a 100644 --- a/packages/web-core/src/shared/hooks/useTerminal.ts +++ b/packages/web-core/src/shared/hooks/useTerminal.ts @@ -48,9 +48,21 @@ export interface TerminalContextType { ) => void; getTerminalInstance: (tabId: string) => TerminalInstance | null; unregisterTerminalInstance: (tabId: string) => void; + /** + * Ensure-style and idempotent: while the tab has a live connection OR an + * in-flight open (generation created, socket not registered yet), calling + * again never opens a second socket — it only refreshes the stored + * callbacks and endpoint source with the caller's fresh closures, so a + * remounted component re-binds without touching the socket. Safe to call + * from mount effects, resize ticks, and session switches. + */ createTerminalConnection: ( tabId: string, - endpoint: string, + // Called fresh on every (re)connect attempt so the socket always attaches + // at the pane's CURRENT fitted grid. Returns null when the pane is not + // measurable yet (hidden/0-height); the provider polls without spending + // the retry budget instead of attaching at a wrong/placeholder size. + getEndpoint: () => string | null, onData: (data: string) => void, onExit?: () => void, getSize?: () => { cols: number; rows: number } | null diff --git a/packages/web-core/src/shared/lib/terminalWsUrl.test.ts b/packages/web-core/src/shared/lib/terminalWsUrl.test.ts index 2c418e05a3..f4cbeb4cec 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.test.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.test.ts @@ -1,6 +1,11 @@ import { describe, it, expect } from 'vitest'; -import { buildTerminalWsUrl } from './terminalWsUrl'; +import { + buildTerminalWsUrl, + resolveTerminalEndpoint, + terminalEndpointsEquivalent, + terminalAttemptIsStale, +} from './terminalWsUrl'; describe('buildTerminalWsUrl', () => { it('carries the provided grid size instead of the 80x24 default', () => { @@ -57,3 +62,167 @@ describe('buildTerminalWsUrl', () => { expect(url).toContain('workspace_id=abc'); }); }); + +describe('resolveTerminalEndpoint', () => { + const base = { + workspaceId: 'ws-1', + protocol: 'https:', + host: 'example.test', + mode: 'cli' as const, + }; + + it('returns null when the pane is unmeasured (hidden / 0-height)', () => { + // FitAddon.proposeDimensions() is undefined for a display:none container. + expect(resolveTerminalEndpoint(base, undefined)).toBeNull(); + expect(resolveTerminalEndpoint(base, null)).toBeNull(); + // A 0-sized container must never bake xterm's 80x24 default into the URL. + expect(resolveTerminalEndpoint(base, { cols: 0, rows: 0 })).toBeNull(); + expect(resolveTerminalEndpoint(base, { cols: 120, rows: 0 })).toBeNull(); + }); + + it('returns null for NaN/non-finite dims (detached or unstyled container)', () => { + // proposeDimensions() computes from parseInt(getComputedStyle(...)) and + // can yield NaN when the container has no usable computed style — + // "cols=NaN" must never reach the URL. + expect(resolveTerminalEndpoint(base, { cols: NaN, rows: 24 })).toBeNull(); + expect(resolveTerminalEndpoint(base, { cols: 80, rows: NaN })).toBeNull(); + expect(resolveTerminalEndpoint(base, { cols: NaN, rows: NaN })).toBeNull(); + expect( + resolveTerminalEndpoint(base, { cols: Infinity, rows: 24 }) + ).toBeNull(); + }); + + it('builds a URL carrying the real measured grid when measurable', () => { + const url = resolveTerminalEndpoint(base, { cols: 203, rows: 51 }); + expect(url).not.toBeNull(); + expect(url).toContain('cols=203'); + expect(url).toContain('rows=51'); + expect(url).toContain('&mode=cli'); + // Never the default that caused the reflow. + expect(url).not.toContain('cols=80'); + expect(url).not.toContain('rows=24'); + }); + + it('reflects the CURRENT grid on each call, so a reconnect re-fits', () => { + // Models the per-attempt getEndpoint: connectWebSocket calls it fresh on + // every (re)connect, so a size change between attempts yields a new URL + // instead of replaying the size frozen at tab creation. + let dims = { cols: 100, rows: 30 }; + const getEndpoint = () => resolveTerminalEndpoint(base, dims); + + const first = getEndpoint(); + dims = { cols: 120, rows: 40 }; + const second = getEndpoint(); + + expect(first).toContain('cols=100'); + expect(second).toContain('cols=120'); + expect(second).not.toBe(first); + }); +}); + +describe('terminalEndpointsEquivalent', () => { + const make = (over: Partial[0]>) => + buildTerminalWsUrl({ + workspaceId: 'ws-1', + cols: 100, + rows: 30, + protocol: 'https:', + host: 'example.test', + mode: 'cli', + sessionId: 'sess-a', + ...over, + }); + + it('treats size-only drift as equivalent (post-open resize corrects it)', () => { + expect( + terminalEndpointsEquivalent(make({}), make({ cols: 203, rows: 51 })) + ).toBe(true); + }); + + it('flags a changed session as stale', () => { + expect( + terminalEndpointsEquivalent(make({}), make({ sessionId: 'sess-b' })) + ).toBe(false); + // Dropping the session entirely is also a material change. + expect( + terminalEndpointsEquivalent(make({}), make({ sessionId: undefined })) + ).toBe(false); + }); + + it('flags changed mode or workspace as stale', () => { + expect( + terminalEndpointsEquivalent( + make({}), + make({ mode: 'shell', sessionId: undefined }) + ) + ).toBe(false); + expect( + terminalEndpointsEquivalent(make({}), make({ workspaceId: 'ws-2' })) + ).toBe(false); + }); +}); + +describe('terminalAttemptIsStale', () => { + const make = (over: Partial[0]>) => + buildTerminalWsUrl({ + workspaceId: 'ws-1', + cols: 100, + rows: 30, + protocol: 'https:', + host: 'example.test', + mode: 'cli', + sessionId: 'sess-a', + ...over, + }); + + it('is never stale when the epoch did not change', () => { + // Unchanged epoch = untouched endpoint source: valid without inspecting + // endpoints, even against a materially different current endpoint (the + // caller never recomputes in this branch). + expect(terminalAttemptIsStale(false, make({}), make({}))).toBe(false); + expect( + terminalAttemptIsStale(false, make({}), make({ sessionId: 'sess-b' })) + ).toBe(false); + expect(terminalAttemptIsStale(false, make({}), null)).toBe(false); + }); + + it('epoch changed but size-only drift is not stale', () => { + // The post-open resize corrects size; a refresh that only re-fit the grid + // must not discard the attempt. + expect( + terminalAttemptIsStale(true, make({}), make({ cols: 203, rows: 51 })) + ).toBe(false); + }); + + it('epoch changed with a material change (session) is stale', () => { + expect( + terminalAttemptIsStale(true, make({}), make({ sessionId: 'sess-b' })) + ).toBe(true); + expect( + terminalAttemptIsStale(true, make({}), make({ sessionId: undefined })) + ).toBe(true); + expect( + terminalAttemptIsStale(true, make({}), make({ workspaceId: 'ws-2' })) + ).toBe(true); + }); + + it('epoch changed and the pane is now unmeasurable (null) is stale', () => { + // Conservative discard: the refresh may have changed the session and there + // is no current URL to prove equivalence. + expect(terminalAttemptIsStale(true, make({}), null)).toBe(true); + }); + + it('forcing epochChanged=true inspects the endpoint (the onopen contract)', () => { + // The onopen site passes epochChanged=true unconditionally so a stale + // socket cannot become live even when no mounted child bumped the epoch + // (the gated-child race). Forced inspection: null / material change = stale, + // equivalent (size-only drift) = not stale. + expect(terminalAttemptIsStale(true, make({}), null)).toBe(true); + expect( + terminalAttemptIsStale(true, make({}), make({ sessionId: 'sess-b' })) + ).toBe(true); + expect( + terminalAttemptIsStale(true, make({}), make({ cols: 5, rows: 5 })) + ).toBe(false); + }); +}); diff --git a/packages/web-core/src/shared/lib/terminalWsUrl.ts b/packages/web-core/src/shared/lib/terminalWsUrl.ts index ff3e084104..c0f5e8a88d 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.ts @@ -40,3 +40,110 @@ export function buildTerminalWsUrl({ mode === 'cli' && sessionId ? `&session_id=${sessionId}` : ''; return `${scheme}//${host}/api/terminal/ws?workspace_id=${workspaceId}&cols=${cols}&rows=${rows}${modeParam}${sessionParam}`; } + +/** + * Resolve the WS endpoint from a freshly measured grid, or `null` when the pane + * could not be measured. + * + * `dims` is `FitAddon.proposeDimensions()`, which returns `undefined` for a + * hidden (`display:none`) or 0-height container. Returning `null` in that case + * is the load-bearing invariant of the stray-newline fix: the caller must NEVER + * open the WS at a placeholder size (xterm's 80x24 constructor default), because + * the backend opens the PTY/tmux at exactly the URL size and claude then reflows + * — stacking blank lines that read as a stray Enter — on the follow-up resize + * once the pane becomes visible. A `null` here means "defer / reschedule the + * connect until the pane is measurable". + * + * Because it re-reads the CURRENT dims on every call, using it as the per-attempt + * endpoint source also makes reconnects attach at the pane's present grid rather + * than the size frozen when the tab was first created. + */ +export function resolveTerminalEndpoint( + params: Omit, + dims: { cols: number; rows: number } | undefined | null +): string | null { + // Number.isFinite also rejects NaN: FitAddon.proposeDimensions() derives + // its numbers from parseInt(getComputedStyle(...)) and can yield NaN for a + // detached or oddly-styled container — that must defer like any other + // unmeasured pane, not bake "cols=NaN" into the URL (the server would + // reject it and the reconnect ladder would burn its budget to giveUp). + if ( + !dims || + !Number.isFinite(dims.cols) || + !Number.isFinite(dims.rows) || + dims.cols <= 0 || + dims.rows <= 0 + ) { + return null; + } + return buildTerminalWsUrl({ ...params, cols: dims.cols, rows: dims.rows }); +} + +/** + * Compare two terminal WS endpoints ignoring the size params (`cols`/`rows`). + * + * Used to validate an in-flight connect attempt after its async open + * resolves: size drift between "attempt started" and "attempt resolved" is + * legitimate (the post-open resize corrects it), but a changed `session_id`, + * `mode`, or `workspace_id` — e.g. a session switch while a slow relay + * transport was opening — makes the attempt STALE: registering it would bind + * the visible pane to the previous conversation's tmux session. + */ +export function terminalEndpointsEquivalent(a: string, b: string): boolean { + try { + const ua = new URL(a); + const ub = new URL(b); + if (ua.origin !== ub.origin || ua.pathname !== ub.pathname) return false; + for (const key of ['cols', 'rows']) { + ua.searchParams.delete(key); + ub.searchParams.delete(key); + } + ua.searchParams.sort(); + ub.searchParams.sort(); + return ua.searchParams.toString() === ub.searchParams.toString(); + } catch { + // Not parseable as URLs (unexpected) — fall back to exact equality. + return a === b; + } +} + +/** + * Decide whether an in-flight terminal WS attempt must be discarded because the + * tab's endpoint source was refreshed (session switch / remount) while the + * socket was opening. + * + * `epochChanged` is the caller's snapshot comparison (`endpointEpoch !== + * attemptEpoch`): when `false` the source is untouched and the attempt is always + * valid — no endpoint inspection needed. When `true`: + * - size-only drift is NOT stale (`terminalEndpointsEquivalent` ignores + * cols/rows; the post-open resize corrects it); + * - a changed `session_id` / `mode` / `workspace_id` IS stale — letting the + * socket become live would bind the pane to the PREVIOUS conversation's tmux + * on a first-attach / post-reap create; + * - a now-unmeasurable pane (`currentEndpoint === null`) is discarded + * CONSERVATIVELY: the refresh may have changed the session and there is no URL + * to prove otherwise, so re-open with the right one once the pane is shown. + * + * The caller must re-run this at EVERY point an attempt could become live — both + * right after the async open resolves AND again at `onopen`, because the default + * transport resolves the open with a still-`CONNECTING` socket, so a switch can + * land in the post-registration / pre-open gap. + * + * At `onopen` (the point of no return) the caller passes `epochChanged = true` + * unconditionally, forcing the endpoint inspection: the epoch is bumped only by + * a MOUNTED terminal child, so a session change while the child is gated off + * (its host stops rendering it) leaves the epoch unchanged; forcing the + * inspection still rejects that stale socket because a gated pane's endpoint + * resolves to `null`. + */ +export function terminalAttemptIsStale( + epochChanged: boolean, + attemptEndpoint: string, + currentEndpoint: string | null +): boolean { + if (!epochChanged) return false; + return ( + currentEndpoint === null || + !terminalEndpointsEquivalent(attemptEndpoint, currentEndpoint) + ); +} diff --git a/packages/web-core/src/shared/providers/TerminalProvider.tsx b/packages/web-core/src/shared/providers/TerminalProvider.tsx index 0e769ca23a..a8acb0882f 100644 --- a/packages/web-core/src/shared/providers/TerminalProvider.tsx +++ b/packages/web-core/src/shared/providers/TerminalProvider.tsx @@ -7,6 +7,7 @@ import { type TerminalInstance, } from '@/shared/hooks/useTerminal'; import { openLocalApiWebSocket } from '@/shared/lib/localApiTransport'; +import { terminalAttemptIsStale } from '@/shared/lib/terminalWsUrl'; interface TerminalConnection { ws: WebSocket; @@ -14,8 +15,32 @@ interface TerminalConnection { resize: (cols: number, rows: number) => void; } +/** + * Poll cadence while a pane is unmeasurable (hidden/detached). A wait-state + * knob, NOT part of the retry backoff ladder — waiting while hidden must + * never spend the retry budget (see `connectWebSocket`). + */ +const UNMEASURED_POLL_MS = 1000; + interface ConnectionGeneration { - endpoint: string; + /** + * Called fresh on every (re)connect attempt; null = pane unmeasurable, + * defer. Full contract on `TerminalContextType.createTerminalConnection` + * and `resolveTerminalEndpoint`. Refreshed by `createTerminalConnection` + * when a remounted component re-registers while this generation is live. + */ + getEndpoint: () => string | null; + /** + * Bumped whenever `getEndpoint` is refreshed (remount / session switch / + * resize tick re-registration). An in-flight open snapshots this before + * awaiting: an unchanged epoch means the endpoint source is untouched and + * the socket is safe to register even if the pane went hidden mid-open; a + * changed epoch requires re-validating against the CURRENT endpoint (see + * `connectWebSocket`), so a session switch that races the open can never + * bind the pane to the previous conversation — including when the pane is + * hidden at resolve time and the endpoint itself is unmeasurable. + */ + endpointEpoch: number; retryCount: number; retryTimer: ReturnType | null; /** Cancelled (tab closed / superseded by a newer generation). */ @@ -303,35 +328,76 @@ export function TerminalProvider({ children }: TerminalProviderProps) { terminalInstancesRef.current.delete(tabId); }, []); + // Stable facade handed to callers: send/resize resolve the CURRENT + // connection on every call, so holders survive reconnects and generation + // swaps. + const makeFacade = useCallback( + (tabId: string) => ({ + send: (data: string) => { + terminalConnectionsRef.current.get(tabId)?.send(data); + }, + resize: (cols: number, rows: number) => { + terminalConnectionsRef.current.get(tabId)?.resize(cols, rows); + }, + }), + [] + ); + const createTerminalConnection = useCallback( ( tabId: string, - endpoint: string, + getEndpoint: () => string | null, onData: (data: string) => void, onExit?: () => void, getSize?: () => { cols: number; rows: number } | null ) => { - // Close any previous connection and cancel its generation. Marking the - // old generation closed (on the object) is what actually cancels its - // in-flight opens/retries; replacing the map slot alone would not. - const previousGeneration = reconnectStateRef.current.get(tabId); - if (previousGeneration) { - previousGeneration.closed = true; - if (previousGeneration.retryTimer) { - clearTimeout(previousGeneration.retryTimer); + // Idempotent while a live connection or in-flight open owns the tab — + // enforced HERE, where the generation state lives, so no caller can + // stack a duplicate backend PTY/tmux attach inside the async open + // window (the socket registers only after the open resolves, which on + // relay transports takes real time). The callbacks are still refreshed: + // a remounted component hands in fresh closures, and the previous + // mount's were built on refs that its cleanup nulled. The generation's + // endpoint source is refreshed for the same reason. + const liveGeneration = reconnectStateRef.current.get(tabId); + const occupied = + terminalConnectionsRef.current.has(tabId) || + (liveGeneration !== undefined && !liveGeneration.closed); + if (occupied) { + connectionCallbacksRef.current.set(tabId, { onData, onExit, getSize }); + if (liveGeneration && !liveGeneration.closed) { + liveGeneration.getEndpoint = getEndpoint; + liveGeneration.endpointEpoch += 1; } + return makeFacade(tabId); } - const existing = terminalConnectionsRef.current.get(tabId); - if (existing) { - existing.ws.close(); - terminalConnectionsRef.current.delete(tabId); + + // Measurability gate, enforced here (not at call sites) so an occupied + // tab above ALWAYS gets its callbacks refreshed even while hidden: a + // pane that has never been measurable gets no generation, no timer, + // and no server-side PTY/tmux churn — callers simply re-invoke on + // resize ticks / mount / session switches until it measures. + if (getEndpoint() === null) { + return makeFacade(tabId); + } + + // Cancel any lingering closed generation and drop its dead socket + // entry. Marking the old generation closed (on the object) is what + // actually cancels its in-flight opens/retries; replacing the map slot + // alone would not. + if (liveGeneration) { + liveGeneration.closed = true; + if (liveGeneration.retryTimer) { + clearTimeout(liveGeneration.retryTimer); + } } // Store callbacks in ref so they can be updated without recreating connection connectionCallbacksRef.current.set(tabId, { onData, onExit, getSize }); const generation: ConnectionGeneration = { - endpoint, + getEndpoint, + endpointEpoch: 0, retryCount: 0, retryTimer: null, closed: false, @@ -354,6 +420,16 @@ export function TerminalProvider({ children }: TerminalProviderProps) { terminalConnectionsRef.current.delete(tabId); }; + const scheduleAttempt = (delay: number) => { + if (generation.closed) { + return; + } + generation.retryTimer = setTimeout(() => { + generation.retryTimer = null; + connectWebSocket(); + }, delay); + }; + const scheduleReconnect = () => { if (generation.closed) { return; @@ -367,10 +443,7 @@ export function TerminalProvider({ children }: TerminalProviderProps) { const delay = Math.min(8000, 500 * Math.pow(2, generation.retryCount)); generation.retryCount += 1; - generation.retryTimer = setTimeout(() => { - generation.retryTimer = null; - connectWebSocket(); - }, delay); + scheduleAttempt(delay); }; const connectWebSocket = () => { @@ -378,6 +451,19 @@ export function TerminalProvider({ children }: TerminalProviderProps) { return; } + // null = pane unmeasurable right now (e.g. hidden tab across an iOS + // background socket kill) — see `resolveTerminalEndpoint`. Being + // hidden is a wait state, not a connect failure: poll without + // spending the retry budget, so a pane hidden longer than the + // backoff ladder doesn't burn its retries and give up before the + // user ever shows it again. + const endpoint = generation.getEndpoint(); + if (endpoint === null) { + scheduleAttempt(UNMEASURED_POLL_MS); + return; + } + const attemptEpoch = generation.endpointEpoch; + void (async () => { try { const ws = await openLocalApiWebSocket(endpoint); @@ -391,14 +477,90 @@ export function TerminalProvider({ children }: TerminalProviderProps) { return; } - ws.onopen = () => { - // Reset retry count on successful connection + // Latches once a re-validation discards this attempt, so the + // socket's own message/close handlers below become no-ops (the + // re-kick is already scheduled). + let superseded = false; + + // Reject an attempt whose endpoint the tab has since refreshed + // (session switch / remount while opening). It MUST run at every + // point the socket could become live: right here after the open + // resolves AND again at `onopen`. The default transport resolves + // the open with a still-CONNECTING socket (see localApiTransport), + // so a switch landing in the post-registration / pre-open gap would + // otherwise let a socket carrying the PREVIOUS session_id become + // the tab's live connection and bind the pane to the wrong + // conversation on a first-attach / post-reap create. + // + // `epochChanged` chooses the strictness (see `terminalAttemptIsStale`): + // - after the open resolves we pass the real epoch delta, so an + // untouched source is accepted without churn (a pane briefly + // unmeasurable WITHOUT a switch keeps its attempt); + // - at `onopen` — the point of no return — we force `true` so the + // endpoint is ALWAYS re-inspected. That closes the gated-child + // race: while `CliMainPane` suppresses `XTermInstance` + // (executorRunning / !sessionsReady) no mounted effect bumps the + // epoch on a session change, so the epoch delta alone would be + // false and wrongly accept the stale socket — but `getEndpoint()` + // is null while gated (the element is detached), so forcing the + // inspection rejects it, and the poll reconnects with the CURRENT + // session once the pane is shown again. + // Discarding is not a failure: re-kick through the single-timer path + // without spending the retry budget (null endpoint → poll). + const discardIfStale = (epochChanged: boolean): boolean => { + const currentEndpoint = generation.getEndpoint(); + if ( + !terminalAttemptIsStale(epochChanged, endpoint, currentEndpoint) + ) { + return false; + } + superseded = true; + if (terminalConnectionsRef.current.get(tabId)?.ws === ws) { + terminalConnectionsRef.current.delete(tabId); + } + ws.close(); + if (generation.retryTimer === null) { + scheduleAttempt( + currentEndpoint === null ? UNMEASURED_POLL_MS : 0 + ); + } + return true; + }; + + if (discardIfStale(generation.endpointEpoch !== attemptEpoch)) { + return; + } + + // End of THIS connection's life without a successor: cancel the + // generation and drop the tab's entries (only if they still + // belong to this ws/generation) so a later mount or session + // switch can start fresh. Without this, the dead socket would + // keep the tab "occupied" and block healing forever. + const release = () => { + generation.closed = true; + if (reconnectStateRef.current.get(tabId) === generation) { + reconnectStateRef.current.delete(tabId); + } + if (terminalConnectionsRef.current.get(tabId)?.ws === ws) { + terminalConnectionsRef.current.delete(tabId); + } + }; + + // Send the current terminal size once the socket is open. The + // initial ResizeObserver fit usually fires before the socket is + // ready and is dropped, which would otherwise leave the PTY/tmux + // stuck at the URL size forever; resending on every (re)connect + // also restores the right size after a reattach. + const syncSize = () => { + // A session switch (or a gate-off that unmounts the terminal + // child) can land after this socket was registered but before it + // opened (the transport returns a CONNECTING socket): re-validate + // — forcing the endpoint inspection — before it becomes live, + // closing the DUP handshake, not a hidden live connection. + if (superseded || discardIfStale(true)) { + return; + } generation.retryCount = 0; - // Send the current terminal size now that the socket is open. The - // initial ResizeObserver fit usually fires before the socket is - // ready and is dropped, which would otherwise leave the PTY/tmux - // stuck at the 80x24 default; resending on every (re)connect also - // restores the right size after a reattach. const size = connectionCallbacksRef.current .get(tabId) ?.getSize?.(); @@ -412,8 +574,24 @@ export function TerminalProvider({ children }: TerminalProviderProps) { ); } }; + ws.onopen = syncSize; + // A pluggable transport may hand back an already-open socket + // whose `open` event has fired; onopen would then never run. + if (ws.readyState === WebSocket.OPEN) { + syncSize(); + // Already-open + stale: syncSize discarded it synchronously — + // don't register a socket we just closed. + if (superseded) { + return; + } + } ws.onmessage = (event) => { + // Discarded stale handshake: a late-arriving frame must not reach + // the (now different-session) pane's callbacks. + if (superseded) { + return; + } try { const msg = JSON.parse(event.data); const callbacks = connectionCallbacksRef.current.get(tabId); @@ -424,8 +602,10 @@ export function TerminalProvider({ children }: TerminalProviderProps) { } else if (msg.type === 'error' && callbacks) { // Hard backend error (e.g. PTY creation failed). Surface it // in the terminal and stop the reconnect loop — retrying a - // failed create_session forever just blinks silently. - generation.closed = true; + // failed create_session forever just blinks silently. The + // release frees the slot so the NEXT user-initiated mount + // or session switch tries once afresh. + release(); callbacks.onData( `\r\n\x1b[31m${msg.message ?? 'terminal error'}\x1b[0m\r\n` ); @@ -440,12 +620,16 @@ export function TerminalProvider({ children }: TerminalProviderProps) { }; ws.onclose = (event) => { - if (generation.closed) { + // `superseded`: WE closed this stale handshake and already + // scheduled the successor — do not reconnect on top of it. + if (superseded || generation.closed) { return; } - // Don't reconnect on clean close (code 1000) or if shell exited + // Clean close (code 1000, e.g. shell exited): don't reconnect, + // but free the slot so a later mount can start a fresh session. if (event.code === 1000 && event.wasClean) { + release(); return; } @@ -476,20 +660,9 @@ export function TerminalProvider({ children }: TerminalProviderProps) { connectWebSocket(); - // Return functions that use the current connection - const send = (data: string) => { - const conn = terminalConnectionsRef.current.get(tabId); - conn?.send(data); - }; - - const resize = (cols: number, rows: number) => { - const conn = terminalConnectionsRef.current.get(tabId); - conn?.resize(cols, rows); - }; - - return { send, resize }; + return makeFacade(tabId); }, - [] + [makeFacade] ); const getTerminalConnection = useCallback(