mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
Make session lookup pagination stable and report listing failures (#51230)
## Why Session activity can move unread threads ahead of a pagination cursor, and timestamp-only cursors can skip threads with equal creation times. Either can prevent a session label lookup from detecting duplicate labels. Database failures must also remain distinguishable from an exhausted listing. ## What changed - Resolve session labels using database-only listings ordered by `CreatedAt`, with thread IDs breaking timestamp ties. Accept unique matches across pages with valid timestamp-and-ID cursors, while retaining ambiguity errors for older cursors that cannot establish uniqueness. - Preserve timestamp index ranges for database continuation queries and timestamp-only cursors for listings that may fall back to the filesystem. - Propagate database-only listing and name-resolution failures. Keep best-effort name enrichment for ordinary listings and retry latest-session lookup with scan-and-repair after a database-only error. - Keep loaded sessions selectable in the resume and fork pickers when a later page fails, stop further pagination, and display `Could not load more sessions`. ## Testing Add regression coverage for creation-time continuation after session activity, cursor query plans, filesystem fallback, database and name-query errors, cross-page label uniqueness, older server cursors, and selection after picker pagination failures. GitOrigin-RevId: 264ab2e69b8ad12d9edaa31fd253cb9805a46a90
This commit is contained in:
@@ -395,7 +395,7 @@ impl RolloutRecorder {
|
||||
default_provider: &str,
|
||||
search_term: Option<&str>,
|
||||
) -> std::io::Result<ThreadsPage> {
|
||||
Self::list_threads_with_db_fallback(
|
||||
let mut page = Self::list_threads_with_db_fallback(
|
||||
state_db_ctx,
|
||||
config,
|
||||
page_size,
|
||||
@@ -410,7 +410,14 @@ impl RolloutRecorder {
|
||||
ThreadListRepairMode::ScanAndRepair,
|
||||
search_term,
|
||||
)
|
||||
.await
|
||||
.await?;
|
||||
// Continuation may fall back to filesystem pagination, which only honors timestamps.
|
||||
if sort_key == ThreadSortKey::CreatedAt
|
||||
&& let Some(cursor) = page.next_cursor.as_mut()
|
||||
{
|
||||
*cursor = Cursor::new(cursor.timestamp());
|
||||
}
|
||||
Ok(page)
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
@@ -460,7 +467,7 @@ impl RolloutRecorder {
|
||||
default_provider: &str,
|
||||
search_term: Option<&str>,
|
||||
) -> std::io::Result<ThreadsPage> {
|
||||
Self::list_threads_with_db_fallback(
|
||||
let mut page = Self::list_threads_with_db_fallback(
|
||||
state_db_ctx,
|
||||
config,
|
||||
page_size,
|
||||
@@ -475,7 +482,14 @@ impl RolloutRecorder {
|
||||
ThreadListRepairMode::ScanAndRepair,
|
||||
search_term,
|
||||
)
|
||||
.await
|
||||
.await?;
|
||||
// Continuation may fall back to filesystem pagination, which only honors timestamps.
|
||||
if sort_key == ThreadSortKey::CreatedAt
|
||||
&& let Some(cursor) = page.next_cursor.as_mut()
|
||||
{
|
||||
*cursor = Cursor::new(cursor.timestamp());
|
||||
}
|
||||
Ok(page)
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
@@ -537,7 +551,7 @@ impl RolloutRecorder {
|
||||
}
|
||||
|
||||
if matches!(repair_mode, ThreadListRepairMode::StateDbOnly) {
|
||||
return Ok(state_db::list_threads_db(
|
||||
return state_db::list_threads_db(
|
||||
state_db_ctx.as_deref(),
|
||||
sqlite,
|
||||
page_size,
|
||||
@@ -555,7 +569,7 @@ impl RolloutRecorder {
|
||||
)
|
||||
.await
|
||||
.map(Into::into)
|
||||
.unwrap_or_default());
|
||||
.ok_or_else(|| std::io::Error::other("failed to list threads from state database"));
|
||||
}
|
||||
|
||||
let listing_has_metadata_filters = !allowed_sources.is_empty()
|
||||
|
||||
@@ -1393,6 +1393,8 @@ async fn list_threads_db_enabled_preserves_metadata_for_missing_rollout_paths()
|
||||
.await
|
||||
.expect("state db upsert should succeed");
|
||||
|
||||
let older_path = write_session_file(home.path(), "2025-01-01T13-00-00", Uuid::from_u128(9014))?;
|
||||
|
||||
let default_provider = config.model_provider_id.clone();
|
||||
let page = RolloutRecorder::list_threads(
|
||||
Some(runtime.clone()),
|
||||
@@ -1420,6 +1422,25 @@ async fn list_threads_db_enabled_preserves_metadata_for_missing_rollout_paths()
|
||||
.await
|
||||
.expect("state db lookup should succeed");
|
||||
assert_eq!(stored_metadata, Some(metadata));
|
||||
let cursor = page.next_cursor.expect("another thread remains");
|
||||
assert_eq!(cursor.thread_id(), None);
|
||||
runtime.close().await;
|
||||
let fallback = RolloutRecorder::list_threads(
|
||||
Some(runtime),
|
||||
&config,
|
||||
/*page_size*/ 1,
|
||||
Some(&cursor),
|
||||
ThreadSortKey::CreatedAt,
|
||||
SortDirection::Desc,
|
||||
&[],
|
||||
/*model_providers*/ None,
|
||||
/*cwd_filters*/ None,
|
||||
default_provider.as_str(),
|
||||
/*search_term*/ None,
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(fallback.items.len(), 1);
|
||||
assert_eq!(fallback.items[0].path, older_path);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -1519,6 +1540,23 @@ async fn list_threads_state_db_only_skips_jsonl_repair_scan() -> std::io::Result
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(repaired_state_db_only_page.items.len(), 1);
|
||||
runtime.close().await;
|
||||
let error = RolloutRecorder::list_threads_from_state_db(
|
||||
Some(runtime),
|
||||
&config,
|
||||
/*page_size*/ 10,
|
||||
/*cursor*/ None,
|
||||
ThreadSortKey::CreatedAt,
|
||||
SortDirection::Desc,
|
||||
&[],
|
||||
/*model_providers*/ None,
|
||||
/*cwd_filters*/ Some(cwd_filters.as_slice()),
|
||||
config.model_provider_id.as_str(),
|
||||
/*search_term*/ None,
|
||||
)
|
||||
.await
|
||||
.expect_err("a failed database query must not look like an exhausted listing");
|
||||
assert_eq!(error.kind(), std::io::ErrorKind::Other);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -706,7 +706,10 @@ pub(crate) fn anchor_from_item(
|
||||
Some(Anchor {
|
||||
ts,
|
||||
id: (include_thread_id_tiebreaker
|
||||
|| matches!(sort_key, SortKey::RecencyAt | SortKey::SectionPosition))
|
||||
|| matches!(
|
||||
sort_key,
|
||||
SortKey::CreatedAt | SortKey::RecencyAt | SortKey::SectionPosition
|
||||
))
|
||||
.then_some(item.id),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -578,7 +578,9 @@ ON CONFLICT(child_thread_id) DO NOTHING
|
||||
},
|
||||
matches!(
|
||||
sort_key,
|
||||
crate::SortKey::RecencyAt | crate::SortKey::SectionPosition
|
||||
crate::SortKey::CreatedAt
|
||||
| crate::SortKey::RecencyAt
|
||||
| crate::SortKey::SectionPosition
|
||||
),
|
||||
);
|
||||
push_thread_order_and_limit(
|
||||
@@ -588,7 +590,9 @@ ON CONFLICT(child_thread_id) DO NOTHING
|
||||
OrderByIndex::Enabled,
|
||||
matches!(
|
||||
sort_key,
|
||||
crate::SortKey::RecencyAt | crate::SortKey::SectionPosition
|
||||
crate::SortKey::CreatedAt
|
||||
| crate::SortKey::RecencyAt
|
||||
| crate::SortKey::SectionPosition
|
||||
),
|
||||
limit,
|
||||
);
|
||||
@@ -1267,7 +1271,7 @@ WITH RECURSIVE subtree(child_thread_id, parent_thread_id) AS (
|
||||
let include_thread_id_tiebreaker = relation_filter.is_some()
|
||||
|| matches!(
|
||||
filters.sort_key,
|
||||
SortKey::RecencyAt | SortKey::SectionPosition
|
||||
SortKey::CreatedAt | SortKey::RecencyAt | SortKey::SectionPosition
|
||||
);
|
||||
push_thread_filters_with_preview(
|
||||
builder,
|
||||
@@ -1509,6 +1513,11 @@ fn push_thread_filters_with_preview<'a>(
|
||||
SortDirection::Asc => ">",
|
||||
SortDirection::Desc => "<",
|
||||
};
|
||||
if include_thread_id_tiebreaker && anchor.id.is_some() {
|
||||
// Keep the timestamp index range despite the UUID tie-breaker's OR predicate.
|
||||
builder.push(format!(" AND {column} {operator}= "));
|
||||
builder.push_bind(anchor_ts);
|
||||
}
|
||||
builder.push(" AND (");
|
||||
builder.push(column);
|
||||
builder.push(" ");
|
||||
@@ -2250,6 +2259,8 @@ mod tests {
|
||||
(other_id, other_cwd, 1_700_000_500),
|
||||
] {
|
||||
let mut metadata = test_thread_metadata(&codex_home, thread_id, cwd);
|
||||
metadata.created_at =
|
||||
DateTime::<Utc>::from_timestamp(1_700_000_300, 0).expect("valid timestamp");
|
||||
metadata.updated_at =
|
||||
DateTime::<Utc>::from_timestamp(updated_at, 0).expect("valid timestamp");
|
||||
runtime
|
||||
@@ -2340,6 +2351,43 @@ mod tests {
|
||||
.expect("list with empty cwd filters should succeed");
|
||||
|
||||
assert_eq!(page.items, Vec::new());
|
||||
|
||||
let anchor = Anchor {
|
||||
ts: DateTime::<Utc>::from_timestamp(1_700_000_300, 0).expect("valid timestamp"),
|
||||
id: Some(second_id),
|
||||
};
|
||||
// Activity must not move the unread thread ahead of the creation cursor.
|
||||
runtime
|
||||
.touch_thread_updated_at(first_id, Utc::now())
|
||||
.await
|
||||
.unwrap();
|
||||
runtime
|
||||
.touch_thread_recency_at(first_id, Utc::now())
|
||||
.await
|
||||
.unwrap();
|
||||
let page = runtime
|
||||
.list_threads(
|
||||
/*page_size*/ 1,
|
||||
ThreadFilterOptions {
|
||||
archived_only: false,
|
||||
allowed_sources: &[],
|
||||
model_providers: None,
|
||||
cwd_filters: Some(cwd_filters.as_slice()),
|
||||
section: None,
|
||||
project_id: None,
|
||||
anchor: Some(&anchor),
|
||||
sort_key: SortKey::CreatedAt,
|
||||
sort_direction: SortDirection::Desc,
|
||||
search_term: None,
|
||||
},
|
||||
)
|
||||
.await
|
||||
.expect("creation-time continuation should succeed");
|
||||
assert_eq!(
|
||||
page.items.iter().map(|item| item.id).collect::<Vec<_>>(),
|
||||
vec![first_id]
|
||||
);
|
||||
assert_eq!(page.next_anchor, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
@@ -2359,7 +2407,7 @@ mod tests {
|
||||
];
|
||||
let anchor = Anchor {
|
||||
ts: DateTime::<Utc>::from_timestamp(1_700_000_000, 0).expect("valid timestamp"),
|
||||
id: None,
|
||||
id: Some(ThreadId::new()),
|
||||
};
|
||||
for (sort_key, visible_index, cwd_index) in [
|
||||
(
|
||||
@@ -2380,6 +2428,7 @@ mod tests {
|
||||
] {
|
||||
for (cwd_filters, anchor, expected_index, expect_temp_sort) in [
|
||||
(None, None, visible_index, false),
|
||||
(None, Some(&anchor), visible_index, false),
|
||||
(Some(&cwd_filters[..1]), None, cwd_index, false),
|
||||
(
|
||||
Some(&cwd_filters[..]),
|
||||
@@ -2422,11 +2471,35 @@ mod tests {
|
||||
.any(|detail| detail.contains(expected_index)),
|
||||
"query plan did not use {expected_index}: {plan_details:?}"
|
||||
);
|
||||
if anchor.is_some() && cwd_filters.is_none() {
|
||||
let timestamp_column = match sort_key {
|
||||
SortKey::CreatedAt => "created_at_ms",
|
||||
SortKey::UpdatedAt => "updated_at_ms",
|
||||
SortKey::RecencyAt => "recency_at_ms",
|
||||
SortKey::SectionPosition => unreachable!(),
|
||||
};
|
||||
assert!(
|
||||
plan_details
|
||||
.iter()
|
||||
.any(|detail| { detail.contains(&format!("{timestamp_column}<?")) }),
|
||||
"cursor query lost its timestamp index range: {plan_details:?}"
|
||||
);
|
||||
}
|
||||
// The visible creation-time index covers timestamps, so only ties need sorting.
|
||||
let expect_tie_sort = sort_key == SortKey::CreatedAt && cwd_filters.is_none();
|
||||
if expect_tie_sort {
|
||||
assert!(
|
||||
plan_details.iter().any(|detail| {
|
||||
detail.contains("TEMP B-TREE FOR LAST TERM OF ORDER BY")
|
||||
}),
|
||||
"unexpected tie sorting plan: {plan_details:?}"
|
||||
);
|
||||
}
|
||||
assert_eq!(
|
||||
plan_details
|
||||
.iter()
|
||||
.any(|detail| detail.contains("TEMP B-TREE")),
|
||||
expect_temp_sort,
|
||||
expect_temp_sort || expect_tie_sort,
|
||||
"unexpected sorting plan: {plan_details:?}"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -238,25 +238,40 @@ pub(super) async fn resolve_thread_section_metadata(
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
/// DB-only listings require complete names; ordinary listings retain best-effort enrichment.
|
||||
pub(super) async fn resolve_thread_names(
|
||||
store: &LocalThreadStore,
|
||||
thread_history_modes: &HashMap<ThreadId, ThreadHistoryMode>,
|
||||
) -> HashMap<ThreadId, String> {
|
||||
use_state_db_only: bool,
|
||||
) -> ThreadStoreResult<HashMap<ThreadId, String>> {
|
||||
let legacy_thread_ids = thread_history_modes
|
||||
.iter()
|
||||
.filter_map(|(&thread_id, &history_mode)| {
|
||||
(history_mode == ThreadHistoryMode::Legacy).then_some(thread_id)
|
||||
})
|
||||
.collect::<HashSet<_>>();
|
||||
let mut names = find_thread_names_by_ids(store.config.codex_home.as_path(), &legacy_thread_ids)
|
||||
.await
|
||||
.unwrap_or_default();
|
||||
let mut names =
|
||||
match find_thread_names_by_ids(store.config.codex_home.as_path(), &legacy_thread_ids).await
|
||||
{
|
||||
Ok(names) => names,
|
||||
Err(err) if use_state_db_only => {
|
||||
return Err(ThreadStoreError::Internal {
|
||||
message: format!("failed to read indexed thread names: {err}"),
|
||||
});
|
||||
}
|
||||
Err(_) => HashMap::new(),
|
||||
};
|
||||
if let Some(state_db_ctx) = store.state_db().await {
|
||||
let thread_ids = thread_history_modes.keys().copied().collect::<Vec<_>>();
|
||||
let metadata_by_id = state_db_ctx
|
||||
.get_threads(&thread_ids)
|
||||
.await
|
||||
.unwrap_or_default();
|
||||
let metadata_by_id = match state_db_ctx.get_threads(&thread_ids).await {
|
||||
Ok(metadata) => metadata,
|
||||
Err(err) if use_state_db_only => {
|
||||
return Err(ThreadStoreError::Internal {
|
||||
message: format!("failed to read thread names from state DB: {err}"),
|
||||
});
|
||||
}
|
||||
Err(_) => return Ok(names),
|
||||
};
|
||||
for (&thread_id, &history_mode) in thread_history_modes {
|
||||
let Some(metadata) = metadata_by_id.get(&thread_id) else {
|
||||
continue;
|
||||
@@ -275,7 +290,7 @@ pub(super) async fn resolve_thread_names(
|
||||
}
|
||||
}
|
||||
}
|
||||
names
|
||||
Ok(names)
|
||||
}
|
||||
|
||||
/// Identifies the derived Guardian label, which must yield to legacy indexed names.
|
||||
|
||||
@@ -87,7 +87,8 @@ pub(super) async fn list_threads(
|
||||
.iter()
|
||||
.map(|thread| (thread.thread_id, thread.history_mode))
|
||||
.collect::<HashMap<_, _>>();
|
||||
let names = resolve_thread_names(store, &thread_history_modes).await;
|
||||
let names =
|
||||
resolve_thread_names(store, &thread_history_modes, params.use_state_db_only).await?;
|
||||
for thread in &mut items {
|
||||
if let Some(name) = names.get(&thread.thread_id).cloned() {
|
||||
set_thread_name(thread, name);
|
||||
@@ -631,27 +632,34 @@ mod tests {
|
||||
async fn list_threads_returns_local_rollout_summary() {
|
||||
let home = TempDir::new().expect("temp dir");
|
||||
let config = test_config(home.path());
|
||||
let store = LocalThreadStore::new(config, /*state_db*/ None);
|
||||
let runtime = codex_state::StateRuntime::init(
|
||||
config.sqlite.clone(),
|
||||
config.default_model_provider_id.clone(),
|
||||
)
|
||||
.await
|
||||
.expect("initialize state DB");
|
||||
let store = LocalThreadStore::new(config, Some(runtime.clone()));
|
||||
let uuid = Uuid::from_u128(101);
|
||||
let path =
|
||||
write_session_file(home.path(), "2025-01-03T12-00-00", uuid).expect("session file");
|
||||
|
||||
let params = ListThreadsParams {
|
||||
page_size: 10,
|
||||
cursor: None,
|
||||
sort_key: ThreadSortKey::CreatedAt,
|
||||
sort_direction: SortDirection::Desc,
|
||||
allowed_sources: vec![SessionSource::Cli],
|
||||
model_providers: Some(vec!["test-provider".to_string()]),
|
||||
cwd_filters: None,
|
||||
section: None,
|
||||
project_id: None,
|
||||
archived: false,
|
||||
search_term: None,
|
||||
relation_filter: None,
|
||||
use_state_db_only: false,
|
||||
};
|
||||
let page = store
|
||||
.list_threads(ListThreadsParams {
|
||||
page_size: 10,
|
||||
cursor: None,
|
||||
sort_key: ThreadSortKey::CreatedAt,
|
||||
sort_direction: SortDirection::Desc,
|
||||
allowed_sources: vec![SessionSource::Cli],
|
||||
model_providers: Some(vec!["test-provider".to_string()]),
|
||||
cwd_filters: None,
|
||||
section: None,
|
||||
project_id: None,
|
||||
archived: false,
|
||||
search_term: None,
|
||||
relation_filter: None,
|
||||
use_state_db_only: false,
|
||||
})
|
||||
.list_threads(params.clone())
|
||||
.await
|
||||
.expect("thread listing");
|
||||
|
||||
@@ -668,6 +676,22 @@ mod tests {
|
||||
assert_eq!(page.items[0].model_provider, "test-provider");
|
||||
assert_eq!(page.items[0].cli_version, "test_version");
|
||||
assert_eq!(page.items[0].source, SessionSource::Cli);
|
||||
|
||||
// Name query errors must abort DB-only lookup while ordinary listing keeps its fallback.
|
||||
let history_modes = HashMap::from([(thread_id, ThreadHistoryMode::Legacy)]);
|
||||
codex_rollout::append_thread_name(home.path(), thread_id, "indexed name")
|
||||
.await
|
||||
.expect("write indexed name");
|
||||
runtime.close().await;
|
||||
resolve_thread_names(&store, &history_modes, /*use_state_db_only*/ true)
|
||||
.await
|
||||
.expect_err("name query errors must be reported");
|
||||
let fallback = store
|
||||
.list_threads(params)
|
||||
.await
|
||||
.expect("ordinary listing must preserve filesystem fallback");
|
||||
assert_eq!(fallback.items[0].thread_id, thread_id);
|
||||
assert_eq!(fallback.items[0].name.as_deref(), Some("indexed name"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -248,7 +248,13 @@ async fn set_thread_search_result_names(
|
||||
.iter()
|
||||
.map(|item| (item.thread.thread_id, item.thread.history_mode))
|
||||
.collect::<HashMap<_, _>>();
|
||||
let names = resolve_thread_names(store, &thread_history_modes).await;
|
||||
let names = resolve_thread_names(
|
||||
store,
|
||||
&thread_history_modes,
|
||||
/*use_state_db_only*/ false,
|
||||
)
|
||||
.await
|
||||
.unwrap_or_default();
|
||||
for item in items {
|
||||
if let Some(name) = names.get(&item.thread.thread_id).cloned() {
|
||||
set_thread_name(&mut item.thread, name);
|
||||
|
||||
@@ -846,7 +846,11 @@ async fn lookup_latest_session_target_with_app_server(
|
||||
include_non_interactive,
|
||||
lookup_mode,
|
||||
))
|
||||
.await?;
|
||||
.await;
|
||||
let response = match response {
|
||||
Err(_) if lookup_mode == LatestSessionLookupMode::StateDbOnly => continue,
|
||||
response => response?,
|
||||
};
|
||||
let target = response
|
||||
.data
|
||||
.into_iter()
|
||||
|
||||
@@ -57,22 +57,18 @@ pub(super) async fn lookup(
|
||||
return Ok(None);
|
||||
}
|
||||
let mut matched: Option<Thread> = None;
|
||||
let mut paginated = false;
|
||||
let mut unverified_pagination = false;
|
||||
for collection in collections {
|
||||
for source_kinds in source_kind_filters {
|
||||
let mut cursor = None;
|
||||
let sort_key = if app_server.uses_embedded_app_server() {
|
||||
ThreadSortKey::RecencyAt
|
||||
} else {
|
||||
ThreadSortKey::UpdatedAt
|
||||
};
|
||||
loop {
|
||||
let response = app_server
|
||||
.thread_list(ThreadListParams {
|
||||
originators: None,
|
||||
cursor,
|
||||
limit: Some(100),
|
||||
sort_key: Some(sort_key),
|
||||
// Activity must not move an unread thread ahead of the cursor.
|
||||
sort_key: Some(ThreadSortKey::CreatedAt),
|
||||
sort_direction: None,
|
||||
model_providers: model_provider.map(|provider| vec![provider.to_string()]),
|
||||
source_kinds: Some(source_kinds.clone()),
|
||||
@@ -82,12 +78,14 @@ pub(super) async fn lookup(
|
||||
parent_thread_id: None,
|
||||
ancestor_thread_id: None,
|
||||
cwd: None,
|
||||
use_state_db_only: false,
|
||||
use_state_db_only: true,
|
||||
search_term: None,
|
||||
})
|
||||
.await
|
||||
.wrap_err("failed to list sessions while resolving session label")?;
|
||||
paginated |= response.next_cursor.is_some();
|
||||
unverified_pagination |= response.next_cursor.as_deref().is_some_and(|cursor| {
|
||||
!cursor.contains('|') || codex_rollout::parse_cursor(cursor).is_none()
|
||||
});
|
||||
for thread in response.data {
|
||||
if display_label(&thread) != name {
|
||||
continue;
|
||||
@@ -165,7 +163,7 @@ pub(super) async fn lookup(
|
||||
}
|
||||
// Older server cursors can skip equal timestamps at a page boundary.
|
||||
if let Some(thread) = matched.as_ref()
|
||||
&& paginated
|
||||
&& unverified_pagination
|
||||
{
|
||||
return Err(AmbiguousSessionName::Paginated(thread.id.clone()).into());
|
||||
}
|
||||
|
||||
@@ -280,7 +280,7 @@ async fn rejects_duplicate_labels_across_server_pages() -> color_eyre::Result<()
|
||||
let runtime = state_runtime(&config).await?;
|
||||
let mut relevant_ids = Vec::new();
|
||||
for index in 0..102 {
|
||||
let thread_id = ThreadId::new();
|
||||
let thread_id = ThreadId::from_string(&format!("00000000-0000-0000-0000-{index:012}"))?;
|
||||
if matches!(index, 0 | 1 | 101) {
|
||||
relevant_ids.push(thread_id);
|
||||
}
|
||||
@@ -319,7 +319,7 @@ async fn rejects_duplicate_labels_across_server_pages() -> color_eyre::Result<()
|
||||
)
|
||||
.await
|
||||
.expect_err("duplicate labels should require an ID");
|
||||
let unverified = lookup(
|
||||
let unique = lookup(
|
||||
&mut app_server,
|
||||
config.codex_home.as_path(),
|
||||
"other-1",
|
||||
@@ -327,8 +327,7 @@ async fn rejects_duplicate_labels_across_server_pages() -> color_eyre::Result<()
|
||||
&[resume_source_kinds(/*include_non_interactive*/ false)],
|
||||
Some(&config.model_provider_id),
|
||||
)
|
||||
.await
|
||||
.expect_err("paginated listings cannot prove uniqueness on older servers");
|
||||
.await?;
|
||||
app_server.shutdown().await?;
|
||||
assert_eq!(
|
||||
error.to_string(),
|
||||
@@ -338,11 +337,8 @@ async fn rejects_duplicate_labels_across_server_pages() -> color_eyre::Result<()
|
||||
)
|
||||
);
|
||||
assert_eq!(
|
||||
unverified.to_string(),
|
||||
format!(
|
||||
"Cannot verify a unique session label across server pages; matching session UUID: {}. Use it only if this is the session you want.",
|
||||
relevant_ids[1]
|
||||
)
|
||||
unique.map(|thread| thread.id),
|
||||
Some(relevant_ids[1].to_string())
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
@@ -447,10 +443,25 @@ async fn uses_listed_thread_when_older_server_cannot_read_it() -> color_eyre::Re
|
||||
.await?;
|
||||
embedded.shutdown().await?;
|
||||
|
||||
for mode in [ThreadParamsMode::Remote, ThreadParamsMode::Embedded] {
|
||||
for (mode, next_cursor) in [
|
||||
(ThreadParamsMode::Remote, None),
|
||||
(ThreadParamsMode::Embedded, None),
|
||||
(
|
||||
ThreadParamsMode::Remote,
|
||||
Some("2025-02-01T10:00:00Z".to_string()),
|
||||
),
|
||||
(
|
||||
ThreadParamsMode::Remote,
|
||||
Some(format!("2025-02-01T10:00:00Z|{thread_id}")),
|
||||
),
|
||||
] {
|
||||
let unverified_pagination = next_cursor
|
||||
.as_ref()
|
||||
.is_some_and(|cursor| !cursor.contains('|'));
|
||||
let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await?;
|
||||
let url = format!("ws://{}", listener.local_addr()?);
|
||||
let listed_for_server = listed.clone();
|
||||
let paginated = next_cursor.is_some();
|
||||
let server = tokio::spawn(async move {
|
||||
let (stream, _) = listener.accept().await.unwrap();
|
||||
let mut socket = tokio_tungstenite::accept_async(stream).await.unwrap();
|
||||
@@ -458,14 +469,32 @@ async fn uses_listed_thread_when_older_server_cannot_read_it() -> color_eyre::Re
|
||||
let JSONRPCMessage::Request(request) = serde_json::from_str(&text).unwrap() else {
|
||||
continue;
|
||||
};
|
||||
let last_page = request.method == "thread/list"
|
||||
&& request
|
||||
.params
|
||||
.as_ref()
|
||||
.is_some_and(|params| params["cursor"].is_string());
|
||||
let response = match request.method.as_str() {
|
||||
"initialize" => {
|
||||
json!({"id": request.id, "result": {"userAgent": "legacy-test"}})
|
||||
}
|
||||
"thread/list" => json!({
|
||||
"id": request.id,
|
||||
"result": {"data": [listed_for_server.clone()], "nextCursor": null}
|
||||
}),
|
||||
"thread/list" => {
|
||||
assert_eq!(
|
||||
request.params.as_ref().unwrap()["sortKey"],
|
||||
json!(codex_app_server_protocol::ThreadSortKey::CreatedAt)
|
||||
);
|
||||
assert_eq!(
|
||||
request.params.as_ref().unwrap()["useStateDbOnly"],
|
||||
json!(true)
|
||||
);
|
||||
let data = if last_page {
|
||||
Vec::new()
|
||||
} else {
|
||||
vec![listed_for_server.clone()]
|
||||
};
|
||||
let cursor = if last_page { None } else { next_cursor.clone() };
|
||||
json!({"id": request.id, "result": {"data": data, "nextCursor": cursor}})
|
||||
}
|
||||
"thread/read" => json!({
|
||||
"id": request.id,
|
||||
"error": {
|
||||
@@ -479,7 +508,7 @@ async fn uses_listed_thread_when_older_server_cannot_read_it() -> color_eyre::Re
|
||||
.send(Message::Text(response.to_string().into()))
|
||||
.await
|
||||
.unwrap();
|
||||
if request.method == "thread/read" {
|
||||
if last_page || (!paginated && request.method == "thread/read") {
|
||||
break;
|
||||
}
|
||||
}
|
||||
@@ -509,8 +538,19 @@ async fn uses_listed_thread_when_older_server_cannot_read_it() -> color_eyre::Re
|
||||
&[resume_source_kinds(/*include_non_interactive*/ false)],
|
||||
/*model_provider*/ None,
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(found, Some(listed.clone()));
|
||||
.await;
|
||||
if unverified_pagination {
|
||||
assert_eq!(
|
||||
found
|
||||
.expect_err("timestamp-only cursors cannot prove uniqueness")
|
||||
.to_string(),
|
||||
format!(
|
||||
"Cannot verify a unique session label across server pages; matching session UUID: {thread_id}. Use it only if this is the session you want."
|
||||
)
|
||||
);
|
||||
} else {
|
||||
assert_eq!(found?, Some(listed.clone()));
|
||||
}
|
||||
server.await?;
|
||||
}
|
||||
Ok(())
|
||||
|
||||
@@ -80,6 +80,10 @@ mod page_loading;
|
||||
#[path = "resume_picker_color_tests.rs"]
|
||||
mod color_tests;
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "resume_picker_pagination_error_tests.rs"]
|
||||
mod pagination_error_tests;
|
||||
|
||||
use page_loading::PageCwdFilter;
|
||||
use page_loading::PageLoadMode;
|
||||
use page_loading::PaginationState;
|
||||
@@ -1526,7 +1530,19 @@ impl PickerState {
|
||||
}));
|
||||
return Ok(None);
|
||||
}
|
||||
let page = page.map_err(color_eyre::Report::from)?;
|
||||
let page = match page {
|
||||
Ok(page) => page,
|
||||
Err(_) if !self.all_rows.is_empty() => {
|
||||
self.pagination.next_cursor = None;
|
||||
self.pending_page_down_target = None;
|
||||
self.frozen_footer_percent = None;
|
||||
self.search_state = SearchState::Idle;
|
||||
self.inline_error = Some("Could not load more sessions".to_string());
|
||||
self.request_frame();
|
||||
return Ok(None);
|
||||
}
|
||||
Err(err) => return Err(err.into()),
|
||||
};
|
||||
self.ingest_page(page);
|
||||
self.complete_pending_page_down();
|
||||
let completed_token = pending.search_token.or(search_token);
|
||||
|
||||
@@ -0,0 +1,101 @@
|
||||
//! Keeps loaded sessions selectable when background pagination fails.
|
||||
|
||||
use super::*;
|
||||
use insta::assert_snapshot;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::sync::Mutex;
|
||||
|
||||
#[tokio::test]
|
||||
async fn later_page_error_preserves_loaded_sessions() {
|
||||
for action in [SessionPickerAction::Resume, SessionPickerAction::Fork] {
|
||||
for query in ["", "unloaded session"] {
|
||||
let requests = Arc::new(Mutex::new(Vec::new()));
|
||||
let request_sink = Arc::clone(&requests);
|
||||
let loader: PickerLoader = Arc::new(move |request| {
|
||||
if let PickerLoadRequest::Page(request) = request {
|
||||
request_sink.lock().unwrap().push(request);
|
||||
}
|
||||
});
|
||||
let mut state = PickerState::new(
|
||||
FrameRequester::test_dummy(),
|
||||
loader,
|
||||
ProviderFilter::Any,
|
||||
/*show_all*/ true,
|
||||
/*filter_cwd*/ None,
|
||||
action,
|
||||
);
|
||||
state.initial_page_mode = PageLoadMode::StateDbOnly;
|
||||
state.start_initial_load();
|
||||
let request = requests.lock().unwrap()[0].clone();
|
||||
let thread_id = ThreadId::from_string("019dabc1-0ef5-7431-b81c-03037f51f62c").unwrap();
|
||||
state
|
||||
.handle_background_event(BackgroundEvent::Page {
|
||||
request_token: request.request_token,
|
||||
search_token: request.search_token,
|
||||
page: Ok(PickerPage {
|
||||
rows: vec![Row {
|
||||
path: None,
|
||||
preview: "Loaded session".to_string(),
|
||||
thread_id: Some(thread_id),
|
||||
thread_name: None,
|
||||
created_at: None,
|
||||
updated_at: None,
|
||||
cwd: None,
|
||||
git_branch: None,
|
||||
}],
|
||||
history_modes: HashMap::new(),
|
||||
next_cursor: Some(PageCursor::AppServer("next-page".to_string())),
|
||||
num_scanned_files: 1,
|
||||
reached_scan_cap: false,
|
||||
}),
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
if query.is_empty() {
|
||||
state
|
||||
.handle_key(KeyEvent::from(KeyCode::PageDown))
|
||||
.await
|
||||
.unwrap();
|
||||
} else {
|
||||
state.set_query(query.to_string());
|
||||
}
|
||||
let request = requests.lock().unwrap()[1].clone();
|
||||
state
|
||||
.handle_background_event(BackgroundEvent::Page {
|
||||
request_token: request.request_token,
|
||||
search_token: request.search_token,
|
||||
page: Err(std::io::Error::other(
|
||||
"failed to list threads from state database",
|
||||
)),
|
||||
})
|
||||
.await
|
||||
.expect("later page errors must not close the picker");
|
||||
assert_eq!(state.all_rows.len(), 1);
|
||||
assert!(!state.pagination.is_loading());
|
||||
assert!(!state.search_state.is_active());
|
||||
assert!(state.pending_page_down_target.is_none());
|
||||
assert!(state.frozen_footer_percent.is_none());
|
||||
assert!(state.pagination.next_cursor.is_none());
|
||||
state.set_query(String::new());
|
||||
state.maybe_load_more_for_scroll();
|
||||
assert_eq!(requests.lock().unwrap().len(), 2);
|
||||
|
||||
if matches!(action, SessionPickerAction::Resume) && query.is_empty() {
|
||||
assert_snapshot!(
|
||||
"resume_picker_pagination_error",
|
||||
search_line(&state, /*width*/ 80).to_string()
|
||||
);
|
||||
}
|
||||
let selection = state
|
||||
.handle_key(KeyEvent::from(KeyCode::Enter))
|
||||
.await
|
||||
.unwrap();
|
||||
let target = match (action, selection) {
|
||||
(SessionPickerAction::Resume, Some(SessionSelection::Resume(target)))
|
||||
| (SessionPickerAction::Fork, Some(SessionSelection::Fork(target))) => target,
|
||||
other => panic!("loaded session was not selectable: {other:?}"),
|
||||
};
|
||||
assert_eq!(target.thread_id, thread_id);
|
||||
}
|
||||
}
|
||||
}
|
||||
+5
@@ -0,0 +1,5 @@
|
||||
---
|
||||
source: tui/src/resume_picker_pagination_error_tests.rs
|
||||
expression: "search_line(&state, 80).to_string()"
|
||||
---
|
||||
Could not load more sessions
|
||||
Reference in New Issue
Block a user