mirror of
https://github.com/openai/codex.git
synced 2026-11-10 21:00:47 +00:00
Keep thread storage available when diagnostic logs fail (#60466)
## Why Failure to open the diagnostic logs database should not disable thread storage or database-only history listing. Thread deletion must still preserve saved conversations when diagnostic log cleanup cannot complete. ## What changed - Allow state initialization to continue after non-corruption errors opening the logs database, while preserving corruption recovery. - Report the original diagnostic database error after tracing is installed in the TUI and app server, and return it from log operations. - Reject thread deletion before destructive cleanup when diagnostic storage failed to initialize. - Continue periodic cleanup of legacy logs when the current logs database is unavailable. ## Testing Extend coverage for unavailable log directories, database-only history listing, startup diagnostics, deletion preserving saved conversations, corrupt log headers, and periodic legacy log cleanup during a diagnostic storage outage. GitOrigin-RevId: a5184580ae6c39ed0c4eefa8cb48ee7731a991c3
This commit is contained in:
committed by
copyberry
parent
e1cc96712f
commit
caae161dd0
@@ -774,6 +774,14 @@ pub async fn run_main_with_transport_options(
|
||||
.with(log_db_layer)
|
||||
.with(otel_layers)
|
||||
.try_init();
|
||||
if let Some(error) = state_db.as_ref().and_then(|db| db.diagnostic_logs_error()) {
|
||||
tracing::warn!(
|
||||
event.name = "diagnostic_database_unavailable",
|
||||
error,
|
||||
"diagnostic database unavailable"
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
if let Some(Err(err)) = nofile_limit_result {
|
||||
warn!(%err, "failed to raise managed app-server file descriptor limit");
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
//! Verifies startup recovery preserves corrupt databases and allows thread creation.
|
||||
//! Verifies database failures preserve saved threads and report actionable diagnostics.
|
||||
|
||||
use anyhow::Result;
|
||||
use app_test_support::TestAppServer;
|
||||
@@ -93,6 +93,24 @@ async fn startup_reports_shared_and_fallback_recovery() -> Result<()> {
|
||||
backup_pool.close().await;
|
||||
server.start_thread(ThreadStartParams::default()).await?;
|
||||
timeout(read_timeout, server.shutdown_gracefully()).await??;
|
||||
|
||||
// An unreadable logs header must also reach the outer recovery path.
|
||||
tokio::fs::write(&logs_path, b"damaged logs database").await?;
|
||||
let mut server = TestAppServer::builder()
|
||||
.with_codex_home(sqlite.home())
|
||||
.build_initialized_with_timeout(read_timeout)
|
||||
.await?;
|
||||
let warning: ConfigWarningNotification =
|
||||
timeout(read_timeout, server.read_notification("configWarning")).await??;
|
||||
assert_eq!(warning.summary, "Codex rebuilt its local database");
|
||||
assert!(
|
||||
warning
|
||||
.details
|
||||
.expect("recovery details")
|
||||
.contains(&logs_path.display().to_string())
|
||||
);
|
||||
server.start_thread(ThreadStartParams::default()).await?;
|
||||
timeout(read_timeout, server.shutdown_gracefully()).await??;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
@@ -142,6 +160,64 @@ async fn state_recovery_restores_saved_threads() -> Result<()> {
|
||||
let before: ThreadReadResponse = timeout(read_timeout, server.read_response(request)).await??;
|
||||
timeout(read_timeout, server.shutdown_gracefully()).await??;
|
||||
|
||||
// A logs outage must be visible after startup installs tracing, and must
|
||||
// reject deletion before touching the saved conversation.
|
||||
let config_path = home.path().join("config.toml");
|
||||
let config = std::fs::read_to_string(&config_path)?;
|
||||
let blocked_logs = home.path().join("blocked-logs");
|
||||
std::fs::write(&blocked_logs, b"not a directory")?;
|
||||
std::fs::write(
|
||||
&config_path,
|
||||
format!("log_dir = {blocked_logs:?}\n{config}"),
|
||||
)?;
|
||||
let mut server = TestAppServer::builder()
|
||||
.with_codex_home(sqlite.home())
|
||||
.with_json_logging("warn")
|
||||
.build_initialized_with_timeout(read_timeout)
|
||||
.await?;
|
||||
let warning = server
|
||||
.wait_for_json_log_event("diagnostic_database_unavailable", read_timeout)
|
||||
.await?;
|
||||
let cause = warning["fields"]["error"]
|
||||
.as_str()
|
||||
.expect("original startup error");
|
||||
assert!(cause.contains(&blocked_logs.display().to_string()));
|
||||
assert!(
|
||||
cause.contains(
|
||||
&std::fs::create_dir_all(&blocked_logs)
|
||||
.unwrap_err()
|
||||
.to_string()
|
||||
)
|
||||
);
|
||||
let rollout_path = before.thread.path.as_ref().expect("saved rollout");
|
||||
let rollout = std::fs::read(rollout_path)?;
|
||||
let request = server
|
||||
.send_thread_delete_request(codex_app_server_protocol::ThreadDeleteParams {
|
||||
thread_id: thread_id.clone(),
|
||||
})
|
||||
.await?;
|
||||
let error = timeout(
|
||||
read_timeout,
|
||||
server.read_stream_until_error_message(codex_app_server_protocol::RequestId::Integer(
|
||||
request,
|
||||
)),
|
||||
)
|
||||
.await??;
|
||||
assert_eq!(error.error.code, -32603);
|
||||
assert!(error.error.message.contains(cause));
|
||||
assert_eq!(std::fs::read(rollout_path)?, rollout);
|
||||
let request = server
|
||||
.send_thread_read_request(ThreadReadParams {
|
||||
thread_id: thread_id.clone(),
|
||||
include_turns: true,
|
||||
})
|
||||
.await?;
|
||||
let retained: ThreadReadResponse =
|
||||
timeout(read_timeout, server.read_response(request)).await??;
|
||||
assert_eq!(retained.thread.turns, before.thread.turns);
|
||||
timeout(read_timeout, server.shutdown_gracefully()).await??;
|
||||
std::fs::write(config_path, config)?;
|
||||
|
||||
// Introduce corruption only after the owning process has closed its pools.
|
||||
let pool = sqlite.open_read_write_pool(&sqlite.state_db_path()).await?;
|
||||
sqlx::raw_sql(
|
||||
|
||||
@@ -1176,14 +1176,17 @@ async fn thread_list_db_only_errors_distinguish_unavailable_database_from_empty_
|
||||
let other_home = TempDir::new()?;
|
||||
create_minimal_config(home.path())?;
|
||||
let loader_overrides = LoaderOverrides::without_managed_config_for_tests();
|
||||
let config = Arc::new(
|
||||
ConfigBuilder::default()
|
||||
.codex_home(home.path().to_path_buf())
|
||||
.fallback_cwd(Some(home.path().to_path_buf()))
|
||||
.loader_overrides(loader_overrides.clone())
|
||||
.build()
|
||||
.await?,
|
||||
);
|
||||
let mut config = ConfigBuilder::default()
|
||||
.codex_home(home.path().to_path_buf())
|
||||
.fallback_cwd(Some(home.path().to_path_buf()))
|
||||
.loader_overrides(loader_overrides.clone())
|
||||
.build()
|
||||
.await?;
|
||||
// Diagnostic storage failure must not disable database-only history listing.
|
||||
let logs_home = home.path().join("unavailable-logs");
|
||||
std::fs::write(&logs_home, "not a directory")?;
|
||||
config.sqlite = config.sqlite.clone().with_local_logs_home(logs_home);
|
||||
let config = Arc::new(config);
|
||||
let right_db = codex_state::StateRuntime::init(
|
||||
config.sqlite_config().clone(),
|
||||
config.model_provider_id.clone(),
|
||||
|
||||
Reference in New Issue
Block a user