mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Unify thread metadata updates above store (#22236)
- make ThreadStore::update_thread_metadata accept a broad range of metadata patches - keep ThreadStore::append_items as raw canonical history append (no metadata side effects) - in the local store, write these metadata updates to a combination of sqlite and rollout jsonl files for backwards-compat. It special cases which fields need to go into jsonl vs sqlite vs whatever, confining the awkwardness to just this implementation - in remote stores we can simply persist the metadata directly to a database, no special casing required. - move the "implicit metadata updates triggered by appending rollout items" from the RolloutRecorder (which is local-threadstore-specific) to the LiveThread layer above the ThreadStore, inside of a private helper utility called ThreadMetadataSync. LiveThread calls ThreadStore append_items and update_metadata separately. - Add a generic update metadata method to ThreadManager that works on both live threads and "cold" threads - Call that ThreadManager method from app server code, so app server doesn't need to worry about whether the thread is live or not
This commit is contained in:
@@ -2846,12 +2846,21 @@ impl Session {
|
||||
&self,
|
||||
turn_context: &TurnContext,
|
||||
token_usage: Option<&TokenUsage>,
|
||||
) {
|
||||
self.record_token_usage_info(turn_context, token_usage)
|
||||
.await;
|
||||
self.send_token_count_event(turn_context).await;
|
||||
}
|
||||
|
||||
pub(crate) async fn record_token_usage_info(
|
||||
&self,
|
||||
turn_context: &TurnContext,
|
||||
token_usage: Option<&TokenUsage>,
|
||||
) {
|
||||
if let Some(token_usage) = token_usage {
|
||||
let mut state = self.state.lock().await;
|
||||
state.update_token_info_from_usage(token_usage, turn_context.model_context_window());
|
||||
}
|
||||
self.send_token_count_event(turn_context).await;
|
||||
}
|
||||
|
||||
pub(crate) async fn recompute_token_usage(&self, turn_context: &TurnContext) {
|
||||
@@ -2892,11 +2901,15 @@ impl Session {
|
||||
turn_context: &TurnContext,
|
||||
new_rate_limits: RateLimitSnapshot,
|
||||
) {
|
||||
self.record_rate_limits_info(new_rate_limits).await;
|
||||
self.send_token_count_event(turn_context).await;
|
||||
}
|
||||
|
||||
pub(crate) async fn record_rate_limits_info(&self, new_rate_limits: RateLimitSnapshot) {
|
||||
{
|
||||
let mut state = self.state.lock().await;
|
||||
state.set_rate_limits(new_rate_limits);
|
||||
}
|
||||
self.send_token_count_event(turn_context).await;
|
||||
}
|
||||
|
||||
pub(crate) async fn mcp_dependency_prompted(&self) -> HashSet<String> {
|
||||
@@ -2927,7 +2940,7 @@ impl Session {
|
||||
state.set_server_reasoning_included(included);
|
||||
}
|
||||
|
||||
async fn send_token_count_event(&self, turn_context: &TurnContext) {
|
||||
pub(crate) async fn send_token_count_event(&self, turn_context: &TurnContext) {
|
||||
let (info, rate_limits) = {
|
||||
let state = self.state.lock().await;
|
||||
state.token_info_and_rate_limits()
|
||||
|
||||
@@ -1872,6 +1872,7 @@ async fn try_run_sampling_request(
|
||||
Box<dyn ToolArgumentDiffConsumer>,
|
||||
)> = None;
|
||||
let mut should_emit_turn_diff = false;
|
||||
let mut should_emit_token_count = false;
|
||||
let reasoning_effort = turn_context.effective_reasoning_effort_for_tracing();
|
||||
let plan_mode = turn_context.collaboration_mode.mode == ModeKind::Plan;
|
||||
let mut assistant_message_stream_parsers = AssistantMessageStreamParsers::new(plan_mode);
|
||||
@@ -2098,7 +2099,8 @@ async fn try_run_sampling_request(
|
||||
ResponseEvent::RateLimits(snapshot) => {
|
||||
// Update internal state with latest rate limits, but defer sending until
|
||||
// token usage is available to avoid duplicate TokenCount events.
|
||||
sess.update_rate_limits(&turn_context, snapshot).await;
|
||||
sess.record_rate_limits_info(snapshot).await;
|
||||
should_emit_token_count = true;
|
||||
}
|
||||
ResponseEvent::ModelsEtag(etag) => {
|
||||
// Update internal state with latest models etag
|
||||
@@ -2116,8 +2118,9 @@ async fn try_run_sampling_request(
|
||||
&mut assistant_message_stream_parsers,
|
||||
)
|
||||
.await;
|
||||
sess.update_token_usage_info(&turn_context, token_usage.as_ref())
|
||||
sess.record_token_usage_info(&turn_context, token_usage.as_ref())
|
||||
.await;
|
||||
should_emit_token_count = true;
|
||||
should_emit_turn_diff = true;
|
||||
if let Some(false) = end_turn {
|
||||
needs_follow_up = true;
|
||||
@@ -2245,6 +2248,14 @@ async fn try_run_sampling_request(
|
||||
|
||||
drain_in_flight(&mut in_flight, sess.clone(), turn_context.clone()).await?;
|
||||
|
||||
if should_emit_token_count {
|
||||
// A tool call such as request_user_input can intentionally pause the turn. Emit token
|
||||
// counts only after pending tools resolve so clients do not see progress events while the
|
||||
// turn is waiting on the user. This also needs to happen before returning cancellation so
|
||||
// token usage already recorded from the completed response is still persisted.
|
||||
sess.send_token_count_event(&turn_context).await;
|
||||
}
|
||||
|
||||
if cancellation_token.is_cancelled() {
|
||||
return Err(CodexErr::TurnAborted);
|
||||
}
|
||||
|
||||
@@ -58,8 +58,10 @@ use codex_thread_store::LocalThreadStoreConfig;
|
||||
use codex_thread_store::ReadThreadByRolloutPathParams;
|
||||
use codex_thread_store::ReadThreadParams;
|
||||
use codex_thread_store::StoredThread;
|
||||
use codex_thread_store::ThreadMetadataPatch;
|
||||
use codex_thread_store::ThreadStore;
|
||||
use codex_thread_store::ThreadStoreError;
|
||||
use codex_thread_store::UpdateThreadMetadataParams;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use futures::StreamExt;
|
||||
use futures::stream::FuturesUnordered;
|
||||
@@ -456,6 +458,44 @@ impl ThreadManager {
|
||||
self.state.get_thread(thread_id).await
|
||||
}
|
||||
|
||||
/// Updates metadata for loaded and cold threads through one entrypoint.
|
||||
///
|
||||
/// Loaded threads route through `CodexThread`/`LiveThread`, so metadata changes stay ordered
|
||||
/// with live rollout writes. Cold threads go directly to the store, which owns unloaded JSONL
|
||||
/// compatibility and SQLite metadata updates.
|
||||
pub async fn update_thread_metadata(
|
||||
&self,
|
||||
thread_id: ThreadId,
|
||||
patch: ThreadMetadataPatch,
|
||||
include_archived: bool,
|
||||
) -> CodexResult<StoredThread> {
|
||||
if let Ok(thread) = self.get_thread(thread_id).await {
|
||||
if thread.config_snapshot().await.ephemeral {
|
||||
return Err(CodexErr::InvalidRequest(format!(
|
||||
"ephemeral thread does not support metadata updates: {thread_id}"
|
||||
)));
|
||||
}
|
||||
return thread
|
||||
.update_thread_metadata(patch, include_archived)
|
||||
.await
|
||||
.map_err(|err| thread_store_metadata_update_error(thread_id, err));
|
||||
}
|
||||
self.state
|
||||
.thread_store
|
||||
.update_thread_metadata(UpdateThreadMetadataParams {
|
||||
thread_id,
|
||||
patch,
|
||||
include_archived,
|
||||
})
|
||||
.await
|
||||
.map_err(|err| match err {
|
||||
ThreadStoreError::ThreadNotFound { thread_id } => {
|
||||
CodexErr::ThreadNotFound(thread_id)
|
||||
}
|
||||
err => thread_store_metadata_update_error(thread_id, err),
|
||||
})
|
||||
}
|
||||
|
||||
/// List `thread_id` plus all known descendants in its spawn subtree.
|
||||
pub async fn list_agent_subtree_thread_ids(
|
||||
&self,
|
||||
@@ -1298,6 +1338,19 @@ fn thread_store_rollout_read_error(err: ThreadStoreError) -> CodexErr {
|
||||
}
|
||||
}
|
||||
|
||||
fn thread_store_metadata_update_error(thread_id: ThreadId, err: ThreadStoreError) -> CodexErr {
|
||||
match err {
|
||||
ThreadStoreError::ThreadNotFound { thread_id } => CodexErr::ThreadNotFound(thread_id),
|
||||
ThreadStoreError::InvalidRequest { message } => CodexErr::InvalidRequest(message),
|
||||
ThreadStoreError::Unsupported { operation } => CodexErr::UnsupportedOperation(format!(
|
||||
"thread metadata update is not supported by this store: {operation}"
|
||||
)),
|
||||
err => CodexErr::Fatal(format!(
|
||||
"failed to update thread metadata {thread_id}: {err}"
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
||||
/// Return a fork snapshot cut strictly before the nth user message (0-based).
|
||||
///
|
||||
/// Out-of-range values keep the full committed history at a turn boundary, but
|
||||
|
||||
@@ -2497,38 +2497,6 @@ async fn token_count_includes_rate_limits_snapshot() {
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let first_token_event =
|
||||
wait_for_event(&codex, |msg| matches!(msg, EventMsg::TokenCount(_))).await;
|
||||
let rate_limit_only = match first_token_event {
|
||||
EventMsg::TokenCount(ev) => ev,
|
||||
_ => unreachable!(),
|
||||
};
|
||||
|
||||
let rate_limit_json = serde_json::to_value(&rate_limit_only).unwrap();
|
||||
pretty_assertions::assert_eq!(
|
||||
rate_limit_json,
|
||||
json!({
|
||||
"info": null,
|
||||
"rate_limits": {
|
||||
"limit_id": "codex",
|
||||
"limit_name": null,
|
||||
"primary": {
|
||||
"used_percent": 12.5,
|
||||
"window_minutes": 10,
|
||||
"resets_at": 1704069000
|
||||
},
|
||||
"secondary": {
|
||||
"used_percent": 40.0,
|
||||
"window_minutes": 60,
|
||||
"resets_at": 1704074400
|
||||
},
|
||||
"credits": null,
|
||||
"plan_type": null,
|
||||
"rate_limit_reached_type": null
|
||||
}
|
||||
})
|
||||
);
|
||||
|
||||
let token_event = wait_for_event(
|
||||
&codex,
|
||||
|msg| matches!(msg, EventMsg::TokenCount(ev) if ev.info.is_some()),
|
||||
|
||||
@@ -891,7 +891,7 @@ async fn handle_response_item_records_tool_result_for_custom_tool_call() {
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
wait_for_event(&codex, |ev| matches!(ev, EventMsg::TokenCount(_))).await;
|
||||
wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await;
|
||||
|
||||
logs_assert(|lines: &[&str]| {
|
||||
let line = lines
|
||||
|
||||
@@ -17,6 +17,7 @@ use core_test_support::responses;
|
||||
use core_test_support::responses::ResponsesRequest;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
use core_test_support::responses::ev_completed;
|
||||
use core_test_support::responses::ev_completed_with_tokens;
|
||||
use core_test_support::responses::ev_function_call;
|
||||
use core_test_support::responses::ev_response_created;
|
||||
use core_test_support::responses::sse;
|
||||
@@ -30,6 +31,8 @@ use core_test_support::wait_for_event_match;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::Value;
|
||||
use serde_json::json;
|
||||
use tokio::time::Duration;
|
||||
use tokio::time::timeout;
|
||||
|
||||
fn call_output(req: &ResponsesRequest, call_id: &str) -> String {
|
||||
let raw = req.function_call_output(call_id);
|
||||
@@ -118,6 +121,7 @@ async fn request_user_input_round_trip_for_mode(mode: ModeKind) -> anyhow::Resul
|
||||
let first_response = sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_function_call(call_id, "request_user_input", &request_args),
|
||||
ev_rate_limits(),
|
||||
ev_completed("resp-1"),
|
||||
]);
|
||||
responses::mount_sse_once(&server, first_response).await;
|
||||
@@ -169,6 +173,22 @@ async fn request_user_input_round_trip_for_mode(mode: ModeKind) -> anyhow::Resul
|
||||
assert_eq!(request.call_id, call_id);
|
||||
assert_eq!(request.questions.len(), 1);
|
||||
assert_eq!(request.questions[0].is_other, true);
|
||||
assert!(
|
||||
timeout(Duration::from_millis(200), async {
|
||||
loop {
|
||||
let event = match codex.next_event().await {
|
||||
Ok(event) => event,
|
||||
Err(err) => panic!("event stream should stay open: {err}"),
|
||||
};
|
||||
if matches!(event.msg, EventMsg::TokenCount(_)) {
|
||||
return;
|
||||
}
|
||||
}
|
||||
})
|
||||
.await
|
||||
.is_err(),
|
||||
"TokenCount should wait until request_user_input resolves"
|
||||
);
|
||||
|
||||
let mut answers = HashMap::new();
|
||||
answers.insert(
|
||||
@@ -185,6 +205,7 @@ async fn request_user_input_round_trip_for_mode(mode: ModeKind) -> anyhow::Resul
|
||||
})
|
||||
.await?;
|
||||
|
||||
wait_for_event(&codex, |event| matches!(event, EventMsg::TokenCount(_))).await;
|
||||
wait_for_event(&codex, |event| matches!(event, EventMsg::TurnComplete(_))).await;
|
||||
|
||||
let req = second_mock.single_request();
|
||||
@@ -202,6 +223,118 @@ async fn request_user_input_round_trip_for_mode(mode: ModeKind) -> anyhow::Resul
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn ev_rate_limits() -> Value {
|
||||
json!({
|
||||
"type": "codex.rate_limits",
|
||||
"plan_type": "plus",
|
||||
"rate_limits": {
|
||||
"allowed": true,
|
||||
"limit_reached": false,
|
||||
"primary": {
|
||||
"used_percent": 42,
|
||||
"window_minutes": 60,
|
||||
"reset_at": 1700000000
|
||||
},
|
||||
"secondary": null
|
||||
},
|
||||
"code_review_rate_limits": null,
|
||||
"credits": null,
|
||||
"promo": null
|
||||
})
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn request_user_input_interrupt_emits_deferred_token_count() -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let TestCodex {
|
||||
codex,
|
||||
cwd,
|
||||
session_configured,
|
||||
..
|
||||
} = test_codex().build(&server).await?;
|
||||
|
||||
let call_id = "user-input-interrupt";
|
||||
let request_args = json!({
|
||||
"questions": [{
|
||||
"id": "confirm_path",
|
||||
"header": "Confirm",
|
||||
"question": "Proceed with the plan?",
|
||||
"options": [{
|
||||
"label": "Yes (Recommended)",
|
||||
"description": "Continue the current plan."
|
||||
}, {
|
||||
"label": "No",
|
||||
"description": "Stop and revisit the approach."
|
||||
}]
|
||||
}]
|
||||
})
|
||||
.to_string();
|
||||
|
||||
let response = sse(vec![
|
||||
ev_response_created("resp-interrupt"),
|
||||
ev_function_call(call_id, "request_user_input", &request_args),
|
||||
ev_completed_with_tokens("resp-interrupt", /*total_tokens*/ 77),
|
||||
]);
|
||||
responses::mount_sse_once(&server, response).await;
|
||||
|
||||
let (sandbox_policy, permission_profile) =
|
||||
turn_permission_fields(PermissionProfile::Disabled, cwd.path());
|
||||
codex
|
||||
.submit(Op::UserTurn {
|
||||
environments: None,
|
||||
items: vec![UserInput::Text {
|
||||
text: "please confirm".into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
final_output_json_schema: None,
|
||||
cwd: cwd.path().to_path_buf(),
|
||||
approval_policy: AskForApproval::Never,
|
||||
approvals_reviewer: None,
|
||||
sandbox_policy,
|
||||
permission_profile,
|
||||
model: session_configured.model.clone(),
|
||||
effort: None,
|
||||
summary: None,
|
||||
service_tier: None,
|
||||
collaboration_mode: Some(CollaborationMode {
|
||||
mode: ModeKind::Plan,
|
||||
settings: Settings {
|
||||
model: session_configured.model,
|
||||
reasoning_effort: None,
|
||||
developer_instructions: None,
|
||||
},
|
||||
}),
|
||||
personality: None,
|
||||
})
|
||||
.await?;
|
||||
|
||||
let request = wait_for_event_match(&codex, |event| match event {
|
||||
EventMsg::RequestUserInput(request) => Some(request.clone()),
|
||||
_ => None,
|
||||
})
|
||||
.await;
|
||||
|
||||
codex.submit(Op::Interrupt).await?;
|
||||
|
||||
let token_count = wait_for_event_match(&codex, |event| match event {
|
||||
EventMsg::TokenCount(token_count) => Some(token_count.clone()),
|
||||
_ => None,
|
||||
})
|
||||
.await;
|
||||
assert_eq!(
|
||||
token_count
|
||||
.info
|
||||
.map(|info| info.total_token_usage.total_tokens),
|
||||
Some(77)
|
||||
);
|
||||
wait_for_event(&codex, |event| matches!(event, EventMsg::TurnAborted(_))).await;
|
||||
|
||||
assert_eq!(request.call_id, call_id);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn assert_request_user_input_rejected<F>(mode_name: &str, build_mode: F) -> anyhow::Result<()>
|
||||
where
|
||||
F: FnOnce(String) -> CollaborationMode,
|
||||
|
||||
@@ -109,7 +109,6 @@ async fn resume_includes_initial_messages_from_rollout_events() -> Result<()> {
|
||||
[
|
||||
EventMsg::TurnStarted(_),
|
||||
EventMsg::UserMessage(_),
|
||||
EventMsg::TokenCount(_),
|
||||
EventMsg::AgentMessage(_),
|
||||
EventMsg::TokenCount(_),
|
||||
EventMsg::TurnComplete(_),
|
||||
@@ -126,7 +125,6 @@ async fn resume_includes_initial_messages_from_rollout_events() -> Result<()> {
|
||||
[
|
||||
EventMsg::TurnStarted(started),
|
||||
EventMsg::UserMessage(first_user),
|
||||
EventMsg::TokenCount(_),
|
||||
EventMsg::AgentMessage(assistant_message),
|
||||
EventMsg::TokenCount(_),
|
||||
EventMsg::TurnComplete(completed),
|
||||
@@ -196,7 +194,6 @@ async fn resume_includes_initial_messages_from_reasoning_events() -> Result<()>
|
||||
[
|
||||
EventMsg::TurnStarted(_),
|
||||
EventMsg::UserMessage(_),
|
||||
EventMsg::TokenCount(_),
|
||||
EventMsg::AgentReasoning(_),
|
||||
EventMsg::AgentReasoningRawContent(_),
|
||||
EventMsg::AgentMessage(_),
|
||||
@@ -215,7 +212,6 @@ async fn resume_includes_initial_messages_from_reasoning_events() -> Result<()>
|
||||
[
|
||||
EventMsg::TurnStarted(started),
|
||||
EventMsg::UserMessage(first_user),
|
||||
EventMsg::TokenCount(_),
|
||||
EventMsg::AgentReasoning(reasoning),
|
||||
EventMsg::AgentReasoningRawContent(raw),
|
||||
EventMsg::AgentMessage(assistant_message),
|
||||
|
||||
@@ -4,7 +4,6 @@ use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use chrono::Utc;
|
||||
use codex_core::EventPersistenceMode;
|
||||
use codex_core::RolloutRecorder;
|
||||
use codex_core::RolloutRecorderParams;
|
||||
use codex_core::config::ConfigBuilder;
|
||||
@@ -189,10 +188,7 @@ async fn find_locates_rollout_file_written_by_recorder() -> std::io::Result<()>
|
||||
/*thread_source*/ None,
|
||||
BaseInstructions::default(),
|
||||
Vec::new(),
|
||||
EventPersistenceMode::Limited,
|
||||
),
|
||||
/*state_db_ctx*/ None,
|
||||
/*state_builder*/ None,
|
||||
)
|
||||
.await?;
|
||||
recorder.persist().await?;
|
||||
|
||||
Reference in New Issue
Block a user