mirror of
https://github.com/openai/codex.git
synced 2026-10-28 18:58:46 +00:00
Drain pending terminal reports before returning to the shell (#57391)
## Why Terminal reports still in flight when Codex exits can leak into the parent shell's input. ## What changed Pause terminal event polling during shutdown and flush pending input before restoring normal terminal input mode. On Unix, use a primary device attributes (DA1) reply to wait for outstanding reports after disabling reporting, with a two-second timeout when startup detected DA1 support. During panic unwinding, flush queued input without locking the event reader. ## Testing Extend PTY tests to cover mouse-report bursts with a split DA1 reply, clean shell input after exit, and queued input cleanup during panic unwinding. GitOrigin-RevId: 304859b654fb2b2c8882e6cac4cb6522fe05475c
This commit is contained in:
@@ -1677,6 +1677,8 @@ See the Codex keymap documentation for supported actions and examples."
|
||||
}
|
||||
}
|
||||
};
|
||||
// Let the pending terminal poll unwind while the app server shuts down.
|
||||
tui.pause_events();
|
||||
app.chat_widget.mention_usage_mut().flush().await;
|
||||
if let Err(err) = backends.shutdown().await {
|
||||
tracing::warn!(error = %err, "failed to shut down embedded app server");
|
||||
|
||||
+10
-9
@@ -2215,8 +2215,8 @@ async fn run_ratatui_app(
|
||||
clippy::print_stderr,
|
||||
reason = "TUI should no longer be displayed, so we can write to stderr."
|
||||
)]
|
||||
fn restore() {
|
||||
if let Err(err) = tui::restore_after_exit() {
|
||||
fn report_restore_error(result: color_eyre::Result<()>) {
|
||||
if let Err(err) = result {
|
||||
eprintln!(
|
||||
"failed to restore terminal. Run `reset` or restart your terminal to recover: {err}"
|
||||
);
|
||||
@@ -2225,27 +2225,28 @@ fn restore() {
|
||||
|
||||
struct TerminalRestoreGuard {
|
||||
active: bool,
|
||||
events: Option<Arc<tui::EventBroker>>,
|
||||
}
|
||||
|
||||
impl TerminalRestoreGuard {
|
||||
fn new() -> Self {
|
||||
Self { active: true }
|
||||
Self {
|
||||
active: true,
|
||||
events: None,
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg_attr(debug_assertions, allow(dead_code))]
|
||||
fn restore(&mut self) -> color_eyre::Result<()> {
|
||||
if self.active {
|
||||
crate::tui::restore_after_exit()?;
|
||||
tui::restore_after_exit_with_events(self.events.take().as_deref())?;
|
||||
self.active = false;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn restore_silently(&mut self) {
|
||||
if self.active {
|
||||
restore();
|
||||
self.active = false;
|
||||
}
|
||||
report_restore_error(self.restore());
|
||||
self.active = false;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -123,7 +123,7 @@ impl StartupDraft {
|
||||
/// Adopt the initialized terminal and apply editing policy before the first composer paint.
|
||||
pub(crate) fn new(
|
||||
mut initialized_terminal: tui::InitializedTerminal,
|
||||
terminal_restore_guard: TerminalRestoreGuard,
|
||||
mut terminal_restore_guard: TerminalRestoreGuard,
|
||||
initial_screen: StartupDraftInitialScreen,
|
||||
session_action: StartupDraftSessionAction,
|
||||
screen: StartupScreen,
|
||||
@@ -135,6 +135,7 @@ impl StartupDraft {
|
||||
initialized_terminal.enhanced_keys_supported,
|
||||
initialized_terminal.stderr_guard,
|
||||
);
|
||||
terminal_restore_guard.events = Some(tui.event_broker.clone());
|
||||
tui.terminal_app_over_ssh = initialized_terminal.terminal_app_over_ssh;
|
||||
tui.set_alt_screen_enabled(screen.use_alt_screen);
|
||||
let mut pump = StartupDraftPump::new(&tui, initial_screen, session_action);
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
//! Short, best-effort terminal response probes for TUI startup and resume.
|
||||
//! Short, best-effort terminal response probes for TUI startup, resume, and exit.
|
||||
//!
|
||||
//! Crossterm's public helpers wait up to two seconds for terminal responses. That is too long for
|
||||
//! TUI startup and resume, where unsupported terminals should simply fall back to conservative
|
||||
@@ -48,6 +48,8 @@ mod imp {
|
||||
use std::io::Write;
|
||||
use std::os::fd::AsRawFd;
|
||||
use std::os::fd::FromRawFd;
|
||||
use std::sync::atomic::AtomicBool;
|
||||
use std::sync::atomic::Ordering;
|
||||
use std::time::Duration;
|
||||
use std::time::Instant;
|
||||
|
||||
@@ -59,6 +61,8 @@ mod imp {
|
||||
const MAX_STARTUP_PROBE_BYTES: usize = 64 * 1024;
|
||||
const MAX_TERMINAL_CSI_BYTES: usize = 64;
|
||||
|
||||
static DA1_SUPPORTED: AtomicBool = AtomicBool::new(false);
|
||||
|
||||
/// Results from the TUI's one-shot startup terminal probe.
|
||||
#[derive(Debug, Clone, Copy, Eq, PartialEq)]
|
||||
pub(crate) struct StartupProbe {
|
||||
@@ -253,10 +257,7 @@ mod imp {
|
||||
pub(crate) fn default_colors(timeout: Duration) -> io::Result<Option<DefaultColors>> {
|
||||
let mut tty = Tty::open()?;
|
||||
tty.write_all(b"\x1B]10;?\x1B\\\x1B]11;?\x1B\\")?;
|
||||
let Some(colors) = read_until(&mut tty, timeout, parse_default_colors)? else {
|
||||
return Ok(None);
|
||||
};
|
||||
Ok(Some(colors))
|
||||
read_until(&mut tty, timeout, |buffer| parse_default_colors(buffer))
|
||||
}
|
||||
|
||||
/// Queries the terminal cursor position while normal input polling is paused.
|
||||
@@ -267,7 +268,24 @@ mod imp {
|
||||
pub(crate) fn cursor_position(timeout: Duration) -> io::Result<Option<Position>> {
|
||||
let mut tty = Tty::open()?;
|
||||
tty.write_all(b"\x1B[6n")?;
|
||||
read_until(&mut tty, timeout, parse_cursor_position)
|
||||
read_until(&mut tty, timeout, |buffer| parse_cursor_position(buffer))
|
||||
}
|
||||
|
||||
/// A DA1 reply follows reports the terminal generated before we disabled reporting.
|
||||
pub(crate) fn wait_for_exit_reports() -> io::Result<()> {
|
||||
if !DA1_SUPPORTED.load(Ordering::Relaxed) {
|
||||
return Ok(());
|
||||
}
|
||||
let mut tty = Tty::open()?;
|
||||
tty.write_all(b"\x1b[c")?;
|
||||
read_until(&mut tty, Duration::from_secs(/*secs*/ 2), |buffer| {
|
||||
let reply = find_primary_device_attributes(buffer);
|
||||
// Keep a possible split reply and discard older input so bursts cannot fill the probe.
|
||||
let keep = b"\x1b[?".len() + MAX_TERMINAL_CSI_BYTES;
|
||||
buffer.drain(..buffer.len().saturating_sub(keep));
|
||||
reply
|
||||
})?
|
||||
.ok_or_else(|| io::Error::new(io::ErrorKind::TimedOut, "no DA1 reply"))
|
||||
}
|
||||
|
||||
/// Runs the optional terminal queries needed during TUI startup under one shared deadline.
|
||||
@@ -278,20 +296,19 @@ mod imp {
|
||||
timeout: Duration,
|
||||
keyboard_probe: StartupKeyboardEnhancementProbe,
|
||||
) -> io::Result<StartupProbe> {
|
||||
DA1_SUPPORTED.store(false, Ordering::Relaxed);
|
||||
let mut tty = Tty::open()?;
|
||||
let query_identity = codex_terminal_detection::terminal_info()
|
||||
.multiplexer
|
||||
.is_none()
|
||||
&& (std::env::var_os("SSH_TTY").is_some()
|
||||
|| std::env::var_os("SSH_CONNECTION").is_some());
|
||||
let mut query = b"\x1B[6n\x1B]10;?\x1B\\\x1B]11;?\x1B\\".to_vec();
|
||||
let mut query = Vec::new();
|
||||
if keyboard_probe == StartupKeyboardEnhancementProbe::Query {
|
||||
query.extend_from_slice(b"\x1B[?u");
|
||||
}
|
||||
// DA1 must follow the keyboard query: its reply also finishes keyboard detection.
|
||||
if keyboard_probe == StartupKeyboardEnhancementProbe::Query || query_identity {
|
||||
query.extend_from_slice(b"\x1B[c");
|
||||
}
|
||||
// DA1 finishes keyboard detection and establishes whether we can use it at exit.
|
||||
query.extend_from_slice(b"\x1B[c\x1B[6n\x1B]10;?\x1B\\\x1B]11;?\x1B\\");
|
||||
if query_identity {
|
||||
query.extend_from_slice(b"\x1B[>c");
|
||||
}
|
||||
@@ -306,24 +323,27 @@ mod imp {
|
||||
&mut buffer,
|
||||
);
|
||||
crossterm::event::buffer_input(&startup_replay_input(&buffer))?;
|
||||
let saw_da1 = find_primary_device_attributes(&buffer).is_some();
|
||||
DA1_SUPPORTED.store(saw_da1, Ordering::Relaxed);
|
||||
result
|
||||
}
|
||||
|
||||
/// Reads available terminal bytes until `parse` recognizes a probe response or time expires.
|
||||
///
|
||||
/// A parser can discard bytes it has ruled out to continue past the byte budget at exit.
|
||||
/// The accumulated buffer may include unrelated terminal input. This helper intentionally does
|
||||
/// not try to replay those bytes, so callers must use it only during short, exclusive probe
|
||||
/// windows before normal crossterm input polling begins or while that polling is paused.
|
||||
fn read_until<T>(
|
||||
tty: &mut Tty,
|
||||
timeout: Duration,
|
||||
mut parse: impl FnMut(&[u8]) -> Option<T>,
|
||||
mut parse: impl FnMut(&mut Vec<u8>) -> Option<T>,
|
||||
) -> io::Result<Option<T>> {
|
||||
let deadline = Instant::now() + timeout;
|
||||
let mut buffer = Vec::new();
|
||||
loop {
|
||||
tty.read_available(&mut buffer, deadline, MAX_TERMINAL_PROBE_BYTES)?;
|
||||
if let Some(value) = parse(&buffer) {
|
||||
if let Some(value) = parse(&mut buffer) {
|
||||
return Ok(Some(value));
|
||||
}
|
||||
let now = Instant::now();
|
||||
|
||||
+29
-4
@@ -52,7 +52,7 @@ use crate::notifications::detect_backend;
|
||||
use crate::terminal_hyperlinks::HyperlinkLine;
|
||||
use crate::terminal_hyperlinks::plain_hyperlink_lines;
|
||||
use crate::tui::alternate_screen::ALTERNATE_SCREEN;
|
||||
use crate::tui::event_stream::EventBroker;
|
||||
pub(super) use crate::tui::event_stream::EventBroker;
|
||||
use crate::tui::event_stream::TuiEventStream;
|
||||
#[cfg(unix)]
|
||||
use crate::tui::job_control::SuspendContext;
|
||||
@@ -315,6 +315,7 @@ impl Command for DisableAlternateScroll {
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
enum RawModeRestore {
|
||||
DrainAndDisable,
|
||||
Disable,
|
||||
Keep,
|
||||
}
|
||||
@@ -356,7 +357,14 @@ fn restore_common(
|
||||
first_error.get_or_insert(err);
|
||||
}
|
||||
let _ = execute!(stdout(), DisableFocusChange);
|
||||
if matches!(raw_mode_restore, RawModeRestore::Disable)
|
||||
if raw_mode_restore == RawModeRestore::DrainAndDisable {
|
||||
#[cfg(unix)]
|
||||
if let Err(err) = crate::terminal_probe::wait_for_exit_reports() {
|
||||
tracing::debug!(%err, "failed to drain outstanding terminal reports");
|
||||
}
|
||||
flush_terminal_input_buffer();
|
||||
}
|
||||
if raw_mode_restore != RawModeRestore::Keep
|
||||
&& let Err(err) = disable_raw_mode()
|
||||
{
|
||||
first_error.get_or_insert(err);
|
||||
@@ -406,8 +414,25 @@ pub(super) fn reapply_raw_mode_after_resume() -> Result<()> {
|
||||
/// Uses a stronger keyboard reset than `restore` so the parent shell recovers even if a
|
||||
/// terminal missed the stack pop that normally pairs with [`set_modes`].
|
||||
pub fn restore_after_exit() -> Result<()> {
|
||||
restore_after_exit_with_events(/*events*/ None)
|
||||
}
|
||||
|
||||
/// Discard reports and interleaved keystrokes before handing the terminal back to the shell.
|
||||
pub(super) fn restore_after_exit_with_events(events: Option<&EventBroker>) -> Result<()> {
|
||||
let raw_mode_restore = match events {
|
||||
Some(_) if std::thread::panicking() => {
|
||||
// The panic hook restored terminal modes. Flush queued reports without locking the reader.
|
||||
flush_terminal_input_buffer();
|
||||
RawModeRestore::Disable
|
||||
}
|
||||
Some(events) => {
|
||||
events.pause_events();
|
||||
RawModeRestore::DrainAndDisable
|
||||
}
|
||||
None => RawModeRestore::Disable,
|
||||
};
|
||||
let mut first_error = restore_common(
|
||||
RawModeRestore::Disable,
|
||||
raw_mode_restore,
|
||||
KeyboardRestore::ResetAfterExit,
|
||||
TerminalHandoff::Restore,
|
||||
)
|
||||
@@ -667,7 +692,7 @@ pub struct Tui {
|
||||
pub(crate) link_hover: link_pointer::LinkHover,
|
||||
frame_requester: FrameRequester,
|
||||
draw_tx: broadcast::Sender<()>,
|
||||
event_broker: Arc<EventBroker>,
|
||||
pub(super) event_broker: Arc<EventBroker>,
|
||||
pub(crate) terminal_size_tags: crate::terminal_size_telemetry::LaunchTags,
|
||||
pub(crate) terminal: Terminal,
|
||||
pending_history_lines: Vec<PendingHistoryLines>,
|
||||
|
||||
@@ -45,6 +45,9 @@ fn caught_panic_rehides_cursor_before_repainting_and_restores_its_shape() {
|
||||
let mut master = unsafe { File::from_raw_fd(master) };
|
||||
// SAFETY: the slave descriptor is distinct from the master and is likewise newly owned.
|
||||
let slave = unsafe { File::from_raw_fd(slave) };
|
||||
master
|
||||
.write_all(b"\x1b[<51;2;56M\n")
|
||||
.expect("queue mouse report");
|
||||
let child = Command::new(std::env::current_exe().expect("current test binary"))
|
||||
.args([
|
||||
"--exact",
|
||||
@@ -157,7 +160,17 @@ async fn caught_panic_pty_child() {
|
||||
mark(BEFORE_VISIBLE_FRAME).expect("mark before visible frame");
|
||||
draw(&mut tui, "after panic", Some((4, 1))).expect("draw recovered visible cursor");
|
||||
mark(AFTER_VISIBLE_FRAME).expect("mark after visible frame");
|
||||
super::restore_after_exit().expect("restore terminal after test");
|
||||
assert!(super::terminal_input_is_readable().expect("caught panic preserves pending input"));
|
||||
let events = tui.event_broker.clone();
|
||||
let unwind = std::panic::catch_unwind(std::panic::AssertUnwindSafe(move || {
|
||||
let _guard = crate::TerminalRestoreGuard {
|
||||
active: true,
|
||||
events: Some(events),
|
||||
};
|
||||
panic!("unwind the terminal owner");
|
||||
}));
|
||||
assert!(unwind.is_err());
|
||||
assert!(!super::terminal_input_is_readable().expect("check shell input after unwind"));
|
||||
}
|
||||
|
||||
fn draw(tui: &mut super::Tui, text: &str, cursor: Option<(u16, u16)>) -> std::io::Result<()> {
|
||||
|
||||
@@ -227,8 +227,22 @@ fn default_owned_screen_entry_paints_before_sync_ends_and_exit_clears_inline_dra
|
||||
(1, (u16::try_from(composer_row)?, 2), false),
|
||||
"first owned-screen frame must contain one composer with its visible cursor"
|
||||
);
|
||||
let exit_start = terminal.output.len();
|
||||
terminal.write_input(b"\x1b[99;5u")?;
|
||||
let deadline = Instant::now() + STARTUP_TIMEOUT;
|
||||
while !contains_bytes(&terminal.output[exit_start..], b"\x1b[c") {
|
||||
terminal.ensure_running()?;
|
||||
ensure!(Instant::now() < deadline, "missing exit DA1 query");
|
||||
terminal.read_output(Duration::from_millis(/*millis*/ 50))?;
|
||||
}
|
||||
assert_eq!(
|
||||
terminal.parser.screen().mouse_protocol_mode(),
|
||||
vt100::MouseProtocolMode::None
|
||||
);
|
||||
// The DA1 reply straddles the first 1 KiB of in-flight mouse reports.
|
||||
let mut reports = b"\x1b[<51;23;56M".repeat(85);
|
||||
reports.extend_from_slice(b"\x1b[?1;2c");
|
||||
terminal.write_input(&reports)?;
|
||||
loop {
|
||||
terminal.read_output(Duration::from_millis(/*millis*/ 50))?;
|
||||
if let Some(status) = terminal.child.try_wait()? {
|
||||
@@ -241,6 +255,10 @@ fn default_owned_screen_entry_paints_before_sync_ends_and_exit_clears_inline_dra
|
||||
terminal.screen_contents()
|
||||
);
|
||||
}
|
||||
terminal.write_input(b"shell\n")?;
|
||||
let mut shell_input = [0; 6];
|
||||
terminal.slave.read_exact(&mut shell_input)?;
|
||||
assert_eq!(&shell_input, b"shell\n");
|
||||
ensure!(
|
||||
!terminal.parser.screen().alternate_screen(),
|
||||
"alternate screen was not restored"
|
||||
@@ -278,6 +296,7 @@ fn fullscreen_transcript_can_opt_out_to_terminal_scrollback() -> Result<()> {
|
||||
|
||||
pub(super) struct PtyCodex {
|
||||
master: File,
|
||||
slave: File,
|
||||
child: Child,
|
||||
parser: vt100::Parser,
|
||||
output: Vec<u8>,
|
||||
@@ -370,12 +389,13 @@ impl PtyCodex {
|
||||
.env("CODEX_HOME", codex_home.path())
|
||||
.stdin(stdin)
|
||||
.stdout(stdout)
|
||||
.stderr(slave)
|
||||
.stderr(slave.try_clone()?)
|
||||
.spawn()
|
||||
.context("start Codex in focus-test pseudo-terminal")?;
|
||||
|
||||
Ok(Self {
|
||||
master,
|
||||
slave,
|
||||
child,
|
||||
parser: vt100::Parser::new(
|
||||
/*rows*/ 32, /*cols*/ 120, /*scrollback_len*/ 0,
|
||||
@@ -446,8 +466,10 @@ impl PtyCodex {
|
||||
self.cursor_answered = true;
|
||||
}
|
||||
|
||||
if !self.keyboard_answered && contains_bytes(&self.output, b"\x1b[?u") {
|
||||
self.write_input(b"\x1b[?7u")?;
|
||||
if !self.keyboard_answered && contains_bytes(&self.output, b"\x1b[c") {
|
||||
if contains_bytes(&self.output, b"\x1b[?u") {
|
||||
self.write_input(b"\x1b[?7u")?;
|
||||
}
|
||||
self.write_input(b"\x1b[?1;2c")?;
|
||||
self.keyboard_answered = true;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user