From cc05b1ff76557fe37c9e51c88af07ee5047c3775 Mon Sep 17 00:00:00 2001 From: Alejandro Blanco-M Date: Fri, 9 Oct 2026 19:14:27 +0200 Subject: [PATCH] fix(tui): restore kitty keyboard protocol and TUI modes on signal exit The SIGINT/SIGTERM/SIGHUP/SIGQUIT handler restored raw mode, the primary screen and the cursor but never popped the kitty keyboard stack nor disabled bracketed paste, focus reporting or mouse capture, so a signal exit left the shell echoing raw sequences like e1;1:3u. The cleanup now disables every TUI mode and pops the kitty stack, writing the disable sequences to stdout where setup_terminal armed them, and skips the pop when this process owns no stack entry. Ownership is tracked process-wide, set on enable, adopted when an exec handoff inherits the previous process's entry, and the push, ownership flip and signal cleanup serialize under one mutex so termination waits out an in-flight push/pop instead of exiting with the entry active. While an editor owns the tty the cleanup leaves terminal modes alone, and the editor handoff publishes only after suspend finished and reclaims before resume re-arms, both under the kitty state lock, so the cleanup never sees a half-applied handoff. Terminal-mode tests pin the skip, single-pop and no-test-bytes-on-stdout semantics, and the editor test child waits on a sentinel the sampler writes once it observes ownership instead of a fixed sleep. Closes #1635. Related #898. --- crates/jcode-tui/src/tui/app/commands.rs | 184 ++++++++++++++ crates/jcode-tui/src/tui/mod.rs | 306 ++++++++++++++++++++++- scripts/code_size_budget.json | 4 +- src/cli/terminal.rs | 146 ++++++++++- 4 files changed, 627 insertions(+), 13 deletions(-) diff --git a/crates/jcode-tui/src/tui/app/commands.rs b/crates/jcode-tui/src/tui/app/commands.rs index 07f7b87ee4..37fd451bc3 100644 --- a/crates/jcode-tui/src/tui/app/commands.rs +++ b/crates/jcode-tui/src/tui/app/commands.rs @@ -3196,7 +3196,29 @@ fn run_interactive_editor_with( resume: impl FnOnce(), ) -> std::io::Result { suspend(); + // Publish only after the restores completed. The flag must never be + // observable while jcode's own modes are still armed: a termination + // signal firing mid-suspend would skip its cleanup and leave the shell in + // raw mode (greptile #4230626960). While the disable writes are in flight + // the flag is still false, so a signal in that window restores everything; + // the flag is then published under the same kitty state lock the signal + // cleanup takes, so cleanup either sees the completed handoff or runs in + // full. + crate::tui::publish_editor_terminal_handoff(); let result = command.status(); + // Reclaim before resume re-arms our modes: while the flag is still set a + // termination signal skips its cleanup, which must only happen while the + // editor really is the one controlling the tty. + crate::tui::reclaim_editor_terminal_handoff(); + // A termination signal that skipped its cleanup while the editor owned + // the terminal recorded termination under the kitty state lock before + // this reclaim ran, and the process is exiting. The reclaim leaves + // ownership set in that case; re-arming modes now would leave them + // enabled on the shell after death because no further cleanup runs + // (greptile #4232838288), so skip the resume entirely. + if crate::tui::editor_owns_terminal() { + return result; + } resume(); result } @@ -3204,6 +3226,10 @@ fn run_interactive_editor_with( fn suspend_terminal_for_editor() { use crossterm::event::{DisableBracketedPaste, DisableFocusChange, DisableMouseCapture}; + // The caller publishes the handoff only after this function returns, so a + // termination signal firing between these restores and the publication + // runs a full cleanup instead of skipping one (greptile #4230626960). + // // These commands are safe even when a mode was not enabled. Disable them // before leaving the alternate screen so the child receives normal input. let _ = crossterm::execute!( @@ -3219,6 +3245,15 @@ fn suspend_terminal_for_editor() { fn resume_terminal_after_editor() { use crossterm::event::{EnableBracketedPaste, EnableFocusChange, EnableMouseCapture}; + // Defense in depth for the run_interactive_editor_with early return: a + // termination signal already recorded that the process is exiting, so + // enabling modes here would leave them active on the shell after death + // (greptile #4232838288). + if crate::tui::editor_owns_terminal() { + return; + } + // The caller reclaimed the handoff before entering here, so a termination + // signal arriving while these modes are re-armed runs the full cleanup. // Re-enter the TUI before returning to the event loop. The existing // ratatui Terminal remains usable and the next loop iteration redraws it. let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(ratatui::init)); @@ -3240,9 +3275,15 @@ mod interactive_editor_tests { use super::run_interactive_editor_with; use std::cell::RefCell; use std::rc::Rc; + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; #[test] fn handoff_waits_for_editor_before_resuming_terminal() { + let _guard = crate::tui::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + crate::tui::set_editor_terminal_ownership(false); let events = Rc::new(RefCell::new(Vec::new())); let child_events = Rc::clone(&events); let mut command = std::process::Command::new("sh"); @@ -3271,6 +3312,10 @@ mod interactive_editor_tests { #[test] fn handoff_resumes_terminal_after_editor_failure_status() { + let _guard = crate::tui::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + crate::tui::set_editor_terminal_ownership(false); let resumed = Rc::new(RefCell::new(false)); let resumed_after = Rc::clone(&resumed); let mut command = std::process::Command::new("sh"); @@ -3287,6 +3332,145 @@ mod interactive_editor_tests { assert_eq!(status.code(), Some(7)); assert!(*resumed.borrow()); + assert!(!crate::tui::editor_owns_terminal()); + } + + #[test] + fn handoff_clears_editor_terminal_ownership_when_command_fails_to_launch() { + let _guard = crate::tui::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + crate::tui::set_editor_terminal_ownership(false); + let mut command = std::process::Command::new("/definitely/missing/jcode-editor-test"); + + let result = run_interactive_editor_with(&mut command, || {}, || {}); + + assert!(result.is_err()); + assert!(!crate::tui::editor_owns_terminal()); + } + + #[test] + fn handoff_publishes_ownership_only_while_editor_runs() { + let _guard = crate::tui::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + crate::tui::set_editor_terminal_ownership(false); + let saw_ownership = Arc::new(AtomicBool::new(false)); + let owns_during_resume = Rc::new(RefCell::new(true)); + + // The child stays alive until the sampler confirms the flag, so a busy + // runner delaying the sampler cannot shrink the observation window + // below what the sampler needs (greptile #4231010584). With a fixed + // child sleep the test failed on correct code whenever the sampler + // started late. The child gives up after ~5s so broken code that never + // publishes still fails the assertions instead of hanging. + let sentinel = std::env::temp_dir().join(format!( + "jcode-editor-ownership-{}-{}.sentinel", + std::process::id(), + line!() + )); + let _ = std::fs::remove_file(&sentinel); + + let sampler_sentinel = Arc::clone(&saw_ownership); + let sampler_saw = sentinel.clone(); + let sampler = std::thread::spawn(move || { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while std::time::Instant::now() < deadline { + if crate::tui::editor_owns_terminal() { + sampler_sentinel.store(true, Ordering::SeqCst); + let _ = std::fs::write(&sampler_saw, b""); + return; + } + std::thread::sleep(std::time::Duration::from_millis(1)); + } + }); + + let mut command = std::process::Command::new("sh"); + command + .env("JCODE_OWNERSHIP_SENTINEL", &sentinel) + .args([ + "-c", + "i=0; while [ ! -f \"$JCODE_OWNERSHIP_SENTINEL\" ] && [ \"$i\" -lt 500 ]; do sleep 0.01; i=$((i+1)); done; [ -f \"$JCODE_OWNERSHIP_SENTINEL\" ]", + ]); + let owns_during_resume_after = Rc::clone(&owns_during_resume); + let status = run_interactive_editor_with( + &mut command, + // The restores are still in flight here: the flag must stay false + // so a termination signal in this window cleans up in full instead + // of skipping it and leaving the shell in raw mode (greptile + // #4230626960). + || assert!(!crate::tui::editor_owns_terminal()), + move || *owns_during_resume_after.borrow_mut() = crate::tui::editor_owns_terminal(), + ) + .expect("shell should launch"); + + sampler.join().expect("sampler thread"); + let _ = std::fs::remove_file(&sentinel); + + // Exit 0 means the sampler confirmed ownership while the child was + // still alive; a nonzero exit means the child gave up waiting, which + // only happens when the flag was never published. + assert_eq!( + status.code(), + Some(0), + "editor exited before the sampler observed ownership" + ); + assert!( + saw_ownership.load(Ordering::SeqCst), + "ownership was never published while the editor ran" + ); + // Reclaimed before resume re-armed jcode's modes. + assert!(!*owns_during_resume.borrow()); + assert!(!crate::tui::editor_owns_terminal()); + } + + #[test] + fn reclaim_resume_does_not_reenable_modes_after_termination_started() { + let _guard = crate::tui::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + crate::tui::set_editor_terminal_ownership(false); + + // Greptile #4232838288: reproduce the race window. The signal cleanup + // saw the editor handoff, skipped its restore, recorded termination + // under the kitty state lock, and released the lock while the process + // was still alive. The editor then exited before the process did, so + // this reclaim runs inside that gap and must not let the resume + // re-arm modes that no later cleanup would ever disable again. + crate::tui::publish_editor_terminal_handoff(); + crate::tui::record_termination_started(); + + let resumed = Rc::new(RefCell::new(false)); + let mut command = std::process::Command::new("sh"); + command.args(["-c", "exit 0"]); + let resumed_after = Rc::clone(&resumed); + let status = run_interactive_editor_with( + &mut command, + || {}, + move || { + *resumed_after.borrow_mut() = true; + }, + ) + .expect("shell should launch"); + + assert!(status.success()); + // The reclaim refused while terminating, so ownership stays published + // and the resume that re-arms every mode never runs. + assert!( + crate::tui::editor_owns_terminal(), + "reclaim cleared ownership while termination had already started" + ); + assert!( + !*resumed.borrow(), + "resume re-armed terminal modes after termination started" + ); + // A late keyboard push is refused too, so no entry is left unpopped + // for the shell to inherit. + assert!(!crate::tui::enable_keyboard_enhancement()); + + // Restore the process-global flags for the other tests in this binary. + crate::tui::set_editor_terminal_ownership(false); + crate::tui::reset_termination_started_for_test(); } } diff --git a/crates/jcode-tui/src/tui/mod.rs b/crates/jcode-tui/src/tui/mod.rs index e4fbf5f0d9..b4ce61d9c6 100644 --- a/crates/jcode-tui/src/tui/mod.rs +++ b/crates/jcode-tui/src/tui/mod.rs @@ -84,6 +84,8 @@ pub use app::{App, CloudHandoff, CopyBadgeUiState, ProcessingStatus, RunResult}; use crate::message::ToolCall; use ratatui::prelude::Frame; use ratatui::text::Line; +use std::sync::Mutex; +use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; pub(crate) fn scheduled_notification_text( @@ -116,6 +118,44 @@ fn keyboard_enhancement_flags() -> crossterm::event::KeyboardEnhancementFlags { | KeyboardEnhancementFlags::REPORT_ALTERNATE_KEYS } +/// Whether this process still owns an unbalanced entry on the terminal's +/// Kitty keyboard-protocol stack. +/// +/// The stack is shared with every program on the terminal: an editor spawned +/// via `suspend_terminal_for_editor` may push its own entry after jcode popped +/// ours. A later cleanup (signal handler, guard drop, picker exit) must not pop +/// again, or it would remove the editor's entry instead of ours. +static KITTY_KEYBOARD_PUSH_ACTIVE: AtomicBool = AtomicBool::new(false); + +/// Whether an external editor currently owns the terminal. +/// +/// During editor handoff jcode has already restored cooked input, left the +/// alternate screen, and disabled its reporting modes. If jcode receives a +/// process-only termination signal while the editor remains alive, cleanup must +/// not write mode disables again because the editor may have enabled those modes +/// for itself. +static EDITOR_TERMINAL_OWNERSHIP: AtomicBool = AtomicBool::new(false); + +/// Whether a termination signal has already begun the process's final cleanup. +/// +/// Once termination started, the process is exiting and the shell behind it +/// must inherit a fully restored terminal. Re-arming any mode afterwards (an +/// editor reclaim/resume that lost the race against the signal cleanup) would +/// leave modes enabled after death because no further cleanup runs (greptile +/// #4232838288). Recorded under the kitty state lock so it cannot interleave +/// with the reclaim's ownership swap. +static TERMINATION_STARTED: AtomicBool = AtomicBool::new(false); + +/// Serializes the terminal writes and the ownership flag transitions of the +/// Kitty push/pop pair. A termination signal (tokio task, not a UNIX signal +/// handler, so taking a lock is allowed) runs its terminal cleanup on the +/// event loop; this lock makes that cleanup wait for an in-flight push or pop +/// to finish instead of observing a half-applied state and skipping the pop. +static KITTY_KEYBOARD_STATE_LOCK: Mutex<()> = Mutex::new(()); + +#[cfg(test)] +pub(crate) static TERMINAL_OWNERSHIP_TEST_LOCK: Mutex<()> = Mutex::new(()); + /// Request Kitty keyboard reporting and tmux's extended-key mode. /// /// Intentionally avoid REPORT_ALL_KEYS_AS_ESCAPE_CODES for now. When that flag is enabled, @@ -127,14 +167,43 @@ fn keyboard_enhancement_flags() -> crossterm::event::KeyboardEnhancementFlags { /// /// Returns whether the requests were written, not whether the terminal supports them. pub fn enable_keyboard_enhancement() -> bool { + let _guard = kitty_keyboard_state_lock() + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + if termination_started() { + crate::logging::info("Keyboard enhancement request skipped: termination already started"); + return false; + } let result = enable_keyboard_enhancement_to(&mut std::io::stdout(), inside_tmux()).is_ok(); crate::logging::info(&format!( "Keyboard enhancement request: {}", if result { "sent" } else { "FAILED" } )); + if result { + // Set while still holding the state lock so a signal cleanup cannot + // observe the push write without the ownership flag. + kitty_keyboard_push_active().store(true, Ordering::Release); + } result } +/// Adopt a Kitty keyboard entry the previous process left on the stack when +/// it exec'd into this one (restart, reload, rebuild, update). +/// +/// The exec handoff deliberately keeps the terminal in raw mode with all +/// modes armed instead of popping and re-pushing, so this process must record +/// that the inherited entry is ours to pop on exit. Otherwise every cleanup +/// gate skips the pop and the following shell is left in CSI u mode. +pub fn adopt_keyboard_enhancement_ownership() { + let _guard = kitty_keyboard_state_lock() + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + if termination_started() { + return; + } + kitty_keyboard_push_active().store(true, Ordering::Release); +} + fn inside_tmux() -> bool { std::env::var_os("TMUX").is_some_and(|value| !value.is_empty()) } @@ -152,8 +221,131 @@ fn enable_keyboard_enhancement_to( } /// Reset tmux extended keys and pop Kitty keyboard reporting. +/// +/// The pop is skipped when this process has no unbalanced push on the +/// terminal's stack (a suspended editor or the calling shell may own the top +/// entry), so an extra pop cannot remove someone else's keyboard settings. pub fn disable_keyboard_enhancement() { - let _ = disable_keyboard_enhancement_to(&mut std::io::stdout(), inside_tmux()); + if !disable_keyboard_enhancement_if_owned() { + crate::logging::info("Keyboard enhancement pop skipped: no active push owned"); + } +} + +/// Pop the Kitty keyboard entry this process pushed, if it is still unbalanced. +/// +/// Returns whether a pop was performed. The state lock orders this against +/// a concurrent push or signal cleanup, so the flag swap and the pop write +/// happen as one unit: termination waits for the pop instead of exiting with +/// the entry still active. +pub fn disable_keyboard_enhancement_if_owned() -> bool { + disable_keyboard_enhancement_if_owned_to(&mut std::io::stdout(), inside_tmux()) +} + +/// Writer-backed form of [`disable_keyboard_enhancement_if_owned`] so tests +/// can verify the pop bytes without writing to the developer's terminal or +/// resetting tmux extended keys. +fn disable_keyboard_enhancement_if_owned_to( + writer: &mut impl std::io::Write, + inside_tmux: bool, +) -> bool { + let _guard = kitty_keyboard_state_lock() + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + if !kitty_keyboard_push_active().swap(false, std::sync::atomic::Ordering::AcqRel) { + return false; + } + let _ = disable_keyboard_enhancement_to(writer, inside_tmux); + true +} + +/// Test hook: reset the ownership flag without touching the terminal. +#[cfg(test)] +fn reset_keyboard_enhancement_ownership_for_test() { + kitty_keyboard_push_active().store(false, Ordering::Release); +} + +fn kitty_keyboard_push_active() -> &'static AtomicBool { + &KITTY_KEYBOARD_PUSH_ACTIVE +} + +fn kitty_keyboard_state_lock() -> &'static Mutex<()> { + &KITTY_KEYBOARD_STATE_LOCK +} + +pub fn editor_owns_terminal() -> bool { + EDITOR_TERMINAL_OWNERSHIP.load(Ordering::Acquire) +} + +pub fn set_editor_terminal_ownership(owns_terminal: bool) { + EDITOR_TERMINAL_OWNERSHIP.store(owns_terminal, Ordering::Release); +} + +/// Publish that an external editor owns the terminal, atomically with the +/// signal cleanup's ownership check (both take the kitty state lock). +/// +/// Callers must only publish AFTER jcode's own terminal restores finished +/// (`suspend_terminal_for_editor` completed): while the restores are in flight +/// the flag must stay false so a termination signal in that window performs a +/// full cleanup instead of skipping one and leaving the shell in raw mode +/// (greptile #4230626960). +pub fn publish_editor_terminal_handoff() { + with_kitty_state_lock(|| { + set_editor_terminal_ownership(true); + }); +} + +/// Reclaim the terminal after the editor exited, atomically with the signal +/// cleanup's ownership check. Called before `resume_terminal_after_editor` +/// re-arms jcode's own modes: a signal arriving after the editor exited must +/// run the full cleanup against whatever state the resume loop has armed so +/// far, not skip it. +pub fn reclaim_editor_terminal_handoff() { + with_kitty_state_lock(|| { + // The signal cleanup released the lock but the process has not exited + // yet: this reclaim runs in that gap, and letting the subsequent resume + // re-arm modes would leave them enabled after death because no further + // cleanup runs (greptile #4232838288). Leave ownership set so the + // resume loop sees the terminating state and writes nothing. + if termination_started() { + return; + } + set_editor_terminal_ownership(false); + }); +} + +/// Run `cleanup` while holding the kitty state lock, so an in-flight editor +/// handoff publication or reclaim (which takes the same lock) is ordered +/// against it. A signal arriving mid-handoff then sees a consistent state: +/// either jcode still owns the terminal (restore everything) or the editor +/// does (skip). +pub fn with_kitty_state_lock(cleanup: impl FnOnce() -> R) -> R { + let lock = kitty_keyboard_state_lock(); + let _state = lock.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); + cleanup() +} + +/// Record that termination has begun, under the kitty state lock. +/// +/// Called by the termination-signal cleanup as soon as it decides what to do +/// with the terminal. Recording under the same lock as the editor handoff's +/// publish/reclaim closes the gap between this cleanup releasing the lock and +/// the process actually exiting: a reclaim racing into that gap observes the +/// flag and leaves the terminal modes alone, so the shell receives normal +/// input after jcode dies instead of escape sequences (greptile #4232838288). +pub fn record_termination_started() { + with_kitty_state_lock(|| { + TERMINATION_STARTED.store(true, Ordering::Release); + }); +} + +fn termination_started() -> bool { + TERMINATION_STARTED.load(Ordering::Acquire) +} + +/// Test hook: clear the shared termination flag without touching the terminal. +#[cfg(test)] +fn reset_termination_started_for_test() { + TERMINATION_STARTED.store(false, Ordering::Release); } fn disable_keyboard_enhancement_to( @@ -247,10 +439,118 @@ fn reapply_terminal_modes_after_focus_to( mod terminal_mode_tests { use super::{ disable_keyboard_enhancement_to, enable_keyboard_enhancement_to, - reapply_keyboard_enhancement_to, reapply_terminal_modes_after_focus_to, - reapply_terminal_modes_to, + kitty_keyboard_push_active, reapply_keyboard_enhancement_to, + reapply_terminal_modes_after_focus_to, reapply_terminal_modes_to, + reset_keyboard_enhancement_ownership_for_test, }; + #[test] + fn disable_skips_kitty_pop_when_no_push_is_owned() { + let _guard = super::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + reset_keyboard_enhancement_ownership_for_test(); + + // Greptile #4228191597: suspend_terminal_for_editor already popped our + // entry while the editor runs, so an extra pop (signal handler firing in + // that window) must be a no-op instead of removing the editor's own + // stack entry. + assert!(!super::disable_keyboard_enhancement_if_owned()); + assert!(!kitty_keyboard_push_active().load(std::sync::atomic::Ordering::Acquire)); + + reset_keyboard_enhancement_ownership_for_test(); + } + + #[test] + fn disable_pops_kitty_entry_exactly_once_then_ownership_is_released() { + let _guard = super::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + reset_keyboard_enhancement_ownership_for_test(); + + // Greptile #4228894633: run the pop against a buffer so the test never + // writes the pop (or the tmux extended-keys reset) to the developer's + // real terminal. + kitty_keyboard_push_active().store(true, std::sync::atomic::Ordering::Release); + let mut output = Vec::new(); + assert!(super::disable_keyboard_enhancement_if_owned_to( + &mut output, + false + )); + assert!(!output.is_empty()); + assert!(!kitty_keyboard_push_active().load(std::sync::atomic::Ordering::Acquire)); + + // A second cleanup (SIGTERM during the editor window, or a duplicated + // teardown) must not pop again now that the ownership is released. + let mut second = Vec::new(); + assert!(!super::disable_keyboard_enhancement_if_owned_to( + &mut second, + false + )); + assert!(second.is_empty()); + + reset_keyboard_enhancement_ownership_for_test(); + } + + #[test] + fn adopt_records_inherited_entry_without_pushing() { + let _guard = super::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + reset_keyboard_enhancement_ownership_for_test(); + + // Greptile #4228894613: after an exec handoff the entry is already on + // the stack, so adopting must only record ownership — no push bytes — + // and the pop must then balance the inherited entry. + super::adopt_keyboard_enhancement_ownership(); + assert!(kitty_keyboard_push_active().load(std::sync::atomic::Ordering::Acquire)); + + let mut output = Vec::new(); + assert!(super::disable_keyboard_enhancement_if_owned_to( + &mut output, + false + )); + assert_eq!(output, b"\x1b[<1u"); + + reset_keyboard_enhancement_ownership_for_test(); + } + + #[test] + fn cleanup_waits_for_in_flight_kitty_push_to_finish() { + let _guard = super::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + reset_keyboard_enhancement_ownership_for_test(); + + // Greptile #4228894626: a termination cleanup must not observe the + // half-applied state between the push write and the ownership flag, + // so the disable waits for the state lock instead of racing it. Hold + // the lock, set the flag as if a push just completed, and confirm a + // concurrent cleanup stays blocked until the lock is released. + let state_guard = super::kitty_keyboard_state_lock() + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + kitty_keyboard_push_active().store(true, std::sync::atomic::Ordering::Release); + + let cleanup = std::thread::spawn(|| { + // Buffer-backed so the blocked cleanup writes no bytes to the + // developer's terminal; the flag swap is the observable effect. + let mut sink = Vec::new(); + super::disable_keyboard_enhancement_if_owned_to(&mut sink, false) + }); + std::thread::sleep(std::time::Duration::from_millis(100)); + // The cleanup is still blocked on the state lock we hold, so the flag + // must be untouched and the thread must not have returned yet. + assert!(!cleanup.is_finished()); + assert!(kitty_keyboard_push_active().load(std::sync::atomic::Ordering::Acquire)); + + drop(state_guard); + assert!(cleanup.join().expect("cleanup thread")); + assert!(!kitty_keyboard_push_active().load(std::sync::atomic::Ordering::Acquire)); + + reset_keyboard_enhancement_ownership_for_test(); + } + #[test] fn tmux_keyboard_lifecycle_requests_and_resets_extended_keys() { let mut output = Vec::new(); diff --git a/scripts/code_size_budget.json b/scripts/code_size_budget.json index 07decdcd84..0219b80189 100644 --- a/scripts/code_size_budget.json +++ b/scripts/code_size_budget.json @@ -81,7 +81,7 @@ "crates/jcode-tui/src/tui/app/auth.rs": 3520, "crates/jcode-tui/src/tui/app/auth_account_commands.rs": 1217, "crates/jcode-tui/src/tui/app/auth_account_picker.rs": 1275, - "crates/jcode-tui/src/tui/app/commands.rs": 3796, + "crates/jcode-tui/src/tui/app/commands.rs": 3980, "crates/jcode-tui/src/tui/app/debug_bench.rs": 1286, "crates/jcode-tui/src/tui/app/helpers.rs": 1751, "crates/jcode-tui/src/tui/app/hotkey_feedback.rs": 1222, @@ -101,7 +101,7 @@ "crates/jcode-tui/src/tui/app/turn.rs": 1738, "crates/jcode-tui/src/tui/backend.rs": 2004, "crates/jcode-tui/src/tui/info_widget.rs": 2267, - "crates/jcode-tui/src/tui/mod.rs": 2196, + "crates/jcode-tui/src/tui/mod.rs": 2496, "crates/jcode-tui/src/tui/session_picker.rs": 2493, "crates/jcode-tui/src/tui/session_picker/loading.rs": 3084, "crates/jcode-tui/src/tui/ui.rs": 3647, diff --git a/src/cli/terminal.rs b/src/cli/terminal.rs index f0dcc70c35..3e8b7b1a59 100644 --- a/src/cli/terminal.rs +++ b/src/cli/terminal.rs @@ -400,9 +400,15 @@ pub fn init_tui_runtime() -> Result<(ratatui::DefaultTerminal, TuiRuntimeGuard)> // The previous process intentionally preserved these modes across exec. // Reassert idempotent modes because terminals, multiplexers, or an older // process may have cleared them during the handoff. Do not push Kitty's - // stack-based keyboard enhancement flags again. A later normal exit must - // still disable every inherited mode, so retain them in the guard. + // stack-based keyboard enhancement flags again: the exec handoff left the + // previous process's entry on the stack, so record that this process + // now owns it and a later normal exit must pop it. Without adopting, + // every cleanup gate would skip the pop and leave the shell in CSI u + // mode after the restarted process exits. let modes = inherited_modes.unwrap_or(fallback_modes); + if modes.keyboard_enhanced { + tui::adopt_keyboard_enhancement_ownership(); + } crossterm::execute!(std::io::stdout(), crossterm::event::EnableBracketedPaste)?; if modes.focus_change { crossterm::execute!(std::io::stdout(), crossterm::event::EnableFocusChange)?; @@ -613,12 +619,27 @@ fn signal_crash_reason(sig: i32) -> String { fn handle_termination_signal(sig: i32) -> ! { mark_current_session_crashed(signal_crash_reason(sig)); - let _ = crossterm::terminal::disable_raw_mode(); - let _ = crossterm::execute!( - std::io::stderr(), - crossterm::terminal::LeaveAlternateScreen, - crossterm::cursor::Show - ); + // Record termination before the terminal cleanup runs. The cleanup may + // skip its restore while an editor owns the terminal and then release the + // kitty state lock while this process is still alive: an editor reclaim + // racing into that gap must observe the flag and refuse to re-arm modes, + // or the shell inherits escape sequences once jcode exits (greptile + // #4232838288). Recording first also keeps the normal path intact: a + // reclaim that slips in before the cleanup still leaves ownership clear, + // so the cleanup runs its full restore. + crate::tui::record_termination_started(); + + if signal_cleanup_terminal_modes_to(signal_cleanup_stream()) { + // The signal handler can fire at any point of the TUI lifetime, including + // while the kitty keyboard protocol is active (Ctrl+C races the cleanup + // below). Pop only when this process still owns an unbalanced kitty entry: + // the pop is a no-op on terminals with no kitty stack, and skipping it when + // a suspended editor already popped ours (or pushed its own) avoids + // removing someone else's keyboard settings. Leaving jcode's own entry + // unpopped would leave the terminal in CSI u mode so the following shell + // echoes raw key sequences like `e1;1:3u` after jcode exits (issue #898). + crate::tui::disable_keyboard_enhancement(); + } if let Some(session_id) = get_current_session() { print_session_resume_hint(&session_id); @@ -627,6 +648,115 @@ fn handle_termination_signal(sig: i32) -> ! { std::process::exit(128 + sig); } +fn signal_cleanup_terminal_modes_to(mut writer: impl Write) -> bool { + // The ownership check and the restore writes run under the kitty state + // lock so an in-flight editor handoff cannot split them: the handoff + // publishes ownership under the same lock after its restores finished + // (greptile #4230626960). Without the lock, a signal could observe + // ownership set while jcode's modes were still armed, skip the cleanup, + // and exit leaving the shell in raw mode. + crate::tui::with_kitty_state_lock(|| { + if crate::tui::editor_owns_terminal() { + return false; + } + + let _ = crossterm::terminal::disable_raw_mode(); + let _ = crossterm::execute!( + writer, + crossterm::event::DisableBracketedPaste, + crossterm::event::DisableFocusChange, + crossterm::event::DisableMouseCapture, + crossterm::terminal::LeaveAlternateScreen, + crossterm::cursor::Show + ); + true + }) +} + +/// Stream the signal cleanup must target: stdout, the interactive terminal +/// the TUI requires, where `setup_terminal` enabled every mode this cleanup +/// disables. Writing to stderr would miss the terminal whenever stderr is +/// redirected and leave the modes armed after jcode exits. +#[cfg(unix)] +fn signal_cleanup_stream() -> std::io::Stdout { + std::io::stdout() +} + +#[cfg(unix)] +#[cfg(test)] +mod signal_cleanup_tests { + use super::*; + use std::sync::Mutex; + + /// Serializes tests that flip the shared editor ownership flag so one + /// test's clear cannot interleave with another's check. + static SIGNAL_CLEANUP_TEST_LOCK: Mutex<()> = Mutex::new(()); + + struct EditorOwnershipGuard; + + impl EditorOwnershipGuard { + fn new(owns_terminal: bool) -> Self { + crate::tui::set_editor_terminal_ownership(owns_terminal); + Self + } + } + + impl Drop for EditorOwnershipGuard { + fn drop(&mut self) { + crate::tui::set_editor_terminal_ownership(false); + } + } + + #[test] + fn signal_cleanup_skips_terminal_modes_while_editor_owns_terminal() { + let _test_lock = SIGNAL_CLEANUP_TEST_LOCK.lock().unwrap(); + let _ownership = EditorOwnershipGuard::new(true); + let mut output = Vec::new(); + + assert!(!signal_cleanup_terminal_modes_to(&mut output)); + assert!(output.is_empty()); + } + + #[test] + fn signal_cleanup_waits_for_in_flight_editor_handoff() { + // Greptile #4230626960: the cleanup must not observe a half-applied + // handoff (flag published while jcode's modes are still armed). The + // publication and reclaim hold the kitty state lock, so the cleanup + // waits for them instead of racing the flag. + let _test_lock = SIGNAL_CLEANUP_TEST_LOCK.lock().unwrap(); + let _ownership = EditorOwnershipGuard::new(false); + + let cleanup = crate::tui::with_kitty_state_lock(|| { + let cleanup = std::thread::spawn(|| { + let mut sink = Vec::new(); + let ran = signal_cleanup_terminal_modes_to(&mut sink); + (ran, sink.is_empty()) + }); + std::thread::sleep(std::time::Duration::from_millis(100)); + assert!( + !cleanup.is_finished(), + "cleanup ran without waiting for the handoff lock" + ); + // Mark the editor as owning the terminal so the released cleanup + // takes the early-return path and writes nothing to the terminal. + crate::tui::set_editor_terminal_ownership(true); + cleanup + }); + + let (ran, sink_empty) = cleanup.join().expect("cleanup thread"); + assert!(!ran); + assert!(sink_empty); + } + + #[test] + fn signal_cleanup_targets_stdout_not_stderr() { + // Same stream object as setup_terminal enables modes on. A write via + // this handle lands on the interactive terminal even when stderr is + // redirected to a file or pipe. + let _stream = signal_cleanup_stream(); + } +} + #[cfg(unix)] pub fn spawn_session_signal_watchers() { use tokio::signal::unix::{SignalKind, signal};