mirror of
https://github.com/openai/codex.git
synced 2026-09-05 15:18:41 +00:00
fix(core): prevent automatic turn span nesting
This commit is contained in:
@@ -5,6 +5,8 @@ use crate::goals::GoalRuntimeEvent;
|
||||
use crate::session::Codex;
|
||||
use crate::session::SessionSettingsUpdate;
|
||||
use crate::session::SteerInputError;
|
||||
#[cfg(test)]
|
||||
use crate::tasks::TurnSpanParent;
|
||||
use codex_features::Feature;
|
||||
use codex_otel::SessionTelemetry;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
@@ -414,7 +416,10 @@ impl CodexThread {
|
||||
.input_queue
|
||||
.queue_response_items_for_next_turn(items)
|
||||
.await;
|
||||
self.codex.session.maybe_start_turn_for_pending_work().await;
|
||||
self.codex
|
||||
.session
|
||||
.maybe_start_turn_for_pending_work(TurnSpanParent::Current)
|
||||
.await;
|
||||
}
|
||||
|
||||
Ok(submission_id)
|
||||
|
||||
@@ -13,6 +13,7 @@ use crate::session::turn_context::TurnContext;
|
||||
use crate::state::ActiveTurn;
|
||||
use crate::state::TurnState;
|
||||
use crate::tasks::RegularTask;
|
||||
use crate::tasks::TurnSpanParent;
|
||||
use crate::tools::handlers::goal_spec::UPDATE_GOAL_TOOL_NAME;
|
||||
use anyhow::Context;
|
||||
use codex_features::Feature;
|
||||
@@ -1268,7 +1269,8 @@ impl Session {
|
||||
}
|
||||
|
||||
async fn maybe_continue_goal_if_idle_runtime(self: &Arc<Self>) {
|
||||
self.maybe_start_turn_for_pending_work().await;
|
||||
self.maybe_start_turn_for_pending_work(TurnSpanParent::Root)
|
||||
.await;
|
||||
self.maybe_start_goal_continuation_turn().await;
|
||||
}
|
||||
|
||||
@@ -1353,8 +1355,13 @@ impl Session {
|
||||
.await;
|
||||
return;
|
||||
}
|
||||
self.start_task(turn_context, Vec::new(), RegularTask::new())
|
||||
.await;
|
||||
self.start_task(
|
||||
turn_context,
|
||||
Vec::new(),
|
||||
RegularTask::new(),
|
||||
TurnSpanParent::Root,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
async fn goal_continuation_candidate_if_active(
|
||||
|
||||
@@ -22,6 +22,7 @@ use crate::realtime_conversation::prefix_realtime_v2_text;
|
||||
use crate::review_prompts::resolve_review_request;
|
||||
use crate::session::spawn_review_thread;
|
||||
use crate::tasks::CompactTask;
|
||||
use crate::tasks::TurnSpanParent;
|
||||
use crate::tasks::UserShellCommandMode;
|
||||
use crate::tasks::UserShellCommandTask;
|
||||
use crate::tasks::execute_user_shell_command;
|
||||
@@ -316,7 +317,7 @@ pub async fn inter_agent_communication(
|
||||
.enqueue_mailbox_communication(communication)
|
||||
.await;
|
||||
if trigger_turn {
|
||||
sess.maybe_start_turn_for_pending_work_with_sub_id(sub_id)
|
||||
sess.maybe_start_turn_for_pending_work_with_sub_id(sub_id, TurnSpanParent::Current)
|
||||
.await;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -66,6 +66,7 @@ use crate::state::ActiveTurn;
|
||||
use crate::state::TaskKind;
|
||||
use crate::tasks::SessionTask;
|
||||
use crate::tasks::SessionTaskContext;
|
||||
use crate::tasks::TurnSpanParent;
|
||||
use crate::tasks::UserShellCommandMode;
|
||||
use crate::tasks::execute_user_shell_command;
|
||||
use crate::tools::ToolRouter;
|
||||
@@ -5830,6 +5831,88 @@ async fn spawn_task_turn_span_inherits_dispatch_trace_context() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn root_task_turn_span_does_not_inherit_current_trace_context() {
|
||||
struct TraceCaptureTask {
|
||||
captured_trace: Arc<std::sync::Mutex<Option<W3cTraceContext>>>,
|
||||
}
|
||||
|
||||
impl SessionTask for TraceCaptureTask {
|
||||
fn kind(&self) -> TaskKind {
|
||||
TaskKind::Regular
|
||||
}
|
||||
|
||||
fn span_name(&self) -> &'static str {
|
||||
"session_task.root_trace_capture"
|
||||
}
|
||||
|
||||
async fn run(
|
||||
self: Arc<Self>,
|
||||
_session: Arc<SessionTaskContext>,
|
||||
_ctx: Arc<TurnContext>,
|
||||
_input: Vec<TurnInput>,
|
||||
_cancellation_token: CancellationToken,
|
||||
) -> Option<String> {
|
||||
let mut trace = self
|
||||
.captured_trace
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
*trace = current_span_w3c_trace_context();
|
||||
None
|
||||
}
|
||||
}
|
||||
|
||||
let _trace_test_context = install_test_tracing("codex-core-tests");
|
||||
let request_parent = W3cTraceContext {
|
||||
traceparent: Some("00-00000000000000000000000000000011-0000000000000022-01".into()),
|
||||
tracestate: Some("vendor=value".into()),
|
||||
};
|
||||
let request_span = tracing::info_span!("completed_turn");
|
||||
assert!(set_parent_from_w3c_trace_context(
|
||||
&request_span,
|
||||
&request_parent
|
||||
));
|
||||
|
||||
let (sess, tc, rx) = make_session_and_context_with_rx().await;
|
||||
let captured_trace = Arc::new(std::sync::Mutex::new(None));
|
||||
let request_trace = async {
|
||||
let request_trace =
|
||||
current_span_w3c_trace_context().expect("request span should have trace context");
|
||||
sess.start_task(
|
||||
Arc::clone(&tc),
|
||||
Vec::new(),
|
||||
TraceCaptureTask {
|
||||
captured_trace: Arc::clone(&captured_trace),
|
||||
},
|
||||
TurnSpanParent::Root,
|
||||
)
|
||||
.await;
|
||||
request_trace
|
||||
}
|
||||
.instrument(request_span)
|
||||
.await;
|
||||
|
||||
let evt = tokio::time::timeout(StdDuration::from_secs(2), rx.recv())
|
||||
.await
|
||||
.expect("timeout waiting for turn completion")
|
||||
.expect("event");
|
||||
assert!(matches!(evt.msg, EventMsg::TurnComplete(_)));
|
||||
|
||||
let task_trace = captured_trace
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.clone()
|
||||
.expect("turn task should capture the current span trace context");
|
||||
let request_context =
|
||||
codex_otel::context_from_w3c_trace_context(&request_trace).expect("request");
|
||||
let task_context = codex_otel::context_from_w3c_trace_context(&task_trace).expect("task");
|
||||
|
||||
assert_ne!(
|
||||
task_context.span().span_context().trace_id(),
|
||||
request_context.span().span_context().trace_id()
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(debug_assertions)]
|
||||
#[tokio::test]
|
||||
async fn shutdown_complete_does_not_append_to_thread_store_after_shutdown() {
|
||||
|
||||
@@ -63,6 +63,14 @@ pub(crate) use user_shell::execute_user_shell_command;
|
||||
const GRACEFULL_INTERRUPTION_TIMEOUT_MS: u64 = 100;
|
||||
const TASK_COMPACT_METRIC: &str = "codex.task.compact";
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub(crate) enum TurnSpanParent {
|
||||
/// Preserve the active request or dispatch context for externally started work.
|
||||
Current,
|
||||
/// Avoid nesting automatic follow-on turns under the completed turn that launched them.
|
||||
Root,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub(crate) enum InterruptedTurnHistoryMarker {
|
||||
Disabled,
|
||||
@@ -307,7 +315,8 @@ impl Session {
|
||||
) {
|
||||
self.abort_all_tasks(TurnAbortReason::Replaced).await;
|
||||
self.clear_connector_selection().await;
|
||||
self.start_task(turn_context, input, task).await;
|
||||
self.start_task(turn_context, input, task, TurnSpanParent::Current)
|
||||
.await;
|
||||
}
|
||||
|
||||
pub(crate) async fn start_task<T: SessionTask>(
|
||||
@@ -315,6 +324,7 @@ impl Session {
|
||||
turn_context: Arc<TurnContext>,
|
||||
input: Vec<TurnInput>,
|
||||
task: T,
|
||||
span_parent: TurnSpanParent,
|
||||
) {
|
||||
let task: Arc<dyn AnySessionTask> = Arc::new(task);
|
||||
let task_kind = task.kind();
|
||||
@@ -386,7 +396,12 @@ impl Session {
|
||||
// Task-owned turn spans keep a core-owned span open for the
|
||||
// full task lifecycle after the submission dispatch span ends.
|
||||
let reasoning_effort = turn_context.effective_reasoning_effort_for_tracing();
|
||||
let parent_span_id = match span_parent {
|
||||
TurnSpanParent::Current => Span::current().id(),
|
||||
TurnSpanParent::Root => None,
|
||||
};
|
||||
let task_span = info_span!(
|
||||
parent: parent_span_id,
|
||||
"turn",
|
||||
otel.name = span_name,
|
||||
thread.id = %self.conversation_id,
|
||||
@@ -457,9 +472,15 @@ impl Session {
|
||||
///
|
||||
/// This helper generates a fresh sub-id for the synthetic turn before delegating to the
|
||||
/// explicit-sub-id variant.
|
||||
pub(crate) async fn maybe_start_turn_for_pending_work(self: &Arc<Self>) {
|
||||
self.maybe_start_turn_for_pending_work_with_sub_id(uuid::Uuid::new_v4().to_string())
|
||||
.await;
|
||||
pub(crate) async fn maybe_start_turn_for_pending_work(
|
||||
self: &Arc<Self>,
|
||||
span_parent: TurnSpanParent,
|
||||
) {
|
||||
self.maybe_start_turn_for_pending_work_with_sub_id(
|
||||
uuid::Uuid::new_v4().to_string(),
|
||||
span_parent,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
|
||||
/// Starts a regular turn with the provided sub-id when pending work should wake an idle
|
||||
@@ -470,6 +491,7 @@ impl Session {
|
||||
pub(crate) async fn maybe_start_turn_for_pending_work_with_sub_id(
|
||||
self: &Arc<Self>,
|
||||
sub_id: String,
|
||||
span_parent: TurnSpanParent,
|
||||
) {
|
||||
if !self
|
||||
.input_queue
|
||||
@@ -491,7 +513,7 @@ impl Session {
|
||||
let turn_context = self.new_default_turn_with_sub_id(sub_id).await;
|
||||
self.maybe_emit_unknown_model_warning_for_turn(turn_context.as_ref())
|
||||
.await;
|
||||
self.start_task(turn_context, Vec::new(), RegularTask::new())
|
||||
self.start_task(turn_context, Vec::new(), RegularTask::new(), span_parent)
|
||||
.await;
|
||||
}
|
||||
|
||||
@@ -530,7 +552,8 @@ impl Session {
|
||||
self.input_queue.clear_pending(&active_turn).await;
|
||||
}
|
||||
if reason == TurnAbortReason::Interrupted && aborted_turn {
|
||||
self.maybe_start_turn_for_pending_work().await;
|
||||
self.maybe_start_turn_for_pending_work(TurnSpanParent::Current)
|
||||
.await;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -577,7 +600,8 @@ impl Session {
|
||||
self.input_queue.clear_pending(&active_turn).await;
|
||||
|
||||
if reason == TurnAbortReason::Interrupted {
|
||||
self.maybe_start_turn_for_pending_work().await;
|
||||
self.maybe_start_turn_for_pending_work(TurnSpanParent::Current)
|
||||
.await;
|
||||
}
|
||||
|
||||
true
|
||||
|
||||
Reference in New Issue
Block a user