mirror of
https://github.com/openai/codex.git
synced 2026-11-03 19:53:25 +00:00
Preserve thread metadata when rollout files are missing (#35644)
## What changed - Skip threads whose rollout files cannot be found while continuing through database pages until the requested number of valid threads is collected. - Retain the skipped threads and their rollout paths in the state database instead of deleting their metadata during listing. ## Testing - Update the state-backed listing test to verify that a missing rollout path does not consume a page slot or remove the stored thread metadata. GitOrigin-RevId: 51c03a78434b0aeeb5a2dc685b97243b6ad958bf
This commit is contained in:
@@ -1007,7 +1007,8 @@ async fn list_threads_db_disabled_does_not_skip_paginated_items() -> std::io::Re
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn list_threads_db_enabled_drops_missing_rollout_paths() -> std::io::Result<()> {
|
||||
async fn list_threads_db_enabled_preserves_metadata_for_missing_rollout_paths()
|
||||
-> std::io::Result<()> {
|
||||
let home = TempDir::new().expect("temp dir");
|
||||
let config = test_config(home.path());
|
||||
|
||||
@@ -1047,11 +1048,34 @@ async fn list_threads_db_enabled_drops_missing_rollout_paths() -> std::io::Resul
|
||||
.await
|
||||
.expect("state db upsert should succeed");
|
||||
|
||||
let valid_uuid = Uuid::from_u128(9011);
|
||||
let valid_thread_id = ThreadId::from_string(&valid_uuid.to_string()).expect("valid thread id");
|
||||
let valid_path = write_session_file(home.path(), "2025-01-02T13-00-00", valid_uuid)?;
|
||||
let valid_created_at = chrono::Utc
|
||||
.with_ymd_and_hms(2025, 1, 2, 13, 0, 0)
|
||||
.single()
|
||||
.expect("valid datetime");
|
||||
let mut valid_builder = codex_state::ThreadMetadataBuilder::new(
|
||||
valid_thread_id,
|
||||
valid_path.clone(),
|
||||
valid_created_at,
|
||||
SessionSource::Cli,
|
||||
);
|
||||
valid_builder.model_provider = Some(config.model_provider_id.clone());
|
||||
valid_builder.cwd = home.path().to_path_buf();
|
||||
let mut valid_metadata = valid_builder.build(config.model_provider_id.as_str());
|
||||
valid_metadata.first_user_message = Some("Older valid thread".to_string());
|
||||
valid_metadata.preview = valid_metadata.first_user_message.clone();
|
||||
runtime
|
||||
.upsert_thread(&valid_metadata)
|
||||
.await
|
||||
.expect("state db upsert should succeed");
|
||||
|
||||
let default_provider = config.model_provider_id.clone();
|
||||
let page = RolloutRecorder::list_threads(
|
||||
Some(runtime.clone()),
|
||||
&config,
|
||||
/*page_size*/ 10,
|
||||
/*page_size*/ 1,
|
||||
/*cursor*/ None,
|
||||
ThreadSortKey::CreatedAt,
|
||||
SortDirection::Desc,
|
||||
@@ -1062,12 +1086,18 @@ async fn list_threads_db_enabled_drops_missing_rollout_paths() -> std::io::Resul
|
||||
/*search_term*/ None,
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(page.items.len(), 0);
|
||||
assert_eq!(page.items.len(), 1);
|
||||
assert_eq!(page.items[0].path, valid_path);
|
||||
let stored_path = runtime
|
||||
.find_rollout_path_by_id(thread_id, Some(false))
|
||||
.await
|
||||
.expect("state db lookup should succeed");
|
||||
assert_eq!(stored_path, None);
|
||||
assert_eq!(stored_path, Some(metadata.rollout_path.clone()));
|
||||
let stored_metadata = runtime
|
||||
.get_thread(thread_id)
|
||||
.await
|
||||
.expect("state db lookup should succeed");
|
||||
assert_eq!(stored_metadata, Some(metadata));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -374,7 +374,7 @@ pub async fn list_threads_db(
|
||||
return None;
|
||||
}
|
||||
|
||||
let anchor = cursor_to_anchor(cursor);
|
||||
let mut anchor = cursor_to_anchor(cursor);
|
||||
let allowed_sources: Vec<String> = allowed_sources
|
||||
.iter()
|
||||
.map(|value| match serde_json::to_value(value) {
|
||||
@@ -390,63 +390,93 @@ pub async fn list_threads_db(
|
||||
.map(|cwd| normalize_cwd_for_state_db(cwd))
|
||||
.collect::<Vec<_>>()
|
||||
});
|
||||
let filters = codex_state::ThreadFilterOptions {
|
||||
archived_only: archived,
|
||||
is_pinned,
|
||||
allowed_sources: allowed_sources.as_slice(),
|
||||
model_providers: model_providers.as_deref(),
|
||||
cwd_filters: normalized_cwd_filters.as_deref(),
|
||||
anchor: anchor.as_ref(),
|
||||
sort_key: match sort_key {
|
||||
ThreadSortKey::CreatedAt => codex_state::SortKey::CreatedAt,
|
||||
ThreadSortKey::UpdatedAt => codex_state::SortKey::UpdatedAt,
|
||||
ThreadSortKey::RecencyAt => codex_state::SortKey::RecencyAt,
|
||||
},
|
||||
sort_direction: match sort_direction {
|
||||
SortDirection::Asc => codex_state::SortDirection::Asc,
|
||||
SortDirection::Desc => codex_state::SortDirection::Desc,
|
||||
},
|
||||
search_term,
|
||||
let state_sort_key = match sort_key {
|
||||
ThreadSortKey::CreatedAt => codex_state::SortKey::CreatedAt,
|
||||
ThreadSortKey::UpdatedAt => codex_state::SortKey::UpdatedAt,
|
||||
ThreadSortKey::RecencyAt => codex_state::SortKey::RecencyAt,
|
||||
};
|
||||
let page = match relation_filter {
|
||||
Some(relation_filter) => {
|
||||
ctx.list_threads_by_relation(page_size, relation_filter, filters)
|
||||
.await
|
||||
}
|
||||
None => ctx.list_threads(page_size, filters).await,
|
||||
let state_sort_direction = match sort_direction {
|
||||
SortDirection::Asc => codex_state::SortDirection::Asc,
|
||||
SortDirection::Desc => codex_state::SortDirection::Desc,
|
||||
};
|
||||
match page {
|
||||
Ok(mut page) => {
|
||||
// Relationship-filtered listings intentionally treat persisted state as authoritative.
|
||||
if relation_filter.is_some() {
|
||||
return Some(page);
|
||||
|
||||
if let Some(relation_filter) = relation_filter {
|
||||
let filters = codex_state::ThreadFilterOptions {
|
||||
archived_only: archived,
|
||||
is_pinned,
|
||||
allowed_sources: allowed_sources.as_slice(),
|
||||
model_providers: model_providers.as_deref(),
|
||||
cwd_filters: normalized_cwd_filters.as_deref(),
|
||||
anchor: anchor.as_ref(),
|
||||
sort_key: state_sort_key,
|
||||
sort_direction: state_sort_direction,
|
||||
search_term,
|
||||
};
|
||||
return match ctx
|
||||
.list_threads_by_relation(page_size, relation_filter, filters)
|
||||
.await
|
||||
{
|
||||
Ok(page) => Some(page),
|
||||
Err(err) => {
|
||||
warn!("state db list_threads failed: {err}");
|
||||
None
|
||||
}
|
||||
let mut valid_items = Vec::with_capacity(page.items.len());
|
||||
for item in page.items {
|
||||
if let Some(existing_path) =
|
||||
crate::compression::existing_rollout_path(item.rollout_path.as_path()).await
|
||||
{
|
||||
let mut item = item;
|
||||
item.rollout_path = existing_path;
|
||||
valid_items.push(item);
|
||||
} else {
|
||||
warn!(
|
||||
"state db list_threads returned stale rollout path for thread {}: {}",
|
||||
item.id,
|
||||
item.rollout_path.display()
|
||||
);
|
||||
warn!("state db discrepancy during list_threads_db: stale_db_path_dropped");
|
||||
let _ = ctx.delete_thread(item.id).await;
|
||||
}
|
||||
}
|
||||
page.items = valid_items;
|
||||
Some(page)
|
||||
}
|
||||
Err(err) => {
|
||||
warn!("state db list_threads failed: {err}");
|
||||
None
|
||||
}
|
||||
};
|
||||
}
|
||||
|
||||
let mut items = Vec::with_capacity(page_size);
|
||||
let mut next_anchor = None;
|
||||
let mut num_scanned_rows = 0usize;
|
||||
while items.len() < page_size {
|
||||
let filters = codex_state::ThreadFilterOptions {
|
||||
archived_only: archived,
|
||||
is_pinned,
|
||||
allowed_sources: allowed_sources.as_slice(),
|
||||
model_providers: model_providers.as_deref(),
|
||||
cwd_filters: normalized_cwd_filters.as_deref(),
|
||||
anchor: anchor.as_ref(),
|
||||
sort_key: state_sort_key,
|
||||
sort_direction: state_sort_direction,
|
||||
search_term,
|
||||
};
|
||||
let page = match ctx.list_threads(page_size - items.len(), filters).await {
|
||||
Ok(page) => page,
|
||||
Err(err) => {
|
||||
warn!("state db list_threads failed: {err}");
|
||||
return None;
|
||||
}
|
||||
};
|
||||
num_scanned_rows = num_scanned_rows.saturating_add(page.num_scanned_rows);
|
||||
next_anchor = page.next_anchor;
|
||||
|
||||
for item in page.items {
|
||||
if let Some(existing_path) =
|
||||
crate::compression::existing_rollout_path(item.rollout_path.as_path()).await
|
||||
{
|
||||
let mut item = item;
|
||||
item.rollout_path = existing_path;
|
||||
items.push(item);
|
||||
} else {
|
||||
warn!(
|
||||
"state db list_threads returned stale rollout path for thread {}: {}",
|
||||
item.id,
|
||||
item.rollout_path.display()
|
||||
);
|
||||
warn!("state db discrepancy during list_threads_db: stale_db_path_retained");
|
||||
}
|
||||
}
|
||||
|
||||
if items.len() == page_size || next_anchor.is_none() {
|
||||
break;
|
||||
}
|
||||
anchor = next_anchor.clone();
|
||||
}
|
||||
Some(codex_state::ThreadsPage {
|
||||
items,
|
||||
parent_thread_ids: Default::default(),
|
||||
next_anchor,
|
||||
num_scanned_rows,
|
||||
})
|
||||
}
|
||||
|
||||
/// Look up the rollout path for a thread id using SQLite.
|
||||
|
||||
Reference in New Issue
Block a user