mirror of
https://github.com/openai/codex.git
synced 2026-10-29 19:08:43 +00:00
Improve TUI reconnect recovery and offline exit handling (#57820)
## What changed - Allow `/quit` and `/exit` while offline, exiting without waiting for server shutdown. - Retry `thread/resume` when the server reports `serverShuttingDown` instead of treating the conversation as unavailable. - Report which reconnect stage timed out, including waiting for server shutdown, without exposing transport errors that may contain endpoint credentials. ## Testing Extend regression coverage for offline quit commands and stage-specific timeout messages. Update the reconnect integration test to exercise a draining server and verify that local approval and sandbox settings are preserved. GitOrigin-RevId: 4f6ed33db809f56ea09fc9423f3ffe28f22b7cc5
This commit is contained in:
@@ -50,6 +50,9 @@ impl App {
|
||||
if matches!(event, AppEvent::ForkCurrentSession { .. }) {
|
||||
self.chat_widget.fork_in_progress = false;
|
||||
}
|
||||
if self.reconnect.offline && matches!(event, AppEvent::Exit(ExitMode::ShutdownFirst)) {
|
||||
return Ok(AppRunControl::Exit(ExitReason::UserRequested));
|
||||
}
|
||||
if self.reconnect.offline
|
||||
&& !matches!(
|
||||
&event,
|
||||
|
||||
@@ -4,6 +4,7 @@
|
||||
|
||||
use super::*;
|
||||
use crate::app_server_session::ResumeModelSettings;
|
||||
use crate::app_server_session::is_server_shutting_down_error;
|
||||
use crate::dynamic_tools_mcp::ThreadToolTransport;
|
||||
|
||||
#[derive(Clone, Copy, Default, PartialEq, Eq)]
|
||||
@@ -62,16 +63,21 @@ pub(super) async fn reconnect(
|
||||
tokio::time::Instant::now() + Duration::from_secs(/*secs*/ 120)
|
||||
}
|
||||
};
|
||||
// Track only the stage. Transport errors can contain endpoint credentials.
|
||||
let mut stage = "connecting to the server";
|
||||
for delay in [0, 1, 2, 4].into_iter().chain(std::iter::repeat(/*elt*/ 8)) {
|
||||
let attempt = async {
|
||||
tokio::time::sleep(Duration::from_secs(delay)).await;
|
||||
stage = "connecting to the server";
|
||||
let client = crate::app_server_connection::connect(&target).await?;
|
||||
let mut session = AppServerSession::new(backend_id, client, mode)
|
||||
.with_local_codex_home(&config.codex_home)
|
||||
.with_remote_cwd_override(remote_cwd.clone())
|
||||
.with_thread_tool_transport(task_tools.clone());
|
||||
stage = "loading session settings";
|
||||
let bootstrap = session.bootstrap(&config).await?;
|
||||
let thread = if let Some(thread_id) = thread_id {
|
||||
stage = "resuming the conversation";
|
||||
match session
|
||||
.resume_thread(
|
||||
&local_settings,
|
||||
@@ -90,14 +96,14 @@ pub(super) async fn reconnect(
|
||||
{
|
||||
return Err(error);
|
||||
}
|
||||
// Unloading threads use the same code as unavailable conversations, but
|
||||
// ordinary resume can reattach once the unload finishes.
|
||||
// Draining servers and unloading threads both reject resume temporarily.
|
||||
Err(error)
|
||||
if matches!(
|
||||
error.downcast_ref::<TypedRequestError>(),
|
||||
Some(TypedRequestError::Server { method, source })
|
||||
if method == "thread/resume" && source.code == -32600
|
||||
&& source.message.starts_with(&format!("thread {thread_id} is closing;"))
|
||||
&& (is_server_shutting_down_error(source)
|
||||
|| source.message.starts_with(&format!("thread {thread_id} is closing;")))
|
||||
) =>
|
||||
{
|
||||
return Err(error);
|
||||
@@ -127,9 +133,9 @@ pub(super) async fn reconnect(
|
||||
Ok(Err(_)) => {}
|
||||
Err(_) => break,
|
||||
}
|
||||
// Transport errors can contain endpoint credentials. Do not render or log them.
|
||||
}
|
||||
color_eyre::eyre::bail!("Server connection could not be restored")
|
||||
tracing::warn!(stage, "automatic reconnect timed out");
|
||||
color_eyre::eyre::bail!("Automatic reconnect timed out while {stage}.")
|
||||
}
|
||||
|
||||
impl App {
|
||||
|
||||
@@ -1383,7 +1383,9 @@ See the Codex keymap documentation for supported actions and examples."
|
||||
&& wait_for_drain
|
||||
{
|
||||
tokio::time::sleep_until(deadline).await;
|
||||
color_eyre::eyre::bail!("Server connection could not be restored");
|
||||
color_eyre::eyre::bail!(
|
||||
"Automatic reconnect timed out waiting for the server to finish shutting down."
|
||||
);
|
||||
}
|
||||
attempt.await
|
||||
}));
|
||||
|
||||
@@ -642,28 +642,32 @@ async fn reconnect_exhaustion_and_unknown_initial_thread_stay_offline() -> Resul
|
||||
drop(listener);
|
||||
tokio::time::pause();
|
||||
let start = tokio::time::Instant::now();
|
||||
for id in [Some(ThreadId::new()), None] {
|
||||
assert!(
|
||||
reconnect(
|
||||
crate::backend_id::BackendId::new(),
|
||||
app.app_server_target.clone(),
|
||||
app.config.clone(),
|
||||
app.local_settings.clone(),
|
||||
id,
|
||||
/*remote_cwd*/ None,
|
||||
crate::dynamic_tools_mcp::ThreadToolTransport::Dynamic,
|
||||
ReconnectPresentation::Conversation
|
||||
)
|
||||
.await
|
||||
.is_err()
|
||||
);
|
||||
}
|
||||
let reconnect_thread = |id| {
|
||||
reconnect(
|
||||
crate::backend_id::BackendId::new(),
|
||||
app.app_server_target.clone(),
|
||||
app.config.clone(),
|
||||
app.local_settings.clone(),
|
||||
id,
|
||||
/*remote_cwd*/ None,
|
||||
crate::dynamic_tools_mcp::ThreadToolTransport::Dynamic,
|
||||
ReconnectPresentation::Conversation,
|
||||
)
|
||||
};
|
||||
assert!(reconnect_thread(None).await.is_err());
|
||||
let error = reconnect_thread(Some(ThreadId::new()))
|
||||
.await
|
||||
.err()
|
||||
.expect("reconnect should time out");
|
||||
assert_eq!(start.elapsed().as_secs(), 120);
|
||||
app.begin_reconnect();
|
||||
app.chat_widget.reconnect_failed();
|
||||
assert_snapshot!(
|
||||
"reconnect_failed",
|
||||
render_bottom_popup(&app.chat_widget, /*width*/ 80)
|
||||
format!(
|
||||
"{error}\n{}",
|
||||
render_bottom_popup(&app.chat_widget, /*width*/ 80)
|
||||
)
|
||||
);
|
||||
tokio::time::resume();
|
||||
Ok(())
|
||||
@@ -768,7 +772,10 @@ async fn reconnect_allows_slow_hydration_but_bounds_a_stalled_server() -> Result
|
||||
);
|
||||
} else {
|
||||
tokio::time::resume();
|
||||
assert!(result.is_err());
|
||||
assert_eq!(
|
||||
result.err().expect("resume should time out").to_string(),
|
||||
"Automatic reconnect timed out while resuming the conversation."
|
||||
);
|
||||
assert_eq!(elapsed, 120);
|
||||
server.abort();
|
||||
}
|
||||
|
||||
@@ -191,6 +191,19 @@ async fn lost_mutation_reply_preserves_work_without_resubmitting() -> Result<()>
|
||||
AppRunControl::Exit(ExitReason::UserRequested)
|
||||
));
|
||||
}
|
||||
for command in ["/exit", "/quit"] {
|
||||
app.chat_widget
|
||||
.restore_user_message_to_composer(command.into());
|
||||
app.handle_tui_event(&mut tui, &mut session, TuiEvent::Key(KeyCode::Enter.into()))
|
||||
.await?;
|
||||
let exit = std::iter::from_fn(|| events.try_recv().ok())
|
||||
.find(|event| matches!(event, AppEvent::Exit(_)))
|
||||
.expect("quit command should request exit");
|
||||
assert!(matches!(
|
||||
app.handle_event(&mut tui, &mut session, exit).await?,
|
||||
AppRunControl::Exit(ExitReason::UserRequested)
|
||||
));
|
||||
}
|
||||
session.shutdown().await?;
|
||||
assert_eq!(server.await??, vec!["initialize", "turn/start"]);
|
||||
Ok(())
|
||||
|
||||
+2
-1
@@ -1,7 +1,8 @@
|
||||
---
|
||||
source: tui/src/app/tests/active_reconnect_tests.rs
|
||||
expression: "render_bottom_popup(&app.chat_widget, 80)"
|
||||
expression: "format!(\"{error}\\n{}\", render_bottom_popup(&app.chat_widget, 80))"
|
||||
---
|
||||
Automatic reconnect timed out while connecting to the server.
|
||||
• Reconnect failed — check the endpoint, then relaunch (0s)
|
||||
|
||||
|
||||
|
||||
@@ -173,7 +173,7 @@
|
||||
//!
|
||||
//! During reconnection, `handle_restricted_key` edits the draft directly without popup dispatch,
|
||||
//! composer shortcuts, or submission; `?` becomes literal input. Enter and Tab leave the draft
|
||||
//! intact until reconnection succeeds, except for the local `/warnings` command.
|
||||
//! intact until reconnection succeeds, except for `/warnings`, `/config`, `/quit`, and `/exit`.
|
||||
//! When connected but the thread is unavailable, configured
|
||||
//! submit keys may dispatch explicitly allowed recovery and local commands; other drafts stay put.
|
||||
//! Empty-prompt agents navigation remains available with its usual editor and draft guards.
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
//! Restricted editing retains drafts and allows the local warnings viewer.
|
||||
//! Restricted editing retains drafts and allows local warnings, configuration, and quit commands.
|
||||
//! Unavailable threads also allow recovery and other local commands.
|
||||
//! Connected unavailable threads retain the empty-prompt agents shortcut.
|
||||
//! Paste Enter handling is shared with normal submission so buffered newlines survive both paths.
|
||||
@@ -89,8 +89,7 @@ impl ChatComposer {
|
||||
.or_else(|| input.inline_command(text).map(|command| command.command))
|
||||
.filter(|_| text.trim().lines().count() == 1);
|
||||
if matches!(command, Some(SlashCommandItem::Builtin(command))
|
||||
if command == SlashCommand::Warnings
|
||||
|| command == SlashCommand::Config
|
||||
if matches!(command, SlashCommand::Warnings | SlashCommand::Config | SlashCommand::Quit | SlashCommand::Exit)
|
||||
|| mode == RestrictedInputMode::UnavailableThread && command.available_when_thread_unavailable())
|
||||
{
|
||||
return self
|
||||
|
||||
@@ -95,7 +95,8 @@ async fn automatic_reconnect_restores_draft_and_routes_new_notifications() -> Re
|
||||
.send(Message::Text(
|
||||
json!({"id": request.id, "error": {
|
||||
"code": -32600,
|
||||
"message": format!("thread {id} is closing; retry after the thread is closed")
|
||||
"message": "Server is draining; retry after reconnecting",
|
||||
"data": {"reason": "serverShuttingDown"}
|
||||
}})
|
||||
.to_string()
|
||||
.into(),
|
||||
@@ -140,8 +141,8 @@ async fn automatic_reconnect_restores_draft_and_routes_new_notifications() -> Re
|
||||
);
|
||||
}
|
||||
json!({"thread": thread, "model": "gpt-5.6-terra", "modelProvider": "openai",
|
||||
"cwd": server_cwd, "approvalPolicy": if starting { "never" } else { "on-request" }, "approvalsReviewer": "user",
|
||||
"sandbox": {"type": if starting { "dangerFullAccess" } else { "readOnly" }}, "reasoningEffort": null})
|
||||
"cwd": server_cwd, "approvalPolicy": if starting { "on-request" } else { "never" }, "approvalsReviewer": "user",
|
||||
"sandbox": {"type": if starting { "readOnly" } else { "workspaceWrite" }}, "reasoningEffort": null})
|
||||
}
|
||||
"turn/start" => {
|
||||
let params = request.params.as_ref().unwrap();
|
||||
@@ -152,7 +153,7 @@ async fn automatic_reconnect_restores_draft_and_routes_new_notifications() -> Re
|
||||
params["sandboxPolicy"],
|
||||
params["permissions"]
|
||||
]),
|
||||
json!(["never", {"type": "dangerFullAccess"}, null])
|
||||
json!(["on-request", {"type": "readOnly", "networkAccess": false}, null])
|
||||
);
|
||||
json!({"turn": {"id": "fresh", "items": [], "status": "inProgress", "error": null}})
|
||||
}
|
||||
@@ -202,8 +203,17 @@ async fn automatic_reconnect_restores_draft_and_routes_new_notifications() -> Re
|
||||
}
|
||||
Ok::<_, anyhow::Error>(methods)
|
||||
});
|
||||
let mut terminal =
|
||||
PtyCodex::start(&workspace_path, codex_home, &["--yolo", "--no-alt-screen"])?;
|
||||
let mut terminal = PtyCodex::start(
|
||||
&workspace_path,
|
||||
codex_home,
|
||||
&[
|
||||
"--sandbox",
|
||||
"read-only",
|
||||
"--ask-for-approval",
|
||||
"on-request",
|
||||
"--no-alt-screen",
|
||||
],
|
||||
)?;
|
||||
terminal.wait_for_startup()?;
|
||||
let mut disconnect_tx = Some(disconnect_tx);
|
||||
let mut restore_tx = Some(restore_tx);
|
||||
|
||||
Reference in New Issue
Block a user