diff --git a/COMMIT_MESSAGE_ISSUE_5922.txt b/COMMIT_MESSAGE_ISSUE_5922.txt new file mode 100644 index 00000000000..92a39672cc9 --- /dev/null +++ b/COMMIT_MESSAGE_ISSUE_5922.txt @@ -0,0 +1 @@ +fix(tui): allow AltGr printable keys on Windows (#5922) diff --git a/COMMIT_MESSAGE_ISSUE_5946.txt b/COMMIT_MESSAGE_ISSUE_5946.txt new file mode 100644 index 00000000000..63e36bf6193 --- /dev/null +++ b/COMMIT_MESSAGE_ISSUE_5946.txt @@ -0,0 +1 @@ +fix(exec): abort PTY readers after forced termination (#5946) diff --git a/PR_BODY.md b/PR_BODY.md index f9128ce12ac..fa77ae22228 100644 --- a/PR_BODY.md +++ b/PR_BODY.md @@ -1,9 +1,10 @@ -## Overview -- AutoRunPhase now carries struct payloads; controller exposes helpers (`is_active`, `is_paused_manual`, `resume_after_submit`, `awaiting_coordinator_submit`, `awaiting_review`, `in_transient_recovery`). -- ChatWidget hot paths (manual pause, coordinator routing, ESC handling, review exit) rely on helpers/`matches!` instead of raw booleans. +## Summary +- abort PTY reader tasks after forced termination so long-running commands (e.g., `dotnet build`) stop hanging on EOF +- join stdout/stderr readers with a short timeout during normal shutdown to guard against pipes left open by orphaned grandchildren +- add an integration regression test that spawns a noisy python loop, calls `kill_all()`, and asserts the exec request completes promptly -## Tests -- `./build-fast.sh` +## Testing +- ./build-fast.sh +- cargo test -p code-core --test dotnet_build_hang *(fails: upstream `cc` 1.2.41 crate is missing generated modules; clear/update the registry and rerun)* -## Follow-ups -- See `docs/auto-drive-phase-migration-TODO.md` for remaining legacy-flag removals and snapshot coverage. +Closes #5946. diff --git a/PR_BODY_ISSUE_5922.md b/PR_BODY_ISSUE_5922.md new file mode 100644 index 00000000000..dbd1c4ab11b --- /dev/null +++ b/PR_BODY_ISSUE_5922.md @@ -0,0 +1,10 @@ +## Summary +- treat Windows AltGr (Control+Alt) key chords as printable characters so `/`, `@`, and other symbols insert correctly in the composer and terminal cells +- keep Ctrl+Alt shortcuts (e.g., Ctrl+Alt+H delete word) by excluding ASCII letters from the AltGr path and add Windows-only regression tests +- cover the change with new `TextArea` unit tests and a ComposerInput integration-style test to prevent future regressions + +## Testing +- ./build-fast.sh +- cargo test -p code-tui --test windows_altgr -- --ignored *(fails: local cargo registry copy of `cc` 1.2.41 is missing generated modules; clear/update the registry and rerun on Windows)* + +Closes #5922. diff --git a/PR_BODY_ISSUE_5946.md b/PR_BODY_ISSUE_5946.md new file mode 100644 index 00000000000..e8dd0ae63aa --- /dev/null +++ b/PR_BODY_ISSUE_5946.md @@ -0,0 +1,10 @@ +## Summary +- abort PTY reader tasks when a shell session is force-killed (timeout or user interrupt) so we don't wait forever for EOF from orphaned grandchildren +- join stdout/stderr readers with a short timeout during normal completion to avoid indefinite hangs when a child leaves pipes open +- add an exec regression test that kills a long-running python loop and ensures `kill_all()` unblocks pending exec requests + +## Testing +- ./build-fast.sh +- cargo test -p code-core --test dotnet_build_hang *(fails: upstream `cc` 1.2.41 crate is missing generated modules in the local cargo registry; clear/update the registry and rerun)* + +Closes #5946. diff --git a/code-rs/core/Cargo.toml b/code-rs/core/Cargo.toml index 897dc22552f..78a24b85ee4 100644 --- a/code-rs/core/Cargo.toml +++ b/code-rs/core/Cargo.toml @@ -109,6 +109,7 @@ serial_test = "3.2.0" pretty_assertions = { workspace = true } tokio-test = { workspace = true } wiremock = { workspace = true } +serde_json = { workspace = true } [package.metadata.cargo-shear] ignored = ["openssl-sys"] diff --git a/code-rs/core/src/exec.rs b/code-rs/core/src/exec.rs index 9c26b3c0e4c..757f6838505 100644 --- a/code-rs/core/src/exec.rs +++ b/code-rs/core/src/exec.rs @@ -15,6 +15,9 @@ use tokio::io::AsyncRead; use tokio::io::AsyncReadExt; use tokio::io::BufReader; use tokio::process::Child; +use tokio::task::JoinHandle; +use tokio::time::Duration as TokioDuration; +use tokio::time::sleep; use crate::codex::Session; use crate::error::CodexErr; @@ -367,6 +370,7 @@ async fn consume_truncated_output( Some(agg_tx.clone()), )); + let mut forced_termination = false; let (exit_status, timed_out) = match timeout { Some(timeout) => { tokio::select! { @@ -378,6 +382,7 @@ async fn consume_truncated_output( } Err(_) => { // timeout + forced_termination = true; #[cfg(unix)] { if let Some(pid) = killer.as_mut().id() { @@ -392,6 +397,7 @@ async fn consume_truncated_output( } } _ = tokio::signal::ctrl_c() => { + forced_termination = true; killer.as_mut().start_kill()?; (synthetic_exit_status(EXIT_CODE_SIGNAL_BASE + SIGKILL_CODE), false) } @@ -405,6 +411,7 @@ async fn consume_truncated_output( (exit_status, false) } _ = tokio::signal::ctrl_c() => { + forced_termination = true; killer.as_mut().start_kill()?; (synthetic_exit_status(EXIT_CODE_SIGNAL_BASE + SIGKILL_CODE), false) } @@ -416,26 +423,24 @@ async fn consume_truncated_output( // avoid re-sending a kill signal during Drop. killer.disarm(); - // If we timed out, abort the readers after a short grace to prevent hanging when pipes - // remain open due to orphaned grandchildren. - let (stdout, stderr) = if timed_out { - // Abort reader tasks to avoid hanging if pipes remain open. - stdout_handle.abort(); - stderr_handle.abort(); - ( - StreamOutput { - text: Vec::new(), - truncated_after_lines: None, - truncated_before_bytes: None, - }, - StreamOutput { - text: Vec::new(), - truncated_after_lines: None, - truncated_before_bytes: None, - }, - ) + let join_timeout = TokioDuration::from_millis(250); + let force_abort = forced_termination || timed_out; + let stdout = if force_abort { + abort_reader(stdout_handle).await } else { - (stdout_handle.await??, stderr_handle.await??) + match join_reader_with_timeout(stdout_handle, join_timeout).await? { + Some(output) => output, + None => empty_stream_output(), + } + }; + + let stderr = if force_abort { + abort_reader(stderr_handle).await + } else { + match join_reader_with_timeout(stderr_handle, join_timeout).await? { + Some(output) => output, + None => empty_stream_output(), + } }; drop(agg_tx); @@ -544,6 +549,42 @@ async fn read_capped( }) } +fn empty_stream_output() -> StreamOutput> { + StreamOutput { + text: Vec::new(), + truncated_after_lines: None, + truncated_before_bytes: None, + } +} + +async fn abort_reader( + handle: JoinHandle>>>, +) -> StreamOutput> { + handle.abort(); + let _ = handle.await; + empty_stream_output() +} + +async fn join_reader_with_timeout( + handle: JoinHandle>>>, + timeout: TokioDuration, +) -> io::Result>>> { + let mut handle = Box::pin(handle); + let sleeper = sleep(timeout); + tokio::pin!(sleeper); + tokio::select! { + res = handle.as_mut() => { + let output = res.map_err(|join_err| io::Error::other(join_err))??; + Ok(Some(output)) + } + _ = &mut sleeper => { + handle.as_ref().abort(); + let _ = handle.await; + Ok(None) + } + } +} + #[cfg(unix)] fn synthetic_exit_status(code: i32) -> ExitStatus { use std::os::unix::process::ExitStatusExt; diff --git a/code-rs/core/tests/dotnet_build_hang.rs b/code-rs/core/tests/dotnet_build_hang.rs new file mode 100644 index 00000000000..1f16f3507f6 --- /dev/null +++ b/code-rs/core/tests/dotnet_build_hang.rs @@ -0,0 +1,47 @@ +#![cfg(unix)] + +use std::sync::Arc; +use std::time::Duration; + +use code_core::exec_command::{ExecCommandParams, ExecSessionManager}; +use serde_json::json; +use tokio::time::timeout; + +fn make_params(script: &str) -> ExecCommandParams { + serde_json::from_value(json!({ + "cmd": script, + "yield_time_ms": 1_000u64, + "max_output_tokens": 1_000u64, + "shell": "/bin/bash", + "login": true, + })) + .expect("deserialize ExecCommandParams") +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn kill_all_unblocks_hanging_exec() { + let manager = Arc::new(ExecSessionManager::default()); + let script = r#"python3 -u - <<'PY' +import sys +import time +while True: + print("tick", flush=True) + time.sleep(0.05) +PY"#; + + let params = make_params(script); + let manager_clone = manager.clone(); + let exec_task = tokio::spawn(async move { + manager_clone.handle_exec_command_request(params).await + }); + + tokio::time::sleep(Duration::from_millis(200)).await; + manager.kill_all().await; + + let result = timeout(Duration::from_secs(2), exec_task) + .await + .expect("exec task should finish after kill_all"); + + let output = result.expect("exec task join"); + assert!(output.is_ok(), "exec request should return Ok even after kill"); +} diff --git a/code-rs/tui/src/bottom_pane/textarea.rs b/code-rs/tui/src/bottom_pane/textarea.rs index f5aecd736f3..66f9975d5c6 100644 --- a/code-rs/tui/src/bottom_pane/textarea.rs +++ b/code-rs/tui/src/bottom_pane/textarea.rs @@ -246,6 +246,20 @@ impl TextArea { code: KeyCode::Char(c), .. } if matches!(c, '\n' | '\r') => self.insert_str("\n"), + #[cfg(target_os = "windows")] + KeyEvent { + code: KeyCode::Char(c), + modifiers, + .. + } if (modifiers == (KeyModifiers::CONTROL | KeyModifiers::ALT) + || modifiers == (KeyModifiers::CONTROL + | KeyModifiers::ALT + | KeyModifiers::SHIFT)) + && !c.is_ascii_control() + && !c.is_ascii_alphabetic() => + { + self.insert_str(&c.to_string()); + } KeyEvent { code: KeyCode::Char(c), // Insert plain characters (and Shift-modified). Do NOT insert when ALT is held, @@ -909,6 +923,43 @@ impl TextArea { } } +#[cfg(all(test, target_os = "windows"))] +mod windows_tests { + use super::*; + use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; + + #[test] + fn altgr_control_alt_characters_insert_printables() { + let mut textarea = TextArea::new(); + let cases = [ + ('/', KeyModifiers::CONTROL | KeyModifiers::ALT), + ('@', KeyModifiers::CONTROL | KeyModifiers::ALT), + ('{', KeyModifiers::CONTROL | KeyModifiers::ALT | KeyModifiers::SHIFT), + ]; + + for (ch, modifiers) in cases { + textarea.set_text(""); + textarea.set_cursor(0); + textarea.input(KeyEvent::new(KeyCode::Char(ch), modifiers)); + assert_eq!(textarea.text(), ch.to_string(), "expected AltGr combination to insert {ch}"); + } + } + + #[test] + fn ctrl_alt_letter_shortcut_preserved() { + let mut textarea = TextArea::new(); + textarea.set_text("word"); + textarea.set_cursor(textarea.text().len()); + + textarea.input(KeyEvent::new( + KeyCode::Char('h'), + KeyModifiers::CONTROL | KeyModifiers::ALT, + )); + + assert_eq!(textarea.text(), "", "Ctrl+Alt+H should still delete backward word"); + } +} + impl WidgetRef for &TextArea { fn render_ref(&self, area: Rect, buf: &mut Buffer) { let lines = self.wrapped_lines(area.width); @@ -955,4 +1006,3 @@ impl TextArea { } } } - diff --git a/code-rs/tui/tests/windows_altgr.rs b/code-rs/tui/tests/windows_altgr.rs new file mode 100644 index 00000000000..368d4e9f4ca --- /dev/null +++ b/code-rs/tui/tests/windows_altgr.rs @@ -0,0 +1,34 @@ +#![cfg(target_os = "windows")] + +use code_tui::public_widgets::ComposerInput; +use crossterm::event::{KeyCode, KeyEvent, KeyModifiers}; + +#[test] +fn composer_input_altgr_characters_insert_text() { + let mut composer = ComposerInput::new(); + + let cases = [ + ('/', KeyModifiers::CONTROL | KeyModifiers::ALT), + ('@', KeyModifiers::CONTROL | KeyModifiers::ALT), + ('{', KeyModifiers::CONTROL | KeyModifiers::ALT | KeyModifiers::SHIFT), + ]; + + for (ch, modifiers) in cases { + composer.clear(); + let _ = composer.input(KeyEvent::new(KeyCode::Char(ch), modifiers)); + assert_eq!(composer.text(), ch.to_string(), "AltGr input should insert printable character"); + } +} + +#[test] +fn composer_input_ctrl_alt_letter_shortcut_still_deletes_word() { + let mut composer = ComposerInput::new(); + composer.handle_paste("word".to_string()); + + let _ = composer.input(KeyEvent::new( + KeyCode::Char('h'), + KeyModifiers::CONTROL | KeyModifiers::ALT, + )); + + assert!(composer.text().is_empty(), "Ctrl+Alt+H should delete the previous word"); +}