From 6294e0e300368ba985f7ee11ee1bb646889573da Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 11:43:05 +0000 Subject: [PATCH 01/11] fix(terminal): connect xterm at measured size and rebuild WS endpoint on reconnect Kill the stray-newline-on-switch/reconnect artifact (claude reflowing and stacking blank lines when a CLI pane attaches at xterm's 80x24 default and is then refit to the real size). - XTermInstance: never open the WS at an unmeasured size. Resolve the endpoint from FitAddon.proposeDimensions() after fit(); if the container is hidden/ 0-height, defer the initial connect until the first non-zero ResizeObserver tick instead of baking cols=80&rows=24 into the URL. Never tears down a live connection (hidden mobile tabs keep theirs by design). - TerminalProvider: store a getEndpoint() callback instead of a frozen endpoint string; connectWebSocket rebuilds it fresh on every (re)connect so reconnects attach at the pane's CURRENT grid, and skips+reschedules when unmeasured. - Observability: log cols/rows on the tmux attach (pty.rs) and warn on a mode=cli attach at the default 80x24 (terminal.rs) as a permanent regression tripwire. - Extract resolveTerminalEndpoint() as a pure, node-testable helper; add vitest covering unmeasured->null, measured->real grid, and reconnect re-fit. --- crates/local-deployment/src/pty.rs | 3 +- crates/server/src/routes/terminal.rs | 16 +++ .../src/shared/components/XTermInstance.tsx | 106 +++++++++++------- .../web-core/src/shared/hooks/useTerminal.ts | 6 +- .../src/shared/lib/terminalWsUrl.test.ts | 47 +++++++- .../web-core/src/shared/lib/terminalWsUrl.ts | 25 +++++ .../src/shared/providers/TerminalProvider.tsx | 23 +++- 7 files changed, 182 insertions(+), 44 deletions(-) diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index 4647f84850..845dbddb5e 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -973,7 +973,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 => { diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index 750eea83ef..614bcfb3d3 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -190,6 +190,22 @@ 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. An absent-or-default + // 80x24 means the frontend connected while the terminal was + // unmeasured (hidden / 0-height), which makes claude reflow and + // stack blank lines on the follow-up resize. After the frontend fix + // (never connect at an unmeasured size) this must never fire. + if query.cols == default_cols() && query.rows == default_rows() { + tracing::warn!( + "CLI terminal attaching at default {}x{} for workspace {} — \ + pane likely connected while unmeasured (stray-newline regression)", + query.cols, + query.rows, + query.workspace_id + ); + } + let pool = &deployment.db().pool; // Resolve the uix session driving the handover. A mid-switch diff --git a/packages/web-core/src/shared/components/XTermInstance.tsx b/packages/web-core/src/shared/components/XTermInstance.tsx index 67fa9d80aa..fcf7e3b297 100644 --- a/packages/web-core/src/shared/components/XTermInstance.tsx +++ b/packages/web-core/src/shared/components/XTermInstance.tsx @@ -8,7 +8,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 +68,77 @@ 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({ + // Re-fit and resolve the WS endpoint at the terminal's CURRENT grid. Returns + // null when the pane is unmeasurable (hidden / 0-height): FitAddon.fit() then + // silently no-ops and proposeDimensions() is undefined, so connecting would + // bake xterm's 80x24 constructor default into the URL and make claude 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. + const getEndpoint = useCallback((): string | null => { + const fitAddon = fitAddonRef.current; + const terminal = terminalRef.current; + if (!fitAddon || !terminal) return null; + fitAddon.fit(); + return resolveTerminalEndpoint( + { workspaceId, - cols, - rows, protocol: window.location.protocol, host: window.location.host, mode, sessionId, - }), - [workspaceId, mode, sessionId] - ); + }, + fitAddon.proposeDimensions() + ); + }, [workspaceId, mode, sessionId]); + + // 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(() => { + if (!terminalRef.current || !fitAddonRef.current) return; + if (getTerminalConnection(tabId)) return; + // Gate on measurability up front so no connection object (and no server-side + // PTY/tmux churn) is created for an invisible pane. + if (getEndpoint() === null) return; + createTerminalConnection( + tabId, + getEndpoint, + (data) => terminalRef.current?.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; + } + ); + }, [ + tabId, + getEndpoint, + onClose, + getTerminalConnection, + createTerminalConnection, + ]); const fitTerminal = useCallback(() => { fitAddonRef.current?.fit(); - if (terminalRef.current) { - const conn = getTerminalConnection(tabId); - conn?.resize(terminalRef.current.cols, terminalRef.current.rows); + const terminal = terminalRef.current; + if (!terminal) return; + const conn = getTerminalConnection(tabId); + if (conn) { + conn.resize(terminal.cols, 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, getTerminalConnection, ensureConnection]); // Terminal + connection lifecycle. Every run of this effect MUST register // the same cleanup: an early return without one leaves `terminalRef` @@ -256,24 +302,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,11 +329,9 @@ export function XTermInstance({ }; }, [ tabId, - buildEndpoint, - onClose, + ensureConnection, getTerminalInstance, registerTerminalInstance, - createTerminalConnection, getTerminalConnection, ]); diff --git a/packages/web-core/src/shared/hooks/useTerminal.ts b/packages/web-core/src/shared/hooks/useTerminal.ts index f9c0c88758..d8a52193d3 100644 --- a/packages/web-core/src/shared/hooks/useTerminal.ts +++ b/packages/web-core/src/shared/hooks/useTerminal.ts @@ -50,7 +50,11 @@ export interface TerminalContextType { unregisterTerminalInstance: (tabId: string) => void; 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 reschedules 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..785d377309 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.test.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from 'vitest'; -import { buildTerminalWsUrl } from './terminalWsUrl'; +import { buildTerminalWsUrl, resolveTerminalEndpoint } from './terminalWsUrl'; describe('buildTerminalWsUrl', () => { it('carries the provided grid size instead of the 80x24 default', () => { @@ -57,3 +57,48 @@ 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('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); + }); +}); diff --git a/packages/web-core/src/shared/lib/terminalWsUrl.ts b/packages/web-core/src/shared/lib/terminalWsUrl.ts index ff3e084104..89ccbae747 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.ts @@ -40,3 +40,28 @@ 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 { + if (!dims || dims.cols <= 0 || dims.rows <= 0) return null; + return buildTerminalWsUrl({ ...params, cols: dims.cols, rows: dims.rows }); +} diff --git a/packages/web-core/src/shared/providers/TerminalProvider.tsx b/packages/web-core/src/shared/providers/TerminalProvider.tsx index 0e769ca23a..199c3b97ec 100644 --- a/packages/web-core/src/shared/providers/TerminalProvider.tsx +++ b/packages/web-core/src/shared/providers/TerminalProvider.tsx @@ -15,7 +15,13 @@ interface TerminalConnection { } interface ConnectionGeneration { - endpoint: string; + /** + * Rebuilt on every (re)connect attempt so retries/reconnects attach at the + * pane's CURRENT fitted grid, not the size frozen when the tab was created. + * Returns null when the pane is not measurable yet (hidden/0-height) — the + * attempt is then rescheduled instead of attaching at a placeholder size. + */ + getEndpoint: () => string | null; retryCount: number; retryTimer: ReturnType | null; /** Cancelled (tab closed / superseded by a newer generation). */ @@ -306,7 +312,7 @@ export function TerminalProvider({ children }: TerminalProviderProps) { const createTerminalConnection = useCallback( ( tabId: string, - endpoint: string, + getEndpoint: () => string | null, onData: (data: string) => void, onExit?: () => void, getSize?: () => { cols: number; rows: number } | null @@ -331,7 +337,7 @@ export function TerminalProvider({ children }: TerminalProviderProps) { connectionCallbacksRef.current.set(tabId, { onData, onExit, getSize }); const generation: ConnectionGeneration = { - endpoint, + getEndpoint, retryCount: 0, retryTimer: null, closed: false, @@ -378,6 +384,17 @@ export function TerminalProvider({ children }: TerminalProviderProps) { return; } + // Rebuild the endpoint at the CURRENT fitted grid on every attempt. A + // null means the pane isn't measurable yet (e.g. a hidden tab across an + // iOS background socket kill); reschedule rather than attach at a + // placeholder 80x24 that would make claude reflow on the follow-up + // resize. + const endpoint = generation.getEndpoint(); + if (endpoint === null) { + scheduleReconnect(); + return; + } + void (async () => { try { const ws = await openLocalApiWebSocket(endpoint); From 56dcac81a3de2279f1e00741b42609ac159be069 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 13:39:16 +0000 Subject: [PATCH 02/11] fix(terminal): disarm pty VEOF on teardown to stop \n+EOT keystroke injection; add attach-window input tripwire --- Cargo.lock | 1 + crates/local-deployment/Cargo.toml | 3 + crates/local-deployment/src/pty.rs | 102 +++++++++++++++++++++++++++ crates/server/src/routes/terminal.rs | 53 ++++++++++++++ 4 files changed, 159 insertions(+) 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 845dbddb5e..890b850d73 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -891,9 +891,51 @@ 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 { + return; + }; + unsafe { + let mut termios: libc::termios = std::mem::zeroed(); + if libc::tcgetattr(fd, &mut termios) == 0 { + termios.c_cc[libc::VEOF] = 0; + let _ = libc::tcsetattr(fd, libc::TCSANOW, &termios); + } + } +} + +#[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`). + /// + /// Field order matters: `writer` is declared before `master` so that on + /// drop it is dropped first — and `Drop for PtySession` runs before either, + /// disarming the master's VEOF so this writer's own Drop injects nothing. writer: Arc>>, master: Box, /// Kills the PTY child (tmux client / shell) on teardown. Required because @@ -911,6 +953,21 @@ struct PtySession { 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 fields then drop in + // declaration order (`writer` first), so this guarantees + // `portable-pty`'s writer-Drop sees `VEOF == 0` and skips its `\n` + + // Ctrl-D injection. This is the single teardown point that every path + // funnels through — `close_session` (map `remove` + drop), process/ + // service shutdown (the sessions `HashMap` dropping), and any future + // cleanup path — so the disarm can never be forgotten. See + // `disarm_master_eof` for why the injection happens at all. + disarm_master_eof(self.master.as_ref()); + } +} + #[derive(Clone)] pub struct PtyService { sessions: Arc>>, @@ -1231,6 +1288,9 @@ impl PtyService { if !session.child_reaped.load(Ordering::Acquire) { let _ = session.child_killer.kill(); } + // `session` drops at the end of this scope: `Drop for PtySession` + // disarms the master's VEOF first, so the writer's own Drop does not + // inject `\n` + Ctrl-D into the (now detaching) tmux client's tty. } Ok(()) } @@ -1467,4 +1527,46 @@ 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 + } + } + } + + /// 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. Uses the same `openpty` path as `create_session`. + #[cfg(unix)] + #[test] + fn disarm_master_eof_zeroes_veof() { + let pair = NativePtySystem::default() + .openpty(PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + .expect("openpty"); + + // 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"); + } } diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index 614bcfb3d3..69ea7bbb2f 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -382,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) @@ -395,6 +403,21 @@ async fn handle_terminal_ws( } }; + // FIX 4 input tripwire: for a strictly bounded window right after 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 we can tell a client-origin injection from a + // server-side one. Bounded to the first `TRIPWIRE_WINDOW` OR + // `TRIPWIRE_MAX_BYTES` (whichever first); it allocates nothing once the + // window closes, so it is permanently safe to leave enabled. + const TRIPWIRE_WINDOW: std::time::Duration = std::time::Duration::from_secs(2); + const TRIPWIRE_MAX_BYTES: usize = 128; + let attach_started = std::time::Instant::now(); + let mut tripwire_bytes_logged: usize = 0; + // 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 @@ -439,6 +462,23 @@ async fn handle_terminal_ws( match cmd { TerminalCommand::Input { data } => { if let Ok(bytes) = BASE64.decode(&data) { + // FIX 4 tripwire (bounded — see above). + let elapsed = attach_started.elapsed(); + if !bytes.is_empty() + && tripwire_bytes_logged < TRIPWIRE_MAX_BYTES + && elapsed < TRIPWIRE_WINDOW + { + let take = (TRIPWIRE_MAX_BYTES - tripwire_bytes_logged) + .min(bytes.len()); + tracing::info!( + session = %tripwire_session, + attach = %session_id_for_input, + ms_since_attach = elapsed.as_millis() as u64, + bytes = %hex_dump(&bytes[..take]), + "terminal input (attach-window tripwire)" + ); + tripwire_bytes_logged += take; + } let _ = pty_service.write(session_id_for_input, &bytes).await; } } @@ -463,6 +503,19 @@ async fn handle_terminal_ws( let _ = deployment.pty().close_session(session_id).await; } +/// Space-separated lowercase hex of a byte slice (e.g. `0a 04`) for the +/// attach-window input tripwire. 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 { + let _ = write!(out, "{byte:02x} "); + } + out.pop(); // trailing space + out +} + async fn send_error(socket: &mut MaybeSignedWebSocket, message: &str) -> anyhow::Result<()> { let msg = TerminalMessage::Error { message: message.to_string(), From 8ae6b98d2f9c6e5d15c9978787f34970204d0649 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 13:44:03 +0000 Subject: [PATCH 03/11] fix(terminal): redact printable bytes in attach-window input tripwire logs Control bytes (the \n+EOT injection / escape-sequence signature) stay verbatim; printable keystrokes are masked so passwords or pasted secrets never reach server logs. --- crates/server/src/routes/terminal.rs | 31 +++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index 69ea7bbb2f..5e3afa58a4 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -503,14 +503,22 @@ async fn handle_terminal_ws( let _ = deployment.pty().close_session(session_id).await; } -/// Space-separated lowercase hex of a byte slice (e.g. `0a 04`) for the -/// attach-window input tripwire. The caller bounds the slice length, so this +/// 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 { - let _ = write!(out, "{byte:02x} "); + if *byte < 0x20 || *byte == 0x7f { + let _ = write!(out, "{byte:02x} "); + } else { + out.push_str(".. "); + } } out.pop(); // trailing space out @@ -529,3 +537,20 @@ 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 super::hex_dump; + + #[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(&[]), ""); + } +} From 8e1f82fc489ad84645c3e62e3979d6d798b11cbf Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 16:26:54 +0000 Subject: [PATCH 04/11] fix(terminal): close VEOF disarm gaps and remount-stale terminal callbacks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Council review round 1: - pty.rs: a failure between take_writer() and PtySession construction (e.g. try_clone_reader under fd pressure) dropped the writer with VEOF still armed, injecting \n+EOT into the just-spawned child — cover the window with a VeofDisarmOnDrop guard (defused once PtySession owns the writer) + unit test; log tcsetattr failure in disarm_master_eof; state the real Drop guarantee (device-scoped termios persistence) in the comments instead of overselling field order. - XTermInstance: the stored connection callbacks captured this mount's refs, which cleanup nulls — after a gate-flip remount (executorRunning toggle, hidden side tabs) a surviving connection silently dropped all output; resolve terminal/fitAddon through the provider registry (same lifetime as the connection) and read sessionId via a latest-value ref so later reconnects join the session the UI currently shows. - TerminalProvider: expose hasTerminalConnection (established OR live in-flight generation) and gate ensureConnection on it so two quick calls can't stack a duplicate backend PTY/tmux attach inside the async open window (real on slow relay transports). - terminal.rs: lock the tripwire redaction invariant with an exhaustive 0x00-0xff mask test. --- crates/local-deployment/src/pty.rs | 114 ++++++++++++++++-- crates/server/src/routes/terminal.rs | 16 +++ .../src/shared/components/XTermInstance.tsx | 55 ++++++--- .../web-core/src/shared/hooks/useTerminal.ts | 8 ++ .../src/shared/providers/TerminalProvider.tsx | 13 ++ 5 files changed, 179 insertions(+), 27 deletions(-) diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index 890b850d73..6bee3430fe 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -921,7 +921,16 @@ fn disarm_master_eof(master: &dyn portable_pty::MasterPty) { let mut termios: libc::termios = std::mem::zeroed(); if libc::tcgetattr(fd, &mut termios) == 0 { termios.c_cc[libc::VEOF] = 0; - let _ = libc::tcsetattr(fd, libc::TCSANOW, &termios); + if libc::tcsetattr(fd, libc::TCSANOW, &termios) != 0 { + // Runs inside Drop, so never panic — but a failed set leaves + // the writer's injection path armed, which is exactly what + // the attach-window tripwire would then catch; leave a trace + // so the two can be correlated. + tracing::debug!( + "tcsetattr failed disarming pty VEOF; writer teardown may \ + inject \\n+EOT" + ); + } } } } @@ -929,13 +938,39 @@ fn disarm_master_eof(master: &dyn portable_pty::MasterPty) { #[cfg(not(unix))] fn disarm_master_eof(_master: &dyn portable_pty::MasterPty) {} +/// Disarms the master's VEOF on drop unless defused. Covers the window in +/// `create_session` between `take_writer()` succeeding and the `PtySession` +/// being constructed (whose own `Drop` takes over the disarm duty): an early +/// return (e.g. `try_clone_reader` failing) or a panic-unwind inside that +/// window drops the writer with no `PtySession` in existence, and its Drop +/// would inject `\n` + Ctrl-D into the just-spawned child. Declared AFTER the +/// writer local so it drops FIRST (locals drop in reverse declaration order). +struct VeofDisarmOnDrop<'a> { + master: &'a dyn portable_pty::MasterPty, + /// Set to true once the writer's ownership is safely inside a + /// `PtySession`; the guard then does nothing. + defused: bool, +} + +impl Drop for VeofDisarmOnDrop<'_> { + fn drop(&mut self) { + if !self.defused { + disarm_master_eof(self.master); + } + } +} + struct PtySession { /// Per-session writer behind its own lock so a blocking PTY write never /// holds up the global session registry (see `write`). /// - /// Field order matters: `writer` is declared before `master` so that on - /// drop it is dropped first — and `Drop for PtySession` runs before either, - /// disarming the master's VEOF so this writer's own Drop injects nothing. + /// Drop safety: `Drop for PtySession` disarms the master's VEOF before any + /// field drops. That disarm is a tcsetattr on the pty DEVICE — persistent + /// state shared by every dup'd fd — so this writer's own Drop reads + /// `VEOF == 0` and injects nothing regardless of when it runs (even from + /// an `Arc` clone in `write` that outlives the session, or after `master` + /// closes). The `writer`-before-`master` declaration order 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 @@ -955,15 +990,17 @@ struct PtySession { 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 fields then drop in - // declaration order (`writer` first), so this guarantees - // `portable-pty`'s writer-Drop sees `VEOF == 0` and skips its `\n` + - // Ctrl-D injection. This is the single teardown point that every path - // funnels through — `close_session` (map `remove` + drop), process/ - // service shutdown (the sessions `HashMap` dropping), and any future - // cleanup path — so the disarm can never be forgotten. See - // `disarm_master_eof` for why the injection happens at all. + // 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. This is the teardown point every + // session-owning path funnels through — `close_session` (map `remove` + // + drop), process/service shutdown (the sessions `HashMap` dropping), + // and any future cleanup path — so the disarm can never be forgotten. + // (The pre-session creation window is covered separately by + // `VeofDisarmOnDrop` in `create_session`.) See `disarm_master_eof` + // for why the injection happens at all. disarm_master_eof(self.master.as_ref()); } } @@ -1139,6 +1176,15 @@ impl PtyService { .take_writer() .map_err(|e| PtyError::CreateFailed(e.to_string()))?; + // From here until the `PtySession` exists there is a writer whose + // Drop injects `\n` + Ctrl-D but no session Drop to disarm it; the + // guard covers every early return and unwind in that window. + // Declared after `writer` so it drops first. + let mut veof_guard = VeofDisarmOnDrop { + master: pty_pair.master.as_ref(), + defused: false, + }; + if shell_name == "zsh" { let _ = writer.write_all(b" PROMPT='$ '; RPROMPT=''\n"); let _ = writer.flush(); @@ -1176,6 +1222,12 @@ impl PtyService { child_reaped_reader.store(true, Ordering::Release); }); + // The writer will be owned by a `PtySession`, whose own Drop takes + // over the disarm duty from here. Dropped explicitly to release + // the guard's borrow before `pty_pair.master` moves out. + veof_guard.defused = true; + drop(veof_guard); + Ok::<_, PtyError>(( pty_pair.master, writer, @@ -1569,4 +1621,40 @@ mod tests { let after = master_veof(pair.master.as_ref()).expect("tcgetattr after"); assert_eq!(after, 0, "VEOF must be disarmed after disarm_master_eof"); } + + /// The `create_session` failure-window guard: dropped undefused (early + /// return / unwind before `PtySession` exists) it must disarm VEOF; + /// dropped defused (ownership handed to `PtySession`) it must leave the + /// live session's VEOF armed. + #[cfg(unix)] + #[test] + fn veof_guard_disarms_only_when_not_defused() { + let pair = NativePtySystem::default() + .openpty(PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + .expect("openpty"); + + // Defused guard (success path): VEOF stays armed for the live session. + let guard = VeofDisarmOnDrop { + master: pair.master.as_ref(), + defused: true, + }; + drop(guard); + let veof = master_veof(pair.master.as_ref()).expect("tcgetattr"); + assert_ne!(veof, 0, "a defused guard must not touch VEOF"); + + // Undefused guard (failure path): VEOF is disarmed before the writer + // local would drop. + let guard = VeofDisarmOnDrop { + master: pair.master.as_ref(), + defused: false, + }; + drop(guard); + let veof = master_veof(pair.master.as_ref()).expect("tcgetattr"); + assert_eq!(veof, 0, "an undefused guard must disarm VEOF"); + } } diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index 5e3afa58a4..577496c76e 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -553,4 +553,20 @@ mod tripwire_tests { 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 fcf7e3b297..4fcad1393b 100644 --- a/packages/web-core/src/shared/components/XTermInstance.tsx +++ b/packages/web-core/src/shared/components/XTermInstance.tsx @@ -66,8 +66,21 @@ export function XTermInstance({ getTerminalInstance, createTerminalConnection, getTerminalConnection, + hasTerminalConnection, } = useTerminal(); + // Latest sessionId for getEndpoint. The provider stores getEndpoint on the + // connection generation for the connection's whole lifetime, and that + // lifetime spans session switches (a live connection is never torn down on + // a sessionId change). Reading the prop through a ref keeps every later + // reconnect on the session the UI currently shows — a value captured at + // connect time could resume the wrong conversation if the tmux session was + // meanwhile reaped and the reconnect had to recreate it. + const sessionIdRef = useRef(sessionId); + useEffect(() => { + sessionIdRef.current = sessionId; + }); + // Re-fit and resolve the WS endpoint at the terminal's CURRENT grid. Returns // null when the pane is unmeasurable (hidden / 0-height): FitAddon.fit() then // silently no-ops and proposeDimensions() is undefined, so connecting would @@ -75,22 +88,26 @@ export function XTermInstance({ // (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. + // + // Resolves the terminal through the provider REGISTRY, not this component's + // refs: the stored callback lives as long as the connection, which survives + // XTermInstance remounts (CliMainPane gate flips, hidden side tabs), while + // this component's refs are nulled on unmount. const getEndpoint = useCallback((): string | null => { - const fitAddon = fitAddonRef.current; - const terminal = terminalRef.current; - if (!fitAddon || !terminal) return null; - fitAddon.fit(); + const instance = getTerminalInstance(tabId); + if (!instance) return null; + instance.fitAddon.fit(); return resolveTerminalEndpoint( { workspaceId, protocol: window.location.protocol, host: window.location.host, mode, - sessionId, + sessionId: sessionIdRef.current, }, - fitAddon.proposeDimensions() + instance.fitAddon.proposeDimensions() ); - }, [workspaceId, mode, sessionId]); + }, [tabId, workspaceId, mode, getTerminalInstance]); // 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) @@ -100,29 +117,39 @@ export function XTermInstance({ // 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(() => { - if (!terminalRef.current || !fitAddonRef.current) return; - if (getTerminalConnection(tabId)) return; + // A live connection OR an in-flight/pending one counts as "already + // connected": the provider registers the socket only after its async open + // resolves, and stacking a second generation into that window would churn + // a duplicate backend PTY/tmux attach (slow relay transports make the + // window real). + if (hasTerminalConnection(tabId)) return; // Gate on measurability up front so no connection object (and no server-side // PTY/tmux churn) is created for an invisible pane. if (getEndpoint() === null) return; createTerminalConnection( tabId, getEndpoint, - (data) => terminalRef.current?.write(data), + // Both callbacks resolve through the registry on every call: they live + // as long as the connection, which survives XTermInstance remounts, + // while this component's refs are nulled on unmount — a closure over + // them would silently drop all output after a reattach. + (data) => getTerminalInstance(tabId)?.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; + const instance = getTerminalInstance(tabId); + if (!instance) return null; + instance.fitAddon.fit(); + return { cols: instance.terminal.cols, rows: instance.terminal.rows }; } ); }, [ tabId, getEndpoint, onClose, - getTerminalConnection, + getTerminalInstance, + hasTerminalConnection, createTerminalConnection, ]); diff --git a/packages/web-core/src/shared/hooks/useTerminal.ts b/packages/web-core/src/shared/hooks/useTerminal.ts index d8a52193d3..c480440e1f 100644 --- a/packages/web-core/src/shared/hooks/useTerminal.ts +++ b/packages/web-core/src/shared/hooks/useTerminal.ts @@ -63,6 +63,14 @@ export interface TerminalContextType { resize: (cols: number, rows: number) => void; }; getTerminalConnection: (tabId: string) => TerminalConnection | null; + /** + * True when the tab has an established connection OR a live in-flight one + * (generation created, socket not registered yet — the async open hasn't + * resolved). Connect-or-defer callers must gate on this, not on + * getTerminalConnection, or two quick calls stack duplicate backend + * PTY/tmux attaches inside the open window. + */ + hasTerminalConnection: (tabId: string) => boolean; } export const TerminalContext = createHmrContext( diff --git a/packages/web-core/src/shared/providers/TerminalProvider.tsx b/packages/web-core/src/shared/providers/TerminalProvider.tsx index 199c3b97ec..cf114b6ba3 100644 --- a/packages/web-core/src/shared/providers/TerminalProvider.tsx +++ b/packages/web-core/src/shared/providers/TerminalProvider.tsx @@ -516,6 +516,17 @@ export function TerminalProvider({ children }: TerminalProviderProps) { [] ); + const hasTerminalConnection = useCallback((tabId: string): boolean => { + if (terminalConnectionsRef.current.has(tabId)) return true; + // A live generation with no registered socket yet is an in-flight open or + // a scheduled retry — callers must treat it as occupied so they never + // stack a second connect (and a duplicate backend PTY/tmux attach) on top + // of it. Cancelled generations are flagged closed / pruned and don't + // count. + const generation = reconnectStateRef.current.get(tabId); + return !!generation && !generation.closed; + }, []); + const value = useMemo( () => ({ getTabsForWorkspace, @@ -530,6 +541,7 @@ export function TerminalProvider({ children }: TerminalProviderProps) { unregisterTerminalInstance, createTerminalConnection, getTerminalConnection, + hasTerminalConnection, }), [ getTabsForWorkspace, @@ -544,6 +556,7 @@ export function TerminalProvider({ children }: TerminalProviderProps) { unregisterTerminalInstance, createTerminalConnection, getTerminalConnection, + hasTerminalConnection, ] ); From 3fbed70c1c3e0ced82557aa3c42ed63d1c653a0e Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 19:50:12 +0000 Subject: [PATCH 05/11] =?UTF-8?q?fix(terminal):=20simplify=20pass=20?= =?UTF-8?q?=E2=80=94=20owned-fd=20VEOF=20guard,=20tripwire=20struct,=20hid?= =?UTF-8?q?den-pane=20retry=20budget?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /simplify review (reuse / simplification / efficiency / altitude): - pty.rs: VeofDisarmOnDrop now owns a dup(2) of the master fd and rides the spawn_blocking return tuple (declared first — tuple fields drop in order), extending creation-window coverage to the request future being cancelled at the .await, where the detached task's tuple previously dropped the writer VEOF-armed with no guard alive; shared disarm_eof_on_fd helper; writer-field doc deduped; open_test_pty test helper; guard test updated for the owned-fd form. - terminal.rs: attach-window tripwire state/bounds co-located in an AttachInputTripwire struct — identical 2s/128-byte semantics; expiry now latches the spent budget so post-window frames cost one integer compare. - TerminalProvider: an unmeasurable pane is a wait state, not a connect failure — poll at 1s without spending the retry budget, so a pane hidden longer than the backoff ladder doesn't burn its 6 retries and give up before the user ever shows it; getEndpoint contract comments deduped to point at resolveTerminalEndpoint. - XTermInstance: shared fitInstance() helper for the registry-resolved callbacks; onClose read via latest-ref so its unstable prop identity stops churning ensureConnection/fitTerminal and the ResizeObserver effect on every parent render. --- crates/local-deployment/src/pty.rs | 169 +++++++++++------- crates/server/src/routes/terminal.rs | 93 ++++++---- .../src/shared/components/XTermInstance.tsx | 51 +++--- .../src/shared/providers/TerminalProvider.tsx | 35 ++-- 4 files changed, 215 insertions(+), 133 deletions(-) diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index 6bee3430fe..ba2fff109e 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -912,11 +912,11 @@ pub enum PtyError { /// 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. +/// The termios write shared by `disarm_master_eof` and `VeofDisarmOnDrop`. +/// termios is pty-DEVICE state shared across every dup'd fd, so a disarm +/// through any fd of the pty is seen by the writer's own `tcgetattr`. #[cfg(unix)] -fn disarm_master_eof(master: &dyn portable_pty::MasterPty) { - let Some(fd) = master.as_raw_fd() else { - return; - }; +fn disarm_eof_on_fd(fd: std::os::unix::io::RawFd) { unsafe { let mut termios: libc::termios = std::mem::zeroed(); if libc::tcgetattr(fd, &mut termios) == 0 { @@ -935,27 +935,72 @@ fn disarm_master_eof(master: &dyn portable_pty::MasterPty) { } } +#[cfg(unix)] +fn disarm_master_eof(master: &dyn portable_pty::MasterPty) { + let Some(fd) = master.as_raw_fd() else { + return; + }; + disarm_eof_on_fd(fd); +} + #[cfg(not(unix))] fn disarm_master_eof(_master: &dyn portable_pty::MasterPty) {} -/// Disarms the master's VEOF on drop unless defused. Covers the window in +/// Disarms the pty's VEOF on drop unless defused. Covers the whole window in /// `create_session` between `take_writer()` succeeding and the `PtySession` -/// being constructed (whose own `Drop` takes over the disarm duty): an early -/// return (e.g. `try_clone_reader` failing) or a panic-unwind inside that -/// window drops the writer with no `PtySession` in existence, and its Drop -/// would inject `\n` + Ctrl-D into the just-spawned child. Declared AFTER the -/// writer local so it drops FIRST (locals drop in reverse declaration order). -struct VeofDisarmOnDrop<'a> { - master: &'a dyn portable_pty::MasterPty, - /// Set to true once the writer's ownership is safely inside a +/// being inserted into the registry (whose own `Drop` takes over the disarm +/// duty from there): an early return (`try_clone_reader` failing), a +/// panic-unwind, or the request future being cancelled at the +/// `spawn_blocking` `.await` (the detached blocking task still completes and +/// the runtime then drops its returned tuple) would each drop the writer +/// with no `PtySession` in existence, and the writer's Drop would inject +/// `\n` + Ctrl-D into the just-spawned child. +/// +/// Owns a `dup(2)` of the master fd instead of borrowing the master so it +/// can travel across the `spawn_blocking` boundary inside the returned tuple +/// — declared FIRST there, because tuple fields drop in declaration order +/// and the disarm must precede the writer's Drop on a cancellation drop. +struct VeofDisarmOnDrop { + #[cfg(unix)] + fd: Option, + /// Set once the writer's ownership is safely inside a registered /// `PtySession`; the guard then does nothing. defused: bool, } -impl Drop for VeofDisarmOnDrop<'_> { +impl VeofDisarmOnDrop { + fn new(master: &dyn portable_pty::MasterPty) -> Self { + #[cfg(unix)] + { + let fd = master.as_raw_fd().and_then(|raw| { + // SAFETY: `raw` is a live fd owned by `master`, which outlives + // this immediate clone-to-owned. + let borrowed = unsafe { std::os::fd::BorrowedFd::borrow_raw(raw) }; + borrowed.try_clone_to_owned().ok() + }); + Self { fd, defused: false } + } + #[cfg(not(unix))] + { + let _ = master; + Self { defused: false } + } + } + + fn defuse(&mut self) { + self.defused = true; + } +} + +impl Drop for VeofDisarmOnDrop { fn drop(&mut self) { - if !self.defused { - disarm_master_eof(self.master); + if self.defused { + return; + } + #[cfg(unix)] + if let Some(fd) = &self.fd { + use std::os::fd::AsRawFd; + disarm_eof_on_fd(fd.as_raw_fd()); } } } @@ -964,12 +1009,8 @@ 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: `Drop for PtySession` disarms the master's VEOF before any - /// field drops. That disarm is a tcsetattr on the pty DEVICE — persistent - /// state shared by every dup'd fd — so this writer's own Drop reads - /// `VEOF == 0` and injects nothing regardless of when it runs (even from - /// an `Arc` clone in `write` that outlives the session, or after `master` - /// closes). The `writer`-before-`master` declaration order is only + /// 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, @@ -1176,14 +1217,11 @@ impl PtyService { .take_writer() .map_err(|e| PtyError::CreateFailed(e.to_string()))?; - // From here until the `PtySession` exists there is a writer whose - // Drop injects `\n` + Ctrl-D but no session Drop to disarm it; the - // guard covers every early return and unwind in that window. - // Declared after `writer` so it drops first. - let mut veof_guard = VeofDisarmOnDrop { - master: pty_pair.master.as_ref(), - defused: false, - }; + // From here until the `PtySession` is registered there is a writer + // whose Drop injects `\n` + Ctrl-D but no session Drop to disarm + // it; the guard rides the returned tuple to cover every early + // return, unwind, and cancellation in that window (see its doc). + let veof_guard = VeofDisarmOnDrop::new(pty_pair.master.as_ref()); if shell_name == "zsh" { let _ = writer.write_all(b" PROMPT='$ '; RPROMPT=''\n"); @@ -1222,13 +1260,11 @@ impl PtyService { child_reaped_reader.store(true, Ordering::Release); }); - // The writer will be owned by a `PtySession`, whose own Drop takes - // over the disarm duty from here. Dropped explicitly to release - // the guard's borrow before `pty_pair.master` moves out. - veof_guard.defused = true; - drop(veof_guard); - + // Guard first: tuple fields drop in declaration order, so if this + // tuple is dropped un-consumed (request future cancelled at the + // `.await` below), the guard disarms before the writer drops. Ok::<_, PtyError>(( + veof_guard, pty_pair.master, writer, child_killer, @@ -1239,7 +1275,7 @@ impl PtyService { .await .map_err(|e| PtyError::CreateFailed(e.to_string()))??; - let (master, writer, child_killer, child_reaped, output_handle) = result; + let (mut veof_guard, master, writer, child_killer, child_reaped, output_handle) = result; let session = PtySession { writer: Arc::new(Mutex::new(writer)), @@ -1255,6 +1291,12 @@ impl PtyService { .map_err(|e| PtyError::CreateFailed(e.to_string()))? .insert(session_id, session); + // The writer is now owned by a registered `PtySession`, whose own Drop + // holds the disarm duty from here. (If the insert above failed, both + // the constructed session's Drop and the still-armed guard disarm — + // idempotent.) + veof_guard.defuse(); + Ok((session_id, output_rx)) } @@ -1596,20 +1638,26 @@ mod tests { } } - /// 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. Uses the same `openpty` path as `create_session`. + /// Same `openpty` path as `create_session`. #[cfg(unix)] - #[test] - fn disarm_master_eof_zeroes_veof() { - let pair = NativePtySystem::default() + fn open_test_pty() -> portable_pty::PtyPair { + NativePtySystem::default() .openpty(PtySize { rows: 24, cols: 80, pixel_width: 0, pixel_height: 0, }) - .expect("openpty"); + .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. @@ -1622,37 +1670,26 @@ mod tests { assert_eq!(after, 0, "VEOF must be disarmed after disarm_master_eof"); } - /// The `create_session` failure-window guard: dropped undefused (early - /// return / unwind before `PtySession` exists) it must disarm VEOF; - /// dropped defused (ownership handed to `PtySession`) it must leave the - /// live session's VEOF armed. + /// The `create_session` creation-window guard: dropped undefused (early + /// return / unwind / cancelled `.await` before the `PtySession` is + /// registered) it must disarm VEOF — via its own dup'd fd, independent of + /// the master; dropped defused (ownership handed to `PtySession`) it must + /// leave the live session's VEOF armed. #[cfg(unix)] #[test] fn veof_guard_disarms_only_when_not_defused() { - let pair = NativePtySystem::default() - .openpty(PtySize { - rows: 24, - cols: 80, - pixel_width: 0, - pixel_height: 0, - }) - .expect("openpty"); + let pair = open_test_pty(); // Defused guard (success path): VEOF stays armed for the live session. - let guard = VeofDisarmOnDrop { - master: pair.master.as_ref(), - defused: true, - }; + let mut guard = VeofDisarmOnDrop::new(pair.master.as_ref()); + guard.defuse(); drop(guard); let veof = master_veof(pair.master.as_ref()).expect("tcgetattr"); assert_ne!(veof, 0, "a defused guard must not touch VEOF"); - // Undefused guard (failure path): VEOF is disarmed before the writer - // local would drop. - let guard = VeofDisarmOnDrop { - master: pair.master.as_ref(), - defused: false, - }; + // Undefused guard (failure/cancellation path): VEOF is disarmed before + // the writer would drop, even with the master untouched. + let guard = VeofDisarmOnDrop::new(pair.master.as_ref()); drop(guard); let veof = master_veof(pair.master.as_ref()).expect("tcgetattr"); assert_eq!(veof, 0, "an undefused guard must disarm VEOF"); diff --git a/crates/server/src/routes/terminal.rs b/crates/server/src/routes/terminal.rs index 577496c76e..85d865827c 100644 --- a/crates/server/src/routes/terminal.rs +++ b/crates/server/src/routes/terminal.rs @@ -403,20 +403,8 @@ async fn handle_terminal_ws( } }; - // FIX 4 input tripwire: for a strictly bounded window right after 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 we can tell a client-origin injection from a - // server-side one. Bounded to the first `TRIPWIRE_WINDOW` OR - // `TRIPWIRE_MAX_BYTES` (whichever first); it allocates nothing once the - // window closes, so it is permanently safe to leave enabled. - const TRIPWIRE_WINDOW: std::time::Duration = std::time::Duration::from_secs(2); - const TRIPWIRE_MAX_BYTES: usize = 128; - let attach_started = std::time::Instant::now(); - let mut tripwire_bytes_logged: usize = 0; + // 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 @@ -462,23 +450,7 @@ async fn handle_terminal_ws( match cmd { TerminalCommand::Input { data } => { if let Ok(bytes) = BASE64.decode(&data) { - // FIX 4 tripwire (bounded — see above). - let elapsed = attach_started.elapsed(); - if !bytes.is_empty() - && tripwire_bytes_logged < TRIPWIRE_MAX_BYTES - && elapsed < TRIPWIRE_WINDOW - { - let take = (TRIPWIRE_MAX_BYTES - tripwire_bytes_logged) - .min(bytes.len()); - tracing::info!( - session = %tripwire_session, - attach = %session_id_for_input, - ms_since_attach = elapsed.as_millis() as u64, - bytes = %hex_dump(&bytes[..take]), - "terminal input (attach-window tripwire)" - ); - tripwire_bytes_logged += take; - } + tripwire.observe(&bytes); let _ = pty_service.write(session_id_for_input, &bytes).await; } } @@ -503,6 +475,65 @@ 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 diff --git a/packages/web-core/src/shared/components/XTermInstance.tsx b/packages/web-core/src/shared/components/XTermInstance.tsx index 4fcad1393b..96ac1622d4 100644 --- a/packages/web-core/src/shared/components/XTermInstance.tsx +++ b/packages/web-core/src/shared/components/XTermInstance.tsx @@ -69,18 +69,33 @@ export function XTermInstance({ hasTerminalConnection, } = useTerminal(); - // Latest sessionId for getEndpoint. The provider stores getEndpoint on the - // connection generation for the connection's whole lifetime, and that - // lifetime spans session switches (a live connection is never torn down on - // a sessionId change). Reading the prop through a ref keeps every later - // reconnect on the session the UI currently shows — a value captured at + // 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. + // 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); useEffect(() => { 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 (hidden / 0-height): FitAddon.fit() then // silently no-ops and proposeDimensions() is undefined, so connecting would @@ -88,15 +103,9 @@ export function XTermInstance({ // (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. - // - // Resolves the terminal through the provider REGISTRY, not this component's - // refs: the stored callback lives as long as the connection, which survives - // XTermInstance remounts (CliMainPane gate flips, hidden side tabs), while - // this component's refs are nulled on unmount. const getEndpoint = useCallback((): string | null => { - const instance = getTerminalInstance(tabId); + const instance = fitInstance(); if (!instance) return null; - instance.fitAddon.fit(); return resolveTerminalEndpoint( { workspaceId, @@ -107,7 +116,7 @@ export function XTermInstance({ }, instance.fitAddon.proposeDimensions() ); - }, [tabId, workspaceId, mode, getTerminalInstance]); + }, [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) @@ -129,25 +138,23 @@ export function XTermInstance({ createTerminalConnection( tabId, getEndpoint, - // Both callbacks resolve through the registry on every call: they live - // as long as the connection, which survives XTermInstance remounts, - // while this component's refs are nulled on unmount — a closure over - // them would silently drop all output after a reattach. + // 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), - onClose, + () => 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 = getTerminalInstance(tabId); + const instance = fitInstance(); if (!instance) return null; - instance.fitAddon.fit(); return { cols: instance.terminal.cols, rows: instance.terminal.rows }; } ); }, [ tabId, getEndpoint, - onClose, + fitInstance, getTerminalInstance, hasTerminalConnection, createTerminalConnection, diff --git a/packages/web-core/src/shared/providers/TerminalProvider.tsx b/packages/web-core/src/shared/providers/TerminalProvider.tsx index cf114b6ba3..15cdc0fc29 100644 --- a/packages/web-core/src/shared/providers/TerminalProvider.tsx +++ b/packages/web-core/src/shared/providers/TerminalProvider.tsx @@ -16,10 +16,9 @@ interface TerminalConnection { interface ConnectionGeneration { /** - * Rebuilt on every (re)connect attempt so retries/reconnects attach at the - * pane's CURRENT fitted grid, not the size frozen when the tab was created. - * Returns null when the pane is not measurable yet (hidden/0-height) — the - * attempt is then rescheduled instead of attaching at a placeholder size. + * Called fresh on every (re)connect attempt; null = pane unmeasurable, + * defer. Full contract on `TerminalContextType.createTerminalConnection` + * and `resolveTerminalEndpoint`. */ getEndpoint: () => string | null; retryCount: number; @@ -360,6 +359,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; @@ -373,10 +382,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 = () => { @@ -384,14 +390,15 @@ export function TerminalProvider({ children }: TerminalProviderProps) { return; } - // Rebuild the endpoint at the CURRENT fitted grid on every attempt. A - // null means the pane isn't measurable yet (e.g. a hidden tab across an - // iOS background socket kill); reschedule rather than attach at a - // placeholder 80x24 that would make claude reflow on the follow-up - // resize. + // 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) { - scheduleReconnect(); + scheduleAttempt(1000); return; } From 1e2a1b16d5e6561d936da135835959c289cbc964 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 21:17:51 +0000 Subject: [PATCH 06/11] fix(terminal): deepen teardown to PtySession::drop; harden connect gating and healing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase-3 review battery (codex xhigh + 10-angle finder pass), all fixes behavior-verified: - pty.rs: construct the PtySession INSIDE create_session's blocking task and move the child kill into Drop for PtySession. Every teardown — close_session, service shutdown, creation failure after spawn, and the caller's future being cancelled at the .await (which previously orphaned the tmux client: the VEOF guard stopped the injection but nothing killed the child or unblocked the reader thread) — now funnels through the one Drop. The VeofDisarmOnDrop guard, its dup(2), and the tuple-order/defuse-placement conventions are deleted outright; the disarm stays teardown-only. New test drives a real pty child through session drop (VEOF disarmed on the device, child killed and reaped). - terminal.rs: cols/rows become Option so the stray-newline tripwire warns on true ABSENCE instead of false-positiving on panes that genuinely measure 80x24 (a literal-default regression stays visible in pty.rs's per-attach size log); unit tests for the attach-window tripwire's byte budget and expiry latch. - terminalWsUrl: reject non-finite dims — FitAddon.proposeDimensions() can yield NaN for detached/unstyled containers, and cols=NaN in the URL would 400 and burn the reconnect ladder; vitest added. - XTermInstance: gate measurability on the terminal element's real DOM box (display:none containers pass proposeDimensions' >=1-cell clamp with a garbage 2x1 grid — the tiny-then-resize bounce in new clothes); heal-on-session-switch effect; fitTerminal reuses fitInstance. - TerminalProvider: createTerminalConnection is now ensure-style idempotent — while a live/in-flight connection owns the tab it only refreshes the stored callbacks + endpoint source, fixing the remount staleness the ref indirection alone missed (a remount creates NEW ref cells; the stored closures kept reading the old mount's frozen ones) — and the public hasTerminalConnection gate is deleted in favor of enforcing the invariant where the state lives. Backend-error and clean-close paths now release the tab's entries (if still theirs) so a later mount/session switch can heal instead of finding the slot occupied by a dead socket forever. Already-open sockets from pluggable transports get the size sync onopen would have sent. UNMEASURED_POLL_MS named to keep the wait-state cadence out of the retry ladder. --- crates/local-deployment/src/pty.rs | 336 +++++++++--------- crates/server/src/routes/terminal.rs | 89 +++-- .../src/shared/components/XTermInstance.tsx | 62 ++-- .../web-core/src/shared/hooks/useTerminal.ts | 20 +- .../src/shared/lib/terminalWsUrl.test.ts | 12 + .../web-core/src/shared/lib/terminalWsUrl.ts | 15 +- .../src/shared/providers/TerminalProvider.tsx | 138 ++++--- 7 files changed, 393 insertions(+), 279 deletions(-) diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index ba2fff109e..88d8c63d87 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -912,99 +912,37 @@ pub enum PtyError { /// 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. -/// The termios write shared by `disarm_master_eof` and `VeofDisarmOnDrop`. -/// termios is pty-DEVICE state shared across every dup'd fd, so a disarm -/// through any fd of the pty is seen by the writer's own `tcgetattr`. #[cfg(unix)] -fn disarm_eof_on_fd(fd: std::os::unix::io::RawFd) { +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 { - // Runs inside Drop, so never panic — but a failed set leaves - // the writer's injection path armed, which is exactly what - // the attach-window tripwire would then catch; leave a trace - // so the two can be correlated. 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(unix)] -fn disarm_master_eof(master: &dyn portable_pty::MasterPty) { - let Some(fd) = master.as_raw_fd() else { - return; - }; - disarm_eof_on_fd(fd); -} - #[cfg(not(unix))] fn disarm_master_eof(_master: &dyn portable_pty::MasterPty) {} -/// Disarms the pty's VEOF on drop unless defused. Covers the whole window in -/// `create_session` between `take_writer()` succeeding and the `PtySession` -/// being inserted into the registry (whose own `Drop` takes over the disarm -/// duty from there): an early return (`try_clone_reader` failing), a -/// panic-unwind, or the request future being cancelled at the -/// `spawn_blocking` `.await` (the detached blocking task still completes and -/// the runtime then drops its returned tuple) would each drop the writer -/// with no `PtySession` in existence, and the writer's Drop would inject -/// `\n` + Ctrl-D into the just-spawned child. -/// -/// Owns a `dup(2)` of the master fd instead of borrowing the master so it -/// can travel across the `spawn_blocking` boundary inside the returned tuple -/// — declared FIRST there, because tuple fields drop in declaration order -/// and the disarm must precede the writer's Drop on a cancellation drop. -struct VeofDisarmOnDrop { - #[cfg(unix)] - fd: Option, - /// Set once the writer's ownership is safely inside a registered - /// `PtySession`; the guard then does nothing. - defused: bool, -} - -impl VeofDisarmOnDrop { - fn new(master: &dyn portable_pty::MasterPty) -> Self { - #[cfg(unix)] - { - let fd = master.as_raw_fd().and_then(|raw| { - // SAFETY: `raw` is a live fd owned by `master`, which outlives - // this immediate clone-to-owned. - let borrowed = unsafe { std::os::fd::BorrowedFd::borrow_raw(raw) }; - borrowed.try_clone_to_owned().ok() - }); - Self { fd, defused: false } - } - #[cfg(not(unix))] - { - let _ = master; - Self { defused: false } - } - } - - fn defuse(&mut self) { - self.defused = true; - } -} - -impl Drop for VeofDisarmOnDrop { - fn drop(&mut self) { - if self.defused { - return; - } - #[cfg(unix)] - if let Some(fd) = &self.fd { - use std::os::fd::AsRawFd; - disarm_eof_on_fd(fd.as_raw_fd()); - } - } -} - struct PtySession { /// Per-session writer behind its own lock so a blocking PTY write never /// holds up the global session registry (see `write`). @@ -1035,14 +973,28 @@ impl Drop for PtySession { // `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. This is the teardown point every - // session-owning path funnels through — `close_session` (map `remove` - // + drop), process/service shutdown (the sessions `HashMap` dropping), - // and any future cleanup path — so the disarm can never be forgotten. - // (The pre-session creation window is covered separately by - // `VeofDisarmOnDrop` in `create_session`.) See `disarm_master_eof` - // for why the injection happens at all. + // 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. Skip the signal if the reader already reaped on the + // natural-exit path, so we never signal a freed/recycled PID (the + // pid can't be recycled before `wait()`, which only the reader + // thread calls). + // + // 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(); + } } } @@ -1207,33 +1159,24 @@ 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()))?; - - // From here until the `PtySession` is registered there is a writer - // whose Drop injects `\n` + Ctrl-D but no session Drop to disarm - // it; the guard rides the returned tuple to cover every early - // return, unwind, and cancellation in that window (see its doc). - let veof_guard = VeofDisarmOnDrop::new(pty_pair.master.as_ref()); - - 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 — kill directly (the child stays a + // zombie until process exit, as on any pre-thread failure, + // but the tmux client / shell itself is gone). + let _ = child_killer.kill(); + return Err(PtyError::CreateFailed(e.to_string())); + } + }; let output_handle = thread::spawn(move || { let mut child = child; @@ -1256,46 +1199,50 @@ 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); }); - // Guard first: tuple fields drop in declaration order, so if this - // tuple is dropped un-consumed (request future cancelled at the - // `.await` below), the guard disarms before the writer drops. - Ok::<_, PtyError>(( - veof_guard, - 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, + closed: false, + }) }) .await .map_err(|e| PtyError::CreateFailed(e.to_string()))??; - let (mut veof_guard, 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); - - // The writer is now owned by a registered `PtySession`, whose own Drop - // holds the disarm duty from here. (If the insert above failed, both - // the constructed session's Drop and the still-armed guard disarm — - // idempotent.) - veof_guard.defuse(); + .insert(session_id, result); Ok((session_id, output_rx)) } @@ -1365,27 +1312,14 @@ impl PtyService { } pub async fn close_session(&self, session_id: Uuid) -> Result<(), PtyError> { - if let Some(mut session) = self - .sessions + // Dropping the removed session runs the full teardown — VEOF disarm + // then child kill — in `Drop for PtySession`. (A send-parked reader + // is additionally released when the caller drops the output receiver + // after this returns.) + 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(); - } - // `session` drops at the end of this scope: `Drop for PtySession` - // disarms the master's VEOF first, so the writer's own Drop does not - // inject `\n` + Ctrl-D into the (now detaching) tmux client's tty. - } + .remove(&session_id); Ok(()) } } @@ -1670,28 +1604,84 @@ mod tests { assert_eq!(after, 0, "VEOF must be disarmed after disarm_master_eof"); } - /// The `create_session` creation-window guard: dropped undefused (early - /// return / unwind / cancelled `.await` before the `PtySession` is - /// registered) it must disarm VEOF — via its own dup'd fd, independent of - /// the master; dropped defused (ownership handed to `PtySession`) it must - /// leave the live session's VEOF armed. + /// `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 veof_guard_disarms_only_when_not_defused() { - let pair = open_test_pty(); + fn pty_session_drop_disarms_veof_and_reaps_child() { + use std::os::fd::{AsRawFd as _, BorrowedFd}; - // Defused guard (success path): VEOF stays armed for the live session. - let mut guard = VeofDisarmOnDrop::new(pair.master.as_ref()); - guard.defuse(); - drop(guard); - let veof = master_veof(pair.master.as_ref()).expect("tcgetattr"); - assert_ne!(veof, 0, "a defused guard must not touch VEOF"); - - // Undefused guard (failure/cancellation path): VEOF is disarmed before - // the writer would drop, even with the master untouched. - let guard = VeofDisarmOnDrop::new(pair.master.as_ref()); - drop(guard); - let veof = master_veof(pair.master.as_ref()).expect("tcgetattr"); - assert_eq!(veof, 0, "an undefused guard must disarm VEOF"); + 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, + closed: false, + }; + 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 85d865827c..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")] @@ -191,17 +190,18 @@ 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. An absent-or-default - // 80x24 means the frontend connected while the terminal was - // unmeasured (hidden / 0-height), which makes claude reflow and - // stack blank lines on the follow-up resize. After the frontend fix - // (never connect at an unmeasured size) this must never fire. - if query.cols == default_cols() && query.rows == default_rows() { + // 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 at default {}x{} for workspace {} — \ - pane likely connected while unmeasured (stray-newline regression)", - query.cols, - query.rows, + "CLI terminal attaching without cols/rows for workspace {} — \ + frontend connected unmeasured (stray-newline regression)", query.workspace_id ); } @@ -365,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, ) @@ -571,7 +571,48 @@ pub(super) fn router() -> Router { #[cfg(test)] mod tripwire_tests { - use super::hex_dump; + 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() { diff --git a/packages/web-core/src/shared/components/XTermInstance.tsx b/packages/web-core/src/shared/components/XTermInstance.tsx index 96ac1622d4..3a681ce5ea 100644 --- a/packages/web-core/src/shared/components/XTermInstance.tsx +++ b/packages/web-core/src/shared/components/XTermInstance.tsx @@ -66,7 +66,6 @@ export function XTermInstance({ getTerminalInstance, createTerminalConnection, getTerminalConnection, - hasTerminalConnection, } = useTerminal(); // Latest sessionId / onClose for the connection callbacks. The provider @@ -97,15 +96,30 @@ export function XTermInstance({ }, [tabId, getTerminalInstance]); // Re-fit and resolve the WS endpoint at the terminal's CURRENT grid. Returns - // null when the pane is unmeasurable (hidden / 0-height): FitAddon.fit() then - // silently no-ops and proposeDimensions() is undefined, so connecting would - // bake xterm's 80x24 constructor default into the URL and make claude 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. + // 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 the terminal element's actual DOM box, 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. display:none and detached containers + // both read as 0x0 here. const getEndpoint = useCallback((): string | null => { const instance = fitInstance(); if (!instance) return null; + const element = instance.terminal.element; + if ( + !element || + !element.isConnected || + element.offsetWidth === 0 || + element.offsetHeight === 0 + ) { + return null; + } return resolveTerminalEndpoint( { workspaceId, @@ -126,14 +140,13 @@ export function XTermInstance({ // 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(() => { - // A live connection OR an in-flight/pending one counts as "already - // connected": the provider registers the socket only after its async open - // resolves, and stacking a second generation into that window would churn - // a duplicate backend PTY/tmux attach (slow relay transports make the - // window real). - if (hasTerminalConnection(tabId)) return; - // Gate on measurability up front so no connection object (and no server-side - // PTY/tmux churn) is created for an invisible pane. + // Gate on measurability up front: no connection (and none of the client + // generation/retry machinery) is set up for a pane that has never been + // visible. createTerminalConnection itself is idempotent while a live or + // in-flight connection owns the tab, so calling this repeatedly — mount, + // ResizeObserver ticks, session switches — never stacks a duplicate + // backend PTY/tmux attach; while occupied it only refreshes the stored + // callbacks to this mount's closures. if (getEndpoint() === null) return; createTerminalConnection( tabId, @@ -156,23 +169,21 @@ export function XTermInstance({ getEndpoint, fitInstance, getTerminalInstance, - hasTerminalConnection, createTerminalConnection, ]); const fitTerminal = useCallback(() => { - fitAddonRef.current?.fit(); - const terminal = terminalRef.current; - if (!terminal) return; + const instance = fitInstance(); + if (!instance) return; const conn = getTerminalConnection(tabId); if (conn) { - conn.resize(terminal.cols, terminal.rows); + 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, ensureConnection]); + }, [tabId, fitInstance, getTerminalConnection, ensureConnection]); // Terminal + connection lifecycle. Every run of this effect MUST register // the same cleanup: an early return without one leaves `terminalRef` @@ -369,6 +380,15 @@ export function XTermInstance({ 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). + useEffect(() => { + 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 c480440e1f..f9ffa3a52a 100644 --- a/packages/web-core/src/shared/hooks/useTerminal.ts +++ b/packages/web-core/src/shared/hooks/useTerminal.ts @@ -48,12 +48,20 @@ 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, // 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 reschedules instead of - // attaching at a wrong/placeholder size. + // 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, @@ -63,14 +71,6 @@ export interface TerminalContextType { resize: (cols: number, rows: number) => void; }; getTerminalConnection: (tabId: string) => TerminalConnection | null; - /** - * True when the tab has an established connection OR a live in-flight one - * (generation created, socket not registered yet — the async open hasn't - * resolved). Connect-or-defer callers must gate on this, not on - * getTerminalConnection, or two quick calls stack duplicate backend - * PTY/tmux attaches inside the open window. - */ - hasTerminalConnection: (tabId: string) => boolean; } export const TerminalContext = createHmrContext( diff --git a/packages/web-core/src/shared/lib/terminalWsUrl.test.ts b/packages/web-core/src/shared/lib/terminalWsUrl.test.ts index 785d377309..8f06343a7d 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.test.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.test.ts @@ -75,6 +75,18 @@ describe('resolveTerminalEndpoint', () => { 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(); diff --git a/packages/web-core/src/shared/lib/terminalWsUrl.ts b/packages/web-core/src/shared/lib/terminalWsUrl.ts index 89ccbae747..648e192f14 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.ts @@ -62,6 +62,19 @@ export function resolveTerminalEndpoint( params: Omit, dims: { cols: number; rows: number } | undefined | null ): string | null { - if (!dims || dims.cols <= 0 || dims.rows <= 0) return 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 }); } diff --git a/packages/web-core/src/shared/providers/TerminalProvider.tsx b/packages/web-core/src/shared/providers/TerminalProvider.tsx index 15cdc0fc29..91a8a583c2 100644 --- a/packages/web-core/src/shared/providers/TerminalProvider.tsx +++ b/packages/web-core/src/shared/providers/TerminalProvider.tsx @@ -14,11 +14,19 @@ 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 { /** * Called fresh on every (re)connect attempt; null = pane unmeasurable, * defer. Full contract on `TerminalContextType.createTerminalConnection` - * and `resolveTerminalEndpoint`. + * and `resolveTerminalEndpoint`. Refreshed by `createTerminalConnection` + * when a remounted component re-registers while this generation is live. */ getEndpoint: () => string | null; retryCount: number; @@ -308,6 +316,21 @@ 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, @@ -316,20 +339,35 @@ export function TerminalProvider({ children }: TerminalProviderProps) { 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; } + return makeFacade(tabId); } - const existing = terminalConnectionsRef.current.get(tabId); - if (existing) { - existing.ws.close(); - terminalConnectionsRef.current.delete(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 @@ -398,7 +436,7 @@ export function TerminalProvider({ children }: TerminalProviderProps) { // user ever shows it again. const endpoint = generation.getEndpoint(); if (endpoint === null) { - scheduleAttempt(1000); + scheduleAttempt(UNMEASURED_POLL_MS); return; } @@ -415,14 +453,28 @@ export function TerminalProvider({ children }: TerminalProviderProps) { return; } - ws.onopen = () => { - // Reset retry count on successful connection + // 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 = () => { 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?.(); @@ -436,6 +488,12 @@ 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(); + } ws.onmessage = (event) => { try { @@ -448,8 +506,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` ); @@ -468,8 +528,10 @@ export function TerminalProvider({ children }: TerminalProviderProps) { 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; } @@ -500,20 +562,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( @@ -523,17 +574,6 @@ export function TerminalProvider({ children }: TerminalProviderProps) { [] ); - const hasTerminalConnection = useCallback((tabId: string): boolean => { - if (terminalConnectionsRef.current.has(tabId)) return true; - // A live generation with no registered socket yet is an in-flight open or - // a scheduled retry — callers must treat it as occupied so they never - // stack a second connect (and a duplicate backend PTY/tmux attach) on top - // of it. Cancelled generations are flagged closed / pruned and don't - // count. - const generation = reconnectStateRef.current.get(tabId); - return !!generation && !generation.closed; - }, []); - const value = useMemo( () => ({ getTabsForWorkspace, @@ -548,7 +588,6 @@ export function TerminalProvider({ children }: TerminalProviderProps) { unregisterTerminalInstance, createTerminalConnection, getTerminalConnection, - hasTerminalConnection, }), [ getTabsForWorkspace, @@ -563,7 +602,6 @@ export function TerminalProvider({ children }: TerminalProviderProps) { unregisterTerminalInstance, createTerminalConnection, getTerminalConnection, - hasTerminalConnection, ] ); From 78b1787cb8f2f81ca3d6eaaa237457e6985a5684 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 21:34:28 +0000 Subject: [PATCH 07/11] fix(terminal): discard stale in-flight opens on session switch; reap on reader-clone failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Council round 2 (mixed engine, 4x codex + claude cross-check): - TerminalProvider: an open started before a remount/session switch could resolve AFTER the endpoint source was refreshed and still register — binding the visible pane to the PREVIOUS session's tmux (all four codex seats, P1). Post-open the attempt is now validated against the current getEndpoint(): size-only drift passes (the post-open resize corrects it), a material change (session/mode/workspace) closes the socket and re-kicks through the single-timer path without spending retry budget. terminalEndpointsEquivalent lives in terminalWsUrl.ts with vitest. - TerminalProvider/XTermInstance: the measurability gate moved from the component into createTerminalConnection, so an occupied-but-hidden tab still refreshes its stored callbacks on remount (the component-side gate skipped the refresh exactly when the predecessor closures were already dead). - XTermInstance: the DOM gate also checks the PARENT box — the one FitAddon actually measures — so a splitter collapsed to 0-height can't slip a ~Nx1 clamped grid past the element-box check. - pty.rs: try_clone_reader failure now kills AND reaps the spawned child inline (was: kill only, one zombie per failed create); the dead PtySession.closed field is gone; close_session drops the removed session outside the registry lock; the Drop comment states the real (accepted, pre-existing, few-instruction) load-then-kill window instead of overclaiming. --- crates/local-deployment/src/pty.rs | 46 ++++++++---------- .../src/shared/components/XTermInstance.tsx | 36 ++++++++------ .../src/shared/lib/terminalWsUrl.test.ts | 48 ++++++++++++++++++- .../web-core/src/shared/lib/terminalWsUrl.ts | 28 +++++++++++ .../src/shared/providers/TerminalProvider.tsx | 31 ++++++++++++ 5 files changed, 148 insertions(+), 41 deletions(-) diff --git a/crates/local-deployment/src/pty.rs b/crates/local-deployment/src/pty.rs index 88d8c63d87..6eda563d69 100644 --- a/crates/local-deployment/src/pty.rs +++ b/crates/local-deployment/src/pty.rs @@ -960,11 +960,10 @@ 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 { @@ -981,10 +980,12 @@ impl Drop for PtySession { // 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. Skip the signal if the reader already reaped on the - // natural-exit path, so we never signal a freed/recycled PID (the - // pid can't be recycled before `wait()`, which only the reader - // thread calls). + // 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 @@ -1170,10 +1171,12 @@ impl PtyService { let mut reader = match pty_pair.master.try_clone_reader() { Ok(reader) => reader, Err(e) => { - // No reaper thread yet — kill directly (the child stays a - // zombie until process exit, as on any pre-thread failure, - // but the tmux client / shell itself is gone). - let _ = child_killer.kill(); + // 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())); } }; @@ -1233,7 +1236,6 @@ impl PtyService { child_killer, child_reaped, _output_handle: output_handle, - closed: false, }) }) .await @@ -1263,10 +1265,6 @@ impl PtyService { .get(&session_id) .ok_or(PtyError::SessionNotFound(session_id))?; - if session.closed { - return Err(PtyError::SessionClosed); - } - session.writer.clone() }; @@ -1294,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 { @@ -1313,13 +1307,16 @@ impl PtyService { pub async fn close_session(&self, session_id: Uuid) -> Result<(), PtyError> { // Dropping the removed session runs the full teardown — VEOF disarm - // then child kill — in `Drop for PtySession`. (A send-parked reader - // is additionally released when the caller drops the output receiver - // after this returns.) - self.sessions + // 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); + drop(session); Ok(()) } } @@ -1655,7 +1652,6 @@ mod tests { child_killer, child_reaped: child_reaped.clone(), _output_handle: output_handle, - closed: false, }; drop(session); diff --git a/packages/web-core/src/shared/components/XTermInstance.tsx b/packages/web-core/src/shared/components/XTermInstance.tsx index 3a681ce5ea..b253d6f75f 100644 --- a/packages/web-core/src/shared/components/XTermInstance.tsx +++ b/packages/web-core/src/shared/components/XTermInstance.tsx @@ -102,21 +102,27 @@ export function XTermInstance({ // 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 the terminal element's actual DOM box, 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. display:none and detached containers - // both read as 0x0 here. + // 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 + element.offsetHeight === 0 || + parent.offsetWidth === 0 || + parent.offsetHeight === 0 ) { return null; } @@ -140,14 +146,14 @@ export function XTermInstance({ // 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(() => { - // Gate on measurability up front: no connection (and none of the client - // generation/retry machinery) is set up for a pane that has never been - // visible. createTerminalConnection itself is idempotent while a live or - // in-flight connection owns the tab, so calling this repeatedly — mount, - // ResizeObserver ticks, session switches — never stacks a duplicate - // backend PTY/tmux attach; while occupied it only refreshes the stored - // callbacks to this mount's closures. - if (getEndpoint() === null) return; + // 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, diff --git a/packages/web-core/src/shared/lib/terminalWsUrl.test.ts b/packages/web-core/src/shared/lib/terminalWsUrl.test.ts index 8f06343a7d..3b4e4b201a 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.test.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.test.ts @@ -1,6 +1,10 @@ import { describe, it, expect } from 'vitest'; -import { buildTerminalWsUrl, resolveTerminalEndpoint } from './terminalWsUrl'; +import { + buildTerminalWsUrl, + resolveTerminalEndpoint, + terminalEndpointsEquivalent, +} from './terminalWsUrl'; describe('buildTerminalWsUrl', () => { it('carries the provided grid size instead of the 80x24 default', () => { @@ -114,3 +118,45 @@ describe('resolveTerminalEndpoint', () => { 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); + }); +}); diff --git a/packages/web-core/src/shared/lib/terminalWsUrl.ts b/packages/web-core/src/shared/lib/terminalWsUrl.ts index 648e192f14..ce8cb080b7 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.ts @@ -78,3 +78,31 @@ export function resolveTerminalEndpoint( } 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; + } +} diff --git a/packages/web-core/src/shared/providers/TerminalProvider.tsx b/packages/web-core/src/shared/providers/TerminalProvider.tsx index 91a8a583c2..298f178871 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 { terminalEndpointsEquivalent } from '@/shared/lib/terminalWsUrl'; interface TerminalConnection { ws: WebSocket; @@ -359,6 +360,15 @@ export function TerminalProvider({ children }: TerminalProviderProps) { return makeFacade(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 @@ -453,6 +463,27 @@ export function TerminalProvider({ children }: TerminalProviderProps) { return; } + // The endpoint source may have been refreshed while the open was + // in flight (remount / session switch): registering this socket + // would bind the visible pane to the PREVIOUS session's tmux. + // Size-only drift is fine (the post-open resize corrects it) — + // only a material change (session/mode/workspace) discards the + // attempt. Not a failure: re-kick through the single-timer path + // without spending the retry budget. A null current endpoint + // (pane went hidden mid-open) keeps the socket — hidden panes + // keep connections by design. + const currentEndpoint = generation.getEndpoint(); + if ( + currentEndpoint !== null && + !terminalEndpointsEquivalent(endpoint, currentEndpoint) + ) { + ws.close(); + if (generation.retryTimer === null) { + scheduleAttempt(0); + } + 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 From 70a25237bd45fd7109aec4f6c64c90a721568033 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 21:41:40 +0000 Subject: [PATCH 08/11] fix(terminal): validate hidden-at-resolve in-flight opens via endpoint epoch Final backstop sweep: the stale-open guard skipped validation when the pane was unmeasurable at resolve time (getEndpoint() null), so a session switch racing a slow open could still register the previous session's socket if the pane hid mid-open. Every getEndpoint refresh now bumps an epoch the attempt snapshots before awaiting: unchanged epoch = untouched source, safe to register even hidden; changed epoch = revalidate (size drift passes, material change discards, unmeasurable discards conservatively and re-polls). Discards never spend the retry budget. --- .../src/shared/providers/TerminalProvider.tsx | 52 +++++++++++++------ 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/packages/web-core/src/shared/providers/TerminalProvider.tsx b/packages/web-core/src/shared/providers/TerminalProvider.tsx index 298f178871..9f8967288f 100644 --- a/packages/web-core/src/shared/providers/TerminalProvider.tsx +++ b/packages/web-core/src/shared/providers/TerminalProvider.tsx @@ -30,6 +30,17 @@ interface ConnectionGeneration { * 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). */ @@ -356,6 +367,7 @@ export function TerminalProvider({ children }: TerminalProviderProps) { connectionCallbacksRef.current.set(tabId, { onData, onExit, getSize }); if (liveGeneration && !liveGeneration.closed) { liveGeneration.getEndpoint = getEndpoint; + liveGeneration.endpointEpoch += 1; } return makeFacade(tabId); } @@ -385,6 +397,7 @@ export function TerminalProvider({ children }: TerminalProviderProps) { const generation: ConnectionGeneration = { getEndpoint, + endpointEpoch: 0, retryCount: 0, retryTimer: null, closed: false, @@ -449,6 +462,7 @@ export function TerminalProvider({ children }: TerminalProviderProps) { scheduleAttempt(UNMEASURED_POLL_MS); return; } + const attemptEpoch = generation.endpointEpoch; void (async () => { try { @@ -466,22 +480,30 @@ export function TerminalProvider({ children }: TerminalProviderProps) { // The endpoint source may have been refreshed while the open was // in flight (remount / session switch): registering this socket // would bind the visible pane to the PREVIOUS session's tmux. - // Size-only drift is fine (the post-open resize corrects it) — - // only a material change (session/mode/workspace) discards the - // attempt. Not a failure: re-kick through the single-timer path - // without spending the retry budget. A null current endpoint - // (pane went hidden mid-open) keeps the socket — hidden panes - // keep connections by design. - const currentEndpoint = generation.getEndpoint(); - if ( - currentEndpoint !== null && - !terminalEndpointsEquivalent(endpoint, currentEndpoint) - ) { - ws.close(); - if (generation.retryTimer === null) { - scheduleAttempt(0); + // Unchanged epoch = untouched source, safe (even if the pane hid + // mid-open: identity can't change without a refresh, every + // refresh bumps the epoch). Changed epoch: size-only drift still + // passes (the post-open resize corrects it), a material change + // (session/mode/workspace) discards — and an unmeasurable + // current endpoint discards CONSERVATIVELY, because the refresh + // may have changed the session and there is no URL to prove + // otherwise; the poll re-opens with the right one when shown. + // Neither discard is a failure: re-kick through the single-timer + // path without spending the retry budget. + if (generation.endpointEpoch !== attemptEpoch) { + const currentEndpoint = generation.getEndpoint(); + if ( + currentEndpoint === null || + !terminalEndpointsEquivalent(endpoint, currentEndpoint) + ) { + ws.close(); + if (generation.retryTimer === null) { + scheduleAttempt( + currentEndpoint === null ? UNMEASURED_POLL_MS : 0 + ); + } + return; } - return; } // End of THIS connection's life without a successor: cancel the From 8845db3c744df3d3a1b3f77eb06cf966b76ad42e Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 21:48:03 +0000 Subject: [PATCH 09/11] fix(terminal): sync session refs and heal-refresh in the commit, not after paint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Backstop round 2: a websocket open resolving in the commit-to-passive- effect gap could still read the previous session's ref value and an unbumped endpointEpoch, registering the old session's socket. The ref sync and the heal/refresh effect are now useLayoutEffect — they run synchronously inside the commit, before any promise continuation can interleave, so an in-flight open can never observe a half-switched session. --- .../src/shared/components/XTermInstance.tsx | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/packages/web-core/src/shared/components/XTermInstance.tsx b/packages/web-core/src/shared/components/XTermInstance.tsx index b253d6f75f..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'; @@ -79,7 +85,11 @@ export function XTermInstance({ // otherwise churn the ResizeObserver effect on every parent render). const sessionIdRef = useRef(sessionId); const onCloseRef = useRef(onClose); - useEffect(() => { + // 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; }); @@ -390,8 +400,14 @@ export function XTermInstance({ // 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). - useEffect(() => { + // 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]); From e8223aa1abd05241e390fc5d0b9e854e948790a1 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 22:47:33 +0000 Subject: [PATCH 10/11] fix(terminal): re-validate endpoint epoch at ws onopen to reject stale CONNECTING attach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The endpoint-epoch check ran only right after openLocalApiWebSocket() resolved, but the default transport resolves with a still-CONNECTING socket. A session switch landing after that socket was registered but before it opened only bumped the epoch (occupied path) without closing or revalidating the connecting socket, so it could become the tab's live connection and bind the pane to the previous session's conversation on a first-attach / post-reap create. Re-validate at onopen (and make the socket's own message/close handlers inert once superseded) via a shared, unit-tested terminalAttemptIsStale() helper that also DRYs the existing post-await check; discard + re-kick through the single timer without spending the retry budget. Never tears down a live/hidden connection — it rejects only a not-yet-live stale handshake. --- .../src/shared/lib/terminalWsUrl.test.ts | 52 +++++++++++ .../web-core/src/shared/lib/terminalWsUrl.ts | 34 +++++++ .../src/shared/providers/TerminalProvider.tsx | 88 +++++++++++++------ 3 files changed, 149 insertions(+), 25 deletions(-) diff --git a/packages/web-core/src/shared/lib/terminalWsUrl.test.ts b/packages/web-core/src/shared/lib/terminalWsUrl.test.ts index 3b4e4b201a..03be817d61 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.test.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.test.ts @@ -4,6 +4,7 @@ import { buildTerminalWsUrl, resolveTerminalEndpoint, terminalEndpointsEquivalent, + terminalAttemptIsStale, } from './terminalWsUrl'; describe('buildTerminalWsUrl', () => { @@ -160,3 +161,54 @@ describe('terminalEndpointsEquivalent', () => { ).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); + }); +}); diff --git a/packages/web-core/src/shared/lib/terminalWsUrl.ts b/packages/web-core/src/shared/lib/terminalWsUrl.ts index ce8cb080b7..134ee72dd2 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.ts @@ -106,3 +106,37 @@ export function terminalEndpointsEquivalent(a: string, b: string): boolean { 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. + */ +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 9f8967288f..8b2cd36992 100644 --- a/packages/web-core/src/shared/providers/TerminalProvider.tsx +++ b/packages/web-core/src/shared/providers/TerminalProvider.tsx @@ -7,7 +7,7 @@ import { type TerminalInstance, } from '@/shared/hooks/useTerminal'; import { openLocalApiWebSocket } from '@/shared/lib/localApiTransport'; -import { terminalEndpointsEquivalent } from '@/shared/lib/terminalWsUrl'; +import { terminalAttemptIsStale } from '@/shared/lib/terminalWsUrl'; interface TerminalConnection { ws: WebSocket; @@ -477,33 +477,52 @@ export function TerminalProvider({ children }: TerminalProviderProps) { return; } - // The endpoint source may have been refreshed while the open was - // in flight (remount / session switch): registering this socket - // would bind the visible pane to the PREVIOUS session's tmux. - // Unchanged epoch = untouched source, safe (even if the pane hid - // mid-open: identity can't change without a refresh, every - // refresh bumps the epoch). Changed epoch: size-only drift still - // passes (the post-open resize corrects it), a material change - // (session/mode/workspace) discards — and an unmeasurable - // current endpoint discards CONSERVATIVELY, because the refresh - // may have changed the session and there is no URL to prove - // otherwise; the poll re-opens with the right one when shown. - // Neither discard is a failure: re-kick through the single-timer - // path without spending the retry budget. - if (generation.endpointEpoch !== attemptEpoch) { + // 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. Unchanged + // epoch = untouched source, always valid; changed epoch discards on + // a material (session/mode/workspace) change or a now-unmeasurable + // pane, and keeps size-only drift (the post-open resize corrects + // it) — see `terminalAttemptIsStale`. Discarding is not a failure: + // re-kick through the single-timer path without spending the retry + // budget (null endpoint = pane unmeasurable now → poll). + const discardIfStale = (): boolean => { const currentEndpoint = generation.getEndpoint(); if ( - currentEndpoint === null || - !terminalEndpointsEquivalent(endpoint, currentEndpoint) + !terminalAttemptIsStale( + generation.endpointEpoch !== attemptEpoch, + endpoint, + currentEndpoint + ) ) { - ws.close(); - if (generation.retryTimer === null) { - scheduleAttempt( - currentEndpoint === null ? UNMEASURED_POLL_MS : 0 - ); - } - return; + 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()) { + return; } // End of THIS connection's life without a successor: cancel the @@ -527,6 +546,13 @@ export function TerminalProvider({ children }: TerminalProviderProps) { // stuck at the URL size forever; resending on every (re)connect // also restores the right size after a reattach. const syncSize = () => { + // A session switch can land after this socket was registered but + // before it opened (the transport returns a CONNECTING socket): + // re-validate before it becomes live, closing the DUP handshake, + // not a hidden live connection. + if (superseded || discardIfStale()) { + return; + } generation.retryCount = 0; const size = connectionCallbacksRef.current .get(tabId) @@ -546,9 +572,19 @@ export function TerminalProvider({ children }: TerminalProviderProps) { // 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); @@ -577,7 +613,9 @@ 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; } From 783a254338f66d1519160f9421174471357c7f66 Mon Sep 17 00:00:00 2001 From: Miguel Rasero Date: Wed, 8 Jul 2026 22:59:33 +0000 Subject: [PATCH 11/11] fix(terminal): force endpoint re-inspection at ws onopen to close gated-child stale bind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The onopen re-validation gated on the epoch delta, but the epoch is bumped only by a MOUNTED terminal child. When the host stops rendering the terminal (executorRunning / !sessionsReady) during the CONNECTING window and the session changes while gated, no mounted effect bumps the epoch, so the epoch delta stays false and the stale socket (carrying the previous session_id) was accepted at onopen — binding the pane to the previous conversation on a first-attach / post-reap create. Pass epochChanged=true at the onopen site (the point of no return) so the endpoint is ALWAYS re-inspected; a gated pane's endpoint resolves to null, so the stale socket is discarded and the poll reconnects with the current session once the pane is shown again. The post-await site keeps the epoch-delta gate to avoid churn when a pane is briefly unmeasurable without a switch. Never tears down a live/hidden connection. --- .../src/shared/lib/terminalWsUrl.test.ts | 14 ++++++ .../web-core/src/shared/lib/terminalWsUrl.ts | 7 +++ .../src/shared/providers/TerminalProvider.tsx | 45 +++++++++++-------- 3 files changed, 47 insertions(+), 19 deletions(-) diff --git a/packages/web-core/src/shared/lib/terminalWsUrl.test.ts b/packages/web-core/src/shared/lib/terminalWsUrl.test.ts index 03be817d61..f4cbeb4cec 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.test.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.test.ts @@ -211,4 +211,18 @@ describe('terminalAttemptIsStale', () => { // 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 134ee72dd2..c0f5e8a88d 100644 --- a/packages/web-core/src/shared/lib/terminalWsUrl.ts +++ b/packages/web-core/src/shared/lib/terminalWsUrl.ts @@ -128,6 +128,13 @@ export function terminalEndpointsEquivalent(a: string, b: string): boolean { * 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, diff --git a/packages/web-core/src/shared/providers/TerminalProvider.tsx b/packages/web-core/src/shared/providers/TerminalProvider.tsx index 8b2cd36992..a8acb0882f 100644 --- a/packages/web-core/src/shared/providers/TerminalProvider.tsx +++ b/packages/web-core/src/shared/providers/TerminalProvider.tsx @@ -490,21 +490,27 @@ export function TerminalProvider({ children }: TerminalProviderProps) { // 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. Unchanged - // epoch = untouched source, always valid; changed epoch discards on - // a material (session/mode/workspace) change or a now-unmeasurable - // pane, and keeps size-only drift (the post-open resize corrects - // it) — see `terminalAttemptIsStale`. Discarding is not a failure: - // re-kick through the single-timer path without spending the retry - // budget (null endpoint = pane unmeasurable now → poll). - const discardIfStale = (): boolean => { + // 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( - generation.endpointEpoch !== attemptEpoch, - endpoint, - currentEndpoint - ) + !terminalAttemptIsStale(epochChanged, endpoint, currentEndpoint) ) { return false; } @@ -521,7 +527,7 @@ export function TerminalProvider({ children }: TerminalProviderProps) { return true; }; - if (discardIfStale()) { + if (discardIfStale(generation.endpointEpoch !== attemptEpoch)) { return; } @@ -546,11 +552,12 @@ export function TerminalProvider({ children }: TerminalProviderProps) { // stuck at the URL size forever; resending on every (re)connect // also restores the right size after a reattach. const syncSize = () => { - // A session switch can land after this socket was registered but - // before it opened (the transport returns a CONNECTING socket): - // re-validate before it becomes live, closing the DUP handshake, - // not a hidden live connection. - if (superseded || discardIfStale()) { + // 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;