mirror of
https://github.com/openai/codex.git
synced 2026-10-22 17:58:46 +00:00
## Why A corrupt agent message-board database can block thread deletion even when the feature is disabled. ## What changed When corruption prevents opening the database for board deletion, back it up and recreate it before recording deletion tombstones. Recovery resets all boards, retaining their previous storage in the backup. Other database open failures continue to return an error without deleting the thread or replacing storage. ## Testing Add an app-server regression test with the feature disabled that verifies ordinary open failures preserve storage and the thread, while corruption recovery preserves the backup, deletes the requested thread, retains another thread, and records the board tombstone in the recreated database. GitOrigin-RevId: 85f7d6303ae5c359782018cad4a78461d86c43db
514 lines
17 KiB
Rust
514 lines
17 KiB
Rust
use anyhow::Result;
|
|
use app_test_support::MockResponsesConfig;
|
|
use app_test_support::TestAppServer;
|
|
use app_test_support::create_fake_paginated_rollout;
|
|
use app_test_support::create_fake_rollout;
|
|
use app_test_support::create_mock_responses_server_repeating_assistant;
|
|
use app_test_support::create_mock_responses_server_sequence;
|
|
use codex_app_server_protocol::ClientRequest;
|
|
use codex_app_server_protocol::JSONRPCError;
|
|
use codex_app_server_protocol::RequestId;
|
|
use codex_app_server_protocol::ThreadArchiveParams;
|
|
use codex_app_server_protocol::ThreadArchiveResponse;
|
|
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::ThreadResumeParams;
|
|
use codex_app_server_protocol::ThreadResumeResponse;
|
|
use codex_app_server_protocol::ThreadStartParams;
|
|
use codex_app_server_protocol::ThreadStartResponse;
|
|
use codex_app_server_protocol::TurnStartParams;
|
|
use codex_app_server_protocol::TurnStatus;
|
|
use codex_app_server_protocol::UserInput;
|
|
use codex_core::find_thread_path_by_id_str;
|
|
use codex_features::Feature;
|
|
use codex_protocol::ThreadId;
|
|
use codex_protocol::protocol::HistoryPosition;
|
|
use codex_state::DirectionalThreadSpawnEdgeStatus;
|
|
use codex_state::SqliteConfig;
|
|
use codex_state::StateRuntime;
|
|
use codex_utils_absolute_path::test_support::PathExt;
|
|
use core_test_support::responses;
|
|
use pretty_assertions::assert_eq;
|
|
use serde_json::json;
|
|
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_rejects_paginated_writer_owned_by_another_process() -> Result<()> {
|
|
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
|
let codex_home = TempDir::new()?;
|
|
MockResponsesConfig::new(&server.uri()).write(codex_home.path())?;
|
|
let thread_id = create_fake_paginated_rollout(
|
|
codex_home.path(),
|
|
"2025-01-01T00-00-00",
|
|
"2025-01-01T00:00:00Z",
|
|
"owned",
|
|
Some("mock_provider"),
|
|
/*git_info*/ None,
|
|
)?;
|
|
let mut owner = TestAppServer::builder()
|
|
.with_codex_home(codex_home.path())
|
|
.build_initialized()
|
|
.await?;
|
|
let _: ThreadResumeResponse = owner
|
|
.request(|request_id| ClientRequest::ThreadResume {
|
|
request_id,
|
|
params: ThreadResumeParams {
|
|
thread_id: thread_id.clone(),
|
|
exclude_turns: true,
|
|
..Default::default()
|
|
},
|
|
})
|
|
.await?;
|
|
|
|
let mut other = TestAppServer::builder()
|
|
.with_codex_home(codex_home.path())
|
|
.build_initialized()
|
|
.await?;
|
|
let request_id = other
|
|
.send_thread_delete_request(ThreadDeleteParams {
|
|
thread_id: thread_id.clone(),
|
|
})
|
|
.await?;
|
|
let error: JSONRPCError = timeout(
|
|
DEFAULT_READ_TIMEOUT,
|
|
other.read_stream_until_error_message(RequestId::Integer(request_id)),
|
|
)
|
|
.await??;
|
|
assert_eq!(error.error.code, -32600);
|
|
assert_eq!(
|
|
error.error.message,
|
|
format!("thread {thread_id} already has an active writer")
|
|
);
|
|
timeout(DEFAULT_READ_TIMEOUT, owner.shutdown_gracefully()).await??;
|
|
let _: ThreadDeleteResponse = other
|
|
.request(|request_id| ClientRequest::ThreadDelete {
|
|
request_id,
|
|
params: ThreadDeleteParams { thread_id },
|
|
})
|
|
.await?;
|
|
Ok(())
|
|
}
|
|
|
|
#[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_state::SqliteConfig::new_for_testing(codex_home.path().abs()),
|
|
"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::builder()
|
|
.with_codex_home(codex_home.path())
|
|
.without_auto_env()
|
|
.build_initialized()
|
|
.await?;
|
|
|
|
let _: ThreadDeleteResponse = mcp
|
|
.request(|request_id| ClientRequest::ThreadDelete {
|
|
request_id,
|
|
params: ThreadDeleteParams {
|
|
thread_id: parent_id.clone(),
|
|
},
|
|
})
|
|
.await?;
|
|
|
|
let mut deleted_ids = Vec::new();
|
|
for _ in 0..3 {
|
|
let deleted_notification: ThreadDeletedNotification = timeout(
|
|
DEFAULT_READ_TIMEOUT,
|
|
mcp.read_notification("thread/deleted"),
|
|
)
|
|
.await??;
|
|
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] {
|
|
assert_eq!(state_db.get_thread(thread_id).await?, None);
|
|
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(())
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn thread_delete_preflights_external_fork_references_for_spawned_subtrees() -> 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 external_id = create_delete_test_rollout(codex_home.path(), /*minute*/ 2, "external")?;
|
|
let parent_thread_id = ThreadId::from_string(&parent_id)?;
|
|
let child_thread_id = ThreadId::from_string(&child_id)?;
|
|
let external_thread_id = ThreadId::from_string(&external_id)?;
|
|
let parent_path = find_thread_path_by_id_str(
|
|
codex_home.path(),
|
|
&parent_thread_id.to_string(),
|
|
/*state_db_ctx*/ None,
|
|
)
|
|
.await?
|
|
.expect("parent rollout path");
|
|
let external_path = find_thread_path_by_id_str(
|
|
codex_home.path(),
|
|
&external_thread_id.to_string(),
|
|
/*state_db_ctx*/ None,
|
|
)
|
|
.await?
|
|
.expect("external rollout path");
|
|
let mut external_meta: serde_json::Value = serde_json::from_str(
|
|
std::fs::read_to_string(external_path.as_path())?
|
|
.lines()
|
|
.next()
|
|
.expect("external session metadata"),
|
|
)?;
|
|
external_meta["payload"]["history_base"] = serde_json::to_value(HistoryPosition {
|
|
thread_id: parent_thread_id,
|
|
end_ordinal_exclusive: 1,
|
|
end_byte_offset: std::fs::metadata(parent_path.as_path())?.len(),
|
|
})?;
|
|
std::fs::write(external_path.as_path(), format!("{external_meta}\n"))?;
|
|
|
|
let state_db = StateRuntime::init(
|
|
SqliteConfig::new_for_testing(codex_home.path().abs()),
|
|
"mock_provider".into(),
|
|
)
|
|
.await?;
|
|
state_db
|
|
.upsert_thread_spawn_edge(
|
|
parent_thread_id,
|
|
child_thread_id,
|
|
DirectionalThreadSpawnEdgeStatus::Closed,
|
|
)
|
|
.await?;
|
|
|
|
let mut mcp = TestAppServer::builder()
|
|
.with_codex_home(codex_home.path())
|
|
.without_auto_env()
|
|
.build_initialized()
|
|
.await?;
|
|
|
|
let delete_id = mcp
|
|
.send_thread_delete_request(ThreadDeleteParams {
|
|
thread_id: parent_id.clone(),
|
|
})
|
|
.await?;
|
|
let delete_err: JSONRPCError = timeout(
|
|
DEFAULT_READ_TIMEOUT,
|
|
mcp.read_stream_until_error_message(RequestId::Integer(delete_id)),
|
|
)
|
|
.await??;
|
|
assert_eq!(
|
|
delete_err.error.message,
|
|
format!("cannot delete thread {parent_thread_id}: forked history still references it")
|
|
);
|
|
|
|
for thread_id in [parent_thread_id, child_thread_id, external_thread_id] {
|
|
assert!(
|
|
find_thread_path_by_id_str(
|
|
codex_home.path(),
|
|
&thread_id.to_string(),
|
|
/*state_db_ctx*/ None,
|
|
)
|
|
.await?
|
|
.is_some(),
|
|
"expected rollout for {thread_id} to remain"
|
|
);
|
|
}
|
|
assert_eq!(
|
|
state_db
|
|
.list_thread_spawn_descendants(parent_thread_id)
|
|
.await?,
|
|
vec![child_thread_id]
|
|
);
|
|
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::builder()
|
|
.with_codex_home(codex_home.path())
|
|
.build_initialized()
|
|
.await?;
|
|
|
|
let persisted_thread = mcp.start_thread(ThreadStartParams::default()).await?.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 _: ThreadDeleteResponse = mcp
|
|
.request(|request_id| ClientRequest::ThreadDelete {
|
|
request_id,
|
|
params: ThreadDeleteParams {
|
|
thread_id: persisted_thread.id,
|
|
},
|
|
})
|
|
.await?;
|
|
|
|
let ThreadStartResponse { thread, .. } = mcp
|
|
.start_thread(ThreadStartParams {
|
|
ephemeral: Some(true),
|
|
..Default::default()
|
|
})
|
|
.await?;
|
|
|
|
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 ThreadLoadedListResponse { mut data, .. } = mcp
|
|
.request(|request_id| ClientRequest::ThreadLoadedList {
|
|
request_id,
|
|
params: ThreadLoadedListParams::default(),
|
|
})
|
|
.await?;
|
|
data.sort();
|
|
assert_eq!(data, vec![thread.id]);
|
|
|
|
Ok(())
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn thread_delete_removes_persisted_board_even_with_feature_disabled() -> Result<()> {
|
|
let server = create_mock_responses_server_sequence(vec![
|
|
responses::sse(vec![
|
|
responses::ev_function_call_with_namespace(
|
|
"post",
|
|
"collaboration",
|
|
"post",
|
|
&json!({"new_channel_name":"design", "text":"Persist this decision"}).to_string(),
|
|
),
|
|
responses::ev_completed("post"),
|
|
]),
|
|
responses::sse(vec![
|
|
responses::ev_assistant_message("done", "Done"),
|
|
responses::ev_completed("done"),
|
|
]),
|
|
])
|
|
.await;
|
|
let home = TempDir::new()?;
|
|
MockResponsesConfig::new(&server.uri())
|
|
.enable_feature(Feature::AgentMessageBoard)
|
|
.enable_feature(Feature::MultiAgentV2)
|
|
.write(home.path())?;
|
|
let mut app = TestAppServer::builder()
|
|
.with_codex_home(home.path())
|
|
.build_initialized()
|
|
.await?;
|
|
let thread = app.start_thread(ThreadStartParams::default()).await?.thread;
|
|
let completed = app
|
|
.start_turn_and_wait_for_completion(TurnStartParams {
|
|
thread_id: thread.id.clone(),
|
|
input: vec![UserInput::Text {
|
|
text: "Post the decision".into(),
|
|
text_elements: Vec::new(),
|
|
}],
|
|
..Default::default()
|
|
})
|
|
.await?;
|
|
assert_eq!(completed.turn.status, TurnStatus::Completed);
|
|
|
|
let sqlite = SqliteConfig::new_for_testing(home.path().abs());
|
|
let pool = sqlite
|
|
.open_read_only_pool(
|
|
&sqlite.home().join("agent_message_board_1.sqlite"),
|
|
/*busy_timeout*/ None,
|
|
)
|
|
.await?;
|
|
assert_eq!(board_counts(&pool).await?, (1, 1, 1));
|
|
let _: ThreadArchiveResponse = app
|
|
.request(|request_id| ClientRequest::ThreadArchive {
|
|
request_id,
|
|
params: ThreadArchiveParams {
|
|
thread_id: thread.id.clone(),
|
|
},
|
|
})
|
|
.await?;
|
|
app.shutdown_gracefully().await?;
|
|
|
|
MockResponsesConfig::new(&server.uri())
|
|
.disable_feature(Feature::AgentMessageBoard)
|
|
.write(home.path())?;
|
|
let mut app = TestAppServer::builder()
|
|
.with_codex_home(home.path())
|
|
.build_initialized()
|
|
.await?;
|
|
assert_eq!(board_counts(&pool).await?, (1, 1, 1));
|
|
let _: ThreadDeleteResponse = app
|
|
.request(|request_id| ClientRequest::ThreadDelete {
|
|
request_id,
|
|
params: ThreadDeleteParams {
|
|
thread_id: thread.id,
|
|
},
|
|
})
|
|
.await?;
|
|
assert_eq!(board_counts(&pool).await?, (0, 0, 0));
|
|
app.shutdown_gracefully().await?;
|
|
Ok(())
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn thread_delete_recovers_only_corrupt_board_with_feature_disabled() -> Result<()> {
|
|
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
|
let home = TempDir::new()?;
|
|
MockResponsesConfig::new(&server.uri())
|
|
.disable_feature(Feature::AgentMessageBoard)
|
|
.write(home.path())?;
|
|
let thread_id = create_delete_test_rollout(home.path(), /*minute*/ 0, "Delete me")?;
|
|
let retained_id = create_delete_test_rollout(home.path(), /*minute*/ 1, "Keep me")?;
|
|
let corrupt = b"not a SQLite database";
|
|
let database = home.path().join("agent_message_board_1.sqlite");
|
|
std::fs::create_dir(&database)?;
|
|
let mut app = TestAppServer::builder()
|
|
.with_codex_home(home.path())
|
|
.build_initialized()
|
|
.await?;
|
|
|
|
// An ordinary open failure must leave both storage and the thread intact.
|
|
let request_id = app
|
|
.send_thread_delete_request(ThreadDeleteParams {
|
|
thread_id: thread_id.clone(),
|
|
})
|
|
.await?;
|
|
let error = timeout(
|
|
DEFAULT_READ_TIMEOUT,
|
|
app.read_stream_until_error_message(RequestId::Integer(request_id)),
|
|
)
|
|
.await??;
|
|
assert_eq!(error.error.code, -32603);
|
|
assert!(database.is_dir());
|
|
assert!(!home.path().join("db-backups").exists());
|
|
assert!(
|
|
find_thread_path_by_id_str(home.path(), &thread_id, /*state_db_ctx*/ None)
|
|
.await?
|
|
.is_some()
|
|
);
|
|
std::fs::remove_dir(&database)?;
|
|
std::fs::write(&database, corrupt)?;
|
|
|
|
let _: ThreadDeleteResponse = app
|
|
.request(|request_id| ClientRequest::ThreadDelete {
|
|
request_id,
|
|
params: ThreadDeleteParams {
|
|
thread_id: thread_id.clone(),
|
|
},
|
|
})
|
|
.await?;
|
|
assert_eq!(
|
|
find_thread_path_by_id_str(home.path(), &thread_id, /*state_db_ctx*/ None).await?,
|
|
None
|
|
);
|
|
assert!(
|
|
find_thread_path_by_id_str(home.path(), &retained_id, /*state_db_ctx*/ None)
|
|
.await?
|
|
.is_some()
|
|
);
|
|
let backups =
|
|
std::fs::read_dir(home.path().join("db-backups"))?.collect::<std::io::Result<Vec<_>>>()?;
|
|
assert_eq!(backups.len(), 1);
|
|
assert_eq!(
|
|
std::fs::read(backups[0].path().join("agent_message_board_1.sqlite"))?,
|
|
corrupt
|
|
);
|
|
let sqlite = SqliteConfig::new_for_testing(home.path().abs());
|
|
let pool = sqlite
|
|
.open_read_only_pool(&database, /*busy_timeout*/ None)
|
|
.await?;
|
|
assert_eq!(board_counts(&pool).await?, (0, 0, 0));
|
|
assert_eq!(
|
|
sqlx::query_scalar::<_, String>("SELECT board FROM deleted_boards")
|
|
.fetch_all(&pool)
|
|
.await?,
|
|
vec![thread_id]
|
|
);
|
|
pool.close().await;
|
|
app.shutdown_gracefully().await?;
|
|
Ok(())
|
|
}
|
|
|
|
async fn board_counts(pool: &sqlx::SqlitePool) -> Result<(i64, i64, i64)> {
|
|
Ok(sqlx::query_as(
|
|
"SELECT (SELECT COUNT(*) FROM channels),
|
|
(SELECT COUNT(*) FROM posts),
|
|
(SELECT COUNT(*) FROM subscriptions)",
|
|
)
|
|
.fetch_one(pool)
|
|
.await?)
|
|
}
|