mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Make thread store process-scoped (#19474)
- Build one app-server process ThreadStore from startup config and share it with ThreadManager and CodexMessageProcessor. - Remove per-thread/fork store reconstruction so effective thread config cannot switch the persistence backend. - Add params to ThreadStore create/resume for specifying thread metadata, since otherwise the metadata from store creation would be used (incorrectly).
This commit is contained in:
@@ -2687,12 +2687,7 @@ mod tests {
|
||||
thread_id: conversation_id,
|
||||
thread: conversation,
|
||||
..
|
||||
} = thread_manager
|
||||
.start_thread(
|
||||
config.clone(),
|
||||
codex_core::thread_store_from_config(&config),
|
||||
)
|
||||
.await?;
|
||||
} = thread_manager.start_thread(config.clone()).await?;
|
||||
let thread_state = new_thread_state();
|
||||
let thread_watch_manager = ThreadWatchManager::new();
|
||||
let (tx, mut rx) = mpsc::channel(CHANNEL_CAPACITY);
|
||||
|
||||
@@ -259,7 +259,6 @@ use codex_core::ThreadManager;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::config::ConfigOverrides;
|
||||
use codex_core::config::NetworkProxyAuditMetadata;
|
||||
use codex_core::config::ThreadStoreConfig;
|
||||
use codex_core::config::edit::ConfigEdit;
|
||||
use codex_core::config::edit::ConfigEditsBuilder;
|
||||
use codex_core::exec::ExecCapturePolicy;
|
||||
@@ -378,12 +377,10 @@ use codex_state::ThreadMetadata;
|
||||
use codex_state::ThreadMetadataBuilder;
|
||||
use codex_state::log_db::LogDbLayer;
|
||||
use codex_thread_store::ArchiveThreadParams as StoreArchiveThreadParams;
|
||||
use codex_thread_store::InMemoryThreadStore;
|
||||
use codex_thread_store::ListThreadsParams as StoreListThreadsParams;
|
||||
use codex_thread_store::LocalThreadStore;
|
||||
use codex_thread_store::ReadThreadByRolloutPathParams as StoreReadThreadByRolloutPathParams;
|
||||
use codex_thread_store::ReadThreadParams as StoreReadThreadParams;
|
||||
use codex_thread_store::RemoteThreadStore;
|
||||
use codex_thread_store::SortDirection as StoreSortDirection;
|
||||
use codex_thread_store::StoredThread;
|
||||
use codex_thread_store::ThreadMetadataPatch as StoreThreadMetadataPatch;
|
||||
@@ -691,18 +688,11 @@ pub(crate) struct CodexMessageProcessorArgs {
|
||||
/// go through `config_manager`.
|
||||
pub(crate) config: Arc<Config>,
|
||||
pub(crate) config_manager: ConfigManager,
|
||||
pub(crate) thread_store: Arc<dyn ThreadStore>,
|
||||
pub(crate) feedback: CodexFeedback,
|
||||
pub(crate) log_db: Option<LogDbLayer>,
|
||||
}
|
||||
|
||||
fn thread_store_from_config(config: &Config) -> Arc<dyn ThreadStore> {
|
||||
match &config.experimental_thread_store {
|
||||
ThreadStoreConfig::Local => Arc::new(configured_local_thread_store(config)),
|
||||
ThreadStoreConfig::Remote { endpoint } => Arc::new(RemoteThreadStore::new(endpoint)),
|
||||
ThreadStoreConfig::InMemory { id } => InMemoryThreadStore::for_id(id),
|
||||
}
|
||||
}
|
||||
|
||||
fn environment_selection_error_message(err: CodexErr) -> String {
|
||||
match err {
|
||||
CodexErr::InvalidRequest(message) => message,
|
||||
@@ -710,10 +700,6 @@ fn environment_selection_error_message(err: CodexErr) -> String {
|
||||
}
|
||||
}
|
||||
|
||||
fn configured_local_thread_store(config: &Config) -> LocalThreadStore {
|
||||
LocalThreadStore::new(codex_rollout::RolloutConfig::from_view(config))
|
||||
}
|
||||
|
||||
impl CodexMessageProcessor {
|
||||
async fn instruction_sources_from_config(config: &Config) -> Vec<AbsolutePathBuf> {
|
||||
codex_core::AgentsMdManager::new(config)
|
||||
@@ -830,6 +816,7 @@ impl CodexMessageProcessor {
|
||||
arg0_paths,
|
||||
config,
|
||||
config_manager,
|
||||
thread_store,
|
||||
feedback,
|
||||
log_db,
|
||||
} = args;
|
||||
@@ -839,7 +826,7 @@ impl CodexMessageProcessor {
|
||||
outgoing: outgoing.clone(),
|
||||
analytics_events_client,
|
||||
arg0_paths,
|
||||
thread_store: thread_store_from_config(&config),
|
||||
thread_store,
|
||||
config,
|
||||
config_manager,
|
||||
active_login: Arc::new(Mutex::new(None)),
|
||||
@@ -2586,7 +2573,6 @@ impl CodexMessageProcessor {
|
||||
let imported_thread = self
|
||||
.thread_manager
|
||||
.start_thread_with_options(StartThreadOptions {
|
||||
thread_store: thread_store_from_config(&config),
|
||||
config,
|
||||
initial_history: InitialHistory::Forked(rollout_items),
|
||||
session_source: None,
|
||||
@@ -2784,7 +2770,6 @@ impl CodexMessageProcessor {
|
||||
} = listener_task_context
|
||||
.thread_manager
|
||||
.start_thread_with_options(StartThreadOptions {
|
||||
thread_store: thread_store_from_config(&config),
|
||||
config,
|
||||
initial_history: match session_start_source
|
||||
.unwrap_or(codex_app_server_protocol::ThreadStartSource::Startup)
|
||||
@@ -4348,7 +4333,6 @@ impl CodexMessageProcessor {
|
||||
.thread_manager
|
||||
.resume_thread_with_history(
|
||||
config.clone(),
|
||||
thread_store_from_config(&config),
|
||||
thread_history,
|
||||
self.auth_manager.clone(),
|
||||
persist_extended_history,
|
||||
@@ -4503,27 +4487,20 @@ impl CodexMessageProcessor {
|
||||
request_id: &ConnectionRequestId,
|
||||
params: &ThreadResumeParams,
|
||||
) -> Result<bool, JSONRPCErrorError> {
|
||||
if let Ok(existing_thread_id) = ThreadId::from_string(¶ms.thread_id)
|
||||
&& let Ok(existing_thread) = self.thread_manager.get_thread(existing_thread_id).await
|
||||
{
|
||||
if params.history.is_some() {
|
||||
let running_thread = if params.history.is_some() {
|
||||
if let Ok(existing_thread_id) = ThreadId::from_string(¶ms.thread_id)
|
||||
&& self
|
||||
.thread_manager
|
||||
.get_thread(existing_thread_id)
|
||||
.await
|
||||
.is_ok()
|
||||
{
|
||||
return Err(invalid_request(format!(
|
||||
"cannot resume thread {existing_thread_id} with history while it is already running"
|
||||
)));
|
||||
}
|
||||
|
||||
if let (Some(requested_path), Some(active_path)) = (
|
||||
params.path.as_ref(),
|
||||
existing_thread.rollout_path().as_ref(),
|
||||
) && requested_path != active_path
|
||||
{
|
||||
return Err(invalid_request(format!(
|
||||
"cannot resume running thread {existing_thread_id} with mismatched path: requested `{}`, active `{}`",
|
||||
requested_path.display(),
|
||||
active_path.display()
|
||||
)));
|
||||
}
|
||||
|
||||
None
|
||||
} else if params.path.is_some() {
|
||||
let source_thread = self
|
||||
.read_stored_thread_for_resume(
|
||||
¶ms.thread_id,
|
||||
@@ -4531,12 +4508,45 @@ impl CodexMessageProcessor {
|
||||
/*include_history*/ true,
|
||||
)
|
||||
.await?;
|
||||
let existing_thread_id = source_thread.thread_id;
|
||||
if let Ok(existing_thread) = self.thread_manager.get_thread(existing_thread_id).await {
|
||||
if let (Some(requested_path), Some(active_path)) = (
|
||||
params.path.as_ref(),
|
||||
existing_thread.rollout_path().as_ref(),
|
||||
) && requested_path != active_path
|
||||
{
|
||||
return Err(invalid_request(format!(
|
||||
"cannot resume running thread {existing_thread_id} with stale path: requested `{}`, active `{}`",
|
||||
requested_path.display(),
|
||||
active_path.display()
|
||||
)));
|
||||
}
|
||||
Some((existing_thread_id, existing_thread, source_thread))
|
||||
} else {
|
||||
None
|
||||
}
|
||||
} else if let Ok(existing_thread_id) = ThreadId::from_string(¶ms.thread_id)
|
||||
&& let Ok(existing_thread) = self.thread_manager.get_thread(existing_thread_id).await
|
||||
{
|
||||
let source_thread = self
|
||||
.read_stored_thread_for_resume(
|
||||
¶ms.thread_id,
|
||||
/*path*/ None,
|
||||
/*include_history*/ true,
|
||||
)
|
||||
.await?;
|
||||
if source_thread.thread_id != existing_thread_id {
|
||||
return Err(invalid_request(format!(
|
||||
"cannot resume running thread {existing_thread_id} from source thread {}",
|
||||
source_thread.thread_id
|
||||
)));
|
||||
}
|
||||
Some((existing_thread_id, existing_thread, source_thread))
|
||||
} else {
|
||||
None
|
||||
};
|
||||
|
||||
if let Some((existing_thread_id, existing_thread, source_thread)) = running_thread {
|
||||
let history_items = source_thread
|
||||
.history
|
||||
.as_ref()
|
||||
@@ -4731,11 +4741,10 @@ impl CodexMessageProcessor {
|
||||
|
||||
async fn read_stored_thread_for_new_fork(
|
||||
&self,
|
||||
thread_store: &dyn ThreadStore,
|
||||
thread_id: ThreadId,
|
||||
include_history: bool,
|
||||
) -> Result<StoredThread, JSONRPCErrorError> {
|
||||
thread_store
|
||||
self.thread_store
|
||||
.read_thread(StoreReadThreadParams {
|
||||
thread_id,
|
||||
include_archived: true,
|
||||
@@ -4938,7 +4947,6 @@ impl CodexMessageProcessor {
|
||||
|
||||
let fallback_model_provider = config.model_provider_id.clone();
|
||||
let instruction_sources = Self::instruction_sources_from_config(&config).await;
|
||||
let fork_thread_store = thread_store_from_config(&config);
|
||||
|
||||
let NewThread {
|
||||
thread_id,
|
||||
@@ -4950,7 +4958,6 @@ impl CodexMessageProcessor {
|
||||
.fork_thread_from_history(
|
||||
ForkSnapshot::Interrupted,
|
||||
config,
|
||||
fork_thread_store.clone(),
|
||||
InitialHistory::Resumed(ResumedHistory {
|
||||
conversation_id: source_thread_id,
|
||||
history: history_items.clone(),
|
||||
@@ -4986,11 +4993,7 @@ impl CodexMessageProcessor {
|
||||
let mut thread =
|
||||
if let Some(fork_rollout_path) = session_configured.rollout_path.as_ref() {
|
||||
let stored_thread = self
|
||||
.read_stored_thread_for_new_fork(
|
||||
fork_thread_store.as_ref(),
|
||||
thread_id,
|
||||
include_turns,
|
||||
)
|
||||
.read_stored_thread_for_new_fork(thread_id, include_turns)
|
||||
.await?;
|
||||
self.stored_thread_to_api_thread(
|
||||
stored_thread,
|
||||
@@ -7250,7 +7253,6 @@ impl CodexMessageProcessor {
|
||||
.fork_thread(
|
||||
ForkSnapshot::Interrupted,
|
||||
config.clone(),
|
||||
thread_store_from_config(&config),
|
||||
rollout_path,
|
||||
/*persist_extended_history*/ false,
|
||||
self.request_trace_context(request_id).await,
|
||||
|
||||
@@ -65,6 +65,7 @@ use codex_arg0::Arg0DispatchPaths;
|
||||
use codex_chatgpt::connectors;
|
||||
use codex_core::ThreadManager;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::thread_store_from_config;
|
||||
use codex_exec_server::EnvironmentManager;
|
||||
use codex_features::Feature;
|
||||
use codex_feedback::CodexFeedback;
|
||||
@@ -285,12 +286,17 @@ impl MessageProcessor {
|
||||
auth_manager.set_external_auth(Arc::new(ExternalAuthRefreshBridge {
|
||||
outgoing: outgoing.clone(),
|
||||
}));
|
||||
// The thread store is intentionally process-scoped. Config reloads can
|
||||
// affect per-thread behavior, but they must not move newly started,
|
||||
// resumed, or forked threads to a different persistence backend/root.
|
||||
let thread_store = thread_store_from_config(config.as_ref());
|
||||
let thread_manager = Arc::new(ThreadManager::new(
|
||||
config.as_ref(),
|
||||
auth_manager.clone(),
|
||||
session_source,
|
||||
environment_manager,
|
||||
Some(analytics_events_client.clone()),
|
||||
Arc::clone(&thread_store),
|
||||
));
|
||||
thread_manager
|
||||
.plugins_manager()
|
||||
@@ -304,6 +310,7 @@ impl MessageProcessor {
|
||||
arg0_paths,
|
||||
config: Arc::clone(&config),
|
||||
config_manager: config_manager.clone(),
|
||||
thread_store,
|
||||
feedback,
|
||||
log_db,
|
||||
});
|
||||
|
||||
@@ -48,6 +48,7 @@ use codex_protocol::models::BaseInstructions;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::RolloutItem;
|
||||
use codex_protocol::protocol::SessionSource as ProtocolSessionSource;
|
||||
use codex_protocol::protocol::ThreadMemoryMode;
|
||||
use codex_protocol::protocol::UserMessageEvent;
|
||||
use codex_protocol::user_input::ByteRange;
|
||||
use codex_protocol::user_input::TextElement;
|
||||
@@ -56,6 +57,7 @@ use codex_thread_store::CreateThreadParams;
|
||||
use codex_thread_store::InMemoryThreadStore;
|
||||
use codex_thread_store::ThreadEventPersistenceMode;
|
||||
use codex_thread_store::ThreadMetadataPatch;
|
||||
use codex_thread_store::ThreadPersistenceMetadata;
|
||||
use codex_thread_store::ThreadStore;
|
||||
use codex_thread_store::UpdateThreadMetadataParams;
|
||||
use core_test_support::responses;
|
||||
@@ -1028,6 +1030,11 @@ async fn seed_pathless_store_thread(
|
||||
source: ProtocolSessionSource::Cli,
|
||||
base_instructions: BaseInstructions::default(),
|
||||
dynamic_tools: Vec::new(),
|
||||
metadata: ThreadPersistenceMetadata {
|
||||
cwd: None,
|
||||
model_provider: "test-provider".to_string(),
|
||||
memory_mode: ThreadMemoryMode::Disabled,
|
||||
},
|
||||
event_persistence_mode: ThreadEventPersistenceMode::default(),
|
||||
})
|
||||
.await?;
|
||||
|
||||
@@ -71,6 +71,7 @@ use core_test_support::skip_if_no_network;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use std::fs::FileTimes;
|
||||
use std::io::Write;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use std::process::Command;
|
||||
@@ -1669,7 +1670,7 @@ async fn thread_resume_rejects_history_when_thread_is_running() -> Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_rejects_mismatched_path_when_thread_is_running() -> Result<()> {
|
||||
async fn thread_resume_uses_path_over_thread_id_when_thread_is_running() -> Result<()> {
|
||||
let server = responses::start_mock_server().await;
|
||||
let first_body = responses::sse(vec![
|
||||
responses::ev_response_created("resp-1"),
|
||||
@@ -1749,24 +1750,71 @@ async fn thread_resume_rejects_mismatched_path_when_thread_is_running() -> Resul
|
||||
)
|
||||
.await??;
|
||||
|
||||
let resume_id = primary
|
||||
let other_thread_id = ThreadId::new().to_string();
|
||||
let stale_path = rollout_path(codex_home.path(), "2025-01-01T00-00-00", &thread_id);
|
||||
std::fs::create_dir_all(stale_path.parent().expect("stale path parent"))?;
|
||||
let thread_uuid = Uuid::parse_str(&thread_id)?;
|
||||
let mut stale_file = std::fs::File::create(&stale_path)?;
|
||||
let stale_meta = json!({
|
||||
"timestamp": "2025-01-01T00:00:00Z",
|
||||
"type": "session_meta",
|
||||
"payload": {
|
||||
"id": thread_uuid,
|
||||
"timestamp": "2025-01-01T00:00:00Z",
|
||||
"cwd": codex_home.path(),
|
||||
"originator": "test_originator",
|
||||
"cli_version": "test_version",
|
||||
"source": "cli",
|
||||
"model_provider": "test-provider",
|
||||
},
|
||||
});
|
||||
writeln!(stale_file, "{stale_meta}")?;
|
||||
let stale_user_event = json!({
|
||||
"timestamp": "2025-01-01T00:00:00Z",
|
||||
"type": "event_msg",
|
||||
"payload": {
|
||||
"type": "user_message",
|
||||
"message": "stale history",
|
||||
"kind": "plain",
|
||||
},
|
||||
});
|
||||
writeln!(stale_file, "{stale_user_event}")?;
|
||||
|
||||
let stale_resume_id = primary
|
||||
.send_thread_resume_request(ThreadResumeParams {
|
||||
thread_id: thread_id.clone(),
|
||||
path: Some(PathBuf::from("/tmp/does-not-match-running-rollout.jsonl")),
|
||||
thread_id: other_thread_id.clone(),
|
||||
path: Some(stale_path),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let resume_err: JSONRPCError = timeout(
|
||||
let stale_resume_err: JSONRPCError = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
primary.read_stream_until_error_message(RequestId::Integer(resume_id)),
|
||||
primary.read_stream_until_error_message(RequestId::Integer(stale_resume_id)),
|
||||
)
|
||||
.await??;
|
||||
assert!(
|
||||
resume_err.error.message.contains("mismatched path"),
|
||||
stale_resume_err.error.message.contains("stale path"),
|
||||
"unexpected resume error: {}",
|
||||
resume_err.error.message
|
||||
stale_resume_err.error.message
|
||||
);
|
||||
|
||||
let resume_by_path_id = primary
|
||||
.send_thread_resume_request(ThreadResumeParams {
|
||||
thread_id: other_thread_id.clone(),
|
||||
path: thread.path,
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let resume_by_path_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
primary.read_stream_until_response_message(RequestId::Integer(resume_by_path_id)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadResumeResponse {
|
||||
thread: resumed, ..
|
||||
} = to_response::<ThreadResumeResponse>(resume_by_path_resp)?;
|
||||
assert_eq!(resumed.id, thread_id);
|
||||
|
||||
primary
|
||||
.interrupt_turn_and_wait_for_aborted(thread_id, running_turn.id, DEFAULT_READ_TIMEOUT)
|
||||
.await?;
|
||||
@@ -2463,7 +2511,7 @@ async fn thread_resume_surfaces_cloud_requirements_load_errors() -> Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_resume_prefers_path_over_thread_id() -> Result<()> {
|
||||
async fn thread_resume_uses_path_over_invalid_thread_id() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml(codex_home.path(), &server.uri())?;
|
||||
@@ -2523,13 +2571,6 @@ async fn thread_resume_prefers_path_over_thread_id() -> Result<()> {
|
||||
thread: resumed, ..
|
||||
} = to_response::<ThreadResumeResponse>(resume_resp)?;
|
||||
assert_eq!(resumed.id, thread.id);
|
||||
let resumed_path = resumed.path.as_ref().expect("resumed thread path");
|
||||
let original_path = thread.path.as_ref().expect("original thread path");
|
||||
assert_eq!(
|
||||
normalized_existing_path(resumed_path)?,
|
||||
normalized_existing_path(original_path)?
|
||||
);
|
||||
assert_eq!(resumed.status, ThreadStatus::Idle);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user