mirror of
https://github.com/openai/codex.git
synced 2026-10-20 08:19:50 +00:00
Defer resume picker and directory changes to a fresh TUI stack (#43376)
## Why Session transitions need stack headroom for embedded-server initialization and configuration loading. Running them inside event dispatch risks exhausting the TUI thread's stack. ## What changed - Queue `/resume` and valid `/cd` requests for processing after event dispatch returns. - Reject additional `/cd` requests while one is pending, and recheck the source session, working directory, idle state, and remote-environment restrictions before applying it. ## Testing Add a Unix PTY regression test that exercises `/resume`, worktree conversation forks, `/cd`, and new worktree conversations through the real TUI event loop. Update the directory-change test to verify that dispatch queues the request without immediately changing configuration. GitOrigin-RevId: 4950921931dfaba9e55e6bb04f98f7ba64b72122
This commit is contained in:
committed by
copyberry
parent
694b6319d3
commit
5ecb3afd1b
@@ -636,6 +636,10 @@ pub(crate) struct App {
|
||||
tokio::sync::broadcast::Sender<codex_app_server_protocol::ThreadStatusChangedNotification>,
|
||||
dynamic_tool_tasks: HashMap<codex_app_server_protocol::RequestId, (String, JoinHandle<()>)>,
|
||||
pending_startup_thread_start: bool,
|
||||
/// Opens the session picker after event dispatch returns, with a fresh stack.
|
||||
pending_open_resume_picker: bool,
|
||||
/// Runs a requested /cd after event dispatch returns, with a fresh stack.
|
||||
pending_working_directory_change: Option<working_directory::PendingWorkingDirectoryChange>,
|
||||
/// Starts worktree setup after the event handler returns, with a fresh stack.
|
||||
pending_start_managed_worktree: Option<(crate::app_event::ManagedWorktreeMode, Option<String>)>,
|
||||
pending_managed_worktree_creation: bool,
|
||||
|
||||
@@ -126,7 +126,8 @@ impl App {
|
||||
thread_id,
|
||||
requested_cwd,
|
||||
} => {
|
||||
if self.primary_thread_id != Some(thread_id)
|
||||
if self.pending_working_directory_change.is_some()
|
||||
|| self.primary_thread_id != Some(thread_id)
|
||||
|| !self.chat_widget.can_change_working_directory(thread_id)
|
||||
{
|
||||
self.chat_widget.add_error_message(
|
||||
@@ -148,7 +149,12 @@ impl App {
|
||||
);
|
||||
match std::fs::metadata(cwd.as_path()) {
|
||||
Ok(metadata) if metadata.is_dir() => {
|
||||
self.change_working_directory(tui, app_server, cwd).await;
|
||||
self.pending_working_directory_change =
|
||||
Some(working_directory::PendingWorkingDirectoryChange {
|
||||
source_thread_id: thread_id,
|
||||
source_cwd: self.config.cwd.clone(),
|
||||
destination: cwd,
|
||||
});
|
||||
}
|
||||
Ok(_) => self
|
||||
.chat_widget
|
||||
@@ -278,7 +284,7 @@ impl App {
|
||||
.await;
|
||||
}
|
||||
AppEvent::OpenResumePicker => {
|
||||
return Box::pin(self.open_resume_picker(tui, app_server)).await;
|
||||
self.pending_open_resume_picker = true;
|
||||
}
|
||||
AppEvent::OpenExternalAgentConfigMigration => {
|
||||
match crate::external_agent_config_migration::flow::handle_external_agent_config_migration_prompt(
|
||||
|
||||
@@ -9,8 +9,8 @@ impl App {
|
||||
tui: &mut tui::Tui,
|
||||
app_server: &mut AppServerSession,
|
||||
) -> Result<AppRunControl> {
|
||||
// This runs inside the active TUI event future. Starting a second embedded server
|
||||
// inline can exhaust that thread's stack while initializing its auth manager.
|
||||
// Keep embedded-server initialization on its own runtime task as well as opening the
|
||||
// picker on a fresh event-loop iteration; its auth manager needs more stack headroom.
|
||||
let picker_config = self.config.clone();
|
||||
let picker_target = self.app_server_target.clone();
|
||||
let picker_state_db = self.state_db.clone();
|
||||
|
||||
@@ -724,6 +724,8 @@ See the Codex keymap documentation for supported actions and examples."
|
||||
dynamic_tool_status_updates,
|
||||
dynamic_tool_tasks: HashMap::new(),
|
||||
pending_startup_thread_start,
|
||||
pending_open_resume_picker: false,
|
||||
pending_working_directory_change: None,
|
||||
pending_start_managed_worktree: None,
|
||||
pending_managed_worktree_creation: false,
|
||||
pending_managed_worktree_created: None,
|
||||
@@ -921,6 +923,21 @@ See the Codex keymap documentation for supported actions and examples."
|
||||
Ok(exit_reason)
|
||||
} else {
|
||||
loop {
|
||||
if app.pending_open_resume_picker {
|
||||
app.pending_open_resume_picker = false;
|
||||
match Box::pin(app.open_resume_picker(tui, &mut app_server)).await {
|
||||
Ok(AppRunControl::Continue) => {}
|
||||
Ok(AppRunControl::Exit(reason)) => break Ok(reason),
|
||||
Err(err) if app.recover_transport_error(&err) => {}
|
||||
Err(err) => break Err(err),
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if let Some(pending) = app.pending_working_directory_change.take() {
|
||||
Box::pin(app.finish_working_directory_change(tui, &mut app_server, pending))
|
||||
.await;
|
||||
continue;
|
||||
}
|
||||
if let Some((mode, name)) = app.pending_start_managed_worktree.take() {
|
||||
Box::pin(app.start_managed_worktree(&mut app_server, mode, name)).await;
|
||||
continue;
|
||||
|
||||
@@ -81,6 +81,8 @@ pub(super) async fn make_test_app() -> App {
|
||||
dynamic_tool_status_updates: tokio::sync::broadcast::channel(/*capacity*/ 64).0,
|
||||
dynamic_tool_tasks: HashMap::new(),
|
||||
pending_startup_thread_start: false,
|
||||
pending_open_resume_picker: false,
|
||||
pending_working_directory_change: None,
|
||||
pending_start_managed_worktree: None,
|
||||
pending_managed_worktree_creation: false,
|
||||
pending_managed_worktree_created: None,
|
||||
|
||||
@@ -5854,7 +5854,9 @@ async fn make_test_app() -> App {
|
||||
dynamic_tool_status_updates: tokio::sync::broadcast::channel(/*capacity*/ 64).0,
|
||||
dynamic_tool_tasks: HashMap::new(),
|
||||
pending_startup_thread_start: false,
|
||||
pending_open_resume_picker: false,
|
||||
pending_managed_worktree_creation: false,
|
||||
pending_working_directory_change: None,
|
||||
pending_start_managed_worktree: None,
|
||||
pending_managed_worktree_created: None,
|
||||
pending_managed_worktree_transition: None,
|
||||
@@ -5946,7 +5948,9 @@ pub(super) async fn make_test_app_with_channels() -> (
|
||||
dynamic_tool_status_updates: tokio::sync::broadcast::channel(/*capacity*/ 64).0,
|
||||
dynamic_tool_tasks: HashMap::new(),
|
||||
pending_startup_thread_start: false,
|
||||
pending_open_resume_picker: false,
|
||||
pending_managed_worktree_creation: false,
|
||||
pending_working_directory_change: None,
|
||||
pending_start_managed_worktree: None,
|
||||
pending_managed_worktree_created: None,
|
||||
pending_managed_worktree_transition: None,
|
||||
|
||||
@@ -3446,6 +3446,9 @@ async fn changing_directory_preserves_project_trust_permissions_history_and_hook
|
||||
let thread_id = [original, ThreadId::new()][usize::from(kind == "stale")];
|
||||
app.handle_event(&mut tui, &mut server, change(thread_id, path))
|
||||
.await?;
|
||||
if let Some(pending) = app.pending_working_directory_change.take() {
|
||||
Box::pin(app.finish_working_directory_change(&mut tui, &mut server, pending)).await;
|
||||
}
|
||||
assert_eq!(app.chat_widget.thread_id(), Some(original));
|
||||
assert_eq!(app.config.cwd, current.clone().abs());
|
||||
assert!(app.runtime_working_directory_override.is_none());
|
||||
@@ -3526,6 +3529,13 @@ async fn changing_directory_preserves_project_trust_permissions_history_and_hook
|
||||
requests.lock().expect("request recorder lock").clear();
|
||||
app.handle_event(&mut tui, &mut server, change(original, "../trusted"))
|
||||
.await?;
|
||||
// Dispatch only queues /cd: configuration loading happens on a fresh loop iteration.
|
||||
assert_eq!(app.config.cwd, current.clone().abs());
|
||||
let pending = app
|
||||
.pending_working_directory_change
|
||||
.take()
|
||||
.expect("valid /cd queued");
|
||||
Box::pin(app.finish_working_directory_change(&mut tui, &mut server, pending)).await;
|
||||
let forked = app.chat_widget.thread_id().expect("forked thread");
|
||||
assert_ne!(forked, original);
|
||||
let forked_rollout = app.chat_widget.rollout_path().expect("forked rollout");
|
||||
|
||||
@@ -25,7 +25,42 @@ pub(super) struct ManagedWorktreeAttach {
|
||||
name_error: Option<String>,
|
||||
}
|
||||
|
||||
/// A /cd request awaiting a fresh event-loop iteration.
|
||||
pub(super) struct PendingWorkingDirectoryChange {
|
||||
pub(super) source_thread_id: ThreadId,
|
||||
pub(super) source_cwd: AbsolutePathBuf,
|
||||
pub(super) destination: AbsolutePathBuf,
|
||||
}
|
||||
|
||||
impl App {
|
||||
pub(super) async fn finish_working_directory_change(
|
||||
&mut self,
|
||||
tui: &mut tui::Tui,
|
||||
app_server: &mut AppServerSession,
|
||||
pending: PendingWorkingDirectoryChange,
|
||||
) {
|
||||
if self.primary_thread_id != Some(pending.source_thread_id)
|
||||
|| self.config.cwd != pending.source_cwd
|
||||
|| !self
|
||||
.chat_widget
|
||||
.can_change_working_directory(pending.source_thread_id)
|
||||
{
|
||||
return self.working_directory_error(
|
||||
"Changing directories requires an idle primary session without queued input.",
|
||||
);
|
||||
}
|
||||
if crate::uses_remote_workspace_or_environment(
|
||||
&self.app_server_target,
|
||||
self.environment_manager.as_ref(),
|
||||
) {
|
||||
return self.working_directory_error(
|
||||
"Changing directories is not supported for remote workspaces or remote execution environments.",
|
||||
);
|
||||
}
|
||||
self.change_working_directory(tui, app_server, pending.destination)
|
||||
.await;
|
||||
}
|
||||
|
||||
pub(super) fn working_directory_error(&mut self, message: impl Into<String>) {
|
||||
self.chat_widget.add_error_message(message.into());
|
||||
}
|
||||
|
||||
@@ -344,6 +344,29 @@ impl PtyCodex {
|
||||
pub(super) fn screen_contents(&self) -> String {
|
||||
self.parser.screen().contents()
|
||||
}
|
||||
|
||||
pub(super) fn wait_for_screen(&mut self, text: &str) -> Result<()> {
|
||||
let deadline = Instant::now() + STARTUP_TIMEOUT;
|
||||
while Instant::now() < deadline {
|
||||
self.read_output(Duration::from_millis(/*millis*/ 50))?;
|
||||
if self.screen_contains(text) {
|
||||
return Ok(());
|
||||
}
|
||||
if let Some(status) = self.child.try_wait()? {
|
||||
bail!("Codex exited while waiting for {text:?} ({status})");
|
||||
}
|
||||
}
|
||||
bail!("missing {text:?}; screen:\n{}", self.screen_contents())
|
||||
}
|
||||
|
||||
pub(super) fn ensure_running(&mut self) -> Result<()> {
|
||||
ensure!(
|
||||
self.child.try_wait()?.is_none(),
|
||||
"Codex exited unexpectedly; screen:\n{}",
|
||||
self.screen_contents()
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
impl Drop for PtyCodex {
|
||||
|
||||
@@ -7,3 +7,5 @@ mod resize_reflow;
|
||||
mod status_indicator;
|
||||
mod vt100_history;
|
||||
mod vt100_live_commit;
|
||||
#[cfg(unix)]
|
||||
mod worktree_stack;
|
||||
|
||||
@@ -0,0 +1,175 @@
|
||||
//! Exercises stack-sensitive session transitions through the real TUI event loop.
|
||||
|
||||
use super::focus_palette::PtyCodex;
|
||||
use super::focus_palette::write_test_config;
|
||||
use anyhow::Result;
|
||||
use anyhow::ensure;
|
||||
use codex_worktree::ManagedWorktree;
|
||||
use codex_worktree::WorktreeManager;
|
||||
use codex_worktree::WorktreeSettings;
|
||||
use core_test_support::responses;
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
use std::process::Command;
|
||||
use std::time::Duration;
|
||||
use std::time::Instant;
|
||||
use wiremock::matchers::body_string_contains;
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn picker_worktree_fork_and_cd_run_on_the_production_stack() -> Result<()> {
|
||||
let repository = tempfile::tempdir_in("/tmp")?;
|
||||
let root = repository.path().canonicalize()?;
|
||||
let status = Command::new("git")
|
||||
.args(["init", "-q", "-b", "main"])
|
||||
.arg(&root)
|
||||
.status()?;
|
||||
ensure!(status.success(), "initialize test repository");
|
||||
fs::write(root.join("README.md"), "stack transition test\n")?;
|
||||
ensure!(
|
||||
Command::new("git")
|
||||
.args(["-C"])
|
||||
.arg(&root)
|
||||
.args(["add", "README.md"])
|
||||
.status()?
|
||||
.success(),
|
||||
"stage test repository"
|
||||
);
|
||||
ensure!(
|
||||
Command::new("git")
|
||||
.args(["-C"])
|
||||
.arg(&root)
|
||||
.args([
|
||||
"-c",
|
||||
"user.name=Codex Test",
|
||||
"-c",
|
||||
"user.email=test@example.invalid"
|
||||
])
|
||||
.args(["commit", "--no-gpg-sign", "-qm", "initial"])
|
||||
.status()?
|
||||
.success(),
|
||||
"commit test repository"
|
||||
);
|
||||
|
||||
let codex_home = tempfile::tempdir_in("/tmp")?;
|
||||
write_test_config(codex_home.path(), &root)?;
|
||||
let server = responses::start_mock_server().await;
|
||||
let config_path = codex_home.path().join("config.toml");
|
||||
let config = fs::read_to_string(&config_path)?
|
||||
.replace("model_provider = \"openai\"", "model_provider = \"test\"");
|
||||
fs::write(
|
||||
&config_path,
|
||||
format!(
|
||||
"{config}\n[model_providers.test]\nname = \"Mock\"\nbase_url = \"{}/v1\"\nwire_api = \"responses\"\nrequires_openai_auth = false\nsupports_websockets = false\n",
|
||||
server.uri()
|
||||
),
|
||||
)?;
|
||||
let _reply = responses::mount_sse_once_match(
|
||||
&server,
|
||||
body_string_contains("marker requested by this test"),
|
||||
responses::sse(vec![
|
||||
responses::ev_assistant_message("saved", "STACK_SAVED_HISTORY"),
|
||||
responses::ev_completed("saved-response"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let worktrees = WorktreeManager::new(WorktreeSettings::for_cli(
|
||||
codex_home.path(),
|
||||
/*desktop*/ None,
|
||||
)?);
|
||||
// The launcher gives codex-main and Tokio workers explicit 16 MiB stacks.
|
||||
let mut terminal = PtyCodex::start(
|
||||
&root,
|
||||
codex_home,
|
||||
&[
|
||||
"-c",
|
||||
"features.worktrees=true",
|
||||
"-c",
|
||||
"features.shell_snapshot=false",
|
||||
"Reply with the marker requested by this test.",
|
||||
],
|
||||
)?;
|
||||
terminal.wait_for_startup()?;
|
||||
terminal.wait_for_screen("STACK_SAVED_HISTORY")?;
|
||||
terminal.wait_for_screen("Ask Codex to do anything")?;
|
||||
|
||||
submit(&mut terminal, "/resume")?;
|
||||
terminal.wait_for_screen("Resume a previous session")?;
|
||||
terminal.ensure_running()?;
|
||||
terminal.write_input(b"\x1b")?;
|
||||
terminal.wait_for_screen("Ask Codex to do anything")?;
|
||||
|
||||
submit(&mut terminal, "/worktree")?;
|
||||
terminal.wait_for_screen("Continue current conversation")?;
|
||||
terminal.write_input(b"1")?;
|
||||
let forked = wait_for_owned_worktrees(&mut terminal, &worktrees, &root, /*count*/ 1)?;
|
||||
let fork_bucket = forked[0].root.parent().expect("bucket");
|
||||
terminal.wait_for_screen(&format!(
|
||||
"/{}/",
|
||||
fork_bucket.file_name().unwrap().to_string_lossy()
|
||||
))?;
|
||||
terminal.ensure_running()?;
|
||||
|
||||
submit(&mut terminal, &format!("/cd {}", root.display()))?;
|
||||
terminal.wait_for_screen(&format!("Working directory changed to: {}", root.display()))?;
|
||||
terminal.ensure_running()?;
|
||||
|
||||
submit(&mut terminal, "/worktree")?;
|
||||
terminal.wait_for_screen("Start new conversation")?;
|
||||
terminal.write_input(b"2")?;
|
||||
let created = wait_for_owned_worktrees(&mut terminal, &worktrees, &root, /*count*/ 2)?;
|
||||
ensure!(
|
||||
created
|
||||
.iter()
|
||||
.any(|checkout| checkout.root == forked[0].root)
|
||||
);
|
||||
let second = created
|
||||
.iter()
|
||||
.find(|checkout| checkout.root != forked[0].root)
|
||||
.expect("second checkout");
|
||||
ensure!(
|
||||
worktrees.owner(&forked[0].root)? != worktrees.owner(&second.root)?,
|
||||
"new worktree reused the forked thread owner"
|
||||
);
|
||||
let second_bucket = second.root.parent().expect("bucket");
|
||||
terminal.wait_for_screen(&format!(
|
||||
"/{}/",
|
||||
second_bucket.file_name().unwrap().to_string_lossy()
|
||||
))?;
|
||||
terminal.ensure_running()?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn submit(terminal: &mut PtyCodex, input: &str) -> Result<()> {
|
||||
terminal.write_input(input.as_bytes())?;
|
||||
terminal.wait_for_screen(input)?;
|
||||
// Bulk PTY writes resemble a paste; wait out the Enter suppression window.
|
||||
std::thread::sleep(Duration::from_millis(/*millis*/ 250));
|
||||
terminal.write_input(b"\r")?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn wait_for_owned_worktrees(
|
||||
terminal: &mut PtyCodex,
|
||||
manager: &WorktreeManager,
|
||||
source: &Path,
|
||||
count: usize,
|
||||
) -> Result<Vec<ManagedWorktree>> {
|
||||
let deadline = Instant::now() + Duration::from_secs(/*secs*/ 30);
|
||||
while Instant::now() < deadline {
|
||||
terminal.read_output(Duration::from_millis(/*millis*/ 50))?;
|
||||
let checkouts = manager.list(source).unwrap_or_default();
|
||||
if checkouts.len() == count
|
||||
&& checkouts
|
||||
.iter()
|
||||
.all(|checkout| manager.owner(&checkout.root).ok().flatten().is_some())
|
||||
{
|
||||
return Ok(checkouts);
|
||||
}
|
||||
terminal.ensure_running()?;
|
||||
}
|
||||
anyhow::bail!(
|
||||
"did not bind {count} managed checkouts; screen:\n{}",
|
||||
terminal.screen_contents()
|
||||
)
|
||||
}
|
||||
Reference in New Issue
Block a user