mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Migrate fork and resume reads to thread store (#18900)
- Route cold thread/resume and thread/fork source loading through ThreadStore reads instead of direct rollout path operations - Keep lookups that explicitly specify a rollout-path using the local thread store methods but return an invalid-request error for remote ThreadStore configurations - Add some additional unit tests for code path coverage
This commit is contained in:
@@ -25,6 +25,7 @@ pub use types::GitInfoPatch;
|
||||
pub use types::ListThreadsParams;
|
||||
pub use types::LoadThreadHistoryParams;
|
||||
pub use types::OptionalStringPatch;
|
||||
pub use types::ReadThreadByRolloutPathParams;
|
||||
pub use types::ReadThreadParams;
|
||||
pub use types::ResumeThreadParams;
|
||||
pub use types::SortDirection;
|
||||
|
||||
@@ -27,6 +27,7 @@ use crate::ArchiveThreadParams;
|
||||
use crate::CreateThreadParams;
|
||||
use crate::ListThreadsParams;
|
||||
use crate::LoadThreadHistoryParams;
|
||||
use crate::ReadThreadByRolloutPathParams;
|
||||
use crate::ReadThreadParams;
|
||||
use crate::ResumeThreadParams;
|
||||
use crate::StoredThread;
|
||||
@@ -207,6 +208,19 @@ impl ThreadStore for LocalThreadStore {
|
||||
read_thread::read_thread(self, params).await
|
||||
}
|
||||
|
||||
async fn read_thread_by_rollout_path(
|
||||
&self,
|
||||
params: ReadThreadByRolloutPathParams,
|
||||
) -> ThreadStoreResult<StoredThread> {
|
||||
read_thread::read_thread_by_rollout_path(
|
||||
self,
|
||||
params.rollout_path,
|
||||
params.include_archived,
|
||||
params.include_history,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
async fn list_threads(&self, params: ListThreadsParams) -> ThreadStoreResult<ThreadPage> {
|
||||
list_threads::list_threads(self, params).await
|
||||
}
|
||||
@@ -508,6 +522,51 @@ mod tests {
|
||||
}));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn read_thread_by_rollout_path_includes_history() {
|
||||
let home = TempDir::new().expect("temp dir");
|
||||
let store = LocalThreadStore::new(test_config(home.path()));
|
||||
let thread_id = ThreadId::default();
|
||||
|
||||
store
|
||||
.create_thread(create_thread_params(thread_id))
|
||||
.await
|
||||
.expect("create thread");
|
||||
store
|
||||
.append_items(AppendThreadItemsParams {
|
||||
thread_id,
|
||||
items: vec![user_message_item("path read")],
|
||||
})
|
||||
.await
|
||||
.expect("append item");
|
||||
store.flush_thread(thread_id).await.expect("flush thread");
|
||||
let rollout_path = store
|
||||
.live_rollout_path(thread_id)
|
||||
.await
|
||||
.expect("load rollout path");
|
||||
|
||||
let thread = store
|
||||
.read_thread_by_rollout_path(
|
||||
rollout_path,
|
||||
/*include_archived*/ true,
|
||||
/*include_history*/ true,
|
||||
)
|
||||
.await
|
||||
.expect("read thread by rollout path");
|
||||
|
||||
assert_eq!(thread.thread_id, thread_id);
|
||||
assert_eq!(
|
||||
thread
|
||||
.history
|
||||
.expect("history")
|
||||
.items
|
||||
.into_iter()
|
||||
.filter(|item| matches!(item, RolloutItem::EventMsg(EventMsg::UserMessage(_))))
|
||||
.count(),
|
||||
1
|
||||
);
|
||||
}
|
||||
|
||||
fn create_thread_params(thread_id: ThreadId) -> CreateThreadParams {
|
||||
CreateThreadParams {
|
||||
thread_id,
|
||||
|
||||
@@ -29,6 +29,13 @@ pub(super) async fn read_thread(
|
||||
let thread_id = params.thread_id;
|
||||
if let Some(metadata) = read_sqlite_metadata(store, thread_id).await
|
||||
&& (params.include_archived || metadata.archived_at.is_none())
|
||||
&& (!params.include_history
|
||||
|| sqlite_rollout_path_can_load_history_for_thread(
|
||||
store,
|
||||
&metadata.rollout_path,
|
||||
thread_id,
|
||||
)
|
||||
.await)
|
||||
{
|
||||
let mut thread = stored_thread_from_sqlite_metadata(store, metadata).await;
|
||||
attach_history_if_requested(&mut thread, params.include_history).await?;
|
||||
@@ -46,6 +53,22 @@ pub(super) async fn read_thread(
|
||||
Ok(thread)
|
||||
}
|
||||
|
||||
async fn sqlite_rollout_path_can_load_history_for_thread(
|
||||
store: &LocalThreadStore,
|
||||
path: &std::path::Path,
|
||||
thread_id: codex_protocol::ThreadId,
|
||||
) -> bool {
|
||||
if !tokio::fs::try_exists(path).await.unwrap_or(false) {
|
||||
return false;
|
||||
}
|
||||
// SQLite metadata can outlive a moved/recreated rollout path. When history is
|
||||
// requested, verify the path still resolves to the requested thread before
|
||||
// trusting it as the source replay.
|
||||
read_thread_from_rollout_path(store, path.to_path_buf())
|
||||
.await
|
||||
.is_ok_and(|thread| thread.thread_id == thread_id)
|
||||
}
|
||||
|
||||
pub(super) async fn read_thread_by_rollout_path(
|
||||
store: &LocalThreadStore,
|
||||
rollout_path: std::path::PathBuf,
|
||||
@@ -640,6 +663,102 @@ mod tests {
|
||||
assert_eq!(history.items.len(), 1);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn read_thread_falls_back_to_rollout_search_when_sqlite_path_is_stale() {
|
||||
let home = TempDir::new().expect("temp dir");
|
||||
let external = TempDir::new().expect("external temp dir");
|
||||
let config = test_config(home.path());
|
||||
let store = LocalThreadStore::new(config.clone());
|
||||
let uuid = Uuid::from_u128(220);
|
||||
let thread_id = ThreadId::from_string(&uuid.to_string()).expect("valid thread id");
|
||||
let rollout_path =
|
||||
write_session_file(home.path(), "2025-01-03T12-00-00", uuid).expect("session file");
|
||||
let stale_path = external.path().join("missing-rollout.jsonl");
|
||||
let runtime = codex_state::StateRuntime::init(
|
||||
config.sqlite_home.clone(),
|
||||
config.model_provider_id.clone(),
|
||||
)
|
||||
.await
|
||||
.expect("state db should initialize");
|
||||
let mut builder = ThreadMetadataBuilder::new(
|
||||
thread_id,
|
||||
stale_path.clone(),
|
||||
Utc::now(),
|
||||
SessionSource::Cli,
|
||||
);
|
||||
builder.model_provider = Some("stale-sqlite-provider".to_string());
|
||||
let mut metadata = builder.build(config.model_provider_id.as_str());
|
||||
metadata.first_user_message = Some("stale sqlite preview".to_string());
|
||||
runtime
|
||||
.upsert_thread(&metadata)
|
||||
.await
|
||||
.expect("state db upsert should succeed");
|
||||
|
||||
let thread = store
|
||||
.read_thread(ReadThreadParams {
|
||||
thread_id,
|
||||
include_archived: true,
|
||||
include_history: true,
|
||||
})
|
||||
.await
|
||||
.expect("read thread");
|
||||
|
||||
assert_eq!(thread.thread_id, thread_id);
|
||||
assert_eq!(thread.rollout_path, Some(rollout_path));
|
||||
assert_eq!(thread.preview, "Hello from user");
|
||||
assert_eq!(thread.model_provider, config.model_provider_id);
|
||||
let history = thread.history.expect("history should load");
|
||||
assert_eq!(history.thread_id, thread_id);
|
||||
assert_eq!(history.items.len(), 2);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn read_thread_falls_back_when_sqlite_path_points_to_another_thread() {
|
||||
let home = TempDir::new().expect("temp dir");
|
||||
let external = TempDir::new().expect("external temp dir");
|
||||
let config = test_config(home.path());
|
||||
let store = LocalThreadStore::new(config.clone());
|
||||
let uuid = Uuid::from_u128(221);
|
||||
let thread_id = ThreadId::from_string(&uuid.to_string()).expect("valid thread id");
|
||||
let rollout_path =
|
||||
write_session_file(home.path(), "2025-01-03T12-00-00", uuid).expect("session file");
|
||||
let other_uuid = Uuid::from_u128(222);
|
||||
let stale_path = write_session_file(external.path(), "2025-01-04T12-00-00", other_uuid)
|
||||
.expect("other session file");
|
||||
let runtime = codex_state::StateRuntime::init(
|
||||
config.sqlite_home.clone(),
|
||||
config.model_provider_id.clone(),
|
||||
)
|
||||
.await
|
||||
.expect("state db should initialize");
|
||||
let mut builder =
|
||||
ThreadMetadataBuilder::new(thread_id, stale_path, Utc::now(), SessionSource::Cli);
|
||||
builder.model_provider = Some("wrong-sqlite-provider".to_string());
|
||||
let mut metadata = builder.build(config.model_provider_id.as_str());
|
||||
metadata.first_user_message = Some("wrong sqlite preview".to_string());
|
||||
runtime
|
||||
.upsert_thread(&metadata)
|
||||
.await
|
||||
.expect("state db upsert should succeed");
|
||||
|
||||
let thread = store
|
||||
.read_thread(ReadThreadParams {
|
||||
thread_id,
|
||||
include_archived: true,
|
||||
include_history: true,
|
||||
})
|
||||
.await
|
||||
.expect("read thread");
|
||||
|
||||
assert_eq!(thread.thread_id, thread_id);
|
||||
assert_eq!(thread.rollout_path, Some(rollout_path));
|
||||
assert_eq!(thread.preview, "Hello from user");
|
||||
assert_eq!(thread.model_provider, config.model_provider_id);
|
||||
let history = thread.history.expect("history should load");
|
||||
assert_eq!(history.thread_id, thread_id);
|
||||
assert_eq!(history.items.len(), 2);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn read_thread_uses_session_meta_for_rollout_without_user_preview_or_sqlite_metadata() {
|
||||
let home = TempDir::new().expect("temp dir");
|
||||
|
||||
@@ -9,6 +9,7 @@ use crate::ArchiveThreadParams;
|
||||
use crate::CreateThreadParams;
|
||||
use crate::ListThreadsParams;
|
||||
use crate::LoadThreadHistoryParams;
|
||||
use crate::ReadThreadByRolloutPathParams;
|
||||
use crate::ReadThreadParams;
|
||||
use crate::ResumeThreadParams;
|
||||
use crate::StoredThread;
|
||||
@@ -25,6 +26,10 @@ mod proto;
|
||||
|
||||
/// gRPC-backed [`ThreadStore`] implementation for deployments whose durable thread data lives
|
||||
/// outside the app-server process.
|
||||
///
|
||||
/// This store is still a work in progress: app-server code should call the generic
|
||||
/// [`ThreadStore`] methods, and unsupported remote operations will return explicit
|
||||
/// `not_implemented` errors until the remote API catches up.
|
||||
#[derive(Clone, Debug)]
|
||||
pub struct RemoteThreadStore {
|
||||
endpoint: String,
|
||||
@@ -187,6 +192,15 @@ impl ThreadStore for RemoteThreadStore {
|
||||
helpers::stored_thread_from_proto(thread)
|
||||
}
|
||||
|
||||
async fn read_thread_by_rollout_path(
|
||||
&self,
|
||||
_params: ReadThreadByRolloutPathParams,
|
||||
) -> ThreadStoreResult<StoredThread> {
|
||||
Err(ThreadStoreError::Internal {
|
||||
message: "remote thread store does not support read_thread_by_rollout_path".to_string(),
|
||||
})
|
||||
}
|
||||
|
||||
async fn list_threads(&self, params: ListThreadsParams) -> ThreadStoreResult<ThreadPage> {
|
||||
list_threads::list_threads(self, params).await
|
||||
}
|
||||
|
||||
@@ -7,6 +7,7 @@ use crate::ArchiveThreadParams;
|
||||
use crate::CreateThreadParams;
|
||||
use crate::ListThreadsParams;
|
||||
use crate::LoadThreadHistoryParams;
|
||||
use crate::ReadThreadByRolloutPathParams;
|
||||
use crate::ReadThreadParams;
|
||||
use crate::ResumeThreadParams;
|
||||
use crate::StoredThread;
|
||||
@@ -18,8 +19,7 @@ use crate::UpdateThreadMetadataParams;
|
||||
/// Storage-neutral thread persistence boundary.
|
||||
#[async_trait]
|
||||
pub trait ThreadStore: Any + Send + Sync {
|
||||
/// Return this store as [`Any`] so callers at API boundaries can reject requests that only
|
||||
/// make sense for a concrete store implementation.
|
||||
/// Return this store as [`Any`] for implementation-owned escape hatches.
|
||||
fn as_any(&self) -> &dyn Any;
|
||||
|
||||
/// Creates a new live thread.
|
||||
@@ -56,6 +56,14 @@ pub trait ThreadStore: Any + Send + Sync {
|
||||
/// Reads a thread summary and optionally its persisted history.
|
||||
async fn read_thread(&self, params: ReadThreadParams) -> ThreadStoreResult<StoredThread>;
|
||||
|
||||
/// Reads a rollout-backed thread by path when the store supports path-addressed lookups.
|
||||
///
|
||||
/// Deprecated: new callers should use [`ThreadStore::read_thread`] instead.
|
||||
async fn read_thread_by_rollout_path(
|
||||
&self,
|
||||
params: ReadThreadByRolloutPathParams,
|
||||
) -> ThreadStoreResult<StoredThread>;
|
||||
|
||||
/// Lists stored threads matching the supplied filters.
|
||||
async fn list_threads(&self, params: ListThreadsParams) -> ThreadStoreResult<ThreadPage>;
|
||||
|
||||
|
||||
@@ -96,6 +96,17 @@ pub struct ReadThreadParams {
|
||||
pub include_history: bool,
|
||||
}
|
||||
|
||||
/// Parameters for reading a local rollout-backed thread by path.
|
||||
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
|
||||
pub struct ReadThreadByRolloutPathParams {
|
||||
/// Local rollout JSONL path to read.
|
||||
pub rollout_path: PathBuf,
|
||||
/// Whether archived threads are eligible.
|
||||
pub include_archived: bool,
|
||||
/// Whether persisted rollout items should be included in the response.
|
||||
pub include_history: bool,
|
||||
}
|
||||
|
||||
/// The sort key to use when listing stored threads.
|
||||
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Serialize, Deserialize)]
|
||||
pub enum ThreadSortKey {
|
||||
|
||||
Reference in New Issue
Block a user