mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Avoid blocking TUI on agent metadata hydration (#21870)
## Why Fixes #16688. The TUI currently hydrates collab receiver metadata by awaiting `thread/read` before each active-thread notification is rendered. During large subagent fan-outs, the embedded app-server can be busy starting agents and processing spawn work, so those synchronous metadata reads queue behind the fan-out and block the TUI event loop. That makes the UI appear frozen even though the underlying agent work can continue. ## What Changed - Replaced eager `thread/read` metadata hydration on the active notification path with local receiver-thread caching. - Kept `ThreadStarted` and picker refreshes as the places that fill in agent nickname/role metadata when it is available. - Skipped caching receiver threads that are explicitly reported as `NotFound`, avoiding live-looking ghost entries for failed stale-agent calls. - Added TUI tests covering both local receiver caching and `NotFound` suppression. ## Verification - `cargo test -p codex-tui collab_receiver_notification` - `just fix -p codex-tui` I also ran the full `cargo test -p codex-tui`; the new test passed, but the full process later aborted with an unrelated stack overflow in `tests::fork_last_filters_latest_session_by_cwd_unless_show_all`.
This commit is contained in:
committed by
GitHub
Unverified
parent
6d747db7d8
commit
90c0bec50c
@@ -247,6 +247,26 @@ fn collab_receiver_thread_ids(notification: &ServerNotification) -> Option<&[Str
|
||||
}
|
||||
}
|
||||
|
||||
fn collab_receiver_is_not_found(
|
||||
notification: &ServerNotification,
|
||||
receiver_thread_id: &str,
|
||||
) -> bool {
|
||||
match notification {
|
||||
ServerNotification::ItemCompleted(notification) => match ¬ification.item {
|
||||
ThreadItem::CollabAgentToolCall { agents_states, .. } => {
|
||||
agents_states.get(receiver_thread_id).is_some_and(|state| {
|
||||
matches!(
|
||||
&state.status,
|
||||
codex_app_server_protocol::CollabAgentStatus::NotFound
|
||||
)
|
||||
})
|
||||
}
|
||||
_ => false,
|
||||
},
|
||||
_ => false,
|
||||
}
|
||||
}
|
||||
|
||||
fn default_exec_approval_decisions(
|
||||
network_approval_context: Option<&codex_app_server_protocol::NetworkApprovalContext>,
|
||||
proposed_execpolicy_amendment: Option<&codex_app_server_protocol::ExecPolicyAmendment>,
|
||||
|
||||
@@ -1249,6 +1249,75 @@ async fn token_usage_update_refreshes_status_line_with_runtime_context_window()
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn collab_receiver_notification_caches_thread_without_app_server_read() {
|
||||
let mut app = make_test_app().await;
|
||||
let receiver_thread_id =
|
||||
ThreadId::from_string("00000000-0000-0000-0000-000000000123").expect("valid thread id");
|
||||
|
||||
app.handle_thread_event_now(ThreadBufferedEvent::Notification(
|
||||
ServerNotification::ItemStarted(ItemStartedNotification {
|
||||
thread_id: ThreadId::new().to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
started_at_ms: 0,
|
||||
item: ThreadItem::CollabAgentToolCall {
|
||||
id: "wait-1".to_string(),
|
||||
tool: codex_app_server_protocol::CollabAgentTool::Wait,
|
||||
status: codex_app_server_protocol::CollabAgentToolCallStatus::InProgress,
|
||||
sender_thread_id: ThreadId::new().to_string(),
|
||||
receiver_thread_ids: vec![receiver_thread_id.to_string()],
|
||||
prompt: None,
|
||||
model: None,
|
||||
reasoning_effort: None,
|
||||
agents_states: HashMap::new(),
|
||||
},
|
||||
}),
|
||||
));
|
||||
|
||||
assert_eq!(
|
||||
app.agent_navigation.get(&receiver_thread_id),
|
||||
Some(&AgentPickerThreadEntry {
|
||||
agent_nickname: None,
|
||||
agent_role: None,
|
||||
is_closed: false,
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn collab_receiver_notification_does_not_cache_not_found_thread() {
|
||||
let mut app = make_test_app().await;
|
||||
let receiver_thread_id =
|
||||
ThreadId::from_string("00000000-0000-0000-0000-000000000124").expect("valid thread id");
|
||||
|
||||
app.handle_thread_event_now(ThreadBufferedEvent::Notification(
|
||||
ServerNotification::ItemCompleted(codex_app_server_protocol::ItemCompletedNotification {
|
||||
thread_id: ThreadId::new().to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
completed_at_ms: 0,
|
||||
item: ThreadItem::CollabAgentToolCall {
|
||||
id: "send-1".to_string(),
|
||||
tool: codex_app_server_protocol::CollabAgentTool::SendInput,
|
||||
status: codex_app_server_protocol::CollabAgentToolCallStatus::Failed,
|
||||
sender_thread_id: ThreadId::new().to_string(),
|
||||
receiver_thread_ids: vec![receiver_thread_id.to_string()],
|
||||
prompt: Some("hello".to_string()),
|
||||
model: None,
|
||||
reasoning_effort: None,
|
||||
agents_states: HashMap::from([(
|
||||
receiver_thread_id.to_string(),
|
||||
codex_app_server_protocol::CollabAgentState {
|
||||
status: codex_app_server_protocol::CollabAgentStatus::NotFound,
|
||||
message: None,
|
||||
},
|
||||
)]),
|
||||
},
|
||||
}),
|
||||
));
|
||||
|
||||
assert_eq!(app.agent_navigation.get(&receiver_thread_id), None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn open_agent_picker_keeps_missing_threads_for_replay() -> Result<()> {
|
||||
let mut app = Box::pin(make_test_app()).await;
|
||||
|
||||
@@ -842,20 +842,14 @@ impl App {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Eagerly fetches nickname and role for receiver threads referenced by a collab notification.
|
||||
/// Locally remembers receiver threads referenced by a collab notification.
|
||||
///
|
||||
/// This runs on every buffered thread notification before it reaches rendering. For each
|
||||
/// receiver thread id that the navigation cache does not yet have metadata for, it issues a
|
||||
/// `thread/read` RPC and registers the result in both `AgentNavigationState` and the
|
||||
/// `ChatWidget` metadata map. Threads that already have a nickname or role cached are skipped,
|
||||
/// so the cost is at most one RPC per thread over the lifetime of a session.
|
||||
///
|
||||
/// Failures are logged and silently ignored -- the worst outcome is that a rendered item shows
|
||||
/// a thread id instead of a human-readable name, which is the same behavior the TUI had before
|
||||
/// this change.
|
||||
pub(super) async fn hydrate_collab_agent_metadata_for_notification(
|
||||
/// This intentionally avoids app-server reads on the active-thread rendering path. During large
|
||||
/// fan-outs the app-server can be saturated with spawn work, and blocking here would freeze the
|
||||
/// TUI event loop. Metadata from `ThreadStarted` or explicit picker refreshes still fills in
|
||||
/// names and roles later; until then, rendering falls back to the thread id.
|
||||
pub(super) fn cache_collab_receiver_threads_for_notification(
|
||||
&mut self,
|
||||
app_server: &mut AppServerSession,
|
||||
notification: &ServerNotification,
|
||||
) {
|
||||
let Some(receiver_thread_ids) = collab_receiver_thread_ids(notification) else {
|
||||
@@ -863,42 +857,26 @@ impl App {
|
||||
};
|
||||
|
||||
for receiver_thread_id in receiver_thread_ids {
|
||||
if collab_receiver_is_not_found(notification, receiver_thread_id) {
|
||||
continue;
|
||||
}
|
||||
|
||||
let Ok(thread_id) = ThreadId::from_string(receiver_thread_id) else {
|
||||
tracing::warn!(
|
||||
thread_id = receiver_thread_id,
|
||||
"ignoring collab receiver with invalid thread id during metadata hydration"
|
||||
"ignoring collab receiver with invalid thread id during local caching"
|
||||
);
|
||||
continue;
|
||||
};
|
||||
|
||||
if self
|
||||
.agent_navigation
|
||||
.get(&thread_id)
|
||||
.is_some_and(|entry| entry.agent_nickname.is_some() || entry.agent_role.is_some())
|
||||
{
|
||||
if self.agent_navigation.get(&thread_id).is_some() {
|
||||
continue;
|
||||
}
|
||||
|
||||
match app_server
|
||||
.thread_read(thread_id, /*include_turns*/ false)
|
||||
.await
|
||||
{
|
||||
Ok(thread) => {
|
||||
self.upsert_agent_picker_thread(
|
||||
thread_id,
|
||||
thread.agent_nickname,
|
||||
thread.agent_role,
|
||||
/*is_closed*/ false,
|
||||
);
|
||||
}
|
||||
Err(err) => {
|
||||
tracing::warn!(
|
||||
thread_id = %thread_id,
|
||||
error = %err,
|
||||
"failed to hydrate collab receiver thread metadata"
|
||||
);
|
||||
}
|
||||
}
|
||||
self.upsert_agent_picker_thread(
|
||||
thread_id, /*agent_nickname*/ None, /*agent_role*/ None,
|
||||
/*is_closed*/ false,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1365,6 +1343,7 @@ impl App {
|
||||
);
|
||||
match event {
|
||||
ThreadBufferedEvent::Notification(notification) => {
|
||||
self.cache_collab_receiver_threads_for_notification(¬ification);
|
||||
self.chat_widget
|
||||
.handle_server_notification(notification, /*replay_kind*/ None);
|
||||
}
|
||||
@@ -1467,11 +1446,6 @@ impl App {
|
||||
// thread, so unrelated shutdowns cannot consume this marker.
|
||||
self.pending_shutdown_exit_thread_id = None;
|
||||
}
|
||||
if let ThreadBufferedEvent::Notification(notification) = &event {
|
||||
self.hydrate_collab_agent_metadata_for_notification(app_server, notification)
|
||||
.await;
|
||||
}
|
||||
|
||||
self.handle_thread_event_now(event);
|
||||
if self.backtrack_render_pending {
|
||||
tui.frame_requester().schedule_frame();
|
||||
|
||||
Reference in New Issue
Block a user