Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/next/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

### Fixed
- Qwen Code panes now use locale-independent terminal-title states and localized confirmation fallbacks, preventing active or blocked turns from appearing idle. (#2756)
- Closing a terminal running `herdr --remote` no longer produces a local client core dump while the remote session stays alive. (#2424)
- Active Space and Agent rows now use dedicated theme colors that remain visible when the host terminal background matches the selected Herdr theme. (#2792)
- `agent prompt` now rejects agents already waiting at approval or question dialogs with `agent_blocked`, without sending text or Enter. (#2788)
- `prefix+e` now preserves logical lines when opening soft-wrapped scrollback in an editor. (#2733)
Expand Down
46 changes: 34 additions & 12 deletions src/client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,7 @@ fn setup_terminal_with_capabilities(
Ok(TerminalGuard {
reset_modify_other_keys: modify_other_keys_mode.is_some(),
reset_host_color_scheme_reports: host_color_scheme_reports,
restored: false,
#[cfg(windows)]
restore_windows_input_mode: windows_virtual_terminal_input.restore_mode,
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Expand All @@ -434,6 +435,7 @@ fn should_enable_host_color_scheme_reports(enable_client_protocols: bool) -> boo
struct TerminalGuard {
reset_modify_other_keys: bool,
reset_host_color_scheme_reports: bool,
restored: bool,
#[cfg(windows)]
restore_windows_input_mode: Option<u32>,
}
Expand Down Expand Up @@ -582,7 +584,7 @@ fn restore_terminal_state(
reset_modify_other_keys: bool,
reset_host_color_scheme_reports: bool,
#[cfg(windows)] restore_windows_input_mode: Option<u32>,
) {
) -> io::Result<()> {
let _ = clear_received_kitty_graphics(&mut io::stdout());

// Reset modifyOtherKeys if we enabled it.
Expand All @@ -606,13 +608,16 @@ fn restore_terminal_state(
restore_windows_input_mode_value(mode);
}

let _ = ratatui::try_restore();
let _ = write_terminal_restore_postlude(&mut io::stdout(), reset_host_color_scheme_reports);
let restore_result = ratatui::try_restore();
let postlude_result =
write_terminal_restore_postlude(&mut io::stdout(), reset_host_color_scheme_reports);

#[cfg(windows)]
if windows_vti_input_backend_enabled() && windows_win32_input_mode_enabled() {
let _ = disable_windows_win32_input_mode(&mut io::stdout());
}

restore_result.and(postlude_result)
}

#[cfg(not(windows))]
Expand Down Expand Up @@ -657,14 +662,28 @@ fn disable_windows_win32_input_mode(writer: &mut impl std::io::Write) -> io::Res
writer.flush()
}

impl Drop for TerminalGuard {
fn drop(&mut self) {
impl TerminalGuard {
fn restore(mut self) -> io::Result<()> {
self.restored = true;
restore_terminal_state(
self.reset_modify_other_keys,
self.reset_host_color_scheme_reports,
#[cfg(windows)]
self.restore_windows_input_mode,
);
)
}
}

impl Drop for TerminalGuard {
fn drop(&mut self) {
if !self.restored {
let _ = restore_terminal_state(
self.reset_modify_other_keys,
self.reset_host_color_scheme_reports,
#[cfg(windows)]
self.restore_windows_input_mode,
);
}
}
}

Expand Down Expand Up @@ -1281,7 +1300,7 @@ fn run_client_with_mode(
let panic_restore_windows_input_mode = terminal_guard.restore_windows_input_mode;
let original_hook = std::panic::take_hook();
std::panic::set_hook(Box::new(move |info| {
restore_terminal_state(
let _ = restore_terminal_state(
panic_resets_modify_other_keys,
panic_resets_host_color_scheme_reports,
#[cfg(windows)]
Expand Down Expand Up @@ -1323,19 +1342,22 @@ fn run_client_with_mode(
});

// Restore the terminal before printing any final status message.
drop(terminal_guard);
let terminal_restore_failed = terminal_guard.restore().is_err();

if let Err(err) = result {
eprintln!("herdr: {err}");
let _ = writeln!(io::stderr(), "herdr: {err}");
rt.shutdown_timeout(Duration::from_millis(100));
crate::logging::shutdown("client");

if matches!(
err,
let detached = matches!(
&err,
ClientError::ServerShutdown {
reason: Some(reason)
} if reason == "detached"
) {
);
let connection_lost_during_terminal_hangup =
terminal_restore_failed && matches!(&err, ClientError::ConnectionLost(_));
if detached || connection_lost_during_terminal_hangup {
return Ok(());
}

Expand Down
59 changes: 59 additions & 0 deletions tests/client_mode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1075,6 +1075,65 @@ fn read_until_client_attaches(client: &SpawnedHerdr) -> String {
panic!("thin client must attach and render a frame; output: {output:?}");
}

#[test]
fn client_exits_cleanly_when_terminal_and_transport_hang_up() {
let _lock = test_lock();
let base = unique_test_dir();
let config_home = base.join("config");
let runtime_dir = base.join("runtime");
let api_socket = runtime_dir.join("herdr.sock");
let client_socket = runtime_dir.join("herdr-client.sock");

let mut spawned_server = spawn_server(&config_home, &runtime_dir, &api_socket, &client_socket);
wait_for_socket(&api_socket, Duration::from_secs(10));
wait_for_socket(&client_socket, Duration::from_secs(10));

let mut thin_client = spawn_client_process(&config_home, &runtime_dir, &api_socket);
read_until_client_attaches(&thin_client);

// Freeze the client so the dead terminal and transport EOF are both
// observable when it resumes, making the `--remote` shutdown race deterministic.
let client_pid = thin_client.child.process_id().expect("thin client pid") as libc::pid_t;
assert_eq!(
unsafe { libc::kill(client_pid, libc::SIGSTOP) },
0,
"stop thin client"
);
let server_pid = spawned_server.child.process_id().expect("server pid") as libc::pid_t;
assert_eq!(
unsafe { libc::kill(server_pid, libc::SIGKILL) },
0,
"kill server transport"
);
spawned_server.close_master();
thin_client.close_master();
assert_eq!(
unsafe { libc::kill(client_pid, libc::SIGCONT) },
0,
"resume thin client"
);

let deadline = Instant::now() + Duration::from_secs(12);
let status = loop {
if let Some(status) = thin_client.child.try_wait().expect("poll thin client") {
break Some(status);
}
if Instant::now() >= deadline {
break None;
}
thread::sleep(Duration::from_millis(20));
};

drop(spawned_server);
cleanup_spawned_herdr(thin_client, base);

let status = status.expect("thin client should exit after terminal and transport hang up");
assert!(
status.success(),
"thin client should exit cleanly after terminal and transport hang up, got {status}"
);
}

#[test]
fn client_exits_cleanly_when_terminal_hangs_up() {
let _lock = test_lock();
Expand Down
Loading