mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Add app-server thread/delete API (#25018)
## Why Clients can archive and unarchive threads today, but there is no app-server API for permanently removing a thread. Deletion also needs to cover the full session tree: deleting a main thread should remove spawned subagent threads and the related local metadata instead of leaving orphaned rollout files, goals, or subagent state behind. ## What - Adds the v2 `thread/delete` request and `thread/deleted` notification, with the response shape kept consistent with `thread/archive`. - Implements local hard delete for active and archived rollout files. - Deletes the requested thread's state DB row as the commit point, then best-effort cleans associated state including spawned descendants, goals, spawn edges, logs, dynamic tools, and agent job assignments. - Updates app-server API docs and generated protocol schema/TypeScript fixtures.
This commit is contained in:
@@ -79,6 +79,7 @@ use codex_app_server_protocol::SkillsExtraRootsSetParams;
|
||||
use codex_app_server_protocol::SkillsListParams;
|
||||
use codex_app_server_protocol::ThreadArchiveParams;
|
||||
use codex_app_server_protocol::ThreadCompactStartParams;
|
||||
use codex_app_server_protocol::ThreadDeleteParams;
|
||||
use codex_app_server_protocol::ThreadForkParams;
|
||||
use codex_app_server_protocol::ThreadInjectItemsParams;
|
||||
use codex_app_server_protocol::ThreadListParams;
|
||||
@@ -456,6 +457,15 @@ impl TestAppServer {
|
||||
self.send_request("thread/archive", params).await
|
||||
}
|
||||
|
||||
/// Send a `thread/delete` JSON-RPC request.
|
||||
pub async fn send_thread_delete_request(
|
||||
&mut self,
|
||||
params: ThreadDeleteParams,
|
||||
) -> anyhow::Result<i64> {
|
||||
let params = Some(serde_json::to_value(params)?);
|
||||
self.send_request("thread/delete", params).await
|
||||
}
|
||||
|
||||
/// Send a `thread/name/set` JSON-RPC request.
|
||||
pub async fn send_thread_set_name_request(
|
||||
&mut self,
|
||||
|
||||
@@ -50,6 +50,7 @@ mod review;
|
||||
mod safety_check_downgrade;
|
||||
mod skills_list;
|
||||
mod thread_archive;
|
||||
mod thread_delete;
|
||||
mod thread_fork;
|
||||
mod thread_inject_items;
|
||||
mod thread_list;
|
||||
|
||||
@@ -20,6 +20,7 @@ use std::sync::Arc;
|
||||
use anyhow::Result;
|
||||
use app_test_support::create_mock_responses_server_repeating_assistant;
|
||||
use codex_app_server::in_process;
|
||||
use codex_app_server::in_process::InProcessClientHandle;
|
||||
use codex_app_server::in_process::InProcessServerEvent;
|
||||
use codex_app_server::in_process::InProcessStartArgs;
|
||||
use codex_app_server_protocol::ClientInfo;
|
||||
@@ -27,6 +28,8 @@ use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::InitializeParams;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ThreadDeleteParams;
|
||||
use codex_app_server_protocol::ThreadDeleteResponse;
|
||||
use codex_app_server_protocol::ThreadListParams;
|
||||
use codex_app_server_protocol::ThreadListResponse;
|
||||
use codex_app_server_protocol::ThreadResumeParams;
|
||||
@@ -42,8 +45,14 @@ use codex_core::config::Config;
|
||||
use codex_core::config::ConfigBuilder;
|
||||
use codex_exec_server::EnvironmentManager;
|
||||
use codex_feedback::CodexFeedback;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::models::BaseInstructions;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_protocol::protocol::ThreadMemoryMode;
|
||||
use codex_thread_store::CreateThreadParams as StoreCreateThreadParams;
|
||||
use codex_thread_store::InMemoryThreadStore;
|
||||
use codex_thread_store::ThreadPersistenceMetadata;
|
||||
use codex_thread_store::ThreadStore;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::TempDir;
|
||||
use tokio::time::timeout;
|
||||
@@ -52,7 +61,7 @@ use uuid::Uuid;
|
||||
const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10);
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_start_with_non_local_thread_store_does_not_create_local_persistence() -> Result<()>
|
||||
async fn thread_delete_with_non_local_thread_store_does_not_create_local_persistence() -> Result<()>
|
||||
{
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let codex_home = TempDir::new()?;
|
||||
@@ -61,43 +70,10 @@ async fn thread_start_with_non_local_thread_store_does_not_create_local_persiste
|
||||
// here so this regression stays focused on thread persistence artifacts.
|
||||
create_config_toml_with_thread_store(codex_home.path(), &server.uri(), &store_id)?;
|
||||
|
||||
let loader_overrides = LoaderOverrides::without_managed_config_for_tests();
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.path().to_path_buf()))
|
||||
.loader_overrides(loader_overrides.clone())
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
let thread_store = InMemoryThreadStore::for_id(store_id.clone());
|
||||
let _in_memory_store = InMemoryThreadStoreId { store_id };
|
||||
|
||||
let mut client = in_process::start(InProcessStartArgs {
|
||||
arg0_paths: Arg0DispatchPaths::default(),
|
||||
config: Arc::new(config),
|
||||
cli_overrides: Vec::new(),
|
||||
loader_overrides,
|
||||
strict_config: false,
|
||||
cloud_config_bundle: CloudConfigBundleLoader::default(),
|
||||
thread_config_loader: Arc::new(NoopThreadConfigLoader),
|
||||
feedback: CodexFeedback::new(),
|
||||
log_db: None,
|
||||
state_db: None,
|
||||
environment_manager: Arc::new(EnvironmentManager::default_for_tests()),
|
||||
config_warnings: Vec::new(),
|
||||
session_source: SessionSource::Cli,
|
||||
enable_codex_api_key_env: false,
|
||||
initialize: InitializeParams {
|
||||
client_info: ClientInfo {
|
||||
name: "codex-app-server-tests".to_string(),
|
||||
title: None,
|
||||
version: "0.1.0".to_string(),
|
||||
},
|
||||
capabilities: None,
|
||||
},
|
||||
channel_capacity: in_process::DEFAULT_IN_PROCESS_CHANNEL_CAPACITY,
|
||||
})
|
||||
.await?;
|
||||
let mut client = start_in_process_server(codex_home.path()).await?;
|
||||
|
||||
let response = client
|
||||
.request(ClientRequest::ThreadStart {
|
||||
@@ -166,11 +142,39 @@ async fn thread_start_with_non_local_thread_store_does_not_create_local_persiste
|
||||
assert_eq!(data[0].id, thread.id);
|
||||
assert_eq!(data[0].path, None);
|
||||
|
||||
delete_thread(&client, /*request_id*/ 4, thread.id.clone()).await?;
|
||||
let unloaded_thread_id = ThreadId::from_string(&Uuid::new_v4().to_string())?;
|
||||
thread_store
|
||||
.create_thread(StoreCreateThreadParams {
|
||||
thread_id: unloaded_thread_id,
|
||||
extra_config: None,
|
||||
forked_from_id: None,
|
||||
parent_thread_id: None,
|
||||
source: SessionSource::Cli,
|
||||
thread_source: None,
|
||||
base_instructions: BaseInstructions::default(),
|
||||
dynamic_tools: Vec::new(),
|
||||
multi_agent_version: None,
|
||||
metadata: ThreadPersistenceMetadata {
|
||||
cwd: Some(codex_home.path().to_path_buf()),
|
||||
model_provider: "mock_provider".to_string(),
|
||||
memory_mode: ThreadMemoryMode::Enabled,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
delete_thread(
|
||||
&client,
|
||||
/*request_id*/ 5,
|
||||
unloaded_thread_id.to_string(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
client.shutdown().await?;
|
||||
|
||||
let calls = thread_store.calls().await;
|
||||
assert_eq!(calls.create_thread, 1);
|
||||
assert_eq!(calls.create_thread, 2);
|
||||
assert_eq!(calls.list_threads, 1);
|
||||
assert_eq!(calls.delete_thread, 2);
|
||||
assert!(
|
||||
calls.append_items > 0,
|
||||
"turn/start should append rollout items through the injected store"
|
||||
@@ -269,10 +273,24 @@ async fn cold_thread_resume_reuses_non_local_history_probe() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn start_in_process_server(codex_home: &Path) -> Result<InProcessClientHandle> {
|
||||
let loader_overrides = LoaderOverrides::without_managed_config_for_tests();
|
||||
let config = Arc::new(
|
||||
ConfigBuilder::default()
|
||||
.codex_home(codex_home.to_path_buf())
|
||||
.fallback_cwd(Some(codex_home.to_path_buf()))
|
||||
.loader_overrides(loader_overrides.clone())
|
||||
.build()
|
||||
.await?,
|
||||
);
|
||||
|
||||
Ok(start_in_process_client(config, loader_overrides).await?)
|
||||
}
|
||||
|
||||
async fn start_in_process_client(
|
||||
config: Arc<Config>,
|
||||
loader_overrides: LoaderOverrides,
|
||||
) -> std::io::Result<in_process::InProcessClientHandle> {
|
||||
) -> std::io::Result<InProcessClientHandle> {
|
||||
in_process::start(InProcessStartArgs {
|
||||
arg0_paths: Arg0DispatchPaths::default(),
|
||||
config,
|
||||
@@ -301,6 +319,22 @@ async fn start_in_process_client(
|
||||
.await
|
||||
}
|
||||
|
||||
async fn delete_thread(
|
||||
client: &InProcessClientHandle,
|
||||
request_id: i64,
|
||||
thread_id: String,
|
||||
) -> Result<()> {
|
||||
let response = client
|
||||
.request(ClientRequest::ThreadDelete {
|
||||
request_id: RequestId::Integer(request_id),
|
||||
params: ThreadDeleteParams { thread_id },
|
||||
})
|
||||
.await?
|
||||
.map_err(|error| anyhow::anyhow!("thread/delete failed: {}", error.message))?;
|
||||
let _: ThreadDeleteResponse = serde_json::from_value(response)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn assert_no_local_persistence_artifacts(codex_home: &Path) -> Result<()> {
|
||||
// These are the observable tripwires for accidental local persistence. If a
|
||||
// future code path constructs a local rollout/session store or opens the
|
||||
|
||||
@@ -0,0 +1,200 @@
|
||||
use anyhow::Result;
|
||||
use app_test_support::TestAppServer;
|
||||
use app_test_support::create_fake_rollout;
|
||||
use app_test_support::to_response;
|
||||
use codex_app_server_protocol::JSONRPCError;
|
||||
use codex_app_server_protocol::JSONRPCResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::ThreadDeleteParams;
|
||||
use codex_app_server_protocol::ThreadDeleteResponse;
|
||||
use codex_app_server_protocol::ThreadDeletedNotification;
|
||||
use codex_app_server_protocol::ThreadLoadedListParams;
|
||||
use codex_app_server_protocol::ThreadLoadedListResponse;
|
||||
use codex_app_server_protocol::ThreadStartParams;
|
||||
use codex_app_server_protocol::ThreadStartResponse;
|
||||
use codex_core::find_thread_path_by_id_str;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_state::DirectionalThreadSpawnEdgeStatus;
|
||||
use codex_state::StateRuntime;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::path::Path;
|
||||
use tempfile::TempDir;
|
||||
use tokio::time::timeout;
|
||||
|
||||
const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10);
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_delete_deletes_spawned_descendants() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
let parent_id = create_delete_test_rollout(codex_home.path(), /*minute*/ 0, "parent")?;
|
||||
let child_id = create_delete_test_rollout(codex_home.path(), /*minute*/ 1, "child")?;
|
||||
let grandchild_id =
|
||||
create_delete_test_rollout(codex_home.path(), /*minute*/ 2, "grandchild")?;
|
||||
|
||||
let state_db =
|
||||
StateRuntime::init(codex_home.path().to_path_buf(), "mock_provider".into()).await?;
|
||||
let parent_thread_id = ThreadId::from_string(&parent_id)?;
|
||||
let child_thread_id = ThreadId::from_string(&child_id)?;
|
||||
let grandchild_thread_id = ThreadId::from_string(&grandchild_id)?;
|
||||
|
||||
for (parent, child, status) in [
|
||||
(
|
||||
parent_thread_id,
|
||||
child_thread_id,
|
||||
DirectionalThreadSpawnEdgeStatus::Closed,
|
||||
),
|
||||
(
|
||||
child_thread_id,
|
||||
grandchild_thread_id,
|
||||
DirectionalThreadSpawnEdgeStatus::Open,
|
||||
),
|
||||
] {
|
||||
state_db
|
||||
.upsert_thread_spawn_edge(parent, child, status)
|
||||
.await?;
|
||||
}
|
||||
|
||||
let mut mcp = TestAppServer::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let delete_id = mcp
|
||||
.send_thread_delete_request(ThreadDeleteParams {
|
||||
thread_id: parent_id.clone(),
|
||||
})
|
||||
.await?;
|
||||
let delete_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(delete_id)),
|
||||
)
|
||||
.await??;
|
||||
let _: ThreadDeleteResponse = to_response::<ThreadDeleteResponse>(delete_resp)?;
|
||||
|
||||
let mut deleted_ids = Vec::new();
|
||||
for _ in 0..3 {
|
||||
let notification = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("thread/deleted"),
|
||||
)
|
||||
.await??;
|
||||
let deleted_notification: ThreadDeletedNotification = serde_json::from_value(
|
||||
notification
|
||||
.params
|
||||
.expect("thread/deleted notification params"),
|
||||
)?;
|
||||
deleted_ids.push(deleted_notification.thread_id);
|
||||
}
|
||||
assert_eq!(deleted_ids, vec![grandchild_id, child_id, parent_id]);
|
||||
|
||||
for thread_id in [parent_thread_id, child_thread_id, grandchild_thread_id] {
|
||||
let rollout_path = find_thread_path_by_id_str(
|
||||
codex_home.path(),
|
||||
&thread_id.to_string(),
|
||||
/*state_db_ctx*/ None,
|
||||
)
|
||||
.await?;
|
||||
assert!(
|
||||
rollout_path.is_none(),
|
||||
"expected active rollout for {thread_id} to be deleted"
|
||||
);
|
||||
}
|
||||
assert_eq!(
|
||||
state_db
|
||||
.list_thread_spawn_descendants(parent_thread_id)
|
||||
.await?,
|
||||
Vec::<ThreadId>::new()
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn create_delete_test_rollout(codex_home: &Path, minute: u8, preview: &str) -> Result<String> {
|
||||
create_fake_rollout(
|
||||
codex_home,
|
||||
&format!("2025-01-01T00-{minute:02}-00"),
|
||||
&format!("2025-01-01T00:{minute:02}:00Z"),
|
||||
preview,
|
||||
Some("mock_provider"),
|
||||
/*git_info*/ None,
|
||||
)
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_delete_handles_live_threads_before_rollout_exists() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
let mut mcp = TestAppServer::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let start_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams::default())
|
||||
.await?;
|
||||
let start_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(start_id)),
|
||||
)
|
||||
.await??;
|
||||
let persisted_thread = to_response::<ThreadStartResponse>(start_resp)?.thread;
|
||||
let rollout_path = find_thread_path_by_id_str(
|
||||
codex_home.path(),
|
||||
&persisted_thread.id,
|
||||
/*state_db_ctx*/ None,
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(rollout_path, None);
|
||||
|
||||
let delete_id = mcp
|
||||
.send_thread_delete_request(ThreadDeleteParams {
|
||||
thread_id: persisted_thread.id,
|
||||
})
|
||||
.await?;
|
||||
let delete_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(delete_id)),
|
||||
)
|
||||
.await??;
|
||||
let _: ThreadDeleteResponse = to_response::<ThreadDeleteResponse>(delete_resp)?;
|
||||
|
||||
let start_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
ephemeral: Some(true),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let start_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(start_id)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadStartResponse { thread, .. } = to_response::<ThreadStartResponse>(start_resp)?;
|
||||
|
||||
let delete_id = mcp
|
||||
.send_thread_delete_request(ThreadDeleteParams {
|
||||
thread_id: thread.id.clone(),
|
||||
})
|
||||
.await?;
|
||||
let delete_err: JSONRPCError = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_error_message(RequestId::Integer(delete_id)),
|
||||
)
|
||||
.await??;
|
||||
let expected_message = format!(
|
||||
"thread is not persisted and cannot be deleted: {}",
|
||||
thread.id
|
||||
);
|
||||
assert_eq!(delete_err.error.message, expected_message);
|
||||
|
||||
let list_id = mcp
|
||||
.send_thread_loaded_list_request(ThreadLoadedListParams::default())
|
||||
.await?;
|
||||
let list_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(list_id)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadLoadedListResponse { mut data, .. } =
|
||||
to_response::<ThreadLoadedListResponse>(list_resp)?;
|
||||
data.sort();
|
||||
assert_eq!(data, vec![thread.id]);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
@@ -43,7 +43,12 @@ use codex_app_server_protocol::ServerRequest;
|
||||
use codex_app_server_protocol::ServerRequestResolvedNotification;
|
||||
use codex_app_server_protocol::SubAgentActivityKind;
|
||||
use codex_app_server_protocol::TextElement;
|
||||
use codex_app_server_protocol::ThreadDeleteParams;
|
||||
use codex_app_server_protocol::ThreadDeleteResponse;
|
||||
use codex_app_server_protocol::ThreadDeletedNotification;
|
||||
use codex_app_server_protocol::ThreadItem;
|
||||
use codex_app_server_protocol::ThreadLoadedListParams;
|
||||
use codex_app_server_protocol::ThreadLoadedListResponse;
|
||||
use codex_app_server_protocol::ThreadSource;
|
||||
use codex_app_server_protocol::ThreadStartParams;
|
||||
use codex_app_server_protocol::ThreadStartResponse;
|
||||
@@ -3370,6 +3375,49 @@ async fn turn_start_emits_spawn_agent_item_with_model_metadata_v2() -> Result<()
|
||||
assert_eq!(turn_completed.thread_id, thread.id);
|
||||
assert_eq!(turn_completed.turn.id, turn.turn.id);
|
||||
|
||||
// Reuse this live spawn setup to cover thread/delete's ThreadManager descendant path.
|
||||
let delete_req = mcp
|
||||
.send_thread_delete_request(ThreadDeleteParams {
|
||||
thread_id: thread.id.clone(),
|
||||
})
|
||||
.await?;
|
||||
let delete_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(delete_req)),
|
||||
)
|
||||
.await??;
|
||||
let _: ThreadDeleteResponse = to_response::<ThreadDeleteResponse>(delete_resp)?;
|
||||
|
||||
let mut deleted_thread_ids = Vec::new();
|
||||
for _ in 0..2 {
|
||||
let deleted_notif = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("thread/deleted"),
|
||||
)
|
||||
.await??;
|
||||
let deleted: ThreadDeletedNotification = serde_json::from_value(
|
||||
deleted_notif
|
||||
.params
|
||||
.expect("thread/deleted notification params"),
|
||||
)?;
|
||||
deleted_thread_ids.push(deleted.thread_id);
|
||||
}
|
||||
assert_eq!(
|
||||
deleted_thread_ids,
|
||||
vec![receiver_thread_id, thread.id.clone()]
|
||||
);
|
||||
|
||||
let list_req = mcp
|
||||
.send_thread_loaded_list_request(ThreadLoadedListParams::default())
|
||||
.await?;
|
||||
let list_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(list_req)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadLoadedListResponse { data, .. } = to_response::<ThreadLoadedListResponse>(list_resp)?;
|
||||
assert_eq!(data, Vec::<String>::new());
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user